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

[Xingyu Wu]

On 2026/5/18 16:31, Andy Shevchenko wrote:
> 
> 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. 

Noted. I will update the driver in next patch.

> 
> ...
> 
> > +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.

Noted.

> 
> ...
> 
> 
> 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.

Will drop.

> 
> > +#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).

Will fix.

> 
> ...
> 
> > +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).

Noted.

> 
> > +	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)!

Explanations will be added.

> 
> > +}
> > +
> > +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.

Drop it.

> 
> > +	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.

Drop it.

> 
> > +module_platform_driver(starfive_saradc_driver);
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Best regards,
Xingyu Wu