[PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs

[PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

Hi all,

Here is the v4 patchset for the Apple pinctrl/GPIO driver.

Changes since v3 [1]:
  - Applied Marc Zyngier's review/patch (with exception noted below)
  - Removed "apple,t8103-pinctrl" compatible from driver
  - Applied Acks/Review tags


With Marc's changes, the irq_chip was being shared between pinctrl
drivers and I was getting the following warning:

  drivers/gpio/gpiolib.c:1456:

    detected irqchip that is shared with multiple gpiochips: please fix
the driver.


So I applied the following diff to Marc's change, to not share the
irq_chips, while still being cleaner overall:

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c
b/drivers/pinctrl/pinctrl-apple-gpio.c
index 732c347a2bbc..ce037a5c15c1 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -35,6 +35,7 @@ struct apple_gpio_pinctrl {

        struct pinctrl_desc pinctrl_desc;
        struct gpio_chip gpio_chip;
+       struct irq_chip irq_chip;
        u8 irqgrps[0];
 };

@@ -369,6 +370,8 @@ static int apple_gpio_gpio_register(struct
apple_gpio_pinctrl *pctl)
                return dev_err_probe(pctl->dev, -ENODEV,
                                     "No gpio-controller property\n");

+       pctl->irq_chip = apple_gpio_irqchip;
+
        pctl->gpio_chip.label = dev_name(pctl->dev);
        pctl->gpio_chip.request = gpiochip_generic_request;
        pctl->gpio_chip.free = gpiochip_generic_free;
@@ -385,7 +388,7 @@ static int apple_gpio_gpio_register(struct
apple_gpio_pinctrl *pctl)
        if (girq->num_parents) {
                int i;

-               girq->chip = &apple_gpio_irqchip;
+               girq->chip = &pctl->irq_chip;
                girq->parent_handler = apple_gpio_gpio_irq_handler;

                girq->parents = kmalloc_array(girq->num_parents,

Marc said that hierarchical IRQ domains should solve this problem, but
I'll let him explain more on the list, maybe that should solved in a different
patch series.

There is a branch here with the driver:
  https://gitlab.arm.com/linux-arm/jg-open/-/tree/pinctrl_apple_v4

Thanks,
Joey

note: For those that have been testing this with PCIe etc, you will
probably want to also include the last commit in the following branch, as I
dropped the clock references in the code (due to the switch to power domains):
https://gitlab.arm.com/linux-arm/jg-open/-/commits/pinctrl_apple_v4_clock

[1]
https://lore.kernel.org/linux-gpio/20211016141839.45460-1-joey.gouly@arm.com/

Joey Gouly (4):
  dt-bindings: pinctrl: add #interrupt-cells to apple,pinctrl
  dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl
  pinctrl: add pinctrl/GPIO driver for Apple SoCs
  MAINTAINERS: add pinctrl-apple-gpio to ARM/APPLE MACHINE

Marc Zyngier (1):
  gpio: Allow per-parent interrupt data

 .../bindings/pinctrl/apple,pinctrl.yaml       |  10 +
 MAINTAINERS                                   |   1 +
 drivers/gpio/gpiolib.c                        |   9 +-
 drivers/pinctrl/Kconfig                       |  16 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c          | 535 ++++++++++++++++++
 include/linux/gpio/driver.h                   |  19 +-
 7 files changed, 587 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

-- 
2.17.1

[PATCH v4 1/5] gpio: Allow per-parent interrupt data

[Joey Gouly]

From: Marc Zyngier <maz@kernel.org>

The core gpiolib code is able to deal with multiple interrupt parents
for a single gpio irqchip. It however only allows a single piece
of data to be conveyed to all flow handlers (either the gpio_chip
or some other, driver-specific data).

This means that drivers have to go through some interesting dance
to find the correct context, something that isn't great in interrupt
context (see aebdc8abc9db86e2bd33070fc2f961012fff74b4 for a prime
example).

Instead, offer an optional way for a pinctrl/gpio driver to provide
an array of pointers which gets used to provide the correct context
to the flow handler.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c      |  9 +++++++--
 include/linux/gpio/driver.h | 19 +++++++++++++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1b9b721218f..abfbf546d159 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1534,9 +1534,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc,
 	}
 
 	if (gc->irq.parent_handler) {
-		void *data = gc->irq.parent_handler_data ?: gc;
-
 		for (i = 0; i < gc->irq.num_parents; i++) {
+			void *data;
+
+			if (gc->irq.per_parent_data)
+				data = gc->irq.parent_handler_data_array[i];
+			else
+				data = gc->irq.parent_handler_data ?: gc;
+
 			/*
 			 * The parent IRQ chip is already using the chip_data
 			 * for this IRQ chip, so our callbacks simply use the
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a0f9901dcae6..a673a359e20b 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -168,11 +168,18 @@ struct gpio_irq_chip {
 
 	/**
 	 * @parent_handler_data:
+	 * @parent_handler_data_array:
 	 *
 	 * Data associated, and passed to, the handler for the parent
-	 * interrupt.
+	 * interrupt. Can either be a single pointer if @per_parent_data
+	 * is false, or an array of @num_parents pointers otherwise.  If
+	 * @per_parent_data is true, @parent_handler_data_array cannot be
+	 * NULL.
 	 */
-	void *parent_handler_data;
+	union {
+		void *parent_handler_data;
+		void **parent_handler_data_array;
+	};
 
 	/**
 	 * @num_parents:
@@ -203,6 +210,14 @@ struct gpio_irq_chip {
 	 */
 	bool threaded;
 
+	/**
+	 * @per_parent_data:
+	 *
+	 * True if parent_handler_data_array describes a @num_parents
+	 * sized array to be used as parent data.
+	 */
+	bool per_parent_data;
+
 	/**
 	 * @init_hw: optional routine to initialize hardware before
 	 * an IRQ chip will be added. This is quite useful when
-- 
2.17.1

[PATCH v4 2/5] dt-bindings: pinctrl: add #interrupt-cells to apple,pinctrl

[Joey Gouly]

The GPIO/pinctrl hardware can act as an interrupt-controller, so add
the #interrupt-cells property to the binding.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
index d50571affd1f..0b3eb068fb12 100644
--- a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
@@ -43,6 +43,9 @@ properties:
 
   interrupt-controller: true
 
+  '#interrupt-cells':
+    const: 2
+
 patternProperties:
   '-pins$':
     type: object
@@ -88,6 +91,7 @@ examples:
         gpio-ranges = <&pinctrl 0 0 212>;
 
         interrupt-controller;
+        #interrupt-cells = <2>;
         interrupt-parent = <&aic>;
         interrupts = <AIC_IRQ 16 IRQ_TYPE_LEVEL_HIGH>,
                      <AIC_IRQ 17 IRQ_TYPE_LEVEL_HIGH>,
-- 
2.17.1

[PATCH v4 3/5] dt-bindings: pinctrl: Add apple,npins property to apple,pinctrl

[Joey Gouly]

This property is used to describe the total number of pins on this
particular pinctrl hardware block.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pinctrl/apple,pinctrl.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
index 0b3eb068fb12..07b00de79755 100644
--- a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
@@ -34,6 +34,10 @@ properties:
   gpio-ranges:
     maxItems: 1
 
+  apple,npins:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: The number of pins in this GPIO controller.
+
   interrupts:
     description: One interrupt for each of the (up to 7) interrupt
       groups supported by the controller sorted by interrupt group
@@ -69,6 +73,7 @@ required:
   - gpio-controller
   - '#gpio-cells'
   - gpio-ranges
+  - apple,npins
 
 additionalProperties: false
 
@@ -89,6 +94,7 @@ examples:
         gpio-controller;
         #gpio-cells = <2>;
         gpio-ranges = <&pinctrl 0 0 212>;
+        apple,npins = <212>;
 
         interrupt-controller;
         #interrupt-cells = <2>;
-- 
2.17.1

[PATCH v4 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

This driver adds support for the pinctrl / GPIO hardware found
on some Apple SoCs.

Co-developed-by: Stan Skowronek <stan@corellium.com>
Signed-off-by: Stan Skowronek <stan@corellium.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/Kconfig              |  16 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c | 535 +++++++++++++++++++++++++++
 3 files changed, 552 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 31921108e456..6a961d5f8726 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -31,6 +31,22 @@ config DEBUG_PINCTRL
 	help
 	  Say Y here to add some extra checks and diagnostics to PINCTRL calls.
 
+config PINCTRL_APPLE_GPIO
+	tristate "Apple SoC GPIO pin controller driver"
+	depends on ARCH_APPLE
+	select PINMUX
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select OF_GPIO
+	help
+	  This is the driver for the GPIO controller found on Apple ARM SoCs,
+	  including M1.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called pinctrl-apple-gpio.
+
 config PINCTRL_ARTPEC6
 	bool "Axis ARTPEC-6 pin controller driver"
 	depends on MACH_ARTPEC6
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 200073bcc2c1..5e63de2ffcf4 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
 obj-$(CONFIG_OF)		+= devicetree.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
+obj-$(CONFIG_PINCTRL_APPLE_GPIO) += pinctrl-apple-gpio.o
 obj-$(CONFIG_PINCTRL_ARTPEC6)	+= pinctrl-artpec6.o
 obj-$(CONFIG_PINCTRL_AS3722)	+= pinctrl-as3722.o
 obj-$(CONFIG_PINCTRL_AXP209)	+= pinctrl-axp209.o
diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
new file mode 100644
index 000000000000..ce037a5c15c1
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -0,0 +1,535 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Apple SoC pinctrl+GPIO+external IRQ driver
+ *
+ * Copyright (C) The Asahi Linux Contributors
+ * Copyright (C) 2020 Corellium LLC
+ *
+ * Based on: pinctrl-pistachio.c
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ * Copyright (C) 2014 Google, Inc.
+ */
+
+#include <dt-bindings/pinctrl/apple.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "pinctrl-utils.h"
+#include "core.h"
+#include "pinmux.h"
+
+struct apple_gpio_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+
+	void __iomem *base;
+	struct regmap *map;
+
+	struct pinctrl_desc pinctrl_desc;
+	struct gpio_chip gpio_chip;
+	struct irq_chip irq_chip;
+	u8 irqgrps[0];
+};
+
+#define REG_GPIO(x)          (4 * (x))
+#define REG_GPIOx_DATA       BIT(0)
+#define REG_GPIOx_MODE       GENMASK(3, 1)
+#define REG_GPIOx_OUT        1
+#define REG_GPIOx_IN_IRQ_HI  2
+#define REG_GPIOx_IN_IRQ_LO  3
+#define REG_GPIOx_IN_IRQ_UP  4
+#define REG_GPIOx_IN_IRQ_DN  5
+#define REG_GPIOx_IN_IRQ_ANY 6
+#define REG_GPIOx_IN_IRQ_OFF 7
+#define REG_GPIOx_PERIPH     GENMASK(6, 5)
+#define REG_GPIOx_PULL       GENMASK(8, 7)
+#define REG_GPIOx_PULL_OFF   0
+#define REG_GPIOx_PULL_DOWN  1
+#define REG_GPIOx_PULL_UP_STRONG 2
+#define REG_GPIOx_PULL_UP    3
+#define REG_GPIOx_INPUT_ENABLE BIT(9)
+#define REG_GPIOx_DRIVE_STRENGTH0 GENMASK(11, 10)
+#define REG_GPIOx_SCHMITT    BIT(15)
+#define REG_GPIOx_GRP        GENMASK(18, 16)
+#define REG_GPIOx_LOCK       BIT(21)
+#define REG_GPIOx_DRIVE_STRENGTH1 GENMASK(23, 22)
+#define REG_IRQ(g, x)        (0x800 + 0x40 * (g) + 4 * ((x) >> 5))
+
+struct regmap_config regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.cache_type = REGCACHE_FLAT,
+	.max_register = 512 * sizeof(u32),
+	.num_reg_defaults_raw = 512,
+	.use_relaxed_mmio = true
+};
+
+// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.
+static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl,
+			       unsigned int pin, u32 mask, u32 value)
+{
+	regmap_update_bits(pctl->map, REG_GPIO(pin), mask, value);
+}
+
+static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
+				   unsigned int pin)
+{
+	unsigned int val = 0;
+
+	regmap_read(pctl->map, REG_GPIO(pin), &val);
+	return val;
+}
+
+/* Pin controller functions */
+
+static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
+				     struct device_node *node,
+				     struct pinctrl_map **map,
+				     unsigned *num_maps)
+{
+	unsigned reserved_maps;
+	struct apple_gpio_pinctrl *pctl;
+	u32 pinfunc, pin, func;
+	int num_pins, i, ret;
+	const char *group_name;
+	const char *function_name;
+
+	*map = NULL;
+	*num_maps = 0;
+	reserved_maps = 0;
+
+	pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	ret = of_property_count_u32_elems(node, "pinmux");
+	if (ret <= 0) {
+		dev_err(pctl->dev,
+			"missing or empty pinmux property in node %pOFn.\n",
+			node);
+		return ret;
+	}
+
+	num_pins = ret;
+
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
+					num_pins);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_pins; i++) {
+		ret = of_property_read_u32_index(node, "pinmux", i, &pinfunc);
+		if (ret)
+			goto free_map;
+
+		pin = APPLE_PIN(pinfunc);
+		func = APPLE_FUNC(pinfunc);
+
+		if (func >= pinmux_generic_get_function_count(pctldev)) {
+			ret = -EINVAL;
+			goto free_map;
+		}
+
+		group_name = pinctrl_generic_get_group_name(pctldev, pin);
+		function_name =
+			pinmux_generic_get_function_name(pctl->pctldev, func);
+		ret = pinctrl_utils_add_map_mux(pctl->pctldev, map,
+						&reserved_maps, num_maps,
+						group_name, function_name);
+		if (ret)
+			goto free_map;
+	}
+
+free_map:
+	if (ret < 0)
+		pinctrl_utils_free_map(pctldev, *map, *num_maps);
+
+	return ret;
+}
+
+static const struct pinctrl_ops apple_gpio_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+	.dt_node_to_map = apple_gpio_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+/* Pin multiplexer functions */
+
+static int apple_gpio_pinmux_set(struct pinctrl_dev *pctldev, unsigned func,
+				    unsigned group)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	apple_gpio_set_reg(
+		pctl, group, REG_GPIOx_PERIPH | REG_GPIOx_INPUT_ENABLE,
+		FIELD_PREP(REG_GPIOx_PERIPH, func) | REG_GPIOx_INPUT_ENABLE);
+
+	return 0;
+}
+
+static const struct pinmux_ops apple_gpio_pinmux_ops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.set_mux = apple_gpio_pinmux_set,
+	.strict = true,
+};
+
+/* GPIO chip functions */
+
+static int apple_gpio_gpio_get_direction(struct gpio_chip *chip,
+					 unsigned int offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+	unsigned int reg = apple_gpio_get_reg(pctl, offset);
+
+	return (FIELD_GET(REG_GPIOx_MODE, reg) == REG_GPIOx_OUT) ?
+		       GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int apple_gpio_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+	unsigned int reg = apple_gpio_get_reg(pctl, offset);
+
+	/*
+	 * If this is an input GPIO, read the actual value (not the
+	 * cached regmap value)
+	 */
+	if (FIELD_GET(REG_GPIOx_MODE, reg) != REG_GPIOx_OUT)
+		reg = readl_relaxed(pctl->base + REG_GPIO(offset));
+
+	return !!(reg & REG_GPIOx_DATA);
+}
+
+static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				int value)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
+			   value ? REG_GPIOx_DATA : 0);
+}
+
+static int apple_gpio_gpio_direction_input(struct gpio_chip *chip,
+					   unsigned int offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, offset,
+			   REG_GPIOx_PERIPH | REG_GPIOx_MODE | REG_GPIOx_DATA |
+				   REG_GPIOx_INPUT_ENABLE,
+			   FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF) |
+				   REG_GPIOx_INPUT_ENABLE);
+	return 0;
+}
+
+static int apple_gpio_gpio_direction_output(struct gpio_chip *chip,
+					    unsigned int offset, int value)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, offset,
+			   REG_GPIOx_PERIPH | REG_GPIOx_MODE | REG_GPIOx_DATA,
+			   FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_OUT) |
+				   (value ? REG_GPIOx_DATA : 0));
+	return 0;
+}
+
+/* IRQ chip functions */
+
+static void apple_gpio_gpio_irq_ack(struct irq_data *data)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	unsigned int irqgrp =
+		FIELD_GET(REG_GPIOx_GRP, apple_gpio_get_reg(pctl, data->hwirq));
+
+	writel(BIT(data->hwirq & 31),
+	       pctl->base + REG_IRQ(irqgrp, data->hwirq));
+}
+
+static int apple_gpio_irq_type(unsigned int type)
+{
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		return REG_GPIOx_IN_IRQ_UP;
+	case IRQ_TYPE_EDGE_FALLING:
+		return REG_GPIOx_IN_IRQ_DN;
+	case IRQ_TYPE_EDGE_BOTH:
+		return REG_GPIOx_IN_IRQ_ANY;
+	case IRQ_TYPE_LEVEL_HIGH:
+		return REG_GPIOx_IN_IRQ_HI;
+	case IRQ_TYPE_LEVEL_LOW:
+		return REG_GPIOx_IN_IRQ_LO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static void apple_gpio_gpio_irq_mask(struct irq_data *data)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
+			   FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF));
+}
+
+static void apple_gpio_gpio_irq_unmask(struct irq_data *data)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	int irqtype = apple_gpio_irq_type(irqd_get_trigger_type(data));
+
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
+			   FIELD_PREP(REG_GPIOx_MODE, irqtype));
+}
+
+static unsigned int apple_gpio_gpio_irq_startup(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP,
+			   FIELD_PREP(REG_GPIOx_GRP, 0));
+
+	apple_gpio_gpio_direction_input(chip, data->hwirq);
+	apple_gpio_gpio_irq_unmask(data);
+
+	return 0;
+}
+
+static int apple_gpio_gpio_irq_set_type(struct irq_data *data,
+					unsigned int type)
+{
+	struct apple_gpio_pinctrl *pctl =
+		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	int irqtype = apple_gpio_irq_type(type);
+
+	if (irqtype < 0)
+		return irqtype;
+
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
+			   FIELD_PREP(REG_GPIOx_MODE, irqtype));
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(data, handle_level_irq);
+	else
+		irq_set_handler_locked(data, handle_edge_irq);
+	return 0;
+}
+
+static void apple_gpio_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u8 *grpp = irq_desc_get_handler_data(desc);
+	struct apple_gpio_pinctrl *pctl;
+	unsigned int pinh, pinl;
+	unsigned long pending;
+	struct gpio_chip *gc;
+
+	pctl = container_of(grpp - *grpp, typeof(*pctl), irqgrps[0]);
+	gc = &pctl->gpio_chip;
+
+	chained_irq_enter(chip, desc);
+	for (pinh = 0; pinh < gc->ngpio; pinh += 32) {
+		pending = readl_relaxed(pctl->base + REG_IRQ(*grpp, pinh));
+		for_each_set_bit(pinl, &pending, 32)
+			generic_handle_domain_irq(gc->irq.domain, pinh + pinl);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+static struct irq_chip apple_gpio_irqchip = {
+	.name		= "Apple-GPIO",
+	.irq_startup	= apple_gpio_gpio_irq_startup,
+	.irq_ack	= apple_gpio_gpio_irq_ack,
+	.irq_mask	= apple_gpio_gpio_irq_mask,
+	.irq_unmask	= apple_gpio_gpio_irq_unmask,
+	.irq_set_type	= apple_gpio_gpio_irq_set_type,
+};
+
+/* Probe & register */
+
+static int apple_gpio_gpio_register(struct apple_gpio_pinctrl *pctl)
+{
+	struct gpio_irq_chip *girq = &pctl->gpio_chip.irq;
+	void **irq_data = NULL;
+	int ret;
+
+	if (!of_property_read_bool(pctl->dev->of_node, "gpio-controller"))
+		return dev_err_probe(pctl->dev,	-ENODEV,
+				     "No gpio-controller property\n");
+
+	pctl->irq_chip = apple_gpio_irqchip;
+
+	pctl->gpio_chip.label = dev_name(pctl->dev);
+	pctl->gpio_chip.request = gpiochip_generic_request;
+	pctl->gpio_chip.free = gpiochip_generic_free;
+	pctl->gpio_chip.get_direction = apple_gpio_gpio_get_direction;
+	pctl->gpio_chip.direction_input = apple_gpio_gpio_direction_input;
+	pctl->gpio_chip.direction_output = apple_gpio_gpio_direction_output;
+	pctl->gpio_chip.get = apple_gpio_gpio_get;
+	pctl->gpio_chip.set = apple_gpio_gpio_set;
+	pctl->gpio_chip.base = -1;
+	pctl->gpio_chip.ngpio = pctl->pinctrl_desc.npins;
+	pctl->gpio_chip.parent = pctl->dev;
+	pctl->gpio_chip.of_node = pctl->dev->of_node;
+
+	if (girq->num_parents) {
+		int i;
+
+		girq->chip = &pctl->irq_chip;
+		girq->parent_handler = apple_gpio_gpio_irq_handler;
+
+		girq->parents = kmalloc_array(girq->num_parents,
+					      sizeof(*girq->parents),
+					      GFP_KERNEL);
+		irq_data = kmalloc_array(girq->num_parents, sizeof(*irq_data),
+					 GFP_KERNEL);
+		if (!girq->parents || !irq_data) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
+		for (i = 0; i < girq->num_parents; i++) {
+			ret = platform_get_irq(to_platform_device(pctl->dev),
+					       i);
+			if (ret < 0)
+				goto out;
+
+			girq->parents[i] = ret;
+			pctl->irqgrps[i] = i;
+			irq_data[i] = &pctl->irqgrps[i];
+		}
+
+		girq->parent_handler_data_array = irq_data;
+		girq->per_parent_data = true;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_level_irq;
+	}
+
+	ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
+out:
+	kfree(girq->parents);
+	kfree(irq_data);
+
+	return ret;
+}
+
+static int apple_gpio_pinctrl_probe(struct platform_device *pdev)
+{
+	struct apple_gpio_pinctrl *pctl;
+	struct pinctrl_pin_desc *pins;
+	unsigned int npins;
+	const char **pin_names;
+	unsigned int *pin_nums;
+	unsigned int i, nirqs = 0;
+	int res;
+
+	if (of_property_read_bool(pdev->dev.of_node, "interrupt-controller")) {
+		res = platform_irq_count(pdev);
+		if (res > 0)
+			nirqs = res;
+	}
+
+	pctl = devm_kzalloc(&pdev->dev, struct_size(pctl, irqgrps, nirqs),
+			    GFP_KERNEL);
+	if (!pctl)
+		return -ENOMEM;
+	pctl->dev = &pdev->dev;
+	pctl->gpio_chip.irq.num_parents = nirqs;
+	dev_set_drvdata(&pdev->dev, pctl);
+
+	if (of_property_read_u32(pdev->dev.of_node, "apple,npins", &npins))
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "apple,npins property not found\n");
+
+	pins = devm_kmalloc_array(&pdev->dev, npins, sizeof(pins[0]),
+				  GFP_KERNEL);
+	pin_names = devm_kmalloc_array(&pdev->dev, npins, sizeof(pin_names[0]),
+				       GFP_KERNEL);
+	pin_nums = devm_kmalloc_array(&pdev->dev, npins, sizeof(pin_nums[0]),
+				      GFP_KERNEL);
+	if (!pins || !pin_names || !pin_nums)
+		return -ENOMEM;
+
+	pctl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pctl->base))
+		return PTR_ERR(pctl->base);
+
+	pctl->map = devm_regmap_init_mmio(&pdev->dev, pctl->base, &regmap_config);
+	if (IS_ERR(pctl->map))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pctl->map),
+				     "Failed to create regmap\n");
+
+	for (i = 0; i < npins; i++) {
+		pins[i].number = i;
+		pins[i].name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "PIN%u", i);
+		pins[i].drv_data = pctl;
+		pin_names[i] = pins[i].name;
+		pin_nums[i] = i;
+	}
+
+	pctl->pinctrl_desc.name = dev_name(pctl->dev);
+	pctl->pinctrl_desc.pins = pins;
+	pctl->pinctrl_desc.npins = npins;
+	pctl->pinctrl_desc.pctlops = &apple_gpio_pinctrl_ops;
+	pctl->pinctrl_desc.pmxops = &apple_gpio_pinmux_ops;
+
+	pctl->pctldev =	devm_pinctrl_register(&pdev->dev, &pctl->pinctrl_desc, pctl);
+	if (IS_ERR(pctl->pctldev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pctl->pctldev),
+				     "Failed to register pinctrl device.\n");
+
+	for (i = 0; i < npins; i++) {
+		res = pinctrl_generic_add_group(pctl->pctldev, pins[i].name,
+						pin_nums + i, 1, pctl);
+		if (res < 0)
+			return dev_err_probe(pctl->dev, res,
+					     "Failed to register group");
+	}
+
+	res = pinmux_generic_add_function(pctl->pctldev, "gpio", pin_names,
+					  npins, pctl);
+	if (res < 0)
+		return dev_err_probe(pctl->dev, res,
+				     "Failed to register function.");
+
+	res = pinmux_generic_add_function(pctl->pctldev, "periph", pin_names,
+					  npins, pctl);
+	if (res < 0)
+		return dev_err_probe(pctl->dev, res,
+				     "Failed to register function.");
+
+	return apple_gpio_gpio_register(pctl);
+}
+
+static const struct of_device_id apple_gpio_pinctrl_of_match[] = {
+	{ .compatible = "apple,pinctrl", },
+	{ }
+};
+
+static struct platform_driver apple_gpio_pinctrl_driver = {
+	.driver = {
+		.name = "apple-gpio-pinctrl",
+		.of_match_table = apple_gpio_pinctrl_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = apple_gpio_pinctrl_probe,
+};
+module_platform_driver(apple_gpio_pinctrl_driver);
+
+MODULE_DESCRIPTION("Apple pinctrl/GPIO driver");
+MODULE_AUTHOR("Stan Skowronek <stan@corellium.com>");
+MODULE_AUTHOR("Joey Gouly <joey.gouly@arm.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v4 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Hector Martin]

On 24/10/2021 19.18, Joey Gouly wrote:
> This driver adds support for the pinctrl / GPIO hardware found
> on some Apple SoCs.
> 
> Co-developed-by: Stan Skowronek <stan@corellium.com>
> Signed-off-by: Stan Skowronek <stan@corellium.com>
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
[snip]

> +/* GPIO chip functions */
> +
> +static int apple_gpio_gpio_get_direction(struct gpio_chip *chip,
> +					 unsigned int offset)

Nit: do we really need to gpio_gpio all the things? I think maz already 
mentioned this one.

> +	for (i = 0; i < npins; i++) {
> +		res = pinctrl_generic_add_group(pctl->pctldev, pins[i].name,
> +						pin_nums + i, 1, pctl);
> +		if (res < 0)
> +			return dev_err_probe(pctl->dev, res,
> +					     "Failed to register group");
> +	}
> +
> +	res = pinmux_generic_add_function(pctl->pctldev, "gpio", pin_names,
> +					  npins, pctl);
> +	if (res < 0)
> +		return dev_err_probe(pctl->dev, res,
> +				     "Failed to register function.");
> +
> +	res = pinmux_generic_add_function(pctl->pctldev, "periph", pin_names,
> +					  npins, pctl);

Since we have up to 3 peripheral functions I guess this should be done 
thrice, as "periph0" - "periph2" (or maybe 1-3 if you prefer the 
1-indexed version, as it matches the selector).

AFAICT this doesn't have DT impact though, so it's not a big deal since 
right now I don't think we use any functions > 1.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH v4 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

Hi,

On Mon, Oct 25, 2021 at 06:07:35PM +0900, Hector Martin wrote:
> On 24/10/2021 19.18, Joey Gouly wrote:
> > This driver adds support for the pinctrl / GPIO hardware found
> > on some Apple SoCs.
> > 
> > Co-developed-by: Stan Skowronek <stan@corellium.com>
> > Signed-off-by: Stan Skowronek <stan@corellium.com>
> > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > ---
> [snip]
> 
> > +/* GPIO chip functions */
> > +
> > +static int apple_gpio_gpio_get_direction(struct gpio_chip *chip,
> > +					 unsigned int offset)
> 
> Nit: do we really need to gpio_gpio all the things? I think maz already
> mentioned this one.
> 

I forgot to reply to that. The reason (perhaps not a good one), is that the
module is called 'apple_gpio' and these set of functions are related to the
GPIO interface (not the pinctrl side of things). I'm not tied to the names
either way.

Thanks,
Joey

Re: [PATCH v4 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Hector Martin]

On 25/10/2021 18.41, Joey Gouly wrote:
> Hi,
> 
> On Mon, Oct 25, 2021 at 06:07:35PM +0900, Hector Martin wrote:
>> On 24/10/2021 19.18, Joey Gouly wrote:
>>> This driver adds support for the pinctrl / GPIO hardware found
>>> on some Apple SoCs.
>>>
>>> Co-developed-by: Stan Skowronek <stan@corellium.com>
>>> Signed-off-by: Stan Skowronek <stan@corellium.com>
>>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>>> ---
>> [snip]
>>
>>> +/* GPIO chip functions */
>>> +
>>> +static int apple_gpio_gpio_get_direction(struct gpio_chip *chip,
>>> +					 unsigned int offset)
>>
>> Nit: do we really need to gpio_gpio all the things? I think maz already
>> mentioned this one.
>>
> 
> I forgot to reply to that. The reason (perhaps not a good one), is that the
> module is called 'apple_gpio' and these set of functions are related to the
> GPIO interface (not the pinctrl side of things). I'm not tied to the names
> either way.

I figured that would be it. Personally I'd just consider the second 
"gpio" implicit here and drop it, but that's me; you can do it however 
you want :)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

[PATCH v4 5/5] MAINTAINERS: add pinctrl-apple-gpio to ARM/APPLE MACHINE

[Joey Gouly]

Add the Apple SoC pinctrl driver to the ARM/APPLE MACHINE entry
in MAINTAINERS.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ca6d6fde85cf..e83e992b8ada 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1722,6 +1722,7 @@ F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
+F:	drivers/pinctrl/pinctrl-apple-gpio.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 F:	include/dt-bindings/pinctrl/apple.h
 
-- 
2.17.1

Re: [PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs

[Linus Walleij]

On Sun, Oct 24, 2021 at 12:19 PM Joey Gouly <joey.gouly@arm.com> wrote:

> Here is the v4 patchset for the Apple pinctrl/GPIO driver.

I'll wait for Marc Z:s comment on the irqchip instances, I'm
lost myself, I always solved it with per-instance irqchips.

Otherwise the patch series looks good to me, if Marcan
ACKs it too I am ready to merge them!

Yours,
Linus Walleij

Re: [PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs

[Hector Martin]

On 24/10/2021 19.18, Joey Gouly wrote:
> Hi all,
> 
> Here is the v4 patchset for the Apple pinctrl/GPIO driver.
> 
> Changes since v3 [1]:
>    - Applied Marc Zyngier's review/patch (with exception noted below)
>    - Removed "apple,t8103-pinctrl" compatible from driver
>    - Applied Acks/Review tags

I mentioned two nits in a reply to #4, but nothing worth blocking 
merging over, so this is:

Acked-by: Hector Martin <marcan@marcan.st>

Linus: You can take everything but the MAINTAINERS patch, I'll push that 
one via SoC together with everything else for this window to avoid merge 
hell :)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs

[Linus Walleij]

On Mon, Oct 25, 2021 at 11:13 AM Hector Martin <marcan@marcan.st> wrote:
> On 24/10/2021 19.18, Joey Gouly wrote:
> > Hi all,
> >
> > Here is the v4 patchset for the Apple pinctrl/GPIO driver.
> >
> > Changes since v3 [1]:
> >    - Applied Marc Zyngier's review/patch (with exception noted below)
> >    - Removed "apple,t8103-pinctrl" compatible from driver
> >    - Applied Acks/Review tags
>
> I mentioned two nits in a reply to #4, but nothing worth blocking
> merging over, so this is:
>
> Acked-by: Hector Martin <marcan@marcan.st>
>
> Linus: You can take everything but the MAINTAINERS patch, I'll push that
> one via SoC together with everything else for this window to avoid merge
> hell :)

Excellent Joey can you respin the series with Hectors fixes on patch #4
and add in the ACKs and review tags also Marc Z:s, so I can queue
this for v5.16 ASAP.

Any remaining issues can certainly be ironed out in-tree.

Yours,
Linus Walleij

Re: [PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs

[Marc Zyngier]

On Sun, 24 Oct 2021 11:18:33 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Hi all,
> 
> Here is the v4 patchset for the Apple pinctrl/GPIO driver.
> 
> Changes since v3 [1]:
>   - Applied Marc Zyngier's review/patch (with exception noted below)
>   - Removed "apple,t8103-pinctrl" compatible from driver
>   - Applied Acks/Review tags
> 
> 
> With Marc's changes, the irq_chip was being shared between pinctrl
> drivers and I was getting the following warning:
> 
>   drivers/gpio/gpiolib.c:1456:
> 
>     detected irqchip that is shared with multiple gpiochips: please fix
> the driver.
> 
> 
> So I applied the following diff to Marc's change, to not share the
> irq_chips, while still being cleaner overall:
> 
> diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c
> b/drivers/pinctrl/pinctrl-apple-gpio.c
> index 732c347a2bbc..ce037a5c15c1 100644
> --- a/drivers/pinctrl/pinctrl-apple-gpio.c
> +++ b/drivers/pinctrl/pinctrl-apple-gpio.c
> @@ -35,6 +35,7 @@ struct apple_gpio_pinctrl {
> 
>         struct pinctrl_desc pinctrl_desc;
>         struct gpio_chip gpio_chip;
> +       struct irq_chip irq_chip;
>         u8 irqgrps[0];
>  };
> 
> @@ -369,6 +370,8 @@ static int apple_gpio_gpio_register(struct
> apple_gpio_pinctrl *pctl)
>                 return dev_err_probe(pctl->dev, -ENODEV,
>                                      "No gpio-controller property\n");
> 
> +       pctl->irq_chip = apple_gpio_irqchip;
> +
>         pctl->gpio_chip.label = dev_name(pctl->dev);
>         pctl->gpio_chip.request = gpiochip_generic_request;
>         pctl->gpio_chip.free = gpiochip_generic_free;
> @@ -385,7 +388,7 @@ static int apple_gpio_gpio_register(struct
> apple_gpio_pinctrl *pctl)
>         if (girq->num_parents) {
>                 int i;
> 
> -               girq->chip = &apple_gpio_irqchip;
> +               girq->chip = &pctl->irq_chip;
>                 girq->parent_handler = apple_gpio_gpio_irq_handler;
> 
>                 girq->parents = kmalloc_array(girq->num_parents,
> 
> Marc said that hierarchical IRQ domains should solve this problem, but
> I'll let him explain more on the list, maybe that should solved in a different
> patch series.

The issue I have with the gpiolib code is that it hijacks function
pointers from a structure that is not under its control, and that is
exactly what the hierarchical IRQ domains/irqchips were supposed to
prevent.

It isn't obvious to me why this cannot be fixed with a gpiolib domain
and irqchip stacked on top of the one exposed by the low-level driver,
providing the required hooks in a standard way. Yes, this is even more
indirection. It also isn't clear why gpiochip_set_irq_hooks() shouts:
if the hooks are already there, move on.

Ultimately, this sort of manipulation is what prevents the irq_chip
structure from being made 'const' everywhere (ok, there is another nit
because of the parent_device field, which I'm looking at getting rid
of). Keeping writable function pointers isn't great, overall.

Now, given that this is an issue that isn't directly related to the
driver at hand, it shouldn't be a blocker for merging it.

So for the driver itself:

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.