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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20111127091803.GD5263@pengutronix.de>
Date:	Sun, 27 Nov 2011 10:18:03 +0100
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	rtc-linux@...glegroups.com
Cc:	a.zummo@...ertech.it, linux-mips@...ux-mips.org,
	linux-kernel@...r.kernel.org, ralf@...ux-mips.org,
	keguang.zhang@...il.com, wuzhangjin@...il.com, r0bertz@...too.org,
	zhao zhang <zhzhl555@...il.com>
Subject: Re: [rtc-linux] [PATCH V1] MIPS: Add RTC support for loongson1B

On Fri, Nov 25, 2011 at 10:52:07AM +0800, zhzhl555@...il.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/  |

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)