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

Daniel Lezcano
2025-10-30T08:27:24Z

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 |