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.
