| Message ID | KL1PR01MB544800D7E51C9209A9BD998BE6669@KL1PR01MB5448.apcprd01.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] gpio: mmio: restroe get multiple gpio mask | expand |
On Sun, Apr 23, 2023 at 05:06:48PM +0800, Yan Wang wrote: > Simplify the code,should not modify its logic. > Fixes: 761b5c30c206 ("gpio: mmio: replace open-coded for_each_set_bit()") What does it fix? ... > for_each_set_bit(i, mask, gc->bgpio_bits) { > - if (test_bit(i, bits)) > - *set_mask |= bgpio_line2mask(gc, i); > - else > - *clear_mask |= bgpio_line2mask(gc, i); > + if (*mask == 0) > + break; Huh?! We never enter here if mask is 0. So, do not add a dead code, please. Moreover, in principle mask can be longer than 1 long, this code simply wrong. NAK > + if (__test_and_clear_bit(i, mask)) { > + if (test_bit(i, bits)) > + *set_mask |= bgpio_line2mask(gc, i); > + else > + *clear_mask |= bgpio_line2mask(gc, i); > + } > }
> On Apr 24, 2023, at 21:31, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sun, Apr 23, 2023 at 05:06:48PM +0800, Yan Wang wrote: >> Simplify the code,should not modify its logic. > >> Fixes: 761b5c30c206 ("gpio: mmio: replace open-coded for_each_set_bit()") > > What does it fix? > > ... > >> for_each_set_bit(i, mask, gc->bgpio_bits) { >> - if (test_bit(i, bits)) >> - *set_mask |= bgpio_line2mask(gc, i); >> - else >> - *clear_mask |= bgpio_line2mask(gc, i); >> + if (*mask == 0) >> + break; > > Huh?! > > We never enter here if mask is 0. So, do not add a dead code, please. > > Moreover, in principle mask can be longer than 1 long, this code simply wrong. You are right. Because I use gpiod_set_array_value_cansleep() to set multiple gpios occur wrong . I restored logic of the original code and found it to be effective. So,I try to modify it. I recheck logic of this position that it’s correct. I think there should be a error in Gpiolib. > NAK > >> + if (__test_and_clear_bit(i, mask)) { >> + if (test_bit(i, bits)) >> + *set_mask |= bgpio_line2mask(gc, i); >> + else >> + *clear_mask |= bgpio_line2mask(gc, i); >> + } >> } > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index d9dff3dc92ae..c2347ef3a4df 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -271,10 +271,14 @@ static void bgpio_multiple_get_masks(struct gpio_chip *gc, *clear_mask = 0; for_each_set_bit(i, mask, gc->bgpio_bits) { - if (test_bit(i, bits)) - *set_mask |= bgpio_line2mask(gc, i); - else - *clear_mask |= bgpio_line2mask(gc, i); + if (*mask == 0) + break; + if (__test_and_clear_bit(i, mask)) { + if (test_bit(i, bits)) + *set_mask |= bgpio_line2mask(gc, i); + else + *clear_mask |= bgpio_line2mask(gc, i); + } } }
Simplify the code,should not modify its logic. Fixes: 761b5c30c206 ("gpio: mmio: replace open-coded for_each_set_bit()") Signed-off-by: Yan Wang <rk.code@outlook.com> --- drivers/gpio/gpio-mmio.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)