| Message ID | 20240102-j7200-pcie-s2r-v1-2-84e55da52400@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > The goal is to extend the active period of pinctrl. > Some devices may need active pinctrl after suspend and/or before resume. > So move suspend/resume to suspend_noirq/resume_noirq to have active > pinctrl until suspend_noirq (included), and from resume_noirq > (included). ->...callback...() (Same comment I have given for the first patch) ... > struct pcs_device *pcs; > > - pcs = platform_get_drvdata(pdev); > + pcs = dev_get_drvdata(dev); > if (!pcs) > return -EINVAL; Drop dead code. This should be simple one line after your change. struct pcs_device *pcs = dev_get_drvdata(dev); ... > struct pcs_device *pcs; > > - pcs = platform_get_drvdata(pdev); > + pcs = dev_get_drvdata(dev); > if (!pcs) > return -EINVAL; Ditto. ... > +static const struct dev_pm_ops pinctrl_single_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, > + pinctrl_single_resume_noirq) > +}; Use proper / modern macro. ... > #endif Why ifdeferry is needed (esp. taking into account pm_ptr() use below)?
On 1/15/24 21:02, Andy Shevchenko wrote: > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> >> The goal is to extend the active period of pinctrl. >> Some devices may need active pinctrl after suspend and/or before resume. >> So move suspend/resume to suspend_noirq/resume_noirq to have active >> pinctrl until suspend_noirq (included), and from resume_noirq >> (included). > > ->...callback...() > (Same comment I have given for the first patch) fixed > > ... > >> struct pcs_device *pcs; >> >> - pcs = platform_get_drvdata(pdev); >> + pcs = dev_get_drvdata(dev); >> if (!pcs) >> return -EINVAL; > > Drop dead code. > This should be simple one line after your change. > > struct pcs_device *pcs = dev_get_drvdata(dev); > dead code dropped > ... > >> struct pcs_device *pcs; >> >> - pcs = platform_get_drvdata(pdev); >> + pcs = dev_get_drvdata(dev); >> if (!pcs) >> return -EINVAL; > > Ditto. > > ... dead code dropped > >> +static const struct dev_pm_ops pinctrl_single_pm_ops = { >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, >> + pinctrl_single_resume_noirq) >> +}; > > Use proper / modern macro. fixed, use DEFINE_NOIRQ_DEV_PM_OPS now > > ... > >> #endif > > Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? We may have an "unused variable" warning for pinctrl_single_pm_ops if CONFIG_PM is undefined (due to pm_ptr).
On Fri, Jan 19, 2024 at 6:08 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 1/15/24 21:02, Andy Shevchenko wrote: > > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: ... > >> +static const struct dev_pm_ops pinctrl_single_pm_ops = { > >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, > >> + pinctrl_single_resume_noirq) > >> +}; > > > > Use proper / modern macro. > > fixed, use DEFINE_NOIRQ_DEV_PM_OPS now ... > >> #endif > > > > Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? > > We may have an "unused variable" warning for pinctrl_single_pm_ops if > CONFIG_PM is undefined (due to pm_ptr). This is coupled with the above. Fixing above will automatically make the right thing.
On 1/19/24 17:11, Andy Shevchenko wrote: > On Fri, Jan 19, 2024 at 6:08 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> On 1/15/24 21:02, Andy Shevchenko wrote: >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard >>> <thomas.richard@bootlin.com> wrote: > > ... > >>>> +static const struct dev_pm_ops pinctrl_single_pm_ops = { >>>> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, >>>> + pinctrl_single_resume_noirq) >>>> +}; >>> >>> Use proper / modern macro. >> >> fixed, use DEFINE_NOIRQ_DEV_PM_OPS now > > ... > >>>> #endif >>> >>> Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? >> >> We may have an "unused variable" warning for pinctrl_single_pm_ops if >> CONFIG_PM is undefined (due to pm_ptr). > > This is coupled with the above. Fixing above will automatically make > the right thing. Yes you're right. By the way I can use pm_sleep_ptr instead of pm_ptr.
On Mon, Jan 22, 2024 at 4:33 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 1/19/24 17:11, Andy Shevchenko wrote: > > On Fri, Jan 19, 2024 at 6:08 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: > >> On 1/15/24 21:02, Andy Shevchenko wrote: > >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > >>> <thomas.richard@bootlin.com> wrote: ... > >>>> +static const struct dev_pm_ops pinctrl_single_pm_ops = { > >>>> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, > >>>> + pinctrl_single_resume_noirq) > >>>> +}; > >>> > >>> Use proper / modern macro. > >> > >> fixed, use DEFINE_NOIRQ_DEV_PM_OPS now > > > > ... > > > >>>> #endif > >>> > >>> Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? > >> > >> We may have an "unused variable" warning for pinctrl_single_pm_ops if > >> CONFIG_PM is undefined (due to pm_ptr). > > > > This is coupled with the above. Fixing above will automatically make > > the right thing. > > Yes you're right. > By the way I can use pm_sleep_ptr instead of pm_ptr. Yes, pm_sleep_ptr() is the correct one in this case.
On Mon, Jan 15, 2024 at 5:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > The goal is to extend the active period of pinctrl. > Some devices may need active pinctrl after suspend and/or before resume. > So move suspend/resume to suspend_noirq/resume_noirq to have active > pinctrl until suspend_noirq (included), and from resume_noirq > (included). > > The deprecated API has been removed to use the new one (dev_pm_ops struct). > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> This needs testing on OMAP and ACK from Tony before I can merge it. Preferably Haojian should test it too, this is a pretty serious semantic change. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 19cc0db771a5..4ad0f66cf42a 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1690,12 +1690,11 @@ static void pcs_restore_context(struct pcs_device *pcs) } } -static int pinctrl_single_suspend(struct platform_device *pdev, - pm_message_t state) +static int pinctrl_single_suspend_noirq(struct device *dev) { struct pcs_device *pcs; - pcs = platform_get_drvdata(pdev); + pcs = dev_get_drvdata(dev); if (!pcs) return -EINVAL; @@ -1710,11 +1709,11 @@ static int pinctrl_single_suspend(struct platform_device *pdev, return pinctrl_force_sleep(pcs->pctl); } -static int pinctrl_single_resume(struct platform_device *pdev) +static int pinctrl_single_resume_noirq(struct device *dev) { struct pcs_device *pcs; - pcs = platform_get_drvdata(pdev); + pcs = dev_get_drvdata(dev); if (!pcs) return -EINVAL; @@ -1723,6 +1722,11 @@ static int pinctrl_single_resume(struct platform_device *pdev) return pinctrl_force_default(pcs->pctl); } + +static const struct dev_pm_ops pinctrl_single_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, + pinctrl_single_resume_noirq) +}; #endif /** @@ -1986,11 +1990,8 @@ static struct platform_driver pcs_driver = { .driver = { .name = DRIVER_NAME, .of_match_table = pcs_of_match, + .pm = pm_ptr(&pinctrl_single_pm_ops), }, -#ifdef CONFIG_PM - .suspend = pinctrl_single_suspend, - .resume = pinctrl_single_resume, -#endif }; module_platform_driver(pcs_driver);
The goal is to extend the active period of pinctrl. Some devices may need active pinctrl after suspend and/or before resume. So move suspend/resume to suspend_noirq/resume_noirq to have active pinctrl until suspend_noirq (included), and from resume_noirq (included). The deprecated API has been removed to use the new one (dev_pm_ops struct). Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/pinctrl/pinctrl-single.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)