[PATCH v1 0/2] Add StarFive SAR-ADC driver

[PATCH v1 0/2] Add StarFive SAR-ADC driver

[Xingyu Wu]

Hi all,

This series adds an IIO ADC driver for the controller of StarFive
Successive Approximation Register A/D converter (SAR-ADC).

The StarFive SAR-ADC is a 12-bit converter with up to 8 input channels
and a fixed 1.8V reference domain. The driver provides raw and processed
voltage readouts via IIO, runtime PM support, and threshold-based
voltage monitoring.

Tested on StarFive JHB100 EVB with all ADC channels and monitor
interrupt path.

Xingyu Wu (2):
  bindings: iio: adc: Add StarFive JHB100 SARADC
  iio: adc: Add StarFive SAR-ADC driver

 .../iio/adc/starfive,jhb100-saradc.yaml       |  62 ++
 MAINTAINERS                                   |   6 +
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/starfive-saradc.c             | 978 ++++++++++++++++++
 5 files changed, 1058 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
 create mode 100644 drivers/iio/adc/starfive-saradc.c

-- 
2.34.1

[PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
new file mode 100644
index 000000000000..ba8e19b72ad7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Successive Approximation Register (SAR) A/D converter for the StarFive JHB100 SoC
+
+maintainers:
+  - Xingyu Wu <xingyu.wu@starfivetech.com>
+
+properties:
+  compatible:
+    const: starfive,jhb100-saradc
+
+  reg:
+    maxItem: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 2
+
+  "#io-channel-cells":
+    const: 1
+
+  upper-bound-mv:
+    description: The upper bound voltage value of the monitor.
+    $ref: /schemas/types.yaml#/definitions/uint16
+
+  lower-bound-mv:
+    description: The lower bound voltage value of the monitor.
+    $ref: /schemas/types.yaml#/definitions/uint16
+
+  scan-freq:
+    description: Number of the scan cycle interval.
+    $ref: /schemas/types.yaml#/definitions/uint16
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - resets
+  - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    adc@11be1400 {
+      compatible = "starfive,jhb100-saradc";
+      reg = <0x11be1400 0x400>;
+      interrupts = <172>;
+      clocks = <&per0crg 18>;
+      resets = <&per0crg 11>, <&per0crg 46>;
+      #io-channel-cells = <1>;
+      };
-- 
2.34.1

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

[Xingyu Wu]

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.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 MAINTAINERS                       |   6 +
 drivers/iio/adc/Kconfig           |  11 +
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/starfive-saradc.c | 978 ++++++++++++++++++++++++++++++
 4 files changed, 996 insertions(+)
 create mode 100644 drivers/iio/adc/starfive-saradc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c3fe46d7c4bc..497b3ae64cb9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25275,6 +25275,12 @@ F:	Documentation/devicetree/bindings/power/starfive*
 F:	drivers/pmdomain/starfive/
 F:	include/dt-bindings/power/starfive,jh7110-pmu.h
 
+STARFIVE JHB100 SARADC DRIVER
+M:	Xingyu Wu <xingyu.wu@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
+F:	drivers/iio/adc/starfive-saradc.c
+
 STARFIVE SOC DRIVERS
 M:	Conor Dooley <conor@kernel.org>
 S:	Maintained
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 60038ae8dfc4..5f85d521f659 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1514,6 +1514,17 @@ config SD_ADC_MODULATOR
 	  This driver can also be built as a module.  If so, the module
 	  will be called sd_adc_modulator.
 
+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.
+
 config STM32_ADC_CORE
 	tristate "STMicroelectronics STM32 adc core"
 	depends on ARCH_STM32 || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index c76550415ff1..d4a3a4af16c6 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -133,6 +133,7 @@ obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
 obj-$(CONFIG_SOPHGO_CV1800B_ADC) += sophgo-cv1800b-adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
+obj-$(CONFIG_STARFIVE_SARADC) += starfive-saradc.o
 obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
 obj-$(CONFIG_STM32_ADC) += stm32-adc.o
 obj-$(CONFIG_STM32_DFSDM_ADC) += stm32-dfsdm-adc.o
diff --git a/drivers/iio/adc/starfive-saradc.c b/drivers/iio/adc/starfive-saradc.c
new file mode 100644
index 000000000000..78409cb1bcb2
--- /dev/null
+++ b/drivers/iio/adc/starfive-saradc.c
@@ -0,0 +1,978 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * StarFive Successive Approximation Register (SAR) A/D Converter
+ *
+ * Copyright (C) 2026 StarFive Technology Co., Ltd.
+ */
+
+#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>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+#define SARADC_CTRL				0x00
+#define SARADC_IRQ_EN_ST			0x04
+#define SARADC_SAMPLE_DAT0			0x10
+#define SARADC_ULB_CH0				0x40
+#define SARADC_SCAN_FREQ			0x70
+
+/* macro SARADC_CTRL reg*/
+#define ADC_EN_MSK				BIT(0)
+#define ADC_PWRDOWN_MSK				BIT(1)
+#define ADC_IDLE_MSK				BIT(8)
+#define ADC_CHAN_EN_MSK				GENMASK(23, 16)
+#define ADC_CHAN_EN_SFT				16
+#define SARADC_CHAN_EN(x)			(BIT(x) << ADC_CHAN_EN_SFT)
+
+/* macro SARADC_IRQ_EN_ST reg */
+#define ADC_IRQ_ST_MSK				GENMASK(7, 0)
+#define ADC_IRQ_EN_MSK				GENMASK(23, 16)
+#define ADC_IRQ_EN_SFT				16
+#define SARADC_IRQ_CH_EN(x)			(BIT(x) << ADC_IRQ_EN_SFT)
+
+/* macro SARADC_SAMPLE_DATx reg */
+#define ADC_DAT_MSK				GENMASK(11, 0)
+#define ADC_DAT_RDY_MSK				BIT(31)
+
+/* macro SARADC_ULB_CHX reg */
+#define ADC_LOWER_BOUND_MSK			GENMASK(11, 0)
+#define ADC_LOWER_BOUND_SFT			0
+#define ADC_UPPER_BOUND_MSK			GENMASK(31, 20)
+#define ADC_UPPER_BOUND_SFT			20
+#define ADC_LOWER_BOUND_DEF			0x0
+#define ADC_UPPER_BOUND_DEF			0xFFF
+
+/* macro SARADC_SCAN_FREQ reg */
+#define ADC_SCAN_FREQ_MSK			GENMASK(19, 0)
+#define ADC_SCAN_FREQ_SFT			0
+#define ADC_SCAN_FREQ_DEF			20
+#define ADC_SCAN_FREQ_MIN			15
+
+#define SARADC_TIMEOUT				(100 * USEC_PER_MSEC)
+#define SARADC_MAX_CHANNELS			8
+#define SARADC_REALBITS				12
+#define SARADC_DATAX_REG_GET(x)			((x) * sizeof(u32) + SARADC_SAMPLE_DAT0)
+#define SARADC_ULB_CHX_REG_GET(x)		((x) * sizeof(u32) + SARADC_ULB_CH0)
+/* AVDD = 1.8v = 1.8 * 1000000 uv */
+#define SARADC_AVDD_VOL				(18 * 100000)
+/* same with: (x * 1800000) >> 12 */
+#define SARADC_VDD_XFER(x)			(((x) * 28125) >> 6)
+
+#define SARADC_ADJUST_B				65217 /* 0.065217 * 1000000 */
+#define SARADC_ADJUST_K				1078 /* 1.078 * 1000 */
+/* For VDD adjustment: ((x - b) * k) v*/
+#define SARADC_VDD_ADJUST(x)			(((x) - SARADC_ADJUST_B) * \
+						 SARADC_ADJUST_K / 1000)
+#define SARADC_RAW_TO_VDD_ADJ(x)	({ \
+						u32 _x = (x); \
+						(_x < 149) ? 0U : ({ \
+							u64 _v = (uint64_t)_x * \
+							28125 - 4173888; \
+							(u32)((_v * 1078 + 32000) / 64000); \
+						}); \
+					})
+
+#define SARADC_VDD_MV_TO_RAW(x)		({ \
+						((x) == 0) ? 0U : ({ \
+							u32 _raw = \
+							(u32)(((u64)(x) * 64000000ULL + \
+							4514610639ULL) / 30318750ULL); \
+							_raw > 4095 ? 4095 : _raw; \
+						}); \
+					})
+
+#define SARADC_CHAN(_index) {					\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = _index,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			BIT(IIO_CHAN_INFO_PROCESSED),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
+	.datasheet_name = "SARADC_"#_index,			\
+	.scan_index = _index,					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = SARADC_REALBITS,			\
+		.endianness = IIO_CPU,				\
+	},							\
+}
+
+struct starfive_saradc {
+	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 const struct iio_chan_spec starfive_saradc_iio_channels[] = {
+	SARADC_CHAN(0),
+	SARADC_CHAN(1),
+	SARADC_CHAN(2),
+	SARADC_CHAN(3),
+	SARADC_CHAN(4),
+	SARADC_CHAN(5),
+	SARADC_CHAN(6),
+	SARADC_CHAN(7),
+};
+
+/* Power on or down */
+static void starfive_saradc_pwr_on(struct starfive_saradc *priv, bool on)
+{
+	unsigned int val = readl(priv->base + SARADC_CTRL);
+
+	if (on)
+		writel(val & ~ADC_PWRDOWN_MSK, priv->base + SARADC_CTRL);
+	else
+		writel(val | ADC_PWRDOWN_MSK, priv->base + SARADC_CTRL);
+}
+
+static inline bool starfive_saradc_is_ready(struct starfive_saradc *priv)
+{
+	return !!(readl(priv->base + SARADC_CTRL) & ADC_IDLE_MSK);
+}
+
+static inline unsigned int starfive_saradc_data_get(struct starfive_saradc *priv)
+{
+	return readl(priv->base + SARADC_DATAX_REG_GET(priv->using_ch));
+}
+
+static inline void starfive_saradc_irq_clr(struct starfive_saradc *priv, u32 reg)
+{
+	writel(reg | ADC_IRQ_ST_MSK, priv->base + SARADC_IRQ_EN_ST);
+}
+
+static inline void starfive_saradc_data_clr_ch(struct starfive_saradc *priv, int ch)
+{
+	writel(ADC_DAT_RDY_MSK, priv->base + SARADC_DATAX_REG_GET(ch));
+}
+
+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++)
+		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);
+}
+
+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);
+
+	/* Enable conversion */
+	writel(reg | ADC_EN_MSK, priv->base + SARADC_CTRL);
+}
+
+static void starfive_saradc_ch_stop(struct starfive_saradc *priv)
+{
+	unsigned int reg = readl(priv->base + SARADC_CTRL);
+
+	/* Disable channel */
+	reg &= ~ADC_CHAN_EN_MSK;
+	writel(reg, priv->base + SARADC_CTRL);
+
+	if (priv->mon_en) {
+		/* Restore IRQ and re-enable channel */
+		reg = readl(priv->base + SARADC_CTRL) | SARADC_CHAN_EN(priv->mon_ch);
+		writel(reg, priv->base + SARADC_CTRL);
+		starfive_saradc_irq_clr_all(priv);
+
+		writel(SARADC_IRQ_CH_EN(priv->mon_ch), priv->base + SARADC_IRQ_EN_ST);
+		priv->mon_working = true;
+	}
+}
+
+static u32 starfive_saradc_scan_freq_get(struct starfive_saradc *priv)
+{
+	return readl(priv->base + SARADC_SCAN_FREQ) & ADC_SCAN_FREQ_MSK;
+}
+
+static void starfive_saradc_scan_freq_set(struct starfive_saradc *priv, u32 data)
+{
+	writel(data & ADC_SCAN_FREQ_MSK, priv->base + SARADC_SCAN_FREQ);
+}
+
+static u32 starfive_saradc_ch_upper_bound_get(struct starfive_saradc *priv, int ch)
+{
+	return FIELD_GET(ADC_UPPER_BOUND_MSK,
+			 readl(priv->base + SARADC_ULB_CHX_REG_GET(ch)));
+}
+
+static void starfive_saradc_ch_upper_bound_set(struct starfive_saradc *priv,
+					       int ch, u32 data)
+{
+	void __iomem *base = priv->base + SARADC_ULB_CHX_REG_GET(ch);
+	u32 reg = readl(base) & ~ADC_UPPER_BOUND_MSK;
+
+	writel(FIELD_PREP(ADC_UPPER_BOUND_MSK, data) | reg, base);
+}
+
+static u32 starfive_saradc_ch_lower_bound_get(struct starfive_saradc *priv, int ch)
+{
+	return FIELD_GET(ADC_LOWER_BOUND_MSK,
+			 readl(priv->base + SARADC_ULB_CHX_REG_GET(ch)));
+}
+
+static void starfive_saradc_ch_lower_bound_set(struct starfive_saradc *priv,
+					       int ch, u32 data)
+{
+	void __iomem *base = priv->base + SARADC_ULB_CHX_REG_GET(ch);
+	u32 reg = readl(base) & ~ADC_LOWER_BOUND_MSK;
+
+	writel(FIELD_PREP(ADC_LOWER_BOUND_MSK, data) | reg, base);
+}
+
+static ssize_t starfive_saradc_scan_freq_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);
+	int ret = pm_runtime_get_sync(priv->dev);
+	ssize_t len;
+
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	len = sprintf(buf, "%d\n", starfive_saradc_scan_freq_get(priv));
+	pm_runtime_put(priv->dev);
+
+	return len;
+}
+
+static ssize_t starfive_saradc_scan_freq_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	u32 freq;
+	int ret;
+
+	if (kstrtou32(buf, 10, &freq))
+		return -EINVAL;
+
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	if (freq < ADC_SCAN_FREQ_MIN || freq > ADC_SCAN_FREQ_MSK) {
+		dev_err(dev, "The data %d is out of range (%d - %ld).\n",
+			freq, ADC_SCAN_FREQ_MIN, ADC_SCAN_FREQ_MSK);
+		pm_runtime_put(priv->dev);
+		return -EINVAL;
+	}
+
+	starfive_saradc_scan_freq_set(priv, freq);
+	pm_runtime_put(priv->dev);
+
+	return len;
+}
+
+static ssize_t starfive_saradc_upper_bound_show(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+	int ch = iio_attr->address;
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	int ret = pm_runtime_get_sync(priv->dev);
+	ssize_t len;
+
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	len = sprintf(buf, "%d\n", priv->up_bounds[ch]);
+	pm_runtime_put(priv->dev);
+
+	return len;
+}
+
+static ssize_t starfive_saradc_upper_bound_store(struct device *dev,
+						 struct device_attribute *attr,
+						 const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+	int ch = iio_attr->address;
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	u32 upper;
+	int ret;
+
+	if (kstrtou32(buf, 10, &upper))
+		return -EINVAL;
+
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	starfive_saradc_ch_upper_bound_set(priv, ch, SARADC_VDD_MV_TO_RAW(upper));
+	priv->up_bounds[ch] = upper;
+	pm_runtime_put(priv->dev);
+
+	return len;
+}
+
+static ssize_t starfive_saradc_lower_bound_show(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+	int ch = iio_attr->address;
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	int ret = pm_runtime_get_sync(priv->dev);
+	ssize_t len;
+
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	len = sprintf(buf, "%d\n", priv->low_bounds[ch]);
+	pm_runtime_put(priv->dev);
+
+	return len;
+}
+
+static ssize_t starfive_saradc_lower_bound_store(struct device *dev,
+						 struct device_attribute *attr,
+						 const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+	int ch = iio_attr->address;
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	u32 lower;
+	int ret;
+
+	if (kstrtou32(buf, 10, &lower))
+		return -EINVAL;
+
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	starfive_saradc_ch_lower_bound_set(priv, ch, SARADC_VDD_MV_TO_RAW(lower));
+	priv->low_bounds[ch] = lower;
+	pm_runtime_put(priv->dev);
+
+	return len;
+}
+
+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);
+}
+
+static ssize_t starfive_saradc_monitor_channel_select(struct device *dev,
+						      struct device_attribute *attr,
+						      const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	u32 ch;
+
+	if (kstrtou32(buf, 10, &ch))
+		return -EINVAL;
+
+	if (ch >= SARADC_MAX_CHANNELS || ch < 0)
+		return -EINVAL;
+
+	priv->mon_ch = ch;
+
+	return len;
+}
+
+static ssize_t starfive_saradc_monitor_status(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_en);
+}
+
+static void starfive_saradc_ch_monitor_start(struct starfive_saradc *priv, u8 ch)
+{
+	u32 reg = readl(priv->base + SARADC_CTRL);
+
+	starfive_saradc_irq_clr(priv, BIT(ch));
+	starfive_saradc_data_clr_ch(priv, ch);
+
+	/* Enable channel */
+	reg |= SARADC_CHAN_EN(ch);
+	writel(reg, priv->base + SARADC_CTRL);
+
+	msleep(20);
+
+	/* Enable conversion */
+	writel(reg | ADC_EN_MSK, priv->base + SARADC_CTRL);
+
+	/* Enable IRQ */
+	reg = readl(priv->base + SARADC_IRQ_EN_ST);
+	writel(reg | SARADC_IRQ_CH_EN(ch), priv->base + SARADC_IRQ_EN_ST);
+
+	priv->mon_en = true;
+	priv->mon_working = true;
+}
+
+static void starfive_saradc_ch_monitor_stop(struct starfive_saradc *priv, u8 ch)
+{
+	unsigned int reg = readl(priv->base + SARADC_IRQ_EN_ST);
+
+	/* Disable IRQ */
+	writel(reg & ~SARADC_IRQ_CH_EN(ch), priv->base + SARADC_IRQ_EN_ST);
+	starfive_saradc_irq_clr(priv, BIT(ch));
+	starfive_saradc_data_clr_ch(priv, ch);
+
+	/* Disable channel */
+	reg = readl(priv->base + SARADC_CTRL) & ~SARADC_CHAN_EN(ch);
+	writel(reg, priv->base + SARADC_CTRL);
+
+	priv->mon_en = false;
+	priv->mon_working = false;
+}
+
+static ssize_t starfive_saradc_monitor_enable(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	u8 ch = priv->mon_ch;
+	u32 enable;
+	int ret;
+
+	if (kstrtou32(buf, 10, &enable))
+		return -EINVAL;
+
+	if (ch >= SARADC_MAX_CHANNELS || ch < 0)
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+	if (enable && !priv->mon_en) {
+		ret = pm_runtime_get_sync(priv->dev);
+		if (ret < 0) {
+			pm_runtime_put_noidle(priv->dev);
+			goto out;
+		}
+
+		if (!starfive_saradc_is_ready(priv)) {
+			dev_err(priv->dev, "ADC do not ready, please try again later!\n");
+			pm_runtime_put(priv->dev);
+			goto out;
+		}
+
+		starfive_saradc_ch_monitor_start(priv, ch);
+	} else if (!enable && priv->mon_en) {
+		starfive_saradc_ch_monitor_stop(priv, ch);
+		pm_runtime_put(priv->dev);
+	}
+
+out:
+	mutex_unlock(&priv->lock);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(scan_frequency, 0644,
+		       starfive_saradc_scan_freq_show,
+		       starfive_saradc_scan_freq_store, 0);
+static IIO_DEVICE_ATTR(in_voltage0_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 0);
+static IIO_DEVICE_ATTR(in_voltage1_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 1);
+static IIO_DEVICE_ATTR(in_voltage2_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 2);
+static IIO_DEVICE_ATTR(in_voltage3_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 3);
+static IIO_DEVICE_ATTR(in_voltage4_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 4);
+static IIO_DEVICE_ATTR(in_voltage5_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 5);
+static IIO_DEVICE_ATTR(in_voltage6_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 6);
+static IIO_DEVICE_ATTR(in_voltage7_upper, 0644,
+		       starfive_saradc_upper_bound_show,
+		       starfive_saradc_upper_bound_store, 7);
+static IIO_DEVICE_ATTR(in_voltage0_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 0);
+static IIO_DEVICE_ATTR(in_voltage1_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 1);
+static IIO_DEVICE_ATTR(in_voltage2_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 2);
+static IIO_DEVICE_ATTR(in_voltage3_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 3);
+static IIO_DEVICE_ATTR(in_voltage4_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 4);
+static IIO_DEVICE_ATTR(in_voltage5_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 5);
+static IIO_DEVICE_ATTR(in_voltage6_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 6);
+static IIO_DEVICE_ATTR(in_voltage7_lower, 0644,
+		       starfive_saradc_lower_bound_show,
+		       starfive_saradc_lower_bound_store, 7);
+static IIO_DEVICE_ATTR(voltage_monitor_channel, 0644,
+		       starfive_saradc_monitor_channel_show,
+		       starfive_saradc_monitor_channel_select, 2);
+static IIO_DEVICE_ATTR(voltage_monitor_en, 0644,
+		       starfive_saradc_monitor_status,
+		       starfive_saradc_monitor_enable, 0);
+
+static struct attribute *starfive_saradc_attributes[] = {
+	&iio_dev_attr_scan_frequency.dev_attr.attr,
+	&iio_dev_attr_in_voltage0_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage1_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage2_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage3_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage4_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage5_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage6_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage7_upper.dev_attr.attr,
+	&iio_dev_attr_in_voltage0_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage1_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage2_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage3_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage4_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage5_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage6_lower.dev_attr.attr,
+	&iio_dev_attr_in_voltage7_lower.dev_attr.attr,
+	&iio_dev_attr_voltage_monitor_channel.dev_attr.attr,
+	&iio_dev_attr_voltage_monitor_en.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group starfive_saradc_attr_group = {
+	.attrs = starfive_saradc_attributes,
+};
+
+static int starfive_saradc_read(struct starfive_saradc *priv)
+{
+	unsigned int tmp;
+	int ret;
+
+	starfive_saradc_ch_dis_save(priv);
+	if (!starfive_saradc_is_ready(priv)) {
+		dev_err(priv->dev, "ADC do not ready, please try again later!\n");
+		starfive_saradc_ch_stop(priv);
+		return -EBUSY;
+	}
+
+	priv->err = false;
+	starfive_saradc_ch_start(priv);
+
+	tmp = starfive_saradc_data_get(priv);
+	/* Check that the data is ready to be read. */
+	if (!(tmp & ADC_DAT_RDY_MSK)) {
+		ret = readl_poll_timeout(priv->base + SARADC_DATAX_REG_GET(priv->using_ch), tmp,
+					 (tmp & ADC_DAT_RDY_MSK), 10, SARADC_TIMEOUT);
+		if (ret) {
+			priv->err = true;
+			dev_err(priv->dev, "channel%d is still not ready to be read! Timeout!\n",
+				priv->using_ch);
+		}
+	}
+
+	if (priv->err)
+		tmp = 0;
+
+	starfive_saradc_ch_stop(priv);
+
+	return (int)(tmp & ADC_DAT_MSK);
+}
+
+static int starfive_saradc_read_raw(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    int *val, int *val2, long mask)
+{
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	int ret;
+	u64 tmp;
+
+	mutex_lock(&priv->lock);
+	priv->using_ch = chan->channel;
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		mutex_unlock(&priv->lock);
+		return ret;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = starfive_saradc_read(priv);
+		if (ret < 0)
+			break;
+
+		*val = ret;
+		ret = IIO_VAL_INT;
+		break;
+
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = starfive_saradc_read(priv);
+		if (ret < 0)
+			break;
+
+		/* VIN = AVDD * data[11:0] / 4096. (AVDD = 1.8v) */
+		tmp = SARADC_RAW_TO_VDD_ADJ(ret);
+		*val = (int)(tmp / 1000000);
+		*val2 = (int)(tmp % 1000000);
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * AVDD is fixed at 1.8v.
+		 * 1.8 / (1 << 12) * 1000000
+		 */
+		*val = 0;
+		*val2 = SARADC_AVDD_VOL / (1 << SARADC_REALBITS);
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	pm_runtime_put_autosuspend(priv->dev);
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static irqreturn_t starfive_saradc_mon_stop_threadfn(int irq, void *data)
+{
+	struct starfive_saradc *priv = data;
+	u8 ch = priv->mon_ch;
+	u32 up, low, raw;
+
+	mutex_lock(&priv->lock);
+	if (!priv->mon_en) {
+		mutex_unlock(&priv->lock);
+		return IRQ_HANDLED;
+	}
+
+	raw = readl(priv->base + SARADC_DATAX_REG_GET(ch)) & ADC_DAT_MSK;
+	up = starfive_saradc_ch_upper_bound_get(priv, ch);
+	low = starfive_saradc_ch_lower_bound_get(priv, ch);
+	dev_err(priv->dev,
+		"channel %d is out of bounds. sample data: %dmv (range: %dmv ~ %dmv)\n",
+		ch, (SARADC_RAW_TO_VDD_ADJ(raw) / 1000),
+		priv->low_bounds[ch], priv->up_bounds[ch]);
+
+	starfive_saradc_ch_monitor_stop(priv, ch);
+	pm_runtime_put_autosuspend(priv->dev);
+	mutex_unlock(&priv->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t starfive_saradc_irq_handler(int irq, void *data)
+{
+	struct starfive_saradc *priv = data;
+	u32 irq_err = readl(priv->base + SARADC_IRQ_EN_ST);
+
+	if (!priv->mon_working)
+		return IRQ_HANDLED;
+
+	/* Error of out of bounds */
+	if (irq_err & BIT(priv->mon_ch)) {
+		/* Clear the interrupt */
+		writel(irq_err, priv->base + SARADC_IRQ_EN_ST);
+		priv->err = true;
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void starfive_saradc_init(struct starfive_saradc *priv)
+{
+	bool use_def = false;
+	u16 up, low, scan, tmp;
+	u32 upmv, lowmv;
+	int i;
+
+	if (of_property_read_u16(priv->dev->of_node, "upper-bound-mv", &tmp)) {
+		use_def = true;
+	} else {
+		up = SARADC_VDD_MV_TO_RAW(tmp);
+		if (up > ADC_UPPER_BOUND_DEF)
+			use_def = true;
+		else
+			upmv = tmp;
+	}
+
+	if (use_def) {
+		up = ADC_UPPER_BOUND_DEF;
+		upmv = SARADC_RAW_TO_VDD_ADJ(up);
+		use_def = false;
+	}
+
+	if (of_property_read_u16(priv->dev->of_node, "lower-bound-mv", &tmp)) {
+		use_def = true;
+	} else {
+		low = SARADC_VDD_MV_TO_RAW(tmp);
+		if (low > ADC_UPPER_BOUND_DEF)
+			use_def = true;
+		else
+			lowmv = tmp;
+	}
+
+	if (use_def) {
+		low = ADC_LOWER_BOUND_DEF;
+		lowmv = SARADC_RAW_TO_VDD_ADJ(low);
+	}
+
+	if (of_property_read_u16(priv->dev->of_node, "scan-freq", &scan))
+		scan = ADC_SCAN_FREQ_DEF;
+
+	if ((scan & ADC_SCAN_FREQ_MSK) < ADC_SCAN_FREQ_MIN) {
+		dev_warn(priv->dev, "The scan_freq is out of range and use the default value!\n");
+		scan = ADC_SCAN_FREQ_DEF;
+	}
+
+	starfive_saradc_scan_freq_set(priv, scan);
+
+	for (i = 0; i < SARADC_MAX_CHANNELS; i++) {
+		starfive_saradc_ch_upper_bound_set(priv, i, up);
+		starfive_saradc_ch_lower_bound_set(priv, i, low);
+		priv->up_bounds[i] = upmv;
+		priv->low_bounds[i] = lowmv;
+	}
+}
+
+static int starfive_saradc_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				      unsigned int writeval, unsigned int *readval)
+{
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (reg % 4 || reg > SARADC_SCAN_FREQ)
+		return -EINVAL;
+
+	ret = pm_runtime_get_sync(priv->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(priv->dev);
+		return ret;
+	}
+
+	mutex_lock(&priv->lock);
+
+	if (readval)
+		*readval = readl(priv->base + reg);
+	else if (reg < SARADC_ULB_CH0)
+		/* Allowed to write from SARADC_ULB_CH0 to SARADC_SCAN_FREQ */
+		ret = -EINVAL;
+	else
+		writel(writeval, priv->base + reg);
+
+	mutex_unlock(&priv->lock);
+	pm_runtime_put_sync_autosuspend(priv->dev);
+
+	return ret;
+}
+
+static const struct iio_info starfive_saradc_iio_info = {
+	.read_raw = starfive_saradc_read_raw,
+	.debugfs_reg_access = starfive_saradc_reg_access,
+	.attrs = &starfive_saradc_attr_group,
+};
+
+static int starfive_saradc_probe(struct platform_device *pdev)
+{
+	struct starfive_saradc *priv;
+	struct iio_dev *indio_dev;
+	int irq, ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
+	if (!indio_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "failed allocating iio device\n");
+
+	priv = iio_priv(indio_dev);
+	platform_set_drvdata(pdev, indio_dev);
+	priv->dev = &pdev->dev;
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return dev_err_probe(&pdev->dev, irq,
+				     "failed to get irq\n");
+
+	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");
+
+	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk),
+				     "failed to get clock\n");
+
+	priv->rst = devm_reset_control_array_get_shared(&pdev->dev);
+	if (IS_ERR(priv->rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rst),
+				     "failed to get resets\n");
+
+	ret = reset_control_deassert(priv->rst);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to deassert reset\n");
+
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &starfive_saradc_iio_info;
+	indio_dev->channels = starfive_saradc_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(starfive_saradc_iio_channels);
+
+	starfive_saradc_init(priv);
+	mutex_init(&priv->lock);
+
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	return devm_iio_device_register(&pdev->dev, indio_dev);
+}
+
+static void starfive_saradc_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+}
+
+static int starfive_saradc_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+
+	starfive_saradc_pwr_on(priv, false);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static int starfive_saradc_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct starfive_saradc *priv = iio_priv(indio_dev);
+	int ret = clk_prepare_enable(priv->clk);
+
+	if (ret)
+		return ret;
+
+	starfive_saradc_pwr_on(priv, true);
+	/* Need time to completely power on. */
+	msleep(20);
+
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(starfive_saradc_pm_ops,
+				 starfive_saradc_runtime_suspend,
+				 starfive_saradc_runtime_resume, NULL);
+
+static const struct of_device_id starfive_saradc_of_match[] = {
+	{ .compatible = "starfive,jhb100-saradc", },
+	{ }
+};
+
+static struct platform_driver starfive_saradc_driver = {
+	.probe	= starfive_saradc_probe,
+	.remove	= starfive_saradc_remove,
+	.driver	= {
+		.name = "starfive_saradc",
+		.of_match_table = starfive_saradc_of_match,
+		.pm = pm_ptr(&starfive_saradc_pm_ops)
+	},
+};
+
+module_platform_driver(starfive_saradc_driver);
+
+MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
+MODULE_DESCRIPTION("StarFive Successive Approximation Register ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Typo in the `reg` property schema keyword where `maxItem` is used instead of `maxItems`.
- [Low] Hardware-specific properties `upper-bound-mv`, `lower-bound-mv`, and `scan-freq` lack a vendor prefix.
- [Low] The `resets` property allows 2 items but does not document their order or meaning.
--

commit f51adfa18df9b732393baa87f11dbef506bb4b5b
Author: Xingyu Wu <xingyu.wu@starfivetech.com>

bindings: iio: adc: Add StarFive JHB100 SARADC

This commit adds a new device tree binding document for the Successive
Approximation Register A/D converter found on the StarFive JHB100 SoC.

> diff --git a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> new file mode 100644
> index 0000000000000..ba8e19b72ad71
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> @@ -0,0 +1,62 @@
[ ... ]
> +properties:
> +  compatible:
> +    const: starfive,jhb100-saradc
> +
> +  reg:
> +    maxItem: 1

This isn't a bug, but should this be maxItems instead of maxItem? The
dt-schema validator might silently ignore the unrecognized keyword, allowing
nodes with multiple reg entries to pass validation.

[ ... ]
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 2

When allowing multiple reset lines, should the binding define their expected
order and meaning? Usually, this is done by providing an items list with
descriptions for each entry, or by defining a corresponding reset-names
property.

[ ... ]
> +  "#io-channel-cells":
> +    const: 1
> +
> +  upper-bound-mv:
> +    description: The upper bound voltage value of the monitor.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +  lower-bound-mv:
> +    description: The lower bound voltage value of the monitor.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +  scan-freq:
> +    description: Number of the scan cycle interval.
> +    $ref: /schemas/types.yaml#/definitions/uint16

Since these appear to be custom, hardware-specific properties, should they be
prefixed with the vendor string (e.g., starfive,upper-bound-mv) to prevent
future namespace collisions with generic properties?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518081852.116909-1-xingyu.wu@starfivetech.com?part=1

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

[Andy Shevchenko]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] System crash due to requesting IRQ before enabling hardware clocks.
- [High] IRQ handler unconditionally returns IRQ_HANDLED without clearing interrupts, leading to infinite IRQ storms.
- [High] Clock reference count underflow due to incompatible mixing of devres and manual clock management.
- [High] Missing cleanup for runtime PM and shared resets in error and removal paths.
- [High] Data race on the `SARADC_IRQ_EN_ST` register causes lost interrupt enables.
- [High] Active hardware monitor is not stopped during device removal.
- [High] Lockless sysfs store functions cause data corruption and IRQ storms.
- [Medium] Multiple IIO ABI violations regarding standard events and data units.
- [Medium] Silent failure on ADC read timeouts.
--

commit 38cba5c631bad8026785f6fd6a6919692ab720de
Author: Xingyu Wu <xingyu.wu@starfivetech.com>
Subject: iio: adc: Add StarFive SAR-ADC driver

Add a new IIO ADC driver for the StarFive JHB100 SAR ADC controller.

> diff --git a/drivers/iio/adc/starfive-saradc.c b/drivers/iio/adc/starfive-saradc.c
> index 0000000000000..78409cb1bcb29
> --- /dev/null
> +++ b/drivers/iio/adc/starfive-saradc.c
> @@ -0,0 +1,978 @@

[ ... ]

> +static void starfive_saradc_ch_monitor_start(struct starfive_saradc *priv, u8 ch)
> +{
[ ... ]
> +	/* Enable IRQ */
> +	reg = readl(priv->base + SARADC_IRQ_EN_ST);
> +	writel(reg | SARADC_IRQ_CH_EN(ch), priv->base + SARADC_IRQ_EN_ST);

Could this race with the interrupt handler? The hardirq handler reads and
writes back to SARADC_IRQ_EN_ST to clear status bits without holding locks.
If this thread context reads, modifies, and writes the register, could the
hardirq handler concurrently write back a stale value and overwrite the
interrupt enable bits set here?

[ ... ]

> +static ssize_t starfive_saradc_upper_bound_store(struct device *dev,
> +						 struct device_attribute *attr,
> +						 const char *buf, size_t len)
> +{
[ ... ]
> +	starfive_saradc_ch_upper_bound_set(priv, ch, SARADC_VDD_MV_TO_RAW(upper));
> +	priv->up_bounds[ch] = upper;
> +	pm_runtime_put(priv->dev);
> +
> +	return len;
> +}

Does this need to hold priv->lock? The starfive_saradc_ch_upper_bound_set
function performs a read-modify-write on shared hardware registers, which
might allow concurrent sysfs writes to corrupt the bounds.

[ ... ]

> +static ssize_t starfive_saradc_monitor_channel_select(struct device *dev,
> +						      struct device_attribute *attr,
> +						      const char *buf, size_t len)
> +{
[ ... ]
> +	if (ch >= SARADC_MAX_CHANNELS || ch < 0)
> +		return -EINVAL;
> +
> +	priv->mon_ch = ch;
> +
> +	return len;
> +}

Does this update priv->mon_ch without acquiring priv->lock? If the channel
is changed while the monitor is active, could the IRQ handler check the
wrong bit, fail to clear the interrupt, and trigger an infinite IRQ storm?

[ ... ]

> +static IIO_DEVICE_ATTR(in_voltage0_upper, 0644,
> +		       starfive_saradc_upper_bound_show,
> +		       starfive_saradc_upper_bound_store, 0);
[ ... ]
> +static IIO_DEVICE_ATTR(voltage_monitor_en, 0644,
> +		       starfive_saradc_monitor_status,
> +		       starfive_saradc_monitor_enable, 0);

Should this driver use standard IIO events (IIO_EV_TYPE_THRESH) instead of
introducing custom sysfs attributes for threshold monitoring?

[ ... ]

> +static int starfive_saradc_read(struct starfive_saradc *priv)
> +{
[ ... ]
> +	tmp = starfive_saradc_data_get(priv);
> +	/* Check that the data is ready to be read. */
> +	if (!(tmp & ADC_DAT_RDY_MSK)) {
> +		ret = readl_poll_timeout(priv->base + SARADC_DATAX_REG_GET(priv->using_ch), tmp,
> +					 (tmp & ADC_DAT_RDY_MSK), 10, SARADC_TIMEOUT);
> +		if (ret) {
> +			priv->err = true;
> +			dev_err(priv->dev, "channel%d is still not ready to be read! Timeout!\n",
> +				priv->using_ch);
> +		}
> +	}
> +
> +	if (priv->err)
> +		tmp = 0;
> +
> +	starfive_saradc_ch_stop(priv);
> +
> +	return (int)(tmp & ADC_DAT_MSK);
> +}

If the poll times out, this sets tmp to 0 and returns it. Will this result
in starfive_saradc_read_raw passing 0 to userspace as a valid ADC reading
instead of propagating an error code like -ETIMEDOUT?

[ ... ]

> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = starfive_saradc_read(priv);
> +		if (ret < 0)
> +			break;
> +
> +		/* VIN = AVDD * data[11:0] / 4096. (AVDD = 1.8v) */
> +		tmp = SARADC_RAW_TO_VDD_ADJ(ret);
> +		*val = (int)(tmp / 1000000);
> +		*val2 = (int)(tmp % 1000000);
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * AVDD is fixed at 1.8v.
> +		 * 1.8 / (1 << 12) * 1000000
> +		 */
> +		*val = 0;
> +		*val2 = SARADC_AVDD_VOL / (1 << SARADC_REALBITS);
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;

Does this calculate the scale to expose Volts instead of millivolts? The
calculation yields 439 instead of 0.439453, meaning users multiplying raw
data by the exposed scale will get Volts. This violates the IIO user-space
ABI requirement of millivolts.

Similarly, IIO_CHAN_INFO_PROCESSED appears to be formatting the value as
fractional Volts rather than millivolts.

[ ... ]

> +static irqreturn_t starfive_saradc_irq_handler(int irq, void *data)
> +{
> +	struct starfive_saradc *priv = data;
> +	u32 irq_err = readl(priv->base + SARADC_IRQ_EN_ST);
> +
> +	if (!priv->mon_working)
> +		return IRQ_HANDLED;
> +
> +	/* Error of out of bounds */
> +	if (irq_err & BIT(priv->mon_ch)) {
> +		/* Clear the interrupt */
> +		writel(irq_err, priv->base + SARADC_IRQ_EN_ST);
> +		priv->err = true;
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}

If priv->mon_working is false, or if the interrupt bit is not set, this
returns IRQ_HANDLED without clearing the hardware interrupt status bits.
Since the interrupt is requested with IRQF_TRIGGER_HIGH, won't failing to
clear the source while returning IRQ_HANDLED cause the interrupt controller
to immediately re-trigger, resulting in an infinite IRQ storm?

[ ... ]

> +static void starfive_saradc_init(struct starfive_saradc *priv)
> +{
[ ... ]
> +	if (of_property_read_u16(priv->dev->of_node, "upper-bound-mv", &tmp)) {

Are the default Device Tree bounds being parsed into microvolts here but
exposed to sysfs where users expect millivolts?

[ ... ]

> +static int starfive_saradc_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	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");
> +
> +	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);

Since the IRQ is requested before the clock is enabled, could a pending or
spurious interrupt fire immediately upon registration? If the handler
executes and accesses the clock-gated MMIO register, it could cause a
synchronous external abort or bus stall.

> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk),
> +				     "failed to get clock\n");
> +
> +	priv->rst = devm_reset_control_array_get_shared(&pdev->dev);
> +	if (IS_ERR(priv->rst))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rst),
> +				     "failed to get resets\n");
> +
> +	ret = reset_control_deassert(priv->rst);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to deassert reset\n");
[ ... ]
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return devm_iio_device_register(&pdev->dev, indio_dev);
> +}

If devm_iio_device_register fails, this returns the error without calling
pm_runtime_disable. Does this leave the PM framework active on freed
devres memory?

Additionally, the shared reset line is deasserted here, but
reset_control_assert is never called in the error paths or in
starfive_saradc_remove. Could this permanently leak the shared reset line's
reference count and prevent the hardware block from ever being reset again?

> +static void starfive_saradc_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +}

If continuous monitor mode is enabled (priv->mon_en = true), the hardware
is actively sampling and generating interrupts. Does this leave the monitor
running and the interrupt unmasked when the driver unbinds, potentially
causing bus lockups when devres subsequently disables the clock?

> +static int starfive_saradc_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct starfive_saradc *priv = iio_priv(indio_dev);
> +
> +	starfive_saradc_pwr_on(priv, false);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}

The clock is acquired via devm_clk_get_enabled in probe, which queues an
automatic disable. Here, the driver manually manages the clock. If the
driver is unbound while the device is runtime-suspended, devres will
execute its queued clk_disable_unprepare. Will this drop the enable
count below zero, causing a kernel warning and resource tracking
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518081852.116909-1-xingyu.wu@starfivetech.com?part=2

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Rob Herring (Arm)]

On Mon, 18 May 2026 16:18:51 +0800, Xingyu Wu wrote:
> Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml: properties:reg: 'anyOf' conditional failed, one must be fixed:
	'maxItem' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
	'type' was expected
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml: properties:reg: 'anyOf' conditional failed, one must be fixed:
	'maxItems' is a required property
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	'maxItem' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	Additional properties are not allowed ('maxItem' was unexpected)
		hint: Arrays must be described with a combination of minItems/maxItems/items
	'maxItem' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
	hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/cell.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260518081852.116909-2-xingyu.wu@starfivetech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> new file mode 100644
> index 000000000000..ba8e19b72ad7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Successive Approximation Register (SAR) A/D converter for the StarFive JHB100 SoC
> +
> +maintainers:
> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    const: starfive,jhb100-saradc
> +
> +  reg:
> +    maxItem: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 2
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  upper-bound-mv:
> +    description: The upper bound voltage value of the monitor.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +  lower-bound-mv:
> +    description: The lower bound voltage value of the monitor.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +  scan-freq:
> +    description: Number of the scan cycle interval.
> +    $ref: /schemas/types.yaml#/definitions/uint16

Can you explain why any of these three properties are something that
should be in the devicetree rather than software controlled?

How are the bounds calculated?

Given the warnings etc,
pw-bot: changes-requested

Cheers,
Conor.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - resets
> +  - "#io-channel-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    adc@11be1400 {
> +      compatible = "starfive,jhb100-saradc";
> +      reg = <0x11be1400 0x400>;
> +      interrupts = <172>;
> +      clocks = <&per0crg 18>;
> +      resets = <&per0crg 11>, <&per0crg 46>;
> +      #io-channel-cells = <1>;
> +      };
> -- 
> 2.34.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

On 2026/5/18 16:24, Sashiko wrote:
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to
> consider:
> - [Low] Typo in the `reg` property schema keyword where `maxItem` is used
> instead of `maxItems`.
> - [Low] Hardware-specific properties `upper-bound-mv`, `lower-bound-mv`, and
> `scan-freq` lack a vendor prefix.
> - [Low] The `resets` property allows 2 items but does not document their order or
> meaning.
> --
> 
> commit f51adfa18df9b732393baa87f11dbef506bb4b5b
> Author: Xingyu Wu <xingyu.wu@starfivetech.com>
> 
> bindings: iio: adc: Add StarFive JHB100 SARADC
> 
> This commit adds a new device tree binding document for the Successive
> Approximation Register A/D converter found on the StarFive JHB100 SoC.
> 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > l
> > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > l
> > new file mode 100644
> > index 0000000000000..ba8e19b72ad71
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > +++ .yaml
> > @@ -0,0 +1,62 @@
> [ ... ]
> > +properties:
> > +  compatible:
> > +    const: starfive,jhb100-saradc
> > +
> > +  reg:
> > +    maxItem: 1
> 
> This isn't a bug, but should this be maxItems instead of maxItem? The dt-schema
> validator might silently ignore the unrecognized keyword, allowing nodes with
> multiple reg entries to pass validation.

Will fix.

> 
> [ ... ]
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 2
> 
> When allowing multiple reset lines, should the binding define their expected order
> and meaning? Usually, this is done by providing an items list with descriptions for
> each entry, or by defining a corresponding reset-names property.

Noted.

> 
> [ ... ]
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +  upper-bound-mv:
> > +    description: The upper bound voltage value of the monitor.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +  lower-bound-mv:
> > +    description: The lower bound voltage value of the monitor.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +  scan-freq:
> > +    description: Number of the scan cycle interval.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> 
> Since these appear to be custom, hardware-specific properties, should they be
> prefixed with the vendor string (e.g., starfive,upper-bound-mv) to prevent future
> namespace collisions with generic properties?

Noted.

Thanks,
Xingyu Wu

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

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

[Xingyu Wu]

On 2026/5/18 16:48, Sashiko wrote:
> 
> Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to
> consider:
> - [High] System crash due to requesting IRQ before enabling hardware clocks.
> - [High] IRQ handler unconditionally returns IRQ_HANDLED without clearing
> interrupts, leading to infinite IRQ storms.
> - [High] Clock reference count underflow due to incompatible mixing of devres and
> manual clock management.
> - [High] Missing cleanup for runtime PM and shared resets in error and removal
> paths.
> - [High] Data race on the `SARADC_IRQ_EN_ST` register causes lost interrupt
> enables.
> - [High] Active hardware monitor is not stopped during device removal.
> - [High] Lockless sysfs store functions cause data corruption and IRQ storms.
> - [Medium] Multiple IIO ABI violations regarding standard events and data units.
> - [Medium] Silent failure on ADC read timeouts.
> --
> 
> commit 38cba5c631bad8026785f6fd6a6919692ab720de
> Author: Xingyu Wu <xingyu.wu@starfivetech.com>
> Subject: iio: adc: Add StarFive SAR-ADC driver
> 
> Add a new IIO ADC driver for the StarFive JHB100 SAR ADC controller.
> 
> > diff --git a/drivers/iio/adc/starfive-saradc.c
> > b/drivers/iio/adc/starfive-saradc.c
> > index 0000000000000..78409cb1bcb29
> > --- /dev/null
> > +++ b/drivers/iio/adc/starfive-saradc.c
> > @@ -0,0 +1,978 @@
> 
> [ ... ]
> 
> > +static void starfive_saradc_ch_monitor_start(struct starfive_saradc
> > +*priv, u8 ch) {
> [ ... ]
> > +	/* Enable IRQ */
> > +	reg = readl(priv->base + SARADC_IRQ_EN_ST);
> > +	writel(reg | SARADC_IRQ_CH_EN(ch), priv->base + SARADC_IRQ_EN_ST);
> 
> Could this race with the interrupt handler? The hardirq handler reads and writes
> back to SARADC_IRQ_EN_ST to clear status bits without holding locks.
> If this thread context reads, modifies, and writes the register, could the hardirq
> handler concurrently write back a stale value and overwrite the interrupt enable
> bits set here?

The function "starfive_saradc_monitor_enable()", which call this, already hold the lock.

> 
> [ ... ]
> 
> > +static ssize_t starfive_saradc_upper_bound_store(struct device *dev,
> > +						 struct device_attribute *attr,
> > +						 const char *buf, size_t len)
> > +{
> [ ... ]
> > +	starfive_saradc_ch_upper_bound_set(priv, ch,
> SARADC_VDD_MV_TO_RAW(upper));
> > +	priv->up_bounds[ch] = upper;
> > +	pm_runtime_put(priv->dev);
> > +
> > +	return len;
> > +}
> 
> Does this need to hold priv->lock? The starfive_saradc_ch_upper_bound_set
> function performs a read-modify-write on shared hardware registers, which might
> allow concurrent sysfs writes to corrupt the bounds.

Will add the lock.

> 
> [ ... ]
> 
> > +static ssize_t starfive_saradc_monitor_channel_select(struct device *dev,
> > +						      struct device_attribute *attr,
> > +						      const char *buf, size_t len) {
> [ ... ]
> > +	if (ch >= SARADC_MAX_CHANNELS || ch < 0)
> > +		return -EINVAL;
> > +
> > +	priv->mon_ch = ch;
> > +
> > +	return len;
> > +}
> 
> Does this update priv->mon_ch without acquiring priv->lock? If the channel is
> changed while the monitor is active, could the IRQ handler check the wrong bit, fail
> to clear the interrupt, and trigger an infinite IRQ storm?

Thanks for your reminder. I will add the check about the status of monitor.
If it is not working, we can change the priv->mon_ch.

> 
> [ ... ]
> 
> > +static IIO_DEVICE_ATTR(in_voltage0_upper, 0644,
> > +		       starfive_saradc_upper_bound_show,
> > +		       starfive_saradc_upper_bound_store, 0);
> [ ... ]
> > +static IIO_DEVICE_ATTR(voltage_monitor_en, 0644,
> > +		       starfive_saradc_monitor_status,
> > +		       starfive_saradc_monitor_enable, 0);
> 
> Should this driver use standard IIO events (IIO_EV_TYPE_THRESH) instead of
> introducing custom sysfs attributes for threshold monitoring?

Noted.

> 
> [ ... ]
> 
> > +static int starfive_saradc_read(struct starfive_saradc *priv) {
> [ ... ]
> > +	tmp = starfive_saradc_data_get(priv);
> > +	/* Check that the data is ready to be read. */
> > +	if (!(tmp & ADC_DAT_RDY_MSK)) {
> > +		ret = readl_poll_timeout(priv->base +
> SARADC_DATAX_REG_GET(priv->using_ch), tmp,
> > +					 (tmp & ADC_DAT_RDY_MSK), 10,
> SARADC_TIMEOUT);
> > +		if (ret) {
> > +			priv->err = true;
> > +			dev_err(priv->dev, "channel%d is still not ready to be read!
> Timeout!\n",
> > +				priv->using_ch);
> > +		}
> > +	}
> > +
> > +	if (priv->err)
> > +		tmp = 0;
> > +
> > +	starfive_saradc_ch_stop(priv);
> > +
> > +	return (int)(tmp & ADC_DAT_MSK);
> > +}
> 
> If the poll times out, this sets tmp to 0 and returns it. Will this result in
> starfive_saradc_read_raw passing 0 to userspace as a valid ADC reading instead of
> propagating an error code like -ETIMEDOUT?

Noted.

> 
> [ ... ]
> 
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		ret = starfive_saradc_read(priv);
> > +		if (ret < 0)
> > +			break;
> > +
> > +		/* VIN = AVDD * data[11:0] / 4096. (AVDD = 1.8v) */
> > +		tmp = SARADC_RAW_TO_VDD_ADJ(ret);
> > +		*val = (int)(tmp / 1000000);
> > +		*val2 = (int)(tmp % 1000000);
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> > +		break;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		/*
> > +		 * AVDD is fixed at 1.8v.
> > +		 * 1.8 / (1 << 12) * 1000000
> > +		 */
> > +		*val = 0;
> > +		*val2 = SARADC_AVDD_VOL / (1 << SARADC_REALBITS);
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> > +		break;
> 
> Does this calculate the scale to expose Volts instead of millivolts? The calculation
> yields 439 instead of 0.439453, meaning users multiplying raw data by the
> exposed scale will get Volts. This violates the IIO user-space ABI requirement of
> millivolts.
> 
> Similarly, IIO_CHAN_INFO_PROCESSED appears to be formatting the value as
> fractional Volts rather than millivolts.

Will fix.

> 
> [ ... ]
> 
> > +static irqreturn_t starfive_saradc_irq_handler(int irq, void *data) {
> > +	struct starfive_saradc *priv = data;
> > +	u32 irq_err = readl(priv->base + SARADC_IRQ_EN_ST);
> > +
> > +	if (!priv->mon_working)
> > +		return IRQ_HANDLED;
> > +
> > +	/* Error of out of bounds */
> > +	if (irq_err & BIT(priv->mon_ch)) {
> > +		/* Clear the interrupt */
> > +		writel(irq_err, priv->base + SARADC_IRQ_EN_ST);
> > +		priv->err = true;
> > +		return IRQ_WAKE_THREAD;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> If priv->mon_working is false, or if the interrupt bit is not set, this returns
> IRQ_HANDLED without clearing the hardware interrupt status bits.
> Since the interrupt is requested with IRQF_TRIGGER_HIGH, won't failing to clear
> the source while returning IRQ_HANDLED cause the interrupt controller to
> immediately re-trigger, resulting in an infinite IRQ storm?

Will fix.

> 
> [ ... ]
> 
> > +static void starfive_saradc_init(struct starfive_saradc *priv) {
> [ ... ]
> > +	if (of_property_read_u16(priv->dev->of_node, "upper-bound-mv",
> > +&tmp)) {
> 
> Are the default Device Tree bounds being parsed into  here but exposed
> to sysfs where users expect millivolts?

We hope to measure in millivolts to reduce the large errors caused by microvolts.

> 
> [ ... ]
> 
> > +static int starfive_saradc_probe(struct platform_device *pdev) {
> [ ... ]
> > +	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");
> > +
> > +	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> 
> Since the IRQ is requested before the clock is enabled, could a pending or spurious
> interrupt fire immediately upon registration? If the handler executes and accesses
> the clock-gated MMIO register, it could cause a synchronous external abort or bus
> stall.

Will fix.

> 
> > +	if (IS_ERR(priv->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk),
> > +				     "failed to get clock\n");
> > +
> > +	priv->rst = devm_reset_control_array_get_shared(&pdev->dev);
> > +	if (IS_ERR(priv->rst))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rst),
> > +				     "failed to get resets\n");
> > +
> > +	ret = reset_control_deassert(priv->rst);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to deassert reset\n");
> [ ... ]
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	return devm_iio_device_register(&pdev->dev, indio_dev); }
> 
> If devm_iio_device_register fails, this returns the error without calling
> pm_runtime_disable. Does this leave the PM framework active on freed devres
> memory?
> 
> Additionally, the shared reset line is deasserted here, but reset_control_assert is
> never called in the error paths or in starfive_saradc_remove. Could this
> permanently leak the shared reset line's reference count and prevent the hardware
> block from ever being reset again?

Will fix.

> 
> > +static void starfive_saradc_remove(struct platform_device *pdev) {
> > +	pm_runtime_disable(&pdev->dev);
> > +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > +}
> 
> If continuous monitor mode is enabled (priv->mon_en = true), the hardware is
> actively sampling and generating interrupts. Does this leave the monitor running
> and the interrupt unmasked when the driver unbinds, potentially causing bus
> lockups when devres subsequently disables the clock?

Your concern is correct. I will disable monitor and power down device when driver is removed.

> 
> > +static int starfive_saradc_runtime_suspend(struct device *dev) {
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct starfive_saradc *priv = iio_priv(indio_dev);
> > +
> > +	starfive_saradc_pwr_on(priv, false);
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return 0;
> > +}
> 
> The clock is acquired via devm_clk_get_enabled in probe, which queues an
> automatic disable. Here, the driver manually manages the clock. If the driver is
> unbound while the device is runtime-suspended, devres will execute its queued
> clk_disable_unprepare. Will this drop the enable count below zero, causing a kernel
> warning and resource tracking corruption?

Will fix.

> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260518081852.116909-1-
> xingyu.wu@starfivetech.com?part=2

Best regards,
Xingyu Wu

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

On 2026/5/19 00:24, Conor Dooley wrote:
> 
> On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> >
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > ---
> >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > l
> > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > l
> > new file mode 100644
> > index 000000000000..ba8e19b72ad7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > +++ .yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Successive Approximation Register (SAR) A/D converter for the
> > +StarFive JHB100 SoC
> > +
> > +maintainers:
> > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: starfive,jhb100-saradc
> > +
> > +  reg:
> > +    maxItem: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 2
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +  upper-bound-mv:
> > +    description: The upper bound voltage value of the monitor.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +  lower-bound-mv:
> > +    description: The lower bound voltage value of the monitor.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +  scan-freq:
> > +    description: Number of the scan cycle interval.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> 
> Can you explain why any of these three properties are something that should be in
> the devicetree rather than software controlled?

My intention is to be able to obtain the initial values from the devicetree during probe and preset them.
Do I need to drop them and just set them through sysfs?

> 
> How are the bounds calculated?

The measurement range of this ADC hardware is from 0 to 1800 mV. This set value cannot exceed it. This explanation will be added later.

Best regards,
Xingyu Wu

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3013 bytes --]

On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> On 2026/5/19 00:24, Conor Dooley wrote:
> > 
> > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > >
> > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > ---
> > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > > l
> > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > > l
> > > new file mode 100644
> > > index 000000000000..ba8e19b72ad7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > > +++ .yaml
> > > @@ -0,0 +1,62 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id:
> > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Successive Approximation Register (SAR) A/D converter for the
> > > +StarFive JHB100 SoC
> > > +
> > > +maintainers:
> > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: starfive,jhb100-saradc
> > > +
> > > +  reg:
> > > +    maxItem: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 2
> > > +
> > > +  "#io-channel-cells":
> > > +    const: 1
> > > +
> > > +  upper-bound-mv:
> > > +    description: The upper bound voltage value of the monitor.
> > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > +
> > > +  lower-bound-mv:
> > > +    description: The lower bound voltage value of the monitor.
> > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > +
> > > +  scan-freq:
> > > +    description: Number of the scan cycle interval.
> > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > 
> > Can you explain why any of these three properties are something that should be in
> > the devicetree rather than software controlled?
> 
> My intention is to be able to obtain the initial values from the devicetree during probe and preset them.
> Do I need to drop them and just set them through sysfs?

Unless the hardware configuration determines the values (which I can't
really see being the case for scan-freq at least) then yes, you need to
drop and set them via sysfs.

> > How are the bounds calculated?
> 
> The measurement range of this ADC hardware is from 0 to 1800 mV. This set value cannot exceed it. This explanation will be added later.

I'm asking how this is calculated so that I can tell if you the property
is permitted or not. 

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

On 2026/5/19 18:00, Conor Dooley wrote:
> 
> On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > On 2026/5/19 00:24, Conor Dooley wrote:
> > >
> > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > >
> > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > ---
> > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > >  1 file changed, 62 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.y
> > > > aml
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > > > .yam
> > > > l
> > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > > > .yam
> > > > l
> > > > new file mode 100644
> > > > index 000000000000..ba8e19b72ad7
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > +++ radc
> > > > +++ .yaml
> > > > @@ -0,0 +1,62 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > > +---
> > > > +$id:
> > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml
> > > > +#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Successive Approximation Register (SAR) A/D converter for
> > > > +the StarFive JHB100 SoC
> > > > +
> > > > +maintainers:
> > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: starfive,jhb100-saradc
> > > > +
> > > > +  reg:
> > > > +    maxItem: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    maxItems: 1
> > > > +
> > > > +  resets:
> > > > +    maxItems: 2
> > > > +
> > > > +  "#io-channel-cells":
> > > > +    const: 1
> > > > +
> > > > +  upper-bound-mv:
> > > > +    description: The upper bound voltage value of the monitor.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > +
> > > > +  lower-bound-mv:
> > > > +    description: The lower bound voltage value of the monitor.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > +
> > > > +  scan-freq:
> > > > +    description: Number of the scan cycle interval.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > >
> > > Can you explain why any of these three properties are something that
> > > should be in the devicetree rather than software controlled?
> >
> > My intention is to be able to obtain the initial values from the devicetree during
> probe and preset them.
> > Do I need to drop them and just set them through sysfs?
> 
> Unless the hardware configuration determines the values (which I can't really see
> being the case for scan-freq at least) then yes, you need to drop and set them via
> sysfs.

The ADC hardware can be set the scan-freq register to determine how frequent it should scan its inputs.
The calculation is:
	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
The maximum allowable scan frequency is 5MHz. 

> 
> > > How are the bounds calculated?
> >
> > The measurement range of this ADC hardware is from 0 to 1800 mV. This set
> value cannot exceed it. This explanation will be added later.
> 
> I'm asking how this is calculated so that I can tell if you the property is permitted or
> not.

The calculation of bound is:
bound-mv = 1800mv * (register value) / 0xFFF

Best regards,
Xingyu Wu

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Jonathan Cameron]

On Tue, 19 May 2026 10:59:59 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > On 2026/5/19 00:24, Conor Dooley wrote:  
> > > 
> > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:  
> > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > >
> > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > ---
> > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > >  1 file changed, 62 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > > > l
> > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > > > l
> > > > new file mode 100644
> > > > index 000000000000..ba8e19b72ad7
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > > > +++ .yaml
> > > > @@ -0,0 +1,62 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > > +---
> > > > +$id:
> > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Successive Approximation Register (SAR) A/D converter for the
> > > > +StarFive JHB100 SoC
> > > > +
> > > > +maintainers:
> > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: starfive,jhb100-saradc
> > > > +
> > > > +  reg:
> > > > +    maxItem: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    maxItems: 1
> > > > +
> > > > +  resets:
> > > > +    maxItems: 2
> > > > +
> > > > +  "#io-channel-cells":
> > > > +    const: 1
> > > > +
> > > > +  upper-bound-mv:
> > > > +    description: The upper bound voltage value of the monitor.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > +
> > > > +  lower-bound-mv:
> > > > +    description: The lower bound voltage value of the monitor.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > +
> > > > +  scan-freq:
> > > > +    description: Number of the scan cycle interval.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint16  
> > > 
> > > Can you explain why any of these three properties are something that should be in
> > > the devicetree rather than software controlled?  
> > 
> > My intention is to be able to obtain the initial values from the devicetree during probe and preset them.
> > Do I need to drop them and just set them through sysfs?  
> 
> Unless the hardware configuration determines the values (which I can't
> really see being the case for scan-freq at least) then yes, you need to
> drop and set them via sysfs.
> 
> > > How are the bounds calculated?  
> > 
> > The measurement range of this ADC hardware is from 0 to 1800 mV. This set value cannot exceed it. This explanation will be added later.  
> 
> I'm asking how this is calculated so that I can tell if you the property
> is permitted or not. 

Far as I can tell this stuff is boot script / udev rule stuff - doesn't
belong in DT.

Jonathan

> 
> Cheers,
> Conor.

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

[Jonathan Cameron]

On Mon, 18 May 2026 16:18:50 +0800
Xingyu Wu <xingyu.wu@starfivetech.com> wrote:

> Hi all,
> 
> This series adds an IIO ADC driver for the controller of StarFive
> Successive Approximation Register A/D converter (SAR-ADC).
> 
> The StarFive SAR-ADC is a 12-bit converter with up to 8 input channels
> and a fixed 1.8V reference domain. The driver provides raw and processed
> voltage readouts via IIO, runtime PM support, and threshold-based
> voltage monitoring.
> 
> Tested on StarFive JHB100 EVB with all ADC channels and monitor
> interrupt path.
Take a look at the AI bot feedback.
https://sashiko.dev/#/patchset/20260518081852.116909-1-xingyu.wu%40starfivetech.com

It won't necessarily all be correct but the hit rate tends to be good.

Jonathan

> 
> Xingyu Wu (2):
>   bindings: iio: adc: Add StarFive JHB100 SARADC
>   iio: adc: Add StarFive SAR-ADC driver
> 
>  .../iio/adc/starfive,jhb100-saradc.yaml       |  62 ++
>  MAINTAINERS                                   |   6 +
>  drivers/iio/adc/Kconfig                       |  11 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/starfive-saradc.c             | 978 ++++++++++++++++++
>  5 files changed, 1058 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
>  create mode 100644 drivers/iio/adc/starfive-saradc.c
> 

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Krzysztof Kozlowski]

On 18/05/2026 10:18, Xingyu Wu wrote:
> Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> ---
>  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> new file mode 100644
> index 000000000000..ba8e19b72ad7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Successive Approximation Register (SAR) A/D converter for the StarFive JHB100 SoC
> +
> +maintainers:
> +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> +
> +properties:
> +  compatible:
> +    const: starfive,jhb100-saradc
> +
> +  reg:
> +    maxItem: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 2

Need to list items. See writing bindings.

> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  upper-bound-mv:
> +    description: The upper bound voltage value of the monitor.

Please read writing bindings about proper naming.


> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +  lower-bound-mv:
> +    description: The lower bound voltage value of the monitor.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +  scan-freq:
> +    description: Number of the scan cycle interval.
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - resets
> +  - "#io-channel-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    adc@11be1400 {
> +      compatible = "starfive,jhb100-saradc";
> +      reg = <0x11be1400 0x400>;
> +      interrupts = <172>;
> +      clocks = <&per0crg 18>;
> +      resets = <&per0crg 11>, <&per0crg 46>;
> +      #io-channel-cells = <1>;
> +      };

Messed indentation.


Best regards,
Krzysztof

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

[Jonathan Cameron]

On Mon, 18 May 2026 16:18:52 +0800
Xingyu Wu <xingyu.wu@starfivetech.com> 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.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
Hi

Welcome to IIO.
As Andy indicated there is quite a lot of work to do here.
As such I did a quick review, but a lot of the code will change anyway
as a result of ripping out the custom ABI and the dt-binding changes
requested.

Jonathan

> diff --git a/drivers/iio/adc/starfive-saradc.c b/drivers/iio/adc/starfive-saradc.c
> new file mode 100644
> index 000000000000..78409cb1bcb2
> --- /dev/null
> +++ b/drivers/iio/adc/starfive-saradc.c

> +
> +#define SARADC_CHAN(_index) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			BIT(IIO_CHAN_INFO_PROCESSED),		\
See below.

> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.datasheet_name = "SARADC_"#_index,			\
> +	.scan_index = _index,					\
Not used.
> +	.scan_type = {						\
> +		.sign = 'u',					\
> +		.realbits = SARADC_REALBITS,			\
> +		.endianness = IIO_CPU,				\
Not used - so don't specify it.
> +	},							\
> +}

> +
> +static const struct iio_chan_spec starfive_saradc_iio_channels[] = {
> +	SARADC_CHAN(0),
> +	SARADC_CHAN(1),
> +	SARADC_CHAN(2),
> +	SARADC_CHAN(3),
> +	SARADC_CHAN(4),
> +	SARADC_CHAN(5),
> +	SARADC_CHAN(6),
> +	SARADC_CHAN(7),

SARADC is a common name, so prefix macros etc with something related
to starfive.

> +};

> +
> +static void starfive_saradc_ch_lower_bound_set(struct starfive_saradc *priv,
> +					       int ch, u32 data)
> +{
> +	void __iomem *base = priv->base + SARADC_ULB_CHX_REG_GET(ch);
> +	u32 reg = readl(base) & ~ADC_LOWER_BOUND_MSK;
> +
> +	writel(FIELD_PREP(ADC_LOWER_BOUND_MSK, data) | reg, base);
These helpers don't really help much. I'd just put the readl/writel inline
where it's needed.

> +}

> +
> +static struct attribute *starfive_saradc_attributes[] = {
> +	&iio_dev_attr_scan_frequency.dev_attr.attr,
> +	&iio_dev_attr_in_voltage0_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage1_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage2_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage3_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage4_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage5_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage6_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage7_upper.dev_attr.attr,
> +	&iio_dev_attr_in_voltage0_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage1_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage2_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage3_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage4_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage5_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage6_lower.dev_attr.attr,
> +	&iio_dev_attr_in_voltage7_lower.dev_attr.attr,
> +	&iio_dev_attr_voltage_monitor_channel.dev_attr.attr,
> +	&iio_dev_attr_voltage_monitor_en.dev_attr.attr,

Look at IIO event support and use that. I'm not seeing anything
in here that should be provided as custom attributes.

Basic rule is that if you add any custom attributes your driver
will get a lot more push back. Key is that
custom attributes == no userspace support in standard code == not much used.


> +	NULL,
> +};
> +
> +static const struct attribute_group starfive_saradc_attr_group = {
> +	.attrs = starfive_saradc_attributes,
> +};
> +
> +static int starfive_saradc_read(struct starfive_saradc *priv)
> +{
> +	unsigned int tmp;
> +	int ret;
> +
> +	starfive_saradc_ch_dis_save(priv);
> +	if (!starfive_saradc_is_ready(priv)) {
> +		dev_err(priv->dev, "ADC do not ready, please try again later!\n");

Generally just return -EBUSY for this - no print because otherwise you provide
a userspace path to spam the kernel logs. If a print is really needed it should
be rate limited.

> +		starfive_saradc_ch_stop(priv);
> +		return -EBUSY;
> +	}
> +
> +	priv->err = false;
> +	starfive_saradc_ch_start(priv);
> +
> +	tmp = starfive_saradc_data_get(priv);
> +	/* Check that the data is ready to be read. */
> +	if (!(tmp & ADC_DAT_RDY_MSK)) {
> +		ret = readl_poll_timeout(priv->base + SARADC_DATAX_REG_GET(priv->using_ch), tmp,
> +					 (tmp & ADC_DAT_RDY_MSK), 10, SARADC_TIMEOUT);
> +		if (ret) {
> +			priv->err = true;
> +			dev_err(priv->dev, "channel%d is still not ready to be read! Timeout!\n",
> +				priv->using_ch);
> +		}
> +	}
> +
> +	if (priv->err)
> +		tmp = 0;
> +
> +	starfive_saradc_ch_stop(priv);
> +
> +	return (int)(tmp & ADC_DAT_MSK);

Prefer FIELD_GET() for all value extraction.

> +}
> +
> +static int starfive_saradc_read_raw(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan,
> +				    int *val, int *val2, long mask)
> +{
> +	struct starfive_saradc *priv = iio_priv(indio_dev);
> +	int ret;
> +	u64 tmp;
> +
> +	mutex_lock(&priv->lock);
guard() and acquire for PM (see below).
But try to do these in the same order in all functions. Makes reasoning
about what is protected simpler.  Normally I'd turn the power on first
as that is reference counted anyway so doesn't need to be under the lock.

> +	priv->using_ch = chan->channel;
> +	ret = pm_runtime_get_sync(priv->dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(priv->dev);
As below. the call that failed doesn't have side effects on failure so this
smells like an underflow.

> +		mutex_unlock(&priv->lock);
> +		return ret;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = starfive_saradc_read(priv);
> +		if (ret < 0)
> +			break;
> +
> +		*val = ret;
> +		ret = IIO_VAL_INT;
> +		break;
> +
> +	case IIO_CHAN_INFO_PROCESSED:

Don't provide both processed and raw for a channel.

If the transform is linear then _raw + _scale if not then _processed.

> +		ret = starfive_saradc_read(priv);
> +		if (ret < 0)
> +			break;
> +
> +		/* VIN = AVDD * data[11:0] / 4096. (AVDD = 1.8v) */
> +		tmp = SARADC_RAW_TO_VDD_ADJ(ret);
> +		*val = (int)(tmp / 1000000);
> +		*val2 = (int)(tmp % 1000000);
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * AVDD is fixed at 1.8v.
> +		 * 1.8 / (1 << 12) * 1000000
> +		 */
> +		*val = 0;
> +		*val2 = SARADC_AVDD_VOL / (1 << SARADC_REALBITS);

> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	pm_runtime_put_autosuspend(priv->dev);
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t starfive_saradc_mon_stop_threadfn(int irq, void *data)
> +{
> +	struct starfive_saradc *priv = data;
> +	u8 ch = priv->mon_ch;
> +	u32 up, low, raw;
> +
> +	mutex_lock(&priv->lock);
guard()

> +	if (!priv->mon_en) {

How would we get here?  If it's a race on shut down of events then normally
we wouldn't worry as that's just precise timing of disable vs this thread
running.

> +		mutex_unlock(&priv->lock);
> +		return IRQ_HANDLED;
> +	}
> +
> +	raw = readl(priv->base + SARADC_DATAX_REG_GET(ch)) & ADC_DAT_MSK;
> +	up = starfive_saradc_ch_upper_bound_get(priv, ch);
> +	low = starfive_saradc_ch_lower_bound_get(priv, ch);
> +	dev_err(priv->dev,
> +		"channel %d is out of bounds. sample data: %dmv (range: %dmv ~ %dmv)\n",
> +		ch, (SARADC_RAW_TO_VDD_ADJ(raw) / 1000),
> +		priv->low_bounds[ch], priv->up_bounds[ch]);

We have the IIO events infrastructure to report these to userspace. Use it
or drop this. dev_err is not a valid way to report this sort of threshold
events.

> +
> +	starfive_saradc_ch_monitor_stop(priv, ch);

Why stop it?  Add a comment.  We aren't interested in future events?

> +	pm_runtime_put_autosuspend(priv->dev);
> +	mutex_unlock(&priv->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t starfive_saradc_irq_handler(int irq, void *data)
> +{
> +	struct starfive_saradc *priv = data;
> +	u32 irq_err = readl(priv->base + SARADC_IRQ_EN_ST);
> +
> +	if (!priv->mon_working)

How do we hit this condition? If it's a race thing then add a comment
on what that race is. If it's not our interrupt then return IRQ_NONE.

> +		return IRQ_HANDLED;
> +
> +	/* Error of out of bounds */
> +	if (irq_err & BIT(priv->mon_ch)) {
> +		/* Clear the interrupt */
> +		writel(irq_err, priv->base + SARADC_IRQ_EN_ST);
> +		priv->err = true;
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void starfive_saradc_init(struct starfive_saradc *priv)
> +{
This will probably go away anyway but comments inline for future reference.

> +	bool use_def = false;
> +	u16 up, low, scan, tmp;
> +	u32 upmv, lowmv;
> +	int i;
> +
> +	if (of_property_read_u16(priv->dev->of_node, "upper-bound-mv", &tmp)) {

Use property.h accessors for new drivers, not the of_ones.
They are cleaner and maybe this driver will one day be used with other firmware
types.  Also I don't want of specific code in IIO drivers because it gets cut
and paste into others where another firmware type is actually likely.


> +		use_def = true;
> +	} else {
> +		up = SARADC_VDD_MV_TO_RAW(tmp);
> +		if (up > ADC_UPPER_BOUND_DEF)

If DT provide an invalid input fail to probe. We need that fixed.
If it was a missing property then current convention is to use a check on the property
existing as a separate call before reading it.  That provides neater error handling.


Mind you as per he discussion in that dt-binding thread this is going away.

> +			use_def = true;
> +		else
> +			upmv = tmp;
> +	}
> +
> +	if (use_def) {
> +		up = ADC_UPPER_BOUND_DEF;
> +		upmv = SARADC_RAW_TO_VDD_ADJ(up);
> +		use_def = false;
> +	}
> +
> +	if (of_property_read_u16(priv->dev->of_node, "lower-bound-mv", &tmp)) {
Same for all of these as commnets above. 
> +		use_def = true;
> +	} else {
> +		low = SARADC_VDD_MV_TO_RAW(tmp);
> +		if (low > ADC_UPPER_BOUND_DEF)
> +			use_def = true;
> +		else
> +			lowmv = tmp;
> +	}
> +
> +	if (use_def) {
> +		low = ADC_LOWER_BOUND_DEF;
> +		lowmv = SARADC_RAW_TO_VDD_ADJ(low);
> +	}
> +
> +	if (of_property_read_u16(priv->dev->of_node, "scan-freq", &scan))
> +		scan = ADC_SCAN_FREQ_DEF;
> +
> +	if ((scan & ADC_SCAN_FREQ_MSK) < ADC_SCAN_FREQ_MIN) {
> +		dev_warn(priv->dev, "The scan_freq is out of range and use the default value!\n");
> +		scan = ADC_SCAN_FREQ_DEF;
> +	}
> +
> +	starfive_saradc_scan_freq_set(priv, scan);
> +
> +	for (i = 0; i < SARADC_MAX_CHANNELS; i++) {
> +		starfive_saradc_ch_upper_bound_set(priv, i, up);
> +		starfive_saradc_ch_lower_bound_set(priv, i, low);
> +		priv->up_bounds[i] = upmv;
> +		priv->low_bounds[i] = lowmv;
> +	}
> +}
> +
> +static int starfive_saradc_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +				      unsigned int writeval, unsigned int *readval)
> +{
> +	struct starfive_saradc *priv = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (reg % 4 || reg > SARADC_SCAN_FREQ)
> +		return -EINVAL;
> +
> +	ret = pm_runtime_get_sync(priv->dev);
Look at  PM_RUNTIME_ACQUIRE_AUTOSUSPEND() 
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(priv->dev);

Why? It failed so we should not have a refcount to drop.

> +		return ret;
> +	}
> +
> +	mutex_lock(&priv->lock);
In combination with  the acquire mentioned above, use a guard() for the mutex.
> +
> +	if (readval)
> +		*readval = readl(priv->base + reg);
> +	else if (reg < SARADC_ULB_CH0)
> +		/* Allowed to write from SARADC_ULB_CH0 to SARADC_SCAN_FREQ */
> +		ret = -EINVAL;
With the changes above you can just return here.

> +	else
> +		writel(writeval, priv->base + reg);
> +
> +	mutex_unlock(&priv->lock);
> +	pm_runtime_put_sync_autosuspend(priv->dev);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info starfive_saradc_iio_info = {
> +	.read_raw = starfive_saradc_read_raw,
> +	.debugfs_reg_access = starfive_saradc_reg_access,
> +	.attrs = &starfive_saradc_attr_group,
> +};
> +
> +static int starfive_saradc_probe(struct platform_device *pdev)
> +{
> +	struct starfive_saradc *priv;
> +	struct iio_dev *indio_dev;
> +	int irq, ret;
Worth using a local struct device.
e.g.
	struct device *dev = &pdev->dev;
just to shorten the various lines where it's used.

> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> +	if (!indio_dev)
> +		return dev_err_probe(&pdev->dev, -ENOMEM, "failed allocating iio device\n");
This print is a noop for -ENOMEM so don't bother having it.
	if (!indio_dev)
		return -ENOMEM;


> +
> +	priv = iio_priv(indio_dev);
> +	platform_set_drvdata(pdev, indio_dev);
> +	priv->dev = &pdev->dev;
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq,
> +				     "failed to get irq\n");

One line. Note that going slightly over 80 chars is fine if it helps
readability. I think this one is just under 80 anyway.

> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq,
> +					starfive_saradc_irq_handler,
> +					starfive_saradc_mon_stop_threadfn,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,

Trigger direction should come from DT.  So don't set it here.
We do this historically in some drivers and can't fix it now because some
board might rely on it.  Don't introduce it in a new driver.

> +					dev_name(&pdev->dev), priv);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to request irq handler\n");
> +
> +	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk),
> +				     "failed to get clock\n");

As sashiko pointed out this might result in a clock underflow.  Fixing
that the mess of devm + runtime pm and working out general patterns is
still a work in progress. For now maybe just check if you get such a warning.


> +
> +	priv->rst = devm_reset_control_array_get_shared(&pdev->dev);
> +	if (IS_ERR(priv->rst))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rst),
> +				     "failed to get resets\n");
> +
> +	ret = reset_control_deassert(priv->rst);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to deassert reset\n");
> +
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &starfive_saradc_iio_info;
> +	indio_dev->channels = starfive_saradc_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(starfive_saradc_iio_channels);
> +
> +	starfive_saradc_init(priv);
> +	mutex_init(&priv->lock);
> +
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return devm_iio_device_register(&pdev->dev, indio_dev);
> +}
> +
> +static void starfive_saradc_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);

This is out of order wrt to the setup in probe.   I suspect you can use
devm_pm_runtime_enable() instead and drop remove entirely.

> +}
> +
> +static int starfive_saradc_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct starfive_saradc *priv = iio_priv(indio_dev);
> +
> +	starfive_saradc_pwr_on(priv, false);

Having a function called pwr_on() is missleading given you use it
to turn the power off. Rename it simply pwr() Or consider if it is
worth having the helper at all as it's very small and maybe having
the code inline is simpler.


> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int starfive_saradc_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct starfive_saradc *priv = iio_priv(indio_dev);
> +	int ret = clk_prepare_enable(priv->clk);
> +
> +	if (ret)
> +		return ret;
slight preference for

	int ret;

	ret = clk_prepare_enable(priv->clk);
	if (ret)
		return ret;

as it keeps the set of ret and it's check right next to each other.
> +
> +	starfive_saradc_pwr_on(priv, true);
> +	/* Need time to completely power on. */
> +	msleep(20);

fsleep() preferred. Takes time in usecs but the point is it will use
the generally right call for whatever the sleep is. That means readers
don't have to think about it. See the docs for the implementation of fsleep()
for more info on how it decides whether to call msleep or not.

If possible reference a datasheet section / table or similar for this sleep
length.  That is helpful if turns out to be too short on some device in
the future.

> +
> +	return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(starfive_saradc_pm_ops,
> +				 starfive_saradc_runtime_suspend,
> +				 starfive_saradc_runtime_resume, NULL);
> +
> +static const struct of_device_id starfive_saradc_of_match[] = {
> +	{ .compatible = "starfive,jhb100-saradc", },
> +	{ }
> +};
> +
> +static struct platform_driver starfive_saradc_driver = {
> +	.probe	= starfive_saradc_probe,
> +	.remove	= starfive_saradc_remove,
> +	.driver	= {
> +		.name = "starfive_saradc",
> +		.of_match_table = starfive_saradc_of_match,
> +		.pm = pm_ptr(&starfive_saradc_pm_ops)
> +	},
> +};

common practice to not have a blank line here. Dropping it makes
a clear association between the struct platform_driver and the macro
that uses it.

> +
> +module_platform_driver(starfive_saradc_driver);
> +
> +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
> +MODULE_DESCRIPTION("StarFive Successive Approximation Register ADC driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:
> On 2026/5/19 18:00, Conor Dooley wrote:
> > 
> > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > > On 2026/5/19 00:24, Conor Dooley wrote:
> > > >
> > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > > >
> > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > ---
> > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > > >  1 file changed, 62 insertions(+)
> > > > >  create mode 100644
> > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.y
> > > > > aml
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > > > > .yam
> > > > > l
> > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > > > > .yam
> > > > > l
> > > > > new file mode 100644
> > > > > index 000000000000..ba8e19b72ad7
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > +++ radc
> > > > > +++ .yaml
> > > > > @@ -0,0 +1,62 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > > > +---
> > > > > +$id:
> > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml
> > > > > +#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Successive Approximation Register (SAR) A/D converter for
> > > > > +the StarFive JHB100 SoC
> > > > > +
> > > > > +maintainers:
> > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: starfive,jhb100-saradc
> > > > > +
> > > > > +  reg:
> > > > > +    maxItem: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  clocks:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  resets:
> > > > > +    maxItems: 2
> > > > > +
> > > > > +  "#io-channel-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  upper-bound-mv:
> > > > > +    description: The upper bound voltage value of the monitor.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > +
> > > > > +  lower-bound-mv:
> > > > > +    description: The lower bound voltage value of the monitor.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > +
> > > > > +  scan-freq:
> > > > > +    description: Number of the scan cycle interval.
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > >
> > > > Can you explain why any of these three properties are something that
> > > > should be in the devicetree rather than software controlled?
> > >
> > > My intention is to be able to obtain the initial values from the devicetree during
> > probe and preset them.
> > > Do I need to drop them and just set them through sysfs?
> > 
> > Unless the hardware configuration determines the values (which I can't really see
> > being the case for scan-freq at least) then yes, you need to drop and set them via
> > sysfs.
> 
> The ADC hardware can be set the scan-freq register to determine how frequent it should scan its inputs.
> The calculation is:
> 	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
> The maximum allowable scan frequency is 5MHz. 
> 
> > 
> > > > How are the bounds calculated?
> > >
> > > The measurement range of this ADC hardware is from 0 to 1800 mV. This set
> > value cannot exceed it. This explanation will be added later.
> > 
> > I'm asking how this is calculated so that I can tell if you the property is permitted or
> > not.
> 
> The calculation of bound is:
> bound-mv = 1800mv * (register value) / 0xFFF

These are the formulas, but how does someone know what the value for
bound-mv needs to be? Why would someone not just want to always use
1800mv?

> 
> Best regards,
> Xingyu Wu
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Wed, May 20, 2026 at 12:11:09PM +0100, Jonathan Cameron wrote:
rom 0 to 1800 mV. This set value cannot exceed it. This explanation will be added later.  
> > 
> > I'm asking how this is calculated so that I can tell if you the property
> > is permitted or not. 
> 
> Far as I can tell this stuff is boot script / udev rule stuff - doesn't
> belong in DT.

I have suspected this to be the case, but I have been giving the
submitter the benefit of the doubt ;)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

On 2026/5/20 19:55, Krzysztof Kozlowski wrote:
> 
> On 18/05/2026 10:18, Xingyu Wu wrote:
> > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> >
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> 
> Please use subject prefixes matching the subsystem. You can get them for example
> with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is
> touching. For bindings, the preferred subjects are explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-
> patches.html#i-for-patch-submitters

Noted.

> 
> > ---
> >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > l
> > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc.yam
> > l
> > new file mode 100644
> > index 000000000000..ba8e19b72ad7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-saradc
> > +++ .yaml
> > @@ -0,0 +1,62 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Successive Approximation Register (SAR) A/D converter for the
> > +StarFive JHB100 SoC
> > +
> > +maintainers:
> > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: starfive,jhb100-saradc
> > +
> > +  reg:
> > +    maxItem: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 2
> 
> Need to list items. See writing bindings.

Will fix.

> 
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +  upper-bound-mv:
> > +    description: The upper bound voltage value of the monitor.
> 
> Please read writing bindings about proper naming.

Noted.

> 
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +  lower-bound-mv:
> > +    description: The lower bound voltage value of the monitor.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +  scan-freq:
> > +    description: Number of the scan cycle interval.
> > +    $ref: /schemas/types.yaml#/definitions/uint16
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - resets
> > +  - "#io-channel-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    adc@11be1400 {
> > +      compatible = "starfive,jhb100-saradc";
> > +      reg = <0x11be1400 0x400>;
> > +      interrupts = <172>;
> > +      clocks = <&per0crg 18>;
> > +      resets = <&per0crg 11>, <&per0crg 46>;
> > +      #io-channel-cells = <1>;
> > +      };
> 
> Messed indentation.

Will fix.

Best regards,
Xingyu Wu

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

[Xingyu Wu]

On 2026/5/20 20:06, Jonathan Cameron wrote:
> 
> On Mon, 18 May 2026 16:18:52 +0800
> Xingyu Wu <xingyu.wu@starfivetech.com> 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.
> >
> > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> Hi
> 
> Welcome to IIO.
> As Andy indicated there is quite a lot of work to do here.
> As such I did a quick review, but a lot of the code will change anyway as a result of
> ripping out the custom ABI and the dt-binding changes requested.
> 
> Jonathan
> 
> > diff --git a/drivers/iio/adc/starfive-saradc.c
> > b/drivers/iio/adc/starfive-saradc.c
> > new file mode 100644
> > index 000000000000..78409cb1bcb2
> > --- /dev/null
> > +++ b/drivers/iio/adc/starfive-saradc.c
> 
> > +
> > +#define SARADC_CHAN(_index) {					\
> > +	.type = IIO_VOLTAGE,					\
> > +	.indexed = 1,						\
> > +	.channel = _index,					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> > +			BIT(IIO_CHAN_INFO_PROCESSED),		\
> See below.
> 
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
> > +	.datasheet_name = "SARADC_"#_index,			\
> > +	.scan_index = _index,					\
> Not used.

Noted.

> > +	.scan_type = {						\
> > +		.sign = 'u',					\
> > +		.realbits = SARADC_REALBITS,			\
> > +		.endianness = IIO_CPU,				\
> Not used - so don't specify it.

Will drop it.

> > +	},							\
> > +}
> 
> > +
> > +static const struct iio_chan_spec starfive_saradc_iio_channels[] = {
> > +	SARADC_CHAN(0),
> > +	SARADC_CHAN(1),
> > +	SARADC_CHAN(2),
> > +	SARADC_CHAN(3),
> > +	SARADC_CHAN(4),
> > +	SARADC_CHAN(5),
> > +	SARADC_CHAN(6),
> > +	SARADC_CHAN(7),
> 
> SARADC is a common name, so prefix macros etc with something related to
> starfive.

Noted.

> 
> > +};
> 
> > +
> > +static void starfive_saradc_ch_lower_bound_set(struct starfive_saradc *priv,
> > +					       int ch, u32 data)
> > +{
> > +	void __iomem *base = priv->base + SARADC_ULB_CHX_REG_GET(ch);
> > +	u32 reg = readl(base) & ~ADC_LOWER_BOUND_MSK;
> > +
> > +	writel(FIELD_PREP(ADC_LOWER_BOUND_MSK, data) | reg, base);
> These helpers don't really help much. I'd just put the readl/writel inline where it's
> needed.

Noted.

> 
> > +}
> 
> > +
> > +static struct attribute *starfive_saradc_attributes[] = {
> > +	&iio_dev_attr_scan_frequency.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage0_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage1_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage2_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage3_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage4_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage5_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage6_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage7_upper.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage0_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage1_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage2_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage3_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage4_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage5_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage6_lower.dev_attr.attr,
> > +	&iio_dev_attr_in_voltage7_lower.dev_attr.attr,
> > +	&iio_dev_attr_voltage_monitor_channel.dev_attr.attr,
> > +	&iio_dev_attr_voltage_monitor_en.dev_attr.attr,
> 
> Look at IIO event support and use that. I'm not seeing anything in here that should
> be provided as custom attributes.
> 
> Basic rule is that if you add any custom attributes your driver will get a lot more
> push back. Key is that custom attributes == no userspace support in standard code
> == not much used.

Well, I will use IIO event instead.

> 
> 
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group starfive_saradc_attr_group = {
> > +	.attrs = starfive_saradc_attributes, };
> > +
> > +static int starfive_saradc_read(struct starfive_saradc *priv) {
> > +	unsigned int tmp;
> > +	int ret;
> > +
> > +	starfive_saradc_ch_dis_save(priv);
> > +	if (!starfive_saradc_is_ready(priv)) {
> > +		dev_err(priv->dev, "ADC do not ready, please try again later!\n");
> 
> Generally just return -EBUSY for this - no print because otherwise you provide a
> userspace path to spam the kernel logs. If a print is really needed it should be rate
> limited.

I will drop the print.

> 
> > +		starfive_saradc_ch_stop(priv);
> > +		return -EBUSY;
> > +	}
> > +
> > +	priv->err = false;
> > +	starfive_saradc_ch_start(priv);
> > +
> > +	tmp = starfive_saradc_data_get(priv);
> > +	/* Check that the data is ready to be read. */
> > +	if (!(tmp & ADC_DAT_RDY_MSK)) {
> > +		ret = readl_poll_timeout(priv->base +
> SARADC_DATAX_REG_GET(priv->using_ch), tmp,
> > +					 (tmp & ADC_DAT_RDY_MSK), 10,
> SARADC_TIMEOUT);
> > +		if (ret) {
> > +			priv->err = true;
> > +			dev_err(priv->dev, "channel%d is still not ready to be read!
> Timeout!\n",
> > +				priv->using_ch);
> > +		}
> > +	}
> > +
> > +	if (priv->err)
> > +		tmp = 0;
> > +
> > +	starfive_saradc_ch_stop(priv);
> > +
> > +	return (int)(tmp & ADC_DAT_MSK);
> 
> Prefer FIELD_GET() for all value extraction.

Noted.

> 
> > +}
> > +
> > +static int starfive_saradc_read_raw(struct iio_dev *indio_dev,
> > +				    struct iio_chan_spec const *chan,
> > +				    int *val, int *val2, long mask) {
> > +	struct starfive_saradc *priv = iio_priv(indio_dev);
> > +	int ret;
> > +	u64 tmp;
> > +
> > +	mutex_lock(&priv->lock);
> guard() and acquire for PM (see below).
> But try to do these in the same order in all functions. Makes reasoning about what
> is protected simpler.  Normally I'd turn the power on first as that is reference
> counted anyway so doesn't need to be under the lock.

Noted.

> 
> > +	priv->using_ch = chan->channel;
> > +	ret = pm_runtime_get_sync(priv->dev);
> > +	if (ret < 0) {
> > +		pm_runtime_put_noidle(priv->dev);
> As below. the call that failed doesn't have side effects on failure so this smells like
> an underflow.
> 
> > +		mutex_unlock(&priv->lock);
> > +		return ret;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = starfive_saradc_read(priv);
> > +		if (ret < 0)
> > +			break;
> > +
> > +		*val = ret;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +
> > +	case IIO_CHAN_INFO_PROCESSED:
> 
> Don't provide both processed and raw for a channel.
> 
> If the transform is linear then _raw + _scale if not then _processed.

Drop it.

> 
> > +		ret = starfive_saradc_read(priv);
> > +		if (ret < 0)
> > +			break;
> > +
> > +		/* VIN = AVDD * data[11:0] / 4096. (AVDD = 1.8v) */
> > +		tmp = SARADC_RAW_TO_VDD_ADJ(ret);
> > +		*val = (int)(tmp / 1000000);
> > +		*val2 = (int)(tmp % 1000000);
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> > +		break;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		/*
> > +		 * AVDD is fixed at 1.8v.
> > +		 * 1.8 / (1 << 12) * 1000000
> > +		 */
> > +		*val = 0;
> > +		*val2 = SARADC_AVDD_VOL / (1 << SARADC_REALBITS);
> 
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> > +		break;
> > +
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	pm_runtime_put_autosuspend(priv->dev);
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t starfive_saradc_mon_stop_threadfn(int irq, void
> > +*data) {
> > +	struct starfive_saradc *priv = data;
> > +	u8 ch = priv->mon_ch;
> > +	u32 up, low, raw;
> > +
> > +	mutex_lock(&priv->lock);
> guard()
> 
> > +	if (!priv->mon_en) {
> 
> How would we get here?  If it's a race on shut down of events then normally we
> wouldn't worry as that's just precise timing of disable vs this thread running.

Drop the check.

> 
> > +		mutex_unlock(&priv->lock);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	raw = readl(priv->base + SARADC_DATAX_REG_GET(ch)) & ADC_DAT_MSK;
> > +	up = starfive_saradc_ch_upper_bound_get(priv, ch);
> > +	low = starfive_saradc_ch_lower_bound_get(priv, ch);
> > +	dev_err(priv->dev,
> > +		"channel %d is out of bounds. sample data: %dmv (range: %dmv
> ~ %dmv)\n",
> > +		ch, (SARADC_RAW_TO_VDD_ADJ(raw) / 1000),
> > +		priv->low_bounds[ch], priv->up_bounds[ch]);
> 
> We have the IIO events infrastructure to report these to userspace. Use it or drop
> this. dev_err is not a valid way to report this sort of threshold events.

Drop it.

> 
> > +
> > +	starfive_saradc_ch_monitor_stop(priv, ch);
> 
> Why stop it?  Add a comment.  We aren't interested in future events?

If we do not stop it and the input voltage remains constant, the ADC could continuously monitor the voltage and continuously trigger interrupts for the bound.
As a result, the CPU utilization rate will be high.

> 
> > +	pm_runtime_put_autosuspend(priv->dev);
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t starfive_saradc_irq_handler(int irq, void *data) {
> > +	struct starfive_saradc *priv = data;
> > +	u32 irq_err = readl(priv->base + SARADC_IRQ_EN_ST);
> > +
> > +	if (!priv->mon_working)
> 
> How do we hit this condition? If it's a race thing then add a comment on what that
> race is. If it's not our interrupt then return IRQ_NONE.

Drop it.

> 
> > +		return IRQ_HANDLED;
> > +
> > +	/* Error of out of bounds */
> > +	if (irq_err & BIT(priv->mon_ch)) {
> > +		/* Clear the interrupt */
> > +		writel(irq_err, priv->base + SARADC_IRQ_EN_ST);
> > +		priv->err = true;
> > +		return IRQ_WAKE_THREAD;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void starfive_saradc_init(struct starfive_saradc *priv) {
> This will probably go away anyway but comments inline for future reference.

Noted.

> 
> > +	bool use_def = false;
> > +	u16 up, low, scan, tmp;
> > +	u32 upmv, lowmv;
> > +	int i;
> > +
> > +	if (of_property_read_u16(priv->dev->of_node, "upper-bound-mv",
> > +&tmp)) {
> 
> Use property.h accessors for new drivers, not the of_ones.
> They are cleaner and maybe this driver will one day be used with other firmware
> types.  Also I don't want of specific code in IIO drivers because it gets cut and paste
> into others where another firmware type is actually likely.

Noted.

> 
> 
> > +		use_def = true;
> > +	} else {
> > +		up = SARADC_VDD_MV_TO_RAW(tmp);
> > +		if (up > ADC_UPPER_BOUND_DEF)
> 
> If DT provide an invalid input fail to probe. We need that fixed.
> If it was a missing property then current convention is to use a check on the
> property existing as a separate call before reading it.  That provides neater error
> handling.
> 
> 
> Mind you as per he discussion in that dt-binding thread this is going away.
> 
> > +			use_def = true;
> > +		else
> > +			upmv = tmp;
> > +	}
> > +
> > +	if (use_def) {
> > +		up = ADC_UPPER_BOUND_DEF;
> > +		upmv = SARADC_RAW_TO_VDD_ADJ(up);
> > +		use_def = false;
> > +	}
> > +
> > +	if (of_property_read_u16(priv->dev->of_node, "lower-bound-mv",
> > +&tmp)) {
> Same for all of these as commnets above.
> > +		use_def = true;
> > +	} else {
> > +		low = SARADC_VDD_MV_TO_RAW(tmp);
> > +		if (low > ADC_UPPER_BOUND_DEF)
> > +			use_def = true;
> > +		else
> > +			lowmv = tmp;
> > +	}
> > +
> > +	if (use_def) {
> > +		low = ADC_LOWER_BOUND_DEF;
> > +		lowmv = SARADC_RAW_TO_VDD_ADJ(low);
> > +	}
> > +
> > +	if (of_property_read_u16(priv->dev->of_node, "scan-freq", &scan))
> > +		scan = ADC_SCAN_FREQ_DEF;
> > +
> > +	if ((scan & ADC_SCAN_FREQ_MSK) < ADC_SCAN_FREQ_MIN) {
> > +		dev_warn(priv->dev, "The scan_freq is out of range and use the
> default value!\n");
> > +		scan = ADC_SCAN_FREQ_DEF;
> > +	}
> > +
> > +	starfive_saradc_scan_freq_set(priv, scan);
> > +
> > +	for (i = 0; i < SARADC_MAX_CHANNELS; i++) {
> > +		starfive_saradc_ch_upper_bound_set(priv, i, up);
> > +		starfive_saradc_ch_lower_bound_set(priv, i, low);
> > +		priv->up_bounds[i] = upmv;
> > +		priv->low_bounds[i] = lowmv;
> > +	}
> > +}
> > +
> > +static int starfive_saradc_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> > +				      unsigned int writeval, unsigned int *readval) {
> > +	struct starfive_saradc *priv = iio_priv(indio_dev);
> > +	int ret = 0;
> > +
> > +	if (reg % 4 || reg > SARADC_SCAN_FREQ)
> > +		return -EINVAL;
> > +
> > +	ret = pm_runtime_get_sync(priv->dev);
> Look at  PM_RUNTIME_ACQUIRE_AUTOSUSPEND()
> > +	if (ret < 0) {
> > +		pm_runtime_put_noidle(priv->dev);
> 
> Why? It failed so we should not have a refcount to drop.

Will fix.

> 
> > +		return ret;
> > +	}
> > +
> > +	mutex_lock(&priv->lock);
> In combination with  the acquire mentioned above, use a guard() for the mutex.

Noted
> > +
> > +	if (readval)
> > +		*readval = readl(priv->base + reg);
> > +	else if (reg < SARADC_ULB_CH0)
> > +		/* Allowed to write from SARADC_ULB_CH0 to
> SARADC_SCAN_FREQ */
> > +		ret = -EINVAL;
> With the changes above you can just return here.
> 
> > +	else
> > +		writel(writeval, priv->base + reg);
> > +
> > +	mutex_unlock(&priv->lock);
> > +	pm_runtime_put_sync_autosuspend(priv->dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_info starfive_saradc_iio_info = {
> > +	.read_raw = starfive_saradc_read_raw,
> > +	.debugfs_reg_access = starfive_saradc_reg_access,
> > +	.attrs = &starfive_saradc_attr_group, };
> > +
> > +static int starfive_saradc_probe(struct platform_device *pdev) {
> > +	struct starfive_saradc *priv;
> > +	struct iio_dev *indio_dev;
> > +	int irq, ret;
> Worth using a local struct device.
> e.g.
> 	struct device *dev = &pdev->dev;
> just to shorten the various lines where it's used.

Noted.

> 
> > +
> > +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> > +	if (!indio_dev)
> > +		return dev_err_probe(&pdev->dev, -ENOMEM, "failed allocating
> iio
> > +device\n");
> This print is a noop for -ENOMEM so don't bother having it.
> 	if (!indio_dev)
> 		return -ENOMEM;

Noted.
> 
> 
> > +
> > +	priv = iio_priv(indio_dev);
> > +	platform_set_drvdata(pdev, indio_dev);
> > +	priv->dev = &pdev->dev;
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return dev_err_probe(&pdev->dev, irq,
> > +				     "failed to get irq\n");
> 
> One line. Note that going slightly over 80 chars is fine if it helps readability. I think
> this one is just under 80 anyway.

Noted.

> 
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq,
> > +					starfive_saradc_irq_handler,
> > +					starfive_saradc_mon_stop_threadfn,
> > +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> 
> Trigger direction should come from DT.  So don't set it here.
> We do this historically in some drivers and can't fix it now because some board
> might rely on it.  Don't introduce it in a new driver.

OK, I will remove it.

> 
> > +					dev_name(&pdev->dev), priv);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to request irq handler\n");
> > +
> > +	priv->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk),
> > +				     "failed to get clock\n");
> 
> As sashiko pointed out this might result in a clock underflow.  Fixing that the mess
> of devm + runtime pm and working out general patterns is still a work in progress.
> For now maybe just check if you get such a warning.

Noted.

> 
> 
> > +
> > +	priv->rst = devm_reset_control_array_get_shared(&pdev->dev);
> > +	if (IS_ERR(priv->rst))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->rst),
> > +				     "failed to get resets\n");
> > +
> > +	ret = reset_control_deassert(priv->rst);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to deassert reset\n");
> > +
> > +	indio_dev->name = dev_name(&pdev->dev);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &starfive_saradc_iio_info;
> > +	indio_dev->channels = starfive_saradc_iio_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(starfive_saradc_iio_channels);
> > +
> > +	starfive_saradc_init(priv);
> > +	mutex_init(&priv->lock);
> > +
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	return devm_iio_device_register(&pdev->dev, indio_dev); }
> > +
> > +static void starfive_saradc_remove(struct platform_device *pdev) {
> > +	pm_runtime_disable(&pdev->dev);
> > +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> 
> This is out of order wrt to the setup in probe.   I suspect you can use
> devm_pm_runtime_enable() instead and drop remove entirely.

Noted.

> 
> > +}
> > +
> > +static int starfive_saradc_runtime_suspend(struct device *dev) {
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct starfive_saradc *priv = iio_priv(indio_dev);
> > +
> > +	starfive_saradc_pwr_on(priv, false);
> 
> Having a function called pwr_on() is missleading given you use it to turn the power
> off. Rename it simply pwr() Or consider if it is worth having the helper at all as it's
> very small and maybe having the code inline is simpler.

Got it.

> 
> 
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int starfive_saradc_runtime_resume(struct device *dev) {
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct starfive_saradc *priv = iio_priv(indio_dev);
> > +	int ret = clk_prepare_enable(priv->clk);
> > +
> > +	if (ret)
> > +		return ret;
> slight preference for
> 
> 	int ret;
> 
> 	ret = clk_prepare_enable(priv->clk);
> 	if (ret)
> 		return ret;
> 
> as it keeps the set of ret and it's check right next to each other.

Noted.

> > +
> > +	starfive_saradc_pwr_on(priv, true);
> > +	/* Need time to completely power on. */
> > +	msleep(20);
> 
> fsleep() preferred. Takes time in usecs but the point is it will use the generally right
> call for whatever the sleep is. That means readers don't have to think about it. See
> the docs for the implementation of fsleep() for more info on how it decides
> whether to call msleep or not.
> 
> If possible reference a datasheet section / table or similar for this sleep length.
> That is helpful if turns out to be too short on some device in the future.

Got it.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(starfive_saradc_pm_ops,
> > +				 starfive_saradc_runtime_suspend,
> > +				 starfive_saradc_runtime_resume, NULL);
> > +
> > +static const struct of_device_id starfive_saradc_of_match[] = {
> > +	{ .compatible = "starfive,jhb100-saradc", },
> > +	{ }
> > +};
> > +
> > +static struct platform_driver starfive_saradc_driver = {
> > +	.probe	= starfive_saradc_probe,
> > +	.remove	= starfive_saradc_remove,
> > +	.driver	= {
> > +		.name = "starfive_saradc",
> > +		.of_match_table = starfive_saradc_of_match,
> > +		.pm = pm_ptr(&starfive_saradc_pm_ops)
> > +	},
> > +};
> 
> common practice to not have a blank line here. Dropping it makes a clear
> association between the struct platform_driver and the macro that uses it.

Noted.

> 
> > +
> > +module_platform_driver(starfive_saradc_driver);
> > +
> > +MODULE_AUTHOR("Xingyu Wu <xingyu.wu@starfivetech.com>");
> > +MODULE_DESCRIPTION("StarFive Successive Approximation Register ADC
> > +driver"); MODULE_LICENSE("GPL");

Thanks.
Best regards,
Xingyu Wu

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

On 2026/5/20 23:15, Conor Dooley wrote:
> 
> On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:
> > On 2026/5/19 18:00, Conor Dooley wrote:
> > >
> > > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > > > On 2026/5/19 00:24, Conor Dooley wrote:
> > > > >
> > > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > > > >
> > > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > ---
> > > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > > > >  1 file changed, 62 insertions(+)  create mode 100644
> > > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sara
> > > > > > dc.y
> > > > > > aml
> > > > > >
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > radc
> > > > > > .yam
> > > > > > l
> > > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > radc
> > > > > > .yam
> > > > > > l
> > > > > > new file mode 100644
> > > > > > index 000000000000..ba8e19b72ad7
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb10
> > > > > > +++ 0-sa
> > > > > > +++ radc
> > > > > > +++ .yaml
> > > > > > @@ -0,0 +1,62 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML
> > > > > > +1.2
> > > > > > +---
> > > > > > +$id:
> > > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.
> > > > > > +yaml
> > > > > > +#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Successive Approximation Register (SAR) A/D converter
> > > > > > +for the StarFive JHB100 SoC
> > > > > > +
> > > > > > +maintainers:
> > > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: starfive,jhb100-saradc
> > > > > > +
> > > > > > +  reg:
> > > > > > +    maxItem: 1
> > > > > > +
> > > > > > +  interrupts:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  clocks:
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  resets:
> > > > > > +    maxItems: 2
> > > > > > +
> > > > > > +  "#io-channel-cells":
> > > > > > +    const: 1
> > > > > > +
> > > > > > +  upper-bound-mv:
> > > > > > +    description: The upper bound voltage value of the monitor.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > +
> > > > > > +  lower-bound-mv:
> > > > > > +    description: The lower bound voltage value of the monitor.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > +
> > > > > > +  scan-freq:
> > > > > > +    description: Number of the scan cycle interval.
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > >
> > > > > Can you explain why any of these three properties are something
> > > > > that should be in the devicetree rather than software controlled?
> > > >
> > > > My intention is to be able to obtain the initial values from the
> > > > devicetree during
> > > probe and preset them.
> > > > Do I need to drop them and just set them through sysfs?
> > >
> > > Unless the hardware configuration determines the values (which I
> > > can't really see being the case for scan-freq at least) then yes,
> > > you need to drop and set them via sysfs.
> >
> > The ADC hardware can be set the scan-freq register to determine how frequent it
> should scan its inputs.
> > The calculation is:
> > 	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
> > The maximum allowable scan frequency is 5MHz.
> >
> > >
> > > > > How are the bounds calculated?
> > > >
> > > > The measurement range of this ADC hardware is from 0 to 1800 mV.
> > > > This set
> > > value cannot exceed it. This explanation will be added later.
> > >
> > > I'm asking how this is calculated so that I can tell if you the
> > > property is permitted or not.
> >
> > The calculation of bound is:
> > bound-mv = 1800mv * (register value) / 0xFFF
> 
> These are the formulas, but how does someone know what the value for bound-
> mv needs to be? Why would someone not just want to always use 1800mv?
> 

Can I add the 'maximum' and ' minimum' to provide clarification? And the driver will also check.

Best regards,
Xingyu Wu

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4946 bytes --]

On Thu, May 21, 2026 at 09:54:27AM +0000, Xingyu Wu wrote:
> On 2026/5/20 23:15, Conor Dooley wrote:
> > 
> > On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:
> > > On 2026/5/19 18:00, Conor Dooley wrote:
> > > >
> > > > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > > > > On 2026/5/19 00:24, Conor Dooley wrote:
> > > > > >
> > > > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > > > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > > > > >
> > > > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > ---
> > > > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > > > > >  1 file changed, 62 insertions(+)  create mode 100644
> > > > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sara
> > > > > > > dc.y
> > > > > > > aml
> > > > > > >
> > > > > > > diff --git
> > > > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > > radc
> > > > > > > .yam
> > > > > > > l
> > > > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > > radc
> > > > > > > .yam
> > > > > > > l
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..ba8e19b72ad7
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb10
> > > > > > > +++ 0-sa
> > > > > > > +++ radc
> > > > > > > +++ .yaml
> > > > > > > @@ -0,0 +1,62 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML
> > > > > > > +1.2
> > > > > > > +---
> > > > > > > +$id:
> > > > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.
> > > > > > > +yaml
> > > > > > > +#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Successive Approximation Register (SAR) A/D converter
> > > > > > > +for the StarFive JHB100 SoC
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    const: starfive,jhb100-saradc
> > > > > > > +
> > > > > > > +  reg:
> > > > > > > +    maxItem: 1
> > > > > > > +
> > > > > > > +  interrupts:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  clocks:
> > > > > > > +    maxItems: 1
> > > > > > > +
> > > > > > > +  resets:
> > > > > > > +    maxItems: 2
> > > > > > > +
> > > > > > > +  "#io-channel-cells":
> > > > > > > +    const: 1
> > > > > > > +
> > > > > > > +  upper-bound-mv:
> > > > > > > +    description: The upper bound voltage value of the monitor.
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > +
> > > > > > > +  lower-bound-mv:
> > > > > > > +    description: The lower bound voltage value of the monitor.
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > +
> > > > > > > +  scan-freq:
> > > > > > > +    description: Number of the scan cycle interval.
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > >
> > > > > > Can you explain why any of these three properties are something
> > > > > > that should be in the devicetree rather than software controlled?
> > > > >
> > > > > My intention is to be able to obtain the initial values from the
> > > > > devicetree during
> > > > probe and preset them.
> > > > > Do I need to drop them and just set them through sysfs?
> > > >
> > > > Unless the hardware configuration determines the values (which I
> > > > can't really see being the case for scan-freq at least) then yes,
> > > > you need to drop and set them via sysfs.
> > >
> > > The ADC hardware can be set the scan-freq register to determine how frequent it
> > should scan its inputs.
> > > The calculation is:
> > > 	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
> > > The maximum allowable scan frequency is 5MHz.
> > >
> > > >
> > > > > > How are the bounds calculated?
> > > > >
> > > > > The measurement range of this ADC hardware is from 0 to 1800 mV.
> > > > > This set
> > > > value cannot exceed it. This explanation will be added later.
> > > >
> > > > I'm asking how this is calculated so that I can tell if you the
> > > > property is permitted or not.
> > >
> > > The calculation of bound is:
> > > bound-mv = 1800mv * (register value) / 0xFFF
> > 
> > These are the formulas, but how does someone know what the value for bound-
> > mv needs to be? Why would someone not just want to always use 1800mv?
> > 
> 
> Can I add the 'maximum' and ' minimum' to provide clarification? And the driver will also check.

All that does is repeat the 1800 mV though, what I am interested in is
how someone determines if they should use 1600 mV or 200 mV etc. What
aspect of the hardware do the bounds depend on?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: 2026年5月21日 18:21
> To: Xingyu Wu <xingyu.wu@starfivetech.com>
> Cc: Jonathan Cameron <jic23@kernel.org>; David Lechner
> <dlechner@baylibre.com>; Nuno Sá <nuno.sa@analog.com>; Andy Shevchenko
> <andy@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> iio@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC
> 
> On Thu, May 21, 2026 at 09:54:27AM +0000, Xingyu Wu wrote:
> > On 2026/5/20 23:15, Conor Dooley wrote:
> > >
> > > On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:
> > > > On 2026/5/19 18:00, Conor Dooley wrote:
> > > > >
> > > > > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > > > > > On 2026/5/19 00:24, Conor Dooley wrote:
> > > > > > >
> > > > > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > > > > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > > > > > >
> > > > > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > ---
> > > > > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > > > > > >  1 file changed, 62 insertions(+)  create mode 100644
> > > > > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-
> > > > > > > > sara
> > > > > > > > dc.y
> > > > > > > > aml
> > > > > > > >
> > > > > > > > diff --git
> > > > > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb10
> > > > > > > > 0-sa
> > > > > > > > radc
> > > > > > > > .yam
> > > > > > > > l
> > > > > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb10
> > > > > > > > 0-sa
> > > > > > > > radc
> > > > > > > > .yam
> > > > > > > > l
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..ba8e19b72ad7
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,j
> > > > > > > > +++ hb10
> > > > > > > > +++ 0-sa
> > > > > > > > +++ radc
> > > > > > > > +++ .yaml
> > > > > > > > @@ -0,0 +1,62 @@
> > > > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > > > > +%YAML
> > > > > > > > +1.2
> > > > > > > > +---
> > > > > > > > +$id:
> > > > > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.
> > > > > > > > +yaml
> > > > > > > > +#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: Successive Approximation Register (SAR) A/D
> > > > > > > > +converter for the StarFive JHB100 SoC
> > > > > > > > +
> > > > > > > > +maintainers:
> > > > > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > +
> > > > > > > > +properties:
> > > > > > > > +  compatible:
> > > > > > > > +    const: starfive,jhb100-saradc
> > > > > > > > +
> > > > > > > > +  reg:
> > > > > > > > +    maxItem: 1
> > > > > > > > +
> > > > > > > > +  interrupts:
> > > > > > > > +    maxItems: 1
> > > > > > > > +
> > > > > > > > +  clocks:
> > > > > > > > +    maxItems: 1
> > > > > > > > +
> > > > > > > > +  resets:
> > > > > > > > +    maxItems: 2
> > > > > > > > +
> > > > > > > > +  "#io-channel-cells":
> > > > > > > > +    const: 1
> > > > > > > > +
> > > > > > > > +  upper-bound-mv:
> > > > > > > > +    description: The upper bound voltage value of the monitor.
> > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > +
> > > > > > > > +  lower-bound-mv:
> > > > > > > > +    description: The lower bound voltage value of the monitor.
> > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > +
> > > > > > > > +  scan-freq:
> > > > > > > > +    description: Number of the scan cycle interval.
> > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > >
> > > > > > > Can you explain why any of these three properties are
> > > > > > > something that should be in the devicetree rather than software
> controlled?
> > > > > >
> > > > > > My intention is to be able to obtain the initial values from
> > > > > > the devicetree during
> > > > > probe and preset them.
> > > > > > Do I need to drop them and just set them through sysfs?
> > > > >
> > > > > Unless the hardware configuration determines the values (which I
> > > > > can't really see being the case for scan-freq at least) then
> > > > > yes, you need to drop and set them via sysfs.
> > > >
> > > > The ADC hardware can be set the scan-freq register to determine
> > > > how frequent it
> > > should scan its inputs.
> > > > The calculation is:
> > > > 	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
> > > > The maximum allowable scan frequency is 5MHz.
> > > >
> > > > >
> > > > > > > How are the bounds calculated?
> > > > > >
> > > > > > The measurement range of this ADC hardware is from 0 to 1800 mV.
> > > > > > This set
> > > > > value cannot exceed it. This explanation will be added later.
> > > > >
> > > > > I'm asking how this is calculated so that I can tell if you the
> > > > > property is permitted or not.
> > > >
> > > > The calculation of bound is:
> > > > bound-mv = 1800mv * (register value) / 0xFFF
> > >
> > > These are the formulas, but how does someone know what the value for
> > > bound- mv needs to be? Why would someone not just want to always use
> 1800mv?
> > >
> >
> > Can I add the 'maximum' and ' minimum' to provide clarification? And the driver
> will also check.
> 
> All that does is repeat the 1800 mV though, what I am interested in is how
> someone determines if they should use 1600 mV or 200 mV etc. What aspect of
> the hardware do the bounds depend on?

These bounds are just for the monitor mode. If the input voltage is outside this range, interrupt will be triggered to report the user space.
The bounds value (1600mv or 200mv) are determined by the users' applications. Users can freely set them according to the range they want to monitor.

Best regards,
Xingyu Wu

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

[Jonathan Cameron]

> 
> >   
> > > +
> > > +	starfive_saradc_ch_monitor_stop(priv, ch);  
> > 
> > Why stop it?  Add a comment.  We aren't interested in future events?  
> 
> If we do not stop it and the input voltage remains constant, the ADC could continuously monitor the voltage and continuously trigger interrupts for the bound.
> As a result, the CPU utilization rate will be high.

Can you use an edge interrupt instead of a level one?

If not, then a common solution is to disable for a period (maybe a second
or so) then reenable. There are various more refined ways of doing this.

Basically we don't want to be in a situation where a momentary blip
disables the event and we miss a later condition that must be handled.

Thanks,

Jonathan

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Jonathan Cameron]

On Thu, 21 May 2026 11:20:52 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Thu, May 21, 2026 at 09:54:27AM +0000, Xingyu Wu wrote:
> > On 2026/5/20 23:15, Conor Dooley wrote:  
> > > 
> > > On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:  
> > > > On 2026/5/19 18:00, Conor Dooley wrote:  
> > > > >
> > > > > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:  
> > > > > > On 2026/5/19 00:24, Conor Dooley wrote:  
> > > > > > >
> > > > > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:  
> > > > > > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > > > > > >
> > > > > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > ---
> > > > > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > > > > > >  1 file changed, 62 insertions(+)  create mode 100644
> > > > > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sara
> > > > > > > > dc.y
> > > > > > > > aml
> > > > > > > >
> > > > > > > > diff --git
> > > > > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > > > radc
> > > > > > > > .yam
> > > > > > > > l
> > > > > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > > > radc
> > > > > > > > .yam
> > > > > > > > l
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..ba8e19b72ad7
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb10
> > > > > > > > +++ 0-sa
> > > > > > > > +++ radc
> > > > > > > > +++ .yaml
> > > > > > > > @@ -0,0 +1,62 @@
> > > > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML
> > > > > > > > +1.2
> > > > > > > > +---
> > > > > > > > +$id:
> > > > > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.
> > > > > > > > +yaml
> > > > > > > > +#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: Successive Approximation Register (SAR) A/D converter
> > > > > > > > +for the StarFive JHB100 SoC
> > > > > > > > +
> > > > > > > > +maintainers:
> > > > > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > +
> > > > > > > > +properties:
> > > > > > > > +  compatible:
> > > > > > > > +    const: starfive,jhb100-saradc
> > > > > > > > +
> > > > > > > > +  reg:
> > > > > > > > +    maxItem: 1
> > > > > > > > +
> > > > > > > > +  interrupts:
> > > > > > > > +    maxItems: 1
> > > > > > > > +
> > > > > > > > +  clocks:
> > > > > > > > +    maxItems: 1
> > > > > > > > +
> > > > > > > > +  resets:
> > > > > > > > +    maxItems: 2
> > > > > > > > +
> > > > > > > > +  "#io-channel-cells":
> > > > > > > > +    const: 1
> > > > > > > > +
> > > > > > > > +  upper-bound-mv:
> > > > > > > > +    description: The upper bound voltage value of the monitor.
> > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > +
> > > > > > > > +  lower-bound-mv:
> > > > > > > > +    description: The lower bound voltage value of the monitor.
> > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > +
> > > > > > > > +  scan-freq:
> > > > > > > > +    description: Number of the scan cycle interval.
> > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16  
> > > > > > >
> > > > > > > Can you explain why any of these three properties are something
> > > > > > > that should be in the devicetree rather than software controlled?  
> > > > > >
> > > > > > My intention is to be able to obtain the initial values from the
> > > > > > devicetree during  
> > > > > probe and preset them.  
> > > > > > Do I need to drop them and just set them through sysfs?  
> > > > >
> > > > > Unless the hardware configuration determines the values (which I
> > > > > can't really see being the case for scan-freq at least) then yes,
> > > > > you need to drop and set them via sysfs.  
> > > >
> > > > The ADC hardware can be set the scan-freq register to determine how frequent it  
> > > should scan its inputs.  
> > > > The calculation is:
> > > > 	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
> > > > The maximum allowable scan frequency is 5MHz.
> > > >  
> > > > >  
> > > > > > > How are the bounds calculated?  
> > > > > >
> > > > > > The measurement range of this ADC hardware is from 0 to 1800 mV.
> > > > > > This set  
> > > > > value cannot exceed it. This explanation will be added later.
> > > > >
> > > > > I'm asking how this is calculated so that I can tell if you the
> > > > > property is permitted or not.  
> > > >
> > > > The calculation of bound is:
> > > > bound-mv = 1800mv * (register value) / 0xFFF  
> > > 
> > > These are the formulas, but how does someone know what the value for bound-
> > > mv needs to be? Why would someone not just want to always use 1800mv?
> > >   
> > 
> > Can I add the 'maximum' and ' minimum' to provide clarification? And the driver will also check.  
> 
> All that does is repeat the 1800 mV though, what I am interested in is
> how someone determines if they should use 1600 mV or 200 mV etc. What
> aspect of the hardware do the bounds depend on?

There are two options here 
1. This is critical stuff to avoid hardware damage. (If you are relying on
   Linux for that you built your system wrong but if we ignore that...)
   Then userspace control should not be possible - or at least should
   only be able to move boundaries in directions that make them tighter.
2. It is advisory only and not related to hardware damage - in that case
   generally doesn't belong in DT.

Jonathan

Re: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6521 bytes --]

On Thu, May 21, 2026 at 12:39:49PM +0100, Jonathan Cameron wrote:
> On Thu, 21 May 2026 11:20:52 +0100
> Conor Dooley <conor@kernel.org> wrote:
> 
> > On Thu, May 21, 2026 at 09:54:27AM +0000, Xingyu Wu wrote:
> > > On 2026/5/20 23:15, Conor Dooley wrote:  
> > > > 
> > > > On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:  
> > > > > On 2026/5/19 18:00, Conor Dooley wrote:  
> > > > > >
> > > > > > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:  
> > > > > > > On 2026/5/19 00:24, Conor Dooley wrote:  
> > > > > > > >
> > > > > > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:  
> > > > > > > > > Add the new documentation of SAR-ADC for the StarFive JHB100 SoC.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > > ---
> > > > > > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62 +++++++++++++++++++
> > > > > > > > >  1 file changed, 62 insertions(+)  create mode 100644
> > > > > > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sara
> > > > > > > > > dc.y
> > > > > > > > > aml
> > > > > > > > >
> > > > > > > > > diff --git
> > > > > > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > > > > radc
> > > > > > > > > .yam
> > > > > > > > > l
> > > > > > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,jhb100-sa
> > > > > > > > > radc
> > > > > > > > > .yam
> > > > > > > > > l
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..ba8e19b72ad7
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfive,jhb10
> > > > > > > > > +++ 0-sa
> > > > > > > > > +++ radc
> > > > > > > > > +++ .yaml
> > > > > > > > > @@ -0,0 +1,62 @@
> > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML
> > > > > > > > > +1.2
> > > > > > > > > +---
> > > > > > > > > +$id:
> > > > > > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.
> > > > > > > > > +yaml
> > > > > > > > > +#
> > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > > +
> > > > > > > > > +title: Successive Approximation Register (SAR) A/D converter
> > > > > > > > > +for the StarFive JHB100 SoC
> > > > > > > > > +
> > > > > > > > > +maintainers:
> > > > > > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > > +
> > > > > > > > > +properties:
> > > > > > > > > +  compatible:
> > > > > > > > > +    const: starfive,jhb100-saradc
> > > > > > > > > +
> > > > > > > > > +  reg:
> > > > > > > > > +    maxItem: 1
> > > > > > > > > +
> > > > > > > > > +  interrupts:
> > > > > > > > > +    maxItems: 1
> > > > > > > > > +
> > > > > > > > > +  clocks:
> > > > > > > > > +    maxItems: 1
> > > > > > > > > +
> > > > > > > > > +  resets:
> > > > > > > > > +    maxItems: 2
> > > > > > > > > +
> > > > > > > > > +  "#io-channel-cells":
> > > > > > > > > +    const: 1
> > > > > > > > > +
> > > > > > > > > +  upper-bound-mv:
> > > > > > > > > +    description: The upper bound voltage value of the monitor.
> > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > > +
> > > > > > > > > +  lower-bound-mv:
> > > > > > > > > +    description: The lower bound voltage value of the monitor.
> > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > > +
> > > > > > > > > +  scan-freq:
> > > > > > > > > +    description: Number of the scan cycle interval.
> > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16  
> > > > > > > >
> > > > > > > > Can you explain why any of these three properties are something
> > > > > > > > that should be in the devicetree rather than software controlled?  
> > > > > > >
> > > > > > > My intention is to be able to obtain the initial values from the
> > > > > > > devicetree during  
> > > > > > probe and preset them.  
> > > > > > > Do I need to drop them and just set them through sysfs?  
> > > > > >
> > > > > > Unless the hardware configuration determines the values (which I
> > > > > > can't really see being the case for scan-freq at least) then yes,
> > > > > > you need to drop and set them via sysfs.  
> > > > >
> > > > > The ADC hardware can be set the scan-freq register to determine how frequent it  
> > > > should scan its inputs.  
> > > > > The calculation is:
> > > > > 	frequency = 100/((register value) + 5) MHz, The register value should >= 15.
> > > > > The maximum allowable scan frequency is 5MHz.
> > > > >  
> > > > > >  
> > > > > > > > How are the bounds calculated?  
> > > > > > >
> > > > > > > The measurement range of this ADC hardware is from 0 to 1800 mV.
> > > > > > > This set  
> > > > > > value cannot exceed it. This explanation will be added later.
> > > > > >
> > > > > > I'm asking how this is calculated so that I can tell if you the
> > > > > > property is permitted or not.  
> > > > >
> > > > > The calculation of bound is:
> > > > > bound-mv = 1800mv * (register value) / 0xFFF  
> > > > 
> > > > These are the formulas, but how does someone know what the value for bound-
> > > > mv needs to be? Why would someone not just want to always use 1800mv?
> > > >   
> > > 
> > > Can I add the 'maximum' and ' minimum' to provide clarification? And the driver will also check.  
> > 
> > All that does is repeat the 1800 mV though, what I am interested in is
> > how someone determines if they should use 1600 mV or 200 mV etc. What
> > aspect of the hardware do the bounds depend on?
> 
> There are two options here 
> 1. This is critical stuff to avoid hardware damage. (If you are relying on
>    Linux for that you built your system wrong but if we ignore that...)
>    Then userspace control should not be possible - or at least should
>    only be able to move boundaries in directions that make them tighter.
> 2. It is advisory only and not related to hardware damage - in that case
>    generally doesn't belong in DT.

Per Xingyu's latest response
| These bounds are just for the monitor mode. If the input voltage is outside this range, interrupt will be triggered to report the user space.
| The bounds value (1600mv or 200mv) are determined by the users' applications. Users can freely set them according to the range they want to monitor.
it controls the level at which interrupts occur, and is therefore not
suitable for DT.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v1 1/2] bindings: iio: adc: Add StarFive JHB100 SARADC

[Xingyu Wu]

On 2026/5/22 00:42, Conor Dooley wrote:
> 
> On Thu, May 21, 2026 at 12:39:49PM +0100, Jonathan Cameron wrote:
> > On Thu, 21 May 2026 11:20:52 +0100
> > Conor Dooley <conor@kernel.org> wrote:
> >
> > > On Thu, May 21, 2026 at 09:54:27AM +0000, Xingyu Wu wrote:
> > > > On 2026/5/20 23:15, Conor Dooley wrote:
> > > > >
> > > > > On Wed, May 20, 2026 at 09:43:02AM +0000, Xingyu Wu wrote:
> > > > > > On 2026/5/19 18:00, Conor Dooley wrote:
> > > > > > >
> > > > > > > On Tue, May 19, 2026 at 09:26:03AM +0000, Xingyu Wu wrote:
> > > > > > > > On 2026/5/19 00:24, Conor Dooley wrote:
> > > > > > > > >
> > > > > > > > > On Mon, May 18, 2026 at 04:18:51PM +0800, Xingyu Wu wrote:
> > > > > > > > > > Add the new documentation of SAR-ADC for the StarFive JHB100
> SoC.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > > > ---
> > > > > > > > > >  .../iio/adc/starfive,jhb100-saradc.yaml       | 62
> +++++++++++++++++++
> > > > > > > > > >  1 file changed, 62 insertions(+)  create mode 100644
> > > > > > > > > > Documentation/devicetree/bindings/iio/adc/starfive,jhb
> > > > > > > > > > 100-sara
> > > > > > > > > > dc.y
> > > > > > > > > > aml
> > > > > > > > > >
> > > > > > > > > > diff --git
> > > > > > > > > > a/Documentation/devicetree/bindings/iio/adc/starfive,j
> > > > > > > > > > hb100-sa
> > > > > > > > > > radc
> > > > > > > > > > .yam
> > > > > > > > > > l
> > > > > > > > > > b/Documentation/devicetree/bindings/iio/adc/starfive,j
> > > > > > > > > > hb100-sa
> > > > > > > > > > radc
> > > > > > > > > > .yam
> > > > > > > > > > l
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000000..ba8e19b72ad7
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/starfi
> > > > > > > > > > +++ ve,jhb10
> > > > > > > > > > +++ 0-sa
> > > > > > > > > > +++ radc
> > > > > > > > > > +++ .yaml
> > > > > > > > > > @@ -0,0 +1,62 @@
> > > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > > > > > > +%YAML
> > > > > > > > > > +1.2
> > > > > > > > > > +---
> > > > > > > > > > +$id:
> > > > > > > > > > +http://devicetree.org/schemas/iio/adc/starfive,jhb100-saradc.
> > > > > > > > > > +yaml
> > > > > > > > > > +#
> > > > > > > > > > +$schema:
> > > > > > > > > > +http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > > > +
> > > > > > > > > > +title: Successive Approximation Register (SAR) A/D
> > > > > > > > > > +converter for the StarFive JHB100 SoC
> > > > > > > > > > +
> > > > > > > > > > +maintainers:
> > > > > > > > > > +  - Xingyu Wu <xingyu.wu@starfivetech.com>
> > > > > > > > > > +
> > > > > > > > > > +properties:
> > > > > > > > > > +  compatible:
> > > > > > > > > > +    const: starfive,jhb100-saradc
> > > > > > > > > > +
> > > > > > > > > > +  reg:
> > > > > > > > > > +    maxItem: 1
> > > > > > > > > > +
> > > > > > > > > > +  interrupts:
> > > > > > > > > > +    maxItems: 1
> > > > > > > > > > +
> > > > > > > > > > +  clocks:
> > > > > > > > > > +    maxItems: 1
> > > > > > > > > > +
> > > > > > > > > > +  resets:
> > > > > > > > > > +    maxItems: 2
> > > > > > > > > > +
> > > > > > > > > > +  "#io-channel-cells":
> > > > > > > > > > +    const: 1
> > > > > > > > > > +
> > > > > > > > > > +  upper-bound-mv:
> > > > > > > > > > +    description: The upper bound voltage value of the monitor.
> > > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > > > +
> > > > > > > > > > +  lower-bound-mv:
> > > > > > > > > > +    description: The lower bound voltage value of the monitor.
> > > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > > > +
> > > > > > > > > > +  scan-freq:
> > > > > > > > > > +    description: Number of the scan cycle interval.
> > > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint16
> > > > > > > > >
> > > > > > > > > Can you explain why any of these three properties are
> > > > > > > > > something that should be in the devicetree rather than software
> controlled?
> > > > > > > >
> > > > > > > > My intention is to be able to obtain the initial values
> > > > > > > > from the devicetree during
> > > > > > > probe and preset them.
> > > > > > > > Do I need to drop them and just set them through sysfs?
> > > > > > >
> > > > > > > Unless the hardware configuration determines the values
> > > > > > > (which I can't really see being the case for scan-freq at
> > > > > > > least) then yes, you need to drop and set them via sysfs.
> > > > > >
> > > > > > The ADC hardware can be set the scan-freq register to
> > > > > > determine how frequent it
> > > > > should scan its inputs.
> > > > > > The calculation is:
> > > > > > 	frequency = 100/((register value) + 5) MHz, The register value
> should >= 15.
> > > > > > The maximum allowable scan frequency is 5MHz.
> > > > > >
> > > > > > >
> > > > > > > > > How are the bounds calculated?
> > > > > > > >
> > > > > > > > The measurement range of this ADC hardware is from 0 to 1800 mV.
> > > > > > > > This set
> > > > > > > value cannot exceed it. This explanation will be added later.
> > > > > > >
> > > > > > > I'm asking how this is calculated so that I can tell if you
> > > > > > > the property is permitted or not.
> > > > > >
> > > > > > The calculation of bound is:
> > > > > > bound-mv = 1800mv * (register value) / 0xFFF
> > > > >
> > > > > These are the formulas, but how does someone know what the value
> > > > > for bound- mv needs to be? Why would someone not just want to always
> use 1800mv?
> > > > >
> > > >
> > > > Can I add the 'maximum' and ' minimum' to provide clarification? And the
> driver will also check.
> > >
> > > All that does is repeat the 1800 mV though, what I am interested in
> > > is how someone determines if they should use 1600 mV or 200 mV etc.
> > > What aspect of the hardware do the bounds depend on?
> >
> > There are two options here
> > 1. This is critical stuff to avoid hardware damage. (If you are relying on
> >    Linux for that you built your system wrong but if we ignore that...)
> >    Then userspace control should not be possible - or at least should
> >    only be able to move boundaries in directions that make them tighter.
> > 2. It is advisory only and not related to hardware damage - in that case
> >    generally doesn't belong in DT.
> 
> Per Xingyu's latest response
> | These bounds are just for the monitor mode. If the input voltage is outside this
> range, interrupt will be triggered to report the user space.
> | The bounds value (1600mv or 200mv) are determined by the users' applications.
> Users can freely set them according to the range they want to monitor.
> it controls the level at which interrupts occur, and is therefore not suitable for DT.
> 

OK, I will drop them in DT and use IIO event to set them in driver by user.

Best regards,
Xingyu Wu

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

[Xingyu Wu]

On 2026/5/21 19:37, Jonathan Cameron wrote:
> 
> >
> > >
> > > > +
> > > > +	starfive_saradc_ch_monitor_stop(priv, ch);
> > >
> > > Why stop it?  Add a comment.  We aren't interested in future events?
> >
> > If we do not stop it and the input voltage remains constant, the ADC could
> continuously monitor the voltage and continuously trigger interrupts for the
> bound.
> > As a result, the CPU utilization rate will be high.
> 
> Can you use an edge interrupt instead of a level one?

No, it is a level interrupt in hardware.

> 
> If not, then a common solution is to disable for a period (maybe a second or so)
> then reenable. There are various more refined ways of doing this.

This is good idea. How about using timer to disable it?

> 
> Basically we don't want to be in a situation where a momentary blip disables the
> event and we miss a later condition that must be handled.
> 

Noted.

Best regards,
Xingyu Wu

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

[Jonathan Cameron]

On Fri, 22 May 2026 02:20:53 +0000
Xingyu Wu <xingyu.wu@starfivetech.com> wrote:

> On 2026/5/21 19:37, Jonathan Cameron wrote:
> >   
> > >  
> > > >  
> > > > > +
> > > > > +	starfive_saradc_ch_monitor_stop(priv, ch);  
> > > >
> > > > Why stop it?  Add a comment.  We aren't interested in future events?  
> > >
> > > If we do not stop it and the input voltage remains constant, the ADC could  
> > continuously monitor the voltage and continuously trigger interrupts for the
> > bound.  
> > > As a result, the CPU utilization rate will be high.  
> > 
> > Can you use an edge interrupt instead of a level one?  
> 
> No, it is a level interrupt in hardware.

Just to check. That's a restriction of the interrupt controller IP in that
it doesn't support edge interrupts on this particularly line?
Seems unfortunate given the block on the other end is providing something
that seems like an edge :(  Ah well - hardly the first time we've hit that.
Add some comments to the code when you add the timer approach to
re enabling.

> 
> > 
> > If not, then a common solution is to disable for a period (maybe a second or so)
> > then reenable. There are various more refined ways of doing this.  
> 
> This is good idea. How about using timer to disable it?

yes, there are examples in tree doing that.

J

> 
> > 
> > Basically we don't want to be in a situation where a momentary blip disables the
> > event and we miss a later condition that must be handled.
> >   
> 
> Noted.
> 
> Best regards,
> Xingyu Wu