Re: [PATCH 2/2] ide: add proper PCI PM support

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Saturday, 2 of August 2008, Bartlomiej Zolnierkiewicz wrote:
> * Keep pointer to ->init_chipset method also in
>   struct ide_host and set it in ide_host_alloc_all().
> 
> * Add ide_pci_suspend() and ide_pci_resume() helpers
>   (default ->suspend and ->resume implementations).
> 
> * ->init_chipset can no longer be marked __devinit.
> 
> * Add proper PCI PM support to IDE PCI host drivers
>   (rz1000.c and tc86c001.c are skipped for now since
>   they need to be converted from using ->init_hwif
>   to use ->init_chipset instead).

Well, first, the work on improving the PCI PM support by IDE drivers is much
appreciated.

Still, since you're adding new routines etc., it may be a good time to
implement them using the new PM framework, as defined in
include/linux/pm.h (please also have a look at drivers/pci/pci-driver.c for the
PCI bus type's PM callbacks implementation details).  Unfortunately, it's not
been well documented yet, so if you have any questions, please ask.

In fact, we need some example implementations of the new PM callbacks and
since you know the drivers in question very well, we could use your
implementations as examples, if you don't mind.  [Certainly, they would be
better than PM callbacks written by me for any driver. ;-) ]

> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---

[--snip--]  

>  static int __init via_ide_init(void)
> Index: b/drivers/ide/setup-pci.c
> ===================================================================
> --- a/drivers/ide/setup-pci.c
> +++ b/drivers/ide/setup-pci.c
> @@ -659,3 +659,36 @@ void ide_pci_remove(struct pci_dev *dev)
>  	pci_disable_device(dev);
>  }
>  EXPORT_SYMBOL_GPL(ide_pci_remove);
> +
> +#ifdef CONFIG_PM
> +int ide_pci_suspend(struct pci_dev *dev, pm_message_t state)
> +{
> +	pci_save_state(dev);
> +	pci_disable_device(dev);
> +	pci_set_power_state(dev, pci_choose_state(dev, state));

pci_choose_state() is now considered as obsolete.  You can use
pci_target_state() or even pci_prepare_to_sleep() here.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ide_pci_suspend);
> +
> +int ide_pci_resume(struct pci_dev *dev)
> +{
> +	struct ide_host *host = pci_get_drvdata(dev);
> +	int rc;
> +
> +	pci_set_power_state(dev, PCI_D0);
> +

If you decide to use pci_prepare_to_sleep() above, please use
pci_back_from_sleep() here.

> +	rc = pci_enable_device(dev);
> +	if (rc)
> +		return rc;
> +
> +	pci_restore_state(dev);
> +	pci_set_master(dev);
> +
> +	if (host->init_chipset)
> +		host->init_chipset(dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ide_pci_resume);
> +#endif

Thanks,
Rafael