From: Justin Chen <[email protected]>
To read a channel we require 3 cycles to send, process, and receive
the data. The transfer buffer for the third transaction is left blank.
This leaves it up to the SPI driver to decide what to do.
In one particular case, if the tx buffer is not set the spi driver
sets it to 0xff. This puts the ADC in a alarm programming state,
therefore the following read to a channel becomes erroneous.
Instead of leaving us to the mercy of the SPI driver, we send the
ADC cmd on the third transaction to prevent inconsistent behavior.
Fixes: 902c4b2446d4 ("iio: adc: New driver for TI ADS7950 chips")
Signed-off-by: Justin Chen <[email protected]>
---
drivers/iio/adc/ti-ads7950.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 0ad6359..1255d8b 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -422,6 +422,7 @@ static int ti_ads7950_probe(struct spi_device *spi)
st->scan_single_xfer[1].tx_buf = &st->single_tx;
st->scan_single_xfer[1].len = 2;
st->scan_single_xfer[1].cs_change = 1;
+ st->scan_single_xfer[2].tx_buf = &st->single_tx;
st->scan_single_xfer[2].rx_buf = &st->single_rx;
st->scan_single_xfer[2].len = 2;
--
2.7.4
On 1/25/19 10:20 AM, [email protected] wrote:
> From: Justin Chen <[email protected]>
>
> To read a channel we require 3 cycles to send, process, and receive
> the data. The transfer buffer for the third transaction is left blank.
> This leaves it up to the SPI driver to decide what to do.
>
> In one particular case, if the tx buffer is not set the spi driver
> sets it to 0xff. This puts the ADC in a alarm programming state,
> therefore the following read to a channel becomes erroneous.
>
> Instead of leaving us to the mercy of the SPI driver, we send the
> ADC cmd on the third transaction to prevent inconsistent behavior.
>
> Fixes: 902c4b2446d4 ("iio: adc: New driver for TI ADS7950 chips")
> Signed-off-by: Justin Chen <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Thanks!
--
Florian
On Fri, 25 Jan 2019 10:20:22 -0800
[email protected] wrote:
> From: Justin Chen <[email protected]>
>
> To read a channel we require 3 cycles to send, process, and receive
> the data. The transfer buffer for the third transaction is left blank.
> This leaves it up to the SPI driver to decide what to do.
Interesting. I think that means you may have a bug in your SPI driver.
The pointer in question is always left set to NULL in the adc
driver (not explicitly but it will be because ultimately comes from
a kzalloc).
Documentation for an SPI message makes it clear that NULL is
allowed. From include/linux/spi/spi.h
"* Those segments always read the same number of bits as they
* write; but one or the other is easily ignored by passing a null buffer
* pointer. "
Hmm. It does say 'ignored'. My assumption has always been that
means it would be filled with zeros, but maybe I'm wrong.
Mark?
>
> In one particular case, if the tx buffer is not set the spi driver
> sets it to 0xff. This puts the ADC in a alarm programming state,
> therefore the following read to a channel becomes erroneous.
>
> Instead of leaving us to the mercy of the SPI driver, we send the
> ADC cmd on the third transaction to prevent inconsistent behavior.
>
> Fixes: 902c4b2446d4 ("iio: adc: New driver for TI ADS7950 chips")
> Signed-off-by: Justin Chen <[email protected]>
> ---
> drivers/iio/adc/ti-ads7950.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 0ad6359..1255d8b 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -422,6 +422,7 @@ static int ti_ads7950_probe(struct spi_device *spi)
> st->scan_single_xfer[1].tx_buf = &st->single_tx;
> st->scan_single_xfer[1].len = 2;
> st->scan_single_xfer[1].cs_change = 1;
> + st->scan_single_xfer[2].tx_buf = &st->single_tx;
> st->scan_single_xfer[2].rx_buf = &st->single_rx;
> st->scan_single_xfer[2].len = 2;
>
On Sat, Jan 26, 2019 at 10:30 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 25 Jan 2019 10:20:22 -0800
> [email protected] wrote:
>
> > From: Justin Chen <[email protected]>
> >
> > To read a channel we require 3 cycles to send, process, and receive
> > the data. The transfer buffer for the third transaction is left blank.
> > This leaves it up to the SPI driver to decide what to do.
> Interesting. I think that means you may have a bug in your SPI driver.
> The pointer in question is always left set to NULL in the adc
> driver (not explicitly but it will be because ultimately comes from
> a kzalloc).
>
> Documentation for an SPI message makes it clear that NULL is
> allowed. From include/linux/spi/spi.h
>
> "* Those segments always read the same number of bits as they
> * write; but one or the other is easily ignored by passing a null buffer
> * pointer. "
>
> Hmm. It does say 'ignored'. My assumption has always been that
> means it would be filled with zeros, but maybe I'm wrong.
>
> Mark?
>
I looked more into this and other SPI drivers do fill it with zeros.
I'll submit a patch for the SPI driver instead and see what the SPI
maintainers say.
Thanks,
Justin
> >
> > In one particular case, if the tx buffer is not set the spi driver
> > sets it to 0xff. This puts the ADC in a alarm programming state,
> > therefore the following read to a channel becomes erroneous.
> >
> > Instead of leaving us to the mercy of the SPI driver, we send the
> > ADC cmd on the third transaction to prevent inconsistent behavior.
> >
> > Fixes: 902c4b2446d4 ("iio: adc: New driver for TI ADS7950 chips")
> > Signed-off-by: Justin Chen <[email protected]>
> > ---
> > drivers/iio/adc/ti-ads7950.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> > index 0ad6359..1255d8b 100644
> > --- a/drivers/iio/adc/ti-ads7950.c
> > +++ b/drivers/iio/adc/ti-ads7950.c
> > @@ -422,6 +422,7 @@ static int ti_ads7950_probe(struct spi_device *spi)
> > st->scan_single_xfer[1].tx_buf = &st->single_tx;
> > st->scan_single_xfer[1].len = 2;
> > st->scan_single_xfer[1].cs_change = 1;
> > + st->scan_single_xfer[2].tx_buf = &st->single_tx;
> > st->scan_single_xfer[2].rx_buf = &st->single_rx;
> > st->scan_single_xfer[2].len = 2;
> >
>