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