spi_mpc8xxx.c: chip select polarity problem

[Grant Likely]

On Wed, Dec 9, 2009 at 8:49 AM, Torsten Fleischer
<to-fleischer at 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 at 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.

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