> From: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>
> Sent: Friday, April 23, 2021 9:33 AM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/__;!!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á
