[PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

There are some boards that do not strictly follow SPI standard and use only 3 wires (SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO based 'spi controller'. In this configuration the MISO line is missing (it is not required if the chip does not transfer any data back to host). The example of such board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard configuration in GPIO-based SPI controller.

Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

--- 

diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 26bd03e..5b75601 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
 
 static inline int getmiso(const struct spi_device *spi)
 {
-	return !!gpio_get_value(SPI_MISO_GPIO);
+	if (SPI_MISO_GPIO)
+		return !!gpio_get_value(SPI_MISO_GPIO);
+	else
+		return 0;
 }
 
 #undef pdata
@@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	if (value)
 		goto done;
 
-	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
-	if (value)
-		goto free_mosi;
+	if (SPI_MISO_GPIO) {
+		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
+		if (value)
+			goto free_mosi;
+	}
 
 	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
 	if (value)
@@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	goto done;
 
 free_miso:
-	gpio_free(SPI_MISO_GPIO);
+	if (SPI_MISO_GPIO)
+		gpio_free(SPI_MISO_GPIO);
 free_mosi:
 	gpio_free(SPI_MOSI_GPIO);
 done:
@@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
 	if (status < 0) {
 		spi_master_put(spi_gpio->bitbang.master);
 gpio_free:
-		gpio_free(SPI_MISO_GPIO);
+		if (SPI_MISO_GPIO)
+			gpio_free(SPI_MISO_GPIO);
 		gpio_free(SPI_MOSI_GPIO);
 		gpio_free(SPI_SCK_GPIO);
 		spi_master_put(master);
@@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	gpio_free(SPI_MISO_GPIO);
+	if (SPI_MISO_GPIO)
+		gpio_free(SPI_MISO_GPIO);
 	gpio_free(SPI_MOSI_GPIO);
 	gpio_free(SPI_SCK_GPIO);

Re: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Ben Dooks]

On Thu, May 07, 2009 at 02:24:11PM +0200, Marek Szyprowski wrote:
> There are some boards that do not strictly follow SPI standard and use only 3 wires (SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO based 'spi controller'. In this configuration the MISO line is missing (it is not required if the chip does not transfer any data back to host). The example of such board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard configuration in GPIO-based SPI controller.

Please wrap your descriptions to 77 or less characters per line.
 
> Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> --- 
> 
> diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
> index 26bd03e..5b75601 100644
> --- a/drivers/spi/spi_gpio.c
> +++ b/drivers/spi/spi_gpio.c
> @@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
>  
>  static inline int getmiso(const struct spi_device *spi)
>  {
> -	return !!gpio_get_value(SPI_MISO_GPIO);
> +	if (SPI_MISO_GPIO)
> +		return !!gpio_get_value(SPI_MISO_GPIO);
> +	else
> +		return 0;
>  }

Is zero a good approximation for 'no gpio' ?
  
>  #undef pdata
> @@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
>  	if (value)
>  		goto done;
>  
> -	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
> -	if (value)
> -		goto free_mosi;
> +	if (SPI_MISO_GPIO) {
> +		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
> +		if (value)
> +			goto free_mosi;
> +	}
>  
>  	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
>  	if (value)
> @@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
>  	goto done;
>  
>  free_miso:
> -	gpio_free(SPI_MISO_GPIO);
> +	if (SPI_MISO_GPIO)
> +		gpio_free(SPI_MISO_GPIO);
>  free_mosi:
>  	gpio_free(SPI_MOSI_GPIO);
>  done:
> @@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
>  	if (status < 0) {
>  		spi_master_put(spi_gpio->bitbang.master);
>  gpio_free:
> -		gpio_free(SPI_MISO_GPIO);
> +		if (SPI_MISO_GPIO)
> +			gpio_free(SPI_MISO_GPIO);
>  		gpio_free(SPI_MOSI_GPIO);
>  		gpio_free(SPI_SCK_GPIO);
>  		spi_master_put(master);
> @@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, NULL);
>  
> -	gpio_free(SPI_MISO_GPIO);
> +	if (SPI_MISO_GPIO)
> +		gpio_free(SPI_MISO_GPIO);
>  	gpio_free(SPI_MOSI_GPIO);
>  	gpio_free(SPI_SCK_GPIO);
> 
> 
> -------------------------------------------------------------------
> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

RE: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

Hello,

On Thursday, May 07, 2009 2:32 PM Ben Dooks wrote:

> On Thu, May 07, 2009 at 02:24:11PM +0200, Marek Szyprowski wrote:
> > There are some boards that do not strictly follow SPI standard and
> use only 3 wires (SCLK, MOSI, SS) for connecting some simple auxiliary
> chips and controls them with GPIO based 'spi controller'. In this
> configuration the MISO line is missing (it is not required if the chip
> does not transfer any data back to host). The example of such board is
> a NCP ARM S3C64XX based machine. This patch adds support for such non-
> standard configuration in GPIO-based SPI controller.
> [...]
> > diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
> > index 26bd03e..5b75601 100644
> > --- a/drivers/spi/spi_gpio.c
> > +++ b/drivers/spi/spi_gpio.c
> > @@ -114,7 +114,10 @@ static inline void setmosi(const struct
> spi_device *spi, int is_on)
> >
> >  static inline int getmiso(const struct spi_device *spi)
> >  {
> > -	return !!gpio_get_value(SPI_MISO_GPIO);
> > +	if (SPI_MISO_GPIO)
> > +		return !!gpio_get_value(SPI_MISO_GPIO);
> > +	else
> > +		return 0;
> >  }
> 
> Is zero a good approximation for 'no gpio' ?

Now I found that zero might be a valid gpio pin number on some architectures
(it just means GPIO0 pin). This is imho a bit strange behavior of gpiolib as
there should be also a special values for INVALID or NOGPIO cases. Does 
anyone have any ideas how such cases should be handled properly? 

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Ben Dooks]

On Mon, May 18, 2009 at 10:30:55AM +0200, Marek Szyprowski wrote:
> Hello,
> 
> On Thursday, May 07, 2009 2:32 PM Ben Dooks wrote:
> 
> > On Thu, May 07, 2009 at 02:24:11PM +0200, Marek Szyprowski wrote:
> > > There are some boards that do not strictly follow SPI standard and
> > use only 3 wires (SCLK, MOSI, SS) for connecting some simple auxiliary
> > chips and controls them with GPIO based 'spi controller'. In this
> > configuration the MISO line is missing (it is not required if the chip
> > does not transfer any data back to host). The example of such board is
> > a NCP ARM S3C64XX based machine. This patch adds support for such non-
> > standard configuration in GPIO-based SPI controller.
> > [...]
> > > diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
> > > index 26bd03e..5b75601 100644
> > > --- a/drivers/spi/spi_gpio.c
> > > +++ b/drivers/spi/spi_gpio.c
> > > @@ -114,7 +114,10 @@ static inline void setmosi(const struct
> > spi_device *spi, int is_on)
> > >
> > >  static inline int getmiso(const struct spi_device *spi)
> > >  {
> > > -	return !!gpio_get_value(SPI_MISO_GPIO);
> > > +	if (SPI_MISO_GPIO)
> > > +		return !!gpio_get_value(SPI_MISO_GPIO);
> > > +	else
> > > +		return 0;
> > >  }
> > 
> > Is zero a good approximation for 'no gpio' ?
> 
> Now I found that zero might be a valid gpio pin number on some architectures
> (it just means GPIO0 pin). This is imho a bit strange behavior of gpiolib as
> there should be also a special values for INVALID or NOGPIO cases. Does 
> anyone have any ideas how such cases should be handled properly? 

I belive there is a gpio_is_valid() function to tell you precisely if the
given GPIO is valid.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Daniel Mack]

On Mon, May 18, 2009 at 09:34:12AM +0100, Ben Dooks wrote:
> > > >  static inline int getmiso(const struct spi_device *spi)
> > > >  {
> > > > -	return !!gpio_get_value(SPI_MISO_GPIO);
> > > > +	if (SPI_MISO_GPIO)
> > > > +		return !!gpio_get_value(SPI_MISO_GPIO);
> > > > +	else
> > > > +		return 0;
> > > >  }
> > > 
> > > Is zero a good approximation for 'no gpio' ?
> > 
> > Now I found that zero might be a valid gpio pin number on some architectures
> > (it just means GPIO0 pin). This is imho a bit strange behavior of gpiolib as
> > there should be also a special values for INVALID or NOGPIO cases. Does 
> > anyone have any ideas how such cases should be handled properly? 
> 
> I belive there is a gpio_is_valid() function to tell you precisely if the
> given GPIO is valid.

And then -1 can be passed in from the platform data which will fail the
gpio_is_valid() test.

Daniel

RE: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

Hello,

On Monday, May 18, 2009 10:57 AM Daniel Mack wrote:

> On Mon, May 18, 2009 at 09:34:12AM +0100, Ben Dooks wrote:
> > > > >  static inline int getmiso(const struct spi_device *spi)
> > > > >  {
> > > > > -	return !!gpio_get_value(SPI_MISO_GPIO);
> > > > > +	if (SPI_MISO_GPIO)
> > > > > +		return !!gpio_get_value(SPI_MISO_GPIO);
> > > > > +	else
> > > > > +		return 0;
> > > > >  }
> > > >
> > > > Is zero a good approximation for 'no gpio' ?
> > >
> > > Now I found that zero might be a valid gpio pin number on some
> architectures
> > > (it just means GPIO0 pin). This is imho a bit strange behavior of
> gpiolib as
> > > there should be also a special values for INVALID or NOGPIO cases.
> Does
> > > anyone have any ideas how such cases should be handled properly?
> >
> > I belive there is a gpio_is_valid() function to tell you precisely if
> > the given GPIO is valid.
> 
> And then -1 can be passed in from the platform data which will fail the
> gpio_is_valid() test.

Ok. I will update my patch. Thank you for the idea.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

[PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

There are some boards that do not strictly follow SPI standard and use only 3 wires
(SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO
based 'spi controller'. In this configuration the MISO line is missing (it is not
required if the chip does not transfer any data back to host). The example of such
board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard
configuration in GPIO-based SPI controller.

Reviewed-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

--- 

diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 26bd03e..16f74fd 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
 
 static inline int getmiso(const struct spi_device *spi)
 {
-	return !!gpio_get_value(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		return !!gpio_get_value(SPI_MISO_GPIO);
+	else
+		return 0;
 }
 
 #undef pdata
@@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	if (value)
 		goto done;
 
-	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
-	if (value)
-		goto free_mosi;
+	if (gpio_is_valid(SPI_MISO_GPIO)) {
+		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
+		if (value)
+			goto free_mosi;
+	}
 
 	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
 	if (value)
@@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	goto done;
 
 free_miso:
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 free_mosi:
 	gpio_free(SPI_MOSI_GPIO);
 done:
@@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
 	if (status < 0) {
 		spi_master_put(spi_gpio->bitbang.master);
 gpio_free:
-		gpio_free(SPI_MISO_GPIO);
+		if (gpio_is_valid(SPI_MISO_GPIO))
+			gpio_free(SPI_MISO_GPIO);
 		gpio_free(SPI_MOSI_GPIO);
 		gpio_free(SPI_SCK_GPIO);
 		spi_master_put(master);
@@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 	gpio_free(SPI_MOSI_GPIO);
 	gpio_free(SPI_SCK_GPIO);



------------------------------------------------------------------------------
Register Now for Creativity and Technology (CaT), June 3rd, NYC. CaT
is a gathering of tech-side developers & brand creativity professionals. Meet
the minds behind Google Creative Lab, Visual Complexity, Processing, & 
iPhoneDevCamp asthey present alongside digital heavyweights like Barbarian
Group, R/GA, & Big Spaceship. http://www.creativitycat.com 

[PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

There are some boards that do not strictly follow SPI standard and use only 3 wires
(SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO
based 'spi controller'. In this configuration the MISO line is missing (it is not
required if the chip does not transfer any data back to host). The example of such
board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard
configuration in GPIO-based SPI controller.

Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

--- 

diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 26bd03e..16f74fd 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
 
 static inline int getmiso(const struct spi_device *spi)
 {
-	return !!gpio_get_value(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		return !!gpio_get_value(SPI_MISO_GPIO);
+	else
+		return 0;
 }
 
 #undef pdata
@@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	if (value)
 		goto done;
 
-	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
-	if (value)
-		goto free_mosi;
+	if (gpio_is_valid(SPI_MISO_GPIO)) {
+		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
+		if (value)
+			goto free_mosi;
+	}
 
 	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
 	if (value)
@@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	goto done;
 
 free_miso:
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 free_mosi:
 	gpio_free(SPI_MOSI_GPIO);
 done:
@@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
 	if (status < 0) {
 		spi_master_put(spi_gpio->bitbang.master);
 gpio_free:
-		gpio_free(SPI_MISO_GPIO);
+		if (gpio_is_valid(SPI_MISO_GPIO))
+			gpio_free(SPI_MISO_GPIO);
 		gpio_free(SPI_MOSI_GPIO);
 		gpio_free(SPI_SCK_GPIO);
 		spi_master_put(master);
@@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 	gpio_free(SPI_MOSI_GPIO);
 	gpio_free(SPI_SCK_GPIO);

[PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

There are some boards that do not strictly follow SPI standard and use only 3 wires
(SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO
based 'spi controller'. In this configuration the MISO line is missing (it is not
required if the chip does not transfer any data back to host). The example of such
board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard
configuration in GPIO-based SPI controller.

Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

--- 

diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 26bd03e..16f74fd 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
 
 static inline int getmiso(const struct spi_device *spi)
 {
-	return !!gpio_get_value(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		return !!gpio_get_value(SPI_MISO_GPIO);
+	else
+		return 0;
 }
 
 #undef pdata
@@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	if (value)
 		goto done;
 
-	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
-	if (value)
-		goto free_mosi;
+	if (gpio_is_valid(SPI_MISO_GPIO)) {
+		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
+		if (value)
+			goto free_mosi;
+	}
 
 	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
 	if (value)
@@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	goto done;
 
 free_miso:
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 free_mosi:
 	gpio_free(SPI_MOSI_GPIO);
 done:
@@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
 	if (status < 0) {
 		spi_master_put(spi_gpio->bitbang.master);
 gpio_free:
-		gpio_free(SPI_MISO_GPIO);
+		if (gpio_is_valid(SPI_MISO_GPIO))
+			gpio_free(SPI_MISO_GPIO);
 		gpio_free(SPI_MOSI_GPIO);
 		gpio_free(SPI_SCK_GPIO);
 		spi_master_put(master);
@@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 	gpio_free(SPI_MOSI_GPIO);
 	gpio_free(SPI_SCK_GPIO);

Re: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[David Brownell]

On Tuesday 23 June 2009, Marek Szyprowski wrote:
> There are some boards that do not strictly follow SPI standard and use only 3 wires
> (SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO
> based 'spi controller'. In this configuration the MISO line is missing (it is not
> required if the chip does not transfer any data back to host). The example of such
> board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard
> configuration in GPIO-based SPI controller.
> 
> Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

This is missing one integrity feature:  it should reject all
requests that provide a read buffer, since obviously it can't
satisfy any such requests.

Also, please add a comment in spi_gpio_request() explaining
this point:  that output-only configs exist, and you're
supporting them by letting MISO be invalid.

Address those two points and this will be almost OK.


Thing is, this raises two related issues:  (a) there's the
analagous input-only case where MOSI isn't used, e.g. for
some kinds of sensor; and (b) there's also "real 3-wire SPI"
(spi->mode & SPI_3WIRE) where interactions are limited to
half duplex and one pin switches roles between MOSI and MISO.

Clearly this "output-only" case is a subset of SPI_3WIRE (the
MOMI/SISO pin can't switch direction) so one more change I
want to see is requiring that spi->mode flag be set in all
SPI devices registered when this mode is used.

If you have time, it would be good to generaize this patch
to cover all of those 3-wire modes ... accept all half
duplex calls, use gpio_direction_*() to switch direction.

If not -- e.g. nothing to test that with! -- then I can
understand, but please make your changes with that model
in mind, and leave an appropriate FIXME in place.

- Dave

p.s. I'll CC you on one more related patch.  Briefly,
     there's a new spi_master->flags field that should
     set SPI_MASTER_HALF_DUPLEX.  Arguably, this specific
     case should also advertise something like NO_RX.


> 
> --- 
> 
> diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
> index 26bd03e..16f74fd 100644
> --- a/drivers/spi/spi_gpio.c
> +++ b/drivers/spi/spi_gpio.c
> @@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
>  
>  static inline int getmiso(const struct spi_device *spi)
>  {
> -	return !!gpio_get_value(SPI_MISO_GPIO);
> +	if (gpio_is_valid(SPI_MISO_GPIO))
> +		return !!gpio_get_value(SPI_MISO_GPIO);
> +	else
> +		return 0;
>  }
>  
>  #undef pdata
> @@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
>  	if (value)
>  		goto done;
>  
> -	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
> -	if (value)
> -		goto free_mosi;
> +	if (gpio_is_valid(SPI_MISO_GPIO)) {
> +		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
> +		if (value)
> +			goto free_mosi;
> +	}
>  
>  	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
>  	if (value)
> @@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
>  	goto done;
>  
>  free_miso:
> -	gpio_free(SPI_MISO_GPIO);
> +	if (gpio_is_valid(SPI_MISO_GPIO))
> +		gpio_free(SPI_MISO_GPIO);
>  free_mosi:
>  	gpio_free(SPI_MOSI_GPIO);
>  done:
> @@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
>  	if (status < 0) {
>  		spi_master_put(spi_gpio->bitbang.master);
>  gpio_free:
> -		gpio_free(SPI_MISO_GPIO);
> +		if (gpio_is_valid(SPI_MISO_GPIO))
> +			gpio_free(SPI_MISO_GPIO);
>  		gpio_free(SPI_MOSI_GPIO);
>  		gpio_free(SPI_SCK_GPIO);
>  		spi_master_put(master);
> @@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, NULL);
>  
> -	gpio_free(SPI_MISO_GPIO);
> +	if (gpio_is_valid(SPI_MISO_GPIO))
> +		gpio_free(SPI_MISO_GPIO);
>  	gpio_free(SPI_MOSI_GPIO);
>  	gpio_free(SPI_SCK_GPIO);
> 
> 
> 

RE: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

Hello,

On Tuesday, June 23, 2009 8:55 PM, David Brownell wrote:

> > There are some boards that do not strictly follow SPI standard and use only 3 wires
> > (SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO
> > based 'spi controller'. In this configuration the MISO line is missing (it is not
> > required if the chip does not transfer any data back to host). The example of such
> > board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard
> > configuration in GPIO-based SPI controller.
> >
> > Reviewed-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> This is missing one integrity feature:  it should reject all
> requests that provide a read buffer, since obviously it can't
> satisfy any such requests.
> 
> Also, please add a comment in spi_gpio_request() explaining
> this point:  that output-only configs exist, and you're
> supporting them by letting MISO be invalid.
> 
> Address those two points and this will be almost OK.

Thanks for your review. I will post an updated patch soon.

> Thing is, this raises two related issues:  (a) there's the
> analagous input-only case where MOSI isn't used, e.g. for
> some kinds of sensor; and (b) there's also "real 3-wire SPI"
> (spi->mode & SPI_3WIRE) where interactions are limited to
> half duplex and one pin switches roles between MOSI and MISO.
> 
> Clearly this "output-only" case is a subset of SPI_3WIRE (the
> MOMI/SISO pin can't switch direction) so one more change I
> want to see is requiring that spi->mode flag be set in all
> SPI devices registered when this mode is used.
> 
> If you have time, it would be good to generaize this patch
> to cover all of those 3-wire modes ... accept all half
> duplex calls, use gpio_direction_*() to switch direction.
>
> If not -- e.g. nothing to test that with! -- then I can
> understand, but please make your changes with that model
> in mind, and leave an appropriate FIXME in place.

I'm sorry, but I have only a hw board with missing MISO pin so I won't
be able to properly check/debug any other configurations. However I
will add a support for missing MOSI pin as well (simple code
reusage).

> p.s. I'll CC you on one more related patch.  Briefly,
>      there's a new spi_master->flags field that should
>      set SPI_MASTER_HALF_DUPLEX.  Arguably, this specific
>      case should also advertise something like NO_RX.

Thanks. I will add SPI_MASTER_NO_TX/NO_RX flags there.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[David Brownell]

On Thursday 25 June 2009, Marek Szyprowski wrote:
> > Thing is, this raises two related issues:  (a) there's the
> > analagous input-only case where MOSI isn't used, e.g. for
> > some kinds of sensor; and (b) there's also "real 3-wire SPI"
> > (spi->mode & SPI_3WIRE) where interactions are limited to
> > half duplex and one pin switches roles between MOSI and MISO.
> > 
> > Clearly this "output-only" case is a subset of SPI_3WIRE (the
> > MOMI/SISO pin can't switch direction) so one more change I
> > want to see is requiring that spi->mode flag be set in all
> > SPI devices registered when this mode is used.

Don't forget that change ...

 
> > If you have time, it would be good to generalize this patch
> > to cover all of those 3-wire modes ... accept all half
> > duplex calls, use gpio_direction_*() to switch direction.
> >
> > If not -- e.g. nothing to test that with! -- then I can
> > understand, but please make your changes with that model
> > in mind, and leave an appropriate FIXME in place.
> 
> I'm sorry, but I have only a hw board with missing MISO pin so I won't
> be able to properly check/debug any other configurations.

OK, then just stick to what you can test, and comment the issue.

[PATCH] [drivers] [SPI] SPI_GPIO: add support for controllers with missing MISO pin

[Marek Szyprowski]

There are some boards that do not strictly follow SPI standard and use only 3 wires
(SCLK, MOSI, SS) for connecting some simple auxiliary chips and controls them with GPIO
based 'spi controller'. In this configuration the MISO line is missing (it is not
required if the chip does not transfer any data back to host). The example of such
board is a NCP ARM S3C64XX based machine. This patch adds support for such non-standard
configuration in GPIO-based SPI controller.

Reviewed-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

--- 

diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 26bd03e..16f74fd 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -114,7 +114,10 @@ static inline void setmosi(const struct spi_device *spi, int is_on)
 
 static inline int getmiso(const struct spi_device *spi)
 {
-	return !!gpio_get_value(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		return !!gpio_get_value(SPI_MISO_GPIO);
+	else
+		return 0;
 }
 
 #undef pdata
@@ -243,9 +246,11 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	if (value)
 		goto done;
 
-	value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
-	if (value)
-		goto free_mosi;
+	if (gpio_is_valid(SPI_MISO_GPIO)) {
+		value = spi_gpio_alloc(SPI_MISO_GPIO, label, true);
+		if (value)
+			goto free_mosi;
+	}
 
 	value = spi_gpio_alloc(SPI_SCK_GPIO, label, false);
 	if (value)
@@ -254,7 +259,8 @@ spi_gpio_request(struct spi_gpio_platform_data *pdata, const char *label)
 	goto done;
 
 free_miso:
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 free_mosi:
 	gpio_free(SPI_MOSI_GPIO);
 done:
@@ -308,7 +314,8 @@ static int __init spi_gpio_probe(struct platform_device *pdev)
 	if (status < 0) {
 		spi_master_put(spi_gpio->bitbang.master);
 gpio_free:
-		gpio_free(SPI_MISO_GPIO);
+		if (gpio_is_valid(SPI_MISO_GPIO))
+			gpio_free(SPI_MISO_GPIO);
 		gpio_free(SPI_MOSI_GPIO);
 		gpio_free(SPI_SCK_GPIO);
 		spi_master_put(master);
@@ -332,7 +339,8 @@ static int __exit spi_gpio_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	gpio_free(SPI_MISO_GPIO);
+	if (gpio_is_valid(SPI_MISO_GPIO))
+		gpio_free(SPI_MISO_GPIO);
 	gpio_free(SPI_MOSI_GPIO);
 	gpio_free(SPI_SCK_GPIO);



------------------------------------------------------------------------------
Are you an open source citizen? Join us for the Open Source Bridge conference!
Portland, OR, June 17-19. Two days of sessions, one day of unconference: $250.
Need another reason to go? 24-hour hacker lounge. Register today!
http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org