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

[Joshua Crofts]

On Thu, 28 May 2026 at 10:11, Petar Stepanovic <pstepanovic@xxxxxxxxxx> 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.
>
> Signed-off-by: Petar Stepanovic <pstepanovic@xxxxxxxxxx>
> ---

Hi Petar, a few comments inline. Additionally, Sashiko came back
with a few issues:
https://sashiko.dev/#/patchset/20260528-axiado-ax3000-ax3005-saradc-v1-0-345dd5f6608a%40axiado.com.

> +++ b/drivers/iio/adc/axiado_saradc.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021-2026 Axiado Corporation
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>

IIO specific headers should follow the generic <linux/*> headers and
be grouped separately.

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

Don't use kernel.h when adding a new driver. The existing drivers in IIO
are currently being moved away from kernel.h, as it is a catch-all header.
You should include what you use manually (there is a tool for this, it's
called iwyu-tool).

> +#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>
> +
> +/* Register offsets */
> +#define AX_SARADC_GLOBAL_CTRL 0x0004
> +#define AX_SARADC_MANUAL_CTRL 0x0008
> +#define AX_SARADC_DOUT 0x001C
> +
> +/* GLOBAL_CTRL fields */
> +#define AX_SARADC_CH_EN_MASK GENMASK(31, 16)
> +#define AX_SARADC_SAMPLE_MASK GENMASK(6, 5)
> +#define AX_SARADC_MODE_MASK GENMASK(4, 3)
> +#define AX_SARADC_PD BIT(2)
> +#define AX_SARADC_ENABLE BIT(0)
> +
> +/* GLOBAL_CTRL values */
> +#define AX_SARADC_SAMPLE_16 FIELD_PREP(AX_SARADC_SAMPLE_MASK, 0)
> +#define AX_SARADC_MODE FIELD_PREP(AX_SARADC_MODE_MASK, 1)
> +
> +#define AX_SARADC_MANUAL_CTRL_EN(n) (BIT(0) | ((n) << 1))
> +#define AX_RESOLUTION_BITS 10
> +#define AX_SARADC_CONV_CYCLES 13
> +
> +struct axiado_saradc {
> +       void __iomem *regs;
> +       struct clk *clk;
> +       unsigned long clk_rate;
> +       int vref_uv;
> +       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;
> +       /* Select the channel to be used and trigger conversion */
> +       iowrite32(AX_SARADC_MANUAL_CTRL_EN(chan->channel),
> +                 info->regs + AX_SARADC_MANUAL_CTRL);
> +
> +       /* Hardware requires 13 conversion cycles at clk_rate */
> +       usecs = DIV_ROUND_UP(AX_SARADC_CONV_CYCLES * 1000000, info->clk_rate);
> +       usleep_range(usecs, usecs + 10);

fsleep() is preferred over usleep_range(), it selects the optimal
sleep function while guaranteeing a sleep of at least usecs time.

> +
> +       *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_read_raw(struct iio_dev *indio_dev,
> +                                 struct iio_chan_spec const *chan, int *val,

Small nit, but I'd move int * val onto the line along with val2 and mask
so it's logically separated.

> +                                 int *val2, long mask)
> +{
> +       struct axiado_saradc *info = iio_priv(indio_dev);
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               mutex_lock(&info->lock);

I wanted to recommend using the guard(mutex) macro, but you're
only using a mutex once, so up to you.

> +               ret = axiado_saradc_conversion(info, chan, val);
> +               mutex_unlock(&info->lock);
> +               return ret ? ret : IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SCALE:
> +               *val = info->vref_uv / 1000;
> +               *val2 = AX_RESOLUTION_BITS;
> +               return IIO_VAL_FRACTIONAL_LOG2;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static const struct iio_info axiado_saradc_iio_info = {
> +       .read_raw = axiado_saradc_read_raw,
> +};
> +
> +struct axiado_saradc_soc_data {
> +       unsigned int num_channels;
> +};
> +
> +static const struct axiado_saradc_soc_data ax3000_saradc_data = {
> +       .num_channels = 16,
> +};
> +
> +static const struct axiado_saradc_soc_data ax3005_saradc_data = {
> +       .num_channels = 8,
> +};
> +
> +#define AX_SARADC_CH(_index, _id)                                       \
> +       {                                                               \
> +               .type = IIO_VOLTAGE, .indexed = 1, .channel = (_index), \
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> +               .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> +               .datasheet_name = (_id), .scan_index = -1,              \
> +               .scan_type = {                                          \
> +                       .sign = 'u',                                    \
> +                       .realbits = AX_RESOLUTION_BITS,                 \
> +                       .storagebits = 16,                              \
> +                       .endianness = IIO_CPU,                          \
> +               },                                                      \
> +       }
> +
> +static const struct iio_chan_spec axiado_saradc_iio_channels[] = {
> +       AX_SARADC_CH(0, "adc0"),   AX_SARADC_CH(1, "adc1"),
> +       AX_SARADC_CH(2, "adc2"),   AX_SARADC_CH(3, "adc3"),
> +       AX_SARADC_CH(4, "adc4"),   AX_SARADC_CH(5, "adc5"),
> +       AX_SARADC_CH(6, "adc6"),   AX_SARADC_CH(7, "adc7"),
> +       AX_SARADC_CH(8, "adc8"),   AX_SARADC_CH(9, "adc9"),
> +       AX_SARADC_CH(10, "adc10"), AX_SARADC_CH(11, "adc11"),
> +       AX_SARADC_CH(12, "adc12"), AX_SARADC_CH(13, "adc13"),
> +       AX_SARADC_CH(14, "adc14"), AX_SARADC_CH(15, "adc15"),
> +};
> +
> +static int axiado_saradc_probe(struct platform_device *pdev)
> +{
> +       struct axiado_saradc *info;
> +       const struct axiado_saradc_soc_data *soc_data;

Use reverse christmas tree order.

> +       struct iio_dev *indio_dev;
> +       int ret;
> +       u32 reg;

Reg isn't a really good name, regval would be better.

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

Use devm_mutex_init since everything else is devm.

> +       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;
> +
> +       iowrite32(AX_SARADC_PD, info->regs + AX_SARADC_GLOBAL_CTRL);

Sashiko notes to add a small delay here to prevent a zero width pulse.

> +       iowrite32(reg, info->regs + AX_SARADC_GLOBAL_CTRL);
> +       indio_dev->name = dev_name(&pdev->dev);

Hmm, Sashiko may have a point here, this could break the ABI.
The name member is usually assigned a hardcoded string and
it shouldn't contain whitespace, dots or dashes, which dev_name()
could return.

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

You can just do `return devm_iio_device_register`.

> +       return 0;
> +}
> +

I don't see any kind of cleanup procedure, like a remove() function
or devm_add_action_or_reset callback, is this intentional?

> +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,
> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, axiado_saradc_match);
> +
> +static struct platform_driver axiado_saradc_driver = {
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +               .of_match_table = axiado_saradc_match,
> +       },
> +       .probe = axiado_saradc_probe,
> +};
> +
> +module_platform_driver(axiado_saradc_driver);
> +
> +MODULE_AUTHOR("AXIADO CORPORATION");
> +MODULE_DESCRIPTION("AXIADO SARADC driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.34.1
>
>

-- 
Kind regards

CJD