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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3360 bytes --]

On Sat, Jun 21, 2025 at 05:28:08PM +0100, Jonathan Cameron wrote:
> 
> > >> +
> > >> +$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 :(

Can't remember if I replied to David's mail here or not, I pretty much
just asked the question because I didn't understand why the usecase and
wanted an explanation for my benefit. Probably makes no sense to explain
why a knob exists in the binding when anyone using the device will not
need one, but that does mean that at times with these devices that are
unfamiliar to me I have to have it explained to me why something is set
in stone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]