[v4,1/2] regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC

Message ID 1689758686-14409-2-git-send-email-alina_yu@richtek.com
State New
Headers
Series Add Richtek RTQ2208 SubPMIC support |

Commit Message

Alina Yu July 19, 2023, 9:24 a.m. UTC
  From: alinayu <alina_yu@richtek.com>

Add bindings for Richtek RTQ2208 IC controlled SubPMIC

Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
v4
- Modify filename to "richtek,rtq2208"
- Add more desciptions for "regulator-allowed-modes"
---
 .../bindings/regulator/richtek,rtq2208.yaml        | 208 +++++++++++++++++++++
 1 file changed, 208 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
  

Comments

Krzysztof Kozlowski July 19, 2023, 9:44 a.m. UTC | #1
On 19/07/2023 11:24, alina_yu@richtek.com wrote:
> From: alinayu <alina_yu@richtek.com>
> 
> Add bindings for Richtek RTQ2208 IC controlled SubPMIC
> 
> Signed-off-by: Alina Yu <alina_yu@richtek.com>
> ---
> v4
> - Modify filename to "richtek,rtq2208"
> - Add more desciptions for "regulator-allowed-modes"
> ---
>  .../bindings/regulator/richtek,rtq2208.yaml        | 208 +++++++++++++++++++++
>  1 file changed, 208 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> new file mode 100644
> index 0000000..6cc441f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> @@ -0,0 +1,208 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rtq2208-regulator.yaml#

Please test the patch before sending.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

Also, one patchset version per day... give people time to review.


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RTQ2208 SubPMIC Regulator
> +
> +maintainers:
> +  - Alina Yu <alina_yu@richtek.com>
> +
> +description: |
> +  RTQ2208 is a highly integrated power converter that offers functional safety dual
> +  multi-configurable synchronous buck converters and two LDOs.
> +
> +  Bucks support "regulator-allowed-modes" and "regulator-mode". The former defines the permitted
> +  switching operation in normal mode; the latter defines the operation in suspend to RAM mode.
> +
> +  No matter the RTQ2208 is configured to normal or suspend to RAM mode, there are two switching
> +  operation modes for all buck rails, automatic power saving mode (Auto mode) and forced continuous
> +  conduction mode (FCCM).
> +
> +  The definition of modes is in the datasheet which is available in below link
> +  and their meaning is::
> +    0 - Auto mode for power saving, which reducing the switching frequency at light load condition
> +    to maintain high frequency.
> +    1 - FCCM to meet the strict voltage regulation accuracy, which keeping constant switching frequency.
> +
> +  Datasheet will be available soon at
> +  https://www.richtek.com/assets/Products
> +
> +properties:
> +  compatible:
> +    enum:
> +      - richtek,rtq2208
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +    
> +  richtek,mtp-sel:
> +    type: boolean
> +    description:
> +      vout register selection based on this boolean value.
> +      false - Using DVS0 register setting to adjust vout
> +      true - Using DVS1 register setting to adjust vout
> +
> +  regulators:
> +    type: object
> +
> +    patternProperties:
> +      "^buck-[a-h]$":
> +        type: object
> +        $ref: regulator.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          description for buck-[a-h] regulator.
> +
> +        properties:
> +          regulator-allowed-modes:
> +            description:
> +              two buck modes in different switching accuracy.
> +              0 - Auto mode
> +              1 - FCCM
> +            items:
> +              enum: [0, 1]
> +
> +          regulator-mode:
> +            enum: [0, 1]
> +            description:
> +              describe buck initial operating mode in suspend state.

There is no such property on this level. Aren't you mixing initial one?

> +
> +      "^ldo[1-2]$":
> +        type: object
> +        $ref: regulator.yaml#

Missing unevaluatedProperties: false.

> +        description:
> +          regulator description for ldo[1-2].
> +
> +        properties:
> +          regulator-compatible:
> +            pattern: "^LDO[1-2]$"
> +
> +          richtek,fixed-uV:
> +            $ref: "/schemas/types.yaml#/definitions/uint32"

This is pointed out by schema, so standard text:
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

> +            enum: [ 900000, 1200000, 1800000, 3300000 ]
> +            description:
> +              the fixed voltage in micro volt which is decided at the factory.

I don't understand this property. Why this is different from min/max
microvolt? Plus, you use incorrect unit suffix.

> +
> +required:
> +  - compatible
> +  - reg
> +  - regulators
> +
> +unevaluatedProperties: false

Instead: additionalProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      pmic@10 {
> +        compatible = "richtek,rtq2208";
> +        reg = <0x10>;
> +        interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
> +        richtek,mtp-sel;
> +
> +        regulators {
> +         buck-a {

Wrong indentation. If you use 2 spaces, use it consistently.

> +            regulator-min-microvolt = <400000>;
> +            regulator-max-microvolt = <2050000>;
> +            regulator-allowed-modes = <0 1>;

...

> +          };
> +         ldo2 {
> +            regulator-always-on;

And three spaces here?

> +            richtek,fixed-uV = <3300000>;
> +            regulator-state-mem {
> +              regulator-on-in-suspend;
> +            };
> +          };
> +        };
> +      };
> +    };

Best regards,
Krzysztof
  
Rob Herring July 19, 2023, 10:43 a.m. UTC | #2
On Wed, 19 Jul 2023 17:24:45 +0800, alina_yu@richtek.com wrote:
> From: alinayu <alina_yu@richtek.com>
> 
> Add bindings for Richtek RTQ2208 IC controlled SubPMIC
> 
> Signed-off-by: Alina Yu <alina_yu@richtek.com>
> ---
> v4
> - Modify filename to "richtek,rtq2208"
> - Add more desciptions for "regulator-allowed-modes"
> ---
>  .../bindings/regulator/richtek,rtq2208.yaml        | 208 +++++++++++++++++++++
>  1 file changed, 208 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
 	 $id: http://devicetree.org/schemas/regulator/richtek,rtq2208-regulator.yaml
 	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1689758686-14409-2-git-send-email-alina_yu@richtek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
  
Alina Yu July 20, 2023, 8:07 a.m. UTC | #3
Hi,
Krzysztof:

On Wed, Jul 19, 2023 at 11:44:45AM +0200, Krzysztof Kozlowski wrote:
> On 19/07/2023 11:24, alina_yu@richtek.com wrote:
> > From: alinayu <alina_yu@richtek.com>
> > 
> > Add bindings for Richtek RTQ2208 IC controlled SubPMIC
> > 
> > Signed-off-by: Alina Yu <alina_yu@richtek.com>
> > ---
> > v4
> > - Modify filename to "richtek,rtq2208"
> > - Add more desciptions for "regulator-allowed-modes"

...

> > +
> > +          regulator-mode:
> > +            enum: [0, 1]
> > +            description:
> > +              describe buck initial operating mode in suspend state.
> 
> There is no such property on this level. Aren't you mixing initial one?

It's the initial mode in suspend-mem state, should I modify that like this ?
        patternProperties:
          "^regulator-state-(standby|mem|disk)$":
	    type: object
	    $ref: regulator.yaml#
	    properties:
	      regulator-mode:
	        enum: [0, 1]
		description:
                  describe byck initial operating mode in suspend state.
...

> 
> > +            enum: [ 900000, 1200000, 1800000, 3300000 ]
> > +            description:
> > +              the fixed voltage in micro volt which is decided at the factory.
> 
> I don't understand this property. Why this is different from min/max


Because ldo has fixed voltage, so I thinks I could use a property to
represent the fixed voltage directly. Do you suggest me modifying that like this:

regulator-min-microvolt = <900000>;
regulator-max-microvolt = <900000>;

Using min voltage equals to max voltage to represent fixed voltage, instead of self-defined property ?


> microvolt? Plus, you use incorrect unit suffix.

if I change "richtek,fixed-uV" to "richtek, fixed-microvolt",
will it be a correct unit suffix ?

Best regards,
Alina
  
Krzysztof Kozlowski July 20, 2023, 8:38 a.m. UTC | #4
On 20/07/2023 10:07, Alina Yu wrote:
> Hi,
> Krzysztof:
> 
> On Wed, Jul 19, 2023 at 11:44:45AM +0200, Krzysztof Kozlowski wrote:
>> On 19/07/2023 11:24, alina_yu@richtek.com wrote:
>>> From: alinayu <alina_yu@richtek.com>
>>>
>>> Add bindings for Richtek RTQ2208 IC controlled SubPMIC
>>>
>>> Signed-off-by: Alina Yu <alina_yu@richtek.com>
>>> ---
>>> v4
>>> - Modify filename to "richtek,rtq2208"
>>> - Add more desciptions for "regulator-allowed-modes"
> 
> ...
> 
>>> +
>>> +          regulator-mode:
>>> +            enum: [0, 1]
>>> +            description:
>>> +              describe buck initial operating mode in suspend state.
>>
>> There is no such property on this level. Aren't you mixing initial one?
> 
> It's the initial mode in suspend-mem state, should I modify that like this ?
>         patternProperties:
>           "^regulator-state-(standby|mem|disk)$":
> 	    type: object
> 	    $ref: regulator.yaml#
> 	    properties:
> 	      regulator-mode:
> 	        enum: [0, 1]
> 		description:
>                   describe byck initial operating mode in suspend state.

Please check how other bindings do it.

> ...
> 
>>
>>> +            enum: [ 900000, 1200000, 1800000, 3300000 ]
>>> +            description:
>>> +              the fixed voltage in micro volt which is decided at the factory.
>>
>> I don't understand this property. Why this is different from min/max
> 
> 
> Because ldo has fixed voltage, so I thinks I could use a property to
> represent the fixed voltage directly. Do you suggest me modifying that like this:
> 
> regulator-min-microvolt = <900000>;
> regulator-max-microvolt = <900000>;
> 
> Using min voltage equals to max voltage to represent fixed voltage, instead of self-defined property ?

Yes.



Best regards,
Krzysztof
  
Alina Yu July 21, 2023, 6:27 a.m. UTC | #5
Hi,
Krzysztof:

> > ...
> > 
> >>> +
> >>> +          regulator-mode:
> >>> +            enum: [0, 1]
> >>> +            description:
> >>> +              describe buck initial operating mode in suspend state.
> >>
> >> There is no such property on this level. Aren't you mixing initial one?
> > 
> > It's the initial mode in suspend-mem state, should I modify that like this ?
> >         patternProperties:
> >           "^regulator-state-(standby|mem|disk)$":
> > 	    type: object
> > 	    $ref: regulator.yaml#
> > 	    properties:
> > 	      regulator-mode:
> > 	        enum: [0, 1]
> > 		description:
> >                   describe byck initial operating mode in suspend state.
> 
> Please check how other bindings do it.
>

If I modify that like this, will it be correct ?


...
          regulator-state-mem:
	    type: object
	    $ref: regulator.yaml#
	    properties:
	      regulator-mode:
	      description:
	        describe buck initial operating mode in suspend state.
		0 - Auto mode
		1 - FCCM

Best regards,
Alina
  
Alina Yu July 24, 2023, 2:23 a.m. UTC | #6
Hi,
Krzysztof:

> > > ...
> > > 
> > >>> +
> > >>> +          regulator-mode:
> > >>> +            enum: [0, 1]
> > >>> +            description:
> > >>> +              describe buck initial operating mode in suspend state.
> > >>
> > >> There is no such property on this level. Aren't you mixing initial one?
> > > 
> > > It's the initial mode in suspend-mem state, should I modify that like this ?
> > >         patternProperties:
> > >           "^regulator-state-(standby|mem|disk)$":
> > > 	    type: object
> > > 	    $ref: regulator.yaml#
> > > 	    properties:
> > > 	      regulator-mode:
> > > 	        enum: [0, 1]
> > > 		description:
> > >                   describe byck initial operating mode in suspend state.
> > 
> > Please check how other bindings do it.
> >
> 
> If I modify that like this, will it be correct ?
> 
> 
> ...
>           regulator-state-mem:
> 	    type: object
> 	    $ref: regulator.yaml#
> 	    properties:
> 	      regulator-mode:
> 	      description:
> 	        describe buck initial operating mode in suspend state.
> 		0 - Auto mode
> 		1 - FCCM
>

Sorry, I think I didn't explain well why to add "regulator-mode".
I just want to add description for this property, for people can get its meaning when they see the yaml.
If it's optional and it's already a general property in regulator.yaml.
May I remove it from my yaml ?

Best regards,
Alina
  

Patch

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
new file mode 100644
index 0000000..6cc441f
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
@@ -0,0 +1,208 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rtq2208-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RTQ2208 SubPMIC Regulator
+
+maintainers:
+  - Alina Yu <alina_yu@richtek.com>
+
+description: |
+  RTQ2208 is a highly integrated power converter that offers functional safety dual
+  multi-configurable synchronous buck converters and two LDOs.
+
+  Bucks support "regulator-allowed-modes" and "regulator-mode". The former defines the permitted
+  switching operation in normal mode; the latter defines the operation in suspend to RAM mode.
+
+  No matter the RTQ2208 is configured to normal or suspend to RAM mode, there are two switching
+  operation modes for all buck rails, automatic power saving mode (Auto mode) and forced continuous
+  conduction mode (FCCM).
+
+  The definition of modes is in the datasheet which is available in below link
+  and their meaning is::
+    0 - Auto mode for power saving, which reducing the switching frequency at light load condition
+    to maintain high frequency.
+    1 - FCCM to meet the strict voltage regulation accuracy, which keeping constant switching frequency.
+
+  Datasheet will be available soon at
+  https://www.richtek.com/assets/Products
+
+properties:
+  compatible:
+    enum:
+      - richtek,rtq2208
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    
+  richtek,mtp-sel:
+    type: boolean
+    description:
+      vout register selection based on this boolean value.
+      false - Using DVS0 register setting to adjust vout
+      true - Using DVS1 register setting to adjust vout
+
+  regulators:
+    type: object
+
+    patternProperties:
+      "^buck-[a-h]$":
+        type: object
+        $ref: regulator.yaml#
+        unevaluatedProperties: false
+        description:
+          description for buck-[a-h] regulator.
+
+        properties:
+          regulator-allowed-modes:
+            description:
+              two buck modes in different switching accuracy.
+              0 - Auto mode
+              1 - FCCM
+            items:
+              enum: [0, 1]
+
+          regulator-mode:
+            enum: [0, 1]
+            description:
+              describe buck initial operating mode in suspend state.
+
+      "^ldo[1-2]$":
+        type: object
+        $ref: regulator.yaml#
+        description:
+          regulator description for ldo[1-2].
+
+        properties:
+          regulator-compatible:
+            pattern: "^LDO[1-2]$"
+
+          richtek,fixed-uV:
+            $ref: "/schemas/types.yaml#/definitions/uint32"
+            enum: [ 900000, 1200000, 1800000, 3300000 ]
+            description:
+              the fixed voltage in micro volt which is decided at the factory.
+
+required:
+  - compatible
+  - reg
+  - regulators
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      pmic@10 {
+        compatible = "richtek,rtq2208";
+        reg = <0x10>;
+        interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
+        richtek,mtp-sel;
+
+        regulators {
+         buck-a {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-b {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-c {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-d {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-e {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-f {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-g {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         buck-h {
+            regulator-min-microvolt = <400000>;
+            regulator-max-microvolt = <2050000>;
+            regulator-allowed-modes = <0 1>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+              regulator-mode = <1>;
+            };
+          };
+         ldo1 {
+            richtek,fixed-uV = <1200000>;
+            regulator-always-on;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+            };
+          };
+         ldo2 {
+            regulator-always-on;
+            richtek,fixed-uV = <3300000>;
+            regulator-state-mem {
+              regulator-on-in-suspend;
+            };
+          };
+        };
+      };
+    };