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

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

[Joey Gouly]

Hi all,

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

Changes since v4 [1]:
  - Renamed functions to remove duplicate 'gpio'
  - Create all 4 functions (gpio, periph1-3)
  - Applied Acks/Review tags

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

Thanks,
Joey

[1]
https://lore.kernel.org/linux-gpio/20211024101838.43107-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          | 534 ++++++++++++++++++
 include/linux/gpio/driver.h                   |  19 +-
 7 files changed, 586 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

-- 
2.17.1

[PATCH v5 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 v5 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 v5 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 v5 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>
Acked-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pinctrl/Kconfig              |  16 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c | 534 +++++++++++++++++++++++++++
 3 files changed, 551 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..0cc346bfc4c3
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -0,0 +1,534 @@
+// 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_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_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_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_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_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_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_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_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_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_direction_input(chip, data->hwirq);
+	apple_gpio_irq_unmask(data);
+
+	return 0;
+}
+
+static int apple_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_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_irq_startup,
+	.irq_ack	= apple_gpio_irq_ack,
+	.irq_mask	= apple_gpio_irq_mask,
+	.irq_unmask	= apple_gpio_irq_unmask,
+	.irq_set_type	= apple_gpio_irq_set_type,
+};
+
+/* Probe & register */
+
+static int apple_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_get_direction;
+	pctl->gpio_chip.direction_input = apple_gpio_direction_input;
+	pctl->gpio_chip.direction_output = apple_gpio_direction_output;
+	pctl->gpio_chip.get = apple_gpio_get;
+	pctl->gpio_chip.set = apple_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_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;
+	static const char* pinmux_functions[] = {
+		"gpio", "periph1", "periph2", "periph3"
+	};
+	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");
+	}
+
+	for (i = 0; i < ARRAY_SIZE(pinmux_functions); ++i) {
+		res = pinmux_generic_add_function(pctl->pctldev, pinmux_functions[i],
+						  pin_names, npins, pctl);
+		if (res < 0)
+			return dev_err_probe(pctl->dev, res,
+					     "Failed to register function.");
+	}
+
+	return apple_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 v5 4/5] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 5:28 AM Joey Gouly <joey.gouly@arm.com> wrote:
>
> This driver adds support for the pinctrl / GPIO hardware found
> on some Apple SoCs.

...

> +#include <dt-bindings/pinctrl/apple.h>

bits.h is missed

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

...

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

Would be good to have a comma.

> +};

...

> +// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.

Wrong style.

...

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

Better

unsigned int val;
int ret;

ret = regmap_read(...);
if (ret)
  return 0;

return val;

> +}

...

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

This is incorrect. It always happens when somebody is in hurry :-)

> +       }

...

> +       apple_gpio_set_reg(
> +               pctl, group, REG_GPIOx_PERIPH | REG_GPIOx_INPUT_ENABLE,
> +               FIELD_PREP(REG_GPIOx_PERIPH, func) | REG_GPIOx_INPUT_ENABLE);

Strange indentation.

...

> +       return (FIELD_GET(REG_GPIOx_MODE, reg) == REG_GPIOx_OUT) ?
> +                      GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

Easier to read with
 if ()
   return X
return Y

...

> +       apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
> +                          value ? REG_GPIOx_DATA : 0);

One line?

> +}


...

> +       struct apple_gpio_pinctrl *pctl =
> +               gpiochip_get_data(irq_data_get_irq_chip_data(data));

One line?

> +       unsigned int irqgrp =
> +               FIELD_GET(REG_GPIOx_GRP, apple_gpio_get_reg(pctl, data->hwirq));

Ditto.

> +       writel(BIT(data->hwirq & 31),

% 32 can also do and be more specific.

> +              pctl->base + REG_IRQ(irqgrp, data->hwirq));

One line?

...

> +static void apple_gpio_irq_mask(struct irq_data *data)
> +{
> +       struct apple_gpio_pinctrl *pctl =
> +               gpiochip_get_data(irq_data_get_irq_chip_data(data));

Missed blank line.

> +       apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
> +                          FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF));
> +}

...

> +       if (!of_property_read_bool(pctl->dev->of_node, "gpio-controller"))
> +               return dev_err_probe(pctl->dev, -ENODEV,
> +                                    "No gpio-controller property\n");

Still not sure why we need this check.

...

> +       pctl->gpio_chip.of_node = pctl->dev->of_node;

It should be done by the OF GPIO library.


> +               for (i = 0; i < girq->num_parents; i++) {

> +                       ret = platform_get_irq(to_platform_device(pctl->dev),
> +                                              i);

This looks ugly even if you take the 80 char rule to your heart (it
has exceptions and here is one of them).

...

> +       ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
> +out:

out_free_irq_data:

(or alike)

> +       kfree(girq->parents);
> +       kfree(irq_data);

...

> +       static const char* pinmux_functions[] = {
> +               "gpio", "periph1", "periph2", "periph3"
> +       };

...

I see it seems pending, so some of the above may be addressed with
follow up, some may be left unconsidered.

-- 
With Best Regards,
Andy Shevchenko

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

[Joey Gouly]

Hi Andy,

On Sun, Oct 31, 2021 at 03:39:01PM +0200, Andy Shevchenko wrote:
> On Wed, Oct 27, 2021 at 5:28 AM Joey Gouly <joey.gouly@arm.com> wrote:
> >
> > This driver adds support for the pinctrl / GPIO hardware found
> > on some Apple SoCs.
> 
> ...
> 
> > +       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;
> 
> This is incorrect. It always happens when somebody is in hurry :-)
I don't understand which bit you are saying is incorrect here, can you expand on your comment?

[...]

Thanks for the other comments, I'll prepare a patch addressing it soon.

Thanks,
Joey

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

[Andy Shevchenko]

On Mon, Nov 1, 2021 at 7:53 PM Joey Gouly <joey.gouly@arm.com> wrote:
> On Sun, Oct 31, 2021 at 03:39:01PM +0200, Andy Shevchenko wrote:
> > On Wed, Oct 27, 2021 at 5:28 AM Joey Gouly <joey.gouly@arm.com> wrote:

...

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

if (ret == 0)
 dev_err();
 return 0;

> > This is incorrect. It always happens when somebody is in hurry :-)
> I don't understand which bit you are saying is incorrect here, can you expand on your comment?

See above.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 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>
Acked-by: Hector Martin <marcan@marcan.st>
---
 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 v5 0/5] pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

Hi,

On Tue, Oct 26, 2021 at 06:58:10PM +0100, Joey Gouly wrote:
> Hi all,
> 
> Here is the v5 patchset for the Apple pinctrl/GPIO driver.
> 
> Changes since v4 [1]:
>   - Renamed functions to remove duplicate 'gpio'
>   - Create all 4 functions (gpio, periph1-3)
>   - Applied Acks/Review tags
> 
> There is a branch here with the driver:
>   https://gitlab.arm.com/linux-arm/jg-open/-/tree/pinctrl_apple_v4

Correct url is:

  https://gitlab.arm.com/linux-arm/jg-open/-/commits/pinctrl_apple_v5/

(I'm guessing the patches will be applied with b4, but just in case..)

Thanks,
Joey

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

[Linus Walleij]

On Tue, Oct 26, 2021 at 7:58 PM Joey Gouly <joey.gouly@arm.com> wrote:

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

Excellent. As agreed I have applied patch 1 to an immutable branch:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-gpio-ppid
If Bartosz needs this patch he can pull it into the GPIO subsystem.

Then I have applied patches 2-4 on top of this branch for v5.16.

I leave patch 5 for Marcan so he can minimize MAINTAINERS
collisions.

Thanks for your hard work on this!

If any issues remains to be addressed they can be done with incremental
patches for sure.

Yours,
Linus Walleij

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

[Hector Martin]

On 27/10/2021 02.58, Joey Gouly wrote:
> Hi all,
> 
> Here is the v5 patchset for the Apple pinctrl/GPIO driver.
> 
> Changes since v4 [1]:
>    - Renamed functions to remove duplicate 'gpio'
>    - Create all 4 functions (gpio, periph1-3)
>    - Applied Acks/Review tags
> 
> There is a branch here with the driver:
>    https://gitlab.arm.com/linux-arm/jg-open/-/tree/pinctrl_apple_v4
> 
> Thanks,
> Joey

Thank you! I've applied patch 5 to asahi-soc/maintainers and will be 
sending that out to SoC shortly :)

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