Message ID | 20190513120024.17026-2-joe.burmeister@devtank.co.uk |
---|---|
State | New |
Headers | show |
Series | [1/2] (v2) Expand MCP23S08 driver for use as interrupt controller. | expand |
Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister: > Though it has a 'standby' it doesn't appear to be an issue and > marking the chip with can_sleep means gpiolib.c won't allow its use > as a interrupt controller. > --- > drivers/pinctrl/pinctrl-mcp23s08.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 3fc63cb5b332..7334d8eb9135 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > return PTR_ERR(mcp->regmap); > > mcp->chip.base = base; > - mcp->chip.can_sleep = true; > + mcp->chip.can_sleep = false; > mcp->chip.parent = dev; > mcp->chip.owner = THIS_MODULE; > > IMHO this is completly wrong, please refer to the documentation of this flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/gpio/driver.h#n217 It essentially means you can't use this GPIOs from atomic context, as SPI or I2C transfers block/sleep, hence the name. In your case the IRQs are probably not requested as threaded, as stated in the link above. Best regards, Alexander
Hi Alexander, You probably noticed there is a v3, but that just adds the missing signoff. On 13/05/2019 13:59, Alexander Stein wrote: > Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister: >> Though it has a 'standby' it doesn't appear to be an issue and >> marking the chip with can_sleep means gpiolib.c won't allow its use >> as a interrupt controller. >> --- >> drivers/pinctrl/pinctrl-mcp23s08.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c >> index 3fc63cb5b332..7334d8eb9135 100644 >> --- a/drivers/pinctrl/pinctrl-mcp23s08.c >> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c >> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >> return PTR_ERR(mcp->regmap); >> >> mcp->chip.base = base; >> - mcp->chip.can_sleep = true; >> + mcp->chip.can_sleep = false; >> mcp->chip.parent = dev; >> mcp->chip.owner = THIS_MODULE; >> >> > IMHO this is completly wrong, please refer to the documentation of this flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/gpio/driver.h#n217 > It essentially means you can't use this GPIOs from atomic context, as SPI or I2C transfers block/sleep, hence the name. > In your case the IRQs are probably not requested as threaded, as stated in the link above. I should have seen that bit of docs, sorry. That opens a bit of a Pandora's box. Now I look again with a better idea of what that means, I see this isn't the only driver that has a mutex, and sets can_sleep to true, uses devm_request_threaded_irq, and called gpiochip_set_nested_irqchip/gpiochip_set_chained_irqchip. Now I'm confused because I can't see how you can use them as nested interrupt controllers. They will all cause "you cannot have chained interrupts on a chip that may sleep" from gpiochip_set_cascaded_irqchip the moment you try. I'm still reading through what I do now, but any hints or tips would be appreciated. > Best regards, > Alexander Regards, Joe
Am Montag, 13. Mai 2019, 16:02:18 CEST schrieb Joe Burmeister: > Hi Alexander, > > You probably noticed there is a v3, but that just adds the missing signoff. > > On 13/05/2019 13:59, Alexander Stein wrote: > > > Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister: > >> Though it has a 'standby' it doesn't appear to be an issue and > >> marking the chip with can_sleep means gpiolib.c won't allow its use > >> as a interrupt controller. > >> --- > >> drivers/pinctrl/pinctrl-mcp23s08.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/ pinctrl-mcp23s08.c > >> index 3fc63cb5b332..7334d8eb9135 100644 > >> --- a/drivers/pinctrl/pinctrl-mcp23s08.c > >> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > >> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > >> return PTR_ERR(mcp->regmap); > >> > >> mcp->chip.base = base; > >> - mcp->chip.can_sleep = true; > >> + mcp->chip.can_sleep = false; > >> mcp->chip.parent = dev; > >> mcp->chip.owner = THIS_MODULE; > >> > >> > > IMHO this is completly wrong, please refer to the documentation of this flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ tree/include/linux/gpio/driver.h#n217 > > It essentially means you can't use this GPIOs from atomic context, as SPI or I2C transfers block/sleep, hence the name. > > In your case the IRQs are probably not requested as threaded, as stated in the link above. > > > I should have seen that bit of docs, sorry. > > That opens a bit of a Pandora's box. > > Now I look again with a better idea of what that means, I see this isn't > the only driver that has a mutex, and sets can_sleep to true, uses > devm_request_threaded_irq, and called > gpiochip_set_nested_irqchip/gpiochip_set_chained_irqchip. > > Now I'm confused because I can't see how you can use them as nested > interrupt controllers. > > They will all cause "you cannot have chained interrupts on a chip that > may sleep" from gpiochip_set_cascaded_irqchip the moment you try. > > I'm still reading through what I do now, but any hints or tips would be > appreciated. Have a look at https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/ linux.git/tree/Documentation/driver-api/gpio/driver.rst#n309 which pretty much describes the types. I think you are confusing chained and nested IRQ controllers. Using gpiochip_set_nested_irqchip cannot result in that error. mcp23s08 driver is using gpiochip_set_nested_irqchip appropriately. I'm wondering what you are trying to resolve. Are you attaching another IRQ controller to MCP23Sxx inputs? Best regards, Alexander
Hi Alexander, On 13/05/2019 15:59, Alexander Stein wrote: > Am Montag, 13. Mai 2019, 16:02:18 CEST schrieb Joe Burmeister: >> Hi Alexander, >> >> You probably noticed there is a v3, but that just adds the missing signoff. >> >> On 13/05/2019 13:59, Alexander Stein wrote: >> >>> Am Montag, 13. Mai 2019, 14:00:24 CEST schrieb Joe Burmeister: >>>> Though it has a 'standby' it doesn't appear to be an issue and >>>> marking the chip with can_sleep means gpiolib.c won't allow its use >>>> as a interrupt controller. >>>> --- >>>> drivers/pinctrl/pinctrl-mcp23s08.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/ > pinctrl-mcp23s08.c >>>> index 3fc63cb5b332..7334d8eb9135 100644 >>>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c >>>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c >>>> @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, > struct device *dev, >>>> return PTR_ERR(mcp->regmap); >>>> >>>> mcp->chip.base = base; >>>> - mcp->chip.can_sleep = true; >>>> + mcp->chip.can_sleep = false; >>>> mcp->chip.parent = dev; >>>> mcp->chip.owner = THIS_MODULE; >>>> >>>> >>> IMHO this is completly wrong, please refer to the documentation of this > flag, e.g. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ > tree/include/linux/gpio/driver.h#n217 >>> It essentially means you can't use this GPIOs from atomic context, as SPI > or I2C transfers block/sleep, hence the name. >>> In your case the IRQs are probably not requested as threaded, as stated in > the link above. >> >> I should have seen that bit of docs, sorry. >> >> That opens a bit of a Pandora's box. >> >> Now I look again with a better idea of what that means, I see this isn't >> the only driver that has a mutex, and sets can_sleep to true, uses >> devm_request_threaded_irq, and called >> gpiochip_set_nested_irqchip/gpiochip_set_chained_irqchip. >> >> Now I'm confused because I can't see how you can use them as nested >> interrupt controllers. >> >> They will all cause "you cannot have chained interrupts on a chip that >> may sleep" from gpiochip_set_cascaded_irqchip the moment you try. >> >> I'm still reading through what I do now, but any hints or tips would be >> appreciated. > Have a look at https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/ > linux.git/tree/Documentation/driver-api/gpio/driver.rst#n309 which pretty much > describes the types. Was just reading https://www.kernel.org/doc/Documentation/gpio/driver.txt as this came through. > I think you are confusing chained and nested IRQ controllers. Using > gpiochip_set_nested_irqchip cannot result in that error. > mcp23s08 driver is using gpiochip_set_nested_irqchip appropriately. It was seeing the error was what I thought brought me to can_sleep. I assumed about can_sleep and assumed completely wrong. I've just removed this second patch and it all seams to still be working fine. Ignore this second patch, it's clearly nonsense. Sorry I wasted your time, if it's any consolation, I wasted much more of mine. Should I resubmit the series with just the first patch? > I'm wondering what you are trying to resolve. Are you attaching another IRQ > controller to MCP23Sxx inputs? We are using a MCP23S017 in a Raspberry Pi Compute Module motherboard, 4 interrupt lines go into it and 2 come out. (Though it turns out might as well be 1 as they both go into the same GPIO bank on the Pi, so same interrupt regardless.) > Best regards, > Alexander Regards, Joe
diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index 3fc63cb5b332..7334d8eb9135 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -890,7 +890,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, return PTR_ERR(mcp->regmap); mcp->chip.base = base; - mcp->chip.can_sleep = true; + mcp->chip.can_sleep = false; mcp->chip.parent = dev; mcp->chip.owner = THIS_MODULE;