[PATCH 1/2] dt-bindings: adc: add AD717X

[PATCH 1/2] dt-bindings: adc: add AD717X

[Dumitru Ceclan]

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

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 .../bindings/iio/adc/adi,ad717x.yaml          | 158 ++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml
new file mode 100644
index 000000000000..f12926e69958
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml
@@ -0,0 +1,158 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad717x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD717X ADC family SPI driver
+
+maintainers:
+  - Ceclan Dumitru <dumitru.ceclan@analog.com>
+
+description: |
+  Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7172-2
+      - adi,ad7173-8
+      - adi,ad7175-2
+      - adi,ad7176-2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  spi-max-frequency:
+    maximum: 20000000
+
+  spi-cpol:
+    type: boolean
+
+  spi-cpha:
+    type: boolean
+
+  adi,temp-channel:
+    description:
+      Enables temperature reading channel
+    type: boolean
+
+  dependencies:
+    adi,temp-channel:
+      compatible:
+        enum:
+          - adi,ad7172-2
+          - adi,ad7173-8
+          - adi,ad7175-2
+
+
+  required:
+    - compatible
+    - reg
+    - interrupts
+    - spi-cpol
+    - spi-cpha
+
+patternProperties:
+  "^channel@([0-9a-f])$":
+    type: object
+    $ref: adc.yaml
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        description: Channel number
+        minimum: 0
+        maximum: 15
+
+      diff-channels:
+        description:
+          Analog input pins
+        items:
+          minimum: 0
+          maximum: 31
+      
+      adi,bipolar:
+        description: Specify if the channel should measure in bipolar mode.
+        type: boolean
+
+    required:
+      - reg
+      - diff-channels
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+      status = "okay";
+
+      ad7173@0 {
+        compatible = "adi,ad7173-8";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpio>;
+        spi-max-frequency = <5000000>;
+        spi-cpol;
+        spi-cpha;
+
+        adi,temp-channel;
+
+        channel@0 {
+          reg = <0>;
+          adi,bipolar;
+
+          diff-channels = <0 1>;
+        };
+
+        channel@1 {
+          reg = <1>;
+
+          diff-channels = <2 3>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          adi,bipolar;
+
+          diff-channels = <4 5>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          adi,bipolar;
+
+          diff-channels = <6 7>;
+        };
+
+        channel@4 {
+          reg = <4>;
+
+          diff-channels = <8 9>;
+        };
+      };
+    };
-- 
2.30.2

[PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Dumitru Ceclan]

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

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 drivers/iio/adc/Kconfig  |   7 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad717x.c | 999 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 1007 insertions(+)
 create mode 100644 drivers/iio/adc/ad717x.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dc14bde31ac1..294a48b05769 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -54,6 +54,13 @@ config AD7124
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad7124.
 
+config AD717X
+	tristate "Analog Devices AD717x driver"
+	depends on SPI_MASTER
+	select AD_SIGMA_DELTA
+	help
+	  Say yes here to build support for Analog Devices AD717x ADC.
+
 config AD7192
 	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index eb6e891790fb..e9491e4eff8d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
 obj-$(CONFIG_AD7124) += ad7124.o
+obj-$(CONFIG_AD717X) += ad717x.o
 obj-$(CONFIG_AD7192) += ad7192.o
 obj-$(CONFIG_AD7266) += ad7266.o
 obj-$(CONFIG_AD7280) += ad7280a.o
diff --git a/drivers/iio/adc/ad717x.c b/drivers/iio/adc/ad717x.c
new file mode 100644
index 000000000000..d14a3e0e2d93
--- /dev/null
+++ b/drivers/iio/adc/ad717x.c
@@ -0,0 +1,999 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
+ * Copyright (C) 2023 Analog Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+#include <linux/gpio/driver.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/adc/ad_sigma_delta.h>
+
+#define AD717X_REG_COMMS		0x00
+#define AD717X_REG_ADC_MODE		0x01
+#define AD717X_REG_INTERFACE_MODE	0x02
+#define AD717X_REG_CRC			0x03
+#define AD717X_REG_DATA			0x04
+#define AD717X_REG_GPIO			0x06
+#define AD717X_REG_ID			0x07
+#define AD717X_REG_CH(x)		(0x10 + (x))
+#define AD717X_REG_SETUP(x)		(0x20 + (x))
+#define AD717X_REG_FILTER(x)		(0x28 + (x))
+#define AD717X_REG_OFFSET(x)		(0x30 + (x))
+#define AD717X_REG_GAIN(x)		(0x38 + (x))
+
+#define AD717X_CH_ENABLE		BIT(15)
+#define AD717X_CH_SETUP_SEL(x)		((x) << 12)
+#define AD717X_CH_SETUP_AINPOS(x)	((x) << 5)
+#define AD717X_CH_SETUP_AINNEG(x)	(x)
+
+#define AD717X_CH_ADDRESS(pos, neg) \
+	(AD717X_CH_SETUP_AINPOS(pos) | AD717X_CH_SETUP_AINNEG(neg))
+
+#define AD7172_ID			0x00d0
+#define AD7173_ID			0x30d0
+#define AD7175_ID			0x0cd0
+#define AD7176_ID			0x0c90
+#define AD717X_ID_MASK			0xfff0
+
+#define AD717X_ADC_MODE_REF_EN		BIT(15)
+#define AD717X_ADC_MODE_SING_CYC	BIT(13)
+#define AD717X_ADC_MODE_MODE_MASK	0x70
+#define AD717X_ADC_MODE_MODE(x)		((x) << 4)
+#define AD717X_ADC_MODE_CLOCKSEL_MASK	0xc
+#define AD717X_ADC_MODE_CLOCKSEL(x)	((x) << 2)
+
+#define AD717X_GPIO_PDSW	BIT(14)
+#define AD717X_GPIO_OP_EN2_3	BIT(13)
+#define AD717X_GPIO_MUX_IO	BIT(12)
+#define AD717X_GPIO_SYNC_EN	BIT(11)
+#define AD717X_GPIO_ERR_EN	BIT(10)
+#define AD717X_GPIO_ERR_DAT	BIT(9)
+#define AD717X_GPIO_GP_DATA3	BIT(7)
+#define AD717X_GPIO_GP_DATA2	BIT(6)
+#define AD717X_GPIO_IP_EN1	BIT(5)
+#define AD717X_GPIO_IP_EN0	BIT(4)
+#define AD717X_GPIO_OP_EN1	BIT(3)
+#define AD717X_GPIO_OP_EN0	BIT(2)
+#define AD717X_GPIO_GP_DATA1	BIT(1)
+#define AD717X_GPIO_GP_DATA0	BIT(0)
+
+#define AD717X_INTERFACE_DATA_STAT	BIT(6)
+#define AD717X_INTERFACE_DATA_STAT_EN(x)\
+	FIELD_PREP(AD717X_INTERFACE_DATA_STAT, x)
+
+#define AD717X_SETUP_BIPOLAR		BIT(12)
+#define AD717X_SETUP_AREF_BUF		(0x3 << 10)
+#define AD717X_SETUP_AIN_BUF		(0x3 << 8)
+#define AD717X_SETUP_REF_SEL_MASK	0x30
+#define AD717X_SETUP_REF_SEL_AVDD1_AVSS	0x30
+#define AD717X_SETUP_REF_SEL_INT_REF	0x20
+#define AD717X_SETUP_REF_SEL_EXT_REF2	0x10
+#define AD717X_SETUP_REF_SEL_EXT_REF	0x00
+
+#define AD717X_FILTER_ODR0_MASK		0x1f
+#define AD717X_MAX_CONFIGS		8
+
+enum ad717x_ids {
+	ID_AD7172_2,
+	ID_AD7173_8,
+	ID_AD7175_2,
+	ID_AD7176_2,
+};
+
+struct ad717x_device_info {
+	unsigned int id;
+	unsigned int num_inputs;
+	unsigned int num_channels;
+	unsigned int num_configs;
+	bool has_gp23;
+	bool has_temp;
+	unsigned int clock;
+
+	const unsigned int *sinc5_data_rates;
+	unsigned int num_sinc5_data_rates;
+};
+
+struct ad717x_channel_config {
+	bool live;
+	unsigned char cfg_slot;
+	/* Fields from this point are used to compare equality of configs */
+	unsigned char odr;
+	bool bipolar;
+	bool input_buf;
+};
+
+struct ad717x_channel {
+	unsigned int chan_reg;
+	struct ad717x_channel_config cfg;
+	unsigned int ain;
+};
+
+struct ad717x_state {
+	const struct ad717x_device_info *info;
+	struct ad_sigma_delta sd;
+	struct ad717x_channel *channels;
+	struct regulator *reg;
+	unsigned int adc_mode;
+	unsigned int interface_mode;
+	unsigned int num_channels;
+	struct mutex cfgs_lock; /* lock for configs access */
+	unsigned long cfg_slots_status; /* slots usage status bitmap*/
+	unsigned long long config_usage_counter;
+	unsigned long long *config_cnts;
+
+#ifdef CONFIG_GPIOLIB
+	struct gpio_chip gpiochip;
+	unsigned int gpio_reg;
+	unsigned int gpio_23_mask;
+#endif
+};
+
+static const unsigned int ad7173_sinc5_data_rates[] = {
+	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,
+	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,
+	49680,	 20010,	  16333,   10000,   5000,    2500,    1250,
+};
+
+static const unsigned int ad7175_sinc5_data_rates[] = {
+	50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000,
+	10000000, 5000000,  2500000,  1000000,	500000,	  397500,   200000,
+	100000,	  59920,    49960,    20000,	16666,	  10000,    5000,
+};
+
+static const struct ad717x_device_info ad717x_device_info[] = {
+	[ID_AD7172_2] = {
+		.id = AD7172_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.has_gp23 = false,
+		.has_temp = true,
+		.clock = 2000000,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD7173_8] = {
+		.id = AD7173_ID,
+		.num_inputs = 17,
+		.num_channels = 16,
+		.num_configs = 8,
+		.has_gp23 = true,
+		.has_temp = true,
+		.clock = 2000000,
+		.sinc5_data_rates = ad7173_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+	},
+	[ID_AD7175_2] = {
+		.id = AD7175_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.has_gp23 = false,
+		.has_temp = true,
+		.clock = 16000000,
+		.sinc5_data_rates = ad7175_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+	},
+	[ID_AD7176_2] = {
+		.id = AD7176_ID,
+		.num_inputs = 5,
+		.num_channels = 4,
+		.num_configs = 4,
+		.has_gp23 = false,
+		.has_temp = false,
+		.clock = 16000000,
+		.sinc5_data_rates = ad7175_sinc5_data_rates,
+		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+	},
+};
+
+#ifdef CONFIG_GPIOLIB
+
+static struct ad717x_state *gpiochip_to_ad717x(struct gpio_chip *chip)
+{
+	return container_of(chip, struct ad717x_state, gpiochip);
+}
+
+static int ad717x_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct ad717x_state *st = gpiochip_to_ad717x(chip);
+	unsigned int mask;
+	unsigned int value;
+	int ret;
+
+	switch (offset) {
+	case 0:
+		mask = AD717X_GPIO_GP_DATA0;
+		break;
+	case 1:
+		mask = AD717X_GPIO_GP_DATA1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = ad_sd_read_reg(&st->sd, AD717X_REG_GPIO, 2, &value);
+	if (ret)
+		return ret;
+
+	return (bool)(value & mask);
+}
+
+static int ad717x_gpio_update(struct ad717x_state *st, unsigned int set_mask,
+			      unsigned int clr_mask)
+{
+	st->gpio_reg |= set_mask;
+	st->gpio_reg &= ~clr_mask;
+
+	return ad_sd_write_reg(&st->sd, AD717X_REG_GPIO, 2, st->gpio_reg);
+}
+
+static void ad717x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct ad717x_state *st = gpiochip_to_ad717x(chip);
+	unsigned int mask, set_mask, clr_mask;
+
+	switch (offset) {
+	case 0:
+		mask = AD717X_GPIO_GP_DATA0;
+		break;
+	case 1:
+		mask = AD717X_GPIO_GP_DATA1;
+		break;
+	case 2:
+		mask = AD717X_GPIO_GP_DATA2;
+		break;
+	case 3:
+		mask = AD717X_GPIO_GP_DATA3;
+		break;
+	default:
+		return;
+	}
+
+	if (value) {
+		set_mask = mask;
+		clr_mask = 0;
+	} else {
+		set_mask = 0;
+		clr_mask = mask;
+	}
+
+	ad717x_gpio_update(st, set_mask, clr_mask);
+}
+
+static int ad717x_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct ad717x_state *st = gpiochip_to_ad717x(chip);
+	unsigned int mask;
+
+	switch (offset) {
+	case 0:
+		mask = AD717X_GPIO_IP_EN0;
+		break;
+	case 1:
+		mask = AD717X_GPIO_IP_EN1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ad717x_gpio_update(st, mask, 0);
+}
+
+static int ad717x_gpio_direction_output
+	(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct ad717x_state *st = gpiochip_to_ad717x(chip);
+	unsigned int set_mask, clr_mask, val_mask;
+
+	switch (offset) {
+	case 0:
+		set_mask = AD717X_GPIO_OP_EN0;
+		val_mask = AD717X_GPIO_GP_DATA0;
+		break;
+	case 1:
+		set_mask = AD717X_GPIO_OP_EN1;
+		val_mask = AD717X_GPIO_GP_DATA1;
+		break;
+	/* GP2 and GP3 can not be enabled independently */
+	case 2:
+		st->gpio_23_mask |= (1 << 2);
+		set_mask = AD717X_GPIO_OP_EN2_3;
+		val_mask = AD717X_GPIO_GP_DATA2;
+		break;
+	case 3:
+		st->gpio_23_mask |= (1 << 3);
+		set_mask = AD717X_GPIO_OP_EN2_3;
+		val_mask = AD717X_GPIO_GP_DATA3;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (value) {
+		set_mask |= val_mask;
+		clr_mask = 0;
+	} else {
+		clr_mask = val_mask;
+	}
+
+	return ad717x_gpio_update(st, set_mask, clr_mask);
+}
+
+static void ad717x_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+	struct ad717x_state *st = gpiochip_to_ad717x(chip);
+	unsigned int mask;
+
+	switch (offset) {
+	case 0:
+		mask = AD717X_GPIO_OP_EN0 | AD717X_GPIO_IP_EN0;
+		break;
+	case 1:
+		mask = AD717X_GPIO_OP_EN1 | AD717X_GPIO_IP_EN1;
+		break;
+	case 2:
+		st->gpio_23_mask &= ~(1 << offset);
+		if (st->gpio_23_mask != 0)
+			return;
+		mask = AD717X_GPIO_OP_EN2_3;
+		break;
+	default:
+		return;
+	}
+
+	ad717x_gpio_update(st, 0, mask);
+}
+
+static int ad717x_gpio_init(struct ad717x_state *st)
+{
+	st->gpiochip.label = dev_name(&st->sd.spi->dev);
+	st->gpiochip.base = -1;
+	if (st->info->has_gp23)
+		st->gpiochip.ngpio = 4;
+	else
+		st->gpiochip.ngpio = 2;
+	st->gpiochip.parent = &st->sd.spi->dev;
+	st->gpiochip.can_sleep = true;
+	st->gpiochip.direction_input = ad717x_gpio_direction_input;
+	st->gpiochip.direction_output = ad717x_gpio_direction_output;
+	st->gpiochip.get = ad717x_gpio_get;
+	st->gpiochip.set = ad717x_gpio_set;
+	st->gpiochip.free = ad717x_gpio_free;
+	st->gpiochip.owner = THIS_MODULE;
+
+	return devm_gpiochip_add_data(&st->sd.spi->dev, &st->gpiochip, NULL);
+}
+
+#else
+
+static int ad717x_gpio_init(struct ad717x_state *st) { return 0 };
+static void ad717x_gpio_cleanup(struct ad717x_state *st) {};
+
+#endif
+
+static struct ad717x_state *ad_sigma_delta_to_ad717x(struct ad_sigma_delta *sd)
+{
+	return container_of(sd, struct ad717x_state, sd);
+}
+
+static void ad717x_reset_usage_cnts(struct ad717x_state *st)
+{
+	int i;
+
+	for (i = 0; i < st->info->num_configs; i++)
+		(st->config_cnts)[i] = 0;
+
+	st->config_usage_counter = 0;
+}
+
+static struct ad717x_channel_config *ad717x_find_live_config
+	(struct ad717x_state *st, struct ad717x_channel_config *cfg)
+{
+	struct ad717x_channel_config *cfg_aux;
+	ptrdiff_t cmp_size, offset;
+	int i;
+
+	offset = offsetof(struct ad717x_channel_config, cfg_slot) +
+		 sizeof(cfg->cfg_slot);
+	cmp_size = sizeof(*cfg) - offset;
+
+	for (i = 0; i < st->num_channels; i++) {
+		cfg_aux = &st->channels[i].cfg;
+
+		if (cfg_aux->live && !memcmp(((void *)cfg) + offset,
+					    ((void *)cfg_aux) + offset, cmp_size))
+			return cfg_aux;
+	}
+	return NULL;
+}
+
+static int ad717x_free_config_slot_lru(struct ad717x_state *st)
+{
+	int i, lru_position = 0;
+
+	for (i = 1; i < st->info->num_configs; i++)
+		if (st->config_cnts[i] < st->config_cnts[lru_position])
+			lru_position = i;
+
+	for (i = 0; i < st->num_channels; i++)
+		if (st->channels[i].cfg.cfg_slot == lru_position)
+			st->channels[i].cfg.live = false;
+
+	assign_bit(lru_position, &st->cfg_slots_status, 0);
+	return lru_position;
+}
+
+static int ad717x_load_config(struct ad717x_state *st,
+			      struct ad717x_channel_config *cfg)
+{
+	unsigned int config;
+	int free_cfg_slot, ret;
+
+	free_cfg_slot = find_first_zero_bit(&st->cfg_slots_status,
+					    st->info->num_configs);
+	if (free_cfg_slot == st->info->num_configs)
+		free_cfg_slot = ad717x_free_config_slot_lru(st);
+
+	assign_bit(free_cfg_slot, &st->cfg_slots_status, 1);
+	cfg->cfg_slot = free_cfg_slot;
+
+	config = AD717X_SETUP_REF_SEL_INT_REF;
+
+	if (cfg->bipolar)
+		config |= AD717X_SETUP_BIPOLAR;
+
+	if (cfg->input_buf)
+		config |= AD717X_SETUP_AIN_BUF;
+
+	ret = ad_sd_write_reg(&st->sd, AD717X_REG_SETUP(free_cfg_slot), 2, config);
+	if (ret)
+		return ret;
+
+	config = AD717X_FILTER_ODR0_MASK & cfg->odr;
+	return ad_sd_write_reg(&st->sd, AD717X_REG_FILTER(free_cfg_slot), 2, config);
+}
+
+static int ad717x_config_channel(struct ad717x_state *st, int addr)
+{
+	struct ad717x_channel_config *cfg = &st->channels[addr].cfg;
+	struct ad717x_channel_config *live_cfg;
+	int ret = 0;
+
+	if (!cfg->live) {
+		live_cfg = ad717x_find_live_config(st, cfg);
+		if (!live_cfg) {
+			ret = ad717x_load_config(st, cfg);
+			if (ret < 0)
+				return ret;
+		} else {
+			cfg->cfg_slot = live_cfg->cfg_slot;
+		}
+	}
+
+	cfg->live = true;
+	if (st->config_usage_counter == U64_MAX)
+		ad717x_reset_usage_cnts(st);
+
+	st->config_usage_counter++;
+	st->config_cnts[cfg->cfg_slot] = st->config_usage_counter;
+
+	return 0;
+}
+
+static int ad717x_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
+{
+	struct ad717x_state *st = ad_sigma_delta_to_ad717x(sd);
+	unsigned int val;
+	int ret;
+
+	ret = ad717x_config_channel(st, channel);
+	if (ret < 0)
+		return ret;
+
+	val = AD717X_CH_ENABLE |
+	      AD717X_CH_SETUP_SEL(st->channels[channel].cfg.cfg_slot) |
+	      st->channels[channel].ain;
+
+	return ad_sd_write_reg(&st->sd, AD717X_REG_CH(channel), 2, val);
+}
+
+static int ad717x_set_mode(struct ad_sigma_delta *sd,
+			   enum ad_sigma_delta_mode mode)
+{
+	struct ad717x_state *st = ad_sigma_delta_to_ad717x(sd);
+
+	st->adc_mode &= ~AD717X_ADC_MODE_MODE_MASK;
+	st->adc_mode |= AD717X_ADC_MODE_MODE(mode);
+
+	return ad_sd_write_reg(&st->sd, AD717X_REG_ADC_MODE, 2, st->adc_mode);
+}
+
+static int ad717x_append_status(struct ad_sigma_delta *sd, bool append)
+{
+	struct ad717x_state *st = container_of(sd, struct ad717x_state, sd);
+	unsigned int interface_mode = st->interface_mode;
+	int ret;
+
+	interface_mode |= AD717X_INTERFACE_DATA_STAT_EN(append);
+	ret = ad_sd_write_reg(&st->sd, AD717X_REG_INTERFACE_MODE, 2, interface_mode);
+	if (ret < 0)
+		return ret;
+
+	st->interface_mode = interface_mode;
+
+	return 0;
+}
+
+static int ad717x_disable_all(struct ad_sigma_delta *sd)
+{
+	struct ad717x_state *st = container_of(sd, struct ad717x_state, sd);
+	int ret;
+	int i;
+
+	for (i = 0; i < st->num_channels; i++) {
+		ret = ad_sd_write_reg(sd, AD717X_REG_CH(i), 2, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct ad_sigma_delta_info ad717x_sigma_delta_info = {
+	.set_channel = ad717x_set_channel,
+	.append_status = ad717x_append_status,
+	.disable_all = ad717x_disable_all,
+	.set_mode = ad717x_set_mode,
+	.has_registers = true,
+	.addr_shift = 0,
+	.read_mask = BIT(6),
+	.status_ch_mask = GENMASK(3, 0),
+	.data_reg = AD717X_REG_DATA,
+	.irq_flags = IRQF_TRIGGER_FALLING
+};
+
+static int ad717x_setup(struct iio_dev *indio_dev)
+{
+	struct ad717x_state *st = iio_priv(indio_dev);
+	unsigned int id;
+	u8 *buf;
+	int ret;
+
+	/* reset the serial interface */
+	buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	memset(buf, 0xff, 8);
+	ret = spi_write(st->sd.spi, buf, 8);
+	kfree(buf);
+	if (ret < 0)
+		return ret;
+
+	/* datasheet recommends a delay of at least 500us after reset */
+	usleep_range(500, 1000);
+
+	ret = ad_sd_read_reg(&st->sd, AD717X_REG_ID, 2, &id);
+	if (ret)
+		return ret;
+
+	id &= AD717X_ID_MASK;
+	if (id != st->info->id)
+		dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected: %x\n",
+					    id, st->info->id);
+
+	mutex_init(&st->cfgs_lock);
+	st->adc_mode |= AD717X_ADC_MODE_REF_EN | AD717X_ADC_MODE_SING_CYC;
+	st->interface_mode = 0x0;
+
+	st->config_usage_counter = 0;
+	st->config_cnts = devm_kzalloc(&indio_dev->dev,
+				       st->info->num_configs * sizeof(u64),
+				       GFP_KERNEL);
+	if (!st->config_cnts)
+		return -ENOMEM;
+
+	/* All channels are enabled by default after a reset */
+	ret = ad717x_disable_all(&st->sd);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int ad717x_read_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int *val, int *val2, long info)
+{
+	struct ad717x_state *st = iio_priv(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
+		if (ret < 0)
+			return ret;
+
+		/* disable channel after single conversion */
+		ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(chan->address), 2,
+				      0);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_TEMP) {
+			*val = 250000000;
+			*val2 = 800273203; /* (2**24 * 477) / 10 */
+			return IIO_VAL_FRACTIONAL;
+		} else {
+			*val = 2500;
+			if (chan->differential)
+				*val2 = 23;
+			else
+				*val2 = 24;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		if (chan->type == IIO_TEMP) {
+			*val = -874379;
+		} else {
+			if (chan->differential)
+				*val = -(1 << (chan->scan_type.realbits - 1));
+			else
+				*val = 0;
+		}
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		reg = st->channels[chan->address].cfg.odr;
+
+		*val = st->info->sinc5_data_rates[reg] / MILLI;
+		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI;
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+	return -EINVAL;
+}
+
+static int ad717x_write_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int val, int val2, long info)
+{
+	struct ad717x_state *st = iio_priv(indio_dev);
+	struct ad717x_channel_config *cfg;
+	unsigned int freq, i, reg;
+	int ret = 0;
+
+	mutex_lock(&st->cfgs_lock);
+	if (iio_buffer_enabled(indio_dev)) {
+		mutex_unlock(&st->cfgs_lock);
+		return -EBUSY;
+	}
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		freq = val * MILLI + val2 / MILLI;
+
+		for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
+			if (freq >= st->info->sinc5_data_rates[i])
+				break;
+		}
+
+		cfg = &st->channels[chan->address].cfg;
+		cfg->odr = i;
+
+		if (cfg->live) {
+			ret = ad_sd_read_reg(&st->sd, AD717X_REG_FILTER(cfg->cfg_slot), 2, &reg);
+			if (ret)
+				break;
+			reg &= ~AD717X_FILTER_ODR0_MASK;
+			reg |= i;
+			ret = ad_sd_write_reg(&st->sd, AD717X_REG_FILTER(cfg->cfg_slot), 2, reg);
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&st->cfgs_lock);
+	return ret;
+}
+
+static int ad717x_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, long mask)
+{
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad717x_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad717x_state *st = iio_priv(indio_dev);
+	bool bit_set;
+	int ret;
+	int i;
+
+	mutex_lock(&st->cfgs_lock);
+	for (i = 0; i < st->num_channels; i++) {
+		bit_set = test_bit(i, scan_mask);
+		if (bit_set)
+			ret = ad717x_set_channel(&st->sd, i);
+		else
+			ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(i), 2, 0);
+
+		if (ret < 0) {
+			mutex_unlock(&st->cfgs_lock);
+			return ret;
+		}
+	}
+
+	mutex_unlock(&st->cfgs_lock);
+
+	return 0;
+}
+
+static int ad717x_debug(struct iio_dev *indio_dev, unsigned int reg,
+			unsigned int writeval, unsigned int *readval)
+{
+	struct ad717x_state *st = iio_priv(indio_dev);
+	u8 reg_size = 2;
+
+	if (reg == 0)
+		reg_size = 1;
+	else if (reg == AD717X_REG_CRC || reg == AD717X_REG_DATA ||
+		 reg >= AD717X_REG_OFFSET(0))
+		reg_size = 3;
+
+	if (readval)
+		return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
+
+	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
+}
+
+static const struct iio_info ad717x_info = {
+	.read_raw = &ad717x_read_raw,
+	.write_raw = &ad717x_write_raw,
+	.write_raw_get_fmt = &ad717x_write_raw_get_fmt,
+	.debugfs_reg_access = &ad717x_debug,
+	.validate_trigger = ad_sd_validate_trigger,
+	.update_scan_mode = ad717x_update_scan_mode,
+};
+
+static const struct iio_chan_spec ad717x_channel_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.channel = 0,
+	.address = AD717X_CH_ADDRESS(0, 0),
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_index = 0,
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.shift = 0,
+		.endianness = IIO_BE,
+	},
+};
+
+static const struct iio_chan_spec ad717x_temp_iio_channel_template = {
+	.type = IIO_TEMP,
+	.indexed = 1,
+	.channel = 17,
+	.channel2 = 18,
+	.address = 0,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+	.scan_index = 0,
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.shift = 0,
+		.endianness = IIO_BE,
+	},
+};
+
+static int ad717x_of_parse_channel_config(struct iio_dev *indio_dev)
+{
+	struct ad717x_state *st = iio_priv(indio_dev);
+	struct ad717x_channel *channels_st_priv;
+	struct fwnode_handle *child;
+	struct device *dev = indio_dev->dev.parent;
+	struct iio_chan_spec *chan;
+	unsigned int num_channels = 0;
+	unsigned int ain[2], chan_index = 0;
+	bool use_temp = false;
+	int ret;
+
+	num_channels = device_get_child_node_count(dev);
+
+	if (device_property_read_bool(dev, "adi,temp-channel")) {
+		if (!st->info->has_temp) {
+			dev_err(indio_dev->dev.parent,
+				"Current chip does not support temperature channel");
+			return -EINVAL;
+		}
+
+		num_channels++;
+		use_temp = true;
+	}
+
+	st->num_channels = num_channels;
+
+	if (num_channels == 0)
+		return 0;
+
+	chan = devm_kcalloc(indio_dev->dev.parent, sizeof(*chan), num_channels,
+			    GFP_KERNEL);
+	if (!chan)
+		return -ENOMEM;
+
+	channels_st_priv = devm_kcalloc(indio_dev->dev.parent, num_channels,
+					sizeof(*channels_st_priv), GFP_KERNEL);
+	if (!channels_st_priv)
+		return -ENOMEM;
+
+	indio_dev->channels = chan;
+	indio_dev->num_channels = num_channels;
+	st->channels = channels_st_priv;
+
+	if (use_temp) {
+		chan[chan_index] = ad717x_temp_iio_channel_template;
+		channels_st_priv[chan_index].ain =
+			AD717X_CH_ADDRESS(chan[chan_index].channel, chan[chan_index].channel2);
+		channels_st_priv[chan_index].cfg.bipolar = false;
+		channels_st_priv[chan_index].cfg.input_buf = true;
+		chan_index++;
+	}
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32_array(child, "diff-channels", ain, 2);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		if (ain[0] >= st->info->num_inputs ||
+		    ain[1] >= st->info->num_inputs) {
+			dev_err(indio_dev->dev.parent,
+				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
+
+		chan[chan_index] = ad717x_channel_template;
+		chan[chan_index].address = chan_index;
+		chan[chan_index].scan_index = chan_index;
+		chan[chan_index].channel = ain[0];
+		chan[chan_index].channel2 = ain[1];
+
+		channels_st_priv[chan_index].ain =
+			AD717X_CH_ADDRESS(ain[0], ain[1]);
+		channels_st_priv[chan_index].chan_reg = chan_index;
+		channels_st_priv[chan_index].cfg.input_buf = true;
+		channels_st_priv[chan_index].cfg.odr = 0;
+
+		chan[chan_index].differential = fwnode_property_read_bool(child, "adi,bipolar");
+		if (chan[chan_index].differential) {
+			chan[chan_index].info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
+			channels_st_priv[chan_index].cfg.bipolar = true;
+		}
+
+		chan_index++;
+	}
+
+	return 0;
+}
+
+static int ad717x_probe(struct spi_device *spi)
+{
+	struct ad717x_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!spi->irq) {
+		dev_err(&spi->dev, "No IRQ specified\n");
+		return -ENODEV;
+	}
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->info = device_get_match_data(&spi->dev);
+	if (!st->info)
+		return -ENODEV;
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad717x_info;
+
+	ad717x_sigma_delta_info.num_slots = st->info->num_configs;
+	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad717x_sigma_delta_info);
+	if (ret < 0)
+		return ret;
+
+	spi_set_drvdata(spi, indio_dev);
+
+	ret = ad717x_of_parse_channel_config(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
+	if (ret < 0)
+		return ret;
+
+	ret = ad717x_setup(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	return ad717x_gpio_init(st);
+}
+
+static const struct of_device_id ad717x_of_match[] = {
+	{ .compatible = "adi,ad7172-2",
+	  .data = &ad717x_device_info[ID_AD7172_2] },
+	{ .compatible = "adi,ad7173-8",
+	  .data = &ad717x_device_info[ID_AD7173_8] },
+	{ .compatible = "adi,ad7175-2",
+	  .data = &ad717x_device_info[ID_AD7175_2] },
+	{ .compatible = "adi,ad7176-2",
+	  .data = &ad717x_device_info[ID_AD7176_2] },
+	{}
+}
+MODULE_DEVICE_TABLE(of, ad717x_of_match);
+
+static const struct spi_device_id ad717x_id_table[] = {
+	{ "ad7172-2", },
+	{ "ad7173-8", },
+	{ "ad7175-2", },
+	{ "ad7176-2", },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad717x_id_table);
+
+static struct spi_driver ad717x_driver = {
+	.driver = {
+		.name	= "ad717x",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(ad717x_of_match),
+	},
+	.probe		= ad717x_probe,
+	.id_table	= ad717x_id_table,
+};
+module_spi_driver(ad717x_driver);
+
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
+MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Nuno Sá]

Hi Dumitru,

Thanks for taking care of this driver which is out of tree for a long time... Some
comments below.

On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:
> The AD717x family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---
>  drivers/iio/adc/Kconfig  |   7 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad717x.c | 999 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1007 insertions(+)
>  create mode 100644 drivers/iio/adc/ad717x.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dc14bde31ac1..294a48b05769 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -54,6 +54,13 @@ config AD7124
>           To compile this driver as a module, choose M here: the module will be
>           called ad7124.
>  
> +config AD717X
> +       tristate "Analog Devices AD717x driver"
> +       depends on SPI_MASTER
> +       select AD_SIGMA_DELTA
> +       help
> +         Say yes here to build support for Analog Devices AD717x ADC.
> +
>  config AD7192
>         tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
>         depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index eb6e891790fb..e9491e4eff8d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
>  obj-$(CONFIG_AD7124) += ad7124.o
> +obj-$(CONFIG_AD717X) += ad717x.o
>  obj-$(CONFIG_AD7192) += ad7192.o
>  obj-$(CONFIG_AD7266) += ad7266.o
>  obj-$(CONFIG_AD7280) += ad7280a.o
> diff --git a/drivers/iio/adc/ad717x.c b/drivers/iio/adc/ad717x.c
> new file mode 100644
> index 000000000000..d14a3e0e2d93
> --- /dev/null
> +++ b/drivers/iio/adc/ad717x.c
> @@ -0,0 +1,999 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
> + * Copyright (C) 2023 Analog Devices, Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +
> +#define AD717X_REG_COMMS               0x00
> +#define AD717X_REG_ADC_MODE            0x01
> +#define AD717X_REG_INTERFACE_MODE      0x02
> +#define AD717X_REG_CRC                 0x03
> +#define AD717X_REG_DATA                        0x04
> +#define AD717X_REG_GPIO                        0x06
> +#define AD717X_REG_ID                  0x07
> +#define AD717X_REG_CH(x)               (0x10 + (x))
> +#define AD717X_REG_SETUP(x)            (0x20 + (x))
> +#define AD717X_REG_FILTER(x)           (0x28 + (x))
> +#define AD717X_REG_OFFSET(x)           (0x30 + (x))
> +#define AD717X_REG_GAIN(x)             (0x38 + (x))
> +
> +#define AD717X_CH_ENABLE               BIT(15)
> +#define AD717X_CH_SETUP_SEL(x)         ((x) << 12)
> +#define AD717X_CH_SETUP_AINPOS(x)      ((x) << 5)
> +#define AD717X_CH_SETUP_AINNEG(x)      (x)
> +
> +#define AD717X_CH_ADDRESS(pos, neg) \
> +       (AD717X_CH_SETUP_AINPOS(pos) | AD717X_CH_SETUP_AINNEG(neg))
> +
> +#define AD7172_ID                      0x00d0
> +#define AD7173_ID                      0x30d0
> +#define AD7175_ID                      0x0cd0
> +#define AD7176_ID                      0x0c90
> +#define AD717X_ID_MASK                 0xfff0
> +
> +#define AD717X_ADC_MODE_REF_EN         BIT(15)
> +#define AD717X_ADC_MODE_SING_CYC       BIT(13)
> +#define AD717X_ADC_MODE_MODE_MASK      0x70
> +#define AD717X_ADC_MODE_MODE(x)                ((x) << 4)
> +#define AD717X_ADC_MODE_CLOCKSEL_MASK  0xc
> +#define AD717X_ADC_MODE_CLOCKSEL(x)    ((x) << 2)
> +
> +#define AD717X_GPIO_PDSW       BIT(14)
> +#define AD717X_GPIO_OP_EN2_3   BIT(13)
> +#define AD717X_GPIO_MUX_IO     BIT(12)
> +#define AD717X_GPIO_SYNC_EN    BIT(11)
> +#define AD717X_GPIO_ERR_EN     BIT(10)
> +#define AD717X_GPIO_ERR_DAT    BIT(9)
> +#define AD717X_GPIO_GP_DATA3   BIT(7)
> +#define AD717X_GPIO_GP_DATA2   BIT(6)
> +#define AD717X_GPIO_IP_EN1     BIT(5)
> +#define AD717X_GPIO_IP_EN0     BIT(4)
> +#define AD717X_GPIO_OP_EN1     BIT(3)
> +#define AD717X_GPIO_OP_EN0     BIT(2)
> +#define AD717X_GPIO_GP_DATA1   BIT(1)
> +#define AD717X_GPIO_GP_DATA0   BIT(0)
> +
> +#define AD717X_INTERFACE_DATA_STAT     BIT(6)
> +#define AD717X_INTERFACE_DATA_STAT_EN(x)\
> +       FIELD_PREP(AD717X_INTERFACE_DATA_STAT, x)
> +
> +#define AD717X_SETUP_BIPOLAR           BIT(12)
> +#define AD717X_SETUP_AREF_BUF          (0x3 << 10)
> +#define AD717X_SETUP_AIN_BUF           (0x3 << 8)
> +#define AD717X_SETUP_REF_SEL_MASK      0x30
> +#define AD717X_SETUP_REF_SEL_AVDD1_AVSS        0x30
> +#define AD717X_SETUP_REF_SEL_INT_REF   0x20
> +#define AD717X_SETUP_REF_SEL_EXT_REF2  0x10
> +#define AD717X_SETUP_REF_SEL_EXT_REF   0x00
> +
> +#define AD717X_FILTER_ODR0_MASK                0x1f
> +#define AD717X_MAX_CONFIGS             8
> +
> +enum ad717x_ids {
> +       ID_AD7172_2,
> +       ID_AD7173_8,
> +       ID_AD7175_2,
> +       ID_AD7176_2,
> +};
> +
> +struct ad717x_device_info {
> +       unsigned int id;
> +       unsigned int num_inputs;
> +       unsigned int num_channels;
> +       unsigned int num_configs;
> +       bool has_gp23;
> +       bool has_temp;
> +       unsigned int clock;
> +
> +       const unsigned int *sinc5_data_rates;
> +       unsigned int num_sinc5_data_rates;
> +};
> +
> +struct ad717x_channel_config {
> +       bool live;
> +       unsigned char cfg_slot;
> +       /* Fields from this point are used to compare equality of configs */
> +       unsigned char odr;
> +       bool bipolar;
> +       bool input_buf;
> +};
> +
> +struct ad717x_channel {
> +       unsigned int chan_reg;
> +       struct ad717x_channel_config cfg;
> +       unsigned int ain;
> +};
> +
> +struct ad717x_state {
> +       const struct ad717x_device_info *info;
> +       struct ad_sigma_delta sd;
> +       struct ad717x_channel *channels;
> +       struct regulator *reg;
> +       unsigned int adc_mode;
> +       unsigned int interface_mode;
> +       unsigned int num_channels;
> +       struct mutex cfgs_lock; /* lock for configs access */
> +       unsigned long cfg_slots_status; /* slots usage status bitmap*/
> +       unsigned long long config_usage_counter;
> +       unsigned long long *config_cnts;
> +
> +#ifdef CONFIG_GPIOLIB
> +       struct gpio_chip gpiochip;
> +       unsigned int gpio_reg;
> +       unsigned int gpio_23_mask;
> +#endif
> +};
> +
> +static const unsigned int ad7173_sinc5_data_rates[] = {
> +       6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,
> +       3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,
> +       49680,   20010,   16333,   10000,   5000,    2500,    1250,
> +};
> +
> +static const unsigned int ad7175_sinc5_data_rates[] = {
> +       50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000,
> +       10000000, 5000000,  2500000,  1000000,  500000,   397500,   200000,
> +       100000,   59920,    49960,    20000,    16666,    10000,    5000,
> +};
> +
> +static const struct ad717x_device_info ad717x_device_info[] = {
> +       [ID_AD7172_2] = {
> +               .id = AD7172_ID,
> +               .num_inputs = 5,
> +               .num_channels = 4,
> +               .num_configs = 4,
> +               .has_gp23 = false,
> +               .has_temp = true,
> +               .clock = 2000000,
> +               .sinc5_data_rates = ad7173_sinc5_data_rates,
> +               .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> +       },
> +       [ID_AD7173_8] = {
> +               .id = AD7173_ID,
> +               .num_inputs = 17,
> +               .num_channels = 16,
> +               .num_configs = 8,
> +               .has_gp23 = true,
> +               .has_temp = true,
> +               .clock = 2000000,
> +               .sinc5_data_rates = ad7173_sinc5_data_rates,
> +               .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> +       },
> +       [ID_AD7175_2] = {
> +               .id = AD7175_ID,
> +               .num_inputs = 5,
> +               .num_channels = 4,
> +               .num_configs = 4,
> +               .has_gp23 = false,
> +               .has_temp = true,
> +               .clock = 16000000,
> +               .sinc5_data_rates = ad7175_sinc5_data_rates,
> +               .num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
> +       },
> +       [ID_AD7176_2] = {
> +               .id = AD7176_ID,
> +               .num_inputs = 5,
> +               .num_channels = 4,
> +               .num_configs = 4,
> +               .has_gp23 = false,
> +               .has_temp = false,
> +               .clock = 16000000,
> +               .sinc5_data_rates = ad7175_sinc5_data_rates,
> +               .num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
> +       },
> +};
> +
> +#ifdef CONFIG_GPIOLIB
> +
> +static struct ad717x_state *gpiochip_to_ad717x(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct ad717x_state, gpiochip);
> +}
> +
> +static int ad717x_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +       unsigned int mask;
> +       unsigned int value;
> +       int ret;
> +
> +       switch (offset) {
> +       case 0:
> +               mask = AD717X_GPIO_GP_DATA0;
> +               break;
> +       case 1:
> +               mask = AD717X_GPIO_GP_DATA1;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       ret = ad_sd_read_reg(&st->sd, AD717X_REG_GPIO, 2, &value);
> +       if (ret)
> +               return ret;
> +
> +       return (bool)(value & mask);
> +}
> +
> +static int ad717x_gpio_update(struct ad717x_state *st, unsigned int set_mask,
> +                             unsigned int clr_mask)
> +{
> +       st->gpio_reg |= set_mask;
> +       st->gpio_reg &= ~clr_mask;
> +
> +       return ad_sd_write_reg(&st->sd, AD717X_REG_GPIO, 2, st->gpio_reg);
> +}
> +
> +static void ad717x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +       unsigned int mask, set_mask, clr_mask;
> +
> +       switch (offset) {
> +       case 0:
> +               mask = AD717X_GPIO_GP_DATA0;
> +               break;
> +       case 1:
> +               mask = AD717X_GPIO_GP_DATA1;
> +               break;
> +       case 2:
> +               mask = AD717X_GPIO_GP_DATA2;
> +               break;
> +       case 3:
> +               mask = AD717X_GPIO_GP_DATA3;
> +               break;
> +       default:
> +               return;
> +       }
> +
> +       if (value) {
> +               set_mask = mask;
> +               clr_mask = 0;
> +       } else {
> +               set_mask = 0;
> +               clr_mask = mask;
> +       }
> +
> +       ad717x_gpio_update(st, set_mask, clr_mask);
> +}
> +
> +static int ad717x_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +       unsigned int mask;
> +
> +       switch (offset) {
> +       case 0:
> +               mask = AD717X_GPIO_IP_EN0;
> +               break;
> +       case 1:
> +               mask = AD717X_GPIO_IP_EN1;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return ad717x_gpio_update(st, mask, 0);
> +}
> +
> +static int ad717x_gpio_direction_output
> +       (struct gpio_chip *chip, unsigned offset, int value)
> +{
> +       struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +       unsigned int set_mask, clr_mask, val_mask;
> +
> +       switch (offset) {
> +       case 0:
> +               set_mask = AD717X_GPIO_OP_EN0;
> +               val_mask = AD717X_GPIO_GP_DATA0;
> +               break;
> +       case 1:
> +               set_mask = AD717X_GPIO_OP_EN1;
> +               val_mask = AD717X_GPIO_GP_DATA1;
> +               break;
> +       /* GP2 and GP3 can not be enabled independently */
> +       case 2:
> +               st->gpio_23_mask |= (1 << 2);
> +               set_mask = AD717X_GPIO_OP_EN2_3;
> +               val_mask = AD717X_GPIO_GP_DATA2;
> +               break;
> +       case 3:
> +               st->gpio_23_mask |= (1 << 3);
> +               set_mask = AD717X_GPIO_OP_EN2_3;
> +               val_mask = AD717X_GPIO_GP_DATA3;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       if (value) {
> +               set_mask |= val_mask;
> +               clr_mask = 0;
> +       } else {
> +               clr_mask = val_mask;
> +       }
> +
> +       return ad717x_gpio_update(st, set_mask, clr_mask);
> +}
> +
> +static void ad717x_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +       unsigned int mask;
> +
> +       switch (offset) {
> +       case 0:
> +               mask = AD717X_GPIO_OP_EN0 | AD717X_GPIO_IP_EN0;
> +               break;
> +       case 1:
> +               mask = AD717X_GPIO_OP_EN1 | AD717X_GPIO_IP_EN1;
> +               break;
> +       case 2:
> +               st->gpio_23_mask &= ~(1 << offset);
> +               if (st->gpio_23_mask != 0)
> +                       return;
> +               mask = AD717X_GPIO_OP_EN2_3;
> +               break;
> +       default:
> +               return;
> +       }
> +
> +       ad717x_gpio_update(st, 0, mask);
> +}
> +
> +static int ad717x_gpio_init(struct ad717x_state *st)
> +{
> +       st->gpiochip.label = dev_name(&st->sd.spi->dev);
> +       st->gpiochip.base = -1;
> +       if (st->info->has_gp23)
> +               st->gpiochip.ngpio = 4;
> +       else
> +               st->gpiochip.ngpio = 2;
> +       st->gpiochip.parent = &st->sd.spi->dev;
> +       st->gpiochip.can_sleep = true;
> +       st->gpiochip.direction_input = ad717x_gpio_direction_input;
> +       st->gpiochip.direction_output = ad717x_gpio_direction_output;
> +       st->gpiochip.get = ad717x_gpio_get;
> +       st->gpiochip.set = ad717x_gpio_set;
> +       st->gpiochip.free = ad717x_gpio_free;
> +       st->gpiochip.owner = THIS_MODULE;
> +
> +       return devm_gpiochip_add_data(&st->sd.spi->dev, &st->gpiochip, NULL);
> +}
> +
> +#else
> +
> +static int ad717x_gpio_init(struct ad717x_state *st) { return 0 };
> +static void ad717x_gpio_cleanup(struct ad717x_state *st) {};

Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just get rid of
the #ifdef wrapper and just select GPIOLIB. How often will it be disabled anyways?

Alternatively, you can just wrap ad717x_gpio_init() and only calls it 'if
(IS_ENABLED(CONFIG_GPIOLIB))... Then, the compiler can figure the rest. Of course you
would still have some extra bytes in your struct. Anyways, I just like to avoid this
#ifdefery.

> +
> +#endif
> +
> +static struct ad717x_state *ad_sigma_delta_to_ad717x(struct ad_sigma_delta *sd)
> +{
> +       return container_of(sd, struct ad717x_state, sd);
> +}
> +
> +static void ad717x_reset_usage_cnts(struct ad717x_state *st)
> +{
> +       int i;
> +
> +       for (i = 0; i < st->info->num_configs; i++)
> +               (st->config_cnts)[i] = 0;
> +
> +       st->config_usage_counter = 0;
> +}
> +
> +static struct ad717x_channel_config *ad717x_find_live_config
> +       (struct ad717x_state *st, struct ad717x_channel_config *cfg)
> +{
> +       struct ad717x_channel_config *cfg_aux;
> +       ptrdiff_t cmp_size, offset;
> +       int i;
> +
> +       offset = offsetof(struct ad717x_channel_config, cfg_slot) +
> +                sizeof(cfg->cfg_slot);
> +       cmp_size = sizeof(*cfg) - offset;
> +
> +       for (i = 0; i < st->num_channels; i++) {
> +               cfg_aux = &st->channels[i].cfg;
> +
> +               if (cfg_aux->live && !memcmp(((void *)cfg) + offset,
> +                                           ((void *)cfg_aux) + offset, cmp_size))
> +                       return cfg_aux;
> +       }
> +       return NULL;
> +}
> +
> +static int ad717x_free_config_slot_lru(struct ad717x_state *st)
> +{
> +       int i, lru_position = 0;
> +
> +       for (i = 1; i < st->info->num_configs; i++)
> +               if (st->config_cnts[i] < st->config_cnts[lru_position])
> +                       lru_position = i;
> +
> +       for (i = 0; i < st->num_channels; i++)
> +               if (st->channels[i].cfg.cfg_slot == lru_position)
> +                       st->channels[i].cfg.live = false;
> +
> +       assign_bit(lru_position, &st->cfg_slots_status, 0);
> +       return lru_position;
> +}
> +
> +static int ad717x_load_config(struct ad717x_state *st,
> +                             struct ad717x_channel_config *cfg)
> +{
> +       unsigned int config;
> +       int free_cfg_slot, ret;
> +
> +       free_cfg_slot = find_first_zero_bit(&st->cfg_slots_status,
> +                                           st->info->num_configs);
> +       if (free_cfg_slot == st->info->num_configs)
> +               free_cfg_slot = ad717x_free_config_slot_lru(st);
> +
> +       assign_bit(free_cfg_slot, &st->cfg_slots_status, 1);
> +       cfg->cfg_slot = free_cfg_slot;
> +
> +       config = AD717X_SETUP_REF_SEL_INT_REF;
> +
> +       if (cfg->bipolar)
> +               config |= AD717X_SETUP_BIPOLAR;
> +
> +       if (cfg->input_buf)
> +               config |= AD717X_SETUP_AIN_BUF;
> +
> +       ret = ad_sd_write_reg(&st->sd, AD717X_REG_SETUP(free_cfg_slot), 2, config);
> +       if (ret)
> +               return ret;
> +
> +       config = AD717X_FILTER_ODR0_MASK & cfg->odr;
> +       return ad_sd_write_reg(&st->sd, AD717X_REG_FILTER(free_cfg_slot), 2,
> config);
> +}
> +
> +static int ad717x_config_channel(struct ad717x_state *st, int addr)
> +{
> +       struct ad717x_channel_config *cfg = &st->channels[addr].cfg;
> +       struct ad717x_channel_config *live_cfg;
> +       int ret = 0;
> +
> +       if (!cfg->live) {
> +               live_cfg = ad717x_find_live_config(st, cfg);
> +               if (!live_cfg) {
> +                       ret = ad717x_load_config(st, cfg);
> +                       if (ret < 0)
> +                               return ret;
> +               } else {
> +                       cfg->cfg_slot = live_cfg->cfg_slot;
> +               }
> +       }
> +
> +       cfg->live = true;
> +       if (st->config_usage_counter == U64_MAX)
> +               ad717x_reset_usage_cnts(st);
> +
> +       st->config_usage_counter++;
> +       st->config_cnts[cfg->cfg_slot] = st->config_usage_counter;
> +
> +       return 0;
> +}

I guess you based yourself in the ad7124 driver for the above? Might be worthy look
at it more carefully to see if there's any nice way to push into the lib so code can
be shared (just some thoughts for future work if that sparks any interest :)).

> +
> +static int ad717x_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
> +{
> +       struct ad717x_state *st = ad_sigma_delta_to_ad717x(sd);
> +       unsigned int val;
> +       int ret;
> +
> +       ret = ad717x_config_channel(st, channel);
> +       if (ret < 0)
> +               return ret;
> +
> +       val = AD717X_CH_ENABLE |
> +             AD717X_CH_SETUP_SEL(st->channels[channel].cfg.cfg_slot) |
> +             st->channels[channel].ain;
> +
> +       return ad_sd_write_reg(&st->sd, AD717X_REG_CH(channel), 2, val);
> +}
> +
> +static int ad717x_set_mode(struct ad_sigma_delta *sd,
> +                          enum ad_sigma_delta_mode mode)
> +{
> +       struct ad717x_state *st = ad_sigma_delta_to_ad717x(sd);
> +
> +       st->adc_mode &= ~AD717X_ADC_MODE_MODE_MASK;
> +       st->adc_mode |= AD717X_ADC_MODE_MODE(mode);
> +
> +       return ad_sd_write_reg(&st->sd, AD717X_REG_ADC_MODE, 2, st->adc_mode);
> +}
> +
> +static int ad717x_append_status(struct ad_sigma_delta *sd, bool append)
> +{
> +       struct ad717x_state *st = container_of(sd, struct ad717x_state, sd);
> +       unsigned int interface_mode = st->interface_mode;
> +       int ret;
> +
> +       interface_mode |= AD717X_INTERFACE_DATA_STAT_EN(append);
> +       ret = ad_sd_write_reg(&st->sd, AD717X_REG_INTERFACE_MODE, 2,
> interface_mode);
> +       if (ret < 0)
> +               return ret;
> +
> +       st->interface_mode = interface_mode;
> +
> +       return 0;
> +}
> +
> +static int ad717x_disable_all(struct ad_sigma_delta *sd)
> +{
> +       struct ad717x_state *st = container_of(sd, struct ad717x_state, sd);
> +       int ret;
> +       int i;
> +
> +       for (i = 0; i < st->num_channels; i++) {
> +               ret = ad_sd_write_reg(sd, AD717X_REG_CH(i), 2, 0);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct ad_sigma_delta_info ad717x_sigma_delta_info = {
> +       .set_channel = ad717x_set_channel,
> +       .append_status = ad717x_append_status,
> +       .disable_all = ad717x_disable_all,
> +       .set_mode = ad717x_set_mode,
> +       .has_registers = true,
> +       .addr_shift = 0,
> +       .read_mask = BIT(6),
> +       .status_ch_mask = GENMASK(3, 0),
> +       .data_reg = AD717X_REG_DATA,
> +       .irq_flags = IRQF_TRIGGER_FALLING
> +};
> +
> +static int ad717x_setup(struct iio_dev *indio_dev)
> +{
> +       struct ad717x_state *st = iio_priv(indio_dev);
> +       unsigned int id;
> +       u8 *buf;
> +       int ret;
> +
> +       /* reset the serial interface */
> +       buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       memset(buf, 0xff, 8);
> +       ret = spi_write(st->sd.spi, buf, 8);
> +       kfree(buf);

Hmm, why allocating the buffer? I guess one could argue that we'll get DMA safe
alignment but then maybe use some define instead of the magic 8.

> +       if (ret < 0)
> +               return ret;
> +
> +       /* datasheet recommends a delay of at least 500us after reset */
> +       usleep_range(500, 1000);
> +
> +       ret = ad_sd_read_reg(&st->sd, AD717X_REG_ID, 2, &id);
> +       if (ret)
> +               return ret;
> +
> +       id &= AD717X_ID_MASK;
> +       if (id != st->info->id)
> +               dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected:
> %x\n",
> +                                           id, st->info->id);
> +

Shouldn't we error out?


> +       mutex_init(&st->cfgs_lock);
> +       st->adc_mode |= AD717X_ADC_MODE_REF_EN | AD717X_ADC_MODE_SING_CYC;
> +       st->interface_mode = 0x0;
> +
> +       st->config_usage_counter = 0;
> +       st->config_cnts = devm_kzalloc(&indio_dev->dev,
> +                                      st->info->num_configs * sizeof(u64),
> +                                      GFP_KERNEL);
> +       if (!st->config_cnts)
> +               return -ENOMEM;
> +
> +       /* All channels are enabled by default after a reset */
> +       ret = ad717x_disable_all(&st->sd);
> +       if (ret < 0)
> +               return ret;
> +

Just return ad717x_disable_all();

> +       return 0;
> +}
> +
> +static int ad717x_read_raw(struct iio_dev *indio_dev,
> +       struct iio_chan_spec const *chan, int *val, int *val2, long info)
> +{
> +       struct ad717x_state *st = iio_priv(indio_dev);
> +       unsigned int reg;
> +       int ret;
> +
> +       switch (info) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +               if (ret < 0)
> +                       return ret;
> +
> +               /* disable channel after single conversion */
> +               ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(chan->address), 2,
> +                                     0);

Hmm, since you're here... I think we should bring this into the sigma delta lib. So,
when sequencer is supported, the device samples all enabled channels one by one so I
get why you're doing this for a single conversion. However, note that
ad_sigma_delta_single_conversion() is already taking care of calling a callback so
the channel can be prepared and enabled for the conversion. So, it's only natural to
disable that channel inside ad_sigma_delta_single_conversion(). We already have a
.disable_all() option so adding one only with .disable() would make sense. Or an
extra parameter in .set_channel() callback but that would be more work and Im not
sure if it brings any benefit.

If you take care of the above, another thing to remember is to update the ad7124.c
driver which is doing the same thing.

> +               if (ret < 0)
> +                       return ret;
> +
> +               return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SCALE:
> +               if (chan->type == IIO_TEMP) {
> +                       *val = 250000000;
> +                       *val2 = 800273203; /* (2**24 * 477) / 10 */
> +                       return IIO_VAL_FRACTIONAL;
> +               } else {
> +                       *val = 2500;
> +                       if (chan->differential)
> +                               *val2 = 23;
> +                       else
> +                               *val2 = 24;
> +                       return IIO_VAL_FRACTIONAL_LOG2;
> +               }
> +       case IIO_CHAN_INFO_OFFSET:
> +               if (chan->type == IIO_TEMP) {
> +                       *val = -874379;
> +               } else {
> +                       if (chan->differential)
> +                               *val = -(1 << (chan->scan_type.realbits - 1));

nit: I don't expect the driver to really be updated with more devices (it's like this
for a long time) but the above is not very extensible... Imagine we add a device with
32bit channels? We would enter shady waters If I'm not missing anything.

> +                       else
> +                               *val = 0;
> +               }
> +               return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               reg = st->channels[chan->address].cfg.odr;
> +
> +               *val = st->info->sinc5_data_rates[reg] / MILLI;
> +               *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI;
> +
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       }
> +       return -EINVAL;
> +}
> +
> +static int ad717x_write_raw(struct iio_dev *indio_dev,
> +       struct iio_chan_spec const *chan, int val, int val2, long info)
> +{
> +       struct ad717x_state *st = iio_priv(indio_dev);
> +       struct ad717x_channel_config *cfg;
> +       unsigned int freq, i, reg;
> +       int ret = 0;
> +
> +       mutex_lock(&st->cfgs_lock);
> +       if (iio_buffer_enabled(indio_dev)) {
> +               mutex_unlock(&st->cfgs_lock);
> +               return -EBUSY;
> +       }
> +

iio_device_claim_direct_mode()

> +       switch (info) {
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               freq = val * MILLI + val2 / MILLI;
> +
> +               for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
> +                       if (freq >= st->info->sinc5_data_rates[i])
> +                               break;
> +               }
> +
> +               cfg = &st->channels[chan->address].cfg;
> +               cfg->odr = i;
> +
> +               if (cfg->live) {
> +                       ret = ad_sd_read_reg(&st->sd, AD717X_REG_FILTER(cfg-
> >cfg_slot), 2, &reg);
> +                       if (ret)
> +                               break;
> +                       reg &= ~AD717X_FILTER_ODR0_MASK;
> +                       reg |= i;
> +                       ret = ad_sd_write_reg(&st->sd, AD717X_REG_FILTER(cfg-
> >cfg_slot), 2, reg);
> +               }
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       mutex_unlock(&st->cfgs_lock);
> +       return ret;
> +}
> +
> +static int ad717x_write_raw_get_fmt(struct iio_dev *indio_dev,
> +                                   struct iio_chan_spec const *chan, long mask)
> +{
> +       return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad717x_update_scan_mode(struct iio_dev *indio_dev,
> +                                  const unsigned long *scan_mask)
> +{
> +       struct ad717x_state *st = iio_priv(indio_dev);
> +       bool bit_set;
> +       int ret;
> +       int i;
> +
> +       mutex_lock(&st->cfgs_lock);
> +       for (i = 0; i < st->num_channels; i++) {
> +               bit_set = test_bit(i, scan_mask);
> +               if (bit_set)

why not test_bit() directly?

> +                       ret = ad717x_set_channel(&st->sd, i);
> +               else
> +                       ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(i), 2, 0);
> +
> +               if (ret < 0) {
> +                       mutex_unlock(&st->cfgs_lock);
> +                       return ret;
> +               }
> +       }
> +
> +       mutex_unlock(&st->cfgs_lock);
> +
> +       return 0;
> +}
> +
> +static int ad717x_debug(struct iio_dev *indio_dev, unsigned int reg,
> +                       unsigned int writeval, unsigned int *readval)
> +{
> +       struct ad717x_state *st = iio_priv(indio_dev);
> +       u8 reg_size = 2;
> +
> +       if (reg == 0)
> +               reg_size = 1;
> +       else if (reg == AD717X_REG_CRC || reg == AD717X_REG_DATA ||
> +                reg >= AD717X_REG_OFFSET(0))
> +               reg_size = 3;
> +
> +       if (readval)
> +               return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
> +
> +       return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
> +}
> +
> +static const struct iio_info ad717x_info = {
> +       .read_raw = &ad717x_read_raw,
> +       .write_raw = &ad717x_write_raw,
> +       .write_raw_get_fmt = &ad717x_write_raw_get_fmt,
> +       .debugfs_reg_access = &ad717x_debug,
> +       .validate_trigger = ad_sd_validate_trigger,
> +       .update_scan_mode = ad717x_update_scan_mode,
> +};
> +
> +static const struct iio_chan_spec ad717x_channel_template = {
> +       .type = IIO_VOLTAGE,
> +       .indexed = 1,
> +       .channel = 0,
> +       .address = AD717X_CH_ADDRESS(0, 0),
> +       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +               BIT(IIO_CHAN_INFO_SCALE),
> +       .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +       .scan_index = 0,
> +       .scan_type = {
> +               .sign = 'u',
> +               .realbits = 24,
> +               .storagebits = 32,
> +               .shift = 0,
> +               .endianness = IIO_BE,
> +       },
> +};
> +
> +static const struct iio_chan_spec ad717x_temp_iio_channel_template = {
> +       .type = IIO_TEMP,
> +       .indexed = 1,
> +       .channel = 17,
> +       .channel2 = 18,
> +       .address = 0,
> +       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +               BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +       .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +       .scan_index = 0,
> +       .scan_type = {
> +               .sign = 'u',
> +               .realbits = 24,
> +               .storagebits = 32,
> +               .shift = 0,
> +               .endianness = IIO_BE,
> +       },
> +};
> +
> +static int ad717x_of_parse_channel_config(struct iio_dev *indio_dev)
> +{
> +       struct ad717x_state *st = iio_priv(indio_dev);
> +       struct ad717x_channel *channels_st_priv;
> +       struct fwnode_handle *child;
> +       struct device *dev = indio_dev->dev.parent;
> +       struct iio_chan_spec *chan;
> +       unsigned int num_channels = 0;
> +       unsigned int ain[2], chan_index = 0;
> +       bool use_temp = false;
> +       int ret;
> +
> +       num_channels = device_get_child_node_count(dev);
> +
> +       if (device_property_read_bool(dev, "adi,temp-channel")) {
> +               if (!st->info->has_temp) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "Current chip does not support temperature
> channel");
> +                       return -EINVAL;
> +               }
> +
> +               num_channels++;
> +               use_temp = true;
> +       }
> +
> +       st->num_channels = num_channels;
> +
> +       if (num_channels == 0)
> +               return 0;
> +
> +       chan = devm_kcalloc(indio_dev->dev.parent, sizeof(*chan), num_channels,
> +                           GFP_KERNEL);
> +       if (!chan)
> +               return -ENOMEM;
> +
> +       channels_st_priv = devm_kcalloc(indio_dev->dev.parent, num_channels,
> +                                       sizeof(*channels_st_priv), GFP_KERNEL);
> +       if (!channels_st_priv)
> +               return -ENOMEM;
> +
> +       indio_dev->channels = chan;
> +       indio_dev->num_channels = num_channels;
> +       st->channels = channels_st_priv;
> +
> +       if (use_temp) {
> +               chan[chan_index] = ad717x_temp_iio_channel_template;
> +               channels_st_priv[chan_index].ain =
> +                       AD717X_CH_ADDRESS(chan[chan_index].channel,
> chan[chan_index].channel2);
> +               channels_st_priv[chan_index].cfg.bipolar = false;
> +               channels_st_priv[chan_index].cfg.input_buf = true;
> +               chan_index++;
> +       }
> +
> +       device_for_each_child_node(dev, child) {
> +               ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
> 2);
> +               if (ret) {
> +                       fwnode_handle_put(child);
> +                       return ret;
> +               }
> +
> +               if (ain[0] >= st->info->num_inputs ||
> +                   ain[1] >= st->info->num_inputs) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "Input pin number out of range for pair (%d %d).",
> ain[0], ain[1]);

dev_err_probe() in all the error paths during probe. Moreover, just use 'dev' as you
are declaring it above.

> +                       fwnode_handle_put(child);
> +                       return -EINVAL;
> +               }
> +
> +               chan[chan_index] = ad717x_channel_template;
> +               chan[chan_index].address = chan_index;
> +               chan[chan_index].scan_index = chan_index;
> +               chan[chan_index].channel = ain[0];
> +               chan[chan_index].channel2 = ain[1];

I think 'channel2' only makes sense for differential channels. I assume the core will
discard it if 'differential' is not set but for readability I would set it only below
inside the if block. The 'diff-channels' is also not perfect as we might actually
have a single ended channel defined in it. But maybe that's actually expected...

> +
> +               channels_st_priv[chan_index].ain =
> +                       AD717X_CH_ADDRESS(ain[0], ain[1]);
> +               channels_st_priv[chan_index].chan_reg = chan_index;
> +               channels_st_priv[chan_index].cfg.input_buf = true;
> +               channels_st_priv[chan_index].cfg.odr = 0;
> +
> +               chan[chan_index].differential = fwnode_property_read_bool(child,
> "adi,bipolar");
> +               if (chan[chan_index].differential) {
> +                       chan[chan_index].info_mask_separate |=
> BIT(IIO_CHAN_INFO_OFFSET);
> +                       channels_st_priv[chan_index].cfg.bipolar = true;
> +               }
> +
> +               chan_index++;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ad717x_probe(struct spi_device *spi)
> +{
> +       struct ad717x_state *st;
> +       struct iio_dev *indio_dev;
> +       int ret;
> +
> +       if (!spi->irq) {
> +               dev_err(&spi->dev, "No IRQ specified\n");
> +               return -ENODEV;
> +       }

Drop the above...

> +
> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       st = iio_priv(indio_dev);
> +       st->info = device_get_match_data(&spi->dev);
> +       if (!st->info)
> +               return -ENODEV;
> +

spi_get_device_match_data() (not really sure if this is still applicable since some
work related to this is being done for i2c - and eventually in spi I guess).

> +       indio_dev->dev.parent = &spi->dev;

drop this...

> +       indio_dev->name = spi_get_device_id(spi)->name;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->info = &ad717x_info;
> +
> +       ad717x_sigma_delta_info.num_slots = st->info->num_configs;
> +       ret = ad_sd_init(&st->sd, indio_dev, spi, &ad717x_sigma_delta_info);
> +       if (ret < 0)
> +               return ret;
> +
> +       spi_set_drvdata(spi, indio_dev);
> +
> +       ret = ad717x_of_parse_channel_config(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = ad717x_setup(indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       return ad717x_gpio_init(st);
> +}
> +
> +static const struct of_device_id ad717x_of_match[] = {
> +       { .compatible = "adi,ad7172-2",
> +         .data = &ad717x_device_info[ID_AD7172_2] },
> +       { .compatible = "adi,ad7173-8",
> +         .data = &ad717x_device_info[ID_AD7173_8] },
> +       { .compatible = "adi,ad7175-2",
> +         .data = &ad717x_device_info[ID_AD7175_2] },
> +       { .compatible = "adi,ad7176-2",
> +         .data = &ad717x_device_info[ID_AD7176_2] },
> +       {}
> +}
> +MODULE_DEVICE_TABLE(of, ad717x_of_match);
> +
> +static const struct spi_device_id ad717x_id_table[] = {
> +       { "ad7172-2", },
> +       { "ad7173-8", },
> +       { "ad7175-2", },
> +       { "ad7176-2", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(spi, ad717x_id_table);
> +
> +static struct spi_driver ad717x_driver = {
> +       .driver = {
> +               .name   = "ad717x",

I would keep the name as we have out of tree which is ad7173.c. I'm not sure if
there's any policy in here but I think typically you just name your driver from the
first supported device and then extend it from there. Since here you are just adding
more than one device at once, it would be nice if you could keep the name of the
driver Lars originally developed...

> +               .owner  = THIS_MODULE,

No need for the above

> +               .of_match_table = of_match_ptr(ad717x_of_match),

Drop of_match_ptr()

- Nuno Sá

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno Sá wrote:
> On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:

...

> Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just get rid of
> the #ifdef wrapper and just select GPIOLIB. How often will it be disabled anyways?

The agreement is that users are depend on and not selecting GPIOLIB.
Any news in these agreement terms?

...

> > +       id &= AD717X_ID_MASK;
> > +       if (id != st->info->id)
> > +               dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected:
> > %x\n",
> > +                                           id, st->info->id);
> > +
> 
> Shouldn't we error out?

It seems a new way of thinking about unsupported CHIP ID. Dunno if hw vendors
won't ever do a dirty trick that new ID must be programmed differently and
otherwise burn hardware to a smoke...

I'm with you here, unknown chips mustn't be supported.

...

> > +                               *val = -(1 << (chan->scan_type.realbits - 1));
> 
> nit: I don't expect the driver to really be updated with more devices (it's
> like this for a long time) but the above is not very extensible... Imagine we
> add a device with 32bit channels? We would enter shady waters If I'm not
> missing anything.

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

...

> > +       st->info = device_get_match_data(&spi->dev);
> > +       if (!st->info)
> > +               return -ENODEV;
> > +
> 
> spi_get_device_match_data() (not really sure if this is still applicable
> since some work related to this is being done for i2c - and eventually in spi
> I guess).

Still applicable.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Jonathan Cameron]

On Thu, 10 Aug 2023 18:36:36 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno Sá wrote:
> > On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:  
> 
> ...
> 
> > Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just get rid of
> > the #ifdef wrapper and just select GPIOLIB. How often will it be disabled anyways?  
> 
> The agreement is that users are depend on and not selecting GPIOLIB.
> Any news in these agreement terms?
> 
> ...
> 
> > > +       id &= AD717X_ID_MASK;
> > > +       if (id != st->info->id)
> > > +               dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected:
> > > %x\n",
> > > +                                           id, st->info->id);
> > > +  
> > 
> > Shouldn't we error out?  
> 
> It seems a new way of thinking about unsupported CHIP ID. Dunno if hw vendors
> won't ever do a dirty trick that new ID must be programmed differently and
> otherwise burn hardware to a smoke...
> 
> I'm with you here, unknown chips mustn't be supported.

Some discussions with DT maintainers on this led me to change my mind...
They are very strongly of the view that if a DT firmware claims a device is compatible
then a device ID register that has an unknown value should be ignored. It gets
more tricky if we have a known wrong value (in which case we assume that it
is the value the hardware is claiming whatever DT says).

Argument is that lots of new versions of devices that are fully compatible with
exception of ID registers are released and if you have an old kernel but a new
device tree (typically running a distro that hasn't caught up yet) then the
fallback compatible is there to make things work.

I changed the way we handle this case in IIO to follow this policy - with
the slight tweak that we print a message whenever such a mismatch occurs.

Jonathan

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Nuno Sá]

On Thu, 2023-08-10 at 18:36 +0300, Andy Shevchenko wrote:
> On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno Sá wrote:
> > On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:
> 
> ...
> 
> > Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just
> > get rid of
> > the #ifdef wrapper and just select GPIOLIB. How often will it be disabled
> > anyways?
> 
> The agreement is that users are depend on and not selecting GPIOLIB.
> Any news in these agreement terms?
> 

Hmm no idea about that. If you say so, it's just one new thing I'm learning :)

- Nuno Sá

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Andy Shevchenko]

On Tue, Aug 29, 2023 at 11:14:31AM +0200, Nuno Sá wrote:
> On Thu, 2023-08-10 at 18:36 +0300, Andy Shevchenko wrote:
> > On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno Sá wrote:
> > > On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:

...

> > > Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just
> > > get rid of
> > > the #ifdef wrapper and just select GPIOLIB. How often will it be disabled
> > > anyways?
> > 
> > The agreement is that users are depend on and not selecting GPIOLIB.
> > Any news in these agreement terms?
> 
> Hmm no idea about that. If you say so, it's just one new thing I'm learning :)

That's the last I know.
Cc'ing to GPIOLIB maintainers...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Arnd Bergmann]

On Tue, Aug 29, 2023, at 09:31, Andy Shevchenko wrote:
> On Tue, Aug 29, 2023 at 11:14:31AM +0200, Nuno Sá wrote:
>> On Thu, 2023-08-10 at 18:36 +0300, Andy Shevchenko wrote:
>> > On Thu, Aug 10, 2023 at 01:57:02PM +0200, Nuno Sá wrote:
>> > > On Thu, 2023-08-10 at 12:33 +0300, Dumitru Ceclan wrote:
>
>> > > Is ad717x_gpio_cleanup() being used anywhere? Moreover I would maybe just
>> > > get rid of
>> > > the #ifdef wrapper and just select GPIOLIB. How often will it be disabled
>> > > anyways?
>> > 
>> > The agreement is that users are depend on and not selecting GPIOLIB.
>> > Any news in these agreement terms?
>> 
>> Hmm no idea about that. If you say so, it's just one new thing I'm learning :)
>Based outside of the U.S.? Some titles might be unavailable in your current location. Go to amazon.de to see the video catalog available in Germany.
> That's the last I know.
> Cc'ing to GPIOLIB maintainers...

From a Kconfig perspective, any user-visible symbol ideally only uses
'depends on', while hidden symbols usually use 'select'.

For the GPIOLIB symbol specifically, we have a mix of both, but the
overall usage is that gpio consumers only use 'depends on',
while some of the providers use 'select'. This risks causing build
breakage from a dependency loop when combined with other symbols
that have the same problem (e.g. I2C), but it tends to work out
as long as a strong hierarchy is kept. In particular, using 'select'
from an arch/*/Kconfig platform option is generally harmless as
long as those don't depend on anything else.

The new driver is a gpio provider and at least ad4130 and
ad5592r uses 'select' here, but then again ad74115 and
ad74113 use 'depends on' and ads7950 uses neither.

I think the best way to handle these is to remove both
the 'select' and the #ifdef in the driver and instead use
'if (IS_ENABLED(CONFIG_GPIOLIB))' to handle optional gpio
providers in the code.

       Arnd

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Jonathan Cameron]

On Thu, 10 Aug 2023 13:57:02 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> Hi Dumitru,
> 
> Thanks for taking care of this driver which is out of tree for a long time... Some
> comments below.
Hi.

A few follow ups...


> > +static int ad717x_setup(struct iio_dev *indio_dev)
> > +{
> > +       struct ad717x_state *st = iio_priv(indio_dev);
> > +       unsigned int id;
> > +       u8 *buf;
> > +       int ret;
> > +
> > +       /* reset the serial interface */
> > +       buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       memset(buf, 0xff, 8);
> > +       ret = spi_write(st->sd.spi, buf, 8);
> > +       kfree(buf);  
> 
> Hmm, why allocating the buffer? I guess one could argue that we'll get DMA safe
> alignment but then maybe use some define instead of the magic 8.
> 

Just use spi_write_then_read() without an read buffer as then it will use
magic bounce buffers for you within the spi subsystem.



> > +static struct spi_driver ad717x_driver = {
> > +       .driver = {
> > +               .name   = "ad717x",  
> 
> I would keep the name as we have out of tree which is ad7173.c. I'm not sure if
> there's any policy in here but I think typically you just name your driver from the
> first supported device and then extend it from there. Since here you are just adding
> more than one device at once, it would be nice if you could keep the name of the
> driver Lars originally developed...

In this case, indeed the one originally developed is a good choice.
Otherwise, pick a supported part.

Wild cards have bitten us far too many times as manufacturers love to
'use up' gaps in their ID space with parts that are either very different
or even worse look the same but have totally different interfaces...


> 

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[kernel test robot]

Hi Dumitru,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.5-rc5 next-20230809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dumitru-Ceclan/iio-adc-ad717x-add-AD717X-driver/20230810-173526
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20230810093322.593259-2-mitrutzceclan%40gmail.com
patch subject: [PATCH 2/2] iio: adc: ad717x: add AD717X driver
config: xtensa-randconfig-m041-20230811 (https://download.01.org/0day-ci/archive/20230811/202308110347.LseABMWN-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230811/202308110347.LseABMWN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308110347.LseABMWN-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/device/driver.h:21,
                    from include/linux/device.h:32,
                    from drivers/iio/adc/ad717x.c:9:
>> include/linux/module.h:244:1: error: expected ',' or ';' before 'extern'
     244 | extern typeof(name) __mod_##type##__##name##_device_table               \
         | ^~~~~~
   drivers/iio/adc/ad717x.c:974:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
     974 | MODULE_DEVICE_TABLE(of, ad717x_of_match);
         | ^~~~~~~~~~~~~~~~~~~


vim +244 include/linux/module.h

^1da177e4c3f41 Linus Torvalds    2005-04-16  240  
cff26a51da5d20 Rusty Russell     2014-02-03  241  #ifdef MODULE
cff26a51da5d20 Rusty Russell     2014-02-03  242  /* Creates an alias so file2alias.c can find device table. */
^1da177e4c3f41 Linus Torvalds    2005-04-16  243  #define MODULE_DEVICE_TABLE(type, name)					\
0bf8bf50eddc75 Matthias Kaehlcke 2017-07-24 @244  extern typeof(name) __mod_##type##__##name##_device_table		\
cff26a51da5d20 Rusty Russell     2014-02-03  245    __attribute__ ((unused, alias(__stringify(name))))
cff26a51da5d20 Rusty Russell     2014-02-03  246  #else  /* !MODULE */
cff26a51da5d20 Rusty Russell     2014-02-03  247  #define MODULE_DEVICE_TABLE(type, name)
cff26a51da5d20 Rusty Russell     2014-02-03  248  #endif
^1da177e4c3f41 Linus Torvalds    2005-04-16  249  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Andy Shevchenko]

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

...

> +	help
> +	  Say yes here to build support for Analog Devices AD717x ADC.

I believe checkpatch.pl expects at least 3 lines of help description.

One of them may tell the user what will be the module name, if chosen
to be built as a module.

...

> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

There is no user of the header. But missing bits.h, mod_devicetable.h,
property.h, and might be more.

So, revisit this block to see which ones are used and which one are missed,
and which are redundant.

> +#include <linux/sched.h>

Is this used? How?

> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/regulator/consumer.h>

Can you keep the above sorted?

...

> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>

Can you keep the above sorted?

+ Blank line

> +#include <linux/iio/adc/ad_sigma_delta.h>

...

> +#define AD717X_ID_MASK			0xfff0

GENMASK()

> +#define AD717X_ADC_MODE_MODE_MASK	0x70

> +#define AD717X_ADC_MODE_CLOCKSEL_MASK	0xc

You are using BIT(), and not GENMASK()...
All can be converted.

...

> +#define AD717X_SETUP_AREF_BUF		(0x3 << 10)
> +#define AD717X_SETUP_AIN_BUF		(0x3 << 8)
> +#define AD717X_SETUP_REF_SEL_MASK	0x30

Ditto for all.

...

> +#define AD717X_SETUP_REF_SEL_AVDD1_AVSS	0x30
> +#define AD717X_SETUP_REF_SEL_INT_REF	0x20
> +#define AD717X_SETUP_REF_SEL_EXT_REF2	0x10
> +#define AD717X_SETUP_REF_SEL_EXT_REF	0x00

Seems to me like plain decimals with shift should be used

#define AD717X_SETUP_REF_SEL_AVDD1_AVSS	(3...
#define AD717X_SETUP_REF_SEL_INT_REF	(2...
#define AD717X_SETUP_REF_SEL_EXT_REF2	(1...
#define AD717X_SETUP_REF_SEL_EXT_REF	(0 << 4)

...

> +#define AD717X_FILTER_ODR0_MASK		0x1f

GENMASK()

...

> +static const struct ad717x_device_info ad717x_device_info[] = {
> +	[ID_AD7172_2] = {

> +		.clock = 2000000,

> +	},
> +	[ID_AD7173_8] = {

> +		.clock = 2000000,

> +	},
> +	[ID_AD7175_2] = {

> +		.clock = 16000000,

> +	},
> +	[ID_AD7176_2] = {

> +		.clock = 16000000,

> +	},
> +};

HZ_PER_MHZ from units.h?

...

> +static int ad717x_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +	unsigned int mask;
> +	unsigned int value;
> +	int ret;
> +
> +	switch (offset) {
> +	case 0:
> +		mask = AD717X_GPIO_GP_DATA0;
> +		break;
> +	case 1:
> +		mask = AD717X_GPIO_GP_DATA1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = ad_sd_read_reg(&st->sd, AD717X_REG_GPIO, 2, &value);
> +	if (ret)
> +		return ret;
> +
> +	return (bool)(value & mask);

This is weird. You have int which you get from bool, wouldn't be better to use
!!(...) as other GPIO drivers do?

> +}

...

> +static void ad717x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +	unsigned int mask, set_mask, clr_mask;
> +
> +	switch (offset) {
> +	case 0:
> +		mask = AD717X_GPIO_GP_DATA0;
> +		break;
> +	case 1:
> +		mask = AD717X_GPIO_GP_DATA1;
> +		break;
> +	case 2:
> +		mask = AD717X_GPIO_GP_DATA2;
> +		break;
> +	case 3:
> +		mask = AD717X_GPIO_GP_DATA3;
> +		break;
> +	default:
> +		return;
> +	}

> +	if (value) {
> +		set_mask = mask;
> +		clr_mask = 0;
> +	} else {
> +		set_mask = 0;
> +		clr_mask = mask;
> +	}
> +
> +	ad717x_gpio_update(st, set_mask, clr_mask);

It's too verbose, just

	if (value)
		ad717x_gpio_update(st, mask, 0);
	else
		ad717x_gpio_update(st, 0, mask);

Would save a half a dozen LoCs.

> +}

...

> +static int ad717x_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct ad717x_state *st = gpiochip_to_ad717x(chip);
> +	unsigned int mask;
> +
> +	switch (offset) {
> +	case 0:
> +		mask = AD717X_GPIO_IP_EN0;
> +		break;
> +	case 1:
> +		mask = AD717X_GPIO_IP_EN1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

> +	return ad717x_gpio_update(st, mask, 0);

Return directly from the switch case.

> +}

...

The GPIO methods are too verbose, I stopped looking into them here.
The main question, why gpio-regmap is not used for this?

...

> +static int ad717x_gpio_init(struct ad717x_state *st)
> +{

	struct device *dev = &st->sd.spi->dev;

makes code neater here and elsewhere.

> +	st->gpiochip.label = dev_name(&st->sd.spi->dev);
> +	st->gpiochip.base = -1;

> +	if (st->info->has_gp23)
> +		st->gpiochip.ngpio = 4;
> +	else
> +		st->gpiochip.ngpio = 2;

Instead of keeping a boolean flag in the info, why you not keep ngpio there
(0 implies no GPIO)?

> +	st->gpiochip.parent = &st->sd.spi->dev;
> +	st->gpiochip.can_sleep = true;
> +	st->gpiochip.direction_input = ad717x_gpio_direction_input;
> +	st->gpiochip.direction_output = ad717x_gpio_direction_output;
> +	st->gpiochip.get = ad717x_gpio_get;
> +	st->gpiochip.set = ad717x_gpio_set;
> +	st->gpiochip.free = ad717x_gpio_free;
> +	st->gpiochip.owner = THIS_MODULE;
> +
> +	return devm_gpiochip_add_data(&st->sd.spi->dev, &st->gpiochip, NULL);
> +}

...

> +static void ad717x_reset_usage_cnts(struct ad717x_state *st)
> +{
> +	int i;
> +
> +	for (i = 0; i < st->info->num_configs; i++)

> +		(st->config_cnts)[i] = 0;

What are the parentheses for?
Wouldn't this a NIH one of memsetXX()?

> +	st->config_usage_counter = 0;
> +}

...

> +	offset = offsetof(struct ad717x_channel_config, cfg_slot) +
> +		 sizeof(cfg->cfg_slot);
> +	cmp_size = sizeof(*cfg) - offset;

My gut's feeling this is some NIH one of macros from overflow.h.

...

> +	for (i = 0; i < st->num_channels; i++) {
> +		cfg_aux = &st->channels[i].cfg;
> +
> +		if (cfg_aux->live && !memcmp(((void *)cfg) + offset,
> +					    ((void *)cfg_aux) + offset, cmp_size))

My gosh! Explicit castings should be really rear. Can't you use proper
struct / array pointers?

> +			return cfg_aux;
> +	}

...

> +	int ret = 0;

How is this 0 used?

> +	if (!cfg->live) {

> +		live_cfg = ad717x_find_live_config(st, cfg);
> +		if (!live_cfg) {

Why not positive check?

> +			ret = ad717x_load_config(st, cfg);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			cfg->cfg_slot = live_cfg->cfg_slot;
> +		}
> +	}

> +	cfg->live = true;

Can be moved inside the conditional.

...

> +	ret = ad717x_config_channel(st, channel);
> +	if (ret < 0)

What is the meaning of > 0?
Same Q to other similar checks.

> +		return ret;

...

> +static int ad717x_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	unsigned int id;
> +	u8 *buf;
> +	int ret;
> +
> +	/* reset the serial interface */
> +	buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);

Magic number!

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	memset(buf, 0xff, 8);
> +	ret = spi_write(st->sd.spi, buf, 8);
> +	kfree(buf);
> +	if (ret < 0)
> +		return ret;

I agree with comments by Nuno on this.

> +	/* datasheet recommends a delay of at least 500us after reset */
> +	usleep_range(500, 1000);

	fsleep(500);

> +	ret = ad_sd_read_reg(&st->sd, AD717X_REG_ID, 2, &id);
> +	if (ret)
> +		return ret;
> +
> +	id &= AD717X_ID_MASK;
> +	if (id != st->info->id)
> +		dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected: %x\n",
> +					    id, st->info->id);

Wrong indentation.

> +	mutex_init(&st->cfgs_lock);
> +	st->adc_mode |= AD717X_ADC_MODE_REF_EN | AD717X_ADC_MODE_SING_CYC;
> +	st->interface_mode = 0x0;
> +
> +	st->config_usage_counter = 0;
> +	st->config_cnts = devm_kzalloc(&indio_dev->dev,
> +				       st->info->num_configs * sizeof(u64),

No, let kernel do the right thing with this.

> +				       GFP_KERNEL);
> +	if (!st->config_cnts)
> +		return -ENOMEM;
> +
> +	/* All channels are enabled by default after a reset */
> +	ret = ad717x_disable_all(&st->sd);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

...

> +static int ad717x_read_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int *val, int *val2, long info)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	unsigned int reg;
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* disable channel after single conversion */

> +		ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(chan->address), 2,
> +				      0);

This is much better on a single line.

> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 250000000;
> +			*val2 = 800273203; /* (2**24 * 477) / 10 */

Use mathematical notations (TeX like)

	(2^24 * 477) / 10

> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			*val = 2500;
> +			if (chan->differential)
> +				*val2 = 23;
> +			else
> +				*val2 = 24;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (chan->type == IIO_TEMP) {
> +			*val = -874379;
> +		} else {
> +			if (chan->differential)
> +				*val = -(1 << (chan->scan_type.realbits - 1));
> +			else
> +				*val = 0;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		reg = st->channels[chan->address].cfg.odr;
> +
> +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;
> +}

...

> +static int ad717x_write_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long info)
> +{

> +	int ret = 0;

You won't need this if...

> +	mutex_lock(&st->cfgs_lock);

...you switch to use guarded mutex (see cleanup.h).

Same applicable to many other functions.

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

...

> +static int ad717x_debug(struct iio_dev *indio_dev, unsigned int reg,
> +			unsigned int writeval, unsigned int *readval)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);

> +	u8 reg_size = 2;

Better to make it an else branch...

> +	if (reg == 0)
> +		reg_size = 1;
> +	else if (reg == AD717X_REG_CRC || reg == AD717X_REG_DATA ||
> +		 reg >= AD717X_REG_OFFSET(0))
> +		reg_size = 3;

	else
		reg_size = 2;

> +	if (readval)
> +		return ad_sd_read_reg(&st->sd, reg, reg_size, readval);
> +
> +	return ad_sd_write_reg(&st->sd, reg, reg_size, writeval);
> +}

...

> +static int ad717x_of_parse_channel_config(struct iio_dev *indio_dev)

of --> fw

> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	struct ad717x_channel *channels_st_priv;
> +	struct fwnode_handle *child;
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *chan;

> +	unsigned int num_channels = 0;

How is this 0 being used?

> +	unsigned int ain[2], chan_index = 0;

> +	bool use_temp = false;

No assignment needed here, see below.

> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);

> +	if (device_property_read_bool(dev, "adi,temp-channel")) {

	use_temp = device_property_...
	if (use_temp) {

> +		if (!st->info->has_temp) {

> +			dev_err(indio_dev->dev.parent,
> +				"Current chip does not support temperature channel");
> +			return -EINVAL;

			return dev_err_probe(...);

> +		}
> +
> +		num_channels++;
> +		use_temp = true;
> +	}

> +	st->num_channels = num_channels;

I believe that it's already 0, so this can be moved...

> +	if (num_channels == 0)
> +		return 0;

...after this check.

> +	chan = devm_kcalloc(indio_dev->dev.parent, sizeof(*chan), num_channels,

Why not use dev?

> +			    GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	channels_st_priv = devm_kcalloc(indio_dev->dev.parent, num_channels,

Ditto.

> +					sizeof(*channels_st_priv), GFP_KERNEL);
> +	if (!channels_st_priv)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = chan;
> +	indio_dev->num_channels = num_channels;
> +	st->channels = channels_st_priv;
> +
> +	if (use_temp) {
> +		chan[chan_index] = ad717x_temp_iio_channel_template;
> +		channels_st_priv[chan_index].ain =
> +			AD717X_CH_ADDRESS(chan[chan_index].channel, chan[chan_index].channel2);
> +		channels_st_priv[chan_index].cfg.bipolar = false;
> +		channels_st_priv[chan_index].cfg.input_buf = true;
> +		chan_index++;
> +	}
> +
> +	device_for_each_child_node(dev, child) {
> +		ret = fwnode_property_read_u32_array(child, "diff-channels", ain, 2);

ARRAY_SIZE() ?

> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (ain[0] >= st->info->num_inputs ||
> +		    ain[1] >= st->info->num_inputs) {
> +			dev_err(indio_dev->dev.parent,
> +				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
> +			fwnode_handle_put(child);
> +			return -EINVAL;

			return dev_err_probe(...);

> +		}
> +
> +		chan[chan_index] = ad717x_channel_template;
> +		chan[chan_index].address = chan_index;
> +		chan[chan_index].scan_index = chan_index;
> +		chan[chan_index].channel = ain[0];
> +		chan[chan_index].channel2 = ain[1];

> +		channels_st_priv[chan_index].ain =
> +			AD717X_CH_ADDRESS(ain[0], ain[1]);

Why not one line here?

> +		channels_st_priv[chan_index].chan_reg = chan_index;
> +		channels_st_priv[chan_index].cfg.input_buf = true;
> +		channels_st_priv[chan_index].cfg.odr = 0;
> +
> +		chan[chan_index].differential = fwnode_property_read_bool(child, "adi,bipolar");
> +		if (chan[chan_index].differential) {
> +			chan[chan_index].info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
> +			channels_st_priv[chan_index].cfg.bipolar = true;
> +		}
> +
> +		chan_index++;
> +	}
> +
> +	return 0;
> +}

...

> +	if (!spi->irq) {
> +		dev_err(&spi->dev, "No IRQ specified\n");
> +		return -ENODEV;

		return dev_err_probe(...);

> +	}

...

> +static const struct spi_device_id ad717x_id_table[] = {
> +	{ "ad7172-2", },
> +	{ "ad7173-8", },
> +	{ "ad7175-2", },
> +	{ "ad7176-2", },
> +	{}

Missing driver_data here.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: ad717x: add AD717X driver

[Jonathan Cameron]

On Thu, 10 Aug 2023 12:33:17 +0300
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

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

Hi Dumitru

Looks like you've have a lot of good review already, so I've only taken
a very quick look at this version.  A few comments inline.

Jonathan


> ---
>  drivers/iio/adc/Kconfig  |   7 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad717x.c | 999 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1007 insertions(+)
>  create mode 100644 drivers/iio/adc/ad717x.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dc14bde31ac1..294a48b05769 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -54,6 +54,13 @@ config AD7124
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad7124.
>  
> +config AD717X

No wild card please. Pick a supported device to name it after.
This applies to all naming of variable, function names etc as well as the Kconfig
naming.


> +	tristate "Analog Devices AD717x driver"
> +	depends on SPI_MASTER
> +	select AD_SIGMA_DELTA
> +	help
> +	  Say yes here to build support for Analog Devices AD717x ADC.
> +
>  config AD7192
>  	tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
>  	depends on SPI

> diff --git a/drivers/iio/adc/ad717x.c b/drivers/iio/adc/ad717x.c
> new file mode 100644
> index 000000000000..d14a3e0e2d93
> --- /dev/null
> +++ b/drivers/iio/adc/ad717x.c
> @@ -0,0 +1,999 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
> + * Copyright (C) 2023 Analog Devices, Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/units.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>

Check if all these headers are needed. I suspect not.

> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/adc/ad_sigma_delta.h>
> +
> +#define AD717X_REG_COMMS		0x00
> +#define AD717X_REG_ADC_MODE		0x01
> +#define AD717X_REG_INTERFACE_MODE	0x02
> +#define AD717X_REG_CRC			0x03
> +#define AD717X_REG_DATA			0x04
> +#define AD717X_REG_GPIO			0x06
> +#define AD717X_REG_ID			0x07
> +#define AD717X_REG_CH(x)		(0x10 + (x))
> +#define AD717X_REG_SETUP(x)		(0x20 + (x))
> +#define AD717X_REG_FILTER(x)		(0x28 + (x))
> +#define AD717X_REG_OFFSET(x)		(0x30 + (x))
> +#define AD717X_REG_GAIN(x)		(0x38 + (x))
> +
> +#define AD717X_CH_ENABLE		BIT(15)
> +#define AD717X_CH_SETUP_SEL(x)		((x) << 12)
> +#define AD717X_CH_SETUP_AINPOS(x)	((x) << 5)
> +#define AD717X_CH_SETUP_AINNEG(x)	(x)
> +
> +#define AD717X_CH_ADDRESS(pos, neg) \
> +	(AD717X_CH_SETUP_AINPOS(pos) | AD717X_CH_SETUP_AINNEG(neg))
> +
> +#define AD7172_ID			0x00d0
> +#define AD7173_ID			0x30d0
> +#define AD7175_ID			0x0cd0
> +#define AD7176_ID			0x0c90
> +#define AD717X_ID_MASK			0xfff0
> +
> +#define AD717X_ADC_MODE_REF_EN		BIT(15)
> +#define AD717X_ADC_MODE_SING_CYC	BIT(13)
> +#define AD717X_ADC_MODE_MODE_MASK	0x70
> +#define AD717X_ADC_MODE_MODE(x)		((x) << 4)

Use FIELD_PREP along with the MODE_MASK wherever this is currently
called. Do the same for other similar cases.  Normally a field
should be defined just using a mask as that includes
any necessary shift.  FIELD_PREP() and FIELD_GET() do the
magic to extract that shift.

> +#define AD717X_ADC_MODE_CLOCKSEL_MASK	0xc
> +#define AD717X_ADC_MODE_CLOCKSEL(x)	((x) << 2)

> +
> +struct ad717x_state {
> +	const struct ad717x_device_info *info;
> +	struct ad_sigma_delta sd;
> +	struct ad717x_channel *channels;
> +	struct regulator *reg;
> +	unsigned int adc_mode;
> +	unsigned int interface_mode;
> +	unsigned int num_channels;
> +	struct mutex cfgs_lock; /* lock for configs access */

What config?

> +	unsigned long cfg_slots_status; /* slots usage status bitmap*/
> +	unsigned long long config_usage_counter;
> +	unsigned long long *config_cnts;
> +
> +#ifdef CONFIG_GPIOLIB
> +	struct gpio_chip gpiochip;
> +	unsigned int gpio_reg;
> +	unsigned int gpio_23_mask;
> +#endif
> +};

> +
> +static int ad717x_gpio_init(struct ad717x_state *st)

Even though it's simple, include the linux-gpio list an maintainers
given the presence of a gpio chip in here.

> +{
> +	st->gpiochip.label = dev_name(&st->sd.spi->dev);
> +	st->gpiochip.base = -1;
> +	if (st->info->has_gp23)
> +		st->gpiochip.ngpio = 4;
> +	else
> +		st->gpiochip.ngpio = 2;
> +	st->gpiochip.parent = &st->sd.spi->dev;
> +	st->gpiochip.can_sleep = true;
> +	st->gpiochip.direction_input = ad717x_gpio_direction_input;
> +	st->gpiochip.direction_output = ad717x_gpio_direction_output;
> +	st->gpiochip.get = ad717x_gpio_get;
> +	st->gpiochip.set = ad717x_gpio_set;
> +	st->gpiochip.free = ad717x_gpio_free;
> +	st->gpiochip.owner = THIS_MODULE;
> +
> +	return devm_gpiochip_add_data(&st->sd.spi->dev, &st->gpiochip, NULL);
> +}
> +
> +#else
> +
> +static int ad717x_gpio_init(struct ad717x_state *st) { return 0 };
> +static void ad717x_gpio_cleanup(struct ad717x_state *st) {};
> +
> +#endif

...

> +
> +static int ad717x_load_config(struct ad717x_state *st,
> +			      struct ad717x_channel_config *cfg)
> +{
> +	unsigned int config;
> +	int free_cfg_slot, ret;
> +
> +	free_cfg_slot = find_first_zero_bit(&st->cfg_slots_status,
> +					    st->info->num_configs);
> +	if (free_cfg_slot == st->info->num_configs)
> +		free_cfg_slot = ad717x_free_config_slot_lru(st);
> +
> +	assign_bit(free_cfg_slot, &st->cfg_slots_status, 1);
> +	cfg->cfg_slot = free_cfg_slot;
> +
> +	config = AD717X_SETUP_REF_SEL_INT_REF;
> +
> +	if (cfg->bipolar)
> +		config |= AD717X_SETUP_BIPOLAR;
> +
> +	if (cfg->input_buf)
> +		config |= AD717X_SETUP_AIN_BUF;
> +
> +	ret = ad_sd_write_reg(&st->sd, AD717X_REG_SETUP(free_cfg_slot), 2, config);
> +	if (ret)
> +		return ret;
> +
> +	config = AD717X_FILTER_ODR0_MASK & cfg->odr;
> +	return ad_sd_write_reg(&st->sd, AD717X_REG_FILTER(free_cfg_slot), 2, config);

Putting value inline is probably more readable here.

> +}


> +static struct ad_sigma_delta_info ad717x_sigma_delta_info = {
> +	.set_channel = ad717x_set_channel,
> +	.append_status = ad717x_append_status,
> +	.disable_all = ad717x_disable_all,
> +	.set_mode = ad717x_set_mode,
> +	.has_registers = true,
> +	.addr_shift = 0,
> +	.read_mask = BIT(6),
> +	.status_ch_mask = GENMASK(3, 0),
> +	.data_reg = AD717X_REG_DATA,
> +	.irq_flags = IRQF_TRIGGER_FALLING
> +};
> +
> +static int ad717x_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	unsigned int id;
> +	u8 *buf;
> +	int ret;
> +
> +	/* reset the serial interface */
> +	buf = kcalloc(8, sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	memset(buf, 0xff, 8);
> +	ret = spi_write(st->sd.spi, buf, 8);

spi_write_then_read() as mentioned in other thread.

> +	kfree(buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* datasheet recommends a delay of at least 500us after reset */
> +	usleep_range(500, 1000);
> +
> +	ret = ad_sd_read_reg(&st->sd, AD717X_REG_ID, 2, &id);
> +	if (ret)
> +		return ret;
> +
> +	id &= AD717X_ID_MASK;
> +	if (id != st->info->id)
> +		dev_warn(&st->sd.spi->dev, "Unexpected device id: %x, expected: %x\n",
> +					    id, st->info->id);
> +
> +	mutex_init(&st->cfgs_lock);
> +	st->adc_mode |= AD717X_ADC_MODE_REF_EN | AD717X_ADC_MODE_SING_CYC;
> +	st->interface_mode = 0x0;
> +
> +	st->config_usage_counter = 0;
> +	st->config_cnts = devm_kzalloc(&indio_dev->dev,
> +				       st->info->num_configs * sizeof(u64),
> +				       GFP_KERNEL);
> +	if (!st->config_cnts)
> +		return -ENOMEM;
> +
> +	/* All channels are enabled by default after a reset */
> +	ret = ad717x_disable_all(&st->sd);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ad717x_read_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int *val, int *val2, long info)

Prefer variables to align with the one on the first line.

> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	unsigned int reg;
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* disable channel after single conversion */
> +		ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(chan->address), 2,
> +				      0);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 250000000;
> +			*val2 = 800273203; /* (2**24 * 477) / 10 */
> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			*val = 2500;
> +			if (chan->differential)
> +				*val2 = 23;
> +			else
> +				*val2 = 24;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (chan->type == IIO_TEMP) {
> +			*val = -874379;
> +		} else {
> +			if (chan->differential)
> +				*val = -(1 << (chan->scan_type.realbits - 1));
> +			else
> +				*val = 0;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		reg = st->channels[chan->address].cfg.odr;
> +
> +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * MILLI;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int ad717x_write_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long info)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	struct ad717x_channel_config *cfg;
> +	unsigned int freq, i, reg;
> +	int ret = 0;
> +
> +	mutex_lock(&st->cfgs_lock);

What is this lock protecting?  I'd kind of expect it to be held in more places
so perhaps I'm misunderstanding the intended scope.

> +	if (iio_buffer_enabled(indio_dev)) {
> +		mutex_unlock(&st->cfgs_lock);
> +		return -EBUSY;

Someone else pointed this out, but this is probably racey as it isn't using the IIO core
locks that protect against a state change, so use iio_device_claim_direct() instead.
Do keep the local locking though as that is protecting different data.

> +	}
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		freq = val * MILLI + val2 / MILLI;
> +
> +		for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++) {
> +			if (freq >= st->info->sinc5_data_rates[i])
> +				break;
> +		}
> +
> +		cfg = &st->channels[chan->address].cfg;
> +		cfg->odr = i;
> +
> +		if (cfg->live) {
> +			ret = ad_sd_read_reg(&st->sd, AD717X_REG_FILTER(cfg->cfg_slot), 2, &reg);
> +			if (ret)
> +				break;
> +			reg &= ~AD717X_FILTER_ODR0_MASK;
> +			reg |= i;
> +			ret = ad_sd_write_reg(&st->sd, AD717X_REG_FILTER(cfg->cfg_slot), 2, reg);
> +		}
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&st->cfgs_lock);
> +	return ret;
> +}
> +
> +static int ad717x_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan, long mask)
> +{
> +	return IIO_VAL_INT_PLUS_MICRO;

Fairly sure that's the default, so you don't need this function to be provided.

> +}
> +
> +static int ad717x_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	bool bit_set;
> +	int ret;
> +	int i;
> +
> +	mutex_lock(&st->cfgs_lock);
> +	for (i = 0; i < st->num_channels; i++) {
> +		bit_set = test_bit(i, scan_mask);
> +		if (bit_set)
> +			ret = ad717x_set_channel(&st->sd, i);
> +		else
> +			ret = ad_sd_write_reg(&st->sd, AD717X_REG_CH(i), 2, 0);
> +
> +		if (ret < 0) {
> +			mutex_unlock(&st->cfgs_lock);
> +			return ret;

Use a goto or break to share the single unlock location.
Or you could use the new scope based locking stuff if you are feeling adventurous.


> +		}
> +	}
> +
> +	mutex_unlock(&st->cfgs_lock);
> +
> +	return 0;
> +}
> +

...

> +
> +static const struct iio_chan_spec ad717x_channel_template = {
> +	.type = IIO_VOLTAGE,
> +	.indexed = 1,
> +	.channel = 0,
> +	.address = AD717X_CH_ADDRESS(0, 0),
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		BIT(IIO_CHAN_INFO_SCALE),
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	.scan_index = 0,
> +	.scan_type = {
> +		.sign = 'u',
> +		.realbits = 24,
> +		.storagebits = 32,
> +		.shift = 0,

As below.

> +		.endianness = IIO_BE,
> +	},
> +};
> +
> +static const struct iio_chan_spec ad717x_temp_iio_channel_template = {
> +	.type = IIO_TEMP,
> +	.indexed = 1,
> +	.channel = 17,

Why set random values in here?  Fine to leave the stuff that will always be written defaulting to 0.

> +	.channel2 = 18,
> +	.address = 0,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	.scan_index = 0,
> +	.scan_type = {
> +		.sign = 'u',
> +		.realbits = 24,
> +		.storagebits = 32,
> +		.shift = 0,
Don't bother defining a shift of 0, that's considered the obvious default (and is what c will
set this to anyway).
> +		.endianness = IIO_BE,
> +	},
> +};
> +
> +static int ad717x_of_parse_channel_config(struct iio_dev *indio_dev)
> +{
> +	struct ad717x_state *st = iio_priv(indio_dev);
> +	struct ad717x_channel *channels_st_priv;
> +	struct fwnode_handle *child;
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *chan;
> +	unsigned int num_channels = 0;

Always set so don't init here.

> +	unsigned int ain[2], chan_index = 0;
> +	bool use_temp = false;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +
> +	if (device_property_read_bool(dev, "adi,temp-channel")) {
> +		if (!st->info->has_temp) {
> +			dev_err(indio_dev->dev.parent,
> +				"Current chip does not support temperature channel");
> +			return -EINVAL;
> +		}
> +
> +		num_channels++;
> +		use_temp = true;
I would make this..
	use_temp = device_property_...
	if (use_temp) {
		....

	}

> +	}
> +
> +	st->num_channels = num_channels;
> +
> +	if (num_channels == 0)
> +		return 0;
Do this 2 lines up, before setting st->num_channels.

Though I'm not clear why st needs a copy of that anyway as it's in the iio_device structure.

> +
> +	chan = devm_kcalloc(indio_dev->dev.parent, sizeof(*chan), num_channels,
> +			    GFP_KERNEL);
> +	if (!chan)
> +		return -ENOMEM;
> +
> +	channels_st_priv = devm_kcalloc(indio_dev->dev.parent, num_channels,
> +					sizeof(*channels_st_priv), GFP_KERNEL);

Use local dev

> +	if (!channels_st_priv)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = chan;
> +	indio_dev->num_channels = num_channels;
> +	st->channels = channels_st_priv;
> +
> +	if (use_temp) {
> +		chan[chan_index] = ad717x_temp_iio_channel_template;
> +		channels_st_priv[chan_index].ain =
> +			AD717X_CH_ADDRESS(chan[chan_index].channel, chan[chan_index].channel2);
> +		channels_st_priv[chan_index].cfg.bipolar = false;
> +		channels_st_priv[chan_index].cfg.input_buf = true;
> +		chan_index++;
> +	}
> +
> +	device_for_each_child_node(dev, child) {
> +		ret = fwnode_property_read_u32_array(child, "diff-channels", ain, 2);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (ain[0] >= st->info->num_inputs ||
> +		    ain[1] >= st->info->num_inputs) {
> +			dev_err(indio_dev->dev.parent,
> +				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
> +
> +		chan[chan_index] = ad717x_channel_template;
> +		chan[chan_index].address = chan_index;
> +		chan[chan_index].scan_index = chan_index;
> +		chan[chan_index].channel = ain[0];
> +		chan[chan_index].channel2 = ain[1];
> +
> +		channels_st_priv[chan_index].ain =
> +			AD717X_CH_ADDRESS(ain[0], ain[1]);
> +		channels_st_priv[chan_index].chan_reg = chan_index;
> +		channels_st_priv[chan_index].cfg.input_buf = true;
> +		channels_st_priv[chan_index].cfg.odr = 0;
> +
> +		chan[chan_index].differential = fwnode_property_read_bool(child, "adi,bipolar");
> +		if (chan[chan_index].differential) {
> +			chan[chan_index].info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
> +			channels_st_priv[chan_index].cfg.bipolar = true;
> +		}
> +
> +		chan_index++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad717x_probe(struct spi_device *spi)
> +{
> +	struct ad717x_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	if (!spi->irq) {
> +		dev_err(&spi->dev, "No IRQ specified\n");

It's rare a device can't be used at all without an IRQ.
Is that the case here, or is it just that the driver currently assumes
one is present?

> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->info = device_get_match_data(&spi->dev);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad717x_info;
> +
> +	ad717x_sigma_delta_info.num_slots = st->info->num_configs;
> +	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad717x_sigma_delta_info);
> +	if (ret < 0)
> +		return ret;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	ret = ad717x_of_parse_channel_config(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ad717x_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return ad717x_gpio_init(st);
> +}
> +
> +static const struct of_device_id ad717x_of_match[] = {
> +	{ .compatible = "adi,ad7172-2",
> +	  .data = &ad717x_device_info[ID_AD7172_2] },
> +	{ .compatible = "adi,ad7173-8",
> +	  .data = &ad717x_device_info[ID_AD7173_8] },
> +	{ .compatible = "adi,ad7175-2",
> +	  .data = &ad717x_device_info[ID_AD7175_2] },
> +	{ .compatible = "adi,ad7176-2",
> +	  .data = &ad717x_device_info[ID_AD7176_2] },
> +	{}
> +}
> +MODULE_DEVICE_TABLE(of, ad717x_of_match);
> +
> +static const struct spi_device_id ad717x_id_table[] = {
> +	{ "ad7172-2", },
> +	{ "ad7173-8", },
> +	{ "ad7175-2", },
> +	{ "ad7176-2", },

Add the info[] pointers here as well. If you get another
firmware type it may not match against the of_device_id table
entries.

As noted by others we have an spi function to grab the data from
which ever place it can be found.
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad717x_id_table);

Re: [PATCH 1/2] dt-bindings: adc: add AD717X

[Rob Herring]

On Thu, 10 Aug 2023 12:33:16 +0300, Dumitru Ceclan wrote:
> The AD717x family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad717x.yaml          | 158 ++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad717x.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:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230810093322.593259-1-mitrutzceclan@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

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 after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] dt-bindings: adc: add AD717X

[Rob Herring]

On Thu, Aug 10, 2023 at 12:33:16PM +0300, Dumitru Ceclan wrote:
> The AD717x family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad717x.yaml          | 158 ++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml
> new file mode 100644
> index 000000000000..f12926e69958
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad717x.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad717x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD717X ADC family SPI driver

Drop 'SPI driver'. This is not a driver.

> +
> +maintainers:
> +  - Ceclan Dumitru <dumitru.ceclan@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7172-2
> +      - adi,ad7173-8
> +      - adi,ad7175-2
> +      - adi,ad7176-2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  spi-max-frequency:
> +    maximum: 20000000
> +
> +  spi-cpol:
> +    type: boolean
> +
> +  spi-cpha:
> +    type: boolean
> +
> +  adi,temp-channel:
> +    description:
> +      Enables temperature reading channel
> +    type: boolean
> +
> +  dependencies:
> +    adi,temp-channel:
> +      compatible:
> +        enum:
> +          - adi,ad7172-2
> +          - adi,ad7173-8
> +          - adi,ad7175-2

That's not actually valid schema. You are missing "properties" above 
"compatible". I'm also not sure the tools which do a number of 
fixups/transforms on schemas would handle this. I do think this is a bit 
nicer than the if/then schemas we normally use for restricting 
properties by compatibles.

> +
> +

Extra blank line.

> +  required:
> +    - compatible
> +    - reg
> +    - interrupts
> +    - spi-cpol
> +    - spi-cpha

If the device(s) are not configurable, then you shouldn't need these 2 
properties. The driver can hardcode the correct setting.

> +
> +patternProperties:
> +  "^channel@([0-9a-f])$":

Don't need ().

> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description: Channel number
> +        minimum: 0
> +        maximum: 15
> +
> +      diff-channels:
> +        description:
> +          Analog input pins
> +        items:
> +          minimum: 0
> +          maximum: 31
> +      
> +      adi,bipolar:
> +        description: Specify if the channel should measure in bipolar mode.
> +        type: boolean
> +
> +    required:
> +      - reg
> +      - diff-channels
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    spi {
> +      status = "okay";

Drop status.

You need #address-cells and #size-cells. Did you test this?

> +
> +      ad7173@0 {
> +        compatible = "adi,ad7173-8";
> +        reg = <0>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
> +        interrupt-parent = <&gpio>;
> +        spi-max-frequency = <5000000>;
> +        spi-cpol;
> +        spi-cpha;
> +
> +        adi,temp-channel;
> +
> +        channel@0 {
> +          reg = <0>;
> +          adi,bipolar;
> +
> +          diff-channels = <0 1>;
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +
> +          diff-channels = <2 3>;
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          adi,bipolar;
> +
> +          diff-channels = <4 5>;
> +        };
> +
> +        channel@3 {
> +          reg = <3>;
> +          adi,bipolar;
> +
> +          diff-channels = <6 7>;
> +        };
> +
> +        channel@4 {
> +          reg = <4>;
> +
> +          diff-channels = <8 9>;
> +        };
> +      };
> +    };
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] dt-bindings: adc: add AD717X

[Jonathan Cameron]

> +  adi,temp-channel:
> +    description:
> +      Enables temperature reading channel

Why not always enable it?

> +    type: boolean
> +
> +  dependencies: