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

[Ceclan Dumitru]

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?

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