Re: [patch 12/14] PCAP2 RTC driver

Hello.

On Fri, 2008-11-21 at 17:14, Alessandro Zummo wrote:
> On Fri, 21 Nov 2008 17:04:15 +0100
> stefan@datenfreihafen.org wrote:
> 
> > 
> > RTC driver based on the PCAP2 multi function device
> > 
> > Signed-off-by: Daniel Ribeiro <drwyrm@gmail.com>
> > Signed-off-by: guiming zhuo <gmzhuo@gmail.com>
> 
>  nack, please see below.
>  you might want to check http://groups.google.com/group/rtc-linux/web/checklist
>  for suggestions.

Thanks for your fast review. Updated patch inline below. I hope we addressed all
your concerns let us know if there is more.

> > +config RTC_DRV_PCAP
> > +	tristate "PCAP RTC"
> > +	depends on EZX_PCAP
> > +	help
> > +	  If you say Y here you will get support for the RTC found on
> > +	   pcap.
> 
>  Please be more descriptive.

Done
 
> > +static struct rtc_device *rtc;
> 
>  please avoid globals

Done. Passing it on now.

> > +
> > +	rtc_time_to_tm(tmv.tv_sec, tm);
> > +
> > +	return 0;
> > +}
> 
>  return rtc_valid_tm(tm);

Done

> > +static int pcap_rtc_set_mmss(struct device *dev, unsigned long secs)
> > +{
> > +	return 0;
> > +}
> 
>  you should be implementing only set_mmss, not set_time with
>  this kind of rtc

OK, using set_mmss instead of set_time now

> > +static int pcap_rtc_proc(struct device *dev, struct seq_file *seq)
> > +{
> > +	struct platform_device *plat_dev = to_platform_device(dev);
> > +
> > +	seq_printf(seq, "pcap\t\t: yes\n");
> > +	seq_printf(seq, "id\t\t: %d\n", plat_dev->id);
> > +
> > +	return 0;
> > +}
> 
>  is this really necessary? /proc will go away sooner
>  or later

No, dropped.
 
> > +static int pcap_rtc_ioctl(struct device *dev, unsigned int cmd,
> > +			  unsigned long arg)
> > +{
> > +	switch (cmd) {
> > +	case RTC_PIE_ON:
> > +	case RTC_PIE_OFF:
> > +	case RTC_UIE_ON:
> > +	case RTC_UIE_OFF:
> > +	case RTC_AIE_ON:
> > +	case RTC_AIE_OFF:
> > +		return 0;
> > +
> > +	default:
> > +		return -ENOIOCTLCMD;
> > +	}
> > +
> > +}
> 
>  this function makes no sense the way it is. you shold 
>  implement the proper irq hooks

Done

> > +error:
> > +	rtc_device_unregister(rtc);
> 
>  wrong! rtc won't be valid here

Removed

> > +static int __init rtc_pcap_init(void)
> > +{
> > +	return platform_driver_register(&pcap_rtc_driver);
> > +}
> 
>  you might want to use platform_driver_probe

We do now.

Also using __init and __exit now.



From: Daniel Ribeiro <drwyrm@gmail.com>
Subject: PCAP2 RTC driver
To: a.zummo@towertech.it
Cc: eric.y.miao@gmail.com, rtc-linux@googlegroups.com, linux-arm-kernel@lists.arm.linux.org.uk

RTC driver based on the PCAP2 multi function device

Signed-off-by: Daniel Ribeiro <drwyrm@gmail.com>
Signed-off-by: guiming zhuo <gmzhuo@gmail.com>

---
 drivers/rtc/Kconfig    |    7 ++
 drivers/rtc/Makefile   |    2 +
 drivers/rtc/rtc-pcap.c |  212 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+), 0 deletions(-)

On Sat, 22 Nov 2008 00:49:53 +0100
Stefan Schmidt <stefan@datenfreihafen.org> wrote:

> >  nack, please see below.
> >  you might want to check http://groups.google.com/group/rtc-linux/web/checklist
> >  for suggestions.
> 
> Thanks for your fast review. Updated patch inline below. I hope we addressed all
> your concerns let us know if there is more.

 Hi Stefan,

  it's almost ok. Still a few comments below.

 Do you want me to carry this via -mm or will be going in via arm?
 Anyway, we'll need your Signed-off-by
 
 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 123092d..3fc15b5 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -679,4 +679,11 @@ config RTC_DRV_STARFIRE
>  	  If you say Y here you will get support for the RTC found on
>  	  Starfire systems.
>  
> +config RTC_DRV_PCAP
> +	tristate "PCAP RTC"
> +	depends on EZX_PCAP
> +	help
> +	  If you say Y here you will get support for the RTC found on
> +	  the PCAP2 ASIC used on some Motorola phones.
> +
>  endif # RTC_CLASS
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 6e79c91..be2f294 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -17,6 +17,7 @@ rtc-core-$(CONFIG_RTC_INTF_SYSFS) += rtc-sysfs.o
>  
>  # Keep the list ordered.
>  
> +
>  obj-$(CONFIG_RTC_DRV_AT32AP700X)+= rtc-at32ap700x.o
>  obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
>  obj-$(CONFIG_RTC_DRV_AT91SAM9)	+= rtc-at91sam9.o
> @@ -48,6 +49,7 @@ obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
>  obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
>  obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
>  obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
> +obj-$(CONFIG_RTC_DRV_PCAP)	+= rtc-pcap.o
>  obj-$(CONFIG_RTC_DRV_PCF8563)	+= rtc-pcf8563.o
>  obj-$(CONFIG_RTC_DRV_PCF8583)	+= rtc-pcf8583.o
>  obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
> diff --git a/drivers/rtc/rtc-pcap.c b/drivers/rtc/rtc-pcap.c
> new file mode 100644
> index 0000000..1b02eae
> --- /dev/null
> +++ b/drivers/rtc/rtc-pcap.c
> @@ -0,0 +1,212 @@
> +/*
> + *  pcap rtc code for Motorola EZX phones
> + *
> + *  Copyright (c) 2008 guiming zhuo <gmzhuo@gmail.com>
> + *
> + *  Based on Motorola's rtc.c Copyright (c) 2003-2005 Motorola
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +#include <linux/sched.h>
> +#include <linux/errno.h>
> +#include <linux/version.h>
> +#include <linux/mfd/ezx-pcap.h>
> +#include <linux/rtc.h>
> +#include <linux/platform_device.h>

 are you sure you need all of those
 (string sched version ? )

> +static void pcap_rtc_irq(u32 events, void *data)
> +{
> +	unsigned long rtc_events = 0;
> +	struct rtc_device *rtc = data;
> +
> +	if (events & PCAP_IRQ_1HZ)
> +		rtc_events |= RTC_IRQF | RTC_UF;
> +	if (events & PCAP_IRQ_TODA)
> +		rtc_events |= RTC_IRQF | RTC_AF;
> +
> +	rtc_update_irq(rtc, 1, rtc_events);
> +	return;
> +}
> +
> +static int pcap_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_time *tm = &alrm->time;
> +	struct timeval tmv;
> +	u32 value;
> +
> +	ezx_pcap_read(PCAP_REG_RTC_TODA, &value);
> +	value &= PCAP_RTC_TOD_MASK;
> +	tmv.tv_sec = value;
> +
> +	ezx_pcap_read(PCAP_REG_RTC_DAYA, &value);
> +	value &= PCAP_RTC_DAY_MASK;
> +	tmv.tv_sec += value * SEC_PER_DAY;
> +
> +	rtc_time_to_tm(tmv.tv_sec, tm);
> +
> +	return 0;
> +}


 why a struct timeval? also in the following routines:

> +
> +static int pcap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_time *tm = &alrm->time;
> +	unsigned long secs;
> +	int err;
> +	struct timeval tmv;

 ??

> +	u32 value;
> +
> +	err = rtc_tm_to_time(tm, &secs);
> +	tmv.tv_sec = secs;
> +	tmv.tv_usec = 0;
> +
> +	ezx_pcap_read(PCAP_REG_RTC_TODA, &value);
> +	value &= ~PCAP_RTC_TOD_MASK;
> +	value |= tmv.tv_sec % SEC_PER_DAY;
> +	ezx_pcap_write(PCAP_REG_RTC_TODA, value);
> +
> +	ezx_pcap_read(PCAP_REG_RTC_DAYA, &value);
> +	value &= ~PCAP_RTC_DAY_MASK;
> +	value |= tmv.tv_sec / SEC_PER_DAY;
> +	ezx_pcap_write(PCAP_REG_RTC_DAYA, value);
> +
> +	return 0;
> +}
> +
> +static int pcap_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct timeval tmv;
> +	u32 value;
> +

 ??

> +	ezx_pcap_read(PCAP_REG_RTC_TOD, &value);
> +	value &= PCAP_RTC_TOD_MASK;
> +	tmv.tv_sec = value;
> +
> +	ezx_pcap_read(PCAP_REG_RTC_DAY, &value);
> +	value &= PCAP_RTC_DAY_MASK;
> +	tmv.tv_sec += value * SEC_PER_DAY;
> +
> +	rtc_time_to_tm(tmv.tv_sec, tm);
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int pcap_rtc_set_mmss(struct device *dev, unsigned long secs)
> +{
> +	u32 value;
> +	
> +	ezx_pcap_read(PCAP_REG_RTC_TOD, &value);
> +	value &= ~PCAP_RTC_TOD_MASK;
> +	value |= secs % SEC_PER_DAY;
> +	ezx_pcap_write(PCAP_REG_RTC_TOD, value);
> +
> +	ezx_pcap_read(PCAP_REG_RTC_DAY, &value);
> +	value &= ~PCAP_RTC_DAY_MASK;
> +	value |= secs / SEC_PER_DAY;
> +	ezx_pcap_write(PCAP_REG_RTC_DAY, value);
> +
> +	return 0;
> +}
> +
> +static int pcap_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned long secs;
> +
> +	rtc_tm_to_time(tm, &secs);
> +	return pcap_rtc_set_mmss(dev, secs);
> +}

 you can omit set_time if you implement set_mmss. the latest rtc subsystem
 will handle it. see http://patchwork.ozlabs.org/patch/9977/


> +static int pcap_rtc_ioctl(struct device *dev, unsigned int cmd,
> +			  unsigned long arg)
> +{
> +	switch (cmd) {
> +	case RTC_UIE_ON:
> +		ezx_pcap_unmask_event(PCAP_IRQ_1HZ);
> +		break;
> +	case RTC_UIE_OFF:
> +		ezx_pcap_mask_event(PCAP_IRQ_1HZ);
> +		break;
> +	case RTC_AIE_ON:
> +		ezx_pcap_unmask_event(PCAP_IRQ_TODA);
> +		break;
> +	case RTC_AIE_OFF:
> +		ezx_pcap_mask_event(PCAP_IRQ_TODA);
> +		break;
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +	return 0;
> +}

 irq enabling should be implemented via
 alarm_irq_enable and update_irq_enable . you can then omit
 ioctl, the rtc subsystem will handle it.

 see http://patchwork.ozlabs.org/patch/10039/ for details

> +
> +static const struct rtc_class_ops pcap_rtc_ops = {
> +	.read_time = pcap_rtc_read_time,
> +	.set_time = pcap_rtc_set_time,
> +	.read_alarm = pcap_rtc_read_alarm,
> +	.set_alarm = pcap_rtc_set_alarm,
> +	.set_mmss = pcap_rtc_set_mmss,
> +	.ioctl = pcap_rtc_ioctl,
> +};
> +
> +static int __init pcap_rtc_probe(struct platform_device *plat_dev)
> +{
> +	struct rtc_device *rtc;
> +	int err;
> +
> +	rtc = rtc_device_register("pcap", &plat_dev->dev,
> +				  &pcap_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc)) {
> +		err = PTR_ERR(rtc);
> +		goto error;
> +	}

 just return PTR_ERR(rtc) here.


> +
> +	platform_set_drvdata(plat_dev, rtc);
> +
> +	ezx_pcap_register_event(PCAP_IRQ_1HZ, pcap_rtc_irq, rtc, "RTC Timer");
> +	ezx_pcap_register_event(PCAP_IRQ_TODA, pcap_rtc_irq, rtc, "RTC Alarm");
> +
> +	return 0;
> +
> +error:
> +	return err;
> +}
> +
> +static int __exit pcap_rtc_remove(struct platform_device *plat_dev)
> +{
> +	struct rtc_device *rtc = platform_get_drvdata(plat_dev);
> +
> +	ezx_pcap_unregister_event(PCAP_IRQ_1HZ | PCAP_IRQ_TODA);
> +	rtc_device_unregister(rtc);
> +	return 0;
> +}
> +
> +static struct platform_driver pcap_rtc_driver = {
> +	.remove = __exit_p(pcap_rtc_remove),
> +	.driver = {
> +		.name  = "rtc-pcap",
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init rtc_pcap_init(void)
> +{
> +	return platform_driver_probe(&pcap_rtc_driver, pcap_rtc_probe);
> +}
> +
> +static void __exit rtc_pcap_exit(void)
> +{
> +	platform_driver_unregister(&pcap_rtc_driver);
> +}
> +
> +module_init(rtc_pcap_init);
> +module_exit(rtc_pcap_exit);
> +
> +MODULE_DESCRIPTION("Motorola pcap rtc driver");
> +MODULE_AUTHOR("guiming zhuo <gmzhuo@gmail.com>");
> +MODULE_LICENSE("GPL");
> -- 
> tg: (851b4d2..) ezx/pcap_rtc (depends on: ezx/local/pcap)
> 
> 
> >