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

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

Hi,

On 18-03-17 19:51, Liam Breck wrote:
> On Sat, Mar 18, 2017 at 7:13 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 18-03-17 08:10, Liam Breck wrote:
>>>
>>> [dropped CC's not relevant to BQ24190; pls re-add anyone appropriate]
>>>
>>> Hi Hans,
>>>
>>> I am the de facto maintainer of BQ24190, with help from Tony. I contracted
>>> the original author to
>>> create the driver. I'm not aware of anyone else relying on it at present.
>>
>>
>> Hi Liam, nice to meet you.
>>
>>> Pls rebase to -next (!) and post BQ24190 changes in a separate patchset.
>>
>>
>> Ok, I will rebase these patches on top of
>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>
> Yep.
>
>>> Reference any
>>>
>>> dependencies in other patchsets with Depends-on lines in patch
>>> descriptions linking to
>>> patchwork.kernel.org. If any dependency might be controversial, pls defer
>>> the BQ24190 patchset til
>>> controversy resolves.
>>
>> There are no dependencies for the BQ24190 patches, they are a dependency for
>> some of the other changes but they can go upstream as is.
>
> The extcon driver elsewhere in patchset?

That is only a runtime dependency and only when the extcon_name is
set in the platform_data, the patches can be merged upstream without
this (no compile / linktime dependency). Even runtime things will
not explode if the extcon driver is not there and asked for in
platform data, the probe() function will then simply return -EPROBE_DEFER.

>
>>> You may be interested in these, which I will support in BQ24190 in near
>>> future:
>>> https://patchwork.kernel.org/patch/9626487/
>>> https://patchwork.kernel.org/patch/9626491/
>>
>>
>> Yeah those are not going to matter for the use-case I'm looking at.
>> I've a feeling you've only looked at the bq24190 patches and not the
>> rest of the set. There is a reason I posted these all together as I
>> mentioned in the cover-letter:
>>
>> "I'm sending this as a single series so that people can see how all the bits
>> fit together."
>
> Reading the entire patchset may be a lot to ask some folks, altho I
> did. It's best to give relevant background in each patch description.
>
>> Anyways I will try to clarify things by answering you questions /
>> responding to your remarks.
>>
>>> Further comments below...
>>>
>>> On Fri, 17 Mar 2017 10:55:16 +0100, Hans de Goede wrote:
>>>
>>>> On some platforms the register have been setup with platform specific
>>>> values by the firmware and should not be reset.
>>>
>>>
>>> Pls describe this mysterious, meddling firmware. Who wrote it, when does
>>> it run, on what devices,
>>> etc? Include URLs to docs, etc. BTW it's not really "firmware" if it runs
>>> on the main cpu, IMO.
>>> A boot loader is not firmware.
>>
>>
>> So not all the world runs on ARM and not all devices using the
>> TI BQ24190 (and variants) use an ARM processor or devicetree
>> for that matter). Having worked on both ARM and x86 quite a bit
>> I've a feeling that you are approaching my patchset with somewhat
>> of an ARM focussed pov.
>>
>> The primary device I'm trying to get battery/charger monitoring
>> working for is the GPDwin: http://www.gpd.hk/gpdwin.asp
>>
>> Which is an Intel Cherrytrail using machine with an Intel
>> Cherrytrail Whiskey Cove PMIC.
>>
>> The firmware I'm talking about here is the EFI system firmware
>> which boots the machine aka the BIOS.
>
> Mention all of the above in patch descriptions.
>
>>> We probably don't want to disable register_reset in all circumstances. Can
>>> you record the
>>> firmware's settings for charger?
>>
>>
>> Yes after a (re)boot they are always reset to the same values.
>
> So record those settings in a platform_data field. That's your
> external config in this case.

Assuming the platform_data has sane values like uA that means
re-implementing most of the bq24190 driver in the code filling
the platform data (to go from register values to uA) that does
not seem like a sane approach when a simple do not reset
flag suffices. Note also see below for another way to deal
with this.

> The driver does a register_reset on resume so it has a consistent
> state with chip.

That assumes someone may be mucking with the chip during
suspend which usually is not the case arguably this is a
bad idea in general since if the chip is programmed with
lower then reset charging limits it may now charge above
the limits until the limits are re-applied.

I really wonder what the reset is there for at all TBH,
are there any cases where this really is necessary ?
Maybe it would be best to just drop the reset altogether,
or make it opt in rather then opt out.

>
>>> I suspect we should replay them in set_mode_host() -- to be
>>> renamed set_operating_params in forthcoming patchset.
>>
>>
>> That is not going to work, the Cherrytrail Whiskey Cove PMIC
>> is used on many different boards and there is no way to get
>> the info from the firmware other then reading it back from
>> the chip, hence the no_register_reset flag.
>
> This isn't about the PMIC. You need to know how boot code for a
> certain device configures the charger. Encode that knowledge in
> platform_data.

Again that info is already there in the registers and as long
as we do not touch them we're fine. Also see above about
needing to reproduce almost all of the bq24190 driver to
actually fill in platform_data from the registers.

> Also have you asked the vendor for docs?

Yes I've asked both the manufacturer and the chip vendor (Intel)
for docs, but I've not gotten an answer from either.

>>> What generates your platform_data objects?
>>
>>
>> The last patch in the patch-set shows this. The
>> Cherrytrail Whiskey Cove PMIC does charger type detection
>> and has a fuelgauge, but it does not contain a battery
>> charger itself. Instead it contains an i2c controller
>> to which an external charger chip, one of the
>> TI BQ24190 variants, gets connected.
>
> More useful background for patch descriptions :-)

I can put this in the patch descriptions as example.
but the patches are intended to be generally useful in case
a similar external fuel gauge situation comes along, which
is why I left the details out of the commit messages.

> Given that design, it sounds like the PMIC driver should be the point
> of contact with userspace, and use sysfs
> (/sys/class/power_supply/bq24190-charger/f_*) to get/set charger
> stats. We can amend the driver to extend that feature. (Also a good
> way to test chip knobs.)

One driver poking into the sysfs files of another driver is
not a sane intra driver interface, also this will require
(again) duplicating much of the bq24190 driver for all the
power_supply properties which it already implement.

I really do not see what your objection is against the
"power: supply: bq24190_charger: Add support for external fuel gauge"
patch is, the changes to bq24190_charger.c are minimal it defines
a clean, clear and simple interface. Where as the go through sysfs
interface solution you are proposing is neither of these 3.

I would appreciate it if you would actually think along with
me in how we can make this all work together nicely, currently
I'm getting the feeling that you want to make as little changes
to the bq24190-charger code as possible, even if that means
adding 10 times as much code elsewhere, that is not an acceptable
solution IMHO.


>
> Some TI fuel gauges also work this way - I2C link to external charger.
>
>> The driver for this i2c-controller calls i2c_new_device
>> with i2c_board_info describing the BQ24190 charger
>> after calling i2c_add_adapter().
>>
>>> Are you forced to use platform_data for some reason?
>>
>>
>> Since this is info being passed around inside the kernel
>> using platform_data is a hell of a lot simpler (and cleaner
>> no need to check types, etc on the receiving side) then
>> using device-properties. Device-properties are nice when
>> getting info from firmware, but this is not that.
>
> OK
>
>> Also one of the things passed is actually a function
>> pointer to a function to get extra power_supply_properties
>> from the fuel-gauge, and passing function pointers certainly
>> is not something which should be done through device-props.
>>
>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>>
>>> Add to all BQ24190 patches:
>>> Cc: Liam Breck <kernel@networkimprov.net>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>>
>>> Pls don't CC me on the related patchsets.
>>>
>>>> ---
>>>>  drivers/power/supply/bq24190_charger.c | 5 +++++
>>>>  include/linux/power/bq24190_charger.h  | 1 +
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index a4f0849..c92a40e4 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -151,6 +151,7 @@ struct bq24190_dev_info {
>>>>         struct device                   *dev;
>>>>         struct power_supply             *charger;
>>>>         struct power_supply             *battery;
>>>> +       struct bq24190_platform_data    *pdata;
>>>
>>>
>>> Use a register settings array/struct here, instead of platform-data. None
>>> of your other patches need
>>> to retain pdata.
>>
>>
>> Sure I can make a 1:1 copy of each field in pdata here, but that
>> is just more lines of code for the same result.
>
> I mean record the chip's UEFI-supplied state here.

Ok, so a flag get state from chip in platform_data would be
an acceptable solution to me as then I can re-use all the
code in bq24190_charger.c to go from register values to
uA (and uV).

> Currently
> no_register_reset appears to be the only pdata field used after
> probe()

The get_ext_bat_property callback also gets used after probe, but
that could be a separate variable in bq24190_dev_info.

Regards,

Hans