Re: [PATCH v10 7/8] gpio: pl061: switch data path to gpio-regmap

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

On Thu, Oct 15, 2026 at 06:53:34PM +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.
> 
> Check the chained-handler regmap_read() return and always run
> chained_irq_exit(). irq_type still ignores MMIO write returns.
> 
> irq_set_parent() nests the GPIO irq_desc under the AMBA parent
> for irq_set_wake() and lockdep; checked with PROVE_LOCKING on
> CV75.

...

> -#define GPIODIR 0x400
> -#define GPIOIS  0x404
> -#define GPIOIBE 0x408
> -#define GPIOIEV 0x40C
> -#define GPIOIE  0x410
> -#define GPIORIS 0x414
> -#define GPIOMIS 0x418
> -#define GPIOIC  0x41C

> +#define PL061_DIR		0x400
> +#define PL061_IS		0x404
> +#define PL061_IBE		0x408
> +#define PL061_IEV		0x40c
> +#define PL061_IE		0x410
> +#define PL061_MIS		0x418
> +#define PL061_IC		0x41c

If you want this rename, it should be done separately, otherwise it sounds like
an unneeded churn (at least for now).

...

> +struct pl061_drvdata {
> +	const struct regmap_config *regmap_config;
> +	const struct pl061_regs *regs;
> +	unsigned int ngpio;
> +	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);

Split this logically

	int (*reg_mask_xlate)(struct gpio_regmap *gpio,
			      unsigned int base, unsigned int offset,
			      unsigned int *reg, unsigned int *mask);

>  };

...

>  struct pl061 {
>  	raw_spinlock_t		lock;

> +	struct device		*dev;

Why? Can't you derive this from regmap?

>  	struct regmap		*regmap;
> -	struct gpio_chip	gc;
> +	const struct pl061_drvdata *data;
> +	struct irq_domain	*irq_domain;
>  	int			parent_irq;
>  
> -	struct pl061_context_save_regs csave_regs;
> +	u32			saved_dat;
>  };

> -static int pl061_get_direction(struct gpio_chip *gc, unsigned offset)
> -{
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -	int ret;
> -
> -	ret = regmap_test_bits(pl061->regmap, GPIODIR, BIT(offset));
> -	if (ret < 0)
> -		return ret;
> -
> -	if (ret)
> -		return GPIO_LINE_DIRECTION_OUT;
> -
> -	return GPIO_LINE_DIRECTION_IN;
> -}
> -
> -static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
> -{
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -
> -	guard(raw_spinlock_irqsave)(&pl061->lock);
> -
> -	regmap_clear_bits(pl061->regmap, GPIODIR, BIT(offset));
> -
> -	return 0;
> -}
> -
> -static int pl061_direction_output(struct gpio_chip *gc, unsigned offset,
> -		int value)
> -{
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -	unsigned int mask = BIT(offset);
> -
> -	guard(raw_spinlock_irqsave)(&pl061->lock);
> -
> -	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> -			   BIT(offset), value);
> -	regmap_set_bits(pl061->regmap, GPIODIR, mask);
> -
> -	/*
> -	 * gpio value is set again, because pl061 doesn't allow to set value of
> -	 * a gpio pin before configuring it in OUT mode.
> -	 */
> -	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> -			   BIT(offset), value);
> -
> -	return 0;
> -}
> -
> -static int pl061_get_value(struct gpio_chip *gc, unsigned offset)
>  {
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -
> -	return regmap_test_bits(pl061->regmap,
> -				BIT(offset + PL061_DATA_OFFSET),
> -				BIT(offset));
>  }
>  
> -static int pl061_set_value(struct gpio_chip *gc, unsigned int offset, int value)
>  {
> -	struct pl061 *pl061 = gpiochip_get_data(gc);
> -
> -	regmap_assign_bits(pl061->regmap, BIT(offset + PL061_DATA_OFFSET),
> -			   BIT(offset), value);
>  
>  	return 0;
>  }

...

> -		dev_dbg(gc->parent, "line %d: IRQ on both edges\n", offset);
> +		dev_dbg(pl061->dev, "line %d: IRQ on both edges\n", offset);

To avoid this type of churn, introduce a local variable to keep parent

	struct device *parent = gc->parent;

and convert the lines in a separate patch. So this one will have much less
unneeded changes.

...

> +	if (irq) {
> +		pl061->irq_domain = irq_domain_create_linear(dev_fwnode(dev),
> +							     data->ngpio,
> +							     &pl061_irq_domain_ops,
> +							     pl061);
> +		if (!pl061->irq_domain)
> +			return -ENOMEM;
> +	}

We have devm_irq_domain_instantiate(). Can't use it? Why?

...

> +	for_each_set_bit(offset, &dir_mask, pl061->data->ngpio)

> +		regmap_write(pl061->regmap,
> +			     BIT(offset + PL061_DATA_OFFSET),

You have room. Please, check your editor settings and keep in mind that the
strict limit is 80 characters (inclusive), so lines may have 80 characters.
In some cases (when readability may be increased) even slightly more.

> +			     !!(pl061->saved_dat & BIT(offset)) << offset);

This is simply

			     pl061->saved_dat & BIT(offset));

no?

-- 
With Best Regards,
Andy Shevchenko