Patchwork [v2,2/6] rtc: pm8xxx: use regmap API for register accesses

login
register
mail settings
Submitter Josh Cartwright
Date March 10, 2014, 6:44 p.m.
Message ID <1394477089-3996-3-git-send-email-joshc@codeaurora.org>
Download mbox | patch
Permalink /patch/328702/
State New
Headers show

Comments

Josh Cartwright - March 10, 2014, 6:44 p.m.
Now that the parent mfd driver has been made to work again, and has been
reworked to create a regmap instance intended for it's children to use,
rework the pm8xxx driver to use the regmap API for it's register
accesses.

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
---
 drivers/rtc/rtc-pm8xxx.c | 145 +++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 88 deletions(-)
Andrew Morton - March 10, 2014, 8:47 p.m.
On Mon, 10 Mar 2014 13:44:45 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:

> Now that the parent mfd driver has been made to work again, and has been
> reworked to create a regmap instance intended for it's children to use,
> rework the pm8xxx driver to use the regmap API for it's register
> accesses.

Do we need a 

	depends on REGMAP

in Kconfig?
Josh Cartwright - March 10, 2014, 9:35 p.m.
On Mon, Mar 10, 2014 at 01:47:41PM -0700, Andrew Morton wrote:
> On Mon, 10 Mar 2014 13:44:45 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:
> > Now that the parent mfd driver has been made to work again, and has been
> > reworked to create a regmap instance intended for it's children to use,
> > rework the pm8xxx driver to use the regmap API for it's register
> > accesses.
> 
> Do we need a 
> 
> 	depends on REGMAP
> 
> in Kconfig?

Good question.  This driver already gets the REGMAP dependency through
it's parent (MFD_PM8XXX), which does 'select REGMAP'.

I recall there being some history as to why individual drivers shouldn't
explicitly depend on REGMAP, as it's the parent drivers/subsystem
responsibility to select it when needed, however I can't recall details.
Perhaps it's due to REGMAP not being a user-selectable option.

(FWIW, I did force a build of this driver with !REGMAP, and it builds
just fine with the regmap_write()/_read() stubs, but of course it
wouldn't do anything meaningful at runtime)

Thanks,
  Josh
Andrew Morton - March 10, 2014, 9:43 p.m.
On Mon, 10 Mar 2014 16:35:24 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:

> On Mon, Mar 10, 2014 at 01:47:41PM -0700, Andrew Morton wrote:
> > On Mon, 10 Mar 2014 13:44:45 -0500 Josh Cartwright <joshc@codeaurora.org> wrote:
> > > Now that the parent mfd driver has been made to work again, and has been
> > > reworked to create a regmap instance intended for it's children to use,
> > > rework the pm8xxx driver to use the regmap API for it's register
> > > accesses.
> > 
> > Do we need a 
> > 
> > 	depends on REGMAP
> > 
> > in Kconfig?
> 
> Good question.  This driver already gets the REGMAP dependency through
> it's parent (MFD_PM8XXX), which does 'select REGMAP'.

OK, thanks, that sounds sufficient.
Mark Brown - March 10, 2014, 11:32 p.m.
On Mon, Mar 10, 2014 at 04:35:24PM -0500, Josh Cartwright wrote:
> On Mon, Mar 10, 2014 at 01:47:41PM -0700, Andrew Morton wrote:

> > Do we need a 

> > 	depends on REGMAP

> > in Kconfig?

> Good question.  This driver already gets the REGMAP dependency through
> it's parent (MFD_PM8XXX), which does 'select REGMAP'.

> I recall there being some history as to why individual drivers shouldn't
> explicitly depend on REGMAP, as it's the parent drivers/subsystem
> responsibility to select it when needed, however I can't recall details.
> Perhaps it's due to REGMAP not being a user-selectable option.

Yeah, it should be a select of regmap because it's more of a helper
library than a user visible feature and it's usually only on the MFD
because you can't enable the function device without the core anyway.

Patch

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index af60ee4..cdc9dc2 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -13,11 +13,12 @@ 
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/rtc.h>
+#include <linux/platform_device.h>
 #include <linux/pm.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
-#include <linux/mfd/pm8xxx/core.h>
 #include <linux/mfd/pm8xxx/rtc.h>
 
 
@@ -37,6 +38,7 @@ 
 /**
  * struct pm8xxx_rtc -  rtc driver internal structure
  * @rtc:		rtc device for this driver.
+ * @regmap:		regmap used to access RTC registers
  * @rtc_alarm_irq:	rtc alarm irq number.
  * @rtc_base:		address of rtc control register.
  * @rtc_read_base:	base address of read registers.
@@ -48,6 +50,7 @@ 
  */
 struct pm8xxx_rtc {
 	struct rtc_device *rtc;
+	struct regmap *regmap;
 	int rtc_alarm_irq;
 	int rtc_base;
 	int rtc_read_base;
@@ -59,44 +62,6 @@  struct pm8xxx_rtc {
 };
 
 /*
- * 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, "PMIC 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, "PMIC write failed\n");
-			return rc;
-		}
-	}
-
-	return 0;
-}
-
-/*
  * Steps to write the RTC registers.
  * 1. Disable alarm if enabled.
  * 2. Write 0x00 to LSB.
@@ -107,7 +72,7 @@  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;
+	u8 value[NUM_8_BIT_RTC_REGS], alarm_enabled = 0, ctrl_reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 
 	rtc_tm_to_time(tm, &secs);
@@ -125,9 +90,8 @@  static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	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) {
+		rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_base, ctrl_reg);
+		if (rc) {
 			dev_err(dev, "Write to RTC control register failed\n");
 			goto rtc_rw_fail;
 		}
@@ -137,33 +101,31 @@  static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	}
 
 	/* Write 0 to Byte[0] */
-	reg = 0;
-	rc = pm8xxx_write_wrapper(rtc_dd, &reg, rtc_dd->rtc_write_base, 1);
-	if (rc < 0) {
+	rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_write_base, 0);
+	if (rc) {
 		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) {
+	rc = regmap_bulk_write(rtc_dd->regmap, rtc_dd->rtc_write_base + 1,
+			       &value[1], sizeof(value) - 1);
+	if (rc) {
 		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) {
+	rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_write_base, value[0]);
+	if (rc) {
 		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) {
+		rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_base, ctrl_reg);
+		if (rc) {
 			dev_err(dev, "Write to RTC control register failed\n");
 			goto rtc_rw_fail;
 		}
@@ -180,13 +142,14 @@  rtc_rw_fail:
 static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	int rc;
-	u8 value[NUM_8_BIT_RTC_REGS], reg;
+	u8 value[NUM_8_BIT_RTC_REGS];
 	unsigned long secs;
+	unsigned int reg;
 	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) {
+	rc = regmap_bulk_read(rtc_dd->regmap, rtc_dd->rtc_read_base,
+			      value, sizeof(value));
+	if (rc) {
 		dev_err(dev, "RTC read data register failed\n");
 		return rc;
 	}
@@ -195,17 +158,16 @@  static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	 * 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);
+	rc = regmap_read(rtc_dd->regmap, rtc_dd->rtc_read_base, &reg);
 	if (rc < 0) {
 		dev_err(dev, "RTC read data register 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) {
+		rc = regmap_bulk_read(rtc_dd->regmap, rtc_dd->rtc_read_base,
+				      value, sizeof(value));
+		if (rc) {
 			dev_err(dev, "RTC read data register failed\n");
 			return rc;
 		}
@@ -244,9 +206,9 @@  static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 
 	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) {
+	rc = regmap_bulk_write(rtc_dd->regmap, rtc_dd->alarm_rw_base, value,
+			       sizeof(value));
+	if (rc) {
 		dev_err(dev, "Write to RTC ALARM register failed\n");
 		goto rtc_rw_fail;
 	}
@@ -258,8 +220,8 @@  static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	else
 		ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
 
-	rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
-	if (rc < 0) {
+	rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_base, ctrl_reg);
+	if (rc) {
 		dev_err(dev, "Write to RTC control register failed\n");
 		goto rtc_rw_fail;
 	}
@@ -282,9 +244,9 @@  static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	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) {
+	rc = regmap_bulk_read(rtc_dd->regmap, rtc_dd->alarm_rw_base, value,
+			      sizeof(value));
+	if (rc) {
 		dev_err(dev, "RTC alarm time read failed\n");
 		return rc;
 	}
@@ -323,8 +285,8 @@  static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 	else
 		ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
 
-	rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
-	if (rc < 0) {
+	rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_base, ctrl_reg);
+	if (rc) {
 		dev_err(dev, "Write to RTC control register failed\n");
 		goto rtc_rw_fail;
 	}
@@ -346,7 +308,7 @@  static struct rtc_class_ops pm8xxx_rtc_ops = {
 static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 {
 	struct pm8xxx_rtc *rtc_dd = dev_id;
-	u8 ctrl_reg;
+	unsigned int ctrl_reg;
 	int rc;
 	unsigned long irq_flags;
 
@@ -358,8 +320,8 @@  static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 	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) {
+	rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_base, ctrl_reg);
+	if (rc) {
 		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 		dev_err(rtc_dd->rtc_dev,
 			"Write to RTC control register failed\n");
@@ -370,18 +332,20 @@  static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 	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) {
+	rc = regmap_read(rtc_dd->regmap,
+			 rtc_dd->rtc_base + PM8XXX_ALARM_CTRL_OFFSET,
+			 &ctrl_reg);
+	if (rc) {
 		dev_err(rtc_dd->rtc_dev,
 			"RTC Alarm control register read 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)
+	rc = regmap_write(rtc_dd->regmap,
+			  rtc_dd->rtc_base + PM8XXX_ALARM_CTRL_OFFSET,
+			  ctrl_reg);
+	if (rc)
 		dev_err(rtc_dd->rtc_dev,
 			"Write to RTC Alarm control register failed\n");
 
@@ -392,7 +356,7 @@  rtc_alarm_handled:
 static int pm8xxx_rtc_probe(struct platform_device *pdev)
 {
 	int rc;
-	u8 ctrl_reg;
+	unsigned int ctrl_reg;
 	bool rtc_write_enable = false;
 	struct pm8xxx_rtc *rtc_dd;
 	struct resource *rtc_resource;
@@ -409,6 +373,12 @@  static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	/* Initialise spinlock to protect RTC control register */
 	spin_lock_init(&rtc_dd->ctrl_reg_lock);
 
+	rtc_dd->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!rtc_dd->regmap) {
+		dev_err(&pdev->dev, "Parent regmap unavailable.\n");
+		return -ENXIO;
+	}
+
 	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");
@@ -432,17 +402,16 @@  static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	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) {
+	rc = regmap_read(rtc_dd->regmap, rtc_dd->rtc_base, &ctrl_reg);
+	if (rc) {
 		dev_err(&pdev->dev, "RTC control register read failed!\n");
 		return rc;
 	}
 
 	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) {
+		rc = regmap_write(rtc_dd->regmap, rtc_dd->rtc_base, ctrl_reg);
+		if (rc) {
 			dev_err(&pdev->dev,
 				"Write to RTC control register failed\n");
 			return rc;