Re: [PATCH v1 1/1] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sun, Sep 26, 2021 at 7:09 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Sat, Sep 25, 2021 at 4:45 PM Joey Gouly <joey.gouly@xxxxxxx> wrote:
> > On Wed, Sep 22, 2021 at 10:20:39AM +0300, Andy Shevchenko wrote:

> > > > +   if (of_find_property(node, "interrupt-controller", NULL)) {
> > >
> > > Are you sure you need this check and OF core doesn't provide a generic way for this?
> > >
> > I don't think so, and pinctrl-equilibrium.c does something similar in
> > `gpiochip_setup`.
>
> Linus? Do we really need this?

I don't really see this as necessary, we don't need to check everything.
Not that it hurts either, so I would say maintainer preference?

> > > > +   if (of_parse_phandle_with_fixed_args(pdev->dev.of_node, "gpio-ranges",
> > > > +                   3, 0, &pinspec)) {
> > > > +           dev_err(&pdev->dev, "gpio-ranges property not found\n");
> > > > +           return -EINVAL;
> > > > +   }
> > > > +
> > > > +   pctl->npins = pinspec.args[2];
> > > > +   pin_base = pinspec.args[1];
> > >
> > >
> > > Isn't this being provided by pin control?
> >
> > Not that I am aware of. It is a similar pattern to other pinctrl drivers
> > like pinctrl-rza1.c and pinctrl-npcm7xx.c. The driver needs to get the
> > number of pins/base from the DT to setup the internal data structures.
>
> So, maybe you need to refactor the pin control core first and provide
> some stubs that will serve your purposes, but to me it sounds weird to
> have all these checks.
>
> Linus, what is your opinion / input here?

I don't remember right now how the review was going on the
mentioned drivers.

I did imagine that of_gpiochip_add_pin_range() would be the
sole parser of this, and drivers would then use the infrastructure
for any necessary cross-reference between the subsystems,
not second-code it.

What is it that you really need to do here?

I think npins should be known from the compatible (we know that
this version of the SoC has so and so many pins) and the base
should always be 0? It's not like we have several pin controllers
of this type in the SoC is it?

Yours,
Linus Walleij