Re: [PATCH v5 01/11] dt-bindings: iio: adc: Add AD4170

[Jonathan Cameron <jic23@kernel.org>]

> >> +
> >> +$defs:
> >> +  reference-buffer:
> >> +    description: |
> >> +      Enable precharge buffer, full buffer, or skip reference buffering of
> >> +      the positive/negative voltage reference. Because the output impedance
> >> +      of the source driving the voltage reference inputs may be dynamic,
> >> +      resistive/capacitive combinations of those inputs can cause DC gain
> >> +      errors if the reference inputs go unbuffered into the ADC. Enable
> >> +      reference buffering if the provided reference source has dynamic high
> >> +      impedance output. Note the absolute voltage allowed on REFINn+ and REFINn-
> >> +      inputs is from AVSS - 50 mV to AVDD + 50 mV when the reference buffers are
> >> +      disabled but narrows to AVSS to AVDD when reference buffering is enabled
> >> +      or in precharge mode. The valid options for this property are:
> >> +      0: Reference precharge buffer.
> >> +      1: Full reference buffering.
> >> +      2: Bypass reference buffers (buffering disabled).
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [0, 1, 2]
> >> +    default: 1  
> > 
> > Why make this property a uint32, rather than a string where you can use
> > something like "precharge", "full" and "bypass" (or "disabled")? The
> > next similar device could use something slightly different then the
> > binding becomes pretty clunky.
> > Can you explain why this is a dt property rather than something
> > adjustable at runtime?
> > 
> > Otherwise, what you have here looks sane enough to me - but I'd like to
> > see some comments from Jonathan or David etc about your approach to the
> > excitation properties.  
> 
> This looks like something that should be in the devicetree to me. For example
> if the external reference supply is high impedance, buffering is pretty
> much required. And using precharge is an application design choice to
> reduce THD at the expense of other limitations.
> 

Agreed that this pretty much only makes sense in DT.

In the ideal world we would have firm rules on when to enable buffering
etc and then the DT would describe the impedance of the circuit connected
and any other relevant properties and then we'd have the driver enable it
only when those rigid rules dictated that we should.

Sadly no such simple rules exist (as far as I know) so we just expose the thing
that gets set dependent on someone's judgement of the suitability of
the buffering choice given the circuit being connected to the input.

If we pushed it to userspace we'd just end up with a per device blob
that dictated the mode to pick on boot and left it like that.  So effectively
another bit of firmware :(


J

<cropping other comments>