Re: [PATCH v2 2/2] gpiolib: of: Handle threecell GPIO chips

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Feb 25, 2025 at 5:58 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> On Tue, Feb 25, 2025 at 10:15 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> >
> > When describing GPIO controllers in the device tree, the ambition
> > of device tree to describe the hardware may require a three-cell
> > scheme:
> >
> > gpios = <&gpio instance offset flags>;
>
> I think we have cases where bank/instance and offset are in 1 cell.
> Not sure if you want to make it possible to split those into separate
> gpio chips.

Hm maybe with your suggested changes that can be done.

> >         /*
> >          * We're discouraging gpio_cells < 2, since that way you'll have to
>
> Note that this function only rejects <2, but I doubt 3 cells worked
> for it. So it should probably check for !=2.

I fixed it, will send v3 with these changes.

> > +       /*
> > +        * Check chip instance number, the driver responds with true if
> > +        * this is the chip we are looking for.
> > +        */
> > +       if (!gc->of_node_instance_match(gc, gpiospec->args[0]))
>
> I would just pass gpiospec here. Then it can look at anything it wants
> in the args.
>
> Taking it a step further, if you made the function return the GPIO
> line number, you could combine the 2 translate functions. You'd need a
> default of_node_instance_match() to return args[0] for the 2 cell
> case.
>
> > +               return -EINVAL;
> > +
> > +       if (gpiospec->args[1] >= gc->ngpio)
> > +               return -EINVAL;
> > +
> > +       if (flags)
> > +               *flags = gpiospec->args[2];
>
> With the above, this would work for both cases:
>
> gpiospec->args[gpiospec->args_count - 1]
>
>
> Just an idea. Keep it as-is if you prefer.

I prefer to merge this and the Spacemit driver because we have already
asked the contributor to rewrite everything so many times.

But these are all internal interfaces, so what about I do the above
refactorings as soon as we have this pile merged? Because I can
see how that makes more transitions to helper libraries possible
also for other GPIO drivers.

Yours,
Linus Walleij