Re: [PATCH v12 09/11] gpio: pl061: switch data path to gpio-regmap

[Andy Shevchenko]

On Tue, Oct 27, 2026 at 12:18:56PM +0800, Long Zhao via B4 Relay wrote:
> 
> Move get, set and direction onto gpio-regmap. ARM PL061 sets
> GPIO_REGMAP_QUIRK_SET_AFTER_DIR so the output value is written
> after the direction change.
> 
> Keep a custom irqchip and irq domain. Create the domain with
> the PL061 private data and let gpio-regmap attach it.
> 
> Leave the chained handler's regmap_read() unchecked. mis stays 0
> on failure, so the pending loop does nothing, and
> chained_irq_exit() still runs. irq_type still ignores MMIO write
> returns.
> 
> The irqchip uses its own child IRQ lockdep class and clears the
> chained handler only after it was installed, checked with lockdep
> on the CV75 board. irq_set_parent() nests
> the GPIO irq_desc under the AMBA parent for irq_set_wake(). The
> gpiochip IRQ resource helpers cannot be kept here: the gpio_chip is
> not reachable through gpio-regmap's external irq_domain path, so
> GPIOD_FLAG_USED_AS_IRQ is never tracked. This matches the other
> users of that path (gpio-idio-16, gpio-pcie-idio-24,
> gpio-104-idi-48).

Something happened to the text formatting, it's quite unbalanced in terms of
occupied room on each of the lines. Make each of the lines to occupy ~72
characters.

...

> +struct pl061_drvdata {
> +	const struct regmap_config *regmap_config;
> +	const struct pl061_regs *regs;
> +	unsigned int ngpio;

Is here any gap (of 4 bytes) in accordance with `pahole`? If so, you better
relocate 'quirks' somewhere to make int and booleans be together.

> +	unsigned long quirks;
> +	bool clear_irq_on_type;
> +	bool pm;
> +	int (*reg_mask_xlate)(struct gpio_regmap *gpio,
> +			      unsigned int base, unsigned int offset,
> +			      unsigned int *reg, unsigned int *mask);
>  };

...

> +static int pl061_irq_domain_map(struct irq_domain *d, unsigned int virq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct pl061 *pl061 = d->host_data;
> +
> +	irq_set_chip_data(virq, pl061);

> +	irq_set_lockdep_class(virq, &pl061_irq_lock_class,
> +			      &pl061_irq_request_class);

Make it a single line.

> +	irq_set_chip_and_handler(virq, &pl061_irq_chip, handle_bad_irq);
> +	irq_set_noprobe(virq);
> +	irq_set_parent(virq, pl061->parent_irq);
> +
> +	return 0;
> +}

...

>  static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  {
> +	struct gpio_regmap_config config = {};
> +	const struct pl061_drvdata *data;
>  	struct device *dev = &adev->dev;
> -	struct gpio_irq_chip *girq;
> +	struct gpio_regmap *gpio_regmap;
> +	const struct pl061_regs *regs;
>  	struct pl061 *pl061;
>  	void __iomem *base;

> -	int ret, irq;
> +	int ret;
> +	int irq;

Unneeded change.

...

> +	if (irq) {
> +		irq_set_chained_handler_and_data(irq, pl061_irq_handler, pl061);

> +		pl061->irq_handler_installed = true;

Can't we remove the chained handler unconditionally?

> +	}

-- 
With Best Regards,
Andy Shevchenko