[PATCH][POWERPC] mpc83xx : allow SPI without cs.

[PATCH][POWERPC] mpc83xx : allow SPI without cs.

[Rini van Zetten]

This patch adds the possibility to have a spi device without a cs.

For example, the dts file should look something like this:

spi-controller {
        gpios = <&pio1 1 0      /* cs0 */
                 0              /* cs1, no GPIO */
                 &pio2 2 0>;    /* cs2 */



Signed-off-by: Rini van Zetten <rini@arvoo.nl>
---
  drivers/spi/spi_mpc83xx.c |   44 +++++++++++++++++++++++---------------------
  1 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/spi/spi_mpc83xx.c b/drivers/spi/spi_mpc83xx.c
index f4573a9..d06027e 100644
--- a/drivers/spi/spi_mpc83xx.c
+++ b/drivers/spi/spi_mpc83xx.c
@@ -687,9 +687,10 @@ static void mpc83xx_spi_cs_control(struct spi_device *spi, bool on)
  	struct mpc83xx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
  	u16 cs = spi->chip_select;
  	int gpio = pinfo->gpios[cs];
-	bool alow = pinfo->alow_flags[cs];
-
-	gpio_set_value(gpio, on ^ alow);
+	if ( gpio != -EEXIST ) {
+		bool alow = pinfo->alow_flags[cs];
+		gpio_set_value(gpio, on ^ alow);
+	}
  }

  static int of_mpc83xx_spi_get_chipselects(struct device *dev)
@@ -728,27 +729,28 @@ static int of_mpc83xx_spi_get_chipselects(struct device *dev)
  		enum of_gpio_flags flags;

  		gpio = of_get_gpio_flags(np, i, &flags);
-		if (!gpio_is_valid(gpio)) {
+		if (gpio_is_valid(gpio)) {
+			ret = gpio_request(gpio, dev_name(dev));
+			if (ret) {
+				dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
+				goto err_loop;
+			}
+			pinfo->gpios[i] = gpio;
+			pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
+
+			ret = gpio_direction_output(pinfo->gpios[i],
+					pinfo->alow_flags[i]);
+			if (ret) {
+				dev_err(dev, "can't set output direction for gpio "
+						"#%d: %d\n", i, ret);
+				goto err_loop;
+			}
+		} else if (gpio == -EEXIST) {
+			pinfo->gpios[i] = -EEXIST;
+		} else {
  			dev_err(dev, "invalid gpio #%d: %d\n", i, gpio);
  			goto err_loop;
  		}
-
-		ret = gpio_request(gpio, dev_name(dev));
-		if (ret) {
-			dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
-			goto err_loop;
-		}
-
-		pinfo->gpios[i] = gpio;
-		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
-		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
-		if (ret) {
-			dev_err(dev, "can't set output direction for gpio "
-				"#%d: %d\n", i, ret);
-			goto err_loop;
-		}
  	}

  	pdata->max_chipselect = ngpios;
-- 

Re: [PATCH][POWERPC] mpc83xx : allow SPI without cs.

[Kumar Gala]

On Jun 11, 2009, at 4:10 AM, Rini van Zetten wrote:

> This patch adds the possibility to have a spi device without a cs.
>
> For example, the dts file should look something like this:
>
> spi-controller {
>       gpios = <&pio1 1 0      /* cs0 */
>                0              /* cs1, no GPIO */
>                &pio2 2 0>;    /* cs2 */
>
>
>
> Signed-off-by: Rini van Zetten <rini@arvoo.nl>
> ---
> drivers/spi/spi_mpc83xx.c |   44 ++++++++++++++++++++++ 
> +---------------------
> 1 files changed, 23 insertions(+), 21 deletions(-)

This patch needs to be respun against the -mm tree as a lot of other  
spi patches are queued up there that will cause this patch to not  
merge correctly.

- k

Re: [PATCH][POWERPC] mpc83xx : allow SPI without cs.

[Rini van Zetten]

excuse me for my ignorance, but which -mm tree do you mean and where can I find it.

Regards,
Rini

Kumar Gala schreef:
> 
> On Jun 11, 2009, at 4:10 AM, Rini van Zetten wrote:
> 
>> This patch adds the possibility to have a spi device without a cs.
>>
>> For example, the dts file should look something like this:
>>
>> spi-controller {
>>       gpios = <&pio1 1 0      /* cs0 */
>>                0              /* cs1, no GPIO */
>>                &pio2 2 0>;    /* cs2 */
>>
>>
>>
>> Signed-off-by: Rini van Zetten <rini@arvoo.nl>
>> ---
>> drivers/spi/spi_mpc83xx.c |   44 
>> +++++++++++++++++++++++---------------------
>> 1 files changed, 23 insertions(+), 21 deletions(-)
> 
> This patch needs to be respun against the -mm tree as a lot of other spi 
> patches are queued up there that will cause this patch to not merge 
> correctly.
> 
> - k

-- 
Rini van Zetten
Senior Software Engineer

-------------------------
ARVOO Engineering B.V.
Tasveld 13
3417 XS Montfoort
The Netherlands

E-mail : <mailto:rini@arvoo.com> Rini van Zetten

Web : www.arvoo.com

[PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Rini van Zetten]

This patch adds the possibility to have a spi device without a cs.

For example, the dts file should look something like this:

spi-controller {
        gpios = <&pio1 1 0      /* cs0 */
                 0              /* cs1, no GPIO */
                 &pio2 2 0>;    /* cs2 */



Signed-off-by: Rini van Zetten <rini@arvoo.nl>
---
Changes :
	patch against 2.6.30-rc8-mm1

--- drivers/spi/spi_mpc8xxx.c.org	2009-06-12 10:45:21.000000000 +0200
+++ drivers/spi/spi_mpc8xxx.c	2009-06-12 10:54:48.000000000 +0200
@@ -666,9 +666,10 @@ static void mpc8xxx_spi_cs_control(struc
  	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
  	u16 cs = spi->chip_select;
  	int gpio = pinfo->gpios[cs];
-	bool alow = pinfo->alow_flags[cs];
-
-	gpio_set_value(gpio, on ^ alow);
+	if (gpio != -EEXIST) {
+		bool alow = pinfo->alow_flags[cs];
+		gpio_set_value(gpio, on ^ alow);
+	}
  }

  static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
@@ -707,27 +708,29 @@ static int of_mpc8xxx_spi_get_chipselect
  		enum of_gpio_flags flags;

  		gpio = of_get_gpio_flags(np, i, &flags);
-		if (!gpio_is_valid(gpio)) {
+		if (gpio_is_valid(gpio)) {
+			ret = gpio_request(gpio, dev_name(dev));
+			if (ret) {
+				dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
+				goto err_loop;
+			}
+
+			pinfo->gpios[i] = gpio;
+			pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
+
+			ret = gpio_direction_output(pinfo->gpios[i],
+					pinfo->alow_flags[i]);
+			if (ret) {
+				dev_err(dev, "can't set output direction for gpio "
+						"#%d: %d\n", i, ret);
+				goto err_loop;
+			}
+		} else if (gpio == -EEXIST) {
+			pinfo->gpios[i] = -EEXIST;
+		} else {
  			dev_err(dev, "invalid gpio #%d: %d\n", i, gpio);
  			goto err_loop;
  		}
-
-		ret = gpio_request(gpio, dev_name(dev));
-		if (ret) {
-			dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
-			goto err_loop;
-		}
-
-		pinfo->gpios[i] = gpio;
-		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
-		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
-		if (ret) {
-			dev_err(dev, "can't set output direction for gpio "
-				"#%d: %d\n", i, ret);
-			goto err_loop;
-		}
  	}

  	pdata->max_chipselect = ngpios;
--

[PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Rini van Zetten]

This patch adds the possibility to have a spi device without a cs.

For example, the dts file should look something like this:

spi-controller {
        gpios = <&pio1 1 0      /* cs0 */
                 0              /* cs1, no GPIO */
                 &pio2 2 0>;    /* cs2 */



Signed-off-by: Rini van Zetten <rini@arvoo.nl>
---
Changes :
	patch against 2.6.30-rc8-mm1

--- drivers/spi/spi_mpc8xxx.c.org	2009-06-12 10:45:21.000000000 +0200
+++ drivers/spi/spi_mpc8xxx.c	2009-06-12 10:54:48.000000000 +0200
@@ -666,9 +666,10 @@ static void mpc8xxx_spi_cs_control(struc
  	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
  	u16 cs = spi->chip_select;
  	int gpio = pinfo->gpios[cs];
-	bool alow = pinfo->alow_flags[cs];
-
-	gpio_set_value(gpio, on ^ alow);
+	if (gpio != -EEXIST) {
+		bool alow = pinfo->alow_flags[cs];
+		gpio_set_value(gpio, on ^ alow);
+	}
  }

  static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
@@ -707,27 +708,29 @@ static int of_mpc8xxx_spi_get_chipselect
  		enum of_gpio_flags flags;

  		gpio = of_get_gpio_flags(np, i, &flags);
-		if (!gpio_is_valid(gpio)) {
+		if (gpio_is_valid(gpio)) {
+			ret = gpio_request(gpio, dev_name(dev));
+			if (ret) {
+				dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
+				goto err_loop;
+			}
+
+			pinfo->gpios[i] = gpio;
+			pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
+
+			ret = gpio_direction_output(pinfo->gpios[i],
+					pinfo->alow_flags[i]);
+			if (ret) {
+				dev_err(dev, "can't set output direction for gpio "
+						"#%d: %d\n", i, ret);
+				goto err_loop;
+			}
+		} else if (gpio == -EEXIST) {
+			pinfo->gpios[i] = -EEXIST;
+		} else {
  			dev_err(dev, "invalid gpio #%d: %d\n", i, gpio);
  			goto err_loop;
  		}
-
-		ret = gpio_request(gpio, dev_name(dev));
-		if (ret) {
-			dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
-			goto err_loop;
-		}
-
-		pinfo->gpios[i] = gpio;
-		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
-
-		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
-		if (ret) {
-			dev_err(dev, "can't set output direction for gpio "
-				"#%d: %d\n", i, ret);
-			goto err_loop;
-		}
  	}

  	pdata->max_chipselect = ngpios;
--


-- 
Rini van Zetten
Senior Software Engineer

-------------------------
ARVOO Engineering B.V.
Tasveld 13
3417 XS Montfoort
The Netherlands

E-mail : <mailto:rini@arvoo.com> Rini van Zetten

Web : www.arvoo.com

Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Kumar Gala]

On Jun 18, 2009, at 1:19 AM, Rini van Zetten wrote:

> This patch adds the possibility to have a spi device without a cs.
>
> For example, the dts file should look something like this:
>
> spi-controller {
>       gpios = <&pio1 1 0      /* cs0 */
>                0              /* cs1, no GPIO */
>                &pio2 2 0>;    /* cs2 */
>
>
>
> Signed-off-by: Rini van Zetten <rini@arvoo.nl>
> ---
> Changes :
> 	patch against 2.6.30-rc8-mm1

Anton,

Can you review and ack this if you are ok with it.

- k

>
>
> --- drivers/spi/spi_mpc8xxx.c.org	2009-06-12 10:45:21.000000000 +0200
> +++ drivers/spi/spi_mpc8xxx.c	2009-06-12 10:54:48.000000000 +0200
> @@ -666,9 +666,10 @@ static void mpc8xxx_spi_cs_control(struc
> 	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev- 
> >platform_data);
> 	u16 cs = spi->chip_select;
> 	int gpio = pinfo->gpios[cs];
> -	bool alow = pinfo->alow_flags[cs];
> -
> -	gpio_set_value(gpio, on ^ alow);
> +	if (gpio != -EEXIST) {
> +		bool alow = pinfo->alow_flags[cs];
> +		gpio_set_value(gpio, on ^ alow);
> +	}
> }
>
> static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
> @@ -707,27 +708,29 @@ static int of_mpc8xxx_spi_get_chipselect
> 		enum of_gpio_flags flags;
>
> 		gpio = of_get_gpio_flags(np, i, &flags);
> -		if (!gpio_is_valid(gpio)) {
> +		if (gpio_is_valid(gpio)) {
> +			ret = gpio_request(gpio, dev_name(dev));
> +			if (ret) {
> +				dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
> +				goto err_loop;
> +			}
> +
> +			pinfo->gpios[i] = gpio;
> +			pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
> +
> +			ret = gpio_direction_output(pinfo->gpios[i],
> +					pinfo->alow_flags[i]);
> +			if (ret) {
> +				dev_err(dev, "can't set output direction for gpio "
> +						"#%d: %d\n", i, ret);
> +				goto err_loop;
> +			}
> +		} else if (gpio == -EEXIST) {
> +			pinfo->gpios[i] = -EEXIST;
> +		} else {
> 			dev_err(dev, "invalid gpio #%d: %d\n", i, gpio);
> 			goto err_loop;
> 		}
> -
> -		ret = gpio_request(gpio, dev_name(dev));
> -		if (ret) {
> -			dev_err(dev, "can't request gpio #%d: %d\n", i, ret);
> -			goto err_loop;
> -		}
> -
> -		pinfo->gpios[i] = gpio;
> -		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
> -
> -		ret = gpio_direction_output(pinfo->gpios[i],
> -					    pinfo->alow_flags[i]);
> -		if (ret) {
> -			dev_err(dev, "can't set output direction for gpio "
> -				"#%d: %d\n", i, ret);
> -			goto err_loop;
> -		}
> 	}
>
> 	pdata->max_chipselect = ngpios;
> --
>
>
> -- 
> Rini van Zetten
> Senior Software Engineer
>
> -------------------------
> ARVOO Engineering B.V.
> Tasveld 13
> 3417 XS Montfoort
> The Netherlands
>
> E-mail : <mailto:rini@arvoo.com> Rini van Zetten
>
> Web : www.arvoo.com
>
>

Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Anton Vorontsov]

On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:
> This patch adds the possibility to have a spi device without a cs.
>
> For example, the dts file should look something like this:
>
> spi-controller {
>        gpios = <&pio1 1 0      /* cs0 */
>                 0              /* cs1, no GPIO */
>                 &pio2 2 0>;    /* cs2 */
>

Interesting scheme. I guess this is for eSPI controllers that can
do their own chip-selects, but we want GPIO chip selects in addition
(or in place of built-in ones), correct?

> Signed-off-by: Rini van Zetten <rini@arvoo.nl>
> ---
> Changes :
> 	patch against 2.6.30-rc8-mm1

I assume this is v2 already, and I overlooked v1, sorry.

Technically the patch looks OK, but please fix some cosmetics issues.

checkpatch reports:

WARNING: patch prefix 'drivers' exists, appears to be a -p0 patch

WARNING: line over 80 characters
#131: FILE: spi/spi_mpc8xxx.c:714:
+                               dev_err(dev, "can't request gpio #%d: %d\n", i, ret);

WARNING: line over 80 characters
#141: FILE: spi/spi_mpc8xxx.c:724:
+                               dev_err(dev, "can't set output direction for gpio "

> --- drivers/spi/spi_mpc8xxx.c.org	2009-06-12 10:45:21.000000000 +0200
> +++ drivers/spi/spi_mpc8xxx.c	2009-06-12 10:54:48.000000000 +0200
> @@ -666,9 +666,10 @@ static void mpc8xxx_spi_cs_control(struc
>  	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data);
>  	u16 cs = spi->chip_select;
>  	int gpio = pinfo->gpios[cs];
> -	bool alow = pinfo->alow_flags[cs];
> -
> -	gpio_set_value(gpio, on ^ alow);
> +	if (gpio != -EEXIST) {
> +		bool alow = pinfo->alow_flags[cs];
> +		gpio_set_value(gpio, on ^ alow);

Please put an empty line after variable declaration.


Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Kumar Gala]

On Jun 18, 2009, at 8:09 AM, Anton Vorontsov wrote:

> On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:
>> This patch adds the possibility to have a spi device without a cs.
>>
>> For example, the dts file should look something like this:
>>
>> spi-controller {
>>       gpios = <&pio1 1 0      /* cs0 */
>>                0              /* cs1, no GPIO */
>>                &pio2 2 0>;    /* cs2 */
>>
>
> Interesting scheme. I guess this is for eSPI controllers that can
> do their own chip-selects, but we want GPIO chip selects in addition
> (or in place of built-in ones), correct?

That is a good question.  What HW is this for (I don't think its for  
eSPI but I could be wrong).

- k

Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Leon Woestenberg]

Hello,

On Thu, Jun 18, 2009 at 4:04 PM, Kumar Gala<galak@kernel.crashing.org> wrot=
e:
> On Jun 18, 2009, at 8:09 AM, Anton Vorontsov wrote:
>> On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:
>>>
>>> This patch adds the possibility to have a spi device without a cs.
>>>
> That is a good question. =A0What HW is this for (I don't think its for eS=
PI
> but I could be wrong).
>
We need SPI without CS too on our MPC8313E design.

In our case having a CS line to each slave (18 of them) is too
expensive and we use a chip select which piggy backs on another
signal.
Once the slave is selected, SPI is used to send large amounts of data.

Regards,
--=20
Leon

Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Anton Vorontsov]

On Fri, Jun 19, 2009 at 01:45:46PM +0200, Leon Woestenberg wrote:
> Hello,
> 
> On Thu, Jun 18, 2009 at 4:04 PM, Kumar Gala<galak@kernel.crashing.org> wrote:
> > On Jun 18, 2009, at 8:09 AM, Anton Vorontsov wrote:
> >> On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:
> >>>
> >>> This patch adds the possibility to have a spi device without a cs.
> >>>
> > That is a good question.  What HW is this for (I don't think its for eSPI
> > but I could be wrong).
> >
> We need SPI without CS too on our MPC8313E design.

We do support SPI without CS, but only for one slave device. If
there is no CS, technically you can address only one device on a
SPI bus.

And apparently Rini's case is not for addressing multiple chips
w/o CS lines, it's just some chip-selects need quirks.

> In our case having a CS line to each slave (18 of them) is too
> expensive and we use a chip select which piggy backs on another
> signal.
> Once the slave is selected, SPI is used to send large amounts of data.

Can you describe it a little bit more? Have you patched the SPI
driver to work with your setup?

If you can address 18 slaves w/o chip-select line, I quess
you have a CS demuxing bridge somewhere, i.e.

spi-controller {
	/*
	 * no chip-selects from the controller, it can only talk to
	 * one device: the cs demuxing bridge, it is always selected.
	 */
	spi-cs-demuxing-bridge {
		/*
		 * knows how to manage chip-selects (or demux
		 * one chip select line for several slaves)
		 */
		spi-slave {
			reg = <0>;
		};
		...
		spi-slave {
			reg = <17>;
		};
	};
};

Surely we can hide the bridge into the SPI controller driver,
but I think it would be beneficial to "factor-out" it to a
stand-alone entity, so that other SPI controllers could work
with this setup without any modifications (oh and btw, we could
also create a bridge called "gpio-chipselects-bridge", and
factor out all GPIO stuff from the drivers).

There are two options of how we can factor out chip-select
machines... the bad and the ugly. :-) No, the first one is bad,
but the second should be OK.

1. We can create full-fledged SPI master bridge drivers, the
   drivers will just pass-through all SPI IO, but will manage
   chip-selects. The bad part is that these drivers could degrade
   performance since they'll have to manage SPI IO, which is
   unneeded for the pure CS machines.

2. Another option (which I like more), is to create "SPI chip-
   select machine driver framework", so we could (finally)
   separate two SPI entities: SPI IO and SPI chip-select machine.
   CS machines of different flavours: GPIO, built-in, and
   board-specific (!) with a lot of demuxing magic.

(1) can be implemented trivially, while (2) is a lot of work.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH -mm][POWERPC] mpc8xxx : allow SPI without cs.

[Grant Likely]

On Fri, Jun 19, 2009 at 7:25 AM, Anton
Vorontsov<avorontsov@ru.mvista.com> wrote:
> Surely we can hide the bridge into the SPI controller driver,
> but I think it would be beneficial to "factor-out" it to a
> stand-alone entity, so that other SPI controllers could work
> with this setup without any modifications (oh and btw, we could
> also create a bridge called "gpio-chipselects-bridge", and
> factor out all GPIO stuff from the drivers).
>
> There are two options of how we can factor out chip-select
> machines... the bad and the ugly. :-) No, the first one is bad,
> but the second should be OK.
[...]
> 2. Another option (which I like more), is to create "SPI chip-
> =A0 select machine driver framework", so we could (finally)
> =A0 separate two SPI entities: SPI IO and SPI chip-select machine.
> =A0 CS machines of different flavours: GPIO, built-in, and
> =A0 board-specific (!) with a lot of demuxing magic.

I agree, I think your option 2 is the way to go.  It would probably
need to be implemented as some form of logical bridge so that SPI
requests don't go straight to the IO driver, but first go through the
CS machine so that the CS driver can do funky stuff like inserting or
modifying SPI requests before they go to the hardware.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.