diff mbox

[v3,4/5] regulator: add regulator driver of hi655x PMIC

Message ID 1450184056-79851-5-git-send-email-puck.chen@hisilicon.com
State New
Headers show

Commit Message

Chen Feng Dec. 15, 2015, 12:54 p.m. UTC
Add driver support for HiSilicon Hi655x voltage regulators.

Signed-off-by: Chen Feng <puck.chen@hisilicon.com>

Signed-off-by: Fei Wang <w.f@huawei.com>

Tested-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>

---
 drivers/regulator/Kconfig                  |   8 +
 drivers/regulator/Makefile                 |   1 +
 drivers/regulator/hi655x-regulator.c       | 246 +++++++++++++++++++++++++++++
 include/linux/regulator/hi655x-regulator.h |  41 +++++
 4 files changed, 296 insertions(+)
 create mode 100644 drivers/regulator/hi655x-regulator.c
 create mode 100644 include/linux/regulator/hi655x-regulator.h

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Mark Brown Dec. 16, 2015, 7:16 p.m. UTC | #1
On Tue, Dec 15, 2015 at 08:54:15PM +0800, Chen Feng wrote:

> +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.

> +


On the previous version of this patch I said:

| For both of these we should have an || COMPILE_TEST and there's no need
| for either to be bool I can see, they should be tristate.

I see you have made this a tristate which is good but you've not enabled
COMPILE_TEST or indicated why - there may be a very good reason for
doing this but nobody has said what it is.  Please don't ignore review
comments, people are generally making them for a reason and are likely
to have the same concerns if issues remain unaddressed.  Having to
repeat the same comments can get repetitive and make people question the
value of time spent reviewing.  If you disagree with the review comments
that's fine but you need to reply and discuss your concerns so that the
reviewer can understand your decisions.

> +static int hi655x_is_enabled(struct regulator_dev *rdev)

> +{

> +	unsigned int value = 0;

> +

> +	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);

> +	struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;


The style here is not what we normally do - normally the struct lookups
would be first, then any other variables and there wouldn't be any blank
lines in the variable declarations.  The same applies to quite a few
functions.

> +static int hi655x_set_voltage(struct regulator_dev *rdev,

> +				int min_uV, int max_uV, unsigned *selector)

> +{


As I commented on the previous version of this driver:

| Use the standard helpers, including one of the map_voltage()s and
| set_voltage_sel_regmap(), don't open code them.

You need to at least split the map and set_voltage_sel operations.

I've stopped reviewing here.
Chen Feng Dec. 17, 2015, 3:18 a.m. UTC | #2
Mark,
Thanks very much for your review.

On 2015/12/17 3:16, Mark Brown wrote:
> On Tue, Dec 15, 2015 at 08:54:15PM +0800, Chen Feng wrote:

> 

>> +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.

>> +

> 

> On the previous version of this patch I said:

> 

> | For both of these we should have an || COMPILE_TEST and there's no need

> | for either to be bool I can see, they should be tristate.

> 

> I see you have made this a tristate which is good but you've not enabled

> COMPILE_TEST or indicated why - there may be a very good reason for

> doing this but nobody has said what it is.  Please don't ignore review

> comments, people are generally making them for a reason and are likely

> to have the same concerns if issues remain unaddressed.  Having to

> repeat the same comments can get repetitive and make people question the

> value of time spent reviewing.  If you disagree with the review comments

> that's fine but you need to reply and discuss your concerns so that the

> reviewer can understand your decisions.

> 

Sorry for this,I am busy with other issue these days.

Really appreciate your comments.

I will add the COMPILE_TEST next version.

>> +static int hi655x_is_enabled(struct regulator_dev *rdev)

>> +{

>> +	unsigned int value = 0;

>> +

>> +	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);

>> +	struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;

> 

> The style here is not what we normally do - normally the struct lookups

> would be first, then any other variables and there wouldn't be any blank

> lines in the variable declarations.  The same applies to quite a few

> functions.

> 


ok, I will fix the style problem.


>> +static int hi655x_set_voltage(struct regulator_dev *rdev,

>> +				int min_uV, int max_uV, unsigned *selector)

>> +{

> 

> As I commented on the previous version of this driver:

> 

> | Use the standard helpers, including one of the map_voltage()s and

> | set_voltage_sel_regmap(), don't open code them.

> 

I change the regulator_enable method to regulator_enable_regmap this version.

But,since the hi655x PMIC use status register to ensure the regulator is okay or not.

Three different register offset in these PMIC series.

enable register.
disable register.
status register.


I can't use regulator_is_enabled_regmap regulator_disable_regmap in helpers.c

int regulator_disable_regmap(struct regulator_dev *rdev)
{
	...
        return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
                                  rdev->desc->enable_mask, val);
	...
}
Only enable_reg in the desc struct.

I will change the set voltage to regulator_set_voltage_sel_regmap next version.


Thanks again.


> You need to at least split the map and set_voltage_sel operations.

> 

> I've stopped reviewing here.

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8df0b0e..c6ecfd7 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..e222ee7
--- /dev/null
+++ b/drivers/regulator/hi655x-regulator.c
@@ -0,0 +1,246 @@ 
+/*
+ * Device driver for regulators in hi655x IC
+ *
+ * Copyright (c) 2015 Hisilicon.
+ *
+ * Chen Feng <puck.chen@hisilicon.com>
+ * Fei  Wang <w.f@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/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>
+#include <linux/bitops.h>
+#include <linux/string.h>
+
+static int hi655x_is_enabled(struct regulator_dev *rdev)
+{
+	unsigned int value = 0;
+
+	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;
+
+	regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
+	return (value & BIT(regulator->ctrl_mask));
+}
+
+static int hi655x_disable(struct regulator_dev *rdev)
+{
+	int ret = 0;
+
+	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+	struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;
+
+	ret = regmap_write(rdev->regmap, ctrl_regs->disable_reg,
+			   BIT(regulator->ctrl_mask));
+	return ret;
+}
+
+static int hi655x_get_voltage(struct regulator_dev *rdev)
+{
+	unsigned int value = 0;
+
+	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+
+	regmap_read(rdev->regmap, regulator->vset_reg, &value);
+	value &= regulator->vset_mask;
+
+	return regulator->rdesc.volt_table[value];
+}
+
+static int hi655x_set_voltage(struct regulator_dev *rdev,
+				int min_uV, int max_uV, unsigned *selector)
+{
+	int i = 0;
+	int vol = 0;
+
+	struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
+
+	/**
+	 * search the matched vol and get its index
+	 */
+	for (i = 0; i < regulator->rdesc.n_voltages; i++) {
+		vol = regulator->rdesc.volt_table[i];
+		if ((vol >= min_uV) && (vol <= max_uV))
+			break;
+	}
+
+	if (i == regulator->rdesc.n_voltages)
+		return -EINVAL;
+
+	regmap_update_bits(rdev->regmap, regulator->vset_reg,
+			   regulator->vset_mask, i);
+
+	*selector = i;
+
+	return 0;
+}
+
+static struct regulator_ops hi655x_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = hi655x_disable,
+	.is_enabled = hi655x_is_enabled,
+	.list_voltage = regulator_list_voltage_table,
+	.get_voltage = hi655x_get_voltage,
+	.set_voltage = hi655x_set_voltage,
+};
+
+static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
+	{
+		.compatible = "hisilicon,hi655x-regulator-pmic",
+	},
+};
+MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl);
+
+/**
+ * get the hi655x specific data from dt node.
+ */
+static int of_get_hi655x_ctr(struct hi655x_regulator *regulator,
+			     struct device *dev, struct device_node *np)
+{
+	int ret;
+	u32 *vset_table;
+	u32 vset_reg;
+	u32 ctrl_mask;
+	u32 vset_mask;
+	u32 vol_numb;
+	struct hi655x_regulator_ctrl_regs ctrl_reg;
+	struct hi655x_regulator_ctrl_regs *t_ctrl;
+
+	ret = of_property_read_u32_array(np, "regulator-ctrl-regs",
+					 (u32 *)&ctrl_reg, 0x3);
+	if (!ret) {
+		t_ctrl = &regulator->ctrl_regs;
+		t_ctrl->enable_reg = HI655X_BUS_ADDR(ctrl_reg.enable_reg);
+		t_ctrl->disable_reg = HI655X_BUS_ADDR(ctrl_reg.disable_reg);
+		t_ctrl->status_reg = HI655X_BUS_ADDR(ctrl_reg.status_reg);
+	} else
+		goto error;
+
+	ret = of_property_read_u32(np, "regulator-ctrl-mask", &ctrl_mask);
+	if (!ret)
+		regulator->ctrl_mask = ctrl_mask;
+	else
+		goto error;
+
+	regulator->rdesc.enable_reg = t_ctrl->enable_reg;
+	regulator->rdesc.enable_val = ctrl_mask;
+	regulator->rdesc.enable_mask = ctrl_mask;
+
+	ret = of_property_read_u32(np, "regulator-vset-regs", &vset_reg);
+	if (!ret)
+		regulator->vset_reg = HI655X_BUS_ADDR(vset_reg);
+	else
+		goto error;
+
+	ret = of_property_read_u32(np, "regulator-vset-mask", &vset_mask);
+	if (!ret)
+		regulator->vset_mask = vset_mask;
+	else
+		goto error;
+
+	ret = of_property_read_u32(np, "regulator-n-vol", &vol_numb);
+	if (!ret)
+		regulator->rdesc.n_voltages = vol_numb;
+	else
+		goto error;
+
+	vset_table = devm_kzalloc(dev, vol_numb * sizeof(int), GFP_KERNEL);
+	if (!vset_table)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(np, "regulator-vset-table",
+					 vset_table,
+					 vol_numb);
+
+	if (!ret)
+		regulator->rdesc.volt_table = vset_table;
+
+	return 0;
+error:
+	dev_err(dev, "get from dts node error!\n");
+	return -ENODEV;
+}
+
+static int hi655x_regulator_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct hi655x_regulator *regulator;
+	struct hi655x_pmic *pmic;
+	struct regulator_init_data *init_data;
+	struct regulator_config config = { };
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+
+	pmic = dev_get_drvdata(pdev->dev.parent);
+	if (!pmic) {
+		dev_err(dev, "no pmic in the regulator parent node\n");
+		return -ENODEV;
+	}
+
+	regulator = devm_kzalloc(dev, sizeof(*regulator), GFP_KERNEL);
+	if (!regulator)
+		return -ENOMEM;
+
+	regulator->rdesc.type = REGULATOR_VOLTAGE;
+	regulator->rdesc.owner = THIS_MODULE;
+	regulator->rdesc.ops = &hi655x_regulator_ops;
+	init_data = of_get_regulator_init_data(dev, dev->of_node,
+					       &regulator->rdesc);
+	if (!init_data)
+		return -EINVAL;
+
+	config.dev = &pdev->dev;
+	config.init_data = init_data;
+	config.driver_data = regulator;
+	config.regmap = pmic->regmap;
+	config.of_node = pdev->dev.of_node;
+	regulator->rdesc.name = init_data->constraints.name;
+
+	ret = of_get_hi655x_ctr(regulator, dev, np);
+	if (ret) {
+		dev_err(dev, "get param from dts error!\n");
+		return ret;
+	}
+
+	regulator->regdev = devm_regulator_register(dev,
+						    &regulator->rdesc,
+						    &config);
+	if (IS_ERR(regulator->regdev))
+		return PTR_ERR(regulator->regdev);
+
+	platform_set_drvdata(pdev, regulator);
+
+	return 0;
+}
+
+static struct platform_driver hi655x_regulator_driver = {
+	.driver = {
+		.name	= "hi655x_regulator",
+		.of_match_table = of_hi655x_regulator_match_tbl,
+	},
+	.probe	= hi655x_regulator_probe,
+};
+module_platform_driver(hi655x_regulator_driver);
+
+MODULE_AUTHOR("Chen Feng <puck.chen@hisilicon.com>");
+MODULE_DESCRIPTION("Hisi hi655x PMIC 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..8c76fc5
--- /dev/null
+++ b/include/linux/regulator/hi655x-regulator.h
@@ -0,0 +1,41 @@ 
+/*
+ * Head file for hi655x regulator
+ *
+ * Copyright (c) 2015 Hisilicon.
+ *
+ * Chen Feng <puck.chen@hisilicon.com>
+ * Fei  Wang <w.f@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __HISI_HI655X_REGULATOR_H__
+#define __HISI_HI655X_REGULATOR_H__
+
+struct hi655x_regulator_ctrl_regs {
+	u32  enable_reg;
+	u32  disable_reg;
+	u32  status_reg;
+};
+
+struct hi655x_regulator_ctrl_data {
+	int          shift;
+	unsigned int val;
+};
+
+struct hi655x_regulator_vset_data {
+	int          shift;
+	unsigned int mask;
+};
+
+struct hi655x_regulator {
+	struct hi655x_regulator_ctrl_regs ctrl_regs;
+	u32 vset_reg;
+	u32 ctrl_mask;
+	u32 vset_mask;
+	struct regulator_desc rdesc;
+	struct regulator_dev *regdev;
+};
+
+#endif