Hi Bartosz,
thank you for the review on the first version of this series.

TL;DR: There's no clock-names property in the DTBs for the GPIO banks,
       and they don't expose a separate debounce clock in hardware. 

Regarding your question: existing DTBs currently do not provide clock-names
for these GPIO nodes and instead rely on positional clock ordering.

While we could extend the binding to optionally support clock-names going
forward, the driver still needs to remain compatible with existing DTBs, so
it cannot rely on name-based lookup here.

For this reason, keeping the index-based of_clk_get(..., 1) lookup together
with devm_add_action_or_reset() for cleanup seemed like the safest option.

A good example can be gpio1 in rk3399-base.dtsi, where the clocks property
is defined as:

  clocks = <&pmucru PCLK_GPIO1_PMU>;

If we switched to name-based lookup via devm_clk_get(dev, "db"), it would
fail for existing DTBs because they do not define the corresponding
clock-names property.

Additionally, PMU banks such as gpio1 do not expose a separate debounce
clock in hardware, so there would not be a matching entry anyway.

Therefore, using of_clk_get(..., 1) is currently the only approach that
preserves compatibility with existing DTBs while avoiding regressions.

Changes in v2:

- Rephrased patch 1 and patch 2 commit messages to use the imperative as
  requested.

- Clarified in the commit message of patch 2 why devm_clk_get() cannot
  be used for the debounce clocks.

The patches included in this series:

1. Convert the main clock (bank->clk) to use devm_clk_get_enabled(),
   simplifying the code and fixing an existing clk_put() leak.

2. Fix several teardown bugs and resource leaks:
   - Fix a reference leak for the debounce clock (bank->db_clk)
     by properly releasing it with a devm action.
   - Fix a potential kernel panic on module unload by unregistering the
     chained IRQ handler (rockchip_irq_demux) in the remove() callback.
   - Fix an IRQ domain and generic chip memory leak by calling
     irq_domain_remove() during module unload.


Marco Scardovi (2):
  gpio: rockchip: convert bank->clk to devm_clk_get_enabled()
  gpio: rockchip: teardown bugs and resource leaks

 drivers/gpio/gpio-rockchip.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

-- 
2.54.0

On Tue, 26 May 2026 19:02:44 +0200, Marco Scardovi wrote:
> thank you for the review on the first version of this series.
> 
> TL;DR: There's no clock-names property in the DTBs for the GPIO banks,
>        and they don't expose a separate debounce clock in hardware.
> 
> Regarding your question: existing DTBs currently do not provide clock-names
> for these GPIO nodes and instead rely on positional clock ordering.
> 
> [...]

Applied, thanks!

[1/2] gpio: rockchip: convert bank->clk to devm_clk_get_enabled()
      https://git.kernel.org/brgl/c/d4573b270d934c35eb77fc348866384fcb4e8eeb
[2/2] gpio: rockchip: teardown bugs and resource leaks
      https://git.kernel.org/brgl/c/9c95f4b920e15c183ebbb15b0a011ba32fcb6d59

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

On Tue, 26 May 2026 19:02:44 +0200, Marco Scardovi <scardracs@disroot.org> said:
> Hi Bartosz,
> thank you for the review on the first version of this series.
>
> TL;DR: There's no clock-names property in the DTBs for the GPIO banks,
>        and they don't expose a separate debounce clock in hardware.
>
> Regarding your question: existing DTBs currently do not provide clock-names
> for these GPIO nodes and instead rely on positional clock ordering.
>
> While we could extend the binding to optionally support clock-names going
> forward, the driver still needs to remain compatible with existing DTBs, so
> it cannot rely on name-based lookup here.
>
> For this reason, keeping the index-based of_clk_get(..., 1) lookup together
> with devm_add_action_or_reset() for cleanup seemed like the safest option.
>
> A good example can be gpio1 in rk3399-base.dtsi, where the clocks property
> is defined as:
>
>   clocks = <&pmucru PCLK_GPIO1_PMU>;
>
> If we switched to name-based lookup via devm_clk_get(dev, "db"), it would
> fail for existing DTBs because they do not define the corresponding
> clock-names property.
>
> Additionally, PMU banks such as gpio1 do not expose a separate debounce
> clock in hardware, so there would not be a matching entry anyway.
>
> Therefore, using of_clk_get(..., 1) is currently the only approach that
> preserves compatibility with existing DTBs while avoiding regressions.
>

I see. Can you please reverse the order of the patches? The fix should go into
v7.1 and stable branches, patch 1/2 is a new feature so it'll go into v7.2.

Bart

On Wed, 27 May 2026 10:10:23 +0200, Bartosz Golaszewski <brgl@kernel.org> said:
> On Tue, 26 May 2026 19:02:44 +0200, Marco Scardovi <scardracs@disroot.org> said:
>> Hi Bartosz,
>> thank you for the review on the first version of this series.
>>
>> TL;DR: There's no clock-names property in the DTBs for the GPIO banks,
>>        and they don't expose a separate debounce clock in hardware.
>>
>> Regarding your question: existing DTBs currently do not provide clock-names
>> for these GPIO nodes and instead rely on positional clock ordering.
>>
>> While we could extend the binding to optionally support clock-names going
>> forward, the driver still needs to remain compatible with existing DTBs, so
>> it cannot rely on name-based lookup here.
>>
>> For this reason, keeping the index-based of_clk_get(..., 1) lookup together
>> with devm_add_action_or_reset() for cleanup seemed like the safest option.
>>
>> A good example can be gpio1 in rk3399-base.dtsi, where the clocks property
>> is defined as:
>>
>>   clocks = <&pmucru PCLK_GPIO1_PMU>;
>>
>> If we switched to name-based lookup via devm_clk_get(dev, "db"), it would
>> fail for existing DTBs because they do not define the corresponding
>> clock-names property.
>>
>> Additionally, PMU banks such as gpio1 do not expose a separate debounce
>> clock in hardware, so there would not be a matching entry anyway.
>>
>> Therefore, using of_clk_get(..., 1) is currently the only approach that
>> preserves compatibility with existing DTBs while avoiding regressions.
>>
>
> I see. Can you please reverse the order of the patches? The fix should go into
> v7.1 and stable branches, patch 1/2 is a new feature so it'll go into v7.2.
>
> Bart
>

Ah, nevermind my comment, I see patch 1/2 is a fix as well. I'll queue these
and fix the dev_err_probe() thing in tree.

Bart