Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger

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

Hi,

On 18-04-17 12:27, Liam Breck wrote:
> On Tue, Apr 18, 2017 at 3:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 18-04-17 11:42, Liam Breck wrote:
>>>
>>> Hi Hans,
>>>
>>> On Tue, Apr 18, 2017 at 1:05 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi Liam,
>>>>
>>>> On 17-04-17 01:06, Liam Breck wrote:
>>>>>
>>>>>
>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> The driver was registering two classes, bq24190-battery & -charger.
>>>>> Because the power supply framework cannot surface features from multiple
>>>>> drivers in a single class, a fuel gauge driver would create a third
>>>>> class,
>>>>> which some power management utilities cannot see.
>>>>>
>>>>> Deprecate the -battery class for future removal and replicate its
>>>>> features
>>>>> in -charger. Set /sys/class...-charger/online = pg_stat &&
>>>>> !batfet_disable.
>>>>> If device_property "omit-battery-class" is set, don't register -battery.
>>>>>
>>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>
>>>>
>>>>
>>>> Thank you for writing this patch.
>>>
>>>
>>> Glad to help!
>>>
>>>> The patch looks good to me:
>>>
>>>
>>> Darn, I was hoping you'd see *something* amiss in all that clutter :-)
>>
>>
>> Wel, I did not find the forward declaration of bq24190_battery_set_online
>> and friends pretty, but I can understand why you did this and we can
>> clean that up when we remove the battery interface altogether later.
>
> Blame the C standards crew for that mess :-)
>
>>> Do you think charger-powered and battery-connected is the right
>>> definition of power_supply_prop_online?
>>
>>
>> Now that you specifically ask I've been thinking a bit more this,
>> but I still can't find anything wrong with that as definition :)
>
> The power_supply_class docs really should tell us how to define this.
>
>>> Sebastian had suggested surfacing the batfet as a regulator, but altho
>>> user settable, it's a binary widget.
>>
>>
>> Turning off the batfet typically will turn off the entire system,
>> I believe it is more of an emergency break style switch which gets
>> thrown by the charger under certain (very bad) conditions then
>> something which we would actually ever want to use / control from
>> within the kernel.
>
> It's supposed to be disabled at the factory prior to shipping to avoid
> depleting the battery before product reaches buyer. The same should
> also be done for storage. So it can be an end-user switch.

True.

Regards,

Hans