Message-ID:
 <TY1PPFAEA8AB8B70D7016E46DD1E5C1F9B4EC5FA@TY1PPFAEA8AB8B7.apcprd03.prod.outlook.com>
Date: Wed, 23 Jul 2025 02:32:55 +0000
From: jeff_chang(張世佳) <jeff_chang@...htek.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: "broonie@...nel.org" <broonie@...nel.org>, "lgirdwood@...il.com"
	<lgirdwood@...il.com>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>
Subject: RE: [PATCH v4 1/2] dt-bindings: regulator: Add Richtek RTR5133
 Support

Confidential C - Richtek

Dear Krzysztof Kozlowski:

        Thanks for your reply.

        Please review my explanation for your questions.

        If I have misunderstood or if there are any parts that need correction, please let me know.

Thanks
Jeff


-----Original Message-----
From: Krzysztof Kozlowski <krzk@...nel.org>
Sent: Tuesday, July 22, 2025 5:05 PM
To: jeff_chang(張世佳) <jeff_chang@...htek.com>; lgirdwood@...il.com; broonie@...nel.org; robh@...nel.org; krzk+dt@...nel.org; conor+dt@...nel.org; linux-kernel@...r.kernel.org; devicetree@...r.kernel.org
Subject: Re: [PATCH v4 1/2] dt-bindings: regulator: Add Richtek RTR5133 Support

On 22/07/2025 10:34, jeff_chang@...htek.com wrote:
> From: Jeff Chang <jeff_chang@...htek.com>
>
> Add bindings for Richtek RT5133 IC Controlled PMIC

Subject - RTR or RT? Google tells me nothing about RTR.

--> I will fix it to RT5133 in next patch.

>
> Signed-off-by: Jeff Chang <jeff_chang@...htek.com>
> ---
>
> PATCH v4
> 1. Add commit message and also /script/checkpatch --strict to fix warning.
> 2. Using subject prefixes matching dt-binding subsystem.
> 3. Re-order patches. DT patch before driver patch.
> 4. Fix description of yaml.
> 5. Add more description for base regulator.
> 6. Drop regulator-compatible proeprty.
> 7. Add prefix for vendor property richtek,oc-shutdown-all and richtek,pgb-shutdown-all.
> 8. Add more description for shutdown-all property.
> 9. Interrupts-extended -> interrupts.
> 10. pio->gpio for proper defines.
> 11. Drop unused labels. Keep rt5133_ldo1 label for ldo7 and ldo8.
>
>  .../bindings/regulator/richtek,rt5133.yaml    | 175 ++++++++++++++++++
>  1 file changed, 175 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
> b/Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
> new file mode 100644
> index 000000000000..0da725596a87
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
> @@ -0,0 +1,175 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause %YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rt5133.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5133 PMIC Regulator
> +
> +maintainers:
> +  - ShihChia Chang <jeff_chang@...htek.com>
> +
> +description:
> +  The RT5133 is an integrated Power Management IC for portable
> +devices, featuring
> +  8 LDOs and 3 GPOs. It allows programmable output voltages,
> +soft-start times,
> +  and protections via I2C. GPO operation depends on LDO1 voltage.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - richtek,rt5133
> +
> +  reg:
> +    maxItems: 1
> +
> +  enable-gpios:
> +    maxItems: 1
> +
> +  wakeup-source: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  regulators:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      base:
> +        type: object
> +        $ref: regulator.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          Properties for base regulator which control force-off base circuit.
> +          Base circuit is the power source for LDO1~LDO6. Disabling it will
> +          reduce IQ for Chip.

I don't understand what this regulator is for. Your example is also incomplete - missing min/max constraints like voltage.

Explain, what is this output pin? I already asked for explanations. I have diagram in front of me, so explain precisely instead of sending THE SAME again - which pin is it?

Also, what is IQ? Except Intelligence Quotient?

--> Thanks to Mark's Suggestion. It's the top-level supply for LDO1 to LDO6. It is not externally visible and functions merely as an on/off switch rather than regulating voltages.
   I will update the description as follows.

   Properties for the base regulator, which is the top-level supply for LDO1 to LDO6. It functions merely as an on/off switch rather than regulating voltages.
   If none of LDO1 to LDO6 are in use, switching off the base will reduce the quiescent current.

> +
> +        properties:
> +          richtek,oc-shutdown-all:
> +            type: boolean
> +            description:
> +              Anyone of LDO is in OC state, shut down all channels to protect CHIP.
> +              Without this property, only shut down the OC LDO channel.

I don't understand this. I also do not understand why this is property of "base" not the chip itself...

So don't send next version with the same.

--> It is a protection for LDO Over Current . The Description in datasheet like below.

Anyone of LDO OC state, shut down all LDO channels
0 : LDO OC only shut down itself (default)
1 : LDO OC shut down all channels
We set it as a property of "base" for using regulator_desc -> of_parse_cb.
Should I move them to chip property? We bind it to base regulator for easily programming.

> +
> +          richtek,pgb-shutdown-all:
> +            type: boolean
> +            description:
> +              Anyone of LDO is in PGB state, shut down all channels to protect CHIP.

CHIP is an acronym? Or chip?
--> chip! I will use "chip" in next patch.

> +              Without this property, only shut down the PGB LDO channel.
> +
> +        required:
> +          - regulator-name
> +
> +    patternProperties:
> +      "^ldo([1-6])$":
> +        type: object
> +        $ref: regulator.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          Properties for single LDO regulator
> +
> +        required:
> +          - regulator-name
> +
> +      "^ldo([7-8])$":
> +        type: object
> +        $ref: regulator.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          Properties for single LDO regulator
> +
> +        properties:
> +          rt5133-ldo1-supply:

supplies do not have vendor prefixes.

> +            description: |
> +              Only for ldo7 ldo8, pvin7 and pvin8 reference design are RT5133 ldo1.
> +              If not connect to ldo1 vout, this property for pvin7 and pvin8 is necessary.

I don't understand why LDO1 supply is here.

Again, which pin is it?


--> Please refer to PVIN7 and PVIN8 in the diagram. They are the power supplies for LDO7 and LDO8, respectively. The reference for PVIN7 and PVIN8 is the VOUT of LDO1 (VOUT1).
   In the driver, we set "rt5133-ldo1" as the supply_name in the regulator_desc for LDO7 and LDO8. Users can overwrite the rt5133-ldo1-supply property according to their own layout.
   I will just use vin-supply and I will remove vendor prefixes of supplies in next version.

> +
> +        required:
> +          - regulator-name
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - wakeup-source
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      rt5133@18 {

Nothing improved.

> +        compatible = "richtek,rt5133";
> +        reg = <0x18>;
> +        wakeup-source;
> +        interrupts-extended = <&gpio 187 0x0>;

Nothing improved

--> I will update them like below.
pmic@18 {
interrupts-extended = <&gpio 187 IRQ_TYPE_LEVEL_LOW>;


Implement previous comments and respond to each of them to confirm you understood them.

>


Best regards,
Krzysztof

Powered by blists - more mailing lists