Re: [PATCH v2 2/2] i2c: eg20t: use generic power management

[Vaibhav Gupta <vaibhavgupta40@gmail.com>]

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
> >