On Wed, Dec 9, 2009 at 8:49 AM, Torsten Fleischer
<to-fleisc...@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-fleisc...@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.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
