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

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

On Tue, Oct 13, 2026 at 04:56:14PM +0800, zl020895 wrote:
> 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. 

Can you elaborate on this a bit? Even if the cached value is 0, you still want
to write 0, do I understand correctly? Wouldn't it just mean that the registers
should be marked as volatile or something like that?

> In the next version I will use regmap_assign_bits() there.

Hmm... If regmap_set_bits() doesn't work for you, regmap_assign_bits() will not
work either.

> 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.

Yeah, some notes are new, I only referred to the old ones.
But I believe I have mentioned that the code is not needed a shuffle
as it seems already in a good place. Otherwise it's unclear from this
change why the almost unmodified code is moved around.

...

> >> -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