[PATCH v5 0/4] gpio: rockchip: Update the GPIO driver

[PATCH v5 0/4] 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.

Changes since v3:
- Give up modifying the debounce config because it is not actually used.
- Do not add support for retrieving clocks using 'clock-names'.

Change since v4:
- Leave the V2 case first and the V1 case last in version judgment.
- Fix the error message in version judgment.

Ye Zhang (4):
  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

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

-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 1/4] 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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.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 365ab947983c..702e711f9c23 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -26,9 +26,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]  - Revision 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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 2/4] gpio: rockchip: change the GPIO version judgment logic

[Ye Zhang]

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

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpio/gpio-rockchip.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 702e711f9c23..49eaefeed8fa 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -667,8 +667,9 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 	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) {
+	switch (id) {
+	case GPIO_TYPE_V2:
+	case 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);
@@ -677,9 +678,14 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 			clk_disable_unprepare(bank->clk);
 			return -EINVAL;
 		}
-	} else {
+		break;
+	case GPIO_TYPE_V1:
 		bank->gpio_regs = &gpio_regs_v1;
 		bank->gpio_type = GPIO_TYPE_V1;
+		break;
+	default:
+		dev_err(bank->dev, "unsupported version ID: 0x%08x\n", id);
+		return -ENODEV;
 	}
 
 	return 0;
-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 2/4] gpio: rockchip: change the GPIO version judgment logic

[Andy Shevchenko]

On Tue, Nov 12, 2024 at 09:54:06AM +0800, Ye Zhang wrote:
> Have a list of valid IDs and default to -ENODEV.

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

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 3/4] gpio: rockchip: support new version GPIO

[Ye Zhang]

Support the next version GPIO controller on SoCs like rk3576.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.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 49eaefeed8fa..c090cac694bf 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -35,6 +35,7 @@
 #define GPIO_TYPE_V1		(0)           /* GPIO Version ID reserved */
 #define GPIO_TYPE_V2		(0x01000C2B)
 #define GPIO_TYPE_V2_1		(0x0101157C)
+#define GPIO_TYPE_V2_2		(0x010219C8)
 
 static const struct rockchip_gpio_regs gpio_regs_v1 = {
 	.port_dr = 0x00,
@@ -670,6 +671,7 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 	switch (id) {
 	case GPIO_TYPE_V2:
 	case GPIO_TYPE_V2_1:
+	case GPIO_TYPE_V2_2:
 		bank->gpio_regs = &gpio_regs_v2;
 		bank->gpio_type = GPIO_TYPE_V2;
 		bank->db_clk = of_clk_get(bank->of_node, 1);
-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v5 4/4] 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>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpio/gpio-rockchip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index c090cac694bf..cdfdd5501a1e 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -476,8 +476,11 @@ 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;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	return gpiochip_reqres_irq(&bank->gpio_chip, d->hwirq);
+	rockchip_gpio_direction_input(&bank->gpio_chip, hwirq);
+
+	return gpiochip_reqres_irq(&bank->gpio_chip, hwirq);
 }
 
 static void rockchip_irq_relres(struct irq_data *d)
-- 
2.34.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

[Bartosz Golaszewski]

On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> 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.
>
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index c090cac694bf..cdfdd5501a1e 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -476,8 +476,11 @@ 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;
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
>
> -       return gpiochip_reqres_irq(&bank->gpio_chip, d->hwirq);
> +       rockchip_gpio_direction_input(&bank->gpio_chip, hwirq);
> +
> +       return gpiochip_reqres_irq(&bank->gpio_chip, hwirq);
>  }
>
>  static void rockchip_irq_relres(struct irq_data *d)
> --
> 2.34.1
>

This looks like a fix to me, do you want it sent for stable? If so,
please add the Fixes tag and put it first in the series.

Bart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

[Andy Shevchenko]

On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> 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.

...

> This looks like a fix to me, do you want it sent for stable? If so,
> please add the Fixes tag and put it first in the series.

Independently on the resolution on this, can the first three be applied to
for-next? I think they are valuable from the documentation perspective as
it adds the explanation of the version register bit fields.

The last one seems to me independent (code wise, meaning no potential
conflicts) to the rest and may be applied to for-current later on.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

[Bartosz Golaszewski]

On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> 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.
>
> ...
>
> > This looks like a fix to me, do you want it sent for stable? If so,
> > please add the Fixes tag and put it first in the series.
>
> Independently on the resolution on this, can the first three be applied to
> for-next? I think they are valuable from the documentation perspective as
> it adds the explanation of the version register bit fields.
>
> The last one seems to me independent (code wise, meaning no potential
> conflicts) to the rest and may be applied to for-current later on.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

There's another issue I see with this patch. It effectively changes
the pin's direction behind the back of the GPIOLIB. If a GPIO is
requested, its direction set to output and another orthogonal user
requests the same pin as input, we'll never update the FLAG_IS_OUT
value and I don't think any subsequent behavior can be considered
defined.

I applied the first 3 patches as they look alright.

Bart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

[Bartosz Golaszewski]

On Tue, Nov 12, 2024 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> 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.
> >
> > ...
> >
> > > This looks like a fix to me, do you want it sent for stable? If so,
> > > please add the Fixes tag and put it first in the series.
> >
> > Independently on the resolution on this, can the first three be applied to
> > for-next? I think they are valuable from the documentation perspective as
> > it adds the explanation of the version register bit fields.
> >
> > The last one seems to me independent (code wise, meaning no potential
> > conflicts) to the rest and may be applied to for-current later on.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> There's another issue I see with this patch. It effectively changes
> the pin's direction behind the back of the GPIOLIB. If a GPIO is
> requested, its direction set to output and another orthogonal user
> requests the same pin as input, we'll never update the FLAG_IS_OUT

I meant to say "same pin as interrupt". Sorry for the noise.

Bart

> value and I don't think any subsequent behavior can be considered
> defined.
>
> I applied the first 3 patches as they look alright.
>
> Bart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1930 bytes --]

Hi,

On Tue, Nov 12, 2024 at 01:53:48PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 12, 2024 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> 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.
> > >
> > > ...
> > >
> > > > This looks like a fix to me, do you want it sent for stable? If so,
> > > > please add the Fixes tag and put it first in the series.
> > >
> > > Independently on the resolution on this, can the first three be applied to
> > > for-next? I think they are valuable from the documentation perspective as
> > > it adds the explanation of the version register bit fields.
> > >
> > > The last one seems to me independent (code wise, meaning no potential
> > > conflicts) to the rest and may be applied to for-current later on.
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> >
> > There's another issue I see with this patch. It effectively changes
> > the pin's direction behind the back of the GPIOLIB. If a GPIO is
> > requested, its direction set to output and another orthogonal user
> > requests the same pin as input, we'll never update the FLAG_IS_OUT
> 
> I meant to say "same pin as interrupt". Sorry for the noise.

GPIO output and interrupt at the same time looks like a misconfiguration
to me. Maybe check for that situation and return -EBUSY?

-- Sebastian

> 
> Bart
> 
> > value and I don't think any subsequent behavior can be considered
> > defined.
> >
> > I applied the first 3 patches as they look alright.
> >
> > Bart

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

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

[Bartosz Golaszewski]

On Tue, Nov 12, 2024 at 2:23 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Tue, Nov 12, 2024 at 01:53:48PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 12, 2024 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> 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.
> > > >
> > > > ...
> > > >
> > > > > This looks like a fix to me, do you want it sent for stable? If so,
> > > > > please add the Fixes tag and put it first in the series.
> > > >
> > > > Independently on the resolution on this, can the first three be applied to
> > > > for-next? I think they are valuable from the documentation perspective as
> > > > it adds the explanation of the version register bit fields.
> > > >
> > > > The last one seems to me independent (code wise, meaning no potential
> > > > conflicts) to the rest and may be applied to for-current later on.
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > >
> > > >
> > >
> > > There's another issue I see with this patch. It effectively changes
> > > the pin's direction behind the back of the GPIOLIB. If a GPIO is
> > > requested, its direction set to output and another orthogonal user
> > > requests the same pin as input, we'll never update the FLAG_IS_OUT
> >
> > I meant to say "same pin as interrupt". Sorry for the noise.
>
> GPIO output and interrupt at the same time looks like a misconfiguration
> to me. Maybe check for that situation and return -EBUSY?
>

Of course it is. That's why we check for it in
gpiod_direction_output() and in gpiochip_lock_as_irq(): refuse to set
direction to output if the GPIO is used as interrupt and refuse to use
it as interrupt if direction already is output respectively.

This patch however proposes to set the direction to input implicitly
which is wrong. The underlying gpiochip_reqres_irq() will check the
current direction and return -EIO if it's output.

Bart

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: (subset) [PATCH v5 0/4] gpio: rockchip: Update the GPIO driver

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Tue, 12 Nov 2024 09:54:04 +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.
> 
> Changes since v1:
> - Split commits with multiple changes into separate commits.
> - Adjust backportable fix to the forefront.
> - Modify messages of some commits.
> 
> [...]

Applied, thanks!

[1/4] gpio: rockchip: explan the format of the GPIO version ID
      commit: 591ae6bed250e4067db926313ff7279d23a1c7d1
[2/4] gpio: rockchip: change the GPIO version judgment logic
      commit: 41209307cad7f14c387c68375a93b50e54261a53
[3/4] gpio: rockchip: support new version GPIO
      commit: 8bcbd0379c05c66ce2e842c7e8901aa317cdf04e

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip