[Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Vaibhav Gupta]

Drivers using legacy PM have to manage PCI states and device's PM states
themselves. They also need to take care of configuration registers.

With improved and powerful support of generic PM, PCI Core takes care of
above mentioned, device-independent, jobs.

This driver makes use of PCI helper functions like
pci_save/restore_state(), pci_enable/disable_device(),
pci_enable_wake() and pci_set_power_state() to do required operations. In
generic mode, they are no longer needed.

Change function parameter in both .suspend() and .resume() to
"struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
"struct pci_dev*" variable and drv data.

Compile-tested only.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index 73f139690e4e..c0ddc4cc2ce7 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
 	kfree(adap_info);
 }
 
-#ifdef CONFIG_PM
-static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused pch_i2c_suspend(struct device *dev)
 {
-	int ret;
 	int i;
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct adapter_info *adap_info = pci_get_drvdata(pdev);
 	void __iomem *p = adap_info->pch_data[0].pch_base_address;
 
@@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
 		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
 		ioread32(p + PCH_I2CESRSTA));
 
-	ret = pci_save_state(pdev);
-
-	if (ret) {
-		pch_pci_err(pdev, "pci_save_state\n");
-		return ret;
-	}
-
-	pci_enable_wake(pdev, PCI_D3hot, 0);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	device_wakeup_disable(dev);
 
 	return 0;
 }
 
-static int pch_i2c_resume(struct pci_dev *pdev)
+static int __maybe_unused pch_i2c_resume(struct device *dev)
 {
 	int i;
-	struct adapter_info *adap_info = pci_get_drvdata(pdev);
-
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
+	struct adapter_info *adap_info = dev_get_drvdata(dev);
 
-	if (pci_enable_device(pdev) < 0) {
-		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
-		return -EIO;
-	}
-
-	pci_enable_wake(pdev, PCI_D3hot, 0);
+	device_wakeup_disable(dev);
 
 	for (i = 0; i < adap_info->ch_num; i++)
 		pch_i2c_init(&adap_info->pch_data[i]);
@@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
 
 	return 0;
 }
-#else
-#define pch_i2c_suspend NULL
-#define pch_i2c_resume NULL
-#endif
+
+static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
 
 static struct pci_driver pch_pcidriver = {
 	.name = KBUILD_MODNAME,
 	.id_table = pch_pcidev_id,
 	.probe = pch_i2c_probe,
 	.remove = pch_i2c_remove,
-	.suspend = pch_i2c_suspend,
-	.resume = pch_i2c_resume
+	.driver.pm = &pch_i2c_pm_ops,
 };
 
 module_pci_driver(pch_pcidriver);
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 3564 bytes --]

On Mon, Jul 20, 2020 at 07:30:32PM +0530, Vaibhav Gupta wrote:
> Drivers using legacy PM have to manage PCI states and device's PM states
> themselves. They also need to take care of configuration registers.
> 
> With improved and powerful support of generic PM, PCI Core takes care of
> above mentioned, device-independent, jobs.
> 
> This driver makes use of PCI helper functions like
> pci_save/restore_state(), pci_enable/disable_device(),
> pci_enable_wake() and pci_set_power_state() to do required operations. In
> generic mode, they are no longer needed.
> 
> Change function parameter in both .suspend() and .resume() to
> "struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
> "struct pci_dev*" variable and drv data.
> 
> Compile-tested only.
> 
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Looks reasonable, but to make sure I'd like a test from someone with HW
or an ack from someone more experienced with PCI details.

> ---
>  drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
>  1 file changed, 9 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> index 73f139690e4e..c0ddc4cc2ce7 100644
> --- a/drivers/i2c/busses/i2c-eg20t.c
> +++ b/drivers/i2c/busses/i2c-eg20t.c
> @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
>  	kfree(adap_info);
>  }
>  
> -#ifdef CONFIG_PM
> -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused pch_i2c_suspend(struct device *dev)
>  {
> -	int ret;
>  	int i;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
>  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
>  
> @@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
>  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
>  		ioread32(p + PCH_I2CESRSTA));
>  
> -	ret = pci_save_state(pdev);
> -
> -	if (ret) {
> -		pch_pci_err(pdev, "pci_save_state\n");
> -		return ret;
> -	}
> -
> -	pci_enable_wake(pdev, PCI_D3hot, 0);
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +	device_wakeup_disable(dev);
>  
>  	return 0;
>  }
>  
> -static int pch_i2c_resume(struct pci_dev *pdev)
> +static int __maybe_unused pch_i2c_resume(struct device *dev)
>  {
>  	int i;
> -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> -
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_restore_state(pdev);
> +	struct adapter_info *adap_info = dev_get_drvdata(dev);
>  
> -	if (pci_enable_device(pdev) < 0) {
> -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> -		return -EIO;
> -	}
> -
> -	pci_enable_wake(pdev, PCI_D3hot, 0);
> +	device_wakeup_disable(dev);
>  
>  	for (i = 0; i < adap_info->ch_num; i++)
>  		pch_i2c_init(&adap_info->pch_data[i]);
> @@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> -#else
> -#define pch_i2c_suspend NULL
> -#define pch_i2c_resume NULL
> -#endif
> +
> +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
>  
>  static struct pci_driver pch_pcidriver = {
>  	.name = KBUILD_MODNAME,
>  	.id_table = pch_pcidev_id,
>  	.probe = pch_i2c_probe,
>  	.remove = pch_i2c_remove,
> -	.suspend = pch_i2c_suspend,
> -	.resume = pch_i2c_resume
> +	.driver.pm = &pch_i2c_pm_ops,
>  };
>  
>  module_pci_driver(pch_pcidriver);

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Bjorn Helgaas]

[+cc Tomoya, Linus, Qi, Ben from e9bc8fa5df1c]

On Mon, Jul 20, 2020 at 07:30:32PM +0530, Vaibhav Gupta wrote:
> Drivers using legacy PM have to manage PCI states and device's PM states
> themselves. They also need to take care of configuration registers.
> 
> With improved and powerful support of generic PM, PCI Core takes care of
> above mentioned, device-independent, jobs.
> 
> This driver makes use of PCI helper functions like
> pci_save/restore_state(), pci_enable/disable_device(),
> pci_enable_wake() and pci_set_power_state() to do required operations. In
> generic mode, they are no longer needed.
> 
> Change function parameter in both .suspend() and .resume() to
> "struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
> "struct pci_dev*" variable and drv data.
> 
> Compile-tested only.
> 
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> ---
>  drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
>  1 file changed, 9 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> index 73f139690e4e..c0ddc4cc2ce7 100644
> --- a/drivers/i2c/busses/i2c-eg20t.c
> +++ b/drivers/i2c/busses/i2c-eg20t.c
> @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
>  	kfree(adap_info);
>  }
>  
> -#ifdef CONFIG_PM
> -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused pch_i2c_suspend(struct device *dev)
>  {
> -	int ret;
>  	int i;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct adapter_info *adap_info = pci_get_drvdata(pdev);

Why don't you use "adap_info = dev_get_drvdata(dev)" as you did below,
so you don't need to_pci_dev()?

>  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
>  
> @@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
>  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
>  		ioread32(p + PCH_I2CESRSTA));
>  
> -	ret = pci_save_state(pdev);
> -
> -	if (ret) {
> -		pch_pci_err(pdev, "pci_save_state\n");
> -		return ret;
> -	}
> -
> -	pci_enable_wake(pdev, PCI_D3hot, 0);
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +	device_wakeup_disable(dev);
>  
>  	return 0;
>  }
>  
> -static int pch_i2c_resume(struct pci_dev *pdev)
> +static int __maybe_unused pch_i2c_resume(struct device *dev)
>  {
>  	int i;
> -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> -
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_restore_state(pdev);
> +	struct adapter_info *adap_info = dev_get_drvdata(dev);
>  
> -	if (pci_enable_device(pdev) < 0) {
> -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> -		return -EIO;
> -	}
> -
> -	pci_enable_wake(pdev, PCI_D3hot, 0);
> +	device_wakeup_disable(dev);

It *looks* wrong to disable wakeup in both suspend and resume.  I
think the usual pattern is to enable wakeup in suspend and disable it
in resume.

But it looks like it's been that way since the driver was added by
e9bc8fa5df1c ("i2c-eg20t: add driver for Intel EG20T").

If the device doesn't support wakeup, I would not expect the driver to
mention wakeup at all.

In any case, I think it's the right thing for *this* patch to preserve
the previous wakeup behavior.  Maybe we want a follow-up patch to just
remove both device_wakeup_disable() calls?

>  	for (i = 0; i < adap_info->ch_num; i++)
>  		pch_i2c_init(&adap_info->pch_data[i]);
> @@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> -#else
> -#define pch_i2c_suspend NULL
> -#define pch_i2c_resume NULL
> -#endif
> +
> +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
>  
>  static struct pci_driver pch_pcidriver = {
>  	.name = KBUILD_MODNAME,
>  	.id_table = pch_pcidev_id,
>  	.probe = pch_i2c_probe,
>  	.remove = pch_i2c_remove,
> -	.suspend = pch_i2c_suspend,
> -	.resume = pch_i2c_resume
> +	.driver.pm = &pch_i2c_pm_ops,
>  };
>  
>  module_pci_driver(pch_pcidriver);
> -- 
> 2.27.0
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Bjorn Helgaas]

[update Linus's address, drop Qi's (bounced)]

On Wed, Aug 05, 2020 at 10:23:31AM -0500, Bjorn Helgaas wrote:
> [+cc Tomoya, Linus, Qi, Ben from e9bc8fa5df1c]
> 
> On Mon, Jul 20, 2020 at 07:30:32PM +0530, Vaibhav Gupta wrote:
> > Drivers using legacy PM have to manage PCI states and device's PM states
> > themselves. They also need to take care of configuration registers.
> > 
> > With improved and powerful support of generic PM, PCI Core takes care of
> > above mentioned, device-independent, jobs.
> > 
> > This driver makes use of PCI helper functions like
> > pci_save/restore_state(), pci_enable/disable_device(),
> > pci_enable_wake() and pci_set_power_state() to do required operations. In
> > generic mode, they are no longer needed.
> > 
> > Change function parameter in both .suspend() and .resume() to
> > "struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
> > "struct pci_dev*" variable and drv data.
> > 
> > Compile-tested only.
> > 
> > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
> >  1 file changed, 9 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> > index 73f139690e4e..c0ddc4cc2ce7 100644
> > --- a/drivers/i2c/busses/i2c-eg20t.c
> > +++ b/drivers/i2c/busses/i2c-eg20t.c
> > @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
> >  	kfree(adap_info);
> >  }
> >  
> > -#ifdef CONFIG_PM
> > -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __maybe_unused pch_i2c_suspend(struct device *dev)
> >  {
> > -	int ret;
> >  	int i;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> 
> Why don't you use "adap_info = dev_get_drvdata(dev)" as you did below,
> so you don't need to_pci_dev()?
> 
> >  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
> >  
> > @@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> >  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
> >  		ioread32(p + PCH_I2CESRSTA));
> >  
> > -	ret = pci_save_state(pdev);
> > -
> > -	if (ret) {
> > -		pch_pci_err(pdev, "pci_save_state\n");
> > -		return ret;
> > -	}
> > -
> > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > -	pci_disable_device(pdev);
> > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > +	device_wakeup_disable(dev);
> >  
> >  	return 0;
> >  }
> >  
> > -static int pch_i2c_resume(struct pci_dev *pdev)
> > +static int __maybe_unused pch_i2c_resume(struct device *dev)
> >  {
> >  	int i;
> > -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > -
> > -	pci_set_power_state(pdev, PCI_D0);
> > -	pci_restore_state(pdev);
> > +	struct adapter_info *adap_info = dev_get_drvdata(dev);
> >  
> > -	if (pci_enable_device(pdev) < 0) {
> > -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> > -		return -EIO;
> > -	}
> > -
> > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > +	device_wakeup_disable(dev);
> 
> It *looks* wrong to disable wakeup in both suspend and resume.  I
> think the usual pattern is to enable wakeup in suspend and disable it
> in resume.
> 
> But it looks like it's been that way since the driver was added by
> e9bc8fa5df1c ("i2c-eg20t: add driver for Intel EG20T").
> 
> If the device doesn't support wakeup, I would not expect the driver to
> mention wakeup at all.
> 
> In any case, I think it's the right thing for *this* patch to preserve
> the previous wakeup behavior.  Maybe we want a follow-up patch to just
> remove both device_wakeup_disable() calls?
> 
> >  	for (i = 0; i < adap_info->ch_num; i++)
> >  		pch_i2c_init(&adap_info->pch_data[i]);
> > @@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
> >  
> >  	return 0;
> >  }
> > -#else
> > -#define pch_i2c_suspend NULL
> > -#define pch_i2c_resume NULL
> > -#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
> >  
> >  static struct pci_driver pch_pcidriver = {
> >  	.name = KBUILD_MODNAME,
> >  	.id_table = pch_pcidev_id,
> >  	.probe = pch_i2c_probe,
> >  	.remove = pch_i2c_remove,
> > -	.suspend = pch_i2c_suspend,
> > -	.resume = pch_i2c_resume
> > +	.driver.pm = &pch_i2c_pm_ops,
> >  };
> >  
> >  module_pci_driver(pch_pcidriver);
> > -- 
> > 2.27.0
> > 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Vaibhav Gupta]

On Wed, Aug 05, 2020 at 10:28:32AM -0500, Bjorn Helgaas wrote:
> [update Linus's address, drop Qi's (bounced)]
> 
> On Wed, Aug 05, 2020 at 10:23:31AM -0500, Bjorn Helgaas wrote:
> > [+cc Tomoya, Linus, Qi, Ben from e9bc8fa5df1c]
> > 
> > On Mon, Jul 20, 2020 at 07:30:32PM +0530, Vaibhav Gupta wrote:
> > > Drivers using legacy PM have to manage PCI states and device's PM states
> > > themselves. They also need to take care of configuration registers.
> > > 
> > > With improved and powerful support of generic PM, PCI Core takes care of
> > > above mentioned, device-independent, jobs.
> > > 
> > > This driver makes use of PCI helper functions like
> > > pci_save/restore_state(), pci_enable/disable_device(),
> > > pci_enable_wake() and pci_set_power_state() to do required operations. In
> > > generic mode, they are no longer needed.
> > > 
> > > Change function parameter in both .suspend() and .resume() to
> > > "struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
> > > "struct pci_dev*" variable and drv data.
> > > 
> > > Compile-tested only.
> > > 
> > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > ---
> > >  drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
> > >  1 file changed, 9 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> > > index 73f139690e4e..c0ddc4cc2ce7 100644
> > > --- a/drivers/i2c/busses/i2c-eg20t.c
> > > +++ b/drivers/i2c/busses/i2c-eg20t.c
> > > @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
> > >  	kfree(adap_info);
> > >  }
> > >  
> > > -#ifdef CONFIG_PM
> > > -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > > +static int __maybe_unused pch_i2c_suspend(struct device *dev)
> > >  {
> > > -	int ret;
> > >  	int i;
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > >  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > 
> > Why don't you use "adap_info = dev_get_drvdata(dev)" as you did below,
> > so you don't need to_pci_dev()?
> >
Actually, line 870, pch_pci_dbg() again needs "struct pci_dev*" type pointer.
Either way I had to use to_pci_dev(), so defined "pdev" which made less number
of required changes in the code.
> > >  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
> > >  
> > > @@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > >  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
> > >  		ioread32(p + PCH_I2CESRSTA));
> > >  
> > > -	ret = pci_save_state(pdev);
> > > -
> > > -	if (ret) {
> > > -		pch_pci_err(pdev, "pci_save_state\n");
> > > -		return ret;
> > > -	}
> > > -
> > > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > > -	pci_disable_device(pdev);
> > > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > > +	device_wakeup_disable(dev);
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > -static int pch_i2c_resume(struct pci_dev *pdev)
> > > +static int __maybe_unused pch_i2c_resume(struct device *dev)
> > >  {
> > >  	int i;
> > > -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > > -
> > > -	pci_set_power_state(pdev, PCI_D0);
> > > -	pci_restore_state(pdev);
> > > +	struct adapter_info *adap_info = dev_get_drvdata(dev);
> > >  
> > > -	if (pci_enable_device(pdev) < 0) {
> > > -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> > > -		return -EIO;
> > > -	}
> > > -
> > > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > > +	device_wakeup_disable(dev);
> > 
> > It *looks* wrong to disable wakeup in both suspend and resume.  I
> > think the usual pattern is to enable wakeup in suspend and disable it
> > in resume.
> > 
> > But it looks like it's been that way since the driver was added by
> > e9bc8fa5df1c ("i2c-eg20t: add driver for Intel EG20T").
> > 
> > If the device doesn't support wakeup, I would not expect the driver to
> > mention wakeup at all.
> > 
> > In any case, I think it's the right thing for *this* patch to preserve
> > the previous wakeup behavior.  Maybe we want a follow-up patch to just
> > remove both device_wakeup_disable() calls?
> > 
We have seen this issue earlier in other drivers too. I remember you even
identified and listed them.
The PCI core calls, pci_enable_wake(pci_dev, PCI_D0, false). And if the driver
does not want to enable-wake on suspend, we discussed and concluded that the
calls should be dropped.
I am sending v2, to include dropping wakeup call.

Thanks
Vaibhav Gupta
> > >  	for (i = 0; i < adap_info->ch_num; i++)
> > >  		pch_i2c_init(&adap_info->pch_data[i]);
> > > @@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
> > >  
> > >  	return 0;
> > >  }
> > > -#else
> > > -#define pch_i2c_suspend NULL
> > > -#define pch_i2c_resume NULL
> > > -#endif
> > > +
> > > +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
> > >  
> > >  static struct pci_driver pch_pcidriver = {
> > >  	.name = KBUILD_MODNAME,
> > >  	.id_table = pch_pcidev_id,
> > >  	.probe = pch_i2c_probe,
> > >  	.remove = pch_i2c_remove,
> > > -	.suspend = pch_i2c_suspend,
> > > -	.resume = pch_i2c_resume
> > > +	.driver.pm = &pch_i2c_pm_ops,
> > >  };
> > >  
> > >  module_pci_driver(pch_pcidriver);
> > > -- 
> > > 2.27.0
> > > 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Bjorn Helgaas]

On Wed, Aug 05, 2020 at 09:51:54PM +0530, Vaibhav Gupta wrote:
> On Wed, Aug 05, 2020 at 10:28:32AM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 05, 2020 at 10:23:31AM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jul 20, 2020 at 07:30:32PM +0530, Vaibhav Gupta wrote:
> > > > Drivers using legacy PM have to manage PCI states and device's PM states
> > > > themselves. They also need to take care of configuration registers.
> > > > 
> > > > With improved and powerful support of generic PM, PCI Core takes care of
> > > > above mentioned, device-independent, jobs.
> > > > 
> > > > This driver makes use of PCI helper functions like
> > > > pci_save/restore_state(), pci_enable/disable_device(),
> > > > pci_enable_wake() and pci_set_power_state() to do required operations. In
> > > > generic mode, they are no longer needed.
> > > > 
> > > > Change function parameter in both .suspend() and .resume() to
> > > > "struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
> > > > "struct pci_dev*" variable and drv data.
> > > > 
> > > > Compile-tested only.
> > > > 
> > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > > ---
> > > >  drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
> > > >  1 file changed, 9 insertions(+), 30 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> > > > index 73f139690e4e..c0ddc4cc2ce7 100644
> > > > --- a/drivers/i2c/busses/i2c-eg20t.c
> > > > +++ b/drivers/i2c/busses/i2c-eg20t.c
> > > > @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
> > > >  	kfree(adap_info);
> > > >  }
> > > >  
> > > > -#ifdef CONFIG_PM
> > > > -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > > > +static int __maybe_unused pch_i2c_suspend(struct device *dev)
> > > >  {
> > > > -	int ret;
> > > >  	int i;
> > > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > >  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > > 
> > > Why don't you use "adap_info = dev_get_drvdata(dev)" as you did below,
> > > so you don't need to_pci_dev()?
> > >
> Actually, line 870, pch_pci_dbg() again needs "struct pci_dev*" type
> pointer.  Either way I had to use to_pci_dev(), so defined "pdev"
> which made less number of required changes in the code.

OK.  pch_pci_dbg() only needs the pdev in order to *get* the struct
device *, so it's all sort of in circles.  But it's fine to do as you
did here.

> > > >  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
> > > >  
> > > > @@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > > >  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
> > > >  		ioread32(p + PCH_I2CESRSTA));
> > > >  
> > > > -	ret = pci_save_state(pdev);
> > > > -
> > > > -	if (ret) {
> > > > -		pch_pci_err(pdev, "pci_save_state\n");
> > > > -		return ret;
> > > > -	}
> > > > -
> > > > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > > > -	pci_disable_device(pdev);
> > > > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > > > +	device_wakeup_disable(dev);
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int pch_i2c_resume(struct pci_dev *pdev)
> > > > +static int __maybe_unused pch_i2c_resume(struct device *dev)
> > > >  {
> > > >  	int i;
> > > > -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > > > -
> > > > -	pci_set_power_state(pdev, PCI_D0);
> > > > -	pci_restore_state(pdev);
> > > > +	struct adapter_info *adap_info = dev_get_drvdata(dev);
> > > >  
> > > > -	if (pci_enable_device(pdev) < 0) {
> > > > -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> > > > -		return -EIO;
> > > > -	}
> > > > -
> > > > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > > > +	device_wakeup_disable(dev);
> > > 
> > > It *looks* wrong to disable wakeup in both suspend and resume.  I
> > > think the usual pattern is to enable wakeup in suspend and disable it
> > > in resume.
> > > 
> > > But it looks like it's been that way since the driver was added by
> > > e9bc8fa5df1c ("i2c-eg20t: add driver for Intel EG20T").
> > > 
> > > If the device doesn't support wakeup, I would not expect the driver to
> > > mention wakeup at all.
> > > 
> > > In any case, I think it's the right thing for *this* patch to preserve
> > > the previous wakeup behavior.  Maybe we want a follow-up patch to just
> > > remove both device_wakeup_disable() calls?
> > > 
> We have seen this issue earlier in other drivers too. I remember you even
> identified and listed them.
> The PCI core calls, pci_enable_wake(pci_dev, PCI_D0, false). And if the driver
> does not want to enable-wake on suspend, we discussed and concluded that the
> calls should be dropped.
> I am sending v2, to include dropping wakeup call.

Personally, I think I would do this in two patches, and in the reverse
order than what I first suggested:

  1) Drop pci_enable_wake() calls
  2) Convert to generic PM

Doing them in that order means patch 2 will be slightly simpler, and
if there's any issue with removing the wakeup stuff, we can debug it
in the context of the original PCI PM code we've been using for years
without muddying the water with the additional generic PM changes.

> > > >  	for (i = 0; i < adap_info->ch_num; i++)
> > > >  		pch_i2c_init(&adap_info->pch_data[i]);
> > > > @@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
> > > >  
> > > >  	return 0;
> > > >  }
> > > > -#else
> > > > -#define pch_i2c_suspend NULL
> > > > -#define pch_i2c_resume NULL
> > > > -#endif
> > > > +
> > > > +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
> > > >  
> > > >  static struct pci_driver pch_pcidriver = {
> > > >  	.name = KBUILD_MODNAME,
> > > >  	.id_table = pch_pcidev_id,
> > > >  	.probe = pch_i2c_probe,
> > > >  	.remove = pch_i2c_remove,
> > > > -	.suspend = pch_i2c_suspend,
> > > > -	.resume = pch_i2c_resume
> > > > +	.driver.pm = &pch_i2c_pm_ops,
> > > >  };
> > > >  
> > > >  module_pci_driver(pch_pcidriver);
> > > > -- 
> > > > 2.27.0
> > > > 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v1] i2c: eg20t: use generic power management

[Vaibhav Gupta]

On Wed, Aug 05, 2020 at 11:56:11AM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 05, 2020 at 09:51:54PM +0530, Vaibhav Gupta wrote:
> > On Wed, Aug 05, 2020 at 10:28:32AM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 05, 2020 at 10:23:31AM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Jul 20, 2020 at 07:30:32PM +0530, Vaibhav Gupta wrote:
> > > > > Drivers using legacy PM have to manage PCI states and device's PM states
> > > > > themselves. They also need to take care of configuration registers.
> > > > > 
> > > > > With improved and powerful support of generic PM, PCI Core takes care of
> > > > > above mentioned, device-independent, jobs.
> > > > > 
> > > > > This driver makes use of PCI helper functions like
> > > > > pci_save/restore_state(), pci_enable/disable_device(),
> > > > > pci_enable_wake() and pci_set_power_state() to do required operations. In
> > > > > generic mode, they are no longer needed.
> > > > > 
> > > > > Change function parameter in both .suspend() and .resume() to
> > > > > "struct device*" type. Use to_pci_dev() and dev_get_drvdata() to get
> > > > > "struct pci_dev*" variable and drv data.
> > > > > 
> > > > > Compile-tested only.
> > > > > 
> > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > > > ---
> > > > >  drivers/i2c/busses/i2c-eg20t.c | 39 ++++++++--------------------------
> > > > >  1 file changed, 9 insertions(+), 30 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> > > > > index 73f139690e4e..c0ddc4cc2ce7 100644
> > > > > --- a/drivers/i2c/busses/i2c-eg20t.c
> > > > > +++ b/drivers/i2c/busses/i2c-eg20t.c
> > > > > @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
> > > > >  	kfree(adap_info);
> > > > >  }
> > > > >  
> > > > > -#ifdef CONFIG_PM
> > > > > -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > > > > +static int __maybe_unused pch_i2c_suspend(struct device *dev)
> > > > >  {
> > > > > -	int ret;
> > > > >  	int i;
> > > > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > > >  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > > > 
> > > > Why don't you use "adap_info = dev_get_drvdata(dev)" as you did below,
> > > > so you don't need to_pci_dev()?
> > > >
> > Actually, line 870, pch_pci_dbg() again needs "struct pci_dev*" type
> > pointer.  Either way I had to use to_pci_dev(), so defined "pdev"
> > which made less number of required changes in the code.
> 
> OK.  pch_pci_dbg() only needs the pdev in order to *get* the struct
> device *, so it's all sort of in circles.  But it's fine to do as you
> did here.
> 
> > > > >  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
> > > > >  
> > > > > @@ -872,34 +871,17 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > > > >  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
> > > > >  		ioread32(p + PCH_I2CESRSTA));
> > > > >  
> > > > > -	ret = pci_save_state(pdev);
> > > > > -
> > > > > -	if (ret) {
> > > > > -		pch_pci_err(pdev, "pci_save_state\n");
> > > > > -		return ret;
> > > > > -	}
> > > > > -
> > > > > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > > > > -	pci_disable_device(pdev);
> > > > > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > > > > +	device_wakeup_disable(dev);
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -static int pch_i2c_resume(struct pci_dev *pdev)
> > > > > +static int __maybe_unused pch_i2c_resume(struct device *dev)
> > > > >  {
> > > > >  	int i;
> > > > > -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > > > > -
> > > > > -	pci_set_power_state(pdev, PCI_D0);
> > > > > -	pci_restore_state(pdev);
> > > > > +	struct adapter_info *adap_info = dev_get_drvdata(dev);
> > > > >  
> > > > > -	if (pci_enable_device(pdev) < 0) {
> > > > > -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> > > > > -		return -EIO;
> > > > > -	}
> > > > > -
> > > > > -	pci_enable_wake(pdev, PCI_D3hot, 0);
> > > > > +	device_wakeup_disable(dev);
> > > > 
> > > > It *looks* wrong to disable wakeup in both suspend and resume.  I
> > > > think the usual pattern is to enable wakeup in suspend and disable it
> > > > in resume.
> > > > 
> > > > But it looks like it's been that way since the driver was added by
> > > > e9bc8fa5df1c ("i2c-eg20t: add driver for Intel EG20T").
> > > > 
> > > > If the device doesn't support wakeup, I would not expect the driver to
> > > > mention wakeup at all.
> > > > 
> > > > In any case, I think it's the right thing for *this* patch to preserve
> > > > the previous wakeup behavior.  Maybe we want a follow-up patch to just
> > > > remove both device_wakeup_disable() calls?
> > > > 
> > We have seen this issue earlier in other drivers too. I remember you even
> > identified and listed them.
> > The PCI core calls, pci_enable_wake(pci_dev, PCI_D0, false). And if the driver
> > does not want to enable-wake on suspend, we discussed and concluded that the
> > calls should be dropped.
> > I am sending v2, to include dropping wakeup call.
> 
> Personally, I think I would do this in two patches, and in the reverse
> order than what I first suggested:
> 
>   1) Drop pci_enable_wake() calls
>   2) Convert to generic PM
> 
> Doing them in that order means patch 2 will be slightly simpler, and
> if there's any issue with removing the wakeup stuff, we can debug it
> in the context of the original PCI PM code we've been using for years
> without muddying the water with the additional generic PM changes.
> 
Yeah, this seems more reasonable.

Thanks
Vaibhav Gupta
> > > > >  	for (i = 0; i < adap_info->ch_num; i++)
> > > > >  		pch_i2c_init(&adap_info->pch_data[i]);
> > > > > @@ -908,18 +890,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -#else
> > > > > -#define pch_i2c_suspend NULL
> > > > > -#define pch_i2c_resume NULL
> > > > > -#endif
> > > > > +
> > > > > +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
> > > > >  
> > > > >  static struct pci_driver pch_pcidriver = {
> > > > >  	.name = KBUILD_MODNAME,
> > > > >  	.id_table = pch_pcidev_id,
> > > > >  	.probe = pch_i2c_probe,
> > > > >  	.remove = pch_i2c_remove,
> > > > > -	.suspend = pch_i2c_suspend,
> > > > > -	.resume = pch_i2c_resume
> > > > > +	.driver.pm = &pch_i2c_pm_ops,
> > > > >  };
> > > > >  
> > > > >  module_pci_driver(pch_pcidriver);
> > > > > -- 
> > > > > 2.27.0
> > > > > 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH v2 0/2] i2c: eg20t: Power management upgrade and clean-ups

[Vaibhav Gupta]

Linux Kernel Mentee: Remove Legacy Power Management.

The purpose of this patch series is to upgrade power management in i2c-eg20t
driver. This has been done by upgrading .suspend() and .resume() callbacks.

The upgrade makes sure that the involvement of PCI Core does not change the
order of operations executed in a driver. Thus, does not change its behavior.

Also, before upgrading PM, some cleanup is required. Both .suspend() and
.resume() invoke pci_enable_wake() just to disable wakeup. This is not
required as if .suspend() does not want to enable-wake the device, PCI core
takes care of the required operations.

v2 : An additional patch had to be added in v1 to drop PCI wakeup calls.

All patches are compile-tested only.

Test tools:
    - Compiler: gcc (GCC) 10.1.0
    - allmodconfig build: make -j$(nproc) W=1 all

Vaibhav Gupta (2):
  i2c: eg20t: Drop PCI wakeup calls from .suspend/.resume
  i2c: eg20t: use generic power management

 drivers/i2c/busses/i2c-eg20t.c | 39 ++++++----------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH v2 1/2] i2c: eg20t: Drop PCI wakeup calls from .suspend/.resume

[Vaibhav Gupta]

The driver calls pci_enable_wake(...., false) in pch_i2c_suspend() as well
as pch_i2c_resume(). Either it should enable-wake the device in .suspend()
or should not invoke pci_enable_wake() at all.

Concluding that this driver doesn't support enable-wake and PCI core calls
pci_enable_wake(pci_dev, PCI_D0, false) during resume, drop it from
.suspend() and .resume().

Reported-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/i2c/busses/i2c-eg20t.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index 73f139690e4e..eb41de22d461 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -879,7 +879,6 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
 		return ret;
 	}
 
-	pci_enable_wake(pdev, PCI_D3hot, 0);
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 
@@ -899,8 +898,6 @@ static int pch_i2c_resume(struct pci_dev *pdev)
 		return -EIO;
 	}
 
-	pci_enable_wake(pdev, PCI_D3hot, 0);
-
 	for (i = 0; i < adap_info->ch_num; i++)
 		pch_i2c_init(&adap_info->pch_data[i]);
 
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH v2 2/2] i2c: eg20t: use generic power management

[Vaibhav Gupta]

Drivers using legacy power management .suspen()/.resume() callbacks
have to manage PCI states and device's PM states themselves. They also
need to take care of standard configuration registers.

Switch to generic power management framework using a single
"struct dev_pm_ops" variable to take the unnecessary load from the driver.
This also avoids the need for the driver to directly call most of the PCI
helper functions and device power state control functions, as through
the generic framework PCI Core takes care of the necessary operations,
and drivers are required to do only device-specific jobs.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/i2c/busses/i2c-eg20t.c | 36 +++++++---------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index eb41de22d461..843b31a0f752 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
 	kfree(adap_info);
 }
 
-#ifdef CONFIG_PM
-static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused pch_i2c_suspend(struct device *dev)
 {
-	int ret;
 	int i;
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct adapter_info *adap_info = pci_get_drvdata(pdev);
 	void __iomem *p = adap_info->pch_data[0].pch_base_address;
 
@@ -872,31 +871,13 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
 		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
 		ioread32(p + PCH_I2CESRSTA));
 
-	ret = pci_save_state(pdev);
-
-	if (ret) {
-		pch_pci_err(pdev, "pci_save_state\n");
-		return ret;
-	}
-
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
 	return 0;
 }
 
-static int pch_i2c_resume(struct pci_dev *pdev)
+static int __maybe_unused pch_i2c_resume(struct device *dev)
 {
 	int i;
-	struct adapter_info *adap_info = pci_get_drvdata(pdev);
-
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
-
-	if (pci_enable_device(pdev) < 0) {
-		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
-		return -EIO;
-	}
+	struct adapter_info *adap_info = dev_get_drvdata(dev);
 
 	for (i = 0; i < adap_info->ch_num; i++)
 		pch_i2c_init(&adap_info->pch_data[i]);
@@ -905,18 +886,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
 
 	return 0;
 }
-#else
-#define pch_i2c_suspend NULL
-#define pch_i2c_resume NULL
-#endif
+
+static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
 
 static struct pci_driver pch_pcidriver = {
 	.name = KBUILD_MODNAME,
 	.id_table = pch_pcidev_id,
 	.probe = pch_i2c_probe,
 	.remove = pch_i2c_remove,
-	.suspend = pch_i2c_suspend,
-	.resume = pch_i2c_resume
+	.driver.pm = &pch_i2c_pm_ops,
 };
 
 module_pci_driver(pch_pcidriver);
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2 2/2] i2c: eg20t: use generic power management

[Bjorn Helgaas]

[+cc Jean for i801 question below]

On Thu, Aug 06, 2020 at 01:06:16AM +0530, Vaibhav Gupta wrote:
> Drivers using legacy power management .suspen()/.resume() callbacks
> have to manage PCI states and device's PM states themselves. They also
> need to take care of standard configuration registers.
> 
> Switch to generic power management framework using a single
> "struct dev_pm_ops" variable to take the unnecessary load from the driver.
> This also avoids the need for the driver to directly call most of the PCI
> helper functions and device power state control functions, as through
> the generic framework PCI Core takes care of the necessary operations,
> and drivers are required to do only device-specific jobs.
> 
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>

s/.suspen/.suspend/ above

These both look right to me.

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

Looking at neighboring drivers, it looks like some already use generic
PM but have unnecessary PCI code, e.g., amd_mp2_pci_suspend().
Probably already on your list.

Also, i801_suspend() looks suspicious because it writes SMBHSTCFG, but
I don't see anything corresponding in i801_resume().

> ---
>  drivers/i2c/busses/i2c-eg20t.c | 36 +++++++---------------------------
>  1 file changed, 7 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> index eb41de22d461..843b31a0f752 100644
> --- a/drivers/i2c/busses/i2c-eg20t.c
> +++ b/drivers/i2c/busses/i2c-eg20t.c
> @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
>  	kfree(adap_info);
>  }
>  
> -#ifdef CONFIG_PM
> -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused pch_i2c_suspend(struct device *dev)
>  {
> -	int ret;
>  	int i;
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
>  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
>  
> @@ -872,31 +871,13 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
>  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
>  		ioread32(p + PCH_I2CESRSTA));
>  
> -	ret = pci_save_state(pdev);
> -
> -	if (ret) {
> -		pch_pci_err(pdev, "pci_save_state\n");
> -		return ret;
> -	}
> -
> -	pci_disable_device(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -
>  	return 0;
>  }
>  
> -static int pch_i2c_resume(struct pci_dev *pdev)
> +static int __maybe_unused pch_i2c_resume(struct device *dev)
>  {
>  	int i;
> -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> -
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_restore_state(pdev);
> -
> -	if (pci_enable_device(pdev) < 0) {
> -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> -		return -EIO;
> -	}
> +	struct adapter_info *adap_info = dev_get_drvdata(dev);
>  
>  	for (i = 0; i < adap_info->ch_num; i++)
>  		pch_i2c_init(&adap_info->pch_data[i]);
> @@ -905,18 +886,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
>  
>  	return 0;
>  }
> -#else
> -#define pch_i2c_suspend NULL
> -#define pch_i2c_resume NULL
> -#endif
> +
> +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
>  
>  static struct pci_driver pch_pcidriver = {
>  	.name = KBUILD_MODNAME,
>  	.id_table = pch_pcidev_id,
>  	.probe = pch_i2c_probe,
>  	.remove = pch_i2c_remove,
> -	.suspend = pch_i2c_suspend,
> -	.resume = pch_i2c_resume
> +	.driver.pm = &pch_i2c_pm_ops,
>  };
>  
>  module_pci_driver(pch_pcidriver);
> -- 
> 2.27.0
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2 2/2] i2c: eg20t: use generic power management

[Vaibhav Gupta]

On Fri, Aug 07, 2020 at 03:23:21PM -0500, Bjorn Helgaas wrote:
> [+cc Jean for i801 question below]
> 
> On Thu, Aug 06, 2020 at 01:06:16AM +0530, Vaibhav Gupta wrote:
> > Drivers using legacy power management .suspen()/.resume() callbacks
> > have to manage PCI states and device's PM states themselves. They also
> > need to take care of standard configuration registers.
> > 
> > Switch to generic power management framework using a single
> > "struct dev_pm_ops" variable to take the unnecessary load from the driver.
> > This also avoids the need for the driver to directly call most of the PCI
> > helper functions and device power state control functions, as through
> > the generic framework PCI Core takes care of the necessary operations,
> > and drivers are required to do only device-specific jobs.
> > 
> > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> 
> s/.suspen/.suspend/ above
> 
> These both look right to me.
> 
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Looking at neighboring drivers, it looks like some already use generic
> PM but have unnecessary PCI code, e.g., amd_mp2_pci_suspend().
> Probably already on your list.
Yes :)
> 
> Also, i801_suspend() looks suspicious because it writes SMBHSTCFG, but
> I don't see anything corresponding in i801_resume().
I will look into it.

Thanks
Vaibhav Gupta
> 
> > ---
> >  drivers/i2c/busses/i2c-eg20t.c | 36 +++++++---------------------------
> >  1 file changed, 7 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> > index eb41de22d461..843b31a0f752 100644
> > --- a/drivers/i2c/busses/i2c-eg20t.c
> > +++ b/drivers/i2c/busses/i2c-eg20t.c
> > @@ -846,11 +846,10 @@ static void pch_i2c_remove(struct pci_dev *pdev)
> >  	kfree(adap_info);
> >  }
> >  
> > -#ifdef CONFIG_PM
> > -static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __maybe_unused pch_i2c_suspend(struct device *dev)
> >  {
> > -	int ret;
> >  	int i;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> >  	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> >  	void __iomem *p = adap_info->pch_data[0].pch_base_address;
> >  
> > @@ -872,31 +871,13 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
> >  		ioread32(p + PCH_I2CSR), ioread32(p + PCH_I2CBUFSTA),
> >  		ioread32(p + PCH_I2CESRSTA));
> >  
> > -	ret = pci_save_state(pdev);
> > -
> > -	if (ret) {
> > -		pch_pci_err(pdev, "pci_save_state\n");
> > -		return ret;
> > -	}
> > -
> > -	pci_disable_device(pdev);
> > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > -
> >  	return 0;
> >  }
> >  
> > -static int pch_i2c_resume(struct pci_dev *pdev)
> > +static int __maybe_unused pch_i2c_resume(struct device *dev)
> >  {
> >  	int i;
> > -	struct adapter_info *adap_info = pci_get_drvdata(pdev);
> > -
> > -	pci_set_power_state(pdev, PCI_D0);
> > -	pci_restore_state(pdev);
> > -
> > -	if (pci_enable_device(pdev) < 0) {
> > -		pch_pci_err(pdev, "pch_i2c_resume:pci_enable_device FAILED\n");
> > -		return -EIO;
> > -	}
> > +	struct adapter_info *adap_info = dev_get_drvdata(dev);
> >  
> >  	for (i = 0; i < adap_info->ch_num; i++)
> >  		pch_i2c_init(&adap_info->pch_data[i]);
> > @@ -905,18 +886,15 @@ static int pch_i2c_resume(struct pci_dev *pdev)
> >  
> >  	return 0;
> >  }
> > -#else
> > -#define pch_i2c_suspend NULL
> > -#define pch_i2c_resume NULL
> > -#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(pch_i2c_pm_ops, pch_i2c_suspend, pch_i2c_resume);
> >  
> >  static struct pci_driver pch_pcidriver = {
> >  	.name = KBUILD_MODNAME,
> >  	.id_table = pch_pcidev_id,
> >  	.probe = pch_i2c_probe,
> >  	.remove = pch_i2c_remove,
> > -	.suspend = pch_i2c_suspend,
> > -	.resume = pch_i2c_resume
> > +	.driver.pm = &pch_i2c_pm_ops,
> >  };
> >  
> >  module_pci_driver(pch_pcidriver);
> > -- 
> > 2.27.0
> > 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2 1/2] i2c: eg20t: Drop PCI wakeup calls from .suspend/.resume

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 600 bytes --]

On Thu, Aug 06, 2020 at 01:06:15AM +0530, Vaibhav Gupta wrote:
> The driver calls pci_enable_wake(...., false) in pch_i2c_suspend() as well
> as pch_i2c_resume(). Either it should enable-wake the device in .suspend()
> or should not invoke pci_enable_wake() at all.
> 
> Concluding that this driver doesn't support enable-wake and PCI core calls
> pci_enable_wake(pci_dev, PCI_D0, false) during resume, drop it from
> .suspend() and .resume().
> 
> Reported-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Applied to for-next, thanks!


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2 2/2] i2c: eg20t: use generic power management

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 794 bytes --]

On Thu, Aug 06, 2020 at 01:06:16AM +0530, Vaibhav Gupta wrote:
> Drivers using legacy power management .suspen()/.resume() callbacks
> have to manage PCI states and device's PM states themselves. They also
> need to take care of standard configuration registers.
> 
> Switch to generic power management framework using a single
> "struct dev_pm_ops" variable to take the unnecessary load from the driver.
> This also avoids the need for the driver to directly call most of the PCI
> helper functions and device power state control functions, as through
> the generic framework PCI Core takes care of the necessary operations,
> and drivers are required to do only device-specific jobs.
> 
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Applied to for-next, thanks!


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2 2/2] i2c: eg20t: use generic power management

[Jean Delvare]

Hi Bjorn, Vaibhav,

On Fri, 07 Aug 2020 15:23:21 -0500, Bjorn Helgaas wrote:
> Also, i801_suspend() looks suspicious because it writes SMBHSTCFG, but
> I don't see anything corresponding in i801_resume().

You're right, it's buggy. Volker Rümelin's patch at:

https://patchwork.ozlabs.org/project/linux-i2c/patch/a2fc5a6d-a3bf-eaf0-bb75-1521be346333@googlemail.com/

should fix it. I was supposed to review it but did not, shame on me.
I'll do it today.

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH v2 2/2] i2c: eg20t: use generic power management

[Bjorn Helgaas]

On Tue, Aug 25, 2020 at 11:53:42AM +0200, Jean Delvare wrote:
> Hi Bjorn, Vaibhav,
> 
> On Fri, 07 Aug 2020 15:23:21 -0500, Bjorn Helgaas wrote:
> > Also, i801_suspend() looks suspicious because it writes SMBHSTCFG, but
> > I don't see anything corresponding in i801_resume().
> 
> You're right, it's buggy. Volker Rümelin's patch at:
> 
> https://patchwork.ozlabs.org/project/linux-i2c/patch/a2fc5a6d-a3bf-eaf0-bb75-1521be346333@googlemail.com/
> 
> should fix it. I was supposed to review it but did not, shame on me.
> I'll do it today.

Always nice when the fix is already there :)  Thanks for following up
on this!

Bjorn
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees