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

[Daniel Lezcano]

Hi Andy,

thanks for the review

On 11/19/25 10:27, Andy Shevchenko wrote:
> On Tue, Nov 18, 2025 at 10:34 PM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:

[ ... ]

> > As the IIO is implementing the cyclic DMA support API, it is not worth
> > to do more spins to the current routine as it will go away when the
> > new API will be available.
>
> ...
>
> > +#define NXP_SAR_ADC_EOC_CH(c)          BIT((c) % 32)
>
> Do you expect "c" to be bigger than 31? In which circumstances?

No, it should be always lesser than 32. We can drop the modulo.

[ ... ]

> > +       /*
> > +        * Ensure there are at least three cycles between the
> > +        * configuration of NCMR and the setting of NSTART.
> > +        */
> > +       if (enable)
> > +               ndelay(div64_u64(NSEC_PER_SEC, clk_get_rate(info->clk) * 3));
>
> I'm wondering how low the clock rate can be? With low enough clock
> rates this becomes a 100% CPU busyloop and in atomic context (is this
> the case?) without even the possibility to schedule.

I believe this question was already addressed in v1:

https://lore.kernel.org/all/a34efc36-0100-4a7f-b131-566413ab88ae@xxxxxxxxxx/

right ?

[ ... ]

> > +static int nxp_sar_adc_read_data(struct nxp_sar_adc *info, unsigned int chan)
> > +{
> > +       u32 ceocfr, cdr;
> > +
> > +       ceocfr = readl(NXP_SAR_ADC_CEOCFR0(info->regs));
>
> > +       /* FIELD_GET() can not be used here because EOC_CH is not constant */
> > +       if (!(NXP_SAR_ADC_EOC_CH(chan) & ceocfr))
> > +               return -EIO;
>
> [nxp_sar_adc_]field_get() may be defined and used. There is a series
> pending to bring field_get() to bitfield.h next release.

TBH I don't have an infinite bandwidth to write temporary helpers. So if 
it is ok, I would prefer to keep it as is

> ...
>
> > +static irqreturn_t nxp_sar_adc_isr(int irq, void *dev_id)
> > +{
> > +       struct iio_dev *indio_dev = (struct iio_dev *)dev_id;
>
> Unneeded explicit casting.

Right I will fix it.
[ ... ]


> > +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 ? 0 : 1
>
> is better to understand (it will be optimised by the compiler anyway,
> no branches will be added).

Ok, will do the change

> > +
> > +       writel(mcr, NXP_SAR_ADC_MCR(info->regs));
> > +
> > +       return 0;
> > +}
> > +
> > +static int nxp_sar_adc_read_channel(struct nxp_sar_adc *info, int channel)
> > +{
> > +       int ret;
> > +
> > +       info->current_channel = channel;
> > +       nxp_sar_adc_channels_enable(info, BIT(channel));
> > +       nxp_sar_adc_irq_cfg(info, true);
> > +       nxp_sar_adc_enable(info);
> > +
> > +       reinit_completion(&info->completion);
> > +       ret = nxp_sar_adc_start_conversion(info, true);
> > +       if (ret < 0)
> > +               goto out_disable;
>
> > +       ret = wait_for_completion_interruptible_timeout(&info->completion,
> > +                                                       NXP_SAR_ADC_CONV_TIMEOUT_JF);
> > +       if (ret == 0)
> > +               ret = -ETIMEDOUT;
> > +       if (ret > 0)
> > +               ret = 0;
>
> Since semantically it's not the same ret, I would write above as
>
>    if (!wait_for_completion...(...))
>      ret = -ETIMEDOUT;
>
> And note, no "else" branch is needed in this case.

Sure, I'll change that
> > +       nxp_sar_adc_channels_disable(info, *indio_dev->active_scan_mask);
>
> Wondering why this can't take a pointer to a mask.
nxp_sar_adc_channels_disable() is also called with BIT(x) parameter in 
other places. So in the function is much easier to do val |= mask;

> > +       ret = devm_request_irq(dev, irq, nxp_sar_adc_isr, 0,
> > +                              dev_name(dev), indio_dev);
> > +       if (ret < 0)
> > +               return dev_err_probe(dev, ret, "failed requesting irq, irq = %d\n", irq);
>
> No error code duplication in the message, please.

Given devm_request will print the "request_irq(%u) %ps %ps %s\n" error 
message. Would you suggest to just return ret here ?

> > +       spin_lock_init(&info->lock);
>
> Shouldn't this be _before_ IRQ registration? Theoretically the  IRQ
> may fire already just after the registration (yeah, it might be
> spurious, but handler and code should be ready for this).

Well it does not hurt moving it before anyway

[ ... ]

Thanks

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