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

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 <redacted>
---
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

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 <redacted>
---
Changes :
	patch against 2.6.30-rc8-mm1

Anton,

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

- k

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 <redacted>
---
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 "

Please put an empty line after variable declaration.


Thanks!

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

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:

quoted

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

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 <redacted>
---
Changes :
	patch against 2.6.30-rc8-mm1
	style updates
	compiler warning fix
comment :
	This feature is needed on our home made board to program an onboard fpga.
	The fpga needs some special pin toggling to put it in programming mode.
	That's why we want to skip the usual gpio cs and do the pin toggling
	in our driver
	

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index 0fd0ec4..3a367ce 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c

@@ -666,9 +666,12 @@ static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
  	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 +710,31 @@ static int of_mpc8xxx_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;

On Jun 19, 2009, at 2:26 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 <redacted>
---
Changes :
	patch against 2.6.30-rc8-mm1
	style updates
	compiler warning fix
comment :
	This feature is needed on our home made board to program an onboard  
fpga.
	The fpga needs some special pin toggling to put it in programming  
mode.
	That's why we want to skip the usual gpio cs and do the pin toggling
	in our driver
	

Which FSL chip are you using?

- k

On Jun 19, 2009, at 2:26 AM, Rini van Zetten wrote:

quoted

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 <redacted>
---
Changes :
    patch against 2.6.30-rc8-mm1
    style updates
    compiler warning fix
comment :
    This feature is needed on our home made board to program an 
onboard fpga.
    The fpga needs some special pin toggling to put it in programming 
mode.
    That's why we want to skip the usual gpio cs and do the pin toggling
    in our driver
    

Which FSL chip are you using?

We use the MPC8377E

Rini

- k

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

-- 
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

Hello,

On Thu, Jun 18, 2009 at 4:04 PM, Kumar Gala[off-list ref] wrot=
e:

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

quoted

On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:

quoted

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

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[off-list ref] wrote:

quoted

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

quoted

On Thu, Jun 18, 2009 at 08:19:44AM +0200, Rini van Zetten wrote:

quoted

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

On Fri, Jun 19, 2009 at 09:26:08AM +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 */

Signed-off-by: Rini van Zetten <redacted>
---
Changes :
	patch against 2.6.30-rc8-mm1
	style updates
	compiler warning fix
comment :
	This feature is needed on our home made board to program an onboard fpga.
	The fpga needs some special pin toggling to put it in programming mode.
	That's why we want to skip the usual gpio cs and do the pin toggling
	in our driver

Cool, thanks for the explanation. You may want to put it
into the commit message.

[...]

+			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);

There is no space between 'for' and 'gpio' words.
(also the whole dev_err() may fit into two lines, no need to
split it into three lines).

Otherwise looks great. On the next resend feel free to add my
Acked-by: Anton Vorontsov <redacted>

Thanks,

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

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 */

This feature is needed on our home made board to program an onboard fpga.
The fpga needs some special pin toggling to put it in programming mode.
That's why we want to skip the usual gpio cs and do the pin toggling
in our driver

Signed-off-by: Rini van Zetten <redacted>
Acked-by: Anton Vorontsov <redacted>
---
Changes :
     patch against 2.6.30-rc8-mm1
     style updates
     compiler warning fix
v4 :
     missing space in error text added
     splitted lines combined

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index 0fd0ec4..de95790 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c

@@ -666,9 +666,12 @@ static void mpc8xxx_spi_cs_control(struct spi_device *spi, bool on)
  	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)

@@ -678,7 +681,7 @@ static int of_mpc8xxx_spi_get_chipselects(struct device *dev)
  	struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(pdata);
  	unsigned int ngpios;
  	int i = 0;
-	int ret;
+	int ret = 0;

  	ngpios = of_gpio_count(np);
  	if (!ngpios) {

@@ -707,27 +710,30 @@ static int of_mpc8xxx_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;

On Fri, Jun 19, 2009 at 7:25 AM, Anton
Vorontsov[off-list ref] 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.

On Friday 19 June 2009, Rini van Zetten wrote:

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

Note that there's now the SPI_NO_CS bit in spi_device.mode
to describe this situation ... so no "-EEXIST" hackery should
ever tempt anyone again.