[PATCH v2 0/9] Adis IRQ fixes and minor improvements

[PATCH v2 0/9] Adis IRQ fixes and minor improvements

[Nuno Sa]

The primary goal of this series was to fix the return value on some
trigger handlers as spotted in [1]. While doing it, I found some minor
improvements that I think are simple enough to include in this series.

As for the first 2 patches, I opted to not do any functional change so
I'm keeping the 'if (!adis->buffer)' check. However, 'adis-buffer' is
allocated in 'update_scan_mode' hook which means we should be sure that
the buffer is allocated and the check is really not needed. I did not
went into the details but is there any way for the trigger handler to be
called before the 'update_scan_mode' hook? If not, I'm happy in sending
a v2 where we just remove the 'if'...


Changes in v2:
 * Remove the 'if' check for the allocated buffer;
 * Make sure the adis 'state_lock' is unlocked on error paths;
 * Fixed the commit message on the first 3 patches.
 * Dropped ("iio: adis16475: re-set max spi transfer") and added 3 new
patches (last 3 in the series ) to fix a potential race with the spi core
as discussed in [2].

[1]: https://marc.info/?l=linux-iio&m=161815197426882&w=2
[2]: https://marc.info/?l=linux-iio&m=161884696722142&w=2

Nuno Sa (9):
  iio: adis_buffer: do not return ints in irq handlers
  iio: adis16400: do not return ints in irq handlers
  iio: adis16475: do not return ints in irq handlers
  iio: adis_buffer: check return value on page change
  iio: adis_buffer: don't push data to buffers on failure
  iio: adis_buffer: update device page after changing it
  iio: adis: add burst_max_speed_hz variable
  iio: adis16475: do not directly change spi 'max_speed_hz'
  iio: adis16400: do not directly change spi 'max_speed_hz'

 drivers/iio/imu/adis16400.c   | 18 ++----------------
 drivers/iio/imu/adis16475.c   |  9 +++------
 drivers/iio/imu/adis_buffer.c | 28 +++++++++++++++++++---------
 include/linux/iio/imu/adis.h  |  2 ++
 4 files changed, 26 insertions(+), 31 deletions(-)

-- 
2.31.1

[PATCH v2 1/9] iio: adis_buffer: do not return ints in irq handlers

[Nuno Sa]

On an IRQ handler we should not return normal error codes as 'irqreturn_t'
is expected.

Fixes: ccd2b52f4ac69 ("staging:iio: Add common ADIS library")
Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index ac354321f63a..175af154e443 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -129,9 +129,6 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	struct adis *adis = iio_device_get_drvdata(indio_dev);
 	int ret;
 
-	if (!adis->buffer)
-		return -ENOMEM;
-
 	if (adis->data->has_paging) {
 		mutex_lock(&adis->state_lock);
 		if (adis->current_page != 0) {
-- 
2.31.1

[PATCH v2 2/9] iio: adis16400: do not return ints in irq handlers

[Nuno Sa]

On an IRQ handler we should not return normal error codes as 'irqreturn_t'
is expected.

Fixes: 5eda3550a3cc1 ("staging:iio:adis16400: Preallocate transfer message")
Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 768aa493a1a6..b2f92b55b910 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -645,9 +645,6 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
 	void *buffer;
 	int ret;
 
-	if (!adis->buffer)
-		return -ENOMEM;
-
 	if (!(st->variant->flags & ADIS16400_NO_BURST) &&
 		st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
 		st->adis.spi->max_speed_hz = ADIS16400_SPI_BURST;
-- 
2.31.1

[PATCH v2 3/9] iio: adis16475: do not return ints in irq handlers

[Nuno Sa]

On an IRQ handler we should return normal error codes as 'irqreturn_t'
is expected.

Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 1de62fc79e0f..51b76444db0b 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -1068,7 +1068,7 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
 
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret)
-		return ret;
+		goto check_burst32;
 
 	adis->spi->max_speed_hz = cached_spi_speed_hz;
 	buffer = adis->buffer;
-- 
2.31.1

[PATCH v2 4/9] iio: adis_buffer: check return value on page change

[Nuno Sa]

On the trigger handler, we might need to change the device page. Hence,
we should check the return value from 'spi_write()' and act accordingly.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 175af154e443..0ae551a748eb 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -134,7 +134,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 		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_write(adis->spi, adis->tx, 2);
+			if (ret) {
+				dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
+				mutex_unlock(&adis->state_lock);
+				goto irq_done;
+			}
 		}
 	}
 
@@ -151,6 +156,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
 		pf->timestamp);
 
+irq_done:
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
-- 
2.31.1

[PATCH v2 5/9] iio: adis_buffer: don't push data to buffers on failure

[Nuno Sa]

There's no point in pushing data to IIO buffers in case 'spi_sync()'
fails.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 0ae551a748eb..a29d22f657ce 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -144,9 +144,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	}
 
 	ret = spi_sync(adis->spi, &adis->msg);
-	if (ret)
+	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
-
+		if (adis->data->has_paging)
+			mutex_unlock(&adis->state_lock);
+		goto irq_done;
+	}
 
 	if (adis->data->has_paging) {
 		adis->current_page = 0;
-- 
2.31.1

[PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Nuno Sa]

With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to buffers on
failure"), we return if 'spi_sync()' fails which would leave
'adis->current_page' in an incoherent state. Hence, set this variable
right after we change the device page.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index a29d22f657ce..dda367071980 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 				mutex_unlock(&adis->state_lock);
 				goto irq_done;
 			}
+
+			adis->current_page = 0;
 		}
 	}
 
@@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 		goto irq_done;
 	}
 
-	if (adis->data->has_paging) {
-		adis->current_page = 0;
+	if (adis->data->has_paging)
 		mutex_unlock(&adis->state_lock);
-	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
 		pf->timestamp);
-- 
2.31.1

[PATCH v2 7/9] iio: adis: add burst_max_speed_hz variable

[Nuno Sa]

Typically, in burst mode, the device cannot operate at it's full spi
speed. Hence, the spi transfers for burst mode have to take this into
account. With this change we avoid a potential race with the spi core as
drivers were 'hacking' the device 'max_speed_hz' directly in the
trigger handler.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis_buffer.c | 4 ++++
 include/linux/iio/imu/adis.h  | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index dda367071980..82239da2f441 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -51,9 +51,13 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
 	adis->xfer[0].tx_buf = tx;
 	adis->xfer[0].bits_per_word = 8;
 	adis->xfer[0].len = 2;
+	if (adis->data->burst_max_speed_hz)
+		adis->xfer[0].speed_hz = adis->data->burst_max_speed_hz;
 	adis->xfer[1].rx_buf = adis->buffer;
 	adis->xfer[1].bits_per_word = 8;
 	adis->xfer[1].len = burst_length;
+	if (adis->data->burst_max_speed_hz)
+		adis->xfer[1].speed_hz = adis->data->burst_max_speed_hz;
 
 	spi_message_init(&adis->msg);
 	spi_message_add_tail(&adis->xfer[0], &adis->msg);
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index f9b728d490b1..cf49997d5903 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -55,6 +55,7 @@ struct adis_timeout {
  *			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
+ * @burst_max_speed_hz:	Maximum spi speed that can be used in burst mode
  */
 struct adis_data {
 	unsigned int read_delay;
@@ -83,6 +84,7 @@ struct adis_data {
 	unsigned int burst_reg_cmd;
 	unsigned int burst_len;
 	unsigned int burst_max_len;
+	unsigned int burst_max_speed_hz;
 };
 
 /**
-- 
2.31.1

[PATCH v2 8/9] iio: adis16475: do not directly change spi 'max_speed_hz'

[Nuno Sa]

With commit 65f2f661e71d ("iio: adis: add burst_max_speed_hz variable"), we
just need to define 'burst_max_speed_hz' and the adis core will take
care of setting up the spi transfers for burst mode. Hence, we fix
a potential race with the spi core where we could be left with an
invalid 'max_speed_hz'.

Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16475.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
index 51b76444db0b..5654c0c15426 100644
--- a/drivers/iio/imu/adis16475.c
+++ b/drivers/iio/imu/adis16475.c
@@ -645,7 +645,8 @@ static int adis16475_enable_irq(struct adis *adis, bool enable)
 	.timeouts = (_timeouts),					\
 	.burst_reg_cmd = ADIS16475_REG_GLOB_CMD,			\
 	.burst_len = ADIS16475_BURST_MAX_DATA,				\
-	.burst_max_len = ADIS16475_BURST32_MAX_DATA			\
+	.burst_max_len = ADIS16475_BURST32_MAX_DATA,			\
+	.burst_max_speed_hz = ADIS16475_BURST_MAX_SPEED			\
 }
 
 static const struct adis16475_sync adis16475_sync_mode[] = {
@@ -1062,15 +1063,11 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
 	bool valid;
 	/* offset until the first element after gyro and accel */
 	const u8 offset = st->burst32 ? 13 : 7;
-	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
-
-	adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
 
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret)
 		goto check_burst32;
 
-	adis->spi->max_speed_hz = cached_spi_speed_hz;
 	buffer = adis->buffer;
 
 	crc = be16_to_cpu(buffer[offset + 2]);
-- 
2.31.1

[PATCH v2 9/9] iio: adis16400: do not directly change spi 'max_speed_hz'

[Nuno Sa]

With commit 65f2f661e71d ("iio: adis: add burst_max_speed_hz variable"), we
just need to define 'burst_max_speed_hz' and the adis core will take
care of setting up the spi transfers for burst mode. Hence, we fix
a potential race with the spi core where we could be left witn an
invalid 'max_speed_hz'.

Fixes: 5eda3550a3cc1 ("staging:iio:adis16400: Preallocate transfer message")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index b2f92b55b910..cb8d3ffab6fc 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -641,25 +641,13 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct adis16400_state *st = iio_priv(indio_dev);
 	struct adis *adis = &st->adis;
-	u32 old_speed_hz = st->adis.spi->max_speed_hz;
 	void *buffer;
 	int ret;
 
-	if (!(st->variant->flags & ADIS16400_NO_BURST) &&
-		st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
-		st->adis.spi->max_speed_hz = ADIS16400_SPI_BURST;
-		spi_setup(st->adis.spi);
-	}
-
 	ret = spi_sync(adis->spi, &adis->msg);
 	if (ret)
 		dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
 
-	if (!(st->variant->flags & ADIS16400_NO_BURST)) {
-		st->adis.spi->max_speed_hz = old_speed_hz;
-		spi_setup(st->adis.spi);
-	}
-
 	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
 		buffer = adis->buffer + sizeof(u16);
 	else
@@ -965,7 +953,8 @@ static const char * const adis16400_status_error_msgs[] = {
 		BIT(ADIS16400_DIAG_STAT_POWER_LOW),			\
 	.timeouts = (_timeouts),					\
 	.burst_reg_cmd = ADIS16400_GLOB_CMD,				\
-	.burst_len = (_burst_len)					\
+	.burst_len = (_burst_len),					\
+	.burst_max_speed_hz = ADIS16400_SPI_BURST			\
 }
 
 static const struct adis_timeout adis16300_timeouts = {
-- 
2.31.1

Re: [PATCH v2 3/9] iio: adis16475: do not return ints in irq handlers

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> On an IRQ handler we should return normal error codes as 'irqreturn_t'
> is expected.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis16475.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 1de62fc79e0f..51b76444db0b 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -1068,7 +1068,7 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
>
>         ret = spi_sync(adis->spi, &adis->msg);
>         if (ret)
> -               return ret;
> +               goto check_burst32;
>
>         adis->spi->max_speed_hz = cached_spi_speed_hz;
>         buffer = adis->buffer;
> --
> 2.31.1
>

Re: [PATCH v2 4/9] iio: adis_buffer: check return value on page change

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> On the trigger handler, we might need to change the device page. Hence,
> we should check the return value from 'spi_write()' and act accordingly.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 175af154e443..0ae551a748eb 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -134,7 +134,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                 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_write(adis->spi, adis->tx, 2);
> +                       if (ret) {
> +                               dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
> +                               mutex_unlock(&adis->state_lock);
> +                               goto irq_done;
> +                       }
>                 }
>         }
>
> @@ -151,6 +156,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>         iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
>                 pf->timestamp);
>
> +irq_done:
>         iio_trigger_notify_done(indio_dev->trig);
>
>         return IRQ_HANDLED;
> --
> 2.31.1
>

Re: [PATCH v2 5/9] iio: adis_buffer: don't push data to buffers on failure

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> There's no point in pushing data to IIO buffers in case 'spi_sync()'
> fails.
>

Overall, this feels like it's adding some duplication.
However, short-term I'm not seeing a considerably better way to do this.
Maybe, this would require some refactoring of the
adis_trigger_handler() to handle the paging logic a bit more
elegantly.
But that's a broader change.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 0ae551a748eb..a29d22f657ce 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -144,9 +144,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>         }
>
>         ret = spi_sync(adis->spi, &adis->msg);
> -       if (ret)
> +       if (ret) {
>                 dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> -
> +               if (adis->data->has_paging)
> +                       mutex_unlock(&adis->state_lock);
> +               goto irq_done;
> +       }
>
>         if (adis->data->has_paging) {
>                 adis->current_page = 0;
> --
> 2.31.1
>

Re: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to buffers on
> failure"), we return if 'spi_sync()' fails which would leave
> 'adis->current_page' in an incoherent state. Hence, set this variable
> right after we change the device page.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index a29d22f657ce..dda367071980 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                                 mutex_unlock(&adis->state_lock);
>                                 goto irq_done;
>                         }
> +
> +                       adis->current_page = 0;
>                 }
>         }
>
> @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>                 goto irq_done;
>         }
>
> -       if (adis->data->has_paging) {
> -               adis->current_page = 0;
> +       if (adis->data->has_paging)
>                 mutex_unlock(&adis->state_lock);
> -       }

So, continuing from my comment here [1]:
   https://patchwork.kernel.org/project/linux-iio/patch/20210422101911.135630-6-nuno.sa@analog.com/

This can become more elegant, because this block:
       if (adis->data->has_paging)
                mutex_unlock(&adis->state_lock);

can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"

And then the duplication added in patch [1] can be cleaned up.
So maybe a re-ordering of patches could simplify/remove the added duplication.

>
>         iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
>                 pf->timestamp);
> --
> 2.31.1
>

Re: [PATCH v2 7/9] iio: adis: add burst_max_speed_hz variable

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> Typically, in burst mode, the device cannot operate at it's full spi
> speed. Hence, the spi transfers for burst mode have to take this into
> account. With this change we avoid a potential race with the spi core as
> drivers were 'hacking' the device 'max_speed_hz' directly in the
> trigger handler.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis_buffer.c | 4 ++++
>  include/linux/iio/imu/adis.h  | 2 ++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index dda367071980..82239da2f441 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -51,9 +51,13 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
>         adis->xfer[0].tx_buf = tx;
>         adis->xfer[0].bits_per_word = 8;
>         adis->xfer[0].len = 2;
> +       if (adis->data->burst_max_speed_hz)
> +               adis->xfer[0].speed_hz = adis->data->burst_max_speed_hz;
>         adis->xfer[1].rx_buf = adis->buffer;
>         adis->xfer[1].bits_per_word = 8;
>         adis->xfer[1].len = burst_length;
> +       if (adis->data->burst_max_speed_hz)
> +               adis->xfer[1].speed_hz = adis->data->burst_max_speed_hz;
>
>         spi_message_init(&adis->msg);
>         spi_message_add_tail(&adis->xfer[0], &adis->msg);
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index f9b728d490b1..cf49997d5903 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -55,6 +55,7 @@ struct adis_timeout {
>   *                     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
> + * @burst_max_speed_hz:        Maximum spi speed that can be used in burst mode
>   */
>  struct adis_data {
>         unsigned int read_delay;
> @@ -83,6 +84,7 @@ struct adis_data {
>         unsigned int burst_reg_cmd;
>         unsigned int burst_len;
>         unsigned int burst_max_len;
> +       unsigned int burst_max_speed_hz;
>  };
>
>  /**
> --
> 2.31.1
>

Re: [PATCH v2 8/9] iio: adis16475: do not directly change spi 'max_speed_hz'

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> With commit 65f2f661e71d ("iio: adis: add burst_max_speed_hz variable"), we
> just need to define 'burst_max_speed_hz' and the adis core will take
> care of setting up the spi transfers for burst mode. Hence, we fix
> a potential race with the spi core where we could be left with an
> invalid 'max_speed_hz'.
>

This looks more elegant.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


> Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis16475.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 51b76444db0b..5654c0c15426 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -645,7 +645,8 @@ static int adis16475_enable_irq(struct adis *adis, bool enable)
>         .timeouts = (_timeouts),                                        \
>         .burst_reg_cmd = ADIS16475_REG_GLOB_CMD,                        \
>         .burst_len = ADIS16475_BURST_MAX_DATA,                          \
> -       .burst_max_len = ADIS16475_BURST32_MAX_DATA                     \
> +       .burst_max_len = ADIS16475_BURST32_MAX_DATA,                    \
> +       .burst_max_speed_hz = ADIS16475_BURST_MAX_SPEED                 \
>  }
>
>  static const struct adis16475_sync adis16475_sync_mode[] = {
> @@ -1062,15 +1063,11 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
>         bool valid;
>         /* offset until the first element after gyro and accel */
>         const u8 offset = st->burst32 ? 13 : 7;
> -       const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
> -
> -       adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
>
>         ret = spi_sync(adis->spi, &adis->msg);
>         if (ret)
>                 goto check_burst32;
>
> -       adis->spi->max_speed_hz = cached_spi_speed_hz;
>         buffer = adis->buffer;
>
>         crc = be16_to_cpu(buffer[offset + 2]);
> --
> 2.31.1
>

Re: [PATCH v2 9/9] iio: adis16400: do not directly change spi 'max_speed_hz'

[Alexandru Ardelean]

On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
>
> With commit 65f2f661e71d ("iio: adis: add burst_max_speed_hz variable"), we
> just need to define 'burst_max_speed_hz' and the adis core will take
> care of setting up the spi transfers for burst mode. Hence, we fix
> a potential race with the spi core where we could be left witn an
> invalid 'max_speed_hz'.
>

This looks even more elegant than the previous.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


> Fixes: 5eda3550a3cc1 ("staging:iio:adis16400: Preallocate transfer message")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis16400.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index b2f92b55b910..cb8d3ffab6fc 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -641,25 +641,13 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
>         struct iio_dev *indio_dev = pf->indio_dev;
>         struct adis16400_state *st = iio_priv(indio_dev);
>         struct adis *adis = &st->adis;
> -       u32 old_speed_hz = st->adis.spi->max_speed_hz;
>         void *buffer;
>         int ret;
>
> -       if (!(st->variant->flags & ADIS16400_NO_BURST) &&
> -               st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
> -               st->adis.spi->max_speed_hz = ADIS16400_SPI_BURST;
> -               spi_setup(st->adis.spi);
> -       }
> -
>         ret = spi_sync(adis->spi, &adis->msg);
>         if (ret)
>                 dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
>
> -       if (!(st->variant->flags & ADIS16400_NO_BURST)) {
> -               st->adis.spi->max_speed_hz = old_speed_hz;
> -               spi_setup(st->adis.spi);
> -       }
> -
>         if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
>                 buffer = adis->buffer + sizeof(u16);
>         else
> @@ -965,7 +953,8 @@ static const char * const adis16400_status_error_msgs[] = {
>                 BIT(ADIS16400_DIAG_STAT_POWER_LOW),                     \
>         .timeouts = (_timeouts),                                        \
>         .burst_reg_cmd = ADIS16400_GLOB_CMD,                            \
> -       .burst_len = (_burst_len)                                       \
> +       .burst_len = (_burst_len),                                      \
> +       .burst_max_speed_hz = ADIS16400_SPI_BURST                       \
>  }
>
>  static const struct adis_timeout adis16300_timeouts = {
> --
> 2.31.1
>

RE: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Sa, Nuno]

> From: Alexandru Ardelean <ardeleanalex@gmail.com>
> Sent: Friday, April 23, 2021 9:33 AM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> changing it
> 
> [External]
> 
> On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> >
> > With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to
> buffers on
> > failure"), we return if 'spi_sync()' fails which would leave
> > 'adis->current_page' in an incoherent state. Hence, set this variable
> > right after we change the device page.
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis_buffer.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis_buffer.c
> b/drivers/iio/imu/adis_buffer.c
> > index a29d22f657ce..dda367071980 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq,
> void *p)
> >                                 mutex_unlock(&adis->state_lock);
> >                                 goto irq_done;
> >                         }
> > +
> > +                       adis->current_page = 0;
> >                 }
> >         }
> >
> > @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int
> irq, void *p)
> >                 goto irq_done;
> >         }
> >
> > -       if (adis->data->has_paging) {
> > -               adis->current_page = 0;
> > +       if (adis->data->has_paging)
> >                 mutex_unlock(&adis->state_lock);
> > -       }
> 
> So, continuing from my comment here [1]:
> 
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> nux-iio/patch/20210422101911.135630-6-
> nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> 
> This can become more elegant, because this block:
>        if (adis->data->has_paging)
>                 mutex_unlock(&adis->state_lock);
> 
> can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"
> 
> And then the duplication added in patch [1] can be cleaned up.
> So maybe a re-ordering of patches could simplify/remove the added
> duplication.
> 

Hmmm I'm not following you :). What's your idea? You mean the block
inside the 'if (ret)' in case spi_sync fails? If so, we can move it but then
we cannot do the goto jump... you mean something like?

ret = spi_sync();
if (adis->data->has_paging)
	mutex_unlock(&adis->state_lock);
if (ret) {
	dev_err();
	goto irq_done;
}

I don't particularly like the paging stuff after the spi_sync but this avoids
some duplication for sure... and reduces some lines of code :)

- Nuno Sá 

Re: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Alexandru Ardelean]

On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
> > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > Sent: Friday, April 23, 2021 9:33 AM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> > changing it
> >
> > [External]
> >
> > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> > wrote:
> > >
> > > With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to
> > buffers on
> > > failure"), we return if 'spi_sync()' fails which would leave
> > > 'adis->current_page' in an incoherent state. Hence, set this variable
> > > right after we change the device page.
> > >
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/imu/adis_buffer.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/adis_buffer.c
> > b/drivers/iio/imu/adis_buffer.c
> > > index a29d22f657ce..dda367071980 100644
> > > --- a/drivers/iio/imu/adis_buffer.c
> > > +++ b/drivers/iio/imu/adis_buffer.c
> > > @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq,
> > void *p)
> > >                                 mutex_unlock(&adis->state_lock);
> > >                                 goto irq_done;
> > >                         }
> > > +
> > > +                       adis->current_page = 0;
> > >                 }
> > >         }
> > >
> > > @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int
> > irq, void *p)
> > >                 goto irq_done;
> > >         }
> > >
> > > -       if (adis->data->has_paging) {
> > > -               adis->current_page = 0;
> > > +       if (adis->data->has_paging)
> > >                 mutex_unlock(&adis->state_lock);
> > > -       }
> >
> > So, continuing from my comment here [1]:
> >
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> > nux-iio/patch/20210422101911.135630-6-
> > nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> > hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> >
> > This can become more elegant, because this block:
> >        if (adis->data->has_paging)
> >                 mutex_unlock(&adis->state_lock);
> >
> > can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"
> >
> > And then the duplication added in patch [1] can be cleaned up.
> > So maybe a re-ordering of patches could simplify/remove the added
> > duplication.
> >
>
> Hmmm I'm not following you :). What's your idea? You mean the block
> inside the 'if (ret)' in case spi_sync fails? If so, we can move it but then
> we cannot do the goto jump... you mean something like?
>
> ret = spi_sync();
> if (adis->data->has_paging)
>         mutex_unlock(&adis->state_lock);
> if (ret) {
>         dev_err();
>         goto irq_done;
> }
>
> I don't particularly like the paging stuff after the spi_sync but this avoids
> some duplication for sure... and reduces some lines of code :)

Yeah, this was the suggestion.
No strong opinion about it.

>
> - Nuno Sá

Re: [PATCH v2 1/9] iio: adis_buffer: do not return ints in irq handlers

[Jonathan Cameron]

On Thu, 22 Apr 2021 12:19:03 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> On an IRQ handler we should not return normal error codes as 'irqreturn_t'
> is expected.
> 
> Fixes: ccd2b52f4ac69 ("staging:iio: Add common ADIS library")
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

This is an interesting one in the sense it's fixing a bug that can't actually
happen (hence why you can just drop the check ;)  As such I've applied it to
the togreg branch of iio.git and added a note that it doesn't need to
go into stable.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis_buffer.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index ac354321f63a..175af154e443 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -129,9 +129,6 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>  	struct adis *adis = iio_device_get_drvdata(indio_dev);
>  	int ret;
>  
> -	if (!adis->buffer)
> -		return -ENOMEM;
> -
>  	if (adis->data->has_paging) {
>  		mutex_lock(&adis->state_lock);
>  		if (adis->current_page != 0) {

Re: [PATCH v2 2/9] iio: adis16400: do not return ints in irq handlers

[Jonathan Cameron]

On Thu, 22 Apr 2021 12:19:04 +0200
Nuno Sa <nuno.sa@analog.com> wrote:

> On an IRQ handler we should not return normal error codes as 'irqreturn_t'
> is expected.
> 
> Fixes: 5eda3550a3cc1 ("staging:iio:adis16400: Preallocate transfer message")
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Same approach as for previous patch.

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/adis16400.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 768aa493a1a6..b2f92b55b910 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -645,9 +645,6 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
>  	void *buffer;
>  	int ret;
>  
> -	if (!adis->buffer)
> -		return -ENOMEM;
> -
>  	if (!(st->variant->flags & ADIS16400_NO_BURST) &&
>  		st->adis.spi->max_speed_hz > ADIS16400_SPI_BURST) {
>  		st->adis.spi->max_speed_hz = ADIS16400_SPI_BURST;

Re: [PATCH v2 3/9] iio: adis16475: do not return ints in irq handlers

[Jonathan Cameron]

On Fri, 23 Apr 2021 09:41:26 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
> >
> > On an IRQ handler we should return normal error codes as 'irqreturn_t'
> > is expected.
> >  
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> 
> > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>

Hi Nuno,

This needs a more detailed commit message as it is simply changing
the return code.  That goto does other stuff.
Please add some more info and send a v3 with this + other patches
I that build on it and hence I won't be able to apply.

Whilst this one is a real bug, I'm not that fussed about backporting
it quickly so will probably be fine to take this via togreg once the
commit message gives enough detail.

Thanks,

Jonathan


> > ---
> >  drivers/iio/imu/adis16475.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> > index 1de62fc79e0f..51b76444db0b 100644
> > --- a/drivers/iio/imu/adis16475.c
> > +++ b/drivers/iio/imu/adis16475.c
> > @@ -1068,7 +1068,7 @@ static irqreturn_t adis16475_trigger_handler(int irq, void *p)
> >
> >         ret = spi_sync(adis->spi, &adis->msg);
> >         if (ret)
> > -               return ret;
> > +               goto check_burst32;
> >
> >         adis->spi->max_speed_hz = cached_spi_speed_hz;
> >         buffer = adis->buffer;
> > --
> > 2.31.1
> >  

Re: [PATCH v2 4/9] iio: adis_buffer: check return value on page change

[Jonathan Cameron]

On Fri, 23 Apr 2021 10:14:48 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
> >
> > On the trigger handler, we might need to change the device page. Hence,
> > we should check the return value from 'spi_write()' and act accordingly.
> >  
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

Applied to the togreg branch of iio.git.

Thanks,

Jonathan

> 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis_buffer.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> > index 175af154e443..0ae551a748eb 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -134,7 +134,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
> >                 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_write(adis->spi, adis->tx, 2);
> > +                       if (ret) {
> > +                               dev_err(&adis->spi->dev, "Failed to change device page: %d\n", ret);
> > +                               mutex_unlock(&adis->state_lock);
> > +                               goto irq_done;
> > +                       }
> >                 }
> >         }
> >
> > @@ -151,6 +156,7 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
> >         iio_push_to_buffers_with_timestamp(indio_dev, adis->buffer,
> >                 pf->timestamp);
> >
> > +irq_done:
> >         iio_trigger_notify_done(indio_dev->trig);
> >
> >         return IRQ_HANDLED;
> > --
> > 2.31.1
> >  

Re: [PATCH v2 5/9] iio: adis_buffer: don't push data to buffers on failure

[Jonathan Cameron]

On Fri, 23 Apr 2021 10:28:08 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
> >
> > There's no point in pushing data to IIO buffers in case 'spi_sync()'
> > fails.
> >  
> 
> Overall, this feels like it's adding some duplication.
> However, short-term I'm not seeing a considerably better way to do this.
> Maybe, this would require some refactoring of the
> adis_trigger_handler() to handle the paging logic a bit more
> elegantly.
> But that's a broader change.
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

In here the read failed, but I think the switch to current_page = 0
succeeded (as was before this spi_sync).  So should we not
be setting current_page = 0 even int his error path?

With that in mind can we just move the if (ret) check past
the existing unlock? + does it make sense to just move the
setting of current_page = 0 up to where it's actually set?


> 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis_buffer.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> > index 0ae551a748eb..a29d22f657ce 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -144,9 +144,12 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
> >         }
> >
> >         ret = spi_sync(adis->spi, &adis->msg);
> > -       if (ret)
> > +       if (ret) {
> >                 dev_err(&adis->spi->dev, "Failed to read data: %d", ret);
> > -
> > +               if (adis->data->has_paging)
> > +                       mutex_unlock(&adis->state_lock);
> > +               goto irq_done;
> > +       }
> >
> >         if (adis->data->has_paging) {
> >                 adis->current_page = 0;
> > --
> > 2.31.1
> >  

Re: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Jonathan Cameron]

On Fri, 23 Apr 2021 16:56:26 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> >  
> > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > Sent: Friday, April 23, 2021 9:33 AM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > <jic23@kernel.org>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > <lars@metafoo.de>
> > > Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> > > changing it
> > >
> > > [External]
> > >
> > > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> > > wrote:  
> > > >
> > > > With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to  
> > > buffers on  
> > > > failure"), we return if 'spi_sync()' fails which would leave
> > > > 'adis->current_page' in an incoherent state. Hence, set this variable
> > > > right after we change the device page.
> > > >
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  drivers/iio/imu/adis_buffer.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/imu/adis_buffer.c  
> > > b/drivers/iio/imu/adis_buffer.c  
> > > > index a29d22f657ce..dda367071980 100644
> > > > --- a/drivers/iio/imu/adis_buffer.c
> > > > +++ b/drivers/iio/imu/adis_buffer.c
> > > > @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq,  
> > > void *p)  
> > > >                                 mutex_unlock(&adis->state_lock);
> > > >                                 goto irq_done;
> > > >                         }
> > > > +
> > > > +                       adis->current_page = 0;
> > > >                 }
> > > >         }
> > > >
> > > > @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int  
> > > irq, void *p)  
> > > >                 goto irq_done;
> > > >         }
> > > >
> > > > -       if (adis->data->has_paging) {
> > > > -               adis->current_page = 0;
> > > > +       if (adis->data->has_paging)
> > > >                 mutex_unlock(&adis->state_lock);
> > > > -       }  
> > >
> > > So, continuing from my comment here [1]:
> > >
> > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> > > nux-iio/patch/20210422101911.135630-6-
> > > nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> > > hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> > >
> > > This can become more elegant, because this block:
> > >        if (adis->data->has_paging)
> > >                 mutex_unlock(&adis->state_lock);
> > >
> > > can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"
> > >
> > > And then the duplication added in patch [1] can be cleaned up.
> > > So maybe a re-ordering of patches could simplify/remove the added
> > > duplication.
> > >  
> >
> > Hmmm I'm not following you :). What's your idea? You mean the block
> > inside the 'if (ret)' in case spi_sync fails? If so, we can move it but then
> > we cannot do the goto jump... you mean something like?
> >
> > ret = spi_sync();
> > if (adis->data->has_paging)
> >         mutex_unlock(&adis->state_lock);
> > if (ret) {
> >         dev_err();
> >         goto irq_done;
> > }
> >
> > I don't particularly like the paging stuff after the spi_sync but this avoids
> > some duplication for sure... and reduces some lines of code :)  
> 
> Yeah, this was the suggestion.
> No strong opinion about it.
Ah. I should probably read ahead a bit before commenting on earlier patches ;)

I was thinking this was the way to go.  It would be an entirely standard pattern 
if not for the if (adis->data->has_paging) being there.

Hmm maybe some helpers would make this more readable?  

adis_mutex_lock_if_paging(adis);
adis_mutex_unlock_if_paging(adis);

Probably not worth it given single usecase.  I'd go with what you have above.
I'd also reorder this before the previous patch as that will make that one simpler.

Jonathan

> 
> >
> > - Nuno Sá  

Re: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Jonathan Cameron]

On Sat, 24 Apr 2021 12:18:06 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 23 Apr 2021 16:56:26 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:  
> > >    
> > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > Sent: Friday, April 23, 2021 9:33 AM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > <jic23@kernel.org>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > <lars@metafoo.de>
> > > > Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> > > > changing it
> > > >
> > > > [External]
> > > >
> > > > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> > > > wrote:    
> > > > >
> > > > > With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to    
> > > > buffers on    
> > > > > failure"), we return if 'spi_sync()' fails which would leave
> > > > > 'adis->current_page' in an incoherent state. Hence, set this variable
> > > > > right after we change the device page.
> > > > >
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > ---
> > > > >  drivers/iio/imu/adis_buffer.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/imu/adis_buffer.c    
> > > > b/drivers/iio/imu/adis_buffer.c    
> > > > > index a29d22f657ce..dda367071980 100644
> > > > > --- a/drivers/iio/imu/adis_buffer.c
> > > > > +++ b/drivers/iio/imu/adis_buffer.c
> > > > > @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq,    
> > > > void *p)    
> > > > >                                 mutex_unlock(&adis->state_lock);
> > > > >                                 goto irq_done;
> > > > >                         }
> > > > > +
> > > > > +                       adis->current_page = 0;
> > > > >                 }
> > > > >         }
> > > > >
> > > > > @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int    
> > > > irq, void *p)    
> > > > >                 goto irq_done;
> > > > >         }
> > > > >
> > > > > -       if (adis->data->has_paging) {
> > > > > -               adis->current_page = 0;
> > > > > +       if (adis->data->has_paging)
> > > > >                 mutex_unlock(&adis->state_lock);
> > > > > -       }    
> > > >
> > > > So, continuing from my comment here [1]:
> > > >
> > > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> > > > nux-iio/patch/20210422101911.135630-6-
> > > > nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> > > > hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> > > >
> > > > This can become more elegant, because this block:
> > > >        if (adis->data->has_paging)
> > > >                 mutex_unlock(&adis->state_lock);
> > > >
> > > > can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"
> > > >
> > > > And then the duplication added in patch [1] can be cleaned up.
> > > > So maybe a re-ordering of patches could simplify/remove the added
> > > > duplication.
> > > >    
> > >
> > > Hmmm I'm not following you :). What's your idea? You mean the block
> > > inside the 'if (ret)' in case spi_sync fails? If so, we can move it but then
> > > we cannot do the goto jump... you mean something like?
> > >
> > > ret = spi_sync();
> > > if (adis->data->has_paging)
> > >         mutex_unlock(&adis->state_lock);
> > > if (ret) {
> > >         dev_err();
> > >         goto irq_done;
> > > }
> > >
> > > I don't particularly like the paging stuff after the spi_sync but this avoids
> > > some duplication for sure... and reduces some lines of code :)    
> > 
> > Yeah, this was the suggestion.
> > No strong opinion about it.  
> Ah. I should probably read ahead a bit before commenting on earlier patches ;)
> 
> I was thinking this was the way to go.  It would be an entirely standard pattern 
> if not for the if (adis->data->has_paging) being there.
> 
> Hmm maybe some helpers would make this more readable?  
> 
> adis_mutex_lock_if_paging(adis);
> adis_mutex_unlock_if_paging(adis);
> 
> Probably not worth it given single usecase.  I'd go with what you have above.
Ah realised this was unclear just after hitting send.   I mean the 

> > > ret = spi_sync();
> > > if (adis->data->has_paging)
> > >         mutex_unlock(&adis->state_lock);
> > > if (ret) {
> > >         dev_err();
> > >         goto irq_done;
> > > }

approach.

> I'd also reorder this before the previous patch as that will make that one simpler.
> 
> Jonathan
> 
> >   
> > >
> > > - Nuno Sá    
> 

Re: [PATCH v2 7/9] iio: adis: add burst_max_speed_hz variable

[Jonathan Cameron]

On Fri, 23 Apr 2021 10:34:24 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com> wrote:
> >
> > Typically, in burst mode, the device cannot operate at it's full spi
> > speed. Hence, the spi transfers for burst mode have to take this into
> > account. With this change we avoid a potential race with the spi core as
> > drivers were 'hacking' the device 'max_speed_hz' directly in the
> > trigger handler.
> >  
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

Rest of series looks good to me.  Nice cleanup / fixes ;)

I'll probably take this the slow way though rather than through fixes.
Race has been there a while after all with no reports so it can wait
a cycle.  I'll think about whether to mark them for stable when applying.
On this particular occasion we may want to do an explicit stable email
as the need to pick up the whole series (minus the first 2 I think)
even though some parts of it are not fixes as such but precursor rework.

Thanks,

Jonathan

> 
> 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/imu/adis_buffer.c | 4 ++++
> >  include/linux/iio/imu/adis.h  | 2 ++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> > index dda367071980..82239da2f441 100644
> > --- a/drivers/iio/imu/adis_buffer.c
> > +++ b/drivers/iio/imu/adis_buffer.c
> > @@ -51,9 +51,13 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
> >         adis->xfer[0].tx_buf = tx;
> >         adis->xfer[0].bits_per_word = 8;
> >         adis->xfer[0].len = 2;
> > +       if (adis->data->burst_max_speed_hz)
> > +               adis->xfer[0].speed_hz = adis->data->burst_max_speed_hz;
> >         adis->xfer[1].rx_buf = adis->buffer;
> >         adis->xfer[1].bits_per_word = 8;
> >         adis->xfer[1].len = burst_length;
> > +       if (adis->data->burst_max_speed_hz)
> > +               adis->xfer[1].speed_hz = adis->data->burst_max_speed_hz;
> >
> >         spi_message_init(&adis->msg);
> >         spi_message_add_tail(&adis->xfer[0], &adis->msg);
> > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > index f9b728d490b1..cf49997d5903 100644
> > --- a/include/linux/iio/imu/adis.h
> > +++ b/include/linux/iio/imu/adis.h
> > @@ -55,6 +55,7 @@ struct adis_timeout {
> >   *                     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
> > + * @burst_max_speed_hz:        Maximum spi speed that can be used in burst mode
> >   */
> >  struct adis_data {
> >         unsigned int read_delay;
> > @@ -83,6 +84,7 @@ struct adis_data {
> >         unsigned int burst_reg_cmd;
> >         unsigned int burst_len;
> >         unsigned int burst_max_len;
> > +       unsigned int burst_max_speed_hz;
> >  };
> >
> >  /**
> > --
> > 2.31.1
> >  

Re: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Jonathan Cameron]

On Sat, 24 Apr 2021 12:19:52 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 24 Apr 2021 12:18:06 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 23 Apr 2021 16:56:26 +0300
> > Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> >   
> > > On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:    
> > > >      
> > > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > > Sent: Friday, April 23, 2021 9:33 AM
> > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > > <jic23@kernel.org>; Hennerich, Michael
> > > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > > <lars@metafoo.de>
> > > > > Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> > > > > changing it
> > > > >
> > > > > [External]
> > > > >
> > > > > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> > > > > wrote:      
> > > > > >
> > > > > > With commit 58ca347b9b24 ("iio: adis_buffer: don't push data to      
> > > > > buffers on      
> > > > > > failure"), we return if 'spi_sync()' fails which would leave
> > > > > > 'adis->current_page' in an incoherent state. Hence, set this variable
> > > > > > right after we change the device page.
> > > > > >
> > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > > ---
> > > > > >  drivers/iio/imu/adis_buffer.c | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/imu/adis_buffer.c      
> > > > > b/drivers/iio/imu/adis_buffer.c      
> > > > > > index a29d22f657ce..dda367071980 100644
> > > > > > --- a/drivers/iio/imu/adis_buffer.c
> > > > > > +++ b/drivers/iio/imu/adis_buffer.c
> > > > > > @@ -140,6 +140,8 @@ static irqreturn_t adis_trigger_handler(int irq,      
> > > > > void *p)      
> > > > > >                                 mutex_unlock(&adis->state_lock);
> > > > > >                                 goto irq_done;
> > > > > >                         }
> > > > > > +
> > > > > > +                       adis->current_page = 0;
> > > > > >                 }
> > > > > >         }
> > > > > >
> > > > > > @@ -151,10 +153,8 @@ static irqreturn_t adis_trigger_handler(int      
> > > > > irq, void *p)      
> > > > > >                 goto irq_done;
> > > > > >         }
> > > > > >
> > > > > > -       if (adis->data->has_paging) {
> > > > > > -               adis->current_page = 0;
> > > > > > +       if (adis->data->has_paging)
> > > > > >                 mutex_unlock(&adis->state_lock);
> > > > > > -       }      
> > > > >
> > > > > So, continuing from my comment here [1]:
> > > > >
> > > > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> > > > > nux-iio/patch/20210422101911.135630-6-
> > > > > nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> > > > > hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> > > > >
> > > > > This can become more elegant, because this block:
> > > > >        if (adis->data->has_paging)
> > > > >                 mutex_unlock(&adis->state_lock);
> > > > >
> > > > > can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"
> > > > >
> > > > > And then the duplication added in patch [1] can be cleaned up.
> > > > > So maybe a re-ordering of patches could simplify/remove the added
> > > > > duplication.
> > > > >      
> > > >
> > > > Hmmm I'm not following you :). What's your idea? You mean the block
> > > > inside the 'if (ret)' in case spi_sync fails? If so, we can move it but then
> > > > we cannot do the goto jump... you mean something like?
> > > >
> > > > ret = spi_sync();
> > > > if (adis->data->has_paging)
> > > >         mutex_unlock(&adis->state_lock);
> > > > if (ret) {
> > > >         dev_err();
> > > >         goto irq_done;
> > > > }
> > > >
> > > > I don't particularly like the paging stuff after the spi_sync but this avoids
> > > > some duplication for sure... and reduces some lines of code :)      
> > > 
> > > Yeah, this was the suggestion.
> > > No strong opinion about it.    
> > Ah. I should probably read ahead a bit before commenting on earlier patches ;)
> > 
> > I was thinking this was the way to go.  It would be an entirely standard pattern 
> > if not for the if (adis->data->has_paging) being there.
> > 
> > Hmm maybe some helpers would make this more readable?  
> > 
> > adis_mutex_lock_if_paging(adis);
> > adis_mutex_unlock_if_paging(adis);
> > 
> > Probably not worth it given single usecase.  I'd go with what you have above.  
> Ah realised this was unclear just after hitting send.   I mean the 
> 
> > > > ret = spi_sync();
> > > > if (adis->data->has_paging)
> > > >         mutex_unlock(&adis->state_lock);
> > > > if (ret) {
> > > >         dev_err();
> > > >         goto irq_done;
> > > > }  
> 
> approach.

As I love replying to myself ;)

From the other adis patch, I noticed that we have adis_dev_lock()

Probably good to have a follow up patch that tidies the drivers up to use
that where appropriate.  Good not to do it in these patches though as that
would just make them locally harder to read.

Thanks,

Jonathan

> 
> > I'd also reorder this before the previous patch as that will make that one simpler.
> > 
> > Jonathan
> >   
> > >     
> > > >
> > > > - Nuno Sá      
> >   
> 

RE: [PATCH v2 3/9] iio: adis16475: do not return ints in irq handlers

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, April 24, 2021 1:07 PM
> To: Alexandru Ardelean <ardeleanalex@gmail.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio <linux-
> iio@vger.kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH v2 3/9] iio: adis16475: do not return ints in irq
> handlers
> 
> [External]
> 
> On Fri, 23 Apr 2021 09:41:26 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> wrote:
> > >
> > > On an IRQ handler we should return normal error codes as
> 'irqreturn_t'
> > > is expected.
> > >
> >
> > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> >
> > > Fixes: fff7352bf7a3c ("iio: imu: Add support for adis16475")
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> Hi Nuno,
> 
> This needs a more detailed commit message as it is simply changing
> the return code.  That goto does other stuff.
> Please add some more info and send a v3 with this + other patches
> I that build on it and hence I won't be able to apply.
> 
> Whilst this one is a real bug, I'm not that fussed about backporting
> it quickly so will probably be fine to take this via togreg once the
> commit message gives enough detail.

Agreed...

- Nuno Sá

RE: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Sa, Nuno]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, April 24, 2021 1:18 PM
> To: Alexandru Ardelean <ardeleanalex@gmail.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio <linux-
> iio@vger.kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> changing it
> 
> [External]
> 
> On Fri, 23 Apr 2021 16:56:26 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> > >
> > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > Sent: Friday, April 23, 2021 9:33 AM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > <jic23@kernel.org>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > <lars@metafoo.de>
> > > > Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page
> after
> > > > changing it
> > > >
> > > > [External]
> > > >
> > > > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa <nuno.sa@analog.com>
> > > > wrote:
> > > > >
> > > > > With commit 58ca347b9b24 ("iio: adis_buffer: don't push data
> to
> > > > buffers on
> > > > > failure"), we return if 'spi_sync()' fails which would leave
> > > > > 'adis->current_page' in an incoherent state. Hence, set this
> variable
> > > > > right after we change the device page.
> > > > >
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > ---
> > > > >  drivers/iio/imu/adis_buffer.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/imu/adis_buffer.c
> > > > b/drivers/iio/imu/adis_buffer.c
> > > > > index a29d22f657ce..dda367071980 100644
> > > > > --- a/drivers/iio/imu/adis_buffer.c
> > > > > +++ b/drivers/iio/imu/adis_buffer.c
> > > > > @@ -140,6 +140,8 @@ static irqreturn_t
> adis_trigger_handler(int irq,
> > > > void *p)
> > > > >                                 mutex_unlock(&adis->state_lock);
> > > > >                                 goto irq_done;
> > > > >                         }
> > > > > +
> > > > > +                       adis->current_page = 0;
> > > > >                 }
> > > > >         }
> > > > >
> > > > > @@ -151,10 +153,8 @@ static irqreturn_t
> adis_trigger_handler(int
> > > > irq, void *p)
> > > > >                 goto irq_done;
> > > > >         }
> > > > >
> > > > > -       if (adis->data->has_paging) {
> > > > > -               adis->current_page = 0;
> > > > > +       if (adis->data->has_paging)
> > > > >                 mutex_unlock(&adis->state_lock);
> > > > > -       }
> > > >
> > > > So, continuing from my comment here [1]:
> > > >
> > > >
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> > > > nux-iio/patch/20210422101911.135630-6-
> > > >
> nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> > > > hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> > > >
> > > > This can become more elegant, because this block:
> > > >        if (adis->data->has_paging)
> > > >                 mutex_unlock(&adis->state_lock);
> > > >
> > > > can be moved right after "ret = spi_sync(adis->spi, &adis->msg);"
> > > >
> > > > And then the duplication added in patch [1] can be cleaned up.
> > > > So maybe a re-ordering of patches could simplify/remove the
> added
> > > > duplication.
> > > >
> > >
> > > Hmmm I'm not following you :). What's your idea? You mean the
> block
> > > inside the 'if (ret)' in case spi_sync fails? If so, we can move it but
> then
> > > we cannot do the goto jump... you mean something like?
> > >
> > > ret = spi_sync();
> > > if (adis->data->has_paging)
> > >         mutex_unlock(&adis->state_lock);
> > > if (ret) {
> > >         dev_err();
> > >         goto irq_done;
> > > }
> > >
> > > I don't particularly like the paging stuff after the spi_sync but this
> avoids
> > > some duplication for sure... and reduces some lines of code :)
> >
> > Yeah, this was the suggestion.
> > No strong opinion about it.
> Ah. I should probably read ahead a bit before commenting on earlier
> patches ;)
> 
> I was thinking this was the way to go.  It would be an entirely standard
> pattern
> if not for the if (adis->data->has_paging) being there.
> 
> Hmm maybe some helpers would make this more readable?
> 
> adis_mutex_lock_if_paging(adis);
> adis_mutex_unlock_if_paging(adis);
> 
> Probably not worth it given single usecase.  I'd go with what you have
> above.
> I'd also reorder this before the previous patch as that will make that
> one simpler.
> 

Agreed...

- Nuno Sá

RE: [PATCH v2 6/9] iio: adis_buffer: update device page after changing it

[Sa, Nuno]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, April 24, 2021 1:31 PM
> To: Alexandru Ardelean <ardeleanalex@gmail.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio <linux-
> iio@vger.kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device page after
> changing it
> 
> [External]
> 
> On Sat, 24 Apr 2021 12:19:52 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Sat, 24 Apr 2021 12:18:06 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > > On Fri, 23 Apr 2021 16:56:26 +0300
> > > Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> > >
> > > > On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> > > > >
> > > > > > From: Alexandru Ardelean <ardeleanalex@gmail.com>
> > > > > > Sent: Friday, April 23, 2021 9:33 AM
> > > > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > > > <jic23@kernel.org>; Hennerich, Michael
> > > > > > <Michael.Hennerich@analog.com>; Lars-Peter Clausen
> > > > > > <lars@metafoo.de>
> > > > > > Subject: Re: [PATCH v2 6/9] iio: adis_buffer: update device
> page after
> > > > > > changing it
> > > > > >
> > > > > > [External]
> > > > > >
> > > > > > On Thu, Apr 22, 2021 at 1:17 PM Nuno Sa
> <nuno.sa@analog.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > With commit 58ca347b9b24 ("iio: adis_buffer: don't push
> data to
> > > > > > buffers on
> > > > > > > failure"), we return if 'spi_sync()' fails which would leave
> > > > > > > 'adis->current_page' in an incoherent state. Hence, set this
> variable
> > > > > > > right after we change the device page.
> > > > > > >
> > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > > > ---
> > > > > > >  drivers/iio/imu/adis_buffer.c | 6 +++---
> > > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/imu/adis_buffer.c
> > > > > > b/drivers/iio/imu/adis_buffer.c
> > > > > > > index a29d22f657ce..dda367071980 100644
> > > > > > > --- a/drivers/iio/imu/adis_buffer.c
> > > > > > > +++ b/drivers/iio/imu/adis_buffer.c
> > > > > > > @@ -140,6 +140,8 @@ static irqreturn_t
> adis_trigger_handler(int irq,
> > > > > > void *p)
> > > > > > >                                 mutex_unlock(&adis->state_lock);
> > > > > > >                                 goto irq_done;
> > > > > > >                         }
> > > > > > > +
> > > > > > > +                       adis->current_page = 0;
> > > > > > >                 }
> > > > > > >         }
> > > > > > >
> > > > > > > @@ -151,10 +153,8 @@ static irqreturn_t
> adis_trigger_handler(int
> > > > > > irq, void *p)
> > > > > > >                 goto irq_done;
> > > > > > >         }
> > > > > > >
> > > > > > > -       if (adis->data->has_paging) {
> > > > > > > -               adis->current_page = 0;
> > > > > > > +       if (adis->data->has_paging)
> > > > > > >                 mutex_unlock(&adis->state_lock);
> > > > > > > -       }
> > > > > >
> > > > > > So, continuing from my comment here [1]:
> > > > > >
> > > > > >
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
> > > > > > nux-iio/patch/20210422101911.135630-6-
> > > > > >
> nuno.sa@analog.com/__;!!A3Ni8CS0y2Y!u1RyPNeh8e5m7lPfDa5H5ZjT
> > > > > > hA9TdsLGvk2m1kFQBbAKe40PmvQS8O8N-f-GEg$
> > > > > >
> > > > > > This can become more elegant, because this block:
> > > > > >        if (adis->data->has_paging)
> > > > > >                 mutex_unlock(&adis->state_lock);
> > > > > >
> > > > > > can be moved right after "ret = spi_sync(adis->spi, &adis-
> >msg);"
> > > > > >
> > > > > > And then the duplication added in patch [1] can be cleaned
> up.
> > > > > > So maybe a re-ordering of patches could simplify/remove the
> added
> > > > > > duplication.
> > > > > >
> > > > >
> > > > > Hmmm I'm not following you :). What's your idea? You mean
> the block
> > > > > inside the 'if (ret)' in case spi_sync fails? If so, we can move it
> but then
> > > > > we cannot do the goto jump... you mean something like?
> > > > >
> > > > > ret = spi_sync();
> > > > > if (adis->data->has_paging)
> > > > >         mutex_unlock(&adis->state_lock);
> > > > > if (ret) {
> > > > >         dev_err();
> > > > >         goto irq_done;
> > > > > }
> > > > >
> > > > > I don't particularly like the paging stuff after the spi_sync but
> this avoids
> > > > > some duplication for sure... and reduces some lines of code :)
> > > >
> > > > Yeah, this was the suggestion.
> > > > No strong opinion about it.
> > > Ah. I should probably read ahead a bit before commenting on
> earlier patches ;)
> > >
> > > I was thinking this was the way to go.  It would be an entirely
> standard pattern
> > > if not for the if (adis->data->has_paging) being there.
> > >
> > > Hmm maybe some helpers would make this more readable?
> > >
> > > adis_mutex_lock_if_paging(adis);
> > > adis_mutex_unlock_if_paging(adis);
> > >
> > > Probably not worth it given single usecase.  I'd go with what you
> have above.
> > Ah realised this was unclear just after hitting send.   I mean the
> >
> > > > > ret = spi_sync();
> > > > > if (adis->data->has_paging)
> > > > >         mutex_unlock(&adis->state_lock);
> > > > > if (ret) {
> > > > >         dev_err();
> > > > >         goto irq_done;
> > > > > }
> >
> > approach.
> 
> As I love replying to myself ;)
> 
> From the other adis patch, I noticed that we have adis_dev_lock()
> 
> Probably good to have a follow up patch that tidies the drivers up to
> use
> that where appropriate.  Good not to do it in these patches though as
> that
> would just make them locally harder to read.
> 

Hi Jonathan,

I did thought about using the helpers here. But my goal with the helpers was
to use them in drivers and keep the core as is. As we can think of adis_buffer
as part of the lib core implementation, I was fine in leaving the 'raw' mutex
access here.

Anyways, I do not have strong feelings about this so if you prefer to have adis.c
and adis_buffer.c using the mutex helpers, I'm happy in doing so in a follow up
patch (not sure if it would make sense to also apply the helpers in adis.h as we might
need some reordering of the helpers)...

- Nuno Sá