Re: [PATCH] Cleanup pc87413 watchdog driver to use request_muxed_region for SuperIO area

[Jonathan McDowell <noodles@earth.li>]

On Fri, May 20, 2011 at 02:21:29PM +0200, Wim Van Sebroeck wrote:
> Hi Jonathan,
> 
> > > @@ -528,7 +511,8 @@ static int __init pc87413_init(void)
> > >  	printk(KERN_INFO PFX "Version " VERSION " at io 0x%X\n",
> > >  							WDT_INDEX_IO_PORT);
> > >  
> > > -	/* request_region(io, 2, "pc87413"); */
> > > +	if (!request_muxed_region(io, 2, MODNAME))
> > > +		return -EBUSY;
> > >  
> > >  	ret = register_reboot_notifier(&pc87413_notifier);
> > >  	if (ret != 0) {
> > > @@ -541,12 +525,32 @@ static int __init pc87413_init(void)
> > >  		printk(KERN_ERR PFX
> > >  			"cannot register miscdev on minor=%d (err=%d)\n",
> > >  			WATCHDOG_MINOR, ret);
> > > -		unregister_reboot_notifier(&pc87413_notifier);
> > > -		return ret;
> > > +		goto reboot_unreg;
> > >  	}
> > >  	printk(KERN_INFO PFX "initialized. timeout=%d min \n", timeout);
> > > +
> > > +	pc87413_select_wdt_out();
> > > +	pc87413_enable_swc();
> > > +	pc87413_get_swc_base_addr();
> > > +
> > > +	if (!request_region(swc_base_addr, 0x20, MODNAME)) {
> > > +		printk(KERN_ERR PFX
> > > +			"cannot request SWC region at 0x%x\n", swc_base_addr);
> > > +		ret = -EBUSY;
> > > +		goto misc_unreg;
> > > +	}
> > > +
> > >  	pc87413_enable();
> > > +
> > > +	release_region(io, 2);
> > >  	return 0;
> > > +
> > > +misc_unreg:
> > > +	misc_deregister(&pc87413_miscdev);
> > > +reboot_unreg:
> > > +	unregister_reboot_notifier(&pc87413_notifier);
> > > +	release_region(io, 2);
> > > +	return ret;
> > >  }
> > >  
> > >  /**
> > > @@ -569,7 +573,7 @@ static void __exit pc87413_exit(void)
> > >  
> > >  	misc_deregister(&pc87413_miscdev);
> > >  	unregister_reboot_notifier(&pc87413_notifier);
> > > -	/* release_region(io, 2); */
> > > +	release_region(swc_base_addr, 0x20);
> > >  
> > >  	printk(KERN_INFO MODNAME " watchdog component driver removed.\n");
> > >  }

> I don't think this is the correct way to do this. You do the
> request_muxed_region only at init and exit of the module.

I do a single request_muxed_region in the init function, and release it
at the end of the init function. It's only necessary to hold the LPC
region while the WDT functionality is enabled and the base address for
the SWC block is retrieved.

After that point all access is via the SWC region, so there's no need to
call request_muxed_region. The call in the exit function releases the
SWC block. We're not holding the LPC region for the entire duration of
the driver being loaded.

J.

-- 
  Hail Eris. All hail Discordia.   |  .''`.  Debian GNU/Linux Developer
              Fnord?               | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.