lkml.org 
[lkml]   [2023]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 9/9] dt-bindings: Add documentation for rt5033 mfd, regulator and charger
    From
    Hi Linus!

    On 20.04.23 10:03, Linus Walleij wrote:
    > On Thu, Apr 20, 2023 at 9:59 AM Linus Walleij <linus.walleij@linaro.org> wrote:
    >>
    >> Hi Jakob,
    >>
    >> thanks for your patch!
    >>
    >> The following caught my eye:
    >>
    >> On Sun, Apr 16, 2023 at 2:50 PM Jakob Hauser <jahau@rocketmail.com> wrote:
    >>
    >>> Add device tree binding documentation for rt5033 multifunction device, voltage
    >>> regulator and battery charger.
    >>>
    >>> Cc: Beomho Seo <beomho.seo@samsung.com>
    >>> Cc: Chanwoo Choi <cw00.choi@samsung.com>
    >>> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
    >>> ---
    >>> The patch is based on linux-next (tag "next-20230413").
    >> (...)
    >>> --- /dev/null
    >>> +++ b/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml
    >> (...)
    >>> + richtek,pre-microamp:
    >>> + description:
    >>> + Current of pre-charge mode. The pre-charge current levels are 350 mA to
    >>> + 650 mA programmed by I2C per 100 mA.
    >>> + maxItems: 1
    >>> +
    >>> + richtek,fast-microamp:
    >>> + description:
    >>> + Current of fast-charge mode. The fast-charge current levels are 700 mA
    >>> + to 2000 mA programmed by I2C per 100 mA.
    >>> + maxItems: 1
    >>> +
    >>> + richtek,eoc-microamp:
    >>> + description:
    >>> + This property is end of charge current. Its level ranges from 150 mA to
    >>> + 600 mA. Between 150 mA and 300 mA in 50 mA steps, between 300 mA and 600 mA
    >>> + in 100 mA steps.
    >>> + maxItems: 1
    >>> +
    >>> + richtek,pre-threshold-microvolt:
    >>> + description:
    >>> + Voltage of pre-charge mode. If the battery voltage is below the pre-charge
    >>> + threshold voltage, the charger is in pre-charge mode with pre-charge current.
    >>> + Its levels are 2.3 V to 3.8 V programmed by I2C per 0.1 V.
    >>> + maxItems: 1
    >>> +
    >>> + richtek,const-microvolt:
    >>> + description:
    >>> + Battery regulation voltage of constant voltage mode. This voltage levels from
    >>> + 3.65 V to 4.4 V by I2C per 0.025 V.
    >>> + maxItems: 1
    >>
    >> These are very generic currents and voltages, and their usage is well known
    >> and generic. So they should not be prefixed "richtek,".
    >>
    >> Use the properties already defined in
    >> Documentation/devicetree/bindings/power/supply/battery.yaml
    >> for these:
    >>
    >> precharge-current-microamp
    >> constant-charge-current-max-microamp
    >> charge-term-current-microamp
    >> precharge-upper-limit-microvolt
    >> constant-charge-voltage-max-microvolt
    >>
    >> Please double-check, I think those are the ones you need.
    >>
    >> Perhaps it is possible to just $ref these properties directly and add
    >> the additional restrictions on top.
    >
    > 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 think your charger should contain a phandle to a battery and the battery
    > node should contain these limits.
    >
    > monitored-battery:
    > $ref: /schemas/types.yaml#/definitions/phandle
    > description: phandle to battery node
    >
    > Then you can just use the standard battery bindings for these properties
    > on the battery.
    >
    > See for example:
    > Documentation/devicetree/bindings/power/supply/stericsson,ab8500-charger.yaml
    >
    > There will be driver changes needed too, but this will be way cleaner.
    >
    > Yours,
    > Linus Walleij

    These are interesting hints!

    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.

    Hardware-wise, the mfd (incl. charger) and fuel gauge are on different
    I2C lines. Therefore, the different drivers access different registers.

    Charger registers:
    https://github.com/torvalds/linux/blob/v6.3-rc7/include/linux/mfd/rt5033-private.h#L13-L23

    Fuel gauge registers:
    https://github.com/torvalds/linux/blob/v6.3-rc7/include/linux/mfd/rt5033-private.h#L215-L243

    The things being set or read in those registers kind of determine which
    things are done in which driver.

    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. The fuel gauge, on the other hand,
    returns information like actual voltage and percentage.

    The only thing that seems not placed well is the "status" property like
    charging/discharging/not-charging/etc. At RT5033 this information is
    within the charger register. Userspace layer "UPower" expects this from
    the "battery" device. Patch 8 of this series v2 carries that property
    from the charger over to the fuel gauge. Though this is a bit of a
    quirk. And it creates dependencies between two drivers which actually
    would be independent from each other.

    ---------------------------------

    Back to the topic. When we talk about "battery" here, it seems to me
    that it's about the representation in the devicetree.

    Currently it is:

    pmic@34 {
    compatible = "richtek,rt5033";
    ....
    charger {
    compatible = "richtek,rt5033-charger";
    richtek,pre-microamp = <450000>;
    richtek,fast-microamp = <1000000>;
    richtek,eoc-microamp = <150000>;
    richtek,pre-threshold-microvolt = <3500000>;
    richtek,const-microvolt = <4350000>;
    extcon = <&muic>;
    };
    };

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

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

    1) The original author of the driver and documentation is Beomho Seo. I
    tried to preserve the original structure as far as possible. This is
    probably rather a question of editing than a technical one.

    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.

    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.

    However, I'm not sure how the dt-bindings would look like in that case.
    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.

    Kind regards,
    Jakob
    \
     
     \ /
      Last update: 2023-04-20 23:17    [W:4.850 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site