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

[Markus Pargmann <mpa@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3857 bytes --]

On Tuesday, February 23, 2016 02:36:42 PM Linus Walleij wrote:
> On Tue, Feb 23, 2016 at 8:54 AM, Markus Pargmann <mpa@pengutronix.de> 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@pengutronix.de>
> 
> 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?

Yes, that sounds good.

Thanks,

Markus

> 
> > +/**
> > + * 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
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]