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

From: Alexandru Ardelean <ardeleanalex@gmail.com>
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
Date: Fri, 23 Apr 2021 16:56:26 +0300	[thread overview]
Message-ID: <CA+U=Dsp08Hv7KtV_K=mG9RfDWpTezhPa0Xjp_R7jZrAuOqKz_w@mail.gmail.com> (raw)
In-Reply-To: <CY4PR03MB311220A749133B44C778F9F799459@CY4PR03MB3112.namprd03.prod.outlook.com>

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á