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

[Linus Walleij <linus.walleij@linaro.org>]

On Wed, Dec 8, 2021 at 7:46 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hmm.. LKML skipped on purpose(?)

Yeah, MAINTAINERS says this:

POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS
M:      Sebastian Reichel <sre@kernel.org>
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:

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

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