diff mbox

[v2,2/3] rtc: mediatek: Add MT6397 RTC driver

Message ID 1426657537-41414-3-git-send-email-eddie.huang@mediatek.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eddie Huang March 18, 2015, 5:45 a.m. UTC
From: Tianping Fang <tianping.fang@mediatek.com>

Add Mediatek MT6397 RTC driver

Signed-off-by: Tianping Fang <tianping.fang@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 drivers/rtc/Kconfig      |  10 ++
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-mt6397.c | 454 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 465 insertions(+)
 create mode 100644 drivers/rtc/rtc-mt6397.c

Comments

Dmitry Torokhov March 21, 2015, 5:25 a.m. UTC | #1
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;
> +
> +}
> +
Tomasz Figa March 30, 2015, 7:41 a.m. UTC | #2
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
Eddie Huang March 30, 2015, 8:15 a.m. UTC | #3
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
Eddie Huang March 31, 2015, 9:44 a.m. UTC | #4
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
Tomasz Figa March 31, 2015, 11:01 a.m. UTC | #5
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
Andrew Morton March 31, 2015, 10:01 p.m. UTC | #6
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 mbox

Patch

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");