[PATCH v6 0/6] Add MFD and regulator drivers for Hi6421 PMIC SoC

[PATCH v6 0/6] Add MFD and regulator drivers for Hi6421 PMIC SoC

[Guodong Xu]

This patchset adds an MFD core driver and a regulator driver for Hi6421 PMIC
SoC.

Hi6421 is a PMIC SoC designed and manufactured by HiSilicon Ltd. It includes
multi-functions, such as regulators, codec, ADCs, Coulomb counter, etc.
Hi6421 can be used in various Hi3620 SoC based boards. Registers in Hi6421 are
memory bus mapped, so in this design they are accessed by regmap-mmio APIs.

Hi6421 has a requirement to keep a guard delay between its regulator's
disable and next enable. Implement this requirement in regulator core,
per Mark Brown's suggestion.

---
changes in v6:
 * rebase to v3.17-rc1:
  - use devm_ioremap_resource in hi6421 mfd driver
  - add 'const' before var regulator_ops in new funcs in regulator core
 * fix build error in drivers/regulator/mc13892-regulator.c
     due to 'const' in regulator_desc.ops
 * remove patch to hi3xxx_defconfig

changes in v5:
 * rename _regulator_delay to _regulator_enable_delay
 * clean up hi6421_regulator_enable(), add 100us protective gap into
     regulator_desc's .enable_time
 * code clean up, fixes for style checking

changes in v4:
 * regulator: core:
  - add const qualifier to ops in struct regulator_desc
  - factor out _regulator_delay from _regulator_do_enable
  - add guard delay between calling regulator_disable and _enable
 * hi6421 regulator:
  - bug fix: missing min_uV for linear regualtor BUCK0~2
  - bug fix: wrong volt values in buck_3_voltages table
  - add const to hi6421 regulator_ops
  - use ldo_linear and ldo_linear_range in applicable LDOs.
  - rename regulator-name to conform to names in board schematics.

changes in v3:
 * hi6421: regulator: remove setting of constraints on board-specific
     capabilities
 * delete unecessary compatible property "hisilicon,hi6421-regulators"
 * change regulator enable/disable callback APIs to support protection
gaps, as
     required by hi6421 spec
 * split regulator set_mode/get_mode callback APIs to LDOs and BUCKs
 * delete hi6421.dtsi, mfd and regulator nodes are now in board config .dts

changes in v2:
 * use MFD APIs to manage sub-devices of Hi6421
 * use regmap-mmio for register access
 * use only generic regulator device tree property
 * rewrite regulator driver using helper APIs from both regulator and regmap

v1: first drop

Guodong Xu (6):
  regulator: core: add const qualifier to ops in struct regulator_desc
  regulator: core: factor out delay function from _regulator_do_enable
  regulator: core: add guard delay between calling regulator_disable and
    _enable
  mfd: Add hi6421 PMIC core driver
  regulator: add driver for hi6421 voltage regulator
  ARM: dts: hi3620-hi4511: Add HI6421 MFD and regulator nodes

 Documentation/devicetree/bindings/mfd/hi6421.txt |  37 ++
 arch/arm/boot/dts/hi3620-hi4511.dts              | 233 ++++++++
 drivers/mfd/Kconfig                              |  13 +
 drivers/mfd/Makefile                             |   1 +
 drivers/mfd/hi6421-pmic-core.c                   | 117 ++++
 drivers/regulator/Kconfig                        |  10 +
 drivers/regulator/Makefile                       |   1 +
 drivers/regulator/core.c                         | 137 +++--
 drivers/regulator/hi6421-regulator.c             | 657 +++++++++++++++++++++++
 drivers/regulator/mc13892-regulator.c            |  11 +-
 include/linux/mfd/hi6421-pmic.h                  |  39 ++
 include/linux/regulator/driver.h                 |   8 +-
 12 files changed, 1209 insertions(+), 55 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
 create mode 100644 drivers/mfd/hi6421-pmic-core.c
 create mode 100644 drivers/regulator/hi6421-regulator.c
 create mode 100644 include/linux/mfd/hi6421-pmic.h

-- 
1.9.1

[PATCH v6 1/6] regulator: core: add const qualifier to ops in struct regulator_desc

[Guodong Xu]

struct regulator_ops *ops is a member in struct regulator_desc, which gets
its value from individual regulator driver upon regulator_register() and
is used by regulator core APIs. It's not allowed for regulator core to
modify any of these callbacks in *ops. Add 'const' qualifier to enforce that.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/regulator/core.c              | 32 ++++++++++++++++----------------
 drivers/regulator/mc13892-regulator.c | 11 +++++++----
 include/linux/regulator/driver.h      |  2 +-
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a3c3785..c4a13db 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -839,7 +839,7 @@ static void print_constraints(struct regulator_dev *rdev)
 static int machine_constraints_voltage(struct regulator_dev *rdev,
 	struct regulation_constraints *constraints)
 {
-	struct regulator_ops *ops = rdev->desc->ops;
+	const struct regulator_ops *ops = rdev->desc->ops;
 	int ret;
 
 	/* do we need to apply the constraint voltage */
@@ -938,7 +938,7 @@ static int machine_constraints_voltage(struct regulator_dev *rdev,
 static int machine_constraints_current(struct regulator_dev *rdev,
 	struct regulation_constraints *constraints)
 {
-	struct regulator_ops *ops = rdev->desc->ops;
+	const struct regulator_ops *ops = rdev->desc->ops;
 	int ret;
 
 	if (!constraints->min_uA && !constraints->max_uA)
@@ -982,7 +982,7 @@ static int set_machine_constraints(struct regulator_dev *rdev,
 	const struct regulation_constraints *constraints)
 {
 	int ret = 0;
-	struct regulator_ops *ops = rdev->desc->ops;
+	const struct regulator_ops *ops = rdev->desc->ops;
 
 	if (constraints)
 		rdev->constraints = kmemdup(constraints, sizeof(*constraints),
@@ -2208,9 +2208,9 @@ EXPORT_SYMBOL_GPL(regulator_count_voltages);
  */
 int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
-	int			ret;
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int ret;
 
 	if (rdev->desc->fixed_uV && rdev->desc->n_voltages == 1 && !selector)
 		return rdev->desc->fixed_uV;
@@ -2270,8 +2270,8 @@ int regulator_get_hardware_vsel_register(struct regulator *regulator,
 					 unsigned *vsel_reg,
 					 unsigned *vsel_mask)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
 
 	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
 		return -EOPNOTSUPP;
@@ -2297,8 +2297,8 @@ EXPORT_SYMBOL_GPL(regulator_get_hardware_vsel_register);
 int regulator_list_hardware_vsel(struct regulator *regulator,
 				 unsigned selector)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
 
 	if (selector >= rdev->desc->n_voltages)
 		return -EINVAL;
@@ -2572,8 +2572,8 @@ EXPORT_SYMBOL_GPL(regulator_set_voltage);
 int regulator_set_voltage_time(struct regulator *regulator,
 			       int old_uV, int new_uV)
 {
-	struct regulator_dev	*rdev = regulator->rdev;
-	struct regulator_ops	*ops = rdev->desc->ops;
+	struct regulator_dev *rdev = regulator->rdev;
+	const struct regulator_ops *ops = rdev->desc->ops;
 	int old_sel = -1;
 	int new_sel = -1;
 	int voltage;
@@ -3336,9 +3336,9 @@ EXPORT_SYMBOL_GPL(regulator_mode_to_status);
  */
 static int add_regulator_attributes(struct regulator_dev *rdev)
 {
-	struct device		*dev = &rdev->dev;
-	struct regulator_ops	*ops = rdev->desc->ops;
-	int			status = 0;
+	struct device *dev = &rdev->dev;
+	const struct regulator_ops *ops = rdev->desc->ops;
+	int status = 0;
 
 	/* some attributes need specific methods to be displayed */
 	if ((ops->get_voltage && ops->get_voltage(rdev) >= 0) ||
@@ -3905,7 +3905,7 @@ core_initcall(regulator_init);
 static int __init regulator_init_complete(void)
 {
 	struct regulator_dev *rdev;
-	struct regulator_ops *ops;
+	const struct regulator_ops *ops;
 	struct regulation_constraints *c;
 	int enabled, ret;
 
diff --git a/drivers/regulator/mc13892-regulator.c b/drivers/regulator/mc13892-regulator.c
index f374fa5..793b662 100644
--- a/drivers/regulator/mc13892-regulator.c
+++ b/drivers/regulator/mc13892-regulator.c
@@ -526,6 +526,7 @@ static unsigned int mc13892_vcam_get_mode(struct regulator_dev *rdev)
 	return REGULATOR_MODE_NORMAL;
 }
 
+static struct regulator_ops mc13892_vcam_ops;
 
 static int mc13892_regulator_probe(struct platform_device *pdev)
 {
@@ -582,10 +583,12 @@ static int mc13892_regulator_probe(struct platform_device *pdev)
 	}
 	mc13xxx_unlock(mc13892);
 
-	mc13892_regulators[MC13892_VCAM].desc.ops->set_mode
-		= mc13892_vcam_set_mode;
-	mc13892_regulators[MC13892_VCAM].desc.ops->get_mode
-		= mc13892_vcam_get_mode;
+	/* update mc13892_vcam ops */
+	memcpy(&mc13892_vcam_ops, mc13892_regulators[MC13892_VCAM].desc.ops,
+						sizeof(struct regulator_ops));
+	mc13892_vcam_ops.set_mode = mc13892_vcam_set_mode,
+	mc13892_vcam_ops.get_mode = mc13892_vcam_get_mode,
+	mc13892_regulators[MC13892_VCAM].desc.ops = &mc13892_vcam_ops;
 
 	mc13xxx_data = mc13xxx_parse_regulators_dt(pdev, mc13892_regulators,
 					ARRAY_SIZE(mc13892_regulators));
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index bbe03a1..4b62813 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -245,7 +245,7 @@ struct regulator_desc {
 	int id;
 	bool continuous_voltage_range;
 	unsigned n_voltages;
-	struct regulator_ops *ops;
+	const struct regulator_ops *ops;
 	int irq;
 	enum regulator_type type;
 	struct module *owner;
-- 
1.9.1

[PATCH v6 2/6] regulator: core: factor out delay function from _regulator_do_enable

[Guodong Xu]

A common delay function can be helpful when implementing new features. Factor
it out to maximize code reusability.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/regulator/core.c | 74 ++++++++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c4a13db..4976451 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1759,6 +1759,45 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 	return 0;
 }
 
+/**
+ * _regulator_enable_delay - a delay helper function
+ * @delay: time to delay in microseconds
+ *
+ * Delay for the requested amount of time as per the guidelines in:
+ *
+ *     Documentation/timers/timers-howto.txt
+ *
+ * The assumption here is that regulators will never be enabled in
+ * atomic context and therefore sleeping functions can be used.
+ */
+static void _regulator_enable_delay(unsigned int delay)
+{
+	unsigned int ms = delay / 1000;
+	unsigned int us = delay % 1000;
+
+	if (ms > 0) {
+		/*
+		 * For small enough values, handle super-millisecond
+		 * delays in the usleep_range() call below.
+		 */
+		if (ms < 20)
+			us += ms * 1000;
+		else
+			msleep(ms);
+	}
+
+	/*
+	 * Give the scheduler some room to coalesce with any other
+	 * wakeup sources. For delays shorter than 10 us, don't even
+	 * bother setting up high-resolution timers and just busy-
+	 * loop.
+	 */
+	if (us >= 10)
+		usleep_range(us, us + 100);
+	else
+		udelay(us);
+}
+
 static int _regulator_do_enable(struct regulator_dev *rdev)
 {
 	int ret, delay;
@@ -1792,40 +1831,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 	 * together.  */
 	trace_regulator_enable_delay(rdev_get_name(rdev));
 
-	/*
-	 * Delay for the requested amount of time as per the guidelines in:
-	 *
-	 *     Documentation/timers/timers-howto.txt
-	 *
-	 * The assumption here is that regulators will never be enabled in
-	 * atomic context and therefore sleeping functions can be used.
-	 */
-	if (delay) {
-		unsigned int ms = delay / 1000;
-		unsigned int us = delay % 1000;
-
-		if (ms > 0) {
-			/*
-			 * For small enough values, handle super-millisecond
-			 * delays in the usleep_range() call below.
-			 */
-			if (ms < 20)
-				us += ms * 1000;
-			else
-				msleep(ms);
-		}
-
-		/*
-		 * Give the scheduler some room to coalesce with any other
-		 * wakeup sources. For delays shorter than 10 us, don't even
-		 * bother setting up high-resolution timers and just busy-
-		 * loop.
-		 */
-		if (us >= 10)
-			usleep_range(us, us + 100);
-		else
-			udelay(us);
-	}
+	_regulator_enable_delay(delay);
 
 	trace_regulator_enable_complete(rdev_get_name(rdev));
 
-- 
1.9.1

[PATCH v6 3/6] regulator: core: add guard delay between calling regulator_disable and _enable

[Guodong Xu]

Some regulator require a minimum delay between its disable and next enable.
This is to avoid damages when out-of-range frequent disable/enable of a
single regulator can bring to the regulator chip.

Add @off_on_delay to struct regulator_desc. Device drivers' can use this field
to set this guard time.

Add @last_off_jiffy to struct regulator_dev. When @off_on_delay is set by
driver, regulator core can store its last off (disable) time into this field.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/regulator/core.c         | 31 +++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |  6 ++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4976451..7bce715 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1813,6 +1813,31 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 
 	trace_regulator_enable(rdev_get_name(rdev));
 
+	if (rdev->desc->off_on_delay) {
+		/* if needed, keep a distance of off_on_delay from last time
+		 * this regulator was disabled.
+		 */
+		unsigned long start_jiffy = jiffies;
+		unsigned long intended, max_delay, remaining;
+
+		max_delay = usecs_to_jiffies(rdev->desc->off_on_delay);
+		intended = rdev->last_off_jiffy + max_delay;
+
+		if (time_before(start_jiffy, intended)) {
+			/* calc remaining jiffies to deal with one-time
+			 * timer wrapping.
+			 * in case of multiple timer wrapping, either it can be
+			 * detected by out-of-range remaining, or it cannot be
+			 * detected and we gets a panelty of
+			 * _regulator_enable_delay().
+			 */
+			remaining = intended - start_jiffy;
+			if (remaining <= max_delay)
+				_regulator_enable_delay(
+						jiffies_to_usecs(remaining));
+		}
+	}
+
 	if (rdev->ena_pin) {
 		ret = regulator_ena_gpio_ctrl(rdev, true);
 		if (ret < 0)
@@ -1925,6 +1950,12 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 			return ret;
 	}
 
+	/* cares about last_off_jiffy only if off_on_delay is required by
+	 * device.
+	 */
+	if (rdev->desc->off_on_delay)
+		rdev->last_off_jiffy = jiffies;
+
 	trace_regulator_disable_complete(rdev_get_name(rdev));
 
 	return 0;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4b62813..efe058f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -238,6 +238,7 @@ enum regulator_type {
  * @bypass_val_off: Disabling value for control when using regmap set_bypass
  *
  * @enable_time: Time taken for initial enable of regulator (in uS).
+ * @off_on_delay: guard time (in uS), before re-enabling a regulator
  */
 struct regulator_desc {
 	const char *name;
@@ -276,6 +277,8 @@ struct regulator_desc {
 	unsigned int bypass_val_off;
 
 	unsigned int enable_time;
+
+	unsigned int off_on_delay;
 };
 
 /**
@@ -348,6 +351,9 @@ struct regulator_dev {
 
 	struct regulator_enable_gpio *ena_pin;
 	unsigned int ena_gpio_state:1;
+
+	/* time when this regulator was disabled last time */
+	unsigned long last_off_jiffy;
 };
 
 struct regulator_dev *
-- 
1.9.1

[PATCH v6 4/6] mfd: Add hi6421 PMIC core driver

[Guodong Xu]

This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
functions, such as regulators, codec, ADCs, Coulomb counter, etc.
This driver includes core APIs _only_.

Drivers for individul components, like voltage regulators, are
implemented in corresponding driver directories and files.

Registers in Hi6421 are memory mapped, so using regmap-mmio API.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++
 drivers/mfd/Kconfig                              |  13 +++
 drivers/mfd/Makefile                             |   1 +
 drivers/mfd/hi6421-pmic-core.c                   | 117 +++++++++++++++++++++++
 include/linux/mfd/hi6421-pmic.h                  |  39 ++++++++
 5 files changed, 207 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
 create mode 100644 drivers/mfd/hi6421-pmic-core.c
 create mode 100644 include/linux/mfd/hi6421-pmic.h

diff --git a/Documentation/devicetree/bindings/mfd/hi6421.txt b/Documentation/devicetree/bindings/mfd/hi6421.txt
new file mode 100644
index 0000000..1512123
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/hi6421.txt
@@ -0,0 +1,37 @@
+* HI6421 Multi-Functional Device (MFD), by HiSilicon Ltd.
+
+Required parent device properties:
+- reg		: register range space of hi6421;
+
+Supported Hi6421 sub-devices include:
+
+Device                     IRQ Names              Supply Names   Description
+------                     ---------              ------------   -----------
+regulators               :                      :              : Regulators
+
+Required child device properties:
+None.
+
+Example:
+	pmic: pmic@c00000 {
+		compatible = "hisilicon,hi6421-pmic";
+		reg = <0xc00000 0x0180>; /* 0x60 << 2 */
+
+		regulators {
+			// supply for MLC NAND/ eMMC
+			hi6421_vout0_reg: hi6421_vout0 {
+				regulator-name = "VOUT0";
+				regulator-min-microvolt = <2850000>;
+				regulator-max-microvolt = <2850000>;
+			};
+
+			// supply for 26M Oscillator
+			hi6421_vout1_reg: hi6421_vout1 {
+				regulator-name = "VOUT1";
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <2000000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+		};
+       	};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index de5abf2..347cbf6 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -210,6 +210,19 @@ config MFD_MC13XXX_I2C
 	help
 	  Select this if your MC13xxx is connected via an I2C bus.
 
+config MFD_HI6421_PMIC
+	tristate "HiSilicon Hi6421 PMU/Codec IC"
+	depends on OF
+	select MFD_CORE
+	select REGMAP_MMIO
+	help
+	  This driver supports HiSilicon Hi6421 PMIC. Hi6421 includes multi-
+	  functions, such as regulators, codec, ADCs, Coulomb counter, etc.
+	  This driver includes core APIs _only_. You have to select
+	  individul components like voltage regulators under corresponding
+	  menus in order to enable them.
+	  Memory-mapped I/O is the way to communicate with Hi6421.
+
 config HTC_EGPIO
 	bool "HTC EGPIO support"
 	depends on GPIOLIB && ARM
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f001487..dc59efd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
 obj-$(CONFIG_MFD_AS3722)	+= as3722.o
 obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
 obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
+obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
 
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
new file mode 100644
index 0000000..2be4d3f
--- /dev/null
+++ b/drivers/mfd/hi6421-pmic-core.c
@@ -0,0 +1,117 @@
+/*
+ * Device driver for Hi6421 IC
+ *
+ * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ * Copyright (c) <2013-2014> Linaro Ltd.
+ *              http://www.linaro.org
+ *
+ * Author: Guodong Xu <guodong.xu@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/mfd/hi6421-pmic.h>
+
+static struct of_device_id of_hi6421_pmic_match_tbl[] = {
+	{
+		.compatible = "hisilicon,hi6421-pmic",
+	},
+	{ /* end */ }
+};
+
+static const struct mfd_cell hi6421_devs[] = {
+	{
+		.name = "hi6421-regulator",
+	},
+};
+
+static struct regmap_config hi6421_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 8,
+	.max_register = HI6421_REG_TO_BUS_ADDR(0xFF),
+};
+
+static int hi6421_pmic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hi6421_pmic *pmic = NULL;
+	struct resource *res;
+	void __iomem *base;
+	int ret;
+
+	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+	if (!pmic) {
+		dev_err(dev, "cannot allocate hi6421_pmic device info\n");
+		return -ENOMEM;
+	}
+
+	/* get resources */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (unlikely(!res)) {
+		dev_err(&pdev->dev, "Invalid mem resource.\n");
+		return -ENODEV;
+	}
+
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
+						 &hi6421_regmap_config);
+	if (IS_ERR(pmic->regmap)) {
+		dev_err(&pdev->dev, "regmap init failed: %ld\n",
+						PTR_ERR(pmic->regmap));
+		return PTR_ERR(pmic->regmap);
+	}
+
+	pmic->dev = dev;
+	platform_set_drvdata(pdev, pmic);
+
+	/* set over-current protection debounce 8ms*/
+	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
+		(HI6421_OCP_DEB_SEL_MASK | HI6421_OCP_EN_DEBOUNCE_MASK |
+		 HI6421_OCP_AUTO_STOP_MASK),
+		(HI6421_OCP_DEB_SEL_8MS | HI6421_OCP_EN_DEBOUNCE_ENABLE));
+
+	ret = mfd_add_devices(dev, 0, hi6421_devs,
+			ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
+
+	return ret;
+}
+
+static int hi6421_pmic_remove(struct platform_device *pdev)
+{
+	mfd_remove_devices(&pdev->dev);
+
+	return 0;
+}
+
+static struct platform_driver hi6421_pmic_driver = {
+	.driver = {
+		.name	= "hi6421_pmic",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_hi6421_pmic_match_tbl,
+	},
+	.probe	= hi6421_pmic_probe,
+	.remove	= hi6421_pmic_remove,
+};
+module_platform_driver(hi6421_pmic_driver);
+
+MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
+MODULE_DESCRIPTION("Hi6421 PMIC driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
new file mode 100644
index 0000000..36bcb34
--- /dev/null
+++ b/include/linux/mfd/hi6421-pmic.h
@@ -0,0 +1,39 @@
+/*
+ * Header file for device driver Hi6421 PMIC
+ *
+ * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ * Copyright (c) <2013-2014> Linaro Ltd.
+ *              http://www.linaro.org
+ *
+ * Author: Guodong Xu <guodong.xu@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef	__HI6421_PMIC_H
+#define	__HI6421_PMIC_H
+
+/* Hi6421 registers are mapped to memory bus in 4 bytes stride */
+#define HI6421_REG_TO_BUS_ADDR(x)	(x << 2)
+
+/* Hi6421 OCP (over current protection) and DEB (debounce) control register */
+#define	HI6421_OCP_DEB_CTRL_REG		HI6421_REG_TO_BUS_ADDR(0x51)
+#define	HI6421_OCP_DEB_SEL_MASK		(0x0C)
+#define HI6421_OCP_DEB_SEL_8MS		(0x00)
+#define HI6421_OCP_DEB_SEL_16MS		(0x04)
+#define HI6421_OCP_DEB_SEL_32MS		(0x08)
+#define HI6421_OCP_DEB_SEL_64MS		(0x0C)
+#define HI6421_OCP_EN_DEBOUNCE_MASK	(0x02)
+#define HI6421_OCP_EN_DEBOUNCE_ENABLE	(0x02)
+#define HI6421_OCP_AUTO_STOP_MASK	(0x01)
+#define HI6421_OCP_AUTO_STOP_ENABLE	(0x01)
+
+struct hi6421_pmic {
+	struct device		*dev;
+	struct regmap		*regmap;
+};
+
+#endif		/* __HI6421_PMIC_H */
-- 
1.9.1

[PATCH v6 5/6] regulator: add driver for hi6421 voltage regulator

[Guodong Xu]

Add driver support for HiSilicon Hi6421 voltage regulators.

Two rules for regulator enabling are defined in hi6421 spec:
1) Between disable and enable of each regulator (LDOs or BUCKs), there must
   be a protection gap. Use @off_on_delay of regulator core to implement this.
2) No two regulators can be enabled at the same time. Use mutex in
   hi6421_regulator_pdata to ensure this. A protection gap of 100us is added
   into each LDO/BUCK's .enable_time.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/regulator/Kconfig            |  10 +
 drivers/regulator/Makefile           |   1 +
 drivers/regulator/hi6421-regulator.c | 657 +++++++++++++++++++++++++++++++++++
 3 files changed, 668 insertions(+)
 create mode 100644 drivers/regulator/hi6421-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 2dc8289..e6b98ed 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -240,6 +240,16 @@ config REGULATOR_GPIO
 	  and the platform has to provide a mapping of GPIO-states
 	  to target volts/amps.
 
+config REGULATOR_HI6421
+	tristate "HiSilicon Hi6421 PMIC voltage regulator support"
+	depends on MFD_HI6421_PMIC && OF
+	help
+	  This driver provides support for the voltage regulators on the
+	  HiSilicon Hi6421 PMU / Codec IC.
+	  Hi6421 is a multi-function device which, on regulator part, provides
+	  21 general purpose LDOs, 3 dedicated LDOs, and 5 BUCKs. All
+	  of them come with support to either ECO (idle) or sleep mode.
+
 config REGULATOR_ISL6271A
 	tristate "Intersil ISL6271A Power regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index aa4a6aa..5513e2c 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_REGULATOR_DBX500_PRCMU) += dbx500-prcmu.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_FAN53555) += fan53555.o
 obj-$(CONFIG_REGULATOR_GPIO) += gpio-regulator.o
+obj-$(CONFIG_REGULATOR_HI6421) += hi6421-regulator.o
 obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
 obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
 obj-$(CONFIG_REGULATOR_LP3972) += lp3972.o
diff --git a/drivers/regulator/hi6421-regulator.c b/drivers/regulator/hi6421-regulator.c
new file mode 100644
index 0000000..b0de92b
--- /dev/null
+++ b/drivers/regulator/hi6421-regulator.c
@@ -0,0 +1,657 @@
+/*
+ * Device driver for regulators in Hi6421 IC
+ *
+ * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ * Copyright (c) <2013-2014> Linaro Ltd.
+ *              http://www.linaro.org
+ *
+ * Author: Guodong Xu <guodong.xu@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/mfd/hi6421-pmic.h>
+#include <linux/delay.h>
+#include <linux/time.h>
+
+/*
+ * struct hi6421_regulator_pdata - Hi6421 regulator data of platform device
+ * @lock: mutex to serialize regulator enable
+ */
+struct hi6421_regulator_pdata {
+	struct mutex lock;
+};
+
+/*
+ * struct hi6421_regulator_info - hi6421 regulator information
+ * @dev: device pointer
+ * @desc: regulator description
+ * @regulator: regulator device
+ * @mode_mask: ECO mode bitmask of LDOs; for BUCKs, this masks sleep
+ * @eco_microamp: eco mode load upper limit (in mA), valid for LDOs only
+ * @valid_modes_mask: valid operating modes
+ */
+struct hi6421_regulator_info {
+	struct device		*dev;
+	struct regulator_desc	desc;
+	struct regulator_dev	*regulator;
+	u8		mode_mask;
+	u32		eco_microamp;
+	unsigned int	valid_modes_mask;
+};
+
+/* HI6421 regulators */
+enum hi6421_regulator_id {
+	HI6421_LDO0,
+	HI6421_LDO1,
+	HI6421_LDO2,
+	HI6421_LDO3,
+	HI6421_LDO4,
+	HI6421_LDO5,
+	HI6421_LDO6,
+	HI6421_LDO7,
+	HI6421_LDO8,
+	HI6421_LDO9,
+	HI6421_LDO10,
+	HI6421_LDO11,
+	HI6421_LDO12,
+	HI6421_LDO13,
+	HI6421_LDO14,
+	HI6421_LDO15,
+	HI6421_LDO16,
+	HI6421_LDO17,
+	HI6421_LDO18,
+	HI6421_LDO19,
+	HI6421_LDO20,
+	HI6421_LDOAUDIO,
+	HI6421_BUCK0,
+	HI6421_BUCK1,
+	HI6421_BUCK2,
+	HI6421_BUCK3,
+	HI6421_BUCK4,
+	HI6421_BUCK5,
+	HI6421_NUM_REGULATORS,
+};
+
+#define HI6421_REGULATOR_OF_MATCH(_name, id)				\
+{									\
+	.name = #_name,							\
+	.driver_data = (void *) HI6421_##id,				\
+}
+
+static struct of_regulator_match hi6421_regulator_match[] = {
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout0, LDO0),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout1, LDO1),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout2, LDO2),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout3, LDO3),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout4, LDO4),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout5, LDO5),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout6, LDO6),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout7, LDO7),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout8, LDO8),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout9, LDO9),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout10, LDO10),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout11, LDO11),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout12, LDO12),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout13, LDO13),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout14, LDO14),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout15, LDO15),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout16, LDO16),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout17, LDO17),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout18, LDO18),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout19, LDO19),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout20, LDO20),
+	HI6421_REGULATOR_OF_MATCH(hi6421_vout_audio, LDOAUDIO),
+	HI6421_REGULATOR_OF_MATCH(hi6421_buck0, BUCK0),
+	HI6421_REGULATOR_OF_MATCH(hi6421_buck1, BUCK1),
+	HI6421_REGULATOR_OF_MATCH(hi6421_buck2, BUCK2),
+	HI6421_REGULATOR_OF_MATCH(hi6421_buck3, BUCK3),
+	HI6421_REGULATOR_OF_MATCH(hi6421_buck4, BUCK4),
+	HI6421_REGULATOR_OF_MATCH(hi6421_buck5, BUCK5),
+};
+
+/* LDO 0, 4~7, 9~14, 16~20 have same voltage table. */
+static const unsigned int ldo_0_voltages[] = {
+	1500000, 1800000, 2400000, 2500000,
+	2600000, 2700000, 2850000, 3000000,
+};
+
+/* LDO 8, 15 have same voltage table. */
+static const unsigned int ldo_8_voltages[] = {
+	1500000, 1800000, 2400000, 2600000,
+	2700000, 2850000, 3000000, 3300000,
+};
+
+/* Ranges are sorted in ascending order. */
+static const struct regulator_linear_range ldo_audio_volt_range[] = {
+	REGULATOR_LINEAR_RANGE(2800000, 0, 3, 50000),
+	REGULATOR_LINEAR_RANGE(3000000, 4, 7, 100000),
+};
+
+static const unsigned int buck_3_voltages[] = {
+	 950000, 1050000, 1100000, 1117000,
+	1134000, 1150000, 1167000, 1200000,
+};
+
+static const unsigned int buck_4_voltages[] = {
+	1150000, 1200000, 1250000, 1350000,
+	1700000, 1800000, 1900000, 2000000,
+};
+
+static const unsigned int buck_5_voltages[] = {
+	1150000, 1200000, 1250000, 1350000,
+	1600000, 1700000, 1800000, 1900000,
+};
+
+static const struct regulator_ops hi6421_ldo_ops;
+static const struct regulator_ops hi6421_ldo_linear_ops;
+static const struct regulator_ops hi6421_ldo_linear_range_ops;
+static const struct regulator_ops hi6421_buck012_ops;
+static const struct regulator_ops hi6421_buck345_ops;
+
+#define HI6421_LDO_ENABLE_TIME (350)
+/*
+ * _id - LDO id name string
+ * v_table - voltage table
+ * vreg - voltage select register
+ * vmask - voltage select mask
+ * ereg - enable register
+ * emask - enable mask
+ * odelay - off/on delay time in uS
+ * ecomask - eco mode mask
+ * ecoamp - eco mode load uppler limit in mA
+ */
+#define HI6421_LDO(_id, v_table, vreg, vmask, ereg, emask,		\
+		   odelay, ecomask, ecoamp)				\
+	[HI6421_##_id] = {						\
+		.desc = {						\
+			.name		= #_id,				\
+			.ops		= &hi6421_ldo_ops,		\
+			.type		= REGULATOR_VOLTAGE,		\
+			.id		= HI6421_##_id,			\
+			.owner		= THIS_MODULE,			\
+			.n_voltages	= ARRAY_SIZE(v_table),		\
+			.volt_table	= v_table,			\
+			.vsel_reg	= HI6421_REG_TO_BUS_ADDR(vreg),	\
+			.vsel_mask	= vmask,			\
+			.enable_reg	= HI6421_REG_TO_BUS_ADDR(ereg),	\
+			.enable_mask	= emask,			\
+			.enable_time	= HI6421_LDO_ENABLE_TIME,	\
+			.off_on_delay	= odelay,			\
+		},							\
+		.mode_mask		= ecomask,			\
+		.eco_microamp		= ecoamp,			\
+		.valid_modes_mask	= (REGULATOR_MODE_NORMAL	\
+					   | REGULATOR_MODE_IDLE),	\
+	}
+
+/* HI6421 LDO1~3 are linear voltage regulators at fixed uV_step
+ *
+ * _id - LDO id name string
+ * _min_uV - minimum voltage supported in uV
+ * n_volt - number of votages available
+ * vstep - voltage increase in each linear step in uV
+ * vreg - voltage select register
+ * vmask - voltage select mask
+ * ereg - enable register
+ * emask - enable mask
+ * odelay - off/on delay time in uS
+ * ecomask - eco mode mask
+ * ecoamp - eco mode load uppler limit in mA
+ */
+#define HI6421_LDO_LINEAR(_id, _min_uV, n_volt, vstep, vreg, vmask,	\
+			  ereg, emask, odelay, ecomask, ecoamp)		\
+	[HI6421_##_id] = {						\
+		.desc = {						\
+			.name		= #_id,				\
+			.ops		= &hi6421_ldo_linear_ops,	\
+			.type		= REGULATOR_VOLTAGE,		\
+			.id		= HI6421_##_id,			\
+			.owner		= THIS_MODULE,			\
+			.min_uV		= _min_uV,			\
+			.n_voltages	= n_volt,			\
+			.uV_step	= vstep,			\
+			.vsel_reg	= HI6421_REG_TO_BUS_ADDR(vreg),	\
+			.vsel_mask	= vmask,			\
+			.enable_reg	= HI6421_REG_TO_BUS_ADDR(ereg),	\
+			.enable_mask	= emask,			\
+			.enable_time	= HI6421_LDO_ENABLE_TIME,	\
+			.off_on_delay	= odelay,			\
+		},							\
+		.mode_mask		= ecomask,			\
+		.eco_microamp		= ecoamp,			\
+		.valid_modes_mask	= (REGULATOR_MODE_NORMAL	\
+					   | REGULATOR_MODE_IDLE),	\
+	}
+
+/* HI6421 LDOAUDIO is a linear voltage regulator with two 4-step ranges
+ *
+ * _id - LDO id name string
+ * n_volt - number of votages available
+ * volt_ranges - array of regulator_linear_range
+ * vstep - voltage increase in each linear step in uV
+ * vreg - voltage select register
+ * vmask - voltage select mask
+ * ereg - enable register
+ * emask - enable mask
+ * odelay - off/on delay time in uS
+ * ecomask - eco mode mask
+ * ecoamp - eco mode load uppler limit in mA
+ */
+#define HI6421_LDO_LINEAR_RANGE(_id, n_volt, volt_ranges, vreg, vmask,	\
+				ereg, emask, odelay, ecomask, ecoamp)	\
+	[HI6421_##_id] = {						\
+		.desc = {						\
+			.name		= #_id,				\
+			.ops		= &hi6421_ldo_linear_range_ops,	\
+			.type		= REGULATOR_VOLTAGE,		\
+			.id		= HI6421_##_id,			\
+			.owner		= THIS_MODULE,			\
+			.n_voltages	= n_volt,			\
+			.linear_ranges	= volt_ranges,			\
+			.n_linear_ranges = ARRAY_SIZE(volt_ranges),	\
+			.vsel_reg	= HI6421_REG_TO_BUS_ADDR(vreg),	\
+			.vsel_mask	= vmask,			\
+			.enable_reg	= HI6421_REG_TO_BUS_ADDR(ereg),	\
+			.enable_mask	= emask,			\
+			.enable_time	= HI6421_LDO_ENABLE_TIME,	\
+			.off_on_delay	= odelay,			\
+		},							\
+		.mode_mask		= ecomask,			\
+		.eco_microamp		= ecoamp,			\
+		.valid_modes_mask	= (REGULATOR_MODE_NORMAL	\
+					   | REGULATOR_MODE_IDLE),	\
+	}
+
+/* HI6421 BUCK0/1/2 are linear voltage regulators at fixed uV_step
+ *
+ * _id - BUCK0/1/2 id name string
+ * vreg - voltage select register
+ * vmask - voltage select mask
+ * ereg - enable register
+ * emask - enable mask
+ * sleepmask - mask of sleep mode
+ * etime - enable time
+ * odelay - off/on delay time in uS
+ */
+#define HI6421_BUCK012(_id, vreg, vmask, ereg, emask, sleepmask,	\
+			etime, odelay)					\
+	[HI6421_##_id] = {						\
+		.desc = {						\
+			.name		= #_id,				\
+			.ops		= &hi6421_buck012_ops,		\
+			.type		= REGULATOR_VOLTAGE,		\
+			.id		= HI6421_##_id,			\
+			.owner		= THIS_MODULE,			\
+			.min_uV		= 700000,			\
+			.n_voltages	= 128,				\
+			.uV_step	= 7086,				\
+			.vsel_reg	= HI6421_REG_TO_BUS_ADDR(vreg),	\
+			.vsel_mask	= vmask,			\
+			.enable_reg	= HI6421_REG_TO_BUS_ADDR(ereg),	\
+			.enable_mask	= emask,			\
+			.enable_time	= etime,			\
+			.off_on_delay	= odelay,			\
+		},							\
+		.mode_mask		= sleepmask,			\
+		.valid_modes_mask	= (REGULATOR_MODE_NORMAL	\
+					   | REGULATOR_MODE_STANDBY),	\
+	}
+
+/* HI6421 BUCK3/4/5 share similar configurations as LDOs, with exception
+ *  that it supports SLEEP mode, so has different .ops.
+ *
+ * _id - LDO id name string
+ * v_table - voltage table
+ * vreg - voltage select register
+ * vmask - voltage select mask
+ * ereg - enable register
+ * emask - enable mask
+ * odelay - off/on delay time in uS
+ * sleepmask - mask of sleep mode
+ */
+#define HI6421_BUCK345(_id, v_table, vreg, vmask, ereg, emask,		\
+			odelay, sleepmask)				\
+	[HI6421_##_id] = {						\
+		.desc = {						\
+			.name		= #_id,				\
+			.ops		= &hi6421_buck345_ops,		\
+			.type		= REGULATOR_VOLTAGE,		\
+			.id		= HI6421_##_id,			\
+			.owner		= THIS_MODULE,			\
+			.n_voltages	= ARRAY_SIZE(v_table),		\
+			.volt_table	= v_table,			\
+			.vsel_reg	= HI6421_REG_TO_BUS_ADDR(vreg),	\
+			.vsel_mask	= vmask,			\
+			.enable_reg	= HI6421_REG_TO_BUS_ADDR(ereg),	\
+			.enable_mask	= emask,			\
+			.enable_time	= HI6421_LDO_ENABLE_TIME,	\
+			.off_on_delay	= odelay,			\
+		},							\
+		.mode_mask		= sleepmask,			\
+		.valid_modes_mask	= (REGULATOR_MODE_NORMAL	\
+					   | REGULATOR_MODE_STANDBY),	\
+	}
+
+/* HI6421 regulator information */
+static struct hi6421_regulator_info
+		hi6421_regulator_info[HI6421_NUM_REGULATORS] = {
+	HI6421_LDO(LDO0, ldo_0_voltages, 0x20, 0x07, 0x20, 0x10,
+		   10000, 0x20, 8000),
+	HI6421_LDO_LINEAR(LDO1, 1700000, 4, 100000, 0x21, 0x03, 0x21, 0x10,
+			  10000, 0x20, 5000),
+	HI6421_LDO_LINEAR(LDO2, 1050000, 8, 50000, 0x22, 0x07, 0x22, 0x10,
+			  20000, 0x20, 8000),
+	HI6421_LDO_LINEAR(LDO3, 1050000, 8, 50000, 0x23, 0x07, 0x23, 0x10,
+			  20000, 0x20, 8000),
+	HI6421_LDO(LDO4, ldo_0_voltages, 0x24, 0x07, 0x24, 0x10,
+		   20000, 0x20, 8000),
+	HI6421_LDO(LDO5, ldo_0_voltages, 0x25, 0x07, 0x25, 0x10,
+		   20000, 0x20, 8000),
+	HI6421_LDO(LDO6, ldo_0_voltages, 0x26, 0x07, 0x26, 0x10,
+		   20000, 0x20, 8000),
+	HI6421_LDO(LDO7, ldo_0_voltages, 0x27, 0x07, 0x27, 0x10,
+		   20000, 0x20, 5000),
+	HI6421_LDO(LDO8, ldo_8_voltages, 0x28, 0x07, 0x28, 0x10,
+		   20000, 0x20, 8000),
+	HI6421_LDO(LDO9, ldo_0_voltages, 0x29, 0x07, 0x29, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO10, ldo_0_voltages, 0x2a, 0x07, 0x2a, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO11, ldo_0_voltages, 0x2b, 0x07, 0x2b, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO12, ldo_0_voltages, 0x2c, 0x07, 0x2c, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO13, ldo_0_voltages, 0x2d, 0x07, 0x2d, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO14, ldo_0_voltages, 0x2e, 0x07, 0x2e, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO15, ldo_8_voltages, 0x2f, 0x07, 0x2f, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO16, ldo_0_voltages, 0x30, 0x07, 0x30, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO17, ldo_0_voltages, 0x31, 0x07, 0x31, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO18, ldo_0_voltages, 0x32, 0x07, 0x32, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO19, ldo_0_voltages, 0x33, 0x07, 0x33, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO(LDO20, ldo_0_voltages, 0x34, 0x07, 0x34, 0x10,
+		   40000, 0x20, 8000),
+	HI6421_LDO_LINEAR_RANGE(LDOAUDIO, 8, ldo_audio_volt_range, 0x36,
+				0x70, 0x36, 0x01, 40000, 0x02, 5000),
+	HI6421_BUCK012(BUCK0, 0x0d, 0x7f, 0x0c, 0x01, 0x10, 400, 20000),
+	HI6421_BUCK012(BUCK1, 0x0f, 0x7f, 0x0e, 0x01, 0x10, 400, 20000),
+	HI6421_BUCK012(BUCK2, 0x11, 0x7f, 0x10, 0x01, 0x10, 350, 100),
+	HI6421_BUCK345(BUCK3, buck_3_voltages, 0x13, 0x07, 0x12, 0x01,
+		       20000, 0x10),
+	HI6421_BUCK345(BUCK4, buck_4_voltages, 0x15, 0x07, 0x14, 0x01,
+		       20000, 0x10),
+	HI6421_BUCK345(BUCK5, buck_5_voltages, 0x17, 0x07, 0x16, 0x01,
+		       20000, 0x10),
+};
+
+static int hi6421_regulator_enable(struct regulator_dev *rdev)
+{
+	struct hi6421_regulator_pdata *pdata;
+
+	pdata = dev_get_drvdata(rdev->dev.parent);
+	/* hi6421 spec requires regulator enablement must be serialized:
+	 *  - Because when BUCK, LDO switching from off to on, it will have
+	 *    a huge instantaneous current; so you can not turn on two or
+	 *    more LDO or BUCKs simultaneously, or it may burn the chip.
+	 */
+	mutex_lock(&pdata->lock);
+
+	/* call regulator regmap helper */
+	regulator_enable_regmap(rdev);
+
+	mutex_unlock(&pdata->lock);
+	return 0;
+}
+
+static unsigned int hi6421_regulator_ldo_get_mode(struct regulator_dev *rdev)
+{
+	struct hi6421_regulator_info *info = rdev_get_drvdata(rdev);
+	u32 reg_val;
+
+	regmap_read(rdev->regmap, rdev->desc->enable_reg, &reg_val);
+	if (reg_val & info->mode_mask)
+		return REGULATOR_MODE_IDLE;
+
+	return REGULATOR_MODE_NORMAL;
+}
+
+static unsigned int hi6421_regulator_buck_get_mode(struct regulator_dev *rdev)
+{
+	struct hi6421_regulator_info *info = rdev_get_drvdata(rdev);
+	u32 reg_val;
+
+	regmap_read(rdev->regmap, rdev->desc->enable_reg, &reg_val);
+	if (reg_val & info->mode_mask)
+		return REGULATOR_MODE_STANDBY;
+
+	return REGULATOR_MODE_NORMAL;
+}
+
+static int hi6421_regulator_ldo_set_mode(struct regulator_dev *rdev,
+						unsigned int mode)
+{
+	struct hi6421_regulator_info *info = rdev_get_drvdata(rdev);
+	u32 new_mode;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		new_mode = 0;
+		break;
+	case REGULATOR_MODE_IDLE:
+		new_mode = info->mode_mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* set mode */
+	regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			   info->mode_mask, new_mode);
+
+	return 0;
+}
+
+static int hi6421_regulator_buck_set_mode(struct regulator_dev *rdev,
+						unsigned int mode)
+{
+	struct hi6421_regulator_info *info = rdev_get_drvdata(rdev);
+	u32 new_mode;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		new_mode = 0;
+		break;
+	case REGULATOR_MODE_STANDBY:
+		new_mode = info->mode_mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* set mode */
+	regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			   info->mode_mask, new_mode);
+
+	return 0;
+}
+
+unsigned int hi6421_regulator_ldo_get_optimum_mode(struct regulator_dev *rdev,
+			int input_uV, int output_uV, int load_uA)
+{
+	struct hi6421_regulator_info *info = rdev_get_drvdata(rdev);
+
+	if (load_uA > info->eco_microamp)
+		return REGULATOR_MODE_NORMAL;
+
+	return REGULATOR_MODE_IDLE;
+}
+
+static const struct regulator_ops hi6421_ldo_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = hi6421_regulator_enable,
+	.disable = regulator_disable_regmap,
+	.list_voltage = regulator_list_voltage_table,
+	.map_voltage = regulator_map_voltage_ascend,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_mode = hi6421_regulator_ldo_get_mode,
+	.set_mode = hi6421_regulator_ldo_set_mode,
+	.get_optimum_mode = hi6421_regulator_ldo_get_optimum_mode,
+};
+
+static const struct regulator_ops hi6421_ldo_linear_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = hi6421_regulator_enable,
+	.disable = regulator_disable_regmap,
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_mode = hi6421_regulator_ldo_get_mode,
+	.set_mode = hi6421_regulator_ldo_set_mode,
+	.get_optimum_mode = hi6421_regulator_ldo_get_optimum_mode,
+};
+
+static const struct regulator_ops hi6421_ldo_linear_range_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = hi6421_regulator_enable,
+	.disable = regulator_disable_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.map_voltage = regulator_map_voltage_linear_range,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_mode = hi6421_regulator_ldo_get_mode,
+	.set_mode = hi6421_regulator_ldo_set_mode,
+	.get_optimum_mode = hi6421_regulator_ldo_get_optimum_mode,
+};
+
+static const struct regulator_ops hi6421_buck012_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = hi6421_regulator_enable,
+	.disable = regulator_disable_regmap,
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_mode = hi6421_regulator_buck_get_mode,
+	.set_mode = hi6421_regulator_buck_set_mode,
+};
+
+static const struct regulator_ops hi6421_buck345_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = hi6421_regulator_enable,
+	.disable = regulator_disable_regmap,
+	.list_voltage = regulator_list_voltage_table,
+	.map_voltage = regulator_map_voltage_ascend,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_mode = hi6421_regulator_buck_get_mode,
+	.set_mode = hi6421_regulator_buck_set_mode,
+};
+
+static int hi6421_regulator_register(struct platform_device *pdev,
+				     struct regmap *rmap,
+				     struct regulator_init_data *init_data,
+				     int id, struct device_node *np)
+{
+	struct hi6421_regulator_info *info = NULL;
+	struct regulator_config config = { };
+
+	/* assign per-regulator data */
+	info = &hi6421_regulator_info[id];
+	info->dev = &pdev->dev;
+
+	config.dev = &pdev->dev;
+	config.init_data = init_data;
+	config.driver_data = info;
+	config.regmap = rmap;
+	config.of_node = np;
+
+	/* register regulator with framework */
+	info->regulator = devm_regulator_register(&pdev->dev, &info->desc,
+						&config);
+	if (IS_ERR(info->regulator)) {
+		dev_err(&pdev->dev, "failed to register regulator %s\n",
+			info->desc.name);
+		return PTR_ERR(info->regulator);
+	}
+
+	return 0;
+}
+
+static int hi6421_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np;
+	struct hi6421_pmic *pmic;
+	struct hi6421_regulator_pdata *pdata;
+	int i, ret = 0;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+	mutex_init(&pdata->lock);
+	platform_set_drvdata(pdev, pdata);
+
+	np = of_get_child_by_name(dev->parent->of_node, "regulators");
+	if (!np)
+		return -ENODEV;
+
+	ret = of_regulator_match(dev, np,
+				 hi6421_regulator_match,
+				 ARRAY_SIZE(hi6421_regulator_match));
+	of_node_put(np);
+	if (ret < 0) {
+		dev_err(dev, "Error parsing regulator init data: %d\n", ret);
+		return ret;
+	}
+
+	pmic = dev_get_drvdata(dev->parent);
+
+	for (i = 0; i < ARRAY_SIZE(hi6421_regulator_info); i++) {
+		ret = hi6421_regulator_register(pdev, pmic->regmap,
+			hi6421_regulator_match[i].init_data, i,
+			hi6421_regulator_match[i].of_node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver hi6421_regulator_driver = {
+	.driver = {
+		.name	= "hi6421-regulator",
+		.owner  = THIS_MODULE,
+	},
+	.probe	= hi6421_regulator_probe,
+};
+module_platform_driver(hi6421_regulator_driver);
+
+MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
+MODULE_DESCRIPTION("Hi6421 regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v6 6/6] ARM: dts: hi3620-hi4511: Add HI6421 MFD and regulator nodes

[Guodong Xu]

Add Hi6421 MFD dts node and regulator nodes into hi3620-hi4511
board config dts file.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 arch/arm/boot/dts/hi3620-hi4511.dts | 233 ++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

diff --git a/arch/arm/boot/dts/hi3620-hi4511.dts b/arch/arm/boot/dts/hi3620-hi4511.dts
index fe62392..312c3f9 100644
--- a/arch/arm/boot/dts/hi3620-hi4511.dts
+++ b/arch/arm/boot/dts/hi3620-hi4511.dts
@@ -635,6 +635,239 @@
 				pinctrl-single,bias-pullup = <0 1 0 1>;
 			};
 		};
+
+		pmic: pmic@c00000 {
+			compatible = "hisilicon,hi6421-pmic";
+			reg = <0xc00000 0x0180>; /* 0x60 << 2 */
+
+			regulators {
+				// supply for MLC NAND/ eMMC
+				hi6421_vout0_reg: hi6421_vout0 {
+					regulator-name = "VOUT0";
+					regulator-min-microvolt = <2850000>;
+					regulator-max-microvolt = <2850000>;
+				};
+
+				// supply for 26M Oscillator
+				hi6421_vout1_reg: hi6421_vout1 {
+					regulator-name = "VOUT1";
+					regulator-min-microvolt = <1700000>;
+					regulator-max-microvolt = <2000000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for AP system
+				hi6421_vout2_reg: hi6421_vout2 {
+					regulator-name = "VOUT2";
+					regulator-min-microvolt = <1050000>;
+					regulator-max-microvolt = <1400000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for DDR PHY I/O
+				hi6421_vout3_reg: hi6421_vout3 {
+					regulator-name = "VOUT3";
+					regulator-min-microvolt = <1050000>;
+					regulator-max-microvolt = <1400000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for MIPI, USB2.0, PCIe, PLL Analog
+				hi6421_vout4_reg: hi6421_vout4 {
+					regulator-name = "VOUT4";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for I/O 1.8V
+				hi6421_vout5_reg: hi6421_vout5 {
+					regulator-name = "VOUT5";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for I/O 2.6V
+				hi6421_vout6_reg: hi6421_vout6 {
+					regulator-name = "VOUT6";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for SD I/O
+				hi6421_vout7_reg: hi6421_vout7 {
+					regulator-name = "VOUT7";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for USB PHY
+				hi6421_vout8_reg: hi6421_vout8 {
+					regulator-name = "VOUT8";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3300000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for Efuse power
+				hi6421_vout9_reg: hi6421_vout9 {
+					regulator-name = "VOUT9";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for BT RF&PA
+				hi6421_vout10_reg: hi6421_vout10 {
+					regulator-name = "VOUT10";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for GPS RF&PA
+				hi6421_vout11_reg: hi6421_vout11 {
+					regulator-name = "VOUT11";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for SD card
+				hi6421_vout12_reg: hi6421_vout12 {
+					regulator-name = "VOUT12";
+					regulator-min-microvolt = <2850000>;
+					regulator-max-microvolt = <2850000>;
+				};
+
+				// supply for CMMB
+				hi6421_vout13_reg: hi6421_vout13 {
+					regulator-name = "VOUT13";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for WIFI I/O
+				hi6421_vout14_reg: hi6421_vout14 {
+					regulator-name = "VOUT14";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for WIFI core & PA
+				hi6421_vout15_reg: hi6421_vout15 {
+					regulator-name = "VOUT15";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				// supply for LCD I/O
+				hi6421_vout16_reg: hi6421_vout16 {
+					regulator-name = "VOUT16";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+					regulator-always-on;
+				};
+
+				// supply for LCD Ananlog
+				hi6421_vout17_reg: hi6421_vout17 {
+					regulator-name = "VOUT17";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+					regulator-always-on;
+				};
+
+				// supply for Camera I/O
+				hi6421_vout18_reg: hi6421_vout18 {
+					regulator-name = "VOUT18";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for Camera Analog
+				hi6421_vout19_reg: hi6421_vout19 {
+					regulator-name = "VOUT19";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for Camera VCM
+				hi6421_vout20_reg: hi6421_vout20 {
+					regulator-name = "VOUT20";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <3000000>;
+				};
+
+				// supply for Audio, incl. HKADC
+				hi6421_vout_audio_reg: hi6421_vout_audio {
+					regulator-name = "AVDD2";
+					regulator-min-microvolt = <2800000>;
+					regulator-max-microvolt = <3300000>;
+				};
+
+				// supply for Cortex-A9 CPU core
+				hi6421_buck0_reg: hi6421_buck0 {
+					regulator-name = "VBUCK0";
+					regulator-min-microvolt = <700000>;
+					regulator-max-microvolt = <1600000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for Cortex-A9 CPU core
+				hi6421_buck1_reg: hi6421_buck1 {
+					regulator-name = "VBUCK1";
+					regulator-min-microvolt = <700000>;
+					regulator-max-microvolt = <1600000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for GPU
+				hi6421_buck2_reg: hi6421_buck2 {
+					regulator-name = "VBUCK2";
+					regulator-min-microvolt = <700000>;
+					regulator-max-microvolt = <1600000>;
+					regulator-boot-on;
+				};
+
+				// supply for Peripheral
+				hi6421_buck3_reg: hi6421_buck3 {
+					regulator-name = "VBUCK3";
+					regulator-min-microvolt = <950000>;
+					regulator-max-microvolt = <1200000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for LPDDR2 and AP I/O
+				hi6421_buck4_reg: hi6421_buck4 {
+					regulator-name = "VBUCK4";
+					regulator-min-microvolt = <1150000>;
+					regulator-max-microvolt = <2000000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+
+				// supply for Low voltage
+				hi6421_buck5_reg: hi6421_buck5 {
+					regulator-name = "VBUCK5";
+					regulator-min-microvolt = <1150000>;
+					regulator-max-microvolt = <1900000>;
+					regulator-boot-on;
+					regulator-always-on;
+				};
+			};
+		};
+
 	};
 
 	gpio-keys {
-- 
1.9.1

Re: [PATCH v6 4/6] mfd: Add hi6421 PMIC core driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

On Mon, Aug 18, 2014 at 09:09:14PM +0800, Guodong Xu wrote:
> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> This driver includes core APIs _only_.

Please don't resend already applied patches, if there are any updates
that need applying please send incremental patches.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 4/6] mfd: Add hi6421 PMIC core driver

[Lee Jones]

On Mon, 18 Aug 2014, Guodong Xu wrote:
> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> This driver includes core APIs _only_.
> 
> Drivers for individul components, like voltage regulators, are
> implemented in corresponding driver directories and files.
> 
> Registers in Hi6421 are memory mapped, so using regmap-mmio API.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++

DT documentation should be in a patch of its own.

See: Documentation/devicetree/bindings/submitting-patches.txt

>  drivers/mfd/Kconfig                              |  13 +++
>  drivers/mfd/Makefile                             |   1 +
>  drivers/mfd/hi6421-pmic-core.c                   | 117 +++++++++++++++++++++++
>  include/linux/mfd/hi6421-pmic.h                  |  39 ++++++++
>  5 files changed, 207 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
>  create mode 100644 drivers/mfd/hi6421-pmic-core.c
>  create mode 100644 include/linux/mfd/hi6421-pmic.h
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hi6421.txt b/Documentation/devicetree/bindings/mfd/hi6421.txt
> new file mode 100644
> index 0000000..1512123
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hi6421.txt
> @@ -0,0 +1,37 @@
> +* HI6421 Multi-Functional Device (MFD), by HiSilicon Ltd.
> +
> +Required parent device properties:
> +- reg		: register range space of hi6421;

You need to describe the compatible string here.

> +Supported Hi6421 sub-devices include:
> +
> +Device                     IRQ Names              Supply Names   Description
> +------                     ---------              ------------   -----------
> +regulators               :                      :              : Regulators

If a cell has been intentionally left blank, put 'None' to be clear.

When will the other devices be added?

> +Required child device properties:
> +None.
> +
> +Example:
> +	pmic: pmic@c00000 {

24bit addressing?

Is this node referenced by another node via phandle?  If not, you can
drop the "pmic" label.  If it is referenced by another node, but there
is more than one PMIC, label "pmic" won't do.

> +		compatible = "hisilicon,hi6421-pmic";
> +		reg = <0xc00000 0x0180>; /* 0x60 << 2 */
> +
> +		regulators {
> +			// supply for MLC NAND/ eMMC
> +			hi6421_vout0_reg: hi6421_vout0 {
> +				regulator-name = "VOUT0";

I may be mistaken, but "vout0" doesn't come across as a very good
regulator name to me.

> +				regulator-min-microvolt = <2850000>;
> +				regulator-max-microvolt = <2850000>;
> +			};
> +
> +			// supply for 26M Oscillator
> +			hi6421_vout1_reg: hi6421_vout1 {
> +				regulator-name = "VOUT1";
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <2000000>;
> +				regulator-boot-on;
> +				regulator-always-on;

Again an assumption here, but doesn't 'egulator-always-on' insinuate
'regulator-boot-on', or does it simply mean "once it's on, it must
stay on"?

> +			};
> +		};
> +       	};

Your tabbing is out here.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..347cbf6 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -210,6 +210,19 @@ config MFD_MC13XXX_I2C
>  	help
>  	  Select this if your MC13xxx is connected via an I2C bus.
>  
> +config MFD_HI6421_PMIC
> +	tristate "HiSilicon Hi6421 PMU/Codec IC"
> +	depends on OF
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	help
> +	  This driver supports HiSilicon Hi6421 PMIC. Hi6421 includes multi-

It doesn't support it, it adds support for it - subtle difference.

> +	  functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> +	  This driver includes core APIs _only_. You have to select
> +	  individul components like voltage regulators under corresponding
> +	  menus in order to enable them.
> +	  Memory-mapped I/O is the way to communicate with Hi6421.

"We communicate with the Hi6421 via memory-mapped I/O." 

>  config HTC_EGPIO
>  	bool "HTC EGPIO support"
>  	depends on GPIOLIB && ARM
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index f001487..dc59efd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> +obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> new file mode 100644
> index 0000000..2be4d3f
> --- /dev/null
> +++ b/drivers/mfd/hi6421-pmic-core.c
> @@ -0,0 +1,117 @@
> +/*
> + * Device driver for Hi6421 IC
> + *
> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
> + *              http://www.hisilicon.com
> + * Copyright (c) <2013-2014> Linaro Ltd.
> + *              http://www.linaro.org

I'm not sure Linaro own the copyright to this driver.  It should still
belong to HiSilicon.

> + * Author: Guodong Xu <guodong.xu@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

This should also contain a link to the full licence.

See: COPYING

> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/hi6421-pmic.h>
> +
> +static struct of_device_id of_hi6421_pmic_match_tbl[] = {
> +	{
> +		.compatible = "hisilicon,hi6421-pmic",
> +	},

This can be just one line.

> +	{ /* end */ }

No real need for this comment.

> +};
> +
> +static const struct mfd_cell hi6421_devs[] = {
> +	{
> +		.name = "hi6421-regulator",
> +	},

Again, one line.

> +};
> +
> +static struct regmap_config hi6421_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 8,
> +	.max_register = HI6421_REG_TO_BUS_ADDR(0xFF),

0xFF should really be defined.

> +};
> +
> +static int hi6421_pmic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;

You only use this a couple of times.  You use '&pdev->dev' more
regularly. May as well remove it and just use '&pdev->dev' all
the time.

> +	struct hi6421_pmic *pmic = NULL;

No need to initialise this.

> +	struct resource *res;
> +	void __iomem *base;
> +	int ret;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic) {
> +		dev_err(dev, "cannot allocate hi6421_pmic device info\n");

No need to print out messages on OOM, the OS will do that for you.

> +		return -ENOMEM;
> +	}
> +
> +	/* get resources */

This comment doesn't add to the code.  The name of the function call
is clear enough.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (unlikely(!res)) {

Only 21 out of 1718 uses of platform_get_resource() use unlikely() to
check the return value.  I think it's okay to drop it.

> +		dev_err(&pdev->dev, "Invalid mem resource.\n");

"No memory resource specified"?

> +		return -ENODEV;
> +	}

Actually, scrap all that.  You can remove error checking altogether
for platform_get_resource(), as devm_ioremap_resource() does it for
you.  Code should read:

  res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  base = devm_ioremap_resource(dev, res);
  if (IS_ERR(base))
	  return PTR_ERR(base);

> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
> +						 &hi6421_regmap_config);
> +	if (IS_ERR(pmic->regmap)) {
> +		dev_err(&pdev->dev, "regmap init failed: %ld\n",
> +						PTR_ERR(pmic->regmap));

Can you break the line at the first ',/' and line up against '('?

> +		return PTR_ERR(pmic->regmap);
> +	}
> +
> +	pmic->dev = dev;
> +	platform_set_drvdata(pdev, pmic);

It's not _that_ important, but I like to see this at the end after you
know everything else has succeeded.

> +	/* set over-current protection debounce 8ms*/

Should be a ' ' between '8ms' and '*/'.

> +	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
> +		(HI6421_OCP_DEB_SEL_MASK | HI6421_OCP_EN_DEBOUNCE_MASK |
> +		 HI6421_OCP_AUTO_STOP_MASK),
> +		(HI6421_OCP_DEB_SEL_8MS | HI6421_OCP_EN_DEBOUNCE_ENABLE));
> +
> +	ret = mfd_add_devices(dev, 0, hi6421_devs,
> +			ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);

I'd like to see an error message if mfd_add_devices() fails.

> +	return ret;
> +}
> +
> +static int hi6421_pmic_remove(struct platform_device *pdev)
> +{
> +	mfd_remove_devices(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver hi6421_pmic_driver = {
> +	.driver = {
> +		.name	= "hi6421_pmic",
> +		.owner  = THIS_MODULE,

This is taken care of for you, you can remove it.

> +		.of_match_table = of_hi6421_pmic_match_tbl,
> +	},
> +	.probe	= hi6421_pmic_probe,
> +	.remove	= hi6421_pmic_remove,
> +};
> +module_platform_driver(hi6421_pmic_driver);
> +
> +MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
> +MODULE_DESCRIPTION("Hi6421 PMIC driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
> new file mode 100644
> index 0000000..36bcb34
> --- /dev/null
> +++ b/include/linux/mfd/hi6421-pmic.h
> @@ -0,0 +1,39 @@
> +/*
> + * Header file for device driver Hi6421 PMIC
> + *
> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
> + *              http://www.hisilicon.com
> + * Copyright (c) <2013-2014> Linaro Ltd.
> + *              http://www.linaro.org
> + *
> + * Author: Guodong Xu <guodong.xu@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef	__HI6421_PMIC_H
> +#define	__HI6421_PMIC_H
> +
> +/* Hi6421 registers are mapped to memory bus in 4 bytes stride */
> +#define HI6421_REG_TO_BUS_ADDR(x)	(x << 2)
> +
> +/* Hi6421 OCP (over current protection) and DEB (debounce) control register */
> +#define	HI6421_OCP_DEB_CTRL_REG		HI6421_REG_TO_BUS_ADDR(0x51)
> +#define	HI6421_OCP_DEB_SEL_MASK		(0x0C)
> +#define HI6421_OCP_DEB_SEL_8MS		(0x00)
> +#define HI6421_OCP_DEB_SEL_16MS		(0x04)
> +#define HI6421_OCP_DEB_SEL_32MS		(0x08)
> +#define HI6421_OCP_DEB_SEL_64MS		(0x0C)
> +#define HI6421_OCP_EN_DEBOUNCE_MASK	(0x02)
> +#define HI6421_OCP_EN_DEBOUNCE_ENABLE	(0x02)
> +#define HI6421_OCP_AUTO_STOP_MASK	(0x01)
> +#define HI6421_OCP_AUTO_STOP_ENABLE	(0x01)

Remove the ()'s.

> +struct hi6421_pmic {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +};
> +
> +#endif		/* __HI6421_PMIC_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v6 4/6] mfd: Add hi6421 PMIC core driver

[Guodong Xu]

Hi, Lee

Thanks. I added for most of your comments. Some items, I have a
different understanding. I added in below.

On 08/20/2014 04:09 PM, Lee Jones wrote:
> On Mon, 18 Aug 2014, Guodong Xu wrote:
>> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
>> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
>> This driver includes core APIs _only_.
>>
>> Drivers for individul components, like voltage regulators, are
>> implemented in corresponding driver directories and files.
>>
>> Registers in Hi6421 are memory mapped, so using regmap-mmio API.
>>
>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++
> 
> DT documentation should be in a patch of its own.
> 
> See: Documentation/devicetree/bindings/submitting-patches.txt
> 

Thanks. Will do.

>>  drivers/mfd/Kconfig                              |  13 +++
>>  drivers/mfd/Makefile                             |   1 +
>>  drivers/mfd/hi6421-pmic-core.c                   | 117 +++++++++++++++++++++++
>>  include/linux/mfd/hi6421-pmic.h                  |  39 ++++++++
>>  5 files changed, 207 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hi6421.txt
>>  create mode 100644 drivers/mfd/hi6421-pmic-core.c
>>  create mode 100644 include/linux/mfd/hi6421-pmic.h
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hi6421.txt b/Documentation/devicetree/bindings/mfd/hi6421.txt
>> new file mode 100644
>> index 0000000..1512123
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hi6421.txt
>> @@ -0,0 +1,37 @@
>> +* HI6421 Multi-Functional Device (MFD), by HiSilicon Ltd.
>> +
>> +Required parent device properties:
>> +- reg		: register range space of hi6421;
> 
> You need to describe the compatible string here.
> 

Yes.

>> +Supported Hi6421 sub-devices include:
>> +
>> +Device                     IRQ Names              Supply Names   Description
>> +------                     ---------              ------------   -----------
>> +regulators               :                      :              : Regulators
> 
> If a cell has been intentionally left blank, put 'None' to be clear.
> 
> When will the other devices be added?
> 

RTC and irq will follow up next Month.

>> +Required child device properties:
>> +None.
>> +
>> +Example:
>> +	pmic: pmic@c00000 {
> 
> 24bit addressing?
> 

No. that's due to a range property in parent node. I will change it to
32-bit address.

> Is this node referenced by another node via phandle?  If not, you can
> drop the "pmic" label.  If it is referenced by another node, but there
> is more than one PMIC, label "pmic" won't do.
> 

ok. Will change node name to
 +               hi6421 {
...

>> +		compatible = "hisilicon,hi6421-pmic";
>> +		reg = <0xc00000 0x0180>; /* 0x60 << 2 */
>> +
>> +		regulators {
>> +			// supply for MLC NAND/ eMMC
>> +			hi6421_vout0_reg: hi6421_vout0 {
>> +				regulator-name = "VOUT0";
> 
> I may be mistaken, but "vout0" doesn't come across as a very good
> regulator name to me.
>

Right, but 'VOUT0' is the name used in hi4511 boards' schematic. So I
used this name to facilitate dts and board design matching.


>> +				regulator-min-microvolt = <2850000>;
>> +				regulator-max-microvolt = <2850000>;
>> +			};
>> +
>> +			// supply for 26M Oscillator
>> +			hi6421_vout1_reg: hi6421_vout1 {
>> +				regulator-name = "VOUT1";
>> +				regulator-min-microvolt = <1700000>;
>> +				regulator-max-microvolt = <2000000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
> 
> Again an assumption here, but doesn't 'egulator-always-on' insinuate
> 'regulator-boot-on', or does it simply mean "once it's on, it must
> stay on"?
>

>From Documentation/devicetree/bindings/regulator/regulator.txt,
- regulator-boot-on: bootloader/firmware enabled regulator
- regulator-always-on: boolean, regulator should never be disabled

'regulator-boot-on' is describing a board status, so I added it into dts
file. Although in kernel code there is no much operation depending on it.

>> +			};
>> +		};
>> +       	};
> 
> Your tabbing is out here.
> 

Sorry. will fix.

>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index de5abf2..347cbf6 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -210,6 +210,19 @@ config MFD_MC13XXX_I2C
>>  	help
>>  	  Select this if your MC13xxx is connected via an I2C bus.
>>  
>> +config MFD_HI6421_PMIC
>> +	tristate "HiSilicon Hi6421 PMU/Codec IC"
>> +	depends on OF
>> +	select MFD_CORE
>> +	select REGMAP_MMIO
>> +	help
>> +	  This driver supports HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> 
> It doesn't support it, it adds support for it - subtle difference.
> 

Thanks, will fix.

>> +	  functions, such as regulators, codec, ADCs, Coulomb counter, etc.
>> +	  This driver includes core APIs _only_. You have to select
>> +	  individul components like voltage regulators under corresponding
>> +	  menus in order to enable them.
>> +	  Memory-mapped I/O is the way to communicate with Hi6421.
> 
> "We communicate with the Hi6421 via memory-mapped I/O." 
> 
>>  config HTC_EGPIO
>>  	bool "HTC EGPIO support"
>>  	depends on GPIOLIB && ARM
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index f001487..dc59efd 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
>> +obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
>>  
>>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
>> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
>> new file mode 100644
>> index 0000000..2be4d3f
>> --- /dev/null
>> +++ b/drivers/mfd/hi6421-pmic-core.c
>> @@ -0,0 +1,117 @@
>> +/*
>> + * Device driver for Hi6421 IC
>> + *
>> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
>> + *              http://www.hisilicon.com
>> + * Copyright (c) <2013-2014> Linaro Ltd.
>> + *              http://www.linaro.org
> 
> I'm not sure Linaro own the copyright to this driver.  It should still
> belong to HiSilicon.
> 

That was discussed with HiSilicon and agreement was we need both. The
driver code is a complete re-write by me. HiSilicon original driver is
reference for functionality understanding purpose.

>> + * Author: Guodong Xu <guodong.xu@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
> 
> This should also contain a link to the full licence.
> 
> See: COPYING
> 

Thanks. I checked COPYING, but there is no 'link' to full license. I
copied a link from other c source: http://www.gnu.org/licenses/
is that OK?

>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/hi6421-pmic.h>
>> +
>> +static struct of_device_id of_hi6421_pmic_match_tbl[] = {
>> +	{
>> +		.compatible = "hisilicon,hi6421-pmic",
>> +	},
> 
> This can be just one line.
> 
>> +	{ /* end */ }
> 
> No real need for this comment.
> 

Fixed.

>> +};
>> +
>> +static const struct mfd_cell hi6421_devs[] = {
>> +	{
>> +		.name = "hi6421-regulator",
>> +	},
> 
> Again, one line.
> 
>> +};
>> +
>> +static struct regmap_config hi6421_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 8,
>> +	.max_register = HI6421_REG_TO_BUS_ADDR(0xFF),
> 
> 0xFF should really be defined.
> 
>> +};
>> +
>> +static int hi6421_pmic_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
> 
> You only use this a couple of times.  You use '&pdev->dev' more
> regularly. May as well remove it and just use '&pdev->dev' all
> the time.
> 

Right. Will fix.

>> +	struct hi6421_pmic *pmic = NULL;
> 
> No need to initialise this.
> 
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	int ret;
>> +
>> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
>> +	if (!pmic) {
>> +		dev_err(dev, "cannot allocate hi6421_pmic device info\n");
> 
> No need to print out messages on OOM, the OS will do that for you.
> 
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* get resources */
> 
> This comment doesn't add to the code.  The name of the function call
> is clear enough.
> 
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (unlikely(!res)) {
> 
> Only 21 out of 1718 uses of platform_get_resource() use unlikely() to
> check the return value.  I think it's okay to drop it.
> 
>> +		dev_err(&pdev->dev, "Invalid mem resource.\n");
> 
> "No memory resource specified"?
> 
>> +		return -ENODEV;
>> +	}
> 
> Actually, scrap all that.  You can remove error checking altogether
> for platform_get_resource(), as devm_ioremap_resource() does it for
> you.  Code should read:
> 

Thanks. Will do.

>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   base = devm_ioremap_resource(dev, res);
>   if (IS_ERR(base))
> 	  return PTR_ERR(base);
> 
>> +	base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
>> +						 &hi6421_regmap_config);
>> +	if (IS_ERR(pmic->regmap)) {
>> +		dev_err(&pdev->dev, "regmap init failed: %ld\n",
>> +						PTR_ERR(pmic->regmap));
> 
> Can you break the line at the first ',/' and line up against '('?
> 
>> +		return PTR_ERR(pmic->regmap);
>> +	}
>> +
>> +	pmic->dev = dev;
>> +	platform_set_drvdata(pdev, pmic);
> 
> It's not _that_ important, but I like to see this at the end after you
> know everything else has succeeded.
>

When I move this after mfd_add_devices(), it fails to boot. In mfd
devices's probe, pmic->regmap is used.


>> +	/* set over-current protection debounce 8ms*/
> 
> Should be a ' ' between '8ms' and '*/'.
> 
>> +	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
>> +		(HI6421_OCP_DEB_SEL_MASK | HI6421_OCP_EN_DEBOUNCE_MASK |
>> +		 HI6421_OCP_AUTO_STOP_MASK),
>> +		(HI6421_OCP_DEB_SEL_8MS | HI6421_OCP_EN_DEBOUNCE_ENABLE));
>> +
>> +	ret = mfd_add_devices(dev, 0, hi6421_devs,
>> +			ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
> 
> I'd like to see an error message if mfd_add_devices() fails.
> 

Will do.

>> +	return ret;
>> +}
>> +
>> +static int hi6421_pmic_remove(struct platform_device *pdev)
>> +{
>> +	mfd_remove_devices(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver hi6421_pmic_driver = {
>> +	.driver = {
>> +		.name	= "hi6421_pmic",
>> +		.owner  = THIS_MODULE,
> 
> This is taken care of for you, you can remove it.
> 

Will do.

>> +		.of_match_table = of_hi6421_pmic_match_tbl,
>> +	},
>> +	.probe	= hi6421_pmic_probe,
>> +	.remove	= hi6421_pmic_remove,
>> +};
>> +module_platform_driver(hi6421_pmic_driver);
>> +
>> +MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
>> +MODULE_DESCRIPTION("Hi6421 PMIC driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
>> new file mode 100644
>> index 0000000..36bcb34
>> --- /dev/null
>> +++ b/include/linux/mfd/hi6421-pmic.h
>> @@ -0,0 +1,39 @@
>> +/*
>> + * Header file for device driver Hi6421 PMIC
>> + *
>> + * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
>> + *              http://www.hisilicon.com
>> + * Copyright (c) <2013-2014> Linaro Ltd.
>> + *              http://www.linaro.org
>> + *
>> + * Author: Guodong Xu <guodong.xu@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef	__HI6421_PMIC_H
>> +#define	__HI6421_PMIC_H
>> +
>> +/* Hi6421 registers are mapped to memory bus in 4 bytes stride */
>> +#define HI6421_REG_TO_BUS_ADDR(x)	(x << 2)
>> +
>> +/* Hi6421 OCP (over current protection) and DEB (debounce) control register */
>> +#define	HI6421_OCP_DEB_CTRL_REG		HI6421_REG_TO_BUS_ADDR(0x51)
>> +#define	HI6421_OCP_DEB_SEL_MASK		(0x0C)
>> +#define HI6421_OCP_DEB_SEL_8MS		(0x00)
>> +#define HI6421_OCP_DEB_SEL_16MS		(0x04)
>> +#define HI6421_OCP_DEB_SEL_32MS		(0x08)
>> +#define HI6421_OCP_DEB_SEL_64MS		(0x0C)
>> +#define HI6421_OCP_EN_DEBOUNCE_MASK	(0x02)
>> +#define HI6421_OCP_EN_DEBOUNCE_ENABLE	(0x02)
>> +#define HI6421_OCP_AUTO_STOP_MASK	(0x01)
>> +#define HI6421_OCP_AUTO_STOP_ENABLE	(0x01)
> 
> Remove the ()'s.
> 

Will do.

Thanks.
Guodong

>> +struct hi6421_pmic {
>> +	struct device		*dev;
>> +	struct regmap		*regmap;
>> +};
>> +
>> +#endif		/* __HI6421_PMIC_H */
> 

Re: [PATCH v6 4/6] mfd: Add hi6421 PMIC core driver

[Lee Jones]

On Mon, 25 Aug 2014, Guodong Xu wrote:
> On 08/20/2014 04:09 PM, Lee Jones wrote:
> > On Mon, 18 Aug 2014, Guodong Xu wrote:
> >> This adds driver to support HiSilicon Hi6421 PMIC. Hi6421 includes multi-
> >> functions, such as regulators, codec, ADCs, Coulomb counter, etc.
> >> This driver includes core APIs _only_.
> >>
> >> Drivers for individul components, like voltage regulators, are
> >> implemented in corresponding driver directories and files.
> >>
> >> Registers in Hi6421 are memory mapped, so using regmap-mmio API.
> >>
> >> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> >> ---
> >>  Documentation/devicetree/bindings/mfd/hi6421.txt |  37 +++++++

[...]

> >> + * Author: Guodong Xu <guodong.xu@linaro.org>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> > 
> > This should also contain a link to the full licence.
> > 
> > See: COPYING
> > 
> 
> Thanks. I checked COPYING, but there is no 'link' to full license. I

I was making reference to the fact that COPYING tells you to provide a
link to the full notice:

"To do so, attach the following notices to the program.  It is safest
to attach them to the start of each source file to most effectively
convey the exclusion of warranty; and each file should have at least
the "copyright" line and a pointer to where the full notice is found."

> copied a link from other c source: http://www.gnu.org/licenses/
> is that OK?

Yes, that's fine.

> >> +	platform_set_drvdata(pdev, pmic);
> > 
> > It's not _that_ important, but I like to see this at the end after you
> > know everything else has succeeded.
> 
> When I move this after mfd_add_devices(), it fails to boot. In mfd
> devices's probe, pmic->regmap is used.

You can move it to just before mfd_add_devices().

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog