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

[Ceclan Dumitru <mitrutzceclan@xxxxxxxxx>]

On 12/12/23 17:09, David Lechner wrote:
> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@xxxxxxxxx> wrote:
>>
>> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
>> which can be used in high precision, low noise single channel applications
>> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
>> primarily for measurement of signals close to DC but also delivers
>> outstanding performance with input bandwidths out to ~10kHz.
> 
> As stated in [1], we should try to make complete bindings. I think
> more could be done here to make this more complete. Most notably, the
> gpio-controller binding is missing. Also maybe something is needed to
> describe how the SYNC/ERROR pin is wired up since it can be an input
> or an output with different functions?
> 

GPIO-controller:
  '#gpio-cells':

    const: 2


  gpio-controller: true
Like this, in properties?

Sync can only be an output, Error is configurable. Are there any
examples for how something like this is described?

...

>> +  interrupts:
>> +    maxItems: 1
> 
> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> pin. Although I could see how RDY could be considered part of the SPI
> bus. In any case, a description explaining what the interrupt is would
> be useful.
> 

I do not see how there could be 2 interrupts. DOUT/RDY is used as an
interrupt when waiting for a conversion to finalize.

Sync and Error are sepparate pins, Sync(if enabled) works only as an
input that resets the modulator and the digital filter.

Error can be configured as input, output or ERROR output (OR between all
internal error sources).

Would this be alright
  interrupts:

    description: Conversion completion interrupt.
		 Pin is shared with SPI DOUT.
    maxItems: 1

...

>> +
>> +patternProperties:
>> +  "^channel@[0-9a-f]$":
>> +    type: object
>> +    $ref: adc.yaml
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      reg:
>> +        minimum: 0
>> +        maximum: 15
>> +
>> +      diff-channels:
>> +        items:
>> +          minimum: 0
>> +          maximum: 31
> 
> Do we need to add overrides to limit the maximums for each compatible string?
> 

Just to be sure, in the allOf section?
If yes, is there any other more elegant method to obtain this behavior?

...

>> +
>> +    required:
>> +      - reg
>> +      - diff-channels
> 
> Individual analog inputs can be used as single-ended or in pairs as
> differential, right? If so, diff-channels should not be required to
> allow for single-ended use.
> 
> And we would need to add something like a single-ended-channel
> property to adc.yaml to allow mapping analog input pins to channels
> similar to how diff-channels works, I think (I don't see anything like
> that there already)?
> 
> So maybe something like:
> 
> oneOf:
>   - required:
>       single-ended-channel
>   - required:
>       diff-channels
> 
All channels must specify 2 analog input sources, there is no input
source wired by default to AVSS.

In my opinion, there is no need to specify channels as single-ended
because that would require a property that specifies the input that is
wired to AVSS.