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

[Daniel Lezcano]

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