Re: [PATCH v12.4 6/10] power: bq27xxx_battery: Add power_supply_battery_info support

["Andrew F. Davis" <afd@ti.com>]

On 04/07/2017 01:58 PM, Liam Breck wrote:
> On Thu, Apr 6, 2017 at 11:23 AM, Liam Breck <liam@networkimprov.net> wrote:
>> On Thu, Apr 6, 2017 at 7:30 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 04/04/2017 03:57 AM, Liam Breck wrote:
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> Previously there was no way to configure these chips in the event that the
>>>> defaults didn't match the battery in question.
>>>>
>>>> We now call power_supply_get_battery_info(), check its values, and write
>>>> battery data to chip data memory (flash/RAM/NVM).
>>>>
>>>
>>> I'm almost convinced now this is *not* the correct thing to do, we
>>> should not be writing to NVM. If the DT states different values than in
>>> the device we need to trust the device. DT is static for a board, the
>>> battery data may not be.
>>>
>>> At most we could have an override flag as a driver param that causes us
>>> to ignore NVM and use the DT values, but always overwriting the device's
>>> NVM with the DT values is not what most want.
>>
>> Reprising my previous response, with addenda...
>>
>> We trust the distro/device kernel package to contain correct config
>> for RAM-only chips, but not NVM? A battery config would not be set in
>> mainline dts files, unless a battery is soldered to the board, or
>> similar. I will note this in our DT binding.
>>

DT is for hardware descriptions, in cases where the hardware cannot be
automatically detected. If we find we can get the battery info from the
battery gauge why would we not only ignore it, but go and overwrite it
with the info from DT? This is backwards.

>> Re DT provisioning... Device makers and distros are extremely careful
>> about what they put into DTs and what dtbs are included with a kernel
>> package. They know that a misconfigured DT is a showstopper, and could
>> be catastrophic. I've seen the latter -- a guru-level kernel
>> maintainer whom I work with set a DT voltage level wrong on a
>> prototype and fried its eMMC chip. If the DT has a gauge config, the
>> default assumption is that it's correct.
>>

And if that eMMC chip could communicate its requested voltage we would
not need to describe it in DT, and we certainly would not say "sorry
eMMC, you requested 3v but DT says you need 33v, so thats what you're
getting".

>> It is wrong to force distros and device makers to include
>> /etc/modprobe.d/xyz.conf to fully configure the hardware.
>>

It's either that or add a driver sysfs entry, we don't configure
hardware in DT, thats rule #1. You got an ack for a binding that
describes a "simple-battery"(one that cannot self report), you are
misusing this binding as input configuration data to feed to batteries
that *can* report their configuration.

>> However we should indeed let users override the DT config with a
>> module param. How about use-on-chip-config=1, type bool?
> 

If we do add a param it needs to be default "off". It should not update
saved battery data on the chip in either case, that needs to be a
manually triggered through sysfs or similar.

> I came up with...
> 
> static bool bq27xxx_dt_battery = true;
> module_param_named(use_dt_battery, bq27xxx_dt_battery, bool, S_IRUGO);
> MODULE_PARM_DESC(use_dt_battery,
>  "Use devicetree monitored-battery config if present. Default is 1/Y.");
> 
> After we check for DT monitored-battery, we test bq27xxx_dt_battery,
> and if false, emit a warning that we are skipping the DT config.
>