Re: [PATCH] PCAP RTC driver (for 2.6.32).

[Andrew Morton]

On Thu, 25 Jun 2009 17:45:49 -0300
Daniel Ribeiro <drwyrm@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Daniel Ribeiro <drwyrm@xxxxxxxxx>

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@xxxxxxxxx>
> + *  Copyright (c) 2009 Daniel Ribeiro <drwyrm@xxxxxxxxx>
> + *
> + *  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@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");>

It's a nice little driver. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/