[PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Venu Byravarasu]

TPS65910 PMIC is a MFD with RTC as one of the device.
Adding RTC driver for supporting RTC device present
inside TPS65910 PMIC.

Only support for RTC alarm is implemented as part of this patch.

Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
---

 drivers/rtc/Kconfig          |   10 ++
 drivers/rtc/Makefile         |    1 +
 drivers/rtc/rtc-tps65910.c   |  353 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps65910.h |   10 ++
 4 files changed, 374 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-tps65910.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 668da59..82abc6c 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -323,6 +323,16 @@ config RTC_DRV_TWL4030
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-twl.
 
+config RTC_DRV_TPS65910
+	tristate "TI TPS65910 RTC driver"
+	depends on RTC_CLASS && MFD_TPS65910
+	help
+	  If you say yes here you get support for the RTC on the
+	  TPS65910 chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-tps65910.
+
 config RTC_DRV_S35390A
 	tristate "Seiko Instruments S-35390A"
 	select BITREVERSE
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 2973921..9bd5620 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_RTC_DRV_TEGRA)	+= rtc-tegra.o
 obj-$(CONFIG_RTC_DRV_TEST)	+= rtc-test.o
 obj-$(CONFIG_RTC_DRV_TILE)	+= rtc-tile.o
 obj-$(CONFIG_RTC_DRV_TWL4030)	+= rtc-twl.o
+obj-$(CONFIG_RTC_DRV_TPS65910)	+= rtc-tps65910.o
 obj-$(CONFIG_RTC_DRV_TX4939)	+= rtc-tx4939.o
 obj-$(CONFIG_RTC_DRV_V3020)	+= rtc-v3020.o
 obj-$(CONFIG_RTC_DRV_VR41XX)	+= rtc-vr41xx.o
diff --git a/drivers/rtc/rtc-tps65910.c b/drivers/rtc/rtc-tps65910.c
new file mode 100644
index 0000000..74c43c7
--- /dev/null
+++ b/drivers/rtc/rtc-tps65910.c
@@ -0,0 +1,353 @@
+/*
+ * rtc-tps65910.c -- TPS65910 Real Time Clock interface
+ *
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ * Author: Venu Byravarasu <vbyravarasu@nvidia.com>
+ *
+ * Based on original TI driver rtc-twl.c
+ *   Copyright (C) 2007 MontaVista Software, Inc
+ *   Author: Alexandre Rusev <source@mvista.com>
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/rtc.h>
+#include <linux/bcd.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/tps65910.h>
+
+struct tps65910_rtc {
+	struct rtc_device	*rtc;
+	/* To store the list of enabled interrupts */
+	u32 irqstat;
+};
+
+/* Total number of RTC registers needed to set time*/
+#define NUM_TIME_REGS	(TPS65910_YEARS - TPS65910_SECONDS + 1)
+
+static int tps65910_rtc_alarm_irq_enable(struct device *dev, unsigned enabled)
+{
+	struct tps65910 *tps = dev_get_drvdata(dev->parent);
+	u8 val = 0;
+
+	if (enabled)
+		val = TPS65910_RTC_INTERRUPTS_IT_ALARM;
+
+	return regmap_write(tps->regmap, TPS65910_RTC_INTERRUPTS, val);
+}
+
+/*
+ * Gets current tps65910 RTC time and date parameters.
+ *
+ * The RTC's time/alarm representation is not what gmtime(3) requires
+ * Linux to use:
+ *
+ *  - Months are 1..12 vs Linux 0-11
+ *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
+ */
+static int tps65910_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	unsigned char rtc_data[NUM_TIME_REGS];
+	struct tps65910 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	/* Copy RTC counting registers to static registers or latches */
+	ret = regmap_update_bits(tps->regmap, TPS65910_RTC_CTRL,
+		TPS65910_RTC_CTRL_GET_TIME, TPS65910_RTC_CTRL_GET_TIME);
+	if (ret < 0) {
+		dev_err(dev, "RTC CTRL reg update failed with err:%d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_bulk_read(tps->regmap, TPS65910_SECONDS, rtc_data,
+		NUM_TIME_REGS);
+	if (ret < 0) {
+		dev_err(dev, "reading from RTC failed with err:%d\n", ret);
+		return ret;
+	}
+
+	tm->tm_sec = bcd2bin(rtc_data[0]);
+	tm->tm_min = bcd2bin(rtc_data[1]);
+	tm->tm_hour = bcd2bin(rtc_data[2]);
+	tm->tm_mday = bcd2bin(rtc_data[3]);
+	tm->tm_mon = bcd2bin(rtc_data[4]) - 1;
+	tm->tm_year = bcd2bin(rtc_data[5]) + 100;
+
+	return ret;
+}
+
+static int tps65910_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	unsigned char rtc_data[NUM_TIME_REGS];
+	struct tps65910 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	rtc_data[0] = bin2bcd(tm->tm_sec);
+	rtc_data[1] = bin2bcd(tm->tm_min);
+	rtc_data[2] = bin2bcd(tm->tm_hour);
+	rtc_data[3] = bin2bcd(tm->tm_mday);
+	rtc_data[4] = bin2bcd(tm->tm_mon + 1);
+	rtc_data[5] = bin2bcd(tm->tm_year - 100);
+
+	/* Stop RTC while updating the RTC time registers */
+	ret = regmap_update_bits(tps->regmap, TPS65910_RTC_CTRL,
+		TPS65910_RTC_CTRL_STOP_RTC, 0);
+	if (ret < 0) {
+		dev_err(dev, "RTC stop failed with err:%d\n", ret);
+		return ret;
+	}
+
+	/* update all the time registers in one shot */
+	ret = regmap_bulk_write(tps->regmap, TPS65910_SECONDS, rtc_data,
+		NUM_TIME_REGS);
+	if (ret < 0) {
+		dev_err(dev, "rtc_set_time error %d\n", ret);
+		return ret;
+	}
+
+	/* Start back RTC */
+	ret = regmap_update_bits(tps->regmap, TPS65910_RTC_CTRL,
+		TPS65910_RTC_CTRL_STOP_RTC, 1);
+	if (ret < 0)
+		dev_err(dev, "RTC start failed with err:%d\n", ret);
+
+	return ret;
+}
+
+/*
+ * Gets current tps65910 RTC alarm time.
+ */
+static int tps65910_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	unsigned char alarm_data[NUM_TIME_REGS];
+	u32 int_val;
+	struct tps65910 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	ret = regmap_bulk_read(tps->regmap, TPS65910_SECONDS, alarm_data,
+		NUM_TIME_REGS);
+	if (ret < 0) {
+		dev_err(dev, "rtc_read_alarm error %d\n", ret);
+		return ret;
+	}
+
+	alm->time.tm_sec = bcd2bin(alarm_data[0]);
+	alm->time.tm_min = bcd2bin(alarm_data[1]);
+	alm->time.tm_hour = bcd2bin(alarm_data[2]);
+	alm->time.tm_mday = bcd2bin(alarm_data[3]);
+	alm->time.tm_mon = bcd2bin(alarm_data[4]) - 1;
+	alm->time.tm_year = bcd2bin(alarm_data[5]) + 100;
+
+	ret = regmap_read(tps->regmap, TPS65910_RTC_INTERRUPTS, &int_val);
+	if (ret < 0)
+		return ret;
+
+	if (int_val & TPS65910_RTC_INTERRUPTS_IT_ALARM)
+		alm->enabled = 1;
+
+	return ret;
+}
+
+static int tps65910_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	unsigned char alarm_data[NUM_TIME_REGS];
+	struct tps65910 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	ret = tps65910_rtc_alarm_irq_enable(dev, 0);
+	if (ret)
+		return ret;
+
+	alarm_data[0] = bin2bcd(alm->time.tm_sec);
+	alarm_data[1] = bin2bcd(alm->time.tm_min);
+	alarm_data[2] = bin2bcd(alm->time.tm_hour);
+	alarm_data[3] = bin2bcd(alm->time.tm_mday);
+	alarm_data[4] = bin2bcd(alm->time.tm_mon + 1);
+	alarm_data[5] = bin2bcd(alm->time.tm_year - 100);
+
+	/* update all the alarm registers in one shot */
+	ret = regmap_bulk_write(tps->regmap, TPS65910_ALARM_SECONDS,
+		alarm_data, NUM_TIME_REGS);
+	if (ret) {
+		dev_err(dev, "rtc_set_alarm error %d\n", ret);
+		return ret;
+	}
+
+	if (alm->enabled)
+		ret = tps65910_rtc_alarm_irq_enable(dev, 1);
+
+	return ret;
+}
+
+static irqreturn_t tps65910_rtc_interrupt(int irq, void *rtc)
+{
+	struct device *dev = rtc;
+	unsigned long events = 0;
+	struct tps65910 *tps = dev_get_drvdata(dev->parent);
+	struct tps65910_rtc *tps_rtc = dev_get_drvdata(dev);
+	int ret;
+	u32 rtc_reg;
+
+	ret = regmap_read(tps->regmap, TPS65910_RTC_STATUS, &rtc_reg);
+	if (ret)
+		return IRQ_NONE;
+
+	if (rtc_reg & TPS65910_RTC_STATUS_ALARM)
+		events = RTC_IRQF | RTC_AF;
+
+	ret = regmap_write(tps->regmap, TPS65910_RTC_STATUS, rtc_reg);
+	if (ret)
+		return IRQ_NONE;
+
+	/* Notify RTC core on event */
+	rtc_update_irq(tps_rtc->rtc, 1, events);
+
+	return IRQ_HANDLED;
+}
+
+static struct rtc_class_ops tps65910_rtc_ops = {
+	.read_time	= tps65910_rtc_read_time,
+	.set_time	= tps65910_rtc_set_time,
+	.read_alarm	= tps65910_rtc_read_alarm,
+	.set_alarm	= tps65910_rtc_set_alarm,
+	.alarm_irq_enable = tps65910_rtc_alarm_irq_enable,
+};
+
+static int __devinit tps65910_rtc_probe(struct platform_device *pdev)
+{
+	struct tps65910 *tps65910 = NULL;
+	struct tps65910_rtc *tps_rtc = NULL;
+	struct tps65910_board *pmic_plat_data;
+	int ret = -EINVAL;
+	int irq = 0;
+	u32 rtc_reg;
+
+	tps65910 = dev_get_drvdata(pdev->dev.parent);
+
+	tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps65910_rtc),
+			GFP_KERNEL);
+	if (!tps_rtc)
+		return -ENOMEM;
+
+	/* Clear pending interrupts */
+	ret = regmap_read(tps65910->regmap, TPS65910_RTC_STATUS, &rtc_reg);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(tps65910->regmap, TPS65910_RTC_STATUS, rtc_reg);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&pdev->dev, "Enabling tps65910-RTC.\n");
+	rtc_reg = TPS65910_RTC_CTRL_STOP_RTC;
+	ret = regmap_write(tps65910->regmap, TPS65910_RTC_CTRL, rtc_reg);
+	if (ret < 0)
+		return ret;
+
+	pmic_plat_data = dev_get_platdata(tps65910->dev);
+	irq = pmic_plat_data->irq_base;
+	if (irq <= 0) {
+		dev_warn(&pdev->dev, "Wake up is not possible as irq = %d\n",
+			irq);
+		return ret;
+	}
+
+	irq += TPS65910_IRQ_RTC_ALARM;
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+		tps65910_rtc_interrupt, IRQF_TRIGGER_LOW,
+		dev_name(&tps_rtc->rtc->dev), &pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "IRQ is not free.\n");
+		return ret;
+	}
+	device_init_wakeup(&pdev->dev, 1);
+
+	tps_rtc->rtc = rtc_device_register(pdev->name, &pdev->dev,
+		&tps65910_rtc_ops, THIS_MODULE);
+	if (IS_ERR(tps_rtc->rtc)) {
+		ret = PTR_ERR(tps_rtc->rtc);
+		dev_err(&pdev->dev, "RTC device register: err %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, tps_rtc);
+
+	return 0;
+}
+
+/*
+ * Disable all tps65910 RTC module interrupts.
+ * Sets status flag to free.
+ */
+static int __devexit tps65910_rtc_remove(struct platform_device *pdev)
+{
+	/* leave rtc running, but disable irqs */
+	struct rtc_device *rtc = platform_get_drvdata(pdev);
+
+	tps65910_rtc_alarm_irq_enable(&rtc->dev, 0);
+
+	rtc_device_unregister(rtc);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+
+static int tps65910_rtc_suspend(struct platform_device *pdev,
+		pm_message_t state)
+{
+	struct tps65910 *tps = dev_get_drvdata(pdev->dev.parent);
+	u8 alarm = TPS65910_RTC_INTERRUPTS_IT_ALARM;
+	int ret;
+
+	/* Store current list of enabled interrupts*/
+	ret = regmap_read(tps->regmap, TPS65910_RTC_INTERRUPTS,
+		&tps->rtc.irqstat);
+	if (ret < 0)
+		return ret;
+
+	/* Enable RTC ALARM interrupt only */
+	return regmap_write(tps->regmap, TPS65910_RTC_INTERRUPTS, alarm);
+}
+
+static int tps65910_rtc_resume(struct platform_device *pdev)
+{
+	struct tps65910 *tps = dev_get_drvdata(pdev->dev.parent);
+
+	/* Restore list of enabled interrupts before suspend */
+	return regmap_write(tps->regmap, TPS65910_RTC_INTERRUPTS,
+		tps->rtc.irqstat);
+}
+
+static const struct dev_pm_ops tps65910_rtc_pm_ops = {
+	.suspend	= tps65910_rtc_suspend,
+	.resume		= tps65910_rtc_resume,
+};
+
+#define DEV_PM_OPS     (&tps65910_rtc_pm_ops)
+#else
+#define DEV_PM_OPS     NULL
+#endif
+
+static struct platform_driver tps65910rtc_driver = {
+	.probe		= tps65910_rtc_probe,
+	.remove		= __devexit_p(tps65910_rtc_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "tps65910-rtc",
+		.pm	= DEV_PM_OPS,
+	},
+};
+
+module_platform_driver(tps65910rtc_driver);
+MODULE_ALIAS("platform:tps65910_rtc");
+MODULE_AUTHOR("Venu Byravarasu <vbyravarasu@nvidia.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index dd8dc0a..8ab0ab3 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -132,6 +132,16 @@
  *
  */
 
+/* RTC_CTRL_REG bitfields */
+#define TPS65910_RTC_CTRL_STOP_RTC			0x01 /*0=stop, 1=run */
+#define TPS65910_RTC_CTRL_GET_TIME			0x40
+
+/* RTC_STATUS_REG bitfields */
+#define TPS65910_RTC_STATUS_ALARM               0x40
+
+/* RTC_INTERRUPTS_REG bitfields */
+#define TPS65910_RTC_INTERRUPTS_EVERY           0x03
+#define TPS65910_RTC_INTERRUPTS_IT_ALARM        0x08
 
 /*Register BCK1  (0x80) register.RegisterDescription */
 #define BCK1_BCKUP_MASK					0xFF
-- 
1.7.1.1

Re: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Samuel Ortiz]

Hi Venu,

On Thu, Jul 26, 2012 at 12:05:19PM +0530, Venu Byravarasu wrote:
> TPS65910 PMIC is a MFD with RTC as one of the device.
> Adding RTC driver for supporting RTC device present
> inside TPS65910 PMIC.
> 
> Only support for RTC alarm is implemented as part of this patch.
> 
> Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
> ---
> 
>  drivers/rtc/Kconfig          |   10 ++
>  drivers/rtc/Makefile         |    1 +
>  drivers/rtc/rtc-tps65910.c   |  353 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/tps65910.h |   10 ++
>  4 files changed, 374 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/rtc/rtc-tps65910.c
For the MFD parts:

Acked-by: Samuel Ortiz <sameo@linux.intel.com>

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Stephen Boyd]

On 7/25/2012 11:35 PM, Venu Byravarasu wrote:
> +
> +static struct rtc_class_ops tps65910_rtc_ops = {

const?

> +	.read_time	= tps65910_rtc_read_time,
> +	.set_time	= tps65910_rtc_set_time,
> +	.read_alarm	= tps65910_rtc_read_alarm,
> +	.set_alarm	= tps65910_rtc_set_alarm,
> +	.alarm_irq_enable = tps65910_rtc_alarm_irq_enable,
> +};
> +
> +static int __devinit tps65910_rtc_probe(struct platform_device *pdev)
> +{
> +	struct tps65910 *tps65910 = NULL;
> +	struct tps65910_rtc *tps_rtc = NULL;
> +	struct tps65910_board *pmic_plat_data;
> +	int ret = -EINVAL;
> +	int irq = 0;
> +	u32 rtc_reg;

It seems like all the above assignments are useless as they're
overwritten later in this function. Can you remove the assignments?

> +
> +	tps65910 = dev_get_drvdata(pdev->dev.parent);
> +
> +	tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps65910_rtc),
> +			GFP_KERNEL);
> +	if (!tps_rtc)
> +		return -ENOMEM;
> +
> +	/* Clear pending interrupts */
> +	ret = regmap_read(tps65910->regmap, TPS65910_RTC_STATUS, &rtc_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(tps65910->regmap, TPS65910_RTC_STATUS, rtc_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_dbg(&pdev->dev, "Enabling tps65910-RTC.\n");

Hmph, looks more like stopping the RTC.

> +	rtc_reg = TPS65910_RTC_CTRL_STOP_RTC;
> +	ret = regmap_write(tps65910->regmap, TPS65910_RTC_CTRL, rtc_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	pmic_plat_data = dev_get_platdata(tps65910->dev);
> +	irq = pmic_plat_data->irq_base;
> +	if (irq <= 0) {
> +		dev_warn(&pdev->dev, "Wake up is not possible as irq = %d\n",
> +			irq);
> +		return ret;
> +	}
> +
> +	irq += TPS65910_IRQ_RTC_ALARM;
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +		tps65910_rtc_interrupt, IRQF_TRIGGER_LOW,
> +		dev_name(&tps_rtc->rtc->dev), &pdev->dev);

How does this work? It doesn't look like tps_rtc->rtc is assigned until
down there at the rtc_device_register() call.

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "IRQ is not free.\n");
> +		return ret;
> +	}
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	tps_rtc->rtc = rtc_device_register(pdev->name, &pdev->dev,
> +		&tps65910_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(tps_rtc->rtc)) {
> +		ret = PTR_ERR(tps_rtc->rtc);
> +		dev_err(&pdev->dev, "RTC device register: err %d\n", ret);
> +		return ret;
> +	}
> +

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

RE: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Venu Byravarasu]

Thanks Stephen for your comments.
Plz see my comments inline.


> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@codeaurora.org]
> Sent: Monday, July 30, 2012 2:47 PM
> To: Venu Byravarasu
> Cc: a.zummo@towertech.it; sameo@linux.intel.com;
> broonie@opensource.wolfsonmicro.com; Laxman Dewangan;
> kyle.manna@fuel7.com; rtc-linux@googlegroups.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC
> 
> On 7/25/2012 11:35 PM, Venu Byravarasu wrote:
> > +
> > +static struct rtc_class_ops tps65910_rtc_ops = {
> 
> const?

Will add it in my next patch. 

> 
> > +	.read_time	= tps65910_rtc_read_time,
> > +	.set_time	= tps65910_rtc_set_time,
> > +	.read_alarm	= tps65910_rtc_read_alarm,
> > +	.set_alarm	= tps65910_rtc_set_alarm,
> > +	.alarm_irq_enable = tps65910_rtc_alarm_irq_enable,
> > +};
> > +
> > +static int __devinit tps65910_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct tps65910 *tps65910 = NULL;
> > +	struct tps65910_rtc *tps_rtc = NULL;
> > +	struct tps65910_board *pmic_plat_data;
> > +	int ret = -EINVAL;
> > +	int irq = 0;
> > +	u32 rtc_reg;
> 
> It seems like all the above assignments are useless as they're
> overwritten later in this function. Can you remove the assignments?
> 

Some of the non-intelligent compilers/tools complain as variables 
may get used uninitialized. Hence to avoid such complaints, initialized
them to some default values.
What harm do you see if I have local variables initialized during their declaration?

> > +
> > +	tps65910 = dev_get_drvdata(pdev->dev.parent);
> > +
> > +	tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps65910_rtc),
> > +			GFP_KERNEL);
> > +	if (!tps_rtc)
> > +		return -ENOMEM;
> > +
> > +	/* Clear pending interrupts */
> > +	ret = regmap_read(tps65910->regmap, TPS65910_RTC_STATUS,
> &rtc_reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = regmap_write(tps65910->regmap, TPS65910_RTC_STATUS,
> rtc_reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev_dbg(&pdev->dev, "Enabling tps65910-RTC.\n");
> 
> Hmph, looks more like stopping the RTC.
> 

No, the register is a misnomer here.
As per data sheet of TPS65910, setting this bit will start RTC, 
instead of stopping as its name suggests.
 

> > +	rtc_reg = TPS65910_RTC_CTRL_STOP_RTC;
> > +	ret = regmap_write(tps65910->regmap, TPS65910_RTC_CTRL,
> rtc_reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pmic_plat_data = dev_get_platdata(tps65910->dev);
> > +	irq = pmic_plat_data->irq_base;
> > +	if (irq <= 0) {
> > +		dev_warn(&pdev->dev, "Wake up is not possible as irq =
> %d\n",
> > +			irq);
> > +		return ret;
> > +	}
> > +
> > +	irq += TPS65910_IRQ_RTC_ALARM;
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > +		tps65910_rtc_interrupt, IRQF_TRIGGER_LOW,
> > +		dev_name(&tps_rtc->rtc->dev), &pdev->dev);
> 
> How does this work? It doesn't look like tps_rtc->rtc is assigned until
> down there at the rtc_device_register() call.
> 

Somehow this got skipped. Thanks for pointing out.
Will fix and push as part of next patch.
 

> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "IRQ is not free.\n");
> > +		return ret;
> > +	}
> > +	device_init_wakeup(&pdev->dev, 1);
> > +
> > +	tps_rtc->rtc = rtc_device_register(pdev->name, &pdev->dev,
> > +		&tps65910_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(tps_rtc->rtc)) {
> > +		ret = PTR_ERR(tps_rtc->rtc);
> > +		dev_err(&pdev->dev, "RTC device register: err %d\n", ret);
> > +		return ret;
> > +	}
> > +
> 
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum.

Re: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Stephen Boyd]

On 07/30/12 03:07, Venu Byravarasu wrote:
>
> +
> +static int __devinit tps65910_rtc_probe(struct platform_device *pdev)
> +{
> +	struct tps65910 *tps65910 = NULL;
> +	struct tps65910_rtc *tps_rtc = NULL;
> +	struct tps65910_board *pmic_plat_data;
> +	int ret = -EINVAL;
> +	int irq = 0;
> +	u32 rtc_reg;
>> It seems like all the above assignments are useless as they're
>> overwritten later in this function. Can you remove the assignments?
>>
> Some of the non-intelligent compilers/tools complain as variables 
> may get used uninitialized. Hence to avoid such complaints, initialized
> them to some default values.
> What harm do you see if I have local variables initialized during their declaration?

If you return early from a function and forget to give a variable a
value you actually want, you may use it pre-initialized with a bad
value. I would be surprised if a compiler complained about these ones
because they are simple assignments and they aren't under conditional
paths. If there is still a problem, you can use the uninitialized_var()
macro but I don't see why you would need to.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Andrew Morton]

On Thu, 26 Jul 2012 12:05:19 +0530
Venu Byravarasu <vbyravarasu@nvidia.com> wrote:

> TPS65910 PMIC is a MFD with RTC as one of the device.
> Adding RTC driver for supporting RTC device present
> inside TPS65910 PMIC.
> 
> Only support for RTC alarm is implemented as part of this patch.

It needs a build fix:

drivers/rtc/rtc-tps65910.c: In function 'tps65910_rtc_suspend':
drivers/rtc/rtc-tps65910.c:313: error: request for member 'irqstat' in something not a structure or union
drivers/rtc/rtc-tps65910.c: In function 'tps65910_rtc_resume':
drivers/rtc/rtc-tps65910.c:327: error: request for member 'irqstat' in something not a structure or union

--- a/drivers/rtc/rtc-tps65910.c~rtc-tps65910-add-rtc-driver-for-tps65910-pmic-rtc-fix
+++ a/drivers/rtc/rtc-tps65910.c
@@ -310,7 +310,7 @@ static int tps65910_rtc_suspend(struct p
 
 	/* Store current list of enabled interrupts*/
 	ret = regmap_read(tps->regmap, TPS65910_RTC_INTERRUPTS,
-		&tps->rtc.irqstat);
+		&tps->rtc->irqstat);
 	if (ret < 0)
 		return ret;
 
@@ -324,7 +324,7 @@ static int tps65910_rtc_resume(struct pl
 
 	/* Restore list of enabled interrupts before suspend */
 	return regmap_write(tps->regmap, TPS65910_RTC_INTERRUPTS,
-		tps->rtc.irqstat);
+		tps->rtc->irqstat);
 }
 
 static const struct dev_pm_ops tps65910_rtc_pm_ops = {


but it still has problems:

drivers/rtc/rtc-tps65910.c:331: warning: initialization from incompatible pointer type
drivers/rtc/rtc-tps65910.c:332: warning: initialization from incompatible pointer type

fix and resend, please?

RE: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC

[Venu Byravarasu]

Hi Andrew,

Already an updated patch v3 (http://lkml.org/lkml/2012/8/8/221 ) was pushed to fix these issues.
Plz review and merged that.

Thanks,
Venu


> -----Original Message-----
> From: Andrew Morton [mailto:akpm@linux-foundation.org]
> Sent: Wednesday, August 15, 2012 5:22 AM
> To: Venu Byravarasu
> Cc: a.zummo@towertech.it; sameo@linux.intel.com;
> broonie@opensource.wolfsonmicro.com; Laxman Dewangan;
> kyle.manna@fuel7.com; rtc-linux@googlegroups.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] rtc: tps65910: Add RTC driver for TPS65910 PMIC RTC
> 
> On Thu, 26 Jul 2012 12:05:19 +0530
> Venu Byravarasu <vbyravarasu@nvidia.com> wrote:
> 
> > TPS65910 PMIC is a MFD with RTC as one of the device.
> > Adding RTC driver for supporting RTC device present
> > inside TPS65910 PMIC.
> >
> > Only support for RTC alarm is implemented as part of this patch.
> 
> It needs a build fix:
> 
> drivers/rtc/rtc-tps65910.c: In function 'tps65910_rtc_suspend':
> drivers/rtc/rtc-tps65910.c:313: error: request for member 'irqstat' in
> something not a structure or union
> drivers/rtc/rtc-tps65910.c: In function 'tps65910_rtc_resume':
> drivers/rtc/rtc-tps65910.c:327: error: request for member 'irqstat' in
> something not a structure or union
> 
> --- a/drivers/rtc/rtc-tps65910.c~rtc-tps65910-add-rtc-driver-for-tps65910-
> pmic-rtc-fix
> +++ a/drivers/rtc/rtc-tps65910.c
> @@ -310,7 +310,7 @@ static int tps65910_rtc_suspend(struct p
> 
>  	/* Store current list of enabled interrupts*/
>  	ret = regmap_read(tps->regmap, TPS65910_RTC_INTERRUPTS,
> -		&tps->rtc.irqstat);
> +		&tps->rtc->irqstat);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -324,7 +324,7 @@ static int tps65910_rtc_resume(struct pl
> 
>  	/* Restore list of enabled interrupts before suspend */
>  	return regmap_write(tps->regmap, TPS65910_RTC_INTERRUPTS,
> -		tps->rtc.irqstat);
> +		tps->rtc->irqstat);
>  }
> 
>  static const struct dev_pm_ops tps65910_rtc_pm_ops = {
> 
> 
> but it still has problems:
> 
> drivers/rtc/rtc-tps65910.c:331: warning: initialization from incompatible
> pointer type
> drivers/rtc/rtc-tps65910.c:332: warning: initialization from incompatible
> pointer type
> 
> fix and resend, please?