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

[Jonathan Cameron]

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
>