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

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

On Thu, May 28, 2026 at 01:10:24AM -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.

(I'll try to not duplicate what Joshua noticed already.)

...

> +config AXIADO_SARADC
> +	tristate "Axiado SARADC driver"
> +	depends on ARCH_AXIADO || COMPILE_TEST

> +	depends on OF

No, in IIO we want a good justification on non-agnostic requirements.
Why can't this device driver be agnostic?

...


> +#include <linux/bitfield.h>

+ bits.h

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/io.h>

> +#include <linux/kernel.h>

No driver should have this header to be included.
Rare and well justified exceptions are possible
(and no, not in this case).

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

...

> +struct axiado_saradc {
> +	void __iomem *regs;
> +	struct clk *clk;
> +	unsigned long clk_rate;

> +	int vref_uv;

_uV (yes, capital letter as per SI).

> +	struct mutex lock; /* Serializes ADC conversions. */
> +};

...

> +static int axiado_saradc_conversion(struct axiado_saradc *info,
> +				    struct iio_chan_spec const *chan, int *val)
> +{
> +	unsigned long usecs;

Missing blank line here.

> +	/* Select the channel to be used and trigger conversion */

> +	iowrite32(AX_SARADC_MANUAL_CTRL_EN(chan->channel),
> +		  info->regs + AX_SARADC_MANUAL_CTRL);

Why not writel()?

> +
> +	/* Hardware requires 13 conversion cycles at clk_rate */
> +	usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * 1000000, info->clk_rate);

USe USEC_PER_SEC from time.h.

> +	usleep_range(usecs, usecs + 10);
> +
> +	*val = ioread32(info->regs + AX_SARADC_DOUT) &
> +	       GENMASK(AX_RESOLUTION_BITS - 1, 0);
> +
> +	/* Stop manual conversion */
> +	iowrite32(0, info->regs + AX_SARADC_MANUAL_CTRL);
> +	return 0;
> +}

...

> +static int axiado_saradc_probe(struct platform_device *pdev)
> +{
> +	struct axiado_saradc *info;
> +	const struct axiado_saradc_soc_data *soc_data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +	u32 reg;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	info->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(info->regs))
> +		return PTR_ERR(info->regs);
> +
> +	info->clk = devm_clk_get_enabled(&pdev->dev, NULL);

Why no name? It will make harder for the next generations of HW in case they
want more than one clock to be used.

> +	if (IS_ERR(info->clk))
> +		return PTR_ERR(info->clk);
> +
> +	info->clk_rate = clk_get_rate(info->clk);
> +	if (!info->clk_rate)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "invalid clock rate\n");

> +	info->vref_uv = devm_regulator_get_enable_read_voltage(&pdev->dev,
> +							       "vref");

Having

	struct device *dev = &pdev->dev;

will make the code shorter and easier to read.

> +	if (info->vref_uv < 0)
> +		return dev_err_probe(&pdev->dev, info->vref_uv,
> +				     "failed to get vref voltage\n");
> +
> +	soc_data = device_get_match_data(&pdev->dev);
> +	if (!soc_data)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "failed to get match data\n");
> +
> +	mutex_init(&info->lock);



> +	reg = FIELD_PREP(AX_SARADC_CH_EN_MASK,
> +			 GENMASK(soc_data->num_channels - 1, 0)) |
> +	      AX_SARADC_SAMPLE_16 | AX_SARADC_MODE | AX_SARADC_ENABLE;

FIELD_PREP_CONST() ?

> +	iowrite32(AX_SARADC_PD, info->regs + AX_SARADC_GLOBAL_CTRL);
> +	iowrite32(reg, info->regs + AX_SARADC_GLOBAL_CTRL);
> +
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->info = &axiado_saradc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = axiado_saradc_iio_channels;
> +	indio_dev->num_channels = soc_data->num_channels;
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to register IIO device\n");
> +
> +	return 0;
> +}

...

> +static const struct of_device_id axiado_saradc_match[] = {
> +	{
> +		.compatible = "axiado,ax3000-saradc",
> +		.data = &ax3000_saradc_data,
> +	},
> +	{
> +		.compatible = "axiado,ax3005-saradc",
> +		.data = &ax3005_saradc_data,
> +	},
> +	{},

No comma for the terminator entry.

> +};

...

> +static struct platform_driver axiado_saradc_driver = {
> +	.driver = {
> +		.name = KBUILD_MODNAME,

We want to have these kind of strings to be fixed.

> +		.of_match_table = axiado_saradc_match,
> +	},
> +	.probe = axiado_saradc_probe,
> +};

> +

Unnecessary blank line.

> +module_platform_driver(axiado_saradc_driver);

-- 
With Best Regards,
Andy Shevchenko