diff mbox series

[v2,02/15] pinctrl: pinctrl-single: move suspend()/resume() callbacks to noirq

Message ID 20240102-j7200-pcie-s2r-v2-2-8e4f7d228ec2@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Add suspend to ram support for PCIe on J7200 | expand

Commit Message

Thomas Richard Jan. 26, 2024, 2:36 p.m. UTC
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() in order 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 | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

Comments

Andy Shevchenko Jan. 26, 2024, 9:31 p.m. UTC | #1
On Fri, Jan 26, 2024 at 4:37 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() in order 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).

LGTM, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Andi Shyti Jan. 27, 2024, 9:53 p.m. UTC | #2
Hi Thomas,

...

> -	struct pcs_device *pcs;
> -
> -	pcs = platform_get_drvdata(pdev);
> -	if (!pcs)
> -		return -EINVAL;
> +	struct pcs_device *pcs = dev_get_drvdata(dev);

I think this cleanup can be placed in a different patch. Besides,
it's not mentioned in the commit log.

Otherwise looks good.

Andi
Linus Walleij Jan. 27, 2024, 10:31 p.m. UTC | #3
On Fri, Jan 26, 2024 at 3:37 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() in order 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>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Do you want to merge this as a series or is this something I
should just apply?

Yours,
Linus Walleij
Andi Shyti Jan. 29, 2024, 10:49 p.m. UTC | #4
Hi Linus,

On Sat, Jan 27, 2024 at 11:31:11PM +0100, Linus Walleij wrote:
> On Fri, Jan 26, 2024 at 3:37 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() in order 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>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Do you want to merge this as a series or is this something I
> should just apply?

there is still a comment from me pending.

Thanks,
Andi

> Yours,
> Linus Walleij
Tony Lindgren Feb. 5, 2024, 7:21 a.m. UTC | #5
* Andi Shyti <andi.shyti@kernel.org> [240129 22:49]:
> On Sat, Jan 27, 2024 at 11:31:11PM +0100, Linus Walleij wrote:
> > On Fri, Jan 26, 2024 at 3:37 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() in order 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>
> > 
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > Do you want to merge this as a series or is this something I
> > should just apply?
> 
> there is still a comment from me pending.

FYI I gave this a brief test and things seem to work fine for me. Sounds
like there will be another revision though so I'll test again then.

Regards,

Tony
Thomas Richard Feb. 8, 2024, 3:52 p.m. UTC | #6
On 1/29/24 23:49, Andi Shyti wrote:
> Hi Linus,
> 
> On Sat, Jan 27, 2024 at 11:31:11PM +0100, Linus Walleij wrote:
>> On Fri, Jan 26, 2024 at 3:37 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() in order 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>
>>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Do you want to merge this as a series or is this something I
>> should just apply?
> 
> there is still a comment from me pending.

Hi Andi,

Based on your comment, for the next iteration, I will move the cleanup
in a dedicated patch.

@Linus, you can apply pinctrl patches once everything is ok for you.

Regards,
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 19cc0db771a5..0dd4b0e11adf 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1625,7 +1625,6 @@  static int pcs_irq_init_chained_handler(struct pcs_device *pcs,
 	return 0;
 }
 
-#ifdef CONFIG_PM
 static int pcs_save_context(struct pcs_device *pcs)
 {
 	int i, mux_bytes;
@@ -1690,14 +1689,9 @@  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);
-	if (!pcs)
-		return -EINVAL;
+	struct pcs_device *pcs = dev_get_drvdata(dev);
 
 	if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
 		int ret;
@@ -1710,20 +1704,19 @@  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);
-	if (!pcs)
-		return -EINVAL;
+	struct pcs_device *pcs = dev_get_drvdata(dev);
 
 	if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
 		pcs_restore_context(pcs);
 
 	return pinctrl_force_default(pcs->pctl);
 }
-#endif
+
+static DEFINE_NOIRQ_DEV_PM_OPS(pinctrl_single_pm_ops,
+			       pinctrl_single_suspend_noirq,
+			       pinctrl_single_resume_noirq);
 
 /**
  * pcs_quirk_missing_pinctrl_cells - handle legacy binding
@@ -1986,11 +1979,8 @@  static struct platform_driver pcs_driver = {
 	.driver = {
 		.name		= DRIVER_NAME,
 		.of_match_table	= pcs_of_match,
+		.pm = pm_sleep_ptr(&pinctrl_single_pm_ops),
 	},
-#ifdef CONFIG_PM
-	.suspend = pinctrl_single_suspend,
-	.resume = pinctrl_single_resume,
-#endif
 };
 
 module_platform_driver(pcs_driver);