| Message ID | 1422437276-41334-3-git-send-email-eddie.huang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote: > From: Tianping Fang <tianping.fang@mediatek.com> > > Add Mediatek MT63xx RTC driver There are a couple of checkpatch warnings which should be addressed, please: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #150: new file mode 100644 WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/ #488: FILE: drivers/rtc/rtc-mt6397.c:334: + { .compatible = "mediatek,mt6397-rtc", }, > > ... > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > +{ > + u32 rdata = 0; > + u32 addr = rtc->addr_base + offset; > + > + if (offset < rtc->addr_range) > + regmap_read(rtc->regmap, addr, &rdata); > + > + return (u16)rdata; > +} > + > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) > +{ > + u32 addr; > + > + addr = rtc->addr_base + offset; > + > + if (offset < rtc->addr_range) > + regmap_write(rtc->regmap, addr, data); > +} regmap_read() and regmap_write() can return errors. There is no checking for this. > +static void rtc_write_trigger(struct mt6397_rtc *rtc) > +{ > + rtc_write(rtc, RTC_WRTGR, 1); > + while (rtc_read(rtc, RTC_BBPU) & RTC_BBPU_CBUSY) > + cpu_relax(); > +} > + > > ... > > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm) > +{ > + struct rtc_time *tm = &alm->time; > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + u16 irqen, pdn2; > + > + mutex_lock(&rtc->lock); > + irqen = rtc_read(rtc, RTC_IRQ_EN); > + pdn2 = rtc_read(rtc, RTC_PDN2); > + tm->tm_sec = rtc_read(rtc, RTC_AL_SEC); > + tm->tm_min = rtc_read(rtc, RTC_AL_MIN); > + tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK; > + tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK; > + tm->tm_mon = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK; > + tm->tm_year = rtc_read(rtc, RTC_AL_YEA); > + mutex_unlock(&rtc->lock); > + > + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); > + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); > + > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > + tm->tm_mon--; > + > + return 0; > +} > + > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) > +{ > + struct rtc_time *tm = &alm->time; > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + u16 irqen; > + > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > + tm->tm_mon++; > + > + if (alm->enabled) { > + mutex_lock(&rtc->lock); > + rtc_write(rtc, RTC_AL_YEA, tm->tm_year); > + rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) & > + RTC_NEW_SPARE3) | tm->tm_mon); > + rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) & > + RTC_NEW_SPARE1) | tm->tm_mday); > + rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) & > + RTC_NEW_SPARE_FG_MASK) | tm->tm_hour); > + rtc_write(rtc, RTC_AL_MIN, tm->tm_min); > + rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); > + rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); > + rtc_write_trigger(rtc); > + irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL; > + rtc_write(rtc, RTC_IRQ_EN, irqen); > + rtc_write_trigger(rtc); > + mutex_unlock(&rtc->lock); > + } > + > + return 0; > +} This all looks a bit racy. Wouldn't it be better if the testing of and modification of ->enabled and ->pending were protected by the mutex? > > ... > > +static int mtk_rtc_probe(struct platform_device *pdev) > +{ > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > + struct mt6397_rtc *rtc; > + u32 reg[2]; > + int ret = 0; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2); > + if (ret) { > + dev_err(&pdev->dev, "couldn't read rtc base address!\n"); > + return -EINVAL; > + } > + rtc->addr_base = reg[0]; > + rtc->addr_range = reg[1]; > + rtc->regmap = mt6397_chip->regmap; > + rtc->dev = &pdev->dev; > + mutex_init(&rtc->lock); > + > + platform_set_drvdata(pdev, rtc); > + > + rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev, > + &mtk_rtc_ops, THIS_MODULE); > + if (IS_ERR(rtc->rtc_dev)) { > + dev_err(&pdev->dev, "register rtc device failed\n"); > + return PTR_ERR(rtc->rtc_dev); > + } > + > + rtc->irq = platform_get_irq(pdev, 0); > + if (rtc->irq < 0) { > + ret = rtc->irq; > + goto out_rtc; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, > + rtc_irq_handler_thread, IRQF_ONESHOT, > + "mt6397-rtc", rtc); > + if (ret) { > + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", > + rtc->irq, ret); > + goto out_rtc; > + } > + > + device_init_wakeup(&pdev->dev, 1); > + > + return 0; > + > +out_rtc: > + rtc_device_unregister(rtc->rtc_dev); > + return ret; > + > +} It seems strange to request the IRQ after having registered the rtc. And possibly racy - I seem to recall another driver having issues with this recently. A lot of rtc drivers are requesting the IRQ after registration so presumably it isn't a huge problem. But still, wouldn't it be better to defer registration until after the IRQ has been successfully obtained? > > ... >
Hi Andrew, On Mon, 2015-02-23 at 13:50 -0800, Andrew Morton wrote: > On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote: > > > From: Tianping Fang <tianping.fang@mediatek.com> > > > > Add Mediatek MT63xx RTC driver > > There are a couple of checkpatch warnings which should be addressed, > please: > > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > #150: > new file mode 100644 > OK, I will update MAINTAINERS file > WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/ > #488: FILE: drivers/rtc/rtc-mt6397.c:334: > + { .compatible = "mediatek,mt6397-rtc", }, > > Do you include patch "[PATCH 1/2] dt: bindings: Add Mediatek RTC driver binding document" in this series ? http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320425.html I think this warning shouldn't happen if include this patch. > > > > ... > > > > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) > > +{ > > + u32 addr; > > + > > + addr = rtc->addr_base + offset; > > + > > + if (offset < rtc->addr_range) > > + regmap_write(rtc->regmap, addr, data); > > +} > > regmap_read() and regmap_write() can return errors. There is no > checking for this. Will fix it. > > ... > > > > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm) > > +{ > > + struct rtc_time *tm = &alm->time; > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + u16 irqen, pdn2; > > + > > + mutex_lock(&rtc->lock); > > + irqen = rtc_read(rtc, RTC_IRQ_EN); > > + pdn2 = rtc_read(rtc, RTC_PDN2); > > + tm->tm_sec = rtc_read(rtc, RTC_AL_SEC); > > + tm->tm_min = rtc_read(rtc, RTC_AL_MIN); > > + tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK; > > + tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK; > > + tm->tm_mon = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK; > > + tm->tm_year = rtc_read(rtc, RTC_AL_YEA); > > + mutex_unlock(&rtc->lock); > > + > > + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); > > + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); > > + > > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon--; > > + > > + return 0; > > +} > > + > > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) > > +{ > > + struct rtc_time *tm = &alm->time; > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + u16 irqen; > > + > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon++; > > + > > + if (alm->enabled) { > > + mutex_lock(&rtc->lock); > > + rtc_write(rtc, RTC_AL_YEA, tm->tm_year); > > + rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) & > > + RTC_NEW_SPARE3) | tm->tm_mon); > > + rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) & > > + RTC_NEW_SPARE1) | tm->tm_mday); > > + rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) & > > + RTC_NEW_SPARE_FG_MASK) | tm->tm_hour); > > + rtc_write(rtc, RTC_AL_MIN, tm->tm_min); > > + rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); > > + rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); > > + rtc_write_trigger(rtc); > > + irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL; > > + rtc_write(rtc, RTC_IRQ_EN, irqen); > > + rtc_write_trigger(rtc); > > + mutex_unlock(&rtc->lock); > > + } > > + > > + return 0; > > +} > > This all looks a bit racy. Wouldn't it be better if the testing of and > modification of ->enabled and ->pending were protected by the mutex? > Will fix it. > > > > ... > > > > +static int mtk_rtc_probe(struct platform_device *pdev) > > +{ > > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > > + struct mt6397_rtc *rtc; > > + u32 reg[2]; > > + int ret = 0; > > + > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > > + if (!rtc) > > + return -ENOMEM; > > + > > + ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2); > > + if (ret) { > > + dev_err(&pdev->dev, "couldn't read rtc base address!\n"); > > + return -EINVAL; > > + } > > + rtc->addr_base = reg[0]; > > + rtc->addr_range = reg[1]; > > + rtc->regmap = mt6397_chip->regmap; > > + rtc->dev = &pdev->dev; > > + mutex_init(&rtc->lock); > > + > > + platform_set_drvdata(pdev, rtc); > > + > > + rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev, > > + &mtk_rtc_ops, THIS_MODULE); > > + if (IS_ERR(rtc->rtc_dev)) { > > + dev_err(&pdev->dev, "register rtc device failed\n"); > > + return PTR_ERR(rtc->rtc_dev); > > + } > > + > > + rtc->irq = platform_get_irq(pdev, 0); > > + if (rtc->irq < 0) { > > + ret = rtc->irq; > > + goto out_rtc; > > + } > > + > > + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, > > + rtc_irq_handler_thread, IRQF_ONESHOT, > > + "mt6397-rtc", rtc); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", > > + rtc->irq, ret); > > + goto out_rtc; > > + } > > + > > + device_init_wakeup(&pdev->dev, 1); > > + > > + return 0; > > + > > +out_rtc: > > + rtc_device_unregister(rtc->rtc_dev); > > + return ret; > > + > > +} > > It seems strange to request the IRQ after having registered the rtc. > And possibly racy - I seem to recall another driver having issues with > this recently. > > A lot of rtc drivers are requesting the IRQ after registration so > presumably it isn't a huge problem. But still, wouldn't it be better > to defer registration until after the IRQ has been successfully > obtained? > OK, will fix it. Eddie
On Mon, 2 Mar 2015 16:20:36 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote: > > WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/ > > #488: FILE: drivers/rtc/rtc-mt6397.c:334: > > + { .compatible = "mediatek,mt6397-rtc", }, > > > > > Do you include patch "[PATCH 1/2] dt: bindings: Add Mediatek RTC driver > binding document" in this series ? > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320425.html > > I think this warning shouldn't happen if include this patch. I can't find that patch on rtc-linux or linux-kernel. Resend, please?
Hi, On Mon, 2015-02-23 at 13:50 -0800, Andrew Morton wrote: > On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote: > > > From: Tianping Fang <tianping.fang@mediatek.com> > > > > Add Mediatek MT63xx RTC driver > > There are a couple of checkpatch warnings which should be addressed, > please: > > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > #150: > new file mode 100644 > > WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/ > #488: FILE: drivers/rtc/rtc-mt6397.c:334: > + { .compatible = "mediatek,mt6397-rtc", }, > > > > > > > > ... > > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > > +{ > > + u32 rdata = 0; > > + u32 addr = rtc->addr_base + offset; > > + > > + if (offset < rtc->addr_range) > > + regmap_read(rtc->regmap, addr, &rdata); > > + > > + return (u16)rdata; > > +} > > + > > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) > > +{ > > + u32 addr; > > + > > + addr = rtc->addr_base + offset; > > + > > + if (offset < rtc->addr_range) > > + regmap_write(rtc->regmap, addr, data); > > +} > > regmap_read() and regmap_write() can return errors. There is no > checking for this. > I encounter some trouble when I add code to check return value of regmap_read and regmap_write. Every RTC register access through regmap, and there are many register read/write in this driver. If I check every return value, the driver will become ugly. I try to make this driver clean using following macro. static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data) { u32 addr = rtc->addr_base + offset; if (offset < rtc->addr_range) return regmap_read(rtc->regmap, addr, data); return -EINVAL; } #define rtc_read(ret, rtc, offset, data) \ ({ \ ret = __rtc_read(rtc, offset, data); \ if (ret < 0) \ goto rtc_exit; \ }) \ And function call rtc_read, rtc_write looks like: static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) { unsigned long time; struct mt6397_rtc *rtc = dev_get_drvdata(dev); int ret = 0; u32 sec; mutex_lock(&rtc->lock); do { rtc_read(ret, rtc, RTC_TC_SEC, &tm->tm_sec); rtc_read(ret, rtc, RTC_TC_MIN, &tm->tm_min); rtc_read(ret, rtc, RTC_TC_HOU, &tm->tm_hour); rtc_read(ret, rtc, RTC_TC_DOM, &tm->tm_mday); rtc_read(ret, rtc, RTC_TC_MTH, &tm->tm_mon); rtc_read(ret, rtc, RTC_TC_YEA, &tm->tm_year); rtc_read(ret, rtc, RTC_TC_SEC, &sec); } while (sec < tm->tm_sec); mutex_unlock(&rtc->lock); tm->tm_year += RTC_MIN_YEAR_OFFSET; tm->tm_mon--; rtc_tm_to_time(tm, &time); tm->tm_wday = (time / 86400 + 4) % 7; return 0; rtc_exit: mutex_unlock(&rtc->lock); return ret; } It's a little tricky, does anyone have good suggestion ? Eddie
Hi Eddie, On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote: > > regmap_read() and regmap_write() can return errors. There is no > > checking for this. > > > > I encounter some trouble when I add code to check return value of > regmap_read and regmap_write. Every RTC register access through regmap, > and there are many register read/write in this driver. If I check every > return value, the driver will become ugly. I try to make this driver > clean using following macro. > > static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data) > { > u32 addr = rtc->addr_base + offset; > > if (offset < rtc->addr_range) > return regmap_read(rtc->regmap, addr, data); > > return -EINVAL; > } > > #define rtc_read(ret, rtc, offset, data) \ > ({ \ > ret = __rtc_read(rtc, offset, data); \ > if (ret < 0) \ > goto rtc_exit; \ > }) \ Hiding a goto (or return) in a macro is a very bad idea. what you can do is ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec); ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min); if (ret) return -EIO; (Don't return ret in this case though as it might contain different error codes orred together) Another possibilty at least for contiguous registers would be regmap_bulk_read(). Sascha
On 13/03/15 11:29, Eddie Huang wrote: > > I encounter some trouble when I add code to check return value of > regmap_read and regmap_write. Every RTC register access through regmap, > and there are many register read/write in this driver. If I check every > return value, the driver will become ugly. I try to make this driver > clean using following macro. > > static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data) > { > u32 addr = rtc->addr_base + offset; > > if (offset < rtc->addr_range) > return regmap_read(rtc->regmap, addr, data); > > return -EINVAL; > } > > #define rtc_read(ret, rtc, offset, data) \ > ({ \ > ret = __rtc_read(rtc, offset, data); \ > if (ret < 0) \ > goto rtc_exit; \ > }) \ > I agree with Sascha on hiding a goto statement in a macro is not a good idea. > > And function call rtc_read, rtc_write looks like: > > static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) > { > unsigned long time; > struct mt6397_rtc *rtc = dev_get_drvdata(dev); > int ret = 0; > u32 sec; > > mutex_lock(&rtc->lock); > do { > rtc_read(ret, rtc, RTC_TC_SEC, &tm->tm_sec); > rtc_read(ret, rtc, RTC_TC_MIN, &tm->tm_min); > rtc_read(ret, rtc, RTC_TC_HOU, &tm->tm_hour); > rtc_read(ret, rtc, RTC_TC_DOM, &tm->tm_mday); > rtc_read(ret, rtc, RTC_TC_MTH, &tm->tm_mon); > rtc_read(ret, rtc, RTC_TC_YEA, &tm->tm_year); > rtc_read(ret, rtc, RTC_TC_SEC, &sec); > } while (sec < tm->tm_sec); What about introducing static int __mtk_rtc_read_time(struct mt6397_rtc *rtc, struct rtc_time *tm, u32 *sec) and hide the checks of return values from regmap_read and the offset check in there. You return the error code or 0. This way the while loop would look like this: do { ret = __mtk_rtc_read_time(rtc, &tm, &sec); if (ret < 0) goto rtc_exit; } while (sec < tm->tm_sec); Best regards, Matthias
Hi Sascha, On Fri, 2015-03-13 at 11:57 +0100, Sascha Hauer wrote: > Hi Eddie, > > On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote: > > > regmap_read() and regmap_write() can return errors. There is no > > > checking for this. > > > > > > > I encounter some trouble when I add code to check return value of > > regmap_read and regmap_write. Every RTC register access through regmap, > > and there are many register read/write in this driver. If I check every > > return value, the driver will become ugly. I try to make this driver > > clean using following macro. > > > > static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data) > > { > > u32 addr = rtc->addr_base + offset; > > > > if (offset < rtc->addr_range) > > return regmap_read(rtc->regmap, addr, data); > > > > return -EINVAL; > > } > > > > #define rtc_read(ret, rtc, offset, data) \ > > ({ \ > > ret = __rtc_read(rtc, offset, data); \ > > if (ret < 0) \ > > goto rtc_exit; \ > > }) \ > > Hiding a goto (or return) in a macro is a very bad idea. > > what you can do is > > ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec); > ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min); > > if (ret) > return -EIO; > > (Don't return ret in this case though as it might contain different > error codes orred together) > OK, I will drop macro, and check regmap_read, regmap_write return value in each function. > Another possibilty at least for contiguous registers would be > regmap_bulk_read(). > > Sascha > Contiguous registers access occurs in reading and writing time. I think Matthias's suggestion is a good way: do { ret = __mtk_rtc_read_time(rtc, &tm, &sec); if (ret < 0) goto rtc_exit; } while (sec < tm->tm_sec); Eddie
Hello Eddie, On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > From: Tianping Fang <tianping.fang@mediatek.com> > > Add Mediatek MT63xx RTC driver MT6397? > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index f15cddf..8ac52d8 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART > This driver can also be built as a module. If so, the module > will be called rtc-moxart > > +config RTC_DRV_MT63XX > + tristate "Mediatek Real Time Clock driver" > + depends on MFD_MT6397 I suggest: depends on MFD_MT6397 || COMPILE_TEST (maybe + any hard dependencies you need for compilation). > + help > + This selects the Mediatek(R) RTC driver, you should add support > + for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver. > + > + If you want to use Mediatek(R) RTC interface, select Y or M here. > + If unsure, Please select N. Given the dependency above I'd say choosing y here is fine. Instead of recommending that I'd just drop this line. > [...] > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) rtc_read is a bad name for a driver. There are already 6 functions with this name in the kernel. Better use a unique prefix. > [...] > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > +{ > + struct mt6397_rtc *rtc = data; > + u16 irqsta, irqen; > + > + mutex_lock(&rtc->lock); > + irqsta = rtc_read(rtc, RTC_IRQ_STA); Do you really need to lock for a single read access? > + mutex_unlock(&rtc->lock); > + > + if (irqsta & RTC_IRQ_STA_AL) { > + rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF); > + irqen = irqsta & ~RTC_IRQ_EN_AL; > + rtc_write(rtc, RTC_IRQ_EN, irqen); > + rtc_write_trigger(rtc); > + return IRQ_HANDLED; > + } > + > + return IRQ_NONE; > +} > + > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + unsigned long time; > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + > + mutex_lock(&rtc->lock); > + do { > + tm->tm_sec = rtc_read(rtc, RTC_TC_SEC); > + tm->tm_min = rtc_read(rtc, RTC_TC_MIN); > + tm->tm_hour = rtc_read(rtc, RTC_TC_HOU); > + tm->tm_mday = rtc_read(rtc, RTC_TC_DOM); > + tm->tm_mon = rtc_read(rtc, RTC_TC_MTH); > + tm->tm_year = rtc_read(rtc, RTC_TC_YEA); > + } while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec); > + mutex_unlock(&rtc->lock); > + > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > + tm->tm_mon--; > + rtc_tm_to_time(tm, &time); rtc_tm_to_time is deprecated, better use rtc_tm_to_time64. > + tm->tm_wday = (time / 86400 + 4) % 7; > + > + return 0; > +} > + > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > + tm->tm_mon++; > + mutex_lock(&rtc->lock); > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you write to it but after you wrote RTC_TC_MIN? > + rtc_write_trigger(rtc); > + mutex_unlock(&rtc->lock); > + > + return 0; > +} > + > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm) > +{ > + struct rtc_time *tm = &alm->time; > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + u16 irqen, pdn2; > + > + mutex_lock(&rtc->lock); > + irqen = rtc_read(rtc, RTC_IRQ_EN); > + pdn2 = rtc_read(rtc, RTC_PDN2); > + tm->tm_sec = rtc_read(rtc, RTC_AL_SEC); > + tm->tm_min = rtc_read(rtc, RTC_AL_MIN); > + tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK; > + tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK; > + tm->tm_mon = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK; > + tm->tm_year = rtc_read(rtc, RTC_AL_YEA); > + mutex_unlock(&rtc->lock); > + > + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); > + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); > + > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > + tm->tm_mon--; > + > + return 0; > +} > + > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) > +{ > + struct rtc_time *tm = &alm->time; > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + u16 irqen; > + > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > + tm->tm_mon++; > + > + if (alm->enabled) { > + mutex_lock(&rtc->lock); > + rtc_write(rtc, RTC_AL_YEA, tm->tm_year); > + rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) & > + RTC_NEW_SPARE3) | tm->tm_mon); This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register name? I would have expected: (rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon; > + rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) & > + RTC_NEW_SPARE1) | tm->tm_mday); > + rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) & > + RTC_NEW_SPARE_FG_MASK) | tm->tm_hour); > + rtc_write(rtc, RTC_AL_MIN, tm->tm_min); > + rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); > + rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); Is this racy? I.e. if the previous set alarm is 2015-03-13 14:15:00 and you write 2015-03.14 17:17:00 is it possible that this triggers an alarm if the update happens at 2015-03-14 14:15:00 ? > + rtc_write_trigger(rtc); > + irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL; > + rtc_write(rtc, RTC_IRQ_EN, irqen); > + rtc_write_trigger(rtc); > + mutex_unlock(&rtc->lock); } else { /* disable alarm here */ > + } > + > + return 0; > +} > + > +static struct rtc_class_ops mtk_rtc_ops = { > + .read_time = mtk_rtc_read_time, > + .set_time = mtk_rtc_set_time, > + .read_alarm = mtk_rtc_read_alarm, > + .set_alarm = mtk_rtc_set_alarm, > +}; > + > +static int mtk_rtc_probe(struct platform_device *pdev) > +{ > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > + struct mt6397_rtc *rtc; > + u32 reg[2]; > + int ret = 0; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2); > + if (ret) { > + dev_err(&pdev->dev, "couldn't read rtc base address!\n"); > + return -EINVAL; > + } > + rtc->addr_base = reg[0]; > + rtc->addr_range = reg[1]; This looks strange, but maybe that's right as you reuse the parent's regmap. > + rtc->regmap = mt6397_chip->regmap; > + rtc->dev = &pdev->dev; > + mutex_init(&rtc->lock); > + > + platform_set_drvdata(pdev, rtc); > + > + rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev, > + &mtk_rtc_ops, THIS_MODULE); > + if (IS_ERR(rtc->rtc_dev)) { > + dev_err(&pdev->dev, "register rtc device failed\n"); > + return PTR_ERR(rtc->rtc_dev); > + } > + > + rtc->irq = platform_get_irq(pdev, 0); > + if (rtc->irq < 0) { platform_get_irq(pdev, 0) = 0 should be treated as error, too. > + ret = rtc->irq; > + goto out_rtc; > + } Best regards Uwe
Hi Uwe, Thanks your review. On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote: > Hello Eddie, > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > From: Tianping Fang <tianping.fang@mediatek.com> > > > > Add Mediatek MT63xx RTC driver > MT6397? Yes, it is better to use MT6397 > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > > index f15cddf..8ac52d8 100644 > > --- a/drivers/rtc/Kconfig > > +++ b/drivers/rtc/Kconfig > > @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART > > This driver can also be built as a module. If so, the module > > will be called rtc-moxart > > > > +config RTC_DRV_MT63XX > > + tristate "Mediatek Real Time Clock driver" > > + depends on MFD_MT6397 > I suggest: > > depends on MFD_MT6397 || COMPILE_TEST > > (maybe + any hard dependencies you need for compilation). OK, will fix it > > > + help > > + This selects the Mediatek(R) RTC driver, you should add support > > + for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver. > > + > > + If you want to use Mediatek(R) RTC interface, select Y or M here. > > + If unsure, Please select N. > Given the dependency above I'd say choosing y here is fine. Instead of > recommending that I'd just drop this line. ok, will drop. > > > [...] > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > rtc_read is a bad name for a driver. There are already 6 functions with > this name in the kernel. Better use a unique prefix. I will use prefix mtk_ > > > [...] > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > +{ > > + struct mt6397_rtc *rtc = data; > > + u16 irqsta, irqen; > > + > > + mutex_lock(&rtc->lock); > > + irqsta = rtc_read(rtc, RTC_IRQ_STA); > Do you really need to lock for a single read access? I think this lock is necessary, because other thread may access rtc register at the same time, for example, call mtk_rtc_set_alarm to modify alarm time. > > > + mutex_unlock(&rtc->lock); > > + > > + if (irqsta & RTC_IRQ_STA_AL) { > > + rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF); > > + irqen = irqsta & ~RTC_IRQ_EN_AL; > > + rtc_write(rtc, RTC_IRQ_EN, irqen); > > + rtc_write_trigger(rtc); > > + return IRQ_HANDLED; > > + } > > + > > + return IRQ_NONE; > > +} > > + > > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + unsigned long time; > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + > > + mutex_lock(&rtc->lock); > > + do { > > + tm->tm_sec = rtc_read(rtc, RTC_TC_SEC); > > + tm->tm_min = rtc_read(rtc, RTC_TC_MIN); > > + tm->tm_hour = rtc_read(rtc, RTC_TC_HOU); > > + tm->tm_mday = rtc_read(rtc, RTC_TC_DOM); > > + tm->tm_mon = rtc_read(rtc, RTC_TC_MTH); > > + tm->tm_year = rtc_read(rtc, RTC_TC_YEA); > > + } while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec); > > + mutex_unlock(&rtc->lock); > > + > > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon--; > > + rtc_tm_to_time(tm, &time); > rtc_tm_to_time is deprecated, better use rtc_tm_to_time64. OK, will change to rtc_tm_to_time64 > > > + tm->tm_wday = (time / 86400 + 4) % 7; > > + > > + return 0; > > +} > > + > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon++; > > + mutex_lock(&rtc->lock); > > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you > write to it but after you wrote RTC_TC_MIN? register value will write to hardware after rtc_write_trigger, so the racy condition not exist. > > > + rtc_write_trigger(rtc); > > + mutex_unlock(&rtc->lock); > > + > > + return 0; > > +} > > + > > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm) > > +{ > > + struct rtc_time *tm = &alm->time; > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + u16 irqen, pdn2; > > + > > + mutex_lock(&rtc->lock); > > + irqen = rtc_read(rtc, RTC_IRQ_EN); > > + pdn2 = rtc_read(rtc, RTC_PDN2); > > + tm->tm_sec = rtc_read(rtc, RTC_AL_SEC); > > + tm->tm_min = rtc_read(rtc, RTC_AL_MIN); > > + tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK; > > + tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK; > > + tm->tm_mon = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK; > > + tm->tm_year = rtc_read(rtc, RTC_AL_YEA); > > + mutex_unlock(&rtc->lock); > > + > > + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); > > + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); > > + > > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon--; > > + > > + return 0; > > +} > > + > > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) > > +{ > > + struct rtc_time *tm = &alm->time; > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + u16 irqen; > > + > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon++; > > + > > + if (alm->enabled) { > > + mutex_lock(&rtc->lock); > > + rtc_write(rtc, RTC_AL_YEA, tm->tm_year); > > + rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) & > > + RTC_NEW_SPARE3) | tm->tm_mon); > This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register > name? I would have expected: > > (rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon; I will remove RTC_NEW_SPARE3. Hardware will return 0 if register bit is useless. So the bit clear is redundant. > > > + rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) & > > + RTC_NEW_SPARE1) | tm->tm_mday); > > + rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) & > > + RTC_NEW_SPARE_FG_MASK) | tm->tm_hour); > > + rtc_write(rtc, RTC_AL_MIN, tm->tm_min); > > + rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); > > + rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); > Is this racy? I.e. if the previous set alarm is > > 2015-03-13 14:15:00 > > and you write > > 2015-03.14 17:17:00 > > is it possible that this triggers an alarm if the update happens at > > 2015-03-14 14:15:00 > > ? > All rtc register value write to RTC hardware after rtc_write_trigger. Race condition should not happen. > > + rtc_write_trigger(rtc); > > + irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL; > > + rtc_write(rtc, RTC_IRQ_EN, irqen); > > + rtc_write_trigger(rtc); > > + mutex_unlock(&rtc->lock); > } else { > /* disable alarm here */ > OK, should clear RTC_IRQ_EN > > + } > > + > > + return 0; > > +} > > + > > +static struct rtc_class_ops mtk_rtc_ops = { > > + .read_time = mtk_rtc_read_time, > > + .set_time = mtk_rtc_set_time, > > + .read_alarm = mtk_rtc_read_alarm, > > + .set_alarm = mtk_rtc_set_alarm, > > +}; > > + > > +static int mtk_rtc_probe(struct platform_device *pdev) > > +{ > > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > > + struct mt6397_rtc *rtc; > > + u32 reg[2]; > > + int ret = 0; > > + > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > > + if (!rtc) > > + return -ENOMEM; > > + > > + ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2); > > + if (ret) { > > + dev_err(&pdev->dev, "couldn't read rtc base address!\n"); > > + return -EINVAL; > > + } > > + rtc->addr_base = reg[0]; > > + rtc->addr_range = reg[1]; > This looks strange, but maybe that's right as you reuse the parent's > regmap. According Sascha and Mark Brown's discussion: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323239.html Address and interrupt will move from device tree to mfd_cell in mt6397-core.c > > > + rtc->regmap = mt6397_chip->regmap; > > + rtc->dev = &pdev->dev; > > + mutex_init(&rtc->lock); > > + > > + platform_set_drvdata(pdev, rtc); > > + > > + rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev, > > + &mtk_rtc_ops, THIS_MODULE); > > + if (IS_ERR(rtc->rtc_dev)) { > > + dev_err(&pdev->dev, "register rtc device failed\n"); > > + return PTR_ERR(rtc->rtc_dev); > > + } > > + > > + rtc->irq = platform_get_irq(pdev, 0); > > + if (rtc->irq < 0) { > platform_get_irq(pdev, 0) = 0 should be treated as error, too. OK, will fix it > > > + ret = rtc->irq; > > + goto out_rtc; > > + } > > Best regards > Uwe >
Hello Eddie, On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote: > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote: > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > > [...] > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > > rtc_read is a bad name for a driver. There are already 6 functions with > > this name in the kernel. Better use a unique prefix. > > I will use prefix mtk_ I would prefer a prefix that is unique to the driver. "mtk_" doesn't work to distinguish between the rtc and a (say) spi driver. What you want here is that if someone reports a bug on any mailinglist with a backtrace you are able to immediately see which driver is affected. > > > [...] > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > > +{ > > > + struct mt6397_rtc *rtc = data; > > > + u16 irqsta, irqen; > > > + > > > + mutex_lock(&rtc->lock); > > > + irqsta = rtc_read(rtc, RTC_IRQ_STA); > > Do you really need to lock for a single read access? > > I think this lock is necessary, because other thread may access rtc > register at the same time, for example, call mtk_rtc_set_alarm to modify > alarm time. That would be a valid reason if mtk_rtc_set_alarm touched that register twice in a single critical section and the handler must not read the value of the first write. Otherwise it should be fine, shouldn't it? > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > > > +{ > > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > > + > > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > > + tm->tm_mon++; > > > + mutex_lock(&rtc->lock); > > > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you > > write to it but after you wrote RTC_TC_MIN? > > register value will write to hardware after rtc_write_trigger, so the > racy condition not exist. Ah, it seems the hardware guys did their job. Nice. Best regards Uwe
Hi Uwe, On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-König wrote: > Hello Eddie, > > On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote: > > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote: > > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > > > [...] > > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) > > > rtc_read is a bad name for a driver. There are already 6 functions with > > > this name in the kernel. Better use a unique prefix. > > > > I will use prefix mtk_ > I would prefer a prefix that is unique to the driver. "mtk_" doesn't > work to distinguish between the rtc and a (say) spi driver. What you > want here is that if someone reports a bug on any mailinglist with a > backtrace you are able to immediately see which driver is affected. > My meaning is mtk_rtc_read, mtk_rtc_write. > > > > [...] > > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > > > +{ > > > > + struct mt6397_rtc *rtc = data; > > > > + u16 irqsta, irqen; > > > > + > > > > + mutex_lock(&rtc->lock); > > > > + irqsta = rtc_read(rtc, RTC_IRQ_STA); > > > Do you really need to lock for a single read access? > > > > I think this lock is necessary, because other thread may access rtc > > register at the same time, for example, call mtk_rtc_set_alarm to modify > > alarm time. > That would be a valid reason if mtk_rtc_set_alarm touched that register > twice in a single critical section and the handler must not read the > value of the first write. Otherwise it should be fine, shouldn't it? > My original though is if disable alarm in mtk_rtc_set_alarm function, RTC_IRQ_STA may be affected, this is why I add mutex. After checking with designer, RTC_IRQ_STA will not be affected. I will remove the mutex. > > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > > > > +{ > > > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > > > + > > > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > > > + tm->tm_mon++; > > > > + mutex_lock(&rtc->lock); > > > > + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > > > + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > > > + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > > > + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > > > + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > > > + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you > > > write to it but after you wrote RTC_TC_MIN? > > > > register value will write to hardware after rtc_write_trigger, so the > > racy condition not exist. > Ah, it seems the hardware guys did their job. Nice. > > Best regards > Uwe >
Hello Eddie, On Wed, Mar 18, 2015 at 11:27:40AM +0800, Eddie Huang wrote: > On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-König wrote: > > On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote: > > > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote: > > > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote: > > > > > [...] > > > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) > > > > > +{ > > > > > + struct mt6397_rtc *rtc = data; > > > > > + u16 irqsta, irqen; > > > > > + > > > > > + mutex_lock(&rtc->lock); > > > > > + irqsta = rtc_read(rtc, RTC_IRQ_STA); > > > > Do you really need to lock for a single read access? > > > > > > I think this lock is necessary, because other thread may access rtc > > > register at the same time, for example, call mtk_rtc_set_alarm to modify > > > alarm time. > > That would be a valid reason if mtk_rtc_set_alarm touched that register > > twice in a single critical section and the handler must not read the > > value of the first write. Otherwise it should be fine, shouldn't it? > > > > My original though is if disable alarm in mtk_rtc_set_alarm function, > RTC_IRQ_STA may be affected, this is why I add mutex. After checking > with designer, RTC_IRQ_STA will not be affected. I will remove the > mutex. Even if IRQ_STA is changed there is no problem. With the lock the following can happen: Either the irq thread comes first: thread1 thread2 lock rtc_read unlock ------------> lock dosomething with IRQ_STA unlock or the other thread comes first: thread1 thread2 lock dosomething with IRQ_STA lock <------------- unlock rtc_read unlock So thread1 either reads the old or the new value of IRQ_STA. Without locking you have the same! Either the write in thread2 comes first or not. And depending on that you either read the new or the old value respectively. Of course this assumes that a write is atomic in the sense that if you update a value from 0x01 to 0x10 there is no point in time a reader can see 0x00 or 0x11. But this is usually given. Also reading the register in question must not have side effects that affect the other threads. Best regards Uwe
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index f15cddf..8ac52d8 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART This driver can also be built as a module. If so, the module will be called rtc-moxart +config RTC_DRV_MT63XX + tristate "Mediatek Real Time Clock driver" + depends on MFD_MT6397 + help + This selects the Mediatek(R) RTC driver, you should add support + for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver. + + If you want to use Mediatek(R) RTC interface, select Y or M here. + If unsure, Please select N. + config RTC_DRV_XGENE tristate "APM X-Gene RTC" depends on HAS_IOMEM diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index c8ef3e1..53e0085 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -150,3 +150,4 @@ obj-$(CONFIG_RTC_DRV_X1205) += rtc-x1205.o obj-$(CONFIG_RTC_DRV_XGENE) += rtc-xgene.o obj-$(CONFIG_RTC_DRV_SIRFSOC) += rtc-sirfsoc.o obj-$(CONFIG_RTC_DRV_MOXART) += rtc-moxart.o +obj-$(CONFIG_RTC_DRV_MT63XX) += rtc-mt6397.o diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c new file mode 100644 index 0000000..b54d605 --- /dev/null +++ b/drivers/rtc/rtc-mt6397.c @@ -0,0 +1,352 @@ +/* +* Copyright (c) 2014-2015 MediaTek Inc. +* Author: Tianping.Fang <tianping.fang@mediatek.com> +* +* 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. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/delay.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/kernel.h> +#include <linux/regmap.h> +#include <linux/rtc.h> +#include <linux/irqdomain.h> +#include <linux/spinlock.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/of_address.h> +#include <linux/kernel.h> +#include <linux/io.h> +#include <linux/mfd/mt6397/core.h> + +#define RTC_BBPU 0x0000 +#define RTC_WRTGR 0x003c +#define RTC_IRQ_EN 0x0004 +#define RTC_IRQ_STA 0x0002 + +#define RTC_BBPU_CBUSY (1 << 6) +#define RTC_BBPU_KEY (0x43 << 8) +#define RTC_BBPU_AUTO (1 << 3) +#define RTC_IRQ_STA_AL (1 << 0) +#define RTC_IRQ_STA_LP (1 << 3) + +#define RTC_TC_SEC 0x000a +#define RTC_TC_MIN 0x000c +#define RTC_TC_HOU 0x000e +#define RTC_TC_DOM 0x0010 +#define RTC_TC_MTH 0x0014 +#define RTC_TC_YEA 0x0016 +#define RTC_AL_SEC 0x0018 +#define RTC_AL_MIN 0x001a + +#define RTC_IRQ_EN_AL (1 << 0) +#define RTC_IRQ_EN_ONESHOT (1 << 2) +#define RTC_IRQ_EN_LP (1 << 3) +#define RTC_IRQ_EN_ONESHOT_AL (RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL) + +#define RTC_TC_MIN_MASK 0x003f +#define RTC_TC_SEC_MASK 0x003f +#define RTC_TC_HOU_MASK 0x001f +#define RTC_TC_DOM_MASK 0x001f +#define RTC_TC_MTH_MASK 0x000f +#define RTC_TC_YEA_MASK 0x007f + +#define RTC_AL_SEC_MASK 0x003f +#define RTC_AL_MIN_MASK 0x003f +#define RTC_AL_MASK_DOW (1 << 4) + +#define RTC_AL_HOU 0x001c +#define RTC_NEW_SPARE_FG_MASK 0xff00 +#define RTC_NEW_SPARE_FG_SHIFT 8 +#define RTC_AL_HOU_MASK 0x001f + +#define RTC_AL_DOM 0x001e +#define RTC_NEW_SPARE1 0xff00 +#define RTC_AL_DOM_MASK 0x001f +#define RTC_AL_MASK 0x0008 + +#define RTC_AL_MTH 0x0022 +#define RTC_NEW_SPARE3 0xff00 +#define RTC_AL_MTH_MASK 0x000f + +#define RTC_AL_YEA 0x0024 +#define RTC_AL_YEA_MASK 0x007f + +#define RTC_PDN1 0x002c +#define RTC_PDN1_PWRON_TIME (1 << 7) + +#define RTC_PDN2 0x002e +#define RTC_PDN2_PWRON_MTH_MASK 0x000f +#define RTC_PDN2_PWRON_MTH_SHIFT 0 +#define RTC_PDN2_PWRON_ALARM (1 << 4) +#define RTC_PDN2_UART_MASK 0x0060 +#define RTC_PDN2_UART_SHIFT 5 +#define RTC_PDN2_PWRON_YEA_MASK 0x7f00 +#define RTC_PDN2_PWRON_YEA_SHIFT 8 +#define RTC_PDN2_PWRON_LOGO (1 << 15) + +#define RTC_MIN_YEAR 1968 +#define RTC_BASE_YEAR 1900 +#define RTC_NUM_YEARS 128 +#define RTC_MIN_YEAR_OFFSET (RTC_MIN_YEAR - RTC_BASE_YEAR) +#define RTC_RELPWR_WHEN_XRST 1 + +struct mt6397_rtc { + struct device *dev; + struct rtc_device *rtc_dev; + struct mutex lock; + struct regmap *regmap; + int irq; + u32 addr_base; + u32 addr_range; +}; + +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset) +{ + u32 rdata = 0; + u32 addr = rtc->addr_base + offset; + + if (offset < rtc->addr_range) + regmap_read(rtc->regmap, addr, &rdata); + + return (u16)rdata; +} + +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) +{ + u32 addr; + + addr = rtc->addr_base + offset; + + if (offset < rtc->addr_range) + regmap_write(rtc->regmap, addr, data); +} + +static void rtc_write_trigger(struct mt6397_rtc *rtc) +{ + rtc_write(rtc, RTC_WRTGR, 1); + while (rtc_read(rtc, RTC_BBPU) & RTC_BBPU_CBUSY) + cpu_relax(); +} + +static irqreturn_t rtc_irq_handler_thread(int irq, void *data) +{ + struct mt6397_rtc *rtc = data; + u16 irqsta, irqen; + + mutex_lock(&rtc->lock); + irqsta = rtc_read(rtc, RTC_IRQ_STA); + mutex_unlock(&rtc->lock); + + if (irqsta & RTC_IRQ_STA_AL) { + rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF); + irqen = irqsta & ~RTC_IRQ_EN_AL; + rtc_write(rtc, RTC_IRQ_EN, irqen); + rtc_write_trigger(rtc); + return IRQ_HANDLED; + } + + return IRQ_NONE; +} + +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) +{ + unsigned long time; + struct mt6397_rtc *rtc = dev_get_drvdata(dev); + + mutex_lock(&rtc->lock); + do { + tm->tm_sec = rtc_read(rtc, RTC_TC_SEC); + tm->tm_min = rtc_read(rtc, RTC_TC_MIN); + tm->tm_hour = rtc_read(rtc, RTC_TC_HOU); + tm->tm_mday = rtc_read(rtc, RTC_TC_DOM); + tm->tm_mon = rtc_read(rtc, RTC_TC_MTH); + tm->tm_year = rtc_read(rtc, RTC_TC_YEA); + } while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec); + mutex_unlock(&rtc->lock); + + tm->tm_year += RTC_MIN_YEAR_OFFSET; + tm->tm_mon--; + rtc_tm_to_time(tm, &time); + + tm->tm_wday = (time / 86400 + 4) % 7; + + return 0; +} + +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) +{ + struct mt6397_rtc *rtc = dev_get_drvdata(dev); + + tm->tm_year -= RTC_MIN_YEAR_OFFSET; + tm->tm_mon++; + mutex_lock(&rtc->lock); + rtc_write(rtc, RTC_TC_YEA, tm->tm_year); + rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); + rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); + rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); + rtc_write(rtc, RTC_TC_MIN, tm->tm_min); + rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); + rtc_write_trigger(rtc); + mutex_unlock(&rtc->lock); + + return 0; +} + +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm) +{ + struct rtc_time *tm = &alm->time; + struct mt6397_rtc *rtc = dev_get_drvdata(dev); + u16 irqen, pdn2; + + mutex_lock(&rtc->lock); + irqen = rtc_read(rtc, RTC_IRQ_EN); + pdn2 = rtc_read(rtc, RTC_PDN2); + tm->tm_sec = rtc_read(rtc, RTC_AL_SEC); + tm->tm_min = rtc_read(rtc, RTC_AL_MIN); + tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK; + tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK; + tm->tm_mon = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK; + tm->tm_year = rtc_read(rtc, RTC_AL_YEA); + mutex_unlock(&rtc->lock); + + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); + + tm->tm_year += RTC_MIN_YEAR_OFFSET; + tm->tm_mon--; + + return 0; +} + +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm) +{ + struct rtc_time *tm = &alm->time; + struct mt6397_rtc *rtc = dev_get_drvdata(dev); + u16 irqen; + + tm->tm_year -= RTC_MIN_YEAR_OFFSET; + tm->tm_mon++; + + if (alm->enabled) { + mutex_lock(&rtc->lock); + rtc_write(rtc, RTC_AL_YEA, tm->tm_year); + rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) & + RTC_NEW_SPARE3) | tm->tm_mon); + rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) & + RTC_NEW_SPARE1) | tm->tm_mday); + rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) & + RTC_NEW_SPARE_FG_MASK) | tm->tm_hour); + rtc_write(rtc, RTC_AL_MIN, tm->tm_min); + rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); + rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); + rtc_write_trigger(rtc); + irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL; + rtc_write(rtc, RTC_IRQ_EN, irqen); + rtc_write_trigger(rtc); + mutex_unlock(&rtc->lock); + } + + return 0; +} + +static struct rtc_class_ops mtk_rtc_ops = { + .read_time = mtk_rtc_read_time, + .set_time = mtk_rtc_set_time, + .read_alarm = mtk_rtc_read_alarm, + .set_alarm = mtk_rtc_set_alarm, +}; + +static int mtk_rtc_probe(struct platform_device *pdev) +{ + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); + struct mt6397_rtc *rtc; + u32 reg[2]; + int ret = 0; + + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); + if (!rtc) + return -ENOMEM; + + ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2); + if (ret) { + dev_err(&pdev->dev, "couldn't read rtc base address!\n"); + return -EINVAL; + } + rtc->addr_base = reg[0]; + rtc->addr_range = reg[1]; + rtc->regmap = mt6397_chip->regmap; + rtc->dev = &pdev->dev; + mutex_init(&rtc->lock); + + platform_set_drvdata(pdev, rtc); + + rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev, + &mtk_rtc_ops, THIS_MODULE); + if (IS_ERR(rtc->rtc_dev)) { + dev_err(&pdev->dev, "register rtc device failed\n"); + return PTR_ERR(rtc->rtc_dev); + } + + rtc->irq = platform_get_irq(pdev, 0); + if (rtc->irq < 0) { + ret = rtc->irq; + goto out_rtc; + } + + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, + rtc_irq_handler_thread, IRQF_ONESHOT, + "mt6397-rtc", rtc); + if (ret) { + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", + rtc->irq, ret); + goto out_rtc; + } + + device_init_wakeup(&pdev->dev, 1); + + return 0; + +out_rtc: + rtc_device_unregister(rtc->rtc_dev); + return ret; + +} + +static int mtk_rtc_remove(struct platform_device *pdev) +{ + struct mt6397_rtc *rtc = platform_get_drvdata(pdev); + + rtc_device_unregister(rtc->rtc_dev); + return 0; +} + +static const struct of_device_id mt6397_rtc_of_match[] = { + { .compatible = "mediatek,mt6397-rtc", }, + { } +}; + +static struct platform_driver mtk_rtc_driver = { + .driver = { + .name = "mt6397-rtc", + .of_match_table = mt6397_rtc_of_match, + }, + .probe = mtk_rtc_probe, + .remove = mtk_rtc_remove, +}; + +module_platform_driver(mtk_rtc_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>"); +MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC"); +MODULE_ALIAS("platform:mt6397-rtc");