linux-kernel - Re: [PATCH v6 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: <ac85d16d-7d9d-41eb-9b1c-08df9a61f672@linaro.org>
Date: Wed, 19 Nov 2025 15:44:04 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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, vkoul@...nel.org
Subject: Re: [PATCH v6 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms


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@...aro.org> 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@linaro.org/

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