Re: [PATCH v2 2/4] dt-bindings: hwmon/pmbus: Add mps,mpq7932 power-management IC

From: Saravanan Sekar
Date: Thu Dec 01 2022 - 12:31:27 EST


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@xxxxxxxxxxx>
---

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