Devicetree
 help / color / mirror / Atom feed
* [PATCH v4 0/2] iio: adc: Add support for Texas Instruments ADS112C04
@ 2026-08-11  2:48 Kyle Hsieh
  2026-08-11  2:48 ` [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04 Kyle Hsieh
  2026-08-11  2:48 ` [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
  0 siblings, 2 replies; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-11  2:48 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, Kyle Hsieh

This patch series introduces support for the Texas Instruments ADS112C04
Analog-to-Digital Converters.

The ADS112C04 (16-bit) is precision, low-power, delta-sigma ADCs with
an I2C interface. They feature a flexible input multiplexer supporting
single-ended and differential measurements, a programmable gain amplifier,
and an internal voltage reference.

Note: While this chip shares similarities with the ADS112C14 (currently 
being upstreamed by David Lechner), the register maps and feature sets 
are sufficiently different to warrant a separate driver. However, the
DT bindings and channel parsing logic have been aligned with the
ADS112C14 conventions.

This initial submission provides a minimal feature set (single-shot 
conversions and basic DRDY interrupt) covering current use cases.

Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
---
Changes in v4:
- dt-bindings: iio: adc: ti,ads112c04:
  - Fixed excitation-current-nanoamp to use single-entry list syntax
    (items: - enum: [...]) instead of combining maxItems: 1 with a
    mapping-style items, which failed dt_binding_check.
  - Widened excitation-channels to maximum: 5 to account for IDAC1/
    IDAC2 also being routable to REFP0/REFN0, and documented the
    mapping, per David Lechner.
  - Reduced excitation-current-nanoamp to a single entry, since the
    two IDAC outputs share one current setting, per David Lechner.
  - Changed burn-out-current-nanoamp to use `const: 10000`, per David
    Lechner.
  - Renamed reference-sources' internal enum value to "internal"
    (dropped voltage suffix, since there is only one internal
    reference), per David Lechner.

- iio: adc: ti-ads112c04:
  - Fixed word-read byte ordering by switching to
    i2c_smbus_read_word_swapped(), addressing a big-endian data
    corruption issue flagged by Sashiko AI review.
  - Masked the DRDY interrupt in the hardirq handler
    (disable_irq_nosync()) and re-enabled it once the conversion data
    is read, to avoid an interrupt storm on level-triggered DRDY
    configurations, without hardcoding the trigger type.
  - Reworked reference-sources handling in parse_channels(): it is now
    validated against the actually-selected refp-supply instead of
    being unconditionally rejected, per David Lechner.
  - Simplified regulator handling in probe() using
    devm_regulator_get_enable_read_voltage(), dropping the now-unused
    vref_reg field, per David Lechner.
  - Corrected the commit message to no longer claim refn-supply
    support, since it remains explicitly unsupported in probe().
- Added a MAINTAINERS entry for the new binding and driver.
- Link to v3: https://lore.kernel.org/r/20260805-ti-ads112c04-driver-v3-0-49f436829e01@gmail.com

Changes in v3:
- dt-bindings: iio: adc: ti,ads112c04:
  - Resolved dt_binding_check errors by removing redundant $ref for
    standard unit suffixes and fixing YAML array syntax.
  - Added full hardware capability descriptions (excitation-channels,
    excitation-current-nanoamp, burn-out-current-nanoamp,
    reference-sources) as suggested by David Lechner.
  - Restricted the reg maximum to 11 and updated the regex to
    ^channel@[0-9a-b]$ to accurately reflect the 12 possible MUX
    combinations.

- iio: adc: ti-ads112c04:
  - Transitioned all I2C read/write wrappers to use SMBus APIs
    (i2c_smbus_read_byte_data, i2c_smbus_read_word_data, etc.) to
    gracefully handle I2C errors, NACKs, and Repeated Starts, addressing
    feedback from Joshua Crofts and Jonathan Cameron.
  - Added forward compatibility checks in probe() and parse_channels()
    using dev_err_probe() to return -EOPNOTSUPP for unimplemented DT
    properties.
  - Reverted devm_request_irq() flags to 0 to let the driver inherit the
    trigger type strictly from the DT, per Jonathan Cameron's advice.
  - Reduced the data wait timeout to 100ms, reflecting the chip's slowest
    data rate of 20 SPS.
  - Fixed #include alphabetical sorting, grouped <linux/iio/*> headers,
    and added missing headers.
  - Fixed variable declaration ordering (reverse xmas tree) and updated
    variable naming to vref_mV (SI unit exception).
  - Optimized GPIO reset logic using GPIOD_OUT_HIGH.
- Link to v2: https://lore.kernel.org/r/20260731-ti-ads112c04-driver-v2-0-aab0168c3c01@gmail.com

Changes in v2:
- Replaced `vref-supply` with `refp-supply` and `refn-supply` to accurately reflect hardware.
- Refactored the driver to dynamically parse channel configurations and routing from DT child nodes.
- Modernized the driver using kernel macros.
- Handled endianness elegantly.
- Added hardware reset fallback logic.
- Inherited IRQ trigger type from device tree instead of hardcoding.
- Fixed a bug where the MUX software cache could desync from hardware if the I2C write failed.
- Added strict return value checking for all I2C writes during probe.
- Updated the `i2c_device_id` array to use C99 named initializers.
- Link to v1: https://lore.kernel.org/r/20260728-ti-ads112c04-driver-v1-0-475efe4e2b78@gmail.com

---
Kyle Hsieh (2):
      dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04
      iio: adc: ti-ads112c04: Add support for TI ADS112C04

 .../devicetree/bindings/iio/adc/ti,ads112c04.yaml  | 148 +++++++
 MAINTAINERS                                        |   7 +
 drivers/iio/adc/Kconfig                            |  10 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ti-ads112c04.c                     | 425 +++++++++++++++++++++
 5 files changed, 591 insertions(+)
---
base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425
change-id: 20260724-ti-ads112c04-driver-be7e89047834

Best regards,
-- 
Kyle Hsieh <kylehsieh1995@gmail.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04
  2026-08-11  2:48 [PATCH v4 0/2] iio: adc: Add support for Texas Instruments ADS112C04 Kyle Hsieh
@ 2026-08-11  2:48 ` Kyle Hsieh
  2026-08-11 14:23   ` David Lechner
  2026-08-11 16:56   ` Conor Dooley
  2026-08-11  2:48 ` [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
  1 sibling, 2 replies; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-11  2:48 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, Kyle Hsieh

Add device tree binding documentation for Texas Instruments ADS112C04
I2C Analog-to-Digital Converters.

These devices provide 4-channel, 16-bit delta-sigma ADCs with an I2C
interface, programmable gain amplifier (PGA), and data-ready (DRDY)
interrupt output.

The binding uses child nodes to dynamically define the connected
single-ended or differential channels.

Excitation current, burn-out current, and reference source properties
are also included to fully describe the chip's hardware capabilities,
aligned with the ti,ads112c14 binding conventions.

Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
---
 .../devicetree/bindings/iio/adc/ti,ads112c04.yaml  | 148 +++++++++++++++++++++
 MAINTAINERS                                        |   6 +
 2 files changed, 154 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
new file mode 100644
index 000000000000..68a0df21cedd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
@@ -0,0 +1,148 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads112c04.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ADS112C04 ADC
+
+maintainers:
+  - Kyle Hsieh <kylehsieh1995@gmail.com>
+
+description:
+  The ADS112C04 (16-bit) are precision analog-to-digital converters (ADCs)
+  with an I2C interface. They feature a flexible input multiplexer, a
+  low-noise programmable gain amplifier (PGA), two programmable excitation
+  current sources, a voltage reference, and a precision temperature sensor.
+
+properties:
+  compatible:
+    enum:
+      - ti,ads112c04
+
+  reg:
+    maxItems: 1
+    description: I2C address of the device.
+
+  interrupts:
+    maxItems: 1
+    description: Data ready (DRDY) interrupt output.
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reset-gpios:
+    maxItems: 1
+    description: GPIO connected to the RESET pin. Active low.
+
+  avdd-supply: true
+  dvdd-supply: true
+
+  refp-supply: true
+  refn-supply: true
+
+  ti,refp-refn-resistor-ohms:
+    description: Resistance of the external resistor between REFP and REFN.
+
+patternProperties:
+  "^channel@[0-9a-b]$":
+    $ref: adc.yaml
+    unevaluatedProperties: false
+    properties:
+      reg:
+        items:
+          - maximum: 11
+
+      single-channel:
+        maximum: 3
+
+      diff-channels:
+        items:
+          maximum: 3
+
+      excitation-channels:
+        maxItems: 2
+        items:
+          maximum: 5
+        description:
+          Additionally to AIN0-AIN3, IDAC1/IDAC2 can also be routed to
+          REFP0 (4) and REFN0 (5).
+
+      excitation-current-nanoamp:
+        items:
+          - enum: [10000, 50000, 100000, 250000, 500000, 1000000, 1500000]
+
+      burn-out-current-nanoamp:
+        items:
+          - const: 10000
+
+      reference-sources:
+        items:
+          - enum: [internal, external, avdd]
+            default: internal
+
+    dependencies:
+      excitation-channels: [ excitation-current-nanoamp ]
+      excitation-current-nanoamp: [ excitation-channels ]
+
+    oneOf:
+      - required: [ single-channel ]
+      - required: [ diff-channels ]
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+  - dvdd-supply
+
+dependencies:
+  refn-supply: [ refp-supply ]
+
+oneOf:
+  - required:
+      - refp-supply
+  - required:
+      - ti,refp-refn-resistor-ohms
+  - properties:
+      refp-supply: false
+      refn-supply: false
+      ti,refp-refn-resistor-ohms: false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@40 {
+            compatible = "ti,ads112c04";
+            reg = <0x40>;
+            interrupt-parent = <&gpio>;
+            interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+
+            reset-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
+            avdd-supply = <&vdd_3v3_reg>;
+            dvdd-supply = <&vdd_3v3_reg>;
+            refp-supply = <&vref_reg>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                diff-channels = <0>, <1>;
+            };
+
+            channel@1 {
+                reg = <1>;
+                single-channel = <2>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 04fa5322d9f7..417d74b6d6cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26987,6 +26987,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
 F:	drivers/iio/adc/ti-ads1119.c
 
+TI ADS112C04 ADC DRIVER
+M:	Kyle Hsieh <kylehsieh1995@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
+
 TI ADS112C14 ADC DRIVER
 M:	David Lechner <dlechner@baylibre.com>
 L:	linux-iio@vger.kernel.org

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11  2:48 [PATCH v4 0/2] iio: adc: Add support for Texas Instruments ADS112C04 Kyle Hsieh
  2026-08-11  2:48 ` [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04 Kyle Hsieh
@ 2026-08-11  2:48 ` Kyle Hsieh
  2026-08-11  9:39   ` Andy Shevchenko
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-11  2:48 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown
  Cc: linux-iio, devicetree, linux-kernel, Kyle Hsieh

Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
delta-sigma ADCs.

The driver implements:
- Single-shot conversions using the IIO raw read interface.
- Dynamic parsing of single-ended and differential channels from
  device tree child nodes.
- Hardware interrupt support via the DRDY pin, falling back to
  software polling if no IRQ is provided.
- Scale calculation based on the internal 2.048V reference.
- Reference voltage scaling via the regulator subsystem (refp-supply),
  falling back to the internal 2.048V reference if not specified.
  refn-supply is not yet supported.
- Hardware reset fallback using GPIO.

Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
---
 MAINTAINERS                    |   1 +
 drivers/iio/adc/Kconfig        |  10 +
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/ti-ads112c04.c | 425 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 437 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 417d74b6d6cc..f51fbda9d4b9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26992,6 +26992,7 @@ M:	Kyle Hsieh <kylehsieh1995@gmail.com>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
+F:	drivers/iio/adc/ti-ads112c04.c
 
 TI ADS112C14 ADC DRIVER
 M:	David Lechner <dlechner@baylibre.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 990e7b3e7212..0ee50828f9e4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1817,6 +1817,16 @@ config TI_ADS1119
          This driver can also be built as a module. If so, the module will be
          called ti-ads1119.
 
+config TI_ADS112C04
+    tristate "Texas Instruments ADS112C04 ADC"
+    depends on I2C
+    help
+      If you say yes here you get support for Texas Instruments
+      ADS112C04 (16-bit) I2C analog to digital converters.
+
+      This driver can also be built as a module. If so, the module will be
+      called ti-ads112c04.
+
 config TI_ADS112C14
 	tristate "Texas Instruments ADS112C14/ADS122C14"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index dcec0abb03b7..d8acf2831fd2 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
 obj-$(CONFIG_TI_ADS1018) += ti-ads1018.o
 obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
 obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
+obj-$(CONFIG_TI_ADS112C04) += ti-ads112c04.o
 obj-$(CONFIG_TI_ADS112C14) += ti-ads112c14.o
 obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
 obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
new file mode 100644
index 000000000000..3a775da2ae23
--- /dev/null
+++ b/drivers/iio/adc/ti-ads112c04.c
@@ -0,0 +1,425 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Texas Instruments ADS112C04 16-bit I2C ADC driver
+ *
+ * Copyright (c) 2026 Kyle Hsieh <kylehsieh1995@gmail.com>
+ *
+ * Datasheet: https://www.ti.com/lit/ds/symlink/ads112c04.pdf
+ * Based on TI Reference Code and standard Linux IIO framework.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define ADS112C04_CMD_RESET         0x06
+#define ADS112C04_CMD_START_SYNC    0x08
+#define ADS112C04_CMD_POWERDOWN     0x02
+#define ADS112C04_CMD_RDATA         0x10
+#define ADS112C04_CMD_RREG(reg)     (0x20 | ((reg) << 2))
+#define ADS112C04_CMD_WREG(reg)     (0x40 | ((reg) << 2))
+
+#define ADS112C04_REG_CONFIG0       0x00
+#define ADS112C04_REG_CONFIG1       0x01
+#define ADS112C04_REG_CONFIG2       0x02
+#define ADS112C04_REG_CONFIG3       0x03
+
+#define ADS112C04_CONFIG0_MUX         GENMASK(7, 4)
+#define ADS112C04_CONFIG0_PGA_BYPASS  BIT(0)
+#define ADS112C04_CONFIG2_DRDY        BIT(7)
+
+struct ads112c04_state {
+	struct i2c_client *client;
+	/* Protects concurrent ADC reads and device configuration */
+	struct mutex lock;
+	struct completion completion;
+	int vref_mV;
+	u8 config0;
+	u8 config1;
+	bool has_refp;
+};
+
+static int ads112c04_write_cmd(struct i2c_client *client, u8 cmd)
+{
+	return i2c_smbus_write_byte(client, cmd);
+}
+
+static int ads112c04_read_reg(struct i2c_client *client, u8 reg, u8 *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, ADS112C04_CMD_RREG(reg));
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
+static int ads112c04_write_reg(struct i2c_client *client, u8 reg, u8 val)
+{
+	return i2c_smbus_write_byte_data(client, ADS112C04_CMD_WREG(reg), val);
+}
+
+static int ads112c04_wait_for_data(struct ads112c04_state *st)
+{
+	int ret, err;
+	u8 val;
+
+	if (st->client->irq > 0) {
+		/* Timeout is 100ms (slowest data rate is 20 SPS) */
+		ret = wait_for_completion_timeout(&st->completion,
+						  msecs_to_jiffies(100));
+		if (!ret)
+			return -ETIMEDOUT;
+
+		return 0;
+	}
+
+	err = read_poll_timeout(ads112c04_read_reg, ret,
+				(ret < 0 || (val & ADS112C04_CONFIG2_DRDY)),
+				1000, 100 * USEC_PER_MSEC, false,
+				st->client, ADS112C04_REG_CONFIG2, &val);
+
+	if (ret < 0)
+		return ret;
+
+	return err;
+}
+
+static int ads112c04_read_data(struct ads112c04_state *st, int *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_swapped(st->client, ADS112C04_CMD_RDATA);
+	if (ret < 0)
+		return ret;
+
+	*val = sign_extend32(ret, 15);
+
+	return 0;
+}
+
+static int ads112c04_get_adc_result(struct ads112c04_state *st,
+				    struct iio_chan_spec const *chan,
+				    int *val)
+{
+	u8 new_config0;
+	int ret;
+
+	new_config0 = st->config0;
+	FIELD_MODIFY(ADS112C04_CONFIG0_MUX, &new_config0, chan->address);
+
+	if (st->config0 != new_config0) {
+		ret = ads112c04_write_reg(st->client, ADS112C04_REG_CONFIG0, new_config0);
+		if (ret < 0)
+			return ret;
+		st->config0 = new_config0;
+	}
+
+	reinit_completion(&st->completion);
+
+	ret = ads112c04_write_cmd(st->client, ADS112C04_CMD_START_SYNC);
+	if (ret < 0)
+		return ret;
+
+	ret = ads112c04_wait_for_data(st);
+	if (ret < 0)
+		return ret;
+
+	ret = ads112c04_read_data(st, val);
+	if (st->client->irq > 0)
+		enable_irq(st->client->irq);
+
+	return ret;
+}
+
+static int ads112c04_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2, long mask)
+{
+	struct ads112c04_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+		ret = ads112c04_get_adc_result(st, chan, val);
+		mutex_unlock(&st->lock);
+
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mV;
+		*val2 = 15;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t ads112c04_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ads112c04_state *st = iio_priv(indio_dev);
+
+	disable_irq_nosync(irq);
+	complete(&st->completion);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info ads112c04_info = {
+	.read_raw = ads112c04_read_raw,
+};
+
+static int ads112c04_parse_channels(struct iio_dev *indio_dev)
+{
+	struct device *dev = indio_dev->dev.parent;
+	struct ads112c04_state *st = iio_priv(indio_dev);
+	struct iio_chan_spec *channels;
+	u32 num_channels, pair[2];
+	int ret, i = 0;
+
+	num_channels = device_get_named_child_node_count(dev, "channel");
+	if (!num_channels)
+		return dev_err_probe(dev, -EINVAL, "no channel subnodes found\n");
+
+	if (num_channels > 12)
+		return dev_err_probe(dev, -EINVAL,
+				     "num of channel nodes exceeds 12\n");
+
+	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	device_for_each_named_child_node_scoped(dev, child, "channel") {
+		struct iio_chan_spec *spec = &channels[i];
+
+		if (fwnode_property_present(child, "excitation-channels"))
+			return dev_err_probe(dev, -EOPNOTSUPP,
+					     "excitation-channels is not supported yet\n");
+
+		if (fwnode_property_present(child, "reference-sources")) {
+			const char *ref;
+
+			ret = fwnode_property_read_string(child, "reference-sources", &ref);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "failed to read reference-sources\n");
+
+			if ((!strcmp(ref, "external") && !st->has_refp) ||
+			    (!strcmp(ref, "internal") && st->has_refp))
+				return dev_err_probe(dev, -EINVAL,
+						     "reference-sources does not match refp-supply\n");
+		}
+
+		spec->type = IIO_VOLTAGE;
+		spec->indexed = 1;
+		spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
+
+		if (fwnode_property_present(child, "single-channel")) {
+			ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "failed to read single-channel property\n");
+
+			if (pair[0] > 3)
+				return dev_err_probe(dev, -EINVAL,
+						     "single-channel must be 0-3\n");
+
+			spec->channel = pair[0];
+			spec->address = 0x08 + pair[0];
+		} else if (fwnode_property_present(child, "diff-channels")) {
+			ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "failed to read diff-channels property\n");
+
+			if (pair[0] > 3 || pair[1] > 3)
+				return dev_err_probe(dev, -EINVAL,
+						     "diff-channels must be 0-3\n");
+
+			spec->channel = pair[0];
+			spec->channel2 = pair[1];
+			spec->differential = 1;
+
+			if (pair[0] == 0 && pair[1] == 1)
+				spec->address = 0x00;
+			else if (pair[0] == 0 && pair[1] == 2)
+				spec->address = 0x01;
+			else if (pair[0] == 0 && pair[1] == 3)
+				spec->address = 0x02;
+			else if (pair[0] == 1 && pair[1] == 0)
+				spec->address = 0x03;
+			else if (pair[0] == 1 && pair[1] == 2)
+				spec->address = 0x04;
+			else if (pair[0] == 1 && pair[1] == 3)
+				spec->address = 0x05;
+			else if (pair[0] == 2 && pair[1] == 3)
+				spec->address = 0x06;
+			else if (pair[0] == 3 && pair[1] == 2)
+				spec->address = 0x07;
+			else
+				return dev_err_probe(dev, -EINVAL,
+						     "invalid diff-channels combination\n");
+		} else {
+			return dev_err_probe(dev, -EINVAL,
+					     "channel node must have single-channel or diff-channels\n");
+		}
+
+		i++;
+	}
+
+	indio_dev->channels = channels;
+	indio_dev->num_channels = i;
+
+	return 0;
+}
+
+#define ADS112C04_VREF_INTERNAL_MV	2048
+
+static int ads112c04_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct ads112c04_state *st;
+	struct gpio_desc *reset_gpio;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->client = client;
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	init_completion(&st->completion);
+
+	indio_dev->name = "ads112c04";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ads112c04_info;
+
+	/* Forward compatibility checks for unimplemented DT properties */
+	if (device_property_present(dev, "refn-supply") ||
+	    device_property_present(dev, "ti,refp-refn-resistor-ohms"))
+		return dev_err_probe(dev, -EOPNOTSUPP,
+				     "refn-supply and external resistors are not supported yet\n");
+
+	st->has_refp = device_property_present(dev, "refp-supply");
+
+	ret = ads112c04_parse_channels(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_regulator_get_enable(dev, "avdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get avdd regulator\n");
+
+	ret = devm_regulator_get_enable(dev, "dvdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get dvdd regulator\n");
+
+	if (device_property_present(dev, "refp-supply")) {
+		ret = devm_regulator_get_enable_read_voltage(dev, "refp");
+		if (ret < 0)
+			return dev_err_probe(dev, ret,
+					     "failed to get refp voltage\n");
+
+		st->vref_mV = ret / (MICRO / MILLI);
+		st->config1 = 0x02;
+	} else {
+		st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
+		st->config1 = 0x00;
+	}
+
+	/* Power-On Reset (POR) delay */
+	fsleep(50 * USEC_PER_MSEC);
+
+	/* Requesting OUT_HIGH asserts the active-low reset pin immediately */
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return PTR_ERR(reset_gpio);
+
+	if (reset_gpio) {
+		fsleep(1000);
+		gpiod_set_value_cansleep(reset_gpio, 0);
+	} else {
+		ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
+		if (ret < 0)
+			return ret;
+	}
+
+	fsleep(1000);
+
+	/* Bypass PGA for now to allow full-scale single-ended measurements */
+	st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
+	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
+	if (ret)
+		return ret;
+
+	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
+	if (ret)
+		return ret;
+
+	if (client->irq > 0) {
+		ret = devm_request_irq(dev, client->irq,
+				       ads112c04_irq_handler,
+				       0,
+				       indio_dev->name, indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id ads112c04_id[] = {
+	{ .name = "ads112c04" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ads112c04_id);
+
+static const struct of_device_id ads112c04_of_match[] = {
+	{ .compatible = "ti,ads112c04" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ads112c04_of_match);
+
+static struct i2c_driver ads112c04_driver = {
+	.driver = {
+		.name = "ads112c04",
+		.of_match_table = ads112c04_of_match,
+	},
+	.probe = ads112c04_probe,
+	.id_table = ads112c04_id,
+};
+module_i2c_driver(ads112c04_driver);
+
+MODULE_AUTHOR("Kyle Hsieh <kylehsieh1995@gmail.com>");
+MODULE_DESCRIPTION("Texas Instruments ADS112C04 ADC driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11  2:48 ` [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
@ 2026-08-11  9:39   ` Andy Shevchenko
  2026-08-12  2:58     ` Kyle Hsieh
  2026-08-11 14:18   ` David Lechner
  2026-08-12  4:29   ` Jonathan Cameron
  2 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2026-08-11  9:39 UTC (permalink / raw)
  To: Kyle Hsieh
  Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown, linux-iio, devicetree, linux-kernel

On Tue, Aug 11, 2026 at 10:48:38AM +0800, Kyle Hsieh wrote:
> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> delta-sigma ADCs.
> 
> The driver implements:
> - Single-shot conversions using the IIO raw read interface.
> - Dynamic parsing of single-ended and differential channels from
>   device tree child nodes.
> - Hardware interrupt support via the DRDY pin, falling back to
>   software polling if no IRQ is provided.
> - Scale calculation based on the internal 2.048V reference.
> - Reference voltage scaling via the regulator subsystem (refp-supply),
>   falling back to the internal 2.048V reference if not specified.
>   refn-supply is not yet supported.
> - Hardware reset fallback using GPIO.

...

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> +{
> +	int ret, err;
> +	u8 val;
> +
> +	if (st->client->irq > 0) {
> +		/* Timeout is 100ms (slowest data rate is 20 SPS) */
> +		ret = wait_for_completion_timeout(&st->completion,
> +						  msecs_to_jiffies(100));
> +		if (!ret)

In this case semantics of ret differs, that's why it's better to write as

		if (!wait_for_completion_timeout(&st->completion, msecs_to_jiffies(100)))

// and I would even dare to put on a single line.

> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
> +
> +	err = read_poll_timeout(ads112c04_read_reg, ret,
> +				(ret < 0 || (val & ADS112C04_CONFIG2_DRDY)),
> +				1000, 100 * USEC_PER_MSEC, false,
> +				st->client, ADS112C04_REG_CONFIG2, &val);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return err;

In this piece I would swap err and ret, so the ret is outer one and err is
the inner one. This will be consistent with other code pieces.

> +}

...

> +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> +				    struct iio_chan_spec const *chan,
> +				    int *val)
> +{
> +	u8 new_config0;
> +	int ret;
> +
> +	new_config0 = st->config0;
> +	FIELD_MODIFY(ADS112C04_CONFIG0_MUX, &new_config0, chan->address);
> +
> +	if (st->config0 != new_config0) {
> +		ret = ads112c04_write_reg(st->client, ADS112C04_REG_CONFIG0, new_config0);
> +		if (ret < 0)
> +			return ret;
> +		st->config0 = new_config0;
> +	}
> +
> +	reinit_completion(&st->completion);
> +
> +	ret = ads112c04_write_cmd(st->client, ADS112C04_CMD_START_SYNC);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ads112c04_wait_for_data(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ads112c04_read_data(st, val);

> +	if (st->client->irq > 0)
> +		enable_irq(st->client->irq);

Why is it fine to leave IRQ enabled even in the error case?

> +	return ret;
> +}

...

> +static int ads112c04_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +		ret = ads112c04_get_adc_result(st, chan, val);
> +		mutex_unlock(&st->lock);
> +
> +		if (ret < 0)
> +			return ret;

If IRQ is left enabled and we call it here, we end up with the unbalanced
depth counting.

> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mV;
> +		*val2 = 15;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ads112c04_state *st = iio_priv(indio_dev);

> +	disable_irq_nosync(irq);

This is unconditionally called. Where is the guarantee that it becomes enabled
once again?

> +	complete(&st->completion);

> +	return IRQ_HANDLED;
> +}

...

> +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *channels;
> +	u32 num_channels, pair[2];

> +	int ret, i = 0;

Why is 'i' signed? And it's better to decouple definition and assignment, so
the assignment will happen closer to when it's really needed.

...

> +		if (fwnode_property_present(child, "reference-sources")) {
> +			const char *ref;
> +
> +			ret = fwnode_property_read_string(child, "reference-sources", &ref);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read reference-sources\n");
> +
> +			if ((!strcmp(ref, "external") && !st->has_refp) ||
> +			    (!strcmp(ref, "internal") && st->has_refp))
> +				return dev_err_probe(dev, -EINVAL,
> +						     "reference-sources does not match refp-supply\n");
> +		}

Reinvention of fwnode_property_match_property_string() ?

...

> +		if (fwnode_property_present(child, "single-channel")) {
> +			ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);

I don't like the (partial) pair reuse here. It's semantically wrong. Just add
another temporary variable and let compiler to choose what to do with a stack
frame in such a case.

> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read single-channel property\n");
> +
> +			if (pair[0] > 3)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "single-channel must be 0-3\n");
> +
> +			spec->channel = pair[0];
> +			spec->address = 0x08 + pair[0];
> +		} else if (fwnode_property_present(child, "diff-channels")) {
> +			ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);

ARRAY_SIZE()

> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read diff-channels property\n");
> +
> +			if (pair[0] > 3 || pair[1] > 3)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "diff-channels must be 0-3\n");
> +
> +			spec->channel = pair[0];
> +			spec->channel2 = pair[1];
> +			spec->differential = 1;

> +			if (pair[0] == 0 && pair[1] == 1)
> +				spec->address = 0x00;
> +			else if (pair[0] == 0 && pair[1] == 2)
> +				spec->address = 0x01;
> +			else if (pair[0] == 0 && pair[1] == 3)
> +				spec->address = 0x02;
> +			else if (pair[0] == 1 && pair[1] == 0)
> +				spec->address = 0x03;
> +			else if (pair[0] == 1 && pair[1] == 2)
> +				spec->address = 0x04;
> +			else if (pair[0] == 1 && pair[1] == 3)
> +				spec->address = 0x05;
> +			else if (pair[0] == 2 && pair[1] == 3)
> +				spec->address = 0x06;
> +			else if (pair[0] == 3 && pair[1] == 2)
> +				spec->address = 0x07;

I would do this as a 4x4 table

	-1, 0, 1, 2,
	3, -1, 4, 5,
	-1, -1, -1, 6,
	-1, -1, 7, -1,

With that done you can even supported the swapped cases

	-1, 0, 1, 2,
	3, -1, 4, 5,
	1, 4, -1, 6,
	2, 5, 7, -1,

(but I haven't studied the code if it's toughly relies on the pair[0]/pair[1]
 values to be in a strong order after the address being assigned).

> +			else
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid diff-channels combination\n");

> +		} else {
> +			return dev_err_probe(dev, -EINVAL,
> +					     "channel node must have single-channel or diff-channels\n");
> +		}
> +
> +		i++;
> +	}
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = i;
> +
> +	return 0;
> +}

...

> +#define ADS112C04_VREF_INTERNAL_MV	2048

_mV

...

> +	if (device_property_present(dev, "refp-supply")) {

A dup property check. if (st->has_refp) should suffice, no?

> +		ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to get refp voltage\n");
> +
> +		st->vref_mV = ret / (MICRO / MILLI);
> +		st->config1 = 0x02;
> +	} else {
> +		st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> +		st->config1 = 0x00;
> +	}

...

> +	/* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);

Why reset-gpio driver can't be used instead?

> +	if (reset_gpio) {
> +		fsleep(1000);

1 * USEC_PER_MSEC

> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	fsleep(1000);

Ditto.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11  2:48 ` [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
  2026-08-11  9:39   ` Andy Shevchenko
@ 2026-08-11 14:18   ` David Lechner
  2026-08-12  1:25     ` Kyle Hsieh
  2026-08-12  3:38     ` Kyle Hsieh
  2026-08-12  4:29   ` Jonathan Cameron
  2 siblings, 2 replies; 13+ messages in thread
From: David Lechner @ 2026-08-11 14:18 UTC (permalink / raw)
  To: Kyle Hsieh, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown
  Cc: linux-iio, devicetree, linux-kernel

On 8/10/26 9:48 PM, Kyle Hsieh wrote:
> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> delta-sigma ADCs.
> 
> The driver implements:
> - Single-shot conversions using the IIO raw read interface.
> - Dynamic parsing of single-ended and differential channels from
>   device tree child nodes.
> - Hardware interrupt support via the DRDY pin, falling back to
>   software polling if no IRQ is provided.
> - Scale calculation based on the internal 2.048V reference.
> - Reference voltage scaling via the regulator subsystem (refp-supply),
>   falling back to the internal 2.048V reference if not specified.
>   refn-supply is not yet supported.
> - Hardware reset fallback using GPIO.
> 
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/iio/adc/Kconfig        |  10 +
>  drivers/iio/adc/Makefile       |   1 +
>  drivers/iio/adc/ti-ads112c04.c | 425 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 437 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 417d74b6d6cc..f51fbda9d4b9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26992,6 +26992,7 @@ M:	Kyle Hsieh <kylehsieh1995@gmail.com>
>  L:	linux-iio@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
> +F:	drivers/iio/adc/ti-ads112c04.c
>  
>  TI ADS112C14 ADC DRIVER
>  M:	David Lechner <dlechner@baylibre.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 990e7b3e7212..0ee50828f9e4 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1817,6 +1817,16 @@ config TI_ADS1119
>           This driver can also be built as a module. If so, the module will be
>           called ti-ads1119.
>  
> +config TI_ADS112C04
> +    tristate "Texas Instruments ADS112C04 ADC"
> +    depends on I2C
> +    help
> +      If you say yes here you get support for Texas Instruments
> +      ADS112C04 (16-bit) I2C analog to digital converters.
> +
> +      This driver can also be built as a module. If so, the module will be
> +      called ti-ads112c04.
> +
>  config TI_ADS112C14
>  	tristate "Texas Instruments ADS112C14/ADS122C14"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index dcec0abb03b7..d8acf2831fd2 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -155,6 +155,7 @@ obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS1018) += ti-ads1018.o
>  obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
>  obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> +obj-$(CONFIG_TI_ADS112C04) += ti-ads112c04.o
>  obj-$(CONFIG_TI_ADS112C14) += ti-ads112c14.o
>  obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>  obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> new file mode 100644
> index 000000000000..3a775da2ae23
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c04.c
> @@ -0,0 +1,425 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Texas Instruments ADS112C04 16-bit I2C ADC driver
> + *
> + * Copyright (c) 2026 Kyle Hsieh <kylehsieh1995@gmail.com>
> + *
> + * Datasheet: https://www.ti.com/lit/ds/symlink/ads112c04.pdf
> + * Based on TI Reference Code and standard Linux IIO framework.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define ADS112C04_CMD_RESET         0x06
> +#define ADS112C04_CMD_START_SYNC    0x08
> +#define ADS112C04_CMD_POWERDOWN     0x02
> +#define ADS112C04_CMD_RDATA         0x10
> +#define ADS112C04_CMD_RREG(reg)     (0x20 | ((reg) << 2))
> +#define ADS112C04_CMD_WREG(reg)     (0x40 | ((reg) << 2))
> +
> +#define ADS112C04_REG_CONFIG0       0x00
> +#define ADS112C04_REG_CONFIG1       0x01
> +#define ADS112C04_REG_CONFIG2       0x02
> +#define ADS112C04_REG_CONFIG3       0x03
> +
> +#define ADS112C04_CONFIG0_MUX         GENMASK(7, 4)
> +#define ADS112C04_CONFIG0_PGA_BYPASS  BIT(0)
> +#define ADS112C04_CONFIG2_DRDY        BIT(7)
> +

I think I mentioned before the preferred way to orgainze the fields
under the corresponding register. And as below, there are more
fields we are using that need macros here.

> +struct ads112c04_state {
> +	struct i2c_client *client;
> +	/* Protects concurrent ADC reads and device configuration */
> +	struct mutex lock;
> +	struct completion completion;
> +	int vref_mV;
> +	u8 config0;
> +	u8 config1;
> +	bool has_refp;

This isn't used outside of probe. Can just be passed as a function argument.

> +};
> +

...

> +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +
> +	disable_irq_nosync(irq);

As mentioned in this recent discussion [1], we should not be disabling the IRQ.
If there is a true need for it, we need lots of comments explaining why. Since
we aren't doing buffered reads though, I can't see a reason why we would need
it right now. Unless we are worried about spurious interrupts, in which case
we should read the DRDY status bit before setting the completion here as that
would be more robust.

[1]: https://lore.kernel.org/linux-iio/20260802184126.04738251@jic23-huawei/

Also, this ia a bug because we can't call syncronous code in an IRQ handler.
We would need to change it to a threaded IRQ handler.

> +	complete(&st->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info ads112c04_info = {
> +	.read_raw = ads112c04_read_raw,
> +};
> +
> +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *channels;
> +	u32 num_channels, pair[2];
> +	int ret, i = 0;
> +
> +	num_channels = device_get_named_child_node_count(dev, "channel");
> +	if (!num_channels)
> +		return dev_err_probe(dev, -EINVAL, "no channel subnodes found\n");
> +
> +	if (num_channels > 12)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "num of channel nodes exceeds 12\n");
> +
> +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		struct iio_chan_spec *spec = &channels[i];
> +
> +		if (fwnode_property_present(child, "excitation-channels"))
> +			return dev_err_probe(dev, -EOPNOTSUPP,
> +					     "excitation-channels is not supported yet\n");
> +
> +		if (fwnode_property_present(child, "reference-sources")) {
> +			const char *ref;
> +
> +			ret = fwnode_property_read_string(child, "reference-sources", &ref);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read reference-sources\n");
> +
> +			if ((!strcmp(ref, "external") && !st->has_refp) ||
> +			    (!strcmp(ref, "internal") && st->has_refp))
> +				return dev_err_probe(dev, -EINVAL,
> +						     "reference-sources does not match refp-supply\n");

As explained more below, this part needs comments to explain why it is
the way it is since it is not implementing fully what is allowed by the
devicetree bindings. And would return EOPNOTSUPP to be consistent that
this is just something that is not implemented yet.

> +		}
> +
> +		spec->type = IIO_VOLTAGE;

I would add a REVISIT comment here to explain that when ti,refp-refn-resistor-ohms is implemented,
then we have the possibility of the channel being resistance instead of votlage.

> +		spec->indexed = 1;
> +		spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> +
> +		if (fwnode_property_present(child, "single-channel")) {
> +			ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read single-channel property\n");
> +
> +			if (pair[0] > 3)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "single-channel must be 0-3\n");
> +
> +			spec->channel = pair[0];
> +			spec->address = 0x08 + pair[0];
> +		} else if (fwnode_property_present(child, "diff-channels")) {
> +			ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read diff-channels property\n");
> +
> +			if (pair[0] > 3 || pair[1] > 3)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "diff-channels must be 0-3\n");
> +
> +			spec->channel = pair[0];
> +			spec->channel2 = pair[1];
> +			spec->differential = 1;
> +
> +			if (pair[0] == 0 && pair[1] == 1)
> +				spec->address = 0x00;
> +			else if (pair[0] == 0 && pair[1] == 2)
> +				spec->address = 0x01;
> +			else if (pair[0] == 0 && pair[1] == 3)
> +				spec->address = 0x02;
> +			else if (pair[0] == 1 && pair[1] == 0)
> +				spec->address = 0x03;
> +			else if (pair[0] == 1 && pair[1] == 2)
> +				spec->address = 0x04;
> +			else if (pair[0] == 1 && pair[1] == 3)
> +				spec->address = 0x05;
> +			else if (pair[0] == 2 && pair[1] == 3)
> +				spec->address = 0x06;
> +			else if (pair[0] == 3 && pair[1] == 2)
> +				spec->address = 0x07;
> +			else
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid diff-channels combination\n");
> +		} else {
> +			return dev_err_probe(dev, -EINVAL,
> +					     "channel node must have single-channel or diff-channels\n");
> +		}
> +
> +		i++;
> +	}
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = i;
> +
> +	return 0;
> +}
> +
> +#define ADS112C04_VREF_INTERNAL_MV	2048

Usually we put this at the top with other macro definitions.

> +
> +static int ads112c04_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct ads112c04_state *st;
> +	struct gpio_desc *reset_gpio;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	init_completion(&st->completion);
> +
> +	indio_dev->name = "ads112c04";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ads112c04_info;
> +
> +	/* Forward compatibility checks for unimplemented DT properties */
> +	if (device_property_present(dev, "refn-supply") ||
> +	    device_property_present(dev, "ti,refp-refn-resistor-ohms"))
> +		return dev_err_probe(dev, -EOPNOTSUPP,
> +				     "refn-supply and external resistors are not supported yet\n");
> +
> +	st->has_refp = device_property_present(dev, "refp-supply");
> +
> +	ret = ads112c04_parse_channels(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_get_enable(dev, "avdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get avdd regulator\n");
> +
> +	ret = devm_regulator_get_enable(dev, "dvdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get dvdd regulator\n");
> +
> +	if (device_property_present(dev, "refp-supply")) {

This can just be st->has_refp now.

> +		ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to get refp voltage\n");
> +
> +		st->vref_mV = ret / (MICRO / MILLI);
> +		st->config1 = 0x02;

I've asked before... these values should have macros to explain what
they are and be using FIELD_PREP().

> +	} else {
> +		st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> +		st->config1 = 0x00;
> +	}

I still don't think storing vref like this is a good idea since it should
be per-channel. If you really are against implementing it in a more
future-proof way, then it needs lots of comments explaining why it is
implemented this way instead and how one would go about doing it the "right
way" in the future.

> +
> +	/* Power-On Reset (POR) delay */
> +	fsleep(50 * USEC_PER_MSEC);
> +
> +	/* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);
> +
> +	if (reset_gpio) {
> +		fsleep(1000);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	fsleep(1000);
> +
> +	/* Bypass PGA for now to allow full-scale single-ended measurements */
> +	st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
> +	if (ret)
> +		return ret;
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_irq(dev, client->irq,
> +				       ads112c04_irq_handler,
> +				       0,

I think I've asked twice now to move this 0 on the previous line.
If you don't agree with that, it is fine, but we just ask that you
reply to the suggestion with an explanation to state your reasoning.
Otherwise, it comes across as carelessness that you continue to
ignore suggestions. This is not the only suggestoin that has been
silently ignored. You might want to go back to the previous revisions
and see if you missed anything else.

> +				       indio_dev->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04
  2026-08-11  2:48 ` [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04 Kyle Hsieh
@ 2026-08-11 14:23   ` David Lechner
  2026-08-11 16:56   ` Conor Dooley
  1 sibling, 0 replies; 13+ messages in thread
From: David Lechner @ 2026-08-11 14:23 UTC (permalink / raw)
  To: Kyle Hsieh, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown
  Cc: linux-iio, devicetree, linux-kernel

On 8/10/26 9:48 PM, Kyle Hsieh wrote:
> Add device tree binding documentation for Texas Instruments ADS112C04
> I2C Analog-to-Digital Converters.
> 
> These devices provide 4-channel, 16-bit delta-sigma ADCs with an I2C
> interface, programmable gain amplifier (PGA), and data-ready (DRDY)
> interrupt output.
> 
> The binding uses child nodes to dynamically define the connected
> single-ended or differential channels.
> 
> Excitation current, burn-out current, and reference source properties
> are also included to fully describe the chip's hardware capabilities,
> aligned with the ti,ads112c14 binding conventions.
> 
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04
  2026-08-11  2:48 ` [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04 Kyle Hsieh
  2026-08-11 14:23   ` David Lechner
@ 2026-08-11 16:56   ` Conor Dooley
  2026-08-12  2:53     ` Kyle Hsieh
  1 sibling, 1 reply; 13+ messages in thread
From: Conor Dooley @ 2026-08-11 16:56 UTC (permalink / raw)
  To: Kyle Hsieh
  Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown, linux-iio, devicetree, linux-kernel

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

On Tue, Aug 11, 2026 at 10:48:37AM +0800, Kyle Hsieh wrote:
> Add device tree binding documentation for Texas Instruments ADS112C04
> I2C Analog-to-Digital Converters.
> 
> These devices provide 4-channel, 16-bit delta-sigma ADCs with an I2C
> interface, programmable gain amplifier (PGA), and data-ready (DRDY)
> interrupt output.
> 
> The binding uses child nodes to dynamically define the connected
> single-ended or differential channels.
> 
> Excitation current, burn-out current, and reference source properties
> are also included to fully describe the chip's hardware capabilities,
> aligned with the ti,ads112c14 binding conventions.
> 
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>

> +patternProperties:
> +  "^channel@[0-9a-b]$":
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +    properties:
> +      reg:
> +        items:
> +          - maximum: 11
> +
> +      single-channel:
> +        maximum: 3

Can someone explain to me please why the maximum for this is 3 when reg
goes up to 11? What do the additional reg entries beyond 3 represent?

> +
> +      diff-channels:
> +        items:
> +          maximum: 3
> +
> +      excitation-channels:
> +        maxItems: 2
> +        items:
> +          maximum: 5
> +        description:
> +          Additionally to AIN0-AIN3, IDAC1/IDAC2 can also be routed to
> +          REFP0 (4) and REFN0 (5).

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11 14:18   ` David Lechner
@ 2026-08-12  1:25     ` Kyle Hsieh
  2026-08-12  3:38     ` Kyle Hsieh
  1 sibling, 0 replies; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-12  1:25 UTC (permalink / raw)
  To: David Lechner
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown,
	linux-iio, devicetree, linux-kernel

On Tue, Aug 11, 2026 at 10:18 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 8/10/26 9:48 PM, Kyle Hsieh wrote:
> > Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> > delta-sigma ADCs.
> >
> > The driver implements:
> > - Single-shot conversions using the IIO raw read interface.
> > - Dynamic parsing of single-ended and differential channels from
> >   device tree child nodes.
> > - Hardware interrupt support via the DRDY pin, falling back to
> >   software polling if no IRQ is provided.
> > - Scale calculation based on the internal 2.048V reference.
> > - Reference voltage scaling via the regulator subsystem (refp-supply),
> >   falling back to the internal 2.048V reference if not specified.
> >   refn-supply is not yet supported.
> > - Hardware reset fallback using GPIO.
> >
> > Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
> > ---
> >  MAINTAINERS                    |   1 +
> >  drivers/iio/adc/Kconfig        |  10 +
> >  drivers/iio/adc/Makefile       |   1 +
> >  drivers/iio/adc/ti-ads112c04.c | 425 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 437 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 417d74b6d6cc..f51fbda9d4b9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26992,6 +26992,7 @@ M:    Kyle Hsieh <kylehsieh1995@gmail.com>
> >  L:   linux-iio@vger.kernel.org
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
> > +F:   drivers/iio/adc/ti-ads112c04.c
> >
> >  TI ADS112C14 ADC DRIVER
> >  M:   David Lechner <dlechner@baylibre.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 990e7b3e7212..0ee50828f9e4 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -1817,6 +1817,16 @@ config TI_ADS1119
> >           This driver can also be built as a module. If so, the module will be
> >           called ti-ads1119.
> >
> > +config TI_ADS112C04
> > +    tristate "Texas Instruments ADS112C04 ADC"
> > +    depends on I2C
> > +    help
> > +      If you say yes here you get support for Texas Instruments
> > +      ADS112C04 (16-bit) I2C analog to digital converters.
> > +
> > +      This driver can also be built as a module. If so, the module will be
> > +      called ti-ads112c04.
> > +
> >  config TI_ADS112C14
> >       tristate "Texas Instruments ADS112C14/ADS122C14"
> >       depends on I2C
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index dcec0abb03b7..d8acf2831fd2 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -155,6 +155,7 @@ obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> >  obj-$(CONFIG_TI_ADS1018) += ti-ads1018.o
> >  obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
> >  obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> > +obj-$(CONFIG_TI_ADS112C04) += ti-ads112c04.o
> >  obj-$(CONFIG_TI_ADS112C14) += ti-ads112c14.o
> >  obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> >  obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> > diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> > new file mode 100644
> > index 000000000000..3a775da2ae23
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads112c04.c
> > @@ -0,0 +1,425 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Texas Instruments ADS112C04 16-bit I2C ADC driver
> > + *
> > + * Copyright (c) 2026 Kyle Hsieh <kylehsieh1995@gmail.com>
> > + *
> > + * Datasheet: https://www.ti.com/lit/ds/symlink/ads112c04.pdf
> > + * Based on TI Reference Code and standard Linux IIO framework.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define ADS112C04_CMD_RESET         0x06
> > +#define ADS112C04_CMD_START_SYNC    0x08
> > +#define ADS112C04_CMD_POWERDOWN     0x02
> > +#define ADS112C04_CMD_RDATA         0x10
> > +#define ADS112C04_CMD_RREG(reg)     (0x20 | ((reg) << 2))
> > +#define ADS112C04_CMD_WREG(reg)     (0x40 | ((reg) << 2))
> > +
> > +#define ADS112C04_REG_CONFIG0       0x00
> > +#define ADS112C04_REG_CONFIG1       0x01
> > +#define ADS112C04_REG_CONFIG2       0x02
> > +#define ADS112C04_REG_CONFIG3       0x03
> > +
> > +#define ADS112C04_CONFIG0_MUX         GENMASK(7, 4)
> > +#define ADS112C04_CONFIG0_PGA_BYPASS  BIT(0)
> > +#define ADS112C04_CONFIG2_DRDY        BIT(7)
> > +
>
> I think I mentioned before the preferred way to orgainze the fields
> under the corresponding register. And as below, there are more
> fields we are using that need macros here.
>
> > +struct ads112c04_state {
> > +     struct i2c_client *client;
> > +     /* Protects concurrent ADC reads and device configuration */
> > +     struct mutex lock;
> > +     struct completion completion;
> > +     int vref_mV;
> > +     u8 config0;
> > +     u8 config1;
> > +     bool has_refp;
>
> This isn't used outside of probe. Can just be passed as a function argument.
>
> > +};
> > +
>
> ...
>
> > +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> > +{
> > +     struct iio_dev *indio_dev = private;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +
> > +     disable_irq_nosync(irq);
>
> As mentioned in this recent discussion [1], we should not be disabling the IRQ.
> If there is a true need for it, we need lots of comments explaining why. Since
> we aren't doing buffered reads though, I can't see a reason why we would need
> it right now. Unless we are worried about spurious interrupts, in which case
> we should read the DRDY status bit before setting the completion here as that
> would be more robust.
>
> [1]: https://lore.kernel.org/linux-iio/20260802184126.04738251@jic23-huawei/
>
> Also, this ia a bug because we can't call syncronous code in an IRQ handler.
> We would need to change it to a threaded IRQ handler.
>
> > +     complete(&st->completion);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info ads112c04_info = {
> > +     .read_raw = ads112c04_read_raw,
> > +};
> > +
> > +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> > +{
> > +     struct device *dev = indio_dev->dev.parent;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +     struct iio_chan_spec *channels;
> > +     u32 num_channels, pair[2];
> > +     int ret, i = 0;
> > +
> > +     num_channels = device_get_named_child_node_count(dev, "channel");
> > +     if (!num_channels)
> > +             return dev_err_probe(dev, -EINVAL, "no channel subnodes found\n");
> > +
> > +     if (num_channels > 12)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                                  "num of channel nodes exceeds 12\n");
> > +
> > +     channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> > +     if (!channels)
> > +             return -ENOMEM;
> > +
> > +     device_for_each_named_child_node_scoped(dev, child, "channel") {
> > +             struct iio_chan_spec *spec = &channels[i];
> > +
> > +             if (fwnode_property_present(child, "excitation-channels"))
> > +                     return dev_err_probe(dev, -EOPNOTSUPP,
> > +                                          "excitation-channels is not supported yet\n");
> > +
> > +             if (fwnode_property_present(child, "reference-sources")) {
> > +                     const char *ref;
> > +
> > +                     ret = fwnode_property_read_string(child, "reference-sources", &ref);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read reference-sources\n");
> > +
> > +                     if ((!strcmp(ref, "external") && !st->has_refp) ||
> > +                         (!strcmp(ref, "internal") && st->has_refp))
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "reference-sources does not match refp-supply\n");
>
> As explained more below, this part needs comments to explain why it is
> the way it is since it is not implementing fully what is allowed by the
> devicetree bindings. And would return EOPNOTSUPP to be consistent that
> this is just something that is not implemented yet.
>
> > +             }
> > +
> > +             spec->type = IIO_VOLTAGE;
>
> I would add a REVISIT comment here to explain that when ti,refp-refn-resistor-ohms is implemented,
> then we have the possibility of the channel being resistance instead of votlage.
>
> > +             spec->indexed = 1;
> > +             spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> > +
> > +             if (fwnode_property_present(child, "single-channel")) {
> > +                     ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read single-channel property\n");
> > +
> > +                     if (pair[0] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "single-channel must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->address = 0x08 + pair[0];
> > +             } else if (fwnode_property_present(child, "diff-channels")) {
> > +                     ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read diff-channels property\n");
> > +
> > +                     if (pair[0] > 3 || pair[1] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "diff-channels must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->channel2 = pair[1];
> > +                     spec->differential = 1;
> > +
> > +                     if (pair[0] == 0 && pair[1] == 1)
> > +                             spec->address = 0x00;
> > +                     else if (pair[0] == 0 && pair[1] == 2)
> > +                             spec->address = 0x01;
> > +                     else if (pair[0] == 0 && pair[1] == 3)
> > +                             spec->address = 0x02;
> > +                     else if (pair[0] == 1 && pair[1] == 0)
> > +                             spec->address = 0x03;
> > +                     else if (pair[0] == 1 && pair[1] == 2)
> > +                             spec->address = 0x04;
> > +                     else if (pair[0] == 1 && pair[1] == 3)
> > +                             spec->address = 0x05;
> > +                     else if (pair[0] == 2 && pair[1] == 3)
> > +                             spec->address = 0x06;
> > +                     else if (pair[0] == 3 && pair[1] == 2)
> > +                             spec->address = 0x07;
> > +                     else
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "invalid diff-channels combination\n");
> > +             } else {
> > +                     return dev_err_probe(dev, -EINVAL,
> > +                                          "channel node must have single-channel or diff-channels\n");
> > +             }
> > +
> > +             i++;
> > +     }
> > +
> > +     indio_dev->channels = channels;
> > +     indio_dev->num_channels = i;
> > +
> > +     return 0;
> > +}
> > +
> > +#define ADS112C04_VREF_INTERNAL_MV   2048
>
> Usually we put this at the top with other macro definitions.
>
> > +
> > +static int ads112c04_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct iio_dev *indio_dev;
> > +     struct ads112c04_state *st;
> > +     struct gpio_desc *reset_gpio;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     st = iio_priv(indio_dev);
> > +     st->client = client;
> > +
> > +     ret = devm_mutex_init(dev, &st->lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     init_completion(&st->completion);
> > +
> > +     indio_dev->name = "ads112c04";
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->info = &ads112c04_info;
> > +
> > +     /* Forward compatibility checks for unimplemented DT properties */
> > +     if (device_property_present(dev, "refn-supply") ||
> > +         device_property_present(dev, "ti,refp-refn-resistor-ohms"))
> > +             return dev_err_probe(dev, -EOPNOTSUPP,
> > +                                  "refn-supply and external resistors are not supported yet\n");
> > +
> > +     st->has_refp = device_property_present(dev, "refp-supply");
> > +
> > +     ret = ads112c04_parse_channels(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_regulator_get_enable(dev, "avdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get avdd regulator\n");
> > +
> > +     ret = devm_regulator_get_enable(dev, "dvdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get dvdd regulator\n");
> > +
> > +     if (device_property_present(dev, "refp-supply")) {
>
> This can just be st->has_refp now.
>
> > +             ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "failed to get refp voltage\n");
> > +
> > +             st->vref_mV = ret / (MICRO / MILLI);
> > +             st->config1 = 0x02;
>
> I've asked before... these values should have macros to explain what
> they are and be using FIELD_PREP().
>
> > +     } else {
> > +             st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> > +             st->config1 = 0x00;
> > +     }
>
> I still don't think storing vref like this is a good idea since it should
> be per-channel. If you really are against implementing it in a more
> future-proof way, then it needs lots of comments explaining why it is
> implemented this way instead and how one would go about doing it the "right
> way" in the future.
>
> > +
> > +     /* Power-On Reset (POR) delay */
> > +     fsleep(50 * USEC_PER_MSEC);
> > +
> > +     /* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> > +     reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(reset_gpio))
> > +             return PTR_ERR(reset_gpio);
> > +
> > +     if (reset_gpio) {
> > +             fsleep(1000);
> > +             gpiod_set_value_cansleep(reset_gpio, 0);
> > +     } else {
> > +             ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     fsleep(1000);
> > +
> > +     /* Bypass PGA for now to allow full-scale single-ended measurements */
> > +     st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
> > +     ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (client->irq > 0) {
> > +             ret = devm_request_irq(dev, client->irq,
> > +                                    ads112c04_irq_handler,
> > +                                    0,
>
> I think I've asked twice now to move this 0 on the previous line.
> If you don't agree with that, it is fine, but we just ask that you
> reply to the suggestion with an explanation to state your reasoning.
> Otherwise, it comes across as carelessness that you continue to
> ignore suggestions. This is not the only suggestoin that has been
> silently ignored. You might want to go back to the previous revisions
> and see if you missed anything else.
Hi David,

Thank you for the review, and I'm sorry for the repeated oversights.

You're right, and I have no good excuse. I went back through the v1-v4
threads as you suggested and found several suggestions I silently
dropped:

- Moving the `0` in devm_request_irq() to the previous line
- Organizing register field macros indented under their register
- I even replied at the time saying I would do this, and then
  did not. Apologies.
- Using a macro and FIELD_PREP() for the CONFIG1 VREF value
- Making the reference voltage per-channel
- Using devm_reset_control_get_optional_exclusive() for the hardware
  reset (v1, and Andy raised the same point on v4)
- Your v1 suggestion to consider regmap with a register cache

All of these will be addressed in v5. For the per-channel reference
voltage, I will implement it properly following the ADS112C14 driver
rather than keeping the single stored value.

I'll also fix the IRQ handling - you're right that disabling the IRQ
was wrong, and that the handler needs to be threaded since it can't
call synchronous code. I'll drop disable_irq_nosync() and read the
DRDY status bit before completing instead.

Thanks for your patience with this series.

Best regards,
Kyle Hsieh
>
> > +                                    indio_dev->name, indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return devm_iio_device_register(dev, indio_dev);
> > +}
> > +

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04
  2026-08-11 16:56   ` Conor Dooley
@ 2026-08-12  2:53     ` Kyle Hsieh
  2026-08-12  4:11       ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-12  2:53 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown, linux-iio, devicetree, linux-kernel

Hi Conor,

On Wed, Aug 12, 2026 at 12:56 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Aug 11, 2026 at 10:48:37AM +0800, Kyle Hsieh wrote:
> > Add device tree binding documentation for Texas Instruments ADS112C04
> > I2C Analog-to-Digital Converters.
> >
> > These devices provide 4-channel, 16-bit delta-sigma ADCs with an I2C
> > interface, programmable gain amplifier (PGA), and data-ready (DRDY)
> > interrupt output.
> >
> > The binding uses child nodes to dynamically define the connected
> > single-ended or differential channels.
> >
> > Excitation current, burn-out current, and reference source properties
> > are also included to fully describe the chip's hardware capabilities,
> > aligned with the ti,ads112c14 binding conventions.
> >
> > Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
>
> > +patternProperties:
> > +  "^channel@[0-9a-b]$":
> > +    $ref: adc.yaml
> > +    unevaluatedProperties: false
> > +    properties:
> > +      reg:
> > +        items:
> > +          - maximum: 11
> > +
> > +      single-channel:
> > +        maximum: 3
>
> Can someone explain to me please why the maximum for this is 3 when reg
> goes up to 11? What do the additional reg entries beyond 3 represent?
reg here is just an index for the channel node, not an input pin number.
The chip has 12 usable input MUX settings (8 differential pairs and 4
single-ended inputs), so at most 12 channel nodes can be described,
hence maximum 11.

single-channel refers to the physical analog input pin, and the chip
only has AIN0-AIN3, hence maximum 3.

Best regards,
Kyle Hsieh
>
> > +
> > +      diff-channels:
> > +        items:
> > +          maximum: 3
> > +
> > +      excitation-channels:
> > +        maxItems: 2
> > +        items:
> > +          maximum: 5
> > +        description:
> > +          Additionally to AIN0-AIN3, IDAC1/IDAC2 can also be routed to
> > +          REFP0 (4) and REFN0 (5).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11  9:39   ` Andy Shevchenko
@ 2026-08-12  2:58     ` Kyle Hsieh
  0 siblings, 0 replies; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-12  2:58 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown, linux-iio, devicetree, linux-kernel

Hi Andy,

Thanks for the detailed review.

On Tue, Aug 11, 2026 at 5:39 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Aug 11, 2026 at 10:48:38AM +0800, Kyle Hsieh wrote:
> > Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> > delta-sigma ADCs.
> >
> > The driver implements:
> > - Single-shot conversions using the IIO raw read interface.
> > - Dynamic parsing of single-ended and differential channels from
> >   device tree child nodes.
> > - Hardware interrupt support via the DRDY pin, falling back to
> >   software polling if no IRQ is provided.
> > - Scale calculation based on the internal 2.048V reference.
> > - Reference voltage scaling via the regulator subsystem (refp-supply),
> >   falling back to the internal 2.048V reference if not specified.
> >   refn-supply is not yet supported.
> > - Hardware reset fallback using GPIO.
>
> ...
>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
>
> ...
>
> > +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> > +{
> > +     int ret, err;
> > +     u8 val;
> > +
> > +     if (st->client->irq > 0) {
> > +             /* Timeout is 100ms (slowest data rate is 20 SPS) */
> > +             ret = wait_for_completion_timeout(&st->completion,
> > +                                               msecs_to_jiffies(100));
> > +             if (!ret)
>
> In this case semantics of ret differs, that's why it's better to write as
>
>                 if (!wait_for_completion_timeout(&st->completion, msecs_to_jiffies(100)))
>
> // and I would even dare to put on a single line.
>
> > +                     return -ETIMEDOUT;
> > +
> > +             return 0;
> > +     }
> > +
> > +     err = read_poll_timeout(ads112c04_read_reg, ret,
> > +                             (ret < 0 || (val & ADS112C04_CONFIG2_DRDY)),
> > +                             1000, 100 * USEC_PER_MSEC, false,
> > +                             st->client, ADS112C04_REG_CONFIG2, &val);
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return err;
>
> In this piece I would swap err and ret, so the ret is outer one and err is
> the inner one. This will be consistent with other code pieces.
>
> > +}
>
> ...
>
> > +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> > +                                 struct iio_chan_spec const *chan,
> > +                                 int *val)
> > +{
> > +     u8 new_config0;
> > +     int ret;
> > +
> > +     new_config0 = st->config0;
> > +     FIELD_MODIFY(ADS112C04_CONFIG0_MUX, &new_config0, chan->address);
> > +
> > +     if (st->config0 != new_config0) {
> > +             ret = ads112c04_write_reg(st->client, ADS112C04_REG_CONFIG0, new_config0);
> > +             if (ret < 0)
> > +                     return ret;
> > +             st->config0 = new_config0;
> > +     }
> > +
> > +     reinit_completion(&st->completion);
> > +
> > +     ret = ads112c04_write_cmd(st->client, ADS112C04_CMD_START_SYNC);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = ads112c04_wait_for_data(st);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = ads112c04_read_data(st, val);
>
> > +     if (st->client->irq > 0)
> > +             enable_irq(st->client->irq);
>
> Why is it fine to leave IRQ enabled even in the error case?
>
> > +     return ret;
> > +}
>
> ...
>
> > +static int ads112c04_read_raw(struct iio_dev *indio_dev,
> > +                           struct iio_chan_spec const *chan,
> > +                           int *val, int *val2, long mask)
> > +{
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             mutex_lock(&st->lock);
> > +             ret = ads112c04_get_adc_result(st, chan, val);
> > +             mutex_unlock(&st->lock);
> > +
> > +             if (ret < 0)
> > +                     return ret;
>
> If IRQ is left enabled and we call it here, we end up with the unbalanced
> depth counting.
>
> > +             return IIO_VAL_INT;
> > +
> > +     case IIO_CHAN_INFO_SCALE:
> > +             *val = st->vref_mV;
> > +             *val2 = 15;
> > +             return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
>
> ...
>
> > +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> > +{
> > +     struct iio_dev *indio_dev = private;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
>
> > +     disable_irq_nosync(irq);
>
> This is unconditionally called. Where is the guarantee that it becomes enabled
> once again?
>
> > +     complete(&st->completion);
>
> > +     return IRQ_HANDLED;
> > +}
>
> ...
>
> > +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> > +{
> > +     struct device *dev = indio_dev->dev.parent;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +     struct iio_chan_spec *channels;
> > +     u32 num_channels, pair[2];
>
> > +     int ret, i = 0;
>
> Why is 'i' signed? And it's better to decouple definition and assignment, so
> the assignment will happen closer to when it's really needed.
>
> ...
>
> > +             if (fwnode_property_present(child, "reference-sources")) {
> > +                     const char *ref;
> > +
> > +                     ret = fwnode_property_read_string(child, "reference-sources", &ref);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read reference-sources\n");
> > +
> > +                     if ((!strcmp(ref, "external") && !st->has_refp) ||
> > +                         (!strcmp(ref, "internal") && st->has_refp))
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "reference-sources does not match refp-supply\n");
> > +             }
>
> Reinvention of fwnode_property_match_property_string() ?
>
> ...
>
> > +             if (fwnode_property_present(child, "single-channel")) {
> > +                     ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
>
> I don't like the (partial) pair reuse here. It's semantically wrong. Just add
> another temporary variable and let compiler to choose what to do with a stack
> frame in such a case.
>
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read single-channel property\n");
> > +
> > +                     if (pair[0] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "single-channel must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->address = 0x08 + pair[0];
> > +             } else if (fwnode_property_present(child, "diff-channels")) {
> > +                     ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
>
> ARRAY_SIZE()
>
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read diff-channels property\n");
> > +
> > +                     if (pair[0] > 3 || pair[1] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "diff-channels must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->channel2 = pair[1];
> > +                     spec->differential = 1;
>
> > +                     if (pair[0] == 0 && pair[1] == 1)
> > +                             spec->address = 0x00;
> > +                     else if (pair[0] == 0 && pair[1] == 2)
> > +                             spec->address = 0x01;
> > +                     else if (pair[0] == 0 && pair[1] == 3)
> > +                             spec->address = 0x02;
> > +                     else if (pair[0] == 1 && pair[1] == 0)
> > +                             spec->address = 0x03;
> > +                     else if (pair[0] == 1 && pair[1] == 2)
> > +                             spec->address = 0x04;
> > +                     else if (pair[0] == 1 && pair[1] == 3)
> > +                             spec->address = 0x05;
> > +                     else if (pair[0] == 2 && pair[1] == 3)
> > +                             spec->address = 0x06;
> > +                     else if (pair[0] == 3 && pair[1] == 2)
> > +                             spec->address = 0x07;
>
> I would do this as a 4x4 table
>
>         -1, 0, 1, 2,
>         3, -1, 4, 5,
>         -1, -1, -1, 6,
>         -1, -1, 7, -1,
>
> With that done you can even supported the swapped cases
>
>         -1, 0, 1, 2,
>         3, -1, 4, 5,
>         1, 4, -1, 6,
>         2, 5, 7, -1,
>
> (but I haven't studied the code if it's toughly relies on the pair[0]/pair[1]
>  values to be in a strong order after the address being assigned).
I will apply your first 4x4 table for diff-channels - it maps directly to
the MUX encoding and is much more readable than the if-else chain.

I did not apply the second (swapped) table. Entries like [2][0] would
map to MUX value 1, which is AINP=AIN0/AINN=AIN2, i.e. the opposite
polarity, so it would silently return negated values. Supporting swapped
pairs properly would need a per-channel inversion flag and negation
after conversion, which I'd rather leave out of this initial minimal
implementation.
>
> > +                     else
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "invalid diff-channels combination\n");
>
> > +             } else {
> > +                     return dev_err_probe(dev, -EINVAL,
> > +                                          "channel node must have single-channel or diff-channels\n");
> > +             }
> > +
> > +             i++;
> > +     }
> > +
> > +     indio_dev->channels = channels;
> > +     indio_dev->num_channels = i;
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +#define ADS112C04_VREF_INTERNAL_MV   2048
>
> _mV
>
> ...
>
> > +     if (device_property_present(dev, "refp-supply")) {
>
> A dup property check. if (st->has_refp) should suffice, no?
>
> > +             ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "failed to get refp voltage\n");
> > +
> > +             st->vref_mV = ret / (MICRO / MILLI);
> > +             st->config1 = 0x02;
> > +     } else {
> > +             st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> > +             st->config1 = 0x00;
> > +     }
>
> ...
>
> > +     /* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> > +     reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(reset_gpio))
> > +             return PTR_ERR(reset_gpio);
>
> Why reset-gpio driver can't be used instead?
David suggested the reset controller framework on v1, but after I moved
to gpiod in v2 he suggested GPIOD_OUT_HIGH to simplify it, so I kept
gpiod. Switching now would mean changing the binding, which already has
a Reviewed-by - happy to do it if you both prefer.

Best regards,
Kyle Hsieh
>
> > +     if (reset_gpio) {
> > +             fsleep(1000);
>
> 1 * USEC_PER_MSEC
>
> > +             gpiod_set_value_cansleep(reset_gpio, 0);
> > +     } else {
> > +             ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     fsleep(1000);
>
> Ditto.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11 14:18   ` David Lechner
  2026-08-12  1:25     ` Kyle Hsieh
@ 2026-08-12  3:38     ` Kyle Hsieh
  1 sibling, 0 replies; 13+ messages in thread
From: Kyle Hsieh @ 2026-08-12  3:38 UTC (permalink / raw)
  To: David Lechner
  Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown,
	linux-iio, devicetree, linux-kernel

On Tue, Aug 11, 2026 at 10:18 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 8/10/26 9:48 PM, Kyle Hsieh wrote:
> > Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> > delta-sigma ADCs.
> >
> > The driver implements:
> > - Single-shot conversions using the IIO raw read interface.
> > - Dynamic parsing of single-ended and differential channels from
> >   device tree child nodes.
> > - Hardware interrupt support via the DRDY pin, falling back to
> >   software polling if no IRQ is provided.
> > - Scale calculation based on the internal 2.048V reference.
> > - Reference voltage scaling via the regulator subsystem (refp-supply),
> >   falling back to the internal 2.048V reference if not specified.
> >   refn-supply is not yet supported.
> > - Hardware reset fallback using GPIO.
> >
> > Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
> > ---
> >  MAINTAINERS                    |   1 +
> >  drivers/iio/adc/Kconfig        |  10 +
> >  drivers/iio/adc/Makefile       |   1 +
> >  drivers/iio/adc/ti-ads112c04.c | 425 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 437 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 417d74b6d6cc..f51fbda9d4b9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26992,6 +26992,7 @@ M:    Kyle Hsieh <kylehsieh1995@gmail.com>
> >  L:   linux-iio@vger.kernel.org
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
> > +F:   drivers/iio/adc/ti-ads112c04.c
> >
> >  TI ADS112C14 ADC DRIVER
> >  M:   David Lechner <dlechner@baylibre.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 990e7b3e7212..0ee50828f9e4 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -1817,6 +1817,16 @@ config TI_ADS1119
> >           This driver can also be built as a module. If so, the module will be
> >           called ti-ads1119.
> >
> > +config TI_ADS112C04
> > +    tristate "Texas Instruments ADS112C04 ADC"
> > +    depends on I2C
> > +    help
> > +      If you say yes here you get support for Texas Instruments
> > +      ADS112C04 (16-bit) I2C analog to digital converters.
> > +
> > +      This driver can also be built as a module. If so, the module will be
> > +      called ti-ads112c04.
> > +
> >  config TI_ADS112C14
> >       tristate "Texas Instruments ADS112C14/ADS122C14"
> >       depends on I2C
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index dcec0abb03b7..d8acf2831fd2 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -155,6 +155,7 @@ obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> >  obj-$(CONFIG_TI_ADS1018) += ti-ads1018.o
> >  obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
> >  obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> > +obj-$(CONFIG_TI_ADS112C04) += ti-ads112c04.o
> >  obj-$(CONFIG_TI_ADS112C14) += ti-ads112c14.o
> >  obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> >  obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> > diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> > new file mode 100644
> > index 000000000000..3a775da2ae23
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads112c04.c
> > @@ -0,0 +1,425 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Texas Instruments ADS112C04 16-bit I2C ADC driver
> > + *
> > + * Copyright (c) 2026 Kyle Hsieh <kylehsieh1995@gmail.com>
> > + *
> > + * Datasheet: https://www.ti.com/lit/ds/symlink/ads112c04.pdf
> > + * Based on TI Reference Code and standard Linux IIO framework.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define ADS112C04_CMD_RESET         0x06
> > +#define ADS112C04_CMD_START_SYNC    0x08
> > +#define ADS112C04_CMD_POWERDOWN     0x02
> > +#define ADS112C04_CMD_RDATA         0x10
> > +#define ADS112C04_CMD_RREG(reg)     (0x20 | ((reg) << 2))
> > +#define ADS112C04_CMD_WREG(reg)     (0x40 | ((reg) << 2))
> > +
> > +#define ADS112C04_REG_CONFIG0       0x00
> > +#define ADS112C04_REG_CONFIG1       0x01
> > +#define ADS112C04_REG_CONFIG2       0x02
> > +#define ADS112C04_REG_CONFIG3       0x03
> > +
> > +#define ADS112C04_CONFIG0_MUX         GENMASK(7, 4)
> > +#define ADS112C04_CONFIG0_PGA_BYPASS  BIT(0)
> > +#define ADS112C04_CONFIG2_DRDY        BIT(7)
> > +
>
> I think I mentioned before the preferred way to orgainze the fields
> under the corresponding register. And as below, there are more
> fields we are using that need macros here.
>
> > +struct ads112c04_state {
> > +     struct i2c_client *client;
> > +     /* Protects concurrent ADC reads and device configuration */
> > +     struct mutex lock;
> > +     struct completion completion;
> > +     int vref_mV;
> > +     u8 config0;
> > +     u8 config1;
> > +     bool has_refp;
>
> This isn't used outside of probe. Can just be passed as a function argument.
>
> > +};
> > +
>
> ...
>
> > +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> > +{
> > +     struct iio_dev *indio_dev = private;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +
> > +     disable_irq_nosync(irq);
>
> As mentioned in this recent discussion [1], we should not be disabling the IRQ.
> If there is a true need for it, we need lots of comments explaining why. Since
> we aren't doing buffered reads though, I can't see a reason why we would need
> it right now. Unless we are worried about spurious interrupts, in which case
> we should read the DRDY status bit before setting the completion here as that
> would be more robust.
>
> [1]: https://lore.kernel.org/linux-iio/20260802184126.04738251@jic23-huawei/
>
> Also, this ia a bug because we can't call syncronous code in an IRQ handler.
> We would need to change it to a threaded IRQ handler.
>
> > +     complete(&st->completion);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info ads112c04_info = {
> > +     .read_raw = ads112c04_read_raw,
> > +};
> > +
> > +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> > +{
> > +     struct device *dev = indio_dev->dev.parent;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +     struct iio_chan_spec *channels;
> > +     u32 num_channels, pair[2];
> > +     int ret, i = 0;
> > +
> > +     num_channels = device_get_named_child_node_count(dev, "channel");
> > +     if (!num_channels)
> > +             return dev_err_probe(dev, -EINVAL, "no channel subnodes found\n");
> > +
> > +     if (num_channels > 12)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                                  "num of channel nodes exceeds 12\n");
> > +
> > +     channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> > +     if (!channels)
> > +             return -ENOMEM;
> > +
> > +     device_for_each_named_child_node_scoped(dev, child, "channel") {
> > +             struct iio_chan_spec *spec = &channels[i];
> > +
> > +             if (fwnode_property_present(child, "excitation-channels"))
> > +                     return dev_err_probe(dev, -EOPNOTSUPP,
> > +                                          "excitation-channels is not supported yet\n");
> > +
> > +             if (fwnode_property_present(child, "reference-sources")) {
> > +                     const char *ref;
> > +
> > +                     ret = fwnode_property_read_string(child, "reference-sources", &ref);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read reference-sources\n");
> > +
> > +                     if ((!strcmp(ref, "external") && !st->has_refp) ||
> > +                         (!strcmp(ref, "internal") && st->has_refp))
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "reference-sources does not match refp-supply\n");
>
> As explained more below, this part needs comments to explain why it is
> the way it is since it is not implementing fully what is allowed by the
> devicetree bindings. And would return EOPNOTSUPP to be consistent that
> this is just something that is not implemented yet.
>
> > +             }
> > +
> > +             spec->type = IIO_VOLTAGE;
>
> I would add a REVISIT comment here to explain that when ti,refp-refn-resistor-ohms is implemented,
> then we have the possibility of the channel being resistance instead of votlage.
>
> > +             spec->indexed = 1;
> > +             spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> > +
> > +             if (fwnode_property_present(child, "single-channel")) {
> > +                     ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read single-channel property\n");
> > +
> > +                     if (pair[0] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "single-channel must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->address = 0x08 + pair[0];
> > +             } else if (fwnode_property_present(child, "diff-channels")) {
> > +                     ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read diff-channels property\n");
> > +
> > +                     if (pair[0] > 3 || pair[1] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "diff-channels must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->channel2 = pair[1];
> > +                     spec->differential = 1;
> > +
> > +                     if (pair[0] == 0 && pair[1] == 1)
> > +                             spec->address = 0x00;
> > +                     else if (pair[0] == 0 && pair[1] == 2)
> > +                             spec->address = 0x01;
> > +                     else if (pair[0] == 0 && pair[1] == 3)
> > +                             spec->address = 0x02;
> > +                     else if (pair[0] == 1 && pair[1] == 0)
> > +                             spec->address = 0x03;
> > +                     else if (pair[0] == 1 && pair[1] == 2)
> > +                             spec->address = 0x04;
> > +                     else if (pair[0] == 1 && pair[1] == 3)
> > +                             spec->address = 0x05;
> > +                     else if (pair[0] == 2 && pair[1] == 3)
> > +                             spec->address = 0x06;
> > +                     else if (pair[0] == 3 && pair[1] == 2)
> > +                             spec->address = 0x07;
> > +                     else
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "invalid diff-channels combination\n");
> > +             } else {
> > +                     return dev_err_probe(dev, -EINVAL,
> > +                                          "channel node must have single-channel or diff-channels\n");
> > +             }
> > +
> > +             i++;
> > +     }
> > +
> > +     indio_dev->channels = channels;
> > +     indio_dev->num_channels = i;
> > +
> > +     return 0;
> > +}
> > +
> > +#define ADS112C04_VREF_INTERNAL_MV   2048
>
> Usually we put this at the top with other macro definitions.
>
> > +
> > +static int ads112c04_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct iio_dev *indio_dev;
> > +     struct ads112c04_state *st;
> > +     struct gpio_desc *reset_gpio;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     st = iio_priv(indio_dev);
> > +     st->client = client;
> > +
> > +     ret = devm_mutex_init(dev, &st->lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     init_completion(&st->completion);
> > +
> > +     indio_dev->name = "ads112c04";
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->info = &ads112c04_info;
> > +
> > +     /* Forward compatibility checks for unimplemented DT properties */
> > +     if (device_property_present(dev, "refn-supply") ||
> > +         device_property_present(dev, "ti,refp-refn-resistor-ohms"))
> > +             return dev_err_probe(dev, -EOPNOTSUPP,
> > +                                  "refn-supply and external resistors are not supported yet\n");
> > +
> > +     st->has_refp = device_property_present(dev, "refp-supply");
> > +
> > +     ret = ads112c04_parse_channels(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_regulator_get_enable(dev, "avdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get avdd regulator\n");
> > +
> > +     ret = devm_regulator_get_enable(dev, "dvdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get dvdd regulator\n");
> > +
> > +     if (device_property_present(dev, "refp-supply")) {
>
> This can just be st->has_refp now.
>
> > +             ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "failed to get refp voltage\n");
> > +
> > +             st->vref_mV = ret / (MICRO / MILLI);
> > +             st->config1 = 0x02;
>
> I've asked before... these values should have macros to explain what
> they are and be using FIELD_PREP().
>
> > +     } else {
> > +             st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> > +             st->config1 = 0x00;
> > +     }
>
> I still don't think storing vref like this is a good idea since it should
> be per-channel. If you really are against implementing it in a more
> future-proof way, then it needs lots of comments explaining why it is
> implemented this way instead and how one would go about doing it the "right
> way" in the future.
>
> > +
> > +     /* Power-On Reset (POR) delay */
> > +     fsleep(50 * USEC_PER_MSEC);
> > +
> > +     /* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> > +     reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(reset_gpio))
> > +             return PTR_ERR(reset_gpio);
> > +
> > +     if (reset_gpio) {
> > +             fsleep(1000);
> > +             gpiod_set_value_cansleep(reset_gpio, 0);
> > +     } else {
> > +             ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     fsleep(1000);
> > +
> > +     /* Bypass PGA for now to allow full-scale single-ended measurements */
> > +     st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
> > +     ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (client->irq > 0) {
> > +             ret = devm_request_irq(dev, client->irq,
> > +                                    ads112c04_irq_handler,
> > +                                    0,
>
> I think I've asked twice now to move this 0 on the previous line.
> If you don't agree with that, it is fine, but we just ask that you
> reply to the suggestion with an explanation to state your reasoning.
> Otherwise, it comes across as carelessness that you continue to
> ignore suggestions. This is not the only suggestoin that has been
> silently ignored. You might want to go back to the previous revisions
> and see if you missed anything else.
Clarifications on my previous mail.

IRQ: I will use devm_request_threaded_irq() with a NULL primary handler and
IRQF_ONESHOT. disable_irq_nosync() is gone and the handler only calls
complete(). This also removes the bare `0` you asked about on v2 and v3,
with the arguments packed rather than one per line:

ret = devm_request_threaded_irq(dev, client->irq, NULL,
ads112c04_irq_handler,
IRQF_ONESHOT, indio_dev->name,
indio_dev);

I left out the DRDY status read for now, since conversions are
single-shot under the mutex. Happy to add it if you prefer.
>
> > +                                    indio_dev->name, indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return devm_iio_device_register(dev, indio_dev);
> > +}
> > +

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04
  2026-08-12  2:53     ` Kyle Hsieh
@ 2026-08-12  4:11       ` Jonathan Cameron
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2026-08-12  4:11 UTC (permalink / raw)
  To: Kyle Hsieh
  Cc: Conor Dooley, David Lechner, Nuno Sá, Andy Shevchenko,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
	Mark Brown, linux-iio, devicetree, linux-kernel

On Wed, 12 Aug 2026 10:53:57 +0800
Kyle Hsieh <kylehsieh1995@gmail.com> wrote:

> Hi Conor,
> 
> On Wed, Aug 12, 2026 at 12:56 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Tue, Aug 11, 2026 at 10:48:37AM +0800, Kyle Hsieh wrote:  
> > > Add device tree binding documentation for Texas Instruments ADS112C04
> > > I2C Analog-to-Digital Converters.
> > >
> > > These devices provide 4-channel, 16-bit delta-sigma ADCs with an I2C
> > > interface, programmable gain amplifier (PGA), and data-ready (DRDY)
> > > interrupt output.
> > >
> > > The binding uses child nodes to dynamically define the connected
> > > single-ended or differential channels.
> > >
> > > Excitation current, burn-out current, and reference source properties
> > > are also included to fully describe the chip's hardware capabilities,
> > > aligned with the ti,ads112c14 binding conventions.
> > >
> > > Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>  
> >  
> > > +patternProperties:
> > > +  "^channel@[0-9a-b]$":
> > > +    $ref: adc.yaml
> > > +    unevaluatedProperties: false
> > > +    properties:
> > > +      reg:
> > > +        items:
> > > +          - maximum: 11
> > > +
> > > +      single-channel:
> > > +        maximum: 3  
> >
> > Can someone explain to me please why the maximum for this is 3 when reg
> > goes up to 11? What do the additional reg entries beyond 3 represent?  
> reg here is just an index for the channel node, not an input pin number.
> The chip has 12 usable input MUX settings (8 differential pairs and 4
> single-ended inputs), so at most 12 channel nodes can be described,
> hence maximum 11.
> 
> single-channel refers to the physical analog input pin, and the chip
> only has AIN0-AIN3, hence maximum 3.

Exactly as Kyle described - there is no right way to match reg to a property
when we have mix of single-channel and diff-channels. Hence in this
one corner case reg is just a magic number - though it may reflect
ordering of channel acquisition I guess, though we've never documented
that requirement in a binding.

We could restrict the differential to be only one direction which would
give max 10 channels (4 single then 0-1, 0-2, 0-3, 1-2, 1-3, 2-3)
but there isn't a huge advantage in doing so even though it is trivial
for software to flip the sign if other direction is wanted.

Note that we don't actually expect to see all the channels configured,
it can just be a useful to do things like single ended voltage at
top of a resistor and differential across the resistor (to get
allow current to be computed) - so it's not unheard to to mix
and match.

Jonathan

> 
> Best regards,
> Kyle Hsieh
> >  
> > > +
> > > +      diff-channels:
> > > +        items:
> > > +          maximum: 3
> > > +
> > > +      excitation-channels:
> > > +        maxItems: 2
> > > +        items:
> > > +          maximum: 5
> > > +        description:
> > > +          Additionally to AIN0-AIN3, IDAC1/IDAC2 can also be routed to
> > > +          REFP0 (4) and REFN0 (5).  


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
  2026-08-11  2:48 ` [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
  2026-08-11  9:39   ` Andy Shevchenko
  2026-08-11 14:18   ` David Lechner
@ 2026-08-12  4:29   ` Jonathan Cameron
  2 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2026-08-12  4:29 UTC (permalink / raw)
  To: Kyle Hsieh
  Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Liam Girdwood, Mark Brown,
	linux-iio, devicetree, linux-kernel

On Tue, 11 Aug 2026 10:48:38 +0800
Kyle Hsieh <kylehsieh1995@gmail.com> wrote:

> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> delta-sigma ADCs.
> 
> The driver implements:
> - Single-shot conversions using the IIO raw read interface.
> - Dynamic parsing of single-ended and differential channels from
>   device tree child nodes.
> - Hardware interrupt support via the DRDY pin, falling back to
>   software polling if no IRQ is provided.
> - Scale calculation based on the internal 2.048V reference.
> - Reference voltage scaling via the regulator subsystem (refp-supply),
>   falling back to the internal 2.048V reference if not specified.
>   refn-supply is not yet supported.
> - Hardware reset fallback using GPIO.
> 
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
Hi Kyle,

Clearly you got plenty of good feedback already from Andy and David.
As such (and given I have 400 IIO messages unread right now :()
I only took a quick look and called out a few things that perhaps others have not
already raised.

Jonathan

> +
> +#define ADS112C04_VREF_INTERNAL_MV	2048

Probably better up top with the other defines.

> +
> +static int ads112c04_probe(struct i2c_client *client)
> +{

...

> +
> +	/* Bypass PGA for now to allow full-scale single-ended measurements */
Given there are several fields in each of these registers, it would be
nicer to fully fill it here (even though they all end up as zeros)

	st->config0 = ADS112C04_CONFIG0_PGA_BYPASS |
		      FIELD_PUT(ADS112C04_CONFIG0_GAIN, ADS112C04_CONFIG0_GAIN_X1) |
		      FIELD_PUT(ADS112C04_CONFIG0_MUX, ADS112C04_CONFIG0_MUX_AIN0_AIN1);

	st->config1 = FIELD_PUT(ADIS112C04_CONFIG1_TEMP_EN, 0) |
		...
		      FIELD_PUT(ADIS112C04_CONFIG1_CONVMODE, ADIS112C04_CONFIG1_CONVMODE_SINGLE)
etc
Long lines so maybe shorten CONFIG to CONF or CFG to help with that.

The aim of this explicit form is to make it obvious what the defaults actually mean.
That is lost if you write a 0 as done to config1.

> +	st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
> +	if (ret)
> +		return ret;

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-08-12  4:30 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-11  2:48 [PATCH v4 0/2] iio: adc: Add support for Texas Instruments ADS112C04 Kyle Hsieh
2026-08-11  2:48 ` [PATCH v4 1/2] dt-bindings: iio: adc: ti,ads112c04: Add binding for ADS112C04 Kyle Hsieh
2026-08-11 14:23   ` David Lechner
2026-08-11 16:56   ` Conor Dooley
2026-08-12  2:53     ` Kyle Hsieh
2026-08-12  4:11       ` Jonathan Cameron
2026-08-11  2:48 ` [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
2026-08-11  9:39   ` Andy Shevchenko
2026-08-12  2:58     ` Kyle Hsieh
2026-08-11 14:18   ` David Lechner
2026-08-12  1:25     ` Kyle Hsieh
2026-08-12  3:38     ` Kyle Hsieh
2026-08-12  4:29   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox