| Message ID | 20190319113755.8982-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion | expand |
On Tue, 19 Mar 2019 13:37:55 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > From: Lars-Peter Clausen <lars@metafoo.de> > > For devices from the SigmaDelta family we need to keep CS low when doing a > conversion, since the device will use the MISO line as a interrupt to > indicate that the conversion is complete. > > This is why the driver locks the SPI bus and when the SPI bus is locked > keeps as long as a conversion is going on. The current implementation gets > one small detail wrong though. CS is only de-asserted after the SPI bus is > unlocked. This means it is possible for a different SPI device on the same > bus to send a message which would be wrongfully be addressed to the > SigmaDelta device as well. Make sure that the last SPI transfer that is > done while holding the SPI bus lock de-asserts the CS signal. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Alexandru Ardelean <Alexandru.Ardelean@analog.com> Hi Alex, So, it's a fix for a long existing problem. Do we have anyone who has experienced it? I'm trying to judge whether this is stable material or not. If it is stable material, do you want to have a go at identifying a patch for the Fixes tag? Patch looks good. Jonathan > --- > > Changelog v1 -> v2: > * added my S-o-B line > > drivers/iio/adc/ad_sigma_delta.c | 16 +++++++++++----- > include/linux/iio/adc/ad_sigma_delta.h | 1 + > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > index ff5f2da2e1b1..af6cbc683214 100644 > --- a/drivers/iio/adc/ad_sigma_delta.c > +++ b/drivers/iio/adc/ad_sigma_delta.c > @@ -62,7 +62,7 @@ int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg, > struct spi_transfer t = { > .tx_buf = data, > .len = size + 1, > - .cs_change = sigma_delta->bus_locked, > + .cs_change = sigma_delta->keep_cs_asserted, > }; > struct spi_message m; > int ret; > @@ -217,6 +217,7 @@ static int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, > > spi_bus_lock(sigma_delta->spi->master); > sigma_delta->bus_locked = true; > + sigma_delta->keep_cs_asserted = true; > reinit_completion(&sigma_delta->completion); > > ret = ad_sigma_delta_set_mode(sigma_delta, mode); > @@ -234,9 +235,10 @@ static int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, > ret = 0; > } > out: > + sigma_delta->keep_cs_asserted = false; > + ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > sigma_delta->bus_locked = false; > spi_bus_unlock(sigma_delta->spi->master); > - ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > return ret; > } > @@ -289,6 +291,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > > spi_bus_lock(sigma_delta->spi->master); > sigma_delta->bus_locked = true; > + sigma_delta->keep_cs_asserted = true; > reinit_completion(&sigma_delta->completion); > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE); > @@ -298,9 +301,6 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > ret = wait_for_completion_interruptible_timeout( > &sigma_delta->completion, HZ); > > - sigma_delta->bus_locked = false; > - spi_bus_unlock(sigma_delta->spi->master); > - > if (ret == 0) > ret = -EIO; > if (ret < 0) > @@ -321,7 +321,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > sigma_delta->irq_dis = true; > } > > + sigma_delta->keep_cs_asserted = false; > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > + sigma_delta->bus_locked = false; > + spi_bus_unlock(sigma_delta->spi->master); > mutex_unlock(&indio_dev->mlock); > > if (ret) > @@ -358,6 +361,8 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev) > > spi_bus_lock(sigma_delta->spi->master); > sigma_delta->bus_locked = true; > + sigma_delta->keep_cs_asserted = true; > + > ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS); > if (ret) > goto err_unlock; > @@ -386,6 +391,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev) > sigma_delta->irq_dis = true; > } > > + sigma_delta->keep_cs_asserted = false; > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > sigma_delta->bus_locked = false; > diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h > index 7e84351fa2c0..6e9fb1932dde 100644 > --- a/include/linux/iio/adc/ad_sigma_delta.h > +++ b/include/linux/iio/adc/ad_sigma_delta.h > @@ -69,6 +69,7 @@ struct ad_sigma_delta { > bool irq_dis; > > bool bus_locked; > + bool keep_cs_asserted; > > uint8_t comm; >
On Sun, 2019-03-24 at 17:47 +0000, Jonathan Cameron wrote: > [External] > > > On Tue, 19 Mar 2019 13:37:55 +0200 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > From: Lars-Peter Clausen <lars@metafoo.de> > > > > For devices from the SigmaDelta family we need to keep CS low when > > doing a > > conversion, since the device will use the MISO line as a interrupt to > > indicate that the conversion is complete. > > > > This is why the driver locks the SPI bus and when the SPI bus is locked > > keeps as long as a conversion is going on. The current implementation > > gets > > one small detail wrong though. CS is only de-asserted after the SPI bus > > is > > unlocked. This means it is possible for a different SPI device on the > > same > > bus to send a message which would be wrongfully be addressed to the > > SigmaDelta device as well. Make sure that the last SPI transfer that is > > done while holding the SPI bus lock de-asserts the CS signal. > > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > Signed-off-by: Alexandru Ardelean <Alexandru.Ardelean@analog.com> > > Hi Alex, > > So, it's a fix for a long existing problem. Do we have anyone who > has experienced it? I'm trying to judge whether this is stable material > or not. If it is stable material, do you want to have a go at > identifying > a patch for the Fixes tag? So I actually forgot to do the preparation before cherry-picking this one [i.e. whether it should have a Fixes tag or not]. Sorry abou that. This change is more in the context of some non-upstream functionality for the SigmaDelta. i.e. it's a fix, but it's more important for a functionality that will be added later. There aren't any people that have experienced issues with the mainline version of the ad_sigma_delta drivers that I know of. Bottom line is, we can skip adding this one to stable. Thanks Alex > Patch looks good. > > Jonathan > > > --- > > > > Changelog v1 -> v2: > > * added my S-o-B line > > > > drivers/iio/adc/ad_sigma_delta.c | 16 +++++++++++----- > > include/linux/iio/adc/ad_sigma_delta.h | 1 + > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c > > b/drivers/iio/adc/ad_sigma_delta.c > > index ff5f2da2e1b1..af6cbc683214 100644 > > --- a/drivers/iio/adc/ad_sigma_delta.c > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > @@ -62,7 +62,7 @@ int ad_sd_write_reg(struct ad_sigma_delta > > *sigma_delta, unsigned int reg, > > struct spi_transfer t = { > > .tx_buf = data, > > .len = size + 1, > > - .cs_change = sigma_delta->bus_locked, > > + .cs_change = sigma_delta->keep_cs_asserted, > > }; > > struct spi_message m; > > int ret; > > @@ -217,6 +217,7 @@ static int ad_sd_calibrate(struct ad_sigma_delta > > *sigma_delta, > > > > spi_bus_lock(sigma_delta->spi->master); > > sigma_delta->bus_locked = true; > > + sigma_delta->keep_cs_asserted = true; > > reinit_completion(&sigma_delta->completion); > > > > ret = ad_sigma_delta_set_mode(sigma_delta, mode); > > @@ -234,9 +235,10 @@ static int ad_sd_calibrate(struct ad_sigma_delta > > *sigma_delta, > > ret = 0; > > } > > out: > > + sigma_delta->keep_cs_asserted = false; > > + ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > sigma_delta->bus_locked = false; > > spi_bus_unlock(sigma_delta->spi->master); > > - ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > > > return ret; > > } > > @@ -289,6 +291,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev > > *indio_dev, > > > > spi_bus_lock(sigma_delta->spi->master); > > sigma_delta->bus_locked = true; > > + sigma_delta->keep_cs_asserted = true; > > reinit_completion(&sigma_delta->completion); > > > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE); > > @@ -298,9 +301,6 @@ int ad_sigma_delta_single_conversion(struct iio_dev > > *indio_dev, > > ret = wait_for_completion_interruptible_timeout( > > &sigma_delta->completion, HZ); > > > > - sigma_delta->bus_locked = false; > > - spi_bus_unlock(sigma_delta->spi->master); > > - > > if (ret == 0) > > ret = -EIO; > > if (ret < 0) > > @@ -321,7 +321,10 @@ int ad_sigma_delta_single_conversion(struct > > iio_dev *indio_dev, > > sigma_delta->irq_dis = true; > > } > > > > + sigma_delta->keep_cs_asserted = false; > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > + sigma_delta->bus_locked = false; > > + spi_bus_unlock(sigma_delta->spi->master); > > mutex_unlock(&indio_dev->mlock); > > > > if (ret) > > @@ -358,6 +361,8 @@ static int ad_sd_buffer_postenable(struct iio_dev > > *indio_dev) > > > > spi_bus_lock(sigma_delta->spi->master); > > sigma_delta->bus_locked = true; > > + sigma_delta->keep_cs_asserted = true; > > + > > ret = ad_sigma_delta_set_mode(sigma_delta, > > AD_SD_MODE_CONTINUOUS); > > if (ret) > > goto err_unlock; > > @@ -386,6 +391,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev > > *indio_dev) > > sigma_delta->irq_dis = true; > > } > > > > + sigma_delta->keep_cs_asserted = false; > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > > > sigma_delta->bus_locked = false; > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h > > b/include/linux/iio/adc/ad_sigma_delta.h > > index 7e84351fa2c0..6e9fb1932dde 100644 > > --- a/include/linux/iio/adc/ad_sigma_delta.h > > +++ b/include/linux/iio/adc/ad_sigma_delta.h > > @@ -69,6 +69,7 @@ struct ad_sigma_delta { > > bool irq_dis; > > > > bool bus_locked; > > + bool keep_cs_asserted; > > > > uint8_t comm; > > > >
On Tue, 26 Mar 2019 12:43:32 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Sun, 2019-03-24 at 17:47 +0000, Jonathan Cameron wrote: > > [External] > > > > > > On Tue, 19 Mar 2019 13:37:55 +0200 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > From: Lars-Peter Clausen <lars@metafoo.de> > > > > > > For devices from the SigmaDelta family we need to keep CS low when > > > doing a > > > conversion, since the device will use the MISO line as a interrupt to > > > indicate that the conversion is complete. > > > > > > This is why the driver locks the SPI bus and when the SPI bus is locked > > > keeps as long as a conversion is going on. The current implementation > > > gets > > > one small detail wrong though. CS is only de-asserted after the SPI bus > > > is > > > unlocked. This means it is possible for a different SPI device on the > > > same > > > bus to send a message which would be wrongfully be addressed to the > > > SigmaDelta device as well. Make sure that the last SPI transfer that is > > > done while holding the SPI bus lock de-asserts the CS signal. > > > > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > > Signed-off-by: Alexandru Ardelean <Alexandru.Ardelean@analog.com> > > > > Hi Alex, > > > > So, it's a fix for a long existing problem. Do we have anyone who > > has experienced it? I'm trying to judge whether this is stable material > > or not. If it is stable material, do you want to have a go at > > identifying > > a patch for the Fixes tag? > > So I actually forgot to do the preparation before cherry-picking this one > [i.e. whether it should have a Fixes tag or not]. > Sorry abou that. > > This change is more in the context of some non-upstream functionality for > the SigmaDelta. i.e. it's a fix, but it's more important for a > functionality that will be added later. > > There aren't any people that have experienced issues with the mainline > version of the ad_sigma_delta drivers that I know of. > > Bottom line is, we can skip adding this one to stable. > Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > Thanks > Alex > > > > Patch looks good. > > > > Jonathan > > > > > --- > > > > > > Changelog v1 -> v2: > > > * added my S-o-B line > > > > > > drivers/iio/adc/ad_sigma_delta.c | 16 +++++++++++----- > > > include/linux/iio/adc/ad_sigma_delta.h | 1 + > > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c > > > b/drivers/iio/adc/ad_sigma_delta.c > > > index ff5f2da2e1b1..af6cbc683214 100644 > > > --- a/drivers/iio/adc/ad_sigma_delta.c > > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > > @@ -62,7 +62,7 @@ int ad_sd_write_reg(struct ad_sigma_delta > > > *sigma_delta, unsigned int reg, > > > struct spi_transfer t = { > > > .tx_buf = data, > > > .len = size + 1, > > > - .cs_change = sigma_delta->bus_locked, > > > + .cs_change = sigma_delta->keep_cs_asserted, > > > }; > > > struct spi_message m; > > > int ret; > > > @@ -217,6 +217,7 @@ static int ad_sd_calibrate(struct ad_sigma_delta > > > *sigma_delta, > > > > > > spi_bus_lock(sigma_delta->spi->master); > > > sigma_delta->bus_locked = true; > > > + sigma_delta->keep_cs_asserted = true; > > > reinit_completion(&sigma_delta->completion); > > > > > > ret = ad_sigma_delta_set_mode(sigma_delta, mode); > > > @@ -234,9 +235,10 @@ static int ad_sd_calibrate(struct ad_sigma_delta > > > *sigma_delta, > > > ret = 0; > > > } > > > out: > > > + sigma_delta->keep_cs_asserted = false; > > > + ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > > sigma_delta->bus_locked = false; > > > spi_bus_unlock(sigma_delta->spi->master); > > > - ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > > > > > return ret; > > > } > > > @@ -289,6 +291,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev > > > *indio_dev, > > > > > > spi_bus_lock(sigma_delta->spi->master); > > > sigma_delta->bus_locked = true; > > > + sigma_delta->keep_cs_asserted = true; > > > reinit_completion(&sigma_delta->completion); > > > > > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE); > > > @@ -298,9 +301,6 @@ int ad_sigma_delta_single_conversion(struct iio_dev > > > *indio_dev, > > > ret = wait_for_completion_interruptible_timeout( > > > &sigma_delta->completion, HZ); > > > > > > - sigma_delta->bus_locked = false; > > > - spi_bus_unlock(sigma_delta->spi->master); > > > - > > > if (ret == 0) > > > ret = -EIO; > > > if (ret < 0) > > > @@ -321,7 +321,10 @@ int ad_sigma_delta_single_conversion(struct > > > iio_dev *indio_dev, > > > sigma_delta->irq_dis = true; > > > } > > > > > > + sigma_delta->keep_cs_asserted = false; > > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > > + sigma_delta->bus_locked = false; > > > + spi_bus_unlock(sigma_delta->spi->master); > > > mutex_unlock(&indio_dev->mlock); > > > > > > if (ret) > > > @@ -358,6 +361,8 @@ static int ad_sd_buffer_postenable(struct iio_dev > > > *indio_dev) > > > > > > spi_bus_lock(sigma_delta->spi->master); > > > sigma_delta->bus_locked = true; > > > + sigma_delta->keep_cs_asserted = true; > > > + > > > ret = ad_sigma_delta_set_mode(sigma_delta, > > > AD_SD_MODE_CONTINUOUS); > > > if (ret) > > > goto err_unlock; > > > @@ -386,6 +391,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev > > > *indio_dev) > > > sigma_delta->irq_dis = true; > > > } > > > > > > + sigma_delta->keep_cs_asserted = false; > > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > > > > > sigma_delta->bus_locked = false; > > > diff --git a/include/linux/iio/adc/ad_sigma_delta.h > > > b/include/linux/iio/adc/ad_sigma_delta.h > > > index 7e84351fa2c0..6e9fb1932dde 100644 > > > --- a/include/linux/iio/adc/ad_sigma_delta.h > > > +++ b/include/linux/iio/adc/ad_sigma_delta.h > > > @@ -69,6 +69,7 @@ struct ad_sigma_delta { > > > bool irq_dis; > > > > > > bool bus_locked; > > > + bool keep_cs_asserted; > > > > > > uint8_t comm; > > > > > > >
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index ff5f2da2e1b1..af6cbc683214 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -62,7 +62,7 @@ int ad_sd_write_reg(struct ad_sigma_delta *sigma_delta, unsigned int reg, struct spi_transfer t = { .tx_buf = data, .len = size + 1, - .cs_change = sigma_delta->bus_locked, + .cs_change = sigma_delta->keep_cs_asserted, }; struct spi_message m; int ret; @@ -217,6 +217,7 @@ static int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, spi_bus_lock(sigma_delta->spi->master); sigma_delta->bus_locked = true; + sigma_delta->keep_cs_asserted = true; reinit_completion(&sigma_delta->completion); ret = ad_sigma_delta_set_mode(sigma_delta, mode); @@ -234,9 +235,10 @@ static int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, ret = 0; } out: + sigma_delta->keep_cs_asserted = false; + ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); sigma_delta->bus_locked = false; spi_bus_unlock(sigma_delta->spi->master); - ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); return ret; } @@ -289,6 +291,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, spi_bus_lock(sigma_delta->spi->master); sigma_delta->bus_locked = true; + sigma_delta->keep_cs_asserted = true; reinit_completion(&sigma_delta->completion); ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_SINGLE); @@ -298,9 +301,6 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, ret = wait_for_completion_interruptible_timeout( &sigma_delta->completion, HZ); - sigma_delta->bus_locked = false; - spi_bus_unlock(sigma_delta->spi->master); - if (ret == 0) ret = -EIO; if (ret < 0) @@ -321,7 +321,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, sigma_delta->irq_dis = true; } + sigma_delta->keep_cs_asserted = false; ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); + sigma_delta->bus_locked = false; + spi_bus_unlock(sigma_delta->spi->master); mutex_unlock(&indio_dev->mlock); if (ret) @@ -358,6 +361,8 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev) spi_bus_lock(sigma_delta->spi->master); sigma_delta->bus_locked = true; + sigma_delta->keep_cs_asserted = true; + ret = ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_CONTINUOUS); if (ret) goto err_unlock; @@ -386,6 +391,7 @@ static int ad_sd_buffer_postdisable(struct iio_dev *indio_dev) sigma_delta->irq_dis = true; } + sigma_delta->keep_cs_asserted = false; ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); sigma_delta->bus_locked = false; diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h index 7e84351fa2c0..6e9fb1932dde 100644 --- a/include/linux/iio/adc/ad_sigma_delta.h +++ b/include/linux/iio/adc/ad_sigma_delta.h @@ -69,6 +69,7 @@ struct ad_sigma_delta { bool irq_dis; bool bus_locked; + bool keep_cs_asserted; uint8_t comm;