[-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 <rini@arvoo.nl>
---
Changes :
	patch against 2.6.30-rc8-mm1

--

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

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!

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

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.

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,

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.

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

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.