[PATCH v5 0/5] Add Support for Hi6220 PMIC Hi6553 MFD Core

[PATCH v5 0/5] Add Support for Hi6220 PMIC Hi6553 MFD Core

[Chen Feng]

The patch sets add support for Hi6220 PMIC Hi655x MFD core and its
regulator driver.
  Current testing and support board is Hikey which is one of 96boards.
  It is an arm64 open source board. For more information about this board,
  please access https://www.96boards.org.

  This is hardware layout for access PMIC Hi655x from AP SoC Hi6220.
  Between PMIC Hi655x and Hi6220, the physical signal channel is SSI.
  We can use memory-mapped I/O to communicate.

  +----------------+             +-------------+
  |                |             |             |
  |    Hi6220      |   SSI bus   |   Hi655x    |
  |                |-------------|             |
  |                |(REGMAP_MMIO)|             |
  +----------------+             +-------------+

  V2: Code refactoring of regulator.

  V3: Drop mtcmos from this patch and use regmap-irq.

  V4: Move the vset-table to driver code and donot open code for it.

  V5: Use of_match to get dt-data

Chen Feng (5):
  doc: bindings: Add document for mfd hi665x PMIC
  doc: bindings: Document for hi655x regulator driver
  mfd: hi655x: Add hi665x pmic driver
  regulator: add regulator driver of hi655x pmic
  hisilicon/dts: Add hi655x pmic dts node

 .../devicetree/bindings/mfd/hisilicon,hi655x.txt   |  28 +++
 .../regulator/hisilicon,hi655x-regulator.txt       |  30 +++
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |   5 +
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          | 102 ++++++++
 drivers/mfd/Kconfig                                |  10 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/hi655x-pmic.c                          | 169 ++++++++++++++
 drivers/regulator/Kconfig                          |   8 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/hi655x-regulator.c               | 256 +++++++++++++++++++++
 include/linux/mfd/hi655x-pmic.h                    |  56 +++++
 11 files changed, 666 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
 create mode 100644 drivers/mfd/hi655x-pmic.c
 create mode 100644 drivers/regulator/hi655x-regulator.c
 create mode 100644 include/linux/mfd/hi655x-pmic.h

-- 
1.9.1

[PATCH v5 1/5] doc: bindings: Add document for mfd hi665x PMIC

[Chen Feng]

Add document for mfd driver hi655x pmic driver

Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
Signed-off-by: Fei Wang <w.f@huawei.com>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
---
 .../devicetree/bindings/mfd/hisilicon,hi655x.txt   | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt

diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
new file mode 100644
index 0000000..3180c40
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
@@ -0,0 +1,28 @@
+Hisilicon hi655x Power Management Integrated Circuit (PMIC)
+
+The hardware layout for access PMIC Hi655x from AP SoC Hi6220.
+Between PMIC Hi655x and Hi6220, the physical signal channel is SSI.
+We can use memory-mapped I/O to communicate.
+
++----------------+             +-------------+
+|                |             |             |
+|    Hi6220      |   SSI bus   |   Hi655x    |
+|                |-------------|             |
+|                |(REGMAP_MMIO)|             |
++----------------+             +-------------+
+
+Required properties:
+- compatible: Should be "hisilicon,hi655x-pmic"
+- reg: Base address of PMIC on hi6220 soc
+- interrupt-controller: Hi655x has internal IRQs (has own IRQ domain).
+- pmic-gpios: The gpio used by pmic irq.
+
+Example:
+	pmic: pmic@f8000000 {
+		compatible = "hisilicon,hi655x-pmic";
+		reg = <0x0 0xf8000000 0x0 0x1000>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		pmic-gpios = <&gpio1 2 0>;
+		status = "disabled";
+	}
-- 
1.9.1

[PATCH v5 2/5] doc: bindings: Document for hi655x regulator driver

[Chen Feng]

Add Document for hi655x pmic regulator driver

Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
Signed-off-by: Fei Wang <w.f@huawei.com>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
---
 .../regulator/hisilicon,hi655x-regulator.txt       | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt b/Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
new file mode 100644
index 0000000..6f94c42
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/hisilicon,hi655x-regulator.txt
@@ -0,0 +1,30 @@
+Hisilicon Hi655x Voltage regulators
+
+Note:
+The hi655x regulator control is managed by hi655x Power IC.
+So the node of this regulator must be child node of hi655x
+pmic node.
+
+The driver uses the regulator core framework, so please also
+take the bindings of regulator.txt for reference.
+
+The valid names for regulators are:
+
+ldo2 ldo7 ldo10 ldo13 ldo14 ldo15 ldo17 ldo19 ldo21 ldo22
+
+Example:
+        pmic: pmic@f8000000 {
+                compatible = "hisilicon,hi655x-pmic";
+		...
+		regulators {
+			ldo2: ldo2@a21 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo2";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <3200000>;
+				regulator-valid-modes-mask = <0x02>;
+				regulator-enable-ramp-delay = <120>;
+			};
+			...
+		}
+	}
-- 
1.9.1

[PATCH v5 3/5] mfd: hi655x: Add hi665x pmic driver

[Chen Feng]

Add pmic mfd driver to support hisilicon hi665x.

Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
Signed-off-by: Fei Wang <w.f@huawei.com>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
---
 drivers/mfd/Kconfig             |  10 +++
 drivers/mfd/Makefile            |   1 +
 drivers/mfd/hi655x-pmic.c       | 169 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/hi655x-pmic.h |  56 +++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/mfd/hi655x-pmic.c
 create mode 100644 include/linux/mfd/hi655x-pmic.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4d92df6..299d972 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -284,6 +284,16 @@ config MFD_HI6421_PMIC
 	  menus in order to enable them.
 	  We communicate with the Hi6421 via memory-mapped I/O.
 
+config MFD_HI655X_PMIC
+	tristate "HiSilicon Hi655X series PMU/Codec IC"
+	depends on ARCH_HISI || (COMPILE_TEST && ARM64)
+	depends on OF
+	select MFD_CORE
+	select REGMAP_MMIO
+	select REGMAP_IRQ
+	help
+	  Select this option to enable Hisilicon hi655x series pmic driver.
+
 config HTC_EGPIO
 	bool "HTC EGPIO support"
 	depends on GPIOLIB && ARM
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a8b76b8..6a7b0e1 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -186,6 +186,7 @@ obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
 obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
 obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
 obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
+obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
 obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
new file mode 100644
index 0000000..aab18f7
--- /dev/null
+++ b/drivers/mfd/hi655x-pmic.c
@@ -0,0 +1,169 @@
+/*
+ * Device driver for regulators in hi655x IC
+ *
+ * Copyright (c) 2016 Hisilicon.
+ *
+ * Chen Feng <puck.chen@hisilicon.com>
+ * Fei  Wang <w.f@huawei.com>
+ *
+ * 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/io.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/hi655x-pmic.h>
+#include <linux/regmap.h>
+
+static const struct mfd_cell hi655x_pmic_devs[] = {
+	{ .name = "hi655x-regulator", },
+};
+
+static const struct regmap_irq hi655x_irqs[] = {
+	{ .reg_offset = 0, .mask = OTMP_D1R_INT },
+	{ .reg_offset = 0, .mask = VSYS_2P5_R_INT },
+	{ .reg_offset = 0, .mask = VSYS_UV_D3R_INT },
+	{ .reg_offset = 0, .mask = VSYS_6P0_D200UR_INT },
+	{ .reg_offset = 0, .mask = PWRON_D4SR_INT },
+	{ .reg_offset = 0, .mask = PWRON_D20F_INT },
+	{ .reg_offset = 0, .mask = PWRON_D20R_INT },
+	{ .reg_offset = 0, .mask = RESERVE_INT },
+};
+
+static const struct regmap_irq_chip hi655x_irq_chip = {
+	.name = "hi655x-pmic",
+	.irqs = hi655x_irqs,
+	.num_regs = 1,
+	.num_irqs = ARRAY_SIZE(hi655x_irqs),
+	.status_base = HI655X_IRQ_STAT_BASE,
+	.mask_base = HI655X_IRQ_MASK_BASE,
+};
+
+static unsigned int hi655x_pmic_get_version(struct hi655x_pmic *pmic)
+{
+	u32 val;
+
+	regmap_read(pmic->regmap,
+		    HI655X_BUS_ADDR(HI655X_VER_REG), &val);
+
+	return val;
+}
+
+static struct regmap_config hi655x_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = HI655X_STRIDE,
+	.val_bits = 8,
+	.max_register = HI655X_BUS_ADDR(0xFFF),
+};
+
+static void hi655x_local_irq_clear(struct regmap *map)
+{
+	int i;
+
+	regmap_write(map, HI655X_ANA_IRQM_BASE, HI655X_IRQ_CLR);
+	for (i = 0; i < HI655X_IRQ_ARRAY; i++) {
+		regmap_write(map, HI655X_IRQ_STAT_BASE + i * HI655X_STRIDE,
+			     HI655X_IRQ_CLR);
+	}
+}
+
+static int hi655x_pmic_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct hi655x_pmic *pmic;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void __iomem *base;
+
+	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
+	pmic->dev = dev;
+
+	pmic->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!pmic->res) {
+		dev_err(dev, "platform_get_resource err\n");
+		return -ENOENT;
+	}
+	base = devm_ioremap_resource(dev, pmic->res);
+	if (!base) {
+		dev_err(dev, "cannot map register memory\n");
+		return -ENOMEM;
+	}
+	pmic->regmap = devm_regmap_init_mmio_clk(dev, NULL, base,
+						 &hi655x_regmap_config);
+
+	pmic->ver = hi655x_pmic_get_version(pmic);
+	if ((pmic->ver < PMU_VER_START) || (pmic->ver > PMU_VER_END)) {
+		dev_warn(dev, "it is wrong pmu version\n");
+		return -EINVAL;
+	}
+
+	hi655x_local_irq_clear(pmic->regmap);
+
+	pmic->gpio = of_get_named_gpio(np, "pmic-gpios", 0);
+	if (!gpio_is_valid(pmic->gpio)) {
+		dev_err(dev, "cannot get the pmic-gpios\n");
+		return -ENODEV;
+	}
+
+	ret = devm_gpio_request_one(dev, pmic->gpio, GPIOF_IN,
+				    "hi655x_pmic_irq");
+	if (ret < 0) {
+		dev_err(dev, "failed to request gpio %d  ret = %d\n",
+			pmic->gpio, ret);
+		return ret;
+	}
+
+	ret = regmap_add_irq_chip(pmic->regmap, gpio_to_irq(pmic->gpio),
+				  IRQF_TRIGGER_LOW | IRQF_NO_SUSPEND, 0,
+				  &hi655x_irq_chip, &pmic->irq_data);
+	if (ret) {
+		dev_err(dev, "add pmic irq chip error! ret %d\n", ret);
+		return ret;
+	}
+
+	/* bind pmic to device */
+	platform_set_drvdata(pdev, pmic);
+
+	ret = mfd_add_devices(dev, 0, hi655x_pmic_devs,
+			      ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
+	if (ret) {
+		dev_err(dev, "add mfd devices failed: %d\n", ret);
+		regmap_del_irq_chip(pmic->irq, pmic->irq_data);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int hi655x_pmic_remove(struct platform_device *pdev)
+{
+	mfd_remove_devices(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id of_hi655x_pmic_match_tbl[] = {
+	{ .compatible = "hisilicon,hi655x-pmic", },
+	{},
+};
+
+static struct platform_driver hi655x_pmic_driver = {
+	.driver	= {
+		.name =	"hi655x-pmic",
+		.of_match_table = of_hi655x_pmic_match_tbl,
+	},
+	.probe  = hi655x_pmic_probe,
+	.remove = hi655x_pmic_remove,
+};
+module_platform_driver(hi655x_pmic_driver);
+
+MODULE_AUTHOR("Chen Feng <puck.chen@hisilicon.com>");
+MODULE_DESCRIPTION("Hisilicon hi655x pmic driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/hi655x-pmic.h b/include/linux/mfd/hi655x-pmic.h
new file mode 100644
index 0000000..e1af0a1
--- /dev/null
+++ b/include/linux/mfd/hi655x-pmic.h
@@ -0,0 +1,56 @@
+/*
+ * Device driver for regulators in hi655x IC
+ *
+ * Copyright (c) 2016 Hisilicon.
+ *
+ * Chen Feng <puck.chen@hisilicon.com>
+ * Fei  Wang <w.f@huawei.com>
+ *
+ * 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 __HI655X_PMIC_H
+#define __HI655X_PMIC_H
+
+/* Hi655x registers are mapped to memory bus in 4 bytes stride */
+#define HI655X_STRIDE                   (4)
+#define HI655X_BUS_ADDR(x)              ((x) << 2)
+
+#define HI655X_BITS                     (8)
+
+#define HI655X_NR_IRQ                   (32)
+
+#define HI655X_IRQ_STAT_BASE            (0x003 << 2)
+#define HI655X_IRQ_MASK_BASE            (0x007 << 2)
+#define HI655X_ANA_IRQM_BASE            (0x1b5 << 2)
+#define HI655X_IRQ_ARRAY                (4)
+#define HI655X_IRQ_MASK                 (0xFF)
+#define HI655X_IRQ_CLR                  (0xFF)
+#define HI655X_VER_REG                  (0x00)
+
+#define PMU_VER_START                   (0x10)
+#define PMU_VER_END                     (0x38)
+
+#define RESERVE_INT                     (BIT(7))
+#define PWRON_D20R_INT                  (BIT(6))
+#define PWRON_D20F_INT                  (BIT(5))
+#define PWRON_D4SR_INT                  (BIT(4))
+#define VSYS_6P0_D200UR_INT             (BIT(3))
+#define VSYS_UV_D3R_INT                 (BIT(2))
+#define VSYS_2P5_R_INT                  (BIT(1))
+#define OTMP_D1R_INT                    (BIT(0))
+
+struct hi655x_pmic {
+	struct resource *res;
+	struct device *dev;
+	struct regmap *regmap;
+	struct clk *clk;
+	int irq;
+	int gpio;
+	unsigned int ver;
+	struct regmap_irq_chip_data *irq_data;
+};
+
+#endif
-- 
1.9.1

[PATCH v5 4/5] regulator: add regulator driver of hi655x pmic

[Chen Feng]

Add regulator support for hi655x pmic

Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
Signed-off-by: Fei Wang <w.f@huawei.com>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
---
 drivers/regulator/Kconfig            |   8 ++
 drivers/regulator/Makefile           |   1 +
 drivers/regulator/hi655x-regulator.c | 256 +++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/regulator/hi655x-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8df0b0e..c00c915 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -261,6 +261,14 @@ 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_HI655X
+	tristate "Hisilicon HI655X PMIC regulators support"
+	depends on ARCH_HISI || (COMPILE_TEST && ARM64)
+	depends on MFD_HI655X_PMIC && OF
+	help
+	  This driver provides support for the voltage regulators of the
+	  Hisilicon Hi655x PMIC device.
+
 config REGULATOR_ISL9305
 	tristate "Intersil ISL9305 regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0f81749..8e4db96 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -34,6 +34,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_HI655X) += hi655x-regulator.o
 obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
 obj-$(CONFIG_REGULATOR_ISL9305) += isl9305.o
 obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
diff --git a/drivers/regulator/hi655x-regulator.c b/drivers/regulator/hi655x-regulator.c
new file mode 100644
index 0000000..e8e9995
--- /dev/null
+++ b/drivers/regulator/hi655x-regulator.c
@@ -0,0 +1,256 @@
+/*
+ * Device driver for regulators in hi655x IC
+ *
+ * Copyright (c) 2016 Hisilicon.
+ *
+ * Chen Feng <puck.chen@hisilicon.com>
+ * Fei  Wang <w.f@huawei.com>
+ *
+ * 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/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/mfd/hi655x-pmic.h>
+
+struct hi655x_regulator {
+	unsigned int disable_reg;
+	unsigned int status_reg;
+	unsigned int ctrl_regs;
+	unsigned int ctrl_mask;
+	struct regulator_desc rdesc;
+};
+
+/* LDO7 & LDO10 */
+static const unsigned int ldo7_voltages[] = {
+	1800000, 1850000, 2850000, 2900000,
+	3000000, 3100000, 3200000, 3300000,
+};
+
+static const unsigned int ldo19_voltages[] = {
+	1800000, 1850000, 1900000, 1750000,
+	2800000, 2850000, 2900000, 3000000,
+};
+
+static const unsigned int ldo22_voltages[] = {
+	 900000, 1000000, 1050000, 1100000,
+	1150000, 1175000, 1185000, 1200000,
+};
+
+enum hi655x_regulator_id {
+	HI655X_LDO0,
+	HI655X_LDO1,
+	HI655X_LDO2,
+	HI655X_LDO3,
+	HI655X_LDO4,
+	HI655X_LDO5,
+	HI655X_LDO6,
+	HI655X_LDO7,
+	HI655X_LDO8,
+	HI655X_LDO9,
+	HI655X_LDO10,
+	HI655X_LDO11,
+	HI655X_LDO12,
+	HI655X_LDO13,
+	HI655X_LDO14,
+	HI655X_LDO15,
+	HI655X_LDO16,
+	HI655X_LDO17,
+	HI655X_LDO18,
+	HI655X_LDO19,
+	HI655X_LDO20,
+	HI655X_LDO21,
+	HI655X_LDO22,
+};
+
+static int hi655x_is_enabled(struct regulator_dev *rdev)
+{
+	unsigned int value = 0;
+
+	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+
+	regmap_read(rdev->regmap, regulator->status_reg, &value);
+	return (value & BIT(regulator->ctrl_mask));
+}
+
+static int hi655x_disable(struct regulator_dev *rdev)
+{
+	int ret = 0;
+
+	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+
+	ret = regmap_write(rdev->regmap, regulator->disable_reg,
+			   BIT(regulator->ctrl_mask));
+	return ret;
+}
+
+static struct regulator_ops hi655x_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = hi655x_disable,
+	.is_enabled = hi655x_is_enabled,
+	.list_voltage = regulator_list_voltage_table,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+};
+
+static struct regulator_ops hi655x_ldo_linear_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = hi655x_disable,
+	.is_enabled = hi655x_is_enabled,
+	.list_voltage = regulator_list_voltage_linear,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+};
+
+#define HI655X_LDO(_ID, vreg, vmask, ereg, dreg,                \
+		   sreg, cmask, vtable) {                       \
+	.rdesc = {                                              \
+		.name           = #_ID,                         \
+		.ops            = &hi655x_regulator_ops,        \
+		.type           = REGULATOR_VOLTAGE,            \
+		.id             = HI655X_##_ID,                 \
+		.owner          = THIS_MODULE,                  \
+		.n_voltages     = ARRAY_SIZE(vtable),           \
+		.volt_table     = vtable,                       \
+		.vsel_reg       = HI655X_BUS_ADDR(vreg),        \
+		.vsel_mask      = vmask,                        \
+		.enable_reg     = HI655X_BUS_ADDR(ereg),        \
+		.enable_mask    = cmask,                        \
+	},                                                      \
+	.disable_reg = HI655X_BUS_ADDR(dreg),                   \
+	.status_reg = HI655X_BUS_ADDR(sreg),                    \
+	.ctrl_mask = cmask,                                     \
+}
+
+#define HI655X_LDO_LINEAR(_ID, vreg, vmask, ereg, dreg,         \
+			  sreg, cmask, minv, nvolt, vstep) {    \
+	.rdesc = {                                              \
+		.name           = #_ID,                         \
+		.ops            = &hi655x_ldo_linear_ops,       \
+		.type           = REGULATOR_VOLTAGE,            \
+		.id             = HI655X_##_ID,                 \
+		.owner          = THIS_MODULE,                  \
+		.min_uV         = minv,                         \
+		.n_voltages     = nvolt,                        \
+		.uV_step        = vstep,                        \
+		.uV_step        = vstep,                        \
+		.vsel_reg       = HI655X_BUS_ADDR(vreg),        \
+		.vsel_mask      = vmask,                        \
+		.enable_reg     = HI655X_BUS_ADDR(ereg),        \
+		.enable_mask    = cmask,                        \
+	},                                                      \
+	.disable_reg = HI655X_BUS_ADDR(dreg),                   \
+	.status_reg = HI655X_BUS_ADDR(sreg),                    \
+	.ctrl_mask = cmask,                                     \
+}
+
+static struct hi655x_regulator regulators[] = {
+	HI655X_LDO_LINEAR(LDO2, 0x72, 0x07, 0x29, 0x2a, 0x2b, 0x01,
+			  2500000, 8, 100000),
+	HI655X_LDO(LDO7, 0x78, 0x07, 0x29, 0x2a, 0x2b, 0x06, ldo7_voltages),
+	HI655X_LDO(LDO10, 0x78, 0x07, 0x29, 0x2a, 0x2b, 0x01, ldo7_voltages),
+	HI655X_LDO_LINEAR(LDO13, 0x7e, 0x07, 0x2c, 0x2d, 0x2e, 0x04,
+			  1600000, 8, 50000),
+	HI655X_LDO_LINEAR(LDO14, 0x7f, 0x07, 0x2c, 0x2d, 0x2e, 0x05,
+			  2500000, 8, 100000),
+	HI655X_LDO_LINEAR(LDO15, 0x80, 0x07, 0x2c, 0x2d, 0x2e, 0x06,
+			  1600000, 8, 50000),
+	HI655X_LDO_LINEAR(LDO17, 0x82, 0x07, 0x2f, 0x30, 0x31, 0x00,
+			  2500000, 8, 100000),
+	HI655X_LDO(LDO19, 0x84, 0x07, 0x2f, 0x30, 0x31, 0x02, ldo19_voltages),
+	HI655X_LDO_LINEAR(LDO21, 0x86, 0x07, 0x2f, 0x30, 0x31, 0x04,
+			  1650000, 8, 50000),
+	HI655X_LDO(LDO22, 0x87, 0x07, 0x2f, 0x30, 0x31, 0x05, ldo22_voltages),
+};
+
+static struct of_regulator_match hi655x_regulator_match[] = {
+	{ .name	= "ldo2", .driver_data = (void *)HI655X_LDO0, },
+	{ .name	= "ldo7", .driver_data = (void *)HI655X_LDO7, },
+	{ .name	= "ldo10", .driver_data = (void *)HI655X_LDO10, },
+	{ .name	= "ldo13", .driver_data = (void *)HI655X_LDO13, },
+	{ .name	= "ldo14", .driver_data = (void *)HI655X_LDO14, },
+	{ .name = "ldo15", .driver_data = (void *)HI655X_LDO15, },
+	{ .name	= "ldo17", .driver_data = (void *)HI655X_LDO17, },
+	{ .name	= "ldo19", .driver_data = (void *)HI655X_LDO19, },
+	{ .name	= "ldo21", .driver_data = (void *)HI655X_LDO21, },
+	{ .name	= "ldo22", .driver_data = (void *)HI655X_LDO22, },
+};
+
+static int hi655x_regulator_probe(struct platform_device *pdev)
+{
+	unsigned int i;
+	int ret;
+	struct hi655x_regulator *regulator;
+	struct hi655x_pmic *pmic;
+	struct regulator_config config = { };
+	struct device *dev = &pdev->dev;
+	struct regulator_dev *rdev;
+	struct device_node *np;
+
+	np = of_get_child_by_name(dev->parent->of_node, "regulators");
+	if (!np)
+		return -ENODEV;
+
+	ret = of_regulator_match(dev, np,
+				 hi655x_regulator_match,
+				 ARRAY_SIZE(hi655x_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(pdev->dev.parent);
+	if (!pmic) {
+		dev_err(dev, "no pmic in the regulator parent node\n");
+		return -ENODEV;
+	}
+
+	regulator = devm_kzalloc(dev, sizeof(*regulator), GFP_KERNEL);
+	if (!regulator)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, regulator);
+
+	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
+		config.dev = &pdev->dev;
+		config.driver_data = regulator;
+		config.regmap = pmic->regmap;
+		config.of_node = hi655x_regulator_match[i].of_node;
+		config.init_data = hi655x_regulator_match[i].init_data;
+
+		/* register regulator with framework */
+		rdev = devm_regulator_register(&pdev->dev,
+					       &regulators[i].rdesc,
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "failed to register regulator %s\n",
+				regulator->rdesc.name);
+			return PTR_ERR(rdev);
+		}
+	}
+	return 0;
+}
+
+static struct platform_driver hi655x_regulator_driver = {
+	.driver = {
+		.name	= "hi655x-regulator",
+	},
+	.probe	= hi655x_regulator_probe,
+};
+module_platform_driver(hi655x_regulator_driver);
+
+MODULE_AUTHOR("Chen Feng <puck.chen@hisilicon.com>");
+MODULE_DESCRIPTION("Hisilicon hi655x regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v5 5/5] hisilicon/dts: Add hi655x pmic dts node

[Chen Feng]

Add the mfd hi655x dts node and regulator support

Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
Signed-off-by: Fei Wang <w.f@huawei.com>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts |   5 ++
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      | 102 +++++++++++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index 8d43a0f..f714ac7 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -31,4 +31,9 @@
 		device_type = "memory";
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+};
+
+&pmic {
+	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 82d2488..4614304 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -208,5 +208,107 @@
 			clock-names = "uartclk", "apb_pclk";
 			status = "disabled";
 		};
+
+	};
+
+	pmic: pmic@f8000000 {
+		compatible = "hisilicon,hi655x-pmic";
+		reg = <0x0 0xf8000000 0x0 0x1000>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		pmic-gpios = <&gpio1 2 0>;
+		status = "disabled";
+
+		regulators {
+			ldo2: ldo2@a21 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo2";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <3200000>;
+				regulator-valid-modes-mask = <0x02>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo7: ldo7@a26 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo7";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-valid-modes-mask = <0x0a>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo10: ldo10@a29 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo10";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-valid-modes-mask = <0x0a>;
+				regulator-enable-ramp-delay = <360>;
+			};
+
+			ldo13: ldo13@a32 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo13";
+				regulator-min-microvolt = <1600000>;
+				regulator-max-microvolt = <1950000>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo14: ldo14@a33 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo14";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <3200000>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo15: ldo15@a34 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo15";
+				regulator-min-microvolt = <1600000>;
+				regulator-max-microvolt = <1950000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo17: ldo17@a36 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo17";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <3200000>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo19: ldo19@a38 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo19";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-enable-ramp-delay = <360>;
+			};
+
+			ldo21: ldo21@a40 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo21";
+				regulator-min-microvolt = <1650000>;
+				regulator-max-microvolt = <2000000>;
+				regulator-always-on;
+				regulator-valid-modes-mask = <0x02>;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo22: ldo22@a41 {
+				compatible = "hisilicon,hi655x-regulator";
+				regulator-name = "ldo22";
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1200000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-valid-modes-mask = <0x02>;
+				regulator-enable-ramp-delay = <120>;
+			};
+		};
 	};
 };
-- 
1.9.1

Re: [PATCH v5 2/5] doc: bindings: Document for hi655x regulator driver

[Mark Brown]

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

On Mon, Jan 11, 2016 at 08:20:14PM +0800, Chen Feng wrote:
> Add Document for hi655x pmic regulator driver

As I said in reply to v4:

| Please also use subject lines reflecting the normal style for the
| subsystem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 4/5] regulator: add regulator driver of hi655x pmic

[Mark Brown]

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

On Mon, Jan 11, 2016 at 08:20:16PM +0800, Chen Feng wrote:

> +config REGULATOR_HI655X
> +	tristate "Hisilicon HI655X PMIC regulators support"
> +	depends on ARCH_HISI || (COMPILE_TEST && ARM64)

Why does this depend on ARM64?  If it's needed it probably indicates a
problem...

> +	np = of_get_child_by_name(dev->parent->of_node, "regulators");
> +	if (!np)
> +		return -ENODEV;
> +
> +	ret = of_regulator_match(dev, np,
> +				 hi655x_regulator_match,
> +				 ARRAY_SIZE(hi655x_regulator_match));

Like I said on the previous version:

| Don't open code this, use the standard support with of_match and
| regulators_node.

The code is now using of_match but still open coding regulators_node.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 2/5] doc: bindings: Document for hi655x regulator driver

[chenfeng]

Mark,

On 2016/1/12 2:17, Mark Brown wrote:
> On Mon, Jan 11, 2016 at 08:20:14PM +0800, Chen Feng wrote:
>> Add Document for hi655x pmic regulator driver
> 
> As I said in reply to v4:
> 
> | Please also use subject lines reflecting the normal style for the
> | subsystem.
> 
Sorry, I don't understand your Prior advice.

I will change it like this.
regulator: hisilicon: Add Document for hi655x PMIC regulator

Re: [PATCH v5 4/5] regulator: add regulator driver of hi655x pmic

[chenfeng]

On 2016/1/12 2:24, Mark Brown wrote:
> On Mon, Jan 11, 2016 at 08:20:16PM +0800, Chen Feng wrote:
> 
>> +config REGULATOR_HI655X
>> +	tristate "Hisilicon HI655X PMIC regulators support"
>> +	depends on ARCH_HISI || (COMPILE_TEST && ARM64)
> 
> Why does this depend on ARM64?  If it's needed it probably indicates a
> problem...
> 
There will be compile warning with arch parisc.

Add the current support platform is ARM64.
>> +	np = of_get_child_by_name(dev->parent->of_node, "regulators");
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	ret = of_regulator_match(dev, np,
>> +				 hi655x_regulator_match,
>> +				 ARRAY_SIZE(hi655x_regulator_match));
> 
> Like I said on the previous version:
> 
> | Don't open code this, use the standard support with of_match and
> | regulators_node.
> 
> The code is now using of_match but still open coding regulators_node.
> 
I am not sure about open coding regulators_node.

I take max8907-regulator.c for reference. The code there is:
224 static int max8907_regulator_parse_dt(struct platform_device *pdev)
225 {
226         struct device_node *np, *regulators;
227         int ret;
228
229         np = pdev->dev.parent->of_node;
230         if (!np)
231                 return 0;
232
233         regulators = of_get_child_by_name(np, "regulators");
	    ...
238
239         ret = of_regulator_match(&pdev->dev, regulators, max8907_matches,
240                                  ARRAY_SIZE(max8907_matches));

249 }

Can you give me some references? Really thanks for your help.

ChenFeng

Re: [PATCH v5 4/5] regulator: add regulator driver of hi655x pmic

[Mark Brown]

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

On Tue, Jan 12, 2016 at 10:18:03AM +0800, chenfeng wrote:
> On 2016/1/12 2:24, Mark Brown wrote:
> > On Mon, Jan 11, 2016 at 08:20:16PM +0800, Chen Feng wrote:

> >> +config REGULATOR_HI655X
> >> +	tristate "Hisilicon HI655X PMIC regulators support"
> >> +	depends on ARCH_HISI || (COMPILE_TEST && ARM64)

> > Why does this depend on ARM64?  If it's needed it probably indicates a
> > problem...

> There will be compile warning with arch parisc.

> Add the current support platform is ARM64.

The whole point of COMPILE_TEST is to allow people who are working
generally rather than with the particular hardware to build things to
improve build test coverage when doing general kernel work.  A warning
on a very obscure architecture is really not a blocker here.

> I am not sure about open coding regulators_node.

> I take max8907-regulator.c for reference. The code there is:
> 224 static int max8907_regulator_parse_dt(struct platform_device *pdev)

The existance of older drivers that have not yet been converted to use
newer core subsystem features is not a good reason to avoid using those
core subsystem features.  You'll commonly find this situation in the
kernel.

> Can you give me some references? Really thanks for your help.

$ grep -l regulators_node drivers/regulator/*.c
drivers/regulator/88pm800.c
drivers/regulator/axp20x-regulator.c
drivers/regulator/da9062-regulator.c
drivers/regulator/isl9305.c
drivers/regulator/max14577.c
drivers/regulator/max77686.c
drivers/regulator/max77693.c
drivers/regulator/max77802.c
drivers/regulator/mt6311-regulator.c
drivers/regulator/of_regulator.c
drivers/regulator/pv88060-regulator.c
drivers/regulator/pv88090-regulator.c
drivers/regulator/rn5t618-regulator.c
drivers/regulator/rt5033-regulator.c
drivers/regulator/sky81452-regulator.c
drivers/regulator/tps65023-regulator.c
drivers/regulator/tps65086-regulator.c
drivers/regulator/tps65217-regulator.c

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v5 4/5] regulator: add regulator driver of hi655x pmic

[chenfeng]

On 2016/1/16 2:07, Mark Brown wrote:
> On Tue, Jan 12, 2016 at 10:18:03AM +0800, chenfeng wrote:
>> On 2016/1/12 2:24, Mark Brown wrote:
>>> On Mon, Jan 11, 2016 at 08:20:16PM +0800, Chen Feng wrote:
> 
>>>> +config REGULATOR_HI655X
>>>> +	tristate "Hisilicon HI655X PMIC regulators support"
>>>> +	depends on ARCH_HISI || (COMPILE_TEST && ARM64)
> 
>>> Why does this depend on ARM64?  If it's needed it probably indicates a
>>> problem...
> 
>> There will be compile warning with arch parisc.
> 
>> Add the current support platform is ARM64.
> 
> The whole point of COMPILE_TEST is to allow people who are working
> generally rather than with the particular hardware to build things to
> improve build test coverage when doing general kernel work.  A warning
> on a very obscure architecture is really not a blocker here.
> 
ok, I will remove the ARM64 depends.
>> I am not sure about open coding regulators_node.
> 
>> I take max8907-regulator.c for reference. The code there is:
>> 224 static int max8907_regulator_parse_dt(struct platform_device *pdev)
> 
> The existance of older drivers that have not yet been converted to use
> newer core subsystem features is not a good reason to avoid using those
> core subsystem features.  You'll commonly find this situation in the
> kernel.
> 
>> Can you give me some references? Really thanks for your help.
> 
> $ grep -l regulators_node drivers/regulator/*.c
> drivers/regulator/88pm800.c
> drivers/regulator/axp20x-regulator.c
> drivers/regulator/da9062-regulator.c
> drivers/regulator/isl9305.c
> drivers/regulator/max14577.c
> drivers/regulator/max77686.c
> drivers/regulator/max77693.c
> drivers/regulator/max77802.c
> drivers/regulator/mt6311-regulator.c
> drivers/regulator/of_regulator.c
> drivers/regulator/pv88060-regulator.c
> drivers/regulator/pv88090-regulator.c
> drivers/regulator/rn5t618-regulator.c
> drivers/regulator/rt5033-regulator.c
> drivers/regulator/sky81452-regulator.c
> drivers/regulator/tps65023-regulator.c
> drivers/regulator/tps65086-regulator.c
> drivers/regulator/tps65217-regulator.c
> 
Understand, I will remove the match-table and add the regulators_node
to match the regulator-compatible in dts.

Re: [PATCH v5 1/5] doc: bindings: Add document for mfd hi665x PMIC

[Lee Jones]

On Mon, 11 Jan 2016, Chen Feng wrote:

> Add document for mfd driver hi655x pmic driver
> 
> Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
> Signed-off-by: Fei Wang <w.f@huawei.com>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> ---
>  .../devicetree/bindings/mfd/hisilicon,hi655x.txt   | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> new file mode 100644
> index 0000000..3180c40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
> @@ -0,0 +1,28 @@
> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
> +
> +The hardware layout for access PMIC Hi655x from AP SoC Hi6220.
> +Between PMIC Hi655x and Hi6220, the physical signal channel is SSI.
> +We can use memory-mapped I/O to communicate.
> +
> ++----------------+             +-------------+
> +|                |             |             |
> +|    Hi6220      |   SSI bus   |   Hi655x    |
> +|                |-------------|             |
> +|                |(REGMAP_MMIO)|             |
> ++----------------+             +-------------+
> +
> +Required properties:
> +- compatible: Should be "hisilicon,hi655x-pmic"
> +- reg: Base address of PMIC on hi6220 soc

SoC

> +- interrupt-controller: Hi655x has internal IRQs (has own IRQ domain).
> +- pmic-gpios: The gpio used by pmic irq.

PMIC IRQ

> +
> +Example:
> +	pmic: pmic@f8000000 {
> +		compatible = "hisilicon,hi655x-pmic";
> +		reg = <0x0 0xf8000000 0x0 0x1000>;
> +		#interrupt-cells = <2>;
> +		interrupt-controller;
> +		pmic-gpios = <&gpio1 2 0>;

What's the last cell for here?  If they are flags, there is probably a
#define you can use in dt-include.

> +		status = "disabled";

What's the point in disabling example code?

> +	}

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

Re: [PATCH v5 3/5] mfd: hi655x: Add hi665x pmic driver

[Lee Jones]

On Mon, 11 Jan 2016, Chen Feng wrote:

> Add pmic mfd driver to support hisilicon hi665x.

PMIC MFD

> Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
> Signed-off-by: Fei Wang <w.f@huawei.com>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> ---
>  drivers/mfd/Kconfig             |  10 +++
>  drivers/mfd/Makefile            |   1 +
>  drivers/mfd/hi655x-pmic.c       | 169 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/hi655x-pmic.h |  56 +++++++++++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/mfd/hi655x-pmic.c
>  create mode 100644 include/linux/mfd/hi655x-pmic.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4d92df6..299d972 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -284,6 +284,16 @@ config MFD_HI6421_PMIC
>  	  menus in order to enable them.
>  	  We communicate with the Hi6421 via memory-mapped I/O.
>  
> +config MFD_HI655X_PMIC
> +	tristate "HiSilicon Hi655X series PMU/Codec IC"
> +	depends on ARCH_HISI || (COMPILE_TEST && ARM64)

Why not just COMPILE_TEST?

> +	depends on OF

So this will not COMPILE_TEST if OF is not enabled.

> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	select REGMAP_IRQ
> +	help
> +	  Select this option to enable Hisilicon hi655x series pmic driver.
> +
>  config HTC_EGPIO
>  	bool "HTC EGPIO support"
>  	depends on GPIOLIB && ARM
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a8b76b8..6a7b0e1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -186,6 +186,7 @@ obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
>  obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
>  obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
> +obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
> diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
> new file mode 100644
> index 0000000..aab18f7
> --- /dev/null
> +++ b/drivers/mfd/hi655x-pmic.c
> @@ -0,0 +1,169 @@
> +/*
> + * Device driver for regulators in hi655x IC

We know it's a device driver.  And I hope it's not a regulator driver.

> + * Copyright (c) 2016 Hisilicon.
> + *
> + * Chen Feng <puck.chen@hisilicon.com>
> + * Fei  Wang <w.f@huawei.com>

Author(s): 

> + * 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/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/init.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/hi655x-pmic.h>
> +#include <linux/regmap.h>

Alphabetical please.

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

What other devices are there?

> +static const struct regmap_irq hi655x_irqs[] = {
> +	{ .reg_offset = 0, .mask = OTMP_D1R_INT },
> +	{ .reg_offset = 0, .mask = VSYS_2P5_R_INT },
> +	{ .reg_offset = 0, .mask = VSYS_UV_D3R_INT },
> +	{ .reg_offset = 0, .mask = VSYS_6P0_D200UR_INT },
> +	{ .reg_offset = 0, .mask = PWRON_D4SR_INT },
> +	{ .reg_offset = 0, .mask = PWRON_D20F_INT },
> +	{ .reg_offset = 0, .mask = PWRON_D20R_INT },
> +	{ .reg_offset = 0, .mask = RESERVE_INT },
> +};
> +
> +static const struct regmap_irq_chip hi655x_irq_chip = {
> +	.name = "hi655x-pmic",
> +	.irqs = hi655x_irqs,
> +	.num_regs = 1,
> +	.num_irqs = ARRAY_SIZE(hi655x_irqs),
> +	.status_base = HI655X_IRQ_STAT_BASE,
> +	.mask_base = HI655X_IRQ_MASK_BASE,
> +};
> +
> +static unsigned int hi655x_pmic_get_version(struct hi655x_pmic *pmic)
> +{
> +	u32 val;
> +
> +	regmap_read(pmic->regmap,
> +		    HI655X_BUS_ADDR(HI655X_VER_REG), &val);
> +
> +	return val;
> +}

This is a small function that you only use once.

Probably better just to call regmap_read() directly from below.

> +static struct regmap_config hi655x_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = HI655X_STRIDE,
> +	.val_bits = 8,
> +	.max_register = HI655X_BUS_ADDR(0xFFF),
> +};
> +
> +static void hi655x_local_irq_clear(struct regmap *map)
> +{
> +	int i;
> +
> +	regmap_write(map, HI655X_ANA_IRQM_BASE, HI655X_IRQ_CLR);
> +	for (i = 0; i < HI655X_IRQ_ARRAY; i++) {
> +		regmap_write(map, HI655X_IRQ_STAT_BASE + i * HI655X_STRIDE,
> +			     HI655X_IRQ_CLR);
> +	}
> +}
> +
> +static int hi655x_pmic_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct hi655x_pmic *pmic;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	void __iomem *base;
> +
> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);

You need a NULL check here.

> +	pmic->dev = dev;
> +
> +	pmic->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!pmic->res) {
> +		dev_err(dev, "platform_get_resource err\n");

This is not a good error message.

Besides, we don't normally try to catch this error, as it's checked
for you in devm_ioremap_resource().

> +		return -ENOENT;
> +	}

'\n' here.

> +	base = devm_ioremap_resource(dev, pmic->res);
> +	if (!base) {
> +		dev_err(dev, "cannot map register memory\n");

devm_ioremap_resource() already prints a message on error.  No need
for this.

> +		return -ENOMEM;
> +	}
> +	pmic->regmap = devm_regmap_init_mmio_clk(dev, NULL, base,
> +						 &hi655x_regmap_config);

You need to check for an error here.

> +	pmic->ver = hi655x_pmic_get_version(pmic);
> +	if ((pmic->ver < PMU_VER_START) || (pmic->ver > PMU_VER_END)) {
> +		dev_warn(dev, "it is wrong pmu version\n");

You can't issue a _warn, then return an error.  Please use _err.

... and change to "PMU version %d unsupported".

> +		return -EINVAL;
> +	}
> +
> +	hi655x_local_irq_clear(pmic->regmap);
> +
> +	pmic->gpio = of_get_named_gpio(np, "pmic-gpios", 0);
> +	if (!gpio_is_valid(pmic->gpio)) {
> +		dev_err(dev, "cannot get the pmic-gpios\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = devm_gpio_request_one(dev, pmic->gpio, GPIOF_IN,
> +				    "hi655x_pmic_irq");
> +	if (ret < 0) {
> +		dev_err(dev, "failed to request gpio %d  ret = %d\n",

More readable if you use the IRQ name, as you know it.

"Failed to obtain 'hi655x_pmic_irq'"

> +			pmic->gpio, ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_add_irq_chip(pmic->regmap, gpio_to_irq(pmic->gpio),
> +				  IRQF_TRIGGER_LOW | IRQF_NO_SUSPEND, 0,
> +				  &hi655x_irq_chip, &pmic->irq_data);
> +	if (ret) {
> +		dev_err(dev, "add pmic irq chip error! ret %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* bind pmic to device */

"Bind PMIC to device"

It would be better if you dropped the comment completely to be honest.

> +	platform_set_drvdata(pdev, pmic);
> +
> +	ret = mfd_add_devices(dev, 0, hi655x_pmic_devs,

Please use the #defines for the second parameter.

> +			      ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(dev, "add mfd devices failed: %d\n", ret);

"Failed to register device".

> +		regmap_del_irq_chip(pmic->irq, pmic->irq_data);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hi655x_pmic_remove(struct platform_device *pdev)
> +{
> +	mfd_remove_devices(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_hi655x_pmic_match_tbl[] = {
> +	{ .compatible = "hisilicon,hi655x-pmic", },
> +	{},
> +};
> +
> +static struct platform_driver hi655x_pmic_driver = {
> +	.driver	= {
> +		.name =	"hi655x-pmic",
> +		.of_match_table = of_hi655x_pmic_match_tbl,

of_match_ptr()

> +	},
> +	.probe  = hi655x_pmic_probe,
> +	.remove = hi655x_pmic_remove,
> +};
> +module_platform_driver(hi655x_pmic_driver);
> +
> +MODULE_AUTHOR("Chen Feng <puck.chen@hisilicon.com>");
> +MODULE_DESCRIPTION("Hisilicon hi655x pmic driver");

PMIC

> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/hi655x-pmic.h b/include/linux/mfd/hi655x-pmic.h
> new file mode 100644
> index 0000000..e1af0a1
> --- /dev/null
> +++ b/include/linux/mfd/hi655x-pmic.h
> @@ -0,0 +1,56 @@
> +/*
> + * Device driver for regulators in hi655x IC
> + *
> + * Copyright (c) 2016 Hisilicon.
> + *
> + * Chen Feng <puck.chen@hisilicon.com>
> + * Fei  Wang <w.f@huawei.com>

Author(s):

> + * 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 __HI655X_PMIC_H
> +#define __HI655X_PMIC_H
> +
> +/* Hi655x registers are mapped to memory bus in 4 bytes stride */
> +#define HI655X_STRIDE                   (4)
> +#define HI655X_BUS_ADDR(x)              ((x) << 2)
> +
> +#define HI655X_BITS                     (8)
> +
> +#define HI655X_NR_IRQ                   (32)
> +
> +#define HI655X_IRQ_STAT_BASE            (0x003 << 2)
> +#define HI655X_IRQ_MASK_BASE            (0x007 << 2)
> +#define HI655X_ANA_IRQM_BASE            (0x1b5 << 2)
> +#define HI655X_IRQ_ARRAY                (4)
> +#define HI655X_IRQ_MASK                 (0xFF)
> +#define HI655X_IRQ_CLR                  (0xFF)
> +#define HI655X_VER_REG                  (0x00)
> +
> +#define PMU_VER_START                   (0x10)
> +#define PMU_VER_END                     (0x38)
> +
> +#define RESERVE_INT                     (BIT(7))
> +#define PWRON_D20R_INT                  (BIT(6))
> +#define PWRON_D20F_INT                  (BIT(5))
> +#define PWRON_D4SR_INT                  (BIT(4))
> +#define VSYS_6P0_D200UR_INT             (BIT(3))
> +#define VSYS_UV_D3R_INT                 (BIT(2))
> +#define VSYS_2P5_R_INT                  (BIT(1))
> +#define OTMP_D1R_INT                    (BIT(0))

No need for the extra ()'s

> +struct hi655x_pmic {
> +	struct resource *res;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct clk *clk;

Is this used?

> +	int irq;
> +	int gpio;
> +	unsigned int ver;
> +	struct regmap_irq_chip_data *irq_data;
> +};

Check to see if they are all used.

> +#endif

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

Re: [PATCH v5 1/5] doc: bindings: Add document for mfd hi665x PMIC

[chenfeng]

On 2016/1/25 20:53, Lee Jones wrote:
> On Mon, 11 Jan 2016, Chen Feng wrote:
> 
>> Add document for mfd driver hi655x pmic driver
>>
>> Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
>> Signed-off-by: Fei Wang <w.f@huawei.com>
>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> ---
>>  .../devicetree/bindings/mfd/hisilicon,hi655x.txt   | 28 ++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
>> new file mode 100644
>> index 0000000..3180c40
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi655x.txt
>> @@ -0,0 +1,28 @@
>> +Hisilicon hi655x Power Management Integrated Circuit (PMIC)
>> +
>> +The hardware layout for access PMIC Hi655x from AP SoC Hi6220.
>> +Between PMIC Hi655x and Hi6220, the physical signal channel is SSI.
>> +We can use memory-mapped I/O to communicate.
>> +
>> ++----------------+             +-------------+
>> +|                |             |             |
>> +|    Hi6220      |   SSI bus   |   Hi655x    |
>> +|                |-------------|             |
>> +|                |(REGMAP_MMIO)|             |
>> ++----------------+             +-------------+
>> +
>> +Required properties:
>> +- compatible: Should be "hisilicon,hi655x-pmic"
>> +- reg: Base address of PMIC on hi6220 soc
> 
> SoC
> 
>> +- interrupt-controller: Hi655x has internal IRQs (has own IRQ domain).
>> +- pmic-gpios: The gpio used by pmic irq.
> 
> PMIC IRQ
> 
>> +
>> +Example:
>> +	pmic: pmic@f8000000 {
>> +		compatible = "hisilicon,hi655x-pmic";
>> +		reg = <0x0 0xf8000000 0x0 0x1000>;
>> +		#interrupt-cells = <2>;
>> +		interrupt-controller;
>> +		pmic-gpios = <&gpio1 2 0>;
> 
> What's the last cell for here?  If they are flags, there is probably a
> #define you can use in dt-include.
ok, I will use the irq flag in dt-include.
> 
>> +		status = "disabled";
> 
> What's the point in disabling example code?
It's just copy from the dts, I will drop this.
> 
>> +	}
> 

Re: [PATCH v5 3/5] mfd: hi655x: Add hi665x pmic driver

[chenfeng]

On 2016/1/25 22:22, Lee Jones wrote:
> On Mon, 11 Jan 2016, Chen Feng wrote:
> 
>> Add pmic mfd driver to support hisilicon hi665x.
> 
> PMIC MFD
> 

ok.

>> Signed-off-by: Chen Feng <puck.chen@hisilicon.com>
>> Signed-off-by: Fei Wang <w.f@huawei.com>
>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> ---
>>  drivers/mfd/Kconfig             |  10 +++
>>  drivers/mfd/Makefile            |   1 +
>>  drivers/mfd/hi655x-pmic.c       | 169 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/hi655x-pmic.h |  56 +++++++++++++
>>  4 files changed, 236 insertions(+)
>>  create mode 100644 drivers/mfd/hi655x-pmic.c
>>  create mode 100644 include/linux/mfd/hi655x-pmic.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 4d92df6..299d972 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -284,6 +284,16 @@ config MFD_HI6421_PMIC
>>  	  menus in order to enable them.
>>  	  We communicate with the Hi6421 via memory-mapped I/O.
>>  
>> +config MFD_HI655X_PMIC
>> +	tristate "HiSilicon Hi655X series PMU/Codec IC"
>> +	depends on ARCH_HISI || (COMPILE_TEST && ARM64)
> 
> Why not just COMPILE_TEST?

ok, the V6 already just COMPILE_TEST.
> 
>> +	depends on OF
> 
> So this will not COMPILE_TEST if OF is not enabled.
> 
>> +	select MFD_CORE
>> +	select REGMAP_MMIO
>> +	select REGMAP_IRQ
>> +	help
>> +	  Select this option to enable Hisilicon hi655x series pmic driver.
>> +
>>  config HTC_EGPIO
>>  	bool "HTC EGPIO support"
>>  	depends on GPIOLIB && ARM
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index a8b76b8..6a7b0e1 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -186,6 +186,7 @@ obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
>>  obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
>>  obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
>> +obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>>  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>> diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c
>> new file mode 100644
>> index 0000000..aab18f7
>> --- /dev/null
>> +++ b/drivers/mfd/hi655x-pmic.c
>> @@ -0,0 +1,169 @@
>> +/*
>> + * Device driver for regulators in hi655x IC
> 
> We know it's a device driver.  And I hope it's not a regulator driver.
> 
ok, I will change the name.
>> + * Copyright (c) 2016 Hisilicon.
>> + *
>> + * Chen Feng <puck.chen@hisilicon.com>
>> + * Fei  Wang <w.f@huawei.com>
> 
> Author(s): 
> 
ok.
>> + * 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/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/init.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/hi655x-pmic.h>
>> +#include <linux/regmap.h>
> 
> Alphabetical please.
> 
ok.
>> +static const struct mfd_cell hi655x_pmic_devs[] = {
>> +	{ .name = "hi655x-regulator", },
>> +};
> 
> What other devices are there?
> 

These patches only add regulator,the RTC and Power-key
will be added later.

So I just list the regulator this time.

>> +static const struct regmap_irq hi655x_irqs[] = {
>> +	{ .reg_offset = 0, .mask = OTMP_D1R_INT },
>> +	{ .reg_offset = 0, .mask = VSYS_2P5_R_INT },
>> +	{ .reg_offset = 0, .mask = VSYS_UV_D3R_INT },
>> +	{ .reg_offset = 0, .mask = VSYS_6P0_D200UR_INT },
>> +	{ .reg_offset = 0, .mask = PWRON_D4SR_INT },
>> +	{ .reg_offset = 0, .mask = PWRON_D20F_INT },
>> +	{ .reg_offset = 0, .mask = PWRON_D20R_INT },
>> +	{ .reg_offset = 0, .mask = RESERVE_INT },
>> +};
>> +
>> +static const struct regmap_irq_chip hi655x_irq_chip = {
>> +	.name = "hi655x-pmic",
>> +	.irqs = hi655x_irqs,
>> +	.num_regs = 1,
>> +	.num_irqs = ARRAY_SIZE(hi655x_irqs),
>> +	.status_base = HI655X_IRQ_STAT_BASE,
>> +	.mask_base = HI655X_IRQ_MASK_BASE,
>> +};
>> +
>> +static unsigned int hi655x_pmic_get_version(struct hi655x_pmic *pmic)
>> +{
>> +	u32 val;
>> +
>> +	regmap_read(pmic->regmap,
>> +		    HI655X_BUS_ADDR(HI655X_VER_REG), &val);
>> +
>> +	return val;
>> +}
> 
> This is a small function that you only use once.
> 
> Probably better just to call regmap_read() directly from below.
> 
ok.

>> +static struct regmap_config hi655x_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = HI655X_STRIDE,
>> +	.val_bits = 8,
>> +	.max_register = HI655X_BUS_ADDR(0xFFF),
>> +};
>> +
>> +static void hi655x_local_irq_clear(struct regmap *map)
>> +{
>> +	int i;
>> +
>> +	regmap_write(map, HI655X_ANA_IRQM_BASE, HI655X_IRQ_CLR);
>> +	for (i = 0; i < HI655X_IRQ_ARRAY; i++) {
>> +		regmap_write(map, HI655X_IRQ_STAT_BASE + i * HI655X_STRIDE,
>> +			     HI655X_IRQ_CLR);
>> +	}
>> +}
>> +
>> +static int hi655x_pmic_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct hi655x_pmic *pmic;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	void __iomem *base;
>> +
>> +	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
> 
> You need a NULL check here.
> 
ok.

>> +	pmic->dev = dev;
>> +
>> +	pmic->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!pmic->res) {
>> +		dev_err(dev, "platform_get_resource err\n");
> 
> This is not a good error message.
> 
> Besides, we don't normally try to catch this error, as it's checked
> for you in devm_ioremap_resource().
> 
>> +		return -ENOENT;
>> +	}
> 
> '\n' here.
> 
>> +	base = devm_ioremap_resource(dev, pmic->res);
>> +	if (!base) {
>> +		dev_err(dev, "cannot map register memory\n");
> 
> devm_ioremap_resource() already prints a message on error.  No need
> for this.
> 
>> +		return -ENOMEM;
>> +	}
>> +	pmic->regmap = devm_regmap_init_mmio_clk(dev, NULL, base,
>> +						 &hi655x_regmap_config);
> 
> You need to check for an error here.
ok
> 
>> +	pmic->ver = hi655x_pmic_get_version(pmic);
>> +	if ((pmic->ver < PMU_VER_START) || (pmic->ver > PMU_VER_END)) {
>> +		dev_warn(dev, "it is wrong pmu version\n");
> 
> You can't issue a _warn, then return an error.  Please use _err.
> 
> ... and change to "PMU version %d unsupported".
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	hi655x_local_irq_clear(pmic->regmap);
>> +
>> +	pmic->gpio = of_get_named_gpio(np, "pmic-gpios", 0);
>> +	if (!gpio_is_valid(pmic->gpio)) {
>> +		dev_err(dev, "cannot get the pmic-gpios\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = devm_gpio_request_one(dev, pmic->gpio, GPIOF_IN,
>> +				    "hi655x_pmic_irq");
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to request gpio %d  ret = %d\n",
> 
> More readable if you use the IRQ name, as you know it.
> 
> "Failed to obtain 'hi655x_pmic_irq'"
> 
>> +			pmic->gpio, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = regmap_add_irq_chip(pmic->regmap, gpio_to_irq(pmic->gpio),
>> +				  IRQF_TRIGGER_LOW | IRQF_NO_SUSPEND, 0,
>> +				  &hi655x_irq_chip, &pmic->irq_data);
>> +	if (ret) {
>> +		dev_err(dev, "add pmic irq chip error! ret %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* bind pmic to device */
> 
> "Bind PMIC to device"
> 
> It would be better if you dropped the comment completely to be honest.
> 
>> +	platform_set_drvdata(pdev, pmic);
>> +
>> +	ret = mfd_add_devices(dev, 0, hi655x_pmic_devs,
> 
> Please use the #defines for the second parameter.
> 
>> +			      ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL);
>> +	if (ret) {
>> +		dev_err(dev, "add mfd devices failed: %d\n", ret);
> 
> "Failed to register device".
> 
>> +		regmap_del_irq_chip(pmic->irq, pmic->irq_data);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int hi655x_pmic_remove(struct platform_device *pdev)
>> +{
>> +	mfd_remove_devices(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_hi655x_pmic_match_tbl[] = {
>> +	{ .compatible = "hisilicon,hi655x-pmic", },
>> +	{},
>> +};
>> +
>> +static struct platform_driver hi655x_pmic_driver = {
>> +	.driver	= {
>> +		.name =	"hi655x-pmic",
>> +		.of_match_table = of_hi655x_pmic_match_tbl,
> 
> of_match_ptr()
> 
>> +	},
>> +	.probe  = hi655x_pmic_probe,
>> +	.remove = hi655x_pmic_remove,
>> +};
>> +module_platform_driver(hi655x_pmic_driver);
>> +
>> +MODULE_AUTHOR("Chen Feng <puck.chen@hisilicon.com>");
>> +MODULE_DESCRIPTION("Hisilicon hi655x pmic driver");
> 
> PMIC
> 
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/hi655x-pmic.h b/include/linux/mfd/hi655x-pmic.h
>> new file mode 100644
>> index 0000000..e1af0a1
>> --- /dev/null
>> +++ b/include/linux/mfd/hi655x-pmic.h
>> @@ -0,0 +1,56 @@
>> +/*
>> + * Device driver for regulators in hi655x IC
>> + *
>> + * Copyright (c) 2016 Hisilicon.
>> + *
>> + * Chen Feng <puck.chen@hisilicon.com>
>> + * Fei  Wang <w.f@huawei.com>
> 
> Author(s):
> 
>> + * 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 __HI655X_PMIC_H
>> +#define __HI655X_PMIC_H
>> +
>> +/* Hi655x registers are mapped to memory bus in 4 bytes stride */
>> +#define HI655X_STRIDE                   (4)
>> +#define HI655X_BUS_ADDR(x)              ((x) << 2)
>> +
>> +#define HI655X_BITS                     (8)
>> +
>> +#define HI655X_NR_IRQ                   (32)
>> +
>> +#define HI655X_IRQ_STAT_BASE            (0x003 << 2)
>> +#define HI655X_IRQ_MASK_BASE            (0x007 << 2)
>> +#define HI655X_ANA_IRQM_BASE            (0x1b5 << 2)
>> +#define HI655X_IRQ_ARRAY                (4)
>> +#define HI655X_IRQ_MASK                 (0xFF)
>> +#define HI655X_IRQ_CLR                  (0xFF)
>> +#define HI655X_VER_REG                  (0x00)
>> +
>> +#define PMU_VER_START                   (0x10)
>> +#define PMU_VER_END                     (0x38)
>> +
>> +#define RESERVE_INT                     (BIT(7))
>> +#define PWRON_D20R_INT                  (BIT(6))
>> +#define PWRON_D20F_INT                  (BIT(5))
>> +#define PWRON_D4SR_INT                  (BIT(4))
>> +#define VSYS_6P0_D200UR_INT             (BIT(3))
>> +#define VSYS_UV_D3R_INT                 (BIT(2))
>> +#define VSYS_2P5_R_INT                  (BIT(1))
>> +#define OTMP_D1R_INT                    (BIT(0))
> 
> No need for the extra ()'s
> 
>> +struct hi655x_pmic {
>> +	struct resource *res;
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct clk *clk;
> 
> Is this used?
> 
>> +	int irq;
>> +	int gpio;
>> +	unsigned int ver;
>> +	struct regmap_irq_chip_data *irq_data;
>> +};
> 
> Check to see if they are all used.
> 
>> +#endif
> 

The other comments are all accepted, thanks for your review.

Re: [PATCH v5 3/5] mfd: hi655x: Add hi665x pmic driver

[chenfeng]

Hi Lee,

Thanks for your review!
There is one things need your confirm.
Please help to see it below.

On 2016/1/25 22:22, Lee Jones wrote:
> On Mon, 11 Jan 2016, Chen Feng wrote:
> 
>> Add pmic mfd driver to support hisilicon hi665x.
[..]

> 
>> +static const struct mfd_cell hi655x_pmic_devs[] = {
>> +	{ .name = "hi655x-regulator", },
>> +};
> 
> What other devices are there?
> 

Current the MFD PMIC driver only has regulator enable.

The RTC & Power-key are not in these patch sets.

They will be added later.

Will this be accepted?

>> +static const struct regmap_irq hi655x_irqs[] = {
>> +	{ .reg_offset = 0, .mask = OTMP_D1R_INT },

Re: [PATCH v5 3/5] mfd: hi655x: Add hi665x pmic driver

[Lee Jones]

On Thu, 28 Jan 2016, chenfeng wrote:

> Hi Lee,
> 
> Thanks for your review!
> There is one things need your confirm.
> Please help to see it below.
> 
> On 2016/1/25 22:22, Lee Jones wrote:
> > On Mon, 11 Jan 2016, Chen Feng wrote:
> > 
> >> Add pmic mfd driver to support hisilicon hi665x.
> [..]
> 
> > 
> >> +static const struct mfd_cell hi655x_pmic_devs[] = {
> >> +	{ .name = "hi655x-regulator", },
> >> +};
> > 
> > What other devices are there?
> > 
> 
> Current the MFD PMIC driver only has regulator enable.
> 
> The RTC & Power-key are not in these patch sets.
> 
> They will be added later.
> 
> Will this be accepted?

Yes.

> >> +static const struct regmap_irq hi655x_irqs[] = {
> >> +	{ .reg_offset = 0, .mask = OTMP_D1R_INT },
> 

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