[v1] gpio: pca953x: Convert to use bitmap API
diff mbox series

Message ID 20191014161148.10543-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • [v1] gpio: pca953x: Convert to use bitmap API
Related show

Commit Message

Andy Shevchenko Oct. 14, 2019, 4:11 p.m. UTC
Instead of customized approach convert the driver to use bitmap API.

Depends-on: 6e9c6674d1bf ("gpio: pca953x: utilize the for_each_set_clump8 macro")
Cc: William Breathitt Gray <vilhelm.gray@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Marek Vasut <marek.vasut+renesas@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 170 ++++++++++++++++--------------------
 1 file changed, 73 insertions(+), 97 deletions(-)

Comments

William Breathitt Gray Oct. 16, 2019, 9:06 p.m. UTC | #1
On Mon, Oct 14, 2019 at 07:11:48PM +0300, Andy Shevchenko wrote:
> Instead of customized approach convert the driver to use bitmap API.
> 
> Depends-on: 6e9c6674d1bf ("gpio: pca953x: utilize the for_each_set_clump8 macro")
> Cc: William Breathitt Gray <vilhelm.gray@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Marek Vasut <marek.vasut+renesas@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

I agree with the concept of this change, but there is one fix I would
like made detailed inline below.

> ---
>  drivers/gpio/gpio-pca953x.c | 170 ++++++++++++++++--------------------
>  1 file changed, 73 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 10b669b8f27d..95c2d6c99f41 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -9,8 +9,7 @@
>   */
>  
>  #include <linux/acpi.h>
> -#include <linux/bits.h>
> -#include <linux/bitops.h>
> +#include <linux/bitmap.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> @@ -116,6 +115,7 @@ MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
>  
>  #define MAX_BANK 5
>  #define BANK_SZ 8
> +#define MAX_LINE	(MAX_BANK * BANK_SZ)
>  
>  #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
>  
> @@ -147,10 +147,10 @@ struct pca953x_chip {
>  
>  #ifdef CONFIG_GPIO_PCA953X_IRQ
>  	struct mutex irq_lock;
> -	u8 irq_mask[MAX_BANK];
> -	u8 irq_stat[MAX_BANK];
> -	u8 irq_trig_raise[MAX_BANK];
> -	u8 irq_trig_fall[MAX_BANK];
> +	DECLARE_BITMAP(irq_mask, MAX_LINE);
> +	DECLARE_BITMAP(irq_stat, MAX_LINE);
> +	DECLARE_BITMAP(irq_trig_raise, MAX_LINE);
> +	DECLARE_BITMAP(irq_trig_fall, MAX_LINE);
>  	struct irq_chip irq_chip;
>  #endif
>  	atomic_t wakeup_path;
> @@ -334,12 +334,16 @@ static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
>  	return regaddr;
>  }
>  
> -static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
> +static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
>  {
>  	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
> -	int ret;
> +	u8 value[MAX_BANK];
> +	int i, ret;
> +
> +	for (i = 0; i < NBANK(chip); i++)
> +		value[i] = bitmap_get_value8(val, i * BANK_SZ);
>  
> -	ret = regmap_bulk_write(chip->regmap, regaddr, val, NBANK(chip));
> +	ret = regmap_bulk_write(chip->regmap, regaddr, value, NBANK(chip));
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev, "failed writing register\n");
>  		return ret;
> @@ -348,17 +352,21 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
>  	return 0;
>  }
>  
> -static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
> +static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
>  {
>  	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, false, true);
> -	int ret;
> +	u8 value[MAX_BANK];
> +	int i, ret;
>  
> -	ret = regmap_bulk_read(chip->regmap, regaddr, val, NBANK(chip));
> +	ret = regmap_bulk_read(chip->regmap, regaddr, value, NBANK(chip));
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev, "failed reading register\n");
>  		return ret;
>  	}
>  
> +	for (i = 0; i < NBANK(chip); i++)
> +		bitmap_set_value8(val, value[i], i * BANK_SZ);
> +
>  	return 0;
>  }
>  
> @@ -457,10 +465,7 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>  				      unsigned long *mask, unsigned long *bits)
>  {
>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	unsigned long offset;
> -	unsigned long bank_mask;
> -	int bank;
> -	u8 reg_val[MAX_BANK];
> +	DECLARE_BITMAP(reg_val, MAX_LINE);
>  	int ret;
>  
>  	mutex_lock(&chip->i2c_lock);
> @@ -468,11 +473,7 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
>  	if (ret)
>  		goto exit;
>  
> -	for_each_set_clump8(offset, bank_mask, mask, gc->ngpio) {
> -		bank = offset / 8;
> -		reg_val[bank] &= ~bank_mask;
> -		reg_val[bank] |= bitmap_get_value8(bits, offset) & bank_mask;
> -	}
> +	bitmap_and(reg_val, bits, mask, gc->ngpio);

When using set_multiple, it's expected that only the GPIO lines
requested to be set are actually set -- albeit if the hardware is
capable of that sort of control.

This bitmap_and operation is ignoring the existing state of reg_val and
overwriting it with (bits & mask), so existing GPIO states are lost and
all bits not masked are set to 0.

What you should do instead is something akin to this (but for bitmaps):

        regval &= ~mask;
        regval |= bits & mask;

That should preserve the existing GPIO states in reg_val, while setting
those requested by mask and supplied by bits.

William Breathitt Gray

>  
>  	pca953x_write_regs(chip, chip->regs->output, reg_val);
>  exit:
> @@ -599,10 +600,10 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
> -	u8 new_irqs;
> -	int level, i;
> -	u8 invert_irq_mask[MAX_BANK];
> -	u8 reg_direction[MAX_BANK];
> +	DECLARE_BITMAP(irq_mask, MAX_LINE);
> +	DECLARE_BITMAP(reg_direction, MAX_LINE);
> +	DECLARE_BITMAP(new_irqs, MAX_LINE);
> +	int level;
>  
>  	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
>  
> @@ -610,25 +611,18 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>  		/* Enable latch on interrupt-enabled inputs */
>  		pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
>  
> -		for (i = 0; i < NBANK(chip); i++)
> -			invert_irq_mask[i] = ~chip->irq_mask[i];
> +		bitmap_complement(irq_mask, chip->irq_mask, gc->ngpio);
>  
>  		/* Unmask enabled interrupts */
> -		pca953x_write_regs(chip, PCAL953X_INT_MASK, invert_irq_mask);
> +		pca953x_write_regs(chip, PCAL953X_INT_MASK, irq_mask);
>  	}
>  
> +	bitmap_or(new_irqs, chip->irq_trig_fall, chip->irq_trig_raise, gc->ngpio);
> +	bitmap_and(irq_mask, new_irqs, reg_direction, gc->ngpio);
> +
>  	/* Look for any newly setup interrupt */
> -	for (i = 0; i < NBANK(chip); i++) {
> -		new_irqs = chip->irq_trig_fall[i] | chip->irq_trig_raise[i];
> -		new_irqs &= reg_direction[i];
> -
> -		while (new_irqs) {
> -			level = __ffs(new_irqs);
> -			pca953x_gpio_direction_input(&chip->gpio_chip,
> -							level + (BANK_SZ * i));
> -			new_irqs &= ~(1 << level);
> -		}
> -	}
> +	for_each_set_bit(level, irq_mask, gc->ngpio)
> +		pca953x_gpio_direction_input(&chip->gpio_chip, level);
>  
>  	mutex_unlock(&chip->irq_lock);
>  }
> @@ -669,15 +663,15 @@ static void pca953x_irq_shutdown(struct irq_data *d)
>  	chip->irq_trig_fall[d->hwirq / BANK_SZ] &= ~mask;
>  }
>  
> -static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
> +static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pending)
>  {
> -	u8 cur_stat[MAX_BANK];
> -	u8 old_stat[MAX_BANK];
> -	bool pending_seen = false;
> -	bool trigger_seen = false;
> -	u8 trigger[MAX_BANK];
> -	u8 reg_direction[MAX_BANK];
> -	int ret, i;
> +	struct gpio_chip *gc = &chip->gpio_chip;
> +	DECLARE_BITMAP(reg_direction, MAX_LINE);
> +	DECLARE_BITMAP(old_stat, MAX_LINE);
> +	DECLARE_BITMAP(cur_stat, MAX_LINE);
> +	DECLARE_BITMAP(new_stat, MAX_LINE);
> +	DECLARE_BITMAP(trigger, MAX_LINE);
> +	int ret;
>  
>  	if (chip->driver_data & PCA_PCAL) {
>  		/* Read the current interrupt status from the device */
> @@ -690,16 +684,13 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
>  		if (ret)
>  			return false;
>  
> -		for (i = 0; i < NBANK(chip); i++) {
> -			/* Apply filter for rising/falling edge selection */
> -			pending[i] = (~cur_stat[i] & chip->irq_trig_fall[i]) |
> -				(cur_stat[i] & chip->irq_trig_raise[i]);
> -			pending[i] &= trigger[i];
> -			if (pending[i])
> -				pending_seen = true;
> -		}
> +		/* Apply filter for rising/falling edge selection */
> +		bitmap_andnot(new_stat, chip->irq_trig_fall, cur_stat, gc->ngpio);
> +		bitmap_and(old_stat, chip->irq_trig_raise, cur_stat, gc->ngpio);
> +		bitmap_or(cur_stat, old_stat, new_stat, gc->ngpio);
> +		bitmap_and(pending, cur_stat, trigger, gc->ngpio);
>  
> -		return pending_seen;
> +		return !bitmap_empty(pending, gc->ngpio);
>  	}
>  
>  	ret = pca953x_read_regs(chip, chip->regs->input, cur_stat);
> @@ -708,55 +699,40 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
>  
>  	/* Remove output pins from the equation */
>  	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
> -	for (i = 0; i < NBANK(chip); i++)
> -		cur_stat[i] &= reg_direction[i];
>  
> -	memcpy(old_stat, chip->irq_stat, NBANK(chip));
> +	bitmap_copy(old_stat, chip->irq_stat, gc->ngpio);
>  
> -	for (i = 0; i < NBANK(chip); i++) {
> -		trigger[i] = (cur_stat[i] ^ old_stat[i]) & chip->irq_mask[i];
> -		if (trigger[i])
> -			trigger_seen = true;
> -	}
> +	bitmap_and(new_stat, cur_stat, reg_direction, gc->ngpio);
> +	bitmap_xor(cur_stat, new_stat, old_stat, gc->ngpio);
> +	bitmap_and(trigger, cur_stat, chip->irq_mask, gc->ngpio);
>  
> -	if (!trigger_seen)
> +	if (bitmap_empty(trigger, gc->ngpio))
>  		return false;
>  
> -	memcpy(chip->irq_stat, cur_stat, NBANK(chip));
> +	bitmap_copy(chip->irq_stat, new_stat, gc->ngpio);
>  
> -	for (i = 0; i < NBANK(chip); i++) {
> -		pending[i] = (old_stat[i] & chip->irq_trig_fall[i]) |
> -			(cur_stat[i] & chip->irq_trig_raise[i]);
> -		pending[i] &= trigger[i];
> -		if (pending[i])
> -			pending_seen = true;
> -	}
> +	bitmap_and(cur_stat, chip->irq_trig_fall, old_stat, gc->ngpio);
> +	bitmap_and(old_stat, chip->irq_trig_raise, new_stat, gc->ngpio);
> +	bitmap_or(new_stat, old_stat, cur_stat, gc->ngpio);
> +	bitmap_and(pending, new_stat, trigger, gc->ngpio);
>  
> -	return pending_seen;
> +	return !bitmap_empty(pending, gc->ngpio);
>  }
>  
>  static irqreturn_t pca953x_irq_handler(int irq, void *devid)
>  {
>  	struct pca953x_chip *chip = devid;
> -	u8 pending[MAX_BANK];
> -	u8 level;
> -	unsigned nhandled = 0;
> -	int i;
> +	struct gpio_chip *gc = &chip->gpio_chip;
> +	DECLARE_BITMAP(pending, MAX_LINE);
> +	int level;
>  
>  	if (!pca953x_irq_pending(chip, pending))
>  		return IRQ_NONE;
>  
> -	for (i = 0; i < NBANK(chip); i++) {
> -		while (pending[i]) {
> -			level = __ffs(pending[i]);
> -			handle_nested_irq(irq_find_mapping(chip->gpio_chip.irq.domain,
> -							level + (BANK_SZ * i)));
> -			pending[i] &= ~(1 << level);
> -			nhandled++;
> -		}
> -	}
> +	for_each_set_bit(level, pending, gc->ngpio)
> +		handle_nested_irq(irq_find_mapping(gc->irq.domain, level));
>  
> -	return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE;
> +	return bitmap_empty(pending, gc->ngpio) ? IRQ_NONE : IRQ_HANDLED;
>  }
>  
>  static int pca953x_irq_setup(struct pca953x_chip *chip,
> @@ -764,8 +740,9 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>  {
>  	struct i2c_client *client = chip->client;
>  	struct irq_chip *irq_chip = &chip->irq_chip;
> -	u8 reg_direction[MAX_BANK];
> -	int ret, i;
> +	DECLARE_BITMAP(reg_direction, MAX_LINE);
> +	DECLARE_BITMAP(irq_stat, MAX_LINE);
> +	int ret;
>  
>  	if (!client->irq)
>  		return 0;
> @@ -776,7 +753,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>  	if (!(chip->driver_data & PCA_INT))
>  		return 0;
>  
> -	ret = pca953x_read_regs(chip, chip->regs->input, chip->irq_stat);
> +	ret = pca953x_read_regs(chip, chip->regs->input, irq_stat);
>  	if (ret)
>  		return ret;
>  
> @@ -786,8 +763,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>  	 * this purpose.
>  	 */
>  	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
> -	for (i = 0; i < NBANK(chip); i++)
> -		chip->irq_stat[i] &= reg_direction[i];
> +	bitmap_and(chip->irq_stat, irq_stat, reg_direction, chip->gpio_chip.ngpio);
>  	mutex_init(&chip->irq_lock);
>  
>  	ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -839,8 +815,8 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>  
>  static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
>  {
> +	DECLARE_BITMAP(val, MAX_LINE);
>  	int ret;
> -	u8 val[MAX_BANK];
>  
>  	ret = regcache_sync_region(chip->regmap, chip->regs->output,
>  				   chip->regs->output + NBANK(chip));
> @@ -854,9 +830,9 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
>  
>  	/* set platform specific polarity inversion */
>  	if (invert)
> -		memset(val, 0xFF, NBANK(chip));
> +		bitmap_fill(val, MAX_LINE);
>  	else
> -		memset(val, 0, NBANK(chip));
> +		bitmap_zero(val, MAX_LINE);
>  
>  	ret = pca953x_write_regs(chip, chip->regs->invert, val);
>  out:
> @@ -865,8 +841,8 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
>  
>  static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
>  {
> +	DECLARE_BITMAP(val, MAX_LINE);
>  	int ret;
> -	u8 val[MAX_BANK];
>  
>  	ret = device_pca95xx_init(chip, invert);
>  	if (ret)
> -- 
> 2.23.0
>
Andy Shevchenko Oct. 18, 2019, 9:21 a.m. UTC | #2
On Wed, Oct 16, 2019 at 05:06:35PM -0400, William Breathitt Gray wrote:
> On Mon, Oct 14, 2019 at 07:11:48PM +0300, Andy Shevchenko wrote:
> > Instead of customized approach convert the driver to use bitmap API.

> Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> 
> I agree with the concept of this change, but there is one fix I would
> like made detailed inline below.

Thanks!

> > -	for_each_set_clump8(offset, bank_mask, mask, gc->ngpio) {
> > -		bank = offset / 8;
> > -		reg_val[bank] &= ~bank_mask;
> > -		reg_val[bank] |= bitmap_get_value8(bits, offset) & bank_mask;
> > -	}
> > +	bitmap_and(reg_val, bits, mask, gc->ngpio);
> 
> When using set_multiple, it's expected that only the GPIO lines
> requested to be set are actually set -- albeit if the hardware is
> capable of that sort of control.
> 
> This bitmap_and operation is ignoring the existing state of reg_val and
> overwriting it with (bits & mask), so existing GPIO states are lost and
> all bits not masked are set to 0.
> 
> What you should do instead is something akin to this (but for bitmaps):
> 
>         regval &= ~mask;
>         regval |= bits & mask;
> 
> That should preserve the existing GPIO states in reg_val, while setting
> those requested by mask and supplied by bits.

Good catch! I suspected I did something wrong here, that's why I Cc'ed you,
and I wasn't mistaken — you helped me, thanks!

Patch
diff mbox series

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 10b669b8f27d..95c2d6c99f41 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -9,8 +9,7 @@ 
  */
 
 #include <linux/acpi.h>
-#include <linux/bits.h>
-#include <linux/bitops.h>
+#include <linux/bitmap.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -116,6 +115,7 @@  MODULE_DEVICE_TABLE(acpi, pca953x_acpi_ids);
 
 #define MAX_BANK 5
 #define BANK_SZ 8
+#define MAX_LINE	(MAX_BANK * BANK_SZ)
 
 #define NBANK(chip) DIV_ROUND_UP(chip->gpio_chip.ngpio, BANK_SZ)
 
@@ -147,10 +147,10 @@  struct pca953x_chip {
 
 #ifdef CONFIG_GPIO_PCA953X_IRQ
 	struct mutex irq_lock;
-	u8 irq_mask[MAX_BANK];
-	u8 irq_stat[MAX_BANK];
-	u8 irq_trig_raise[MAX_BANK];
-	u8 irq_trig_fall[MAX_BANK];
+	DECLARE_BITMAP(irq_mask, MAX_LINE);
+	DECLARE_BITMAP(irq_stat, MAX_LINE);
+	DECLARE_BITMAP(irq_trig_raise, MAX_LINE);
+	DECLARE_BITMAP(irq_trig_fall, MAX_LINE);
 	struct irq_chip irq_chip;
 #endif
 	atomic_t wakeup_path;
@@ -334,12 +334,16 @@  static u8 pca953x_recalc_addr(struct pca953x_chip *chip, int reg, int off,
 	return regaddr;
 }
 
-static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_write_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
 	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
-	int ret;
+	u8 value[MAX_BANK];
+	int i, ret;
+
+	for (i = 0; i < NBANK(chip); i++)
+		value[i] = bitmap_get_value8(val, i * BANK_SZ);
 
-	ret = regmap_bulk_write(chip->regmap, regaddr, val, NBANK(chip));
+	ret = regmap_bulk_write(chip->regmap, regaddr, value, NBANK(chip));
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed writing register\n");
 		return ret;
@@ -348,17 +352,21 @@  static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
 	return 0;
 }
 
-static int pca953x_read_regs(struct pca953x_chip *chip, int reg, u8 *val)
+static int pca953x_read_regs(struct pca953x_chip *chip, int reg, unsigned long *val)
 {
 	u8 regaddr = pca953x_recalc_addr(chip, reg, 0, false, true);
-	int ret;
+	u8 value[MAX_BANK];
+	int i, ret;
 
-	ret = regmap_bulk_read(chip->regmap, regaddr, val, NBANK(chip));
+	ret = regmap_bulk_read(chip->regmap, regaddr, value, NBANK(chip));
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "failed reading register\n");
 		return ret;
 	}
 
+	for (i = 0; i < NBANK(chip); i++)
+		bitmap_set_value8(val, value[i], i * BANK_SZ);
+
 	return 0;
 }
 
@@ -457,10 +465,7 @@  static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	unsigned long offset;
-	unsigned long bank_mask;
-	int bank;
-	u8 reg_val[MAX_BANK];
+	DECLARE_BITMAP(reg_val, MAX_LINE);
 	int ret;
 
 	mutex_lock(&chip->i2c_lock);
@@ -468,11 +473,7 @@  static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 	if (ret)
 		goto exit;
 
-	for_each_set_clump8(offset, bank_mask, mask, gc->ngpio) {
-		bank = offset / 8;
-		reg_val[bank] &= ~bank_mask;
-		reg_val[bank] |= bitmap_get_value8(bits, offset) & bank_mask;
-	}
+	bitmap_and(reg_val, bits, mask, gc->ngpio);
 
 	pca953x_write_regs(chip, chip->regs->output, reg_val);
 exit:
@@ -599,10 +600,10 @@  static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	u8 new_irqs;
-	int level, i;
-	u8 invert_irq_mask[MAX_BANK];
-	u8 reg_direction[MAX_BANK];
+	DECLARE_BITMAP(irq_mask, MAX_LINE);
+	DECLARE_BITMAP(reg_direction, MAX_LINE);
+	DECLARE_BITMAP(new_irqs, MAX_LINE);
+	int level;
 
 	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
 
@@ -610,25 +611,18 @@  static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 		/* Enable latch on interrupt-enabled inputs */
 		pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
 
-		for (i = 0; i < NBANK(chip); i++)
-			invert_irq_mask[i] = ~chip->irq_mask[i];
+		bitmap_complement(irq_mask, chip->irq_mask, gc->ngpio);
 
 		/* Unmask enabled interrupts */
-		pca953x_write_regs(chip, PCAL953X_INT_MASK, invert_irq_mask);
+		pca953x_write_regs(chip, PCAL953X_INT_MASK, irq_mask);
 	}
 
+	bitmap_or(new_irqs, chip->irq_trig_fall, chip->irq_trig_raise, gc->ngpio);
+	bitmap_and(irq_mask, new_irqs, reg_direction, gc->ngpio);
+
 	/* Look for any newly setup interrupt */
-	for (i = 0; i < NBANK(chip); i++) {
-		new_irqs = chip->irq_trig_fall[i] | chip->irq_trig_raise[i];
-		new_irqs &= reg_direction[i];
-
-		while (new_irqs) {
-			level = __ffs(new_irqs);
-			pca953x_gpio_direction_input(&chip->gpio_chip,
-							level + (BANK_SZ * i));
-			new_irqs &= ~(1 << level);
-		}
-	}
+	for_each_set_bit(level, irq_mask, gc->ngpio)
+		pca953x_gpio_direction_input(&chip->gpio_chip, level);
 
 	mutex_unlock(&chip->irq_lock);
 }
@@ -669,15 +663,15 @@  static void pca953x_irq_shutdown(struct irq_data *d)
 	chip->irq_trig_fall[d->hwirq / BANK_SZ] &= ~mask;
 }
 
-static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
+static bool pca953x_irq_pending(struct pca953x_chip *chip, unsigned long *pending)
 {
-	u8 cur_stat[MAX_BANK];
-	u8 old_stat[MAX_BANK];
-	bool pending_seen = false;
-	bool trigger_seen = false;
-	u8 trigger[MAX_BANK];
-	u8 reg_direction[MAX_BANK];
-	int ret, i;
+	struct gpio_chip *gc = &chip->gpio_chip;
+	DECLARE_BITMAP(reg_direction, MAX_LINE);
+	DECLARE_BITMAP(old_stat, MAX_LINE);
+	DECLARE_BITMAP(cur_stat, MAX_LINE);
+	DECLARE_BITMAP(new_stat, MAX_LINE);
+	DECLARE_BITMAP(trigger, MAX_LINE);
+	int ret;
 
 	if (chip->driver_data & PCA_PCAL) {
 		/* Read the current interrupt status from the device */
@@ -690,16 +684,13 @@  static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 		if (ret)
 			return false;
 
-		for (i = 0; i < NBANK(chip); i++) {
-			/* Apply filter for rising/falling edge selection */
-			pending[i] = (~cur_stat[i] & chip->irq_trig_fall[i]) |
-				(cur_stat[i] & chip->irq_trig_raise[i]);
-			pending[i] &= trigger[i];
-			if (pending[i])
-				pending_seen = true;
-		}
+		/* Apply filter for rising/falling edge selection */
+		bitmap_andnot(new_stat, chip->irq_trig_fall, cur_stat, gc->ngpio);
+		bitmap_and(old_stat, chip->irq_trig_raise, cur_stat, gc->ngpio);
+		bitmap_or(cur_stat, old_stat, new_stat, gc->ngpio);
+		bitmap_and(pending, cur_stat, trigger, gc->ngpio);
 
-		return pending_seen;
+		return !bitmap_empty(pending, gc->ngpio);
 	}
 
 	ret = pca953x_read_regs(chip, chip->regs->input, cur_stat);
@@ -708,55 +699,40 @@  static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
 
 	/* Remove output pins from the equation */
 	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
-	for (i = 0; i < NBANK(chip); i++)
-		cur_stat[i] &= reg_direction[i];
 
-	memcpy(old_stat, chip->irq_stat, NBANK(chip));
+	bitmap_copy(old_stat, chip->irq_stat, gc->ngpio);
 
-	for (i = 0; i < NBANK(chip); i++) {
-		trigger[i] = (cur_stat[i] ^ old_stat[i]) & chip->irq_mask[i];
-		if (trigger[i])
-			trigger_seen = true;
-	}
+	bitmap_and(new_stat, cur_stat, reg_direction, gc->ngpio);
+	bitmap_xor(cur_stat, new_stat, old_stat, gc->ngpio);
+	bitmap_and(trigger, cur_stat, chip->irq_mask, gc->ngpio);
 
-	if (!trigger_seen)
+	if (bitmap_empty(trigger, gc->ngpio))
 		return false;
 
-	memcpy(chip->irq_stat, cur_stat, NBANK(chip));
+	bitmap_copy(chip->irq_stat, new_stat, gc->ngpio);
 
-	for (i = 0; i < NBANK(chip); i++) {
-		pending[i] = (old_stat[i] & chip->irq_trig_fall[i]) |
-			(cur_stat[i] & chip->irq_trig_raise[i]);
-		pending[i] &= trigger[i];
-		if (pending[i])
-			pending_seen = true;
-	}
+	bitmap_and(cur_stat, chip->irq_trig_fall, old_stat, gc->ngpio);
+	bitmap_and(old_stat, chip->irq_trig_raise, new_stat, gc->ngpio);
+	bitmap_or(new_stat, old_stat, cur_stat, gc->ngpio);
+	bitmap_and(pending, new_stat, trigger, gc->ngpio);
 
-	return pending_seen;
+	return !bitmap_empty(pending, gc->ngpio);
 }
 
 static irqreturn_t pca953x_irq_handler(int irq, void *devid)
 {
 	struct pca953x_chip *chip = devid;
-	u8 pending[MAX_BANK];
-	u8 level;
-	unsigned nhandled = 0;
-	int i;
+	struct gpio_chip *gc = &chip->gpio_chip;
+	DECLARE_BITMAP(pending, MAX_LINE);
+	int level;
 
 	if (!pca953x_irq_pending(chip, pending))
 		return IRQ_NONE;
 
-	for (i = 0; i < NBANK(chip); i++) {
-		while (pending[i]) {
-			level = __ffs(pending[i]);
-			handle_nested_irq(irq_find_mapping(chip->gpio_chip.irq.domain,
-							level + (BANK_SZ * i)));
-			pending[i] &= ~(1 << level);
-			nhandled++;
-		}
-	}
+	for_each_set_bit(level, pending, gc->ngpio)
+		handle_nested_irq(irq_find_mapping(gc->irq.domain, level));
 
-	return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE;
+	return bitmap_empty(pending, gc->ngpio) ? IRQ_NONE : IRQ_HANDLED;
 }
 
 static int pca953x_irq_setup(struct pca953x_chip *chip,
@@ -764,8 +740,9 @@  static int pca953x_irq_setup(struct pca953x_chip *chip,
 {
 	struct i2c_client *client = chip->client;
 	struct irq_chip *irq_chip = &chip->irq_chip;
-	u8 reg_direction[MAX_BANK];
-	int ret, i;
+	DECLARE_BITMAP(reg_direction, MAX_LINE);
+	DECLARE_BITMAP(irq_stat, MAX_LINE);
+	int ret;
 
 	if (!client->irq)
 		return 0;
@@ -776,7 +753,7 @@  static int pca953x_irq_setup(struct pca953x_chip *chip,
 	if (!(chip->driver_data & PCA_INT))
 		return 0;
 
-	ret = pca953x_read_regs(chip, chip->regs->input, chip->irq_stat);
+	ret = pca953x_read_regs(chip, chip->regs->input, irq_stat);
 	if (ret)
 		return ret;
 
@@ -786,8 +763,7 @@  static int pca953x_irq_setup(struct pca953x_chip *chip,
 	 * this purpose.
 	 */
 	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
-	for (i = 0; i < NBANK(chip); i++)
-		chip->irq_stat[i] &= reg_direction[i];
+	bitmap_and(chip->irq_stat, irq_stat, reg_direction, chip->gpio_chip.ngpio);
 	mutex_init(&chip->irq_lock);
 
 	ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -839,8 +815,8 @@  static int pca953x_irq_setup(struct pca953x_chip *chip,
 
 static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 {
+	DECLARE_BITMAP(val, MAX_LINE);
 	int ret;
-	u8 val[MAX_BANK];
 
 	ret = regcache_sync_region(chip->regmap, chip->regs->output,
 				   chip->regs->output + NBANK(chip));
@@ -854,9 +830,9 @@  static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 
 	/* set platform specific polarity inversion */
 	if (invert)
-		memset(val, 0xFF, NBANK(chip));
+		bitmap_fill(val, MAX_LINE);
 	else
-		memset(val, 0, NBANK(chip));
+		bitmap_zero(val, MAX_LINE);
 
 	ret = pca953x_write_regs(chip, chip->regs->invert, val);
 out:
@@ -865,8 +841,8 @@  static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
 
 static int device_pca957x_init(struct pca953x_chip *chip, u32 invert)
 {
+	DECLARE_BITMAP(val, MAX_LINE);
 	int ret;
-	u8 val[MAX_BANK];
 
 	ret = device_pca95xx_init(chip, invert);
 	if (ret)