Hi Phil,

On 18-09-27 21:05, Phil Reid wrote:

G'day Marco,

On 25/09/2018 10:56 PM, Marco Felsch wrote:

...

Since 'commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' the irq request isn't the last devm_* allocation. Without a deeper look at the irq and testing this isn't a good solution. Since this driver relies on the devm mechanism, requesting a interrupt should be the last thing to avoid memory corruptions during unbinding.

'Commit 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order")' fixed the order for the interrupt-controller use case only. The mcp23s08_irq_setup() must be split into two to fix it for the interrupt-controller use case and to register the irq at last. So the irq will be freed first during unbind.

I'm no expert on the irq's, but after a bit of reading the patch makes sense to me. I've got one question below.

Thanks for review.

...

Cc: stable@vger.kernel.org Cc: Dmitry Mastykin mastichi@gmail.com Cc: Sebastian Reichel sebastian.reichel@collabora.co.uk Fixes: 82039d244f87 ("pinctrl: mcp23s08: add pinconf support") Fixes: 02e389e63e35 ("pinctrl: mcp23s08: fix irq setup order") Signed-off-by: Marco Felsch m.felsch@pengutronix.de

---

drivers/pinctrl/pinctrl-mcp23s08.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index 472746931ea8..367b648be7c7 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -636,6 +636,14 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) return err; }

• return 0;

+}

+static int mcp23s08_irqchip_setup(struct mcp23s08 *mcp) +{

• struct gpio_chip *chip = &mcp->chip;
• int err;
• err = gpiochip_irqchip_add_nested(chip, &mcp23s08_irq_chip, 0,

@@ -908,8 +916,8 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, goto fail; }

• if (mcp->irq && mcp->irq_controller) {

• ret = mcp23s08_irq_setup(mcp);
  

• if (mcp->irq_controller) {

• ret = mcp23s08_irqchip_setup(mcp);
  

The condition check changes, which may make sense to someone more knowledgeable., but gpiochip_set_nested_irqchip in mcp23s08_irqchip_setup references mcp->irq as well so I'm a little confused.

You're right, it make no sense to setup a irqchip without having a upstream irq. I will fix this for a v2.

...

if (ret)
	goto fail;

} @@ -941,6 +949,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, goto fail; }

• if (mcp->irq)

• ret = mcp23s08_irq_setup(mcp);
  

Is there any point if it's not a irq_controller?

AFAK, if it is a irqchip/irq_controller it can be used as a irq-parent for other devices. The irq itself can be setup without irq_controller must be set. E.g. Inform the System about a changed gpio state.

Kind regards, Marco

...

fail: if (ret < 0) dev_dbg(dev, "can't setup chip %d, --> %d\n", addr, ret);

Otherwise LGTM.

-- Regards Phil Reid