All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] iio: adc: add max11410 adc driver
@ 2022-09-08 14:49 Ibrahim Tilki
  2022-09-08 14:49 ` [PATCH v4 1/3] " Ibrahim Tilki
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ibrahim Tilki @ 2022-09-08 14:49 UTC (permalink / raw)
  To: jic23; +Cc: Ibrahim Tilki, linux-iio, Nuno.Sa, Nurettin.Bolucu

Hi Jonathan,

I added another filter attribute for sinc4 filter. Enabling sinc4 disables
fir filters. Disabling sinc4 defaults back to both fir filters being enabled.
Sinc4 filter center matches the configured sampling frequency.

GPIO configuration now depends on the interrupt-names property. Leaving it blank
results in probe error when there is an interrupt specified.

Regards,
Ibrahim

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

Changelog:

since v4:
  - add in_voltage_filter2_notch_{center,en} attrs for sinc4 filter
  - add ABI documentation for filter sysfs
  - check interrupt-names property for configuring gpio of max11410
  - remove hardwaregain property
  - add scale_available property for channes using PGA
  - separate vref regulator error -ENODEV from other errors
  - don't register trigger if no irq specified
  - style fixes

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 (3):
  iio: adc: add max11410 adc driver
  dt-bindings: iio: adc: add adi,max11410.yaml
  Documentation: ABI: testing: add max11410 doc

 .../ABI/testing/sysfs-bus-iio-adc-max11410    |   13 +
 .../bindings/iio/adc/adi,max11410.yaml        |  174 +++
 drivers/iio/adc/Kconfig                       |   13 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/max11410.c                    | 1051 +++++++++++++++++
 5 files changed, 1252 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
 create mode 100644 drivers/iio/adc/max11410.c

-- 
2.36.1


^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v4 1/3] iio: adc: add max11410 adc driver
  2022-09-08 14:49 [PATCH v4 0/3] iio: adc: add max11410 adc driver Ibrahim Tilki
@ 2022-09-08 14:49 ` Ibrahim Tilki
  2022-09-18 15:17   ` Jonathan Cameron
  2022-09-08 14:49 ` [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml Ibrahim Tilki
  2022-09-08 14:49 ` [PATCH v4 3/3] Documentation: ABI: testing: add max11410 doc Ibrahim Tilki
  2 siblings, 1 reply; 9+ messages in thread
From: Ibrahim Tilki @ 2022-09-08 14:49 UTC (permalink / raw)
  To: jic23; +Cc: Ibrahim Tilki, linux-iio, Nuno.Sa, Nurettin.Bolucu

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>
---
 drivers/iio/adc/Kconfig    |   13 +
 drivers/iio/adc/Makefile   |    1 +
 drivers/iio/adc/max11410.c | 1051 ++++++++++++++++++++++++++++++++++++
 3 files changed, 1065 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..7c1d0e725
--- /dev/null
+++ b/drivers/iio/adc/max11410.c
@@ -0,0 +1,1051 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MAX11410 SPI ADC driver
+ *
+ * Copyright 2022 Analog Devices Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_irq.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_GPIO_CTRL(ch)		((ch) ? 0x05 : 0x04)
+#define		MAX11410_GPIO_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;
+	u32 *scale_avail;
+	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_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 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);
+	unsigned int val;
+	int ret;
+
+	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;
+	case 2:
+		val = FIELD_GET(MAX11410_FILTER_LINEF_MASK, val) == 3;
+		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_bits;
+	bool enable;
+	int ret;
+
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		return ret;
+
+	switch (iio_attr->address) {
+	case 0:
+		filter_bits = MAX11410_FILTER_50HZ;
+		break;
+	case 1:
+		filter_bits = MAX11410_FILTER_60HZ;
+		break;
+	case 2:
+	default:
+		filter_bits = MAX11410_FILTER_50HZ | MAX11410_FILTER_60HZ;
+		enable = !enable;
+		break;
+	}
+
+	if (enable)
+		ret = regmap_clear_bits(state->regmap, MAX11410_REG_FILTER,
+					filter_bits);
+	else
+		ret = regmap_set_bits(state->regmap, MAX11410_REG_FILTER,
+				      filter_bits);
+
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t in_voltage_filter2_notch_center_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);
+	int ret, reg, rate, filter;
+
+	ret = regmap_read(state->regmap, MAX11410_REG_FILTER, &reg);
+	if (ret)
+		return ret;
+
+	rate = FIELD_GET(MAX11410_FILTER_RATE_MASK, reg);
+	rate = clamp_val(rate, 0,
+			 max11410_sampling_len[MAX11410_FILTER_SINC4] - 1);
+	filter = max11410_sampling_rates[MAX11410_FILTER_SINC4][rate][0];
+
+	return sysfs_emit(buf, "%d\n", filter);
+}
+
+static IIO_CONST_ATTR(in_voltage_filter0_notch_center, "50");
+static IIO_CONST_ATTR(in_voltage_filter1_notch_center, "60");
+static IIO_DEVICE_ATTR_RO(in_voltage_filter2_notch_center, 2);
+
+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 IIO_DEVICE_ATTR(in_voltage_filter2_notch_en, 0644,
+		       max11410_notch_en_show, max11410_notch_en_store, 2);
+
+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_filter2_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,
+	&iio_dev_attr_in_voltage_filter2_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 0;
+}
+
+static int max11410_sample(struct max11410_state *st, int *sample_raw,
+			   struct iio_chan_spec const *chan)
+{
+	int val, ret;
+
+	ret = max11410_configure_channel(st, chan);
+	if (ret)
+		return 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 int max11410_get_scale(struct max11410_state *state,
+			      struct max11410_channel_config cfg)
+{
+	struct regulator *vrefp, *vrefn;
+	int scale;
+
+	vrefp = max11410_get_vrefp(state, cfg.refsel);
+
+	scale = regulator_get_voltage(vrefp) / 1000;
+	vrefn = max11410_get_vrefn(state, cfg.refsel);
+	if (vrefn)
+		scale -= regulator_get_voltage(vrefn) / 1000;
+
+	if (cfg.bipolar)
+		scale *= 2;
+
+	return scale >> cfg.gain;
+}
+
+static int max11410_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long info)
+{
+	struct max11410_state *state = iio_priv(indio_dev);
+	struct max11410_channel_config cfg = state->channels[chan->address];
+	int ret, reg_val, filter, rate;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		*val = max11410_get_scale(state, cfg);
+		*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_sample(state, &reg_val, chan);
+
+		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_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 int max11410_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct max11410_state *st = iio_priv(indio_dev);
+	int i, ret, reg_val, filter, gain;
+	u32 *scale_avail;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		scale_avail = st->channels[chan->address].scale_avail;
+		if (!scale_avail)
+			return -EOPNOTSUPP;
+
+		/* Accept values in range 0.000001 <= scale < 1.000000 */
+		if (val != 0 || val2 == 0)
+			return -EINVAL;
+
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		/* Convert from INT_PLUS_MICRO to FRACTIONAL_LOG2 */
+		val2 = val2 * DIV_ROUND_CLOSEST(BIT(24), 1000000);
+		val2 = DIV_ROUND_CLOSEST(scale_avail[0], val2);
+		gain = order_base_2(val2);
+
+		st->channels[chan->address].gain = clamp_val(gain, 0, 7);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		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)
+{
+	struct max11410_state *st = iio_priv(indio_dev);
+	struct max11410_channel_config cfg;
+	int ret, reg_val, filter;
+
+	switch (info) {
+	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;
+	case IIO_CHAN_INFO_SCALE:
+		cfg = st->channels[chan->address];
+
+		if (!cfg.scale_avail)
+			return -EINVAL;
+
+		*vals = cfg.scale_avail;
+		*length = 8 * 2;
+		*type = IIO_VAL_FRACTIONAL_LOG2;
+
+		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);
+
+	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 chanspec = chanspec_template;
+	struct device *dev = &st->spi_dev->dev;
+	struct max11410_channel_config *cfg;
+	struct iio_chan_spec *channels;
+	struct fwnode_handle *child;
+	u32 reference, sig_path;
+	const char *node_name;
+	u32 inputs[2], scale;
+	unsigned int num_ch;
+	int chan_idx = 0;
+	int ret, i;
+
+	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(*channels), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	st->channels = devm_kcalloc(dev, num_ch, sizeof(*st->channels),
+				    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);
+		}
+
+		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;
+		cfg->gain = 0;
+
+		/* Enable scale_available property if input mode is PGA */
+		if (sig_path == MAX11410_PGA_SIG_PATH_PGA) {
+			__set_bit(IIO_CHAN_INFO_SCALE,
+				  &chanspec.info_mask_separate_available);
+			cfg->scale_avail = devm_kcalloc(dev, 8 * 2,
+							sizeof(*cfg->scale_avail),
+							GFP_KERNEL);
+			if (!cfg->scale_avail) {
+				fwnode_handle_put(child);
+				return -ENOMEM;
+			}
+
+			scale = max11410_get_scale(st, *cfg);
+			for (i = 0; i < 8; i++) {
+				cfg->scale_avail[2 * i] = scale >> i;
+				cfg->scale_avail[2 * i + 1] = chanspec.scan_type.realbits;
+			}
+		} else {
+			__clear_bit(IIO_CHAN_INFO_SCALE,
+				    &chanspec.info_mask_separate_available);
+		}
+
+		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 (PTR_ERR(reg) == -ENODEV) {
+		*vref = NULL;
+		return 0;
+	} else if (IS_ERR(reg)) {
+		return PTR_ERR(reg);
+	}
+	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 < 8; ++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;
+
+	if (of_irq_get_byname(dev->of_node, "gpio0") > 0)
+		ret = regmap_write(st->regmap, MAX11410_REG_GPIO_CTRL(0),
+				   MAX11410_GPIO_INTRB);
+	else if (of_irq_get_byname(dev->of_node, "gpio1") > 0)
+		ret = regmap_write(st->regmap, MAX11410_REG_GPIO_CTRL(1),
+				   MAX11410_GPIO_INTRB);
+	else if (spi->irq > 0)
+		return dev_err_probe(dev, -ENODEV,
+				     "no interrupt name specified");
+
+	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;
+	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_iio_trigger_register(dev, st->trig);
+		if (ret)
+			return ret;
+
+		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
+						&max11410_interrupt,
+						IRQF_ONESHOT,
+						"max11410", indio_dev);
+		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


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml
  2022-09-08 14:49 [PATCH v4 0/3] iio: adc: add max11410 adc driver Ibrahim Tilki
  2022-09-08 14:49 ` [PATCH v4 1/3] " Ibrahim Tilki
@ 2022-09-08 14:49 ` Ibrahim Tilki
  2022-09-18 15:22   ` Jonathan Cameron
  2022-09-08 14:49 ` [PATCH v4 3/3] Documentation: ABI: testing: add max11410 doc Ibrahim Tilki
  2 siblings, 1 reply; 9+ messages in thread
From: Ibrahim Tilki @ 2022-09-08 14:49 UTC (permalink / raw)
  To: jic23; +Cc: Ibrahim Tilki, linux-iio, Nuno.Sa, Nurettin.Bolucu

Adding devicetree binding documentation for max11410 adc.

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
---
 .../bindings/iio/adc/adi,max11410.yaml        | 174 ++++++++++++++++++
 1 file changed, 174 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..3ffab284b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
@@ -0,0 +1,174 @@
+# 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
+
+  interrupt-names:
+    description: Name of the gpio pin of max11410 used for IRQ
+    maxItems: 1
+    items:
+      enum:
+        - gpio0
+        - gpio1
+
+  '#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: 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. 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 {
+        reg = <0>;
+        compatible = "adi,max11410";
+        spi-max-frequency = <8000000>;
+
+        interrupt-parent = <&gpio>;
+        interrupts = <25 2>;
+        interrupt-names = "gpio1";
+
+        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


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v4 3/3] Documentation: ABI: testing: add max11410 doc
  2022-09-08 14:49 [PATCH v4 0/3] iio: adc: add max11410 adc driver Ibrahim Tilki
  2022-09-08 14:49 ` [PATCH v4 1/3] " Ibrahim Tilki
  2022-09-08 14:49 ` [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml Ibrahim Tilki
@ 2022-09-08 14:49 ` Ibrahim Tilki
  2022-09-18 15:26   ` Jonathan Cameron
  2 siblings, 1 reply; 9+ messages in thread
From: Ibrahim Tilki @ 2022-09-08 14:49 UTC (permalink / raw)
  To: jic23; +Cc: Ibrahim Tilki, linux-iio, Nuno.Sa, Nurettin.Bolucu

Adding documentation for Analog Devices max11410 adc userspace sysfs.

Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-max11410          | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max11410

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410 b/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
new file mode 100644
index 000000000..2a53c6b37
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
@@ -0,0 +1,13 @@
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_filterY_notch_en
+Date:		September 2022
+KernelVersion:  6.0
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Enable or disable a notch filter.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_filterY_notch_center
+Date:		September 2022
+KernelVersion:  6.0
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Center frequency of the notch filter in Hz.
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/3] iio: adc: add max11410 adc driver
  2022-09-08 14:49 ` [PATCH v4 1/3] " Ibrahim Tilki
@ 2022-09-18 15:17   ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2022-09-18 15:17 UTC (permalink / raw)
  To: Ibrahim Tilki; +Cc: linux-iio, Nuno.Sa, Nurettin.Bolucu

On Thu, 8 Sep 2022 17:49:22 +0300
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>

Hi Ibrahim,

Sorry for slow reply - busy week at Linux Plumbers.

Anyhow, a few new things introduced by v4 and looks like I missed
that you have a deadlock.

See inline,

Jonathan


> diff --git a/drivers/iio/adc/max11410.c b/drivers/iio/adc/max11410.c
> new file mode 100644
> index 000000000..7c1d0e725
> --- /dev/null
> +++ b/drivers/iio/adc/max11410.c
> @@ -0,0 +1,1051 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MAX11410 SPI ADC driver
> + *
> + * Copyright 2022 Analog Devices Inc.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>

See below, but an of specific include is a bad sign in a modern driver.
There isn't anything in here that doesn't have support in property.h now
to enable various types of firmware (potentially at least!)

> +#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>
> +

> +
> +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);

int is not sized which seems unwise. Given this is about alignment of
data in the buffer you need to be careful, so make it a u32.


> +		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_SAMP_FREQ),
> +	.scan_type = {
> +		.sign = 's',
> +		.realbits = 24,
> +		.storagebits = 24,

I guess I missed this before, but storagebits is a power of 2 to allow
for efficient non packed accessing.  So 32 here.  You might see odd results
if you were to run in buffered mode with the timestamp disabled for example.
Userspace code tends to correctly assume power of 2 storage bits elements in the scan.

> +		.endianness = IIO_LE,
> +	},
> +};
> +

> +
> +static int max11410_sample(struct max11410_state *st, int *sample_raw,
> +			   struct iio_chan_spec const *chan)
> +{
> +	int val, ret;
> +
> +	ret = max11410_configure_channel(st, chan);
> +	if (ret)
> +		return 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) {

All this irq stuff should be via the irq that you get from
fwnode_irq_by_name(), not the one cached in the spi_dev.
Obviously it doesn't matter that much as you are just using it
as a flag here, but it's better to use the right source for that flag.

> +		/* 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 int max11410_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long info)
> +{
> +	struct max11410_state *state = iio_priv(indio_dev);
> +	struct max11410_channel_config cfg = state->channels[chan->address];
> +	int ret, reg_val, filter, rate;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = max11410_get_scale(state, cfg);
> +		*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_sample(state, &reg_val, chan);
> +
> +		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_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 int max11410_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct max11410_state *st = iio_priv(indio_dev);
> +	int i, ret, reg_val, filter, gain;
> +	u32 *scale_avail;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		scale_avail = st->channels[chan->address].scale_avail;
> +		if (!scale_avail)
> +			return -EOPNOTSUPP;
> +
> +		/* Accept values in range 0.000001 <= scale < 1.000000 */
> +		if (val != 0 || val2 == 0)
> +			return -EINVAL;
> +
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		/* Convert from INT_PLUS_MICRO to FRACTIONAL_LOG2 */
> +		val2 = val2 * DIV_ROUND_CLOSEST(BIT(24), 1000000);
> +		val2 = DIV_ROUND_CLOSEST(scale_avail[0], val2);
> +		gain = order_base_2(val2);
> +
> +		st->channels[chan->address].gain = clamp_val(gain, 0, 7);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		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);

Hmm. This could be problematic.  Normally we claim direct_mode (which takes
iio_dev->mlock) before we grab a driver specific lock.  
You've done that in some paths here, and in others you have reversed
the ordering. As such I think we have a deadlock (vs read_raw, _RAW path
above for example)

I may have missed other instances, so please look at that carefully.

 
> +
> +		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)
> +{
> +	struct max11410_state *st = iio_priv(indio_dev);
> +	struct max11410_channel_config cfg;
> +	int ret, reg_val, filter;
> +
> +	switch (info) {
> +	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;
> +	case IIO_CHAN_INFO_SCALE:
> +		cfg = st->channels[chan->address];
> +
> +		if (!cfg.scale_avail)
> +			return -EINVAL;
> +
> +		*vals = cfg.scale_avail;
> +		*length = 8 * 2;

Better to add a define for that 8 and then use that to enforce this list
is always of that size at point of allocation and access.

> +		*type = IIO_VAL_FRACTIONAL_LOG2;
> +
> +		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 int max11410_init_vref(struct device *dev,
> +			      struct regulator **vref,
> +			      const char *id)
> +{
> +	int ret;
> +	struct regulator *reg;
In a lot of cases you've gone with reverse xmas tree ordering,
which is good because it makes the ordering of declarations
consistent.  Here, you haven't. I'd prefer you did that everywhere.

> +
> +	reg = devm_regulator_get_optional(dev, id);
> +	if (PTR_ERR(reg) == -ENODEV) {
> +		*vref = NULL;
> +		return 0;
> +	} else if (IS_ERR(reg)) {
> +		return PTR_ERR(reg);
> +	}
> +	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);
> +	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;
> +
> +	if (of_irq_get_byname(dev->of_node, "gpio0") > 0)

Please use the generic firmware accessors not the of specific ones as that
restricts the firmware that can be used to instantiate this device.
Here that means using fwnode_irq_get_by_name(dev_fwnode(dev), ...0) 

We should also be using the irq returned by the call, not spi->irq.
Otherwise we may get a different IRQ if multiple are specified.

> +		ret = regmap_write(st->regmap, MAX11410_REG_GPIO_CTRL(0),
> +				   MAX11410_GPIO_INTRB);
> +	else if (of_irq_get_byname(dev->of_node, "gpio1") > 0)
> +		ret = regmap_write(st->regmap, MAX11410_REG_GPIO_CTRL(1),
> +				   MAX11410_GPIO_INTRB);
> +	else if (spi->irq > 0)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "no interrupt name specified");
> +
> +	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));

You seems to be allocating the trigger whether or not the irq is provided?
There is no reason to do that - just move this block down to alongside
the bit where we register the trigger so it's protected by the if (irq).

> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops = &max11410_trigger_ops;
> +	indio_dev->trig = iio_trigger_get(st->trig);

I'm not particularly keen on default triggers as they create a rather odd requirement
on removing the driver that you have to set current_trigger to "" in order to get
the reference count to drop.  If the device works fine with other triggers,
selecting it's own one is just a policy choice and should be left to userspace.

Also, the trigger is optional - so if you use it as the default, make sure
the trigger is present.  Finally, don't take a reference to the trigger until
after you have registered the trigger.

> +
> +	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_iio_trigger_register(dev, st->trig);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
> +						&max11410_interrupt,
> +						IRQF_ONESHOT,
> +						"max11410", indio_dev);
> +		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);
> +}
> +


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml
  2022-09-08 14:49 ` [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml Ibrahim Tilki
@ 2022-09-18 15:22   ` Jonathan Cameron
  2022-09-20 15:27     ` Tilki, Ibrahim
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2022-09-18 15:22 UTC (permalink / raw)
  To: Ibrahim Tilki; +Cc: linux-iio, Nuno.Sa, Nurettin.Bolucu

On Thu, 8 Sep 2022 17:49:23 +0300
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> Adding devicetree binding documentation for max11410 adc.
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> ---
>  .../bindings/iio/adc/adi,max11410.yaml        | 174 ++++++++++++++++++
>  1 file changed, 174 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..3ffab284b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> @@ -0,0 +1,174 @@
> +# 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

Why?  If someone wants to wire both lines, they can do so.
Then it will be up to the driver to pick which one to use.
Given we have names, we will know which is which and be able
to pick our favourite line to use.

> +
> +  interrupt-names:
> +    description: Name of the gpio pin of max11410 used for IRQ
> +    maxItems: 1
> +    items:
> +      enum:
> +        - gpio0
> +        - gpio1
> +
> +  '#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
hmm.

If explicit references are supplied and used, then will we query the voltage
of the avdd supply?  If not, it doesn't need to be supplied. Power is needed
but it might be coming from a fixed regulator no one bothered to put
in the device tree.  Perhaps we just don't care about that corner case?

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

name isn't that flexible as we only allow reg 0-9

> +    $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: 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. 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

This now needs to use the new spi-peripheral-props.yaml 
https://lore.kernel.org/all/20220816124321.67817-1-krzysztof.kozlowski@linaro.org/

Your series crossed with that cleanup / binding documentation refactor.

> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      adc@0 {
> +        reg = <0>;
> +        compatible = "adi,max11410";
> +        spi-max-frequency = <8000000>;
> +
> +        interrupt-parent = <&gpio>;
> +        interrupts = <25 2>;
> +        interrupt-names = "gpio1";
> +
> +        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>;
> +        };
> +      };
> +    };


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v4 3/3] Documentation: ABI: testing: add max11410 doc
  2022-09-08 14:49 ` [PATCH v4 3/3] Documentation: ABI: testing: add max11410 doc Ibrahim Tilki
@ 2022-09-18 15:26   ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2022-09-18 15:26 UTC (permalink / raw)
  To: Ibrahim Tilki; +Cc: linux-iio, Nuno.Sa, Nurettin.Bolucu

On Thu, 8 Sep 2022 17:49:24 +0300
Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:

> Adding documentation for Analog Devices max11410 adc userspace sysfs.
> 
> Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>

I debated a bit with myself when reviewing the driver this time on whether
we wanted
filter_notchY_en or as you have it filterY_notch_en

It's a bit similar to channel naming in which we potentially allow
in_accel0_x_raw
in_accel1_x_raw
in_accel0_y_raw

or
in_accel0_x_raw
in_accel1_x_raw
in_accel2_y_raw

etc depending on whether the index is considered global across all modifiers
or not.  We decided long ago that either was fine as they are uniquely
identifiable either by channel index, or by index + modifier.

Anyhow, upshot is I'm fine with the ordering you have here - but if anyone
else has time to take a look and comment on why one option is better than
the other that would be great.

Thanks,

Jonathan
 

> ---
>  .../ABI/testing/sysfs-bus-iio-adc-max11410          | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410 b/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
> new file mode 100644
> index 000000000..2a53c6b37
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max11410
> @@ -0,0 +1,13 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_filterY_notch_en
> +Date:		September 2022
> +KernelVersion:  6.0
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Enable or disable a notch filter.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage_filterY_notch_center
> +Date:		September 2022
> +KernelVersion:  6.0
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Center frequency of the notch filter in Hz.


^ permalink raw reply	[flat|nested] 9+ messages in thread
* RE: [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml
  2022-09-18 15:22   ` Jonathan Cameron
@ 2022-09-20 15:27     ` Tilki, Ibrahim
  2022-09-24 13:58       ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Tilki, Ibrahim @ 2022-09-20 15:27 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio, Sa, Nuno, Bolucu, Nurettin


> On Thu, 8 Sep 2022 17:49:23 +0300
> Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:
> 
> > Adding devicetree binding documentation for max11410 adc.
> > 
> > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > ---
> >  .../bindings/iio/adc/adi,max11410.yaml        | 174 ++++++++++++++++++
> >  1 file changed, 174 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..3ffab284b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,max11410.yaml
> > @@ -0,0 +1,174 @@
> > +# 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!9LhA6Qy9HXqdtKQfrUEeHtQ7xhm_bIEYwWU
> > +8CyFFP9qvWNxJ5EE2jJ90UG_gph6M7EecEx_r5PuFIsbL$
> > +$schema: 
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> > +aml*__;Iw!!A3Ni8CS0y2Y!9LhA6Qy9HXqdtKQfrUEeHtQ7xhm_bIEYwWU8CyFFP9qvWN
> > +xJ5EE2jJ90UG_gph6M7EecEx_r5ClUIiuZ$
> > +
> > +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
> 
> Why?  If someone wants to wire both lines, they can do so.
> Then it will be up to the driver to pick which one to use.
> Given we have names, we will know which is which and be able to pick our favourite line to use.
> 

Done

> > +
> > +  interrupt-names:
> > +    description: Name of the gpio pin of max11410 used for IRQ
> > +    maxItems: 1
> > +    items:
> > +      enum:
> > +        - gpio0
> > +        - gpio1
> > +
> > +  '#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
> hmm.
> 
> If explicit references are supplied and used, then will we query the voltage of the avdd supply?  If not, it doesn't need to be supplied. Power is needed but it might be coming from a fixed regulator no one bothered to put in the device tree.  Perhaps we just don't care about that corner case?
> 

That is correct. If all of the channels use external vref, then avdd-supply
is no longer required. It is fine by me if we ignore this corner case.

Current driver checks for avdd-supply unconditionally. Maybe we can change the
driver so that it results in error only if one of the channels use avdd as reference
in the absence of avdd-supply?


> > +
> > +patternProperties:
> > +  "^channel(@[0-9a-f]+)?$":
> 
> name isn't that flexible as we only allow reg 0-9
> 

I changed it to "^channel(@[0-9])?$" in v5 but will this allow us to define
more than 10 channels? Sharing analog inputs accross multiple channels is
supported by driver. But I don't know how to correctly specify it here.
For example following configuration is valid:

  channel@0 {
    reg = <0>;
  };
  channel@1 {
    reg = <1>;
  };
  channel@2 {
    reg = <2>;
  };
  channel@3 {
    reg = <3>;
  };
  channel@4 {
    reg = <4>;
  };
  channel@5 {
    reg = <5>;
  };
  channel@6 {
    reg = <6>;
  };
  channel@7 {
    reg = <7>;
  };
  channel@8 {
    reg = <8>;
  };
  channel@9 {
    reg = <9>;
  };
  channel@a {
    diff-channels = <1 2>;
  };
  channel@b {
    diff-channels = <7 9>;
  };


> > +    $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: 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. 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
> 
> This now needs to use the new spi-peripheral-props.yaml
> https://lore.kernel.org/all/20220816124321.67817-1-krzysztof.kozlowski@linaro.org/ 
> 
> Your series crossed with that cleanup / binding documentation refactor.
> 

Updated in v5

> > +
> > +examples:
> > +  - |
> > +    spi {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      adc@0 {
> > +        reg = <0>;
> > +        compatible = "adi,max11410";
> > +        spi-max-frequency = <8000000>;
> > +
> > +        interrupt-parent = <&gpio>;
> > +        interrupts = <25 2>;
> > +        interrupt-names = "gpio1";
> > +
> > +        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>;
> > +        };
> > +      };
> > +    };


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml
  2022-09-20 15:27     ` Tilki, Ibrahim
@ 2022-09-24 13:58       ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2022-09-24 13:58 UTC (permalink / raw)
  To: Tilki, Ibrahim; +Cc: linux-iio, Sa, Nuno, Bolucu, Nurettin

On Tue, 20 Sep 2022 15:27:23 +0000
"Tilki, Ibrahim" <Ibrahim.Tilki@analog.com> wrote:

> > On Thu, 8 Sep 2022 17:49:23 +0300
> > Ibrahim Tilki <Ibrahim.Tilki@analog.com> wrote:
> >   
> > > Adding devicetree binding documentation for max11410 adc.
> > > 
> > > Signed-off-by: Ibrahim Tilki <Ibrahim.Tilki@analog.com>
> > > ---
> > >  .../bindings/iio/adc/adi,max11410.yaml        | 174 ++++++++++++++++++
> > >  1 file changed, 174 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
...

> > > +
> > > +  interrupt-names:
> > > +    description: Name of the gpio pin of max11410 used for IRQ
> > > +    maxItems: 1
> > > +    items:
> > > +      enum:
> > > +        - gpio0
> > > +        - gpio1
> > > +
> > > +  '#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  
> > hmm.
> > 
> > If explicit references are supplied and used, then will we query the voltage of the avdd supply?  If not, it doesn't need to be supplied. Power is needed but it might be coming from a fixed regulator no one bothered to put in the device tree.  Perhaps we just don't care about that corner case?
> >   
> 
> That is correct. If all of the channels use external vref, then avdd-supply
> is no longer required. It is fine by me if we ignore this corner case.
> 
> Current driver checks for avdd-supply unconditionally. Maybe we can change the
> driver so that it results in error only if one of the channels use avdd as reference
> in the absence of avdd-supply?

Does it query the voltage in this corner case?  I don't think it does.
If that's the case, you are fine getting it and if not supplied (and various
other conditions are met) the regulator core will provide a stub regulator
that will work fine here.  All we might want to do is remove the required
line from the binding.

> 
> 
> > > +
> > > +patternProperties:
> > > +  "^channel(@[0-9a-f]+)?$":  
> > 
> > name isn't that flexible as we only allow reg 0-9
> >   
> 
> I changed it to "^channel(@[0-9])?$" in v5 but will this allow us to define
> more than 10 channels? Sharing analog inputs accross multiple channels is
> supported by driver. But I don't know how to correctly specify it here.
> For example following configuration is valid:

Ah. I'd not understood this correctly.
The adc.yaml binding requires the @X number to match with reg.
It doesn't provide a way to not have an @reg.

Interesting corner case if you have overlap of single ended and differential
channels.  I guess we could specify reg as have no meaning for differential
channels other than as an index.  Not sure what the DT maintainers would think
of that though.  It isn't obvious what we should set reg to for differential
channels.

Ah. That meant I just checked if this had gone to the right people.
For all dt-bindings you need to +CC the list and maintainers listed
in MAINTAINERS.  They aren't going to see the binding otherwise and
I won't take a new binding without their review.

> 
>   channel@0 {
>     reg = <0>;
>   };
>   channel@1 {
>     reg = <1>;
>   };
>   channel@2 {
>     reg = <2>;
>   };
>   channel@3 {
>     reg = <3>;
>   };
>   channel@4 {
>     reg = <4>;
>   };
>   channel@5 {
>     reg = <5>;
>   };
>   channel@6 {
>     reg = <6>;
>   };
>   channel@7 {
>     reg = <7>;
>   };
>   channel@8 {
>     reg = <8>;
>   };
>   channel@9 {
>     reg = <9>;
>   };
>   channel@a {
>     diff-channels = <1 2>;
>   };
>   channel@b {
>     diff-channels = <7 9>;
>   };
> 

Thanks,

Joanthan


^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-24 13:58 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-08 14:49 [PATCH v4 0/3] iio: adc: add max11410 adc driver Ibrahim Tilki
2022-09-08 14:49 ` [PATCH v4 1/3] " Ibrahim Tilki
2022-09-18 15:17   ` Jonathan Cameron
2022-09-08 14:49 ` [PATCH v4 2/3] dt-bindings: iio: adc: add adi,max11410.yaml Ibrahim Tilki
2022-09-18 15:22   ` Jonathan Cameron
2022-09-20 15:27     ` Tilki, Ibrahim
2022-09-24 13:58       ` Jonathan Cameron
2022-09-08 14:49 ` [PATCH v4 3/3] Documentation: ABI: testing: add max11410 doc Ibrahim Tilki
2022-09-18 15:26   ` Jonathan Cameron

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