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.
Best regards,
Krzysztof
