[RFC, PATCH] SPI: fix return value for spi_read and spi_write

[RFC, PATCH] SPI: fix return value for spi_read and spi_write

[Marc Pignat]

spi_read and spi_write functions don't report late errors, for instance fifo
underrun.

Signed-off-by: Marc Pignat <marc.pignat-7TsPiqsLilE@public.gmane.org>

---

Hi all!

spi_read and spi_write should not only report errors from spi_sync, but
also from message->status when spi_sync worked.

Regards

Marc

--- include/linux/spi/spi.h.orig	2007-10-18 18:24:15.000000000 +0200
+++ include/linux/spi/spi.h	2007-10-18 18:38:10.000000000 +0200
@@ -593,6 +593,7 @@ extern int spi_sync(struct spi_device *s
 static inline int
 spi_write(struct spi_device *spi, const u8 *buf, size_t len)
 {
+	int status;
 	struct spi_transfer	t = {
 			.tx_buf		= buf,
 			.len		= len,
@@ -601,7 +602,10 @@ spi_write(struct spi_device *spi, const 
 
 	spi_message_init(&m);
 	spi_message_add_tail(&t, &m);
-	return spi_sync(spi, &m);
+
+	status = spi_sync(spi, &m);
+
+	return (status) ? status : m.status;
 }
 
 /**
@@ -617,6 +621,7 @@ spi_write(struct spi_device *spi, const 
 static inline int
 spi_read(struct spi_device *spi, u8 *buf, size_t len)
 {
+	int status;
 	struct spi_transfer	t = {
 			.rx_buf		= buf,
 			.len		= len,
@@ -625,7 +630,10 @@ spi_read(struct spi_device *spi, u8 *buf
 
 	spi_message_init(&m);
 	spi_message_add_tail(&t, &m);
-	return spi_sync(spi, &m);
+
+	status = spi_sync(spi, &m);
+
+	return (status) ? status : m.status;
 }
 
 /* this copies txbuf and rxbuf data; for small transfers only! */

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [RFC, PATCH] SPI: fix return value for spi_read and spi_write

[David Brownell]

> From: Marc Pignat <marc.pignat-7TsPiqsLilE@public.gmane.org>
> Date: Thu, 18 Oct 2007 18:40:38 +0200
>
> spi_read and spi_write functions don't report late errors, for
> instance fifo underrun.
>
> Signed-off-by: Marc Pignat <marc.pignat-7TsPiqsLilE@public.gmane.org>
>
> ---

Seems right to me.  This aspect of the spi_sync() calling
convention can be annoying and error-prone.  I've sometimes
thought about changing that instead of auditing all callers.

Are there any comments from others on this list?  Which
approach would be preferred:  fixing callers (this is at
least a partial fix for that) or updating spi_sync() so
that callers don't need to check m->status?

- Dave


> --- include/linux/spi/spi.h.orig	2007-10-18 18:24:15.000000000 +0200
> +++ include/linux/spi/spi.h	2007-10-18 18:38:10.000000000 +0200
> @@ -593,6 +593,7 @@ extern int spi_sync(struct spi_device *s
>  static inline int
>  spi_write(struct spi_device *spi, const u8 *buf, size_t len)
>  {
> +	int status;
>  	struct spi_transfer	t = {
>  			.tx_buf		= buf,
>  			.len		= len,
> @@ -601,7 +602,10 @@ spi_write(struct spi_device *spi, const 
>  
>  	spi_message_init(&m);
>  	spi_message_add_tail(&t, &m);
> -	return spi_sync(spi, &m);
> +
> +	status = spi_sync(spi, &m);
> +
> +	return (status) ? status : m.status;
>  }
>  
>  /**
> @@ -617,6 +621,7 @@ spi_write(struct spi_device *spi, const 
>  static inline int
>  spi_read(struct spi_device *spi, u8 *buf, size_t len)
>  {
> +	int status;
>  	struct spi_transfer	t = {
>  			.rx_buf		= buf,
>  			.len		= len,
> @@ -625,7 +630,10 @@ spi_read(struct spi_device *spi, u8 *buf
>  
>  	spi_message_init(&m);
>  	spi_message_add_tail(&t, &m);
> -	return spi_sync(spi, &m);
> +
> +	status = spi_sync(spi, &m);
> +
> +	return (status) ? status : m.status;
>  }
>  
>  /* this copies txbuf and rxbuf data; for small transfers only! */
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [RFC, PATCH] SPI: fix return value for spi_read and spi_write

[Marc Pignat]

Hi!

On Friday 19 October 2007, David Brownell wrote:
> > From: Marc Pignat <marc.pignat-7TsPiqsLilE@public.gmane.org>
...
> Seems right to me.  This aspect of the spi_sync() calling
> convention can be annoying and error-prone.  I've sometimes
> thought about changing that instead of auditing all callers.
> 
> Are there any comments from others on this list?  Which
> approach would be preferred:  fixing callers (this is at
> least a partial fix for that) or updating spi_sync() so
> that callers don't need to check m->status?
> 
> - Dave

The API will be simpler with an updated spi_sync().

Regards

Marc

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [RFC, PATCH] SPI: fix return value for spi_read and spi_write

[Marc Pignat]

Hi Dave!

On Friday 19 October 2007, David Brownell wrote:
> > From: Marc Pignat <marc.pignat-7TsPiqsLilE@public.gmane.org>
> > Date: Thu, 18 Oct 2007 18:40:38 +0200
> >
> > spi_read and spi_write functions don't report late errors, for
> > instance fifo underrun.
> >
> > Signed-off-by: Marc Pignat <marc.pignat-7TsPiqsLilE@public.gmane.org>
> >
> > ---
> 
> Seems right to me.  This aspect of the spi_sync() calling
> convention can be annoying and error-prone.  I've sometimes
> thought about changing that instead of auditing all callers.
> 
> Are there any comments from others on this list?  Which
> approach would be preferred:  fixing callers (this is at
> least a partial fix for that) or updating spi_sync() so
> that callers don't need to check m->status?
> 

Here is a list of files using spi_sync (as in 2.6.23.1):
check only return value, does not check m->status:
drivers/mtd/devices/m25p80.c
drivers/mtd/devices/mtd_dataflash.c
drivers/spi/tle62x0.c
drivers/spi/spidev.c
drivers/spi/at25.c
include/linux/spi.h

read both return value and m->status, but does some unnecessary code when status!=0
drivers/rtc/rtc-max6902.c
drivers/spi/spi.c
drivers/input/touchscreen/ads7846.c

My conclusions: 
 * I can't find any spi_sync user wich does The Right Think(TM)
 * We should fix spi_sync (and not the callers), so forget the this patch and wait for the next one!

Regards

Marc

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/