| Message ID | 1444340074-15437-7-git-send-email-sylvain.rochet@finsecur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sylvain, The rest of the series looks good to me, one comment below: On 08/10/2015 at 23:34:34 +0200, Sylvain Rochet wrote : > watchdog_init_timeout function doesn't try to get the "timeout-sec" DT > property if timeout_parm is not zero. This change makes this DT property > working for the sama5d4 watchdog driver. > While I'm not sure of the feasibility, I think that the module parameter should override the DT property.
> -----Original Message----- > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] > Sent: 2015?10?12? 15:50 > To: Sylvain Rochet > Cc: Guenter Roeck; Boris BREZILLON; linux-kernel@vger.kernel.org; Ferre, > Nicolas; Desroches, Ludovic; linux-arm-kernel@lists.infradead.org; Yang, > Wenyou; Wim Van Sebroeck > Subject: Re: [PATCH 6/6] watchdog: sama5d4: try to set timeout from device tree > first > > Hi Sylvain, > > The rest of the series looks good to me, one comment below: > > On 08/10/2015 at 23:34:34 +0200, Sylvain Rochet wrote : > > watchdog_init_timeout function doesn't try to get the "timeout-sec" DT > > property if timeout_parm is not zero. This change makes this DT > > property working for the sama5d4 watchdog driver. > > > > While I'm not sure of the feasibility, I think that the module parameter should > override the DT property. The patch should be right, the DT property overrides the module parameter. If the DT property is not a valid value, it uses the default value, initialized with the module parameter at the beginning of probe. > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering http://free-electrons.com Best Regards, Wenyou Yang
Hi Alexandre, On Mon, Oct 12, 2015 at 09:50:01AM +0200, Alexandre Belloni wrote: > Hi Sylvain, > > The rest of the series looks good to me, one comment below: > > On 08/10/2015 at 23:34:34 +0200, Sylvain Rochet wrote : > > watchdog_init_timeout function doesn't try to get the "timeout-sec" DT > > property if timeout_parm is not zero. This change makes this DT property > > working for the sama5d4 watchdog driver. > > While I'm not sure of the feasibility, I think that the module parameter > should override the DT property. That's not that hard, we can remove the initialisation of wdt_timeout to WDT_DEFAULT_TIMEOUT and use the 0 magic value, which is not an acceptable timeout value, to tell whether the variable was set with a module parameter or not. I followed what was done in the at91sam9_wdt driver but I agree the module parameter should override the DT property, if we all agree on that, I will also change this behavior in at91sam9_wdt in v2, at least for the sake of coherency between drivers. Sylvain
Hi Wenyou, On Mon, Oct 12, 2015 at 08:12:42AM +0000, Yang, Wenyou wrote: > > -----Original Message----- > > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] > > Sent: 2015?10?12? 15:50 > > To: Sylvain Rochet > > Cc: Guenter Roeck; Boris BREZILLON; linux-kernel@vger.kernel.org; Ferre, > > Nicolas; Desroches, Ludovic; linux-arm-kernel@lists.infradead.org; Yang, > > Wenyou; Wim Van Sebroeck > > Subject: Re: [PATCH 6/6] watchdog: sama5d4: try to set timeout from device tree > > first > > > > Hi Sylvain, > > > > The rest of the series looks good to me, one comment below: > > > > On 08/10/2015 at 23:34:34 +0200, Sylvain Rochet wrote : > > > watchdog_init_timeout function doesn't try to get the "timeout-sec" DT > > > property if timeout_parm is not zero. This change makes this DT > > > property working for the sama5d4 watchdog driver. > > > > > > > While I'm not sure of the feasibility, I think that the module parameter should > > override the DT property. > > The patch should be right, the DT property overrides the module > parameter. > > If the DT property is not a valid value, it uses the default value, > initialized with the module parameter at the beginning of probe. Well, the principle of least surprise applied here means if you load the module with a timeout argument, you expect the timeout argument to be used and not the dt one. As such, it makes more sense to have the parameter value takes precedence over the dt value. Sylvain
diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c index a49634c..2e2049b 100644 --- a/drivers/watchdog/sama5d4_wdt.c +++ b/drivers/watchdog/sama5d4_wdt.c @@ -222,7 +222,10 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) } } - ret = watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev); + /* Try to set timeout from device tree first */ + ret = watchdog_init_timeout(wdd, 0, &pdev->dev); + if (ret) + ret = watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev); if (ret) { dev_err(&pdev->dev, "unable to set timeout value\n"); return ret; @@ -243,7 +246,7 @@ static int sama5d4_wdt_probe(struct platform_device *pdev) platform_set_drvdata(pdev, wdt); dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = %d)\n", - wdt_timeout, nowayout); + wdd->timeout, nowayout); return 0; }
watchdog_init_timeout function doesn't try to get the "timeout-sec" DT property if timeout_parm is not zero. This change makes this DT property working for the sama5d4 watchdog driver. Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com> --- drivers/watchdog/sama5d4_wdt.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)