RE: [PATCH v6 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with SCI

[Limonciello, Mario]

[Public]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Sunday, October 31, 2021 08:15
> To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>; Natikar, Basavaraj
> <Basavaraj.Natikar@xxxxxxx>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@xxxxxxx>; open list:PIN CONTROL SUBSYSTEM <linux-
> gpio@xxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>; ACPI Devel
> Maling List <linux-acpi@xxxxxxxxxxxxxxx>; Shah, Nehal-bakulchandra <Nehal-
> bakulchandra.Shah@xxxxxxx>; stable@xxxxxxxxxx; Joerie de Gram
> <j.de.gram@xxxxxxxxx>
> Subject: Re: [PATCH v6 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with
> SCI
> 
> On Fri, Oct 29, 2021 at 11:42 PM Mario Limonciello
> <mario.limonciello@xxxxxxx> wrote:
> >
> > On some Lenovo AMD Gen2 platforms the IRQ for the SCI and pinctrl drivers
> > are shared.  Due to how the s2idle loop handling works, this case needs
> > an extra explicit check whether the interrupt was caused by SCI or by
> > the GPIO controller.
> >
> > To fix this rework the existing IRQ handler function to function as a
> > checker and an IRQ handler depending on the calling arguments.
> >
> > Cc: stable@xxxxxxxxxx
> > BugLink:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fr
> eedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1738&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7
> C8962eb61c66843248eff08d99c708f19%7C3dd8961fe4884e608e11a82d994e18
> 3d%7C0%7C0%7C637712829517705057%7CUnknown%7CTWFpbGZsb3d8eyJWI
> joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100
> 0&amp;sdata=57LKx3moIAVwtjmncHiqDMgvYP5tkEL7JuAP76iaCHI%3D&amp;re
> served=0
> > Reported-by: Joerie de Gram <j.de.gram@xxxxxxxxx>
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > Acked-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> 
> ...
> 
> > +static bool _amd_gpio_irq_handler(int irq, void *dev_id)
> 
> I know Linus does not like leading _* in the function names, what
> about 'do_' instead?
> 
> ...
> 
> > +                       /* called from resume context on a shared IRQ, just
> > +                        * checking wake source.
> > +                        */
> 
> Is this comment aligned with the style used elsewhere in the driver code?
> 
> ...
> 
> > +                               dev_dbg(&gpio_dev->pdev->dev,
> > +                                       "Waking due to GPIO %ld: 0x%x",
> > +                                       (long)(regs + i - ((u32 __iomem *)gpio_dev->base)),
> regval);
> 
> Oy vey, these castings are ugly. The rule of thumb is that if one does
> such a thing for printf() it means something is really wrong (in 99%
> of the cases).
> 
> AFAICS you may simply use 'irqnr + i' as the other message does.
> 

Andy,

Appreciate your comments.  You're correct.  I've sent a follow up addressing them.

> ...
> 
> >         platform_set_drvdata(pdev, gpio_dev);
> > +       acpi_register_wakeup_handler(gpio_dev->irq, amd_gpio_check_wake,
> gpio_dev);
> >
> >         dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
> >         return ret;
> > @@ -1021,6 +1045,7 @@ static int amd_gpio_remove(struct platform_device
> *pdev)
> >         gpio_dev = platform_get_drvdata(pdev);
> >
> >         gpiochip_remove(&gpio_dev->gc);
> > +       acpi_unregister_wakeup_handler(amd_gpio_check_wake, gpio_dev);
> 
> Thinking about making this in the generic GPIO library code, but this
> is out of scope of the patch...

Sure, we can think about this if/when there ends up being another consumer of it.

> 
> --
> With Best Regards,
> Andy Shevchenko