[v3,2/7] gpiolib: Add support for gpiochipN-based table lookup

Currently GPIO controllers can only be referred to by label in GPIO
lookup tables.

Add support for looking them up by "gpiochipN" name, with "N" either the
corresponding GPIO device's ID number, or the GPIO controller's first
GPIO number.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
If this is rejected, the GPIO Aggregator documentation must be updated.

The second variant is currently used by the legacy sysfs interface only,
so perhaps the chip->base check should be dropped?

v3:
  - New.
---
 drivers/gpio/gpiolib.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
> 
> Currently GPIO controllers can only be referred to by label in GPIO
> lookup tables.
> 
> Add support for looking them up by "gpiochipN" name, with "N" either the
> corresponding GPIO device's ID number, or the GPIO controller's first
> GPIO number.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> If this is rejected, the GPIO Aggregator documentation must be updated.
> 
> The second variant is currently used by the legacy sysfs interface only,
> so perhaps the chip->base check should be dropped?
> 
> v3:
>   - New.
> ---
>  drivers/gpio/gpiolib.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index c9e47620d2434983..d24a3d79dcfe69ad 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1746,9 +1746,29 @@ static int gpiochip_match_name(struct gpio_chip *chip, void *data)
>  	return !strcmp(chip->label, name);
>  }
>  
> +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> +{
> +	int id = (uintptr_t)data;
> +
> +	return id == chip->base || id == chip->gpiodev->id;
> +}
> +
>  static struct gpio_chip *find_chip_by_name(const char *name)
>  {
> -	return gpiochip_find((void *)name, gpiochip_match_name);
> +	struct gpio_chip *chip;
> +	int id;
> +
> +	chip = gpiochip_find((void *)name, gpiochip_match_name);
> +	if (chip)
> +		return chip;
> +
> +	if (!str_has_prefix(name, GPIOCHIP_NAME))
> +		return NULL;
> +
> +	if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
> +		return NULL;
> +
> +	return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
>  }
>  
>  #ifdef CONFIG_GPIOLIB_IRQCHIP
> -- 
> 2.17.1
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Hi Geert!

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Currently GPIO controllers can only be referred to by label in GPIO
> lookup tables.
>
> Add support for looking them up by "gpiochipN" name, with "N" either the
> corresponding GPIO device's ID number, or the GPIO controller's first
> GPIO number.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

What the commit message is missing is a rationale, why is this needed?

> If this is rejected, the GPIO Aggregator documentation must be updated.
>
> The second variant is currently used by the legacy sysfs interface only,
> so perhaps the chip->base check should be dropped?

Anything improving the sysfs is actively discouraged by me.
If it is just about staying compatible it is another thing.

> +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> +{
> +       int id = (uintptr_t)data;
> +
> +       return id == chip->base || id == chip->gpiodev->id;
> +}
>  static struct gpio_chip *find_chip_by_name(const char *name)
>  {
> -       return gpiochip_find((void *)name, gpiochip_match_name);
> +       struct gpio_chip *chip;
> +       int id;
> +
> +       chip = gpiochip_find((void *)name, gpiochip_match_name);
> +       if (chip)
> +               return chip;
> +
> +       if (!str_has_prefix(name, GPIOCHIP_NAME))
> +               return NULL;
> +
> +       if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
> +               return NULL;
> +
> +       return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);

Isn't it easier to just  augment the existing match function to
check like this:

static int gpiochip_match_name(struct gpio_chip *chip, void *data)
{
        const char *name = data;

        if (!strcmp(chip->label, name))
               return 0;
        return !strcmp(dev_name(&chip->gpiodev->dev), name);
}

We should I guess also add some kerneldoc to say we first
match on the label and second on dev_name().

Yours,
Linus Walleij

Hi Linus,

On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Currently GPIO controllers can only be referred to by label in GPIO
> > lookup tables.
> >
> > Add support for looking them up by "gpiochipN" name, with "N" either the
> > corresponding GPIO device's ID number, or the GPIO controller's first
> > GPIO number.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> What the commit message is missing is a rationale, why is this needed?

Right. To be added: so they can be looked up in the GPIO lookup table
using either the chip's label, or the "gpiochipN" name.

> > If this is rejected, the GPIO Aggregator documentation must be updated.
> >
> > The second variant is currently used by the legacy sysfs interface only,
> > so perhaps the chip->base check should be dropped?
>
> Anything improving the sysfs is actively discouraged by me.
> If it is just about staying compatible it is another thing.

OK, so N must be the corresponding GPIO device's ID number.

> > +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> > +{
> > +       int id = (uintptr_t)data;
> > +
> > +       return id == chip->base || id == chip->gpiodev->id;
> > +}
> >  static struct gpio_chip *find_chip_by_name(const char *name)
> >  {
> > -       return gpiochip_find((void *)name, gpiochip_match_name);
> > +       struct gpio_chip *chip;
> > +       int id;
> > +
> > +       chip = gpiochip_find((void *)name, gpiochip_match_name);
> > +       if (chip)
> > +               return chip;
> > +
> > +       if (!str_has_prefix(name, GPIOCHIP_NAME))
> > +               return NULL;
> > +
> > +       if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, &id))
> > +               return NULL;
> > +
> > +       return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
>
> Isn't it easier to just  augment the existing match function to
> check like this:
>
> static int gpiochip_match_name(struct gpio_chip *chip, void *data)
> {
>         const char *name = data;
>
>         if (!strcmp(chip->label, name))
>                return 0;

return true;

>         return !strcmp(dev_name(&chip->gpiodev->dev), name);
> }

Oh, didn't think of using dev_name() on the gpiodev.
Yes, with the chip->base check removed, the code can be simplified.

Or just

        return !strcmp(chip->label, name) ||
               !strcmp(dev_name(&chip->gpiodev->dev), name);

> We should I guess also add some kerneldoc to say we first
> match on the label and second on dev_name().

OK.

Gr{oetje,eeting}s,

                        Geert

On Thu, Dec 12, 2019 at 2:33 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > Currently GPIO controllers can only be referred to by label in GPIO
> > > lookup tables.
> > >
> > > Add support for looking them up by "gpiochipN" name, with "N" either the
> > > corresponding GPIO device's ID number, or the GPIO controller's first
> > > GPIO number.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > What the commit message is missing is a rationale, why is this needed?
>
> Right. To be added: so they can be looked up in the GPIO lookup table
> using either the chip's label, or the "gpiochipN" name.

After reading the aggregator/forwarder driver I am not convinced
that this is needed at all and I think this patch can be dropped,
but check my review and see what you think!

Thanks,
Linus Walleij