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

[Saravanan Sekar]

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


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