| Message ID | 20170414125919.25771-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote: > Allow a caller of power_supply_am_i_supplied to differentiate between > there not being any suppliers, vs no suppliers being online by returning > -ENODEV if there are no suppliers matching supplied_to / supplied_from. > This is missing important piece of information - why you need to differentiate that? What is the difference for you between no supplies at all and not-being-supplied? > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/power/supply/power_supply_core.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c > index 1e0960b..13a39da 100644 > --- a/drivers/power/supply/power_supply_core.c > +++ b/drivers/power/supply/power_supply_core.c > @@ -280,13 +280,19 @@ static inline int power_supply_check_supplies(struct power_supply *psy) > } > #endif > > -static int __power_supply_am_i_supplied(struct device *dev, void *data) > +struct am_i_supplied_data { How about a prefix, e.g.: "psy_am_i_supplied_data"? Best regards, Krzysztof
Hi, On 14-04-17 15:56, Krzysztof Kozlowski wrote: > On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote: >> Allow a caller of power_supply_am_i_supplied to differentiate between >> there not being any suppliers, vs no suppliers being online by returning >> -ENODEV if there are no suppliers matching supplied_to / supplied_from. >> > > This is missing important piece of information - why you need to > differentiate that? What is the difference for you between no supplies > at all and not-being-supplied? Thank you for the quick response. I think it is sensible to assume that the hardware actually always has a way of charging the battery so where you say "no supplies at all" in reality what would be the case is : "no power_supply-drivers registered / bound for any supplies at all". At which point we can not determine in many fuel-gauge drivers (which are the only user of power_supply_am_i_supplied) if we're charging or discharging. Here is the code for the STATUS property I'm adding to the max17042 driver in a later commit in the set: static int max17042_get_status(struct max17042_chip *chip, int *status) { int ret, charge_full, charge_now; ret = power_supply_am_i_supplied(chip->battery); if (ret < 0) { *status = POWER_SUPPLY_STATUS_UNKNOWN; return 0; } if (ret == 0) { *status = POWER_SUPPLY_STATUS_DISCHARGING; return 0; } ... } This is where the -ENODEV comes in to make it properly return POWER_SUPPLY_STATUS_UNKNOWN instead of always returning POWER_SUPPLY_STATUS_DISCHARGING even if that may be untrue. Note that I've since found out that the only in tree user of the max17042 driver is arch/arm/boot/dts/exynos4412-trats2.dts and the last patch in my makes the max77693 charger driver properly set supplied_to so that power_supply_am_i_supplied() will work correctly for the max17047 / max77693 combo on that board, which sort of renders this patch unnecessary. I still think this patch is a good idea, but it can be dropped if other people disagree. Regards, Hans > > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/power/supply/power_supply_core.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c >> index 1e0960b..13a39da 100644 >> --- a/drivers/power/supply/power_supply_core.c >> +++ b/drivers/power/supply/power_supply_core.c >> @@ -280,13 +280,19 @@ static inline int power_supply_check_supplies(struct power_supply *psy) >> } >> #endif >> >> -static int __power_supply_am_i_supplied(struct device *dev, void *data) >> +struct am_i_supplied_data { > > How about a prefix, e.g.: "psy_am_i_supplied_data"? > > Best regards, > Krzysztof >
On Fri, Apr 14, 2017 at 04:07:22PM +0200, Hans de Goede wrote: > Hi, > > On 14-04-17 15:56, Krzysztof Kozlowski wrote: > > On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote: > > > Allow a caller of power_supply_am_i_supplied to differentiate between > > > there not being any suppliers, vs no suppliers being online by returning > > > -ENODEV if there are no suppliers matching supplied_to / supplied_from. > > > > > > > This is missing important piece of information - why you need to > > differentiate that? What is the difference for you between no supplies > > at all and not-being-supplied? > > Thank you for the quick response. > > I think it is sensible to assume that the hardware actually always has a > way of charging the battery so where you say "no supplies at all" in > reality what would be the case is : "no power_supply-drivers > registered / bound for any supplies at all". At which point we can not > determine in many fuel-gauge drivers (which are the only user of > power_supply_am_i_supplied) if we're charging or discharging. > > Here is the code for the STATUS property I'm adding to the max17042 > driver in a later commit in the set: > > static int max17042_get_status(struct max17042_chip *chip, int *status) > { > int ret, charge_full, charge_now; > > ret = power_supply_am_i_supplied(chip->battery); > if (ret < 0) { > *status = POWER_SUPPLY_STATUS_UNKNOWN; > return 0; > } > if (ret == 0) { > *status = POWER_SUPPLY_STATUS_DISCHARGING; > return 0; > } > > ... > } The explanation makes sense. It should be put in the commit msg as rationale for this patch. Best regards, Krzysztof > > This is where the -ENODEV comes in to make it properly return > POWER_SUPPLY_STATUS_UNKNOWN instead of always returning > POWER_SUPPLY_STATUS_DISCHARGING even if that may be untrue. > > Note that I've since found out that the only in tree user of the > max17042 driver is arch/arm/boot/dts/exynos4412-trats2.dts and > the last patch in my makes the max77693 charger driver properly > set supplied_to so that power_supply_am_i_supplied() will work > correctly for the max17047 / max77693 combo on that board, which > sort of renders this patch unnecessary. I still think this patch > is a good idea, but it can be dropped if other people disagree. > > Regards, > > Hans >
Hi, On 14-04-17 16:31, Krzysztof Kozlowski wrote: > On Fri, Apr 14, 2017 at 04:07:22PM +0200, Hans de Goede wrote: >> Hi, >> >> On 14-04-17 15:56, Krzysztof Kozlowski wrote: >>> On Fri, Apr 14, 2017 at 02:59:07PM +0200, Hans de Goede wrote: >>>> Allow a caller of power_supply_am_i_supplied to differentiate between >>>> there not being any suppliers, vs no suppliers being online by returning >>>> -ENODEV if there are no suppliers matching supplied_to / supplied_from. >>>> >>> >>> This is missing important piece of information - why you need to >>> differentiate that? What is the difference for you between no supplies >>> at all and not-being-supplied? >> >> Thank you for the quick response. >> >> I think it is sensible to assume that the hardware actually always has a >> way of charging the battery so where you say "no supplies at all" in >> reality what would be the case is : "no power_supply-drivers >> registered / bound for any supplies at all". At which point we can not >> determine in many fuel-gauge drivers (which are the only user of >> power_supply_am_i_supplied) if we're charging or discharging. >> >> Here is the code for the STATUS property I'm adding to the max17042 >> driver in a later commit in the set: >> >> static int max17042_get_status(struct max17042_chip *chip, int *status) >> { >> int ret, charge_full, charge_now; >> >> ret = power_supply_am_i_supplied(chip->battery); >> if (ret < 0) { >> *status = POWER_SUPPLY_STATUS_UNKNOWN; >> return 0; >> } >> if (ret == 0) { >> *status = POWER_SUPPLY_STATUS_DISCHARGING; >> return 0; >> } >> >> ... >> } > > The explanation makes sense. It should be put in the commit msg as > rationale for this patch. Ok, I've added a similar text to the commit msg for v2, I will wait until you've had a chance to respond to the rest of the set before sending out a v2. Regards, Hans
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c index 1e0960b..13a39da 100644 --- a/drivers/power/supply/power_supply_core.c +++ b/drivers/power/supply/power_supply_core.c @@ -280,13 +280,19 @@ static inline int power_supply_check_supplies(struct power_supply *psy) } #endif -static int __power_supply_am_i_supplied(struct device *dev, void *data) +struct am_i_supplied_data { + struct power_supply *psy; + unsigned int count; +}; + +static int __power_supply_am_i_supplied(struct device *dev, void *_data) { union power_supply_propval ret = {0,}; - struct power_supply *psy = data; struct power_supply *epsy = dev_get_drvdata(dev); + struct am_i_supplied_data *data = _data; - if (__power_supply_is_supplied_by(epsy, psy)) + data->count++; + if (__power_supply_is_supplied_by(epsy, data->psy)) if (!epsy->desc->get_property(epsy, POWER_SUPPLY_PROP_ONLINE, &ret)) return ret.intval; @@ -296,12 +302,16 @@ static int __power_supply_am_i_supplied(struct device *dev, void *data) int power_supply_am_i_supplied(struct power_supply *psy) { + struct am_i_supplied_data data = { psy, 0 }; int error; - error = class_for_each_device(power_supply_class, NULL, psy, + error = class_for_each_device(power_supply_class, NULL, &data, __power_supply_am_i_supplied); - dev_dbg(&psy->dev, "%s %d\n", __func__, error); + dev_dbg(&psy->dev, "%s count %u err %d\n", __func__, data.count, error); + + if (data.count == 0) + return -ENODEV; return error; }
Allow a caller of power_supply_am_i_supplied to differentiate between there not being any suppliers, vs no suppliers being online by returning -ENODEV if there are no suppliers matching supplied_to / supplied_from. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/power_supply_core.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)