Re: [PATCH 2/3] ARM: MXC: add driver for MXC's internal RTC module

From: Daniel Mack <daniel@caiaq.de>

Hi Alessandro,

On Tue, May 19, 2009 at 05:23:08PM +0200, Alessandro Zummo wrote:
> > +#include <linux/io.h>
> > +#include <linux/rtc.h>
> > +#include <linux/module.h>
> > +#include <linux/fs.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/clk.h>
> > +#include <linux/uaccess.h>
> 
>  need all of those?

Cleaned.

> > +static u32 rtc_freq = 2;	/* minimun value for PIE */
> 
>  minimum

Fixed.

> > +static unsigned long rtc_status;
> > +
> > +static struct rtc_time g_rtc_alarm = {
> > +	.tm_year = 0,
> > +	.tm_mon = 0,
> > +	.tm_mday = 0,
> > +	.tm_hour = 0,
> > +	.tm_mon = 0,
> > +	.tm_sec = 0,
> > +};
> 
>  gcc will probably write zeroing code by itself,
>  but I can't remember the rules (maybe if the thing goes into .bss).

Yes, you're right. The variable is static, no init needed.

> > +static DEFINE_SPINLOCK(rtc_lock);
> 
>  you probably don't need your own lock. check what
>  the other drivers do. 

I'll take rtc->irq_lock

> > +	if (time_alarm == MXC_RTC_TIME) {
> > +		day = readw(ioaddr + RTC_DAYR);
> > +		hr_min = readw(ioaddr + RTC_HOURMIN);
> > +		sec = readw(ioaddr + RTC_SECOND);
> > +	} else if (time_alarm == MXC_RTC_ALARM) {
> > +		day = readw(ioaddr + RTC_DAYALARM);
> > +		hr_min = readw(ioaddr + RTC_ALRM_HM) & 0xffff;
> > +		sec = readw(ioaddr + RTC_ALRM_SEC);
> > +	} else {
> > +		dev_err(dev, "wrong value for time_alarm=%d\n", time_alarm);
> > +		return 0;
> > +	}
> 
>  if you are the only caller of this function there's no need to check
>  that time_alarm is out of specs

Agreed, fixed.

> > + * This function is used to open the RTC driver by registering the RTC
> > + * interrupt service routine.
> > + */
> > +static int mxc_rtc_open(struct device *dev)
> > +{
> > +	if (test_and_set_bit(1, &rtc_status))
> > +		return -EBUSY;
> > +	
> > +	return 0;
> > +}
> 
>  ? 
> 
>  the overlaying subsystem should take care of that,

Ok.

> > +	case RTC_IRQP_SET:
> > +		if (arg < 2 || arg > MAX_PIE_FREQ || (arg % 2) != 0)
> > +			return -EINVAL;	/* Also make sure a power of 2Hz */
> > +		if ((arg > 64) && (!capable(CAP_SYS_RESOURCE)))
> > +			return -EACCES;
> > +		rtc_freq = arg;
> > +		return 0;
> > +
> > +	case RTC_IRQP_READ:
> > +		return put_user(rtc_freq, (u32 *) arg);
> > +
> > +	case RTC_PIE_ON:
> > +		for (i = 0; i < MAX_PIE_NUM; i++)
> > +			if (PIE_BIT_DEF[i][0] == rtc_freq)
> > +				break;
> > +		
> > +		if (i == MAX_PIE_NUM)
> > +			return -EACCES;
> > +
> > +		spin_lock_irq(&rtc_lock);
> > +		writew((readw(ioaddr + RTC_RTCIENR) | PIE_BIT_DEF[i][1]),
> > +		       ioaddr + RTC_RTCIENR);
> > +		spin_unlock_irq(&rtc_lock);
> > +		return 0;
> > +
> > +	case RTC_AIE_OFF:
> > +		spin_lock_irq(&rtc_lock);
> > +		writew((readw(ioaddr + RTC_RTCIENR) & ~RTC_ALM_BIT),
> > +		       ioaddr + RTC_RTCIENR);
> > +		spin_unlock_irq(&rtc_lock);
> > +		return 0;
> > +
> > +	case RTC_AIE_ON:
> > +		spin_lock_irq(&rtc_lock);
> > +		writew((readw(ioaddr + RTC_RTCIENR) | RTC_ALM_BIT),
> > +		       ioaddr + RTC_RTCIENR);
> > +		spin_unlock_irq(&rtc_lock);
> > +		return 0;
> > +
> > +	case RTC_UIE_OFF:	/* UIE is for the 1Hz interrupt */
> > +		spin_lock_irq(&rtc_lock);
> > +		writew((readw(ioaddr + RTC_RTCIENR) & ~RTC_1HZ_BIT),
> > +		       ioaddr + RTC_RTCIENR);
> > +		spin_unlock_irq(&rtc_lock);
> > +		return 0;
> > +
> > +	case RTC_UIE_ON:
> > +		spin_lock_irq(&rtc_lock);
> > +		writew((readw(ioaddr + RTC_RTCIENR) | RTC_1HZ_BIT),
> > +		       ioaddr + RTC_RTCIENR);
> > +		spin_unlock_irq(&rtc_lock);
> > +		return 0;
> > +	}
> > +
> > +	return -ENOIOCTLCMD;
> > +}
> 
>  please only use ioctl for functions that are not already available
>  within the rtc ->ops .

Not quite sure what you mean here - I don't see any overlapping call.
But I don't know the RTC API too well though ...

> > +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	unsigned long time;
> > +	int ret = rtc_tm_to_time(tm, &time);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Avoid roll-over from reading the different registers */
> > +	do {
> > +		set_alarm_or_time(dev, MXC_RTC_TIME, time);
> > +	} while (time != get_alarm_or_time(dev, MXC_RTC_TIME));
> > +
> > +	return 0;
> > +}
> 
>  you need to implement set_mmss instead of set_time

Done.

> > +/* Proc fs layer */
> > +static int mxc_rtc_proc(struct device *dev, struct seq_file *sq)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
> > +	void __iomem *ioaddr = pdata->ioaddr;
> > +	char *p = sq->buf;
> > +
> > +	p += sprintf(p, "alarm_IRQ\t: %s\n",
> > +		     (((readw(ioaddr + RTC_RTCIENR)) & RTC_ALM_BIT) !=
> > +		      0) ? "yes" : "no");
> > +	p += sprintf(p, "update_IRQ\t: %s\n",
> > +		     (((readw(ioaddr + RTC_RTCIENR)) & RTC_1HZ_BIT) !=
> > +		      0) ? "yes" : "no");
> > +	p += sprintf(p, "periodic_IRQ\t: %s\n",
> > +		     (((readw(ioaddr + RTC_RTCIENR)) & PIT_ALL_ON) !=
> > +		      0) ? "yes" : "no");
> > +	p += sprintf(p, "periodic_freq\t: %d\n", rtc_freq);
> > +
> > +	return p - (sq->buf);
> > +}
> 
>  this is deprecated. do you really need it? better do it
>  via sysfs.

Yes, legacy from copied code. Removed that.

> > +	/* Configure and enable the RTC */
> > +	pdata->irq = platform_get_irq(pdev, 0);
> > +
> > +	if (pdata->irq >= 0) {
> > +		if (request_irq(pdata->irq, mxc_rtc_interrupt, IRQF_SHARED,
> > +				pdev->name, pdev) < 0) {
> > +			dev_warn(&pdev->dev, "interrupt not available.\n");
> > +			pdata->irq = -1;
> > +		}
> > +	}
> 
>  request irq should come after rtc_register

Yep, cleaned up the whole function. Should be better now.

> > +static int __init mxc_rtc_init(void)
> > +{
> > +	return platform_driver_register(&mxc_rtc_driver);
> > +}
> > +
> 
>  you can use platform_driver_probe. __exit_p/__init/__exit are only
>  correct with oplatform_DriveR_probe, otherwise you have to use
>  the dev variant.

Ok, fixed.

> > +static void __exit mxc_rtc_exit(void)
> > +{
> > +	platform_driver_unregister(&mxc_rtc_driver);
> > +
> > +}
> > +
> > +module_init(mxc_rtc_init);
> > +module_exit(mxc_rtc_exit);
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> 
>  email here please.

Hmm. I can't find any reference to the original author, and I'm just the
one who cleans up behind him. Don't know ...

> > +MODULE_DESCRIPTION("Realtime Clock Driver (RTC)");
> 
>  bad description

Fixed.

New version attached - thanks for your review!

Daniel

From 30465d9b966db1ac727850aa5223f35eea61e661 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Mon, 18 May 2009 21:41:44 +0200
Subject: [PATCH] ARM: MXC: add driver for MXC's internal RTC module

This adds a driver for Freescale's MXC internal real time clock modules.

The code is taken from Freescale's BSPs, but modified to fit the current
kernel coding mechanisms. Also, the PMIC external clock function was
removed for now to not add dead bits and keep the code as simple as
possible.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Alessandro Zummo <a.zummo@towertech.it>
---
 drivers/rtc/Kconfig   |   11 +
 drivers/rtc/Makefile  |    1 +
 drivers/rtc/rtc-mxc.c |  611 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 623 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-mxc.c

Re: [PATCH 2/3] ARM: MXC: add driver for MXC's internal RTC module

Commit Message

Comments

Patch