Re: [PATCH v7 10/15] gpio: pl061: convert to gpio-regmap and a custom irqchip

[Andy Shevchenko]

On Tue, Sep 15, 2026 at 07:15:40PM +0800, Long Zhao via B4 Relay wrote:
> 
> Move line get/set/direction onto gpio-regmap. Keep the existing ARM
> masked data addresses and write-after-direction behaviour.
> 
> Do not use gpiochip irqchip setup (girq) or regmap-irq. gpio-regmap
> owns gpiochip registration, so girq would have to be plumbed through
> that helper. PL061 IRQ type programming needs IS/IBE/IEV, including
> both-edge, plus a hardirq chained demux from the parent AMBA IRQ;
> regmap-irq is a poor fit for that.
> 
> Create a linear irq_domain with gpio_chip as host data, attach it with
> gpiochip_irqchip_add_domain(), and chain the parent IRQ in this driver.

Besides some comments below about splitting more of this patch to better
isolated logical changes, this one can also be split to a few stages. For
example, one stage is to move to regmap (from direct MMIO access) without
really switching over to gpio-regmap.

...

> +#include <linux/err.h>
>  #include <linux/errno.h>

Now errno.h can be removed as basic errno is provided by err.h.

...

> -#include <linux/irqchip/chained_irq.h>

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

Unneeded churn.

...

> -	if ((trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) &&
> -	    (trigger & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)))
> -	{

> +	if ((trigger & IRQ_TYPE_LEVEL_MASK) && (trigger & IRQ_TYPE_EDGE_BOTH)) {

See below, this belongs to a separate change.

...

> @@ -142,14 +121,19 @@ static int pl061_irq_type(struct irq_data *d, unsigned trigger)
>  		return -EINVAL;
>  	}
>  
> -
>  	raw_spin_lock_irqsave(&pl061->lock, flags);

Stray change. If required, should be in a separate patch.

...

> -	if (trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
> +	if (trigger & IRQ_TYPE_LEVEL_MASK) {

This change should be in a separate patch.

...

> -	writeb(gpiois, pl061->base + GPIOIS);
> -	writeb(gpioibe, pl061->base + GPIOIBE);
> -	writeb(gpioiev, pl061->base + GPIOIEV);
> +	ret = regmap_write(pl061->regmap, regs->is, gpiois);
> +	if (ret)
> +		goto out;
> +	ret = regmap_write(pl061->regmap, regs->ibe, gpioibe);
> +	if (ret)
> +		goto out;
> +	ret = regmap_write(pl061->regmap, regs->iev, gpioiev);
> +	if (ret)
> +		goto out;
> +	if (pl061->data->clear_irq_on_type)
> +		ret = regmap_write(pl061->regmap, regs->ic, bit);
>  
> +out:
>  	raw_spin_unlock_irqrestore(&pl061->lock, flags);
> -
> -	return 0;
> +	return ret;

Make an additional patch to move current driver to use cleanup.h, id est
guard()() and possibly scoped_guard() from there.

Then in this patch those ton's of goto:s should be replaced with simple
'return ret;'.`

>  }

...

> +	if (!regmap_read(pl061->regmap, pl061->data->regs->mis, &mis) && mis) {

Why not handling error properly here?

	ret = regmap_read(...);
	if (ret)
		goto out_irq_exit;

> +		pending = mis;
> +		for_each_set_bit(offset, &pending, pl061->data->ngpio)
> +			generic_handle_domain_irq(pl061->irq_domain, offset);
>  	}

...

> +static int pl061_irq_domain_map(struct irq_domain *d, unsigned int virq,
> +				irq_hw_number_t hwirq)
> +{
> +	struct gpio_chip *gc = d->host_data;
> +	struct pl061 *pl061 = pl061_from_gpio_chip(gc);
> +
> +	irq_set_chip_data(virq, gc);
> +	irq_set_chip_and_handler(virq, &pl061_irqchip, handle_bad_irq);
> +	irq_set_noprobe(virq);
> +	irq_set_parent(virq, pl061->parent_irq);


Is lockdep happy with this?

> +	return 0;
> +}

...

> +static void pl061_remove_irq(void *data)
> +{
> +	struct pl061 *pl061 = data;
> +
> +	irq_set_chained_handler_and_data(pl061->parent_irq, NULL, NULL);
> +
> +	for (unsigned int i = 0; i < pl061->data->ngpio; i++) {

> +		unsigned int virq = irq_find_mapping(pl061->irq_domain, i);

Split assignment

		unsigned int virq;

		virq = irq_find_mapping(pl061->irq_domain, i);
		if (virq)

> +		if (virq)
> +			irq_dispose_mapping(virq);
> +	}
> +
> +	irq_domain_remove(pl061->irq_domain);
> +}

...

>  	struct device *dev = &adev->dev;
> +	const struct pl061_drvdata *data = id->data;

Split assignment as it is validated later on.

> +	const struct pl061_regs *regs;
> +	struct gpio_regmap_config config = {};
> +	struct gpio_regmap *gpio_regmap;
> +	struct gpio_chip *gc;
>  	struct pl061 *pl061;
> -	struct gpio_irq_chip *girq;
> +	void __iomem *base;
>  	int ret, irq;
>  
> +	if (!data)
> +		return -EINVAL;

-ENODATA?

> +	regs = data->regs;

...

> +	for (offset = 0; offset < pl061->data->ngpio; offset++) {
> +		if (!(dir & BIT(offset)))
> +			continue;

for_each_set_bit(), but make sure the type of the variable is unsigned long
(might require a temporary one).

> +		ret = regmap_read_bypassed(pl061->regmap,
> +					   BIT(offset + PL061_DATA_OFFSET),
> +					   &val);
> +		if (ret)
> +			return ret;
> +		pl061->saved_dat |= val;
>  	}

...

> +	for (offset = 0; offset < pl061->data->ngpio; offset++) {
> +		if (!(dir & BIT(offset)))
> +			continue;

Ditto.

> +		ret = regmap_write(pl061->regmap,
> +				   BIT(offset + PL061_DATA_OFFSET),
> +				   !!(pl061->saved_dat & BIT(offset)) << offset);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regcache_sync_region(pl061->regmap, regs->is, regs->ie);
>  }

...

> static const struct amba_id pl061_ids[] = {
>  	{
>  		.id	= 0x00041061,
>  		.mask	= 0x000fffff,
> +		.data	= (void *)&pl061_arm,

Oh, this is something that needs to be fixed (but it's not your issue).
Why on earth this is not const?

>  	},
>  	{ 0, 0 },
>  };

-- 
With Best Regards,
Andy Shevchenko