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

On Wed, Dec 8, 2021 at 7:46 AM Matti Vaittinen [off-list ref] wrote:

Hmm.. LKML skipped on purpose(?)

Yeah, MAINTAINERS says this:

POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
M:      Sebastian Reichel [off-list ref]
L:      linux-pm@vger.kernel.org

LKML is only the bin where we put everything that doesn't have a proper
maintenance mailing list.

On 12/6/21 02:06, Linus Walleij wrote:

quoted

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?

No it is a single battery. But the data is kept in the kernel and matched
per-battery, I pushed the latest code from my desk so you can see how
it looks in practice:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-nomadik.git/commit/?h=ux500-href-charging-v5.16-rc1

I'd like to understand how you plan
to do mapping to compatibles - AFAIR, at the moment the compatible must
be "simple-battery".

This is for the Samsung batteries using these bindings:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/power/supply/samsung,battery.yaml

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).

The Samsung battery bindings are already in use, I used them for
example in these:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ste-ux500-samsung-janice.dts
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ste-ab8500.dtsi

It's very simplistic:

battery: battery {
    compatible = "samsung,eb535151vu";
};
(...)

ab8500 {
   compatible = "stericsson,ab8500";
(...)
  ab8500_charger {
     compatible = "stericsson,ab8500-charger";
(...)
       monitored-battery = <&battery>;

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...(?)

I suppose if footprint becomes a problem one can just split the
battery data into smaller files, and compile each one with a
KConfig.

As you can see in my example the Samsung batteries actually
share the same capacity table across three different batteries
so they actually benefit from being defined together when we
want to support all (which we do).

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.

But we already (partly) do that, look in power_supply_get_battery_info()
as touched by this patch:

(...)
        list = of_get_property(battery_np, "resistance-temp-table", &len);
        if (!list || !len)
                goto out_ret_pointer;

        info->resist_table_size = len / (2 * sizeof(__be32));
        resist_table = info->resist_table = devm_kcalloc(&psy->dev,

info->resist_table_size,
                                                         sizeof(*resist_table),
                                                         GFP_KERNEL);

Notice the goto out_ret_pointer; we return the pointer without
allocating any resistance-temp-table if it is not present in the
device tree. The same can be done with static data and
consumers need to cope with some members being NULL.

I don't see the usecase for mixing static and dynamic info
really, I think we have two cases:

- All information about the battery obtained from one single
  key such as the compatible string (as for the Samsung
  SDI batteries)

- All information about the battery contained in the device
  tree (as for simple-battery)

The way discussion has been going in the device tree
community the "define it all in devicetree" approach is
kind of soft discouraged.

The simple-battery IMO should be for
things like prototypes or devices where you don't really
know which battery is connected so you will have some
locally modified device trees for them.

In the Samsung case, these phones are mechanically
designed to just fit with one battery, and this seems to
be what Samsung does consistently: a battery with one
physical characteristic should not fit physically in the
slot of a device with different requirements.

That said I see what you're doing with the init data in
the bd99954 charger driver and it is similar to what the
regulator subsystem does too.

The dynamic data must be freed by (keep put batttery info API or just
allow user to kfree?)

The way the drivers use it is that some just call
power_supply_get_battery_info() copy some values from the
struct power_supply_battery_info into its own structs and then
just call power_supply_put_battery_info() and get rid of it all
again, and then the use is zero. (bd99954 for example)

Other drivers keep it around and reference it but it is
lifecycled with devres to the psy so it goes away when
removing the module (etc).

Either way works I guess, I just have a habit of using the
kernel structs as-is.

Or is this just adding a lot of extra complexity in order to save few
bytes?

On a general note the footprint discussion is kind of dead because
the community does not really prioritize that. Nicolas Pitre
invested lots of time to make the kernel configurable and
strippable with the goal of using it for IoT targets in 2018, and there
just wasn't interest. But anyone can drive such change of course.

The power supply core isn't really the worst offender though.

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.

I will probably need to add more entries to it as well and then I expect
to make them optional not compulsory, like what we do for resistance
already.

quoted

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?

This driver was a challenge so no :) it uses the info
differently from all others.

I looked over your comments and I missed how the init
data is used and then discarded. I will respin the patch
and fix this.

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

The patch didn't apply on v5.16-rc4

It's based on Sebastians tree so you need to use that, or just
grab my branch (should be possible to merge in v5.16-rc4
if you need)

Yours,
Linus Walleij