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

[Jonathan Cameron <jic23@kernel.org>]

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á      
> >   
>