linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130
@ 2022-04-13  9:40 Cosmin Tanislav
  2022-04-13  9:40 ` [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available} Cosmin Tanislav
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-13  9:40 UTC (permalink / raw)
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	linux-gpio, linux-kernel, devicetree, Cosmin Tanislav

AD4130-8 is an ultra-low power, high precision,
measurement solution for low bandwidth battery
operated applications.

The fully integrated AFE (Analog Front-End)
includes a multiplexer for up to 16 single-ended
or 8 differential inputs, PGA (Programmable Gain
Amplifier), 24-bit Sigma-Delta ADC, on-chip
reference and oscillator, selectable filter
options, smart sequencer, sensor biasing and
excitation options, diagnostics, and a FIFO
buffer.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
new file mode 100644
index 000000000000..e9dce54e9802
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
@@ -0,0 +1,255 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4130 ADC device driver
+
+maintainers:
+  - Cosmin Tanislav <cosmin.tanislav@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4130-8-16-lfcsp
+      - adi,ad4130-8-16-wlcsp
+      - adi,ad4130-8-24-lfcsp
+      - adi,ad4130-8-24-wlcsp
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: phandle to the master clock (mclk)
+
+  clock-names:
+    items:
+      - const: mclk
+
+  interrupts:
+    minItems: 1
+    maxItems: 1
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 1
+    description:
+      Default if not supplied is dout-int.
+    items:
+      enum:
+        - dout-int
+        - clk
+        - p1
+        - dout
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  refin1-supply:
+    description: refin1 supply. Can be used as reference for conversion.
+
+  refin2-supply:
+    description: refin2 supply. Can be used as reference for conversion.
+
+  avdd-supply:
+    description: AVDD voltage supply. Can be used as reference for conversion.
+
+  iovdd-supply:
+    description: IOVDD voltage supply. Used for the chip interface.
+
+  spi-max-frequency:
+    maximum: 5000000
+
+  adi,mclk-sel:
+    description: |
+      Select the clock.
+      0: Internal 76.8kHz clock.
+      1: Internal 76.8kHz clock, output to the CLK pin.
+      2: External 76.8kHz clock.
+      3. External 153.6kHz clock.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+    default: 0
+
+  adi,int-ref-en:
+    description: |
+      Specify if internal reference should be enabled.
+    type: boolean
+    default: true
+
+  adi,bipolar:
+    description: Specify if the device should be used in bipolar mode.
+    type: boolean
+    default: false
+
+  adi,vbias-pins:
+    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
+    items:
+      minimum: 0
+      maximum: 15
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":
+    type: object
+    $ref: adc.yaml
+
+    properties:
+      reg:
+        description: |
+          The channel number.
+        items:
+          minimum: 0
+          maximum: 15
+
+      diff-channels:
+        description: |
+          Besides the analog inputs available, internal inputs can be used.
+          16: Internal temperature sensor.
+          17: AVss
+          18: Internal reference.
+          19: DGND.
+          20: (AVDD − AVSS)/6+
+          21: (AVDD − AVSS)/6-
+          22: (IOVDD − DGND)/6+
+          23: (IOVDD − DGND)/6-
+          24: (ALDO − AVSS)/6+
+          25: (ALDO − AVSS)/6-
+          26: (DLDO − DGND)/6+
+          27: (DLDO − DGND)/6-
+          28: V_MV_P
+          29: V_MV_M
+        $ref: adc.yaml
+        items:
+          minimum: 0
+          maximum: 29
+
+      adi,reference-select:
+        description: |
+          Select the reference source to use when converting on the
+          specific channel. Valid values are:
+          0: REFIN1(+)/REFIN1(−).
+          1: REFIN2(+)/REFIN2(−).
+          2: REFOUT/AVSS (Internal reference)
+          3: AVDD/AVSS
+          If not specified, internal reference is used.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1, 2, 3]
+        default: 2
+
+      adi,excitation-pin-0:
+        description: |
+          Analog input to apply excitation current to while the channel
+          is active.
+        minimum: 0
+        maximum: 15
+        default: 0
+
+      adi,excitation-pin-1:
+        description: |
+          Analog input to apply excitation current to while this channel
+          is active.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        minimum: 0
+        maximum: 15
+        default: 0
+
+      adi,excitation-current-0-nanoamps:
+        description: |
+          Excitation current in nanoamps to be applied to pin specified in
+          adi,excitation-pin-0 while this channel is active.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
+        default: 0
+
+      adi,excitation-current-1-nanoamps:
+        description: |
+          Excitation current in nanoamps to be applied to pin specified in
+          adi,excitation-pin-1 while this channel is active.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
+        default: 0
+
+      adi,burnout-current-nanoamps:
+        description: |
+          Burnout current in nanoamps to be applied for this channel.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 500, 2000, 4000]
+        default: 0
+
+      adi,buffered-positive:
+        description: Enable buffered mode for positive input.
+        type: boolean
+
+      adi,buffered-negative:
+        description: Enable buffered mode for negative input.
+        type: boolean
+
+    required:
+      - reg
+      - diff-channels
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,ad4130-8-24-wlcsp";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        spi-max-frequency = <5000000>;
+        interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpio>;
+
+        channel@0 {
+          reg = <0>;
+          /* AIN8, AIN9 */
+          diff-channels = <8 9>;
+        };
+
+        channel@1 {
+          reg = <1>;
+          /* AIN10, AIN11 */
+          diff-channels = <10 11>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          /* Temperature Sensor, DGND */
+          diff-channels = <16 19>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          /* Internal reference, DGND */
+          diff-channels = <18 19>;
+        };
+
+        channel@4 {
+          reg = <4>;
+          /* DGND, DGND */
+          diff-channels = <19 19>;
+        };
+      };
+    };
-- 
2.35.1


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

* [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available}
  2022-04-13  9:40 [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Cosmin Tanislav
@ 2022-04-13  9:40 ` Cosmin Tanislav
  2022-04-13 14:51   ` Andy Shevchenko
  2022-04-16 15:02   ` Jonathan Cameron
  2022-04-13  9:40 ` [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver Cosmin Tanislav
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-13  9:40 UTC (permalink / raw)
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	linux-gpio, linux-kernel, devicetree, Cosmin Tanislav

AD4130-8 is an ultra-low power, high precision,
measurement solution for low bandwidth battery
operated applications.

The fully integrated AFE (Analog Front-End)
includes a multiplexer for up to 16 single-ended
or 8 differential inputs, PGA (Programmable Gain
Amplifier), 24-bit Sigma-Delta ADC, on-chip
reference and oscillator, selectable filter
options, smart sequencer, sensor biasing and
excitation options, diagnostics, and a FIFO
buffer.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-ad4130      | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
new file mode 100644
index 000000000000..942150991e75
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
@@ -0,0 +1,36 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Reading returns a list with the possible filter modes.
+		"sinc4"       - Sinc 4. Excellent noise performance. Long 1st
+				conversion time. No natural 50/60Hz rejection.
+		"sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion time.
+		"sinc3"	      - Sinc3. Moderate 1st conversion time. Good noise
+				performance.
+		"sinc3+rej60" - Sinc3 + 60Hz rejection. At a sampling frequency
+				of 50Hz, achieves simultaneous 50Hz and 60Hz
+				rejection.
+		"sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion time.
+				Best used with a sampling frequency of at least
+				216.19Hz.
+		"sinc3+pf1"   - Sinc3 + Post Filter 1.
+				53dB rejection @ 50Hz, 58dB rejection @ 60Hz.
+		"sinc3+pf2"   - Sinc3 + Post Filter 2.
+				70dB rejection @ 50Hz, 70dB rejection @ 60Hz.
+		"sinc3+pf3"   - Sinc3 + Post Filter 3.
+				99dB rejection @ 50Hz, 103dB rejection @ 60Hz.
+		"sinc3+pf4"   - Sinc3 + Post Filter 4.
+				103dB rejection @ 50Hz, 109dB rejection @ 60Hz.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY-voltageZ_filter_mode
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Set the filter mode of the differential channel. When the filter
+		mode changes, the in_voltageY-voltageZ_sampling_frequency and
+		in_voltageY-voltageZ_sampling_frequency_available attributes
+		might also change to accomodate the new filter mode.
+		If the current sampling frequency is out of range for the new
+		filter mode, the sampling frequency will be changed to the
+		closest valid one.
-- 
2.35.1


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

* [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-13  9:40 [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Cosmin Tanislav
  2022-04-13  9:40 ` [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available} Cosmin Tanislav
@ 2022-04-13  9:40 ` Cosmin Tanislav
  2022-04-13 15:41   ` Andy Shevchenko
  2022-04-16 16:21   ` Jonathan Cameron
  2022-04-13 12:26 ` [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Rob Herring
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-13  9:40 UTC (permalink / raw)
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	linux-gpio, linux-kernel, devicetree, Cosmin Tanislav

AD4130-8 is an ultra-low power, high precision,
measurement solution for low bandwidth battery
operated applications.

The fully integrated AFE (Analog Front-End)
includes a multiplexer for up to 16 single-ended
or 8 differential inputs, PGA (Programmable Gain
Amplifier), 24-bit Sigma-Delta ADC, on-chip
reference and oscillator, selectable filter
options, smart sequencer, sensor biasing and
excitation options, diagnostics, and a FIFO
buffer.

Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
---
 MAINTAINERS              |    8 +
 drivers/iio/adc/Kconfig  |   13 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4130.c | 2072 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 2094 insertions(+)
 create mode 100644 drivers/iio/adc/ad4130.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a1dfb3fab5f8..8719a246b55d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1054,6 +1054,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
 F:	drivers/net/amt.c
 
+ANALOG DEVICES INC AD4130 DRIVER
+M:	Cosmin Tanislav <cosmin.tanislav@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
+F:	drivers/iio/adc/ad4130.c
+
 ANALOG DEVICES INC AD7192 DRIVER
 M:	Alexandru Tachici <alexandru.tachici@analog.com>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 71ab0a06aa82..2dac0f029c41 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -21,6 +21,19 @@ config AD_SIGMA_DELTA
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 
+config AD4130
+	tristate "Analog Device AD4130 ADC Driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD4130-8 SPI analog
+	  to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4130.
+
 config AD7091R5
 	tristate "Analog Devices AD7091R5 ADC Driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 39d806f6d457..c3aa7e4eca2f 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 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_AD7192) += ad7192.o
diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
new file mode 100644
index 000000000000..89fb9b413ff0
--- /dev/null
+++ b/drivers/iio/adc/ad4130.c
@@ -0,0 +1,2072 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD4130 SPI ADC driver
+ *
+ * Copyright 2022 Analog Devices Inc.
+ */
+#include <asm/div64.h>
+#include <asm/unaligned.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define AD4130_8_NAME			"ad4130-8"
+
+#define AD4130_COMMS_READ_MASK		BIT(6)
+
+#define AD4130_REG_STATUS		0x00
+#define AD4130_STATUS_POR_FLAG_MASK	BIT(4)
+
+#define AD4130_REG_ADC_CONTROL		0x01
+#define AD4130_BIPOLAR_MASK		BIT(14)
+#define AD4130_INT_REF_VAL_MASK		BIT(13)
+#define AD4130_INT_REF_2_5V		2500000
+#define AD4130_INT_REF_1_25V		1250000
+#define AD4130_CSB_EN_MASK		BIT(9)
+#define AD4130_INT_REF_EN_MASK		BIT(8)
+#define AD4130_MODE_MASK		GENMASK(5, 2)
+#define AD4130_MCLK_SEL_MASK		GENMASK(1, 0)
+
+#define AD4130_REG_DATA			0x02
+
+#define AD4130_REG_IO_CONTROL		0x03
+#define AD4130_INT_PIN_SEL_MASK		GENMASK(9, 8)
+#define AD4130_GPIO_DATA_MASK		GENMASK(7, 4)
+#define AD4130_GPIO_CTRL_MASK		GENMASK(3, 0)
+
+#define AD4130_REG_VBIAS		0x04
+
+#define AD4130_REG_ID			0x05
+
+#define AD4130_REG_ERROR		0x06
+
+#define AD4130_REG_ERROR_EN		0x07
+
+#define AD4130_REG_CHANNEL_X(x)		(0x09 + (x))
+#define AD4130_CHANNEL_EN_MASK		BIT(23)
+#define AD4130_SETUP_MASK		GENMASK(22, 20)
+#define AD4130_AINP_MASK		GENMASK(17, 13)
+#define AD4130_AINM_MASK		GENMASK(12, 8)
+#define AD4130_IOUT1_MASK		GENMASK(7, 4)
+#define AD4130_IOUT2_MASK		GENMASK(3, 0)
+
+#define AD4130_REG_CONFIG_X(x)		(0x19 + (x))
+#define AD4130_IOUT1_VAL_MASK		GENMASK(15, 13)
+#define AD4130_IOUT2_VAL_MASK		GENMASK(12, 10)
+#define AD4130_BURNOUT_MASK		GENMASK(9, 8)
+#define AD4130_REF_BUFP_MASK		BIT(7)
+#define AD4130_REF_BUFM_MASK		BIT(6)
+#define AD4130_REF_SEL_MASK		GENMASK(5, 4)
+#define AD4130_PGA_MASK			GENMASK(3, 1)
+#define AD4130_PGA_NUM			8
+
+#define AD4130_REG_FILTER_X(x)		(0x21 + (x))
+#define AD4130_FILTER_MODE_MASK		GENMASK(15, 12)
+#define AD4130_FILTER_SELECT_MASK	GENMASK(10, 0)
+#define AD4130_FS_MIN			1
+#define AD4130_MAX_ODR			2400
+
+#define AD4130_REG_FIFO_CONTROL		0x3a
+#define AD4130_ADD_FIFO_HEADER_MASK	BIT(18)
+#define AD4130_FIFO_MODE_MASK		GENMASK(17, 16)
+#define AD4130_WATERMARK_INT_EN_MASK	BIT(9)
+#define AD4130_WATERMARK_MASK		GENMASK(7, 0)
+#define AD4130_WATERMARK_256		0
+
+#define AD4130_REG_FIFO_STATUS		0x3b
+
+#define AD4130_REG_FIFO_DATA		0x3d
+#define AD4130_FIFO_SIZE		256
+#define AD4130_FIFO_MAX_SAMPLE_SIZE	3
+
+#define AD4130_MAX_GPIOS		4
+#define AD4130_MAX_SETUPS		8
+#define AD4130_MAX_CHANNELS		16
+#define AD4130_MAX_ANALOG_PINS		16
+#define AD4130_MAX_DIFF_INPUTS		30
+
+#define AD4130_AIN2_P1			0x2
+
+#define AD4130_MCLK_FREQ_76_8KHZ	76800
+#define AD4130_MCLK_FREQ_153_6KHZ	153600
+
+#define AD4130_RESET_CLK_COUNT		64
+#define AD4130_RESET_BUF_SIZE		(AD4130_RESET_CLK_COUNT / 8)
+
+#define AD4130_SOFT_RESET_SLEEP		(160 * 1000000 / AD4130_MCLK_FREQ_76_8KHZ)
+
+#define AD4130_INVALID_SLOT		-1
+
+#define AD4130_FREQ_FACTOR		1000000000ull
+#define AD4130_DB3_FACTOR		1000
+
+static const unsigned int ad4130_reg_size[] = {
+	[AD4130_REG_STATUS] = 1,
+	[AD4130_REG_ADC_CONTROL] = 2,
+	[AD4130_REG_IO_CONTROL] = 2,
+	[AD4130_REG_VBIAS] = 2,
+	[AD4130_REG_ID] = 1,
+	[AD4130_REG_ERROR] = 2,
+	[AD4130_REG_ERROR_EN] = 2,
+	[AD4130_REG_CHANNEL_X(0) ...
+	 AD4130_REG_CHANNEL_X(AD4130_MAX_CHANNELS)] = 3,
+	[AD4130_REG_CONFIG_X(0) ...
+	 AD4130_REG_CONFIG_X(AD4130_MAX_SETUPS)] = 2,
+	[AD4130_REG_FILTER_X(0) ...
+	 AD4130_REG_FILTER_X(AD4130_MAX_SETUPS)] = 3,
+	[AD4130_REG_FIFO_CONTROL] = 3,
+	[AD4130_REG_FIFO_STATUS] = 1,
+};
+
+enum ad4130_id {
+	ID_AD4130_8_24_WLCSP,
+	ID_AD4130_8_24_LFCSP,
+	ID_AD4130_8_16_WLCSP,
+	ID_AD4130_8_16_LFCSP,
+};
+
+enum ad4130_int_ref_val {
+	AD4130_INT_REF_VAL_2_5V,
+	AD4130_INT_REF_VAL_1_25V,
+};
+
+enum ad4130_mclk_sel {
+	AD4130_MCLK_76_8KHZ,
+	AD4130_MCLK_76_8KHZ_OUT,
+	AD4130_MCLK_76_8KHZ_EXT,
+	AD4130_MCLK_153_6KHZ_EXT,
+	AD4130_MCLK_SEL_MAX,
+};
+
+enum ad4130_int_pin_sel {
+	AD4130_INT_PIN_DOUT_OR_INT,
+	AD4130_INT_PIN_CLK,
+	AD4130_INT_PIN_P1,
+	AD4130_INT_PIN_DOUT,
+};
+
+enum ad4130_iout {
+	AD4130_IOUT_OFF,
+	AD4130_IOUT_10000NA,
+	AD4130_IOUT_20000NA,
+	AD4130_IOUT_50000NA,
+	AD4130_IOUT_100000NA,
+	AD4130_IOUT_150000NA,
+	AD4130_IOUT_200000NA,
+	AD4130_IOUT_100NA,
+	AD4130_IOUT_MAX,
+};
+
+enum ad4130_burnout {
+	AD4130_BURNOUT_OFF,
+	AD4130_BURNOUT_500NA,
+	AD4130_BURNOUT_2000NA,
+	AD4130_BURNOUT_4000NA,
+	AD4130_BURNOUT_MAX,
+};
+
+enum ad4130_ref_sel {
+	AD4130_REF_REFIN1,
+	AD4130_REF_REFIN2,
+	AD4130_REF_REFOUT_AVSS,
+	AD4130_REF_AVDD_AVSS,
+	AD4130_REF_SEL_MAX,
+};
+
+enum ad4130_fifo_mode {
+	AD4130_FIFO_MODE_DISABLED = 0b00,
+	AD4130_FIFO_MODE_WATERMARK = 0b01,
+};
+
+enum ad4130_mode {
+	AD4130_MODE_CONTINUOUS = 0b0000,
+	AD4130_MODE_IDLE = 0b0100,
+};
+
+enum ad4130_filter_mode {
+	AD4130_FILTER_SINC4,
+	AD4130_FILTER_SINC4_SINC1,
+	AD4130_FILTER_SINC3,
+	AD4130_FILTER_SINC3_REJ60,
+	AD4130_FILTER_SINC3_SINC1,
+	AD4130_FILTER_SINC3_PF1,
+	AD4130_FILTER_SINC3_PF2,
+	AD4130_FILTER_SINC3_PF3,
+	AD4130_FILTER_SINC3_PF4,
+	AD4130_FILTER_MAX,
+};
+
+enum ad4130_pin_function {
+	AD4130_PIN_FN_NONE,
+	AD4130_PIN_FN_SPECIAL = 1 << 0,
+	AD4130_PIN_FN_DIFF = 1 << 1,
+	AD4130_PIN_FN_EXCITATION = 1 << 2,
+	AD4130_PIN_FN_VBIAS = 1 << 3,
+};
+
+struct ad4130_chip_info {
+	const char	*name;
+	u8		resolution;
+	bool		has_int_pin;
+};
+
+struct ad4130_setup_info {
+	unsigned int			iout0_val;
+	unsigned int			iout1_val;
+	unsigned int			burnout;
+	unsigned int			pga;
+	unsigned int			fs;
+	bool				ref_bufp;
+	bool				ref_bufm;
+	u32				ref_sel;
+	enum ad4130_filter_mode		filter_mode;
+	unsigned int			enabled_channels;
+	unsigned int			channels;
+};
+
+#define AD4130_SETUP_SIZE		offsetof(struct ad4130_setup_info, \
+						 enabled_channels)
+
+struct ad4130_chan_info {
+	u32				iout0;
+	u32				iout1;
+	bool				enabled;
+	struct ad4130_setup_info	setup;
+	int				slot;
+	bool				initialized;
+};
+
+struct ad4130_filter_config {
+	enum ad4130_filter_mode		filter_mode;
+	unsigned int			odr_div;
+	unsigned int			fs_max;
+	unsigned int			db3_div;
+	enum iio_available_type		samp_freq_avail_type;
+	int				samp_freq_avail_len;
+	int				samp_freq_avail[3][2];
+	enum iio_available_type		db3_freq_avail_type;
+	int				db3_freq_avail_len;
+	int				db3_freq_avail[3][2];
+};
+
+struct ad4130_state {
+	const struct ad4130_chip_info	*chip_info;
+	struct spi_device		*spi;
+	struct regmap			*regmap;
+	struct clk			*mclk;
+	struct regulator_bulk_data	regulators[4];
+	u32				irq_trigger;
+	u32				inv_irq_trigger;
+
+	/*
+	 * Synchronize access to members of driver state, and ensure atomicity
+	 * of consecutive regmap operations.
+	 */
+	struct mutex			lock;
+	struct completion		completion;
+
+	struct iio_chan_spec		chans[AD4130_MAX_CHANNELS];
+	struct ad4130_chan_info		chans_info[AD4130_MAX_CHANNELS];
+	struct ad4130_setup_info	setups_info[AD4130_MAX_SETUPS];
+	enum ad4130_pin_function	pins_fn[AD4130_MAX_ANALOG_PINS];
+	u32				vbias_pins[AD4130_MAX_ANALOG_PINS];
+	u32				num_vbias_pins;
+	int				scale_tbls[AD4130_REF_SEL_MAX]
+						  [AD4130_PGA_NUM][2];
+	struct gpio_chip		gc;
+	unsigned int			gpio_offsets[AD4130_MAX_GPIOS];
+	unsigned int			num_gpios;
+
+	u32			int_pin_sel;
+	bool			int_ref_en;
+	u32			int_ref_uv;
+	u32			mclk_sel;
+	bool			bipolar;
+
+	unsigned int		num_enabled_channels;
+	unsigned int		effective_watermark;
+	unsigned int		watermark;
+
+	struct spi_message	fifo_msg;
+	struct spi_transfer	fifo_xfer[2];
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	u8			reset_buf[AD4130_RESET_BUF_SIZE] ____cacheline_aligned;
+	u8			reg_write_tx_buf[4];
+	u8			reg_read_tx_buf[1];
+	u8			reg_read_rx_buf[3];
+	u8			fifo_tx_buf[2];
+	u8			fifo_rx_buf[AD4130_FIFO_SIZE *
+					    AD4130_FIFO_MAX_SAMPLE_SIZE];
+};
+
+static const char * const ad4130_int_pin_names[] = {
+	[AD4130_INT_PIN_DOUT_OR_INT] = "dout-int",
+	[AD4130_INT_PIN_CLK] = "clk",
+	[AD4130_INT_PIN_P1] = "p1",
+	[AD4130_INT_PIN_DOUT] = "dout",
+};
+
+static const unsigned int ad4130_iout_current_na_tbl[AD4130_IOUT_MAX] = {
+	[AD4130_IOUT_OFF] = 0,
+	[AD4130_IOUT_100NA] = 100,
+	[AD4130_IOUT_10000NA] = 10000,
+	[AD4130_IOUT_20000NA] = 20000,
+	[AD4130_IOUT_50000NA] = 50000,
+	[AD4130_IOUT_100000NA] = 100000,
+	[AD4130_IOUT_150000NA] = 150000,
+	[AD4130_IOUT_200000NA] = 200000,
+};
+
+static const unsigned int ad4130_burnout_current_na_tbl[AD4130_BURNOUT_MAX] = {
+	[AD4130_BURNOUT_OFF] = 0,
+	[AD4130_BURNOUT_500NA] = 500,
+	[AD4130_BURNOUT_2000NA] = 2000,
+	[AD4130_BURNOUT_4000NA] = 4000,
+};
+
+#define AD4130_VARIABLE_ODR_CONFIG(_filter_mode, _odr_div, _fs_max, _db3_div)	\
+{										\
+		.filter_mode = (_filter_mode),					\
+		.odr_div = (_odr_div),						\
+		.fs_max = (_fs_max),						\
+		.db3_div = (_db3_div),						\
+		.samp_freq_avail_type = IIO_AVAIL_RANGE,			\
+		.samp_freq_avail_len = 3,					\
+		.samp_freq_avail = {						\
+			{ AD4130_MAX_ODR, (_odr_div) * (_fs_max) },		\
+			{ AD4130_MAX_ODR, (_odr_div) * (_fs_max) },		\
+			{ AD4130_MAX_ODR, (_odr_div) },				\
+		},								\
+		.db3_freq_avail_type = IIO_AVAIL_RANGE,				\
+		.db3_freq_avail_len = 3,					\
+		.db3_freq_avail = {						\
+			{							\
+				AD4130_MAX_ODR * (_db3_div),			\
+				(_odr_div) * (_fs_max) * AD4130_DB3_FACTOR,	\
+			},							\
+			{							\
+				AD4130_MAX_ODR * (_db3_div),			\
+				(_odr_div) * (_fs_max) * AD4130_DB3_FACTOR,	\
+			},							\
+			{							\
+				AD4130_MAX_ODR * (_db3_div),			\
+				(_odr_div) * AD4130_DB3_FACTOR,			\
+			},							\
+		},								\
+}
+
+#define AD4130_FIXED_ODR_CONFIG(_filter_mode, _odr_div, _db3_div)	\
+{									\
+		.filter_mode = (_filter_mode),				\
+		.odr_div = (_odr_div),					\
+		.fs_max = AD4130_FS_MIN,				\
+		.db3_div = (_db3_div),					\
+		.samp_freq_avail_type = IIO_AVAIL_LIST,			\
+		.samp_freq_avail_len = 1,				\
+		.samp_freq_avail = {					\
+			{ AD4130_MAX_ODR, (_odr_div) },			\
+		},							\
+		.db3_freq_avail_type = IIO_AVAIL_LIST,			\
+		.db3_freq_avail_len = 1,				\
+		.db3_freq_avail = {					\
+			{						\
+				AD4130_MAX_ODR * AD4130_DB3_FACTOR,	\
+				(_odr_div) * (_db3_div)			\
+			},						\
+		},							\
+}
+
+static const struct ad4130_filter_config ad4130_filter_configs[] = {
+	AD4130_VARIABLE_ODR_CONFIG(AD4130_FILTER_SINC4,       1,  10,   234),
+	AD4130_VARIABLE_ODR_CONFIG(AD4130_FILTER_SINC4_SINC1, 11, 10,   590),
+	AD4130_VARIABLE_ODR_CONFIG(AD4130_FILTER_SINC3,       1,  2047, 268),
+	AD4130_VARIABLE_ODR_CONFIG(AD4130_FILTER_SINC3_REJ60, 1,  2047, 268),
+	AD4130_VARIABLE_ODR_CONFIG(AD4130_FILTER_SINC3_SINC1, 10, 2047, 545),
+	AD4130_FIXED_ODR_CONFIG(AD4130_FILTER_SINC3_PF1,      92,       675),
+	AD4130_FIXED_ODR_CONFIG(AD4130_FILTER_SINC3_PF2,      100,      675),
+	AD4130_FIXED_ODR_CONFIG(AD4130_FILTER_SINC3_PF3,      124,      675),
+	AD4130_FIXED_ODR_CONFIG(AD4130_FILTER_SINC3_PF4,      148,      675),
+};
+
+static const char * const ad4130_filter_modes_str[] = {
+	[AD4130_FILTER_SINC4] = "sinc4",
+	[AD4130_FILTER_SINC4_SINC1] = "sinc4+sinc1",
+	[AD4130_FILTER_SINC3] = "sinc3",
+	[AD4130_FILTER_SINC3_REJ60] = "sinc3+rej60",
+	[AD4130_FILTER_SINC3_SINC1] = "sinc3+sinc1",
+	[AD4130_FILTER_SINC3_PF1] = "sinc3+pf1",
+	[AD4130_FILTER_SINC3_PF2] = "sinc3+pf2",
+	[AD4130_FILTER_SINC3_PF3] = "sinc3+pf3",
+	[AD4130_FILTER_SINC3_PF4] = "sinc3+pf4",
+};
+
+static unsigned int ad4130_data_reg_size(struct ad4130_state *st)
+{
+	return st->chip_info->resolution / 8;
+}
+
+static int ad4130_get_reg_size(struct ad4130_state *st, unsigned int reg,
+			       unsigned int *size)
+{
+	if (reg >= ARRAY_SIZE(ad4130_reg_size))
+		return -EINVAL;
+
+	if (reg == AD4130_REG_DATA) {
+		*size = ad4130_data_reg_size(st);
+		return 0;
+	}
+
+	*size = ad4130_reg_size[reg];
+
+	if (!*size)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ad4130_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct ad4130_state *st = context;
+	unsigned int size;
+	int ret;
+
+	ret = ad4130_get_reg_size(st, reg, &size);
+	if (ret)
+		return ret;
+
+	st->reg_write_tx_buf[0] = reg;
+
+	switch (size) {
+	case 3:
+		put_unaligned_be24(val, &st->reg_write_tx_buf[1]);
+		break;
+	case 2:
+		put_unaligned_be16(val, &st->reg_write_tx_buf[1]);
+		break;
+	case 1:
+		st->reg_write_tx_buf[1] = val;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return spi_write(st->spi, st->reg_write_tx_buf, size + 1);
+}
+
+static int ad4130_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct ad4130_state *st = context;
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = st->reg_read_tx_buf,
+			.len = sizeof(st->reg_read_tx_buf),
+		},
+		{
+			.rx_buf = st->reg_read_rx_buf,
+		},
+	};
+	unsigned int size;
+	int ret;
+
+	ret = ad4130_get_reg_size(st, reg, &size);
+	if (ret)
+		return ret;
+
+	st->reg_read_tx_buf[0] = AD4130_COMMS_READ_MASK | reg;
+	t[1].len = size;
+
+	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+	if (ret)
+		return ret;
+
+	switch (size) {
+	case 3:
+		*val = get_unaligned_be24(st->reg_read_rx_buf);
+		break;
+	case 2:
+		*val = get_unaligned_be16(st->reg_read_rx_buf);
+		break;
+	case 1:
+		*val = st->reg_read_rx_buf[0];
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return 0;
+}
+
+static const struct regmap_config ad4130_regmap_config = {
+	.reg_read = ad4130_reg_read,
+	.reg_write = ad4130_reg_write,
+};
+
+static int ad4130_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void ad4130_gpio_set(struct gpio_chip *gc, unsigned int offset,
+			    int value)
+{
+	struct ad4130_state *st = gpiochip_get_data(gc);
+	unsigned int real_offset = st->gpio_offsets[offset];
+	unsigned int mask = FIELD_PREP(AD4130_GPIO_DATA_MASK, BIT(real_offset));
+
+	regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
+			   value ? mask : 0);
+}
+
+static int ad4130_set_mode(struct ad4130_state *st, enum ad4130_mode mode)
+{
+	return regmap_update_bits(st->regmap, AD4130_REG_ADC_CONTROL,
+				  AD4130_MODE_MASK,
+				  FIELD_PREP(AD4130_MODE_MASK, mode));
+}
+
+static int ad4130_set_watermark_interrupt_en(struct ad4130_state *st, bool en)
+{
+	return regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
+				  AD4130_WATERMARK_INT_EN_MASK,
+				  en ? AD4130_WATERMARK_INT_EN_MASK : 0);
+}
+
+static unsigned int ad4130_watermark_reg_val(unsigned int val)
+{
+	if (val == AD4130_FIFO_SIZE)
+		val = AD4130_WATERMARK_256;
+
+	return val;
+}
+
+static int ad4130_set_fifo_mode(struct ad4130_state *st,
+				enum ad4130_fifo_mode mode)
+{
+	return regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
+				  AD4130_FIFO_MODE_MASK,
+				  FIELD_PREP(AD4130_FIFO_MODE_MASK, mode));
+}
+
+static void ad4130_push_fifo_data(struct iio_dev *indio_dev)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int data_reg_size = ad4130_data_reg_size(st);
+	unsigned int transfer_len = st->effective_watermark * data_reg_size;
+	unsigned int set_size = st->num_enabled_channels * data_reg_size;
+	unsigned int i;
+	int ret;
+
+	st->fifo_tx_buf[1] = ad4130_watermark_reg_val(st->effective_watermark);
+	st->fifo_xfer[1].len = transfer_len;
+
+	ret = spi_sync(st->spi, &st->fifo_msg);
+	if (ret)
+		return;
+
+	for (i = 0; i < transfer_len; i += set_size)
+		iio_push_to_buffers(indio_dev, &st->fifo_rx_buf[i]);
+}
+
+static irqreturn_t ad4130_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ad4130_state *st = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		ad4130_push_fifo_data(indio_dev);
+	else
+		complete(&st->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int ad4130_find_slot(struct ad4130_state *st,
+			    struct ad4130_setup_info *target_setup_info,
+			    unsigned int *slot, bool *overwrite)
+{
+	unsigned int i;
+
+	*slot = AD4130_INVALID_SLOT;
+	*overwrite = false;
+
+	for (i = 0; i < AD4130_MAX_SETUPS; i++) {
+		struct ad4130_setup_info *setup_info = &st->setups_info[i];
+
+		/* Immediately accept a matching setup info. */
+		if (!memcmp(target_setup_info, setup_info, AD4130_SETUP_SIZE)) {
+			*slot = i;
+			return 0;
+		}
+
+		/* Ignore all setups which are used by enabled channels. */
+		if (setup_info->enabled_channels)
+			continue;
+
+		/* Find the least used slot. */
+		if (*slot == AD4130_INVALID_SLOT ||
+		    setup_info->channels < st->setups_info[*slot].channels)
+			*slot = i;
+	}
+
+	if (*slot == AD4130_INVALID_SLOT)
+		return -EINVAL;
+
+	*overwrite = true;
+
+	return 0;
+}
+
+static void ad4130_unlink_channel(struct ad4130_state *st, unsigned int channel)
+{
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4130_setup_info *setup_info = &st->setups_info[chan_info->slot];
+
+	chan_info->slot = AD4130_INVALID_SLOT;
+	setup_info->channels--;
+}
+
+static int ad4130_unlink_slot(struct ad4130_state *st, unsigned int slot)
+{
+	struct ad4130_setup_info *setup_info = &st->setups_info[slot];
+	unsigned int i;
+
+	if (setup_info->enabled_channels)
+		return -EINVAL;
+
+	for (i = 0; i < AD4130_MAX_CHANNELS; i++) {
+		struct ad4130_chan_info *chan_info = &st->chans_info[i];
+
+		if (!chan_info->initialized || chan_info->slot != slot)
+			continue;
+
+		ad4130_unlink_channel(st, i);
+	}
+
+	return 0;
+}
+
+static int ad4130_link_channel_slot(struct ad4130_state *st,
+				    unsigned int channel, unsigned int slot)
+{
+	struct ad4130_setup_info *setup_info = &st->setups_info[slot];
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	int ret;
+
+	ret = regmap_update_bits(st->regmap, AD4130_REG_CHANNEL_X(channel),
+				 AD4130_SETUP_MASK,
+				 FIELD_PREP(AD4130_SETUP_MASK, slot));
+	if (ret)
+		return ret;
+
+	chan_info->slot = slot;
+	setup_info->channels++;
+
+	return 0;
+}
+
+static int ad4130_write_slot_setup(struct ad4130_state *st,
+				   unsigned int slot,
+				   struct ad4130_setup_info *setup_info)
+{
+	unsigned int val;
+	int ret;
+
+	val = FIELD_PREP(AD4130_IOUT1_VAL_MASK, setup_info->iout0_val) |
+	      FIELD_PREP(AD4130_IOUT1_VAL_MASK, setup_info->iout1_val) |
+	      FIELD_PREP(AD4130_BURNOUT_MASK, setup_info->burnout) |
+	      FIELD_PREP(AD4130_REF_BUFP_MASK, setup_info->ref_bufp) |
+	      FIELD_PREP(AD4130_REF_BUFM_MASK, setup_info->ref_bufm) |
+	      FIELD_PREP(AD4130_REF_SEL_MASK, setup_info->ref_sel) |
+	      FIELD_PREP(AD4130_PGA_MASK, setup_info->pga);
+
+	ret = regmap_write(st->regmap, AD4130_REG_CONFIG_X(slot), val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4130_FILTER_MODE_MASK, setup_info->filter_mode) |
+	      FIELD_PREP(AD4130_FILTER_SELECT_MASK, setup_info->fs);
+
+	ret = regmap_write(st->regmap, AD4130_REG_FILTER_X(slot), val);
+	if (ret)
+		return ret;
+
+	memcpy(&st->setups_info[slot], setup_info, AD4130_SETUP_SIZE);
+
+	return 0;
+}
+
+static int ad4130_write_channel_setup(struct ad4130_state *st,
+				      unsigned int channel, bool on_enable)
+{
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4130_setup_info *setup_info = &chan_info->setup;
+	bool overwrite;
+	int slot;
+	int ret;
+
+	/*
+	 * The following cases need to be handled.
+	 *
+	 * 1. Enabled and linked channel with setup changes:
+	 *    - Find slot. If not possible, return error.
+	 *    - Unlink channel from current slot.
+	 *    - If slot has channels linked to it, unlink all channels, and
+	 *      write the new setup to it.
+	 *    - Link channel to new slot.
+	 *
+	 * 2. Soon to be enabled and unlinked channel:
+	 *    - Find slot. If not possible, return error.
+	 *    - If slot has channels linked to it, unlink all channels, and
+	 *      write the new setup to it.
+	 *    - Link channel to slot.
+	 *
+	 * 3. Disabled and linked channel with setup changes:
+	 *    - Unlink channel from current slot.
+	 *
+	 * 4. Soon to be enabled and linked channel:
+	 * 5. Disabled and unlinked channel with setup changes:
+	 *    - Do nothing.
+	 */
+
+	/* Case 4 */
+	if (on_enable && chan_info->slot != AD4130_INVALID_SLOT)
+		return 0;
+
+	if (!on_enable && !chan_info->enabled) {
+		if (chan_info->slot != AD4130_INVALID_SLOT)
+			/* Case 3 */
+			ad4130_unlink_channel(st, channel);
+
+		/* Case 3 & 5 */
+		return 0;
+	}
+
+	/* Case 1 & 2 */
+	ret = ad4130_find_slot(st, setup_info, &slot, &overwrite);
+	if (ret)
+		return ret;
+
+	if (chan_info->slot != AD4130_INVALID_SLOT)
+		/* Case 1 only */
+		ad4130_unlink_channel(st, channel);
+
+	if (overwrite) {
+		ret = ad4130_unlink_slot(st, slot);
+		if (ret)
+			return ret;
+
+		ret = ad4130_write_slot_setup(st, slot, setup_info);
+		if (ret)
+			return ret;
+	}
+
+	return ad4130_link_channel_slot(st, channel, slot);
+}
+
+static int ad4130_set_channel_enable(struct ad4130_state *st,
+				     unsigned int channel, bool status)
+{
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4130_setup_info *setup_info;
+	int ret;
+
+	if (chan_info->enabled == status)
+		return 0;
+
+	if (status) {
+		ret = ad4130_write_channel_setup(st, channel, true);
+		if (ret)
+			return ret;
+	}
+
+	setup_info = &st->setups_info[chan_info->slot];
+
+	ret = regmap_update_bits(st->regmap, AD4130_REG_CHANNEL_X(channel),
+				 AD4130_CHANNEL_EN_MASK,
+				 status ? AD4130_CHANNEL_EN_MASK : 0);
+	if (ret)
+		return ret;
+
+	setup_info->enabled_channels += status ? 1 : -1;
+	chan_info->enabled = status;
+
+	return 0;
+}
+
+static void ad4130_freq_to_fs(enum ad4130_filter_mode filter_mode,
+			      int val, int val2, unsigned int *fs, bool db3)
+{
+	const struct ad4130_filter_config *filter_config =
+		&ad4130_filter_configs[filter_mode];
+	unsigned long long dividend, divisor;
+	int temp;
+
+	dividend = filter_config->fs_max * filter_config->odr_div *
+		   (val * AD4130_FREQ_FACTOR + val2);
+	divisor = AD4130_MAX_ODR * AD4130_FREQ_FACTOR;
+
+	if (db3) {
+		dividend *= AD4130_DB3_FACTOR;
+		divisor *= filter_config->db3_div;
+	}
+
+	temp = AD4130_FS_MIN + filter_config->fs_max -
+	       DIV64_U64_ROUND_CLOSEST(dividend, divisor);
+
+	if (temp < AD4130_FS_MIN)
+		temp = AD4130_FS_MIN;
+	else if (temp > filter_config->fs_max)
+		temp = filter_config->fs_max;
+
+	*fs = temp;
+}
+
+static void ad4130_fs_to_freq(enum ad4130_filter_mode filter_mode,
+			      unsigned int fs, int *val, int *val2, bool db3)
+{
+	const struct ad4130_filter_config *filter_config =
+		&ad4130_filter_configs[filter_mode];
+	unsigned int dividend, divisor;
+	u64 temp;
+
+	dividend = (filter_config->fs_max - fs + AD4130_FS_MIN) *
+		   AD4130_MAX_ODR;
+	divisor = filter_config->fs_max * filter_config->odr_div;
+
+	if (db3) {
+		dividend *= filter_config->db3_div;
+		divisor *= AD4130_DB3_FACTOR;
+	}
+
+	temp = div_u64(dividend * AD4130_FREQ_FACTOR, divisor);
+	*val = div_u64_rem(temp, AD4130_FREQ_FACTOR, val2);
+}
+
+static int ad4130_set_filter_mode(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int val)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int channel = chan->scan_index;
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4130_setup_info *setup_info = &chan_info->setup;
+	enum ad4130_filter_mode old_filter_mode;
+	int freq_val, freq_val2;
+	unsigned int old_fs;
+	int ret = 0;
+
+	mutex_lock(&st->lock);
+	if (setup_info->filter_mode == val)
+		goto out;
+
+	old_fs = setup_info->fs;
+	old_filter_mode = setup_info->filter_mode;
+
+	ad4130_fs_to_freq(setup_info->filter_mode, setup_info->fs,
+			  &freq_val, &freq_val2, false);
+
+	ad4130_freq_to_fs(val, freq_val, freq_val2, &setup_info->fs, false);
+
+	setup_info->filter_mode = val;
+
+	ret = ad4130_write_channel_setup(st, channel, false);
+	if (ret) {
+		setup_info->fs = old_fs;
+		setup_info->filter_mode = old_filter_mode;
+	}
+
+ out:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int ad4130_get_filter_mode(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int channel = chan->scan_index;
+	struct ad4130_setup_info *setup_info = &st->chans_info[channel].setup;
+	enum ad4130_filter_mode filter_mode;
+
+	mutex_lock(&st->lock);
+	filter_mode = setup_info->filter_mode;
+	mutex_unlock(&st->lock);
+
+	return filter_mode;
+}
+
+static const struct iio_enum ad4130_filter_mode_enum = {
+	.items = ad4130_filter_modes_str,
+	.num_items = ARRAY_SIZE(ad4130_filter_modes_str),
+	.set = ad4130_set_filter_mode,
+	.get = ad4130_get_filter_mode
+};
+
+static const struct iio_chan_spec_ext_info ad4130_filter_mode_ext_info[] = {
+	IIO_ENUM("filter_mode", IIO_SEPARATE, &ad4130_filter_mode_enum),
+	IIO_ENUM_AVAILABLE("filter_mode", IIO_SHARED_BY_TYPE,
+			   &ad4130_filter_mode_enum),
+	{ },
+};
+
+static const struct iio_chan_spec ad4130_channel_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.differential = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			      BIT(IIO_CHAN_INFO_SCALE) |
+			      BIT(IIO_CHAN_INFO_OFFSET) |
+			      BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+			      BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE) |
+					BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	.ext_info = ad4130_filter_mode_ext_info,
+	.scan_type = {
+		.sign = 'u',
+		.endianness = IIO_BE,
+	},
+};
+
+static int ad4130_set_channel_pga(struct ad4130_state *st, unsigned int channel,
+				  int val, int val2)
+{
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4130_setup_info *setup_info = &chan_info->setup;
+	unsigned int pga, old_pga;
+	int ret = 0;
+
+	for (pga = 0; pga < AD4130_PGA_NUM; pga++)
+		if (val == st->scale_tbls[setup_info->ref_sel][pga][0] &&
+		    val2 == st->scale_tbls[setup_info->ref_sel][pga][1])
+			break;
+
+	if (pga == AD4130_PGA_NUM)
+		return -EINVAL;
+
+	mutex_lock(&st->lock);
+	if (pga == setup_info->pga)
+		goto out;
+
+	old_pga = setup_info->pga;
+	setup_info->pga = pga;
+
+	ret = ad4130_write_channel_setup(st, channel, false);
+	if (ret)
+		setup_info->pga = old_pga;
+
+out:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int ad4130_set_channel_freq(struct ad4130_state *st,
+				   unsigned int channel,
+				   int val, int val2, bool db3)
+{
+	struct ad4130_chan_info *chan_info = &st->chans_info[channel];
+	struct ad4130_setup_info *setup_info = &chan_info->setup;
+	unsigned int fs, old_fs;
+	int ret = 0;
+
+	mutex_lock(&st->lock);
+	old_fs = setup_info->fs;
+
+	ad4130_freq_to_fs(setup_info->filter_mode, val, val2, &fs, db3);
+
+	if (fs == setup_info->fs)
+		goto out;
+
+	setup_info->fs = fs;
+
+	ret = ad4130_write_channel_setup(st, channel, false);
+	if (ret)
+		setup_info->fs = old_fs;
+
+out:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int _ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
+			       int *val)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4130_set_channel_enable(st, channel, true);
+	if (ret)
+		return ret;
+
+	reinit_completion(&st->completion);
+
+	ret = ad4130_set_mode(st, AD4130_MODE_CONTINUOUS);
+	if (ret)
+		return ret;
+
+	ret = wait_for_completion_timeout(&st->completion,
+					  msecs_to_jiffies(1000));
+	if (!ret)
+		return -ETIMEDOUT;
+
+	ret = ad4130_set_mode(st, AD4130_MODE_IDLE);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4130_REG_DATA, val);
+	if (ret)
+		return ret;
+
+	ret = ad4130_set_channel_enable(st, channel, false);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT;
+}
+
+static int ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
+			      int *val)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	mutex_lock(&st->lock);
+	ret = _ad4130_read_sample(indio_dev, channel, val);
+	mutex_unlock(&st->lock);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
+}
+
+static int ad4130_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int channel = chan->scan_index;
+	struct ad4130_setup_info *setup_info = &st->chans_info[channel].setup;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		return ad4130_read_sample(indio_dev, channel, val);
+	case IIO_CHAN_INFO_SCALE:
+		mutex_lock(&st->lock);
+		*val = st->scale_tbls[setup_info->ref_sel][setup_info->pga][0];
+		*val2 = st->scale_tbls[setup_info->ref_sel][setup_info->pga][1];
+		mutex_unlock(&st->lock);
+
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&st->lock);
+		ad4130_fs_to_freq(setup_info->filter_mode, setup_info->fs,
+				  val, val2, false);
+		mutex_unlock(&st->lock);
+
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		mutex_lock(&st->lock);
+		ad4130_fs_to_freq(setup_info->filter_mode, setup_info->fs,
+				  val, val2, true);
+		mutex_unlock(&st->lock);
+
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4130_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int channel = chan->scan_index;
+	struct ad4130_setup_info *setup_info = &st->chans_info[channel].setup;
+	const struct ad4130_filter_config *filter_config;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)st->scale_tbls[setup_info->ref_sel];
+		*length = ARRAY_SIZE(st->scale_tbls[setup_info->ref_sel]) * 2;
+
+		*type = IIO_VAL_INT_PLUS_NANO;
+
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&st->lock);
+		filter_config = &ad4130_filter_configs[setup_info->filter_mode];
+		mutex_unlock(&st->lock);
+
+		*vals = (int *)filter_config->samp_freq_avail;
+		*length = filter_config->samp_freq_avail_len * 2;
+		*type = IIO_VAL_FRACTIONAL;
+
+		return filter_config->samp_freq_avail_type;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		mutex_lock(&st->lock);
+		filter_config = &ad4130_filter_configs[setup_info->filter_mode];
+		mutex_unlock(&st->lock);
+
+		*vals = (int *)filter_config->db3_freq_avail;
+		*length = filter_config->db3_freq_avail_len * 2;
+		*type = IIO_VAL_FRACTIONAL;
+
+		return filter_config->db3_freq_avail_type;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4130_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+	case IIO_CHAN_INFO_SAMP_FREQ:
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4130_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int channel = chan->scan_index;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		return ad4130_set_channel_pga(st, channel, val, val2);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4130_set_channel_freq(st, channel, val, val2, false);
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return ad4130_set_channel_freq(st, channel, val, val2, true);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4130_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static int ad4130_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int val = 0;
+	unsigned int i;
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		bool status = test_bit(i, scan_mask);
+
+		if (!status)
+			continue;
+
+		ret = ad4130_set_channel_enable(st, i, status);
+		if (ret)
+			goto out;
+
+		val++;
+	}
+
+	st->num_enabled_channels = val;
+
+out:
+	mutex_unlock(&st->lock);
+
+	return 0;
+}
+
+static int ad4130_set_fifo_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int eff;
+	int ret = 0;
+
+	if (val > AD4130_FIFO_SIZE)
+		return -EINVAL;
+
+	/*
+	 * Always set watermark to a multiple of the number of enabled channels
+	 * to avoid making the FIFO unaligned.
+	 */
+	eff = rounddown(val, st->num_enabled_channels);
+
+	mutex_lock(&st->lock);
+
+	ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
+				 AD4130_WATERMARK_MASK,
+				 FIELD_PREP(AD4130_WATERMARK_MASK,
+					    ad4130_watermark_reg_val(eff)));
+	if (ret)
+		goto out;
+
+	st->effective_watermark = eff;
+	st->watermark = val;
+
+out:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static const struct iio_info ad4130_info = {
+	.read_raw = ad4130_read_raw,
+	.read_avail = ad4130_read_avail,
+	.write_raw_get_fmt = ad4130_write_raw_get_fmt,
+	.write_raw = ad4130_write_raw,
+	.update_scan_mode = ad4130_update_scan_mode,
+	.hwfifo_set_watermark = ad4130_set_fifo_watermark,
+	.debugfs_reg_access = ad4130_reg_access,
+};
+
+static int ad4130_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	ret = ad4130_set_watermark_interrupt_en(st, true);
+	if (ret)
+		goto out;
+
+	/* When the chip enters FIFO mode, IRQ polarity is inversed. */
+	ret = irq_set_irq_type(st->spi->irq, st->inv_irq_trigger);
+	if (ret)
+		goto out;
+
+	ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_WATERMARK);
+	if (ret)
+		goto out;
+
+	ret = ad4130_set_mode(st, AD4130_MODE_CONTINUOUS);
+
+out:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int ad4130_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int i;
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	ret = ad4130_set_mode(st, AD4130_MODE_IDLE);
+	if (ret)
+		goto out;
+
+	/* When the chip exits FIFO mode, IRQ polarity returns to normal. */
+	ret = irq_set_irq_type(st->spi->irq, st->irq_trigger);
+	if (ret)
+		goto out;
+
+	ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_DISABLED);
+	if (ret)
+		goto out;
+
+	ret = ad4130_set_watermark_interrupt_en(st, false);
+	if (ret)
+		goto out;
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		ret = ad4130_set_channel_enable(st, i, false);
+		if (ret)
+			goto out;
+	}
+
+out:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static const struct iio_buffer_setup_ops ad4130_buffer_ops = {
+	.postenable = ad4130_buffer_postenable,
+	.predisable = ad4130_buffer_predisable,
+};
+
+static ssize_t ad4130_get_fifo_watermark(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct ad4130_state *st = iio_priv(dev_to_iio_dev(dev));
+	unsigned int val;
+
+	mutex_lock(&st->lock);
+	val = st->watermark;
+	mutex_unlock(&st->lock);
+
+	return sysfs_emit(buf, "%d\n", val);
+}
+
+static ssize_t ad4130_get_fifo_enabled(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct ad4130_state *st = iio_priv(dev_to_iio_dev(dev));
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4130_REG_FIFO_CONTROL, &val);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(AD4130_FIFO_MODE_MASK, val);
+
+	return sysfs_emit(buf, "%d\n", val != AD4130_FIFO_MODE_DISABLED);
+}
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_CONST_ATTR(hwfifo_watermark_max,
+		      __stringify(AD4130_FIFO_SIZE));
+static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
+		       ad4130_get_fifo_watermark, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
+		       ad4130_get_fifo_enabled, NULL, 0);
+
+static const struct attribute *ad4130_fifo_attributes[] = {
+	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL,
+};
+
+static int find_table_index(const unsigned int *tbl, size_t len,
+			    unsigned int val)
+{
+	unsigned int i;
+
+	for (i = 0; i < len; i++)
+		if (tbl[i] == val)
+			return i;
+
+	return -EINVAL;
+}
+
+static int ad4130_get_ref_voltage(struct ad4130_state *st,
+				  enum ad4130_ref_sel ref_sel,
+				  unsigned int *ref_uv)
+{
+	struct device *dev = &st->spi->dev;
+	int ret;
+
+	switch (ref_sel) {
+	case AD4130_REF_REFIN1:
+		ret = regulator_get_voltage(st->regulators[2].consumer);
+		break;
+	case AD4130_REF_REFIN2:
+		ret = regulator_get_voltage(st->regulators[3].consumer);
+		break;
+	case AD4130_REF_AVDD_AVSS:
+		ret = regulator_get_voltage(st->regulators[0].consumer);
+		break;
+	case AD4130_REF_REFOUT_AVSS:
+		if (!st->int_ref_en) {
+			ret = -EINVAL;
+			break;
+		}
+
+		ret = st->int_ref_uv;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (ret <= 0)
+		return dev_err_probe(dev, ret, "Cannot use reference %u\n",
+				     ref_sel);
+
+	if (ref_uv)
+		*ref_uv = ret;
+
+	return 0;
+}
+
+static int ad4130_parse_fw_setup(struct ad4130_state *st,
+				 struct fwnode_handle *child,
+				 struct ad4130_setup_info *setup_info)
+{
+	struct device *dev = &st->spi->dev;
+	u32 current_na;
+	int ret;
+
+	current_na = 0;
+	fwnode_property_read_u32(child, "adi,excitation-current-0-nanoamps",
+				 &current_na);
+	ret = find_table_index(ad4130_iout_current_na_tbl, AD4130_IOUT_MAX,
+			       current_na);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Invalid excitation current %unA\n",
+				     current_na);
+	setup_info->iout0_val = ret;
+
+	current_na = 0;
+	fwnode_property_read_u32(child, "adi,excitation-current-1-nanoamps",
+				 &current_na);
+	ret = find_table_index(ad4130_iout_current_na_tbl, AD4130_IOUT_MAX,
+			       current_na);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Invalid excitation current %unA\n",
+				     current_na);
+	setup_info->iout1_val = ret;
+
+	current_na = 0;
+	fwnode_property_read_u32(child, "adi,burnout-current-nanoamps",
+				 &current_na);
+	ret = find_table_index(ad4130_burnout_current_na_tbl,
+			       AD4130_BURNOUT_MAX, current_na);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Invalid burnout current %unA\n",
+				     current_na);
+	setup_info->burnout = ret;
+
+	setup_info->ref_bufp = fwnode_property_read_bool(child, "adi,buffered-positive");
+	setup_info->ref_bufm = fwnode_property_read_bool(child, "adi,buffered-negative");
+
+	setup_info->ref_sel = AD4130_REF_REFOUT_AVSS;
+	fwnode_property_read_u32(child, "adi,reference-select",
+				 &setup_info->ref_sel);
+	if (setup_info->ref_sel >= AD4130_REF_SEL_MAX)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid reference selected %u\n",
+				     setup_info->ref_sel);
+
+	ret = ad4130_get_ref_voltage(st, setup_info->ref_sel, NULL);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ad4130_validate_diff_channel(struct ad4130_state *st, u32 pin)
+{
+	struct device *dev = &st->spi->dev;
+
+	if (pin >= AD4130_MAX_DIFF_INPUTS)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid diffreential channel %u\n", pin);
+
+	if (pin >= AD4130_MAX_ANALOG_PINS)
+		return 0;
+
+	if (st->pins_fn[pin] == AD4130_PIN_FN_SPECIAL)
+		return dev_err_probe(dev, -EINVAL,
+				     "Pin %u already used with fn %u\n", pin,
+				     st->pins_fn[pin]);
+
+	st->pins_fn[pin] |= AD4130_PIN_FN_DIFF;
+
+	return 0;
+}
+
+static int ad4130_validate_diff_channels(struct ad4130_state *st,
+					 u32 *pins, unsigned int len)
+{
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < len; i++) {
+		ret = ad4130_validate_diff_channel(st, pins[i]);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int ad4130_validate_excitation_pin(struct ad4130_state *st, u32 pin)
+{
+	struct device *dev = &st->spi->dev;
+
+	if (pin >= AD4130_MAX_ANALOG_PINS)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid excitation pin %u\n", pin);
+
+	if (st->pins_fn[pin] == AD4130_PIN_FN_SPECIAL)
+		return dev_err_probe(dev, -EINVAL,
+				     "Pin %u already used with fn %u\n", pin,
+				     st->pins_fn[pin]);
+
+	st->pins_fn[pin] |= AD4130_PIN_FN_EXCITATION;
+
+	return 0;
+}
+
+static int ad4130_validate_vbias_pin(struct ad4130_state *st, u32 pin)
+{
+	struct device *dev = &st->spi->dev;
+
+	if (pin >= AD4130_MAX_ANALOG_PINS)
+		return dev_err_probe(dev, -EINVAL, "Invalid vbias pin %u\n",
+				     pin);
+
+	if (st->pins_fn[pin] == AD4130_PIN_FN_SPECIAL)
+		return dev_err_probe(dev, -EINVAL,
+				     "Pin %u already used with fn %u\n", pin,
+				     st->pins_fn[pin]);
+
+	st->pins_fn[pin] |= AD4130_PIN_FN_VBIAS;
+
+	return 0;
+}
+
+static int ad4130_validate_vbias_pins(struct ad4130_state *st,
+				      u32 *pins, unsigned int len)
+{
+	unsigned int i;
+	int ret = 0;
+
+	for (i = 0; i < st->num_vbias_pins; i++) {
+		ret = ad4130_validate_vbias_pin(st, pins[i]);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int ad4130_parse_fw_channel(struct iio_dev *indio_dev,
+				   struct fwnode_handle *child)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	unsigned int index = indio_dev->num_channels++;
+	struct device *dev = &st->spi->dev;
+	struct ad4130_chan_info *chan_info;
+	struct iio_chan_spec *chan;
+	u32 pins[2];
+	int ret;
+
+	if (index >= AD4130_MAX_CHANNELS)
+		return dev_err_probe(dev, -EINVAL, "Too many channels\n");
+
+	chan = &st->chans[index];
+	chan_info = &st->chans_info[index];
+
+	*chan = ad4130_channel_template;
+	chan->scan_type.realbits = st->chip_info->resolution;
+	chan->scan_type.storagebits = st->chip_info->resolution;
+	chan->scan_index = index;
+
+	chan_info->slot = AD4130_INVALID_SLOT;
+	chan_info->setup.fs = AD4130_FS_MIN;
+	chan_info->initialized = true;
+
+	ret = fwnode_property_read_u32_array(child, "diff-channels", pins,
+					     ARRAY_SIZE(pins));
+	if (ret)
+		return ret;
+
+	ret = ad4130_validate_diff_channels(st, pins, ARRAY_SIZE(pins));
+	if (ret)
+		return ret;
+
+	chan->channel = pins[0];
+	chan->channel2 = pins[1];
+
+	fwnode_property_read_u32(child, "adi,excitation-pin-0",
+				 &chan_info->iout0);
+	ret = ad4130_validate_excitation_pin(st, chan_info->iout0);
+	if (ret)
+		return ret;
+
+	fwnode_property_read_u32(child, "adi,excitation-pin-1",
+				 &chan_info->iout1);
+	ret = ad4130_validate_excitation_pin(st, chan_info->iout1);
+	if (ret)
+		return ret;
+
+	return ad4130_parse_fw_setup(st, child, &chan_info->setup);
+}
+
+static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	struct fwnode_handle *child;
+	int ret;
+
+	indio_dev->channels = st->chans;
+
+	device_for_each_child_node(dev, child) {
+		ret = ad4130_parse_fw_channel(indio_dev, child);
+		if (ret)
+			break;
+	}
+
+	fwnode_handle_put(child);
+
+	return ret;
+}
+
+static int ad4310_parse_fw(struct iio_dev *indio_dev)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	unsigned int i;
+	int avdd_uv;
+	int irq;
+	int ret;
+
+	st->mclk = devm_clk_get_optional(dev, "mclk");
+	if (IS_ERR(st->mclk))
+		return dev_err_probe(dev, PTR_ERR(st->mclk),
+				     "Failed to get mclk\n");
+
+	st->int_pin_sel = AD4130_INT_PIN_DOUT_OR_INT;
+
+	for (i = 0; i < ARRAY_SIZE(ad4130_int_pin_names); i++) {
+		irq = of_irq_get_byname(dev->of_node, ad4130_int_pin_names[i]);
+		if (irq > 0) {
+			st->int_pin_sel = i;
+			break;
+		}
+	}
+
+	if (st->int_pin_sel == AD4130_INT_PIN_DOUT ||
+	    (st->int_pin_sel == AD4130_INT_PIN_DOUT_OR_INT &&
+	     !st->chip_info->has_int_pin))
+		return dev_err_probe(dev, -EINVAL,
+				     "Cannot use DOUT as interrupt pin\n");
+
+	if (st->int_pin_sel == AD4130_INT_PIN_P1)
+		st->pins_fn[AD4130_AIN2_P1] = AD4130_PIN_FN_SPECIAL;
+
+	st->mclk_sel = AD4130_MCLK_76_8KHZ;
+	device_property_read_u32(dev, "adi,mclk-sel", &st->mclk_sel);
+
+	if (st->mclk_sel >= AD4130_MCLK_SEL_MAX)
+		return dev_err_probe(dev, -EINVAL, "Invalid clock %u\n",
+				     st->mclk_sel);
+
+	if (st->mclk && (st->mclk_sel == AD4130_MCLK_76_8KHZ ||
+			 st->mclk_sel == AD4130_MCLK_76_8KHZ_OUT))
+		return dev_err_probe(dev, -EINVAL,
+				     "Cannot use external clock\n");
+
+	if (st->int_pin_sel == AD4130_INT_PIN_CLK &&
+	    st->mclk_sel != AD4130_MCLK_76_8KHZ)
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid clock %u for interrupt pin %u\n",
+				     st->mclk_sel, st->int_pin_sel);
+
+	st->int_ref_en = true;
+	if (device_property_present(dev, "adi,int-ref-en"))
+		st->int_ref_en = device_property_read_bool(dev, "adi,int-ref-en");
+
+	st->int_ref_uv = AD4130_INT_REF_2_5V;
+
+	/*
+	 * When the AVDD supply is set to below 2.5V the internal reference of
+	 * 1.25V should be selected.
+	 */
+	avdd_uv = regulator_get_voltage(st->regulators[0].consumer);
+	if (avdd_uv > 0 && avdd_uv < AD4130_INT_REF_2_5V)
+		st->int_ref_uv = AD4130_INT_REF_1_25V;
+
+	st->bipolar = device_property_read_bool(dev, "adi,bipolar");
+
+	ret = device_property_count_u32(dev, "adi,vbias-pins");
+	if (ret > 0) {
+		st->num_vbias_pins = ret;
+
+		ret = device_property_read_u32_array(dev, "adi,vbias-pins",
+						     st->vbias_pins,
+						     st->num_vbias_pins);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to read vbias pins\n");
+
+		ret = ad4130_validate_vbias_pins(st, st->vbias_pins,
+						 st->num_vbias_pins);
+		if (ret)
+			return ret;
+	}
+
+	ret = ad4130_parse_fw_children(indio_dev);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < AD4130_MAX_GPIOS; i++) {
+		if (st->pins_fn[i + AD4130_AIN2_P1] != AD4130_PIN_FN_NONE)
+			continue;
+
+		st->gpio_offsets[st->num_gpios++] = i;
+	}
+
+	return 0;
+}
+
+static void ad4130_fill_scale_tbls(struct ad4130_state *st)
+{
+	unsigned int i, j;
+
+	for (i = 0; i < AD4130_REF_SEL_MAX; i++) {
+		unsigned int ref_uv;
+		int ret;
+
+		ret = ad4130_get_ref_voltage(st, i, &ref_uv);
+		if (ret)
+			continue;
+
+		for (j = 0; j < AD4130_PGA_NUM; j++) {
+			unsigned int pow = st->chip_info->resolution + j -
+					   st->bipolar;
+			unsigned int nv = div_u64(((ref_uv * 1000000000ull) >>
+						   pow), 1000);
+
+			st->scale_tbls[i][j][0] = 0;
+			st->scale_tbls[i][j][1] = nv;
+		}
+	}
+}
+
+static void ad4130_clk_disable_unprepare(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+static int ad4130_setup(struct iio_dev *indio_dev)
+{
+	struct ad4130_state *st = iio_priv(indio_dev);
+	struct device *dev = &st->spi->dev;
+	unsigned int int_ref_val;
+	unsigned long rate = AD4130_MCLK_FREQ_76_8KHZ;
+	unsigned int val;
+	unsigned int i;
+	int ret;
+
+	if (st->mclk_sel == AD4130_MCLK_153_6KHZ_EXT)
+		rate = AD4130_MCLK_FREQ_153_6KHZ;
+
+	ret = clk_set_rate(st->mclk, rate);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(st->mclk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, ad4130_clk_disable_unprepare,
+				       st->mclk);
+	if (ret)
+		return ret;
+
+	if (st->int_ref_uv == AD4130_INT_REF_2_5V)
+		int_ref_val = AD4130_INT_REF_VAL_2_5V;
+	else
+		int_ref_val = AD4130_INT_REF_VAL_1_25V;
+
+	/* Switch to SPI 4-wire mode. */
+	val = AD4130_CSB_EN_MASK;
+	val |= st->bipolar ? AD4130_BIPOLAR_MASK : 0;
+	val |= st->int_ref_en ? AD4130_INT_REF_EN_MASK : 0;
+	val |= FIELD_PREP(AD4130_MODE_MASK, AD4130_MODE_IDLE);
+	val |= FIELD_PREP(AD4130_MCLK_SEL_MASK, st->mclk_sel);
+	val |= FIELD_PREP(AD4130_INT_REF_VAL_MASK, int_ref_val);
+
+	ret = regmap_write(st->regmap, AD4130_REG_ADC_CONTROL, val);
+	if (ret)
+		return ret;
+
+	val = FIELD_PREP(AD4130_INT_PIN_SEL_MASK, st->int_pin_sel);
+	for (i = 0; i < st->num_gpios; i++)
+		val |= BIT(st->gpio_offsets[i]);
+
+	ret = regmap_write(st->regmap, AD4130_REG_IO_CONTROL, val);
+	if (ret)
+		return ret;
+
+	val = 0;
+	for (i = 0; i < st->num_vbias_pins; i++)
+		val |= BIT(st->vbias_pins[i]);
+
+	ret = regmap_write(st->regmap, AD4130_REG_VBIAS, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
+				 AD4130_ADD_FIFO_HEADER_MASK, 0);
+	if (ret)
+		return ret;
+
+	/* FIFO watermark interrupt starts out as enabled, disable it. */
+	ret = ad4130_set_watermark_interrupt_en(st, false);
+	if (ret)
+		return ret;
+
+	/* Setup channels. */
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		struct ad4130_chan_info *chan_info = &st->chans_info[i];
+		struct iio_chan_spec *chan = &st->chans[i];
+		unsigned int val;
+
+		val = FIELD_PREP(AD4130_AINP_MASK, chan->channel) |
+		      FIELD_PREP(AD4130_AINM_MASK, chan->channel2) |
+		      FIELD_PREP(AD4130_IOUT1_MASK, chan_info->iout0) |
+		      FIELD_PREP(AD4130_IOUT2_MASK, chan_info->iout1);
+
+		ret = regmap_write(st->regmap, AD4130_REG_CHANNEL_X(i), val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad4130_soft_reset(struct ad4130_state *st)
+{
+	int ret;
+
+	ret = spi_write(st->spi, st->reset_buf, sizeof(st->reset_buf));
+	if (ret)
+		return ret;
+
+	fsleep(AD4130_SOFT_RESET_SLEEP);
+
+	return 0;
+}
+
+static void ad4130_disable_regulators(void *data)
+{
+	struct ad4130_state *st = data;
+
+	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
+}
+
+static int ad4130_probe(struct spi_device *spi)
+{
+	const struct ad4130_chip_info *info;
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad4130_state *st;
+	int ret;
+
+	info = device_get_match_data(dev);
+	if (!info)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	memset(st->reset_buf, 0xff, AD4130_RESET_BUF_SIZE);
+	init_completion(&st->completion);
+	mutex_init(&st->lock);
+	st->chip_info = info;
+	st->spi = spi;
+
+	/*
+	 * Xfer:   [ XFR1 ] [         XFR2         ]
+	 * Master:  0x7D N   ......................
+	 * Slave:   ......   DATA1 DATA2 ... DATAN
+	 */
+	st->fifo_tx_buf[0] = AD4130_COMMS_READ_MASK | AD4130_REG_FIFO_DATA;
+	st->fifo_xfer[0].tx_buf = st->fifo_tx_buf;
+	st->fifo_xfer[0].len = sizeof(st->fifo_tx_buf);
+	st->fifo_xfer[1].rx_buf = st->fifo_rx_buf;
+	spi_message_init_with_transfers(&st->fifo_msg, st->fifo_xfer,
+					ARRAY_SIZE(st->fifo_xfer));
+
+	indio_dev->name = st->chip_info->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad4130_info;
+
+	st->regmap = devm_regmap_init(dev, NULL, st,
+				      &ad4130_regmap_config);
+	if (IS_ERR(st->regmap))
+		return PTR_ERR(st->regmap);
+
+	st->regulators[0].supply = "avdd";
+	st->regulators[1].supply = "iovdd";
+	st->regulators[2].supply = "refin1";
+	st->regulators[3].supply = "refin2";
+
+	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, ad4130_disable_regulators, st);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add regulators disable action\n");
+
+	ret = ad4130_soft_reset(st);
+	if (ret)
+		return ret;
+
+	ret = ad4310_parse_fw(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ad4130_setup(indio_dev);
+	if (ret)
+		return ret;
+
+	ad4130_fill_scale_tbls(st);
+
+	if (st->num_gpios) {
+		st->gc.owner = THIS_MODULE;
+		st->gc.label = st->chip_info->name;
+		st->gc.base = -1;
+		st->gc.ngpio = AD4130_MAX_GPIOS;
+		st->gc.parent = dev;
+		st->gc.can_sleep = true;
+		st->gc.get_direction = ad4130_gpio_get_direction;
+		st->gc.set = ad4130_gpio_set;
+
+		ret = devm_gpiochip_add_data(dev, &st->gc, st);
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
+					      INDIO_BUFFER_SOFTWARE,
+					      &ad4130_buffer_ops,
+					      ad4130_fifo_attributes);
+	if (ret)
+		return ret;
+
+	ret = devm_request_threaded_irq(dev, spi->irq, NULL,
+					ad4130_irq_handler, IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request irq\n");
+
+	st->irq_trigger = irq_get_trigger_type(spi->irq);
+	if (st->irq_trigger & IRQF_TRIGGER_RISING)
+		st->inv_irq_trigger = IRQF_TRIGGER_FALLING;
+	else if (st->irq_trigger & IRQF_TRIGGER_FALLING)
+		st->inv_irq_trigger = IRQF_TRIGGER_RISING;
+	else
+		return dev_err_probe(dev, -EINVAL, "Invalid irq flags: %u\n",
+				     st->irq_trigger);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct ad4130_chip_info ad4130_chip_info_tbl[] = {
+	[ID_AD4130_8_24_WLCSP] = {
+		.name = AD4130_8_NAME,
+		.resolution = 24,
+		.has_int_pin = true,
+	},
+	[ID_AD4130_8_24_LFCSP] = {
+		.name = AD4130_8_NAME,
+		.resolution = 24,
+		.has_int_pin = false,
+	},
+	[ID_AD4130_8_16_WLCSP] = {
+		.name = AD4130_8_NAME,
+		.resolution = 16,
+		.has_int_pin = true,
+	},
+	[ID_AD4130_8_16_LFCSP] = {
+		.name = AD4130_8_NAME,
+		.resolution = 16,
+		.has_int_pin = false,
+	},
+};
+
+static const struct of_device_id ad4130_of_match[] = {
+	{
+		.compatible = "adi,ad4130-8-16-lfcsp",
+		.data = &ad4130_chip_info_tbl[ID_AD4130_8_16_LFCSP],
+	},
+	{
+		.compatible = "adi,ad4130-8-16-wlcsp",
+		.data = &ad4130_chip_info_tbl[ID_AD4130_8_16_WLCSP],
+	},
+	{
+		.compatible = "adi,ad4130-8-24-lfcsp",
+		.data = &ad4130_chip_info_tbl[ID_AD4130_8_24_LFCSP],
+	},
+	{
+		.compatible = "adi,ad4130-8-24-wlcsp",
+		.data = &ad4130_chip_info_tbl[ID_AD4130_8_24_WLCSP],
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ad4130_of_match);
+
+static struct spi_driver ad4130_driver = {
+	.driver = {
+		.name = "ad4130",
+		.of_match_table = ad4130_of_match,
+	},
+	.probe = ad4130_probe,
+};
+module_spi_driver(ad4130_driver);
+
+MODULE_AUTHOR("Cosmin Tanislav <cosmin.tanislav@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4130 SPI driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1


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

* Re: [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130
  2022-04-13  9:40 [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Cosmin Tanislav
  2022-04-13  9:40 ` [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available} Cosmin Tanislav
  2022-04-13  9:40 ` [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver Cosmin Tanislav
@ 2022-04-13 12:26 ` Rob Herring
  2022-04-13 14:50 ` Andy Shevchenko
  2022-04-16 15:00 ` Jonathan Cameron
  4 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2022-04-13 12:26 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Rob Herring, Jonathan Cameron, Cosmin Tanislav, linux-iio,
	linux-kernel, devicetree, linux-gpio, Linus Walleij

On Wed, 13 Apr 2022 12:40:09 +0300, Cosmin Tanislav wrote:
> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
> 
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
>  1 file changed, 255 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: patternProperties:^channel@([0-9]|1[0-5])$:properties:adi,excitation-pin-0: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('minimum', 'maximum', 'default' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: patternProperties:^channel@([0-9]|1[0-5])$:properties:adi,excitation-pin-0: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: patternProperties:^channel@([0-9]|1[0-5])$:properties:adi,excitation-pin-0: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:interrupts: 'anyOf' conditional failed, one must be fixed:
	'minItems' is not one of ['maxItems', 'description', 'deprecated']
		hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values.
	'minItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref']
	'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref']
	1 is less than the minimum of 2
		hint: Arrays must be described with a combination of minItems/maxItems/items
	hint: cell array properties must define how many entries and what the entries are when there is more than one entry.
	from schema $id: http://devicetree.org/meta-schemas/interrupts.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,int-ref-en: 'oneOf' conditional failed, one must be fixed:
	Additional properties are not allowed ('default' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,int-ref-en: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	Additional properties are not allowed ('type', 'default' were unexpected)
		hint: A vendor string property with exact values has an implicit type
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,int-ref-en: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,vbias-pins: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('items' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,vbias-pins: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,vbias-pins: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,bipolar: 'oneOf' conditional failed, one must be fixed:
	Additional properties are not allowed ('default' was unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,bipolar: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	Additional properties are not allowed ('type', 'default' were unexpected)
		hint: A vendor string property with exact values has an implicit type
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: properties:adi,bipolar: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml: ignoring, error in schema: patternProperties: ^channel@([0-9]|1[0-5])$: properties: adi,excitation-pin-0
Error: Documentation/devicetree/bindings/iio/adc/adi,ad4130.example.dts:35.30-31 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:364: Documentation/devicetree/bindings/iio/adc/adi,ad4130.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1401: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


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

* Re: [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130
  2022-04-13  9:40 [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Cosmin Tanislav
                   ` (2 preceding siblings ...)
  2022-04-13 12:26 ` [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Rob Herring
@ 2022-04-13 14:50 ` Andy Shevchenko
  2022-04-16 15:00 ` Jonathan Cameron
  4 siblings, 0 replies; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-13 14:50 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav

On Wed, Apr 13, 2022 at 2:08 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>
> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
>
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.

Something is wrong about the indentation above. It may be reconfigured
to fit more characters per line.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available}
  2022-04-13  9:40 ` [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available} Cosmin Tanislav
@ 2022-04-13 14:51   ` Andy Shevchenko
  2022-04-14  7:41     ` Cosmin Tanislav
  2022-04-16 15:02   ` Jonathan Cameron
  1 sibling, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-13 14:51 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav

On Wed, Apr 13, 2022 at 4:17 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:

It's good you provided documentation, but I think the part "ABI:" is
not needed in the Subject.

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
>
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.

Indentation issue as per patch 1.

...

> +               Set the filter mode of the differential channel. When the filter
> +               mode changes, the in_voltageY-voltageZ_sampling_frequency and
> +               in_voltageY-voltageZ_sampling_frequency_available attributes
> +               might also change to accomodate the new filter mode.

accommodate

> +               If the current sampling frequency is out of range for the new
> +               filter mode, the sampling frequency will be changed to the
> +               closest valid one.


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-13  9:40 ` [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver Cosmin Tanislav
@ 2022-04-13 15:41   ` Andy Shevchenko
  2022-04-14 11:06     ` Cosmin Tanislav
  2022-04-16 16:21   ` Jonathan Cameron
  1 sibling, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-13 15:41 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav

On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:


Thanks for the contribution, my comments below.

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
>
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.

Indentation issue as per previous patches.

...

> +// SPDX-License-Identifier: GPL-2.0+

The

// SPDX-License-Identifier: GPL-2.0-or-later

can be a bit more explicit, but it's up to your company lawyers.

...

> +#include <asm/div64.h>
> +#include <asm/unaligned.h>

Please, move this after linux/*

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>

Looks like iio.h is missed, in any case, can you split this group of
headers and put it after all the rest of linux/* and asm/* ? Ah, you
even have them below, so move these there.

> +#include <linux/module.h>

> +#include <linux/of_irq.h>

Get rid of this one.

> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

...

> +#define AD4130_8_NAME                  "ad4130-8"

What the meaning of -8 ? Is it number of channels? Or is it part of
the official model (part number)? Can we see, btw, Datasheet: tag with
a corresponding link in the commit message?

...

> +#define AD4130_RESET_CLK_COUNT         64
> +#define AD4130_RESET_BUF_SIZE          (AD4130_RESET_CLK_COUNT / 8)

To be more precise shouldn't the above need to have DIV_ROUND_UP() ?

...

> +#define AD4130_SOFT_RESET_SLEEP                (160 * 1000000 / AD4130_MCLK_FREQ_76_8KHZ)

Units? Also, can you use definitions from units.h?

...

> +#define AD4130_FREQ_FACTOR             1000000000ull
> +#define AD4130_DB3_FACTOR              1000

Ditto.

...

> +enum ad4130_mclk_sel {
> +       AD4130_MCLK_76_8KHZ,
> +       AD4130_MCLK_76_8KHZ_OUT,
> +       AD4130_MCLK_76_8KHZ_EXT,
> +       AD4130_MCLK_153_6KHZ_EXT,
> +       AD4130_MCLK_SEL_MAX,

No comma after MAX, if I understood correctly that it's a terminator.
Ditto for other MAXes in the other enums.

> +};

...

> +enum ad4130_fifo_mode {
> +       AD4130_FIFO_MODE_DISABLED = 0b00,
> +       AD4130_FIFO_MODE_WATERMARK = 0b01,
> +};
> +
> +enum ad4130_mode {
> +       AD4130_MODE_CONTINUOUS = 0b0000,
> +       AD4130_MODE_IDLE = 0b0100,
> +};

0b?! Hmm... Not that this is bad, just not so usual :-)

...

> +enum ad4130_pin_function {
> +       AD4130_PIN_FN_NONE,
> +       AD4130_PIN_FN_SPECIAL = 1 << 0,
> +       AD4130_PIN_FN_DIFF = 1 << 1,
> +       AD4130_PIN_FN_EXCITATION = 1 << 2,
> +       AD4130_PIN_FN_VBIAS = 1 << 3,

Why not BIT()?

> +};

...

> +#define AD4130_SETUP_SIZE              offsetof(struct ad4130_setup_info, \
> +                                                enabled_channels)

It's uglier than simply

#define AD4130_SETUP_SIZE              offsetof(struct
ad4130_setup_info, enabled_channels)

or

#define AD4130_SETUP_SIZE              \
        offsetof(struct ad4130_setup_info, enabled_channels)

...

> +struct ad4130_filter_config {
> +       enum ad4130_filter_mode         filter_mode;
> +       unsigned int                    odr_div;
> +       unsigned int                    fs_max;
> +       unsigned int                    db3_div;
> +       enum iio_available_type         samp_freq_avail_type;
> +       int                             samp_freq_avail_len;
> +       int                             samp_freq_avail[3][2];
> +       enum iio_available_type         db3_freq_avail_type;
> +       int                             db3_freq_avail_len;
> +       int                             db3_freq_avail[3][2];

These 3:s can be defined?

> +};

...

> +       int                             scale_tbls[AD4130_REF_SEL_MAX]
> +                                                 [AD4130_PGA_NUM][2];

Why not on one line?

...

> +       u32                     int_pin_sel;
> +       bool                    int_ref_en;
> +       u32                     int_ref_uv;
> +       u32                     mclk_sel;
> +       bool                    bipolar;

You may save a few bytes if you group bool:s.

...

> +       u8                      fifo_rx_buf[AD4130_FIFO_SIZE *
> +                                           AD4130_FIFO_MAX_SAMPLE_SIZE];

One line?

Also it might be good to add a static_assert() to make sure that
multiplication don't overflow.

...


> +static int ad4130_get_reg_size(struct ad4130_state *st, unsigned int reg,
> +                              unsigned int *size)
> +{

> +       if (reg >= ARRAY_SIZE(ad4130_reg_size))
> +               return -EINVAL;

When this condition is true?

> +       if (reg == AD4130_REG_DATA) {
> +               *size = ad4130_data_reg_size(st);
> +               return 0;
> +       }
> +
> +       *size = ad4130_reg_size[reg];

> +

Redundant blank line.

> +       if (!*size)
> +               return -EINVAL;
> +
> +       return 0;
> +}

...

> +       regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
> +                          value ? mask : 0);

One line?

No error check?

> +}

...

> +static int ad4130_set_watermark_interrupt_en(struct ad4130_state *st, bool en)
> +{
> +       return regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> +                                 AD4130_WATERMARK_INT_EN_MASK,
> +                                 en ? AD4130_WATERMARK_INT_EN_MASK : 0);

I believe with temporary variable for mask it will be neater.

> +}

...

> +       if (setup_info->enabled_channels)
> +               return -EINVAL;

-EBUSY?

...

> +       ret = regmap_update_bits(st->regmap, AD4130_REG_CHANNEL_X(channel),
> +                                AD4130_CHANNEL_EN_MASK,
> +                                status ? AD4130_CHANNEL_EN_MASK : 0);

Temporary variable for mask?

...

> +static void ad4130_freq_to_fs(enum ad4130_filter_mode filter_mode,
> +                             int val, int val2, unsigned int *fs, bool db3)
> +{
> +       const struct ad4130_filter_config *filter_config =
> +               &ad4130_filter_configs[filter_mode];
> +       unsigned long long dividend, divisor;
> +       int temp;
> +
> +       dividend = filter_config->fs_max * filter_config->odr_div *
> +                  (val * AD4130_FREQ_FACTOR + val2);
> +       divisor = AD4130_MAX_ODR * AD4130_FREQ_FACTOR;
> +
> +       if (db3) {
> +               dividend *= AD4130_DB3_FACTOR;
> +               divisor *= filter_config->db3_div;
> +       }
> +
> +       temp = AD4130_FS_MIN + filter_config->fs_max -
> +              DIV64_U64_ROUND_CLOSEST(dividend, divisor);
> +
> +       if (temp < AD4130_FS_MIN)
> +               temp = AD4130_FS_MIN;
> +       else if (temp > filter_config->fs_max)
> +               temp = filter_config->fs_max;
> +
> +       *fs = temp;

Would be nice to put a comment explaining the math behind this code.

> +}
> +
> +static void ad4130_fs_to_freq(enum ad4130_filter_mode filter_mode,
> +                             unsigned int fs, int *val, int *val2, bool db3)
> +{
> +       const struct ad4130_filter_config *filter_config =
> +               &ad4130_filter_configs[filter_mode];
> +       unsigned int dividend, divisor;
> +       u64 temp;
> +
> +       dividend = (filter_config->fs_max - fs + AD4130_FS_MIN) *
> +                  AD4130_MAX_ODR;
> +       divisor = filter_config->fs_max * filter_config->odr_div;
> +
> +       if (db3) {
> +               dividend *= filter_config->db3_div;
> +               divisor *= AD4130_DB3_FACTOR;
> +       }
> +
> +       temp = div_u64(dividend * AD4130_FREQ_FACTOR, divisor);
> +       *val = div_u64_rem(temp, AD4130_FREQ_FACTOR, val2);


Ditto.

> +}

...

> + out:

out_unlock: ?
Ditto for similar cases.

> +       mutex_unlock(&st->lock);
> +
> +       return ret;

...

> +static const struct iio_enum ad4130_filter_mode_enum = {
> +       .items = ad4130_filter_modes_str,
> +       .num_items = ARRAY_SIZE(ad4130_filter_modes_str),
> +       .set = ad4130_set_filter_mode,

> +       .get = ad4130_get_filter_mode

+ Comma at the end.

> +};

...

> +static const struct iio_chan_spec_ext_info ad4130_filter_mode_ext_info[] = {
> +       IIO_ENUM("filter_mode", IIO_SEPARATE, &ad4130_filter_mode_enum),
> +       IIO_ENUM_AVAILABLE("filter_mode", IIO_SHARED_BY_TYPE,
> +                          &ad4130_filter_mode_enum),

> +       { },

No comma for terminator.

> +};

...

> +               *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;

Hmm... It seems like specific way to have a sign_extended, or actually
reduced) mask.
Can you rewrite it with the (potential)UB-free approach?

(Note, that if realbits == 32, this will have a lot of fun in
accordance with C standard.)

...

> +               *vals = (int *)st->scale_tbls[setup_info->ref_sel];

Can we get rid of casting here and in the similar cases?

...

> +       for (i = 0; i < indio_dev->num_channels; i++) {
> +               bool status = test_bit(i, scan_mask);
> +
> +               if (!status)
> +                       continue;

Can't you use for_each_set_bit() instead?

> +       }

...

> +static int ad4130_set_fifo_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> +       struct ad4130_state *st = iio_priv(indio_dev);
> +       unsigned int eff;

> +       int ret = 0;

Redundant assignment

> +
> +       if (val > AD4130_FIFO_SIZE)
> +               return -EINVAL;
> +
> +       /*
> +        * Always set watermark to a multiple of the number of enabled channels
> +        * to avoid making the FIFO unaligned.
> +        */
> +       eff = rounddown(val, st->num_enabled_channels);
> +
> +       mutex_lock(&st->lock);
> +
> +       ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> +                                AD4130_WATERMARK_MASK,
> +                                FIELD_PREP(AD4130_WATERMARK_MASK,
> +                                           ad4130_watermark_reg_val(eff)));

Temporary variable for mask?

> +       if (ret)
> +               goto out;
> +
> +       st->effective_watermark = eff;
> +       st->watermark = val;
> +
> +out:

out_unlock: ?

> +       mutex_unlock(&st->lock);
> +
> +       return ret;
> +}

...

> +static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
> +static IIO_CONST_ATTR(hwfifo_watermark_max,
> +                     __stringify(AD4130_FIFO_SIZE));
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +                      ad4130_get_fifo_watermark, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +                      ad4130_get_fifo_enabled, NULL, 0);

Can these all be oneliners?

...

> +static const struct attribute *ad4130_fifo_attributes[] = {
> +       &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
> +       &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
> +       &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +       &iio_dev_attr_hwfifo_enabled.dev_attr.attr,

> +       NULL,

No comma for terminator.

> +};

...

> +static int ad4130_get_ref_voltage(struct ad4130_state *st,
> +                                 enum ad4130_ref_sel ref_sel,
> +                                 unsigned int *ref_uv)
> +{
> +       struct device *dev = &st->spi->dev;
> +       int ret;
> +
> +       switch (ref_sel) {
> +       case AD4130_REF_REFIN1:
> +               ret = regulator_get_voltage(st->regulators[2].consumer);
> +               break;
> +       case AD4130_REF_REFIN2:
> +               ret = regulator_get_voltage(st->regulators[3].consumer);
> +               break;
> +       case AD4130_REF_AVDD_AVSS:
> +               ret = regulator_get_voltage(st->regulators[0].consumer);
> +               break;
> +       case AD4130_REF_REFOUT_AVSS:

> +               if (!st->int_ref_en) {
> +                       ret = -EINVAL;
> +                       break;
> +               }
> +
> +               ret = st->int_ref_uv;
> +               break;

Can be one if-else instead.

> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       if (ret <= 0)

= 0 ?! Can you elaborate, please, this case taking into account below?

> +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> +                                    ref_sel);
> +
> +       if (ref_uv)
> +               *ref_uv = ret;
> +
> +       return 0;
> +}

...

> +       ret = ad4130_get_ref_voltage(st, setup_info->ref_sel, NULL);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

  return ad4130_get_ref_voltage(st, setup_info->ref_sel, NULL);

...

> +       fwnode_property_read_u32(child, "adi,excitation-pin-0",
> +                                &chan_info->iout0);

No default and/or error check?

...

> +       fwnode_property_read_u32(child, "adi,excitation-pin-1",
> +                                &chan_info->iout1);

Ditto.

...

> +static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
> +{
> +       struct ad4130_state *st = iio_priv(indio_dev);
> +       struct device *dev = &st->spi->dev;
> +       struct fwnode_handle *child;
> +       int ret;
> +
> +       indio_dev->channels = st->chans;
> +
> +       device_for_each_child_node(dev, child) {
> +               ret = ad4130_parse_fw_channel(indio_dev, child);
> +               if (ret)
> +                       break;
> +       }

> +       fwnode_handle_put(child);

There is no need to put fwnode if child is NULL. Moreover, the above
pattern might be percepted wrongly, i.e. one may think that
fwnode_handle_put() is a must after a loop.

> +       return ret;
> +}

...

> +       for (i = 0; i < ARRAY_SIZE(ad4130_int_pin_names); i++) {
> +               irq = of_irq_get_byname(dev->of_node, ad4130_int_pin_names[i]);

fwnode_irq_get_byname()

> +               if (irq > 0) {
> +                       st->int_pin_sel = i;
> +                       break;
> +               }
> +       }

...

> +               st->num_vbias_pins = ret;

I haven't checked this, but be sure that it won't overflow any
preallocated array in the code.

> +               ret = device_property_read_u32_array(dev, "adi,vbias-pins",
> +                                                    st->vbias_pins,
> +                                                    st->num_vbias_pins);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "Failed to read vbias pins\n");

...

> +               for (j = 0; j < AD4130_PGA_NUM; j++) {
> +                       unsigned int pow = st->chip_info->resolution + j -
> +                                          st->bipolar;
> +                       unsigned int nv = div_u64(((ref_uv * 1000000000ull) >>
> +                                                  pow), 1000);

Perhaps macros from units.h?

> +                       st->scale_tbls[i][j][0] = 0;
> +                       st->scale_tbls[i][j][1] = nv;
> +               }

...

> +       [ID_AD4130_8_24_LFCSP] = {
> +               .name = AD4130_8_NAME,
> +               .resolution = 24,

> +               .has_int_pin = false,

No need.

> +       },

...

> +       [ID_AD4130_8_16_LFCSP] = {
> +               .name = AD4130_8_NAME,
> +               .resolution = 16,

> +               .has_int_pin = false,

Ditto.

> +       },

...

> +static const struct of_device_id ad4130_of_match[] = {

> +       { },

No comma for terminator.

> +};

...

Can you explain why regmap locking is needed?

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available}
  2022-04-13 14:51   ` Andy Shevchenko
@ 2022-04-14  7:41     ` Cosmin Tanislav
  0 siblings, 0 replies; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-14  7:41 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav



On 4/13/22 17:51, Andy Shevchenko wrote:
> On Wed, Apr 13, 2022 at 4:17 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
> It's good you provided documentation, but I think the part "ABI:" is
> not needed in the Subject.
> 

Then I guess I could merge this patch into the driver patch?

>> AD4130-8 is an ultra-low power, high precision,
>> measurement solution for low bandwidth battery
>> operated applications.
>>
>> The fully integrated AFE (Analog Front-End)
>> includes a multiplexer for up to 16 single-ended
>> or 8 differential inputs, PGA (Programmable Gain
>> Amplifier), 24-bit Sigma-Delta ADC, on-chip
>> reference and oscillator, selectable filter
>> options, smart sequencer, sensor biasing and
>> excitation options, diagnostics, and a FIFO
>> buffer.
> 
> Indentation issue as per patch 1.
> 
> ...
> 
>> +               Set the filter mode of the differential channel. When the filter
>> +               mode changes, the in_voltageY-voltageZ_sampling_frequency and
>> +               in_voltageY-voltageZ_sampling_frequency_available attributes
>> +               might also change to accomodate the new filter mode.
> 
> accommodate
> 
>> +               If the current sampling frequency is out of range for the new
>> +               filter mode, the sampling frequency will be changed to the
>> +               closest valid one.
> 
> 

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-13 15:41   ` Andy Shevchenko
@ 2022-04-14 11:06     ` Cosmin Tanislav
  2022-04-14 13:45       ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-14 11:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav



On 4/13/22 18:41, Andy Shevchenko wrote:
> On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
> 
> ...
> 
>> +#define AD4130_8_NAME                  "ad4130-8"
> 
> What the meaning of -8 ? Is it number of channels? Or is it part of
> the official model (part number)? Can we see, btw, Datasheet: tag with
> a corresponding link in the commit message?
> 

That's just the name specified in the datasheet. I honestly don't have
much of an idea about why it is like that. Also, I already put the
datasheet in the .yaml documentation. Do you really also want it
in each commit message too?

> 
>> +#define AD4130_RESET_CLK_COUNT         64
>> +#define AD4130_RESET_BUF_SIZE          (AD4130_RESET_CLK_COUNT / 8)
> 
> To be more precise shouldn't the above need to have DIV_ROUND_UP() ?
> 

Does it look like 64 / 8 needs any rounding?

> ...
> 
>> +#define AD4130_FREQ_FACTOR             1000000000ull
>> +#define AD4130_DB3_FACTOR              1000
> 
> Ditto.

AD4130_DB3_FACTOR is unit-less. In the datasheet, the relation between
sampling frequency and 3db frequency is represented as a 0.xyz value,
hence why the db3_div values and 1000 factor.

> 
>> +enum ad4130_fifo_mode {
>> +       AD4130_FIFO_MODE_DISABLED = 0b00,
>> +       AD4130_FIFO_MODE_WATERMARK = 0b01,
>> +};
>> +
>> +enum ad4130_mode {
>> +       AD4130_MODE_CONTINUOUS = 0b0000,
>> +       AD4130_MODE_IDLE = 0b0100,
>> +};
> 
> 0b?! Hmm... Not that this is bad, just not so usual :-)
> 

I always use 0b to be consistent with the datasheet values which are
represented in binary. I think it makes it easier to not mess up
when initially translating the datasheet into code and later when
cross-checking with the datasheet.

> 
>> +struct ad4130_filter_config {
>> +       enum ad4130_filter_mode         filter_mode;
>> +       unsigned int                    odr_div;
>> +       unsigned int                    fs_max;
>> +       unsigned int                    db3_div;
>> +       enum iio_available_type         samp_freq_avail_type;
>> +       int                             samp_freq_avail_len;
>> +       int                             samp_freq_avail[3][2];
>> +       enum iio_available_type         db3_freq_avail_type;
>> +       int                             db3_freq_avail_len;
>> +       int                             db3_freq_avail[3][2];
> 
> These 3:s can be defined?
>
I could define IIO_AVAIL_RANGE_LEN and IIO_AVAIL_SINGLE_LEN and then
define another IIO_AVAIL_LEN that is the max between the two.
But that's just over-complicating it, really.

> ...
> 
> 
>> +static int ad4130_get_reg_size(struct ad4130_state *st, unsigned int reg,
>> +                              unsigned int *size)
>> +{
> 
>> +       if (reg >= ARRAY_SIZE(ad4130_reg_size))
>> +               return -EINVAL;
> 
> When this condition is true?

When the user tries reading a register from direct_reg_access
that hasn't had its size defined.

> 
>> +       regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
>> +                          value ? mask : 0);
> 
> One line?
> 
> No error check?
> 

I actually can't think of a scenario where this would fail. It doesn't
if the chip is not even connected.

> 
>> +       if (setup_info->enabled_channels)
>> +               return -EINVAL;
> 
> -EBUSY?
> 

Eh, I don't think so. It would be pretty impossible for the code to hit
this if statement, taking into account the ad4130_find_slot() logic.
I could as well not have it at all.

> 
>> +static void ad4130_freq_to_fs(enum ad4130_filter_mode filter_mode,
>> +                             int val, int val2, unsigned int *fs, bool db3)
>> +{
>> +       const struct ad4130_filter_config *filter_config =
>> +               &ad4130_filter_configs[filter_mode];
>> +       unsigned long long dividend, divisor;
>> +       int temp;
>> +
>> +       dividend = filter_config->fs_max * filter_config->odr_div *
>> +                  (val * AD4130_FREQ_FACTOR + val2);
>> +       divisor = AD4130_MAX_ODR * AD4130_FREQ_FACTOR;
>> +
>> +       if (db3) {
>> +               dividend *= AD4130_DB3_FACTOR;
>> +               divisor *= filter_config->db3_div;
>> +       }
>> +
>> +       temp = AD4130_FS_MIN + filter_config->fs_max -
>> +              DIV64_U64_ROUND_CLOSEST(dividend, divisor);
>> +
>> +       if (temp < AD4130_FS_MIN)
>> +               temp = AD4130_FS_MIN;
>> +       else if (temp > filter_config->fs_max)
>> +               temp = filter_config->fs_max;
>> +
>> +       *fs = temp;
> 
> Would be nice to put a comment explaining the math behind this code.
> 
>> +}
>> +
>> +static void ad4130_fs_to_freq(enum ad4130_filter_mode filter_mode,
>> +                             unsigned int fs, int *val, int *val2, bool db3)
>> +{
>> +       const struct ad4130_filter_config *filter_config =
>> +               &ad4130_filter_configs[filter_mode];
>> +       unsigned int dividend, divisor;
>> +       u64 temp;
>> +
>> +       dividend = (filter_config->fs_max - fs + AD4130_FS_MIN) *
>> +                  AD4130_MAX_ODR;
>> +       divisor = filter_config->fs_max * filter_config->odr_div;
>> +
>> +       if (db3) {
>> +               dividend *= filter_config->db3_div;
>> +               divisor *= AD4130_DB3_FACTOR;
>> +       }
>> +
>> +       temp = div_u64(dividend * AD4130_FREQ_FACTOR, divisor);
>> +       *val = div_u64_rem(temp, AD4130_FREQ_FACTOR, val2);
> 
> 
> Ditto.
> 
I'll see what I can come up with.

> 
>> + out:
> 
> out_unlock: ?
> Ditto for similar cases.

There's a single label in the function, and there's a mutex being
taken, and, logically, the mutex must be released on the exit path.
It's clear what the label is for to me.

> 
>> +               *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
> 
> Hmm... It seems like specific way to have a sign_extended, or actually
> reduced) mask.
> Can you rewrite it with the (potential)UB-free approach?
> 
> (Note, that if realbits == 32, this will have a lot of fun in
> accordance with C standard.)
> 

Can you elaborate on this? The purpose of this statement is to shift the
results so that, when bipolar configuration is enabled, the raw value is
offset with 1 << (realbits - 1) towards negative.

For the 24bit chips, 0x800000 becomes 0x000000.

Maybe you misread it as left shift on a negative number? The number
is turned negative only after the shift...

> ...
> 
>> +               *vals = (int *)st->scale_tbls[setup_info->ref_sel];
> 
> Can we get rid of casting here and in the similar cases?
> 

I feel like scale_tbls is best defined as an array of two-element
arrays. Because its type is IIO_VAL_FRACTIONAL.
But obviously the IIO framework can't take this case into account by
itself, so we cast it so it receives what it wants.

> 
>> +
>> +       if (val > AD4130_FIFO_SIZE)
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * Always set watermark to a multiple of the number of enabled channels
>> +        * to avoid making the FIFO unaligned.
>> +        */
>> +       eff = rounddown(val, st->num_enabled_channels);
>> +
>> +       mutex_lock(&st->lock);
>> +
>> +       ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
>> +                                AD4130_WATERMARK_MASK,
>> +                                FIELD_PREP(AD4130_WATERMARK_MASK,
>> +                                           ad4130_watermark_reg_val(eff)));
> 
> Temporary variable for mask?
> 

You mean for value?

> 
>> +static int ad4130_get_ref_voltage(struct ad4130_state *st,
>> +                                 enum ad4130_ref_sel ref_sel,
>> +                                 unsigned int *ref_uv)
>> +{
>> +       struct device *dev = &st->spi->dev;
>> +       int ret;
>> +
>> +       switch (ref_sel) {
>> +       case AD4130_REF_REFIN1:
>> +               ret = regulator_get_voltage(st->regulators[2].consumer);
>> +               break;
>> +       case AD4130_REF_REFIN2:
>> +               ret = regulator_get_voltage(st->regulators[3].consumer);
>> +               break;
>> +       case AD4130_REF_AVDD_AVSS:
>> +               ret = regulator_get_voltage(st->regulators[0].consumer);
>> +               break;
>> +       case AD4130_REF_REFOUT_AVSS:
> 
>> +               if (!st->int_ref_en) {
>> +                       ret = -EINVAL;
>> +                       break;
>> +               }
>> +
>> +               ret = st->int_ref_uv;
>> +               break;
> 
> Can be one if-else instead.
> 
>> +       default:
>> +               ret = -EINVAL;
>> +               break;
>> +       }
>> +
>> +       if (ret <= 0)
> 
> = 0 ?! Can you elaborate, please, this case taking into account below?
> 

I guess I just did it because voltage = 0 doesn't make sense and would
make scale be 0.0.


>> +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
>> +                                    ref_sel);
>> +
>> +       if (ref_uv)
>> +               *ref_uv = ret;
>> +
>> +       return 0;
>> +}
> 

> 
>> +       fwnode_property_read_u32(child, "adi,excitation-pin-0",
>> +                                &chan_info->iout0);
> 
> No default and/or error check?
> 
> ...
> 
>> +       fwnode_property_read_u32(child, "adi,excitation-pin-1",
>> +                                &chan_info->iout1);
> 
> Ditto.

Default is 0, just like the register defaults.

> 
> ...
> 
>> +static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
>> +{
>> +       struct ad4130_state *st = iio_priv(indio_dev);
>> +       struct device *dev = &st->spi->dev;
>> +       struct fwnode_handle *child;
>> +       int ret;
>> +
>> +       indio_dev->channels = st->chans;
>> +
>> +       device_for_each_child_node(dev, child) {
>> +               ret = ad4130_parse_fw_channel(indio_dev, child);
>> +               if (ret)
>> +                       break;
>> +       }
> 
>> +       fwnode_handle_put(child);
> 
> There is no need to put fwnode if child is NULL. Moreover, the above
> pattern might be percepted wrongly, i.e. one may think that
> fwnode_handle_put() is a must after a loop.
> 

fwnode_handle_put already checks if the child is NULL. Why do the same
check twice?

> 
> Can you explain why regmap locking is needed?
> 

Am I supposed to set .disable_locking = true since SPI has its own
locking?

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-14 11:06     ` Cosmin Tanislav
@ 2022-04-14 13:45       ` Andy Shevchenko
  2022-04-14 14:53         ` Cosmin Tanislav
  0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-14 13:45 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav

On Thu, Apr 14, 2022 at 2:06 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> On 4/13/22 18:41, Andy Shevchenko wrote:
> > On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:

...

> >> +#define AD4130_8_NAME                  "ad4130-8"
> >
> > What the meaning of -8 ? Is it number of channels? Or is it part of
> > the official model (part number)? Can we see, btw, Datasheet: tag with
> > a corresponding link in the commit message?
>
> That's just the name specified in the datasheet. I honestly don't have
> much of an idea about why it is like that. Also, I already put the
> datasheet in the .yaml documentation.

That's cool!

> Do you really also want it
> in each commit message too?

No, just in this one.

...

> >> +#define AD4130_RESET_CLK_COUNT         64
> >> +#define AD4130_RESET_BUF_SIZE          (AD4130_RESET_CLK_COUNT / 8)
> >
> > To be more precise shouldn't the above need to have DIV_ROUND_UP() ?
>
> Does it look like 64 / 8 needs any rounding?

Currently no, but if someone puts 63 there or 65, what would be the outcome?
OTOH, you may add a static assert to guarantee that CLK_COUNT is multiple of 8.

...

> >> +#define AD4130_FREQ_FACTOR             1000000000ull
> >> +#define AD4130_DB3_FACTOR              1000
> >
> > Ditto.
>
> AD4130_DB3_FACTOR is unit-less. In the datasheet, the relation between
> sampling frequency and 3db frequency is represented as a 0.xyz value,
> hence why the db3_div values and 1000 factor.

But does the above mean MILLI or KILO? Similar for the FREQ factor.

...

> >> +       int                             samp_freq_avail_len;
> >> +       int                             samp_freq_avail[3][2];

> >> +       int                             db3_freq_avail_len;
> >> +       int                             db3_freq_avail[3][2];
> >
> > These 3:s can be defined?
> >
> I could define IIO_AVAIL_RANGE_LEN and IIO_AVAIL_SINGLE_LEN and then
> define another IIO_AVAIL_LEN that is the max between the two.
> But that's just over-complicating it, really.

I was talking only about 3:s (out array). IIRC I saw 3 hard coded in
the driver, but not sure if its meaning is the same. Might be still
good to define.

...

> >> +       if (reg >= ARRAY_SIZE(ad4130_reg_size))
> >> +               return -EINVAL;
> >
> > When this condition is true?
>
> When the user tries reading a register from direct_reg_access
> that hasn't had its size defined.

But how is it possible? Is the reg parameter taken directly from the user?

...

> >> +       regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
> >> +                          value ? mask : 0);
> >
> > One line?
> >
> > No error check?
>
> I actually can't think of a scenario where this would fail. It doesn't
> if the chip is not even connected.

Why to check errors in many other cases then? Be consistent one way or
the other.

...

> >> +       if (setup_info->enabled_channels)
> >> +               return -EINVAL;
> >
> > -EBUSY?
> >
>
> Eh, I don't think so. It would be pretty impossible for the code to hit
> this if statement, taking into account the ad4130_find_slot() logic.
> I could as well not have it at all.

If it's a dead code, we do not want it.

...

> >> + out:
> >
> > out_unlock: ?
> > Ditto for similar cases.
>
> There's a single label in the function, and there's a mutex being
> taken, and, logically, the mutex must be released on the exit path.
> It's clear what the label is for to me.

Wasn't clear to me until I went to the end of each of them (who
guarantees that's the case for all of them?).

...

> >> +               *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
> >
> > Hmm... It seems like specific way to have a sign_extended, or actually
> > reduced) mask.
> > Can you rewrite it with the (potential)UB-free approach?
> >
> > (Note, that if realbits == 32, this will have a lot of fun in
> > accordance with C standard.)
>
> Can you elaborate on this? The purpose of this statement is to shift the
> results so that, when bipolar configuration is enabled, the raw value is
> offset with 1 << (realbits - 1) towards negative.
>
> For the 24bit chips, 0x800000 becomes 0x000000.
>
> Maybe you misread it as left shift on a negative number? The number
> is turned negative only after the shift...

1 << 31 is UB in accordance with the C standard.

And the magic above seems to me the opposite to what sign_extend()
does. Maybe even providing a general function for sign_comact() or so
(you name it) would be also nice to have.

...

> >> +       ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> >> +                                AD4130_WATERMARK_MASK,
> >> +                                FIELD_PREP(AD4130_WATERMARK_MASK,
> >> +                                           ad4130_watermark_reg_val(eff)));
> >
> > Temporary variable for mask?
>
> You mean for value?

      mask = AD4130_WATERMARK_MASK;

      ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
                               mask, FIELD_PREP(mask,
ad4130_watermark_reg_val(eff)));

...

> >> +       if (ret <= 0)
> >
> > = 0 ?! Can you elaborate, please, this case taking into account below?
> >
>
> I guess I just did it because voltage = 0 doesn't make sense and would
> make scale be 0.0.

Again, what's the meaning of having it in the conjunction with
dev_err_probe() call?

> >> +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> >> +                                    ref_sel);

It's confusing. I believe you need two different messages if you want
to handle the 0 case.

...

> >> +static int ad4130_parse_fw_children(struct iio_dev *indio_dev)
> >> +{
> >> +       struct ad4130_state *st = iio_priv(indio_dev);
> >> +       struct device *dev = &st->spi->dev;
> >> +       struct fwnode_handle *child;
> >> +       int ret;
> >> +
> >> +       indio_dev->channels = st->chans;
> >> +
> >> +       device_for_each_child_node(dev, child) {
> >> +               ret = ad4130_parse_fw_channel(indio_dev, child);
> >> +               if (ret)
> >> +                       break;
> >> +       }
> >
> >> +       fwnode_handle_put(child);
> >
> > There is no need to put fwnode if child is NULL. Moreover, the above
> > pattern might be percepted wrongly, i.e. one may think that
> > fwnode_handle_put() is a must after a loop.
> >
>
> fwnode_handle_put already checks if the child is NULL. Why do the same
> check twice?

Exactly my point. Why do you check it twice?

...

> > Can you explain why regmap locking is needed?

> Am I supposed to set .disable_locking = true since SPI has its own
> locking?

You tell me. I have no idea of what the locking schema is being used
in your code.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-14 13:45       ` Andy Shevchenko
@ 2022-04-14 14:53         ` Cosmin Tanislav
  2022-04-14 15:37           ` Andy Shevchenko
  0 siblings, 1 reply; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-14 14:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav



On 4/14/22 16:45, Andy Shevchenko wrote:
> On Thu, Apr 14, 2022 at 2:06 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>> On 4/13/22 18:41, Andy Shevchenko wrote:
>>> On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>>>> +#define AD4130_RESET_CLK_COUNT         64
>>>> +#define AD4130_RESET_BUF_SIZE          (AD4130_RESET_CLK_COUNT / 8)
>>>
>>> To be more precise shouldn't the above need to have DIV_ROUND_UP() ?
>>
>> Does it look like 64 / 8 needs any rounding?
> 
> Currently no, but if someone puts 63 there or 65, what would be the outcome?
> OTOH, you may add a static assert to guarantee that CLK_COUNT is multiple of 8.
> 

No one will. 64 is defined in the datasheet and will never change. I'm
not gonna do anything about it. Actually, I can do something about it.
Remove AD4130_RESET_CLK_COUNT and only define AD4130_RESET_BUF_SIZE as
8.

>>>> +       int                             samp_freq_avail_len;
>>>> +       int                             samp_freq_avail[3][2];
> 
>>>> +       int                             db3_freq_avail_len;
>>>> +       int                             db3_freq_avail[3][2];
>>>
>>> These 3:s can be defined?
>>>
>> I could define IIO_AVAIL_RANGE_LEN and IIO_AVAIL_SINGLE_LEN and then
>> define another IIO_AVAIL_LEN that is the max between the two.
>> But that's just over-complicating it, really.
> 
> I was talking only about 3:s (out array). IIRC I saw 3 hard coded in
> the driver, but not sure if its meaning is the same. Might be still
> good to define
Actually I just checked, and it's not even needed. The framework
always expects 3 elements for IIO_AVAIL_RANGE. I'll keep those two
3s as they are.

>>>> +       if (reg >= ARRAY_SIZE(ad4130_reg_size))
>>>> +               return -EINVAL;
>>>
>>> When this condition is true?
>>
>> When the user tries reading a register from direct_reg_access
>> that hasn't had its size defined.
> 
> But how is it possible? Is the reg parameter taken directly from the user?
> 

Users can write whatever they want to direct_reg_access. Unless I add
max_register to the regmap_config, the register that the user selects
will just be passed to our reg_read and reg_write callbacks.

Then it will be checked against the register size table.

>>>> +       regmap_update_bits(st->regmap, AD4130_REG_IO_CONTROL, mask,
>>>> +                          value ? mask : 0);
>>>
>>> One line?
>>>
>>> No error check?
>>
>> I actually can't think of a scenario where this would fail. It doesn't
>> if the chip is not even connected.
> 
> Why to check errors in many other cases then? Be consistent one way or
> the other.
> 

Yeah, right. I didn't add any error checking because the callback can't
handle errors, so all I can do is print a message to dmesg.

>>>> + out:
>>>
>>> out_unlock: ?
>>> Ditto for similar cases.
>>
>> There's a single label in the function, and there's a mutex being
>> taken, and, logically, the mutex must be released on the exit path.
>> It's clear what the label is for to me.
> 
> Wasn't clear to me until I went to the end of each of them (who
> guarantees that's the case for all of them?).
> 

Let's hope other people looking at that code will be able to figure out
what that label does then.

>>>> +               *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
>>>
>>> Hmm... It seems like specific way to have a sign_extended, or actually
>>> reduced) mask.
>>> Can you rewrite it with the (potential)UB-free approach?
>>>
>>> (Note, that if realbits == 32, this will have a lot of fun in
>>> accordance with C standard.)
>>
>> Can you elaborate on this? The purpose of this statement is to shift the
>> results so that, when bipolar configuration is enabled, the raw value is
>> offset with 1 << (realbits - 1) towards negative.
>>
>> For the 24bit chips, 0x800000 becomes 0x000000.
>>
>> Maybe you misread it as left shift on a negative number? The number
>> is turned negative only after the shift...
> 
> 1 << 31 is UB in accordance with the C standard.
> 
> And the magic above seems to me the opposite to what sign_extend()
> does. Maybe even providing a general function for sign_comact() or so
> (you name it) would be also nice to have.
> 

I'm not trying to comact (I guess you meant compact) the sign of any
value. Please try to understand what is written in there. It's not
magic. If the chip is 24bit, and it's set up as bipolar, the raw value
must be offset by -0x800000, to account for 0x800000 being the
zero-scale value. For 16 bits, it's 0x8000.

>>>> +       ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
>>>> +                                AD4130_WATERMARK_MASK,
>>>> +                                FIELD_PREP(AD4130_WATERMARK_MASK,
>>>> +                                           ad4130_watermark_reg_val(eff)));
>>>
>>> Temporary variable for mask?
>>
>> You mean for value?
> 
>        mask = AD4130_WATERMARK_MASK;
> 
>        ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
>                                 mask, FIELD_PREP(mask,
> ad4130_watermark_reg_val(eff)));
> 

Please bother reading the macro definition next-time. The mask argument
to FIELD_PREP must be a compile-time constant.

>>>> +       if (ret <= 0)
>>>
>>> = 0 ?! Can you elaborate, please, this case taking into account below?
>>>
>>
>> I guess I just did it because voltage = 0 doesn't make sense and would
>> make scale be 0.0.
> 
> Again, what's the meaning of having it in the conjunction with
> dev_err_probe() call?
> 
>>>> +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
>>>> +                                    ref_sel);
> 
> It's confusing. I believe you need two different messages if you want
> to handle the 0 case.
> 

Why would I? The chip can't possibly use regulators with a voltage of 0,
right? Or dummy regulators, since these return negative. I think it's
fine as it is.

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-14 14:53         ` Cosmin Tanislav
@ 2022-04-14 15:37           ` Andy Shevchenko
  2022-04-16 15:07             ` Jonathan Cameron
  0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2022-04-14 15:37 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Jonathan Cameron, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav

On Thu, Apr 14, 2022 at 5:53 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> On 4/14/22 16:45, Andy Shevchenko wrote:
> > On Thu, Apr 14, 2022 at 2:06 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >> On 4/13/22 18:41, Andy Shevchenko wrote:
> >>> On Wed, Apr 13, 2022 at 1:41 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:

...

> >>>> +#define AD4130_RESET_CLK_COUNT         64
> >>>> +#define AD4130_RESET_BUF_SIZE          (AD4130_RESET_CLK_COUNT / 8)
> >>>
> >>> To be more precise shouldn't the above need to have DIV_ROUND_UP() ?
> >>
> >> Does it look like 64 / 8 needs any rounding?
> >
> > Currently no, but if someone puts 63 there or 65, what would be the outcome?
> > OTOH, you may add a static assert to guarantee that CLK_COUNT is multiple of 8.
> >
>
> No one will. 64 is defined in the datasheet and will never change. I'm
> not gonna do anything about it. Actually, I can do something about it.
> Remove AD4130_RESET_CLK_COUNT and only define AD4130_RESET_BUF_SIZE as
> 8.

It would be better, indeed. And to whom it may concern you may add a
comment explaining how 8 is derived.

...

> >>>> +       if (reg >= ARRAY_SIZE(ad4130_reg_size))
> >>>> +               return -EINVAL;
> >>>
> >>> When this condition is true?
> >>
> >> When the user tries reading a register from direct_reg_access
> >> that hasn't had its size defined.
> >
> > But how is it possible? Is the reg parameter taken directly from the user?
> >
>
> Users can write whatever they want to direct_reg_access. Unless I add
> max_register to the regmap_config, the register that the user selects
> will just be passed to our reg_read and reg_write callbacks.
>
> Then it will be checked against the register size table.

Thanks, I got it.

...

> >>>> + out:
> >>>
> >>> out_unlock: ?
> >>> Ditto for similar cases.
> >>
> >> There's a single label in the function, and there's a mutex being
> >> taken, and, logically, the mutex must be released on the exit path.
> >> It's clear what the label is for to me.
> >
> > Wasn't clear to me until I went to the end of each of them (who
> > guarantees that's the case for all of them?).
>
> Let's hope other people looking at that code will be able to figure out
> what that label does then.

OK. Let the maintainer decide.

...

> >>>> +               *val = st->bipolar ? -(1 << (chan->scan_type.realbits - 1)) : 0;
> >>>
> >>> Hmm... It seems like specific way to have a sign_extended, or actually
> >>> reduced) mask.
> >>> Can you rewrite it with the (potential)UB-free approach?
> >>>
> >>> (Note, that if realbits == 32, this will have a lot of fun in
> >>> accordance with C standard.)
> >>
> >> Can you elaborate on this? The purpose of this statement is to shift the
> >> results so that, when bipolar configuration is enabled, the raw value is
> >> offset with 1 << (realbits - 1) towards negative.
> >>
> >> For the 24bit chips, 0x800000 becomes 0x000000.
> >>
> >> Maybe you misread it as left shift on a negative number? The number
> >> is turned negative only after the shift...
> >
> > 1 << 31 is UB in accordance with the C standard.
> >
> > And the magic above seems to me the opposite to what sign_extend()
> > does. Maybe even providing a general function for sign_comact() or so
> > (you name it) would be also nice to have.
>
> I'm not trying to comact (I guess you meant compact) the sign of any
> value. Please try to understand what is written in there. It's not
> magic. If the chip is 24bit, and it's set up as bipolar, the raw value
> must be offset by -0x800000, to account for 0x800000 being the
> zero-scale value. For 16 bits, it's 0x8000.

Yes, you shift zero offset to some value. I see that in several
drivers, so I think it would be nice to have a macro for that
somewhere in math.h. But it can be done later on.

...

> >>>> +       ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> >>>> +                                AD4130_WATERMARK_MASK,
> >>>> +                                FIELD_PREP(AD4130_WATERMARK_MASK,
> >>>> +                                           ad4130_watermark_reg_val(eff)));
> >>>
> >>> Temporary variable for mask?
> >>
> >> You mean for value?
> >
> >        mask = AD4130_WATERMARK_MASK;
> >
> >        ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> >                                 mask, FIELD_PREP(mask,
> > ad4130_watermark_reg_val(eff)));
>
> Please bother reading the macro definition next-time. The mask argument
> to FIELD_PREP must be a compile-time constant.

Yes, it needs to be u32_encode_bits(), but in any case it's up to you,
it's not anyhow a critical change.

...

> >>>> +       if (ret <= 0)
> >>>
> >>> = 0 ?! Can you elaborate, please, this case taking into account below?
> >>>
> >>
> >> I guess I just did it because voltage = 0 doesn't make sense and would
> >> make scale be 0.0.
> >
> > Again, what's the meaning of having it in the conjunction with
> > dev_err_probe() call?
> >
> >>>> +               return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> >>>> +                                    ref_sel);
> >
> > It's confusing. I believe you need two different messages if you want
> > to handle the 0 case.
>
> Why would I? The chip can't possibly use regulators with a voltage of 0,
> right? Or dummy regulators, since these return negative. I think it's
> fine as it is.

Confusing part is what dev_err_probe() prints here when ret == 0.


--
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130
  2022-04-13  9:40 [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Cosmin Tanislav
                   ` (3 preceding siblings ...)
  2022-04-13 14:50 ` Andy Shevchenko
@ 2022-04-16 15:00 ` Jonathan Cameron
  2022-04-17 10:16   ` Cosmin Tanislav
  4 siblings, 1 reply; 20+ messages in thread
From: Jonathan Cameron @ 2022-04-16 15:00 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav

On Wed, 13 Apr 2022 12:40:09 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
> 
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.

Hi Cosmin,

Please wrap at around 70-75 chars for patch descriptions.  That
leaves a bit of room for indenting due to automated tooling
/ email threads before we hit 80 chars.  Definitely don't need
30 chars of room for it!

It seems you hit a lot of things that Rob's bot had found that
you would have seen on doing a build test.  Please make sure
you do one in future to save time!

Please use a cover letter for a series like this. It provides several
advantages:

1) Somewhere to add a brief introduction to the whole series.
2) Place for people to give tags for whole series that the b4 tool
   I use to pick up patches can then automatically find them from.
3) Gives series a nice unified name in patchwork ;)
https://patchwork.kernel.org/project/linux-iio/list/?series=631830

> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
>  1 file changed, 255 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> new file mode 100644
> index 000000000000..e9dce54e9802
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> @@ -0,0 +1,255 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4130 ADC device driver
> +
> +maintainers:
> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4130-8-16-lfcsp
> +      - adi,ad4130-8-16-wlcsp
> +      - adi,ad4130-8-24-lfcsp
> +      - adi,ad4130-8-24-wlcsp

What are the variants?   They look to possibly be package differences?
+ resolution differences.
They definitely need some description here.
It may make more sense to have one compatible and then some extra
booleans to say what it supported.

Long shot, but do the different packages have different model IDs?
The datasheet says
Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and
are read only.  If there is one for each of these models then it would be
better to have a single compatible and do the detection of variant in
the driver.

I'm not immediately spotting the resolution information in the data sheet.
It is marked Preliminary but if there are details missing, please mention
in cover letter so we don't go looking for information that doesn't exist.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +    description: phandle to the master clock (mclk)
> +
> +  clock-names:
> +    items:
> +      - const: mclk
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 1
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 1
> +    description:
> +      Default if not supplied is dout-int.
> +    items:
> +      enum:
> +        - dout-int
> +        - clk
> +        - p1

This is unusual.  It is probably helpful to add more description to
explain that the data ready/ fifo interrupt can be routed to any of these
pins.

> +        - dout
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  refin1-supply:
> +    description: refin1 supply. Can be used as reference for conversion.
> +
> +  refin2-supply:
> +    description: refin2 supply. Can be used as reference for conversion.
> +
> +  avdd-supply:
> +    description: AVDD voltage supply. Can be used as reference for conversion.

Whilst these are optional in theory, you should call out that they must be
provided if any of the channels use them as a reference.

> +
> +  iovdd-supply:
> +    description: IOVDD voltage supply. Used for the chip interface.
> +
> +  spi-max-frequency:
> +    maximum: 5000000
> +
> +  adi,mclk-sel:
> +    description: |
> +      Select the clock.
> +      0: Internal 76.8kHz clock.
> +      1: Internal 76.8kHz clock, output to the CLK pin.
> +      2: External 76.8kHz clock.
> +      3. External 153.6kHz clock.

For the external clocks, can we use the fact that one is supplied
as enough to tell us we should be using them?  Then query the
frequency directly from that clock?

If no clock provided then clearly internal.  All that is
necessary after that is a boolean to control if the CLK output
is enabled or not (and ideally constrain that to only be possible
if in internal clock mode).

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +    default: 0
> +
> +  adi,int-ref-en:

Mentioned below...

> +    description: |
> +      Specify if internal reference should be enabled.
> +    type: boolean
> +    default: true
> +
> +  adi,bipolar:
> +    description: Specify if the device should be used in bipolar mode.
> +    type: boolean
> +    default: false
> +
> +  adi,vbias-pins:
> +    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
> +    items:
> +      minimum: 0
> +      maximum: 15

If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines.
So why use a single value rather than either a list of pins, or a bitmap?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +    $ref: adc.yaml
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number.
> +        items:
> +          minimum: 0
> +          maximum: 15
> +
> +      diff-channels:
> +        description: |
> +          Besides the analog inputs available, internal inputs can be used.
> +          16: Internal temperature sensor.
> +          17: AVss
> +          18: Internal reference.
> +          19: DGND.
> +          20: (AVDD − AVSS)/6+
> +          21: (AVDD − AVSS)/6-
> +          22: (IOVDD − DGND)/6+
> +          23: (IOVDD − DGND)/6-
> +          24: (ALDO − AVSS)/6+
> +          25: (ALDO − AVSS)/6-
> +          26: (DLDO − DGND)/6+
> +          27: (DLDO − DGND)/6-
> +          28: V_MV_P
> +          29: V_MV_M
> +        $ref: adc.yaml
> +        items:
> +          minimum: 0
> +          maximum: 29

Interesting. So we have a part that has a 16 channel sequencer, but
can you have more channels as long as you don't want them all at once?
For example, I doubt anyone wants to permanently configure a device to monitor
the various supplies, but they will want to occasionally.

As such, perhaps we need to treat this device more flexibly?
There are obviously contraints on what channels + references make sense
but maybe we should allow more than 16 to be specified?

> +
> +      adi,reference-select:
> +        description: |
> +          Select the reference source to use when converting on the
> +          specific channel. Valid values are:
> +          0: REFIN1(+)/REFIN1(−).
> +          1: REFIN2(+)/REFIN2(−).
> +          2: REFOUT/AVSS (Internal reference)
> +          3: AVDD/AVSS
> +          If not specified, internal reference is used.

That's not a good idea if it might be turned off...
Perhaps a better approach would be to drop the int_ref_en and
simply walk the channels to find out if any of them use it.
If they don't turn it off, otherwise leave it on.

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3]
> +        default: 2
> +
> +      adi,excitation-pin-0:
> +        description: |
> +          Analog input to apply excitation current to while the channel
> +          is active.
> +        minimum: 0
> +        maximum: 15
> +        default: 0
> +
> +      adi,excitation-pin-1:
> +        description: |
> +          Analog input to apply excitation current to while this channel
> +          is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 15
> +        default: 0
> +
> +      adi,excitation-current-0-nanoamps:
> +        description: |
> +          Excitation current in nanoamps to be applied to pin specified in
> +          adi,excitation-pin-0 while this channel is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
> +        default: 0
> +
> +      adi,excitation-current-1-nanoamps:
> +        description: |
> +          Excitation current in nanoamps to be applied to pin specified in
> +          adi,excitation-pin-1 while this channel is active.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
> +        default: 0
> +
> +      adi,burnout-current-nanoamps:
> +        description: |
> +          Burnout current in nanoamps to be applied for this channel.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 500, 2000, 4000]
> +        default: 0
> +
> +      adi,buffered-positive:
> +        description: Enable buffered mode for positive input.
> +        type: boolean
> +
> +      adi,buffered-negative:
> +        description: Enable buffered mode for negative input.
> +        type: boolean
> +
> +    required:
> +      - reg
> +      - diff-channels
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +        compatible = "adi,ad4130-8-24-wlcsp";
> +        reg = <0>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        spi-max-frequency = <5000000>;
> +        interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
> +        interrupt-parent = <&gpio>;
> +
> +        channel@0 {
> +          reg = <0>;
> +          /* AIN8, AIN9 */
> +          diff-channels = <8 9>;
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          /* AIN10, AIN11 */
> +          diff-channels = <10 11>;
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          /* Temperature Sensor, DGND */
> +          diff-channels = <16 19>;
> +        };
> +
> +        channel@3 {
> +          reg = <3>;
> +          /* Internal reference, DGND */
> +          diff-channels = <18 19>;
> +        };
> +
> +        channel@4 {
> +          reg = <4>;
> +          /* DGND, DGND */
> +          diff-channels = <19 19>;
> +        };
> +      };
> +    };


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

* Re: [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available}
  2022-04-13  9:40 ` [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available} Cosmin Tanislav
  2022-04-13 14:51   ` Andy Shevchenko
@ 2022-04-16 15:02   ` Jonathan Cameron
  1 sibling, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2022-04-16 15:02 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav

On Wed, 13 Apr 2022 12:40:10 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
> 
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ad4130      | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
> new file mode 100644
> index 000000000000..942150991e75
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
> @@ -0,0 +1,36 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available
> +KernelVersion:

Make a guess at the kernel version (hopefully this cycle!)
We never remember to fill these in later so better to have it slightly wrong
than not present at all.

> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading returns a list with the possible filter modes.
> +		"sinc4"       - Sinc 4. Excellent noise performance. Long 1st
> +				conversion time. No natural 50/60Hz rejection.
> +		"sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion time.
> +		"sinc3"	      - Sinc3. Moderate 1st conversion time. Good noise
> +				performance.
> +		"sinc3+rej60" - Sinc3 + 60Hz rejection. At a sampling frequency
> +				of 50Hz, achieves simultaneous 50Hz and 60Hz
> +				rejection.
> +		"sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion time.
> +				Best used with a sampling frequency of at least
> +				216.19Hz.
> +		"sinc3+pf1"   - Sinc3 + Post Filter 1.
> +				53dB rejection @ 50Hz, 58dB rejection @ 60Hz.
> +		"sinc3+pf2"   - Sinc3 + Post Filter 2.
> +				70dB rejection @ 50Hz, 70dB rejection @ 60Hz.
> +		"sinc3+pf3"   - Sinc3 + Post Filter 3.
> +				99dB rejection @ 50Hz, 103dB rejection @ 60Hz.
> +		"sinc3+pf4"   - Sinc3 + Post Filter 4.
> +				103dB rejection @ 50Hz, 109dB rejection @ 60Hz.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY-voltageZ_filter_mode
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Set the filter mode of the differential channel. When the filter
> +		mode changes, the in_voltageY-voltageZ_sampling_frequency and
> +		in_voltageY-voltageZ_sampling_frequency_available attributes
> +		might also change to accomodate the new filter mode.
> +		If the current sampling frequency is out of range for the new
> +		filter mode, the sampling frequency will be changed to the
> +		closest valid one.


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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-14 15:37           ` Andy Shevchenko
@ 2022-04-16 15:07             ` Jonathan Cameron
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2022-04-16 15:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Cosmin Tanislav, Rob Herring, Linus Walleij, linux-iio,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, devicetree,
	Cosmin Tanislav

> ...
> 
> > >>>> + out:  
> > >>>
> > >>> out_unlock: ?
> > >>> Ditto for similar cases.  
> > >>
> > >> There's a single label in the function, and there's a mutex being
> > >> taken, and, logically, the mutex must be released on the exit path.
> > >> It's clear what the label is for to me.  
> > >
> > > Wasn't clear to me until I went to the end of each of them (who
> > > guarantees that's the case for all of them?).  
> >
> > Let's hope other people looking at that code will be able to figure out
> > what that label does then.  
> 
> OK. Let the maintainer decide.

Slight preference for giving the extra info of out_unlock, but not critical.
(note I haven't read the code yet so might change my mind :)

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-13  9:40 ` [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver Cosmin Tanislav
  2022-04-13 15:41   ` Andy Shevchenko
@ 2022-04-16 16:21   ` Jonathan Cameron
  2022-04-17 10:26     ` Cosmin Tanislav
  1 sibling, 1 reply; 20+ messages in thread
From: Jonathan Cameron @ 2022-04-16 16:21 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav

On Wed, 13 Apr 2022 12:40:11 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> AD4130-8 is an ultra-low power, high precision,
> measurement solution for low bandwidth battery
> operated applications.
> 
> The fully integrated AFE (Analog Front-End)
> includes a multiplexer for up to 16 single-ended
> or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> reference and oscillator, selectable filter
> options, smart sequencer, sensor biasing and
> excitation options, diagnostics, and a FIFO
> buffer.
> 
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>

Hi Cosmin,

I've only glanced at Andy's comments, so may well overlap in places
though I'll try and avoid too much repetition if I happen to remember
Andy commented on something already.

Only a few minor things from me.  For such a complex device this
is looking pretty good for a first version posted.

Jonathan


> ---
>  MAINTAINERS              |    8 +
>  drivers/iio/adc/Kconfig  |   13 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad4130.c | 2072 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 2094 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4130.c
> 

...

> diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
> new file mode 100644
> index 000000000000..89fb9b413ff0
> --- /dev/null
> +++ b/drivers/iio/adc/ad4130.c
> @@ -0,0 +1,2072 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD4130 SPI ADC driver
> + *
> + * Copyright 2022 Analog Devices Inc.
> + */
> +#include <asm/div64.h>
> +#include <asm/unaligned.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define AD4130_8_NAME			"ad4130-8"
> +
> +#define AD4130_COMMS_READ_MASK		BIT(6)
> +
> +#define AD4130_REG_STATUS		0x00
> +#define AD4130_STATUS_POR_FLAG_MASK	BIT(4)
> +
> +#define AD4130_REG_ADC_CONTROL		0x01
> +#define AD4130_BIPOLAR_MASK		BIT(14)
where possibly it is good to name register fields such that it's
obvious which register they are fields of.  Makes it easier
to be sure we have the right one.
(I fell into this trap myself this week and wasted an hour or
so before I figured out that there were two different registers
with fields with exactly the same name ;)

Lots of different conventions for this one and I don't mind
which one you pick. e.g.  This works, but isn't perfect by
any means.

#define AD4130_ADC_CTRL_REG
#define  AD4130_ADC_CTRL_BIPOLAR_MASK

Note I quite like the subtle indenting to make it easier
to read these definitions as well.

> +#define AD4130_INT_REF_VAL_MASK		BIT(13)
> +#define AD4130_INT_REF_2_5V		2500000
> +#define AD4130_INT_REF_1_25V		1250000
> +#define AD4130_CSB_EN_MASK		BIT(9)
> +#define AD4130_INT_REF_EN_MASK		BIT(8)
> +#define AD4130_MODE_MASK		GENMASK(5, 2)
> +#define AD4130_MCLK_SEL_MASK		GENMASK(1, 0)

...


> +
> +#define AD4130_RESET_CLK_COUNT		64
> +#define AD4130_RESET_BUF_SIZE		(AD4130_RESET_CLK_COUNT / 8)
> +
> +#define AD4130_SOFT_RESET_SLEEP		(160 * 1000000 / AD4130_MCLK_FREQ_76_8KHZ)
> +
> +#define AD4130_INVALID_SLOT		-1
> +
> +#define AD4130_FREQ_FACTOR		1000000000ull

Arguably should use the standard define for NANO

> +#define AD4130_DB3_FACTOR		1000
> +

> +
> +struct ad4130_chip_info {
> +	const char	*name;
> +	u8		resolution;
> +	bool		has_int_pin;
> +};
> +
> +struct ad4130_setup_info {
> +	unsigned int			iout0_val;
> +	unsigned int			iout1_val;
> +	unsigned int			burnout;
> +	unsigned int			pga;
> +	unsigned int			fs;
> +	bool				ref_bufp;
> +	bool				ref_bufm;
> +	u32				ref_sel;
> +	enum ad4130_filter_mode		filter_mode;
> +	unsigned int			enabled_channels;
> +	unsigned int			channels;
> +};
> +
> +#define AD4130_SETUP_SIZE		offsetof(struct ad4130_setup_info, \
> +						 enabled_channels)

Perhaps a comment on what this is?  Or define this as one structure
containing another so that you can have the meta data alongside the
actual stuff you want to be able to compare.

Or just rename it as AD4130_SETUP_MATCH_SIZE or something along
those lines.

...

> +struct ad4130_state {
> +	const struct ad4130_chip_info	*chip_info;
> +	struct spi_device		*spi;
> +	struct regmap			*regmap;
> +	struct clk			*mclk;
> +	struct regulator_bulk_data	regulators[4];
> +	u32				irq_trigger;
> +	u32				inv_irq_trigger;
> +
> +	/*
> +	 * Synchronize access to members of driver state, and ensure atomicity
> +	 * of consecutive regmap operations.
> +	 */
> +	struct mutex			lock;
> +	struct completion		completion;
> +
> +	struct iio_chan_spec		chans[AD4130_MAX_CHANNELS];
> +	struct ad4130_chan_info		chans_info[AD4130_MAX_CHANNELS];
> +	struct ad4130_setup_info	setups_info[AD4130_MAX_SETUPS];
> +	enum ad4130_pin_function	pins_fn[AD4130_MAX_ANALOG_PINS];
> +	u32				vbias_pins[AD4130_MAX_ANALOG_PINS];
> +	u32				num_vbias_pins;
> +	int				scale_tbls[AD4130_REF_SEL_MAX]
> +						  [AD4130_PGA_NUM][2];
> +	struct gpio_chip		gc;
> +	unsigned int			gpio_offsets[AD4130_MAX_GPIOS];
> +	unsigned int			num_gpios;
> +
> +	u32			int_pin_sel;
> +	bool			int_ref_en;
> +	u32			int_ref_uv;
> +	u32			mclk_sel;
> +	bool			bipolar;
> +
> +	unsigned int		num_enabled_channels;
> +	unsigned int		effective_watermark;
> +	unsigned int		watermark;
> +
> +	struct spi_message	fifo_msg;
> +	struct spi_transfer	fifo_xfer[2];
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	u8			reset_buf[AD4130_RESET_BUF_SIZE] ____cacheline_aligned;
> +	u8			reg_write_tx_buf[4];
> +	u8			reg_read_tx_buf[1];
> +	u8			reg_read_rx_buf[3];
> +	u8			fifo_tx_buf[2];
> +	u8			fifo_rx_buf[AD4130_FIFO_SIZE *
> +					    AD4130_FIFO_MAX_SAMPLE_SIZE];

This is quite a large buffer.  Perhaps it would be better to drain the fifo
in multiple steps if it is very full?  I guess that could be added
later if anyone ever ran into a problem with the buffer size.


> +};

> +
> +static const struct iio_info ad4130_info = {
> +	.read_raw = ad4130_read_raw,
> +	.read_avail = ad4130_read_avail,
> +	.write_raw_get_fmt = ad4130_write_raw_get_fmt,
> +	.write_raw = ad4130_write_raw,
> +	.update_scan_mode = ad4130_update_scan_mode,
> +	.hwfifo_set_watermark = ad4130_set_fifo_watermark,
> +	.debugfs_reg_access = ad4130_reg_access,
> +};
> +
> +static int ad4130_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4130_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = ad4130_set_watermark_interrupt_en(st, true);
> +	if (ret)
> +		goto out;
> +
> +	/* When the chip enters FIFO mode, IRQ polarity is inversed. */

That is downright odd :)  Perhaps a datasheet section reference is
appropriate here.

> +	ret = irq_set_irq_type(st->spi->irq, st->inv_irq_trigger);
> +	if (ret)
> +		goto out;
> +
> +	ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_WATERMARK);
> +	if (ret)
> +		goto out;
> +
> +	ret = ad4130_set_mode(st, AD4130_MODE_CONTINUOUS);
> +
> +out:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int ad4130_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4130_state *st = iio_priv(indio_dev);
> +	unsigned int i;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = ad4130_set_mode(st, AD4130_MODE_IDLE);
> +	if (ret)
> +		goto out;
> +
> +	/* When the chip exits FIFO mode, IRQ polarity returns to normal. */
> +	ret = irq_set_irq_type(st->spi->irq, st->irq_trigger);
> +	if (ret)
> +		goto out;
> +
> +	ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_DISABLED);
> +	if (ret)
> +		goto out;
> +
> +	ret = ad4130_set_watermark_interrupt_en(st, false);
> +	if (ret)
> +		goto out;
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
Comment here on why we do this in predisable and not the equivalent in
postenable.  (I assume because we don't call update_scan_mode in
the disable path).

> +		ret = ad4130_set_channel_enable(st, i, false);
> +		if (ret)
> +			goto out;
> +	}
> +
> +out:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops ad4130_buffer_ops = {
> +	.postenable = ad4130_buffer_postenable,
> +	.predisable = ad4130_buffer_predisable,
> +};
> +

...

> +
> +static int find_table_index(const unsigned int *tbl, size_t len,
> +			    unsigned int val)

This is a generic enough name you may well find you have
ended up clashing with something added in a header in future.
So I'd prefix this with the part number.

> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < len; i++)
> +		if (tbl[i] == val)
> +			return i;
> +
> +	return -EINVAL;
> +}
> +
> +static int ad4130_get_ref_voltage(struct ad4130_state *st,
> +				  enum ad4130_ref_sel ref_sel,
> +				  unsigned int *ref_uv)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret;
> +
> +	switch (ref_sel) {
> +	case AD4130_REF_REFIN1:
> +		ret = regulator_get_voltage(st->regulators[2].consumer);
> +		break;
> +	case AD4130_REF_REFIN2:
> +		ret = regulator_get_voltage(st->regulators[3].consumer);
> +		break;
> +	case AD4130_REF_AVDD_AVSS:
> +		ret = regulator_get_voltage(st->regulators[0].consumer);
> +		break;
> +	case AD4130_REF_REFOUT_AVSS:
> +		if (!st->int_ref_en) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		ret = st->int_ref_uv;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	if (ret <= 0)
> +		return dev_err_probe(dev, ret, "Cannot use reference %u\n",
> +				     ref_sel);
> +
> +	if (ref_uv)
> +		*ref_uv = ret;

I'd probably keep this simpler and have the caller always provide
ref_uv. Use a local dummy variable where it doesn't need the answer
or even better just return the voltage as positive value.

> +
> +	return 0;
> +}

...

> +static int ad4310_parse_fw(struct iio_dev *indio_dev)
> +{
> +	struct ad4130_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	unsigned int i;
> +	int avdd_uv;
> +	int irq;
> +	int ret;
> +
> +	st->mclk = devm_clk_get_optional(dev, "mclk");
> +	if (IS_ERR(st->mclk))
> +		return dev_err_probe(dev, PTR_ERR(st->mclk),
> +				     "Failed to get mclk\n");
> +
> +	st->int_pin_sel = AD4130_INT_PIN_DOUT_OR_INT;
> +
> +	for (i = 0; i < ARRAY_SIZE(ad4130_int_pin_names); i++) {
> +		irq = of_irq_get_byname(dev->of_node, ad4130_int_pin_names[i]);

fwnode version.

> +		if (irq > 0) {
> +			st->int_pin_sel = i;
> +			break;
> +		}
> +	}
> +
> +	if (st->int_pin_sel == AD4130_INT_PIN_DOUT ||
> +	    (st->int_pin_sel == AD4130_INT_PIN_DOUT_OR_INT &&
> +	     !st->chip_info->has_int_pin))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Cannot use DOUT as interrupt pin\n");
> +
> +	if (st->int_pin_sel == AD4130_INT_PIN_P1)
> +		st->pins_fn[AD4130_AIN2_P1] = AD4130_PIN_FN_SPECIAL;

Looking at datasheet I see an option for P2, but not P1?

> +
> +	st->mclk_sel = AD4130_MCLK_76_8KHZ;
> +	device_property_read_u32(dev, "adi,mclk-sel", &st->mclk_sel);
> +
> +	if (st->mclk_sel >= AD4130_MCLK_SEL_MAX)
> +		return dev_err_probe(dev, -EINVAL, "Invalid clock %u\n",
> +				     st->mclk_sel);
> +
> +	if (st->mclk && (st->mclk_sel == AD4130_MCLK_76_8KHZ ||
> +			 st->mclk_sel == AD4130_MCLK_76_8KHZ_OUT))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Cannot use external clock\n");
> +
> +	if (st->int_pin_sel == AD4130_INT_PIN_CLK &&
> +	    st->mclk_sel != AD4130_MCLK_76_8KHZ)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid clock %u for interrupt pin %u\n",
> +				     st->mclk_sel, st->int_pin_sel);
> +
> +	st->int_ref_en = true;
> +	if (device_property_present(dev, "adi,int-ref-en"))
> +		st->int_ref_en = device_property_read_bool(dev, "adi,int-ref-en");
> +
> +	st->int_ref_uv = AD4130_INT_REF_2_5V;
> +
> +	/*
> +	 * When the AVDD supply is set to below 2.5V the internal reference of
> +	 * 1.25V should be selected.

Good to give specific reference to datasheet section for things like this.
Seems to be in the ADC REFERENCE section.

> +	 */
> +	avdd_uv = regulator_get_voltage(st->regulators[0].consumer);
> +	if (avdd_uv > 0 && avdd_uv < AD4130_INT_REF_2_5V)
> +		st->int_ref_uv = AD4130_INT_REF_1_25V;
> +
> +	st->bipolar = device_property_read_bool(dev, "adi,bipolar");
> +
> +	ret = device_property_count_u32(dev, "adi,vbias-pins");
> +	if (ret > 0) {
> +		st->num_vbias_pins = ret;
> +
> +		ret = device_property_read_u32_array(dev, "adi,vbias-pins",
> +						     st->vbias_pins,
> +						     st->num_vbias_pins);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to read vbias pins\n");
> +
> +		ret = ad4130_validate_vbias_pins(st, st->vbias_pins,
> +						 st->num_vbias_pins);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = ad4130_parse_fw_children(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < AD4130_MAX_GPIOS; i++) {
> +		if (st->pins_fn[i + AD4130_AIN2_P1] != AD4130_PIN_FN_NONE)
> +			continue;

I'm a bit confused. pins_fn seems to be for the Analog pins, yet here is being
used for the GPIOs?  Maybe some explanatory comments

> +
> +		st->gpio_offsets[st->num_gpios++] = i;
> +	}
> +
> +	return 0;
> +}

...

> +static int ad4130_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad4130_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +	unsigned int int_ref_val;
> +	unsigned long rate = AD4130_MCLK_FREQ_76_8KHZ;
> +	unsigned int val;
> +	unsigned int i;
> +	int ret;
> +
> +	if (st->mclk_sel == AD4130_MCLK_153_6KHZ_EXT)
> +		rate = AD4130_MCLK_FREQ_153_6KHZ;
> +
> +	ret = clk_set_rate(st->mclk, rate);

Ah. I'd neglected in my review of the dt bindings that we'd
be specifying the clock in here. We will need a parameter
to specify the clock speed if external is used, but I'd still like
that separated from the question of internal vs external clocks.

> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(st->mclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, ad4130_clk_disable_unprepare,
> +				       st->mclk);
> +	if (ret)
> +		return ret;
> +
> +	if (st->int_ref_uv == AD4130_INT_REF_2_5V)
> +		int_ref_val = AD4130_INT_REF_VAL_2_5V;
> +	else
> +		int_ref_val = AD4130_INT_REF_VAL_1_25V;
> +
> +	/* Switch to SPI 4-wire mode. */
> +	val = AD4130_CSB_EN_MASK;
> +	val |= st->bipolar ? AD4130_BIPOLAR_MASK : 0;

Prefer field PREP even for these single bit cases.

> +	val |= st->int_ref_en ? AD4130_INT_REF_EN_MASK : 0;
> +	val |= FIELD_PREP(AD4130_MODE_MASK, AD4130_MODE_IDLE);
> +	val |= FIELD_PREP(AD4130_MCLK_SEL_MASK, st->mclk_sel);
> +	val |= FIELD_PREP(AD4130_INT_REF_VAL_MASK, int_ref_val);
> +
> +	ret = regmap_write(st->regmap, AD4130_REG_ADC_CONTROL, val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(AD4130_INT_PIN_SEL_MASK, st->int_pin_sel);
> +	for (i = 0; i < st->num_gpios; i++)
> +		val |= BIT(st->gpio_offsets[i]);
> +
> +	ret = regmap_write(st->regmap, AD4130_REG_IO_CONTROL, val);
> +	if (ret)
> +		return ret;
> +
> +	val = 0;
> +	for (i = 0; i < st->num_vbias_pins; i++)
> +		val |= BIT(st->vbias_pins[i]);
> +
> +	ret = regmap_write(st->regmap, AD4130_REG_VBIAS, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
> +				 AD4130_ADD_FIFO_HEADER_MASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* FIFO watermark interrupt starts out as enabled, disable it. */
> +	ret = ad4130_set_watermark_interrupt_en(st, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup channels. */
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		struct ad4130_chan_info *chan_info = &st->chans_info[i];
> +		struct iio_chan_spec *chan = &st->chans[i];
> +		unsigned int val;
> +
> +		val = FIELD_PREP(AD4130_AINP_MASK, chan->channel) |
> +		      FIELD_PREP(AD4130_AINM_MASK, chan->channel2) |
> +		      FIELD_PREP(AD4130_IOUT1_MASK, chan_info->iout0) |
> +		      FIELD_PREP(AD4130_IOUT2_MASK, chan_info->iout1);
> +
> +		ret = regmap_write(st->regmap, AD4130_REG_CHANNEL_X(i), val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +

...

> +static int ad4130_probe(struct spi_device *spi)
> +{
> +	const struct ad4130_chip_info *info;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4130_state *st;
> +	int ret;
> +
> +	info = device_get_match_data(dev);
> +	if (!info)
> +		return -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	memset(st->reset_buf, 0xff, AD4130_RESET_BUF_SIZE);
> +	init_completion(&st->completion);
> +	mutex_init(&st->lock);
> +	st->chip_info = info;
> +	st->spi = spi;
> +
> +	/*
> +	 * Xfer:   [ XFR1 ] [         XFR2         ]
> +	 * Master:  0x7D N   ......................
> +	 * Slave:   ......   DATA1 DATA2 ... DATAN
> +	 */
> +	st->fifo_tx_buf[0] = AD4130_COMMS_READ_MASK | AD4130_REG_FIFO_DATA;
> +	st->fifo_xfer[0].tx_buf = st->fifo_tx_buf;
> +	st->fifo_xfer[0].len = sizeof(st->fifo_tx_buf);
> +	st->fifo_xfer[1].rx_buf = st->fifo_rx_buf;
> +	spi_message_init_with_transfers(&st->fifo_msg, st->fifo_xfer,
> +					ARRAY_SIZE(st->fifo_xfer));
> +
> +	indio_dev->name = st->chip_info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad4130_info;
> +
> +	st->regmap = devm_regmap_init(dev, NULL, st,
> +				      &ad4130_regmap_config);

Don't wrap below 80 chars unless there is some extra meaning conveyed
by doing so.  Don't think that's true ehre.


> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	st->regulators[0].supply = "avdd";
> +	st->regulators[1].supply = "iovdd";
> +	st->regulators[2].supply = "refin1";
> +	st->regulators[3].supply = "refin2";
> +
> +	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, ad4130_disable_regulators, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add regulators disable action\n");
> +
> +	ret = ad4130_soft_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4310_parse_fw(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4130_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad4130_fill_scale_tbls(st);
> +
> +	if (st->num_gpios) {
> +		st->gc.owner = THIS_MODULE;
> +		st->gc.label = st->chip_info->name;
> +		st->gc.base = -1;
> +		st->gc.ngpio = AD4130_MAX_GPIOS;
> +		st->gc.parent = dev;
> +		st->gc.can_sleep = true;
> +		st->gc.get_direction = ad4130_gpio_get_direction;
> +		st->gc.set = ad4130_gpio_set;
> +
> +		ret = devm_gpiochip_add_data(dev, &st->gc, st);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
> +					      INDIO_BUFFER_SOFTWARE,
> +					      &ad4130_buffer_ops,
> +					      ad4130_fifo_attributes);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_threaded_irq(dev, spi->irq, NULL,
> +					ad4130_irq_handler, IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> +	st->irq_trigger = irq_get_trigger_type(spi->irq);
> +	if (st->irq_trigger & IRQF_TRIGGER_RISING)
> +		st->inv_irq_trigger = IRQF_TRIGGER_FALLING;
> +	else if (st->irq_trigger & IRQF_TRIGGER_FALLING)
> +		st->inv_irq_trigger = IRQF_TRIGGER_RISING;
> +	else
> +		return dev_err_probe(dev, -EINVAL, "Invalid irq flags: %u\n",
> +				     st->irq_trigger);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +


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

* Re: [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130
  2022-04-16 15:00 ` Jonathan Cameron
@ 2022-04-17 10:16   ` Cosmin Tanislav
  2022-04-24 16:05     ` Jonathan Cameron
  0 siblings, 1 reply; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-17 10:16 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav



On 4/16/22 18:00, Jonathan Cameron wrote:
> On Wed, 13 Apr 2022 12:40:09 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> AD4130-8 is an ultra-low power, high precision,
>> measurement solution for low bandwidth battery
>> operated applications.
>>
>> The fully integrated AFE (Analog Front-End)
>> includes a multiplexer for up to 16 single-ended
>> or 8 differential inputs, PGA (Programmable Gain
>> Amplifier), 24-bit Sigma-Delta ADC, on-chip
>> reference and oscillator, selectable filter
>> options, smart sequencer, sensor biasing and
>> excitation options, diagnostics, and a FIFO
>> buffer.
> 
> Hi Cosmin,
> 
> Please wrap at around 70-75 chars for patch descriptions.  That
> leaves a bit of room for indenting due to automated tooling
> / email threads before we hit 80 chars.  Definitely don't need
> 30 chars of room for it!
> 

Yeah, I'll do it.

> It seems you hit a lot of things that Rob's bot had found that
> you would have seen on doing a build test.  Please make sure
> you do one in future to save time!
> 

Yeah, sorry. I already fixed them for V2.

> Please use a cover letter for a series like this. It provides several
> advantages:
> 
> 1) Somewhere to add a brief introduction to the whole series.
> 2) Place for people to give tags for whole series that the b4 tool
>     I use to pick up patches can then automatically find them from.
> 3) Gives series a nice unified name in patchwork ;)
> https://patchwork.kernel.org/project/linux-iio/list/?series=631830
> 

Sure thing. I'll have it there for V2 anyway.

>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
>> ---
>>   .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
>>   1 file changed, 255 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
>> new file mode 100644
>> index 000000000000..e9dce54e9802
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
>> @@ -0,0 +1,255 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2022 Analog Devices Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices AD4130 ADC device driver
>> +
>> +maintainers:
>> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
>> +
>> +description: |
>> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - adi,ad4130-8-16-lfcsp
>> +      - adi,ad4130-8-16-wlcsp
>> +      - adi,ad4130-8-24-lfcsp
>> +      - adi,ad4130-8-24-wlcsp
> 
> What are the variants?   They look to possibly be package differences?
> + resolution differences.
> They definitely need some description here.
> It may make more sense to have one compatible and then some extra
> booleans to say what it supported.
>

Packaging + available interrupt pins + resolution. Is having extra
booleans to describe what is supported really the best approach?
It's not really about how the hardware is configured anymore, is it?
They're different chips.

> Long shot, but do the different packages have different model IDs?
> The datasheet says
> Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and
> are read only.  If there is one for each of these models then it would be
> better to have a single compatible and do the detection of variant in
> the driver.
> 
> I'm not immediately spotting the resolution information in the data sheet.
> It is marked Preliminary but if there are details missing, please mention
> in cover letter so we don't go looking for information that doesn't exist.
>

I don't have enough information about the other models to know what
Model IDs they will have. That's why I took this approach.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: phandle to the master clock (mclk)
>> +
>> +  clock-names:
>> +    items:
>> +      - const: mclk
>> +
>> +  interrupts:
>> +    minItems: 1
>> +    maxItems: 1
>> +
>> +  interrupt-names:
>> +    minItems: 1
>> +    maxItems: 1
>> +    description:
>> +      Default if not supplied is dout-int.
>> +    items:
>> +      enum:
>> +        - dout-int
>> +        - clk
>> +        - p1
> 
> This is unusual.  It is probably helpful to add more description to
> explain that the data ready/ fifo interrupt can be routed to any of these
> pins.

Is it unusual? ADIS16480 follows a similar approach.

description: |
   Specify which interrupt pin should be configured as Data Ready / FIFO
   interrupt.
   Default if not supplied is dout-int.

How does this sound?

> 
>> +        - dout
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  refin1-supply:
>> +    description: refin1 supply. Can be used as reference for conversion.
>> +
>> +  refin2-supply:
>> +    description: refin2 supply. Can be used as reference for conversion.
>> +
>> +  avdd-supply:
>> +    description: AVDD voltage supply. Can be used as reference for conversion.
> 
> Whilst these are optional in theory, you should call out that they must be
> provided if any of the channels use them as a reference.
> 

I thought that "Can be used as reference for conversion." + it being an
option in adi,reference-select property would make it obvious, no?

>> +
>> +  iovdd-supply:
>> +    description: IOVDD voltage supply. Used for the chip interface.
>> +
>> +  spi-max-frequency:
>> +    maximum: 5000000
>> +
>> +  adi,mclk-sel:
>> +    description: |
>> +      Select the clock.
>> +      0: Internal 76.8kHz clock.
>> +      1: Internal 76.8kHz clock, output to the CLK pin.
>> +      2: External 76.8kHz clock.
>> +      3. External 153.6kHz clock.
> 
> For the external clocks, can we use the fact that one is supplied
> as enough to tell us we should be using them?  Then query the
> frequency directly from that clock?
> 

Aren't we supposed to set the frequency of that clock ourselves,
in the driver?

> If no clock provided then clearly internal.  All that is
> necessary after that is a boolean to control if the CLK output
> is enabled or not (and ideally constrain that to only be possible
> if in internal clock mode).
> 

Well...

So, mclk present => external, not present => internal.
adi,int-clk-out-enable to specify if the internal clock should be
exposed? adi,ext-clk-freq to specify the desired clock speed of the
external clk?

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1, 2, 3]
>> +    default: 0
>> +
>> +  adi,int-ref-en:
> 
> Mentioned below...
> 
>> +    description: |
>> +      Specify if internal reference should be enabled.
>> +    type: boolean
>> +    default: true
>> +
>> +  adi,bipolar:
>> +    description: Specify if the device should be used in bipolar mode.
>> +    type: boolean
>> +    default: false
>> +
>> +  adi,vbias-pins:
>> +    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
>> +    items:
>> +      minimum: 0
>> +      maximum: 15
> 
> If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines.
> So why use a single value rather than either a list of pins, or a bitmap?
> 

Umm. Isn't this a list of pins? That's why everything is plural here.
I guess I should add `maxItems: 16`?
I already added `$ref: /schemas/types.yaml#/definitions/uint32-array`.

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +patternProperties:
>> +  "^channel@([0-9]|1[0-5])$":
>> +    type: object
>> +    $ref: adc.yaml
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          The channel number.
>> +        items:
>> +          minimum: 0
>> +          maximum: 15
>> +
>> +      diff-channels:
>> +        description: |
>> +          Besides the analog inputs available, internal inputs can be used.
>> +          16: Internal temperature sensor.
>> +          17: AVss
>> +          18: Internal reference.
>> +          19: DGND.
>> +          20: (AVDD − AVSS)/6+
>> +          21: (AVDD − AVSS)/6-
>> +          22: (IOVDD − DGND)/6+
>> +          23: (IOVDD − DGND)/6-
>> +          24: (ALDO − AVSS)/6+
>> +          25: (ALDO − AVSS)/6-
>> +          26: (DLDO − DGND)/6+
>> +          27: (DLDO − DGND)/6-
>> +          28: V_MV_P
>> +          29: V_MV_M
>> +        $ref: adc.yaml
>> +        items:
>> +          minimum: 0
>> +          maximum: 29
> 
> Interesting. So we have a part that has a 16 channel sequencer, but
> can you have more channels as long as you don't want them all at once?
> For example, I doubt anyone wants to permanently configure a device to monitor
> the various supplies, but they will want to occasionally.
> 
> As such, perhaps we need to treat this device more flexibly?
> There are obviously contraints on what channels + references make sense
> but maybe we should allow more than 16 to be specified?
> 

Ehhhhh. Look at the driver. It's already pushing 2k+ lines with
the 8 setups for 16 channels situation + all the extra options the
chip provides. If we also make it so that channels are rewritten at
runtime, it will turn into a mess. Or at least I don't see a clean
way of adding that. Besides, then I'd have to do all these extra
allocations depending on the number of channels in the device tree...
It gets complicated. If a customer expresses his interest in this,
I guess I'll have to add it.
Also, presumably the extra inputs are marketed as diagnostics.

>> +
>> +      adi,reference-select:
>> +        description: |
>> +          Select the reference source to use when converting on the
>> +          specific channel. Valid values are:
>> +          0: REFIN1(+)/REFIN1(−).
>> +          1: REFIN2(+)/REFIN2(−).
>> +          2: REFOUT/AVSS (Internal reference)
>> +          3: AVDD/AVSS
>> +          If not specified, internal reference is used.
> 
> That's not a good idea if it might be turned off...
> Perhaps a better approach would be to drop the int_ref_en and
> simply walk the channels to find out if any of them use it.
> If they don't turn it off, otherwise leave it on.
> 

Yeah, I guess I could do that.

>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 1, 2, 3]
>> +        default: 2
>> +
>> +      adi,excitation-pin-0:
>> +        description: |
>> +          Analog input to apply excitation current to while the channel
>> +          is active.
>> +        minimum: 0
>> +        maximum: 15
>> +        default: 0
>> +
>> +      adi,excitation-pin-1:
>> +        description: |
>> +          Analog input to apply excitation current to while this channel
>> +          is active.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 15
>> +        default: 0
>> +
>> +      adi,excitation-current-0-nanoamps:
>> +        description: |
>> +          Excitation current in nanoamps to be applied to pin specified in
>> +          adi,excitation-pin-0 while this channel is active.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
>> +        default: 0
>> +
>> +      adi,excitation-current-1-nanoamps:
>> +        description: |
>> +          Excitation current in nanoamps to be applied to pin specified in
>> +          adi,excitation-pin-1 while this channel is active.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000]
>> +        default: 0
>> +
>> +      adi,burnout-current-nanoamps:
>> +        description: |
>> +          Burnout current in nanoamps to be applied for this channel.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        enum: [0, 500, 2000, 4000]
>> +        default: 0
>> +
>> +      adi,buffered-positive:
>> +        description: Enable buffered mode for positive input.
>> +        type: boolean
>> +
>> +      adi,buffered-negative:
>> +        description: Enable buffered mode for negative input.
>> +        type: boolean
>> +
>> +    required:
>> +      - reg
>> +      - diff-channels
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    spi {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      adc@0 {
>> +        compatible = "adi,ad4130-8-24-wlcsp";
>> +        reg = <0>;
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        spi-max-frequency = <5000000>;
>> +        interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
>> +        interrupt-parent = <&gpio>;
>> +
>> +        channel@0 {
>> +          reg = <0>;
>> +          /* AIN8, AIN9 */
>> +          diff-channels = <8 9>;
>> +        };
>> +
>> +        channel@1 {
>> +          reg = <1>;
>> +          /* AIN10, AIN11 */
>> +          diff-channels = <10 11>;
>> +        };
>> +
>> +        channel@2 {
>> +          reg = <2>;
>> +          /* Temperature Sensor, DGND */
>> +          diff-channels = <16 19>;
>> +        };
>> +
>> +        channel@3 {
>> +          reg = <3>;
>> +          /* Internal reference, DGND */
>> +          diff-channels = <18 19>;
>> +        };
>> +
>> +        channel@4 {
>> +          reg = <4>;
>> +          /* DGND, DGND */
>> +          diff-channels = <19 19>;
>> +        };
>> +      };
>> +    };
> 

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-16 16:21   ` Jonathan Cameron
@ 2022-04-17 10:26     ` Cosmin Tanislav
  2022-04-24 15:51       ` Jonathan Cameron
  0 siblings, 1 reply; 20+ messages in thread
From: Cosmin Tanislav @ 2022-04-17 10:26 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav



On 4/16/22 19:21, Jonathan Cameron wrote:
> On Wed, 13 Apr 2022 12:40:11 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> AD4130-8 is an ultra-low power, high precision,
>> measurement solution for low bandwidth battery
>> operated applications.
>>
>> The fully integrated AFE (Analog Front-End)
>> includes a multiplexer for up to 16 single-ended
>> or 8 differential inputs, PGA (Programmable Gain
>> Amplifier), 24-bit Sigma-Delta ADC, on-chip
>> reference and oscillator, selectable filter
>> options, smart sequencer, sensor biasing and
>> excitation options, diagnostics, and a FIFO
>> buffer.
>>
>> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> 
> Hi Cosmin,
> 
> I've only glanced at Andy's comments, so may well overlap in places
> though I'll try and avoid too much repetition if I happen to remember
> Andy commented on something already.
> 
> Only a few minor things from me.  For such a complex device this
> is looking pretty good for a first version posted.
> 
> Jonathan
> 
> 
>> ---
>>   MAINTAINERS              |    8 +
>>   drivers/iio/adc/Kconfig  |   13 +
>>   drivers/iio/adc/Makefile |    1 +
>>   drivers/iio/adc/ad4130.c | 2072 ++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 2094 insertions(+)
>>   create mode 100644 drivers/iio/adc/ad4130.c
>>
> 
> ...
> 
>> diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
>> new file mode 100644
>> index 000000000000..89fb9b413ff0
>> --- /dev/null
>> +++ b/drivers/iio/adc/ad4130.c
>> @@ -0,0 +1,2072 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AD4130 SPI ADC driver
>> + *
>> + * Copyright 2022 Analog Devices Inc.
>> + */
>> +#include <asm/div64.h>
>> +#include <asm/unaligned.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +#include <linux/module.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define AD4130_8_NAME			"ad4130-8"
>> +
>> +#define AD4130_COMMS_READ_MASK		BIT(6)
>> +
>> +#define AD4130_REG_STATUS		0x00
>> +#define AD4130_STATUS_POR_FLAG_MASK	BIT(4)
>> +
>> +#define AD4130_REG_ADC_CONTROL		0x01
>> +#define AD4130_BIPOLAR_MASK		BIT(14)
> where possibly it is good to name register fields such that it's
> obvious which register they are fields of.  Makes it easier
> to be sure we have the right one.
> (I fell into this trap myself this week and wasted an hour or
> so before I figured out that there were two different registers
> with fields with exactly the same name ;)
> 
> Lots of different conventions for this one and I don't mind
> which one you pick. e.g.  This works, but isn't perfect by
> any means.
> 
> #define AD4130_ADC_CTRL_REG
> #define  AD4130_ADC_CTRL_BIPOLAR_MASK
>  > Note I quite like the subtle indenting to make it easier
> to read these definitions as well.
> 

Well. It's not late to change it now, if you insist.

If you look at my past drivers, I kept the register prefix
for masks, but it seemed kind of redundant and I dropped it
for this one.

By subtle indenting, you mean, making the masks look like
sub-definitions of the register?

>> +#define AD4130_INT_REF_VAL_MASK		BIT(13)
>> +#define AD4130_INT_REF_2_5V		2500000
>> +#define AD4130_INT_REF_1_25V		1250000
>> +#define AD4130_CSB_EN_MASK		BIT(9)
>> +#define AD4130_INT_REF_EN_MASK		BIT(8)
>> +#define AD4130_MODE_MASK		GENMASK(5, 2)
>> +#define AD4130_MCLK_SEL_MASK		GENMASK(1, 0)
> 
> ...
> 
> 
>> +
>> +#define AD4130_RESET_CLK_COUNT		64
>> +#define AD4130_RESET_BUF_SIZE		(AD4130_RESET_CLK_COUNT / 8)
>> +
>> +#define AD4130_SOFT_RESET_SLEEP		(160 * 1000000 / AD4130_MCLK_FREQ_76_8KHZ)
>> +
>> +#define AD4130_INVALID_SLOT		-1
>> +
>> +#define AD4130_FREQ_FACTOR		1000000000ull
> 
> Arguably should use the standard define for NANO

Already addressed by Andy.

> 
>> +#define AD4130_DB3_FACTOR		1000
>> +
> 
>> +
>> +struct ad4130_chip_info {
>> +	const char	*name;
>> +	u8		resolution;
>> +	bool		has_int_pin;
>> +};
>> +
>> +struct ad4130_setup_info {
>> +	unsigned int			iout0_val;
>> +	unsigned int			iout1_val;
>> +	unsigned int			burnout;
>> +	unsigned int			pga;
>> +	unsigned int			fs;
>> +	bool				ref_bufp;
>> +	bool				ref_bufm;
>> +	u32				ref_sel;
>> +	enum ad4130_filter_mode		filter_mode;
>> +	unsigned int			enabled_channels;
>> +	unsigned int			channels;
>> +};
>> +
>> +#define AD4130_SETUP_SIZE		offsetof(struct ad4130_setup_info, \
>> +						 enabled_channels)
> 
> Perhaps a comment on what this is?  Or define this as one structure
> containing another so that you can have the meta data alongside the
> actual stuff you want to be able to compare.
> 
> Or just rename it as AD4130_SETUP_MATCH_SIZE or something along
> those lines.
> 

Sure.

> ...
> 
>> +struct ad4130_state {
>> +	const struct ad4130_chip_info	*chip_info;
>> +	struct spi_device		*spi;
>> +	struct regmap			*regmap;
>> +	struct clk			*mclk;
>> +	struct regulator_bulk_data	regulators[4];
>> +	u32				irq_trigger;
>> +	u32				inv_irq_trigger;
>> +
>> +	/*
>> +	 * Synchronize access to members of driver state, and ensure atomicity
>> +	 * of consecutive regmap operations.
>> +	 */
>> +	struct mutex			lock;
>> +	struct completion		completion;
>> +
>> +	struct iio_chan_spec		chans[AD4130_MAX_CHANNELS];
>> +	struct ad4130_chan_info		chans_info[AD4130_MAX_CHANNELS];
>> +	struct ad4130_setup_info	setups_info[AD4130_MAX_SETUPS];
>> +	enum ad4130_pin_function	pins_fn[AD4130_MAX_ANALOG_PINS];
>> +	u32				vbias_pins[AD4130_MAX_ANALOG_PINS];
>> +	u32				num_vbias_pins;
>> +	int				scale_tbls[AD4130_REF_SEL_MAX]
>> +						  [AD4130_PGA_NUM][2];
>> +	struct gpio_chip		gc;
>> +	unsigned int			gpio_offsets[AD4130_MAX_GPIOS];
>> +	unsigned int			num_gpios;
>> +
>> +	u32			int_pin_sel;
>> +	bool			int_ref_en;
>> +	u32			int_ref_uv;
>> +	u32			mclk_sel;
>> +	bool			bipolar;
>> +
>> +	unsigned int		num_enabled_channels;
>> +	unsigned int		effective_watermark;
>> +	unsigned int		watermark;
>> +
>> +	struct spi_message	fifo_msg;
>> +	struct spi_transfer	fifo_xfer[2];
>> +
>> +	/*
>> +	 * DMA (thus cache coherency maintenance) requires the
>> +	 * transfer buffers to live in their own cache lines.
>> +	 */
>> +	u8			reset_buf[AD4130_RESET_BUF_SIZE] ____cacheline_aligned;
>> +	u8			reg_write_tx_buf[4];
>> +	u8			reg_read_tx_buf[1];
>> +	u8			reg_read_rx_buf[3];
>> +	u8			fifo_tx_buf[2];
>> +	u8			fifo_rx_buf[AD4130_FIFO_SIZE *
>> +					    AD4130_FIFO_MAX_SAMPLE_SIZE];
> 
> This is quite a large buffer.  Perhaps it would be better to drain the fifo
> in multiple steps if it is very full?  I guess that could be added
> later if anyone ever ran into a problem with the buffer size.
> 

We're quite time-constrained when receiving the FIFO watermark
interrupt, I'm not sure two separate transfers would be any better.

> 
>> +};
> 
>> +
>> +static const struct iio_info ad4130_info = {
>> +	.read_raw = ad4130_read_raw,
>> +	.read_avail = ad4130_read_avail,
>> +	.write_raw_get_fmt = ad4130_write_raw_get_fmt,
>> +	.write_raw = ad4130_write_raw,
>> +	.update_scan_mode = ad4130_update_scan_mode,
>> +	.hwfifo_set_watermark = ad4130_set_fifo_watermark,
>> +	.debugfs_reg_access = ad4130_reg_access,
>> +};
>> +
>> +static int ad4130_buffer_postenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4130_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&st->lock);
>> +
>> +	ret = ad4130_set_watermark_interrupt_en(st, true);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* When the chip enters FIFO mode, IRQ polarity is inversed. */
> 
> That is downright odd :)  Perhaps a datasheet section reference is
> appropriate here.

Page 65, FIFO Watermark Interrupt section.

+

Page 71, Bit Descriptions for STATUS Register, RDYB.

I'll add them as a comment.

> 
>> +	ret = irq_set_irq_type(st->spi->irq, st->inv_irq_trigger);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_WATERMARK);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = ad4130_set_mode(st, AD4130_MODE_CONTINUOUS);
>> +
>> +out:
>> +	mutex_unlock(&st->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ad4130_buffer_predisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4130_state *st = iio_priv(indio_dev);
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	mutex_lock(&st->lock);
>> +
>> +	ret = ad4130_set_mode(st, AD4130_MODE_IDLE);
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* When the chip exits FIFO mode, IRQ polarity returns to normal. */
>> +	ret = irq_set_irq_type(st->spi->irq, st->irq_trigger);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = ad4130_set_fifo_mode(st, AD4130_FIFO_MODE_DISABLED);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = ad4130_set_watermark_interrupt_en(st, false);
>> +	if (ret)
>> +		goto out;
>> +
>> +	for (i = 0; i < indio_dev->num_channels; i++) {
> Comment here on why we do this in predisable and not the equivalent in
> postenable.  (I assume because we don't call update_scan_mode in
> the disable path).
>

Yeah, that's exactly why. I'll add the comment.

>> +		ret = ad4130_set_channel_enable(st, i, false);
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>> +out:
>> +	mutex_unlock(&st->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ad4130_buffer_ops = {
>> +	.postenable = ad4130_buffer_postenable,
>> +	.predisable = ad4130_buffer_predisable,
>> +};
>> +
> 
> ...
> 
>> +
>> +static int find_table_index(const unsigned int *tbl, size_t len,
>> +			    unsigned int val)
> 
> This is a generic enough name you may well find you have
> ended up clashing with something added in a header in future.
> So I'd prefix this with the part number.
> 

Sure thing.

>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < len; i++)
>> +		if (tbl[i] == val)
>> +			return i;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int ad4130_get_ref_voltage(struct ad4130_state *st,
>> +				  enum ad4130_ref_sel ref_sel,
>> +				  unsigned int *ref_uv)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	int ret;
>> +
>> +	switch (ref_sel) {
>> +	case AD4130_REF_REFIN1:
>> +		ret = regulator_get_voltage(st->regulators[2].consumer);
>> +		break;
>> +	case AD4130_REF_REFIN2:
>> +		ret = regulator_get_voltage(st->regulators[3].consumer);
>> +		break;
>> +	case AD4130_REF_AVDD_AVSS:
>> +		ret = regulator_get_voltage(st->regulators[0].consumer);
>> +		break;
>> +	case AD4130_REF_REFOUT_AVSS:
>> +		if (!st->int_ref_en) {
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		ret = st->int_ref_uv;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	if (ret <= 0)
>> +		return dev_err_probe(dev, ret, "Cannot use reference %u\n",
>> +				     ref_sel);
>> +
>> +	if (ref_uv)
>> +		*ref_uv = ret;
> 
> I'd probably keep this simpler and have the caller always provide
> ref_uv. Use a local dummy variable where it doesn't need the answer
> or even better just return the voltage as positive value.
> 

I'll switch to returning it.

>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int ad4310_parse_fw(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4130_state *st = iio_priv(indio_dev);
>> +	struct device *dev = &st->spi->dev;
>> +	unsigned int i;
>> +	int avdd_uv;
>> +	int irq;
>> +	int ret;
>> +
>> +	st->mclk = devm_clk_get_optional(dev, "mclk");
>> +	if (IS_ERR(st->mclk))
>> +		return dev_err_probe(dev, PTR_ERR(st->mclk),
>> +				     "Failed to get mclk\n");
>> +
>> +	st->int_pin_sel = AD4130_INT_PIN_DOUT_OR_INT;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ad4130_int_pin_names); i++) {
>> +		irq = of_irq_get_byname(dev->of_node, ad4130_int_pin_names[i]);
> 
> fwnode version.
> 

Yep, already mentioned by Andy.

>> +		if (irq > 0) {
>> +			st->int_pin_sel = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (st->int_pin_sel == AD4130_INT_PIN_DOUT ||
>> +	    (st->int_pin_sel == AD4130_INT_PIN_DOUT_OR_INT &&
>> +	     !st->chip_info->has_int_pin))
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Cannot use DOUT as interrupt pin\n");
>> +
>> +	if (st->int_pin_sel == AD4130_INT_PIN_P1)
>> +		st->pins_fn[AD4130_AIN2_P1] = AD4130_PIN_FN_SPECIAL;
> 
> Looking at datasheet I see an option for P2, but not P1?
> 

I messed this one up. Thanks.

>> +
>> +	st->mclk_sel = AD4130_MCLK_76_8KHZ;
>> +	device_property_read_u32(dev, "adi,mclk-sel", &st->mclk_sel);
>> +
>> +	if (st->mclk_sel >= AD4130_MCLK_SEL_MAX)
>> +		return dev_err_probe(dev, -EINVAL, "Invalid clock %u\n",
>> +				     st->mclk_sel);
>> +
>> +	if (st->mclk && (st->mclk_sel == AD4130_MCLK_76_8KHZ ||
>> +			 st->mclk_sel == AD4130_MCLK_76_8KHZ_OUT))
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Cannot use external clock\n");
>> +
>> +	if (st->int_pin_sel == AD4130_INT_PIN_CLK &&
>> +	    st->mclk_sel != AD4130_MCLK_76_8KHZ)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid clock %u for interrupt pin %u\n",
>> +				     st->mclk_sel, st->int_pin_sel);
>> +
>> +	st->int_ref_en = true;
>> +	if (device_property_present(dev, "adi,int-ref-en"))
>> +		st->int_ref_en = device_property_read_bool(dev, "adi,int-ref-en");
>> +
>> +	st->int_ref_uv = AD4130_INT_REF_2_5V;
>> +
>> +	/*
>> +	 * When the AVDD supply is set to below 2.5V the internal reference of
>> +	 * 1.25V should be selected.
> 
> Good to give specific reference to datasheet section for things like this.
> Seems to be in the ADC REFERENCE section.
> 

I'll add it.

>> +	 */
>> +	avdd_uv = regulator_get_voltage(st->regulators[0].consumer);
>> +	if (avdd_uv > 0 && avdd_uv < AD4130_INT_REF_2_5V)
>> +		st->int_ref_uv = AD4130_INT_REF_1_25V;
>> +
>> +	st->bipolar = device_property_read_bool(dev, "adi,bipolar");
>> +
>> +	ret = device_property_count_u32(dev, "adi,vbias-pins");
>> +	if (ret > 0) {
>> +		st->num_vbias_pins = ret;
>> +
>> +		ret = device_property_read_u32_array(dev, "adi,vbias-pins",
>> +						     st->vbias_pins,
>> +						     st->num_vbias_pins);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Failed to read vbias pins\n");
>> +
>> +		ret = ad4130_validate_vbias_pins(st, st->vbias_pins,
>> +						 st->num_vbias_pins);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = ad4130_parse_fw_children(indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < AD4130_MAX_GPIOS; i++) {
>> +		if (st->pins_fn[i + AD4130_AIN2_P1] != AD4130_PIN_FN_NONE)
>> +			continue;
> 
> I'm a bit confused. pins_fn seems to be for the Analog pins, yet here is being
> used for the GPIOs?  Maybe some explanatory comments
> 

AIN2 = P1, AIN3 = P2, AIN4 = P3, AIN5 = P4. I'll add some comments.

>> +
>> +		st->gpio_offsets[st->num_gpios++] = i;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int ad4130_setup(struct iio_dev *indio_dev)
>> +{
>> +	struct ad4130_state *st = iio_priv(indio_dev);
>> +	struct device *dev = &st->spi->dev;
>> +	unsigned int int_ref_val;
>> +	unsigned long rate = AD4130_MCLK_FREQ_76_8KHZ;
>> +	unsigned int val;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	if (st->mclk_sel == AD4130_MCLK_153_6KHZ_EXT)
>> +		rate = AD4130_MCLK_FREQ_153_6KHZ;
>> +
>> +	ret = clk_set_rate(st->mclk, rate);
> 
> Ah. I'd neglected in my review of the dt bindings that we'd
> be specifying the clock in here. We will need a parameter
> to specify the clock speed if external is used, but I'd still like
> that separated from the question of internal vs external clocks.
> 

Addressed in the dt bindings reply.

>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = clk_prepare_enable(st->mclk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_add_action_or_reset(dev, ad4130_clk_disable_unprepare,
>> +				       st->mclk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (st->int_ref_uv == AD4130_INT_REF_2_5V)
>> +		int_ref_val = AD4130_INT_REF_VAL_2_5V;
>> +	else
>> +		int_ref_val = AD4130_INT_REF_VAL_1_25V;
>> +
>> +	/* Switch to SPI 4-wire mode. */
>> +	val = AD4130_CSB_EN_MASK;
>> +	val |= st->bipolar ? AD4130_BIPOLAR_MASK : 0;
> 
> Prefer field PREP even for these single bit cases >

Do you want this for the places where I used `status ? mask : 0`
inside regmap_update_bits() calls too?

>> +	val |= st->int_ref_en ? AD4130_INT_REF_EN_MASK : 0;
>> +	val |= FIELD_PREP(AD4130_MODE_MASK, AD4130_MODE_IDLE);
>> +	val |= FIELD_PREP(AD4130_MCLK_SEL_MASK, st->mclk_sel);
>> +	val |= FIELD_PREP(AD4130_INT_REF_VAL_MASK, int_ref_val);
>> +
>> +	ret = regmap_write(st->regmap, AD4130_REG_ADC_CONTROL, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = FIELD_PREP(AD4130_INT_PIN_SEL_MASK, st->int_pin_sel);
>> +	for (i = 0; i < st->num_gpios; i++)
>> +		val |= BIT(st->gpio_offsets[i]);
>> +
>> +	ret = regmap_write(st->regmap, AD4130_REG_IO_CONTROL, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	val = 0;
>> +	for (i = 0; i < st->num_vbias_pins; i++)
>> +		val |= BIT(st->vbias_pins[i]);
>> +
>> +	ret = regmap_write(st->regmap, AD4130_REG_VBIAS, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
>> +				 AD4130_ADD_FIFO_HEADER_MASK, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* FIFO watermark interrupt starts out as enabled, disable it. */
>> +	ret = ad4130_set_watermark_interrupt_en(st, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Setup channels. */
>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>> +		struct ad4130_chan_info *chan_info = &st->chans_info[i];
>> +		struct iio_chan_spec *chan = &st->chans[i];
>> +		unsigned int val;
>> +
>> +		val = FIELD_PREP(AD4130_AINP_MASK, chan->channel) |
>> +		      FIELD_PREP(AD4130_AINM_MASK, chan->channel2) |
>> +		      FIELD_PREP(AD4130_IOUT1_MASK, chan_info->iout0) |
>> +		      FIELD_PREP(AD4130_IOUT2_MASK, chan_info->iout1);
>> +
>> +		ret = regmap_write(st->regmap, AD4130_REG_CHANNEL_X(i), val);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
> ...
> 
>> +static int ad4130_probe(struct spi_device *spi)
>> +{
>> +	const struct ad4130_chip_info *info;
>> +	struct device *dev = &spi->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct ad4130_state *st;
>> +	int ret;
>> +
>> +	info = device_get_match_data(dev);
>> +	if (!info)
>> +		return -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	memset(st->reset_buf, 0xff, AD4130_RESET_BUF_SIZE);
>> +	init_completion(&st->completion);
>> +	mutex_init(&st->lock);
>> +	st->chip_info = info;
>> +	st->spi = spi;
>> +
>> +	/*
>> +	 * Xfer:   [ XFR1 ] [         XFR2         ]
>> +	 * Master:  0x7D N   ......................
>> +	 * Slave:   ......   DATA1 DATA2 ... DATAN
>> +	 */
>> +	st->fifo_tx_buf[0] = AD4130_COMMS_READ_MASK | AD4130_REG_FIFO_DATA;
>> +	st->fifo_xfer[0].tx_buf = st->fifo_tx_buf;
>> +	st->fifo_xfer[0].len = sizeof(st->fifo_tx_buf);
>> +	st->fifo_xfer[1].rx_buf = st->fifo_rx_buf;
>> +	spi_message_init_with_transfers(&st->fifo_msg, st->fifo_xfer,
>> +					ARRAY_SIZE(st->fifo_xfer));
>> +
>> +	indio_dev->name = st->chip_info->name;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->info = &ad4130_info;
>> +
>> +	st->regmap = devm_regmap_init(dev, NULL, st,
>> +				      &ad4130_regmap_config);
> 
> Don't wrap below 80 chars unless there is some extra meaning conveyed
> by doing so.  Don't think that's true ehre.
> 

Sure.

> 
>> +	if (IS_ERR(st->regmap))
>> +		return PTR_ERR(st->regmap);
>> +
>> +	st->regulators[0].supply = "avdd";
>> +	st->regulators[1].supply = "iovdd";
>> +	st->regulators[2].supply = "refin1";
>> +	st->regulators[3].supply = "refin2";
>> +
>> +	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, ad4130_disable_regulators, st);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to add regulators disable action\n");
>> +
>> +	ret = ad4130_soft_reset(st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ad4310_parse_fw(indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ad4130_setup(indio_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ad4130_fill_scale_tbls(st);
>> +
>> +	if (st->num_gpios) {
>> +		st->gc.owner = THIS_MODULE;
>> +		st->gc.label = st->chip_info->name;
>> +		st->gc.base = -1;
>> +		st->gc.ngpio = AD4130_MAX_GPIOS;
>> +		st->gc.parent = dev;
>> +		st->gc.can_sleep = true;
>> +		st->gc.get_direction = ad4130_gpio_get_direction;
>> +		st->gc.set = ad4130_gpio_set;
>> +
>> +		ret = devm_gpiochip_add_data(dev, &st->gc, st);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
>> +					      INDIO_BUFFER_SOFTWARE,
>> +					      &ad4130_buffer_ops,
>> +					      ad4130_fifo_attributes);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_request_threaded_irq(dev, spi->irq, NULL,
>> +					ad4130_irq_handler, IRQF_ONESHOT,
>> +					indio_dev->name, indio_dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to request irq\n");
>> +
>> +	st->irq_trigger = irq_get_trigger_type(spi->irq);
>> +	if (st->irq_trigger & IRQF_TRIGGER_RISING)
>> +		st->inv_irq_trigger = IRQF_TRIGGER_FALLING;
>> +	else if (st->irq_trigger & IRQF_TRIGGER_FALLING)
>> +		st->inv_irq_trigger = IRQF_TRIGGER_RISING;
>> +	else
>> +		return dev_err_probe(dev, -EINVAL, "Invalid irq flags: %u\n",
>> +				     st->irq_trigger);
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
> 

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

* Re: [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver
  2022-04-17 10:26     ` Cosmin Tanislav
@ 2022-04-24 15:51       ` Jonathan Cameron
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2022-04-24 15:51 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav

On Sun, 17 Apr 2022 13:26:38 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> On 4/16/22 19:21, Jonathan Cameron wrote:
> > On Wed, 13 Apr 2022 12:40:11 +0300
> > Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >   
> >> AD4130-8 is an ultra-low power, high precision,
> >> measurement solution for low bandwidth battery
> >> operated applications.
> >>
> >> The fully integrated AFE (Analog Front-End)
> >> includes a multiplexer for up to 16 single-ended
> >> or 8 differential inputs, PGA (Programmable Gain
> >> Amplifier), 24-bit Sigma-Delta ADC, on-chip
> >> reference and oscillator, selectable filter
> >> options, smart sequencer, sensor biasing and
> >> excitation options, diagnostics, and a FIFO
> >> buffer.
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>  
> > 
> > Hi Cosmin,
> > 
> > I've only glanced at Andy's comments, so may well overlap in places
> > though I'll try and avoid too much repetition if I happen to remember
> > Andy commented on something already.
> > 
> > Only a few minor things from me.  For such a complex device this
> > is looking pretty good for a first version posted.
> > 
> > Jonathan
> > 
> >   
> >> ---
> >>   MAINTAINERS              |    8 +
> >>   drivers/iio/adc/Kconfig  |   13 +
> >>   drivers/iio/adc/Makefile |    1 +
> >>   drivers/iio/adc/ad4130.c | 2072 ++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 2094 insertions(+)
> >>   create mode 100644 drivers/iio/adc/ad4130.c
> >>  
> > 
> > ...
> >   
> >> diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
> >> new file mode 100644
> >> index 000000000000..89fb9b413ff0
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ad4130.c
> >> @@ -0,0 +1,2072 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * AD4130 SPI ADC driver
> >> + *
> >> + * Copyright 2022 Analog Devices Inc.
> >> + */
> >> +#include <asm/div64.h>
> >> +#include <asm/unaligned.h>
> >> +#include <linux/bitfield.h>
> >> +#include <linux/bitops.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/gpio/driver.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/irq.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/iio/buffer.h>
> >> +#include <linux/iio/kfifo_buf.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/property.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/regulator/consumer.h>
> >> +#include <linux/spi/spi.h>
> >> +
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/iio/sysfs.h>
> >> +
> >> +#define AD4130_8_NAME			"ad4130-8"
> >> +
> >> +#define AD4130_COMMS_READ_MASK		BIT(6)
> >> +
> >> +#define AD4130_REG_STATUS		0x00
> >> +#define AD4130_STATUS_POR_FLAG_MASK	BIT(4)
> >> +
> >> +#define AD4130_REG_ADC_CONTROL		0x01
> >> +#define AD4130_BIPOLAR_MASK		BIT(14)  
> > where possibly it is good to name register fields such that it's
> > obvious which register they are fields of.  Makes it easier
> > to be sure we have the right one.
> > (I fell into this trap myself this week and wasted an hour or
> > so before I figured out that there were two different registers
> > with fields with exactly the same name ;)
> > 
> > Lots of different conventions for this one and I don't mind
> > which one you pick. e.g.  This works, but isn't perfect by
> > any means.
> > 
> > #define AD4130_ADC_CTRL_REG
> > #define  AD4130_ADC_CTRL_BIPOLAR_MASK  
> >  > Note I quite like the subtle indenting to make it easier  
> > to read these definitions as well.
> >   
> 
> Well. It's not late to change it now, if you insist.
> 
> If you look at my past drivers, I kept the register prefix
> for masks, but it seemed kind of redundant and I dropped it
> for this one.

To a certain extent this is about consistency.  Even if it's
not necessary for clarity in this particular driver I'd like
to keep that clarity of definition in all drivers if possible
to provide good examples for cases where maybe it's more
important.

> 
> By subtle indenting, you mean, making the masks look like
> sub-definitions of the register?

Sort of - I mean the extra space as in the example above between
define and the name.

> 
> >> +#define AD4130_INT_REF_VAL_MASK		BIT(13)
> >> +#define AD4130_INT_REF_2_5V		2500000
> >> +#define AD4130_INT_REF_1_25V		1250000
> >> +#define AD4130_CSB_EN_MASK		BIT(9)
> >> +#define AD4130_INT_REF_EN_MASK		BIT(8)
> >> +#define AD4130_MODE_MASK		GENMASK(5, 2)
> >> +#define AD4130_MCLK_SEL_MASK		GENMASK(1, 0)  


> > ...
> >   
> >> +struct ad4130_state {
> >> +	const struct ad4130_chip_info	*chip_info;
> >> +	struct spi_device		*spi;
> >> +	struct regmap			*regmap;
> >> +	struct clk			*mclk;
> >> +	struct regulator_bulk_data	regulators[4];
> >> +	u32				irq_trigger;
> >> +	u32				inv_irq_trigger;
> >> +
> >> +	/*
> >> +	 * Synchronize access to members of driver state, and ensure atomicity
> >> +	 * of consecutive regmap operations.
> >> +	 */
> >> +	struct mutex			lock;
> >> +	struct completion		completion;
> >> +
> >> +	struct iio_chan_spec		chans[AD4130_MAX_CHANNELS];
> >> +	struct ad4130_chan_info		chans_info[AD4130_MAX_CHANNELS];
> >> +	struct ad4130_setup_info	setups_info[AD4130_MAX_SETUPS];
> >> +	enum ad4130_pin_function	pins_fn[AD4130_MAX_ANALOG_PINS];
> >> +	u32				vbias_pins[AD4130_MAX_ANALOG_PINS];
> >> +	u32				num_vbias_pins;
> >> +	int				scale_tbls[AD4130_REF_SEL_MAX]
> >> +						  [AD4130_PGA_NUM][2];
> >> +	struct gpio_chip		gc;
> >> +	unsigned int			gpio_offsets[AD4130_MAX_GPIOS];
> >> +	unsigned int			num_gpios;
> >> +
> >> +	u32			int_pin_sel;
> >> +	bool			int_ref_en;
> >> +	u32			int_ref_uv;
> >> +	u32			mclk_sel;
> >> +	bool			bipolar;
> >> +
> >> +	unsigned int		num_enabled_channels;
> >> +	unsigned int		effective_watermark;
> >> +	unsigned int		watermark;
> >> +
> >> +	struct spi_message	fifo_msg;
> >> +	struct spi_transfer	fifo_xfer[2];
> >> +
> >> +	/*
> >> +	 * DMA (thus cache coherency maintenance) requires the
> >> +	 * transfer buffers to live in their own cache lines.
> >> +	 */
> >> +	u8			reset_buf[AD4130_RESET_BUF_SIZE] ____cacheline_aligned;
> >> +	u8			reg_write_tx_buf[4];
> >> +	u8			reg_read_tx_buf[1];
> >> +	u8			reg_read_rx_buf[3];
> >> +	u8			fifo_tx_buf[2];
> >> +	u8			fifo_rx_buf[AD4130_FIFO_SIZE *
> >> +					    AD4130_FIFO_MAX_SAMPLE_SIZE];  
> > 
> > This is quite a large buffer.  Perhaps it would be better to drain the fifo
> > in multiple steps if it is very full?  I guess that could be added
> > later if anyone ever ran into a problem with the buffer size.
> >   
> 
> We're quite time-constrained when receiving the FIFO watermark
> interrupt, I'm not sure two separate transfers would be any better.

Potential issue is that you get an SPI master that can't do such a bit
transfer.  There are a few out there which are quite limited because
they aren't DMA based. As stated, perhaps this is one to fix only
when someone runs into the problem.

> 
> >   
> >> +};  
> >   
> >> +
> >> +static const struct iio_info ad4130_info = {
> >> +	.read_raw = ad4130_read_raw,
> >> +	.read_avail = ad4130_read_avail,
> >> +	.write_raw_get_fmt = ad4130_write_raw_get_fmt,
> >> +	.write_raw = ad4130_write_raw,
> >> +	.update_scan_mode = ad4130_update_scan_mode,
> >> +	.hwfifo_set_watermark = ad4130_set_fifo_watermark,
> >> +	.debugfs_reg_access = ad4130_reg_access,
> >> +};
> >> +
> >> +static int ad4130_buffer_postenable(struct iio_dev *indio_dev)
> >> +{
> >> +	struct ad4130_state *st = iio_priv(indio_dev);
> >> +	int ret;
> >> +
> >> +	mutex_lock(&st->lock);
> >> +
> >> +	ret = ad4130_set_watermark_interrupt_en(st, true);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	/* When the chip enters FIFO mode, IRQ polarity is inversed. */  
> > 
> > That is downright odd :)  Perhaps a datasheet section reference is
> > appropriate here.  
> 
> Page 65, FIFO Watermark Interrupt section.
> 
> +
> 
> Page 71, Bit Descriptions for STATUS Register, RDYB.
> 
> I'll add them as a comment.

Great.

...

...

> >> +	ret = ad4130_parse_fw_children(indio_dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < AD4130_MAX_GPIOS; i++) {
> >> +		if (st->pins_fn[i + AD4130_AIN2_P1] != AD4130_PIN_FN_NONE)
> >> +			continue;  
> > 
> > I'm a bit confused. pins_fn seems to be for the Analog pins, yet here is being
> > used for the GPIOs?  Maybe some explanatory comments
> >   
> 
> AIN2 = P1, AIN3 = P2, AIN4 = P3, AIN5 = P4. I'll add some comments.

Ah. I'd missed that relationship.

> 
> >> +
> >> +		st->gpio_offsets[st->num_gpios++] = i;
> >> +	}
> >> +
> >> +	return 0;
> >> +}  
...

> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = clk_prepare_enable(st->mclk);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = devm_add_action_or_reset(dev, ad4130_clk_disable_unprepare,
> >> +				       st->mclk);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (st->int_ref_uv == AD4130_INT_REF_2_5V)
> >> +		int_ref_val = AD4130_INT_REF_VAL_2_5V;
> >> +	else
> >> +		int_ref_val = AD4130_INT_REF_VAL_1_25V;
> >> +
> >> +	/* Switch to SPI 4-wire mode. */
> >> +	val = AD4130_CSB_EN_MASK;
> >> +	val |= st->bipolar ? AD4130_BIPOLAR_MASK : 0;  
> > 
> > Prefer field PREP even for these single bit cases >  
> 
> Do you want this for the places where I used `status ? mask : 0`
> inside regmap_update_bits() calls too?

That would be great.   Though probably not for the gpio one as
that is used in a more complex fashion so would be more confusing
done with two FIELD_PREP() calls.

> 
> >> +	val |= st->int_ref_en ? AD4130_INT_REF_EN_MASK : 0;

Sorry I didn't get back to this earlier (I see you sent a v2 and v3).
Fun week of spec review against a short timescale so I've not had any
time to get much IIO mailing list reading done!

Thanks,

Jonathan



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

* Re: [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130
  2022-04-17 10:16   ` Cosmin Tanislav
@ 2022-04-24 16:05     ` Jonathan Cameron
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2022-04-24 16:05 UTC (permalink / raw)
  To: Cosmin Tanislav
  Cc: Rob Herring, Linus Walleij, linux-iio, linux-gpio, linux-kernel,
	devicetree, Cosmin Tanislav


> 
> >>
> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> >> ---
> >>   .../bindings/iio/adc/adi,ad4130.yaml          | 255 ++++++++++++++++++
> >>   1 file changed, 255 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> >> new file mode 100644
> >> index 000000000000..e9dce54e9802
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
> >> @@ -0,0 +1,255 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +# Copyright 2022 Analog Devices Inc.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Analog Devices AD4130 ADC device driver
> >> +
> >> +maintainers:
> >> +  - Cosmin Tanislav <cosmin.tanislav@analog.com>
> >> +
> >> +description: |
> >> +  Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here:
> >> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - adi,ad4130-8-16-lfcsp
> >> +      - adi,ad4130-8-16-wlcsp
> >> +      - adi,ad4130-8-24-lfcsp
> >> +      - adi,ad4130-8-24-wlcsp  
> > 
> > What are the variants?   They look to possibly be package differences?
> > + resolution differences.
> > They definitely need some description here.
> > It may make more sense to have one compatible and then some extra
> > booleans to say what it supported.
> >  
> 
> Packaging + available interrupt pins + resolution. Is having extra
> booleans to describe what is supported really the best approach?
> It's not really about how the hardware is configured anymore, is it?
> They're different chips.

It's unusual to have compatibles for packaging alone and I couldn't find
any clear references to the variants.  Maybe best we can do here is
add a bunch of documentation.

> 
> > Long shot, but do the different packages have different model IDs?
> > The datasheet says
> > Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and
> > are read only.  If there is one for each of these models then it would be
> > better to have a single compatible and do the detection of variant in
> > the driver.
> > 
> > I'm not immediately spotting the resolution information in the data sheet.
> > It is marked Preliminary but if there are details missing, please mention
> > in cover letter so we don't go looking for information that doesn't exist.
> >  
> 
> I don't have enough information about the other models to know what
> Model IDs they will have. That's why I took this approach.

My inclination is to probably not add the compatibles for those until we
do have that information.

> 
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clocks:
> >> +    maxItems: 1
> >> +    description: phandle to the master clock (mclk)
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: mclk
> >> +
> >> +  interrupts:
> >> +    minItems: 1
> >> +    maxItems: 1
> >> +
> >> +  interrupt-names:
> >> +    minItems: 1
> >> +    maxItems: 1
> >> +    description:
> >> +      Default if not supplied is dout-int.
> >> +    items:
> >> +      enum:
> >> +        - dout-int
> >> +        - clk
> >> +        - p1  
> > 
> > This is unusual.  It is probably helpful to add more description to
> > explain that the data ready/ fifo interrupt can be routed to any of these
> > pins.  
> 
> Is it unusual? ADIS16480 follows a similar approach.

I think I got thrown by the large number of choices for a single interrupt.
Flexibility of interrupt routing is common but I've never seen 4 choices for
a single interrupt before :)

> 
> description: |
>    Specify which interrupt pin should be configured as Data Ready / FIFO
>    interrupt.
>    Default if not supplied is dout-int.
> 
> How does this sound?

Sounds good to me.

> 
> >   
> >> +        - dout
> >> +
> >> +  '#address-cells':
> >> +    const: 1
> >> +
> >> +  '#size-cells':
> >> +    const: 0
> >> +
> >> +  refin1-supply:
> >> +    description: refin1 supply. Can be used as reference for conversion.
> >> +
> >> +  refin2-supply:
> >> +    description: refin2 supply. Can be used as reference for conversion.
> >> +
> >> +  avdd-supply:
> >> +    description: AVDD voltage supply. Can be used as reference for conversion.  
> > 
> > Whilst these are optional in theory, you should call out that they must be
> > provided if any of the channels use them as a reference.
> >   
> 
> I thought that "Can be used as reference for conversion." + it being an
> option in adi,reference-select property would make it obvious, no?

More obvious is always good, but you are probably right and I was just
not terribly awake when I wrote that :)

> 
> >> +
> >> +  iovdd-supply:
> >> +    description: IOVDD voltage supply. Used for the chip interface.
> >> +
> >> +  spi-max-frequency:
> >> +    maximum: 5000000
> >> +
> >> +  adi,mclk-sel:
> >> +    description: |
> >> +      Select the clock.
> >> +      0: Internal 76.8kHz clock.
> >> +      1: Internal 76.8kHz clock, output to the CLK pin.
> >> +      2: External 76.8kHz clock.
> >> +      3. External 153.6kHz clock.  
> > 
> > For the external clocks, can we use the fact that one is supplied
> > as enough to tell us we should be using them?  Then query the
> > frequency directly from that clock?
> >   
> 
> Aren't we supposed to set the frequency of that clock ourselves,
> in the driver?

True, I got that backwards.

> 
> > If no clock provided then clearly internal.  All that is
> > necessary after that is a boolean to control if the CLK output
> > is enabled or not (and ideally constrain that to only be possible
> > if in internal clock mode).
> >   
> 
> Well...
> 
> So, mclk present => external, not present => internal.
> adi,int-clk-out-enable to specify if the internal clock should be
> exposed? adi,ext-clk-freq to specify the desired clock speed of the
> external clk?

Yes that sounds good to me.  I just don't like magic numbers
in bindings if we can avoid them.   With the above a reader should
be able to figure out what is going on without reading this doc.

> 
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [0, 1, 2, 3]
> >> +    default: 0
> >> +
> >> +  adi,int-ref-en:  
> > 
> > Mentioned below...
> >   
> >> +    description: |
> >> +      Specify if internal reference should be enabled.
> >> +    type: boolean
> >> +    default: true
> >> +
> >> +  adi,bipolar:
> >> +    description: Specify if the device should be used in bipolar mode.
> >> +    type: boolean
> >> +    default: false
> >> +
> >> +  adi,vbias-pins:
> >> +    description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to.
> >> +    items:
> >> +      minimum: 0
> >> +      maximum: 15  
> > 
> > If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines.
> > So why use a single value rather than either a list of pins, or a bitmap?
> >   
> 
> Umm. Isn't this a list of pins? That's why everything is plural here.
> I guess I should add `maxItems: 16`?
> I already added `$ref: /schemas/types.yaml#/definitions/uint32-array`.

Ah. Yes, with an array and maxItems this will be fine I think.

> 
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupts
> >> +
> >> +patternProperties:
> >> +  "^channel@([0-9]|1[0-5])$":
> >> +    type: object
> >> +    $ref: adc.yaml
> >> +
> >> +    properties:
> >> +      reg:
> >> +        description: |
> >> +          The channel number.
> >> +        items:
> >> +          minimum: 0
> >> +          maximum: 15
> >> +
> >> +      diff-channels:
> >> +        description: |
> >> +          Besides the analog inputs available, internal inputs can be used.
> >> +          16: Internal temperature sensor.
> >> +          17: AVss
> >> +          18: Internal reference.
> >> +          19: DGND.
> >> +          20: (AVDD − AVSS)/6+
> >> +          21: (AVDD − AVSS)/6-
> >> +          22: (IOVDD − DGND)/6+
> >> +          23: (IOVDD − DGND)/6-
> >> +          24: (ALDO − AVSS)/6+
> >> +          25: (ALDO − AVSS)/6-
> >> +          26: (DLDO − DGND)/6+
> >> +          27: (DLDO − DGND)/6-
> >> +          28: V_MV_P
> >> +          29: V_MV_M
> >> +        $ref: adc.yaml
> >> +        items:
> >> +          minimum: 0
> >> +          maximum: 29  
> > 
> > Interesting. So we have a part that has a 16 channel sequencer, but
> > can you have more channels as long as you don't want them all at once?
> > For example, I doubt anyone wants to permanently configure a device to monitor
> > the various supplies, but they will want to occasionally.
> > 
> > As such, perhaps we need to treat this device more flexibly?
> > There are obviously contraints on what channels + references make sense
> > but maybe we should allow more than 16 to be specified?
> >   
> 
> Ehhhhh. Look at the driver. It's already pushing 2k+ lines with
> the 8 setups for 16 channels situation + all the extra options the
> chip provides. If we also make it so that channels are rewritten at
> runtime, it will turn into a mess. Or at least I don't see a clean
> way of adding that. Besides, then I'd have to do all these extra
> allocations depending on the number of channels in the device tree...
> It gets complicated. If a customer expresses his interest in this,
> I guess I'll have to add it.
> Also, presumably the extra inputs are marketed as diagnostics.

Another approach would be to 'always' provide those diagnostic channels
but not via buffered interface (if that's sensible to do).  Then potentially
drop them from this binding?

Maybe waiting for customer demand is the best way to go.  Even if you
added a path to read them without them being in DT later it should be
easy enough to maintain backwards compatibility.

Jonathan

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

end of thread, other threads:[~2022-04-24 15:57 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13  9:40 [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Cosmin Tanislav
2022-04-13  9:40 ` [PATCH v1 2/3] iio: ABI: adc: ad4130: document filter_mode{,_available} Cosmin Tanislav
2022-04-13 14:51   ` Andy Shevchenko
2022-04-14  7:41     ` Cosmin Tanislav
2022-04-16 15:02   ` Jonathan Cameron
2022-04-13  9:40 ` [PATCH v1 3/3] iio: adc: ad4130: add AD4130 driver Cosmin Tanislav
2022-04-13 15:41   ` Andy Shevchenko
2022-04-14 11:06     ` Cosmin Tanislav
2022-04-14 13:45       ` Andy Shevchenko
2022-04-14 14:53         ` Cosmin Tanislav
2022-04-14 15:37           ` Andy Shevchenko
2022-04-16 15:07             ` Jonathan Cameron
2022-04-16 16:21   ` Jonathan Cameron
2022-04-17 10:26     ` Cosmin Tanislav
2022-04-24 15:51       ` Jonathan Cameron
2022-04-13 12:26 ` [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 Rob Herring
2022-04-13 14:50 ` Andy Shevchenko
2022-04-16 15:00 ` Jonathan Cameron
2022-04-17 10:16   ` Cosmin Tanislav
2022-04-24 16:05     ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).