| Message ID | 0840d55707dacd1121659723246fa9f55737f426.1551598603.git.vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce the for_each_set_clump8 macro | expand |
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 >
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?
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
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.
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.
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);
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(-)