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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]

On Mon, Nov 13, 2023 at 10:32:17AM +0100, Nuno Sá wrote:
> On Fri, 2023-11-10 at 18:42 +0000, Conor Dooley wrote:
> > On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote:

> > > +  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:

Yeah, sorry - I was kinda making a general point there about not liking
having integer values mapped to some sort of meaning, when units or some
other sort of more meaningful property is possible.
I really do not like these sorts of properties that you go and put
"gpio-mode = <3>;" or whatever in the devicetree.
I know its not quite units, but you could use 5, 10 & 15 as the
allowable values for the divider property and I think that'd be fine.

Oh and now that I think of it - for the divider property, how does
"adi,undervoltage-dividers = 0" differ from omitting the property
altogether? It's not entirely apparently from the binding what that
actually means. If they do differ, I think you need to mention what
the omission of the property means, and if they do not, then that = 0
case should be removed IMO.

> 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 :))

Yeah, with one hat on, sure, I don't care. Realistically I am aware that
having these integers makes your life a little easier though.
> 
> 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 :)).

From me at least, I try to push people away from these sorts of integer
properties and will continue to do so.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]