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

[Joey Gouly <joey.gouly@xxxxxxx>]

Hi Andy,

Thanks for the review!

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.

[...]

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

[...]

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

[...]

> 
> ...
> 
> > +	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`.

[...] 

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

Thanks,
Joey