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

From: Alexandru Ardelean <ardeleanalex@gmail.com>
To: Nuno Sa <nuno.sa@analog.com>
Cc: linux-iio <linux-iio@vger.kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	Michael Hennerich <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
Date: Fri, 23 Apr 2021 10:32:42 +0300	[thread overview]
Message-ID: <CA+U=Dsphpu97Et6sgjd+9TUEHTw7xq44M5yL=CFtuCEU9QLxQQ@mail.gmail.com> (raw)
In-Reply-To: <20210422101911.135630-7-nuno.sa@analog.com>

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
>