Date: Mon, 22 Jan 2024 17:48:44 +0000
From: Conor Dooley <conor@...nel.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Ceclan Dumitru <mitrutzceclan@...il.com>, linus.walleij@...aro.org,
	brgl@...ev.pl, andy@...nel.org, linux-gpio@...r.kernel.org,
	Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Michael Walle <michael@...le.cc>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Arnd Bergmann <arnd@...db.de>, ChiaEn Wu <chiaen_wu@...htek.com>,
	Niklas Schnelle <schnelle@...ux.ibm.com>,
	Leonard Göhrs <l.goehrs@...gutronix.de>,
	Mike Looijmans <mike.looijmans@...ic.nl>,
	Haibo Chen <haibo.chen@....com>,
	Hugo Villeneuve <hvilleneuve@...onoff.com>,
	David Lechner <dlechner@...libre.com>,
	Ceclan Dumitru <dumitru.ceclan@...log.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 1/2] dt-bindings: adc: add AD7173

On Sun, Jan 21, 2024 at 03:41:18PM +0000, Jonathan Cameron wrote:
> On Thu, 18 Jan 2024 16:06:29 +0000
> Conor Dooley <conor@...nel.org> wrote:
> > On Thu, Jan 18, 2024 at 05:51:20PM +0200, Ceclan Dumitru wrote:
> > > On 1/18/24 17:23, Conor Dooley wrote:  
> > > > On Thu, Jan 18, 2024 at 02:49:22PM +0200, Dumitru Ceclan wrote:  
> > > >> +  adi,clock-select:
> > > >> +    description: |
> > > >> +      Select the ADC clock source. Valid values are:
> > > >> +      int         : Internal oscillator
> > > >> +      int-out     : Internal oscillator with output on XTAL2 pin
> > > >> +      ext-clk     : External clock input on XTAL2 pin
> > > >> +      xtal        : External crystal on XTAL1 and XTAL2 pins
> > > >> +
> > > >> +    $ref: /schemas/types.yaml#/definitions/string
> > > >> +    enum:
> > > >> +      - int
> > > >> +      - int-out
> > > >> +      - ext-clk
> > > >> +      - xtal
> > > >> +    default: int  
> > > > I am not a fan of properties like this one, that in my view reimplement
> > > > things that are supported by the regular clocks properties. I've got
> > > > some questions for you so I can understand whether or not this custom
> > > > property is required.
> > > > 
> > > > Whether or not the ext-clk or xtal is used is known based on
> > > > clock-names - why is the custom property required to determine that?  
> > 
> > > > If neither of those clocks are present, then the internal clock would be
> > > > used. Choosing to use the internal clock if an external one is provided
> > > > sounds to me like a software policy decision made by the operating
> > > > system.  
> > > 
> > > If there was no int-out, sure. I considered that the choice between int
> > > and int-out could be made here. So better for driver to choose int/int-out?  
> > 
> > This part of my comments was specifically about choosing between use of
> > the internal clock when ext-clk or xtal are provided, which I think
> > excludes the possibility of using int-out, since the XTAL2 pin is an
> > input.
> > 
> > There's 3 situations:
> > - no external clock provided
> > - ext-clk provided
> > - xtal provided
> > 
> > For the former, you know you're in that state when no "clocks" property
> > is present. The latter two you can differentiate based on "clock-names".
> > 
> > Choosing to use the internal clock if an external clock is provided
> > seems to be a software policy decision, unless I am mistaken.
> 
> Agreed, though it rarely makes sense as if someone put down a precision
> clock they normally wanted you to use it!
> 
> So as a general rule we don't both providing policy controls beyond if
> there is extra hardware (external clock source) then use that.
> 
> If someone has a good reason to want to do something else then we can
> probably figure out a reasonable way to control it.

Yah, I figured there'd be few situations, outside of maybe debugging
hardware issues, where that internal clock is more desirable to use.

> > > > Finally, if the ADC has a clock output, why can that not be represented
> > > > by making the ADC a clock-controller?
> > > >   
> > > 
> > > Was not familiar with this/did not cross my mind. So if xtal/ext-clk is
> > > present, the driver should detect it and disable the option for clock
> > > output? (Common pin XTAL2)  
> > 
> > Yeah, if those clocks are provided you would not register as a clock
> > controller. If there is a user of the output clock, it should have its
> > own "clocks" property that references the ADC's output.
> > 
> > Your dt-binding could also make clocks/clock-names & clock-controller
> > mutually exclusive.
> 
> That would indeed be the nicest solution.  How this has been done
> in drivers has somewhat 'evolved' over time, but this is the nicest
> option from point of view of standard bindings and clarity over what
> is going on.

Yeah, I know that this has not really been normal thing to do in some
corners of the kernel (ethernet PHYs in particular I think have been
rolling their own clock controller stuff) and I've been trying to push
back on these kinds of things for new devices.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists