linux-mips
[Top] [All Lists]

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

To: rtc-linux@googlegroups.com
Subject: Re: [rtc-linux] [PATCH V1] MIPS: Add RTC support for loongson1B
From: Wolfram Sang <w.sang@pengutronix.de>
Date: Sun, 27 Nov 2011 10:18:03 +0100
Cc: a.zummo@towertech.it, linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, ralf@linux-mips.org, keguang.zhang@gmail.com, wuzhangjin@gmail.com, r0bertz@gentoo.org, zhao zhang <zhzhl555@gmail.com>
In-reply-to: <1322189527-4761-1-git-send-email-zhzhl555@gmail.com>
References: <1322189527-4761-1-git-send-email-zhzhl555@gmail.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
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/  |

Attachment: signature.asc
Description: Digital signature

<Prev in Thread] Current Thread [Next in Thread>