the bit will be cleared. so if hardware has problem, the while loop will be
infinite, it can never break out. Does i really need to add timeout checking
code although have checked in probe code.
2: Just set the default value, the real return value will be set in the following
code. Line 154, err information will be put into dev_err-block.
3: The minor, accept.
4: Because in probe, we can not assume the hardware is OK,
so add a counter( v = 0x100000) to avoid infinite loop.
5: a): Since i have checked the RTC timing was OK, and toytrim write status was
OK again, so i can sure, the next writing will be OK.
b): Just following the Documentation/timers/timers-howto.txt.
c): i can make sure.
6: agree.
在 2011年11月27日 下午5:18，Wolfram Sang <w.sang@xxxxxxxxxxxxxx>写道：
> + writel(t, SYS_TOYWRITE1);Timeout?
> + while (readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TS)
> + usleep_range(1000, 3000);
> + __asm__ volatile ("sync");
Why not putting this line to the corresponding dev_err-block?
> +
> +static int __devinit ls1x_rtc_probe(struct platform_device *pdev)
> +{
> + struct rtc_device *rtcdev;
> + unsigned long v;
> + int ret;
> +
> + v = readl(SYS_COUNTER_CNTRL);
> + if (!(v & RTC_CNTR_OK)) {
> + dev_err(&pdev->dev, "rtc counters not working\n");
> + ret = -ENODEV;
> + goto err;
> + }
> + ret = -ETIMEDOUT;
Minor: Spaces around comment-markers, here and in other places
> + /*set to 1 HZ if needed*/
/* Comment */
Timeout?
> + if (readl(SYS_TOYTRIM) != 32767) {
> + v = 0x100000;
> + while ((readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TTS) && --v)
> + usleep_range(1000, 3000);
Timeout again. First, the comment does not help. Why are you sure the loop
> +
> + if (!v) {
> + dev_err(&pdev->dev, "time out\n");
> + goto err;
> + }
> + writel(32767, SYS_TOYTRIM);
> + __asm__ volatile("sync");
> + }
> + /*this loop coundn't be endless*/
> + while (readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TTS)
> + usleep_range(1000, 3000);
could not be endless? And: Does it really need to be a usleep_range() instead
of a simple msleep()? And to make sure: There is no interrupt signalling the
status changed?
...
> +
> + rtcdev = rtc_device_register("ls1x-rtc", &pdev->dev,
> + &ls1x_rtc_ops , THIS_MODULE);
> + if (IS_ERR(rtcdev)) {
> + ret = PTR_ERR(rtcdev);
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, rtcdev);
> + return 0;
> +err:
> + return ret;
> +}
> +
Please use the new module_platform_driver()-macro.
> +static int __init ls1x_rtc_init(void)
> +{
> + return platform_driver_probe(&ls1x_rtc_driver, ls1x_rtc_probe);
> +}
> +
> +static void __exit ls1x_rtc_exit(void)
> +{
> + platform_driver_unregister(&ls1x_rtc_driver);
> +}
> +
> +
> +module_init(ls1x_rtc_init);
> +module_exit(ls1x_rtc_exit);
Thanks,
Wolfram
