Re: [PATCH v4 3/4] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

[Linus Walleij]

Hi Yu-Chun,

thanks for your patch!

On Mon, Jun 22, 2026 at 10:33 AM Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx> wrote:

> From: Tzuyi Chang <tychang@xxxxxxxxxxx>
>
> 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.
>
> Additionally, the RTD1625 GPIO controller has a specific hardware quirk:
> it fires both 'assert' and 'de-assert' interrupts simultaneously on any
> edge toggle. To handle this, we utilize the polarity register to route
> the requested edge (rising/falling) to the 'assert' IRQ line. The driver
> then filters out the unwanted 'de-assert' interrupt in the IRQ handler
> and pre-clears edge interrupts to prevent interrupt storms caused by
> unhandled dropped interrupts.
>
> Interrupt support is optional for this device, matching the dt-bindings.
> If the interrupts property is not provided, the driver simply skips IRQ
> initialization and operates purely as a basic GPIO controller.
>
> Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>
> Signed-off-by: Tzuyi Chang <tychang@xxxxxxxxxxx>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
(...)
> +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_domain *domain = data->gpio_chip.irq.domain;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       unsigned int irq = irq_desc_get_irq(desc);
> +       unsigned long status;
> +       unsigned int reg_offset, i, j;
> +       unsigned int girq;

So this

> +       irq_hw_number_t hwirq;
> +       u32 irq_type;
> +
> +       if (irq == data->irqs[RTD1625_IRQ_ASSERT])
> +               get_reg_offset = &rtd1625_gpio_gpa_offset;
> +       else if (irq == data->irqs[RTD1625_IRQ_DEASSERT])
> +               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);
> +               status = readl_relaxed(data->irq_base + reg_offset);
> +
> +               /*
> +                * Hardware quirk: The controller fires both "assert" and "de-assert"
> +                * interrupts simultaneously on any edge toggle.
> +                * We must pre-clear edge interrupts here. If we drop an unwanted
> +                * de-assert interrupt below, it will never reach the IRQ core
> +                * (generic_handle_domain_irq), meaning ->irq_ack() won't be called.
> +                * Failing to clear it here leads to an interrupt storm.
> +                */
> +               if (irq != data->irqs[RTD1625_IRQ_LEVEL])
> +                       writel_relaxed(status, data->irq_base + reg_offset);
> +
> +               for_each_set_bit(j, &status, 32) {
> +                       hwirq = i + j;
> +                       girq = irq_find_mapping(domain, hwirq);
> +                       irq_type = irq_get_trigger_type(girq);

Just
irq_type = irq_get_trigger_type(irq_find_mapping(domain, hwirq));

Drop the intermediate variable.

> +static void rtd1625_gpio_ack_irq(struct irq_data *d)
> +{
> +       struct rtd1625_gpio *data = 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);

This is a clear sign that your GPIOs and IRQs should be three-cell
(bank and offset) since they clearly have one each a separate
status bit in this register.

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

Same here.

> +static int rtd1625_gpio_setup_irq(struct platform_device *pdev, struct rtd1625_gpio *data)
> +{
> +       struct gpio_irq_chip *irq_chip;

This is a super-confusing name for this variable.

It is called irq_chip but it's not struct irq_chip at all.

Call this girq like all other drivers.

Yours,
Linus Walleij