| Message ID | 20210709101110.1814294-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jonathan Cameron |
| Headers | show |
| Series | iio: adc: ti-ads7950: Ensure CS is deasserted after reading channels | expand |
On 7/9/21 5:11 AM, Uwe Kleine-König wrote: > The ADS7950 requires that CS is deasserted after each SPI word. Before > commit e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce > CPU usage") the driver used a message with one spi transfer per channel > where each but the last one had .cs_change set to enforce a CS toggle. > This was wrongly translated into a message with a single transfer and > .cs_change set which results in a CS toggle after each word but the > last which corrupts the first adc conversion of all readouts after the > first readout. > > Fixes: e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/iio/adc/ti-ads7950.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c > index 2383eacada87..a2b83f0bd526 100644 > --- a/drivers/iio/adc/ti-ads7950.c > +++ b/drivers/iio/adc/ti-ads7950.c > @@ -568,7 +568,6 @@ static int ti_ads7950_probe(struct spi_device *spi) > st->ring_xfer.tx_buf = &st->tx_buf[0]; > st->ring_xfer.rx_buf = &st->rx_buf[0]; > /* len will be set later */ > - st->ring_xfer.cs_change = true; > > spi_message_add_tail(&st->ring_xfer, &st->ring_msg); > > Yes, it seems like the SPI_CS_WORD flag should have replaced this (it's been too long, I can't remember if it was intentional). And removing it doesn't seem to break anything for me. Reviewed-by: David Lechner <david@lechnology.com> Tested-by: David Lechner <david@lechnology.com>
Hello, Cc += Mark + linux-spi On Fri, Jul 09, 2021 at 11:39:48AM -0500, David Lechner wrote: > On 7/9/21 5:11 AM, Uwe Kleine-König wrote: > > The ADS7950 requires that CS is deasserted after each SPI word. Before > > commit e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce > > CPU usage") the driver used a message with one spi transfer per channel > > where each but the last one had .cs_change set to enforce a CS toggle. > > This was wrongly translated into a message with a single transfer and > > .cs_change set which results in a CS toggle after each word but the > > last which corrupts the first adc conversion of all readouts after the > > first readout. > > > > Fixes: e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/iio/adc/ti-ads7950.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c > > index 2383eacada87..a2b83f0bd526 100644 > > --- a/drivers/iio/adc/ti-ads7950.c > > +++ b/drivers/iio/adc/ti-ads7950.c > > @@ -568,7 +568,6 @@ static int ti_ads7950_probe(struct spi_device *spi) > > st->ring_xfer.tx_buf = &st->tx_buf[0]; > > st->ring_xfer.rx_buf = &st->rx_buf[0]; > > /* len will be set later */ > > - st->ring_xfer.cs_change = true; > > spi_message_add_tail(&st->ring_xfer, &st->ring_msg); > > > > Yes, it seems like the SPI_CS_WORD flag should have replaced this (it's > been too long, I can't remember if it was intentional). And removing it > doesn't seem to break anything for me. If it's not broken for you without my patch, your spi bus driver doesn't honor .cs_change in the last transfer. Out of interest: Which bus are you using? I wonder if the driver should refuse the request if it cannot honer .cs_change?! (spi-imx does honor it only if gpios are used as chip select, the native chip selects cannot do that.) > Reviewed-by: David Lechner <david@lechnology.com> > Tested-by: David Lechner <david@lechnology.com> Thanks Best regards Uwe
On 7/10/21 7:18 AM, Uwe Kleine-König wrote: > Hello, > > Cc += Mark + linux-spi > > On Fri, Jul 09, 2021 at 11:39:48AM -0500, David Lechner wrote: >> On 7/9/21 5:11 AM, Uwe Kleine-König wrote: >>> The ADS7950 requires that CS is deasserted after each SPI word. Before >>> commit e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce >>> CPU usage") the driver used a message with one spi transfer per channel >>> where each but the last one had .cs_change set to enforce a CS toggle. >>> This was wrongly translated into a message with a single transfer and >>> .cs_change set which results in a CS toggle after each word but the >>> last which corrupts the first adc conversion of all readouts after the >>> first readout. >>> >>> Fixes: e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage") >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> --- >>> drivers/iio/adc/ti-ads7950.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c >>> index 2383eacada87..a2b83f0bd526 100644 >>> --- a/drivers/iio/adc/ti-ads7950.c >>> +++ b/drivers/iio/adc/ti-ads7950.c >>> @@ -568,7 +568,6 @@ static int ti_ads7950_probe(struct spi_device *spi) >>> st->ring_xfer.tx_buf = &st->tx_buf[0]; >>> st->ring_xfer.rx_buf = &st->rx_buf[0]; >>> /* len will be set later */ >>> - st->ring_xfer.cs_change = true; >>> spi_message_add_tail(&st->ring_xfer, &st->ring_msg); >>> >> >> Yes, it seems like the SPI_CS_WORD flag should have replaced this (it's >> been too long, I can't remember if it was intentional). And removing it >> doesn't seem to break anything for me. > > If it's not broken for you without my patch, your spi bus driver doesn't > honor .cs_change in the last transfer. Out of interest: Which bus are > you using? I wonder if the driver should refuse the request if it cannot > honer .cs_change?! (spi-imx does honor it only if gpios are used as chip > select, the native chip selects cannot do that.) > I'm using spi-davinci. It uses the standard spi_transfer_one_message() which handles cs_change. But I suspect when the SPI_CS_WORD flag is set, and the message is big enough to use DMA, the hardware is probably automatically toggling CS after the last transfer before the cs_change logic asserts it again. So unless there is a valid use case where we need both SPI_CS_WORD and cs_change, I don't think we need to fix spi-davinci.
On Sat, Jul 10, 2021 at 10:32:46AM -0500, David Lechner wrote: > On 7/10/21 7:18 AM, Uwe Kleine-König wrote: > > If it's not broken for you without my patch, your spi bus driver doesn't > > honor .cs_change in the last transfer. Out of interest: Which bus are > > you using? I wonder if the driver should refuse the request if it cannot > > honer .cs_change?! (spi-imx does honor it only if gpios are used as chip > > select, the native chip selects cannot do that.) > I'm using spi-davinci. It uses the standard spi_transfer_one_message() > which handles cs_change. But I suspect when the SPI_CS_WORD flag is set, > and the message is big enough to use DMA, the hardware is probably > automatically toggling CS after the last transfer before the cs_change > logic asserts it again. > So unless there is a valid use case where we need both SPI_CS_WORD > and cs_change, I don't think we need to fix spi-davinci. In theory it does mean something, in practice the number of controllers that can implement it must be approaching zero.
On Fri, 9 Jul 2021 11:39:48 -0500 David Lechner <david@lechnology.com> wrote: > On 7/9/21 5:11 AM, Uwe Kleine-König wrote: > > The ADS7950 requires that CS is deasserted after each SPI word. Before > > commit e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce > > CPU usage") the driver used a message with one spi transfer per channel > > where each but the last one had .cs_change set to enforce a CS toggle. > > This was wrongly translated into a message with a single transfer and > > .cs_change set which results in a CS toggle after each word but the > > last which corrupts the first adc conversion of all readouts after the > > first readout. > > > > Fixes: e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/iio/adc/ti-ads7950.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c > > index 2383eacada87..a2b83f0bd526 100644 > > --- a/drivers/iio/adc/ti-ads7950.c > > +++ b/drivers/iio/adc/ti-ads7950.c > > @@ -568,7 +568,6 @@ static int ti_ads7950_probe(struct spi_device *spi) > > st->ring_xfer.tx_buf = &st->tx_buf[0]; > > st->ring_xfer.rx_buf = &st->rx_buf[0]; > > /* len will be set later */ > > - st->ring_xfer.cs_change = true; > > > > spi_message_add_tail(&st->ring_xfer, &st->ring_msg); > > > > > > Yes, it seems like the SPI_CS_WORD flag should have replaced this (it's > been too long, I can't remember if it was intentional). And removing it > doesn't seem to break anything for me. > > Reviewed-by: David Lechner <david@lechnology.com> > Tested-by: David Lechner <david@lechnology.com> Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c index 2383eacada87..a2b83f0bd526 100644 --- a/drivers/iio/adc/ti-ads7950.c +++ b/drivers/iio/adc/ti-ads7950.c @@ -568,7 +568,6 @@ static int ti_ads7950_probe(struct spi_device *spi) st->ring_xfer.tx_buf = &st->tx_buf[0]; st->ring_xfer.rx_buf = &st->rx_buf[0]; /* len will be set later */ - st->ring_xfer.cs_change = true; spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
The ADS7950 requires that CS is deasserted after each SPI word. Before commit e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage") the driver used a message with one spi transfer per channel where each but the last one had .cs_change set to enforce a CS toggle. This was wrongly translated into a message with a single transfer and .cs_change set which results in a CS toggle after each word but the last which corrupts the first adc conversion of all readouts after the first readout. Fixes: e2540da86ef8 ("iio: adc: ti-ads7950: use SPI_CS_WORD to reduce CPU usage") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/iio/adc/ti-ads7950.c | 1 - 1 file changed, 1 deletion(-)