From: "Limonciello, Mario" <Mario.Limonciello@amd.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
"Natikar, Basavaraj" <Basavaraj.Natikar@amd.com>,
"S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>,
"open list:PIN CONTROL SUBSYSTEM" <linux-gpio@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>,
ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
"Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@amd.com>,
"stable@kernel.org" <stable@kernel.org>,
Joerie de Gram <j.de.gram@gmail.com>
Subject: RE: [PATCH v6 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with SCI
Date: Mon, 1 Nov 2021 01:50:29 +0000 [thread overview]
Message-ID: <SA0PR12MB45104B9765BF4C693A5B2D7EE28A9@SA0PR12MB4510.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CAHp75VfOBBpt84aQYGOW32kKqeNH5gZUbJtqf1sPP7O-ZZkW+g@mail.gmail.com>
[Public]
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Sunday, October 31, 2021 08:15
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Natikar, Basavaraj
> <Basavaraj.Natikar@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@amd.com>; open list:PIN CONTROL SUBSYSTEM <linux-
> gpio@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; ACPI Devel
> Maling List <linux-acpi@vger.kernel.org>; Shah, Nehal-bakulchandra <Nehal-
> bakulchandra.Shah@amd.com>; stable@kernel.org; Joerie de Gram
> <j.de.gram@gmail.com>
> 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@amd.com> 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@kernel.org
> > BugLink:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fr
> eedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1738&data=04%7C01%7Cmario.limonciello%40amd.com%7
> C8962eb61c66843248eff08d99c708f19%7C3dd8961fe4884e608e11a82d994e18
> 3d%7C0%7C0%7C637712829517705057%7CUnknown%7CTWFpbGZsb3d8eyJWI
> joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100
> 0&sdata=57LKx3moIAVwtjmncHiqDMgvYP5tkEL7JuAP76iaCHI%3D&re
> served=0
> > Reported-by: Joerie de Gram <j.de.gram@gmail.com>
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > Acked-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>
> ...
>
> > +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
prev parent reply other threads:[~2021-11-01 1:50 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-29 20:40 [PATCH v6 1/2] ACPI: Add stubs for wakeup handler functions Mario Limonciello
2021-10-29 20:40 ` [PATCH v6 2/2] pinctrl: amd: Fix wakeups when IRQ is shared with SCI Mario Limonciello
2021-10-31 13:15 ` Andy Shevchenko
2021-11-01 1:50 ` Limonciello, Mario [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SA0PR12MB45104B9765BF4C693A5B2D7EE28A9@SA0PR12MB4510.namprd12.prod.outlook.com \
--to=mario.limonciello@amd.com \
--cc=Basavaraj.Natikar@amd.com \
--cc=Nehal-bakulchandra.Shah@amd.com \
--cc=Shyam-sundar.S-k@amd.com \
--cc=andy.shevchenko@gmail.com \
--cc=j.de.gram@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=stable@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).