On Sat, May 07, 2011 at 01:20:47AM -0400, Nat Gurumoorthy wrote:> 01 - Changes to it87 watchdog driver to use "request_muxed_region"> Serialize access to the hardware by using "request_muxed_region" macro defined> by Alan Cox. Call to this macro will hold off the requestor if the resource is> currently busy.> > The use of the above macro makes it possible to get rid of> spinlocks in it8712f_wdt.c and it87_wdt.c watchdog drivers.> This also greatly simplifies the implementation of it87_wdt.c driver.> > "superio_enter" will return an error if call to "request_muxed_region" fails.> Rest of the code change is to ripple an error return from superio_enter to> the top level.> > Signed-off-by: Nat Gurumoorthy <natg@google.com>

Hi Nat,

it87_wdt_init() and wdt_ioctl() already have a variable named "rc" defined."ret" in addition to "rc" is confusing (granted, the use of "rc" to indicate that requesting region 1 failed in it87_wdt_init() is confusing as well).

In it8712f_wdt_find(), both err and ret are now defined as return variables.That should not be necessary.

In wdt_ioctl(), you still have one instance where you return -EBUSY andnot the returned error. I suspect it is because you did not want to define ret at the function level ... but as mentioned above, rc is already defined, so that isnot really an argument; just use rc.

In it87_wdt_init(), the error return path is problematic. There are a couple of added return statements without performing the expected error return action, which may leavesome superio bits as well as memory regions dangling. See below for details.