[PATCH 0/6] IIO driver for MCP3550/1/3

[PATCH 0/6] IIO driver for MCP3550/1/3

[Lukas Wunner]

Two fixes, one speedup & one cleanup for the mcp320x.c driver,
plus support for new chips:

The MCP3550/1/3 are 22 bit delta-sigma ADCs with high accuracy,
low noise characteristics and an extended temperature range.
It's the top-of-the-line SPI ADC in Microchip's portfolio.

The chip is used by the "Revolution Pi" family of open source PLCs
based on the Raspberry Pi (https://revolution.kunbus.com/).

Thanks,

Lukas


Lukas Wunner (6):
  iio: adc: mcp320x: Fix oops on module unload
  iio: adc: mcp320x: Fix readout of negative voltages
  iio: adc: mcp320x: Speed up readout of single-channel ADCs
  iio: adc: mcp320x: Drop unnecessary of_device_id attributes
  dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3
  iio: adc: mcp320x: Add support for mcp3550/1/3

 .../devicetree/bindings/iio/adc/mcp320x.txt        |  23 ++
 drivers/iio/adc/Kconfig                            |   5 +-
 drivers/iio/adc/mcp320x.c                          | 264 ++++++++++++++-------
 3 files changed, 202 insertions(+), 90 deletions(-)

-- 
2.11.0

[PATCH 6/6] iio: adc: mcp320x: Add support for mcp3550/1/3

[Lukas Wunner]

These ADCs are marketed as single-channel 22 bit delta-sigma ADCs, but
in reality their resolution is 21 bit with an overrange or underrange
of 12% beyond Vref.  In other words, "full scale" means +/- 2^20.

This driver does not explicitly signal back to the user when an
overrange or underrange occurs, but the user can detect it by comparing
the raw value to +/- 2^20 (or the scaled value to Vref).

The chips feature an extended temperature range and high accuracy,
low noise characteristics, but their conversion times are slow with
up to 80 ms +/- 2% (on the MCP3550-50).

Hence, unlike the other ADCs supported by the driver, conversion does
not take place in realtime upon lowering CS.  Instead, CS is asserted
for 8 usec to start the conversion.  After waiting for the duration of
the conversion, the result can be fetched.  While waiting, control of
the bus is ceased so it may be used by a different device.

After the result has been fetched and 10 us have passed, the chip goes
into shutdown and an additional power-up delay of 144 clock periods is
then required to wake the analog circuitry upon the next conversion
(footnote below table 4-1, page 16 in the spec).

Optionally, the chips can be used in so-called "continuous conversion
mode":  Conversions then take place continuously and the last result may
be fetched at any time without observing a delay.  The mode is enabled
by permanently driving CS low, e.g. by wiring it to ground.  The driver
defaults to "single conversion mode" but can be instructed to use
"continuous conversion mode" by setting a corresponding boolean in the
device tree.

After power-on reset, the chip sometimes fails to perform conversions
and clocks out "all ones".  For some reason it can be unwedged by
carrying out two consecutive conversions, without any delay between them.
If there is a delay the chip stays wedged.  I tried various other things
to no avail, such as extending the conversion time, extending the CS
assertion time to start a conversion or clocking out a few bytes.
This may be an undocumented bug in the chip's internal state machine,
possibly caused by the SPI pins' state on boot of the bcm2837.
The driver unconditionally performs these two conversions on ->probe to
ascertain all subsequent conversions succeed.  On platforms that support
system sleep, it may be necessary to repeat this procedure upon resume.

The chips clock out a 3 byte word, unlike the other ADCs supported by
the driver which all have a lower resolution than 16 bit and thus make
do with 2 bytes.  Calculate the word length on probe by rounding up the
resolution to full bytes.  Crucially, if the clock idles low, the
transfer is preceded by a useless Data Ready bit, increasing its
length from 24 bit to 25 bit = 4 bytes (section 5.5 in the spec).
Autosense this based on the SPI slave's configuration.

Cc: Mathias Duckeck <m.duckeck@kunbus.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/iio/adc/Kconfig   |   5 +-
 drivers/iio/adc/mcp320x.c | 137 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 133 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 57625653fcb6..84dd04621650 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -475,12 +475,13 @@ config	MAX9611
 	  called max9611.
 
 config MCP320X
-	tristate "Microchip Technology MCP3x01/02/04/08"
+	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
 	depends on SPI
 	help
 	  Say yes here to build support for Microchip Technology's
 	  MCP3001, MCP3002, MCP3004, MCP3008, MCP3201, MCP3202, MCP3204,
-	  MCP3208 or MCP3301 analog to digital converter.
+	  MCP3208, MCP3301, MCP3550, MCP3551 and MCP3553 analog to digital
+	  converters.
 
 	  This driver can also be built as a module. If so, the module will be
 	  called mcp320x.
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 6e10fa934aca..c4ea8794c4c6 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -19,6 +19,11 @@
  * ------------
  * 13 bit converter
  * MCP3301
+ * ------------
+ * 22 bit converter
+ * MCP3550
+ * MCP3551
+ * MCP3553
  *
  * Datasheet can be found here:
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21293C.pdf  mcp3001
@@ -28,6 +33,7 @@
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21034D.pdf  mcp3202
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21298c.pdf  mcp3204/08
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21700E.pdf  mcp3301
+ * http://ww1.microchip.com/downloads/en/DeviceDoc/21950D.pdf  mcp3550/1/3
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -51,25 +57,50 @@ enum {
 	mcp3204,
 	mcp3208,
 	mcp3301,
+	mcp3550_50,
+	mcp3550_60,
+	mcp3551,
+	mcp3553,
 };
 
 struct mcp320x_chip_info {
 	const struct iio_chan_spec *channels;
 	unsigned int num_channels;
 	unsigned int resolution;
+	unsigned int conv_time; /* usec */
 };
 
+/**
+ * struct mcp320x - Microchip SPI ADC instance
+ * @spi: SPI slave (parent of the IIO device)
+ * @msg: SPI message to select a channel and receive a value from the ADC
+ * @transfer: SPI transfers used by @msg
+ * @start_conv_msg: SPI message to start a conversion by briefly asserting CS
+ * @start_conv_transfer: SPI transfer used by @start_conv_msg
+ * @continuous_conv: whether CS is permanently driven low such that conversions
+ *	take place continuously, obviating the need to explicitly start them
+ *	and wait for them to finish
+ * @reg: regulator generating Vref
+ * @lock: protects read sequences
+ * @chip_info: ADC properties
+ * @tx_buf: buffer for @transfer[0] (not used on single-channel converters)
+ * @rx_buf: buffer for @transfer[1]
+ */
 struct mcp320x {
 	struct spi_device *spi;
 	struct spi_message msg;
 	struct spi_transfer transfer[2];
 
+	struct spi_message start_conv_msg;
+	struct spi_transfer start_conv_transfer;
+	bool continuous_conv;
+
 	struct regulator *reg;
 	struct mutex lock;
 	const struct mcp320x_chip_info *chip_info;
 
 	u8 tx_buf ____cacheline_aligned;
-	u8 rx_buf[2];
+	u8 rx_buf[4];
 };
 
 static int mcp320x_channel_to_tx_data(int device_index,
@@ -96,10 +127,19 @@ static int mcp320x_channel_to_tx_data(int device_index,
 static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 				  bool differential, int device_index, int *val)
 {
+	unsigned int overflow;
 	int ret;
 
-	adc->rx_buf[0] = 0;
-	adc->rx_buf[1] = 0;
+	if (adc->chip_info->conv_time && !adc->continuous_conv) {
+		ret = spi_sync(adc->spi, &adc->start_conv_msg);
+		if (ret < 0)
+			return ret;
+
+		usleep_range(adc->chip_info->conv_time,
+			     adc->chip_info->conv_time + 100);
+	}
+
+	memset(&adc->rx_buf, 0, sizeof(adc->rx_buf));
 	if (adc->chip_info->num_channels > 1)
 		adc->tx_buf = mcp320x_channel_to_tx_data(device_index, channel,
 							 differential);
@@ -129,6 +169,34 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 		*val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
 				    | adc->rx_buf[1], 12);
 		return 0;
+	case mcp3550_50:
+	case mcp3550_60:
+	case mcp3551:
+	case mcp3553:
+		/* strip leading Data Ready bit in SPI mode 0,0 */
+		if (!(adc->spi->mode & SPI_CPOL)) {
+			adc->rx_buf[0] <<= 1;
+			adc->rx_buf[0] |= adc->rx_buf[1] >> 7;
+			adc->rx_buf[1] <<= 1;
+			adc->rx_buf[1] |= adc->rx_buf[2] >> 7;
+			adc->rx_buf[2] <<= 1;
+			adc->rx_buf[2] |= adc->rx_buf[3] >> 7;
+		}
+		/*
+		 * If the input is within -vref and vref, bit 21 is the sign.
+		 * Up to 12% overrange or underrange are allowed, in which case
+		 * bit 23 is the sign and bit 0 to 21 is the value.
+		 */
+		overflow = adc->rx_buf[0] >> 6;
+		if ((!overflow && adc->rx_buf[0] & 0x20) || overflow == 2)
+			adc->rx_buf[0] |= 0xc0;  /* negative or underrange */
+		else if (overflow == 1)
+			adc->rx_buf[0] &= ~0xc0; /* overrange */
+		else if (overflow == 3)
+			return -EIO; /* cannot have overrange AND underrange */
+		*val = sign_extend32(adc->rx_buf[0] << 16 | adc->rx_buf[1] << 8
+							  | adc->rx_buf[2], 23);
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -288,6 +356,31 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
 		.resolution = 13
 	},
+	[mcp3550_50] = {
+		.channels = mcp3201_channels,
+		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.resolution = 21,
+		/* 2% max deviation + 144 clock periods to exit shutdown */
+		.conv_time = 80000 * 1.02 + 144000 / 102.4,
+	},
+	[mcp3550_60] = {
+		.channels = mcp3201_channels,
+		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.resolution = 21,
+		.conv_time = 66670 * 1.02 + 144000 / 122.88,
+	},
+	[mcp3551] = {
+		.channels = mcp3201_channels,
+		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.resolution = 21,
+		.conv_time = 73100 * 1.02 + 144000 / 112.64,
+	},
+	[mcp3553] = {
+		.channels = mcp3201_channels,
+		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.resolution = 21,
+		.conv_time = 16670 * 1.02 + 144000 / 122.88,
+	},
 };
 
 static int mcp320x_probe(struct spi_device *spi)
@@ -295,7 +388,7 @@ static int mcp320x_probe(struct spi_device *spi)
 	struct iio_dev *indio_dev;
 	struct mcp320x *adc;
 	const struct mcp320x_chip_info *chip_info;
-	int ret;
+	int ret, device_index;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
 	if (!indio_dev)
@@ -311,7 +404,8 @@ static int mcp320x_probe(struct spi_device *spi)
 	indio_dev->info = &mcp320x_info;
 	spi_set_drvdata(spi, indio_dev);
 
-	chip_info = &mcp320x_chip_infos[spi_get_device_id(spi)->driver_data];
+	device_index = spi_get_device_id(spi)->driver_data;
+	chip_info = &mcp320x_chip_infos[device_index];
 	indio_dev->channels = chip_info->channels;
 	indio_dev->num_channels = chip_info->num_channels;
 
@@ -320,7 +414,8 @@ static int mcp320x_probe(struct spi_device *spi)
 	adc->transfer[0].tx_buf = &adc->tx_buf;
 	adc->transfer[0].len = sizeof(adc->tx_buf);
 	adc->transfer[1].rx_buf = adc->rx_buf;
-	adc->transfer[1].len = sizeof(adc->rx_buf);
+	adc->transfer[1].len = DIV_ROUND_UP(chip_info->resolution, 8);
+
 	if (chip_info->num_channels == 1)
 		/* single-channel converters are rx only (no MOSI pin) */
 		spi_message_init_with_transfers(&adc->msg,
@@ -329,6 +424,26 @@ static int mcp320x_probe(struct spi_device *spi)
 		spi_message_init_with_transfers(&adc->msg, adc->transfer,
 						ARRAY_SIZE(adc->transfer));
 
+	if (device_index == mcp3550_50 || device_index == mcp3551 ||
+	    device_index == mcp3550_60 || device_index == mcp3553) {
+		unsigned int val;
+
+		/* rx len increases from 24 to 25 bit in SPI mode 0,0 */
+		if (!(spi->mode & SPI_CPOL))
+			adc->transfer[1].len++;
+
+		/* conversions are started by asserting CS pin for 8 usec */
+		adc->start_conv_transfer.delay_usecs = 8;
+		spi_message_init_with_transfers(&adc->start_conv_msg,
+						&adc->start_conv_transfer, 1);
+		adc->continuous_conv = device_property_read_bool(&spi->dev,
+					"microchip,continuous-conversion");
+
+		/* perform two consecutive conversions to unwedge device */
+		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);
+		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);
+	}
+
 	adc->reg = devm_regulator_get(&spi->dev, "vref");
 	if (IS_ERR(adc->reg))
 		return PTR_ERR(adc->reg);
@@ -383,6 +498,10 @@ static const struct of_device_id mcp320x_dt_ids[] = {
 	{ .compatible = "microchip,mcp3204" },
 	{ .compatible = "microchip,mcp3208" },
 	{ .compatible = "microchip,mcp3301" },
+	{ .compatible = "microchip,mcp3550-50" },
+	{ .compatible = "microchip,mcp3550-60" },
+	{ .compatible = "microchip,mcp3551" },
+	{ .compatible = "microchip,mcp3553" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, mcp320x_dt_ids);
@@ -398,6 +517,10 @@ static const struct spi_device_id mcp320x_id[] = {
 	{ "mcp3204", mcp3204 },
 	{ "mcp3208", mcp3208 },
 	{ "mcp3301", mcp3301 },
+	{ "mcp3550-50", mcp3550_50 },
+	{ "mcp3550-60", mcp3550_60 },
+	{ "mcp3551", mcp3551 },
+	{ "mcp3553", mcp3553 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, mcp320x_id);
@@ -414,5 +537,5 @@ static struct spi_driver mcp320x_driver = {
 module_spi_driver(mcp320x_driver);
 
 MODULE_AUTHOR("Oskar Andero <oskar.andero@gmail.com>");
-MODULE_DESCRIPTION("Microchip Technology MCP3x01/02/04/08");
+MODULE_DESCRIPTION("Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3");
 MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH 6/6] iio: adc: mcp320x: Add support for mcp3550/1/3

[Jonathan Cameron]

On Tue, 22 Aug 2017 15:33:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> These ADCs are marketed as single-channel 22 bit delta-sigma ADCs, but
> in reality their resolution is 21 bit with an overrange or underrange
> of 12% beyond Vref.  In other words, "full scale" means +/- 2^20.
> 
> This driver does not explicitly signal back to the user when an
> overrange or underrange occurs, but the user can detect it by comparing
> the raw value to +/- 2^20 (or the scaled value to Vref).
> 
> The chips feature an extended temperature range and high accuracy,
> low noise characteristics, but their conversion times are slow with
> up to 80 ms +/- 2% (on the MCP3550-50).
> 
> Hence, unlike the other ADCs supported by the driver, conversion does
> not take place in realtime upon lowering CS.  Instead, CS is asserted
> for 8 usec to start the conversion.  After waiting for the duration of
> the conversion, the result can be fetched.  While waiting, control of
> the bus is ceased so it may be used by a different device.
> 
> After the result has been fetched and 10 us have passed, the chip goes
> into shutdown and an additional power-up delay of 144 clock periods is
> then required to wake the analog circuitry upon the next conversion
> (footnote below table 4-1, page 16 in the spec).
> 
> Optionally, the chips can be used in so-called "continuous conversion
> mode":  Conversions then take place continuously and the last result may
> be fetched at any time without observing a delay.  The mode is enabled
> by permanently driving CS low, e.g. by wiring it to ground.  The driver
> defaults to "single conversion mode" but can be instructed to use
> "continuous conversion mode" by setting a corresponding boolean in the
> device tree.
> 
> After power-on reset, the chip sometimes fails to perform conversions
> and clocks out "all ones".  For some reason it can be unwedged by
> carrying out two consecutive conversions, without any delay between them.

Put this description inline in the driver.

> If there is a delay the chip stays wedged.  I tried various other things
> to no avail, such as extending the conversion time, extending the CS
> assertion time to start a conversion or clocking out a few bytes.
> This may be an undocumented bug in the chip's internal state machine,
> possibly caused by the SPI pins' state on boot of the bcm2837.
> The driver unconditionally performs these two conversions on ->probe to
> ascertain all subsequent conversions succeed.  On platforms that support
> system sleep, it may be necessary to repeat this procedure upon resume.
> 
> The chips clock out a 3 byte word, unlike the other ADCs supported by
> the driver which all have a lower resolution than 16 bit and thus make
> do with 2 bytes.  Calculate the word length on probe by rounding up the
> resolution to full bytes.  Crucially, if the clock idles low, the
> transfer is preceded by a useless Data Ready bit, increasing its
> length from 24 bit to 25 bit = 4 bytes (section 5.5 in the spec).
> Autosense this based on the SPI slave's configuration.

Useless at the moment, but if you want to do 'buffered' iio capture
later, the data ready bit and continuous mode can be combined
to do a tightish poll and ensure you don't miss any samples (you
read at twice the sampling frequency and check the data ready bit.

Of course you could just use the rdy line to do the same thing
if it is wired.

> 
> Cc: Mathias Duckeck <m.duckeck@kunbus.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/iio/adc/Kconfig   |   5 +-
>  drivers/iio/adc/mcp320x.c | 137 +++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 133 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 57625653fcb6..84dd04621650 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -475,12 +475,13 @@ config	MAX9611
>  	  called max9611.
>  
>  config MCP320X
> -	tristate "Microchip Technology MCP3x01/02/04/08"
> +	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
>  	depends on SPI
>  	help
>  	  Say yes here to build support for Microchip Technology's
>  	  MCP3001, MCP3002, MCP3004, MCP3008, MCP3201, MCP3202, MCP3204,
> -	  MCP3208 or MCP3301 analog to digital converter.
> +	  MCP3208, MCP3301, MCP3550, MCP3551 and MCP3553 analog to digital
> +	  converters.
>  
>  	  This driver can also be built as a module. If so, the module will be
>  	  called mcp320x.
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 6e10fa934aca..c4ea8794c4c6 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -19,6 +19,11 @@
>   * ------------
>   * 13 bit converter
>   * MCP3301
> + * ------------
> + * 22 bit converter
> + * MCP3550
> + * MCP3551
> + * MCP3553
>   *
>   * Datasheet can be found here:
>   * http://ww1.microchip.com/downloads/en/DeviceDoc/21293C.pdf  mcp3001
> @@ -28,6 +33,7 @@
>   * http://ww1.microchip.com/downloads/en/DeviceDoc/21034D.pdf  mcp3202
>   * http://ww1.microchip.com/downloads/en/DeviceDoc/21298c.pdf  mcp3204/08
>   * http://ww1.microchip.com/downloads/en/DeviceDoc/21700E.pdf  mcp3301
> + * http://ww1.microchip.com/downloads/en/DeviceDoc/21950D.pdf  mcp3550/1/3
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -51,25 +57,50 @@ enum {
>  	mcp3204,
>  	mcp3208,
>  	mcp3301,
> +	mcp3550_50,
> +	mcp3550_60,
> +	mcp3551,
> +	mcp3553,
>  };
>  
>  struct mcp320x_chip_info {
>  	const struct iio_chan_spec *channels;
>  	unsigned int num_channels;
>  	unsigned int resolution;
> +	unsigned int conv_time; /* usec */
>  };
>  
> +/**
> + * struct mcp320x - Microchip SPI ADC instance
> + * @spi: SPI slave (parent of the IIO device)
> + * @msg: SPI message to select a channel and receive a value from the ADC
> + * @transfer: SPI transfers used by @msg
> + * @start_conv_msg: SPI message to start a conversion by briefly asserting CS
> + * @start_conv_transfer: SPI transfer used by @start_conv_msg
> + * @continuous_conv: whether CS is permanently driven low such that conversions
> + *	take place continuously, obviating the need to explicitly start them
> + *	and wait for them to finish
> + * @reg: regulator generating Vref
> + * @lock: protects read sequences
> + * @chip_info: ADC properties
> + * @tx_buf: buffer for @transfer[0] (not used on single-channel converters)
> + * @rx_buf: buffer for @transfer[1]
> + */

This is good to have, but I would prefer that the docs are introduced
in a separate patch - ideal is before this one then add the new stuff or
changes in this patch.

>  struct mcp320x {
>  	struct spi_device *spi;
>  	struct spi_message msg;
>  	struct spi_transfer transfer[2];
>  
> +	struct spi_message start_conv_msg;
> +	struct spi_transfer start_conv_transfer;
> +	bool continuous_conv;
> +
>  	struct regulator *reg;
>  	struct mutex lock;
>  	const struct mcp320x_chip_info *chip_info;
>  
>  	u8 tx_buf ____cacheline_aligned;
> -	u8 rx_buf[2];
> +	u8 rx_buf[4];
>  };
>  
>  static int mcp320x_channel_to_tx_data(int device_index,
> @@ -96,10 +127,19 @@ static int mcp320x_channel_to_tx_data(int device_index,
>  static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
>  				  bool differential, int device_index, int *val)
>  {
> +	unsigned int overflow;
>  	int ret;
>  
> -	adc->rx_buf[0] = 0;
> -	adc->rx_buf[1] = 0;
> +	if (adc->chip_info->conv_time && !adc->continuous_conv) {
> +		ret = spi_sync(adc->spi, &adc->start_conv_msg);
> +		if (ret < 0)
> +			return ret;
> +
> +		usleep_range(adc->chip_info->conv_time,
> +			     adc->chip_info->conv_time + 100);
> +	}
> +
> +	memset(&adc->rx_buf, 0, sizeof(adc->rx_buf));
>  	if (adc->chip_info->num_channels > 1)
>  		adc->tx_buf = mcp320x_channel_to_tx_data(device_index, channel,
>  							 differential);
> @@ -129,6 +169,34 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
>  		*val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
>  				    | adc->rx_buf[1], 12);
>  		return 0;
> +	case mcp3550_50:
> +	case mcp3550_60:
> +	case mcp3551:
> +	case mcp3553:
> +		/* strip leading Data Ready bit in SPI mode 0,0 */
> +		if (!(adc->spi->mode & SPI_CPOL)) {
> +			adc->rx_buf[0] <<= 1;
> +			adc->rx_buf[0] |= adc->rx_buf[1] >> 7;
> +			adc->rx_buf[1] <<= 1;
> +			adc->rx_buf[1] |= adc->rx_buf[2] >> 7;
> +			adc->rx_buf[2] <<= 1;
> +			adc->rx_buf[2] |= adc->rx_buf[3] >> 7;
> +		}
> +		/*
> +		 * If the input is within -vref and vref, bit 21 is the sign.
> +		 * Up to 12% overrange or underrange are allowed, in which case
> +		 * bit 23 is the sign and bit 0 to 21 is the value.
> +		 */

That is seriously bizarre! Never under estimate the ability of hardware
designers to do crazy things...

> +		overflow = adc->rx_buf[0] >> 6;
> +		if ((!overflow && adc->rx_buf[0] & 0x20) || overflow == 2)
> +			adc->rx_buf[0] |= 0xc0;  /* negative or underrange */
> +		else if (overflow == 1)
> +			adc->rx_buf[0] &= ~0xc0; /* overrange */
> +		else if (overflow == 3)
> +			return -EIO; /* cannot have overrange AND underrange */
> +		*val = sign_extend32(adc->rx_buf[0] << 16 | adc->rx_buf[1] << 8
> +							  | adc->rx_buf[2], 23);
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -288,6 +356,31 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
>  		.num_channels = ARRAY_SIZE(mcp3201_channels),
>  		.resolution = 13
>  	},
> +	[mcp3550_50] = {
> +		.channels = mcp3201_channels,
> +		.num_channels = ARRAY_SIZE(mcp3201_channels),
> +		.resolution = 21,
> +		/* 2% max deviation + 144 clock periods to exit shutdown */
> +		.conv_time = 80000 * 1.02 + 144000 / 102.4,
> +	},
> +	[mcp3550_60] = {
> +		.channels = mcp3201_channels,
> +		.num_channels = ARRAY_SIZE(mcp3201_channels),
> +		.resolution = 21,
> +		.conv_time = 66670 * 1.02 + 144000 / 122.88,
> +	},
> +	[mcp3551] = {
> +		.channels = mcp3201_channels,
> +		.num_channels = ARRAY_SIZE(mcp3201_channels),
> +		.resolution = 21,
> +		.conv_time = 73100 * 1.02 + 144000 / 112.64,
> +	},
> +	[mcp3553] = {
> +		.channels = mcp3201_channels,
> +		.num_channels = ARRAY_SIZE(mcp3201_channels),
> +		.resolution = 21,
> +		.conv_time = 16670 * 1.02 + 144000 / 122.88,

Hmm.  This is interesting.  Floating point in kernel, but with the compiler
evaluating it to an integer constant.  I suppose that is fine if unusual.

> +	},
>  };
>  
>  static int mcp320x_probe(struct spi_device *spi)
> @@ -295,7 +388,7 @@ static int mcp320x_probe(struct spi_device *spi)
>  	struct iio_dev *indio_dev;
>  	struct mcp320x *adc;
>  	const struct mcp320x_chip_info *chip_info;
> -	int ret;
> +	int ret, device_index;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
>  	if (!indio_dev)
> @@ -311,7 +404,8 @@ static int mcp320x_probe(struct spi_device *spi)
>  	indio_dev->info = &mcp320x_info;
>  	spi_set_drvdata(spi, indio_dev);
>  
> -	chip_info = &mcp320x_chip_infos[spi_get_device_id(spi)->driver_data];
> +	device_index = spi_get_device_id(spi)->driver_data;
> +	chip_info = &mcp320x_chip_infos[device_index];
>  	indio_dev->channels = chip_info->channels;
>  	indio_dev->num_channels = chip_info->num_channels;
>  
> @@ -320,7 +414,8 @@ static int mcp320x_probe(struct spi_device *spi)
>  	adc->transfer[0].tx_buf = &adc->tx_buf;
>  	adc->transfer[0].len = sizeof(adc->tx_buf);
>  	adc->transfer[1].rx_buf = adc->rx_buf;
> -	adc->transfer[1].len = sizeof(adc->rx_buf);
> +	adc->transfer[1].len = DIV_ROUND_UP(chip_info->resolution, 8);
> +
>  	if (chip_info->num_channels == 1)
>  		/* single-channel converters are rx only (no MOSI pin) */
>  		spi_message_init_with_transfers(&adc->msg,
> @@ -329,6 +424,26 @@ static int mcp320x_probe(struct spi_device *spi)
>  		spi_message_init_with_transfers(&adc->msg, adc->transfer,
>  						ARRAY_SIZE(adc->transfer));
>  
> +	if (device_index == mcp3550_50 || device_index == mcp3551 ||
> +	    device_index == mcp3550_60 || device_index == mcp3553) {

Once we have a list like this, a switch statement is preferable as it
is easier to read.

> +		unsigned int val;
> +
> +		/* rx len increases from 24 to 25 bit in SPI mode 0,0 */
> +		if (!(spi->mode & SPI_CPOL))
> +			adc->transfer[1].len++;
> +
> +		/* conversions are started by asserting CS pin for 8 usec */
> +		adc->start_conv_transfer.delay_usecs = 8;
> +		spi_message_init_with_transfers(&adc->start_conv_msg,
> +						&adc->start_conv_transfer, 1);
> +		adc->continuous_conv = device_property_read_bool(&spi->dev,
> +					"microchip,continuous-conversion");

Comments on this were in the previous patch...

> +
> +		/* perform two consecutive conversions to unwedge device */
> +		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);
> +		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);

That needs more explanation.  Why would the device need unwedging?

> +	}
> +
>  	adc->reg = devm_regulator_get(&spi->dev, "vref");
>  	if (IS_ERR(adc->reg))
>  		return PTR_ERR(adc->reg);
> @@ -383,6 +498,10 @@ static const struct of_device_id mcp320x_dt_ids[] = {
>  	{ .compatible = "microchip,mcp3204" },
>  	{ .compatible = "microchip,mcp3208" },
>  	{ .compatible = "microchip,mcp3301" },
> +	{ .compatible = "microchip,mcp3550-50" },
> +	{ .compatible = "microchip,mcp3550-60" },
> +	{ .compatible = "microchip,mcp3551" },
> +	{ .compatible = "microchip,mcp3553" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, mcp320x_dt_ids);
> @@ -398,6 +517,10 @@ static const struct spi_device_id mcp320x_id[] = {
>  	{ "mcp3204", mcp3204 },
>  	{ "mcp3208", mcp3208 },
>  	{ "mcp3301", mcp3301 },
> +	{ "mcp3550-50", mcp3550_50 },
> +	{ "mcp3550-60", mcp3550_60 },

>From a kernel point of view do we care if they have 50Hz or 60Hz rejection?
i.e. do we want to represent the two models, or are we better off with
just mcp3350?

> +	{ "mcp3551", mcp3551 },
> +	{ "mcp3553", mcp3553 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, mcp320x_id);
> @@ -414,5 +537,5 @@ static struct spi_driver mcp320x_driver = {
>  module_spi_driver(mcp320x_driver);
>  
>  MODULE_AUTHOR("Oskar Andero <oskar.andero@gmail.com>");
> -MODULE_DESCRIPTION("Microchip Technology MCP3x01/02/04/08");
> +MODULE_DESCRIPTION("Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3");
>  MODULE_LICENSE("GPL v2");

Re: [PATCH 6/6] iio: adc: mcp320x: Add support for mcp3550/1/3

[Lukas Wunner]

On Sun, Sep 03, 2017 at 03:22:39PM +0100, Jonathan Cameron wrote:
> On Tue, 22 Aug 2017 15:33:00 +0200 Lukas Wunner <lukas@wunner.de> wrote:
> > After power-on reset, the chip sometimes fails to perform conversions
> > and clocks out "all ones".  For some reason it can be unwedged by
> > carrying out two consecutive conversions, without any delay between them.
> 
> Put this description inline in the driver.
> 
> > If there is a delay the chip stays wedged.  I tried various other things
> > to no avail, such as extending the conversion time, extending the CS
> > assertion time to start a conversion or clocking out a few bytes.
> > This may be an undocumented bug in the chip's internal state machine,
> > possibly caused by the SPI pins' state on boot of the bcm2837.
> > The driver unconditionally performs these two conversions on ->probe to
> > ascertain all subsequent conversions succeed.  On platforms that support
> > system sleep, it may be necessary to repeat this procedure upon resume.
[snip]
> > +		/* perform two consecutive conversions to unwedge device */
> > +		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);
> > +		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);
> 
> That needs more explanation.  Why would the device need unwedging?

The pin controller on the BCM2835 (Raspberry Pi) by default initializes
all GPIOs to direction "input" with pull down.  Thus, when the machine
boots, the CS pin is either floating or low until the SPI master is
initialized, which happens several seconds after power-on.  (I recall
seeing on the oscilloscope that CS was initially low and then jumped
to high on SPI master initialization, so the pin is probably not floating
but low on boot.)

Per the spec, if CS is asserted for prolonged time the chip enters
continuous conversion mode.  If CS is then driven high, the chip goes
into shutdown (Fig 5-3, page 22 of the datasheet).

If CS is subsequently driven low again, the chip should wake up and
start a conversion, but sometimes it doesn't and clocks out 0xffffff.
Thus, about 50% of the time the chip would not work after boot.
To me this looks like a bug in the chip's internal state machine.

By accident I've found that performing two conversions without any
delay between them causes the chip to work again.

It's possible to change the initial pin controller configuration
of the BCM2835 by supplying a dt-blob.bin which is consumed by the
firmware.  If I set the initial configuration of the CS pin to output /
active-low, the chip *always* works on boot.  However it seems fragile
to depend on proper pin configuration, hence I've decided to always
reset the chip on ->probe using the two conversion magic sequence.

I will expand the comment as requested.

These pin issues suggest that switching back and forth between single
conversion and continuous conversion may not work as expected.  The chip
is hairy and I'm glad that I have it working reliably now in single
conversion mode.  The boards I have here aren't designed to run it in
continuous conversion mode, so I can't really test that.  I've moved
support for it to a separate commit and will mark it informational when
posting, that commit is not intended for merging but merely to get others
started who have a suitable board.


> > @@ -398,6 +517,10 @@ static const struct spi_device_id mcp320x_id[] = {
> >  	{ "mcp3204", mcp3204 },
> >  	{ "mcp3208", mcp3208 },
> >  	{ "mcp3301", mcp3301 },
> > +	{ "mcp3550-50", mcp3550_50 },
> > +	{ "mcp3550-60", mcp3550_60 },
> 
> From a kernel point of view do we care if they have 50Hz or 60Hz rejection?
> i.e. do we want to represent the two models, or are we better off with
> just mcp3350?

The 50 Hz and 60 Hz versions have different conversion times, hence the
distinction.

Thanks a lot for the review,

Lukas

Re: [PATCH 6/6] iio: adc: mcp320x: Add support for mcp3550/1/3

[Jonathan Cameron]

On Thu, 7 Sep 2017 08:44:50 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Sun, Sep 03, 2017 at 03:22:39PM +0100, Jonathan Cameron wrote:
> > On Tue, 22 Aug 2017 15:33:00 +0200 Lukas Wunner <lukas@wunner.de> wrote:  
> > > After power-on reset, the chip sometimes fails to perform conversions
> > > and clocks out "all ones".  For some reason it can be unwedged by
> > > carrying out two consecutive conversions, without any delay between them.  
> > 
> > Put this description inline in the driver.
> >   
> > > If there is a delay the chip stays wedged.  I tried various other things
> > > to no avail, such as extending the conversion time, extending the CS
> > > assertion time to start a conversion or clocking out a few bytes.
> > > This may be an undocumented bug in the chip's internal state machine,
> > > possibly caused by the SPI pins' state on boot of the bcm2837.
> > > The driver unconditionally performs these two conversions on ->probe to
> > > ascertain all subsequent conversions succeed.  On platforms that support
> > > system sleep, it may be necessary to repeat this procedure upon resume.  
> [snip]
> > > +		/* perform two consecutive conversions to unwedge device */
> > > +		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);
> > > +		mcp320x_adc_conversion(adc, 0, 1, device_index, &val);  
> > 
> > That needs more explanation.  Why would the device need unwedging?  
> 
> The pin controller on the BCM2835 (Raspberry Pi) by default initializes
> all GPIOs to direction "input" with pull down.  Thus, when the machine
> boots, the CS pin is either floating or low until the SPI master is
> initialized, which happens several seconds after power-on.  (I recall
> seeing on the oscilloscope that CS was initially low and then jumped
> to high on SPI master initialization, so the pin is probably not floating
> but low on boot.)
> 
> Per the spec, if CS is asserted for prolonged time the chip enters
> continuous conversion mode.  If CS is then driven high, the chip goes
> into shutdown (Fig 5-3, page 22 of the datasheet).
> 
> If CS is subsequently driven low again, the chip should wake up and
> start a conversion, but sometimes it doesn't and clocks out 0xffffff.
> Thus, about 50% of the time the chip would not work after boot.
> To me this looks like a bug in the chip's internal state machine.
> 
> By accident I've found that performing two conversions without any
> delay between them causes the chip to work again.
> 
> It's possible to change the initial pin controller configuration
> of the BCM2835 by supplying a dt-blob.bin which is consumed by the
> firmware.  If I set the initial configuration of the CS pin to output /
> active-low, the chip *always* works on boot.  However it seems fragile
> to depend on proper pin configuration, hence I've decided to always
> reset the chip on ->probe using the two conversion magic sequence.

It is certainly preferable to not have to rely on correct initialization
but there are numerous cases where it is true with other hardware.

Good to put a workaround in place if one is known though - like here!
> 
> I will expand the comment as requested.

Great, will be good to not lose the hard work you've done to come
up with a workaround!

> 
> These pin issues suggest that switching back and forth between single
> conversion and continuous conversion may not work as expected.  The chip
> is hairy and I'm glad that I have it working reliably now in single
> conversion mode.  The boards I have here aren't designed to run it in
> continuous conversion mode, so I can't really test that.  I've moved
> support for it to a separate commit and will mark it informational when
> posting, that commit is not intended for merging but merely to get others
> started who have a suitable board.
> 
> 
> > > @@ -398,6 +517,10 @@ static const struct spi_device_id mcp320x_id[] = {
> > >  	{ "mcp3204", mcp3204 },
> > >  	{ "mcp3208", mcp3208 },
> > >  	{ "mcp3301", mcp3301 },
> > > +	{ "mcp3550-50", mcp3550_50 },
> > > +	{ "mcp3550-60", mcp3550_60 },  
> > 
> > From a kernel point of view do we care if they have 50Hz or 60Hz rejection?
> > i.e. do we want to represent the two models, or are we better off with
> > just mcp3350?  
> 
> The 50 Hz and 60 Hz versions have different conversion times, hence the
> distinction.
> 
> Thanks a lot for the review,
> 
> Lukas

[PATCH 4/6] iio: adc: mcp320x: Drop unnecessary of_device_id attributes

[Lukas Wunner]

The driver sets a .data pointer for each .compatible string but never
calls of_device_get_match_data().  Instead, ADC properties are looked up
with spi_get_device_id().  The .data pointer is therefore unnecessary,
so drop it.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/iio/adc/mcp320x.c | 75 ++++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 56 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 790d3e857c80..6e10fa934aca 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -365,62 +365,25 @@ static int mcp320x_remove(struct spi_device *spi)
 #if defined(CONFIG_OF)
 static const struct of_device_id mcp320x_dt_ids[] = {
 	/* NOTE: The use of compatibles with no vendor prefix is deprecated. */
-	{
-		.compatible = "mcp3001",
-		.data = &mcp320x_chip_infos[mcp3001],
-	}, {
-		.compatible = "mcp3002",
-		.data = &mcp320x_chip_infos[mcp3002],
-	}, {
-		.compatible = "mcp3004",
-		.data = &mcp320x_chip_infos[mcp3004],
-	}, {
-		.compatible = "mcp3008",
-		.data = &mcp320x_chip_infos[mcp3008],
-	}, {
-		.compatible = "mcp3201",
-		.data = &mcp320x_chip_infos[mcp3201],
-	}, {
-		.compatible = "mcp3202",
-		.data = &mcp320x_chip_infos[mcp3202],
-	}, {
-		.compatible = "mcp3204",
-		.data = &mcp320x_chip_infos[mcp3204],
-	}, {
-		.compatible = "mcp3208",
-		.data = &mcp320x_chip_infos[mcp3208],
-	}, {
-		.compatible = "mcp3301",
-		.data = &mcp320x_chip_infos[mcp3301],
-	}, {
-		.compatible = "microchip,mcp3001",
-		.data = &mcp320x_chip_infos[mcp3001],
-	}, {
-		.compatible = "microchip,mcp3002",
-		.data = &mcp320x_chip_infos[mcp3002],
-	}, {
-		.compatible = "microchip,mcp3004",
-		.data = &mcp320x_chip_infos[mcp3004],
-	}, {
-		.compatible = "microchip,mcp3008",
-		.data = &mcp320x_chip_infos[mcp3008],
-	}, {
-		.compatible = "microchip,mcp3201",
-		.data = &mcp320x_chip_infos[mcp3201],
-	}, {
-		.compatible = "microchip,mcp3202",
-		.data = &mcp320x_chip_infos[mcp3202],
-	}, {
-		.compatible = "microchip,mcp3204",
-		.data = &mcp320x_chip_infos[mcp3204],
-	}, {
-		.compatible = "microchip,mcp3208",
-		.data = &mcp320x_chip_infos[mcp3208],
-	}, {
-		.compatible = "microchip,mcp3301",
-		.data = &mcp320x_chip_infos[mcp3301],
-	}, {
-	}
+	{ .compatible = "mcp3001" },
+	{ .compatible = "mcp3002" },
+	{ .compatible = "mcp3004" },
+	{ .compatible = "mcp3008" },
+	{ .compatible = "mcp3201" },
+	{ .compatible = "mcp3202" },
+	{ .compatible = "mcp3204" },
+	{ .compatible = "mcp3208" },
+	{ .compatible = "mcp3301" },
+	{ .compatible = "microchip,mcp3001" },
+	{ .compatible = "microchip,mcp3002" },
+	{ .compatible = "microchip,mcp3004" },
+	{ .compatible = "microchip,mcp3008" },
+	{ .compatible = "microchip,mcp3201" },
+	{ .compatible = "microchip,mcp3202" },
+	{ .compatible = "microchip,mcp3204" },
+	{ .compatible = "microchip,mcp3208" },
+	{ .compatible = "microchip,mcp3301" },
+	{ }
 };
 MODULE_DEVICE_TABLE(of, mcp320x_dt_ids);
 #endif
-- 
2.11.0

Re: [PATCH 4/6] iio: adc: mcp320x: Drop unnecessary of_device_id attributes

[Jonathan Cameron]

On Tue, 22 Aug 2017 15:33:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> The driver sets a .data pointer for each .compatible string but never
> calls of_device_get_match_data().  Instead, ADC properties are looked up
> with spi_get_device_id().  The .data pointer is therefore unnecessary,
> so drop it.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Hmm. The use of spi_get_device_id strikes me as fragile but I may be missing
something.  Seems to be a match on the name from devicetree against the older
table.

Can't disagree that your patch removes currently unused code though.

Jonathan
> ---
>  drivers/iio/adc/mcp320x.c | 75 ++++++++++++-----------------------------------
>  1 file changed, 19 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 790d3e857c80..6e10fa934aca 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -365,62 +365,25 @@ static int mcp320x_remove(struct spi_device *spi)
>  #if defined(CONFIG_OF)
>  static const struct of_device_id mcp320x_dt_ids[] = {
>  	/* NOTE: The use of compatibles with no vendor prefix is deprecated. */
> -	{
> -		.compatible = "mcp3001",
> -		.data = &mcp320x_chip_infos[mcp3001],
> -	}, {
> -		.compatible = "mcp3002",
> -		.data = &mcp320x_chip_infos[mcp3002],
> -	}, {
> -		.compatible = "mcp3004",
> -		.data = &mcp320x_chip_infos[mcp3004],
> -	}, {
> -		.compatible = "mcp3008",
> -		.data = &mcp320x_chip_infos[mcp3008],
> -	}, {
> -		.compatible = "mcp3201",
> -		.data = &mcp320x_chip_infos[mcp3201],
> -	}, {
> -		.compatible = "mcp3202",
> -		.data = &mcp320x_chip_infos[mcp3202],
> -	}, {
> -		.compatible = "mcp3204",
> -		.data = &mcp320x_chip_infos[mcp3204],
> -	}, {
> -		.compatible = "mcp3208",
> -		.data = &mcp320x_chip_infos[mcp3208],
> -	}, {
> -		.compatible = "mcp3301",
> -		.data = &mcp320x_chip_infos[mcp3301],
> -	}, {
> -		.compatible = "microchip,mcp3001",
> -		.data = &mcp320x_chip_infos[mcp3001],
> -	}, {
> -		.compatible = "microchip,mcp3002",
> -		.data = &mcp320x_chip_infos[mcp3002],
> -	}, {
> -		.compatible = "microchip,mcp3004",
> -		.data = &mcp320x_chip_infos[mcp3004],
> -	}, {
> -		.compatible = "microchip,mcp3008",
> -		.data = &mcp320x_chip_infos[mcp3008],
> -	}, {
> -		.compatible = "microchip,mcp3201",
> -		.data = &mcp320x_chip_infos[mcp3201],
> -	}, {
> -		.compatible = "microchip,mcp3202",
> -		.data = &mcp320x_chip_infos[mcp3202],
> -	}, {
> -		.compatible = "microchip,mcp3204",
> -		.data = &mcp320x_chip_infos[mcp3204],
> -	}, {
> -		.compatible = "microchip,mcp3208",
> -		.data = &mcp320x_chip_infos[mcp3208],
> -	}, {
> -		.compatible = "microchip,mcp3301",
> -		.data = &mcp320x_chip_infos[mcp3301],
> -	}, {
> -	}
> +	{ .compatible = "mcp3001" },
> +	{ .compatible = "mcp3002" },
> +	{ .compatible = "mcp3004" },
> +	{ .compatible = "mcp3008" },
> +	{ .compatible = "mcp3201" },
> +	{ .compatible = "mcp3202" },
> +	{ .compatible = "mcp3204" },
> +	{ .compatible = "mcp3208" },
> +	{ .compatible = "mcp3301" },
> +	{ .compatible = "microchip,mcp3001" },
> +	{ .compatible = "microchip,mcp3002" },
> +	{ .compatible = "microchip,mcp3004" },
> +	{ .compatible = "microchip,mcp3008" },
> +	{ .compatible = "microchip,mcp3201" },
> +	{ .compatible = "microchip,mcp3202" },
> +	{ .compatible = "microchip,mcp3204" },
> +	{ .compatible = "microchip,mcp3208" },
> +	{ .compatible = "microchip,mcp3301" },
> +	{ }
>  };
>  MODULE_DEVICE_TABLE(of, mcp320x_dt_ids);
>  #endif

[PATCH 3/6] iio: adc: mcp320x: Speed up readout of single-channel ADCs

[Lukas Wunner]

Single-channel converters such as mcp3001, mcp3201, mcp3301 and the
upcoming mcp3550/1/3 lack a MOSI pin, so there's no need to call
mcp320x_channel_to_tx_data() for them.

Moreover, instead of calling spi_read() for these converters, which
generates an spi_message and spi_transfer on the stack on every readout,
it's more efficient to use the spi_message and spi_transfer[] included
in struct mcp320x (as we do for multi-channel ADCs), but initialize the
spi_message only with the receive transfer.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/iio/adc/mcp320x.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 071dd23a33d9..790d3e857c80 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -78,10 +78,6 @@ static int mcp320x_channel_to_tx_data(int device_index,
 	int start_bit = 1;
 
 	switch (device_index) {
-	case mcp3001:
-	case mcp3201:
-	case mcp3301:
-		return 0;
 	case mcp3002:
 	case mcp3202:
 		return ((start_bit << 4) | (!differential << 3) |
@@ -104,18 +100,13 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 
 	adc->rx_buf[0] = 0;
 	adc->rx_buf[1] = 0;
-	adc->tx_buf = mcp320x_channel_to_tx_data(device_index,
-						channel, differential);
+	if (adc->chip_info->num_channels > 1)
+		adc->tx_buf = mcp320x_channel_to_tx_data(device_index, channel,
+							 differential);
 
-	if (device_index != mcp3001 && device_index != mcp3201 && device_index != mcp3301) {
-		ret = spi_sync(adc->spi, &adc->msg);
-		if (ret < 0)
-			return ret;
-	} else {
-		ret = spi_read(adc->spi, &adc->rx_buf, sizeof(adc->rx_buf));
-		if (ret < 0)
-			return ret;
-	}
+	ret = spi_sync(adc->spi, &adc->msg);
+	if (ret < 0)
+		return ret;
 
 	switch (device_index) {
 	case mcp3001:
@@ -330,9 +321,13 @@ static int mcp320x_probe(struct spi_device *spi)
 	adc->transfer[0].len = sizeof(adc->tx_buf);
 	adc->transfer[1].rx_buf = adc->rx_buf;
 	adc->transfer[1].len = sizeof(adc->rx_buf);
-
-	spi_message_init_with_transfers(&adc->msg, adc->transfer,
-					ARRAY_SIZE(adc->transfer));
+	if (chip_info->num_channels == 1)
+		/* single-channel converters are rx only (no MOSI pin) */
+		spi_message_init_with_transfers(&adc->msg,
+						&adc->transfer[1], 1);
+	else
+		spi_message_init_with_transfers(&adc->msg, adc->transfer,
+						ARRAY_SIZE(adc->transfer));
 
 	adc->reg = devm_regulator_get(&spi->dev, "vref");
 	if (IS_ERR(adc->reg))
-- 
2.11.0

Re: [PATCH 3/6] iio: adc: mcp320x: Speed up readout of single-channel ADCs

[Jonathan Cameron]

On Tue, 22 Aug 2017 15:33:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Single-channel converters such as mcp3001, mcp3201, mcp3301 and the
> upcoming mcp3550/1/3 lack a MOSI pin, so there's no need to call
> mcp320x_channel_to_tx_data() for them.
> 
> Moreover, instead of calling spi_read() for these converters, which
> generates an spi_message and spi_transfer on the stack on every readout,
> it's more efficient to use the spi_message and spi_transfer[] included
> in struct mcp320x (as we do for multi-channel ADCs), but initialize the
> spi_message only with the receive transfer.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Looks good, but I'll hold this for now as the fixes will not show up
in my upstream tree for a least 3-4 weeks.

Please give me a bump if it looks like I have forgotten it, or include
it in updated versions of the rest of the series as appropriate.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/mcp320x.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 071dd23a33d9..790d3e857c80 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -78,10 +78,6 @@ static int mcp320x_channel_to_tx_data(int device_index,
>  	int start_bit = 1;
>  
>  	switch (device_index) {
> -	case mcp3001:
> -	case mcp3201:
> -	case mcp3301:
> -		return 0;
>  	case mcp3002:
>  	case mcp3202:
>  		return ((start_bit << 4) | (!differential << 3) |
> @@ -104,18 +100,13 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
>  
>  	adc->rx_buf[0] = 0;
>  	adc->rx_buf[1] = 0;
> -	adc->tx_buf = mcp320x_channel_to_tx_data(device_index,
> -						channel, differential);
> +	if (adc->chip_info->num_channels > 1)
> +		adc->tx_buf = mcp320x_channel_to_tx_data(device_index, channel,
> +							 differential);
>  
> -	if (device_index != mcp3001 && device_index != mcp3201 && device_index != mcp3301) {
> -		ret = spi_sync(adc->spi, &adc->msg);
> -		if (ret < 0)
> -			return ret;
> -	} else {
> -		ret = spi_read(adc->spi, &adc->rx_buf, sizeof(adc->rx_buf));
> -		if (ret < 0)
> -			return ret;
> -	}
> +	ret = spi_sync(adc->spi, &adc->msg);
> +	if (ret < 0)
> +		return ret;
>  
>  	switch (device_index) {
>  	case mcp3001:
> @@ -330,9 +321,13 @@ static int mcp320x_probe(struct spi_device *spi)
>  	adc->transfer[0].len = sizeof(adc->tx_buf);
>  	adc->transfer[1].rx_buf = adc->rx_buf;
>  	adc->transfer[1].len = sizeof(adc->rx_buf);
> -
> -	spi_message_init_with_transfers(&adc->msg, adc->transfer,
> -					ARRAY_SIZE(adc->transfer));
> +	if (chip_info->num_channels == 1)
> +		/* single-channel converters are rx only (no MOSI pin) */
> +		spi_message_init_with_transfers(&adc->msg,
> +						&adc->transfer[1], 1);
> +	else
> +		spi_message_init_with_transfers(&adc->msg, adc->transfer,
> +						ARRAY_SIZE(adc->transfer));
>  
>  	adc->reg = devm_regulator_get(&spi->dev, "vref");
>  	if (IS_ERR(adc->reg))

[PATCH 1/6] iio: adc: mcp320x: Fix oops on module unload

[Lukas Wunner]

The driver calls spi_get_drvdata() in its ->remove hook even though it
has never called spi_set_drvdata().  Stack trace for posterity:

Unable to handle kernel NULL pointer dereference at virtual address 00000220
Internal error: Oops: 5 [#1] SMP ARM
[<8072f564>] (mutex_lock) from [<7f1400d0>] (iio_device_unregister+0x24/0x7c [industrialio])
[<7f1400d0>] (iio_device_unregister [industrialio]) from [<7f15e020>] (mcp320x_remove+0x20/0x30 [mcp320x])
[<7f15e020>] (mcp320x_remove [mcp320x]) from [<8055a8cc>] (spi_drv_remove+0x2c/0x44)
[<8055a8cc>] (spi_drv_remove) from [<805087bc>] (__device_release_driver+0x98/0x134)
[<805087bc>] (__device_release_driver) from [<80509180>] (driver_detach+0xdc/0xe0)
[<80509180>] (driver_detach) from [<8050823c>] (bus_remove_driver+0x5c/0xb0)
[<8050823c>] (bus_remove_driver) from [<80509ab0>] (driver_unregister+0x38/0x58)
[<80509ab0>] (driver_unregister) from [<7f15e69c>] (mcp320x_driver_exit+0x14/0x1c [mcp320x])
[<7f15e69c>] (mcp320x_driver_exit [mcp320x]) from [<801a78d0>] (SyS_delete_module+0x184/0x1d0)
[<801a78d0>] (SyS_delete_module) from [<80108100>] (ret_fast_syscall+0x0/0x1c)

Fixes: f5ce4a7a9291 ("iio: adc: add driver for MCP3204/08 12-bit ADC")
Cc: Oskar Andero <oskar.andero@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/iio/adc/mcp320x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 634717ae12f3..45d043c9a888 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -312,6 +312,7 @@ static int mcp320x_probe(struct spi_device *spi)
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &mcp320x_info;
+	spi_set_drvdata(spi, indio_dev);
 
 	chip_info = &mcp320x_chip_infos[spi_get_device_id(spi)->driver_data];
 	indio_dev->channels = chip_info->channels;
-- 
2.11.0

Re: [PATCH 1/6] iio: adc: mcp320x: Fix oops on module unload

[Jonathan Cameron]

On Tue, 22 Aug 2017 15:33:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> The driver calls spi_get_drvdata() in its ->remove hook even though it
> has never called spi_set_drvdata().  Stack trace for posterity:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000220
> Internal error: Oops: 5 [#1] SMP ARM
> [<8072f564>] (mutex_lock) from [<7f1400d0>] (iio_device_unregister+0x24/0x7c [industrialio])
> [<7f1400d0>] (iio_device_unregister [industrialio]) from [<7f15e020>] (mcp320x_remove+0x20/0x30 [mcp320x])
> [<7f15e020>] (mcp320x_remove [mcp320x]) from [<8055a8cc>] (spi_drv_remove+0x2c/0x44)
> [<8055a8cc>] (spi_drv_remove) from [<805087bc>] (__device_release_driver+0x98/0x134)
> [<805087bc>] (__device_release_driver) from [<80509180>] (driver_detach+0xdc/0xe0)
> [<80509180>] (driver_detach) from [<8050823c>] (bus_remove_driver+0x5c/0xb0)
> [<8050823c>] (bus_remove_driver) from [<80509ab0>] (driver_unregister+0x38/0x58)
> [<80509ab0>] (driver_unregister) from [<7f15e69c>] (mcp320x_driver_exit+0x14/0x1c [mcp320x])
> [<7f15e69c>] (mcp320x_driver_exit [mcp320x]) from [<801a78d0>] (SyS_delete_module+0x184/0x1d0)
> [<801a78d0>] (SyS_delete_module) from [<80108100>] (ret_fast_syscall+0x0/0x1c)
> 
> Fixes: f5ce4a7a9291 ("iio: adc: add driver for MCP3204/08 12-bit ADC")
> Cc: Oskar Andero <oskar.andero@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to the fixes-togreg branch of iio.git and marked for stable.

This won't now hit until after the merge window, but it should be fairly
soon after that.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/mcp320x.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 634717ae12f3..45d043c9a888 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -312,6 +312,7 @@ static int mcp320x_probe(struct spi_device *spi)
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &mcp320x_info;
> +	spi_set_drvdata(spi, indio_dev);
>  
>  	chip_info = &mcp320x_chip_infos[spi_get_device_id(spi)->driver_data];
>  	indio_dev->channels = chip_info->channels;

[PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Lukas Wunner]

All chips supported by this driver clock data out on the falling edge
and latch data in on the rising edge, hence SPI mode (0,0) or (1,1)
must be used.

Furthermore, none of the chips has an internal reference voltage
regulator, so an external supply is always required and needs to be
specified in the device tree lest the IIO "scale" in sysfs cannot be
calculated.

Document these requirements in the device tree binding, together with
the new "microchip,continuous-conversion" property added specifically
for the mcp3550/1/3.

Cc: Mathias Duckeck <m.duckeck@kunbus.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 .../devicetree/bindings/iio/adc/mcp320x.txt        | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
index bcd3ac8e6e0c..cf28af9ec0ac 100644
--- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
+++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
@@ -29,15 +29,38 @@ Required properties:
 				"microchip,mcp3204"
 				"microchip,mcp3208"
 				"microchip,mcp3301"
+				"microchip,mcp3550-50"
+				"microchip,mcp3550-60"
+				"microchip,mcp3551"
+				"microchip,mcp3553"
 
 			NOTE: The use of the compatibles with no vendor prefix
 			is deprecated and only listed because old DT use them.
 
+	- spi-cpha, spi-cpol (boolean):
+			Either SPI mode (0,0) or (1,1) must be used, so specify
+			none or both of spi-cpha, spi-cpol.  The MCP3550/1/3
+			is more efficient in mode (1,1) as only 3 instead of
+			4 bytes need to be read from the ADC, but not all SPI
+			masters support it.
+
+	- vref-supply:	Phandle to the external reference voltage supply.
+
+Optional properties:
+	- microchip,continuous-conversion (boolean):
+			Only applicable to MCP3550/1/3:  These ADCs have long
+			conversion times and therefore support "continuous
+			conversion mode" to allow retrieval of conversions
+			at any time without observing a delay.  The mode is
+			enabled by permanently driving CS low, e.g. by wiring
+			it to ground.
+
 Examples:
 spi_controller {
 	mcp3x0x@0 {
 		compatible = "mcp3002";
 		reg = <0>;
 		spi-max-frequency = <1000000>;
+		vref-supply = <&vref_reg>;
 	};
 };
-- 
2.11.0

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Rob Herring]

On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:
> All chips supported by this driver clock data out on the falling edge
> and latch data in on the rising edge, hence SPI mode (0,0) or (1,1)
> must be used.
> 
> Furthermore, none of the chips has an internal reference voltage
> regulator, so an external supply is always required and needs to be
> specified in the device tree lest the IIO "scale" in sysfs cannot be
> calculated.
> 
> Document these requirements in the device tree binding, together with
> the new "microchip,continuous-conversion" property added specifically
> for the mcp3550/1/3.
> 
> Cc: Mathias Duckeck <m.duckeck@kunbus.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  .../devicetree/bindings/iio/adc/mcp320x.txt        | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> index bcd3ac8e6e0c..cf28af9ec0ac 100644
> --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> @@ -29,15 +29,38 @@ Required properties:
>  				"microchip,mcp3204"
>  				"microchip,mcp3208"
>  				"microchip,mcp3301"
> +				"microchip,mcp3550-50"
> +				"microchip,mcp3550-60"
> +				"microchip,mcp3551"
> +				"microchip,mcp3553"
>  
>  			NOTE: The use of the compatibles with no vendor prefix
>  			is deprecated and only listed because old DT use them.
>  
> +	- spi-cpha, spi-cpol (boolean):
> +			Either SPI mode (0,0) or (1,1) must be used, so specify
> +			none or both of spi-cpha, spi-cpol.  The MCP3550/1/3
> +			is more efficient in mode (1,1) as only 3 instead of
> +			4 bytes need to be read from the ADC, but not all SPI
> +			masters support it.
> +
> +	- vref-supply:	Phandle to the external reference voltage supply.
> +
> +Optional properties:
> +	- microchip,continuous-conversion (boolean):

Second binding I have seen today with a continuous property. Make this 
common (or maybe we already have one).

> +			Only applicable to MCP3550/1/3:  These ADCs have long
> +			conversion times and therefore support "continuous
> +			conversion mode" to allow retrieval of conversions
> +			at any time without observing a delay.  The mode is
> +			enabled by permanently driving CS low, e.g. by wiring
> +			it to ground.
> +
>  Examples:
>  spi_controller {
>  	mcp3x0x@0 {
>  		compatible = "mcp3002";
>  		reg = <0>;
>  		spi-max-frequency = <1000000>;
> +		vref-supply = <&vref_reg>;
>  	};
>  };
> -- 
> 2.11.0
> 

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Lukas Wunner]

On Fri, Aug 25, 2017 at 02:59:41PM -0500, Rob Herring wrote:
> On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:
> > --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> > +Optional properties:
> > +	- microchip,continuous-conversion (boolean):
> > +			Only applicable to MCP3550/1/3:  These ADCs have long
> > +			conversion times and therefore support "continuous
> > +			conversion mode" to allow retrieval of conversions
> > +			at any time without observing a delay.  The mode is
> > +			enabled by permanently driving CS low, e.g. by wiring
> > +			it to ground.
> 
> Second binding I have seen today with a continuous property. Make this 
> common (or maybe we already have one).

The other one was the TI LMP92001 ADC driver posted by Abhisit Sangjan
(cc), however looking at the datasheet of that chip reveals that
continuous versus one-shot mode is selected by flipping a bit in the
chip's register map.

So it is configurable at run-time.  It's not something that's hardwired.
(Which is the case with the MCP3550 in my patch.)

My understanding was that run-time configurable options should not be
listed in the device-tree at all, only hardware features.  If that is
correct then that device-tree property should be dropped from Abhisit
Sangjan's patch.  Configuring the feature via sysfs is fine I guess.

However we do have another driver supporting continuous versus one-shot
mode and that is drivers/iio/light/us5182d.c by Adriana Reus (cc).
The feature was added with commit c3304c212326.  I'm not sure if it's
hardwired or runtime-configurable, the datasheet is gone from the
manufacturer's website.

I agree that a common "continuous" property makes sense.  We haven't
defined any common IIO properties so far and that has already led to
inconsistencies.  E.g. most ADC/DAC drivers name the reference regulator
"vref-supply", but e.g. drivers/iio/dac/ad7303.c calls it "REF-supply".

What do you think of this:

-- >8 --
Subject: [PATCH] dt-bindings: iio: Document common properties

It's about time we standardize on common names for frequently used IIO
properties.  For starters, document "vref-supply" and "continuous".

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 Documentation/devicetree/bindings/iio/iio-bindings.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
index 68d6f8c..c3e87e15 100644
--- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
+++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
@@ -95,3 +95,18 @@ vdd channel is connected to output 0 of the &ref device.
 		io-channels = <&adc 10>, <&adc 11>;
 		io-channel-names = "adc1", "adc2";
 	};
+
+==Common IIO properties==
+
+Reference voltage:
+ADCs, DACs and several other IIO devices require a reference voltage.
+By convention the property specifying this regulator is named "vref-supply".
+If the chip lacks a dedicated Vref pin and instead uses its own power supply
+as reference, the property specifying the regulator is commonly named
+"vdd-supply" or "vcc-supply".
+
+Continuous mode:
+Some sensors can be configured to perform continuous (versus one-shot)
+measurements.  Continuous mode may require more energy in return for faster
+or more reliable measurements.  A boolean property named "continuous"
+signifies that the device is configured for this mode.
-- 
2.11.0

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Adriana Reus]

Hi,

On Sun, Aug 27, 2017 at 6:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Fri, Aug 25, 2017 at 02:59:41PM -0500, Rob Herring wrote:
>> On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:
>> > --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
>> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
>> > +Optional properties:
>> > +   - microchip,continuous-conversion (boolean):
>> > +                   Only applicable to MCP3550/1/3:  These ADCs have long
>> > +                   conversion times and therefore support "continuous
>> > +                   conversion mode" to allow retrieval of conversions
>> > +                   at any time without observing a delay.  The mode is
>> > +                   enabled by permanently driving CS low, e.g. by wiring
>> > +                   it to ground.
>>
>> Second binding I have seen today with a continuous property. Make this
>> common (or maybe we already have one).
>
> The other one was the TI LMP92001 ADC driver posted by Abhisit Sangjan
> (cc), however looking at the datasheet of that chip reveals that
> continuous versus one-shot mode is selected by flipping a bit in the
> chip's register map.
>
> So it is configurable at run-time.  It's not something that's hardwired.
> (Which is the case with the MCP3550 in my patch.)
>
> My understanding was that run-time configurable options should not be
> listed in the device-tree at all, only hardware features.  If that is
> correct then that device-tree property should be dropped from Abhisit
> Sangjan's patch.  Configuring the feature via sysfs is fine I guess.
>
> However we do have another driver supporting continuous versus one-shot
> mode and that is drivers/iio/light/us5182d.c by Adriana Reus (cc).
> The feature was added with commit c3304c212326.  I'm not sure if it's
> hardwired or runtime-configurable, the datasheet is gone from the
> manufacturer's website.
For the us5182 continuous versus one-shot is also a configuration in the
chip's registers. I think it would be fine also as a sysfs property instead.
>
> I agree that a common "continuous" property makes sense.  We haven't
> defined any common IIO properties so far and that has already led to
> inconsistencies.  E.g. most ADC/DAC drivers name the reference regulator
> "vref-supply", but e.g. drivers/iio/dac/ad7303.c calls it "REF-supply".
>
> What do you think of this:
>
> -- >8 --
> Subject: [PATCH] dt-bindings: iio: Document common properties
>
> It's about time we standardize on common names for frequently used IIO
> properties.  For starters, document "vref-supply" and "continuous".
>
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  Documentation/devicetree/bindings/iio/iio-bindings.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> index 68d6f8c..c3e87e15 100644
> --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -95,3 +95,18 @@ vdd channel is connected to output 0 of the &ref device.
>                 io-channels = <&adc 10>, <&adc 11>;
>                 io-channel-names = "adc1", "adc2";
>         };
> +
> +==Common IIO properties==
> +
> +Reference voltage:
> +ADCs, DACs and several other IIO devices require a reference voltage.
> +By convention the property specifying this regulator is named "vref-supply".
> +If the chip lacks a dedicated Vref pin and instead uses its own power supply
> +as reference, the property specifying the regulator is commonly named
> +"vdd-supply" or "vcc-supply".
> +
> +Continuous mode:
> +Some sensors can be configured to perform continuous (versus one-shot)
> +measurements.  Continuous mode may require more energy in return for faster
> +or more reliable measurements.  A boolean property named "continuous"
> +signifies that the device is configured for this mode.
> --
> 2.11.0

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Jonathan Cameron]

On Tue, 29 Aug 2017 10:21:13 +0300
Adriana Reus <adi.reus@gmail.com> wrote:

+cc Mark Brown and linux-spi to address question about how to represent
a hardwired chip select.  See below for why I think that is what we should
be representing rather than the fact it puts it in 'continuous mode'.

> Hi,
> 
> On Sun, Aug 27, 2017 at 6:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, Aug 25, 2017 at 02:59:41PM -0500, Rob Herring wrote:  
> >> On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:  
> >> > --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> >> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> >> > +Optional properties:
> >> > +   - microchip,continuous-conversion (boolean):
> >> > +                   Only applicable to MCP3550/1/3:  These ADCs have long
> >> > +                   conversion times and therefore support "continuous
> >> > +                   conversion mode" to allow retrieval of conversions
> >> > +                   at any time without observing a delay.  The mode is
> >> > +                   enabled by permanently driving CS low, e.g. by wiring
> >> > +                   it to ground.  

hmm.  This is odd.  We probably need to make the SPI subsystem aware of this.
It is possible to ask for exclusive use of an SPI bus and I think we should
be doing this here.  It may be wired low on your board, but it may be wired to
a controllable chip select on other boards and we can still force it low
to trigger this mode if it makes sense for the current application.
Datasheet says that this mode kicks in whenever we don't raise the cs rather
than it being a one time thing.

So I'd argue what we actually need to represent here is that the CS line
is not controllable.  What this means to the driver should be handled
in the driver - ideally also dealing with the case where it is controllable
appropriately (via exclusive bus usage).  Spi devices have the SPI_NO_CS
bit in the mode member of the spi device but I'm not sure about bindings.

Mark or other SPI people (cc'd) any thoughts on how to handle this cleanly?

> >>
> >> Second binding I have seen today with a continuous property. Make this
> >> common (or maybe we already have one).  
> >
> > The other one was the TI LMP92001 ADC driver posted by Abhisit Sangjan
> > (cc), however looking at the datasheet of that chip reveals that
> > continuous versus one-shot mode is selected by flipping a bit in the
> > chip's register map.
> >
> > So it is configurable at run-time.  It's not something that's hardwired.
> > (Which is the case with the MCP3550 in my patch.)

For the runtime configurable ones, the vast majority of the time this should
not be exposed to userspace at all.  It's a fairly esoteric option and
exactly what it means varies hugely between different boards.  Most of the
time userspace has not idea what to do with it - if 'explicitly'
provided as a control.  It might make sense for a particular application
with tightly coupled userspace and kernelspace, but that's not what we
are targeting in IIO.

So it is up to the driver to make sensible decisions based on what is
going on.  Kind of like auto suspend for runtime pm.  

The one case where we normally want to flip to continuous modes is when
we have a chardev access going on to the device.  In IIO that reflects
the fact we are in a push mode rather than userspace polling for new data.

A lot of hardware supports continuous and one shot capture in this fashion!
Pretty much anything with an internal sequencing clock.

> >
> > My understanding was that run-time configurable options should not be
> > listed in the device-tree at all, only hardware features.  If that is
> > correct then that device-tree property should be dropped from Abhisit
> > Sangjan's patch.  Configuring the feature via sysfs is fine I guess.
> >
> > However we do have another driver supporting continuous versus one-shot
> > mode and that is drivers/iio/light/us5182d.c by Adriana Reus (cc).
> > The feature was added with commit c3304c212326.  I'm not sure if it's
> > hardwired or runtime-configurable, the datasheet is gone from the
> > manufacturer's website.  
> For the us5182 continuous versus one-shot is also a configuration in the
> chip's registers. I think it would be fine also as a sysfs property instead.
> >
> > I agree that a common "continuous" property makes sense.  We haven't
> > defined any common IIO properties so far and that has already led to
> > inconsistencies.  E.g. most ADC/DAC drivers name the reference regulator
> > "vref-supply", but e.g. drivers/iio/dac/ad7303.c calls it "REF-supply".
> >
> > What do you think of this:

Personally I don't think we are in a position yet to make this a generic
property - this is the first device where it is actually to do with the
physical circuit (and arguably it isn't really - see above).

It might become common, but I actually think it unlikely. It's odd
to have a device that sits in the sweet spot between being dumb enough to
not make it software configurable and clever enough to implement this
stuff at all.

Reference voltages are an oddity as the supply naming typically should match
that on the datasheet. It's 'fairly' consistent but some devices
have a set of relatively obscure references to different parts of the
input circuitry.  We can document it as a 'default' assuming nothing
strange is going on though.  This is why we have the vagueness below
on VDD and VCC.


> >  
> > -- >8 --  
> > Subject: [PATCH] dt-bindings: iio: Document common properties
> >
> > It's about time we standardize on common names for frequently used IIO
> > properties.  For starters, document "vref-supply" and "continuous".
> >
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  Documentation/devicetree/bindings/iio/iio-bindings.txt | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > index 68d6f8c..c3e87e15 100644
> > --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -95,3 +95,18 @@ vdd channel is connected to output 0 of the &ref device.
> >                 io-channels = <&adc 10>, <&adc 11>;
> >                 io-channel-names = "adc1", "adc2";
> >         };
> > +
> > +==Common IIO properties==
> > +
> > +Reference voltage:
> > +ADCs, DACs and several other IIO devices require a reference voltage.
> > +By convention the property specifying this regulator is named "vref-supply".
> > +If the chip lacks a dedicated Vref pin and instead uses its own power supply
> > +as reference, the property specifying the regulator is commonly named
> > +"vdd-supply" or "vcc-supply".
> > +
> > +Continuous mode:
> > +Some sensors can be configured to perform continuous (versus one-shot)
> > +measurements.  Continuous mode may require more energy in return for faster
> > +or more reliable measurements.  A boolean property named "continuous"
> > +signifies that the device is configured for this mode.
> > --
> > 2.11.0  

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Lukas Wunner]

On Sun, Sep 03, 2017 at 02:37:49PM +0100, Jonathan Cameron wrote:
> On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:  
> > --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> > +Optional properties:
> > +   - microchip,continuous-conversion (boolean):
> > +           Only applicable to MCP3550/1/3:  These ADCs have long
> > +           conversion times and therefore support "continuous
> > +           conversion mode" to allow retrieval of conversions
> > +           at any time without observing a delay.  The mode is
> > +           enabled by permanently driving CS low, e.g. by wiring
> > +           it to ground.  
> 
> hmm.  This is odd.  We probably need to make the SPI subsystem aware
> of this. It is possible to ask for exclusive use of an SPI bus and
> I think we should be doing this here.  It may be wired low on your
> board, but it may be wired to a controllable chip select on other
> boards and we can still force it low to trigger this mode if it makes
> sense for the current application.
> 
> So I'd argue what we actually need to represent here is that the CS line
> is not controllable.  What this means to the driver should be handled
> in the driver - ideally also dealing with the case where it is controllable
> appropriately (via exclusive bus usage).  Spi devices have the SPI_NO_CS
> bit in the mode member of the spi device but I'm not sure about bindings.
[...]
> The one case where we normally want to flip to continuous modes is when
> we have a chardev access going on to the device.  In IIO that reflects
> the fact we are in a push mode rather than userspace polling for new data.

It seems there is no DT binding so far to set SPI_NO_CS.

Conceivably, continuous mode could be used even with multiple devices
on the bus if CLK and MISO is AND-gated with the CS signal coming from
the SPI master.  (And the CS of the ADC is pulled low.)  In that case,
the notion that "continuous mode == CS not controllable" would be
incorrect, hence the approach I've chosen.

On the Revolution Pi we don't use continuous mode.  I merely included
it in the driver for completeness.  If it is too controversial I'd be
inclined to drop the feature.

On-demand switching to continuous mode by keeping CS low would be
possible by setting the cs_change bit of struct mcp320x ->transfer[1],
but that might not work if there are other devices on the bus.


> Personally I don't think we are in a position yet to make this a generic
> property - this is the first device where it is actually to do with the
> physical circuit (and arguably it isn't really - see above).

Okay.


> Reference voltages are an oddity as the supply naming typically should match
> that on the datasheet. It's 'fairly' consistent but some devices
> have a set of relatively obscure references to different parts of the
> input circuitry.  We can document it as a 'default' assuming nothing
> strange is going on though.  This is why we have the vagueness below
> on VDD and VCC.

That is new to me, I believe it's not documented or am I missing something?
I'd be happy to respin the below patch without the "Continuous mode"
portion if you want?  (Amended with the info you gave above.)
Do you think iio-bindings.txt is the right place to put this or would
a separate common.txt be more appropriate? (See e.g. leds/common.txt)

Thanks,

Lukas

> -- >8 --  
> Subject: [PATCH] dt-bindings: iio: Document common properties
>
> It's about time we standardize on common names for frequently used IIO
> properties.  For starters, document "vref-supply" and "continuous".
>
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  Documentation/devicetree/bindings/iio/iio-bindings.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> index 68d6f8c..c3e87e15 100644
> --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -95,3 +95,18 @@ vdd channel is connected to output 0 of the &ref device.
>                 io-channels = <&adc 10>, <&adc 11>;
>                 io-channel-names = "adc1", "adc2";
>         };
> +
> +==Common IIO properties==
> +
> +Reference voltage:
> +ADCs, DACs and several other IIO devices require a reference voltage.
> +By convention the property specifying this regulator is named "vref-supply".
> +If the chip lacks a dedicated Vref pin and instead uses its own power supply
> +as reference, the property specifying the regulator is commonly named
> +"vdd-supply" or "vcc-supply".
> +
> +Continuous mode:
> +Some sensors can be configured to perform continuous (versus one-shot)
> +measurements.  Continuous mode may require more energy in return for faster
> +or more reliable measurements.  A boolean property named "continuous"
> +signifies that the device is configured for this mode.
> --
> 2.11.0  

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Jonathan Cameron]

On Sun, 3 Sep 2017 20:20:46 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Sun, Sep 03, 2017 at 02:37:49PM +0100, Jonathan Cameron wrote:
> > On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:    
> > > --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> > > +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
> > > +Optional properties:
> > > +   - microchip,continuous-conversion (boolean):
> > > +           Only applicable to MCP3550/1/3:  These ADCs have long
> > > +           conversion times and therefore support "continuous
> > > +           conversion mode" to allow retrieval of conversions
> > > +           at any time without observing a delay.  The mode is
> > > +           enabled by permanently driving CS low, e.g. by wiring
> > > +           it to ground.    
> > 
> > hmm.  This is odd.  We probably need to make the SPI subsystem aware
> > of this. It is possible to ask for exclusive use of an SPI bus and
> > I think we should be doing this here.  It may be wired low on your
> > board, but it may be wired to a controllable chip select on other
> > boards and we can still force it low to trigger this mode if it makes
> > sense for the current application.
> > 
> > So I'd argue what we actually need to represent here is that the CS line
> > is not controllable.  What this means to the driver should be handled
> > in the driver - ideally also dealing with the case where it is controllable
> > appropriately (via exclusive bus usage).  Spi devices have the SPI_NO_CS
> > bit in the mode member of the spi device but I'm not sure about bindings.  
> [...]
> > The one case where we normally want to flip to continuous modes is when
> > we have a chardev access going on to the device.  In IIO that reflects
> > the fact we are in a push mode rather than userspace polling for new data.  
> 
> It seems there is no DT binding so far to set SPI_NO_CS.

Time to add one perhaps ;)

> 
> Conceivably, continuous mode could be used even with multiple devices
> on the bus if CLK and MISO is AND-gated with the CS signal coming from
> the SPI master.  (And the CS of the ADC is pulled low.)  In that case,
> the notion that "continuous mode == CS not controllable" would be
> incorrect, hence the approach I've chosen.
> 
> On the Revolution Pi we don't use continuous mode.  I merely included
> it in the driver for completeness.  If it is too controversial I'd be
> inclined to drop the feature.
> 
> On-demand switching to continuous mode by keeping CS low would be
> possible by setting the cs_change bit of struct mcp320x ->transfer[1],
> but that might not work if there are other devices on the bus.
> 

spi has exclusive bus access functions.  Doing this sort of thing isn't
that unusual.  Some devices require that the cs is held low whilst they
take a single reading, but provide an interrupt for when they are done.
For those we have to hold it for some time.

> 
> > Personally I don't think we are in a position yet to make this a generic
> > property - this is the first device where it is actually to do with the
> > physical circuit (and arguably it isn't really - see above).  
> 
> Okay.
> 
> 
> > Reference voltages are an oddity as the supply naming typically should match
> > that on the datasheet. It's 'fairly' consistent but some devices
> > have a set of relatively obscure references to different parts of the
> > input circuitry.  We can document it as a 'default' assuming nothing
> > strange is going on though.  This is why we have the vagueness below
> > on VDD and VCC.  
> 
> That is new to me, I believe it's not documented or am I missing something?
> I'd be happy to respin the below patch without the "Continuous mode"
> portion if you want?  (Amended with the info you gave above.)

It might make sense to drop continuous mode with the intent to add it
as a follow up patch.  Lets see what Mark and co come back with on how
to support the hard wired cs on the spi.

> Do you think iio-bindings.txt is the right place to put this or would
> a separate common.txt be more appropriate? (See e.g. leds/common.txt)

Perhaps a question best directed to the devicetree binding maintainers.
I can't say that I personally care either way ;)

> 
> Thanks,
> 
> Lukas
> 
> > -- >8 --    
> > Subject: [PATCH] dt-bindings: iio: Document common properties
> >
> > It's about time we standardize on common names for frequently used IIO
> > properties.  For starters, document "vref-supply" and "continuous".
> >
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  Documentation/devicetree/bindings/iio/iio-bindings.txt | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > index 68d6f8c..c3e87e15 100644
> > --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -95,3 +95,18 @@ vdd channel is connected to output 0 of the &ref device.
> >                 io-channels = <&adc 10>, <&adc 11>;
> >                 io-channel-names = "adc1", "adc2";
> >         };
> > +
> > +==Common IIO properties==
> > +
> > +Reference voltage:
> > +ADCs, DACs and several other IIO devices require a reference voltage.
> > +By convention the property specifying this regulator is named "vref-supply".
> > +If the chip lacks a dedicated Vref pin and instead uses its own power supply
> > +as reference, the property specifying the regulator is commonly named
> > +"vdd-supply" or "vcc-supply".
> > +
> > +Continuous mode:
> > +Some sensors can be configured to perform continuous (versus one-shot)
> > +measurements.  Continuous mode may require more energy in return for faster
> > +or more reliable measurements.  A boolean property named "continuous"
> > +signifies that the device is configured for this mode.
> > --
> > 2.11.0    
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Mark Brown]

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

On Sun, Sep 03, 2017 at 02:37:49PM +0100, Jonathan Cameron wrote:

> +cc Mark Brown and linux-spi to address question about how to represent
> a hardwired chip select.  See below for why I think that is what we should
> be representing rather than the fact it puts it in 'continuous mode'.

There's a good chance that if you just send me mail with a not obviously
related subject line it might get discarded unread, people CC me on lots
of things that are of questionable relevance.

> > >> > +   - microchip,continuous-conversion (boolean):
> > >> > +                   Only applicable to MCP3550/1/3:  These ADCs have long
> > >> > +                   conversion times and therefore support "continuous
> > >> > +                   conversion mode" to allow retrieval of conversions
> > >> > +                   at any time without observing a delay.  The mode is
> > >> > +                   enabled by permanently driving CS low, e.g. by wiring
> > >> > +                   it to ground.  

> hmm.  This is odd.  We probably need to make the SPI subsystem aware of this.
> It is possible to ask for exclusive use of an SPI bus and I think we should
> be doing this here.  It may be wired low on your board, but it may be wired to

This sounds like SPI_NO_CS, though it's vanishingly rare for it to be
implemented as there's a lot of ways for it to go wrong electrically -
it's a lot simpler to just have a chip select and minimise the number of
times it gets asserted.

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

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Jonathan Cameron]

On Mon, 4 Sep 2017 18:22:21 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Sun, Sep 03, 2017 at 02:37:49PM +0100, Jonathan Cameron wrote:
> 
> > +cc Mark Brown and linux-spi to address question about how to represent
> > a hardwired chip select.  See below for why I think that is what we should
> > be representing rather than the fact it puts it in 'continuous mode'.  
> 
> There's a good chance that if you just send me mail with a not obviously
> related subject line it might get discarded unread, people CC me on lots
> of things that are of questionable relevance.
> 
> > > >> > +   - microchip,continuous-conversion (boolean):
> > > >> > +                   Only applicable to MCP3550/1/3:  These ADCs have long
> > > >> > +                   conversion times and therefore support "continuous
> > > >> > +                   conversion mode" to allow retrieval of conversions
> > > >> > +                   at any time without observing a delay.  The mode is
> > > >> > +                   enabled by permanently driving CS low, e.g. by wiring
> > > >> > +                   it to ground.    
> 
> > hmm.  This is odd.  We probably need to make the SPI subsystem aware of this.
> > It is possible to ask for exclusive use of an SPI bus and I think we should
> > be doing this here.  It may be wired low on your board, but it may be wired to  
> 
> This sounds like SPI_NO_CS, though it's vanishingly rare for it to be
> implemented as there's a lot of ways for it to go wrong electrically -
> it's a lot simpler to just have a chip select and minimise the number of
> times it gets asserted.

Thanks Mark.  It sounds like we don't actually need it for the driver under
consideration on the particular platform it is being used on.  If it comes
up later, then adding bindings for SPI_NO_CS and adding relevant support
in the driver looks the way to go to me.

Jonathan

Re: [PATCH 5/6] dt-bindings: iio: adc: mcp320x: Update for mcp3550/1/3

[Rob Herring]

On Sun, Aug 27, 2017 at 10:34 AM, Lukas Wunner <lukas@wunner.de> wrote:
> On Fri, Aug 25, 2017 at 02:59:41PM -0500, Rob Herring wrote:
>> On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:
>> > --- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
>> > +++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
>> > +Optional properties:
>> > +   - microchip,continuous-conversion (boolean):
>> > +                   Only applicable to MCP3550/1/3:  These ADCs have long
>> > +                   conversion times and therefore support "continuous
>> > +                   conversion mode" to allow retrieval of conversions
>> > +                   at any time without observing a delay.  The mode is
>> > +                   enabled by permanently driving CS low, e.g. by wiring
>> > +                   it to ground.
>>
>> Second binding I have seen today with a continuous property. Make this
>> common (or maybe we already have one).
>
> The other one was the TI LMP92001 ADC driver posted by Abhisit Sangjan
> (cc), however looking at the datasheet of that chip reveals that
> continuous versus one-shot mode is selected by flipping a bit in the
> chip's register map.
>
> So it is configurable at run-time.  It's not something that's hardwired.
> (Which is the case with the MCP3550 in my patch.)

Couldn't you have CS tied to a GPIO line and then it is a run-time decision?

> My understanding was that run-time configurable options should not be
> listed in the device-tree at all, only hardware features.  If that is
> correct then that device-tree property should be dropped from Abhisit
> Sangjan's patch.  Configuring the feature via sysfs is fine I guess.

It depends. The question who decides the mode. If an end-user would
want to, then yes sysfs is the right place. If the h/w setup dictates
what the configuration should be, then in DT is fine.

> However we do have another driver supporting continuous versus one-shot
> mode and that is drivers/iio/light/us5182d.c by Adriana Reus (cc).
> The feature was added with commit c3304c212326.  I'm not sure if it's
> hardwired or runtime-configurable, the datasheet is gone from the
> manufacturer's website.
>
> I agree that a common "continuous" property makes sense.  We haven't
> defined any common IIO properties so far and that has already led to
> inconsistencies.  E.g. most ADC/DAC drivers name the reference regulator
> "vref-supply", but e.g. drivers/iio/dac/ad7303.c calls it "REF-supply".
>
> What do you think of this:
>
> -- >8 --
> Subject: [PATCH] dt-bindings: iio: Document common properties
>
> It's about time we standardize on common names for frequently used IIO
> properties.  For starters, document "vref-supply" and "continuous".
>
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  Documentation/devicetree/bindings/iio/iio-bindings.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> index 68d6f8c..c3e87e15 100644
> --- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -95,3 +95,18 @@ vdd channel is connected to output 0 of the &ref device.
>                 io-channels = <&adc 10>, <&adc 11>;
>                 io-channel-names = "adc1", "adc2";
>         };
> +
> +==Common IIO properties==
> +
> +Reference voltage:
> +ADCs, DACs and several other IIO devices require a reference voltage.
> +By convention the property specifying this regulator is named "vref-supply".
> +If the chip lacks a dedicated Vref pin and instead uses its own power supply
> +as reference, the property specifying the regulator is commonly named
> +"vdd-supply" or "vcc-supply".
> +
> +Continuous mode:
> +Some sensors can be configured to perform continuous (versus one-shot)
> +measurements.  Continuous mode may require more energy in return for faster
> +or more reliable measurements.  A boolean property named "continuous"
> +signifies that the device is configured for this mode.

Seems file, but start with the property names rather than buried in
the paragraph.

[PATCH 2/6] iio: adc: mcp320x: Fix readout of negative voltages

[Lukas Wunner]

Commit f686a36b4b79 ("iio: adc: mcp320x: Add support for mcp3301")
returns a signed voltage from mcp320x_adc_conversion() but neglects that
the caller interprets a negative return value as failure.  Only mcp3301
(and the upcoming mcp3550/1/3) is affected as the other chips are
incapable of measuring negative voltages.

Fix and while at it, add mcp3301 to the list of supported chips at the
top of the file.

Fixes: f686a36b4b79 ("iio: adc: mcp320x: Add support for mcp3301")
Cc: Andrea Galbusera <gizero@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/iio/adc/mcp320x.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 45d043c9a888..071dd23a33d9 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -17,6 +17,8 @@
  * MCP3204
  * MCP3208
  * ------------
+ * 13 bit converter
+ * MCP3301
  *
  * Datasheet can be found here:
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21293C.pdf  mcp3001
@@ -96,7 +98,7 @@ static int mcp320x_channel_to_tx_data(int device_index,
 }
 
 static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
-				  bool differential, int device_index)
+				  bool differential, int device_index, int *val)
 {
 	int ret;
 
@@ -117,19 +119,25 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 
 	switch (device_index) {
 	case mcp3001:
-		return (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
+		*val = (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
+		return 0;
 	case mcp3002:
 	case mcp3004:
 	case mcp3008:
-		return (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
+		*val = (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
+		return 0;
 	case mcp3201:
-		return (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
+		*val = (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
+		return 0;
 	case mcp3202:
 	case mcp3204:
 	case mcp3208:
-		return (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
+		*val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
+		return 0;
 	case mcp3301:
-		return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);
+		*val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
+				    | adc->rx_buf[1], 12);
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -150,12 +158,10 @@ static int mcp320x_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = mcp320x_adc_conversion(adc, channel->address,
-			channel->differential, device_index);
-
+			channel->differential, device_index, val);
 		if (ret < 0)
 			goto out;
 
-		*val = ret;
 		ret = IIO_VAL_INT;
 		break;
 
-- 
2.11.0

Re: [PATCH 2/6] iio: adc: mcp320x: Fix readout of negative voltages

[Jonathan Cameron]

On Tue, 22 Aug 2017 15:33:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Commit f686a36b4b79 ("iio: adc: mcp320x: Add support for mcp3301")
> returns a signed voltage from mcp320x_adc_conversion() but neglects that
> the caller interprets a negative return value as failure.  Only mcp3301
> (and the upcoming mcp3550/1/3) is affected as the other chips are
> incapable of measuring negative voltages.
> 
> Fix and while at it, add mcp3301 to the list of supported chips at the
> top of the file.
> 
> Fixes: f686a36b4b79 ("iio: adc: mcp320x: Add support for mcp3301")
> Cc: Andrea Galbusera <gizero@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/mcp320x.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 45d043c9a888..071dd23a33d9 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -17,6 +17,8 @@
>   * MCP3204
>   * MCP3208
>   * ------------
> + * 13 bit converter
> + * MCP3301
>   *
>   * Datasheet can be found here:
>   * http://ww1.microchip.com/downloads/en/DeviceDoc/21293C.pdf  mcp3001
> @@ -96,7 +98,7 @@ static int mcp320x_channel_to_tx_data(int device_index,
>  }
>  
>  static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
> -				  bool differential, int device_index)
> +				  bool differential, int device_index, int *val)
>  {
>  	int ret;
>  
> @@ -117,19 +119,25 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
>  
>  	switch (device_index) {
>  	case mcp3001:
> -		return (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
> +		*val = (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
> +		return 0;
>  	case mcp3002:
>  	case mcp3004:
>  	case mcp3008:
> -		return (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
> +		*val = (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
> +		return 0;
>  	case mcp3201:
> -		return (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
> +		*val = (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
> +		return 0;
>  	case mcp3202:
>  	case mcp3204:
>  	case mcp3208:
> -		return (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
> +		*val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
> +		return 0;
>  	case mcp3301:
> -		return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);
> +		*val = sign_extend32((adc->rx_buf[0] & 0x1f) << 8
> +				    | adc->rx_buf[1], 12);
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -150,12 +158,10 @@ static int mcp320x_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = mcp320x_adc_conversion(adc, channel->address,
> -			channel->differential, device_index);
> -
> +			channel->differential, device_index, val);
>  		if (ret < 0)
>  			goto out;
>  
> -		*val = ret;
>  		ret = IIO_VAL_INT;
>  		break;
>