| Message ID | 20240126112945.1389573-1-naresh.solanki@9elements.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: hwmon: tda38640: Add interrupt & regulator properties | expand |
On Fri, Jan 26, 2024 at 04:59:44PM +0530, Naresh Solanki wrote: > Add properties for interrupt & regulator. > Also update example. Feeling like a broken record, given I am leaving the same comments on multiple patches. The commit message needs to explain why you're doing something. I can read the diff and see what you did! > > TEST=Run below command & make sure there is no error. > make DT_CHECKER_FLAGS=-m dt_binding_check Same comment here as elsewhere. > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > --- > .../hwmon/pmbus/infineon,tda38640.yaml | 20 +++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > index ded1c115764b..2df625a8b514 100644 > --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > @@ -30,6 +30,15 @@ properties: > unconnected(has internal pull-down). > type: boolean > > + interrupts: > + maxItems: 1 > + > + regulators: > + $ref: /schemas/regulator/regulator.yaml# > + type: object > + description: | The | here is not needed, there's no formatting to preserve. From a quick check, most bindings with regulator subnodes restrict the subnode names with patternproperties. Is there a reason you have not? > + list of regulators provided by this controller. > + > required: > - compatible > - reg > @@ -38,6 +47,7 @@ additionalProperties: false > > examples: > - | > + #include <dt-bindings/interrupt-controller/irq.h> > i2c { > #address-cells = <1>; > #size-cells = <0>; > @@ -45,5 +55,15 @@ examples: > tda38640@40 { > compatible = "infineon,tda38640"; > reg = <0x40>; > + > + //interrupt-parent = <&smb_pex_cpu0_event>; Why is this commented out? Please either restore it or remove it (with justification). Thanks Conor. > + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; Blank line here please. > + regulators { > + pvnn_main_cpu0: vout0 { > + regulator-compatible = "vout0"; > + regulator-name = "pvnn_main_cpu0"; > + regulator-enable-ramp-delay = <200>; > + }; > + }; > }; > }; > > base-commit: ecb1b8288dc7ccbdcb3b9df005fa1c0e0c0388a7 > -- > 2.42.0 >
Hi, On Fri, 26 Jan 2024 at 21:54, Conor Dooley <conor@kernel.org> wrote: > > On Fri, Jan 26, 2024 at 04:59:44PM +0530, Naresh Solanki wrote: > > Add properties for interrupt & regulator. > > Also update example. > > Feeling like a broken record, given I am leaving the same comments on > multiple patches. The commit message needs to explain why you're doing > something. I can read the diff and see what you did! > > > > > TEST=Run below command & make sure there is no error. > > make DT_CHECKER_FLAGS=-m dt_binding_check > > Same comment here as elsewhere. Ack > > > > > Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> > > --- > > .../hwmon/pmbus/infineon,tda38640.yaml | 20 +++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > > index ded1c115764b..2df625a8b514 100644 > > --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml > > @@ -30,6 +30,15 @@ properties: > > unconnected(has internal pull-down). > > type: boolean > > > > + interrupts: > > + maxItems: 1 > > + > > + regulators: > > + $ref: /schemas/regulator/regulator.yaml# > > + type: object > > + description: | > > The | here is not needed, there's no formatting to preserve. Ack > > From a quick check, most bindings with regulator subnodes restrict the > subnode names with patternproperties. Is there a reason you have not? I have corrected it now & will push in next revision. regulators is expected to have subnode vout0. like below: regulators: type: object description: list of regulators provided by this controller. properties: vout0: $ref: /schemas/regulator/regulator.yaml# type: object unevaluatedProperties: false additionalProperties: false > > > + list of regulators provided by this controller. > > + > > required: > > - compatible > > - reg > > @@ -38,6 +47,7 @@ additionalProperties: false > > > > examples: > > - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > i2c { > > #address-cells = <1>; > > #size-cells = <0>; > > @@ -45,5 +55,15 @@ examples: > > tda38640@40 { > > compatible = "infineon,tda38640"; > > reg = <0x40>; > > + > > + //interrupt-parent = <&smb_pex_cpu0_event>; > > Why is this commented out? Please either restore it or remove it (with > justification). Ack. will restore it. > > Thanks > > Conor. > > > + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; > > Blank line here please. Ack Regards, Naresh > > > + regulators { > > + pvnn_main_cpu0: vout0 { > > + regulator-compatible = "vout0"; > > + regulator-name = "pvnn_main_cpu0"; > > + regulator-enable-ramp-delay = <200>; > > + }; > > + }; > > }; > > }; > > > > base-commit: ecb1b8288dc7ccbdcb3b9df005fa1c0e0c0388a7 > > -- > > 2.42.0 > >
diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml index ded1c115764b..2df625a8b514 100644 --- a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml @@ -30,6 +30,15 @@ properties: unconnected(has internal pull-down). type: boolean + interrupts: + maxItems: 1 + + regulators: + $ref: /schemas/regulator/regulator.yaml# + type: object + description: | + list of regulators provided by this controller. + required: - compatible - reg @@ -38,6 +47,7 @@ additionalProperties: false examples: - | + #include <dt-bindings/interrupt-controller/irq.h> i2c { #address-cells = <1>; #size-cells = <0>; @@ -45,5 +55,15 @@ examples: tda38640@40 { compatible = "infineon,tda38640"; reg = <0x40>; + + //interrupt-parent = <&smb_pex_cpu0_event>; + interrupts = <10 IRQ_TYPE_LEVEL_LOW>; + regulators { + pvnn_main_cpu0: vout0 { + regulator-compatible = "vout0"; + regulator-name = "pvnn_main_cpu0"; + regulator-enable-ramp-delay = <200>; + }; + }; }; };
Add properties for interrupt & regulator. Also update example. TEST=Run below command & make sure there is no error. make DT_CHECKER_FLAGS=-m dt_binding_check Signed-off-by: Naresh Solanki <naresh.solanki@9elements.com> --- .../hwmon/pmbus/infineon,tda38640.yaml | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) base-commit: ecb1b8288dc7ccbdcb3b9df005fa1c0e0c0388a7