Re: [PATCH v1 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 03 Sep 2025 12:20:39 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano wrote:
> > From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > 
> > The NXP S32G2 and S32G3 platforms integrate a successive approximation
> > register (SAR) ADC. Two instances are available, each providing 8
> > multiplexed input channels with 12-bit resolution. The conversion rate
> > is up to 1 Msps depending on the configuration and sampling window.
> > 
> > The SAR ADC supports raw, buffer, and trigger modes. It can operate
> > in both single-shot and continuous conversion modes, with optional
> > hardware triggering through the cross-trigger unit (CTU) or external
> > events. An internal prescaler allows adjusting the sampling clock,
> > while per-channel programmable sampling times provide fine-grained
> > trade-offs between accuracy and latency. Automatic calibration is
> > performed at probe time to minimize offset and gain errors.
> > 
> > The driver is derived from the BSP implementation and has been partly
> > rewritten to comply with upstream requirements. For this reason, all
> > contributors are listed as co-developers, while the author refers to
> > the initial BSP driver file creator.
> > 
> > All modes have been validated on the S32G274-RDB2 platform using an
> > externally generated square wave captured by the ADC. Tests covered
> > buffered streaming via IIO, trigger synchronization, and accuracy
> > verification against a precision laboratory signal source.
> > 
> > Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> > Co-developed-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > Signed-off-by: Ciprian Costea <ciprianmarian.costea@nxp.com>
> > Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
> > Co-developed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---  
> 
> Hi David,
> 
> Just some minor review for now...
A couple of follow ups (ignoring the DMA buf as others are better
than I am to comment on that!)

> > +/*
> > + * The documentation describes the reset values for the
> > + * registers. However some registers do not have these values after a
> > + * reset. It is a not desirable situation. In some other SoC family
> > + * documentation NXP recommend to not assume the default values are
> > + * set and to initialize the registers conforming to the documentation
> > + * reset information to prevent this situation. Assume the same rule
> > + * applies here as there is a discrepancy between what is read from
> > + * the registers at reset time and the documentation.
> > + */
> > +static void nxp_sar_adc_set_default_values(struct nxp_sar_adc *info)
> > +{
> > +	const u32 mcr_default	= 0x00003901;
> > +	const u32 msr_default	= 0x00000001;
> > +	const u32 ctr_default	= 0x00000014;
> > +	const u32 cimr_default	= 0x00000000;
> > +	const u32 ncmr_default	= 0x00000000;
> > +  
> 
> const does not really bring much here. I would rather have them as #defines.

Unless they can be broken down into meaningful fields I'd
not bother with defines. Just put the values in the writel()
as their meaning is clear from what is being registered.

> 
> > +	writel(mcr_default, REG_ADC_MCR(info->regs));
> > +	writel(msr_default, REG_ADC_MSR(info->regs));
> > +	writel(ctr_default, REG_ADC_CTR0(info->regs));
> > +	writel(ctr_default, REG_ADC_CTR1(info->regs));
> > +	writel(cimr_default, REG_ADC_CIMR0(info->regs));
> > +	writel(cimr_default, REG_ADC_CIMR1(info->regs));
> > +	writel(ncmr_default, REG_ADC_NCMR0(info->regs));
> > +	writel(ncmr_default, REG_ADC_NCMR1(info->regs));
> > +}

> > +};
> > +MODULE_DEVICE_TABLE(of, nxp_sar_adc_match);
> > +
> > +static struct platform_driver nxp_sar_adc_driver = {
> > +	.probe          = nxp_sar_adc_probe,
> > +	.remove         = nxp_sar_adc_remove,
> > +	.driver         = {
> > +		.name   = DRIVER_NAME,
> > +		.of_match_table = nxp_sar_adc_match,
> > +#ifdef CONFIG_PM_SLEEP  
> 
> You should not need the above. Look at pm_ptr() and friends.

Further to that, DEFINE_SIMPLE_DEV_PM_OPS() and drop the guards
around the functions.  The trick here is that it exposes
the functions to the compiler but lets it figure out they aren't
actually used and drop them.  All with out ifdef or __maybe_unused
etc.

> 
> > +		.pm     = &nxp_sar_adc_pm_ops,
> > +#endif
> > +	},
> > +};
> > +
> > +module_platform_driver(nxp_sar_adc_driver);
> > +
> > +MODULE_AUTHOR("NXP");
> > +MODULE_DESCRIPTION("NXP SAR-ADC driver");
> > +MODULE_LICENSE("GPL");  
>