diff mbox

[v2] mfd: tps6586x: add RTC driver for TI TPS6586x

Message ID 4D3E727F.8090105@nvidia.com
State Superseded
Headers show

Commit Message

Varun Wadekar Jan. 25, 2011, 6:49 a.m. UTC
v2: Merged original commits into one and changed
the kconfig to depend on MFD_TPS6586X

Original-Author: Gary King <gking@nvidia.com>
Signed-off-by: Varun Wadekar <vwadekar@nvidia.com>
---
 drivers/rtc/Kconfig          |    6 +
 drivers/rtc/Makefile         |    1 +
 drivers/rtc/rtc-tps6586x.c   |  325
++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps6586x.h |    4 +
 4 files changed, 336 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-tps6586x.c

Comments

Mark Brown Jan. 25, 2011, 2:43 p.m. UTC | #1
On Tue, Jan 25, 2011 at 12:19:35PM +0530, Varun Wadekar wrote:

>  drivers/rtc/rtc-tps6586x.c   |  325
> ++++++++++++++++++++++++++++++++++++++++++

It looks like you still have the line wrapping issues.

> +config RTC_DRV_TPS6586X
> +    tristate "TI TPS6586X RTC"
> +    depends on I2C && MFD_TPS6586X

No need to depend on I2C, the MFD for the device deals with that for
you.

> +struct tps6586x_rtc {
> +    unsigned long     epoch_start;
> +    int          irq;

All the indentation in the driver is broken - you should be using tabs
for indentation.  This may well be the same issue with your MUA that is
causing the line wrapping.

> +static int tps6586x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> +{

> +    if (rtc->irq == -1)
> +        return -EIO;

> +    if (rtc->irq_en && rtc->irq_en && (rtc->irq != -1)) {
> +        disable_irq(rtc->irq);
> +        rtc->irq_en = false;

You're already checking rtc->irq further up, the check for rtc->irq_en
is duplicated.  Probably just need to replace the if with a single check
for rtc->irq_en?

> +    if (alrm->enabled && (rtc->irq != -1)) {

No need to check the IRQ here either.

> +static int tps6586x_rtc_update_irq_enable(struct device *dev,
> +                      unsigned int enabled)
> +{
> +    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
> +
> +    if (rtc->irq == -1)
> +        return -EIO;
> +
> +    enabled = !!enabled;
> +    if (enabled == rtc->irq_en)
> +        return 0;
> +
> +    if (enabled)
> +        enable_irq(rtc->irq);
> +    else
> +        disable_irq(rtc->irq);

This looks to be the same IRQ as is used for alarms above - how do these
two bits of code play together?  The IRQ hander...

> +static irqreturn_t tps6586x_rtc_irq(int irq, void *data)
> +{
> +    struct device *dev = data;
> +    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
> +
> +    rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
> +    return IRQ_HANDLED;

...only appears to handle alarm interrupts.

> +    rtc->epoch_start = mktime(TPS_EPOCH, 1, 1, 0, 0, 0);

Should this be configurable in platform data?

> +    if (pdata && (pdata->irq >= 0)) {
> +        rtc->irq = pdata->irq;
> +        err = request_threaded_irq(pdata->irq, NULL, tps6586x_rtc_irq,
> +                       IRQF_ONESHOT, "tps6586x-rtc",
> +                       &pdev->dev);
> +        if (err) {
> +            dev_warn(&pdev->dev, "unable to request IRQ\n");
> +            rtc->irq = -1;

It'd be nice to print out the interrupt number here in case the issue is
someone misspecified it.

> +MODULE_DESCRIPTION("TI TPS6586x RTC driver");
> +MODULE_AUTHOR("NVIDIA Corporation");
> +MODULE_LICENSE("GPL");

Should really have a MODULE_ALIAS() too.
Varun Wadekar Jan. 31, 2011, 11:12 a.m. UTC | #2
On Tuesday 25 January 2011 08:13 PM, Mark Brown wrote:
> On Tue, Jan 25, 2011 at 12:19:35PM +0530, Varun Wadekar wrote:
>
>>  drivers/rtc/rtc-tps6586x.c   |  325
>> ++++++++++++++++++++++++++++++++++++++++++
> It looks like you still have the line wrapping issues.
>
>> +config RTC_DRV_TPS6586X
>> +    tristate "TI TPS6586X RTC"
>> +    depends on I2C && MFD_TPS6586X
> No need to depend on I2C, the MFD for the device deals with that for
> you.
>
Will do the needful
>> +struct tps6586x_rtc {
>> +    unsigned long     epoch_start;
>> +    int          irq;
> All the indentation in the driver is broken - you should be using tabs
> for indentation.  This may well be the same issue with your MUA that is
> causing the line wrapping.
>
Will do the needful
>> +static int tps6586x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
>> *alrm)
>> +{
>> +    if (rtc->irq == -1)
>> +        return -EIO;
>> +    if (rtc->irq_en && rtc->irq_en && (rtc->irq != -1)) {
>> +        disable_irq(rtc->irq);
>> +        rtc->irq_en = false;
> You're already checking rtc->irq further up, the check for rtc->irq_en
> is duplicated.  Probably just need to replace the if with a single check
> for rtc->irq_en?
>
Will do the needful
>> +    if (alrm->enabled && (rtc->irq != -1)) {
> No need to check the IRQ here either.
>
Will do the needful
>> +static int tps6586x_rtc_update_irq_enable(struct device *dev,
>> +                      unsigned int enabled)
>> +{
>> +    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
>> +
>> +    if (rtc->irq == -1)
>> +        return -EIO;
>> +
>> +    enabled = !!enabled;
>> +    if (enabled == rtc->irq_en)
>> +        return 0;
>> +
>> +    if (enabled)
>> +        enable_irq(rtc->irq);
>> +    else
>> +        disable_irq(rtc->irq);
> This looks to be the same IRQ as is used for alarms above - how do these
> two bits of code play together?  The IRQ hander...
>
>> +static irqreturn_t tps6586x_rtc_irq(int irq, void *data)
>> +{
>> +    struct device *dev = data;
>> +    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
>> +
>> +    rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
>> +    return IRQ_HANDLED;
> ...only appears to handle alarm interrupts.
>
That's right. Since only a single alarm is being used, rtc->irq is
treated as the controlling element.
>> +    rtc->epoch_start = mktime(TPS_EPOCH, 1, 1, 0, 0, 0);
> Should this be configurable in platform data?
>
Will do the needful
>> +    if (pdata && (pdata->irq >= 0)) {
>> +        rtc->irq = pdata->irq;
>> +        err = request_threaded_irq(pdata->irq, NULL, tps6586x_rtc_irq,
>> +                       IRQF_ONESHOT, "tps6586x-rtc",
>> +                       &pdev->dev);
>> +        if (err) {
>> +            dev_warn(&pdev->dev, "unable to request IRQ\n");
>> +            rtc->irq = -1;
> It'd be nice to print out the interrupt number here in case the issue is
> someone misspecified it.
>
Will do the needful
>> +MODULE_DESCRIPTION("TI TPS6586x RTC driver");
>> +MODULE_AUTHOR("NVIDIA Corporation");
>> +MODULE_LICENSE("GPL");
> Should really have a MODULE_ALIAS() too.
Will do the needful

nvpublic
Mark Brown Jan. 31, 2011, 11:18 a.m. UTC | #3
On Mon, Jan 31, 2011 at 04:42:39PM +0530, Varun Wadekar wrote:
> On Tuesday 25 January 2011 08:13 PM, Mark Brown wrote:

> > This looks to be the same IRQ as is used for alarms above - how do these
> > two bits of code play together?  The IRQ hander...

> > ...only appears to handle alarm interrupts.

> That's right. Since only a single alarm is being used, rtc->irq is
> treated as the controlling element.

That's a problem, it means that changing the update IRQ status will
disable alarms too which really isn't what we want.
Varun Wadekar Jan. 31, 2011, 12:13 p.m. UTC | #4
On Monday 31 January 2011 04:48 PM, Mark Brown wrote:
> On Mon, Jan 31, 2011 at 04:42:39PM +0530, Varun Wadekar wrote:
>> On Tuesday 25 January 2011 08:13 PM, Mark Brown wrote:
>>> This looks to be the same IRQ as is used for alarms above - how do these
>>> two bits of code play together?  The IRQ hander...
>>> ...only appears to handle alarm interrupts.
>> That's right. Since only a single alarm is being used, rtc->irq is
>> treated as the controlling element.
> That's a problem, it means that changing the update IRQ status will
> disable alarms too which really isn't what we want.
The right way to fix this would be to add support for all the alarms
provided by tps. Will add that support and post it in my next patch.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Varun Wadekar Jan. 31, 2011, 1:28 p.m. UTC | #5
> The right way to fix this would be to add support for all the alarms
> provided by tps. Will add that support and post it in my next patch.
alarm2 is a lower resolution alarm so ideally we would want to
use only alarm1 for setting alarms. the original issue of disabling
the master irq for disabling the alarm should be handled by
disabling the alarm bit in the RTC_CONFIG register. will add this
change to the original patch instead of unnecessary making the
driver complex.
diff mbox

Patch

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 4941cad..75f8be3 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -297,6 +297,12 @@  config RTC_DRV_DM355EVM
     help
       Supports the RTC firmware in the MSP430 on the DM355 EVM.
 
+config RTC_DRV_TPS6586X
+    tristate "TI TPS6586X RTC"
+    depends on I2C && MFD_TPS6586X
+    help
+      This driver supports TPS6586X RTC
+
 config RTC_DRV_TWL92330
     boolean "TI TWL92330/Menelaus"
     depends on MENELAUS
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 2afdaf3..e8ff797 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -94,6 +94,7 @@  obj-$(CONFIG_RTC_DRV_STK17TA8)    += rtc-stk17ta8.o
 obj-$(CONFIG_RTC_DRV_STMP)    += rtc-stmp3xxx.o
 obj-$(CONFIG_RTC_DRV_SUN4V)    += rtc-sun4v.o
 obj-$(CONFIG_RTC_DRV_TEST)    += rtc-test.o
+obj-$(CONFIG_RTC_DRV_TPS6586X)    += rtc-tps6586x.o
 obj-$(CONFIG_RTC_DRV_TWL4030)    += rtc-twl.o
 obj-$(CONFIG_RTC_DRV_TX4939)    += rtc-tx4939.o
 obj-$(CONFIG_RTC_DRV_V3020)    += rtc-v3020.o
diff --git a/drivers/rtc/rtc-tps6586x.c b/drivers/rtc/rtc-tps6586x.c
new file mode 100644
index 0000000..ca6138b
--- /dev/null
+++ b/drivers/rtc/rtc-tps6586x.c
@@ -0,0 +1,325 @@ 
+/*
+ * drivers/rtc/rtc-tps6586x.c
+ *
+ * RTC driver for TI TPS6586x
+ *
+ * Copyright (c) 2010, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mfd/tps6586x.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
+
+#define TPS_EPOCH    2009
+
+#define RTC_CTRL    0xc0
+#define RTC_ENABLE    (1 << 5)    /* enables tick updates */
+#define RTC_HIRES    (1 << 4)    /* 1Khz or 32Khz updates */
+#define RTC_ALARM1_HI    0xc1
+#define RTC_COUNT4    0xc6
+
+struct tps6586x_rtc {
+    unsigned long     epoch_start;
+    int          irq;
+    bool          irq_en;
+    struct rtc_device *rtc;
+};
+
+static inline struct device *to_tps6586x_dev(struct device *dev)
+{
+    return dev->parent;
+}
+
+static int tps6586x_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
+    struct device *tps_dev = to_tps6586x_dev(dev);
+    unsigned long long ticks = 0;
+    unsigned long seconds;
+    u8 buff[5];
+    int err;
+    int i;
+
+    err = tps6586x_reads(tps_dev, RTC_COUNT4, sizeof(buff), buff);
+    if (err < 0) {
+        dev_err(dev, "failed to read counter\n");
+        return err;
+    }
+
+    for (i = 0; i < sizeof(buff); i++) {
+        ticks <<= 8;
+        ticks |= buff[i];
+    }
+
+    seconds = ticks >> 10;
+
+    seconds += rtc->epoch_start;
+    rtc_time_to_tm(seconds, tm);
+    return rtc_valid_tm(tm);
+}
+
+static int tps6586x_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
+    struct device *tps_dev = to_tps6586x_dev(dev);
+    unsigned long long ticks;
+    unsigned long seconds;
+    u8 buff[5];
+    int err;
+
+    rtc_tm_to_time(tm, &seconds);
+
+    if (WARN_ON(seconds < rtc->epoch_start)) {
+        dev_err(dev, "requested time unsupported\n");
+        return -EINVAL;
+    }
+
+    seconds -= rtc->epoch_start;
+
+    ticks = (unsigned long long)seconds << 10;
+    buff[0] = (ticks >> 32) & 0xff;
+    buff[1] = (ticks >> 24) & 0xff;
+    buff[2] = (ticks >> 16) & 0xff;
+    buff[3] = (ticks >> 8) & 0xff;
+    buff[4] = ticks & 0xff;
+
+    err = tps6586x_clr_bits(tps_dev, RTC_CTRL, RTC_ENABLE);
+    if (err < 0) {
+        dev_err(dev, "failed to clear RTC_ENABLE\n");
+        return err;
+    }
+
+    err = tps6586x_writes(tps_dev, RTC_COUNT4, sizeof(buff), buff);
+    if (err < 0) {
+        dev_err(dev, "failed to program new time\n");
+        return err;
+    }
+
+    err = tps6586x_set_bits(tps_dev, RTC_CTRL, RTC_ENABLE);
+    if (err < 0) {
+        dev_err(dev, "failed to set RTC_ENABLE\n");
+        return err;
+    }
+
+    return 0;
+}
+
+static int tps6586x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
*alrm)
+{
+    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
+    struct device *tps_dev = to_tps6586x_dev(dev);
+    unsigned long seconds;
+    unsigned long ticks;
+    u8 buff[3];
+    int err;
+
+    if (rtc->irq == -1)
+        return -EIO;
+
+    rtc_tm_to_time(&alrm->time, &seconds);
+
+    if (WARN_ON(alrm->enabled && (seconds < rtc->epoch_start))) {
+        dev_err(dev, "can't set alarm to requested time\n");
+        return -EINVAL;
+    }
+
+    if (rtc->irq_en && rtc->irq_en && (rtc->irq != -1)) {
+        disable_irq(rtc->irq);
+        rtc->irq_en = false;
+    }
+
+    seconds -= rtc->epoch_start;
+    ticks = (unsigned long long)seconds << 10;
+
+    buff[0] = (ticks >> 16) & 0xff;
+    buff[1] = (ticks >> 8) & 0xff;
+    buff[2] = ticks & 0xff;
+
+    err = tps6586x_writes(tps_dev, RTC_ALARM1_HI, sizeof(buff), buff);
+    if (err) {
+        dev_err(tps_dev, "unable to program alarm\n");
+        return err;
+    }
+
+    if (alrm->enabled && (rtc->irq != -1)) {
+        enable_irq(rtc->irq);
+        rtc->irq_en = true;
+    }
+
+    return err;
+}
+
+static int tps6586x_rtc_read_alarm(struct device *dev, struct
rtc_wkalrm *alrm)
+{
+    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
+    struct device *tps_dev = to_tps6586x_dev(dev);
+    unsigned long ticks;
+    unsigned long seconds;
+    u8 buff[3];
+    int err;
+
+    err = tps6586x_reads(tps_dev, RTC_ALARM1_HI, sizeof(buff), buff);
+    if (err)
+        return err;
+
+    ticks = (buff[0] << 16) | (buff[1] << 8) | buff[2];
+    seconds = ticks >> 10;
+    seconds += rtc->epoch_start;
+
+    rtc_time_to_tm(seconds, &alrm->time);
+    alrm->enabled = rtc->irq_en;
+
+    return 0;
+}
+
+static int tps6586x_rtc_update_irq_enable(struct device *dev,
+                      unsigned int enabled)
+{
+    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
+
+    if (rtc->irq == -1)
+        return -EIO;
+
+    enabled = !!enabled;
+    if (enabled == rtc->irq_en)
+        return 0;
+
+    if (enabled)
+        enable_irq(rtc->irq);
+    else
+        disable_irq(rtc->irq);
+
+    rtc->irq_en = enabled;
+    return 0;
+}
+
+static const struct rtc_class_ops tps6586x_rtc_ops = {
+    .read_time    = tps6586x_rtc_read_time,
+    .set_time    = tps6586x_rtc_set_time,
+    .set_alarm    = tps6586x_rtc_set_alarm,
+    .read_alarm    = tps6586x_rtc_read_alarm,
+    .update_irq_enable = tps6586x_rtc_update_irq_enable,
+};
+
+static irqreturn_t tps6586x_rtc_irq(int irq, void *data)
+{
+    struct device *dev = data;
+    struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
+
+    rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
+    return IRQ_HANDLED;
+}
+
+static int __devinit tps6586x_rtc_probe(struct platform_device *pdev)
+{
+    struct tps6586x_rtc_platform_data *pdata = pdev->dev.platform_data;
+    struct device *tps_dev = to_tps6586x_dev(&pdev->dev);
+    struct tps6586x_rtc *rtc;
+    int err;
+
+    rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
+
+    if (!rtc)
+        return -ENOMEM;
+
+    rtc->irq = -1;
+    if (!pdata || (pdata->irq < 0))
+        dev_warn(&pdev->dev, "no IRQ specified, wakeup is disabled\n");
+
+    rtc->epoch_start = mktime(TPS_EPOCH, 1, 1, 0, 0, 0);
+
+    rtc->rtc = rtc_device_register("tps6586x-rtc", &pdev->dev,
+                       &tps6586x_rtc_ops, THIS_MODULE);
+
+    if (IS_ERR(rtc->rtc)) {
+        err = PTR_ERR(rtc->rtc);
+        goto fail;
+    }
+
+    /* disable high-res mode, enable tick counting */
+    err = tps6586x_update(tps_dev, RTC_CTRL,
+                  (RTC_ENABLE | RTC_HIRES), RTC_ENABLE);
+    if (err < 0) {
+        dev_err(&pdev->dev, "unable to start counter\n");
+        goto fail;
+    }
+
+    dev_set_drvdata(&pdev->dev, rtc);
+    if (pdata && (pdata->irq >= 0)) {
+        rtc->irq = pdata->irq;
+        err = request_threaded_irq(pdata->irq, NULL, tps6586x_rtc_irq,
+                       IRQF_ONESHOT, "tps6586x-rtc",
+                       &pdev->dev);
+        if (err) {
+            dev_warn(&pdev->dev, "unable to request IRQ\n");
+            rtc->irq = -1;
+        } else {
+            device_init_wakeup(&pdev->dev, 1);
+            disable_irq(rtc->irq);
+            enable_irq_wake(rtc->irq);
+        }
+    }
+
+    return 0;
+
+fail:
+    if (!IS_ERR_OR_NULL(rtc->rtc))
+        rtc_device_unregister(rtc->rtc);
+    kfree(rtc);
+    return err;
+}
+
+static int __devexit tps6586x_rtc_remove(struct platform_device *pdev)
+{
+    struct tps6586x_rtc *rtc = dev_get_drvdata(&pdev->dev);
+
+    if (rtc->irq != -1)
+        free_irq(rtc->irq, rtc);
+    rtc_device_unregister(rtc->rtc);
+    kfree(rtc);
+    return 0;
+}
+
+static struct platform_driver tps6586x_rtc_driver = {
+    .driver    = {
+        .name    = "tps6586x-rtc",
+        .owner    = THIS_MODULE,
+    },
+    .probe    = tps6586x_rtc_probe,
+    .remove    = __devexit_p(tps6586x_rtc_remove),
+};
+
+static int __init tps6586x_rtc_init(void)
+{
+    return platform_driver_register(&tps6586x_rtc_driver);
+}
+module_init(tps6586x_rtc_init);
+
+static void __exit tps6586x_rtc_exit(void)
+{
+    platform_driver_unregister(&tps6586x_rtc_driver);
+}
+module_exit(tps6586x_rtc_exit);
+
+MODULE_DESCRIPTION("TI TPS6586x RTC driver");
+MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index b6bab1b..d96fb3d 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -54,6 +54,10 @@  struct tps6586x_subdev_info {
     void        *platform_data;
 };
 
+struct tps6586x_rtc_platform_data {
+    int irq;
+};
+
 struct tps6586x_platform_data {
     int num_subdevs;
     struct tps6586x_subdev_info *subdevs;