RE: [PATCH v6 8/8] gpio: realtek: Add driver for Realtek DHC RTD1625 SoC

["Yu-Chun Lin [林祐君]" <eleanor.lin@realtek.com>]

Hi Andy,
Thanks for your feedback.

> On Tue, Jul 21, 2026 at 02:58:02PM +0800, Yu-Chun Lin wrote:
> 
> > 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 RTD1625 GPIO controller has a hardware quirk where both 'assert'
> > and 'de-assert' interrupts are fired simultaneously on any edge toggle.
> > The driver works around this quirk to correctly handle edge 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.
> 
> ...
> 
> > +struct rtd1625_gpio_info {
> > +     unsigned int num_gpios;
> 
> Do you need this? Isn't it available via struct gpio_chip?
> 

I need num_gpios here because I need to know the number of GPIOs before the
gpio_chip is registered.

> > +     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;
> > +};
> 
> ...
> 
> > +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_OP:
> > +             *mask = RTD1625_GPIO_OUT;
> > +             break;
> > +
> > +     case GPIO_REGMAP_GET_OP:
> > +     case GPIO_REGMAP_GET_DIR_OP:
> > +     case GPIO_REGMAP_SET_DIR_OP:
> > +             *mask = RTD1625_GPIO_DIR;
> > +             break;
> > +
> > +     default:
> > +             return -ENOTSUPP;
> > +     }
> 
> > +
> > +     return 0;
> 
> Just spread this to each case by replacing break:s.
> 

Ack.

> > +}
> > +
> > +static int rtd1625_value_xlate(struct gpio_regmap *gpio,
> > +                            enum gpio_regmap_operation op,
> > +                            unsigned int base, unsigned int offset,
> > +                            unsigned int reg, unsigned int *mask,
> > +                            unsigned int *val) {
> > +     switch (op) {
> > +     case GPIO_REGMAP_SET_OP:
> > +             *val |= RTD1625_GPIO_WREN(RTD1625_GPIO_OUT);
> > +             *mask |= RTD1625_GPIO_WREN(RTD1625_GPIO_OUT);
> > +             break;
> > +
> > +     case GPIO_REGMAP_SET_DIR_OP:
> > +             *val |= RTD1625_GPIO_WREN(RTD1625_GPIO_DIR);
> > +             *mask |= RTD1625_GPIO_WREN(RTD1625_GPIO_DIR);
> > +             break;
> > +
> > +     default:
> > +             return -ENOTSUPP;
> > +     }
> > +
> > +     return 0;
> 
> Ditto.
> 

Ack.

> > +}
> 
> ...
> 
> > +static 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;
> > +     int ret;
> > +
> > +     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;
> > +
> > +     scoped_guard(raw_spinlock_irqsave, &data->lock)
> > +             ret = regmap_write(data->regmap, data->info->base_offset
> + GPIO_CONTROL(offset),
> > +                                val);
> > +
> > +     return ret;
> 
> guard()() is fine here.
> 
>         val = FIELD_PREP(RTD1625_GPIO_DEBOUNCE, deb_val) |
> RTD1625_GPIO_DEBOUNCE_WREN;
> 
>         guard(raw_spinlock_irqsave)(&data->lock);
> 
>         return regmap_write(data->regmap, data->info->base_offset +
> GPIO_CONTROL(offset), val);
> 

Ack.

> > +}
> 
> ...
> 
> > +static int rtd1625_gpio_irq_set_level_type(struct irq_data *d, bool
> > +level) {
> > +     u32 val = RTD1625_GPIO_WREN(RTD1625_GPIO_LEVEL_INT_DP);
> > +     irq_hw_number_t hwirq = irqd_to_hwirq(d);
> > +     struct rtd1625_gpio *data;
> > +     int ret;
> > +
> > +     data = irq_data_get_irq_chip_data(d);
> > +     if (!(data->info->irq_type_support & IRQ_TYPE_LEVEL_MASK))
> > +             return -EINVAL;
> > +
> > +     if (level)
> > +             val |= RTD1625_GPIO_LEVEL_INT_DP;
> 
> > +     scoped_guard(raw_spinlock_irqsave, &data->lock)
> > +             ret = regmap_write(data->regmap, data->info->base_offset
> > + + GPIO_CONTROL(hwirq), val);
> > +
> > +     if (ret)
> > +             return ret;
> 
> Better to move it inside the scoped_guard() loop.
> 

Ack.

> > +     irq_set_handler_locked(d, handle_level_irq);
> > +
> > +     return 0;
> > +}
> 
> ...
> 
> > +static int rtd1625_gpio_setup_irq(struct platform_device *pdev,
> > +struct rtd1625_gpio *data) {
> > +     unsigned int num_irqs;
> > +     int irq;
> > +
> > +     irq = platform_get_irq_optional(pdev, 0);
> 
> Perhaps a comment to explain that IRQ is (fully) optional?
> 

Ack.

> > +     if (irq == -ENXIO)
> > +             return 0;
> > +     if (irq < 0)
> > +             return irq;
> > +
> > +     num_irqs = (data->info->irq_type_support &
> IRQ_TYPE_LEVEL_MASK)
> > + ? 3 : 2;
> > +
> > +     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_probe(struct platform_device *pdev) {
> > +     struct gpio_regmap_config config = {};
> > +     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;
> 
> -ENODATA
> 

Ack.

> > +     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);
> > +
> > +     raw_spin_lock_init(&data->lock);
> > +     platform_set_drvdata(pdev, data);
> > +
> > +     data->save_regs = devm_kcalloc(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_dir_out_base = data->info->base_offset;
> > +
> > +     config.reg_mask_xlate = rtd1625_reg_mask_xlate;
> > +     config.set_config = rtd1625_gpio_set_config;
> > +     config.value_xlate = rtd1625_value_xlate;
> > +
> > +     data->domain = irq_domain_create_linear(dev_fwnode(dev),
> > +
> data->info->num_gpios,
> > +
> &rtd1625_gpio_irq_domain_ops,
> > +                                             data);
> > +     if (!data->domain)
> > +             return -ENOMEM;
> 
> 
> devm_irq_domain_instantiate()
> 

Ack.

Best Regards,
Yu-Chun

> > +     ret = devm_add_action_or_reset(dev, (void (*)(void
> *))irq_domain_remove, data->domain);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = rtd1625_gpio_setup_irq(pdev, data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     config.irq_domain = data->domain;
> > +     config.drvdata = data;
> > +
> > +     gpio_reg = devm_gpio_regmap_register(dev, &config);
> > +     if (IS_ERR(gpio_reg))
> > +             return PTR_ERR(gpio_reg);
> > +
> > +     data->gpio_reg = gpio_reg;
> > +
> > +     return 0;
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
>