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

[PATCH 0/6] 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.

In order to enable future extension of Hi6421v530 functionality, a new
mfd driver and regulator driver are added in this patchset. Only header
file hi6421-pmic.h is reused between them. Patch 2 is for just this
purpose.

Patch 3 and 4 adds mfd and regulator driver respectively.
Patch 5 is dts change, it depends and can be applied on hi3660/hikey960
        patchset [1].
Patch 6 enables the relevant config items.

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

Guodong Xu (4):
  dt-bindings: mfd: Add hi6421v530 bindings
  mfd: hi6421-pmic: move hi6421_regmap_config definition to header file
  mfd: hi6421v530: add support for HiSilicon Hi6421v530
  arm64: defconfig: enable hi6421v530 MFD and regulator

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

 .../bindings/mfd/hisilicon,hi6421v530.txt          |  25 ++
 arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts  |  60 ++++
 arch/arm64/configs/defconfig                       |   2 +
 drivers/mfd/Kconfig                                |   9 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/hi6421-pmic-core.c                     |   7 -
 drivers/mfd/hi6421v530-pmic.c                      |  92 ++++++
 drivers/regulator/Kconfig                          |  10 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/hi6421v530-regulator.c           | 355 +++++++++++++++++++++
 include/linux/mfd/hi6421-pmic.h                    |   6 +
 11 files changed, 561 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
 create mode 100644 drivers/mfd/hi6421v530-pmic.c
 create mode 100644 drivers/regulator/hi6421v530-regulator.c

-- 
2.10.2

[PATCH 1/6] dt-bindings: mfd: Add hi6421v530 bindings

[Guodong Xu]

DT bindings for hisilicon HI655x PMIC chip.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 .../bindings/mfd/hisilicon,hi6421v530.txt          | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt

diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
new file mode 100644
index 0000000..6ffe6f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
@@ -0,0 +1,25 @@
+Hisilicon Hi6421v530 Power Management Integrated Circuit (PMIC)
+
+The hardware layout for access PMIC Hi6421v530 from AP SoC Hi3660.
+Between PMIC Hi6421v530 and Hi3660, the physical signal channel is SSI.
+We can use memory-mapped I/O to communicate.
+
++----------------+             +-------------+
+|                |             |             |
+|    Hi3660      |   SSI bus   |  Hi6421v530 |
+|                |-------------|             |
+|                |(REGMAP_MMIO)|             |
++----------------+             +-------------+
+
+Required properties:
+- compatible:           Should be "hisilicon,hi6421v530-pmic".
+- reg:                  Base address of PMIC on Hi3660 SoC.
+- interrupt-controller: Hi6421v530 has internal IRQs (has own IRQ domain).
+
+Example:
+	pmic: pmic@fff34000 {
+		compatible = "hisilicon,hi6421v530-pmic";
+		reg = <0x0 0xfff34000 0x0 0x1000>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	}
-- 
2.10.2

Re: [PATCH 1/6] dt-bindings: mfd: Add hi6421v530 bindings

[Rob Herring]

On Fri, May 26, 2017 at 02:35:13PM +0800, Guodong Xu wrote:
> DT bindings for hisilicon HI655x PMIC chip.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  .../bindings/mfd/hisilicon,hi6421v530.txt          | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
> new file mode 100644
> index 0000000..6ffe6f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
> @@ -0,0 +1,25 @@
> +Hisilicon Hi6421v530 Power Management Integrated Circuit (PMIC)
> +
> +The hardware layout for access PMIC Hi6421v530 from AP SoC Hi3660.
> +Between PMIC Hi6421v530 and Hi3660, the physical signal channel is SSI.
> +We can use memory-mapped I/O to communicate.
> +
> ++----------------+             +-------------+
> +|                |             |             |
> +|    Hi3660      |   SSI bus   |  Hi6421v530 |
> +|                |-------------|             |
> +|                |(REGMAP_MMIO)|             |
> ++----------------+             +-------------+

regmap is a Linuxism and should not be part of the binding.

So there some sort of controller that generates SSI packets? based on 
MMIO addresses? That should be more fully described here. For example, 
the PMIC should probably be a child of the controller.

Re: [PATCH 1/6] dt-bindings: mfd: Add hi6421v530 bindings

[Guodong Xu]

On Thu, Jun 1, 2017 at 2:07 AM, Rob Herring <robh@kernel.org> wrote:
> On Fri, May 26, 2017 at 02:35:13PM +0800, Guodong Xu wrote:
>> DT bindings for hisilicon HI655x PMIC chip.
>>
>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>> ---
>>  .../bindings/mfd/hisilicon,hi6421v530.txt          | 25 ++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
>> new file mode 100644
>> index 0000000..6ffe6f6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421v530.txt
>> @@ -0,0 +1,25 @@
>> +Hisilicon Hi6421v530 Power Management Integrated Circuit (PMIC)
>> +
>> +The hardware layout for access PMIC Hi6421v530 from AP SoC Hi3660.
>> +Between PMIC Hi6421v530 and Hi3660, the physical signal channel is SSI.
>> +We can use memory-mapped I/O to communicate.
>> +
>> ++----------------+             +-------------+
>> +|                |             |             |
>> +|    Hi3660      |   SSI bus   |  Hi6421v530 |
>> +|                |-------------|             |
>> +|                |(REGMAP_MMIO)|             |
>> ++----------------+             +-------------+
>
> regmap is a Linuxism and should not be part of the binding.
>
> So there some sort of controller that generates SSI packets? based on
> MMIO addresses? That should be more fully described here. For example,
> the PMIC should probably be a child of the controller.

Hi, Rob

Thanks for review. I just sent v2 of this patchset, according to
review comments I got. In v2, I discarded this new binding file.
Actually, I extended the existing
Documentation/devicetree/bindings/mfd/hi6421.txt to make it support
v530. There is no regmap in that.

Yes, there is a controller to generate SSI packets. However that is
completely blind to main SoC. It's more MMIO.

-Guodong

[PATCH 2/6] mfd: hi6421-pmic: move hi6421_regmap_config definition to header file

[Guodong Xu]

Move hi6421_regmap_config definition from c code to common header:
 - include/linux/mfd/hi6421-pmic.h

This is to improve code re-use for upcoming hi6421 series of MFD driver.

Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/mfd/hi6421-pmic-core.c  | 7 -------
 include/linux/mfd/hi6421-pmic.h | 6 ++++++
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/hi6421-pmic-core.c b/drivers/mfd/hi6421-pmic-core.c
index 3fd703f..dc1b5cf 100644
--- a/drivers/mfd/hi6421-pmic-core.c
+++ b/drivers/mfd/hi6421-pmic-core.c
@@ -35,13 +35,6 @@ static const struct mfd_cell hi6421_devs[] = {
 	{ .name = "hi6421-regulator", },
 };
 
-static const struct regmap_config hi6421_regmap_config = {
-	.reg_bits = 32,
-	.reg_stride = 4,
-	.val_bits = 8,
-	.max_register = HI6421_REG_TO_BUS_ADDR(HI6421_REG_MAX),
-};
-
 static int hi6421_pmic_probe(struct platform_device *pdev)
 {
 	struct hi6421_pmic *pmic;
diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
index 587273e..f4674ff 100644
--- a/include/linux/mfd/hi6421-pmic.h
+++ b/include/linux/mfd/hi6421-pmic.h
@@ -38,4 +38,10 @@ struct hi6421_pmic {
 	struct regmap		*regmap;
 };
 
+static const struct regmap_config hi6421_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 8,
+	.max_register = HI6421_REG_TO_BUS_ADDR(HI6421_REG_MAX),
+};
 #endif		/* __HI6421_PMIC_H */
-- 
2.10.2

Re: [PATCH 2/6] mfd: hi6421-pmic: move hi6421_regmap_config definition to header file

[Arnd Bergmann]

On Fri, May 26, 2017 at 8:35 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> Move hi6421_regmap_config definition from c code to common header:
>  - include/linux/mfd/hi6421-pmic.h
>
> This is to improve code re-use for upcoming hi6421 series of MFD driver.
>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>

> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
> index 587273e..f4674ff 100644
> --- a/include/linux/mfd/hi6421-pmic.h
> +++ b/include/linux/mfd/hi6421-pmic.h
> @@ -38,4 +38,10 @@ struct hi6421_pmic {
>         struct regmap           *regmap;
>  };
>
> +static const struct regmap_config hi6421_regmap_config = {
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 8,
> +       .max_register = HI6421_REG_TO_BUS_ADDR(HI6421_REG_MAX),
> +};
>  #endif         /* __HI6421_PMIC_H */

Header files should not have static variables in general, it will cause warnings
about unused variables when you include the header from another file
(depending on compiler version and warning options, I think older gcc
versions don't warn about this, but clang and latest gcc do).

How about adding the new code into the existing
drivers/mfd/hi6421-pmic-core.c file, and splitting out the part that differs
(the regmap_update_bits is the only difference I see) into a callback
that you reference through the of_device_id->data pointer?

        Arnd

Re: [PATCH 2/6] mfd: hi6421-pmic: move hi6421_regmap_config definition to header file

[Guodong Xu]

On Fri, May 26, 2017 at 4:33 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, May 26, 2017 at 8:35 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
>> Move hi6421_regmap_config definition from c code to common header:
>>  - include/linux/mfd/hi6421-pmic.h
>>
>> This is to improve code re-use for upcoming hi6421 series of MFD driver.
>>
>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>
>> diff --git a/include/linux/mfd/hi6421-pmic.h b/include/linux/mfd/hi6421-pmic.h
>> index 587273e..f4674ff 100644
>> --- a/include/linux/mfd/hi6421-pmic.h
>> +++ b/include/linux/mfd/hi6421-pmic.h
>> @@ -38,4 +38,10 @@ struct hi6421_pmic {
>>         struct regmap           *regmap;
>>  };
>>
>> +static const struct regmap_config hi6421_regmap_config = {
>> +       .reg_bits = 32,
>> +       .reg_stride = 4,
>> +       .val_bits = 8,
>> +       .max_register = HI6421_REG_TO_BUS_ADDR(HI6421_REG_MAX),
>> +};
>>  #endif         /* __HI6421_PMIC_H */
>
> Header files should not have static variables in general, it will cause warnings
> about unused variables when you include the header from another file
> (depending on compiler version and warning options, I think older gcc
> versions don't warn about this, but clang and latest gcc do).
>

I will fix that.

> How about adding the new code into the existing
> drivers/mfd/hi6421-pmic-core.c file, and splitting out the part that differs
> (the regmap_update_bits is the only difference I see)

Yes, indeed.

> into a callback
> that you reference through the of_device_id->data pointer?
>

Thanks Arnd. I will look into this. I'll drop hi6421v530-pmic.c and
resend this patchset.

-Guodong

>         Arnd

[PATCH 3/6] mfd: hi6421v530: 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 regulator details, eg. LDO voltage points.

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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3eb5c93..bdc8304 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -388,6 +388,15 @@ config MFD_HI6421_PMIC
 	  menus in order to enable them.
 	  We communicate with the Hi6421 via memory-mapped I/O.
 
+config MFD_HI6421V530_PMIC
+	tristate "HiSilicon Hi6421v530 PMU/Codec IC"
+	depends on OF
+	select MFD_CORE
+	select REGMAP_MMIO
+	help
+	  Add support for HiSilicon Hi6421v530 PMIC. Hi6421v530 communicates
+	  with main SoC via memory-mapped I/O.
+
 config MFD_HI655X_PMIC
 	tristate "HiSilicon Hi655X series PMU/Codec IC"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c16bf1e..cde62fc 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -206,6 +206,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_HI6421V530_PMIC)	+= hi6421v530-pmic.o
 obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
 obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
diff --git a/drivers/mfd/hi6421v530-pmic.c b/drivers/mfd/hi6421v530-pmic.c
new file mode 100644
index 0000000..651659a
--- /dev/null
+++ b/drivers/mfd/hi6421v530-pmic.c
@@ -0,0 +1,92 @@
+/*
+ * Device driver for Hi6421 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 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/>.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/hi6421-pmic.h>
+
+static const struct mfd_cell hi6421v530_devs[] = {
+	{ .name = "hi6421v530-regulator", },
+};
+
+static int hi6421v530_pmic_probe(struct platform_device *pdev)
+{
+	struct hi6421_pmic *pmic;
+	struct resource *res;
+	void __iomem *base;
+	int ret;
+
+	pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
+	if (!pmic)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
+						 &hi6421_regmap_config);
+	if (IS_ERR(pmic->regmap)) {
+		dev_err(&pdev->dev,
+			"regmap init failed: %ld\n", PTR_ERR(pmic->regmap));
+		return PTR_ERR(pmic->regmap);
+	}
+
+	platform_set_drvdata(pdev, pmic);
+
+	ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421v530_devs,
+				   ARRAY_SIZE(hi6421v530_devs), NULL, 0, NULL);
+	if (ret) {
+		dev_err(&pdev->dev, "add mfd devices failed: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_hi6421v530_pmic_match_tbl[] = {
+	{ .compatible = "hisilicon,hi6421v530-pmic", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_hi6421v530_pmic_match_tbl);
+
+static struct platform_driver hi6421v530_pmic_driver = {
+	.driver = {
+		.name	= "hi6421v530_pmic",
+		.of_match_table = of_hi6421v530_pmic_match_tbl,
+	},
+	.probe	= hi6421v530_pmic_probe,
+};
+module_platform_driver(hi6421v530_pmic_driver);
+
+MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
+MODULE_DESCRIPTION("Hi6421v530 PMIC driver");
+MODULE_LICENSE("GPL v2");
-- 
2.10.2

Re: [PATCH 3/6] mfd: hi6421v530: add support for HiSilicon Hi6421v530

[Lee Jones]

On Fri, 26 May 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 regulator details, eg. LDO voltage points.
> 
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
> ---
>  drivers/mfd/Kconfig           |  9 +++++
>  drivers/mfd/Makefile          |  1 +
>  drivers/mfd/hi6421v530-pmic.c | 92 +++++++++++++++++++++++++++++++++++++++++++

As previously discussed, this support should be added to the existing
driver.

>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/mfd/hi6421v530-pmic.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3eb5c93..bdc8304 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -388,6 +388,15 @@ config MFD_HI6421_PMIC
>  	  menus in order to enable them.
>  	  We communicate with the Hi6421 via memory-mapped I/O.
>  
> +config MFD_HI6421V530_PMIC
> +	tristate "HiSilicon Hi6421v530 PMU/Codec IC"
> +	depends on OF
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	help
> +	  Add support for HiSilicon Hi6421v530 PMIC. Hi6421v530 communicates
> +	  with main SoC via memory-mapped I/O.
> +
>  config MFD_HI655X_PMIC
>  	tristate "HiSilicon Hi655X series PMU/Codec IC"
>  	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c16bf1e..cde62fc 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -206,6 +206,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_HI6421V530_PMIC)	+= hi6421v530-pmic.o
>  obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
> diff --git a/drivers/mfd/hi6421v530-pmic.c b/drivers/mfd/hi6421v530-pmic.c
> new file mode 100644
> index 0000000..651659a
> --- /dev/null
> +++ b/drivers/mfd/hi6421v530-pmic.c
> @@ -0,0 +1,92 @@
> +/*
> + * Device driver for Hi6421 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 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/>.

Can you use the shorter licence script?

> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/hi6421-pmic.h>

Alphabetical.

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

Chen Feng promised me that he would add other devices to the original
driver nearly 18 months ago.  Until more devices are added, these are
not MFD drivers.  When will you add the remaining devices?

> +static int hi6421v530_pmic_probe(struct platform_device *pdev)
> +{
> +	struct hi6421_pmic *pmic;
> +	struct resource *res;
> +	void __iomem *base;
> +	int ret;
> +
> +	pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
> +						 &hi6421_regmap_config);
> +	if (IS_ERR(pmic->regmap)) {
> +		dev_err(&pdev->dev,
> +			"regmap init failed: %ld\n", PTR_ERR(pmic->regmap));

"Failed to initialise Regmap"

> +		return PTR_ERR(pmic->regmap);
> +	}
> +
> +	platform_set_drvdata(pdev, pmic);
> +
> +	ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421v530_devs,

Use the #defines provided instead of '0'.

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

"Failed to add child devices"

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_hi6421v530_pmic_match_tbl[] = {
> +	{ .compatible = "hisilicon,hi6421v530-pmic", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_hi6421v530_pmic_match_tbl);

Drop the "_tbl" part, it's implied and superfluous.

> +static struct platform_driver hi6421v530_pmic_driver = {
> +	.driver = {
> +		.name	= "hi6421v530_pmic",

One space please.

> +		.of_match_table = of_hi6421v530_pmic_match_tbl,
> +	},
> +	.probe	= hi6421v530_pmic_probe,
> +};
> +module_platform_driver(hi6421v530_pmic_driver);
> +
> +MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
> +MODULE_DESCRIPTION("Hi6421v530 PMIC driver");
> +MODULE_LICENSE("GPL v2");

This does not match the header comment.

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

Re: [PATCH 3/6] mfd: hi6421v530: add support for HiSilicon Hi6421v530

[Guodong Xu]

On Tue, May 30, 2017 at 3:36 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 26 May 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 regulator details, eg. LDO voltage points.
>>
>> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
>> Signed-off-by: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
>> ---
>>  drivers/mfd/Kconfig           |  9 +++++
>>  drivers/mfd/Makefile          |  1 +
>>  drivers/mfd/hi6421v530-pmic.c | 92 +++++++++++++++++++++++++++++++++++++++++++
>
> As previously discussed, this support should be added to the existing
> driver.

Yes, I will do that.

>
>>  3 files changed, 102 insertions(+)
>>  create mode 100644 drivers/mfd/hi6421v530-pmic.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 3eb5c93..bdc8304 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -388,6 +388,15 @@ config MFD_HI6421_PMIC
>>         menus in order to enable them.
>>         We communicate with the Hi6421 via memory-mapped I/O.
>>
>> +config MFD_HI6421V530_PMIC
>> +     tristate "HiSilicon Hi6421v530 PMU/Codec IC"
>> +     depends on OF
>> +     select MFD_CORE
>> +     select REGMAP_MMIO
>> +     help
>> +       Add support for HiSilicon Hi6421v530 PMIC. Hi6421v530 communicates
>> +       with main SoC via memory-mapped I/O.
>> +
>>  config MFD_HI655X_PMIC
>>       tristate "HiSilicon Hi655X series PMU/Codec IC"
>>       depends on ARCH_HISI || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index c16bf1e..cde62fc 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -206,6 +206,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_HI6421V530_PMIC)    += hi6421v530-pmic.o
>>  obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>>  obj-$(CONFIG_MFD_DLN2)               += dln2.o
>>  obj-$(CONFIG_MFD_RT5033)     += rt5033.o
>> diff --git a/drivers/mfd/hi6421v530-pmic.c b/drivers/mfd/hi6421v530-pmic.c
>> new file mode 100644
>> index 0000000..651659a
>> --- /dev/null
>> +++ b/drivers/mfd/hi6421v530-pmic.c
>> @@ -0,0 +1,92 @@
>> +/*
>> + * Device driver for Hi6421 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 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/>.
>
> Can you use the shorter licence script?
>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/hi6421-pmic.h>
>
> Alphabetical.
>

I will update that in the existing drivers/mfd/hi6421-pmic-core.c
together with my other changes.

>> +static const struct mfd_cell hi6421v530_devs[] = {
>> +     { .name = "hi6421v530-regulator", },
>> +};
>
> Chen Feng promised me that he would add other devices to the original
> driver nearly 18 months ago.  Until more devices are added, these are
> not MFD drivers.  When will you add the remaining devices?
>

New devices will be added when enabling functions on the hikey960
board. v530 provides a clk for wifi/bt, that can be added very quick.

>> +static int hi6421v530_pmic_probe(struct platform_device *pdev)
>> +{
>> +     struct hi6421_pmic *pmic;
>> +     struct resource *res;
>> +     void __iomem *base;
>> +     int ret;
>> +
>> +     pmic = devm_kzalloc(&pdev->dev, sizeof(*pmic), GFP_KERNEL);
>> +     if (!pmic)
>> +             return -ENOMEM;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     base = devm_ioremap_resource(&pdev->dev, res);
>> +     if (IS_ERR(base))
>> +             return PTR_ERR(base);
>> +
>> +     pmic->regmap = devm_regmap_init_mmio_clk(&pdev->dev, NULL, base,
>> +                                              &hi6421_regmap_config);
>> +     if (IS_ERR(pmic->regmap)) {
>> +             dev_err(&pdev->dev,
>> +                     "regmap init failed: %ld\n", PTR_ERR(pmic->regmap));
>
> "Failed to initialise Regmap"

Thanks, will update in the existing drivers/mfd/hi6421-pmic-core.c

>
>> +             return PTR_ERR(pmic->regmap);
>> +     }
>> +
>> +     platform_set_drvdata(pdev, pmic);
>> +
>> +     ret = devm_mfd_add_devices(&pdev->dev, 0, hi6421v530_devs,
>
> Use the #defines provided instead of '0'.
>
>> +                                ARRAY_SIZE(hi6421v530_devs), NULL, 0, NULL);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "add mfd devices failed: %d\n", ret);
>
> "Failed to add child devices"
>

Thanks, will update in the existing drivers/mfd/hi6421-pmic-core.c

>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id of_hi6421v530_pmic_match_tbl[] = {
>> +     { .compatible = "hisilicon,hi6421v530-pmic", },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_hi6421v530_pmic_match_tbl);
>
> Drop the "_tbl" part, it's implied and superfluous.
>

Will update.

>> +static struct platform_driver hi6421v530_pmic_driver = {
>> +     .driver = {
>> +             .name   = "hi6421v530_pmic",
>
> One space please.
>

Thanks, will update.

I will send updates in v3 soon.

-Guodong

>> +             .of_match_table = of_hi6421v530_pmic_match_tbl,
>> +     },
>> +     .probe  = hi6421v530_pmic_probe,
>> +};
>> +module_platform_driver(hi6421v530_pmic_driver);
>> +
>> +MODULE_AUTHOR("Guodong Xu <guodong.xu@linaro.org>");
>> +MODULE_DESCRIPTION("Hi6421v530 PMIC driver");
>> +MODULE_LICENSE("GPL v2");
>
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/6] 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 | 355 +++++++++++++++++++++++++++++++
 3 files changed, 366 insertions(+)
 create mode 100644 drivers/regulator/hi6421v530-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 48db87d..c389ce8 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_HI6421V530_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..82854d0
--- /dev/null
+++ b/drivers/regulator/hi6421v530-regulator.c
@@ -0,0 +1,355 @@
+/*
+ * 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>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/mfd/hi6421-pmic.h>
+#include <linux/io.h>
+
+/*
+ * struct hi6421c530_regulator_pdata - Hi6421V530 regulator data
+ * of platform device.
+ * @lock: mutex to serialize regulator enable
+ */
+struct hi6421v530_regulator_pdata {
+	struct mutex lock;
+};
+
+/*
+ * 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	desc;
+	u8		mode_mask;
+	u32		eco_microamp;
+};
+
+/* HI6421v530 regulators */
+enum hi6421v530_regulator_id {
+	HI6421V530_LDO3,
+	HI6421V530_LDO9,
+	HI6421V530_LDO11,
+	HI6421V530_LDO15,
+	HI6421V530_LDO16,
+	HI6421V530_NUM_REGULATORS,
+};
+
+#define HI6421V530_REGULATOR_OF_MATCH(_name, id)	\
+{							\
+	.name = #_name,					\
+	.driver_data = (void *) HI6421V530_##id,	\
+}
+
+static struct of_regulator_match hi6421v530_regulator_match[] = {
+	HI6421V530_REGULATOR_OF_MATCH(hi6421v530_ldo3, LDO3),
+	HI6421V530_REGULATOR_OF_MATCH(hi6421v530_ldo9, LDO9),
+	HI6421V530_REGULATOR_OF_MATCH(hi6421v530_ldo11, LDO11),
+	HI6421V530_REGULATOR_OF_MATCH(hi6421v530_ldo15, LDO15),
+	HI6421V530_REGULATOR_OF_MATCH(hi6421v530_ldo16, 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)				\
+	[HI6421V530_##_id] = {						\
+		.desc = {						\
+			.name		= #_id,				\
+			.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_NUM_REGULATORS] = {
+	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 int hi6421v530_regulator_enable(struct regulator_dev *rdev)
+{
+	struct hi6421v530_regulator_pdata *pdata;
+	int ret = 0;
+
+	pdata = dev_get_drvdata(rdev->dev.parent);
+	mutex_lock(&pdata->lock);
+
+	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			rdev->desc->enable_mask,
+			1 << (ffs(rdev->desc->enable_mask) - 1));
+
+	mutex_unlock(&pdata->lock);
+	return ret;
+}
+
+static int hi6421v530_regulator_disable(struct regulator_dev *rdev)
+{
+	struct hi6421v530_regulator_pdata *pdata;
+	int ret = 0;
+
+	pdata = dev_get_drvdata(rdev->dev.parent);
+	mutex_lock(&pdata->lock);
+
+	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+		rdev->desc->enable_mask, 0);
+
+	mutex_unlock(&pdata->lock);
+	return ret;
+}
+
+static int hi6421v530_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	unsigned int reg_val = 0;
+	int ret = 0;
+
+	regmap_read(rdev->regmap, rdev->desc->enable_reg, &reg_val);
+
+	ret = (reg_val & (rdev->desc->enable_mask)) ? 1 : 0;
+	return ret;
+}
+
+static int hi6421v530_regulator_set_voltage(struct regulator_dev *rdev,
+						unsigned int sel)
+{
+	struct hi6421v530_regulator_pdata *pdata;
+	int ret = 0;
+
+	pdata = dev_get_drvdata(rdev->dev.parent);
+	mutex_lock(&pdata->lock);
+
+	ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+				rdev->desc->vsel_mask,
+				sel << (ffs(rdev->desc->vsel_mask) - 1));
+
+	mutex_unlock(&pdata->lock);
+	return ret;
+}
+
+static int hi6421v530_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	unsigned int reg_val = 0;
+	int voltage;
+
+	regmap_read(rdev->regmap, rdev->desc->vsel_reg, &reg_val);
+
+	voltage = reg_val >> (ffs(rdev->desc->vsel_mask) - 1);
+	return voltage;
+}
+
+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;
+	struct hi6421v530_regulator_pdata *pdata;
+	unsigned int new_mode;
+
+	info = rdev_get_drvdata(rdev);
+	pdata = dev_get_drvdata(rdev->dev.parent);
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		new_mode = 0;
+		break;
+	case REGULATOR_MODE_IDLE:
+		new_mode = info->mode_mask;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&pdata->lock);
+	regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			   info->mode_mask, new_mode);
+	mutex_unlock(&pdata->lock);
+
+	return 0;
+}
+
+
+static const struct regulator_ops hi6421v530_ldo_ops = {
+	.is_enabled = hi6421v530_regulator_is_enabled,
+	.enable = hi6421v530_regulator_enable,
+	.disable = hi6421v530_regulator_disable,
+	.list_voltage = regulator_list_voltage_table,
+	.map_voltage = regulator_map_voltage_ascend,
+	.get_voltage_sel = hi6421v530_regulator_get_voltage,
+	.set_voltage_sel = hi6421v530_regulator_set_voltage,
+	.get_mode = hi6421v530_regulator_ldo_get_mode,
+	.set_mode = hi6421v530_regulator_ldo_set_mode,
+};
+
+static int hi6421v530_regulator_register(struct platform_device *pdev,
+				     struct regmap *rmap,
+				     struct regulator_init_data *init_data,
+				     int id, struct device_node *np)
+{
+	struct hi6421v530_regulator_info *info = NULL;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+
+	/* assign per-regulator data */
+	info = &hi6421v530_regulator_info[id];
+
+	config.dev = &pdev->dev;
+	config.init_data = init_data;
+	config.driver_data = info;
+	config.regmap = rmap;
+	config.of_node = np;
+
+	/* register regulator with framework */
+	rdev = devm_regulator_register(&pdev->dev, &info->desc, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register regulator %s\n",
+			info->desc.name);
+		return PTR_ERR(rdev);
+	}
+
+	rdev->constraints->valid_modes_mask = info->mode_mask;
+	rdev->constraints->valid_ops_mask |=
+			REGULATOR_CHANGE_VOLTAGE | REGULATOR_CHANGE_MODE;
+
+	return 0;
+}
+
+static int hi6421v530_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np;
+	struct hi6421_pmic *pmic;
+	struct hi6421v530_regulator_pdata *pdata;
+	int i, ret = 0;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+	mutex_init(&pdata->lock);
+
+	platform_set_drvdata(pdev, pdata);
+
+	np = of_get_child_by_name(dev->parent->of_node, "regulators");
+	if (!np)
+		return -ENODEV;
+
+	ret = of_regulator_match(dev, np,
+				 hi6421v530_regulator_match,
+				 ARRAY_SIZE(hi6421v530_regulator_match));
+	of_node_put(np);
+	if (ret < 0) {
+		dev_err(dev, "Error parsing regulator init data: %d\n", ret);
+		return ret;
+	}
+
+	pmic = dev_get_drvdata(dev->parent);
+
+	for (i = 0; i < ARRAY_SIZE(hi6421v530_regulator_match); i++) {
+		ret = hi6421v530_regulator_register(pdev, pmic->regmap,
+			hi6421v530_regulator_match[i].init_data, i,
+			hi6421v530_regulator_match[i].of_node);
+
+		if (ret)
+			return ret;
+	}
+
+	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

Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530 voltage regulator

[Mark Brown]

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

On Fri, May 26, 2017 at 02:35:16PM +0800, Guodong Xu wrote:

Overall this driver needs quite a lot of modernization, it's at least a
couple of years out of date in how it's using the framework - there's
barely any use of helpers.  It does look like it should be fairly easy
to get it up to date though, it's mostly going to be a case of deleting
code that's reimplementing helpers rather than anything else.

> +/*
> + * struct hi6421c530_regulator_pdata - Hi6421V530 regulator data
> + * of platform device.
> + * @lock: mutex to serialize regulator enable
> + */
> +struct hi6421v530_regulator_pdata {
> +	struct mutex lock;
> +};

This isn't platform data so it probably shouldn't be called pdata.  I
also can't tell what the lock is protecting, every use seems to be a
call to regmap_update_bits() which is atomic anyway - could we just drop
the whole thing?

> +static int hi6421v530_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct hi6421v530_regulator_pdata *pdata;
> +	int ret = 0;
> +
> +	pdata = dev_get_drvdata(rdev->dev.parent);
> +	mutex_lock(&pdata->lock);
> +
> +	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> +			rdev->desc->enable_mask,
> +			1 << (ffs(rdev->desc->enable_mask) - 1));
> +
> +	mutex_unlock(&pdata->lock);
> +	return ret;
> +}

This looks like it should be able to use the regmap helpers for all the
enable operations rather than open coding.  

> +static int hi6421v530_regulator_set_voltage(struct regulator_dev *rdev,
> +						unsigned int sel)
> +{
> +	struct hi6421v530_regulator_pdata *pdata;
> +	int ret = 0;
> +
> +	pdata = dev_get_drvdata(rdev->dev.parent);
> +	mutex_lock(&pdata->lock);
> +
> +	ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
> +				rdev->desc->vsel_mask,
> +				sel << (ffs(rdev->desc->vsel_mask) - 1));

Same for all the voltage operations :(

> +	rdev->constraints->valid_modes_mask = info->mode_mask;
> +	rdev->constraints->valid_ops_mask |=
> +			REGULATOR_CHANGE_VOLTAGE | REGULATOR_CHANGE_MODE;

The driver should *never* modify constraints, it's up to the machine
integration to say what can be supported on a given board.

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

Don't open code this, use of_match and regulators_node.

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

Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530 voltage regulator

[Guodong Xu]

On Fri, May 26, 2017 at 7:38 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, May 26, 2017 at 02:35:16PM +0800, Guodong Xu wrote:
>
> Overall this driver needs quite a lot of modernization, it's at least a
> couple of years out of date in how it's using the framework - there's
> barely any use of helpers.  It does look like it should be fairly easy
> to get it up to date though, it's mostly going to be a case of deleting
> code that's reimplementing helpers rather than anything else.
>
>> +/*
>> + * struct hi6421c530_regulator_pdata - Hi6421V530 regulator data
>> + * of platform device.
>> + * @lock: mutex to serialize regulator enable
>> + */
>> +struct hi6421v530_regulator_pdata {
>> +     struct mutex lock;
>> +};
>
> This isn't platform data so it probably shouldn't be called pdata.  I
> also can't tell what the lock is protecting, every use seems to be a
> call to regmap_update_bits() which is atomic anyway - could we just drop
> the whole thing?

In original hi6421 chip, this lock can protect from enabling multiple
LDOs simultaneously. Because it cannot afford such surging current.

I will double check whether this is still the case for hi6421v530. If
not, I will remove the whole thing.

>
>> +static int hi6421v530_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +     struct hi6421v530_regulator_pdata *pdata;
>> +     int ret = 0;
>> +
>> +     pdata = dev_get_drvdata(rdev->dev.parent);
>> +     mutex_lock(&pdata->lock);
>> +
>> +     ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>> +                     rdev->desc->enable_mask,
>> +                     1 << (ffs(rdev->desc->enable_mask) - 1));
>> +
>> +     mutex_unlock(&pdata->lock);
>> +     return ret;
>> +}
>
> This looks like it should be able to use the regmap helpers for all the
> enable operations rather than open coding.
>


>> +static int hi6421v530_regulator_set_voltage(struct regulator_dev *rdev,
>> +                                             unsigned int sel)
>> +{
>> +     struct hi6421v530_regulator_pdata *pdata;
>> +     int ret = 0;
>> +
>> +     pdata = dev_get_drvdata(rdev->dev.parent);
>> +     mutex_lock(&pdata->lock);
>> +
>> +     ret = regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>> +                             rdev->desc->vsel_mask,
>> +                             sel << (ffs(rdev->desc->vsel_mask) - 1));
>
> Same for all the voltage operations :(
>
>> +     rdev->constraints->valid_modes_mask = info->mode_mask;
>> +     rdev->constraints->valid_ops_mask |=
>> +                     REGULATOR_CHANGE_VOLTAGE | REGULATOR_CHANGE_MODE;
>
> The driver should *never* modify constraints, it's up to the machine
> integration to say what can be supported on a given board.
>
>> +     np = of_get_child_by_name(dev->parent->of_node, "regulators");
>> +     if (!np)
>> +             return -ENODEV;
>> +
>> +     ret = of_regulator_match(dev, np,
>> +                              hi6421v530_regulator_match,
>> +                              ARRAY_SIZE(hi6421v530_regulator_match));
>
> Don't open code this, use of_match and regulators_node.

I will fix that.

Thanks Mark for your review.

-Guodong

Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530 voltage regulator

[Javier Martinez Canillas]

Hello Guodong,

On Fri, May 26, 2017 at 8:35 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
> From: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
>

[snip]

>
> +config REGULATOR_HI6421V530
> +       tristate "HiSilicon Hi6421v530 PMIC voltage regulator support"

The Kconfig symbol is tristate so the driver can be built as a module...

> +
> +static struct platform_driver hi6421v530_regulator_driver = {
> +       .driver = {
> +               .name   = "hi6421v530-regulator",
> +       },
> +       .probe  = hi6421v530_regulator_probe,
> +};
> +module_platform_driver(hi6421v530_regulator_driver);

... but the driver doesn't have a platform device ID table nor export
it as a module alias using MODULE_DEVICE_TABLE().

That means that if built as a module, it won't be autoloaded when the
"hi6421v530-regulator" device is registered by the MFD driver.

> +
> +MODULE_AUTHOR("Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>");
> +MODULE_DESCRIPTION("Hi6421v530 regulator driver");
> +MODULE_LICENSE("GPL v2");

Alternative you can add a MODULE_ALIAS().

Best regards,
Javier

Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530 voltage regulator

[Guodong Xu]

On Fri, May 26, 2017 at 8:13 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> Hello Guodong,
>
> On Fri, May 26, 2017 at 8:35 AM, Guodong Xu <guodong.xu@linaro.org> wrote:
>> From: Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>
>>
>
> [snip]
>
>>
>> +config REGULATOR_HI6421V530
>> +       tristate "HiSilicon Hi6421v530 PMIC voltage regulator support"
>
> The Kconfig symbol is tristate so the driver can be built as a module...
>
>> +
>> +static struct platform_driver hi6421v530_regulator_driver = {
>> +       .driver = {
>> +               .name   = "hi6421v530-regulator",
>> +       },
>> +       .probe  = hi6421v530_regulator_probe,
>> +};
>> +module_platform_driver(hi6421v530_regulator_driver);
>
> ... but the driver doesn't have a platform device ID table nor export
> it as a module alias using MODULE_DEVICE_TABLE().
>

I will add that.
Thanks for your review.

-Guodong

> That means that if built as a module, it won't be autoloaded when the
> "hi6421v530-regulator" device is registered by the MFD driver.
>
>> +
>> +MODULE_AUTHOR("Wang Xiaoyin <hw.wangxiaoyin@hisilicon.com>");
>> +MODULE_DESCRIPTION("Hi6421v530 regulator driver");
>> +MODULE_LICENSE("GPL v2");
>
> Alternative you can add a MODULE_ALIAS().
>
> Best regards,
> Javier

[PATCH 5/6] 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 | 60 +++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
index ca448f0..b7a404a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
@@ -97,6 +97,66 @@
 			default-state = "off";
 		};
 	};
+
+	pmic: pmic@fff34000 {
+		compatible = "hisilicon,hi6421v530-pmic";
+		reg = <0x0 0xfff34000 0x0 0x1000>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		regulators {
+			ldo1: LDO1 {
+				regulator-compatible = "hi6421v530_ldo1";
+					regulator-name = "LDO1";
+					regulator-min-microvolt = <1000000>;
+					regulator-max-microvolt = <1500000>;
+					regulator-always-on;
+					regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo3: LDO3 {
+				regulator-compatible = "hi6421v530_ldo3";
+				regulator-name = "LDO3";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <2200000>;
+				regulator-always-on;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo9: LDO9 {
+				regulator-compatible = "hi6421v530_ldo9";
+				regulator-name = "LDO9";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <240>;
+			};
+
+			ldo11: LDO11 {
+				regulator-compatible = "hi6421v530_ldo11";
+				regulator-name = "LDO11";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-enable-ramp-delay = <240>;
+			};
+
+			ldo15: LDO15 {
+				regulator-compatible = "hi6421v530_ldo15";
+				regulator-name = "LDO15";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-always-on;
+				regulator-enable-ramp-delay = <120>;
+			};
+
+			ldo16: LDO16 {
+				regulator-compatible = "hi6421v530_ldo16";
+				regulator-name = "LDO16";
+				regulator-min-microvolt = <1750000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-enable-ramp-delay = <360>;
+			};
+		};
+	};
 };
 
 &i2c0 {
-- 
2.10.2

[PATCH 6/6] arm64: defconfig: enable hi6421v530 MFD and regulator

[Guodong Xu]

Enable configs for hi6421v530 mfd and regulator driver

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..287e943 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_HI6421V530_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