Hi Arnd,
Great thanks for your review,
The feedback is inline below:
On 05/15/2015 09:57 PM, Arnd Bergmann wrote:
> On Friday 15 May 2015 19:24:49 fu....@linaro.org wrote:
>> From: Fu Wei <fu....@linaro.org>
>>
>> (1)Use linux kernel watchdog framework
>> (2)Work with FDT on ARM64
>> (3)Use "pretimeout" in watchdog framework
>> (4)In first timeout(WS0), do panic to save system context
>> (5)support geting timeout and pretimeout from
>> parameter and FDT at the driver init stage.
>>
>> Signed-off-by: Fu Wei <fu....@linaro.org>
>
> The patch looks good overall. Please try to describe in the patch in
> full sentences in the changelog, as we normally do.
Sure, you will see changelog in the next version.
>
> A few tiny details that I'd do differently, but don't have to change
> if the watchdog maintainer is fine with your version:
>
>> +struct sbsa_gwdt {
>> + struct watchdog_device wdd;
>> + u32 clk;
>> + void __iomem *refresh_base;
>> + void __iomem *control_base;
>> +#ifdef CONFIG_PM_SLEEP
>> + spinlock_t lock;
>> + u8 pm_status_store;
>> +#endif
>> +};
>
> I would drop the #ifdef here, and favor readability over saving
> a few bytes.
yes, that make sense :-)
>
>> + /*
>> + * Try to determine the frequency from the cp15 interface
>> + */
>> + clk = arch_timer_get_cntfrq();
>> + if (!clk) {
>> + dev_err(dev, "System Counter frequency not available\n");
>> + return -EINVAL;
>> + }
>
> Is it guaranteed that the same clock feeds the arch timer and the
> watchdog? Maybe it would be better to use the clk API to read
> the frequency, so we can avoid this dependency.
yes. you are right. According to SBSA doc, the clocksource of SBSA watchdog is
System Counter.
And System Counter is in (arm_)arch_timer. So I think we should do
depends on ARM_ARCH_TIMER
and use the relevant interface :
clk = arch_timer_get_rate();
will improve it, thanks for your suggestion!
>
>> +
>> + pr_debug("sbsa_gwdt: ioremap %s frame 0x%llx(size: %llu)-->%p.\n",
>> + res->name, (unsigned long long)res->start,
>> + (unsigned long long)(res->end - res->start + 1), rf_base);
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
>> + cf_base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(rf_base))
>> + return PTR_ERR(rf_base);
>> +
>> + pr_debug("sbsa_gwdt: ioremap %s frame 0x%llx(size: %llu)-->%p.\n",
>> + res->name, (unsigned long long)res->start,
>> + (unsigned long long)(res->end - res->start + 1), cf_base);
>
> I would probably drop the various pr_debug() calls here. Once the driver
> works fine, they are normally not that useful any more.
yes, for this drive, if it works fine, we can drop it, but I keep these info
for some reason:
(1)they can help engineer debug GTDT table or DTS, if the info of watchdog goes
wrong.
(2)check the memory map
(3)if DEBUG is disable, all pr_debug are no_printk, it won't increase Image
size or output any through console.
>
> Arnd
>
--
Best regards,
Fu Wei
Software Engineer From Red Hat
LEG Team
Linaro.org | Open source software for ARM SoCs
Ph: +86 186 2020 4684 (mobile)
IRC: fuwei
Skype: tekkamanninja
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
