* Re: [PATCH] iio: adc: ti-ads7950: Ensure CS is deasserted after reading channels [not found] ` <00b3dd46-48ba-3d40-36dd-79372a956085@lechnology.com> @ 2021-07-10 12:18 ` Uwe Kleine-König 2021-07-10 15:32 ` David Lechner 0 siblings, 1 reply; 3+ messages in thread From: Uwe Kleine-König @ 2021-07-10 12:18 UTC (permalink / raw) To: David Lechner Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, kernel@pengutronix.de Mark Brown, linux-spi [-- Attachment #1: Type: text/plain, Size: 2304 bytes --] 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: adc: ti-ads7950: Ensure CS is deasserted after reading channels 2021-07-10 12:18 ` [PATCH] iio: adc: ti-ads7950: Ensure CS is deasserted after reading channels Uwe Kleine-König @ 2021-07-10 15:32 ` David Lechner 2021-07-12 11:49 ` Mark Brown 0 siblings, 1 reply; 3+ messages in thread From: David Lechner @ 2021-07-10 15:32 UTC (permalink / raw) To: Uwe Kleine-König Cc: Jonathan Cameron, linux-iio, Lars-Peter Clausen, kernel@pengutronix.de Mark Brown, linux-spi 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: adc: ti-ads7950: Ensure CS is deasserted after reading channels 2021-07-10 15:32 ` David Lechner @ 2021-07-12 11:49 ` Mark Brown 0 siblings, 0 replies; 3+ messages in thread From: Mark Brown @ 2021-07-12 11:49 UTC (permalink / raw) To: David Lechner Cc: Uwe Kleine-König, Jonathan Cameron, linux-iio, Lars-Peter Clausen, linux-spi [-- Attachment #1: Type: text/plain, Size: 1055 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-12 11:49 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210709101110.1814294-1-u.kleine-koenig@pengutronix.de> [not found] ` <00b3dd46-48ba-3d40-36dd-79372a956085@lechnology.com> 2021-07-10 12:18 ` [PATCH] iio: adc: ti-ads7950: Ensure CS is deasserted after reading channels Uwe Kleine-König 2021-07-10 15:32 ` David Lechner 2021-07-12 11:49 ` Mark Brown
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).