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

On 12/14/21 08:53, Matti Vaittinen wrote:

On 12/14/21 08:44, Matti Vaittinen wrote:

quoted

Hi deee Ho peeps,

On 12/13/21 11:23, Matti Vaittinen wrote:

quoted

On 12/10/21 07:57, Matti Vaittinen wrote:

quoted

On Thu, Dec 9, 2021 at 2:46 AM Linus Walleij 
[off-list ref] wrote:

quoted

quoted

Also I would love if you could test v2 on hardware!

quoted

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.

quoted

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 <redacted>
Tested-By: Matti Vaittinen <redacted>

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 ~~