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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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:

...

> > > +F: drivers/pinctrl/pinctrl-apple-gpio.c
> >
> > Are you sure it's a good naming? Have you guaranteed that next Apple silicons
> > will use the same / compatible IP?
> We don't know what will be in future Apple SoCs, however this same GPIO
> HW has been in iPhones dating back to at least the iPhone 7 (2016). If
> there are minor changes we can add a new compatible, and if there is new
> HW in the future we can introduce a new file for it.

Do we have a chip name? For example, for M1 it's T8101 or something
like that. I would use a chip name/family rather than a broad brand
name.

...

> > > +   prev = readl(ppin);
> > > +   cfg = (prev & ~clr) | set;
> > > +
> > > +   if(!(prev & REG_GPIOx_CFG_DONE))
> > > +           writel(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> > > +   writel(cfg, ppin);
> >
> > Is it IP requirement to have sequential writes? Can it be done in one?
> We unfortunately don't have the documentation for this HW, so this behaviour is
> based on observing what macOS does.

So, then there are following remarks/questions:
1/ Have you tested when it does a single write?
2/ If it doesn't work, this piece deserves a good comment.

...

> > > +   if(!of_find_property(node, "gpio-controller", NULL)) {
> > > +           dev_err(pctl->dev, "Apple GPIO must have 'gpio-controller' property.\n");
> > > +           return -ENODEV;
> > > +   }
> >
> > How is it possible?
> This is possible if booted with an invalid DTB. "gpio-controller" is a
> required property according to Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml.

No, we don't do silly checks. Compare to other pin control drivers. If
DTB is wrong, you will see it sooner or later.

...

> > > +   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?

...

> > > +   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?

-- 
With Best Regards,
Andy Shevchenko