Message-ID: <eedbfbfd1ba625b6750eb3ae20a69301b8bc3ef9.camel@gmail.com>
Date: Wed, 03 Sep 2025 12:20:39 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>, jic23@...nel.org, 
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, 	conor+dt@...nel.org, krzk+dt@...nel.org
Cc: 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
Subject: Re: [PATCH v1 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On Wed, 2025-09-03 at 12:27 +0200, Daniel Lezcano wrote:
> From: Stefan-Gabriel Mirea <stefan-gabriel.mirea@....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@....com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@....com>
> Co-developed-by: Ciprian Costea <ciprianmarian.costea@....com>
> Signed-off-by: Ciprian Costea <ciprianmarian.costea@....com>
> Co-developed-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@....nxp.com>
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@....nxp.com>
> Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@....com>
> Co-developed-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> ---

Hi David,

Just some minor review for now...

>  drivers/iio/adc/Kconfig       |   13 +
>  drivers/iio/adc/Makefile      |    1 +
>  drivers/iio/adc/nxp-sar-adc.c | 1046 +++++++++++++++++++++++++++++++++
>  3 files changed, 1060 insertions(+)
>  create mode 100644 drivers/iio/adc/nxp-sar-adc.c
> 
> 

...

> +
> +static int nxp_sar_adc_dma_probe(struct device *dev, struct nxp_sar_adc
> *info)
> +{
> +	struct device *dev_dma;
> +	int ret;
> +	u8 *rx_buf;
> +
> +	info->dma_chan = devm_dma_request_chan(dev, "rx");
> +	if (IS_ERR(info->dma_chan))
> +		return PTR_ERR(info->dma_chan);
> +
> +	dev_dma = info->dma_chan->device->dev;
> +	rx_buf = dma_alloc_coherent(dev_dma, NXP_SAR_ADC_DMA_BUFF_SZ,
> +				    &info->rx_dma_buf, GFP_KERNEL);
> +	if (!rx_buf)
> +		return -ENOMEM;
> +

The above needs some discussion at the very least. Have you considered the IIO
DMA buffer interface? It should be extendable to accommodate any particularity
of your usecase (or we should at least discuss it).

With it, you also gain a userspace interface where you can actually share DMA
buffers in a zero copy fashion. You can also share these buffers with USB
gadgets. For instance, with libiio, you would be able to fetch samples from your
host machine (through USB) in a very fast way (zero copy between IIO and USB).

Setting up DMA to then "having" to push it to a SW buffer and needing a syscall
to retrieve the data seems counter-productive. 

> +	info->dma_buf.buf = rx_buf;
> +
> +	info->dma_config.direction = DMA_DEV_TO_MEM;
> +	info->dma_config.src_addr_width = NXP_SAR_ADC_DMA_SAMPLE_SZ;
> +	info->dma_config.src_maxburst = 1;
> +
> +	ret = devm_add_action_or_reset(dev, nxp_sar_adc_dma_remove, info);
> +	if (ret) {
> +		nxp_sar_adc_dma_remove(info);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * 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.

> +	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));
> +}
> +
> +static int nxp_sar_adc_probe(struct platform_device *pdev)
> +{
> +	const struct nxp_sar_adc_data *data;
> +	struct nxp_sar_adc *info;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	struct device *dev = &pdev->dev;
> +	int irq;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct nxp_sar_adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	data = of_device_get_match_data(dev);
> +

device_get_match_data()
> +	info->vref = data->vref;
> +
> +	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");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "no irq resource\n");
> +
> +	ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> +			       dev_name(dev), indio_dev);
> +	if (ret < 0)
> +		dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n",
> irq);
> +
> +	info->regs_phys = mem->start;
> +	spin_lock_init(&info->lock);
> +
> +	info->clk = devm_clk_get_enabled(dev, "adc");
> +	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 = dev_name(dev);
> +	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(dev, "Calibration failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = nxp_sar_adc_dma_probe(dev, info);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize the dma\n");
> +		return ret;
> +	}
> +
> +	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) {
> +		dev_err(dev, "Couldn't initialise the buffer\n");
> +		return ret;
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register the device.\n");

dev_err_probe(). Ditto for all other places.

> +		return ret;
> +	}
> +
> +	dev_info(dev, "Device initialized successfully.\n");
> 
dev_dbg() if you want to keep it but kind of useless IMHO.

> +	return 0;
> +}
> +
> +static void nxp_sar_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +
> +	nxp_sar_adc_stop_conversion(info);
> +	nxp_sar_adc_channels_disable(info, REG_ADC_CH_MASK);
> +	nxp_sar_adc_dma_channels_disable(info, REG_ADC_CH_MASK);
> +	nxp_sar_adc_dma_cfg(info, false);
> +	nxp_sar_adc_disable(info);
> +	dmaengine_terminate_sync(info->dma_chan);
> +	iio_device_unregister(indio_dev);

You're using devm_iio_device_register().

> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int nxp_sar_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +
> +	info->pwdn = nxp_sar_adc_disable(info);
> +	info->inpsamp = nxp_sar_adc_conversion_timing_get(info);
> +
> +	clk_disable_unprepare(info->clk);
> +
> +	return 0;
> +}
> +
> +static int nxp_sar_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret)
> +		return ret;
> +
> +	nxp_sar_adc_conversion_timing_set(info, info->inpsamp);
> +
> +	if (!info->pwdn)
> +		nxp_sar_adc_enable(info);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(nxp_sar_adc_pm_ops, nxp_sar_adc_suspend,
> nxp_sar_adc_resume);
> +
> +static const struct nxp_sar_adc_data s32g2_sar_adc_data = { .vref = 1800 };
> +
> +static const struct of_device_id nxp_sar_adc_match[] = {
> +	{ .compatible = "nxp,s32g2-sar-adc", .data = &s32g2_sar_adc_data },
> +	{ /* sentinel */ }

No need for the comment. Kind of obvious :)

> +};
> +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.

> +		.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");

Powered by blists - more mailing lists