lkml.org 
[lkml]   [2010]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] watchdog: add f71862fg support
On Mon, Sep 06, 2010 at 03:24:34PM +0200, Lutz Ballaschke wrote:
> Fintek Super-I/O f71862fg watchdog support added to f71808e_wdt driver.
> Furthermore the WDIOC_GETTIMELEFT ioctl is added to the driver.

You're patch makes three changes:
* Add f71862 support
* Add the WDIOC_GETTIMELEFT ioctl
* Change some config-register (magic) numbers into constants

Please split these things into separate patches.

> diff -Nurp linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c
> --- linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c 2010-08-16 02:41:37.000000000 +0200
> +++ linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c 2010-09-04 18:52:14.000000000 +0200
> @@ -252,6 +255,24 @@ exit_unlock:
> return err;
> }
>
> +static int watchdog_time_left(void)
> +{
> + int ret = 0;
> +
> + mutex_lock(&watchdog.lock);
> + ret = superio_enter(watchdog.sioaddr);
> + if (ret)
> + goto exit_unlock;
> +
> + superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
> + ret = (int) superio_inb(watchdog.sioaddr, F71808FG_REG_WD_TIME);
> + superio_exit(watchdog.sioaddr);
> +
> +exit_unlock:
> + mutex_unlock(&watchdog.lock);
> + return ret;
> +}
> +
> static int watchdog_keepalive(void)
> {
> int err = 0;

This function doesn't perform any unit-conversion. The ioctl expects to
receive seconds as the result, while this function can either return
minutes or seconds depending on the value of WD_UNIT (see datasheet
and/or uses of minutes_mode in the driver).

Further, why are you casting superio_inb()'s return value to (int) while
that's already the return type of that function?

> @@ -308,4 +329,10 @@ static int watchdog_start(void)
> superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
> break;
>
> + case f71862fg:
> + /* Set pin 63 to WDTRST# (SPI must be disabled first!) */
> + superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> + superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
> + break;
> +
> default:

Given that the F71862 has *two* pins assignable to the WDTRST# signal,
please document that fact, and the reason for choosing this one in the
code.

Perhaps it might even be desirable to insert a printk there. This to
make the user aware of the fact that this driver might not work if the
pin configuration of their hardware doesn't match the assumption you
make about it in this code.

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2010-09-07 15:21    [W:0.036 / U:6.772 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site