[PATCH 0/4] Refactor ADIS Burst Mode

[PATCH 0/4] Refactor ADIS Burst Mode

[Nuno Sá]

This series refactors the ADIS burst mode. The 2 main ideas of the
refactor are:

1. As discussed in previous patches, there's no point in
enabling/disabling burst mode at runtime. Hence, we can drop the `en`
variable.
2. Replace the `extra_len` by `burst_len` where users have now to
explicitly define the size of the burst buffer. The point is to remove
the following line from the lib:

```
/* All but the timestamp channel */
burst_length = (indio_dev->num_channels - 1) * sizeof(u16);
```

The library should not assume that a timestamp channel is defined.
Moreover, most parts also include some diagnostic data, crc, etc.. in
the burst buffer that needed to be included in an `extra_len` variable
which is not that nice. On top of this, some devices already start to
have some 32bit size channels ...

While doing this (and mainly when looking at the adis16400) drivers it
felt that the burst variables belong to the per chip `adis_data`
structure. As seen in the adis16400 driver, some drivers might support
multiple devices with different burst sizes.

For now, it does not feel necessary to wrap these variables in a
`adis_burst` structure but I don't see any problem in doing so if
required...

Nuno Sá (4):
  iio: adis: Move burst mode into adis_data
  iio: adis16400: Drop adis_burst usage
  iio: adis16475: Drop adis_burst usage
  iio: adis. Drop adis_burst struct

 drivers/iio/imu/adis16400.c   | 32 +++++++++++++-------------------
 drivers/iio/imu/adis16475.c   | 18 +++---------------
 drivers/iio/imu/adis_buffer.c | 12 +++++-------
 include/linux/iio/imu/adis.h  | 26 +++++++++-----------------
 4 files changed, 30 insertions(+), 58 deletions(-)

-- 
2.28.0

[PATCH 1/4] iio: adis: Move burst mode into adis_data

[Nuno Sá]

Add burst mode variables in the per device specific data structure. As
some drivers support multiple devices with different burst sizes it
makes sense this data to be in `adis_data`. While moving the variables,
there are two main differences:

1. The `en`variable is dropped. If a device supports burst mode, it will
just use it as it will has better performance for almost all real use
cases.
2. Replace `extra_len` by `burst_len`. Users should now explicitly
define the length of the burst buffer as it is typically constant. This
also allows to remove the following line from the library:

```
/* All but the timestamp channel */
burst_length = (indio_dev->num_channels - 1) * sizeof(u16);
```

The library should not assume that a timestamp channel is defined.
Moreover, most parts also include some diagnostic data, crc, etc.. in
the burst buffer that needed to be included in an `extra_len` variable
which is not that nice. On top of this, some devices already start to
have some 32bit size channels ...

This patch is also a move to completely drop the `struct adis_burst`
from the library.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 12 +++++-------
 include/linux/iio/imu/adis.h  |  9 +++++++++
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index df6144285470..ac354321f63a 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -26,12 +26,10 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
 	unsigned int burst_length, burst_max_length;
 	u8 *tx;
 
-	/* All but the timestamp channel */
-	burst_length = (indio_dev->num_channels - 1) * sizeof(u16);
-	burst_length += adis->burst->extra_len + adis->burst_extra_len;
+	burst_length = adis->data->burst_len + adis->burst_extra_len;
 
-	if (adis->burst->burst_max_len)
-		burst_max_length = adis->burst->burst_max_len;
+	if (adis->data->burst_max_len)
+		burst_max_length = adis->data->burst_max_len;
 	else
 		burst_max_length = burst_length;
 
@@ -47,7 +45,7 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
 	}
 
 	tx = adis->buffer + burst_max_length;
-	tx[0] = ADIS_READ_REG(adis->burst->reg_cmd);
+	tx[0] = ADIS_READ_REG(adis->data->burst_reg_cmd);
 	tx[1] = 0;
 
 	adis->xfer[0].tx_buf = tx;
@@ -76,7 +74,7 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
 	kfree(adis->xfer);
 	kfree(adis->buffer);
 
-	if (adis->burst && adis->burst->en)
+	if (adis->data->burst_len)
 		return adis_update_scan_mode_burst(indio_dev, scan_mask);
 
 	scan_count = indio_dev->scan_bytes / 2;
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 01ba691da2f3..c502ea3b9199 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -51,6 +51,11 @@ struct adis_timeout {
  * @timeouts: Chip specific delays
  * @enable_irq: Hook for ADIS devices that have a special IRQ enable/disable
  * @has_paging: True if ADIS device has paged registers
+ * @burst_reg_cmd:	Register command that triggers burst
+ * @burst_len:		Burst size in the SPI RX buffer. If @burst_max_len is defined,
+ *			this should be the minimum size supported by the device.
+ * @burst_max_len:	Holds the maximum burst size when the device supports
+ *			more than one burst mode with different sizes
  */
 struct adis_data {
 	unsigned int read_delay;
@@ -75,6 +80,10 @@ struct adis_data {
 	int (*enable_irq)(struct adis *adis, bool enable);
 
 	bool has_paging;
+
+	unsigned int burst_reg_cmd;
+	unsigned int burst_len;
+	unsigned int burst_max_len;
 };
 
 /**
-- 
2.28.0

[PATCH 2/4] iio: adis16400: Drop adis_burst usage

[Nuno Sá]

Burst mode variables are now part of the `adis_data` struct. The driver
also has now to explicitly define the length of the burst buffer.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 889c8c2a19f4..421b1988c1d4 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -317,11 +317,6 @@ enum adis16400_chip_variant {
 	ADIS16448,
 };
 
-static struct adis_burst adis16400_burst = {
-	.en = true,
-	.reg_cmd = ADIS16400_GLOB_CMD,
-};
-
 static int adis16334_get_freq(struct adis16400_state *st)
 {
 	int ret;
@@ -947,7 +942,7 @@ static const char * const adis16400_status_error_msgs[] = {
 	[ADIS16400_DIAG_STAT_POWER_LOW] = "Power supply below 4.75V",
 };
 
-#define ADIS16400_DATA(_timeouts)					\
+#define ADIS16400_DATA(_timeouts, _burst_len)				\
 {									\
 	.msc_ctrl_reg = ADIS16400_MSC_CTRL,				\
 	.glob_cmd_reg = ADIS16400_GLOB_CMD,				\
@@ -973,6 +968,8 @@ static const char * const adis16400_status_error_msgs[] = {
 		BIT(ADIS16400_DIAG_STAT_POWER_HIGH) |			\
 		BIT(ADIS16400_DIAG_STAT_POWER_LOW),			\
 	.timeouts = (_timeouts),					\
+	.burst_reg_cmd = ADIS16400_GLOB_CMD,				\
+	.burst_len = (_burst_len)					\
 }
 
 static const struct adis_timeout adis16300_timeouts = {
@@ -1023,7 +1020,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 140000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16300_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16300_timeouts, 18),
 	},
 	[ADIS16334] = {
 		.channels = adis16334_channels,
@@ -1036,7 +1033,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 67850, /* 25 C = 0x00 */
 		.set_freq = adis16334_set_freq,
 		.get_freq = adis16334_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16334_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16334_timeouts, 0),
 	},
 	[ADIS16350] = {
 		.channels = adis16350_channels,
@@ -1048,7 +1045,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.flags = ADIS16400_NO_BURST | ADIS16400_HAS_SLOW_MODE,
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16300_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16300_timeouts, 0),
 	},
 	[ADIS16360] = {
 		.channels = adis16350_channels,
@@ -1061,7 +1058,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16300_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16300_timeouts, 28),
 	},
 	[ADIS16362] = {
 		.channels = adis16350_channels,
@@ -1074,7 +1071,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16362_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16362_timeouts, 28),
 	},
 	[ADIS16364] = {
 		.channels = adis16350_channels,
@@ -1087,7 +1084,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16362_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16362_timeouts, 28),
 	},
 	[ADIS16367] = {
 		.channels = adis16350_channels,
@@ -1100,7 +1097,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16300_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16300_timeouts, 28),
 	},
 	[ADIS16400] = {
 		.channels = adis16400_channels,
@@ -1112,7 +1109,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 140000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16400_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16400_timeouts, 24),
 	},
 	[ADIS16445] = {
 		.channels = adis16445_channels,
@@ -1126,7 +1123,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 31000000 / 73860, /* 31 C = 0x00 */
 		.set_freq = adis16334_set_freq,
 		.get_freq = adis16334_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16445_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16445_timeouts, 16),
 	},
 	[ADIS16448] = {
 		.channels = adis16448_channels,
@@ -1140,7 +1137,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 31000000 / 73860, /* 31 C = 0x00 */
 		.set_freq = adis16334_set_freq,
 		.get_freq = adis16334_get_freq,
-		.adis_data = ADIS16400_DATA(&adis16448_timeouts),
+		.adis_data = ADIS16400_DATA(&adis16448_timeouts, 24),
 	}
 };
 
@@ -1196,9 +1193,6 @@ static int adis16400_probe(struct spi_device *spi)
 	if (!(st->variant->flags & ADIS16400_NO_BURST)) {
 		adis16400_setup_chan_mask(st);
 		indio_dev->available_scan_masks = st->avail_scan_mask;
-		st->adis.burst = &adis16400_burst;
-		if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
-			st->adis.burst_extra_len = sizeof(u16);
 	}
 
 	adis16400_data = &st->variant->adis_data;
-- 
2.28.0

[PATCH 3/4] iio: adis16475: Drop adis_burst usage

[Nuno Sá]

Burst mode variables are now part of the `adis_data` struct. The driver
also has now to explicitly define the length of the burst buffer.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 35d10ccb66c2..197d48240991 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -565,6 +565,9 @@ static int adis16475_enable_irq(struct adis *adis, bool enable)
 		BIT(ADIS16475_DIAG_STAT_CLK),				\
 	.enable_irq = adis16475_enable_irq,				\
 	.timeouts = (_timeouts),					\
+	.burst_reg_cmd = ADIS16475_REG_GLOB_CMD,			\
+	.burst_len = ADIS16475_BURST_MAX_DATA,				\
+	.burst_max_len = ADIS16475_BURST32_MAX_DATA			\
 }
 
 static const struct adis16475_sync adis16475_sync_mode[] = {
@@ -910,20 +913,6 @@ static const struct iio_info adis16475_info = {
 	.debugfs_reg_access = adis_debugfs_reg_access,
 };
 
-static struct adis_burst adis16475_burst = {
-	.en = true,
-	.reg_cmd = ADIS16475_REG_GLOB_CMD,
-	/*
-	 * adis_update_scan_mode_burst() sets the burst length in respect with
-	 * the number of channels and allocates 16 bits for each. However,
-	 * adis1647x devices also need space for DIAG_STAT, DATA_CNTR or
-	 * TIME_STAMP (depending on the clock mode but for us these bytes are
-	 * don't care...) and CRC.
-	 */
-	.extra_len = 3 * sizeof(u16),
-	.burst_max_len = ADIS16475_BURST32_MAX_DATA,
-};
-
 static bool adis16475_validate_crc(const u8 *buffer, u16 crc,
 				   const bool burst32)
 {
@@ -1279,7 +1268,6 @@ static int adis16475_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 	spi_set_drvdata(spi, indio_dev);
-	st->adis.burst = &adis16475_burst;
 
 	st->info = device_get_match_data(&spi->dev);
 	if (!st->info)
-- 
2.28.0

[PATCH 4/4] iio: adis. Drop adis_burst struct

[Nuno Sá]

As there are no users anymore of this structure, it can be safely
removed.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 include/linux/iio/imu/adis.h | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index c502ea3b9199..04e96d688ba9 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -20,7 +20,6 @@
 #define ADIS_REG_PAGE_ID 0x00
 
 struct adis;
-struct adis_burst;
 
 /**
  * struct adis_timeouts - ADIS chip variant timeouts
@@ -108,7 +107,6 @@ struct adis {
 	struct iio_trigger	*trig;
 
 	const struct adis_data	*data;
-	struct adis_burst	*burst;
 	unsigned int		burst_extra_len;
 	/**
 	 * The state_lock is meant to be used during operations that require
@@ -508,21 +506,6 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 
 #ifdef CONFIG_IIO_ADIS_LIB_BUFFER
 
-/**
- * struct adis_burst - ADIS data for burst transfers
- * @en			burst mode enabled
- * @reg_cmd		register command that triggers burst
- * @extra_len		extra length to account in the SPI RX buffer
- * @burst_max_len	holds the maximum burst size when the device supports
- *			more than one burst mode with different sizes
- */
-struct adis_burst {
-	bool		en;
-	unsigned int	reg_cmd;
-	const u32	extra_len;
-	const u32	burst_max_len;
-};
-
 int
 devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
 				   irq_handler_t trigger_handler);
-- 
2.28.0

Re: [PATCH 0/4] Refactor ADIS Burst Mode

[Jonathan Cameron]

On Thu, 17 Sep 2020 17:52:19 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> This series refactors the ADIS burst mode. The 2 main ideas of the
> refactor are:
> 
> 1. As discussed in previous patches, there's no point in
> enabling/disabling burst mode at runtime. Hence, we can drop the `en`
> variable.
> 2. Replace the `extra_len` by `burst_len` where users have now to
> explicitly define the size of the burst buffer. The point is to remove
> the following line from the lib:
> 
> ```
> /* All but the timestamp channel */
> burst_length = (indio_dev->num_channels - 1) * sizeof(u16);
> ```
> 
> The library should not assume that a timestamp channel is defined.
> Moreover, most parts also include some diagnostic data, crc, etc.. in
> the burst buffer that needed to be included in an `extra_len` variable
> which is not that nice. On top of this, some devices already start to
> have some 32bit size channels ...
> 
> While doing this (and mainly when looking at the adis16400) drivers it
> felt that the burst variables belong to the per chip `adis_data`
> structure. As seen in the adis16400 driver, some drivers might support
> multiple devices with different burst sizes.
> 
> For now, it does not feel necessary to wrap these variables in a
> `adis_burst` structure but I don't see any problem in doing so if
> required...

Looks good to me as it stands.

Series applied to the togreg branch of iio.git.

Thanks,

Jonathan

> 
> Nuno Sá (4):
>   iio: adis: Move burst mode into adis_data
>   iio: adis16400: Drop adis_burst usage
>   iio: adis16475: Drop adis_burst usage
>   iio: adis. Drop adis_burst struct
> 
>  drivers/iio/imu/adis16400.c   | 32 +++++++++++++-------------------
>  drivers/iio/imu/adis16475.c   | 18 +++---------------
>  drivers/iio/imu/adis_buffer.c | 12 +++++-------
>  include/linux/iio/imu/adis.h  | 26 +++++++++-----------------
>  4 files changed, 30 insertions(+), 58 deletions(-)
>