| Message ID | 20200708082634.30191-4-digetx@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improvements for MAX77620 GPIO driver | expand |
On Wed, Jul 8, 2020 at 11:30 AM Dmitry Osipenko <digetx@gmail.com> wrote: > > There is no need to dedicate an array where a bitmap could be used. > Let's replace the interrupt's enable-array with the enable-mask in order > to improve the code a tad. ... > +#include <linux/bitops.h> > unsigned int irq_type[MAX77620_GPIO_NR]; > - bool irq_enabled[MAX77620_GPIO_NR]; > + unsigned long irq_enb_mask; I would rather to move to DECLARE_BITMAP() (the macro is defined in types.h IIRC)
08.07.2020 11:44, Andy Shevchenko пишет: > On Wed, Jul 8, 2020 at 11:30 AM Dmitry Osipenko <digetx@gmail.com> wrote: >> >> There is no need to dedicate an array where a bitmap could be used. >> Let's replace the interrupt's enable-array with the enable-mask in order >> to improve the code a tad. > > ... > >> +#include <linux/bitops.h> > >> unsigned int irq_type[MAX77620_GPIO_NR]; >> - bool irq_enabled[MAX77620_GPIO_NR]; >> + unsigned long irq_enb_mask; > > I would rather to move to DECLARE_BITMAP() > (the macro is defined in types.h IIRC) > Hello, Andy! I know about DECLARE_BITMAP(), it is a very useful macro for bitmaps that are over 32 bits, which is absolutely not the case here. This macro will make code more difficult to read and then we will have to use the bitmap API, which is unnecessary overhead for this case, and thus, it won't be an improvement anymore, IMO. I'd either keep this patch as-is or drop it.
diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c index 6a7ede6b8b74..dd83c16a1ec6 100644 --- a/drivers/gpio/gpio-max77620.c +++ b/drivers/gpio/gpio-max77620.c @@ -5,6 +5,7 @@ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved. */ +#include <linux/bitops.h> #include <linux/gpio/driver.h> #include <linux/interrupt.h> #include <linux/mfd/max77620.h> @@ -20,7 +21,7 @@ struct max77620_gpio { struct device *dev; struct mutex buslock; /* irq_bus_lock */ unsigned int irq_type[MAX77620_GPIO_NR]; - bool irq_enabled[MAX77620_GPIO_NR]; + unsigned long irq_enb_mask; }; static irqreturn_t max77620_gpio_irqhandler(int irq, void *data) @@ -53,7 +54,7 @@ static void max77620_gpio_irq_mask(struct irq_data *data) struct gpio_chip *chip = irq_data_get_irq_chip_data(data); struct max77620_gpio *gpio = gpiochip_get_data(chip); - gpio->irq_enabled[data->hwirq] = false; + clear_bit(data->hwirq, &gpio->irq_enb_mask); } static void max77620_gpio_irq_unmask(struct irq_data *data) @@ -61,7 +62,7 @@ static void max77620_gpio_irq_unmask(struct irq_data *data) struct gpio_chip *chip = irq_data_get_irq_chip_data(data); struct max77620_gpio *gpio = gpiochip_get_data(chip); - gpio->irq_enabled[data->hwirq] = true; + set_bit(data->hwirq, &gpio->irq_enb_mask); } static int max77620_gpio_set_irq_type(struct irq_data *data, unsigned int type) @@ -108,7 +109,10 @@ static void max77620_gpio_bus_sync_unlock(struct irq_data *data) unsigned int value, offset = data->hwirq; int err; - value = gpio->irq_enabled[offset] ? gpio->irq_type[offset] : 0; + if (test_bit(offset, &gpio->irq_enb_mask)) + value = gpio->irq_type[offset]; + else + value = 0; err = regmap_update_bits(gpio->rmap, GPIO_REG_ADDR(offset), MAX77620_CNFG_GPIO_INT_MASK, value);
There is no need to dedicate an array where a bitmap could be used. Let's replace the interrupt's enable-array with the enable-mask in order to improve the code a tad. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpio/gpio-max77620.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)