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

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

[Tzuyi 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:
v3->v4:
1. Arrange the compatible list in alphanumerical order.
2. Remove the size check for the offset array.
3. Add the debounce callback.
4. Conducted a review of the critical section, employing raw_spinlock_t for locking purposes.
5. Add gpiochip_enable_irq/gpiochip_disable_irq to fulfill the immutability requirements.
6. Use irqd_to_hwirq to get hwirq.
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                       | 616 ++++++++++++++++++
 4 files changed, 695 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd-gpio.yaml
 create mode 100644 drivers/gpio/gpio-rtd.c

-- 
2.43.0

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

[Tzuyi Chang]

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

Signed-off-by: Tzuyi Chang <tychang@realtek.com>
---
v3 to v4 change:
    1. Arrange the compatible list in alphanumerical order.
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..dd768db37a98
--- /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,rtd1315e-iso-gpio
+      - realtek,rtd1319-iso-gpio
+      - realtek,rtd1319d-iso-gpio
+      - realtek,rtd1395-iso-gpio
+      - realtek,rtd1619-iso-gpio
+      - realtek,rtd1619b-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>,
+            <0x0 0xb0>;
+      interrupt-parent = <&iso_irq_mux>;
+      interrupts = <19>, <20>;
+      gpio-ranges = <&pinctrl 0 0 82>;
+      gpio-controller;
+      #gpio-cells = <2>;
+    };
-- 
2.43.0

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

[Tzuyi Chang]

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>
---
v3 to v4 change:
    1. Remove the size check for the offset array.
    2. Add the debounce callback to get values, register offsets and shifts for each chip.
    3. Conducted a review of the critical section, employing raw_spinlock_t for locking purposes.
    4. Use irqd_to_hwirq to get hwirq.
    5. Add gpiochip_enable_irq/gpiochip_disable_irq to fulfill the immutability requirements.
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 | 616 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 626 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..619f88bf0bd5
--- /dev/null
+++ b/drivers/gpio/gpio-rtd.c
@@ -0,0 +1,616 @@
+// 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
+
+/**
+ * struct rtd_gpio_info - Specific GPIO register information
+ * @name: GPIO device name
+ * @gpio_base: GPIO base number
+ * @num_gpios: The number of GPIOs
+ * @dir_offset: Offset for GPIO direction registers
+ * @dato_offset: Offset for GPIO data output registers
+ * @dati_offset: Offset for GPIO data input registers
+ * @ie_offset: Offset for GPIO interrupt enable registers
+ * @dp_offset: Offset for GPIO detection polarity registers
+ * @gpa_offset: Offset for GPIO assert interrupt status registers
+ * @gpda_offset: Offset for GPIO deassert interrupt status registers
+ * @deb_offset: Offset for GPIO debounce registers
+ * @deb_val: Register values representing the GPIO debounce time
+ * @get_deb_setval: Used to get the corresponding value for setting the debounce register
+ */
+struct rtd_gpio_info {
+	const char		*name;
+	unsigned int		gpio_base;
+	unsigned int		num_gpios;
+	u8			*dir_offset;
+	u8			*dato_offset;
+	u8			*dati_offset;
+	u8			*ie_offset;
+	u8			*dp_offset;
+	u8			*gpa_offset;
+	u8			*gpda_offset;
+	u8			*deb_offset;
+	u8			*deb_val;
+	u8			(*get_deb_setval)(const struct rtd_gpio_info *info,
+						   unsigned int offset, u8 *reg_offset,
+						   u8 *shift, u8 deb_index);
+};
+
+struct rtd_gpio {
+	struct gpio_chip		gpio_chip;
+	const struct rtd_gpio_info	*info;
+	void __iomem			*base;
+	void __iomem			*irq_base;
+	unsigned int			irqs[2];
+	raw_spinlock_t			lock;
+};
+
+static u8 rtd_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned int offset,
+				u8 *reg_offset, u8 *shift, u8 deb_val)
+{
+	*reg_offset = info->deb_offset[offset / 8];
+	*shift = (offset % 8) * 4;
+	return info->deb_val[deb_val];
+}
+
+static u8 rtd1295_misc_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned int offset,
+					 u8 *reg_offset, u8 *shift, u8 deb_index)
+{
+	*reg_offset = info->deb_offset[0];
+	*shift = (offset % 8) * 4;
+	return info->deb_val[deb_index];
+}
+
+static u8 rtd1295_iso_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned int offset,
+					u8 *reg_offset, u8 *shift, u8 deb_index)
+{
+	*reg_offset = info->deb_offset[0];
+	*shift = 0;
+	return info->deb_val[deb_index];
+}
+
+static const struct rtd_gpio_info rtd_iso_gpio_info = {
+	.name			= "rtd_iso_gpio",
+	.gpio_base		= 0,
+	.num_gpios		= 82,
+	.dir_offset		= (u8 []){ 0x0, 0x18, 0x2c },
+	.dato_offset		= (u8 []){ 0x4, 0x1c, 0x30 },
+	.dati_offset		= (u8 []){ 0x8, 0x20, 0x34 },
+	.ie_offset		= (u8 []){ 0xc, 0x24, 0x38 },
+	.dp_offset		= (u8 []){ 0x10, 0x28, 0x3c },
+	.gpa_offset		= (u8 []){ 0x8, 0xe0, 0x90 },
+	.gpda_offset		= (u8 []){ 0xc, 0xe4, 0x94 },
+	.deb_offset		= (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
+					   0x60, 0x64, 0x68, 0x6c },
+	.deb_val		= (u8 []){ 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6 },
+	.get_deb_setval		= rtd_gpio_get_deb_setval,
+};
+
+static const struct rtd_gpio_info rtd1619_iso_gpio_info = {
+	.name			= "rtd1619_iso_gpio",
+	.gpio_base		= 0,
+	.num_gpios		= 86,
+	.dir_offset		= (u8 []){ 0x0, 0x18, 0x2c },
+	.dato_offset		= (u8 []){ 0x4, 0x1c, 0x30 },
+	.dati_offset		= (u8 []){ 0x8, 0x20, 0x34 },
+	.ie_offset		= (u8 []){ 0xc, 0x24, 0x38 },
+	.dp_offset		= (u8 []){ 0x10, 0x28, 0x3c },
+	.gpa_offset		= (u8 []){ 0x8, 0xe0, 0x90 },
+	.gpda_offset		= (u8 []){ 0xc, 0xe4, 0x94 },
+	.deb_offset		= (u8 []){ 0x44, 0x48, 0x4c, 0x50, 0x54, 0x58, 0x5c,
+					   0x60, 0x64, 0x68, 0x6c },
+	.deb_val		= (u8 []){ 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6 },
+	.get_deb_setval		= rtd_gpio_get_deb_setval,
+};
+
+static const struct rtd_gpio_info rtd1395_iso_gpio_info = {
+	.name			= "rtd1395_iso_gpio",
+	.gpio_base		= 0,
+	.num_gpios		= 57,
+	.dir_offset		= (u8 []){ 0x0, 0x18 },
+	.dato_offset		= (u8 []){ 0x4, 0x1c },
+	.dati_offset		= (u8 []){ 0x8, 0x20 },
+	.ie_offset		= (u8 []){ 0xc, 0x24 },
+	.dp_offset		= (u8 []){ 0x10, 0x28 },
+	.gpa_offset		= (u8 []){ 0x8, 0xe0 },
+	.gpda_offset		= (u8 []){ 0xc, 0xe4 },
+	.deb_offset		= (u8 []){ 0x30, 0x34, 0x38, 0x3c, 0x40, 0x44, 0x48, 0x4c },
+	.deb_val		= (u8 []){ 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6 },
+	.get_deb_setval		= rtd_gpio_get_deb_setval,
+};
+
+static const struct rtd_gpio_info rtd1295_misc_gpio_info = {
+	.name			= "rtd1295_misc_gpio",
+	.gpio_base		= 0,
+	.num_gpios		= 101,
+	.dir_offset		= (u8 []){ 0x0, 0x4, 0x8, 0xc },
+	.dato_offset		= (u8 []){ 0x10, 0x14, 0x18, 0x1c },
+	.dati_offset		= (u8 []){ 0x20, 0x24, 0x28, 0x2c },
+	.ie_offset		= (u8 []){ 0x30, 0x34, 0x38, 0x3c },
+	.dp_offset		= (u8 []){ 0x40, 0x44, 0x48, 0x4c },
+	.gpa_offset		= (u8 []){ 0x40, 0x44, 0xa4, 0xb8 },
+	.gpda_offset		= (u8 []){ 0x54, 0x58, 0xa8, 0xbc},
+	.deb_offset		= (u8 []){ 0x50 },
+	.deb_val		= (u8 []){ 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7 },
+	.get_deb_setval		= rtd1295_misc_gpio_get_deb_setval,
+};
+
+static const struct rtd_gpio_info rtd1295_iso_gpio_info = {
+	.name			= "rtd1295_iso_gpio",
+	.gpio_base		= 101,
+	.num_gpios		= 35,
+	.dir_offset		= (u8 []){ 0x0, 0x18 },
+	.dato_offset		= (u8 []){ 0x4, 0x1c },
+	.dati_offset		= (u8 []){ 0x8, 0x20 },
+	.ie_offset		= (u8 []){ 0xc, 0x24 },
+	.dp_offset		= (u8 []){ 0x10, 0x28 },
+	.gpa_offset		= (u8 []){ 0x8, 0xe0 },
+	.gpda_offset		= (u8 []){ 0xc, 0xe4 },
+	.deb_offset		= (u8 []){ 0x14 },
+	.deb_val		= (u8 []){ 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7 },
+	.get_deb_setval		= rtd1295_iso_gpio_get_deb_setval,
+};
+
+static int rtd_gpio_dir_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->dir_offset[offset / 32];
+}
+
+static int rtd_gpio_dato_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->dato_offset[offset / 32];
+}
+
+static int rtd_gpio_dati_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->dati_offset[offset / 32];
+}
+
+static int rtd_gpio_ie_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->ie_offset[offset / 32];
+}
+
+static int rtd_gpio_dp_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->dp_offset[offset / 32];
+}
+
+static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->gpa_offset[offset / 31];
+}
+
+static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int offset)
+{
+	return data->info->gpda_offset[offset / 31];
+}
+
+static int rtd_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
+				   unsigned int debounce)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	u8 deb_val, deb_index, reg_offset, shift;
+	unsigned int write_en;
+	unsigned long flags;
+	u32 val;
+
+	switch (debounce) {
+	case 1:
+		deb_index = RTD_GPIO_DEBOUNCE_1US;
+		break;
+	case 10:
+		deb_index = RTD_GPIO_DEBOUNCE_10US;
+		break;
+	case 100:
+		deb_index = RTD_GPIO_DEBOUNCE_100US;
+		break;
+	case 1000:
+		deb_index = RTD_GPIO_DEBOUNCE_1MS;
+		break;
+	case 10000:
+		deb_index = RTD_GPIO_DEBOUNCE_10MS;
+		break;
+	case 20000:
+		deb_index = RTD_GPIO_DEBOUNCE_20MS;
+		break;
+	case 30000:
+		deb_index = RTD_GPIO_DEBOUNCE_30MS;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	deb_val = data->info->get_deb_setval(data->info, offset, &reg_offset, &shift, deb_index);
+	write_en = BIT(shift + 3);
+	val = (deb_val << shift) | write_en;
+
+	raw_spin_lock_irqsave(&data->lock, flags);
+	writel_relaxed(val, data->base + reg_offset);
+	raw_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 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);
+
+	raw_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);
+
+	raw_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;
+	u32 val;
+
+	dir_reg_offset = rtd_gpio_dir_offset(data, offset);
+
+	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;
+
+	return val;
+}
+
+static int rtd_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct rtd_gpio *data = gpiochip_get_data(chip);
+	int reg_offset;
+	u32 val;
+
+	reg_offset = rtd_gpio_dir_offset(data, offset);
+	val = readl_relaxed(data->base + reg_offset);
+	val &= BIT(offset % 32);
+
+	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);
+
+	raw_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);
+
+	raw_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)
+{
+	rtd_gpio_set(chip, offset, value);
+
+	return rtd_gpio_set_direction(chip, offset, true);
+}
+
+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);
+	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);
+	unsigned long status;
+	int reg_offset, i, j;
+	unsigned int hwirq;
+
+	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;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < data->info->num_gpios; i += 31) {
+		reg_offset = get_reg_offset(data, i);
+
+		status = readl_relaxed(data->irq_base + reg_offset) >> 1;
+		writel_relaxed(status << 1, data->irq_base + reg_offset);
+
+		for_each_set_bit(j, &status, 31) {
+			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_domain_irq(domain, hwirq);
+			}
+		}
+	}
+
+	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);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 clr_mask = BIT(hwirq % 31) << 1;
+	u32 ie_mask = BIT(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, hwirq);
+	gpa_reg_offset = rtd_gpio_gpa_offset(data, hwirq);
+	gpda_reg_offset = rtd_gpio_gpda_offset(data, hwirq);
+
+	gpiochip_enable_irq(gc, hwirq);
+	raw_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);
+
+	raw_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);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 ie_mask = BIT(hwirq % 32);
+	unsigned long flags;
+	int ie_reg_offset;
+	u32 val;
+
+	ie_reg_offset = rtd_gpio_ie_offset(data, hwirq);
+
+	raw_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);
+
+	raw_spin_unlock_irqrestore(&data->lock, flags);
+	gpiochip_disable_irq(gc, hwirq);
+}
+
+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);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 mask = BIT(hwirq % 32);
+	unsigned long flags;
+	int dp_reg_offset;
+	bool polarity;
+	u32 val;
+
+	dp_reg_offset = rtd_gpio_dp_offset(data, hwirq);
+
+	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;
+	}
+
+	raw_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);
+
+	raw_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 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;
+
+	raw_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(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.parent = 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(dev, &data->gpio_chip, data);
+}
+
+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 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);
+}
+
+module_init(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 v4 2/2] Add GPIO support for Realtek DHC(Digital Home Center) RTD SoCs.

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 03:58:12PM +0800, Tzuyi Chang wrote:
> This driver enables configuration of GPIO direction, GPIO values, GPIO
> debounce settings and handles GPIO interrupts.

...

> +	help
> +	  Say yes here to support GPIO on Realtek DHC(Digital Home Center)
> +	  SoCs.

checkpatch.pl complains if it's less than 3 lines.

...

Please, follow IWYU principle.

> +#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>

+ types.h

...

> +struct rtd_gpio_info {
> +	const char		*name;
> +	unsigned int		gpio_base;
> +	unsigned int		num_gpios;
> +	u8			*dir_offset;
> +	u8			*dato_offset;
> +	u8			*dati_offset;
> +	u8			*ie_offset;
> +	u8			*dp_offset;
> +	u8			*gpa_offset;
> +	u8			*gpda_offset;
> +	u8			*deb_offset;
> +	u8			*deb_val;
> +	u8			(*get_deb_setval)(const struct rtd_gpio_info *info,
> +						   unsigned int offset, u8 *reg_offset,
> +						   u8 *shift, u8 deb_index);

Basically you should group input parameters and output for better
understanding.

	u8		(*get_deb_setval)(const struct rtd_gpio_info *info,
					  unsigned int offset, u8 deb_index,
					  u8 *reg_offset, u8 *shift);

Also indent the lines properly (besides the TABs).

> +};

Make it one TAB less in the middle.

...

> +static u8 rtd_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned int offset,
> +				u8 *reg_offset, u8 *shift, u8 deb_val)

Why is it called val here and index in the other cases?
Can you come up with better naming that it can be consistent in all four places?

> +{
> +	*reg_offset = info->deb_offset[offset / 8];
> +	*shift = (offset % 8) * 4;
> +	return info->deb_val[deb_val];
> +}
> +
> +static u8 rtd1295_misc_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned int offset,
> +					 u8 *reg_offset, u8 *shift, u8 deb_index)
> +{
> +	*reg_offset = info->deb_offset[0];
> +	*shift = (offset % 8) * 4;
> +	return info->deb_val[deb_index];
> +}

> +static u8 rtd1295_iso_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned int offset,
> +					u8 *reg_offset, u8 *shift, u8 deb_index)
> +{
> +	*reg_offset = info->deb_offset[0];
> +	*shift = 0;
> +	return info->deb_val[deb_index];
> +}

...

> +static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int offset)
> +{
> +	return data->info->gpa_offset[offset / 31];
> +}
> +
> +static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int offset)
> +{
> +	return data->info->gpda_offset[offset / 31];
> +}

The / 31 so-o-o counter intuitive, please add a comment in each case to explain
why [it's not 32 or other power-of-2].

...

> +	raw_spin_lock_irqsave(&data->lock, flags);
> +	writel_relaxed(val, data->base + reg_offset);
> +	raw_spin_unlock_irqrestore(&data->lock, flags);

Convert to use cleanup.c, in particular here it becomes

	guard(raw_spinlock_irqsave)(&data->lock);

	writel_relaxed(val, data->base + reg_offset);

...

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

Replace 3 LoCs by 1:

	return !!(val & BIT(ofsset % 32));


Missed locking. How do you guarantee that you will get consistent results
between the reads?

...

> +	val &= BIT(offset % 32);
> +
> +	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

	if (val & BIT(...))
		return _OUT;
	return _IN;

...

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

Same, add explanation why 31.

Note, I actually prefer to see use of valid_mask instead of this weirdness.
Then you will need to comment only once and use 32 (almost?) everywhere.

> +		reg_offset = get_reg_offset(data, i);
> +
> +		status = readl_relaxed(data->irq_base + reg_offset) >> 1;
> +		writel_relaxed(status << 1, data->irq_base + reg_offset);
> +
> +		for_each_set_bit(j, &status, 31) {
> +			hwirq = i + j;

Nice, but you can do better

		/* Bit 0 is special... bla-bla-bla... */
		status = readl_relaxed(data->irq_base + reg_offset);
		status &= ~BIT(0);
		writel_relaxed(status, data->irq_base + reg_offset);

		for_each_set_bit(j, &status, 32) {
			hwirq = i + j - 1;

> +			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))

Do you need mask? Isn't irq_type already properly masked here?

> +					break;
> +				generic_handle_domain_irq(domain, hwirq);
> +			}
> +		}
> +	}

...

> +	u32 clr_mask = BIT(hwirq % 31) << 1;
> +	u32 ie_mask = BIT(hwirq % 32);

This blows the mind. Needs a comment.

...

> +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);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 mask = BIT(hwirq % 32);
> +	unsigned long flags;
> +	int dp_reg_offset;
> +	bool polarity;
> +	u32 val;
> +
> +	dp_reg_offset = rtd_gpio_dp_offset(data, hwirq);
> +
> +	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;
> +	}
> +
> +	raw_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);
> +
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return 0;
> +}

...

> +	irq_chip->handler = handle_simple_irq;

Please, apply bad handler here and lock it in the set_type callback above.

You may read eb441337c714 ("gpio: pca953x: Set IRQ type when handle Intel
Galileo Gen 2") to understand the difference.

...

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

> +

Redundant blank line, but see below.

> +module_init(rtd_gpio_init);
> +
> +static void __exit rtd_gpio_exit(void)
> +{
> +	platform_driver_unregister(&rtd_gpio_platform_driver);
> +}
> +module_exit(rtd_gpio_exit);

There is no special initcall, you may use module_platform_driver() macro
instead.

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Kozlowski]

On 22/12/2023 08:58, Tzuyi Chang wrote:
> 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>
> ---


...

> +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);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 mask = BIT(hwirq % 32);
> +	unsigned long flags;
> +	int dp_reg_offset;
> +	bool polarity;
> +	u32 val;
> +
> +	dp_reg_offset = rtd_gpio_dp_offset(data, hwirq);
> +
> +	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;
> +	}
> +
> +	raw_spin_lock_irqsave(&data->lock, flags);

Why are you using raw spinlock? This question applies to entire driver.

> +
> +	val = readl_relaxed(data->base + dp_reg_offset);
> +	if (polarity)
> +		val |= mask;
> +	else
> +		val &= ~mask;
> +	writel_relaxed(val, data->base + dp_reg_offset);
> +
> +	raw_spin_unlock_irqrestore(&data->lock, flags);
> +
> +	return 0;
> +}

...

}
> +
> +module_init(rtd_gpio_init);
> +
> +static void __exit rtd_gpio_exit(void)
> +{
> +	platform_driver_unregister(&rtd_gpio_platform_driver);
> +}
> +module_exit(rtd_gpio_exit);

Why not using module_platform_driver?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 22/12/2023 08:58, Tzuyi Chang wrote:
> Add the device tree bindings for the Realtek DHC(Digital Home Center)
> RTD SoCs GPIO controllers.
> 
> Signed-off-by: Tzuyi Chang <tychang@realtek.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Andy Shevchenko]

On Sat, Dec 23, 2023 at 4:19 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 22/12/2023 08:58, Tzuyi Chang wrote:

...

> > +     raw_spin_lock_irqsave(&data->lock, flags);
>
> Why are you using raw spinlock? This question applies to entire driver.

If you want to have your IRQ chip to work in the RT kernel, you need a
real spinlock.

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


-- 
With Best Regards,
Andy Shevchenko

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

[TY_Chang[張子逸]]

Hi Andy,

Thank you for the review.

>On Fri, Dec 22, 2023 at 03:58:12PM +0800, Tzuyi Chang wrote:
>> This driver enables configuration of GPIO direction, GPIO values, GPIO
>> debounce settings and handles GPIO interrupts.
>
>...
>
>> +     help
>> +       Say yes here to support GPIO on Realtek DHC(Digital Home Center)
>> +       SoCs.
>
>checkpatch.pl complains if it's less than 3 lines.
>

I will add more description.

>...
>
>Please, follow IWYU principle.
>
>> +#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>
>
>+ types.h
>

I will revise it.

>...
>
>> +struct rtd_gpio_info {
>> +     const char              *name;
>> +     unsigned int            gpio_base;
>> +     unsigned int            num_gpios;
>> +     u8                      *dir_offset;
>> +     u8                      *dato_offset;
>> +     u8                      *dati_offset;
>> +     u8                      *ie_offset;
>> +     u8                      *dp_offset;
>> +     u8                      *gpa_offset;
>> +     u8                      *gpda_offset;
>> +     u8                      *deb_offset;
>> +     u8                      *deb_val;
>> +     u8                      (*get_deb_setval)(const struct
>rtd_gpio_info *info,
>> +                                                unsigned int
>offset, u8 *reg_offset,
>> +                                                u8 *shift, u8
>> +deb_index);
>
>Basically you should group input parameters and output for better
>understanding.
>

I will define a structure to hold the necessary information for the output.

>        u8              (*get_deb_setval)(const struct rtd_gpio_info
>*info,
>                                          unsigned int offset, u8
>deb_index,
>                                          u8 *reg_offset, u8 *shift);
>
>Also indent the lines properly (besides the TABs).
>
>> +};
>
>Make it one TAB less in the middle.
>

I will revise it.

>...
>
>> +static u8 rtd_gpio_get_deb_setval(const struct rtd_gpio_info *info, unsigned
>int offset,
>> +                             u8 *reg_offset, u8 *shift, u8 deb_val)
>
>Why is it called val here and index in the other cases?
>Can you come up with better naming that it can be consistent in all four places?
>

I missed to rename it to 'deb_index'. Sorry about that.

>> +{
>> +     *reg_offset = info->deb_offset[offset / 8];
>> +     *shift = (offset % 8) * 4;
>> +     return info->deb_val[deb_val];
>> +}
>> +
>> +static u8 rtd1295_misc_gpio_get_deb_setval(const struct rtd_gpio_info *info,
>unsigned int offset,
>> +                                      u8 *reg_offset, u8 *shift, u8
>> +deb_index) {
>> +     *reg_offset = info->deb_offset[0];
>> +     *shift = (offset % 8) * 4;
>> +     return info->deb_val[deb_index]; }
>
>> +static u8 rtd1295_iso_gpio_get_deb_setval(const struct rtd_gpio_info *info,
>unsigned int offset,
>> +                                     u8 *reg_offset, u8 *shift, u8
>> +deb_index) {
>> +     *reg_offset = info->deb_offset[0];
>> +     *shift = 0;
>> +     return info->deb_val[deb_index]; }
>
>...
>
>> +static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int
>> +offset) {
>> +     return data->info->gpa_offset[offset / 31]; }
>> +
>> +static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int
>> +offset) {
>> +     return data->info->gpda_offset[offset / 31]; }
>
>The / 31 so-o-o counter intuitive, please add a comment in each case to explain
>why [it's not 32 or other power-of-2].
>

In our hardware design, the bit 0 of the gpda and gpa status registers does not correspond to a GPIO.
If bit 0 is set to 1, the other bit can be set to 1 by writing 1.
If bit 0 is set to 0, the other bit can be clear to 0 by writing 1.

Therefore, each status register only contains the status of 31 GPIOs. I will add the comment for this.

>...
>
>> +     raw_spin_lock_irqsave(&data->lock, flags);
>> +     writel_relaxed(val, data->base + reg_offset);
>> +     raw_spin_unlock_irqrestore(&data->lock, flags);
>
>Convert to use cleanup.c, in particular here it becomes
>
>        guard(raw_spinlock_irqsave)(&data->lock);
>
>        writel_relaxed(val, data->base + reg_offset);
>

I will revise it.

>...
>
>> +     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;
>
>Replace 3 LoCs by 1:
>
>        return !!(val & BIT(ofsset % 32));
>
>
>Missed locking. How do you guarantee that you will get consistent results
>between the reads?
>

I will revise it.

>...
>
>> +     val &= BIT(offset % 32);
>> +
>> +     return val ? GPIO_LINE_DIRECTION_OUT :
>GPIO_LINE_DIRECTION_IN;
>
>        if (val & BIT(...))
>                return _OUT;
>        return _IN;
>
>...
>
>> +     for (i = 0; i < data->info->num_gpios; i += 31) {
>
>Same, add explanation why 31.
>
>Note, I actually prefer to see use of valid_mask instead of this weirdness.
>Then you will need to comment only once and use 32 (almost?) everywhere.
>

The reason remains consistent with the previous explanation. Each status register
exclusively holds the status of 31 GPIOs.

>> +             reg_offset = get_reg_offset(data, i);
>> +
>> +             status = readl_relaxed(data->irq_base + reg_offset) >> 1;
>> +             writel_relaxed(status << 1, data->irq_base +
>> + reg_offset);
>> +
>> +             for_each_set_bit(j, &status, 31) {
>> +                     hwirq = i + j;
>
>Nice, but you can do better
>
>                /* Bit 0 is special... bla-bla-bla... */
>                status = readl_relaxed(data->irq_base + reg_offset);
>                status &= ~BIT(0);
>                writel_relaxed(status, data->irq_base + reg_offset);
>
>                for_each_set_bit(j, &status, 32) {
>                        hwirq = i + j - 1;
>

Given that each status register accommodates the status of only 31 GPIOs, I think utilizing 
the upper format and including explanatory comments would be appropriate. It can indicate the
status registers only contains 31 GPIOs. Please correct me if my understanding is incorrect.

>> +                     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))
>
>Do you need mask? Isn't irq_type already properly masked here?
>
>> +                                     break;
>> +                             generic_handle_domain_irq(domain,
>hwirq);
>> +                     }
>> +             }
>> +     }
>

It has already been masked in the irq_get_trigger_type. I will revise it.

>...
>
>> +     u32 clr_mask = BIT(hwirq % 31) << 1;
>> +     u32 ie_mask = BIT(hwirq % 32);
>
>This blows the mind. Needs a comment.
>

The clr_mask is used to clear the gpa/gpda registers, each of which accommodates only 31 GPIOs.

>...
>
>> +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);
>> +     irq_hw_number_t hwirq = irqd_to_hwirq(d);
>> +     u32 mask = BIT(hwirq % 32);
>> +     unsigned long flags;
>> +     int dp_reg_offset;
>> +     bool polarity;
>> +     u32 val;
>> +
>> +     dp_reg_offset = rtd_gpio_dp_offset(data, hwirq);
>> +
>> +     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;
>> +     }
>> +
>> +     raw_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);
>> +
>> +     raw_spin_unlock_irqrestore(&data->lock, flags);
>> +
>> +     return 0;
>> +}
>
>...
>
>> +     irq_chip->handler = handle_simple_irq;
>
>Please, apply bad handler here and lock it in the set_type callback above.
>
>You may read eb441337c714 ("gpio: pca953x: Set IRQ type when handle Intel
>Galileo Gen 2") to understand the difference.
>

I will revise it.

>...
>
>> +static int rtd_gpio_init(void)
>> +{
>> +     return platform_driver_register(&rtd_gpio_platform_driver);
>> +}
>
>> +
>
>Redundant blank line, but see below.
>
>> +module_init(rtd_gpio_init);
>> +
>> +static void __exit rtd_gpio_exit(void) {
>> +     platform_driver_unregister(&rtd_gpio_platform_driver);
>> +}
>> +module_exit(rtd_gpio_exit);
>
>There is no special initcall, you may use module_platform_driver() macro
>instead.
>

I will revise it.

Thanks,
Tzuyi Chang

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

[TY_Chang[張子逸]]

Hi Krzysztof,

Thank you for the review.

>...
>
>> +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);
>> +     irq_hw_number_t hwirq = irqd_to_hwirq(d);
>> +     u32 mask = BIT(hwirq % 32);
>> +     unsigned long flags;
>> +     int dp_reg_offset;
>> +     bool polarity;
>> +     u32 val;
>> +
>> +     dp_reg_offset = rtd_gpio_dp_offset(data, hwirq);
>> +
>> +     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;
>> +     }
>> +
>> +     raw_spin_lock_irqsave(&data->lock, flags);
>
>Why are you using raw spinlock? This question applies to entire driver.
>

I think consumer driver may operate GPIOs within the ISR, so it needs a lock.

>> +
>> +     val = readl_relaxed(data->base + dp_reg_offset);
>> +     if (polarity)
>> +             val |= mask;
>> +     else
>> +             val &= ~mask;
>> +     writel_relaxed(val, data->base + dp_reg_offset);
>> +
>> +     raw_spin_unlock_irqrestore(&data->lock, flags);
>> +
>> +     return 0;
>> +}
>
>...
>
>}
>> +
>> +module_init(rtd_gpio_init);
>> +
>> +static void __exit rtd_gpio_exit(void) {
>> +     platform_driver_unregister(&rtd_gpio_platform_driver);
>> +}
>> +module_exit(rtd_gpio_exit);
>
>Why not using module_platform_driver?
>

I will revise it.

Thanks,
Tzuyi Chang

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

[Andy Shevchenko]

On Tue, Dec 26, 2023 at 07:34:37AM +0000, TY_Chang[張子逸] wrote:
> >On Fri, Dec 22, 2023 at 03:58:12PM +0800, Tzuyi Chang wrote:

...

> >> +static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int
> >> +offset) {
> >> +     return data->info->gpa_offset[offset / 31]; }
> >> +
> >> +static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned int
> >> +offset) {
> >> +     return data->info->gpda_offset[offset / 31]; }
> >
> >The / 31 so-o-o counter intuitive, please add a comment in each case to explain
> >why [it's not 32 or other power-of-2].
> >
> 
> In our hardware design, the bit 0 of the gpda and gpa status registers does not correspond to a GPIO.
> If bit 0 is set to 1, the other bit can be set to 1 by writing 1.
> If bit 0 is set to 0, the other bit can be clear to 0 by writing 1.
> 
> Therefore, each status register only contains the status of 31 GPIOs. I will add the comment for this.

Yes, please add in all places, while it's a dup, it helps understanding
the point without looking around for a while.

...

> >> +     for (i = 0; i < data->info->num_gpios; i += 31) {
> >
> >Same, add explanation why 31.
> >
> >Note, I actually prefer to see use of valid_mask instead of this weirdness.
> >Then you will need to comment only once and use 32 (almost?) everywhere.
> >
> 
> The reason remains consistent with the previous explanation. Each status
> register exclusively holds the status of 31 GPIOs.

As per above, add a comment.

> >> +             reg_offset = get_reg_offset(data, i);
> >> +
> >> +             status = readl_relaxed(data->irq_base + reg_offset) >> 1;
> >> +             writel_relaxed(status << 1, data->irq_base +
> >> + reg_offset);
> >> +
> >> +             for_each_set_bit(j, &status, 31) {
> >> +                     hwirq = i + j;
> >
> >Nice, but you can do better
> >
> >                /* Bit 0 is special... bla-bla-bla... */
> >                status = readl_relaxed(data->irq_base + reg_offset);
> >                status &= ~BIT(0);
> >                writel_relaxed(status, data->irq_base + reg_offset);
> >
> >                for_each_set_bit(j, &status, 32) {
> >                        hwirq = i + j - 1;
> >
> 
> Given that each status register accommodates the status of only 31 GPIOs, I
> think utilizing the upper format and including explanatory comments would be
> appropriate. It can indicate the status registers only contains 31 GPIOs.
> Please correct me if my understanding is incorrect.

The above is just a code hack to help bitops to optimise. 32 is power-of-2
which might be treated better by the compiler and hence produce better code.

Yet, it's an interrupt handler where we want to have the ops as shorter as
possible, so even micro-optimizations are good to have here (I don't insist
to follow the same idea elsewhere).

> >> +                     }
> >> +             }
> >> +     }

-- 
With Best Regards,
Andy Shevchenko

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

[TY_Chang[張子逸]]

Hi Andy,

>On Tue, Dec 26, 2023 at 07:34:37AM +0000, TY_Chang[張子逸] wrote:
>> >On Fri, Dec 22, 2023 at 03:58:12PM +0800, Tzuyi Chang wrote:
>
>...
>
>> >> +static int rtd_gpio_gpa_offset(struct rtd_gpio *data, unsigned int
>> >> +offset) {
>> >> +     return data->info->gpa_offset[offset / 31]; }
>> >> +
>> >> +static int rtd_gpio_gpda_offset(struct rtd_gpio *data, unsigned
>> >> +int
>> >> +offset) {
>> >> +     return data->info->gpda_offset[offset / 31]; }
>> >
>> >The / 31 so-o-o counter intuitive, please add a comment in each case
>> >to explain why [it's not 32 or other power-of-2].
>> >
>>
>> In our hardware design, the bit 0 of the gpda and gpa status registers does not
>correspond to a GPIO.
>> If bit 0 is set to 1, the other bit can be set to 1 by writing 1.
>> If bit 0 is set to 0, the other bit can be clear to 0 by writing 1.
>>
>> Therefore, each status register only contains the status of 31 GPIOs. I will add
>the comment for this.
>
>Yes, please add in all places, while it's a dup, it helps understanding the point
>without looking around for a while.
>
>...
>
>> >> +     for (i = 0; i < data->info->num_gpios; i += 31) {
>> >
>> >Same, add explanation why 31.
>> >
>> >Note, I actually prefer to see use of valid_mask instead of this weirdness.
>> >Then you will need to comment only once and use 32 (almost?) everywhere.
>> >
>>
>> The reason remains consistent with the previous explanation. Each
>> status register exclusively holds the status of 31 GPIOs.
>
>As per above, add a comment.
>
>> >> +             reg_offset = get_reg_offset(data, i);
>> >> +
>> >> +             status = readl_relaxed(data->irq_base + reg_offset) >> 1;
>> >> +             writel_relaxed(status << 1, data->irq_base +
>> >> + reg_offset);
>> >> +
>> >> +             for_each_set_bit(j, &status, 31) {
>> >> +                     hwirq = i + j;
>> >
>> >Nice, but you can do better
>> >
>> >                /* Bit 0 is special... bla-bla-bla... */
>> >                status = readl_relaxed(data->irq_base + reg_offset);
>> >                status &= ~BIT(0);
>> >                writel_relaxed(status, data->irq_base + reg_offset);
>> >
>> >                for_each_set_bit(j, &status, 32) {
>> >                        hwirq = i + j - 1;
>> >
>>
>> Given that each status register accommodates the status of only 31
>> GPIOs, I think utilizing the upper format and including explanatory
>> comments would be appropriate. It can indicate the status registers only
>contains 31 GPIOs.
>> Please correct me if my understanding is incorrect.
>
>The above is just a code hack to help bitops to optimise. 32 is power-of-2 which
>might be treated better by the compiler and hence produce better code.
>
>Yet, it's an interrupt handler where we want to have the ops as shorter as
>possible, so even micro-optimizations are good to have here (I don't insist to
>follow the same idea elsewhere).
>

I understand. Thank you for explaining. I will revise it.

>> >> +                     }
>> >> +             }
>> >> +     }
>

Thanks,
Tzuyi Chang