Re: [RFC][PATCH] Add suspend and resume support to uli526x

2007-06-04 Thread Nigel Cunningham
Hi.

On Mon, 2007-06-04 at 15:49 +0200, Rafael J. Wysocki wrote:
 On Monday, 4 June 2007 13:11, Pavel Machek wrote:
   From: Rafael J. Wysocki [EMAIL PROTECTED]
   
   Add suspend/resume support to the uli526x network driver (tested on 
   x86_64,
   with 'Ethernet controller: ALi Corporation M5263 Ethernet Controller, rev 
   40').
   
   Signed-off-by: Rafael J. Wysocki [EMAIL PROTECTED]
  
  Looks ok to me.
  
   +#ifdef CONFIG_PM
   +
   +/*
   + *   Suspend the interface.
   + */
   +
   +static int uli526x_suspend(struct pci_dev *pdev, pm_message_t state)
   +{
   + struct net_device *dev = pci_get_drvdata(pdev);
   +
   + ULI526X_DBUG(0, uli526x_suspend, 0);
   +
   + if (dev  netdev_priv(dev)) {
   + if (netif_running(dev)) {
   + netif_device_detach(dev);
   + uli526x_down(dev);
   + }
   + pci_save_state(pdev);
   + pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
   + pci_disable_device(pdev);
   + pci_set_power_state(pdev, pci_choose_state(pdev, state));
   + }
   + return 0;
   +}
   +
   +/*
   + *   Resume the interface.
   + */
   +
   +static int uli526x_resume(struct pci_dev *pdev)
   +{
   + struct net_device *dev = pci_get_drvdata(pdev);
   + struct uli526x_board_info *db = netdev_priv(dev);
   + int err;
   +
   + ULI526X_DBUG(0, uli526x_resume, 0);
   +
   + if (dev  db) {
   + pci_set_power_state(pdev, PCI_D0);
   + err = pci_enable_device(pdev);
   + if (err) {
   + printk(KERN_WARNING %s: Could not enable device \n,
   + dev-name);
   + return err;
   + }
   + pci_restore_state(pdev);
   + pci_set_master(pdev);
   + if (netif_running(dev)) {
   + uli526x_up(dev);
   + netif_device_attach(dev);
   + }
   + }
   + return 0;
   +}
   +
  #else 
  #define *_resume NULL
  #define *_suspend NULL
 
   +#endif /* CONFIG_PM */
  
   @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
 .id_table   = uli526x_pci_tbl,
 .probe  = uli526x_init_one,
 .remove = __devexit_p(uli526x_remove_one),
   +#ifdef CONFIG_PM
   + .suspend= uli526x_suspend,
   + .resume = uli526x_resume,
   +#endif
  
  ...so that this ifdef is not needed?
 
 OK, why not.

Because it's uglier and #ifdef is the established way of doing things?

Regards,

Nigel


signature.asc
Description: This is a digitally signed message part


Re: [RFC][PATCH] Add suspend and resume support to uli526x

2007-06-04 Thread Nigel Cunningham
Hi.

On Tue, 2007-06-05 at 00:41 +0200, Pavel Machek wrote:
 Hi!
 
 +#endif /* CONFIG_PM */

 @@ -1689,6 +1775,10 @@ static struct pci_driver uli526x_driver 
   .id_table   = uli526x_pci_tbl,
   .probe  = uli526x_init_one,
   .remove = __devexit_p(uli526x_remove_one),
 +#ifdef CONFIG_PM
 + .suspend= uli526x_suspend,
 + .resume = uli526x_resume,
 +#endif

...so that this ifdef is not needed?
   
   OK, why not.
  
  Because it's uglier and #ifdef is the established way of doing things?
 
 Actually the way I suggested is nicer, IIRC akpm invented it. It keeps
 ifdefs localized around the block that _needs_ to be ifdefed.

The localised point is true. I'll also admit that 'nicer'/'uglier' is a
matter of aesthetics and therefore personal opinion.

I guess that leaves the question, What's the precedent to follow? or
Is there a driver that's already got this new format?.

Regards,

Nigel


signature.asc
Description: This is a digitally signed message part