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

[zl020895]

Hi Andy,

Sorry for the churn and thanks for the review.

Looking back at your v9 review of the convert patch, the only point I
did not follow is the data-register path. You marked those
regmap_write() sites with _set_bits(). I did switch direction and IRQ
enable to regmap_test_bits() / regmap_clear_bits() / regmap_set_bits(),
but I left the data-register writes as regmap_write(), because
regmap_set_bits() cannot clear a bit to drive the line low. 
In the next version I will use regmap_assign_bits()
there.

I did not find the other notes from this mail — drop fast_io, keep the
regmap_config in the convert patch, and the guard() blank lines — in
the v9 comments. I will fix those in the next version as well, along
with reverse Christmas tree in irq_type and a precursor to drop the
redundant if (pending) check.

Best regards,
Long Zhao

At 2026-10-13 15:15:37, "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>On Fri, Oct 09, 2026 at 06:57:59PM +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.
>
>...
>
>> -static bool pl061_is_data_reg(unsigned int reg)
>> -{
>> -	return is_power_of_2(reg) &&
>> -	       reg >= BIT(PL061_DATA_OFFSET) &&
>> -	       reg <= BIT(PL061_DATA_OFFSET + PL061_GPIO_NR - 1);
>> -}
>> -
>> -static bool pl061_volatile_reg(struct device *dev, unsigned int reg)
>> -{
>> -	switch (reg) {
>> -	case GPIOMIS:
>> -	case GPIOIC:
>> -		return true;
>> -	default:
>> -		return pl061_is_data_reg(reg);
>> -	}
>> -}
>> -
>> -static const struct regmap_config pl061_regmap_config = {
>> -	.reg_bits = 16,
>> -	.val_bits = 8,
>> -	.reg_stride = 4,
>> -	.max_register = GPIOIC,
>> -	.volatile_reg = pl061_volatile_reg,
>> -	.fast_io = true,
>> -	.use_raw_spinlock = true,
>> -};
>
>No, I have already told you do not move these. If you need to rename, just make
>sure it's named accordingly in the previous patch that introduced this.
>
>...
>
>I have a feeling that you ignored my last review. I stop it here, you need to
>carefully read *all* the comments and act accordingly.
>
>-- 
>With Best Regards,
>Andy Shevchenko
>