| Message ID | 1426657537-41414-3-git-send-email-eddie.huang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Eddie, On Tue, Mar 17, 2015 at 10:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote: > +static int mtk_rtc_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > + struct mt6397_rtc *rtc; > + int ret = 0; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + rtc->addr_base = res->start; > + rtc->addr_range = res->end - res->start; > + > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start); > + if (rtc->irq <= 0) > + goto out_rtc; > + > + rtc->regmap = mt6397_chip->regmap; > + rtc->dev = &pdev->dev; > + mutex_init(&rtc->lock); > + > + platform_set_drvdata(pdev, rtc); > + > + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, > + mtk_rtc_irq_handler_thread, > + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > + "mt6397-rtc", rtc); > + if (ret) { > + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", > + rtc->irq, ret); > + goto out_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); > + } > + > + device_init_wakeup(&pdev->dev, 1); > + > + return 0; > + > +out_rtc: > + rtc_device_unregister(rtc->rtc_dev); This is wrong. Whenever you jump to this label the RTC device has not been registered yet. > + return ret; > + > +} > +
Hi Eddie, Please see my comments inline. On Wed, Mar 18, 2015 at 2:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote: > From: Tianping Fang <tianping.fang@mediatek.com> > > Add Mediatek MT6397 RTC driver [snip] > +#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_IRQ_STA_AL (1 << 0) > +#define RTC_IRQ_STA_LP (1 << 3) nit: Could you use BIT() macro for definitions of single bits? (+ further occurrences in the patch) > + > +#define RTC_AL_MASK 0x0008 > +#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 [snip] > + > +static int mtk_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; > +} > + > +static int mtk_rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) > +{ > + u32 addr; > + > + addr = rtc->addr_base + offset; > + > + if (offset < rtc->addr_range) > + return regmap_write(rtc->regmap, addr, data); > + > + return -EINVAL; > +} Do you actually need these wrappers? Could you use regmap_write() and _read() directly? This would also enable you to use regmap_update_bits() instead of implicit read, modify and write. > + > +static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) > +{ > + int ret; > + u32 data; > + > + ret = mtk_rtc_write(rtc, RTC_WRTGR, 1); > + if (ret < 0) > + goto exit; > + > + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); > + if (ret < 0) > + goto exit; > + > + while (data & RTC_BBPU_CBUSY) { > + cpu_relax(); > + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); > + if (ret < 0) > + goto exit; > + } The initial read and the loop could be folded into a do {} while loop? Also it would be safer to have a timeout here. > + > +exit: > + return ret; > +} > + > +static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data) > +{ > + struct mt6397_rtc *rtc = data; > + u32 irqsta, irqen; > + int ret; > + > + ret = mtk_rtc_read(rtc, RTC_IRQ_STA, &irqsta); > + > + if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) { > + rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF); > + irqen = irqsta & ~RTC_IRQ_EN_AL; > + mutex_lock(&rtc->lock); > + if (mtk_rtc_write(rtc, RTC_IRQ_EN, irqen) < 0) > + mtk_rtc_write_trigger(rtc); > + mutex_unlock(&rtc->lock); > + > + return IRQ_HANDLED; > + } > + > + return IRQ_NONE; > +} > + > +static int __mtk_rtc_read_time(struct mt6397_rtc *rtc, > + struct rtc_time *tm, int *sec) > +{ > + int ret; > + > + mutex_lock(&rtc->lock); > + ret = mtk_rtc_read(rtc, RTC_TC_SEC, &tm->tm_sec); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_TC_MIN, &tm->tm_min); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_TC_HOU, &tm->tm_hour); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_TC_DOM, &tm->tm_mday); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_TC_MTH, &tm->tm_mon); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_TC_YEA, &tm->tm_year); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_TC_SEC, sec); Would the hardware allow this to be merged into single burst transfer reading all the registers into a buffer, so then you could just copy the values from that buffer into target struct instead of issuing multiple reads one by one? Also shouldn't the unused bits be masked out? > + > +exit: > + mutex_unlock(&rtc->lock); > + return ret; > +} > + > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + time64_t time; > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + int sec, ret; > + > + do { > + ret = __mtk_rtc_read_time(rtc, tm, &sec); > + if (ret < 0) > + goto exit; > + } while (sec < tm->tm_sec); Shouldn't this be while (sec > tm->tm_sec)? > + > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > + tm->tm_mon--; Could you add a comment explaining why this is decremented? > + time = rtc_tm_to_time64(tm); > + > + tm->tm_wday = (time / 86400 + 4) % 7; Could you add a comment, or even better, an inline function with a comment, explaining this calculation? > + > +exit: > + return ret; > +} > + > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > + int ret; > + > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > + tm->tm_mon++; > + > + mutex_lock(&rtc->lock); > + ret = mtk_rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write_trigger(rtc); > + > +exit: > + mutex_unlock(&rtc->lock); > + return ret; > +} > + > +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); > + u32 irqen, pdn2; > + int ret; > + > + mutex_lock(&rtc->lock); > + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_PDN2, &pdn2); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_AL_SEC, &tm->tm_sec); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_AL_MIN, &tm->tm_min); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_AL_HOU, &tm->tm_hour); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_AL_DOM, &tm->tm_mday); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_AL_MTH, &tm->tm_mon); > + if (ret < 0) > + goto err_exit; > + ret = mtk_rtc_read(rtc, RTC_AL_YEA, &tm->tm_year); > + if (ret < 0) > + goto err_exit; Similarly to _read_time(), could this be changed into a single burst read? > + > + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); > + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); > + mutex_unlock(&rtc->lock); > + > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > + tm->tm_mon--; > + > + return 0; > +err_exit: > + mutex_unlock(&rtc->lock); > + return ret; > +} > + > +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); > + u32 irqen; > + int ret; > + > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > + tm->tm_mon++; > + > + mutex_lock(&rtc->lock); > + if (alm->enabled) { Is this possible that an alarm was already set? Is it okay to keep it enabled while changing the alarm time to new one? > + ret = mtk_rtc_write(rtc, RTC_AL_YEA, tm->tm_year); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_AL_MTH, tm->tm_mon); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_AL_DOM, tm->tm_mday); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_AL_HOU, tm->tm_hour); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_AL_MIN, tm->tm_min); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_write_trigger(rtc); > + if (ret < 0) > + goto exit; > + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); > + if (ret < 0) > + goto exit; > + irqen |= RTC_IRQ_EN_ONESHOT_AL; > + ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen); > + if (ret < 0) > + goto exit; regmap_update_bits() could be used instead of the read, modify and write above. > + ret = mtk_rtc_write_trigger(rtc); > + if (ret < 0) > + goto exit; > + } else { > + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); > + if (ret < 0) > + goto exit; > + irqen &= ~RTC_IRQ_EN_ONESHOT_AL; > + ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen); > + if (ret < 0) > + goto exit; Ditto. > + } > + > +exit: > + mutex_unlock(&rtc->lock); > + return ret; > +} > + > +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 resource *res; > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > + struct mt6397_rtc *rtc; > + int ret = 0; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + rtc->addr_base = res->start; > + rtc->addr_range = res->end - res->start; > + > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start); > + if (rtc->irq <= 0) > + goto out_rtc; Just return an error code here directly. Which one is actually a good question. Looks like existing code is using -EINVAL or -ENXIO. Any ideas? > + > + rtc->regmap = mt6397_chip->regmap; > + rtc->dev = &pdev->dev;q > + mutex_init(&rtc->lock); > + > + platform_set_drvdata(pdev, rtc); > + > + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, > + mtk_rtc_irq_handler_thread, > + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > + "mt6397-rtc", rtc); > + if (ret) { > + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", > + rtc->irq, ret); > + goto out_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); > + } > + > + device_init_wakeup(&pdev->dev, 1); > + > + return 0; > + > +out_rtc: > + rtc_device_unregister(rtc->rtc_dev); All references to this label are actually before rtc_device_register() is even called. The proper thing to do here is to dispose the created IRQ mapping. > + 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); What about the IRQ mapping created in probe? Best regards, Tomasz
Hi Dmitry, On Fri, 2015-03-20 at 22:25 -0700, Dmitry Torokhov wrote: > Hi Eddie, > > > On Tue, Mar 17, 2015 at 10:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote: > > +static int mtk_rtc_probe(struct platform_device *pdev) > > +{ > > + struct resource *res; > > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > > + struct mt6397_rtc *rtc; > > + int ret = 0; > > + > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > > + if (!rtc) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + rtc->addr_base = res->start; > > + rtc->addr_range = res->end - res->start; > > + > > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start); > > + if (rtc->irq <= 0) > > + goto out_rtc; > > + > > + rtc->regmap = mt6397_chip->regmap; > > + rtc->dev = &pdev->dev; > > + mutex_init(&rtc->lock); > > + > > + platform_set_drvdata(pdev, rtc); > > + > > + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, > > + mtk_rtc_irq_handler_thread, > > + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > > + "mt6397-rtc", rtc); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", > > + rtc->irq, ret); > > + goto out_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); > > + } > > + > > + device_init_wakeup(&pdev->dev, 1); > > + > > + return 0; > > + > > +out_rtc: > > + rtc_device_unregister(rtc->rtc_dev); > > This is wrong. Whenever you jump to this label the RTC device has not > been registered yet. Oops, will fix in next round. Eddie
Hi Tomasz, On Mon, 2015-03-30 at 16:41 +0900, Tomasz Figa wrote: > Hi Eddie, > > Please see my comments inline. > > On Wed, Mar 18, 2015 at 2:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote: > > From: Tianping Fang <tianping.fang@mediatek.com> > > > > Add Mediatek MT6397 RTC driver > > [snip] > > > +#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_IRQ_STA_AL (1 << 0) > > +#define RTC_IRQ_STA_LP (1 << 3) > > nit: Could you use BIT() macro for definitions of single bits? (+ > further occurrences in the patch) Will fix it. > > [snip] > > > + > > +static int mtk_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; > > +} > > + > > +static int mtk_rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) > > +{ > > + u32 addr; > > + > > + addr = rtc->addr_base + offset; > > + > > + if (offset < rtc->addr_range) > > + return regmap_write(rtc->regmap, addr, data); > > + > > + return -EINVAL; > > +} > > Do you actually need these wrappers? Could you use regmap_write() and > _read() directly? This would also enable you to use > regmap_update_bits() instead of implicit read, modify and write. These wrappers used to check register range. But I think the check is redundant, I will bypass the check and use regmap API directly. > > > + > > +static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) > > +{ > > + int ret; > > + u32 data; > > + > > + ret = mtk_rtc_write(rtc, RTC_WRTGR, 1); > > + if (ret < 0) > > + goto exit; > > + > > + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); > > + if (ret < 0) > > + goto exit; > > + > > + while (data & RTC_BBPU_CBUSY) { > > + cpu_relax(); > > + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); > > + if (ret < 0) > > + goto exit; > > + } > > The initial read and the loop could be folded into a do {} while loop? > Also it would be safer to have a timeout here. Because I need to check return value, so not put initial read in do { }. Indeed, it is safer to add timeout here. > > + > > +static int __mtk_rtc_read_time(struct mt6397_rtc *rtc, > > + struct rtc_time *tm, int *sec) > > +{ > > + int ret; > > + > > + mutex_lock(&rtc->lock); > > + ret = mtk_rtc_read(rtc, RTC_TC_SEC, &tm->tm_sec); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_TC_MIN, &tm->tm_min); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_TC_HOU, &tm->tm_hour); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_TC_DOM, &tm->tm_mday); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_TC_MTH, &tm->tm_mon); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_TC_YEA, &tm->tm_year); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_TC_SEC, sec); > > Would the hardware allow this to be merged into single burst transfer > reading all the registers into a buffer, so then you could just copy > the values from that buffer into target struct instead of issuing > multiple reads one by one? OK, Sascha already mentioned this before, I think I should change to use single burst reading. > > Also shouldn't the unused bits be masked out? Hardware return zero in unused bits. So I think it not necessary to add mask. > > > + > > +exit: > > + mutex_unlock(&rtc->lock); > > + return ret; > > +} > > + > > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + time64_t time; > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + int sec, ret; > > + > > + do { > > + ret = __mtk_rtc_read_time(rtc, tm, &sec); > > + if (ret < 0) > > + goto exit; > > + } while (sec < tm->tm_sec); > > Shouldn't this be while (sec > tm->tm_sec)? No, it should keep it as is, this is used to check whether second overflow (from 59 to 0). If yes, read time again. > > > + > > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon--; > > Could you add a comment explaining why this is decremented? Year register only have 7bits, use RTC_MIN_YEAR_OFFSET to reduce bit usage. Minus the offset before write to register and add back the offset after read register back. And month register start from 1, but tm_mon start from zero. I will add comment. > > > + time = rtc_tm_to_time64(tm); > > + > > + tm->tm_wday = (time / 86400 + 4) % 7; > > Could you add a comment, or even better, an inline function with a > comment, explaining this calculation? rtc_tm_to_time64 function return time base on 01-01-1970 00:00:00. This base time is Thursday. I will add comment > > > + > > +exit: > > + return ret; > > +} > > + > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); > > + int ret; > > + > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon++; > > + > > + mutex_lock(&rtc->lock); > > + ret = mtk_rtc_write(rtc, RTC_TC_YEA, tm->tm_year); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_TC_MIN, tm->tm_min); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write_trigger(rtc); > > + > > +exit: > > + mutex_unlock(&rtc->lock); > > + return ret; > > +} > > + > > +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); > > + u32 irqen, pdn2; > > + int ret; > > + > > + mutex_lock(&rtc->lock); > > + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_PDN2, &pdn2); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_AL_SEC, &tm->tm_sec); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_AL_MIN, &tm->tm_min); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_AL_HOU, &tm->tm_hour); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_AL_DOM, &tm->tm_mday); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_AL_MTH, &tm->tm_mon); > > + if (ret < 0) > > + goto err_exit; > > + ret = mtk_rtc_read(rtc, RTC_AL_YEA, &tm->tm_year); > > + if (ret < 0) > > + goto err_exit; > > Similarly to _read_time(), could this be changed into a single burst read? will change API. > > > + > > + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); > > + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); > > + mutex_unlock(&rtc->lock); > > + > > + tm->tm_year += RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon--; > > + > > + return 0; > > +err_exit: > > + mutex_unlock(&rtc->lock); > > + return ret; > > +} > > + > > +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); > > + u32 irqen; > > + int ret; > > + > > + tm->tm_year -= RTC_MIN_YEAR_OFFSET; > > + tm->tm_mon++; > > + > > + mutex_lock(&rtc->lock); > > + if (alm->enabled) { > > Is this possible that an alarm was already set? Is it okay to keep it > enabled while changing the alarm time to new one? It's ok because all alarm time register set to hardware after call mtk_rtc_write_trigger. > > > + ret = mtk_rtc_write(rtc, RTC_AL_YEA, tm->tm_year); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_AL_MTH, tm->tm_mon); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_AL_DOM, tm->tm_mday); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_AL_HOU, tm->tm_hour); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_AL_MIN, tm->tm_min); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_write_trigger(rtc); > > + if (ret < 0) > > + goto exit; > > + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); > > + if (ret < 0) > > + goto exit; > > + irqen |= RTC_IRQ_EN_ONESHOT_AL; > > + ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen); > > + if (ret < 0) > > + goto exit; > > regmap_update_bits() could be used instead of the read, modify and write above. I will check how to use this api. > > > + ret = mtk_rtc_write_trigger(rtc); > > + if (ret < 0) > > + goto exit; > > + } else { > > + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); > > + if (ret < 0) > > + goto exit; > > + irqen &= ~RTC_IRQ_EN_ONESHOT_AL; > > + ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen); > > + if (ret < 0) > > + goto exit; > > Ditto. > > > + } > > + > > +exit: > > + mutex_unlock(&rtc->lock); > > + return ret; > > +} > > + > > +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 resource *res; > > + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); > > + struct mt6397_rtc *rtc; > > + int ret = 0; > > + > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); > > + if (!rtc) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + rtc->addr_base = res->start; > > + rtc->addr_range = res->end - res->start; > > + > > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start); > > + if (rtc->irq <= 0) > > + goto out_rtc; > > Just return an error code here directly. Which one is actually a good > question. Looks like existing code is using -EINVAL or -ENXIO. Any > ideas? I tend to use -EINVAL > > > + > > + rtc->regmap = mt6397_chip->regmap; > > + rtc->dev = &pdev->dev;q > > + mutex_init(&rtc->lock); > > + > > + platform_set_drvdata(pdev, rtc); > > + > > + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, > > + mtk_rtc_irq_handler_thread, > > + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > > + "mt6397-rtc", rtc); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", > > + rtc->irq, ret); > > + goto out_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); > > + } > > + > > + device_init_wakeup(&pdev->dev, 1); > > + > > + return 0; > > + > > +out_rtc: > > + rtc_device_unregister(rtc->rtc_dev); > > All references to this label are actually before rtc_device_register() > is even called. The proper thing to do here is to dispose the created > IRQ mapping. OK, will call irq_dispose_mapping and free_irq > > > + 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); > > What about the IRQ mapping created in probe? OK, will call irq_dispose_mapping and free_irq > Eddie
Hi Eddie, Please see my response inline. On Tue, Mar 31, 2015 at 6:44 PM, Eddie Huang <eddie.huang@mediatek.com> wrote: [snip] >> > + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); >> > + if (ret < 0) >> > + goto exit; >> > + >> > + while (data & RTC_BBPU_CBUSY) { >> > + cpu_relax(); >> > + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); >> > + if (ret < 0) >> > + goto exit; >> > + } >> >> The initial read and the loop could be folded into a do {} while loop? >> Also it would be safer to have a timeout here. > Because I need to check return value, so not put initial read in do { }. Hmm, inside the loop you also check return value. Considering the fact that cpu_relax() doesn't do anything interesting besides issuing a memory barrier (and probably could be omitted here) I don't see why this couldn't be made a do {} while loop. (Obviously this is a bit of bikeshedding, but by the way of other changes this could be changed as well.) [snip] >> >> Also shouldn't the unused bits be masked out? > Hardware return zero in unused bits. So I think it not necessary to add > mask. > OK. Thanks for explaining this. >> >> > + >> > +exit: >> > + mutex_unlock(&rtc->lock); >> > + return ret; >> > +} >> > + >> > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) >> > +{ >> > + time64_t time; >> > + struct mt6397_rtc *rtc = dev_get_drvdata(dev); >> > + int sec, ret; >> > + >> > + do { >> > + ret = __mtk_rtc_read_time(rtc, tm, &sec); >> > + if (ret < 0) >> > + goto exit; >> > + } while (sec < tm->tm_sec); >> >> Shouldn't this be while (sec > tm->tm_sec)? > No, it should keep it as is, this is used to check whether second > overflow (from 59 to 0). If yes, read time again. > Ah, right, of course, an overlooking on my side. Thanks for clarifying this. [snip] >> > + mutex_lock(&rtc->lock); >> > + if (alm->enabled) { >> >> Is this possible that an alarm was already set? Is it okay to keep it >> enabled while changing the alarm time to new one? > It's ok because all alarm time register set to hardware after call > mtk_rtc_write_trigger. > Fair enough. Thanks for explanation. Could you maybe add a comment here saying that the new alarm setting will be committed after calling mtk_rtc_write_trigger()? [snip] >> > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> > + rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start); >> > + if (rtc->irq <= 0) >> > + goto out_rtc; >> >> Just return an error code here directly. Which one is actually a good >> question. Looks like existing code is using -EINVAL or -ENXIO. Any >> ideas? > I tend to use -EINVAL SGTM. [snip] >> > + >> > +out_rtc: >> > + rtc_device_unregister(rtc->rtc_dev); >> >> All references to this label are actually before rtc_device_register() >> is even called. The proper thing to do here is to dispose the created >> IRQ mapping. > OK, will call irq_dispose_mapping and free_irq > OK, thanks. Please note that this will also mean changing devm_request_threaded_irq() to normal request_threaded_irq(). Still, now as I think of it, I'm not sure if this driver is the right place to call irq_create_mapping(). Instead, shouldn't the parent MFD driver create the mapping and pass the final virtual IRQ number to this driver through resources? Lee, could you comment on this, please? Best regards, Tomasz
On Mon, 30 Mar 2015 16:15:50 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote: > > > +out_rtc: > > > + rtc_device_unregister(rtc->rtc_dev); > > > > This is wrong. Whenever you jump to this label the RTC device has not > > been registered yet. > > Oops, will fix in next round. Please ensure that Uwe's review comments (starting at https://lkml.org/lkml/2015/3/17/494) have been addressed.
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig index b5b5c3d..dcb94af 100644 --- a/drivers/rtc/Kconfig +++ b/drivers/rtc/Kconfig @@ -1510,6 +1510,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_MT6397 + tristate "Mediatek Real Time Clock driver" + depends on MFD_MT6397 || COMPILE_TEST + help + This selects the Mediatek(R) RTC driver. RTC is part of Mediatek + MT6397 PMIC. You should enable MT6397 PMIC MFD before select + Mediatek(R) RTC driver. + + If you want to use Mediatek(R) RTC interface, select Y or M here. + config RTC_DRV_XGENE tristate "APM X-Gene RTC" depends on HAS_IOMEM diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index 69c8706..85c8bea 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -153,3 +153,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_MT6397) += rtc-mt6397.o diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c new file mode 100644 index 0000000..130480d --- /dev/null +++ b/drivers/rtc/rtc-mt6397.c @@ -0,0 +1,454 @@ +/* +* 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/of_irq.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_IRQ_STA_AL (1 << 0) +#define RTC_IRQ_STA_LP (1 << 3) + +#define RTC_AL_MASK 0x0008 +#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_AL_HOU_MASK 0x001f + +#define RTC_AL_DOM 0x001e +#define RTC_AL_DOM_MASK 0x001f + +#define RTC_AL_MTH 0x0022 +#define RTC_AL_MTH_MASK 0x000f + +#define RTC_AL_YEA 0x0024 +#define RTC_AL_YEA_MASK 0x007f + +#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 int mtk_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; +} + +static int mtk_rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data) +{ + u32 addr; + + addr = rtc->addr_base + offset; + + if (offset < rtc->addr_range) + return regmap_write(rtc->regmap, addr, data); + + return -EINVAL; +} + +static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc) +{ + int ret; + u32 data; + + ret = mtk_rtc_write(rtc, RTC_WRTGR, 1); + if (ret < 0) + goto exit; + + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); + if (ret < 0) + goto exit; + + while (data & RTC_BBPU_CBUSY) { + cpu_relax(); + ret = mtk_rtc_read(rtc, RTC_BBPU, &data); + if (ret < 0) + goto exit; + } + +exit: + return ret; +} + +static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data) +{ + struct mt6397_rtc *rtc = data; + u32 irqsta, irqen; + int ret; + + ret = mtk_rtc_read(rtc, RTC_IRQ_STA, &irqsta); + + if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) { + rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF); + irqen = irqsta & ~RTC_IRQ_EN_AL; + mutex_lock(&rtc->lock); + if (mtk_rtc_write(rtc, RTC_IRQ_EN, irqen) < 0) + mtk_rtc_write_trigger(rtc); + mutex_unlock(&rtc->lock); + + return IRQ_HANDLED; + } + + return IRQ_NONE; +} + +static int __mtk_rtc_read_time(struct mt6397_rtc *rtc, + struct rtc_time *tm, int *sec) +{ + int ret; + + mutex_lock(&rtc->lock); + ret = mtk_rtc_read(rtc, RTC_TC_SEC, &tm->tm_sec); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_TC_MIN, &tm->tm_min); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_TC_HOU, &tm->tm_hour); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_TC_DOM, &tm->tm_mday); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_TC_MTH, &tm->tm_mon); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_TC_YEA, &tm->tm_year); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_TC_SEC, sec); + +exit: + mutex_unlock(&rtc->lock); + return ret; +} + +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm) +{ + time64_t time; + struct mt6397_rtc *rtc = dev_get_drvdata(dev); + int sec, ret; + + do { + ret = __mtk_rtc_read_time(rtc, tm, &sec); + if (ret < 0) + goto exit; + } while (sec < tm->tm_sec); + + tm->tm_year += RTC_MIN_YEAR_OFFSET; + tm->tm_mon--; + time = rtc_tm_to_time64(tm); + + tm->tm_wday = (time / 86400 + 4) % 7; + +exit: + return ret; +} + +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm) +{ + struct mt6397_rtc *rtc = dev_get_drvdata(dev); + int ret; + + tm->tm_year -= RTC_MIN_YEAR_OFFSET; + tm->tm_mon++; + + mutex_lock(&rtc->lock); + ret = mtk_rtc_write(rtc, RTC_TC_YEA, tm->tm_year); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_TC_MTH, tm->tm_mon); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_TC_DOM, tm->tm_mday); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_TC_HOU, tm->tm_hour); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_TC_MIN, tm->tm_min); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_TC_SEC, tm->tm_sec); + if (ret < 0) + goto exit; + ret = mtk_rtc_write_trigger(rtc); + +exit: + mutex_unlock(&rtc->lock); + return ret; +} + +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); + u32 irqen, pdn2; + int ret; + + mutex_lock(&rtc->lock); + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_PDN2, &pdn2); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_AL_SEC, &tm->tm_sec); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_AL_MIN, &tm->tm_min); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_AL_HOU, &tm->tm_hour); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_AL_DOM, &tm->tm_mday); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_AL_MTH, &tm->tm_mon); + if (ret < 0) + goto err_exit; + ret = mtk_rtc_read(rtc, RTC_AL_YEA, &tm->tm_year); + if (ret < 0) + goto err_exit; + + alm->enabled = !!(irqen & RTC_IRQ_EN_AL); + alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM); + mutex_unlock(&rtc->lock); + + tm->tm_year += RTC_MIN_YEAR_OFFSET; + tm->tm_mon--; + + return 0; +err_exit: + mutex_unlock(&rtc->lock); + return ret; +} + +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); + u32 irqen; + int ret; + + tm->tm_year -= RTC_MIN_YEAR_OFFSET; + tm->tm_mon++; + + mutex_lock(&rtc->lock); + if (alm->enabled) { + ret = mtk_rtc_write(rtc, RTC_AL_YEA, tm->tm_year); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_AL_MTH, tm->tm_mon); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_AL_DOM, tm->tm_mday); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_AL_HOU, tm->tm_hour); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_AL_MIN, tm->tm_min); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_AL_SEC, tm->tm_sec); + if (ret < 0) + goto exit; + ret = mtk_rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW); + if (ret < 0) + goto exit; + ret = mtk_rtc_write_trigger(rtc); + if (ret < 0) + goto exit; + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); + if (ret < 0) + goto exit; + irqen |= RTC_IRQ_EN_ONESHOT_AL; + ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen); + if (ret < 0) + goto exit; + ret = mtk_rtc_write_trigger(rtc); + if (ret < 0) + goto exit; + } else { + ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen); + if (ret < 0) + goto exit; + irqen &= ~RTC_IRQ_EN_ONESHOT_AL; + ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen); + if (ret < 0) + goto exit; + } + +exit: + mutex_unlock(&rtc->lock); + return ret; +} + +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 resource *res; + struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent); + struct mt6397_rtc *rtc; + int ret = 0; + + rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL); + if (!rtc) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + rtc->addr_base = res->start; + rtc->addr_range = res->end - res->start; + + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); + rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start); + if (rtc->irq <= 0) + goto out_rtc; + + rtc->regmap = mt6397_chip->regmap; + rtc->dev = &pdev->dev; + mutex_init(&rtc->lock); + + platform_set_drvdata(pdev, rtc); + + ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL, + mtk_rtc_irq_handler_thread, + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, + "mt6397-rtc", rtc); + if (ret) { + dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n", + rtc->irq, ret); + goto out_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); + } + + 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");