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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jul 16, 2026 at 10:53:02PM -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.

...

Please, follow IWYU principle.

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

You can omit this since you have platform_device.h.

> +#include <linux/err.h>
> +#include <linux/io.h>

+ math.h

> +#include <linux/mod_devicetable.h>

No to this header in a new code, Uwe did some rework recently.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

+ types.h // uXX

> +#include <linux/units.h>

...

> +/* Register offsets */
> +#define AX_SARADC_GLOBAL_CTRL_REG 0x0004
> +#define AX_SARADC_MANUAL_CTRL_REG 0x0008
> +#define AX_SARADC_DOUT_REG 0x001C

Be consistent, use tabs to indent values of the offsets.

...

> +/* GLOBAL_CTRL register values */
> +#define AX_SARADC_GLOBAL_CTRL_SAMPLE_16	\
> +	FIELD_PREP(AX_SARADC_GLOBAL_CTRL_SAMPLE_MASK, 0)
> +
> +#define AX_SARADC_GLOBAL_CTRL_MODE_MANUAL	\
> +	FIELD_PREP(AX_SARADC_GLOBAL_CTRL_MODE_MASK, 1)

FIELD_PREP_CONST() in both cases.

...

> +#define AX_SARADC_MANUAL_CTRL_EN(ch)	\
> +	(AX_SARADC_MANUAL_CTRL_ENABLE |	\
> +	FIELD_PREP(AX_SARADC_MANUAL_CTRL_CH_SEL_MASK, ch))

Missing parentheses for ch. Also wrong indentation of the second line, missing
one space.

...

> +#define AX_RESOLUTION_BITS 10
> +#define AX_SARADC_CONV_CYCLES 13
> +#define AX_SARADC_CONV_DELAY_MARGIN_US 10

Again, be consistent, use tabs to indent values.

...

> +struct axiado_saradc {
> +	struct regmap *regmap;
> +	struct clk *clk;

Makes no sense to keep it here, your code takes the rate and uses that,
I do not see how clk is being used right now. Perhaps you have plans
for power management? But then add it when it's needed and being used.

> +	struct mutex lock; /* Serializes ADC conversions. */
> +	unsigned long clk_rate;
> +	int vref_uV;
> +};
> +
> +static const struct regmap_config axiado_saradc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = AX_SARADC_DOUT_REG,

No cache?

> +};
> +
> +

Single blank line is enough.

...

> +static int axiado_saradc_conversion(struct axiado_saradc *info,
> +				    struct iio_chan_spec const *chan, int *val)
> +{
> +	unsigned long usecs;
> +	unsigned int regval;
> +	int stop_ret;
> +	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_EN(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +

Ditto.

> +	/* 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);
> +
> +	/* Stop manual conversion */
> +	stop_ret = regmap_write(info->regmap, AX_SARADC_MANUAL_CTRL_REG, 0);
> +
> +	if (ret)
> +		return ret;
> +	if (stop_ret)
> +		return stop_ret;

Why do we care about stop error? Isn't it the best effort we can do?

> +	*val = regval & GENMASK(AX_RESOLUTION_BITS - 1, 0);

Is device responding always in native endianess?

> +	return 0;
> +}

...

> +static int axiado_saradc_read_raw(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan, int *val,
> +				  int *val2, long mask)

Split logically:

static int axiado_saradc_read_raw(struct iio_dev *indio_dev,
				  struct iio_chan_spec const *chan,
				  int *val, int *val2, long mask)

...

> +static void axiado_saradc_disable(void *data)
> +{
> +	struct axiado_saradc *info = data;
> +
> +	regmap_write(info->regmap, AX_SARADC_GLOBAL_CTRL_REG,
> +		     AX_SARADC_GLOBAL_CTRL_PD);
> +}

Supply regmap instead of info and make this simpler

static void axiado_saradc_disable(void *map)
{
	regmap_write(map, AX_SARADC_GLOBAL_CTRL_REG, AX_SARADC_GLOBAL_CTRL_PD);
}

...

> +	regval = FIELD_PREP(AX_SARADC_GLOBAL_CTRL_CH_EN_MASK,
> +			    GENMASK(soc_data->num_channels - 1, 0)) |
> +		 AX_SARADC_GLOBAL_CTRL_SAMPLE_16 |
> +		 AX_SARADC_GLOBAL_CTRL_MODE_MANUAL |
> +		 AX_SARADC_GLOBAL_CTRL_ENABLE;

This is not used in the below call, move it closer to its user.

> +	ret = regmap_write(info->regmap, AX_SARADC_GLOBAL_CTRL_REG,
> +			   AX_SARADC_GLOBAL_CTRL_PD);

With

	struct regmap *map;

at the top, this and other will be shorter and easier to follow.
And I would dare to use a single line:

	ret = regmap_write(map, AX_SARADC_GLOBAL_CTRL_REG, AX_SARADC_GLOBAL_CTRL_PD);

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(info->regmap, AX_SARADC_GLOBAL_CTRL_REG, regval);
> +	if (ret)
> +		return ret;

> +	ret = devm_add_action_or_reset(dev, axiado_saradc_disable, info);

	ret = devm_add_action_or_reset(dev, axiado_saradc_disable, map);

> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko