On Sat, 2019-10-05 at 15:49 +0100, Jonathan Cameron wrote: > [External] > > On Sat, 5 Oct 2019 15:38:45 +0100 > Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > On Thu, 19 Sep 2019 14:57:16 +0300 > > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote: > > > > > The internal lock that is by the ADIS library should be sufficient to > > > keep > > > state consistent. There is no need for an extra lock. > > > > I'm not sure that's true. In theory we could get two different > > attempts > > to set the sampling frequency running concurrently. That could lead to > > a race between the point where we set the spi frequency for future > > messages and the point where we set the devices sampling frequency. > > > > Bang it stops working. So, whilst it is arguably paranoid I think > > you do need a lock here, but it should be something that is driver > > local rather than mlock. > > Thinking a bit more on this do we have a potential issue where we > race with a read on the spi bus as we are changing this frequency? > They might all be 'safe' but I haven't thought it through properly yet. > To be honest, I don't know. I am seeing what you're saying about the `max_speed_hz` change and that could be an issue. That detail has slipped me. I guess, I could rework this patch to use the ADIS library's lock. thanks Alex > > thanks, > > > > Jonathan > > > > > > > This patch removes it. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > > > --- > > > drivers/iio/gyro/adis16260.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/drivers/iio/gyro/adis16260.c > > > b/drivers/iio/gyro/adis16260.c > > > index 207a0ce13439..0fa93d02062a 100644 > > > --- a/drivers/iio/gyro/adis16260.c > > > +++ b/drivers/iio/gyro/adis16260.c > > > @@ -293,7 +293,6 @@ static int adis16260_write_raw(struct iio_dev > > > *indio_dev, > > > addr = adis16260_addresses[chan->scan_index][1]; > > > return adis_write_reg_16(adis, addr, val); > > > case IIO_CHAN_INFO_SAMP_FREQ: > > > - mutex_lock(&indio_dev->mlock); > > > if (spi_get_device_id(adis->spi)->driver_data) > > > t = 256 / val; > > > else > > > @@ -310,7 +309,6 @@ static int adis16260_write_raw(struct iio_dev > > > *indio_dev, > > > adis->spi->max_speed_hz = ADIS16260_SPI_FAST; > > > ret = adis_write_reg_8(adis, ADIS16260_SMPL_PRD, t); > > > > > > - mutex_unlock(&indio_dev->mlock); > > > return ret; > > > } > > > return -EINVAL;
