[PATCH v2 0/5] iio: adc: mcp320x: Add triggered buffer support

[PATCH v2 0/5] iio: adc: mcp320x: Add triggered buffer support

[Vincent Whitchurch]

This series makes some suggested cleanups and then adds triggered buffer
support to the mcp320x ADC driver.

v2:
- Replace device_index rx switch with callbacks in chip_info
- Replace device_index tx switch with computation based
  on number of channels.
- Use conv_time instead of device_index switch when initializing
  conv_msg.
- Keep includes in alphabetical order with iio includes in a separate group
- Replace diff_off with scan_index as parameter to macro
- Remove reformatting of macros which makes the real change hard
  to spot

Cc: andy.shevchenko@gmail.com
Cc: lars@metafoo.de
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Axel Jonsson (1):
  iio: adc: mcp320x: add triggered buffer support

Vincent Whitchurch (4):
  iio: adc: mcp320x: use callbacks for RX conversion
  iio: adc: mcp320x: remove device_index check for TX
  iio: adc: mcp320x: use conv_time instead of device_index switch
  iio: adc: mcp320x: use device managed functions

 drivers/iio/adc/Kconfig   |   2 +
 drivers/iio/adc/mcp320x.c | 297 +++++++++++++++++++++++---------------
 2 files changed, 183 insertions(+), 116 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Vincent Whitchurch]

Replace the device_index switch with callbacks from the chip_info
structure, so that the latter has all the information needed to handle
the variants.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/iio/adc/mcp320x.c | 115 ++++++++++++++++++++++----------------
 1 file changed, 67 insertions(+), 48 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index b4c69acb33e3..c71d90babb39 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -61,11 +61,14 @@ enum {
 	mcp3553,
 };
 
+struct mcp320x;
+
 struct mcp320x_chip_info {
 	const struct iio_chan_spec *channels;
 	unsigned int num_channels;
 	unsigned int resolution;
 	unsigned int conv_time; /* usec */
+	int (*convert_rx)(struct mcp320x *adc);
 };
 
 /**
@@ -96,6 +99,54 @@ struct mcp320x {
 	u8 rx_buf[4];
 };
 
+static int mcp3001_convert_rx(struct mcp320x *adc)
+{
+	return adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3;
+}
+
+static int mcp3002_convert_rx(struct mcp320x *adc)
+{
+	return adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6;
+}
+
+static int mcp3201_convert_rx(struct mcp320x *adc)
+{
+	return adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1;
+}
+
+static int mcp3202_convert_rx(struct mcp320x *adc)
+{
+	return adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4;
+}
+
+static int mcp3301_convert_rx(struct mcp320x *adc)
+{
+	return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);
+}
+
+static int mcp3550_convert_rx(struct mcp320x *adc)
+{
+	u32 raw = be32_to_cpup((__be32 *)adc->rx_buf);
+
+	if (!(adc->spi->mode & SPI_CPOL))
+		raw <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
+
+	/*
+	 * 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.
+	 */
+	raw >>= 8;
+	if (raw & BIT(22) && raw & BIT(23))
+		return -EIO; /* cannot have overrange AND underrange */
+	else if (raw & BIT(22))
+		raw &= ~BIT(22); /* overrange */
+	else if (raw & BIT(23) || raw & BIT(21))
+		raw |= GENMASK(31, 22); /* underrange or negative */
+
+	return (s32)raw;
+}
+
 static int mcp320x_channel_to_tx_data(int device_index,
 			const unsigned int channel, bool differential)
 {
@@ -120,6 +171,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, int *val)
 {
+	const struct mcp320x_chip_info *info = adc->chip_info;
 	int ret;
 
 	if (adc->chip_info->conv_time) {
@@ -140,55 +192,9 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 	if (ret < 0)
 		return ret;
 
-	switch (device_index) {
-	case mcp3001:
-		*val = (adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3);
-		return 0;
-	case mcp3002:
-	case mcp3004:
-	case mcp3008:
-		*val = (adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6);
-		return 0;
-	case mcp3201:
-		*val = (adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1);
-		return 0;
-	case mcp3202:
-	case mcp3204:
-	case mcp3208:
-		*val = (adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4);
-		return 0;
-	case mcp3301:
-		*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: {
-		u32 raw = be32_to_cpup((__be32 *)adc->rx_buf);
-
-		if (!(adc->spi->mode & SPI_CPOL))
-			raw <<= 1; /* strip Data Ready bit in SPI mode 0,0 */
+	*val = info->convert_rx(adc);
 
-		/*
-		 * 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.
-		 */
-		raw >>= 8;
-		if (raw & BIT(22) && raw & BIT(23))
-			return -EIO; /* cannot have overrange AND underrange */
-		else if (raw & BIT(22))
-			raw &= ~BIT(22); /* overrange */
-		else if (raw & BIT(23) || raw & BIT(21))
-			raw |= GENMASK(31, 22); /* underrange or negative */
-
-		*val = (s32)raw;
-		return 0;
-		}
-	default:
-		return -EINVAL;
-	}
+	return 0;
 }
 
 static int mcp320x_read_raw(struct iio_dev *indio_dev,
@@ -302,51 +308,61 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
 	[mcp3001] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3001_convert_rx,
 		.resolution = 10
 	},
 	[mcp3002] = {
 		.channels = mcp3202_channels,
 		.num_channels = ARRAY_SIZE(mcp3202_channels),
+		.convert_rx = mcp3002_convert_rx,
 		.resolution = 10
 	},
 	[mcp3004] = {
 		.channels = mcp3204_channels,
 		.num_channels = ARRAY_SIZE(mcp3204_channels),
+		.convert_rx = mcp3002_convert_rx,
 		.resolution = 10
 	},
 	[mcp3008] = {
 		.channels = mcp3208_channels,
 		.num_channels = ARRAY_SIZE(mcp3208_channels),
+		.convert_rx = mcp3002_convert_rx,
 		.resolution = 10
 	},
 	[mcp3201] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3201_convert_rx,
 		.resolution = 12
 	},
 	[mcp3202] = {
 		.channels = mcp3202_channels,
 		.num_channels = ARRAY_SIZE(mcp3202_channels),
+		.convert_rx = mcp3202_convert_rx,
 		.resolution = 12
 	},
 	[mcp3204] = {
 		.channels = mcp3204_channels,
 		.num_channels = ARRAY_SIZE(mcp3204_channels),
+		.convert_rx = mcp3202_convert_rx,
 		.resolution = 12
 	},
 	[mcp3208] = {
 		.channels = mcp3208_channels,
 		.num_channels = ARRAY_SIZE(mcp3208_channels),
+		.convert_rx = mcp3202_convert_rx,
 		.resolution = 12
 	},
 	[mcp3301] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3301_convert_rx,
 		.resolution = 13
 	},
 	[mcp3550_50] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3550_convert_rx,
 		.resolution = 21,
 		/* 2% max deviation + 144 clock periods to exit shutdown */
 		.conv_time = 80000 * 1.02 + 144000 / 102.4,
@@ -354,18 +370,21 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
 	[mcp3550_60] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3550_convert_rx,
 		.resolution = 21,
 		.conv_time = 66670 * 1.02 + 144000 / 122.88,
 	},
 	[mcp3551] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3550_convert_rx,
 		.resolution = 21,
 		.conv_time = 73100 * 1.02 + 144000 / 112.64,
 	},
 	[mcp3553] = {
 		.channels = mcp3201_channels,
 		.num_channels = ARRAY_SIZE(mcp3201_channels),
+		.convert_rx = mcp3550_convert_rx,
 		.resolution = 21,
 		.conv_time = 16670 * 1.02 + 144000 / 122.88,
 	},
-- 
2.34.1

[PATCH v2 2/5] iio: adc: mcp320x: remove device_index check for TX

[Vincent Whitchurch]

Replace the device_index switch with a TX value computation based on the
number of channels in the chip_info structure, so that the latter has
all the information needed to handle the variants.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/iio/adc/mcp320x.c | 46 +++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index c71d90babb39..77fb4522a378 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -147,29 +147,34 @@ static int mcp3550_convert_rx(struct mcp320x *adc)
 	return (s32)raw;
 }
 
-static int mcp320x_channel_to_tx_data(int device_index,
-			const unsigned int channel, bool differential)
+static int mcp320x_channel_to_tx_data(const struct mcp320x_chip_info *info,
+				      const struct iio_chan_spec *channel)
 {
 	int start_bit = 1;
+	bool differential = channel->differential;
+	u8 address = channel->address;
+	/*
+	 * This happens to be the same as the last number of the model name for
+	 * multi-channel MCP300X and MCP320X.
+	 */
+	unsigned int num_nondiff_channels = info->num_channels / 2;
 
-	switch (device_index) {
-	case mcp3002:
-	case mcp3202:
+	switch (num_nondiff_channels) {
+	case 2:
 		return ((start_bit << 4) | (!differential << 3) |
-							(channel << 2));
-	case mcp3004:
-	case mcp3204:
-	case mcp3008:
-	case mcp3208:
+			(address << 2));
+	case 4:
+	case 8:
 		return ((start_bit << 6) | (!differential << 5) |
-							(channel << 2));
+			(address << 2));
 	default:
-		return -EINVAL;
+		return 0;
 	}
 }
 
-static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
-				  bool differential, int device_index, int *val)
+static int mcp320x_adc_conversion(struct mcp320x *adc,
+				  const struct iio_chan_spec *channel,
+				  int *val)
 {
 	const struct mcp320x_chip_info *info = adc->chip_info;
 	int ret;
@@ -185,8 +190,7 @@ static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel,
 
 	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);
+		adc->tx_buf = mcp320x_channel_to_tx_data(info, channel);
 
 	ret = spi_sync(adc->spi, &adc->msg);
 	if (ret < 0)
@@ -203,16 +207,12 @@ static int mcp320x_read_raw(struct iio_dev *indio_dev,
 {
 	struct mcp320x *adc = iio_priv(indio_dev);
 	int ret = -EINVAL;
-	int device_index = 0;
 
 	mutex_lock(&adc->lock);
 
-	device_index = spi_get_device_id(adc->spi)->driver_data;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = mcp320x_adc_conversion(adc, channel->address,
-			channel->differential, device_index, val);
+		ret = mcp320x_adc_conversion(adc, channel, val);
 		if (ret < 0)
 			goto out;
 
@@ -452,8 +452,8 @@ static int mcp320x_probe(struct spi_device *spi)
 		 * conversions without delay between them resets the chip
 		 * and ensures all subsequent conversions succeed.
 		 */
-		mcp320x_adc_conversion(adc, 0, 1, device_index, &ret);
-		mcp320x_adc_conversion(adc, 0, 1, device_index, &ret);
+		mcp320x_adc_conversion(adc, &chip_info->channels[0], &ret);
+		mcp320x_adc_conversion(adc, &chip_info->channels[0], &ret);
 	}
 
 	adc->reg = devm_regulator_get(&spi->dev, "vref");
-- 
2.34.1

[PATCH v2 3/5] iio: adc: mcp320x: use conv_time instead of device_index switch

[Vincent Whitchurch]

In mcp320x_adc_conversion(), the presence of the chip_info's conv_time
is used as a condition for using the conversion message.  Use that same
condition when initializing the conversion message and the other
handling for variants which need it, instead of the different condition
(checking of the device_index) which is used currently.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/iio/adc/mcp320x.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 77fb4522a378..8ed27df9a0bb 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -429,11 +429,7 @@ static int mcp320x_probe(struct spi_device *spi)
 		spi_message_init_with_transfers(&adc->msg, adc->transfer,
 						ARRAY_SIZE(adc->transfer));
 
-	switch (device_index) {
-	case mcp3550_50:
-	case mcp3550_60:
-	case mcp3551:
-	case mcp3553:
+	if (chip_info->conv_time) {
 		/* rx len increases from 24 to 25 bit in SPI mode 0,0 */
 		if (!(spi->mode & SPI_CPOL))
 			adc->transfer[1].len++;
-- 
2.34.1

[PATCH v2 4/5] iio: adc: mcp320x: use device managed functions

[Vincent Whitchurch]

Use devm_* functions in probe to remove some code and to make it easier
to add further calls to the probe function.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/iio/adc/mcp320x.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index 8ed27df9a0bb..b1c1bf4b8047 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -390,6 +390,11 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
 	},
 };
 
+static void mcp320x_reg_disable(void *reg)
+{
+	regulator_disable(reg);
+}
+
 static int mcp320x_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -460,27 +465,13 @@ static int mcp320x_probe(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
-	mutex_init(&adc->lock);
-
-	ret = iio_device_register(indio_dev);
-	if (ret < 0)
-		goto reg_disable;
-
-	return 0;
-
-reg_disable:
-	regulator_disable(adc->reg);
-
-	return ret;
-}
+	ret = devm_add_action_or_reset(&spi->dev, mcp320x_reg_disable, adc->reg);
+	if (ret)
+		return ret;
 
-static void mcp320x_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct mcp320x *adc = iio_priv(indio_dev);
+	mutex_init(&adc->lock);
 
-	iio_device_unregister(indio_dev);
-	regulator_disable(adc->reg);
+	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
 static const struct of_device_id mcp320x_dt_ids[] = {
@@ -535,7 +526,6 @@ static struct spi_driver mcp320x_driver = {
 		.of_match_table = mcp320x_dt_ids,
 	},
 	.probe = mcp320x_probe,
-	.remove = mcp320x_remove,
 	.id_table = mcp320x_id,
 };
 module_spi_driver(mcp320x_driver);
-- 
2.34.1

[PATCH v2 5/5] iio: adc: mcp320x: add triggered buffer support

[Vincent Whitchurch]

From: Axel Jonsson <axel.jonsson@axis.com>

Add support for triggered buffers.  The hardware doesn't provide any
special method of reading multiple channels, so just read the channels
in a loop to keep things simple.

Signed-off-by: Axel Jonsson <axel.jonsson@axis.com>
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 drivers/iio/adc/Kconfig   |   2 +
 drivers/iio/adc/mcp320x.c | 100 ++++++++++++++++++++++++++++++--------
 2 files changed, 82 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 48ace7412874..9f2628120885 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -696,6 +696,8 @@ config MAX9611
 config MCP320X
 	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
 	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Microchip Technology's
 	  MCP3001, MCP3002, MCP3004, MCP3008, MCP3201, MCP3202, MCP3204,
diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
index b1c1bf4b8047..bdc986285e1b 100644
--- a/drivers/iio/adc/mcp320x.c
+++ b/drivers/iio/adc/mcp320x.c
@@ -37,13 +37,18 @@
  * http://ww1.microchip.com/downloads/en/DeviceDoc/21950D.pdf  mcp3550/1/3
  */
 
-#include <linux/err.h>
 #include <linux/delay.h>
-#include <linux/spi/spi.h>
-#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
 #include <linux/mod_devicetable.h>
-#include <linux/iio/iio.h>
+#include <linux/module.h>
 #include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 enum {
 	mcp3001,
@@ -243,31 +248,45 @@ static int mcp320x_read_raw(struct iio_dev *indio_dev,
 		.indexed = 1,					\
 		.channel = (num),				\
 		.address = (num),				\
+		.scan_index = (num),				\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 32,				\
+			.storagebits = 32,			\
+			.endianness = IIO_CPU,			\
+		},						\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
 	}
 
-#define MCP320X_VOLTAGE_CHANNEL_DIFF(chan1, chan2)		\
+#define MCP320X_VOLTAGE_CHANNEL_DIFF(si, chan1, chan2)		\
 	{							\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
 		.channel = (chan1),				\
 		.channel2 = (chan2),				\
 		.address = (chan1),				\
+		.scan_index = (si),				\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 32,				\
+			.storagebits = 32,			\
+			.endianness = IIO_CPU,			\
+		},						\
 		.differential = 1,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
 	}
 
 static const struct iio_chan_spec mcp3201_channels[] = {
-	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 0, 1),
 };
 
 static const struct iio_chan_spec mcp3202_channels[] = {
 	MCP320X_VOLTAGE_CHANNEL(0),
 	MCP320X_VOLTAGE_CHANNEL(1),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 0, 1),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 1, 0),
 };
 
 static const struct iio_chan_spec mcp3204_channels[] = {
@@ -275,10 +294,10 @@ static const struct iio_chan_spec mcp3204_channels[] = {
 	MCP320X_VOLTAGE_CHANNEL(1),
 	MCP320X_VOLTAGE_CHANNEL(2),
 	MCP320X_VOLTAGE_CHANNEL(3),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 3),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 2),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(4, 0, 1),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(5, 1, 0),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(6, 2, 3),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(7, 3, 2),
 };
 
 static const struct iio_chan_spec mcp3208_channels[] = {
@@ -290,14 +309,14 @@ static const struct iio_chan_spec mcp3208_channels[] = {
 	MCP320X_VOLTAGE_CHANNEL(5),
 	MCP320X_VOLTAGE_CHANNEL(6),
 	MCP320X_VOLTAGE_CHANNEL(7),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 3),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 2),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(4, 5),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(5, 4),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(6, 7),
-	MCP320X_VOLTAGE_CHANNEL_DIFF(7, 6),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(8, 0, 1),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(9, 1, 0),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(10, 2, 3),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(11, 3, 2),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(12, 4, 5),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(13, 5, 4),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(14, 6, 7),
+	MCP320X_VOLTAGE_CHANNEL_DIFF(15, 7, 6),
 };
 
 static const struct iio_info mcp320x_info = {
@@ -390,6 +409,41 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
 	},
 };
 
+static irqreturn_t mcp320x_buffer_trigger_handler(int irq, void *pollfunc)
+{
+	struct iio_poll_func *pf = pollfunc;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct mcp320x *adc = iio_priv(indio_dev);
+	s32 data[ARRAY_SIZE(mcp3208_channels)];
+	int i = 0;
+	int chan;
+
+	mutex_lock(&adc->lock);
+
+	for_each_set_bit(chan, indio_dev->active_scan_mask, indio_dev->masklength) {
+		const struct iio_chan_spec *spec = &indio_dev->channels[chan];
+		int ret, val;
+
+		ret = mcp320x_adc_conversion(adc, spec, &val);
+		if (ret < 0) {
+			dev_err_ratelimited(&adc->spi->dev, "Failed to read data: %d\n",
+					    ret);
+			goto out;
+		}
+
+		data[i++] = val;
+	}
+
+	iio_push_to_buffers(indio_dev, data);
+
+out:
+	mutex_unlock(&adc->lock);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static void mcp320x_reg_disable(void *reg)
 {
 	regulator_disable(reg);
@@ -471,6 +525,12 @@ static int mcp320x_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
+					      mcp320x_buffer_trigger_handler,
+					      NULL);
+	if (ret)
+		return ret;
+
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
-- 
2.34.1

Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Andy Shevchenko]

On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> Replace the device_index switch with callbacks from the chip_info
> structure, so that the latter has all the information needed to handle
> the variants.

Below are for the further patches, as I see the original code has the
same disadvantages.

...

>  struct mcp320x_chip_info {
>         const struct iio_chan_spec *channels;
>         unsigned int num_channels;
>         unsigned int resolution;

>         unsigned int conv_time; /* usec */

Instead of a comment, I would rename it to conv_time_us.

> +       int (*convert_rx)(struct mcp320x *adc);
>  };

...

> +       return adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3;

> +       return adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6;

> +       return adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1;

> +       return adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4;

> +       return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);

All above should really use

u16 buf = be16_to_cpu(&adc->rx_buf[0]);

return buf >> 3 /* 6, 1, 4 (respectively) */;

...

> +       if (raw & BIT(22) && raw & BIT(23))

> +               return -EIO; /* cannot have overrange AND underrange */
> +       else if (raw & BIT(22))

Redundant  'else'.

> +               raw &= ~BIT(22); /* overrange */
> +       else if (raw & BIT(23) || raw & BIT(21))

if (raw & (BIT(23) | BIT(21))) ?

> +               raw |= GENMASK(31, 22); /* underrange or negative */

...

>         [mcp3201] = {
>                 .channels = mcp3201_channels,
>                 .num_channels = ARRAY_SIZE(mcp3201_channels),
> +               .convert_rx = mcp3201_convert_rx,

>                 .resolution = 12

+ Comma in such lines.

>         },

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] iio: adc: mcp320x: remove device_index check for TX

[Andy Shevchenko]

On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> Replace the device_index switch with a TX value computation based on the
> number of channels in the chip_info structure, so that the latter has
> all the information needed to handle the variants.

...

>                 return ((start_bit << 4) | (!differential << 3) |
> +                       (address << 2));

At the same time can be put on the above line and removed couple of ().

...

>                 return ((start_bit << 6) | (!differential << 5) |
> -                                                       (channel << 2));
> +                       (address << 2));

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/5] iio: adc: mcp320x: use conv_time instead of device_index switch

[Andy Shevchenko]

On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> In mcp320x_adc_conversion(), the presence of the chip_info's conv_time
> is used as a condition for using the conversion message.  Use that same
> condition when initializing the conversion message and the other
> handling for variants which need it, instead of the different condition
> (checking of the device_index) which is used currently.

...

> +       if (chip_info->conv_time) {

It would be nice to have conv_time_us renamed before this change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] iio: adc: mcp320x: use device managed functions

[Andy Shevchenko]

On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> Use devm_* functions in probe to remove some code and to make it easier
> to add further calls to the probe function.

devm_regulator_get_enable() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Vincent Whitchurch]

On Wed, Aug 31, 2022 at 02:40:49PM +0200, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> > Replace the device_index switch with callbacks from the chip_info
> > structure, so that the latter has all the information needed to handle
> > the variants.
> 
> Below are for the further patches, as I see the original code has the
> same disadvantages.

Right, these comments are in the original code that is either being just
being moved or that even just happens to be in the context of the diff.

Just to clarify, do you mean by "further patches" that you expect
patches to fix these comments to be added to this series which adds
triggered buffer support?

Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Andy Shevchenko]

On Wed, Aug 31, 2022 at 5:19 PM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
> On Wed, Aug 31, 2022 at 02:40:49PM +0200, Andy Shevchenko wrote:
> > On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> > <vincent.whitchurch@axis.com> wrote:
> > > Replace the device_index switch with callbacks from the chip_info
> > > structure, so that the latter has all the information needed to handle
> > > the variants.
> >
> > Below are for the further patches, as I see the original code has the
> > same disadvantages.
>
> Right, these comments are in the original code that is either being just
> being moved or that even just happens to be in the context of the diff.
>
> Just to clarify, do you mean by "further patches" that you expect
> patches to fix these comments to be added to this series which adds
> triggered buffer support?

Yes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] iio: adc: mcp320x: use device managed functions

[Jonathan Cameron]

On Wed, 31 Aug 2022 15:50:39 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> >
> > Use devm_* functions in probe to remove some code and to make it easier
> > to add further calls to the probe function.  
> 
> devm_regulator_get_enable() ?
> 
Not yet generally available as I don't think there is a suitable immutable
branch.

So for material aimed at this cycle I'm fine with it not being used.

Jonathan

Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Jonathan Cameron]

On Wed, 31 Aug 2022 15:40:49 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> >
> > Replace the device_index switch with callbacks from the chip_info
> > structure, so that the latter has all the information needed to handle
> > the variants.  
> 
> Below are for the further patches, as I see the original code has the
> same disadvantages.
> 
> ...
> 
> >  struct mcp320x_chip_info {
> >         const struct iio_chan_spec *channels;
> >         unsigned int num_channels;
> >         unsigned int resolution;  
> 
> >         unsigned int conv_time; /* usec */  
> 
> Instead of a comment, I would rename it to conv_time_us.
> 
> > +       int (*convert_rx)(struct mcp320x *adc);
> >  };  
> 
> ...
> 
> > +       return adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3;  
> 
> > +       return adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6;  
> 
> > +       return adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1;  
> 
> > +       return adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4;  
> 
> > +       return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);  
> 
> All above should really use
> 
> u16 buf = be16_to_cpu(&adc->rx_buf[0]);
> 
> return buf >> 3 /* 6, 1, 4 (respectively) */;

This made me look a bit closer at the code.  
rx_buf isn't necessarily aligned...
This may be a good usecase for an anonymous union.

Which would be fine except one of the callbacks (and original
code) uses be32_to_cpup() which is not unaligned safe.

I'm not keen to push unrelated work onto someone doing good stuff
on a driver, but in this particular case it does seem reasonable to
tidy all this up given you are moving the code anyway.

Jonathan

> 
> ...
> 
> > +       if (raw & BIT(22) && raw & BIT(23))  
> 
> > +               return -EIO; /* cannot have overrange AND underrange */
> > +       else if (raw & BIT(22))  
> 
> Redundant  'else'.
> 
> > +               raw &= ~BIT(22); /* overrange */
> > +       else if (raw & BIT(23) || raw & BIT(21))  
> 
> if (raw & (BIT(23) | BIT(21))) ?
> 
> > +               raw |= GENMASK(31, 22); /* underrange or negative */  
> 
> ...
> 
> >         [mcp3201] = {
> >                 .channels = mcp3201_channels,
> >                 .num_channels = ARRAY_SIZE(mcp3201_channels),
> > +               .convert_rx = mcp3201_convert_rx,  
> 
> >                 .resolution = 12  
> 
> + Comma in such lines.
> 
> >         },  
> 

Re: [PATCH v2 5/5] iio: adc: mcp320x: add triggered buffer support

[Jonathan Cameron]

On Wed, 31 Aug 2022 12:05:06 +0200
Vincent Whitchurch <vincent.whitchurch@axis.com> wrote:

> From: Axel Jonsson <axel.jonsson@axis.com>
> 
> Add support for triggered buffers.  The hardware doesn't provide any
> special method of reading multiple channels, so just read the channels
> in a loop to keep things simple.
> 
> Signed-off-by: Axel Jonsson <axel.jonsson@axis.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>

Hi.  

Just one question inline:  Is it worth providing the option of a timestamp?
Adds very little complexity to the code and sometimes very useful depending
on application.  Obviously, on a device like this it's very approximate and
is more or less just the time at which you start the read with no info on
how long that scan readout took. Still handy for some usecases - such as
when a sysfs trigger is being used to grab data in an uneven fashion.

Jonathan


> ---
>  drivers/iio/adc/Kconfig   |   2 +
>  drivers/iio/adc/mcp320x.c | 100 ++++++++++++++++++++++++++++++--------
>  2 files changed, 82 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 48ace7412874..9f2628120885 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -696,6 +696,8 @@ config MAX9611
>  config MCP320X
>  	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
>  	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Microchip Technology's
>  	  MCP3001, MCP3002, MCP3004, MCP3008, MCP3201, MCP3202, MCP3204,
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index b1c1bf4b8047..bdc986285e1b 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -37,13 +37,18 @@
>   * http://ww1.microchip.com/downloads/en/DeviceDoc/21950D.pdf  mcp3550/1/3
>   */
>  
> -#include <linux/err.h>
>  #include <linux/delay.h>
> -#include <linux/spi/spi.h>
> -#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
>  #include <linux/mod_devicetable.h>
> -#include <linux/iio/iio.h>
> +#include <linux/module.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

Would have slightly preferred to see header reorg in a separate patch
as then easier to see what is being added rather than moved, but I'll cope
if it's a pain to factor out.

> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  enum {
>  	mcp3001,
> @@ -243,31 +248,45 @@ static int mcp320x_read_raw(struct iio_dev *indio_dev,
>  		.indexed = 1,					\
>  		.channel = (num),				\
>  		.address = (num),				\
> +		.scan_index = (num),				\
> +		.scan_type = {					\
> +			.sign = 's',				\
> +			.realbits = 32,				\
> +			.storagebits = 32,			\
> +			.endianness = IIO_CPU,			\
> +		},						\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
>  	}
>  
> -#define MCP320X_VOLTAGE_CHANNEL_DIFF(chan1, chan2)		\
> +#define MCP320X_VOLTAGE_CHANNEL_DIFF(si, chan1, chan2)		\
>  	{							\
>  		.type = IIO_VOLTAGE,				\
>  		.indexed = 1,					\
>  		.channel = (chan1),				\
>  		.channel2 = (chan2),				\
>  		.address = (chan1),				\
> +		.scan_index = (si),				\
> +		.scan_type = {					\
> +			.sign = 's',				\
> +			.realbits = 32,				\
> +			.storagebits = 32,			\
> +			.endianness = IIO_CPU,			\
> +		},						\
>  		.differential = 1,				\
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
>  	}
>  
>  static const struct iio_chan_spec mcp3201_channels[] = {
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 0, 1),
>  };
>  
>  static const struct iio_chan_spec mcp3202_channels[] = {
>  	MCP320X_VOLTAGE_CHANNEL(0),
>  	MCP320X_VOLTAGE_CHANNEL(1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 0, 1),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 1, 0),
>  };
>  
>  static const struct iio_chan_spec mcp3204_channels[] = {
> @@ -275,10 +294,10 @@ static const struct iio_chan_spec mcp3204_channels[] = {
>  	MCP320X_VOLTAGE_CHANNEL(1),
>  	MCP320X_VOLTAGE_CHANNEL(2),
>  	MCP320X_VOLTAGE_CHANNEL(3),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 3),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 2),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(4, 0, 1),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(5, 1, 0),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(6, 2, 3),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(7, 3, 2),
>  };
>  
>  static const struct iio_chan_spec mcp3208_channels[] = {
> @@ -290,14 +309,14 @@ static const struct iio_chan_spec mcp3208_channels[] = {
>  	MCP320X_VOLTAGE_CHANNEL(5),
>  	MCP320X_VOLTAGE_CHANNEL(6),
>  	MCP320X_VOLTAGE_CHANNEL(7),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 3),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 2),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(4, 5),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(5, 4),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(6, 7),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(7, 6),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(8, 0, 1),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(9, 1, 0),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(10, 2, 3),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(11, 3, 2),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(12, 4, 5),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(13, 5, 4),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(14, 6, 7),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(15, 7, 6),
>  };
>  
>  static const struct iio_info mcp320x_info = {
> @@ -390,6 +409,41 @@ static const struct mcp320x_chip_info mcp320x_chip_infos[] = {
>  	},
>  };
>  
> +static irqreturn_t mcp320x_buffer_trigger_handler(int irq, void *pollfunc)
> +{
> +	struct iio_poll_func *pf = pollfunc;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct mcp320x *adc = iio_priv(indio_dev);
> +	s32 data[ARRAY_SIZE(mcp3208_channels)];
> +	int i = 0;
> +	int chan;
> +
> +	mutex_lock(&adc->lock);
> +
> +	for_each_set_bit(chan, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		const struct iio_chan_spec *spec = &indio_dev->channels[chan];
> +		int ret, val;
> +
> +		ret = mcp320x_adc_conversion(adc, spec, &val);
> +		if (ret < 0) {
> +			dev_err_ratelimited(&adc->spi->dev, "Failed to read data: %d\n",
> +					    ret);
> +			goto out;
> +		}
> +
> +		data[i++] = val;
> +	}
> +
> +	iio_push_to_buffers(indio_dev, data);

Worth option of a timestamp?  If you do add one, you'll need to expand data and
force alignment to be at least 8 bytes.

> +
> +out:
> +	mutex_unlock(&adc->lock);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static void mcp320x_reg_disable(void *reg)
>  {
>  	regulator_disable(reg);
> @@ -471,6 +525,12 @@ static int mcp320x_probe(struct spi_device *spi)
>  
>  	mutex_init(&adc->lock);
>  
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
> +					      mcp320x_buffer_trigger_handler,
> +					      NULL);
> +	if (ret)
> +		return ret;
> +
>  	return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>  

Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Vincent Whitchurch]

On Sun, Sep 04, 2022 at 06:15:59PM +0200, Jonathan Cameron wrote:
> I'm not keen to push unrelated work onto someone doing good stuff
> on a driver, but in this particular case it does seem reasonable to
> tidy all this up given you are moving the code anyway.

Well, even the moving of the code is unrelated to the original goal of
adding triggered buffered support and isn't necessary for that.  The
moving of the code was only to eliminate the use of the "device_index",
which was already used in the existing code.

I'm of course happy to fix problems with the code I'm actually adding,
but it seems to me that it would really be simpler for everyone if the
trivial comments (especially the purely cosmetic ones) on the existing,
unrelated code would be fixed by the people with the opinions about how
the existing code should look like.  I don't have any special ability to
test the dozen different chips this driver supports, so having me do it
by proxy seems rather suboptimal.  I can only run it in roadtest, which
anyone can do with the following commands (against v5.19 due to the
regressions in mainline I mentioned in my other email), without special
hardware:

 git checkout v5.19
 git remote add vwax https://github.com/vwax/linux.git
 git fetch vwax 
 git archive vwax/roadtest/mcp320x tools/testing/roadtest | tar xf -
 make -C tools/testing/roadtest/ -j24 OPTS="-v -k 'mcp and not trigger'"

Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

[Andy Shevchenko]

On Mon, Sep 5, 2022 at 11:27 AM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> On Sun, Sep 04, 2022 at 06:15:59PM +0200, Jonathan Cameron wrote:
> > I'm not keen to push unrelated work onto someone doing good stuff
> > on a driver, but in this particular case it does seem reasonable to
> > tidy all this up given you are moving the code anyway.
>
> Well, even the moving of the code is unrelated to the original goal of
> adding triggered buffered support and isn't necessary for that.  The
> moving of the code was only to eliminate the use of the "device_index",
> which was already used in the existing code.
>
> I'm of course happy to fix problems with the code I'm actually adding,
> but it seems to me that it would really be simpler for everyone if the
> trivial comments (especially the purely cosmetic ones) on the existing,
> unrelated code would be fixed by the people with the opinions about how
> the existing code should look like.

That's what Jonathan basically says, but with one remark that some of
the fixes are affecting the code one is going to add. In any case, you
may look at the "people with the opinions about how the existing code
should look like" at different angle, i.e. that those people may be
way overloaded while doing _a lot_ (and I mean it) of the stuff, so
from their perspective it's easier if somebody who is already working
on the driver can add a few trivial things which takes from her/him a
couple of hours to accomplish. In the collaboration we get the product
(Linux kernel) better!

-- 
With Best Regards,
Andy Shevchenko