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

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

Hi,

On 20-03-17 22:14, Sebastian Reichel wrote:
> On Mon, Mar 20, 2017 at 08:22:55PM +0100, Hans de Goede wrote:
>> 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.
>
> From my understanding Liam's platform uses bq27xxx based fuel-gauge,
> so it also should not provide the bq24190-battery (since that would
> result in two battery devices being exposed for the same battery).
> So removing it seems to be the way to go.

Ok, so for v2 of my series I will just remove it and then we can see
from there, we probably need to do some repair work on top to not
break stuff for Liam, but I agree that just removing it seems best.

Regards,

Hans