Re: [PATCH v1] gpio: pca953x: Convert to use bitmap API

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Oct 16, 2019 at 05:06:35PM -0400, William Breathitt Gray wrote:
> On Mon, Oct 14, 2019 at 07:11:48PM +0300, Andy Shevchenko wrote:
> > Instead of customized approach convert the driver to use bitmap API.

> Acked-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> 
> I agree with the concept of this change, but there is one fix I would
> like made detailed inline below.

Thanks!

> > -	for_each_set_clump8(offset, bank_mask, mask, gc->ngpio) {
> > -		bank = offset / 8;
> > -		reg_val[bank] &= ~bank_mask;
> > -		reg_val[bank] |= bitmap_get_value8(bits, offset) & bank_mask;
> > -	}
> > +	bitmap_and(reg_val, bits, mask, gc->ngpio);
> 
> When using set_multiple, it's expected that only the GPIO lines
> requested to be set are actually set -- albeit if the hardware is
> capable of that sort of control.
> 
> This bitmap_and operation is ignoring the existing state of reg_val and
> overwriting it with (bits & mask), so existing GPIO states are lost and
> all bits not masked are set to 0.
> 
> What you should do instead is something akin to this (but for bitmaps):
> 
>         regval &= ~mask;
>         regval |= bits & mask;
> 
> That should preserve the existing GPIO states in reg_val, while setting
> those requested by mask and supplied by bits.

Good catch! I suspected I did something wrong here, that's why I Cc'ed you,
and I wasn't mistaken — you helped me, thanks!

-- 
With Best Regards,
Andy Shevchenko