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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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