PCAP RTC driver (for 2.6.32).

RTC driver for PCAP2 PMIC.

This patch was sent some time ago and acked by Alessandro, but we had to
change some stuff on the core driver and consequently on this driver
too, so I'm submitting it again for review (sorry).

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

---
 drivers/rtc/Kconfig    |    7 ++
 drivers/rtc/Makefile   |    1 +
 drivers/rtc/rtc-pcap.c |  222 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 0 deletions(-)

On Thu, 25 Jun 2009 17:45:49 -0300
Daniel Ribeiro <drwyrm@gmail.com> wrote:

> RTC driver for PCAP2 PMIC.
> 
> This patch was sent some time ago and acked by Alessandro, but we had to
> change some stuff on the core driver and consequently on this driver
> too, so I'm submitting it again for review (sorry).
> 
> Signed-off-by: guiming zhuo <gmzhuo@gmail.com>
> Signed-off-by: Daniel Ribeiro <drwyrm@gmail.com>

 Acked-by: Alessandro Zummo <a.zummo@towertech.it>

On Thu, 25 Jun 2009 17:45:49 -0300
Daniel Ribeiro <drwyrm@gmail.com> wrote:

> RTC driver for PCAP2 PMIC.
> 
> This patch was sent some time ago and acked by Alessandro, but we had to
> change some stuff on the core driver and consequently on this driver
> too, so I'm submitting it again for review (sorry).
> 
> Signed-off-by: guiming zhuo <gmzhuo@gmail.com>
> Signed-off-by: Daniel Ribeiro <drwyrm@gmail.com>

The above paragraph contains nothing which is relevant to the final
commit changelog, so I removed it.  I thus ended up without any
changelog at all.

Oh well - perhaps there wasn't much to say anyway.


> diff -puN drivers/rtc/Kconfig~pcap-rtc-driver-for-2632 drivers/rtc/Kconfig
> --- a/drivers/rtc/Kconfig~pcap-rtc-driver-for-2632
> +++ a/drivers/rtc/Kconfig
> @@ -778,4 +778,11 @@ config RTC_DRV_PS3
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-ps3.
>  
> +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 -puN drivers/rtc/Makefile~pcap-rtc-driver-for-2632 drivers/rtc/Makefile
> --- a/drivers/rtc/Makefile~pcap-rtc-driver-for-2632
> +++ a/drivers/rtc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max
>  obj-$(CONFIG_RTC_DRV_MSM6242)	+= rtc-msm6242.o
>  obj-$(CONFIG_RTC_DRV_MV)	+= rtc-mv.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 -puN /dev/null drivers/rtc/rtc-pcap.c
> --- /dev/null
> +++ a/drivers/rtc/rtc-pcap.c
> @@ -0,0 +1,222 @@
> +/*
> + *  pcap rtc code for Motorola EZX phones
> + *
> + *  Copyright (c) 2008 guiming zhuo <gmzhuo@gmail.com>
> + *  Copyright (c) 2009 Daniel Ribeiro <drwyrm@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/mfd/ezx-pcap.h>
> +#include <linux/rtc.h>
> +#include <linux/platform_device.h>
> +
> +struct pcap_rtc {
> +	struct pcap_chip *pcap;
> +	struct rtc_device *rtc;
> +};
> +
> +static irqreturn_t pcap_rtc_irq(int irq, void *_pcap_rtc)
> +{
> +	struct pcap_rtc *pcap_rtc = _pcap_rtc;
> +	unsigned long rtc_events = 0;
> +
> +	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> +		rtc_events |= RTC_IRQF | RTC_UF;
> +	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> +		rtc_events |= RTC_IRQF | RTC_AF;
> +
> +	rtc_update_irq(pcap_rtc->rtc, 1, rtc_events);
> +	return IRQ_HANDLED;
> +}

This could be coded more simply:

	unsigned long rtc_events;

	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
		rtc_events = RTC_IRQF | RTC_UF;
	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
		rtc_events = RTC_IRQF | RTC_AF;


> +static int pcap_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pcap_rtc *pcap_rtc = platform_get_drvdata(pdev);
> +	struct rtc_time *tm = &alrm->time;
> +	unsigned long secs;
> +	u32 tmp;
> +
> +	ezx_pcap_read(pcap_rtc->pcap, PCAP_REG_RTC_TODA, &tmp);
> +	secs = tmp & PCAP_RTC_TOD_MASK;
> +
> +	ezx_pcap_read(pcap_rtc->pcap, PCAP_REG_RTC_DAYA, &tmp);
> +	secs += (tmp & PCAP_RTC_DAY_MASK) * SEC_PER_DAY;
> +
> +	rtc_time_to_tm(secs, tm);
> +
> +	return 0;
> +}

SEC_PER_DAY is defined in include/linux/mfd/ezx-pcap.h.  It should not
be, because it is not specific to that driver and can be used elsewhere
in the kernel.

I'd suggest that we

- define SECS_PER_DAY in include/linux/time.h

- remove the private definitions of SECS_PER_DAY from
  arch/m68k/mac/misc, arch/parisc/include/asm/rtc.h,
  arch/ia64/hp/sim/boot/fw-emu.c, fs/udf/udftime.c, fs/fat/misc.c and
  wherever else it appears, make those files use the common definition

- migrate rtc-pcap.c from SEC_PER_DAY over to the common SECS_PER_DAY.

- Then do it all again for SECS_PER_MIN and SECS_PER_HOUR.


What a mess.


None of the above is applicable in the context of this patch.


> +static int pcap_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pcap_rtc *pcap_rtc = platform_get_drvdata(pdev);
> +	struct rtc_time *tm = &alrm->time;
> +	unsigned long secs;
> +	u32 tmp;

"tmp" is a poor identifier.  We had an opportunity here to use an
identifier which would communicate useful information to the reader. 
But we blew it and used the information-free "tmp" instead.

Something like

	u32 time_of_day;	/* In seconds since midnight */

would be nice.  If that is indeed what the variable contains.  How
would I know?  It's called 'tmp" and is undescribed!

> +	rtc_tm_to_time(tm, &secs);
> +
> +	tmp = secs % SEC_PER_DAY;
> +	ezx_pcap_write(pcap_rtc->pcap, PCAP_REG_RTC_TODA, tmp);
> +
> +	tmp = secs / SEC_PER_DAY;
> +	ezx_pcap_write(pcap_rtc->pcap, PCAP_REG_RTC_DAYA, tmp);
> +
> +	return 0;
> +}
> +
> +static int pcap_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pcap_rtc *pcap_rtc = platform_get_drvdata(pdev);
> +	unsigned long secs;
> +	u32 tmp;
> +
> +	ezx_pcap_read(pcap_rtc->pcap, PCAP_REG_RTC_TOD, &tmp);
> +	secs = tmp & PCAP_RTC_TOD_MASK;
> +
> +	ezx_pcap_read(pcap_rtc->pcap, PCAP_REG_RTC_DAY, &tmp);
> +	secs += (tmp & PCAP_RTC_DAY_MASK) * SEC_PER_DAY;
> +
> +	rtc_time_to_tm(secs, tm);
> +
> +	return rtc_valid_tm(tm);
> +}
> +
> +static int pcap_rtc_set_mmss(struct device *dev, unsigned long secs)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pcap_rtc *pcap_rtc = platform_get_drvdata(pdev);
> +	u32 tmp;
> +
> +	tmp = secs % SEC_PER_DAY;
> +	ezx_pcap_write(pcap_rtc->pcap, PCAP_REG_RTC_TOD, tmp);
> +
> +	tmp = secs / SEC_PER_DAY;
> +	ezx_pcap_write(pcap_rtc->pcap, PCAP_REG_RTC_DAY, tmp);
> +
> +	return 0;
> +}

etceteras

> +static inline int pcap_rtc_irq_enable(struct device *dev, int pirq,
> +							unsigned int en)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct pcap_rtc *pcap_rtc = platform_get_drvdata(pdev);
> +
> +	if (en)
> +		enable_irq(pcap_to_irq(pcap_rtc->pcap, pirq));
> +	else
> +		disable_irq(pcap_to_irq(pcap_rtc->pcap, pirq));
> +
> +	return 0;
> +}

Inlining this function probably made the code larger and slower.

> +static int pcap_rtc_alarm_irq_enable(struct device *dev, unsigned int en)
> +{
> +	return pcap_rtc_irq_enable(dev, PCAP_IRQ_TODA, en);
> +}
> +
> +static int pcap_rtc_update_irq_enable(struct device *dev, unsigned int en)
> +{
> +	return pcap_rtc_irq_enable(dev, PCAP_IRQ_1HZ, en);
> +}
> +
> +static const struct rtc_class_ops pcap_rtc_ops = {
> +	.read_time = pcap_rtc_read_time,
> +	.read_alarm = pcap_rtc_read_alarm,
> +	.set_alarm = pcap_rtc_set_alarm,
> +	.set_mmss = pcap_rtc_set_mmss,
> +	.alarm_irq_enable = pcap_rtc_alarm_irq_enable,
> +	.update_irq_enable = pcap_rtc_update_irq_enable,
> +};
> +
> +static int __devinit pcap_rtc_probe(struct platform_device *pdev)
> +{
> +	struct pcap_rtc *pcap_rtc;
> +	int timer_irq, alarm_irq;
> +	int err = -ENOMEM;
> +
> +	pcap_rtc = kmalloc(sizeof(struct pcap_rtc), GFP_KERNEL);
> +	if (!pcap_rtc)
> +		return err;
> +
> +	pcap_rtc->pcap = platform_get_drvdata(pdev);
> +
> +	pcap_rtc->rtc = rtc_device_register("pcap", &pdev->dev,
> +				  &pcap_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(pcap_rtc->rtc)) {
> +		err = PTR_ERR(pcap_rtc->rtc);
> +		goto fail_rtc;
> +	}
> +
> +	platform_set_drvdata(pdev, pcap_rtc);
> +
> +	timer_irq = pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ);
> +	alarm_irq = pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA);
> +
> +	err = request_irq(timer_irq, pcap_rtc_irq, 0, "RTC Timer", pcap_rtc);
> +	if (err)
> +		goto fail_timer;
> +
> +	err = request_irq(alarm_irq, pcap_rtc_irq, 0, "RTC Alarm", pcap_rtc);
> +	if (err)
> +		goto fail_alarm;
> +
> +	return 0;
> +fail_alarm:
> +	free_irq(timer_irq, pcap_rtc);
> +fail_timer:
> +	rtc_device_unregister(pcap_rtc->rtc);
> +fail_rtc:
> +	kfree(pcap_rtc);
> +	return err;
> +}
> +
> +static int __devexit pcap_rtc_remove(struct platform_device *pdev)
> +{
> +	struct pcap_rtc *pcap_rtc = platform_get_drvdata(pdev);
> +
> +	free_irq(pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ), pcap_rtc);
> +	free_irq(pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA), pcap_rtc);
> +	rtc_device_unregister(pcap_rtc->rtc);
> +	kfree(pcap_rtc);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver pcap_rtc_driver = {
> +	.remove = __devexit_p(pcap_rtc_remove),
> +	.driver = {
> +		.name  = "pcap-rtc",
> +		.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");>

It's a nice little driver. 

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---

On Fri, 2009-06-26 at 13:23 -0700, Andrew Morton wrote:
> > diff -puN /dev/null drivers/rtc/rtc-pcap.c
> > --- /dev/null
> > +++ a/drivers/rtc/rtc-pcap.c
[]
> > +static irqreturn_t pcap_rtc_irq(int irq, void *_pcap_rtc)
> > +{
> > +	struct pcap_rtc *pcap_rtc = _pcap_rtc;
> > +	unsigned long rtc_events = 0;
> > +
> > +	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> > +		rtc_events |= RTC_IRQF | RTC_UF;
> > +	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> > +		rtc_events |= RTC_IRQF | RTC_AF;
> > +
> > +	rtc_update_irq(pcap_rtc->rtc, 1, rtc_events);
> > +	return IRQ_HANDLED;
> > +}
> 
> This could be coded more simply:
> 
> 	unsigned long rtc_events;
> 
> 	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> 		rtc_events = RTC_IRQF | RTC_UF;
> 	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> 		rtc_events = RTC_IRQF | RTC_AF;

else
	rec_events = 0;

> > +     rtc_update_irq(pcap_rtc->rtc, 1, rtc_events);
> > +     return IRQ_HANDLED;

Does that buy any clarity?



--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---

Hi Andrew,

Em Sex, 2009-06-26 às 13:23 -0700, Andrew Morton escreveu:
> This could be coded more simply:
> 
> 	unsigned long rtc_events;
> 
> 	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> 		rtc_events = RTC_IRQF | RTC_UF;
> 	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> 		rtc_events = RTC_IRQF | RTC_AF;

This would make gcc complain about rtc_events being used uninitialized.

> SEC_PER_DAY is defined in include/linux/mfd/ezx-pcap.h.  It should not
> be, because it is not specific to that driver and can be used elsewhere
> in the kernel.
> 
> I'd suggest that we
> 
> - define SECS_PER_DAY in include/linux/time.h
> 
> - remove the private definitions of SECS_PER_DAY from
>   arch/m68k/mac/misc, arch/parisc/include/asm/rtc.h,
>   arch/ia64/hp/sim/boot/fw-emu.c, fs/udf/udftime.c, fs/fat/misc.c and
>   wherever else it appears, make those files use the common definition
> 
> - migrate rtc-pcap.c from SEC_PER_DAY over to the common SECS_PER_DAY.
> 
> - Then do it all again for SECS_PER_MIN and SECS_PER_HOUR.
> 
> 
> What a mess.

I can't work on this right now, but i will be happy to reserve some
hours for this next month if nobody else does it before.

> "tmp" is a poor identifier.  We had an opportunity here to use an
> identifier which would communicate useful information to the reader. 
> But we blew it and used the information-free "tmp" instead.
> 
> Something like
> 
> 	u32 time_of_day;	/* In seconds since midnight */
> 
> would be nice.  If that is indeed what the variable contains.  How
> would I know?  It's called 'tmp" and is undescribed!

Hum.. I used tmp because the variable is reused and it may be "days
since 1/1/1970" or "seconds since today's 00:00:00". I will add some
comments to make this clear.

> > +static inline int pcap_rtc_irq_enable(struct device *dev, int pirq,
> > +							unsigned int en)
> > +{
> Inlining this function probably made the code larger and slower.

Right, i will amend this.


I see that you have already added it to the -mm tree. Do you prefer an
incremental patch to address your comments or should I send a patch to
replace the current version?

Thanks for the review! :)

On Fri, 26 Jun 2009 20:57:38 -0300 Daniel Ribeiro <drwyrm@gmail.com> wrote:

> Hi Andrew,
> 
> Em Sex, 2009-06-26 __s 13:23 -0700, Andrew Morton escreveu:
> > This could be coded more simply:
> > 
> > 	unsigned long rtc_events;
> > 
> > 	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> > 		rtc_events = RTC_IRQF | RTC_UF;
> > 	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> > 		rtc_events = RTC_IRQF | RTC_AF;
> 
> This would make gcc complain about rtc_events being used uninitialized.

So initialise it ;)

It's the "|=" which is strange and a bit misleading - it's a coding
trick which doesn't reflect what is happening at a functional level.

But it's a minor point.

> > SEC_PER_DAY is defined in include/linux/mfd/ezx-pcap.h.  It should not
> > be, because it is not specific to that driver and can be used elsewhere
> > in the kernel.
> > 
> > I'd suggest that we
> > 
> > - define SECS_PER_DAY in include/linux/time.h
> > 
> > - remove the private definitions of SECS_PER_DAY from
> >   arch/m68k/mac/misc, arch/parisc/include/asm/rtc.h,
> >   arch/ia64/hp/sim/boot/fw-emu.c, fs/udf/udftime.c, fs/fat/misc.c and
> >   wherever else it appears, make those files use the common definition
> > 
> > - migrate rtc-pcap.c from SEC_PER_DAY over to the common SECS_PER_DAY.
> > 
> > - Then do it all again for SECS_PER_MIN and SECS_PER_HOUR.
> > 
> > 
> > What a mess.
> 
> I can't work on this right now, but i will be happy to reserve some
> hours for this next month if nobody else does it before.

Well, whatever.  Lots of people could pick up this projectlet.

> > "tmp" is a poor identifier.  We had an opportunity here to use an
> > identifier which would communicate useful information to the reader. 
> > But we blew it and used the information-free "tmp" instead.
> > 
> > Something like
> > 
> > 	u32 time_of_day;	/* In seconds since midnight */
> > 
> > would be nice.  If that is indeed what the variable contains.  How
> > would I know?  It's called 'tmp" and is undescribed!
> 
> Hum.. I used tmp because the variable is reused and it may be "days
> since 1/1/1970" or "seconds since today's 00:00:00".

That's even worse!

> I will add some comments to make this clear.

It'd be clearest to create two appropriately-named variables.  This
shouldn't affect emitted code unless gcc is busted.

> I see that you have already added it to the -mm tree.

Yes, I often do that.  To get a bit of early testing and so the whole
thing doesn't get lost.  Also so that I remember that I've reviewed it
once.

> Do you prefer an
> incremental patch to address your comments or should I send a patch to
> replace the current version?

A replacement is OK at this early stage.  I will turn that into an
incremental here so that I can see what you did.


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---