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

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

[mitrutzceclan]

From: Dumitru Ceclan <mitrutzceclan@gmail.com>

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

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V5->V6
 - Moved global required property to proper placement

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

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

[PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[mitrutzceclan]

From: Dumitru Ceclan <mitrutzceclan@gmail.com>

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


Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
V5->V6
 - No changes

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

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

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[Andy Shevchenko]

On Thu, Nov 23, 2023 at 05:23:22PM +0200, mitrutzceclan wrote:
> From: Dumitru Ceclan <mitrutzceclan@gmail.com>

Thank you for the update!
My comments below.

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

One blank line is enough here.

> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

...

> V5->V6
>  - No changes

Don't issue patches too often (minimum gap between versions is 24h).

...

> +	help
> +	  Say yes here to build support for Analog Devices AD7173 and similar ADC
> +	  Currently supported models:
> +	    AD7172-2,
> +	    AD7173-8,
> +	    AD7175-2,
> +	    AD7176-2

I would use

 - FOO
 - BAR

style that will reduce amount of potential churn if you need to add an entry at
the end of this list.

> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad7173.

...

> +#include <linux/stddef.h>

You probably meant types.h here (it will include stddef, at least most of
the code relies on that), which is currently absent.

...

> +struct ad7173_device_info {
> +	char *name;
> +	unsigned int id;
> +	unsigned int num_inputs;
> +	unsigned int num_configs;
> +	unsigned int num_channels;

> +	unsigned char num_gpios;

I would use u8 as you have done for cfg_slot, for example. As it holds a number
and not a real character.

> +	bool has_temp;
> +	unsigned int clock;
> +
> +	const unsigned int *sinc5_data_rates;
> +	unsigned int num_sinc5_data_rates;
> +};

...

> +struct ad7173_channel_config {
> +	u8 cfg_slot;
> +	bool live;

Perhaps a blank line?

> +	/* Following fields are used to compare equality. */
> +	struct_group(config_props,
> +		bool bipolar;
> +		bool input_buf;
> +		u8 odr;
> +		u8 ref_sel;
> +	);
> +};

...

> +struct ad7173_state {
> +	const struct ad7173_device_info *info;
> +	struct ad_sigma_delta sd;

It might be better to embed that struct first. In any case you always can
consult with `pahole` tool for data structure layouts.

> +	struct ad7173_channel *channels;
> +	struct regulator_bulk_data regulators[3];
> +	unsigned int adc_mode;
> +	unsigned int interface_mode;
> +	unsigned int num_channels;
> +	struct ida cfg_slots_status;
> +	unsigned long long config_usage_counter;
> +	unsigned long long *config_cnts;
> +#if IS_ENABLED(CONFIG_GPIOLIB)
> +	struct regmap *reg_gpiocon_regmap;
> +	struct gpio_regmap *gpio_regmap;
> +#endif
> +};

...

> +static const char *const ad7173_ref_sel_str[] = {
> +	[AD7173_SETUP_REF_SEL_EXT_REF]    = "refin",
> +	[AD7173_SETUP_REF_SEL_EXT_REF2]   = "refin2",
> +	[AD7173_SETUP_REF_SEL_INT_REF]    = "refout-avss",

> +	[AD7173_SETUP_REF_SEL_AVDD1_AVSS] = "avdd"

Leave trailing comma here as well.

> +};

...

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

For example, here st->sd become a no-op at compile time (see above about
placing sd to be the first member). The code generation can be checked
(for the size) by bloat-o-meter.

...

> +static int ad7173_free_config_slot_lru(struct ad7173_state *st)

> +static int ad7173_load_config(struct ad7173_state *st,
> +			      struct ad7173_channel_config *cfg)

Have you checked, btw, list_lru.h? Maybe all this can be simply changed by
using existing library?

...

> +	return vref / (MICRO/MILLI);

Wouldn't MILLI in the denominator just suffice?

...

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		reg = st->channels[chan->address].cfg.odr;
> +
> +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}

> +	return -EINVAL;

Make this 'default' case.

...

> +static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	int i, ret;
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		if (test_bit(i, scan_mask))
> +			ret = ad7173_set_channel(&st->sd, i);
> +		else
> +			ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);

> +

Unneeded blank line.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

> +static int ad7173_debug(struct iio_dev *indio_dev, unsigned int reg,
> +			unsigned int writeval, unsigned int *readval)

Hmm... The function suggests it debugs something or helps with debugging
something. Without actual description is hard to understand the purpose.
Can you add a top comment on this function with explanations?

...

> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);

One line?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");

...

> +			return dev_err_probe(dev, -EINVAL,
> +				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);

Seems broken indentation.

...

> +		ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> +		if (!ret) {
> +			for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> +				if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> +					ref_sel = i;
> +					break;
> +				}

> +			if (i == ARRAY_SIZE(ad7173_ref_sel_str))
> +				return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);

Too long line.

> +		} else if (ret != -EINVAL) {
> +			return dev_err_probe(dev, ret, "Invalid channel reference value");
> +		}


Use standard pattern and it will be easier to see that 'else' is redundant.

		if (ret == -EINVAL) // However I don't like this handling of
				    // properties, but up to you and maintainer
			ret = 0;
		if (ret)
			return dev_err_probe(...);


BUT. Isn't it a home grown variant of fwnode_property_match_property_string()?

...

> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);

Why casting?

> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Cannot use reference %u", ref_sel);

...

> +			return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8");

Missing \n. Check all your messages that they are terminated with \n.

...

> +	struct ad7173_state *st;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	int ret;

Reversed xmas tree order?

	struct device *dev = &spi->dev;
	struct iio_dev *indio_dev;
	struct ad7173_state *st;
	int ret;

...

> +static const struct of_device_id ad7173_of_match[] = {
> +	{ .compatible = "adi,ad7172-2",
> +	  .data = &ad7173_device_info[ID_AD7172_2], },
> +	{ .compatible = "adi,ad7173-8",
> +	  .data = &ad7173_device_info[ID_AD7173_8], },
> +	{ .compatible = "adi,ad7175-2",
> +	  .data = &ad7173_device_info[ID_AD7175_2], },
> +	{ .compatible = "adi,ad7176-2",
> +	  .data = &ad7173_device_info[ID_AD7176_2], },

Last inner commas are not needed.

> +	{ }
> +};

...

> +static const struct spi_device_id ad7173_id_table[] = {
> +	{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2], },
> +	{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8], },
> +	{ "ad7175-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_2], },
> +	{ "ad7176-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7176_2], },

Ditto.

> +	{ }
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[Andy Shevchenko]

On Thu, Nov 23, 2023 at 06:47:26PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 23, 2023 at 05:23:22PM +0200, mitrutzceclan wrote:

...

> > +static int ad7173_free_config_slot_lru(struct ad7173_state *st)
> 
> > +static int ad7173_load_config(struct ad7173_state *st,
> > +			      struct ad7173_channel_config *cfg)
> 
> Have you checked, btw, list_lru.h? Maybe all this can be simply changed by
> using existing library?

Okay, it seems specific to MM, but maybe there something similar done which
can be split into generic LRU library? In any case it seems too much for this
nice series, so can you just add a comment on top of these functions to
mention that it may be switched to a generic LRU implementation if one exists?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[Jonathan Cameron]

> 
> ...
> 
> > +	return vref / (MICRO/MILLI);  
> 
> Wouldn't MILLI in the denominator just suffice?

Just a quick comment here. Given this is converting from micro to milli units
I'd consider the maths here be acting as documentation of that which would be lost if
/MILLI only used.  Need spaces around the / though


> 
> ...
> 
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		reg = st->channels[chan->address].cfg.odr;
> > +
> > +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> > +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> > +
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	}  
> 
> > +		ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> > +		if (!ret) {
> > +			for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> > +				if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> > +					ref_sel = i;
> > +					break;
> > +				}  
> 
> > +			if (i == ARRAY_SIZE(ad7173_ref_sel_str))
> > +				return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);  
> 
> Too long line.
> 
> > +		} else if (ret != -EINVAL) {
> > +			return dev_err_probe(dev, ret, "Invalid channel reference value");
> > +		}  
> 
> 
> Use standard pattern and it will be easier to see that 'else' is redundant.
> 
> 		if (ret == -EINVAL) // However I don't like this handling of
> 				    // properties, but up to you and maintainer

Personally I'd check for existence of property first and only try reading if it
exists.  Avoid dance with resetting ret to 0.

> 			ret = 0;
> 		if (ret)
> 			return dev_err_probe(...);
> 
> 
> BUT. Isn't it a home grown variant of fwnode_property_match_property_string()?

true enough...  I'd still add an existence check first given this one is optional.

Jonathan

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[kernel test robot]

Hi mitrutzceclan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.7-rc2 next-20231124]
[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/mitrutzceclan/iio-adc-ad7173-add-AD7173-driver/20231123-233012
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231123152331.5751-2-user%40HYB-hhAwRlzzMZb
patch subject: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20231124/202311241251.qDeS0ZtB-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241251.qDeS0ZtB-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/202311241251.qDeS0ZtB-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/ad7173.c:329:22: warning: variable 'offset' set but not used [-Wunused-but-set-variable]
           ptrdiff_t cmp_size, offset;
                               ^
   1 warning generated.


vim +/offset +329 drivers/iio/adc/ad7173.c

   324	
   325	static struct ad7173_channel_config *
   326	ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
   327	{
   328		struct ad7173_channel_config *cfg_aux;
 > 329		ptrdiff_t cmp_size, offset;
   330		int i;
   331	
   332		offset = offsetofend(struct ad7173_channel_config, live);
   333		cmp_size = sizeof_field(struct ad7173_channel_config, config_props);
   334	
   335		for (i = 0; i < st->num_channels; i++) {
   336			cfg_aux = &st->channels[i].cfg;
   337	
   338			if (cfg_aux->live &&
   339			    !memcmp(&cfg->bipolar, &cfg_aux->bipolar, cmp_size))
   340				return cfg_aux;
   341		}
   342		return NULL;
   343	}
   344	

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

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[Jonathan Cameron]

On Thu, 23 Nov 2023 17:23:22 +0200
mitrutzceclan <mitrutzceclan@gmail.com> wrote:

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

Hi.

A few minor things from me.

thanks,

Jonathan

> new file mode 100644
> index 000000000000..ca307d406cee
> --- /dev/null
> +++ b/drivers/iio/adc/ad7173.c
> @@ -0,0 +1,962 @@

...

> +
> +static int ad7173_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct ad7173_channel *ch = &st->channels[chan->address];
> +	unsigned int reg;
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* disable channel after single conversion */
> +		ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 250000000;
> +			*val2 = 800273203; /* (2^24 * 477) / 10 */
> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> +			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (chan->type == IIO_TEMP) {
> +			*val = -874379;
> +		} else {
> +			if (ch->cfg.bipolar)
> +				*val = -BIT(chan->scan_type.realbits - 1);
> +			else
> +				*val = 0;
Ideally if the channel isn't bipolar, don't expose the offset at all. Default
assumption if there is no such file is 0 offset. 
If that's fiddly to do then fine to keep the file though.

> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		reg = st->channels[chan->address].cfg.odr;
> +
> +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;


...


> +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> +{
> +	struct ad7173_channel *channels_st_priv_arr, *chan_st_priv;
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *chan_arr, *chan;
> +	struct fwnode_handle *child;
> +	unsigned int ain[2], chan_index = 0;
> +	unsigned int num_channels;
> +	const char *ref_label;
> +	u32 ref_sel;
> +	int i, ret;
> +
> +	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
> +	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
> +	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);

If only some of them are supplied, the driver works fine as long as no channel is using them?
If so should probably get the optionally then check for availability of the ones you
want.  I'm sure someone will just wire up ref.  Incidentally, ref2 isn't there for all chips
I think, so we should not get it on the ones where it can't exist.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add regulators disable action\n");
> +
> +	num_channels = device_get_child_node_count(dev);
> +
> +	if (st->info->has_temp)
> +		num_channels++;
> +
> +	if (num_channels == 0)
> +		return 0;

Isn't it all a bit pointless if this happens?  So maybe an error makes more sense?

> +	st->num_channels = num_channels;
> +
> +	chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels, GFP_KERNEL);
> +	if (!chan_arr)
> +		return -ENOMEM;
> +
> +	channels_st_priv_arr = devm_kcalloc(dev, num_channels,
> +					    sizeof(*channels_st_priv_arr),
> +					    GFP_KERNEL);
> +	if (!channels_st_priv_arr)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = chan_arr;
> +	indio_dev->num_channels = num_channels;
> +	st->channels = channels_st_priv_arr;
> +
> +	if (st->info->has_temp) {
> +		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
> +		chan_st_priv = &channels_st_priv_arr[chan_index];
> +		chan_st_priv->ain =
> +			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
> +		chan_st_priv->cfg.bipolar = false;
> +		chan_st_priv->cfg.input_buf = true;
> +		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> +
> +		chan_index++;
> +	}
> +
> +	device_for_each_child_node(dev, child) {
> +		chan = &chan_arr[chan_index];
> +		chan_st_priv = &channels_st_priv_arr[chan_index];
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     ain, ARRAY_SIZE(ain));
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (ain[0] >= st->info->num_inputs ||
> +		    ain[1] >= st->info->num_inputs) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
> +		}
> +
> +		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> +		if (!ret) {
> +			for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> +				if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> +					ref_sel = i;
> +					break;
> +				}
> +
> +			if (i == ARRAY_SIZE(ad7173_ref_sel_str))
Missing fwnode_handle_put(child). Same for other error paths in th device_for_each_child_node() 
loop. I wonder if that can get cleaned up with the cleanup.h magic.  Anyhow, that's a question
for another day.

> +				return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);


> +		} else if (ret != -EINVAL) {
Up to you, but you could be lazier and set ref_label = ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF] before
trying to read it then don't bother handling the fwnode property read error at all
That's common practice when there is a default.

It would unnecessarily run the string match when the property wasn't there, but that's not too bad
for the code simplification it would give I think

> +			return dev_err_probe(dev, ret, "Invalid channel reference value");
> +		}
> +
> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Cannot use reference %u", ref_sel);
> +		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 &&
> +		    st->info->id != AD7173_ID)
> +			return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8");

Why all the way down here?  I'd do this a few lines earlier so we haven't already used the bonkers
value.

> +		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
> +			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> +		chan_st_priv->cfg.ref_sel = ref_sel;
> +
> +		*chan = ad7173_channel_template;
> +		chan->address = chan_index;
> +		chan->scan_index = chan_index;
> +		chan->channel = ain[0];
> +		chan->channel2 = ain[1];
> +		chan->differential = true;
> +
> +		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> +		chan_st_priv->chan_reg = chan_index;
> +		chan_st_priv->cfg.input_buf = true;
> +		chan_st_priv->cfg.odr = 0;
> +
> +		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
> +		if (chan_st_priv->cfg.bipolar)
> +			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
> +
> +		chan_index++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7173_probe(struct spi_device *spi)
> +{
> +	struct ad7173_state *st;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->info = device_get_match_data(dev);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	ida_init(&st->cfg_slots_status);
> +	ret = devm_add_action_or_reset(dev, ad7173_ida_destroy, st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad7173_info;
> +
> +	spi->mode = SPI_MODE_3;
> +
> +	ad7173_sigma_delta_info.num_slots = st->info->num_configs;
> +	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, indio_dev);

Used?  I'm not immediately seeing where. If not, don't set it.

Thanks,

Jonathan

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[Ceclan Dumitru]

On 11/25/23 19:21, Jonathan Cameron wrote:
> On Thu, 23 Nov 2023 17:23:22 +0200
> mitrutzceclan <mitrutzceclan@gmail.com> wrote:
> 
>> From: Dumitru Ceclan <mitrutzceclan@gmail.com>

...
>> +	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
>> +	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
>> +	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
>> +
>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
>> +				      st->regulators);
> 
> If only some of them are supplied, the driver works fine as long as no channel is using them?
> If so should probably get the optionally then check for availability of the ones you
> want.  I'm sure someone will just wire up ref.  Incidentally, ref2 isn't there for all chips
> I think, so we should not get it on the ones where it can't exist.
> 


This sets a dummy regulator in place if no proper supply is found. Then
the call regulator_get_voltage() on the dummy will fail. About getting
ref2, sure, I'll set the string only with the right ID.

...
>> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);
>> +		if (ret < 0)
>> +			return dev_err_probe(dev, ret,
>> +					     "Cannot use reference %u", ref_sel);

Here the probe would not continue if a channel selects a supply that is
not available in the DT.

Re: [PATCH v6 2/2] iio: adc: ad7173: add AD7173 driver

[Jonathan Cameron]

On Mon, 4 Dec 2023 18:49:21 +0200
Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:

> On 11/25/23 19:21, Jonathan Cameron wrote:
> > On Thu, 23 Nov 2023 17:23:22 +0200
> > mitrutzceclan <mitrutzceclan@gmail.com> wrote:
> >   
> >> From: Dumitru Ceclan <mitrutzceclan@gmail.com>  
> 
> ...
> >> +	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
> >> +	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
> >> +	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
> >> +
> >> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> >> +				      st->regulators);  
> > 
> > If only some of them are supplied, the driver works fine as long as no channel is using them?
> > If so should probably get the optionally then check for availability of the ones you
> > want.  I'm sure someone will just wire up ref.  Incidentally, ref2 isn't there for all chips
> > I think, so we should not get it on the ones where it can't exist.
> >   
> 
> 
> This sets a dummy regulator in place if no proper supply is found. Then
> the call regulator_get_voltage() on the dummy will fail. About getting
> ref2, sure, I'll set the string only with the right ID.
> 
> ...
> >> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);
> >> +		if (ret < 0)
> >> +			return dev_err_probe(dev, ret,
> >> +					     "Cannot use reference %u", ref_sel);  
> 
> Here the probe would not continue if a channel selects a supply that is
> not available in the DT.
Ok. It's a little ugly though - so maybe a comment at the bulk_get to say
it will fail cleanly a bit later.

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

[Conor Dooley]

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

Yo,

This looks a lot better IMO, thanks.

On Thu, Nov 23, 2023 at 05:23:21PM +0200, mitrutzceclan wrote:
> +patternProperties:
> +  "^channel@[0-9a-f]$":
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false

> +      bipolar:
> +        type: boolean

This should be defined in adc.yaml, so duping it here is not needed?

> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              const: adi,ad7173-8
> +    then:
> +      properties:
> +        refin2-supply: false
> +      patternProperties:
> +        "^channel@[0-9a-f]$":
> +          properties:
> +            adi,reference-select:
> +              enum:
> +                - refin
> +                - refout-avss
> +                - avdd

I assume you tested that this restriction works as intended?

Cheers,
Conor.

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

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

[Ceclan Dumitru]

On 11/23/23 20:02, Conor Dooley wrote:
> On Thu, Nov 23, 2023 at 05:23:21PM +0200, mitrutzceclan wrote:
>> +allOf:
>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          not:
>> +            contains:
>> +              const: adi,ad7173-8
>> +    then:
>> +      properties:
>> +        refin2-supply: false
>> +      patternProperties:
>> +        "^channel@[0-9a-f]$":
>> +          properties:
>> +            adi,reference-select:
>> +              enum:
>> +                - refin
>> +                - refout-avss
>> +                - avdd
> 
> I assume you tested that this restriction works as intended?
> 

Yes, for both reference and reference-select

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

[Jonathan Cameron]

On Thu, 23 Nov 2023 17:23:21 +0200
mitrutzceclan <mitrutzceclan@gmail.com> wrote:

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

An example of reference selection would be good to have. Otherwise
looks fine to me, but I'll wait for the binding experts to take another
look.