Skip to contentSkip to conversation

drivers: rtc: Add support for Qualcomm PMIC8xxx RTC

PATCH Superseded Anirudh Ghayal 5 participants 8 messages May 5, 2011 – May 26, 2011 1 message not in the archive

Subsystems (3)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 drivers: rtc: Add support for Qualcomm PMIC8xxx RTC

This patch adds support for PMIC8xxx based RTC.
PMIC8xxx is Qualcomm's power management IC that
internally houses an RTC module. This driver
communicates with the PMIC module over SSBI bus.

Cc Wan ZongShun <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
---
 drivers/rtc/Kconfig            |   10 +
 drivers/rtc/Makefile           |    1 +
 drivers/rtc/rtc-pm8xxx.c       |  568 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/pm8xxx/rtc.h |   25 ++
 4 files changed, 604 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-pm8xxx.c
 create mode 100644 include/linux/mfd/pm8xxx/rtc.h
drivers/rtc/Kconfig +10 -0
index e187887..39b7032 100644
@@ -985,6 +985,16 @@ config RTC_DRV_LPC32XX
 	  This driver can also be buillt as a module. If so, the module
 	  will be called rtc-lpc32xx.
 
+config RTC_DRV_PM8XXX
+	tristate "Qualcomm PMIC8XXX RTC"
+	depends on MFD_PM8XXX
+	help
+	  If you say yes here you get support for the
+	  Qualcomm PMIC8XXX RTC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rtc-pm8xxx.
+
 config RTC_DRV_TEGRA
 	tristate "NVIDIA Tegra Internal RTC driver"
 	depends on RTC_CLASS && ARCH_TEGRA
drivers/rtc/Makefile +1 -0
index ca91c3c..40a6e66 100644
@@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123)	+= rtc-pcf2123.o
 obj-$(CONFIG_RTC_DRV_PCF50633)	+= rtc-pcf50633.o
 obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
 obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
+obj-$(CONFIG_RTC_DRV_PM8XXX)    += rtc-pm8xxx.o
 obj-$(CONFIG_RTC_DRV_PS3)	+= rtc-ps3.o
 obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
drivers/rtc/rtc-pm8xxx.c new file +568 -0

568 lines not on this page · raw message

include/linux/mfd/pm8xxx/rtc.h new file +25 -0
new file mode 100644
index 0000000..14f1983
@@ -0,0 +1,25 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#ifndef __RTC_PM8XXX_H__
+#define __RTC_PM8XXX_H__
+
+#define PM8XXX_RTC_DEV_NAME     "rtc-pm8xxx"
+/**
+ * struct pm8xxx_rtc_pdata - RTC driver platform data
+ * @rtc_write_enable: variable stating RTC write capability
+ */
+struct pm8xxx_rtc_platform_data {
+	bool rtc_write_enable;
+};
+
+#endif /* __RTC_PM8XXX_H__ */
Signature · 1 line
Sent by a consultant of the Qualcomm Innovation Center, Inc.\nThe Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#1 lore raw

Hi Peter, Wan,

I have updated the patch with your suggested changes. Could you please re-review this patch.

Thank you,
~Anirudh

Quoted 656 lines · Anirudh Ghayal

This patch adds support for PMIC8xxx based RTC.
PMIC8xxx is Qualcomm's power management IC that
internally houses an RTC module. This driver
communicates with the PMIC module over SSBI bus.

Cc Wan ZongShun<[email protected]>
Cc Andrew Morton<[email protected]>
Cc Lars-Peter Clausen<[email protected]>
---
  drivers/rtc/Kconfig            |   10 +
  drivers/rtc/Makefile           |    1 +
  drivers/rtc/rtc-pm8xxx.c       |  568 ++++++++++++++++++++++++++++++++++++++++
  include/linux/mfd/pm8xxx/rtc.h |   25 ++
  4 files changed, 604 insertions(+), 0 deletions(-)

create mode 100644 drivers/rtc/rtc-pm8xxx.c create mode 100644 include/linux/mfd/pm8xxx/rtc.h

drivers/rtc/Kconfig +10 -0
index e187887..39b7032 100644
@@ -985,6 +985,16 @@ config RTC_DRV_LPC32XX
  	  This driver can also be buillt as a module. If so, the module
  	  will be called rtc-lpc32xx.
 
+config RTC_DRV_PM8XXX
+	tristate "Qualcomm PMIC8XXX RTC"
+	depends on MFD_PM8XXX
+	help
+	  If you say yes here you get support for the
+	  Qualcomm PMIC8XXX RTC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rtc-pm8xxx.
+
  config RTC_DRV_TEGRA
  	tristate "NVIDIA Tegra Internal RTC driver"
  	depends on RTC_CLASS&&  ARCH_TEGRA
drivers/rtc/Makefile +1 -0
index ca91c3c..40a6e66 100644
@@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123)	+= rtc-pcf2123.o
  obj-$(CONFIG_RTC_DRV_PCF50633)	+= rtc-pcf50633.o
  obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
  obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
+obj-$(CONFIG_RTC_DRV_PM8XXX)    += rtc-pm8xxx.o
  obj-$(CONFIG_RTC_DRV_PS3)	+= rtc-ps3.o
  obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
  obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
drivers/rtc/rtc-pm8xxx.c new file +568 -0
new file mode 100644
index 0000000..95e3911
@@ -0,0 +1,568 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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/module.h>
+#include<linux/init.h>
+#include<linux/rtc.h>
+#include<linux/pm.h>
+#include<linux/slab.h>
+#include<linux/spinlock.h>
+
+#include<linux/mfd/pm8xxx/core.h>
+#include<linux/mfd/pm8xxx/rtc.h>
+
+
+/* RTC Register offsets from RTC CTRL REG */
+#define PM8XXX_ALARM_CTRL_OFFSET 0x01
+#define PM8XXX_RTC_WRITE_OFFSET 0x02
+#define PM8XXX_RTC_READ_OFFSET 0x06
+#define PM8XXX_ALARM_RW_OFFSET 0x0A
+
+/* RTC_CTRL register bit fields */
+#define PM8xxx_RTC_ENABLE	BIT(7)
+#define PM8xxx_RTC_ALARM_ENABLE	BIT(1)
+#define PM8xxx_RTC_ALARM_CLEAR  BIT(0)
+
+#define NUM_8_BIT_RTC_REGS	0x4
+
+/**
+ * struct pm8xxx_rtc - rtc driver internal structure
+ * @rtc: rtc device for this driver
+ * @rtc_alarm_irq: rtc alarm irq number
+ */
+struct pm8xxx_rtc {
+	struct rtc_device *rtc;
+	int rtc_alarm_irq;
+	int rtc_base;
+	int rtc_read_base;
+	int rtc_write_base;
+	int alarm_rw_base;
+	u8  ctrl_reg;
+	struct device *rtc_dev;
+	spinlock_t ctrl_reg_lock;
+};
+
+/*
+ * The RTC registers need to be read/written one byte at a time. This is a
+ * hardware limitation.
+ */
+
+static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+			int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i<  count; i++) {
+		rc = pm8xxx_readb(parent, base + i,&rtc_val[i]);
+		if (rc<  0) {
+			dev_err(rtc_dd->rtc_dev, "PM8xxx read failed\n");
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+			int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i<  count; i++) {
+		rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
+		if (rc<  0) {
+			dev_err(rtc_dd->rtc_dev, "PM8xxx write failed\n");
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+
+/*
+ * Steps to write the RTC registers.
+ * 1. Disable alarm if enabled.
+ * 2. Write 0x00 to LSB.
+ * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
+ * 4. Enable alarm if disabled in step 1.
+ */
+static int
+pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	int rc;
+	unsigned long secs, irq_flags;
+	u8 value[4], reg = 0, alarm_enabled = 0, ctrl_reg;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	rtc_tm_to_time(tm,&secs);
+
+	value[0] = secs&  0xFF;
+	value[1] = (secs>>  8)&  0xFF;
+	value[2] = (secs>>  16)&  0xFF;
+	value[3] = (secs>>  24)&  0xFF;
+
+	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	ctrl_reg = rtc_dd->ctrl_reg;
+
+	if (ctrl_reg&  PM8xxx_RTC_ALARM_ENABLE) {
+		alarm_enabled = 1;
+		ctrl_reg&= ~PM8xxx_RTC_ALARM_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc<  0) {
+			dev_err(dev, "PM8xxx write failed\n");
+			goto rtc_rw_fail;
+		}
+	} else
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Write Byte[1], Byte[2], Byte[3], Byte[0] */
+	/* Write 0 to Byte[0] */
+	reg = 0;
+	rc = pm8xxx_write_wrapper(rtc_dd,&reg, rtc_dd->rtc_write_base, 1);
+	if (rc<  0) {
+		dev_err(dev, "PM8xxx write failed\n");
+		goto rtc_rw_fail;
+	}
+
+	/* Write Byte[1], Byte[2], Byte[3] */
+	rc = pm8xxx_write_wrapper(rtc_dd, value + 1,
+					rtc_dd->rtc_write_base + 1, 3);
+	if (rc<  0) {
+		dev_err(dev, "Write to RTC registers failed\n");
+		goto rtc_rw_fail;
+	}
+
+	/* Write Byte[0] */
+	rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->rtc_write_base, 1);
+	if (rc<  0) {
+		dev_err(dev, "Write to RTC register failed\n");
+		goto rtc_rw_fail;
+	}
+
+	if (alarm_enabled) {
+		ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc<  0) {
+			dev_err(dev, "PM8xxx write failed\n");
+			goto rtc_rw_fail;
+		}
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+
+rtc_rw_fail:
+	if (alarm_enabled)
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	return rc;
+}
+
+static int
+pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	int rc;
+	u8 value[4], reg;
+	unsigned long secs;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->rtc_read_base,
+							NUM_8_BIT_RTC_REGS);
+	if (rc<  0) {
+		dev_err(dev, "RTC time read failed\n");
+		return rc;
+	}
+
+	/*
+	 * Read the LSB again and check if there has been a carry over.
+	 * If there is, redo the read operation.
+	 */
+	rc = pm8xxx_read_wrapper(rtc_dd,&reg, rtc_dd->rtc_read_base, 1);
+	if (rc<  0) {
+		dev_err(dev, "PM8xxx read failed\n");
+		return rc;
+	}
+
+	if (unlikely(reg<  value[0])) {
+		rc = pm8xxx_read_wrapper(rtc_dd, value,
+				rtc_dd->rtc_read_base, NUM_8_BIT_RTC_REGS);
+		if (rc<  0) {
+			dev_err(dev, "RTC time read failed\n");
+			return rc;
+		}
+	}
+
+	secs = value[0] | (value[1]<<  8) | (value[2]<<  16) \
+						| (value[3]<<  24);
+
+	rtc_time_to_tm(secs, tm);
+
+	rc = rtc_valid_tm(tm);
+	if (rc<  0) {
+		dev_err(dev, "Invalid time read from PM8xxx\n");
+		return rc;
+	}
+
+	dev_dbg(dev, "secs = %lu, h:m:s == %d:%d:%d, d/m/y = %d/%d/%d\n",
+			secs, tm->tm_hour, tm->tm_min, tm->tm_sec,
+			tm->tm_mday, tm->tm_mon, tm->tm_year);
+
+	return 0;
+}
+
+static int
+pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	int rc;
+	u8 value[4], ctrl_reg;
+	unsigned long secs, secs_rtc, irq_flags;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+	struct rtc_time rtc_tm;
+
+	rtc_tm_to_time(&alarm->time,&secs);
+
+	/*
+	 * Read the current RTC time and verify if the alarm time is in the
+	 * past. If yes, return invalid.
+	 */
+	rc = pm8xxx_rtc_read_time(dev,&rtc_tm);
+	if (rc<  0) {
+		dev_err(dev, "Unamble to read RTC time\n");
+		return -EINVAL;
+	}
+
+	rtc_tm_to_time(&rtc_tm,&secs_rtc);
+	if (secs<  secs_rtc) {
+		dev_err(dev, "Trying to set alarm in the past\n");
+		return -EINVAL;
+	}
+
+	value[0] = secs&  0xFF;
+	value[1] = (secs>>  8)&  0xFF;
+	value[2] = (secs>>  16)&  0xFF;
+	value[3] = (secs>>  24)&  0xFF;
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+							NUM_8_BIT_RTC_REGS);
+	if (rc<  0) {
+		dev_err(dev, "Write to RTC ALARM registers failed\n");
+		goto rtc_rw_fail;
+	}
+
+	ctrl_reg = rtc_dd->ctrl_reg;
+	ctrl_reg = (alarm->enabled) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
+					(ctrl_reg&  ~PM8xxx_RTC_ALARM_ENABLE);
+
+	rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc<  0) {
+		dev_err(dev, "PM8xxx write failed\n");
+		goto rtc_rw_fail;
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+
+	dev_dbg(dev, "Alarm Set for h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+			alarm->time.tm_hour, alarm->time.tm_min,
+			alarm->time.tm_sec, alarm->time.tm_mday,
+			alarm->time.tm_mon, alarm->time.tm_year);
+rtc_rw_fail:
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+	return rc;
+}
+
+static int
+pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	int rc;
+	u8 value[4];
+	unsigned long secs;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+			NUM_8_BIT_RTC_REGS);
+	if (rc<  0) {
+		dev_err(dev, "RTC alarm time read failed\n");
+		return rc;
+	}
+
+	secs = value[0] | (value[1]<<  8) | (value[2]<<  16) | \
+						 (value[3]<<  24);
+
+	rtc_time_to_tm(secs,&alarm->time);
+
+	rc = rtc_valid_tm(&alarm->time);
+	if (rc<  0) {
+		dev_err(dev, "Invalid time read from PM8xxx\n");
+		return rc;
+	}
+
+	dev_dbg(dev, "Alarm set for - h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+		alarm->time.tm_hour, alarm->time.tm_min,
+				alarm->time.tm_sec, alarm->time.tm_mday,
+				alarm->time.tm_mon, alarm->time.tm_year);
+
+	return 0;
+}
+
+
+static int
+pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	int rc;
+	unsigned long irq_flags;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+	u8 ctrl_reg;
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	ctrl_reg = rtc_dd->ctrl_reg;
+	ctrl_reg = (enable) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
+				(ctrl_reg&  ~PM8xxx_RTC_ALARM_ENABLE);
+
+	rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc<  0) {
+		dev_err(dev, "PM8xxx write failed\n");
+		goto rtc_rw_fail;
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+
+rtc_rw_fail:
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+	return rc;
+}
+
+static struct rtc_class_ops pm8xxx_rtc_ops = {
+	.read_time	= pm8xxx_rtc_read_time,
+	.set_alarm	= pm8xxx_rtc_set_alarm,
+	.read_alarm	= pm8xxx_rtc_read_alarm,
+	.alarm_irq_enable = pm8xxx_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_id;
+	u8 ctrl_reg;
+	int rc;
+	unsigned long irq_flags;
+
+	rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Clear the alarm enable bit */
+	ctrl_reg = rtc_dd->ctrl_reg;
+	ctrl_reg&= ~PM8xxx_RTC_ALARM_ENABLE;
+
+	rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc<  0) {
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+		dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+		goto rtc_alarm_handled;
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Clear RTC alarm register */
+	rc = pm8xxx_read_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base +
+						PM8XXX_ALARM_CTRL_OFFSET, 1);
+	if (rc<  0) {
+		dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+		goto rtc_alarm_handled;
+	}
+
+	ctrl_reg&= ~PM8xxx_RTC_ALARM_CLEAR;
+	rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base +
+						PM8XXX_ALARM_CTRL_OFFSET, 1);
+	if (rc<  0)
+		dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+
+rtc_alarm_handled:
+	return IRQ_HANDLED;
+}
+
+static int __devinit pm8xxx_rtc_probe(struct platform_device *pdev)
+{
+	int rc;
+	u8 ctrl_reg;
+	bool rtc_write_enable = false;
+	struct pm8xxx_rtc *rtc_dd;
+	struct resource *rtc_resource;
+	const struct pm8xxx_rtc_platform_data *pdata = mfd_get_data(pdev);
+
+	if (pdata != NULL)
+		rtc_write_enable = pdata->rtc_write_enable;
+
+	rtc_dd = kzalloc(sizeof(*rtc_dd), GFP_KERNEL);
+	if (rtc_dd == NULL) {
+		dev_err(&pdev->dev, "Unable to allocate memory!\n");
+		return -ENOMEM;
+	}
+
+	/* Initialise spinlock to protect RTC cntrol register */
+	spin_lock_init(&rtc_dd->ctrl_reg_lock);
+
+	rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
+	if (rtc_dd->rtc_alarm_irq<  0) {
+		dev_err(&pdev->dev, "Alarm IRQ resource absent!\n");
+		rc = -ENXIO;
+		goto fail_rtc_enable;
+	}
+
+	rtc_resource = platform_get_resource_byname(pdev, IORESOURCE_IO,
+							"pmic_rtc_base");
+	if (!(rtc_resource&&  rtc_resource->start)) {
+		dev_err(&pdev->dev, "RTC IO resource absent!\n");
+		rc = -ENXIO;
+		goto fail_rtc_enable;
+	}
+
+	rtc_dd->rtc_base = rtc_resource->start;
+
+	/* Setup RTC register addresses */
+	rtc_dd->rtc_write_base = rtc_dd->rtc_base + PM8XXX_RTC_WRITE_OFFSET;
+	rtc_dd->rtc_read_base = rtc_dd->rtc_base + PM8XXX_RTC_READ_OFFSET;
+	rtc_dd->alarm_rw_base = rtc_dd->rtc_base + PM8XXX_ALARM_RW_OFFSET;
+
+	rtc_dd->rtc_dev =&(pdev->dev);
+
+	/* Check if the RTC is on, else turn it on */
+	rc = pm8xxx_read_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc<  0) {
+		dev_err(&pdev->dev, "PM8xxx read failed!\n");
+		goto fail_rtc_enable;
+	}
+
+	if (!(ctrl_reg&  PM8xxx_RTC_ENABLE)) {
+		ctrl_reg |= PM8xxx_RTC_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc<  0) {
+			dev_err(&pdev->dev, "PM8xxx write failed!\n");
+			goto fail_rtc_enable;
+		}
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+	if (rtc_write_enable == true)
+		pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
+
+	platform_set_drvdata(pdev, rtc_dd);
+
+	/* Register the RTC device */
+	rtc_dd->rtc = rtc_device_register("pm8xxx_rtc",&pdev->dev,
+				&pm8xxx_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc_dd->rtc)) {
+		dev_err(&pdev->dev, "%s: RTC registration failed (%ld)\n",
+					__func__, PTR_ERR(rtc_dd->rtc));
+		rc = PTR_ERR(rtc_dd->rtc);
+		goto fail_rtc_enable;
+	}
+
+	/* Request the alarm IRQ */
+	rc = request_any_context_irq(rtc_dd->rtc_alarm_irq,
+				 pm8xxx_alarm_trigger, IRQF_TRIGGER_RISING,
+				 "pm8xxx_rtc_alarm", rtc_dd);
+	if (rc<  0) {
+		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", rc);
+		goto fail_req_irq;
+	}
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	dev_dbg(&pdev->dev, "Probe success !!\n");
+
+	return 0;
+
+fail_req_irq:
+	rtc_device_unregister(rtc_dd->rtc);
+fail_rtc_enable:
+	platform_set_drvdata(pdev, NULL);
+	kfree(rtc_dd);
+	return rc;
+}
+
+#ifdef CONFIG_PM
+static int pm8xxx_rtc_resume(struct device *dev)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+	return 0;
+}
+
+static int pm8xxx_rtc_suspend(struct device *dev)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops pm8xxx_rtc_pm_ops = {
+	.suspend = pm8xxx_rtc_suspend,
+	.resume = pm8xxx_rtc_resume,
+};
+#endif
+static int __devexit pm8xxx_rtc_remove(struct platform_device *pdev)
+{
+	struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+	device_init_wakeup(&pdev->dev, 0);
+	free_irq(rtc_dd->rtc_alarm_irq, rtc_dd);
+	rtc_device_unregister(rtc_dd->rtc);
+	platform_set_drvdata(pdev, NULL);
+	kfree(rtc_dd);
+
+	return 0;
+}
+
+static struct platform_driver pm8xxx_rtc_driver = {
+	.probe		= pm8xxx_rtc_probe,
+	.remove		= __devexit_p(pm8xxx_rtc_remove),
+	.driver	= {
+		.name	= PM8XXX_RTC_DEV_NAME,
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	=&pm8xxx_rtc_pm_ops,
+#endif
+	},
+};
+
+static int __init pm8xxx_rtc_init(void)
+{
+	return platform_driver_register(&pm8xxx_rtc_driver);
+}
+module_init(pm8xxx_rtc_init);
+
+static void __exit pm8xxx_rtc_exit(void)
+{
+	platform_driver_unregister(&pm8xxx_rtc_driver);
+}
+module_exit(pm8xxx_rtc_exit);
+
+MODULE_ALIAS("platform:rtc-pm8xxx");
+MODULE_DESCRIPTION("PMIC8xxx RTC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anirudh Ghayal<[email protected]>");
include/linux/mfd/pm8xxx/rtc.h new file +25 -0
new file mode 100644
index 0000000..14f1983
@@ -0,0 +1,25 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#ifndef __RTC_PM8XXX_H__
+#define __RTC_PM8XXX_H__
+
+#define PM8XXX_RTC_DEV_NAME     "rtc-pm8xxx"
+/**
+ * struct pm8xxx_rtc_pdata - RTC driver platform data
+ * @rtc_write_enable: variable stating RTC write capability
+ */
+struct pm8xxx_rtc_platform_data {
+	bool rtc_write_enable;
+};
+
+#endif /* __RTC_PM8XXX_H__ */
Acked-by
#2 lore raw

2011/5/9 Anirudh Ghayal <[email protected]>:

Quoted 4 lines

Hi Peter, Wan,

I have updated the patch with your suggested changes. Could you please re-review this patch.

thanks!

Quoted 684 lines

Thank you,
~Anirudh

Quoted 677 lines · Anirudh Ghayal

This patch adds support for PMIC8xxx based RTC.
PMIC8xxx is Qualcomm's power management IC that
internally houses an RTC module. This driver
communicates with the PMIC module over SSBI bus.

Cc Wan ZongShun<[email protected]>
Cc Andrew Morton<[email protected]>
Cc Lars-Peter Clausen<[email protected]>

---
 drivers/rtc/Kconfig            |   10 +
 drivers/rtc/Makefile           |    1 +
 drivers/rtc/rtc-pm8xxx.c       |  568
++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/pm8xxx/rtc.h |   25 ++

 4 files changed, 604 insertions(+), 0 deletions(-)

 create mode 100644 drivers/rtc/rtc-pm8xxx.c create mode 100644 include/linux/mfd/pm8xxx/rtc.h

drivers/rtc/Kconfig +0 -0
index e187887..39b7032 100644
@@ -985,6 +985,16 @@ config RTC_DRV_LPC32XX

         This driver can also be buillt as a module. If so, the module will be called rtc-lpc32xx.

drivers/rtc/Kconfig +10 -0
+config RTC_DRV_PM8XXX
+       tristate "Qualcomm PMIC8XXX RTC"
+       depends on MFD_PM8XXX
+       help
+         If you say yes here you get support for the
+         Qualcomm PMIC8XXX RTC.
+
+         To compile this driver as a module, choose M here: the
+         module will be called rtc-pm8xxx.
+

 config RTC_DRV_TEGRA
       tristate "NVIDIA Tegra Internal RTC driver" depends on RTC_CLASS&&  ARCH_TEGRA

drivers/rtc/Makefile +0 -0
index ca91c3c..40a6e66 100644
@@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123) += rtc-pcf2123.o

 obj-$(CONFIG_RTC_DRV_PCF50633)        += rtc-pcf50633.o obj-$(CONFIG_RTC_DRV_PL030)   += rtc-pl030.o obj-$(CONFIG_RTC_DRV_PL031)   += rtc-pl031.o +obj-$(CONFIG_RTC_DRV_PM8XXX)    += rtc-pm8xxx.o obj-$(CONFIG_RTC_DRV_PS3)     += rtc-ps3.o obj-$(CONFIG_RTC_DRV_PXA)     += rtc-pxa.o obj-$(CONFIG_RTC_DRV_R9701)   += rtc-r9701.o

drivers/rtc/rtc-pm8xxx.c new file +55 -0
new file mode 100644
index 0000000..95e3911
@@ -0,0 +1,568 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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/module.h>
+#include<linux/init.h>
+#include<linux/rtc.h>
+#include<linux/pm.h>
+#include<linux/slab.h>
+#include<linux/spinlock.h>
+
+#include<linux/mfd/pm8xxx/core.h>
+#include<linux/mfd/pm8xxx/rtc.h>
+
+
+/* RTC Register offsets from RTC CTRL REG */
+#define PM8XXX_ALARM_CTRL_OFFSET 0x01
+#define PM8XXX_RTC_WRITE_OFFSET 0x02
+#define PM8XXX_RTC_READ_OFFSET 0x06
+#define PM8XXX_ALARM_RW_OFFSET 0x0A
+
+/* RTC_CTRL register bit fields */
+#define PM8xxx_RTC_ENABLE      BIT(7)
+#define PM8xxx_RTC_ALARM_ENABLE        BIT(1)
+#define PM8xxx_RTC_ALARM_CLEAR  BIT(0)
+
+#define NUM_8_BIT_RTC_REGS     0x4
+
+/**
+ * struct pm8xxx_rtc - rtc driver internal structure
+ * @rtc: rtc device for this driver
+ * @rtc_alarm_irq: rtc alarm irq number
+ */
+struct pm8xxx_rtc {
+       struct rtc_device *rtc;
+       int rtc_alarm_irq;
+       int rtc_base;
+       int rtc_read_base;
+       int rtc_write_base;
+       int alarm_rw_base;
+       u8  ctrl_reg;
+       struct device *rtc_dev;
+       spinlock_t ctrl_reg_lock;
+};
+
+/*
+ * The RTC registers need to be read/written one byte at a time. This is

a
+ * hardware limitation.
+ */

drivers/rtc/rtc-pm8xxx.c new file +67 -0
+
+static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+                       int base, int count)
+{
+       int i, rc;
+       struct device *parent = rtc_dd->rtc_dev->parent;
+
+       for (i = 0; i<  count; i++) {
+               rc = pm8xxx_readb(parent, base + i,&rtc_val[i]);
+               if (rc<  0) {
+                       dev_err(rtc_dd->rtc_dev, "PM8xxx read failed\n");
+                       return rc;
+               }
+       }
+
+       return 0;
+}
+
+static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+                       int base, int count)
+{
+       int i, rc;
+       struct device *parent = rtc_dd->rtc_dev->parent;
+
+       for (i = 0; i<  count; i++) {
+               rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
+               if (rc<  0) {
+                       dev_err(rtc_dd->rtc_dev, "PM8xxx write failed\n");
+                       return rc;
+               }
+       }
+
+       return 0;
+}
+
+
+/*
+ * Steps to write the RTC registers.
+ * 1. Disable alarm if enabled.
+ * 2. Write 0x00 to LSB.
+ * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
+ * 4. Enable alarm if disabled in step 1.
+ */
+static int
+pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+       int rc;
+       unsigned long secs, irq_flags;
+       u8 value[4], reg = 0, alarm_enabled = 0, ctrl_reg;
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+       rtc_tm_to_time(tm,&secs);
+
+       value[0] = secs&  0xFF;
+       value[1] = (secs>>  8)&  0xFF;
+       value[2] = (secs>>  16)&  0xFF;
+       value[3] = (secs>>  24)&  0xFF;
+
+       dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+
+       spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+       ctrl_reg = rtc_dd->ctrl_reg;
+
+       if (ctrl_reg&  PM8xxx_RTC_ALARM_ENABLE) {
+               alarm_enabled = 1;
+               ctrl_reg&= ~PM8xxx_RTC_ALARM_ENABLE;
+               rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg,

rtc_dd->rtc_base,
+
1);
+               if (rc<  0) {
+                       dev_err(dev, "PM8xxx write failed\n");
+                       goto rtc_rw_fail;
+               }
+       } else
+               spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);

drivers/rtc/rtc-pm8xxx.c new file +20 -0
+
+       /* Write Byte[1], Byte[2], Byte[3], Byte[0] */
+       /* Write 0 to Byte[0] */
+       reg = 0;
+       rc = pm8xxx_write_wrapper(rtc_dd,&reg, rtc_dd->rtc_write_base, 1);
+       if (rc<  0) {
+               dev_err(dev, "PM8xxx write failed\n");
+               goto rtc_rw_fail;
+       }
+
+       /* Write Byte[1], Byte[2], Byte[3] */
+       rc = pm8xxx_write_wrapper(rtc_dd, value + 1,
+                                       rtc_dd->rtc_write_base + 1, 3);
+       if (rc<  0) {
+               dev_err(dev, "Write to RTC registers failed\n");
+               goto rtc_rw_fail;
+       }
+
+       /* Write Byte[0] */
+       rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->rtc_write_base,

1);
+       if (rc<  0) {
+               dev_err(dev, "Write to RTC register failed\n");
+               goto rtc_rw_fail;
+       }
+
+       if (alarm_enabled) {
+               ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
+               rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base,
+
1);
+               if (rc<  0) {
+                       dev_err(dev, "PM8xxx write failed\n");
+                       goto rtc_rw_fail;
+               }
+       }

drivers/rtc/rtc-pm8xxx.c new file +20 -0
+
+       rtc_dd->ctrl_reg = ctrl_reg;
+
+rtc_rw_fail:
+       if (alarm_enabled)
+               spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+       return rc;
+}
+
+static int
+pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+       int rc;
+       u8 value[4], reg;
+       unsigned long secs;
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+       rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->rtc_read_base,
+

NUM_8_BIT_RTC_REGS);
+       if (rc<  0) {
+               dev_err(dev, "RTC time read failed\n");
+               return rc;
+       }

drivers/rtc/rtc-pm8xxx.c new file +14 -0
+
+       /*
+        * Read the LSB again and check if there has been a carry over.
+        * If there is, redo the read operation.
+        */
+       rc = pm8xxx_read_wrapper(rtc_dd,&reg, rtc_dd->rtc_read_base, 1);
+       if (rc<  0) {
+               dev_err(dev, "PM8xxx read failed\n");
+               return rc;
+       }
+
+       if (unlikely(reg<  value[0])) {
+               rc = pm8xxx_read_wrapper(rtc_dd, value,
+                               rtc_dd->rtc_read_base,

NUM_8_BIT_RTC_REGS);
+               if (rc<  0) {
+                       dev_err(dev, "RTC time read failed\n");
+                       return rc;
+               }
+       }

drivers/rtc/rtc-pm8xxx.c new file +32 -0
+
+       secs = value[0] | (value[1]<<  8) | (value[2]<<  16) \
+                                               | (value[3]<<  24);
+
+       rtc_time_to_tm(secs, tm);
+
+       rc = rtc_valid_tm(tm);
+       if (rc<  0) {
+               dev_err(dev, "Invalid time read from PM8xxx\n");
+               return rc;
+       }
+
+       dev_dbg(dev, "secs = %lu, h:m:s == %d:%d:%d, d/m/y = %d/%d/%d\n",
+                       secs, tm->tm_hour, tm->tm_min, tm->tm_sec,
+                       tm->tm_mday, tm->tm_mon, tm->tm_year);
+
+       return 0;
+}
+
+static int
+pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+       int rc;
+       u8 value[4], ctrl_reg;
+       unsigned long secs, secs_rtc, irq_flags;
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+       struct rtc_time rtc_tm;
+
+       rtc_tm_to_time(&alarm->time,&secs);
+
+       /*
+        * Read the current RTC time and verify if the alarm time is in

the
+        * past. If yes, return invalid.
+        */
+       rc = pm8xxx_rtc_read_time(dev,&rtc_tm);
+       if (rc<  0) {
+               dev_err(dev, "Unamble to read RTC time\n");
+               return -EINVAL;
+       }

drivers/rtc/rtc-pm8xxx.c new file +16 -0
+
+       rtc_tm_to_time(&rtc_tm,&secs_rtc);
+       if (secs<  secs_rtc) {
+               dev_err(dev, "Trying to set alarm in the past\n");
+               return -EINVAL;
+       }
+
+       value[0] = secs&  0xFF;
+       value[1] = (secs>>  8)&  0xFF;
+       value[2] = (secs>>  16)&  0xFF;
+       value[3] = (secs>>  24)&  0xFF;
+
+       spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+       rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+

NUM_8_BIT_RTC_REGS);
+       if (rc<  0) {
+               dev_err(dev, "Write to RTC ALARM registers failed\n");
+               goto rtc_rw_fail;
+       }
+
+       ctrl_reg = rtc_dd->ctrl_reg;
+       ctrl_reg = (alarm->enabled) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
+                                       (ctrl_reg& ~PM8xxx_RTC_ALARM_ENABLE);

drivers/rtc/rtc-pm8xxx.c new file +114 -0
+
+       rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+       if (rc<  0) {
+               dev_err(dev, "PM8xxx write failed\n");
+               goto rtc_rw_fail;
+       }
+
+       rtc_dd->ctrl_reg = ctrl_reg;
+
+       dev_dbg(dev, "Alarm Set for h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+                       alarm->time.tm_hour, alarm->time.tm_min,
+                       alarm->time.tm_sec, alarm->time.tm_mday,
+                       alarm->time.tm_mon, alarm->time.tm_year);
+rtc_rw_fail:
+       spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+       return rc;
+}
+
+static int
+pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+       int rc;
+       u8 value[4];
+       unsigned long secs;
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+       rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+                       NUM_8_BIT_RTC_REGS);
+       if (rc<  0) {
+               dev_err(dev, "RTC alarm time read failed\n");
+               return rc;
+       }
+
+       secs = value[0] | (value[1]<<  8) | (value[2]<<  16) | \
+                                                (value[3]<<  24);
+
+       rtc_time_to_tm(secs,&alarm->time);
+
+       rc = rtc_valid_tm(&alarm->time);
+       if (rc<  0) {
+               dev_err(dev, "Invalid time read from PM8xxx\n");
+               return rc;
+       }
+
+       dev_dbg(dev, "Alarm set for - h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+               alarm->time.tm_hour, alarm->time.tm_min,
+                               alarm->time.tm_sec, alarm->time.tm_mday,
+                               alarm->time.tm_mon, alarm->time.tm_year);
+
+       return 0;
+}
+
+
+static int
+pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+       int rc;
+       unsigned long irq_flags;
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+       u8 ctrl_reg;
+
+       spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+       ctrl_reg = rtc_dd->ctrl_reg;
+       ctrl_reg = (enable) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
+                               (ctrl_reg&  ~PM8xxx_RTC_ALARM_ENABLE);
+
+       rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+       if (rc<  0) {
+               dev_err(dev, "PM8xxx write failed\n");
+               goto rtc_rw_fail;
+       }
+
+       rtc_dd->ctrl_reg = ctrl_reg;
+
+rtc_rw_fail:
+       spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+       return rc;
+}
+
+static struct rtc_class_ops pm8xxx_rtc_ops = {
+       .read_time      = pm8xxx_rtc_read_time,
+       .set_alarm      = pm8xxx_rtc_set_alarm,
+       .read_alarm     = pm8xxx_rtc_read_alarm,
+       .alarm_irq_enable = pm8xxx_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
+{
+       struct pm8xxx_rtc *rtc_dd = dev_id;
+       u8 ctrl_reg;
+       int rc;
+       unsigned long irq_flags;
+
+       rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
+
+       spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+       /* Clear the alarm enable bit */
+       ctrl_reg = rtc_dd->ctrl_reg;
+       ctrl_reg&= ~PM8xxx_RTC_ALARM_ENABLE;
+
+       rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+       if (rc<  0) {
+               spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+               dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+               goto rtc_alarm_handled;
+       }
+
+       rtc_dd->ctrl_reg = ctrl_reg;
+       spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+       /* Clear RTC alarm register */
+       rc = pm8xxx_read_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base +
+                                               PM8XXX_ALARM_CTRL_OFFSET,

1);
+       if (rc<  0) {
+               dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+               goto rtc_alarm_handled;
+       }
+
+       ctrl_reg&= ~PM8xxx_RTC_ALARM_CLEAR;
+       rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base +
+                                               PM8XXX_ALARM_CTRL_OFFSET, 1);
+       if (rc<  0)
+               dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");

drivers/rtc/rtc-pm8xxx.c new file +45 -0
+
+rtc_alarm_handled:
+       return IRQ_HANDLED;
+}
+
+static int __devinit pm8xxx_rtc_probe(struct platform_device *pdev)
+{
+       int rc;
+       u8 ctrl_reg;
+       bool rtc_write_enable = false;
+       struct pm8xxx_rtc *rtc_dd;
+       struct resource *rtc_resource;
+       const struct pm8xxx_rtc_platform_data *pdata = mfd_get_data(pdev);
+
+       if (pdata != NULL)
+               rtc_write_enable = pdata->rtc_write_enable;
+
+       rtc_dd = kzalloc(sizeof(*rtc_dd), GFP_KERNEL);
+       if (rtc_dd == NULL) {
+               dev_err(&pdev->dev, "Unable to allocate memory!\n");
+               return -ENOMEM;
+       }
+
+       /* Initialise spinlock to protect RTC cntrol register */
+       spin_lock_init(&rtc_dd->ctrl_reg_lock);
+
+       rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
+       if (rtc_dd->rtc_alarm_irq<  0) {
+               dev_err(&pdev->dev, "Alarm IRQ resource absent!\n");
+               rc = -ENXIO;
+               goto fail_rtc_enable;
+       }
+
+       rtc_resource = platform_get_resource_byname(pdev, IORESOURCE_IO,
+                                                       "pmic_rtc_base");
+       if (!(rtc_resource&&  rtc_resource->start)) {
+               dev_err(&pdev->dev, "RTC IO resource absent!\n");
+               rc = -ENXIO;
+               goto fail_rtc_enable;
+       }
+
+       rtc_dd->rtc_base = rtc_resource->start;
+
+       /* Setup RTC register addresses */
+       rtc_dd->rtc_write_base = rtc_dd->rtc_base +

PM8XXX_RTC_WRITE_OFFSET;
+       rtc_dd->rtc_read_base = rtc_dd->rtc_base + PM8XXX_RTC_READ_OFFSET;
+       rtc_dd->alarm_rw_base = rtc_dd->rtc_base + PM8XXX_ALARM_RW_OFFSET;

drivers/rtc/rtc-pm8xxx.c new file +13 -0
+
+       rtc_dd->rtc_dev =&(pdev->dev);
+
+       /* Check if the RTC is on, else turn it on */
+       rc = pm8xxx_read_wrapper(rtc_dd,&ctrl_reg, rtc_dd->rtc_base, 1);
+       if (rc<  0) {
+               dev_err(&pdev->dev, "PM8xxx read failed!\n");
+               goto fail_rtc_enable;
+       }
+
+       if (!(ctrl_reg&  PM8xxx_RTC_ENABLE)) {
+               ctrl_reg |= PM8xxx_RTC_ENABLE;
+               rc = pm8xxx_write_wrapper(rtc_dd,&ctrl_reg,

rtc_dd->rtc_base,
+
1);
+               if (rc<  0) {
+                       dev_err(&pdev->dev, "PM8xxx write failed!\n");
+                       goto fail_rtc_enable;
+               }
+       }

drivers/rtc/rtc-pm8xxx.c new file +20 -0
+
+       rtc_dd->ctrl_reg = ctrl_reg;
+       if (rtc_write_enable == true)
+               pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
+
+       platform_set_drvdata(pdev, rtc_dd);
+
+       /* Register the RTC device */
+       rtc_dd->rtc = rtc_device_register("pm8xxx_rtc",&pdev->dev,
+                               &pm8xxx_rtc_ops, THIS_MODULE);
+       if (IS_ERR(rtc_dd->rtc)) {
+               dev_err(&pdev->dev, "%s: RTC registration failed (%ld)\n",
+                                       __func__, PTR_ERR(rtc_dd->rtc));
+               rc = PTR_ERR(rtc_dd->rtc);
+               goto fail_rtc_enable;
+       }
+
+       /* Request the alarm IRQ */
+       rc = request_any_context_irq(rtc_dd->rtc_alarm_irq,
+                                pm8xxx_alarm_trigger,

IRQF_TRIGGER_RISING,
+                                "pm8xxx_rtc_alarm", rtc_dd);
+       if (rc<  0) {
+               dev_err(&pdev->dev, "Request IRQ failed (%d)\n", rc);
+               goto fail_req_irq;
+       }

drivers/rtc/rtc-pm8xxx.c new file +82 -0
+
+       device_init_wakeup(&pdev->dev, 1);
+
+       dev_dbg(&pdev->dev, "Probe success !!\n");
+
+       return 0;
+
+fail_req_irq:
+       rtc_device_unregister(rtc_dd->rtc);
+fail_rtc_enable:
+       platform_set_drvdata(pdev, NULL);
+       kfree(rtc_dd);
+       return rc;
+}
+
+#ifdef CONFIG_PM
+static int pm8xxx_rtc_resume(struct device *dev)
+{
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+       if (device_may_wakeup(dev))
+               disable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+       return 0;
+}
+
+static int pm8xxx_rtc_suspend(struct device *dev)
+{
+       struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+       if (device_may_wakeup(dev))
+               enable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+       return 0;
+}
+
+static const struct dev_pm_ops pm8xxx_rtc_pm_ops = {
+       .suspend = pm8xxx_rtc_suspend,
+       .resume = pm8xxx_rtc_resume,
+};
+#endif
+static int __devexit pm8xxx_rtc_remove(struct platform_device *pdev)
+{
+       struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+       device_init_wakeup(&pdev->dev, 0);
+       free_irq(rtc_dd->rtc_alarm_irq, rtc_dd);
+       rtc_device_unregister(rtc_dd->rtc);
+       platform_set_drvdata(pdev, NULL);
+       kfree(rtc_dd);
+
+       return 0;
+}
+
+static struct platform_driver pm8xxx_rtc_driver = {
+       .probe          = pm8xxx_rtc_probe,
+       .remove         = __devexit_p(pm8xxx_rtc_remove),
+       .driver = {
+               .name   = PM8XXX_RTC_DEV_NAME,
+               .owner  = THIS_MODULE,
+#ifdef CONFIG_PM
+               .pm     =&pm8xxx_rtc_pm_ops,
+#endif
+       },
+};
+
+static int __init pm8xxx_rtc_init(void)
+{
+       return platform_driver_register(&pm8xxx_rtc_driver);
+}
+module_init(pm8xxx_rtc_init);
+
+static void __exit pm8xxx_rtc_exit(void)
+{
+       platform_driver_unregister(&pm8xxx_rtc_driver);
+}
+module_exit(pm8xxx_rtc_exit);
+
+MODULE_ALIAS("platform:rtc-pm8xxx");
+MODULE_DESCRIPTION("PMIC8xxx RTC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anirudh Ghayal<[email protected]>");
include/linux/mfd/pm8xxx/rtc.h new file +25 -0
new file mode 100644
index 0000000..14f1983
@@ -0,0 +1,25 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#ifndef __RTC_PM8XXX_H__
+#define __RTC_PM8XXX_H__
+
+#define PM8XXX_RTC_DEV_NAME     "rtc-pm8xxx"
+/**
+ * struct pm8xxx_rtc_pdata - RTC driver platform data
+ * @rtc_write_enable: variable stating RTC write capability
+ */
+struct pm8xxx_rtc_platform_data {
+       bool rtc_write_enable;
+};
+
+#endif /* __RTC_PM8XXX_H__ */
Signature · 2 lines
Wan ZongShun.
www.mcuos.com
#3 lore raw
In reply to Anirudh Ghayal

drivers/rtc/Makefile obj-$(CONFIG_RTC_DRV_PCF2123) += rtc-pcf2123.o · line 77

Quoted 9 lines · Anirudh Ghayal
drivers/rtc/Makefile +1 -0
index ca91c3c..40a6e66 100644
@@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123)	+= rtc-pcf2123.o
 obj-$(CONFIG_RTC_DRV_PCF50633)	+= rtc-pcf50633.o
 obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
 obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
+obj-$(CONFIG_RTC_DRV_PM8XXX)    += rtc-pm8xxx.o

Please use a tab instead of spaces here.

drivers/rtc/Makefile obj-$(CONFIG_RTC_DRV_PCF2123) += rtc-pcf2123.o

Quoted 11 lines
drivers/rtc/Makefile +11 -0
+
+/* RTC Register offsets from RTC CTRL REG */
+#define PM8XXX_ALARM_CTRL_OFFSET 0x01
+#define PM8XXX_RTC_WRITE_OFFSET 0x02
+#define PM8XXX_RTC_READ_OFFSET 0x06
+#define PM8XXX_ALARM_RW_OFFSET 0x0A
+
+/* RTC_CTRL register bit fields */
+#define PM8xxx_RTC_ENABLE	BIT(7)
+#define PM8xxx_RTC_ALARM_ENABLE	BIT(1)
+#define PM8xxx_RTC_ALARM_CLEAR  BIT(0)

Tabs here too I think?

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+
+#define NUM_8_BIT_RTC_REGS	0x4
+
+/**
+ * struct pm8xxx_rtc - rtc driver internal structure
+ * @rtc: rtc device for this driver
+ * @rtc_alarm_irq: rtc alarm irq number
+ */
+struct pm8xxx_rtc {
+	struct rtc_device *rtc;

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+	int rtc_alarm_irq;
+	int rtc_base;
+	int rtc_read_base;
+	int rtc_write_base;
+	int alarm_rw_base;
+	u8  ctrl_reg;
+	struct device *rtc_dev;
+	spinlock_t ctrl_reg_lock;
+};
+

Can you document all fields?

Quoted 5 lines
drivers/rtc/Makefile +5 -0
+/*
+ * The RTC registers need to be read/written one byte at a time. This is a
+ * hardware limitation.
+ */
+

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+			int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i < count; i++) {
+		rc = pm8xxx_readb(parent, base + i, &rtc_val[i]);
+		if (rc < 0) {
+			dev_err(rtc_dd->rtc_dev, "PM8xxx read failed\n");

Please remove these printks in the wrapper functions and put more informative error messages at their call sites.

Quoted 28 lines
drivers/rtc/Makefile +28 -0
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+			int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i < count; i++) {
+		rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
+		if (rc < 0) {
+			dev_err(rtc_dd->rtc_dev, "PM8xxx write failed\n");
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+
+/*
+ * Steps to write the RTC registers.
+ * 1. Disable alarm if enabled.

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+ * 2. Write 0x00 to LSB.
+ * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
+ * 4. Enable alarm if disabled in step 1.
+ */
+static int
+pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	int rc;
+	unsigned long secs, irq_flags;
+	u8 value[4], reg = 0, alarm_enabled = 0, ctrl_reg;

Why doesn't value[] use NUM_8_BIT_RTC_REGS instead of hard-coded 4?

drivers/rtc/Makefile

Quoted 8 lines
drivers/rtc/Makefile +8 -0
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	rtc_tm_to_time(tm, &secs);
+
+	value[0] = secs & 0xFF;
+	value[1] = (secs >> 8) & 0xFF;
+	value[2] = (secs >> 16) & 0xFF;
+	value[3] = (secs >> 24) & 0xFF;

And then this could be a loop

for (int i = 0; i < ARRAY_SIZE(value); i++) {
     value[i] = secs & 0xFF;
     secs >>= 8;
}
Quoted 9 lines
drivers/rtc/Makefile +9 -0
+
+	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	ctrl_reg = rtc_dd->ctrl_reg;
+
+	if (ctrl_reg & PM8xxx_RTC_ALARM_ENABLE) {
+		alarm_enabled = 1;
+		ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc < 0) {
+			dev_err(dev, "PM8xxx write failed\n");
+			goto rtc_rw_fail;
+		}
+	} else
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Write Byte[1], Byte[2], Byte[3], Byte[0] */

This comment looks misplaced. Perhaps it should be after you write an 0x0 to Byte[0]? Or just flat out removed.

Quoted 24 lines
drivers/rtc/Makefile +24 -0
+	/* Write 0 to Byte[0] */
+	reg = 0;
+	rc = pm8xxx_write_wrapper(rtc_dd, &reg, rtc_dd->rtc_write_base, 1);
+	if (rc < 0) {
+		dev_err(dev, "PM8xxx write failed\n");
+		goto rtc_rw_fail;
+	}
+
+	/* Write Byte[1], Byte[2], Byte[3] */
+	rc = pm8xxx_write_wrapper(rtc_dd, value + 1,
+					rtc_dd->rtc_write_base + 1, 3);
+	if (rc < 0) {
+		dev_err(dev, "Write to RTC registers failed\n");
+		goto rtc_rw_fail;
+	}
+
+	/* Write Byte[0] */
+	rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->rtc_write_base, 1);
+	if (rc < 0) {
+		dev_err(dev, "Write to RTC register failed\n");
+		goto rtc_rw_fail;
+	}
+
+	if (alarm_enabled) {

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+		ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc < 0) {
+			dev_err(dev, "PM8xxx write failed\n");
+			goto rtc_rw_fail;
+		}
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;

Hm. This looks wrong since you're modifying the shadow version of ctrl_reg without the lock being held in all cases. Are you sure this is ok?

Quoted 18 lines
drivers/rtc/Makefile +18 -0
+
+rtc_rw_fail:
+	if (alarm_enabled)
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	return rc;
+}
+
+static int
+pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	int rc;
+	u8 value[4], reg;
+	unsigned long secs;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->rtc_read_base,
+							NUM_8_BIT_RTC_REGS);

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+	if (rc < 0) {
+		dev_err(dev, "RTC time read failed\n");
+		return rc;
+	}
+
+	/*
+	 * Read the LSB again and check if there has been a carry over.
+	 * If there is, redo the read operation.
+	 */
+	rc = pm8xxx_read_wrapper(rtc_dd, &reg, rtc_dd->rtc_read_base, 1);

It might be clearer to replace all pm8xxx_write_wrapper() calls with pm8xxx_writeb() when their last argument is 1.

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+	if (rc < 0) {
+		dev_err(dev, "PM8xxx read failed\n");
+		return rc;
+	}
+
+	if (unlikely(reg < value[0])) {

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+		rc = pm8xxx_read_wrapper(rtc_dd, value,
+				rtc_dd->rtc_read_base, NUM_8_BIT_RTC_REGS);
+		if (rc < 0) {
+			dev_err(dev, "RTC time read failed\n");
+			return rc;
+		}
+	}
+
+	secs = value[0] | (value[1] << 8) | (value[2] << 16) \
+						| (value[3] << 24);

The '\' is unnecessary.

Quoted 24 lines
drivers/rtc/Makefile +24 -0
+
+	rtc_time_to_tm(secs, tm);
+
+	rc = rtc_valid_tm(tm);
+	if (rc < 0) {
+		dev_err(dev, "Invalid time read from PM8xxx\n");
+		return rc;
+	}
+
+	dev_dbg(dev, "secs = %lu, h:m:s == %d:%d:%d, d/m/y = %d/%d/%d\n",
+			secs, tm->tm_hour, tm->tm_min, tm->tm_sec,
+			tm->tm_mday, tm->tm_mon, tm->tm_year);
+
+	return 0;
+}
+
+static int
+pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	int rc;
+	u8 value[4], ctrl_reg;
+	unsigned long secs, secs_rtc, irq_flags;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+	struct rtc_time rtc_tm;

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+
+	rtc_tm_to_time(&alarm->time, &secs);
+
+	/*
+	 * Read the current RTC time and verify if the alarm time is in the
+	 * past. If yes, return invalid.
+	 */
+	rc = pm8xxx_rtc_read_time(dev, &rtc_tm);
+	if (rc < 0) {
+		dev_err(dev, "Unamble to read RTC time\n");

s/Unamble/Unable/

drivers/rtc/Makefile

Quoted 8 lines
drivers/rtc/Makefile +8 -0
+		return -EINVAL;
+	}
+
+	rtc_tm_to_time(&rtc_tm, &secs_rtc);
+	if (secs < secs_rtc) {
+		dev_err(dev, "Trying to set alarm in the past\n");
+		return -EINVAL;
+	}

I wonder why this check is here at all. The RTC layer already checks for setting an alarm in the past, right? Why can't we rely on that? If we can't, shouldn't we return -ETIME instead of -EINVAL?

Quoted 7 lines
drivers/rtc/Makefile +7 -0
+
+	value[0] = secs & 0xFF;
+	value[1] = (secs >> 8) & 0xFF;
+	value[2] = (secs >> 16) & 0xFF;
+	value[3] = (secs >> 24) & 0xFF;
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+
+	rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+							NUM_8_BIT_RTC_REGS);
+	if (rc < 0) {
+		dev_err(dev, "Write to RTC ALARM registers failed\n");
+		goto rtc_rw_fail;
+	}
+
+	ctrl_reg = rtc_dd->ctrl_reg;
+	ctrl_reg = (alarm->enabled) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :

Please drop unnecessary () around alarm->enabled.

Quoted 26 lines
drivers/rtc/Makefile +26 -0
+					(ctrl_reg & ~PM8xxx_RTC_ALARM_ENABLE);
+
+	rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc < 0) {
+		dev_err(dev, "PM8xxx write failed\n");
+		goto rtc_rw_fail;
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+
+	dev_dbg(dev, "Alarm Set for h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+			alarm->time.tm_hour, alarm->time.tm_min,
+			alarm->time.tm_sec, alarm->time.tm_mday,
+			alarm->time.tm_mon, alarm->time.tm_year);
+rtc_rw_fail:
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+	return rc;
+}
+
+static int
+pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	int rc;
+	u8 value[4];
+	unsigned long secs;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+
+	rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+			NUM_8_BIT_RTC_REGS);
+	if (rc < 0) {
+		dev_err(dev, "RTC alarm time read failed\n");
+		return rc;
+	}
+
+	secs = value[0] | (value[1] << 8) | (value[2] << 16) | \
+						 (value[3] << 24);

The '\' is unnecessary here too.

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+
+	rtc_time_to_tm(secs, &alarm->time);
+
+	rc = rtc_valid_tm(&alarm->time);
+	if (rc < 0) {
+		dev_err(dev, "Invalid time read from PM8xxx\n");

Do you really need to say PM8xxx in any of these printks? They're already prefixed by "rtc-pm8xxx:".

Quoted 97 lines
drivers/rtc/Makefile +97 -0
+		return rc;
+	}
+
+	dev_dbg(dev, "Alarm set for - h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+		alarm->time.tm_hour, alarm->time.tm_min,
+				alarm->time.tm_sec, alarm->time.tm_mday,
+				alarm->time.tm_mon, alarm->time.tm_year);
+
+	return 0;
+}
+
+
+static int
+pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	int rc;
+	unsigned long irq_flags;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+	u8 ctrl_reg;
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	ctrl_reg = rtc_dd->ctrl_reg;
+	ctrl_reg = (enable) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
+				(ctrl_reg & ~PM8xxx_RTC_ALARM_ENABLE);
+
+	rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc < 0) {
+		dev_err(dev, "PM8xxx write failed\n");
+		goto rtc_rw_fail;
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+
+rtc_rw_fail:
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+	return rc;
+}
+
+static struct rtc_class_ops pm8xxx_rtc_ops = {
+	.read_time	= pm8xxx_rtc_read_time,
+	.set_alarm	= pm8xxx_rtc_set_alarm,
+	.read_alarm	= pm8xxx_rtc_read_alarm,
+	.alarm_irq_enable = pm8xxx_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_id;
+	u8 ctrl_reg;
+	int rc;
+	unsigned long irq_flags;
+
+	rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Clear the alarm enable bit */
+	ctrl_reg = rtc_dd->ctrl_reg;
+	ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
+
+	rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc < 0) {
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+		dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+		goto rtc_alarm_handled;
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Clear RTC alarm register */
+	rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base +
+						PM8XXX_ALARM_CTRL_OFFSET, 1);
+	if (rc < 0) {
+		dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+		goto rtc_alarm_handled;
+	}
+
+	ctrl_reg &= ~PM8xxx_RTC_ALARM_CLEAR;
+	rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base +
+						PM8XXX_ALARM_CTRL_OFFSET, 1);
+	if (rc < 0)
+		dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+
+rtc_alarm_handled:
+	return IRQ_HANDLED;
+}
+
+static int __devinit pm8xxx_rtc_probe(struct platform_device *pdev)
+{
+	int rc;
+	u8 ctrl_reg;
+	bool rtc_write_enable = false;
+	struct pm8xxx_rtc *rtc_dd;
+	struct resource *rtc_resource;
+	const struct pm8xxx_rtc_platform_data *pdata = mfd_get_data(pdev);
+

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+	if (pdata != NULL)
+		rtc_write_enable = pdata->rtc_write_enable;
+
+	rtc_dd = kzalloc(sizeof(*rtc_dd), GFP_KERNEL);
+	if (rtc_dd == NULL) {
+		dev_err(&pdev->dev, "Unable to allocate memory!\n");
+		return -ENOMEM;
+	}
+
+	/* Initialise spinlock to protect RTC cntrol register */

s/cntrol/control/

Quoted 98 lines
drivers/rtc/Makefile +98 -0
+	spin_lock_init(&rtc_dd->ctrl_reg_lock);
+
+	rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
+	if (rtc_dd->rtc_alarm_irq < 0) {
+		dev_err(&pdev->dev, "Alarm IRQ resource absent!\n");
+		rc = -ENXIO;
+		goto fail_rtc_enable;
+	}
+
+	rtc_resource = platform_get_resource_byname(pdev, IORESOURCE_IO,
+							"pmic_rtc_base");
+	if (!(rtc_resource && rtc_resource->start)) {
+		dev_err(&pdev->dev, "RTC IO resource absent!\n");
+		rc = -ENXIO;
+		goto fail_rtc_enable;
+	}
+
+	rtc_dd->rtc_base = rtc_resource->start;
+
+	/* Setup RTC register addresses */
+	rtc_dd->rtc_write_base = rtc_dd->rtc_base + PM8XXX_RTC_WRITE_OFFSET;
+	rtc_dd->rtc_read_base = rtc_dd->rtc_base + PM8XXX_RTC_READ_OFFSET;
+	rtc_dd->alarm_rw_base = rtc_dd->rtc_base + PM8XXX_ALARM_RW_OFFSET;
+
+	rtc_dd->rtc_dev = &(pdev->dev);
+
+	/* Check if the RTC is on, else turn it on */
+	rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "PM8xxx read failed!\n");
+		goto fail_rtc_enable;
+	}
+
+	if (!(ctrl_reg & PM8xxx_RTC_ENABLE)) {
+		ctrl_reg |= PM8xxx_RTC_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc < 0) {
+			dev_err(&pdev->dev, "PM8xxx write failed!\n");
+			goto fail_rtc_enable;
+		}
+	}
+
+	rtc_dd->ctrl_reg = ctrl_reg;
+	if (rtc_write_enable == true)
+		pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
+
+	platform_set_drvdata(pdev, rtc_dd);
+
+	/* Register the RTC device */
+	rtc_dd->rtc = rtc_device_register("pm8xxx_rtc", &pdev->dev,
+				&pm8xxx_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc_dd->rtc)) {
+		dev_err(&pdev->dev, "%s: RTC registration failed (%ld)\n",
+					__func__, PTR_ERR(rtc_dd->rtc));
+		rc = PTR_ERR(rtc_dd->rtc);
+		goto fail_rtc_enable;
+	}
+
+	/* Request the alarm IRQ */
+	rc = request_any_context_irq(rtc_dd->rtc_alarm_irq,
+				 pm8xxx_alarm_trigger, IRQF_TRIGGER_RISING,
+				 "pm8xxx_rtc_alarm", rtc_dd);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", rc);
+		goto fail_req_irq;
+	}
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	dev_dbg(&pdev->dev, "Probe success !!\n");
+
+	return 0;
+
+fail_req_irq:
+	rtc_device_unregister(rtc_dd->rtc);
+fail_rtc_enable:
+	platform_set_drvdata(pdev, NULL);
+	kfree(rtc_dd);
+	return rc;
+}
+
+#ifdef CONFIG_PM
+static int pm8xxx_rtc_resume(struct device *dev)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+	return 0;
+}
+
+static int pm8xxx_rtc_suspend(struct device *dev)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+		enable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops pm8xxx_rtc_pm_ops = {
+	.suspend = pm8xxx_rtc_suspend,
+	.resume = pm8xxx_rtc_resume,
+};
+#endif

Missing a newline here? Also, can you move the probe and remove functions next to each other and move the pm related functions somewhere else (preferably above the probe and remove)?

Quoted 41 lines
drivers/rtc/Makefile +41 -0
+static int __devexit pm8xxx_rtc_remove(struct platform_device *pdev)
+{
+	struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+	device_init_wakeup(&pdev->dev, 0);
+	free_irq(rtc_dd->rtc_alarm_irq, rtc_dd);
+	rtc_device_unregister(rtc_dd->rtc);
+	platform_set_drvdata(pdev, NULL);
+	kfree(rtc_dd);
+
+	return 0;
+}
+
+static struct platform_driver pm8xxx_rtc_driver = {
+	.probe		= pm8xxx_rtc_probe,
+	.remove		= __devexit_p(pm8xxx_rtc_remove),
+	.driver	= {
+		.name	= PM8XXX_RTC_DEV_NAME,
+		.owner	= THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm	= &pm8xxx_rtc_pm_ops,
+#endif
+	},
+};
+
+static int __init pm8xxx_rtc_init(void)
+{
+	return platform_driver_register(&pm8xxx_rtc_driver);
+}
+module_init(pm8xxx_rtc_init);
+
+static void __exit pm8xxx_rtc_exit(void)
+{
+	platform_driver_unregister(&pm8xxx_rtc_driver);
+}
+module_exit(pm8xxx_rtc_exit);
+
+MODULE_ALIAS("platform:rtc-pm8xxx");
+MODULE_DESCRIPTION("PMIC8xxx RTC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anirudh Ghayal <[email protected]>");
Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw

On 5/13/2011 1:04 PM, Ashay Jaiswal wrote:
Hi Stephen,

Thank you for reviewing.

Quoted 18 lines

---------- Forwarded message ----------
From: *Stephen Boyd* <[email protected] <mailto:[email protected]>>
Date: Tue, May 10, 2011 at 11:25 PM
Subject: Re: [PATCH V2] drivers: rtc: Add support for Qualcomm PMIC8xxx RTC
To: Anirudh Ghayal <[email protected] <mailto:[email protected]>>
Cc: Alessandro Zummo <[email protected] <mailto:[email protected]>>, [email protected] <mailto:[email protected]>, [email protected] <mailto:[email protected]>, [email protected] <mailto:[email protected]>, Wan ZongShun <mcuos.com
<http://mcuos.com>@gmail.com <http://gmail.com>>, Andrew Morton <[email protected] <mailto:[email protected]>>, Lars-Peter Clausen <[email protected] <mailto:[email protected]>>

On 5/4/2011 10:48 PM, Anirudh Ghayal wrote:

Quoted 11 lines

drivers/rtc/Makefile obj-$(CONFIG_RTC_DRV_PCF2123) += rtc-pcf2123.o · line 77

Quoted 9 lines
drivers/rtc/Makefile +1 -0
index ca91c3c..40a6e66 100644
@@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123)       += rtc-pcf2123.o
 obj-$(CONFIG_RTC_DRV_PCF50633)       += rtc-pcf50633.o
 obj-$(CONFIG_RTC_DRV_PL030)  += rtc-pl030.o
 obj-$(CONFIG_RTC_DRV_PL031)  += rtc-pl031.o
+obj-$(CONFIG_RTC_DRV_PM8XXX)    += rtc-pm8xxx.o

Please use a tab instead of spaces here.

Ok.

Quoted 14 lines

drivers/rtc/Makefile obj-$(CONFIG_RTC_DRV_PCF2123) += rtc-pcf2123.o

Quoted 12 lines
drivers/rtc/Makefile +11 -0
+
+/* RTC Register offsets from RTC CTRL REG */
+#define PM8XXX_ALARM_CTRL_OFFSET 0x01
+#define PM8XXX_RTC_WRITE_OFFSET 0x02
+#define PM8XXX_RTC_READ_OFFSET 0x06
+#define PM8XXX_ALARM_RW_OFFSET 0x0A
+
+/* RTC_CTRL register bit fields */
+#define PM8xxx_RTC_ENABLE    BIT(7)
+#define PM8xxx_RTC_ALARM_ENABLE      BIT(1)
+#define PM8xxx_RTC_ALARM_CLEAR  BIT(0)

Tabs here too I think?

Ok.

Quoted 15 lines
Quoted 15 lines
drivers/rtc/Makefile +14 -0
+
+#define NUM_8_BIT_RTC_REGS   0x4
+
+/**
+ * struct pm8xxx_rtc - rtc driver internal structure
+ * @rtc: rtc device for this driver
+ * @rtc_alarm_irq: rtc alarm irq number
+ */
+struct pm8xxx_rtc {
+     struct rtc_device *rtc;
+     int rtc_alarm_irq;
+     int rtc_base;
+     int rtc_read_base;
+     int rtc_write_base;
Quoted 8 lines

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+     int alarm_rw_base;
+     u8  ctrl_reg;
+     struct device *rtc_dev;
+     spinlock_t ctrl_reg_lock;
+};
+

Can you document all fields?

Ok, will document all members of structure.

Quoted 4 lines
Quoted 3 lines

+/*
+ * The RTC registers need to be read/written one byte at a time.

This is a

Quoted 16 lines

drivers/rtc/Makefile

Quoted 13 lines

+ * hardware limitation.
+ */

drivers/rtc/Makefile +11 -0
+
+static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+                     int base, int count)
+{
+     int i, rc;
+     struct device *parent = rtc_dd->rtc_dev->parent;
+
+     for (i = 0; i < count; i++) {
+             rc = pm8xxx_readb(parent, base + i, &rtc_val[i]);
+             if (rc < 0) {
+                     dev_err(rtc_dd->rtc_dev, "PM8xxx read failed\n");

Please remove these printks in the wrapper functions and put more informative error messages at their call sites.

Call site function (from pmic driver) only returns error and does not print any information about the failure. It is better to have prints in driver as it can be helpful in debugging rtc issues.

Quoted 33 lines
Quoted 33 lines
drivers/rtc/Makefile +32 -0
+                     return rc;
+             }
+     }
+
+     return 0;
+}
+
+static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+                     int base, int count)
+{
+     int i, rc;
+     struct device *parent = rtc_dd->rtc_dev->parent;
+
+     for (i = 0; i < count; i++) {
+             rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
+             if (rc < 0) {
+                     dev_err(rtc_dd->rtc_dev, "PM8xxx write failed\n");
+                     return rc;
+             }
+     }
+
+     return 0;
+}
+
+
+/*
+ * Steps to write the RTC registers.
+ * 1. Disable alarm if enabled.
+ * 2. Write 0x00 to LSB.
+ * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
+ * 4. Enable alarm if disabled in step 1.
+ */
Quoted 8 lines

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+static int
+pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+     int rc;
+     unsigned long secs, irq_flags;
+     u8 value[4], reg = 0, alarm_enabled = 0, ctrl_reg;

Why doesn't value[] use NUM_8_BIT_RTC_REGS instead of hard-coded 4?

Ok.

Quoted 11 lines

drivers/rtc/Makefile

Quoted 9 lines
drivers/rtc/Makefile +8 -0
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+     rtc_tm_to_time(tm, &secs);
+
+     value[0] = secs & 0xFF;
+     value[1] = (secs >> 8) & 0xFF;
+     value[2] = (secs >> 16) & 0xFF;
+     value[3] = (secs >> 24) & 0xFF;

And then this could be a loop

Yes, this is a better approach. Will do the change.

Quoted 27 lines
for (int i = 0; i < ARRAY_SIZE(value); i++) {
     value[i] = secs & 0xFF;
     secs >>= 8;
}

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+
+     dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+
+     spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+     ctrl_reg = rtc_dd->ctrl_reg;
+
+     if (ctrl_reg & PM8xxx_RTC_ALARM_ENABLE) {
+             alarm_enabled = 1;
+             ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
+             rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,

rtc_dd->rtc_base,

drivers/rtc/Makefile

Quoted 9 lines
drivers/rtc/Makefile +9 -0
+                                                                     1);
+             if (rc < 0) {
+                     dev_err(dev, "PM8xxx write failed\n");
+                     goto rtc_rw_fail;
+             }
+     } else
+             spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+     /* Write Byte[1], Byte[2], Byte[3], Byte[0] */
Quoted 2 lines

This comment looks misplaced. Perhaps it should be after you write an 0x0 to Byte[0]? Or just flat out removed.

Yes, will remove the comment.

Quoted 38 lines
Quoted 9 lines
drivers/rtc/Makefile +8 -0
+     /* Write 0 to Byte[0] */
+     reg = 0;
+     rc = pm8xxx_write_wrapper(rtc_dd, &reg, rtc_dd->rtc_write_base, 1);
+     if (rc < 0) {
+             dev_err(dev, "PM8xxx write failed\n");
+             goto rtc_rw_fail;
+     }
+

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+     /* Write Byte[1], Byte[2], Byte[3] */
+     rc = pm8xxx_write_wrapper(rtc_dd, value + 1,
+                                     rtc_dd->rtc_write_base + 1, 3);
+     if (rc < 0) {
+             dev_err(dev, "Write to RTC registers failed\n");
+             goto rtc_rw_fail;
+     }
+
+     /* Write Byte[0] */
+     rc = pm8xxx_write_wrapper(rtc_dd, value,

rtc_dd->rtc_write_base, 1);

drivers/rtc/Makefile

Quoted 8 lines
drivers/rtc/Makefile +8 -0
+     if (rc < 0) {
+             dev_err(dev, "Write to RTC register failed\n");
+             goto rtc_rw_fail;
+     }
+
+     if (alarm_enabled) {
+             ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
+             rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,

rtc_dd->rtc_base,

drivers/rtc/Makefile

Quoted 8 lines
drivers/rtc/Makefile +8 -0
+                                                                     1);
+             if (rc < 0) {
+                     dev_err(dev, "PM8xxx write failed\n");
+                     goto rtc_rw_fail;
+             }
+     }
+
+     rtc_dd->ctrl_reg = ctrl_reg;
Quoted 2 lines

Hm. This looks wrong since you're modifying the shadow version of ctrl_reg without the lock being held in all cases. Are you sure this is ok?

Yes, this assignment should be done within lock. Will do the change.

Quoted 31 lines
Quoted 9 lines
drivers/rtc/Makefile +8 -0
+
+rtc_rw_fail:
+     if (alarm_enabled)
+             spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+     return rc;
+}
+

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+static int
+pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+     int rc;
+     u8 value[4], reg;
+     unsigned long secs;
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+     rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->rtc_read_base,
+

NUM_8_BIT_RTC_REGS);

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+     if (rc < 0) {
+             dev_err(dev, "RTC time read failed\n");
+             return rc;
+     }
+
+     /*
+      * Read the LSB again and check if there has been a carry over.
+      * If there is, redo the read operation.
+      */
+     rc = pm8xxx_read_wrapper(rtc_dd, &reg, rtc_dd->rtc_read_base, 1);
Quoted 2 lines

It might be clearer to replace all pm8xxx_write_wrapper() calls with pm8xxx_writeb() when their last argument is 1.

pm8xxx_writeb() function needs parent mfd device structure as it's first parameter which requires dereferencing of rtc_dd pointer to get parent device, also keeping common function throughout driver will keep driver consistent in term of readability.

Quoted 11 lines
Quoted 11 lines
drivers/rtc/Makefile +10 -0
+     if (rc < 0) {
+             dev_err(dev, "PM8xxx read failed\n");
+             return rc;
+     }
+
+     if (unlikely(reg < value[0])) {
+             rc = pm8xxx_read_wrapper(rtc_dd, value,
+                             rtc_dd->rtc_read_base, NUM_8_BIT_RTC_REGS);
+             if (rc < 0) {
+                     dev_err(dev, "RTC time read failed\n");
Quoted 8 lines

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+                     return rc;
+             }
+     }
+
+     secs = value[0] | (value[1] << 8) | (value[2] << 16) \
+                                             | (value[3] << 24);

The '\' is unnecessary.

Ok.

Quoted 29 lines
Quoted 29 lines
drivers/rtc/Makefile +28 -0
+
+     rtc_time_to_tm(secs, tm);
+
+     rc = rtc_valid_tm(tm);
+     if (rc < 0) {
+             dev_err(dev, "Invalid time read from PM8xxx\n");
+             return rc;
+     }
+
+     dev_dbg(dev, "secs = %lu, h:m:s == %d:%d:%d, d/m/y = %d/%d/%d\n",
+                     secs, tm->tm_hour, tm->tm_min, tm->tm_sec,
+                     tm->tm_mday, tm->tm_mon, tm->tm_year);
+
+     return 0;
+}
+
+static int
+pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+     int rc;
+     u8 value[4], ctrl_reg;
+     unsigned long secs, secs_rtc, irq_flags;
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+     struct rtc_time rtc_tm;
+
+     rtc_tm_to_time(&alarm->time, &secs);
+
+     /*
Quoted 8 lines

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+      * Read the current RTC time and verify if the alarm time is in the
+      * past. If yes, return invalid.
+      */
+     rc = pm8xxx_rtc_read_time(dev, &rtc_tm);
+     if (rc < 0) {
+             dev_err(dev, "Unamble to read RTC time\n");

s/Unamble/Unable/

Ok.

Quoted 10 lines

drivers/rtc/Makefile

Quoted 9 lines
drivers/rtc/Makefile +8 -0
+             return -EINVAL;
+     }
+
+     rtc_tm_to_time(&rtc_tm, &secs_rtc);
+     if (secs < secs_rtc) {
+             dev_err(dev, "Trying to set alarm in the past\n");
+             return -EINVAL;
+     }
Quoted 3 lines

I wonder why this check is here at all. The RTC layer already checks for setting an alarm in the past, right? Why can't we rely on that? If we can't, shouldn't we return -ETIME instead of -EINVAL?

Yes, RTC layer does this check. Will remove this check from driver.

Quoted 19 lines

drivers/rtc/Makefile

Quoted 11 lines
drivers/rtc/Makefile +10 -0
+
+     value[0] = secs & 0xFF;
+     value[1] = (secs >> 8) & 0xFF;
+     value[2] = (secs >> 16) & 0xFF;
+     value[3] = (secs >> 24) & 0xFF;
+
+     spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+     rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+

NUM_8_BIT_RTC_REGS);

drivers/rtc/Makefile

Quoted 7 lines
drivers/rtc/Makefile +7 -0
+     if (rc < 0) {
+             dev_err(dev, "Write to RTC ALARM registers failed\n");
+             goto rtc_rw_fail;
+     }
+
+     ctrl_reg = rtc_dd->ctrl_reg;
+     ctrl_reg = (alarm->enabled) ? (ctrl_reg |
Quoted 3 lines

PM8xxx_RTC_ALARM_ENABLE) :

Please drop unnecessary () around alarm->enabled.

Ok.

Quoted 32 lines
Quoted 2 lines

+ (ctrl_reg &

~PM8xxx_RTC_ALARM_ENABLE);

Quoted 29 lines
drivers/rtc/Makefile +29 -0
+
+     rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+     if (rc < 0) {
+             dev_err(dev, "PM8xxx write failed\n");
+             goto rtc_rw_fail;
+     }
+
+     rtc_dd->ctrl_reg = ctrl_reg;
+
+     dev_dbg(dev, "Alarm Set for h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+                     alarm->time.tm_hour, alarm->time.tm_min,
+                     alarm->time.tm_sec, alarm->time.tm_mday,
+                     alarm->time.tm_mon, alarm->time.tm_year);
+rtc_rw_fail:
+     spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+     return rc;
+}
+
+static int
+pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+     int rc;
+     u8 value[4];
+     unsigned long secs;
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+     rc = pm8xxx_read_wrapper(rtc_dd, value, rtc_dd->alarm_rw_base,
+                     NUM_8_BIT_RTC_REGS);
+     if (rc < 0) {
Quoted 8 lines

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+             dev_err(dev, "RTC alarm time read failed\n");
+             return rc;
+     }
+
+     secs = value[0] | (value[1] << 8) | (value[2] << 16) | \
+                                              (value[3] << 24);

The '\' is unnecessary here too.

Ok.

Quoted 10 lines

drivers/rtc/Makefile

Quoted 7 lines
drivers/rtc/Makefile +6 -0
+
+     rtc_time_to_tm(secs, &alarm->time);
+
+     rc = rtc_valid_tm(&alarm->time);
+     if (rc < 0) {
+             dev_err(dev, "Invalid time read from PM8xxx\n");

Do you really need to say PM8xxx in any of these printks? They're already prefixed by "rtc-pm8xxx:".

Ok.

Quoted 104 lines
Quoted 64 lines
drivers/rtc/Makefile +63 -0
+             return rc;
+     }
+
+     dev_dbg(dev, "Alarm set for - h:r:s=%d:%d:%d, d/m/y=%d/%d/%d\n",
+             alarm->time.tm_hour, alarm->time.tm_min,
+                             alarm->time.tm_sec, alarm->time.tm_mday,
+                             alarm->time.tm_mon, alarm->time.tm_year);
+
+     return 0;
+}
+
+
+static int
+pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+     int rc;
+     unsigned long irq_flags;
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+     u8 ctrl_reg;
+
+     spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+     ctrl_reg = rtc_dd->ctrl_reg;
+     ctrl_reg = (enable) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
+                             (ctrl_reg & ~PM8xxx_RTC_ALARM_ENABLE);
+
+     rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+     if (rc < 0) {
+             dev_err(dev, "PM8xxx write failed\n");
+             goto rtc_rw_fail;
+     }
+
+     rtc_dd->ctrl_reg = ctrl_reg;
+
+rtc_rw_fail:
+     spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+     return rc;
+}
+
+static struct rtc_class_ops pm8xxx_rtc_ops = {
+     .read_time      = pm8xxx_rtc_read_time,
+     .set_alarm      = pm8xxx_rtc_set_alarm,
+     .read_alarm     = pm8xxx_rtc_read_alarm,
+     .alarm_irq_enable = pm8xxx_rtc_alarm_irq_enable,
+};
+
+static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
+{
+     struct pm8xxx_rtc *rtc_dd = dev_id;
+     u8 ctrl_reg;
+     int rc;
+     unsigned long irq_flags;
+
+     rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
+
+     spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+     /* Clear the alarm enable bit */
+     ctrl_reg = rtc_dd->ctrl_reg;
+     ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
+
+     rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+     if (rc < 0) {
+             spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+             dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+             goto rtc_alarm_handled;
+     }
+
+     rtc_dd->ctrl_reg = ctrl_reg;
+     spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+     /* Clear RTC alarm register */
+     rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base +
+

PM8XXX_ALARM_CTRL_OFFSET, 1);

drivers/rtc/Makefile

Quoted 8 lines
drivers/rtc/Makefile +8 -0
+     if (rc < 0) {
+             dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+             goto rtc_alarm_handled;
+     }
+
+     ctrl_reg &= ~PM8xxx_RTC_ALARM_CLEAR;
+     rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base +
+

PM8XXX_ALARM_CTRL_OFFSET, 1);

Quoted 20 lines
drivers/rtc/Makefile +20 -0
+     if (rc < 0)
+             dev_err(rtc_dd->rtc_dev, "PM8xxx write failed!\n");
+
+rtc_alarm_handled:
+     return IRQ_HANDLED;
+}
+
+static int __devinit pm8xxx_rtc_probe(struct platform_device *pdev)
+{
+     int rc;
+     u8 ctrl_reg;
+     bool rtc_write_enable = false;
+     struct pm8xxx_rtc *rtc_dd;
+     struct resource *rtc_resource;
+     const struct pm8xxx_rtc_platform_data *pdata = mfd_get_data(pdev);
+
+     if (pdata != NULL)
+             rtc_write_enable = pdata->rtc_write_enable;
+
+     rtc_dd = kzalloc(sizeof(*rtc_dd), GFP_KERNEL);
Quoted 8 lines

drivers/rtc/Makefile

Quoted 6 lines
drivers/rtc/Makefile +6 -0
+     if (rtc_dd == NULL) {
+             dev_err(&pdev->dev, "Unable to allocate memory!\n");
+             return -ENOMEM;
+     }
+
+     /* Initialise spinlock to protect RTC cntrol register */

s/cntrol/control/

Ok.

Quoted 107 lines
Quoted 12 lines
drivers/rtc/Makefile +11 -0
+     spin_lock_init(&rtc_dd->ctrl_reg_lock);
+
+     rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
+     if (rtc_dd->rtc_alarm_irq < 0) {
+             dev_err(&pdev->dev, "Alarm IRQ resource absent!\n");
+             rc = -ENXIO;
+             goto fail_rtc_enable;
+     }
+
+     rtc_resource = platform_get_resource_byname(pdev, IORESOURCE_IO,
+ "pmic_rtc_base");

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+     if (!(rtc_resource && rtc_resource->start)) {
+             dev_err(&pdev->dev, "RTC IO resource absent!\n");
+             rc = -ENXIO;
+             goto fail_rtc_enable;
+     }
+
+     rtc_dd->rtc_base = rtc_resource->start;
+
+     /* Setup RTC register addresses */
+     rtc_dd->rtc_write_base = rtc_dd->rtc_base +

PM8XXX_RTC_WRITE_OFFSET;

Quoted 5 lines
drivers/rtc/Makefile +5 -0
+     rtc_dd->rtc_read_base = rtc_dd->rtc_base + PM8XXX_RTC_READ_OFFSET;
+     rtc_dd->alarm_rw_base = rtc_dd->rtc_base + PM8XXX_ALARM_RW_OFFSET;
+
+     rtc_dd->rtc_dev = &(pdev->dev);
+

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+     /* Check if the RTC is on, else turn it on */
+     rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+     if (rc < 0) {
+             dev_err(&pdev->dev, "PM8xxx read failed!\n");
+             goto fail_rtc_enable;
+     }
+
+     if (!(ctrl_reg & PM8xxx_RTC_ENABLE)) {
+             ctrl_reg |= PM8xxx_RTC_ENABLE;
+             rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,

rtc_dd->rtc_base,

Quoted 68 lines
drivers/rtc/Makefile +68 -0
+                                                                     1);
+             if (rc < 0) {
+                     dev_err(&pdev->dev, "PM8xxx write failed!\n");
+                     goto fail_rtc_enable;
+             }
+     }
+
+     rtc_dd->ctrl_reg = ctrl_reg;
+     if (rtc_write_enable == true)
+             pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
+
+     platform_set_drvdata(pdev, rtc_dd);
+
+     /* Register the RTC device */
+     rtc_dd->rtc = rtc_device_register("pm8xxx_rtc", &pdev->dev,
+ &pm8xxx_rtc_ops, THIS_MODULE);
+     if (IS_ERR(rtc_dd->rtc)) {
+             dev_err(&pdev->dev, "%s: RTC registration failed (%ld)\n",
+                                     __func__, PTR_ERR(rtc_dd->rtc));
+             rc = PTR_ERR(rtc_dd->rtc);
+             goto fail_rtc_enable;
+     }
+
+     /* Request the alarm IRQ */
+     rc = request_any_context_irq(rtc_dd->rtc_alarm_irq,
+                              pm8xxx_alarm_trigger, IRQF_TRIGGER_RISING,
+ "pm8xxx_rtc_alarm", rtc_dd);
+     if (rc < 0) {
+             dev_err(&pdev->dev, "Request IRQ failed (%d)\n", rc);
+             goto fail_req_irq;
+     }
+
+     device_init_wakeup(&pdev->dev, 1);
+
+     dev_dbg(&pdev->dev, "Probe success !!\n");
+
+     return 0;
+
+fail_req_irq:
+     rtc_device_unregister(rtc_dd->rtc);
+fail_rtc_enable:
+     platform_set_drvdata(pdev, NULL);
+     kfree(rtc_dd);
+     return rc;
+}
+
+#ifdef CONFIG_PM
+static int pm8xxx_rtc_resume(struct device *dev)
+{
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+     if (device_may_wakeup(dev))
+             disable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+     return 0;
+}
+
+static int pm8xxx_rtc_suspend(struct device *dev)
+{
+     struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+     if (device_may_wakeup(dev))
+             enable_irq_wake(rtc_dd->rtc_alarm_irq);
+
+     return 0;
+}
+
+static const struct dev_pm_ops pm8xxx_rtc_pm_ops = {
Quoted 8 lines

drivers/rtc/Makefile

Quoted 4 lines
drivers/rtc/Makefile +4 -0
+     .suspend = pm8xxx_rtc_suspend,
+     .resume = pm8xxx_rtc_resume,
+};
+#endif

Missing a newline here? Also, can you move the probe and remove functions next to each other and move the pm related functions somewhere else (preferably above the probe and remove)?

Ok.

Quoted 53 lines
Quoted 32 lines
drivers/rtc/Makefile +31 -0
+static int __devexit pm8xxx_rtc_remove(struct platform_device *pdev)
+{
+     struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+     device_init_wakeup(&pdev->dev, 0);
+     free_irq(rtc_dd->rtc_alarm_irq, rtc_dd);
+     rtc_device_unregister(rtc_dd->rtc);
+     platform_set_drvdata(pdev, NULL);
+     kfree(rtc_dd);
+
+     return 0;
+}
+
+static struct platform_driver pm8xxx_rtc_driver = {
+     .probe          = pm8xxx_rtc_probe,
+     .remove         = __devexit_p(pm8xxx_rtc_remove),
+     .driver = {
+             .name   = PM8XXX_RTC_DEV_NAME,
+             .owner  = THIS_MODULE,
+#ifdef CONFIG_PM
+             .pm     = &pm8xxx_rtc_pm_ops,
+#endif
+     },
+};
+
+static int __init pm8xxx_rtc_init(void)
+{
+     return platform_driver_register(&pm8xxx_rtc_driver);
+}
+module_init(pm8xxx_rtc_init);
+

drivers/rtc/Makefile

Quoted 10 lines
drivers/rtc/Makefile +10 -0
+static void __exit pm8xxx_rtc_exit(void)
+{
+     platform_driver_unregister(&pm8xxx_rtc_driver);
+}
+module_exit(pm8xxx_rtc_exit);
+
+MODULE_ALIAS("platform:rtc-pm8xxx");
+MODULE_DESCRIPTION("PMIC8xxx RTC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anirudh Ghayal <[email protected]

<mailto:[email protected]>>");
--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected] <mailto:[email protected]>
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
Ashay Jaiswal
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
#6 lore raw

PATCH v3 1/1 drivers: rtc: Add support for Qualcomm PMIC8xxx RTC

In reply to Anirudh Ghayal

This patch adds support for PMIC8xxx based RTC.
PMIC8xxx is Qualcomm's power management IC that
internally houses an RTC module. This driver
communicates with the PMIC module over SSBI bus.

---
 drivers/rtc/Kconfig            |   10 +
 drivers/rtc/Makefile           |    1 +
 drivers/rtc/rtc-pm8xxx.c       |  557 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/pm8xxx/rtc.h |   25 ++
 4 files changed, 593 insertions(+), 0 deletions(-)
 create mode 100644 drivers/rtc/rtc-pm8xxx.c
 create mode 100644 include/linux/mfd/pm8xxx/rtc.h
drivers/rtc/Kconfig +10 -0
index e187887..39b7032 100644
@@ -985,6 +985,16 @@ config RTC_DRV_LPC32XX
 	  This driver can also be buillt as a module. If so, the module
 	  will be called rtc-lpc32xx.
 
+config RTC_DRV_PM8XXX
+	tristate "Qualcomm PMIC8XXX RTC"
+	depends on MFD_PM8XXX
+	help
+	  If you say yes here you get support for the
+	  Qualcomm PMIC8XXX RTC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rtc-pm8xxx.
+
 config RTC_DRV_TEGRA
 	tristate "NVIDIA Tegra Internal RTC driver"
 	depends on RTC_CLASS && ARCH_TEGRA
drivers/rtc/Makefile +1 -0
index ca91c3c..f13337c 100644
@@ -74,6 +74,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123)	+= rtc-pcf2123.o
 obj-$(CONFIG_RTC_DRV_PCF50633)	+= rtc-pcf50633.o
 obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
 obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
+obj-$(CONFIG_RTC_DRV_PM8XXX)	+= rtc-pm8xxx.o
 obj-$(CONFIG_RTC_DRV_PS3)	+= rtc-ps3.o
 obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
drivers/rtc/rtc-pm8xxx.c new file +557 -0

557 lines not on this page · raw message

include/linux/mfd/pm8xxx/rtc.h new file +25 -0
new file mode 100644
index 0000000..14f1983
@@ -0,0 +1,25 @@
+/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#ifndef __RTC_PM8XXX_H__
+#define __RTC_PM8XXX_H__
+
+#define PM8XXX_RTC_DEV_NAME     "rtc-pm8xxx"
+/**
+ * struct pm8xxx_rtc_pdata - RTC driver platform data
+ * @rtc_write_enable: variable stating RTC write capability
+ */
+struct pm8xxx_rtc_platform_data {
+	bool rtc_write_enable;
+};
+
+#endif /* __RTC_PM8XXX_H__ */
Signature · 2 lines
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Reviewed-by on v3 1/1
#7 lore raw

drivers/rtc/rtc-pm8xxx.c line 38

Quoted 8 lines · Anirudh Ghayal
drivers/rtc/rtc-pm8xxx.c new file +3 -0
new file mode 100644
index 0000000..4694c4e
+
+/**
+ * struct pm8xxx_rtc	rtc driver internal structure

This is missing a dash '-'.

drivers/rtc/rtc-pm8xxx.c line 76

Quoted 10 lines
drivers/rtc/rtc-pm8xxx.c new file +10 -0
+static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+		int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i < count; i++) {
+		rc = pm8xxx_readb(parent, base + i, &rtc_val[i]);
+		if (rc < 0) {
+			dev_err(rtc_dd->rtc_dev, "PMIC read failed\n");

I still think these printks are useless when you already have better printks at the call sites on the error path.

Quoted 94 lines
drivers/rtc/rtc-pm8xxx.c new file +94 -0
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+		int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i < count; i++) {
+		rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
+		if (rc < 0) {
+			dev_err(rtc_dd->rtc_dev, "PMIC write failed\n");
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Steps to write the RTC registers.
+ * 1. Disable alarm if enabled.
+ * 2. Write 0x00 to LSB.
+ * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
+ * 4. Enable alarm if disabled in step 1.
+ */
+static int
+pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	int rc, i;
+	unsigned long secs, irq_flags;
+	u8 value[NUM_8_BIT_RTC_REGS], reg = 0, alarm_enabled = 0, ctrl_reg;
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+
+	rtc_tm_to_time(tm, &secs);
+
+	for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) {
+		value[i] = secs & 0xFF;
+		secs >>= 8;
+	}
+
+	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+
+	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	ctrl_reg = rtc_dd->ctrl_reg;
+
+	if (ctrl_reg & PM8xxx_RTC_ALARM_ENABLE) {
+		alarm_enabled = 1;
+		ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
+				1);
+		if (rc < 0) {
+			dev_err(dev, "Write to RTC control register "
+								"failed\n");
+			goto rtc_rw_fail;
+		}
+		rtc_dd->ctrl_reg = ctrl_reg;
+	} else
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+
+	/* Write 0 to Byte[0] */
+	reg = 0;
+	rc = pm8xxx_write_wrapper(rtc_dd, &reg, rtc_dd->rtc_write_base, 1);
+	if (rc < 0) {
+		dev_err(dev, "Write to RTC write data register failed\n");
+		goto rtc_rw_fail;
+	}
+
+	/* Write Byte[1], Byte[2], Byte[3] */
+	rc = pm8xxx_write_wrapper(rtc_dd, value + 1,
+					rtc_dd->rtc_write_base + 1, 3);
+	if (rc < 0) {
+		dev_err(dev, "Write to RTC write data register failed\n");
+		goto rtc_rw_fail;
+	}
+
+	/* Write Byte[0] */
+	rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->rtc_write_base, 1);
+	if (rc < 0) {
+		dev_err(dev, "Write to RTC write data register failed\n");
+		goto rtc_rw_fail;
+	}
+
+	if (alarm_enabled) {
+		ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
+		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
+									1);
+		if (rc < 0) {
+			dev_err(dev, "Write to RTC control register "

drivers/rtc/rtc-pm8xxx.c line 180

Quoted 10 lines
drivers/rtc/rtc-pm8xxx.c new file +10 -0
+								"failed\n");
+			goto rtc_rw_fail;
+		}
+		rtc_dd->ctrl_reg = ctrl_reg;
+	}
+
+rtc_rw_fail:
+	if (alarm_enabled)
+		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+

This could be simplified by putting the label inside the if and then putting the spin_unlock at the end of the if. You would save a branch. Not sure it really matters though since the compiler should be doing it anyway.

Is it really that bad to take the lock all the time? It would simplify this function a bit if you did.

If you fix the kernel-doc error you can add a

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
#8 lore raw
Quoted 12 lines · Stephen Boyd

drivers/rtc/rtc-pm8xxx.c line 38

Quoted 8 lines · Anirudh Ghayal
drivers/rtc/rtc-pm8xxx.c new file +3 -0
new file mode 100644
index 0000000..4694c4e
+
+/**
+ * struct pm8xxx_rtc	rtc driver internal structure

This is missing a dash '-'.

Quoted 13 lines

drivers/rtc/rtc-pm8xxx.c line 76

Quoted 10 lines
drivers/rtc/rtc-pm8xxx.c new file +10 -0
+static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+		int base, int count)
+{
+	int i, rc;
+	struct device *parent = rtc_dd->rtc_dev->parent;
+
+	for (i = 0; i<  count; i++) {
+		rc = pm8xxx_readb(parent, base + i,&rtc_val[i]);
+		if (rc<  0) {
+			dev_err(rtc_dd->rtc_dev, "PMIC read failed\n");

I still think these printks are useless when you already have better printks at the call sites on the error path.

I actually don't mind seeing the entire error flow in the log. pm8xxx_readb() might report a failure but it is sometimes help to know who the caller was that may have triggered the error.

Signature · 2 lines
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.