Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 01 Dec 2023 09:49:38 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2023-11-30 at 15:50 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > > are done which means we need to properly protect the whole operation so
> > > we are sure we will be in a sane state if two concurrent calls occur.
> > > 
> > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/adc/ad9467.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 04474dbfa631..91821dee03b7 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> > > @@ -4,7 +4,7 @@
> > >   *
> > >   * Copyright 2012-2020 Analog Devices Inc.
> > >   */
> > > -
> > > +#include <linux/cleanup.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>  
> > 
> > Ah, the case of the misplaced header from the previous patch is solved. :-)
> >   
> 
> Yeps, it needs to be in this patch :)
> 
> > >  #include <linux/device.h>
> > > @@ -122,6 +122,8 @@ struct ad9467_state {
> > >         unsigned int                    output_mode;
> > > 
> > >         struct gpio_desc                *pwrdown_gpio;
> > > +       /* protect against concurrent accesses to the device */
> > > +       struct mutex                    lock;
> > >  };
> > > 
> > >  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > > @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > > unsigned int reg,
> > >         int ret;
> > > 
> > >         if (!readval) {
> > > +               guard(mutex)(&st->lock);
> > >                 ret = ad9467_spi_write(spi, reg, writeval);
> > >                 if (ret)
> > >                         return ret;
> > > @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > > int val, int val2)
> > >                 if (scale_val[0] != val || scale_val[1] != val2)
> > >                         continue;
> > > 
> > > +               guard(mutex)(&st->lock);
> > >                 ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> > >                                        info->scale_table[i][1]);
> > >                 if (ret < 0)
> > > 
> > > --
> > > 2.42.1
> > > 
> > >   
> > 
> > Alternately, this could probably be solved with spi_bus_lock/unlock
> > and spi_sync_locked rather than introducing a new mutex.  
> 
> Hmm, typically you just have your own lock. No reason to lock the spi bus. And I also
> have some plans to eventually change this to regmap :)

Bus lock typically implies that we can't let other users grab the bus in between
for reasons like the chip select needing to be held. I'm not keen on it being
used if the locking is just needed for a specific driver to deal with its
associated device and driver state.

Jonathan

> 
> - Nuno Sá
>