Linux-IIO Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg
@ 2019-01-25 18:20 justinpopo6
  2019-01-25 18:38 ` Florian Fainelli
  2019-01-26 18:30 ` Jonathan Cameron
  0 siblings, 2 replies; 4+ messages in thread
From: justinpopo6 @ 2019-01-25 18:20 UTC (permalink / raw)
  To: linux-iio; +Cc: jic23, knaack.h, lars, linux-kernel, f.fainelli, Justin Chen

From: Justin Chen <justinpopo6@gmail.com>

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 <justinpopo6@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg
  2019-01-25 18:20 [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg justinpopo6
@ 2019-01-25 18:38 ` Florian Fainelli
  2019-01-26 18:30 ` Jonathan Cameron
  1 sibling, 0 replies; 4+ messages in thread
From: Florian Fainelli @ 2019-01-25 18:38 UTC (permalink / raw)
  To: justinpopo6, linux-iio; +Cc: jic23, knaack.h, lars, linux-kernel

On 1/25/19 10:20 AM, justinpopo6@gmail.com wrote:
> From: Justin Chen <justinpopo6@gmail.com>
> 
> 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 <justinpopo6@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!
-- 
Florian

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg
  2019-01-25 18:20 [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg justinpopo6
  2019-01-25 18:38 ` Florian Fainelli
@ 2019-01-26 18:30 ` Jonathan Cameron
  2019-01-31  5:15   ` Justin Chen
  1 sibling, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2019-01-26 18:30 UTC (permalink / raw)
  To: justinpopo6
  Cc: linux-iio, knaack.h, lars, linux-kernel, f.fainelli, Mark Brown

On Fri, 25 Jan 2019 10:20:22 -0800
justinpopo6@gmail.com wrote:

> From: Justin Chen <justinpopo6@gmail.com>
> 
> 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 <justinpopo6@gmail.com>
> ---
>  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;
>  


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg
  2019-01-26 18:30 ` Jonathan Cameron
@ 2019-01-31  5:15   ` Justin Chen
  0 siblings, 0 replies; 4+ messages in thread
From: Justin Chen @ 2019-01-31  5:15 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, knaack.h, lars, linux-kernel, Florian Fainelli, Mark Brown

On Sat, Jan 26, 2019 at 10:30 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 25 Jan 2019 10:20:22 -0800
> justinpopo6@gmail.com wrote:
>
> > From: Justin Chen <justinpopo6@gmail.com>
> >
> > 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 <justinpopo6@gmail.com>
> > ---
> >  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;
> >
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-25 18:20 [PATCH v2] iio: adc: ti-ads7950: inconsistency with spi msg justinpopo6
2019-01-25 18:38 ` Florian Fainelli
2019-01-26 18:30 ` Jonathan Cameron
2019-01-31  5:15   ` Justin Chen

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox