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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Andy,

On 10/18/25 22:12, Andy Shevchenko wrote:
> 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?

I'm sorry but I don't get your comment.

Do you mean why not use kfifo.h or use kfifo API and change all the code 
using the circ_buf by the 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?

I don't find such a macro, do you have a pointer ?

> ...
> 
>> +	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?

May be not needed, but harmless. I can remove them if you want

>> +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?

Sure
> (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?

TBH, I don't know the answer :/

How can I check that ?

>> +	dmaengine_tx_status(info->dma_chan, info->cookie, &state);
> 
> No return value check?

The return value is not necessary here because the caller of the 
callback will check with dma_submit_error() in case of error which 
covers the DMA_ERROR case and the other cases are not useful because the 
residue is taken into account right after.

It could be written differently with the DMA_COMPLETE check but the 
result will be the same. I prefer to keep the current implementation 
which has been tested.

>> +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.
Ok, I'll add it

-- 
<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