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:
signature.asc
Description: PGP signature
