| Message ID | 20231205-iio-backend-prep-v1-3-7c9bc18d612b@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: ad9467 and axi-adc cleanups | expand |
On Tue, Dec 5, 2023 at 11:06 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. ad9467_outputmode_set() also has multiple calls to ad9467_spi_write(). Does it need similar protection? > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC") > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/iio/adc/ad9467.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c > index 0f2dce730a0a..badbef2ce9f8 100644 > --- a/drivers/iio/adc/ad9467.c > +++ b/drivers/iio/adc/ad9467.c > @@ -4,8 +4,9 @@ > * > * Copyright 2012-2020 Analog Devices Inc. > */ > - > +#include <linux/cleanup.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/device.h> > #include <linux/kernel.h> > #include <linux/slab.h> > @@ -121,6 +122,8 @@ struct ad9467_state { > unsigned int output_mode; > > struct gpio_desc *pwrdown_gpio; > + /* ensure consistent state obtained on multiple related accesses */ > + struct mutex lock; > }; > > static int ad9467_spi_read(struct spi_device *spi, unsigned int reg) > @@ -161,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg, > int ret; > > if (readval == NULL) { > + guard(mutex)(&st->lock); > ret = ad9467_spi_write(spi, reg, writeval); > if (ret) > return ret; > @@ -310,6 +314,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); Why is the guard inside of the for loop instead of outside? __ad9467_get_scale() called in this loop calls ad9467_spi_read() too, but since this is not a read-modify-write, I guess I can see why that might not be inside of the gurad. > ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF, > info->scale_table[i][1]); > if (ret < 0) > > -- > 2.43.0 > >
On Wed, 2023-12-06 at 16:27 -0600, David Lechner wrote: > On Tue, Dec 5, 2023 at 11:06 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. > > > ad9467_outputmode_set() also has multiple calls to ad9467_spi_write(). > Does it need similar protection? > Just called during probe (of the axi-adc driver) before registering the IIO device. We should not need the lock in there (for now). > > > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC") > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > drivers/iio/adc/ad9467.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c > > index 0f2dce730a0a..badbef2ce9f8 100644 > > --- a/drivers/iio/adc/ad9467.c > > +++ b/drivers/iio/adc/ad9467.c > > @@ -4,8 +4,9 @@ > > * > > * Copyright 2012-2020 Analog Devices Inc. > > */ > > - > > +#include <linux/cleanup.h> > > #include <linux/module.h> > > +#include <linux/mutex.h> > > #include <linux/device.h> > > #include <linux/kernel.h> > > #include <linux/slab.h> > > @@ -121,6 +122,8 @@ struct ad9467_state { > > unsigned int output_mode; > > > > struct gpio_desc *pwrdown_gpio; > > + /* ensure consistent state obtained on multiple related accesses */ > > + struct mutex lock; > > }; > > > > static int ad9467_spi_read(struct spi_device *spi, unsigned int reg) > > @@ -161,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, > > unsigned int reg, > > int ret; > > > > if (readval == NULL) { > > + guard(mutex)(&st->lock); > > ret = ad9467_spi_write(spi, reg, writeval); > > if (ret) > > return ret; > > @@ -310,6 +314,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); > > Why is the guard inside of the for loop instead of outside? > > __ad9467_get_scale() called in this loop calls ad9467_spi_read() too, Hmm, am I missing something? __ad9467_get_scale() is not doing any spi access, is it? I think you made confusion with the version without underscore... - Nuno Sá >
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c index 0f2dce730a0a..badbef2ce9f8 100644 --- a/drivers/iio/adc/ad9467.c +++ b/drivers/iio/adc/ad9467.c @@ -4,8 +4,9 @@ * * Copyright 2012-2020 Analog Devices Inc. */ - +#include <linux/cleanup.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/device.h> #include <linux/kernel.h> #include <linux/slab.h> @@ -121,6 +122,8 @@ struct ad9467_state { unsigned int output_mode; struct gpio_desc *pwrdown_gpio; + /* ensure consistent state obtained on multiple related accesses */ + struct mutex lock; }; static int ad9467_spi_read(struct spi_device *spi, unsigned int reg) @@ -161,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg, int ret; if (readval == NULL) { + guard(mutex)(&st->lock); ret = ad9467_spi_write(spi, reg, writeval); if (ret) return ret; @@ -310,6 +314,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)