Re: [PATCH 1/2] dt-bindings: iio: adc: imx93: Add calibration properties

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2025-07-14 at 07:56 +0200, Primoz Fiser wrote:
> Hi all,
> 
> On 13. 07. 25 17:02, Jonathan Cameron wrote:
> > On Thu, 10 Jul 2025 10:46:44 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> > 
> > > On 7/10/25 2:39 AM, Primoz Fiser wrote:
> > > > From: Andrej Picej <andrej.picej@norik.com>
> > > > 
> > > > Document i.MX93 ADC calibration properties and how to set them.
> > > > 
> > > > Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> > > > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
> > > > ---
> > > >  .../bindings/iio/adc/nxp,imx93-adc.yaml       | 21 +++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-
> > > > adc.yaml b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> > > > index c2e5ff418920..d1c04cf85fe6 100644
> > > > --- a/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/nxp,imx93-adc.yaml
> > > > @@ -52,6 +52,27 @@ properties:
> > > >    "#io-channel-cells":
> > > >      const: 1
> > > >  
> > > > +  nxp,calib-avg-en:
> > > > +    default: 1
> > > > +    description:
> > > > +      Enable or disable calibration averaging function (AVGEN).
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [ 0, 1 ]
> > > > +
> > > > +  nxp,calib-nr-samples:
> > > > +    default: 512
> > > > +    description:
> > > > +      Selects number of samples (NRSMPL) to be used during calibration.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [ 16, 32, 128, 512 ]
> > 
> > Allow 1 as a value and drop the enabled above.   Averaging over 1 sample
> > is same as no averaging and gives simpler binding.
> > 
> > > > +
> > > > +  nxp,calib-t-sample:
> > > > +    default: 22
> > > > +    description:
> > > > +      Selects sample time (TSAMP) of calibration conversions in ADC
> > > > clock cycles
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    enum: [ 8, 16, 22, 32 ]
> > > > +
> > > >  required:
> > > >    - compatible
> > > >    - reg  
> > > 
> > > This seem like things that should be set at runtime rather than
> > > in the devicetree. Unless there is some justification on why
> > > these values depend on how the chip is wired up?
> 
> It depends how ADC 1.8V Vref is wired up, especially how noisy it is.
> 
> > 
> > Further to that, I'd like to see some explanation of why we care
> > to change it at all. Is it ever a bad idea to enable averaging and
> > pick a large number of samples for calibration?
> 
> This is a snippet from the i.MX93 TRM, chapter Analog-to-Digital
> Converter (SAR_ADC) describing calibration steps:
> 
> 1. Wait for deassertion of functional reset.
> 2. Configure SAR controller operating clock (MCR[ADCLKSE] = 0).
> 3. Bring ADC out of Power-down state (MCR[PWDN] = 0).
> 4. Configure desired calibration settings (default values kept for
> highest accuracy maximum time).
> • MCR[TSAMP]: Sample time for calibration conversion
> • MCR[NRSMPL]: Number of samples in averaging
> • MCR[AVGEN]: Averaging function enable in calibration
> 5. Run calibration by writing a one to MCR[CALSTART].
> 6. Check calibration run status in MSR[CALBUSY]—wait until MSR[CALBUSY]
> = 0; alternatively, MSR[ADCSTAT] can be
> used to check status.
> 7. Check calibration pass/fail status in MSR[CALFAIL] field. If
> MSR[CALFAIL] = 1 then calibration failed. Detailed status
> can be checked in CALSTAT.
> 
> 
> See point 4).
> 
> Not sure why would there be an option to configure i.MX93 ADC
> calibration parameters if one use-case (max accuracy max time) to rule
> them all?
> 

Sometimes HW guys just want to give you some options. Does not mean we have to
use them all :).

I guess what Jonathan is interested in, is to understand in what conditions the
defaults are no good for the calibration? If we can have a set of values that
should pretty much always work, no need to further complicate the bindings or
the driver.

- Nuno Sá 
> On the other hand, public TRM doesn't give much more information and
> >