On Sat, 24 Apr 2021 12:18:06 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Fri, 23 Apr 2021 16:56:26 +0300
> Alexandru Ardelean <ardeleanalex@xxxxxxxxx> wrote:
>
> > On Fri, Apr 23, 2021 at 3:20 PM Sa, Nuno <Nuno.Sa@xxxxxxxxxx> wrote:
> > >
> > > > 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 :)
> >
> > 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á
>
