linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] Introduce max12xx ADC support
@ 2019-10-03 17:33 Miquel Raynal
  2019-10-03 17:33 ` [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support Miquel Raynal
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:33 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

Hello, here is a patchset updating the existing max1027.c driver (for
10-bit max1027/29/31 ADCs) with a few corrections/improvements and
then introducing their 12-bit cousins named max1227/29/31.

As on my hardware setup the "start conversion" and "end of conversion"
pin are not wired (which is absolutely fine for this chip), I also
updated the driver and the bindings to support optional interrupts. In
this case, triggered buffers are not available and the user must poll
the value from sysfs.

Thanks,
Miquèl


Changes in v2:
==============
* Removed the addition of three compatibles from patch 4 (the
  preparation patch) to add these lines back in patch 5 (the actual
  introduction).


Miquel Raynal (7):
  iio: adc: max1027: Add debugfs register read support
  iio: adc: max1027: Make it optional to use interrupts
  iio: adc: max1027: Reset the device at probe time
  iio: adc: max1027: Prepare the introduction of different resolutions
  iio: adc: max1027: Introduce 12-bit devices support
  dt-bindings: iio: adc: max1027: Mark interrupts as optional
  dt-bindings: iio: adc: max1027: Document max12xx series compatibles

 .../bindings/iio/adc/max1027-adc.txt          |  12 +-
 drivers/iio/adc/Kconfig                       |   4 +-
 drivers/iio/adc/max1027.c                     | 190 +++++++++++-------
 3 files changed, 133 insertions(+), 73 deletions(-)

-- 
2.20.1


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

* [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
@ 2019-10-03 17:33 ` Miquel Raynal
  2019-10-06 10:04   ` Jonathan Cameron
  2019-10-03 17:33 ` [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts Miquel Raynal
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:33 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

Until now, only write operations were supported. Force two bytes read
operation when reading from this register (might be wrong when reading
the temperature, but will work with any other value).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 214883458582..6cdfe9ef73fc 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -309,8 +309,11 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev,
 	struct max1027_state *st = iio_priv(indio_dev);
 	u8 *val = (u8 *)st->buffer;
 
-	if (readval != NULL)
-		return -EINVAL;
+	if (readval) {
+		int ret = spi_read(st->spi, val, 2);
+		*readval = be16_to_cpu(st->buffer[0]);
+		return ret;
+	}
 
 	*val = (u8)writeval;
 	return spi_write(st->spi, val, 1);
-- 
2.20.1


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

* [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
  2019-10-03 17:33 ` [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support Miquel Raynal
@ 2019-10-03 17:33 ` Miquel Raynal
  2019-10-06 10:18   ` Jonathan Cameron
  2019-10-03 17:33 ` [PATCH v2 3/7] iio: adc: max1027: Reset the device at probe time Miquel Raynal
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:33 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

The chip has a 'start conversion' and a 'end of conversion' pair of
pins. They can be used but this is absolutely not mandatory as regular
polling of the value is totally fine with the current internal
clocking setup. Turn the interrupts optional and do not error out if
they are not inquired in the device tree. This has the effect to
prevent triggered buffers use though.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 57 +++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 6cdfe9ef73fc..823223b77a70 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -430,35 +430,40 @@ static int max1027_probe(struct spi_device *spi)
 		return -ENOMEM;
 	}
 
-	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
-					&iio_pollfunc_store_time,
-					&max1027_trigger_handler, NULL);
-	if (ret < 0) {
-		dev_err(&indio_dev->dev, "Failed to setup buffer\n");
-		return ret;
-	}
+	if (spi->irq) {
+		ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+						      &iio_pollfunc_store_time,
+						      &max1027_trigger_handler,
+						      NULL);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
+			return ret;
+		}
 
-	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
-							indio_dev->name);
-	if (st->trig == NULL) {
-		ret = -ENOMEM;
-		dev_err(&indio_dev->dev, "Failed to allocate iio trigger\n");
-		return ret;
-	}
+		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
+						  indio_dev->name);
+		if (st->trig == NULL) {
+			ret = -ENOMEM;
+			dev_err(&indio_dev->dev,
+				"Failed to allocate iio trigger\n");
+			return ret;
+		}
 
-	st->trig->ops = &max1027_trigger_ops;
-	st->trig->dev.parent = &spi->dev;
-	iio_trigger_set_drvdata(st->trig, indio_dev);
-	iio_trigger_register(st->trig);
+		st->trig->ops = &max1027_trigger_ops;
+		st->trig->dev.parent = &spi->dev;
+		iio_trigger_set_drvdata(st->trig, indio_dev);
+		iio_trigger_register(st->trig);
 
-	ret = devm_request_threaded_irq(&spi->dev, spi->irq,
-					iio_trigger_generic_data_rdy_poll,
-					NULL,
-					IRQF_TRIGGER_FALLING,
-					spi->dev.driver->name, st->trig);
-	if (ret < 0) {
-		dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
-		return ret;
+		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
+						iio_trigger_generic_data_rdy_poll,
+						NULL,
+						IRQF_TRIGGER_FALLING,
+						spi->dev.driver->name,
+						st->trig);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
+			return ret;
+		}
 	}
 
 	/* Disable averaging */
-- 
2.20.1


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

* [PATCH v2 3/7] iio: adc: max1027: Reset the device at probe time
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
  2019-10-03 17:33 ` [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support Miquel Raynal
  2019-10-03 17:33 ` [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts Miquel Raynal
@ 2019-10-03 17:33 ` Miquel Raynal
  2019-10-03 17:33 ` [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions Miquel Raynal
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:33 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

All the registers are configured by the driver, let's reset the chip
at probe time, avoiding any conflict with a possible earlier
configuration.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 823223b77a70..f9b473ee6711 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -466,6 +466,14 @@ static int max1027_probe(struct spi_device *spi)
 		}
 	}
 
+	/* Internal reset */
+	st->reg = MAX1027_RST_REG;
+	ret = spi_write(st->spi, &st->reg, 1);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Failed to reset the ADC\n");
+		return ret;
+	}
+
 	/* Disable averaging */
 	st->reg = MAX1027_AVG_REG;
 	ret = spi_write(st->spi, &st->reg, 1);
-- 
2.20.1


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

* [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
                   ` (2 preceding siblings ...)
  2019-10-03 17:33 ` [PATCH v2 3/7] iio: adc: max1027: Reset the device at probe time Miquel Raynal
@ 2019-10-03 17:33 ` Miquel Raynal
  2019-10-06 10:22   ` Jonathan Cameron
  2019-10-03 17:33 ` [PATCH v2 5/7] iio: adc: max1027: Introduce 12-bit devices support Miquel Raynal
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:33 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

Maxim's max1027/29/31 series returns the measured voltages with a
resolution of 10 bits. There is a very similar series, max1227/29/31
which works identically but uses a resolution of 12 bits. Prepare the
support for these chips by turning the 'depth' into a macro parameter
instead of hardcoding it everywhere. Also reorganize just a bit the
macros at the top to avoid repeating tens of lines when adding support
for a new chip.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 78 ++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index f9b473ee6711..5d5d223dd42a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -83,7 +83,7 @@ static const struct of_device_id max1027_adc_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
 #endif
 
-#define MAX1027_V_CHAN(index)						\
+#define MAX1027_V_CHAN(index, depth)					\
 	{								\
 		.type = IIO_VOLTAGE,					\
 		.indexed = 1,						\
@@ -93,7 +93,7 @@ MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
 		.scan_index = index + 1,				\
 		.scan_type = {						\
 			.sign = 'u',					\
-			.realbits = 10,					\
+			.realbits = depth,				\
 			.storagebits = 16,				\
 			.shift = 2,					\
 			.endianness = IIO_BE,				\
@@ -115,52 +115,42 @@ MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
 		},							\
 	}
 
+#define MAX1X27_CHANNELS(depth)			\
+	MAX1027_T_CHAN,				\
+	MAX1027_V_CHAN(0, depth),		\
+	MAX1027_V_CHAN(1, depth),		\
+	MAX1027_V_CHAN(2, depth),		\
+	MAX1027_V_CHAN(3, depth),		\
+	MAX1027_V_CHAN(4, depth),		\
+	MAX1027_V_CHAN(5, depth),		\
+	MAX1027_V_CHAN(6, depth),		\
+	MAX1027_V_CHAN(7, depth)
+
+#define MAX1X29_CHANNELS(depth)			\
+	MAX1027_V_CHAN(8, depth),		\
+	MAX1027_V_CHAN(9, depth),		\
+	MAX1027_V_CHAN(10, depth),		\
+	MAX1027_V_CHAN(11, depth)
+
+#define MAX1X31_CHANNELS(depth)			\
+	MAX1027_V_CHAN(12, depth),		\
+	MAX1027_V_CHAN(13, depth),		\
+	MAX1027_V_CHAN(14, depth),		\
+	MAX1027_V_CHAN(15, depth)
+
 static const struct iio_chan_spec max1027_channels[] = {
-	MAX1027_T_CHAN,
-	MAX1027_V_CHAN(0),
-	MAX1027_V_CHAN(1),
-	MAX1027_V_CHAN(2),
-	MAX1027_V_CHAN(3),
-	MAX1027_V_CHAN(4),
-	MAX1027_V_CHAN(5),
-	MAX1027_V_CHAN(6),
-	MAX1027_V_CHAN(7)
+	MAX1X27_CHANNELS(10)
 };
 
 static const struct iio_chan_spec max1029_channels[] = {
-	MAX1027_T_CHAN,
-	MAX1027_V_CHAN(0),
-	MAX1027_V_CHAN(1),
-	MAX1027_V_CHAN(2),
-	MAX1027_V_CHAN(3),
-	MAX1027_V_CHAN(4),
-	MAX1027_V_CHAN(5),
-	MAX1027_V_CHAN(6),
-	MAX1027_V_CHAN(7),
-	MAX1027_V_CHAN(8),
-	MAX1027_V_CHAN(9),
-	MAX1027_V_CHAN(10),
-	MAX1027_V_CHAN(11)
+	MAX1X27_CHANNELS(10),
+	MAX1X29_CHANNELS(10)
 };
 
 static const struct iio_chan_spec max1031_channels[] = {
-	MAX1027_T_CHAN,
-	MAX1027_V_CHAN(0),
-	MAX1027_V_CHAN(1),
-	MAX1027_V_CHAN(2),
-	MAX1027_V_CHAN(3),
-	MAX1027_V_CHAN(4),
-	MAX1027_V_CHAN(5),
-	MAX1027_V_CHAN(6),
-	MAX1027_V_CHAN(7),
-	MAX1027_V_CHAN(8),
-	MAX1027_V_CHAN(9),
-	MAX1027_V_CHAN(10),
-	MAX1027_V_CHAN(11),
-	MAX1027_V_CHAN(12),
-	MAX1027_V_CHAN(13),
-	MAX1027_V_CHAN(14),
-	MAX1027_V_CHAN(15)
+	MAX1X27_CHANNELS(10),
+	MAX1X29_CHANNELS(10),
+	MAX1X31_CHANNELS(10)
 };
 
 static const unsigned long max1027_available_scan_masks[] = {
@@ -181,6 +171,7 @@ static const unsigned long max1031_available_scan_masks[] = {
 struct max1027_chip_info {
 	const struct iio_chan_spec *channels;
 	unsigned int num_channels;
+	unsigned int depth;
 	const unsigned long *available_scan_masks;
 };
 
@@ -188,16 +179,19 @@ static const struct max1027_chip_info max1027_chip_info_tbl[] = {
 	[max1027] = {
 		.channels = max1027_channels,
 		.num_channels = ARRAY_SIZE(max1027_channels),
+		.depth = 10,
 		.available_scan_masks = max1027_available_scan_masks,
 	},
 	[max1029] = {
 		.channels = max1029_channels,
 		.num_channels = ARRAY_SIZE(max1029_channels),
+		.depth = 10,
 		.available_scan_masks = max1029_available_scan_masks,
 	},
 	[max1031] = {
 		.channels = max1031_channels,
 		.num_channels = ARRAY_SIZE(max1031_channels),
+		.depth = 10,
 		.available_scan_masks = max1031_available_scan_masks,
 	},
 };
@@ -284,7 +278,7 @@ static int max1027_read_raw(struct iio_dev *indio_dev,
 			break;
 		case IIO_VOLTAGE:
 			*val = 2500;
-			*val2 = 10;
+			*val2 = st->info->depth;
 			ret = IIO_VAL_FRACTIONAL_LOG2;
 			break;
 		default:
-- 
2.20.1


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

* [PATCH v2 5/7] iio: adc: max1027: Introduce 12-bit devices support
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
                   ` (3 preceding siblings ...)
  2019-10-03 17:33 ` [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions Miquel Raynal
@ 2019-10-03 17:33 ` Miquel Raynal
  2019-10-06 10:24   ` Jonathan Cameron
  2019-10-03 17:34 ` [PATCH v2 6/7] dt-bindings: iio: adc: max1027: Mark interrupts as optional Miquel Raynal
  2019-10-03 17:34 ` [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles Miquel Raynal
  6 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:33 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

Maxim's max12xx series is very similar to the max10xx series, with the
difference of the measurements depth which is upgraded from 10 to 12
bits per channel. Everything else looks the same.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/Kconfig   |  4 ++--
 drivers/iio/adc/max1027.c | 44 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f0af3a42f53c..6ac16d738822 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -508,8 +508,8 @@ config MAX1027
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
-	  Say yes here to build support for Maxim SPI ADC models
-	  max1027, max1029 and max1031.
+	  Say yes here to build support for Maxim SPI {10,12}-bit ADC models:
+	  max1027, max1029, max1031, max1227, max1229 and max1231.
 
 	  To compile this driver as a module, choose M here: the module will be
 	  called max1027.
diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 5d5d223dd42a..0d7116e9a63b 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -63,12 +63,18 @@ enum max1027_id {
 	max1027,
 	max1029,
 	max1031,
+	max1227,
+	max1229,
+	max1231,
 };
 
 static const struct spi_device_id max1027_id[] = {
 	{"max1027", max1027},
 	{"max1029", max1029},
 	{"max1031", max1031},
+	{"max1227", max1227},
+	{"max1229", max1229},
+	{"max1231", max1231},
 	{}
 };
 MODULE_DEVICE_TABLE(spi, max1027_id);
@@ -78,6 +84,9 @@ static const struct of_device_id max1027_adc_dt_ids[] = {
 	{ .compatible = "maxim,max1027" },
 	{ .compatible = "maxim,max1029" },
 	{ .compatible = "maxim,max1031" },
+	{ .compatible = "maxim,max1227" },
+	{ .compatible = "maxim,max1229" },
+	{ .compatible = "maxim,max1231" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
@@ -153,6 +162,21 @@ static const struct iio_chan_spec max1031_channels[] = {
 	MAX1X31_CHANNELS(10)
 };
 
+static const struct iio_chan_spec max1227_channels[] = {
+	MAX1X27_CHANNELS(12)
+};
+
+static const struct iio_chan_spec max1229_channels[] = {
+	MAX1X27_CHANNELS(12),
+	MAX1X29_CHANNELS(12)
+};
+
+static const struct iio_chan_spec max1231_channels[] = {
+	MAX1X27_CHANNELS(12),
+	MAX1X29_CHANNELS(12),
+	MAX1X31_CHANNELS(12)
+};
+
 static const unsigned long max1027_available_scan_masks[] = {
 	0x000001ff,
 	0x00000000,
@@ -194,6 +218,24 @@ static const struct max1027_chip_info max1027_chip_info_tbl[] = {
 		.depth = 10,
 		.available_scan_masks = max1031_available_scan_masks,
 	},
+	[max1227] = {
+		.channels = max1227_channels,
+		.num_channels = ARRAY_SIZE(max1227_channels),
+		.depth = 12,
+		.available_scan_masks = max1027_available_scan_masks,
+	},
+	[max1229] = {
+		.channels = max1229_channels,
+		.num_channels = ARRAY_SIZE(max1229_channels),
+		.depth = 12,
+		.available_scan_masks = max1029_available_scan_masks,
+	},
+	[max1231] = {
+		.channels = max1231_channels,
+		.num_channels = ARRAY_SIZE(max1231_channels),
+		.depth = 12,
+		.available_scan_masks = max1031_available_scan_masks,
+	},
 };
 
 struct max1027_state {
@@ -490,5 +532,5 @@ static struct spi_driver max1027_driver = {
 module_spi_driver(max1027_driver);
 
 MODULE_AUTHOR("Philippe Reynes <tremyfr@yahoo.fr>");
-MODULE_DESCRIPTION("MAX1027/MAX1029/MAX1031 ADC");
+MODULE_DESCRIPTION("MAX1X27/MAX1X29/MAX1X31 ADC");
 MODULE_LICENSE("GPL v2");
-- 
2.20.1


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

* [PATCH v2 6/7] dt-bindings: iio: adc: max1027: Mark interrupts as optional
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
                   ` (4 preceding siblings ...)
  2019-10-03 17:33 ` [PATCH v2 5/7] iio: adc: max1027: Introduce 12-bit devices support Miquel Raynal
@ 2019-10-03 17:34 ` Miquel Raynal
  2019-10-03 17:34 ` [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles Miquel Raynal
  6 siblings, 0 replies; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:34 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

The chips have a 'start conversion' and a 'end of conversion' pair of
pins. They can be used but this is absolutely not mandatory as regular
polling is supported by the chip depending on its internal clocking
setup.

There is no physical reason to force the use of interrupts so turn
them optional.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 Documentation/devicetree/bindings/iio/adc/max1027-adc.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt b/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
index e680c61dfb84..7b23d68f655c 100644
--- a/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
@@ -3,6 +3,8 @@
 Required properties:
   - compatible: Should be "maxim,max1027" or "maxim,max1029" or "maxim,max1031"
   - reg: SPI chip select number for the device
+
+Optional properties:
   - interrupts: IRQ line for the ADC
   see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 
-- 
2.20.1


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

* [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles
  2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
                   ` (5 preceding siblings ...)
  2019-10-03 17:34 ` [PATCH v2 6/7] dt-bindings: iio: adc: max1027: Mark interrupts as optional Miquel Raynal
@ 2019-10-03 17:34 ` Miquel Raynal
  2019-10-06 10:27   ` Jonathan Cameron
  6 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-03 17:34 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland
  Cc: devicetree, linux-iio, linux-kernel, Thomas Petazzoni, Miquel Raynal

Update the bindings documentation with new Maxim ADCs compatibles.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/iio/adc/max1027-adc.txt        | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt b/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
index 7b23d68f655c..1b703a01d882 100644
--- a/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
@@ -1,7 +1,13 @@
-* Maxim 1027/1029/1031 Analog to Digital Converter (ADC)
+* Maxim 1027/1029/1031/1227/1229/1231 Analog to Digital Converter (ADC)
 
 Required properties:
-  - compatible: Should be "maxim,max1027" or "maxim,max1029" or "maxim,max1031"
+  - compatible: Should be one of:
+    * "maxim,max1027"
+    * "maxim,max1029"
+    * "maxim,max1031"
+    * "maxim,max1227"
+    * "maxim,max1229"
+    * "maxim,max1231"
   - reg: SPI chip select number for the device
 
 Optional properties:
-- 
2.20.1


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

* Re: [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support
  2019-10-03 17:33 ` [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support Miquel Raynal
@ 2019-10-06 10:04   ` Jonathan Cameron
  2019-10-07 10:00     ` Miquel Raynal
  0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-06 10:04 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

On Thu,  3 Oct 2019 19:33:55 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Until now, only write operations were supported. Force two bytes read
> operation when reading from this register (might be wrong when reading
> the temperature, but will work with any other value).

That's worrying as comments go.  Just return an error on the temperature
register if it's going to do the wrong thing.

Thanks,

Jonathan

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/max1027.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 214883458582..6cdfe9ef73fc 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -309,8 +309,11 @@ static int max1027_debugfs_reg_access(struct iio_dev *indio_dev,
>  	struct max1027_state *st = iio_priv(indio_dev);
>  	u8 *val = (u8 *)st->buffer;
>  
> -	if (readval != NULL)
> -		return -EINVAL;
> +	if (readval) {
> +		int ret = spi_read(st->spi, val, 2);
> +		*readval = be16_to_cpu(st->buffer[0]);
> +		return ret;
> +	}
>  
>  	*val = (u8)writeval;
>  	return spi_write(st->spi, val, 1);


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

* Re: [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts
  2019-10-03 17:33 ` [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts Miquel Raynal
@ 2019-10-06 10:18   ` Jonathan Cameron
  2019-10-07 10:01     ` Miquel Raynal
  0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-06 10:18 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

On Thu,  3 Oct 2019 19:33:56 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> The chip has a 'start conversion' and a 'end of conversion' pair of
> pins. They can be used but this is absolutely not mandatory as regular
> polling of the value is totally fine with the current internal
> clocking setup. Turn the interrupts optional and do not error out if
> they are not inquired in the device tree. This has the effect to
> prevent triggered buffers use though.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Hmm. I haven't looked a this in a great deal of depth but if we support
single channel reads it should be possible to allow the use of a
trigger from elsewhere.  Looks like a fair bit of new code would be needed
to support that though.  So perhaps this is a good first step.

It's a bit annoying that the hardware doesn't provide a EOC bit
anywhere in the registers.  That would have allowed us to be a bit
cleverer.

Anyhow, this looks fine to me.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/max1027.c | 57 +++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 6cdfe9ef73fc..823223b77a70 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -430,35 +430,40 @@ static int max1027_probe(struct spi_device *spi)
>  		return -ENOMEM;
>  	}
>  
> -	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> -					&iio_pollfunc_store_time,
> -					&max1027_trigger_handler, NULL);
> -	if (ret < 0) {
> -		dev_err(&indio_dev->dev, "Failed to setup buffer\n");
> -		return ret;
> -	}

> +	if (spi->irq) {
> +		ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +						      &iio_pollfunc_store_time,
> +						      &max1027_trigger_handler,
> +						      NULL);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Failed to setup buffer\n");
> +			return ret;
> +		}
>  
> -	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
> -							indio_dev->name);
> -	if (st->trig == NULL) {
> -		ret = -ENOMEM;
> -		dev_err(&indio_dev->dev, "Failed to allocate iio trigger\n");
> -		return ret;
> -	}
> +		st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-trigger",
> +						  indio_dev->name);
> +		if (st->trig == NULL) {
> +			ret = -ENOMEM;
> +			dev_err(&indio_dev->dev,
> +				"Failed to allocate iio trigger\n");
> +			return ret;
> +		}
>  
> -	st->trig->ops = &max1027_trigger_ops;
> -	st->trig->dev.parent = &spi->dev;
> -	iio_trigger_set_drvdata(st->trig, indio_dev);
> -	iio_trigger_register(st->trig);
> +		st->trig->ops = &max1027_trigger_ops;
> +		st->trig->dev.parent = &spi->dev;
> +		iio_trigger_set_drvdata(st->trig, indio_dev);
> +		iio_trigger_register(st->trig);
>  
> -	ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> -					iio_trigger_generic_data_rdy_poll,
> -					NULL,
> -					IRQF_TRIGGER_FALLING,
> -					spi->dev.driver->name, st->trig);
> -	if (ret < 0) {
> -		dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
> -		return ret;
> +		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> +						iio_trigger_generic_data_rdy_poll,
> +						NULL,
> +						IRQF_TRIGGER_FALLING,
> +						spi->dev.driver->name,
> +						st->trig);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Failed to allocate IRQ.\n");
> +			return ret;
> +		}
>  	}
>  
>  	/* Disable averaging */


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

* Re: [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions
  2019-10-03 17:33 ` [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions Miquel Raynal
@ 2019-10-06 10:22   ` Jonathan Cameron
  2019-10-07 10:03     ` Miquel Raynal
  0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-06 10:22 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

On Thu,  3 Oct 2019 19:33:58 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Maxim's max1027/29/31 series returns the measured voltages with a
> resolution of 10 bits. There is a very similar series, max1227/29/31
> which works identically but uses a resolution of 12 bits. Prepare the
> support for these chips by turning the 'depth' into a macro parameter
> instead of hardcoding it everywhere. Also reorganize just a bit the
> macros at the top to avoid repeating tens of lines when adding support
> for a new chip.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Minor comments inline.

Jonathan

> ---
>  drivers/iio/adc/max1027.c | 78 ++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index f9b473ee6711..5d5d223dd42a 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -83,7 +83,7 @@ static const struct of_device_id max1027_adc_dt_ids[] = {
>  MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
>  #endif
>  
> -#define MAX1027_V_CHAN(index)						\
> +#define MAX1027_V_CHAN(index, depth)					\
>  	{								\
>  		.type = IIO_VOLTAGE,					\
>  		.indexed = 1,						\
> @@ -93,7 +93,7 @@ MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
>  		.scan_index = index + 1,				\
>  		.scan_type = {						\
>  			.sign = 'u',					\
> -			.realbits = 10,					\
> +			.realbits = depth,				\
>  			.storagebits = 16,				\
>  			.shift = 2,					\
>  			.endianness = IIO_BE,				\
> @@ -115,52 +115,42 @@ MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
>  		},							\
>  	}
>  
> +#define MAX1X27_CHANNELS(depth)			\
> +	MAX1027_T_CHAN,				\
> +	MAX1027_V_CHAN(0, depth),		\
> +	MAX1027_V_CHAN(1, depth),		\
> +	MAX1027_V_CHAN(2, depth),		\
> +	MAX1027_V_CHAN(3, depth),		\
> +	MAX1027_V_CHAN(4, depth),		\
> +	MAX1027_V_CHAN(5, depth),		\
> +	MAX1027_V_CHAN(6, depth),		\
> +	MAX1027_V_CHAN(7, depth)
> +
> +#define MAX1X29_CHANNELS(depth)			\
> +	MAX1027_V_CHAN(8, depth),		\
> +	MAX1027_V_CHAN(9, depth),		\
> +	MAX1027_V_CHAN(10, depth),		\
> +	MAX1027_V_CHAN(11, depth)
> +

Modify this a touch so the macro for MAX1X29_CHANNELS includes
MAX1X27_CHANNELS.  That way each macro's name matches what it
does rather than the 'additional channels' for that device.

> +#define MAX1X31_CHANNELS(depth)			\
> +	MAX1027_V_CHAN(12, depth),		\
> +	MAX1027_V_CHAN(13, depth),		\
> +	MAX1027_V_CHAN(14, depth),		\
> +	MAX1027_V_CHAN(15, depth)
> +
>  static const struct iio_chan_spec max1027_channels[] = {
> -	MAX1027_T_CHAN,
> -	MAX1027_V_CHAN(0),
> -	MAX1027_V_CHAN(1),
> -	MAX1027_V_CHAN(2),
> -	MAX1027_V_CHAN(3),
> -	MAX1027_V_CHAN(4),
> -	MAX1027_V_CHAN(5),
> -	MAX1027_V_CHAN(6),
> -	MAX1027_V_CHAN(7)
> +	MAX1X27_CHANNELS(10)
>  };
>  
>  static const struct iio_chan_spec max1029_channels[] = {
> -	MAX1027_T_CHAN,
> -	MAX1027_V_CHAN(0),
> -	MAX1027_V_CHAN(1),
> -	MAX1027_V_CHAN(2),
> -	MAX1027_V_CHAN(3),
> -	MAX1027_V_CHAN(4),
> -	MAX1027_V_CHAN(5),
> -	MAX1027_V_CHAN(6),
> -	MAX1027_V_CHAN(7),
> -	MAX1027_V_CHAN(8),
> -	MAX1027_V_CHAN(9),
> -	MAX1027_V_CHAN(10),
> -	MAX1027_V_CHAN(11)
> +	MAX1X27_CHANNELS(10),
> +	MAX1X29_CHANNELS(10)
>  };
>  
>  static const struct iio_chan_spec max1031_channels[] = {
> -	MAX1027_T_CHAN,
> -	MAX1027_V_CHAN(0),
> -	MAX1027_V_CHAN(1),
> -	MAX1027_V_CHAN(2),
> -	MAX1027_V_CHAN(3),
> -	MAX1027_V_CHAN(4),
> -	MAX1027_V_CHAN(5),
> -	MAX1027_V_CHAN(6),
> -	MAX1027_V_CHAN(7),
> -	MAX1027_V_CHAN(8),
> -	MAX1027_V_CHAN(9),
> -	MAX1027_V_CHAN(10),
> -	MAX1027_V_CHAN(11),
> -	MAX1027_V_CHAN(12),
> -	MAX1027_V_CHAN(13),
> -	MAX1027_V_CHAN(14),
> -	MAX1027_V_CHAN(15)
> +	MAX1X27_CHANNELS(10),
> +	MAX1X29_CHANNELS(10),
> +	MAX1X31_CHANNELS(10)
>  };
>  
>  static const unsigned long max1027_available_scan_masks[] = {
> @@ -181,6 +171,7 @@ static const unsigned long max1031_available_scan_masks[] = {
>  struct max1027_chip_info {
>  	const struct iio_chan_spec *channels;
>  	unsigned int num_channels;
> +	unsigned int depth;

Could we use the channel real_bits field instead of replicating the info?

>  	const unsigned long *available_scan_masks;
>  };
>  
> @@ -188,16 +179,19 @@ static const struct max1027_chip_info max1027_chip_info_tbl[] = {
>  	[max1027] = {
>  		.channels = max1027_channels,
>  		.num_channels = ARRAY_SIZE(max1027_channels),
> +		.depth = 10,
>  		.available_scan_masks = max1027_available_scan_masks,
>  	},
>  	[max1029] = {
>  		.channels = max1029_channels,
>  		.num_channels = ARRAY_SIZE(max1029_channels),
> +		.depth = 10,
>  		.available_scan_masks = max1029_available_scan_masks,
>  	},
>  	[max1031] = {
>  		.channels = max1031_channels,
>  		.num_channels = ARRAY_SIZE(max1031_channels),
> +		.depth = 10,
>  		.available_scan_masks = max1031_available_scan_masks,
>  	},
>  };
> @@ -284,7 +278,7 @@ static int max1027_read_raw(struct iio_dev *indio_dev,
>  			break;
>  		case IIO_VOLTAGE:
>  			*val = 2500;
> -			*val2 = 10;
> +			*val2 = st->info->depth;
>  			ret = IIO_VAL_FRACTIONAL_LOG2;
>  			break;
>  		default:


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

* Re: [PATCH v2 5/7] iio: adc: max1027: Introduce 12-bit devices support
  2019-10-03 17:33 ` [PATCH v2 5/7] iio: adc: max1027: Introduce 12-bit devices support Miquel Raynal
@ 2019-10-06 10:24   ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-06 10:24 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

On Thu,  3 Oct 2019 19:33:59 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Maxim's max12xx series is very similar to the max10xx series, with the
> difference of the measurements depth which is upgraded from 10 to 12
> bits per channel. Everything else looks the same.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Looks good.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig   |  4 ++--
>  drivers/iio/adc/max1027.c | 44 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f0af3a42f53c..6ac16d738822 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -508,8 +508,8 @@ config MAX1027
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  	help
> -	  Say yes here to build support for Maxim SPI ADC models
> -	  max1027, max1029 and max1031.
> +	  Say yes here to build support for Maxim SPI {10,12}-bit ADC models:
> +	  max1027, max1029, max1031, max1227, max1229 and max1231.
>  
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1027.
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 5d5d223dd42a..0d7116e9a63b 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -63,12 +63,18 @@ enum max1027_id {
>  	max1027,
>  	max1029,
>  	max1031,
> +	max1227,
> +	max1229,
> +	max1231,
>  };
>  
>  static const struct spi_device_id max1027_id[] = {
>  	{"max1027", max1027},
>  	{"max1029", max1029},
>  	{"max1031", max1031},
> +	{"max1227", max1227},
> +	{"max1229", max1229},
> +	{"max1231", max1231},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(spi, max1027_id);
> @@ -78,6 +84,9 @@ static const struct of_device_id max1027_adc_dt_ids[] = {
>  	{ .compatible = "maxim,max1027" },
>  	{ .compatible = "maxim,max1029" },
>  	{ .compatible = "maxim,max1031" },
> +	{ .compatible = "maxim,max1227" },
> +	{ .compatible = "maxim,max1229" },
> +	{ .compatible = "maxim,max1231" },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, max1027_adc_dt_ids);
> @@ -153,6 +162,21 @@ static const struct iio_chan_spec max1031_channels[] = {
>  	MAX1X31_CHANNELS(10)
>  };
>  
> +static const struct iio_chan_spec max1227_channels[] = {
> +	MAX1X27_CHANNELS(12)
> +};
> +
> +static const struct iio_chan_spec max1229_channels[] = {
> +	MAX1X27_CHANNELS(12),
> +	MAX1X29_CHANNELS(12)
> +};
> +
> +static const struct iio_chan_spec max1231_channels[] = {
> +	MAX1X27_CHANNELS(12),
> +	MAX1X29_CHANNELS(12),
> +	MAX1X31_CHANNELS(12)
> +};
> +
>  static const unsigned long max1027_available_scan_masks[] = {
>  	0x000001ff,
>  	0x00000000,
> @@ -194,6 +218,24 @@ static const struct max1027_chip_info max1027_chip_info_tbl[] = {
>  		.depth = 10,
>  		.available_scan_masks = max1031_available_scan_masks,
>  	},
> +	[max1227] = {
> +		.channels = max1227_channels,
> +		.num_channels = ARRAY_SIZE(max1227_channels),
> +		.depth = 12,
> +		.available_scan_masks = max1027_available_scan_masks,
> +	},
> +	[max1229] = {
> +		.channels = max1229_channels,
> +		.num_channels = ARRAY_SIZE(max1229_channels),
> +		.depth = 12,
> +		.available_scan_masks = max1029_available_scan_masks,
> +	},
> +	[max1231] = {
> +		.channels = max1231_channels,
> +		.num_channels = ARRAY_SIZE(max1231_channels),
> +		.depth = 12,
> +		.available_scan_masks = max1031_available_scan_masks,
> +	},
>  };
>  
>  struct max1027_state {
> @@ -490,5 +532,5 @@ static struct spi_driver max1027_driver = {
>  module_spi_driver(max1027_driver);
>  
>  MODULE_AUTHOR("Philippe Reynes <tremyfr@yahoo.fr>");
> -MODULE_DESCRIPTION("MAX1027/MAX1029/MAX1031 ADC");
> +MODULE_DESCRIPTION("MAX1X27/MAX1X29/MAX1X31 ADC");
>  MODULE_LICENSE("GPL v2");


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

* Re: [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles
  2019-10-03 17:34 ` [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles Miquel Raynal
@ 2019-10-06 10:27   ` Jonathan Cameron
  2019-10-07 10:04     ` Miquel Raynal
  0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-06 10:27 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

On Thu,  3 Oct 2019 19:34:01 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Update the bindings documentation with new Maxim ADCs compatibles.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Both DT patches look fine to me... 

If you happened to fancy doing the yaml conversion it would be
appreciated... :) 

If not we'll get to this one one day.

Anyhow, a few trivial bits in the earlier patches so v3 should be
good to go.

Thanks,

Jonathan

> ---
>  .../devicetree/bindings/iio/adc/max1027-adc.txt        | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt b/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
> index 7b23d68f655c..1b703a01d882 100644
> --- a/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/max1027-adc.txt
> @@ -1,7 +1,13 @@
> -* Maxim 1027/1029/1031 Analog to Digital Converter (ADC)
> +* Maxim 1027/1029/1031/1227/1229/1231 Analog to Digital Converter (ADC)
>  
>  Required properties:
> -  - compatible: Should be "maxim,max1027" or "maxim,max1029" or "maxim,max1031"
> +  - compatible: Should be one of:
> +    * "maxim,max1027"
> +    * "maxim,max1029"
> +    * "maxim,max1031"
> +    * "maxim,max1227"
> +    * "maxim,max1229"
> +    * "maxim,max1231"
>    - reg: SPI chip select number for the device
>  
>  Optional properties:


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

* Re: [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support
  2019-10-06 10:04   ` Jonathan Cameron
@ 2019-10-07 10:00     ` Miquel Raynal
  2019-10-12 13:56       ` Jonathan Cameron
  0 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-07 10:00 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

Hi Jonathan,

Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:04:24
+0100:

> On Thu,  3 Oct 2019 19:33:55 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Until now, only write operations were supported. Force two bytes read
> > operation when reading from this register (might be wrong when reading
> > the temperature, but will work with any other value).  
> 
> That's worrying as comments go.  Just return an error on the temperature
> register if it's going to do the wrong thing.

Actually the debugfs_reg_access hook is supposedly stateless. When
reading registers I don't know what I am reading because the "source" is
selected during the write operation, so I have no reliable way to know
what I am reading.

I set the read length to 2 bytes because most of the "atomic"reads are
two bytes and it allows us to test various commands directly from
userspace and read meaningful values. This is a limitation as:
* Voltage 'atomic' reads are 2 bytes
* Temperature 'atomic' reads are 2 bytes but never come alone (usually
  one voltage input of 2B will follow).
* Any other 'condensed' input will be more than 2 bytes, ie. several
  voltage values in one go.

In any case, doing a software reset of the chip will turn it back
into a working state no matter what was requested/read.

For me, 2-byte reads is a "good enough" solution that will work with
almost all the simplest ('atomic') SPI operations, but if you think
limiting to 2-bytes access is a problem (right now there is only write
access, which is kind of useless on its own) then let's drop the patch.
But I wanted to contribute it because it really helped me during the
development. 


Thanks,
Miquèl

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

* Re: [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts
  2019-10-06 10:18   ` Jonathan Cameron
@ 2019-10-07 10:01     ` Miquel Raynal
  2019-10-07 11:44       ` Jonathan Cameron
  0 siblings, 1 reply; 19+ messages in thread
From: Miquel Raynal @ 2019-10-07 10:01 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

Hi Jonathan,

Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:18:37
+0100:

> On Thu,  3 Oct 2019 19:33:56 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > The chip has a 'start conversion' and a 'end of conversion' pair of
> > pins. They can be used but this is absolutely not mandatory as regular
> > polling of the value is totally fine with the current internal
> > clocking setup. Turn the interrupts optional and do not error out if
> > they are not inquired in the device tree. This has the effect to
> > prevent triggered buffers use though.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Hmm. I haven't looked a this in a great deal of depth but if we support
> single channel reads it should be possible to allow the use of a
> trigger from elsewhere.  Looks like a fair bit of new code would be needed
> to support that though.  So perhaps this is a good first step.
> 
> It's a bit annoying that the hardware doesn't provide a EOC bit
> anywhere in the registers.  That would have allowed us to be a bit
> cleverer.

I totally agree. Actually, this chip does not support any 'register
read', the only things we can read are measures (temperature/voltages).


Thanks,
Miquèl

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

* Re: [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions
  2019-10-06 10:22   ` Jonathan Cameron
@ 2019-10-07 10:03     ` Miquel Raynal
  0 siblings, 0 replies; 19+ messages in thread
From: Miquel Raynal @ 2019-10-07 10:03 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

Hi Jonathan,

> >  
> > +#define MAX1X27_CHANNELS(depth)			\
> > +	MAX1027_T_CHAN,				\
> > +	MAX1027_V_CHAN(0, depth),		\
> > +	MAX1027_V_CHAN(1, depth),		\
> > +	MAX1027_V_CHAN(2, depth),		\
> > +	MAX1027_V_CHAN(3, depth),		\
> > +	MAX1027_V_CHAN(4, depth),		\
> > +	MAX1027_V_CHAN(5, depth),		\
> > +	MAX1027_V_CHAN(6, depth),		\
> > +	MAX1027_V_CHAN(7, depth)
> > +
> > +#define MAX1X29_CHANNELS(depth)			\
> > +	MAX1027_V_CHAN(8, depth),		\
> > +	MAX1027_V_CHAN(9, depth),		\
> > +	MAX1027_V_CHAN(10, depth),		\
> > +	MAX1027_V_CHAN(11, depth)
> > +  
> 
> Modify this a touch so the macro for MAX1X29_CHANNELS includes
> MAX1X27_CHANNELS.  That way each macro's name matches what it
> does rather than the 'additional channels' for that device.

Sure.

> 
> > +#define MAX1X31_CHANNELS(depth)			\
> > +	MAX1027_V_CHAN(12, depth),		\
> > +	MAX1027_V_CHAN(13, depth),		\
> > +	MAX1027_V_CHAN(14, depth),		\
> > +	MAX1027_V_CHAN(15, depth)
> > +
> >  static const struct iio_chan_spec max1027_channels[] = {
> > -	MAX1027_T_CHAN,
> > -	MAX1027_V_CHAN(0),
> > -	MAX1027_V_CHAN(1),
> > -	MAX1027_V_CHAN(2),
> > -	MAX1027_V_CHAN(3),
> > -	MAX1027_V_CHAN(4),
> > -	MAX1027_V_CHAN(5),
> > -	MAX1027_V_CHAN(6),
> > -	MAX1027_V_CHAN(7)
> > +	MAX1X27_CHANNELS(10)
> >  };
> >  
> >  static const struct iio_chan_spec max1029_channels[] = {
> > -	MAX1027_T_CHAN,
> > -	MAX1027_V_CHAN(0),
> > -	MAX1027_V_CHAN(1),
> > -	MAX1027_V_CHAN(2),
> > -	MAX1027_V_CHAN(3),
> > -	MAX1027_V_CHAN(4),
> > -	MAX1027_V_CHAN(5),
> > -	MAX1027_V_CHAN(6),
> > -	MAX1027_V_CHAN(7),
> > -	MAX1027_V_CHAN(8),
> > -	MAX1027_V_CHAN(9),
> > -	MAX1027_V_CHAN(10),
> > -	MAX1027_V_CHAN(11)
> > +	MAX1X27_CHANNELS(10),
> > +	MAX1X29_CHANNELS(10)
> >  };
> >  
> >  static const struct iio_chan_spec max1031_channels[] = {
> > -	MAX1027_T_CHAN,
> > -	MAX1027_V_CHAN(0),
> > -	MAX1027_V_CHAN(1),
> > -	MAX1027_V_CHAN(2),
> > -	MAX1027_V_CHAN(3),
> > -	MAX1027_V_CHAN(4),
> > -	MAX1027_V_CHAN(5),
> > -	MAX1027_V_CHAN(6),
> > -	MAX1027_V_CHAN(7),
> > -	MAX1027_V_CHAN(8),
> > -	MAX1027_V_CHAN(9),
> > -	MAX1027_V_CHAN(10),
> > -	MAX1027_V_CHAN(11),
> > -	MAX1027_V_CHAN(12),
> > -	MAX1027_V_CHAN(13),
> > -	MAX1027_V_CHAN(14),
> > -	MAX1027_V_CHAN(15)
> > +	MAX1X27_CHANNELS(10),
> > +	MAX1X29_CHANNELS(10),
> > +	MAX1X31_CHANNELS(10)
> >  };
> >  
> >  static const unsigned long max1027_available_scan_masks[] = {
> > @@ -181,6 +171,7 @@ static const unsigned long max1031_available_scan_masks[] = {
> >  struct max1027_chip_info {
> >  	const struct iio_chan_spec *channels;
> >  	unsigned int num_channels;
> > +	unsigned int depth;  
> 
> Could we use the channel real_bits field instead of replicating the info?

I'll try. Indeed I would prefer not to replicate the info.


Thanks,
Miquèl

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

* Re: [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles
  2019-10-06 10:27   ` Jonathan Cameron
@ 2019-10-07 10:04     ` Miquel Raynal
  0 siblings, 0 replies; 19+ messages in thread
From: Miquel Raynal @ 2019-10-07 10:04 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

Hi Jonathan,

Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:27:51
+0100:

> On Thu,  3 Oct 2019 19:34:01 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Update the bindings documentation with new Maxim ADCs compatibles.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Both DT patches look fine to me... 
> 
> If you happened to fancy doing the yaml conversion it would be
> appreciated... :) 

I'm being requested all the time, that would be my third yaml
conversion, so let's go for it (if it's not overly complicated, I'll
check first...) :)

> If not we'll get to this one one day.
> 
> Anyhow, a few trivial bits in the earlier patches so v3 should be
> good to go.
> 
> Thanks,
> 
> Jonathan
> 

Thanks,
Miquèl

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

* Re: [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts
  2019-10-07 10:01     ` Miquel Raynal
@ 2019-10-07 11:44       ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-07 11:44 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Rob Herring, Mark Rutland, devicetree,
	linux-iio, linux-kernel, Thomas Petazzoni

On Mon, 7 Oct 2019 12:01:22 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:18:37
> +0100:
> 
> > On Thu,  3 Oct 2019 19:33:56 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > The chip has a 'start conversion' and a 'end of conversion' pair of
> > > pins. They can be used but this is absolutely not mandatory as regular
> > > polling of the value is totally fine with the current internal
> > > clocking setup. Turn the interrupts optional and do not error out if
> > > they are not inquired in the device tree. This has the effect to
> > > prevent triggered buffers use though.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>    
> > 
> > Hmm. I haven't looked a this in a great deal of depth but if we support
> > single channel reads it should be possible to allow the use of a
> > trigger from elsewhere.  Looks like a fair bit of new code would be needed
> > to support that though.  So perhaps this is a good first step.
> > 
> > It's a bit annoying that the hardware doesn't provide a EOC bit
> > anywhere in the registers.  That would have allowed us to be a bit
> > cleverer.  
> 
> I totally agree. Actually, this chip does not support any 'register
> read', the only things we can read are measures (temperature/voltages).

Ah. Good point.  Shall we polled reading of channels which is what
I meant ;)

Jonathan

> 
> 
> Thanks,
> Miquèl



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

* Re: [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support
  2019-10-07 10:00     ` Miquel Raynal
@ 2019-10-12 13:56       ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2019-10-12 13:56 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, devicetree, linux-iio, linux-kernel,
	Thomas Petazzoni

On Mon, 7 Oct 2019 12:00:01 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Jonathan,
> 
> Jonathan Cameron <jic23@kernel.org> wrote on Sun, 6 Oct 2019 11:04:24
> +0100:
> 
> > On Thu,  3 Oct 2019 19:33:55 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Until now, only write operations were supported. Force two bytes read
> > > operation when reading from this register (might be wrong when reading
> > > the temperature, but will work with any other value).    
> > 
> > That's worrying as comments go.  Just return an error on the temperature
> > register if it's going to do the wrong thing.  
> 
> Actually the debugfs_reg_access hook is supposedly stateless. When
> reading registers I don't know what I am reading because the "source" is
> selected during the write operation, so I have no reliable way to know
> what I am reading.
> 
> I set the read length to 2 bytes because most of the "atomic"reads are
> two bytes and it allows us to test various commands directly from
> userspace and read meaningful values. This is a limitation as:
> * Voltage 'atomic' reads are 2 bytes
> * Temperature 'atomic' reads are 2 bytes but never come alone (usually
>   one voltage input of 2B will follow).
> * Any other 'condensed' input will be more than 2 bytes, ie. several
>   voltage values in one go.
> 
> In any case, doing a software reset of the chip will turn it back
> into a working state no matter what was requested/read.
> 
> For me, 2-byte reads is a "good enough" solution that will work with
> almost all the simplest ('atomic') SPI operations, but if you think
> limiting to 2-bytes access is a problem (right now there is only write
> access, which is kind of useless on its own) then let's drop the patch.
> But I wanted to contribute it because it really helped me during the
> development. 

This is fine as is.  Comment was worrying so could perhaps have given
more detail.  Still it's a debug interface, people get to look at the
datasheet if they are using this :)

Jonathan


> 
> 
> Thanks,
> Miquèl


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

end of thread, other threads:[~2019-10-12 13:56 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03 17:33 [PATCH v2 0/7] Introduce max12xx ADC support Miquel Raynal
2019-10-03 17:33 ` [PATCH v2 1/7] iio: adc: max1027: Add debugfs register read support Miquel Raynal
2019-10-06 10:04   ` Jonathan Cameron
2019-10-07 10:00     ` Miquel Raynal
2019-10-12 13:56       ` Jonathan Cameron
2019-10-03 17:33 ` [PATCH v2 2/7] iio: adc: max1027: Make it optional to use interrupts Miquel Raynal
2019-10-06 10:18   ` Jonathan Cameron
2019-10-07 10:01     ` Miquel Raynal
2019-10-07 11:44       ` Jonathan Cameron
2019-10-03 17:33 ` [PATCH v2 3/7] iio: adc: max1027: Reset the device at probe time Miquel Raynal
2019-10-03 17:33 ` [PATCH v2 4/7] iio: adc: max1027: Prepare the introduction of different resolutions Miquel Raynal
2019-10-06 10:22   ` Jonathan Cameron
2019-10-07 10:03     ` Miquel Raynal
2019-10-03 17:33 ` [PATCH v2 5/7] iio: adc: max1027: Introduce 12-bit devices support Miquel Raynal
2019-10-06 10:24   ` Jonathan Cameron
2019-10-03 17:34 ` [PATCH v2 6/7] dt-bindings: iio: adc: max1027: Mark interrupts as optional Miquel Raynal
2019-10-03 17:34 ` [PATCH v2 7/7] dt-bindings: iio: adc: max1027: Document max12xx series compatibles Miquel Raynal
2019-10-06 10:27   ` Jonathan Cameron
2019-10-07 10:04     ` Miquel Raynal

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