[PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[Antoniu Miclaus]

The actual hardware pin name for the supply of max31827 is vdd.
Update the dt-binding to reflect the hardware properties accordingly.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 Documentation/devicetree/bindings/hwmon/adi,max31827.yaml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
index 2dc8b07b4d3b..21f2d350373b 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
@@ -27,7 +27,7 @@ properties:
   reg:
     maxItems: 1
 
-  vref-supply:
+  vdd-supply:
     description:
       Must have values in the interval (1.6V; 3.6V) in order for the device to
       function correctly.
@@ -35,7 +35,7 @@ properties:
 required:
   - compatible
   - reg
-  - vref-supply
+  - vdd-supply
 
 additionalProperties: false
 
@@ -48,7 +48,7 @@ examples:
         temperature-sensor@42 {
             compatible = "adi,max31827";
             reg = <0x42>;
-            vref-supply = <&reg_vdd>;
+            vdd-supply = <&reg_vdd>;
         };
     };
 ...
-- 
2.42.0

[PATCH 2/2] drivers: hwmon: max31827: handle vdd regulator

[Antoniu Miclaus]

Add missing implementation for the max31827 supply regulator.
This is a hardware required property that is not handled.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/hwmon/max31827.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index 602f4e4f81ff..43709f47f518 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -427,6 +427,11 @@ static int max31827_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(st->regmap),
 				     "Failed to allocate regmap.\n");
 
+	err = devm_regulator_get_enable(dev, "vdd");
+	if (err)
+		return dev_err_probe(dev, err,
+				     "failed to enable vdd regulator\n");
+
 	err = max31827_init_client(st);
 	if (err)
 		return err;
-- 
2.42.0

Re: [PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[Rob Herring]

On Thu, Sep 21, 2023 at 05:20:03PM +0300, Antoniu Miclaus wrote:
> The actual hardware pin name for the supply of max31827 is vdd.
> Update the dt-binding to reflect the hardware properties accordingly.

Changing this breaks the ABI. I see the old one wasn't used by the 
driver, but that's just one driver potentially. You need some 
justification here why it's okay to break the ABI.

> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  Documentation/devicetree/bindings/hwmon/adi,max31827.yaml | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> index 2dc8b07b4d3b..21f2d350373b 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> @@ -27,7 +27,7 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  vref-supply:
> +  vdd-supply:
>      description:
>        Must have values in the interval (1.6V; 3.6V) in order for the device to
>        function correctly.
> @@ -35,7 +35,7 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - vref-supply
> +  - vdd-supply
>  
>  additionalProperties: false
>  
> @@ -48,7 +48,7 @@ examples:
>          temperature-sensor@42 {
>              compatible = "adi,max31827";
>              reg = <0x42>;
> -            vref-supply = <&reg_vdd>;
> +            vdd-supply = <&reg_vdd>;
>          };
>      };
>  ...
> -- 
> 2.42.0
> 

RE: [PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[Miclaus, Antoniu]

> On Thu, Sep 21, 2023 at 05:20:03PM +0300, Antoniu Miclaus wrote:
> > The actual hardware pin name for the supply of max31827 is vdd.
> > Update the dt-binding to reflect the hardware properties accordingly.
> 
> Changing this breaks the ABI. I see the old one wasn't used by the
> driver, but that's just one driver potentially. You need some
> justification here why it's okay to break the ABI.
> 
As I mentioned also in the commit description, the supply should match the
actual hardware pin name. Otherwise it might create confusion. Usually vref
refers to an external voltage reference pin used for ADC/DACs which is not
exactly the case for this part, taking into account that there is no "reference"
word mentioned in the datasheet at all. VREF and VDD are usually separate
hardware pins. There is a hint indeed in the dts example that the vref-supply 
might be referenced to a vdd regulator node, but from my point of view
that is not enough. Moreover the current vref-supply is not handled at all in
the driver, it is only mentioned in the dt-binding (That's why I added a second
patch in the series handling the supply).

If the justification is not enough to apply this change, then I can keep only the
second patch, which handles the regulator in the driver and use the  old `vref`
naming which currently appears only in the dt-binding.

Antoniu
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> >  Documentation/devicetree/bindings/hwmon/adi,max31827.yaml | 6 +++--
> -
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git
> a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> > index 2dc8b07b4d3b..21f2d350373b 100644
> > --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> > @@ -27,7 +27,7 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > -  vref-supply:
> > +  vdd-supply:
> >      description:
> >        Must have values in the interval (1.6V; 3.6V) in order for the device to
> >        function correctly.
> > @@ -35,7 +35,7 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - vref-supply
> > +  - vdd-supply
> >
> >  additionalProperties: false
> >
> > @@ -48,7 +48,7 @@ examples:
> >          temperature-sensor@42 {
> >              compatible = "adi,max31827";
> >              reg = <0x42>;
> > -            vref-supply = <&reg_vdd>;
> > +            vdd-supply = <&reg_vdd>;
> >          };
> >      };
> >  ...
> > --
> > 2.42.0
> >

Re: [PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[Guenter Roeck]

On Sat, Sep 23, 2023 at 02:19:45PM +0000, Miclaus, Antoniu wrote:
> 
> 
> > On Thu, Sep 21, 2023 at 05:20:03PM +0300, Antoniu Miclaus wrote:
> > > The actual hardware pin name for the supply of max31827 is vdd.
> > > Update the dt-binding to reflect the hardware properties accordingly.
> > 
> > Changing this breaks the ABI. I see the old one wasn't used by the
> > driver, but that's just one driver potentially. You need some
> > justification here why it's okay to break the ABI.
> > 
> As I mentioned also in the commit description, the supply should match the
> actual hardware pin name. Otherwise it might create confusion. Usually vref
> refers to an external voltage reference pin used for ADC/DACs which is not
> exactly the case for this part, taking into account that there is no "reference"
> word mentioned in the datasheet at all. VREF and VDD are usually separate
> hardware pins. There is a hint indeed in the dts example that the vref-supply 
> might be referenced to a vdd regulator node, but from my point of view
> that is not enough. Moreover the current vref-supply is not handled at all in
> the driver, it is only mentioned in the dt-binding (That's why I added a second
> patch in the series handling the supply).
> 
> If the justification is not enough to apply this change, then I can keep only the
> second patch, which handles the regulator in the driver and use the  old `vref`
> naming which currently appears only in the dt-binding.
> 

That would have been a good argument when the property was introduced, but if
there are any systems with existing bindings out there they will use the old
name and fail after this change is applied.

I don't thnk it is mandated that every system in the world would publish their
devicetree bindings in the kernel. That would not scale. So any argument along
the line of "this binding is not used" is not really a valid argument.

Guenter

Re: [PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[Nuno Sá]

On Sun, 2023-09-24 at 05:02 -0700, Guenter Roeck wrote:
> On Sat, Sep 23, 2023 at 02:19:45PM +0000, Miclaus, Antoniu wrote:
> > 
> > 
> > > On Thu, Sep 21, 2023 at 05:20:03PM +0300, Antoniu Miclaus wrote:
> > > > The actual hardware pin name for the supply of max31827 is vdd.
> > > > Update the dt-binding to reflect the hardware properties accordingly.
> > > 
> > > Changing this breaks the ABI. I see the old one wasn't used by the
> > > driver, but that's just one driver potentially. You need some
> > > justification here why it's okay to break the ABI.
> > > 
> > As I mentioned also in the commit description, the supply should match the
> > actual hardware pin name. Otherwise it might create confusion. Usually vref
> > refers to an external voltage reference pin used for ADC/DACs which is not
> > exactly the case for this part, taking into account that there is no "reference"
> > word mentioned in the datasheet at all. VREF and VDD are usually separate
> > hardware pins. There is a hint indeed in the dts example that the vref-supply 
> > might be referenced to a vdd regulator node, but from my point of view
> > that is not enough. Moreover the current vref-supply is not handled at all in
> > the driver, it is only mentioned in the dt-binding (That's why I added a second
> > patch in the series handling the supply).
> > 
> > If the justification is not enough to apply this change, then I can keep only the
> > second patch, which handles the regulator in the driver and use the  old `vref`
> > naming which currently appears only in the dt-binding.
> > 
> 
> That would have been a good argument when the property was introduced, but if
> there are any systems with existing bindings out there they will use the old
> name and fail after this change is applied.
> 

How about introducing the new property and add 'deprecated: true' to the old one. I
guess the second patch would still remain as-is. Or is this just not worth the noise?

- Nuno Sá

RE: [PATCH 1/2] dt-bindings: hwmon: max31827: use supply pin name

[Miclaus, Antoniu]

> On Sat, Sep 23, 2023 at 02:19:45PM +0000, Miclaus, Antoniu wrote:
> >
> >
> > > On Thu, Sep 21, 2023 at 05:20:03PM +0300, Antoniu Miclaus wrote:
> > > > The actual hardware pin name for the supply of max31827 is vdd.
> > > > Update the dt-binding to reflect the hardware properties accordingly.
> > >
> > > Changing this breaks the ABI. I see the old one wasn't used by the
> > > driver, but that's just one driver potentially. You need some
> > > justification here why it's okay to break the ABI.
> > >
> > As I mentioned also in the commit description, the supply should match the
> > actual hardware pin name. Otherwise it might create confusion. Usually
> vref
> > refers to an external voltage reference pin used for ADC/DACs which is not
> > exactly the case for this part, taking into account that there is no
> "reference"
> > word mentioned in the datasheet at all. VREF and VDD are usually separate
> > hardware pins. There is a hint indeed in the dts example that the vref-
> supply
> > might be referenced to a vdd regulator node, but from my point of view
> > that is not enough. Moreover the current vref-supply is not handled at all in
> > the driver, it is only mentioned in the dt-binding (That's why I added a
> second
> > patch in the series handling the supply).
> >
> > If the justification is not enough to apply this change, then I can keep only
> the
> > second patch, which handles the regulator in the driver and use the  old
> `vref`
> > naming which currently appears only in the dt-binding.
> >
> 
> That would have been a good argument when the property was introduced,
> but if
> there are any systems with existing bindings out there they will use the old
> name and fail after this change is applied.
> 
> I don't thnk it is mandated that every system in the world would publish their
> devicetree bindings in the kernel. That would not scale. So any argument
> along
> the line of "this binding is not used" is not really a valid argument.
> 
> Guenter
Will keep then only the second patch which targets the driver.
Thanks for the feedback!