On Mon, 25 Mar 2024 09:55:23 +0100 Primoz Fiser <primoz.fiser@xxxxxxxxx> wrote: > Hi Jonathan, > > On 25. 03. 24 09:32, Andrej Picej wrote: > > Hi Jonathan, > > > > On 24. 03. 24 14:55, Jonathan Cameron wrote: > >> On Wed, 20 Mar 2024 11:04:04 +0100 > >> Andrej Picej <andrej.picej@xxxxxxxxx> wrote: > >> > >>> Hi all, > >>> > >>> we had some problems with failing ADC calibration on the i.MX93 boards. > >>> Changing default calibration settings fixed this. The board where this > >>> patches are useful is not yet upstream but will be soon (hopefully). > >> > >> Tell us more. My initial instinct is that this shouldn't be board > >> specific. > >> What's the trade off we are making here? Time vs precision of > >> calibration or > >> something else? If these are set to a level by default that doesn't work > >> for our board, maybe we should just change them for all devices? > >> > > The imx93_adc driver is quite new. > > If you look at line #162, you will find a comment by the original author: > > > /* > > * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR, > > * can add the setting of these bit if need in future. > > */ > > URL: > https://github.com/torvalds/linux/blob/master/drivers/iio/adc/imx93_adc.c#L162 > > So, for most use-cases the default setting should work, but why not make > them configurable? > > So this patch-series just implement what was missing from the beginning > / was planned for later. Hi Primoz, I doubt anyone reviewed the comment closely enough to say if what it was suggesting was sensible or not, so the fact it was listed as a todo doesn't directly impact this discussion. > > BR, > Primoz > > > > > > So we have two different boards with the same SoC. On one, the > > calibration works with the default values, on the second one the > > calibration fails, which makes the ADC unusable. What the ADC lines > > measure differ between the boards though. But the implementation is > > nothing out of the ordinary. > > > > We tried different things but the only thing that helped is to use > > different calibration properties. We tried deferring the probe and > > calibration until later boot and after boot, but it did not help. > > > > In the Reference Manual [1] (chapter 72.5.1) it is written: > > > >> 4. Configure desired calibration settings (default values kept for > >> highest accuracy maximum time). > > > > So your assumption is correct, longer calibration time (more averaging > > samples) -> higher precision. The default values go for a high accuracy. > > And since we use a NRSMPL (Number of Averaging Samples) of 32 instead of > > default 512, we reduce the accuracy so the calibration values pass the > > internal defined limits. Ouch. Let me try to dig into this. Is this effectively relaxing the constraints? I guess because a value that is perhaps always biased one way is considered within bounds if those acceptable bounds are wider because of lower precision? I was assuming it was the other way around and the device had fixed constraint limits and you needed to take more samples due to higher noise. Seems the opposite is true here and that worries me. I'll definitely need input from NXP on this as a workaround and their strong support to consider it. > > > > I'm not sure that changing default values is the right solution here. We > > saw default values work with one of the boards. And since the NXP kept > > these values adjustable I think there is a reason behind it. I'd assume trade off between time and calibration precision, not the sort of use I think you are describing. > > > > Note: When I say one of the boards I mean one board form. So same board > > version, but different HW. Superficially I'm struggling to not see this as broken hardware that it is out of expected tolerances in some fashion. Maybe I misunderstood the issue. Jonathan > > > > Best regards, > > Andrej > > > > [1] i.MX 93 Applications Processor Reference Manual, Rev. 4, 12/2023 > > _______________________________________________ > > upstream mailing list > > upstream@xxxxxxxxxxxxxxx > > http://lists.phytec.de/cgi-bin/mailman/listinfo/upstream > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
