[PATCH 1/2] iio: imu: adis16480: Add support for burst read function

[PATCH 1/2] iio: imu: adis16480: Add support for burst read function

[Alexandru Ardelean]

From: Stefan Popa <stefan.popa@analog.com>

The burst read function (BRF) provides a method for reading a batch of
data (status, temperature, gyroscopes, accelerometers, timestamp/data
counter and CRC code), which does not require a stall time between each
16-bit sgment and only requires on command on the DIN line to initiate.

The BRF contains either 19 or 20 different data segments of 16-bits
each. After making the burst request, the response can have one or two
"don't care" 16-bit responses before the BURST_ID. This is why the
received burst sequence needs to be followed until the BURST_ID is
detected, which indicates that the data will be contained in the
following 17 segments of 16-bits each. Temperature is the first data
channel in the sequence.

Currently, the burst function is activated by default for all devices
that support it. A future patch will offer the option to enable/disable
this feature from userspace.

Also, the CRC code is not checked. This will be added in a future patch
as well.

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 106 +++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 1eb4f98076f1..9b100c8fb744 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -23,6 +23,9 @@
 #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>
 
 #define ADIS16480_PAGE_SIZE 0x80
@@ -101,6 +104,9 @@
  * 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
+#define ADIS16495_BURST_MAX_DATA		20
 
 #define ADIS16480_REG_SERIAL_NUM		ADIS16480_REG(0x04, 0x20)
 
@@ -138,6 +144,7 @@ struct adis16480_chip_info {
 	unsigned int max_dec_rate;
 	const unsigned int *filter_freqs;
 	bool has_pps_clk_mode;
+	struct adis_burst *burst;
 	const struct adis_data adis_data;
 };
 
@@ -163,6 +170,20 @@ struct adis16480 {
 	unsigned int clk_freq;
 };
 
+static struct adis_burst adis16495_burst = {
+	.en = true,
+	.reg_cmd = ADIS16495_REG_BURST_CMD,
+	/*
+	 * adis_update_scan_mode_burst() sets the burst length in respect with
+	 * the number of channels and allocates 16 bits for each. However, for
+	 * adis1649x devices, the data for each channel is composed of a 16-bit
+	 * low and 16-bit high part. Besides this, the burst sequence contains
+	 * data for BURST_ID, SYS_E_FLAG, TIME_STAMP, CRC_LWR, CRC_UPR, one or
+	 * two don't care segments.
+	 */
+	.extra_len = 12 * sizeof(u16),
+};
+
 static const char * const adis16480_int_pin_names[4] = {
 	[ADIS16480_PIN_DIO1] = "DIO1",
 	[ADIS16480_PIN_DIO2] = "DIO2",
@@ -953,6 +974,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,
+		.burst = &adis16495_burst,
 		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
 	},
 	[ADIS16495_2] = {
@@ -967,6 +989,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,
+		.burst = &adis16495_burst,
 		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
 	},
 	[ADIS16495_3] = {
@@ -981,6 +1004,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,
+		.burst = &adis16495_burst,
 		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
 	},
 	[ADIS16497_1] = {
@@ -995,6 +1019,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,
+		.burst = &adis16495_burst,
 		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
 	},
 	[ADIS16497_2] = {
@@ -1009,6 +1034,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,
+		.burst = &adis16495_burst,
 		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
 	},
 	[ADIS16497_3] = {
@@ -1023,10 +1049,81 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.burst = &adis16495_burst,
 		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
 	},
 };
 
+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 data[ADIS16495_BURST_MAX_DATA], *buffer, *d;
+
+	if (!adis->buffer)
+		return -ENOMEM;
+
+	mutex_lock(&adis->state_lock);
+	if (adis->current_page != 0) {
+		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
+		adis->tx[1] = 0;
+		spi_write(adis->spi, adis->tx, 2);
+	}
+
+	ret = spi_sync(adis->spi, &adis->msg);
+	if (ret)
+		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
+
+	adis->current_page = 0;
+	mutex_unlock(&adis->state_lock);
+
+	if (!(adis->burst && adis->burst->en)) {
+		buffer = adis->buffer;
+		goto push_to_buffers;
+	}
+	/*
+	 * After making the burst request, the response can have one or two
+	 * "don't care" 16-bit responses, before the BURST_ID.
+	 */
+	d = (__be16 *)adis->buffer;
+	for (offset = 0; offset < 3; offset++) {
+		if (d[offset] == ADIS16495_BURST_ID) {
+			offset += 2; /* TEMP_OUT */
+			break;
+		}
+	}
+
+	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.
+		 */
+		if (bit == ADIS16480_SCAN_TEMP) {
+			data[2 * i] = d[offset];
+		} else {
+			/* The lower register data is sequenced first */
+			data[2 * i] = d[2 * bit + offset + 2];
+			data[2 * i + 1] = d[2 * bit + offset + 1];
+		}
+		i++;
+	}
+
+	buffer = data;
+
+push_to_buffers:
+	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+		pf->timestamp);
+
+	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,
@@ -1264,7 +1361,14 @@ static int adis16480_probe(struct spi_device *spi)
 		st->clk_freq = st->chip_info->int_clk;
 	}
 
-	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
+	/* If burst mode is supported, enable it by default */
+	if (st->chip_info->burst) {
+		st->adis.burst = st->chip_info->burst;
+		st->adis.burst_extra_len = st->chip_info->burst->extra_len;
+	}
+
+	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev,
+					    adis16480_trigger_handler);
 	if (ret)
 		goto error_clk_disable_unprepare;
 
-- 
2.17.1

[PATCH 2/2] iio: imu: adis16480: Add the option to enable/disable burst mode

[Alexandru Ardelean]

From: Stefan Popa <stefan.popa@analog.com>

Although the burst read function does not require a stall time between
each 16-bit segment, it however requires more processing since the
software needs to look for the BURST_ID and take into account the offset
to the first data channel. Some users might find it useful to be able to
switch between the two modes.

Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 9b100c8fb744..7d7712c33435 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -330,6 +330,45 @@ static int adis16480_debugfs_init(struct iio_dev *indio_dev)
 
 #endif
 
+static ssize_t adis16495_burst_mode_enable_get(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	struct adis16480 *st = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%d\n", st->adis.burst->en);
+}
+
+static ssize_t adis16495_burst_mode_enable_set(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t len)
+{
+	struct adis16480 *st = iio_priv(dev_to_iio_dev(dev));
+	bool val;
+	int ret;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	st->adis.burst->en = val;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(burst_mode_enable, 0644,
+		       adis16495_burst_mode_enable_get,
+		       adis16495_burst_mode_enable_set, 0);
+
+static struct attribute *adis16495_attributes[] = {
+	&iio_dev_attr_burst_mode_enable.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group adis16495_attribute_group = {
+	.attrs = adis16495_attributes,
+};
+
 static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 {
 	struct adis16480 *st = iio_priv(indio_dev);
@@ -1131,6 +1170,14 @@ static const struct iio_info adis16480_info = {
 	.debugfs_reg_access = adis_debugfs_reg_access,
 };
 
+static const struct iio_info adis16495_info = {
+	.attrs = &adis16495_attribute_group,
+	.read_raw = &adis16480_read_raw,
+	.write_raw = &adis16480_write_raw,
+	.update_scan_mode = adis_update_scan_mode,
+	.debugfs_reg_access = adis_debugfs_reg_access,
+};
+
 static int adis16480_stop_device(struct iio_dev *indio_dev)
 {
 	struct adis16480 *st = iio_priv(indio_dev);
@@ -1365,6 +1412,7 @@ static int adis16480_probe(struct spi_device *spi)
 	if (st->chip_info->burst) {
 		st->adis.burst = st->chip_info->burst;
 		st->adis.burst_extra_len = st->chip_info->burst->extra_len;
+		indio_dev->info = &adis16495_info;
 	}
 
 	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev,
-- 
2.17.1

Re: [PATCH 1/2] iio: imu: adis16480: Add support for burst read function

[Andy Shevchenko]

On Tue, Aug 4, 2020 at 1:04 PM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:

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

It makes it easier to understand the code if the assignment will be
attached to the actual user, i.e. for_each_set_bit() below.

> +       __be16 data[ADIS16495_BURST_MAX_DATA], *buffer, *d;

> +       if (!adis->buffer)
> +               return -ENOMEM;

Is it possible?

> +       mutex_lock(&adis->state_lock);
> +       if (adis->current_page != 0) {
> +               adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> +               adis->tx[1] = 0;
> +               spi_write(adis->spi, adis->tx, 2);
> +       }
> +
> +       ret = spi_sync(adis->spi, &adis->msg);
> +       if (ret)

> +               dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);

No bail out? Then it's dev_warn().

> +       adis->current_page = 0;
> +       mutex_unlock(&adis->state_lock);
> +
> +       if (!(adis->burst && adis->burst->en)) {
> +               buffer = adis->buffer;
> +               goto push_to_buffers;
> +       }
> +       /*
> +        * After making the burst request, the response can have one or two
> +        * "don't care" 16-bit responses, before the BURST_ID.
> +        */
> +       d = (__be16 *)adis->buffer;
> +       for (offset = 0; offset < 3; offset++) {
> +               if (d[offset] == ADIS16495_BURST_ID) {
> +                       offset += 2; /* TEMP_OUT */
> +                       break;
> +               }
> +       }

So, we can end up with offset == 3 here. Is it by design?
Otherwise, offset+=2 maybe moved out of the loop.
And in any case comment is needed.

> +       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.
> +                */
> +               if (bit == ADIS16480_SCAN_TEMP) {
> +                       data[2 * i] = d[offset];
> +               } else {
> +                       /* The lower register data is sequenced first */

> +                       data[2 * i] = d[2 * bit + offset + 2];

I would do ' + 0', but it's up to you.

> +                       data[2 * i + 1] = d[2 * bit + offset + 1];
> +               }
> +               i++;
> +       }
> +
> +       buffer = data;
> +
> +push_to_buffers:
> +       iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> +               pf->timestamp);
> +
> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}

...

> +       /* If burst mode is supported, enable it by default */
> +       if (st->chip_info->burst) {
> +               st->adis.burst = st->chip_info->burst;
> +               st->adis.burst_extra_len = st->chip_info->burst->extra_len;
> +       }

You may use it directly (the sizeof struct somelike 16 bytes and one
pointer on a 64-bit machine is 8, simply embed it into the struct and
assign it always.

...

+       .extra_len = 12 * sizeof(u16),

__be16 would be precise?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: imu: adis16480: Add support for burst read function

[Jonathan Cameron]

On Tue, 4 Aug 2020 13:04:13 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Stefan Popa <stefan.popa@analog.com>
> 
> The burst read function (BRF) provides a method for reading a batch of
> data (status, temperature, gyroscopes, accelerometers, timestamp/data
> counter and CRC code), which does not require a stall time between each
> 16-bit sgment and only requires on command on the DIN line to initiate.
> 
> The BRF contains either 19 or 20 different data segments of 16-bits
> each. After making the burst request, the response can have one or two
> "don't care" 16-bit responses before the BURST_ID. This is why the
> received burst sequence needs to be followed until the BURST_ID is
> detected, which indicates that the data will be contained in the
> following 17 segments of 16-bits each. Temperature is the first data
> channel in the sequence.
> 
> Currently, the burst function is activated by default for all devices
> that support it. A future patch will offer the option to enable/disable
> this feature from userspace.
That is problematic if you mean an explicit control.  We considered that
for similar cases in the past, and ruled it out as an interface that
is hard for userspace to know what to do with it.

As such, best option is to try and estimate whether it will help for
a given set of channels.  If it doesn't then don't use it :)

A few comments inline to add to Andy's ones.

Thanks,

Jonathan

> 
> Also, the CRC code is not checked. This will be added in a future patch
> as well.
> 
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/imu/adis16480.c | 106 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 1eb4f98076f1..9b100c8fb744 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -23,6 +23,9 @@
>  #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>
>  
>  #define ADIS16480_PAGE_SIZE 0x80
> @@ -101,6 +104,9 @@
>   * 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
> +#define ADIS16495_BURST_MAX_DATA		20
>  
>  #define ADIS16480_REG_SERIAL_NUM		ADIS16480_REG(0x04, 0x20)
>  
> @@ -138,6 +144,7 @@ struct adis16480_chip_info {
>  	unsigned int max_dec_rate;
>  	const unsigned int *filter_freqs;
>  	bool has_pps_clk_mode;
> +	struct adis_burst *burst;
>  	const struct adis_data adis_data;
>  };
>  
> @@ -163,6 +170,20 @@ struct adis16480 {
>  	unsigned int clk_freq;
>  };
>  
> +static struct adis_burst adis16495_burst = {
> +	.en = true,
> +	.reg_cmd = ADIS16495_REG_BURST_CMD,
> +	/*
> +	 * adis_update_scan_mode_burst() sets the burst length in respect with
> +	 * the number of channels and allocates 16 bits for each. However, for
> +	 * adis1649x devices, the data for each channel is composed of a 16-bit
> +	 * low and 16-bit high part. Besides this, the burst sequence contains
> +	 * data for BURST_ID, SYS_E_FLAG, TIME_STAMP, CRC_LWR, CRC_UPR, one or
> +	 * two don't care segments.
> +	 */
> +	.extra_len = 12 * sizeof(u16),
> +};
> +
>  static const char * const adis16480_int_pin_names[4] = {
>  	[ADIS16480_PIN_DIO1] = "DIO1",
>  	[ADIS16480_PIN_DIO2] = "DIO2",
> @@ -953,6 +974,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,
> +		.burst = &adis16495_burst,
>  		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
>  	},
>  	[ADIS16495_2] = {
> @@ -967,6 +989,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,
> +		.burst = &adis16495_burst,
>  		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
>  	},
>  	[ADIS16495_3] = {
> @@ -981,6 +1004,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,
> +		.burst = &adis16495_burst,
>  		.adis_data = ADIS16480_DATA(16495, &adis16495_1_timeouts),
>  	},
>  	[ADIS16497_1] = {
> @@ -995,6 +1019,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,
> +		.burst = &adis16495_burst,
>  		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
>  	},
>  	[ADIS16497_2] = {
> @@ -1009,6 +1034,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,
> +		.burst = &adis16495_burst,
>  		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
>  	},
>  	[ADIS16497_3] = {
> @@ -1023,10 +1049,81 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
>  		.max_dec_rate = 4250,
>  		.filter_freqs = adis16495_def_filter_freqs,
>  		.has_pps_clk_mode = true,
> +		.burst = &adis16495_burst,
>  		.adis_data = ADIS16480_DATA(16497, &adis16495_1_timeouts),
>  	},
>  };
>  
> +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 data[ADIS16495_BURST_MAX_DATA], *buffer, *d;
> +
> +	if (!adis->buffer)
> +		return -ENOMEM;
> +
> +	mutex_lock(&adis->state_lock);
> +	if (adis->current_page != 0) {
> +		adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> +		adis->tx[1] = 0;
> +		spi_write(adis->spi, adis->tx, 2);
> +	}
> +
> +	ret = spi_sync(adis->spi, &adis->msg);
> +	if (ret)
> +		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
> +
> +	adis->current_page = 0;
> +	mutex_unlock(&adis->state_lock);
> +
> +	if (!(adis->burst && adis->burst->en)) {
> +		buffer = adis->buffer;
> +		goto push_to_buffers;
> +	}
> +	/*
> +	 * After making the burst request, the response can have one or two
> +	 * "don't care" 16-bit responses, before the BURST_ID.

That's nasty... :)

> +	 */
> +	d = (__be16 *)adis->buffer;
> +	for (offset = 0; offset < 3; offset++) {

If it's one or two, start at one!

> +		if (d[offset] == ADIS16495_BURST_ID) {
> +			offset += 2; /* TEMP_OUT */
> +			break;
> +		}
> +	}
> +
> +	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.
> +		 */
> +		if (bit == ADIS16480_SCAN_TEMP) {
> +			data[2 * i] = d[offset];
> +		} else {
> +			/* The lower register data is sequenced first */
> +			data[2 * i] = d[2 * bit + offset + 2];
> +			data[2 * i + 1] = d[2 * bit + offset + 1];
> +		}
> +		i++;
> +	}

> +
> +	buffer = data;
> +
> +push_to_buffers:
> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,

This function requires the whole buffer to be aligned suitable for putting
an 8 byte timestamp in at a naturally aligned location.  Basically
that means you have to mark buffer __aligned(8).  There are lots
of broken cases of this in IIO and for most of them there have been
patches on the list.


> +		pf->timestamp);
> +
> +	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,
> @@ -1264,7 +1361,14 @@ static int adis16480_probe(struct spi_device *spi)
>  		st->clk_freq = st->chip_info->int_clk;
>  	}
>  
> -	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
> +	/* If burst mode is supported, enable it by default */
> +	if (st->chip_info->burst) {
> +		st->adis.burst = st->chip_info->burst;
> +		st->adis.burst_extra_len = st->chip_info->burst->extra_len;
> +	}
> +
> +	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> +					    adis16480_trigger_handler);
>  	if (ret)
>  		goto error_clk_disable_unprepare;
>  

Re: [PATCH 2/2] iio: imu: adis16480: Add the option to enable/disable burst mode

[Jonathan Cameron]

On Tue, 4 Aug 2020 13:04:14 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Stefan Popa <stefan.popa@analog.com>
> 
> Although the burst read function does not require a stall time between
> each 16-bit segment, it however requires more processing since the
> software needs to look for the BURST_ID and take into account the offset
> to the first data channel. Some users might find it useful to be able to
> switch between the two modes.

Ah, when you say future patch, you meant extremely near future. :)

> 
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

First rule of new ABI, document it.

As mentioned in previous patch, this sort of user interface is very hard to
use. You are much better off estimating whether it is a good idea or
not for a given set of channels.   If it isn't don't use it, if
it is turn it on.
There is compelling reason to let users choose that I can think of...


Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 48 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 9b100c8fb744..7d7712c33435 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -330,6 +330,45 @@ static int adis16480_debugfs_init(struct iio_dev *indio_dev)
>  
>  #endif
>  
> +static ssize_t adis16495_burst_mode_enable_get(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct adis16480 *st = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", st->adis.burst->en);
> +}
> +
> +static ssize_t adis16495_burst_mode_enable_set(struct device *dev,
> +					       struct device_attribute *attr,
> +					       const char *buf, size_t len)
> +{
> +	struct adis16480 *st = iio_priv(dev_to_iio_dev(dev));
> +	bool val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	st->adis.burst->en = val;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(burst_mode_enable, 0644,
> +		       adis16495_burst_mode_enable_get,
> +		       adis16495_burst_mode_enable_set, 0);
> +
> +static struct attribute *adis16495_attributes[] = {
> +	&iio_dev_attr_burst_mode_enable.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group adis16495_attribute_group = {
> +	.attrs = adis16495_attributes,
> +};
> +
>  static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
>  {
>  	struct adis16480 *st = iio_priv(indio_dev);
> @@ -1131,6 +1170,14 @@ static const struct iio_info adis16480_info = {
>  	.debugfs_reg_access = adis_debugfs_reg_access,
>  };
>  
> +static const struct iio_info adis16495_info = {
> +	.attrs = &adis16495_attribute_group,
> +	.read_raw = &adis16480_read_raw,
> +	.write_raw = &adis16480_write_raw,
> +	.update_scan_mode = adis_update_scan_mode,
> +	.debugfs_reg_access = adis_debugfs_reg_access,
> +};
> +
>  static int adis16480_stop_device(struct iio_dev *indio_dev)
>  {
>  	struct adis16480 *st = iio_priv(indio_dev);
> @@ -1365,6 +1412,7 @@ static int adis16480_probe(struct spi_device *spi)
>  	if (st->chip_info->burst) {
>  		st->adis.burst = st->chip_info->burst;
>  		st->adis.burst_extra_len = st->chip_info->burst->extra_len;
> +		indio_dev->info = &adis16495_info;
>  	}
>  
>  	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev,

Re: [PATCH 2/2] iio: imu: adis16480: Add the option to enable/disable burst mode

[Andy Shevchenko]

On Tue, Aug 4, 2020 at 1:04 PM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:
>
> From: Stefan Popa <stefan.popa@analog.com>
>
> Although the burst read function does not require a stall time between
> each 16-bit segment, it however requires more processing since the
> software needs to look for the BURST_ID and take into account the offset
> to the first data channel. Some users might find it useful to be able to
> switch between the two modes.

...

> +static struct attribute *adis16495_attributes[] = {
> +       &iio_dev_attr_burst_mode_enable.dev_attr.attr,

> +       NULL,

A nit: drop comma.

> +};

-- 
With Best Regards,
Andy Shevchenko