Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651101 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] MFD: add driver for HiSilicon Hi6421v530 PMIC Guodong Xu <guodong.xu@linaro.org> - 2017-05-26 08:40 +0200
    [PATCH 2/6] mfd: hi6421-pmic: move hi6421_regmap_config definition to header file Guodong Xu <guodong.xu@linaro.org> - 2017-05-26 08:40 +0200
      Re: [PATCH 2/6] mfd: hi6421-pmic: move hi6421_regmap_config  definition to header file Arnd Bergmann <arnd@arndb.de> - 2017-05-26 10:40 +0200
        Re: [PATCH 2/6] mfd: hi6421-pmic: move hi6421_regmap_config  definition to header file Guodong Xu <guodong.xu@linaro.org> - 2017-05-27 05:10 +0200
    [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530 voltage regulator Guodong Xu <guodong.xu@linaro.org> - 2017-05-26 08:40 +0200
      Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530  voltage regulator Mark Brown <broonie@kernel.org> - 2017-05-26 13:40 +0200
        Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530  voltage regulator Guodong Xu <guodong.xu@linaro.org> - 2017-05-27 11:50 +0200
      Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530  voltage regulator Javier Martinez Canillas <javier@dowhile0.org> - 2017-05-26 14:20 +0200
        Re: [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530  voltage regulator Guodong Xu <guodong.xu@linaro.org> - 2017-05-27 11:50 +0200
    [PATCH 6/6] arm64: defconfig: enable hi6421v530 MFD and regulator Guodong Xu <guodong.xu@linaro.org> - 2017-05-26 08:40 +0200
    [PATCH 5/6] arm64: dts: hikey960: add device node for pmic and regulators Guodong Xu <guodong.xu@linaro.org> - 2017-05-26 08:40 +0200
    [PATCH 1/6] dt-bindings: mfd: Add hi6421v530 bindings Guodong Xu <guodong.xu@linaro.org> - 2017-05-26 08:40 +0200
      Re: [PATCH 1/6] dt-bindings: mfd: Add hi6421v530 bindings Rob Herring <robh@kernel.org> - 2017-05-31 20:10 +0200
        Re: [PATCH 1/6] dt-bindings: mfd: Add hi6421v530 bindings Guodong Xu <guodong.xu@linaro.org> - 2017-06-02 11:10 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1651103 — [PATCH 4/6] regulator: hi6421v530: add driver for hi6421v530 voltage regulator

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1651105 — [PATCH 5/6] arm64: dts: hikey960: add device node for pmic and regulators

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web