On Tue, 2011-07-05 at 18:51 +0530, ashishj3 wrote:
This driver adds support for the watchdog functionality provided by the Dialog Semiconductor DA9052 PMIC chip.
Signed-off-by: David Dajun Chen dchen@diasemi.com Signed-off-by: Ashish Jangam ashish.jangam@kpitcummins.com
If there are no comments then can you ACK this patch?
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@diasemi.com Signed-off-by: Ashish Jangam ashish.jangam@kpitcummins.com
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.
-----Original Message----- From: Wim Van Sebroeck [mailto:wim@iguana.be] Sent: Thursday, July 14, 2011 3:59 PM To: Ashish Jangam Cc: linux-kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; Dajun; linaro-dev@lists.linaro.org Subject: Re: [PATCH 07/11] Watchdog: DA9052 watchdog support v1
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@diasemi.com Signed-off-by: Ashish Jangam ashish.jangam@kpitcummins.com
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);elsewdt->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.
If strobe filter is "Enable" (set/reset thru ioctl) then only a new wdt time window is set else if strobe filter is "Disable", new wdt time window is set to a min value or wdt is disabled depending upon condition. If the logic is complex then I will simplify it.
Kind regards, Wim.
-
Ashish Jangam -
ashishj3
-
Wim Van Sebroeck