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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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