Patchwork spi_mpc8xxx.c: chip select polarity problem

login
register
mail settings
Submitter Torsten Fleischer
Date Dec. 9, 2009, 3:49 p.m.
Message ID <200912091649.19380.to-fleischer@t-online.de>
Download mbox | patch
Permalink /patch/40726/
State Superseded
Delegated to: Kumar Gala
Headers show

Comments

Torsten Fleischer - Dec. 9, 2009, 3:49 p.m.
On Thu, Nov 26, 2009 at 20:17:35,  Grant Likely wrote:
[...]
> spi-cs-high is definitely not a complete solution, but it isn't
> actively evil either.  Plus it is documented and (presumably) in
> active use. so support for it should not be dropped.
> 
> Regardless, there needs to be a library function for parsing all the
> SPI child nodes and returning the active state for each GPIO chip
> select.  All the code for parsing the old spi-cs-high properties can
> be contained in the same place as a new yet-to-be-defined bus node cs
> polarity property.  The rework to the driver itself is not ugly.
> 

The following patch adds a function to get the active state of the chip select
of a SPI device by looking for the 'spi-cs-high' property in the associated device
tree node.
This function is used by the spi_mpc8xxx driver to set a proper initial value
to the associated GPIOs.


Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
---
Grant Likely - Dec. 9, 2009, 5:46 p.m.
On Wed, Dec 9, 2009 at 8:49 AM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Thu, Nov 26, 2009 at 20:17:35,  Grant Likely wrote:
> [...]
>> spi-cs-high is definitely not a complete solution, but it isn't
>> actively evil either.  Plus it is documented and (presumably) in
>> active use. so support for it should not be dropped.
>>
>> Regardless, there needs to be a library function for parsing all the
>> SPI child nodes and returning the active state for each GPIO chip
>> select.  All the code for parsing the old spi-cs-high properties can
>> be contained in the same place as a new yet-to-be-defined bus node cs
>> polarity property.  The rework to the driver itself is not ugly.
>>
>
> The following patch adds a function to get the active state of the chip select
> of a SPI device by looking for the 'spi-cs-high' property in the associated device
> tree node.
> This function is used by the spi_mpc8xxx driver to set a proper initial value
> to the associated GPIOs.
>
>
> Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>

I like this better.  See comments below.

> ---
>
> diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c linux-2.6.32/drivers/of/of_spi.c
> --- linux-2.6.32_orig//drivers/of/of_spi.c      2009-12-03 04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/of/of_spi.c    2009-12-09 12:37:01.000000000 +0100
> @@ -10,6 +10,49 @@
>  #include <linux/device.h>
>  #include <linux/spi/spi.h>
>  #include <linux/of_spi.h>
> +#include <linux/errno.h>
> +
> +/**
> + * of_get_spi_cs_active_state - Gets the chip select's active state of a SPI
> + * child devices.
> + * @np:        parent node of the SPI device nodes
> + * @index:     index/address of the SPI device (refers to the 'reg' property)
> + * @cs_active: pointer to return the chip select's active state
> + *             (true = active high; false = active low)
> + *
> + * Returns 0 on success; negative errno on failure
> + */
> +int of_get_spi_cs_active_state(struct device_node *np, int index, bool *cs_active)
> +{
> +       struct device_node *child;
> +       const int *prop;
> +       int len;
> +       bool active = 0;
> +
> +       /* search for the matching SPI device */
> +       for_each_child_of_node(np, child) {
> +               prop = of_get_property(child, "reg", &len);
> +               if (!prop || len < sizeof(*prop)) {
> +                       /* property 'reg' not available (not an error) */
> +                       continue;
> +               }
> +
> +               if ( *prop == index ) {
> +                       /* matching device found */
> +                       if (of_find_property(child, "spi-cs-high", NULL))
> +                               active = 1;
> +
> +                       if (cs_active)
> +                               *cs_active = active;

A little odd.  If cs_active is NULL, then this routine does nothing,
and the caller is entirely defective.  Either test at the top of the
function or not at all.  Then *cs_active can be assigned directly.
But even then, this function will probably need to be reworked (see
comments below).

> +
> +                       return 0;
> +               }
> +       }
> +
> +       return -ENODEV;
> +}
> +EXPORT_SYMBOL(of_get_spi_cs_active_state);
> +
>
>  /**
>  * of_register_spi_devices - Register child devices onto the SPI bus
> diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.32/drivers/spi/spi_mpc8xxx.c
> --- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c        2009-12-03 04:51:21.000000000 +0100
> +++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c      2009-12-09 12:50:36.000000000 +0100
> @@ -705,6 +705,7 @@
>        for (; i < ngpios; i++) {
>                int gpio;
>                enum of_gpio_flags flags;
> +               bool astate;
>
>                gpio = of_get_gpio_flags(np, i, &flags);
>                if (!gpio_is_valid(gpio)) {
> @@ -721,8 +722,15 @@
>                pinfo->gpios[i] = gpio;
>                pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
>
> +               ret = of_get_spi_cs_active_state(np, i, &astate);
> +               if (ret) {
> +                       dev_err(dev, "can't get cs active state of device "
> +                               "#%d: %d\n", i, ret);
> +                       goto err_loop;
> +               }

This is a bit heavy handed in that it expects the device tree to be
fully populated with all SPI devices which isn't always a given.  For
example a board that has some unpopulated SPI devices could have some
gaps in the GPIO CS layout.  If a node can't be found, then just
ignore it silently and move on to the next.  I'd do something like
this:

+               astate = of_get_spi_cs_active_state(np, i);
               ret = gpio_direction_output(pinfo->gpios[i],
-                                           pinfo->alow_flags[i]);
+                                           pinfo->alow_flags[i] ^ !astate);

BTW, why the xor?  The usage is non-obvious enough that I'd like to
see a comment describing the use case.

g.
Torsten Fleischer - Dec. 9, 2009, 7:13 p.m.
On Wed, Dec 9, 2009 at 18:46:51 Grant Likely wrote:
[...]
> > +               ret = of_get_spi_cs_active_state(np, i, &astate);
> > +               if (ret) {
> > +                       dev_err(dev, "can't get cs active state of device
> > " +                               "#%d: %d\n", i, ret);
> > +                       goto err_loop;
> > +               }
> 
> This is a bit heavy handed in that it expects the device tree to be
> fully populated with all SPI devices which isn't always a given.  For
> example a board that has some unpopulated SPI devices could have some
> gaps in the GPIO CS layout.  If a node can't be found, then just
> ignore it silently and move on to the next.  I'd do something like
> this:
> 
> +               astate = of_get_spi_cs_active_state(np, i);

What should be returned if the node can't be found, 'true' or 'false? 

Maybe its better to do the following:

+               ret = of_get_spi_cs_active_state(np, i, &astate);
+               if (ret) {
+                       /* Device node not found */
+                       continue;
+               }

>                ret = gpio_direction_output(pinfo->gpios[i],
> -                                           pinfo->alow_flags[i]);
> +                                           pinfo->alow_flags[i] ^ !astate);
> 
> BTW, why the xor?  The usage is non-obvious enough that I'd like to
> see a comment describing the use case.
 
If I understand it right, the alow_flags describe the wiring. If set to 0 the 
wiring is non-inverted, if set to 1 its inverted respectively. To take this 
into account the active state has to be xor'd with the appropriate alow_flag.

Patch

diff -ruN linux-2.6.32_orig//drivers/of/of_spi.c linux-2.6.32/drivers/of/of_spi.c
--- linux-2.6.32_orig//drivers/of/of_spi.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/drivers/of/of_spi.c	2009-12-09 12:37:01.000000000 +0100
@@ -10,6 +10,49 @@ 
 #include <linux/device.h>
 #include <linux/spi/spi.h>
 #include <linux/of_spi.h>
+#include <linux/errno.h>
+
+/**
+ * of_get_spi_cs_active_state - Gets the chip select's active state of a SPI
+ * child devices.
+ * @np:        parent node of the SPI device nodes
+ * @index:     index/address of the SPI device (refers to the 'reg' property)
+ * @cs_active: pointer to return the chip select's active state
+ *             (true = active high; false = active low)
+ *
+ * Returns 0 on success; negative errno on failure
+ */
+int of_get_spi_cs_active_state(struct device_node *np, int index, bool *cs_active)
+{
+	struct device_node *child;
+	const int *prop;
+	int len;
+	bool active = 0;
+
+	/* search for the matching SPI device */
+	for_each_child_of_node(np, child) {
+		prop = of_get_property(child, "reg", &len);
+		if (!prop || len < sizeof(*prop)) {
+			/* property 'reg' not available (not an error) */
+			continue;
+		}
+
+		if ( *prop == index ) {
+			/* matching device found */
+			if (of_find_property(child, "spi-cs-high", NULL))
+				active = 1;
+
+			if (cs_active)
+				*cs_active = active;
+
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL(of_get_spi_cs_active_state);
+
 
 /**
  * of_register_spi_devices - Register child devices onto the SPI bus
diff -ruN linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.32/drivers/spi/spi_mpc8xxx.c
--- linux-2.6.32_orig//drivers/spi/spi_mpc8xxx.c	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/drivers/spi/spi_mpc8xxx.c	2009-12-09 12:50:36.000000000 +0100
@@ -705,6 +705,7 @@ 
 	for (; i < ngpios; i++) {
 		int gpio;
 		enum of_gpio_flags flags;
+		bool astate;
 
 		gpio = of_get_gpio_flags(np, i, &flags);
 		if (!gpio_is_valid(gpio)) {
@@ -721,8 +722,15 @@ 
 		pinfo->gpios[i] = gpio;
 		pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW;
 
+		ret = of_get_spi_cs_active_state(np, i, &astate);
+		if (ret) {
+			dev_err(dev, "can't get cs active state of device "
+				"#%d: %d\n", i, ret);
+			goto err_loop;
+		}
+
 		ret = gpio_direction_output(pinfo->gpios[i],
-					    pinfo->alow_flags[i]);
+					    pinfo->alow_flags[i] ^ !astate);
 		if (ret) {
 			dev_err(dev, "can't set output direction for gpio "
 				"#%d: %d\n", i, ret);
diff -ruN linux-2.6.32_orig//include/linux/of_spi.h linux-2.6.32/include/linux/of_spi.h
--- linux-2.6.32_orig//include/linux/of_spi.h	2009-12-03 04:51:21.000000000 +0100
+++ linux-2.6.32/include/linux/of_spi.h	2009-12-09 12:36:48.000000000 +0100
@@ -15,4 +15,7 @@ 
 extern void of_register_spi_devices(struct spi_master *master,
 				    struct device_node *np);
 
+extern int of_get_spi_cs_active_state(struct device_node *np,
+				      int index, bool *cs_active);
+
 #endif /* __LINUX_OF_SPI */