Re: [PATCH v2 15/26] RTC: Add JZ4740 RTC driver

[Wan ZongShun]

2010/6/19 Lars-Peter Clausen <lars@xxxxxxxxxx>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Wan ZongShun wrote:
>> Hi Lars-Peter,
>>
>>
>>> Hi
>>>
>>> Marek Vasut wrote:
>>>> Dne So 19. Äervna 2010 07:08:20 Lars-Peter Clausen napsal(a):
>>>>
>>>>> This patch adds support for the RTC unit on JZ4740 SoCs.
>>>>>
>>>>> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
>>>>> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
>>>>> Cc: Paul Gortmaker <p_gortmaker@xxxxxxxxx>
>>>>> Cc: Wan ZongShun <mcuos.com@xxxxxxxxx>
>>>>> Cc: Marek Vasut <marek.vasut@xxxxxxxxx>
>>>>> Cc: rtc-linux@xxxxxxxxxxxxxxxx
>>>>>
>>>>> ---
>>>>> Changes since v1
>>>>> - Use dev_get_drvdata directly instead of wrapping it in dev_to_rtc
>>>>> - Add common implementation for jz4740_rtc_{alarm,update}_irq_enable
>>>>> - Check whether rtc structure could be allocated
>>>>> - Fix deadlocks which could occur if the HW was broken
>>>>> ---
>>>>> Âdrivers/rtc/Kconfig   Â|  11 ++
>>>>> Âdrivers/rtc/Makefile   |  Â1 +
>>>>> Âdrivers/rtc/rtc-jz4740.c | Â341
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 353
>>>>> insertions(+), 0 deletions(-)
>>>>> Âcreate mode 100644 drivers/rtc/rtc-jz4740.c
>>>>>
>>>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>>>>> index 10ba12c..d0ed7e6 100644
>>>>> --- a/drivers/rtc/Kconfig
>>>>> +++ b/drivers/rtc/Kconfig
>>>>> @@ -905,4 +905,15 @@ config RTC_DRV_MPC5121
>>>>> Â Â Â ÂThis driver can also be built as a module. If so, the module
>>>>> Â Â Â Âwill be called rtc-mpc5121.
>>>>>
>>>>> +config RTC_DRV_JZ4740
>>>>> + Â Âtristate "Ingenic JZ4740 SoC"
>>>>> + Â Âdepends on RTC_CLASS
>>>>> + Â Âdepends on MACH_JZ4740
>>>>> + Â Âhelp
>>>>> + Â Â ÂIf you say yes here you get support for the Ingenic JZ4740
>>>>> SoC RTC
>>>>> + Â Â Âcontroller.
>>>>> +
>>>>> + Â Â ÂThis driver can also be buillt as a module. If so, the module
>>>>> + Â Â Âwill be called rtc-jz4740.
>>>>> +
>>>>> Âendif # RTC_CLASS
>>>>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>>>>> index 5adbba7..fedf9bb 100644
>>>>> --- a/drivers/rtc/Makefile
>>>>> +++ b/drivers/rtc/Makefile
>>>>> @@ -47,6 +47,7 @@ obj-$(CONFIG_RTC_DRV_EP93XX) Â Â+= rtc-ep93xx.o
>>>>> Âobj-$(CONFIG_RTC_DRV_FM3130) Â Â+= rtc-fm3130.o
>>>>> Âobj-$(CONFIG_RTC_DRV_GENERIC) Â Â+= rtc-generic.o
>>>>> Âobj-$(CONFIG_RTC_DRV_ISL1208) Â Â+= rtc-isl1208.o
>>>>> +obj-$(CONFIG_RTC_DRV_JZ4740) Â Â+= rtc-jz4740.o
>>>>> Âobj-$(CONFIG_RTC_DRV_M41T80) Â Â+= rtc-m41t80.o
>>>>> Âobj-$(CONFIG_RTC_DRV_M41T94) Â Â+= rtc-m41t94.o
>>>>> Âobj-$(CONFIG_RTC_DRV_M48T35) Â Â+= rtc-m48t35.o
>>>>> diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
>>>>> new file mode 100644
>>>>> index 0000000..720afb2
>>>>> --- /dev/null
>>>>> +++ b/drivers/rtc/rtc-jz4740.c
>>>>> @@ -0,0 +1,341 @@
>>>>> +/*
>>>>> + * ÂCopyright (C) 2009-2010, Lars-Peter Clausen <lars@xxxxxxxxxx>
>>>>> + * Â ÂJZ4740 SoC RTC driver
>>>>> + *
>>>>> + * Â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.
>>>>> + *
>>>>> + * ÂYou should have received a copy of the GNU General Public
>>>>> License
>>>>> along + * Âwith this program; if not, write to the Free Software
>>>>> Foundation, Inc., + * Â675 Mass Ave, Cambridge, MA 02139, USA.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/rtc.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/spinlock.h>
>>>>> +
>>>>> +#define JZ_REG_RTC_CTRL Â Â Â Â0x00
>>>>> +#define JZ_REG_RTC_SEC Â Â Â Â0x04
>>>>> +#define JZ_REG_RTC_SEC_ALARM Â Â0x08
>>>>> +#define JZ_REG_RTC_REGULATOR Â Â0x0C
>>>>> +#define JZ_REG_RTC_HIBERNATE Â Â0x20
>>>>> +#define JZ_REG_RTC_SCRATCHPAD Â Â0x34
>>>>> +
>>>>> +#define JZ_RTC_CTRL_WRDY Â ÂBIT(7)
>>>>> +#define JZ_RTC_CTRL_1HZ Â Â Â ÂBIT(6)
>>>>> +#define JZ_RTC_CTRL_1HZ_IRQ Â ÂBIT(5)
>>>>> +#define JZ_RTC_CTRL_AF Â Â Â ÂBIT(4)
>>>>> +#define JZ_RTC_CTRL_AF_IRQ Â ÂBIT(3)
>>>>> +#define JZ_RTC_CTRL_AE Â Â Â ÂBIT(2)
>>>>> +#define JZ_RTC_CTRL_ENABLE Â ÂBIT(0)
>>>>> +
>>>>> +struct jz4740_rtc {
>>>>> + Â Âstruct resource *mem;
>>>>> + Â Âvoid __iomem *base;
>>>>> +
>>>>> + Â Âstruct rtc_device *rtc;
>>>>> +
>>>>> + Â Âunsigned int irq;
>>>>> +
>>>>> + Â Âspinlock_t lock;
>>>>> +};
>>>>> +
>>>>> +static inline uint32_t jz4740_rtc_reg_read(struct jz4740_rtc
>>>>> *rtc, size_t
>>>>> reg) +{
>>>>> + Â Âreturn readl(rtc->base + reg);
>>>>> +}
>>>>> +
>>>>> +static inline void jz4740_rtc_wait_write_ready(struct jz4740_rtc
>>>>> *rtc)
>>>>> +{
>>>>> + Â Âuint32_t ctrl;
>>>>> + Â Âint timeout = 1000;
>>>>> +
>>>>> + Â Âdo {
>>>>> + Â Â Â Âctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
>>>>> + Â Â} while (!(ctrl & JZ_RTC_CTRL_WRDY) && --timeout);
>>>>>
>>>> if (!timeout) {
>>>> Â Â scream_and_die_in_pain();
>>>> Â Â dev_err("I died");
>>>> ... or something like that ... what if it times out, in this
>>>> implementation, noone will know this failed.
>>>>
>>>> I haven't looked through the whole source code, but can't this be
>>>> wrapped into the reg_write() ?
>>>> }
>>>>
>>> Well IF it will ever die, you'll notice cause your rtc clock won't
>>> work
>>> anymore.
>>>
>>> It could be wrapped into reg_write, but there is a different
>>> version of
>>> the SoC with the only difference of the RTC unit being that a
>>> different
>>> mechanism is used determine whether it is ok to write or not. So it
>>> makes sense to keep it seperate.
>>>>> +}
>>>>> +
>>>>> +static inline void jz4740_rtc_reg_write(struct jz4740_rtc *rtc,
>>>>> size_t
>>>>> reg, + Â Âuint32_t val)
>>>>> +{
>>>>> + Â Âjz4740_rtc_wait_write_ready(rtc);
>>>>> + Â Âwritel(val, rtc->base + reg);
>>>>> +}
>>>>> +
>>>>> +static void jz4740_rtc_ctrl_set_bits(struct jz4740_rtc *rtc,
>>>>> uint32_t
>>>>> mask, + Â Âuint32_t val)
>>>>> +{
>>>>> + Â Âunsigned long flags;
>>>>> + Â Âuint32_t ctrl;
>>>>> +
>>>>> + Â Âspin_lock_irqsave(&rtc->lock, flags);
>>>>>
>>>> Can't we use local_irq_save()/local_irq_restore() ?
>>>>
>>> Why would that be preferable? In the non-debug, non-rt case this will
>>> expand to local_irq_{save,restore} anyway, but you'll lose the
>>> semantics
>>> of an lock.
>>
>> Anyway,spin_lock_irqsave is most universal and secure lock function,
>> it can
>> apply to smp or single cpu, it can be ok here.
>>
>>
>>>>
>>>>> +
>>>>> + Â Âctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
>>>>> +
>>>>> + Â Â/* Don't clear interrupt flags by accident */
>>>>> + Â Âctrl |= JZ_RTC_CTRL_1HZ | JZ_RTC_CTRL_AF;
>>>>> +
>>>>> + Â Âctrl &= ~mask;
>>>>> + Â Âctrl |= val;
>>>>> +
>>>>> + Â Âjz4740_rtc_reg_write(rtc, JZ_REG_RTC_CTRL, ctrl);
>>>>> +
>>>>> + Â Âspin_unlock_irqrestore(&rtc->lock, flags);
>>>>> +}
>>>>> +
>>>>> +static int jz4740_rtc_read_time(struct device *dev, struct
>>>>> rtc_time *time)
>>>>> +{
>>>>> + Â Âstruct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>>>> + Â Âuint32_t secs, secs2;
>>>>> + Â Âint timeout = 5;
>>>>> +
>>>>> + Â Â/* If the seconds register is read while it is updated, it
>>>>> can contain a
>>>>> + Â Â * bogus value. This can be avoided by making sure that two
>>>>> consecutive
>>>>> + Â Â * reads have the same value.
>>>>> + Â Â */
>>>>> + Â Âsecs = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC);
>>>>> + Â Âsecs2 = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC);
>>>>> +
>>>>> + Â Âwhile (secs != secs2 && --timeout) {
>>>>> + Â Â Â Âsecs = secs2;
>>>>> + Â Â Â Âsecs2 = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC);
>>>>> + Â Â}
>>>>> +
>>>>> + Â Âif (timeout == 0)
>>>>> + Â Â Â Âreturn -EIO;
>>>>> +
>>>>> + Â Ârtc_time_to_tm(secs, time);
>>>>> +
>>>>> + Â Âreturn rtc_valid_tm(time);
>>>>> +}
>>>>> +
>>>>> +static int jz4740_rtc_set_mmss(struct device *dev, unsigned long
>>>>> secs)
>>>>> +{
>>>>> + Â Âstruct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>>>> +
>>>>> + Â Âif ((uint32_t)secs != secs)
>>>>> + Â Â Â Âreturn -EINVAL;
>>>>>
>>>> Is the typecast here necessary ?
>>>>
>>> Strictly speaking not.
>>>>
>>>>> +
>>>>> + Â Âjz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, secs);
>>>>> +
>>>>> + Â Âreturn 0;
>>>>> +}
>>>>> +
>>>>> +static int jz4740_rtc_read_alarm(struct device *dev, struct
>>>>> rtc_wkalrm
>>>>> *alrm) +{
>>>>> + Â Âstruct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>>>> + Â Âuint32_t secs;
>>>>> + Â Âuint32_t ctrl;
>>>>> +
>>>>> + Â Âsecs = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC_ALARM);
>>>>> +
>>>>> + Â Âctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
>>>>> +
>>>>> + Â Âalrm->enabled = !!(ctrl & JZ_RTC_CTRL_AE);
>>>>> + Â Âalrm->pending = !!(ctrl & JZ_RTC_CTRL_AF);
>>>>> +
>>>>>
>>>> Is the double negation (!!) here necessary ?
>>>>
>>>>
>>> To quote rtc.h "/* 0 = alarm disabled, 1 = alarm enabled */", so yes.
>>
>> You are right, but it is not true reason.
>>
>> Please keep (!!) here, to make sure 'enabled' and 'pending'
>> to be '0' or '1' is good habit, since they are 'unsigned char'
>> variables.
>>
>> Thanks!
>>
>>>>> + Â Ârtc_time_to_tm(secs, &alrm->time);
>>>>> +
>>>>> + Â Âreturn rtc_valid_tm(&alrm->time);
>>>>> +}
>>>>> +
>>>>> +static int jz4740_rtc_set_alarm(struct device *dev, struct
>>>>> rtc_wkalrm
>>>>> *alrm) +{
>>>>> + Â Âstruct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>>>> + Â Âunsigned long secs;
>>>>> +
>>>>> + Â Ârtc_tm_to_time(&alrm->time, &secs);
>>>>> +
>>>>> + Â Âif ((uint32_t)secs != secs)
>>>>> + Â Â Â Âreturn -EINVAL;
>>>>>
>>>> DTTO above
>>>>
>>>>
>>>>> +
>>>>> + Â Âjz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC_ALARM,
>>>>> (uint32_t)secs);
>>>>>
>>>> DTTO
>>>>
>>>>
>>>>> + Â Âjz4740_rtc_ctrl_set_bits(rtc, JZ_RTC_CTRL_AE,
>>>>> + Â Â Â Â Â Â Â Â Â Âalrm->enabled ? JZ_RTC_CTRL_AE : 0);
>>>>>
>>>> Possibly the double negation above wasn't necessary
>>>>
>>>>
>>>>> +
>>>>> + Â Âreturn 0;
>>>>> +}
>>>>> +
>>>>> +static inline int jz4740_irq_enable(struct device *dev, int irq,
>>>>> + Â Âunsigned int enable)
>>>>> +{
>>>>> + Â Âstruct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>>>> + Â Âjz4740_rtc_ctrl_set_bits(rtc, irq, enable ? irq : 0);
>>>>> +
>>>>> + Â Âreturn 0;
>>>>> +}
>>>>> +
>>>>> +static int jz4740_rtc_update_irq_enable(struct device *dev,
>>>>> unsigned int
>>>>> enable) +{
>>>>> + Â Âreturn jz4740_irq_enable(dev, JZ_RTC_CTRL_1HZ_IRQ, enable);
>>>>> +}
>>>>> +
>>>>> +static int jz4740_rtc_alarm_irq_enable(struct device *dev,
>>>>> unsigned int
>>>>> enable) +{
>>>>> + Â Âreturn jz4740_irq_enable(dev, JZ_RTC_CTRL_AF_IRQ, enable);
>>>>> +}
>>>>> +
>>>>> +static struct rtc_class_ops jz4740_rtc_ops = {
>>>>> +  Â.read_time  Â= jz4740_rtc_read_time,
>>>>> +  Â.set_mmss  Â= jz4740_rtc_set_mmss,
>>>>> +  Â.read_alarm  Â= jz4740_rtc_read_alarm,
>>>>> +  Â.set_alarm  Â= jz4740_rtc_set_alarm,
>>>>> + Â Â.update_irq_enable = jz4740_rtc_update_irq_enable,
>>>>> + Â Â.alarm_irq_enable = jz4740_rtc_alarm_irq_enable,
>>>>> +};
>>>>> +
>>>>> +static irqreturn_t jz4740_rtc_irq(int irq, void *data)
>>>>> +{
>>>>> + Â Âstruct jz4740_rtc *rtc = data;
>>>>> + Â Âuint32_t ctrl;
>>>>> + Â Âunsigned long events = 0;
>>>>> + Â Âctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
>>>>> +
>>>>> + Â Âif (ctrl & JZ_RTC_CTRL_1HZ)
>>>>> + Â Â Â Âevents |= (RTC_UF | RTC_IRQF);
>>>>> +
>>>>> + Â Âif (ctrl & JZ_RTC_CTRL_AF)
>>>>> + Â Â Â Âevents |= (RTC_AF | RTC_IRQF);
>>>>> +
>>>>> + Â Ârtc_update_irq(rtc->rtc, 1, events);
>>>>> +
>>>>> + Â Âjz4740_rtc_ctrl_set_bits(rtc, JZ_RTC_CTRL_1HZ |
>>>>> JZ_RTC_CTRL_AF, 0);
>>>>> +
>>>>> + Â Âreturn IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +void jz4740_rtc_poweroff(struct device *dev)
>>>>> +{
>>>>> + Â Âstruct jz4740_rtc *rtc = dev_get_drvdata(dev);
>>>>> + Â Âjz4740_rtc_reg_write(rtc, JZ_REG_RTC_HIBERNATE, 1);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(jz4740_rtc_poweroff);
>>>>> +
>>>>> +static int __devinit jz4740_rtc_probe(struct platform_device *pdev)
>>>>> +{
>>>>> + Â Âint ret;
>>>>> + Â Âstruct jz4740_rtc *rtc;
>>>>> + Â Âuint32_t scratchpad;
>>>>> +
>>>>> + Â Ârtc = kmalloc(sizeof(*rtc), GFP_KERNEL);
>>
>> Please use kzalloc or kmalloc plus memset, but you forget to add
>> memset,
>> I prefer kzalloc, of course, you can use latter.
>>
> All fields of the struct are initialized in probe function, so kzalloc
> is unnecessary overhead(small though).

Of course, you have the correct reason, but  using kzalloc is good habit,
now, many people submit their patches to convert kmalloc + memset.

In addition, if some guys write their drivers refer to your this
driver, but they
did not know initializing all the fields of struct is indispensible,
which will be
a potential dangerous.

Other parts of The driver look good to me, except this issue, so I
cannot recommend
it to Andrew for merging your patch, you have to wait for Andrew's ACK.

>>>>> + Â Âif (!rtc)
>>>>> + Â Â Â Âreturn -ENOMEM;
>>>>> +
>>>>> + Â Ârtc->irq = platform_get_irq(pdev, 0);
>>>>> + Â Âif (rtc->irq < 0) {
>>>>> + Â Â Â Âret = -ENOENT;
>>>>> + Â Â Â Âdev_err(&pdev->dev, "Failed to get platform irq\n");
>>>>> + Â Â Â Âgoto err_free;
>>>>> + Â Â}
>>>>> +
>>>>> + Â Ârtc->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> + Â Âif (!rtc->mem) {
>>>>> + Â Â Â Âret = -ENOENT;
>>>>> + Â Â Â Âdev_err(&pdev->dev, "Failed to get platform mmio
>>>>> memory\n");
>>>>> + Â Â Â Âgoto err_free;
>>>>> + Â Â}
>>>>> +
>>>>> + Â Ârtc->mem = request_mem_region(rtc->mem->start,
>>>>> resource_size(rtc->mem),
>>>>> + Â Â Â Â Â Â Â Â Â Âpdev->name);
>>>>> + Â Âif (!rtc->mem) {
>>>>> + Â Â Â Âret = -EBUSY;
>>>>> + Â Â Â Âdev_err(&pdev->dev, "Failed to request mmio memory
>>>>> region\n");
>>>>> + Â Â Â Âgoto err_free;
>>>>> + Â Â}
>>>>> +
>>>>> + Â Ârtc->base = ioremap_nocache(rtc->mem->start,
>>>>> resource_size(rtc->mem));
>>>>> + Â Âif (!rtc->base) {
>>>>> + Â Â Â Âret = -EBUSY;
>>>>> + Â Â Â Âdev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
>>>>> + Â Â Â Âgoto err_release_mem_region;
>>>>> + Â Â}
>>>>> +
>>>>> + Â Âspin_lock_init(&rtc->lock);
>>>>> +
>>>>> + Â Âplatform_set_drvdata(pdev, rtc);
>>>>>
>>>> dev_set_drvdata()?
>>>>
>>>>
>>> No.
>>>>> +
>>>>> + Â Ârtc->rtc = rtc_device_register(pdev->name, &pdev->dev,
>>>>> &jz4740_rtc_ops,
>>>>> + Â Â Â Â Â Â Â Â Â ÂTHIS_MODULE);
>>>>> + Â Âif (IS_ERR(rtc->rtc)) {
>>>>> + Â Â Â Âret = PTR_ERR(rtc->rtc);
>>>>> + Â Â Â Âdev_err(&pdev->dev, "Failed to register rtc device:
>>>>> %d\n", ret);
>>>>> + Â Â Â Âgoto err_iounmap;
>>>>> + Â Â}
>>>>> +
>>>>> + Â Âret = request_irq(rtc->irq, jz4740_rtc_irq, 0,
>>>>> + Â Â Â Â Â Â Â Âpdev->name, rtc);
>> In fact, I prefer you can use this IRQ flags, such as IRQF_DISABLED,
>> IRQF_SHARED.
> IRQF_DISABLED is deprecated and IRQF_SHARED doesn't make any sense
> here. In fact not requesting with IRQF_SHARED might help to spot
> errors if another driver requested the IRQ by accident (Or this driver
> requested the wrong irq).
>>
>>>>> + Â Âif (ret) {
>>>>> + Â Â Â Âdev_err(&pdev->dev, "Failed to request rtc irq: %d\n",
>>>>> ret);
>>>>> + Â Â Â Âgoto err_unregister_rtc;
>>>>> + Â Â}
>>>>> +
>>>>> + Â Âscratchpad = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
>>>>> + Â Âif (scratchpad != 0x12345678) {
>>>>> + Â Â Â Âjz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD,
>>>>> 0x12345678);
>>>>> + Â Â Â Âjz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, 0);
>>>>> + Â Â}
>>>>> +
>>>>> + Â Âreturn 0;
>>>>> +
>>>>> +err_unregister_rtc:
>>>>> + Â Ârtc_device_unregister(rtc->rtc);
>>>>> +err_iounmap:
>>>>> + Â Âplatform_set_drvdata(pdev, NULL);
>>>>> + Â Âiounmap(rtc->base);
>>>>> +err_release_mem_region:
>>>>> + Â Ârelease_mem_region(rtc->mem->start, resource_size(rtc->mem));
>>>>> +err_free:
>>>>> + Â Âkfree(rtc);
>>>>> +
>>>>> + Â Âreturn ret;
>>>>> +}
>>>>> +
>>>>> +static int __devexit jz4740_rtc_remove(struct platform_device
>>>>> *pdev)
>>>>> +{
>>>>> + Â Âstruct jz4740_rtc *rtc = platform_get_drvdata(pdev);
>>>>>
>>>> dev_get_drvdata();
>>>>
>>>>
>>>>> +
>>>>> + Â Âfree_irq(rtc->irq, rtc);
>>>>> +
>>>>> + Â Ârtc_device_unregister(rtc->rtc);
>>>>> +
>>>>> + Â Âiounmap(rtc->base);
>>>>> + Â Ârelease_mem_region(rtc->mem->start, resource_size(rtc->mem));
>>>>> +
>>>>> + Â Âkfree(rtc);
>>>>> +
>>>>> + Â Âplatform_set_drvdata(pdev, NULL);
>>>>>
>>>> DTTO
>>>>
>>>>
>>>>> +
>>>>> + Â Âreturn 0;
>>>>> +}
>>>>> +
>>>>> +struct platform_driver jz4740_rtc_driver = {
>>>>> + Â Â.probe = jz4740_rtc_probe,
>>>>> + Â Â.remove = __devexit_p(jz4740_rtc_remove),
>>>>> + Â Â.driver = {
>>>>> + Â Â Â Â.name = "jz4740-rtc",
>>>>> + Â Â Â Â.owner = THIS_MODULE,
>>>>> + Â Â},
>>>>> +};
>>>>> +
>>>>> +static int __init jz4740_rtc_init(void)
>>>>> +{
>>>>> + Â Âreturn platform_driver_register(&jz4740_rtc_driver);
>>>>> +}
>>>>> +module_init(jz4740_rtc_init);
>>>>> +
>>>>> +static void __exit jz4740_rtc_exit(void)
>>>>> +{
>>>>> + Â Âplatform_driver_unregister(&jz4740_rtc_driver);
>>>>> +}
>>>>> +module_exit(jz4740_rtc_exit);
>>>>> +
>>>>> +MODULE_AUTHOR("Lars-Peter Clausen <lars@xxxxxxxxxx>");
>>>>> +MODULE_LICENSE("GPL");
>>>>> +MODULE_DESCRIPTION("RTC driver for the JZ4740 SoC\n");
>>>>> +MODULE_ALIAS("platform:jz4740-rtc");
>>>>>
>>>> Cheers
>>>>
> Thanks for reviewing
> - - Lars
>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkwcy80ACgkQBX4mSR26RiM+lgCfZPjAyf1kJstVzqIVlv2uCGMc
> ruMAoIRF2fDWMG8mQJFb9V7iTmVTSgqs
> =2MQV
> -----END PGP SIGNATURE-----
>
>



-- 
*linux-arm-kernel mailing list
mail addr:linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
you can subscribe by:
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

* linux-arm-NUC900 mailing list
mail addr:NUC900@xxxxxxxxxxxxxxxx
main web: https://groups.google.com/group/NUC900
you can subscribe it by sending me mail:
mcuos.com@xxxxxxxxx
--
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/