Re: [PATCH 4/4] gpio: Add parsing of DT GPIO line-names

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Feb 23, 2016 at 8:54 AM, Markus Pargmann <mpa@xxxxxxxxxxxxxx> wrote:

> This patch reuses the DT bindings that are already in place for the
> gpio-hogging mechanism. These bindings define line-name properties for
> GPIOs inside the gpio-chip device node.
>
> of_parse_own_gpio() now sets the gpio descriptor name using the newly
> introduced gpiod_set_name(). It checks for name collisions within a GPIO
> chip to avoid GPIOs with the same name that are exported over the same
> GPIO character device.
>
> The GPIO flags that describe the GPIO state are not required anymore in
> general but are checked if the gpio-hog property was found.
>
> This can be used to use the line names from the schematic. Example of lsgpio on
> a modified i.MX6s Riotboard:
>
>         GPIO chip: gpiochip0, "209c000.gpio", 32 GPIO lines
>                 line 0: unnamed unlabeled
>                 line 1: unnamed unlabeled
>                 line 2: SD2_WP "wp" [kernel output open-drain]
>                 line 3: GPIO_3_CLK unlabeled
>                 line 4: SD2_CD "cd" [kernel output open-drain]
>                 ...
>
> The modified DT:
>          &gpio1 {
>                 sd2_wp {
>                         gpios = <2 0>;
>                         line-name = "SD2_WP";
>                 };
>
>                 gpio_3_clk {
>                         gpios = <3 0>;
>                         line-name = "GPIO_3_CLK";
>                 };
>
>                 sd2_cd {
>                         gpios = <4 0>;
>                         line-name = "SD2_CD";
>                 };
>         };
>
> Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>

NICE! And this is what I want too.

We need to remove some rough edges:

> +static struct gpio_desc *gpiodev_find_gpiod_by_name(struct gpio_device *gdev,
> +                                                   const char *name)
> +{
> +       int i;
> +
> +       for (i = 0; i != gdev->ngpio; ++i) {
> +               struct gpio_desc *desc = &gdev->descs[i];
> +
> +               if (desc->name && !strcmp(desc->name, name))
> +                       return desc;
> +       }
> +
> +       return NULL;
> +}

We already have gpio_name_to_desc() which does something
similar but across all chips.

Can we break out one gpiodev_name_to_desc() like
yours, and refactor gpio_name_to_desc() to just call
that for each chip?

> +/**
> + * gpid_set_name() - sets the name of a gpio descriptor

Missing "o" in gpid_

> + * @desc: the gpio descriptor
> + * @name: the name pointer that is assigned. It is internally not copied.
> + *
> + * This function sets a new name for the GPIO. It checks for collisions with
> + * other GPIOs with the same name within the gpio chip. It returns 0 on success
> + * or -EEXIST if the name is already used within the GPIO chip.
> + */
> +int gpiod_set_name(struct gpio_desc *desc, const char *name)
> +{
> +       struct gpio_desc *coll = gpiodev_find_gpiod_by_name(desc->gdev, name);
> +
> +       if (coll)
> +               return -EEXIST;
> +
> +       desc->name = name;
> +
> +       return 0;
> +}

Otherwise I'm OK with this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html