Message-ID: <aR4ezOOnBl6S6G2e@smile.fi.intel.com>
Date: Wed, 19 Nov 2025 21:47:24 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, robh@...nel.org, conor+dt@...nel.org,
	krzk+dt@...nel.org, linux-iio@...r.kernel.org, s32@....com,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	chester62515@...il.com, mbrugger@...e.com,
	ghennadi.procopciuc@....nxp.com, vkoul@...nel.org
Subject: Re: [PATCH v7 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On Wed, Nov 19, 2025 at 08:15:45PM +0100, Daniel Lezcano wrote:
> 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.
> 
> 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.
> 
> One potential scenario, not detected during testing, is that in some
> corner cases the DMA may already have been armed for the next
> transfer, which can lead dmaengine_tx_status() to return an incorrect
> residue.  The callback_result() operation—intended to supply the
> residue directly and eliminate the need to call
> dmaengine_tx_status()—also does not work.  Attempting to use
> dmaengine_pause() and dmaengine_resume() to prevent the residue from
> being updated does not work either.
> 
> This potential scenario should apply to any driver using cyclic DMA.
> However, no current driver actually handles this case, and they all rely
> on the same acquisition routine (e.g., the STM32 implementation).
> The NXP SAR acquisition routine has been used in production for several
> years, which is a good indication of its robustness.
> 
> As the IIO is implementing the cyclic DMA support API, it is not worth
> to do more spins to the current routine as it will go away when the
> new API will be available.
> 
> 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.

...contributors to the original code are...


This version is good enough, only a couple of nit-picks most important of which
is PM related macros usage.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...el.com>

...

> +static int nxp_sar_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct nxp_sar_adc_data *data = device_get_match_data(dev);
> +	struct nxp_sar_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	int irq, ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +	info->vref_mV = data->vref_mV;
> +	spin_lock_init(&info->lock);
> +	info->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
> +	if (IS_ERR(info->regs))
> +		return dev_err_probe(dev, PTR_ERR(info->regs),
> +				     "failed to get and remap resource");
> +
> +	info->regs_phys = mem->start;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;

> +	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> +			       dev_name(dev), indio_dev);

At least dev_name(dev) can be placed on the previous line.

> +	if (ret < 0)
> +		return ret;
> +
> +	info->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(info->clk))
> +		return dev_err_probe(dev, PTR_ERR(info->clk),
> +				     "failed to get the clock\n");
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	init_completion(&info->completion);
> +
> +	indio_dev->name = data->model;
> +	indio_dev->info = &nxp_sar_adc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	indio_dev->channels = nxp_sar_adc_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(nxp_sar_adc_iio_channels);
> +
> +	nxp_sar_adc_set_default_values(info);
> +
> +	ret = nxp_sar_adc_calibration(info);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Calibration failed\n");

Some of the messages started with capital letter, some with a small. Please,
make them consistent with whatever style you choose.

> +	ret = nxp_sar_adc_dma_probe(dev, info);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialize the dma\n");

DMA

> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      &nxp_sar_adc_trigger_handler,
> +					      &iio_triggered_buffer_setup_ops);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Couldn't initialise the buffer\n");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Couldn't register the device\n");
> +
> +	return 0;
> +}

...

> +static SIMPLE_DEV_PM_OPS(nxp_sar_adc_pm_ops, nxp_sar_adc_suspend, nxp_sar_adc_resume);

include/linux/pm.h:

/* Deprecated. Use DEFINE_SIMPLE_DEV_PM_OPS() instead. */

...

> +static struct platform_driver nxp_sar_adc_driver = {
> +	.probe = nxp_sar_adc_probe,
> +	.driver = {
> +		.name = "nxp-sar-adc",
> +		.of_match_table = nxp_sar_adc_match,
> +		.pm = pm_ptr(&nxp_sar_adc_pm_ops),

Shouldn't this be pm_sleep_ptr()?

> +	},
> +};

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists