| Message ID | 20221009191839.102686-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] power: supply: bq25890: Add CC voltage to ADC properties | expand |
On Sun, Oct 09, 2022 at 09:18:39PM +0200, Marek Vasut wrote: > The POWER_SUPPLY_PROP_CURRENT_NOW property represents, as far as I can tell, > the immediate power supply input current, however, this driver reports the > immediate battery charge current using that property, i.e. content of REG12 > ICHGR -- ADC conversion of Charge Current (IBAT). Replace the property with > POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, which as far as I can tell should > be used to report immediate battery charge voltage. > > This also aligns the behavior of POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT > with POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, as the former reports IBAT > and the later reports VBAT. I believe this is wrong: CC_CURRENT would be the constant-current charge phase current limit (ICHG in REG04). Documentation/power/power_supply_class.rst mentions that: CONSTANT_CHARGE_CURRENT constant charge current programmed by charger. and: Postfixes: _NOW momentary/instantaneous values. Best Regards Michał Mirosław
On 10/10/22 00:58, Michał Mirosław wrote: > On Sun, Oct 09, 2022 at 09:18:39PM +0200, Marek Vasut wrote: >> The POWER_SUPPLY_PROP_CURRENT_NOW property represents, as far as I can tell, >> the immediate power supply input current, however, this driver reports the >> immediate battery charge current using that property, i.e. content of REG12 >> ICHGR -- ADC conversion of Charge Current (IBAT). Replace the property with >> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, which as far as I can tell should >> be used to report immediate battery charge voltage. >> >> This also aligns the behavior of POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT >> with POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, as the former reports IBAT >> and the later reports VBAT. > > I believe this is wrong: CC_CURRENT would be the constant-current charge phase > current limit (ICHG in REG04). > > Documentation/power/power_supply_class.rst mentions that: > > CONSTANT_CHARGE_CURRENT > constant charge current programmed by charger. I think I will wait for the subsystem maintainer to clarify these properties, because right now, this driver seems to be a complete mess in terms of what it reports and through which property. And I'm afraid that is because neither of those properties are clearly documented.
Hi, On 10/10/22 02:00, Marek Vasut wrote: > On 10/10/22 00:58, Michał Mirosław wrote: >> On Sun, Oct 09, 2022 at 09:18:39PM +0200, Marek Vasut wrote: >>> The POWER_SUPPLY_PROP_CURRENT_NOW property represents, as far as I can tell, >>> the immediate power supply input current, however, this driver reports the >>> immediate battery charge current using that property, i.e. content of REG12 >>> ICHGR -- ADC conversion of Charge Current (IBAT). Replace the property with >>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, which as far as I can tell should >>> be used to report immediate battery charge voltage. >>> >>> This also aligns the behavior of POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT >>> with POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, as the former reports IBAT >>> and the later reports VBAT. >> >> I believe this is wrong: CC_CURRENT would be the constant-current charge phase >> current limit (ICHG in REG04). >> >> Documentation/power/power_supply_class.rst mentions that: >> >> CONSTANT_CHARGE_CURRENT >> constant charge current programmed by charger. > > I think I will wait for the subsystem maintainer to clarify these properties, because right now, this driver seems to be a complete mess in terms of what it reports and through which property. And I'm afraid that is because neither of those properties are clearly documented. CURRENT_NOW normally is only used in fuel-gauge IC drivers (or in the fuel-gauge/battery-type power_supply device registered by a MFD device). When CURRENT_NOW is used on a fuel-gauge / battery type power_supply class device then CURRENT_NOW indeed should work as mentioned in the commit msg: "this driver reports the immediate battery charge current using that property" re-purposing this for reporting "the immediate power supply input current" seems wrong to me. For reporting "the immediate power supply input current" IMHO adding a new POWER_SUPPLY_PROP_INPUT_CURRENT_NOW property would be more appropriate to go hand in hand with the existing POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT limit. As for what to do with the existing use of POWER_SUPPLY_PROP_CURRENT_NOW in the bq25890 driver. I agree that this is a bit confusing, since charger type ICs really have 2/3 currents which are relevant: 1. The input-current from an external powersupply (if present) 2. The output-current going to the rest of the system 3. The (dis)charge-current flowing to/from the battery having an attribute simply named "current" is not really helpful, it is fine (and widely used by userspace) on battery-type power_supply class devices since there is only the 1 current going into/out-off the battery. I see 3 solutions for this for charger style devices: 1. Document that current_now is for the (dis)charge current flowing into/out-off the attached battery. So basically document what the bq25890 driver is doing now. 2. Add a new charge_current_now attribute for this and switch bq25890 over. 3. Don't report the charge-current in the charger driver, since this is duplicate info with the fuel-gauge driver. 2. has a bit of a risk of breaking userspace, so I'm not sure if that is a viable option. Given this I have a slight preference for solution 1. I hope this helps clarify the confusion surrounding this a bit. Regards, Hans
On 10/10/22 15:27, Hans de Goede wrote: > Hi, Hi, > On 10/10/22 02:00, Marek Vasut wrote: >> On 10/10/22 00:58, Michał Mirosław wrote: >>> On Sun, Oct 09, 2022 at 09:18:39PM +0200, Marek Vasut wrote: >>>> The POWER_SUPPLY_PROP_CURRENT_NOW property represents, as far as I can tell, >>>> the immediate power supply input current, however, this driver reports the >>>> immediate battery charge current using that property, i.e. content of REG12 >>>> ICHGR -- ADC conversion of Charge Current (IBAT). Replace the property with >>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, which as far as I can tell should >>>> be used to report immediate battery charge voltage. >>>> >>>> This also aligns the behavior of POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT >>>> with POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, as the former reports IBAT >>>> and the later reports VBAT. >>> >>> I believe this is wrong: CC_CURRENT would be the constant-current charge phase >>> current limit (ICHG in REG04). >>> >>> Documentation/power/power_supply_class.rst mentions that: >>> >>> CONSTANT_CHARGE_CURRENT >>> constant charge current programmed by charger. >> >> I think I will wait for the subsystem maintainer to clarify these properties, because right now, this driver seems to be a complete mess in terms of what it reports and through which property. And I'm afraid that is because neither of those properties are clearly documented. > > CURRENT_NOW normally is only used in fuel-gauge IC drivers (or in > the fuel-gauge/battery-type power_supply device registered by a MFD device). > > When CURRENT_NOW is used on a fuel-gauge / battery type power_supply class > device then CURRENT_NOW indeed should work as mentioned in the commit msg: > "this driver reports the immediate battery charge current using that property" > re-purposing this for reporting "the immediate power supply input current" > seems wrong to me. > > For reporting "the immediate power supply input current" IMHO adding > a new POWER_SUPPLY_PROP_INPUT_CURRENT_NOW property would be more > appropriate to go hand in hand with the existing > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT limit. The BQ25896 only ever samples one type of current using its ADC -- I_BAT -- charge current that is at this time used to charge the battery. So we do not really need a new property for that (maybe, let's see below). > As for what to do with the existing use of POWER_SUPPLY_PROP_CURRENT_NOW > in the bq25890 driver. I agree that this is a bit confusing, since > charger type ICs really have 2/3 currents which are relevant: > > 1. The input-current from an external powersupply (if present) Not present on BQ25896 > 2. The output-current going to the rest of the system Not present on BQ25896 > 3. The (dis)charge-current flowing to/from the battery That's the I_BAT > having an attribute simply named "current" is not really helpful, > it is fine (and widely used by userspace) on battery-type power_supply > class devices since there is only the 1 current going into/out-off > the battery. > > I see 3 solutions for this for charger style devices: > > 1. Document that current_now is for the (dis)charge current flowing > into/out-off the attached battery. So basically document what > the bq25890 driver is doing now. > > 2. Add a new charge_current_now attribute for this and switch > bq25890 over. > > 3. Don't report the charge-current in the charger driver, since > this is duplicate info with the fuel-gauge driver. > > 2. has a bit of a risk of breaking userspace, so I'm not > sure if that is a viable option. Given this I have a slight > preference for solution 1. > > I hope this helps clarify the confusion surrounding this a bit. Maybe we should add POWER_SUPPLY_PROP_CHARGE_CURRENT_NOW and deprecate POWER_SUPPLY_PROP_CURRENT_NOW ? That won't break userspace and would be clear in what it does. Yes, we would have duplicate sysfs attribute.
Hi, On 10/10/22 18:45, Marek Vasut wrote: > On 10/10/22 15:27, Hans de Goede wrote: >> Hi, > > Hi, > >> On 10/10/22 02:00, Marek Vasut wrote: >>> On 10/10/22 00:58, Michał Mirosław wrote: >>>> On Sun, Oct 09, 2022 at 09:18:39PM +0200, Marek Vasut wrote: >>>>> The POWER_SUPPLY_PROP_CURRENT_NOW property represents, as far as I can tell, >>>>> the immediate power supply input current, however, this driver reports the >>>>> immediate battery charge current using that property, i.e. content of REG12 >>>>> ICHGR -- ADC conversion of Charge Current (IBAT). Replace the property with >>>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, which as far as I can tell should >>>>> be used to report immediate battery charge voltage. >>>>> >>>>> This also aligns the behavior of POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT >>>>> with POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, as the former reports IBAT >>>>> and the later reports VBAT. >>>> >>>> I believe this is wrong: CC_CURRENT would be the constant-current charge phase >>>> current limit (ICHG in REG04). >>>> >>>> Documentation/power/power_supply_class.rst mentions that: >>>> >>>> CONSTANT_CHARGE_CURRENT >>>> constant charge current programmed by charger. >>> >>> I think I will wait for the subsystem maintainer to clarify these properties, because right now, this driver seems to be a complete mess in terms of what it reports and through which property. And I'm afraid that is because neither of those properties are clearly documented. >> >> CURRENT_NOW normally is only used in fuel-gauge IC drivers (or in >> the fuel-gauge/battery-type power_supply device registered by a MFD device). >> >> When CURRENT_NOW is used on a fuel-gauge / battery type power_supply class >> device then CURRENT_NOW indeed should work as mentioned in the commit msg: >> "this driver reports the immediate battery charge current using that property" >> re-purposing this for reporting "the immediate power supply input current" >> seems wrong to me. >> >> For reporting "the immediate power supply input current" IMHO adding >> a new POWER_SUPPLY_PROP_INPUT_CURRENT_NOW property would be more >> appropriate to go hand in hand with the existing >> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT limit. > > The BQ25896 only ever samples one type of current using its ADC -- I_BAT -- charge current that is at this time used to charge the battery. So we do not really need a new property for that (maybe, let's see below). Ah, ok from the discussion I was under the impression that the bq25890 could measure its input current. If it only ever measure the current to/from the battery then we indeed don't need a new property for this. >> As for what to do with the existing use of POWER_SUPPLY_PROP_CURRENT_NOW >> in the bq25890 driver. I agree that this is a bit confusing, since >> charger type ICs really have 2/3 currents which are relevant: >> >> 1. The input-current from an external powersupply (if present) > > Not present on BQ25896 > >> 2. The output-current going to the rest of the system > > Not present on BQ25896 > >> 3. The (dis)charge-current flowing to/from the battery > > That's the I_BAT Ack. >> having an attribute simply named "current" is not really helpful, >> it is fine (and widely used by userspace) on battery-type power_supply >> class devices since there is only the 1 current going into/out-off >> the battery. >> >> I see 3 solutions for this for charger style devices: >> >> 1. Document that current_now is for the (dis)charge current flowing >> into/out-off the attached battery. So basically document what >> the bq25890 driver is doing now. >> >> 2. Add a new charge_current_now attribute for this and switch >> bq25890 over. >> >> 3. Don't report the charge-current in the charger driver, since >> this is duplicate info with the fuel-gauge driver. >> >> 2. has a bit of a risk of breaking userspace, so I'm not >> sure if that is a viable option. Given this I have a slight >> preference for solution 1. >> >> I hope this helps clarify the confusion surrounding this a bit. > > Maybe we should add POWER_SUPPLY_PROP_CHARGE_CURRENT_NOW and deprecate POWER_SUPPLY_PROP_CURRENT_NOW ? That won't break userspace and would be clear in what it does. Yes, we would have duplicate sysfs attribute. We have a long history of upower (GNOME, XFCE, others) and the KDE equivalent (PowerDevil?) using POWER_SUPPLY_PROP_CURRENT_NOW so I'm afraid we will not be able to remove that in a long long time. As such I believe it would be better to just clearly document that the current attribute is for the power going into/out-of the battery and leave it at that. Regards, Hans
On 10/10/22 19:31, Hans de Goede wrote: > Hi, Hi, [...] >>> having an attribute simply named "current" is not really helpful, >>> it is fine (and widely used by userspace) on battery-type power_supply >>> class devices since there is only the 1 current going into/out-off >>> the battery. >>> >>> I see 3 solutions for this for charger style devices: >>> >>> 1. Document that current_now is for the (dis)charge current flowing >>> into/out-off the attached battery. So basically document what >>> the bq25890 driver is doing now. >>> >>> 2. Add a new charge_current_now attribute for this and switch >>> bq25890 over. >>> >>> 3. Don't report the charge-current in the charger driver, since >>> this is duplicate info with the fuel-gauge driver. >>> >>> 2. has a bit of a risk of breaking userspace, so I'm not >>> sure if that is a viable option. Given this I have a slight >>> preference for solution 1. >>> >>> I hope this helps clarify the confusion surrounding this a bit. >> >> Maybe we should add POWER_SUPPLY_PROP_CHARGE_CURRENT_NOW and deprecate POWER_SUPPLY_PROP_CURRENT_NOW ? That won't break userspace and would be clear in what it does. Yes, we would have duplicate sysfs attribute. > > We have a long history of upower (GNOME, XFCE, others) and > the KDE equivalent (PowerDevil?) using POWER_SUPPLY_PROP_CURRENT_NOW > so I'm afraid we will not be able to remove that in a long long time. > > As such I believe it would be better to just clearly > document that the current attribute is for the power > going into/out-of the battery and leave it at that. ACK. This already seems documented in the sysfs attributes in Documentation, so I will only add a comment into this driver.
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 34dbd498f0f51..c4c830247e0e0 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -432,8 +432,8 @@ static bool bq25890_is_adc_property(enum power_supply_property psp) { switch (psp) { case POWER_SUPPLY_PROP_VOLTAGE_NOW: + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE: - case POWER_SUPPLY_PROP_CURRENT_NOW: case POWER_SUPPLY_PROP_TEMP: return true; @@ -589,7 +589,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, val->intval = 2304000 + ret * 20000; break; - case POWER_SUPPLY_PROP_CURRENT_NOW: + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT: ret = bq25890_field_read(bq, F_ICHGR); /* read measured value */ if (ret < 0) return ret; @@ -881,6 +881,7 @@ static const enum power_supply_property bq25890_power_supply_props[] = { POWER_SUPPLY_PROP_CHARGE_TYPE, POWER_SUPPLY_PROP_ONLINE, POWER_SUPPLY_PROP_HEALTH, + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX, POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX, @@ -888,7 +889,6 @@ static const enum power_supply_property bq25890_power_supply_props[] = { POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, POWER_SUPPLY_PROP_VOLTAGE_NOW, - POWER_SUPPLY_PROP_CURRENT_NOW, POWER_SUPPLY_PROP_TEMP, };
The POWER_SUPPLY_PROP_CURRENT_NOW property represents, as far as I can tell, the immediate power supply input current, however, this driver reports the immediate battery charge current using that property, i.e. content of REG12 ICHGR -- ADC conversion of Charge Current (IBAT). Replace the property with POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT, which as far as I can tell should be used to report immediate battery charge voltage. This also aligns the behavior of POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT with POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE, as the former reports IBAT and the later reports VBAT. Fixes: 1e4724d0b7d1 ("power: bq25890: use proper CURRENT_NOW property for I_BAT") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Michał Mirosław <mirq-linux@rere.qmqm.pl> Cc: Sebastian Reichel <sebastian.reichel@collabora.com> To: linux-pm@vger.kernel.org --- drivers/power/supply/bq25890_charger.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)