On Sun, May 15, 2016 at 7:04 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>> +
>> +struct aspeed_wdt {
>> + struct watchdog_device wdd;
>> + unsigned long rate;
>
> I still don't see why you need this variable. Why not just use
> WDT_1MHZ_CLOCK directly ?
Yes, I can. It's left over from when the driver ran the device from pclk.
>> +static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>> + unsigned int timeout)
>> +{
>> + wdd->timeout = timeout;
>> +
>> + return aspeed_wdt_start(wdd);
>
> The watchdog can be stopped here (with WDIOC_SETOPTIONS/WDIOS_DISABLECARD).
> An implicit enable is not really a good idea; neither the infrastructure
> nor user space would in this case be aware that the watchdog is running.
Okay, good to know. The documentation does not mention this.
I will change it to write the value but not change the running state.
>> +}
>> +
>> +static int aspeed_wdt_restart(struct watchdog_device *wdd,
>> + unsigned long action, void *data)
>> +{
>> + struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>> +
>> + aspeed_wdt_enable(wdt, 128 * wdt->rate / 1000);
>> +
>
>
> Is that the smallest accepted value, or is there some other reason
> not to make it smaller ?
It's what the vendor BSP was using, and it's what we've been using
since I wrote this code (which happens to be one year ago today!).
>
> It is also quite common to wait here to let the reset 'catch'
> before returning. This would ensure that the system doesn't trigger
> a lower priority reset.
Okay. I will add a mdelay(1000) here.
>> + ret = watchdog_register_device(&wdt->wdd);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to register\n");
>> + return ret;
>> + }
>> +
>> + dev_info(&pdev->dev, "rate %lu, max timeout %u, timeout %d\n",
>> + wdt->rate, wdt->wdd.max_timeout, wdt->wdd.timeout);
>> +
>
> 'rate 1000000' doesn't seem very informative. All it does is to expose
> an implementation detail which no one really cares about. Even if you
> were to add 'Hz', I would argue that no one will even understand
> what it means without looking into the driver, but then it will be
> understood without the message.
Okay. I will remove the dev_info all together.
Cheers,
Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
