[PATCH 0/9] gpio: mxc: bug fixes and probe cleanup

[PATCH 0/9] gpio: mxc: bug fixes and probe cleanup

[Peng Fan (OSS)]

Two bug fixes and follows up with probe modernization and bitops cleanup.

We not receive bug report and met issues during our test, so patch 1&2
are not critial for now.

Patches 1-2 are bug fixes:

  1. Fix a race where the chained IRQ handler is installed before the
     IRQ domain, generic IRQ chip, and port list entry are ready.
     Also fixes a latent use-after-free on probe failure paths that
     never unregistered the handler.

  2. Fix wakeup_pads being typed as u32 while accessed through
     set_bit/clear_bit (unsigned long *), causing adjacent field
     corruption on 64-bit platforms. Switch to atomic bitops for
     concurrency safety and fix the disable path to preserve the
     wakeup_pads bit on disable_irq_wake() failure.

Patches 3-9 are cleanups, each building on the previous:

  3. Cache of_device_is_compatible() results at probe into struct
     fields, avoiding repeated DT string comparisons in suspend/resume.

  4. Use devm_add_action_or_reset() for irq_domain_remove(), removing
     manual cleanup in error paths.

  5. Switch to devm-managed PM runtime and dev_err_probe(), eliminating
     the remaining goto error labels entirely.

  6. Introduce a local 'dev' variable and migrate to
     device_is_compatible() for firmware-agnostic matching.

  7. Introduce MXC_ICR_REG/MXC_ICR_MASK macros and use
     field_prep()/field_get() for ICR register access, replacing
     duplicated magic-number arithmetic in gpio_set_irq_type() and
     mxc_flip_edge().

  8. Replace open-coded '1 << n' with BIT() throughout the driver.

  9. Simplify gpio_set_wake_irq() using irq_set_irq_wake() and
     assign_bit().

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (9):
      gpio: mxc: fix race between chained IRQ handler install and probe completion
      gpio: mxc: fix wakeup_pads bit operations for correctness
      gpio: mxc: cache compatible checks at probe time
      gpio: mxc: use devm action for irq_domain cleanup
      gpio: mxc: use devres-managed PM runtime and dev_err_probe
      gpio: mxc: use local dev variable and device_is_compatible()
      gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get
      gpio: mxc: use BIT() macro for single-bit operations
      gpio: mxc: simplify gpio_set_wake_irq() with irq_set_irq_wake and assign_bit

 drivers/gpio/gpio-mxc.c | 158 ++++++++++++++++++++++++------------------------
 1 file changed, 79 insertions(+), 79 deletions(-)
---
base-commit: f0406245cb9855e6318335a8a223551354291a46
change-id: 20261003-gpio-mxc-cleanup-e49cc626c51e

Best regards,
--  
Peng Fan <peng.fan@nxp.com>

[PATCH 1/9] gpio: mxc: fix race between chained IRQ handler install and probe completion

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

mxc_update_irq_chained_handler() is called before the IRQ domain, the
generic IRQ chip, and the port list entry are set up. If an interrupt
arrives in that window:

 - mx3_gpio_irq_handler() calls generic_handle_domain_irq() with
   port->domain still NULL.
 - mx2_gpio_irq_handler() walks mxc_gpio_ports, but the port has not
   been added to the list yet.

Additionally, if any of the subsequent probe steps (gpio_generic_chip_init,
devm_gpiochip_add_data, irq_domain_create_legacy, or mxc_gpio_init_gc)
fail, the error paths never unregister the chained handler, leaving a
dangling handler that points at freed memory.

Move the handler installation after all its dependencies are ready and
after list_add_tail(), so the handler is never live while the data
structures it touches are incomplete, and is never installed if probe
fails.

Fixes: 5f6d1998adeb ("gpio: mxc: release the parent IRQ in runtime suspend")
Assisted-by: LLM
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 7e2690d92df6..e05f276a50e8 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -474,8 +474,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	} else
 		port->mx_irq_handler = mx3_gpio_irq_handler;
 
-	mxc_update_irq_chained_handler(port, true);
-
 	config.dev = &pdev->dev;
 	config.sz = 4;
 	config.dat = port->base + GPIO_PSR;
@@ -525,6 +523,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 
 	list_add_tail(&port->node, &mxc_gpio_ports);
 
+	mxc_update_irq_chained_handler(port, true);
+
 	platform_set_drvdata(pdev, port);
 	pm_runtime_put_autosuspend(&pdev->dev);
 

-- 
2.51.0

Re: [PATCH 1/9] gpio: mxc: fix race between chained IRQ handler install and probe completion

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> mxc_update_irq_chained_handler() is called before the IRQ domain, the
> generic IRQ chip, and the port list entry are set up. If an interrupt
> arrives in that window:
>
>  - mx3_gpio_irq_handler() calls generic_handle_domain_irq() with
>    port->domain still NULL.
>  - mx2_gpio_irq_handler() walks mxc_gpio_ports, but the port has not
>    been added to the list yet.
>
> Additionally, if any of the subsequent probe steps (gpio_generic_chip_init,
> devm_gpiochip_add_data, irq_domain_create_legacy, or mxc_gpio_init_gc)

We refer to the functions as func(), like you have done above, but here...
(No need to resend just for this.)

> fail, the error paths never unregister the chained handler, leaving a
> dangling handler that points at freed memory.
>
> Move the handler installation after all its dependencies are ready and
> after list_add_tail(), so the handler is never live while the data
> structures it touches are incomplete, and is never installed if probe
> fails.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/9] gpio: mxc: fix wakeup_pads bit operations for correctness

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Replace the open-coded BIT() / mask operations with the atomic
set_bit() / clear_bit() / test_bit() API. Atomic variants are required
because gpio_set_wake_irq() can be called concurrently for different
pins on the same port - irq_set_irq_wake() only holds the per-IRQ
descriptor lock, not a per-port lock, so concurrent modification of
different bits in wakeup_pads is possible.

However wakeup_pads field is typed as u32 but accessed via set_bit() /
clear_bit() / test_bit() which operate on unsigned long pointers. On
64-bit platforms this causes an 8-byte read-modify-write on a 4-byte
field, corrupting the adjacent is_pad_wakeup field.

Change wakeup_pads from u32 to unsigned long to match the bitops API
width requirements.

Also fix the disable path to only clear the wakeup_pads bit when
disable_irq_wake() succeeds, matching the enable path which already
checks the return value. Previously, a failed disable_irq_wake() would
still clear the bit, causing the driver to lose track of the wakeup
source.

Also fix a latent signed-shift bug: the old (1 << i) expression in
mxc_gpio_set_pad_wakeup() has implementation-defined behavior when
i == 31, since 1 is a signed int.

Fixes: f60c9eac54af ("gpio: mxc: enable pad wakeup on i.MX8x platforms")
Assisted-by: LLM
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index e05f276a50e8..8a755ac1af83 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -71,7 +71,7 @@ struct mxc_gpio_port {
 	u32 both_edges;
 	struct mxc_gpio_reg_saved gpio_saved_reg;
 	bool power_off;
-	u32 wakeup_pads;
+	unsigned long wakeup_pads;
 	bool is_pad_wakeup;
 	u32 pad_type[32];
 	const struct mxc_gpio_hwdata *hwdata;
@@ -330,13 +330,15 @@ static int gpio_set_wake_irq(struct irq_data *d, u32 enable)
 			ret = enable_irq_wake(port->irq_high);
 		else
 			ret = enable_irq_wake(port->irq);
-		port->wakeup_pads |= BIT(gpio_idx);
+		if (!ret)
+			set_bit(gpio_idx, &port->wakeup_pads);
 	} else {
 		if (port->irq_high && (gpio_idx >= 16))
 			ret = disable_irq_wake(port->irq_high);
 		else
 			ret = disable_irq_wake(port->irq);
-		port->wakeup_pads &= ~BIT(gpio_idx);
+		if (!ret)
+			clear_bit(gpio_idx, &port->wakeup_pads);
 	}
 
 	return ret;
@@ -599,7 +601,7 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
 	};
 
 	for (i = 0; i < 32; i++) {
-		if ((port->wakeup_pads & (1 << i))) {
+		if (test_bit(i, &port->wakeup_pads)) {
 			type = port->pad_type[i];
 			if (enable)
 				config = pad_type_map[type];

-- 
2.51.0

Re: [PATCH 2/9] gpio: mxc: fix wakeup_pads bit operations for correctness

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> Replace the open-coded BIT() / mask operations with the atomic
> set_bit() / clear_bit() / test_bit() API. Atomic variants are required
> because gpio_set_wake_irq() can be called concurrently for different
> pins on the same port - irq_set_irq_wake() only holds the per-IRQ
> descriptor lock, not a per-port lock, so concurrent modification of
> different bits in wakeup_pads is possible.
>
> However wakeup_pads field is typed as u32 but accessed via set_bit() /
> clear_bit() / test_bit() which operate on unsigned long pointers. On
> 64-bit platforms this causes an 8-byte read-modify-write on a 4-byte
> field, corrupting the adjacent is_pad_wakeup field.
>
> Change wakeup_pads from u32 to unsigned long to match the bitops API
> width requirements.
>
> Also fix the disable path to only clear the wakeup_pads bit when
> disable_irq_wake() succeeds, matching the enable path which already
> checks the return value. Previously, a failed disable_irq_wake() would
> still clear the bit, causing the driver to lose track of the wakeup
> source.

The above is too verbose, try to squeeze it to the point.

> Also fix a latent signed-shift bug: the old (1 << i) expression in
> mxc_gpio_set_pad_wakeup() has implementation-defined behavior when
> i == 31, since 1 is a signed int.

Too many words for a simple (non-critical) update.

...

>  struct mxc_gpio_port {
>         u32 both_edges;
>         struct mxc_gpio_reg_saved gpio_saved_reg;
>         bool power_off;
> -       u32 wakeup_pads;
> +       unsigned long wakeup_pads;
>         bool is_pad_wakeup;
>         u32 pad_type[32];
>         const struct mxc_gpio_hwdata *hwdata;

While at it, run `pahole` and update the arrangement (of the members
you touched here) accordingly.

...

> static int gpio_set_wake_irq(struct irq_data *d, u32 enable)

>                         ret = enable_irq_wake(port->irq_high);
>                 else
>                         ret = enable_irq_wake(port->irq);
> -               port->wakeup_pads |= BIT(gpio_idx);
> +               if (!ret)
> +                       set_bit(gpio_idx, &port->wakeup_pads);
>         } else {
>                 if (port->irq_high && (gpio_idx >= 16))
>                         ret = disable_irq_wake(port->irq_high);
>                 else
>                         ret = disable_irq_wake(port->irq);
> -               port->wakeup_pads &= ~BIT(gpio_idx);
> +               if (!ret)
> +                       clear_bit(gpio_idx, &port->wakeup_pads);
>         }
>
>         return ret;

Instead do the following after the if (enable) {} else {} block, namely

  if (ret)
    return ret;

  assign_bit(..., enable)
  return 0;

...

>  static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)

>         for (i = 0; i < 32; i++) {
> -               if ((port->wakeup_pads & (1 << i))) {
> +               if (test_bit(i, &port->wakeup_pads)) {

Instead just start using for_each_set_bits() from bitops.h.

>                         type = port->pad_type[i];
>                         if (enable)
>                                 config = pad_type_map[type];

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/9] gpio: mxc: cache compatible checks at probe time

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

mxc_gpio_generic_config() and mxc_gpio_set_pad_wakeup() call
of_device_is_compatible() on every invocation to determine pad wakeup
capability and i.MX8QM-specific behavior. These properties are
invariant for the lifetime of the device.

Cache them as bool fields (has_pad_wakeup, is_imx8qm) in mxc_gpio_port
during probe, eliminating repeated device tree string comparisons in
the suspend/resume hot path.

Assisted-by: LLM
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 8a755ac1af83..1c27232f6a80 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -73,6 +73,8 @@ struct mxc_gpio_port {
 	bool power_off;
 	unsigned long wakeup_pads;
 	bool is_pad_wakeup;
+	bool has_pad_wakeup;
+	bool is_imx8qm;
 	u32 pad_type[32];
 	const struct mxc_gpio_hwdata *hwdata;
 };
@@ -457,6 +459,14 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (of_device_is_compatible(np, "fsl,imx7d-gpio"))
 		port->power_off = true;
 
+	if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
+	    of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
+	    of_device_is_compatible(np, "fsl,imx8qm-gpio"))
+		port->has_pad_wakeup = true;
+
+	if (of_device_is_compatible(np, "fsl,imx8qm-gpio"))
+		port->is_imx8qm = true;
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
@@ -570,15 +580,10 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port *port)
 static bool mxc_gpio_generic_config(struct mxc_gpio_port *port,
 		unsigned int offset, unsigned long conf)
 {
-	struct device_node *np = port->dev->of_node;
-
-	if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
-	    of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
-	    of_device_is_compatible(np, "fsl,imx8qm-gpio"))
-		return (gpiochip_generic_config(&port->gen_gc.gc,
-						offset, conf) == 0);
+	if (!port->has_pad_wakeup)
+		return false;
 
-	return false;
+	return (gpiochip_generic_config(&port->gen_gc.gc, offset, conf) == 0);
 }
 
 static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
@@ -586,7 +591,6 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
 	unsigned long config;
 	bool ret = false;
 	int i, type;
-	bool is_imx8qm = of_device_is_compatible(port->dev->of_node, "fsl,imx8qm-gpio");
 
 	static const u32 pad_type_map[] = {
 		IMX_SCU_WAKEUP_OFF,		/* 0 */
@@ -608,7 +612,7 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
 			else
 				config = IMX_SCU_WAKEUP_OFF;
 
-			if (is_imx8qm && config == IMX_SCU_WAKEUP_FALL_EDGE) {
+			if (port->is_imx8qm && config == IMX_SCU_WAKEUP_FALL_EDGE) {
 				dev_warn_once(port->dev,
 					      "No falling-edge support for wakeup on i.MX8QM\n");
 				config = IMX_SCU_WAKEUP_OFF;

-- 
2.51.0

Re: [PATCH 3/9] gpio: mxc: cache compatible checks at probe time

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> mxc_gpio_generic_config() and mxc_gpio_set_pad_wakeup() call
> of_device_is_compatible() on every invocation to determine pad wakeup
> capability and i.MX8QM-specific behavior. These properties are
> invariant for the lifetime of the device.
>
> Cache them as bool fields (has_pad_wakeup, is_imx8qm) in mxc_gpio_port
> during probe, eliminating repeated device tree string comparisons in
> the suspend/resume hot path.

...

>  {
> -       struct device_node *np = port->dev->of_node;
> -
> -       if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
> -           of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
> -           of_device_is_compatible(np, "fsl,imx8qm-gpio"))
> -               return (gpiochip_generic_config(&port->gen_gc.gc,
> -                                               offset, conf) == 0);
> +       if (!port->has_pad_wakeup)
> +               return false;
>
> -       return false;
> +       return (gpiochip_generic_config(&port->gen_gc.gc, offset, conf) == 0);

Too many parentheses, also the semantic of 0 is not obvious. Better,
for example, this one

  int ret;
  ...
  ret = gpiochip_generic_config(...);
  if (ret)
    return false;

  return true;

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] gpio: mxc: cache compatible checks at probe time

[Frank Li]

On Sat, Oct 03, 2026 at 08:06:45PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> mxc_gpio_generic_config() and mxc_gpio_set_pad_wakeup() call
> of_device_is_compatible() on every invocation to determine pad wakeup
> capability and i.MX8QM-specific behavior. These properties are
> invariant for the lifetime of the device.
>
> Cache them as bool fields (has_pad_wakeup, is_imx8qm) in mxc_gpio_port
> during probe, eliminating repeated device tree string comparisons in
> the suspend/resume hot path.
>
> Assisted-by: LLM
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/gpio/gpio-mxc.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 8a755ac1af83..1c27232f6a80 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -73,6 +73,8 @@ struct mxc_gpio_port {
>  	bool power_off;
>  	unsigned long wakeup_pads;
>  	bool is_pad_wakeup;
> +	bool has_pad_wakeup;
> +	bool is_imx8qm;
>  	u32 pad_type[32];
>  	const struct mxc_gpio_hwdata *hwdata;
>  };
> @@ -457,6 +459,14 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	if (of_device_is_compatible(np, "fsl,imx7d-gpio"))
>  		port->power_off = true;
>
> +	if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
> +	    of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
> +	    of_device_is_compatible(np, "fsl,imx8qm-gpio"))
> +		port->has_pad_wakeup = true;

can you move has_pad_wakeup/is_imx8qm in mxc_gpio_hw_data?

Frank

> +
> +	if (of_device_is_compatible(np, "fsl,imx8qm-gpio"))
> +		port->is_imx8qm = true;
> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> @@ -570,15 +580,10 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port *port)
>  static bool mxc_gpio_generic_config(struct mxc_gpio_port *port,
>  		unsigned int offset, unsigned long conf)
>  {
> -	struct device_node *np = port->dev->of_node;
> -
> -	if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
> -	    of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
> -	    of_device_is_compatible(np, "fsl,imx8qm-gpio"))
> -		return (gpiochip_generic_config(&port->gen_gc.gc,
> -						offset, conf) == 0);
> +	if (!port->has_pad_wakeup)
> +		return false;
>
> -	return false;
> +	return (gpiochip_generic_config(&port->gen_gc.gc, offset, conf) == 0);
>  }
>
>  static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
> @@ -586,7 +591,6 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
>  	unsigned long config;
>  	bool ret = false;
>  	int i, type;
> -	bool is_imx8qm = of_device_is_compatible(port->dev->of_node, "fsl,imx8qm-gpio");
>
>  	static const u32 pad_type_map[] = {
>  		IMX_SCU_WAKEUP_OFF,		/* 0 */
> @@ -608,7 +612,7 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
>  			else
>  				config = IMX_SCU_WAKEUP_OFF;
>
> -			if (is_imx8qm && config == IMX_SCU_WAKEUP_FALL_EDGE) {
> +			if (port->is_imx8qm && config == IMX_SCU_WAKEUP_FALL_EDGE) {
>  				dev_warn_once(port->dev,
>  					      "No falling-edge support for wakeup on i.MX8QM\n");
>  				config = IMX_SCU_WAKEUP_OFF;
>
> --
> 2.51.0
>
>

[PATCH 4/9] gpio: mxc: use devm action for irq_domain cleanup

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Replace the manual irq_domain_remove() error path with
devm_add_action_or_reset(), so the IRQ domain is cleaned up
automatically on both probe failure to eliminate the
out_irqdomain_remove goto label.

Assisted-by: LLM
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 1c27232f6a80..3c395c82d7d4 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -417,6 +417,13 @@ static void mxc_update_irq_chained_handler(struct mxc_gpio_port *port, bool enab
 	}
 }
 
+static void mxc_gpio_irq_domain_remove(void *data)
+{
+	struct irq_domain *domain = data;
+
+	irq_domain_remove(domain);
+}
+
 static int mxc_gpio_probe(struct platform_device *pdev)
 {
 	struct gpio_generic_chip_config config = { };
@@ -526,12 +533,16 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 		goto out_bgio;
 	}
 
+	err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain);
+	if (err)
+		goto out_bgio;
+
 	irq_domain_set_pm_device(port->domain, &pdev->dev);
 
 	/* gpio-mxc can be a generic irq chip */
 	err = mxc_gpio_init_gc(port, irq_base);
 	if (err < 0)
-		goto out_irqdomain_remove;
+		goto out_bgio;
 
 	list_add_tail(&port->node, &mxc_gpio_ports);
 
@@ -542,8 +553,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 
 	return 0;
 
-out_irqdomain_remove:
-	irq_domain_remove(port->domain);
 out_bgio:
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);

-- 
2.51.0

Re: [PATCH 4/9] gpio: mxc: use devm action for irq_domain cleanup

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> Replace the manual irq_domain_remove() error path with
> devm_add_action_or_reset(), so the IRQ domain is cleaned up
> automatically on both probe failure to eliminate the
> out_irqdomain_remove goto label.

...

> +       err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain);
> +       if (err)
> +               goto out_bgio;
> +

We have devm_irq_domain_instantiate() and the respective wrappers.

...

>         /* gpio-mxc can be a generic irq chip */
>         err = mxc_gpio_init_gc(port, irq_base);
>         if (err < 0)
> -               goto out_irqdomain_remove;
> +               goto out_bgio;

This is simply wrong. No devm_*() call should be followed by goto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/9] gpio: mxc: use devm action for irq_domain cleanup

[Frank Li]

On Sat, Oct 03, 2026 at 08:06:46PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Replace the manual irq_domain_remove() error path with
> devm_add_action_or_reset(), so the IRQ domain is cleaned up
> automatically on both probe failure to eliminate the
> out_irqdomain_remove goto label.
>
> Assisted-by: LLM
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/gpio/gpio-mxc.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 1c27232f6a80..3c395c82d7d4 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -417,6 +417,13 @@ static void mxc_update_irq_chained_handler(struct mxc_gpio_port *port, bool enab
>  	}
>  }
>
> +static void mxc_gpio_irq_domain_remove(void *data)
> +{
> +	struct irq_domain *domain = data;
> +
> +	irq_domain_remove(domain);
> +}
> +
>  static int mxc_gpio_probe(struct platform_device *pdev)
>  {
>  	struct gpio_generic_chip_config config = { };
> @@ -526,12 +533,16 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  		goto out_bgio;
>  	}
>

No sure why not irq_domain_create_linear(),
https://lore.kernel.org/imx/aoW8V84mQ7UZhpaC@SMW015318/

Thomas Gleixner accept add devm_irq_domain_create_linear().

Frank

> +	err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain);
> +	if (err)
> +		goto out_bgio;
> +
>  	irq_domain_set_pm_device(port->domain, &pdev->dev);
>
>  	/* gpio-mxc can be a generic irq chip */
>  	err = mxc_gpio_init_gc(port, irq_base);
>  	if (err < 0)
> -		goto out_irqdomain_remove;
> +		goto out_bgio;
>
>  	list_add_tail(&port->node, &mxc_gpio_ports);
>
> @@ -542,8 +553,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>
>  	return 0;
>
> -out_irqdomain_remove:
> -	irq_domain_remove(port->domain);
>  out_bgio:
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);
>
> --
> 2.51.0
>
>

[PATCH 5/9] gpio: mxc: use devres-managed PM runtime and dev_err_probe

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Switch pm_runtime_get_noresume() and pm_runtime_enable() to their
devm-managed variants so that pm_runtime_put_noidle() and
pm_runtime_disable() are handled automatically by devres on both
probe failure and device unbind.

Remove the out_bgio goto label and replacing all error paths with
direct returns using dev_err_probe(), which provides better
diagnostic output and handles -EPROBE_DEFER.

Assisted-by: LLM
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 3c395c82d7d4..73e19d2bf235 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -474,9 +474,9 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (of_device_is_compatible(np, "fsl,imx8qm-gpio"))
 		port->is_imx8qm = true;
 
-	pm_runtime_get_noresume(&pdev->dev);
+	devm_pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	devm_pm_runtime_enable(&pdev->dev);
 
 	/* disable the interrupt and clear the status */
 	writel(0, port->base + GPIO_IMR);
@@ -502,7 +502,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 
 	err = gpio_generic_chip_init(&port->gen_gc, &config);
 	if (err)
-		goto out_bgio;
+		return dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n");
 
 	port->gen_gc.gc.request = mxc_gpio_request;
 	port->gen_gc.gc.free = mxc_gpio_free;
@@ -518,31 +518,27 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 
 	err = devm_gpiochip_add_data(&pdev->dev, &port->gen_gc.gc, port);
 	if (err)
-		goto out_bgio;
+		return dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n");
 
 	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, 32, numa_node_id());
-	if (irq_base < 0) {
-		err = irq_base;
-		goto out_bgio;
-	}
+	if (irq_base < 0)
+		return dev_err_probe(&pdev->dev, irq_base, "Failed to alloc irq desc\n");
 
 	port->domain = irq_domain_create_legacy(dev_fwnode(&pdev->dev), 32, irq_base, 0,
 						&irq_domain_simple_ops, NULL);
-	if (!port->domain) {
-		err = -ENODEV;
-		goto out_bgio;
-	}
+	if (!port->domain)
+		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to create irq domain\n");
 
 	err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain);
 	if (err)
-		goto out_bgio;
+		return dev_err_probe(&pdev->dev, err, "Failed to add irq_domain_remove\n");
 
 	irq_domain_set_pm_device(port->domain, &pdev->dev);
 
 	/* gpio-mxc can be a generic irq chip */
 	err = mxc_gpio_init_gc(port, irq_base);
 	if (err < 0)
-		goto out_bgio;
+		return dev_err_probe(&pdev->dev, err, "Failed mxc_gpio_init_gc\n");
 
 	list_add_tail(&port->node, &mxc_gpio_ports);
 
@@ -552,12 +548,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
-
-out_bgio:
-	pm_runtime_disable(&pdev->dev);
-	pm_runtime_put_noidle(&pdev->dev);
-	dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
-	return err;
 }
 
 static void mxc_gpio_save_regs(struct mxc_gpio_port *port)

-- 
2.51.0

Re: [PATCH 5/9] gpio: mxc: use devres-managed PM runtime and dev_err_probe

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> Switch pm_runtime_get_noresume() and pm_runtime_enable() to their
> devm-managed variants so that pm_runtime_put_noidle() and
> pm_runtime_disable() are handled automatically by devres on both
> probe failure and device unbind.
>
> Remove the out_bgio goto label and replacing all error paths with

replace

> direct returns using dev_err_probe(), which provides better
> diagnostic output and handles -EPROBE_DEFER.

...

> -       pm_runtime_get_noresume(&pdev->dev);
> +       devm_pm_runtime_get_noresume(&pdev->dev);
>         pm_runtime_set_active(&pdev->dev);
> -       pm_runtime_enable(&pdev->dev);
> +       devm_pm_runtime_enable(&pdev->dev);

Definitely not.  There is little point to using devm_*() if you don't
check the return value.

...

>         err = gpio_generic_chip_init(&port->gen_gc, &config);
>         if (err)
> -               goto out_bgio;
> +               return dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n");

>         err = devm_gpiochip_add_data(&pdev->dev, &port->gen_gc.gc, port);
>         if (err)
> -               goto out_bgio;
> +               return dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n");

These (and more) don't belong to the change — split it to the
logically isolated ones.
One patch for dev_err_probe() and another for PM calls.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/9] gpio: mxc: use devres-managed PM runtime and dev_err_probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: mxc: PM usage counter underflow due to devm_pm_runtime_get_noresume and manual put
- [High] gpio: mxc: ignored devm_pm_runtime_enable and get_noresume returns
--

commit e443f1e5bd54a410ade428061ede6f400132fc30
Author: Peng Fan <peng.fan@nxp.com>

gpio: mxc: use devres-managed PM runtime and dev_err_probe

This patch switches the driver to use devres-managed PM runtime functions
and replaces goto error paths with direct returns using dev_err_probe().

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 3c395c82d7d46..73e19d2bf2351 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -474,9 +474,9 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	if (of_device_is_compatible(np, "fsl,imx8qm-gpio"))
>  		port->is_imx8qm = true;
>  
> -	pm_runtime_get_noresume(&pdev->dev);
> +	devm_pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	devm_pm_runtime_enable(&pdev->dev);

[Severity: High]
Should the return values of devm_pm_runtime_get_noresume() and
devm_pm_runtime_enable() be checked?

Both functions allocate memory for devres actions and can return error codes
like -ENOMEM. If devm_pm_runtime_get_noresume() fails, it drops its reference
internally by calling pm_runtime_put_noidle(). 

If we ignore this failure and continue, the probe proceeds under the false
assumption that it holds a PM reference. The manual call to
pm_runtime_put_autosuspend() at the end of mxc_gpio_probe() would then
immediately underflow the PM usage counter.

[ ... ]

> @@ -552,12 +548,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	pm_runtime_put_autosuspend(&pdev->dev);

[Severity: High]
Will this result in a PM usage counter underflow when the device is unbound?

Because devm_pm_runtime_get_noresume() registers a devres action to call
pm_runtime_put_noidle() on device unbind, the initial reference is already
managed. By manually calling pm_runtime_put_autosuspend() here at the end
of probe to enter autosuspend, the usage counter is decremented early. 

When the device is subsequently unregistered or unbound, the devres cleanup
will execute and drop the already zeroed usage counter, causing a kernel
warning.

>  
>  	return 0;
> -
> -out_bgio:
> -	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_put_noidle(&pdev->dev);
> -	dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
> -	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-gpio-mxc-cleanup-v1-0-dad728ce27f2@nxp.com?part=5

Re: [PATCH 5/9] gpio: mxc: use devres-managed PM runtime and dev_err_probe

[Frank Li]

On Sat, Oct 03, 2026 at 08:06:47PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Switch pm_runtime_get_noresume() and pm_runtime_enable() to their
> devm-managed variants so that pm_runtime_put_noidle() and
> pm_runtime_disable() are handled automatically by devres on both
> probe failure and device unbind.
>
> Remove the out_bgio goto label and replacing all error paths with
> direct returns using dev_err_probe(), which provides better
> diagnostic output and handles -EPROBE_DEFER.
>
> Assisted-by: LLM
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/gpio/gpio-mxc.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 3c395c82d7d4..73e19d2bf235 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -474,9 +474,9 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	if (of_device_is_compatible(np, "fsl,imx8qm-gpio"))
>  		port->is_imx8qm = true;
>
> -	pm_runtime_get_noresume(&pdev->dev);
> +	devm_pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	devm_pm_runtime_enable(&pdev->dev);

devm_pm_runtime_set_active_enabled() can include pm_runtime_set_active()
and need check return value here.

And why call pm_runtime_get_noresume() before enable()?

Frank

>
>  	/* disable the interrupt and clear the status */
>  	writel(0, port->base + GPIO_IMR);
> @@ -502,7 +502,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>
>  	err = gpio_generic_chip_init(&port->gen_gc, &config);
>  	if (err)
> -		goto out_bgio;
> +		return dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n");
>
>  	port->gen_gc.gc.request = mxc_gpio_request;
>  	port->gen_gc.gc.free = mxc_gpio_free;
> @@ -518,31 +518,27 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>
>  	err = devm_gpiochip_add_data(&pdev->dev, &port->gen_gc.gc, port);
>  	if (err)
> -		goto out_bgio;
> +		return dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n");
>
>  	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, 32, numa_node_id());
> -	if (irq_base < 0) {
> -		err = irq_base;
> -		goto out_bgio;
> -	}
> +	if (irq_base < 0)
> +		return dev_err_probe(&pdev->dev, irq_base, "Failed to alloc irq desc\n");
>
>  	port->domain = irq_domain_create_legacy(dev_fwnode(&pdev->dev), 32, irq_base, 0,
>  						&irq_domain_simple_ops, NULL);
> -	if (!port->domain) {
> -		err = -ENODEV;
> -		goto out_bgio;
> -	}
> +	if (!port->domain)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to create irq domain\n");
>
>  	err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain);
>  	if (err)
> -		goto out_bgio;
> +		return dev_err_probe(&pdev->dev, err, "Failed to add irq_domain_remove\n");
>
>  	irq_domain_set_pm_device(port->domain, &pdev->dev);
>
>  	/* gpio-mxc can be a generic irq chip */
>  	err = mxc_gpio_init_gc(port, irq_base);
>  	if (err < 0)
> -		goto out_bgio;
> +		return dev_err_probe(&pdev->dev, err, "Failed mxc_gpio_init_gc\n");
>
>  	list_add_tail(&port->node, &mxc_gpio_ports);
>
> @@ -552,12 +548,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	pm_runtime_put_autosuspend(&pdev->dev);
>
>  	return 0;
> -
> -out_bgio:
> -	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_put_noidle(&pdev->dev);
> -	dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
> -	return err;
>  }
>
>  static void mxc_gpio_save_regs(struct mxc_gpio_port *port)
>
> --
> 2.51.0
>
>

[PATCH 6/9] gpio: mxc: use local dev variable and device_is_compatible()

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Introduce a local 'struct device *dev' variable to replace repeated
'&pdev->dev' dereferences throughout mxc_gpio_probe(), improving
readability.

Switch from of_device_is_compatible(np, ...) to the device-model
device_is_compatible(dev, ...) API which works with both DT and ACPI
firmware backends. The 'np' variable is retained for
of_alias_get_id() which has no device-model equivalent.

No functional change.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 53 +++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 73e19d2bf235..a3274be7126a 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -428,17 +428,18 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 {
 	struct gpio_generic_chip_config config = { };
 	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
 	struct mxc_gpio_port *port;
 	int irq_count;
 	int irq_base;
 	int err;
 
-	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
 	if (!port)
 		return -ENOMEM;
 
-	port->dev = &pdev->dev;
-	port->hwdata = device_get_match_data(&pdev->dev);
+	port->dev = dev;
+	port->hwdata = device_get_match_data(dev);
 
 	port->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(port->base))
@@ -459,30 +460,30 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 		return port->irq;
 
 	/* the controller clock is optional */
-	port->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	port->clk = devm_clk_get_optional_enabled(dev, NULL);
 	if (IS_ERR(port->clk))
 		return PTR_ERR(port->clk);
 
-	if (of_device_is_compatible(np, "fsl,imx7d-gpio"))
+	if (device_is_compatible(dev, "fsl,imx7d-gpio"))
 		port->power_off = true;
 
-	if (of_device_is_compatible(np, "fsl,imx8dxl-gpio") ||
-	    of_device_is_compatible(np, "fsl,imx8qxp-gpio") ||
-	    of_device_is_compatible(np, "fsl,imx8qm-gpio"))
+	if (device_is_compatible(dev, "fsl,imx8dxl-gpio") ||
+	    device_is_compatible(dev, "fsl,imx8qxp-gpio") ||
+	    device_is_compatible(dev, "fsl,imx8qm-gpio"))
 		port->has_pad_wakeup = true;
 
-	if (of_device_is_compatible(np, "fsl,imx8qm-gpio"))
+	if (device_is_compatible(dev, "fsl,imx8qm-gpio"))
 		port->is_imx8qm = true;
 
-	devm_pm_runtime_get_noresume(&pdev->dev);
-	pm_runtime_set_active(&pdev->dev);
-	devm_pm_runtime_enable(&pdev->dev);
+	devm_pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	devm_pm_runtime_enable(dev);
 
 	/* disable the interrupt and clear the status */
 	writel(0, port->base + GPIO_IMR);
 	writel(~0, port->base + GPIO_ISR);
 
-	if (of_device_is_compatible(np, "fsl,imx21-gpio")) {
+	if (device_is_compatible(dev, "fsl,imx21-gpio")) {
 		/*
 		 * Setup one handler for all GPIO interrupts. Actually setting
 		 * the handler is needed only once, but doing it for every port
@@ -493,7 +494,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	} else
 		port->mx_irq_handler = mx3_gpio_irq_handler;
 
-	config.dev = &pdev->dev;
+	config.dev = dev;
 	config.sz = 4;
 	config.dat = port->base + GPIO_PSR;
 	config.set = port->base + GPIO_DR;
@@ -502,7 +503,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 
 	err = gpio_generic_chip_init(&port->gen_gc, &config);
 	if (err)
-		return dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n");
+		return dev_err_probe(dev, err, "Failed to init gpio chip\n");
 
 	port->gen_gc.gc.request = mxc_gpio_request;
 	port->gen_gc.gc.free = mxc_gpio_free;
@@ -516,36 +517,36 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	else /* silence boot time warning */
 		port->gen_gc.gc.base = -1;
 
-	err = devm_gpiochip_add_data(&pdev->dev, &port->gen_gc.gc, port);
+	err = devm_gpiochip_add_data(dev, &port->gen_gc.gc, port);
 	if (err)
-		return dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n");
+		return dev_err_probe(dev, err, "Failed to add gpiochip data\n");
 
-	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, 32, numa_node_id());
+	irq_base = devm_irq_alloc_descs(dev, -1, 0, 32, numa_node_id());
 	if (irq_base < 0)
-		return dev_err_probe(&pdev->dev, irq_base, "Failed to alloc irq desc\n");
+		return dev_err_probe(dev, irq_base, "Failed to alloc irq desc\n");
 
-	port->domain = irq_domain_create_legacy(dev_fwnode(&pdev->dev), 32, irq_base, 0,
+	port->domain = irq_domain_create_legacy(dev_fwnode(dev), 32, irq_base, 0,
 						&irq_domain_simple_ops, NULL);
 	if (!port->domain)
-		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to create irq domain\n");
+		return dev_err_probe(dev, -ENODEV, "Failed to create irq domain\n");
 
-	err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain);
+	err = devm_add_action_or_reset(dev, mxc_gpio_irq_domain_remove, port->domain);
 	if (err)
-		return dev_err_probe(&pdev->dev, err, "Failed to add irq_domain_remove\n");
+		return dev_err_probe(dev, err, "Failed to add irq_domain_remove\n");
 
-	irq_domain_set_pm_device(port->domain, &pdev->dev);
+	irq_domain_set_pm_device(port->domain, dev);
 
 	/* gpio-mxc can be a generic irq chip */
 	err = mxc_gpio_init_gc(port, irq_base);
 	if (err < 0)
-		return dev_err_probe(&pdev->dev, err, "Failed mxc_gpio_init_gc\n");
+		return dev_err_probe(dev, err, "Failed mxc_gpio_init_gc\n");
 
 	list_add_tail(&port->node, &mxc_gpio_ports);
 
 	mxc_update_irq_chained_handler(port, true);
 
 	platform_set_drvdata(pdev, port);
-	pm_runtime_put_autosuspend(&pdev->dev);
+	pm_runtime_put_autosuspend(dev);
 
 	return 0;
 }

-- 
2.51.0

Re: [PATCH 6/9] gpio: mxc: use local dev variable and device_is_compatible()

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:10 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> Introduce a local 'struct device *dev' variable to replace repeated
> '&pdev->dev' dereferences throughout mxc_gpio_probe(), improving
> readability.
>
> Switch from of_device_is_compatible(np, ...) to the device-model
> device_is_compatible(dev, ...) API which works with both DT and ACPI
> firmware backends. The 'np' variable is retained for
> of_alias_get_id() which has no device-model equivalent.
>
> No functional change.

> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -428,17 +428,18 @@ static int mxc_gpio_probe(struct platform_device *pdev)

It seems you missed updating the headers (like switching from of.h to
property.h). Other than that, it looks good.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 7/9] gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Both gpio_set_irq_type() and mxc_flip_edge() open-code the same ICR
register selection and 2-bit field shift/mask arithmetic with magic
numbers (0x10, 0xf, 0x3).

Introduce two macros:
  - MXC_ICR_REG(gpio):  selects ICR1 (pins 0-15) or ICR2 (pins 16-31)
  - MXC_ICR_MASK(gpio): 2-bit mask at the correct position

Use field_prep() and field_get() from linux/bitfield.h for the
shift/extract operations instead of open-coded shifts. The lowercase
variants accept runtime-computed masks.

Eliminate the intermediate 'bit' variable from both functions and
makes the register access pattern self-documenting.

No functional change.

Assisted-by: LLM
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index a3274be7126a..18ff33a0abfb 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -7,6 +7,7 @@
 // Authors: Daniel Mack, Juergen Beisert.
 // Copyright (C) 2004-2010 Freescale Semiconductor, Inc. All Rights Reserved.
 
+#include <linux/bitfield.h>
 #include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/err.h>
@@ -139,6 +140,9 @@ static struct mxc_gpio_hwdata imx35_gpio_hwdata = {
 #define GPIO_INT_FALL_EDGE	(port->hwdata->fall_edge)
 #define GPIO_INT_BOTH_EDGES	0x4
 
+#define MXC_ICR_REG(gpio)	(GPIO_ICR1 + (((gpio) & 0x10) >> 2))
+#define MXC_ICR_MASK(gpio)	(0x3 << (((gpio) & 0xf) << 1))
+
 static const struct of_device_id mxc_gpio_dt_ids[] = {
 	{ .compatible = "fsl,imx1-gpio", .data =  &imx1_imx21_gpio_hwdata },
 	{ .compatible = "fsl,imx21-gpio", .data = &imx1_imx21_gpio_hwdata },
@@ -165,7 +169,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct mxc_gpio_port *port = gc->private;
-	u32 bit, val;
+	u32 val;
 	u32 gpio_idx = d->hwirq;
 	int edge;
 	void __iomem *reg = port->base;
@@ -215,10 +219,9 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 		}
 
 		if (edge != GPIO_INT_BOTH_EDGES) {
-			reg += GPIO_ICR1 + ((gpio_idx & 0x10) >> 2); /* lower or upper register */
-			bit = gpio_idx & 0xf;
-			val = readl(reg) & ~(0x3 << (bit << 1));
-			writel(val | (edge << (bit << 1)), reg);
+			reg += MXC_ICR_REG(gpio_idx);
+			val = readl(reg) & ~MXC_ICR_MASK(gpio_idx);
+			writel(val | field_prep(MXC_ICR_MASK(gpio_idx), edge), reg);
 		}
 
 		writel(1 << gpio_idx, port->base + GPIO_ISR);
@@ -231,16 +234,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 {
 	void __iomem *reg = port->base;
-	u32 bit, val;
+	u32 val;
 	int edge;
 
 	guard(gpio_generic_lock_irqsave)(&port->gen_gc);
 
-	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
-	bit = gpio & 0xf;
+	reg += MXC_ICR_REG(gpio);
 	val = readl(reg);
-	edge = (val >> (bit << 1)) & 3;
-	val &= ~(0x3 << (bit << 1));
+	edge = field_get(MXC_ICR_MASK(gpio), val);
+	val &= ~MXC_ICR_MASK(gpio);
 	if (edge == GPIO_INT_HIGH_LEV) {
 		edge = GPIO_INT_LOW_LEV;
 		pr_debug("mxc: switch GPIO %d to low trigger\n", gpio);
@@ -252,7 +254,7 @@ static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
 		       gpio, edge);
 		return;
 	}
-	writel(val | (edge << (bit << 1)), reg);
+	writel(val | field_prep(MXC_ICR_MASK(gpio), edge), reg);
 }
 
 /* handle 32 interrupts in one status register */

-- 
2.51.0

Re: [PATCH 7/9] gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: mxc: sign extension in field_get() breaks edge flipping for pins 15/31
--

commit afaa23e01f6d59d21c3b7e082aaa50a10fe5daeb
Author: Peng Fan <peng.fan@nxp.com>

gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get

This commit introduces MXC_ICR_REG and MXC_ICR_MASK macros to replace
open-coded shift and mask arithmetic in the driver. It then updates
gpio_set_irq_type() and mxc_flip_edge() to use the new macros alongside
field_prep() and field_get().

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index a3274be7126a1..18ff33a0abfb7 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c

[ ... ]

> @@ -139,6 +140,9 @@ static struct mxc_gpio_hwdata imx35_gpio_hwdata = {
>  #define GPIO_INT_FALL_EDGE	(port->hwdata->fall_edge)
>  #define GPIO_INT_BOTH_EDGES	0x4
>  
> +#define MXC_ICR_REG(gpio)	(GPIO_ICR1 + (((gpio) & 0x10) >> 2))
> +#define MXC_ICR_MASK(gpio)	(0x3 << (((gpio) & 0xf) << 1))

[Severity: High]
Does this literal 0x3 evaluate to a signed integer?

If the mask is calculated for pin 15 or 31, the shift will be 30 bits.
Shifting the signed integer 0x3 by 30 bits produces a negative signed
integer (0xC0000000). Would using an unsigned literal like 0x3U prevent
this?

[ ... ]

> @@ -231,16 +234,15 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
>  static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  {
>  	void __iomem *reg = port->base;
> -	u32 bit, val;
> +	u32 val;
>  	int edge;
>  
>  	guard(gpio_generic_lock_irqsave)(&port->gen_gc);
>  
> -	reg += GPIO_ICR1 + ((gpio & 0x10) >> 2); /* lower or upper register */
> -	bit = gpio & 0xf;
> +	reg += MXC_ICR_REG(gpio);
>  	val = readl(reg);
> -	edge = (val >> (bit << 1)) & 3;
> -	val &= ~(0x3 << (bit << 1));
> +	edge = field_get(MXC_ICR_MASK(gpio), val);

[Severity: High]
Does this field_get() result in sign-extension corruption when reading
pins 15 or 31?

Because MXC_ICR_MASK(gpio) evaluates to a signed integer, the internal
__field_get() macro casts the register value to typeof(mask), which is
signed. When it performs the right shift (>> 30) to extract the field,
it will do an arithmetic right shift, causing the extracted 2-bit field
to sign-extend into a negative value (e.g., 0x80000000 >> 30 becomes -2
instead of 2).

> +	val &= ~MXC_ICR_MASK(gpio);
>  	if (edge == GPIO_INT_HIGH_LEV) {
>  		edge = GPIO_INT_LOW_LEV;
>  		pr_debug("mxc: switch GPIO %d to low trigger\n", gpio);
> @@ -252,7 +254,7 @@ static void mxc_flip_edge(struct mxc_gpio_port *port, u32 gpio)
>  		       gpio, edge);
>  		return;

[Severity: High]
Could the sign-extended negative value trigger this error path?

If edge evaluates to a negative number like -2, it will fail the
validation checks for GPIO_INT_HIGH_LEV and GPIO_INT_LOW_LEV. This
regression would cause IRQ_TYPE_EDGE_BOTH interrupts on pins 15 and 31
to abort here on the first edge, failing to invert the polarity and
causing subsequent interrupts to be missed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-gpio-mxc-cleanup-v1-0-dad728ce27f2@nxp.com?part=7

[PATCH 8/9] gpio: mxc: use BIT() macro for single-bit operations

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Replace open-coded '1 << n' shifts with the BIT() macro throughout
the driver for consistency and to avoid potential signed-shift issues
when the bit index is 31 (1 << 31 is implementation-defined for
signed int).

No functional change.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 18ff33a0abfb..bf1207f30460 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -174,7 +174,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 	int edge;
 	void __iomem *reg = port->base;
 
-	port->both_edges &= ~(1 << gpio_idx);
+	port->both_edges &= ~BIT(gpio_idx);
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
 		edge = GPIO_INT_RISE_EDGE;
@@ -194,7 +194,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 				edge = GPIO_INT_HIGH_LEV;
 				pr_debug("mxc: set GPIO %d to high trigger\n", gpio_idx);
 			}
-			port->both_edges |= 1 << gpio_idx;
+			port->both_edges |= BIT(gpio_idx);
 		}
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
@@ -211,10 +211,10 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 		if (GPIO_EDGE_SEL >= 0) {
 			val = readl(port->base + GPIO_EDGE_SEL);
 			if (edge == GPIO_INT_BOTH_EDGES)
-				writel(val | (1 << gpio_idx),
+				writel(val | BIT(gpio_idx),
 				       port->base + GPIO_EDGE_SEL);
 			else
-				writel(val & ~(1 << gpio_idx),
+				writel(val & ~BIT(gpio_idx),
 				       port->base + GPIO_EDGE_SEL);
 		}
 
@@ -224,7 +224,7 @@ static int gpio_set_irq_type(struct irq_data *d, u32 type)
 			writel(val | field_prep(MXC_ICR_MASK(gpio_idx), edge), reg);
 		}
 
-		writel(1 << gpio_idx, port->base + GPIO_ISR);
+		writel(BIT(gpio_idx), port->base + GPIO_ISR);
 		port->pad_type[gpio_idx] = type;
 	}
 
@@ -263,12 +263,12 @@ static void mxc_gpio_irq_handler(struct mxc_gpio_port *port, u32 irq_stat)
 	while (irq_stat != 0) {
 		int irqoffset = fls(irq_stat) - 1;
 
-		if (port->both_edges & (1 << irqoffset))
+		if (port->both_edges & BIT(irqoffset))
 			mxc_flip_edge(port, irqoffset);
 
 		generic_handle_domain_irq(port->domain, irqoffset);
 
-		irq_stat &= ~(1 << irqoffset);
+		irq_stat &= ~BIT(irqoffset);
 	}
 }
 

-- 
2.51.0

[PATCH 9/9] gpio: mxc: simplify gpio_set_wake_irq() with irq_set_irq_wake and assign_bit

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

To simplify gpio_set_wake_irq():
 - Replace the enable/disable_irq_wake() if/else branches with a single
   irq_set_irq_wake() call which handles both directions internally.
 - Replace the separate set_bit()/clear_bit() calls with assign_bit()
   which sets or clears the bit based on the enable parameter.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index bf1207f30460..546a46857e1d 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -329,21 +329,13 @@ static int gpio_set_wake_irq(struct irq_data *d, u32 enable)
 	u32 gpio_idx = d->hwirq;
 	int ret;
 
-	if (enable) {
-		if (port->irq_high && (gpio_idx >= 16))
-			ret = enable_irq_wake(port->irq_high);
-		else
-			ret = enable_irq_wake(port->irq);
-		if (!ret)
-			set_bit(gpio_idx, &port->wakeup_pads);
-	} else {
-		if (port->irq_high && (gpio_idx >= 16))
-			ret = disable_irq_wake(port->irq_high);
-		else
-			ret = disable_irq_wake(port->irq);
-		if (!ret)
-			clear_bit(gpio_idx, &port->wakeup_pads);
-	}
+	if (port->irq_high && (gpio_idx >= 16))
+		ret = irq_set_irq_wake(port->irq_high, enable);
+	else
+		ret = irq_set_irq_wake(port->irq, enable);
+
+	if (!ret)
+		assign_bit(gpio_idx, &port->wakeup_pads, enable);
 
 	return ret;
 }

-- 
2.51.0

Re: [PATCH 9/9] gpio: mxc: simplify gpio_set_wake_irq() with irq_set_irq_wake and assign_bit

[Andy Shevchenko]

On Sat, Oct 3, 2026 at 3:10 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> To simplify gpio_set_wake_irq():
>  - Replace the enable/disable_irq_wake() if/else branches with a single
>    irq_set_irq_wake() call which handles both directions internally.
>  - Replace the separate set_bit()/clear_bit() calls with assign_bit()
>    which sets or clears the bit based on the enable parameter.

...

> -       if (enable) {
> -               if (port->irq_high && (gpio_idx >= 16))
> -                       ret = enable_irq_wake(port->irq_high);
> -               else
> -                       ret = enable_irq_wake(port->irq);
> -               if (!ret)
> -                       set_bit(gpio_idx, &port->wakeup_pads);
> -       } else {
> -               if (port->irq_high && (gpio_idx >= 16))
> -                       ret = disable_irq_wake(port->irq_high);
> -               else
> -                       ret = disable_irq_wake(port->irq);
> -               if (!ret)
> -                       clear_bit(gpio_idx, &port->wakeup_pads);
> -       }
> +       if (port->irq_high && (gpio_idx >= 16))
> +               ret = irq_set_irq_wake(port->irq_high, enable);
> +       else
> +               ret = irq_set_irq_wake(port->irq, enable);

> +       if (!ret)
> +               assign_bit(gpio_idx, &port->wakeup_pads, enable);
>
>         return ret;

This uses an unusual pattern, we check for the error first.
But also this part should not be ping-ponged over the series, it
should be from the start like this, see my comment against the
respective patch.

>  }

-- 
With Best Regards,
Andy Shevchenko