| Message ID | 20240201141406.32484-2-arturas.moskvinas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: mcp23s08: Check only GPIOs which have interrupts enabled | expand |
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!
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
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
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
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(-) base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3