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