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

[Matti Vaittinen <mazziesaccount@gmail.com>]

Good Morning Linus,

Hmm.. LKML skipped on purpose(?)

On 12/6/21 02:06, Linus Walleij wrote:
> The function to retrieve battery info (from the device tree) assumes
> we have a static info struct that gets populated by calling into
> power_supply_get_battery_info().

I kind of see the value of having the static info structure. It is damn 
easy to use and works well for small items. IMNAAHAWBA-O (In My Not 
Always As Humble As Would Be Appropriate - Opinion) the mistake has been 
adding the large, fixed size arrays in the same struct.

> This is awkward since I want to support tables of static battery
> info by just assigning a pointer to all info based on e.g. a
> compatible value in the device tree.

Do you have a case where you have battery data in multiple DT nodes? 
Some kind of multi-battery use-case? I'd like to understand how you plan 
to do mapping to compatibles - AFAIR, at the moment the compatible must 
be "simple-battery". Do you plan to have multiple compatibles in the DT, 
one of them matching the "simple-battery", rest being used as a key? Or 
do you plan to use the charger compatible (charger which references the 
battery with the monitored-battery as a key? My initial feeling is that 
it kind of makes sense).

I was wondering how it would work out if the battery info was splitted 
to smaller (optional) pieces instead of being just one big struct? It 
kind of makes no sense to always reserve space for all of the 
calibration data arrays when some of them are likely to be missing...(?)

something like:

struct static_batinfo static_info;
struct dynamic_batinfo *dynamic_info;

power_supply_get_battery_info(bd->charger, &static_info, &dynamic_info)

dynamic_info can be NULL if it is not expected.
dynamic_info will be NULL'ed by call if it is not populated
dynamic_info will be allocated if it is not NULL when called and if 
dynamic data is found from the firmware.
The dynamic data must be freed by (keep put batttery info API or just 
allow user to kfree?)

Or is this just adding a lot of extra complexity in order to save few 
bytes? I am not pushing this - it's up to you guys. I am "just a random 
guy from the streets" as someone once put it ;) For me it is just 
troubling to always have all the arrays in battery data - and I still do 
like the ability to avoid dynamic allocation when we don't have much of 
the info in the DT.

> 
> We also have a mixture of static and dynamically allocated
> variables here.
> 
> Bite the bullet and let power_supply_get_battery_info() allocate
> also the memory used for the very top level
> struct power_supply_battery_info container. Pass pointers
> around and lifecycle this with the psy device just like the
> stuff we allocate inside it.
> 
> Change all current users over.
> 
> In the bd99954 charger driver we need to stop issuing
> power_supply_put_battery_info() at the end of the probe since
> the struct continues to be used after probe().

Hm. Are you sure?
The patch didn't apply on v5.16-rc4 so I may have missed something as I 
was just reading the diff. AFAIR, the original idea was that the batery 
info lifetime is only the fw_probe - where the value from battery info 
was used to get the initialization register values which are then stored 
to init data. The init data was allocated at probe.

	/* Allocated at probe */
	struct bd9995x_init_data *init = &bd->init_data;
	struct battery_init battery_inits[] = {
	{
		.name = "trickle-charging current",
		.info_data = &info.tricklecharge_current_ua,
		.range = &charging_current_ranges[0],
		.ranges = 2,

		/* Pointer to allocated init data */
		.data = &init->itrich_set,
	}, {
		...


	ret = power_supply_get_battery_info(bd->charger, &info);
	...

	/* Use info to get correct register value */
	...
	
	/* Store info to allocated init data */
	*(battery_inits[i].data) = regval;

Maybe I am missing something?


Best Regards
	-- Matti