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

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

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