Re: [PATCH v12 1/2] dt-bindings: adc: add AD7173

[Jonathan Cameron <jic23@xxxxxxxxxx>]

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.


> 
> > > 
> > > 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.

> 
> Cheers,
> Conor.