RE: [PATCH v3 1/2] watchdog: Add watchdog driver for Intel Keembay Soc

["Ayyathurai, Vijayakannan" <vijayakannan.ayyathurai@intel.com>]

Hi Guenter,

> From: Guenter Roeck <linux@roeck-us.net>
> 
> On Wed, Dec 02, 2020 at 02:55:32PM +0000, Ayyathurai, Vijayakannan wrote:
> >
> > > From: Guenter Roeck <linux@roeck-us.net>
> > > Sent: Wednesday, 2 December, 2020 12:31 AM
> > >
> > > On Tue, Dec 01, 2020 at 11:10:33PM +0800,
> > > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > > >
> > > >
> > > > +static void keembay_wdt_set_timeout_reg(struct watchdog_device
> *wdog,
> > > bool ping)
> > > > +{
> > > > +	struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> > > > +	u32 th_val = 0;
> > > > +
> > > > +	if (!ping && wdog->pretimeout) {
> > > > +		th_val = wdog->timeout - wdog->pretimeout;
> > > > +		keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val
> > > * wdt->rate);
> > >
> > > Sorry for annoying you now, but I may have found another potential
> problem.
> > >
> > > What happens if the user sets a pretimeout, then removes it ?
> > > What should TIM_WATCHDOG_INT_THRES be set to in that case ?
> > > Right now TIM_WATCHDOG_INT_THRES won't be updated anymore
> >
> > It is a good catch. Indeed, I don't have coverage like this.
> >
> > > in that case, which seems wrong. This might get worse with
> > > the following sequence.
> > >
> > > - set pretimeout
> > > - clear pretimeout
> > > - set timeout to some other value
> > >
> >
> > Can the below method resolve this issue?
> >
> >
> > static int keembay_wdt_set_pretimeout(struct watchdog_device *wdog, u32
> t)
> > {
> >         struct keembay_wdt *wdt = watchdog_get_drvdata(wdog);
> >
> >         if(!t)
> >                 keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, 0);
> >
> 
> Partially, only it makes for awkward code. After all, it is never really
> necessary to set the timeout register after updating the pretimeout.
> The "ping" parameter makes less and less sense with this in mind.

Yes.

> It might be better to split the set_timeout_reg function into
> set_timeout_reg and set_pretimeout_reg and call those functions as needed
> (and handle the if() above in the set_pretimeout_reg function).
> 

Ok. Thanks for your suggestion.
Let me incorporate necessary changes in the next version.

> Thanks,
> Guenter
>

Thanks,
Vijay