[v9,9/9] gpio: uniphier: Utilize for_each_set_clump8 macro
diff mbox series

Message ID 0840d55707dacd1121659723246fa9f55737f426.1551598603.git.vilhelm.gray@gmail.com
State New
Headers show
Series
  • Introduce the for_each_set_clump8 macro
Related show

Commit Message

William Breathitt Gray March 3, 2019, 7:51 a.m. UTC
Replace verbose implementation in set_multiple callback with
for_each_set_clump8 macro to simplify code and improve clarity. An
improvement in this case is that banks that are not masked will now be
skipped.

Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 drivers/gpio/gpio-uniphier.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Comments

Masahiro Yamada March 12, 2019, 4:36 a.m. UTC | #1
On Sun, Mar 3, 2019 at 4:51 PM William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
>
> Replace verbose implementation in set_multiple callback with
> for_each_set_clump8 macro to simplify code and improve clarity. An
> improvement in this case is that banks that are not masked will now be
> skipped.
>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>  drivers/gpio/gpio-uniphier.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
> index 0f662b297a95..df640cb29b9c 100644
> --- a/drivers/gpio/gpio-uniphier.c
> +++ b/drivers/gpio/gpio-uniphier.c
> @@ -15,9 +15,6 @@
>  #include <linux/spinlock.h>
>  #include <dt-bindings/gpio/uniphier-gpio.h>
>
> -#define UNIPHIER_GPIO_BANK_MASK                \
> -                               GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
> -
>  #define UNIPHIER_GPIO_IRQ_MAX_NUM      24
>
>  #define UNIPHIER_GPIO_PORT_DATA                0x0     /* data */
> @@ -147,15 +144,14 @@ static void uniphier_gpio_set(struct gpio_chip *chip,
>  static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
>                                        unsigned long *mask, unsigned long *bits)
>  {
> -       unsigned int bank, shift, bank_mask, bank_bits;
> -       int i;
> +       unsigned int i;
> +       unsigned long bank_mask;
> +       unsigned int bank;
> +       unsigned int bank_bits;
>
> -       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
> +       for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
>                 bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
> -               shift = i % BITS_PER_LONG;
> -               bank_mask = (mask[BIT_WORD(i)] >> shift) &
> -                                               UNIPHIER_GPIO_BANK_MASK;
> -               bank_bits = bits[BIT_WORD(i)] >> shift;
> +               bank_bits = bitmap_get_value8(bits, chip->ngpio, i);
>
>                 uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
>                                          bank_mask, bank_bits);


Please do not do this.

Nothing in this driver says the GPIO width is 8-bit.

You are hard-coding '8-bit'.







> --
> 2.21.0
>
Andy Shevchenko March 12, 2019, 7:17 a.m. UTC | #2
On Tue, Mar 12, 2019 at 6:40 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Sun, Mar 3, 2019 at 4:51 PM William Breathitt Gray
> <vilhelm.gray@gmail.com> wrote:
> >
> > Replace verbose implementation in set_multiple callback with
> > for_each_set_clump8 macro to simplify code and improve clarity. An
> > improvement in this case is that banks that are not masked will now be
> > skipped.

> Please do not do this.
>
> Nothing in this driver says the GPIO width is 8-bit.

Huh?

https://elixir.bootlin.com/linux/latest/source/include/dt-bindings/gpio/uniphier-gpio.h#L9

Isn't a hardcoding?
William Breathitt Gray March 12, 2019, 7:29 a.m. UTC | #3
On Tue, Mar 12, 2019 at 01:36:57PM +0900, Masahiro Yamada wrote:
> On Sun, Mar 3, 2019 at 4:51 PM William Breathitt Gray
> <vilhelm.gray@gmail.com> wrote:
> >
> > Replace verbose implementation in set_multiple callback with
> > for_each_set_clump8 macro to simplify code and improve clarity. An
> > improvement in this case is that banks that are not masked will now be
> > skipped.
> >
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> >  drivers/gpio/gpio-uniphier.c | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
> > index 0f662b297a95..df640cb29b9c 100644
> > --- a/drivers/gpio/gpio-uniphier.c
> > +++ b/drivers/gpio/gpio-uniphier.c
> > @@ -15,9 +15,6 @@
> >  #include <linux/spinlock.h>
> >  #include <dt-bindings/gpio/uniphier-gpio.h>
> >
> > -#define UNIPHIER_GPIO_BANK_MASK                \
> > -                               GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
> > -
> >  #define UNIPHIER_GPIO_IRQ_MAX_NUM      24
> >
> >  #define UNIPHIER_GPIO_PORT_DATA                0x0     /* data */
> > @@ -147,15 +144,14 @@ static void uniphier_gpio_set(struct gpio_chip *chip,
> >  static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
> >                                        unsigned long *mask, unsigned long *bits)
> >  {
> > -       unsigned int bank, shift, bank_mask, bank_bits;
> > -       int i;
> > +       unsigned int i;
> > +       unsigned long bank_mask;
> > +       unsigned int bank;
> > +       unsigned int bank_bits;
> >
> > -       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
> > +       for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
> >                 bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
> > -               shift = i % BITS_PER_LONG;
> > -               bank_mask = (mask[BIT_WORD(i)] >> shift) &
> > -                                               UNIPHIER_GPIO_BANK_MASK;
> > -               bank_bits = bits[BIT_WORD(i)] >> shift;
> > +               bank_bits = bitmap_get_value8(bits, chip->ngpio, i);
> >
> >                 uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
> >                                          bank_mask, bank_bits);
> 
> 
> Please do not do this.
> 
> Nothing in this driver says the GPIO width is 8-bit.
> 
> You are hard-coding '8-bit'.

The for_each_set_clump8 macro is hardcoded to 8-bit clumps because the
current drivers utilizing this functionality are only updating their
GPIO ports 8-bits at a time.

However, if this driver updates more or less GPIO at a time, we can
easily update the macro by replacing the hardcoded '8' value with a
variable, thus giving us the generic for_each_set_clump macro.

How many GPIO can be updated at a time for this device?

William Breathitt Gray

> 
> 
> 
> 
> 
> 
> 
> > --
> > 2.21.0
> >
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
Masahiro Yamada March 12, 2019, 8:57 a.m. UTC | #4
On Tue, Mar 12, 2019 at 4:19 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Mar 12, 2019 at 6:40 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> > On Sun, Mar 3, 2019 at 4:51 PM William Breathitt Gray
> > <vilhelm.gray@gmail.com> wrote:
> > >
> > > Replace verbose implementation in set_multiple callback with
> > > for_each_set_clump8 macro to simplify code and improve clarity. An
> > > improvement in this case is that banks that are not masked will now be
> > > skipped.
>
> > Please do not do this.
> >
> > Nothing in this driver says the GPIO width is 8-bit.
>
> Huh?
>
> https://elixir.bootlin.com/linux/latest/source/include/dt-bindings/gpio/uniphier-gpio.h#L9
>
> Isn't a hardcoding?


Semi-hardcoding.

I needed to factor out some magic numbers
shared between DT and drivers.

Then, dt-bindings is out of realm of operating system.

If I am doing wrong, I take back my comments, though.
Andy Shevchenko March 12, 2019, 9:09 a.m. UTC | #5
On Tue, Mar 12, 2019 at 10:58 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Tue, Mar 12, 2019 at 4:19 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Mar 12, 2019 at 6:40 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> > > On Sun, Mar 3, 2019 at 4:51 PM William Breathitt Gray
> > > <vilhelm.gray@gmail.com> wrote:
> > > >
> > > > Replace verbose implementation in set_multiple callback with
> > > > for_each_set_clump8 macro to simplify code and improve clarity. An
> > > > improvement in this case is that banks that are not masked will now be
> > > > skipped.
> >
> > > Please do not do this.
> > >
> > > Nothing in this driver says the GPIO width is 8-bit.
> >
> > Huh?
> >
> > https://elixir.bootlin.com/linux/latest/source/include/dt-bindings/gpio/uniphier-gpio.h#L9
> >
> > Isn't a hardcoding?
>
>
> Semi-hardcoding.
>
> I needed to factor out some magic numbers
> shared between DT and drivers.

Effectively means you introduced an ABI, which we are not supposed to
change, where the number is carved in stone for all hardware covered
by this driver + DT pair.
If you would ever need another one it would require extending existing
bindings without dropping them away.

> Then, dt-bindings is out of realm of operating system.

Exactly!

> If I am doing wrong, I take back my comments, though.

Patch
diff mbox series

diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
index 0f662b297a95..df640cb29b9c 100644
--- a/drivers/gpio/gpio-uniphier.c
+++ b/drivers/gpio/gpio-uniphier.c
@@ -15,9 +15,6 @@ 
 #include <linux/spinlock.h>
 #include <dt-bindings/gpio/uniphier-gpio.h>
 
-#define UNIPHIER_GPIO_BANK_MASK		\
-				GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
-
 #define UNIPHIER_GPIO_IRQ_MAX_NUM	24
 
 #define UNIPHIER_GPIO_PORT_DATA		0x0	/* data */
@@ -147,15 +144,14 @@  static void uniphier_gpio_set(struct gpio_chip *chip,
 static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
 				       unsigned long *mask, unsigned long *bits)
 {
-	unsigned int bank, shift, bank_mask, bank_bits;
-	int i;
+	unsigned int i;
+	unsigned long bank_mask;
+	unsigned int bank;
+	unsigned int bank_bits;
 
-	for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
+	for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
 		bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
-		shift = i % BITS_PER_LONG;
-		bank_mask = (mask[BIT_WORD(i)] >> shift) &
-						UNIPHIER_GPIO_BANK_MASK;
-		bank_bits = bits[BIT_WORD(i)] >> shift;
+		bank_bits = bitmap_get_value8(bits, chip->ngpio, i);
 
 		uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
 					 bank_mask, bank_bits);