Re: [PATCH v2 9/9] dt-bindings: Add documentation for rt5033 mfd, regulator and charger

[Jakob Hauser]

Hi Linus,

On 21.04.23 11:20, Linus Walleij wrote:
> On Thu, Apr 20, 2023 at 11:16 PM Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote:
...
> > 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.

OK, this is expressing a strong preference. I’ll switch to the "battery" 
node in v3.

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

The charger driver needs five parameters. And the person writing the dts 
file should know about what range and granularity is possible for those 
values. This information could be put into the description of the 
"monitored-battery", as shown below, but that's less strict and clear as 
it is currently in patch 9.

    title: Richtek RT5033 PIMC Battery Charger

    maintainers:
      - Jakob Hauser <jahau@xxxxxxxxxxxxxx>

    description:
      The battery charger of the multifunction device RT5033 has to be
      instantiated under sub-node named "charger" using the following
      format.

    properties:
      compatible:
        const: richtek,rt5033-charger

      monitored-battery:
        description: |
          Phandle to the monitored battery according to battery.yaml.
          The battery node needs to contain five parameters.

          precharge-current-microamp:
          Current of pre-charge mode. The pre-charge current levels are
          350 mA to 650 mA programmed by I2C per 100 mA.

          constant-charge-current-max-microamp:
          Current of fast-charge mode. The fast-charge current levels
          are 700 mA to 2000 mA programmed by I2C per 100 mA.

          charge-term-current-microamp:
          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.

          precharge-upper-limit-microvolt:
          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.

          constant-charge-voltage-max-microvolt:
          Battery regulation voltage of constant voltage mode. This
          voltage levels from 3.65 V to 4.4 V by I2C per 0.025 V.

      extcon:
        description:
          Phandle to the extcon device.
        maxItems: 1

    required:
      - monitored-battery

    additionalProperties: false

    examples:
      - |

        charger {
            compatible = "richtek,rt5033-charger";
            monitored-battery = <&battery>;
            extcon = <&muic>;
        };

Kind regards,
Jakob