Message-ID: <CACRpkdaYaE+1GKNv5SczC+Xn8UuBonZcW4RSdbsU53HWTR_tTg@mail.gmail.com>
Date:   Fri, 21 Apr 2023 11:20:09 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Jakob Hauser <jahau@...ketmail.com>
Cc:     Sebastian Reichel <sre@...nel.org>, Lee Jones <lee@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Beomho Seo <beomho.seo@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Stephan Gerhold <stephan@...hold.net>,
        Raymond Hackley <raymondhackley@...tonmail.com>,
        Pavel Machek <pavel@....cz>, Axel Lin <axel.lin@...ics.com>,
        ChiYuan Huang <cy_huang@...htek.com>, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 9/9] dt-bindings: Add documentation for rt5033 mfd,
 regulator and charger

Hi Jakob,

On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <jahau@...ketmail.com> wrote:
> > On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <linus.walleij@...aro.org> wrote:

> > On second thought, these are really weird properties to have on the
> > *charger* isn't it?
> >
> > It is really *battery* restrictions.
> >
> > A charger can charge many different batteries with different CC/CV
> > settings.

(...)
> I was first a bit confused by the term "battery". I associated that term
> with the driver "rt5033-battery". But I think that thought was wrong.
> The driver "rt5033-battery" is just the fuel gauge.

Yeah that is a different thing altogether, it should be named
rt5033-fuel-gauge if I could turn back time, I would review it
and say this, perhaps the confusion can be fixed. Mistakes
were made.

> The properties we talk about here are the settings for the charger. They
> tell the charger how it should behave. It makes sense to process those
> settings within the charger driver.

It may make sense to *parse* this in the charger driver, by
following the monitored-battery phandle to inspect the battery
properties and get the information you need.

The architecture of any Linux driver does not really concern
the DT bindings, the bindings should reflect the hardware.
The hardware has a charger, and the charger is monitoring
a battery, so it needs to be its own DT node.

> The fuel gauge, on the other hand,
> returns information like actual voltage and percentage.

The fuel gauge should probably have a phandle to the same
battery for compleness sake, but may not need it. If it ever needs
any battery properties, it should definately have that.

> According to your remarks, the properties could be "outsourced" into a
> battery node. (Btw. I have double-checked the property names.)
>
>      battery: battery {
>          compatible = "simple-battery";
>          precharge-current-microamp = <450000>;
>          constant-charge-current-max-microamp = <1000000>;
>          charge-term-current-microamp = <150000>;
>          precharge-upper-limit-microvolt = <3500000>;
>          constant-charge-voltage-max-microvolt = <4350000>;
>      };
>
>      pmic@34 {
>          compatible = "richtek,rt5033";
>          ....
>          charger {
>              compatible = "richtek,rt5033-charger";
>              monitored-battery = <&battery>;
>              extcon = <&muic>;
>          };
>      };

Yups this is how it should look :)

> Personally I would choose the current implementation for two reasons
> (possibly weak ones):

The device tree binding isn't any "implementation", and make sure
to not go into the trap that DT bindings should be done to be
convenient for any specific Linux driver to use.

> 2) At least in my mind it's still the setup for the charger. It sets up
> a the charging behavior of a certain consumer device. And the choice of
> their values is limited to the hardware of the charger. Accordingly the
> dt-bindings would say what the charger hardware is capable to do.
> Therefore I'd say it's reasonable to have those values in the charger
> node and use vendor properties.
>
> I agree to you that actually the physical battery is determining how
> these values should be set. In the end, as far as I can see, it is a
> representation thing in the devicetree. At least in our case here.

The DT bindings should reflect the hardware, and not what some
or any driver "would like to see" (to make its life easier...)

As these things are programmed into registers, clearly the
hardware is adoptable for different batteries, and the purpose
of these registers is to support different batteries. Ergo: they
belong in a battery node.

> Not sure how to proceed here. I would stick to the current
> implementation. If someone strongly prefers the "battery" representation
> style, I'm open to switch to this.

Again this is not an implementation but a hardware description.

It should use a phandle to a montored-battery and follow that to
read the battery properties.

> However, I'm not sure how the dt-bindings would look like in that case.

Just like you sketched above, just reuse simple-battery if the battery
is hardcoded into the platform, such as soldered in or has a form
factor such that no different battery can be fitted.

> Those battery properties would not be part of the RT5033 node, thus they
> basically would not be part of the RT5033 documentation. Again I think
> it makes sense to handle those properties within the charger node as
> "charger settings" properties.

Why?

This is like saying that the number of pixels on your monitor should
be part of the graphics card DT node as "configuration". And we
clearly do not do that.

Yours,
Linus Walleij

Powered by blists - more mailing lists