Re: [PATCH v2] power: supply_core: Pass pointer to battery info

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

On 12/14/21 08:53, Matti Vaittinen wrote:
> On 12/14/21 08:44, Matti Vaittinen wrote:
>> Hi deee Ho peeps,
>>
>> On 12/13/21 11:23, Matti Vaittinen wrote:
>>> On 12/10/21 07:57, Matti Vaittinen wrote:
>>>> On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij 
>>>> <linus.walleij@linaro.org> wrote:
>>>
>>>>> Also I would love if you could test v2 on hardware!
>>
> 
>> It appears the patch worked as expected - but it also appears the 
>> BD99954 driver does not handle missing info too well... I typoed the 
>> trickle-charger current property in DT - and as a result the driver 
>> decided -EINVAL to be valid value (just too large) and set the largest 
>> current BD99954 supports for trickle-charging... 
> 
> I should have looked this more carefully. It appears the BD99954 does 
> check for the -EINVAL - but the power_supply_core does not initialize 
> the tricklecharge_current_ua to -EINVAL.
> 
>> Linus, want to fix this while at it - or do you prefer me to patch the 
>> BD99954 with some sanity checks? I think it'd be nice to get the fixes 
>> in stable so it might be best to add the sanity checks before changing 
>> the battery-info allocation - that might be nicer for the stable 
>> folks. (I guess you have plenty of other things to code + some IRL 
>> tasks as well ...;] So, I can patch this but it means there is likely 
>> to be some conflicts with your series. Hence I thought I'll ask if you 
>> wish to add checks for uninitialized battery-info values)
> 
> I think this is a trivial thing to fix and won't be too hard a conflict 
> to resolve :) So I'll just send a patch
> 
> Sorry for the hassle.
> 
> So, what it's worth:
> Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Tested-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 

May I withdraw all of my statements regarding this?

I did suddenly recall why I did not initialize any of the battery-info 
fields I added back when I developed BD99954 driver. Before this change, 
the content and initialization of battery-info fields (which I added) 
was in control of the caller. It made sense to _not_ override values 
caller might have added to battery-info buffer if there was no better 
information in device-tree available. For example, the BD99954 driver 
did initialize the information to zero - which is in BD99954 case is the 
safest option as it yields lowest voltages/current limits. It did work 
untill this change - which now requires the core to initialize all the 
fields.

So, as I see it (at this particular moment and hopefully also 10 minutes 
from now :] ) - initialization was _not_ needed prior this change from 
Linus. It however is required after this change - so Linus, could you 
please add the:

         info->charge_term_current_ua         = -EINVAL;
         info->constant_charge_current_max_ua = -EINVAL;
         info->constant_charge_voltage_max_uv = -EINVAL;
+       info->tricklecharge_current_ua       = -EINVAL;
+       info->precharge_voltage_max_uv       = -EINVAL;
+       info->charge_restart_voltage_uv      = -EINVAL;
+       info->overvoltage_limit_uv           = -EINVAL;
         info->temp_ambient_alert_min         = INT_MIN;
         info->temp_ambient_alert_max         = INT_MAX;
         info->temp_alert_min                 = INT_MIN;

After this my tested-by and reviewed-by can stay there :)

Best Regards
	-- Matti Vaittinen

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~