Re: [PATCH 1/3] watchdog_dev: Add support for having more then 1 watchdog

[Alan Cox <alan@xxxxxxxxxxxxxxxxxxx>]

> +static unsigned long dog_mask; /* Watcodgs that are in use */
> 
> Please fix (spelling) the comment after dog_mask.

Oops

> +               err = misc_register(&watchdog_miscdev);
> +               if (err == 0)
> +                       old_wdd = watchdog;
> +               else {
> +                       pr_err("%s: cannot register miscdev on minor=%d (err=%d
> +                               watchdog->info->identity, WATCHDOG_MINOR, err);
> +                       pr_err("%s: probably a legacy watchdog module is presen
> +                               watchdog->info->identity);
> +                       watchdog_release_id(watchdog);
> +                       err = watchdog_assign_id(watchdog, 1);
> +                       if (err < 0)
> +                               return err;
> +               }
> +       }
> 
> You may want to check for err == -EBUSY in the fallback instead of assuming
> the problem is a legacy watchdog driver

Yes that is probably wise

> +       err = watchdog_add_device(watchdog);
> +       }
> 
> You do this even for watchdog0 / for the watchdog we've also registered as
> misc_dev. I guess this is intentional and you want the watchdog to be
> available as both misc,130 as well as as MAJOR(dog_devt),0 ?

Yes - so that it appears in the watchdog class in sysfs for example.

> Other then these few remarks it looks fine. If you can resend it
> with my remarks addressed I'll add a Reviewed-By.

Ok I'll sort those next week.

Thanks

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html