[PATCH v3 0/2] Add gpio driver support for Realtek DHC SoCs

[PATCH v3 0/2] Add gpio driver support for Realtek DHC SoCs

[TY Chang]

These patches add the bindings and the gpio driver for Realtek
DHC(Digital Home Center) RTD SoCs, including RTD1295, RTD1395,
RTD1619, RTD1319, RTD1619B, RTD1319D and RTD1315E.

Change log:
v2->v3:
1. Remove generic compatible and use SoC-specific compatible instead.
2. Add the missing descriptions for the rtd_gpio_info structure members.
3. Assign gpio_chip fwnode.
v1->v2:
1. Add description for DHC RTD SoCs in the bindings.
2. Revise the compatible names in the bindings.
3. Transitioned from OF API to platform_device API.
4. Use u8 for the offset array within the rtd_gpio_info structure.
5. Record the size of each array within the rtd_gpio_info structure and
   implement checks to prevent out-of-bounds access.
6. Use GPIOLIB_IRQCHIP helpers to register interrupts.
7. Use dynamic allocation for GPIO base.

Tzuyi Chang (2):
  dt-bindings: gpio: realtek: Add realtek,rtd-gpio
  Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

 .../bindings/gpio/realtek,rtd-gpio.yaml       |  69 ++
 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-rtd.c                       | 748 ++++++++++++++++++
 4 files changed, 827 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
 create mode 100644 drivers/gpio/gpio-rtd.c

-- 
2.43.0

[PATCH v3 1/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio

[TY Chang]

From: Tzuyi Chang <tychang@realtek.com>

Add the device tree bindings for the Realtek DHC(Digital Home Center)
RTD SoCs GPIO controllers.

Signed-off-by: Tzuyi Chang <tychang@realtek.com>
---
v2 to v3 change:
    1. Remove generic compatible and use SoC-specific compatible instead.
v1 to v2 change:
    1. Add description for DHC RTD SoCs.
    2. Revise the compatible names.
    3. Add descriptions for reg and interrupts properties.
---
 .../bindings/gpio/realtek,rtd-gpio.yaml       | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
new file mode 100644
index 000000000000..984e7dbd322e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Realtek Semiconductor Corporation
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/realtek,rtd-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek DHC GPIO controller
+
+maintainers:
+  - Tzuyi Chang <tychang@realtek.com>
+
+description:
+  The GPIO controller is designed for the Realtek DHC (Digital Home Center)
+  RTD series SoC family, which are high-definition media processor SoCs.
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd1295-misc-gpio
+      - realtek,rtd1295-iso-gpio
+      - realtek,rtd1395-iso-gpio
+      - realtek,rtd1619-iso-gpio
+      - realtek,rtd1319-iso-gpio
+      - realtek,rtd1619b-iso-gpio
+      - realtek,rtd1319d-iso-gpio
+      - realtek,rtd1315e-iso-gpio
+
+  reg:
+    items:
+      - description: GPIO controller registers
+      - description: GPIO interrupt registers
+
+  interrupts:
+    items:
+      - description: Interrupt number of the assert GPIO interrupt, which is
+                     triggered when there is a rising edge.
+      - description: Interrupt number of the deassert GPIO interrupt, which is
+                     triggered when there is a falling edge.
+
+  gpio-ranges: true
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - gpio-ranges
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio@100 {
+      compatible = "realtek,rtd1319d-iso-gpio";
+      reg = <0x100 0x100>,
+            <0x000 0x0b0>;
+      interrupt-parent = <&iso_irq_mux>;
+      interrupts = <19>, <20>;
+      gpio-ranges = <&pinctrl 0 0 82>;
+      gpio-controller;
+      #gpio-cells = <2>;
+    };
-- 
2.43.0

Re: [PATCH v3 1/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio

[Krzysztof Kozlowski]

On 07/12/2023 11:07, TY Chang wrote:
> From: Tzuyi Chang <tychang@realtek.com>
> 
> Add the device tree bindings for the Realtek DHC(Digital Home Center)
> RTD SoCs GPIO controllers.
> 
> Signed-off-by: Tzuyi Chang <tychang@realtek.com>
> ---
> v2 to v3 change:
>     1. Remove generic compatible and use SoC-specific compatible instead.
> v1 to v2 change:
>     1. Add description for DHC RTD SoCs.
>     2. Revise the compatible names.
>     3. Add descriptions for reg and interrupts properties.
> ---
>  .../bindings/gpio/realtek,rtd-gpio.yaml       | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
> new file mode 100644
> index 000000000000..984e7dbd322e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/realtek,rtd-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC GPIO controller
> +
> +maintainers:
> +  - Tzuyi Chang <tychang@realtek.com>
> +
> +description:
> +  The GPIO controller is designed for the Realtek DHC (Digital Home Center)
> +  RTD series SoC family, which are high-definition media processor SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - realtek,rtd1295-misc-gpio
> +      - realtek,rtd1295-iso-gpio
> +      - realtek,rtd1395-iso-gpio
> +      - realtek,rtd1619-iso-gpio
> +      - realtek,rtd1319-iso-gpio
> +      - realtek,rtd1619b-iso-gpio
> +      - realtek,rtd1319d-iso-gpio
> +      - realtek,rtd1315e-iso-gpio

If there is going to be resend, please order this list alphanumerically.

> +
> +  reg:
> +    items:
> +      - description: GPIO controller registers
> +      - description: GPIO interrupt registers
> +

...

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio@100 {
> +      compatible = "realtek,rtd1319d-iso-gpio";
> +      reg = <0x100 0x100>,
> +            <0x000 0x0b0>;

That's odd. Why order is decreasing? Isn't 0x0 the address of the SoC
(soc@0)?

It is, btw, 0x0, not 0x000. The same for 0x0b0 -> 0xb0, unless you want
to pad to full word.

Best regards,
Krzysztof

RE: [PATCH v3 1/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio

[TY_Chang[張子逸]]

Hi Krzysztof,

>> diff --git
>> a/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
>> b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
>> new file mode 100644
>> index 000000000000..984e7dbd322e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
>> @@ -0,0 +1,69 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2023
>> +Realtek Semiconductor Corporation %YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/gpio/realtek,rtd-gpio.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Realtek DHC GPIO controller
>> +
>> +maintainers:
>> +  - Tzuyi Chang <tychang@realtek.com>
>> +
>> +description:
>> +  The GPIO controller is designed for the Realtek DHC (Digital Home
>> +Center)
>> +  RTD series SoC family, which are high-definition media processor SoCs.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - realtek,rtd1295-misc-gpio
>> +      - realtek,rtd1295-iso-gpio
>> +      - realtek,rtd1395-iso-gpio
>> +      - realtek,rtd1619-iso-gpio
>> +      - realtek,rtd1319-iso-gpio
>> +      - realtek,rtd1619b-iso-gpio
>> +      - realtek,rtd1319d-iso-gpio
>> +      - realtek,rtd1315e-iso-gpio
>
>If there is going to be resend, please order this list alphanumerically.
>

I will revise it in the next version

>> +
>> +  reg:
>> +    items:
>> +      - description: GPIO controller registers
>> +      - description: GPIO interrupt registers
>> +
>
>...
>
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    gpio@100 {
>> +      compatible = "realtek,rtd1319d-iso-gpio";
>> +      reg = <0x100 0x100>,
>> +            <0x000 0x0b0>;
>
>That's odd. Why order is decreasing? Isn't 0x0 the address of the SoC (soc@0)?
>

The DTS use in our platform follows this structure:

soc@0 {
    rbus: rbus@98000000 {
	    ...
        iso: syscon@7000 {
		    ...
            gpio: gpio@100 {
                compatible = "realtek,rtd1319d-iso-gpio";
                reg = <0x100 0x100>,
                      <0x000 0x0b0>;
                interrupt-parent = <&iso_irq_mux>;
                interrupts = <19>, <20>;
                gpio-ranges = <&pinctrl 0 0 82>;
                gpio-controller;
                #gpio-cells = <2>;
            };
        };
    };
};

The base address for the GPIO controller is 0x98007100. The second line of
'reg' refers to the GPIO interrupt status registers, which are distributed
within the range of 0x98007000 to 0x980070AF. Would it be advisable to fetch the
syscon from parent node(iso: syscon@7000) to handle the GPIO interrupt status
registers?

>It is, btw, 0x0, not 0x000. The same for 0x0b0 -> 0xb0, unless you want to pad
>to full word.
>

I will revise it in the next version.

Thanks,
Tzuyi Chang

Re: [PATCH v3 1/2] dt-bindings: gpio: realtek: Add realtek,rtd-gpio

[Krzysztof Kozlowski]

On 08/12/2023 10:03, TY_Chang[張子逸] wrote:

> 
> The DTS use in our platform follows this structure:
> 
> soc@0 {
>     rbus: rbus@98000000 {
> 	    ...
>         iso: syscon@7000 {
> 		    ...
>             gpio: gpio@100 {
>                 compatible = "realtek,rtd1319d-iso-gpio";
>                 reg = <0x100 0x100>,
>                       <0x000 0x0b0>;
>                 interrupt-parent = <&iso_irq_mux>;
>                 interrupts = <19>, <20>;
>                 gpio-ranges = <&pinctrl 0 0 82>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>             };
>         };
>     };
> };
> 
> The base address for the GPIO controller is 0x98007100. The second line of
> 'reg' refers to the GPIO interrupt status registers, which are distributed
> within the range of 0x98007000 to 0x980070AF. 

Ah, ok.

> Would it be advisable to fetch the
> syscon from parent node(iso: syscon@7000) to handle the GPIO interrupt status
> registers?

If these are GPIO interrupts then current description is fine.

> 
>> It is, btw, 0x0, not 0x000. The same for 0x0b0 -> 0xb0, unless you want to pad
>> to full word.
>>

Best regards,
Krzysztof

[PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[TY Chang]

From: Tzuyi Chang <tychang@realtek.com>

This driver enables configuration of GPIO direction, GPIO values, GPIO
debounce settings and handles GPIO interrupts.

Signed-off-by: Tzuyi Chang <tychang@realtek.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
v2 to v3 change:
    1. Remove generic compatible and use SoC-specific compatible instead.
    2. Add the missing descriptions for the rtd_gpio_info structure members.
    3. Assign gpio_chip fwnode.
v1 to v2 change:
    1. Remove legacy headers.
    2. Transitioned from OF API to platform_device API.
    3. Use u8 for the offset member within the rtd_gpio_info structure.
    4. Record the size of each array within the rtd_gpio_info structure and
       implement checks to prevent out-of-bounds access.
    5. Use GPIOLIB_IRQCHIP helpers to register interrupts.
    6. Use dynamic allocation for GPIO base.
---
 drivers/gpio/Kconfig    |   9 +
 drivers/gpio/Makefile   |   1 +
 drivers/gpio/gpio-rtd.c | 748 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 758 insertions(+)
 create mode 100644 drivers/gpio/gpio-rtd.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b3a133ed31ee..f0bdf9dbdefc 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -553,6 +553,15 @@ config GPIO_ROCKCHIP
 	help
 	  Say yes here to support GPIO on Rockchip SoCs.
 
+config GPIO_RTD
+	tristate "Realtek DHC GPIO support"
+	depends on ARCH_REALTEK
+	default y
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support GPIO on Realtek DHC(Digital Home Center)
+	  SoCs.
+
 config GPIO_SAMA5D2_PIOBU
 	tristate "SAMA5D2 PIOBU GPIO support"
 	depends on MFD_SYSCON
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index eb73b5d633eb..16bb40717e87 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -137,6 +137,7 @@ obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
 obj-$(CONFIG_GPIO_ROCKCHIP)	+= gpio-rockchip.o
+obj-$(CONFIG_GPIO_RTD)			+= gpio-rtd.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
diff --git a/drivers/gpio/gpio-rtd.c b/drivers/gpio/gpio-rtd.c
new file mode 100644
index 000000000000..f3489b636bc7
--- /dev/null
+++ b/drivers/gpio/gpio-rtd.c
@@ -0,0 +1,748 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Realtek DHC gpio driver
+ *
+ * Copyright (c) 2023 Realtek Semiconductor Corp.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/spinlock.h>
+
+#define RTD_GPIO_DEBOUNCE_1US 0
+#define RTD_GPIO_DEBOUNCE_10US 1
+#define RTD_GPIO_DEBOUNCE_100US 2
+#define RTD_GPIO_DEBOUNCE_1MS 3
+#define RTD_GPIO_DEBOUNCE_10MS 4
+#define RTD_GPIO_DEBOUNCE_20MS 5
+#define RTD_GPIO_DEBOUNCE_30MS 6
+
+enum rtd_gpio_type {
+	RTD_ISO_GPIO = 0,
+	RTD1295_ISO_GPIO,
+	RTD1295_MISC_GPIO,
+	RTD1395_ISO_GPIO,
+	RTD1619_ISO_GPIO,
+};
+
+/**
+ * struct rtd_gpio_info - Specific GPIO register information
+ * @name: GPIO device name
+ * @type: RTD GPIO ID
+ * @gpio_base: GPIO base number
+ * @num_gpios: The number of GPIOs
+ * @dir_offset: Offset for GPIO direction registers
+ * @num_dir: The number of GPIO direction registers
+ * @dato_offset: Offset for GPIO data output registers
+ * @num_dato: The number of GPIO data output registers
+ * @dati_offset: Offset for GPIO data input registers
+ * @num_dati: The number of GPIO data input registers
+ * @ie_offset: Offset for GPIO interrupt enable registers
+ * @num_ie: The number of GPIO interrupt enable registers
+ * @dp_offset: Offset for GPIO detection polarity registers
+ * @num_dp: The number of GPIO detection polarity registers
+ * @gpa_offset: Offset for GPIO assert interrupt status registers
+ * @num_gpa: The number of GPIO assert interrupt status registers
+ * @gpda_offset: Offset for GPIO deassert interrupt status registers
+ * @num_gpda: The number of GPIO deassert interrupt status registers
+ * @deb_offset: Offset for GPIO debounce registers
+ * @num_deb: The number of GPIO debounce registers
+ */
+struct rtd_gpio_info {
+	const char		*name;
+	enum rtd_gpio_type	type;
+	unsigned int		gpio_base;
+	unsigned int		num_gpios;
+	u8			*dir_offset;
+	u8			num_dir;
+	u8			*dato_offset;
+	u8			num_dato;
+	u8			*dati_offset;
+	u8			num_dati;
+	u8			*ie_offset;
+	u8			num_ie;
+	u8			*dp_offset;
+	u8			num_dp;
+	u8			*gpa_offset;
+	u8			num_gpa;
+	u8			*gpda_offset;
+	u8			num_gpda;
+	u8			*deb_offset;
+	u8			num_deb;
+};
+
+struct rtd_gpio {
+	struct platform_device		*pdev;
+	const struct rtd_gpio_info	*info;
+	void __iomem			*base;
+	void __iomem			*irq_base;
+	struct gpio_chip		gpio_chip;
+	unsigned int			irqs[2];
+	spinlock_t			lock;
+};
+
+
+static const struct rtd_gpio_info rtd_iso_gpio_info = {
+	.name		= "rtd_iso_gpio",
+	.type		= RTD_ISO_GPIO,
+	.gpio_base	= 0,
+	.num_gpios	= 82,
+	.dir_offset	= (u8 []){ 0x0, 0x18, 0x2c },
+	.num_dir	= 3,
+	.dato_offset	= (u8 []){ 0x4, 0x1c, 0x30 },
+	.num_dato	= 3,
+	.dati_offset	= (u8 []){ 0x8, 0x20, 0x34 },
+	.num_dati	= 3,
+	.ie_offset	= (u8 []){ 0xc, 0x24, 0x38 },
+	.num_ie		= 3,
+	.dp_offset	= (u8 []){ 0x10, 0x28, 0x3c },
+	.num_dp		= 3,
+	.gpa_offset	= (u8 []){ 0x8, 0xe0, 0x90 },
+	.num_gpa	= 3,
+	.gpda_offset	= (u8 []){ 0xc, 0xe4, 0x94 },
+	.num_gpda	= 3,
+	.deb_offset	= (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
+				   0x60, 0x64, 0x68, 0x6c },
+	.num_deb	= 11,
+};
+
+static const struct rtd_gpio_info rtd1619_iso_gpio_info = {
+	.name		= "rtd1619_iso_gpio",
+	.type		= RTD1619_ISO_GPIO,
+	.gpio_base	= 0,
+	.num_gpios	= 86,
+	.dir_offset	= (u8 []){ 0x0, 0x18, 0x2c },
+	.num_dir	= 3,
+	.dato_offset	= (u8 []){ 0x4, 0x1c, 0x30 },
+	.num_dato	= 3,
+	.dati_offset	= (u8 []){ 0x8, 0x20, 0x34 },
+	.num_dati	= 3,
+	.ie_offset	= (u8 []){ 0xc, 0x24, 0x38 },
+	.num_ie		= 3,
+	.dp_offset	= (u8 []){ 0x10, 0x28, 0x3c },
+	.num_dp		= 3,
+	.gpa_offset	= (u8 []){ 0x8, 0xe0, 0x90 },
+	.num_gpa	= 3,
+	.gpda_offset	= (u8 []){ 0xc, 0xe4, 0x94 },
+	.num_gpda	= 3,
+	.deb_offset	= (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
+				   0x60, 0x64, 0x68, 0x6c },
+	.num_deb	= 11,
+};
+
+static const struct rtd_gpio_info rtd1395_iso_gpio_info = {
+	.name		= "rtd1395_iso_gpio",
+	.type		= RTD1395_ISO_GPIO,
+	.gpio_base	= 0,
+	.num_gpios	= 57,
+	.dir_offset	= (u8 []){ 0x0, 0x18 },
+	.num_dir	= 2,
+	.dato_offset	= (u8 []){ 0x4, 0x1c },
+	.num_dato	= 2,
+	.dati_offset	= (u8 []){ 0x8, 0x20 },
+	.num_dati	= 2,
+	.ie_offset	= (u8 []){ 0xc, 0x24 },
+	.num_ie		= 2,
+	.dp_offset	= (u8 []){ 0x10, 0x28 },
+	.num_dp		= 2,
+	.gpa_offset	= (u8 []){ 0x8, 0xe0 },
+	.num_gpa	= 2,
+	.gpda_offset	= (u8 []){ 0xc, 0xe4 },
+	.num_gpda	= 2,
+	.deb_offset	= (u8 []){ 0x30, 0x34, 0x38, 0x3c, 0x40, 0x44, 0x48, 0x4c },
+	.num_deb	= 8,
+};
+
+static const struct rtd_gpio_info rtd1295_misc_gpio_info = {
+	.name		= "rtd1295_misc_gpio",
+	.type		= RTD1295_ISO_GPIO,
+	.gpio_base	= 0,
+	.num_gpios	= 101,
+	.dir_offset	= (u8 []){ 0x0, 0x4, 0x8, 0xc },
+	.num_dir	= 4,
+	.dato_offset	= (u8 []){ 0x10, 0x14, 0x18, 0x1c },
+	.num_dato	= 4,
+	.dati_offset	= (u8 []){ 0x20, 0x24, 0x28, 0x2c },
+	.num_dati	= 4,
+	.ie_offset	= (u8 []){ 0x30, 0x34, 0x38, 0x3c },
+	.num_ie		= 4,
+	.dp_offset	= (u8 []){ 0x40, 0x44, 0x48, 0x4c },
+	.num_dp		= 4,
+	.gpa_offset	= (u8 []){ 0x40, 0x44, 0xa4, 0xb8 },
+	.num_gpa	= 4,
+	.gpda_offset	= (u8 []){ 0x54, 0x58, 0xa8, 0xbc},
+	.num_gpda	= 4,
+	.deb_offset	= (u8 []){ 0x50 },
+	.num_deb	= 1,
+};
+
+static const struct rtd_gpio_info rtd1295_iso_gpio_info = {
+	.name		= "rtd1295_iso_gpio",
+	.type		= RTD1295_ISO_GPIO,
+	.gpio_base	= 101,
+	.num_gpios	= 35,
+	.dir_offset	= (u8 []){ 0x0, 0x18 },
+	.num_dir	= 2,
+	.dato_offset	= (u8 []){ 0x4, 0x1c },
+	.num_dato	= 2,
+	.dati_offset	= (u8 []){ 0x8, 0x20 },
+	.num_dati	= 2,
+	.ie_offset	= (u8 []){ 0xc, 0x24 },
+	.num_ie		= 2,
+	.dp_offset	= (u8 []){ 0x10, 0x28 },
+	.num_dp		= 2,
+	.gpa_offset	= (u8 []){ 0x8, 0xe0 },
+	.num_gpa	= 2,
+	.gpda_offset	= (u8 []){ 0xc, 0xe4 },
+	.num_gpda	= 2,
+	.deb_offset	= (u8 []){ 0x14 },
+	.num_deb	= 1,
+};
+
+static int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 32;
+
+	if (index > data->info->num_dir)
+		return -EINVAL;
+
+	return data->info->dir_offset[index];
+}
+
+static int rtd_gpio_dato_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 32;
+
+	if (index > data->info->num_dato)
+		return -EINVAL;
+
+	return data->info->dato_offset[index];
+}
+
+static int rtd_gpio_dati_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 32;
+
+	if (index > data->info->num_dati)
+		return -EINVAL;
+
+	return data->info->dati_offset[index];
+}
+
+static int rtd_gpio_ie_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 32;
+
+	if (index > data->info->num_ie)
+		return -EINVAL;
+
+	return data->info->ie_offset[index];
+}
+
+static int rtd_gpio_dp_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 32;
+
+	if (index > data->info->num_dp)
+		return -EINVAL;
+
+	return data->info->dp_offset[index];
+}
+
+static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 31;
+
+	if (index > data->info->num_gpa)
+		return -EINVAL;
+
+	return data->info->gpa_offset[index];
+}
+
+static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 31;
+
+	if (index > data->info->num_gpda)
+		return -EINVAL;
+
+	return data->info->gpda_offset[index];
+}
+
+static int rtd_gpio_deb_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	int index = offset / 8;
+
+	if (index > data->info->num_deb)
+		return -EINVAL;
+
+	return data->info->deb_offset[index];
+}
+
+static int rtd_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
+				   unsigned int debounce)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	unsigned int write_en;
+	unsigned long flags;
+	unsigned int shift;
+	int reg_offset;
+	u32 deb_val;
+	u32 val;
+
+	switch (debounce) {
+	case 1:
+		deb_val = RTD_GPIO_DEBOUNCE_1US;
+		break;
+	case 10:
+		deb_val = RTD_GPIO_DEBOUNCE_10US;
+		break;
+	case 100:
+		deb_val = RTD_GPIO_DEBOUNCE_100US;
+		break;
+	case 1000:
+		deb_val = RTD_GPIO_DEBOUNCE_1MS;
+		break;
+	case 10000:
+		deb_val = RTD_GPIO_DEBOUNCE_10MS;
+		break;
+	case 20000:
+		deb_val = RTD_GPIO_DEBOUNCE_20MS;
+		break;
+	case 30000:
+		deb_val = RTD_GPIO_DEBOUNCE_30MS;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	if (data->info->type == RTD1295_ISO_GPIO) {
+		reg_offset = rtd_gpio_deb_offset(data, 0);
+		if (reg_offset < 0)
+			return reg_offset;
+		shift = 0;
+		deb_val += 1;
+		write_en = BIT(shift + 3);
+	} else if (data->info->type == RTD1295_MISC_GPIO) {
+		reg_offset = rtd_gpio_deb_offset(data, 0);
+		if (reg_offset < 0)
+			return reg_offset;
+		shift = (offset >> 4) * 4;
+		deb_val += 1;
+		write_en = BIT(shift + 3);
+	} else {
+		reg_offset = rtd_gpio_deb_offset(data, offset);
+		if (reg_offset < 0)
+			return reg_offset;
+		shift = (offset % 8) * 4;
+		write_en = BIT(shift + 3);
+	}
+	val = (deb_val << shift) | write_en;
+
+	spin_lock_irqsave(&data->lock, flags);
+	writel_relaxed(val, data->base + reg_offset);
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static int rtd_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				 unsigned long config)
+{
+	int debounce;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_PULL_UP:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		return gpiochip_generic_config(chip, offset, config);
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		debounce = pinconf_to_config_argument(config);
+		return rtd_gpio_set_debounce(chip, offset, debounce);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	unsigned long flags;
+	int reg_offset;
+	u32 val;
+
+	reg_offset = rtd_gpio_dir_offset(data, offset);
+	if (reg_offset < 0)
+		return reg_offset;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	val = readl_relaxed(data->base + reg_offset);
+	val &= BIT(offset % 32);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int rtd_gpio_set_direction(struct gpio_chip *chip, unsigned int offset, bool out)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	u32 mask = BIT(offset % 32);
+	unsigned long flags;
+	int reg_offset;
+	u32 val;
+
+	reg_offset = rtd_gpio_dir_offset(data, offset);
+	if (reg_offset < 0)
+		return reg_offset;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	val = readl_relaxed(data->base + reg_offset);
+	if (out)
+		val |= mask;
+	else
+		val &= ~mask;
+	writel_relaxed(val, data->base + reg_offset);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static int rtd_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	return rtd_gpio_set_direction(chip, offset, false);
+}
+
+static int rtd_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	chip->set(chip, offset, value);
+
+	return rtd_gpio_set_direction(chip, offset, true);
+}
+
+static void rtd_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	u32 mask = BIT(offset % 32);
+	unsigned long flags;
+	int dato_reg_offset;
+	u32 val;
+
+	dato_reg_offset = rtd_gpio_dato_offset(data, offset);
+	if (dato_reg_offset < 0)
+		return;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	val = readl_relaxed(data->base + dato_reg_offset);
+	if (value)
+		val |= mask;
+	else
+		val &= ~mask;
+	writel_relaxed(val, data->base + dato_reg_offset);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static int rtd_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	int dir_reg_offset, dat_reg_offset;
+	unsigned long flags;
+	u32 val;
+
+	dir_reg_offset = rtd_gpio_dir_offset(data, offset);
+	if (dir_reg_offset < 0)
+		return dir_reg_offset;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	val = readl_relaxed(data->base + dir_reg_offset);
+	val &= BIT(offset % 32);
+	dat_reg_offset = val ?
+			 rtd_gpio_dato_offset(data, offset) : rtd_gpio_dati_offset(data, offset);
+
+	val = readl_relaxed(data->base + dat_reg_offset);
+	val >>= offset % 32;
+	val &= 0x1;
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return val;
+}
+
+static bool rtd_gpio_check_ie(struct rtd_gpio *data, int irq)
+{
+	int mask = BIT(irq % 32);
+	int ie_reg_offset;
+	u32 enable;
+
+	ie_reg_offset = rtd_gpio_ie_offset(data, irq);
+	if (ie_reg_offset < 0)
+		return ie_reg_offset;
+	enable = readl_relaxed(data->base + ie_reg_offset);
+
+	return enable & mask;
+}
+
+static void rtd_gpio_irq_handle(struct irq_desc *desc)
+{
+	int (*get_reg_offset)(struct rtd_gpio *gpio, unsigned int offset);
+	struct rtd_gpio *data = irq_desc_get_handler_data(desc);
+	struct irq_domain *domain = data->gpio_chip.irq.domain;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int irq = irq_desc_get_irq(desc);
+	int reg_offset;
+	u32 status;
+	int hwirq;
+	int i;
+	int j;
+
+	chained_irq_enter(chip, desc);
+
+	if (irq == data->irqs[0])
+		get_reg_offset = &rtd_gpio_gpa_offset;
+	else if (irq == data->irqs[1])
+		get_reg_offset = &rtd_gpio_gpda_offset;
+
+	for (i = 0; i < data->info->num_gpios; i = i + 31) {
+		reg_offset = get_reg_offset(data, i);
+		if (reg_offset < 0)
+			return;
+
+		status = readl_relaxed(data->irq_base + reg_offset) >> 1;
+		writel_relaxed(status << 1, data->irq_base + reg_offset);
+
+		while (status) {
+			j = __ffs(status);
+			status &= ~BIT(j);
+			hwirq = i + j;
+			if (rtd_gpio_check_ie(data, hwirq)) {
+				int girq = irq_find_mapping(domain, hwirq);
+				u32 irq_type = irq_get_trigger_type(girq);
+
+				if ((irq == data->irqs[1]) && ((irq_type & IRQ_TYPE_SENSE_MASK) !=
+					IRQ_TYPE_EDGE_BOTH))
+					break;
+				generic_handle_irq(girq);
+			}
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void rtd_gpio_enable_irq(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct rtd_gpio *data = gpiochip_get_data(gc);
+	u32 clr_mask = BIT(d->hwirq % 31) << 1;
+	u32 ie_mask = BIT(d->hwirq % 32);
+	unsigned long flags;
+	int gpda_reg_offset;
+	int gpa_reg_offset;
+	int ie_reg_offset;
+	u32 val;
+
+	ie_reg_offset = rtd_gpio_ie_offset(data, d->hwirq);
+	if (ie_reg_offset < 0)
+		return;
+	gpa_reg_offset = rtd_gpio_gpa_offset(data, d->hwirq);
+	if (gpa_reg_offset < 0)
+		return;
+	gpda_reg_offset = rtd_gpio_gpda_offset(data, d->hwirq);
+	if (gpda_reg_offset < 0)
+		return;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	writel_relaxed(clr_mask, data->irq_base + gpa_reg_offset);
+	writel_relaxed(clr_mask, data->irq_base + gpda_reg_offset);
+
+	val = readl_relaxed(data->base + ie_reg_offset);
+	val |= ie_mask;
+	writel_relaxed(val, data->base + ie_reg_offset);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+}
+
+static void rtd_gpio_disable_irq(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct rtd_gpio *data = gpiochip_get_data(gc);
+	u32 ie_mask = BIT(d->hwirq % 32);
+	unsigned long flags;
+	int ie_reg_offset;
+	u32 val;
+
+	ie_reg_offset = rtd_gpio_ie_offset(data, d->hwirq);
+	if (ie_reg_offset < 0)
+		return;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	val = readl_relaxed(data->base + ie_reg_offset);
+	val &= ~ie_mask;
+	writel_relaxed(val, data->base + ie_reg_offset);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static int rtd_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct rtd_gpio *data = gpiochip_get_data(gc);
+	u32 mask = BIT(d->hwirq % 32);
+	unsigned long flags;
+	int dp_reg_offset;
+	bool polarity;
+	u32 val;
+
+	dp_reg_offset = rtd_gpio_dp_offset(data, d->hwirq);
+	if (dp_reg_offset < 0)
+		return dp_reg_offset;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		polarity = 1;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		polarity = 0;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		polarity = 1;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	val = readl_relaxed(data->base + dp_reg_offset);
+	if (polarity)
+		val |= mask;
+	else
+		val &= ~mask;
+	writel_relaxed(val, data->base + dp_reg_offset);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+
+	return 0;
+}
+
+static const struct irq_chip rtd_gpio_irq_chip = {
+	.name = "rtd-gpio",
+	.irq_enable = rtd_gpio_enable_irq,
+	.irq_disable = rtd_gpio_disable_irq,
+	.irq_set_type = rtd_gpio_irq_set_type,
+	.flags = IRQCHIP_IMMUTABLE,
+};
+
+static const struct of_device_id rtd_gpio_of_matches[] = {
+	{ .compatible = "realtek,rtd1295-misc-gpio", .data = &rtd1295_misc_gpio_info },
+	{ .compatible = "realtek,rtd1295-iso-gpio", .data = &rtd1295_iso_gpio_info },
+	{ .compatible = "realtek,rtd1395-iso-gpio", .data = &rtd1395_iso_gpio_info },
+	{ .compatible = "realtek,rtd1619-iso-gpio", .data = &rtd1619_iso_gpio_info },
+	{ .compatible = "realtek,rtd1319-iso-gpio", .data = &rtd_iso_gpio_info },
+	{ .compatible = "realtek,rtd1619b-iso-gpio", .data = &rtd_iso_gpio_info },
+	{ .compatible = "realtek,rtd1319d-iso-gpio", .data = &rtd_iso_gpio_info },
+	{ .compatible = "realtek,rtd1315e-iso-gpio", .data = &rtd_iso_gpio_info },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);
+
+static int rtd_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_irq_chip *irq_chip;
+	struct rtd_gpio *data;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->irqs[0] = platform_get_irq(pdev, 0);
+	if (data->irqs[0] < 0)
+		return data->irqs[0];
+
+	data->irqs[1] = platform_get_irq(pdev, 1);
+	if (data->irqs[1] < 0)
+		return data->irqs[1];
+
+	data->info = device_get_match_data(dev);
+	if (!data->info)
+		return -EINVAL;
+
+	spin_lock_init(&data->lock);
+
+	data->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(data->base))
+		return PTR_ERR(data->base);
+
+	data->irq_base = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(data->irq_base))
+		return PTR_ERR(data->irq_base);
+
+	data->gpio_chip.label = dev_name(&pdev->dev);
+	data->gpio_chip.base = -1;
+	data->gpio_chip.ngpio = data->info->num_gpios;
+	data->gpio_chip.request = gpiochip_generic_request;
+	data->gpio_chip.free = gpiochip_generic_free;
+	data->gpio_chip.get_direction = rtd_gpio_get_direction;
+	data->gpio_chip.direction_input = rtd_gpio_direction_input;
+	data->gpio_chip.direction_output = rtd_gpio_direction_output;
+	data->gpio_chip.set = rtd_gpio_set;
+	data->gpio_chip.get = rtd_gpio_get;
+	data->gpio_chip.set_config = rtd_gpio_set_config;
+	data->gpio_chip.fwnode = dev_fwnode(&pdev->dev);
+
+	irq_chip = &data->gpio_chip.irq;
+	irq_chip->handler = handle_simple_irq;
+	irq_chip->default_type = IRQ_TYPE_NONE;
+	irq_chip->parent_handler = rtd_gpio_irq_handle;
+	irq_chip->parent_handler_data = data;
+	irq_chip->num_parents = 2;
+	irq_chip->parents = data->irqs;
+
+	gpio_irq_chip_set_chip(irq_chip, &rtd_gpio_irq_chip);
+
+	return devm_gpiochip_add_data(&pdev->dev, &data->gpio_chip, data);
+}
+
+static struct platform_driver rtd_gpio_platform_driver = {
+	.driver = {
+		.name = "gpio-rtd",
+		.of_match_table = rtd_gpio_of_matches,
+	},
+	.probe = rtd_gpio_probe,
+};
+
+static int rtd_gpio_init(void)
+{
+	return platform_driver_register(&rtd_gpio_platform_driver);
+}
+
+subsys_initcall(rtd_gpio_init);
+
+static void __exit rtd_gpio_exit(void)
+{
+	platform_driver_unregister(&rtd_gpio_platform_driver);
+}
+module_exit(rtd_gpio_exit);
+
+MODULE_DESCRIPTION("Realtek DHC SoC gpio driver");
+MODULE_LICENSE("GPL v2");
-- 
2.43.0

Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Andy Shevchenko]

On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
> From: Tzuyi Chang <tychang@realtek.com>
> 
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.

Why gpio-regmap can't be used?

...

> +struct rtd_gpio_info {

> +	u8			*dir_offset;
> +	u8			num_dir;
> +	u8			*dato_offset;
> +	u8			num_dato;
> +	u8			*dati_offset;
> +	u8			num_dati;
> +	u8			*ie_offset;
> +	u8			num_ie;
> +	u8			*dp_offset;
> +	u8			num_dp;
> +	u8			*gpa_offset;
> +	u8			num_gpa;
> +	u8			*gpda_offset;
> +	u8			num_gpda;
> +	u8			*deb_offset;
> +	u8			num_deb;

A lot of wasted space. Can you group pointers followed by u8 members?
Note, use `pahole` tool to check the struct layout in C.

> +};

...

> +struct rtd_gpio {
> +	struct platform_device		*pdev;

Why

	struct device *dev;

is not suffice?

> +	const struct rtd_gpio_info	*info;
> +	void __iomem			*base;
> +	void __iomem			*irq_base;

> +	struct gpio_chip		gpio_chip;

Make this to be the first member, it might reduce some code
(due to pointer arithmetics).

> +	unsigned int			irqs[2];
> +	spinlock_t			lock;
> +};
> +
> +

One blank line is enough.

...

> +static const struct rtd_gpio_info rtd_iso_gpio_info = {
> +	.name		= "rtd_iso_gpio",
> +	.type		= RTD_ISO_GPIO,
> +	.gpio_base	= 0,
> +	.num_gpios	= 82,
> +	.dir_offset	= (u8 []){ 0x0, 0x18, 0x2c },
> +	.num_dir	= 3,
> +	.dato_offset	= (u8 []){ 0x4, 0x1c, 0x30 },
> +	.num_dato	= 3,
> +	.dati_offset	= (u8 []){ 0x8, 0x20, 0x34 },
> +	.num_dati	= 3,
> +	.ie_offset	= (u8 []){ 0xc, 0x24, 0x38 },
> +	.num_ie		= 3,
> +	.dp_offset	= (u8 []){ 0x10, 0x28, 0x3c },
> +	.num_dp		= 3,
> +	.gpa_offset	= (u8 []){ 0x8, 0xe0, 0x90 },
> +	.num_gpa	= 3,
> +	.gpda_offset	= (u8 []){ 0xc, 0xe4, 0x94 },
> +	.num_gpda	= 3,
> +	.deb_offset	= (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
> +				   0x60, 0x64, 0x68, 0x6c },
> +	.num_deb	= 11,

Use ARRAY_SIZE() from array_size.h for all num_* assignments.

> +};

...

> +static const struct rtd_gpio_info rtd1619_iso_gpio_info = {

Ditto.

> +};

...

> +static const struct rtd_gpio_info rtd1395_iso_gpio_info = {

Ditto.

> +};
> +
> +static const struct rtd_gpio_info rtd1295_misc_gpio_info = {

Ditto.

> +};
> +
> +static const struct rtd_gpio_info rtd1295_iso_gpio_info = {

Ditto.

> +};

...

> +static int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int offset)
> +{
> +	int index = offset / 32;

> +	if (index > data->info->num_dir)
> +		return -EINVAL;

When this conditional can be true?
Same Q to the similar checks over the code.

> +	return data->info->dir_offset[index];
> +}

...

> +	if (data->info->type == RTD1295_ISO_GPIO) {
> +		reg_offset = rtd_gpio_deb_offset(data, 0);
> +		if (reg_offset < 0)
> +			return reg_offset;
> +		shift = 0;
> +		deb_val += 1;
> +		write_en = BIT(shift + 3);
> +	} else if (data->info->type == RTD1295_MISC_GPIO) {
> +		reg_offset = rtd_gpio_deb_offset(data, 0);
> +		if (reg_offset < 0)
> +			return reg_offset;
> +		shift = (offset >> 4) * 4;
> +		deb_val += 1;
> +		write_en = BIT(shift + 3);
> +	} else {
> +		reg_offset = rtd_gpio_deb_offset(data, offset);
> +		if (reg_offset < 0)
> +			return reg_offset;
> +		shift = (offset % 8) * 4;
> +		write_en = BIT(shift + 3);
> +	}

You should probably have kind of chip_info constant structure that goes via
driver_data and will have a callback, so, here you will call one and get all
three at once:
 - register offset;
 - shift
 - updated debounce value

...

> +static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct rtd_gpio *data = gpiochip_get_data(chip);
> +	unsigned long flags;
> +	int reg_offset;
> +	u32 val;
> +
> +	reg_offset = rtd_gpio_dir_offset(data, offset);
> +	if (reg_offset < 0)
> +		return reg_offset;

> +	spin_lock_irqsave(&data->lock, flags);

So, is your IRQ chip going to work with CONFIG_PREEMT_RT?

> +	val = readl_relaxed(data->base + reg_offset);

> +	val &= BIT(offset % 32);

Why this is is under lock?

> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}

...

> +static int rtd_gpio_set_direction(struct gpio_chip *chip, unsigned int offset, bool out)
> +{

> +	unsigned long flags;


> +	spin_lock_irqsave(&data->lock, flags);


> +	spin_unlock_irqrestore(&data->lock, flags);

Consider to utilise guard() / scoped_guard() from cleanup.h.

> +}

...

> +static int rtd_gpio_direction_output(struct gpio_chip *chip, unsigned int offset, int value)
> +{

> +	chip->set(chip, offset, value);

Why? Can't you call the function by its name directly?

> +
> +	return rtd_gpio_set_direction(chip, offset, true);
> +}

...

> +static int rtd_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct rtd_gpio *data = gpiochip_get_data(chip);
> +	int dir_reg_offset, dat_reg_offset;
> +	unsigned long flags;
> +	u32 val;
> +
> +	dir_reg_offset = rtd_gpio_dir_offset(data, offset);
> +	if (dir_reg_offset < 0)
> +		return dir_reg_offset;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> +	val = readl_relaxed(data->base + dir_reg_offset);
> +	val &= BIT(offset % 32);

> +	dat_reg_offset = val ?
> +			 rtd_gpio_dato_offset(data, offset) : rtd_gpio_dati_offset(data, offset);

Can't you have the direction be cached and already know which offset to use
even before the lock?

> +	val = readl_relaxed(data->base + dat_reg_offset);

> +	val >>= offset % 32;
> +	val &= 0x1;

Why were these operations done under the lock?

> +	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return val;
> +}

...

> +static void rtd_gpio_irq_handle(struct irq_desc *desc)
> +{
> +	int (*get_reg_offset)(struct rtd_gpio *gpio, unsigned int offset);
> +	struct rtd_gpio *data = irq_desc_get_handler_data(desc);
> +	struct irq_domain *domain = data->gpio_chip.irq.domain;
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	int reg_offset;
> +	u32 status;

> +	int hwirq;

Why signed?

> +	int i;
> +	int j;
> +
> +	chained_irq_enter(chip, desc);

> +	if (irq == data->irqs[0])
> +		get_reg_offset = &rtd_gpio_gpa_offset;
> +	else if (irq == data->irqs[1])
> +		get_reg_offset = &rtd_gpio_gpda_offset;

Can't it be done before entering into chained IRQ handler?

> +	for (i = 0; i < data->info->num_gpios; i = i + 31) {

31 ?!  In any case i += 31 is simply shorter.

> +		reg_offset = get_reg_offset(data, i);
> +		if (reg_offset < 0)
> +			return;
> +
> +		status = readl_relaxed(data->irq_base + reg_offset) >> 1;
> +		writel_relaxed(status << 1, data->irq_base + reg_offset);

> +		while (status) {
> +			j = __ffs(status);
> +			status &= ~BIT(j);

NIH for_each_set_bit()

> +			hwirq = i + j;
> +			if (rtd_gpio_check_ie(data, hwirq)) {
> +				int girq = irq_find_mapping(domain, hwirq);
> +				u32 irq_type = irq_get_trigger_type(girq);
> +
> +				if ((irq == data->irqs[1]) && ((irq_type & IRQ_TYPE_SENSE_MASK) !=
> +					IRQ_TYPE_EDGE_BOTH))
> +					break;

> +				generic_handle_irq(girq);

Why you can't use generic_handle_domain_irq()?

> +			}
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}

...

> +	u32 mask = BIT(d->hwirq % 32);

Use proper type and getter for hwirq. It's mentioned in the Documentation.

...

> +static const struct irq_chip rtd_gpio_irq_chip = {
> +	.name = "rtd-gpio",
> +	.irq_enable = rtd_gpio_enable_irq,
> +	.irq_disable = rtd_gpio_disable_irq,
> +	.irq_set_type = rtd_gpio_irq_set_type,

> +	.flags = IRQCHIP_IMMUTABLE,

Is it? You seems missed something to fulfill the immutability requirements.
Please consult with the Documentation, it's all written there.

> +};

...

> +static const struct of_device_id rtd_gpio_of_matches[] = {
> +	{ .compatible = "realtek,rtd1295-misc-gpio", .data = &rtd1295_misc_gpio_info },
> +	{ .compatible = "realtek,rtd1295-iso-gpio", .data = &rtd1295_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1395-iso-gpio", .data = &rtd1395_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1619-iso-gpio", .data = &rtd1619_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1319-iso-gpio", .data = &rtd_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1619b-iso-gpio", .data = &rtd_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1319d-iso-gpio", .data = &rtd_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1315e-iso-gpio", .data = &rtd_iso_gpio_info },

> +	{ },

No comma in the terminator entry.

> +};
> +MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);

Move all these closer to its user (struct platform_device below).

...

> +	data->gpio_chip.label = dev_name(&pdev->dev);

dev

...

> +	data->gpio_chip.fwnode = dev_fwnode(&pdev->dev);

dev

But why setting parent device is not suffice?

...

> +static int rtd_gpio_init(void)
> +{
> +	return platform_driver_register(&rtd_gpio_platform_driver);
> +}

> +

Unneeded blank line.

> +subsys_initcall(rtd_gpio_init);

Why? Anything that is not on standard initcall must be justified.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[TY_Chang[張子逸]]

Hi Andy,

Thank you for the review.

>On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
>> From: Tzuyi Chang <tychang@realtek.com>
>>
>> This driver enables configuration of GPIO direction, GPIO values, GPIO
>> debounce settings and handles GPIO interrupts.
>
>Why gpio-regmap can't be used?
>

I will try to use gpio-remap in the next version.

>...
>
>> +struct rtd_gpio_info {
>
>> +     u8                      *dir_offset;
>> +     u8                      num_dir;
>> +     u8                      *dato_offset;
>> +     u8                      num_dato;
>> +     u8                      *dati_offset;
>> +     u8                      num_dati;
>> +     u8                      *ie_offset;
>> +     u8                      num_ie;
>> +     u8                      *dp_offset;
>> +     u8                      num_dp;
>> +     u8                      *gpa_offset;
>> +     u8                      num_gpa;
>> +     u8                      *gpda_offset;
>> +     u8                      num_gpda;
>> +     u8                      *deb_offset;
>> +     u8                      num_deb;
>
>A lot of wasted space. Can you group pointers followed by u8 members?
>Note, use `pahole` tool to check the struct layout in C.
>

I will revise it in the next version.

>> +};
>
>...
>
>> +struct rtd_gpio {
>> +     struct platform_device          *pdev;
>
>Why
>
>        struct device *dev;
>
>is not suffice?
>

I will remove the pdev.

>> +     const struct rtd_gpio_info      *info;
>> +     void __iomem                    *base;
>> +     void __iomem                    *irq_base;
>
>> +     struct gpio_chip                gpio_chip;
>
>Make this to be the first member, it might reduce some code (due to pointer
>arithmetics).
>

I will move the gpio_chip to the first member.

>> +     unsigned int                    irqs[2];
>> +     spinlock_t                      lock;
>> +};
>> +
>> +
>
>One blank line is enough.
>

I will remove it.

>...
>
>> +static const struct rtd_gpio_info rtd_iso_gpio_info = {
>> +     .name           = "rtd_iso_gpio",
>> +     .type           = RTD_ISO_GPIO,
>> +     .gpio_base      = 0,
>> +     .num_gpios      = 82,
>> +     .dir_offset     = (u8 []){ 0x0, 0x18, 0x2c },
>> +     .num_dir        = 3,
>> +     .dato_offset    = (u8 []){ 0x4, 0x1c, 0x30 },
>> +     .num_dato       = 3,
>> +     .dati_offset    = (u8 []){ 0x8, 0x20, 0x34 },
>> +     .num_dati       = 3,
>> +     .ie_offset      = (u8 []){ 0xc, 0x24, 0x38 },
>> +     .num_ie         = 3,
>> +     .dp_offset      = (u8 []){ 0x10, 0x28, 0x3c },
>> +     .num_dp         = 3,
>> +     .gpa_offset     = (u8 []){ 0x8, 0xe0, 0x90 },
>> +     .num_gpa        = 3,
>> +     .gpda_offset    = (u8 []){ 0xc, 0xe4, 0x94 },
>> +     .num_gpda       = 3,
>> +     .deb_offset     = (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
>> +                                0x60, 0x64, 0x68, 0x6c },
>> +     .num_deb        = 11,
>
>Use ARRAY_SIZE() from array_size.h for all num_* assignments.
>

I will revise it.

>> +};
>
>...
>
>> +static const struct rtd_gpio_info rtd1619_iso_gpio_info = {
>
>Ditto.
>
>> +};
>
>...
>
>> +static const struct rtd_gpio_info rtd1395_iso_gpio_info = {
>
>Ditto.
>
>> +};
>> +
>> +static const struct rtd_gpio_info rtd1295_misc_gpio_info = {
>
>Ditto.
>
>> +};
>> +
>> +static const struct rtd_gpio_info rtd1295_iso_gpio_info = {
>
>Ditto.
>
>> +};
>
>...
>
>> +static int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int
>> +offset) {
>> +     int index = offset / 32;
>
>> +     if (index > data->info->num_dir)
>> +             return -EINVAL;
>
>When this conditional can be true?
>Same Q to the similar checks over the code.
>

It is only to check if the offset value is missing in the rtd_gpio_info. I'm uncertain
about the necessity of these checks. If they are not necessary, I will remove
the num_* members in the rtd_gpio_info structure along with these checks. 

>> +     return data->info->dir_offset[index]; }
>
>...
>
>> +     if (data->info->type == RTD1295_ISO_GPIO) {
>> +             reg_offset = rtd_gpio_deb_offset(data, 0);
>> +             if (reg_offset < 0)
>> +                     return reg_offset;
>> +             shift = 0;
>> +             deb_val += 1;
>> +             write_en = BIT(shift + 3);
>> +     } else if (data->info->type == RTD1295_MISC_GPIO) {
>> +             reg_offset = rtd_gpio_deb_offset(data, 0);
>> +             if (reg_offset < 0)
>> +                     return reg_offset;
>> +             shift = (offset >> 4) * 4;
>> +             deb_val += 1;
>> +             write_en = BIT(shift + 3);
>> +     } else {
>> +             reg_offset = rtd_gpio_deb_offset(data, offset);
>> +             if (reg_offset < 0)
>> +                     return reg_offset;
>> +             shift = (offset % 8) * 4;
>> +             write_en = BIT(shift + 3);
>> +     }
>
>You should probably have kind of chip_info constant structure that goes via
>driver_data and will have a callback, so, here you will call one and get all three
>at once:
> - register offset;
> - shift
> - updated debounce value
>

I will add a callback in the struct rtd_gpio_info to get these values.

>...
>
>> +static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned
>> +int offset) {
>> +     struct rtd_gpio *data = gpiochip_get_data(chip);
>> +     unsigned long flags;
>> +     int reg_offset;
>> +     u32 val;
>> +
>> +     reg_offset = rtd_gpio_dir_offset(data, offset);
>> +     if (reg_offset < 0)
>> +             return reg_offset;
>
>> +     spin_lock_irqsave(&data->lock, flags);
>
>So, is your IRQ chip going to work with CONFIG_PREEMT_RT?
>

No, we do not enable CONFIG_PREEMT_RT. However, a custom driver might change
the GPIO status in the ISR. I will utilize raw_spinlock instead and only lock
the write operations.

>> +     val = readl_relaxed(data->base + reg_offset);
>
>> +     val &= BIT(offset % 32);
>
>Why this is is under lock?
>

I'll move it outside of the lock.

>> +     spin_unlock_irqrestore(&data->lock, flags);
>> +
>> +     return val ? GPIO_LINE_DIRECTION_OUT :
>GPIO_LINE_DIRECTION_IN; }
>
>...
>
>> +static int rtd_gpio_set_direction(struct gpio_chip *chip, unsigned
>> +int offset, bool out) {
>
>> +     unsigned long flags;
>
>
>> +     spin_lock_irqsave(&data->lock, flags);
>
>
>> +     spin_unlock_irqrestore(&data->lock, flags);
>
>Consider to utilise guard() / scoped_guard() from cleanup.h.
>

I will try to use these macros.

>> +}
>
>...
>
>> +static int rtd_gpio_direction_output(struct gpio_chip *chip, unsigned
>> +int offset, int value) {
>
>> +     chip->set(chip, offset, value);
>
>Why? Can't you call the function by its name directly?
>

I will revise it.

>> +
>> +     return rtd_gpio_set_direction(chip, offset, true); }
>
>...
>
>> +static int rtd_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +     struct rtd_gpio *data = gpiochip_get_data(chip);
>> +     int dir_reg_offset, dat_reg_offset;
>> +     unsigned long flags;
>> +     u32 val;
>> +
>> +     dir_reg_offset = rtd_gpio_dir_offset(data, offset);
>> +     if (dir_reg_offset < 0)
>> +             return dir_reg_offset;
>> +
>> +     spin_lock_irqsave(&data->lock, flags);
>> +
>> +     val = readl_relaxed(data->base + dir_reg_offset);
>> +     val &= BIT(offset % 32);
>
>> +     dat_reg_offset = val ?
>> +                      rtd_gpio_dato_offset(data, offset) :
>> + rtd_gpio_dati_offset(data, offset);
>
>Can't you have the direction be cached and already know which offset to use
>even before the lock?
>

I will move these codes outside of the lock.

>> +     val = readl_relaxed(data->base + dat_reg_offset);
>
>> +     val >>= offset % 32;
>> +     val &= 0x1;
>
>Why were these operations done under the lock?
>

I will move these codes outside of the lock.

>> +     spin_unlock_irqrestore(&data->lock, flags);
>> +
>> +     return val;
>> +}
>
>...
>
>> +static void rtd_gpio_irq_handle(struct irq_desc *desc) {
>> +     int (*get_reg_offset)(struct rtd_gpio *gpio, unsigned int offset);
>> +     struct rtd_gpio *data = irq_desc_get_handler_data(desc);
>> +     struct irq_domain *domain = data->gpio_chip.irq.domain;
>> +     struct irq_chip *chip = irq_desc_get_chip(desc);
>> +     unsigned int irq = irq_desc_get_irq(desc);
>> +     int reg_offset;
>> +     u32 status;
>
>> +     int hwirq;
>
>Why signed?
>

I will revised it to unsigned int.

>> +     int i;
>> +     int j;
>> +
>> +     chained_irq_enter(chip, desc);
>
>> +     if (irq == data->irqs[0])
>> +             get_reg_offset = &rtd_gpio_gpa_offset;
>> +     else if (irq == data->irqs[1])
>> +             get_reg_offset = &rtd_gpio_gpda_offset;
>
>Can't it be done before entering into chained IRQ handler?
>

I will revise it.

>> +     for (i = 0; i < data->info->num_gpios; i = i + 31) {
>
>31 ?!  In any case i += 31 is simply shorter.
>

I will revise it.

>> +             reg_offset = get_reg_offset(data, i);
>> +             if (reg_offset < 0)
>> +                     return;
>> +
>> +             status = readl_relaxed(data->irq_base + reg_offset) >> 1;
>> +             writel_relaxed(status << 1, data->irq_base +
>> + reg_offset);
>
>> +             while (status) {
>> +                     j = __ffs(status);
>> +                     status &= ~BIT(j);
>
>NIH for_each_set_bit()
>

I will revise it.

>> +                     hwirq = i + j;
>> +                     if (rtd_gpio_check_ie(data, hwirq)) {
>> +                             int girq = irq_find_mapping(domain,
>hwirq);
>> +                             u32 irq_type =
>> + irq_get_trigger_type(girq);
>> +
>> +                             if ((irq == data->irqs[1]) && ((irq_type &
>IRQ_TYPE_SENSE_MASK) !=
>> +                                     IRQ_TYPE_EDGE_BOTH))
>> +                                     break;
>
>> +                             generic_handle_irq(girq);
>
>Why you can't use generic_handle_domain_irq()?
>

I will revise it.

>> +                     }
>> +             }
>> +     }
>> +
>> +     chained_irq_exit(chip, desc);
>> +}
>
>...
>
>> +     u32 mask = BIT(d->hwirq % 32);
>
>Use proper type and getter for hwirq. It's mentioned in the Documentation.
>

I will use irqd_to_hwirq(d) to get hwirq.

>...
>
>> +static const struct irq_chip rtd_gpio_irq_chip = {
>> +     .name = "rtd-gpio",
>> +     .irq_enable = rtd_gpio_enable_irq,
>> +     .irq_disable = rtd_gpio_disable_irq,
>> +     .irq_set_type = rtd_gpio_irq_set_type,
>
>> +     .flags = IRQCHIP_IMMUTABLE,
>
>Is it? You seems missed something to fulfill the immutability requirements.
>Please consult with the Documentation, it's all written there.
>

I think I missed to call the gpiochip_disable_irq/gpiochip_enable_irq in the
.irq_disable/.irq_enable callback function to inform the gpiolib.
I will revise it.

>> +};
>
>...
>
>> +static const struct of_device_id rtd_gpio_of_matches[] = {
>> +     { .compatible = "realtek,rtd1295-misc-gpio", .data =
>&rtd1295_misc_gpio_info },
>> +     { .compatible = "realtek,rtd1295-iso-gpio", .data =
>&rtd1295_iso_gpio_info },
>> +     { .compatible = "realtek,rtd1395-iso-gpio", .data =
>&rtd1395_iso_gpio_info },
>> +     { .compatible = "realtek,rtd1619-iso-gpio", .data =
>&rtd1619_iso_gpio_info },
>> +     { .compatible = "realtek,rtd1319-iso-gpio", .data =
>&rtd_iso_gpio_info },
>> +     { .compatible = "realtek,rtd1619b-iso-gpio", .data =
>&rtd_iso_gpio_info },
>> +     { .compatible = "realtek,rtd1319d-iso-gpio", .data =
>&rtd_iso_gpio_info },
>> +     { .compatible = "realtek,rtd1315e-iso-gpio", .data =
>> +&rtd_iso_gpio_info },
>
>> +     { },
>
>No comma in the terminator entry.
>

I will revise it.

>> +};
>> +MODULE_DEVICE_TABLE(of, rtd_gpio_of_matches);
>
>Move all these closer to its user (struct platform_device below).
>

I will revise it.

>...
>
>> +     data->gpio_chip.label = dev_name(&pdev->dev);
>
>dev
>
>...
>
>> +     data->gpio_chip.fwnode = dev_fwnode(&pdev->dev);
>
>dev
>
>But why setting parent device is not suffice?
>

I will revise it to data->gpio_chip.parent = dev;.

>...
>
>> +static int rtd_gpio_init(void)
>> +{
>> +     return platform_driver_register(&rtd_gpio_platform_driver);
>> +}
>
>> +
>
>Unneeded blank line.
>

I will remove it.

>> +subsys_initcall(rtd_gpio_init);
>
>Why? Anything that is not on standard initcall must be justified.
>

I will revise it to module_init.

>--
>With Best Regards,
>Andy Shevchenko


Thanks,
Tzuyi Chang

Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Andy Shevchenko]

On Tue, Dec 12, 2023 at 09:55:59AM +0000, TY_Chang[張子逸] wrote:
> >On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:

...

> >> This driver enables configuration of GPIO direction, GPIO values, GPIO
> >> debounce settings and handles GPIO interrupts.
> >
> >Why gpio-regmap can't be used?
> 
> I will try to use gpio-remap in the next version.

If it appears that it makes code uglier / complicated, please add the note
somewhere to answer the above question.

...

> >> +     if (index > data->info->num_dir)
> >> +             return -EINVAL;
> >
> >When this conditional can be true?
> >Same Q to the similar checks over the code.
> 
> It is only to check if the offset value is missing in the rtd_gpio_info.
> I'm uncertain about the necessity of these checks. If they are not necessary,
> I will remove the num_* members in the rtd_gpio_info structure along with
> these checks.

My understanding that these checks are equivalent to the

	if (offset >= ngpio)

one, which is performed by GPIO library, i.o.w. you will never get an offset
outside the range of supported GPIO lines.

If my understanding is wrong, these checks need a comment why.

...

> >> +     if (irq == data->irqs[0])
> >> +             get_reg_offset = &rtd_gpio_gpa_offset;
> >> +     else if (irq == data->irqs[1])
> >> +             get_reg_offset = &rtd_gpio_gpda_offset;
> >
> >Can't it be done before entering into chained IRQ handler?
> 
> I will revise it.

Thinking about this more, perhaps you can register two IRQ chips with
different functions, so this won't be part of the very critical interrupt
handler (as we all want to reduce overhead in it as much as possible).
Anyway, think about this and try different options, choose the one you
think the best.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[TY_Chang[張子逸]]

Hi Andy,

Thank you for the suggestions.

>On Tue, Dec 12, 2023 at 09:55:59AM +0000, TY_Chang[張子逸] wrote:
>> >On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:
>
>...
>
>> >> This driver enables configuration of GPIO direction, GPIO values,
>> >> GPIO debounce settings and handles GPIO interrupts.
>> >
>> >Why gpio-regmap can't be used?
>>
>> I will try to use gpio-remap in the next version.
>
>If it appears that it makes code uglier / complicated, please add the note
>somewhere to answer the above question.
>

I've traced the gpio-regmap.c file. It appears that for the driver to register
gpio_irq_chip, it must create the irq_domain and add it into gpio_regmap_config.
Additionally, the driver needs to register the irq handler by itself.
However, this process can be managed by the gpiolib if the driver fills in the struct
gpio_irq_chip inside struct gpio_chip before invoking gpiochip_add_data.

Moreover, apart from managing the registers for gpio direction and value, there
are several other registers that require access(interrupt enable, debounce...).
The GPIO IRQ status registers are located at different base addresses and are
not contiguous. It may need to create an additional regmap and assign the access
table to this regmap.

With the above consideration, I tend to keep using the existing method.

>...
>
>> >> +     if (index > data->info->num_dir)
>> >> +             return -EINVAL;
>> >
>> >When this conditional can be true?
>> >Same Q to the similar checks over the code.
>>
>> It is only to check if the offset value is missing in the rtd_gpio_info.
>> I'm uncertain about the necessity of these checks. If they are not
>> necessary, I will remove the num_* members in the rtd_gpio_info
>> structure along with these checks.
>
>My understanding that these checks are equivalent to the
>
>        if (offset >= ngpio)
>
>one, which is performed by GPIO library, i.o.w. you will never get an offset
>outside the range of supported GPIO lines.
>
>If my understanding is wrong, these checks need a comment why.
>

I agree with you. I will remove these checks.

>...
>
>> >> +     if (irq == data->irqs[0])
>> >> +             get_reg_offset = &rtd_gpio_gpa_offset;
>> >> +     else if (irq == data->irqs[1])
>> >> +             get_reg_offset = &rtd_gpio_gpda_offset;
>> >
>> >Can't it be done before entering into chained IRQ handler?
>>
>> I will revise it.
>
>Thinking about this more, perhaps you can register two IRQ chips with different
>functions, so this won't be part of the very critical interrupt handler (as we all
>want to reduce overhead in it as much as possible).
>Anyway, think about this and try different options, choose the one you think the
>best.
>

In the previous patch (v1), I had registered two IRQ chips with different handlers. 
However, these two handlers appeared quite similar and the gpio_irq_chip only allows
the registration of a single handler. Therefore, I ended up registering one handler
for both IRQs and included conditional checks within the handler to differentiate
between the two.

>--
>With Best Regards,
>Andy Shevchenko

Thanks,
Tzuyi Chang

Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Andy Shevchenko]

+Cc: Michael (GPIO regmap maintainer)

On Thu, Dec 14, 2023 at 07:57:55AM +0000, TY_Chang[張子逸] wrote:
> >On Tue, Dec 12, 2023 at 09:55:59AM +0000, TY_Chang[張子逸] wrote:
> >> >On Thu, Dec 07, 2023 at 06:07:23PM +0800, TY Chang wrote:

...

> >> >> This driver enables configuration of GPIO direction, GPIO values,
> >> >> GPIO debounce settings and handles GPIO interrupts.
> >> >
> >> >Why gpio-regmap can't be used?
> >>
> >> I will try to use gpio-remap in the next version.
> >
> >If it appears that it makes code uglier / complicated, please add the note
> >somewhere to answer the above question.
> 
> I've traced the gpio-regmap.c file. It appears that for the driver to register
> gpio_irq_chip, it must create the irq_domain and add it into gpio_regmap_config.
> Additionally, the driver needs to register the irq handler by itself.
> However, this process can be managed by the gpiolib if the driver fills in the struct
> gpio_irq_chip inside struct gpio_chip before invoking gpiochip_add_data.

Hmm... I thought this is solvable issue.
Michael, is there a limitation in GPIO regmap that this driver can't be converted?

> Moreover, apart from managing the registers for gpio direction and value, there
> are several other registers that require access(interrupt enable, debounce...).
> The GPIO IRQ status registers are located at different base addresses and are
> not contiguous. It may need to create an additional regmap and assign the access
> table to this regmap.

AFAIK this is not a problem as you can provide your own xlate function that
will take care about register mapping.

> With the above consideration, I tend to keep using the existing method.

I would like to hear from Michael if it's indeed a big obstacle.

...

> >> >> +     if (irq == data->irqs[0])
> >> >> +             get_reg_offset = &rtd_gpio_gpa_offset;
> >> >> +     else if (irq == data->irqs[1])
> >> >> +             get_reg_offset = &rtd_gpio_gpda_offset;
> >> >
> >> >Can't it be done before entering into chained IRQ handler?
> >>
> >> I will revise it.
> >
> >Thinking about this more, perhaps you can register two IRQ chips with
> >different functions, so this won't be part of the very critical interrupt
> >handler (as we all want to reduce overhead in it as much as possible).
> >Anyway, think about this and try different options, choose the one you think
> >the best.
> 
> In the previous patch (v1), I had registered two IRQ chips with different
> handlers. However, these two handlers appeared quite similar and the
> gpio_irq_chip only allows the registration of a single handler. Therefore,
> I ended up registering one handler for both IRQs and included conditional
> checks within the handler to differentiate between the two.

What is the performance impact that you have that condition in the interrupt
handler?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Michael Walle]

Hi,

>> >> >> This driver enables configuration of GPIO direction, GPIO values,
>> >> >> GPIO debounce settings and handles GPIO interrupts.
>> >> >
>> >> >Why gpio-regmap can't be used?
>> >>
>> >> I will try to use gpio-remap in the next version.
>> >
>> >If it appears that it makes code uglier / complicated, please add the note
>> >somewhere to answer the above question.
>> 
>> I've traced the gpio-regmap.c file. It appears that for the driver to 
>> register
>> gpio_irq_chip, it must create the irq_domain and add it into 
>> gpio_regmap_config.
>> Additionally, the driver needs to register the irq handler by itself.
>> However, this process can be managed by the gpiolib if the driver 
>> fills in the struct
>> gpio_irq_chip inside struct gpio_chip before invoking 
>> gpiochip_add_data.
> 
> Hmm... I thought this is solvable issue.
> Michael, is there a limitation in GPIO regmap that this driver can't be 
> converted?

gpio-regmap is designed that regmap-irq (drivers/base/regmap/irq.c) can 
be
used. So, if regmap-irq fit this driver, then it can be used together 
with
gpio-regmap.

 From a quick glance at the patch, it looks like the gpio portion might 
fit
gpio-regmap.

>> Moreover, apart from managing the registers for gpio direction and 
>> value, there
>> are several other registers that require access(interrupt enable, 
>> debounce...).
>> The GPIO IRQ status registers are located at different base addresses 
>> and are
>> not contiguous. It may need to create an additional regmap and assign 
>> the access
>> table to this regmap.
> 
> AFAIK this is not a problem as you can provide your own xlate function 
> that
> will take care about register mapping.

Just for the gpio part. IIRC regmap has it own translation (regmap 
fields).

>> With the above consideration, I tend to keep using the existing 
>> method.
> 
> I would like to hear from Michael if it's indeed a big obstacle.

So, regarding the irq portion, again, it must fit the regmap-irq. For 
the
additional requirement to set the debounce, you can add a .set_config to
gpio_regmap_config and supply your own set_config callback. See also 
[1].

-michael

[1] 
https://lore.kernel.org/linux-gpio/d4a6a640c373b6d939e147691efa596c@walle.cc/

Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Andy Shevchenko]

On Thu, Dec 14, 2023 at 03:35:18PM +0100, Michael Walle wrote:

> > > >> >> This driver enables configuration of GPIO direction, GPIO values,
> > > >> >> GPIO debounce settings and handles GPIO interrupts.
> > > >> >
> > > >> >Why gpio-regmap can't be used?
> > > >>
> > > >> I will try to use gpio-remap in the next version.
> > > >
> > > >If it appears that it makes code uglier / complicated, please add the note
> > > >somewhere to answer the above question.
> > > 
> > > I've traced the gpio-regmap.c file. It appears that for the driver
> > > to register
> > > gpio_irq_chip, it must create the irq_domain and add it into
> > > gpio_regmap_config.
> > > Additionally, the driver needs to register the irq handler by itself.
> > > However, this process can be managed by the gpiolib if the driver
> > > fills in the struct
> > > gpio_irq_chip inside struct gpio_chip before invoking
> > > gpiochip_add_data.
> > 
> > Hmm... I thought this is solvable issue.
> > Michael, is there a limitation in GPIO regmap that this driver can't be
> > converted?
> 
> gpio-regmap is designed that regmap-irq (drivers/base/regmap/irq.c) can be
> used. So, if regmap-irq fit this driver, then it can be used together with
> gpio-regmap.
> 
> From a quick glance at the patch, it looks like the gpio portion might fit
> gpio-regmap.
> 
> > > Moreover, apart from managing the registers for gpio direction and
> > > value, there
> > > are several other registers that require access(interrupt enable,
> > > debounce...).
> > > The GPIO IRQ status registers are located at different base
> > > addresses and are
> > > not contiguous. It may need to create an additional regmap and
> > > assign the access
> > > table to this regmap.
> > 
> > AFAIK this is not a problem as you can provide your own xlate function
> > that
> > will take care about register mapping.
> 
> Just for the gpio part. IIRC regmap has it own translation (regmap fields).
> 
> > > With the above consideration, I tend to keep using the existing
> > > method.
> > 
> > I would like to hear from Michael if it's indeed a big obstacle.
> 
> So, regarding the irq portion, again, it must fit the regmap-irq. For the
> additional requirement to set the debounce, you can add a .set_config to
> gpio_regmap_config and supply your own set_config callback. See also [1].

Thank you, Michael, for the prompt answer. It's insightful to me, I will try to
remember these aspects for the future reviews.

> [1]
> https://lore.kernel.org/linux-gpio/d4a6a640c373b6d939e147691efa596c@walle.cc/

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[TY_Chang[張子逸]]

Hi Andy,

>On Thu, Dec 14, 2023 at 03:35:18PM +0100, Michael Walle wrote:
>
>> > > >> >> This driver enables configuration of GPIO direction, GPIO
>> > > >> >> values, GPIO debounce settings and handles GPIO interrupts.
>> > > >> >
>> > > >> >Why gpio-regmap can't be used?
>> > > >>
>> > > >> I will try to use gpio-remap in the next version.
>> > > >
>> > > >If it appears that it makes code uglier / complicated, please add
>> > > >the note somewhere to answer the above question.
>> > >
>> > > I've traced the gpio-regmap.c file. It appears that for the driver
>> > > to register gpio_irq_chip, it must create the irq_domain and add
>> > > it into gpio_regmap_config.
>> > > Additionally, the driver needs to register the irq handler by itself.
>> > > However, this process can be managed by the gpiolib if the driver
>> > > fills in the struct gpio_irq_chip inside struct gpio_chip before
>> > > invoking gpiochip_add_data.
>> >
>> > Hmm... I thought this is solvable issue.
>> > Michael, is there a limitation in GPIO regmap that this driver can't
>> > be converted?
>>
>> gpio-regmap is designed that regmap-irq (drivers/base/regmap/irq.c)
>> can be used. So, if regmap-irq fit this driver, then it can be used
>> together with gpio-regmap.
>>
>> From a quick glance at the patch, it looks like the gpio portion might
>> fit gpio-regmap.
>>
>> > > Moreover, apart from managing the registers for gpio direction and
>> > > value, there are several other registers that require
>> > > access(interrupt enable, debounce...).
>> > > The GPIO IRQ status registers are located at different base
>> > > addresses and are not contiguous. It may need to create an
>> > > additional regmap and assign the access table to this regmap.
>> >
>> > AFAIK this is not a problem as you can provide your own xlate
>> > function that will take care about register mapping.
>>
>> Just for the gpio part. IIRC regmap has it own translation (regmap fields).
>>
>> > > With the above consideration, I tend to keep using the existing
>> > > method.
>> >
>> > I would like to hear from Michael if it's indeed a big obstacle.
>>
>> So, regarding the irq portion, again, it must fit the regmap-irq. For
>> the additional requirement to set the debounce, you can add a
>> .set_config to gpio_regmap_config and supply your own set_config callback.
>See also [1].
>
>Thank you, Michael, for the prompt answer. It's insightful to me, I will try to
>remember these aspects for the future reviews.
>
>> [1]
>>
>https://lore.kernel.org/linux-gpio/d4a6a640c373b6d939e147691efa596c@wa
>> lle.cc/
>

I have looked into regmap-irq, it appears that using the default irq
thread_fn(regmap_irq_thread) is required. However, due to hardware limitation,
we need to inspect the IRQ_TYPE to determine whether to manage the irq within
the irq handler. I think our irq portion does not perfectly fit the regmap-irq.
Moreover, it seems that the gpio-regmap.c file needs to be modified if the GPIO driver
requires debounce settings. I think the gpio-regmap may not be appropriate for our driver.
Do you have any suggestions?

Thanks,
Tzuyi Chang

Re: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Michael Walle]

Hi,

> I have looked into regmap-irq, it appears that using the default irq
> thread_fn(regmap_irq_thread) is required. However, due to hardware 
> limitation,
> we need to inspect the IRQ_TYPE to determine whether to manage the irq 
> within
> the irq handler. I think our irq portion does not perfectly fit the 
> regmap-irq.
> Moreover, it seems that the gpio-regmap.c file needs to be modified if 
> the GPIO driver
> requires debounce settings. I think the gpio-regmap may not be 
> appropriate for our driver.
> Do you have any suggestions?

Can't say anything regarding the interrupt handling but adding literally
one line to gpio-regmap shouldn't be a reason not to use it.

-michael

RE: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[TY_Chang[張子逸]]

Hi Michael,

>Hi,
>
>> I have looked into regmap-irq, it appears that using the default irq
>> thread_fn(regmap_irq_thread) is required. However, due to hardware
>> limitation, we need to inspect the IRQ_TYPE to determine whether to
>> manage the irq within the irq handler. I think our irq portion does
>> not perfectly fit the regmap-irq.
>> Moreover, it seems that the gpio-regmap.c file needs to be modified if
>> the GPIO driver requires debounce settings. I think the gpio-regmap
>> may not be appropriate for our driver.
>> Do you have any suggestions?
>
>Can't say anything regarding the interrupt handling but adding literally one line
>to gpio-regmap shouldn't be a reason not to use it.
>

I agree that this should not be the reason. Sorry about that.
However, the interrupt handler issue may still be the limitation for our platform.

Thanks,
Tzuyi Chang

RE: [PATCH v3 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[TY_Chang[張子逸]]

Hi Andy,

>> >> >> +     if (irq == data->irqs[0])
>> >> >> +             get_reg_offset = &rtd_gpio_gpa_offset;
>> >> >> +     else if (irq == data->irqs[1])
>> >> >> +             get_reg_offset = &rtd_gpio_gpda_offset;
>> >> >
>> >> >Can't it be done before entering into chained IRQ handler?
>> >>
>> >> I will revise it.
>> >
>> >Thinking about this more, perhaps you can register two IRQ chips with
>> >different functions, so this won't be part of the very critical
>> >interrupt handler (as we all want to reduce overhead in it as much as
>possible).
>> >Anyway, think about this and try different options, choose the one
>> >you think the best.
>>
>> In the previous patch (v1), I had registered two IRQ chips with
>> different handlers. However, these two handlers appeared quite similar
>> and the gpio_irq_chip only allows the registration of a single
>> handler. Therefore, I ended up registering one handler for both IRQs
>> and included conditional checks within the handler to differentiate between
>the two.
>
>What is the performance impact that you have that condition in the interrupt
>handler?
>

I believe the performance impact is minimal since this conditional check is
a simple operation aimed at retrieving the corresponding offset of the
interrupt status registers.
Or is there something I might not have considered?

Thanks,
Tzuyi Chang