Re: [PATCH 2/2] (v2) Mark MCP23S08 as one that will not sleep.

[Alexander Stein <alexander.stein@xxxxxxxxxxxxxxxxxxxxx>]

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