Re: [PATCH v5 2/2] iio: adc: add Axiado SARADC driver

[Joshua Crofts]

On Mon, 17 Aug 2026 03:45:41 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

> On Mon, 10 Aug 2026 21:27:26 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:
> 
> > On Mon, Aug 10, 2026 at 07:49:36AM -0700, Petar Stepanovic wrote:  
> > > Add support for the SARADC controller found on Axiado AX3000 and
> > > AX3005 SoCs.
> > > 
> > > The driver supports single-shot voltage reads through the IIO
> > > subsystem. The number of available input channels is selected from
> > > the SoC match data, allowing AX3000 and AX3005 variants to use the
> > > same driver.    
> > 
> > ...
> >   
> > > +/* MANUAL_CTRL register fields */    
> > 
> > ^^^
> >   
> > > +#define AX_SARADC_MANUAL_CTRL_ENABLE	BIT(0)
> > > +#define AX_SARADC_MANUAL_CTRL_CH_SEL_MASK	GENMASK(4, 1)
> > > +
> > > +#define AX_RESOLUTION_BITS	10
> > > +#define AX_SARADC_CONV_CYCLES	13
> > > +#define AX_SARADC_CONV_DELAY_MARGIN_US	10
> > > +
> > > +struct axiado_saradc {
> > > +	struct regmap *regmap;
> > > +	struct mutex lock; /* Serializes ADC conversions. */    
> > 
> > Choose the same style for all single-line comments (here is a period present
> > while in the above, for instance, there is none).  
> 
> Hmm. It is inconsistent in other places, but those two are arguably
> correct. The second is a sentence, the first is not (no verb)
> 
> Anyhow, I did a sweep for other sentences and not all of them have
> periods.  Given the rest of the driver looked fine to me and the other
> suggestions Andy made are easy to apply.
> 
> Applied with the following diff to the testing branch of iio.git
> Note I'm fine with rebasing that (and will do on rc1 once that's available)
> so extra tags or review comments are easy to add for a few weeks at least.
> 
> Mostly I did this just to reduce how many patch sets were undergoing
> revisions... I'm still over a 100 emails to read and run out of
> time for today.
> 
> Jonathan
> 
> 
> diff --git a/drivers/iio/adc/axiado_saradc.c b/drivers/iio/adc/axiado_saradc.c
> index daa98dd2f176..699ee31616fc 100644
> --- a/drivers/iio/adc/axiado_saradc.c
> +++ b/drivers/iio/adc/axiado_saradc.c
> @@ -34,10 +34,10 @@
>  #define AX_SARADC_GLOBAL_CTRL_PD               BIT(2)
>  #define AX_SARADC_GLOBAL_CTRL_ENABLE           BIT(0)
>  
> -/* GLOBAL_CTRL SAMPLE_MASK field value: 0 selects 16 samples */
> +/* GLOBAL_CTRL SAMPLE_MASK field value: 0 selects 16 samples. */
>  #define AX_SARADC_GLOBAL_CTRL_SAMPLE_16        0
>  
> -/* GLOBAL_CTRL MODE_MASK field value: 1 selects manual mode */
> +/* GLOBAL_CTRL MODE_MASK field value: 1 selects manual mode. */
>  #define AX_SARADC_GLOBAL_CTRL_MODE_MANUAL      1
>  
>  /* MANUAL_CTRL register fields */
> @@ -76,16 +76,15 @@ static int axiado_saradc_conversion(struct axiado_saradc *info,
>  
>         guard(mutex)(&info->lock);
>  
> -       /* Select the channel to be used and trigger conversion */
> +       /* Select the channel to be used and trigger conversion. */
>         ret = regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG,
>                            AX_SARADC_MANUAL_CTRL_ENABLE |
>                            FIELD_PREP(AX_SARADC_MANUAL_CTRL_CH_SEL_MASK, chan->channel));
>         if (ret)
>                 return ret;
>  
> -       /* Hardware requires 13 conversion cycles at clk_rate */
> -       usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * USEC_PER_SEC,
> -                            info->clk_rate);
> +       /* Hardware requires 13 conversion cycles at clk_rate. */
> +       usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * USEC_PER_SEC, info->clk_rate);
>         fsleep(usecs + AX_SARADC_CONV_DELAY_MARGIN_US);
>  
>         ret = regmap_read(info->regmap, AX_SARADC_DOUT_REG, &regval);
> @@ -220,7 +219,7 @@ static int axiado_saradc_probe(struct platform_device *pdev)
>  
>         soc_data = device_get_match_data(dev);
>         if (!soc_data)
> -               return dev_err_probe(dev, -EINVAL, "failed to get match data\n");
> +               return dev_err_probe(dev, -ENODATA, "failed to get match data\n");
> >   
> > > +	unsigned long clk_rate;
> > > +	int vref_uV;
> > > +};    
> > 
> > ...
> >   
> > > +static int axiado_saradc_conversion(struct axiado_saradc *info,
> > > +				    struct iio_chan_spec const *chan, int *val)
> > > +{
> > > +	unsigned long usecs;
> > > +	unsigned int regval;
> > > +	int ret;
> > > +
> > > +	guard(mutex)(&info->lock);
> > > +
> > > +	/* Select the channel to be used and trigger conversion */
> > > +	ret = regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG,
> > > +			   AX_SARADC_MANUAL_CTRL_ENABLE |
> > > +			   FIELD_PREP(AX_SARADC_MANUAL_CTRL_CH_SEL_MASK, chan->channel));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Hardware requires 13 conversion cycles at clk_rate */    
> >   
> > > +	usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * USEC_PER_SEC,
> > > +			     info->clk_rate);    
> > 
> > I think it's okay to have this on a single line (83 characters).
> >   
> > > +	fsleep(usecs + AX_SARADC_CONV_DELAY_MARGIN_US);
> > > +
> > > +	ret = regmap_read(info->regmap, AX_SARADC_DOUT_REG, &regval);
> > > +
> > > +	/* Best effort to stop manual conversion. */
> > > +	regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG, 0);
> > > +
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	*val = regval & GENMASK(AX_RESOLUTION_BITS - 1, 0);
> > > +
> > > +	return 0;
> > > +}    
> > 
> > ...
> >   
> > > +	soc_data = device_get_match_data(dev);
> > > +	if (!soc_data)
> > > +		return dev_err_probe(dev, -EINVAL, "failed to get match data\n");    
> > 
> > -ENODATA
> >   
> 
> 

With the diff Jonathan sent in mind,

Reviewed-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>

-- 
Kind regards,
Joshua Crofts