All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 1/2] dt-bindings: adc: add AD7173
@ 2023-12-12 10:44 ` Dumitru Ceclan
  0 siblings, 0 replies; 17+ messages in thread
From: Dumitru Ceclan @ 2023-12-12 10:44 UTC (permalink / raw)
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel, Dumitru Ceclan

The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel applications
or higher speed multiplexed applications. The Sigma-Delta ADC is intended
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V7->V8
 - include missing fix from V6

 .../bindings/iio/adc/adi,ad7173.yaml          | 170 ++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
new file mode 100644
index 000000000000..25a5404ee353
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -0,0 +1,170 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7173 ADC
+
+maintainers:
+  - Ceclan Dumitru <dumitru.ceclan@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7172-2
+      - adi,ad7173-8
+      - adi,ad7175-2
+      - adi,ad7176-2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  spi-max-frequency:
+    maximum: 20000000
+
+  refin-supply:
+    description: external reference supply, can be used as reference for conversion.
+
+  refin2-supply:
+    description: external reference supply, can be used as reference for conversion.
+
+  avdd-supply:
+    description: avdd supply, can be used as reference for conversion.
+
+patternProperties:
+  "^channel@[0-9a-f]$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 15
+
+      diff-channels:
+        items:
+          minimum: 0
+          maximum: 31
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on
+          the specific channel. Valid values are:
+          refin      : REFIN(+)/REFIN(−).
+          refin2     : REFIN2(+)/REFIN2(−)
+          refout-avss: REFOUT/AVSS (Internal reference)
+          avdd       : AVDD
+
+          External reference refin2 only available on ad7173-8.
+          If not specified, internal reference used.
+        enum:
+          - refin
+          - refin2
+          - refout-avss
+          - avdd
+        default: refout-avss
+
+    required:
+      - reg
+      - diff-channels
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              const: adi,ad7173-8
+    then:
+      properties:
+        refin2-supply: false
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            adi,reference-select:
+              enum:
+                - refin
+                - refout-avss
+                - avdd
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,ad7173-8";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpio>;
+        spi-max-frequency = <5000000>;
+
+        refin-supply = <&dummy_regulator>;
+
+        channel@0 {
+          reg = <0>;
+          bipolar;
+          diff-channels = <0 1>;
+          adi,reference-select = "refin";
+        };
+
+        channel@1 {
+          reg = <1>;
+          diff-channels = <2 3>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          bipolar;
+          diff-channels = <4 5>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          bipolar;
+          diff-channels = <6 7>;
+        };
+
+        channel@4 {
+          reg = <4>;
+          diff-channels = <8 9>;
+          adi,reference-select = "avdd";
+        };
+      };
+    };
-- 
2.42.0


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

* [PATCH v8 1/2] dt-bindings: adc: add AD7173
@ 2023-12-12 10:44 ` Dumitru Ceclan
  0 siblings, 0 replies; 17+ messages in thread
From: Dumitru Ceclan @ 2023-12-12 10:44 UTC (permalink / raw)
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel, Dumitru Ceclan

The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel applications
or higher speed multiplexed applications. The Sigma-Delta ADC is intended
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V7->V8
 - include missing fix from V6

 .../bindings/iio/adc/adi,ad7173.yaml          | 170 ++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
new file mode 100644
index 000000000000..25a5404ee353
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -0,0 +1,170 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7173 ADC
+
+maintainers:
+  - Ceclan Dumitru <dumitru.ceclan@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7172-2
+      - adi,ad7173-8
+      - adi,ad7175-2
+      - adi,ad7176-2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  spi-max-frequency:
+    maximum: 20000000
+
+  refin-supply:
+    description: external reference supply, can be used as reference for conversion.
+
+  refin2-supply:
+    description: external reference supply, can be used as reference for conversion.
+
+  avdd-supply:
+    description: avdd supply, can be used as reference for conversion.
+
+patternProperties:
+  "^channel@[0-9a-f]$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 15
+
+      diff-channels:
+        items:
+          minimum: 0
+          maximum: 31
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on
+          the specific channel. Valid values are:
+          refin      : REFIN(+)/REFIN(−).
+          refin2     : REFIN2(+)/REFIN2(−)
+          refout-avss: REFOUT/AVSS (Internal reference)
+          avdd       : AVDD
+
+          External reference refin2 only available on ad7173-8.
+          If not specified, internal reference used.
+        enum:
+          - refin
+          - refin2
+          - refout-avss
+          - avdd
+        default: refout-avss
+
+    required:
+      - reg
+      - diff-channels
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              const: adi,ad7173-8
+    then:
+      properties:
+        refin2-supply: false
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            adi,reference-select:
+              enum:
+                - refin
+                - refout-avss
+                - avdd
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,ad7173-8";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpio>;
+        spi-max-frequency = <5000000>;
+
+        refin-supply = <&dummy_regulator>;
+
+        channel@0 {
+          reg = <0>;
+          bipolar;
+          diff-channels = <0 1>;
+          adi,reference-select = "refin";
+        };
+
+        channel@1 {
+          reg = <1>;
+          diff-channels = <2 3>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          bipolar;
+          diff-channels = <4 5>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          bipolar;
+          diff-channels = <6 7>;
+        };
+
+        channel@4 {
+          reg = <4>;
+          diff-channels = <8 9>;
+          adi,reference-select = "avdd";
+        };
+      };
+    };
-- 
2.42.0


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

* [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-12 10:44 ` Dumitru Ceclan
@ 2023-12-12 10:44   ` Dumitru Ceclan
  -1 siblings, 0 replies; 17+ messages in thread
From: Dumitru Ceclan @ 2023-12-12 10:44 UTC (permalink / raw)
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel, Dumitru Ceclan

The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel
applications or higher speed multiplexed applications. The Sigma-Delta
ADC is intended primarily for measurement of signals close to DC but also
delivers outstanding performance with input bandwidths out to ~10kHz.

Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V7->V8
 - remove Kconfig commas
 - reorder include <...gpio/regmap.h>
 - GPO12_DATA(x)  from BIT(x) -> BIT(x+0)
 - use the same pointer to dev in ad7173_setup()
 - rewrite temp channel read_raw scale for better readability
 - add comment to explain temp channel offset
 - follow multi-line comment style
 - change ref_sel type to signed int
 - use fwnode_property_match_property_string()

 drivers/iio/adc/Kconfig  |  17 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7173.c | 964 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 982 insertions(+)
 create mode 100644 drivers/iio/adc/ad7173.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1e2b7a2c67c6..1d7b9ade4d59 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -54,6 +54,23 @@ config AD7124
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7124.
 
+config AD7173
+	tristate "Analog Devices AD7173 driver"
+	depends on SPI_MASTER
+	select AD_SIGMA_DELTA
+	select GPIO_REGMAP if GPIOLIB
+	select REGMAP_SPI if GPIOLIB
+	help
+	  Say yes here to build support for Analog Devices AD7173 and similar ADC
+	  Currently supported models:
+	   - AD7172-2
+	   - AD7173-8
+	   - AD7175-2
+	   - AD7176-2
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad7173.
+
 config AD7192
 	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index c0803383a7cc..12e723cdc1f1 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
 obj-$(CONFIG_AD7124) += ad7124.o
+obj-$(CONFIG_AD7173) += ad7173.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7266) += ad7266.o
 obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
new file mode 100644
index 000000000000..96918b24a10a
--- /dev/null
+++ b/drivers/iio/adc/ad7173.c
@@ -0,0 +1,964 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
+ * Copyright (C) 2015, 2023 Analog Devices, Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define AD7173_REG_COMMS		0x00
+#define AD7173_REG_ADC_MODE		0x01
+#define AD7173_REG_INTERFACE_MODE	0x02
+#define AD7173_REG_CRC			0x03
+#define AD7173_REG_DATA			0x04
+#define AD7173_REG_GPIO			0x06
+#define AD7173_REG_ID			0x07
+#define AD7173_REG_CH(x)		(0x10 + (x))
+#define AD7173_REG_SETUP(x)		(0x20 + (x))
+#define AD7173_REG_FILTER(x)		(0x28 + (x))
+#define AD7173_REG_OFFSET(x)		(0x30 + (x))
+#define AD7173_REG_GAIN(x)		(0x38 + (x))
+
+#define AD7173_RESET_LENGTH		BITS_TO_BYTES(64)
+
+#define AD7173_CH_ENABLE		BIT(15)
+#define AD7173_CH_SETUP_SEL_MASK	GENMASK(14, 12)
+#define AD7173_CH_SETUP_AINPOS_MASK	GENMASK(9, 5)
+#define AD7173_CH_SETUP_AINNEG_MASK	GENMASK(4, 0)
+
+#define AD7173_CH_ADDRESS(pos, neg) \
+	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
+	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
+#define AD7173_AIN_TEMP_POS	17
+#define AD7173_AIN_TEMP_NEG	18
+
+#define AD7172_ID			0x00d0
+#define AD7173_ID			0x30d0
+#define AD7175_ID			0x0cd0
+#define AD7176_ID			0x0c90
+#define AD7173_ID_MASK			GENMASK(15, 4)
+
+#define AD7173_ADC_MODE_REF_EN		BIT(15)
+#define AD7173_ADC_MODE_SING_CYC	BIT(13)
+#define AD7173_ADC_MODE_MODE_MASK	GENMASK(6, 4)
+#define AD7173_ADC_MODE_CLOCKSEL_MASK	GENMASK(3, 2)
+
+#define AD7173_GPIO_PDSW	BIT(14)
+#define AD7173_GPIO_OP_EN2_3	BIT(13)
+#define AD7173_GPIO_MUX_IO	BIT(12)
+#define AD7173_GPIO_SYNC_EN	BIT(11)
+#define AD7173_GPIO_ERR_EN	BIT(10)
+#define AD7173_GPIO_ERR_DAT	BIT(9)
+#define AD7173_GPIO_GP_DATA3	BIT(7)
+#define AD7173_GPIO_GP_DATA2	BIT(6)
+#define AD7173_GPIO_IP_EN1	BIT(5)
+#define AD7173_GPIO_IP_EN0	BIT(4)
+#define AD7173_GPIO_OP_EN1	BIT(3)
+#define AD7173_GPIO_OP_EN0	BIT(2)
+#define AD7173_GPIO_GP_DATA1	BIT(1)
+#define AD7173_GPIO_GP_DATA0	BIT(0)
+
+#define AD7173_GPO12_DATA(x)	BIT(x + 0)
+#define AD7173_GPO23_DATA(x)	BIT(x + 4)
+#define AD7173_GPO_DATA(x)	(x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
+
+#define AD7173_INTERFACE_DATA_STAT	BIT(6)
+#define AD7173_INTERFACE_DATA_STAT_EN(x) \
+	FIELD_PREP(AD7173_INTERFACE_DATA_STAT, x)
+
+#define AD7173_SETUP_BIPOLAR		BIT(12)
+#define AD7173_SETUP_AREF_BUF_MASK	GENMASK(11, 10)
+#define AD7173_SETUP_AIN_BUF_MASK	GENMASK(9, 8)
+
+#define AD7173_SETUP_REF_SEL_MASK	GENMASK(5, 4)
+#define AD7173_SETUP_REF_SEL_AVDD1_AVSS	0x3
+#define AD7173_SETUP_REF_SEL_INT_REF	0x2
+#define AD7173_SETUP_REF_SEL_EXT_REF2	0x1
+#define AD7173_SETUP_REF_SEL_EXT_REF	0x0
+#define AD7173_VOLTAGE_INT_REF_uV	2500000
+#define AD7173_TEMP_SENSIIVITY_uV_per_C	477
+
+#define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
+#define AD7173_MAX_CONFIGS		8
+
+enum ad7173_ids {
+	ID_AD7172_2,
+	ID_AD7173_8,
+	ID_AD7175_2,
+	ID_AD7176_2,
+};
+
+struct ad7173_device_info {
+	const unsigned int *sinc5_data_rates;
+	unsigned int num_sinc5_data_rates;
+	unsigned int num_channels;
+	unsigned int num_configs;
+	unsigned int num_inputs;
+	unsigned int clock;
+	unsigned int id;
+	char *name;
+	bool has_temp;
+	u8 num_gpios;
+};
+
+struct ad7173_channel_config {
+	u8 cfg_slot;
+	bool live;
+
+	/* Following fields are used to compare equality. */
+	struct_group(config_props,
+		bool bipolar;
+		bool input_buf;
+		u8 odr;
+		u8 ref_sel;
+	);
+};
+
+struct ad7173_channel {
+	unsigned int chan_reg;
+	unsigned int ain;
+	struct ad7173_channel_config cfg;
+};
+
+struct ad7173_state {
+	struct ad_sigma_delta sd;
+	const struct ad7173_device_info *info;
+	struct ad7173_channel *channels;
+	struct regulator_bulk_data regulators[3];
+	unsigned int adc_mode;
+	unsigned int interface_mode;
+	unsigned int num_channels;
+	struct ida cfg_slots_status;
+	unsigned long long config_usage_counter;
+	unsigned long long *config_cnts;
+#if IS_ENABLED(CONFIG_GPIOLIB)
+	struct regmap *reg_gpiocon_regmap;
+	struct gpio_regmap *gpio_regmap;
+#endif
+};
+
+static const unsigned int ad7173_sinc5_data_rates[] = {
+	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,
+	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,
+	49680,	 20010,	  16333,   10000,   5000,    2500,    1250,
+};
+
+static const unsigned int ad7175_sinc5_data_rates[] = {
+	50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000,
+	10000000, 5000000,  2500000,  1000000,	500000,	  397500,   200000,
+	100000,	  59920,    49960,    20000,	16666,	  10000,    5000,
+};
+
+static const struct ad7173_device_info ad7173_device_info[] = {
+	[ID_AD7172_2] = {
+		.name = "ad7172-2",
+		.id = AD7172_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.num_gpios = 2,
+		.has_temp = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD7173_8] = {
+		.name = "ad7173-8",
+		.id = AD7173_ID,
+		.num_inputs = 17,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_gpios = 4,
+		.has_temp = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD7175_2] = {
+		.name = "ad7175-2",
+		.id = AD7175_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.num_gpios = 2,
+		.has_temp = true,
+		.clock = 16 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7175_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+	},
+	[ID_AD7176_2] = {
+		.name = "ad7176-2",
+		.id = AD7176_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.num_gpios = 2,
+		.has_temp = false,
+		.clock = 16 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7175_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+	},
+};
+
+static const char *const ad7173_ref_sel_str[] = {
+	[AD7173_SETUP_REF_SEL_EXT_REF]    = "refin",
+	[AD7173_SETUP_REF_SEL_EXT_REF2]   = "refin2",
+	[AD7173_SETUP_REF_SEL_INT_REF]    = "refout-avss",
+	[AD7173_SETUP_REF_SEL_AVDD1_AVSS] = "avdd",
+};
+
+#if IS_ENABLED(CONFIG_GPIOLIB)
+
+static const struct regmap_range ad7173_range_gpio[] = {
+	regmap_reg_range(AD7173_REG_GPIO, AD7173_REG_GPIO),
+};
+
+static const struct regmap_access_table ad7173_access_table = {
+	.yes_ranges = ad7173_range_gpio,
+	.n_yes_ranges = ARRAY_SIZE(ad7173_range_gpio),
+};
+
+static const struct regmap_config ad7173_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.rd_table = &ad7173_access_table,
+	.wr_table = &ad7173_access_table,
+	.read_flag_mask = BIT(6),
+};
+
+static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
+			     unsigned int offset, unsigned int *reg,
+			     unsigned int *mask)
+{
+	*mask = AD7173_GPO_DATA(offset);
+	*reg = base;
+	return 0;
+}
+
+static void ad7173_gpio_disable(void *data)
+{
+	struct ad7173_state *st = data;
+	unsigned int mask;
+
+	mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | AD7173_GPIO_OP_EN2_3;
+	regmap_update_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, mask, ~mask);
+}
+
+static int ad7173_gpio_init(struct ad7173_state *st)
+{
+	struct gpio_regmap_config gpio_regmap = {};
+	struct device *dev = &st->sd.spi->dev;
+	unsigned int mask;
+	int ret;
+
+	st->reg_gpiocon_regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
+	ret = PTR_ERR_OR_ZERO(st->reg_gpiocon_regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to init regmap\n");
+
+	mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | AD7173_GPIO_OP_EN2_3;
+	regmap_update_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, mask, mask);
+
+	ret = devm_add_action_or_reset(dev, ad7173_gpio_disable, st);
+	if (ret)
+		return ret;
+
+	gpio_regmap.parent = dev;
+	gpio_regmap.regmap = st->reg_gpiocon_regmap;
+	gpio_regmap.ngpio = st->info->num_gpios;
+	gpio_regmap.reg_set_base = AD7173_REG_GPIO;
+	gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
+
+	st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
+	ret = PTR_ERR_OR_ZERO(st->gpio_regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to init gpio-regmap\n");
+
+	return 0;
+}
+
+#endif /* CONFIG_GPIOLIB */
+
+static struct ad7173_state *ad_sigma_delta_to_ad7173(struct ad_sigma_delta *sd)
+{
+	return container_of(sd, struct ad7173_state, sd);
+}
+
+static void ad7173_ida_destroy(void *data)
+{
+	struct ad7173_state *st = data;
+
+	ida_destroy(&st->cfg_slots_status);
+}
+
+static void ad7173_reset_usage_cnts(struct ad7173_state *st)
+{
+	memset64(st->config_cnts, 0, st->info->num_configs);
+	st->config_usage_counter = 0;
+}
+
+static struct ad7173_channel_config *
+ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
+{
+	struct ad7173_channel_config *cfg_aux;
+	ptrdiff_t cmp_size;
+	int i;
+
+	cmp_size = sizeof_field(struct ad7173_channel_config, config_props);
+	for (i = 0; i < st->num_channels; i++) {
+		cfg_aux = &st->channels[i].cfg;
+
+		if (cfg_aux->live &&
+		    !memcmp(&cfg->config_props, &cfg_aux->config_props, cmp_size))
+			return cfg_aux;
+	}
+	return NULL;
+}
+
+/* Could be replaced with a generic LRU implementation */
+static int ad7173_free_config_slot_lru(struct ad7173_state *st)
+{
+	int i, lru_position = 0;
+
+	for (i = 1; i < st->info->num_configs; i++)
+		if (st->config_cnts[i] < st->config_cnts[lru_position])
+			lru_position = i;
+
+	for (i = 0; i < st->num_channels; i++)
+		if (st->channels[i].cfg.cfg_slot == lru_position)
+			st->channels[i].cfg.live = false;
+
+	ida_free(&st->cfg_slots_status, lru_position);
+	return ida_alloc(&st->cfg_slots_status, GFP_KERNEL);
+}
+
+/* Could be replaced with a generic LRU implementation */
+static int ad7173_load_config(struct ad7173_state *st,
+			      struct ad7173_channel_config *cfg)
+{
+	unsigned int config;
+	int free_cfg_slot, ret;
+
+	free_cfg_slot = ida_alloc_range(&st->cfg_slots_status, 0,
+					st->info->num_configs - 1, GFP_KERNEL);
+	if (free_cfg_slot < 0)
+		free_cfg_slot = ad7173_free_config_slot_lru(st);
+
+	cfg->cfg_slot = free_cfg_slot;
+	config = FIELD_PREP(AD7173_SETUP_REF_SEL_MASK, cfg->ref_sel);
+
+	if (cfg->bipolar)
+		config |= AD7173_SETUP_BIPOLAR;
+
+	if (cfg->input_buf)
+		config |= AD7173_SETUP_AIN_BUF_MASK;
+
+	ret = ad_sd_write_reg(&st->sd, AD7173_REG_SETUP(free_cfg_slot), 2, config);
+	if (ret)
+		return ret;
+
+	return ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(free_cfg_slot), 2,
+			       AD7173_FILTER_ODR0_MASK & cfg->odr);
+}
+
+static int ad7173_config_channel(struct ad7173_state *st, int addr)
+{
+	struct ad7173_channel_config *cfg = &st->channels[addr].cfg;
+	struct ad7173_channel_config *live_cfg;
+	int ret;
+
+	if (!cfg->live) {
+		live_cfg = ad7173_find_live_config(st, cfg);
+		if (live_cfg) {
+			cfg->cfg_slot = live_cfg->cfg_slot;
+		} else {
+			ret = ad7173_load_config(st, cfg);
+			if (ret)
+				return ret;
+			cfg->live = true;
+		}
+	}
+
+	if (st->config_usage_counter == U64_MAX)
+		ad7173_reset_usage_cnts(st);
+
+	st->config_usage_counter++;
+	st->config_cnts[cfg->cfg_slot] = st->config_usage_counter;
+
+	return 0;
+}
+
+static int ad7173_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+	unsigned int val;
+	int ret;
+
+	ret = ad7173_config_channel(st, channel);
+	if (ret)
+		return ret;
+
+	val = AD7173_CH_ENABLE |
+	      FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, st->channels[channel].cfg.cfg_slot) |
+	      st->channels[channel].ain;
+
+	return ad_sd_write_reg(&st->sd, AD7173_REG_CH(channel), 2, val);
+}
+
+static int ad7173_set_mode(struct ad_sigma_delta *sd,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+
+	st->adc_mode &= ~AD7173_ADC_MODE_MODE_MASK;
+	st->adc_mode |= FIELD_PREP(AD7173_ADC_MODE_MODE_MASK, mode);
+
+	return ad_sd_write_reg(&st->sd, AD7173_REG_ADC_MODE, 2, st->adc_mode);
+}
+
+static int ad7173_append_status(struct ad_sigma_delta *sd, bool append)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+	unsigned int interface_mode = st->interface_mode;
+	int ret;
+
+	interface_mode |= AD7173_INTERFACE_DATA_STAT_EN(append);
+	ret = ad_sd_write_reg(&st->sd, AD7173_REG_INTERFACE_MODE, 2, interface_mode);
+	if (ret)
+		return ret;
+
+	st->interface_mode = interface_mode;
+
+	return 0;
+}
+
+static int ad7173_disable_all(struct ad_sigma_delta *sd)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+	int ret;
+	int i;
+
+	for (i = 0; i < st->num_channels; i++) {
+		ret = ad_sd_write_reg(sd, AD7173_REG_CH(i), 2, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
+	.set_channel = ad7173_set_channel,
+	.append_status = ad7173_append_status,
+	.disable_all = ad7173_disable_all,
+	.set_mode = ad7173_set_mode,
+	.has_registers = true,
+	.addr_shift = 0,
+	.read_mask = BIT(6),
+	.status_ch_mask = GENMASK(3, 0),
+	.data_reg = AD7173_REG_DATA,
+	.irq_flags = IRQF_TRIGGER_FALLING,
+};
+
+static int ad7173_setup(struct iio_dev *indio_dev)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	u8 buf[AD7173_RESET_LENGTH];
+	unsigned int id;
+	int ret;
+
+	/* reset the serial interface */
+	memset(buf, 0xff, AD7173_RESET_LENGTH);
+	ret = spi_write_then_read(st->sd.spi, buf, sizeof(buf), NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	/* datasheet recommends a delay of at least 500us after reset */
+	fsleep(500);
+
+	ret = ad_sd_read_reg(&st->sd, AD7173_REG_ID, 2, &id);
+	if (ret)
+		return ret;
+
+	id &= AD7173_ID_MASK;
+	if (id != st->info->id)
+		dev_warn(&st->sd.spi->dev,
+			 "Unexpected device id: 0x%04X, expected: 0x%04X\n",
+			 id, st->info->id);
+
+	st->adc_mode |= AD7173_ADC_MODE_SING_CYC;
+	st->interface_mode = 0x0;
+
+	st->config_usage_counter = 0;
+	st->config_cnts = devm_kcalloc(&st->sd.spi->dev, st->info->num_configs,
+				       sizeof(u64), GFP_KERNEL);
+	if (!st->config_cnts)
+		return -ENOMEM;
+
+	/* All channels are enabled by default after a reset */
+	return ad7173_disable_all(&st->sd);
+}
+
+static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
+						 u8 reference_select)
+{
+	int vref;
+
+	switch (reference_select) {
+	case AD7173_SETUP_REF_SEL_EXT_REF:
+		vref = regulator_get_voltage(st->regulators[0].consumer);
+		break;
+
+	case AD7173_SETUP_REF_SEL_EXT_REF2:
+		vref = regulator_get_voltage(st->regulators[1].consumer);
+		break;
+
+	case AD7173_SETUP_REF_SEL_INT_REF:
+		vref = AD7173_VOLTAGE_INT_REF_uV;
+		break;
+
+	case AD7173_SETUP_REF_SEL_AVDD1_AVSS:
+		vref = regulator_get_voltage(st->regulators[2].consumer);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	if (vref < 0)
+		return vref;
+
+	return vref / (MICRO / MILLI);
+}
+
+static int ad7173_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *ch = &st->channels[chan->address];
+	unsigned int reg, temp;
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
+		if (ret < 0)
+			return ret;
+
+		/* disable channel after single conversion */
+		ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_TEMP) {
+			temp = ((u32)AD7173_VOLTAGE_INT_REF_uV) * MILLI;
+			temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
+			*val = temp;
+			*val2 = chan->scan_type.realbits;
+		} else {
+			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
+			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+		}
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		if (chan->type == IIO_TEMP)
+			*val = -874379; //-milli_kelvin_to_millicelsius(0)/scale
+		else
+			*val = -BIT(chan->scan_type.realbits - 1);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		reg = st->channels[chan->address].cfg.odr;
+
+		*val = st->info->sinc5_data_rates[reg] / MILLI;
+		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO / MILLI);
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7173_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel_config *cfg;
+	unsigned int freq, i, reg;
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		freq = val * MILLI + val2 / MILLI;
+		for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++)
+			if (freq >= st->info->sinc5_data_rates[i])
+				break;
+
+		cfg = &st->channels[chan->address].cfg;
+		cfg->odr = i;
+
+		if (!cfg->live)
+			break;
+
+		ret = ad_sd_read_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, &reg);
+		if (ret)
+			break;
+		reg &= ~AD7173_FILTER_ODR0_MASK;
+		reg |= FIELD_PREP(AD7173_FILTER_ODR0_MASK, i);
+		ret = ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, reg);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	iio_device_release_direct_mode(indio_dev);
+	return ret;
+}
+
+static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	int i, ret;
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		if (test_bit(i, scan_mask))
+			ret = ad7173_set_channel(&st->sd, i);
+		else
+			ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				   unsigned int writeval, unsigned int *readval)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	u8 reg_size;
+
+	if (reg == 0)
+		reg_size = 1;
+	else if (reg == AD7173_REG_CRC || reg == AD7173_REG_DATA ||
+		 reg >= AD7173_REG_OFFSET(0))
+		reg_size = 3;
+	else
+		reg_size = 2;
+
+	if (readval)
+		return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
+
+	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
+}
+
+static const struct iio_info ad7173_info = {
+	.read_raw = &ad7173_read_raw,
+	.write_raw = &ad7173_write_raw,
+	.debugfs_reg_access = &ad7173_debug_reg_access,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7173_update_scan_mode,
+};
+
+static const struct iio_chan_spec ad7173_channel_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static const struct iio_chan_spec ad7173_temp_iio_channel_template = {
+	.type = IIO_TEMP,
+	.indexed = 1,
+	.channel = AD7173_AIN_TEMP_POS,
+	.channel2 = AD7173_AIN_TEMP_NEG,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static void ad7173_disable_regulators(void *data)
+{
+	struct ad7173_state *st = data;
+
+	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
+}
+
+static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
+{
+	struct ad7173_channel *channels_st_priv_arr, *chan_st_priv;
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_chan_spec *chan_arr, *chan;
+	struct fwnode_handle *child;
+	unsigned int ain[2], chan_index = 0;
+	unsigned int num_channels;
+	int ref_sel;
+	int ret;
+
+	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
+	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
+	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
+
+	/*
+	 * If a regulator is not available, it will be set to a dummy regulator.
+	 * Each channel reference is checked with regulator_get_voltage() before
+	 * setting attributes so if any channel uses a dummy supply the driver
+	 * probe will fail.
+	 */
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
+				      st->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add regulators disable action\n");
+
+	num_channels = device_get_child_node_count(dev);
+
+	if (st->info->has_temp)
+		num_channels++;
+
+	if (num_channels == 0)
+		return dev_err_probe(dev, -EINVAL, "No channels specified\n");
+	st->num_channels = num_channels;
+
+	chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels, GFP_KERNEL);
+	if (!chan_arr)
+		return -ENOMEM;
+
+	channels_st_priv_arr = devm_kcalloc(dev, num_channels,
+					    sizeof(*channels_st_priv_arr),
+					    GFP_KERNEL);
+	if (!channels_st_priv_arr)
+		return -ENOMEM;
+
+	indio_dev->channels = chan_arr;
+	indio_dev->num_channels = num_channels;
+	st->channels = channels_st_priv_arr;
+
+	if (st->info->has_temp) {
+		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
+		chan_st_priv = &channels_st_priv_arr[chan_index];
+		chan_st_priv->ain =
+			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
+		chan_st_priv->cfg.bipolar = false;
+		chan_st_priv->cfg.input_buf = true;
+		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
+		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
+
+		chan_index++;
+	}
+
+	device_for_each_child_node(dev, child) {
+		chan = &chan_arr[chan_index];
+		chan_st_priv = &channels_st_priv_arr[chan_index];
+		ret = fwnode_property_read_u32_array(child, "diff-channels",
+						     ain, ARRAY_SIZE(ain));
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		if (ain[0] >= st->info->num_inputs ||
+		    ain[1] >= st->info->num_inputs) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL,
+					     "Input pin number out of range for pair (%d %d).\n",
+					     ain[0], ain[1]);
+		}
+
+		ret = fwnode_property_match_property_string(child,
+							    "adi,reference-select",
+							    ad7173_ref_sel_str,
+							    ARRAY_SIZE(ad7173_ref_sel_str));
+
+		if (ret < 0)
+			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
+		else
+			ref_sel = ret;
+
+		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 &&
+		    st->info->id != AD7173_ID) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8\n");
+		}
+
+		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+		if (ret < 0) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, ret,
+					     "Cannot use reference %u\n", ref_sel);
+		}
+		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
+			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
+		chan_st_priv->cfg.ref_sel = ref_sel;
+
+		*chan = ad7173_channel_template;
+		chan->address = chan_index;
+		chan->scan_index = chan_index;
+		chan->channel = ain[0];
+		chan->channel2 = ain[1];
+		chan->differential = true;
+
+		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+		chan_st_priv->chan_reg = chan_index;
+		chan_st_priv->cfg.input_buf = true;
+		chan_st_priv->cfg.odr = 0;
+
+		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
+		if (chan_st_priv->cfg.bipolar)
+			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
+
+		chan_index++;
+	}
+
+	return 0;
+}
+
+static int ad7173_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ad7173_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
+	ida_init(&st->cfg_slots_status);
+	ret = devm_add_action_or_reset(dev, ad7173_ida_destroy, st);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->info->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad7173_info;
+
+	spi->mode = SPI_MODE_3;
+
+	ad7173_sigma_delta_info.num_slots = st->info->num_configs;
+	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
+	if (ret)
+		return ret;
+
+	ret = ad7173_fw_parse_channel_config(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7173_setup(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_GPIOLIB))
+		return ad7173_gpio_init(st);
+
+	return 0;
+}
+
+static const struct of_device_id ad7173_of_match[] = {
+	{ .compatible = "adi,ad7172-2",
+	  .data = &ad7173_device_info[ID_AD7172_2]},
+	{ .compatible = "adi,ad7173-8",
+	  .data = &ad7173_device_info[ID_AD7173_8]},
+	{ .compatible = "adi,ad7175-2",
+	  .data = &ad7173_device_info[ID_AD7175_2]},
+	{ .compatible = "adi,ad7176-2",
+	  .data = &ad7173_device_info[ID_AD7176_2]},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7173_of_match);
+
+static const struct spi_device_id ad7173_id_table[] = {
+	{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
+	{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
+	{ "ad7175-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_2]},
+	{ "ad7176-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7176_2]},
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad7173_id_table);
+
+static struct spi_driver ad7173_driver = {
+	.driver = {
+		.name	= "ad7173",
+		.of_match_table = ad7173_of_match,
+	},
+	.probe		= ad7173_probe,
+	.id_table	= ad7173_id_table,
+};
+module_spi_driver(ad7173_driver);
+
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
+MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.42.0


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

* [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
@ 2023-12-12 10:44   ` Dumitru Ceclan
  0 siblings, 0 replies; 17+ messages in thread
From: Dumitru Ceclan @ 2023-12-12 10:44 UTC (permalink / raw)
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel, Dumitru Ceclan

The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel
applications or higher speed multiplexed applications. The Sigma-Delta
ADC is intended primarily for measurement of signals close to DC but also
delivers outstanding performance with input bandwidths out to ~10kHz.

Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V7->V8
 - remove Kconfig commas
 - reorder include <...gpio/regmap.h>
 - GPO12_DATA(x)  from BIT(x) -> BIT(x+0)
 - use the same pointer to dev in ad7173_setup()
 - rewrite temp channel read_raw scale for better readability
 - add comment to explain temp channel offset
 - follow multi-line comment style
 - change ref_sel type to signed int
 - use fwnode_property_match_property_string()

 drivers/iio/adc/Kconfig  |  17 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad7173.c | 964 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 982 insertions(+)
 create mode 100644 drivers/iio/adc/ad7173.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1e2b7a2c67c6..1d7b9ade4d59 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -54,6 +54,23 @@ config AD7124
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7124.
 
+config AD7173
+	tristate "Analog Devices AD7173 driver"
+	depends on SPI_MASTER
+	select AD_SIGMA_DELTA
+	select GPIO_REGMAP if GPIOLIB
+	select REGMAP_SPI if GPIOLIB
+	help
+	  Say yes here to build support for Analog Devices AD7173 and similar ADC
+	  Currently supported models:
+	   - AD7172-2
+	   - AD7173-8
+	   - AD7175-2
+	   - AD7176-2
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad7173.
+
 config AD7192
 	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index c0803383a7cc..12e723cdc1f1 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
 obj-$(CONFIG_AD7124) += ad7124.o
+obj-$(CONFIG_AD7173) += ad7173.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7266) += ad7266.o
 obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
new file mode 100644
index 000000000000..96918b24a10a
--- /dev/null
+++ b/drivers/iio/adc/ad7173.c
@@ -0,0 +1,964 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
+ * Copyright (C) 2015, 2023 Analog Devices, Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define AD7173_REG_COMMS		0x00
+#define AD7173_REG_ADC_MODE		0x01
+#define AD7173_REG_INTERFACE_MODE	0x02
+#define AD7173_REG_CRC			0x03
+#define AD7173_REG_DATA			0x04
+#define AD7173_REG_GPIO			0x06
+#define AD7173_REG_ID			0x07
+#define AD7173_REG_CH(x)		(0x10 + (x))
+#define AD7173_REG_SETUP(x)		(0x20 + (x))
+#define AD7173_REG_FILTER(x)		(0x28 + (x))
+#define AD7173_REG_OFFSET(x)		(0x30 + (x))
+#define AD7173_REG_GAIN(x)		(0x38 + (x))
+
+#define AD7173_RESET_LENGTH		BITS_TO_BYTES(64)
+
+#define AD7173_CH_ENABLE		BIT(15)
+#define AD7173_CH_SETUP_SEL_MASK	GENMASK(14, 12)
+#define AD7173_CH_SETUP_AINPOS_MASK	GENMASK(9, 5)
+#define AD7173_CH_SETUP_AINNEG_MASK	GENMASK(4, 0)
+
+#define AD7173_CH_ADDRESS(pos, neg) \
+	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
+	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
+#define AD7173_AIN_TEMP_POS	17
+#define AD7173_AIN_TEMP_NEG	18
+
+#define AD7172_ID			0x00d0
+#define AD7173_ID			0x30d0
+#define AD7175_ID			0x0cd0
+#define AD7176_ID			0x0c90
+#define AD7173_ID_MASK			GENMASK(15, 4)
+
+#define AD7173_ADC_MODE_REF_EN		BIT(15)
+#define AD7173_ADC_MODE_SING_CYC	BIT(13)
+#define AD7173_ADC_MODE_MODE_MASK	GENMASK(6, 4)
+#define AD7173_ADC_MODE_CLOCKSEL_MASK	GENMASK(3, 2)
+
+#define AD7173_GPIO_PDSW	BIT(14)
+#define AD7173_GPIO_OP_EN2_3	BIT(13)
+#define AD7173_GPIO_MUX_IO	BIT(12)
+#define AD7173_GPIO_SYNC_EN	BIT(11)
+#define AD7173_GPIO_ERR_EN	BIT(10)
+#define AD7173_GPIO_ERR_DAT	BIT(9)
+#define AD7173_GPIO_GP_DATA3	BIT(7)
+#define AD7173_GPIO_GP_DATA2	BIT(6)
+#define AD7173_GPIO_IP_EN1	BIT(5)
+#define AD7173_GPIO_IP_EN0	BIT(4)
+#define AD7173_GPIO_OP_EN1	BIT(3)
+#define AD7173_GPIO_OP_EN0	BIT(2)
+#define AD7173_GPIO_GP_DATA1	BIT(1)
+#define AD7173_GPIO_GP_DATA0	BIT(0)
+
+#define AD7173_GPO12_DATA(x)	BIT(x + 0)
+#define AD7173_GPO23_DATA(x)	BIT(x + 4)
+#define AD7173_GPO_DATA(x)	(x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
+
+#define AD7173_INTERFACE_DATA_STAT	BIT(6)
+#define AD7173_INTERFACE_DATA_STAT_EN(x) \
+	FIELD_PREP(AD7173_INTERFACE_DATA_STAT, x)
+
+#define AD7173_SETUP_BIPOLAR		BIT(12)
+#define AD7173_SETUP_AREF_BUF_MASK	GENMASK(11, 10)
+#define AD7173_SETUP_AIN_BUF_MASK	GENMASK(9, 8)
+
+#define AD7173_SETUP_REF_SEL_MASK	GENMASK(5, 4)
+#define AD7173_SETUP_REF_SEL_AVDD1_AVSS	0x3
+#define AD7173_SETUP_REF_SEL_INT_REF	0x2
+#define AD7173_SETUP_REF_SEL_EXT_REF2	0x1
+#define AD7173_SETUP_REF_SEL_EXT_REF	0x0
+#define AD7173_VOLTAGE_INT_REF_uV	2500000
+#define AD7173_TEMP_SENSIIVITY_uV_per_C	477
+
+#define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
+#define AD7173_MAX_CONFIGS		8
+
+enum ad7173_ids {
+	ID_AD7172_2,
+	ID_AD7173_8,
+	ID_AD7175_2,
+	ID_AD7176_2,
+};
+
+struct ad7173_device_info {
+	const unsigned int *sinc5_data_rates;
+	unsigned int num_sinc5_data_rates;
+	unsigned int num_channels;
+	unsigned int num_configs;
+	unsigned int num_inputs;
+	unsigned int clock;
+	unsigned int id;
+	char *name;
+	bool has_temp;
+	u8 num_gpios;
+};
+
+struct ad7173_channel_config {
+	u8 cfg_slot;
+	bool live;
+
+	/* Following fields are used to compare equality. */
+	struct_group(config_props,
+		bool bipolar;
+		bool input_buf;
+		u8 odr;
+		u8 ref_sel;
+	);
+};
+
+struct ad7173_channel {
+	unsigned int chan_reg;
+	unsigned int ain;
+	struct ad7173_channel_config cfg;
+};
+
+struct ad7173_state {
+	struct ad_sigma_delta sd;
+	const struct ad7173_device_info *info;
+	struct ad7173_channel *channels;
+	struct regulator_bulk_data regulators[3];
+	unsigned int adc_mode;
+	unsigned int interface_mode;
+	unsigned int num_channels;
+	struct ida cfg_slots_status;
+	unsigned long long config_usage_counter;
+	unsigned long long *config_cnts;
+#if IS_ENABLED(CONFIG_GPIOLIB)
+	struct regmap *reg_gpiocon_regmap;
+	struct gpio_regmap *gpio_regmap;
+#endif
+};
+
+static const unsigned int ad7173_sinc5_data_rates[] = {
+	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,
+	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,
+	49680,	 20010,	  16333,   10000,   5000,    2500,    1250,
+};
+
+static const unsigned int ad7175_sinc5_data_rates[] = {
+	50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000,
+	10000000, 5000000,  2500000,  1000000,	500000,	  397500,   200000,
+	100000,	  59920,    49960,    20000,	16666,	  10000,    5000,
+};
+
+static const struct ad7173_device_info ad7173_device_info[] = {
+	[ID_AD7172_2] = {
+		.name = "ad7172-2",
+		.id = AD7172_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.num_gpios = 2,
+		.has_temp = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD7173_8] = {
+		.name = "ad7173-8",
+		.id = AD7173_ID,
+		.num_inputs = 17,
+		.num_channels = 16,
+		.num_configs = 8,
+		.num_gpios = 4,
+		.has_temp = true,
+		.clock = 2 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD7175_2] = {
+		.name = "ad7175-2",
+		.id = AD7175_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.num_gpios = 2,
+		.has_temp = true,
+		.clock = 16 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7175_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+	},
+	[ID_AD7176_2] = {
+		.name = "ad7176-2",
+		.id = AD7176_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.num_gpios = 2,
+		.has_temp = false,
+		.clock = 16 * HZ_PER_MHZ,
+		.sinc5_data_rates = ad7175_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+	},
+};
+
+static const char *const ad7173_ref_sel_str[] = {
+	[AD7173_SETUP_REF_SEL_EXT_REF]    = "refin",
+	[AD7173_SETUP_REF_SEL_EXT_REF2]   = "refin2",
+	[AD7173_SETUP_REF_SEL_INT_REF]    = "refout-avss",
+	[AD7173_SETUP_REF_SEL_AVDD1_AVSS] = "avdd",
+};
+
+#if IS_ENABLED(CONFIG_GPIOLIB)
+
+static const struct regmap_range ad7173_range_gpio[] = {
+	regmap_reg_range(AD7173_REG_GPIO, AD7173_REG_GPIO),
+};
+
+static const struct regmap_access_table ad7173_access_table = {
+	.yes_ranges = ad7173_range_gpio,
+	.n_yes_ranges = ARRAY_SIZE(ad7173_range_gpio),
+};
+
+static const struct regmap_config ad7173_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.rd_table = &ad7173_access_table,
+	.wr_table = &ad7173_access_table,
+	.read_flag_mask = BIT(6),
+};
+
+static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
+			     unsigned int offset, unsigned int *reg,
+			     unsigned int *mask)
+{
+	*mask = AD7173_GPO_DATA(offset);
+	*reg = base;
+	return 0;
+}
+
+static void ad7173_gpio_disable(void *data)
+{
+	struct ad7173_state *st = data;
+	unsigned int mask;
+
+	mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | AD7173_GPIO_OP_EN2_3;
+	regmap_update_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, mask, ~mask);
+}
+
+static int ad7173_gpio_init(struct ad7173_state *st)
+{
+	struct gpio_regmap_config gpio_regmap = {};
+	struct device *dev = &st->sd.spi->dev;
+	unsigned int mask;
+	int ret;
+
+	st->reg_gpiocon_regmap = devm_regmap_init_spi(st->sd.spi, &ad7173_regmap_config);
+	ret = PTR_ERR_OR_ZERO(st->reg_gpiocon_regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to init regmap\n");
+
+	mask = AD7173_GPIO_OP_EN0 | AD7173_GPIO_OP_EN1 | AD7173_GPIO_OP_EN2_3;
+	regmap_update_bits(st->reg_gpiocon_regmap, AD7173_REG_GPIO, mask, mask);
+
+	ret = devm_add_action_or_reset(dev, ad7173_gpio_disable, st);
+	if (ret)
+		return ret;
+
+	gpio_regmap.parent = dev;
+	gpio_regmap.regmap = st->reg_gpiocon_regmap;
+	gpio_regmap.ngpio = st->info->num_gpios;
+	gpio_regmap.reg_set_base = AD7173_REG_GPIO;
+	gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
+
+	st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
+	ret = PTR_ERR_OR_ZERO(st->gpio_regmap);
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to init gpio-regmap\n");
+
+	return 0;
+}
+
+#endif /* CONFIG_GPIOLIB */
+
+static struct ad7173_state *ad_sigma_delta_to_ad7173(struct ad_sigma_delta *sd)
+{
+	return container_of(sd, struct ad7173_state, sd);
+}
+
+static void ad7173_ida_destroy(void *data)
+{
+	struct ad7173_state *st = data;
+
+	ida_destroy(&st->cfg_slots_status);
+}
+
+static void ad7173_reset_usage_cnts(struct ad7173_state *st)
+{
+	memset64(st->config_cnts, 0, st->info->num_configs);
+	st->config_usage_counter = 0;
+}
+
+static struct ad7173_channel_config *
+ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
+{
+	struct ad7173_channel_config *cfg_aux;
+	ptrdiff_t cmp_size;
+	int i;
+
+	cmp_size = sizeof_field(struct ad7173_channel_config, config_props);
+	for (i = 0; i < st->num_channels; i++) {
+		cfg_aux = &st->channels[i].cfg;
+
+		if (cfg_aux->live &&
+		    !memcmp(&cfg->config_props, &cfg_aux->config_props, cmp_size))
+			return cfg_aux;
+	}
+	return NULL;
+}
+
+/* Could be replaced with a generic LRU implementation */
+static int ad7173_free_config_slot_lru(struct ad7173_state *st)
+{
+	int i, lru_position = 0;
+
+	for (i = 1; i < st->info->num_configs; i++)
+		if (st->config_cnts[i] < st->config_cnts[lru_position])
+			lru_position = i;
+
+	for (i = 0; i < st->num_channels; i++)
+		if (st->channels[i].cfg.cfg_slot == lru_position)
+			st->channels[i].cfg.live = false;
+
+	ida_free(&st->cfg_slots_status, lru_position);
+	return ida_alloc(&st->cfg_slots_status, GFP_KERNEL);
+}
+
+/* Could be replaced with a generic LRU implementation */
+static int ad7173_load_config(struct ad7173_state *st,
+			      struct ad7173_channel_config *cfg)
+{
+	unsigned int config;
+	int free_cfg_slot, ret;
+
+	free_cfg_slot = ida_alloc_range(&st->cfg_slots_status, 0,
+					st->info->num_configs - 1, GFP_KERNEL);
+	if (free_cfg_slot < 0)
+		free_cfg_slot = ad7173_free_config_slot_lru(st);
+
+	cfg->cfg_slot = free_cfg_slot;
+	config = FIELD_PREP(AD7173_SETUP_REF_SEL_MASK, cfg->ref_sel);
+
+	if (cfg->bipolar)
+		config |= AD7173_SETUP_BIPOLAR;
+
+	if (cfg->input_buf)
+		config |= AD7173_SETUP_AIN_BUF_MASK;
+
+	ret = ad_sd_write_reg(&st->sd, AD7173_REG_SETUP(free_cfg_slot), 2, config);
+	if (ret)
+		return ret;
+
+	return ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(free_cfg_slot), 2,
+			       AD7173_FILTER_ODR0_MASK & cfg->odr);
+}
+
+static int ad7173_config_channel(struct ad7173_state *st, int addr)
+{
+	struct ad7173_channel_config *cfg = &st->channels[addr].cfg;
+	struct ad7173_channel_config *live_cfg;
+	int ret;
+
+	if (!cfg->live) {
+		live_cfg = ad7173_find_live_config(st, cfg);
+		if (live_cfg) {
+			cfg->cfg_slot = live_cfg->cfg_slot;
+		} else {
+			ret = ad7173_load_config(st, cfg);
+			if (ret)
+				return ret;
+			cfg->live = true;
+		}
+	}
+
+	if (st->config_usage_counter == U64_MAX)
+		ad7173_reset_usage_cnts(st);
+
+	st->config_usage_counter++;
+	st->config_cnts[cfg->cfg_slot] = st->config_usage_counter;
+
+	return 0;
+}
+
+static int ad7173_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+	unsigned int val;
+	int ret;
+
+	ret = ad7173_config_channel(st, channel);
+	if (ret)
+		return ret;
+
+	val = AD7173_CH_ENABLE |
+	      FIELD_PREP(AD7173_CH_SETUP_SEL_MASK, st->channels[channel].cfg.cfg_slot) |
+	      st->channels[channel].ain;
+
+	return ad_sd_write_reg(&st->sd, AD7173_REG_CH(channel), 2, val);
+}
+
+static int ad7173_set_mode(struct ad_sigma_delta *sd,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+
+	st->adc_mode &= ~AD7173_ADC_MODE_MODE_MASK;
+	st->adc_mode |= FIELD_PREP(AD7173_ADC_MODE_MODE_MASK, mode);
+
+	return ad_sd_write_reg(&st->sd, AD7173_REG_ADC_MODE, 2, st->adc_mode);
+}
+
+static int ad7173_append_status(struct ad_sigma_delta *sd, bool append)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+	unsigned int interface_mode = st->interface_mode;
+	int ret;
+
+	interface_mode |= AD7173_INTERFACE_DATA_STAT_EN(append);
+	ret = ad_sd_write_reg(&st->sd, AD7173_REG_INTERFACE_MODE, 2, interface_mode);
+	if (ret)
+		return ret;
+
+	st->interface_mode = interface_mode;
+
+	return 0;
+}
+
+static int ad7173_disable_all(struct ad_sigma_delta *sd)
+{
+	struct ad7173_state *st = ad_sigma_delta_to_ad7173(sd);
+	int ret;
+	int i;
+
+	for (i = 0; i < st->num_channels; i++) {
+		ret = ad_sd_write_reg(sd, AD7173_REG_CH(i), 2, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
+	.set_channel = ad7173_set_channel,
+	.append_status = ad7173_append_status,
+	.disable_all = ad7173_disable_all,
+	.set_mode = ad7173_set_mode,
+	.has_registers = true,
+	.addr_shift = 0,
+	.read_mask = BIT(6),
+	.status_ch_mask = GENMASK(3, 0),
+	.data_reg = AD7173_REG_DATA,
+	.irq_flags = IRQF_TRIGGER_FALLING,
+};
+
+static int ad7173_setup(struct iio_dev *indio_dev)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	u8 buf[AD7173_RESET_LENGTH];
+	unsigned int id;
+	int ret;
+
+	/* reset the serial interface */
+	memset(buf, 0xff, AD7173_RESET_LENGTH);
+	ret = spi_write_then_read(st->sd.spi, buf, sizeof(buf), NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	/* datasheet recommends a delay of at least 500us after reset */
+	fsleep(500);
+
+	ret = ad_sd_read_reg(&st->sd, AD7173_REG_ID, 2, &id);
+	if (ret)
+		return ret;
+
+	id &= AD7173_ID_MASK;
+	if (id != st->info->id)
+		dev_warn(&st->sd.spi->dev,
+			 "Unexpected device id: 0x%04X, expected: 0x%04X\n",
+			 id, st->info->id);
+
+	st->adc_mode |= AD7173_ADC_MODE_SING_CYC;
+	st->interface_mode = 0x0;
+
+	st->config_usage_counter = 0;
+	st->config_cnts = devm_kcalloc(&st->sd.spi->dev, st->info->num_configs,
+				       sizeof(u64), GFP_KERNEL);
+	if (!st->config_cnts)
+		return -ENOMEM;
+
+	/* All channels are enabled by default after a reset */
+	return ad7173_disable_all(&st->sd);
+}
+
+static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
+						 u8 reference_select)
+{
+	int vref;
+
+	switch (reference_select) {
+	case AD7173_SETUP_REF_SEL_EXT_REF:
+		vref = regulator_get_voltage(st->regulators[0].consumer);
+		break;
+
+	case AD7173_SETUP_REF_SEL_EXT_REF2:
+		vref = regulator_get_voltage(st->regulators[1].consumer);
+		break;
+
+	case AD7173_SETUP_REF_SEL_INT_REF:
+		vref = AD7173_VOLTAGE_INT_REF_uV;
+		break;
+
+	case AD7173_SETUP_REF_SEL_AVDD1_AVSS:
+		vref = regulator_get_voltage(st->regulators[2].consumer);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	if (vref < 0)
+		return vref;
+
+	return vref / (MICRO / MILLI);
+}
+
+static int ad7173_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel *ch = &st->channels[chan->address];
+	unsigned int reg, temp;
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
+		if (ret < 0)
+			return ret;
+
+		/* disable channel after single conversion */
+		ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_TEMP) {
+			temp = ((u32)AD7173_VOLTAGE_INT_REF_uV) * MILLI;
+			temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
+			*val = temp;
+			*val2 = chan->scan_type.realbits;
+		} else {
+			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
+			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+		}
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		if (chan->type == IIO_TEMP)
+			*val = -874379; //-milli_kelvin_to_millicelsius(0)/scale
+		else
+			*val = -BIT(chan->scan_type.realbits - 1);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		reg = st->channels[chan->address].cfg.odr;
+
+		*val = st->info->sinc5_data_rates[reg] / MILLI;
+		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO / MILLI);
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad7173_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct ad7173_channel_config *cfg;
+	unsigned int freq, i, reg;
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		freq = val * MILLI + val2 / MILLI;
+		for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++)
+			if (freq >= st->info->sinc5_data_rates[i])
+				break;
+
+		cfg = &st->channels[chan->address].cfg;
+		cfg->odr = i;
+
+		if (!cfg->live)
+			break;
+
+		ret = ad_sd_read_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, &reg);
+		if (ret)
+			break;
+		reg &= ~AD7173_FILTER_ODR0_MASK;
+		reg |= FIELD_PREP(AD7173_FILTER_ODR0_MASK, i);
+		ret = ad_sd_write_reg(&st->sd, AD7173_REG_FILTER(cfg->cfg_slot), 2, reg);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	iio_device_release_direct_mode(indio_dev);
+	return ret;
+}
+
+static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	int i, ret;
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		if (test_bit(i, scan_mask))
+			ret = ad7173_set_channel(&st->sd, i);
+		else
+			ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+				   unsigned int writeval, unsigned int *readval)
+{
+	struct ad7173_state *st = iio_priv(indio_dev);
+	u8 reg_size;
+
+	if (reg == 0)
+		reg_size = 1;
+	else if (reg == AD7173_REG_CRC || reg == AD7173_REG_DATA ||
+		 reg >= AD7173_REG_OFFSET(0))
+		reg_size = 3;
+	else
+		reg_size = 2;
+
+	if (readval)
+		return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
+
+	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
+}
+
+static const struct iio_info ad7173_info = {
+	.read_raw = &ad7173_read_raw,
+	.write_raw = &ad7173_write_raw,
+	.debugfs_reg_access = &ad7173_debug_reg_access,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad7173_update_scan_mode,
+};
+
+static const struct iio_chan_spec ad7173_channel_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static const struct iio_chan_spec ad7173_temp_iio_channel_template = {
+	.type = IIO_TEMP,
+	.indexed = 1,
+	.channel = AD7173_AIN_TEMP_POS,
+	.channel2 = AD7173_AIN_TEMP_NEG,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
+static void ad7173_disable_regulators(void *data)
+{
+	struct ad7173_state *st = data;
+
+	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
+}
+
+static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
+{
+	struct ad7173_channel *channels_st_priv_arr, *chan_st_priv;
+	struct ad7173_state *st = iio_priv(indio_dev);
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_chan_spec *chan_arr, *chan;
+	struct fwnode_handle *child;
+	unsigned int ain[2], chan_index = 0;
+	unsigned int num_channels;
+	int ref_sel;
+	int ret;
+
+	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
+	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
+	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
+
+	/*
+	 * If a regulator is not available, it will be set to a dummy regulator.
+	 * Each channel reference is checked with regulator_get_voltage() before
+	 * setting attributes so if any channel uses a dummy supply the driver
+	 * probe will fail.
+	 */
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
+				      st->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulators\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add regulators disable action\n");
+
+	num_channels = device_get_child_node_count(dev);
+
+	if (st->info->has_temp)
+		num_channels++;
+
+	if (num_channels == 0)
+		return dev_err_probe(dev, -EINVAL, "No channels specified\n");
+	st->num_channels = num_channels;
+
+	chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels, GFP_KERNEL);
+	if (!chan_arr)
+		return -ENOMEM;
+
+	channels_st_priv_arr = devm_kcalloc(dev, num_channels,
+					    sizeof(*channels_st_priv_arr),
+					    GFP_KERNEL);
+	if (!channels_st_priv_arr)
+		return -ENOMEM;
+
+	indio_dev->channels = chan_arr;
+	indio_dev->num_channels = num_channels;
+	st->channels = channels_st_priv_arr;
+
+	if (st->info->has_temp) {
+		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
+		chan_st_priv = &channels_st_priv_arr[chan_index];
+		chan_st_priv->ain =
+			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
+		chan_st_priv->cfg.bipolar = false;
+		chan_st_priv->cfg.input_buf = true;
+		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
+		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
+
+		chan_index++;
+	}
+
+	device_for_each_child_node(dev, child) {
+		chan = &chan_arr[chan_index];
+		chan_st_priv = &channels_st_priv_arr[chan_index];
+		ret = fwnode_property_read_u32_array(child, "diff-channels",
+						     ain, ARRAY_SIZE(ain));
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		if (ain[0] >= st->info->num_inputs ||
+		    ain[1] >= st->info->num_inputs) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL,
+					     "Input pin number out of range for pair (%d %d).\n",
+					     ain[0], ain[1]);
+		}
+
+		ret = fwnode_property_match_property_string(child,
+							    "adi,reference-select",
+							    ad7173_ref_sel_str,
+							    ARRAY_SIZE(ad7173_ref_sel_str));
+
+		if (ret < 0)
+			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
+		else
+			ref_sel = ret;
+
+		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 &&
+		    st->info->id != AD7173_ID) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8\n");
+		}
+
+		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+		if (ret < 0) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, ret,
+					     "Cannot use reference %u\n", ref_sel);
+		}
+		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
+			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
+		chan_st_priv->cfg.ref_sel = ref_sel;
+
+		*chan = ad7173_channel_template;
+		chan->address = chan_index;
+		chan->scan_index = chan_index;
+		chan->channel = ain[0];
+		chan->channel2 = ain[1];
+		chan->differential = true;
+
+		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+		chan_st_priv->chan_reg = chan_index;
+		chan_st_priv->cfg.input_buf = true;
+		chan_st_priv->cfg.odr = 0;
+
+		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
+		if (chan_st_priv->cfg.bipolar)
+			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
+
+		chan_index++;
+	}
+
+	return 0;
+}
+
+static int ad7173_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ad7173_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
+	ida_init(&st->cfg_slots_status);
+	ret = devm_add_action_or_reset(dev, ad7173_ida_destroy, st);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->info->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad7173_info;
+
+	spi->mode = SPI_MODE_3;
+
+	ad7173_sigma_delta_info.num_slots = st->info->num_configs;
+	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
+	if (ret)
+		return ret;
+
+	ret = ad7173_fw_parse_channel_config(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_ad_sd_setup_buffer_and_trigger(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad7173_setup(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_GPIOLIB))
+		return ad7173_gpio_init(st);
+
+	return 0;
+}
+
+static const struct of_device_id ad7173_of_match[] = {
+	{ .compatible = "adi,ad7172-2",
+	  .data = &ad7173_device_info[ID_AD7172_2]},
+	{ .compatible = "adi,ad7173-8",
+	  .data = &ad7173_device_info[ID_AD7173_8]},
+	{ .compatible = "adi,ad7175-2",
+	  .data = &ad7173_device_info[ID_AD7175_2]},
+	{ .compatible = "adi,ad7176-2",
+	  .data = &ad7173_device_info[ID_AD7176_2]},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7173_of_match);
+
+static const struct spi_device_id ad7173_id_table[] = {
+	{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
+	{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
+	{ "ad7175-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_2]},
+	{ "ad7176-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7176_2]},
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad7173_id_table);
+
+static struct spi_driver ad7173_driver = {
+	.driver = {
+		.name	= "ad7173",
+		.of_match_table = ad7173_of_match,
+	},
+	.probe		= ad7173_probe,
+	.id_table	= ad7173_id_table,
+};
+module_spi_driver(ad7173_driver);
+
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
+MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_LICENSE("GPL");
-- 
2.42.0


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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-12 10:44 ` Dumitru Ceclan
  (?)
  (?)
@ 2023-12-12 15:09 ` David Lechner
  2023-12-14 12:43   ` Ceclan Dumitru
  -1 siblings, 1 reply; 17+ messages in thread
From: David Lechner @ 2023-12-12 15:09 UTC (permalink / raw)
  To: Dumitru Ceclan
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel

On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
>
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.

As stated in [1], we should try to make complete bindings. I think
more could be done here to make this more complete. Most notably, the
gpio-controller binding is missing. Also maybe something is needed to
describe how the SYNC/ERROR pin is wired up since it can be an input
or an output with different functions?

[1]: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html

>
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---
> V7->V8
>  - include missing fix from V6

Including the cumulative changelog for all revisions would be helpful
to reviewers who haven't been following closely.

>
>  .../bindings/iio/adc/adi,ad7173.yaml          | 170 ++++++++++++++++++
>  1 file changed, 170 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> new file mode 100644
> index 000000000000..25a5404ee353
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -0,0 +1,170 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7173 ADC
> +
> +maintainers:
> +  - Ceclan Dumitru <dumitru.ceclan@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7172-2
> +      - adi,ad7173-8
> +      - adi,ad7175-2
> +      - adi,ad7176-2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
pin. Although I could see how RDY could be considered part of the SPI
bus. In any case, a description explaining what the interrupt is would
be useful.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  spi-max-frequency:
> +    maximum: 20000000
> +
> +  refin-supply:
> +    description: external reference supply, can be used as reference for conversion.
> +
> +  refin2-supply:
> +    description: external reference supply, can be used as reference for conversion.
> +
> +  avdd-supply:
> +    description: avdd supply, can be used as reference for conversion.

What about other supplies? AVDD1, AVDD2, IOVDD.


> +
> +patternProperties:
> +  "^channel@[0-9a-f]$":
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 15
> +
> +      diff-channels:
> +        items:
> +          minimum: 0
> +          maximum: 31

Do we need to add overrides to limit the maximums for each compatible string?

> +
> +      adi,reference-select:
> +        description: |
> +          Select the reference source to use when converting on
> +          the specific channel. Valid values are:
> +          refin      : REFIN(+)/REFIN(−).
> +          refin2     : REFIN2(+)/REFIN2(−)
> +          refout-avss: REFOUT/AVSS (Internal reference)
> +          avdd       : AVDD
> +
> +          External reference refin2 only available on ad7173-8.
> +          If not specified, internal reference used.
> +        enum:
> +          - refin
> +          - refin2
> +          - refout-avss
> +          - avdd
> +        default: refout-avss

Missing string type?

> +
> +    required:
> +      - reg
> +      - diff-channels

Individual analog inputs can be used as single-ended or in pairs as
differential, right? If so, diff-channels should not be required to
allow for single-ended use.

And we would need to add something like a single-ended-channel
property to adc.yaml to allow mapping analog input pins to channels
similar to how diff-channels works, I think (I don't see anything like
that there already)?

So maybe something like:

oneOf:
  - required:
      single-ended-channel
  - required:
      diff-channels

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              const: adi,ad7173-8
> +    then:
> +      properties:
> +        refin2-supply: false
> +      patternProperties:
> +        "^channel@[0-9a-f]$":
> +          properties:
> +            adi,reference-select:
> +              enum:
> +                - refin
> +                - refout-avss
> +                - avdd
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +        compatible = "adi,ad7173-8";
> +        reg = <0>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> +        interrupt-parent = <&gpio>;
> +        spi-max-frequency = <5000000>;
> +
> +        refin-supply = <&dummy_regulator>;
> +
> +        channel@0 {
> +          reg = <0>;
> +          bipolar;
> +          diff-channels = <0 1>;
> +          adi,reference-select = "refin";
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <2 3>;
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          bipolar;
> +          diff-channels = <4 5>;
> +        };
> +
> +        channel@3 {
> +          reg = <3>;
> +          bipolar;
> +          diff-channels = <6 7>;
> +        };
> +
> +        channel@4 {
> +          reg = <4>;
> +          diff-channels = <8 9>;
> +          adi,reference-select = "avdd";
> +        };
> +      };
> +    };
> --
> 2.42.0
>
>

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

* Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-12 10:44   ` Dumitru Ceclan
  (?)
@ 2023-12-13 14:24   ` Andy Shevchenko
  2023-12-14 11:02     ` Ceclan Dumitru
  -1 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-13 14:24 UTC (permalink / raw)
  To: Dumitru Ceclan
  Cc: linus.walleij, brgl, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Arnd Bergmann, ChiaEn Wu, Niklas Schnelle,
	Leonard Göhrs, Mike Looijmans, Haibo Chen, Hugo Villeneuve,
	Ceclan Dumitru, linux-iio, devicetree, linux-kernel

On Tue, Dec 12, 2023 at 12:44:36PM +0200, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.

I do not see any major problem in the code,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

Some nit-picks below, but it's fine if it get addressed later on. Up to you
and Jonathan.

...

> +static const unsigned int ad7173_sinc5_data_rates[] = {
> +	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,
> +	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,
> +	49680,	 20010,	  16333,   10000,   5000,    2500,    1250,
> +};
> +
> +static const unsigned int ad7175_sinc5_data_rates[] = {
> +	50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000,

I would add a comment with offsets, like

	... /* 0-6 */

But better to make it power of two, like each 4 on one line or 8.

> +	10000000, 5000000,  2500000,  1000000,	500000,	  397500,   200000,
> +	100000,	  59920,    49960,    20000,	16666,	  10000,    5000,
> +};

Not that I insist, just consider readability of these tables.

...

> +		if (chan->type == IIO_TEMP) {
> +			temp = ((u32)AD7173_VOLTAGE_INT_REF_uV) * MILLI;

Hmm... Is the casting mandatory here?

> +			temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
> +			*val = temp;
> +			*val2 = chan->scan_type.realbits;
> +		} else {
> +			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> +			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +		}

...

> +		if (chan->type == IIO_TEMP)
> +			*val = -874379; //-milli_kelvin_to_millicelsius(0)/scale

Hmm... Besides C99 comment format, can we actually use the mentioned API?
In such a case the comment won't be needed and the value semantics is better
to get.

> +		else
> +			*val = -BIT(chan->scan_type.realbits - 1);

...

> +static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> +				   unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	u8 reg_size;
> +
> +	if (reg == 0)

0 does not have its definition, does it?

> +		reg_size = 1;
> +	else if (reg == AD7173_REG_CRC || reg == AD7173_REG_DATA ||
> +		 reg >= AD7173_REG_OFFSET(0))
> +		reg_size = 3;
> +	else
> +		reg_size = 2;
> +
> +	if (readval)
> +		return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
> +
> +	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
> +}

...

> +	channels_st_priv_arr = devm_kcalloc(dev, num_channels,
> +					    sizeof(*channels_st_priv_arr),
> +					    GFP_KERNEL);
> +	if (!channels_st_priv_arr)
> +		return -ENOMEM;

The variable name can be made shorter and hence the above will take less LoCs.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-13 14:24   ` Andy Shevchenko
@ 2023-12-14 11:02     ` Ceclan Dumitru
  0 siblings, 0 replies; 17+ messages in thread
From: Ceclan Dumitru @ 2023-12-14 11:02 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linus.walleij, brgl, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Arnd Bergmann, ChiaEn Wu, Niklas Schnelle,
	Leonard Göhrs, Mike Looijmans, Haibo Chen, Hugo Villeneuve,
	Ceclan Dumitru, linux-iio, devicetree, linux-kernel



On 12/13/23 16:24, Andy Shevchenko wrote:
> On Tue, Dec 12, 2023 at 12:44:36PM +0200, Dumitru Ceclan wrote:
> ...
> 
>> +		if (chan->type == IIO_TEMP) {
>> +			temp = ((u32)AD7173_VOLTAGE_INT_REF_uV) * MILLI;
> 
> Hmm... Is the casting mandatory here?
> 

Yep, not really needed as MILLI is already declared as unsigned and it
will promote the _INT_REF as well. On signed32 it would have overflowed.

Are there any cases where this would not be alright?

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

* Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-12 10:44   ` Dumitru Ceclan
  (?)
  (?)
@ 2023-12-14 12:30   ` Jonathan Cameron
  2023-12-14 12:57     ` Ceclan Dumitru
  -1 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2023-12-14 12:30 UTC (permalink / raw)
  To: Dumitru Ceclan
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel

On Tue, 12 Dec 2023 12:44:36 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
> 
> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
Hi

Given it seems like you'll be doing a v9, one quick comment from me below.

Jonathan

> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> new file mode 100644
> index 000000000000..96918b24a10a
> --- /dev/null
> +++ b/drivers/iio/adc/ad7173.c
> @@ -0,0 +1,964 @@
...

> +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> +{

...

> +
> +	if (st->info->has_temp) {
> +		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
> +		chan_st_priv = &channels_st_priv_arr[chan_index];
> +		chan_st_priv->ain =
> +			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
> +		chan_st_priv->cfg.bipolar = false;
> +		chan_st_priv->cfg.input_buf = true;
> +		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> +
> +		chan_index++;
> +	}
> +
> +	device_for_each_child_node(dev, child) {
> +		chan = &chan_arr[chan_index];
> +		chan_st_priv = &channels_st_priv_arr[chan_index];
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     ain, ARRAY_SIZE(ain));
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (ain[0] >= st->info->num_inputs ||
> +		    ain[1] >= st->info->num_inputs) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Input pin number out of range for pair (%d %d).\n",
> +					     ain[0], ain[1]);
> +		}
> +
> +		ret = fwnode_property_match_property_string(child,
> +							    "adi,reference-select",
> +							    ad7173_ref_sel_str,
> +							    ARRAY_SIZE(ad7173_ref_sel_str));
> +
> +		if (ret < 0)
> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		else
> +			ref_sel = ret;
Simpler pattern for properties with a default is not to check the error code.

		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;

		fwnode_property_match_property_String(child, ...

so only if it succeeds is the value overridden.

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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-12 15:09 ` [PATCH v8 1/2] dt-bindings: adc: add AD7173 David Lechner
@ 2023-12-14 12:43   ` Ceclan Dumitru
  2023-12-14 16:12     ` David Lechner
  0 siblings, 1 reply; 17+ messages in thread
From: Ceclan Dumitru @ 2023-12-14 12:43 UTC (permalink / raw)
  To: David Lechner
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel



On 12/12/23 17:09, David Lechner wrote:
> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
>>
>> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
>> which can be used in high precision, low noise single channel applications
>> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
>> primarily for measurement of signals close to DC but also delivers
>> outstanding performance with input bandwidths out to ~10kHz.
> 
> As stated in [1], we should try to make complete bindings. I think
> more could be done here to make this more complete. Most notably, the
> gpio-controller binding is missing. Also maybe something is needed to
> describe how the SYNC/ERROR pin is wired up since it can be an input
> or an output with different functions?
> 

GPIO-controller:
  '#gpio-cells':

    const: 2


  gpio-controller: true
Like this, in properties?

Sync can only be an output, Error is configurable. Are there any
examples for how something like this is described?

...

>> +  interrupts:
>> +    maxItems: 1
> 
> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> pin. Although I could see how RDY could be considered part of the SPI
> bus. In any case, a description explaining what the interrupt is would
> be useful.
> 

I do not see how there could be 2 interrupts. DOUT/RDY is used as an
interrupt when waiting for a conversion to finalize.

Sync and Error are sepparate pins, Sync(if enabled) works only as an
input that resets the modulator and the digital filter.

Error can be configured as input, output or ERROR output (OR between all
internal error sources).

Would this be alright
  interrupts:

    description: Conversion completion interrupt.
		 Pin is shared with SPI DOUT.
    maxItems: 1

...

>> +
>> +patternProperties:
>> +  "^channel@[0-9a-f]$":
>> +    type: object
>> +    $ref: adc.yaml
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      reg:
>> +        minimum: 0
>> +        maximum: 15
>> +
>> +      diff-channels:
>> +        items:
>> +          minimum: 0
>> +          maximum: 31
> 
> Do we need to add overrides to limit the maximums for each compatible string?
> 

Just to be sure, in the allOf section?
If yes, is there any other more elegant method to obtain this behavior?

...

>> +
>> +    required:
>> +      - reg
>> +      - diff-channels
> 
> Individual analog inputs can be used as single-ended or in pairs as
> differential, right? If so, diff-channels should not be required to
> allow for single-ended use.
> 
> And we would need to add something like a single-ended-channel
> property to adc.yaml to allow mapping analog input pins to channels
> similar to how diff-channels works, I think (I don't see anything like
> that there already)?
> 
> So maybe something like:
> 
> oneOf:
>   - required:
>       single-ended-channel
>   - required:
>       diff-channels
> 
All channels must specify 2 analog input sources, there is no input
source wired by default to AVSS.

In my opinion, there is no need to specify channels as single-ended
because that would require a property that specifies the input that is
wired to AVSS.

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

* Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-14 12:30   ` Jonathan Cameron
@ 2023-12-14 12:57     ` Ceclan Dumitru
  2023-12-14 14:47       ` Andy Shevchenko
  0 siblings, 1 reply; 17+ messages in thread
From: Ceclan Dumitru @ 2023-12-14 12:57 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel



On 12/14/23 14:30, Jonathan Cameron wrote:
> On Tue, 12 Dec 2023 12:44:36 +0200
> Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
> 
>> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
>> which can be used in high precision, low noise single channel
>> applications or higher speed multiplexed applications. The Sigma-Delta
>> ADC is intended primarily for measurement of signals close to DC but also
>> delivers outstanding performance with input bandwidths out to ~10kHz.
>>
>> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
>> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> Hi
> 
> Given it seems like you'll be doing a v9, one quick comment from me below.
> 
> Jonathan
> 
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> new file mode 100644
>> index 000000000000..96918b24a10a
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -0,0 +1,964 @@
> ...
> 
>> +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>> +{
> 
> ...
> 
>> +
>> +	if (st->info->has_temp) {
>> +		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
>> +		chan_st_priv = &channels_st_priv_arr[chan_index];
>> +		chan_st_priv->ain =
>> +			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
>> +		chan_st_priv->cfg.bipolar = false;
>> +		chan_st_priv->cfg.input_buf = true;
>> +		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
>> +		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
>> +
>> +		chan_index++;
>> +	}
>> +
>> +	device_for_each_child_node(dev, child) {
>> +		chan = &chan_arr[chan_index];
>> +		chan_st_priv = &channels_st_priv_arr[chan_index];
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     ain, ARRAY_SIZE(ain));
>> +		if (ret) {
>> +			fwnode_handle_put(child);
>> +			return ret;
>> +		}
>> +
>> +		if (ain[0] >= st->info->num_inputs ||
>> +		    ain[1] >= st->info->num_inputs) {
>> +			fwnode_handle_put(child);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "Input pin number out of range for pair (%d %d).\n",
>> +					     ain[0], ain[1]);
>> +		}
>> +
>> +		ret = fwnode_property_match_property_string(child,
>> +							    "adi,reference-select",
>> +							    ad7173_ref_sel_str,
>> +							    ARRAY_SIZE(ad7173_ref_sel_str));
>> +
>> +		if (ret < 0)
>> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
>> +		else
>> +			ref_sel = ret;
> Simpler pattern for properties with a default is not to check the error code.
> 
> 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> 
> 		fwnode_property_match_property_String(child, ...
> 
> so only if it succeeds is the value overridden.

Where exactly would the value be overridden, the function does not have
an argument passed for the found index. The function is written to
return either the found index or a negative error.

The proposed pattern would just ignore the returned index and would
always leave ref_sel to default. Am I missing something?

I can see in the thread where it was introduced that you proposed:
"Looking at the usecases I wonder if it would be better to pass in

an unsigned int *ret which is only updated on a match?"

But on the iio togreg branch that was suggested I could the function on,
it does not have that parameter.

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

* Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-14 12:57     ` Ceclan Dumitru
@ 2023-12-14 14:47       ` Andy Shevchenko
  2023-12-17 13:31         ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2023-12-14 14:47 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Jonathan Cameron, linus.walleij, brgl, linux-gpio,
	Lars-Peter Clausen, Jonathan Cameron, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Michael Walle, Arnd Bergmann,
	ChiaEn Wu, Niklas Schnelle, Leonard Göhrs, Mike Looijmans,
	Haibo Chen, Hugo Villeneuve, Ceclan Dumitru, linux-iio,
	devicetree, linux-kernel

On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote:
> On 12/14/23 14:30, Jonathan Cameron wrote:
> > On Tue, 12 Dec 2023 12:44:36 +0200
> > Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

...

> >> +		ret = fwnode_property_match_property_string(child,
> >> +							    "adi,reference-select",
> >> +							    ad7173_ref_sel_str,
> >> +							    ARRAY_SIZE(ad7173_ref_sel_str));

> >> +

Redundant blank line.

> >> +		if (ret < 0)
> >> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> >> +		else
> >> +			ref_sel = ret;
> > Simpler pattern for properties with a default is not to check the error code.
> > 
> > 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> > 
> > 		fwnode_property_match_property_String(child, ...
> > 
> > so only if it succeeds is the value overridden.
> 
> Where exactly would the value be overridden, the function does not have an
> argument passed for the found index. The function is written to return either
> the found index or a negative error.
> 
> The proposed pattern would just ignore the returned index and would always
> leave ref_sel to default. Am I missing something?
> 
> I can see in the thread where it was introduced that you proposed:
> "Looking at the usecases I wonder if it would be better to pass in
> an unsigned int *ret which is only updated on a match?"
> 
> But on the iio togreg branch that was suggested I could the function on, it
> does not have that parameter.

Yeah, with the current API we can have one check (no 'else' branch):

		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
		ret = ...
		if (ret >= 0)
			ref_sel = ret;

But your approach is good to me.

...

It's always possible to change prototype, and now of course is the best time
as all the users are provided in the single tree. That said, patches are
welcome if this is what we want. (My proposal was to return index in case of
no error, but at the same time leave it in the returned code, so it will be
aligned with other match functions of fwnode.

But this in either way will complicate the implementation. And I don't find
critical to have if-else in each caller as some of them may do something
different on the error case, when option is mandatory. In such cases we
usually don't provide output if we know that an error condition occurs.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-14 12:43   ` Ceclan Dumitru
@ 2023-12-14 16:12     ` David Lechner
  2023-12-14 17:03       ` Ceclan Dumitru
  0 siblings, 1 reply; 17+ messages in thread
From: David Lechner @ 2023-12-14 16:12 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel

On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:
>
>
>
> On 12/12/23 17:09, David Lechner wrote:
> > On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
> >>
> >> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> >> which can be used in high precision, low noise single channel applications
> >> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> >> primarily for measurement of signals close to DC but also delivers
> >> outstanding performance with input bandwidths out to ~10kHz.
> >
> > As stated in [1], we should try to make complete bindings. I think
> > more could be done here to make this more complete. Most notably, the
> > gpio-controller binding is missing. Also maybe something is needed to
> > describe how the SYNC/ERROR pin is wired up since it can be an input
> > or an output with different functions?
> >
>
> GPIO-controller:
>   '#gpio-cells':
>
>     const: 2
>
>
>   gpio-controller: true
> Like this, in properties?

Yes (with not so many blank lines).

>
> Sync can only be an output, Error is configurable. Are there any
> examples for how something like this is described?
>

Configurable pins sounds like a pinmux. Sounds a bit overkill to
specify everything for a pin-controller for one pin if no one is ever
going to use it. But I will leave it to the DT maintainers to say how
complete the bindings should be.

> ...
>
> >> +  interrupts:
> >> +    maxItems: 1
> >
> > Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> > signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> > pin. Although I could see how RDY could be considered part of the SPI
> > bus. In any case, a description explaining what the interrupt is would
> > be useful.
> >
>
> I do not see how there could be 2 interrupts. DOUT/RDY is used as an
> interrupt when waiting for a conversion to finalize.
>
> Sync and Error are sepparate pins, Sync(if enabled) works only as an
> input that resets the modulator and the digital filter.

I only looked at the AD7172-2 datasheet and pin 15 is labeled
SYNC/ERROR. Maybe they are separate pins on other chips?

>
> Error can be configured as input, output or ERROR output (OR between all
> internal error sources).
>
> Would this be alright
>   interrupts:
>
>     description: Conversion completion interrupt.
>                  Pin is shared with SPI DOUT.
>     maxItems: 1

Since ERROR is an output, I would expect it to be an interrupt. The
RDY output, on the other hand, would be wired to a SPI controller with
the SPI_READY feature (I use the Linux flag name here because I'm not
aware of a corresponding devicetree flag). So I don't think the RDY
signal would be an interrupt.

>
> ...
>
> >> +
> >> +patternProperties:
> >> +  "^channel@[0-9a-f]$":
> >> +    type: object
> >> +    $ref: adc.yaml
> >> +    unevaluatedProperties: false
> >> +
> >> +    properties:
> >> +      reg:
> >> +        minimum: 0
> >> +        maximum: 15
> >> +
> >> +      diff-channels:
> >> +        items:
> >> +          minimum: 0
> >> +          maximum: 31
> >
> > Do we need to add overrides to limit the maximums for each compatible string?
> >
>
> Just to be sure, in the allOf section?
> If yes, is there any other more elegant method to obtain this behavior?

I'm not sure. I would like to know if there is a more elegant way as well. ;-)

>
> ...
>
> >> +
> >> +    required:
> >> +      - reg
> >> +      - diff-channels
> >
> > Individual analog inputs can be used as single-ended or in pairs as
> > differential, right? If so, diff-channels should not be required to
> > allow for single-ended use.
> >
> > And we would need to add something like a single-ended-channel
> > property to adc.yaml to allow mapping analog input pins to channels
> > similar to how diff-channels works, I think (I don't see anything like
> > that there already)?
> >
> > So maybe something like:
> >
> > oneOf:
> >   - required:
> >       single-ended-channel
> >   - required:
> >       diff-channels
> >
> All channels must specify 2 analog input sources, there is no input
> source wired by default to AVSS.
>
> In my opinion, there is no need to specify channels as single-ended
> because that would require a property that specifies the input that is
> wired to AVSS.

Makes sense to me.

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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-14 16:12     ` David Lechner
@ 2023-12-14 17:03       ` Ceclan Dumitru
  2023-12-17 13:50         ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: Ceclan Dumitru @ 2023-12-14 17:03 UTC (permalink / raw)
  To: David Lechner
  Cc: linus.walleij, brgl, andy, linux-gpio, Lars-Peter Clausen,
	Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Michael Walle, Andy Shevchenko, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel



On 12/14/23 18:12, David Lechner wrote:
> On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:
>> On 12/12/23 17:09, David Lechner wrote:
>>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

>> ...
>>
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>
>>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
>>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
>>> pin. Although I could see how RDY could be considered part of the SPI
>>> bus. In any case, a description explaining what the interrupt is would
>>> be useful.
>>>
>>
>> I do not see how there could be 2 interrupts. DOUT/RDY is used as an
>> interrupt when waiting for a conversion to finalize.
>>
>> Sync and Error are sepparate pins, Sync(if enabled) works only as an
>> input that resets the modulator and the digital filter.
> 
> I only looked at the AD7172-2 datasheet and pin 15 is labeled
> SYNC/ERROR. Maybe they are separate pins on other chips?

Yep, sorry, missed that. All other supported models have them separate.

> 
>>
>> Error can be configured as input, output or ERROR output (OR between all
>> internal error sources).
>>
>> Would this be alright
>>   interrupts:
>>
>>     description: Conversion completion interrupt.
>>                  Pin is shared with SPI DOUT.
>>     maxItems: 1
> 
> Since ERROR is an output, I would expect it to be an interrupt. The
> RDY output, on the other hand, would be wired to a SPI controller with
> the SPI_READY feature (I use the Linux flag name here because I'm not
> aware of a corresponding devicetree flag). So I don't think the RDY
> signal would be an interrupt.
> 

Error does not have the purpose to be an interrupt. The only interrupt
used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired
to the SPI controller, but when you can't also receive interrupts on
that very same CPU pin an issue arises. So that pin is also wired to
another GPIO with interrupt support.

This is the same way that ad4130.yaml is written for example (with the
exception that ad4130 supports configuring where the interrupt is routed).

In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the
ad_sigma_delta framework (if it can be called that) is written to expect
a pin interrupt, not to use SPI_READY controller feature.


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

* Re: [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver
  2023-12-14 14:47       ` Andy Shevchenko
@ 2023-12-17 13:31         ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2023-12-17 13:31 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Ceclan Dumitru, Jonathan Cameron, linus.walleij, brgl,
	linux-gpio, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Michael Walle, Arnd Bergmann, ChiaEn Wu,
	Niklas Schnelle, Leonard Göhrs, Mike Looijmans, Haibo Chen,
	Hugo Villeneuve, Ceclan Dumitru, linux-iio, devicetree,
	linux-kernel

On Thu, 14 Dec 2023 16:47:29 +0200
Andy Shevchenko <andy@kernel.org> wrote:

> On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote:
> > On 12/14/23 14:30, Jonathan Cameron wrote:  
> > > On Tue, 12 Dec 2023 12:44:36 +0200
> > > Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:  
> 
> ...
> 
> > >> +		ret = fwnode_property_match_property_string(child,
> > >> +							    "adi,reference-select",
> > >> +							    ad7173_ref_sel_str,
> > >> +							    ARRAY_SIZE(ad7173_ref_sel_str));  
> 
> > >> +  
> 
> Redundant blank line.
> 
> > >> +		if (ret < 0)
> > >> +			ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> > >> +		else
> > >> +			ref_sel = ret;  
> > > Simpler pattern for properties with a default is not to check the error code.
> > > 
> > > 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> > > 
> > > 		fwnode_property_match_property_String(child, ...
> > > 
> > > so only if it succeeds is the value overridden.  
> > 
> > Where exactly would the value be overridden, the function does not have an
> > argument passed for the found index. The function is written to return either
> > the found index or a negative error.
> > 
> > The proposed pattern would just ignore the returned index and would always
> > leave ref_sel to default. Am I missing something?
> > 
> > I can see in the thread where it was introduced that you proposed:
> > "Looking at the usecases I wonder if it would be better to pass in
> > an unsigned int *ret which is only updated on a match?"
> > 
> > But on the iio togreg branch that was suggested I could the function on, it
> > does not have that parameter.  
> 
> Yeah, with the current API we can have one check (no 'else' branch):
> 
> 		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> 		ret = ...
> 		if (ret >= 0)
> 			ref_sel = ret;
> 
Yeah. I was clearly lacking in coffee or just being an idiot that day!

> But your approach is good to me.
> 
> ...
> 
> It's always possible to change prototype, and now of course is the best time
> as all the users are provided in the single tree. That said, patches are
> welcome if this is what we want. (My proposal was to return index in case of
> no error, but at the same time leave it in the returned code, so it will be
> aligned with other match functions of fwnode.
> 
> But this in either way will complicate the implementation. And I don't find
> critical to have if-else in each caller as some of them may do something
> different on the error case, when option is mandatory. In such cases we
> usually don't provide output if we know that an error condition occurs.

I'm fine with it being as it is.  Was just having a slow brain day.

J
> 


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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-14 17:03       ` Ceclan Dumitru
@ 2023-12-17 13:50         ` Jonathan Cameron
  2023-12-18  1:00           ` David Lechner
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2023-12-17 13:50 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: David Lechner, linus.walleij, brgl, andy, linux-gpio,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Michael Walle, Andy Shevchenko, Arnd Bergmann,
	ChiaEn Wu, Niklas Schnelle, Leonard Göhrs, Mike Looijmans,
	Haibo Chen, Hugo Villeneuve, Ceclan Dumitru, linux-iio,
	devicetree, linux-kernel, Mark Brown

On Thu, 14 Dec 2023 19:03:28 +0200
Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:

> On 12/14/23 18:12, David Lechner wrote:
> > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:  
> >> On 12/12/23 17:09, David Lechner wrote:  
> >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:  
> 
> >> ...
> >>  
> >>>> +  interrupts:
> >>>> +    maxItems: 1  
> >>>
> >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> >>> pin. Although I could see how RDY could be considered part of the SPI
> >>> bus. In any case, a description explaining what the interrupt is would
> >>> be useful.
> >>>  
> >>
> >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an
> >> interrupt when waiting for a conversion to finalize.
> >>
> >> Sync and Error are sepparate pins, Sync(if enabled) works only as an
> >> input that resets the modulator and the digital filter.  
> > 
> > I only looked at the AD7172-2 datasheet and pin 15 is labeled
> > SYNC/ERROR. Maybe they are separate pins on other chips?  
> 
> Yep, sorry, missed that. All other supported models have them separate.

 
> >   
> >>
> >> Error can be configured as input, output or ERROR output (OR between all
> >> internal error sources).
> >>
> >> Would this be alright
> >>   interrupts:
> >>
> >>     description: Conversion completion interrupt.
> >>                  Pin is shared with SPI DOUT.
> >>     maxItems: 1  
> > 
> > Since ERROR is an output, I would expect it to be an interrupt. The
> > RDY output, on the other hand, would be wired to a SPI controller with
> > the SPI_READY feature (I use the Linux flag name here because I'm not
> > aware of a corresponding devicetree flag). So I don't think the RDY
> > signal would be an interrupt.
> >   
> 
> Error does not have the purpose to be an interrupt. The only interrupt
> used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired
> to the SPI controller, but when you can't also receive interrupts on
> that very same CPU pin an issue arises. So that pin is also wired to
> another GPIO with interrupt support.

You've lost me.  It's a pin that has a state change when an error condition
occurs.  Why not an interrupt?  Doesn't matter that the driver doesn't
use this functionality. dt-bindings should be as comprehensive as possible.
Given it's a multipurpose pin you'd also want to support it as a gpio to be
complete alongside the other GPIOs.

> 
> This is the same way that ad4130.yaml is written for example (with the
> exception that ad4130 supports configuring where the interrupt is routed).
> 
> In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the
> ad_sigma_delta framework (if it can be called that) is written to expect
> a pin interrupt, not to use SPI_READY controller feature.

SPI_READY is supported by only a couple of controllers. I'm not even that
sure exactly how it is defined and whether that lines up with this usecase.
From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/

Flow control: Ready Sequence
Master CS   |-----1\_______________________|
Slave  FC   |--------2\____________________|
DATA        |-----------3\_________________|

So you set master and then wait for a flow control pin (the ready signal) before
you can actually talk to the device.

Here we are indicating data is ready to be be read out.

So I don't 'think' SPI_READY applies.

Jonathan


> 


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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-17 13:50         ` Jonathan Cameron
@ 2023-12-18  1:00           ` David Lechner
  2023-12-20 12:38             ` Jonathan Cameron
  0 siblings, 1 reply; 17+ messages in thread
From: David Lechner @ 2023-12-18  1:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Ceclan Dumitru, linus.walleij, brgl, andy, linux-gpio,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Michael Walle, Andy Shevchenko, Arnd Bergmann,
	ChiaEn Wu, Niklas Schnelle, Leonard Göhrs, Mike Looijmans,
	Haibo Chen, Hugo Villeneuve, Ceclan Dumitru, linux-iio,
	devicetree, linux-kernel, Mark Brown

On Sun, Dec 17, 2023 at 7:50 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 14 Dec 2023 19:03:28 +0200
> Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:
>
> > On 12/14/23 18:12, David Lechner wrote:
> > > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:
> > >> On 12/12/23 17:09, David Lechner wrote:
> > >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
> >
> > >> ...
> > >>
> > >>>> +  interrupts:
> > >>>> +    maxItems: 1
> > >>>
> > >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> > >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> > >>> pin. Although I could see how RDY could be considered part of the SPI
> > >>> bus. In any case, a description explaining what the interrupt is would
> > >>> be useful.
> > >>>
> > >>
> > >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an
> > >> interrupt when waiting for a conversion to finalize.
> > >>
> > >> Sync and Error are sepparate pins, Sync(if enabled) works only as an
> > >> input that resets the modulator and the digital filter.
> > >
> > > I only looked at the AD7172-2 datasheet and pin 15 is labeled
> > > SYNC/ERROR. Maybe they are separate pins on other chips?
> >
> > Yep, sorry, missed that. All other supported models have them separate.
>
>
> > >
> > >>
> > >> Error can be configured as input, output or ERROR output (OR between all
> > >> internal error sources).
> > >>
> > >> Would this be alright
> > >>   interrupts:
> > >>
> > >>     description: Conversion completion interrupt.
> > >>                  Pin is shared with SPI DOUT.
> > >>     maxItems: 1
> > >
> > > Since ERROR is an output, I would expect it to be an interrupt. The
> > > RDY output, on the other hand, would be wired to a SPI controller with
> > > the SPI_READY feature (I use the Linux flag name here because I'm not
> > > aware of a corresponding devicetree flag). So I don't think the RDY
> > > signal would be an interrupt.
> > >
> >
> > Error does not have the purpose to be an interrupt. The only interrupt
> > used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired
> > to the SPI controller, but when you can't also receive interrupts on
> > that very same CPU pin an issue arises. So that pin is also wired to
> > another GPIO with interrupt support.
>
> You've lost me.  It's a pin that has a state change when an error condition
> occurs.  Why not an interrupt?  Doesn't matter that the driver doesn't
> use this functionality. dt-bindings should be as comprehensive as possible.
> Given it's a multipurpose pin you'd also want to support it as a gpio to be
> complete alongside the other GPIOs.
>
> >
> > This is the same way that ad4130.yaml is written for example (with the
> > exception that ad4130 supports configuring where the interrupt is routed).
> >
> > In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the
> > ad_sigma_delta framework (if it can be called that) is written to expect
> > a pin interrupt, not to use SPI_READY controller feature.
>
> SPI_READY is supported by only a couple of controllers. I'm not even that
> sure exactly how it is defined and whether that lines up with this usecase.
> From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/
>
> Flow control: Ready Sequence
> Master CS   |-----1\_______________________|
> Slave  FC   |--------2\____________________|
> DATA        |-----------3\_________________|
>
> So you set master and then wait for a flow control pin (the ready signal) before
> you can actually talk to the device.
>
> Here we are indicating data is ready to be be read out.
>
> So I don't 'think' SPI_READY applies.
>
> Jonathan
>

I'm not arguing that SPI_READY applies in this particular case, but
FWIW it does seem to me like...

1) SPI_READY could be implemented in any SPI driver using a GPIO
interrupt (similar to how we already have GPIO CS)
2) In cases where the SPI controller does have actual hardware support
for SPI_READY and the ADC chip A) uses CS to trigger a conversion and
B) has a "busy" signal that goes low when the conversion is complete,
then the SPI_READY feature could be used to make reading sample data
more efficient by avoiding any CPU intervention between CS assertion
and starting the data xfer due to waiting for the conversion to
complete either by waiting for an interrupt or sleeping for a fixed
amount of time.

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

* Re: [PATCH v8 1/2] dt-bindings: adc: add AD7173
  2023-12-18  1:00           ` David Lechner
@ 2023-12-20 12:38             ` Jonathan Cameron
  0 siblings, 0 replies; 17+ messages in thread
From: Jonathan Cameron @ 2023-12-20 12:38 UTC (permalink / raw)
  To: David Lechner
  Cc: Ceclan Dumitru, linus.walleij, brgl, andy, linux-gpio,
	Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Michael Walle, Andy Shevchenko, Arnd Bergmann,
	ChiaEn Wu, Niklas Schnelle, Leonard Göhrs, Mike Looijmans,
	Haibo Chen, Hugo Villeneuve, Ceclan Dumitru, linux-iio,
	devicetree, linux-kernel, Mark Brown

On Sun, 17 Dec 2023 19:00:32 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Sun, Dec 17, 2023 at 7:50 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 14 Dec 2023 19:03:28 +0200
> > Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:
> >  
> > > On 12/14/23 18:12, David Lechner wrote:  
> > > > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:  
> > > >> On 12/12/23 17:09, David Lechner wrote:  
> > > >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:  
> > >  
> > > >> ...
> > > >>  
> > > >>>> +  interrupts:
> > > >>>> +    maxItems: 1  
> > > >>>
> > > >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready"
> > > >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR
> > > >>> pin. Although I could see how RDY could be considered part of the SPI
> > > >>> bus. In any case, a description explaining what the interrupt is would
> > > >>> be useful.
> > > >>>  
> > > >>
> > > >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an
> > > >> interrupt when waiting for a conversion to finalize.
> > > >>
> > > >> Sync and Error are sepparate pins, Sync(if enabled) works only as an
> > > >> input that resets the modulator and the digital filter.  
> > > >
> > > > I only looked at the AD7172-2 datasheet and pin 15 is labeled
> > > > SYNC/ERROR. Maybe they are separate pins on other chips?  
> > >
> > > Yep, sorry, missed that. All other supported models have them separate.  
> >
> >  
> > > >  
> > > >>
> > > >> Error can be configured as input, output or ERROR output (OR between all
> > > >> internal error sources).
> > > >>
> > > >> Would this be alright
> > > >>   interrupts:
> > > >>
> > > >>     description: Conversion completion interrupt.
> > > >>                  Pin is shared with SPI DOUT.
> > > >>     maxItems: 1  
> > > >
> > > > Since ERROR is an output, I would expect it to be an interrupt. The
> > > > RDY output, on the other hand, would be wired to a SPI controller with
> > > > the SPI_READY feature (I use the Linux flag name here because I'm not
> > > > aware of a corresponding devicetree flag). So I don't think the RDY
> > > > signal would be an interrupt.
> > > >  
> > >
> > > Error does not have the purpose to be an interrupt. The only interrupt
> > > used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired
> > > to the SPI controller, but when you can't also receive interrupts on
> > > that very same CPU pin an issue arises. So that pin is also wired to
> > > another GPIO with interrupt support.  
> >
> > You've lost me.  It's a pin that has a state change when an error condition
> > occurs.  Why not an interrupt?  Doesn't matter that the driver doesn't
> > use this functionality. dt-bindings should be as comprehensive as possible.
> > Given it's a multipurpose pin you'd also want to support it as a gpio to be
> > complete alongside the other GPIOs.
> >  
> > >
> > > This is the same way that ad4130.yaml is written for example (with the
> > > exception that ad4130 supports configuring where the interrupt is routed).
> > >
> > > In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the
> > > ad_sigma_delta framework (if it can be called that) is written to expect
> > > a pin interrupt, not to use SPI_READY controller feature.  
> >
> > SPI_READY is supported by only a couple of controllers. I'm not even that
> > sure exactly how it is defined and whether that lines up with this usecase.
> > From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/
> >
> > Flow control: Ready Sequence
> > Master CS   |-----1\_______________________|
> > Slave  FC   |--------2\____________________|
> > DATA        |-----------3\_________________|
> >
> > So you set master and then wait for a flow control pin (the ready signal) before
> > you can actually talk to the device.
> >
> > Here we are indicating data is ready to be be read out.
> >
> > So I don't 'think' SPI_READY applies.
> >
> > Jonathan
> >  
> 
> I'm not arguing that SPI_READY applies in this particular case, but
> FWIW it does seem to me like...
> 
> 1) SPI_READY could be implemented in any SPI driver using a GPIO
> interrupt (similar to how we already have GPIO CS)
> 2) In cases where the SPI controller does have actual hardware support
> for SPI_READY and the ADC chip A) uses CS to trigger a conversion and
> B) has a "busy" signal that goes low when the conversion is complete,
> then the SPI_READY feature could be used to make reading sample data
> more efficient by avoiding any CPU intervention between CS assertion
> and starting the data xfer due to waiting for the conversion to
> complete either by waiting for an interrupt or sleeping for a fixed
> amount of time.

Agreed that SPI_READY is possible if an SPI controller uses GPIOs for
CS and that signal.  If not a GPIO for CS then the SPI_READY should
also be hardware managed.

I could potentially be adapted to this sort of case if conditions
like the CS being active before the ready is set is taking into account.

This is a bit like SPI in general - far too many things that could
be built and no particular standards for them.

Jonathan



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

end of thread, other threads:[~2023-12-20 12:38 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-12 10:44 [PATCH v8 1/2] dt-bindings: adc: add AD7173 Dumitru Ceclan
2023-12-12 10:44 ` Dumitru Ceclan
2023-12-12 10:44 ` [PATCH v8 2/2] iio: adc: ad7173: add AD7173 driver Dumitru Ceclan
2023-12-12 10:44   ` Dumitru Ceclan
2023-12-13 14:24   ` Andy Shevchenko
2023-12-14 11:02     ` Ceclan Dumitru
2023-12-14 12:30   ` Jonathan Cameron
2023-12-14 12:57     ` Ceclan Dumitru
2023-12-14 14:47       ` Andy Shevchenko
2023-12-17 13:31         ` Jonathan Cameron
2023-12-12 15:09 ` [PATCH v8 1/2] dt-bindings: adc: add AD7173 David Lechner
2023-12-14 12:43   ` Ceclan Dumitru
2023-12-14 16:12     ` David Lechner
2023-12-14 17:03       ` Ceclan Dumitru
2023-12-17 13:50         ` Jonathan Cameron
2023-12-18  1:00           ` David Lechner
2023-12-20 12:38             ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.