Re: [rtc-linux] [PATCH V1] MIPS: Add RTC support for loongson1B

[zhao zhang <zhzhl555@gmail.com>]

1:  Here is a  polling checking for TOY write status bit. if hardware done,
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@pengutronix.de>写道：

On Fri, Nov 25, 2011 at 10:52:07AM +0800, zhzhl555@gmail.com wrote:

> +     writel(t, SYS_TOYWRITE1);
> +     while (readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TS)
> +             usleep_range(1000, 3000);
> +     __asm__ volatile ("sync");

Timeout?

> +
> +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;

Why not putting this line to the corresponding dev_err-block?

> +     /*set to 1 HZ if needed*/

Minor: Spaces around comment-markers, here and in other places

/* Comment */

> +     if (readl(SYS_TOYTRIM) != 32767) {
> +             v = 0x100000;
> +             while ((readl(SYS_COUNTER_CNTRL) & SYS_CNTRL_TTS) && --v)
> +                     usleep_range(1000, 3000);

Timeout?

> +
> +             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);

Timeout again. First, the comment does not help. Why are you sure the loop
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;
> +}
> +

...

> +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);

Please use the new module_platform_driver()-macro.

Thanks,

  Wolfram

--
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |