[PATCH v1 0/1] pinctrl/GPIO driver for Apple SoCs

[PATCH v1 0/1] pinctrl/GPIO driver for Apple SoCs

[Joey Gouly]

Hi all,

This is a driver for the combined pinctrl / GPIO hw in the Apple M1 computers.
The driver is based on Corellium's driver [1], and has been rebased and cleaned up.
The bindings are in Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml

This has been tested with out-of-tree patches for the keyboard on the Macbook
Air, it has also been tested with i2c for the USB PD driver and PCIe (all
out-of-tree drivers currently).

I left two defines at the top 'USE_PINMUX_GENERIC_FN' and
'USE_PINCTRL_GENERIC_FN', I wasn't sure if I should use the generic functions for
getting the groups/functions, so I left both approaches in and will remove one
of them for the next version! Which approach should I use?

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

There is also a branch which contains all the commits as I was
developing here (and keyboard drivers):
  https://gitlab.arm.com/linux-arm/jg-open/-/commits/m1-keyboard

I look forward to feedback!

Thanks,
Joey

note: I'm sending this from my arm work e-mail address, however it was done on
personal time.

note2: 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_v1_clock

[1]
https://github.com/corellium/linux-m1/blob/d5ec2a737e64de23a21025f9eddc554588deb23f/drivers/pinctrl/pinctrl-apple-gpio.c

Stan Skowronek (1):
  pinctrl: add pinctrl/GPIO driver for Apple SoCs

 MAINTAINERS                          |   1 +
 drivers/pinctrl/Kconfig              |  13 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c | 652 +++++++++++++++++++++++++++
 4 files changed, 667 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

-- 
2.17.1

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

[Joey Gouly]

From: Stan Skowronek <stan@corellium.com>

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

Co-authored-by: Joey Gouly <joey.gouly@arm.com>
Signed-off-by: Stan Skowronek <stan@corellium.com>
Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 MAINTAINERS                          |   1 +
 drivers/pinctrl/Kconfig              |  13 +
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-apple-gpio.c | 652 +++++++++++++++++++++++++++
 4 files changed, 667 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c

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
 
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 31921108e456..7269614c85a1 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -31,6 +31,19 @@ config DEBUG_PINCTRL
 	help
 	  Say Y here to add some extra checks and diagnostics to PINCTRL calls.
 
+config PINCTRL_APPLE_GPIO
+	bool "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.
+
 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..a27d21682f3a
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -0,0 +1,652 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Apple SoC pinctrl+GPIO+external IRQ driver
+ *
+ * Copyright (C) 2021 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.
+ */
+
+#define USE_PINMUX_GENERIC_FN 1
+#define USE_PINCTRL_GENERIC_FN 1
+
+#include <dt-bindings/pinctrl/apple.h>
+#include <linux/clk.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 "pinctrl-utils.h"
+#include "core.h"
+#include "pinmux.h"
+
+struct apple_gpio_pincfg {
+	uint8_t irqtype;
+	uint8_t stat;
+};
+
+#define PINCFG_STAT_OUTVAL	0x01
+#define PINCFG_STAT_OUTEN	0x02
+#define PINCFG_STAT_PERIPH	0x20
+#define PINCFG_STAT_IRQEN	0x80
+
+struct apple_gpio_pinctrl {
+	struct device *dev;
+	struct pinctrl_dev *pctldev;
+
+	unsigned int npins;
+	struct pinctrl_pin_desc *pins;
+	struct apple_gpio_pincfg *pin_cfgs;
+	const char **pin_names;
+	unsigned *pin_nums;
+
+	void __iomem *base;
+	unsigned int nirqgrps;
+
+	struct pinctrl_desc pinctrl_desc;
+	struct gpio_chip gpio_chip;
+	struct irq_chip irq_chip;
+};
+
+#define REG_GPIO(x)		(4 * (x))
+#define  REG_GPIOx_DATA		BIT(0)
+#define  REG_GPIOx_MODE_MASK	GENMASK(3, 1)
+#define REG_GPIOx_OUT	(1 << REG_GPIOx_DATA)
+#define REG_GPIOx_IN_IRQ_HI	(2 << REG_GPIOx_DATA)
+#define REG_GPIOx_IN_IRQ_LO	(3 << REG_GPIOx_DATA)
+#define REG_GPIOx_IN_IRQ_UP	(4 << REG_GPIOx_DATA)
+#define REG_GPIOx_IN_IRQ_DN	(5 << REG_GPIOx_DATA)
+#define REG_GPIOx_IN_IRQ_ANY	(6 << REG_GPIOx_DATA)
+#define REG_GPIOx_IN_IRQ_OFF	(7 << REG_GPIOx_DATA)
+#define  REG_GPIOx_PERIPH	BIT(5)
+#define  REG_GPIOx_CFG_DONE	BIT(9)
+#define  REG_GPIOx_GRP_MASK	GENMASK(18, 16)
+#define REG_IRQ(g,x)		(0x800 + 0x40 * (g) + 4 * ((x) >> 5))
+
+static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl, unsigned pin, uint32_t clr, uint32_t set)
+{
+	void __iomem *ppin = pctl->base + pin * 4;
+	uint32_t prev, cfg;
+
+	prev = readl(ppin);
+	cfg = (prev & ~clr) | set;
+
+	if(!(prev & REG_GPIOx_CFG_DONE))
+		writel(cfg & ~REG_GPIOx_CFG_DONE, ppin);
+	writel(cfg, ppin);
+}
+
+static void apple_gpio_refresh_reg(struct apple_gpio_pinctrl *pctl, unsigned pin)
+{
+	struct apple_gpio_pincfg *pincfg = &pctl->pin_cfgs[pin];
+
+	int stat = pincfg->stat;
+	int outval = (stat & PINCFG_STAT_OUTVAL);
+
+	int clear = REG_GPIOx_MODE_MASK | REG_GPIOx_DATA;
+	int set = REG_GPIOx_CFG_DONE | outval;
+
+	if (stat & PINCFG_STAT_PERIPH) {
+		set |= REG_GPIOx_PERIPH;
+	} else {
+		clear |= REG_GPIOx_PERIPH;
+		if (stat & PINCFG_STAT_OUTEN)
+			set |= REG_GPIOx_OUT;
+		else if (stat & PINCFG_STAT_IRQEN)
+			set |= pincfg->irqtype;
+		else
+			set |= REG_GPIOx_IN_IRQ_OFF;
+	}
+
+	apple_gpio_set_reg(pctl, pin, clear, set);
+}
+
+static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl, unsigned pin)
+{
+	return readl(pctl->base + pin * 4);
+}
+
+static void apple_gpio_init_reg(struct apple_gpio_pinctrl *pctl, unsigned pin)
+{
+	struct apple_gpio_pincfg *pincfg = &pctl->pin_cfgs[pin];
+	uint32_t reg = apple_gpio_get_reg(pctl, pin);
+
+	pincfg->irqtype = 0;
+	if(reg & REG_GPIOx_PERIPH) {
+		pincfg->stat = PINCFG_STAT_PERIPH;
+	} else if((reg & REG_GPIOx_MODE_MASK) == REG_GPIOx_OUT) {
+		pincfg->stat = PINCFG_STAT_OUTEN | (reg & PINCFG_STAT_OUTVAL);
+	} else if((reg & REG_GPIOx_MODE_MASK) == REG_GPIOx_IN_IRQ_OFF || !(reg & REG_GPIOx_MODE_MASK)) {
+		pincfg->stat = 0;
+	} else {
+		pincfg->irqtype = reg & REG_GPIOx_MODE_MASK;
+		pincfg->stat = PINCFG_STAT_IRQEN;
+	}
+}
+
+/* Pin controller functions */
+
+#if !USE_PINCTRL_GENERIC_FN
+static int apple_gpio_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->npins;
+}
+
+static const char *apple_gpio_pinctrl_get_group_name(struct pinctrl_dev *pctldev, unsigned group)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pctl->pins[group].name;
+}
+
+static int apple_gpio_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, unsigned group, const unsigned **pins, unsigned *num_pins)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = &pctl->pin_nums[group];
+	*num_pins = 1;
+
+	return 0;
+}
+#endif
+
+#if !USE_PINMUX_GENERIC_FN
+static const char *apple_gpio_pinmux_get_function_name(struct pinctrl_dev *pctldev, unsigned func);
+static int apple_gpio_pinmux_get_functions_count(struct pinctrl_dev *pctldev);
+#endif
+
+static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
+				 struct device_node *np_config,
+				 struct pinctrl_map **map, unsigned *num_maps)
+{
+	unsigned reserved_maps;
+	struct apple_gpio_pinctrl *pctl;
+	u32 pinfunc, pin, func;
+	int num_pins, i;
+	const char* group_name;
+	const char* function_name;
+	struct device_node *node = np_config;
+	int ret = 0;
+
+	*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 -EINVAL;
+	}
+
+	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 USE_PINMUX_GENERIC_FN
+		if (func >=pinmux_generic_get_function_count(pctldev)) {
+#else
+		if (func >= apple_gpio_pinmux_get_functions_count(pctldev)) {
+#endif
+			ret = -EINVAL;
+			goto free_map;
+		}
+
+#if USE_PINCTRL_GENERIC_FN
+		group_name = pinctrl_generic_get_group_name(pctldev, pin);
+#else
+		group_name = apple_gpio_pinctrl_get_group_name(pctldev, pin);
+#endif
+
+#if USE_PINMUX_GENERIC_FN
+		function_name = pinmux_generic_get_function_name(pctl->pctldev, func);
+#else
+		function_name = apple_gpio_pinmux_get_function_name(pctl->pctldev, func);
+#endif
+
+		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;
+	}
+
+	return 0;
+}
+
+static const struct pinctrl_ops apple_gpio_pinctrl_ops = {
+#if USE_PINCTRL_GENERIC_FN
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name = pinctrl_generic_get_group_name,
+	.get_group_pins = pinctrl_generic_get_group_pins,
+#else
+	.get_groups_count = apple_gpio_pinctrl_get_groups_count,
+	.get_group_name = apple_gpio_pinctrl_get_group_name,
+	.get_group_pins = apple_gpio_pinctrl_get_group_pins,
+#endif
+	.dt_node_to_map = apple_gpio_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+/* Pin multiplexer functions */
+
+#if !USE_PINMUX_GENERIC_FN
+static int apple_gpio_pinmux_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return 2;
+}
+
+static const char *apple_gpio_pinmux_get_function_name(struct pinctrl_dev *pctldev, unsigned func)
+{
+	return func ? "periph" : "gpio";
+}
+
+static int apple_gpio_pinmux_get_function_groups(struct pinctrl_dev *pctldev, unsigned func, const char * const **groups, unsigned * const num_groups)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pctl->pin_names;
+	*num_groups = pctl->npins;
+	return 0;
+}
+#endif
+
+static int apple_gpio_pinmux_enable(struct pinctrl_dev *pctldev, unsigned func, unsigned group)
+{
+	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+
+	if(func)
+		pctl->pin_cfgs[group].stat |= PINCFG_STAT_PERIPH;
+	else
+		pctl->pin_cfgs[group].stat &= ~PINCFG_STAT_PERIPH;
+	apple_gpio_refresh_reg(pctl, group);
+
+	return 0;
+}
+
+static const struct pinmux_ops apple_gpio_pinmux_ops = {
+#if USE_PINMUX_GENERIC_FN
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+#else
+	.get_functions_count = apple_gpio_pinmux_get_functions_count,
+	.get_function_name = apple_gpio_pinmux_get_function_name,
+	.get_function_groups = apple_gpio_pinmux_get_function_groups,
+#endif
+	.set_mux = apple_gpio_pinmux_enable,
+	.strict = true,
+};
+
+/* GPIO chip functions */
+
+static int apple_gpio_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	return !(pctl->pin_cfgs[offset].stat & PINCFG_STAT_OUTEN) ?
+			GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int apple_gpio_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+	uint32_t reg;
+
+	reg = apple_gpio_get_reg(pctl, offset);
+	return !!(reg & REG_GPIOx_DATA);
+}
+
+static void apple_gpio_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	if(value)
+		pctl->pin_cfgs[offset].stat |= PINCFG_STAT_OUTVAL;
+	else
+		pctl->pin_cfgs[offset].stat &= ~PINCFG_STAT_OUTVAL;
+	apple_gpio_refresh_reg(pctl, offset);
+}
+
+static int apple_gpio_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	pctl->pin_cfgs[offset].stat &= ~PINCFG_STAT_OUTEN;
+	apple_gpio_refresh_reg(pctl, offset);
+	return 0;
+}
+
+static int apple_gpio_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
+{
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
+
+	int clear = PINCFG_STAT_PERIPH;
+	int set = PINCFG_STAT_OUTEN;
+
+	if (value)
+		set |= PINCFG_STAT_OUTVAL;
+	else
+		clear |= PINCFG_STAT_OUTVAL;
+
+	pctl->pin_cfgs[offset].stat &= ~clear;
+	pctl->pin_cfgs[offset].stat |= set;
+
+	apple_gpio_refresh_reg(pctl, offset);
+	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 irqgrp = FIELD_GET(REG_GPIOx_GRP_MASK, apple_gpio_get_reg(pctl, data->hwirq));
+
+	writel(1u << (data->hwirq & 31), pctl->base + REG_IRQ(irqgrp, data->hwirq));
+}
+
+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));
+
+	pctl->pin_cfgs[data->hwirq].stat &= ~PINCFG_STAT_IRQEN;
+	apple_gpio_refresh_reg(pctl, data->hwirq);
+}
+
+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));
+
+	pctl->pin_cfgs[data->hwirq].stat |= PINCFG_STAT_IRQEN;
+	apple_gpio_refresh_reg(pctl, data->hwirq);
+}
+
+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);
+	unsigned irqgrp = 0;
+
+	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK, FIELD_PREP(REG_GPIOx_GRP_MASK, irqgrp));
+
+	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));
+
+	switch(type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_UP;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_DN;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_ANY;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_HI;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_LO;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	apple_gpio_refresh_reg(pctl, data->hwirq);
+
+	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 gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(gc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned irqgrp, pinh, pinl;
+	unsigned long pending;
+	unsigned int parent = irq_desc_get_irq(desc);
+
+	for (irqgrp = 0; irqgrp < pctl->nirqgrps; ++irqgrp) {
+		if (parent == gc->irq.parents[irqgrp])
+			break;
+	}
+
+	WARN_ON(irqgrp == pctl->nirqgrps);
+
+	chained_irq_enter(chip, desc);
+	for(pinh=0; pinh<pctl->npins; pinh+=32) {
+		pending = readl(pctl->base + REG_IRQ(irqgrp, pinh));
+		for_each_set_bit(pinl, &pending, 32)
+			generic_handle_irq(irq_linear_revmap(gc->irq.domain, pinh + pinl));
+	}
+	chained_irq_exit(chip, desc);
+}
+
+/* Probe & register */
+
+static int apple_gpio_gpio_register(struct apple_gpio_pinctrl *pctl)
+{
+	struct device_node *node = pctl->dev->of_node;
+	struct gpio_irq_chip *girq;
+	int i, ret = 0;
+
+	if(!of_find_property(node, "gpio-controller", NULL)) {
+		dev_err(pctl->dev, "Apple GPIO must have 'gpio-controller' property.\n");
+		return -ENODEV;
+	}
+
+	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->npins;
+	pctl->gpio_chip.parent = pctl->dev;
+	pctl->gpio_chip.of_node = node;
+
+	if (of_find_property(node, "interrupt-controller", NULL)) {
+		ret = platform_irq_count(to_platform_device(pctl->dev));
+		if(ret < 0)
+			return ret;
+
+		pctl->nirqgrps = ret;
+
+		pctl->irq_chip.name = dev_name(pctl->dev);
+		pctl->irq_chip.irq_startup = apple_gpio_gpio_irq_startup;
+		pctl->irq_chip.irq_ack = apple_gpio_gpio_irq_ack;
+		pctl->irq_chip.irq_mask = apple_gpio_gpio_irq_mask;
+		pctl->irq_chip.irq_unmask = apple_gpio_gpio_irq_unmask;
+		pctl->irq_chip.irq_set_type = apple_gpio_gpio_irq_set_type;
+
+		girq = &pctl->gpio_chip.irq;
+		girq->chip = &pctl->irq_chip;
+		girq->parent_handler = apple_gpio_gpio_irq_handler;
+		girq->num_parents = pctl->nirqgrps;
+
+		girq->parents = devm_kmalloc_array(pctl->dev, pctl->nirqgrps,
+			sizeof(girq->parents[0]), GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+
+		for(i = 0; i < pctl->nirqgrps; i++) {
+			ret = platform_get_irq(to_platform_device(pctl->dev), i);
+			if(ret < 0) {
+				if(ret != -EPROBE_DEFER)
+					dev_err(pctl->dev, "Failed to map IRQ %d (%d).\n", i, ret);
+				return ret;
+			}
+			girq->parents[i] = ret;
+		}
+
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_level_irq;
+	}
+
+	ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
+	if(ret < 0) {
+		dev_err(pctl->dev, "Failed to add GPIO chip (%d).\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id apple_gpio_pinctrl_of_match[] = {
+	{ .compatible = "apple,t8103-pinctrl", },
+	{ },
+};
+
+static int apple_gpio_pinctrl_probe(struct platform_device *pdev)
+{
+	struct apple_gpio_pinctrl *pctl;
+	struct of_phandle_args pinspec;
+	int res;
+	unsigned pin_base, i;
+
+	pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+	if(!pctl)
+		return -ENOMEM;
+	pctl->dev = &pdev->dev;
+	dev_set_drvdata(&pdev->dev, pctl);
+
+	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];
+
+	pctl->pins = devm_kmalloc_array(&pdev->dev, pctl->npins, sizeof(pctl->pins[0]), GFP_KERNEL);
+	if(!pctl->pins)
+		return -ENOMEM;
+	pctl->pin_names = devm_kmalloc_array(&pdev->dev, pctl->npins, sizeof(pctl->pin_names[0]), GFP_KERNEL);
+	if(!pctl->pin_names)
+		return -ENOMEM;
+	pctl->pin_nums = devm_kmalloc_array(&pdev->dev, pctl->npins, sizeof(pctl->pin_nums[0]), GFP_KERNEL);
+	if(!pctl->pin_nums)
+		return -ENOMEM;
+	pctl->pin_cfgs = devm_kmalloc_array(&pdev->dev, pctl->npins, sizeof(pctl->pin_cfgs[0]), GFP_KERNEL);
+	if(!pctl->pin_cfgs)
+		return -ENOMEM;
+
+	pctl->base = devm_platform_ioremap_resource(pdev, 0);
+	if(IS_ERR(pctl->base))
+		return PTR_ERR(pctl->base);
+
+	for(i=0; i<pctl->npins; i++) {
+		apple_gpio_init_reg(pctl, i);
+
+		pctl->pins[i].number = i + pin_base;
+		pctl->pins[i].name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "PIN%u", i + pin_base);
+		pctl->pins[i].drv_data = pctl;
+		pctl->pin_names[i] = pctl->pins[i].name;
+		pctl->pin_nums[i] = i + pin_base;
+	}
+
+	pctl->pinctrl_desc.name = dev_name(pctl->dev);
+	pctl->pinctrl_desc.pins = pctl->pins;
+	pctl->pinctrl_desc.npins = pctl->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)) {
+		dev_err(&pdev->dev, "Failed to register pinctrl device.\n");
+		return PTR_ERR(pctl->pctldev);
+	}
+
+#if USE_PINCTRL_GENERIC_FN
+	for (i = 0; i < pctl->npins; i++) {
+		res = pinctrl_generic_add_group(pctl->pctldev, pctl->pins[i].name,
+						pctl->pin_nums + i, 1, pctl);
+		if (res < 0) {
+			dev_err(pctl->dev, "Failed to register group.");
+			return res;
+		}
+	}
+#endif
+
+#if USE_PINMUX_GENERIC_FN
+	res = pinmux_generic_add_function(pctl->pctldev, "gpio", pctl->pin_names, pctl->npins, pctl);
+
+	if (res < 0) {
+		dev_err(pctl->dev, "Failed to register function.");
+		return res;
+	}
+
+	res = pinmux_generic_add_function(pctl->pctldev, "periph", pctl->pin_names, pctl->npins, pctl);
+
+	if (res < 0) {
+		dev_err(pctl->dev, "Failed to register function.");
+		return res;
+	}
+#endif
+
+	return apple_gpio_gpio_register(pctl);
+}
+
+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 v1 1/1] pinctrl: add pinctrl/GPIO driver for Apple SoCs

[Andy Shevchenko]

On Tue, Sep 21, 2021 at 11:29:56PM +0100, Joey Gouly wrote:
> From: Stan Skowronek <stan@corellium.com>
> 
> This driver adds support for the pinctrl / GPIO hardware found
> on some Apple SoCs.
> 
> Co-authored-by: Joey Gouly <joey.gouly@arm.com>

Have you read Submitting Patches document? Please read and act accordingly.

> Signed-off-by: Stan Skowronek <stan@corellium.com>
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>

...

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

...

> +config PINCTRL_APPLE_GPIO
> +	bool "Apple SoC GPIO pin controller driver"

Why not module?

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

After fixing above, put a standard sentence on how the module will be called.

...

> +#define USE_PINMUX_GENERIC_FN 1
> +#define USE_PINCTRL_GENERIC_FN 1

Why you left them if they are hard coded?

...

> +#define PINCFG_STAT_OUTVAL	0x01
> +#define PINCFG_STAT_OUTEN	0x02
> +#define PINCFG_STAT_PERIPH	0x20
> +#define PINCFG_STAT_IRQEN	0x80

BIT() ?

...

> +	struct apple_gpio_pincfg *pin_cfgs;

Can you use the data structures from generic pin control?

...

> +#define REG_GPIOx_OUT	(1 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_HI	(2 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_LO	(3 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_UP	(4 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_DN	(5 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_ANY	(6 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_OFF	(7 << REG_GPIOx_DATA)

Easy to read and understand if the shift is a plain number.

..

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

...

> +#if USE_PINMUX_GENERIC_FN
> +		if (func >=pinmux_generic_get_function_count(pctldev)) {

Missed space.
I noticed more places, so please carefully check the style.

> +#else
> +		if (func >= apple_gpio_pinmux_get_functions_count(pctldev)) {
> +#endif
> +			ret = -EINVAL;
> +			goto free_map;
> +		}

...

> +static int apple_gpio_pinmux_get_function_groups(struct pinctrl_dev *pctldev, unsigned func, const char * const **groups, unsigned * const num_groups)

Seems quite too long line.

...

> +	int i, ret = 0;

Don't hid ret assignment here. It's error prone.

...

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

...

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

> +		ret = platform_irq_count(to_platform_device(pctl->dev));
> +		if(ret < 0)
> +			return ret;
> +
> +		pctl->nirqgrps = ret;
> +
> +		pctl->irq_chip.name = dev_name(pctl->dev);
> +		pctl->irq_chip.irq_startup = apple_gpio_gpio_irq_startup;
> +		pctl->irq_chip.irq_ack = apple_gpio_gpio_irq_ack;
> +		pctl->irq_chip.irq_mask = apple_gpio_gpio_irq_mask;
> +		pctl->irq_chip.irq_unmask = apple_gpio_gpio_irq_unmask;
> +		pctl->irq_chip.irq_set_type = apple_gpio_gpio_irq_set_type;
> +
> +		girq = &pctl->gpio_chip.irq;
> +		girq->chip = &pctl->irq_chip;
> +		girq->parent_handler = apple_gpio_gpio_irq_handler;
> +		girq->num_parents = pctl->nirqgrps;
> +
> +		girq->parents = devm_kmalloc_array(pctl->dev, pctl->nirqgrps,
> +			sizeof(girq->parents[0]), GFP_KERNEL);

sizeof(*...)

> +		if (!girq->parents)
> +			return -ENOMEM;
> +
> +		for(i = 0; i < pctl->nirqgrps; i++) {
> +			ret = platform_get_irq(to_platform_device(pctl->dev), i);
> +			if(ret < 0) {

> +				if(ret != -EPROBE_DEFER)
> +					dev_err(pctl->dev, "Failed to map IRQ %d (%d).\n", i, ret);
> +				return ret;

return dev_err_probe(...);

> +			}
> +			girq->parents[i] = ret;
> +		}
> +
> +		girq->default_type = IRQ_TYPE_NONE;
> +		girq->handler = handle_level_irq;
> +	}

...

> +	ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
> +	if(ret < 0) {
> +		dev_err(pctl->dev, "Failed to add GPIO chip (%d).\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;

Simple `return devm_gpiochip_add_data(...);` will do the job.

...

> +static const struct of_device_id apple_gpio_pinctrl_of_match[] = {
> +	{ .compatible = "apple,t8103-pinctrl", },

> +	{ },

No comma for the array termination.

> +};

...

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

...

> +		pctl->pins[i].number = i + pin_base;
> +		pctl->pins[i].name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "PIN%u", i + pin_base);
> +		pctl->pins[i].drv_data = pctl;
> +		pctl->pin_names[i] = pctl->pins[i].name;
> +		pctl->pin_nums[i] = i + pin_base;

Try to reduce Yoda style, i.e. use `pin_base + i`.

...

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

> +

Redundant blank line.

> +module_platform_driver(apple_gpio_pinctrl_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Joey Gouly]

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

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

[Andy Shevchenko]

On Sat, Sep 25, 2021 at 4:45 PM Joey Gouly <joey.gouly@arm.com> 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

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

[Linus Walleij]

On Sun, Sep 26, 2021 at 7:09 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Sep 25, 2021 at 4:45 PM Joey Gouly <joey.gouly@arm.com> wrote:
> > On Wed, Sep 22, 2021 at 10:20:39AM +0300, Andy Shevchenko wrote:

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

I don't really see this as necessary, we don't need to check everything.
Not that it hurts either, so I would say maintainer preference?

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

I don't remember right now how the review was going on the
mentioned drivers.

I did imagine that of_gpiochip_add_pin_range() would be the
sole parser of this, and drivers would then use the infrastructure
for any necessary cross-reference between the subsystems,
not second-code it.

What is it that you really need to do here?

I think npins should be known from the compatible (we know that
this version of the SoC has so and so many pins) and the base
should always be 0? It's not like we have several pin controllers
of this type in the SoC is it?

Yours,
Linus Walleij

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

[Sven Peter]

Hi,

On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:
> I think npins should be known from the compatible (we know that
> this version of the SoC has so and so many pins) and the base
> should always be 0? It's not like we have several pin controllers
> of this type in the SoC is it?

I've just checked: Looks like there are four different pin controllers of
this type with a different number of pins each in Apple's device tree for
the M1.


Best,


Sven

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

[Linus Walleij]

On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:

> > I think npins should be known from the compatible (we know that
> > this version of the SoC has so and so many pins) and the base
> > should always be 0? It's not like we have several pin controllers
> > of this type in the SoC is it?
>
> I've just checked: Looks like there are four different pin controllers of
> this type with a different number of pins each in Apple's device tree for
> the M1.

So we need to understand this hardware: is this something like
south/north/east/west, so the pins are oriented around the chip?

I would suspect they should then be compatibles:
apple,t8103-pinctrl-north, apple,t8103-pinctrl
apple,t8103-pinctrl-south, apple,t8103-pinctrl
apple,t8103-pinctrl-west, apple,t8103-pinctrl
apple,t8103-pinctrl-east, apple,t8103-pinctrl

going from specific to general signifying that we know which one
we are dealing with and then we know the npins etc.

This also gives a normal grouping of functions associated with
pins and the portion of the SoC, see for
example drivers/pinctrl/intel/pinctrl-broxton.c.

This shows that it might have been a bad idea to define the
pins as opaque, because now that is hiding the fact that
these pins are grouped in four distinct sets.
APPLE_PINMUX(pin, func)

Should rather have been APPLE_PINMUX(cardinal, pin, func)
where cardinal would be 0..3 for north, south, west, east.

This is a bit of guessing actually, but we should definitely
try to make the driver reflect the reality and not over-generalize.
If these four blocks in the SoC are different, they should have
different compatible strings, because they are not, by
definition, compatible.

Yours,
Linus Walleij

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

[Sven Peter]

On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
> On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
>> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:
>
>> > I think npins should be known from the compatible (we know that
>> > this version of the SoC has so and so many pins) and the base
>> > should always be 0? It's not like we have several pin controllers
>> > of this type in the SoC is it?
>>
>> I've just checked: Looks like there are four different pin controllers of
>> this type with a different number of pins each in Apple's device tree for
>> the M1.
>
> So we need to understand this hardware: is this something like
> south/north/east/west, so the pins are oriented around the chip?
>
> I would suspect they should then be compatibles:
> apple,t8103-pinctrl-north, apple,t8103-pinctrl
> apple,t8103-pinctrl-south, apple,t8103-pinctrl
> apple,t8103-pinctrl-west, apple,t8103-pinctrl
> apple,t8103-pinctrl-east, apple,t8103-pinctrl
>
> going from specific to general signifying that we know which one
> we are dealing with and then we know the npins etc.

Apple calls those four controllers "gpio", "nub-gpio", "smc-gpio"
and "aop-gpio". SMC is their system management controller and AOP
is their "always-on processor". No idea what "nub-gpio" is.

>
> This also gives a normal grouping of functions associated with
> pins and the portion of the SoC, see for
> example drivers/pinctrl/intel/pinctrl-broxton.c.
>
> This shows that it might have been a bad idea to define the
> pins as opaque, because now that is hiding the fact that
> these pins are grouped in four distinct sets.
> APPLE_PINMUX(pin, func)
>
> Should rather have been APPLE_PINMUX(cardinal, pin, func)
> where cardinal would be 0..3 for north, south, west, east.
>
> This is a bit of guessing actually, but we should definitely
> try to make the driver reflect the reality and not over-generalize.
> If these four blocks in the SoC are different, they should have
> different compatible strings, because they are not, by
> definition, compatible.

I'd prefer to have a single compatible and get the npins from some
property and I don't think that's necessarily over-generalizing.
AFAICT Apple has been using the exact same MMIO interface for years
and I'd expect them to continue using it in the future. The only thing
that seems to change is the number of pins available and their assignment.
If we just have a single compatible we can likely support the M1X/2 or
however Apple calls the next SoCs with just a simple DTB change without
touching any driver code.



Best,

Sven

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

[Andy Shevchenko]

On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@svenpeter.dev> wrote:
> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
> > On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
> >> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:
> >
> >> > I think npins should be known from the compatible (we know that
> >> > this version of the SoC has so and so many pins) and the base
> >> > should always be 0? It's not like we have several pin controllers
> >> > of this type in the SoC is it?
> >>
> >> I've just checked: Looks like there are four different pin controllers of
> >> this type with a different number of pins each in Apple's device tree for
> >> the M1.
> >
> > So we need to understand this hardware: is this something like
> > south/north/east/west, so the pins are oriented around the chip?
> >
> > I would suspect they should then be compatibles:
> > apple,t8103-pinctrl-north, apple,t8103-pinctrl
> > apple,t8103-pinctrl-south, apple,t8103-pinctrl
> > apple,t8103-pinctrl-west, apple,t8103-pinctrl
> > apple,t8103-pinctrl-east, apple,t8103-pinctrl
> >
> > going from specific to general signifying that we know which one
> > we are dealing with and then we know the npins etc.
>
> Apple calls those four controllers "gpio", "nub-gpio", "smc-gpio"
> and "aop-gpio". SMC is their system management controller and AOP
> is their "always-on processor". No idea what "nub-gpio" is.

It's similar to what we have in Baytrail/Cherrytrail.
AOP -> SUS
SMC -> ...

nub is probably related to some type of hub (or maybe simple typo, or
typo on purpose?).

> > This also gives a normal grouping of functions associated with
> > pins and the portion of the SoC, see for
> > example drivers/pinctrl/intel/pinctrl-broxton.c.
> >
> > This shows that it might have been a bad idea to define the
> > pins as opaque, because now that is hiding the fact that
> > these pins are grouped in four distinct sets.
> > APPLE_PINMUX(pin, func)
> >
> > Should rather have been APPLE_PINMUX(cardinal, pin, func)
> > where cardinal would be 0..3 for north, south, west, east.
> >
> > This is a bit of guessing actually, but we should definitely
> > try to make the driver reflect the reality and not over-generalize.
> > If these four blocks in the SoC are different, they should have
> > different compatible strings, because they are not, by
> > definition, compatible.
>
> I'd prefer to have a single compatible and get the npins from some
> property and I don't think that's necessarily over-generalizing.
> AFAICT Apple has been using the exact same MMIO interface for years
> and I'd expect them to continue using it in the future. The only thing
> that seems to change is the number of pins available and their assignment.
> If we just have a single compatible we can likely support the M1X/2 or
> however Apple calls the next SoCs with just a simple DTB change without
> touching any driver code.

Hmm... Dunno the details, but at least AOP GPIO is definitely ca[able
of waking a system from a deep sleep (that's what SUS == suspend do on
Intel). Haven't checked if you implemented ->irq_set_wake() callbacks,
though.

And I don't know if the pin's in the rest of the GPIO blocks have the
wake-source capable pins. Also I don't know if it's fine to have one
compatible string if you really know that the difference is the amount
of pins and nothing else (like crucial properties being changed).

-- 
With Best Regards,
Andy Shevchenko

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

[Sven Peter]

Hi Andy,

On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:
> On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@svenpeter.dev> wrote:
>> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
>> > On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
>> >> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:
>> >
>> >> > I think npins should be known from the compatible (we know that
>> >> > this version of the SoC has so and so many pins) and the base
>> >> > should always be 0? It's not like we have several pin controllers
>> >> > of this type in the SoC is it?
>> >>
>> >> I've just checked: Looks like there are four different pin controllers of
>> >> this type with a different number of pins each in Apple's device tree for
>> >> the M1.
>> >
>> > So we need to understand this hardware: is this something like
>> > south/north/east/west, so the pins are oriented around the chip?
>> >
>> > I would suspect they should then be compatibles:
>> > apple,t8103-pinctrl-north, apple,t8103-pinctrl
>> > apple,t8103-pinctrl-south, apple,t8103-pinctrl
>> > apple,t8103-pinctrl-west, apple,t8103-pinctrl
>> > apple,t8103-pinctrl-east, apple,t8103-pinctrl
>> >
>> > going from specific to general signifying that we know which one
>> > we are dealing with and then we know the npins etc.
>>
>> Apple calls those four controllers "gpio", "nub-gpio", "smc-gpio"
>> and "aop-gpio". SMC is their system management controller and AOP
>> is their "always-on processor". No idea what "nub-gpio" is.
>
> It's similar to what we have in Baytrail/Cherrytrail.
> AOP -> SUS
> SMC -> ...

Interesting! I'll take a look at those.
 
>
> nub is probably related to some type of hub (or maybe simple typo, or
> typo on purpose?).
>
>> > This also gives a normal grouping of functions associated with
>> > pins and the portion of the SoC, see for
>> > example drivers/pinctrl/intel/pinctrl-broxton.c.
>> >
>> > This shows that it might have been a bad idea to define the
>> > pins as opaque, because now that is hiding the fact that
>> > these pins are grouped in four distinct sets.
>> > APPLE_PINMUX(pin, func)
>> >
>> > Should rather have been APPLE_PINMUX(cardinal, pin, func)
>> > where cardinal would be 0..3 for north, south, west, east.
>> >
>> > This is a bit of guessing actually, but we should definitely
>> > try to make the driver reflect the reality and not over-generalize.
>> > If these four blocks in the SoC are different, they should have
>> > different compatible strings, because they are not, by
>> > definition, compatible.
>>
>> I'd prefer to have a single compatible and get the npins from some
>> property and I don't think that's necessarily over-generalizing.
>> AFAICT Apple has been using the exact same MMIO interface for years
>> and I'd expect them to continue using it in the future. The only thing
>> that seems to change is the number of pins available and their assignment.
>> If we just have a single compatible we can likely support the M1X/2 or
>> however Apple calls the next SoCs with just a simple DTB change without
>> touching any driver code.
>
> Hmm... Dunno the details, but at least AOP GPIO is definitely ca[able
> of waking a system from a deep sleep (that's what SUS == suspend do on
> Intel). Haven't checked if you implemented ->irq_set_wake() callbacks,
> though.

I don't think Joey implemented the set_wake callback because we didn't
even consider that the AOP GPIOs might be able to wake the system from
deep sleep. I'll see if I can figure out some details about that though.

>
> And I don't know if the pin's in the rest of the GPIO blocks have the
> wake-source capable pins. Also I don't know if it's fine to have one
> compatible string if you really know that the difference is the amount
> of pins and nothing else (like crucial properties being changed).

Yeah, I don't know either. Another thing we could do is have the base
compatible just support the maximum number of pins supported by the MMIO
space and only limit that (and possibly add wake-capable pins or other
different properties if there are any) for the more specific ones.


Best,


Sven

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

[Andy Shevchenko]

+Cc: Hans (just for a bit offtopic comment below)

On Mon, Sep 27, 2021 at 8:46 AM Sven Peter <sven@svenpeter.dev> wrote:
> On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:
> > On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@svenpeter.dev> wrote:
> >> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
> >> > On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
> >> >> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:

...

> >> I'd prefer to have a single compatible and get the npins from some
> >> property and I don't think that's necessarily over-generalizing.
> >> AFAICT Apple has been using the exact same MMIO interface for years
> >> and I'd expect them to continue using it in the future. The only thing
> >> that seems to change is the number of pins available and their assignment.
> >> If we just have a single compatible we can likely support the M1X/2 or
> >> however Apple calls the next SoCs with just a simple DTB change without
> >> touching any driver code.
> >
> > Hmm... Dunno the details, but at least AOP GPIO is definitely ca[able
> > of waking a system from a deep sleep (that's what SUS == suspend do on
> > Intel). Haven't checked if you implemented ->irq_set_wake() callbacks,
> > though.
>
> I don't think Joey implemented the set_wake callback because we didn't
> even consider that the AOP GPIOs might be able to wake the system from
> deep sleep. I'll see if I can figure out some details about that though.

I have checked Intel drivers and above mentioned do not implement
->irq_set_wake() callback. Hmm... Maybe Hans can share his thoughts
why it's so
(note, the Skylake and newest are all based on pinctrl-intel.c which
implements it. So does Merrifield) and if we also need to consider
adding it.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 9/27/21 11:00 AM, Andy Shevchenko wrote:
> +Cc: Hans (just for a bit offtopic comment below)
> 
> On Mon, Sep 27, 2021 at 8:46 AM Sven Peter <sven@svenpeter.dev> wrote:
>> On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:
>>> On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@svenpeter.dev> wrote:
>>>> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
>>>>> On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
>>>>>> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:
> 
> ...
> 
>>>> I'd prefer to have a single compatible and get the npins from some
>>>> property and I don't think that's necessarily over-generalizing.
>>>> AFAICT Apple has been using the exact same MMIO interface for years
>>>> and I'd expect them to continue using it in the future. The only thing
>>>> that seems to change is the number of pins available and their assignment.
>>>> If we just have a single compatible we can likely support the M1X/2 or
>>>> however Apple calls the next SoCs with just a simple DTB change without
>>>> touching any driver code.
>>>
>>> Hmm... Dunno the details, but at least AOP GPIO is definitely ca[able
>>> of waking a system from a deep sleep (that's what SUS == suspend do on
>>> Intel). Haven't checked if you implemented ->irq_set_wake() callbacks,
>>> though.
>>
>> I don't think Joey implemented the set_wake callback because we didn't
>> even consider that the AOP GPIOs might be able to wake the system from
>> deep sleep. I'll see if I can figure out some details about that though.
> 
> I have checked Intel drivers and above mentioned do not implement
> ->irq_set_wake() callback. Hmm... Maybe Hans can share his thoughts
> why it's so
> (note, the Skylake and newest are all based on pinctrl-intel.c which
> implements it. So does Merrifield) and if we also need to consider
> adding it.

Bay Trail and Cherry Trail always use suspend2idle, which means any
IRQ is a wake IRQ, since the CPU is in a S0ix (deep-idle, rather
then full suspended) state.

Drivers still need to make irq_set_irq_wake() calls
though, to avoid the IRQ code disabling the IRQ on suspend.

To allow those calls to succeed the baytrail and cherryview
pinctrl drivers set IRQCHIP_SKIP_SET_WAKE in their irqchip.flags.

There are also some more standard (non tablet targetting)
CPUs which are using the same GPIO IP block, e.g.
the Celeron N2840 uses the pinctrl-baytrail.c code but
laptops using this will typically use normal S3 suspend.

I assume that in this case configuring which IRQs are wakeup
sources is actually controlled the BIOS, since S3 suspend is
heavily BIOS assisted. I would not even be surprised if the
BIOS even completely reprograms all IRQ settings (including
IRQs left enabled) on suspend.

Regards,

Hans

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

[Andy Shevchenko]

On Mon, Sep 27, 2021 at 12:27 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/27/21 11:00 AM, Andy Shevchenko wrote:
> > On Mon, Sep 27, 2021 at 8:46 AM Sven Peter <sven@svenpeter.dev> wrote:
> >> On Sun, Sep 26, 2021, at 18:28, Andy Shevchenko wrote:
> >>> On Sun, Sep 26, 2021 at 5:36 PM Sven Peter <sven@svenpeter.dev> wrote:
> >>>> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
> >>>>> On Sun, Sep 26, 2021 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
> >>>>>> On Sun, Sep 26, 2021, at 14:48, Linus Walleij wrote:

...

> >>> Hmm... Dunno the details, but at least AOP GPIO is definitely ca[able
> >>> of waking a system from a deep sleep (that's what SUS == suspend do on
> >>> Intel). Haven't checked if you implemented ->irq_set_wake() callbacks,
> >>> though.
> >>
> >> I don't think Joey implemented the set_wake callback because we didn't
> >> even consider that the AOP GPIOs might be able to wake the system from
> >> deep sleep. I'll see if I can figure out some details about that though.
> >
> > I have checked Intel drivers and above mentioned do not implement
> > ->irq_set_wake() callback. Hmm... Maybe Hans can share his thoughts
> > why it's so
> > (note, the Skylake and newest are all based on pinctrl-intel.c which
> > implements it. So does Merrifield) and if we also need to consider
> > adding it.
>
> Bay Trail and Cherry Trail always use suspend2idle, which means any
> IRQ is a wake IRQ, since the CPU is in a S0ix (deep-idle, rather
> then full suspended) state.
>
> Drivers still need to make irq_set_irq_wake() calls
> though, to avoid the IRQ code disabling the IRQ on suspend.
>
> To allow those calls to succeed the baytrail and cherryview
> pinctrl drivers set IRQCHIP_SKIP_SET_WAKE in their irqchip.flags.
>
> There are also some more standard (non tablet targetting)
> CPUs which are using the same GPIO IP block, e.g.
> the Celeron N2840 uses the pinctrl-baytrail.c code but
> laptops using this will typically use normal S3 suspend.
>
> I assume that in this case configuring which IRQs are wakeup
> sources is actually controlled the BIOS, since S3 suspend is
> heavily BIOS assisted. I would not even be surprised if the
> BIOS even completely reprograms all IRQ settings (including
> IRQs left enabled) on suspend.

Thank you, Hans!
I will try not to forget this, but it makes me wonder if we may
collect this kind of information in some README-like file inside the
source code.

-- 
With Best Regards,
Andy Shevchenko

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

[Alyssa Rosenzweig]

> > Apple calls those four controllers "gpio", "nub-gpio", "smc-gpio"
> > and "aop-gpio". SMC is their system management controller and AOP
> > is their "always-on processor". No idea what "nub-gpio" is.
> 
> It's similar to what we have in Baytrail/Cherrytrail.
> AOP -> SUS
> SMC -> ...
> 
> nub is probably related to some type of hub (or maybe simple typo, or
> typo on purpose?).

Unlikely a typo. "nub" is an Apple term. In software, an XNU (macOS)
kernel driver has a "nub" for exposing its public API. I don't know what
it means for hardware but very likely intentional.

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

[Andy Shevchenko]

On Mon, Sep 27, 2021 at 11:46 AM Alyssa Rosenzweig <alyssa@collabora.com> wrote:

...

> > nub is probably related to some type of hub (or maybe simple typo, or
> > typo on purpose?).
>
> Unlikely a typo. "nub" is an Apple term. In software, an XNU (macOS)
> kernel driver has a "nub" for exposing its public API. I don't know what
> it means for hardware but very likely intentional.

Good to know!

So, it would be nice to have a better understanding of what it's exactly...

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Sun, Sep 26, 2021 at 4:36 PM Sven Peter <sven@svenpeter.dev> wrote:
> On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:

> > If these four blocks in the SoC are different, they should have
> > different compatible strings, because they are not, by
> > definition, compatible.
>
> I'd prefer to have a single compatible and get the npins from some
> property and I don't think that's necessarily over-generalizing.
> AFAICT Apple has been using the exact same MMIO interface for years
> and I'd expect them to continue using it in the future. The only thing
> that seems to change is the number of pins available and their assignment.
> If we just have a single compatible we can likely support the M1X/2 or
> however Apple calls the next SoCs with just a simple DTB change without
> touching any driver code.

Admittedly the word "compatible" in DT context is a bit fuzzy around the
edges. Sometimes it is more like "parameterizable" which is what you
are saying here.

I don't have a very strong opinion on it, both approaches are possible.
Being able to define new SoCs are not always helpful for developers
as the data in DT can become opaque and hard to understand if it
is too general.

Yours,
Linus Walleij

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

[Joey Gouly]

Hello!

On Tue, Sep 28, 2021 at 01:34:39AM +0200, Linus Walleij wrote:
> On Sun, Sep 26, 2021 at 4:36 PM Sven Peter <sven@svenpeter.dev> wrote:
> > On Sun, Sep 26, 2021, at 15:10, Linus Walleij wrote:
> 
> > > If these four blocks in the SoC are different, they should have
> > > different compatible strings, because they are not, by
> > > definition, compatible.
> >
> > I'd prefer to have a single compatible and get the npins from some
> > property and I don't think that's necessarily over-generalizing.
> > AFAICT Apple has been using the exact same MMIO interface for years
> > and I'd expect them to continue using it in the future. The only thing
> > that seems to change is the number of pins available and their assignment.
> > If we just have a single compatible we can likely support the M1X/2 or
> > however Apple calls the next SoCs with just a simple DTB change without
> > touching any driver code.
> 
> Admittedly the word "compatible" in DT context is a bit fuzzy around the
> edges. Sometimes it is more like "parameterizable" which is what you
> are saying here.
> 
> I don't have a very strong opinion on it, both approaches are possible.
> Being able to define new SoCs are not always helpful for developers
> as the data in DT can become opaque and hard to understand if it
> is too general.
I think we want to try avoid hardcoding big lists of GPIO pins,
especially when we don't know what most of them are used for yet.
As Sven has said it would be nice to not need to change the driver if
future SoCs have slightly different pin configurations or maybe even
extra pinctrl hardware blocks.

I have a v2 of this series mostly ready, if it's decided it's ok to go
with the current approach.

Thanks,
Joey

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

[Joey Gouly]

Hi,

On Sun, Sep 26, 2021 at 02:48:18PM +0200, Linus Walleij wrote:
> On Sun, Sep 26, 2021 at 7:09 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Sep 25, 2021 at 4:45 PM Joey Gouly <joey.gouly@arm.com> wrote:
> > > On Wed, Sep 22, 2021 at 10:20:39AM +0300, Andy Shevchenko wrote:
> 
> > > > > +   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?
> 
> I don't really see this as necessary, we don't need to check everything.
> Not that it hurts either, so I would say maintainer preference?
> 
I'm unsure if that means you (Linus), or Hector Martin as I put this
file under the "ARM/APPLE MACHINE SUPPORT" section in MAINTAINERS.

> > > > > +   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?
> 
> I don't remember right now how the review was going on the
> mentioned drivers.
> 
> I did imagine that of_gpiochip_add_pin_range() would be the
> sole parser of this, and drivers would then use the infrastructure
> for any necessary cross-reference between the subsystems,
> not second-code it.
> 
> What is it that you really need to do here?
> 
> I think npins should be known from the compatible (we know that
> this version of the SoC has so and so many pins) and the base
> should always be 0? It's not like we have several pin controllers
> of this type in the SoC is it?
All we need is the number of GPIOs from the DT now.
I got a bit confused with the 'base' here, locally I have removed the
'pin_base' variable and usage. I was confusing it with the `gpio_chip.base` field,
however that seems to be about the internal GPIO numbering.

Thanks,
Joey

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

[Linus Walleij]

On Tue, Sep 28, 2021 at 8:21 PM Joey Gouly <joey.gouly@arm.com> wrote:

> > > Linus? Do we really need this?
> >
> > I don't really see this as necessary, we don't need to check everything.
> > Not that it hurts either, so I would say maintainer preference?
> >
> I'm unsure if that means you (Linus), or Hector Martin as I put this
> file under the "ARM/APPLE MACHINE SUPPORT" section in MAINTAINERS.

Your preference, I am sure that even if Hector certainly may want to
chime in, he expects the code author (you) to assume a bit of
ownership of this piece of code for the foreseeable future,
so do what you consider reasonable.

> > I think npins should be known from the compatible (we know that
> > this version of the SoC has so and so many pins) and the base
> > should always be 0? It's not like we have several pin controllers
> > of this type in the SoC is it?
>
> All we need is the number of GPIOs from the DT now.
> I got a bit confused with the 'base' here, locally I have removed the
> 'pin_base' variable and usage. I was confusing it with the `gpio_chip.base` field,
> however that seems to be about the internal GPIO numbering.

The GPIO bindings have a standardized "ngpios" property that
indicates if you use less than the HW max number of GPIO
lines (roughly).

This is about pins rather, so something custom like
apple,npins = <...>;
could be used in that case.

Yours,
Linus Walleij

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

[Marc Zyngier]

Hi Joey,

On Tue, 21 Sep 2021 23:29:56 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> From: Stan Skowronek <stan@corellium.com>
> 
> This driver adds support for the pinctrl / GPIO hardware found
> on some Apple SoCs.
> 
> Co-authored-by: Joey Gouly <joey.gouly@arm.com>
> Signed-off-by: Stan Skowronek <stan@corellium.com>
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> ---
>  MAINTAINERS                          |   1 +
>  drivers/pinctrl/Kconfig              |  13 +
>  drivers/pinctrl/Makefile             |   1 +
>  drivers/pinctrl/pinctrl-apple-gpio.c | 652 +++++++++++++++++++++++++++
>  4 files changed, 667 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-apple-gpio.c
> 
> 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
>  
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 31921108e456..7269614c85a1 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -31,6 +31,19 @@ config DEBUG_PINCTRL
>  	help
>  	  Say Y here to add some extra checks and diagnostics to PINCTRL calls.
>  
> +config PINCTRL_APPLE_GPIO
> +	bool "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.
> +
>  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..a27d21682f3a
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-apple-gpio.c
> @@ -0,0 +1,652 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Apple SoC pinctrl+GPIO+external IRQ driver
> + *
> + * Copyright (C) 2021 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.
> + */
> +
> +#define USE_PINMUX_GENERIC_FN 1
> +#define USE_PINCTRL_GENERIC_FN 1
> +
> +#include <dt-bindings/pinctrl/apple.h>
> +#include <linux/clk.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 "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinmux.h"
> +
> +struct apple_gpio_pincfg {
> +	uint8_t irqtype;
> +	uint8_t stat;
> +};
> +
> +#define PINCFG_STAT_OUTVAL	0x01
> +#define PINCFG_STAT_OUTEN	0x02
> +#define PINCFG_STAT_PERIPH	0x20
> +#define PINCFG_STAT_IRQEN	0x80
> +
> +struct apple_gpio_pinctrl {
> +	struct device *dev;
> +	struct pinctrl_dev *pctldev;
> +
> +	unsigned int npins;
> +	struct pinctrl_pin_desc *pins;
> +	struct apple_gpio_pincfg *pin_cfgs;
> +	const char **pin_names;
> +	unsigned *pin_nums;
> +
> +	void __iomem *base;
> +	unsigned int nirqgrps;
> +
> +	struct pinctrl_desc pinctrl_desc;
> +	struct gpio_chip gpio_chip;
> +	struct irq_chip irq_chip;
> +};
> +
> +#define REG_GPIO(x)		(4 * (x))
> +#define  REG_GPIOx_DATA		BIT(0)
> +#define  REG_GPIOx_MODE_MASK	GENMASK(3, 1)
> +#define REG_GPIOx_OUT	(1 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_HI	(2 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_LO	(3 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_UP	(4 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_DN	(5 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_ANY	(6 << REG_GPIOx_DATA)
> +#define REG_GPIOx_IN_IRQ_OFF	(7 << REG_GPIOx_DATA)
> +#define  REG_GPIOx_PERIPH	BIT(5)
> +#define  REG_GPIOx_CFG_DONE	BIT(9)
> +#define  REG_GPIOx_GRP_MASK	GENMASK(18, 16)
> +#define REG_IRQ(g,x)		(0x800 + 0x40 * (g) + 4 * ((x) >> 5))
> +
> +static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl, unsigned pin, uint32_t clr, uint32_t set)
> +{
> +	void __iomem *ppin = pctl->base + pin * 4;
> +	uint32_t prev, cfg;
> +
> +	prev = readl(ppin);
> +	cfg = (prev & ~clr) | set;
> +
> +	if(!(prev & REG_GPIOx_CFG_DONE))
> +		writel(cfg & ~REG_GPIOx_CFG_DONE, ppin);
> +	writel(cfg, ppin);

I'll add my usual rant about the use of relaxed accessors, or lack
thereof. Unless there is a compelling reason for spitting out
barriers everywhere, please use the _relaxed version.

> +}
> +
> +static void apple_gpio_refresh_reg(struct apple_gpio_pinctrl *pctl, unsigned pin)
> +{
> +	struct apple_gpio_pincfg *pincfg = &pctl->pin_cfgs[pin];
> +
> +	int stat = pincfg->stat;
> +	int outval = (stat & PINCFG_STAT_OUTVAL);
> +
> +	int clear = REG_GPIOx_MODE_MASK | REG_GPIOx_DATA;
> +	int set = REG_GPIOx_CFG_DONE | outval;
> +
> +	if (stat & PINCFG_STAT_PERIPH) {
> +		set |= REG_GPIOx_PERIPH;
> +	} else {
> +		clear |= REG_GPIOx_PERIPH;
> +		if (stat & PINCFG_STAT_OUTEN)
> +			set |= REG_GPIOx_OUT;
> +		else if (stat & PINCFG_STAT_IRQEN)
> +			set |= pincfg->irqtype;
> +		else
> +			set |= REG_GPIOx_IN_IRQ_OFF;
> +	}
> +
> +	apple_gpio_set_reg(pctl, pin, clear, set);

Please add some documentation on how these bits behave. This is
specially important as the mask/unmask operations can be used
concurrently. Unless the HW ensures mutual exclusion one way or
another (such as separate registers per interrupts?), you'll need some
locking here.

[...]

I know nothing about pinctrl and GPIO, so I'll skip directly to my own
turf...

> +/* 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 irqgrp = FIELD_GET(REG_GPIOx_GRP_MASK, apple_gpio_get_reg(pctl, data->hwirq));
> +
> +	writel(1u << (data->hwirq & 31), pctl->base + REG_IRQ(irqgrp, data->hwirq));

Use BIT().

> +}
> +
> +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));
> +
> +	pctl->pin_cfgs[data->hwirq].stat &= ~PINCFG_STAT_IRQEN;
> +	apple_gpio_refresh_reg(pctl, data->hwirq);

See my above note about the potential requirement for locking.

> +}
> +
> +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));
> +
> +	pctl->pin_cfgs[data->hwirq].stat |= PINCFG_STAT_IRQEN;
> +	apple_gpio_refresh_reg(pctl, data->hwirq);
> +}
> +
> +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);
> +	unsigned irqgrp = 0;

Useless variable?

> +
> +	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP_MASK, FIELD_PREP(REG_GPIOx_GRP_MASK, irqgrp));
> +
> +	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));
> +
> +	switch(type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_UP;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_DN;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_ANY;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_HI;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		pctl->pin_cfgs[data->hwirq].irqtype = REG_GPIOx_IN_IRQ_LO;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	apple_gpio_refresh_reg(pctl, data->hwirq);

A general question about how the HW works: you seem to shadow some of
the state in memory, only to end-up committing it into the HW at a
later point. Why the indirection? I don't think a RMW operation is
going to hurt.

> +
> +	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 gpio_chip *gc = irq_desc_get_handler_data(desc);
> +	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(gc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned irqgrp, pinh, pinl;
> +	unsigned long pending;
> +	unsigned int parent = irq_desc_get_irq(desc);
> +
> +	for (irqgrp = 0; irqgrp < pctl->nirqgrps; ++irqgrp) {
> +		if (parent == gc->irq.parents[irqgrp])
> +			break;
> +	}

This seems counter-productive. Why can't the handler data give you the
correct context without having to iterate on internal data structures?

> +
> +	WARN_ON(irqgrp == pctl->nirqgrps);

WARN_ON(), followed by the dereferencing of random data? If that
cannot happen, please drop it. If it can happen, just fix it. See
below for my take on this.

> +
> +	chained_irq_enter(chip, desc);
> +	for(pinh=0; pinh<pctl->npins; pinh+=32) {

Coding style, please (all over the file).

> +		pending = readl(pctl->base + REG_IRQ(irqgrp, pinh));
> +		for_each_set_bit(pinl, &pending, 32)
> +			generic_handle_irq(irq_linear_revmap(gc->irq.domain, pinh + pinl));

This should be a call to generic_handle_domain_irq().
irq_linear_revmap() is going away soon.

> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +/* Probe & register */
> +
> +static int apple_gpio_gpio_register(struct apple_gpio_pinctrl *pctl)
> +{
> +	struct device_node *node = pctl->dev->of_node;
> +	struct gpio_irq_chip *girq;
> +	int i, ret = 0;
> +
> +	if(!of_find_property(node, "gpio-controller", NULL)) {
> +		dev_err(pctl->dev, "Apple GPIO must have 'gpio-controller' property.\n");
> +		return -ENODEV;
> +	}
> +
> +	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->npins;
> +	pctl->gpio_chip.parent = pctl->dev;
> +	pctl->gpio_chip.of_node = node;
> +
> +	if (of_find_property(node, "interrupt-controller", NULL)) {
> +		ret = platform_irq_count(to_platform_device(pctl->dev));
> +		if(ret < 0)
> +			return ret;
> +
> +		pctl->nirqgrps = ret;
> +
> +		pctl->irq_chip.name = dev_name(pctl->dev);

No, please. We don't need a massively long name that will make
/proc/interrupts more messy than it needs to be (and creates ABI
issues when someone repaints the DT). Just say "GPIO".

> +		pctl->irq_chip.irq_startup = apple_gpio_gpio_irq_startup;
> +		pctl->irq_chip.irq_ack = apple_gpio_gpio_irq_ack;
> +		pctl->irq_chip.irq_mask = apple_gpio_gpio_irq_mask;
> +		pctl->irq_chip.irq_unmask = apple_gpio_gpio_irq_unmask;
> +		pctl->irq_chip.irq_set_type = apple_gpio_gpio_irq_set_type;
> +
> +		girq = &pctl->gpio_chip.irq;
> +		girq->chip = &pctl->irq_chip;
> +		girq->parent_handler = apple_gpio_gpio_irq_handler;
> +		girq->num_parents = pctl->nirqgrps;
> +
> +		girq->parents = devm_kmalloc_array(pctl->dev, pctl->nirqgrps,
> +			sizeof(girq->parents[0]), GFP_KERNEL);
> +		if (!girq->parents)
> +			return -ENOMEM;
> +
> +		for(i = 0; i < pctl->nirqgrps; i++) {
> +			ret = platform_get_irq(to_platform_device(pctl->dev), i);
> +			if(ret < 0) {
> +				if(ret != -EPROBE_DEFER)
> +					dev_err(pctl->dev, "Failed to map IRQ %d (%d).\n", i, ret);
> +				return ret;
> +			}
> +			girq->parents[i] = ret;
> +		}

In general, keeping track of the parent interrupts in an interrupt
controller driver is a sure sign that things are badly organised.
Here, you have a single irqchip context that handles multiple muxes.
You really should have one context per mux, which would simplify your
interrupt handling.

An alternative is to use the fact that all the interrupts to the AIC
form a contiguous space, and use that to directly index into the array.

Thanks,

	M.

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

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

[Linus Walleij]

On Wed, Sep 22, 2021 at 3:09 PM Marc Zyngier <maz@kernel.org> wrote:
> Joey Gouly <joey.gouly@arm.com> wrote:

> > +             pctl->irq_chip.name = dev_name(pctl->dev);
>
> No, please. We don't need a massively long name that will make
> /proc/interrupts more messy than it needs to be (and creates ABI
> issues when someone repaints the DT). Just say "GPIO".

There may be some GPIO expanders in some systems as well
so I'd use "APPLE-GPIO" or similar so we know it is the one
on the SoC.

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Tue, Sep 21, 2021 at 11:29:55PM +0100, Joey Gouly wrote:
> Hi all,
> 
> This is a driver for the combined pinctrl / GPIO hw in the Apple M1 computers.
> The driver is based on Corellium's driver [1], and has been rebased and cleaned up.
> The bindings are in Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml

Have you checked if there is any (qualcomm / samsung / ?) existing driver is
close enough to this hardware?

> This has been tested with out-of-tree patches for the keyboard on the Macbook
> Air, it has also been tested with i2c for the USB PD driver and PCIe (all
> out-of-tree drivers currently).
> 
> I left two defines at the top 'USE_PINMUX_GENERIC_FN' and
> 'USE_PINCTRL_GENERIC_FN', I wasn't sure if I should use the generic functions for
> getting the groups/functions, so I left both approaches in and will remove one
> of them for the next version! Which approach should I use?

Try to get rid of defines.

> There is a branch here with the driver:
>   https://gitlab.arm.com/linux-arm/jg-open/-/commits/pinctrl_apple_v1
> 
> There is also a branch which contains all the commits as I was
> developing here (and keyboard drivers):
>   https://gitlab.arm.com/linux-arm/jg-open/-/commits/m1-keyboard
> 
> I look forward to feedback!
> 
> Thanks,
> Joey
> 
> note: I'm sending this from my arm work e-mail address, however it was done on
> personal time.

It doesn't matter to the community. Check this with your employer.

> note2: 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_v1_clock
> 
> [1]
> https://github.com/corellium/linux-m1/blob/d5ec2a737e64de23a21025f9eddc554588deb23f/drivers/pinctrl/pinctrl-apple-gpio.c

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Wed, Sep 22, 2021 at 12:30 AM Joey Gouly <joey.gouly@arm.com> wrote:

> I left two defines at the top 'USE_PINMUX_GENERIC_FN' and
> 'USE_PINCTRL_GENERIC_FN', I wasn't sure if I should use the generic functions for
> getting the groups/functions, so I left both approaches in and will remove one
> of them for the next version! Which approach should I use?

I think you should go with generic as far as possible, it usually
gives less code to maintain.

Yours,
Linus Walleij