Message-ID: <20250721093847.GD4844@nxa18884-linux.ap.freescale.net>
Date: Mon, 21 Jul 2025 17:38:47 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Nuno S? <noname.nuno@...il.com>
Cc: Primoz Fiser <primoz.fiser@...ik.com>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Haibo Chen <haibo.chen@....com>, Nuno Sa <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, linux-iio@...r.kernel.org,
	imx@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	upstream@...ts.phytec.de, andrej.picej@...ik.com
Subject: Re: [PATCH 1/2] dt-bindings: iio: adc: imx93: Add calibration
 properties

On Mon, Jul 14, 2025 at 05:11:31PM +0100, Nuno S? wrote:
>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@...libre.com> wrote:
>> > 
>> > > On 7/10/25 2:39 AM, Primoz Fiser wrote:
>> > > > From: Andrej Picej <andrej.picej@...ik.com>
>> > > > 
>> > > > Document i.MX93 ADC calibration properties and how to set them.
>> > > > 
>> > > > Signed-off-by: Andrej Picej <andrej.picej@...ik.com>
>> > > > Signed-off-by: Primoz Fiser <primoz.fiser@...ik.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.

Just my understanding, it is hard to use one fixed settings to fit all
kinds of conditions.

                 Noise induced from PCB tracks  Electro- magnetic noise
		     |                           |
		     V                           V
 ---------
|SOC(ADC)|   <---------------------------------<- (~) external Signal
 ---------
                 ^                 ^
                 |                 |
             I/O coupled noise    Internal noise


So OEM A's board may needs different settings compared with OEM B's board.

Regards,
Peng

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

Powered by blists - more mailing lists