[PATCH v3 0/7] gpio: realtek: Add support for Realtek DHC RTD1625

[PATCH v3 0/7] gpio: realtek: Add support for Realtek DHC RTD1625

[Yu-Chun Lin]

Hi all,

This series adds GPIO support for the Realtek DHC RTD1625 SoC.

Unlike the existing driver (gpio-rtd.c) which uses shared bank registers,
the RTD1625 features a per-pin register architecture where each GPIO line
is managed by its own dedicated 32-bit control register. This distinct
hardware design requires a new, separate driver.

To accommodate this, we extend the gpio-regmap core framework to handle
per-pin register operations, write-enable mechanisms, and add custom
set_config callback.

Best Regards,
Yu-Chun Lin
---
Changes in v3:

patch 1 (gpio: Replace "default y" with "default ARCH_REATLEK" in Kconfig):
- Chang "remove default y" to "replace it with default ARCH_REALTEK".

patch 2 (gpio: regmap: add gpio_regmap_get_gpiochip() accessor):
- New patch

patch 3 (gpio: regmap: Add gpio_regmap_operation and write-enable support):
- New patch
- Update all drivers utilizing the gpio-regmap framework to accommodate
the new reg_mask_xlate function signature.

patch 4 (gpio: regmap: Add set_config callback):
- New patch

patch 5 (dt-bindings: gpio: realtek: Add realtek,rtd1625-gpio):
- Remove description for reg.
- Add Reviewed-by tag from Krzysztof.

patch 6 (gpio: realtek: Add driver for Realtek DHC RTD1625 SoC):
- Refactor to utilize the gpio-regmap framework.
- Create a custom irqdomain.

patch 7(arm64: dts: realtek: Add GPIO support for RTD1625):
- Add Reviewed-by tag from Bartosz.

v2: https://lore.kernel.org/lkml/20260408025243.1155482-1-eleanor.lin@realtek.com/
v1: https://lore.kernel.org/lkml/20260331113835.3510341-1-eleanor.lin@realtek.com/

Tzuyi Chang (2):
  dt-bindings: gpio: realtek: Add realtek,rtd1625-gpio
  gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

Yu-Chun Lin (5):
  gpio: Replace "default y" with "default ARCH_REALTEK" in Kconfig
  gpio: regmap: add gpio_regmap_get_gpiochip() accessor
  gpio: regmap: Add gpio_regmap_operation and write-enable support
  gpio: regmap: Add set_config callback
  arm64: dts: realtek: Add GPIO support for RTD1625

 .../bindings/gpio/realtek,rtd1625-gpio.yaml   |  71 ++
 arch/arm64/boot/dts/realtek/kent.dtsi         |  39 ++
 drivers/gpio/Kconfig                          |  15 +-
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-104-idi-48.c                |  18 +-
 drivers/gpio/gpio-i8255.c                     |  13 +-
 drivers/gpio/gpio-idio-16.c                   |  16 +-
 drivers/gpio/gpio-max7360.c                   |  10 +
 drivers/gpio/gpio-pcie-idio-24.c              |  15 +-
 drivers/gpio/gpio-regmap.c                    |  80 ++-
 drivers/gpio/gpio-rtd1625.c                   | 608 ++++++++++++++++++
 drivers/iio/adc/ad7173.c                      |  32 +-
 drivers/iio/addac/stx104.c                    |  17 +-
 drivers/pinctrl/bcm/pinctrl-bcm63xx.c         |  12 +-
 drivers/pinctrl/pinctrl-tps6594.c             |  10 +
 include/linux/gpio/regmap.h                   |  51 +-
 16 files changed, 963 insertions(+), 45 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml
 create mode 100644 drivers/gpio/gpio-rtd1625.c

-- 
2.34.1

[PATCH v3 1/7] gpio: Replace "default y" with "default ARCH_REALTEK" in Kconfig

[Yu-Chun Lin]

Replace "default y" with "default ARCH_REALTEK" to avoid bloating the build
for non-Realtek platforms when COMPILE_TEST is enabled on other platforms.

Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
 drivers/gpio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 020e51e30317..504b4bdd75d4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -637,7 +637,7 @@ config GPIO_ROCKCHIP
 config GPIO_RTD
 	tristate "Realtek DHC GPIO support"
 	depends on ARCH_REALTEK || COMPILE_TEST
-	default y
+	default ARCH_REALTEK
 	select GPIOLIB_IRQCHIP
 	help
 	  This option enables support for GPIOs found on Realtek DHC(Digital
-- 
2.34.1

[PATCH v3 2/7] gpio: regmap: add gpio_regmap_get_gpiochip() accessor

[Yu-Chun Lin]

Expose an accessor function to retrieve the gpio_chip pointer from
a gpio_regmap instance.

This is needed by drivers that use gpio_regmap but also manage their
own irq_chip, where gpiochip_enable_irq()/gpiochip_disable_irq() must
be called with the gpio_chip pointer.

Add gpio_regmap_get_gpiochip() to allow drivers with complex custom IRQ
implementations.

Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
 drivers/gpio/gpio-regmap.c  | 6 ++++++
 include/linux/gpio/regmap.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 9ae4a41a2427..deb9eebb58de 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -230,6 +230,12 @@ void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
 }
 EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
 
+struct gpio_chip *gpio_regmap_get_gpiochip(struct gpio_regmap *gpio)
+{
+	return &gpio->gpio_chip;
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_get_gpiochip);
+
 /**
  * gpio_regmap_register() - Register a generic regmap GPIO controller
  * @config: configuration for gpio_regmap
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 12d154732ca9..e4a95f805a81 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -113,5 +113,6 @@ void gpio_regmap_unregister(struct gpio_regmap *gpio);
 struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
 					      const struct gpio_regmap_config *config);
 void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio);
+struct gpio_chip *gpio_regmap_get_gpiochip(struct gpio_regmap *gpio);
 
 #endif /* _LINUX_GPIO_REGMAP_H */
-- 
2.34.1

Re: [PATCH v3 2/7] gpio: regmap: add gpio_regmap_get_gpiochip() accessor

[Andy Shevchenko]

On Tue, May 12, 2026 at 11:33:12AM +0800, Yu-Chun Lin wrote:
> Expose an accessor function to retrieve the gpio_chip pointer from
> a gpio_regmap instance.
> 
> This is needed by drivers that use gpio_regmap but also manage their
> own irq_chip, where gpiochip_enable_irq()/gpiochip_disable_irq() must
> be called with the gpio_chip pointer.
> 
> Add gpio_regmap_get_gpiochip() to allow drivers with complex custom IRQ
> implementations.

Hmm... Can't we rather add gpio_regmap_enable_irq()/gpio_regmap_disable_irq()
that take regmap or GPIO regmap (whatever suits better for the purpose) and
do the magic inside GPIO regmap library code?


-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[Yu-Chun Lin]

Extend the reg_mask_xlate callback with an operation type parameter
(gpio_regmap_operation) to allow drivers to return different
register/mask combinations for different GPIO operations.

Also add write-enable mechanism for hardware that requires setting a
write-enable bit before modifying GPIO control registers.

Consequently, update all existing drivers utilizing the gpio-regmap
framework (across drivers/gpio, drivers/iio, and drivers/pinctrl)
to accommodate the new reg_mask_xlate function signature.

Suggested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
 drivers/gpio/gpio-104-idi-48.c        | 18 +++++--
 drivers/gpio/gpio-i8255.c             | 13 ++++-
 drivers/gpio/gpio-idio-16.c           | 16 ++++--
 drivers/gpio/gpio-max7360.c           | 10 ++++
 drivers/gpio/gpio-pcie-idio-24.c      | 15 ++++--
 drivers/gpio/gpio-regmap.c            | 72 ++++++++++++++++++++-------
 drivers/iio/adc/ad7173.c              | 32 +++++++++---
 drivers/iio/addac/stx104.c            | 17 +++++--
 drivers/pinctrl/bcm/pinctrl-bcm63xx.c | 12 ++++-
 drivers/pinctrl/pinctrl-tps6594.c     | 10 ++++
 include/linux/gpio/regmap.h           | 43 ++++++++++++++--
 11 files changed, 214 insertions(+), 44 deletions(-)

diff --git a/drivers/gpio/gpio-104-idi-48.c b/drivers/gpio/gpio-104-idi-48.c
index ba73ee9c0c29..ebd0587f99c5 100644
--- a/drivers/gpio/gpio-104-idi-48.c
+++ b/drivers/gpio/gpio-104-idi-48.c
@@ -36,9 +36,10 @@ MODULE_PARM_DESC(irq, "ACCES 104-IDI-48 interrupt line numbers");
 #define IDI48_IRQ_STATUS 0x7
 #define IDI48_IRQ_ENABLE IDI48_IRQ_STATUS
 
-static int idi_48_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
-				 unsigned int offset, unsigned int *reg,
-				 unsigned int *mask)
+static int idi_48_reg_mask_xlate(struct gpio_regmap *gpio,
+				 enum gpio_regmap_operation op,
+				 unsigned int base, unsigned int offset,
+				 unsigned int *reg, unsigned int *mask)
 {
 	const unsigned int line = offset % 8;
 	const unsigned int stride = offset / 8;
@@ -46,7 +47,16 @@ static int idi_48_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
 	const unsigned int port_stride = stride % 3;
 
 	*reg = base + port + port_stride;
-	*mask = BIT(line);
+
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		break;
+	default:
+		*mask = BIT(line);
+		break;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpio/gpio-i8255.c b/drivers/gpio/gpio-i8255.c
index 953018bfa2b1..ec3c3186e2d1 100644
--- a/drivers/gpio/gpio-i8255.c
+++ b/drivers/gpio/gpio-i8255.c
@@ -67,8 +67,8 @@ static int i8255_ppi_init(struct regmap *const map, const unsigned int base)
 	return regmap_write(map, base + I8255_PORTC, 0x00);
 }
 
-static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
-				unsigned int offset, unsigned int *reg,
+static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
+				unsigned int base, unsigned int offset, unsigned int *reg,
 				unsigned int *mask)
 {
 	const unsigned int ppi = offset / I8255_NGPIO;
@@ -76,6 +76,15 @@ static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
 	const unsigned int stride = ppi_offset / I8255_NGPIO_PER_REG;
 	const unsigned int line = ppi_offset % I8255_NGPIO_PER_REG;
 
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		return 0;
+	default:
+		break;
+	}
+
 	switch (base) {
 	case I8255_REG_DAT_BASE:
 		*reg = base + stride + ppi * 4;
diff --git a/drivers/gpio/gpio-idio-16.c b/drivers/gpio/gpio-idio-16.c
index 4fbae6f6a497..d78a05c22531 100644
--- a/drivers/gpio/gpio-idio-16.c
+++ b/drivers/gpio/gpio-idio-16.c
@@ -66,9 +66,9 @@ static int idio_16_handle_mask_sync(const int index, const unsigned int mask_buf
 	return 0;
 }
 
-static int idio_16_reg_mask_xlate(struct gpio_regmap *const gpio, const unsigned int base,
-				  const unsigned int offset, unsigned int *const reg,
-				  unsigned int *const mask)
+static int idio_16_reg_mask_xlate(struct gpio_regmap *const gpio, enum gpio_regmap_operation op,
+				  const unsigned int base, const unsigned int offset,
+				  unsigned int *const reg, unsigned int *const mask)
 {
 	unsigned int stride;
 
@@ -81,7 +81,15 @@ static int idio_16_reg_mask_xlate(struct gpio_regmap *const gpio, const unsigned
 		*reg = IDIO_16_IN_BASE + stride * IDIO_16_REG_STRIDE;
 	}
 
-	*mask = BIT(offset % IDIO_16_NGPIO_PER_REG);
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		break;
+	default:
+		*mask = BIT(offset % IDIO_16_NGPIO_PER_REG);
+		break;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpio/gpio-max7360.c b/drivers/gpio/gpio-max7360.c
index db92a43776a9..2a26313d021d 100644
--- a/drivers/gpio/gpio-max7360.c
+++ b/drivers/gpio/gpio-max7360.c
@@ -94,9 +94,19 @@ static int max7360_set_gpos_count(struct device *dev, struct regmap *regmap)
 }
 
 static int max7360_gpio_reg_mask_xlate(struct gpio_regmap *gpio,
+				       enum gpio_regmap_operation op,
 				       unsigned int base, unsigned int offset,
 				       unsigned int *reg, unsigned int *mask)
 {
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		return 0;
+	default:
+		break;
+	}
+
 	if (base == MAX7360_REG_PWMBASE) {
 		/*
 		 * GPIO output is using PWM duty cycle registers: one register
diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
index 80c0ba0afa67..dc80f96174c2 100644
--- a/drivers/gpio/gpio-pcie-idio-24.c
+++ b/drivers/gpio/gpio-pcie-idio-24.c
@@ -225,9 +225,9 @@ static int idio_24_set_type_config(unsigned int **const buf, const unsigned int
 	return ret;
 }
 
-static int idio_24_reg_mask_xlate(struct gpio_regmap *const gpio, const unsigned int base,
-				  const unsigned int offset, unsigned int *const reg,
-				  unsigned int *const mask)
+static int idio_24_reg_mask_xlate(struct gpio_regmap *const gpio, enum gpio_regmap_operation op,
+				  const unsigned int base, const unsigned int offset,
+				  unsigned int *const reg, unsigned int *const mask)
 {
 	const unsigned int out_stride = offset / IDIO_24_NGPIO_PER_REG;
 	const unsigned int in_stride = (offset - 24) / IDIO_24_NGPIO_PER_REG;
@@ -235,6 +235,15 @@ static int idio_24_reg_mask_xlate(struct gpio_regmap *const gpio, const unsigned
 	int err;
 	unsigned int ctrl_reg;
 
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		return 0;
+	default:
+		break;
+	}
+
 	switch (base) {
 	case IDIO_24_OUT_BASE:
 		*mask = BIT(offset % IDIO_24_NGPIO_PER_REG);
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index deb9eebb58de..c76eef20e412 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -38,9 +38,10 @@ struct gpio_regmap {
 	struct regmap_irq_chip_data *irq_chip_data;
 #endif
 
-	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
-			      unsigned int offset, unsigned int *reg,
-			      unsigned int *mask);
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio,
+			      enum gpio_regmap_operation op,
+			      unsigned int base, unsigned int offset,
+			      unsigned int *reg, unsigned int *mask);
 
 	void *driver_data;
 };
@@ -54,6 +55,7 @@ static unsigned int gpio_regmap_addr(unsigned int addr)
 }
 
 static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
+				    enum gpio_regmap_operation op,
 				    unsigned int base, unsigned int offset,
 				    unsigned int *reg, unsigned int *mask)
 {
@@ -61,7 +63,16 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
 	unsigned int stride = offset / gpio->ngpio_per_reg;
 
 	*reg = base + stride * gpio->reg_stride;
-	*mask = BIT(line);
+
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		break;
+	default:
+		*mask = BIT(line);
+		break;
+	}
 
 	return 0;
 }
@@ -69,7 +80,7 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
 static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
-	unsigned int base, val, reg, mask;
+	unsigned int base, val, reg, mask, dir_mask;
 	int ret;
 
 	/* we might not have an output register if we are input only */
@@ -78,10 +89,24 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
 	else
 		base = gpio_regmap_addr(gpio->reg_set_base);
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_OP, base, offset, &reg, &dir_mask);
 	if (ret)
 		return ret;
 
+	ret = regmap_read(gpio->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	if (val & dir_mask) {
+		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_OUT, base, offset, &reg, &mask);
+		if (ret)
+			return ret;
+	} else {
+		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_IN, base, offset, &reg, &mask);
+		if (ret)
+			return ret;
+	}
+
 	/* ensure we don't spoil any register cache with pin input values */
 	if (gpio->reg_dat_base == gpio->reg_set_base)
 		ret = regmap_read_bypassed(gpio->regmap, reg, &val);
@@ -98,10 +123,14 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
 	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
-	unsigned int reg, mask, mask_val;
+	unsigned int reg, mask, mask_val, wren_mask;
 	int ret;
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
+	if (ret)
+		return ret;
+
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base, offset, &reg, &mask);
 	if (ret)
 		return ret;
 
@@ -112,9 +141,9 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
 
 	/* ignore input values which shadow the old output value */
 	if (gpio->reg_dat_base == gpio->reg_set_base)
-		ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
+		ret = regmap_write_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
 	else
-		ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
+		ret = regmap_update_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
 
 	return ret;
 }
@@ -123,7 +152,7 @@ static int gpio_regmap_set_with_clear(struct gpio_chip *chip,
 				      unsigned int offset, int val)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
-	unsigned int base, reg, mask;
+	unsigned int base, reg, mask, wren_mask;
 	int ret;
 
 	if (val)
@@ -131,11 +160,15 @@ static int gpio_regmap_set_with_clear(struct gpio_chip *chip,
 	else
 		base = gpio_regmap_addr(gpio->reg_clr_base);
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
 	if (ret)
 		return ret;
 
-	return regmap_write(gpio->regmap, reg, mask);
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WITH_CLEAR_OP, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	return regmap_write(gpio->regmap, reg, mask | wren_mask);
 }
 
 static int gpio_regmap_get_direction(struct gpio_chip *chip,
@@ -167,7 +200,7 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
 		return -ENOTSUPP;
 	}
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_DIR_OP, base, offset, &reg, &mask);
 	if (ret)
 		return ret;
 
@@ -185,7 +218,7 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
 				     unsigned int offset, bool output)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
-	unsigned int base, val, reg, mask;
+	unsigned int base, val, reg, mask, wren_mask;
 	int invert, ret;
 
 	if (gpio->reg_dir_out_base) {
@@ -198,7 +231,12 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
 		return -ENOTSUPP;
 	}
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_OP, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_WREN_OP, base, offset, &reg,
+				   &wren_mask);
 	if (ret)
 		return ret;
 
@@ -207,7 +245,7 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
 	else
 		val = output ? mask : 0;
 
-	return regmap_update_bits(gpio->regmap, reg, mask, val);
+	return regmap_update_bits(gpio->regmap, reg, mask | wren_mask, val | wren_mask);
 }
 
 static int gpio_regmap_direction_input(struct gpio_chip *chip,
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index f76a9e08f39e..e8a6921bc443 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -561,21 +561,41 @@ static int ad4111_openwire_event(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
-			     unsigned int offset, unsigned int *reg,
+static int ad7173_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
+			     unsigned int base, unsigned int offset, unsigned int *reg,
 			     unsigned int *mask)
 {
-	*mask = AD7173_GPO_DATA(offset);
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		return 0;
+	default:
+		*mask = AD7173_GPO_DATA(offset);
+		break;
+	}
+
 	*reg = base;
+
 	return 0;
 }
 
-static int ad4111_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
-			     unsigned int offset, unsigned int *reg,
+static int ad4111_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
+			     unsigned int base, unsigned int offset, unsigned int *reg,
 			     unsigned int *mask)
 {
-	*mask = AD4111_GPO01_DATA(offset);
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		break;
+	default:
+		*mask = AD4111_GPO01_DATA(offset);
+		break;
+	}
+
 	*reg = base;
+
 	return 0;
 }
 
diff --git a/drivers/iio/addac/stx104.c b/drivers/iio/addac/stx104.c
index 7bdf2cb94176..a4e54ed102a2 100644
--- a/drivers/iio/addac/stx104.c
+++ b/drivers/iio/addac/stx104.c
@@ -349,16 +349,25 @@ static const struct iio_chan_spec stx104_channels_diff[] = {
 	STX104_IN_CHAN(6, 1), STX104_IN_CHAN(7, 1)
 };
 
-static int stx104_reg_mask_xlate(struct gpio_regmap *const gpio, const unsigned int base,
-				 unsigned int offset, unsigned int *const reg,
-				 unsigned int *const mask)
+static int stx104_reg_mask_xlate(struct gpio_regmap *const gpio, enum gpio_regmap_operation op,
+				 const unsigned int base, unsigned int offset,
+				 unsigned int *const reg, unsigned int *const mask)
 {
 	/* Output lines are located at same register bit offsets as input lines */
 	if (offset >= 4)
 		offset -= 4;
 
 	*reg = base;
-	*mask = BIT(offset);
+
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		break;
+	default:
+		*mask = BIT(offset);
+		break;
+	}
 
 	return 0;
 }
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm63xx.c b/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
index 59d2ce8462d8..3a868deb7793 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
@@ -20,6 +20,7 @@
 #define BCM63XX_DATA_REG	0x0c
 
 static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
+				  enum gpio_regmap_operation op,
 				  unsigned int base, unsigned int offset,
 				  unsigned int *reg, unsigned int *mask)
 {
@@ -27,7 +28,16 @@ static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
 	unsigned int stride = offset / BCM63XX_BANK_GPIOS;
 
 	*reg = base - stride * BCM63XX_BANK_SIZE;
-	*mask = BIT(line);
+
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		break;
+	default:
+		*mask = BIT(line);
+		break;
+	}
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-tps6594.c b/drivers/pinctrl/pinctrl-tps6594.c
index 6726853110d1..e30a856712af 100644
--- a/drivers/pinctrl/pinctrl-tps6594.c
+++ b/drivers/pinctrl/pinctrl-tps6594.c
@@ -347,12 +347,22 @@ static struct tps6594_pinctrl tps6594_template_pinctrl = {
 };
 
 static int tps6594_gpio_regmap_xlate(struct gpio_regmap *gpio,
+				     enum gpio_regmap_operation op,
 				     unsigned int base, unsigned int offset,
 				     unsigned int *reg, unsigned int *mask)
 {
 	unsigned int line = offset % 8;
 	unsigned int stride = offset / 8;
 
+	switch (op) {
+	case GPIO_REGMAP_SET_WREN_OP:
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = 0;
+		return 0;
+	default:
+		break;
+	}
+
 	switch (base) {
 	case TPS6594_REG_GPIOX_CONF(0):
 		*reg = TPS6594_REG_GPIOX_CONF(offset);
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index e4a95f805a81..519fc81add8a 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -13,6 +13,43 @@ struct regmap;
 #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1))
 #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
 
+/**
+ * enum gpio_regmap_operation - Operation type for reg_mask_xlate callback
+ *
+ * This enum is used to distinguish between different types of GPIO operations
+ * so that the reg_mask_xlate callback can return the appropriate mask for each
+ * operation type.
+ *
+ * Value operations:
+ * @GPIO_REGMAP_GET_OP: Mask for reading direction to detect if GPIO is input or output.
+ *                      Used in gpio_regmap_get() to determine the GPIO direction.
+ * @GPIO_REGMAP_IN: Mask for reading input value. Used when GPIO is configured as input.
+ * @GPIO_REGMAP_OUT: Mask for reading output value. Used when GPIO is configured as output.
+ *
+ * Output operations:
+ * @GPIO_REGMAP_SET_OP: Mask for setting GPIO output value.
+ * @GPIO_REGMAP_SET_WITH_CLEAR_OP: Mask for setting/clearing GPIO using separate registers.
+ * @GPIO_REGMAP_SET_WREN_OP: Write-enable mask for output operations. May be used to enable
+ *                           writes to protected registers.
+ *
+ * Direction operations:
+ * @GPIO_REGMAP_GET_DIR_OP: Mask for reading GPIO direction (input/output).
+ * @GPIO_REGMAP_SET_DIR_OP: Mask for setting GPIO direction (input/output).
+ * @GPIO_REGMAP_SET_DIR_WREN_OP: Write-enable mask for direction operations. May be used to
+ *                               enable writes to protected direction registers.
+ */
+enum gpio_regmap_operation {
+	GPIO_REGMAP_GET_OP,
+	GPIO_REGMAP_SET_OP,
+	GPIO_REGMAP_SET_WITH_CLEAR_OP,
+	GPIO_REGMAP_SET_WREN_OP,
+	GPIO_REGMAP_GET_DIR_OP,
+	GPIO_REGMAP_SET_DIR_OP,
+	GPIO_REGMAP_SET_DIR_WREN_OP,
+	GPIO_REGMAP_IN,
+	GPIO_REGMAP_OUT,
+};
+
 /**
  * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
  * @parent:		The parent device
@@ -97,9 +134,9 @@ struct gpio_regmap_config {
 	unsigned long regmap_irq_flags;
 #endif
 
-	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
-			      unsigned int offset, unsigned int *reg,
-			      unsigned int *mask);
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio, enum gpio_regmap_operation,
+			      unsigned int base, unsigned int offset,
+			      unsigned int *reg, unsigned int *mask);
 
 	int (*init_valid_mask)(struct gpio_chip *gc,
 			       unsigned long *valid_mask,
-- 
2.34.1

Re: [PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[Andy Shevchenko]

On Tue, May 12, 2026 at 11:33:13AM +0800, Yu-Chun Lin wrote:
> Extend the reg_mask_xlate callback with an operation type parameter
> (gpio_regmap_operation) to allow drivers to return different
> register/mask combinations for different GPIO operations.
> 
> Also add write-enable mechanism for hardware that requires setting a
> write-enable bit before modifying GPIO control registers.
> 
> Consequently, update all existing drivers utilizing the gpio-regmap
> framework (across drivers/gpio, drivers/iio, and drivers/pinctrl)
> to accommodate the new reg_mask_xlate function signature.

Dunno if we want per-driver patches (in that case it will be a new name and
callback, conversion per driver, removal old name, and, if required, renaming
back). In any case looks reasonable change.

...

> -static int idi_48_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
> -				 unsigned int offset, unsigned int *reg,
> -				 unsigned int *mask)
> +static int idi_48_reg_mask_xlate(struct gpio_regmap *gpio,
> +				 enum gpio_regmap_operation op,
> +				 unsigned int base, unsigned int offset,
> +				 unsigned int *reg, unsigned int *mask)

In every case, use this logical split.

...

> -static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
> -				unsigned int offset, unsigned int *reg,
> +static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
> +				unsigned int base, unsigned int offset, unsigned int *reg,
>  				unsigned int *mask)

Exempli gratia, this one looks illogical, harder to read.

...

> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_WREN_OP, base, offset, &reg,
> +				   &wren_mask);

Ditto. Easier to follow when

	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_WREN_OP, base, offset,
				   &reg, &wren_mask);

>  	if (ret)
>  		return ret;

...

> +/**
> + * enum gpio_regmap_operation - Operation type for reg_mask_xlate callback
> + *
> + * This enum is used to distinguish between different types of GPIO operations
> + * so that the reg_mask_xlate callback can return the appropriate mask for each
> + * operation type.
> + *
> + * Value operations:

Have you checked the rendered text (HTML, PDF)? I believe this will look awfully wrong.

> + * @GPIO_REGMAP_GET_OP: Mask for reading direction to detect if GPIO is input or output.
> + *                      Used in gpio_regmap_get() to determine the GPIO direction.
> + * @GPIO_REGMAP_IN: Mask for reading input value. Used when GPIO is configured as input.
> + * @GPIO_REGMAP_OUT: Mask for reading output value. Used when GPIO is configured as output.
> + *
> + * Output operations:
> + * @GPIO_REGMAP_SET_OP: Mask for setting GPIO output value.
> + * @GPIO_REGMAP_SET_WITH_CLEAR_OP: Mask for setting/clearing GPIO using separate registers.
> + * @GPIO_REGMAP_SET_WREN_OP: Write-enable mask for output operations. May be used to enable
> + *                           writes to protected registers.
> + *
> + * Direction operations:
> + * @GPIO_REGMAP_GET_DIR_OP: Mask for reading GPIO direction (input/output).
> + * @GPIO_REGMAP_SET_DIR_OP: Mask for setting GPIO direction (input/output).
> + * @GPIO_REGMAP_SET_DIR_WREN_OP: Write-enable mask for direction operations. May be used to
> + *                               enable writes to protected direction registers.
> + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[Jonathan Cameron]

On Tue, 12 May 2026 11:33:13 +0800
Yu-Chun Lin <eleanor.lin@realtek.com> wrote:

> Extend the reg_mask_xlate callback with an operation type parameter
> (gpio_regmap_operation) to allow drivers to return different
> register/mask combinations for different GPIO operations.
> 
> Also add write-enable mechanism for hardware that requires setting a
> write-enable bit before modifying GPIO control registers.
> 
> Consequently, update all existing drivers utilizing the gpio-regmap
> framework (across drivers/gpio, drivers/iio, and drivers/pinctrl)
> to accommodate the new reg_mask_xlate function signature.

What is the reasoning behind setting *mask = 0 for unsupported operations?
If it is a special value why not just return an error code to indicate
not supported?  This seems to come from the assumption that you will want
to | that with masks for another operation.

I'm coming into this late but to me there look to be a bunch of undocumented
assumptions. Why do the operations have to share a register for example?

Perhaps an interface where you provide a single operation for write_enable
and whatever else and hence t here is only one 'reg' would work better?


> 
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>

> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index deb9eebb58de..c76eef20e412 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -38,9 +38,10 @@ struct gpio_regmap {
>  	struct regmap_irq_chip_data *irq_chip_data;
>  #endif
>  
> -	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
> -			      unsigned int offset, unsigned int *reg,
> -			      unsigned int *mask);
> +	int (*reg_mask_xlate)(struct gpio_regmap *gpio,
> +			      enum gpio_regmap_operation op,
> +			      unsigned int base, unsigned int offset,
> +			      unsigned int *reg, unsigned int *mask);
>  
>  	void *driver_data;
>  };
> @@ -54,6 +55,7 @@ static unsigned int gpio_regmap_addr(unsigned int addr)
>  }
>  
>  static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
> +				    enum gpio_regmap_operation op,
>  				    unsigned int base, unsigned int offset,
>  				    unsigned int *reg, unsigned int *mask)
>  {
> @@ -61,7 +63,16 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
>  	unsigned int stride = offset / gpio->ngpio_per_reg;
>  
>  	*reg = base + stride * gpio->reg_stride;
> -	*mask = BIT(line);
> +
> +	switch (op) {
> +	case GPIO_REGMAP_SET_WREN_OP:
> +	case GPIO_REGMAP_SET_DIR_WREN_OP:
> +		*mask = 0;
> +		break;
> +	default:
> +		*mask = BIT(line);
> +		break;
> +	}
>  
>  	return 0;
>  }
> @@ -69,7 +80,7 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
>  static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
> -	unsigned int base, val, reg, mask;
> +	unsigned int base, val, reg, mask, dir_mask;
>  	int ret;
>  
>  	/* we might not have an output register if we are input only */
> @@ -78,10 +89,24 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
>  	else
>  		base = gpio_regmap_addr(gpio->reg_set_base);
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_OP, base, offset, &reg, &dir_mask);
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_read(gpio->regmap, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & dir_mask) {
> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_OUT, base, offset, &reg, &mask);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_IN, base, offset, &reg, &mask);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* ensure we don't spoil any register cache with pin input values */
>  	if (gpio->reg_dat_base == gpio->reg_set_base)
>  		ret = regmap_read_bypassed(gpio->regmap, reg, &val);
> @@ -98,10 +123,14 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
>  	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> -	unsigned int reg, mask, mask_val;
> +	unsigned int reg, mask, mask_val, wren_mask;
>  	int ret;
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base, offset, &reg, &mask);
>  	if (ret)
>  		return ret;
>  
> @@ -112,9 +141,9 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  
>  	/* ignore input values which shadow the old output value */
>  	if (gpio->reg_dat_base == gpio->reg_set_base)
> -		ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
> +		ret = regmap_write_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
>  	else
> -		ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
> +		ret = regmap_update_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
>  
>  	return ret;
>  }
> @@ -123,7 +152,7 @@ static int gpio_regmap_set_with_clear(struct gpio_chip *chip,
>  				      unsigned int offset, int val)
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
> -	unsigned int base, reg, mask;
> +	unsigned int base, reg, mask, wren_mask;
>  	int ret;
>  
>  	if (val)
> @@ -131,11 +160,15 @@ static int gpio_regmap_set_with_clear(struct gpio_chip *chip,
>  	else
>  		base = gpio_regmap_addr(gpio->reg_clr_base);
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
>  	if (ret)
>  		return ret;
>  
> -	return regmap_write(gpio->regmap, reg, mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WITH_CLEAR_OP, base, offset, &reg, &mask);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(gpio->regmap, reg, mask | wren_mask);
>  }
>  
>  static int gpio_regmap_get_direction(struct gpio_chip *chip,
> @@ -167,7 +200,7 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip,
>  		return -ENOTSUPP;
>  	}
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_DIR_OP, base, offset, &reg, &mask);
>  	if (ret)
>  		return ret;
>  
> @@ -185,7 +218,7 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
>  				     unsigned int offset, bool output)
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
> -	unsigned int base, val, reg, mask;
> +	unsigned int base, val, reg, mask, wren_mask;
>  	int invert, ret;
>  
>  	if (gpio->reg_dir_out_base) {
> @@ -198,7 +231,12 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
>  		return -ENOTSUPP;
>  	}
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_OP, base, offset, &reg, &mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_DIR_WREN_OP, base, offset, &reg,
> +				   &wren_mask);

What constrains these two to provide the same value back for reg?
To me it seems like the write enable might well be in a different register.

>  	if (ret)
>  		return ret;
>  
> @@ -207,7 +245,7 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
>  	else
>  		val = output ? mask : 0;
>  
> -	return regmap_update_bits(gpio->regmap, reg, mask, val);
> +	return regmap_update_bits(gpio->regmap, reg, mask | wren_mask, val | wren_mask);
>  }
>  
>  static int gpio_regmap_direction_input(struct gpio_chip *chip,

Re: [PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] gpio_regmap_get() incorrectly uses the physical GPIO data level to determine its direction.
- [High] The new direction check in gpio_regmap_get() spoils the regmap cache with volatile input values.
- [High] The write-enable (WREN) implementation overwrites the register pointer and corrupts hardware registers if WREN and data registers differ.
--

commit aa0f1e735514010e24fe4769039de6cdc0302e5d
Author: Yu-Chun Lin <eleanor.lin@realtek.com>

gpio: regmap: Add gpio_regmap_operation and write-enable support

This commit extends the reg_mask_xlate callback with an operation type
parameter to allow different register and mask combinations per GPIO
operation. It also introduces a write-enable mechanism for hardware
requiring a protection bit before modifying control registers.

> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
[ ... ]
> @@ -78,10 +89,24 @@ static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
>  	else
>  		base = gpio_regmap_addr(gpio->reg_set_base);
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_GET_OP, base, offset, &reg, &dir_mask);
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_read(gpio->regmap, reg, &val);

Could this read operation spoil the regmap cache with volatile input values?

The comment further down in this function mentions bypassing the cache to
avoid spoiling it with pin input values, but calling regmap_read() here
appears to cache the state.

> +	if (ret)
> +		return ret;
> +
> +	if (val & dir_mask) {

Will this incorrectly evaluate the physical GPIO data level as the direction?

Since base is set to the data register base earlier in the function, it
looks like this checks if the data bit is high or low rather than querying
a separate direction register. Would this cause an input pin driven high
to be treated as an output?

> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_OUT, base, offset, &reg, &mask);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_IN, base, offset, &reg, &mask);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* ensure we don't spoil any register cache with pin input values */
>  	if (gpio->reg_dat_base == gpio->reg_set_base)
>  		ret = regmap_read_bypassed(gpio->regmap, reg, &val);
[ ... ]
> @@ -98,10 +123,14 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  {
>  	struct gpio_regmap *gpio = gpiochip_get_data(chip);
>  	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> -	unsigned int reg, mask, mask_val;
> +	unsigned int reg, mask, mask_val, wren_mask;
>  	int ret;
>  
> -	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_WREN_OP, base, offset, &reg, &wren_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = gpio->reg_mask_xlate(gpio, GPIO_REGMAP_SET_OP, base, offset, &reg, &mask);
>  	if (ret)
>  		return ret;

Is it possible for the second reg_mask_xlate() call to overwrite the
register pointer if the write-enable register is different from the data
register? Both calls pass the same reg pointer, which might result in the
first queried address being lost.

>  
> @@ -112,9 +141,9 @@ static int gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
>  
>  	/* ignore input values which shadow the old output value */
>  	if (gpio->reg_dat_base == gpio->reg_set_base)
> -		ret = regmap_write_bits(gpio->regmap, reg, mask, mask_val);
> +		ret = regmap_write_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
>  	else
> -		ret = regmap_update_bits(gpio->regmap, reg, mask, mask_val);
> +		ret = regmap_update_bits(gpio->regmap, reg, mask | wren_mask, mask_val | wren_mask);
>  
>  	return ret;
>  }

Does applying both masks at the same time contradict the mechanism described
in the commit message?

The message states the hardware requires setting the write-enable bit before
modifying GPIO control registers, but this writes both bits simultaneously.
Additionally, if the write-enable mask was intended for a different register,
could this bitwise OR apply the combined masks to the wrong register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=3

Re: [PATCH v3 3/7] gpio: regmap: Add gpio_regmap_operation and write-enable support

[Linus Walleij]

Hi Yu-Chun,

thanks for your patch! I really like the direction this is taking.

On Tue, May 12, 2026 at 5:33 AM Yu-Chun Lin <eleanor.lin@realtek.com> wrote:

> Extend the reg_mask_xlate callback with an operation type parameter
> (gpio_regmap_operation) to allow drivers to return different
> register/mask combinations for different GPIO operations.
>
> Also add write-enable mechanism for hardware that requires setting a
> write-enable bit before modifying GPIO control registers.
>
> Consequently, update all existing drivers utilizing the gpio-regmap
> framework (across drivers/gpio, drivers/iio, and drivers/pinctrl)
> to accommodate the new reg_mask_xlate function signature.
>
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>

linusw@kernel.org these days.

> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>

The rest of the comments I had Andy, Jonathan and Sashiko has
already pointed out, just hash through it and this will look really
nice in the end.

Yours,
Linus Walleij

[PATCH v3 4/7] gpio: regmap: Add set_config callback

[Yu-Chun Lin]

Add a new set_config callback to struct gpio_regmap_config to allow drivers
to implement hardware-specific configuration such as debounce settings,
or other platform-specific GPIO properties.

Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
 drivers/gpio/gpio-regmap.c  | 2 ++
 include/linux/gpio/regmap.h | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index c76eef20e412..490a35fe8768 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -371,6 +371,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (!gpio->reg_mask_xlate)
 		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
 
+	chip->set_config = config->set_config;
+
 	ret = gpiochip_add_data(chip, gpio);
 	if (ret < 0)
 		goto err_free_bitmap;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 519fc81add8a..0660fd9be928 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -89,6 +89,9 @@ enum gpio_regmap_operation {
  *			domain will be set accordingly.
  * @regmap_irq_line:	(Optional) The IRQ the device uses to signal interrupts.
  * @regmap_irq_flags:	(Optional) The IRQF_ flags to use for the interrupt.
+ * @set_config:		(Optional) Callback for setting GPIO configuration such
+ *			as debounce, drive strength, or other hardware specific
+ *			settings.
  *
  * The ->reg_mask_xlate translates a given base address and GPIO offset to
  * register and mask pair. The base address is one of the given register
@@ -142,6 +145,10 @@ struct gpio_regmap_config {
 			       unsigned long *valid_mask,
 			       unsigned int ngpios);
 
+	int (*set_config)(struct gpio_chip *gc,
+			  unsigned int offset,
+			  unsigned long config);
+
 	void *drvdata;
 };
 
-- 
2.34.1

Re: [PATCH v3 4/7] gpio: regmap: Add set_config callback

[Andy Shevchenko]

On Tue, May 12, 2026 at 11:33:14AM +0800, Yu-Chun Lin wrote:
> Add a new set_config callback to struct gpio_regmap_config to allow drivers
> to implement hardware-specific configuration such as debounce settings,
> or other platform-specific GPIO properties.

Okay, if it's needed...
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/7] gpio: regmap: Add set_config callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `set_config` callback leaks the underlying `struct gpio_chip` to the driver, breaking the abstraction boundary provided by `gpio_regmap`.
--

commit ac512057e733370619e59d76ffbee25d7ea97d9f
Author: Yu-Chun Lin <eleanor.lin@realtek.com>

gpio: regmap: Add set_config callback

This commit adds a new set_config callback to struct gpio_regmap_config
to allow drivers to implement hardware-specific configurations like
debounce settings or other platform-specific GPIO properties.

> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
[ ... ]
> @@ -142,6 +145,10 @@ struct gpio_regmap_config {
>  			       unsigned long *valid_mask,
>  			       unsigned int ngpios);
>  
> +	int (*set_config)(struct gpio_chip *gc,
> +			  unsigned int offset,
> +			  unsigned long config);
> +
>  	void *drvdata;
>  };

Does exposing struct gpio_chip directly to the driver break the
abstraction boundary provided by gpio_regmap?

The gpio_regmap library appears designed to abstract away struct gpio_chip
from generic regmap-based drivers. For instance, the reg_mask_xlate
callback takes a struct gpio_regmap pointer rather than the raw chip.

Because gpio_regmap sets its own internal structure as the private data
of the gpio_chip during registration via gpiochip_add_data(), drivers cannot
simply access their private data using gpiochip_get_data(). They would
instead be forced to use a lookup chain like
gpio_regmap_get_drvdata(gpiochip_get_data(gc)).

Would it be better to have the callback in struct gpio_regmap_config take
a struct gpio_regmap pointer instead?

> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
[ ... ]
> @@ -371,6 +371,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>  	if (!gpio->reg_mask_xlate)
>  		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
>  
> +	chip->set_config = config->set_config;
> +
>  	ret = gpiochip_add_data(chip, gpio);
>  	if (ret < 0)

If the struct gpio_regmap_config callback is updated to use struct
gpio_regmap, could an internal wrapper function be added here in
gpio-regmap.c?

The wrapper could implement the struct gpio_chip set_config signature,
handle the translation from struct gpio_chip to struct gpio_regmap, and then
invoke the driver's config->set_config callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=4

[PATCH v3 5/7] dt-bindings: gpio: realtek: Add realtek,rtd1625-gpio

[Yu-Chun Lin]

From: Tzuyi Chang <tychang@realtek.com>

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

The RTD1625 GPIO controller features a per-pin register architecture
that differs significantly from previous generations. It utilizes
separate register blocks for GPIO configuration and interrupt control.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Tzuyi Chang <tychang@realtek.com>
Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
 .../bindings/gpio/realtek,rtd1625-gpio.yaml   | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml
new file mode 100644
index 000000000000..f13c910b73c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml
@@ -0,0 +1,71 @@
+# 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,rtd1625-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek DHC RTD1625 GPIO controller
+
+maintainers:
+  - Tzuyi Chang <tychang@realtek.com>
+
+description: |
+  GPIO controller for the Realtek RTD1625 SoC, featuring a per-pin register
+  architecture that differs significantly from earlier RTD series controllers.
+  Each GPIO has dedicated registers for configuration (direction, input/output
+  values, debounce), and interrupt control supporting edge and level detection
+  modes.
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtd1625-iso-gpio
+      - realtek,rtd1625-isom-gpio
+
+  reg:
+    maxItems: 1
+
+  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.
+      - description: Interrupt number of the level-sensitive GPIO interrupt,
+                     triggered by a configured logic level.
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - gpio-ranges
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio@89100 {
+      compatible = "realtek,rtd1625-isom-gpio";
+      reg = <0x89100 0x30>;
+      interrupt-parent = <&iso_m_irq_mux>;
+      interrupts = <0>, <1>, <2>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      gpio-ranges = <&isom_pinctrl 0 0 4>;
+      gpio-controller;
+      #gpio-cells = <2>;
+    };
-- 
2.34.1

Re: [PATCH v3 5/7] dt-bindings: gpio: realtek: Add realtek,rtd1625-gpio

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing required properties for interrupt controller capabilities.
- [Low] Mismatch between the commit message description of register blocks and the `reg` property constraint.
--

commit e48cb449a1787cbc350f702042fd90d1f1e83047
Author: Tzuyi Chang <tychang@realtek.com>

dt-bindings: gpio: realtek: Add realtek,rtd1625-gpio

This commit introduces device tree bindings for the Realtek DHC RTD1625 GPIO
controllers. It notes a per-pin register architecture and includes properties
for configuring pins and handling interrupts.

> Add the device tree bindings for the Realtek DHC (Digital Home Center)
> RTD1625 GPIO controllers.
> 
> The RTD1625 GPIO controller features a per-pin register architecture
> that differs significantly from previous generations. It utilizes
> separate register blocks for GPIO configuration and interrupt control.

The commit message states the controller utilizes separate register blocks
for GPIO configuration and interrupt control. However, the schema constrains
the reg property to maxItems: 1 and the example uses a single contiguous
block. 

If these are truly separate memory regions, should the reg property be
updated to define multiple items? If they are internal register groups
within the same memory-mapped region, could the commit message be clarified
to reflect this?

> diff --git a/Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml b/Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml
> new file mode 100644
> index 0000000000000..f13c910b73c69
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/realtek,rtd1625-gpio.yaml

[ ... ]

> +required:
> +  - compatible
> +  - reg
> +  - gpio-ranges
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false

The binding defines interrupt-controller and #interrupt-cells properties
earlier in the file, and the example demonstrates their use.

Do these properties need to be included in this required list? Since the
hardware acts as an interrupt controller provider, adding them to the
required list would prevent incomplete device tree nodes from silently
passing schema validation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=5

[PATCH v3 6/7] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

[Yu-Chun Lin]

From: Tzuyi Chang <tychang@realtek.com>

Add support for the GPIO controller found on Realtek DHC RTD1625 SoCs.

Unlike the existing Realtek GPIO driver (drivers/gpio/gpio-rtd.c),
which manages pins via shared bank registers, the RTD1625 introduces
a per-pin register architecture. Each GPIO line now has its own
dedicated 32-bit control register to manage configuration independently,
including direction, output value, input value, interrupt enable, and
debounce. Therefore, this distinct hardware design requires a separate
driver.

The driver leverages the gpio-regmap framework, utilizing the recently
introduced write-enable and operation-specific mask translation features.

Additionally, because the controller utilizes multiple independent IRQ
status registers (assert, de-assert, and level) which cannot be mapped
via standard regmap_irq_chip, the driver implements a custom irq_domain.

Signed-off-by: Tzuyi Chang <tychang@realtek.com>
Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek.com>
Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
 drivers/gpio/Kconfig        |  13 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-rtd1625.c | 608 ++++++++++++++++++++++++++++++++++++
 3 files changed, 622 insertions(+)
 create mode 100644 drivers/gpio/gpio-rtd1625.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 504b4bdd75d4..4449b288dfd5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -647,6 +647,19 @@ config GPIO_RTD
 	  Say yes here to support GPIO functionality and GPIO interrupt on
 	  Realtek DHC SoCs.
 
+config GPIO_RTD1625
+	tristate "Realtek DHC RTD1625 GPIO support"
+	depends on ARCH_REALTEK || COMPILE_TEST
+	default ARCH_REALTEK
+	select GPIOLIB_IRQCHIP
+	select GPIO_REGMAP
+	help
+	  This option enables support for the GPIO controller on Realtek
+	  DHC (Digital Home Center) RTD1625 SoC.
+
+	  Say yes here to support both basic GPIO line functionality
+	  and GPIO interrupt handling capabilities for this platform.
+
 config GPIO_SAMA5D2_PIOBU
 	tristate "SAMA5D2 PIOBU GPIO support"
 	depends on OF
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index b267598b517d..d837061d2df7 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -158,6 +158,7 @@ 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_GPIO_RTD1625)		+= gpio-rtd1625.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-rtd1625.c b/drivers/gpio/gpio-rtd1625.c
new file mode 100644
index 000000000000..0eae4bb5577d
--- /dev/null
+++ b/drivers/gpio/gpio-rtd1625.c
@@ -0,0 +1,608 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Realtek DHC RTD1625 gpio driver
+ *
+ * Copyright (c) 2023-2026 Realtek Semiconductor Corp.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#define RTD1625_GPIO_DIR BIT(0)
+#define RTD1625_GPIO_OUT BIT(2)
+#define RTD1625_GPIO_IN BIT(4)
+#define RTD1625_GPIO_EDGE_INT_DP BIT(6)
+#define RTD1625_GPIO_EDGE_INT_EN BIT(8)
+#define RTD1625_GPIO_LEVEL_INT_EN BIT(16)
+#define RTD1625_GPIO_LEVEL_INT_DP BIT(18)
+#define RTD1625_GPIO_DEBOUNCE GENMASK(30, 28)
+#define RTD1625_GPIO_DEBOUNCE_WREN BIT(31)
+
+#define RTD1625_GPIO_WREN(x) ((x) << 1)
+
+/* Write-enable masks for all GPIO configs and reserved hardware bits */
+#define RTD1625_ISO_GPIO_WREN_ALL 0x8000aa8a
+#define RTD1625_ISOM_GPIO_WREN_ALL 0x800aaa8a
+
+#define RTD1625_GPIO_DEBOUNCE_1US 0
+#define RTD1625_GPIO_DEBOUNCE_10US 1
+#define RTD1625_GPIO_DEBOUNCE_100US 2
+#define RTD1625_GPIO_DEBOUNCE_1MS 3
+#define RTD1625_GPIO_DEBOUNCE_10MS 4
+#define RTD1625_GPIO_DEBOUNCE_20MS 5
+#define RTD1625_GPIO_DEBOUNCE_30MS 6
+#define RTD1625_GPIO_DEBOUNCE_50MS 7
+
+#define GPIO_CONTROL(gpio) ((gpio) * 4)
+
+/**
+ * struct rtd1625_gpio_info - Specific GPIO register information
+ * @num_gpios: The number of GPIOs
+ * @irq_type_support: Supported IRQ types
+ * @gpa_offset: Offset for GPIO assert interrupt status registers
+ * @gpda_offset: Offset for GPIO deassert interrupt status registers
+ * @level_offset: Offset of level interrupt status register
+ * @write_en_all: Write-enable mask for all configurable bits
+ */
+struct rtd1625_gpio_info {
+	unsigned int num_gpios;
+	unsigned int irq_type_support;
+	unsigned int base_offset;
+	unsigned int gpa_offset;
+	unsigned int gpda_offset;
+	unsigned int level_offset;
+	unsigned int write_en_all;
+};
+
+struct rtd1625_gpio {
+	struct gpio_chip *gpio_chip;
+	const struct rtd1625_gpio_info *info;
+	void __iomem *base;
+	struct regmap *regmap;
+	unsigned int irqs[3];
+	raw_spinlock_t lock;
+	struct irq_domain *domain;
+	unsigned int *save_regs;
+};
+
+static unsigned int rtd1625_gpio_gpa_offset(struct rtd1625_gpio *data, unsigned int offset)
+{
+	return data->info->gpa_offset + ((offset / 32) * 4);
+}
+
+static unsigned int rtd1625_gpio_gpda_offset(struct rtd1625_gpio *data, unsigned int offset)
+{
+	return data->info->gpda_offset + ((offset / 32) * 4);
+}
+
+static unsigned int rtd1625_gpio_level_offset(struct rtd1625_gpio *data, unsigned int offset)
+{
+	return data->info->level_offset + ((offset / 32) * 4);
+}
+
+static int rtd1625_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
+				  unsigned int base, unsigned int offset, unsigned int *reg,
+				  unsigned int *mask)
+{
+	/* Each GPIO has its own dedicated 32-bit register */
+	*reg = base + offset * 4;
+
+	switch (op) {
+	case GPIO_REGMAP_IN:
+		*mask = RTD1625_GPIO_IN;
+		break;
+	case GPIO_REGMAP_OUT:
+		*mask = RTD1625_GPIO_OUT;
+		break;
+	case GPIO_REGMAP_SET_WREN_OP:
+		*mask = RTD1625_GPIO_WREN(RTD1625_GPIO_OUT);
+		break;
+	case GPIO_REGMAP_SET_WITH_CLEAR_OP:
+	case GPIO_REGMAP_SET_OP:
+		*mask = RTD1625_GPIO_OUT;
+		break;
+	case GPIO_REGMAP_SET_DIR_WREN_OP:
+		*mask = RTD1625_GPIO_WREN(RTD1625_GPIO_DIR);
+		break;
+	case GPIO_REGMAP_GET_OP:
+	case GPIO_REGMAP_GET_DIR_OP:
+		*mask = RTD1625_GPIO_DIR;
+		break;
+	default:
+		*mask = 0;
+		break;
+	}
+
+	return 0;
+}
+
+static unsigned int rtd1625_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
+					      unsigned int debounce)
+{
+	struct rtd1625_gpio *data = gpiochip_get_data(chip);
+	u8 deb_val;
+	u32 val;
+
+	switch (debounce) {
+	case 1:
+		deb_val = RTD1625_GPIO_DEBOUNCE_1US;
+		break;
+	case 10:
+		deb_val = RTD1625_GPIO_DEBOUNCE_10US;
+		break;
+	case 100:
+		deb_val = RTD1625_GPIO_DEBOUNCE_100US;
+		break;
+	case 1000:
+		deb_val = RTD1625_GPIO_DEBOUNCE_1MS;
+		break;
+	case 10000:
+		deb_val = RTD1625_GPIO_DEBOUNCE_10MS;
+		break;
+	case 20000:
+		deb_val = RTD1625_GPIO_DEBOUNCE_20MS;
+		break;
+	case 30000:
+		deb_val = RTD1625_GPIO_DEBOUNCE_30MS;
+		break;
+	case 50000:
+		deb_val = RTD1625_GPIO_DEBOUNCE_50MS;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	val = FIELD_PREP(RTD1625_GPIO_DEBOUNCE, deb_val) | RTD1625_GPIO_DEBOUNCE_WREN;
+	regmap_write(data->regmap, GPIO_CONTROL(offset), val);
+
+	return 0;
+}
+
+static int rtd1625_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				   unsigned long config)
+{
+	int debounce;
+
+	if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) {
+		debounce = pinconf_to_config_argument(config);
+		return rtd1625_gpio_set_debounce(chip, offset, debounce);
+	}
+
+	return gpiochip_generic_config(chip, offset, config);
+}
+
+static void rtd1625_gpio_irq_handle(struct irq_desc *desc)
+{
+	unsigned int (*get_reg_offset)(struct rtd1625_gpio *gpio, unsigned int offset);
+	struct rtd1625_gpio *data = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int irq = irq_desc_get_irq(desc);
+	struct irq_domain *domain = data->domain;
+	unsigned int reg_offset, i, j, val;
+	irq_hw_number_t hwirq;
+	unsigned long status;
+	unsigned int girq;
+	u32 irq_type;
+
+	if (irq == data->irqs[0])
+		get_reg_offset = &rtd1625_gpio_gpa_offset;
+	else if (irq == data->irqs[1])
+		get_reg_offset = &rtd1625_gpio_gpda_offset;
+	else if (irq == data->irqs[2])
+		get_reg_offset = &rtd1625_gpio_level_offset;
+	else
+		return;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < data->info->num_gpios; i += 32) {
+		reg_offset = get_reg_offset(data, i);
+		regmap_read(data->regmap, reg_offset, &val);
+
+		status = val;
+
+		/* Clear edge interrupts; level interrupts are cleared in ->irq_ack() */
+		if (irq != data->irqs[2])
+			regmap_write(data->regmap, reg_offset, status);
+
+		for_each_set_bit(j, &status, 32) {
+			hwirq = i + j;
+			girq = irq_find_mapping(domain, hwirq);
+			irq_type = irq_get_trigger_type(girq);
+
+			if (irq == data->irqs[1] && irq_type != IRQ_TYPE_EDGE_BOTH)
+				continue;
+
+			generic_handle_domain_irq(domain, hwirq);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static void rtd1625_gpio_ack_irq(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 irq_type = irqd_get_trigger_type(d);
+	u32 bit_mask = BIT(hwirq % 32);
+	struct rtd1625_gpio *data;
+	struct gpio_regmap *gpio;
+	int reg_offset;
+
+	gpio = gpiochip_get_data(gc);
+	data = gpio_regmap_get_drvdata(gpio);
+
+	if (irq_type & IRQ_TYPE_LEVEL_MASK) {
+		reg_offset = rtd1625_gpio_level_offset(data, hwirq);
+		regmap_write(data->regmap, reg_offset, bit_mask);
+	}
+}
+
+static void rtd1625_gpio_enable_edge_irq(struct rtd1625_gpio *data, irq_hw_number_t hwirq)
+{
+	int gpda_reg_offset = rtd1625_gpio_gpda_offset(data, hwirq);
+	int gpa_reg_offset = rtd1625_gpio_gpa_offset(data, hwirq);
+	u32 clr_mask = BIT(hwirq % 32);
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&data->lock);
+	regmap_write(data->regmap, gpa_reg_offset, clr_mask);
+	regmap_write(data->regmap, gpda_reg_offset, clr_mask);
+	val = RTD1625_GPIO_EDGE_INT_EN | RTD1625_GPIO_WREN(RTD1625_GPIO_EDGE_INT_EN);
+	regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
+}
+
+static void rtd1625_gpio_disable_edge_irq(struct rtd1625_gpio *data, irq_hw_number_t hwirq)
+{
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&data->lock);
+	val = RTD1625_GPIO_WREN(RTD1625_GPIO_EDGE_INT_EN);
+	regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
+}
+
+static void rtd1625_gpio_enable_level_irq(struct rtd1625_gpio *data, irq_hw_number_t hwirq)
+{
+	int level_reg_offset = rtd1625_gpio_level_offset(data, hwirq);
+	u32 clr_mask = BIT(hwirq % 32);
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&data->lock);
+	regmap_write(data->regmap, level_reg_offset, clr_mask);
+	val = RTD1625_GPIO_LEVEL_INT_EN | RTD1625_GPIO_WREN(RTD1625_GPIO_LEVEL_INT_EN);
+	regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
+}
+
+static void rtd1625_gpio_disable_level_irq(struct rtd1625_gpio *data, irq_hw_number_t hwirq)
+{
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&data->lock);
+	val = RTD1625_GPIO_WREN(RTD1625_GPIO_LEVEL_INT_EN);
+	regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
+}
+
+static void rtd1625_gpio_enable_irq(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 irq_type = irqd_get_trigger_type(d);
+	struct rtd1625_gpio *data;
+	struct gpio_regmap *gpio;
+
+	gpio = gpiochip_get_data(gc);
+	data = gpio_regmap_get_drvdata(gpio);
+
+	gpiochip_enable_irq(gc, hwirq);
+
+	if (irq_type & IRQ_TYPE_EDGE_BOTH)
+		rtd1625_gpio_enable_edge_irq(data, hwirq);
+	else if (irq_type & IRQ_TYPE_LEVEL_MASK)
+		rtd1625_gpio_enable_level_irq(data, hwirq);
+}
+
+static void rtd1625_gpio_disable_irq(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 irq_type = irqd_get_trigger_type(d);
+	struct rtd1625_gpio *data;
+	struct gpio_regmap *gpio;
+
+	gpio = gpiochip_get_data(gc);
+	data = gpio_regmap_get_drvdata(gpio);
+
+	if (irq_type & IRQ_TYPE_EDGE_BOTH)
+		rtd1625_gpio_disable_edge_irq(data, hwirq);
+	else if (irq_type & IRQ_TYPE_LEVEL_MASK)
+		rtd1625_gpio_disable_level_irq(data, hwirq);
+
+	gpiochip_disable_irq(gc, hwirq);
+}
+
+static int rtd1625_gpio_irq_set_level_type(struct irq_data *d, bool level)
+{
+	u32 val = RTD1625_GPIO_WREN(RTD1625_GPIO_LEVEL_INT_DP);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct rtd1625_gpio *data;
+	struct gpio_regmap *gpio;
+
+	gpio = gpiochip_get_data(gc);
+	data = gpio_regmap_get_drvdata(gpio);
+	if (!(data->info->irq_type_support & IRQ_TYPE_LEVEL_MASK))
+		return -EINVAL;
+
+	scoped_guard(raw_spinlock_irqsave, &data->lock) {
+		if (level)
+			val |= RTD1625_GPIO_LEVEL_INT_DP;
+		regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
+	}
+
+	irq_set_handler_locked(d, handle_level_irq);
+
+	return 0;
+}
+
+static int rtd1625_gpio_irq_set_edge_type(struct irq_data *d, bool polarity)
+{
+	u32 val = RTD1625_GPIO_WREN(RTD1625_GPIO_EDGE_INT_DP);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct rtd1625_gpio *data;
+	struct gpio_regmap *gpio;
+
+	gpio = gpiochip_get_data(gc);
+	data = gpio_regmap_get_drvdata(gpio);
+	if (!(data->info->irq_type_support & IRQ_TYPE_EDGE_BOTH))
+		return -EINVAL;
+
+	scoped_guard(raw_spinlock_irqsave, &data->lock) {
+		if (polarity)
+			val |= RTD1625_GPIO_EDGE_INT_DP;
+		regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
+	}
+
+	irq_set_handler_locked(d, handle_edge_irq);
+
+	return 0;
+}
+
+static int rtd1625_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	int ret;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		ret = rtd1625_gpio_irq_set_edge_type(d, 1);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		ret = rtd1625_gpio_irq_set_edge_type(d, 0);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		ret = rtd1625_gpio_irq_set_edge_type(d, 1);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		ret = rtd1625_gpio_irq_set_level_type(d, 0);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		ret = rtd1625_gpio_irq_set_level_type(d, 1);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static struct irq_chip rtd1625_iso_gpio_irq_chip = {
+	.name = "rtd1625-gpio",
+	.irq_ack = rtd1625_gpio_ack_irq,
+	.irq_mask = rtd1625_gpio_disable_irq,
+	.irq_unmask = rtd1625_gpio_enable_irq,
+	.irq_set_type = rtd1625_gpio_irq_set_type,
+	.flags = IRQCHIP_IMMUTABLE | IRQCHIP_SKIP_SET_WAKE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int rtd1625_gpio_setup_irq(struct platform_device *pdev, struct rtd1625_gpio *data)
+{
+	int num_irqs, irq, i;
+
+	irq = platform_get_irq_optional(pdev, 0);
+	if (irq == -ENXIO)
+		return 0;
+	if (irq < 0)
+		return irq;
+
+	num_irqs = (data->info->irq_type_support & IRQ_TYPE_LEVEL_MASK) ? 3 : 2;
+
+	for (i = 0; i < num_irqs; i++) {
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0)
+			return irq;
+
+		data->irqs[i] = irq;
+		irq_set_chained_handler_and_data(data->irqs[i], rtd1625_gpio_irq_handle, data);
+	}
+
+	return 0;
+}
+
+static int rtd1625_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
+				irq_hw_number_t hwirq)
+{
+	struct rtd1625_gpio *data = domain->host_data;
+
+	irq_set_chip_data(irq, data->gpio_chip);
+
+	irq_set_chip_and_handler(irq, &rtd1625_iso_gpio_irq_chip, handle_bad_irq);
+
+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops rtd1625_gpio_irq_domain_ops = {
+	.map = rtd1625_gpio_irq_map,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static const struct regmap_config rtd1625_gpio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.disable_locking = true,
+};
+
+static int rtd1625_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_regmap_config config = {0};
+	struct device *dev = &pdev->dev;
+	struct gpio_regmap *gpio_reg;
+	struct rtd1625_gpio *data;
+	void __iomem *irq_base;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->info = device_get_match_data(dev);
+	if (!data->info)
+		return -EINVAL;
+
+	irq_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(irq_base))
+		return PTR_ERR(irq_base);
+
+	data->regmap = devm_regmap_init_mmio(dev, irq_base,
+					     &rtd1625_gpio_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	data->save_regs = devm_kzalloc(dev, data->info->num_gpios *
+				       sizeof(*data->save_regs), GFP_KERNEL);
+	if (!data->save_regs)
+		return -ENOMEM;
+
+	config.parent = dev;
+	config.regmap = data->regmap;
+	config.ngpio = data->info->num_gpios;
+	config.reg_dat_base = data->info->base_offset;
+	config.reg_set_base = data->info->base_offset;
+	config.reg_mask_xlate = rtd1625_reg_mask_xlate;
+	config.set_config = rtd1625_gpio_set_config;
+	config.reg_dir_out_base = data->info->base_offset;
+
+	data->domain = irq_domain_add_linear(dev->of_node,
+					     data->info->num_gpios,
+					     &rtd1625_gpio_irq_domain_ops,
+					     data);
+	if (!data->domain)
+		return -ENOMEM;
+
+	ret = rtd1625_gpio_setup_irq(pdev, data);
+	if (ret) {
+		irq_domain_remove(data->domain);
+		return ret;
+	}
+
+	config.irq_domain = data->domain;
+	config.drvdata = data;
+	platform_set_drvdata(pdev, data);
+
+	gpio_reg = devm_gpio_regmap_register(dev, &config);
+	if (IS_ERR(gpio_reg)) {
+		irq_domain_remove(data->domain);
+		return PTR_ERR(gpio_reg);
+	}
+
+	data->gpio_chip = gpio_regmap_get_gpiochip(gpio_reg);
+
+	return 0;
+}
+
+static const struct rtd1625_gpio_info rtd1625_iso_gpio_info = {
+	.num_gpios		= 166,
+	.irq_type_support	= IRQ_TYPE_EDGE_BOTH,
+	.base_offset		= 0x100,
+	.gpa_offset		= 0x0,
+	.gpda_offset		= 0x20,
+	.write_en_all		= RTD1625_ISO_GPIO_WREN_ALL,
+};
+
+static const struct rtd1625_gpio_info rtd1625_isom_gpio_info = {
+	.num_gpios		= 4,
+	.irq_type_support	= IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_LOW |
+				  IRQ_TYPE_LEVEL_HIGH,
+	.base_offset		= 0x20,
+	.gpa_offset		= 0x0,
+	.gpda_offset		= 0x4,
+	.level_offset		= 0x18,
+	.write_en_all		= RTD1625_ISOM_GPIO_WREN_ALL,
+};
+
+static const struct of_device_id rtd1625_gpio_of_matches[] = {
+	{ .compatible = "realtek,rtd1625-iso-gpio", .data = &rtd1625_iso_gpio_info },
+	{ .compatible = "realtek,rtd1625-isom-gpio", .data = &rtd1625_isom_gpio_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rtd1625_gpio_of_matches);
+
+static int rtd1625_gpio_suspend(struct device *dev)
+{
+	struct rtd1625_gpio *data = dev_get_drvdata(dev);
+	const struct rtd1625_gpio_info *info = data->info;
+	int i;
+
+	for (i = 0; i < info->num_gpios; i++)
+		regmap_read(data->regmap, data->info->base_offset + GPIO_CONTROL(i),
+			    &data->save_regs[i]);
+
+	return 0;
+}
+
+static int rtd1625_gpio_resume(struct device *dev)
+{
+	struct rtd1625_gpio *data = dev_get_drvdata(dev);
+	const struct rtd1625_gpio_info *info = data->info;
+	int i;
+
+	for (i = 0; i < info->num_gpios; i++)
+		regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(i),
+			     data->save_regs[i] | info->write_en_all);
+
+	return 0;
+}
+
+DEFINE_NOIRQ_DEV_PM_OPS(rtd1625_gpio_pm_ops, rtd1625_gpio_suspend, rtd1625_gpio_resume);
+
+static struct platform_driver rtd1625_gpio_platform_driver = {
+	.driver = {
+		.name = "gpio-rtd1625",
+		.of_match_table = rtd1625_gpio_of_matches,
+		.pm = pm_sleep_ptr(&rtd1625_gpio_pm_ops),
+	},
+	.probe = rtd1625_gpio_probe,
+};
+module_platform_driver(rtd1625_gpio_platform_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Realtek Semiconductor Corporation");
+MODULE_DESCRIPTION("Realtek DHC SoC RTD1625 gpio driver");
-- 
2.34.1

Re: [PATCH v3 6/7] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

[Andy Shevchenko]

On Tue, May 12, 2026 at 11:33:16AM +0800, Yu-Chun Lin wrote:

> Add support for the GPIO controller found on Realtek DHC RTD1625 SoCs.

The below paragraphs are all the implementation details that are not so
important to be in the commit message. They fit the comment block after
the '---' line below, though...

> Unlike the existing Realtek GPIO driver (drivers/gpio/gpio-rtd.c),
> which manages pins via shared bank registers, the RTD1625 introduces
> a per-pin register architecture. Each GPIO line now has its own
> dedicated 32-bit control register to manage configuration independently,
> including direction, output value, input value, interrupt enable, and
> debounce. Therefore, this distinct hardware design requires a separate
> driver.
> 
> The driver leverages the gpio-regmap framework, utilizing the recently
> introduced write-enable and operation-specific mask translation features.
> 
> Additionally, because the controller utilizes multiple independent IRQ
> status registers (assert, de-assert, and level) which cannot be mapped
> via standard regmap_irq_chip, the driver implements a custom irq_domain.
> 
> Signed-off-by: Tzuyi Chang <tychang@realtek.com>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> ---

...somewhere here.

...


> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>

Missing cleanup.h, may be more.

> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>

> +#include <linux/irqdomain.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>

If you sort headers alphabetically, you will see the duplication.

> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>

...

> +struct rtd1625_gpio {
> +	struct gpio_chip *gpio_chip;
> +	const struct rtd1625_gpio_info *info;

> +	void __iomem *base;

Is it used? Also naming is confusing with unsigned int base in some functions
below.

> +	struct regmap *regmap;
> +	unsigned int irqs[3];

This 3 is also used when assign num_irqs below, perhaps define it with
meaningful name? (The 2 also might need a definition in the same way.)

> +	raw_spinlock_t lock;
> +	struct irq_domain *domain;
> +	unsigned int *save_regs;
> +};

...

> +static int rtd1625_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
> +				  unsigned int base, unsigned int offset, unsigned int *reg,
> +				  unsigned int *mask)

Use logical split.

static int rtd1625_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
				  unsigned int base, unsigned int offset,
				  unsigned int *reg, unsigned int *mask)

OR (as suggested earlier)

static int rtd1625_reg_mask_xlate(struct gpio_regmap *gpio,
				  enum gpio_regmap_operation op,
				  unsigned int base, unsigned int offset,
				  unsigned int *reg, unsigned int *mask)

...

> +static unsigned int rtd1625_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +					      unsigned int debounce)
> +{
> +	struct rtd1625_gpio *data = gpiochip_get_data(chip);
> +	u8 deb_val;
> +	u32 val;
> +
> +	switch (debounce) {
> +	case 1:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_1US;
> +		break;
> +	case 10:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_10US;
> +		break;
> +	case 100:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_100US;
> +		break;
> +	case 1000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_1MS;
> +		break;
> +	case 10000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_10MS;
> +		break;
> +	case 20000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_20MS;
> +		break;
> +	case 30000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_30MS;
> +		break;
> +	case 50000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_50MS;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	val = FIELD_PREP(RTD1625_GPIO_DEBOUNCE, deb_val) | RTD1625_GPIO_DEBOUNCE_WREN;

> +	regmap_write(data->regmap, GPIO_CONTROL(offset), val);

Neither here, nor in some other places the returned value from regmap APIs is
not checked. Is it okay?

> +	return 0;
> +}
> +
> +static int rtd1625_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
> +				   unsigned long config)
> +{
> +	int debounce;

Can it be negative?

> +	if (pinconf_to_config_param(config) == PIN_CONFIG_INPUT_DEBOUNCE) {
> +		debounce = pinconf_to_config_argument(config);
> +		return rtd1625_gpio_set_debounce(chip, offset, debounce);
> +	}
> +
> +	return gpiochip_generic_config(chip, offset, config);
> +}

...

> +static void rtd1625_gpio_irq_handle(struct irq_desc *desc)
> +{
> +	unsigned int (*get_reg_offset)(struct rtd1625_gpio *gpio, unsigned int offset);
> +	struct rtd1625_gpio *data = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	struct irq_domain *domain = data->domain;
> +	unsigned int reg_offset, i, j, val;
> +	irq_hw_number_t hwirq;
> +	unsigned long status;
> +	unsigned int girq;
> +	u32 irq_type;
> +
> +	if (irq == data->irqs[0])
> +		get_reg_offset = &rtd1625_gpio_gpa_offset;
> +	else if (irq == data->irqs[1])
> +		get_reg_offset = &rtd1625_gpio_gpda_offset;
> +	else if (irq == data->irqs[2])
> +		get_reg_offset = &rtd1625_gpio_level_offset;
> +	else
> +		return;
> +
> +	chained_irq_enter(chip, desc);

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

	for (unsigned int i = 0; i < data->info->num_gpios; i += 32) {

> +		reg_offset = get_reg_offset(data, i);
> +		regmap_read(data->regmap, reg_offset, &val);
> +
> +		status = val;
> +
> +		/* Clear edge interrupts; level interrupts are cleared in ->irq_ack() */
> +		if (irq != data->irqs[2])
> +			regmap_write(data->regmap, reg_offset, status);

This is interesting... Can't we rely on IRQ core to take care of this? (And
replay if required.)

> +		for_each_set_bit(j, &status, 32) {
> +			hwirq = i + j;
> +			girq = irq_find_mapping(domain, hwirq);
> +			irq_type = irq_get_trigger_type(girq);

> +			if (irq == data->irqs[1] && irq_type != IRQ_TYPE_EDGE_BOTH)

Why is the IRQ type check here and not in set_type() callback?
I think we should simply reject to even lock such a configuration as IRQ.

> +				continue;
> +
> +			generic_handle_domain_irq(domain, hwirq);
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}

> +static void rtd1625_gpio_enable_edge_irq(struct rtd1625_gpio *data, irq_hw_number_t hwirq)
> +{
> +	int gpda_reg_offset = rtd1625_gpio_gpda_offset(data, hwirq);
> +	int gpa_reg_offset = rtd1625_gpio_gpa_offset(data, hwirq);
> +	u32 clr_mask = BIT(hwirq % 32);
> +	u32 val;
> +
> +	guard(raw_spinlock_irqsave)(&data->lock);

+ blank line. Same to other cases when guard()() is being used.

> +	regmap_write(data->regmap, gpa_reg_offset, clr_mask);
> +	regmap_write(data->regmap, gpda_reg_offset, clr_mask);
> +	val = RTD1625_GPIO_EDGE_INT_EN | RTD1625_GPIO_WREN(RTD1625_GPIO_EDGE_INT_EN);
> +	regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
> +}

...

> +static void rtd1625_gpio_enable_irq(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 irq_type = irqd_get_trigger_type(d);
> +	struct rtd1625_gpio *data;
> +	struct gpio_regmap *gpio;

> +	gpio = gpiochip_get_data(gc);
> +	data = gpio_regmap_get_drvdata(gpio);

Why not unify these with the above, we don't validate them anyway...
Same Q to other cases like this.

> +	gpiochip_enable_irq(gc, hwirq);
> +
> +	if (irq_type & IRQ_TYPE_EDGE_BOTH)
> +		rtd1625_gpio_enable_edge_irq(data, hwirq);
> +	else if (irq_type & IRQ_TYPE_LEVEL_MASK)
> +		rtd1625_gpio_enable_level_irq(data, hwirq);
> +}

...

> +static int rtd1625_gpio_irq_set_level_type(struct irq_data *d, bool level)
> +{
> +	u32 val = RTD1625_GPIO_WREN(RTD1625_GPIO_LEVEL_INT_DP);
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	struct rtd1625_gpio *data;
> +	struct gpio_regmap *gpio;
> +
> +	gpio = gpiochip_get_data(gc);
> +	data = gpio_regmap_get_drvdata(gpio);

+ blank line (and actually see above).

> +	if (!(data->info->irq_type_support & IRQ_TYPE_LEVEL_MASK))
> +		return -EINVAL;

> +	scoped_guard(raw_spinlock_irqsave, &data->lock) {

> +		if (level)
> +			val |= RTD1625_GPIO_LEVEL_INT_DP;

Why this local variable change is under the lock?

> +		regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(hwirq), val);
> +	}
> +
> +	irq_set_handler_locked(d, handle_level_irq);
> +
> +	return 0;
> +}

...

> +static int rtd1625_gpio_irq_set_edge_type(struct irq_data *d, bool polarity)

Similar comments as per above.

...

> +static int rtd1625_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	int ret;

Unneeded, just return directly from each of the cases.

> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		ret = rtd1625_gpio_irq_set_edge_type(d, 1);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		ret = rtd1625_gpio_irq_set_edge_type(d, 0);
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		ret = rtd1625_gpio_irq_set_edge_type(d, 1);
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		ret = rtd1625_gpio_irq_set_level_type(d, 0);
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		ret = rtd1625_gpio_irq_set_level_type(d, 1);
> +		break;
> +	default:
> +		ret = -EINVAL;

Missing break, but rather simply

		return -EINVAL;

as mentioned above. In the similar way for the rest of switch-cases.

> +	}
> +
> +	return ret;
> +}

...

> +static int rtd1625_gpio_setup_irq(struct platform_device *pdev, struct rtd1625_gpio *data)
> +{
> +	int num_irqs, irq, i;

Why is 'num_irqs' signed?

> +	irq = platform_get_irq_optional(pdev, 0);
> +	if (irq == -ENXIO)
> +		return 0;

This is strange. You mean that you are only interested in the first IRQ? What
about the case when the first one is not available, while the second is?

> +	if (irq < 0)
> +		return irq;

This partially duplicates the below.

> +	num_irqs = (data->info->irq_type_support & IRQ_TYPE_LEVEL_MASK) ? 3 : 2;

> +	for (i = 0; i < num_irqs; i++) {

The iterator is local to the loop, hence

	for (unsigned int i = 0; i < num_irqs; i++) {

> +		irq = platform_get_irq(pdev, i);
> +		if (irq < 0)
> +			return irq;
> +
> +		data->irqs[i] = irq;
> +		irq_set_chained_handler_and_data(data->irqs[i], rtd1625_gpio_irq_handle, data);
> +	}
> +
> +	return 0;
> +}

...

> +static int rtd1625_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct rtd1625_gpio *data = domain->host_data;
> +
> +	irq_set_chip_data(irq, data->gpio_chip);
> +
> +	irq_set_chip_and_handler(irq, &rtd1625_iso_gpio_irq_chip, handle_bad_irq);
> +
> +	irq_set_noprobe(irq);

Shouldn't you assign a lockdep class?

> +	return 0;
> +}

...

> +static const struct regmap_config rtd1625_gpio_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,

> +	.disable_locking = true,

Hmm... This effectively drops the debugfs support, perhaps make it depend on
CONFIG_DEBUG_GPIO?

> +};

...

> +static int rtd1625_gpio_probe(struct platform_device *pdev)
> +{
> +	struct gpio_regmap_config config = {0};

'0' is redundant.

> +	struct device *dev = &pdev->dev;
> +	struct gpio_regmap *gpio_reg;
> +	struct rtd1625_gpio *data;
> +	void __iomem *irq_base;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->info = device_get_match_data(dev);
> +	if (!data->info)
> +		return -EINVAL;
> +
> +	irq_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(irq_base))
> +		return PTR_ERR(irq_base);
> +
> +	data->regmap = devm_regmap_init_mmio(dev, irq_base,
> +					     &rtd1625_gpio_regmap_config);

I would go with a single line

	data->regmap = devm_regmap_init_mmio(dev, irq_base, &rtd1625_gpio_regmap_config);

> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);
> +
> +	data->save_regs = devm_kzalloc(dev, data->info->num_gpios *
> +				       sizeof(*data->save_regs), GFP_KERNEL);

This is devm_kcalloc().

> +	if (!data->save_regs)
> +		return -ENOMEM;
> +
> +	config.parent = dev;
> +	config.regmap = data->regmap;
> +	config.ngpio = data->info->num_gpios;
> +	config.reg_dat_base = data->info->base_offset;
> +	config.reg_set_base = data->info->base_offset;
> +	config.reg_mask_xlate = rtd1625_reg_mask_xlate;
> +	config.set_config = rtd1625_gpio_set_config;
> +	config.reg_dir_out_base = data->info->base_offset;
> +
> +	data->domain = irq_domain_add_linear(dev->of_node,

No, use fwnode variant from the start.

> +					     data->info->num_gpios,
> +					     &rtd1625_gpio_irq_domain_ops,
> +					     data);
> +	if (!data->domain)
> +		return -ENOMEM;
> +
> +	ret = rtd1625_gpio_setup_irq(pdev, data);
> +	if (ret) {
> +		irq_domain_remove(data->domain);
> +		return ret;
> +	}
> +
> +	config.irq_domain = data->domain;
> +	config.drvdata = data;
> +	platform_set_drvdata(pdev, data);
> +
> +	gpio_reg = devm_gpio_regmap_register(dev, &config);
> +	if (IS_ERR(gpio_reg)) {

> +		irq_domain_remove(data->domain);

This is wrong. The remove will do the opposite order. It means that
irq_domain_create_linear() has to be wrapped to become a managed resource.

> +		return PTR_ERR(gpio_reg);
> +	}
> +
> +	data->gpio_chip = gpio_regmap_get_gpiochip(gpio_reg);
> +
> +	return 0;
> +}
> +
> +static const struct rtd1625_gpio_info rtd1625_iso_gpio_info = {
> +	.num_gpios		= 166,
> +	.irq_type_support	= IRQ_TYPE_EDGE_BOTH,
> +	.base_offset		= 0x100,
> +	.gpa_offset		= 0x0,
> +	.gpda_offset		= 0x20,

Use fixed-width for the register offsets, id est 0x000, 0x020, and so on...

> +	.write_en_all		= RTD1625_ISO_GPIO_WREN_ALL,
> +};
> +
> +static const struct rtd1625_gpio_info rtd1625_isom_gpio_info = {
> +	.num_gpios		= 4,
> +	.irq_type_support	= IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_LOW |
> +				  IRQ_TYPE_LEVEL_HIGH,
> +	.base_offset		= 0x20,
> +	.gpa_offset		= 0x0,
> +	.gpda_offset		= 0x4,
> +	.level_offset		= 0x18,
> +	.write_en_all		= RTD1625_ISOM_GPIO_WREN_ALL,
> +};

...

> +static const struct of_device_id rtd1625_gpio_of_matches[] = {
> +	{ .compatible = "realtek,rtd1625-iso-gpio", .data = &rtd1625_iso_gpio_info },
> +	{ .compatible = "realtek,rtd1625-isom-gpio", .data = &rtd1625_isom_gpio_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rtd1625_gpio_of_matches);

Move the initialiser and respective MODULE_DEVICE_TABLE() closer to its user.

...

> +static int rtd1625_gpio_suspend(struct device *dev)
> +{
> +	struct rtd1625_gpio *data = dev_get_drvdata(dev);
> +	const struct rtd1625_gpio_info *info = data->info;
> +	int i;
> +
> +	for (i = 0; i < info->num_gpios; i++)

	for (unsigned int i = 0; i < info->num_gpios; i++)

> +		regmap_read(data->regmap, data->info->base_offset + GPIO_CONTROL(i),
> +			    &data->save_regs[i]);
> +
> +	return 0;
> +}
> +
> +static int rtd1625_gpio_resume(struct device *dev)
> +{
> +	struct rtd1625_gpio *data = dev_get_drvdata(dev);
> +	const struct rtd1625_gpio_info *info = data->info;
> +	int i;
> +
> +	for (i = 0; i < info->num_gpios; i++)

Ditto.

> +		regmap_write(data->regmap, data->info->base_offset + GPIO_CONTROL(i),
> +			     data->save_regs[i] | info->write_en_all);
> +
> +	return 0;
> +}

...

> +DEFINE_NOIRQ_DEV_PM_OPS(rtd1625_gpio_pm_ops, rtd1625_gpio_suspend, rtd1625_gpio_resume);

Not static? Why?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/7] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Uninitialized raw spinlock `data->lock` triggers a BUG/panic.
- [High] Chained IRQ handlers and IRQ domain are leaked on probe failure and driver unbind, causing Use-After-Free.
- [High] Type mismatch in `rtd1625_gpio_set_debounce()` causes memory corruption and panic.
- [High] Missing base offset in `rtd1625_gpio_set_debounce()` corrupts interrupt status registers.
- [High] Race condition in IRQ mapping leads to a NULL pointer dereference.
- [High] Incorrect hardware mask in `GPIO_REGMAP_GET_OP` breaks GPIO input value reading.
- [Medium] Unprotected state modifications via `gpiochip_disable_irq()` in `.irq_mask`.
--

commit 2806fbf586d5ca5b8f36cb1b33138f23dd2f5b19
Author: Tzuyi Chang <tychang@realtek.com>

gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

Add support for the GPIO controller found on Realtek DHC RTD1625 SoCs.

> diff --git a/drivers/gpio/gpio-rtd1625.c b/drivers/gpio/gpio-rtd1625.c
> new file mode 100644
> index 0000000000000..0eae4bb5577d9
> --- /dev/null
> +++ b/drivers/gpio/gpio-rtd1625.c

[ ... ]

> +static int rtd1625_reg_mask_xlate(struct gpio_regmap *gpio, enum gpio_regmap_operation op,
> +				  unsigned int base, unsigned int offset, unsigned int *reg,
> +				  unsigned int *mask)
> +{
> +	/* Each GPIO has its own dedicated 32-bit register */
> +	*reg = base + offset * 4;
> +
> +	switch (op) {

[ ... ]

> +	case GPIO_REGMAP_SET_DIR_WREN_OP:
> +		*mask = RTD1625_GPIO_WREN(RTD1625_GPIO_DIR);
> +		break;
> +	case GPIO_REGMAP_GET_OP:
> +	case GPIO_REGMAP_GET_DIR_OP:
> +		*mask = RTD1625_GPIO_DIR;

GPIO_REGMAP_GET_OP is mapped to RTD1625_GPIO_DIR. Will this return the
direction configuration instead of the actual physical logic level of the pin
(RTD1625_GPIO_IN) when calling gpio_get_value()?

> +		break;
> +	default:
> +		*mask = 0;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned int rtd1625_gpio_set_debounce(struct gpio_chip *chip, unsigned int offset,
> +					      unsigned int debounce)
> +{
> +	struct rtd1625_gpio *data = gpiochip_get_data(chip);

gpiochip_get_data(chip) returns a pointer to struct gpio_regmap when using the
gpio-regmap framework, not struct rtd1625_gpio.

Will dereferencing data->regmap later in this function cause a crash? Should
this use gpio_regmap_get_drvdata(gpiochip_get_data(chip)) like the other
functions?

> +	u8 deb_val;
> +	u32 val;
> +
> +	switch (debounce) {

[ ... ]

> +	case 50000:
> +		deb_val = RTD1625_GPIO_DEBOUNCE_50MS;
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	val = FIELD_PREP(RTD1625_GPIO_DEBOUNCE, deb_val) | RTD1625_GPIO_DEBOUNCE_WREN;
> +	regmap_write(data->regmap, GPIO_CONTROL(offset), val);

Is data->info->base_offset missing from the register calculation here?

Writing to GPIO_CONTROL(offset) without the base offset will overwrite the
gpa_offset and gpda_offset interrupt status registers instead of the target
GPIO control register.

> +
> +	return 0;
> +}

[ ... ]

> +static void rtd1625_gpio_disable_irq(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 irq_type = irqd_get_trigger_type(d);
> +	struct rtd1625_gpio *data;
> +	struct gpio_regmap *gpio;
> +
> +	gpio = gpiochip_get_data(gc);
> +	data = gpio_regmap_get_drvdata(gpio);
> +
> +	if (irq_type & IRQ_TYPE_EDGE_BOTH)
> +		rtd1625_gpio_disable_edge_irq(data, hwirq);
> +	else if (irq_type & IRQ_TYPE_LEVEL_MASK)
> +		rtd1625_gpio_disable_level_irq(data, hwirq);
> +
> +	gpiochip_disable_irq(gc, hwirq);

This function is registered as the .irq_mask callback. Since the core
temporarily masks level-triggered interrupts during processing, will calling
gpiochip_disable_irq() clear GPIOD_FLAG_IRQ_IS_ENABLED?

If so, could this inadvertently allow a concurrent thread to bypass protections
(e.g. gpiod_direction_output()) and change the GPIO direction to output while
the interrupt is actively firing?

> +}

[ ... ]

> +static int rtd1625_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct rtd1625_gpio *data = domain->host_data;
> +
> +	irq_set_chip_data(irq, data->gpio_chip);

Could data->gpio_chip be NULL here?

The IRQ domain is added in rtd1625_gpio_probe() before data->gpio_chip is
populated by gpio_regmap_get_gpiochip(). If an interrupt is mapped
concurrently during probe, setting NULL chip data will cause a NULL pointer
dereference later when retrieving the chip data.

> +
> +	irq_set_chip_and_handler(irq, &rtd1625_iso_gpio_irq_chip, handle_bad_irq);
> +
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}

[ ... ]

> +static int rtd1625_gpio_probe(struct platform_device *pdev)
> +{
> +	struct gpio_regmap_config config = {0};
> +	struct device *dev = &pdev->dev;
> +	struct gpio_regmap *gpio_reg;
> +	struct rtd1625_gpio *data;
> +	void __iomem *irq_base;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;

Is there a missing initialization for data->lock?

Since data is zero-initialized by devm_kzalloc(), acquiring this uninitialized
raw spinlock via guard(raw_spinlock_irqsave)(&data->lock) in the IRQ callbacks
will trigger a kernel panic when lock debugging or PREEMPT_RT is enabled.

> +
> +	data->info = device_get_match_data(dev);
> +	if (!data->info)
> +		return -EINVAL;

[ ... ]

> +	data->domain = irq_domain_add_linear(dev->of_node,
> +					     data->info->num_gpios,
> +					     &rtd1625_gpio_irq_domain_ops,
> +					     data);
> +	if (!data->domain)
> +		return -ENOMEM;
> +
> +	ret = rtd1625_gpio_setup_irq(pdev, data);
> +	if (ret) {
> +		irq_domain_remove(data->domain);
> +		return ret;
> +	}
> +
> +	config.irq_domain = data->domain;
> +	config.drvdata = data;
> +	platform_set_drvdata(pdev, data);
> +
> +	gpio_reg = devm_gpio_regmap_register(dev, &config);
> +	if (IS_ERR(gpio_reg)) {
> +		irq_domain_remove(data->domain);
> +		return PTR_ERR(gpio_reg);
> +	}

If devm_gpio_regmap_register() fails, or when the driver is later unbound
(since there is no .remove callback), data is automatically freed by devres.

However, rtd1625_gpio_setup_irq() configures chained handlers on the parent
IRQs which are not cleaned up. Will this cause a use-after-free if the parent
hardware interrupt fires after cleanup? Also, is the irq_domain leaked on
driver unbind?

> +
> +	data->gpio_chip = gpio_regmap_get_gpiochip(gpio_reg);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=6

[PATCH v3 7/7] arm64: dts: realtek: Add GPIO support for RTD1625

[Yu-Chun Lin]

Add the GPIO node for the Realtek RTD1625 SoC.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
---
The changes are based on commit 856540ac9b441a8c0e39f1f1787277edc4097c9b,
which was merged into the soc/for-next branch.
---
 arch/arm64/boot/dts/realtek/kent.dtsi | 39 +++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/realtek/kent.dtsi b/arch/arm64/boot/dts/realtek/kent.dtsi
index 8d4293cd4c03..228b82dfdb7a 100644
--- a/arch/arm64/boot/dts/realtek/kent.dtsi
+++ b/arch/arm64/boot/dts/realtek/kent.dtsi
@@ -151,6 +151,37 @@ uart0: serial@7800 {
 				status = "disabled";
 			};
 
+			gpio: gpio@31000 {
+				compatible = "realtek,rtd1625-iso-gpio";
+				reg = <0x31000 0x398>;
+				gpio-controller;
+				gpio-ranges = <&isom_pinctrl 0 0 2>,
+					      <&ve4_pinctrl 2 0 6>,
+					      <&iso_pinctrl 8 0 4>,
+					      <&ve4_pinctrl 12 6 2>,
+					      <&main2_pinctrl 14 0 2>,
+					      <&ve4_pinctrl 16 8 4>,
+					      <&main2_pinctrl 20 2 3>,
+					      <&ve4_pinctrl 23 12 3>,
+					      <&iso_pinctrl 26 4 2>,
+					      <&isom_pinctrl 28 2 2>,
+					      <&ve4_pinctrl 30 15 6>,
+					      <&main2_pinctrl 36 5 6>,
+					      <&ve4_pinctrl 42 21 3>,
+					      <&iso_pinctrl 45 6 6>,
+					      <&ve4_pinctrl 51 24 1>,
+					      <&iso_pinctrl 52 12 1>,
+					      <&ve4_pinctrl 53 25 11>,
+					      <&main2_pinctrl 64 11 28>,
+					      <&ve4_pinctrl 92 36 2>,
+					      <&iso_pinctrl 94 13 19>,
+					      <&iso_pinctrl 128 32 4>,
+					      <&ve4_pinctrl 132 38 13>,
+					      <&iso_pinctrl 145 36 19>,
+					      <&ve4_pinctrl 164 51 2>;
+				#gpio-cells = <2>;
+			};
+
 			iso_pinctrl: pinctrl@4e000 {
 				compatible = "realtek,rtd1625-iso-pinctrl";
 				reg = <0x4e000 0x1a4>;
@@ -161,6 +192,14 @@ main2_pinctrl: pinctrl@4f200 {
 				reg = <0x4f200 0x50>;
 			};
 
+			iso_m_gpio: gpio@89100 {
+				compatible = "realtek,rtd1625-isom-gpio";
+				reg = <0x89100 0x30>;
+				gpio-controller;
+				gpio-ranges = <&isom_pinctrl 0 0 4>;
+				#gpio-cells = <2>;
+			};
+
 			isom_pinctrl: pinctrl@146200 {
 				compatible = "realtek,rtd1625-isom-pinctrl";
 				reg = <0x146200 0x34>;
-- 
2.34.1

Re: [PATCH v3 7/7] arm64: dts: realtek: Add GPIO support for RTD1625

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Both the `gpio@31000` and `iso_m_gpio@89100` nodes omit essential interrupt properties (`interrupts`, `interrupt-controller`, and `#interrupt-cells`).
--

commit d2b46f0c89e5920bcae5cf858be075ac05b6c104
Author: Yu-Chun Lin <eleanor.lin@realtek.com>

arm64: dts: realtek: Add GPIO support for RTD1625

Add the GPIO node for the Realtek RTD1625 SoC.

> diff --git a/arch/arm64/boot/dts/realtek/kent.dtsi b/arch/arm64/boot/dts/realtek/kent.dtsi
> index 8d4293cd4c036..228b82dfdb7ac 100644
> --- a/arch/arm64/boot/dts/realtek/kent.dtsi
> +++ b/arch/arm64/boot/dts/realtek/kent.dtsi
> @@ -151,6 +151,37 @@ uart0: serial@7800 {
>  				status = "disabled";
>  			};
>  
> +			gpio: gpio@31000 {
> +				compatible = "realtek,rtd1625-iso-gpio";
> +				reg = <0x31000 0x398>;
> +				gpio-controller;

Should this node include interrupt properties such as "interrupts",
"interrupt-controller", and "#interrupt-cells"?

In drivers/gpio/gpio-rtd1625.c, rtd1625_gpio_probe() unconditionally registers
an irq_domain. Since the "interrupts" property is absent from the device tree,
platform_get_irq_optional() will return -ENXIO and skip registering the
chained parent IRQ handler, but leave the irq_domain active.

If a consumer driver requests an interrupt for one of these GPIOs, the request
will succeed and return a valid software IRQ mapping from the domain. However,
because the parent IRQ was never hooked up, the hardware will never deliver
the interrupt to the CPU, leading to silent failures for peripherals relying
on GPIO interrupts.

[ ... ]
> +			iso_m_gpio: gpio@89100 {
> +				compatible = "realtek,rtd1625-isom-gpio";
> +				reg = <0x89100 0x30>;
> +				gpio-controller;

Are the interrupt properties also missing here for the same reason?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033317.1602537-1-eleanor.lin@realtek.com?part=7