[PATCH v2 0/2] iio: adc: ti-ads1119: Add driver

[PATCH v2 0/2] iio: adc: ti-ads1119: Add driver

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
that features two differential or four single-ended inputs through a
flexible input multiplexer (MUX), rail-to-rail input
buffers, a programmable gain stage, a voltage reference, and an
oscillator.

Apart from normal single conversion, the driver also supports
continuous conversion mode using a triggered buffer. However, in this
mode only one channel can be scanned at a time, and it only uses the data
ready interrupt as a trigger. This is because the device channels are
multiplexed, and using its own data ready interrupt as a trigger guarantees
the signal sampling frequency.

v2:
 bindings:
 - add diff-channels and single-channel
 - remove ti,gain and ti,datarate
 - more details on the patch changelog

 driver:
 - adaptation for the binding changes
 - changed i2c read/write to the device (not using regmap anymore, use FIELD macros)
 - more details on the patch changelog

v1: https://lore.kernel.org/all/20240527154050.24975-1-francesco@dolcini.it/

João Paulo Gonçalves (2):
  dt-bindings: iio: adc: add ti,ads1119
  iio: adc: ti-ads1119: Add driver

 .../bindings/iio/adc/ti,ads1119.yaml          | 148 +++
 MAINTAINERS                                   |   8 +
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ti-ads1119.c                  | 850 ++++++++++++++++++
 5 files changed, 1019 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
 create mode 100644 drivers/iio/adc/ti-ads1119.c

-- 
2.39.2

[PATCH v2 1/2] dt-bindings: iio: adc: add ti,ads1119

[Francesco Dolcini]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
with I2C interface.

Datasheet: https://www.ti.com/lit/gpn/ads1119
Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2:
 - add diff-channels and single-channel
 - add XOR check to make diff/single channel property required 
 - add interrupts, reset-gpios and vref-supply to the example 
 - fix missing additionalProperties/unevaluatedProperties warning in channels
 - remove ti,gain and ti,datarate as they aren't fixed hw properties
 - remove unnecessary | 
---
 .../bindings/iio/adc/ti,ads1119.yaml          | 148 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 155 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
new file mode 100644
index 000000000000..cbf0d4ef3a11
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
@@ -0,0 +1,148 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1119.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ADS1119 ADC
+
+maintainers:
+  - João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
+
+description:
+  The TI ADS1119 is a precision 16-bit ADC over I2C that offers single-ended and
+  differential measurements using a multiplexed input. It features a programmable
+  gain, a programmable sample rate, an internal oscillator and voltage reference,
+  and a 50/60Hz rejection filter.
+
+properties:
+  compatible:
+    const: ti,ads1119
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      ADC external reference voltage (VREF).
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+patternProperties:
+  "^channel@([0-6])$":
+    $ref: adc.yaml
+    type: object
+    properties:
+      reg:
+        minimum: 0
+        maximum: 6
+
+      diff-channels:
+        description:
+          Differential input channels AIN0-AIN1, AIN2-AIN3 and AIN1-AIN2.
+        oneOf:
+          - items:
+              - const: 0
+              - const: 1
+          - items:
+              - const: 2
+              - const: 3
+          - items:
+              - const: 1
+              - const: 2
+
+      single-channel:
+        description:
+          Single-ended input channels AIN0, AIN1, AIN2 and AIN3.
+        minimum: 0
+        maximum: 3
+
+    oneOf:
+      - required:
+          - diff-channels
+      - required:
+          - single-channel
+
+    required:
+      - reg
+
+    unevaluatedProperties: false
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@40 {
+            compatible = "ti,ads1119";
+            reg = <0x40>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
+            vref-supply = <&reg_vref_ads1119>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            #io-channel-cells = <1>;
+
+            channel@0 {
+                reg = <0>;
+                single-channel = <0>;
+            };
+
+            channel@1 {
+                reg = <1>;
+                diff-channels = <0 1>;
+            };
+
+            channel@2 {
+                reg = <2>;
+                single-channel = <3>;
+            };
+
+            channel@3 {
+                reg = <3>;
+                single-channel = <1>;
+            };
+
+            channel@4 {
+                reg = <4>;
+                single-channel = <2>;
+            };
+
+            channel@5 {
+                reg = <5>;
+                diff-channels = <1 2>;
+            };
+
+            channel@6 {
+                reg = <6>;
+                diff-channels = <2 3>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..f1b2c4b815e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22380,6 +22380,13 @@ M:	Robert Richter <rric@kernel.org>
 S:	Odd Fixes
 F:	drivers/gpio/gpio-thunderx.c
 
+TI ADS1119 ADC DRIVER
+M:	Francesco Dolcini <francesco@dolcini.it>
+M:	João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
+
 TI ADS7924 ADC DRIVER
 M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
 L:	linux-iio@vger.kernel.org
-- 
2.39.2

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add ti,ads1119

[Conor Dooley]

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

On Thu, Jun 06, 2024 at 06:35:28PM +0200, Francesco Dolcini wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> with I2C interface.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2:
>  - add diff-channels and single-channel
>  - add XOR check to make diff/single channel property required 
>  - add interrupts, reset-gpios and vref-supply to the example 
>  - fix missing additionalProperties/unevaluatedProperties warning in channels
>  - remove ti,gain and ti,datarate as they aren't fixed hw properties
>  - remove unnecessary | 

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add ti,ads1119

[Rob Herring]

On Thu, Jun 06, 2024 at 06:35:28PM +0200, Francesco Dolcini wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> with I2C interface.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> v2:
>  - add diff-channels and single-channel

Running the checks gives this error:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml: single-channel: missing type definition

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add ti,ads1119

[Francesco Dolcini]

Hello Rob,

On Thu, Jun 06, 2024 at 02:49:24PM -0600, Rob Herring wrote:
> On Thu, Jun 06, 2024 at 06:35:28PM +0200, Francesco Dolcini wrote:
> > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > 
> > Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> > with I2C interface.
> > 
> > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> > v2:
> >  - add diff-channels and single-channel
> 
> Running the checks gives this error:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml: single-channel: missing type definition

It's defined starting from commit dd471a2b7759 ("dt-bindings: iio: adc: Add single-channel property")
that is part of the IIO branch (and linux-next).

Francesco

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add ti,ads1119

[Jonathan Cameron]

On Thu,  6 Jun 2024 18:35:28 +0200
Francesco Dolcini <francesco@dolcini.it> wrote:

> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> with I2C interface.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>

I missed it on previous version but you only have description in here
for vref and few devices power them selves from something called vref!

The binding should describe the other power supplies as well and mark
them as required.

We've left these out too many times in the past and ended up having
a patch very soon after adding them. Better to have them from the start.
Driver should just use devm_regulator_get_enabled() to turn them on and
register them to be turned off on driver removal, and ignore them
after that.  If anyone has controlled supplies and wants to do more
sophisticated handling they can add it later.

Note that even though the dt-binding will list them as required, if
a particular DTS doesn't provide them the regulator framework will
give a dummy uncontrolled regulator to represent the assumption that
the power is always there.  We still document them as required in
the dt-binding though as other OS may not be so clever.

Only needs minimal entries though - see inline

Jonathan

> ---
> v2:
>  - add diff-channels and single-channel
>  - add XOR check to make diff/single channel property required 
>  - add interrupts, reset-gpios and vref-supply to the example 
>  - fix missing additionalProperties/unevaluatedProperties warning in channels
>  - remove ti,gain and ti,datarate as they aren't fixed hw properties
>  - remove unnecessary | 
> ---
>  .../bindings/iio/adc/ti,ads1119.yaml          | 148 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 155 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> new file mode 100644
> index 000000000000..cbf0d4ef3a11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> @@ -0,0 +1,148 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1119.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments ADS1119 ADC
> +
> +maintainers:
> +  - João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
> +
> +description:
> +  The TI ADS1119 is a precision 16-bit ADC over I2C that offers single-ended and
> +  differential measurements using a multiplexed input. It features a programmable
> +  gain, a programmable sample rate, an internal oscillator and voltage reference,
> +  and a 50/60Hz rejection filter.
> +
> +properties:
> +  compatible:
> +    const: ti,ads1119
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
avdd-supply: true
dvdd-supply: true

> +  vref-supply:
> +    description:
> +      ADC external reference voltage (VREF).
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
    - avdd-supply
    - dvdd-supply

> +
> +patternProperties:
> +  "^channel@([0-6])$":
> +    $ref: adc.yaml
> +    type: object
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 6
> +
> +      diff-channels:
> +        description:
> +          Differential input channels AIN0-AIN1, AIN2-AIN3 and AIN1-AIN2.
> +        oneOf:
> +          - items:
> +              - const: 0
> +              - const: 1
> +          - items:
> +              - const: 2
> +              - const: 3
> +          - items:
> +              - const: 1
> +              - const: 2
> +
> +      single-channel:
> +        description:
> +          Single-ended input channels AIN0, AIN1, AIN2 and AIN3.
> +        minimum: 0
> +        maximum: 3
> +
> +    oneOf:
> +      - required:
> +          - diff-channels
> +      - required:
> +          - single-channel
> +
> +    required:
> +      - reg
> +
> +    unevaluatedProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@40 {
> +            compatible = "ti,ads1119";
> +            reg = <0x40>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> +            reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
> +            vref-supply = <&reg_vref_ads1119>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            #io-channel-cells = <1>;
> +
> +            channel@0 {
> +                reg = <0>;
> +                single-channel = <0>;
> +            };
> +
> +            channel@1 {
> +                reg = <1>;
> +                diff-channels = <0 1>;
> +            };
> +
> +            channel@2 {
> +                reg = <2>;
> +                single-channel = <3>;
> +            };
> +
> +            channel@3 {
> +                reg = <3>;
> +                single-channel = <1>;
> +            };
> +
> +            channel@4 {
> +                reg = <4>;
> +                single-channel = <2>;
> +            };
> +
> +            channel@5 {
> +                reg = <5>;
> +                diff-channels = <1 2>;
> +            };
> +
> +            channel@6 {
> +                reg = <6>;
> +                diff-channels = <2 3>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d6c90161c7bf..f1b2c4b815e2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22380,6 +22380,13 @@ M:	Robert Richter <rric@kernel.org>
>  S:	Odd Fixes
>  F:	drivers/gpio/gpio-thunderx.c
>  
> +TI ADS1119 ADC DRIVER
> +M:	Francesco Dolcini <francesco@dolcini.it>
> +M:	João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> +
>  TI ADS7924 ADC DRIVER
>  M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
>  L:	linux-iio@vger.kernel.org

[PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

[Francesco Dolcini]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
that features two differential or four single-ended inputs through a
flexible input multiplexer (MUX), rail-to-rail input
buffers, a programmable gain stage, a voltage reference, and an
oscillator.

Apart from normal single conversion, the driver also supports
continuous conversion mode using a triggered buffer. However, in this
mode only one channel can be scanned at a time, and it only uses the data
ready interrupt as a trigger. This is because the device channels are
multiplexed, and using its own data ready interrupt as a trigger guarantees
the signal sampling frequency.

Datasheet: https://www.ti.com/lit/gpn/ads1119
Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
v2:
 - add 2 functions to wr/rd regs values: ads1119_update_config_reg and ads1119_data_ready
 - add FIELD(PREP, GET) for bitmasks
 - add driver string inline
 - add devm_regulator_get_enable_read_voltage to handle external vref
 - add comment for resume function
 - add userspace configuration of gain/datarate
 - changed bitmasks names to FIELD suffix
 - changed channel allocation to handle diff-channels/single-channel
 - changed single conversion to power down on error
 - changed to create scan masks dynamically to avoid warnings
 - fix ret=0 in ads1119_read_data()
 - fix irq trigger handler error handling order
 - removed regmap from code and KCONFIG
 - removed of_match_ptr()
 - removed ads1119_get_configs() and put inlined on probe
 - removed static allocation of iio channels
 - removed ads1119_set_power() and called it inline on necessary places
 - removed not necessary bitmasks and macros
 - removed match data struct
 - removed set mux/gain/datarate functions and called them inline
---
 MAINTAINERS                  |   1 +
 drivers/iio/adc/Kconfig      |  12 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-ads1119.c | 850 +++++++++++++++++++++++++++++++++++
 4 files changed, 864 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads1119.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f1b2c4b815e2..ab9febe5dc9c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22386,6 +22386,7 @@ M:	João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
+F:	drivers/iio/adc/ti-ads1119.c
 
 TI ADS7924 ADC DRIVER
 M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 74fecc284f1a..3022b1ac93a0 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1334,6 +1334,18 @@ config TI_ADS1015
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1015.
 
+config TI_ADS1119
+       tristate "Texas Instruments ADS1119 ADC"
+       depends on I2C
+       select IIO_BUFFER
+       select IIO_TRIGGERED_BUFFER
+       help
+         If you say yes here you get support for Texas Instruments ADS1119
+         ADC chip.
+
+         This driver can also be built as a module. If so, the module will be
+         called ti-ads1119.
+
 config TI_ADS7924
 	tristate "Texas Instruments ADS7924 ADC"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 6a8c2da0e707..50cde66790cf 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -120,6 +120,7 @@ obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 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_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-ads1119.c b/drivers/iio/adc/ti-ads1119.c
new file mode 100644
index 000000000000..ea0573f07279
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1119.c
@@ -0,0 +1,850 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Texas Instruments ADS1119 ADC driver.
+ *
+ * Copyright 2024 Toradex
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define ADS1119_CMD_RESET		0x06
+#define ADS1119_CMD_POWERDOWN		0x02
+#define ADS1119_CMD_START_SYNC		0x08
+#define ADS1119_CMD_RDATA		0x10
+#define ADS1119_CMD_RREG(reg)		(0x20 | ((reg) << 2))
+#define ADS1119_CMD_WREG		0x40
+
+/* Config register */
+#define ADS1119_REG_CONFIG	0x00
+#define ADS1119_CONFIG_VREF_FIELD	BIT(0)
+#define ADS1119_CONFIG_CM_FIELD		BIT(1)
+#define ADS1119_CONFIG_DR_FIELD		GENMASK(3, 2)
+#define ADS1119_CONFIG_GAIN_FIELD	BIT(4)
+#define ADS1119_CONFIG_MUX_FIELD	GENMASK(7, 5)
+
+#define ADS1119_VREF_INTERNAL	0
+#define ADS1119_VREF_EXTERNAL	1
+#define ADS1119_VREF_INTERNAL_VAL 2048000
+
+#define ADS1119_CM_SINGLE	0
+#define ADS1119_CM_CONTINUOUS	1
+
+#define ADS1119_DR_20_SPS	0
+#define ADS1119_DR_90_SPS	1
+#define ADS1119_DR_330_SPS	2
+#define ADS1119_DR_1000_SPS	3
+
+#define ADS1119_GAIN_1	0
+#define ADS1119_GAIN_4	1
+
+#define ADS1119_MUX_AIN0_AIN1	0
+#define ADS1119_MUX_AIN2_AIN3	1
+#define ADS1119_MUX_AIN1_AIN2	2
+#define ADS1119_MUX_AIN0	3
+#define ADS1119_MUX_AIN1	4
+#define ADS1119_MUX_AIN2	5
+#define ADS1119_MUX_AIN3	6
+#define ADS1119_MUX_SHORTED	7
+
+/* Status register */
+#define ADS1119_REG_STATUS	0x01
+#define ADS1119_STATUS_DRDY_FIELD	BIT(7)
+
+#define ADS1119_DEFAULT_GAIN		1
+#define ADS1119_DEFAULT_DATARATE	20
+
+#define ADS1119_SUSPEND_DELAY		2000
+
+/* Timeout based on the minimum sample rate of 20 SPS (50000us) */
+#define ADS1119_MAX_DRDY_TIMEOUT	85000
+
+/*
+ * Static allocate buffer for scans assuming one channel sample (2 or 4 bytes)
+ * + _aligned(8) timestamp (8 bytes).
+ */
+#define ADS1119_SCAN_BUFFER_SIZE	16
+
+#define ADS1119_MAX_CHANNELS		7
+#define ADS1119_MAX_SINGLE_CHANNELS	4
+
+#define ADS1119_V_CHAN(_chan, _chan2, _addr) {			\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.address = _addr,					\
+	.channel = _chan,					\
+	.channel2 = _chan2,					\
+	.info_mask_separate =					\
+		BIT(IIO_CHAN_INFO_RAW) |			\
+		BIT(IIO_CHAN_INFO_SCALE) |			\
+		BIT(IIO_CHAN_INFO_OFFSET) |			\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.info_mask_shared_by_all_available =			\
+		BIT(IIO_CHAN_INFO_SCALE) |			\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ),			\
+	.scan_index = _addr,					\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 16,					\
+		.storagebits = 16,				\
+		.endianness = IIO_CPU,				\
+	},							\
+}
+
+struct ads1119_channel_config {
+	int gain;
+	int datarate;
+	int mux;
+};
+
+struct ads1119_state {
+	unsigned long scan_masks[ADS1119_MAX_CHANNELS + 1];
+	u8 scan_buf[ADS1119_SCAN_BUFFER_SIZE];
+	struct completion completion;
+	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
+	struct iio_trigger *trig;
+	struct ads1119_channel_config *channels_cfg;
+	unsigned int num_bytes_sample;
+	unsigned int num_channels_cfg;
+	int vref_uV;
+};
+
+static const int ads1119_available_datarates[] = {
+	20, 90, 330, 1000,
+};
+
+static const int ads1119_available_gains[] = {
+	1, 1,
+	1, 4,
+};
+
+static int ads1119_cmd(struct ads1119_state *st, unsigned int cmd)
+{
+	dev_dbg(&st->client->dev, "cmd: %#x\n", cmd);
+
+	return i2c_smbus_write_byte(st->client, cmd);
+}
+
+static int ads1119_cmd_wreg(struct ads1119_state *st, unsigned int val)
+{
+	dev_dbg(&st->client->dev, "wreg: %#x\n", val);
+
+	return i2c_smbus_write_byte_data(st->client, ADS1119_CMD_WREG, val);
+}
+
+static int ads1119_cmd_rreg(struct ads1119_state *st, unsigned int reg,
+			    unsigned int *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(st->client, ADS1119_CMD_RREG(reg));
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&st->client->dev, "rreg %#x: %#x\n", reg, ret);
+
+	*val = ret;
+
+	return 0;
+}
+
+static int ads1119_cmd_rdata(struct ads1119_state *st, unsigned int *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_swapped(st->client, ADS1119_CMD_RDATA);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&st->client->dev, "rdata: %#x\n", ret);
+
+	*val = ret;
+
+	return 0;
+}
+
+static int ads1119_update_config_reg(struct ads1119_state *st,
+				     unsigned int fields,
+				     unsigned int val)
+{
+	unsigned int config;
+	int ret;
+
+	ret = ads1119_cmd_rreg(st, ADS1119_REG_CONFIG, &config);
+	if (ret)
+		return ret;
+
+	config &= ~fields;
+	config |= val;
+
+	return ads1119_cmd_wreg(st, config);
+}
+
+static bool ads1119_data_ready(struct ads1119_state *st)
+{
+	unsigned int status;
+
+	if (!ads1119_cmd_rreg(st, ADS1119_REG_STATUS, &status))
+		return FIELD_GET(ADS1119_STATUS_DRDY_FIELD, status);
+
+	return 0;
+}
+
+static int ads1119_reset(struct ads1119_state *st)
+{
+	if (!st->reset_gpio)
+		return ads1119_cmd(st, ADS1119_CMD_RESET);
+
+	gpiod_set_value_cansleep(st->reset_gpio, 1);
+	udelay(1);
+	gpiod_set_value_cansleep(st->reset_gpio, 0);
+	udelay(1);
+
+	return 0;
+}
+
+static int ads1119_set_conv_mode(struct ads1119_state *st, bool continuous)
+{
+	unsigned int mode;
+
+	if (continuous)
+		mode = ADS1119_CM_CONTINUOUS;
+	else
+		mode = ADS1119_CM_SINGLE;
+
+	return ads1119_update_config_reg(st,
+					 ADS1119_CONFIG_CM_FIELD,
+					 FIELD_PREP(ADS1119_CONFIG_CM_FIELD, mode));
+}
+
+static int ads1119_get_hw_gain(int gain)
+{
+	if (gain == 4)
+		return ADS1119_GAIN_4;
+	else
+		return ADS1119_GAIN_1;
+}
+
+static int ads1119_get_hw_datarate(int datarate)
+{
+	switch (datarate) {
+	case 90:
+		datarate = ADS1119_DR_90_SPS;
+		break;
+	case 330:
+		datarate = ADS1119_DR_330_SPS;
+		break;
+	case 1000:
+		datarate = ADS1119_DR_1000_SPS;
+		break;
+	case 20:
+	default:
+		datarate = ADS1119_DR_20_SPS;
+		break;
+	}
+
+	return datarate;
+}
+
+static int ads1119_configure_channel(struct ads1119_state *st, int mux,
+				     int gain, int datarate)
+{
+	int ret;
+
+	ret = ads1119_update_config_reg(st,
+					ADS1119_CONFIG_MUX_FIELD,
+					FIELD_PREP(ADS1119_CONFIG_MUX_FIELD, mux));
+	if (ret)
+		return ret;
+
+	ret = ads1119_update_config_reg(st,
+					ADS1119_CONFIG_GAIN_FIELD,
+					FIELD_PREP(ADS1119_CONFIG_GAIN_FIELD,
+						   ads1119_get_hw_gain(gain)));
+	if (ret)
+		return ret;
+
+	return ads1119_update_config_reg(st,
+					 ADS1119_CONFIG_DR_FIELD,
+					 FIELD_PREP(ADS1119_CONFIG_DR_FIELD,
+						    ads1119_get_hw_datarate(datarate)));
+}
+
+static int ads1119_poll_data_ready(struct ads1119_state *st,
+				   struct iio_chan_spec const *chan)
+{
+	unsigned int datarate = st->channels_cfg[chan->address].datarate;
+	unsigned long wait_time;
+	bool data_ready;
+
+	/* Sleep between poll half of the data rate */
+	wait_time = DIV_ROUND_CLOSEST(1000000, 2 * datarate);
+
+	return read_poll_timeout(ads1119_data_ready,
+				 data_ready,
+				 (data_ready == true),
+				 wait_time,
+				 ADS1119_MAX_DRDY_TIMEOUT,
+				 false,
+				 st);
+}
+
+static int ads1119_read_data(struct ads1119_state *st,
+			     struct iio_chan_spec const *chan,
+			     unsigned int *val)
+{
+	unsigned int timeout;
+	int ret = 0;
+
+	timeout = msecs_to_jiffies(ADS1119_MAX_DRDY_TIMEOUT);
+
+	if (!st->client->irq)
+		ret = ads1119_poll_data_ready(st, chan);
+	else if (!wait_for_completion_timeout(&st->completion, timeout))
+		ret = -ETIMEDOUT;
+
+	if (ret)
+		return ret;
+
+	return ads1119_cmd_rdata(st, val);
+}
+
+static int ads1119_single_conversion(struct ads1119_state *st,
+				     struct iio_chan_spec const *chan,
+				     int *val,
+				     bool calib_offset)
+{
+	struct device *dev = &st->client->dev;
+	int mux, gain, datarate;
+	unsigned int sample;
+	int ret;
+
+	mux = st->channels_cfg[chan->address].mux;
+	gain = st->channels_cfg[chan->address].gain;
+	datarate = st->channels_cfg[chan->address].datarate;
+
+	if (calib_offset)
+		mux = ADS1119_MUX_SHORTED;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		goto end;
+
+	ret = ads1119_configure_channel(st, mux, gain, datarate);
+	if (ret)
+		goto pdown;
+
+	ret = ads1119_cmd(st, ADS1119_CMD_START_SYNC);
+	if (ret)
+		goto pdown;
+
+	ret = ads1119_read_data(st, chan, &sample);
+	if (ret)
+		goto pdown;
+
+	*val = sign_extend32(sample, chan->scan_type.realbits - 1);
+	ret = IIO_VAL_INT;
+pdown:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+end:
+	return ret;
+}
+
+static int ads1119_validate_datarate(struct ads1119_state *st, int datarate)
+{
+	switch (datarate) {
+	case 20:
+	case 90:
+	case 330:
+	case 1000:
+		return datarate;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ads1119_validate_gain(struct ads1119_state *st, int scale, int uscale)
+{
+	int gain = 1000000 / ((scale * 1000000) + uscale);
+
+	switch (gain) {
+	case 1:
+	case 4:
+		return gain;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ads1119_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_FRACTIONAL;
+		*vals = ads1119_available_gains;
+		*length = ARRAY_SIZE(ads1119_available_gains);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT;
+		*vals = ads1119_available_datarates;
+		*length = ARRAY_SIZE(ads1119_available_datarates);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ads1119_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct ads1119_state *st = iio_priv(indio_dev);
+	unsigned int index = chan->address;
+
+	if (index >= st->num_channels_cfg)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+			return ads1119_single_conversion(st, chan, val, false);
+		unreachable();
+	case IIO_CHAN_INFO_OFFSET:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+			return ads1119_single_conversion(st, chan, val, true);
+		unreachable();
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_uV / 1000;
+		*val /= st->channels_cfg[index].gain;
+		*val2 = chan->scan_type.realbits - 1;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->channels_cfg[index].datarate;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ads1119_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct ads1119_state *st = iio_priv(indio_dev);
+	unsigned int index = chan->address;
+	int ret;
+
+	if (index >= st->num_channels_cfg)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		ret = ads1119_validate_gain(st, val, val2);
+		if (ret < 0)
+			return ret;
+
+		st->channels_cfg[index].gain = ret;
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = ads1119_validate_datarate(st, val);
+		if (ret < 0)
+			return ret;
+
+		st->channels_cfg[index].datarate = ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ads1119_debugfs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				      unsigned int writeval, unsigned int *readval)
+{
+	struct ads1119_state *st = iio_priv(indio_dev);
+
+	if (reg > ADS1119_REG_STATUS)
+		return -EINVAL;
+
+	if (readval)
+		return ads1119_cmd_rreg(st, reg, readval);
+
+	if (reg > ADS1119_REG_CONFIG)
+		return -EINVAL;
+
+	return ads1119_cmd_wreg(st, writeval);
+}
+
+static const struct iio_info ads1119_info = {
+	.read_avail = ads1119_read_avail,
+	.read_raw = ads1119_read_raw,
+	.write_raw = ads1119_write_raw,
+	.debugfs_reg_access = ads1119_debugfs_reg_access,
+};
+
+static int ads1119_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct ads1119_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->client->dev;
+	unsigned int index;
+	int ret;
+
+	index = find_first_bit(indio_dev->active_scan_mask,
+			       indio_dev->masklength);
+
+	if (index >= st->num_channels_cfg)
+		return -EINVAL;
+
+	st->num_bytes_sample =
+		indio_dev->channels[index].scan_type.storagebits / 8;
+
+	ret = ads1119_set_conv_mode(st, state);
+	if (ret)
+		return ret;
+
+	if (!state) {
+		pm_runtime_mark_last_busy(dev);
+		return pm_runtime_put_autosuspend(dev);
+	}
+
+	ret = ads1119_configure_channel(st,
+					st->channels_cfg[index].mux,
+					st->channels_cfg[index].gain,
+					st->channels_cfg[index].datarate);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	return ads1119_cmd(st, ADS1119_CMD_START_SYNC);
+}
+
+static const struct iio_trigger_ops ads1119_trigger_ops = {
+	.set_trigger_state = &ads1119_set_trigger_state,
+	.validate_device = &iio_trigger_validate_own_device,
+};
+
+static irqreturn_t ads1119_irq_handler(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct ads1119_state *st = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll(indio_dev->trig);
+	else
+		complete(&st->completion);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t ads1119_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ads1119_state *st = iio_priv(indio_dev);
+	unsigned int sample;
+	int ret;
+
+	ret = ads1119_cmd_rdata(st, &sample);
+	if (ret) {
+		dev_err(&st->client->dev,
+			"Failed to read data on trigger (%d)\n", ret);
+		goto done;
+	}
+
+	memcpy(st->scan_buf, &sample, st->num_bytes_sample);
+	iio_push_to_buffers_with_timestamp(indio_dev, st->scan_buf,
+					   iio_get_time_ns(indio_dev));
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int ads1119_init(struct ads1119_state *st, bool vref_external)
+{
+	int ret;
+
+	ret = ads1119_reset(st);
+	if (ret)
+		return ret;
+
+	if (vref_external)
+		return ads1119_update_config_reg(st,
+						 ADS1119_CONFIG_VREF_FIELD,
+						 FIELD_PREP(ADS1119_CONFIG_VREF_FIELD,
+							    ADS1119_VREF_EXTERNAL));
+	return 0;
+}
+
+
+static int ads1119_map_analog_inputs_mux(int ain_pos, int ain_neg, bool differential)
+{
+	if (ain_pos >= ADS1119_MAX_SINGLE_CHANNELS)
+		return -EINVAL;
+
+	if (!differential)
+		return ADS1119_MUX_AIN0 + ain_pos;
+
+	if (ain_pos == 0 && ain_neg == 1)
+		return ADS1119_MUX_AIN0_AIN1;
+	else if (ain_pos == 1 && ain_neg == 2)
+		return ADS1119_MUX_AIN1_AIN2;
+	else if (ain_pos == 2 && ain_neg == 3)
+		return ADS1119_MUX_AIN2_AIN3;
+
+	return -EINVAL;
+}
+
+static int ads1119_alloc_and_config_channels(struct iio_dev *indio_dev)
+{
+	const struct iio_chan_spec ads1119_channel = ADS1119_V_CHAN(0, 0, 0);
+	const struct iio_chan_spec ads1119_timestamp = IIO_CHAN_SOFT_TIMESTAMP(0);
+	struct ads1119_state *st = iio_priv(indio_dev);
+	struct iio_chan_spec *iio_channels, *chan;
+	struct device *dev = &st->client->dev;
+	unsigned int num_channels, i;
+	bool differential;
+	u32 ain[2];
+	int ret;
+
+	st->num_channels_cfg = device_get_child_node_count(dev);
+	if (st->num_channels_cfg > ADS1119_MAX_CHANNELS)
+		return dev_err_probe(dev, -EINVAL,
+				     "Too many channels %d, max is %d\n",
+				     st->num_channels_cfg, ADS1119_MAX_CHANNELS);
+
+	st->channels_cfg = devm_kcalloc(dev, st->num_channels_cfg,
+					sizeof(*st->channels_cfg), GFP_KERNEL);
+	if (!st->channels_cfg)
+		return -ENOMEM;
+
+	/* Allocate one more iio channel for the timestamp */
+	num_channels = st->num_channels_cfg + 1;
+	iio_channels = devm_kcalloc(dev, num_channels,
+				    sizeof(*iio_channels),
+				    GFP_KERNEL);
+	if (!iio_channels)
+		return -ENOMEM;
+
+	i = 0;
+
+	device_for_each_child_node_scoped(dev, child) {
+		chan = &iio_channels[i];
+
+		differential = fwnode_property_present(child, "diff-channels");
+		if (differential)
+			ret = fwnode_property_read_u32_array(child, "diff-channels", ain, 2);
+		else
+			ret = fwnode_property_read_u32(child, "single-channel", &ain[0]);
+
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to get channel property\n");
+
+		ret = ads1119_map_analog_inputs_mux(ain[0], ain[1], differential);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "Invalid channel value\n");
+
+		st->channels_cfg[i].mux = ret;
+		st->channels_cfg[i].gain = ADS1119_DEFAULT_GAIN;
+		st->channels_cfg[i].datarate = ADS1119_DEFAULT_DATARATE;
+		st->scan_masks[i] = BIT(i);
+
+		*chan = ads1119_channel;
+		chan->channel = ain[0];
+		chan->address = i;
+		chan->scan_index = i;
+
+		if (differential) {
+			chan->channel2 = ain[1];
+			chan->differential = 1;
+		}
+
+		dev_dbg(dev, "channel: index %d, mux %d\n", i, st->channels_cfg[i].mux);
+
+		i++;
+	}
+
+	iio_channels[i] = ads1119_timestamp;
+	iio_channels[i].address = i;
+	iio_channels[i].scan_index = i;
+
+	indio_dev->channels = iio_channels;
+	indio_dev->num_channels = num_channels;
+	indio_dev->available_scan_masks = st->scan_masks;
+
+	return 0;
+}
+
+static int ads1119_probe(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev;
+	struct ads1119_state *st;
+	struct device *dev = &client->dev;
+	bool vref_external = true;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM,
+				     "Failed to allocate IIO device\n");
+
+	st = iio_priv(indio_dev);
+	st->client = client;
+
+	indio_dev->name = "ads1119";
+	indio_dev->info = &ads1119_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	i2c_set_clientdata(client, indio_dev);
+
+	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "vref");
+	if (st->vref_uV == -ENODEV) {
+		vref_external = false;
+		st->vref_uV = ADS1119_VREF_INTERNAL_VAL;
+	} else if (st->vref_uV < 0) {
+		return dev_err_probe(dev, st->vref_uV, "Failed to get vref\n");
+	}
+
+	st->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(st->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->reset_gpio),
+				     "Failed to get reset gpio\n");
+
+	ret = ads1119_alloc_and_config_channels(indio_dev);
+	if (ret)
+		return ret;
+
+	init_completion(&st->completion);
+
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(dev,
+						client->irq,
+						ads1119_irq_handler,
+						NULL,
+						IRQF_TRIGGER_FALLING,
+						"ads1119",
+						indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to allocate irq\n");
+
+		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+						  indio_dev->name,
+						  iio_device_id(indio_dev));
+		if (!st->trig)
+			return dev_err_probe(dev, -ENOMEM,
+					     "Failed to allocate IIO trigger\n");
+
+		st->trig->ops = &ads1119_trigger_ops;
+		iio_trigger_set_drvdata(st->trig, indio_dev);
+
+		ret = devm_iio_trigger_register(dev, st->trig);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to register IIO trigger\n");
+
+		indio_dev->trig = iio_trigger_get(st->trig);
+
+		ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+						      ads1119_trigger_handler,
+						      NULL);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to setup IIO buffer\n");
+	}
+
+	ret = ads1119_init(st, vref_external);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to initialize device\n");
+
+	pm_runtime_set_autosuspend_delay(dev, ADS1119_SUSPEND_DELAY);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_set_active(dev);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable pm runtime\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int ads1119_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct ads1119_state *st = iio_priv(indio_dev);
+
+	return ads1119_cmd(st, ADS1119_CMD_POWERDOWN);
+}
+
+/*
+ * The ADS1119 does not require a resume function because it automatically powers
+ * on after a reset.
+ * After a power down command, the ADS1119 can still communicate but turns off its
+ * analog parts. To resume from power down, the device will power up again
+ * upon receiving a start/sync command.
+ */
+static DEFINE_RUNTIME_DEV_PM_OPS(ads1119_pm_ops,
+				 ads1119_runtime_suspend,
+				 NULL,
+				 NULL);
+
+
+static const struct of_device_id __maybe_unused ads1119_of_match[] = {
+	{ .compatible = "ti,ads1119" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ads1119_of_match);
+
+static const struct i2c_device_id ads1119_id[] = {
+	{ "ads1119", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ads1119_id);
+
+static struct i2c_driver ads1119_driver = {
+	.driver = {
+		.name = "ads1119",
+		.of_match_table = ads1119_of_match,
+		.pm = pm_ptr(&ads1119_pm_ops),
+	},
+	.probe = ads1119_probe,
+	.id_table = ads1119_id,
+};
+module_i2c_driver(ads1119_driver);
+
+MODULE_AUTHOR("João Paulo Gonçalves <joao.goncalves@toradex.com>");
+MODULE_DESCRIPTION("Texas Instruments ADS1119 ADC Driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

[Jonathan Cameron]

On Thu,  6 Jun 2024 18:35:29 +0200
Francesco Dolcini <francesco@dolcini.it> wrote:

> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
> that features two differential or four single-ended inputs through a
> flexible input multiplexer (MUX), rail-to-rail input
> buffers, a programmable gain stage, a voltage reference, and an
> oscillator.
> 
> Apart from normal single conversion, the driver also supports
> continuous conversion mode using a triggered buffer. However, in this
> mode only one channel can be scanned at a time, and it only uses the data
> ready interrupt as a trigger. This is because the device channels are
> multiplexed, and using its own data ready interrupt as a trigger guarantees
> the signal sampling frequency.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>

A few more comments inline. Some of these I missed in the first
versions - sorry about that.  Takes a few passes to pick up
on everything unfortunately.

> diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
> new file mode 100644
> index 000000000000..ea0573f07279
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1119.c
> @@ -0,0 +1,850 @@

> +
> +struct ads1119_state {
> +	unsigned long scan_masks[ADS1119_MAX_CHANNELS + 1];

Could you just use the onehot support in the IIO core?
iio_validate_scan_mask_onehot() 

That combined with the channels not being available should
provide the required restrictions I think.

> +	u8 scan_buf[ADS1119_SCAN_BUFFER_SIZE];
> +	struct completion completion;
> +	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct iio_trigger *trig;
> +	struct ads1119_channel_config *channels_cfg;
> +	unsigned int num_bytes_sample;
> +	unsigned int num_channels_cfg;
> +	int vref_uV;
> +};

> +
> +static int ads1119_cmd(struct ads1119_state *st, unsigned int cmd)
> +{
> +	dev_dbg(&st->client->dev, "cmd: %#x\n", cmd);
> +
> +	return i2c_smbus_write_byte(st->client, cmd);
I'm not a fan of tiny wrappers to add debug info.
The i2c core has trace points that let you get to the relevant data. Better
to use those for debug and flatten this code so we
see the actual bus accesses inline.

> +}
> +
> +static int ads1119_cmd_wreg(struct ads1119_state *st, unsigned int val)
> +{
> +	dev_dbg(&st->client->dev, "wreg: %#x\n", val);
> +
> +	return i2c_smbus_write_byte_data(st->client, ADS1119_CMD_WREG, val);
> +}
> +
> +
> +static int ads1119_get_hw_datarate(int datarate)
> +{
> +	switch (datarate) {
> +	case 90:
> +		datarate = ADS1119_DR_90_SPS;
		return ADS1...
etc

> +		break;
> +	case 330:
> +		datarate = ADS1119_DR_330_SPS;
> +		break;
> +	case 1000:
> +		datarate = ADS1119_DR_1000_SPS;
> +		break;
> +	case 20:
> +	default:
> +		datarate = ADS1119_DR_20_SPS;
> +		break;
> +	}
> +
> +	return datarate;
and this can go as unused.
> +}
> +

> +
> +static int ads1119_poll_data_ready(struct ads1119_state *st,
> +				   struct iio_chan_spec const *chan)
> +{
> +	unsigned int datarate = st->channels_cfg[chan->address].datarate;
> +	unsigned long wait_time;
> +	bool data_ready;
> +
> +	/* Sleep between poll half of the data rate */
> +	wait_time = DIV_ROUND_CLOSEST(1000000, 2 * datarate);
> +
> +	return read_poll_timeout(ads1119_data_ready,
> +				 data_ready,
> +				 (data_ready == true),
> +				 wait_time,
> +				 ADS1119_MAX_DRDY_TIMEOUT,
> +				 false,
> +				 st);
Over wrapped. Aim for closer to 80 chars.

> +}
> +
> +static int ads1119_read_data(struct ads1119_state *st,
> +			     struct iio_chan_spec const *chan,
> +			     unsigned int *val)
> +{
> +	unsigned int timeout;
> +	int ret = 0;
> +
> +	timeout = msecs_to_jiffies(ADS1119_MAX_DRDY_TIMEOUT);
> +
> +	if (!st->client->irq)
> +		ret = ads1119_poll_data_ready(st, chan);
		if (ret)
			return ret;
	} else if () {
> +	else if (!wait_for_completion_timeout(&st->completion, timeout))
> +		ret = -ETIMEDOUT;
		return -ETIMEDOUT;
}

Is probably clearer to the reader than poking a value into ret in the
timeout path just to share this error check.

> +
> +	if (ret)
> +		return ret;
> +
> +	return ads1119_cmd_rdata(st, val);
> +}
> +
> +static int ads1119_single_conversion(struct ads1119_state *st,
> +				     struct iio_chan_spec const *chan,
> +				     int *val,
> +				     bool calib_offset)
> +{
> +	struct device *dev = &st->client->dev;
> +	int mux, gain, datarate;
> +	unsigned int sample;
> +	int ret;
> +
> +	mux = st->channels_cfg[chan->address].mux;
> +	gain = st->channels_cfg[chan->address].gain;
> +	datarate = st->channels_cfg[chan->address].datarate;
> +
> +	if (calib_offset)
> +		mux = ADS1119_MUX_SHORTED;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto end;
> +
> +	ret = ads1119_configure_channel(st, mux, gain, datarate);
> +	if (ret)
> +		goto pdown;
> +
> +	ret = ads1119_cmd(st, ADS1119_CMD_START_SYNC);
> +	if (ret)
> +		goto pdown;
> +
> +	ret = ads1119_read_data(st, chan, &sample);
> +	if (ret)
> +		goto pdown;
> +
> +	*val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +	ret = IIO_VAL_INT;
> +pdown:
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +end:

Just return above rather than having this label.

> +	return ret;
> +}
> +

> +
> +static int ads1119_validate_gain(struct ads1119_state *st, int scale, int uscale)
> +{
> +	int gain = 1000000 / ((scale * 1000000) + uscale);
> +
> +	switch (gain) {
> +	case 1:
> +	case 4:
> +		return gain;
Odd to calculate it if we don't need it
		return MICRO / (scale * MICRO + uscale); 
use constants as it's easy to drop a 0 in these without anyone noticing.

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

> +static int ads1119_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	unsigned int index = chan->address;
> +
> +	if (index >= st->num_channels_cfg)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ads1119_single_conversion(st, chan, val, false);
> +		unreachable();
> +	case IIO_CHAN_INFO_OFFSET:
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> +			return ads1119_single_conversion(st, chan, val, true);
> +		unreachable();
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_uV / 1000;
> +		*val /= st->channels_cfg[index].gain;
> +		*val2 = chan->scan_type.realbits - 1;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->channels_cfg[index].datarate;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
Can't get here so drop this. 

> +	return 0;
> +}
> +
> +static int ads1119_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	unsigned int index = chan->address;
> +	int ret;
> +
> +	if (index >= st->num_channels_cfg)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = ads1119_validate_gain(st, val, val2);
> +		if (ret < 0)
> +			return ret;
> +
> +		st->channels_cfg[index].gain = ret;
> +		break;

		return 0; etc
No point in a reader having to go look for the what happens after
the switch statement if nothing does.


> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = ads1119_validate_datarate(st, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		st->channels_cfg[index].datarate = ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

> +static int ads1119_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->client->dev;
> +	unsigned int index;
> +	int ret;
> +
> +	index = find_first_bit(indio_dev->active_scan_mask,
> +			       indio_dev->masklength);
> +
> +	if (index >= st->num_channels_cfg)
> +		return -EINVAL;
> +
> +	st->num_bytes_sample =
> +		indio_dev->channels[index].scan_type.storagebits / 8;
> +
> +	ret = ads1119_set_conv_mode(st, state);
> +	if (ret)
> +		return ret;
> +
> +	if (!state) {
> +		pm_runtime_mark_last_busy(dev);
> +		return pm_runtime_put_autosuspend(dev);
> +	}

When a trigger is tightly coupled to a device, the boundaries between
what makes sense in trigger_ops and what makes sense in the buffer ones
gets blurry.

To make it easier to relax need for the trigger (typical for some boards
to not wire the interrupt), in which case we set the device going and read
at a known lower rate using a hrtimer trigger or similar, I'd suggest
moving at least some of this.

I'd expect the interrupt source enabling if it's independent of enabling
capture in here and stuff more related to channel configuration either
in update_scan_modes, or in preenable for the buffer.

So almost everything in here belongs in the preenable for the buffer and
postdisable I think...



> +
> +	ret = ads1119_configure_channel(st,
> +					st->channels_cfg[index].mux,
> +					st->channels_cfg[index].gain,
> +					st->channels_cfg[index].datarate);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	return ads1119_cmd(st, ADS1119_CMD_START_SYNC);
> +}


> +static int ads1119_init(struct ads1119_state *st, bool vref_external)
> +{
> +	int ret;
> +
> +	ret = ads1119_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	if (vref_external)
> +		return ads1119_update_config_reg(st,
> +						 ADS1119_CONFIG_VREF_FIELD,
> +						 FIELD_PREP(ADS1119_CONFIG_VREF_FIELD,
> +							    ADS1119_VREF_EXTERNAL));
> +	return 0;
> +}
> +

Single blank line

> +
> +static int ads1119_map_analog_inputs_mux(int ain_pos, int ain_neg, bool differential)
...

> +
> +static int ads1119_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ads1119_state *st;
> +	struct device *dev = &client->dev;
> +	bool vref_external = true;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate IIO device\n");
> +
> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +
> +	indio_dev->name = "ads1119";
> +	indio_dev->info = &ads1119_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (st->vref_uV == -ENODEV) {
> +		vref_external = false;
> +		st->vref_uV = ADS1119_VREF_INTERNAL_VAL;
> +	} else if (st->vref_uV < 0) {
> +		return dev_err_probe(dev, st->vref_uV, "Failed to get vref\n");
> +	}
> +
> +	st->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->reset_gpio),
> +				     "Failed to get reset gpio\n");
> +
> +	ret = ads1119_alloc_and_config_channels(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	init_completion(&st->completion);
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(dev,
> +						client->irq,
> +						ads1119_irq_handler,
> +						NULL,
> +						IRQF_TRIGGER_FALLING,
> +						"ads1119",
> +						indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to allocate irq\n");
> +
> +		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						  indio_dev->name,
> +						  iio_device_id(indio_dev));
> +		if (!st->trig)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "Failed to allocate IIO trigger\n");
> +
> +		st->trig->ops = &ads1119_trigger_ops;
> +		iio_trigger_set_drvdata(st->trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(dev, st->trig);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to register IIO trigger\n");
> +
> +		indio_dev->trig = iio_trigger_get(st->trig);
> +
> +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +						      ads1119_trigger_handler,
> +						      NULL);

Is the device unable to use other triggers? You have the trigger validating
it is being used with the device, but nothing stopping the device being
disconnected from it's own trigger and connected to another one.
The validate_trigger callback IIRC.
We also have iio_trigger_set_immutable() to stop the unbind.

If it's not too hard to make it work with other triggers that can be
a very useful feature.


> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to setup IIO buffer\n");
> +	}

...

> +
> +static int ads1119_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct ads1119_state *st = iio_priv(indio_dev);
> +
> +	return ads1119_cmd(st, ADS1119_CMD_POWERDOWN);

I'd like an additional call of this power down command in the driver removal path
via a devm_add_action_or_reset() as we always want to leave a device
powered down if we rip out the userspace intefaces!

In many cases it might be powered down anyway due to runtimepm, but
it might not by coincidence of timing, or because runtime_pm isn't built
or enabled for some reason.


> +}
> +
> +/*
> + * The ADS1119 does not require a resume function because it automatically powers
> + * on after a reset.
> + * After a power down command, the ADS1119 can still communicate but turns off its
> + * analog parts. To resume from power down, the device will power up again
> + * upon receiving a start/sync command.
> + */
> +static DEFINE_RUNTIME_DEV_PM_OPS(ads1119_pm_ops,
> +				 ads1119_runtime_suspend,
> +				 NULL,
> +				 NULL);
> +
Single blank line here, not 2.
> +
> +static const struct of_device_id __maybe_unused ads1119_of_match[] = {

Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

[Francesco Dolcini]

Hello Jonathan,
thanks for the review.

On Sun, Jun 09, 2024 at 11:52:34AM +0100, Jonathan Cameron wrote:
> On Thu,  6 Jun 2024 18:35:29 +0200
> Francesco Dolcini <francesco@dolcini.it> wrote:
> 
> > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > 
> > The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
> > that features two differential or four single-ended inputs through a
> > flexible input multiplexer (MUX), rail-to-rail input
> > buffers, a programmable gain stage, a voltage reference, and an
> > oscillator.
> > 
> > Apart from normal single conversion, the driver also supports
> > continuous conversion mode using a triggered buffer. However, in this
> > mode only one channel can be scanned at a time, and it only uses the data
> > ready interrupt as a trigger. This is because the device channels are
> > multiplexed, and using its own data ready interrupt as a trigger guarantees
> > the signal sampling frequency.
> > 
> > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> A few more comments inline. Some of these I missed in the first
> versions - sorry about that.  Takes a few passes to pick up
> on everything unfortunately.
> 
> > diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
> > new file mode 100644
> > index 000000000000..ea0573f07279
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads1119.c
> > @@ -0,0 +1,850 @@

...

> > +
> > +static int ads1119_cmd(struct ads1119_state *st, unsigned int cmd)
> > +{
> > +	dev_dbg(&st->client->dev, "cmd: %#x\n", cmd);
> > +
> > +	return i2c_smbus_write_byte(st->client, cmd);
> I'm not a fan of tiny wrappers to add debug info.
> The i2c core has trace points that let you get to the relevant data. Better
> to use those for debug and flatten this code so we
> see the actual bus accesses inline.

Fine on removing the debug prints, we'll do.

However ads1119_cmd_rdata() and ads1119_update_config_reg() wrappers are
making the error handling in the caller easier.

Either we remove only ads1119_cmd/ads1119_cmd_wreg or we keep them all
for orthogonality. Both works for me, just let me know if you agree and
which option do you prefer. Personally I would keep them all without
debug prints.

> > +
> > +	return read_poll_timeout(ads1119_data_ready,
> > +				 data_ready,
> > +				 (data_ready == true),
> > +				 wait_time,
> > +				 ADS1119_MAX_DRDY_TIMEOUT,
> > +				 false,
> > +				 st);
> Over wrapped. Aim for closer to 80 chars.

I'll do.
Do you have any rules on the IIO subsystem driver? IOW is it fine to
have line slightly longer than 80chars if this improves readability ?

Francesco

Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

[Jonathan Cameron]

On Mon, 10 Jun 2024 13:59:09 +0200
Francesco Dolcini <francesco@dolcini.it> wrote:

> Hello Jonathan,
> thanks for the review.
> 
> On Sun, Jun 09, 2024 at 11:52:34AM +0100, Jonathan Cameron wrote:
> > On Thu,  6 Jun 2024 18:35:29 +0200
> > Francesco Dolcini <francesco@dolcini.it> wrote:
> >   
> > > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > 
> > > The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
> > > that features two differential or four single-ended inputs through a
> > > flexible input multiplexer (MUX), rail-to-rail input
> > > buffers, a programmable gain stage, a voltage reference, and an
> > > oscillator.
> > > 
> > > Apart from normal single conversion, the driver also supports
> > > continuous conversion mode using a triggered buffer. However, in this
> > > mode only one channel can be scanned at a time, and it only uses the data
> > > ready interrupt as a trigger. This is because the device channels are
> > > multiplexed, and using its own data ready interrupt as a trigger guarantees
> > > the signal sampling frequency.
> > > 
> > > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>  
> > 
> > A few more comments inline. Some of these I missed in the first
> > versions - sorry about that.  Takes a few passes to pick up
> > on everything unfortunately.
> >   
> > > diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
> > > new file mode 100644
> > > index 000000000000..ea0573f07279
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ti-ads1119.c
> > > @@ -0,0 +1,850 @@  
> 
> ...
> 
> > > +
> > > +static int ads1119_cmd(struct ads1119_state *st, unsigned int cmd)
> > > +{
> > > +	dev_dbg(&st->client->dev, "cmd: %#x\n", cmd);
> > > +
> > > +	return i2c_smbus_write_byte(st->client, cmd);  
> > I'm not a fan of tiny wrappers to add debug info.
> > The i2c core has trace points that let you get to the relevant data. Better
> > to use those for debug and flatten this code so we
> > see the actual bus accesses inline.  
> 
> Fine on removing the debug prints, we'll do.
> 
> However ads1119_cmd_rdata() and ads1119_update_config_reg() wrappers are
> making the error handling in the caller easier.

for rdata() only 2 calls so not bad to add the error handling in those
locations.

The update one is more justifiable given the read modify write and that
it is reasonably common.  So perhaps just keep that one, but it is
odd that your cmd_wreg() hard codes the address and cmd_rreg() doesn't.
If keeping the update function I would pass in the register address.

Maybe better would be to just cache the contents of the cmd register.
Then you can avoid a bunch or RMW and make them simply a write.

Wrapping that up makes sense as both a register write and on success
update of a cached value.

Pity this isn't quite all register based as if it was I'd say just
use regmap which gives you all the accessors for free.


> 
> Either we remove only ads1119_cmd/ads1119_cmd_wreg or we keep them all
> for orthogonality. Both works for me, just let me know if you agree and
> which option do you prefer. Personally I would keep them all without
> debug prints.

I'd drop them, but have a update_config_reg() that uses a cached
value to avoid the reads and updates that cached value on the write
succeeding.  That one is non trivial.

> 
> > > +
> > > +	return read_poll_timeout(ads1119_data_ready,
> > > +				 data_ready,
> > > +				 (data_ready == true),
> > > +				 wait_time,
> > > +				 ADS1119_MAX_DRDY_TIMEOUT,
> > > +				 false,
> > > +				 st);  
> > Over wrapped. Aim for closer to 80 chars.  
> 
> I'll do.
> Do you have any rules on the IIO subsystem driver? IOW is it fine to
> have line slightly longer than 80chars if this improves readability ?

Absolutely.  Good to keep to 80 chars except:
1) String involved - don't break it up as can't grep - those can go way
   over 80 chars
2) Non trivial readability improvement.  Then stay under 100 chars.
   If you need to go over that, normally can use a local variable
   or similar to avoid it.


> 
> Francesco
> 
> 

Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

[João Paulo Gonçalves]

Hi Jonathan,

Thanks for the review!

> > +
> > +static int ads1119_validate_gain(struct ads1119_state *st, int scale, int uscale)
> > +{
> > +	int gain = 1000000 / ((scale * 1000000) + uscale);
> > +
> > +	switch (gain) {
> > +	case 1:
> > +	case 4:
> > +		return gain;
> Odd to calculate it if we don't need it
> 		return MICRO / (scale * MICRO + uscale);
> use constants as it's easy to drop a 0 in these without anyone noticing.
>
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +

Just a minor. I do use the calculated value on write_raw() by storing it as the
new channel gain and would still need to validate it as scale/uscale comes from
userspace. Maybe I can just remove the validate_gain function and do the check
directly on write_raw(). What do you think?

Regards,
João Paulo Gonçalves

Re: [PATCH v2 2/2] iio: adc: ti-ads1119: Add driver

[Jonathan Cameron]

On Mon, 10 Jun 2024 10:26:29 -0300
João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com> wrote:

> Hi Jonathan,
> 
> Thanks for the review!
> 
> > > +
> > > +static int ads1119_validate_gain(struct ads1119_state *st, int scale, int uscale)
> > > +{
> > > +	int gain = 1000000 / ((scale * 1000000) + uscale);
> > > +
> > > +	switch (gain) {
> > > +	case 1:
> > > +	case 4:
> > > +		return gain;  
> > Odd to calculate it if we don't need it
> > 		return MICRO / (scale * MICRO + uscale);
> > use constants as it's easy to drop a 0 in these without anyone noticing.
> >  
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +  
> 
> Just a minor. I do use the calculated value on write_raw() by storing it as the
> new channel gain and would still need to validate it as scale/uscale comes from
> userspace. Maybe I can just remove the validate_gain function and do the check
> directly on write_raw(). What do you think?

If that's the only caller, sure, move it to write raw.

Jonathan

> 
> Regards,
> João Paulo Gonçalves
>