[RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Ajith Anandhan]

This RFC patch series adds support for the Texas Instruments ADS1120,
a precision 16-bit delta-sigma ADC with SPI interface.

The driver provides:
- 4 single-ended voltage input channels
- Programmable gain amplifier (1 to 128)
- Configurable data rates (20 to 1000 SPS)
- Single-shot conversion mode

I'm looking for feedback on:
1. The implementation approach for single-shot conversions
2. Any other suggestions for improvement

Datasheet: https://www.ti.com/lit/gpn/ads1120

Ajith Anandhan (3):
  dt-bindings: iio: adc: Add TI ADS1120 binding
  iio: adc: Add support for TI ADS1120
  MAINTAINERS: Add entry for TI ADS1120 ADC driver

 .../bindings/iio/adc/ti,ads1120.yaml          |  50 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ti-ads1120.c                  | 509 ++++++++++++++++++
 5 files changed, 577 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
 create mode 100644 drivers/iio/adc/ti-ads1120.c

-- 
2.34.1

[RFC PATCH 1/3] dt-bindings: iio: adc: Add TI ADS1120 binding

[Ajith Anandhan]

Add device tree binding documentation for the Texas Instruments
ADS1120.

The binding defines required properties like compatible, reg, and
SPI configuration parameters.

Link: https://www.ti.com/lit/gpn/ads1120
Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
---
 .../bindings/iio/adc/ti,ads1120.yaml          | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
new file mode 100644
index 000000000..09285c981
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1120.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ADS1120 4-channel, 16-bit, 2kSPS ADC
+
+maintainers:
+  - Ajith Anandhan <ajithanandhan0406@gmail.com>
+
+properties:
+  compatible:
+    const: ti,ads1120
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 4000000
+
+  spi-cpha: true
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "ti,ads1120";
+            reg = <0>;
+            spi-max-frequency = <4000000>;
+            spi-cpha;
+        };
+    };
+...
+
-- 
2.34.1

Re: [RFC PATCH 1/3] dt-bindings: iio: adc: Add TI ADS1120 binding

[Jonathan Cameron]

On Thu, 30 Oct 2025 22:04:09 +0530
Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:

> Add device tree binding documentation for the Texas Instruments
> ADS1120.
> 
> The binding defines required properties like compatible, reg, and
> SPI configuration parameters.
> 
> Link: https://www.ti.com/lit/gpn/ads1120
Datasheet: https://www.ti.com/lit/gpn/ads1120

Is a somewhat official tag for these. Though better to put it in the dt-binding
doc itself as well or instead of here.

> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
> ---
>  .../bindings/iio/adc/ti,ads1120.yaml          | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
> new file mode 100644
> index 000000000..09285c981
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1120.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments ADS1120 4-channel, 16-bit, 2kSPS ADC
> +
> +maintainers:
> +  - Ajith Anandhan <ajithanandhan0406@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: ti,ads1120
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 4000000
> +
> +  spi-cpha: true
> +
> +  "#io-channel-cells":
> +    const: 1

Power supplies should be here and required (even if real boards
rely on stub regulators). 

Looks like there is an optional reference as well - so include that
but not as required (use internal ref if not supplied).

There is a data ready pin as well so I'd expect an interrupt.

All these should be in the binding from the start as we want it
to be as complete as possible.  The driver doesn't have to use everything
the binding supplies.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "ti,ads1120";
> +            reg = <0>;
> +            spi-max-frequency = <4000000>;
> +            spi-cpha;
> +        };
> +    };
> +...
> +

Re: [RFC PATCH 1/3] dt-bindings: iio: adc: Add TI ADS1120 binding

[David Lechner]

On 10/30/25 12:12 PM, Jonathan Cameron wrote:
> On Thu, 30 Oct 2025 22:04:09 +0530
> Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:
> 
>> Add device tree binding documentation for the Texas Instruments
>> ADS1120.
>>
>> The binding defines required properties like compatible, reg, and
>> SPI configuration parameters.
>>
>> Link: https://www.ti.com/lit/gpn/ads1120
> Datasheet: https://www.ti.com/lit/gpn/ads1120
> 
> Is a somewhat official tag for these. Though better to put it in the dt-binding
> doc itself as well or instead of here.
> 
>> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
>> ---
>>  .../bindings/iio/adc/ti,ads1120.yaml          | 50 +++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>> new file mode 100644
>> index 000000000..09285c981
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1120.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments ADS1120 4-channel, 16-bit, 2kSPS ADC
>> +
>> +maintainers:
>> +  - Ajith Anandhan <ajithanandhan0406@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,ads1120
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  spi-max-frequency:
>> +    maximum: 4000000
>> +
>> +  spi-cpha: true
>> +
>> +  "#io-channel-cells":
>> +    const: 1
> 
> Power supplies should be here and required (even if real boards
> rely on stub regulators). 
> 
> Looks like there is an optional reference as well - so include that
> but not as required (use internal ref if not supplied).

Actually, there are two. REF{P,N}1 is an alternative function of
the AIN{0,3} pins.

It is also possible that the analog power supply can be used as
a reference source instead of the internal one.

This came up recently and we glossed over it. However, I think
it would make sense to have a flag property that means "the
AVSS supply is of sufficient quality that it is better than 
the internal reference supply", e.g. ti,avdss-is-ref. And
drivers can use this info to decide if they want to select it
as the reference voltage or not.

> 
> There is a data ready pin as well so I'd expect an interrupt.

There is actually two DRDY pins. One is shared with DOUT, so we
should have two interrupts and interrupt-names so we know which
pin is actually wired up.

> 
> All these should be in the binding from the start as we want it
> to be as complete as possible.  The driver doesn't have to use everything
> the binding supplies.
> 
Another trivial one is an optional clocks property for the external
clock. It doesn't need clock-names since there is only one.

Additional bindings needed when this is used with temperature sensors
are not so trivial though, so we don't need to add those until someone
actually needs them.

Re: [RFC PATCH 1/3] dt-bindings: iio: adc: Add TI ADS1120 binding

[Ajith Anandhan]

On 10/31/25 1:34 AM, David Lechner wrote:
> On 10/30/25 12:12 PM, Jonathan Cameron wrote:
>> On Thu, 30 Oct 2025 22:04:09 +0530
>> Ajith Anandhan<ajithanandhan0406@gmail.com> wrote:
>>
>>> Add device tree binding documentation for the Texas Instruments
>>> ADS1120.
>>>
>>> The binding defines required properties like compatible, reg, and
>>> SPI configuration parameters.
>>>
>>> Link:https://www.ti.com/lit/gpn/ads1120
>> Datasheet:https://www.ti.com/lit/gpn/ads1120
>>
>> Is a somewhat official tag for these. Though better to put it in the dt-binding
>> doc itself as well or instead of here.
>>
>>> Signed-off-by: Ajith Anandhan<ajithanandhan0406@gmail.com>
>>> ---
>>>   .../bindings/iio/adc/ti,ads1120.yaml          | 50 +++++++++++++++++++
>>>   1 file changed, 50 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>>> new file mode 100644
>>> index 000000000..09285c981
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>>> @@ -0,0 +1,50 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id:http://devicetree.org/schemas/iio/adc/ti,ads1120.yaml#
>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Texas Instruments ADS1120 4-channel, 16-bit, 2kSPS ADC
>>> +
>>> +maintainers:
>>> +  - Ajith Anandhan<ajithanandhan0406@gmail.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ti,ads1120
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  spi-max-frequency:
>>> +    maximum: 4000000
>>> +
>>> +  spi-cpha: true
>>> +
>>> +  "#io-channel-cells":
>>> +    const: 1
>> Power supplies should be here and required (even if real boards
>> rely on stub regulators).
>>
>> Looks like there is an optional reference as well - so include that
>> but not as required (use internal ref if not supplied).
> Actually, there are two. REF{P,N}1 is an alternative function of
> the AIN{0,3} pins.

I'll add avdd-supply as required and vref-supply as optional to support 
both external reference configurations (REFP0/REFN0 and REFP1/REFN1).

>
> It is also possible that the analog power supply can be used as
> a reference source instead of the internal one.
>
> This came up recently and we glossed over it. However, I think
> it would make sense to have a flag property that means "the
> AVSS supply is of sufficient quality that it is better than
> the internal reference supply", e.g. ti,avdss-is-ref. And
> drivers can use this info to decide if they want to select it
> as the reference voltage or not.

I'll add the ti,avdss-is-ref boolean property to let the driver know 
when AVDD is suitable as a reference source.

>
>> There is a data ready pin as well so I'd expect an interrupt.
> There is actually two DRDY pins. One is shared with DOUT, so we
> should have two interrupts and interrupt-names so we know which
> pin is actually wired up.
Thanks for the clarification. I'll add support for up to two interrupts 
with interrupt-names ("drdy" and "dout") to handle both pin configurations.
>
>> All these should be in the binding from the start as we want it
>> to be as complete as possible.  The driver doesn't have to use everything
>> the binding supplies.
>>
> Another trivial one is an optional clocks property for the external
> clock. It doesn't need clock-names since there is only one.
Will add the optional clocks property for external clock support.
>
> Additional bindings needed when this is used with temperature sensors
> are not so trivial though, so we don't need to add those until someone
> actually needs them.

I'll incorporate all these changes in v2. Thank you both for the 
thorough review!


-- 
Best Regards,
Ajith Anandhan.

Re: [RFC PATCH 1/3] dt-bindings: iio: adc: Add TI ADS1120 binding

[Ajith Anandhan]

On 10/30/25 10:42 PM, Jonathan Cameron wrote:
> On Thu, 30 Oct 2025 22:04:09 +0530
> Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:
>
>> Add device tree binding documentation for the Texas Instruments
>> ADS1120.
>>
>> The binding defines required properties like compatible, reg, and
>> SPI configuration parameters.
>>
>> Link: https://www.ti.com/lit/gpn/ads1120
> Datasheet: https://www.ti.com/lit/gpn/ads1120
>
> Is a somewhat official tag for these. Though better to put it in the dt-binding
> doc itself as well or instead of here.
>
>> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
>> ---
>>   .../bindings/iio/adc/ti,ads1120.yaml          | 50 +++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>> new file mode 100644
>> index 000000000..09285c981
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1120.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments ADS1120 4-channel, 16-bit, 2kSPS ADC
>> +
>> +maintainers:
>> +  - Ajith Anandhan <ajithanandhan0406@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: ti,ads1120
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  spi-max-frequency:
>> +    maximum: 4000000
>> +
>> +  spi-cpha: true
>> +
>> +  "#io-channel-cells":
>> +    const: 1
> Power supplies should be here and required (even if real boards
> rely on stub regulators).
>
> Looks like there is an optional reference as well - so include that
> but not as required (use internal ref if not supplied).
>
> There is a data ready pin as well so I'd expect an interrupt.
>
> All these should be in the binding from the start as we want it
> to be as complete as possible.  The driver doesn't have to use everything
> the binding supplies.
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +allOf:
>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    spi {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        adc@0 {
>> +            compatible = "ti,ads1120";
>> +            reg = <0>;
>> +            spi-max-frequency = <4000000>;
>> +            spi-cpha;
>> +        };
>> +    };
>> +...
>> +

Hi Jonathan,

Thank you for the detailed review and helpful suggestions.

I'll update the binding to include the following:
- Add required power supply property (avdd-supply) and optional vref-supply.
- Add interrupt and interrupt-names for DRDY and DOUT.
- Move the datasheet link into the dt-binding description.

I'll incorporate these changes and resend as a V2 (non-RFC) patch series.

-- 
BR,
Ajith Anandhan.

[RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[Ajith Anandhan]

Add driver for the Texas Instruments ADS1120, a precision 16-bit
analog-to-digital converter with an SPI interface.

The driver provides:
- 4 single-ended voltage input channels
- Programmable gain amplifier (1 to 128)
- Configurable data rates (20 to 1000 SPS)
- Single-shot conversion mode

Link: https://www.ti.com/lit/gpn/ads1120
Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
---
 drivers/iio/adc/Kconfig      |  10 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-ads1120.c | 509 +++++++++++++++++++++++++++++++++++
 3 files changed, 520 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads1120.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 58a14e683..afb7895dd 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1655,6 +1655,16 @@ config TI_ADS1119
          This driver can also be built as a module. If so, the module will be
          called ti-ads1119.
 
+config TI_ADS1120
+	tristate "Texas Instruments ADS1120"
+	depends on SPI
+	help
+	  Say yes here to build support for Texas Instruments ADS1120
+	  4-channel, 2kSPS, 16-bit delta-sigma ADC.
+
+	  This driver can also be built as module. If so, the module
+	  will be called ti-ads1120.
+
 config TI_ADS124S08
 	tristate "Texas Instruments ADS124S08"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d008f78dc..49c56b459 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
 obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
 obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
+obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
 obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
 obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
 obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
new file mode 100644
index 000000000..07a6fb309
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1120.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI ADS1120 4-channel, 2kSPS, 16-bit delta-sigma ADC
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/ads1120
+ *
+ * Copyright (C) 2025 Ajith Anandhan <ajithanandhan0406@gmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+
+/* Commands */
+#define ADS1120_CMD_RESET		0x06
+#define ADS1120_CMD_START		0x08
+#define ADS1120_CMD_PWRDWN		0x02
+#define ADS1120_CMD_RDATA		0x10
+#define ADS1120_CMD_RREG		0x20
+#define ADS1120_CMD_WREG		0x40
+
+/* Registers */
+#define ADS1120_REG_CONFIG0		0x00
+#define ADS1120_REG_CONFIG1		0x01
+#define ADS1120_REG_CONFIG2		0x02
+#define ADS1120_REG_CONFIG3		0x03
+
+/* Config Register 0 bit definitions */
+#define ADS1120_MUX_MASK		GENMASK(7, 4)
+#define ADS1120_MUX_AIN0_AVSS		0x80
+#define ADS1120_MUX_AIN1_AVSS		0x90
+#define ADS1120_MUX_AIN2_AVSS		0xa0
+#define ADS1120_MUX_AIN3_AVSS		0xb0
+
+#define ADS1120_GAIN_MASK		GENMASK(3, 1)
+#define ADS1120_GAIN_1			0x00
+#define ADS1120_GAIN_2			0x02
+#define ADS1120_GAIN_4			0x04
+#define ADS1120_GAIN_8			0x06
+#define ADS1120_GAIN_16			0x08
+#define ADS1120_GAIN_32			0x0a
+#define ADS1120_GAIN_64			0x0c
+#define ADS1120_GAIN_128		0x0e
+
+#define ADS1120_PGA_BYPASS		BIT(0)
+
+/* Config Register 1 bit definitions */
+#define ADS1120_DR_MASK			GENMASK(7, 5)
+#define ADS1120_DR_20SPS		0x00
+#define ADS1120_DR_45SPS		0x20
+#define ADS1120_DR_90SPS		0x40
+#define ADS1120_DR_175SPS		0x60
+#define ADS1120_DR_330SPS		0x80
+#define ADS1120_DR_600SPS		0xa0
+#define ADS1120_DR_1000SPS		0xc0
+
+#define ADS1120_MODE_MASK		GENMASK(4, 3)
+#define ADS1120_MODE_NORMAL		0x00
+
+#define ADS1120_CM_SINGLE		0x00
+#define ADS1120_CM_CONTINUOUS		BIT(2)
+
+#define ADS1120_TS_EN			BIT(1)
+#define ADS1120_BCS_EN			BIT(0)
+
+/* Config Register 2 bit definitions */
+#define ADS1120_VREF_MASK		GENMASK(7, 6)
+#define ADS1120_VREF_INTERNAL		0x00
+#define ADS1120_VREF_EXT_REFP0		0x40
+#define ADS1120_VREF_EXT_AIN0		0x80
+#define ADS1120_VREF_AVDD		0xc0
+
+#define ADS1120_REJECT_MASK		GENMASK(5, 4)
+#define ADS1120_REJECT_OFF		0x00
+#define ADS1120_REJECT_50_60		0x10
+#define ADS1120_REJECT_50		0x20
+#define ADS1120_REJECT_60		0x30
+
+#define ADS1120_PSW_EN			BIT(3)
+
+#define ADS1120_IDAC_MASK		GENMASK(2, 0)
+
+/* Config Register 3 bit definitions */
+#define ADS1120_IDAC1_MASK		GENMASK(7, 5)
+#define ADS1120_IDAC2_MASK		GENMASK(4, 2)
+#define ADS1120_DRDYM_EN		BIT(1)
+
+/* Default configuration values */
+#define ADS1120_DEFAULT_GAIN		1
+#define ADS1120_DEFAULT_DATARATE	175
+
+struct ads1120_state {
+	struct spi_device	*spi;
+	struct mutex		lock;
+	/*
+	 * Used to correctly align data.
+	 * Ensure natural alignment for potential future timestamp support.
+	 */
+	u8 data[4] __aligned(IIO_DMA_MINALIGN);
+
+	u8 config[4];
+	int current_channel;
+	int gain;
+	int datarate;
+	int conv_time_ms;
+};
+
+struct ads1120_datarate {
+	int rate;
+	int conv_time_ms;
+	u8 reg_value;
+};
+
+static const struct ads1120_datarate ads1120_datarates[] = {
+	{ 20,   51, ADS1120_DR_20SPS },
+	{ 45,   24, ADS1120_DR_45SPS },
+	{ 90,   13, ADS1120_DR_90SPS },
+	{ 175,   7, ADS1120_DR_175SPS },
+	{ 330,   4, ADS1120_DR_330SPS },
+	{ 600,   3, ADS1120_DR_600SPS },
+	{ 1000,  2, ADS1120_DR_1000SPS },
+};
+
+static const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
+
+#define ADS1120_CHANNEL(index)					\
+{								\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = index,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec ads1120_channels[] = {
+	ADS1120_CHANNEL(0),
+	ADS1120_CHANNEL(1),
+	ADS1120_CHANNEL(2),
+	ADS1120_CHANNEL(3),
+};
+
+static int ads1120_write_cmd(struct ads1120_state *st, u8 cmd)
+{
+	st->data[0] = cmd;
+	return spi_write(st->spi, st->data, 1);
+}
+
+static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
+{
+	u8 buf[2];
+
+	if (reg > ADS1120_REG_CONFIG3)
+		return -EINVAL;
+
+	buf[0] = ADS1120_CMD_WREG | (reg << 2);
+	buf[1] = value;
+
+	return spi_write(st->spi, buf, 2);
+}
+
+static int ads1120_reset(struct ads1120_state *st)
+{
+	int ret;
+
+	ret = ads1120_write_cmd(st, ADS1120_CMD_RESET);
+	if (ret)
+		return ret;
+
+	/*
+	 * Datasheet specifies reset takes 50us + 32 * t(CLK)
+	 * where t(CLK) = 1/4.096MHz. Use 200us to be safe.
+	 */
+	usleep_range(200, 300);
+
+	return 0;
+}
+
+static int ads1120_set_channel(struct ads1120_state *st, int channel)
+{
+	u8 mux_val;
+	u8 config0;
+
+	if (channel < 0 || channel > 3)
+		return -EINVAL;
+
+	/* Map channel to AINx/AVSS single-ended input */
+	mux_val = ADS1120_MUX_AIN0_AVSS + (channel << 4);
+
+	config0 = (st->config[0] & ~ADS1120_MUX_MASK) | mux_val;
+	st->config[0] = config0;
+
+	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
+}
+
+static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
+{
+	u8 gain_bits;
+	u8 config0;
+	int i;
+
+	/* Find gain in supported values */
+	for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
+		if (ads1120_gain_values[i] == gain_val) {
+			gain_bits = i << 1;
+			goto found;
+		}
+	}
+	return -EINVAL;
+
+found:
+	config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
+	st->config[0] = config0;
+	st->gain = gain_val;
+
+	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
+}
+
+static int ads1120_set_datarate(struct ads1120_state *st, int rate)
+{
+	u8 config1;
+	int i;
+
+	/* Find data rate in supported values */
+	for (i = 0; i < ARRAY_SIZE(ads1120_datarates); i++) {
+		if (ads1120_datarates[i].rate == rate) {
+			config1 = (st->config[1] & ~ADS1120_DR_MASK) |
+				  ads1120_datarates[i].reg_value;
+			st->config[1] = config1;
+			st->datarate = rate;
+			st->conv_time_ms = ads1120_datarates[i].conv_time_ms;
+
+			return ads1120_write_reg(st, ADS1120_REG_CONFIG1,
+						 config1);
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int ads1120_read_raw_adc(struct ads1120_state *st, int *val)
+{
+	u8 rx_buf[4];
+	u8 tx_buf[4] = { ADS1120_CMD_RDATA, 0xff, 0xff, 0xff };
+	int ret;
+	struct spi_transfer xfer = {
+		.tx_buf = tx_buf,
+		.rx_buf = rx_buf,
+		.len = 4,
+	};
+
+	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	if (ret)
+		return ret;
+
+	/*
+	 * Data format: 16-bit two's complement MSB first
+	 * rx_buf[0] is echo of command
+	 * rx_buf[1] is MSB of data
+	 * rx_buf[2] is LSB of data
+	 */
+	*val = (s16)((rx_buf[1] << 8) | rx_buf[2]);
+
+	return 0;
+}
+
+static int ads1120_read_measurement(struct ads1120_state *st, int channel,
+				    int *val)
+{
+	int ret;
+
+	ret = ads1120_set_channel(st, channel);
+	if (ret)
+		return ret;
+
+	/* Start single-shot conversion */
+	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
+	if (ret)
+		return ret;
+
+	/* Wait for conversion to complete */
+	msleep(st->conv_time_ms);
+
+	/* Read the result */
+	ret = ads1120_read_raw_adc(st, val);
+	if (ret)
+		return ret;
+
+	st->current_channel = channel;
+
+	return 0;
+}
+
+static int ads1120_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct ads1120_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+		ret = ads1120_read_measurement(st, chan->channel, val);
+		mutex_unlock(&st->lock);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->gain;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->datarate;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ads1120_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct ads1120_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		mutex_lock(&st->lock);
+		ret = ads1120_set_gain(st, val);
+		mutex_unlock(&st->lock);
+		return ret;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&st->lock);
+		ret = ads1120_set_datarate(st, val);
+		mutex_unlock(&st->lock);
+		return ret;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ads1120_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	static const int datarates[] = { 20, 45, 90, 175, 330, 600, 1000 };
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = ads1120_gain_values;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(ads1120_gain_values);
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = datarates;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(datarates);
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ads1120_info = {
+	.read_raw = ads1120_read_raw,
+	.write_raw = ads1120_write_raw,
+	.read_avail = ads1120_read_avail,
+};
+
+static int ads1120_init(struct ads1120_state *st)
+{
+	int ret;
+
+	/* Reset device to default state */
+	ret = ads1120_reset(st);
+	if (ret) {
+		dev_err(&st->spi->dev, "Failed to reset device\n");
+		return ret;
+	}
+
+	/*
+	 * Configure Register 0:
+	 * - Input MUX: AIN0/AVSS (updated per channel read)
+	 * - Gain: 1
+	 * - PGA bypassed (lower power consumption)
+	 */
+	st->config[0] = ADS1120_MUX_AIN0_AVSS | ADS1120_GAIN_1 |
+			ADS1120_PGA_BYPASS;
+	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG0, st->config[0]);
+	if (ret)
+		return ret;
+
+	/*
+	 * Configure Register 1:
+	 * - Data rate: 175 SPS
+	 * - Mode: Normal
+	 * - Conversion mode: Single-shot
+	 * - Temperature sensor: Disabled
+	 * - Burnout current: Disabled
+	 */
+	st->config[1] = ADS1120_DR_175SPS | ADS1120_MODE_NORMAL |
+			ADS1120_CM_SINGLE;
+	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG1, st->config[1]);
+	if (ret)
+		return ret;
+
+	/*
+	 * Configure Register 2:
+	 * - Voltage reference: AVDD
+	 * - 50/60Hz rejection: Off
+	 * - Power switch: Off
+	 * - IDAC: Off
+	 */
+	st->config[2] = ADS1120_VREF_AVDD | ADS1120_REJECT_OFF;
+	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
+	if (ret)
+		return ret;
+
+	/*
+	 * Configure Register 3:
+	 * - IDAC1: Disabled
+	 * - IDAC2: Disabled
+	 * - DRDY mode: DOUT/DRDY pin behavior
+	 */
+	st->config[3] = ADS1120_DRDYM_EN;
+	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG3, st->config[3]);
+	if (ret)
+		return ret;
+
+	/* Set default operating parameters */
+	st->gain = ADS1120_DEFAULT_GAIN;
+	st->datarate = ADS1120_DEFAULT_DATARATE;
+	st->conv_time_ms = 7; /* For 175 SPS */
+	st->current_channel = -1;
+
+	return 0;
+}
+
+static int ads1120_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct ads1120_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	mutex_init(&st->lock);
+
+	indio_dev->name = "ads1120";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ads1120_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
+	indio_dev->info = &ads1120_info;
+
+	ret = ads1120_init(st);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to initialize device: %d\n", ret);
+		return ret;
+	}
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ads1120_of_match[] = {
+	{ .compatible = "ti,ads1120" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ads1120_of_match);
+
+static const struct spi_device_id ads1120_id[] = {
+	{ "ads1120" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ads1120_id);
+
+static struct spi_driver ads1120_driver = {
+	.driver = {
+		.name = "ads1120",
+		.of_match_table = ads1120_of_match,
+	},
+	.probe = ads1120_probe,
+	.id_table = ads1120_id,
+};
+module_spi_driver(ads1120_driver);
+
+MODULE_AUTHOR("Ajith Anandhan  <ajithanandhan0406@gmail.com>");
+MODULE_DESCRIPTION("Texas Instruments ADS1120 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[Jonathan Cameron]

On Thu, 30 Oct 2025 22:04:10 +0530
Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:

> Add driver for the Texas Instruments ADS1120, a precision 16-bit
> analog-to-digital converter with an SPI interface.
> 
> The driver provides:
> - 4 single-ended voltage input channels
> - Programmable gain amplifier (1 to 128)
> - Configurable data rates (20 to 1000 SPS)
> - Single-shot conversion mode
> 
> Link: https://www.ti.com/lit/gpn/ads1120
Datasheet:

> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>

Hi Ajith

Various comments inline.  Mostly superficial stuff but the DMA safety
of SPI buffers needs fixing.  There is a useful talk from this given
by Wolfram Sang if you want to understand more about this
https://www.youtube.com/watch?v=JDwaMClvV-s

Thanks,

Jonathan


> diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
> new file mode 100644
> index 000000000..07a6fb309
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1120.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TI ADS1120 4-channel, 2kSPS, 16-bit delta-sigma ADC
> + *
> + * Datasheet: https://www.ti.com/lit/gpn/ads1120
> + *
> + * Copyright (C) 2025 Ajith Anandhan <ajithanandhan0406@gmail.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>

Figure out what you are actually using. There is ongoing effort to not include
kernel.h in drivers because there is usually a small set of more appropriate
fine grained includes.

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Commands */
> +#define ADS1120_CMD_RESET		0x06
> +#define ADS1120_CMD_START		0x08
> +#define ADS1120_CMD_PWRDWN		0x02
> +#define ADS1120_CMD_RDATA		0x10
> +#define ADS1120_CMD_RREG		0x20
> +#define ADS1120_CMD_WREG		0x40
> +
> +/* Registers */
> +#define ADS1120_REG_CONFIG0		0x00
> +#define ADS1120_REG_CONFIG1		0x01
> +#define ADS1120_REG_CONFIG2		0x02
> +#define ADS1120_REG_CONFIG3		0x03
> +
> +/* Config Register 0 bit definitions */
> +#define ADS1120_MUX_MASK		GENMASK(7, 4)
> +#define ADS1120_MUX_AIN0_AVSS		0x80
> +#define ADS1120_MUX_AIN1_AVSS		0x90
> +#define ADS1120_MUX_AIN2_AVSS		0xa0
> +#define ADS1120_MUX_AIN3_AVSS		0xb0
> +
> +#define ADS1120_GAIN_MASK		GENMASK(3, 1)

Better to name these so it's obvious which register they are in.
#define ADS1120_CFG0_GAIN_MSK or similar.
Saves anyone checking every time that it's being written to
the appropriate register.

> +#define ADS1120_GAIN_1			0x00
> +#define ADS1120_GAIN_2			0x02
> +#define ADS1120_GAIN_4			0x04
> +#define ADS1120_GAIN_8			0x06
> +#define ADS1120_GAIN_16			0x08
> +#define ADS1120_GAIN_32			0x0a
> +#define ADS1120_GAIN_64			0x0c
> +#define ADS1120_GAIN_128		0x0e
Same as called out for DR below. (applies in other places
as well).
> +
> +#define ADS1120_PGA_BYPASS		BIT(0)
> +
> +/* Config Register 1 bit definitions */
> +#define ADS1120_DR_MASK			GENMASK(7, 5)
> +#define ADS1120_DR_20SPS		0x00
> +#define ADS1120_DR_45SPS		0x20
> +#define ADS1120_DR_90SPS		0x40
> +#define ADS1120_DR_175SPS		0x60
> +#define ADS1120_DR_330SPS		0x80
> +#define ADS1120_DR_600SPS		0xa0
> +#define ADS1120_DR_1000SPS		0xc0
Define these as values of the field (So not shifted)

#define ADS1120_DR_20SPS	0
#define ADS1120_DR_45SPS	1
etc.
Then use FIELD_PREP(ADS1120_DR_MASK, ADIS1120_DR_20SPS)
and similar to set them.

> +
> +#define ADS1120_MODE_MASK		GENMASK(4, 3)
> +#define ADS1120_MODE_NORMAL		0x00
No other values for a 2 bit field? Define all values even
if you only use one for now.  Makes it easier to review the
values
> +
> +#define ADS1120_CM_SINGLE		0x00
> +#define ADS1120_CM_CONTINUOUS		BIT(2)

> +
> +#define ADS1120_TS_EN			BIT(1)
> +#define ADS1120_BCS_EN			BIT(0)
> +
> +/* Config Register 2 bit definitions */
> +#define ADS1120_VREF_MASK		GENMASK(7, 6)
> +#define ADS1120_VREF_INTERNAL		0x00
> +#define ADS1120_VREF_EXT_REFP0		0x40
> +#define ADS1120_VREF_EXT_AIN0		0x80
> +#define ADS1120_VREF_AVDD		0xc0
> +
> +#define ADS1120_REJECT_MASK		GENMASK(5, 4)
> +#define ADS1120_REJECT_OFF		0x00
> +#define ADS1120_REJECT_50_60		0x10
> +#define ADS1120_REJECT_50		0x20
> +#define ADS1120_REJECT_60		0x30
> +
> +#define ADS1120_PSW_EN			BIT(3)
> +
> +#define ADS1120_IDAC_MASK		GENMASK(2, 0)
> +
> +/* Config Register 3 bit definitions */
> +#define ADS1120_IDAC1_MASK		GENMASK(7, 5)
> +#define ADS1120_IDAC2_MASK		GENMASK(4, 2)
> +#define ADS1120_DRDYM_EN		BIT(1)
> +
> +/* Default configuration values */
> +#define ADS1120_DEFAULT_GAIN		1
> +#define ADS1120_DEFAULT_DATARATE	175

These should be just handled by writing the registers in init.
The defines in of themselves don't help over seeing the default
set in code.

> +
> +struct ads1120_state {
> +	struct spi_device	*spi;
> +	struct mutex		lock;

Locks should always have comments to say what data they protect.

> +	/*
> +	 * Used to correctly align data.
> +	 * Ensure natural alignment for potential future timestamp support.

You don't do that except by coincidence of IIO_DMA_MINALIGN being large
enough. So this comment is misleading - Drop it.

> +	 */
> +	u8 data[4] __aligned(IIO_DMA_MINALIGN);
Unless everything after this is used for DMA buffers you have defeated
the point of the __aligned 'trick'.  We need to ensure nothing that isn't
used for DMA and can potentially be modified in parallel with this
is not in the cache line.  Probably a case of just moving data to the
end of the structure.

> +
> +	u8 config[4];
> +	int current_channel;
> +	int gain;
> +	int datarate;
> +	int conv_time_ms;
> +};
> +
> +struct ads1120_datarate {
> +	int rate;
> +	int conv_time_ms;
> +	u8 reg_value;
> +};
> +
> +static const struct ads1120_datarate ads1120_datarates[] = {
> +	{ 20,   51, ADS1120_DR_20SPS },
As above, use the field values not the shifted ones.

> +	{ 45,   24, ADS1120_DR_45SPS },
> +	{ 90,   13, ADS1120_DR_90SPS },
> +	{ 175,   7, ADS1120_DR_175SPS },
> +	{ 330,   4, ADS1120_DR_330SPS },
> +	{ 600,   3, ADS1120_DR_600SPS },
> +	{ 1000,  2, ADS1120_DR_1000SPS },
> +};

> +static int ads1120_write_cmd(struct ads1120_state *st, u8 cmd)
> +{
> +	st->data[0] = cmd;
> +	return spi_write(st->spi, st->data, 1);
> +}
> +
> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
> +{
> +	u8 buf[2];
> +
> +	if (reg > ADS1120_REG_CONFIG3)
> +		return -EINVAL;
> +
> +	buf[0] = ADS1120_CMD_WREG | (reg << 2);
> +	buf[1] = value;
> +
> +	return spi_write(st->spi, buf, 2);
sizeof(buf);

However DMA safety thing applies here as well so you can't just use
a buffer in the stack.  Can use spi_write_then_read() though as that bounce
buffers.

> +}

> +
> +static int ads1120_set_channel(struct ads1120_state *st, int channel)
> +{
> +	u8 mux_val;
> +	u8 config0;
> +
> +	if (channel < 0 || channel > 3)
> +		return -EINVAL;
> +
> +	/* Map channel to AINx/AVSS single-ended input */
> +	mux_val = ADS1120_MUX_AIN0_AVSS + (channel << 4);
> +
> +	config0 = (st->config[0] & ~ADS1120_MUX_MASK) | mux_val;
> +	st->config[0] = config0;

FIELD_MODIFY() after the defines are field values (not shifted version)
and that << 4 is gone.

> +
> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
> +}
> +
> +static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
> +{
> +	u8 gain_bits;
> +	u8 config0;
> +	int i;
> +
> +	/* Find gain in supported values */
> +	for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
> +		if (ads1120_gain_values[i] == gain_val) {
> +			gain_bits = i << 1;
			gain_bits = BIT(i);

> +			goto found;

Avoid this goto by the following simplifying code flow.

			break;
> +		}
> +	}
	if (i == ARRAY_SIZE(ads1120_gain_values)
		return -EINVAL;

	config0 = ...

> +	return -EINVAL;
> +
> +found:
> +	config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
> +	st->config[0] = config0;
FIELD_MODIFY()

> +	st->gain = gain_val;

Similar to below - I'd not store this explicitly. Where it is used
is not a fast path so add loop to look it up there.

It's much easier to be sure there is no getting out of sync with
only one store of information, even if it does bloat the code a it.

> +
> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
> +}
> +
> +static int ads1120_set_datarate(struct ads1120_state *st, int rate)
> +{
> +	u8 config1;
> +	int i;
> +
> +	/* Find data rate in supported values */
> +	for (i = 0; i < ARRAY_SIZE(ads1120_datarates); i++) {
> +		if (ads1120_datarates[i].rate == rate) {

Flip logic to reduce indent
		if (ads1120_datarates[i].rate != rate)
			continue;

		config1 =...

> +			config1 = (st->config[1] & ~ADS1120_DR_MASK) |
> +				  ads1120_datarates[i].reg_value;

FIELD_MODIFY() once reg_value is the field value not a shifted version of it.
And operate directly on st->config[1]

> +			st->config[1] = config1;
> +			st->datarate = rate;
> +			st->conv_time_ms = ads1120_datarates[i].conv_time_ms;
> +
> +			return ads1120_write_reg(st, ADS1120_REG_CONFIG1,
> +						 config1);
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ads1120_read_raw_adc(struct ads1120_state *st, int *val)
> +{
> +	u8 rx_buf[4];
> +	u8 tx_buf[4] = { ADS1120_CMD_RDATA, 0xff, 0xff, 0xff };
> +	int ret;
> +	struct spi_transfer xfer = {
> +		.tx_buf = tx_buf,
> +		.rx_buf = rx_buf,
> +		.len = 4,
> +	};
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);

SPI requires DMA safe buffers.  2 ways to make that true
here.
1. Put a buffer at the end of iio_priv() structure and mark it 
   __aligned(IIO_DMA_MINALIGN);
2. Allocate on the heap here.
(Can't use spi_write_then read() here if those '0xff's are required values).

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Data format: 16-bit two's complement MSB first
> +	 * rx_buf[0] is echo of command
> +	 * rx_buf[1] is MSB of data
> +	 * rx_buf[2] is LSB of data
> +	 */
> +	*val = (s16)((rx_buf[1] << 8) | rx_buf[2]);

	*val = sign_extend32(get_unaligned_be16(&rx_buf[1]), 15);
or something along those lines.


> +
> +	return 0;
> +}
> +
> +static int ads1120_read_measurement(struct ads1120_state *st, int channel,
> +				    int *val)
> +{
> +	int ret;
> +
> +	ret = ads1120_set_channel(st, channel);
> +	if (ret)
> +		return ret;
> +
> +	/* Start single-shot conversion */

This all seems fairly standard so not sure what your RFC question was
looking for feedback on wrt to how you did single conversions?

> +	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for conversion to complete */
> +	msleep(st->conv_time_ms);
> +
> +	/* Read the result */
> +	ret = ads1120_read_raw_adc(st, val);
> +	if (ret)
> +		return ret;
> +
> +	st->current_channel = channel;
> +
> +	return 0;
> +}
> +
> +static int ads1120_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ads1120_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +		ret = ads1120_read_measurement(st, chan->channel, val);

guard() as below.

> +		mutex_unlock(&st->lock);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->gain;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->datarate;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ads1120_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ads1120_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		mutex_lock(&st->lock);

include cleanup.h and use

		guard(mutex)(&st->lock;
		return ads1120_set_gain(st, val);
here.  Similar for other cases.
 

> +		ret = ads1120_set_gain(st, val);
> +		mutex_unlock(&st->lock);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&st->lock);
> +		ret = ads1120_set_datarate(st, val);
> +		mutex_unlock(&st->lock);
> +		return ret;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ads1120_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length,
> +			      long mask)
> +{
> +	static const int datarates[] = { 20, 45, 90, 175, 330, 600, 1000 };
I'd put this up alongside the register defines.  Much easier to see it aligns
with those that way.
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = ads1120_gain_values;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ads1120_gain_values);
> +		return IIO_AVAIL_LIST;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = datarates;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(datarates);
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +
> +static int ads1120_init(struct ads1120_state *st)
> +{
> +	int ret;
> +
> +	/* Reset device to default state */

I think that is is obvious enough from the function name that I'd
drop this comment.

> +	ret = ads1120_reset(st);
> +	if (ret) {
> +		dev_err(&st->spi->dev, "Failed to reset device\n");
> +		return ret;

return dev_err_probe()

> +	}
> +
> +	/*
> +	 * Configure Register 0:
> +	 * - Input MUX: AIN0/AVSS (updated per channel read)
> +	 * - Gain: 1
> +	 * - PGA bypassed (lower power consumption)
> +	 */
> +	st->config[0] = ADS1120_MUX_AIN0_AVSS | ADS1120_GAIN_1 |
> +			ADS1120_PGA_BYPASS;
> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG0, st->config[0]);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 1:
> +	 * - Data rate: 175 SPS
> +	 * - Mode: Normal
> +	 * - Conversion mode: Single-shot
> +	 * - Temperature sensor: Disabled
> +	 * - Burnout current: Disabled
If you want to make this more self documenting you can use
the definition changes above and
	st->config[1] = FIELD_PREP(ADS1120_CFG1_DR_MASK, ADS1120_CFG1_DR_175_SPS) |
			FIELD_PREP(ADS1120_CFG1_MODE_MASK, ADS1120_CFG1_MODE_NORMAL) |
			FIELD_PREP(ADS1120_CFG1_CONTINOUS, 0) |
			FIELD_PREP(ADS1120_CFG1_TEMP, 0) |
			FIELD_PREP(ADS1120_CFG1_BCS, 0);
So provide field writes with 0 rather than setting them by their absence.


	
> +	 */
> +	st->config[1] = ADS1120_DR_175SPS | ADS1120_MODE_NORMAL |
> +			ADS1120_CM_SINGLE;
> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG1, st->config[1]);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 2:
> +	 * - Voltage reference: AVDD
> +	 * - 50/60Hz rejection: Off
> +	 * - Power switch: Off
> +	 * - IDAC: Off
> +	 */
> +	st->config[2] = ADS1120_VREF_AVDD | ADS1120_REJECT_OFF;

Currently config[2] and config[3] are unused outside this function.
Might make sense to use local variables for now.

> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 3:
> +	 * - IDAC1: Disabled
> +	 * - IDAC2: Disabled
> +	 * - DRDY mode: DOUT/DRDY pin behavior
> +	 */
> +	st->config[3] = ADS1120_DRDYM_EN;
> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG3, st->config[3]);
> +	if (ret)
> +		return ret;
> +
> +	/* Set default operating parameters */
> +	st->gain = ADS1120_DEFAULT_GAIN;
> +	st->datarate = ADS1120_DEFAULT_DATARATE;
> +	st->conv_time_ms = 7; /* For 175 SPS */

I'd set these alongside where you do the writes.
Where possible just retrieve the value from what is cached in the config[]
registers rather than having two ways to get to related info.

> +	st->current_channel = -1;
> +
> +	return 0;
> +}
> +
> +static int ads1120_probe(struct spi_device *spi)
> +{

There are enough uses of spi->dev that I'd add a local variable
	struct device *dev = &spi->dev;

> +	struct iio_dev *indio_dev;
> +	struct ads1120_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	mutex_init(&st->lock);
	ret = devm_mutex_init(&spi->dev, st->lock);
	if (ret)
		return ret;

This helper is so easy to use it makes sense to use it here even though
the lock debugging it enables is unlikely to be particularly useful.

> +
> +	indio_dev->name = "ads1120";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads1120_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
> +	indio_dev->info = &ads1120_info;
> +
> +	ret = ads1120_init(st);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to initialize device: %d\n", ret);
> +		return ret;
For errors in code only called form probe path use
		return dev_err_probe(&spi->dev, ret, "Failed to initialize device\n");

Whilst this particular path presumably doesn't defer it is still a useful
helper and pretty prints the return value + takes a few lines less than what
you currently have.

> +	}
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[Ajith Anandhan]

On 10/30/25 11:24 PM, Jonathan Cameron wrote:
> On Thu, 30 Oct 2025 22:04:10 +0530
> Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:
>
>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>> analog-to-digital converter with an SPI interface.
>>
>> The driver provides:
>> - 4 single-ended voltage input channels
>> - Programmable gain amplifier (1 to 128)
>> - Configurable data rates (20 to 1000 SPS)
>> - Single-shot conversion mode
>>
>> Link: https://www.ti.com/lit/gpn/ads1120
> Datasheet:
>
>> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
> Hi Ajith
>
> Various comments inline.  Mostly superficial stuff but the DMA safety
> of SPI buffers needs fixing.  There is a useful talk from this given
> by Wolfram Sang if you want to understand more about this
> https://www.youtube.com/watch?v=JDwaMClvV-s
>
> Thanks,
>
> Jonathan
>
>
> Thank you for the detailed review and the helpful video reference!
>> diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
>> new file mode 100644
>> index 000000000..07a6fb309
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads1120.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TI ADS1120 4-channel, 2kSPS, 16-bit delta-sigma ADC
>> + *
>> + * Datasheet: https://www.ti.com/lit/gpn/ads1120
>> + *
>> + * Copyright (C) 2025 Ajith Anandhan <ajithanandhan0406@gmail.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
> Figure out what you are actually using. There is ongoing effort to not include
> kernel.h in drivers because there is usually a small set of more appropriate
> fine grained includes.
>
Sure. I'll replace kernel.h with the specific includes needed.
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +/* Commands */
>> +#define ADS1120_CMD_RESET		0x06
>> +#define ADS1120_CMD_START		0x08
>> +#define ADS1120_CMD_PWRDWN		0x02
>> +#define ADS1120_CMD_RDATA		0x10
>> +#define ADS1120_CMD_RREG		0x20
>> +#define ADS1120_CMD_WREG		0x40
>> +
>> +/* Registers */
>> +#define ADS1120_REG_CONFIG0		0x00
>> +#define ADS1120_REG_CONFIG1		0x01
>> +#define ADS1120_REG_CONFIG2		0x02
>> +#define ADS1120_REG_CONFIG3		0x03
>> +
>> +/* Config Register 0 bit definitions */
>> +#define ADS1120_MUX_MASK		GENMASK(7, 4)
>> +#define ADS1120_MUX_AIN0_AVSS		0x80
>> +#define ADS1120_MUX_AIN1_AVSS		0x90
>> +#define ADS1120_MUX_AIN2_AVSS		0xa0
>> +#define ADS1120_MUX_AIN3_AVSS		0xb0
>> +
>> +#define ADS1120_GAIN_MASK		GENMASK(3, 1)
> Better to name these so it's obvious which register they are in.
> #define ADS1120_CFG0_GAIN_MSK or similar.
> Saves anyone checking every time that it's being written to
> the appropriate register.
I'll rename all masks to include register names.
>> +#define ADS1120_GAIN_1			0x00
>> +#define ADS1120_GAIN_2			0x02
>> +#define ADS1120_GAIN_4			0x04
>> +#define ADS1120_GAIN_8			0x06
>> +#define ADS1120_GAIN_16			0x08
>> +#define ADS1120_GAIN_32			0x0a
>> +#define ADS1120_GAIN_64			0x0c
>> +#define ADS1120_GAIN_128		0x0e
> Same as called out for DR below. (applies in other places
> as well).
Sure.
>> +
>> +#define ADS1120_PGA_BYPASS		BIT(0)
>> +
>> +/* Config Register 1 bit definitions */
>> +#define ADS1120_DR_MASK			GENMASK(7, 5)
>> +#define ADS1120_DR_20SPS		0x00
>> +#define ADS1120_DR_45SPS		0x20
>> +#define ADS1120_DR_90SPS		0x40
>> +#define ADS1120_DR_175SPS		0x60
>> +#define ADS1120_DR_330SPS		0x80
>> +#define ADS1120_DR_600SPS		0xa0
>> +#define ADS1120_DR_1000SPS		0xc0
> Define these as values of the field (So not shifted)
>
> #define ADS1120_DR_20SPS	0
> #define ADS1120_DR_45SPS	1
> etc.
> Then use FIELD_PREP(ADS1120_DR_MASK, ADIS1120_DR_20SPS)
> and similar to set them.
I'll use the FIELD_PREP and fix the values.
>
>> +
>> +#define ADS1120_MODE_MASK		GENMASK(4, 3)
>> +#define ADS1120_MODE_NORMAL		0x00
> No other values for a 2 bit field? Define all values even
> if you only use one for now.  Makes it easier to review the
> values
I'll define all the available values.
>> +
>> +#define ADS1120_CM_SINGLE		0x00
>> +#define ADS1120_CM_CONTINUOUS		BIT(2)
>> +
>> +#define ADS1120_TS_EN			BIT(1)
>> +#define ADS1120_BCS_EN			BIT(0)
>> +
>> +/* Config Register 2 bit definitions */
>> +#define ADS1120_VREF_MASK		GENMASK(7, 6)
>> +#define ADS1120_VREF_INTERNAL		0x00
>> +#define ADS1120_VREF_EXT_REFP0		0x40
>> +#define ADS1120_VREF_EXT_AIN0		0x80
>> +#define ADS1120_VREF_AVDD		0xc0
>> +
>> +#define ADS1120_REJECT_MASK		GENMASK(5, 4)
>> +#define ADS1120_REJECT_OFF		0x00
>> +#define ADS1120_REJECT_50_60		0x10
>> +#define ADS1120_REJECT_50		0x20
>> +#define ADS1120_REJECT_60		0x30
>> +
>> +#define ADS1120_PSW_EN			BIT(3)
>> +
>> +#define ADS1120_IDAC_MASK		GENMASK(2, 0)
>> +
>> +/* Config Register 3 bit definitions */
>> +#define ADS1120_IDAC1_MASK		GENMASK(7, 5)
>> +#define ADS1120_IDAC2_MASK		GENMASK(4, 2)
>> +#define ADS1120_DRDYM_EN		BIT(1)
>> +
>> +/* Default configuration values */
>> +#define ADS1120_DEFAULT_GAIN		1
>> +#define ADS1120_DEFAULT_DATARATE	175
> These should be just handled by writing the registers in init.
> The defines in of themselves don't help over seeing the default
> set in code.
Sure.I'll write it at init.
>
>> +
>> +struct ads1120_state {
>> +	struct spi_device	*spi;
>> +	struct mutex		lock;
> Locks should always have comments to say what data they protect.
I'll add the appropriate comments.
>
>> +	/*
>> +	 * Used to correctly align data.
>> +	 * Ensure natural alignment for potential future timestamp support.
> You don't do that except by coincidence of IIO_DMA_MINALIGN being large
> enough. So this comment is misleading - Drop it.
Sure.
>
>> +	 */
>> +	u8 data[4] __aligned(IIO_DMA_MINALIGN);
> Unless everything after this is used for DMA buffers you have defeated
> the point of the __aligned 'trick'.  We need to ensure nothing that isn't
> used for DMA and can potentially be modified in parallel with this
> is not in the cache line.  Probably a case of just moving data to the
> end of the structure.
I'll move the data buffer to the end of the struct.
>
>> +
>> +	u8 config[4];
>> +	int current_channel;
>> +	int gain;
>> +	int datarate;
>> +	int conv_time_ms;
>> +};
>> +
>> +struct ads1120_datarate {
>> +	int rate;
>> +	int conv_time_ms;
>> +	u8 reg_value;
>> +};
>> +
>> +static const struct ads1120_datarate ads1120_datarates[] = {
>> +	{ 20,   51, ADS1120_DR_20SPS },
> As above, use the field values not the shifted ones.
Agreed.
>
>> +	{ 45,   24, ADS1120_DR_45SPS },
>> +	{ 90,   13, ADS1120_DR_90SPS },
>> +	{ 175,   7, ADS1120_DR_175SPS },
>> +	{ 330,   4, ADS1120_DR_330SPS },
>> +	{ 600,   3, ADS1120_DR_600SPS },
>> +	{ 1000,  2, ADS1120_DR_1000SPS },
>> +};
>> +static int ads1120_write_cmd(struct ads1120_state *st, u8 cmd)
>> +{
>> +	st->data[0] = cmd;
>> +	return spi_write(st->spi, st->data, 1);
>> +}
>> +
>> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
>> +{
>> +	u8 buf[2];
>> +
>> +	if (reg > ADS1120_REG_CONFIG3)
>> +		return -EINVAL;
>> +
>> +	buf[0] = ADS1120_CMD_WREG | (reg << 2);
>> +	buf[1] = value;
>> +
>> +	return spi_write(st->spi, buf, 2);
> sizeof(buf);
>
> However DMA safety thing applies here as well so you can't just use
> a buffer in the stack.  Can use spi_write_then_read() though as that bounce
> buffers.
I prefer to use the global buffer for now.
>
>> +}
>> +
>> +static int ads1120_set_channel(struct ads1120_state *st, int channel)
>> +{
>> +	u8 mux_val;
>> +	u8 config0;
>> +
>> +	if (channel < 0 || channel > 3)
>> +		return -EINVAL;
>> +
>> +	/* Map channel to AINx/AVSS single-ended input */
>> +	mux_val = ADS1120_MUX_AIN0_AVSS + (channel << 4);
>> +
>> +	config0 = (st->config[0] & ~ADS1120_MUX_MASK) | mux_val;
>> +	st->config[0] = config0;
> FIELD_MODIFY() after the defines are field values (not shifted version)
> and that << 4 is gone.
I'll use the FIELD_MODIFY with proper arguments.
>
>> +
>> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
>> +}
>> +
>> +static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
>> +{
>> +	u8 gain_bits;
>> +	u8 config0;
>> +	int i;
>> +
>> +	/* Find gain in supported values */
>> +	for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
>> +		if (ads1120_gain_values[i] == gain_val) {
>> +			gain_bits = i << 1;
> 			gain_bits = BIT(i);
>
>> +			goto found;
> Avoid this goto by the following simplifying code flow.

I'll refactor it.

>
> 			break;
>> +		}
>> +	}
> 	if (i == ARRAY_SIZE(ads1120_gain_values)
> 		return -EINVAL;
>
> 	config0 = ...
>
>> +	return -EINVAL;
>> +
>> +found:
>> +	config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
>> +	st->config[0] = config0;
> FIELD_MODIFY()
Sure.
>
>> +	st->gain = gain_val;
> Similar to below - I'd not store this explicitly. Where it is used
> is not a fast path so add loop to look it up there.
>
> It's much easier to be sure there is no getting out of sync with
> only one store of information, even if it does bloat the code a it.
MSTM. I'll correct it.
>
>> +
>> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
>> +}
>> +
>> +static int ads1120_set_datarate(struct ads1120_state *st, int rate)
>> +{
>> +	u8 config1;
>> +	int i;
>> +
>> +	/* Find data rate in supported values */
>> +	for (i = 0; i < ARRAY_SIZE(ads1120_datarates); i++) {
>> +		if (ads1120_datarates[i].rate == rate) {
> Flip logic to reduce indent
> 		if (ads1120_datarates[i].rate != rate)
> 			continue;
>
> 		config1 =...
I'll follow the same.
>> +			config1 = (st->config[1] & ~ADS1120_DR_MASK) |
>> +				  ads1120_datarates[i].reg_value;
> FIELD_MODIFY() once reg_value is the field value not a shifted version of it.
> And operate directly on st->config[1]
Agreed.
>
>> +			st->config[1] = config1;
>> +			st->datarate = rate;
>> +			st->conv_time_ms = ads1120_datarates[i].conv_time_ms;
>> +
>> +			return ads1120_write_reg(st, ADS1120_REG_CONFIG1,
>> +						 config1);
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int ads1120_read_raw_adc(struct ads1120_state *st, int *val)
>> +{
>> +	u8 rx_buf[4];
>> +	u8 tx_buf[4] = { ADS1120_CMD_RDATA, 0xff, 0xff, 0xff };
>> +	int ret;
>> +	struct spi_transfer xfer = {
>> +		.tx_buf = tx_buf,
>> +		.rx_buf = rx_buf,
>> +		.len = 4,
>> +	};
>> +
>> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> SPI requires DMA safe buffers.  2 ways to make that true
> here.
> 1. Put a buffer at the end of iio_priv() structure and mark it
>     __aligned(IIO_DMA_MINALIGN);
> 2. Allocate on the heap here.
> (Can't use spi_write_then read() here if those '0xff's are required values).
I have chose to move(option 1) the buffer to the end of structure.
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Data format: 16-bit two's complement MSB first
>> +	 * rx_buf[0] is echo of command
>> +	 * rx_buf[1] is MSB of data
>> +	 * rx_buf[2] is LSB of data
>> +	 */
>> +	*val = (s16)((rx_buf[1] << 8) | rx_buf[2]);
> 	*val = sign_extend32(get_unaligned_be16(&rx_buf[1]), 15);
> or something along those lines.
I'll fix.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_read_measurement(struct ads1120_state *st, int channel,
>> +				    int *val)
>> +{
>> +	int ret;
>> +
>> +	ret = ads1120_set_channel(st, channel);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Start single-shot conversion */
> This all seems fairly standard so not sure what your RFC question was
> looking for feedback on wrt to how you did single conversions?

I was indeed concerned about using the polling(adding wait) method to 
read adc values.

That's the reason i have asked it in the cover letter.

>
>> +	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for conversion to complete */
>> +	msleep(st->conv_time_ms);
>> +
>> +	/* Read the result */
>> +	ret = ads1120_read_raw_adc(st, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	st->current_channel = channel;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	struct ads1120_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&st->lock);
>> +		ret = ads1120_read_measurement(st, chan->channel, val);
> guard() as below.
Sure.
>
>> +		mutex_unlock(&st->lock);
>> +		if (ret)
>> +			return ret;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = st->gain;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = st->datarate;
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ads1120_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	struct ads1120_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		mutex_lock(&st->lock);
> include cleanup.h and use
>
> 		guard(mutex)(&st->lock;
> 		return ads1120_set_gain(st, val);
> here.  Similar for other cases.
I'll include cleanup.h and use guard instead of handling mutex directly.
>   
>> +		ret = ads1120_set_gain(st, val);
>> +		mutex_unlock(&st->lock);
>> +		return ret;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		mutex_lock(&st->lock);
>> +		ret = ads1120_set_datarate(st, val);
>> +		mutex_unlock(&st->lock);
>> +		return ret;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ads1120_read_avail(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      const int **vals, int *type, int *length,
>> +			      long mask)
>> +{
>> +	static const int datarates[] = { 20, 45, 90, 175, 330, 600, 1000 };
> I'd put this up alongside the register defines.  Much easier to see it aligns
> with those that way.
I'll fix.
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*vals = ads1120_gain_values;
>> +		*type = IIO_VAL_INT;
>> +		*length = ARRAY_SIZE(ads1120_gain_values);
>> +		return IIO_AVAIL_LIST;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*vals = datarates;
>> +		*type = IIO_VAL_INT;
>> +		*length = ARRAY_SIZE(datarates);
>> +		return IIO_AVAIL_LIST;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> ...
>
>> +
>> +static int ads1120_init(struct ads1120_state *st)
>> +{
>> +	int ret;
>> +
>> +	/* Reset device to default state */
> I think that is is obvious enough from the function name that I'd
> drop this comment.
I'll remove the comment.
>
>> +	ret = ads1120_reset(st);
>> +	if (ret) {
>> +		dev_err(&st->spi->dev, "Failed to reset device\n");
>> +		return ret;
> return dev_err_probe()
Noted.
>
>> +	}
>> +
>> +	/*
>> +	 * Configure Register 0:
>> +	 * - Input MUX: AIN0/AVSS (updated per channel read)
>> +	 * - Gain: 1
>> +	 * - PGA bypassed (lower power consumption)
>> +	 */
>> +	st->config[0] = ADS1120_MUX_AIN0_AVSS | ADS1120_GAIN_1 |
>> +			ADS1120_PGA_BYPASS;
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG0, st->config[0]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Configure Register 1:
>> +	 * - Data rate: 175 SPS
>> +	 * - Mode: Normal
>> +	 * - Conversion mode: Single-shot
>> +	 * - Temperature sensor: Disabled
>> +	 * - Burnout current: Disabled
> If you want to make this more self documenting you can use
> the definition changes above and
> 	st->config[1] = FIELD_PREP(ADS1120_CFG1_DR_MASK, ADS1120_CFG1_DR_175_SPS) |
> 			FIELD_PREP(ADS1120_CFG1_MODE_MASK, ADS1120_CFG1_MODE_NORMAL) |
> 			FIELD_PREP(ADS1120_CFG1_CONTINOUS, 0) |
> 			FIELD_PREP(ADS1120_CFG1_TEMP, 0) |
> 			FIELD_PREP(ADS1120_CFG1_BCS, 0);
> So provide field writes with 0 rather than setting them by their absence.
I'll use FIELD_PREP and add the fields with 0 to show their disable status.
>
>
> 	
>> +	 */
>> +	st->config[1] = ADS1120_DR_175SPS | ADS1120_MODE_NORMAL |
>> +			ADS1120_CM_SINGLE;
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG1, st->config[1]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Configure Register 2:
>> +	 * - Voltage reference: AVDD
>> +	 * - 50/60Hz rejection: Off
>> +	 * - Power switch: Off
>> +	 * - IDAC: Off
>> +	 */
>> +	st->config[2] = ADS1120_VREF_AVDD | ADS1120_REJECT_OFF;
> Currently config[2] and config[3] are unused outside this function.
> Might make sense to use local variables for now.
I'll use local variables for config[2] and config[3].
>
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Configure Register 3:
>> +	 * - IDAC1: Disabled
>> +	 * - IDAC2: Disabled
>> +	 * - DRDY mode: DOUT/DRDY pin behavior
>> +	 */
>> +	st->config[3] = ADS1120_DRDYM_EN;
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG3, st->config[3]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set default operating parameters */
>> +	st->gain = ADS1120_DEFAULT_GAIN;
>> +	st->datarate = ADS1120_DEFAULT_DATARATE;
>> +	st->conv_time_ms = 7; /* For 175 SPS */
> I'd set these alongside where you do the writes.
> Where possible just retrieve the value from what is cached in the config[]
> registers rather than having two ways to get to related info.
Sure, I'll fix.
>
>> +	st->current_channel = -1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_probe(struct spi_device *spi)
>> +{
> There are enough uses of spi->dev that I'd add a local variable
> 	struct device *dev = &spi->dev;
>
I'll add local var to handle it.
>> +	struct iio_dev *indio_dev;
>> +	struct ads1120_state *st;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +	st->spi = spi;
>> +
>> +	mutex_init(&st->lock);
> 	ret = devm_mutex_init(&spi->dev, st->lock);
> 	if (ret)
> 		return ret;
>
> This helper is so easy to use it makes sense to use it here even though
> the lock debugging it enables is unlikely to be particularly useful.
Agreed.
>
>> +
>> +	indio_dev->name = "ads1120";
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ads1120_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
>> +	indio_dev->info = &ads1120_info;
>> +
>> +	ret = ads1120_init(st);
>> +	if (ret) {
>> +		dev_err(&spi->dev, "Failed to initialize device: %d\n", ret);
>> +		return ret;
> For errors in code only called form probe path use
> 		return dev_err_probe(&spi->dev, ret, "Failed to initialize device\n");
>
> Whilst this particular path presumably doesn't defer it is still a useful
> helper and pretty prints the return value + takes a few lines less than what
> you currently have.
Agreed. I'll follow the same.
>
>> +	}
>> +
>> +	return devm_iio_device_register(&spi->dev, indio_dev);
>> +}

I’ll post v2 with these fixes shortly.

BR,

Ajith.

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[Jonathan Cameron]

On Fri, 7 Nov 2025 20:10:15 +0530
Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:

> On 10/30/25 11:24 PM, Jonathan Cameron wrote:
> > On Thu, 30 Oct 2025 22:04:10 +0530
> > Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:
> >  
> >> Add driver for the Texas Instruments ADS1120, a precision 16-bit
> >> analog-to-digital converter with an SPI interface.
> >>
> >> The driver provides:
> >> - 4 single-ended voltage input channels
> >> - Programmable gain amplifier (1 to 128)
> >> - Configurable data rates (20 to 1000 SPS)
> >> - Single-shot conversion mode
> >>
> >> Link: https://www.ti.com/lit/gpn/ads1120  
> > Datasheet:
> >  
> >> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>  
> > Hi Ajith
> >
> > Various comments inline.  Mostly superficial stuff but the DMA safety
> > of SPI buffers needs fixing.  There is a useful talk from this given
> > by Wolfram Sang if you want to understand more about this
> > https://www.youtube.com/watch?v=JDwaMClvV-s
> >
> > Thanks,
> >
> > Jonathan

Hi Ajith,

A small process thing around efficiency.

Crop your reply to only include things where you are answering questions
or wish the discussion to focus.  If you accept changes, just put that stuff
in the change log for the next version.
Save a lot of scrolling and makes it a lot less likely important stuff
will be lost in the noise!

> >> +static int ads1120_read_measurement(struct ads1120_state *st, int channel,
> >> +				    int *val)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = ads1120_set_channel(st, channel);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Start single-shot conversion */  
> > This all seems fairly standard so not sure what your RFC question was
> > looking for feedback on wrt to how you did single conversions?  
> 
> I was indeed concerned about using the polling(adding wait) method to 
> read adc values.
> 
> That's the reason i have asked it in the cover letter.
Ok. A bit more detail next time on what you want feedback on will
help focus things.

> 
> >  
> >> +	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
> >> +	if (ret)

Thanks,

Jonathan

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[David Lechner]

On 10/30/25 11:34 AM, Ajith Anandhan wrote:
> Add driver for the Texas Instruments ADS1120, a precision 16-bit
> analog-to-digital converter with an SPI interface.
> 
> The driver provides:
> - 4 single-ended voltage input channels
> - Programmable gain amplifier (1 to 128)
> - Configurable data rates (20 to 1000 SPS)

I don't think there is much point in having a configureble data rate
until we add buffered reads since direct mode is using single-shot
conversions. So we can save that for a later patch/series that also
implementes buffered reads based off of the DRDY interrupt.

Maybe I'm wrong though and being able to specify the conversion
time for a single sample is still important. For example, if it
has some filtering effect.

Otherwise, I would just set it to the "best" value for single-shot
mode (if there is one) and always use that one rate.

> - Single-shot conversion mode
> 
> Link: https://www.ti.com/lit/gpn/ads1120
> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
> ---
>  drivers/iio/adc/Kconfig      |  10 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ti-ads1120.c | 509 +++++++++++++++++++++++++++++++++++
>  3 files changed, 520 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads1120.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58a14e683..afb7895dd 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1655,6 +1655,16 @@ config TI_ADS1119
>           This driver can also be built as a module. If so, the module will be
>           called ti-ads1119.
>  
> +config TI_ADS1120
> +	tristate "Texas Instruments ADS1120"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Texas Instruments ADS1120
> +	  4-channel, 2kSPS, 16-bit delta-sigma ADC.
> +
> +	  This driver can also be built as module. If so, the module
> +	  will be called ti-ads1120.
> +
>  config TI_ADS124S08
>  	tristate "Texas Instruments ADS124S08"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d008f78dc..49c56b459 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
>  obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> +obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
>  obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>  obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
>  obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
> diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
> new file mode 100644
> index 000000000..07a6fb309
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1120.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0

Prefer more specific GPL-2.0-only or GPL-2.0-or-later (your choice).

> +/*
> + * TI ADS1120 4-channel, 2kSPS, 16-bit delta-sigma ADC
> + *
> + * Datasheet: https://www.ti.com/lit/gpn/ads1120
> + *
> + * Copyright (C) 2025 Ajith Anandhan <ajithanandhan0406@gmail.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Commands */
> +#define ADS1120_CMD_RESET		0x06
> +#define ADS1120_CMD_START		0x08
> +#define ADS1120_CMD_PWRDWN		0x02
> +#define ADS1120_CMD_RDATA		0x10
> +#define ADS1120_CMD_RREG		0x20
> +#define ADS1120_CMD_WREG		0x40
> +
> +/* Registers */
> +#define ADS1120_REG_CONFIG0		0x00
> +#define ADS1120_REG_CONFIG1		0x01
> +#define ADS1120_REG_CONFIG2		0x02
> +#define ADS1120_REG_CONFIG3		0x03
> +
> +/* Config Register 0 bit definitions */
> +#define ADS1120_MUX_MASK		GENMASK(7, 4)
> +#define ADS1120_MUX_AIN0_AVSS		0x80
> +#define ADS1120_MUX_AIN1_AVSS		0x90
> +#define ADS1120_MUX_AIN2_AVSS		0xa0
> +#define ADS1120_MUX_AIN3_AVSS		0xb0
> +
> +#define ADS1120_GAIN_MASK		GENMASK(3, 1)
> +#define ADS1120_GAIN_1			0x00
> +#define ADS1120_GAIN_2			0x02
> +#define ADS1120_GAIN_4			0x04
> +#define ADS1120_GAIN_8			0x06
> +#define ADS1120_GAIN_16			0x08
> +#define ADS1120_GAIN_32			0x0a
> +#define ADS1120_GAIN_64			0x0c
> +#define ADS1120_GAIN_128		0x0e
> +
> +#define ADS1120_PGA_BYPASS		BIT(0)
> +
> +/* Config Register 1 bit definitions */
> +#define ADS1120_DR_MASK			GENMASK(7, 5)
> +#define ADS1120_DR_20SPS		0x00
> +#define ADS1120_DR_45SPS		0x20
> +#define ADS1120_DR_90SPS		0x40
> +#define ADS1120_DR_175SPS		0x60
> +#define ADS1120_DR_330SPS		0x80
> +#define ADS1120_DR_600SPS		0xa0
> +#define ADS1120_DR_1000SPS		0xc0
> +
> +#define ADS1120_MODE_MASK		GENMASK(4, 3)
> +#define ADS1120_MODE_NORMAL		0x00
> +
> +#define ADS1120_CM_SINGLE		0x00
> +#define ADS1120_CM_CONTINUOUS		BIT(2)
> +
> +#define ADS1120_TS_EN			BIT(1)
> +#define ADS1120_BCS_EN			BIT(0)
> +
> +/* Config Register 2 bit definitions */
> +#define ADS1120_VREF_MASK		GENMASK(7, 6)
> +#define ADS1120_VREF_INTERNAL		0x00
> +#define ADS1120_VREF_EXT_REFP0		0x40
> +#define ADS1120_VREF_EXT_AIN0		0x80
> +#define ADS1120_VREF_AVDD		0xc0
> +
> +#define ADS1120_REJECT_MASK		GENMASK(5, 4)
> +#define ADS1120_REJECT_OFF		0x00
> +#define ADS1120_REJECT_50_60		0x10
> +#define ADS1120_REJECT_50		0x20
> +#define ADS1120_REJECT_60		0x30
> +
> +#define ADS1120_PSW_EN			BIT(3)
> +
> +#define ADS1120_IDAC_MASK		GENMASK(2, 0)
> +
> +/* Config Register 3 bit definitions */
> +#define ADS1120_IDAC1_MASK		GENMASK(7, 5)
> +#define ADS1120_IDAC2_MASK		GENMASK(4, 2)
> +#define ADS1120_DRDYM_EN		BIT(1)
> +
> +/* Default configuration values */
> +#define ADS1120_DEFAULT_GAIN		1
> +#define ADS1120_DEFAULT_DATARATE	175
> +
> +struct ads1120_state {
> +	struct spi_device	*spi;
> +	struct mutex		lock;
> +	/*
> +	 * Used to correctly align data.
> +	 * Ensure natural alignment for potential future timestamp support.
> +	 */
> +	u8 data[4] __aligned(IIO_DMA_MINALIGN);
> +
> +	u8 config[4];
> +	int current_channel;
> +	int gain;

Since inputs are multiplexed, we can make this gain a per-channel value, no?

It also sounds like that certain mux input have to have the PGA bypassed
which means they are limited to only 3 gain values. So these would have
a different scale_available attribute.

> +	int datarate;
> +	int conv_time_ms;
> +};
> +
> +struct ads1120_datarate {
> +	int rate;
> +	int conv_time_ms;
> +	u8 reg_value;
> +};
> +
> +static const struct ads1120_datarate ads1120_datarates[] = {
> +	{ 20,   51, ADS1120_DR_20SPS },
> +	{ 45,   24, ADS1120_DR_45SPS },
> +	{ 90,   13, ADS1120_DR_90SPS },
> +	{ 175,   7, ADS1120_DR_175SPS },
> +	{ 330,   4, ADS1120_DR_330SPS },
> +	{ 600,   3, ADS1120_DR_600SPS },
> +	{ 1000,  2, ADS1120_DR_1000SPS },
> +};
> +
> +static const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> +
> +#define ADS1120_CHANNEL(index)					\
> +{								\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			      BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \

	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),

is also need to actually make use of the function you implemented. ;-)

> +}
> +
> +static const struct iio_chan_spec ads1120_channels[] = {
> +	ADS1120_CHANNEL(0),
> +	ADS1120_CHANNEL(1),
> +	ADS1120_CHANNEL(2),
> +	ADS1120_CHANNEL(3),

The mux has 15 possible values, so I would expect 15 channels
to coorespond to all possible combinations. 8 differnetial
channels for the first 8, then these 4 single-ended channels.
Then a few more differential channels for the 3 diagnostic
inputs.

> +};
> +
> +static int ads1120_write_cmd(struct ads1120_state *st, u8 cmd)
> +{
> +	st->data[0] = cmd;
> +	return spi_write(st->spi, st->data, 1);
> +}
> +
> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
> +{
> +	u8 buf[2];
> +
> +	if (reg > ADS1120_REG_CONFIG3)
> +		return -EINVAL;
> +
> +	buf[0] = ADS1120_CMD_WREG | (reg << 2);
> +	buf[1] = value;
> +
> +	return spi_write(st->spi, buf, 2);
> +}

Can we use the regmap framework instead of writing our own?

> +
> +static int ads1120_reset(struct ads1120_state *st)
> +{
> +	int ret;
> +
> +	ret = ads1120_write_cmd(st, ADS1120_CMD_RESET);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Datasheet specifies reset takes 50us + 32 * t(CLK)
> +	 * where t(CLK) = 1/4.096MHz. Use 200us to be safe.
> +	 */
> +	usleep_range(200, 300);

	fsleep(200);

> +
> +	return 0;
> +}
> +
> +static int ads1120_set_channel(struct ads1120_state *st, int channel)
> +{
> +	u8 mux_val;
> +	u8 config0;
> +
> +	if (channel < 0 || channel > 3)
> +		return -EINVAL;
> +
> +	/* Map channel to AINx/AVSS single-ended input */
> +	mux_val = ADS1120_MUX_AIN0_AVSS + (channel << 4);
> +
> +	config0 = (st->config[0] & ~ADS1120_MUX_MASK) | mux_val;
> +	st->config[0] = config0;
> +
> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
> +}
> +
> +static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
> +{
> +	u8 gain_bits;
> +	u8 config0;
> +	int i;
> +
> +	/* Find gain in supported values */
> +	for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
> +		if (ads1120_gain_values[i] == gain_val) {
> +			gain_bits = i << 1;
> +			goto found;
> +		}
> +	}
> +	return -EINVAL;
> +
> +found:
> +	config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
> +	st->config[0] = config0;
> +	st->gain = gain_val;
> +
> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
> +}
> +
> +static int ads1120_set_datarate(struct ads1120_state *st, int rate)
> +{
> +	u8 config1;
> +	int i;
> +
> +	/* Find data rate in supported values */
> +	for (i = 0; i < ARRAY_SIZE(ads1120_datarates); i++) {
> +		if (ads1120_datarates[i].rate == rate) {
> +			config1 = (st->config[1] & ~ADS1120_DR_MASK) |
> +				  ads1120_datarates[i].reg_value;
> +			st->config[1] = config1;
> +			st->datarate = rate;
> +			st->conv_time_ms = ads1120_datarates[i].conv_time_ms;
> +
> +			return ads1120_write_reg(st, ADS1120_REG_CONFIG1,
> +						 config1);
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ads1120_read_raw_adc(struct ads1120_state *st, int *val)
> +{
> +	u8 rx_buf[4];
> +	u8 tx_buf[4] = { ADS1120_CMD_RDATA, 0xff, 0xff, 0xff };
> +	int ret;
> +	struct spi_transfer xfer = {
> +		.tx_buf = tx_buf,
> +		.rx_buf = rx_buf,
> +		.len = 4,
> +	};

This should be split into two transfers (still only one SPI message).
Then we don't need to pad the buffers. Also, it seems to have one
more byte than needed. Only to to tx one byte then rx two bytes.

> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Data format: 16-bit two's complement MSB first
> +	 * rx_buf[0] is echo of command
> +	 * rx_buf[1] is MSB of data
> +	 * rx_buf[2] is LSB of data
> +	 */
> +	*val = (s16)((rx_buf[1] << 8) | rx_buf[2]);
> +
> +	return 0;
> +}
> +
> +static int ads1120_read_measurement(struct ads1120_state *st, int channel,
> +				    int *val)
> +{
> +	int ret;
> +
> +	ret = ads1120_set_channel(st, channel);
> +	if (ret)
> +		return ret;
> +



> +	/* Start single-shot conversion */
> +	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for conversion to complete */
> +	msleep(st->conv_time_ms);
> +
> +	/* Read the result */
> +	ret = ads1120_read_raw_adc(st, val);
> +	if (ret)
> +		return ret;
> +

This could actually all be done in a single SPI message with 3
transers. The spi_transfer struct has delay fields that can
provide the delay instead of msleep().

> +	st->current_channel = channel;
> +
> +	return 0;
> +}
> +
> +static int ads1120_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ads1120_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +		ret = ads1120_read_measurement(st, chan->channel, val);
> +		mutex_unlock(&st->lock);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->gain;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->datarate;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ads1120_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ads1120_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		mutex_lock(&st->lock);
> +		ret = ads1120_set_gain(st, val);
> +		mutex_unlock(&st->lock);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&st->lock);
> +		ret = ads1120_set_datarate(st, val);
> +		mutex_unlock(&st->lock);
> +		return ret;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ads1120_read_avail(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      const int **vals, int *type, int *length,
> +			      long mask)
> +{
> +	static const int datarates[] = { 20, 45, 90, 175, 330, 600, 1000 };
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = ads1120_gain_values;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ads1120_gain_values);

Scale also depends on the reference voltage, so it isn't quite so simple.

> +		return IIO_AVAIL_LIST;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = datarates;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(datarates);
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ads1120_info = {
> +	.read_raw = ads1120_read_raw,
> +	.write_raw = ads1120_write_raw,
> +	.read_avail = ads1120_read_avail,
> +};
> +
> +static int ads1120_init(struct ads1120_state *st)
> +{
> +	int ret;
> +
> +	/* Reset device to default state */
> +	ret = ads1120_reset(st);
> +	if (ret) {
> +		dev_err(&st->spi->dev, "Failed to reset device\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Configure Register 0:
> +	 * - Input MUX: AIN0/AVSS (updated per channel read)
> +	 * - Gain: 1
> +	 * - PGA bypassed (lower power consumption)

Should extend the comment to say that if gain is set higher than 4,
this value is ignored, so it is safe to leave it set all the time.

> +	 */
> +	st->config[0] = ADS1120_MUX_AIN0_AVSS | ADS1120_GAIN_1 |
> +			ADS1120_PGA_BYPASS;
> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG0, st->config[0]);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 1:
> +	 * - Data rate: 175 SPS
> +	 * - Mode: Normal
> +	 * - Conversion mode: Single-shot
> +	 * - Temperature sensor: Disabled
> +	 * - Burnout current: Disabled
> +	 */
> +	st->config[1] = ADS1120_DR_175SPS | ADS1120_MODE_NORMAL |
> +			ADS1120_CM_SINGLE;
> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG1, st->config[1]);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 2:
> +	 * - Voltage reference: AVDD
> +	 * - 50/60Hz rejection: Off
> +	 * - Power switch: Off
> +	 * - IDAC: Off
> +	 */
> +	st->config[2] = ADS1120_VREF_AVDD | ADS1120_REJECT_OFF;
> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Configure Register 3:

	 * - Internal voltage reference
	 * - No FIR filter
	 * - Power switch always open

> +	 * - IDAC1: Disabled
> +	 * - IDAC2: Disabled
> +	 * - DRDY mode: DOUT/DRDY pin behavior
> +	 */
> +	st->config[3] = ADS1120_DRDYM_EN;

It doesn't make sense to enable the DRDY pin on the DOUT line unless
we know that it is wired up to an interrupt.

> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG3, st->config[3]);
> +	if (ret)
> +		return ret;
> +
> +	/* Set default operating parameters */
> +	st->gain = ADS1120_DEFAULT_GAIN;
> +	st->datarate = ADS1120_DEFAULT_DATARATE;
> +	st->conv_time_ms = 7; /* For 175 SPS */
> +	st->current_channel = -1;
> +
> +	return 0;
> +}
> +
> +static int ads1120_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ads1120_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	mutex_init(&st->lock);
> +
> +	indio_dev->name = "ads1120";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads1120_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
> +	indio_dev->info = &ads1120_info;
> +
> +	ret = ads1120_init(st);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to initialize device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ads1120_of_match[] = {
> +	{ .compatible = "ti,ads1120" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ads1120_of_match);
> +
> +static const struct spi_device_id ads1120_id[] = {
> +	{ "ads1120" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ads1120_id);
> +
> +static struct spi_driver ads1120_driver = {
> +	.driver = {
> +		.name = "ads1120",
> +		.of_match_table = ads1120_of_match,
> +	},
> +	.probe = ads1120_probe,
> +	.id_table = ads1120_id,
> +};
> +module_spi_driver(ads1120_driver);
> +
> +MODULE_AUTHOR("Ajith Anandhan  <ajithanandhan0406@gmail.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADS1120 ADC driver");
> +MODULE_LICENSE("GPL");

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[Ajith Anandhan]

On 10/31/25 2:43 AM, David Lechner wrote:
> On 10/30/25 11:34 AM, Ajith Anandhan wrote:
>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>> analog-to-digital converter with an SPI interface.
>>
>> The driver provides:
>> - 4 single-ended voltage input channels
>> - Programmable gain amplifier (1 to 128)
>> - Configurable data rates (20 to 1000 SPS)
> I don't think there is much point in having a configureble data rate
> until we add buffered reads since direct mode is using single-shot
> conversions. So we can save that for a later patch/series that also
> implementes buffered reads based off of the DRDY interrupt.
>
> Maybe I'm wrong though and being able to specify the conversion
> time for a single sample is still important. For example, if it
> has some filtering effect.
>
> Otherwise, I would just set it to the "best" value for single-shot
> mode (if there is one) and always use that one rate.

You're right. I'll remove the configurable data rate for now and

just use a fixed optimal rate (20 SPS as default since its highest 
accuracy).

We'll add this back when implementing buffered reads with DRDY interrupt

support in a future patch series.

>
>> - Single-shot conversion mode
>>
>> Link: https://www.ti.com/lit/gpn/ads1120
>> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
>> ---
>>   drivers/iio/adc/Kconfig      |  10 +
>>   drivers/iio/adc/Makefile     |   1 +
>>   drivers/iio/adc/ti-ads1120.c | 509 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 520 insertions(+)
>>   create mode 100644 drivers/iio/adc/ti-ads1120.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 58a14e683..afb7895dd 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -1655,6 +1655,16 @@ config TI_ADS1119
>>            This driver can also be built as a module. If so, the module will be
>>            called ti-ads1119.
>>   
>> +config TI_ADS1120
>> +	tristate "Texas Instruments ADS1120"
>> +	depends on SPI
>> +	help
>> +	  Say yes here to build support for Texas Instruments ADS1120
>> +	  4-channel, 2kSPS, 16-bit delta-sigma ADC.
>> +
>> +	  This driver can also be built as module. If so, the module
>> +	  will be called ti-ads1120.
>> +
>>   config TI_ADS124S08
>>   	tristate "Texas Instruments ADS124S08"
>>   	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index d008f78dc..49c56b459 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -144,6 +144,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>>   obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>   obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
>>   obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
>> +obj-$(CONFIG_TI_ADS1120) += ti-ads1120.o
>>   obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>>   obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
>>   obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
>> diff --git a/drivers/iio/adc/ti-ads1120.c b/drivers/iio/adc/ti-ads1120.c
>> new file mode 100644
>> index 000000000..07a6fb309
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads1120.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
> Prefer more specific GPL-2.0-only or GPL-2.0-or-later (your choice).
Noted.
>
>> +/*
>> + * TI ADS1120 4-channel, 2kSPS, 16-bit delta-sigma ADC
>> + *
>> + * Datasheet: https://www.ti.com/lit/gpn/ads1120
>> + *
>> + * Copyright (C) 2025 Ajith Anandhan <ajithanandhan0406@gmail.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +/* Commands */
>> +#define ADS1120_CMD_RESET		0x06
>> +#define ADS1120_CMD_START		0x08
>> +#define ADS1120_CMD_PWRDWN		0x02
>> +#define ADS1120_CMD_RDATA		0x10
>> +#define ADS1120_CMD_RREG		0x20
>> +#define ADS1120_CMD_WREG		0x40
>> +
>> +/* Registers */
>> +#define ADS1120_REG_CONFIG0		0x00
>> +#define ADS1120_REG_CONFIG1		0x01
>> +#define ADS1120_REG_CONFIG2		0x02
>> +#define ADS1120_REG_CONFIG3		0x03
>> +
>> +/* Config Register 0 bit definitions */
>> +#define ADS1120_MUX_MASK		GENMASK(7, 4)
>> +#define ADS1120_MUX_AIN0_AVSS		0x80
>> +#define ADS1120_MUX_AIN1_AVSS		0x90
>> +#define ADS1120_MUX_AIN2_AVSS		0xa0
>> +#define ADS1120_MUX_AIN3_AVSS		0xb0
>> +
>> +#define ADS1120_GAIN_MASK		GENMASK(3, 1)
>> +#define ADS1120_GAIN_1			0x00
>> +#define ADS1120_GAIN_2			0x02
>> +#define ADS1120_GAIN_4			0x04
>> +#define ADS1120_GAIN_8			0x06
>> +#define ADS1120_GAIN_16			0x08
>> +#define ADS1120_GAIN_32			0x0a
>> +#define ADS1120_GAIN_64			0x0c
>> +#define ADS1120_GAIN_128		0x0e
>> +
>> +#define ADS1120_PGA_BYPASS		BIT(0)
>> +
>> +/* Config Register 1 bit definitions */
>> +#define ADS1120_DR_MASK			GENMASK(7, 5)
>> +#define ADS1120_DR_20SPS		0x00
>> +#define ADS1120_DR_45SPS		0x20
>> +#define ADS1120_DR_90SPS		0x40
>> +#define ADS1120_DR_175SPS		0x60
>> +#define ADS1120_DR_330SPS		0x80
>> +#define ADS1120_DR_600SPS		0xa0
>> +#define ADS1120_DR_1000SPS		0xc0
>> +
>> +#define ADS1120_MODE_MASK		GENMASK(4, 3)
>> +#define ADS1120_MODE_NORMAL		0x00
>> +
>> +#define ADS1120_CM_SINGLE		0x00
>> +#define ADS1120_CM_CONTINUOUS		BIT(2)
>> +
>> +#define ADS1120_TS_EN			BIT(1)
>> +#define ADS1120_BCS_EN			BIT(0)
>> +
>> +/* Config Register 2 bit definitions */
>> +#define ADS1120_VREF_MASK		GENMASK(7, 6)
>> +#define ADS1120_VREF_INTERNAL		0x00
>> +#define ADS1120_VREF_EXT_REFP0		0x40
>> +#define ADS1120_VREF_EXT_AIN0		0x80
>> +#define ADS1120_VREF_AVDD		0xc0
>> +
>> +#define ADS1120_REJECT_MASK		GENMASK(5, 4)
>> +#define ADS1120_REJECT_OFF		0x00
>> +#define ADS1120_REJECT_50_60		0x10
>> +#define ADS1120_REJECT_50		0x20
>> +#define ADS1120_REJECT_60		0x30
>> +
>> +#define ADS1120_PSW_EN			BIT(3)
>> +
>> +#define ADS1120_IDAC_MASK		GENMASK(2, 0)
>> +
>> +/* Config Register 3 bit definitions */
>> +#define ADS1120_IDAC1_MASK		GENMASK(7, 5)
>> +#define ADS1120_IDAC2_MASK		GENMASK(4, 2)
>> +#define ADS1120_DRDYM_EN		BIT(1)
>> +
>> +/* Default configuration values */
>> +#define ADS1120_DEFAULT_GAIN		1
>> +#define ADS1120_DEFAULT_DATARATE	175
>> +
>> +struct ads1120_state {
>> +	struct spi_device	*spi;
>> +	struct mutex		lock;
>> +	/*
>> +	 * Used to correctly align data.
>> +	 * Ensure natural alignment for potential future timestamp support.
>> +	 */
>> +	u8 data[4] __aligned(IIO_DMA_MINALIGN);
>> +
>> +	u8 config[4];
>> +	int current_channel;
>> +	int gain;
> Since inputs are multiplexed, we can make this gain a per-channel value, no?

Yes we can, However i want  to keep the initial version simple so would 
it be

fine to support per-channel gain configurationin upcoming patches?

>
> It also sounds like that certain mux input have to have the PGA bypassed
> which means they are limited to only 3 gain values. So these would have
> a different scale_available attribute.

  Since, I'm gonna enable all the 15 channels. I believe we have to have 
all

gains(for differential channels). Correct me if i'm wrong.

>
>> +	int datarate;
>> +	int conv_time_ms;
>> +};
>> +
>> +struct ads1120_datarate {
>> +	int rate;
>> +	int conv_time_ms;
>> +	u8 reg_value;
>> +};
>> +
>> +static const struct ads1120_datarate ads1120_datarates[] = {
>> +	{ 20,   51, ADS1120_DR_20SPS },
>> +	{ 45,   24, ADS1120_DR_45SPS },
>> +	{ 90,   13, ADS1120_DR_90SPS },
>> +	{ 175,   7, ADS1120_DR_175SPS },
>> +	{ 330,   4, ADS1120_DR_330SPS },
>> +	{ 600,   3, ADS1120_DR_600SPS },
>> +	{ 1000,  2, ADS1120_DR_1000SPS },
>> +};
>> +
>> +static const int ads1120_gain_values[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
>> +
>> +#define ADS1120_CHANNEL(index)					\
>> +{								\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.channel = index,					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
>> +			      BIT(IIO_CHAN_INFO_SCALE),		\
>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> 	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
>
> is also need to actually make use of the function you implemented. ;-)
Sure. I'll fix.
>
>> +}
>> +
>> +static const struct iio_chan_spec ads1120_channels[] = {
>> +	ADS1120_CHANNEL(0),
>> +	ADS1120_CHANNEL(1),
>> +	ADS1120_CHANNEL(2),
>> +	ADS1120_CHANNEL(3),
> The mux has 15 possible values, so I would expect 15 channels
> to coorespond to all possible combinations. 8 differnetial
> channels for the first 8, then these 4 single-ended channels.
> Then a few more differential channels for the 3 diagnostic
> inputs.
Sure, I'll add all the 15 channels.
>
>> +};
>> +
>> +static int ads1120_write_cmd(struct ads1120_state *st, u8 cmd)
>> +{
>> +	st->data[0] = cmd;
>> +	return spi_write(st->spi, st->data, 1);
>> +}
>> +
>> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
>> +{
>> +	u8 buf[2];
>> +
>> +	if (reg > ADS1120_REG_CONFIG3)
>> +		return -EINVAL;
>> +
>> +	buf[0] = ADS1120_CMD_WREG | (reg << 2);
>> +	buf[1] = value;
>> +
>> +	return spi_write(st->spi, buf, 2);
>> +}
> Can we use the regmap framework instead of writing our own?

I’d like to keep the first version simple so i will add the regmap 
support in the

later patch since the single ended has less spi transaction to handle.

>
>> +
>> +static int ads1120_reset(struct ads1120_state *st)
>> +{
>> +	int ret;
>> +
>> +	ret = ads1120_write_cmd(st, ADS1120_CMD_RESET);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Datasheet specifies reset takes 50us + 32 * t(CLK)
>> +	 * where t(CLK) = 1/4.096MHz. Use 200us to be safe.
>> +	 */
>> +	usleep_range(200, 300);
> 	fsleep(200);
I'll use fsleep instead of usleep_range.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_set_channel(struct ads1120_state *st, int channel)
>> +{
>> +	u8 mux_val;
>> +	u8 config0;
>> +
>> +	if (channel < 0 || channel > 3)
>> +		return -EINVAL;
>> +
>> +	/* Map channel to AINx/AVSS single-ended input */
>> +	mux_val = ADS1120_MUX_AIN0_AVSS + (channel << 4);
>> +
>> +	config0 = (st->config[0] & ~ADS1120_MUX_MASK) | mux_val;
>> +	st->config[0] = config0;
>> +
>> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
>> +}
>> +
>> +static int ads1120_set_gain(struct ads1120_state *st, int gain_val)
>> +{
>> +	u8 gain_bits;
>> +	u8 config0;
>> +	int i;
>> +
>> +	/* Find gain in supported values */
>> +	for (i = 0; i < ARRAY_SIZE(ads1120_gain_values); i++) {
>> +		if (ads1120_gain_values[i] == gain_val) {
>> +			gain_bits = i << 1;
>> +			goto found;
>> +		}
>> +	}
>> +	return -EINVAL;
>> +
>> +found:
>> +	config0 = (st->config[0] & ~ADS1120_GAIN_MASK) | gain_bits;
>> +	st->config[0] = config0;
>> +	st->gain = gain_val;
>> +
>> +	return ads1120_write_reg(st, ADS1120_REG_CONFIG0, config0);
>> +}
>> +
>> +static int ads1120_set_datarate(struct ads1120_state *st, int rate)
>> +{
>> +	u8 config1;
>> +	int i;
>> +
>> +	/* Find data rate in supported values */
>> +	for (i = 0; i < ARRAY_SIZE(ads1120_datarates); i++) {
>> +		if (ads1120_datarates[i].rate == rate) {
>> +			config1 = (st->config[1] & ~ADS1120_DR_MASK) |
>> +				  ads1120_datarates[i].reg_value;
>> +			st->config[1] = config1;
>> +			st->datarate = rate;
>> +			st->conv_time_ms = ads1120_datarates[i].conv_time_ms;
>> +
>> +			return ads1120_write_reg(st, ADS1120_REG_CONFIG1,
>> +						 config1);
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int ads1120_read_raw_adc(struct ads1120_state *st, int *val)
>> +{
>> +	u8 rx_buf[4];
>> +	u8 tx_buf[4] = { ADS1120_CMD_RDATA, 0xff, 0xff, 0xff };
>> +	int ret;
>> +	struct spi_transfer xfer = {
>> +		.tx_buf = tx_buf,
>> +		.rx_buf = rx_buf,
>> +		.len = 4,
>> +	};
> This should be split into two transfers (still only one SPI message).
> Then we don't need to pad the buffers. Also, it seems to have one
> more byte than needed. Only to to tx one byte then rx two bytes.
Sure, I'll fix.
>> +
>> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Data format: 16-bit two's complement MSB first
>> +	 * rx_buf[0] is echo of command
>> +	 * rx_buf[1] is MSB of data
>> +	 * rx_buf[2] is LSB of data
>> +	 */
>> +	*val = (s16)((rx_buf[1] << 8) | rx_buf[2]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_read_measurement(struct ads1120_state *st, int channel,
>> +				    int *val)
>> +{
>> +	int ret;
>> +
>> +	ret = ads1120_set_channel(st, channel);
>> +	if (ret)
>> +		return ret;
>> +
>
>
>> +	/* Start single-shot conversion */
>> +	ret = ads1120_write_cmd(st, ADS1120_CMD_START);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for conversion to complete */
>> +	msleep(st->conv_time_ms);
>> +
>> +	/* Read the result */
>> +	ret = ads1120_read_raw_adc(st, val);
>> +	if (ret)
>> +		return ret;
>> +
> This could actually all be done in a single SPI message with 3
> transers. The spi_transfer struct has delay fields that can
> provide the delay instead of msleep().
Good point. I'll defer this improvement to a subsequent patch.
>
>> +	st->current_channel = channel;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long mask)
>> +{
>> +	struct ads1120_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&st->lock);
>> +		ret = ads1120_read_measurement(st, chan->channel, val);
>> +		mutex_unlock(&st->lock);
>> +		if (ret)
>> +			return ret;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = st->gain;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = st->datarate;
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ads1120_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	struct ads1120_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		mutex_lock(&st->lock);
>> +		ret = ads1120_set_gain(st, val);
>> +		mutex_unlock(&st->lock);
>> +		return ret;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		mutex_lock(&st->lock);
>> +		ret = ads1120_set_datarate(st, val);
>> +		mutex_unlock(&st->lock);
>> +		return ret;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ads1120_read_avail(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      const int **vals, int *type, int *length,
>> +			      long mask)
>> +{
>> +	static const int datarates[] = { 20, 45, 90, 175, 330, 600, 1000 };
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*vals = ads1120_gain_values;
>> +		*type = IIO_VAL_INT;
>> +		*length = ARRAY_SIZE(ads1120_gain_values);
> Scale also depends on the reference voltage, so it isn't quite so simple.
Yeah sure. I'll fix.
>
>> +		return IIO_AVAIL_LIST;
>> +
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*vals = datarates;
>> +		*type = IIO_VAL_INT;
>> +		*length = ARRAY_SIZE(datarates);
>> +		return IIO_AVAIL_LIST;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct iio_info ads1120_info = {
>> +	.read_raw = ads1120_read_raw,
>> +	.write_raw = ads1120_write_raw,
>> +	.read_avail = ads1120_read_avail,
>> +};
>> +
>> +static int ads1120_init(struct ads1120_state *st)
>> +{
>> +	int ret;
>> +
>> +	/* Reset device to default state */
>> +	ret = ads1120_reset(st);
>> +	if (ret) {
>> +		dev_err(&st->spi->dev, "Failed to reset device\n");
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Configure Register 0:
>> +	 * - Input MUX: AIN0/AVSS (updated per channel read)
>> +	 * - Gain: 1
>> +	 * - PGA bypassed (lower power consumption)
> Should extend the comment to say that if gain is set higher than 4,
> this value is ignored, so it is safe to leave it set all the time.
Noted.
>
>> +	 */
>> +	st->config[0] = ADS1120_MUX_AIN0_AVSS | ADS1120_GAIN_1 |
>> +			ADS1120_PGA_BYPASS;
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG0, st->config[0]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Configure Register 1:
>> +	 * - Data rate: 175 SPS
>> +	 * - Mode: Normal
>> +	 * - Conversion mode: Single-shot
>> +	 * - Temperature sensor: Disabled
>> +	 * - Burnout current: Disabled
>> +	 */
>> +	st->config[1] = ADS1120_DR_175SPS | ADS1120_MODE_NORMAL |
>> +			ADS1120_CM_SINGLE;
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG1, st->config[1]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Configure Register 2:
>> +	 * - Voltage reference: AVDD
>> +	 * - 50/60Hz rejection: Off
>> +	 * - Power switch: Off
>> +	 * - IDAC: Off
>> +	 */
>> +	st->config[2] = ADS1120_VREF_AVDD | ADS1120_REJECT_OFF;
>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG2, st->config[2]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Configure Register 3:
> 	 * - Internal voltage reference
> 	 * - No FIR filter
> 	 * - Power switch always open
>
>> +	 * - IDAC1: Disabled
>> +	 * - IDAC2: Disabled
>> +	 * - DRDY mode: DOUT/DRDY pin behavior
>> +	 */
>> +	st->config[3] = ADS1120_DRDYM_EN;
> It doesn't make sense to enable the DRDY pin on the DOUT line unless
> we know that it is wired up to an interrupt.

Thanks for pointing it out. I'll address the this in V2 patch.

>> +	ret = ads1120_write_reg(st, ADS1120_REG_CONFIG3, st->config[3]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set default operating parameters */
>> +	st->gain = ADS1120_DEFAULT_GAIN;
>> +	st->datarate = ADS1120_DEFAULT_DATARATE;
>> +	st->conv_time_ms = 7; /* For 175 SPS */
>> +	st->current_channel = -1;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ads1120_probe(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct ads1120_state *st;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +	st->spi = spi;
>> +
>> +	mutex_init(&st->lock);
>> +
>> +	indio_dev->name = "ads1120";
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ads1120_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ads1120_channels);
>> +	indio_dev->info = &ads1120_info;
>> +
>> +	ret = ads1120_init(st);
>> +	if (ret) {
>> +		dev_err(&spi->dev, "Failed to initialize device: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return devm_iio_device_register(&spi->dev, indio_dev);
>> +}
>> +
>> +static const struct of_device_id ads1120_of_match[] = {
>> +	{ .compatible = "ti,ads1120" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ads1120_of_match);
>> +
>> +static const struct spi_device_id ads1120_id[] = {
>> +	{ "ads1120" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, ads1120_id);
>> +
>> +static struct spi_driver ads1120_driver = {
>> +	.driver = {
>> +		.name = "ads1120",
>> +		.of_match_table = ads1120_of_match,
>> +	},
>> +	.probe = ads1120_probe,
>> +	.id_table = ads1120_id,
>> +};
>> +module_spi_driver(ads1120_driver);
>> +
>> +MODULE_AUTHOR("Ajith Anandhan  <ajithanandhan0406@gmail.com>");
>> +MODULE_DESCRIPTION("Texas Instruments ADS1120 ADC driver");
>> +MODULE_LICENSE("GPL");

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[David Lechner]

On 11/7/25 9:50 AM, Ajith Anandhan wrote:
> On 10/31/25 2:43 AM, David Lechner wrote:
>> On 10/30/25 11:34 AM, Ajith Anandhan wrote:
>>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>>> analog-to-digital converter with an SPI interface.
>>>

One note about the review process. Any suggestions you agree with, you
don't need to reply to specifically. You can trim out those parts in
your reply. It saves time for those reading the replies.

>>> +struct ads1120_state {
>>> +    struct spi_device    *spi;
>>> +    struct mutex        lock;
>>> +    /*
>>> +     * Used to correctly align data.
>>> +     * Ensure natural alignment for potential future timestamp support.
>>> +     */
>>> +    u8 data[4] __aligned(IIO_DMA_MINALIGN);
>>> +
>>> +    u8 config[4];
>>> +    int current_channel;
>>> +    int gain;
>> Since inputs are multiplexed, we can make this gain a per-channel value, no?
> 
> Yes we can, However i want  to keep the initial version simple so would it be
> 
> fine to support per-channel gain configurationin upcoming patches?

Absolutely. I really appreciate splitting things up like that as it makes
it much easier to review.

> 
>>
>> It also sounds like that certain mux input have to have the PGA bypassed
>> which means they are limited to only 3 gain values. So these would have
>> a different scale_available attribute.
> 
>  Since, I'm gonna enable all the 15 channels. I believe we have to have all
> 
> gains(for differential channels). Correct me if i'm wrong.

Yes, that is how I understood the datasheet. Differential channels have all
gains. Single-ended channels and diagnostic channels only get the low gains
(1, 2, 4).


>>> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
>>> +{
>>> +    u8 buf[2];
>>> +
>>> +    if (reg > ADS1120_REG_CONFIG3)
>>> +        return -EINVAL;
>>> +
>>> +    buf[0] = ADS1120_CMD_WREG | (reg << 2);
>>> +    buf[1] = value;
>>> +
>>> +    return spi_write(st->spi, buf, 2);
>>> +}
>> Can we use the regmap framework instead of writing our own?
> 
> I’d like to keep the first version simple so i will add the regmap support in the
> 
> later patch since the single ended has less spi transaction to handle.

It would be less churn to implement the regmap right away. Typically
we try to avoid churn if we can help it. So this would be an exception
to the general suggestion that splitting things up is better.

Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

[Ajith Anandhan]

On 11/7/25 9:48 PM, David Lechner wrote:
> On 11/7/25 9:50 AM, Ajith Anandhan wrote:
>> On 10/31/25 2:43 AM, David Lechner wrote:
>>> On 10/30/25 11:34 AM, Ajith Anandhan wrote:
>>>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>>>> analog-to-digital converter with an SPI interface.
>>>>
> One note about the review process. Any suggestions you agree with, you
> don't need to reply to specifically. You can trim out those parts in
> your reply. It saves time for those reading the replies.
>
>>>> +struct ads1120_state {
>>>> +    struct spi_device    *spi;
>>>> +    struct mutex        lock;
>>>> +    /*
>>>> +     * Used to correctly align data.
>>>> +     * Ensure natural alignment for potential future timestamp support.
>>>> +     */
>>>> +    u8 data[4] __aligned(IIO_DMA_MINALIGN);
>>>> +
>>>> +    u8 config[4];
>>>> +    int current_channel;
>>>> +    int gain;
>>> Since inputs are multiplexed, we can make this gain a per-channel value, no?
>> Yes we can, However i want  to keep the initial version simple so would it be
>>
>> fine to support per-channel gain configurationin upcoming patches?
> Absolutely. I really appreciate splitting things up like that as it makes
> it much easier to review.
>
>>> It also sounds like that certain mux input have to have the PGA bypassed
>>> which means they are limited to only 3 gain values. So these would have
>>> a different scale_available attribute.
>>   Since, I'm gonna enable all the 15 channels. I believe we have to have all
>>
>> gains(for differential channels). Correct me if i'm wrong.
> Yes, that is how I understood the datasheet. Differential channels have all
> gains. Single-ended channels and diagnostic channels only get the low gains
> (1, 2, 4).
>
>
>>>> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
>>>> +{
>>>> +    u8 buf[2];
>>>> +
>>>> +    if (reg > ADS1120_REG_CONFIG3)
>>>> +        return -EINVAL;
>>>> +
>>>> +    buf[0] = ADS1120_CMD_WREG | (reg << 2);
>>>> +    buf[1] = value;
>>>> +
>>>> +    return spi_write(st->spi, buf, 2);
>>>> +}
>>> Can we use the regmap framework instead of writing our own?
>> I’d like to keep the first version simple so i will add the regmap support in the
>>
>> later patch since the single ended has less spi transaction to handle.
> It would be less churn to implement the regmap right away. Typically
> we try to avoid churn if we can help it. So this would be an exception
> to the general suggestion that splitting things up is better.


Got it, I’ll add regmap support and address all review comments in the v2
patch series.

BR,

Ajith.

[RFC PATCH 3/3] MAINTAINERS: Add entry for TI ADS1120 ADC driver

[Ajith Anandhan]

Add a new MAINTAINERS entry for the Texas Instruments ADS1120
ADC driver and its device tree binding.
Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3da2c26a7..1efe88fc9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25613,6 +25613,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
 F:	drivers/iio/adc/ti-ads1119.c
 
+TI ADS1120 ADC DRIVER
+M:	Ajith Anandhan <ajithanandhan0406@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
+F:	drivers/iio/adc/ti-ads1120.c
+
 TI ADS7924 ADC DRIVER
 M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
 L:	linux-iio@vger.kernel.org
-- 
2.34.1

Re: [RFC PATCH 3/3] MAINTAINERS: Add entry for TI ADS1120 ADC driver

[Jonathan Cameron]

On Thu, 30 Oct 2025 22:04:11 +0530
Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:

> Add a new MAINTAINERS entry for the Texas Instruments ADS1120
> ADC driver and its device tree binding.
blank line before tag block.
> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>

Just bring this in along with the code, it doesn't need a separate
commit.

Thanks,

Jonathan
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3da2c26a7..1efe88fc9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25613,6 +25613,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
>  F:	drivers/iio/adc/ti-ads1119.c
>  
> +TI ADS1120 ADC DRIVER
> +M:	Ajith Anandhan <ajithanandhan0406@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
> +F:	drivers/iio/adc/ti-ads1120.c
> +
>  TI ADS7924 ADC DRIVER
>  M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
>  L:	linux-iio@vger.kernel.org

Re: [RFC PATCH 3/3] MAINTAINERS: Add entry for TI ADS1120 ADC driver

[Ajith Anandhan]

On 10/30/25 11:25 PM, Jonathan Cameron wrote:
> On Thu, 30 Oct 2025 22:04:11 +0530
> Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:
>
>> Add a new MAINTAINERS entry for the Texas Instruments ADS1120
>> ADC driver and its device tree binding.
> blank line before tag block.
Noted.
>> Signed-off-by: Ajith Anandhan <ajithanandhan0406@gmail.com>
> Just bring this in along with the code, it doesn't need a separate
> commit.
>
> Thanks,
>
> Jonathan

I will add along with the code.

>> ---
>>   MAINTAINERS | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3da2c26a7..1efe88fc9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -25613,6 +25613,13 @@ S:	Maintained
>>   F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
>>   F:	drivers/iio/adc/ti-ads1119.c
>>   
>> +TI ADS1120 ADC DRIVER
>> +M:	Ajith Anandhan <ajithanandhan0406@gmail.com>
>> +L:	linux-iio@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/iio/adc/ti,ads1120.yaml
>> +F:	drivers/iio/adc/ti-ads1120.c
>> +
>>   TI ADS7924 ADC DRIVER
>>   M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
>>   L:	linux-iio@vger.kernel.org

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Krzysztof Kozlowski]

On 30/10/2025 17:34, Ajith Anandhan wrote:
> This RFC patch series adds support for the Texas Instruments ADS1120,
> a precision 16-bit delta-sigma ADC with SPI interface.
> 
> The driver provides:
> - 4 single-ended voltage input channels
> - Programmable gain amplifier (1 to 128)
> - Configurable data rates (20 to 1000 SPS)
> - Single-shot conversion mode
> 
> I'm looking for feedback on:
> 1. The implementation approach for single-shot conversions
> 2. Any other suggestions for improvement


No need to call your patches RFC then. It only stops from merging and
some people will not review the code (RFC means not ready for inclusion).

Best regards,
Krzysztof

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Ajith Anandhan]

Hi Krzysztof,

Thank you for the feedback! I’ll resend this as a regular PATCH series
shortly. I appreciate you taking the time to review.

Best regards,
Ajith

On Thu, Oct 30, 2025 at 10:19 PM Ajith Anandhan
<ajithanandhan0406@gmail.com> wrote:
>
> Thank you for the feedback! I’ll resend this as regular PATCH series shortly. I appreciate you taking the time to review.
>
> Best regards,
> Ajith
>
> On Thu, Oct 30, 2025 at 10:10 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 30/10/2025 17:34, Ajith Anandhan wrote:
>> > This RFC patch series adds support for the Texas Instruments ADS1120,
>> > a precision 16-bit delta-sigma ADC with SPI interface.
>> >
>> > The driver provides:
>> > - 4 single-ended voltage input channels
>> > - Programmable gain amplifier (1 to 128)
>> > - Configurable data rates (20 to 1000 SPS)
>> > - Single-shot conversion mode
>> >
>> > I'm looking for feedback on:
>> > 1. The implementation approach for single-shot conversions
>> > 2. Any other suggestions for improvement
>>
>>
>> No need to call your patches RFC then. It only stops from merging and
>> some people will not review the code (RFC means not ready for inclusion).
>>
>> Best regards,
>> Krzysztof

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Jonathan Cameron]

On Thu, 30 Oct 2025 22:28:10 +0530
Ajith Anandhan <ajithanandhan0406@gmail.com> wrote:

> Hi Krzysztof,
> 
> Thank you for the feedback! I’ll resend this as a regular PATCH series
> shortly. I appreciate you taking the time to review.
Given you've sent it out, leave it as RFC for a day or so.

> 
> Best regards,
> Ajith
> 
> On Thu, Oct 30, 2025 at 10:19 PM Ajith Anandhan
> <ajithanandhan0406@gmail.com> wrote:
> >
> > Thank you for the feedback! I’ll resend this as regular PATCH series shortly. I appreciate you taking the time to review.
> >
> > Best regards,
> > Ajith
> >
> > On Thu, Oct 30, 2025 at 10:10 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:  
> >>
> >> On 30/10/2025 17:34, Ajith Anandhan wrote:  
> >> > This RFC patch series adds support for the Texas Instruments ADS1120,
> >> > a precision 16-bit delta-sigma ADC with SPI interface.
> >> >
> >> > The driver provides:
> >> > - 4 single-ended voltage input channels
> >> > - Programmable gain amplifier (1 to 128)
> >> > - Configurable data rates (20 to 1000 SPS)
> >> > - Single-shot conversion mode
> >> >
> >> > I'm looking for feedback on:
> >> > 1. The implementation approach for single-shot conversions
> >> > 2. Any other suggestions for improvement  
> >>
> >>
> >> No need to call your patches RFC then. It only stops from merging and
> >> some people will not review the code (RFC means not ready for inclusion).
> >>
> >> Best regards,
> >> Krzysztof  
> 

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Krzysztof Kozlowski]

On 30/10/2025 17:49, Ajith Anandhan wrote:
> Thank you for the feedback! I’ll resend this as regular PATCH series
> shortly. I appreciate you taking the time to review.

Please avoid top-posting.

Don't resend now. Next version will be v2 and send it after you receive
review.

Best regards,
Krzysztof

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 10:04:08PM +0530, Ajith Anandhan wrote:
> This RFC patch series adds support for the Texas Instruments ADS1120,
> a precision 16-bit delta-sigma ADC with SPI interface.
> 
> The driver provides:
> - 4 single-ended voltage input channels
> - Programmable gain amplifier (1 to 128)
> - Configurable data rates (20 to 1000 SPS)
> - Single-shot conversion mode
> 
> I'm looking for feedback on:
> 1. The implementation approach for single-shot conversions
> 2. Any other suggestions for improvement
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1120

The cover letter missed to answer the Q: Why a new driver? Have you checked the
existing drivers? Do we have a similar enough one that may be extended to
support this chip?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Ajith Anandhan]

On 10/31/25 2:07 PM, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 10:04:08PM +0530, Ajith Anandhan wrote:
>> This RFC patch series adds support for the Texas Instruments ADS1120,
>> a precision 16-bit delta-sigma ADC with SPI interface.
>>
>> The driver provides:
>> - 4 single-ended voltage input channels
>> - Programmable gain amplifier (1 to 128)
>> - Configurable data rates (20 to 1000 SPS)
>> - Single-shot conversion mode
>>
>> I'm looking for feedback on:
>> 1. The implementation approach for single-shot conversions
>> 2. Any other suggestions for improvement
>>
>> Datasheet: https://www.ti.com/lit/gpn/ads1120
> The cover letter missed to answer the Q: Why a new driver? Have you checked the
> existing drivers? Do we have a similar enough one that may be extended to
> support this chip?
>
Hi Andy,

Thank you for the feedback.

I evaluated the following existing driver before creating a new one:

ads124s08.c - TI ADS124S08

- This is the closest match (both are delta-sigma, SPI-based)

- However, significant differences exist:

     * Different register layout (ADS124S08 has more registers)

     * Different command set ADS124S08 has built-in MUX for differential 
inputs

     * Different register addressing and bit fields and conversion 
timing and data retrieval.

would require extensive conditional code paths that might reduce 
maintainability for both devices. A separate, focused driver seemed cleaner.

BR,
Ajith Anandhan.

Re: [RFC PATCH 0/3] iio: adc: Add support for TI ADS1120 ADC

[Andy Shevchenko]

On Sat, Nov 01, 2025 at 05:07:38PM +0530, Ajith Anandhan wrote:
> On 10/31/25 2:07 PM, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 10:04:08PM +0530, Ajith Anandhan wrote:
> > > This RFC patch series adds support for the Texas Instruments ADS1120,
> > > a precision 16-bit delta-sigma ADC with SPI interface.
> > > 
> > > The driver provides:
> > > - 4 single-ended voltage input channels
> > > - Programmable gain amplifier (1 to 128)
> > > - Configurable data rates (20 to 1000 SPS)
> > > - Single-shot conversion mode
> > > 
> > > I'm looking for feedback on:
> > > 1. The implementation approach for single-shot conversions
> > > 2. Any other suggestions for improvement
> > > 
> > > Datasheet: https://www.ti.com/lit/gpn/ads1120
> > The cover letter missed to answer the Q: Why a new driver? Have you checked the
> > existing drivers? Do we have a similar enough one that may be extended to
> > support this chip?
> > 
> Thank you for the feedback.
> 
> I evaluated the following existing driver before creating a new one:
> 
> ads124s08.c - TI ADS124S08
> 
> - This is the closest match (both are delta-sigma, SPI-based)
> 
> - However, significant differences exist:
> 
>     * Different register layout (ADS124S08 has more registers)
> 
>     * Different command set ADS124S08 has built-in MUX for differential
> inputs
> 
>     * Different register addressing and bit fields and conversion timing and
> data retrieval.
> 
> would require extensive conditional code paths that might reduce
> maintainability for both devices. A separate, focused driver seemed cleaner.

Good, please add this summary to the cover letter of next version.

-- 
With Best Regards,
Andy Shevchenko