Message-ID: <dfb902083154ef3f8c7fc3bf15852b7c372f8c60.camel@codeconstruct.com.au>
Date: Thu, 13 Feb 2025 11:03:08 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Conor Dooley <conor@...nel.org>
Cc: Naresh Solanki <naresh.solanki@...ements.com>, 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 Wed, 2025-02-12 at 18:56 +0000, Conor Dooley wrote:
> On Wed, Feb 12, 2025 at 09:13:11PM +1030, Andrew Jeffery wrote:
> > 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:
> > > > > > > > +  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.
> 
> Ye, it's not a jab at aspeed maintainership, it's about the bmc stuff in
> particular. I saw far too many warnings from Rob's bot on series with a
> version number where the submitter should know better, so the idea that
> it had not been tested in other ways wasn't exactly a stretch.

Thanks for elaborating :)

> 
> I made a mistake how I pulled these devices out of trivial-devices.yaml,
> given the existing driver didn't work with that binding, but I don't
> really see why there's a requirement for a regulator child here in the
> first place. I get it for something like the lm25066 that is a monitor
> IC that you connect a regulator to, as the regulator is a distinct
> device - but the ir38060 is a regulator that has a pmbus interface so
> both describe the same device.

Makes sense. Maybe it's best to support the existing description in
pmbus core as Rob's already suggested in another part of the thread.

Andrew

Powered by blists - more mailing lists