Re: [PATCH 07/11] Watchdog: DA9052 watchdog support v1

[Wim Van Sebroeck]

Hi Ashish, Dajun,

> > This driver adds support for the watchdog functionality provided by the Dialog
> > Semiconductor DA9052 PMIC chip.
> > 
> > Signed-off-by: David Dajun Chen <dchen@xxxxxxxxxxx>
> > Signed-off-by: Ashish Jangam <ashish.jangam@xxxxxxxxxxxxxxx>
> > ---
> If there are no comments then can you ACK this patch?

I remain with my comment of Wed 26 Jan 2011:
> The ioctl WDIOC_SETOPTIONS call is not in line with the watchdog API.
> the supported options are:
> #define WDIOS_DISABLECARD       0x0001  /* Turn off the watchdog timer */
> #define WDIOS_ENABLECARD        0x0002  /* Turn on the watchdog timer */
> #define WDIOS_TEMPPANIC         0x0004  /* Kernel panic on temperature trip */
> Your options are:
> #define DA9052_STROBING_FILTER_ENABLE   0x0001
> #define DA9052_STROBING_FILTER_DISABLE  0x0002
> #define DA9052_SET_STROBING_MODE_MANUAL 0x0004
> #define DA9052_SET_STROBING_MODE_AUTO   0x0008
> 
> Please explain what you want to do.

FYI: this is how the code looks now:
+       case WDIOC_SETOPTIONS:
+               if (get_user(new_value, p))
+                       return -EFAULT;
+               if (new_value == DA9052_ENABLE || new_value == DA9052_DISABLE)
+                       da9052_sm_set_strobing_filter(wdt, new_value);
+               else
+                       wdt->pwdt->sm_strobe_mode_flag = new_value;
+               return 0;

Since the code is still not in line with the watchdog API and since I did not get
an answer yet on what you (or Dajun) want to do, there is definitely NO ACK yet.
If there is a good reasoning behind your needs, we might conclude that this is indeed
worthwhile and extend the API in a proper way. But as it is now, it's not correct.

Kind regards,
Wim.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/