Hi Andy,

Thanks for the review!

On Wed, Sep 22, 2021 at 10:20:39AM +0300, Andy Shevchenko wrote:

[...]

quoted

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

[...]

quoted

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

[...]

quoted

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

[...]

...

quoted

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

[...] 

quoted

+	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