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

[David Lechner <dlechner@xxxxxxxxxxxx>]

On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@xxxxxxxxx> wrote:
>
>
>
> 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?

Yes (with not so many blank lines).

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

Configurable pins sounds like a pinmux. Sounds a bit overkill to
specify everything for a pin-controller for one pin if no one is ever
going to use it. But I will leave it to the DT maintainers to say how
complete the bindings should be.

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

I only looked at the AD7172-2 datasheet and pin 15 is labeled
SYNC/ERROR. Maybe they are separate pins on other chips?

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

Since ERROR is an output, I would expect it to be an interrupt. The
RDY output, on the other hand, would be wired to a SPI controller with
the SPI_READY feature (I use the Linux flag name here because I'm not
aware of a corresponding devicetree flag). So I don't think the RDY
signal would be an interrupt.

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

I'm not sure. I would like to know if there is a more elegant way as well. ;-)

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

Makes sense to me.