Re: [PATCH 1/2] dt-bindings: hwmon: Add LTC4282 bindings

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2023-11-10 at 18:42 +0000, Conor Dooley wrote:
> Yo,
> 
> On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote:
> > Add bindings for the LTC4282 High Current Hot Swap Controller with I2C
> > Compatible Monitoring.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  .../bindings/hwmon/adi,ltc4282.yaml           | 228 ++++++++++++++++++
> >  MAINTAINERS                                   |   6 +
> >  2 files changed, 234 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> > b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> > new file mode 100644
> > index 000000000000..0a5d540f014e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
> > @@ -0,0 +1,228 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/adi,ltc4282.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C
> > +
> > +maintainers:
> > +  - Nuno Sa <nuno.sa@analog.com>
> > +
> > +description: |
> > +  Analog Devices LTC4282 I2C High Current Hot Swap Controller over I2C.
> > +
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ltc4282.pdf
> > +
> > +
> 
> Extra blank line here FYI.
> 

Right...

> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ltc4282
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  adi,clkout-mode:
> > +    description: |
> > +      Controls in which mode the CLKOUT PIN should work:
> > +        0 - Configures the CLKOUT pin to output the internal system clock
> > +        1 - Configures the CLKOUT pin to output the internal conversion
> > time
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> 
> I really am not a fan of these types of properties. Part of me says that
> if you're outputting clocks from this device, then you should be a clock
> controller. How do consumers of this @clkout@ pin get the rate of the
> clock?

I explained it to Guenter as he also argued about this. I'll wait for more
feedback but it's likely this will just turn into a clock provider, yes.

> I'd kinda be expecting to see a clocks property with a maxItems of 1 and
> clock-names with two, mutually exclusive, options.
> 
> The other part says, and it applies in multiple places here, that having
> integer properties with non-integer meanings is a poor ABI. I'd be vastly
> happier if the various instances in this file became enums of strings,
> or $re┤evant-unit so that a dts containing these properties is
> immediately understandable.

Well, I think you're mentioning the 'gpio-mode' 'and under/over-voltage-
dividers'. I think for both it's clear that having the relevant units is not
feasible (at least I'm not seeing a way of properly do it). As for the strings,
well, I don't have any much to argue other than:

1) It's pattern seen in a lot of bindings - yes, that's not an excuse to copy
bad/wrong things over new bindings - but, honestly, it's the first time I have
someone complaining about it so I never thought it was wrong.

2) It makes much more easier to handle the properties in the driver (yeah, I
know that, as far as you're concerned, this does not matter to you :))

So yeah, if you insist on it, no strong reasons on my side to not do it. As long
as I see some consistency down the road :)).


- Nuno Sá