Re: [PATCH 2/2] iio: adc: imx93: Make calibration parameters configurable

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jul 10, 2025 at 12:23:58PM +0200, Primoz Fiser wrote:
> On 10. 07. 25 11:22, Andy Shevchenko wrote:
> > On Thu, Jul 10, 2025 at 09:39:04AM +0200, Primoz Fiser wrote:

...

> >> +	ret = device_property_read_u32(adc->dev, "nxp,calib-avg-en", &val);
> >> +	if (!ret) {
> >> +		if (val != 0 && val != 1) {
> >> +			dev_err(adc->dev, "invalid nxp,calib-avg-en: %d\n", val);
> >> +			return -EINVAL;
> >> +		}
> >> +		reg = val;
> >> +		mcr &= ~IMX93_ADC_MCR_AVGEN_MASK;
> >> +		mcr |= FIELD_PREP(IMX93_ADC_MCR_AVGEN_MASK, reg);
> >> +	}
> > 
> > Please, since it's optional, do other way around.
> > 
> > 	val = $DEFAUTL;
> > 	device_property_read_u32(adc->dev, "nxp,calib-avg-en", &val);
> > 	FIELD_MODIFY(...)
> > 
> > Similar approach may be used for the other properties.
> 
> OK, I guess I could implement it like you suggested to explicitly set
> the default parameter values.
> 
> But in current implementation MCR values are read at the beginning of
> imx93_adc_calibration(), meaning calibration parameters are register POR
> defaults. With you suggestion, we put defaults in software rather than
> reading them from the hw directly.

I see, then you need to read, do FIELD_GET()/device_property_read()/FIELD_MODIFY().
You got the idea.

...

> > Please, factor out this to the function, so we won't see the direct IO in the
> > ->probe().
> 
> Sorry I don't understand this part.
> 
> What do you mean by factoring out this writel()?
> 
> Do you perhaps suggest to implement function
> imx93_adc_configure_calibration() and put all our changes into it?
> 
> But we are already in imx93_adc_calibration() which is separate from
> probe().

Ah, sorry for the mistakenly read the function name. Ignore this comment.

-- 
With Best Regards,
Andy Shevchenko