Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756014Ab0FSOGf (ORCPT ); Sat, 19 Jun 2010 10:06:35 -0400 Received: from mail-fx0-f46.google.com ([209.85.161.46]:53398 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755978Ab0FSOGd convert rfc822-to-8bit (ORCPT ); Sat, 19 Jun 2010 10:06:33 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:organization:to:subject:date:user-agent:cc:references :in-reply-to:mime-version:content-type:content-transfer-encoding :message-id; b=XsGUYnOE3LXlMneOc14JeBX2UKkHjEj7uoWwS4S7maNMWDonmLGa3tmJmONlSzuZ4l LgSJCPU1J331hjHwfDKaQIKjet/lhVUaucXLeD3xfaJxxkIosbs8U/LfX6M+SxPn7J9P cOLmfZvjbsJNZH3Sb+fCIRGOU/LMb2aIt901s= From: Marek Vasut Organization: Hack&Dev To: "Lars-Peter Clausen" Subject: Re: [PATCH v2 15/26] RTC: Add JZ4740 RTC driver Date: Sat, 19 Jun 2010 16:04:25 +0200 User-Agent: KMail/1.13.3 (Linux/2.6.33-2-amd64; KDE/4.4.3; x86_64; ; ) Cc: Ralf Baechle , linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, Alessandro Zummo , Paul Gortmaker , Wan ZongShun , rtc-linux@googlegroups.com References: <1276924111-11158-1-git-send-email-lars@metafoo.de> <201006191243.39793.marek.vasut@gmail.com> <4C1CC08E.5050009@metafoo.de> In-Reply-To: <4C1CC08E.5050009@metafoo.de> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Message-Id: <201006191604.25329.marek.vasut@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14760 Lines: 499 Dne So 19. června 2010 15:05:18 Lars-Peter Clausen napsal(a): > 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 > >> Cc: Alessandro Zummo > >> Cc: Paul Gortmaker > >> Cc: Wan ZongShun > >> Cc: Marek Vasut > >> Cc: rtc-linux@googlegroups.com > >> > >> --- > >> 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 > >> + * 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 > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#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. Then maybe some dev_err() would be good to have there. > > 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. OK > > >> +} > >> + > >> +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. I believe on SMP systems, local_irq_save will give you finer locking granularity. > > >> + > >> + 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. OK > > >> + > >> + 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. Oh my ... well, maybe someone should fix that to take 0 for NO, !0 for YES. > > >> + 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); > >> + 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. Why not ? > > >> + > >> + 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); > >> + 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 "); > >> +MODULE_LICENSE("GPL"); > >> +MODULE_DESCRIPTION("RTC driver for the JZ4740 SoC\n"); > >> +MODULE_ALIAS("platform:jz4740-rtc"); > > > > Cheers > > Thanks for reviewing > > - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/