Hi, > -----Original Message----- > From: Bartosz Golaszewski <[email protected]> > Sent: Friday, March 26, 2021 10:58 PM > To: Michal Simek <[email protected]> > Cc: Syed Nayyar Waris <[email protected]>; Srinivas Neeli > <[email protected]>; Andy Shevchenko > <[email protected]>; William Breathitt Gray > <[email protected]>; Arnd Bergmann <[email protected]>; Robert > Richter <[email protected]>; Linus Walleij <[email protected]>; > Masahiro Yamada <[email protected]>; Andrew Morton > <[email protected]>; Zhang Rui <[email protected]>; Daniel > Lezcano <[email protected]>; Amit Kucheria > <[email protected]>; Linux-Arch <[email protected]>; > linux-gpio <[email protected]>; LKML <linux- > [email protected]>; arm-soc <[email protected]>; > linux-pm <[email protected]>; Srinivas Goud <[email protected]> > Subject: Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and > _set_value > > On Mon, Mar 8, 2021 at 8:13 AM Michal Simek <[email protected]> > wrote: > > > > > > > > On 3/6/21 3:06 PM, Syed Nayyar Waris wrote: > > > This patch reimplements the xgpio_set_multiple() function in > > > drivers/gpio/gpio-xilinx.c to use the new generic functions: > > > bitmap_get_value() and bitmap_set_value(). The code is now simpler > > > to read and understand. Moreover, instead of looping for each bit in > > > xgpio_set_multiple() function, now we can check each channel at a > > > time and save cycles. > > > > > > Cc: Bartosz Golaszewski <[email protected]> > > > Cc: Michal Simek <[email protected]> > > > Signed-off-by: Syed Nayyar Waris <[email protected]> > > > Acked-by: William Breathitt Gray <[email protected]> > > > --- > > > drivers/gpio/gpio-xilinx.c | 63 > > > +++++++++++++++++++------------------- > > > 1 file changed, 32 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c > > > index be539381fd82..8445e69cf37b 100644 > > > --- a/drivers/gpio/gpio-xilinx.c > > > +++ b/drivers/gpio/gpio-xilinx.c > > > @@ -15,6 +15,7 @@ > > > #include <linux/of_device.h> > > > #include <linux/of_platform.h> > > > #include <linux/slab.h> > > > +#include "gpiolib.h" > > > > > > /* Register Offset Definitions */ > > > #define XGPIO_DATA_OFFSET (0x0) /* Data register */ > > > @@ -141,37 +142,37 @@ static void xgpio_set_multiple(struct > > > gpio_chip *gc, unsigned long *mask, { > > > unsigned long flags; > > > struct xgpio_instance *chip = gpiochip_get_data(gc); > > > - int index = xgpio_index(chip, 0); > > > - int offset, i; > > > - > > > - spin_lock_irqsave(&chip->gpio_lock[index], flags); > > > - > > > - /* Write to GPIO signals */ > > > - for (i = 0; i < gc->ngpio; i++) { > > > - if (*mask == 0) > > > - break; > > > - /* Once finished with an index write it out to the register > > > */ > > > - if (index != xgpio_index(chip, i)) { > > > - xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > > > - index * XGPIO_CHANNEL_OFFSET, > > > - chip->gpio_state[index]); > > > - spin_unlock_irqrestore(&chip->gpio_lock[index], > > > flags); > > > - index = xgpio_index(chip, i); > > > - spin_lock_irqsave(&chip->gpio_lock[index], flags); > > > - } > > > - if (__test_and_clear_bit(i, mask)) { > > > - offset = xgpio_offset(chip, i); > > > - if (test_bit(i, bits)) > > > - chip->gpio_state[index] |= BIT(offset); > > > - else > > > - chip->gpio_state[index] &= ~BIT(offset); > > > - } > > > - } > > > - > > > - xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > > > - index * XGPIO_CHANNEL_OFFSET, > > > chip->gpio_state[index]); > > > - > > > - spin_unlock_irqrestore(&chip->gpio_lock[index], flags); > > > + u32 *const state = chip->gpio_state; > > > + unsigned int *const width = chip->gpio_width; > > > + > > > + DECLARE_BITMAP(old, 64); > > > + DECLARE_BITMAP(new, 64); > > > + DECLARE_BITMAP(changed, 64); > > > + > > > + spin_lock_irqsave(&chip->gpio_lock[0], flags); > > > + spin_lock(&chip->gpio_lock[1]); > > > + > > > + bitmap_set_value(old, 64, state[0], width[0], 0); > > > + bitmap_set_value(old, 64, state[1], width[1], width[0]); > > > + bitmap_replace(new, old, bits, mask, gc->ngpio); > > > + > > > + bitmap_set_value(old, 64, state[0], 32, 0); > > > + bitmap_set_value(old, 64, state[1], 32, 32); > > > + state[0] = bitmap_get_value(new, 0, width[0]); > > > + state[1] = bitmap_get_value(new, width[0], width[1]); > > > + bitmap_set_value(new, 64, state[0], 32, 0); > > > + bitmap_set_value(new, 64, state[1], 32, 32); > > > + bitmap_xor(changed, old, new, 64); > > > + > > > + if (((u32 *)changed)[0]) > > > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET, > > > + state[0]); > > > + if (((u32 *)changed)[1]) > > > + xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET + > > > + XGPIO_CHANNEL_OFFSET, state[1]); > > > + > > > + spin_unlock(&chip->gpio_lock[1]); > > > + spin_unlock_irqrestore(&chip->gpio_lock[0], flags); > > > } > > > > > > /** > > > > > > > Srinivas N: Can you please test this code? > > > > Thanks, > > Michal > > Hey, any chance of getting that Tested-by? I tested patches with few modifications in code (spin_lock handling and merge conflict). functionality wise it's working fine.
> > Bart
