[PATCH v10 0/8] gpio: add Ambarella CV75 PL061 support

[PATCH v10 0/8] gpio: add Ambarella CV75 PL061 support

[Long Zhao via B4 Relay]

The ambarella vendor prefix [1] is applied in the devicetree tree.
Do not apply the binding until the prefix reaches linux-next. The
driver patches do not have that dependency.

Please take this series through the GPIO tree. The quirk, the PL061
conversion and the Ambarella layout belong together.

This is v10 of the GPIO patches from the combined CV75 series. The
posting on Oct 9 numbered them v1 after the split. That was a
mistake.

These patches convert the ARM PL061 data path to regmap, then add
the CV75 layout.

GPIO data path uses gpio-regmap with two layouts (ARM PrimeCell and
Ambarella). ARM PL061 sets GPIO_REGMAP_QUIRK_SET_AFTER_DIR. IRQ stays
a custom PL061 irqchip. The linear domain is created with the PL061
private data and attached by gpio-regmap. regmap-irq is not used.

Tested on the CV75 EVK. The board boots. gpio-hammer toggles gpiochip0
line 26 ten times, and gpio-event-mon on line 27 records ten
alternating rising and falling edges.

Changes since the combined v9 [2]:
- dt-bindings: explain that CV75 is a modified PL061 (Conor).
- gpio-regmap: move quirks after fixed_direction_output (Andy).
- gpio: pl061: Andy's review on the regmap conversion and switch:
  use regmap_test_bits()/clear_bits()/set_bits()/assign_bits(),
  drop the redundant if (pending) and fast_io, introduce the ARM
  regmap_config (including REGCACHE_FLAT_S) in the convert patch
  and leave it there, blank line after guard() plus the blank-line
  and reverse Christmas tree cleanups, and keep the upstream
  pl061_irq_chip name and unsigned irq_type() trigger without
  adding .name.
- gpio: pl061: cache DAT (REGCACHE_FLAT_S, not volatile) so
  gpio_set() does not read-modify-write the pin level (Sashiko).

Changes since the Oct 9 GPIO posting [3]:
- gpio: pl061: Andy's follow-ups since that posting: assign_bits
  for DAT writes, drop fast_io, name the ARM regmap_config in the
  convert patch, the guard()/blank-line/irq_type cleanups, and keep
  the upstream pl061_irq_chip name and unsigned trigger without
  adding .name.
- gpiolib: regmap: add Michael Walle's Reviewed-by on
  GPIO_REGMAP_QUIRK_SET_AFTER_DIR.
- gpio: pl061: restore Andy's and Linus's Reviewed-by on the
  Ambarella variant.

[1] https://patch.msgid.link/20261014-cv75-prefix-v1-1-fe95ed567880@ambarella.com
[2] https://patch.msgid.link/20261001-cv75-v5-v9-0-486cc289a4b8@ambarella.com
[3] https://patch.msgid.link/20261009-cv75-gpio-v1-0-5836bd438423@ambarella.com

To: Bartosz Golaszewski <brgl@kernel.org>
To: linux-gpio@vger.kernel.org
Cc: Linus Walleij <linusw@kernel.org>
Cc: Michael Walle <mwalle@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: Long Zhao <longzhao@ambarella.com>
Cc: Long Zhao <zl020895@163.com>
Cc: linux-kernel@vger.kernel.org

Assisted-by: LLM
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
Long Zhao (8):
      dt-bindings: gpio: pl061: add Ambarella CV75 variant
      gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR
      gpio: pl061: drop redundant pending check in the IRQ handler
      gpio: pl061: convert register access to regmap
      gpio: pl061: use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH
      gpio: pl061: use cleanup helpers for locks
      gpio: pl061: switch data path to gpio-regmap
      gpio: pl061: add Ambarella register-layout variant

 .../devicetree/bindings/gpio/pl061-gpio.yaml       |  25 +-
 drivers/gpio/Kconfig                               |   2 +
 drivers/gpio/gpio-pl061.c                          | 552 +++++++++++++--------
 drivers/gpio/gpio-regmap.c                         |  15 +-
 include/linux/amba/bus.h                           |   1 +
 include/linux/gpio/regmap.h                        |  14 +
 6 files changed, 409 insertions(+), 200 deletions(-)
---
base-commit: 34cf6dafc47441dfb6b356a095b89c3585a93714
change-id: 20261007-cv75-gpio-333d7c1dfe11

Best regards,
--  
Long Zhao <longzhao@ambarella.com>

[PATCH v10 1/8] dt-bindings: gpio: pl061: add Ambarella CV75 variant

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

The CV75 GPIO is a modified PL061. The basic registers have the same
names and roles (DAT/DIR/IS/IBE/IEV/IE/MIS/IC). Each bank is 32-bit
and 32 lines and adds MASK and ENABLE, so it is not
register-compatible with the ARM PL061 and arm,pl061 must not be
used as a fallback.

The AMBA core matches the PL061 driver by PrimeCell ID, not by the DT
compatible string. These blocks do not implement the standard PL061 ID
(0x00041061) in the ID registers, so arm,primecell-periphid is required
and is fixed to 0x000e8061; the driver uses that ID to select the
Ambarella layout. gpio-ranges is required because each bank is a slice
of the SoC pinctrl pin space.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 .../devicetree/bindings/gpio/pl061-gpio.yaml       | 25 +++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml b/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
index 4d970e55104b..c22dd8a1d2c1 100644
--- a/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
@@ -15,7 +15,9 @@ select:
   properties:
     compatible:
       contains:
-        const: arm,pl061
+        enum:
+          - arm,pl061
+          - ambarella,cv75-gpio
   required:
     - compatible
 
@@ -25,7 +27,9 @@ properties:
 
   compatible:
     items:
-      - const: arm,pl061
+      - enum:
+          - ambarella,cv75-gpio
+          - arm,pl061
       - const: arm,primecell
 
   reg:
@@ -64,6 +68,21 @@ required:
   - "#gpio-cells"
   - gpio-controller
 
-additionalProperties: false
+allOf:
+  - $ref: /schemas/arm/primecell.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: ambarella,cv75-gpio
+    then:
+      properties:
+        arm,primecell-periphid:
+          const: 0x000e8061
+      required:
+        - arm,primecell-periphid
+        - gpio-ranges
+
+unevaluatedProperties: false
 
 ...

-- 
2.34.1

[PATCH v10 2/8] gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

Some controllers ignore output writes while a line is still an input.
Add a behaviour flag so the output value is written after the direction
change. This is a legacy quirk. New hardware must not use it.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/gpio-regmap.c  | 15 +++++++++++++--
 include/linux/gpio/regmap.h | 14 ++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 51b4d69b8740..0a44deae3d01 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -33,6 +33,7 @@ struct gpio_regmap {
 	unsigned int reg_dir_out_base;
 	unsigned long *fixed_direction_mask;
 	unsigned long *fixed_direction_output;
+	unsigned long quirks;
 
 #ifdef CONFIG_REGMAP_IRQ
 	int regmap_irq_line;
@@ -271,9 +272,17 @@ static int gpio_regmap_direction_output(struct gpio_chip *chip,
 			return ret;
 	}
 
-	gpio_regmap_set(chip, offset, value);
+	if (!(gpio->quirks & GPIO_REGMAP_QUIRK_SET_AFTER_DIR))
+		gpio_regmap_set(chip, offset, value);
 
-	return gpio_regmap_set_direction(chip, offset, true);
+	ret = gpio_regmap_set_direction(chip, offset, true);
+	if (ret)
+		return ret;
+
+	if (gpio->quirks & GPIO_REGMAP_QUIRK_SET_AFTER_DIR)
+		gpio_regmap_set(chip, offset, value);
+
+	return 0;
 }
 
 void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
@@ -376,6 +385,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 			    config->fixed_direction_output, chip->ngpio);
 	}
 
+	gpio->quirks = config->quirks;
+
 	/* if not set, assume there is only one register */
 	gpio->ngpio_per_reg = config->ngpio_per_reg;
 	if (!gpio->ngpio_per_reg)
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 06255756710d..f9090073c852 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -3,6 +3,8 @@
 #ifndef _LINUX_GPIO_REGMAP_H
 #define _LINUX_GPIO_REGMAP_H
 
+#include <linux/bits.h>
+
 struct device;
 struct fwnode_handle;
 struct gpio_regmap;
@@ -13,6 +15,14 @@ struct regmap;
 #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1))
 #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
 
+/*
+ * Write the output value after the line is switched to output.
+ * Needed when hardware ignores output writes while the line is an input.
+ * This is a legacy quirk; new hardware must not use it. Direction changes
+ * will glitch.
+ */
+#define GPIO_REGMAP_QUIRK_SET_AFTER_DIR BIT(0)
+
 /**
  * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
  * @parent:		The parent device
@@ -48,6 +58,7 @@ struct regmap;
  *			(Optional) Bitmap representing the fixed direction of
  *			the GPIO lines. Useful when there are GPIO lines with a
  *			fixed direction mixed together in the same register.
+ * @quirks:		(Optional) Behaviour flags, OR of GPIO_REGMAP_QUIRK_*.
  * @drvdata:		(Optional) Pointer to driver specific data which is
  *			not used by gpio-remap but is provided "as is" to the
  *			driver callback(s).
@@ -98,6 +109,9 @@ struct gpio_regmap_config {
 	unsigned long *fixed_direction_mask;
 	unsigned long *fixed_direction_output;
 
+	/* Regmap GPIO behaviour flags */
+	unsigned long quirks;
+
 #ifdef CONFIG_REGMAP_IRQ
 	struct regmap_irq_chip *regmap_irq_chip;
 	int regmap_irq_line;

-- 
2.34.1

Re: [PATCH v10 2/8] gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: regmap: direction_output bypasses set_with_clear callback
- [Medium] gpio: regmap: silent masking of regmap I/O errors in direction_output
--

commit 3bcfc79235e7b203090c7124352804ddb1bcdde7
Author: Long Zhao <longzhao@ambarella.com>

gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR

This commit introduces a new quirk flag, GPIO_REGMAP_QUIRK_SET_AFTER_DIR,
which allows the output value to be written after the direction change.
This supports legacy controllers that ignore output writes while a line
is still configured as an input.

> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c

[ ... ]

> @@ -271,9 +272,17 @@ static int gpio_regmap_direction_output(struct gpio_chip *chip,
>  			return ret;
>  	}
>  
> -	gpio_regmap_set(chip, offset, value);
> +	if (!(gpio->quirks & GPIO_REGMAP_QUIRK_SET_AFTER_DIR))
> +		gpio_regmap_set(chip, offset, value);
>  
> -	return gpio_regmap_set_direction(chip, offset, true);
> +	ret = gpio_regmap_set_direction(chip, offset, true);
> +	if (ret)
> +		return ret;
> +
> +	if (gpio->quirks & GPIO_REGMAP_QUIRK_SET_AFTER_DIR)
> +		gpio_regmap_set(chip, offset, value);

[Severity: High]
Does this bypass the chip->set callback for hardware that uses separate set
and clear registers?

When setting the initial value during a direction change to output,
hardcoding a call to gpio_regmap_set() instead of using the properly assigned
callback (such as gpio_regmap_set_with_clear) means that if value == 0, it
writes 0 to the reg_set_base register.

On set/clear hardware, this is typically a no-op instead of properly writing
the mask to reg_clr_base. This could leave the pin incorrectly driven high.

[Severity: Medium]
Is it safe to ignore the return value of gpio_regmap_set() here?

If the underlying bus (like I2C or SPI) encounters a failure or timeout
during the register write, the negative error code is silently dropped.

The gpiolib core would be falsely informed that the direction and value were
successfully configured, leading to state inconsistencies where software
believes a line is driven high or low but the hardware register write failed.

> +
> +	return 0;
>  }
>  
>  void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-cv75-gpio-v10-0-3fe3eb315a55@ambarella.com?part=2

[PATCH v10 3/8] gpio: pl061: drop redundant pending check in the IRQ handler

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

for_each_set_bit() does nothing when the mask is zero, so the
if (pending) wrapper around it is redundant.

Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/gpio-pl061.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 919cf86fd590..e48842601310 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -219,11 +219,9 @@ static void pl061_irq_handler(struct irq_desc *desc)
 	chained_irq_enter(irqchip, desc);
 
 	pending = readb(pl061->base + GPIOMIS);
-	if (pending) {
-		for_each_set_bit(offset, &pending, PL061_GPIO_NR)
-			generic_handle_domain_irq(gc->irq.domain,
-						  offset);
-	}
+	for_each_set_bit(offset, &pending, PL061_GPIO_NR)
+		generic_handle_domain_irq(gc->irq.domain,
+					  offset);
 
 	chained_irq_exit(irqchip, desc);
 }

-- 
2.34.1

[PATCH v10 4/8] gpio: pl061: convert register access to regmap

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

Replace readb()/writeb() with a MMIO regmap while keeping the existing
gpio_chip and girq irqchip. Use a raw spinlock so irq_ack() can run
under the irq descriptor lock.

Reviewed-by: Linus Walleij <linusw@kernel.org>
Tested-by: Chanho Min <chanho.min@lge.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/Kconfig      |   1 +
 drivers/gpio/gpio-pl061.c | 135 ++++++++++++++++++++++++++++++----------------
 2 files changed, 91 insertions(+), 45 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index f03c05288376..2a1487bd1a94 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -583,6 +583,7 @@ config GPIO_PL061
 	depends on ARM_AMBA || COMPILE_TEST
 	select IRQ_DOMAIN
 	select GPIOLIB_IRQCHIP
+	select REGMAP_MMIO
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device.
 
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index e48842601310..68d5a9e362ef 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -19,9 +19,11 @@
 #include <linux/ioport.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
+#include <linux/regmap.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -36,6 +38,7 @@
 #define GPIOIC  0x41C
 
 #define PL061_GPIO_NR	8
+#define PL061_DATA_OFFSET	2
 
 struct pl061_context_save_regs {
 	u8 gpio_data;
@@ -49,18 +52,51 @@ struct pl061_context_save_regs {
 struct pl061 {
 	raw_spinlock_t		lock;
 
-	void __iomem		*base;
+	struct regmap		*regmap;
 	struct gpio_chip	gc;
 	int			parent_irq;
 
 	struct pl061_context_save_regs csave_regs;
 };
 
+static bool pl061_arm_is_data_reg(unsigned int reg)
+{
+	return reg >= BIT(PL061_DATA_OFFSET) &&
+	       reg <= BIT(PL061_DATA_OFFSET + PL061_GPIO_NR - 1) &&
+	       is_power_of_2(reg);
+}
+
+static bool pl061_arm_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case GPIOMIS:
+	case GPIOIC:
+		return true;
+	default:
+		return pl061_arm_is_data_reg(reg);
+	}
+}
+
+static const struct regmap_config pl061_arm_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.reg_stride = 4,
+	.max_register = GPIOIC,
+	.volatile_reg = pl061_arm_volatile_reg,
+	.cache_type = REGCACHE_FLAT_S,
+	.use_raw_spinlock = true,
+};
+
 static int pl061_get_direction(struct gpio_chip *gc, unsigned offset)
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
+	int ret;
+
+	ret = regmap_test_bits(pl061->regmap, GPIODIR, BIT(offset));
+	if (ret < 0)
+		return ret;
 
-	if (readb(pl061->base + GPIODIR) & BIT(offset))
+	if (ret)
 		return GPIO_LINE_DIRECTION_OUT;
 
 	return GPIO_LINE_DIRECTION_IN;
@@ -70,12 +106,9 @@ static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	unsigned long flags;
-	unsigned char gpiodir;
 
 	raw_spin_lock_irqsave(&pl061->lock, flags);
-	gpiodir = readb(pl061->base + GPIODIR);
-	gpiodir &= ~(BIT(offset));
-	writeb(gpiodir, pl061->base + GPIODIR);
+	regmap_clear_bits(pl061->regmap, GPIODIR, BIT(offset));
 	raw_spin_unlock_irqrestore(&pl061->lock, flags);
 
 	return 0;
@@ -86,19 +119,19 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	unsigned long flags;
-	unsigned char gpiodir;
+	unsigned int mask = BIT(offset);
 
 	raw_spin_lock_irqsave(&pl061->lock, flags);
-	writeb(!!value << offset, pl061->base + (BIT(offset + 2)));
-	gpiodir = readb(pl061->base + GPIODIR);
-	gpiodir |= BIT(offset);
-	writeb(gpiodir, pl061->base + GPIODIR);
+	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
+			   BIT(offset), value);
+	regmap_set_bits(pl061->regmap, GPIODIR, mask);
 
 	/*
 	 * gpio value is set again, because pl061 doesn't allow to set value of
 	 * a gpio pin before configuring it in OUT mode.
 	 */
-	writeb(!!value << offset, pl061->base + (BIT(offset + 2)));
+	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
+			   BIT(offset), value);
 	raw_spin_unlock_irqrestore(&pl061->lock, flags);
 
 	return 0;
@@ -108,14 +141,17 @@ static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 
-	return !!readb(pl061->base + (BIT(offset + 2)));
+	return regmap_test_bits(pl061->regmap,
+				BIT(offset + PL061_DATA_OFFSET),
+				BIT(offset));
 }
 
 static int pl061_set_value(struct gpio_chip *gc, unsigned int offset, int value)
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 
-	writeb(!!value << offset, pl061->base + (BIT(offset + 2)));
+	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
+			   BIT(offset), value);
 
 	return 0;
 }
@@ -126,8 +162,10 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	int offset = irqd_to_hwirq(d);
 	unsigned long flags;
-	u8 gpiois, gpioibe, gpioiev;
-	u8 bit = BIT(offset);
+	unsigned int gpioiev = 0;
+	unsigned int gpioibe = 0;
+	unsigned int gpiois = 0;
+	unsigned int bit = BIT(offset);
 
 	if (offset < 0 || offset >= PL061_GPIO_NR)
 		return -EINVAL;
@@ -145,9 +183,9 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 
 	raw_spin_lock_irqsave(&pl061->lock, flags);
 
-	gpioiev = readb(pl061->base + GPIOIEV);
-	gpiois = readb(pl061->base + GPIOIS);
-	gpioibe = readb(pl061->base + GPIOIBE);
+	regmap_read(pl061->regmap, GPIOIEV, &gpioiev);
+	regmap_read(pl061->regmap, GPIOIS, &gpiois);
+	regmap_read(pl061->regmap, GPIOIBE, &gpioibe);
 
 	if (trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
 		bool polarity = trigger & IRQ_TYPE_LEVEL_HIGH;
@@ -199,9 +237,9 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 			 offset);
 	}
 
-	writeb(gpiois, pl061->base + GPIOIS);
-	writeb(gpioibe, pl061->base + GPIOIBE);
-	writeb(gpioiev, pl061->base + GPIOIEV);
+	regmap_write(pl061->regmap, GPIOIS, gpiois);
+	regmap_write(pl061->regmap, GPIOIBE, gpioibe);
+	regmap_write(pl061->regmap, GPIOIEV, gpioiev);
 
 	raw_spin_unlock_irqrestore(&pl061->lock, flags);
 
@@ -211,6 +249,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 static void pl061_irq_handler(struct irq_desc *desc)
 {
 	unsigned long pending;
+	unsigned int mis = 0;
 	int offset;
 	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
 	struct pl061 *pl061 = gpiochip_get_data(gc);
@@ -218,7 +257,8 @@ static void pl061_irq_handler(struct irq_desc *desc)
 
 	chained_irq_enter(irqchip, desc);
 
-	pending = readb(pl061->base + GPIOMIS);
+	regmap_read(pl061->regmap, GPIOMIS, &mis);
+	pending = mis;
 	for_each_set_bit(offset, &pending, PL061_GPIO_NR)
 		generic_handle_domain_irq(gc->irq.domain,
 					  offset);
@@ -231,11 +271,9 @@ static void pl061_irq_mask(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
-	u8 gpioie;
 
 	raw_spin_lock(&pl061->lock);
-	gpioie = readb(pl061->base + GPIOIE) & ~mask;
-	writeb(gpioie, pl061->base + GPIOIE);
+	regmap_clear_bits(pl061->regmap, GPIOIE, mask);
 	raw_spin_unlock(&pl061->lock);
 
 	gpiochip_disable_irq(gc, d->hwirq);
@@ -246,13 +284,11 @@ static void pl061_irq_unmask(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
-	u8 gpioie;
 
 	gpiochip_enable_irq(gc, d->hwirq);
 
 	raw_spin_lock(&pl061->lock);
-	gpioie = readb(pl061->base + GPIOIE) | mask;
-	writeb(gpioie, pl061->base + GPIOIE);
+	regmap_set_bits(pl061->regmap, GPIOIE, mask);
 	raw_spin_unlock(&pl061->lock);
 }
 
@@ -270,9 +306,7 @@ static void pl061_irq_ack(struct irq_data *d)
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
 
-	raw_spin_lock(&pl061->lock);
-	writeb(mask, pl061->base + GPIOIC);
-	raw_spin_unlock(&pl061->lock);
+	regmap_write(pl061->regmap, GPIOIC, mask);
 }
 
 static int pl061_irq_set_wake(struct irq_data *d, unsigned int state)
@@ -306,15 +340,20 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	struct device *dev = &adev->dev;
 	struct pl061 *pl061;
 	struct gpio_irq_chip *girq;
+	void __iomem *base;
 	int ret, irq;
 
 	pl061 = devm_kzalloc(dev, sizeof(*pl061), GFP_KERNEL);
 	if (pl061 == NULL)
 		return -ENOMEM;
 
-	pl061->base = devm_ioremap_resource(dev, &adev->res);
-	if (IS_ERR(pl061->base))
-		return PTR_ERR(pl061->base);
+	base = devm_ioremap_resource(dev, &adev->res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pl061->regmap = devm_regmap_init_mmio(dev, base, &pl061_arm_regmap_config);
+	if (IS_ERR(pl061->regmap))
+		return PTR_ERR(pl061->regmap);
 
 	raw_spin_lock_init(&pl061->lock);
 	pl061->gc.request = gpiochip_generic_request;
@@ -333,7 +372,7 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	/*
 	 * irq_chip support
 	 */
-	writeb(0, pl061->base + GPIOIE); /* disable irqs */
+	regmap_write(pl061->regmap, GPIOIE, 0); /* disable irqs */
 	irq = adev->irq[0];
 	if (!irq)
 		dev_warn(&adev->dev, "IRQ support disabled\n");
@@ -364,14 +403,20 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 static int pl061_suspend(struct device *dev)
 {
 	struct pl061 *pl061 = dev_get_drvdata(dev);
+	unsigned int val = 0;
 	int offset;
 
 	pl061->csave_regs.gpio_data = 0;
-	pl061->csave_regs.gpio_dir = readb(pl061->base + GPIODIR);
-	pl061->csave_regs.gpio_is = readb(pl061->base + GPIOIS);
-	pl061->csave_regs.gpio_ibe = readb(pl061->base + GPIOIBE);
-	pl061->csave_regs.gpio_iev = readb(pl061->base + GPIOIEV);
-	pl061->csave_regs.gpio_ie = readb(pl061->base + GPIOIE);
+	regmap_read(pl061->regmap, GPIODIR, &val);
+	pl061->csave_regs.gpio_dir = val;
+	regmap_read(pl061->regmap, GPIOIS, &val);
+	pl061->csave_regs.gpio_is = val;
+	regmap_read(pl061->regmap, GPIOIBE, &val);
+	pl061->csave_regs.gpio_ibe = val;
+	regmap_read(pl061->regmap, GPIOIEV, &val);
+	pl061->csave_regs.gpio_iev = val;
+	regmap_read(pl061->regmap, GPIOIE, &val);
+	pl061->csave_regs.gpio_ie = val;
 
 	for (offset = 0; offset < PL061_GPIO_NR; offset++) {
 		if (pl061->csave_regs.gpio_dir & (BIT(offset)))
@@ -396,10 +441,10 @@ static int pl061_resume(struct device *dev)
 			pl061_direction_input(&pl061->gc, offset);
 	}
 
-	writeb(pl061->csave_regs.gpio_is, pl061->base + GPIOIS);
-	writeb(pl061->csave_regs.gpio_ibe, pl061->base + GPIOIBE);
-	writeb(pl061->csave_regs.gpio_iev, pl061->base + GPIOIEV);
-	writeb(pl061->csave_regs.gpio_ie, pl061->base + GPIOIE);
+	regmap_write(pl061->regmap, GPIOIS, pl061->csave_regs.gpio_is);
+	regmap_write(pl061->regmap, GPIOIBE, pl061->csave_regs.gpio_ibe);
+	regmap_write(pl061->regmap, GPIOIEV, pl061->csave_regs.gpio_iev);
+	regmap_write(pl061->regmap, GPIOIE, pl061->csave_regs.gpio_ie);
 
 	return 0;
 }

-- 
2.34.1

[PATCH v10 5/8] gpio: pl061: use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

Use the standard IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH helpers
instead of open-coded edge and level comparisons.

Reviewed-by: Linus Walleij <linusw@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/gpio-pl061.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 68d5a9e362ef..8f431d08573d 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -170,9 +170,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 	if (offset < 0 || offset >= PL061_GPIO_NR)
 		return -EINVAL;
 
-	if ((trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) &&
-	    (trigger & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)))
-	{
+	if ((trigger & IRQ_TYPE_LEVEL_MASK) && (trigger & IRQ_TYPE_EDGE_BOTH)) {
 		dev_err(gc->parent,
 			"trying to configure line %d for both level and edge "
 			"detection, choose one!\n",
@@ -187,7 +185,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 	regmap_read(pl061->regmap, GPIOIS, &gpiois);
 	regmap_read(pl061->regmap, GPIOIBE, &gpioibe);
 
-	if (trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
+	if (trigger & IRQ_TYPE_LEVEL_MASK) {
 		bool polarity = trigger & IRQ_TYPE_LEVEL_HIGH;
 
 		/* Disable edge detection */

-- 
2.34.1

[PATCH v10 6/8] gpio: pl061: use cleanup helpers for locks

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

Convert raw_spinlock critical sections to guard() from cleanup.h.

Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/gpio-pl061.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 8f431d08573d..5c6d4209dc8e 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -10,6 +10,7 @@
  */
 #include <linux/amba/bus.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
@@ -105,11 +106,10 @@ static int pl061_get_direction(struct gpio_chip *gc, unsigned offset)
 static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pl061->lock, flags);
+	guard(raw_spinlock_irqsave)(&pl061->lock);
+
 	regmap_clear_bits(pl061->regmap, GPIODIR, BIT(offset));
-	raw_spin_unlock_irqrestore(&pl061->lock, flags);
 
 	return 0;
 }
@@ -118,10 +118,10 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 		int value)
 {
 	struct pl061 *pl061 = gpiochip_get_data(gc);
-	unsigned long flags;
 	unsigned int mask = BIT(offset);
 
-	raw_spin_lock_irqsave(&pl061->lock, flags);
+	guard(raw_spinlock_irqsave)(&pl061->lock);
+
 	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
 			   BIT(offset), value);
 	regmap_set_bits(pl061->regmap, GPIODIR, mask);
@@ -132,7 +132,6 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
 	 */
 	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
 			   BIT(offset), value);
-	raw_spin_unlock_irqrestore(&pl061->lock, flags);
 
 	return 0;
 }
@@ -161,7 +160,6 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	int offset = irqd_to_hwirq(d);
-	unsigned long flags;
 	unsigned int gpioiev = 0;
 	unsigned int gpioibe = 0;
 	unsigned int gpiois = 0;
@@ -178,8 +176,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 		return -EINVAL;
 	}
 
-
-	raw_spin_lock_irqsave(&pl061->lock, flags);
+	guard(raw_spinlock_irqsave)(&pl061->lock);
 
 	regmap_read(pl061->regmap, GPIOIEV, &gpioiev);
 	regmap_read(pl061->regmap, GPIOIS, &gpiois);
@@ -239,8 +236,6 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 	regmap_write(pl061->regmap, GPIOIBE, gpioibe);
 	regmap_write(pl061->regmap, GPIOIEV, gpioiev);
 
-	raw_spin_unlock_irqrestore(&pl061->lock, flags);
-
 	return 0;
 }
 
@@ -270,9 +265,9 @@ static void pl061_irq_mask(struct irq_data *d)
 	struct pl061 *pl061 = gpiochip_get_data(gc);
 	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
 
-	raw_spin_lock(&pl061->lock);
+	guard(raw_spinlock)(&pl061->lock);
+
 	regmap_clear_bits(pl061->regmap, GPIOIE, mask);
-	raw_spin_unlock(&pl061->lock);
 
 	gpiochip_disable_irq(gc, d->hwirq);
 }
@@ -285,9 +280,9 @@ static void pl061_irq_unmask(struct irq_data *d)
 
 	gpiochip_enable_irq(gc, d->hwirq);
 
-	raw_spin_lock(&pl061->lock);
+	guard(raw_spinlock)(&pl061->lock);
+
 	regmap_set_bits(pl061->regmap, GPIOIE, mask);
-	raw_spin_unlock(&pl061->lock);
 }
 
 /**

-- 
2.34.1

[PATCH v10 7/8] gpio: pl061: switch data path to gpio-regmap

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

Move get, set and direction onto gpio-regmap. ARM PL061 sets
GPIO_REGMAP_QUIRK_SET_AFTER_DIR so the output value is written
after the direction change.

Keep a custom irqchip and irq domain. Create the domain with
the PL061 private data and let gpio-regmap attach it.

Check the chained-handler regmap_read() return and always run
chained_irq_exit(). irq_type still ignores MMIO write returns.

irq_set_parent() nests the GPIO irq_desc under the AMBA parent
for irq_set_wake() and lockdep; checked with PROVE_LOCKING on
CV75.

Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/Kconfig      |   1 +
 drivers/gpio/gpio-pl061.c | 458 ++++++++++++++++++++++++++--------------------
 2 files changed, 264 insertions(+), 195 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 2a1487bd1a94..55a129836158 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -583,6 +583,7 @@ config GPIO_PL061
 	depends on ARM_AMBA || COMPILE_TEST
 	select IRQ_DOMAIN
 	select GPIOLIB_IRQCHIP
+	select GPIO_REGMAP
 	select REGMAP_MMIO
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device.
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 5c6d4209dc8e..3822faa3e276 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -12,52 +12,67 @@
 #include <linux/bitops.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
-#include <linux/errno.h>
-#include <linux/gpio/driver.h>
-#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/gpio/regmap.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/log2.h>
 #include <linux/module.h>
-#include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/seq_file.h>
-#include <linux/slab.h>
 #include <linux/spinlock.h>
 
-#define GPIODIR 0x400
-#define GPIOIS  0x404
-#define GPIOIBE 0x408
-#define GPIOIEV 0x40C
-#define GPIOIE  0x410
-#define GPIORIS 0x414
-#define GPIOMIS 0x418
-#define GPIOIC  0x41C
-
 #define PL061_GPIO_NR	8
 #define PL061_DATA_OFFSET	2
 
-struct pl061_context_save_regs {
-	u8 gpio_data;
-	u8 gpio_dir;
-	u8 gpio_is;
-	u8 gpio_ibe;
-	u8 gpio_iev;
-	u8 gpio_ie;
+#define PL061_DIR		0x400
+#define PL061_IS		0x404
+#define PL061_IBE		0x408
+#define PL061_IEV		0x40c
+#define PL061_IE		0x410
+#define PL061_MIS		0x418
+#define PL061_IC		0x41c
+
+struct pl061_regs {
+	unsigned int dat;
+	unsigned int dir;
+	unsigned int is;
+	unsigned int ibe;
+	unsigned int iev;
+	unsigned int ie;
+	unsigned int mis;
+	unsigned int ic;
+	unsigned int mask;
+	unsigned int enable;
+};
+
+struct pl061_drvdata {
+	const struct regmap_config *regmap_config;
+	const struct pl061_regs *regs;
+	unsigned int ngpio;
+	unsigned long quirks;
+	bool clear_irq_on_type;
+	bool pm;
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
+			      unsigned int offset, unsigned int *reg,
+			      unsigned int *mask);
 };
 
 struct pl061 {
 	raw_spinlock_t		lock;
 
+	struct device		*dev;
 	struct regmap		*regmap;
-	struct gpio_chip	gc;
+	const struct pl061_drvdata *data;
+	struct irq_domain	*irq_domain;
 	int			parent_irq;
 
-	struct pl061_context_save_regs csave_regs;
+	u32			saved_dat;
 };
 
 static bool pl061_arm_is_data_reg(unsigned int reg)
@@ -70,8 +85,8 @@ static bool pl061_arm_is_data_reg(unsigned int reg)
 static bool pl061_arm_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
-	case GPIOMIS:
-	case GPIOIC:
+	case PL061_MIS:
+	case PL061_IC:
 		return true;
 	default:
 		return pl061_arm_is_data_reg(reg);
@@ -82,94 +97,45 @@ static const struct regmap_config pl061_arm_regmap_config = {
 	.reg_bits = 16,
 	.val_bits = 8,
 	.reg_stride = 4,
-	.max_register = GPIOIC,
+	.max_register = PL061_IC,
 	.volatile_reg = pl061_arm_volatile_reg,
 	.cache_type = REGCACHE_FLAT_S,
 	.use_raw_spinlock = true,
 };
 
-static int pl061_get_direction(struct gpio_chip *gc, unsigned offset)
-{
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-	int ret;
-
-	ret = regmap_test_bits(pl061->regmap, GPIODIR, BIT(offset));
-	if (ret < 0)
-		return ret;
-
-	if (ret)
-		return GPIO_LINE_DIRECTION_OUT;
-
-	return GPIO_LINE_DIRECTION_IN;
-}
-
-static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
-{
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-
-	guard(raw_spinlock_irqsave)(&pl061->lock);
-
-	regmap_clear_bits(pl061->regmap, GPIODIR, BIT(offset));
-
-	return 0;
-}
-
-static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
-		int value)
-{
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-	unsigned int mask = BIT(offset);
-
-	guard(raw_spinlock_irqsave)(&pl061->lock);
-
-	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
-			   BIT(offset), value);
-	regmap_set_bits(pl061->regmap, GPIODIR, mask);
-
-	/*
-	 * gpio value is set again, because pl061 doesn't allow to set value of
-	 * a gpio pin before configuring it in OUT mode.
-	 */
-	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
-			   BIT(offset), value);
-
-	return 0;
-}
-
-static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
+static unsigned int pl061_line_mask(struct pl061 *pl061, irq_hw_number_t hwirq)
 {
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-
-	return regmap_test_bits(pl061->regmap,
-				BIT(offset + PL061_DATA_OFFSET),
-				BIT(offset));
+	return BIT(hwirq % pl061->data->ngpio);
 }
 
-static int pl061_set_value(struct gpio_chip *gc, unsigned int offset, int value)
+static int pl061_arm_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
+				    unsigned int offset, unsigned int *reg,
+				    unsigned int *mask)
 {
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-
-	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
-			   BIT(offset), value);
+	if (base == 0)
+		*reg = BIT(offset + PL061_DATA_OFFSET);
+	else
+		*reg = base;
+	*mask = BIT(offset);
 
 	return 0;
 }
 
 static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct pl061 *pl061 = gpiochip_get_data(gc);
+	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
+	const struct pl061_regs *regs = pl061->data->regs;
 	int offset = irqd_to_hwirq(d);
 	unsigned int gpioiev = 0;
 	unsigned int gpioibe = 0;
 	unsigned int gpiois = 0;
 	unsigned int bit = BIT(offset);
 
-	if (offset < 0 || offset >= PL061_GPIO_NR)
+	if (offset < 0 || offset >= pl061->data->ngpio)
 		return -EINVAL;
 
 	if ((trigger & IRQ_TYPE_LEVEL_MASK) && (trigger & IRQ_TYPE_EDGE_BOTH)) {
-		dev_err(gc->parent,
+		dev_err(pl061->dev,
 			"trying to configure line %d for both level and edge "
 			"detection, choose one!\n",
 			offset);
@@ -178,9 +144,9 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 
 	guard(raw_spinlock_irqsave)(&pl061->lock);
 
-	regmap_read(pl061->regmap, GPIOIEV, &gpioiev);
-	regmap_read(pl061->regmap, GPIOIS, &gpiois);
-	regmap_read(pl061->regmap, GPIOIBE, &gpioibe);
+	regmap_read(pl061->regmap, regs->iev, &gpioiev);
+	regmap_read(pl061->regmap, regs->is, &gpiois);
+	regmap_read(pl061->regmap, regs->ibe, &gpioibe);
 
 	if (trigger & IRQ_TYPE_LEVEL_MASK) {
 		bool polarity = trigger & IRQ_TYPE_LEVEL_HIGH;
@@ -195,7 +161,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 		else
 			gpioiev &= ~bit;
 		irq_set_handler_locked(d, handle_level_irq);
-		dev_dbg(gc->parent, "line %d: IRQ on %s level\n",
+		dev_dbg(pl061->dev, "line %d: IRQ on %s level\n",
 			offset,
 			polarity ? "HIGH" : "LOW");
 	} else if ((trigger & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
@@ -204,7 +170,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 		/* Select both edges, setting this makes GPIOEV be ignored */
 		gpioibe |= bit;
 		irq_set_handler_locked(d, handle_edge_irq);
-		dev_dbg(gc->parent, "line %d: IRQ on both edges\n", offset);
+		dev_dbg(pl061->dev, "line %d: IRQ on both edges\n", offset);
 	} else if ((trigger & IRQ_TYPE_EDGE_RISING) ||
 		   (trigger & IRQ_TYPE_EDGE_FALLING)) {
 		bool rising = trigger & IRQ_TYPE_EDGE_RISING;
@@ -219,7 +185,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 		else
 			gpioiev &= ~bit;
 		irq_set_handler_locked(d, handle_edge_irq);
-		dev_dbg(gc->parent, "line %d: IRQ on %s edge\n",
+		dev_dbg(pl061->dev, "line %d: IRQ on %s edge\n",
 			offset,
 			rising ? "RISING" : "FALLING");
 	} else {
@@ -228,61 +194,56 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
 		gpioibe &= ~bit;
 		gpioiev &= ~bit;
 		irq_set_handler_locked(d, handle_bad_irq);
-		dev_warn(gc->parent, "no trigger selected for line %d\n",
+		dev_warn(pl061->dev, "no trigger selected for line %d\n",
 			 offset);
 	}
 
-	regmap_write(pl061->regmap, GPIOIS, gpiois);
-	regmap_write(pl061->regmap, GPIOIBE, gpioibe);
-	regmap_write(pl061->regmap, GPIOIEV, gpioiev);
+	regmap_write(pl061->regmap, regs->is, gpiois);
+	regmap_write(pl061->regmap, regs->ibe, gpioibe);
+	regmap_write(pl061->regmap, regs->iev, gpioiev);
+	if (pl061->data->clear_irq_on_type)
+		regmap_write(pl061->regmap, regs->ic, bit);
 
 	return 0;
 }
 
 static void pl061_irq_handler(struct irq_desc *desc)
 {
+	struct pl061 *pl061 = irq_desc_get_handler_data(desc);
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
 	unsigned long pending;
 	unsigned int mis = 0;
 	int offset;
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	int ret;
 
 	chained_irq_enter(irqchip, desc);
 
-	regmap_read(pl061->regmap, GPIOMIS, &mis);
+	ret = regmap_read(pl061->regmap, pl061->data->regs->mis, &mis);
+	if (ret)
+		goto out;
+
 	pending = mis;
-	for_each_set_bit(offset, &pending, PL061_GPIO_NR)
-		generic_handle_domain_irq(gc->irq.domain,
-					  offset);
+	for_each_set_bit(offset, &pending, pl061->data->ngpio)
+		generic_handle_domain_irq(pl061->irq_domain, offset);
 
+out:
 	chained_irq_exit(irqchip, desc);
 }
 
 static void pl061_irq_mask(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
-
-	guard(raw_spinlock)(&pl061->lock);
+	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
+	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
 
-	regmap_clear_bits(pl061->regmap, GPIOIE, mask);
-
-	gpiochip_disable_irq(gc, d->hwirq);
+	regmap_clear_bits(pl061->regmap, pl061->data->regs->ie, mask);
 }
 
 static void pl061_irq_unmask(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
-
-	gpiochip_enable_irq(gc, d->hwirq);
-
-	guard(raw_spinlock)(&pl061->lock);
+	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
+	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
 
-	regmap_set_bits(pl061->regmap, GPIOIE, mask);
+	regmap_set_bits(pl061->regmap, pl061->data->regs->ie, mask);
 }
 
 /**
@@ -295,26 +256,24 @@ static void pl061_irq_unmask(struct irq_data *d)
  */
 static void pl061_irq_ack(struct irq_data *d)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct pl061 *pl061 = gpiochip_get_data(gc);
-	u8 mask = BIT(irqd_to_hwirq(d) % PL061_GPIO_NR);
+	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
+	unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
 
-	regmap_write(pl061->regmap, GPIOIC, mask);
+	regmap_write(pl061->regmap, pl061->data->regs->ic, mask);
 }
 
 static int pl061_irq_set_wake(struct irq_data *d, unsigned int state)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct pl061 *pl061 = gpiochip_get_data(gc);
+	struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
 
 	return irq_set_irq_wake(pl061->parent_irq, state);
 }
 
 static void pl061_irq_print_chip(struct irq_data *data, struct seq_file *p)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct pl061 *pl061 = irq_data_get_irq_chip_data(data);
 
-	seq_puts(p, dev_name(gc->parent));
+	seq_puts(p, dev_name(pl061->dev));
 }
 
 static const struct irq_chip pl061_irq_chip = {
@@ -325,67 +284,159 @@ static const struct irq_chip pl061_irq_chip = {
 	.irq_set_wake		= pl061_irq_set_wake,
 	.irq_print_chip		= pl061_irq_print_chip,
 	.flags			= IRQCHIP_IMMUTABLE,
-	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int pl061_irq_domain_map(struct irq_domain *d, unsigned int virq,
+				irq_hw_number_t hwirq)
+{
+	struct pl061 *pl061 = d->host_data;
+
+	irq_set_chip_data(virq, pl061);
+	irq_set_chip_and_handler(virq, &pl061_irq_chip, handle_bad_irq);
+	irq_set_noprobe(virq);
+	irq_set_parent(virq, pl061->parent_irq);
+
+	return 0;
+}
+
+static void pl061_irq_domain_unmap(struct irq_domain *d, unsigned int virq)
+{
+	irq_set_chip_and_handler(virq, NULL, NULL);
+	irq_set_chip_data(virq, NULL);
+}
+
+static const struct irq_domain_ops pl061_irq_domain_ops = {
+	.map	= pl061_irq_domain_map,
+	.unmap	= pl061_irq_domain_unmap,
+	.xlate	= irq_domain_xlate_twothreecell,
+};
+
+static void pl061_remove_irq(void *data)
+{
+	struct pl061 *pl061 = data;
+
+	irq_set_chained_handler_and_data(pl061->parent_irq, NULL, NULL);
+
+	for (unsigned int i = 0; i < pl061->data->ngpio; i++) {
+		unsigned int virq;
+
+		virq = irq_find_mapping(pl061->irq_domain, i);
+		if (virq)
+			irq_dispose_mapping(virq);
+	}
+
+	irq_domain_remove(pl061->irq_domain);
+}
+
+static const struct pl061_regs pl061_arm_regs = {
+	.dir	= PL061_DIR,
+	.is	= PL061_IS,
+	.ibe	= PL061_IBE,
+	.iev	= PL061_IEV,
+	.ie	= PL061_IE,
+	.mis	= PL061_MIS,
+	.ic	= PL061_IC,
+};
+
+static const struct pl061_drvdata pl061_arm = {
+	.regmap_config		= &pl061_arm_regmap_config,
+	.regs			= &pl061_arm_regs,
+	.ngpio			= PL061_GPIO_NR,
+	.quirks			= GPIO_REGMAP_QUIRK_SET_AFTER_DIR,
+	.pm			= true,
+	.reg_mask_xlate		= pl061_arm_reg_mask_xlate,
 };
 
 static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 {
+	struct gpio_regmap_config config = {};
+	const struct pl061_drvdata *data;
 	struct device *dev = &adev->dev;
+	struct gpio_regmap *gpio_regmap;
+	const struct pl061_regs *regs;
 	struct pl061 *pl061;
-	struct gpio_irq_chip *girq;
 	void __iomem *base;
-	int ret, irq;
+	int ret;
+	int irq;
+
+	data = id->data;
+	if (!data)
+		return -ENODATA;
+
+	regs = data->regs;
 
 	pl061 = devm_kzalloc(dev, sizeof(*pl061), GFP_KERNEL);
 	if (pl061 == NULL)
 		return -ENOMEM;
 
+	pl061->dev = dev;
+	pl061->data = data;
+
 	base = devm_ioremap_resource(dev, &adev->res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	pl061->regmap = devm_regmap_init_mmio(dev, base, &pl061_arm_regmap_config);
+	pl061->regmap = devm_regmap_init_mmio(dev, base, data->regmap_config);
 	if (IS_ERR(pl061->regmap))
 		return PTR_ERR(pl061->regmap);
 
 	raw_spin_lock_init(&pl061->lock);
-	pl061->gc.request = gpiochip_generic_request;
-	pl061->gc.free = gpiochip_generic_free;
-	pl061->gc.base = -1;
-	pl061->gc.get_direction = pl061_get_direction;
-	pl061->gc.direction_input = pl061_direction_input;
-	pl061->gc.direction_output = pl061_direction_output;
-	pl061->gc.get = pl061_get_value;
-	pl061->gc.set = pl061_set_value;
-	pl061->gc.ngpio = PL061_GPIO_NR;
-	pl061->gc.label = dev_name(dev);
-	pl061->gc.parent = dev;
-	pl061->gc.owner = THIS_MODULE;
+
+	if (regs->enable) {
+		ret = regmap_write(pl061->regmap, regs->enable, ~0U);
+		if (ret)
+			return ret;
+	}
+	if (regs->mask) {
+		ret = regmap_write(pl061->regmap, regs->mask, ~0U);
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * irq_chip support
 	 */
-	regmap_write(pl061->regmap, GPIOIE, 0); /* disable irqs */
+	regmap_write(pl061->regmap, regs->ie, 0); /* disable irqs */
+
 	irq = adev->irq[0];
 	if (!irq)
-		dev_warn(&adev->dev, "IRQ support disabled\n");
+		dev_warn(dev, "IRQ support disabled\n");
 	pl061->parent_irq = irq;
 
-	girq = &pl061->gc.irq;
-	gpio_irq_chip_set_chip(girq, &pl061_irq_chip);
-	girq->parent_handler = pl061_irq_handler;
-	girq->num_parents = 1;
-	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
-				     GFP_KERNEL);
-	if (!girq->parents)
-		return -ENOMEM;
-	girq->parents[0] = irq;
-	girq->default_type = IRQ_TYPE_NONE;
-	girq->handler = handle_bad_irq;
+	if (irq) {
+		pl061->irq_domain = irq_domain_create_linear(dev_fwnode(dev),
+							     data->ngpio,
+							     &pl061_irq_domain_ops,
+							     pl061);
+		if (!pl061->irq_domain)
+			return -ENOMEM;
+	}
 
-	ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061);
-	if (ret)
-		return ret;
+	config.parent = dev;
+	config.regmap = pl061->regmap;
+	config.ngpio = data->ngpio;
+	config.reg_dat_base = GPIO_REGMAP_ADDR(regs->dat);
+	config.reg_set_base = GPIO_REGMAP_ADDR(regs->dat);
+	config.reg_dir_out_base = GPIO_REGMAP_ADDR(regs->dir);
+	config.reg_mask_xlate = data->reg_mask_xlate;
+	config.quirks = data->quirks;
+	config.irq_domain = pl061->irq_domain;
+	config.drvdata = pl061;
+
+	gpio_regmap = devm_gpio_regmap_register(dev, &config);
+	if (IS_ERR(gpio_regmap)) {
+		if (pl061->irq_domain)
+			irq_domain_remove(pl061->irq_domain);
+		return PTR_ERR(gpio_regmap);
+	}
+
+	if (irq) {
+		ret = devm_add_action_or_reset(dev, pl061_remove_irq, pl061);
+		if (ret)
+			return ret;
+
+		irq_set_chained_handler_and_data(irq, pl061_irq_handler, pl061);
+	}
 
 	amba_set_drvdata(adev, pl061);
 	dev_info(dev, "PL061 GPIO chip registered\n");
@@ -393,28 +444,39 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	return 0;
 }
 
+static void pl061_cache_ctrl_regs(struct pl061 *pl061, unsigned int *dir)
+{
+	const struct pl061_regs *regs = pl061->data->regs;
+	unsigned int val;
+
+	regmap_read(pl061->regmap, regs->dir, dir);
+	regmap_read(pl061->regmap, regs->is, &val);
+	regmap_read(pl061->regmap, regs->ibe, &val);
+	regmap_read(pl061->regmap, regs->iev, &val);
+	regmap_read(pl061->regmap, regs->ie, &val);
+}
+
 static int pl061_suspend(struct device *dev)
 {
 	struct pl061 *pl061 = dev_get_drvdata(dev);
-	unsigned int val = 0;
-	int offset;
-
-	pl061->csave_regs.gpio_data = 0;
-	regmap_read(pl061->regmap, GPIODIR, &val);
-	pl061->csave_regs.gpio_dir = val;
-	regmap_read(pl061->regmap, GPIOIS, &val);
-	pl061->csave_regs.gpio_is = val;
-	regmap_read(pl061->regmap, GPIOIBE, &val);
-	pl061->csave_regs.gpio_ibe = val;
-	regmap_read(pl061->regmap, GPIOIEV, &val);
-	pl061->csave_regs.gpio_iev = val;
-	regmap_read(pl061->regmap, GPIOIE, &val);
-	pl061->csave_regs.gpio_ie = val;
-
-	for (offset = 0; offset < PL061_GPIO_NR; offset++) {
-		if (pl061->csave_regs.gpio_dir & (BIT(offset)))
-			pl061->csave_regs.gpio_data |=
-				pl061_get_value(&pl061->gc, offset) << offset;
+	unsigned long dir_mask;
+	unsigned int dir = 0;
+	unsigned int offset;
+	unsigned int val;
+
+	if (!pl061->data->pm)
+		return 0;
+
+	pl061_cache_ctrl_regs(pl061, &dir);
+
+	pl061->saved_dat = 0;
+	dir_mask = dir;
+	for_each_set_bit(offset, &dir_mask, pl061->data->ngpio) {
+		val = 0;
+		regmap_read_bypassed(pl061->regmap,
+				     BIT(offset + PL061_DATA_OFFSET),
+				     &val);
+		pl061->saved_dat |= val;
 	}
 
 	return 0;
@@ -423,23 +485,28 @@ static int pl061_suspend(struct device *dev)
 static int pl061_resume(struct device *dev)
 {
 	struct pl061 *pl061 = dev_get_drvdata(dev);
-	int offset;
+	const struct pl061_regs *regs;
+	unsigned long dir_mask;
+	unsigned int dir = 0;
+	unsigned int offset;
 
-	for (offset = 0; offset < PL061_GPIO_NR; offset++) {
-		if (pl061->csave_regs.gpio_dir & (BIT(offset)))
-			pl061_direction_output(&pl061->gc, offset,
-					pl061->csave_regs.gpio_data &
-					(BIT(offset)));
-		else
-			pl061_direction_input(&pl061->gc, offset);
-	}
+	if (!pl061->data->pm)
+		return 0;
 
-	regmap_write(pl061->regmap, GPIOIS, pl061->csave_regs.gpio_is);
-	regmap_write(pl061->regmap, GPIOIBE, pl061->csave_regs.gpio_ibe);
-	regmap_write(pl061->regmap, GPIOIEV, pl061->csave_regs.gpio_iev);
-	regmap_write(pl061->regmap, GPIOIE, pl061->csave_regs.gpio_ie);
+	regs = pl061->data->regs;
 
-	return 0;
+	regmap_read(pl061->regmap, regs->dir, &dir);
+
+	regcache_mark_dirty(pl061->regmap);
+	regcache_sync_region(pl061->regmap, regs->dir, regs->dir);
+
+	dir_mask = dir;
+	for_each_set_bit(offset, &dir_mask, pl061->data->ngpio)
+		regmap_write(pl061->regmap,
+			     BIT(offset + PL061_DATA_OFFSET),
+			     !!(pl061->saved_dat & BIT(offset)) << offset);
+
+	return regcache_sync_region(pl061->regmap, regs->is, regs->ie);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(pl061_dev_pm_ops, pl061_suspend, pl061_resume);
@@ -448,6 +515,7 @@ static const struct amba_id pl061_ids[] = {
 	{
 		.id	= 0x00041061,
 		.mask	= 0x000fffff,
+		.data	= (void *)&pl061_arm,
 	},
 	{ 0, 0 },
 };

-- 
2.34.1

Re: [PATCH v10 7/8] gpio: pl061: switch data path to gpio-regmap

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: pl061: irq_domain use-after-free on teardown due to devres LIFO order
- [Medium] gpio: pl061: Missing IRQ resource helpers allows concurrent GPIO direction changes
--

commit 754bf2316c41a0da3dbb26f8d31b1ba0d0a17c9d
Author: Long Zhao <longzhao@ambarella.com>
Subject: gpio: pl061: switch data path to gpio-regmap

This patch moves the PL061 get, set, and direction operations to use
gpio-regmap. It keeps a custom irqchip and irq domain, creating the domain
with private data and attaching it via gpio-regmap.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 5c6d4209dc8e7..3822faa3e2768 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -325,67 +284,159 @@ static const struct irq_chip pl061_irq_chip = {
>  	.irq_set_wake		= pl061_irq_set_wake,
>  	.irq_print_chip		= pl061_irq_print_chip,
>  	.flags			= IRQCHIP_IMMUTABLE,
> -	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

[Severity: Medium]
Does removing GPIOCHIP_IRQ_RESOURCE_HELPERS leave the interrupt pins
unprotected against concurrent GPIO direction changes?

Without the .irq_request_resources and .irq_release_resources callbacks,
the gpiolib gpiochip_lock_as_irq() helper is never invoked when an interrupt
is requested. This means userspace or other subsystems could inadvertently
reconfigure an actively interrupting pin as an output.

While omitting the default helpers avoids crashing since
irq_data_get_irq_chip_data() now returns struct pl061 * rather than
struct gpio_chip *, are custom callbacks needed here to properly fetch
the gpio_chip and lock the line?

[ ... ]
> +	gpio_regmap = devm_gpio_regmap_register(dev, &config);
> +	if (IS_ERR(gpio_regmap)) {
> +		if (pl061->irq_domain)
> +			irq_domain_remove(pl061->irq_domain);
> +		return PTR_ERR(gpio_regmap);
> +	}
> +
> +	if (irq) {
> +		ret = devm_add_action_or_reset(dev, pl061_remove_irq, pl061);

[Severity: High]
Will this devres registration order cause a use-after-free during device
teardown?

The driver registers the GPIO chip via devm_gpio_regmap_register(), which
queues a devres action to unregister the chip via gpiochip_remove().

Later, devm_add_action_or_reset() is called to register pl061_remove_irq().

Since devres actions are unwound in LIFO order upon device unbind or probe
failure, pl061_remove_irq() will execute first and free the irq_domain:

pl061_remove_irq()
    ...
    irq_domain_remove(pl061->irq_domain);

Afterward, the gpio_regmap teardown action runs and calls gpiochip_remove().
This function iterates over active IRQs and dereferences the domain to
dispose of mappings:

gpiochip_remove()
    ... accesses gc->irq.domain ...

Since the domain was already freed by pl061_remove_irq(), does this result
in a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-cv75-gpio-v10-0-3fe3eb315a55@ambarella.com?part=7

[PATCH v10 8/8] gpio: pl061: add Ambarella register-layout variant

[Long Zhao via B4 Relay]

From: Long Zhao <longzhao@ambarella.com>

Add a 32-bit, 32-line layout with DAT/DIR/IS/IBE/IEV/IE/MIS/IC plus
MASK and ENABLE. Probe unmasks the bank and enables the controller.
IRQ type changes also clear the pending bit. This derivative does not
need write-after-direction or system suspend save/restore.

Cache DAT (REGCACHE_FLAT_S, not volatile) so gpio_set() updates one
bit from the output shadow instead of read-modify-writing the pin
level across the bank.

Match the part with designer AMBA_VENDOR_AMBARELLA (JEP106 identity
0x68, bit 7 set) and part number 0x061.

Reviewed-by: Linus Walleij <linusw@kernel.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
 drivers/gpio/gpio-pl061.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h  |  1 +
 2 files changed, 59 insertions(+)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 3822faa3e276..075af7985d89 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -38,6 +38,18 @@
 #define PL061_MIS		0x418
 #define PL061_IC		0x41c
 
+#define AMBARELLA_GPIO_NR	32
+#define AMBARELLA_DAT		0x00
+#define AMBARELLA_DIR		0x04
+#define AMBARELLA_IS		0x08
+#define AMBARELLA_IBE		0x0c
+#define AMBARELLA_IEV		0x10
+#define AMBARELLA_IE		0x14
+#define AMBARELLA_MIS		0x20
+#define AMBARELLA_IC		0x24
+#define AMBARELLA_MASK		0x28
+#define AMBARELLA_ENABLE	0x2c
+
 struct pl061_regs {
 	unsigned int dat;
 	unsigned int dir;
@@ -93,6 +105,17 @@ static bool pl061_arm_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
+static bool pl061_ambarella_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case AMBARELLA_MIS:
+	case AMBARELLA_IC:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static const struct regmap_config pl061_arm_regmap_config = {
 	.reg_bits = 16,
 	.val_bits = 8,
@@ -103,6 +126,16 @@ static const struct regmap_config pl061_arm_regmap_config = {
 	.use_raw_spinlock = true,
 };
 
+static const struct regmap_config pl061_ambarella_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = AMBARELLA_ENABLE,
+	.volatile_reg = pl061_ambarella_volatile_reg,
+	.cache_type = REGCACHE_FLAT_S,
+	.use_raw_spinlock = true,
+};
+
 static unsigned int pl061_line_mask(struct pl061 *pl061, irq_hw_number_t hwirq)
 {
 	return BIT(hwirq % pl061->data->ngpio);
@@ -338,6 +371,19 @@ static const struct pl061_regs pl061_arm_regs = {
 	.ic	= PL061_IC,
 };
 
+static const struct pl061_regs pl061_ambarella_regs = {
+	.dat	= AMBARELLA_DAT,
+	.dir	= AMBARELLA_DIR,
+	.is	= AMBARELLA_IS,
+	.ibe	= AMBARELLA_IBE,
+	.iev	= AMBARELLA_IEV,
+	.ie	= AMBARELLA_IE,
+	.mis	= AMBARELLA_MIS,
+	.ic	= AMBARELLA_IC,
+	.mask	= AMBARELLA_MASK,
+	.enable	= AMBARELLA_ENABLE,
+};
+
 static const struct pl061_drvdata pl061_arm = {
 	.regmap_config		= &pl061_arm_regmap_config,
 	.regs			= &pl061_arm_regs,
@@ -347,6 +393,13 @@ static const struct pl061_drvdata pl061_arm = {
 	.reg_mask_xlate		= pl061_arm_reg_mask_xlate,
 };
 
+static const struct pl061_drvdata pl061_ambarella = {
+	.regmap_config		= &pl061_ambarella_regmap_config,
+	.regs			= &pl061_ambarella_regs,
+	.ngpio			= AMBARELLA_GPIO_NR,
+	.clear_irq_on_type	= true,
+};
+
 static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	struct gpio_regmap_config config = {};
@@ -517,6 +570,11 @@ static const struct amba_id pl061_ids[] = {
 		.mask	= 0x000fffff,
 		.data	= (void *)&pl061_arm,
 	},
+	{
+		.id	= (AMBA_VENDOR_AMBARELLA << 12) | 0x061,
+		.mask	= 0x000fffff,
+		.data	= (void *)&pl061_ambarella,
+	},
 	{ 0, 0 },
 };
 MODULE_DEVICE_TABLE(amba, pl061_ids);
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 80a74cd2da7e..411533bfa61d 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -98,6 +98,7 @@ enum amba_vendor {
 	AMBA_VENDOR_ST = 0x80,
 	AMBA_VENDOR_QCOM = 0x51,
 	AMBA_VENDOR_LSI = 0xb6,
+	AMBA_VENDOR_AMBARELLA = 0xe8,
 };
 
 extern const struct bus_type amba_bustype;

-- 
2.34.1

Re: [PATCH v10 8/8] gpio: pl061: add Ambarella register-layout variant

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: devres LIFO order UAF on irq.domain during teardown
--

commit a177d737541186c7b1e51ef9f245ebda1871ffa1
Author: Long Zhao <longzhao@ambarella.com>

gpio: pl061: add Ambarella register-layout variant

Add a 32-bit, 32-line layout with DAT/DIR/IS/IBE/IEV/IE/MIS/IC plus
MASK and ENABLE. Probe unmasks the bank and enables the controller.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 3822faa3e2768..075af7985d893 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -347,6 +393,13 @@ static const struct pl061_drvdata pl061_arm = {
>  	.reg_mask_xlate		= pl061_arm_reg_mask_xlate,
>  };
>  
> +static const struct pl061_drvdata pl061_ambarella = {
> +	.regmap_config		= &pl061_ambarella_regmap_config,
> +	.regs			= &pl061_ambarella_regs,
> +	.ngpio			= AMBARELLA_GPIO_NR,
> +	.clear_irq_on_type	= true,
> +};
> +
>  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	struct gpio_regmap_config config = {};

[Severity: High]
Can this lead to a use-after-free during device teardown due to devres
ordering?

While this patch adds the new Ambarella variant, the shared pl061_probe()
function registers devres actions in an order that might be unsafe during
teardown.

In pl061_probe(), the gpiochip is registered first:

drivers/gpio/gpio-pl061.c:pl061_probe() {
        ...
        gpio_regmap = devm_gpio_regmap_register(dev, &config);
        ...
        ret = devm_add_action_or_reset(dev, pl061_remove_irq, pl061);
        ...
}

Because devres executes actions in reverse order of registration (LIFO),
pl061_remove_irq() will run first during a late probe failure or device
removal, calling irq_domain_remove() and freeing the IRQ domain.

At this point, the gpiochip is still registered. If a concurrent
call to gpiod_to_irq() acquires the valid gpio_chip, it will call into:

drivers/gpio/gpiolib.c:gpiochip_to_irq() {
        struct irq_domain *domain = gc->irq.domain;
        ...
        return irq_create_mapping(domain, offset);
}

Will this dereference the newly freed gc->irq.domain before the gpiochip
itself is unregistered? Should the IRQ teardown action be registered before
the gpiochip registration to ensure the chip is removed first?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-cv75-gpio-v10-0-3fe3eb315a55@ambarella.com?part=8