| 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
| ||
|
Message-Id: <20100525135159.166d91c6.akpm@linux-foundation.org>
Date: Tue, 25 May 2010 13:51:59 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Rabin Vincent <rabin.vincent@...ricsson.com>
Cc: Alessandro Zummo <a.zummo@...ertech.it>,
<rtc-linux@...glegroups.com>, Samuel Ortiz <sameo@...ux.intel.com>,
<linux-kernel@...r.kernel.org>,
<STEricsson_nomadik_linux@...t.st.com>,
Virupax Sadashivpetimath
<virupax.sadashivpetimath@...ricsson.com>,
Linus Walleij <linus.walleij@...ricsson.com>,
Srinidhi Kasagar <srinidhi.kasagar@...ricsson.com>
Subject: Re: [PATCH] rtc: AB8500 RTC driver
On Fri, 21 May 2010 20:26:36 +0530
Rabin Vincent <rabin.vincent@...ricsson.com> wrote:
> From: Virupax Sadashivpetimath <virupax.sadashivpetimath@...ricsson.com>
>
> Add a driver for the RTC on the AB8500 power management chip. This is a
> client of the AB8500 MFD driver.
>
>
> ...
>
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -611,6 +611,14 @@ config RTC_DRV_AB3100
> Select this to enable the ST-Ericsson AB3100 Mixed Signal IC RTC
> support. This chip contains a battery- and capacitor-backed RTC.
>
> +config RTC_DRV_AB8500
> + tristate "ST-Ericsson AB8500 RTC"
> + depends on AB8500_CORE
> + default y
Really? So all AB8500_CORE users will get to include this driver in
their builds just by running `make oldconfig'?
> + help
> + Select this to enable the ST-Ericsson AB8500 power management IC RTC
> + support. This chip contains a battery- and capacitor-backed RTC.
> +
>
> ...
>
> +static unsigned long get_elapsed_seconds(int year)
> +{
> + unsigned long secs;
> + struct rtc_time tm;
> +
> + memset(&tm, 0, sizeof(tm));
> + tm.tm_year = year - 1900;
> + tm.tm_mon = 0;
> + tm.tm_mday = 1;
It would be neater to do
struct rtc_time tm = {
.tm_year = year - 1900,
.tm_mday = 1,
};
> + /*
> + * This function calculates secs from 1970 and not from
> + * 1900, even if we supply the offset from year 1900.
> + */
> + rtc_tm_to_time(&tm, &secs);
> + return secs;
> +}
> +
> +static int ab8500_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ab8500 *ab8500 = dev_get_drvdata(dev->parent);
> + unsigned long timeout = jiffies + HZ;
> + int retval, i;
> + unsigned long mins, secs;
> + unsigned char buf[5];
> + const unsigned long time_regs[] = {AB8500_RTC_WATCH_TMIN_HI_REG,
> + AB8500_RTC_WATCH_TMIN_MID_REG, AB8500_RTC_WATCH_TMIN_LOW_REG,
> + AB8500_RTC_WATCH_TSECHI_REG, AB8500_RTC_WATCH_TSECMID_REG};
iirc, the compiler will quietly treat this as `static const', which is
what we want.
> + /* Request a data read */
> + retval = ab8500_write(ab8500, AB8500_RTC_READ_REQ_REG,
> + RTC_READ_REQUEST);
> + if (retval < 0)
> + return retval;
> +
> + if (!ab8500->revision) {
> + msleep(1);
This bit needs a comment. Because there's no way of understanding it
by reading the code!
> + } else {
> + /* Wait for some cycles after enabling the rtc read in ab8500 */
> + while (time_before(jiffies, timeout)) {
> + retval = ab8500_read(ab8500, AB8500_RTC_READ_REQ_REG);
> + if (retval < 0)
> + return retval;
> +
> + if (!(retval & RTC_READ_REQUEST))
> + break;
> +
> + msleep(1);
> + }
> + }
> +
> + /* Read the Watchtime registers */
> + for (i = 0; i < ARRAY_SIZE(time_regs); i++) {
> + retval = ab8500_read(ab8500, time_regs[i]);
> + if (retval < 0)
> + return retval;
> + buf[i] = retval;
> + }
> +
> + mins = (buf[0] << 16) | (buf[1] << 8) | buf[2];
> +
> + secs = (buf[3] << 8) | buf[4];
> + secs = secs / COUNTS_PER_SEC;
> + secs = secs + (mins * 60);
> +
> + /* Add back the initially subtracted number of seconds */
> + secs += get_elapsed_seconds(AB8500_RTC_EPOCH);
> +
> + rtc_time_to_tm(secs, tm);
> + return rtc_valid_tm(tm);
> +}
> +
> +static int ab8500_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct ab8500 *ab8500 = dev_get_drvdata(dev->parent);
> + int retval, i;
> + unsigned char buf[5];
> + unsigned long no_secs, no_mins, secs = 0;
> + const unsigned long time_regs[] = {AB8500_RTC_WATCH_TMIN_HI_REG,
> + AB8500_RTC_WATCH_TMIN_MID_REG,
> + AB8500_RTC_WATCH_TMIN_LOW_REG, AB8500_RTC_WATCH_TSECHI_REG,
> + AB8500_RTC_WATCH_TSECMID_REG};
> +
> + if (tm->tm_year < (AB8500_RTC_EPOCH - 1900)) {
> + dev_err(dev, "year should be equal to or more than %d\n",
> + AB8500_RTC_EPOCH);
hm, is this really worth the console spam?
"equal to or greater than" would sound more mathematical ;)
> + return -EINVAL;
> + }
> +
> + /* Get the number of seconds since 1970 */
> + rtc_tm_to_time(tm, &secs);
> +
> + /*
> + * Convert it to the number of seconds since 01-01-2000 00:00:00, since
> + * we only have a small counter in the RTC.
> + */
> + secs -= get_elapsed_seconds(AB8500_RTC_EPOCH);
> +
> + no_mins = secs / 60;
> +
> + no_secs = secs % 60;
> + /* Make the seconds count as per the RTC resolution */
> + no_secs = no_secs * COUNTS_PER_SEC;
> +
> + buf[4] = no_secs & 0xFF;
> + buf[3] = (no_secs >> 8) & 0xFF;
> +
> + buf[2] = no_mins & 0xFF;
> + buf[1] = (no_mins >> 8) & 0xFF;
> + buf[0] = (no_mins >> 16) & 0xFF;
> +
> + for (i = 0; i < ARRAY_SIZE(time_regs); i++) {
> + retval = ab8500_write(ab8500, time_regs[i], buf[i]);
> + if (retval < 0)
> + return retval;
> + }
> +
> + /* Request a data write */
> + return ab8500_write(ab8500, AB8500_RTC_READ_REQ_REG, RTC_WRITE_REQUEST);
> +}
> +
>
> ...
>
> +static int __devexit ab8500_rtc_remove(struct platform_device *pdev)
> +{
> + struct rtc_device *rtc = platform_get_drvdata(pdev);
> + int irq = platform_get_irq_byname(pdev, "ALARM");
Seems a bit fragile. I guess that platform_get_irq_byname("ALARM")
will always return the same value, but still. Would it be better to
store the irq number in private storage? Unsure..
> + if (irq >= 0)
> + free_irq(irq, rtc);
> +
> + rtc_device_unregister(rtc);
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists