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

[zl020895]

Hi Andy,

Thanks for the follow-up.

After your v9 note marked those sites with _set_bits(), I took that
too literally as regmap_set_bits() alone. That helper cannot clear a
bit, so I left regmap_write() instead of moving to a helper that can
both set and clear. You are right that regmap_assign_bits() is that
helper. There is no separate hardware or cache reason to keep
regmap_write() here. I will use regmap_assign_bits() in the next
version.

Best regards,
Long Zhao

At 2026-10-13 17:50:46, "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>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
>