[PATCH v2] gpio: rockchip: Update the GPIO driver

[PATCH v2] 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. 

Ye Zhang (11):
  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: support new version gpio
  gpio: rockchip: Set input direction when request irq
  gpio: rockchip: support ACPI
  gpio: rockchip: driver works without pinctrl device

 drivers/gpio/gpio-rockchip.c | 297 +++++++++++++++++++++--------------
 1 file changed, 181 insertions(+), 116 deletions(-)

-- 
2.34.1

[PATCH v2] 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

Re: [PATCH v2] gpio: rockchip: avoid division by zero

[Heiko Stübner]

Am Freitag, 23. August 2024, 05:43:04 CEST schrieb 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>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  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;
> 

Re: [PATCH v2] gpio: rockchip: avoid division by zero

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:43:04AM +0800, Ye Zhang wrote:
> If the clk_get_rate return '0', it will happen division by zero.

At the abstraction level this is good to avoid 0 division and return an error,
but...

>  		freq = clk_get_rate(bank->db_clk);
> +		if (!freq)
> +			return -EINVAL;

...do you this the absence of debounce here is a fatal error?
(Yes, I see it's a fatal when it's bigger than maximum.)

>  		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
>  		if (debounce > max_debounce)
>  			return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] 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

Re: [PATCH v2] gpio: rockchip: release reference to device node

[Heiko Stübner]

Am Freitag, 23. August 2024, 05:43:05 CEST schrieb 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>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v2] gpio: rockchip: release reference to device node

[Shawn Lin]

在 2024/8/23 11:43, 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>

Reviewed-by: Shawn Lin <shawn.lin@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;
>   

[PATCH v2] 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 5f60162baaeb..bf22b103b6a2 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -209,11 +209,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 * 1000000;
+		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

Re: [PATCH v2] gpio: rockchip: resolve overflow issues

[Heiko Stübner]

Am Freitag, 23. August 2024, 05:43:06 CEST schrieb Ye Zhang:
> Prevent overflow issues when performing debounce-related calculations.

Please add some more explanation here.
I.e. something about previous max_debounce calculation does overflow
the type of max_debounce

Thanks
Heiko

> Fixes: 3bcbd1a85b68 ("gpio/rockchip: support next version gpio controller")
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 5f60162baaeb..bf22b103b6a2 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -209,11 +209,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 * 1000000;
> +		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;
> 

Re: [PATCH v2] gpio: rockchip: resolve overflow issues

[Shawn Lin]

在 2024/8/23 11:43, 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 | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 5f60162baaeb..bf22b103b6a2 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -209,11 +209,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 * 1000000;
> +		max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);

can't max_debounce = DIV_ROUND_CLOSEST_ULL((GENMASK(23, 0) + 1) * 2 *
1000000, freq) work?

>   		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;

Re: [PATCH v2] gpio: rockchip: resolve overflow issues

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 06:59:40PM +0800, Shawn Lin wrote:
> 在 2024/8/23 11:43, Ye Zhang 写道:

...

> > -		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
> > +		div = (u64)(GENMASK(23, 0) + 1) * 2 * 1000000;
> > +		max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
> 
> can't max_debounce = DIV_ROUND_CLOSEST_ULL((GENMASK(23, 0) + 1) * 2 *
> 1000000, freq) work?

Wouldn't be too long line in this case?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: rockchip: resolve overflow issues

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:43:06AM +0800, Ye Zhang wrote:
> Prevent overflow issues when performing debounce-related calculations.

...

> -		max_debounce = (GENMASK(23, 0) + 1) * 2 * 1000000 / freq;
> +		div = (u64)(GENMASK(23, 0) + 1) * 2 * 1000000;

You probably want to use HZ_PER_MHZ from units.h or so?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] 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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index bf22b103b6a2..c3a87d075908 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -204,8 +204,8 @@ 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)) {
-		div_debounce_support = true;
+	div_debounce_support = (bank->gpio_type == GPIO_TYPE_V2) && !IS_ERR(bank->db_clk);
+	if (debounce && div_debounce_support) {
 		freq = clk_get_rate(bank->db_clk);
 		if (!freq)
 			return -EINVAL;
@@ -216,8 +216,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

Re: [PATCH v2] gpio: rockchip: resolve underflow issue

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:43:07AM +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)) {
> -		div_debounce_support = true;

Wouldn't be cleaner (from the patch perspective) to simply move else branch here?

	else
		div_debounce_support = false;

> +	div_debounce_support = (bank->gpio_type == GPIO_TYPE_V2) && !IS_ERR(bank->db_clk);
> +	if (debounce && div_debounce_support) {
>  		freq = clk_get_rate(bank->db_clk);
>  		if (!freq)
>  			return -EINVAL;
> @@ -216,8 +216,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;
>  	}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] 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 c3a87d075908..c246f116a3b5 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -209,13 +209,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 * 1000000;
+		div = (u64)(GENMASK(23, 0) + 1) * 1000000;
 		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 v2] gpio: rockchip: Update debounce config function

[Ye Zhang]

In the GPIO with version number 0x01000C2B, debounce configuration is
already supported.

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

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index c246f116a3b5..aff8bec79062 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -250,6 +250,8 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 			clk_prepare_enable(bank->db_clk);
 		else
 			clk_disable_unprepare(bank->db_clk);
+	} else {
+		return -ENOTSUPP;
 	}
 
 	return 0;
@@ -278,22 +280,11 @@ static int rockchip_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
 				  unsigned long config)
 {
 	enum pin_config_param param = pinconf_to_config_param(config);
+	unsigned int debounce = pinconf_to_config_argument(config);
 
 	switch (param) {
 	case PIN_CONFIG_INPUT_DEBOUNCE:
-		rockchip_gpio_set_debounce(gc, offset, true);
-		/*
-		 * Rockchip's gpio could only support up to one period
-		 * of the debounce clock(pclk), which is far away from
-		 * satisftying the requirement, as pclk is usually near
-		 * 100MHz shared by all peripherals. So the fact is it
-		 * has crippled debounce capability could only be useful
-		 * to prevent any spurious glitches from waking up the system
-		 * if the gpio is conguired as wakeup interrupt source. Let's
-		 * still return -ENOTSUPP as before, to make sure the caller
-		 * of gpiod_set_debounce won't change its behaviour.
-		 */
-		return -ENOTSUPP;
+		return rockchip_gpio_set_debounce(gc, offset, debounce);
 	default:
 		return -ENOTSUPP;
 	}
-- 
2.34.1

[PATCH v2] gpio: rockchip: support 'clock-names' from dt nodes

[Ye Zhang]

Added support for retrieving clocks using 'clock-names' from dt nodes

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

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index aff8bec79062..25ddf6a82c09 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -646,23 +646,12 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 	if (!bank->irq)
 		return -EINVAL;
 
-	bank->clk = of_clk_get(bank->of_node, 0);
-	if (IS_ERR(bank->clk))
-		return PTR_ERR(bank->clk);
-
-	clk_prepare_enable(bank->clk);
 	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;
-		bank->db_clk = of_clk_get(bank->of_node, 1);
-		if (IS_ERR(bank->db_clk)) {
-			dev_err(bank->dev, "cannot find debounce clk\n");
-			clk_disable_unprepare(bank->clk);
-			return -EINVAL;
-		}
 	} else {
 		bank->gpio_regs = &gpio_regs_v1;
 		bank->gpio_type = GPIO_TYPE_V1;
@@ -722,10 +711,29 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 
 	raw_spin_lock_init(&bank->slock);
 
+	bank->clk = devm_clk_get(dev, "bus");
+	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);
+		}
+	}
+
 	ret = rockchip_get_bank_data(bank);
 	if (ret)
 		return ret;
 
+	bank->db_clk = devm_clk_get(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;
+	}
+
+	clk_prepare_enable(bank->clk);
+	clk_prepare_enable(bank->db_clk);
+
 	/*
 	 * Prevent clashes with a deferred output setting
 	 * being added right at this moment.
@@ -734,9 +742,8 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 
 	ret = rockchip_gpiolib_register(bank);
 	if (ret) {
-		clk_disable_unprepare(bank->clk);
-		mutex_unlock(&bank->deferred_lock);
-		return ret;
+		dev_err(bank->dev, "Failed to register gpio %d\n", ret);
+		goto err_unlock;
 	}
 
 	while (!list_empty(&bank->deferred_pins)) {
@@ -769,6 +776,12 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 	dev_info(dev, "probed %pOF\n", np);
 
 	return 0;
+err_unlock:
+	mutex_unlock(&bank->deferred_lock);
+	clk_disable_unprepare(bank->clk);
+	clk_disable_unprepare(bank->db_clk);
+
+	return ret;
 }
 
 static void rockchip_gpio_remove(struct platform_device *pdev)
@@ -776,6 +789,7 @@ static void rockchip_gpio_remove(struct platform_device *pdev)
 	struct rockchip_pin_bank *bank = platform_get_drvdata(pdev);
 
 	clk_disable_unprepare(bank->clk);
+	clk_disable_unprepare(bank->db_clk);
 	gpiochip_remove(&bank->gpio_chip);
 }
 
-- 
2.34.1

Re: [PATCH v2] gpio: rockchip: support 'clock-names' from dt nodes

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:43:10AM +0800, Ye Zhang wrote:
> Added support for retrieving clocks using 'clock-names' from dt nodes

...

> +	bank->clk = devm_clk_get(dev, "bus");
> +	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);

			return dev_err_probe(...);

> +		}
> +	}

...

> +	clk_prepare_enable(bank->clk);
> +	clk_prepare_enable(bank->db_clk);

Any of this may fail. You perhaps want

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

Not sure for the fallback case (of_clk_get() should be killed as we almost done
in GPIO library for GPIO APIs).

...

> +err_unlock:
> +	mutex_unlock(&bank->deferred_lock);

Don't you want to use guard()/scoped_guard() at some point?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] gpio: rockchip: support new version gpio

[Ye Zhang]

The next version gpio controller on SoCs like rk3576 which support four
OS operation and four interrupts

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

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 25ddf6a82c09..5289c94d5c60 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -29,6 +29,7 @@
 #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_2		(0x010219C8)  /* GPIO Version ID 0x010219C8 */
 
 static const struct rockchip_gpio_regs gpio_regs_v1 = {
 	.port_dr = 0x00,
@@ -78,7 +79,7 @@ static inline void rockchip_gpio_writel(struct rockchip_pin_bank *bank,
 {
 	void __iomem *reg = bank->reg_base + offset;
 
-	if (bank->gpio_type == GPIO_TYPE_V2)
+	if (bank->gpio_type >= GPIO_TYPE_V2)
 		gpio_writel_v2(value, reg);
 	else
 		writel(value, reg);
@@ -90,7 +91,7 @@ static inline u32 rockchip_gpio_readl(struct rockchip_pin_bank *bank,
 	void __iomem *reg = bank->reg_base + offset;
 	u32 value;
 
-	if (bank->gpio_type == GPIO_TYPE_V2)
+	if (bank->gpio_type >= GPIO_TYPE_V2)
 		value = gpio_readl_v2(reg);
 	else
 		value = readl(reg);
@@ -105,7 +106,7 @@ static inline void rockchip_gpio_writel_bit(struct rockchip_pin_bank *bank,
 	void __iomem *reg = bank->reg_base + offset;
 	u32 data;
 
-	if (bank->gpio_type == GPIO_TYPE_V2) {
+	if (bank->gpio_type >= GPIO_TYPE_V2) {
 		if (value)
 			data = BIT(bit % 16) | BIT(bit % 16 + 16);
 		else
@@ -126,7 +127,7 @@ static inline u32 rockchip_gpio_readl_bit(struct rockchip_pin_bank *bank,
 	void __iomem *reg = bank->reg_base + offset;
 	u32 data;
 
-	if (bank->gpio_type == GPIO_TYPE_V2) {
+	if (bank->gpio_type >= GPIO_TYPE_V2) {
 		data = readl(bit >= 16 ? reg + 0x4 : reg);
 		data >>= bit % 16;
 	} else {
@@ -204,18 +205,24 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 	unsigned int cur_div_reg;
 	u64 div;
 
-	div_debounce_support = (bank->gpio_type == GPIO_TYPE_V2) && !IS_ERR(bank->db_clk);
+	div_debounce_support = (bank->gpio_type >= GPIO_TYPE_V2) && !IS_ERR(bank->db_clk);
 	if (debounce && div_debounce_support) {
 		freq = clk_get_rate(bank->db_clk);
 		if (!freq)
 			return -EINVAL;
 		div = (u64)(GENMASK(23, 0) + 1) * 1000000;
-		max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
+		if (bank->gpio_type == GPIO_TYPE_V2)
+			max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
+		else
+			max_debounce = DIV_ROUND_CLOSEST_ULL(div, 2 * freq);
 		if (debounce > max_debounce)
 			return -EINVAL;
 
 		div = (u64)debounce * freq;
-		div_reg = DIV_ROUND_CLOSEST_ULL(div, USEC_PER_SEC) - 1;
+		if (bank->gpio_type == GPIO_TYPE_V2)
+			div_reg = DIV_ROUND_CLOSEST_ULL(div, USEC_PER_SEC) - 1;
+		else
+			div_reg = DIV_ROUND_CLOSEST_ULL(div, USEC_PER_SEC / 2) - 1;
 	}
 
 	raw_spin_lock_irqsave(&bank->slock, flags);
@@ -401,7 +408,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	polarity = rockchip_gpio_readl(bank, bank->gpio_regs->int_polarity);
 
 	if (type == IRQ_TYPE_EDGE_BOTH) {
-		if (bank->gpio_type == GPIO_TYPE_V2) {
+		if (bank->gpio_type >= GPIO_TYPE_V2) {
 			rockchip_gpio_writel_bit(bank, d->hwirq, 1,
 						 bank->gpio_regs->int_bothedge);
 			goto out;
@@ -420,7 +427,7 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 				polarity |= mask;
 		}
 	} else {
-		if (bank->gpio_type == GPIO_TYPE_V2) {
+		if (bank->gpio_type >= GPIO_TYPE_V2) {
 			rockchip_gpio_writel_bit(bank, d->hwirq, 0,
 						 bank->gpio_regs->int_bothedge);
 		} else {
@@ -526,7 +533,7 @@ static int rockchip_interrupts_register(struct rockchip_pin_bank *bank)
 	}
 
 	gc = irq_get_domain_generic_chip(bank->domain, 0);
-	if (bank->gpio_type == GPIO_TYPE_V2) {
+	if (bank->gpio_type >= GPIO_TYPE_V2) {
 		gc->reg_writel = gpio_writel_v2;
 		gc->reg_readl = gpio_readl_v2;
 	}
@@ -648,13 +655,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) {
+	switch (id) {
+	case GPIO_TYPE_V2:
+	case GPIO_TYPE_V2_1:
 		bank->gpio_regs = &gpio_regs_v2;
 		bank->gpio_type = GPIO_TYPE_V2;
-	} else {
+		break;
+	case GPIO_TYPE_V2_2:
+		bank->gpio_regs = &gpio_regs_v2;
+		bank->gpio_type = GPIO_TYPE_V2_2;
+		break;
+	default:
 		bank->gpio_regs = &gpio_regs_v1;
 		bank->gpio_type = GPIO_TYPE_V1;
+		pr_info("Note: Use default GPIO_TYPE_V1!\n");
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v2] gpio: rockchip: support new version gpio

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

Hi,

On Fri, Aug 23, 2024 at 11:43:11AM GMT, Ye Zhang wrote:
> The next version gpio controller on SoCs like rk3576 which support four
> OS operation and four interrupts
> 
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 40 ++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 25ddf6a82c09..5289c94d5c60 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -29,6 +29,7 @@
>  #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_2		(0x010219C8)  /* GPIO Version ID 0x010219C8 */

The comments for anything but the V1 define are redundant.

[...]

> @@ -648,13 +655,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) {
> +	switch (id) {
> +	case GPIO_TYPE_V2:
> +	case GPIO_TYPE_V2_1:
>  		bank->gpio_regs = &gpio_regs_v2;
>  		bank->gpio_type = GPIO_TYPE_V2;
> -	} else {
> +		break;
> +	case GPIO_TYPE_V2_2:
> +		bank->gpio_regs = &gpio_regs_v2;
> +		bank->gpio_type = GPIO_TYPE_V2_2;
> +		break;

Can't this just be handled like GPIO_TYPE_V2_1 (i.e. reusing the V2
case)? Also it looks risky to use >= on gpio_type. GPIO_TYPE_V2_2
looks like a simple enum, but it contains the ID. Is it guranteed to
be increasing? In that case any ID > GPIO_TYPE_V2_2 should not
default to GPIO_TYPE_V1?

> +	default:
>  		bank->gpio_regs = &gpio_regs_v1;
>  		bank->gpio_type = GPIO_TYPE_V1;
> +		pr_info("Note: Use default GPIO_TYPE_V1!\n");

Can we have a list of valid V1 IDs and default to -ENODEV?

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] gpio: rockchip: support new version gpio

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:43:11AM +0800, Ye Zhang wrote:
> The next version gpio controller on SoCs like rk3576 which support four
> OS operation and four interrupts

...

>  #define GPIO_TYPE_V2		(0x01000C2B)  /* GPIO Version ID 0x01000C2B */
>  #define GPIO_TYPE_V2_1		(0x0101157C)  /* GPIO Version ID 0x0101157C */
> +#define GPIO_TYPE_V2_2		(0x010219C8)  /* GPIO Version ID 0x010219C8 */

These needs a bit of decoding. As far as I can decipher these it's something like

0x01 00 00 00 ???

0x00 xx 00 00 // seems like a subversion, 2.xx

0x00 00 xx xx // seems like a release which is

3115
5500
6600

in decimal representation.

But again, can you make the comments better explaining these cryptic 4-byte
values?

With that done it might be better approach to check the version of the IP.

...

> +	switch (id) {
> +	case GPIO_TYPE_V2:
> +	case GPIO_TYPE_V2_1:
>  		bank->gpio_regs = &gpio_regs_v2;
>  		bank->gpio_type = GPIO_TYPE_V2;
> -	} else {
> +		break;
> +	case GPIO_TYPE_V2_2:
> +		bank->gpio_regs = &gpio_regs_v2;
> +		bank->gpio_type = GPIO_TYPE_V2_2;
> +		break;
> +	default:
>  		bank->gpio_regs = &gpio_regs_v1;
>  		bank->gpio_type = GPIO_TYPE_V1;
> +		pr_info("Note: Use default GPIO_TYPE_V1!\n");

Missed break;

>  	}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] gpio: rockchip: Set input direction when request irq

[Ye Zhang]

Since the GPIO can only generate interrupts when its direction is set to
input, it is set to input before requesting the interrupt resources.

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 5289c94d5c60..4f8d50626fcc 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -469,6 +469,8 @@ static int rockchip_irq_reqres(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct rockchip_pin_bank *bank = gc->private;
 
+	rockchip_gpio_direction_input(&bank->gpio_chip, d->hwirq);
+
 	return gpiochip_reqres_irq(&bank->gpio_chip, d->hwirq);
 }
 
-- 
2.34.1

Re: [PATCH v2] gpio: rockchip: Set input direction when request irq

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:43:12AM +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.

...

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

In both cases you want to use irq_hw_number_t and irqd_to_hwirq() at the top of
the function(s).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] gpio: rockchip: support ACPI

[Ye Zhang]

Adds support for Advanced Configuration and Power Interface (ACPI) within
the Rockchip GPIO driver.

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

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 4f8d50626fcc..9e4a8cd94c66 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2021 Rockchip Electronics Co. Ltd.
  */
 
+#include <linux/acpi.h>
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/device.h>
@@ -17,10 +18,12 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 
 #include "../pinctrl/core.h"
@@ -210,6 +213,7 @@ 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) * 1000000;
 		if (bank->gpio_type == GPIO_TYPE_V2)
 			max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
@@ -515,7 +519,7 @@ static int rockchip_interrupts_register(struct rockchip_pin_bank *bank)
 	struct irq_chip_generic *gc;
 	int ret;
 
-	bank->domain = irq_domain_add_linear(bank->of_node, 32,
+	bank->domain = irq_domain_create_linear(dev_fwnode(bank->dev), 32,
 					&irq_generic_chip_ops, NULL);
 	if (!bank->domain) {
 		dev_warn(bank->dev, "could not init irq domain for bank %s\n",
@@ -637,25 +641,9 @@ static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
 	return ret;
 }
 
-static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
+static void rockchip_gpio_get_ver(struct rockchip_pin_bank *bank)
 {
-	struct resource res;
-	int id = 0;
-
-	if (of_address_to_resource(bank->of_node, 0, &res)) {
-		dev_err(bank->dev, "cannot find IO resource for bank\n");
-		return -ENOENT;
-	}
-
-	bank->reg_base = devm_ioremap_resource(bank->dev, &res);
-	if (IS_ERR(bank->reg_base))
-		return PTR_ERR(bank->reg_base);
-
-	bank->irq = irq_of_parse_and_map(bank->of_node, 0);
-	if (!bank->irq)
-		return -EINVAL;
-
-	id = readl(bank->reg_base + gpio_regs_v2.version_id);
+	int id = readl(bank->reg_base + gpio_regs_v2.version_id);
 
 	switch (id) {
 	case GPIO_TYPE_V2:
@@ -672,8 +660,6 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 		bank->gpio_type = GPIO_TYPE_V1;
 		pr_info("Note: Use default GPIO_TYPE_V1!\n");
 	}
-
-	return 0;
 }
 
 static struct rockchip_pin_bank *
@@ -695,61 +681,120 @@ rockchip_gpio_find_bank(struct pinctrl_dev *pctldev, int id)
 	return found ? bank : NULL;
 }
 
+static int rockchip_gpio_of_get_bank_id(struct device *dev)
+{
+	static int gpio;
+	int bank_id = -1;
+
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+		bank_id = of_alias_get_id(dev->of_node, "gpio");
+		if (bank_id < 0)
+			bank_id = gpio++;
+	}
+
+	return bank_id;
+}
+
+#ifdef CONFIG_ACPI
+static int rockchip_gpio_acpi_get_bank_id(struct device *dev)
+{
+	struct acpi_device *adev;
+	unsigned long bank_id = -1;
+	const char *uid;
+	int ret;
+
+	adev = ACPI_COMPANION(dev);
+	if (!adev)
+		return -ENXIO;
+
+	uid = acpi_device_uid(adev);
+	if (!uid || !(*uid)) {
+		dev_err(dev, "Cannot retrieve UID\n");
+		return -ENODEV;
+	}
+
+	ret = kstrtoul(uid, 0, &bank_id);
+
+	return !ret ? bank_id : -ERANGE;
+}
+#else
+static int rockchip_gpio_acpi_get_bank_id(struct device *dev)
+{
+	return -ENOENT;
+}
+#endif /* CONFIG_ACPI */
+
 static int rockchip_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	struct device_node *pctlnp = of_get_parent(np);
 	struct pinctrl_dev *pctldev = NULL;
 	struct rockchip_pin_bank *bank = NULL;
-	struct rockchip_pin_deferred *cfg;
-	static int gpio;
-	int id, ret;
+	int bank_id = 0;
+	int ret;
 
-	if (!np || !pctlnp)
-		return -ENODEV;
+	bank_id = rockchip_gpio_acpi_get_bank_id(dev);
+	if (bank_id < 0) {
+		bank_id = rockchip_gpio_of_get_bank_id(dev);
+		if (bank_id < 0)
+			return bank_id;
+	}
 
-	pctldev = of_pinctrl_get(pctlnp);
-	of_node_put(pctlnp);
-	if (!pctldev)
-		return -EPROBE_DEFER;
+	if (!ACPI_COMPANION(dev)) {
+		struct device_node *pctlnp = of_get_parent(dev->of_node);
 
-	id = of_alias_get_id(np, "gpio");
-	if (id < 0)
-		id = gpio++;
+		pctldev = of_pinctrl_get(pctlnp);
+		of_node_put(pctlnp);
+		if (!pctldev)
+			return -EPROBE_DEFER;
 
-	bank = rockchip_gpio_find_bank(pctldev, id);
-	if (!bank)
-		return -EINVAL;
+		bank = rockchip_gpio_find_bank(pctldev, bank_id);
+		if (!bank)
+			return -ENODEV;
+	}
 
+	if (!bank) {
+		bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
+		if (!bank)
+			return -ENOMEM;
+	}
+
+	bank->bank_num = bank_id;
 	bank->dev = dev;
-	bank->of_node = np;
+	bank->of_node = dev->of_node;
+
+	bank->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(bank->reg_base))
+		return PTR_ERR(bank->reg_base);
+
+	bank->irq = platform_get_irq(pdev, 0);
+	if (bank->irq < 0)
+		return bank->irq;
 
 	raw_spin_lock_init(&bank->slock);
 
-	bank->clk = devm_clk_get(dev, "bus");
-	if (IS_ERR(bank->clk)) {
-		bank->clk = of_clk_get(dev->of_node, 0);
+	if (!ACPI_COMPANION(dev)) {
+		bank->clk = devm_clk_get(dev, "bus");
 		if (IS_ERR(bank->clk)) {
-			dev_err(dev, "fail to get apb clock\n");
-			return PTR_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);
+			}
 		}
-	}
 
-	ret = rockchip_get_bank_data(bank);
-	if (ret)
-		return ret;
-
-	bank->db_clk = devm_clk_get(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;
+		bank->db_clk = devm_clk_get(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;
+		}
 	}
 
 	clk_prepare_enable(bank->clk);
 	clk_prepare_enable(bank->db_clk);
 
+	rockchip_gpio_get_ver(bank);
+
 	/*
 	 * Prevent clashes with a deferred output setting
 	 * being added right at this moment.
@@ -763,8 +808,13 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 	}
 
 	while (!list_empty(&bank->deferred_pins)) {
+		struct rockchip_pin_deferred *cfg;
+
 		cfg = list_first_entry(&bank->deferred_pins,
 				       struct rockchip_pin_deferred, head);
+		if (!cfg)
+			break;
+
 		list_del(&cfg->head);
 
 		switch (cfg->param) {
@@ -789,7 +839,7 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 	mutex_unlock(&bank->deferred_lock);
 
 	platform_set_drvdata(pdev, bank);
-	dev_info(dev, "probed %pOF\n", np);
+	dev_info(dev, "probed %pfw\n", dev_fwnode(dev));
 
 	return 0;
 err_unlock:
-- 
2.34.1

Re: [PATCH v2] gpio: rockchip: support ACPI

[Mika Westerberg]

Hi,

On Fri, Aug 23, 2024 at 11:43:13AM +0800, Ye Zhang wrote:
> Adds support for Advanced Configuration and Power Interface (ACPI) within
> the Rockchip GPIO driver.
> 
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 160 +++++++++++++++++++++++------------
>  1 file changed, 105 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 4f8d50626fcc..9e4a8cd94c66 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2021 Rockchip Electronics Co. Ltd.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/device.h>
> @@ -17,10 +18,12 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_device.h>

Why this is needed for ACPI support?

>  #include <linux/of_irq.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pinctrl/pinconf-generic.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  
>  #include "../pinctrl/core.h"
> @@ -210,6 +213,7 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
>  		freq = clk_get_rate(bank->db_clk);
>  		if (!freq)
>  			return -EINVAL;
> +

Unrelated whitespace change.

>  		div = (u64)(GENMASK(23, 0) + 1) * 1000000;
>  		if (bank->gpio_type == GPIO_TYPE_V2)
>  			max_debounce = DIV_ROUND_CLOSEST_ULL(div, freq);
> @@ -515,7 +519,7 @@ static int rockchip_interrupts_register(struct rockchip_pin_bank *bank)
>  	struct irq_chip_generic *gc;
>  	int ret;
>  
> -	bank->domain = irq_domain_add_linear(bank->of_node, 32,
> +	bank->domain = irq_domain_create_linear(dev_fwnode(bank->dev), 32,
>  					&irq_generic_chip_ops, NULL);

I wonder if it would be simpler to first convert the driver to use
fwnodes and then in a separate patch, add the ACPI glue?

>  	if (!bank->domain) {
>  		dev_warn(bank->dev, "could not init irq domain for bank %s\n",
> @@ -637,25 +641,9 @@ static int rockchip_gpiolib_register(struct rockchip_pin_bank *bank)
>  	return ret;
>  }
>  
> -static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
> +static void rockchip_gpio_get_ver(struct rockchip_pin_bank *bank)
>  {
> -	struct resource res;
> -	int id = 0;
> -
> -	if (of_address_to_resource(bank->of_node, 0, &res)) {
> -		dev_err(bank->dev, "cannot find IO resource for bank\n");
> -		return -ENOENT;
> -	}
> -
> -	bank->reg_base = devm_ioremap_resource(bank->dev, &res);
> -	if (IS_ERR(bank->reg_base))
> -		return PTR_ERR(bank->reg_base);
> -
> -	bank->irq = irq_of_parse_and_map(bank->of_node, 0);
> -	if (!bank->irq)
> -		return -EINVAL;
> -
> -	id = readl(bank->reg_base + gpio_regs_v2.version_id);
> +	int id = readl(bank->reg_base + gpio_regs_v2.version_id);
>  
>  	switch (id) {
>  	case GPIO_TYPE_V2:
> @@ -672,8 +660,6 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
>  		bank->gpio_type = GPIO_TYPE_V1;
>  		pr_info("Note: Use default GPIO_TYPE_V1!\n");
>  	}
> -
> -	return 0;
>  }
>  
>  static struct rockchip_pin_bank *
> @@ -695,61 +681,120 @@ rockchip_gpio_find_bank(struct pinctrl_dev *pctldev, int id)
>  	return found ? bank : NULL;
>  }
>  
> +static int rockchip_gpio_of_get_bank_id(struct device *dev)
> +{
> +	static int gpio;
> +	int bank_id = -1;

Instead of this -1 probably good to use something like -ENODEV or so.

Also suggest doing the analogous to ACPI way here so that you have the
stub function when CONFIG_OF is not set. This makes it bit more
consistent IMO.

> +
> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> +		bank_id = of_alias_get_id(dev->of_node, "gpio");
> +		if (bank_id < 0)
> +			bank_id = gpio++;
> +	}
> +
> +	return bank_id;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static int rockchip_gpio_acpi_get_bank_id(struct device *dev)
> +{
> +	struct acpi_device *adev;
> +	unsigned long bank_id = -1;

Ditto here regarding -1, also this does not need to be initialized at
all here.

> +	const char *uid;
> +	int ret;
> +
> +	adev = ACPI_COMPANION(dev);
> +	if (!adev)
> +		return -ENXIO;

Or -ENODEV? And is this even possible?

> +
> +	uid = acpi_device_uid(adev);
> +	if (!uid || !(*uid)) {

There are bunch of helpers for this such as acpi_dev_hid_uid_match() and
the like. Not sure if you can use them here but wanted to mention. We
use these in Intel driver for example in intel_pinctrl_get_soc_data() in
case you want to check an example.

Also the !(*uid) looks horrible. Can it be empty string?

> +		dev_err(dev, "Cannot retrieve UID\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = kstrtoul(uid, 0, &bank_id);
> +
> +	return !ret ? bank_id : -ERANGE;
> +}
> +#else
> +static int rockchip_gpio_acpi_get_bank_id(struct device *dev)
> +{
> +	return -ENOENT;
> +}
> +#endif /* CONFIG_ACPI */
> +
>  static int rockchip_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = dev->of_node;
> -	struct device_node *pctlnp = of_get_parent(np);
>  	struct pinctrl_dev *pctldev = NULL;
>  	struct rockchip_pin_bank *bank = NULL;
> -	struct rockchip_pin_deferred *cfg;
> -	static int gpio;
> -	int id, ret;
> +	int bank_id = 0;
> +	int ret;
>  
> -	if (!np || !pctlnp)
> -		return -ENODEV;
> +	bank_id = rockchip_gpio_acpi_get_bank_id(dev);
> +	if (bank_id < 0) {
> +		bank_id = rockchip_gpio_of_get_bank_id(dev);
> +		if (bank_id < 0)
> +			return bank_id;

So here with your of_ function you would get -1 which is -EPERM so which
is probably not what you want.

> +	}
>  
> -	pctldev = of_pinctrl_get(pctlnp);
> -	of_node_put(pctlnp);
> -	if (!pctldev)
> -		return -EPROBE_DEFER;
> +	if (!ACPI_COMPANION(dev)) {
> +		struct device_node *pctlnp = of_get_parent(dev->of_node);

Should it check instead that it has the dev->of_node and not assume that
if it does not have ACPI compantion device it is then enumerated from DT?

> -	id = of_alias_get_id(np, "gpio");
> -	if (id < 0)
> -		id = gpio++;
> +		pctldev = of_pinctrl_get(pctlnp);
> +		of_node_put(pctlnp);
> +		if (!pctldev)
> +			return -EPROBE_DEFER;
>  
> -	bank = rockchip_gpio_find_bank(pctldev, id);
> -	if (!bank)
> -		return -EINVAL;
> +		bank = rockchip_gpio_find_bank(pctldev, bank_id);
> +		if (!bank)
> +			return -ENODEV;
> +	}
>  
> +	if (!bank) {
> +		bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
> +		if (!bank)
> +			return -ENOMEM;
> +	}
> +
> +	bank->bank_num = bank_id;
>  	bank->dev = dev;
> -	bank->of_node = np;
> +	bank->of_node = dev->of_node;
> +
> +	bank->reg_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(bank->reg_base))
> +		return PTR_ERR(bank->reg_base);
> +
> +	bank->irq = platform_get_irq(pdev, 0);
> +	if (bank->irq < 0)
> +		return bank->irq;
>  
>  	raw_spin_lock_init(&bank->slock);
>  
> -	bank->clk = devm_clk_get(dev, "bus");
> -	if (IS_ERR(bank->clk)) {
> -		bank->clk = of_clk_get(dev->of_node, 0);
> +	if (!ACPI_COMPANION(dev)) {

Ditto here.

Also maybe it needs to have separate probe paths for ACPI and DT because
this makes it look kind of confusing.

> +		bank->clk = devm_clk_get(dev, "bus");
>  		if (IS_ERR(bank->clk)) {
> -			dev_err(dev, "fail to get apb clock\n");
> -			return PTR_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);
> +			}
>  		}
> -	}
>  
> -	ret = rockchip_get_bank_data(bank);
> -	if (ret)
> -		return ret;
> -
> -	bank->db_clk = devm_clk_get(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;
> +		bank->db_clk = devm_clk_get(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;
> +		}
>  	}
>  
>  	clk_prepare_enable(bank->clk);
>  	clk_prepare_enable(bank->db_clk);
>  
> +	rockchip_gpio_get_ver(bank);
> +
>  	/*
>  	 * Prevent clashes with a deferred output setting
>  	 * being added right at this moment.
> @@ -763,8 +808,13 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	while (!list_empty(&bank->deferred_pins)) {
> +		struct rockchip_pin_deferred *cfg;
> +
>  		cfg = list_first_entry(&bank->deferred_pins,
>  				       struct rockchip_pin_deferred, head);
> +		if (!cfg)
> +			break;
> +

These changes look unrelated?

>  		list_del(&cfg->head);
>  
>  		switch (cfg->param) {
> @@ -789,7 +839,7 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>  	mutex_unlock(&bank->deferred_lock);
>  
>  	platform_set_drvdata(pdev, bank);
> -	dev_info(dev, "probed %pOF\n", np);
> +	dev_info(dev, "probed %pfw\n", dev_fwnode(dev));

Is this really useful inro for user to see?

>  
>  	return 0;
>  err_unlock:
> -- 
> 2.34.1

Re: [PATCH v2] gpio: rockchip: Update the GPIO driver

[Bartosz Golaszewski]

On Fri, Aug 23, 2024 at 5:45 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
>
> 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.
>
> Ye Zhang (11):
>   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: support new version gpio
>   gpio: rockchip: Set input direction when request irq
>   gpio: rockchip: support ACPI
>   gpio: rockchip: driver works without pinctrl device
>
>  drivers/gpio/gpio-rockchip.c | 297 +++++++++++++++++++++--------------
>  1 file changed, 181 insertions(+), 116 deletions(-)
>
> --
> 2.34.1
>

I think something went wrong here, the patches are not numbered like
they should.

Bart

Re: [PATCH v2] gpio: rockchip: Update the GPIO driver

[Dragan Simic]

Hello Ye,

On 2024-08-23 05:43, Ye Zhang wrote:
> 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.

It seems that you sent a bunch of separate patches, instead of sending
a patch series.  I believe that wasn't intended or requested, so could
you, please, resend the v2 patches as a series?

> Ye Zhang (11):
>   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: support new version gpio
>   gpio: rockchip: Set input direction when request irq
>   gpio: rockchip: support ACPI
>   gpio: rockchip: driver works without pinctrl device
> 
>  drivers/gpio/gpio-rockchip.c | 297 +++++++++++++++++++++--------------
>  1 file changed, 181 insertions(+), 116 deletions(-)

Re: [PATCH v2] gpio: rockchip: Update the GPIO driver

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 09:06:50AM +0200, Dragan Simic wrote:
> Hello Ye,
> 
> On 2024-08-23 05:43, Ye Zhang wrote:
> > 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.
> 
> It seems that you sent a bunch of separate patches, instead of sending
> a patch series.  I believe that wasn't intended or requested, so could
> you, please, resend the v2 patches as a series?

No resend, please!

Rather, address the comments you have got so far and send properly a v3.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: rockchip: Update the GPIO driver

[Heiko Stübner]

Hi Ye Zhang,

Am Freitag, 23. August 2024, 05:43:03 CEST schrieb 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. 
> 
> Ye Zhang (11):
>   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: support new version gpio
>   gpio: rockchip: Set input direction when request irq
>   gpio: rockchip: support ACPI
>   gpio: rockchip: driver works without pinctrl device

this is a patch series, so in theory 

	git format-patch --11 --cover-letter

should have generated subjects like
	[PATCH v2 00/11] ....
	[PATCH v2 01/11] gpio: rockchip: avoid division by zero
and so on.

As you've seen in the replies people get confused, but also kernel patch-
handling tools get confused :-) .


Heiko