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

Sebastian Reichel
2017-03-20T21:14:54Z

[-- Attachment #1: Type: text/plain, Size: 5577 bytes --]

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.

Most of the properties should also be available from the fuel-gauge.
I see two exceptions:

* The battery disable FET
* The battery temperature monitoring

If that is needed by any of your platforms, something like the
API proposed by Hans to get properties from bq24190 into the
fuel-gauge driver should be introduced. According to my
understanding bq24190 checks temperature itself, so both
features seem to be optional.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]