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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 15 Oct 2025 09:17:40 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> 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@linaro.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 ?

I'm not against mixing FIELD_GET/PREP etc with single bit booleans where
it make sense.  However this was definitely a 'maybe' type of review
comment for exactly the reasons of inconsistency you've identified.

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

When it's effectively a boolean I'm not fussed if people use FIELD_GET()
or not. 

> 
> -       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);
If the comment is more necessary than before (I'm not sure it is but
then I'm more comfortable with these macros than many!) then the modification
probably doesn't make sense.
> 
>          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)),

Similar to above, For a simple boolean we don't need to extract
the value, a shifted bit is fine.  The compiler might sort that out. I've
never checked.

>                                   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);
>   }
> 
> 
>