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

[Ashish Jangam]

> -----Original Message-----
> From: Wim Van Sebroeck [mailto:wim@xxxxxxxxx]
> Sent: Thursday, July 14, 2011 3:59 PM
> To: Ashish Jangam
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; Dajun;
> linaro-dev@xxxxxxxxxxxxxxxx
> 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@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.
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.
> 



¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_