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

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

On Fri, Oct 17, 2025 at 06:42:38PM +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.

...

> +#include <linux/circ_buf.h>

Why not kfifo?

...

> +#define NXP_SAR_ADC_IIO_BUFF_SZ		(NXP_SAR_ADC_NR_CHANNELS + (sizeof(u64) / sizeof(u16)))

Hmm... Don't we have some macros so we can avoid this kind of hard coding?

...

> +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);

Do you need those 'U':s? clk_get_rate() already returns unsigned value of the
same or higher rank than int. No?

...

> +static int nxp_sar_adc_start_conversion(struct nxp_sar_adc *info, bool raw)
> +{
> +	u32 mcr;
> +
> +	mcr = readl(NXP_SAR_ADC_MCR(info->regs));
> +
> +	FIELD_MODIFY(NXP_SAR_ADC_MCR_NSTART, &mcr, 0x1);
> +	FIELD_MODIFY(NXP_SAR_ADC_MCR_MODE, &mcr, !raw);

!raw, which is boolean, as a parameter to FIELD_MODIFY() seems a bit odd to me,
perhaps simple

	raw ? 0 : 1

would work better?

(Note, optimizer of the complier will avoid any branching)

> +	writel(mcr, NXP_SAR_ADC_MCR(info->regs));
> +
> +	return 0;
> +}

...

> +	dma_samples = (u32 *)dma_buf->buf;

Is it aligned properly for this type of casting?

...

> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);

No return value check?

...

> +static const struct nxp_sar_adc_data s32g2_sar_adc_data = {
> +	.vref_mV = 1800,
> +	.model = "s32g2-sar-adc"

Keep a trailing comma as here it's not a termination member.

> +};

-- 
With Best Regards,
Andy Shevchenko