linux-kernel - Re: [PATCH v1 2/2] iio: adc: Add the NXP SAR ADC support for the s32g2/3 platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLlAugdr-hwMNIje@smile.fi.intel.com>
Date: Thu, 4 Sep 2025 10:33:14 +0300
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
Subject: Re: [PATCH v1 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

On Wed, Sep 03, 2025 at 05:28:09PM +0200, Daniel Lezcano wrote:
> On 03/09/2025 13:48, Andy Shevchenko wrote:
> > On Wed, Sep 03, 2025 at 12:27:56PM +0200, Daniel Lezcano wrote:

...

> > > +#include <linux/circ_buf.h>
> > 
> > Why not kfifo?
> 
> Are you suggesting to use kfifo instead of the circular buffer in the code ?

Asking, due to relatively recent move in the whole serial subsystem.

...

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

Because instead of counting 0:s and checking the suffix, I can immediately read
from the value like:

"This is 2 milliseconds in microseconds units."

It's more understandable and robust against possible typos.

...

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

Sure, define it like HW page size

/* This ADC relies on the page size to be 4kB */
NXP_SAR_PAGE_SIZE		0x1000 // or SZ_4K, or ...

...

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

Up to you. Only the question above (readl_poll_timeout() use) stays unanswered
so far.

...

> > > +	/*
> > > +	 * 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.

So, you guarantee that clk_get_rate() never returns, let's say 1000, right?
(Personally I don't see how it's possible to guarantee...)

...

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

But is the above a bug in the original proposal or not? I mean one wanted left
instead of right shift.

...

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

...which doesn't mean it's a good example to follow.

-- 
With Best Regards,
Andy Shevchenko