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://gitlab.freedesktop.org/drm/amd/-/issues/1738 > 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. ... > 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... -- With Best Regards, Andy Shevchenko
