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

[Joey Gouly <joey.gouly@arm.com>]

Hi,

On Sun, Sep 26, 2021 at 02:48:18PM +0200, Linus Walleij wrote:
> On Sun, Sep 26, 2021 at 7:09 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Sep 25, 2021 at 4:45 PM Joey Gouly <joey.gouly@arm.com> 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?
> 
I'm unsure if that means you (Linus), or Hector Martin as I put this
file under the "ARM/APPLE MACHINE SUPPORT" section in MAINTAINERS.

> > > > > +   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?
All we need is the number of GPIOs from the DT now.
I got a bit confused with the 'base' here, locally I have removed the
'pin_base' variable and usage. I was confusing it with the `gpio_chip.base` field,
however that seems to be about the internal GPIO numbering.

Thanks,
Joey