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

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]