diff mbox

[4/8] regulator: Hi655x: Add support for Hi655x regulator

Message ID 1443611111-3196-5-git-send-email-w.f@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wangfei (William, Euler) Sept. 30, 2015, 11:05 a.m. UTC
Add Hi655x regulator driver.

Signed-off-by: Fei Wang <w.f@huawei.com>
---
 drivers/regulator/Kconfig                  |    8 +
 drivers/regulator/Makefile                 |    1 +
 drivers/regulator/hi655x-regulator.c       |  517 ++++++++++++++++++++++++++++
 include/linux/regulator/hi655x-regulator.h |   69 ++++
 4 files changed, 595 insertions(+)
 create mode 100644 drivers/regulator/hi655x-regulator.c
 create mode 100644 include/linux/regulator/hi655x-regulator.h

Comments

Mark Brown Sept. 30, 2015, 5:58 p.m. UTC | #1
On Wed, Sep 30, 2015 at 07:05:07PM +0800, Fei Wang wrote:

> +config REGULATOR_HI655X
> +        tristate "Hisilicon HI655X PMIC regulators support"
> +        depends on ARCH_HISI
> +	depends on MFD_HI655X_PMIC && OF

You've got some tab/space confusion above.  Also, can't we have an ||
COMPILE_TEST here?

> +#define REG_VALUE_SETBITS(reg_value, pos, bits, bits_value) \
> +		(reg_value = (reg_value & \
> +		~((((unsigned int)1 << bits) - 1) << pos)) | \
> +		((unsigned int)(bits_value & \
> +		(((unsigned int)1 << bits) - 1)) << pos))
> +
> +#define REG_VALUE_GETBITS(reg_value, pos, bits) \
> +			((reg_value >> pos) & (((unsigned int)1 << bits) - 1))

These are just really hard to read, sorry, and they appear to duplicate
existing regmap functionality.  If there is a strong reason to add them
consider doing so in the core and if you can then please at least make
them regular inline functions rather than using macros.  It's much safer
and more readable.

> +static int hi655x_regulator_pmic_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret = 0;
> +	unsigned int value = 0;
> +
> +	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
> +	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
> +	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
> +
> +	/*
> +	* regulator is all set,but the pmu is only subset.
> +	* maybe this "buck"/"ldo"/"lvs" is not contrl by a core.
> +	* and in regulator have a "status" member ("okey" or "disable").
> +	*/

I'm having a hard time parsing the above comment.  Please also use the
normal kernel comment style (this is a problem throughout the driver).

> +	regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
> +	ret = (int)REG_VALUE_GETBITS(value, ctrl_data->shift,
> +					ctrl_data->mask);

This appears to just duplicate regulator core functionality for reading
enable state from a bitfield?  Also note that the cast here isn't a
great advert for the macros above.

> +static int hi655x_regulator_pmic_enable(struct regulator_dev *rdev)
> +{
> +	int ret = 0;
> +	unsigned char value_u8 = 0;
> +	unsigned int value_u32 = 0;
> +	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
> +	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
> +	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
> +
> +	REG_VALUE_SETBITS(value_u32, ctrl_data->shift, ctrl_data->mask, 0x1);
> +	value_u8  = (unsigned char)value_u32;
> +	regmap_write(rdev->regmap, ctrl_regs->enable_reg, value_u8);

I'm not *entirely* sure what this is supposed to be doing but it looks
like it's duplicating core functionality in a fashion that's really
quite hard to read.  Why not just use the core functions for setting
bits?

> +	udelay(sreg->off_on_delay);

Use the regualtor core delay functionality please.

> +static int hi655x_regulator_pmic_list_voltage_linear(struct regulator_dev *rdev,
> +				  unsigned int selector)

This is *definitely* duplicating core functionality, I think you want to
use regulator_list_voltage_linear_range() or possibly just plain
_linear() and use separate operations for the LVS regulator.

We at least need to restructure the code so that the core helper
functions are used and we don't have regulator type decisions everywhere
- the whole goal of having per regulator ops is to avoid having to open
code decisions about which regulator we're dealing with into each op
function.

> +static unsigned int hi655x_regulator_pmic_get_mode(
> +			struct regulator_dev *rdev)
> +{
> +	return REGULATOR_MODE_NORMAL;
> +}

Don't implement empty functions, remove all these.

> +	num_consumer_supplies = of_get_property(np,
> +				"hisilicon,num_consumer_supplies", NULL);
> +
> +	if ((NULL == num_consumer_supplies) || (0 == *num_consumer_supplies)) {
> +		dev_warn(dev, "%s no consumer_supplies\n", __func__);
> +		return init_data;
> +	}

Obviously the binding is completely undocumented but this is setting off
alarm bells - why is the driver even considering consumers?  Please make
sure you are using the core regulator bindings rather than open coding
something which translates into platform data (which is what this looks
like).

I'm not going to review any more of the DT code without binding
documentation.

> +	/*
> +	*initdata mem will release auto;
> +	*this is kernel 3.10 import.
> +	*/

Remove anything related to your vendor kernel support, this is not
relevant to upstream.
Wangfei (William, Euler) Oct. 8, 2015, 7:38 a.m. UTC | #2
On 2015/10/1 1:58, Mark Brown wrote:
> On Wed, Sep 30, 2015 at 07:05:07PM +0800, Fei Wang wrote:
> 
>> +config REGULATOR_HI655X
>> +        tristate "Hisilicon HI655X PMIC regulators support"
>> +        depends on ARCH_HISI
>> +	depends on MFD_HI655X_PMIC && OF
> 
> You've got some tab/space confusion above.  Also, can't we have an ||
> COMPILE_TEST here?
> 
OK, i will add it.
>> +#define REG_VALUE_SETBITS(reg_value, pos, bits, bits_value) \
>> +		(reg_value = (reg_value & \
>> +		~((((unsigned int)1 << bits) - 1) << pos)) | \
>> +		((unsigned int)(bits_value & \
>> +		(((unsigned int)1 << bits) - 1)) << pos))
>> +
>> +#define REG_VALUE_GETBITS(reg_value, pos, bits) \
>> +			((reg_value >> pos) & (((unsigned int)1 << bits) - 1))
> 
> These are just really hard to read, sorry, and they appear to duplicate
> existing regmap functionality.  If there is a strong reason to add them
> consider doing so in the core and if you can then please at least make
> them regular inline functions rather than using macros.  It's much safer
> and more readable.
> 
i agree with you ?i will refactor all the unreadable code?
>> +static int hi655x_regulator_pmic_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	int ret = 0;
>> +	unsigned int value = 0;
>> +
>> +	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
>> +	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
>> +	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
>> +
>> +	/*
>> +	* regulator is all set,but the pmu is only subset.
>> +	* maybe this "buck"/"ldo"/"lvs" is not contrl by a core.
>> +	* and in regulator have a "status" member ("okey" or "disable").
>> +	*/
> 
> I'm having a hard time parsing the above comment.  Please also use the
> normal kernel comment style (this is a problem throughout the driver).
> 
OK. i will modify all of these?
>> +	regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
>> +	ret = (int)REG_VALUE_GETBITS(value, ctrl_data->shift,
>> +					ctrl_data->mask);
> 
> This appears to just duplicate regulator core functionality for reading
> enable state from a bitfield?  Also note that the cast here isn't a
> great advert for the macros above.
> 
>> +static int hi655x_regulator_pmic_enable(struct regulator_dev *rdev)
>> +{
>> +	int ret = 0;
>> +	unsigned char value_u8 = 0;
>> +	unsigned int value_u32 = 0;
>> +	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
>> +	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
>> +	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
>> +
>> +	REG_VALUE_SETBITS(value_u32, ctrl_data->shift, ctrl_data->mask, 0x1);
>> +	value_u8  = (unsigned char)value_u32;
>> +	regmap_write(rdev->regmap, ctrl_regs->enable_reg, value_u8);
> 
> I'm not *entirely* sure what this is supposed to be doing but it looks
> like it's duplicating core functionality in a fashion that's really
> quite hard to read.  Why not just use the core functions for setting
> bits?
> 
>> +	udelay(sreg->off_on_delay);
> 
> Use the regualtor core delay functionality please.

OK?i will modify it?
> 
>> +static int hi655x_regulator_pmic_list_voltage_linear(struct regulator_dev *rdev,
>> +				  unsigned int selector)
> 
> This is *definitely* duplicating core functionality, I think you want to
> use regulator_list_voltage_linear_range() or possibly just plain
> _linear() and use separate operations for the LVS regulator.
> 
> We at least need to restructure the code so that the core helper
> functions are used and we don't have regulator type decisions everywhere
> - the whole goal of having per regulator ops is to avoid having to open
> code decisions about which regulator we're dealing with into each op
> function.
> 
OK?i will modify it?
>> +static unsigned int hi655x_regulator_pmic_get_mode(
>> +			struct regulator_dev *rdev)
>> +{
>> +	return REGULATOR_MODE_NORMAL;
>> +}
> 
> Don't implement empty functions, remove all these.
> 
>> +	num_consumer_supplies = of_get_property(np,
>> +				"hisilicon,num_consumer_supplies", NULL);
>> +
>> +	if ((NULL == num_consumer_supplies) || (0 == *num_consumer_supplies)) {
>> +		dev_warn(dev, "%s no consumer_supplies\n", __func__);
>> +		return init_data;
>> +	}
> 
> Obviously the binding is completely undocumented but this is setting off
> alarm bells - why is the driver even considering consumers?  Please make
> sure you are using the core regulator bindings rather than open coding
> something which translates into platform data (which is what this looks
> like).
> 
> I'm not going to review any more of the DT code without binding
> documentation.

I will document the dt-binding first?
> 
>> +	/*
>> +	*initdata mem will release auto;
>> +	*this is kernel 3.10 import.
>> +	*/
> 
> Remove anything related to your vendor kernel support, this is not
> relevant to upstream.
> 
Thanks?Mark?i agree with you and  will modify all of you mentioned soon?
Javier Martinez Canillas Oct. 8, 2015, 7:47 a.m. UTC | #3
Hello Fei Wang,

On Wed, Sep 30, 2015 at 1:05 PM, Fei Wang <w.f@huawei.com> wrote:
> Add Hi655x regulator driver.
>
> Signed-off-by: Fei Wang <w.f@huawei.com>
> ---

[snip]

>
> +config REGULATOR_HI655X
> +        tristate "Hisilicon HI655X PMIC regulators support"

The Kconfig symbol is tree state which means it can be built as a module...

> +
> +static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
> +       {
> +               .compatible = "hisilicon,hi655x-regulator-pmic",
> +               .data = &hi655x_regulator_pmic,
> +       },
> +       { /* end */ }
> +};

...but the OF module alias information is not built into the module so
module autoloading won't work.

Please add a MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl) here.

Best regards,
Javier
diff mbox

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 64bccff..d13210b 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -261,6 +261,14 @@  config REGULATOR_HI6421
 	  21 general purpose LDOs, 3 dedicated LDOs, and 5 BUCKs. All
 	  of them come with support to either ECO (idle) or sleep mode.
 
+config REGULATOR_HI655X
+        tristate "Hisilicon HI655X PMIC regulators support"
+        depends on ARCH_HISI
+	depends on MFD_HI655X_PMIC && OF
+        help
+          This driver provides support for the voltage regulators of the
+          Hisilicon Hi655x PMIC device.
+
 config REGULATOR_ISL9305
 	tristate "Intersil ISL9305 regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0f81749..8e4db96 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -34,6 +34,7 @@  obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_FAN53555) += fan53555.o
 obj-$(CONFIG_REGULATOR_GPIO) += gpio-regulator.o
 obj-$(CONFIG_REGULATOR_HI6421) += hi6421-regulator.o
+obj-$(CONFIG_REGULATOR_HI655X) += hi655x-regulator.o
 obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
 obj-$(CONFIG_REGULATOR_ISL9305) += isl9305.o
 obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
diff --git a/drivers/regulator/hi655x-regulator.c b/drivers/regulator/hi655x-regulator.c
new file mode 100644
index 0000000..3423a84
--- /dev/null
+++ b/drivers/regulator/hi655x-regulator.c
@@ -0,0 +1,517 @@ 
+/*
+ * Device driver for regulators in HI655X IC
+ *
+ * Copyright (c) 2015 Hisilicon.
+ *
+ * Fei Wang <w.f@huawei.com>
+ *
+ * this regulator's probe function will be called lots of times,,
+ * because of there are lots of regulator nodes in dtb.
+ * so,that's say, the driver must be inited before the regulator nodes
+ * registor to system.
+ *
+ * Makefile have proved my guess, please refor to the makefile.
+ * when the code is rebuild i hope we can build pmu sub_system.
+ * init order can not base on compile
+ */
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/delay.h>
+#include <linux/time.h>
+#include <linux/regulator/hi655x-regulator.h>
+#include <linux/mfd/hi655x-pmic.h>
+#include <linux/regmap.h>
+
+#define REG_VALUE_SETBITS(reg_value, pos, bits, bits_value) \
+		(reg_value = (reg_value & \
+		~((((unsigned int)1 << bits) - 1) << pos)) | \
+		((unsigned int)(bits_value & \
+		(((unsigned int)1 << bits) - 1)) << pos))
+
+#define REG_VALUE_GETBITS(reg_value, pos, bits) \
+			((reg_value >> pos) & (((unsigned int)1 << bits) - 1))
+
+static int hi655x_regulator_pmic_is_enabled(struct regulator_dev *rdev)
+{
+	int ret = 0;
+	unsigned int value = 0;
+
+	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
+	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
+
+	/*
+	* regulator is all set,but the pmu is only subset.
+	* maybe this "buck"/"ldo"/"lvs" is not contrl by a core.
+	* and in regulator have a "status" member ("okey" or "disable").
+	*/
+	regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
+	ret = (int)REG_VALUE_GETBITS(value, ctrl_data->shift,
+					ctrl_data->mask);
+
+	return ret;
+}
+
+static int hi655x_regulator_pmic_enable(struct regulator_dev *rdev)
+{
+	int ret = 0;
+	unsigned char value_u8 = 0;
+	unsigned int value_u32 = 0;
+	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
+	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
+
+	REG_VALUE_SETBITS(value_u32, ctrl_data->shift, ctrl_data->mask, 0x1);
+	value_u8  = (unsigned char)value_u32;
+	regmap_write(rdev->regmap, ctrl_regs->enable_reg, value_u8);
+	udelay(sreg->off_on_delay);
+
+	return ret;
+}
+
+static int hi655x_regulator_pmic_disable(struct regulator_dev *rdev)
+{
+	int ret = 0;
+	int flag = 1;
+	unsigned char value_u8 = 0;
+	unsigned int value_u32 = 0;
+
+	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_ctrl_regs  *ctrl_regs = &(sreg->ctrl_regs);
+	struct hi655x_regulator_ctrl_data  *ctrl_data = &(sreg->ctrl_data);
+
+	/*
+	* regulator is all set,but the pmu is only subset.
+	* maybe this "buck"/"ldo"/"lvs" is not contrl by a core.
+	* and in regulator have a "status" member (okey or disable).
+	* maybe we can del some regulator which is not contrl by core.
+	*/
+	if (sreg->type == PMIC_BOOST_TYPE)
+		flag = 0;
+
+	/*
+	* for flag init value = 1;
+	*/
+
+	REG_VALUE_SETBITS(value_u32, ctrl_data->shift, ctrl_data->mask, flag);
+	value_u8  = (unsigned char)value_u32;
+	regmap_write(rdev->regmap, ctrl_regs->disable_reg, value_u8);
+	return ret;
+}
+
+static int hi655x_regulator_pmic_list_voltage_linear(struct regulator_dev *rdev,
+				  unsigned int selector)
+{
+
+	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
+	/*
+	* regulator is all set,but the pmu is only subset.
+	* maybe this "buck"/"ldo"/"lvs" is not contrl by a core.
+	* and in regulator have a "status" member (okey or disable).
+	* maybe we can del some regulator which is not contrl by core.
+	* we will return min_uV
+	*/
+	if (sreg->type == PMIC_LVS_TYPE)
+		return 900000;
+
+	if (selector >= sreg->vol_numb) {
+		pr_err("selector err %s %d\n", __func__, __LINE__);
+		return -1;
+	}
+
+	return sreg->vset_table[selector];
+}
+
+static int hi655x_regulator_pmic_get_voltage(struct regulator_dev *rdev)
+{
+	int index = 0;
+	unsigned int value = 0;
+
+	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_vset_regs  *vset_regs = &(sreg->vset_regs);
+	struct hi655x_regulator_vset_data  *vset_data = &(sreg->vset_data);
+
+	if (sreg->type == PMIC_LVS_TYPE)
+		return 900000;
+
+	regmap_read(rdev->regmap, vset_regs->vset_reg, &value);
+	index = (unsigned int)REG_VALUE_GETBITS(value,
+		vset_data->shift, vset_data->mask);
+
+	return sreg->vset_table[index];
+}
+
+static int hi655x_regulator_pmic_set_voltage(struct regulator_dev *rdev,
+				int min_uV, int max_uV, unsigned *selector)
+{
+	int i = 0;
+	int ret = 0;
+	int vol = 0;
+	unsigned int value = 0;
+
+	struct hi655x_regulator *sreg = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_vset_regs  *vset_regs = &(sreg->vset_regs);
+	struct hi655x_regulator_vset_data  *vset_data = &(sreg->vset_data);
+
+	if (sreg->type == PMIC_LVS_TYPE)
+		return 0;
+	/*
+	* search the matched vol and get its index
+	*/
+	for (i = 0; i < sreg->vol_numb; i++) {
+		vol = sreg->vset_table[i];
+
+		if ((vol >= min_uV) && (vol <= max_uV))
+			break;
+	}
+
+	if (i == sreg->vol_numb)
+		return -1;
+
+
+	regmap_read(rdev->regmap, vset_regs->vset_reg, &value);
+	REG_VALUE_SETBITS(value, vset_data->shift, vset_data->mask, i);
+	regmap_write(rdev->regmap, vset_regs->vset_reg, value);
+	*selector = i;
+
+	return ret;
+}
+
+static unsigned int hi655x_regulator_pmic_get_mode(
+			struct regulator_dev *rdev)
+{
+	return REGULATOR_MODE_NORMAL;
+}
+
+static int hi655x_regulator_pmic_set_mode(struct regulator_dev *rdev,
+						unsigned int mode)
+
+{
+	return 0;
+}
+
+static unsigned int hi655x_regulator_pmic_get_optimum_mode(
+	struct regulator_dev *rdev, int input_uV, int output_uV, int load_uA)
+
+{
+	return REGULATOR_MODE_NORMAL;
+}
+
+static struct regulator_ops hi655x_regulator_pmic_rops = {
+	.is_enabled = hi655x_regulator_pmic_is_enabled,
+	.enable = hi655x_regulator_pmic_enable,
+	.disable = hi655x_regulator_pmic_disable,
+	.list_voltage = hi655x_regulator_pmic_list_voltage_linear,
+	.get_voltage = hi655x_regulator_pmic_get_voltage,
+	.set_voltage = hi655x_regulator_pmic_set_voltage,
+	.get_mode = hi655x_regulator_pmic_get_mode,
+	.set_mode = hi655x_regulator_pmic_set_mode,
+	.get_optimum_mode = hi655x_regulator_pmic_get_optimum_mode,
+};
+
+static int hi655x_regualtor_pmic_dt_parse(struct hi655x_regulator *sreg,
+					struct platform_device *pdev)
+{
+	return 0;
+}
+
+static const struct hi655x_regulator hi655x_regulator_pmic = {
+	.rdesc = {
+		.ops = &hi655x_regulator_pmic_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+	},
+	.dt_parse = hi655x_regualtor_pmic_dt_parse,
+};
+
+
+static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
+	{
+		.compatible = "hisilicon,hi655x-regulator-pmic",
+		.data = &hi655x_regulator_pmic,
+	},
+	{ /* end */ }
+};
+
+static struct regulator_init_data *hi655x_of_get_regulator_init_data(
+	struct device *dev, struct device_node *np)
+{
+	struct regulator_init_data *init_data = NULL;
+	const __be32 *num_consumer_supplies = NULL;
+	struct regulator_consumer_supply *consumer_supplies = NULL;
+	int consumer_id = 0;
+
+	init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL);
+	if (!init_data)
+		return NULL;
+
+	num_consumer_supplies = of_get_property(np,
+				"hisilicon,num_consumer_supplies", NULL);
+
+	if ((NULL == num_consumer_supplies) || (0 == *num_consumer_supplies)) {
+		dev_warn(dev, "%s no consumer_supplies\n", __func__);
+		return init_data;
+	}
+
+	init_data->num_consumer_supplies = be32_to_cpu(*num_consumer_supplies);
+	init_data->consumer_supplies = (struct regulator_consumer_supply *)
+		devm_kzalloc(dev, init_data->num_consumer_supplies *
+		sizeof(struct regulator_consumer_supply), GFP_KERNEL);
+
+	if (NULL == init_data->consumer_supplies) {
+		dev_err(dev, "%s devm_kzalloc consumer_supplies err\n",
+			__func__);
+		return NULL;
+	}
+
+	consumer_supplies = init_data->consumer_supplies;
+
+	for (consumer_id = 0; consumer_id < init_data->num_consumer_supplies;
+		consumer_id++, consumer_supplies++) {
+		int ret = of_property_read_string_index(np,
+			"hisilicon,consumer-supplies",
+			consumer_id, &consumer_supplies->supply);
+
+		if (ret) {
+			dev_err(dev,
+			"%s %s of_property_read_string_index consumer-supplies err\n",
+			__func__, np->name);
+		}
+	}
+
+	return init_data;
+}
+
+static int hi655x_of_get_regulator_constraint(
+	struct regulation_constraints *constraints, struct device_node *np)
+{
+	const __be32 *min_uV, *max_uV;
+	unsigned int *valid_modes_mask;
+	unsigned int *valid_ops_mask;
+	unsigned int *initial_mode;
+
+	if (!np)
+		return -1;
+
+	if (!constraints)
+		return -1;
+
+	(constraints)->name = of_get_property(np, "regulator-name", NULL);
+
+	min_uV = of_get_property(np, "regulator-min-microvolt", NULL);
+	if (min_uV)	{
+		(constraints)->min_uV = be32_to_cpu(*min_uV);
+		(constraints)->min_uA = be32_to_cpu(*min_uV);
+	}
+
+	max_uV = of_get_property(np, "regulator-max-microvolt", NULL);
+	if (max_uV)	{
+		(constraints)->max_uV = be32_to_cpu(*max_uV);
+		(constraints)->max_uA = be32_to_cpu(*max_uV);
+	}
+
+	valid_modes_mask = (unsigned int *)of_get_property(np,
+			"hisilicon,valid-modes-mask", NULL);
+
+	if (valid_modes_mask)
+		(constraints)->valid_modes_mask =
+			be32_to_cpu(*valid_modes_mask);
+
+	valid_ops_mask = (unsigned int *)of_get_property(np,
+				"hisilicon,valid-ops-mask", NULL);
+	if (valid_ops_mask)
+		(constraints)->valid_ops_mask =
+			be32_to_cpu(*valid_ops_mask);
+
+	initial_mode = (unsigned int *)of_get_property(np,
+				"hisilicon,initial-mode", NULL);
+	if (initial_mode)
+		(constraints)->initial_mode = be32_to_cpu(*initial_mode);
+
+	(constraints)->always_on = !!(of_find_property(np,
+				"regulator-always-on", NULL));
+
+	(constraints)->boot_on = !!(of_find_property(np,
+				"regulator-boot-on", NULL));
+	return 0;
+
+}
+
+static int hi655x_of_get_regulator_sreg(struct hi655x_regulator *sreg,
+		struct device *dev, struct device_node *np)
+{
+	int *vol_numb;
+	unsigned int *off_on_delay;
+	enum hi655x_regulator_type *regulator_type;
+	const char *status = NULL;
+	unsigned int *vset_table = NULL;
+	int *regulator_id;
+
+	status = of_get_property(np, "hisilicon,regulator-status", NULL);
+	if (status)
+		sreg->status = !(strcmp(status, "okey"));
+
+	regulator_type = (enum hi655x_regulator_type *)of_get_property(np,
+				"hisilicon,regulator-type", NULL);
+
+	if (regulator_type)
+		sreg->type = be32_to_cpu(*regulator_type);
+
+	off_on_delay = (unsigned int *)of_get_property(np,
+				"hisilicon,off-on-delay", NULL);
+	if (off_on_delay)
+		sreg->off_on_delay = be32_to_cpu(*off_on_delay);
+
+	(void)of_property_read_u32_array(np, "hisilicon,ctrl-regs",
+		(unsigned int *)(&sreg->ctrl_regs), 0x3);
+
+	(void)of_property_read_u32_array(np, "hisilicon,ctrl-data",
+		(unsigned int *)(&sreg->ctrl_data), 0x2);
+
+	(void)of_property_read_u32_array(np, "hisilicon,vset-regs",
+		(unsigned int *)(&sreg->vset_regs), 0x1);
+
+	(void)of_property_read_u32_array(np, "hisilicon,vset-data",
+		(unsigned int *)(&sreg->vset_data), 0x2);
+
+	vol_numb = (int *)of_get_property(np, "hisilicon,regulator-n-vol",
+				NULL);
+	if (vol_numb)
+		sreg->vol_numb = be32_to_cpu(*vol_numb);
+
+	regulator_id = (int *)of_get_property(np,
+		"hisilicon, hisi-scharger-regulator-id", NULL);
+
+	if (regulator_id)
+		sreg->regulator_id =  be32_to_cpu(*regulator_id);
+
+	vset_table = devm_kzalloc(dev, sreg->vol_numb * sizeof(int),
+					GFP_KERNEL);
+	if (!vset_table)
+		return -1;
+
+	(void)of_property_read_u32_array(np,
+		"hisilicon,vset-table", (unsigned int *)vset_table,
+			sreg->vol_numb);
+	sreg->vset_table = vset_table;
+
+	return 0;
+
+}
+
+static int hi655x_regulator_probe(struct platform_device *pdev)
+{
+
+	int ret = 0;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct hi655x_pmic *pmic;
+	struct regulator_dev *rdev = NULL;
+	struct regulator_desc *rdesc = NULL;
+	struct hi655x_regulator *sreg = NULL;
+	struct regulator_init_data *initdata = NULL;
+	const struct of_device_id *match = NULL;
+	const struct hi655x_regulator *template = NULL;
+	struct regulator_config config = { };
+
+	pmic = dev_get_drvdata(dev->parent);
+
+	/*
+	* build hi655x_regulator device
+	*/
+
+	/* to check which type of regulator this is */
+	match = of_match_device(of_hi655x_regulator_match_tbl, &pdev->dev);
+
+	if (NULL == match) {
+		dev_err(dev, "of match hi655x regulator fail!\n\r");
+		return -EINVAL;
+	}
+	/*tempdev is regulator device*/
+	template = match->data;
+
+	/*
+	*initdata mem will release auto;
+	*this is kernel 3.10 import.
+	*/
+
+	/*just for getting "std regulator node" value-key about constraint*/
+	initdata = hi655x_of_get_regulator_init_data(dev, np);
+	if (!initdata) {
+		dev_err(dev, "get regulator init data error !\n");
+		return -EINVAL;
+	}
+
+	ret = hi655x_of_get_regulator_constraint(&initdata->constraints, np);
+	if (!!ret) {
+		dev_err(dev, "get regulator constraint error !\n");
+		return -EINVAL;
+	}
+
+	/* TODO:hi655x regulator supports two modes */
+	sreg = kmemdup(template, sizeof(*sreg), GFP_KERNEL);
+	if (!sreg)
+		return -ENOMEM;
+
+	if (0 != hi655x_of_get_regulator_sreg(sreg, dev, np)) {
+		kfree(sreg);
+		return -EINVAL;
+	}
+
+	rdesc = &sreg->rdesc;
+	rdesc->n_voltages = sreg->vol_numb;
+	rdesc->name = initdata->constraints.name;
+	rdesc->id = sreg->regulator_id;
+	rdesc->min_uV = initdata->constraints.min_uV;
+
+	/*just for skeleton for future*/
+	/* to parse device tree data for regulator specific */
+	config.dev = &pdev->dev;
+	config.init_data = initdata;
+	config.driver_data = sreg;
+	config.regmap = pmic->regmap;
+	config.of_node = pdev->dev.of_node;
+	/* register regulator */
+	rdev = regulator_register(rdesc, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(dev, "regulator failed to register %s\n", rdesc->name);
+		ret = PTR_ERR(rdev);
+		return -EINVAL;
+	}
+
+	platform_set_drvdata(pdev, rdev);
+	regulator_has_full_constraints();
+
+	return ret;
+}
+
+static int hi655x_regulator_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver hi655x_regulator_driver = {
+	.driver = {
+		.name	= "hi655x_regulator",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_hi655x_regulator_match_tbl,
+	},
+	.probe	= hi655x_regulator_probe,
+	.remove	= hi655x_regulator_remove,
+};
+module_platform_driver(hi655x_regulator_driver);
+
+MODULE_AUTHOR("Fei Wang <w.f@huawei.com>");
+MODULE_DESCRIPTION("Hisi hi655x regulator driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regulator/hi655x-regulator.h b/include/linux/regulator/hi655x-regulator.h
new file mode 100644
index 0000000..387b352
--- /dev/null
+++ b/include/linux/regulator/hi655x-regulator.h
@@ -0,0 +1,69 @@ 
+/*
+ * Device driver for regulators in HI655X IC
+ *
+ * Copyright (c) 2015 Hisilicon.
+ *
+ * Fei Wang <w.f@huawei.com>
+ *
+ * this regulator's probe function will be called lots of times,,
+ * because of there are lots of regulator nodes in dtb.
+ * so,that's say, the driver must be inited before the regulator nodes
+ * registor to system.
+ *
+ * Makefile have proved my guess, please refor to the makefile.
+ * when the code is rebuild i hope we can build pmu sub_system.
+ * init order can not base on compile
+ */
+
+#ifndef __HISI_HI655X_REGULATOR_H__
+#define __HISI_HI655X_REGULATOR_H__
+
+enum hi655x_regulator_type {
+	PMIC_BUCK_TYPE = 0,
+	PMIC_LDO_TYPE  = 1,
+	PMIC_LVS_TYPE  = 2,
+	PMIC_BOOST_TYPE = 3,
+	MTCMOS_SC_ON_TYPE      = 4,
+	MTCMOS_ACPU_ON_TYPE    = 5,
+	SCHARGE_TYPE           = 6,
+};
+
+struct hi655x_regulator_ctrl_regs {
+	unsigned int  enable_reg;
+	unsigned int  disable_reg;
+	unsigned int  status_reg;
+};
+
+struct hi655x_regulator_vset_regs {
+	unsigned int vset_reg;
+};
+
+struct hi655x_regulator_ctrl_data {
+	int          shift;
+	unsigned int mask;
+};
+
+struct hi655x_regulator_vset_data {
+	int          shift;
+	unsigned int mask;
+};
+
+struct hi655x_regulator {
+	int status;                             /*this property in child  node*/
+	unsigned int off_on_delay;              /*this property in parent node*/
+	enum hi655x_regulator_type type;        /*this property in child  node*/
+	int regulator_id;
+
+	/*this property must be unify which is in child node*/
+	struct hi655x_regulator_ctrl_regs   ctrl_regs;
+	struct hi655x_regulator_ctrl_data   ctrl_data;
+
+	struct hi655x_regulator_vset_regs   vset_regs;
+	struct hi655x_regulator_vset_data   vset_data;
+	unsigned int                        vol_numb;
+	unsigned int                        *vset_table;
+	struct regulator_desc rdesc;
+	int (*dt_parse)(struct hi655x_regulator*, struct platform_device*);
+};
+
+#endif