From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
To: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
Cc: "lars@metafoo.de" <lars@metafoo.de>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH V2] iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion
Date: Sun, 31 Mar 2019 12:07:46 +0100 [thread overview]
Message-ID: <20190331120746.01ab83ba@archlinux> (raw)
In-Reply-To: <547665b579684b09246670e7df9b60cde730c1e9.camel@analog.com>
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;
> > >
> >
> >
prev parent reply other threads:[~2019-03-31 11:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-19 11:37 [PATCH V2] iio: ad_sigma_delta: Properly handle SPI bus locking vs CS assertion Alexandru Ardelean
2019-03-24 17:47 ` Jonathan Cameron
2019-03-26 12:43 ` Ardelean, Alexandru
2019-03-31 11:07 ` Jonathan Cameron [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190331120746.01ab83ba@archlinux \
--to=jic23@jic23.retrosnub.co.uk \
--cc=alexandru.Ardelean@analog.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).