Message-ID: <a34efc36-0100-4a7f-b131-566413ab88ae@linaro.org>
Date: Wed, 3 Sep 2025 17:28:09 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
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
Subject: Re: [PATCH v1 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms


Hi Andy,

thank you for the review


On 03/09/2025 13:48, Andy Shevchenko wrote:
> On Wed, Sep 03, 2025 at 12:27:56PM +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.
> 
> ...
> 
>> +#include <linux/circ_buf.h>
> 
> Why not kfifo?

Are you suggesting to use kfifo instead of the circular buffer in the code ?

>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
> 
> + match.h and more are missing...
> 
>> +#include <linux/module.h>
> 
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
> 
> Misuse of headers, and please make driver agnostic. There is none OF specifics
> in the code AFAICT.
> 
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
> 
> ...
> 
>> +/* Main Configuration Register */
>> +#define REG_ADC_MCR(__base)		(__base)
> 
> Useless macro. Perhaps (looking the others) this should be
> ((__base) + 0x00) which makes much more sense.

Sure

>> +#define REG_ADC_MCR_NRSMPL_32		BIT(11)
>> +#define REG_ADC_MCR_NRSMPL_128		BIT(12)
>> +#define REG_ADC_MCR_NRSMPL_512		(BIT(11) | BIT(12))
> 
> These are not bits, please use them in a form of 0, 1, 2, 3 and why not using
> bitfield.h?
>
> ...
> 
>> +#define NXP_SAR_ADC_CONV_TIMEOUT_MS	100
>> +#define NXP_SAR_ADC_CAL_TIMEOUT_US	100000
> 
> (100 * USEC_PER_MSEC)
> 
>> +#define NXP_SAR_ADC_WAIT_US		2000
> 
> (2 * USEC_PER_MSEC)

Why is this more understandable than the raw value ?

> 
>> +#define NXP_SAR_ADC_DMA_BUFF_SZ		(PAGE_SIZE * NXP_SAR_ADC_DMA_SAMPLE_SZ)
> 
> Oh, PAGE_SIZE is not good to use. I believe this HW is not tolerant to any page size.
> (Note, we made similar mistake in Intel IPU3 camera driver, which is now fixed)

Is it acceptable to put a hardcoded 4096 value ?

> ...
> 
>> +	/* Protect circular buffers access. */
>> +	spinlock_t lock;
> 
> + spinlock.h
> 
>> +	/*
>> +	 * Save and restore context
>> +	 */
>> +	u32 inpsamp;
>> +	u32 pwdn;
> 
> + types.h
> 
> ...
> 
>> +		ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3U));
> 
> + delay.h
> 
> Actually + math64.h (no need to include math.h which I mentioned elsewhere).
> 
> ...
> 
>> +static inline int nxp_sar_adc_calibration_wait(void __iomem *base)
>> +{
>> +	u32 msr, ret;
>> +
>> +	ret = read_poll_timeout(readl, msr, !(msr & REG_ADC_MSR_CALBUSY),
> 
> Why not readl_poll_timeout()?
> 
>> +				NXP_SAR_ADC_WAIT_US,
>> +				NXP_SAR_ADC_CAL_TIMEOUT_US,
>> +				true, REG_ADC_MSR(base));
>> +	if (ret)
>> +		return ret;
> 
>> +	if (!(msr & REG_ADC_MSR_CALFAIL))
>> +		return 0;
> 
> I would expect standard pattern — "errors first", but here either works.

Does it mean this chunk of code can be preserved or do you prefer an 
error block followed with a return 0 ?

>> +	/*
>> +	 * If the calibration fails, the status register bit must be
>> +	 * cleared
>> +	 */
>> +	msr &= ~REG_ADC_MSR_CALFAIL;
>> +
>> +	writel(msr, REG_ADC_MSR(base));
>> +
>> +	return -EAGAIN;
>> +}
> 
> ...
> 
>> +{
>> +	struct nxp_sar_adc *info = iio_priv(indio_dev);
>> +	int i, ret;
> 
> Why is 'i' signed?
> 
>> +
>> +	for (i = 0; i < info->channels_used; i++) {
>> +		ret = nxp_sar_adc_read_data(info, info->buffered_chan[i]);
>> +		if (ret < 0) {
>> +			nxp_sar_adc_read_notify(info);
>> +			return;
>> +		}
>> +
>> +		info->buffer[i] = ret;
>> +	}
>> +
>> +	nxp_sar_adc_read_notify(info);
>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>> +					   info->buffer,
>> +					   iio_get_time_ns(indio_dev));
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +}
> 
> ...
> 
>> +	/*
>> +	 * On disable, we have to wait for the transaction to finish.
>> +	 * ADC does not abort the transaction if a chain conversion
>> +	 * is in progress.
>> +	 * Wait for the worst case scenario - 80 ADC clk cycles.
>> +	 */
>> +	ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk)) * 80U);
> 
> Could it possible go wrong and with low rate clocks (kHz:ish) this will go into
> lo-o-o-o-ng *atomic* delay?

It is the ADC clock where we need to wait for 80 cycles. The lowest 
clock rate is 40MHz, but on this platform it is 80MHz IIRC. This routine 
is called only when the capture finishes. Except I'm missing something, 
this scenario should not happen.

>> +}
> 
> ...
> 
>> +		nxp_sar_adc_channels_enable(info, 1 >> chan->channel);
> 
> 1 >> ?!? Did you want BIT(channel)? Or simply channel != 0?

Yeah, BIT(chan->channel) is better

>> +static void nxp_sar_adc_dma_cb(void *data)
>> +{
>> +	struct nxp_sar_adc *info = iio_priv((struct iio_dev *)data);
>> +	struct iio_dev *indio_dev = data;
>> +	struct dma_tx_state state;
>> +	struct circ_buf *dma_buf;
>> +	struct device *dev_dma;
>> +	unsigned long flags;
>> +	u32 *dma_samples;
>> +	s64 timestamp;
>> +	int idx, ret;
>> +
>> +	dma_buf = &info->dma_buf;
>> +	dma_samples = (u32 *)dma_buf->buf;
>> +	dev_dma = info->dma_chan->device->dev;
> 
>> +	spin_lock_irqsave(&info->lock, flags);
> 
> Why not guard()() from cleanup.h?

sure

>> +	dmaengine_tx_status(info->dma_chan,
>> +			    info->cookie, &state);
> 
> Perfectly one line. No return check?

Ok, will see if the IIO DMA API has an impact on this portion of code 
before checking the return code. However, the status is often ignored in 
the other drivers.

[ ... ]

>> +/*
>> + * 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;
> 
> What is the purpose of having these constant to be temporary variables in the
> code?

The purpose is to group everything in a single function. As the default 
values are only used in this function, creating the macros where we have 
to roll up and down to check their values seemed to me pointless. But if 
you prefer macros, it is fine I can convert them.

>> +	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);
> 
> We have an agnostic alternative in property.h.
> 
>> +	info->vref = data->vref;
> 
> vref_uV / vref_mV in both cases?

Good suggestion, it makes sense

>> +	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");
> 
> No need, it prints message if fails.
> 
>> +	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;
> 
> Be consistent. It looks like driver is written by 2+ people who do not
> communicate with each other.

Indeed ... :)

>> +	}
>> +
>> +	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");
>> +		return ret;
>> +	}
> 
>> +	dev_info(dev, "Device initialized successfully.\n");
> 
> Noise. This should be dropped.

Ok, thanks again for the review

   -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists