| Message ID | 20221201044643.1150870-3-saravanan@linumiz.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for mpq7932 PMIC IC | expand |
On 01/12/2022 05:46, Saravanan Sekar wrote: > Document mpq7932 power-management IC > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. Best regards, Krzysztof
On 01/12/2022 12:29, Saravanan Sekar wrote: > On 01/12/22 11:26, Krzysztof Kozlowski wrote: >> On 01/12/2022 05:46, Saravanan Sekar wrote: >>> Document mpq7932 power-management IC >>> >>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >>> --- >> >> This is a friendly reminder during the review process. >> >> It seems my previous comments were not fully addressed. Maybe my >> feedback got lost between the quotes, maybe you just forgot to apply it. >> Please go back to the previous discussion and either implement all >> requested changes or keep discussing them. >> > Hi Krzysztof, > > Thanks for your time to review and feedback. > > Here are the summary of comments on V1, I have fixed all according to my > understanding. > > > 1. Use subject prefixes matching the subsystem (git log --oneline -- ...). > > git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/ > 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC > 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC > 7f464532b05d dt-bindings: Add missing 'additionalProperties: false' > 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer > > I have used the same format of 373c0a77934c. > > 2. Does not look like you tested the bindings. Please run `make > dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > > I did run dt_binding_check on V1 but failed to notice warnings. Fixed > warning on V2 and didn't observed any warnings. > > 3. Why requiring nodename? Device schemas usually don't do that. > dropped "pattern: "pmic@[0-9a-f]{1,2}"" > > 4. regulators node is a regulator with one more regulator? Drop. > dropped "$ref: regulator.yaml# " The comment was - drop entire regulators node. Plus additional comment for the driver (and related to bindings) was that this is not hwmon but a regulator driver. Why putting regulator driver in hwmon? Best regards, Krzysztof
On Thu, 01 Dec 2022 05:46:41 +0100, Saravanan Sekar wrote: > Document mpq7932 power-management IC > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- > .../bindings/hwmon/pmbus/mps,mpq7932.yaml | 63 +++++++++++++++++++ > 1 file changed, 63 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.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/hwmon/pmbus/mps,mpq7932.example.dtb: pmic@3: regulators: Additional properties are not allowed ('buck1' was unexpected) From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221201044643.1150870-3-saravanan@linumiz.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.
On 12/1/22 03:38, Krzysztof Kozlowski wrote: > On 01/12/2022 12:29, Saravanan Sekar wrote: >> On 01/12/22 11:26, Krzysztof Kozlowski wrote: >>> On 01/12/2022 05:46, Saravanan Sekar wrote: >>>> Document mpq7932 power-management IC >>>> >>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >>>> --- >>> >>> This is a friendly reminder during the review process. >>> >>> It seems my previous comments were not fully addressed. Maybe my >>> feedback got lost between the quotes, maybe you just forgot to apply it. >>> Please go back to the previous discussion and either implement all >>> requested changes or keep discussing them. >>> >> Hi Krzysztof, >> >> Thanks for your time to review and feedback. >> >> Here are the summary of comments on V1, I have fixed all according to my >> understanding. >> >> >> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...). >> >> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/ >> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC >> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC >> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false' >> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer >> >> I have used the same format of 373c0a77934c. >> >> 2. Does not look like you tested the bindings. Please run `make >> dt_binding_check` (see >> Documentation/devicetree/bindings/writing-schema.rst for instructions). >> >> I did run dt_binding_check on V1 but failed to notice warnings. Fixed >> warning on V2 and didn't observed any warnings. >> >> 3. Why requiring nodename? Device schemas usually don't do that. >> dropped "pattern: "pmic@[0-9a-f]{1,2}"" >> >> 4. regulators node is a regulator with one more regulator? Drop. >> dropped "$ref: regulator.yaml# " > > The comment was - drop entire regulators node. > > Plus additional comment for the driver (and related to bindings) was > that this is not hwmon but a regulator driver. Why putting regulator > driver in hwmon? > Turns out this is primarily a hardware monitoring driver, like the drivers for all other PMBus chips. Regulator support is actually optional; the driver works perfectly well with CONFIG_REGULATOR=n (except that it needs some #ifdefs to address that situation). Guenter
On 01/12/22 16:37, Guenter Roeck wrote: > On 12/1/22 03:38, Krzysztof Kozlowski wrote: >> On 01/12/2022 12:29, Saravanan Sekar wrote: >>> On 01/12/22 11:26, Krzysztof Kozlowski wrote: >>>> On 01/12/2022 05:46, Saravanan Sekar wrote: >>>>> Document mpq7932 power-management IC >>>>> >>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >>>>> --- >>>> >>>> This is a friendly reminder during the review process. >>>> >>>> It seems my previous comments were not fully addressed. Maybe my >>>> feedback got lost between the quotes, maybe you just forgot to apply >>>> it. >>>> Please go back to the previous discussion and either implement all >>>> requested changes or keep discussing them. >>>> >>> Hi Krzysztof, >>> >>> Thanks for your time to review and feedback. >>> >>> Here are the summary of comments on V1, I have fixed all according to my >>> understanding. >>> >>> >>> 1. Use subject prefixes matching the subsystem (git log --oneline -- >>> ...). >>> >>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/ >>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 >>> power-management IC >>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 >>> power-management IC >>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false' >>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer >>> >>> I have used the same format of 373c0a77934c. >>> >>> 2. Does not look like you tested the bindings. Please run `make >>> dt_binding_check` (see >>> Documentation/devicetree/bindings/writing-schema.rst for instructions). >>> >>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed >>> warning on V2 and didn't observed any warnings. >>> >>> 3. Why requiring nodename? Device schemas usually don't do that. >>> dropped "pattern: "pmic@[0-9a-f]{1,2}"" >>> >>> 4. regulators node is a regulator with one more regulator? Drop. >>> dropped "$ref: regulator.yaml# " >> >> The comment was - drop entire regulators node. >> PMBUS_REGULATOR_STEP helper macro has .regulators_node = of_match_ptr("regulators"), \ regulator subsystem parse (regulator_of_get_init_node) based on regulators_node. I think it is common all the regulator/pmic dts has regulators node. >> Plus additional comment for the driver (and related to bindings) was >> that this is not hwmon but a regulator driver. Why putting regulator >> driver in hwmon? >> > > Turns out this is primarily a hardware monitoring driver, like the drivers > for all other PMBus chips. Regulator support is actually optional; the > driver > works perfectly well with CONFIG_REGULATOR=n (except that it needs some > #ifdefs to address that situation). > Here is my view, communication to MPQ7932 PMIC chip is based on pmbus specification. Now the conflict is that we have pmbus directory under hwmon subsystem, if pmbus spec implementation would have been separate like i2c-smbus then we can implement chip driver in regulator subsystem which access pmbus. pmbus_core does supports regulator framework PMBUS_REGUALTOR and I believe it is valid MPQ7932 driver implantation under pmbus directory. Kindly suggest to remove pmbus dependency on hwmon and proceed further. Thanks, Saravanan
On 01/12/2022 16:37, Guenter Roeck wrote: > On 12/1/22 03:38, Krzysztof Kozlowski wrote: >> On 01/12/2022 12:29, Saravanan Sekar wrote: >>> On 01/12/22 11:26, Krzysztof Kozlowski wrote: >>>> On 01/12/2022 05:46, Saravanan Sekar wrote: >>>>> Document mpq7932 power-management IC >>>>> >>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >>>>> --- >>>> >>>> This is a friendly reminder during the review process. >>>> >>>> It seems my previous comments were not fully addressed. Maybe my >>>> feedback got lost between the quotes, maybe you just forgot to apply it. >>>> Please go back to the previous discussion and either implement all >>>> requested changes or keep discussing them. >>>> >>> Hi Krzysztof, >>> >>> Thanks for your time to review and feedback. >>> >>> Here are the summary of comments on V1, I have fixed all according to my >>> understanding. >>> >>> >>> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...). >>> >>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/ >>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC >>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC >>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false' >>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer >>> >>> I have used the same format of 373c0a77934c. >>> >>> 2. Does not look like you tested the bindings. Please run `make >>> dt_binding_check` (see >>> Documentation/devicetree/bindings/writing-schema.rst for instructions). >>> >>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed >>> warning on V2 and didn't observed any warnings. >>> >>> 3. Why requiring nodename? Device schemas usually don't do that. >>> dropped "pattern: "pmic@[0-9a-f]{1,2}"" >>> >>> 4. regulators node is a regulator with one more regulator? Drop. >>> dropped "$ref: regulator.yaml# " >> >> The comment was - drop entire regulators node. >> >> Plus additional comment for the driver (and related to bindings) was >> that this is not hwmon but a regulator driver. Why putting regulator >> driver in hwmon? >> > > Turns out this is primarily a hardware monitoring driver, like the drivers > for all other PMBus chips. Regulator support is actually optional; the driver > works perfectly well with CONFIG_REGULATOR=n (except that it needs some > #ifdefs to address that situation). OK, this would explain location of the driver. However the bindings are saying: "Monolithic Power System MPQ7932 PMIC" and PMIC is not mainly a hwmon device, even if it has such capabilities. It might be missing description and proper title... or might be misplaced. Best regards, Krzysztof
On 01/12/2022 05:46, Saravanan Sekar wrote: > Document mpq7932 power-management IC > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- > .../bindings/hwmon/pmbus/mps,mpq7932.yaml | 63 +++++++++++++++++++ > 1 file changed, 63 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml > new file mode 100644 > index 000000000000..5f20c59eb7ff > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml > @@ -0,0 +1,63 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwmon/pmbus/mps,mpq7932.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Monolithic Power System MPQ7932 PMIC > + > +maintainers: > + - Saravanan Sekar <saravanan@linumiz.com> > + > +properties: > + compatible: > + enum: > + - mps,mpq7932 > + > + reg: > + maxItems: 1 > + > + regulators: > + type: object The comment was - drop regulators node if you have just one regulator... but it turns out you have more regulators? Then you need to follow regulator bindings. Open few and take a look. > + > + description: | > + list of regulators provided by this controller, must be named > + after their hardware counterparts BUCK[1-6] > + This misses patternProperties. > + "^buck[1-6]$": > + type: object > + $ref: regulator.yaml# > + unevaluatedProperties: false > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - regulators > + > +additionalProperties: false Best regards, Krzysztof
On 01/12/22 17:15, Krzysztof Kozlowski wrote: > On 01/12/2022 16:37, Guenter Roeck wrote: >> On 12/1/22 03:38, Krzysztof Kozlowski wrote: >>> On 01/12/2022 12:29, Saravanan Sekar wrote: >>>> On 01/12/22 11:26, Krzysztof Kozlowski wrote: >>>>> On 01/12/2022 05:46, Saravanan Sekar wrote: >>>>>> Document mpq7932 power-management IC >>>>>> >>>>>> Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> >>>>>> --- >>>>> >>>>> This is a friendly reminder during the review process. >>>>> >>>>> It seems my previous comments were not fully addressed. Maybe my >>>>> feedback got lost between the quotes, maybe you just forgot to apply it. >>>>> Please go back to the previous discussion and either implement all >>>>> requested changes or keep discussing them. >>>>> >>>> Hi Krzysztof, >>>> >>>> Thanks for your time to review and feedback. >>>> >>>> Here are the summary of comments on V1, I have fixed all according to my >>>> understanding. >>>> >>>> >>>> 1. Use subject prefixes matching the subsystem (git log --oneline -- ...). >>>> >>>> git log --oneline -- Documentation/devicetree/bindings/hwmon/pmbus/ >>>> 1ccca53618c4 dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC >>>> 373c0a77934c dt-bindings: hwmon/pmbus: Add ti,lm25066 power-management IC >>>> 7f464532b05d dt-bindings: Add missing 'additionalProperties: false' >>>> 8a36e38d8b0f dt-bindings: hwmon/pmbus: Add ti,ucd90320 power sequencer >>>> >>>> I have used the same format of 373c0a77934c. >>>> >>>> 2. Does not look like you tested the bindings. Please run `make >>>> dt_binding_check` (see >>>> Documentation/devicetree/bindings/writing-schema.rst for instructions). >>>> >>>> I did run dt_binding_check on V1 but failed to notice warnings. Fixed >>>> warning on V2 and didn't observed any warnings. >>>> >>>> 3. Why requiring nodename? Device schemas usually don't do that. >>>> dropped "pattern: "pmic@[0-9a-f]{1,2}"" >>>> >>>> 4. regulators node is a regulator with one more regulator? Drop. >>>> dropped "$ref: regulator.yaml# " >>> >>> The comment was - drop entire regulators node. >>> >>> Plus additional comment for the driver (and related to bindings) was >>> that this is not hwmon but a regulator driver. Why putting regulator >>> driver in hwmon? >>> >> >> Turns out this is primarily a hardware monitoring driver, like the drivers >> for all other PMBus chips. Regulator support is actually optional; the driver >> works perfectly well with CONFIG_REGULATOR=n (except that it needs some >> #ifdefs to address that situation). > > OK, this would explain location of the driver. However the bindings are > saying: > "Monolithic Power System MPQ7932 PMIC" > and PMIC is not mainly a hwmon device, even if it has such capabilities. > It might be missing description and proper title... or might be misplaced. > Indeed it is PMIC chip. I think this is not the first and not sure title has to be changed for hwmon subsystem. bindings/hwmon/pmbus/ti,lm25066.yaml title: National Semiconductor/Texas Instruments LM250x6/LM506x power-management ICs Thanks, Saravanan
On 01/12/2022 17:38, Saravanan Sekar wrote: >>>> Plus additional comment for the driver (and related to bindings) was >>>> that this is not hwmon but a regulator driver. Why putting regulator >>>> driver in hwmon? >>>> >>> >>> Turns out this is primarily a hardware monitoring driver, like the drivers >>> for all other PMBus chips. Regulator support is actually optional; the driver >>> works perfectly well with CONFIG_REGULATOR=n (except that it needs some >>> #ifdefs to address that situation). >> >> OK, this would explain location of the driver. However the bindings are >> saying: >> "Monolithic Power System MPQ7932 PMIC" >> and PMIC is not mainly a hwmon device, even if it has such capabilities. >> It might be missing description and proper title... or might be misplaced. >> > > Indeed it is PMIC chip. I think this is not the first and not sure title > has to be changed for hwmon subsystem. > > bindings/hwmon/pmbus/ti,lm25066.yaml > title: National Semiconductor/Texas Instruments LM250x6/LM506x > power-management ICs Then I propose to put it in regulator directory. Best regards, Krzysztof
On 01/12/22 17:43, Krzysztof Kozlowski wrote: > On 01/12/2022 17:38, Saravanan Sekar wrote: >>>>> Plus additional comment for the driver (and related to bindings) was >>>>> that this is not hwmon but a regulator driver. Why putting regulator >>>>> driver in hwmon? >>>>> >>>> >>>> Turns out this is primarily a hardware monitoring driver, like the drivers >>>> for all other PMBus chips. Regulator support is actually optional; the driver >>>> works perfectly well with CONFIG_REGULATOR=n (except that it needs some >>>> #ifdefs to address that situation). >>> >>> OK, this would explain location of the driver. However the bindings are >>> saying: >>> "Monolithic Power System MPQ7932 PMIC" >>> and PMIC is not mainly a hwmon device, even if it has such capabilities. >>> It might be missing description and proper title... or might be misplaced. >>> >> >> Indeed it is PMIC chip. I think this is not the first and not sure title >> has to be changed for hwmon subsystem. >> >> bindings/hwmon/pmbus/ti,lm25066.yaml >> title: National Semiconductor/Texas Instruments LM250x6/LM506x >> power-management ICs > > Then I propose to put it in regulator directory. > Just for clarification, should bindings put in regulator directory? Thanks, Saravanan
On 01/12/2022 17:52, Saravanan Sekar wrote: >>> >>> Indeed it is PMIC chip. I think this is not the first and not sure title >>> has to be changed for hwmon subsystem. >>> >>> bindings/hwmon/pmbus/ti,lm25066.yaml >>> title: National Semiconductor/Texas Instruments LM250x6/LM506x >>> power-management ICs >> >> Then I propose to put it in regulator directory. >> > > Just for clarification, should bindings put in regulator directory? Yes, only the bindings. Assuming of course that the PMIC is the main role of this device and it's not a MFD (then usually we put them into mfd). Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml new file mode 100644 index 000000000000..5f20c59eb7ff --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml @@ -0,0 +1,63 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/pmbus/mps,mpq7932.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Monolithic Power System MPQ7932 PMIC + +maintainers: + - Saravanan Sekar <saravanan@linumiz.com> + +properties: + compatible: + enum: + - mps,mpq7932 + + reg: + maxItems: 1 + + regulators: + type: object + + description: | + list of regulators provided by this controller, must be named + after their hardware counterparts BUCK[1-6] + + "^buck[1-6]$": + type: object + $ref: regulator.yaml# + unevaluatedProperties: false + + additionalProperties: false + +required: + - compatible + - reg + - regulators + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pmic@3 { + compatible = "mps,mpq7932"; + reg = <0x3>; + + regulators { + buck1 { + regulator-name = "buck1"; + regulator-min-microvolt = <400000>; + regulator-max-microvolt = <3587500>; + regulator-min-microamp = <460000>; + regulator-max-microamp = <7600000>; + regulator-boot-on; + }; + }; + }; + }; +...
Document mpq7932 power-management IC Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> --- .../bindings/hwmon/pmbus/mps,mpq7932.yaml | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/mps,mpq7932.yaml