[PATCH v5 0/2] iio: adc: Add the Ingenic X1600 ADC

[PATCH v5 0/2] iio: adc: Add the Ingenic X1600 ADC

[Olivier Louvignes]

A driver for the SAR ADC of the Ingenic X1600: four auxiliary channels,
12 bits, one interrupt per channel. Tested on a HiBy R1, whose keys are on
channel 0 behind adc-keys.

As in v3 and v4, this version is X1600-only. The X1830 and the X2000 have
the same register layout by their programming manuals, and a chip_info
with the channel count and the resolution can add them later, with a test
on either.

Changes in v5:
- Binding: the reference is the SADC_VREFP pin (X1600 datasheet: ball
  P12, 3.3 V typical, at most the ADC's own supply), so an optional
  vref-supply, and the description says so in Nikolaus's words. The
  driver takes the supply's voltage, and 3.3 V without one. The ADC's
  analog supply (SADC_AVDD) is left out, like the SoC's other supply
  pins (Conor's question; Nikolaus answered it).
- Binding example: reg covers the 4 KiB the programming manual's memory
  map gives the SADC (Conor; Nikolaus on the map).
- The driver's declarations in reverse xmas tree, the timeout macro in
  jiffies with an if/else on the wait, a blank line, and clamp()'s
  bounds on one line (Andy).
- The clock stays unnamed: Krzysztof (on v2) and Conor (on v1) asked for
  clock-names to go, as the block has one clock. A variant with more
  would come with a compatible of its own and could name them there
  (Andy asked for a name).
- The name stays "adc" rather than the vendor's "sadc" (Nikolaus): the
  JZ4740 datasheet calls its block the SADC too, and mainline binds it
  as ingenic,jz4740-adc.
- Andy's Reviewed-by on patch 2, and Nikolaus's Tested-by (LX16, driver
  only, given on v4) on both.
- Rebased on togreg after -rc1.

Tested on the HiBy R1 with v5: without a vref-supply, as the R1 is
described, the scale falls back to 3.3 V and the three buttons on
channel 0 work through adc-keys; with a vref-supply pointed, for the
test only, at the board's 3.3 V PMIC rail, the driver waits for the
regulator and takes its voltage. v5 is also built with W=1 with and
without CONFIG_REGULATOR.

Changes in v4:
- The Kconfig prompt, the header comment and MODULE_DESCRIPTION() are
  the same text (Andy).
- The read computes the data register's bank and half next to each
  other, and the two macros take them (Andy).
- X1600_ADC_CLK_RATE_HZ (Andy).
- Read and write helpers that add the base (Andy). Plain MMIO rather
  than regmap: nothing is cached or shared, and the only
  read-modify-writes are under the conversion mutex or in probe.
- The device clock rate is clamped before dividing (Andy); the divider
  comes out the same for every rate.
- Binding: the description's last sentence and clock-names dropped; the
  driver takes its one clock unnamed (Krzysztof on v2 and Conor on v1:
  their reviews and v3 crossed).
- Joshua's Reviewed-by on patch 2: the changes to it are the points
  above, with no change in behaviour.

Changes in v3:
- Cover letter (Andy, Joshua).
- MAINTAINERS: no list line; the binding's path in patch 1, the driver's
  in patch 2 (Joshua).
- Includes for what the driver uses, copyright line, MODULE_AUTHOR(),
  reverse xmas tree, open-coded return, no message after
  devm_request_irq(), a blank line (Joshua; Andy on the return).
- fsleep(2 * USEC_PER_MSEC) (Andy).
- The ADC clock divider is now even, as the X1600 programming manual
  has it; v2 wrote the divider minus one, as the vendor kernel does. The
  divider fields are named as in the manual, and the two clocks only
  repeated sampling uses stay at their reset values.
- The probe comment on staying powered up no longer names the keys
  (Jonathan).

Changes in v2 (Jonathan):
- The comparison with the JZ47xx ADCs dropped from the file header.
- No mod_devicetable.h include.
- X1600_ADC_VREF_mV.
- The status declaration split from its readl() in the interrupt
  handler.
- field_get() for the channel's result.
- fsleep(2000) instead of usleep_range().

v4: https://lore.kernel.org/all/20261102201155.10948-1-olivier@mgcrea.io/
v3: https://lore.kernel.org/all/20261026083631.48406-1-olivier@mgcrea.io/
v2: https://lore.kernel.org/all/20261024041357.34346-1-olivier@mgcrea.io/
v1: https://lore.kernel.org/all/20261023204209.48556-1-olivier@mgcrea.io/

Olivier Louvignes (2):
  dt-bindings: iio: adc: Add the Ingenic X1600 ADC
  iio: adc: Add a driver for the Ingenic X1600 ADC

 .../bindings/iio/adc/ingenic,x1600-adc.yaml   |  59 ++++
 MAINTAINERS                                   |   6 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ingenic-x1600-adc.c           | 269 ++++++++++++++++++
 5 files changed, 345 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml
 create mode 100644 drivers/iio/adc/ingenic-x1600-adc.c


base-commit: 1390ca4953a7400ca932cb49635f02c727f885a6
-- 
2.39.5

[PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[Olivier Louvignes]

The X1600's SAR ADC has four auxiliary channels and none of the
touchscreen or battery engines of the JZ47xx ADCs in ingenic,adc.yaml,
so it gets a binding of its own.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - driver only, no real world test
---
 .../bindings/iio/adc/ingenic,x1600-adc.yaml   | 59 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml b/Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml
new file mode 100644
index 000000000000..f5d4726fdbae
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ingenic,x1600-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ingenic X1600 ADC
+
+maintainers:
+  - Olivier Louvignes <olivier@mgcrea.io>
+
+description:
+  The SAR ADC of the Ingenic X1600 SoC, with four auxiliary input channels
+  measured against an external SADC_VREFP reference (typically 3.3 V).
+
+properties:
+  compatible:
+    const: ingenic,x1600-adc
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#io-channel-cells':
+    const: 1
+
+  vref-supply:
+    description:
+      The SADC_VREFP reference. If absent, the reference is taken as 3.3 V.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+  - '#io-channel-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    adc@10070000 {
+        compatible = "ingenic,x1600-adc";
+        reg = <0x10070000 0x1000>;
+
+        clocks = <&cgu 28>;
+
+        interrupt-parent = <&intc>;
+        interrupts = <11>;
+
+        vref-supply = <&vcc_3v3>;
+
+        #io-channel-cells = <1>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 0017755f593b..4ab4ae67f788 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13176,6 +13176,11 @@ F:	include/linux/mfd/ingenic-tcu.h
 F:	sound/soc/codecs/jz47*
 F:	sound/soc/jz4740/
 
+INGENIC X1600 ADC DRIVER
+M:	Olivier Louvignes <olivier@mgcrea.io>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml
+
 INJOINIC IP5xxx POWER BANK IC DRIVER
 M:	Samuel Holland <samuel@sholland.org>
 S:	Maintained
-- 
2.39.5

[PATCH v5 2/2] iio: adc: Add a driver for the Ingenic X1600 ADC

[Olivier Louvignes]

The X1600's SAR ADC has four auxiliary channels, each with its own start
bit, interrupt flag and 12-bit result, and none of the touchscreen or
battery engines of the JZ47xx ADCs that ingenic-adc drives. Add a small
driver that exposes the four channels as voltages against the SADC_VREFP
reference: the vref-supply's voltage when there is one, else 3.3 V, its
typical value.

The registers are in chapter 19 of the X1600 programming manual. The
ADC clock divider is even, as the manual has it (an odd value divides
by one less); the vendor kernel writes the divider minus one, which
gives a slightly faster clock than it asks for.

Tested on a HiBy R1, whose play/pause and volume keys sit on channel 0
behind adc-keys: all three read and report correctly.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - driver only, no real world test
---
 MAINTAINERS                         |   1 +
 drivers/iio/adc/Kconfig             |  10 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/ingenic-x1600-adc.c | 269 ++++++++++++++++++++++++++++
 4 files changed, 281 insertions(+)
 create mode 100644 drivers/iio/adc/ingenic-x1600-adc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ab4ae67f788..81d5d1c89cff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13180,6 +13180,7 @@ INGENIC X1600 ADC DRIVER
 M:	Olivier Louvignes <olivier@mgcrea.io>
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml
+F:	drivers/iio/adc/ingenic-x1600-adc.c
 
 INJOINIC IP5xxx POWER BANK IC DRIVER
 M:	Samuel Holland <samuel@sholland.org>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f8456c803988..fab8147834b1 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -891,6 +891,16 @@ config INGENIC_ADC
 	  This driver can also be built as a module. If so, the module will be
 	  called ingenic_adc.
 
+config INGENIC_X1600_ADC
+	tristate "Ingenic X1600 ADC driver"
+	depends on MIPS || COMPILE_TEST
+	help
+	  Say yes here to build support for the SAR ADC of the Ingenic X1600
+	  SoC and its four auxiliary channels.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ingenic-x1600-adc.
+
 config INTEL_DC_TI_ADC
 	tristate "Intel Bay Trail / Cherry Trail Dollar Cove TI ADC driver"
 	depends on INTEL_SOC_PMIC_CHTDC_TI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d7bb4815df61..702715603c94 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
 obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
 obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
 obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
+obj-$(CONFIG_INGENIC_X1600_ADC) += ingenic-x1600-adc.o
 obj-$(CONFIG_INTEL_DC_TI_ADC) += intel_dc_ti_adc.o
 obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/ingenic-x1600-adc.c b/drivers/iio/adc/ingenic-x1600-adc.c
new file mode 100644
index 000000000000..81bc651731b2
--- /dev/null
+++ b/drivers/iio/adc/ingenic-x1600-adc.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ingenic X1600 ADC driver
+ *
+ * Copyright (c) 2026 Olivier Louvignes <olivier@mgcrea.io>
+ *
+ * The X1600's SAR ADC has four auxiliary channels, each with its own start
+ * bit, interrupt and 12-bit result (X1600 programming manual, chapter 19).
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/time.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#define X1600_ADC_REG_ENABLE		0x00
+#define X1600_ADC_REG_CTRL		0x08
+#define X1600_ADC_REG_STATUS		0x0c
+#define X1600_ADC_REG_DATA(bank)	(0x10 + (bank) * 4)
+#define X1600_ADC_REG_CLKDIV		0x20
+#define X1600_ADC_REG_STABLE		0x24
+
+/* ENABLE: bits 3..0 start a channel's conversion; bit 15 powers down. */
+#define X1600_ADC_ENABLE_POWER_DOWN	BIT(15)
+/* CTRL: bits 3..0 mask the channels' interrupts. STATUS: write 1 to clear. */
+#define X1600_ADC_CHANNELS_MASK		GENMASK(3, 0)
+#define X1600_ADC_DATA_MASK(shift)	(GENMASK(11, 0) << ((shift) * 16))
+/* CLKDIV: an even divider from the device clock; US and MS: for repeats. */
+#define X1600_ADC_CLKDIV_CLKDIV		GENMASK(7, 0)
+#define X1600_ADC_CLKDIV_US		GENMASK(15, 8)
+#define X1600_ADC_CLKDIV_MS		GENMASK(31, 16)
+#define X1600_ADC_STABLE_TIME		GENMASK(15, 0)
+
+#define X1600_ADC_NUM_CHANNELS		4
+/* SADC_VREFP when no vref-supply gives it: the datasheet's typical value. */
+#define X1600_ADC_VREF_DEFAULT_mV	3300
+#define X1600_ADC_BITS			12
+/* The ADC clock the vendor kernel runs at. */
+#define X1600_ADC_CLK_RATE_HZ		1166667
+#define X1600_ADC_TIMEOUT		msecs_to_jiffies(20)
+
+struct x1600_adc {
+	void __iomem *base;
+	struct clk *clk;
+	struct completion done;
+	int vref_mV;
+	/* Serialises conversions: there is one completion for all channels. */
+	struct mutex lock;
+};
+
+static u32 x1600_adc_read(struct x1600_adc *adc, unsigned int offset)
+{
+	return readl(adc->base + offset);
+}
+
+static void x1600_adc_write(struct x1600_adc *adc, unsigned int offset, u32 value)
+{
+	writel(value, adc->base + offset);
+}
+
+static irqreturn_t x1600_adc_irq(int irq, void *data)
+{
+	struct x1600_adc *adc = data;
+	u32 status;
+
+	status = x1600_adc_read(adc, X1600_ADC_REG_STATUS) & X1600_ADC_CHANNELS_MASK;
+	if (!status)
+		return IRQ_NONE;
+
+	x1600_adc_write(adc, X1600_ADC_REG_STATUS, status);
+	complete(&adc->done);
+	return IRQ_HANDLED;
+}
+
+static int x1600_adc_read_raw_channel(struct x1600_adc *adc, unsigned int ch, int *val)
+{
+	unsigned int bank, shift;
+	u32 enable, data;
+	int ret;
+
+	guard(mutex)(&adc->lock);
+
+	reinit_completion(&adc->done);
+	x1600_adc_write(adc, X1600_ADC_REG_STATUS, BIT(ch));
+	enable = x1600_adc_read(adc, X1600_ADC_REG_ENABLE);
+	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, enable | BIT(ch));
+
+	if (wait_for_completion_timeout(&adc->done, X1600_ADC_TIMEOUT))
+		ret = 0;
+	else
+		ret = -ETIMEDOUT;
+
+	enable = x1600_adc_read(adc, X1600_ADC_REG_ENABLE);
+	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, enable & ~BIT(ch));
+	if (ret)
+		return ret;
+
+	/* Two results to a data register, the even channel's in the low half. */
+	bank = ch / 2;
+	shift = ch % 2;
+	data = x1600_adc_read(adc, X1600_ADC_REG_DATA(bank));
+	*val = field_get(X1600_ADC_DATA_MASK(shift), data);
+
+	return 0;
+}
+
+static int x1600_adc_read_raw(struct iio_dev *iio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2, long mask)
+{
+	struct x1600_adc *adc = iio_priv(iio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = x1600_adc_read_raw_channel(adc, chan->channel, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = adc->vref_mV;
+		*val2 = X1600_ADC_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info x1600_adc_info = {
+	.read_raw = x1600_adc_read_raw,
+};
+
+#define X1600_ADC_CHANNEL(_ch) {				\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = (_ch),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+}
+
+static const struct iio_chan_spec x1600_adc_channels[] = {
+	X1600_ADC_CHANNEL(0),
+	X1600_ADC_CHANNEL(1),
+	X1600_ADC_CHANNEL(2),
+	X1600_ADC_CHANNEL(3),
+};
+
+static void x1600_adc_power_down(void *data)
+{
+	struct x1600_adc *adc = data;
+
+	x1600_adc_write(adc, X1600_ADC_REG_CTRL, X1600_ADC_CHANNELS_MASK);
+	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, X1600_ADC_ENABLE_POWER_DOWN);
+}
+
+static int x1600_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iio_dev *iio_dev;
+	struct x1600_adc *adc;
+	unsigned long rate;
+	u32 div, reg;
+	int irq, ret;
+
+	iio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(iio_dev);
+	init_completion(&adc->done);
+	ret = devm_mutex_init(dev, &adc->lock);
+	if (ret)
+		return ret;
+
+	adc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(adc->base))
+		return PTR_ERR(adc->base);
+
+	adc->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(adc->clk))
+		return dev_err_probe(dev, PTR_ERR(adc->clk), "Unable to get clock\n");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Unable to get the reference voltage\n");
+
+	adc->vref_mV = ret == -ENODEV ? X1600_ADC_VREF_DEFAULT_mV : ret / (MICRO / MILLI);
+
+	/* Powered down, interrupts masked and cleared, as the vendor does. */
+	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, X1600_ADC_ENABLE_POWER_DOWN);
+	x1600_adc_write(adc, X1600_ADC_REG_CTRL, X1600_ADC_CHANNELS_MASK);
+	x1600_adc_write(adc, X1600_ADC_REG_STATUS, X1600_ADC_CHANNELS_MASK);
+
+	rate = clamp(clk_get_rate(adc->clk),
+		     2UL * X1600_ADC_CLK_RATE_HZ, 254UL * X1600_ADC_CLK_RATE_HZ);
+	div = round_up(DIV_ROUND_UP(rate, X1600_ADC_CLK_RATE_HZ), 2);
+	/* US and MS at their reset values: single conversions don't use them. */
+	x1600_adc_write(adc, X1600_ADC_REG_CLKDIV,
+			FIELD_PREP(X1600_ADC_CLKDIV_CLKDIV, div) |
+			FIELD_PREP(X1600_ADC_CLKDIV_US, 16) |
+			FIELD_PREP(X1600_ADC_CLKDIV_MS, 4));
+	reg = x1600_adc_read(adc, X1600_ADC_REG_STABLE) & ~X1600_ADC_STABLE_TIME;
+	x1600_adc_write(adc, X1600_ADC_REG_STABLE,
+			reg | FIELD_PREP(X1600_ADC_STABLE_TIME, 1));
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, x1600_adc_irq, 0, dev_name(dev), adc);
+	if (ret)
+		return ret;
+
+	/*
+	 * Left powered up: a typical use, such as buttons read through adc-keys,
+	 * polls every few tens of milliseconds, and powering up takes 2 ms.
+	 */
+	x1600_adc_write(adc, X1600_ADC_REG_ENABLE, 0);
+	fsleep(2 * USEC_PER_MSEC);
+	x1600_adc_write(adc, X1600_ADC_REG_CTRL, 0);
+
+	ret = devm_add_action_or_reset(dev, x1600_adc_power_down, adc);
+	if (ret)
+		return ret;
+
+	iio_dev->name = "x1600-adc";
+	iio_dev->modes = INDIO_DIRECT_MODE;
+	iio_dev->channels = x1600_adc_channels;
+	iio_dev->num_channels = ARRAY_SIZE(x1600_adc_channels);
+	iio_dev->info = &x1600_adc_info;
+
+	return devm_iio_device_register(dev, iio_dev);
+}
+
+static const struct of_device_id x1600_adc_of_match[] = {
+	{ .compatible = "ingenic,x1600-adc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, x1600_adc_of_match);
+
+static struct platform_driver x1600_adc_driver = {
+	.driver = {
+		.name = "ingenic-x1600-adc",
+		.of_match_table = x1600_adc_of_match,
+	},
+	.probe = x1600_adc_probe,
+};
+module_platform_driver(x1600_adc_driver);
+MODULE_AUTHOR("Olivier Louvignes <olivier@mgcrea.io>");
+MODULE_DESCRIPTION("Ingenic X1600 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[Krzysztof Kozlowski]

On 05/11/2026 22:32, Olivier Louvignes wrote:
> The X1600's SAR ADC has four auxiliary channels and none of the
> touchscreen or battery engines of the JZ47xx ADCs in ingenic,adc.yaml,
> so it gets a binding of its own.
> 
> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - driver only, no real world test

Drop, did not happen. Even comment says it did not happen!

Otherwise please explain me how this code was executed on a device.

Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[H. Nikolaus Schaller]

> Am 06.11.2026 um 08:05 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
> On 05/11/2026 22:32, Olivier Louvignes wrote:
>> The X1600's SAR ADC has four auxiliary channels and none of the
>> touchscreen or battery engines of the JZ47xx ADCs in ingenic,adc.yaml,
>> so it gets a binding of its own.
>> 
>> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - driver only, no real world test
> 
> Drop, did not happen. Even comment says it did not happen!

What did not happen? Are you expecting a software test or a full hardware test?

> 
> Otherwise please explain me how this code was executed on a device.

1. add decive tree entries, 2. compile, 3. install, 4. boot and load module, 5. check sysfs

root@letux-lx16v0-3:~# ls -l /sys/devices/platform/apb/10070000.sadc
total 0
lrwxrwxrwx 1 root root    0 Nov  6 08:00 driver -> ../../../../bus/platform/drivers/ingenic-x1600-adc
-rw-r--r-- 1 root root 4096 Nov  6 08:00 driver_override
drwxr-xr-x 3 root root    0 Nov  6 07:58 iio:device0
-r--r--r-- 1 root root 4096 Nov  6 08:00 modalias
lrwxrwxrwx 1 root root    0 Nov  6 08:00 of_node -> ../../../../firmware/devicetree/base/apb/sadc@10070000
drwxr-xr-x 2 root root    0 Nov  6 08:00 power
lrwxrwxrwx 1 root root    0 Nov  6 08:00 subsystem -> ../../../../bus/platform
-rw-r--r-- 1 root root 4096 Nov  6 08:00 uevent
root@letux-lx16v0-3:~# cat /sys/devices/platform/apb/10070000.sadc/iio\:device0/in_voltage*
1512
1793
1796
1798
0.805664062
root@letux-lx16v0-3:~# cat /sys/devices/platform/apb/10070000.sadc/iio\:device0/in_voltage*
1512
1760
1766
1773
0.805664062
root@letux-lx16v0-3:~# cat /sys/devices/platform/apb/10070000.sadc/iio\:device0/in_voltage*
1518
1767
1767
1776
0.805664062
root@letux-lx16v0-3:~# 

You can see that the values look reasonable and fluctuate a little by noise as you would
expect from an ADC with floating inputs.

But are you expecting a hardware test that the values are similar to the real voltages?
This is beyond my hardware capabilities.

Maybe you are more happy with:

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - bindings and driver function only, no real electrical performance test

BR and have a good day,
Nikolaus

Re: [PATCH v5 0/2] iio: adc: Add the Ingenic X1600 ADC

[H. Nikolaus Schaller]

> Am 05.11.2026 um 22:32 schrieb Olivier Louvignes <olivier@mgcrea.io>:
> 
> A driver for the SAR ADC of the Ingenic X1600: four auxiliary channels,
> 12 bits, one interrupt per channel. Tested on a HiBy R1, whose keys are on
> channel 0 behind adc-keys.
> 
> As in v3 and v4, this version is X1600-only. The X1830 and the X2000 have
> the same register layout by their programming manuals, and a chip_info
> with the channel count and the resolution can add them later, with a test
> on either.
> 
> Changes in v5:
> - Binding: the reference is the SADC_VREFP pin (X1600 datasheet: ball
>  P12, 3.3 V typical, at most the ADC's own supply), so an optional
>  vref-supply, and the description says so in Nikolaus's words. The
>  driver takes the supply's voltage, and 3.3 V without one. The ADC's
>  analog supply (SADC_AVDD) is left out, like the SoC's other supply
>  pins (Conor's question; Nikolaus answered it).
> - Binding example: reg covers the 4 KiB the programming manual's memory
>  map gives the SADC (Conor; Nikolaus on the map).
> - The driver's declarations in reverse xmas tree, the timeout macro in
>  jiffies with an if/else on the wait, a blank line, and clamp()'s
>  bounds on one line (Andy).
> - The clock stays unnamed: Krzysztof (on v2) and Conor (on v1) asked for
>  clock-names to go, as the block has one clock. A variant with more
>  would come with a compatible of its own and could name them there
>  (Andy asked for a name).
> - The name stays "adc" rather than the vendor's "sadc" (Nikolaus): the
>  JZ4740 datasheet calls its block the SADC too, and mainline binds it
>  as ingenic,jz4740-adc.

Agreed. I wasn't aware of that. So let's stay consistent with the inconsistency :)

> - Andy's Reviewed-by on patch 2, and Nikolaus's Tested-by (LX16, driver
>  only, given on v4) on both.
> - Rebased on togreg after -rc1.
> 
> Tested on the HiBy R1 with v5: without a vref-supply, as the R1 is
> described, the scale falls back to 3.3 V and the three buttons on
> channel 0 work through adc-keys; with a vref-supply pointed, for the
> test only, at the board's 3.3 V PMIC rail, the driver waits for the
> regulator and takes its voltage. v5 is also built with W=1 with and
> without CONFIG_REGULATOR.
> 
> Changes in v4:
> - The Kconfig prompt, the header comment and MODULE_DESCRIPTION() are
>  the same text (Andy).
> - The read computes the data register's bank and half next to each
>  other, and the two macros take them (Andy).
> - X1600_ADC_CLK_RATE_HZ (Andy).
> - Read and write helpers that add the base (Andy). Plain MMIO rather
>  than regmap: nothing is cached or shared, and the only
>  read-modify-writes are under the conversion mutex or in probe.
> - The device clock rate is clamped before dividing (Andy); the divider
>  comes out the same for every rate.
> - Binding: the description's last sentence and clock-names dropped; the
>  driver takes its one clock unnamed (Krzysztof on v2 and Conor on v1:
>  their reviews and v3 crossed).
> - Joshua's Reviewed-by on patch 2: the changes to it are the points
>  above, with no change in behaviour.
> 
> Changes in v3:
> - Cover letter (Andy, Joshua).
> - MAINTAINERS: no list line; the binding's path in patch 1, the driver's
>  in patch 2 (Joshua).
> - Includes for what the driver uses, copyright line, MODULE_AUTHOR(),
>  reverse xmas tree, open-coded return, no message after
>  devm_request_irq(), a blank line (Joshua; Andy on the return).
> - fsleep(2 * USEC_PER_MSEC) (Andy).
> - The ADC clock divider is now even, as the X1600 programming manual
>  has it; v2 wrote the divider minus one, as the vendor kernel does. The
>  divider fields are named as in the manual, and the two clocks only
>  repeated sampling uses stay at their reset values.
> - The probe comment on staying powered up no longer names the keys
>  (Jonathan).
> 
> Changes in v2 (Jonathan):
> - The comparison with the JZ47xx ADCs dropped from the file header.
> - No mod_devicetable.h include.
> - X1600_ADC_VREF_mV.
> - The status declaration split from its readl() in the interrupt
>  handler.
> - field_get() for the channel's result.
> - fsleep(2000) instead of usleep_range().
> 
> v4: https://lore.kernel.org/all/20261102201155.10948-1-olivier@mgcrea.io/
> v3: https://lore.kernel.org/all/20261026083631.48406-1-olivier@mgcrea.io/
> v2: https://lore.kernel.org/all/20261024041357.34346-1-olivier@mgcrea.io/
> v1: https://lore.kernel.org/all/20261023204209.48556-1-olivier@mgcrea.io/
> 
> Olivier Louvignes (2):
>  dt-bindings: iio: adc: Add the Ingenic X1600 ADC
>  iio: adc: Add a driver for the Ingenic X1600 ADC
> 
> .../bindings/iio/adc/ingenic,x1600-adc.yaml   |  59 ++++
> MAINTAINERS                                   |   6 +
> drivers/iio/adc/Kconfig                       |  10 +
> drivers/iio/adc/Makefile                      |   1 +
> drivers/iio/adc/ingenic-x1600-adc.c           | 269 ++++++++++++++++++
> 5 files changed, 345 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,x1600-adc.yaml
> create mode 100644 drivers/iio/adc/ingenic-x1600-adc.c
> 
> 
> base-commit: 1390ca4953a7400ca932cb49635f02c727f885a6
> -- 
> 2.39.5
> 

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[Krzysztof Kozlowski]

On 06/11/2026 09:05, H. Nikolaus Schaller wrote:
> 
> 
>> Am 06.11.2026 um 08:05 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>
>> On 05/11/2026 22:32, Olivier Louvignes wrote:
>>> The X1600's SAR ADC has four auxiliary channels and none of the
>>> touchscreen or battery engines of the JZ47xx ADCs in ingenic,adc.yaml,
>>> so it gets a binding of its own.
>>>
>>> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
>>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - driver only, no real world test
>>
>> Drop, did not happen. Even comment says it did not happen!
> 
> What did not happen? Are you expecting a software test or a full hardware test?

Testing of this patch did not happen.

> 
>>
>> Otherwise please explain me how this code was executed on a device.
> 
> 1. add decive tree entries, 2. compile, 3. install, 4. boot and load module, 5. check sysfs

And none of above is in this patch, so you just confirmed this patch was
not tested.

Again, please drop inaccurate tag from this patch.


Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[H. Nikolaus Schaller]

> Am 06.11.2026 um 09:11 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
> 
> On 06/11/2026 09:05, H. Nikolaus Schaller wrote:
>> 
>> 
>>> Am 06.11.2026 um 08:05 schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>>> 
>>> On 05/11/2026 22:32, Olivier Louvignes wrote:
>>>> The X1600's SAR ADC has four auxiliary channels and none of the
>>>> touchscreen or battery engines of the JZ47xx ADCs in ingenic,adc.yaml,
>>>> so it gets a binding of its own.
>>>> 
>>>> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
>>>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # LX16 - driver only, no real world test
>>> 
>>> Drop, did not happen. Even comment says it did not happen!
>> 
>> What did not happen? Are you expecting a software test or a full hardware test?
> 
> Testing of this patch did not happen.
> 
>> 
>>> 
>>> Otherwise please explain me how this code was executed on a device.
>> 
>> 1. add decive tree entries, 2. compile, 3. install, 4. boot and load module, 5. check sysfs
> 
> And none of above is in this patch, so you just confirmed this patch was
> not tested.

???

Sorry but I can't follow you here..

> 
> Again, please drop inaccurate tag from this patch.

Ah, on third thought I start to understand...

Because you look at *this* patch in isolation while I looked at the whole
series.

Yes, you are right - the dt-bindings is not directly tested (indirectly it 
was because I copied the example from this patch into my device tree and
it worked). Then, the ADC driver was tested (but not the hardware).

So this tag should not simply be dropped but moved to the driver code.

BR and thanks,
Nikolaus

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[Krzysztof Kozlowski]

On 06/11/2026 09:18, H. Nikolaus Schaller wrote:
>>
>> Again, please drop inaccurate tag from this patch.
> 
> Ah, on third thought I start to understand...
> 
> Because you look at *this* patch in isolation while I looked at the whole
> series.

I did not comment under the driver patch, nor under the cover letter. I
placed my comment in specific context - this patch touching the
bindings, because you cannot test bindings on a device.


> 
> Yes, you are right - the dt-bindings is not directly tested (indirectly it 
> was because I copied the example from this patch into my device tree and
> it worked). Then, the ADC driver was tested (but not the hardware).
> 
> So this tag should not simply be dropped but moved to the driver code.
Yes, please. Thank you for testing, it is important and should be
credited, just accordingly.


Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[Olivier Louvignes]

Hi Krzysztof,

On 06/11/2026 09:23, Krzysztof Kozlowski wrote:
> On 06/11/2026 09:18, H. Nikolaus Schaller wrote:
>> So this tag should not simply be dropped but moved to the driver code.
> Yes, please. Thank you for testing, it is important and should be
> credited, just accordingly.

Right, my mistake, I carried it to both patches. Sorry about that.

Jonathan, could you drop Nikolaus's Tested-by from 1/2 when applying? It
stays on 2/2, which already has it. Or I can send a v6 with only that
change if you prefer,

Thanks,
Olivier

Re: [PATCH v5 1/2] dt-bindings: iio: adc: Add the Ingenic X1600 ADC

[Krzysztof Kozlowski]

On 06/11/2026 10:59, Olivier Louvignes wrote:
> Hi Krzysztof,
> 
> On 06/11/2026 09:23, Krzysztof Kozlowski wrote:
>> On 06/11/2026 09:18, H. Nikolaus Schaller wrote:
>>> So this tag should not simply be dropped but moved to the driver code.
>> Yes, please. Thank you for testing, it is important and should be
>> credited, just accordingly.
> 
> Right, my mistake, I carried it to both patches. Sorry about that.
> 
> Jonathan, could you drop Nikolaus's Tested-by from 1/2 when applying? It
> stays on 2/2, which already has it. Or I can send a v6 with only that
> change if you prefer,
> 

I suggest sending v6, dropping the tag here and adding mine after review:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof