linux-kernel - Re: [PATCH v4 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 for Android: free password hash cracker in your pocket

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

Message-ID: <0ac22118-fd0f-49c0-9aa8-5739925587d2@linaro.org>
Date: Wed, 15 Oct 2025 09:17:40 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: 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 v4 2/2] iio: adc: Add the NXP SAR ADC support for the
 s32g2/3 platforms

Hi Jonathan,

back to this driver after the merge window ...

On 9/20/25 11:27, Jonathan Cameron wrote:
> On Fri, 19 Sep 2025 15:56:18 +0200
> Daniel Lezcano <daniel.lezcano@...aro.org> wrote:

[ ... ]

>> +static int nxp_sar_adc_start_conversion(struct nxp_sar_adc *info, bool raw)
>> +{
>> +	u32 mcr;
>> +
>> +	mcr = readl(NXP_SAR_ADC_MCR(info->regs));
>> +	mcr |= NXP_SAR_ADC_MCR_NSTART;
>> +
>> +	if (raw)
>> +		mcr &= ~NXP_SAR_ADC_MCR_MODE;
>> +	else
>> +		mcr |= NXP_SAR_ADC_MCR_MODE;
> 
> Could use FIELD_MODIFY() for this though saving is minor.
> Same applies in various other places in this driver (and
> many others!)

[ ... ]

I gave a try to use the macro FIELD_MODIFY(). Logically, FIELD_GET() 
should be used too for consistency. From my POV, the result looks less 
readable than the usual annotation but may be I not used to the FIELD_ 
usage. Here is a snippet of the changes, do you really want to convert 
all the driver ?

         mcr = readl(NXP_SAR_ADC_MCR(info->regs));

         /* Return the current state. */
-       pwdn = mcr & NXP_SAR_ADC_MCR_PWDN;
+       pwdn = FIELD_GET(NXP_SAR_ADC_MCR_PWDN, mcr);

-       if (enable)
-               mcr &= ~NXP_SAR_ADC_MCR_PWDN;
-       else
-               mcr |= NXP_SAR_ADC_MCR_PWDN;
+       /* When the enabled flag is not set, we set the power down bit */
+       FIELD_MODIFY(NXP_SAR_ADC_MCR_PWDN, &mcr, !enable);

         writel(mcr, NXP_SAR_ADC_MCR(info->regs));

This looks ok but then:

  {
         u32 msr, ret;

-       ret = readl_poll_timeout(NXP_SAR_ADC_MSR(base), msr, !(msr & 
NXP_SAR_ADC_MSR_CALBUSY),
+       ret = readl_poll_timeout(NXP_SAR_ADC_MSR(base), msr,
+                                !FIELD_GET(NXP_SAR_ADC_MSR_CALBUSY, msr)),
                                  NXP_SAR_ADC_WAIT_US,
                                  NXP_SAR_ADC_CAL_TIMEOUT_US);
         if (ret)
                 return ret;

-       if (msr & NXP_SAR_ADC_MSR_CALFAIL) {
+       if (FIELD_GET(NXP_SAR_ADC_MSR_CALFAIL, msr)) {
                 /*
                  * If the calibration fails, the status register bit
                  * must be cleared.
                  */
-               msr &= ~NXP_SAR_ADC_MSR_CALFAIL;
+               FIELD_MODIFY(NXP_SAR_ADC_MSR_CALFAIL, &msr, 0x0);
                 writel(msr, NXP_SAR_ADC_MSR(base));

                 return -EAGAIN;

[ ... ]

         ceocfr = readl(NXP_SAR_ADC_CEOCFR0(info->regs));
-       if (!(ceocfr & NXP_SAR_ADC_EOC_CH(chan)))
+
+       /* FIELD_GET() can not be used here because EOC_CH is not 
constant */
+       if (!(NXP_SAR_ADC_EOC_CH(chan) & ceocfr))
                 return -EIO;

         cdr = readl(NXP_SAR_ADC_CDR(info->regs, chan));
-       if (!(cdr & NXP_SAR_ADC_CDR_VALID))
+       if (!(FIELD_GET(NXP_SAR_ADC_CDR_VALID, cdr)))
                 return -EIO;

-       return cdr & NXP_SAR_ADC_CDR_CDATA_MASK;
+       return FIELD_GET(NXP_SAR_ADC_CDR_CDATA_MASK, cdr);
  }



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