Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <1654581161-12349-1-git-send-email-u0084500@gmail.com>
 <1654581161-12349-2-git-send-email-u0084500@gmail.com> <96ecca0b-b65c-749d-d66b-33443cacf2e4@linaro.org>
In-Reply-To: <96ecca0b-b65c-749d-d66b-33443cacf2e4@linaro.org>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Wed, 8 Jun 2022 10:52:44 +0800
Message-ID: <CADiBU39BTr9FjtXgBe55aOTHNVotHfC1n=aHrH3XAcVoWkk8sA@mail.gmail.com>
Subject: Re: [PATCH 1/4] dt-binding: mfd: Add Richtek RT5120 PMIC support
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Lee Jones <lee.jones@linaro.org>,
        Mark Brown <broonie@kernel.org>, dmitry.torokhov@gmail.com,
        Liam Girdwood <lgirdwood@gmail.com>,
        cy_huang <cy_huang@richtek.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        linux-input@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> =E6=96=BC 2022=E5=B9=
=B46=E6=9C=887=E6=97=A5 =E9=80=B1=E4=BA=8C =E4=B8=8B=E5=8D=887:52=E5=AF=AB=
=E9=81=93=EF=BC=9A
>
> On 07/06/2022 07:52, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add Richtek RT5120 PMIC devicetree document.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  .../devicetree/bindings/mfd/richtek,rt5120.yaml    | 180 +++++++++++++=
++++++++
> >  1 file changed, 180 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt512=
0.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml =
b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> > new file mode 100644
> > index 00000000..376bf73
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/richtek,rt5120.yaml
> > @@ -0,0 +1,180 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/richtek,rt5120.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT5120 PMIC
> > +
> > +maintainers:
> > +  - ChiYuan Huang <cy_huang@richtek.com>
> > +
> > +description: |
> > +  The RT5120 provides four high-efficiency buck converters and one LDO=
 voltage
> > +  regulator. The device is targeted at providingthe processor voltage,=
 memory,
> > +  I/O, and peripheral rails in home entertainment devices. The I2C int=
erface is
> > +  used for dynamic voltage scaling of the processor voltage, power rai=
ls on/off
> > +  sequence control, operation mode selection.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - richtek,rt5120
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> > +  wakeup-source: true
> > +
> > +  richtek,enable-undervolt-hiccup:
> > +    type: boolean
> > +    description: |
> > +      If used, under voltage protection trigger hiccup behavior, else =
latchup as
> > +      default
> > +
> > +  richtek,enable-overvolt-hiccup:
> > +    type: boolean
> > +    description:
> > +      Like as 'enable-uv-hiccup', it configures over voltage protectio=
n to
> > +      hiccup, else latchup as default
> > +
> > +  vin1-supply:
> > +    description: phandle for buck1 input power source
> > +
> > +  vin2-supply:
> > +    description: phandle for buck2 input power source
> > +
> > +  vin3-supply:
> > +    description: phandle for buck3 input power source
> > +
> > +  vin4-supply:
> > +    description: phandle for buck4 input power source
> > +
> > +  vinldo-supply:
> > +    description: phandle for ldo input power source
> > +
> > +  regulators:
> > +    type: object
> > +
> > +    patternProperties:
> > +      "^buck[1-4]$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
> > +
> > +        properties:
> > +          regulator-allowed-modes:
> > +            description: |
> > +              Used to specify the allowed buck converter operating mod=
e
> > +              mode mapping:
> > +                0: auto mode
> > +                1: force pwm mode
> > +            items:
> > +              enum: [0, 1]
> > +
> > +        unevaluatedProperties: false
>
> Better to put it after '$ref' for readability.
OK, Fix in next
>
> > +
> > +      "^(ldo|exten)$":
> > +        type: object
> > +        $ref: /schemas/regulator/regulator.yaml#
>
> You need here unevaluatedProperties:false as well (for the ldo/exten
> properties)
Fix in next.
>
> > +
> > +    additionalProperties: false
> > +
> > +  powerkey:
> > +    type: object
> > +    description:
> > +      The power key driver may be optional. If not used, change node s=
tatus to
> > +      'disabled'
>
> This description is not helpful, does not describe the hardware. Please
> describe hardware, not Devicetree usage.
That's because it's a PMIC. Power key is also connected to it.
For power key press, all power rails will start to power up.
But in the application, there may be other PMIC that's also connected
to power key.
That's why this power key driver may need to be optional.
One system only need one driver to report the power key status.

Currently in some linux OS, it uses the auto module loading mechanism.
All kernel module files may be all the same, but it uses the
devicetree to decide how many devices
need to be declared. Since RT5120 power key device may be optional,
following by mfd_add_device, if of_node is
found, and status is "disabled", the sub device would be skipped.

Actually, I'm also confused about it. There may be three ways to implement =
it
1. not to build this kernel module -> seems to violate my above application
2. Use one boolean property to decide power key cell need to be used or not=
??
3. like as now, use the node status to decide it.

Is there the better way to do it?
>
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - richtek,rt5120-pwrkey
> > +
> > +    required:
> > +      - compatible
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - '#interrupt-cells'
> > +  - interrupt-controller
> > +  - regulators
> > +  - powerkey
>
> You wrote powerkey is optional... so the node should not be required, rig=
ht?
Yes, required. Please refer to the above explanation.
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2c {
> > +      #address-cells =3D <1>;
> > +      #size-cells =3D <0>;
> > +
> > +      pmic@62 {
> > +        compatible =3D "richtek,rt5120";
> > +        reg =3D <0x62>;
> > +        interrupts-extended =3D <&gpio_intc 32 IRQ_TYPE_LEVEL_LOW>;
> > +        interrupt-controller;
> > +        #interrupt-cells =3D <1>;
> > +        wakeup-source;
> > +
> > +        regulators {
> > +          buck1 {
> > +            regulator-name =3D "rt5120-buck1";
> > +            regulator-min-microvolt =3D <600000>;
> > +            regulator-max-microvolt =3D <1393750>;
> > +            regulator-allowed-modes =3D <0 1>;
> > +            regulator-boot-on;
> > +          };
> > +          buck2 {
> > +            regulator-name =3D "rt5120-buck2";
> > +            regulator-min-microvolt =3D <1100000>;
> > +            regulator-max-microvolt =3D <1100000>;
> > +            regulator-allowed-modes =3D <0 1>;
> > +            regulator-always-on;
> > +          };
> > +          buck3 {
> > +            regulator-name =3D "rt5120-buck3";
> > +            regulator-min-microvolt =3D <1800000>;
> > +            regulator-max-microvolt =3D <1800000>;
> > +            regulator-allowed-modes =3D <0 1>;
> > +            regulator-always-on;
> > +          };
> > +          buck4 {
> > +            regulator-name =3D "rt5120-buck4";
> > +            regulator-min-microvolt =3D <3300000>;
> > +            regulator-max-microvolt =3D <3300000>;
> > +            regulator-allowed-modes =3D <0 1>;
> > +            regulator-always-on;
> > +          };
> > +          ldo {
> > +            regulator-name =3D "rt5120-ldo";
> > +            regulator-min-microvolt =3D <1800000>;
> > +            regulator-max-microvolt =3D <1800000>;
> > +            regulator-always-on;
> > +          };
> > +          exten {
> > +            regulator-name =3D "rt5120-exten";
> > +            regulator-min-microvolt =3D <3000000>;
> > +            regulator-max-microvolt =3D <3000000>;
> > +            regulator-always-on;
> > +          };
> > +        };
> > +        powerkey {
> > +                status =3D "okay";
>
> Messed up indentation. No need for status in examples.
Fix in next.
>
> > +                compatible =3D "richtek,rt5120-pwrkey";
> > +        };
> > +      };
> > +    };
>
>
> Best regards,
> Krzysztof