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

[Marek Vasut <marex@denx.de>]

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.