On Mon, May 04, 2015 at 05:19:26PM +0200, Arnd Bergmann wrote:
> On Monday 04 May 2015 20:04:47 fu.wei@linaro.org wrote:
> >  drivers/watchdog/Kconfig     |  10 +
> >  drivers/watchdog/Makefile    |   1 +
> >  drivers/watchdog/sbsa_gwdt.c | 578 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/watchdog/sbsa_gwdt.h |  99 ++++++++
> >
>
> To clarify my earlier comment: I actually meant to suggest moving the contents
> of sbsa_gwdt.h into sbsa_gwdt.c itself, and having only one file.
>
> > +static long sbsa_gwdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
> > +                               unsigned long arg)
> > +{
> > +       struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> > +       void __user *argp = (void __user *)arg;
> > +       int ret = -ENOIOCTLCMD;
> > +       unsigned int __user *p = argp;
> > +       unsigned int new_value;
> > +
> > +       /* FIXME: what else do we need here?? */
> > +       switch (cmd) {
> > +       case WDIOC_SETPRETIMEOUT:
> > +               if (get_user(new_value, p))
> > +                       return -EFAULT;
> > +               ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
> > +               break;
> > +       case WDIOC_GETPRETIMEOUT:
> > +               ret = put_user(gwdt->pretimeout, (unsigned int __user
> > *)arg);
> > +               break;
> > +       }
> > +
> > +       return ret;
> > +}
>
> Here, I'd like to see the two ioctl commands get handled by the ioctl
> handler in drivers/watchdog/watchdog_dev.c in the same way that
> WDIOC_GETTIMEOUT/WDIOC_SETTIMEOUT are. Just add one more patch to the
> series to introduce a generic watchdog_set_pretimeout() function
> and a 'pretimeout' field in 'struct watchdog_device'.
>

Makes sense.

Guenter