[PATCH v4 0/8] MFD: add driver for HiSilicon Hi6421v530 PMIC

[PATCH v4 0/8] MFD: add driver for HiSilicon Hi6421v530 PMIC

[Guodong Xu]

This patchset adds driver for HiSilicon Hi6421v530 PMIC.

Mainline kernel already has driver support to a similar chip, Hi6421.
Hi6421 and Hi6421v530 are both from the same vendor, HiSilicon, but
they are at different revisions. They both use the same Memory-mapped
I/O method to communicate with Main SoC. However, they differ quite a
lot in their regulator designs. Eg. they have completely different LDO
voltage points.

Patch 1 and 2 are hi6421-pmic cleaning up.
Patch 3 and 4 extends hi6421-pmic-core.c to support Hi6421v530 revision.
Patch 5 add hi6421v530-regulator.c driver for LDO regulators.
Patch 6 fixes an issue for hi6421 regulator, which is not related to v530
         but it's found in this review.
Patch 7 is dts change, it depends on and can be applied to hi3660/hikey960
        patchset [1].
Patch 8 enables the relevant config items.

[1], http://www.spinics.net/lists/devicetree/msg178303.html

Major changes in v4:
 - put hi6421-pmic cleanup in separate patches.
 - solve review comments from Lee Johes.
 - regulator-name should not have '/' character. Otherwise it "Failed to
     create debugfs directory"

Major changes in v3:
 - in hi6421-pmic-core.c
    * use shorter license script.
    * arrange #include in alphabetical order.
    * using recommended error log messages from Lee Jones.
 - in hi6421v530-regulator.c
    * remove unused #include files
    * arrange remaining ones in alphabetical order.

Major changes in v2:
 - instead of writing a new driver, extend hi6421-pmic-core.c
     to support its v530 revision
 - update hi6421v530-regulator.c to use modern regulator driver
     design logics.

Guodong Xu (6):
  mfd: hi6421-pmic: cleanup: change license text to shorter form
  mfd: hi6421-pmic: cleanup: update dev_err messages
  dt-bindings: mfd: hi6421: Add hi6421v530 compatible string
  mfd: hi6421-pmic: add support for HiSilicon Hi6421v530
  regulator: hi6421: Describe consumed platform device
  arm64: defconfig: enable support hi6421v530 PMIC

Wang Xiaoyin (2):
  regulator: hi6421v530: add driver for hi6421v530 voltage regulator
  arm64: dts: hikey960: add device node for pmic and regulators

 Documentation/devicetree/bindings/mfd/hi6421.txt  |   4 +-
 arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts |  46 +++++
 arch/arm64/configs/defconfig                      |   2 +
 drivers/mfd/hi6421-pmic-core.c                    |  87 +++++----
 drivers/regulator/Kconfig                         |  10 ++
 drivers/regulator/Makefile                        |   1 +
 drivers/regulator/hi6421-regulator.c              |   7 +
 drivers/regulator/hi6421v530-regulator.c          | 207 ++++++++++++++++++++++
 include/linux/mfd/hi6421-pmic.h                   |   5 +
 9 files changed, 336 insertions(+), 33 deletions(-)
 create mode 100644 drivers/regulator/hi6421v530-regulator.c

-- 
2.10.2

[PATCH v4 1/8] mfd: hi6421-pmic: cleanup: change license text to shorter form

[Guodong Xu]

Change license text to a shorter form of GPLv2.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/mfd/hi6421-pmic-core.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
index 3fd703f..ad9e3d8 100644
--- a/drivers/mfd/hi6421-pmic-core.c
+++ b/drivers/mfd/hi6421-pmic-core.c
@@ -9,17 +9,8 @@
  * 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 as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
  */
 
 #include <linux/device.h>
-- 
2.10.2

[PATCH v4 2/8] mfd: hi6421-pmic: cleanup: update dev_err messages

[Guodong Xu]

Update dev_err messages to make them more readable.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/mfd/hi6421-pmic-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
index ad9e3d8..b1139d4 100644
--- a/drivers/mfd/hi6421-pmic-core.c
+++ b/drivers/mfd/hi6421-pmic-core.c
@@ -52,8 +52,8 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
 	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));
+		dev_err(&pdev->dev, "Failed to initialise Regmap: %ld\n",
+						PTR_ERR(pmic->regmap));
 		return PTR_ERR(pmic->regmap);
 	}
 
@@ -70,7 +70,7 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
 	ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421_devs,
 				   ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
 	if (ret) {
-		dev_err(&pdev->dev, "add mfd devices failed: %d\n", ret);
+		dev_err(&pdev->dev, "Failed to add child devices: %d\n", ret);
 		return ret;
 	}
 
-- 
2.10.2

[PATCH v4 3/8] dt-bindings: mfd: hi6421: Add hi6421v530 compatible string

[Guodong Xu]

Add compatible string for HiSilicon Hi6421v530 PMIC.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
Acked-for-mfd-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/mfd/hi6421.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/hi6421.txt b/Documentation/devicetree/bindings/mfd/hi6421.txt
index 0d5a446..22da96d 100644
--- a/Documentation/devicetree/bindings/mfd/hi6421.txt
+++ b/Documentation/devicetree/bindings/mfd/hi6421.txt
@@ -1,7 +1,9 @@
 * HI6421 Multi-Functional Device (MFD), by HiSilicon Ltd.
 
 Required parent device properties:
-- compatible	: contains "hisilicon,hi6421-pmic";
+- compatible    : One of the following chip-specific strings:
+	"hisilicon,hi6421-pmic";
+	"hisilicon,hi6421v530-pmic";
 - reg		: register range space of hi6421;
 
 Supported Hi6421 sub-devices include:
-- 
2.10.2

[PATCH v4 4/8] mfd: hi6421-pmic: add support for HiSilicon Hi6421v530

[Guodong Xu]

Add support for HiSilicon Hi6421v530 PMIC. Hi6421v530 communicates with
main SoC via memory-mapped I/O.

Hi6421v530 and Hi6421 are PMIC chips from the same vendor, HiSilicon,
but at different revisions. They share the same memory-mapped I/O
design. They differ in integrated devices, such as regulator details,
LDO voltage points.

Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/mfd/hi6421-pmic-core.c  | 68 ++++++++++++++++++++++++++++++-----------
 include/linux/mfd/hi6421-pmic.h |  5 +++
 2 files changed, 55 insertions(+), 18 deletions(-)

diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
index b1139d4..4568a43 100644
--- a/drivers/mfd/hi6421-pmic-core.c
+++ b/drivers/mfd/hi6421-pmic-core.c
@@ -1,9 +1,9 @@
 /*
- * Device driver for Hi6421 IC
+ * Device driver for Hi6421 PMIC
  *
  * Copyright (c) <2011-2014> HiSilicon Technologies Co., Ltd.
  *              http://www.hisilicon.com
- * Copyright (c) <2013-2014> Linaro Ltd.
+ * Copyright (c) <2013-2017> Linaro Ltd.
  *              http://www.linaro.org
  *
  * Author: Guodong Xu <guodong.xu@linaro.org>
@@ -16,16 +16,20 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/mfd/core.h>
+#include <linux/mfd/hi6421-pmic.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
-#include <linux/mfd/hi6421-pmic.h>
 
 static const struct mfd_cell hi6421_devs[] = {
 	{ .name = "hi6421-regulator", },
 };
 
+static const struct mfd_cell hi6421v530_devs[] = {
+	{ .name = "hi6421v530-regulator", },
+};
+
 static const struct regmap_config hi6421_regmap_config = {
 	.reg_bits = 32,
 	.reg_stride = 4,
@@ -33,12 +37,31 @@ static const struct regmap_config hi6421_regmap_config = {
 	.max_register = HI6421_REG_TO_BUS_ADDR(HI6421_REG_MAX),
 };
 
+static const struct of_device_id of_hi6421_pmic_match[] = {
+	{
+		.compatible = "hisilicon,hi6421-pmic",
+		.data = (void *)HI6421
+	},
+	{
+		.compatible = "hisilicon,hi6421v530-pmic",
+		.data = (void *)HI6421_V530
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_hi6421_pmic_match);
+
 static int hi6421_pmic_probe(struct platform_device *pdev)
 {
 	struct hi6421_pmic *pmic;
 	struct resource *res;
+	const struct of_device_id *id;
+	const struct mfd_cell *subdevs;
 	void __iomem *base;
-	int ret;
+	int n_subdevs, ret;
+
+	id = of_match_device(of_hi6421_pmic_match, &pdev->dev);
+	if (!id)
+		return -EINVAL;
 
 	pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
 	if (!pmic)
@@ -57,18 +80,33 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
 		return PTR_ERR(pmic->regmap);
 	}
 
-	/* set over-current protection debounce 8ms */
-	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
+	platform_set_drvdata(pdev, pmic);
+
+	switch ((unsigned long)id->data) {
+	case HI6421:
+		/* 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));
 
-	platform_set_drvdata(pdev, pmic);
+		subdevs = hi6421_devs;
+		n_subdevs = ARRAY_SIZE(hi6421_devs);
+		break;
+	case HI6421_V530:
+		subdevs = hi6421v530_devs;
+		n_subdevs = ARRAY_SIZE(hi6421v530_devs);
+		break;
+	default:
+		dev_err(&pdev->dev, "Unknown device type %ld\n",
+						(unsigned long)id->data);
+		return -EINVAL;
+	}
 
-	ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421_devs,
-				   ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
+	ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
+				   subdevs, n_subdevs, NULL, 0, NULL);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add child devices: %d\n", ret);
 		return ret;
@@ -77,16 +115,10 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id of_hi6421_pmic_match_tbl[] = {
-	{ .compatible = "hisilicon,hi6421-pmic", },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, of_hi6421_pmic_match_tbl);
-
 static struct platform_driver hi6421_pmic_driver = {
 	.driver = {
-		.name	= "hi6421_pmic",
-		.of_match_table = of_hi6421_pmic_match_tbl,
+		.name = "hi6421_pmic",
+		.of_match_table = of_hi6421_pmic_match,
 	},
 	.probe	= hi6421_pmic_probe,
 };
diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
index 587273e..2457438 100644
--- a/include/linux/mfd/hi6421-pmic.h
+++ b/include/linux/mfd/hi6421-pmic.h
@@ -38,4 +38,9 @@ struct hi6421_pmic {
 	struct regmap		*regmap;
 };
 
+enum hi6421_type {
+	HI6421      = 1,
+	HI6421_V530 = 2,
+};
+
 #endif		/* __HI6421_PMIC_H */
-- 
2.10.2

[PATCH v4 5/8] regulator: hi6421v530: add driver for hi6421v530 voltage regulator

[Guodong Xu]

From: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>

add the driver for hi6421v530 voltage regulator

Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/regulator/Kconfig                |  10 ++
 drivers/regulator/Makefile               |   1 +
 drivers/regulator/hi6421v530-regulator.c | 207 +++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 drivers/regulator/hi6421v530-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 48db87d..78cd8d8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -296,6 +296,16 @@ config REGULATOR_HI6421
 	  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_HI6421V530
+	tristate "HiSilicon Hi6421v530 PMIC voltage regulator support"
+	depends on MFD_HI6421_PMIC && OF
+	help
+	  This driver provides support for the voltage regulators on
+	  HiSilicon Hi6421v530 PMU / Codec IC.
+	  Hi6421v530 is a multi-function device which, on regulator part,
+	  provides 5 general purpose LDOs, and all of them come with support
+	  to either ECO (idle) or sleep mode.
+
 config REGULATOR_HI655X
 	tristate "Hisilicon HI655X PMIC regulators support"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index dc3503f..36e2b75 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -38,6 +38,7 @@ 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_HI6421V530) += hi6421v530-regulator.o
 obj-$(CONFIG_REGULATOR_HI655X) += hi655x-regulator.o
 obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
 obj-$(CONFIG_REGULATOR_ISL9305) += isl9305.o
diff --git a/drivers/regulator/hi6421v530-regulator.c b/drivers/regulator/hi6421v530-regulator.c
new file mode 100644
index 0000000..46bbba9
--- /dev/null
+++ b/drivers/regulator/hi6421v530-regulator.c
@@ -0,0 +1,207 @@
+/*
+ * Device driver for regulators in Hi6421V530 IC
+ *
+ * Copyright (c) <2017> HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ * Copyright (c) <2017> Linaro Ltd.
+ *              http://www.linaro.org
+ *
+ * Author: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
+ *         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/mfd/hi6421-pmic.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+/*
+ * struct hi6421v530_regulator_info - hi6421v530 regulator information
+ * @desc: regulator description
+ * @mode_mask: ECO mode bitmask of LDOs; for BUCKs, this masks sleep
+ * @eco_microamp: eco mode load upper limit (in uA), valid for LDOs only
+ */
+struct hi6421v530_regulator_info {
+	struct regulator_desc rdesc;
+	u8 mode_mask;
+	u32 eco_microamp;
+};
+
+/* HI6421v530 regulators */
+enum hi6421v530_regulator_id {
+	HI6421V530_LDO3,
+	HI6421V530_LDO9,
+	HI6421V530_LDO11,
+	HI6421V530_LDO15,
+	HI6421V530_LDO16,
+};
+
+static const unsigned int ldo_3_voltages[] = {
+	1800000, 1825000, 1850000, 1875000,
+	1900000, 1925000, 1950000, 1975000,
+	2000000, 2025000, 2050000, 2075000,
+	2100000, 2125000, 2150000, 2200000,
+};
+
+static const unsigned int ldo_9_11_voltages[] = {
+	1750000, 1800000, 1825000, 2800000,
+	2850000, 2950000, 3000000, 3300000,
+};
+
+static const unsigned int ldo_15_16_voltages[] = {
+	1750000, 1800000, 2400000, 2600000,
+	2700000, 2850000, 2950000, 3000000,
+};
+
+static const struct regulator_ops hi6421v530_ldo_ops;
+
+#define HI6421V530_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 uA
+ */
+#define HI6421V530_LDO(_ID, v_table, vreg, vmask, ereg, emask,		\
+		   odelay, ecomask, ecoamp) {				\
+	.rdesc = {							\
+		.name		 = #_ID,				\
+		.of_match        = of_match_ptr(#_ID),			\
+		.regulators_node = of_match_ptr("regulators"),		\
+		.ops		 = &hi6421v530_ldo_ops,			\
+		.type		 = REGULATOR_VOLTAGE,			\
+		.id		 = HI6421V530_##_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	 = HI6421V530_LDO_ENABLE_TIME,		\
+		.off_on_delay	 = odelay,				\
+	},								\
+	.mode_mask	= ecomask,					\
+	.eco_microamp	= ecoamp,					\
+}
+
+/* HI6421V530 regulator information */
+
+static struct hi6421v530_regulator_info hi6421v530_regulator_info[] = {
+	HI6421V530_LDO(LDO3, ldo_3_voltages, 0x061, 0xf, 0x060, 0x2,
+		   20000, 0x6, 8000),
+	HI6421V530_LDO(LDO9, ldo_9_11_voltages, 0x06b, 0x7, 0x06a, 0x2,
+		   40000, 0x6, 8000),
+	HI6421V530_LDO(LDO11, ldo_9_11_voltages, 0x06f, 0x7, 0x06e, 0x2,
+		   40000, 0x6, 8000),
+	HI6421V530_LDO(LDO15, ldo_15_16_voltages, 0x077, 0x7, 0x076, 0x2,
+		   40000, 0x6, 8000),
+	HI6421V530_LDO(LDO16, ldo_15_16_voltages, 0x079, 0x7, 0x078, 0x2,
+		   40000, 0x6, 8000),
+};
+
+static unsigned int hi6421v530_regulator_ldo_get_mode(
+					struct regulator_dev *rdev)
+{
+	struct hi6421v530_regulator_info *info;
+	unsigned int reg_val;
+
+	info = rdev_get_drvdata(rdev);
+	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 int hi6421v530_regulator_ldo_set_mode(struct regulator_dev *rdev,
+						unsigned int mode)
+{
+	struct hi6421v530_regulator_info *info;
+	unsigned int new_mode;
+
+	info = rdev_get_drvdata(rdev);
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		new_mode = 0;
+		break;
+	case REGULATOR_MODE_IDLE:
+		new_mode = info->mode_mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			   info->mode_mask, new_mode);
+
+	return 0;
+}
+
+
+static const struct regulator_ops hi6421v530_ldo_ops = {
+	.is_enabled = regulator_is_enabled_regmap,
+	.enable = regulator_enable_regmap,
+	.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 = hi6421v530_regulator_ldo_get_mode,
+	.set_mode = hi6421v530_regulator_ldo_set_mode,
+};
+
+static int hi6421v530_regulator_probe(struct platform_device *pdev)
+{
+	struct hi6421_pmic *pmic;
+	struct regulator_dev *rdev;
+	struct regulator_config config = { };
+	unsigned int i;
+
+	pmic = dev_get_drvdata(pdev->dev.parent);
+	if (!pmic) {
+		dev_err(&pdev->dev, "no pmic in the regulator parent node\n");
+		return -ENODEV;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hi6421v530_regulator_info); i++) {
+		config.dev = pdev->dev.parent;
+		config.regmap = pmic->regmap;
+		config.driver_data = &hi6421v530_regulator_info[i];
+
+		rdev = devm_regulator_register(&pdev->dev,
+				&hi6421v530_regulator_info[i].rdesc,
+				&config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "failed to register regulator %s\n",
+				hi6421v530_regulator_info[i].rdesc.name);
+			return PTR_ERR(rdev);
+		}
+	}
+	return 0;
+}
+
+static struct platform_driver hi6421v530_regulator_driver = {
+	.driver = {
+		.name	= "hi6421v530-regulator",
+	},
+	.probe	= hi6421v530_regulator_probe,
+};
+module_platform_driver(hi6421v530_regulator_driver);
+
+MODULE_AUTHOR("Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>");
+MODULE_DESCRIPTION("Hi6421v530 regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
2.10.2

[PATCH v4 6/8] regulator: hi6421: Describe consumed platform device

[Guodong Xu]

The hi6421-regulator driver consumes a similarly named platform device.
Adding that to the module device table, allows modprobe to locate this
driver once the device is created.

Cc: Jeremy Linton <lintonrjeremy@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/regulator/hi6421-regulator.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/regulator/hi6421-regulator.c b/drivers/regulator/hi6421-regulator.c
index 62c5f54..259c3a8 100644
--- a/drivers/regulator/hi6421-regulator.c
+++ b/drivers/regulator/hi6421-regulator.c
@@ -621,7 +621,14 @@ static int hi6421_regulator_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct platform_device_id hi6421_regulator_table[] = {
+	{ .name = "hi6421-regulator" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, hi6421_regulator_table);
+
 static struct platform_driver hi6421_regulator_driver = {
+	.id_table = hi6421_regulator_table,
 	.driver = {
 		.name	= "hi6421-regulator",
 	},
-- 
2.10.2

[PATCH v4 7/8] arm64: dts: hikey960: add device node for pmic and regulators

[Guodong Xu]

From: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>

add device node for hi6421 pmic core and hi6421v530
voltage regulator,include LDO(1,3,9,11,15,16)

Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 46 +++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
index ca448f0..e579333 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
@@ -97,6 +97,52 @@
 			default-state = "off";
 		};
 	};
+
+	pmic: pmic@fff34000 {
+		compatible = "hisilicon,hi6421v530-pmic";
+		reg = <0x0 0xfff34000 0x0 0x1000>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		regulators {
+			ldo3: LDO3 { /* HDMI */
+				regulator-name = "VOUT3_1V85";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <2200000>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo9: LDO9 { /* SDCARD I/O */
+				regulator-name = "VOUT9_1V8_2V95";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <240>;
+			};
+
+			ldo11: LDO11 { /* Low Speed Connector */
+				regulator-name = "VOUT11_1V8_2V95";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <240>;
+			};
+
+			ldo15: LDO15 { /* UFS VCC */
+				regulator-name = "VOUT15_3V0";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo16: LDO16 { /* SD VDD */
+				regulator-name = "VOUT16_2V95";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-enable-ramp-delay = <360>;
+			};
+		};
+	};
 };
 
 &i2c0 {
-- 
2.10.2

[PATCH v4 8/8] arm64: defconfig: enable support hi6421v530 PMIC

[Guodong Xu]

Enable configs for hi6421v530 mfd and regulator driver
 + CONFIG_MFD_HI6421_PMIC=y
 + CONFIG_REGULATOR_HI6421V530=y

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index ce07285..867d68c 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -305,6 +305,7 @@ CONFIG_S3C2410_WATCHDOG=y
 CONFIG_MESON_GXBB_WATCHDOG=m
 CONFIG_MESON_WATCHDOG=m
 CONFIG_MFD_EXYNOS_LPASS=m
+CONFIG_MFD_HI6421_PMIC=y
 CONFIG_MFD_MAX77620=y
 CONFIG_MFD_RK808=y
 CONFIG_MFD_SPMI_PMIC=y
@@ -315,6 +316,7 @@ CONFIG_MFD_CROS_EC=y
 CONFIG_MFD_CROS_EC_I2C=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
 CONFIG_REGULATOR_GPIO=y
+CONFIG_REGULATOR_HI6421V530=y
 CONFIG_REGULATOR_HI655X=y
 CONFIG_REGULATOR_MAX77620=y
 CONFIG_REGULATOR_PWM=y
-- 
2.10.2

Re: [PATCH v4 0/8] MFD: add driver for HiSilicon Hi6421v530 PMIC

[Arnd Bergmann]

On Tue, Jun 6, 2017 at 10:51 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> This patchset adds driver for HiSilicon Hi6421v530 PMIC.
>
> Mainline kernel already has driver support to a similar chip, Hi6421.
> Hi6421 and Hi6421v530 are both from the same vendor, HiSilicon, but
> they are at different revisions. They both use the same Memory-mapped
> I/O method to communicate with Main SoC. However, they differ quite a
> lot in their regulator designs. Eg. they have completely different LDO
> voltage points.
>
> Patch 1 and 2 are hi6421-pmic cleaning up.
> Patch 3 and 4 extends hi6421-pmic-core.c to support Hi6421v530 revision.
> Patch 5 add hi6421v530-regulator.c driver for LDO regulators.
> Patch 6 fixes an issue for hi6421 regulator, which is not related to v530
>          but it's found in this review.
> Patch 7 is dts change, it depends on and can be applied to hi3660/hikey960
>         patchset [1].
> Patch 8 enables the relevant config items.

Looks good to me,

    Arnd

Re: [PATCH v4 1/8] mfd: hi6421-pmic: cleanup: change license text to shorter form

[Lee Jones]

On Tue, 06 Jun 2017, Guodong Xu wrote:

> Change license text to a shorter form of GPLv2.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  drivers/mfd/hi6421-pmic-core.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
  
> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> index 3fd703f..ad9e3d8 100644
> --- a/drivers/mfd/hi6421-pmic-core.c
> +++ b/drivers/mfd/hi6421-pmic-core.c
> @@ -9,17 +9,8 @@
>   * 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 as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope it will be useful, but WITHOUT
> - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> - * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> - * more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
>   */
>  
>  #include <linux/device.h>

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

Re: [PATCH v4 2/8] mfd: hi6421-pmic: cleanup: update dev_err messages

[Lee Jones]

On Tue, 06 Jun 2017, Guodong Xu wrote:

> Update dev_err messages to make them more readable.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  drivers/mfd/hi6421-pmic-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
  
> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> index ad9e3d8..b1139d4 100644
> --- a/drivers/mfd/hi6421-pmic-core.c
> +++ b/drivers/mfd/hi6421-pmic-core.c
> @@ -52,8 +52,8 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
>  	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));
> +		dev_err(&pdev->dev, "Failed to initialise Regmap: %ld\n",
> +						PTR_ERR(pmic->regmap));
>  		return PTR_ERR(pmic->regmap);
>  	}
>  
> @@ -70,7 +70,7 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
>  	ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421_devs,
>  				   ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
>  	if (ret) {
> -		dev_err(&pdev->dev, "add mfd devices failed: %d\n", ret);
> +		dev_err(&pdev->dev, "Failed to add child devices: %d\n", ret);
>  		return ret;
>  	}
>  

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

Re: [PATCH v4 4/8] mfd: hi6421-pmic: add support for HiSilicon Hi6421v530

[Lee Jones]

On Tue, 06 Jun 2017, Guodong Xu wrote:

> Add support for HiSilicon Hi6421v530 PMIC. Hi6421v530 communicates with
> main SoC via memory-mapped I/O.
> 
> Hi6421v530 and Hi6421 are PMIC chips from the same vendor, HiSilicon,
> but at different revisions. They share the same memory-mapped I/O
> design. They differ in integrated devices, such as regulator details,
> LDO voltage points.
> 
> Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  drivers/mfd/hi6421-pmic-core.c  | 68 ++++++++++++++++++++++++++++++-----------
>  include/linux/mfd/hi6421-pmic.h |  5 +++
>  2 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> index b1139d4..4568a43 100644
> --- a/drivers/mfd/hi6421-pmic-core.c
> +++ b/drivers/mfd/hi6421-pmic-core.c

[...]

>  static int hi6421_pmic_probe(struct platform_device *pdev)
>  {
>  	struct hi6421_pmic *pmic;
>  	struct resource *res;
> +	const struct of_device_id *id;
> +	const struct mfd_cell *subdevs;
>  	void __iomem *base;
> -	int ret;
> +	int n_subdevs, ret;
> +
> +	id = of_match_device(of_hi6421_pmic_match, &pdev->dev);
> +	if (!id)
> +		return -EINVAL;
>  
>  	pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
>  	if (!pmic)
> @@ -57,18 +80,33 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
>  		return PTR_ERR(pmic->regmap);
>  	}
>  
> -	/* set over-current protection debounce 8ms */
> -	regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
> +	platform_set_drvdata(pdev, pmic);
> +
> +	switch ((unsigned long)id->data) {

Whilst this is not a blocker, it would be nicer to place this into a
'type' variable, in order to clarify what data you are using.

> +	case HI6421:
> +		/* 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));
>  
> -	platform_set_drvdata(pdev, pmic);
> +		subdevs = hi6421_devs;
> +		n_subdevs = ARRAY_SIZE(hi6421_devs);
> +		break;
> +	case HI6421_V530:
> +		subdevs = hi6421v530_devs;
> +		n_subdevs = ARRAY_SIZE(hi6421v530_devs);
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "Unknown device type %ld\n",
> +						(unsigned long)id->data);
> +		return -EINVAL;
> +	}
>  
> -	ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421_devs,
> -				   ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
> +	ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> +				   subdevs, n_subdevs, NULL, 0, NULL);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to add child devices: %d\n", ret);
>  		return ret;
> @@ -77,16 +115,10 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id of_hi6421_pmic_match_tbl[] = {
> -	{ .compatible = "hisilicon,hi6421-pmic", },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, of_hi6421_pmic_match_tbl);
> -
>  static struct platform_driver hi6421_pmic_driver = {
>  	.driver = {
> -		.name	= "hi6421_pmic",
> -		.of_match_table = of_hi6421_pmic_match_tbl,
> +		.name = "hi6421_pmic",
> +		.of_match_table = of_hi6421_pmic_match,
>  	},
>  	.probe	= hi6421_pmic_probe,
>  };
> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
> index 587273e..2457438 100644
> --- a/include/linux/mfd/hi6421-pmic.h
> +++ b/include/linux/mfd/hi6421-pmic.h
> @@ -38,4 +38,9 @@ struct hi6421_pmic {
>  	struct regmap		*regmap;
>  };
>  
> +enum hi6421_type {
> +	HI6421      = 1,
> +	HI6421_V530 = 2,
> +};
> +

As programmers we usually start counting at 0.  Then you can rely on
the C standard to populate the remaining values.

Something like:

enum hi6421_type {
	HI6421 = 0,
	HI6421_V530,
};

... is more normal.

>  #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 v4 0/8] MFD: add driver for HiSilicon Hi6421v530 PMIC

[Lee Jones]

On Tue, 06 Jun 2017, Arnd Bergmann wrote:

> On Tue, Jun 6, 2017 at 10:51 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> > This patchset adds driver for HiSilicon Hi6421v530 PMIC.
> >
> > Mainline kernel already has driver support to a similar chip, Hi6421.
> > Hi6421 and Hi6421v530 are both from the same vendor, HiSilicon, but
> > they are at different revisions. They both use the same Memory-mapped
> > I/O method to communicate with Main SoC. However, they differ quite a
> > lot in their regulator designs. Eg. they have completely different LDO
> > voltage points.
> >
> > Patch 1 and 2 are hi6421-pmic cleaning up.
> > Patch 3 and 4 extends hi6421-pmic-core.c to support Hi6421v530 revision.
> > Patch 5 add hi6421v530-regulator.c driver for LDO regulators.
> > Patch 6 fixes an issue for hi6421 regulator, which is not related to v530
> >          but it's found in this review.
> > Patch 7 is dts change, it depends on and can be applied to hi3660/hikey960
> >         patchset [1].
> > Patch 8 enables the relevant config items.
> 
> Looks good to me,

Is that an Ack?

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

Re: [PATCH v4 4/8] mfd: hi6421-pmic: add support for HiSilicon Hi6421v530

[Guodong Xu]

On Tue, Jun 6, 2017 at 9:54 PM, Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 06 Jun 2017, Guodong Xu wrote:
>
> > Add support for HiSilicon Hi6421v530 PMIC. Hi6421v530 communicates with
> > main SoC via memory-mapped I/O.
> >
> > Hi6421v530 and Hi6421 are PMIC chips from the same vendor, HiSilicon,
> > but at different revisions. They share the same memory-mapped I/O
> > design. They differ in integrated devices, such as regulator details,
> > LDO voltage points.
> >
> > Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
> > Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> > ---
> >  drivers/mfd/hi6421-pmic-core.c  | 68 ++++++++++++++++++++++++++++++-----------
> >  include/linux/mfd/hi6421-pmic.h |  5 +++
> >  2 files changed, 55 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
> > index b1139d4..4568a43 100644
> > --- a/drivers/mfd/hi6421-pmic-core.c
> > +++ b/drivers/mfd/hi6421-pmic-core.c
>
> [...]
>
> >  static int hi6421_pmic_probe(struct platform_device *pdev)
> >  {
> >       struct hi6421_pmic *pmic;
> >       struct resource *res;
> > +     const struct of_device_id *id;
> > +     const struct mfd_cell *subdevs;
> >       void __iomem *base;
> > -     int ret;
> > +     int n_subdevs, ret;
> > +
> > +     id = of_match_device(of_hi6421_pmic_match, &pdev->dev);
> > +     if (!id)
> > +             return -EINVAL;
> >
> >       pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
> >       if (!pmic)
> > @@ -57,18 +80,33 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
> >               return PTR_ERR(pmic->regmap);
> >       }
> >
> > -     /* set over-current protection debounce 8ms */
> > -     regmap_update_bits(pmic->regmap, HI6421_OCP_DEB_CTRL_REG,
> > +     platform_set_drvdata(pdev, pmic);
> > +
> > +     switch ((unsigned long)id->data) {
>
> Whilst this is not a blocker, it would be nicer to place this into a
> 'type' variable, in order to clarify what data you are using.
>

I will add that.
enum hi6421_type type;


>
> > +     case HI6421:
> > +             /* 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));
> >
> > -     platform_set_drvdata(pdev, pmic);
> > +             subdevs = hi6421_devs;
> > +             n_subdevs = ARRAY_SIZE(hi6421_devs);
> > +             break;
> > +     case HI6421_V530:
> > +             subdevs = hi6421v530_devs;
> > +             n_subdevs = ARRAY_SIZE(hi6421v530_devs);
> > +             break;
> > +     default:
> > +             dev_err(&pdev->dev, "Unknown device type %ld\n",
> > +                                             (unsigned long)id->data);
> > +             return -EINVAL;
> > +     }
> >
> > -     ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421_devs,
> > -                                ARRAY_SIZE(hi6421_devs), NULL, 0, NULL);
> > +     ret = devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> > +                                subdevs, n_subdevs, NULL, 0, NULL);
> >       if (ret) {
> >               dev_err(&pdev->dev, "Failed to add child devices: %d\n", ret);
> >               return ret;
> > @@ -77,16 +115,10 @@ static int hi6421_pmic_probe(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > -static const struct of_device_id of_hi6421_pmic_match_tbl[] = {
> > -     { .compatible = "hisilicon,hi6421-pmic", },
> > -     { },
> > -};
> > -MODULE_DEVICE_TABLE(of, of_hi6421_pmic_match_tbl);
> > -
> >  static struct platform_driver hi6421_pmic_driver = {
> >       .driver = {
> > -             .name   = "hi6421_pmic",
> > -             .of_match_table = of_hi6421_pmic_match_tbl,
> > +             .name = "hi6421_pmic",
> > +             .of_match_table = of_hi6421_pmic_match,
> >       },
> >       .probe  = hi6421_pmic_probe,
> >  };
> > diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
> > index 587273e..2457438 100644
> > --- a/include/linux/mfd/hi6421-pmic.h
> > +++ b/include/linux/mfd/hi6421-pmic.h
> > @@ -38,4 +38,9 @@ struct hi6421_pmic {
> >       struct regmap           *regmap;
> >  };
> >
> > +enum hi6421_type {
> > +     HI6421      = 1,
> > +     HI6421_V530 = 2,
> > +};
> > +
>
> As programmers we usually start counting at 0.  Then you can rely on
> the C standard to populate the remaining values.
>
> Something like:
>
> enum hi6421_type {
>         HI6421 = 0,
>         HI6421_V530,
> };
>
> ... is more normal.


:) sure. thanks. I will do.

-Guodong

>
>
> >  #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 v4 0/8] MFD: add driver for HiSilicon Hi6421v530 PMIC

[Arnd Bergmann]

On Tue, Jun 6, 2017 at 3:58 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 06 Jun 2017, Arnd Bergmann wrote:
>
>> On Tue, Jun 6, 2017 at 10:51 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
>> > This patchset adds driver for HiSilicon Hi6421v530 PMIC.
>> >
>> > Mainline kernel already has driver support to a similar chip, Hi6421.
>> > Hi6421 and Hi6421v530 are both from the same vendor, HiSilicon, but
>> > they are at different revisions. They both use the same Memory-mapped
>> > I/O method to communicate with Main SoC. However, they differ quite a
>> > lot in their regulator designs. Eg. they have completely different LDO
>> > voltage points.
>> >
>> > Patch 1 and 2 are hi6421-pmic cleaning up.
>> > Patch 3 and 4 extends hi6421-pmic-core.c to support Hi6421v530 revision.
>> > Patch 5 add hi6421v530-regulator.c driver for LDO regulators.
>> > Patch 6 fixes an issue for hi6421 regulator, which is not related to v530
>> >          but it's found in this review.
>> > Patch 7 is dts change, it depends on and can be applied to hi3660/hikey960
>> >         patchset [1].
>> > Patch 8 enables the relevant config items.
>>
>> Looks good to me,
>
> Is that an Ack?

Yes, sorry for not being explicit about this.

Acked-by: Arnd Bergmann <arnd@arndb.de>