Date:	Thu, 18 Dec 2008 10:03:10 +0100
From:	Alessandro Zummo <alessandro.zummo@...ertech.it>
To:	Balaji Rao <balajirrao@...nmoko.org>
Cc:	linux-kernel@...r.kernel.org, Balaji Rao <balajirrao@...nmoko.org>,
	Andy Green <andy@...nmoko.com>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	rtc-linux@...glegroups.com
Subject: Re: [PATCH V2 4/7] rtc: PCF50633 rtc driver

On Thu, 18 Dec 2008 11:27:26 +0530
Balaji Rao <balajirrao@...nmoko.org> wrote:

> Changes from V1:
> 	- Removed include/linux/mfd/pcf50633/rtc.h and moved defenitions into
> 	  the source file.
> 	- Remove PIE and introduce UIE support. UIE being the one actually
> 	  supported in the chip.
> 
> Alessandro, I'll change to the new API for AIE once it appears upstream.

 Ok, I guess the driver will not go in tomorrow, so you can wait.
 I'd add it now, while you're at it :)

 some comments below:


> Signed-off-by: Balaji Rao <balajirrao@...nmoko.org>
> Cc: Andy Green <andy@...nmoko.com>
> Cc: Alessandro Zummo <a.zummo@...ertech.it>
> Cc: Paul Gortmaker <a.zummo@...ertech.it>
> Cc: rtc-linux@...glegroups.com
> ---
>  drivers/rtc/Kconfig        |    6 +
>  drivers/rtc/Makefile       |    1 
>  drivers/rtc/rtc-pcf50633.c |  336 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 343 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/rtc/rtc-pcf50633.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 123092d..68e68d2 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -497,6 +497,12 @@ config RTC_DRV_WM8350
>  	  This driver can also be built as a module. If so, the module
>  	  will be called "rtc-wm8350".
>  
> +config RTC_DRV_PCF50633
> +	depends on MFD_PCF50633
> +	tristate "NXP PCF50633 RTC"
> +	help
> +	  If you say yes here you get support for the NXP PCF50633 RTC.

 a more detailed description please stating which platforms commonly
 have this rtc

>  comment "on-CPU RTC drivers"
>  
>  config RTC_DRV_OMAP
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 6e79c91..a717fec 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -70,3 +70,4 @@ obj-$(CONFIG_RTC_DRV_V3020)	+= rtc-v3020.o
>  obj-$(CONFIG_RTC_DRV_VR41XX)	+= rtc-vr41xx.o
>  obj-$(CONFIG_RTC_DRV_WM8350)	+= rtc-wm8350.o
>  obj-$(CONFIG_RTC_DRV_X1205)	+= rtc-x1205.o
> +obj-$(CONFIG_RTC_DRV_PCF50633)	+= rtc-pcf50633.o
> diff --git a/drivers/rtc/rtc-pcf50633.c b/drivers/rtc/rtc-pcf50633.c
> new file mode 100644
> index 0000000..2f33509
> --- /dev/null
> +++ b/drivers/rtc/rtc-pcf50633.c
> @@ -0,0 +1,336 @@
> +/* NXP PCF50633 RTC Driver
> + *
> + * (C) 2006-2008 by Openmoko, Inc.
> + * Author: Balaji Rao <balajirrao@...nmoko.org>
> + * All rights reserved.
> + *
> + * Broken down from monstrous PCF50633 driver mainly by
> + * Harald Welte, Andy Green and Werner Almesberger
> + *
> + *  This program is free software; you can redistribute  it and/or modify it
> + *  under  the terms of  the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the  License, or (at your
> + *  option) any later version.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +#include <linux/bcd.h>
> +#include <linux/err.h>
> +
> +#include <linux/mfd/pcf50633/core.h>
> +
> +#define PCF50633_REG_RTCSC	0x59 /* Second */
> +#define PCF50633_REG_RTCMN	0x5a /* Minute */
> +#define PCF50633_REG_RTCHR	0x5b /* Hour */
> +#define PCF50633_REG_RTCWD	0x5c /* Weekday */
> +#define PCF50633_REG_RTCDT	0x5d /* Day */
> +#define PCF50633_REG_RTCMT	0x5e /* Month */
> +#define PCF50633_REG_RTCYR	0x5f /* Year */
> +#define PCF50633_REG_RTCSCA	0x60 /* Alarm Second */
> +#define PCF50633_REG_RTCMNA	0x61 /* Alarm Minute */
> +#define PCF50633_REG_RTCHRA	0x62 /* Alarm Hour */
> +#define PCF50633_REG_RTCWDA	0x63 /* Alarm Weekday */
> +#define PCF50633_REG_RTCDTA	0x64 /* Alarm Day */
> +#define PCF50633_REG_RTCMTA	0x65 /* Alarm Month */
> +#define PCF50633_REG_RTCYRA	0x66 /* Alarm Year */
> +
> +enum pcf50633_time_indexes {
> +	PCF50633_TI_SEC,
> +	PCF50633_TI_MIN,
> +	PCF50633_TI_HOUR,
> +	PCF50633_TI_WKDAY,
> +	PCF50633_TI_DAY,
> +	PCF50633_TI_MONTH,
> +	PCF50633_TI_YEAR,
> +	PCF50633_TI_EXTENT /* always last */
> +};
> +
> +struct pcf50633_time {
> +	u_int8_t time[PCF50633_TI_EXTENT];
> +};
> +
> +struct pcf50633_rtc {
> +	int alarm_enabled;
> +	int second_enabled;
> +
> +	struct pcf50633 *pcf;
> +	struct rtc_device *rtc_dev;
> +};
> +
> +static void pcf2rtc_time(struct rtc_time *rtc, struct pcf50633_time *pcf)
> +{
> +	rtc->tm_sec = bcd2bin(pcf->time[PCF50633_TI_SEC]);
> +	rtc->tm_min = bcd2bin(pcf->time[PCF50633_TI_MIN]);
> +	rtc->tm_hour = bcd2bin(pcf->time[PCF50633_TI_HOUR]);
> +	rtc->tm_wday = bcd2bin(pcf->time[PCF50633_TI_WKDAY]);
> +	rtc->tm_mday = bcd2bin(pcf->time[PCF50633_TI_DAY]);
> +	rtc->tm_mon = bcd2bin(pcf->time[PCF50633_TI_MONTH]);
> +	rtc->tm_year = bcd2bin(pcf->time[PCF50633_TI_YEAR]) + 100;
> +}
> +
> +static void rtc2pcf_time(struct pcf50633_time *pcf, struct rtc_time *rtc)
> +{
> +	pcf->time[PCF50633_TI_SEC] = bin2bcd(rtc->tm_sec);
> +	pcf->time[PCF50633_TI_MIN] = bin2bcd(rtc->tm_min);
> +	pcf->time[PCF50633_TI_HOUR] = bin2bcd(rtc->tm_hour);
> +	pcf->time[PCF50633_TI_WKDAY] = bin2bcd(rtc->tm_wday);
> +	pcf->time[PCF50633_TI_DAY] = bin2bcd(rtc->tm_mday);
> +	pcf->time[PCF50633_TI_MONTH] = bin2bcd(rtc->tm_mon);
> +	pcf->time[PCF50633_TI_YEAR] = bin2bcd(rtc->tm_year % 100);
> +}
> +
> +static int
> +pcf50633_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> +	struct pcf50633_rtc *rtc = dev_get_drvdata(dev);
> +
> +	switch (cmd) {
> +	case RTC_AIE_OFF:
> +		rtc->alarm_enabled = 0;
> +		pcf50633_irq_mask(rtc->pcf, PCF50633_IRQ_ALARM);
> +		return 0;
> +	case RTC_AIE_ON:
> +		rtc->alarm_enabled = 1;
> +		pcf50633_irq_unmask(rtc->pcf, PCF50633_IRQ_ALARM);
> +		return 0;
> +	case RTC_UIE_OFF:
> +		rtc->second_enabled = 0;
> +		pcf50633_irq_mask(rtc->pcf, PCF50633_IRQ_SECOND);
> +		return 0;
> +	case RTC_UIE_ON:
> +		rtc->second_enabled = 1;
> +		pcf50633_irq_unmask(rtc->pcf, PCF50633_IRQ_SECOND);
> +		return 0;
> +	}
> +
> +	return -ENOIOCTLCMD;
> +}
> +
> +static int pcf50633_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct pcf50633_rtc *rtc;
> +	struct pcf50633_time pcf_tm;
> +	int ret;
> +
> +	rtc = dev_get_drvdata(dev);
> +
> +	ret = pcf50633_read_block(rtc->pcf, PCF50633_REG_RTCSC,
> +					    PCF50633_TI_EXTENT,
> +					    &pcf_tm.time[0]);
> +	if (ret != PCF50633_TI_EXTENT) {
> +		dev_err(dev, "Failed to read time\n");
> +		return -EIO;
> +	}
> +
> +	dev_dbg(dev, "PCF_TIME: %02x.%02x.%02x %02x:%02x:%02x\n",
> +		pcf_tm.time[PCF50633_TI_DAY],
> +		pcf_tm.time[PCF50633_TI_MONTH],
> +		pcf_tm.time[PCF50633_TI_YEAR],
> +		pcf_tm.time[PCF50633_TI_HOUR],
> +		pcf_tm.time[PCF50633_TI_MIN],
> +		pcf_tm.time[PCF50633_TI_SEC]);
> +
> +	pcf2rtc_time(tm, &pcf_tm);
> +
> +	dev_dbg(dev, "RTC_TIME: %u.%u.%u %u:%u:%u\n",
> +		tm->tm_mday, tm->tm_mon, tm->tm_year,
> +		tm->tm_hour, tm->tm_min, tm->tm_sec);
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int pcf50633_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct pcf50633_rtc *rtc;
> +	struct pcf50633_time pcf_tm;
> +	int second_masked, alarm_masked, ret = 0;
> +
> +	rtc = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "RTC_TIME: %u.%u.%u %u:%u:%u\n",
> +		tm->tm_mday, tm->tm_mon, tm->tm_year,
> +		tm->tm_hour, tm->tm_min, tm->tm_sec);
> +
> +	rtc2pcf_time(&pcf_tm, tm);
> +
> +	dev_dbg(dev, "PCF_TIME: %02x.%02x.%02x %02x:%02x:%02x\n",
> +		pcf_tm.time[PCF50633_TI_DAY],
> +		pcf_tm.time[PCF50633_TI_MONTH],
> +		pcf_tm.time[PCF50633_TI_YEAR],
> +		pcf_tm.time[PCF50633_TI_HOUR],
> +		pcf_tm.time[PCF50633_TI_MIN],
> +		pcf_tm.time[PCF50633_TI_SEC]);
> +
> +
> +	second_masked = pcf50633_irq_mask_get(rtc->pcf, PCF50633_IRQ_SECOND);
> +	alarm_masked = pcf50633_irq_mask_get(rtc->pcf, PCF50633_IRQ_ALARM);
> +
> +	if (!second_masked)
> +		pcf50633_irq_mask(rtc->pcf, PCF50633_IRQ_SECOND);
> +	if (!alarm_masked)
> +		pcf50633_irq_mask(rtc->pcf, PCF50633_IRQ_ALARM);
> +
> +	/* Returns 0 on success */
> +	ret = pcf50633_write_block(rtc->pcf, PCF50633_REG_RTCSC,
> +					     PCF50633_TI_EXTENT,
> +					     &pcf_tm.time[0]);
> +
> +	if (!second_masked)
> +		pcf50633_irq_unmask(rtc->pcf, PCF50633_IRQ_SECOND);
> +	if (!alarm_masked)
> +		pcf50633_irq_unmask(rtc->pcf, PCF50633_IRQ_ALARM);
> +
> +	return ret;
> +}
> +
> +static int pcf50633_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct pcf50633_rtc *rtc;
> +	struct pcf50633_time pcf_tm;
> +	int ret = 0;
> +
> +	rtc = dev_get_drvdata(dev);
> +
> +	alrm->enabled = rtc->alarm_enabled;
> +
> +	ret = pcf50633_read_block(rtc->pcf, PCF50633_REG_RTCSCA,
> +				PCF50633_TI_EXTENT, &pcf_tm.time[0]);
> +	if (ret != PCF50633_TI_EXTENT) {
> +		dev_err(dev, "Failed to read time\n");
> +		return -EIO;
> +	}
> +
> +	pcf2rtc_time(&alrm->time, &pcf_tm);
> +
> +	return rtc_valid_tm(&alrm->time);
> +}
> +
> +static int pcf50633_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct pcf50633_rtc *rtc;
> +	struct pcf50633_time pcf_tm;
> +	int alarm_masked, ret = 0;
> +
> +	rtc = dev_get_drvdata(dev);
> +
> +	rtc2pcf_time(&pcf_tm, &alrm->time);
> +
> +	/* do like mktime does and ignore tm_wday */
> +	pcf_tm.time[PCF50633_TI_WKDAY] = 7;
> +
> +	alarm_masked = pcf50633_irq_mask_get(rtc->pcf, PCF50633_IRQ_ALARM);
> +
> +	/* disable alarm interrupt */
> +	if (!alarm_masked)
> +		pcf50633_irq_mask(rtc->pcf, PCF50633_IRQ_ALARM);
> +
> +	/* Returns 0 on success */
> +	ret = pcf50633_write_block(rtc->pcf, PCF50633_REG_RTCSCA,
> +				PCF50633_TI_EXTENT, &pcf_tm.time[0]);
> +
> +	if (!alarm_masked)
> +		pcf50633_irq_unmask(rtc->pcf, PCF50633_IRQ_ALARM);
> +
> +	return ret;
> +}
> +
> +static struct rtc_class_ops pcf50633_rtc_ops = {
> +	.ioctl		= pcf50633_rtc_ioctl,
> +	.read_time	= pcf50633_rtc_read_time,
> +	.set_time	= pcf50633_rtc_set_time,
> +	.read_alarm	= pcf50633_rtc_read_alarm,
> +	.set_alarm	= pcf50633_rtc_set_alarm,
> +};
> +
> +static void pcf50633_rtc_irq(int irq, void *data)
> +{
> +	struct pcf50633_rtc *rtc = data;
> +
> +	switch (irq) {
> +	case PCF50633_IRQ_ALARM:
> +		rtc_update_irq(rtc->rtc_dev, 1, RTC_AF | RTC_IRQF);
> +		break;
> +	case PCF50633_IRQ_SECOND:
> +		rtc_update_irq(rtc->rtc_dev, 1, RTC_UF | RTC_IRQF);
> +		break;
> +	}
> +}
> +
> +static int __devinit pcf50633_rtc_probe(struct platform_device *pdev)
> +{
> +	struct pcf50633_subdev_pdata *pdata;
> +	struct pcf50633_rtc *rtc;
> +	struct rtc_device *rtc_dev;
> +
> +	rtc_dev = rtc_device_register("pcf50633-rtc", &pdev->dev,
> +				&pcf50633_rtc_ops, THIS_MODULE);
> +
> +	if (IS_ERR(rtc_dev))
> +		return PTR_ERR(rtc_dev);
> +
> +	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> +	if (!rtc) {
> +		dev_err(&pdev->dev, "allocation of pcf50633_rtc failed\n");
> +		rtc_device_unregister(rtc_dev);
> +		return -ENOMEM;
> +	}

 once registered, the rtc could be immediately in use. you should first allocate
 and setup your data structures and only then register the device:

	rtc = kzalloc(....
	if (rtc == NULL)
		return -ENOMEM;

	..setup rtc structs here...

	rtc->dev = rtc_register(...

	..irqs...

> +	pdata = pdev->dev.platform_data;
> +	rtc->pcf = pdata->pcf;
> +	rtc->rtc_dev = rtc_dev;
> +	platform_set_drvdata(pdev, rtc);
> +
> +	pcf50633_register_irq(rtc->pcf, PCF50633_IRQ_ALARM,
> +					pcf50633_rtc_irq, rtc);
> +	pcf50633_register_irq(rtc->pcf, PCF50633_IRQ_SECOND,
> +					pcf50633_rtc_irq, rtc);
> +
> +	return 0;
> +}
> +
> +static int __devexit pcf50633_rtc_remove(struct platform_device *pdev)
> +{
> +	struct pcf50633_rtc *rtc;
> +
> +	rtc = platform_get_drvdata(pdev);
> +	rtc_device_unregister(rtc->rtc_dev);
> +
> +	pcf50633_free_irq(rtc->pcf, PCF50633_IRQ_ALARM);
> +	pcf50633_free_irq(rtc->pcf, PCF50633_IRQ_SECOND);

 please remove rtc before unregistering

> +	kfree(rtc);
> +
> +	return 0;
> +}
> +
> +
> +static struct platform_driver pcf50633_rtc_driver = {
> +	.driver = {
> +		.name = "pcf50633-rtc",
> +	},
> +	.probe = pcf50633_rtc_probe,
> +	.remove = __devexit_p(pcf50633_rtc_remove),
> +};
> +
> +static int __init pcf50633_rtc_init(void)
> +{
> +	return platform_driver_register(&pcf50633_rtc_driver);
> +}
> +module_init(pcf50633_rtc_init);
> +
> +static void __exit pcf50633_rtc_exit(void)
> +{
> +	platform_driver_unregister(&pcf50633_rtc_driver);
> +}
> +module_exit(pcf50633_rtc_exit);
> +
> +MODULE_DESCRIPTION("PCF50633 RTC driver");
> +MODULE_AUTHOR("Balaji Rao <balajirrao@...nmoko.org>");
> +MODULE_LICENSE("GPL");
> +
> 


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

--
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