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

[Hans de Goede <hdegoede@redhat.com>]

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