* [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-06 14:06 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-03-06 14:06 UTC (permalink / raw)
To: bgolaszewski
Cc: andriy.shevchenko, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, bgolaszewski, yamada.masahiro, akpm, rui.zhang,
daniel.lezcano, amit.kucheria, linux-arch, linux-gpio,
linux-kernel, linux-arm-kernel, linux-pm
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);
}
/**
--
2.29.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-06 14:06 ` Syed Nayyar Waris
@ 2021-03-08 7:13 ` Michal Simek
-1 siblings, 0 replies; 38+ messages in thread
From: Michal Simek @ 2021-03-08 7:13 UTC (permalink / raw)
To: Syed Nayyar Waris, bgolaszewski, Srinivas Neeli
Cc: andriy.shevchenko, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, yamada.masahiro, akpm, rui.zhang, daniel.lezcano,
amit.kucheria, linux-arch, linux-gpio, linux-kernel,
linux-arm-kernel, linux-pm, Srinivas Goud
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-08 7:13 ` Michal Simek
0 siblings, 0 replies; 38+ messages in thread
From: Michal Simek @ 2021-03-08 7:13 UTC (permalink / raw)
To: Syed Nayyar Waris, bgolaszewski, Srinivas Neeli
Cc: andriy.shevchenko, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, yamada.masahiro, akpm, rui.zhang, daniel.lezcano,
amit.kucheria, linux-arch, linux-gpio, linux-kernel,
linux-arm-kernel, linux-pm, Srinivas Goud
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-08 7:13 ` Michal Simek
@ 2021-03-26 17:27 ` Bartosz Golaszewski
-1 siblings, 0 replies; 38+ messages in thread
From: Bartosz Golaszewski @ 2021-03-26 17:27 UTC (permalink / raw)
To: Michal Simek
Cc: Syed Nayyar Waris, Srinivas Neeli, Andy Shevchenko,
William Breathitt Gray, Arnd Bergmann, Robert Richter,
Linus Walleij, Masahiro Yamada, Andrew Morton, Zhang Rui,
Daniel Lezcano, Amit Kucheria, Linux-Arch, linux-gpio, LKML,
arm-soc, linux-pm, Srinivas Goud
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-26 17:27 ` Bartosz Golaszewski
0 siblings, 0 replies; 38+ messages in thread
From: Bartosz Golaszewski @ 2021-03-26 17:27 UTC (permalink / raw)
To: Michal Simek
Cc: Syed Nayyar Waris, Srinivas Neeli, Andy Shevchenko,
William Breathitt Gray, Arnd Bergmann, Robert Richter,
Linus Walleij, Masahiro Yamada, Andrew Morton, Zhang Rui,
Daniel Lezcano, Amit Kucheria, Linux-Arch, linux-gpio, LKML,
arm-soc, linux-pm, Srinivas Goud
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-26 17:27 ` Bartosz Golaszewski
@ 2021-03-31 15:26 ` Srinivas Neeli
-1 siblings, 0 replies; 38+ messages in thread
From: Srinivas Neeli @ 2021-03-31 15:26 UTC (permalink / raw)
To: Bartosz Golaszewski, Michal Simek
Cc: Syed Nayyar Waris, Andy Shevchenko, William Breathitt Gray,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, linux-gpio, LKML, arm-soc, linux-pm, Srinivas Goud
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-31 15:26 ` Srinivas Neeli
0 siblings, 0 replies; 38+ messages in thread
From: Srinivas Neeli @ 2021-03-31 15:26 UTC (permalink / raw)
To: Bartosz Golaszewski, Michal Simek
Cc: Syed Nayyar Waris, Andy Shevchenko, William Breathitt Gray,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, linux-gpio, LKML, arm-soc, linux-pm, Srinivas Goud
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-31 15:26 ` Srinivas Neeli
@ 2021-04-01 11:15 ` Syed Nayyar Waris
-1 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-01 11:15 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Michal Simek, Andy Shevchenko, William Breathitt Gray,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, linux-gpio, LKML, arm-soc, linux-pm, Srinivas Goud,
Srinivas Neeli
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-04-01 11:15 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-01 11:15 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Michal Simek, Andy Shevchenko, William Breathitt Gray,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, linux-gpio, LKML, arm-soc, linux-pm, Srinivas Goud,
Srinivas Neeli
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-04-01 11:15 ` Syed Nayyar Waris
@ 2021-04-01 12:51 ` Bartosz Golaszewski
-1 siblings, 0 replies; 38+ messages in thread
From: Bartosz Golaszewski @ 2021-04-01 12:51 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: Michal Simek, Andy Shevchenko, William Breathitt Gray,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, linux-gpio, LKML, arm-soc, linux-pm, Srinivas Goud,
Srinivas Neeli
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-04-01 12:51 ` Bartosz Golaszewski
0 siblings, 0 replies; 38+ messages in thread
From: Bartosz Golaszewski @ 2021-04-01 12:51 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: Michal Simek, Andy Shevchenko, William Breathitt Gray,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, linux-gpio, LKML, arm-soc, linux-pm, Srinivas Goud,
Srinivas Neeli
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-06 14:06 ` Syed Nayyar Waris
@ 2021-03-26 17:57 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-26 17:57 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-26 17:57 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-26 17:57 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-26 17:57 ` Andy Shevchenko
@ 2021-04-02 9:40 ` Syed Nayyar Waris
-1 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-02 9:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-04-02 9:40 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-02 9:40 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-06 14:06 ` Syed Nayyar Waris
@ 2021-03-26 18:02 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-26 18:02 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-26 18:02 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-26 18:02 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-26 18:02 ` Andy Shevchenko
@ 2021-03-26 22:12 ` Syed Nayyar Waris
-1 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-03-26 22:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-26 22:12 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-03-26 22:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, Andy Shevchenko, William Breathitt Gray,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
[parent not found: <CAHp75VfDZbJjCOEGdHc=-D6W8_7m2=CinXj-itwn6hvoVqdWYQ@mail.gmail.com>]
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
[not found] ` <CAHp75VfDZbJjCOEGdHc=-D6W8_7m2=CinXj-itwn6hvoVqdWYQ@mail.gmail.com>
@ 2021-03-27 12:02 ` William Breathitt Gray
0 siblings, 0 replies; 38+ messages in thread
From: William Breathitt Gray @ 2021-03-27 12:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Syed Nayyar Waris, Bartosz Golaszewski, Andy Shevchenko,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-27 12:02 ` William Breathitt Gray
0 siblings, 0 replies; 38+ messages in thread
From: William Breathitt Gray @ 2021-03-27 12:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Syed Nayyar Waris, Bartosz Golaszewski, Andy Shevchenko,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
[-- Attachment #1.1: Type: text/plain, Size: 1261 bytes --]
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
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-27 12:02 ` William Breathitt Gray
@ 2021-03-27 16:35 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-27 16:35 UTC (permalink / raw)
To: William Breathitt Gray
Cc: Syed Nayyar Waris, Bartosz Golaszewski, Andy Shevchenko,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-27 16:35 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-27 16:35 UTC (permalink / raw)
To: William Breathitt Gray
Cc: Syed Nayyar Waris, Bartosz Golaszewski, Andy Shevchenko,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-27 16:35 ` Andy Shevchenko
@ 2021-04-01 17:50 ` Syed Nayyar Waris
-1 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-01 17:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: William Breathitt Gray, Bartosz Golaszewski, Andy Shevchenko,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-04-01 17:50 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-01 17:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: William Breathitt Gray, Bartosz Golaszewski, Andy Shevchenko,
Michal Simek, Arnd Bergmann, Robert Richter, Linus Walleij,
Masahiro Yamada, Andrew Morton, Zhang, Rui, Daniel Lezcano,
Amit Kucheria, Linux-Arch, open list:GPIO SUBSYSTEM,
Linux Kernel Mailing List, linux-arm Mailing List, Linux PM
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-06 14:06 ` Syed Nayyar Waris
@ 2021-03-29 15:24 ` Andy Shevchenko
-1 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-29 15:24 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: bgolaszewski, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, yamada.masahiro, akpm, rui.zhang, daniel.lezcano,
amit.kucheria, linux-arch, linux-gpio, linux-kernel,
linux-arm-kernel, linux-pm
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-03-29 15:24 ` Andy Shevchenko
0 siblings, 0 replies; 38+ messages in thread
From: Andy Shevchenko @ 2021-03-29 15:24 UTC (permalink / raw)
To: Syed Nayyar Waris
Cc: bgolaszewski, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, yamada.masahiro, akpm, rui.zhang, daniel.lezcano,
amit.kucheria, linux-arch, linux-gpio, linux-kernel,
linux-arm-kernel, linux-pm
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-29 15:24 ` Andy Shevchenko
@ 2021-04-01 17:47 ` Syed Nayyar Waris
-1 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-01 17:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: bgolaszewski, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, yamada.masahiro, akpm, rui.zhang, daniel.lezcano,
amit.kucheria, linux-arch, linux-gpio, linux-kernel,
linux-arm-kernel, linux-pm
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-04-01 17:47 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-01 17:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: bgolaszewski, vilhelm.gray, michal.simek, arnd, rrichter,
linus.walleij, yamada.masahiro, akpm, rui.zhang, daniel.lezcano,
amit.kucheria, linux-arch, linux-gpio, linux-kernel,
linux-arm-kernel, linux-pm
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
2021-03-29 15:24 ` Andy Shevchenko
@ 2021-04-02 10:07 ` Syed Nayyar Waris
-1 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-02 10:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, William Breathitt Gray, Michal Simek,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang, Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
linux-arm Mailing List, Linux PM
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
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 3/3] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
@ 2021-04-02 10:07 ` Syed Nayyar Waris
0 siblings, 0 replies; 38+ messages in thread
From: Syed Nayyar Waris @ 2021-04-02 10:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bartosz Golaszewski, William Breathitt Gray, Michal Simek,
Arnd Bergmann, Robert Richter, Linus Walleij, Masahiro Yamada,
Andrew Morton, Zhang, Rui, Daniel Lezcano, Amit Kucheria,
Linux-Arch, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
linux-arm Mailing List, Linux PM
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
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 38+ messages in thread