Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

2018-02-14 Thread Jerry Hoemann
On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> Hi Jerry,
> 
> On Sun, Feb 11, 2018 at 10:21:06PM -0700, Jerry Hoemann wrote:
> > Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> > convert hpwdt from legacy watchdog driver to use the watchdog core.
> > 
> > Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> > Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> > Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> > Added functions: hpwdt_settimeout
> > Added structures: watchdog_device
> > 
> > Signed-off-by: Jerry Hoemann 
> 
> I think it would be better to use
> dev_emerg()
> dev_crit()
> dev_alert()
> dev_err()
> dev_warn()
> dev_notice()
> 
> instead of pr_* functions now when we have a device to use.



> >  }
> > @@ -128,8 +126,10 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> > struct pt_regs *regs)
> > if ((ulReason == NMI_UNKNOWN) && !mynmi)
> > return NMI_DONE;
> >  
> > +   pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> 
> dev_dbg()
> 

Sorry, wasn't clear on my earlier response.  As hpwdt_pretiemout isn't
being passed a device, this instance will still be a pr_debug.  I have
converted the others to dev_.* in the next version of the series.




-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-


Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

2018-02-13 Thread Jerry Hoemann
On Tue, Feb 13, 2018 at 01:55:49PM -0800, Guenter Roeck wrote:
> On Tue, Feb 13, 2018 at 02:36:48PM -0700, Jerry Hoemann wrote:
> > 
> > 
> > > 
> > > dev_dbg()
> > 
> > I think you are proposing I do:
> > 
> > dev_dbg(dev->parent, "settimeout = %d\n", val);
> > 
> > This raises the issue that I didn't set parent and I believe I should have.
> > 
> Correct.

Will fix.


> > > > retval = hpwdt_init_nmi_decoding(dev);
> > > > if (retval != 0)
> > > > goto error_init_nmi_decoding;
> > > >  
> > > > -   retval = misc_register(_miscdev);
> > > > +   retval = watchdog_register_device(_dev);
> > > > if (retval < 0) {
> > > > -   dev_warn(>dev,
> > > > -   "Unable to register miscdev on minor=%d 
> > > > (err=%d).\n",
> > > > -   WATCHDOG_MINOR, retval);
> > > > -   goto error_misc_register;
> > > > +   dev_warn(>dev, "Unable to register hpe watchdog 
> > > > (err=%d).\n", retval);
> > > > +   goto error_wd_register;
> > > > +   }
> > > > +
> > > > +   watchdog_set_nowayout(_dev, nowayout);
> > > > +   if (watchdog_init_timeout(_dev, soft_margin, NULL)) {
> > > > +   dev_warn(>dev, "Invalid soft_margin: %d. Using 
> > > > default\n", soft_margin);
> > > > +   soft_margin = DEFAULT_MARGIN;
> > > > }
> > > 
> > > In this case watchdog_init_timout() will:
> > > 1) Check if soft_margin is valid
> > > 2) if not, keep hpwdt_dev.timout intact (which is already set in
> > > declaration of hpwdt_dev)
> > > 
> > > So we could remove the condition and only keep
> > > watchdog_init_timeout(_dev, soft_margin, NULL);
> > > 
> > > 
> > > Also, we need to set an invalid initial value for soft_margin to make
> > > the logic for watchdog_init_timeout work. 
> > > 
> > > i.e
> > > - static unsigned int soft_margin = DEFAULT_MARGIN;   /* in seconds */
> > > + static unsigned int soft_margin;
> > 
> > 
> > I don't see the core printing a warning message if an invalid value
> > is specified for soft_margin when loading the module.
> > 
> > I don't mind the hpwdt module correcting an out of range parameter, but I
> > don't think it should do so siliently.
> > 
> 
> The point here is that setting soft_margin to 0 and setting the
> timeout in the watchdog_device structure to DEFAULT_MARGIN
> means that it is not necessary to override it on error.

Yes, I don't need to set soft_margin = DEFAULT_MARGIN inside the
conditional on watchdog_init_timeout.  I was doing that to keep
the value of soft_margin consistent with the value of hpwdt_dev.timeout
so that it would show the current timeout value in sysfs.  But, as
you pointed out, the watchdog sysfs displays timeout.  So, i don't
need that asignment anymore.

But, I want to keep the initialization:
static unsigned int soft_margin = DEFAULT_MARGIN;


Reasoning: I want soft_margin to be an optional parameter that
defaults to DEFAULT_MARGIN without warning.  If soft_margin is initialized
to a value outside of the valid range, say N, the code can't tell the
difference between the static value of N or if the module was loaded with the
soft_margin=N.  I only want to warn on the latter.


> If you want to be verbose, you can still log a warning message
> if the timeout provided by the module parameter is wrong.
> 
> Anyway, this driver will presumably never support devicetree,
> so the watchdog core will never read the timeout from there,
> and this is not a showstopper.
> 



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-


Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

2018-02-13 Thread Guenter Roeck
On Tue, Feb 13, 2018 at 02:36:48PM -0700, Jerry Hoemann wrote:
> 
> 
> Thanks for the review. Comments inline.
> 
> On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> > Hi Jerry,
> > 
> > On Sun, Feb 11, 2018 at 10:21:06PM -0700, Jerry Hoemann wrote:
> > > Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> > > convert hpwdt from legacy watchdog driver to use the watchdog core.
> > > 
> > > Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> > > Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> > > Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> > > Added functions: hpwdt_settimeout
> > > Added structures: watchdog_device
> > > 
> > > Signed-off-by: Jerry Hoemann 
> > 
> > I think it would be better to use
> > dev_emerg()
> > dev_crit()
> > dev_alert()
> > dev_err()
> > dev_warn()
> > dev_notice()
> > 
> > instead of pr_* functions now when we have a device to use.
> 
> In general, is there something bad with using pr_debug, etc.,?
> 

No, but it is desirable to use dev_ functions where possible.

> When converting the driver over, I had many more debug messages being
> printed from locations where I didn't have a valid device handle and
> those needed to be done w/ the pr_.* functions.  So, I just uniformally
> used pr_.*.
> 
> 
> > > -static int hpwdt_time_left(void)
> > > +static unsigned int hpwdt_gettimeleft(struct watchdog_device *dev)
> > >  {
> > >   return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> > >  }
> > >  
> > > +static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int 
> > > val)
> > > +{
> > > + pr_debug("settimeout = %d\n", val);
> > 
> > dev_dbg()
> 
> I think you are proposing I do:
> 
>   dev_dbg(dev->parent, "settimeout = %d\n", val);
> 
> This raises the issue that I didn't set parent and I believe I should have.
> 
Correct.

> 
> > 
> > 
> > > +
> > > + soft_margin = dev->timeout = val;
> > 
> > No need to update soft_margin
> 
> I'd made the permission on the module parameters 0444 so that they
> would show up in sysfs.  I updated this value so that I could see
> current value of timeout in sysfs.  But, as Guenter points out in a
> later review, these values are available in under CONFIG_WATCHDOG_SYSFS.
> So, I'll remove.
> 
> 
> > > + pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> > 
> > dev_dbg()
> 
> See above.
> 
> > 
> > >   retval = hpwdt_init_nmi_decoding(dev);
> > >   if (retval != 0)
> > >   goto error_init_nmi_decoding;
> > >  
> > > - retval = misc_register(_miscdev);
> > > + retval = watchdog_register_device(_dev);
> > >   if (retval < 0) {
> > > - dev_warn(>dev,
> > > - "Unable to register miscdev on minor=%d (err=%d).\n",
> > > - WATCHDOG_MINOR, retval);
> > > - goto error_misc_register;
> > > + dev_warn(>dev, "Unable to register hpe watchdog 
> > > (err=%d).\n", retval);
> > > + goto error_wd_register;
> > > + }
> > > +
> > > + watchdog_set_nowayout(_dev, nowayout);
> > > + if (watchdog_init_timeout(_dev, soft_margin, NULL)) {
> > > + dev_warn(>dev, "Invalid soft_margin: %d. Using default\n", 
> > > soft_margin);
> > > + soft_margin = DEFAULT_MARGIN;
> > >   }
> > 
> > In this case watchdog_init_timout() will:
> > 1) Check if soft_margin is valid
> > 2) if not, keep hpwdt_dev.timout intact (which is already set in
> > declaration of hpwdt_dev)
> > 
> > So we could remove the condition and only keep
> > watchdog_init_timeout(_dev, soft_margin, NULL);
> > 
> > 
> > Also, we need to set an invalid initial value for soft_margin to make
> > the logic for watchdog_init_timeout work. 
> > 
> > i.e
> > - static unsigned int soft_margin = DEFAULT_MARGIN; /* in seconds */
> > + static unsigned int soft_margin;
> 
> 
> I don't see the core printing a warning message if an invalid value
> is specified for soft_margin when loading the module.
> 
> I don't mind the hpwdt module correcting an out of range parameter, but I
> don't think it should do so siliently.
> 

The point here is that setting soft_margin to 0 and setting the
timeout in the watchdog_device structure to DEFAULT_MARGIN
means that it is not necessary to override it on error.
If you want to be verbose, you can still log a warning message
if the timeout provided by the module parameter is wrong.

Anyway, this driver will presumably never support devicetree,
so the watchdog core will never read the timeout from there,
and this is not a showstopper.

Guenter


Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

2018-02-13 Thread Jerry Hoemann


Thanks for the review. Comments inline.

On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> Hi Jerry,
> 
> On Sun, Feb 11, 2018 at 10:21:06PM -0700, Jerry Hoemann wrote:
> > Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> > convert hpwdt from legacy watchdog driver to use the watchdog core.
> > 
> > Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> > Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> > Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> > Added functions: hpwdt_settimeout
> > Added structures: watchdog_device
> > 
> > Signed-off-by: Jerry Hoemann 
> 
> I think it would be better to use
> dev_emerg()
> dev_crit()
> dev_alert()
> dev_err()
> dev_warn()
> dev_notice()
> 
> instead of pr_* functions now when we have a device to use.

In general, is there something bad with using pr_debug, etc.,?

When converting the driver over, I had many more debug messages being
printed from locations where I didn't have a valid device handle and
those needed to be done w/ the pr_.* functions.  So, I just uniformally
used pr_.*.


> > -static int hpwdt_time_left(void)
> > +static unsigned int hpwdt_gettimeleft(struct watchdog_device *dev)
> >  {
> > return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> >  }
> >  
> > +static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> > +{
> > +   pr_debug("settimeout = %d\n", val);
> 
> dev_dbg()

I think you are proposing I do:

dev_dbg(dev->parent, "settimeout = %d\n", val);

This raises the issue that I didn't set parent and I believe I should have.


> 
> 
> > +
> > +   soft_margin = dev->timeout = val;
> 
> No need to update soft_margin

I'd made the permission on the module parameters 0444 so that they
would show up in sysfs.  I updated this value so that I could see
current value of timeout in sysfs.  But, as Guenter points out in a
later review, these values are available in under CONFIG_WATCHDOG_SYSFS.
So, I'll remove.


> > +   pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> 
> dev_dbg()

See above.

> 
> > retval = hpwdt_init_nmi_decoding(dev);
> > if (retval != 0)
> > goto error_init_nmi_decoding;
> >  
> > -   retval = misc_register(_miscdev);
> > +   retval = watchdog_register_device(_dev);
> > if (retval < 0) {
> > -   dev_warn(>dev,
> > -   "Unable to register miscdev on minor=%d (err=%d).\n",
> > -   WATCHDOG_MINOR, retval);
> > -   goto error_misc_register;
> > +   dev_warn(>dev, "Unable to register hpe watchdog 
> > (err=%d).\n", retval);
> > +   goto error_wd_register;
> > +   }
> > +
> > +   watchdog_set_nowayout(_dev, nowayout);
> > +   if (watchdog_init_timeout(_dev, soft_margin, NULL)) {
> > +   dev_warn(>dev, "Invalid soft_margin: %d. Using default\n", 
> > soft_margin);
> > +   soft_margin = DEFAULT_MARGIN;
> > }
> 
> In this case watchdog_init_timout() will:
> 1) Check if soft_margin is valid
> 2) if not, keep hpwdt_dev.timout intact (which is already set in
> declaration of hpwdt_dev)
> 
> So we could remove the condition and only keep
> watchdog_init_timeout(_dev, soft_margin, NULL);
> 
> 
> Also, we need to set an invalid initial value for soft_margin to make
> the logic for watchdog_init_timeout work. 
> 
> i.e
> - static unsigned int soft_margin = DEFAULT_MARGIN;   /* in seconds */
> + static unsigned int soft_margin;


I don't see the core printing a warning message if an invalid value
is specified for soft_margin when loading the module.

I don't mind the hpwdt module correcting an out of range parameter, but I
don't think it should do so siliently.


> 
> >  
> > dev_info(>dev, "HPE Watchdog Timer Driver: %s"
> > ", timer margin: %d seconds (nowayout=%d).\n",
> > -   HPWDT_VERSION, soft_margin, nowayout);
> 
> print hpwdt_dev.timeout instead of soft_margin
> 


Will do.



-- 

-
Jerry Hoemann  Software Engineer   Hewlett Packard Enterprise
-


Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

2018-02-13 Thread Guenter Roeck
On Mon, Feb 12, 2018 at 10:06:21AM +0100, Marcus Folkesson wrote:
> Hi Jerry,
> 
> On Sun, Feb 11, 2018 at 10:21:06PM -0700, Jerry Hoemann wrote:
> > Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> > convert hpwdt from legacy watchdog driver to use the watchdog core.
> > 
> > Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> > Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> > Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> > Added functions: hpwdt_settimeout
> > Added structures: watchdog_device
> > 
> > Signed-off-by: Jerry Hoemann 
> 
I second Marcus' feedback.

Thanks,
Guenter

> I think it would be better to use
> dev_emerg()
> dev_crit()
> dev_alert()
> dev_err()
> dev_warn()
> dev_notice()
> 
> instead of pr_* functions now when we have a device to use.
> 
> > ---
> >  drivers/watchdog/hpwdt.c | 251 
> > ---
> >  1 file changed, 63 insertions(+), 188 deletions(-)
> > 
> > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> > index dead59f9ca80..740d0c633204 100644
> > --- a/drivers/watchdog/hpwdt.c
> > +++ b/drivers/watchdog/hpwdt.c
> > @@ -14,18 +14,13 @@
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> >  #include 
> > -#include 
> > -#include 
> > -#include 
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> > -#include 
> >  #include 
> > +
> >  #include 
> >  
> >  #define HPWDT_VERSION  "1.4.0"
> > @@ -40,8 +35,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
> >  #ifdef CONFIG_HPWDT_NMI_DECODING
> >  static unsigned int allow_kdump = 1;
> >  #endif
> > -static char expect_release;
> > -static unsigned long hpwdt_is_open;
> >  
> >  static void __iomem *pci_mem_addr; /* the PCI-memory address */
> >  static unsigned long __iomem *hpwdt_nmistat;
> > @@ -55,53 +48,58 @@ static const struct pci_device_id hpwdt_devices[] = {
> >  };
> >  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
> >  
> > +static struct watchdog_device hpwdt_dev;
> >  
> >  /*
> >   * Watchdog operations
> >   */
> > -static void hpwdt_start(void)
> > +static int hpwdt_start(struct watchdog_device *dev)
> >  {
> > -   reload = SECS_TO_TICKS(soft_margin);
> > +   reload = SECS_TO_TICKS(dev->timeout);
> > +
> > iowrite16(reload, hpwdt_timer_reg);
> > iowrite8(0x85, hpwdt_timer_con);
> > +
> > +   return 0;
> >  }
> >  
> > -static void hpwdt_stop(void)
> > +static int hpwdt_stop(struct watchdog_device *dev)
> >  {
> > unsigned long data;
> >  
> > data = ioread8(hpwdt_timer_con);
> > data &= 0xFE;
> > iowrite8(data, hpwdt_timer_con);
> > +   return 0;
> >  }
> >  
> > -static void hpwdt_ping(void)
> > -{
> > -   iowrite16(reload, hpwdt_timer_reg);
> > -}
> > -
> > -static int hpwdt_change_timer(int new_margin)
> > +static int hpwdt_ping(struct watchdog_device *dev)
> >  {
> > -   if (new_margin < 1 || new_margin > HPWDT_MAX_TIMER) {
> > -   pr_warn("New value passed in is invalid: %d seconds\n",
> > -   new_margin);
> > -   return -EINVAL;
> > -   }
> > +   reload = SECS_TO_TICKS(dev->timeout);
> >  
> > -   soft_margin = new_margin;
> > -   pr_debug("New timer passed in is %d seconds\n", new_margin);
> > -   reload = SECS_TO_TICKS(soft_margin);
> > +   iowrite16(reload, hpwdt_timer_reg);
> >  
> > return 0;
> >  }
> >  
> > -static int hpwdt_time_left(void)
> > +static unsigned int hpwdt_gettimeleft(struct watchdog_device *dev)
> >  {
> > return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
> >  }
> >  
> > +static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> > +{
> > +   pr_debug("settimeout = %d\n", val);
> 
> dev_dbg()
> 
> 
> > +
> > +   soft_margin = dev->timeout = val;
> 
> No need to update soft_margin
> 
> > +   hpwdt_ping(dev);
> > +
> > +   return 0;
> > +}
> > +
> >  #ifdef CONFIG_HPWDT_NMI_DECODING   /* { */
> > -static int hpwdt_my_nmi(void)
> > +
> > +static unsigned int hpwdt_my_nmi(void)
> >  {
> > return ioread8(hpwdt_nmistat) & 0x6;
> >  }
> > @@ -128,8 +126,10 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> > struct pt_regs *regs)
> > if ((ulReason == NMI_UNKNOWN) && !mynmi)
> > return NMI_DONE;
> >  
> > +   pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);
> 
> dev_dbg()
> 
> > +
> > if (allow_kdump)
> > -   hpwdt_stop();
> > +   hpwdt_stop(_dev);
> >  
> > panic_msg[0] = hexdigit((mynmi>>4)&0xf);
> > panic_msg[1] = hexdigit(mynmi&0xf);
> > @@ -140,68 +140,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> > struct pt_regs *regs)
> >  }
> >  #endif /* } */
> >  
> > -/*
> > - * /dev/watchdog handling
> > - */
> > -static int hpwdt_open(struct inode *inode, struct file *file)
> > -{
> > -   /* /dev/watchdog can only be opened once */

Re: [PATCH v2 06/11] watchdog/hpwdt: Modify to use watchdog core.

2018-02-12 Thread Marcus Folkesson
Hi Jerry,

On Sun, Feb 11, 2018 at 10:21:06PM -0700, Jerry Hoemann wrote:
> Follow Documentation/watchdog/convert_drivers_to_kernel_api.txt to
> convert hpwdt from legacy watchdog driver to use the watchdog core.
> 
> Removed functions: hpwdt_open, hpwdt_release, hpwdt_write, hpwdt_ioctl
> Removed data structures: hpwdt_fops, hpwdt_miscdev, watchdog_device
> Modified functions: hpwdt_start, hpwdt_stop, hpwdt_ping, hpwdt_gettimeleft
> Added functions: hpwdt_settimeout
> Added structures: watchdog_device
> 
> Signed-off-by: Jerry Hoemann 

I think it would be better to use
dev_emerg()
dev_crit()
dev_alert()
dev_err()
dev_warn()
dev_notice()

instead of pr_* functions now when we have a device to use.

> ---
>  drivers/watchdog/hpwdt.c | 251 
> ---
>  1 file changed, 63 insertions(+), 188 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index dead59f9ca80..740d0c633204 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -14,18 +14,13 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include 
> -#include 
> -#include 
> -#include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
> -#include 
>  #include 
> +
>  #include 
>  
>  #define HPWDT_VERSION"1.4.0"
> @@ -40,8 +35,6 @@ static bool nowayout = WATCHDOG_NOWAYOUT;
>  #ifdef CONFIG_HPWDT_NMI_DECODING
>  static unsigned int allow_kdump = 1;
>  #endif
> -static char expect_release;
> -static unsigned long hpwdt_is_open;
>  
>  static void __iomem *pci_mem_addr;   /* the PCI-memory address */
>  static unsigned long __iomem *hpwdt_nmistat;
> @@ -55,53 +48,58 @@ static const struct pci_device_id hpwdt_devices[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>  
> +static struct watchdog_device hpwdt_dev;
>  
>  /*
>   *   Watchdog operations
>   */
> -static void hpwdt_start(void)
> +static int hpwdt_start(struct watchdog_device *dev)
>  {
> - reload = SECS_TO_TICKS(soft_margin);
> + reload = SECS_TO_TICKS(dev->timeout);
> +
>   iowrite16(reload, hpwdt_timer_reg);
>   iowrite8(0x85, hpwdt_timer_con);
> +
> + return 0;
>  }
>  
> -static void hpwdt_stop(void)
> +static int hpwdt_stop(struct watchdog_device *dev)
>  {
>   unsigned long data;
>  
>   data = ioread8(hpwdt_timer_con);
>   data &= 0xFE;
>   iowrite8(data, hpwdt_timer_con);
> + return 0;
>  }
>  
> -static void hpwdt_ping(void)
> -{
> - iowrite16(reload, hpwdt_timer_reg);
> -}
> -
> -static int hpwdt_change_timer(int new_margin)
> +static int hpwdt_ping(struct watchdog_device *dev)
>  {
> - if (new_margin < 1 || new_margin > HPWDT_MAX_TIMER) {
> - pr_warn("New value passed in is invalid: %d seconds\n",
> - new_margin);
> - return -EINVAL;
> - }
> + reload = SECS_TO_TICKS(dev->timeout);
>  
> - soft_margin = new_margin;
> - pr_debug("New timer passed in is %d seconds\n", new_margin);
> - reload = SECS_TO_TICKS(soft_margin);
> + iowrite16(reload, hpwdt_timer_reg);
>  
>   return 0;
>  }
>  
> -static int hpwdt_time_left(void)
> +static unsigned int hpwdt_gettimeleft(struct watchdog_device *dev)
>  {
>   return TICKS_TO_SECS(ioread16(hpwdt_timer_reg));
>  }
>  
> +static int hpwdt_settimeout(struct watchdog_device *dev, unsigned int val)
> +{
> + pr_debug("settimeout = %d\n", val);

dev_dbg()


> +
> + soft_margin = dev->timeout = val;

No need to update soft_margin

> + hpwdt_ping(dev);
> +
> + return 0;
> +}
> +
>  #ifdef CONFIG_HPWDT_NMI_DECODING /* { */
> -static int hpwdt_my_nmi(void)
> +
> +static unsigned int hpwdt_my_nmi(void)
>  {
>   return ioread8(hpwdt_nmistat) & 0x6;
>  }
> @@ -128,8 +126,10 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> struct pt_regs *regs)
>   if ((ulReason == NMI_UNKNOWN) && !mynmi)
>   return NMI_DONE;
>  
> + pr_debug("nmi: ulReason=%d, mynmi=0x%0x\n", ulReason, mynmi);

dev_dbg()

> +
>   if (allow_kdump)
> - hpwdt_stop();
> + hpwdt_stop(_dev);
>  
>   panic_msg[0] = hexdigit((mynmi>>4)&0xf);
>   panic_msg[1] = hexdigit(mynmi&0xf);
> @@ -140,68 +140,6 @@ static int hpwdt_pretimeout(unsigned int ulReason, 
> struct pt_regs *regs)
>  }
>  #endif   /* } */
>  
> -/*
> - *   /dev/watchdog handling
> - */
> -static int hpwdt_open(struct inode *inode, struct file *file)
> -{
> - /* /dev/watchdog can only be opened once */
> - if (test_and_set_bit(0, _is_open))
> - return -EBUSY;
> -
> - /* Start the watchdog */
> - hpwdt_start();
> - hpwdt_ping();
> -
> - return nonseekable_open(inode, file);
> -}
> -
> -static int hpwdt_release(struct inode *inode, struct file *file)
> -{
> - /* Stop the watchdog */
> - if (expect_release == 42) {
> -