linux-kernel - Re: [PATCH v4 1/2] dt-bindings: regulator: Add Richtek RTR5133 Support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250723064813.2742303-1-jeff_chang@richtek.com>
Date: Wed, 23 Jul 2025 14:47:38 +0800
From: <jeff_chang@...htek.com>
To: <krzk@...nel.org>
CC: <broonie@...nel.org>, <conor+dt@...nel.org>, <devicetree@...r.kernel.org>,
	<jeff_chang@...htek.com>, <krzk+dt@...nel.org>, <lgirdwood@...il.com>,
	<linux-kernel@...r.kernel.org>, <robh@...nel.org>
Subject: Re: [PATCH v4 1/2] dt-bindings: regulator: Add Richtek RTR5133 Support

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


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.


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