[PATCH v3 0/2] iio: adc: add max11410 adc driver

[PATCH v3 0/2] iio: adc: add max11410 adc driver

[Ibrahim Tilki]

I added filter enable attributes for 50Hz and 60Hz FIR filters.
Disabling both will set filter to sinc4, allowing higher sampling rates.

Regards,
Ibrahim

Note: No sign-off tag for David as he was unreachable when the initial patch was sent.

Changelog:
since v3:
  - prefix defines with MAX11410_
  - group vref regulators
  - use builtin iio_validate_scan_mask_onehot
  - validate iio trigger
  - move scan data into state struct
  - require vrefn regulator in DT if used by any channel
  - don't require irq for triggered buffer
  - remove filter sysfs attr and ABI documentation
  - add in_voltage_filter[0-1]_notch_{center,en} attrs

since v2:
  - remove bit position shifting, use field_prep instead
  - reduce the amount of reg writes in max11410_configure_channel
  - add error checking in max11410_parse_channels
  - remove some unneeded blank lines and minor style fixes
  - remove scan data assignment in max11410_trigger_handler


Ibrahim Tilki (2):
  iio: adc: add max11410 adc driver
  dt-bindings: iio: adc: add adi,max11410.yaml

 .../bindings/iio/adc/adi,max11410.yaml        | 165 +++
 drivers/iio/adc/Kconfig                       |  13 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max11410.c                    | 954 ++++++++++++++++++
 4 files changed, 1133 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
 create mode 100644 drivers/iio/adc/max11410.c

-- 
2.36.1

[PATCH v3 1/2] iio: adc: add max11410 adc driver

[Ibrahim Tilki]

Adding support for max11410 24-bit, 1.9ksps delta-sigma adc which
has 3 differential reference and 10 differential channel inputs.
Inputs and references can be buffered internally. Inputs can also
be amplified with internal PGA.

Device has four digital filter modes: FIR50/60, FIR50, FIR60 and SINC4.
FIR 50Hz and 60Hz rejections can be enabled/disabled separately.
Digital filter selection affects sampling frequency range so driver
has to consider the configured filter when configuring sampling frequency.

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>
---
 drivers/iio/adc/Kconfig    |  13 +
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max11410.c | 954 +++++++++++++++++++++++++++++++++++++
 3 files changed, 968 insertions(+)
 create mode 100644 drivers/iio/adc/max11410.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 71ab0a06a..eccb8d139 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -654,6 +654,19 @@ config MAX1118
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1118.
 
+config MAX11410
+	tristate "Analog Devices MAX11410 ADC driver"
+	depends on SPI
+	select REGMAP_SPI
+	select IIO_BUFFER
+	select IIO_TRIGGER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Analog Devices MAX11410 ADCs.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max11410.
+
 config MAX1241
 	tristate "Maxim max1241 ADC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 39d806f6d..679cc3c05 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
 obj-$(CONFIG_MAX1118) += max1118.o
+obj-$(CONFIG_MAX11410) += max11410.o
 obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX9611) += max9611.o
diff --git a/drivers/iio/adc/max11410.c b/drivers/iio/adc/max11410.c
new file mode 100644
index 000000000..383cef3a0
--- /dev/null
+++ b/drivers/iio/adc/max11410.c
@@ -0,0 +1,954 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#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/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.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>
+
+#define MAX11410_REG_CONV_START	0x01
+#define		MAX11410_CONV_TYPE_SINGLE	0x00
+#define		MAX11410_CONV_TYPE_CONTINUOUS	0x01
+#define MAX11410_REG_CAL_START	0x03
+#define		MAX11410_CAL_START_SELF		0x00
+#define		MAX11410_CAL_START_PGA		0x01
+#define MAX11410_REG_GP1_CTRL	0x05
+#define		MAX11410_GP1_CTRL_INTRB		0xC1
+#define MAX11410_REG_FILTER	0x08
+#define		MAX11410_FILTER_RATE_MASK	GENMASK(3, 0)
+#define		MAX11410_FILTER_RATE_MAX	0x0F
+#define		MAX11410_FILTER_LINEF_MASK	GENMASK(5, 4)
+#define		MAX11410_FILTER_50HZ		BIT(5)
+#define		MAX11410_FILTER_60HZ		BIT(4)
+#define MAX11410_REG_CTRL	0x09
+#define		MAX11410_CTRL_REFSEL_MASK	GENMASK(2, 0)
+#define		MAX11410_CTRL_VREFN_BUF_BIT	BIT(3)
+#define		MAX11410_CTRL_VREFP_BUF_BIT	BIT(4)
+#define		MAX11410_CTRL_FORMAT_BIT	BIT(5)
+#define		MAX11410_CTRL_UNIPOLAR_BIT	BIT(6)
+#define MAX11410_REG_MUX_CTRL0	0x0B
+#define MAX11410_REG_PGA	0x0E
+#define		MAX11410_PGA_GAIN_MASK		GENMASK(2, 0)
+#define		MAX11410_PGA_SIG_PATH_MASK	GENMASK(5, 4)
+#define		MAX11410_PGA_SIG_PATH_BUFFERED	0x00
+#define		MAX11410_PGA_SIG_PATH_BYPASS	0x01
+#define		MAX11410_PGA_SIG_PATH_PGA	0x02
+#define MAX11410_REG_DATA0	0x30
+#define MAX11410_REG_STATUS	0x38
+#define		MAX11410_STATUS_CONV_READY_BIT	BIT(0)
+#define		MAX11410_STATUS_CAL_READY_BIT	BIT(2)
+
+#define MAX11410_REFSEL_AVDD_AGND	0x03
+#define MAX11410_REFSEL_MAX		0x06
+#define MAX11410_SIG_PATH_MAX		0x02
+#define MAX11410_CHANNEL_INDEX_MAX	0x0A
+#define MAX11410_AINP_AVDD	0x0A
+#define MAX11410_AINN_GND	0x0A
+
+#define MAX11410_CONVERSION_TIMEOUT_MS	2000
+#define MAX11410_CALIB_TIMEOUT_MS	2000
+
+enum max11410_filter {
+	MAX11410_FILTER_FIR5060,
+	MAX11410_FILTER_FIR50,
+	MAX11410_FILTER_FIR60,
+	MAX11410_FILTER_SINC4,
+};
+
+static const u8 max11410_sampling_len[] = {
+	[MAX11410_FILTER_FIR5060] = 5,
+	[MAX11410_FILTER_FIR50] = 6,
+	[MAX11410_FILTER_FIR60] = 6,
+	[MAX11410_FILTER_SINC4] = 10,
+};
+
+static const int max11410_sampling_rates[4][10][2] = {
+	[MAX11410_FILTER_FIR5060] = {
+		{1, 100000},
+		{2, 100000},
+		{4, 200000},
+		{8, 400000},
+		{16, 800000}
+	},
+	[MAX11410_FILTER_FIR50] = {
+		{1, 300000},
+		{2, 700000},
+		{5, 300000},
+		{10, 700000},
+		{21, 300000},
+		{40}
+	},
+	[MAX11410_FILTER_FIR60] = {
+		{1, 300000},
+		{2, 700000},
+		{5, 300000},
+		{10, 700000},
+		{21, 300000},
+		{40}
+	},
+	[MAX11410_FILTER_SINC4] = {
+		{4},
+		{10},
+		{20},
+		{40},
+		{60},
+		{120},
+		{240},
+		{480},
+		{960},
+		{1920}
+	}
+};
+
+struct max11410_channel_config {
+	u32 settling_time_us;
+	u8 refsel;
+	u8 sig_path;
+	u8 gain;
+	bool bipolar;
+	bool buffered_vrefp;
+	bool buffered_vrefn;
+};
+
+struct max11410_state {
+	struct spi_device *spi_dev;
+	struct iio_trigger *trig;
+	struct completion completion;
+	struct mutex lock; /* Prevent changing channel config during sampling */
+	struct regmap *regmap;
+	struct regulator *avdd;
+	struct regulator *vrefp[3];
+	struct regulator *vrefn[3];
+	struct max11410_channel_config *channels;
+	struct {
+		int data __aligned(IIO_DMA_MINALIGN);
+		s64 ts __aligned(8);
+	} scan;
+};
+
+static const struct iio_chan_spec chanspec_template = {
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.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),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_HARDWAREGAIN)
+					| BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_type = {
+			.sign = 's',
+			.realbits = 24,
+			.storagebits = 24,
+			.endianness = IIO_LE,
+		},
+};
+
+static unsigned int max11410_reg_size(unsigned int reg)
+{
+	/* Registers from 0x00 to 0x10 are 1 byte, the rest are 3 bytes long. */
+	return reg <= 0x10 ? 1 : 3;
+}
+
+static int max11410_write_reg(struct max11410_state *st,
+			      unsigned int reg,
+			      unsigned int val)
+{
+	/* This driver only needs to write 8-bit registers */
+	if (max11410_reg_size(reg) != 1)
+		return -EINVAL;
+
+	return regmap_write(st->regmap, reg, val);
+}
+
+static int max11410_read_reg(struct max11410_state *st,
+			     unsigned int reg,
+			     int *val)
+{
+	int ret;
+
+	if (max11410_reg_size(reg) == 3) {
+		ret = regmap_bulk_read(st->regmap, reg, &st->scan.data, 3);
+		if (ret)
+			return ret;
+
+		*val = get_unaligned_be24(&st->scan.data);
+		return 0;
+	}
+
+	return regmap_read(st->regmap, reg, val);
+}
+
+static struct regulator *max11410_get_vrefp(struct max11410_state *st, u8 refsel)
+{
+	refsel = refsel % 4;
+	if (refsel == 3)
+		return st->avdd;
+
+	return st->vrefp[refsel];
+}
+
+static struct regulator *max11410_get_vrefn(struct max11410_state *st, u8 refsel)
+{
+	if (refsel > 2)
+		return NULL;
+
+	return st->vrefn[refsel];
+}
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x39,
+};
+
+static int max11410_sample(struct max11410_state *st, int *sample_raw)
+{
+	int val, ret;
+
+	if (st->spi_dev->irq > 0)
+		reinit_completion(&st->completion);
+
+	/* Start Conversion */
+	ret = max11410_write_reg(st, MAX11410_REG_CONV_START, MAX11410_CONV_TYPE_SINGLE);
+	if (ret)
+		return ret;
+
+	if (st->spi_dev->irq > 0) {
+		/* Wait for an interrupt. */
+		ret = wait_for_completion_timeout(&st->completion,
+						  msecs_to_jiffies(MAX11410_CONVERSION_TIMEOUT_MS));
+		if (!ret)
+			return -ETIMEDOUT;
+	} else {
+		/* Wait for status register Conversion Ready flag */
+		ret = read_poll_timeout(max11410_read_reg, ret,
+					ret || (val & MAX11410_STATUS_CONV_READY_BIT),
+					5000, MAX11410_CONVERSION_TIMEOUT_MS * 1000, true,
+					st, MAX11410_REG_STATUS, &val);
+		if (ret)
+			return ret;
+	}
+
+	/* Read ADC Data */
+	return max11410_read_reg(st, MAX11410_REG_DATA0, sample_raw);
+}
+
+static ssize_t max11410_notch_en_show(struct device *dev,
+				      struct device_attribute *devattr,
+				      char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct max11410_state *state = iio_priv(indio_dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(devattr);
+	int ret;
+	unsigned int val;
+
+	ret = max11410_read_reg(state, MAX11410_REG_FILTER, &val);
+	if (ret)
+		return ret;
+
+	switch (iio_attr->address) {
+	case 0:
+		val = !FIELD_GET(MAX11410_FILTER_50HZ, val);
+		break;
+	case 1:
+		val = !FIELD_GET(MAX11410_FILTER_60HZ, val);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%d\n", val);
+}
+
+static ssize_t max11410_notch_en_store(struct device *dev,
+				       struct device_attribute *devattr,
+				       const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct max11410_state *state = iio_priv(indio_dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(devattr);
+	unsigned int filter_bit;
+	int ret;
+
+	filter_bit = iio_attr->address == 0 ? MAX11410_FILTER_50HZ : MAX11410_FILTER_60HZ;
+	if (sysfs_streq(buf, "0"))
+		ret = regmap_set_bits(state->regmap, MAX11410_REG_FILTER, filter_bit);
+	else if (sysfs_streq(buf, "1"))
+		ret = regmap_clear_bits(state->regmap, MAX11410_REG_FILTER, filter_bit);
+	else
+		return -EINVAL;
+
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static IIO_CONST_ATTR(in_voltage_filter0_notch_center, "50");
+static IIO_CONST_ATTR(in_voltage_filter1_notch_center, "60");
+
+static IIO_DEVICE_ATTR(in_voltage_filter0_notch_en, 0644,
+		       max11410_notch_en_show, max11410_notch_en_store, 0);
+static IIO_DEVICE_ATTR(in_voltage_filter1_notch_en, 0644,
+		       max11410_notch_en_show, max11410_notch_en_store, 1);
+
+static struct attribute *max11410_attributes[] = {
+	&iio_const_attr_in_voltage_filter0_notch_center.dev_attr.attr,
+	&iio_const_attr_in_voltage_filter1_notch_center.dev_attr.attr,
+	&iio_dev_attr_in_voltage_filter0_notch_en.dev_attr.attr,
+	&iio_dev_attr_in_voltage_filter1_notch_en.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group max11410_attribute_group = {
+	.attrs = max11410_attributes,
+};
+
+static int max11410_set_input_mux(struct max11410_state *st, u8 ainp, u8 ainn)
+{
+	if (ainp > MAX11410_CHANNEL_INDEX_MAX || ainn > MAX11410_CHANNEL_INDEX_MAX)
+		return -EINVAL;
+
+	return max11410_write_reg(st, MAX11410_REG_MUX_CTRL0, (ainp << 4) | ainn);
+}
+
+static int max11410_configure_channel(struct max11410_state *st,
+				      struct iio_chan_spec const *chan)
+{
+	int ret;
+	unsigned int regval;
+	struct max11410_channel_config cfg = st->channels[chan->address];
+
+	if (chan->differential)
+		ret = max11410_set_input_mux(st, chan->channel, chan->channel2);
+	else
+		ret = max11410_set_input_mux(st, chan->channel, MAX11410_AINN_GND);
+
+	if (ret)
+		return ret;
+
+	regval = FIELD_PREP(MAX11410_CTRL_VREFP_BUF_BIT, cfg.buffered_vrefp) |
+		 FIELD_PREP(MAX11410_CTRL_VREFN_BUF_BIT, cfg.buffered_vrefn) |
+		 FIELD_PREP(MAX11410_CTRL_REFSEL_MASK, cfg.refsel) |
+		 FIELD_PREP(MAX11410_CTRL_UNIPOLAR_BIT, !cfg.bipolar);
+	ret = regmap_update_bits(st->regmap, MAX11410_REG_CTRL,
+				 MAX11410_CTRL_REFSEL_MASK | MAX11410_CTRL_VREFN_BUF_BIT |
+				 MAX11410_CTRL_VREFN_BUF_BIT | MAX11410_CTRL_UNIPOLAR_BIT,
+				 regval);
+	if (ret)
+		return ret;
+
+	regval = FIELD_PREP(MAX11410_PGA_SIG_PATH_MASK, cfg.sig_path) |
+		 FIELD_PREP(MAX11410_PGA_GAIN_MASK, cfg.gain);
+	ret = regmap_write(st->regmap, MAX11410_REG_PGA, regval);
+	if (ret)
+		return ret;
+
+	if (cfg.settling_time_us)
+		fsleep(cfg.settling_time_us);
+
+	return ret;
+}
+
+static int max11410_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long info)
+{
+	int ret, reg_val, filter, rate;
+	struct regulator *vrefp, *vrefn;
+	struct max11410_state *state = iio_priv(indio_dev);
+	struct max11410_channel_config cfg = state->channels[chan->address];
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		vrefp = max11410_get_vrefp(state, cfg.refsel);
+		if (!vrefp) {
+			/* We should never get here */
+			return -ENODEV;
+		}
+
+		*val = regulator_get_voltage(vrefp) / 1000;
+		vrefn = max11410_get_vrefn(state, cfg.refsel);
+		if (vrefn)
+			*val -= regulator_get_voltage(vrefn) / 1000;
+
+		if (cfg.bipolar)
+			*val *= 2;
+
+		*val = *val >> cfg.gain;
+
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		if (cfg.bipolar)
+			*val = -BIT(chan->scan_type.realbits - 1);
+		else
+			*val = 0;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&state->lock);
+
+		ret = max11410_configure_channel(state, chan);
+
+		if (!ret)
+			ret = max11410_sample(state, &reg_val);
+
+		mutex_unlock(&state->lock);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		if (ret)
+			return ret;
+
+		*val = reg_val;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val = BIT(cfg.gain);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(state->regmap, MAX11410_REG_FILTER, &reg_val);
+		if (ret)
+			return ret;
+
+		filter = FIELD_GET(MAX11410_FILTER_LINEF_MASK, reg_val);
+		rate = reg_val & MAX11410_FILTER_RATE_MASK;
+		if (rate >= max11410_sampling_len[filter])
+			rate = max11410_sampling_len[filter] - 1;
+
+		*val = max11410_sampling_rates[filter][rate][0];
+		*val2 = max11410_sampling_rates[filter][rate][1];
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+	return -EINVAL;
+}
+
+static const int max11410_hwgain_list[] = {1, 2, 4, 8, 16, 32, 64, 128};
+
+static int max11410_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	int i, ret, reg_val, filter;
+	struct max11410_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		for (i = 0; i < ARRAY_SIZE(max11410_hwgain_list); ++i) {
+			if (val == max11410_hwgain_list[i])
+				break;
+		}
+		if (i == ARRAY_SIZE(max11410_hwgain_list))
+			return -EINVAL;
+
+		st->channels[chan->address].gain = i;
+
+		return 0;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&st->lock);
+
+		ret = regmap_read(st->regmap, MAX11410_REG_FILTER, &reg_val);
+		if (ret)
+			goto out;
+
+		filter = FIELD_GET(MAX11410_FILTER_LINEF_MASK, reg_val);
+
+		for (i = 0; i < max11410_sampling_len[filter]; ++i) {
+			if (val == max11410_sampling_rates[filter][i][0] &&
+			    val2 == max11410_sampling_rates[filter][i][1])
+				break;
+		}
+		if (i == max11410_sampling_len[filter]) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			goto out;
+
+		ret = regmap_write_bits(st->regmap, MAX11410_REG_FILTER,
+					MAX11410_FILTER_RATE_MASK, i);
+		iio_device_release_direct_mode(indio_dev);
+
+out:
+		mutex_unlock(&st->lock);
+
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int max11410_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long info)
+{
+	int ret, reg_val, filter;
+	struct max11410_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(max11410_hwgain_list);
+		*vals = max11410_hwgain_list;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(st->regmap, MAX11410_REG_FILTER, &reg_val);
+		if (ret)
+			return ret;
+
+		filter = FIELD_GET(MAX11410_FILTER_LINEF_MASK, reg_val);
+
+		*vals = (const int *)max11410_sampling_rates[filter];
+		*length = max11410_sampling_len[filter] * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+
+		return IIO_AVAIL_LIST;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info max11410_info = {
+	.read_raw = max11410_read_raw,
+	.write_raw = max11410_write_raw,
+	.read_avail = max11410_read_avail,
+	.attrs = &max11410_attribute_group,
+};
+
+static irqreturn_t max11410_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct max11410_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = max11410_read_reg(st, MAX11410_REG_DATA0, &st->scan.data);
+	if (ret) {
+		dev_err(&indio_dev->dev, "cannot read data\n");
+		goto out;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
+					   iio_get_time_ns(indio_dev));
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int max11410_buffer_postenable(struct iio_dev *indio_dev)
+{
+	int scan_ch, ret;
+	struct max11410_state *st = iio_priv(indio_dev);
+
+	/* Configure GPIO1 of MAX11410 as interrupt output. */
+	ret = max11410_write_reg(st, MAX11410_REG_GP1_CTRL, MAX11410_GP1_CTRL_INTRB);
+	if (ret)
+		return ret;
+
+	scan_ch = ffs(*indio_dev->active_scan_mask) - 1;
+
+	ret = max11410_configure_channel(st, &indio_dev->channels[scan_ch]);
+	if (ret)
+		return ret;
+
+	/* Start continuous conversion. */
+	return max11410_write_reg(st, MAX11410_REG_CONV_START, MAX11410_CONV_TYPE_CONTINUOUS);
+}
+
+static int max11410_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct max11410_state *st = iio_priv(indio_dev);
+
+	/* Stop continuous conversion. */
+	return max11410_write_reg(st, MAX11410_REG_CONV_START, MAX11410_CONV_TYPE_SINGLE);
+}
+
+static const struct iio_buffer_setup_ops max11410_buffer_ops = {
+	.postenable = &max11410_buffer_postenable,
+	.predisable = &max11410_buffer_predisable,
+	.validate_scan_mask = &iio_validate_scan_mask_onehot,
+};
+
+static const struct iio_trigger_ops max11410_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+};
+
+static irqreturn_t max11410_interrupt(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct max11410_state *st = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll_chained(st->trig);
+	else
+		complete(&st->completion);
+
+	return IRQ_HANDLED;
+};
+
+static int max11410_parse_channels(struct max11410_state *st,
+				   struct iio_dev *indio_dev)
+{
+	struct iio_chan_spec *channels;
+	struct fwnode_handle *child;
+	struct max11410_channel_config *cfg;
+	struct device *dev = &st->spi_dev->dev;
+	const char *node_name;
+	unsigned int num_ch;
+	u32 inputs[2];
+	u32 reference, sig_path;
+	int chan_idx = 0;
+	int ret;
+	struct iio_chan_spec chanspec = chanspec_template;
+
+	num_ch = device_get_child_node_count(dev);
+	if (num_ch == 0)
+		return dev_err_probe(&indio_dev->dev,
+				     -ENODEV,
+				     "FW has no channels defined\n");
+
+	/* Reserve space for soft timestamp channel */
+	num_ch++;
+	channels = devm_kcalloc(dev, num_ch,
+				sizeof(struct iio_chan_spec),
+				GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	st->channels = devm_kcalloc(dev, num_ch,
+				    sizeof(struct max11410_channel_config),
+				    GFP_KERNEL);
+	if (!st->channels)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		node_name = fwnode_get_name(child);
+		if (fwnode_property_present(child, "diff-channels")) {
+			ret = fwnode_property_read_u32_array(child,
+							     "diff-channels",
+							     inputs,
+							     ARRAY_SIZE(inputs));
+
+			chanspec.differential = 1;
+		} else {
+			ret = fwnode_property_read_u32(child, "reg", &inputs[0]);
+
+			inputs[1] = 0;
+			chanspec.differential = 0;
+		}
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		if (inputs[0] > MAX11410_CHANNEL_INDEX_MAX ||
+		    inputs[1] > MAX11410_CHANNEL_INDEX_MAX) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&indio_dev->dev,
+					     -EINVAL,
+					     "Invalid channel index for %s, should be less than %d\n",
+					     node_name,
+					     MAX11410_CHANNEL_INDEX_MAX + 1);
+		}
+
+		cfg = &st->channels[chan_idx];
+
+		reference = MAX11410_REFSEL_AVDD_AGND;
+		fwnode_property_read_u32(child, "adi,reference", &reference);
+		if (reference > MAX11410_REFSEL_MAX) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&indio_dev->dev,
+					     -EINVAL,
+					     "Invalid adi,reference value for %s, should be less than %d.\n",
+					     node_name,
+					     MAX11410_REFSEL_MAX + 1);
+		}
+
+		if (!max11410_get_vrefp(st, reference) ||
+		    (!max11410_get_vrefn(st, reference) && reference <= 2)) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&indio_dev->dev,
+					     -EINVAL,
+					     "Invalid VREF configuration for %s, either specify corresponding VREF regulators or change adi,reference property.\n",
+					     node_name);
+		}
+
+		sig_path = MAX11410_PGA_SIG_PATH_BUFFERED;
+		fwnode_property_read_u32(child, "adi,input-mode", &sig_path);
+		if (sig_path > MAX11410_SIG_PATH_MAX) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&indio_dev->dev,
+					     -EINVAL,
+					     "Invalid adi,input-mode value for %s, should be less than %d.\n",
+					     node_name,
+					     MAX11410_SIG_PATH_MAX + 1);
+		}
+
+		/* Enable hardware gain property if input mode is PGA */
+		if (sig_path == MAX11410_PGA_SIG_PATH_PGA)
+			__set_bit(IIO_CHAN_INFO_HARDWAREGAIN, &chanspec.info_mask_separate);
+		else
+			__clear_bit(IIO_CHAN_INFO_HARDWAREGAIN, &chanspec.info_mask_separate);
+
+		fwnode_property_read_u32(child,
+					 "settling-time-us",
+					 &cfg->settling_time_us);
+		cfg->bipolar = fwnode_property_read_bool(child, "bipolar");
+		cfg->buffered_vrefp = fwnode_property_read_bool(child, "adi,buffered-vrefp");
+		cfg->buffered_vrefn = fwnode_property_read_bool(child, "adi,buffered-vrefn");
+		cfg->refsel = reference;
+		cfg->sig_path = sig_path;
+
+		chanspec.address = chan_idx;
+		chanspec.scan_index = chan_idx;
+		chanspec.channel = inputs[0];
+		chanspec.channel2 = inputs[1];
+
+		channels[chan_idx] = chanspec;
+		chan_idx++;
+	}
+
+	channels[chan_idx] = (struct iio_chan_spec)IIO_CHAN_SOFT_TIMESTAMP(chan_idx);
+
+	indio_dev->num_channels = chan_idx + 1;
+	indio_dev->channels = channels;
+
+	return 0;
+}
+
+static void max11410_disable_reg(void *reg)
+{
+	regulator_disable(reg);
+}
+
+static int max11410_init_vref(struct device *dev,
+			      struct regulator **vref,
+			      const char *id)
+{
+	int ret;
+	struct regulator *reg;
+
+	reg = devm_regulator_get_optional(dev, id);
+	if (IS_ERR(reg)) {
+		*vref = NULL;
+		return 0;
+	}
+	ret = regulator_enable(reg);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to enable regulator %s\n", id);
+
+	*vref = reg;
+	return devm_add_action_or_reset(dev, max11410_disable_reg, reg);
+}
+
+static int max11410_calibrate(struct max11410_state *st, u32 cal_type)
+{
+	int ret, val;
+
+	ret = max11410_write_reg(st, MAX11410_REG_CAL_START, cal_type);
+	if (ret)
+		return ret;
+
+	/* Wait for status register Calibration Ready flag */
+	return read_poll_timeout(max11410_read_reg, ret,
+				 ret || (val & MAX11410_STATUS_CAL_READY_BIT),
+				 50000, MAX11410_CALIB_TIMEOUT_MS * 1000, true,
+				 st, MAX11410_REG_STATUS, &val);
+}
+
+static int max11410_self_calibrate(struct max11410_state *st)
+{
+	int ret, i;
+
+	ret = regmap_write_bits(st->regmap, MAX11410_REG_FILTER, MAX11410_FILTER_RATE_MASK,
+				FIELD_PREP(MAX11410_FILTER_RATE_MASK, MAX11410_FILTER_RATE_MAX));
+	if (ret)
+		return ret;
+
+	ret = max11410_calibrate(st, MAX11410_CAL_START_SELF);
+	if (ret)
+		return ret;
+
+	ret = regmap_write_bits(st->regmap, MAX11410_REG_PGA, MAX11410_PGA_SIG_PATH_MASK,
+				FIELD_PREP(MAX11410_PGA_SIG_PATH_MASK, MAX11410_PGA_SIG_PATH_PGA));
+	if (ret)
+		return ret;
+
+	/* PGA calibrations */
+	for (i = 1; i < ARRAY_SIZE(max11410_hwgain_list); ++i) {
+		ret = regmap_write_bits(st->regmap, MAX11410_REG_PGA, MAX11410_PGA_GAIN_MASK, i);
+		if (ret)
+			return ret;
+
+		ret = max11410_calibrate(st, MAX11410_CAL_START_PGA);
+		if (ret)
+			return ret;
+	}
+
+	/* Cleanup */
+	ret = regmap_write_bits(st->regmap, MAX11410_REG_PGA, MAX11410_PGA_GAIN_MASK, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write_bits(st->regmap, MAX11410_REG_FILTER, MAX11410_FILTER_RATE_MASK, 0);
+	if (ret)
+		return ret;
+
+	return regmap_write_bits(st->regmap, MAX11410_REG_PGA, MAX11410_PGA_SIG_PATH_MASK,
+				 FIELD_PREP(MAX11410_PGA_SIG_PATH_MASK,
+					    MAX11410_PGA_SIG_PATH_BUFFERED));
+}
+
+static int max11410_probe(struct spi_device *spi)
+{
+	struct max11410_state *st;
+	struct iio_dev *indio_dev;
+	struct device *dev = &spi->dev;
+	const char *vrefp_regs[] = { "vref0p", "vref1p", "vref2p" };
+	const char *vrefn_regs[] = { "vref0n", "vref1n", "vref2n" };
+	int ret;
+	int i;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi_dev = spi;
+	init_completion(&st->completion);
+	mutex_init(&st->lock);
+
+	indio_dev->name = "max11410";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &max11410_info;
+
+	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "regmap initialization failed\n");
+
+	st->avdd = devm_regulator_get(dev, "avdd");
+	if (IS_ERR(st->avdd))
+		return dev_err_probe(dev, PTR_ERR(st->avdd),
+				     "avdd-supply is required.\n");
+
+	ret = regulator_enable(st->avdd);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, max11410_disable_reg, st->avdd);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(vrefp_regs); i++) {
+		ret = max11410_init_vref(dev, &st->vrefp[i], vrefp_regs[i]);
+		if (ret)
+			return ret;
+
+		ret = max11410_init_vref(dev, &st->vrefn[i], vrefn_regs[i]);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Regulators must be configured before parsing channels for
+	 * validating "adi,reference" property of each channel.
+	 */
+	ret = max11410_parse_channels(st, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(st->regmap, MAX11410_REG_CTRL, MAX11410_CTRL_FORMAT_BIT);
+	if (ret)
+		return ret;
+
+	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+					  indio_dev->name,
+					  iio_device_id(indio_dev));
+	if (!st->trig)
+		return -ENOMEM;
+
+	st->trig->ops = &max11410_trigger_ops;
+	ret = devm_iio_trigger_register(dev, st->trig);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(st->trig);
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      NULL,
+					      &max11410_trigger_handler,
+					      &max11410_buffer_ops);
+	if (ret)
+		return ret;
+
+	if (spi->irq > 0) {
+		ret = devm_request_threaded_irq(dev, spi->irq,
+						NULL,
+						&max11410_interrupt,
+						IRQF_ONESHOT,
+						"max11410", indio_dev);
+		if (ret)
+			return ret;
+
+		ret = max11410_write_reg(st, MAX11410_REG_GP1_CTRL, MAX11410_GP1_CTRL_INTRB);
+		if (ret)
+			return ret;
+	}
+
+	ret = max11410_self_calibrate(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot perform device self calibration\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id max11410_spi_of_id[] = {
+	{ .compatible = "adi,max11410" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max11410_spi_of_id);
+
+static const struct spi_device_id max11410_id[] = {
+	{ "max11410" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, max11410_id);
+
+static struct spi_driver max11410_driver = {
+	.driver = {
+		.name	= "max11410",
+		.of_match_table = max11410_spi_of_id,
+	},
+	.probe		= max11410_probe,
+	.id_table	= max11410_id,
+};
+module_spi_driver(max11410_driver);
+
+MODULE_AUTHOR("David Jung <david.jung@analog.com>");
+MODULE_AUTHOR("Ibrahim Tilki <ibrahim.tilki@analog.com>");
+MODULE_DESCRIPTION("Analog Devices MAX11410 ADC");
+MODULE_LICENSE("GPL v2");
-- 
2.36.1

Re: [PATCH v3 1/2] iio: adc: add max11410 adc driver

[Jonathan Cameron]

On Thu, 11 Aug 2022 13:42:42 +0000
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> Adding support for max11410 24-bit, 1.9ksps delta-sigma adc which
> has 3 differential reference and 10 differential channel inputs.
> Inputs and references can be buffered internally. Inputs can also
> be amplified with internal PGA.
> 
> Device has four digital filter modes: FIR50/60, FIR50, FIR60 and SINC4.
> FIR 50Hz and 60Hz rejections can be enabled/disabled separately.
> Digital filter selection affects sampling frequency range so driver
> has to consider the configured filter when configuring sampling frequency.
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>

Hi

Line length is a bit random currently.  Rough set of rules I'd go by:
1) If it doesn't hurt readability stay under 80 chars, but don't overwrap.
2) If there is a string someone might grep for, long lines are fine.
3) Try to keep under 100 chars in any case. (you are fine on this)

unfortunately I got confused and gave you some misleading feedback on v2.
Short version is: ADC drivers do not present HARDWAREGAIN as it has no
relevant meaning for them.  They present only SCALE (and maybe CALIBSCALE
if there are tuning values to be tweaked).  The driver is then expected
to work out the right settings of PGAs etc to achieve the requested scale
value.


> diff --git a/drivers/iio/adc/max11410.c b/drivers/iio/adc/max11410.c
> new file mode 100644
> index 000000000..383cef3a0
> --- /dev/null
> +++ b/drivers/iio/adc/max11410.c
> @@ -0,0 +1,954 @@
> +// SPDX-License-Identifier: GPL-2.0-only

No copyright notice?  Unusual for such a big piece of work.

> +
> +static const u8 max11410_sampling_len[] = {
> +	[MAX11410_FILTER_FIR5060] = 5,
> +	[MAX11410_FILTER_FIR50] = 6,
> +	[MAX11410_FILTER_FIR60] = 6,
> +	[MAX11410_FILTER_SINC4] = 10,
> +};
> +
> +static const int max11410_sampling_rates[4][10][2] = {
> +	[MAX11410_FILTER_FIR5060] = {
> +		{1, 100000},
		{ 1, 100000 }, throughout to match most common kernel style.
We don't always bother fixing this in old code, because it creates
a lot of noise, but good to get it right in new code.

> +		{2, 100000},
> +		{4, 200000},
> +		{8, 400000},
> +		{16, 800000}
> +	},
> +	[MAX11410_FILTER_FIR50] = {
> +		{1, 300000},
> +		{2, 700000},
> +		{5, 300000},
> +		{10, 700000},
> +		{21, 300000},
> +		{40}
> +	},
> +	[MAX11410_FILTER_FIR60] = {
> +		{1, 300000},
> +		{2, 700000},
> +		{5, 300000},
> +		{10, 700000},
> +		{21, 300000},
> +		{40}
> +	},
> +	[MAX11410_FILTER_SINC4] = {
> +		{4},
> +		{10},
> +		{20},
> +		{40},
> +		{60},
> +		{120},
> +		{240},
> +		{480},
> +		{960},
> +		{1920}
> +	}
> +};




> +static ssize_t max11410_notch_en_store(struct device *dev,
> +				       struct device_attribute *devattr,
> +				       const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct max11410_state *state = iio_priv(indio_dev);
> +	struct iio_dev_attr *iio_attr = to_iio_dev_attr(devattr);
> +	unsigned int filter_bit;
> +	int ret;
> +
> +	filter_bit = iio_attr->address == 0 ? MAX11410_FILTER_50HZ : MAX11410_FILTER_60HZ;
> +	if (sysfs_streq(buf, "0"))

Tend to use kstrtobool() for cases like this. It's a little more generous in what it
accepts. Documentation can carry on saying 0 or 1 though and we print 0 or 1 as most
appropriate values for this attribute.

> +		ret = regmap_set_bits(state->regmap, MAX11410_REG_FILTER, filter_bit);
> +	else if (sysfs_streq(buf, "1"))
> +		ret = regmap_clear_bits(state->regmap, MAX11410_REG_FILTER, filter_bit);
> +	else
> +		return -EINVAL;
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static IIO_CONST_ATTR(in_voltage_filter0_notch_center, "50");
> +static IIO_CONST_ATTR(in_voltage_filter1_notch_center, "60");
> +
> +static IIO_DEVICE_ATTR(in_voltage_filter0_notch_en, 0644,
> +		       max11410_notch_en_show, max11410_notch_en_store, 0);
> +static IIO_DEVICE_ATTR(in_voltage_filter1_notch_en, 0644,
> +		       max11410_notch_en_show, max11410_notch_en_store, 1);
> +
> +static struct attribute *max11410_attributes[] = {
> +	&iio_const_attr_in_voltage_filter0_notch_center.dev_attr.attr,
> +	&iio_const_attr_in_voltage_filter1_notch_center.dev_attr.attr,
> +	&iio_dev_attr_in_voltage_filter0_notch_en.dev_attr.attr,
> +	&iio_dev_attr_in_voltage_filter1_notch_en.dev_attr.attr,

As mentioned in reply to cover letter this is all new, so please
provide ABI documentatio to cover it.
I'm not fully comfortable with the approach of getting sinc4 if nothing
else is configured but perhaps that makes sense. I'm a bit surprised that
the hardware doesn't allow both the sinc4 and the FIR filters as would
seem logical to get rid of Mains and sampling frequency associated noise.

We could take advantage of the fact that IIO ABI allows any file write
to effect any other (needed as we can't always describe complex dependencies
and hence have additional attrs
	in_voltage_filter2_notch_en (if you turn both the fir filters off you get this.
		if you turn this on, then turn off).
	in_voltage_filter2_notch_centre which matches the sampling frequency (I think!)

That might be clearer than implicit control of that filter?


> +	NULL
> +};
> +

> +static int max11410_configure_channel(struct max11410_state *st,
> +				      struct iio_chan_spec const *chan)
> +{
> +	int ret;
> +	unsigned int regval;
> +	struct max11410_channel_config cfg = st->channels[chan->address];
> +
> +	if (chan->differential)
> +		ret = max11410_set_input_mux(st, chan->channel, chan->channel2);
> +	else
> +		ret = max11410_set_input_mux(st, chan->channel, MAX11410_AINN_GND);
> +
> +	if (ret)
> +		return ret;
> +
> +	regval = FIELD_PREP(MAX11410_CTRL_VREFP_BUF_BIT, cfg.buffered_vrefp) |
> +		 FIELD_PREP(MAX11410_CTRL_VREFN_BUF_BIT, cfg.buffered_vrefn) |
> +		 FIELD_PREP(MAX11410_CTRL_REFSEL_MASK, cfg.refsel) |
> +		 FIELD_PREP(MAX11410_CTRL_UNIPOLAR_BIT, !cfg.bipolar);
> +	ret = regmap_update_bits(st->regmap, MAX11410_REG_CTRL,
> +				 MAX11410_CTRL_REFSEL_MASK | MAX11410_CTRL_VREFN_BUF_BIT |
> +				 MAX11410_CTRL_VREFN_BUF_BIT | MAX11410_CTRL_UNIPOLAR_BIT,
> +				 regval);
> +	if (ret)
> +		return ret;
> +
> +	regval = FIELD_PREP(MAX11410_PGA_SIG_PATH_MASK, cfg.sig_path) |
> +		 FIELD_PREP(MAX11410_PGA_GAIN_MASK, cfg.gain);
> +	ret = regmap_write(st->regmap, MAX11410_REG_PGA, regval);
> +	if (ret)
> +		return ret;
> +
> +	if (cfg.settling_time_us)
> +		fsleep(cfg.settling_time_us);
> +
> +	return ret;

return 0;  Can't get here with any other value, so good to make that explicit.

> +}
> +
> +static int max11410_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long info)
> +{
> +	int ret, reg_val, filter, rate;
> +	struct regulator *vrefp, *vrefn;
> +	struct max11410_state *state = iio_priv(indio_dev);
> +	struct max11410_channel_config cfg = state->channels[chan->address];
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		vrefp = max11410_get_vrefp(state, cfg.refsel);
> +		if (!vrefp) {
> +			/* We should never get here */
> +			return -ENODEV;
> +		}
> +
> +		*val = regulator_get_voltage(vrefp) / 1000;
> +		vrefn = max11410_get_vrefn(state, cfg.refsel);
> +		if (vrefn)
> +			*val -= regulator_get_voltage(vrefn) / 1000;
> +
> +		if (cfg.bipolar)
> +			*val *= 2;
> +
> +		*val = *val >> cfg.gain;
> +
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (cfg.bipolar)
> +			*val = -BIT(chan->scan_type.realbits - 1);
> +		else
> +			*val = 0;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&state->lock);
> +
> +		ret = max11410_configure_channel(state, chan);
> +
> +		if (!ret)
> +			ret = max11410_sample(state, &reg_val);

Perhaps pull out these few lines of code as a helper so as to avoid
the nasty if (!ret) handling.
max11410_read_channel() or something like that.


> +
> +		mutex_unlock(&state->lock);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_val;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		*val = BIT(cfg.gain);

See below. This shouldn't exist for an ADC driver.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = regmap_read(state->regmap, MAX11410_REG_FILTER, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		filter = FIELD_GET(MAX11410_FILTER_LINEF_MASK, reg_val);
> +		rate = reg_val & MAX11410_FILTER_RATE_MASK;
> +		if (rate >= max11410_sampling_len[filter])
> +			rate = max11410_sampling_len[filter] - 1;
> +
> +		*val = max11410_sampling_rates[filter][rate][0];
> +		*val2 = max11410_sampling_rates[filter][rate][1];
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;
> +}
> +
> +static const int max11410_hwgain_list[] = {1, 2, 4, 8, 16, 32, 64, 128};
> +
> +static int max11410_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	int i, ret, reg_val, filter;
> +	struct max11410_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:

Ah. I gave a misleading response to discussion on previous patch as I misunderstood
your question. Sorry about that!  Just checked back on the v2 thread because I
was surprised to see this here.

We shouldn't be using hardwaregain at all in this driver.  Instead
we should be computing the available _scale values for the various settings
of hardware gain and making _scale controllable.

Hardware gain is intended for usecases where the gain is not directly
on the channel value being measured (e.g. on the ADC used to measure a pulse
on a time of flight sensor - the scale is on the voltage, not distance which
is derived from the timing of those pulses). Userspace software as a result
doesn't understand the use of hardware gain you have here.  It does tend to
understand using writable _SCALE.

It is more complex to write the code for scale, but it is an easier interface
for userspace to use as it needs to change one value, not change something
seemingly unrelated then notice the change in _scale which is what it cares
about.  You will need to provide an available_scale and that will require
runtime computation based on the reference voltage.  Sometimes it's better
to just precompute that in probe() rather than doing it dynamically later.

> +		for (i = 0; i < ARRAY_SIZE(max11410_hwgain_list); ++i) {
> +			if (val == max11410_hwgain_list[i])
> +				break;
> +		}
> +		if (i == ARRAY_SIZE(max11410_hwgain_list))
> +			return -EINVAL;
> +
> +		st->channels[chan->address].gain = i;
> +
> +		return 0;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&st->lock);
> +
> +		ret = regmap_read(st->regmap, MAX11410_REG_FILTER, &reg_val);
> +		if (ret)
> +			goto out;
> +
> +		filter = FIELD_GET(MAX11410_FILTER_LINEF_MASK, reg_val);
> +
> +		for (i = 0; i < max11410_sampling_len[filter]; ++i) {
> +			if (val == max11410_sampling_rates[filter][i][0] &&
> +			    val2 == max11410_sampling_rates[filter][i][1])
> +				break;
> +		}
> +		if (i == max11410_sampling_len[filter]) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_write_bits(st->regmap, MAX11410_REG_FILTER,
> +					MAX11410_FILTER_RATE_MASK, i);
> +		iio_device_release_direct_mode(indio_dev);
> +
> +out:
> +		mutex_unlock(&st->lock);
> +
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>


> +
> +static int max11410_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	int scan_ch, ret;
> +	struct max11410_state *st = iio_priv(indio_dev);
> +
> +	/* Configure GPIO1 of MAX11410 as interrupt output. */
I'd missed this before, but it's come up for some other drivers in the meantime..

This will follow through to the binding, but it's very hard to retrofit support for
different interrupt lines being wired.  Much easier to add that now.
Alternative is having the binding be flexible but check and reject interrupt
wirings the driver doesn't know about yet.

> +	ret = max11410_write_reg(st, MAX11410_REG_GP1_CTRL, MAX11410_GP1_CTRL_INTRB);
> +	if (ret)
> +		return ret;
> +
> +	scan_ch = ffs(*indio_dev->active_scan_mask) - 1;
> +
> +	ret = max11410_configure_channel(st, &indio_dev->channels[scan_ch]);
> +	if (ret)
> +		return ret;
> +
> +	/* Start continuous conversion. */
> +	return max11410_write_reg(st, MAX11410_REG_CONV_START, MAX11410_CONV_TYPE_CONTINUOUS);
> +}
> +

> +static int max11410_parse_channels(struct max11410_state *st,
> +				   struct iio_dev *indio_dev)
> +{
> +	struct iio_chan_spec *channels;
> +	struct fwnode_handle *child;
> +	struct max11410_channel_config *cfg;
> +	struct device *dev = &st->spi_dev->dev;
> +	const char *node_name;
> +	unsigned int num_ch;
> +	u32 inputs[2];
> +	u32 reference, sig_path;
> +	int chan_idx = 0;
> +	int ret;
> +	struct iio_chan_spec chanspec = chanspec_template;

Where it doesn't hurt grouping for readability, slight preference for reverse
xmas tree.

> +
> +	num_ch = device_get_child_node_count(dev);
> +	if (num_ch == 0)
> +		return dev_err_probe(&indio_dev->dev,
> +				     -ENODEV,
> +				     "FW has no channels defined\n");
> +
> +	/* Reserve space for soft timestamp channel */
> +	num_ch++;
> +	channels = devm_kcalloc(dev, num_ch,
> +				sizeof(struct iio_chan_spec),
> +				GFP_KERNEL);

overwrapped.  That fits nicely on one fewer lines.

> +	if (!channels)
> +		return -ENOMEM;
> +
> +	st->channels = devm_kcalloc(dev, num_ch,
> +				    sizeof(struct max11410_channel_config),

sizeof(*st->channels) preferred for cases like this.

> +				    GFP_KERNEL);
> +	if (!st->channels)
> +		return -ENOMEM;
...

> +}

> +
> +static int max11410_init_vref(struct device *dev,
> +			      struct regulator **vref,
> +			      const char *id)
> +{
> +	int ret;
> +	struct regulator *reg;
> +
> +	reg = devm_regulator_get_optional(dev, id);

You need to separate handling of -ENODEV which indicates that a regulator is not specified
from other errors which you do not want to hid. e.g. -EPROBE_DEFER
which just indicates we need to wait for some other driver to load
and then retry this one.
 
> +	if (IS_ERR(reg)) {
> +		*vref = NULL;
> +		return 0;
> +	}
> +	ret = regulator_enable(reg);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable regulator %s\n", id);
> +
> +	*vref = reg;
> +	return devm_add_action_or_reset(dev, max11410_disable_reg, reg);
> +}
> +
>
> +
> +static int max11410_probe(struct spi_device *spi)
> +{
> +	struct max11410_state *st;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	const char *vrefp_regs[] = { "vref0p", "vref1p", "vref2p" };
> +	const char *vrefn_regs[] = { "vref0n", "vref1n", "vref2n" };
> +	int ret;
> +	int i;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi_dev = spi;
> +	init_completion(&st->completion);
> +	mutex_init(&st->lock);
> +
> +	indio_dev->name = "max11410";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &max11410_info;
> +
> +	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "regmap initialization failed\n");
> +
> +	st->avdd = devm_regulator_get(dev, "avdd");
> +	if (IS_ERR(st->avdd))
> +		return dev_err_probe(dev, PTR_ERR(st->avdd),
> +				     "avdd-supply is required.\n");
> +
> +	ret = regulator_enable(st->avdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, max11410_disable_reg, st->avdd);
FYI. It probably won't land before this but there is a series adding
devm_regulator_enable() under review that would replace this (and 100s more
instances in IIO).

> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(vrefp_regs); i++) {
> +		ret = max11410_init_vref(dev, &st->vrefp[i], vrefp_regs[i]);
> +		if (ret)
> +			return ret;
> +
> +		ret = max11410_init_vref(dev, &st->vrefn[i], vrefn_regs[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Regulators must be configured before parsing channels for
> +	 * validating "adi,reference" property of each channel.
> +	 */
> +	ret = max11410_parse_channels(st, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_set_bits(st->regmap, MAX11410_REG_CTRL, MAX11410_CTRL_FORMAT_BIT);
> +	if (ret)
> +		return ret;
> +
> +	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +					  indio_dev->name,
> +					  iio_device_id(indio_dev));
> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops = &max11410_trigger_ops;
> +	ret = devm_iio_trigger_register(dev, st->trig);

You register a trigger that might not have an irq to drive it?
That's unusual and I think pointless - move the trigger allocation down to under
the if (spi->irq > 0 ) block.


> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(st->trig);
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      NULL,

NULL, fits on previous line so move it up.
I'm reading backwards and got bored at this point, but check for other
similar cases.

> +					      &max11410_trigger_handler,
> +					      &max11410_buffer_ops);
> +	if (ret)
> +		return ret;
> +
> +	if (spi->irq > 0) {
> +		ret = devm_request_threaded_irq(dev, spi->irq,
> +						NULL,
> +						&max11410_interrupt,
> +						IRQF_ONESHOT,
> +						"max11410", indio_dev);
overwrapped. Perhaps keep the two callbacks on one line given that NULL
can go on either the line above or the one below where you have it.

		ret = devm_request_threaded_irq(dev, spi->irq,
						NULL, &max11410_interrupt,
						IRQF_ONESHOT,
						"max11410", indio_dev);

> +		if (ret)
> +			return ret;
> +
> +		ret = max11410_write_reg(st, MAX11410_REG_GP1_CTRL, MAX11410_GP1_CTRL_INTRB);
No loss of readability by adding line breaks as
> +		ret = max11410_write_reg(st, MAX11410_REG_GP1_CTRL,
					 MAX11410_GP1_CTRL_INTRB);


> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = max11410_self_calibrate(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "cannot perform device self calibration\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[PATCH v3 2/2] dt-bindings: iio: adc: add adi,max11410.yaml

[Ibrahim Tilki]

Adding devicetree binding documentation for max11410 adc.

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>
---
 .../bindings/iio/adc/adi,max11410.yaml        | 165 ++++++++++++++++++
 1 file changed, 165 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
new file mode 100644
index 000000000..a782bfcaf
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
@@ -0,0 +1,165 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,max11410.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX11410 ADC device driver
+
+maintainers:
+  - Ibrahim Tilki <ibrahim.tilki@analog.com>
+
+description: |
+  Bindings for the Analog Devices MAX11410 ADC device. Datasheet can be
+  found here:
+    https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,max11410
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  avdd-supply:
+    description: Necessarry avdd supply. Used as reference when no explicit reference supplied.
+
+  vref0p-supply:
+    description: vref0p supply can be used as reference for conversion.
+
+  vref1p-supply:
+    description: vref1p supply can be used as reference for conversion.
+
+  vref2p-supply:
+    description: vref2p supply can be used as reference for conversion.
+
+  vref0n-supply:
+    description: vref0n supply can be used as reference for conversion.
+
+  vref1n-supply:
+    description: vref1n supply can be used as reference for conversion.
+
+  vref2n-supply:
+    description: vref2n supply can be used as reference for conversion.
+
+  spi-max-frequency:
+    maximum: 8000000
+
+required:
+  - compatible
+  - reg
+  - avdd-supply
+
+patternProperties:
+  "^channel(@[0-9a-f]+)?$":
+    $ref: "adc.yaml"
+    type: object
+    description: Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: The channel number in single-ended mode.
+        minimum: 0
+        maximum: 10
+
+      adi,reference:
+        description: |
+          Select the reference source to use when converting on
+          the specific channel. Valid values are:
+          0: VREF0P/VREF0N
+          1: VREF1P/VREF1N
+          2: VREF2P/VREF2N
+          3: AVDD/AGND
+          4: VREF0P/AGND
+          5: VREF1P/AGND
+          6: VREF2P/AGND
+          If this field is left empty, AVDD/AGND is selected.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1, 2, 3, 4, 5, 6]
+        default: 3
+
+      adi,input-mode:
+        description: |
+          Select signal path of input channels. When PGA path is selected,
+          hardwaregain property is enabled for channel. Valid values are:
+          0: Buffered, low-power, unity-gain path (default)
+          1: Bypass path
+          2: PGA path
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1, 2]
+        default: 0
+
+      diff-channels: true
+
+      bipolar: true
+
+      settling-time-us: true
+
+      adi,buffered-vrefp:
+        description: Enable buffered mode for positive reference.
+        type: boolean
+
+      adi,buffered-vrefn:
+        description: Enable buffered mode for negative reference.
+        type: boolean
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      adc@0 {
+        compatible = "adi,max11410";
+        reg = <0>;
+        spi-max-frequency = <8000000>;
+        interrupts = <25 2>;
+        interrupt-parent = <&gpio>;
+
+        avdd-supply = <&adc_avdd>;
+
+        vref1p-supply = <&adc_vref1p>;
+        vref1n-supply = <&adc_vref1n>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@0 {
+          reg = <0>;
+        };
+
+        channel@1 {
+          reg = <1>;
+          diff-channels = <2 3>;
+          adi,reference = <1>;
+          bipolar;
+          settling-time-us = <100000>;
+        };
+
+        channel@2 {
+          reg = <2>;
+          diff-channels = <7 9>;
+          adi,reference = <5>;
+          adi,input-mode = <2>;
+          settling-time-us = <50000>;
+        };
+      };
+    };
-- 
2.36.1

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add adi,max11410.yaml

[Jonathan Cameron]

On Thu, 11 Aug 2022 13:42:43 +0000
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> Adding devicetree binding documentation for max11410 adc.
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>

As a general rule, prefer to see review on list.
Whilst Analog folk are usually good at doing reviews properly some other companies have been
known to add reviewed-by tags without doing a proper job of review.  Hence we prefer
to hear from the reviewer on the public list if possible, even if it's a quick
note to say what sort of review they have done (general correctness / check against
datasheet / detailed subsystem interaction review etc) as that reduces the focus others
may put on the same areas.

I regularly do this wrong in Huawei code btw as we still do more review before posting
than we perhaps should :)

A few comments inline. Biggest is the interrupts description needing to be more
general to avoid us having problems if we extend it in future to cover the other
possible pin.

> ---
>  .../bindings/iio/adc/adi,max11410.yaml        | 165 ++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> new file mode 100644
> index 000000000..a782bfcaf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,max11410.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX11410 ADC device driver
> +
> +maintainers:
> +  - Ibrahim Tilki <ibrahim.tilki@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices MAX11410 ADC device. Datasheet can be
> +  found here:
> +    https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max11410
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

There are at least two possible pins, so this binding needs to take into account which
one / ones are wired.  Hence you need interrupt-names and the driver
needs to route things appropriately or at very least give a clear 'I don't support
GPIO0 usage' error message.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  avdd-supply:
> +    description: Necessarry avdd supply. Used as reference when no explicit reference supplied.
> +
> +  vref0p-supply:
> +    description: vref0p supply can be used as reference for conversion.
> +
> +  vref1p-supply:
> +    description: vref1p supply can be used as reference for conversion.
> +
> +  vref2p-supply:
> +    description: vref2p supply can be used as reference for conversion.
> +
> +  vref0n-supply:
> +    description: vref0n supply can be used as reference for conversion.
> +
> +  vref1n-supply:
> +    description: vref1n supply can be used as reference for conversion.
> +
> +  vref2n-supply:
> +    description: vref2n supply can be used as reference for conversion.
> +
> +  spi-max-frequency:
> +    maximum: 8000000
> +
> +required:
> +  - compatible
> +  - reg
> +  - avdd-supply
> +
> +patternProperties:
> +  "^channel(@[0-9a-f]+)?$":
> +    $ref: "adc.yaml"
> +    type: object
> +    description: Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        description: The channel number in single-ended mode.
> +        minimum: 0
> +        maximum: 10

the @ address seems to allow more than 0 to 10. Perhaps need to bring those inline and
make them both hex?  Curious. What's the 11th channel if max isn't 9?

> +
> +      adi,reference:
> +        description: |
> +          Select the reference source to use when converting on
> +          the specific channel. Valid values are:
> +          0: VREF0P/VREF0N
> +          1: VREF1P/VREF1N
> +          2: VREF2P/VREF2N
> +          3: AVDD/AGND
> +          4: VREF0P/AGND
> +          5: VREF1P/AGND
> +          6: VREF2P/AGND
> +          If this field is left empty, AVDD/AGND is selected.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3, 4, 5, 6]
> +        default: 3
> +
> +      adi,input-mode:
> +        description: |
> +          Select signal path of input channels. When PGA path is selected,
> +          hardwaregain property is enabled for channel. Valid values are:

A binding should not mention details of what the linux driver is doing, so drop
that bit about hardwaregain.  Whilst bindings exist in the Linux tree they
are used by various other bits of software.

> +          0: Buffered, low-power, unity-gain path (default)
> +          1: Bypass path
> +          2: PGA path
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2]
> +        default: 0
> +
> +      diff-channels: true
> +
> +      bipolar: true
> +
> +      settling-time-us: true
> +
> +      adi,buffered-vrefp:
> +        description: Enable buffered mode for positive reference.
> +        type: boolean
> +
> +      adi,buffered-vrefn:
> +        description: Enable buffered mode for negative reference.
> +        type: boolean
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +        compatible = "adi,max11410";
> +        reg = <0>;
> +        spi-max-frequency = <8000000>;
> +        interrupts = <25 2>;
> +        interrupt-parent = <&gpio>;
> +
> +        avdd-supply = <&adc_avdd>;
> +
> +        vref1p-supply = <&adc_vref1p>;
> +        vref1n-supply = <&adc_vref1n>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        channel@0 {
> +          reg = <0>;
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <2 3>;
> +          adi,reference = <1>;
> +          bipolar;
> +          settling-time-us = <100000>;
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          diff-channels = <7 9>;
> +          adi,reference = <5>;
> +          adi,input-mode = <2>;
> +          settling-time-us = <50000>;
> +        };
> +      };
> +    };

RE: [PATCH v3 2/2] dt-bindings: iio: adc: add adi,max11410.yaml

[Bolucu, Nurettin]

Hello Jonathan,

We are using private GitHub repository and review our projects during Pull Request. We reviewed the MAX11410 Linux driver based on the general correctness, best practices, Linux coding style and IIO Driver implementation. The repository also contains test scripts that we can review the test coverage as well.


Best Regards,

Nurettin Bolucu
Senior MTS, Software - Istanbul Software Design Center
Eski Buyukdere Cad. Maslak No.1 6th Floor 34485 Sariyer Istanbul TR

Office      90 (212) 952-5168
Mobile     90 (506) 992-7486
Web         analog.com

        










-----Original Message-----
From: Jonathan Cameron <jic23@kernel.org> 
Sent: Sunday, August 14, 2022 18:56
To: Tilki, Ibrahim <Ibrahim.Tilki@analog.com>
Cc: linux-iio@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>; Bolucu, Nurettin <Nurettin.Bolucu@analog.com>
Subject: Re: [PATCH v3 2/2] dt-bindings: iio: adc: add adi,max11410.yaml

[External]

On Thu, 11 Aug 2022 13:42:43 +0000
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> Adding devicetree binding documentation for max11410 adc.
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>

As a general rule, prefer to see review on list.
Whilst Analog folk are usually good at doing reviews properly some other companies have been known to add reviewed-by tags without doing a proper job of review.  Hence we prefer to hear from the reviewer on the public list if possible, even if it's a quick note to say what sort of review they have done (general correctness / check against datasheet / detailed subsystem interaction review etc) as that reduces the focus others may put on the same areas.

I regularly do this wrong in Huawei code btw as we still do more review before posting than we perhaps should :)

A few comments inline. Biggest is the interrupts description needing to be more general to avoid us having problems if we extend it in future to cover the other possible pin.

> ---
>  .../bindings/iio/adc/adi,max11410.yaml        | 165 ++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml 
> b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> new file mode 100644
> index 000000000..a782bfcaf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2022 
> +Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: 
> +https://urldefense.com/v3/__http://devicetree.org/schemas/iio/adc/adi
> +,max11410.yaml*__;Iw!!A3Ni8CS0y2Y!7sok_RjOsQmX2sw37vgNw48j1VkwrSqKY0l
> +gDlIAlxYUTh6-mbuZqS6ysSEQcFSTmpRIinW_-cwEpcG7BSg$
> +$schema: 
> +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> +aml*__;Iw!!A3Ni8CS0y2Y!7sok_RjOsQmX2sw37vgNw48j1VkwrSqKY0lgDlIAlxYUTh
> +6-mbuZqS6ysSEQcFSTmpRIinW_-cwEqTILbNA$
> +
> +title: Analog Devices MAX11410 ADC device driver
> +
> +maintainers:
> +  - Ibrahim Tilki <ibrahim.tilki@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices MAX11410 ADC device. Datasheet can 
> +be
> +  found here:
> +    https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max11410
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

There are at least two possible pins, so this binding needs to take into account which one / ones are wired.  Hence you need interrupt-names and the driver needs to route things appropriately or at very least give a clear 'I don't support
GPIO0 usage' error message.

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  avdd-supply:
> +    description: Necessarry avdd supply. Used as reference when no explicit reference supplied.
> +
> +  vref0p-supply:
> +    description: vref0p supply can be used as reference for conversion.
> +
> +  vref1p-supply:
> +    description: vref1p supply can be used as reference for conversion.
> +
> +  vref2p-supply:
> +    description: vref2p supply can be used as reference for conversion.
> +
> +  vref0n-supply:
> +    description: vref0n supply can be used as reference for conversion.
> +
> +  vref1n-supply:
> +    description: vref1n supply can be used as reference for conversion.
> +
> +  vref2n-supply:
> +    description: vref2n supply can be used as reference for conversion.
> +
> +  spi-max-frequency:
> +    maximum: 8000000
> +
> +required:
> +  - compatible
> +  - reg
> +  - avdd-supply
> +
> +patternProperties:
> +  "^channel(@[0-9a-f]+)?$":
> +    $ref: "adc.yaml"
> +    type: object
> +    description: Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        description: The channel number in single-ended mode.
> +        minimum: 0
> +        maximum: 10

the @ address seems to allow more than 0 to 10. Perhaps need to bring those inline and make them both hex?  Curious. What's the 11th channel if max isn't 9?

> +
> +      adi,reference:
> +        description: |
> +          Select the reference source to use when converting on
> +          the specific channel. Valid values are:
> +          0: VREF0P/VREF0N
> +          1: VREF1P/VREF1N
> +          2: VREF2P/VREF2N
> +          3: AVDD/AGND
> +          4: VREF0P/AGND
> +          5: VREF1P/AGND
> +          6: VREF2P/AGND
> +          If this field is left empty, AVDD/AGND is selected.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3, 4, 5, 6]
> +        default: 3
> +
> +      adi,input-mode:
> +        description: |
> +          Select signal path of input channels. When PGA path is selected,
> +          hardwaregain property is enabled for channel. Valid values are:

A binding should not mention details of what the linux driver is doing, so drop that bit about hardwaregain.  Whilst bindings exist in the Linux tree they are used by various other bits of software.

> +          0: Buffered, low-power, unity-gain path (default)
> +          1: Bypass path
> +          2: PGA path
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2]
> +        default: 0
> +
> +      diff-channels: true
> +
> +      bipolar: true
> +
> +      settling-time-us: true
> +
> +      adi,buffered-vrefp:
> +        description: Enable buffered mode for positive reference.
> +        type: boolean
> +
> +      adi,buffered-vrefn:
> +        description: Enable buffered mode for negative reference.
> +        type: boolean
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +        compatible = "adi,max11410";
> +        reg = <0>;
> +        spi-max-frequency = <8000000>;
> +        interrupts = <25 2>;
> +        interrupt-parent = <&gpio>;
> +
> +        avdd-supply = <&adc_avdd>;
> +
> +        vref1p-supply = <&adc_vref1p>;
> +        vref1n-supply = <&adc_vref1n>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        channel@0 {
> +          reg = <0>;
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <2 3>;
> +          adi,reference = <1>;
> +          bipolar;
> +          settling-time-us = <100000>;
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          diff-channels = <7 9>;
> +          adi,reference = <5>;
> +          adi,input-mode = <2>;
> +          settling-time-us = <50000>;
> +        };
> +      };
> +    };

Re: [PATCH v3 2/2] dt-bindings: iio: adc: add adi,max11410.yaml

[Jonathan Cameron]

On Fri, 19 Aug 2022 06:32:42 +0000
"Bolucu, Nurettin" <Nurettin.Bolucu@analog.com> wrote:

> Hello Jonathan,
> 
> We are using private GitHub repository and review our projects during Pull Request. We reviewed the MAX11410 Linux driver based on the general correctness, best practices, Linux coding style and IIO Driver implementation. The repository also contains test scripts that we can review the test coverage as well.
> 
> 

While nice to know, that's not much use for the upstream community who
can't see what level of review etc is going on.

So probably better to drop the Reviewed-by tags from that process
when upstreaming.

Thanks,

Jonathan


> Best Regards,
> 
> Nurettin Bolucu
> Senior MTS, Software - Istanbul Software Design Center
> Eski Buyukdere Cad. Maslak No.1 6th Floor 34485 Sariyer Istanbul TR
> 
> Office_____ 90 (212) 952-5168
> Mobile____ 90 (506) 992-7486
> Web________ analog.com
> 
> _ ______
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org> 
> Sent: Sunday, August 14, 2022 18:56
> To: Tilki, Ibrahim <Ibrahim.Tilki@analog.com>
> Cc: linux-iio@vger.kernel.org; Sa, Nuno <Nuno.Sa@analog.com>; Bolucu, Nurettin <Nurettin.Bolucu@analog.com>
> Subject: Re: [PATCH v3 2/2] dt-bindings: iio: adc: add adi,max11410.yaml
> 
> [External]
> 
> On Thu, 11 Aug 2022 13:42:43 +0000
> Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:
> 
> > Adding devicetree binding documentation for max11410 adc.
> > 
> > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > Reviewed-by: Nurettin Bolucu <Nurettin.Bolucu@analog.com>  
> 
> As a general rule, prefer to see review on list.
> Whilst Analog folk are usually good at doing reviews properly some other companies have been known to add reviewed-by tags without doing a proper job of review.  Hence we prefer to hear from the reviewer on the public list if possible, even if it's a quick note to say what sort of review they have done (general correctness / check against datasheet / detailed subsystem interaction review etc) as that reduces the focus others may put on the same areas.
> 
> I regularly do this wrong in Huawei code btw as we still do more review before posting than we perhaps should :)
> 
> A few comments inline. Biggest is the interrupts description needing to be more general to avoid us having problems if we extend it in future to cover the other possible pin.
> 
> > ---
> >  .../bindings/iio/adc/adi,max11410.yaml        | 165 ++++++++++++++++++
> >  1 file changed, 165 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml 
> > b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > new file mode 100644
> > index 000000000..a782bfcaf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > @@ -0,0 +1,165 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2022 
> > +Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: 
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/iio/adc/adi
> > +,max11410.yaml*__;Iw!!A3Ni8CS0y2Y!7sok_RjOsQmX2sw37vgNw48j1VkwrSqKY0l
> > +gDlIAlxYUTh6-mbuZqS6ysSEQcFSTmpRIinW_-cwEpcG7BSg$
> > +$schema: 
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> > +aml*__;Iw!!A3Ni8CS0y2Y!7sok_RjOsQmX2sw37vgNw48j1VkwrSqKY0lgDlIAlxYUTh
> > +6-mbuZqS6ysSEQcFSTmpRIinW_-cwEqTILbNA$
> > +
> > +title: Analog Devices MAX11410 ADC device driver
> > +
> > +maintainers:
> > +  - Ibrahim Tilki <ibrahim.tilki@analog.com>
> > +
> > +description: |
> > +  Bindings for the Analog Devices MAX11410 ADC device. Datasheet can 
> > +be
> > +  found here:
> > +    https://datasheets.maximintegrated.com/en/ds/MAX11410.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max11410
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1  
> 
> There are at least two possible pins, so this binding needs to take into account which one / ones are wired.  Hence you need interrupt-names and the driver needs to route things appropriately or at very least give a clear 'I don't support
> GPIO0 usage' error message.
> 
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +  avdd-supply:
> > +    description: Necessarry avdd supply. Used as reference when no explicit reference supplied.
> > +
> > +  vref0p-supply:
> > +    description: vref0p supply can be used as reference for conversion.
> > +
> > +  vref1p-supply:
> > +    description: vref1p supply can be used as reference for conversion.
> > +
> > +  vref2p-supply:
> > +    description: vref2p supply can be used as reference for conversion.
> > +
> > +  vref0n-supply:
> > +    description: vref0n supply can be used as reference for conversion.
> > +
> > +  vref1n-supply:
> > +    description: vref1n supply can be used as reference for conversion.
> > +
> > +  vref2n-supply:
> > +    description: vref2n supply can be used as reference for conversion.
> > +
> > +  spi-max-frequency:
> > +    maximum: 8000000
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - avdd-supply
> > +
> > +patternProperties:
> > +  "^channel(@[0-9a-f]+)?$":
> > +    $ref: "adc.yaml"
> > +    type: object
> > +    description: Represents the external channels which are connected to the ADC.
> > +
> > +    properties:
> > +      reg:
> > +        description: The channel number in single-ended mode.
> > +        minimum: 0
> > +        maximum: 10  
> 
> the @ address seems to allow more than 0 to 10. Perhaps need to bring those inline and make them both hex?  Curious. What's the 11th channel if max isn't 9?
> 
> > +
> > +      adi,reference:
> > +        description: |
> > +          Select the reference source to use when converting on
> > +          the specific channel. Valid values are:
> > +          0: VREF0P/VREF0N
> > +          1: VREF1P/VREF1N
> > +          2: VREF2P/VREF2N
> > +          3: AVDD/AGND
> > +          4: VREF0P/AGND
> > +          5: VREF1P/AGND
> > +          6: VREF2P/AGND
> > +          If this field is left empty, AVDD/AGND is selected.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        enum: [0, 1, 2, 3, 4, 5, 6]
> > +        default: 3
> > +
> > +      adi,input-mode:
> > +        description: |
> > +          Select signal path of input channels. When PGA path is selected,
> > +          hardwaregain property is enabled for channel. Valid values are:  
> 
> A binding should not mention details of what the linux driver is doing, so drop that bit about hardwaregain.  Whilst bindings exist in the Linux tree they are used by various other bits of software.
> 
> > +          0: Buffered, low-power, unity-gain path (default)
> > +          1: Bypass path
> > +          2: PGA path
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        enum: [0, 1, 2]
> > +        default: 0
> > +
> > +      diff-channels: true
> > +
> > +      bipolar: true
> > +
> > +      settling-time-us: true
> > +
> > +      adi,buffered-vrefp:
> > +        description: Enable buffered mode for positive reference.
> > +        type: boolean
> > +
> > +      adi,buffered-vrefn:
> > +        description: Enable buffered mode for negative reference.
> > +        type: boolean
> > +
> > +    required:
> > +      - reg
> > +
> > +    additionalProperties: false
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    spi {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      adc@0 {
> > +        compatible = "adi,max11410";
> > +        reg = <0>;
> > +        spi-max-frequency = <8000000>;
> > +        interrupts = <25 2>;
> > +        interrupt-parent = <&gpio>;
> > +
> > +        avdd-supply = <&adc_avdd>;
> > +
> > +        vref1p-supply = <&adc_vref1p>;
> > +        vref1n-supply = <&adc_vref1n>;
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        channel@0 {
> > +          reg = <0>;
> > +        };
> > +
> > +        channel@1 {
> > +          reg = <1>;
> > +          diff-channels = <2 3>;
> > +          adi,reference = <1>;
> > +          bipolar;
> > +          settling-time-us = <100000>;
> > +        };
> > +
> > +        channel@2 {
> > +          reg = <2>;
> > +          diff-channels = <7 9>;
> > +          adi,reference = <5>;
> > +          adi,input-mode = <2>;
> > +          settling-time-us = <50000>;
> > +        };
> > +      };
> > +    };  
> 

Re: [PATCH v3 0/2] iio: adc: add max11410 adc driver

[Jonathan Cameron]

On Thu, 11 Aug 2022 13:42:41 +0000
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> I added filter enable attributes for 50Hz and 60Hz FIR filters.
> Disabling both will set filter to sinc4, allowing higher sampling rates.
> 
> Regards,
> Ibrahim
> 
> Note: No sign-off tag for David as he was unreachable when the initial patch was sent.
> 
> Changelog:
> since v3:
>   - prefix defines with MAX11410_
>   - group vref regulators
>   - use builtin iio_validate_scan_mask_onehot
>   - validate iio trigger
>   - move scan data into state struct
>   - require vrefn regulator in DT if used by any channel
>   - don't require irq for triggered buffer
>   - remove filter sysfs attr and ABI documentation
>   - add in_voltage_filter[0-1]_notch_{center,en} attrs

I'm not seeing a documentation patch for this new ABI.
It's subtly different from the existing entries in
Documentation/ABI/testing/sysfs-bus-iio and the _en part is
new in general for filters.

I'll take a look at the rest of the code, but Docs need to
be in place and reviewed before I can take the driver.

Thanks,

Jonathan



> 
> since v2:
>   - remove bit position shifting, use field_prep instead
>   - reduce the amount of reg writes in max11410_configure_channel
>   - add error checking in max11410_parse_channels
>   - remove some unneeded blank lines and minor style fixes
>   - remove scan data assignment in max11410_trigger_handler
> 
> 
> Ibrahim Tilki (2):
>   iio: adc: add max11410 adc driver
>   dt-bindings: iio: adc: add adi,max11410.yaml
> 
>  .../bindings/iio/adc/adi,max11410.yaml        | 165 +++
>  drivers/iio/adc/Kconfig                       |  13 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/max11410.c                    | 954 ++++++++++++++++++
>  4 files changed, 1133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
>  create mode 100644 drivers/iio/adc/max11410.c
>