Re: [04/15] power: supply: bq24190_charger: Add no_register_reset pdata flag

Hans de Goede
2017-03-20T19:23:04Z

Hi,

On 20-03-17 19:19, Liam Breck wrote:
> On Mon, Mar 20, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 20-03-17 18:51, Liam Breck wrote:
>>>
>>> On Mon, Mar 20, 2017 at 10:04 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> On 20-03-17 14:54, Hans de Goede wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 20-03-17 06:27, Sebastian Reichel wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Sun, Mar 19, 2017 at 10:42:00AM +0100, Hans de Goede wrote:
>>>>>
>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>>> We want one driver which is solidly in control of the charger
>>>>>>> since getting that wrong is quite bad and we want to extend
>>>>>>> the information it is exporting to userspace in the form of
>>>>>>> power_supply properties with some extra info.
>>>>>>>
>>>>>>> So taking that as a starting point and generalizing that,
>>>>>>> I think that if we want we can make something more generic then
>>>>>>> my original patch for this. My idea is to introduce something
>>>>>>> called a power_supply_properties_provider, with an API
>>>>>>> like this:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks for thinking about this. From what I can see it should
>>>>>> be enough to just avoid exposing a battery device at all in
>>>>>> the charger driver, though. It does not really provide useful
>>>>>> information anyways.
>>>>>
>>>>>
>>>>>
>>>>> Interesting suggestion, 2 remarks / questions:
>>>>>
>>>>> 1) Given that we do not want to break existing setups
>>>>> which may depend on the existing battery interface on the bq24190
>>>>> driver and add a flag to not register it, or are you suggesting
>>>>> to simply remove it all together? Removing it all together would
>>>>> be better from a maintenance pov. Liam, you indicated that you
>>>>> are the main user of the bq24190 driver currently, do you need
>>>>> the battery power_supply interface for your use case ?
>>>>>
>>>>> 2) Not using the battery interface of the bq24190 driver at all
>>>>> means that the fuelgauge driver needs to grow some extra
>>>>> properties, specifically it will need to start reporting status,
>>>>> something which the bq24190 driver really has a better idea
>>>>> of the the fuel gauge.
>>>
>>>
>>> Our userspace looks at bq24190-battery & -charger. If your userspace
>>> can work with /sys/class/power_supply/whisky_cove-battery/* can't you
>>> leave .bq24190-battery as is?
>>
>>
>> No, my userspace is a generic distro which uses upower, which as I
>> explained before will look at *all* battery type power_supply devices
>> and will consider each of them being a separate battery. There *must*
>> be only one battery type power_supply per physical battery, that is
>> simply how the userspace ABI works.
>>
>>> bq24190-battery's properties could move to -charger, tho health &
>>> online would have to change name, replace same in -charger, or be
>>> dropped.
>>
>>
>> That is one option, I can also simply make the registration dependent
>> on a device-property, then you do not need to change your userspace.
>>
>> In that case I would prefer for the behavior to be to not register
>> the battery power_supply device unless the boolean device(tree)-property
>> named "linux,register-battery-power-supply" is present. But if you
>> need things to keep working the same with older dtb files which
>> will not contain that property, we can also go for:
>> "linux,disable-battery-power-supply"
>
> Just if (!pdata->x) register(battery). Eventually I'll have to move
> those properties to charger, as we added a fuel gauge after charger
> driver was written.

Sebastian want to kill the pdata and move to device-properties as
both you (IIRC) and Andy already suggested. So that is the plan now.

So we will end up with something like:

if (!device_property_read_bool(dev, "disable-battery-power-supply")) {
register(battery)...
}

> Or change bq24190_battery_desc.type to not be _BATTERY?

That sounds like an ugly hack to me, the above will work fine, if you
want to you can move the bits you need to the charger power_supply
(as time permits) and then when they are all gone we can kill of the
battery one.

Regards,

Hans