lkml.org 
[lkml]   [2009]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: add bcm47xx watchdog driver
    On Thu, 04 Jun 2009 22:24:56 +0200
    matthieu castet <castet.matthieu@free.fr> wrote:

    >
    >
    > This add watchdog driver for broadcom 47xx device.
    > It uses the ssb subsytem to access embeded watchdog device.
    >
    > Because the watchdog timeout is very short (about 2s), a soft timer is used
    > to increase the watchdog period.
    >
    > Note : A patch for exporting the ssb_watchdog_timer_set will
    > be submitted on next linux-mips merge. Without this patch it can't
    > be build as a module.
    >
    >
    > ...
    >
    > --- linux-2.6.orig/drivers/watchdog/Kconfig 2009-05-25 22:22:02.000000000 +0200
    > +++ linux-2.6/drivers/watchdog/Kconfig 2009-05-25 22:26:06.000000000 +0200
    > @@ -764,6 +764,12 @@
    > help
    > Hardware driver for the built-in watchdog timer on TXx9 MIPS SoCs.
    >
    > +config BCM47XX_WDT
    > + tristate "Broadcom BCM47xx Watchdog Timer"
    > + depends on BCM47XX
    > + help
    > + Hardware driver for the Broadcom BCM47xx Watchog Timer.
    > +

    Please indent the kconfig body with a single tab character.

    >
    > ...
    >
    > +#define DRV_NAME "bcm47xx_wdt"
    > +
    > +#define WDT_DEFAULT_TIME 30 /* seconds */
    > +#define WDT_MAX_TIME 256 /* seconds */
    > +
    > +static int wdt_time = WDT_DEFAULT_TIME;
    > +static int nowayout = WATCHDOG_NOWAYOUT;
    > +
    > +module_param(wdt_time, int, 0);
    > +MODULE_PARM_DESC(wdt_time, "Watchdog time in seconds. (default="
    > + __MODULE_STRING(WDT_DEFAULT_TIME) ")");
    > +
    > +#ifdef CONFIG_WATCHDOG_NOWAYOUT
    > +module_param(nowayout, int, 0);
    > +MODULE_PARM_DESC(nowayout,
    > + "Watchdog cannot be stopped once started (default="
    > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
    > +#endif

    hm, now what's happening with the third arg to module_param()?

    > +static struct platform_device *bcm47xx_wdt_platform_device;
    > +
    > +static unsigned long bcm47xx_wdt_busy;
    > +static char expect_release;
    > +static struct timer_list wdt_timer;
    > +static atomic_t ticks;
    > +
    > +static inline void bcm47xx_wdt_hw_start(void)
    > +{
    > + /* this is 2,5s on 100Mhz clock and 2s on 133 Mhz */
    > + ssb_watchdog_timer_set(&ssb_bcm47xx, 0xfffffff);
    > +}
    > +
    > +static inline int bcm47xx_wdt_hw_stop(void)
    > +{
    > + return ssb_watchdog_timer_set(&ssb_bcm47xx, 0);
    > +}
    > +
    > +static void bcm47xx_timer_tick(unsigned long unused)
    > +{
    > + if(!atomic_dec_and_test(&ticks)) {

    Please pass this patch (and all others) through scripts/checkpatch.pl
    and review the resulting output.

    > + bcm47xx_wdt_hw_start();
    > + mod_timer(&wdt_timer, jiffies + HZ);
    > + }
    > + else {
    > + printk(KERN_CRIT PFX "Watchdog will fire soon!!!.\n");
    > + }
    > +}
    > +
    > +static inline void bcm47xx_wdt_pet(void)
    > +{
    > + atomic_set(&ticks, wdt_time);
    > +}

    What does "pet" stand for?

    > +static void bcm47xx_wdt_start(void)
    > +{
    > + bcm47xx_wdt_pet();
    > + bcm47xx_timer_tick(0);
    > +}
    > +
    > +static void bcm47xx_wdt_pause(void)
    > +{
    > + del_timer(&wdt_timer);

    Should this be del_timer_sync()? The timer callback can still be
    executing after del_timer() returns.

    > + bcm47xx_wdt_hw_stop();
    > +}
    > +
    > +static void bcm47xx_wdt_stop(void)
    > +{
    > + bcm47xx_wdt_pause();
    > +}
    > +
    > +static int bcm47xx_wdt_settimeout(int new_time)
    > +{
    > + if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
    > + return -EINVAL;
    > +
    > + wdt_time = new_time;
    > + return 0;
    > +}
    > +
    > +static int bcm47xx_wdt_open(struct inode *inode, struct file *file)
    > +{
    > + if (test_and_set_bit(0, &bcm47xx_wdt_busy))
    > + return -EBUSY;
    > +
    > + bcm47xx_wdt_start();
    > + return nonseekable_open(inode, file);
    > +}
    > +
    > +static int bcm47xx_wdt_release(struct inode *inode, struct file *file)
    > +{
    > + if (expect_release == 42) {
    > + bcm47xx_wdt_stop();
    > + } else {
    > + printk(KERN_CRIT DRV_NAME ": Unexpected close, not stopping watchdog!\n");

    Can this happen?

    > + bcm47xx_wdt_start();
    > + }
    > +
    > + clear_bit(0, &bcm47xx_wdt_busy);
    > + expect_release = 0;
    > + return 0;
    > +}
    > +
    >
    > ...
    >



    \
     
     \ /
      Last update: 2009-06-05 21:51    [W:0.034 / U:62.496 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site