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

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

[Andy Shevchenko]

On Fri, Feb 02, 2024 at 12:40:23PM +0200, Arturas Moskvinas wrote:
> 
> On 2/1/24 16:30, Andy Shevchenko wrote:
> > On Thu, Feb 01, 2024 at 04:14:07PM +0200, Arturas Moskvinas 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>
> > > ---
> > You forgot to add a changelog here, but no need to resend, just you can respond
> > to the email since it's not a big issue in this case.
> Ack.
> > > +	if (mcp_read(mcp, MCP_GPINTEN, &gpinten))
> > > +		goto unlock;
> > Do all hw variants have this register available?
> > Esp. I2C part, wouldn't it be problematic (exception with NACK on the bus)?
> According to specification sheets MCP(s0)17 [1] page 19, MCP(s0)18 [2] page
> 19, MCP(s0)08 [3] page 11 - all supported expanders have that register also
> that register needs to be used [4] to mask/unmask interrupts on given GPIO,
> without it - expander won't even fire an interrupt. I tested on MCP23018 I2C
> expander though but module itself is not treating that expander differently
> for interrupt handling purposes.

Thank you for the clarification!

> Do you want that information to be added as part of commit message or
> information in the mailing thread will be enough?

Up to maintainers. I'm fine with this email. If Linus uses b4 to generate
a Link tag to this discussion it will be enough (in my opinion).

> [1] https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP23017-Data-Sheet-DS20001952.pdf
> [2] https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP23018-Data-Sheet-DS20002103.pdf
> [3] https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP23008-MCP23S08-Data-Sheet-DS20001919.pdf
> [4] https://elixir.bootlin.com/linux/v6.7/source/drivers/pinctrl/pinctrl-mcp23s08.c#L473

-- 
With Best Regards,
Andy Shevchenko

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

[Arturas Moskvinas]

On 2/5/24 15:56, Andy Shevchenko wrote:
>> Do you want that information to be added as part of commit message or
>> information in the mailing thread will be enough?
> Up to maintainers. I'm fine with this email. If Linus uses b4 to generate
> a Link tag to this discussion it will be enough (in my opinion).

Is there anything left for me to update/change? Or we're waiting for 
other reviewers and/or Linus to chime in?

Arturas Moskvinas

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

[Andy Shevchenko]

On Wed, Feb 07, 2024 at 12:50:51PM +0200, Arturas Moskvinas wrote:
> On 2/5/24 15:56, Andy Shevchenko wrote:
> > > Do you want that information to be added as part of commit message or
> > > information in the mailing thread will be enough?
> > Up to maintainers. I'm fine with this email. If Linus uses b4 to generate
> > a Link tag to this discussion it will be enough (in my opinion).
> 
> Is there anything left for me to update/change? Or we're waiting for other
> reviewers and/or Linus to chime in?

I believe we are waiting for Linus' decision.

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Thu, Feb 1, 2024 at 3:15 PM 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>

LGTM, patch applied!
As Andy points out, the commit now links to the discussion thread so
people can find the details if need be.

Yours,
Linus Walleij

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

[Arturas Moskvinas]

On 2/1/24 16:30, Andy Shevchenko wrote:
> On Thu, Feb 01, 2024 at 04:14:07PM +0200, Arturas Moskvinas 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>
>> ---
> You forgot to add a changelog here, but no need to resend, just you can respond
> to the email since it's not a big issue in this case.
Ack.
>> +	if (mcp_read(mcp, MCP_GPINTEN, &gpinten))
>> +		goto unlock;
> Do all hw variants have this register available?
> Esp. I2C part, wouldn't it be problematic (exception with NACK on the bus)?
According to specification sheets MCP(s0)17 [1] page 19, MCP(s0)18 [2] 
page 19, MCP(s0)08 [3] page 11 - all supported expanders have that 
register also that register needs to be used [4] to mask/unmask 
interrupts on given GPIO, without it - expander won't even fire an 
interrupt. I tested on MCP23018 I2C expander though but module itself is 
not treating that expander differently for interrupt handling purposes.

Do you want that information to be added as part of commit message or 
information in the mailing thread will be enough?

[1] 
https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP23017-Data-Sheet-DS20001952.pdf
[2] 
https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP23018-Data-Sheet-DS20002103.pdf
[3] 
https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP23008-MCP23S08-Data-Sheet-DS20001919.pdf
[4] 
https://elixir.bootlin.com/linux/v6.7/source/drivers/pinctrl/pinctrl-mcp23s08.c#L473

Arturas Moskvinas

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

[Andy Shevchenko]

On Thu, Feb 01, 2024 at 04:14:07PM +0200, Arturas Moskvinas 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>
> ---

You forgot to add a changelog here, but no need to resend, just you can respond
to the email since it's not a big issue in this case.

...

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

Do all hw variants have this register available?
Esp. I2C part, wouldn't it be problematic (exception with NACK on the bus)?

...

The rest LGTM, thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] 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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 4551575e4e7d..38c3a14c8b58 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,9 @@ 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;
+
 	if (mcp_read(mcp, MCP_DEFVAL, &defval))
 		goto unlock;
 
@@ -410,9 +414,12 @@ 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.
+	enabled_interrupts = gpinten;
+	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 more than one pin.
 		 *
 		 * On at least the mcp23s17, INTCAP is only updated
 		 * one byte at a time(INTCAPA and INTCAPB are

base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
-- 
2.43.0