[PATCH] pinctrl: mcp23s08: Check only GPIOs which have interrupts enabled

[PATCH] pinctrl: mcp23s08: Check only GPIOs which have interrupts enabled

[Arturas Moskvinas]

GPINTEN register contains information about GPIOs with enabled
interrupts no need to check other GPIOs for changes.

Signed-off-by: Arturas Moskvinas <arturas.moskvinas@gmail.com>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 4551575e4e7d..bfa933284e84 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -375,7 +375,8 @@ mcp23s08_direction_output(struct gpio_chip *chip, unsigned offset, int value)
 static irqreturn_t mcp23s08_irq(int irq, void *data)
 {
 	struct mcp23s08 *mcp = data;
-	int intcap, intcon, intf, i, gpio, gpio_orig, intcap_mask, defval;
+	int intcap, intcon, intf, i, gpio, gpio_orig, intcap_mask, defval, gpinten;
+	unsigned long int enabled_interrupts;
 	unsigned int child_irq;
 	bool intf_set, intcap_changed, gpio_bit_changed,
 		defval_changed, gpio_set;
@@ -395,6 +396,10 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 	if (mcp_read(mcp, MCP_INTCON, &intcon))
 		goto unlock;
 
+	if (mcp_read(mcp, MCP_GPINTEN, &gpinten))
+		goto unlock;
+	enabled_interrupts = gpinten;
+
 	if (mcp_read(mcp, MCP_DEFVAL, &defval))
 		goto unlock;
 
@@ -410,9 +415,10 @@ static irqreturn_t mcp23s08_irq(int irq, void *data)
 		"intcap 0x%04X intf 0x%04X gpio_orig 0x%04X gpio 0x%04X\n",
 		intcap, intf, gpio_orig, gpio);
 
-	for (i = 0; i < mcp->chip.ngpio; i++) {
-		/* We must check all of the inputs on the chip,
-		 * otherwise we may not notice a change on >=2 pins.
+	for_each_set_bit(i, &enabled_interrupts, mcp->chip.ngpio) {
+		/* We must check all of the inputs with enabled interrupts
+		 * on the chip, otherwise we may not notice a change
+		 * on >=2 pins.
 		 *
 		 * On at least the mcp23s17, INTCAP is only updated
 		 * one byte at a time(INTCAPA and INTCAPB are

base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.43.0

Re: [PATCH] pinctrl: mcp23s08: Check only GPIOs which have interrupts enabled

[Linus Walleij]

On Tue, Jan 30, 2024 at 8:37 AM Arturas Moskvinas
<arturas.moskvinas@gmail.com> wrote:

> GPINTEN register contains information about GPIOs with enabled
> interrupts no need to check other GPIOs for changes.
>
> Signed-off-by: Arturas Moskvinas <arturas.moskvinas@gmail.com>

This driver has a lot of users, so adding some reviewers.
https://lore.kernel.org/linux-gpio/20240130073710.10110-1-arturas.moskvinas@gmail.com/

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mcp23s08: Check only GPIOs which have interrupts enabled

[Andy Shevchenko]

On Tue, Jan 30, 2024 at 09:37:10AM +0200, Arturas Moskvinas wrote:
> GPINTEN register contains information about GPIOs with enabled
> interrupts no need to check other GPIOs for changes.

...

> +	if (mcp_read(mcp, MCP_GPINTEN, &gpinten))
> +		goto unlock;

> +	enabled_interrupts = gpinten;

Move this line to be...

...

> -	for (i = 0; i < mcp->chip.ngpio; i++) {
> -		/* We must check all of the inputs on the chip,
> -		 * otherwise we may not notice a change on >=2 pins.

...just here (w/o any blank line in between).

> +	for_each_set_bit(i, &enabled_interrupts, mcp->chip.ngpio) {

...

> +		/* We must check all of the inputs with enabled interrupts
> +		 * on the chip, otherwise we may not notice a change
> +		 * on >=2 pins.

Missing space after =. But better to spell in proper English, i.e.
"...great than or equal to 2 pins."

>  		 *
>  		 * On at least the mcp23s17, INTCAP is only updated

/*
 * Use proper multi-line
 * comment style as depicted
 * in this example.
 */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: mcp23s08: Check only GPIOs which have interrupts enabled

[Andy Shevchenko]

On Thu, Feb 01, 2024 at 02:19:38PM +0200, Arturas Moskvinas wrote:
> On Thu, Feb 1, 2024 at 2:03 PM Andy Shevchenko <andriy.shevchenko@intel.com>
> wrote:

...

> > > +             /* We must check all of the inputs with enabled interrupts
> > > +              * on the chip, otherwise we may not notice a change
> > > +              * on >=2 pins.
> >
> > Missing space after =. But better to spell in proper English, i.e.
> > "...great than or equal to 2 pins."
> >
> +               /*
> +                * We must check all of the inputs with enabled interrupts
> +                * on the chip, otherwise we may not notice a change
> +                * on more than one pin.
> 
> Does this sound better?

LGTM, thanks.

-- 
With Best Regards,
Andy Shevchenko