[PATCH v3 00/12] gpio: rockchip: Update the GPIO driver

[PATCH v3 00/12] gpio: rockchip: Update the GPIO driver

[Ye Zhang]

GPIO driver support acpi and new version, set input direction in
irq_request_resources, fix division error and debounce config error.

Changes since v1:
- Split commits with multiple changes into separate commits.
- Adjust backportable fix to the forefront.
- Modify messages of some commits.

Changes since v2:
- Optimize version number comments.
- Modify the GPIO version judgment logic.
- Use devm_clk_get_enabled to simplify the code.
- Use guard instead of mutex_lock to simplify the code.
- Use irq_hw_number_t and irqd_to_hwirq() in the request irq function.
- Since list_first_entry cannot return NULL, remove the NULL check.
- Temporarily do not add support for ACPI.

Ye Zhang (12):
  gpio: rockchip: avoid division by zero
  gpio: rockchip: release reference to device node
  gpio: rockchip: resolve overflow issues
  gpio: rockchip: resolve underflow issue
  gpio: rockchip: fix debounce calculate
  gpio: rockchip: Update debounce config function
  gpio: rockchip: support 'clock-names' from dt nodes
  gpio: rockchip: explan the format of the GPIO version ID
  gpio: rockchip: change the GPIO version judgment logic
  gpio: rockchip: support new version gpio
  gpio: rockchip: Set input direction when request irq
  gpio: rockchip: replace mutex_lock() with guard()

 drivers/gpio/gpio-rockchip.c       | 151 ++++++++++++++++++-----------
 drivers/pinctrl/pinctrl-rockchip.h |   2 +
 2 files changed, 99 insertions(+), 54 deletions(-)

-- 
2.34.1

[PATCH v3 01/12] gpio: rockchip: avoid division by zero

[Ye Zhang]

If the clk_get_rate return '0', it will happen division by zero.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 0bd339813110..712258224eb3 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -207,6 +207,8 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
 		div_debounce_support = true;
 		freq = clk_get_rate(bank->db_clk);
+		if (!freq)
+			return -EINVAL;
 		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
 		if (debounce > max_debounce)
 			return -EINVAL;
-- 
2.34.1

[PATCH v3 02/12] gpio: rockchip: release reference to device node

[Ye Zhang]

Added a call to of_node_put(pctlnp) in rockchip_gpio_probe to properly
release the reference to the device node, improving memory management
and preventing potential leaks.

Fixes: 936ee2675eee ("gpio/rockchip: add driver for rockchip gpio")
Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 712258224eb3..5f60162baaeb 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -715,6 +715,7 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	pctldev = of_pinctrl_get(pctlnp);
+	of_node_put(pctlnp);
 	if (!pctldev)
 		return -EPROBE_DEFER;
 
-- 
2.34.1

[PATCH v3 03/12] gpio: rockchip: resolve overflow issues

[Ye Zhang]

Prevent overflow issues when performing debounce-related calculations.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 5f60162baaeb..6dcb8bb0d1b4 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -22,6 +22,7 @@
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/units.h>
 
 #include "../pinctrl/core.h"
 #include "../pinctrl/pinctrl-rockchip.h"
@@ -209,11 +210,12 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 		freq = clk_get_rate(bank->db_clk);
 		if (!freq)
 			return -EINVAL;
-		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
+		div = (u64)(GENMASK(23, 0) + 1) * 2 * HZ_PER_MHZ;
+		max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
 		if (debounce > max_debounce)
 			return -EINVAL;
 
-		div = debounce * freq;
+		div = (u64)debounce * freq;
 		div_reg = DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) - 1;
 	} else {
 		div_debounce_support = false;
-- 
2.34.1

[PATCH v3 04/12] gpio: rockchip: resolve underflow issue

[Ye Zhang]

div_reg may be < 0 if debounce is zero, causing the unsigned int to
overflow.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 6dcb8bb0d1b4..26191197cd37 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -205,8 +205,11 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 	unsigned int cur_div_reg;
 	u64 div;
 
-	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
+	if ((bank->gpio_type == GPIO_TYPE_V2) && !IS_ERR(bank->db_clk))
 		div_debounce_support = true;
+	else
+		div_debounce_support = false;
+	if (debounce && div_debounce_support) {
 		freq = clk_get_rate(bank->db_clk);
 		if (!freq)
 			return -EINVAL;
@@ -217,8 +220,6 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 
 		div = (u64)debounce * freq;
 		div_reg = DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) - 1;
-	} else {
-		div_debounce_support = false;
 	}
 
 	raw_spin_lock_irqsave(&bank->slock, flags);
-- 
2.34.1

[PATCH v3 05/12] gpio: rockchip: fix debounce calculate

[Ye Zhang]

The previous configuration ensured that signals with a duration greater
than the debounce value would always be detected, while signals with a
duration less than debounce / 2 would always not be detected. After the
modification, it is changed to ensure that signals with a duration greater
than 2 * debounce will always be detected, while signals with a duration
less than debounce/2 will still not be detected.

Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 26191197cd37..75355f799751 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -213,13 +213,13 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 		freq = clk_get_rate(bank->db_clk);
 		if (!freq)
 			return -EINVAL;
-		div = (u64)(GENMASK(23, 0) + 1) * 2 * HZ_PER_MHZ;
+		div = (u64)(GENMASK(23, 0) + 1) * HZ_PER_MHZ;
 		max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
 		if (debounce > max_debounce)
 			return -EINVAL;
 
 		div = (u64)debounce * freq;
-		div_reg = DIV_ROUND_CLOSEST_ULL(div, 2 * USEC_PER_SEC) - 1;
+		div_reg = DIV_ROUND_CLOSEST_ULL(div, USEC_PER_SEC) - 1;
 	}
 
 	raw_spin_lock_irqsave(&bank->slock, flags);
-- 
2.34.1

[PATCH v3 08/12] gpio: rockchip: explan the format of the GPIO version ID

[Ye Zhang]

Remove redundant comments and provide a detailed explanation of the
GPIO version ID.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 83df1632112d..04a24f1d77eb 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -27,9 +27,15 @@
 #include "../pinctrl/core.h"
 #include "../pinctrl/pinctrl-rockchip.h"
 
+/*
+ * Version ID Register
+ * Bits [31:24] - Major Version
+ * Bits [23:16] - Minor Version
+ * Bits [15:0]  - SVN Number
+ */
 #define GPIO_TYPE_V1		(0)           /* GPIO Version ID reserved */
-#define GPIO_TYPE_V2		(0x01000C2B)  /* GPIO Version ID 0x01000C2B */
-#define GPIO_TYPE_V2_1		(0x0101157C)  /* GPIO Version ID 0x0101157C */
+#define GPIO_TYPE_V2		(0x01000C2B)
+#define GPIO_TYPE_V2_1		(0x0101157C)
 
 static const struct rockchip_gpio_regs gpio_regs_v1 = {
 	.port_dr = 0x00,
-- 
2.34.1

[PATCH v3 09/12] gpio: rockchip: change the GPIO version judgment logic

[Ye Zhang]

Have a list of valid V1 IDs and default to -ENODEV.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 04a24f1d77eb..579701ad3c6f 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -658,13 +658,20 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 
 	id = readl(bank->reg_base + gpio_regs_v2.version_id);
 
-	/* If not gpio v2, that is default to v1. */
-	if (id == GPIO_TYPE_V2 || id == GPIO_TYPE_V2_1) {
-		bank->gpio_regs = &gpio_regs_v2;
-		bank->gpio_type = GPIO_TYPE_V2;
-	} else {
+	/* The GPIO version ID is incrementing. */
+	switch (id) {
+	case GPIO_TYPE_V1:
 		bank->gpio_regs = &gpio_regs_v1;
 		bank->gpio_type = GPIO_TYPE_V1;
+		break;
+	case GPIO_TYPE_V2:
+	case GPIO_TYPE_V2_1:
+		bank->gpio_regs = &gpio_regs_v2;
+		bank->gpio_type = GPIO_TYPE_V2;
+		break;
+	default:
+		dev_err(bank->dev, "cannot get the version ID\n");
+		return -ENODEV;
 	}
 
 	return 0;
-- 
2.34.1

[PATCH v3 12/12] gpio: rockchip: replace mutex_lock() with guard()

[Ye Zhang]

Replacing mutex_lock with guard() simplifies the code and helps avoid
deadlocks.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 73e57efb46fc..d5c57617fc86 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -765,20 +765,19 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = rockchip_get_bank_data(bank);
-	if (ret)
-		goto err_disabled_clk;
-
 	/*
 	 * Prevent clashes with a deferred output setting
 	 * being added right at this moment.
 	 */
-	mutex_lock(&bank->deferred_lock);
+	guard(mutex)(&bank->deferred_lock);
+	ret = rockchip_get_bank_data(bank);
+	if (ret)
+		goto err_disabled_clk;
 
 	ret = rockchip_gpiolib_register(bank);
 	if (ret) {
 		dev_err(bank->dev, "Failed to register gpio %d\n", ret);
-		goto err_unlock;
+		goto err_disabled_clk;
 	}
 
 	while (!list_empty(&bank->deferred_pins)) {
@@ -805,14 +804,11 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 		kfree(cfg);
 	}
 
-	mutex_unlock(&bank->deferred_lock);
 
 	platform_set_drvdata(pdev, bank);
 	dev_info(dev, "probed %pOF\n", np);
 
 	return 0;
-err_unlock:
-	mutex_unlock(&bank->deferred_lock);
 err_disabled_clk:
 	if (bank->manual_clk_release)
 		clk_disable_unprepare(bank->clk);
-- 
2.34.1

Re: [PATCH v3 01/12] gpio: rockchip: avoid division by zero

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:38PM +0800, Ye Zhang wrote:
> If the clk_get_rate return '0', it will happen division by zero.

I don't understand the circumstances when it may happen.

> Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")

Not sure that this actually fixes anything. See below why I think so.

...

>  	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {

Here you explicitly checked that the clock is provided by DT.

...

>  		freq = clk_get_rate(bank->db_clk);

Here you read the frequency which may be 0 in two cases:
1) in DT explicitly set to 0;
2) CCF is disabled.

So, wrong DTs have to be validated / fixed beforehand, correct?

But if the CCF is disabled, the db_clk is NULL. Moreover I don't see
how the db_clk may contain error pointer as you have it filtered out
at _get_bank_data(). So, maybe what you need is to have NULL check
in the conditional and explaining more in the commit message why it
is currently a problematic code?

> +		if (!freq)
> +			return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/12] gpio: rockchip: resolve underflow issue

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:41PM +0800, Ye Zhang wrote:
> div_reg may be < 0 if debounce is zero, causing the unsigned int to
> overflow.

...

> -	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
> +	if ((bank->gpio_type == GPIO_TYPE_V2) && !IS_ERR(bank->db_clk))

Stray change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/12] gpio: rockchip: support 'clock-names' from dt nodes

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:44PM +0800, Ye Zhang wrote:
> Added support for retrieving clocks using 'clock-names' from dt nodes

...

> +	bank->clk = devm_clk_get_enabled(dev, "bus");

I would use _optional() variant.

> +	if (IS_ERR(bank->clk)) {
> +		bank->clk = of_clk_get(dev->of_node, 0);
> +		if (IS_ERR(bank->clk)) {
> +			dev_err(dev, "fail to get apb clock\n");
> +			return PTR_ERR(bank->clk);
> +		}

> +		clk_prepare_enable(bank->clk);

Why? How was it tested?

> +		bank->manual_clk_release = true;
> +	}

It seems you need to unnest the conditionals and think through the logic.

> +	bank->db_clk = devm_clk_get_enabled(dev, "db");
> +	if (IS_ERR(bank->db_clk)) {
> +		bank->db_clk = of_clk_get(dev->of_node, 1);
> +		if (IS_ERR(bank->db_clk)) {
> +			bank->db_clk = NULL;
> +		} else {
> +			clk_prepare_enable(bank->db_clk);

Ditto.

> +			bank->manual_dbclk_release = true;
> +		}
> +	}

...

>  	ret = rockchip_get_bank_data(bank);
>  	if (ret)
> -		return ret;
> +		goto err_disabled_clk;

No, just make sure it's properly wrapped by devm.

...

> +	bool				manual_clk_release;
> +	bool				manual_dbclk_release;

Why do you need them?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/12] gpio: rockchip: Update the GPIO driver

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:37PM +0800, Ye Zhang wrote:
> GPIO driver support acpi and new version, set input direction in
> irq_request_resources, fix division error and debounce config error.

Looking at patch 7 it seems the new feature has been barely tested.
First of all, I recommend to split out the real fixes for now and send them
separately while working on the rest. Second, perform better bisectability
(both compile and run time) and overall test coverage in different scenarios.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/12] gpio: rockchip: explan the format of the GPIO version ID

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:45PM +0800, Ye Zhang wrote:
> Remove redundant comments and provide a detailed explanation of the
> GPIO version ID.

"explain" in the Subject.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
(and maybe others)

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Bart, I think this one is good to go (with spelling fix). And it makes these
values to be described which looks like a win to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/12] gpio: rockchip: change the GPIO version judgment logic

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:46PM +0800, Ye Zhang wrote:
> Have a list of valid V1 IDs and default to -ENODEV.

s/V1//

...

>  	id = readl(bank->reg_base + gpio_regs_v2.version_id);

>  

You may remove this blank line now...

> -	/* If not gpio v2, that is default to v1. */
> -	if (id == GPIO_TYPE_V2 || id == GPIO_TYPE_V2_1) {
> -		bank->gpio_regs = &gpio_regs_v2;
> -		bank->gpio_type = GPIO_TYPE_V2;
> -	} else {
> +	/* The GPIO version ID is incrementing. */
> +	switch (id) {

...basically to have

	/* The GPIO version ID is incrementing. */
	id = readl(bank->reg_base + gpio_regs_v2.version_id);
	switch (id) {

> +	case GPIO_TYPE_V1:
>  		bank->gpio_regs = &gpio_regs_v1;
>  		bank->gpio_type = GPIO_TYPE_V1;
> +		break;
> +	case GPIO_TYPE_V2:
> +	case GPIO_TYPE_V2_1:
> +		bank->gpio_regs = &gpio_regs_v2;
> +		bank->gpio_type = GPIO_TYPE_V2;
> +		break;
> +	default:
> +		dev_err(bank->dev, "cannot get the version ID\n");
> +		return -ENODEV;
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/12] gpio: rockchip: support new version gpio

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:47PM +0800, Ye Zhang wrote:
> The next version gpio controller on SoCs like rk3576 which support four

GPIO

> OS operation and four interrupts

operations ?

Also missing period at the end.

...

What does this all mean?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/12] gpio: rockchip: Set input direction when request irq

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:48PM +0800, Ye Zhang wrote:
> Since the GPIO can only generate interrupts when its direction is set to
> input, it is set to input before requesting the interrupt resources.

...

>  static int rockchip_irq_reqres(struct irq_data *d)
>  {

> +	irq_hw_number_t hwirq;
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	struct rockchip_pin_bank *bank = gc->private;
>  
> -	return gpiochip_reqres_irq(&bank->gpio_chip, d->hwirq);
> +	hwirq = irqd_to_hwirq(d);
> +	rockchip_gpio_direction_input(&bank->gpio_chip, hwirq);

	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
	struct rockchip_pin_bank *bank = gc->private;
	irq_hw_number_t hwirq = irqd_to_hwirq(d);

	rockchip_gpio_direction_input(&bank->gpio_chip, hwirq);

> +	return gpiochip_reqres_irq(&bank->gpio_chip, hwirq);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 12/12] gpio: rockchip: replace mutex_lock() with guard()

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 03:36:49PM +0800, Ye Zhang wrote:
> Replacing mutex_lock with guard() simplifies the code and helps avoid

mutex_lock()

avoiding

> deadlocks.

...

> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c

+ cleanup.h

...

>  	}
> -	mutex_lock(&bank->deferred_lock);
> +	guard(mutex)(&bank->deferred_lock);

Make it surrounded by blank lines.

> +	ret = rockchip_get_bank_data(bank);
> +	if (ret)
> +		goto err_disabled_clk;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 12/12] gpio: rockchip: replace mutex_lock() with guard()

[Christophe JAILLET]

Le 03/09/2024 à 09:36, Ye Zhang a écrit :
> Replacing mutex_lock with guard() simplifies the code and helps avoid
> deadlocks.
> 
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> ---
>   drivers/gpio/gpio-rockchip.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 73e57efb46fc..d5c57617fc86 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -765,20 +765,19 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	ret = rockchip_get_bank_data(bank);
> -	if (ret)
> -		goto err_disabled_clk;
> -
>   	/*
>   	 * Prevent clashes with a deferred output setting
>   	 * being added right at this moment.
>   	 */
> -	mutex_lock(&bank->deferred_lock);
> +	guard(mutex)(&bank->deferred_lock);
> +	ret = rockchip_get_bank_data(bank);

rockchip_get_bank_data() was out of the lock before, now it is inside.

It looks ok, but is it on purpose? If so, maybe it could be mentioned or 
explained why in the changelog ?

CJ

> +	if (ret)
> +		goto err_disabled_clk;
>   
>   	ret = rockchip_gpiolib_register(bank);
>   	if (ret) {
>   		dev_err(bank->dev, "Failed to register gpio %d\n", ret);
> -		goto err_unlock;
> +		goto err_disabled_clk;
>   	}
>   
>   	while (!list_empty(&bank->deferred_pins)) {
> @@ -805,14 +804,11 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>   		kfree(cfg);
>   	}
>   
> -	mutex_unlock(&bank->deferred_lock);
>   
>   	platform_set_drvdata(pdev, bank);
>   	dev_info(dev, "probed %pOF\n", np);
>   
>   	return 0;
> -err_unlock:
> -	mutex_unlock(&bank->deferred_lock);
>   err_disabled_clk:
>   	if (bank->manual_clk_release)
>   		clk_disable_unprepare(bank->clk);

Re: [PATCH v3 12/12] gpio: rockchip: replace mutex_lock() with guard()

[Christophe JAILLET]

Le 03/09/2024 à 09:36, Ye Zhang a écrit :
> Replacing mutex_lock with guard() simplifies the code and helps avoid
> deadlocks.
> 
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> ---
>   drivers/gpio/gpio-rockchip.c | 14 +++++---------
>   1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 73e57efb46fc..d5c57617fc86 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -765,20 +765,19 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>   		}
>   	}
>   
> -	ret = rockchip_get_bank_data(bank);
> -	if (ret)
> -		goto err_disabled_clk;
> -
>   	/*
>   	 * Prevent clashes with a deferred output setting
>   	 * being added right at this moment.
>   	 */
> -	mutex_lock(&bank->deferred_lock);
> +	guard(mutex)(&bank->deferred_lock);
> +	ret = rockchip_get_bank_data(bank);

rockchip_get_bank_data() was out of the lock before, now it is inside.

It looks ok, but is it on purpose? If so, maybe it could be mentioned or 
explained why in the changelog ?

CJ

> +	if (ret)
> +		goto err_disabled_clk;
>   
>   	ret = rockchip_gpiolib_register(bank);
>   	if (ret) {
>   		dev_err(bank->dev, "Failed to register gpio %d\n", ret);
> -		goto err_unlock;
> +		goto err_disabled_clk;
>   	}
>   
>   	while (!list_empty(&bank->deferred_pins)) {
> @@ -805,14 +804,11 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>   		kfree(cfg);
>   	}
>   
> -	mutex_unlock(&bank->deferred_lock);
>   
>   	platform_set_drvdata(pdev, bank);
>   	dev_info(dev, "probed %pOF\n", np);
>   
>   	return 0;
> -err_unlock:
> -	mutex_unlock(&bank->deferred_lock);
>   err_disabled_clk:
>   	if (bank->manual_clk_release)
>   		clk_disable_unprepare(bank->clk);