Message-ID: <4619661d7375c71710a22520f6ebbf353a5aff59.camel@codeconstruct.com.au>
Date: Wed, 12 Feb 2025 21:13:11 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Conor Dooley <conor@...nel.org>, Naresh Solanki
	 <naresh.solanki@...ements.com>
Cc: Guenter Roeck <linux@...ck-us.net>, broonie@...nel.org, Jean Delvare
 <jdelvare@...e.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Liam Girdwood
 <lgirdwood@...il.com>, linux-hwmon@...r.kernel.org,
 devicetree@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: hwmon: ir38060: Move & update dt
 binding

On Thu, 2025-02-06 at 18:09 +0000, Conor Dooley wrote:
> On Thu, Feb 06, 2025 at 09:23:03PM +0530, Naresh Solanki wrote:
> > On Thu, 6 Feb 2025 at 01:43, Conor Dooley <conor@...nel.org> wrote:
> > > On Wed, Feb 05, 2025 at 03:51:25PM +0530, Naresh Solanki wrote:
> > > > On Wed, 5 Feb 2025 at 00:52, Conor Dooley <conor@...nel.org>
> > > > wrote:
> > > > > On Tue, Feb 04, 2025 at 11:33:03PM +0530, Naresh Solanki
> > > > > wrote:
> > > > > > Move dt binding under hwmon/pmbus & align accordingly.
> > > > > > 
> > > > > > Signed-off-by: Naresh Solanki
> > > > > > <naresh.solanki@...ements.com>
> > > > > > ---
> > > > > >  .../hwmon/pmbus/infineon,ir38060.yaml         | 61
> > > > > > +++++++++++++++++++
> > > > > >  .../bindings/regulator/infineon,ir38060.yaml  | 45 -------
> > > > > > -------
> > > > > >  2 files changed, 61 insertions(+), 45 deletions(-)
> > > > > >  create mode 100644
> > > > > > Documentation/devicetree/bindings/hwmon/pmbus/infineon,ir38
> > > > > > 060.yaml
> > > > > >  delete mode 100644
> > > > > > Documentation/devicetree/bindings/regulator/infineon,ir3806
> > > > > > 0.yaml
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,ir
> > > > > > 38060.yaml
> > > > > > b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,ir
> > > > > > 38060.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..e1f683846a54
> > > > > > --- /dev/null
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,ir
> > > > > > 38060.yaml
> > > > > > @@ -0,0 +1,61 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id:
> > > > > > http://devicetree.org/schemas/hwmon/pmbus/infineon,ir38060.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Infineon Buck Regulators with PMBUS interfaces
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Not Me.
> > > > > 
> > > > > How the hell did this get merged!
> > > > > 
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - infineon,ir38060
> > > > > > +      - infineon,ir38064
> > > > > > +      - infineon,ir38164
> > > > > > +      - infineon,ir38263
> > > > > > +
> > > > > > +  reg:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  regulators:
> > > > > > +    type: object
> > > > > > +    description:
> > > > > > +      list of regulators provided by this controller.
> > > > > 
> > > > > Can you explain why this change is justified? Your commit
> > > > > message is
> > > > > explaining what you're doing but not why it's okay to do.
> > > 
> > > > This is based on other similar dt-bindings under hwmon/pmbus.
> > > 
> > > Okay, but what I am looking for is an explanation of why it is
> > > okay to
> > > change the node from
> > > 
> > > > regulator@34 {
> > > >   compatible = "infineon,ir38060";
> > > >   reg = <0x34>;
> > > > 
> > > >   regulator-min-microvolt = <437500>;
> > > >   regulator-max-microvolt = <1387500>;
> > > > };
> > As I have understood the driver, this isn't supported.
> > > 
> > > to
> > > 
> > > > regulator@34 {
> > > >     compatible = "infineon,ir38060";
> > > >     reg = <0x34>;
> > > > 
> > > >     regulators {
> > > >         vout {
> > > >             regulator-name = "p5v_aux";
> > > >             regulator-min-microvolt = <437500>;
> > > >             regulator-max-microvolt = <1387500>;
> > > >         };
> > > >     };
> > Above is the typical approach in other pmbus dt bindings.
> > Even pmbus driver expects this approach.
> > > 
> > > ?
> > > 
> > > Will the driver handle both of these identically? Is backwards
> > > compatibility with the old format maintained? Was the original
> > > format
> > > wrong and does not work? Why is a list of regulators needed when
> > > the
> > > device only provides one?
> > Driver doesn't support both.
> > Based on the pmbus driver original format was wrong.
> > pmbus driver looks for a regulator node to start with.
> > 
> > Reference:
> > https://github.com/torvalds/linux/blob/master/drivers/hwmon/pmbus/pmbus.h#L515
> 
> Then all of the in-tree users are all just broken? They're in aspeed
> bmcs, so I would not be surprised at all if that were the case.

Can you unpack the intent of this remark for me a little?

The history of the problem from what I can see looks like:

   1. pmbus regulator support exploiting "regulators" as an OF child
      node was merged for 3.19[1]
   2. The infineon driver support was merged with trivial bindings[2]
      and released in v5.17
   3. A patch was proposed that extracted the Infineon regulator
      compatibles and provided a dedicated binding[3], however it
      lacked the "regulators" object property 
   4. The patch in 3 was merged as [4] with relevant tags, and was
      released in v6.9
   5. The system1 devicetree was merged and released in v6.10, and sbp1
      is merged in v6.14-rc1 for release in v6.14. Both devicetrees, as
      far as I'm aware, conform to the binding as written.

In addition to keeping an eye out for Rob's bot, I check all Aspeed-
related devicetree patches against the bindings using the usual tooling
while applying them. I would like to avoid diving into driver
implementations as a blocker to applying devicetree patches where
possible - the formalised bindings and tooling should exist to separate
us from having to do that.

If the complaint is that people submitting Aspeed devicetree patches
are regularly not testing them to make sure they behave correctly on
hardware, then sure, that's something to complain about. Otherwise, I'm
well aware of the (Aspeed) bindings and warnings situation; we've
spoken about it previously. If there's something I should change in my
process (beyond eventually addressing all the warnings) please let me
know, but I don't see that there is in this specific instance.

Andrew

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ddbb4db4ced1ba784fcd3500179a7291b6c5d7b7
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ca003af3aa1574646b784abee861626a52d345ea
[3]: https://lore.kernel.org/all/20240223-blabber-obnoxious-353e519541a6@spud/
[4]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bad582f9879812bcf74adb520e005051eb021cfb

Powered by blists - more mailing lists