| Message ID | 4c259d34b5943bf384fd3cb0d98eccf798a34f0f.1615038553.git.syednwaris@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Introduce the for_each_set_nbits macro | expand |
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 <bgolaszewski@baylibre.com> > Cc: Michal Simek <michal.simek@xilinx.com> > Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com> > Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > --- > 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
On Mon, Mar 8, 2021 at 8:13 AM Michal Simek <michal.simek@xilinx.com> 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 <bgolaszewski@baylibre.com> > > Cc: Michal Simek <michal.simek@xilinx.com> > > Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com> > > Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > --- > > 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? Bart
On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> 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. ... > + u32 *const state = chip->gpio_state; > + unsigned int *const width = chip->gpio_width; > + Extra blank line. > + 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]); I understand why this is done at the top of the function in the original code. I do not understand why you put some operations under spin lock. Have you checked what each of these spin locks protects? Please check and try to lock as minimum as possible. > + 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); Original code and this is cryptic. Can you add a few comments explaining what is going on here? > + spin_unlock(&chip->gpio_lock[1]); > + spin_unlock_irqrestore(&chip->gpio_lock[0], flags);
On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > + bitmap_set_value(old, 64, state[0], 32, 0); > + bitmap_set_value(old, 64, state[1], 32, 32); Isn't it effectively bitnap_from_arr32() ? > + bitmap_set_value(new, 64, state[0], 32, 0); > + bitmap_set_value(new, 64, state[1], 32, 32); Ditto.
On Fri, Mar 26, 2021 at 11:32 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > + bitmap_set_value(old, 64, state[0], 32, 0); > > + bitmap_set_value(old, 64, state[1], 32, 32); > > Isn't it effectively bitnap_from_arr32() ? > > > + bitmap_set_value(new, 64, state[0], 32, 0); > > + bitmap_set_value(new, 64, state[1], 32, 32); > > Ditto. > > -- > With Best Regards, > Andy Shevchenko Hi Andy, With bitmap_set_value() we are also specifying the offset (or start) position too. so that the remainder of the array remains unaffected. I think it would not be feasible to use bitmap_from/to_arr32() here. Regards Syed Nayyar Waris
On Sat, Mar 27, 2021 at 09:29:26AM +0200, Andy Shevchenko wrote: > On Saturday, March 27, 2021, Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > On Fri, Mar 26, 2021 at 11:32 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> > > wrote: > > > > > > > + bitmap_set_value(old, 64, state[0], 32, 0); > > > > + bitmap_set_value(old, 64, state[1], 32, 32); > > > > > > Isn't it effectively bitnap_from_arr32() ? > > > > > > > + bitmap_set_value(new, 64, state[0], 32, 0); > > > > + bitmap_set_value(new, 64, state[1], 32, 32); > > > > > > Ditto. > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko > > > > Hi Andy, > > > > With bitmap_set_value() we are also specifying the offset (or start) > > position too. so that the remainder of the array remains unaffected. I > > think it would not be feasible to use bitmap_from/to_arr32() here. > > > You have hard coded start and nbits parameters to 32. How is it not the > same? Would these four lines become something like this: bitmap_from_arr32(old, state, 64); ... bitmap_from_arr32(new, state, 64); Sincerely, William Breathitt Gray
On Sat, Mar 27, 2021 at 2:02 PM William Breathitt Gray <vilhelm.gray@gmail.com> wrote: > On Sat, Mar 27, 2021 at 09:29:26AM +0200, Andy Shevchenko wrote: > > On Saturday, March 27, 2021, Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > On Fri, Mar 26, 2021 at 11:32 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> > > > wrote: > > > > > > > > > + bitmap_set_value(old, 64, state[0], 32, 0); > > > > > + bitmap_set_value(old, 64, state[1], 32, 32); > > > > > > > > Isn't it effectively bitnap_from_arr32() ? > > > > > > > > > + bitmap_set_value(new, 64, state[0], 32, 0); > > > > > + bitmap_set_value(new, 64, state[1], 32, 32); > > > > > > > > Ditto. > > > With bitmap_set_value() we are also specifying the offset (or start) > > > position too. so that the remainder of the array remains unaffected. I > > > think it would not be feasible to use bitmap_from/to_arr32() here. > > > > > > You have hard coded start and nbits parameters to 32. How is it not the > > same? > > Would these four lines become something like this: > > bitmap_from_arr32(old, state, 64); > ... > bitmap_from_arr32(new, state, 64); This is my understanding, but I might miss something. I mean driver specifics that make my proposal incorrect.
On Sat, Mar 06, 2021 at 07:36:30PM +0530, 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. ... > + u32 *const state = chip->gpio_state; Looking at this... What's the point of the const here? Am I right that this tells: pointer is a const, while the data underneath can be modified? > + unsigned int *const width = chip->gpio_width; Ditto. Putting const:s here and there for sake of the const is not good practice. It makes code harder to read.
Hi, > -----Original Message----- > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Sent: Friday, March 26, 2021 10:58 PM > To: Michal Simek <michals@xilinx.com> > Cc: Syed Nayyar Waris <syednwaris@gmail.com>; Srinivas Neeli > <sneeli@xilinx.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; William Breathitt Gray > <vilhelm.gray@gmail.com>; Arnd Bergmann <arnd@arndb.de>; Robert > Richter <rrichter@marvell.com>; Linus Walleij <linus.walleij@linaro.org>; > Masahiro Yamada <yamada.masahiro@socionext.com>; Andrew Morton > <akpm@linux-foundation.org>; Zhang Rui <rui.zhang@intel.com>; Daniel > Lezcano <daniel.lezcano@linaro.org>; Amit Kucheria > <amit.kucheria@verdurent.com>; Linux-Arch <linux-arch@vger.kernel.org>; > linux-gpio <linux-gpio@vger.kernel.org>; LKML <linux- > kernel@vger.kernel.org>; arm-soc <linux-arm-kernel@lists.infradead.org>; > linux-pm <linux-pm@vger.kernel.org>; Srinivas Goud <sgoud@xilinx.com> > 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 <michal.simek@xilinx.com> > 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 <bgolaszewski@baylibre.com> > > > Cc: Michal Simek <michal.simek@xilinx.com> > > > Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com> > > > Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > > --- > > > 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
On Wed, Mar 31, 2021 at 8:56 PM Srinivas Neeli <sneeli@xilinx.com> wrote: > > Hi, > > > -----Original Message----- > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Sent: Friday, March 26, 2021 10:58 PM > > To: Michal Simek <michals@xilinx.com> > > Cc: Syed Nayyar Waris <syednwaris@gmail.com>; Srinivas Neeli > > <sneeli@xilinx.com>; Andy Shevchenko > > <andriy.shevchenko@linux.intel.com>; William Breathitt Gray > > <vilhelm.gray@gmail.com>; Arnd Bergmann <arnd@arndb.de>; Robert > > Richter <rrichter@marvell.com>; Linus Walleij <linus.walleij@linaro.org>; > > Masahiro Yamada <yamada.masahiro@socionext.com>; Andrew Morton > > <akpm@linux-foundation.org>; Zhang Rui <rui.zhang@intel.com>; Daniel > > Lezcano <daniel.lezcano@linaro.org>; Amit Kucheria > > <amit.kucheria@verdurent.com>; Linux-Arch <linux-arch@vger.kernel.org>; > > linux-gpio <linux-gpio@vger.kernel.org>; LKML <linux- > > kernel@vger.kernel.org>; arm-soc <linux-arm-kernel@lists.infradead.org>; > > linux-pm <linux-pm@vger.kernel.org>; Srinivas Goud <sgoud@xilinx.com> > > 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 <michal.simek@xilinx.com> > > 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 <bgolaszewski@baylibre.com> > > > > Cc: Michal Simek <michal.simek@xilinx.com> > > > > Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com> > > > > Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > > > --- > > > > 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 Hi Bartosz, May I please know the URL of the tree that you are using. I had been using the tree below for submitting this patchset on GPIO to you. https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git I think I am using the wrong tree. On which tree should I base my patches on for my next (v4) submission? Should I use the tree below? : https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git Regards Syed Nayyar Waris
On Thu, Apr 1, 2021 at 1:16 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > On Wed, Mar 31, 2021 at 8:56 PM Srinivas Neeli <sneeli@xilinx.com> wrote: > > > > Hi, > > > > > -----Original Message----- > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > Sent: Friday, March 26, 2021 10:58 PM > > > To: Michal Simek <michals@xilinx.com> > > > Cc: Syed Nayyar Waris <syednwaris@gmail.com>; Srinivas Neeli > > > <sneeli@xilinx.com>; Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com>; William Breathitt Gray > > > <vilhelm.gray@gmail.com>; Arnd Bergmann <arnd@arndb.de>; Robert > > > Richter <rrichter@marvell.com>; Linus Walleij <linus.walleij@linaro.org>; > > > Masahiro Yamada <yamada.masahiro@socionext.com>; Andrew Morton > > > <akpm@linux-foundation.org>; Zhang Rui <rui.zhang@intel.com>; Daniel > > > Lezcano <daniel.lezcano@linaro.org>; Amit Kucheria > > > <amit.kucheria@verdurent.com>; Linux-Arch <linux-arch@vger.kernel.org>; > > > linux-gpio <linux-gpio@vger.kernel.org>; LKML <linux- > > > kernel@vger.kernel.org>; arm-soc <linux-arm-kernel@lists.infradead.org>; > > > linux-pm <linux-pm@vger.kernel.org>; Srinivas Goud <sgoud@xilinx.com> > > > 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 <michal.simek@xilinx.com> > > > 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 <bgolaszewski@baylibre.com> > > > > > Cc: Michal Simek <michal.simek@xilinx.com> > > > > > Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com> > > > > > Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com> > > > > > --- > > > > > 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 > > Hi Bartosz, > > May I please know the URL of the tree that you are using. I had been > using the tree below for submitting this patchset on GPIO to you. > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git > > I think I am using the wrong tree. On which tree should I base my > patches on for my next (v4) submission? Should I use the tree below? > : > https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git > > Regards > Syed Nayyar Waris Yes this is the one. Please address new issues raised by reviewers. Bart
On Mon, Mar 29, 2021 at 8:54 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sat, Mar 06, 2021 at 07:36:30PM +0530, 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. > > ... > > > + u32 *const state = chip->gpio_state; > > Looking at this... What's the point of the const here? > > Am I right that this tells: pointer is a const, while the data underneath > can be modified? > > > + unsigned int *const width = chip->gpio_width; > > Ditto. > > Putting const:s here and there for sake of the const is not good practice. > It makes code harder to read. > > -- > With Best Regards, > Andy Shevchenko > Okay. I will incorporate your comments in my next submission. Thank You. Regards Syed Nayyar Waris
On Sat, Mar 27, 2021 at 10:05 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sat, Mar 27, 2021 at 2:02 PM William Breathitt Gray > <vilhelm.gray@gmail.com> wrote: > > On Sat, Mar 27, 2021 at 09:29:26AM +0200, Andy Shevchenko wrote: > > > On Saturday, March 27, 2021, Syed Nayyar Waris <syednwaris@gmail.com> wrote: > > > > On Fri, Mar 26, 2021 at 11:32 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> > > > > wrote: > > > > > > > > > > > + bitmap_set_value(old, 64, state[0], 32, 0); > > > > > > + bitmap_set_value(old, 64, state[1], 32, 32); > > > > > > > > > > Isn't it effectively bitnap_from_arr32() ? > > > > > > > > > > > + bitmap_set_value(new, 64, state[0], 32, 0); > > > > > > + bitmap_set_value(new, 64, state[1], 32, 32); > > > > > > > > > > Ditto. > > > > > With bitmap_set_value() we are also specifying the offset (or start) > > > > position too. so that the remainder of the array remains unaffected. I > > > > think it would not be feasible to use bitmap_from/to_arr32() here. > > > > > > > > > You have hard coded start and nbits parameters to 32. How is it not the > > > same? > > > > Would these four lines become something like this: > > > > bitmap_from_arr32(old, state, 64); > > ... > > bitmap_from_arr32(new, state, 64); > > This is my understanding, but I might miss something. I mean driver > specifics that make my proposal incorrect. > > -- > With Best Regards, > Andy Shevchenko I initially (incorrectly) thought that all of the bitmap_set_value() statements have to be replaced. But now I realised, only those specific bitmap_set_value() calls containing 32 bits width have to replaced. I will incorporate the above review comments in my next v4 submission. Regards Syed Nayyar Waris
On Fri, Mar 26, 2021 at 11:27 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sat, Mar 6, 2021 at 4:08 PM Syed Nayyar Waris <syednwaris@gmail.com> 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. > > ... > > > + u32 *const state = chip->gpio_state; > > + unsigned int *const width = chip->gpio_width; > > > + > > Extra blank line. > > > + 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]); > > I understand why this is done at the top of the function in the original code. > I do not understand why you put some operations under spin lock. > > Have you checked what each of these spin locks protects? > Please check and try to lock as minimum as possible. > > > + 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); > > Original code and this is cryptic. Can you add a few comments > explaining what is going on here? > > > + spin_unlock(&chip->gpio_lock[1]); > > + spin_unlock_irqrestore(&chip->gpio_lock[0], flags); > > -- > With Best Regards, > Andy Shevchenko Have removed the extra line and added comments. Regarding locking - I see that now there is just a single lock available instead of 2 locks. Have made necessary changes. Thanks https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpio/gpio-xilinx.c?id=37ef334680800263b32bb96a5156a4b47f0244a2 Regards Syed Nayyar Waris
On Mon, Mar 29, 2021 at 8:54 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sat, Mar 06, 2021 at 07:36:30PM +0530, 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. > > ... > > > + u32 *const state = chip->gpio_state; > > Looking at this... What's the point of the const here? > > Am I right that this tells: pointer is a const, while the data underneath > can be modified? Yes you are right and the data underneath can be modified. I have removed the 'const' in v4 > > > + unsigned int *const width = chip->gpio_width; > > Ditto. > > Putting const:s here and there for sake of the const is not good practice. > It makes code harder to read. Okay. > > -- > With Best Regards, > Andy Shevchenko > >
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); } /**