[PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[Marek Vasut]

The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , representing register
REG0E field BATV is an ADC conversion of Battery Voltage (VBAT). Mark
the property as ADC one.

Fixes: 21d90eda433f ("power: bq25890: fix ADC mode configuration")
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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 6020b58c641d2..34dbd498f0f51 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -432,6 +432,7 @@ 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_VOLTAGE:
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 	case POWER_SUPPLY_PROP_TEMP:
 		return true;
-- 
2.35.1

[PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Marek Vasut]

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(-)

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,
 };
 
-- 
2.35.1

Re: [PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Michał Mirosław]

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

Re: [PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Marek Vasut]

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.

Re: [PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Hans de Goede]

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

Re: [PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Marek Vasut]

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.

Re: [PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Hans de Goede]

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

Re: [PATCH 2/2] power: supply: bq25890: Rename POWER_SUPPLY_PROP_CURRENT_NOW to CC current

[Marek Vasut]

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.

Re: [PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[Michał Mirosław]

On Sun, Oct 09, 2022 at 09:18:38PM +0200, Marek Vasut wrote:
> The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , representing register
> REG0E field BATV is an ADC conversion of Battery Voltage (VBAT). Mark
> the property as ADC one.

In this case I believe the property is representing wrong value: it
should be REG06 - the programmed voltage limit, and _MAX should reflect
maximum setting possible. Though I think there is no proper property
for the VSYS value that is currently occupying VOLTAGE_NOW - this
might be better modelled as a separate regulator maybe?

But, for the time being this look ok.

> Fixes: 21d90eda433f ("power: bq25890: fix ADC mode configuration")
> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 6020b58c641d2..34dbd498f0f51 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -432,6 +432,7 @@ 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_VOLTAGE:
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  	case POWER_SUPPLY_PROP_TEMP:
>  		return true;
> -- 
> 2.35.1
> 

-- 
Michał Mirosław

Re: [PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[Hans de Goede]

Hi,

On 10/10/22 01:08, Michał Mirosław wrote:
> On Sun, Oct 09, 2022 at 09:18:38PM +0200, Marek Vasut wrote:
>> The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , representing register
>> REG0E field BATV is an ADC conversion of Battery Voltage (VBAT). Mark
>> the property as ADC one.
> 
> In this case I believe the property is representing wrong value: it
> should be REG06 - the programmed voltage limit, and _MAX should reflect
> maximum setting possible.

I believe this is correct, POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE
should contain the voltage up to which the charger will charge the
connected battery.

The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE and
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT basically reflect
2 configuration settings / values for the 2 phases most charge-ICs use:

Phase 1, when the voltage of the battery is below the constant-charge-voltage
the charger operates in constant-current mode, operating as a current source
delivering constant-charge-current Ampere into the battery.

Phase 2, when the open-clamp voltage has reached the constant-charge-voltage
value, then the charger IC will switch to constant-voltage mode, operating
as a voltage source, feeding a smaller current into the battery for as long
as there is a voltage differential between the open-clamp voltage of the
battery and the voltage source. Once the 2 reach equilibrium the current
becomes 0 Amps and charging is done.

The POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE and
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT are supposed to reflect the
current resp. voltage settings for these 2 phases and are not supposed
to be ADC readings (an ADC reading is not constant).

As for VOLTAGE_NOW, that is supposed to be an ADC reading, but as
I mentioned about current_now in my earlier reply to patch 2/2 in this
series, VOLTAGE_NOW normally is only used on battery-type power_supply
class devices (so with fuel-gauge ICs).

The problem with using VOLTAGE_NOW with a charger IC is that
(like currents) a charger IC basically has 3 voltages:

1. The input-voltage from an external powersupply (if present)
2. The output-voltage going to the rest of the system
3. The voltage of the connected battery

With 3. being complicated by there being both an actually measured
voltage, which is the result of the internal battery voltage +
an offset caused by the (dis)charge current. as well as a so
called open-clamp voltage which is purely the internal battery
voltage (which gets measured or guessed through magic)

For battery-type power-supply class devices VOLTAGE_NOW is
used to reflect the actual measured voltage of the battery
connections. So using it for Vsys here, which I assume is 2.
from my list of 3 voltages above pretty much feels wrong.

As I mentioned in my reply to 2/2 I believe that for
CURRENT_NOW and likewise VOLTAGE_NOW we should document that
when these are used with charger type power-supply class devices
these 2 should reflect the current/voltage of the battery
connection of the charger, so that they match with their
use in battery-type power-supply devices (where they
are found much more often, many charger-ICs don't have an
ADC for this at all.

We don't really have any existing properties to convey the
Vsys voltage (nor current) atm. So if we want to export
this then IMHO we should add new:

POWER_SUPPLY_PROP_SYSTEM_VOLTAGE,
POWER_SUPPLY_PROP_SYSTEM_CURRENT,

properties for these.

> Though I think there is no proper property
> for the VSYS value that is currently occupying VOLTAGE_NOW - this
> might be better modelled as a separate regulator maybe?

Ack, see above.

> 
> But, for the time being this look ok.

I disagree the current abuse of POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE
should be fixed, "REG0E field BATV is an ADC conversion of Battery Voltage (VBAT)"
should be the value used for VOLTAGE_NOW and the current Vsys reading exported
in VOLTAGE_NOW should instead be reported in a new property.

And then POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE should be used
to actually report the 4.2 or 4.35 volt limit (guessing here) to which
the charger will actually charge the battery.

As with my other reply I hope this helps to clarify how these properties
should be used. Or at least how I strongly believe that they should be
used...

Regards,

Hans



> 
>> Fixes: 21d90eda433f ("power: bq25890: fix ADC mode configuration")
>> 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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index 6020b58c641d2..34dbd498f0f51 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -432,6 +432,7 @@ 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_VOLTAGE:
>>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>>  	case POWER_SUPPLY_PROP_TEMP:
>>  		return true;
>> -- 
>> 2.35.1
>>
> 

Re: [PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[Marek Vasut]

On 10/10/22 15:50, Hans de Goede wrote:
> Hi,

Hi,

[...]

>> Though I think there is no proper property
>> for the VSYS value that is currently occupying VOLTAGE_NOW - this
>> might be better modelled as a separate regulator maybe?
> 
> Ack, see above.

We already do have a regulator in the bq25890 driver. The regulator is 
used as a switch to toggle OTG boost mode (supply from battery to VBUS), 
but I don't see any users of this functionality, and I cannot imagine 
how this would be modeled in DT. (Hans, can you clarify?)

There is the usb_work (usb_register_notifier()) which triggers workqueue 
which does the same, toggles OTG boost mode, but this is only used in 
case a valid USB PHY is found. I didn't find any users of this either.

Anyway, maybe we can extend the regulator to report VBus and register 
another one to report VSys, where the VSys one can be plugged e.g. as 
supply for PMIC in DT ?

[...]

Re: [PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[Hans de Goede]

Hi,

On 10/10/22 21:22, Marek Vasut wrote:
> On 10/10/22 15:50, Hans de Goede wrote:
>> Hi,
> 
> Hi,
> 
> [...]
> 
>>> Though I think there is no proper property
>>> for the VSYS value that is currently occupying VOLTAGE_NOW - this
>>> might be better modelled as a separate regulator maybe?
>>
>> Ack, see above.
> 
> We already do have a regulator in the bq25890 driver. The regulator is used as a switch to toggle OTG boost mode (supply from battery to VBUS), but I don't see any users of this functionality, and I cannot imagine how this would be modeled in DT. (Hans, can you clarify?)

Ah, with the Ack I meant ack for the "I think there is no proper property for the VSYS value" I did not meant to ack the regulator bit, I don't directly see how having registering a regulator device for Vsys would be useful, sorry. As mentioned in my original email I believe that just adding a new property for Vsys makes the most sense.

Note the OTG regulator is useful to enable/disable 5V boost output when used with e.g. a micro-usb connector and that micro-usb connector is used with an micro-USB OTG host-mode cable / dongle.

> There is the usb_work (usb_register_notifier()) which triggers workqueue which does the same, toggles OTG boost mode, but this is only used in case a valid USB PHY is found. I didn't find any users of this either.

This is used, but the use is hidden away pretty well I admit. Some X86 tablets from the Cherry Trail era have ACPI tables where the charging / fuel-gauge ICs are not handled in ACPI (as they typically are on x86) so we need to do it ourselves.

Specifically the charger IC is connected to a special I2c bus of the PMIC (which itself is an i2c-device so we have an i2c-attached i2c-controller, crazy ...) the driver for this special PMIC embedded i2c-controller is also responsible for instantiating the charger chip i2c-client and this setup uses the OTG regulator device, see: drivers/i2c/busses/i2c-cht-wc.c specifically these bits:

/********** Xiaomi Mi Pad 2 charger IC settings  **********/
static struct regulator_consumer_supply bq2589x_vbus_consumer = {
        .supply = "vbus",
        .dev_name = "cht_wcove_pwrsrc",
};

static const struct regulator_init_data bq2589x_vbus_init_data = {
        .constraints = {
                .valid_ops_mask = REGULATOR_CHANGE_STATUS,
        },
        .consumer_supplies = &bq2589x_vbus_consumer,
        .num_consumer_supplies = 1,
};

static struct bq25890_platform_data bq2589x_pdata = {
        .regulator_init_data = &bq2589x_vbus_init_data,
};

static const struct property_entry xiaomi_mipad2_props[] = {
        PROPERTY_ENTRY_BOOL("linux,skip-reset"),
        PROPERTY_ENTRY_BOOL("linux,read-back-settings"),
        { }
};

static const struct software_node xiaomi_mipad2_node = {
        .properties = xiaomi_mipad2_props,
};

static struct i2c_board_info xiaomi_mipad2_board_info = {
        .type = "bq25890",
        .addr = 0x6a,
        .dev_name = "bq25890",
        .swnode = &xiaomi_mipad2_node,
        .platform_data = &bq2589x_pdata,
};

And then the drivers/extcon/extcon-intel-cht-wc.c uses
the vbus regulator to control the 5V out on the micro-USB.

(also note these devices do not instantiate a usb_phy device anywhere, which is why the V5 boost is handled through the regulator framework)

TL;DR: the regulator device for the V5 boost output is used, please don't remove it :)

> Anyway, maybe we can extend the regulator to report VBus and register another one to report VSys, where the VSys one can be plugged e.g. as supply for PMIC in DT ?

I'm not sure if a regulator device for Vsys is really useful, AFAIK Vsys can not be turned off, nor can the voltage level be controlled... 

Regards,

Hans

Re: [PATCH 1/2] power: supply: bq25890: Add CC voltage to ADC properties

[Marek Vasut]

On 10/11/22 09:38, Hans de Goede wrote:
> Hi,

Hi,

>>>> Though I think there is no proper property
>>>> for the VSYS value that is currently occupying VOLTAGE_NOW - this
>>>> might be better modelled as a separate regulator maybe?
>>>
>>> Ack, see above.
>>
>> We already do have a regulator in the bq25890 driver. The regulator is used as a switch to toggle OTG boost mode (supply from battery to VBUS), but I don't see any users of this functionality, and I cannot imagine how this would be modeled in DT. (Hans, can you clarify?)
> 
> Ah, with the Ack I meant ack for the "I think there is no proper property for the VSYS value" I did not meant to ack the regulator bit, I don't directly see how having registering a regulator device for Vsys would be useful, sorry. As mentioned in my original email I believe that just adding a new property for Vsys makes the most sense.

I disagree here, have a look at the new series I posted that adds the 
Vsys regulator, esp. patch 7/7 . I think the Vsys regulator does make 
sense, since it can be the supply for PMIC, which would let us model 
that hardware connection between the charger and PMIC properly, and even 
in DT.

> Note the OTG regulator is useful to enable/disable 5V boost output when used with e.g. a micro-usb connector and that micro-usb connector is used with an micro-USB OTG host-mode cable / dongle.
> 
>> There is the usb_work (usb_register_notifier()) which triggers workqueue which does the same, toggles OTG boost mode, but this is only used in case a valid USB PHY is found. I didn't find any users of this either.
> 
> This is used, but the use is hidden away pretty well I admit. Some X86 tablets from the Cherry Trail era have ACPI tables where the charging / fuel-gauge ICs are not handled in ACPI (as they typically are on x86) so we need to do it ourselves.
> 
> Specifically the charger IC is connected to a special I2c bus of the PMIC (which itself is an i2c-device so we have an i2c-attached i2c-controller, crazy ...) the driver for this special PMIC embedded i2c-controller is also responsible for instantiating the charger chip i2c-client and this setup uses the OTG regulator device, see: drivers/i2c/busses/i2c-cht-wc.c specifically these bits:

[...]

> And then the drivers/extcon/extcon-intel-cht-wc.c uses
> the vbus regulator to control the 5V out on the micro-USB.

Uh, now I understand, thanks for clarification.

> (also note these devices do not instantiate a usb_phy device anywhere, which is why the V5 boost is handled through the regulator framework)
> 
> TL;DR: the regulator device for the V5 boost output is used, please don't remove it :)

I won't, I only extended the current regulator registration in the end.

>> Anyway, maybe we can extend the regulator to report VBus and register another one to report VSys, where the VSys one can be plugged e.g. as supply for PMIC in DT ?
> 
> I'm not sure if a regulator device for Vsys is really useful, AFAIK Vsys can not be turned off, nor can the voltage level be controlled...

It can be used as a supply for system PMIC, so I think it is actually 
useful. Sure, you cannot turn the Vsys on/off, but that's not the sole 
purpose of the regulator. The PMIC can get its input voltage and 
configure itself accordingly, which without the Vsys regulator providing 
its current voltage is not possible.