* [PATCH v13 01/13] dt-bindings: gpio: pl061: add Ambarella CV75 variant
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
@ 2026-11-02 6:45 ` Long Zhao via B4 Relay
2026-11-02 6:57 ` sashiko-bot
2026-11-02 6:45 ` [PATCH v13 03/13] gpio: pl061: drop redundant pending check in the IRQ handler Long Zhao via B4 Relay
` (11 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:45 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel, Conor Dooley
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>
Reviewed-by: Linus Walleij <linusw@kernel.org>
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v13 01/13] dt-bindings: gpio: pl061: add Ambarella CV75 variant
2026-11-02 6:45 ` [PATCH v13 01/13] dt-bindings: gpio: pl061: add Ambarella CV75 variant Long Zhao via B4 Relay
@ 2026-11-02 6:57 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-11-02 6:57 UTC (permalink / raw)
To: Long Zhao, linux-gpio; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: gpio: pl061: split incompatible ambarella,cv75-gpio schema
- [Low] dt-bindings: gpio: pl061: undocumented ambarella vendor prefix
--
commit 698f86de70b79c39ee77a80fb98df0823e4d3989
Author: Long Zhao <longzhao@ambarella.com>
dt-bindings: gpio: pl061: add Ambarella CV75 variant
This commit adds the ambarella,cv75-gpio compatible string and variant
constraints to the ARM PL061 GPIO controller Device Tree binding schema,
requiring gpio-ranges and a fixed arm,primecell-periphid of 0x000e8061.
> diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml b/Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
> index 4d970e55104bb..c22dd8a1d2c18 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
[Severity: Low]
As reported by Krzysztof Kozlowski on v1 [1], does the ambarella vendor
prefix in ambarella,cv75-gpio need to be documented in
Documentation/devicetree/bindings/vendor-prefixes.yaml first so that
checkpatch.pl and make dt_binding_check do not report an undocumented vendor
prefix?
> 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
[Severity: Medium]
Should ambarella,cv75-gpio be split into its own binding schema document
instead of adding conditional rules to
Documentation/devicetree/bindings/gpio/pl061-gpio.yaml?
As noted in the commit description, the Ambarella CV75 controller has 32-bit
registers and 32 lines per bank, adds MASK and ENABLE registers, is not
register-compatible with the 8-line ARM PL061, and does not implement a
standard PrimeCell ID in its hardware ID registers.
In Documentation/devicetree/bindings/gpio/pl061-gpio.yaml, the existing
properties are constrained for an 8-line PL061 block:
interrupts:
oneOf:
- maxItems: 1
- maxItems: 8
...
gpio-ranges:
minItems: 1
maxItems: 8
Requiring gpio-ranges for ambarella,cv75-gpio in this schema leaves the
8-item limit in place for a 32-line bank, and arm,primecell-periphid is only
defined inside the conditional then branch rather than top-level properties.
Given these hardware and schema differences, would a separate schema file for
ambarella,cv75-gpio be cleaner?
[1] https://lore.kernel.org/r/a8b0729c-d6ff-4fa0-955e-deb0af0e726b@kernel.org
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-cv75-gpio-v13-0-4f93114ea204@ambarella.com?part=1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v13 03/13] gpio: pl061: drop redundant pending check in the IRQ handler
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
2026-11-02 6:45 ` [PATCH v13 01/13] dt-bindings: gpio: pl061: add Ambarella CV75 variant Long Zhao via B4 Relay
@ 2026-11-02 6:45 ` Long Zhao via B4 Relay
2026-11-02 6:45 ` [PATCH v13 04/13] gpio: pl061: convert register access to regmap Long Zhao via B4 Relay
` (10 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:45 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
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.
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
drivers/gpio/gpio-pl061.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 919cf86fd590..721f14cf404e 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -219,11 +219,8 @@ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v13 04/13] gpio: pl061: convert register access to regmap
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
2026-11-02 6:45 ` [PATCH v13 01/13] dt-bindings: gpio: pl061: add Ambarella CV75 variant Long Zhao via B4 Relay
2026-11-02 6:45 ` [PATCH v13 03/13] gpio: pl061: drop redundant pending check in the IRQ handler Long Zhao via B4 Relay
@ 2026-11-02 6:45 ` Long Zhao via B4 Relay
2026-11-02 6:45 ` [PATCH v13 05/13] gpio: pl061: use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH Long Zhao via B4 Relay
` (9 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:45 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel, Chanho Min
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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 | 144 ++++++++++++++++++++++++++++++----------------
2 files changed, 94 insertions(+), 51 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 721f14cf404e..781bef987339 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;
@@ -85,20 +118,20 @@ 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);
unsigned long flags;
- unsigned char gpiodir;
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,16 +141,16 @@ 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)));
-
- return 0;
+ return regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
+ BIT(offset), value);
}
static int pl061_irq_type(struct irq_data *d, unsigned trigger)
@@ -125,9 +158,11 @@ 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 int bit = BIT(offset);
+ unsigned int gpioiev = 0;
+ unsigned int gpioibe = 0;
+ unsigned int gpiois = 0;
unsigned long flags;
- u8 gpiois, gpioibe, gpioiev;
- u8 bit = BIT(offset);
if (offset < 0 || offset >= PL061_GPIO_NR)
return -EINVAL;
@@ -145,9 +180,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 +234,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);
@@ -210,15 +245,17 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
static void pl061_irq_handler(struct irq_desc *desc)
{
- unsigned long pending;
- 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);
+ struct pl061 *pl061 = gpiochip_get_data(gc);
+ unsigned long pending;
+ unsigned int mis = 0;
+ int offset;
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);
@@ -230,11 +267,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);
@@ -245,13 +280,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);
}
@@ -269,9 +302,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)
@@ -303,17 +334,22 @@ static const struct irq_chip pl061_irq_chip = {
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;
+ struct pl061 *pl061;
+ 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;
@@ -332,7 +368,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");
@@ -363,14 +399,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)))
@@ -395,10 +437,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v13 05/13] gpio: pl061: use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (2 preceding siblings ...)
2026-11-02 6:45 ` [PATCH v13 04/13] gpio: pl061: convert register access to regmap Long Zhao via B4 Relay
@ 2026-11-02 6:45 ` Long Zhao via B4 Relay
2026-11-02 6:45 ` [PATCH v13 06/13] gpio: pl061: use cleanup helpers for locks Long Zhao via B4 Relay
` (8 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:45 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
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 781bef987339..e0ac8335bae4 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -167,9 +167,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",
@@ -184,7 +182,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v13 06/13] gpio: pl061: use cleanup helpers for locks
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (3 preceding siblings ...)
2026-11-02 6:45 ` [PATCH v13 05/13] gpio: pl061: use IRQ_TYPE_LEVEL_MASK and IRQ_TYPE_EDGE_BOTH Long Zhao via B4 Relay
@ 2026-11-02 6:45 ` Long Zhao via B4 Relay
2026-11-02 6:46 ` [PATCH v13 07/13] gpio: pl061: keep the parent device in a local Long Zhao via B4 Relay
` (7 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:45 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
From: Long Zhao <longzhao@ambarella.com>
Convert raw_spinlock critical sections to guard()() from cleanup.h.
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 | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index e0ac8335bae4..a232b63fbc0b 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;
}
@@ -119,9 +119,9 @@ static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
{
struct pl061 *pl061 = gpiochip_get_data(gc);
unsigned int mask = BIT(offset);
- unsigned long flags;
- 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;
}
@@ -162,7 +161,6 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
unsigned int gpioiev = 0;
unsigned int gpioibe = 0;
unsigned int gpiois = 0;
- unsigned long flags;
if (offset < 0 || offset >= PL061_GPIO_NR)
return -EINVAL;
@@ -175,8 +173,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);
@@ -236,8 +233,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;
}
@@ -266,9 +261,8 @@ 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);
- regmap_clear_bits(pl061->regmap, GPIOIE, mask);
- raw_spin_unlock(&pl061->lock);
+ scoped_guard(raw_spinlock, &pl061->lock)
+ regmap_clear_bits(pl061->regmap, GPIOIE, mask);
gpiochip_disable_irq(gc, d->hwirq);
}
@@ -281,9 +275,8 @@ static void pl061_irq_unmask(struct irq_data *d)
gpiochip_enable_irq(gc, d->hwirq);
- raw_spin_lock(&pl061->lock);
- regmap_set_bits(pl061->regmap, GPIOIE, mask);
- raw_spin_unlock(&pl061->lock);
+ scoped_guard(raw_spinlock, &pl061->lock)
+ regmap_set_bits(pl061->regmap, GPIOIE, mask);
}
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v13 07/13] gpio: pl061: keep the parent device in a local
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (4 preceding siblings ...)
2026-11-02 6:45 ` [PATCH v13 06/13] gpio: pl061: use cleanup helpers for locks Long Zhao via B4 Relay
@ 2026-11-02 6:46 ` Long Zhao via B4 Relay
2026-11-02 6:46 ` [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip Long Zhao via B4 Relay
` (6 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:46 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
From: Long Zhao <longzhao@ambarella.com>
IRQ logging uses gc->parent. Keep that device in a local so the
gpio-regmap switch can change where it comes from without rewriting
every log line.
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
drivers/gpio/gpio-pl061.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index a232b63fbc0b..27ed759486ca 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -156,6 +156,7 @@ 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 device *parent = gc->parent;
int offset = irqd_to_hwirq(d);
unsigned int bit = BIT(offset);
unsigned int gpioiev = 0;
@@ -166,7 +167,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
return -EINVAL;
if ((trigger & IRQ_TYPE_LEVEL_MASK) && (trigger & IRQ_TYPE_EDGE_BOTH)) {
- dev_err(gc->parent,
+ dev_err(parent,
"trying to configure line %d for both level and edge "
"detection, choose one!\n",
offset);
@@ -192,7 +193,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(parent, "line %d: IRQ on %s level\n",
offset,
polarity ? "HIGH" : "LOW");
} else if ((trigger & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
@@ -201,7 +202,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(parent, "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;
@@ -216,7 +217,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(parent, "line %d: IRQ on %s edge\n",
offset,
rising ? "RISING" : "FALLING");
} else {
@@ -225,8 +226,7 @@ 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",
- offset);
+ dev_warn(parent, "no trigger selected for line %d\n", offset);
}
regmap_write(pl061->regmap, GPIOIS, gpiois);
@@ -307,8 +307,9 @@ static int pl061_irq_set_wake(struct irq_data *d, unsigned int 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 device *parent = gc->parent;
- seq_puts(p, dev_name(gc->parent));
+ seq_puts(p, dev_name(parent));
}
static const struct irq_chip pl061_irq_chip = {
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (5 preceding siblings ...)
2026-11-02 6:46 ` [PATCH v13 07/13] gpio: pl061: keep the parent device in a local Long Zhao via B4 Relay
@ 2026-11-02 6:46 ` Long Zhao via B4 Relay
2026-11-02 8:33 ` Michael Walle
2026-11-03 17:46 ` Andy Shevchenko
2026-11-02 6:46 ` [PATCH v13 09/13] gpiolib: request ACPI GPIO events for external IRQ domains Long Zhao via B4 Relay
` (5 subsequent siblings)
12 siblings, 2 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:46 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
From: Long Zhao <longzhao@ambarella.com>
gpio_regmap_unregister() currently calls regmap_del_irq_chip() before
gpiochip_remove(). That disposes the mapped virtual IRQs and frees the
irq_domain while ACPI GPIO-signaled events may still be requested and
while gpiod_free_irqs() still needs gc->irq.domain.
Remove the gpio_chip first so ACPI teardown and USED_AS_IRQ cleanup run
against a live domain, then delete the regmap irqchip. Use the same
order on the registration error path, which previously leaked the
irqchip when gpiochip_irqchip_add_domain() failed after
regmap_add_irq_chip_fwnode().
Cc: Michael Walle <mwalle@kernel.org>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
drivers/gpio/gpio-regmap.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 0a44deae3d01..6b520bd80834 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -429,6 +429,11 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
err_remove_gpiochip:
gpiochip_remove(chip);
+
+#ifdef CONFIG_REGMAP_IRQ
+ if (gpio->irq_chip_data)
+ regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
+#endif
err_free_bitmap_output:
bitmap_free(gpio->fixed_direction_output);
err_free_bitmap_dirmask:
@@ -445,12 +450,13 @@ EXPORT_SYMBOL_GPL(gpio_regmap_register);
*/
void gpio_regmap_unregister(struct gpio_regmap *gpio)
{
+ gpiochip_remove(&gpio->gpio_chip);
+
#ifdef CONFIG_REGMAP_IRQ
if (gpio->irq_chip_data)
regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
#endif
- gpiochip_remove(&gpio->gpio_chip);
bitmap_free(gpio->fixed_direction_output);
bitmap_free(gpio->fixed_direction_mask);
kfree(gpio);
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip
2026-11-02 6:46 ` [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip Long Zhao via B4 Relay
@ 2026-11-02 8:33 ` Michael Walle
2026-11-03 17:50 ` Andy Shevchenko
2026-11-03 17:46 ` Andy Shevchenko
1 sibling, 1 reply; 21+ messages in thread
From: Michael Walle @ 2026-11-02 8:33 UTC (permalink / raw)
To: longzhao, Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Krzysztof Kozlowski, Krzysztof Kozlowski, Conor Dooley,
Mika Westerberg, linux-acpi, devicetree, Long Zhao, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]
On Mon Nov 2, 2026 at 7:46 AM CET, Long Zhao via B4 Relay wrote:
> From: Long Zhao <longzhao@ambarella.com>
>
> gpio_regmap_unregister() currently calls regmap_del_irq_chip() before
> gpiochip_remove(). That disposes the mapped virtual IRQs and frees the
> irq_domain while ACPI GPIO-signaled events may still be requested and
> while gpiod_free_irqs() still needs gc->irq.domain.
Maybe Andy have an opinion here. But that deviated from the usual
cleanup in the reverse order and at least deserves a comment in the
code.
> Remove the gpio_chip first so ACPI teardown and USED_AS_IRQ cleanup run
> against a live domain, then delete the regmap irqchip. Use the same
> order on the registration error path, which previously leaked the
> irqchip when gpiochip_irqchip_add_domain() failed after
> regmap_add_irq_chip_fwnode().
This should be split into a seperate patch with a Fixes: tag.
-michael
>
> Cc: Michael Walle <mwalle@kernel.org>
> Signed-off-by: Long Zhao <longzhao@ambarella.com>
> ---
> drivers/gpio/gpio-regmap.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 0a44deae3d01..6b520bd80834 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -429,6 +429,11 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>
> err_remove_gpiochip:
> gpiochip_remove(chip);
> +
> +#ifdef CONFIG_REGMAP_IRQ
> + if (gpio->irq_chip_data)
> + regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
> +#endif
> err_free_bitmap_output:
> bitmap_free(gpio->fixed_direction_output);
> err_free_bitmap_dirmask:
> @@ -445,12 +450,13 @@ EXPORT_SYMBOL_GPL(gpio_regmap_register);
> */
> void gpio_regmap_unregister(struct gpio_regmap *gpio)
> {
> + gpiochip_remove(&gpio->gpio_chip);
> +
> #ifdef CONFIG_REGMAP_IRQ
> if (gpio->irq_chip_data)
> regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
> #endif
>
> - gpiochip_remove(&gpio->gpio_chip);
> bitmap_free(gpio->fixed_direction_output);
> bitmap_free(gpio->fixed_direction_mask);
> kfree(gpio);
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip
2026-11-02 8:33 ` Michael Walle
@ 2026-11-03 17:50 ` Andy Shevchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-11-03 17:50 UTC (permalink / raw)
To: Michael Walle
Cc: longzhao, Bartosz Golaszewski, linux-gpio, Linus Walleij,
Rob Herring, Krzysztof Kozlowski, Krzysztof Kozlowski,
Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-acpi,
devicetree, Long Zhao, linux-kernel
On Mon, Nov 02, 2026 at 09:33:20AM +0100, Michael Walle wrote:
> On Mon Nov 2, 2026 at 7:46 AM CET, Long Zhao via B4 Relay wrote:
> >
> > gpio_regmap_unregister() currently calls regmap_del_irq_chip() before
> > gpiochip_remove(). That disposes the mapped virtual IRQs and frees the
> > irq_domain while ACPI GPIO-signaled events may still be requested and
> > while gpiod_free_irqs() still needs gc->irq.domain.
>
> Maybe Andy have an opinion here. But that deviated from the usual
> cleanup in the reverse order and at least deserves a comment in the
> code.
Yeah, this is unfortunate. I think to make it work as proposed the IRQ domain
handling in GPIOLIB should be revisited. So, at least a good comment is a must,
indeed.
> > Remove the gpio_chip first so ACPI teardown and USED_AS_IRQ cleanup run
> > against a live domain, then delete the regmap irqchip. Use the same
> > order on the registration error path, which previously leaked the
> > irqchip when gpiochip_irqchip_add_domain() failed after
> > regmap_add_irq_chip_fwnode().
>
> This should be split into a seperate patch with a Fixes: tag.
Not sure if we need a separate change here as two are coupled.
I leave it to Bart to decide.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip
2026-11-02 6:46 ` [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip Long Zhao via B4 Relay
2026-11-02 8:33 ` Michael Walle
@ 2026-11-03 17:46 ` Andy Shevchenko
1 sibling, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-11-03 17:46 UTC (permalink / raw)
To: longzhao
Cc: Bartosz Golaszewski, linux-gpio, Linus Walleij, Michael Walle,
Rob Herring, Krzysztof Kozlowski, Krzysztof Kozlowski,
Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-acpi,
devicetree, Long Zhao, linux-kernel
On Mon, Nov 02, 2026 at 02:46:01PM +0800, Long Zhao via B4 Relay wrote:
> gpio_regmap_unregister() currently calls regmap_del_irq_chip() before
> gpiochip_remove(). That disposes the mapped virtual IRQs and frees the
> irq_domain while ACPI GPIO-signaled events may still be requested and
> while gpiod_free_irqs() still needs gc->irq.domain.
>
> Remove the gpio_chip first so ACPI teardown and USED_AS_IRQ cleanup run
> against a live domain, then delete the regmap irqchip. Use the same
> order on the registration error path, which previously leaked the
> irqchip when gpiochip_irqchip_add_domain() failed after
> regmap_add_irq_chip_fwnode().
...
> Cc: Michael Walle <mwalle@kernel.org>
Use --cc in the command line for `git format-patch` or at least move Cc list to
be after the --- line. This will reduce a noise in the commit message.
...
> +#ifdef CONFIG_REGMAP_IRQ
> + if (gpio->irq_chip_data)
> + regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
> +#endif
> #ifdef CONFIG_REGMAP_IRQ
> if (gpio->irq_chip_data)
> regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
> #endif
Now it seems a dup, make it a helper, like
static void gpio_regmap_del_irq_chip(... *gpio)
{
#ifdef CONFIG_REGMAP_IRQ
if (gpio->irq_chip_data)
regmap_del_irq_chip(gpio->regmap_irq_line, gpio->irq_chip_data);
#endif
}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v13 09/13] gpiolib: request ACPI GPIO events for external IRQ domains
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (6 preceding siblings ...)
2026-11-02 6:46 ` [PATCH v13 08/13] gpio: regmap: remove gpiochip before tearing down irqchip Long Zhao via B4 Relay
@ 2026-11-02 6:46 ` Long Zhao via B4 Relay
2026-11-03 11:58 ` Andy Shevchenko
2026-11-02 6:46 ` [PATCH v13 11/13] gpio: pl061: add Ambarella register-layout variant Long Zhao via B4 Relay
` (4 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:46 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
From: Long Zhao <longzhao@ambarella.com>
gpiochip_add_irqchip() requests GPIO-signaled ACPI events after
installing the irqchip. A gpiochip that gets its IRQ domain through
gpiochip_irqchip_add_domain() never goes through that path, so _AEI
events, such as the power button on ARM64 ACPI platforms, are not
requested.
Request them here as well. gpiochip_remove() already calls
acpi_gpiochip_free_interrupts().
Cc: Mika Westerberg <westeri@kernel.org>
Cc: linux-acpi@vger.kernel.org
Acked-by: Mika Westerberg (Intel) <westeri@kernel.org>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
drivers/gpio/gpiolib.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e5fb60111151..c708d81ef243 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2353,7 +2353,14 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc)
int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
struct irq_domain *domain)
{
- return gpiochip_irqchip_add_allocated_domain(gc, domain, true);
+ int ret;
+
+ ret = gpiochip_irqchip_add_allocated_domain(gc, domain, true);
+ if (ret)
+ return ret;
+
+ acpi_gpiochip_request_interrupts(gc);
+ return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_domain);
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v13 09/13] gpiolib: request ACPI GPIO events for external IRQ domains
2026-11-02 6:46 ` [PATCH v13 09/13] gpiolib: request ACPI GPIO events for external IRQ domains Long Zhao via B4 Relay
@ 2026-11-03 11:58 ` Andy Shevchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-11-03 11:58 UTC (permalink / raw)
To: longzhao
Cc: Bartosz Golaszewski, linux-gpio, Linus Walleij, Michael Walle,
Rob Herring, Krzysztof Kozlowski, Krzysztof Kozlowski,
Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-acpi,
devicetree, Long Zhao, linux-kernel
On Mon, Nov 02, 2026 at 02:46:02PM +0800, Long Zhao via B4 Relay wrote:
> gpiochip_add_irqchip() requests GPIO-signaled ACPI events after
> installing the irqchip. A gpiochip that gets its IRQ domain through
> gpiochip_irqchip_add_domain() never goes through that path, so _AEI
> events, such as the power button on ARM64 ACPI platforms, are not
> requested.
>
> Request them here as well. gpiochip_remove() already calls
> acpi_gpiochip_free_interrupts().
> Cc: Mika Westerberg <westeri@kernel.org>
First of all, Cc is a dup when other trailer for the same person is present.
> Cc: linux-acpi@vger.kernel.org
Second, move Cc: list to be after the '---' line, it will reduce the noise in
the commit message while sending to the same list of people.
> Acked-by: Mika Westerberg (Intel) <westeri@kernel.org>
> Signed-off-by: Long Zhao <longzhao@ambarella.com>
...
> int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
> struct irq_domain *domain)
> {
> - return gpiochip_irqchip_add_allocated_domain(gc, domain, true);
> + int ret;
> +
> + ret = gpiochip_irqchip_add_allocated_domain(gc, domain, true);
> + if (ret)
> + return ret;
> +
> + acpi_gpiochip_request_interrupts(gc);
In such a case where is the freeing these requested interrupts?
> + return 0;
> }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v13 11/13] gpio: pl061: add Ambarella register-layout variant
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (7 preceding siblings ...)
2026-11-02 6:46 ` [PATCH v13 09/13] gpiolib: request ACPI GPIO events for external IRQ domains Long Zhao via B4 Relay
@ 2026-11-02 6:46 ` Long Zhao via B4 Relay
2026-11-02 6:46 ` [PATCH v13 12/13] gpio: pl061: tidy up IRQ trigger logging Long Zhao via B4 Relay
` (3 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:46 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
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 cb1ef0f3774c..ff9215447bbb 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -39,6 +39,18 @@
#define PL061_GPIO_NR 8
#define PL061_DATA_OFFSET 2
+#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);
@@ -336,6 +369,19 @@ static const struct pl061_regs pl061_arm_regs = {
.ic = GPIOIC,
};
+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,
@@ -345,6 +391,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 = {};
@@ -521,6 +574,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v13 12/13] gpio: pl061: tidy up IRQ trigger logging
2026-11-02 6:45 [PATCH v13 00/13] gpio: add Ambarella CV75 PL061 support Long Zhao via B4 Relay
` (8 preceding siblings ...)
2026-11-02 6:46 ` [PATCH v13 11/13] gpio: pl061: add Ambarella register-layout variant Long Zhao via B4 Relay
@ 2026-11-02 6:46 ` Long Zhao via B4 Relay
2026-11-03 16:53 ` Andy Shevchenko
[not found] ` <20261102-cv75-gpio-v13-2-4f93114ea204@ambarella.com>
` (2 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Long Zhao via B4 Relay @ 2026-11-02 6:46 UTC (permalink / raw)
To: Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Michael Walle, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Krzysztof Kozlowski, Krzysztof Kozlowski,
Conor Dooley, Mika Westerberg, linux-acpi, devicetree, Long Zhao,
Long Zhao, linux-kernel
From: Long Zhao <longzhao@ambarella.com>
Join the level-and-edge dev_err() into one string literal so a grep
for "edge detection" finds it.
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Long Zhao <longzhao@ambarella.com>
---
drivers/gpio/gpio-pl061.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index ff9215447bbb..63ec5363dd5f 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -170,8 +170,7 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
if ((trigger & IRQ_TYPE_LEVEL_MASK) && (trigger & IRQ_TYPE_EDGE_BOTH)) {
dev_err(parent,
- "trying to configure line %d for both level and edge "
- "detection, choose one!\n",
+ "trying to configure line %d for both level and edge detection, choose one!\n",
offset);
return -EINVAL;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v13 12/13] gpio: pl061: tidy up IRQ trigger logging
2026-11-02 6:46 ` [PATCH v13 12/13] gpio: pl061: tidy up IRQ trigger logging Long Zhao via B4 Relay
@ 2026-11-03 16:53 ` Andy Shevchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-11-03 16:53 UTC (permalink / raw)
To: longzhao
Cc: Bartosz Golaszewski, linux-gpio, Linus Walleij, Michael Walle,
Rob Herring, Krzysztof Kozlowski, Krzysztof Kozlowski,
Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-acpi,
devicetree, Long Zhao, linux-kernel
On Mon, Nov 02, 2026 at 02:46:05PM +0800, Long Zhao via B4 Relay wrote:
> Join the level-and-edge dev_err() into one string literal so a grep
> for "edge detection" finds it.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20261102-cv75-gpio-v13-2-4f93114ea204@ambarella.com>]
* Re: [PATCH v13 02/13] gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR
[not found] ` <20261102-cv75-gpio-v13-2-4f93114ea204@ambarella.com>
@ 2026-11-02 7:01 ` sashiko-bot
2026-11-02 8:35 ` Michael Walle
1 sibling, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-11-02 7:01 UTC (permalink / raw)
To: Long Zhao, linux-gpio; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpio: regmap: unchecked gpio_regmap_set() in gpio_regmap_direction_output()
--
commit eae7ad18ef30360e98f853dbc86b39ea56d2da3f
Author: Long Zhao <longzhao@ambarella.com>
gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR
This commit adds a quirk flag to struct gpio_regmap_config and struct
gpio_regmap so that gpio_regmap_direction_output() writes the output value
after switching the line direction to output for controllers that ignore
output writes while a line is configured as an input.
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 51b4d69b87403..0a44deae3d01e 100644
> --- 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);
> +
> + return 0;
> }
[Severity: Medium]
As reported by Sashiko on v11 [1], should gpio_regmap_direction_output()
check and propagate the return value of gpio_regmap_set()?
In drivers/gpio/gpio-regmap.c, gpio_regmap_set() returns a negative error
code if gpio->reg_mask_xlate() fails or if regmap_write_bits() or
regmap_update_bits() fails:
drivers/gpio/gpio-regmap.c:gpio_regmap_set() {
ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &mask);
if (ret)
return ret;
...
if (gpio->reg_dat_base == gpio->reg_set_base)
ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
else
ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
return ret;
}
When the quirk is not set, a failure in gpio_regmap_set() is ignored and
gpio_regmap_direction_output() still proceeds to call
gpio_regmap_set_direction(), enabling the output with a stale value. When
the quirk is set, a failure in gpio_regmap_set() after changing the
direction is also discarded and 0 is returned to the caller.
[1] https://lore.kernel.org/r/20261020-cv75-gpio-v11-2-e058bae19571@ambarella.com
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-cv75-gpio-v13-0-4f93114ea204@ambarella.com?part=2
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v13 02/13] gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR
[not found] ` <20261102-cv75-gpio-v13-2-4f93114ea204@ambarella.com>
2026-11-02 7:01 ` [PATCH v13 02/13] gpiolib: regmap: add GPIO_REGMAP_QUIRK_SET_AFTER_DIR sashiko-bot
@ 2026-11-02 8:35 ` Michael Walle
1 sibling, 0 replies; 21+ messages in thread
From: Michael Walle @ 2026-11-02 8:35 UTC (permalink / raw)
To: longzhao, Bartosz Golaszewski, linux-gpio
Cc: Linus Walleij, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Krzysztof Kozlowski, Krzysztof Kozlowski, Conor Dooley,
Mika Westerberg, linux-acpi, devicetree, Long Zhao, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 680 bytes --]
On Mon Nov 2, 2026 at 7:45 AM CET, Long Zhao via B4 Relay wrote:
> 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>
> Reviewed-by: Linus Walleij <linusw@kernel.org>
> Signed-off-by: Long Zhao <longzhao@ambarella.com>
Nitpick, if you'll send a new version: I've just noticed the
subject. It should be "gpio: regmap:" not gpiolib.
-michael
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20261102-cv75-gpio-v13-10-4f93114ea204@ambarella.com>]
* Re: [PATCH v13 10/13] gpio: pl061: switch data path to gpio-regmap
[not found] ` <20261102-cv75-gpio-v13-10-4f93114ea204@ambarella.com>
@ 2026-11-02 7:10 ` sashiko-bot
0 siblings, 0 replies; 21+ messages in thread
From: sashiko-bot @ 2026-11-02 7:10 UTC (permalink / raw)
To: Long Zhao, linux-gpio; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: pl061: dropping IRQ resource helpers breaks IRQ locking and unbind
--
commit 074c89211872cf47652e8bf1f251d42902b7bc3b
Author: Long Zhao <longzhao@ambarella.com>
gpio: pl061: switch data path to gpio-regmap
This commit switches the PL061 GPIO data path callbacks (get, set, and
direction) to use the gpio-regmap helper library while keeping a custom
irq_chip and instantiating an external irq_domain for gpio-regmap to attach.
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 27ed759486caf..cb1ef0f3774c1 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
[ ... ]
> @@ -229,54 +198,47 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
[ ... ]
> 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);
> + struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
> + unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
>
> - scoped_guard(raw_spinlock, &pl061->lock)
> - 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);
> + struct pl061 *pl061 = irq_data_get_irq_chip_data(d);
> + unsigned int mask = pl061_line_mask(pl061, irqd_to_hwirq(d));
>
> - scoped_guard(raw_spinlock, &pl061->lock)
> - regmap_set_bits(pl061->regmap, GPIOIE, mask);
> + regmap_set_bits(pl061->regmap, pl061->data->regs->ie, mask);
> }
[ ... ]
> @@ -320,67 +280,169 @@ 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,
> +};
[ ... ]
> + if (irq) {
> + const struct irq_domain_info info = {
> + .fwnode = dev_fwnode(dev),
> + .size = data->ngpio,
> + .hwirq_max = data->ngpio,
> + .ops = &pl061_irq_domain_ops,
> + .host_data = pl061,
> + };
> +
> + pl061->irq_domain = devm_irq_domain_instantiate(dev, &info);
> + if (IS_ERR(pl061->irq_domain))
> + return PTR_ERR(pl061->irq_domain);
> +
> + /*
> + * Registered before the gpiochip so unbind runs it after
> + * gpiochip_free_remaining_irqs(), while the domain is still
> + * alive. devm removes the domain after this action.
> + */
> + ret = devm_add_action_or_reset(dev, pl061_dispose_irq_mappings, pl061);
> + if (ret)
> + return ret;
> + }
[Severity: High]
As reported by Sashiko on v11 [1], does dropping GPIOCHIP_IRQ_RESOURCE_HELPERS
from pl061_irq_chip and removing gpiochip_disable_irq() and
gpiochip_enable_irq() from pl061_irq_mask() and pl061_irq_unmask() introduce
regressions during device unbind and GPIO direction changes?
Without gpiochip_reqres_irq() and gpiochip_relres_irq(),
GPIOD_FLAG_USED_AS_IRQ and GPIOD_FLAG_IRQ_IS_ENABLED are never set on the GPIO
descriptors.
On device unbind while an interrupt is requested (for example via userspace
cdev edge detection in edge_detector_setup() -> request_threaded_irq() or an
in-kernel consumer), devm_gpio_regmap_unregister() -> gpiochip_remove() ->
gpiochip_free_remaining_irqs() only frees descriptors that have
GPIOD_FLAG_USED_AS_IRQ set:
drivers/gpio/gpiolib.c:gpiochip_free_remaining_irqs() {
for_each_gpio_desc_with_flag(gc, desc, GPIOD_FLAG_USED_AS_IRQ) {
...
}
}
Because that flag is never set, gpiochip_free_remaining_irqs() becomes a no-op
despite the comment in pl061_probe() above devm_add_action_or_reset(). Next,
pl061_dispose_irq_mappings() calls irq_dispose_mapping() -> irq_free_desc()
while desc->action and any threaded handler are still active. This triggers a
warning in remove_proc_entry() on /proc/irq/<virq> and frees the irq_desc.
When the consumer later calls free_irq() (such as on closing the GPIO character
device file descriptor), irq_to_desc(virq) returns NULL, leaking the irqaction
and leaving the edge_irq_thread kthread running with a dangling dev_id pointer
to the freed struct line.
In addition, without these helpers:
- gpiochip_lock_as_irq() is not called when an interrupt is requested, so
requesting an IRQ on a line configured as a push-pull output is no longer
rejected.
- gpiod_direction_output() no longer prevents switching a line with an active
interrupt to output mode.
- Module reference counting in gpiochip_reqres_irq() and gpiochip_relres_irq()
is bypassed.
Although the commit message compares this with gpio-idio-16,
gpio-pcie-idio-24, and gpio-104-idi-48, those drivers use fixed-direction
inputs for their interrupt-capable lines and did not previously use
GPIOCHIP_IRQ_RESOURCE_HELPERS, whereas PL061 lines have software-configurable
direction.
[1] https://lore.kernel.org/r/20261020-cv75-gpio-v11-8-e058bae19571@ambarella.com
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-cv75-gpio-v13-0-4f93114ea204@ambarella.com?part=10
^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <20261102-cv75-gpio-v13-13-4f93114ea204@ambarella.com>]
* Re: [PATCH v13 13/13] gpio: pl061: use string_choices helpers for IRQ trigger logging
[not found] ` <20261102-cv75-gpio-v13-13-4f93114ea204@ambarella.com>
@ 2026-11-03 16:54 ` Andy Shevchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2026-11-03 16:54 UTC (permalink / raw)
To: longzhao
Cc: Bartosz Golaszewski, linux-gpio, Linus Walleij, Michael Walle,
Rob Herring, Krzysztof Kozlowski, Krzysztof Kozlowski,
Krzysztof Kozlowski, Conor Dooley, Mika Westerberg, linux-acpi,
devicetree, Long Zhao, linux-kernel
On Mon, Nov 02, 2026 at 02:46:06PM +0800, Long Zhao via B4 Relay wrote:
> Add str_rising_falling() next to the other str_* helpers and use
> str_high_low() / str_rising_falling() for the level and edge debug
> lines.
It should be two separate patches. Otherwise LGTM.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread