Re: [PATCH v1 2/2] iio: adc: Add StarFive SAR-ADC driver

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

On Mon, May 18, 2026 at 04:18:52PM +0800, Xingyu Wu wrote:
> Add a new IIO ADC driver for the StarFive JHB100 SAR ADC controller.
> 
> The hardware provides 12-bit conversion precision and up to 8 input
> channels. This driver supports single-shot channel reads and exposes
> standard IIO interfaces for raw ADC values, processed voltages, and
> scale reporting. The driver also supports channel monitor mode with
> out-of-bound detection and scan frequency configuration.

Seems like this is just dust off code from ca 2020, as it uses sometimes
quite old APIs (no driver uses in the past few years in IIO!).
This needs much more work, please take your time to go via existing IIO
drivers that were submitted and accepted in 2025/2026 and take them as
examples (may be not the best, but good enough) and update your code
accordingly. Note, reviewing others' patches may help a lot to get
your knowledge up-to-date.

...

> +config STARFIVE_SARADC
> +	tristate "StarFive SARADC driver"
> +	depends on ARCH_STARFIVE || COMPILE_TEST
> +	depends on COMMON_CLK && RESET_CONTROLLER
> +	help
> +	  Say yes here to build support for the SARADC found in SoCs from
> +	  StarFive.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called starfive_saradc.

Indentation issues.

...


Follow IWYU.

> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>

> +#include <linux/of.h>

No OF in a new code for the drivers like this one.

> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>

...

> +#define SARADC_VDD_MV_TO_RAW(x)		({ \
> +						((x) == 0) ? 0U : ({ \
> +							u32 _raw = \
> +							(u32)(((u64)(x) * 64000000ULL + \
> +							4514610639ULL) / 30318750ULL); \
> +							_raw > 4095 ? 4095 : _raw; \
> +						}); \
> +					})

This is so ugly! Use your common sense and see tons of the examples around (the
more recent driver is the average better the code is).

...

> +struct starfive_saradc {

Use `pahole` to check the layout, also shuffle members to see what
`bloat-o-meter` says about the resulting binary (for example, moving 'dev' up
might give less code).

> +	void __iomem		*base;
> +	struct device		*dev;
> +	struct clk		*clk;
> +	struct reset_control	*rst;
> +	/* lock to protect against multiple access to the device */
> +	struct mutex		lock;
> +	int			using_ch;
> +	/* flag of interrupts by error or data done */
> +	bool			err;
> +	bool			mon_working;
> +	u8			mon_ch;
> +	bool			mon_en;
> +	u32			up_bounds[SARADC_MAX_CHANNELS];
> +	u32			low_bounds[SARADC_MAX_CHANNELS];
> +};

...

> +static inline void starfive_saradc_irq_clr_all(struct starfive_saradc *priv)
> +{
> +	unsigned int reg = readl(priv->base + SARADC_IRQ_EN_ST);
> +	int i;
> +
> +	starfive_saradc_irq_clr(priv, reg);

> +	for (i = 0; i < SARADC_MAX_CHANNELS; i++)

	for (unsigned int i = 0; i < SARADC_MAX_CHANNELS; i++)


> +		starfive_saradc_data_clr_ch(priv, i);
> +}

...

> +static void starfive_saradc_ch_dis_save(struct starfive_saradc *priv)
> +{
> +	unsigned int reg;
> +
> +	if (priv->mon_en) {
> +		writel(ADC_IRQ_ST_MSK, priv->base + SARADC_IRQ_EN_ST);
> +
> +		reg = readl(priv->base + SARADC_CTRL) & ~ADC_CHAN_EN_MSK;
> +		writel(reg, priv->base + SARADC_CTRL);
> +		priv->mon_working = false;
> +	}
> +
> +	starfive_saradc_irq_clr_all(priv);
> +	msleep(20);

So long sleeps must be explained (in the code comments with the reference to
datasheet or citing it in case of non-public document)!

> +}
> +
> +static void starfive_saradc_ch_start(struct starfive_saradc *priv)
> +{
> +	int ch = priv->using_ch;
> +	unsigned int reg = readl(priv->base + SARADC_CTRL);
> +
> +	/* Enable channel */
> +	reg = (reg & ~ADC_CHAN_EN_MSK) | SARADC_CHAN_EN(ch);
> +	writel(reg, priv->base + SARADC_CTRL);
> +
> +	msleep(20);

Ditto!

> +	/* Enable conversion */
> +	writel(reg | ADC_EN_MSK, priv->base + SARADC_CTRL);
> +}

...

> +static ssize_t starfive_saradc_monitor_channel_show(struct device *dev,
> +						    struct device_attribute *attr,
> +						    char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct starfive_saradc *priv = iio_priv(indio_dev);

> +	return sprintf(buf, "%d\n", priv->mon_ch);

Huh?! Is this driver taken from the ancient times and got just dust off?

> +}

...

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq,
> +				     "failed to get irq\n");

No, we don't do dup messages.

> +	ret = devm_request_threaded_irq(&pdev->dev, irq,
> +					starfive_saradc_irq_handler,
> +					starfive_saradc_mon_stop_threadfn,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					dev_name(&pdev->dev), priv);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to request irq handler\n");

Ditto.

...

> +

Unneeded blank line.

> +module_platform_driver(starfive_saradc_driver);

-- 
With Best Regards,
Andy Shevchenko