[PATCH v2] iio: adis16480: support burst read function

[PATCH v2] iio: adis16480: support burst read function

[Nuno Sa]

Some supported devices support burst read function. This provides a method
for reading a batch of data (status, temperature, gyroscopes,
accelerometers, time stamp/data counter, and CRC code), which does not
require a stall time between each 16-bit segment and only requires one
command on the DIN line to initiate. Devices supporting this mode
are:

  * adis16495-1
  * adis16495-2
  * adis16495-3
  * adis16497-1
  * adis16497-2
  * adis16497-3

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
Changes in v2:
 * Return right away if offset == 4 (no valid transition found from
brurst_id to sys_flags)

 drivers/iio/imu/adis16480.c | 161 +++++++++++++++++++++++++++++++++---
 1 file changed, 148 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index f81b86690b76..b782070a27f5 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -5,6 +5,7 @@
  * Copyright 2012 Analog Devices Inc.
  */
 
+#include <linux/crc32.h>
 #include <linux/clk.h>
 #include <linux/bitfield.h>
 #include <linux/of_irq.h>
@@ -19,11 +20,14 @@
 #include <linux/sysfs.h>
 #include <linux/module.h>
 #include <linux/lcm.h>
+#include <linux/swab.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/imu/adis.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #include <linux/debugfs.h>
 
@@ -103,6 +107,12 @@
  * Available only for ADIS1649x devices
  */
 #define ADIS16495_REG_SYNC_SCALE		ADIS16480_REG(0x03, 0x10)
+#define ADIS16495_REG_BURST_CMD			ADIS16480_REG(0x00, 0x7C)
+#define ADIS16495_BURST_ID			0xA5A5
+/* total number of segments in burst */
+#define ADIS16495_BURST_MAX_DATA		20
+/* spi max speed in burst mode */
+#define ADIS16495_BURST_MAX_SPEED              6000000
 
 #define ADIS16480_REG_SERIAL_NUM		ADIS16480_REG(0x04, 0x20)
 
@@ -163,6 +173,8 @@ struct adis16480 {
 	struct clk *ext_clk;
 	enum adis16480_clock_mode clk_mode;
 	unsigned int clk_freq;
+	/* Alignment needed for the timestamp */
+	__be16 data[ADIS16495_BURST_MAX_DATA] __aligned(8);
 };
 
 static const char * const adis16480_int_pin_names[4] = {
@@ -863,7 +875,7 @@ static const char * const adis16480_status_error_msgs[] = {
 
 static int adis16480_enable_irq(struct adis *adis, bool enable);
 
-#define ADIS16480_DATA(_prod_id, _timeouts)				\
+#define ADIS16480_DATA(_prod_id, _timeouts, _burst_len)			\
 {									\
 	.diag_stat_reg = ADIS16480_REG_DIAG_STS,			\
 	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,				\
@@ -887,6 +899,8 @@ static int adis16480_enable_irq(struct adis *adis, bool enable);
 		BIT(ADIS16480_DIAG_STAT_BARO_FAIL),			\
 	.enable_irq = adis16480_enable_irq,				\
 	.timeouts = (_timeouts),					\
+	.burst_reg_cmd = ADIS16495_REG_BURST_CMD,			\
+	.burst_len = (_burst_len),					\
 }
 
 static const struct adis_timeout adis16485_timeouts = {
@@ -931,7 +945,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16375, &adis16485_timeouts),
+		.adis_data = ADIS16480_DATA(16375, &adis16485_timeouts, 0),
 	},
 	[ADIS16480] = {
 		.channels = adis16480_channels,
@@ -944,7 +958,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16480, &adis16480_timeouts),
+		.adis_data = ADIS16480_DATA(16480, &adis16480_timeouts, 0),
 	},
 	[ADIS16485] = {
 		.channels = adis16485_channels,
@@ -957,7 +971,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16485, &adis16485_timeouts),
+		.adis_data = ADIS16480_DATA(16485, &adis16485_timeouts, 0),
 	},
 	[ADIS16488] = {
 		.channels = adis16480_channels,
@@ -970,7 +984,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
-		.adis_data = ADIS16480_DATA(16488, &adis16485_timeouts),
+		.adis_data = ADIS16480_DATA(16488, &adis16485_timeouts, 0),
 	},
 	[ADIS16490] = {
 		.channels = adis16485_channels,
@@ -984,7 +998,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16490, &adis16495_timeouts),
+		.adis_data = ADIS16480_DATA(16490, &adis16495_timeouts, 0),
 	},
 	[ADIS16495_1] = {
 		.channels = adis16485_channels,
@@ -998,7 +1012,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2),
 	},
 	[ADIS16495_2] = {
 		.channels = adis16485_channels,
@@ -1012,7 +1028,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2),
 	},
 	[ADIS16495_3] = {
 		.channels = adis16485_channels,
@@ -1026,7 +1044,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2),
 	},
 	[ADIS16497_1] = {
 		.channels = adis16485_channels,
@@ -1040,7 +1060,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2),
 	},
 	[ADIS16497_2] = {
 		.channels = adis16485_channels,
@@ -1054,7 +1076,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2),
 	},
 	[ADIS16497_3] = {
 		.channels = adis16485_channels,
@@ -1068,10 +1092,121 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
-		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
+		/* 20 elements of 16bits */
+		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
+					    ADIS16495_BURST_MAX_DATA * 2),
 	},
 };
 
+static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc)
+{
+	u32 crc_calc;
+	u16 crc_buf[15];
+	int j;
+
+	for (j = 0; j < n_elem; j++)
+		crc_buf[j] = swab16(buf[j]);
+
+	crc_calc = crc32(~0, crc_buf, n_elem * 2);
+	crc_calc ^= ~0;
+
+	return (crc == crc_calc);
+}
+
+static irqreturn_t adis16480_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adis16480 *st = iio_priv(indio_dev);
+	struct adis *adis = &st->adis;
+	int ret, bit, offset, i = 0;
+	__be16 *buffer;
+	u32 crc;
+	bool valid;
+	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
+
+	adis_dev_lock(adis);
+	if (adis->current_page != 0) {
+		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
+		adis->tx[1] = 0;
+		ret = spi_write(adis->spi, adis->tx, 2);
+		if (ret) {
+			dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
+			adis_dev_unlock(adis);
+			return ret;
+		}
+	}
+
+	adis->spi->max_speed_hz = ADIS16495_BURST_MAX_SPEED;
+
+	ret = spi_sync(adis->spi, &adis->msg);
+	if (ret) {
+		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
+		adis_dev_unlock(adis);
+		return ret;
+	}
+
+	adis->spi->max_speed_hz = cached_spi_speed_hz;
+	adis->current_page = 0;
+	adis_dev_unlock(adis);
+
+	/*
+	 * After making the burst request, the response can have one or two
+	 * 16-bit responses containing the BURST_ID depending on the sclk. If
+	 * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ,
+	 * we have only one. To manage that variation, we use the transition from the
+	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If
+	 * we not find this variation in the first 4 segments, then the data should
+	 * not be valid.
+	 */
+	buffer = adis->buffer;
+	for (offset = 0; offset < 4; offset++) {
+		u16 curr = be16_to_cpu(buffer[offset]);
+		u16 next = be16_to_cpu(buffer[offset + 1]);
+
+		if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) {
+			offset++;
+			break;
+		}
+	}
+
+	if (offset == 4) {
+		dev_err(&adis->spi->dev, "Invalid burst data\n");
+		goto irq_done;
+	}
+
+	crc = be16_to_cpu(buffer[offset + 16]) << 16 | be16_to_cpu(buffer[offset + 15]);
+	valid = adis16480_validate_crc((u16 *)&buffer[offset], 15, crc);
+	if (!valid) {
+		dev_err(&adis->spi->dev, "Invalid crc\n");
+		goto irq_done;
+	}
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
+		/*
+		 * When burst mode is used, temperature is the first data
+		 * channel in the sequence, but the temperature scan index
+		 * is 10.
+		 */
+		switch (bit) {
+		case ADIS16480_SCAN_TEMP:
+			st->data[i++] = buffer[offset + 1];
+			break;
+		case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z:
+			/* The lower register data is sequenced first */
+			st->data[i++] = buffer[2 * bit + offset + 3];
+			st->data[i++] = buffer[2 * bit + offset + 2];
+			break;
+		}
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp);
+irq_done:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static const struct iio_info adis16480_info = {
 	.read_raw = &adis16480_read_raw,
 	.write_raw = &adis16480_write_raw,
@@ -1341,7 +1476,7 @@ static int adis16480_probe(struct spi_device *spi)
 		st->clk_freq = st->chip_info->int_clk;
 	}
 
-	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
+	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, adis16480_trigger_handler);
 	if (ret)
 		return ret;
 
-- 
2.31.1

Re: [PATCH v2] iio: adis16480: support burst read function

[Lars-Peter Clausen]

On 4/8/21 9:56 AM, Nuno Sa wrote:
> Some supported devices support burst read function. This provides a method
> for reading a batch of data (status, temperature, gyroscopes,
> accelerometers, time stamp/data counter, and CRC code), which does not
> require a stall time between each 16-bit segment and only requires one
> command on the DIN line to initiate. Devices supporting this mode
> are:
>
>    * adis16495-1
>    * adis16495-2
>    * adis16495-3
>    * adis16497-1
>    * adis16497-2
>    * adis16497-3
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

Looks good to me.

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

Re: [PATCH v2] iio: adis16480: support burst read function

[Jonathan Cameron]

On Thu, 8 Apr 2021 09:56:43 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> Some supported devices support burst read function. This provides a method
> for reading a batch of data (status, temperature, gyroscopes,
> accelerometers, time stamp/data counter, and CRC code), which does not
> require a stall time between each 16-bit segment and only requires one
> command on the DIN line to initiate. Devices supporting this mode
> are:
> 
>   * adis16495-1
>   * adis16495-2
>   * adis16495-3
>   * adis16497-1
>   * adis16497-2
>   * adis16497-3
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Small issue with return values in the trigger handler and
some stylistic comments.

Otherwise, looks good to me.

> ---
> Changes in v2:
>  * Return right away if offset == 4 (no valid transition found from
> brurst_id to sys_flags)
> 
>  drivers/iio/imu/adis16480.c | 161 +++++++++++++++++++++++++++++++++---
>  1 file changed, 148 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index f81b86690b76..b782070a27f5 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -5,6 +5,7 @@
>   * Copyright 2012 Analog Devices Inc.
>   */
>  
> +#include <linux/crc32.h>

Hmm. The ordering here is a bit odd so I guess this doesn't
really matter, but seems strange to have something so specific as
crc32.h as the first header included when it's not the first one
alphabetically.

>  #include <linux/clk.h>
>  #include <linux/bitfield.h>
>  #include <linux/of_irq.h>
> @@ -19,11 +20,14 @@
>  #include <linux/sysfs.h>
>  #include <linux/module.h>
>  #include <linux/lcm.h>
> +#include <linux/swab.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/imu/adis.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #include <linux/debugfs.h>
>  
> @@ -103,6 +107,12 @@
>   * Available only for ADIS1649x devices
>   */
>  #define ADIS16495_REG_SYNC_SCALE		ADIS16480_REG(0x03, 0x10)
> +#define ADIS16495_REG_BURST_CMD			ADIS16480_REG(0x00, 0x7C)
> +#define ADIS16495_BURST_ID			0xA5A5
> +/* total number of segments in burst */
> +#define ADIS16495_BURST_MAX_DATA		20
> +/* spi max speed in burst mode */
> +#define ADIS16495_BURST_MAX_SPEED              6000000
>  
>  #define ADIS16480_REG_SERIAL_NUM		ADIS16480_REG(0x04, 0x20)
>  
> @@ -163,6 +173,8 @@ struct adis16480 {
>  	struct clk *ext_clk;
>  	enum adis16480_clock_mode clk_mode;
>  	unsigned int clk_freq;
> +	/* Alignment needed for the timestamp */
> +	__be16 data[ADIS16495_BURST_MAX_DATA] __aligned(8);
>  };
>  
>  static const char * const adis16480_int_pin_names[4] = {
> @@ -863,7 +875,7 @@ static const char * const adis16480_status_error_msgs[] = {
>  
>  static int adis16480_enable_irq(struct adis *adis, bool enable);
>  
> -#define ADIS16480_DATA(_prod_id, _timeouts)				\
> +#define ADIS16480_DATA(_prod_id, _timeouts, _burst_len)			\
>  {									\
>  	.diag_stat_reg = ADIS16480_REG_DIAG_STS,			\
>  	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,				\
> @@ -887,6 +899,8 @@ static int adis16480_enable_irq(struct adis *adis, bool enable);
>  		BIT(ADIS16480_DIAG_STAT_BARO_FAIL),			\
>  	.enable_irq = adis16480_enable_irq,				\
>  	.timeouts = (_timeouts),					\
> +	.burst_reg_cmd = ADIS16495_REG_BURST_CMD,			\
> +	.burst_len = (_burst_len),					\
>  }
>  
>  static const struct adis_timeout adis16485_timeouts = {
> @@ -931,7 +945,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.int_clk = 2460000,
>  		.max_dec_rate = 2048,
>  		.filter_freqs = adis16480_def_filter_freqs,
> -		.adis_data = ADIS16480_DATA(16375, &adis16485_timeouts),
> +		.adis_data = ADIS16480_DATA(16375, &adis16485_timeouts, 0),
>  	},
>  	[ADIS16480] = {
>  		.channels = adis16480_channels,
> @@ -944,7 +958,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.int_clk = 2460000,
>  		.max_dec_rate = 2048,
>  		.filter_freqs = adis16480_def_filter_freqs,
> -		.adis_data = ADIS16480_DATA(16480, &adis16480_timeouts),
> +		.adis_data = ADIS16480_DATA(16480, &adis16480_timeouts, 0),
>  	},
>  	[ADIS16485] = {
>  		.channels = adis16485_channels,
> @@ -957,7 +971,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.int_clk = 2460000,
>  		.max_dec_rate = 2048,
>  		.filter_freqs = adis16480_def_filter_freqs,
> -		.adis_data = ADIS16480_DATA(16485, &adis16485_timeouts),
> +		.adis_data = ADIS16480_DATA(16485, &adis16485_timeouts, 0),
>  	},
>  	[ADIS16488] = {
>  		.channels = adis16480_channels,
> @@ -970,7 +984,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.int_clk = 2460000,
>  		.max_dec_rate = 2048,
>  		.filter_freqs = adis16480_def_filter_freqs,
> -		.adis_data = ADIS16480_DATA(16488, &adis16485_timeouts),
> +		.adis_data = ADIS16480_DATA(16488, &adis16485_timeouts, 0),
>  	},
>  	[ADIS16490] = {
>  		.channels = adis16485_channels,
> @@ -984,7 +998,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16490, &adis16495_timeouts),
> +		.adis_data = ADIS16480_DATA(16490, &adis16495_timeouts, 0),
>  	},
>  	[ADIS16495_1] = {
>  		.channels = adis16485_channels,
> @@ -998,7 +1012,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
> +		/* 20 elements of 16bits */
> +		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
> +					    ADIS16495_BURST_MAX_DATA * 2),
>  	},
>  	[ADIS16495_2] = {
>  		.channels = adis16485_channels,
> @@ -1012,7 +1028,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
> +		/* 20 elements of 16bits */
> +		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
> +					    ADIS16495_BURST_MAX_DATA * 2),
>  	},
>  	[ADIS16495_3] = {
>  		.channels = adis16485_channels,
> @@ -1026,7 +1044,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
> +		/* 20 elements of 16bits */
> +		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts,
> +					    ADIS16495_BURST_MAX_DATA * 2),
>  	},
>  	[ADIS16497_1] = {
>  		.channels = adis16485_channels,
> @@ -1040,7 +1060,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
> +		/* 20 elements of 16bits */
> +		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
> +					    ADIS16495_BURST_MAX_DATA * 2),
>  	},
>  	[ADIS16497_2] = {
>  		.channels = adis16485_channels,
> @@ -1054,7 +1076,9 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
> +		/* 20 elements of 16bits */
> +		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
> +					    ADIS16495_BURST_MAX_DATA * 2),
>  	},
>  	[ADIS16497_3] = {
>  		.channels = adis16485_channels,
> @@ -1068,10 +1092,121 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> -		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
> +		/* 20 elements of 16bits */
> +		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts,
> +					    ADIS16495_BURST_MAX_DATA * 2),
>  	},
>  };
>  
> +static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc)
> +{
> +	u32 crc_calc;
> +	u16 crc_buf[15];
> +	int j;
> +
> +	for (j = 0; j < n_elem; j++)
> +		crc_buf[j] = swab16(buf[j]);
> +
> +	crc_calc = crc32(~0, crc_buf, n_elem * 2);
> +	crc_calc ^= ~0;
> +
> +	return (crc == crc_calc);
> +}
> +
> +static irqreturn_t adis16480_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adis16480 *st = iio_priv(indio_dev);
> +	struct adis *adis = &st->adis;
> +	int ret, bit, offset, i = 0;
> +	__be16 *buffer;
> +	u32 crc;
> +	bool valid;
> +	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
> +
> +	adis_dev_lock(adis);
> +	if (adis->current_page != 0) {
> +		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> +		adis->tx[1] = 0;
> +		ret = spi_write(adis->spi, adis->tx, 2);
> +		if (ret) {
> +			dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
> +			adis_dev_unlock(adis);
> +			return ret;

This is an interrupt handler, you should be careful what you return
as they will be treated as irqreturn_t not ints.

return IRQ_HANDLED even in error paths.

> +		}
> +	}
> +
> +	adis->spi->max_speed_hz = ADIS16495_BURST_MAX_SPEED;
> +
> +	ret = spi_sync(adis->spi, &adis->msg);
> +	if (ret) {
> +		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
> +		adis_dev_unlock(adis);
> +		return ret;
> +	}
> +
> +	adis->spi->max_speed_hz = cached_spi_speed_hz;
> +	adis->current_page = 0;

Does it make more sense to move this to just after we changed the page?
If something more complex is going on, perhaps a comment here?

> +	adis_dev_unlock(adis);
> +
> +	/*
> +	 * After making the burst request, the response can have one or two
> +	 * 16-bit responses containing the BURST_ID depending on the sclk. If
> +	 * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ,
> +	 * we have only one. To manage that variation, we use the transition from the
> +	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If
> +	 * we not find this variation in the first 4 segments, then the data should
> +	 * not be valid.
> +	 */
> +	buffer = adis->buffer;
> +	for (offset = 0; offset < 4; offset++) {
> +		u16 curr = be16_to_cpu(buffer[offset]);
> +		u16 next = be16_to_cpu(buffer[offset + 1]);
> +
> +		if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) {
> +			offset++;
> +			break;
> +		}
> +	}
> +
> +	if (offset == 4) {
> +		dev_err(&adis->spi->dev, "Invalid burst data\n");
> +		goto irq_done;
> +	}
> +
> +	crc = be16_to_cpu(buffer[offset + 16]) << 16 | be16_to_cpu(buffer[offset + 15]);
> +	valid = adis16480_validate_crc((u16 *)&buffer[offset], 15, crc);
> +	if (!valid) {
> +		dev_err(&adis->spi->dev, "Invalid crc\n");
> +		goto irq_done;
> +	}
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		/*
> +		 * When burst mode is used, temperature is the first data
> +		 * channel in the sequence, but the temperature scan index
> +		 * is 10.
> +		 */
> +		switch (bit) {
> +		case ADIS16480_SCAN_TEMP:
> +			st->data[i++] = buffer[offset + 1];
> +			break;
> +		case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z:
> +			/* The lower register data is sequenced first */
> +			st->data[i++] = buffer[2 * bit + offset + 3];
> +			st->data[i++] = buffer[2 * bit + offset + 2];
> +			break;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp);
> +irq_done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct iio_info adis16480_info = {
>  	.read_raw = &adis16480_read_raw,
>  	.write_raw = &adis16480_write_raw,
> @@ -1341,7 +1476,7 @@ static int adis16480_probe(struct spi_device *spi)
>  		st->clk_freq = st->chip_info->int_clk;
>  	}
>  
> -	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
> +	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev, adis16480_trigger_handler);

Whilst 100 chars is fine, it's preferred to stay under 80 when it doesn't hurt readability
so I'd like to see this on two lines.

>  	if (ret)
>  		return ret;
>  

RE: [PATCH v2] iio: adis16480: support burst read function

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, April 11, 2021 4:40 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH v2] iio: adis16480: support burst read function
> 
> 
> On Thu, 8 Apr 2021 09:56:43 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > Some supported devices support burst read function. This provides a
> method
> > for reading a batch of data (status, temperature, gyroscopes,
> > accelerometers, time stamp/data counter, and CRC code), which
> does not
> > require a stall time between each 16-bit segment and only requires
> one
> > command on the DIN line to initiate. Devices supporting this mode
> > are:
> >
> >   * adis16495-1
> >   * adis16495-2
> >   * adis16495-3
> >   * adis16497-1
> >   * adis16497-2
> >   * adis16497-3
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> Small issue with return values in the trigger handler and
> some stylistic comments.
> 
> Otherwise, looks good to me.
> 
> > ---
> > Changes in v2:
> >  * Return right away if offset == 4 (no valid transition found from
> > brurst_id to sys_flags)
> >
> >  drivers/iio/imu/adis16480.c | 161
> +++++++++++++++++++++++++++++++++---
> >  1 file changed, 148 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index f81b86690b76..b782070a27f5 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -5,6 +5,7 @@
> >   * Copyright 2012 Analog Devices Inc.
> >   */
> >
> > +#include <linux/crc32.h>
> 
> Hmm. The ordering here is a bit odd so I guess this doesn't
> really matter, but seems strange to have something so specific as
> crc32.h as the first header included when it's not the first one
> alphabetically.

Yeah, it was not ordered already, so I didn't cared much. I will move more
to the middle...

> >  #include <linux/clk.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/of_irq.h>
> > @@ -19,11 +20,14 @@
> >  #include <linux/sysfs.h>
> >  #include <linux/module.h>
> >  #include <linux/lcm.h>
> > +#include <linux/swab.h>
> >
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/imu/adis.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> >
> >  #include <linux/debugfs.h>
> >
> > @@ -103,6 +107,12 @@
> >   * Available only for ADIS1649x devices
> >   */
> >  #define ADIS16495_REG_SYNC_SCALE
> 	ADIS16480_REG(0x03, 0x10)
> > +#define ADIS16495_REG_BURST_CMD
> 	ADIS16480_REG(0x00, 0x7C)
> > +#define ADIS16495_BURST_ID			0xA5A5
> > +/* total number of segments in burst */
> > +#define ADIS16495_BURST_MAX_DATA		20
> > +/* spi max speed in burst mode */
> > +#define ADIS16495_BURST_MAX_SPEED              6000000
> >
> >  #define ADIS16480_REG_SERIAL_NUM
> 	ADIS16480_REG(0x04, 0x20)
> >
> > @@ -163,6 +173,8 @@ struct adis16480 {
> >  	struct clk *ext_clk;
> >  	enum adis16480_clock_mode clk_mode;
> >  	unsigned int clk_freq;
> > +	/* Alignment needed for the timestamp */
> > +	__be16 data[ADIS16495_BURST_MAX_DATA] __aligned(8);
> >  };
> >
> >  static const char * const adis16480_int_pin_names[4] = {
> > @@ -863,7 +875,7 @@ static const char * const
> adis16480_status_error_msgs[] = {
> >
> >  static int adis16480_enable_irq(struct adis *adis, bool enable);
> >
> > -#define ADIS16480_DATA(_prod_id, _timeouts)
> 	\
> > +#define ADIS16480_DATA(_prod_id, _timeouts, _burst_len)
> 		\
> >  {									\
> >  	.diag_stat_reg = ADIS16480_REG_DIAG_STS,			\
> >  	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,
> 		\
> > @@ -887,6 +899,8 @@ static int adis16480_enable_irq(struct adis
> *adis, bool enable);
> >  		BIT(ADIS16480_DIAG_STAT_BARO_FAIL),
> 	\
> >  	.enable_irq = adis16480_enable_irq,				\
> >  	.timeouts = (_timeouts),					\
> > +	.burst_reg_cmd = ADIS16495_REG_BURST_CMD,
> 	\
> > +	.burst_len = (_burst_len),					\
> >  }
> >
> >  static const struct adis_timeout adis16485_timeouts = {
> > @@ -931,7 +945,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.int_clk = 2460000,
> >  		.max_dec_rate = 2048,
> >  		.filter_freqs = adis16480_def_filter_freqs,
> > -		.adis_data = ADIS16480_DATA(16375,
> &adis16485_timeouts),
> > +		.adis_data = ADIS16480_DATA(16375,
> &adis16485_timeouts, 0),
> >  	},
> >  	[ADIS16480] = {
> >  		.channels = adis16480_channels,
> > @@ -944,7 +958,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.int_clk = 2460000,
> >  		.max_dec_rate = 2048,
> >  		.filter_freqs = adis16480_def_filter_freqs,
> > -		.adis_data = ADIS16480_DATA(16480,
> &adis16480_timeouts),
> > +		.adis_data = ADIS16480_DATA(16480,
> &adis16480_timeouts, 0),
> >  	},
> >  	[ADIS16485] = {
> >  		.channels = adis16485_channels,
> > @@ -957,7 +971,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.int_clk = 2460000,
> >  		.max_dec_rate = 2048,
> >  		.filter_freqs = adis16480_def_filter_freqs,
> > -		.adis_data = ADIS16480_DATA(16485,
> &adis16485_timeouts),
> > +		.adis_data = ADIS16480_DATA(16485,
> &adis16485_timeouts, 0),
> >  	},
> >  	[ADIS16488] = {
> >  		.channels = adis16480_channels,
> > @@ -970,7 +984,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.int_clk = 2460000,
> >  		.max_dec_rate = 2048,
> >  		.filter_freqs = adis16480_def_filter_freqs,
> > -		.adis_data = ADIS16480_DATA(16488,
> &adis16485_timeouts),
> > +		.adis_data = ADIS16480_DATA(16488,
> &adis16485_timeouts, 0),
> >  	},
> >  	[ADIS16490] = {
> >  		.channels = adis16485_channels,
> > @@ -984,7 +998,7 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16490,
> &adis16495_timeouts),
> > +		.adis_data = ADIS16480_DATA(16490,
> &adis16495_timeouts, 0),
> >  	},
> >  	[ADIS16495_1] = {
> >  		.channels = adis16485_channels,
> > @@ -998,7 +1012,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts),
> > +		/* 20 elements of 16bits */
> > +		.adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> >  	},
> >  	[ADIS16495_2] = {
> >  		.channels = adis16485_channels,
> > @@ -1012,7 +1028,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts),
> > +		/* 20 elements of 16bits */
> > +		.adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> >  	},
> >  	[ADIS16495_3] = {
> >  		.channels = adis16485_channels,
> > @@ -1026,7 +1044,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts),
> > +		/* 20 elements of 16bits */
> > +		.adis_data = ADIS16480_DATA(16495,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> >  	},
> >  	[ADIS16497_1] = {
> >  		.channels = adis16485_channels,
> > @@ -1040,7 +1060,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts),
> > +		/* 20 elements of 16bits */
> > +		.adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> >  	},
> >  	[ADIS16497_2] = {
> >  		.channels = adis16485_channels,
> > @@ -1054,7 +1076,9 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts),
> > +		/* 20 elements of 16bits */
> > +		.adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> >  	},
> >  	[ADIS16497_3] = {
> >  		.channels = adis16485_channels,
> > @@ -1068,10 +1092,121 @@ static const struct adis16480_chip_info
> adis16480_chip_info[] = {
> >  		.max_dec_rate = 4250,
> >  		.filter_freqs = adis16495_def_filter_freqs,
> >  		.has_pps_clk_mode = true,
> > -		.adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts),
> > +		/* 20 elements of 16bits */
> > +		.adis_data = ADIS16480_DATA(16497,
> &adis16495_1_timeouts,
> > +
> ADIS16495_BURST_MAX_DATA * 2),
> >  	},
> >  };
> >
> > +static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem,
> const u32 crc)
> > +{
> > +	u32 crc_calc;
> > +	u16 crc_buf[15];
> > +	int j;
> > +
> > +	for (j = 0; j < n_elem; j++)
> > +		crc_buf[j] = swab16(buf[j]);
> > +
> > +	crc_calc = crc32(~0, crc_buf, n_elem * 2);
> > +	crc_calc ^= ~0;
> > +
> > +	return (crc == crc_calc);
> > +}
> > +
> > +static irqreturn_t adis16480_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct adis16480 *st = iio_priv(indio_dev);
> > +	struct adis *adis = &st->adis;
> > +	int ret, bit, offset, i = 0;
> > +	__be16 *buffer;
> > +	u32 crc;
> > +	bool valid;
> > +	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
> > +
> > +	adis_dev_lock(adis);
> > +	if (adis->current_page != 0) {
> > +		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> > +		adis->tx[1] = 0;
> > +		ret = spi_write(adis->spi, adis->tx, 2);
> > +		if (ret) {
> > +			dev_err(&adis->spi->dev, "Failed to change
> device page: %d\n", ret);
> > +			adis_dev_unlock(adis);
> > +			return ret;
> 
> This is an interrupt handler, you should be careful what you return
> as they will be treated as irqreturn_t not ints.
> 
> return IRQ_HANDLED even in error paths.

Hmm, yeah, this is definitely not ok. Also imposes the question if we should
call ' iio_trigger_notify_done()' in these error paths? I'm pending to do it as
it might be a big assumption to say the device is 'broken' if some spi transfer
fails...

Not doing it means we will never receive another irq (I think this is also true if
we do not return IRQ_HANDLED)...

Also need to check other places as I'm fairly sure we have this problem (at least)
in the adis16475 driver...
> > +		}
> > +	}
> > +
> > +	adis->spi->max_speed_hz = ADIS16495_BURST_MAX_SPEED;
> > +
> > +	ret = spi_sync(adis->spi, &adis->msg);
> > +	if (ret) {
> > +		dev_err(&adis->spi->dev, "Failed to read data: %d\n",
> ret);
> > +		adis_dev_unlock(adis);
> > +		return ret;
> > +	}
> > +
> > +	adis->spi->max_speed_hz = cached_spi_speed_hz;
> > +	adis->current_page = 0;
> 
> Does it make more sense to move this to just after we changed the
> page?

Yes, it does. If the second spi transfer fails, we already moved to page 0
but did not updated this variable...

- Nuno Sá

Re: [PATCH v2] iio: adis16480: support burst read function

[Jonathan Cameron]

...
> > > +static irqreturn_t adis16480_trigger_handler(int irq, void *p)
> > > +{
> > > +	struct iio_poll_func *pf = p;
> > > +	struct iio_dev *indio_dev = pf->indio_dev;
> > > +	struct adis16480 *st = iio_priv(indio_dev);
> > > +	struct adis *adis = &st->adis;
> > > +	int ret, bit, offset, i = 0;
> > > +	__be16 *buffer;
> > > +	u32 crc;
> > > +	bool valid;
> > > +	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
> > > +
> > > +	adis_dev_lock(adis);
> > > +	if (adis->current_page != 0) {
> > > +		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> > > +		adis->tx[1] = 0;
> > > +		ret = spi_write(adis->spi, adis->tx, 2);
> > > +		if (ret) {
> > > +			dev_err(&adis->spi->dev, "Failed to change  
> > device page: %d\n", ret);  
> > > +			adis_dev_unlock(adis);
> > > +			return ret;  
> > 
> > This is an interrupt handler, you should be careful what you return
> > as they will be treated as irqreturn_t not ints.
> > 
> > return IRQ_HANDLED even in error paths.  
> 
> Hmm, yeah, this is definitely not ok. Also imposes the question if we should
> call ' iio_trigger_notify_done()' in these error paths? I'm pending to do it as
> it might be a big assumption to say the device is 'broken' if some spi transfer
> fails...

Yup, that has always been a bit of an open question in drivers. As likely
as not, any breakage leaves the device in a state from which we can't recover
anyway.  I've mostly left whether to call iio_trigger_notify_done() to the
discretion of the driver writers.

> 
> Not doing it means we will never receive another irq (I think this is also true if
> we do not return IRQ_HANDLED)...
> 
> Also need to check other places as I'm fairly sure we have this problem (at least)
> in the adis16475 driver...
oops. Guess I missed it there ;)
> > > +		}
> > > +	}
> > > +
> > > +	adis->spi->max_speed_hz = ADIS16495_BURST_MAX_SPEED;
> > > +
> > > +	ret = spi_sync(adis->spi, &adis->msg);
> > > +	if (ret) {
> > > +		dev_err(&adis->spi->dev, "Failed to read data: %d\n",  
> > ret);  
> > > +		adis_dev_unlock(adis);
> > > +		return ret;
> > > +	}
> > > +
> > > +	adis->spi->max_speed_hz = cached_spi_speed_hz;
> > > +	adis->current_page = 0;  
> > 
> > Does it make more sense to move this to just after we changed the
> > page?  
> 
> Yes, it does. If the second spi transfer fails, we already moved to page 0
> but did not updated this variable...
> 
> - Nuno Sá