Re: [PATCH v2 1/2] watchdog: Add watchdog driver for Intel Keembay Soc

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Dec 01, 2020 at 06:19:09AM +0000, Ayyathurai, Vijayakannan wrote:
> Hi Guenter,
> 
> > From: Guenter Roeck <linux@roeck-us.net>
> > 
> > On Wed, Nov 11, 2020 at 01:53:07AM +0800,
> > vijayakannan.ayyathurai@intel.com wrote:
> > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>
> > >
> > > Intel Keembay Soc requires watchdog timer support.
> > > Add watchdog driver to enable this.
> > >
> > > +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)
> > > +		keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout *
> > wdt->rate);
> > > +
> > > +	if (!ping && wdog->pretimeout) {
> > > +		th_val = wdog->timeout - wdog->pretimeout;
> > > +		keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val
> > * wdt->rate);
> > > +	}
> > > +
> > > +	if (!ping)
> > > +		keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout *
> > wdt->rate);
> > 
> > I am a bit at loss here. This seems unnecessarily complex. Why not just the
> > following ?
> > 
> 
> Sure. I can follow the below way.
> Let me know if there is further comments to improve for the next version.
> 
No, that was all. Driver looks good otherwise.

Guenter

> > 	if (!ping && wdog->pretimeout) {
> > 		th_val = wdog->timeout - wdog->pretimeout;
> > 		keembay_wdt_writel(wdt, TIM_WATCHDOG_INT_THRES, th_val
> > * wdt->rate);
> > 	}
> > 	keembay_wdt_writel(wdt, TIM_WATCHDOG, wdog->timeout * wdt-
> > >rate);
> > 
> > Thanks,
> > Guenter
> 
> Thanks,
> Vijay