[PATCH 0/2] regulator: act8945a: add regulator driver for the sub-device of ACT8945A MFD

[PATCH 0/2] regulator: act8945a: add regulator driver for the sub-device of ACT8945A MFD

[Wenyou Yang]

The ACT8945A is a Multi Function Device with the following subdevices:
 - Regulator
 - Charger

This patch set is to add regulator driver for ACT8945A.
It is based on the patch set:
	[PATCH] mfd: act8945a: add Active-semi ACT8945A PMIC MFD driver


Wenyou Yang (2):
  regulator: act8945a: add regulator driver for ACT8945A
  regulator: add documentation for ACT8945A's regulator DT bindings

 .../bindings/regulator/act8945a-regulator.txt      |   78 +++++
 drivers/regulator/Kconfig                          |    7 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/act8945a-regulator.c             |  299 ++++++++++++++++++++
 4 files changed, 385 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
 create mode 100644 drivers/regulator/act8945a-regulator.c

-- 
1.7.9.5

[PATCH 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Wenyou Yang]

This patch adds new regulator driver to support ACT8945A MFD
chip's regulators.

The ACT8945A has three step-down DC/DC converters and four
low-dropout regulators.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

 drivers/regulator/Kconfig              |    7 +
 drivers/regulator/Makefile             |    1 +
 drivers/regulator/act8945a-regulator.c |  299 ++++++++++++++++++++++++++++++++
 3 files changed, 307 insertions(+)
 create mode 100644 drivers/regulator/act8945a-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 3904dc0..9a5c950 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -78,6 +78,13 @@ config REGULATOR_ACT8865
 	  This driver controls a active-semi act8865 voltage output
 	  regulator via I2C bus.
 
+config REGULATOR_ACT8945A
+	tristate "Active-semi ACT8945A voltage regulator"
+	depends on MFD_ACT8945A
+	help
+	  This driver controls a active-semi ACT8945A voltage regulator
+	  via I2C bus.
+
 config REGULATOR_AD5398
 	tristate "Analog Devices AD5398/AD5821 regulators"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 980b194..348cfd7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
 obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o
 obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500-ext.o ab8500.o
 obj-$(CONFIG_REGULATOR_ACT8865) += act8865-regulator.o
+obj-$(CONFIG_REGULATOR_ACT8945A) += act8945a-regulator.o
 obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
 obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 obj-$(CONFIG_REGULATOR_ARIZONA) += arizona-micsupp.o arizona-ldo1.o
diff --git a/drivers/regulator/act8945a-regulator.c b/drivers/regulator/act8945a-regulator.c
new file mode 100644
index 0000000..3912ea3
--- /dev/null
+++ b/drivers/regulator/act8945a-regulator.c
@@ -0,0 +1,299 @@
+/*
+ * Voltage regulation driver for active-semi ACT8945A PMICs
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *		      Wenyou Yang <wenyou.yang@atmel.com>
+ *
+ * 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 that 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.
+ */
+
+#include <linux/mfd/act8945a.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+/**
+ * ACT8945A Global Register Map.
+ */
+#define	ACT8945A_SYS_MODE	0x00
+#define	ACT8945A_SYS_CTRL	0x01
+#define	ACT8945A_DCDC1_VSET1	0x20
+#define	ACT8945A_DCDC1_VSET2	0x21
+#define	ACT8945A_DCDC1_CTRL	0x22
+#define	ACT8945A_DCDC2_VSET1	0x30
+#define	ACT8945A_DCDC2_VSET2	0x31
+#define	ACT8945A_DCDC2_CTRL	0x32
+#define	ACT8945A_DCDC3_VSET1	0x40
+#define	ACT8945A_DCDC3_VSET2	0x41
+#define	ACT8945A_DCDC3_CTRL	0x42
+#define	ACT8945A_LDO1_VSET	0x50
+#define	ACT8945A_LDO1_CTRL	0x51
+#define	ACT8945A_LDO2_VSET	0x54
+#define	ACT8945A_LDO2_CTRL	0x55
+#define	ACT8945A_LDO3_VSET	0x60
+#define	ACT8945A_LDO3_CTRL	0x61
+#define	ACT8945A_LDO4_VSET	0x64
+#define	ACT8945A_LDO4_CTRL	0x65
+
+/**
+ * Field Definitions.
+ */
+#define	ACT8945A_ENA		0x80	/* ON - [7] */
+#define	ACT8945A_VSEL_MASK	0x3F	/* VSET - [5:0] */
+
+/**
+ * ACT8945A voltage number
+ */
+#define	ACT8945A_VOLTAGE_NUM	64
+
+enum {
+	ACT8945A_ID_DCDC1,
+	ACT8945A_ID_DCDC2,
+	ACT8945A_ID_DCDC3,
+	ACT8945A_ID_LDO1,
+	ACT8945A_ID_LDO2,
+	ACT8945A_ID_LDO3,
+	ACT8945A_ID_LDO4,
+	ACT8945A_REG_NUM,
+};
+
+struct act8945a_regulator_init_data {
+	int id;
+	const char *name;
+	struct regulator_init_data *init_data;
+};
+
+struct act8945a_pmic_data {
+	int num_regulators;
+	struct act8945a_regulator_init_data *regulators;
+};
+
+struct act8945a_pmic {
+	struct act8945a_dev *mfd;
+};
+
+static const struct regulator_linear_range act8945a_voltage_ranges[] = {
+	REGULATOR_LINEAR_RANGE(600000, 0, 23, 25000),
+	REGULATOR_LINEAR_RANGE(1200000, 24, 47, 50000),
+	REGULATOR_LINEAR_RANGE(2400000, 48, 63, 100000),
+};
+
+static struct regulator_ops act8945a_ops = {
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+#define ACT89xx_REG(_name, _family, _id, _vsel_reg, _supply)		\
+	[_family##_ID_##_id] = {					\
+		.name			= _name,			\
+		.supply_name		= _supply,			\
+		.id			= _family##_ID_##_id,		\
+		.type			= REGULATOR_VOLTAGE,		\
+		.ops			= &act8945a_ops,			\
+		.n_voltages		= ACT8945A_VOLTAGE_NUM,		\
+		.linear_ranges		= act8945a_voltage_ranges,	\
+		.n_linear_ranges	= ARRAY_SIZE(act8945a_voltage_ranges), \
+		.vsel_reg		= _family##_##_id##_##_vsel_reg, \
+		.vsel_mask		= ACT8945A_VSEL_MASK,		\
+		.enable_reg		= _family##_##_id##_CTRL,	\
+		.enable_mask		= ACT8945A_ENA,			\
+		.owner			= THIS_MODULE,			\
+	}
+
+static const struct regulator_desc act8945a_regulators[] = {
+	ACT89xx_REG("DCDC_REG1", ACT8945A, DCDC1, VSET1, "vp1"),
+	ACT89xx_REG("DCDC_REG2", ACT8945A, DCDC2, VSET1, "vp2"),
+	ACT89xx_REG("DCDC_REG3", ACT8945A, DCDC3, VSET1, "vp3"),
+	ACT89xx_REG("LDO_REG1", ACT8945A, LDO1, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG2", ACT8945A, LDO2, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG3", ACT8945A, LDO3, VSET, "inl67"),
+	ACT89xx_REG("LDO_REG4", ACT8945A, LDO4, VSET, "inl67"),
+};
+
+static const struct regulator_desc act8945a_alt_regulators[] = {
+	ACT89xx_REG("DCDC_REG1", ACT8945A, DCDC1, VSET2, "vp1"),
+	ACT89xx_REG("DCDC_REG2", ACT8945A, DCDC2, VSET2, "vp2"),
+	ACT89xx_REG("DCDC_REG3", ACT8945A, DCDC3, VSET2, "vp3"),
+	ACT89xx_REG("LDO_REG1", ACT8945A, LDO1, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG2", ACT8945A, LDO2, VSET, "inl45"),
+	ACT89xx_REG("LDO_REG3", ACT8945A, LDO3, VSET, "inl67"),
+	ACT89xx_REG("LDO_REG4", ACT8945A, LDO4, VSET, "inl67"),
+};
+
+static struct of_regulator_match act8945a_matches[] = {
+	[ACT8945A_ID_DCDC1]	= { .name = "DCDC_REG1"},
+	[ACT8945A_ID_DCDC2]	= { .name = "DCDC_REG2"},
+	[ACT8945A_ID_DCDC3]	= { .name = "DCDC_REG3"},
+	[ACT8945A_ID_LDO1]	= { .name = "LDO_REG1"},
+	[ACT8945A_ID_LDO2]	= { .name = "LDO_REG2"},
+	[ACT8945A_ID_LDO3]	= { .name = "LDO_REG3"},
+	[ACT8945A_ID_LDO4]	= { .name = "LDO_REG4"},
+};
+
+static int act8945a_parse_dt_reg_data(struct platform_device *pdev,
+			struct act8945a_pmic_data *pdata,
+			struct of_regulator_match **act8945a_reg_matches)
+{
+	struct device_node *np;
+	struct of_regulator_match *matches;
+	struct act8945a_regulator_init_data *regulator;
+	unsigned int i, num_matches;
+	int ret;
+
+	np = of_get_child_by_name(pdev->dev.of_node, "regulators");
+	if (!np) {
+		dev_err(&pdev->dev, "regulator node not found\n");
+		return -EINVAL;
+	}
+
+	matches = act8945a_matches;
+	num_matches = ARRAY_SIZE(act8945a_matches);
+
+	ret = of_regulator_match(&pdev->dev, np, matches, num_matches);
+	of_node_put(np);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Error parsing regulator init data: %d\n",
+			ret);
+		return -EINVAL;
+	}
+
+	*act8945a_reg_matches = matches;
+
+	pdata->regulators = devm_kzalloc(&pdev->dev,
+		sizeof(struct act8945a_regulator_init_data) * num_matches,
+		GFP_KERNEL);
+	if (!pdata->regulators)
+		return -ENOMEM;
+
+	pdata->num_regulators = num_matches;
+	regulator = pdata->regulators;
+
+	for (i = 0; i < num_matches; i++) {
+		regulator->id = i;
+		regulator->name = matches[i].name;
+		regulator->init_data = matches[i].init_data;
+		regulator++;
+	}
+
+	return 0;
+}
+
+static struct regulator_init_data *act8945a_get_init_data(int id,
+				struct act8945a_pmic_data *pdata)
+{
+	int i;
+
+	if (!pdata)
+		return NULL;
+
+	for (i = 0; i < pdata->num_regulators; i++) {
+		if (pdata->regulators[i].id == id)
+			return pdata->regulators[i].init_data;
+	}
+
+	return NULL;
+}
+
+static int act8945a_pmic_probe(struct platform_device *pdev)
+{
+	struct act8945a_dev *act8945a_dev = dev_get_drvdata(pdev->dev.parent);
+	struct act8945a_pmic *act8945a_pmic;
+	struct of_regulator_match *act8945a_reg_matches = NULL;
+	struct regulator_config config = { };
+	const struct regulator_desc *regulators, *desc;
+	struct regulator_dev *rdev;
+	struct act8945a_pmic_data pmic_data, *pdata;
+	int i, num_regulators;
+	int voltage_select = 0;
+	int ret;
+
+	act8945a_pmic = devm_kzalloc(&pdev->dev,
+				     sizeof(*act8945a_pmic), GFP_KERNEL);
+	if (!act8945a_pmic)
+		return -ENOMEM;
+
+	act8945a_pmic->mfd = act8945a_dev;
+
+	ret = act8945a_parse_dt_reg_data(pdev, &pmic_data,
+					 &act8945a_reg_matches);
+	if (ret < 0)
+		return ret;
+
+	pdata = &pmic_data;
+
+	voltage_select = !!of_get_property(pdev->dev.of_node,
+					   "active-semi,vsel-high", NULL);
+
+	if (voltage_select) {
+		regulators = act8945a_alt_regulators;
+		num_regulators = ARRAY_SIZE(act8945a_alt_regulators);
+	} else {
+		regulators = act8945a_regulators;
+		num_regulators = ARRAY_SIZE(act8945a_regulators);
+	}
+
+	for (i = 0; i < num_regulators; i++) {
+		desc = &regulators[i];
+
+		config.dev = act8945a_dev->dev;
+		config.init_data = act8945a_get_init_data(desc->id, pdata);
+		config.driver_data = act8945a_pmic;
+		config.regmap = act8945a_dev->regmap;
+		if (act8945a_reg_matches)
+			config.of_node = act8945a_reg_matches[i].of_node;
+
+		rdev = devm_regulator_register(&pdev->dev, desc, &config);
+		if (IS_ERR(rdev)) {
+			dev_err(act8945a_dev->dev,
+				"failed to register %s regulator\n",
+				desc->name);
+			return PTR_ERR(rdev);
+		}
+
+		dev_info(act8945a_dev->dev, "%s regulator registered\n",
+			desc->name);
+	}
+
+	platform_set_drvdata(pdev, act8945a_dev);
+
+	return 0;
+}
+
+static struct platform_driver act8945a_pmic_driver = {
+	.driver = {
+		.name = "act8945a-pmic",
+	},
+	.probe = act8945a_pmic_probe,
+};
+
+static int __init act8945a_pmic_init(void)
+{
+	return platform_driver_register(&act8945a_pmic_driver);
+}
+subsys_initcall(act8945a_pmic_init);
+
+static void __exit act8945a_pmic_cleanup(void)
+{
+	platform_driver_unregister(&act8945a_pmic_driver);
+}
+module_exit(act8945a_pmic_cleanup);
+
+MODULE_DESCRIPTION("Active-semi ACT8945A voltage regulator driver");
+MODULE_AUTHOR("Wenyou Yang <wenyou.yang@atmel.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH 2/2] regulator: add documentation for ACT8945A's regulator DT bindings

[Wenyou Yang]

This patch adds documentation for the DT bindings of the regulator
subdevice of ACT8945A MFD.

Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
---

 .../bindings/regulator/act8945a-regulator.txt      |   78 ++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/act8945a-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt b/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
new file mode 100644
index 0000000..f67ff86
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
@@ -0,0 +1,78 @@
+Device-Tree bindings for regulators of Active-semi ACT8945A Multi-Function Device
+
+Required properties:
+ - compatible: "active-semi,act8945a-regulator"
+
+Optional properties:
+- active-semi,vsel-high: Indicates if the VSEL pin is set to logic-high.
+  If this property is missing, assume the VSEL pin is set to logic-low.
+
+Optional input supply properties:
+  - vp1-supply: The input supply for DCDC_REG1
+  - vp2-supply: The input supply for DCDC_REG2
+  - vp3-supply: The input supply for DCDC_REG3
+  - inl45-supply: The input supply for LDO_REG1 and LDO_REG2
+  - inl67-supply: The input supply for LDO_REG3 and LDO_REG4
+
+Any standard regulator properties can be used to configure the single regulator.
+
+The valid names for regulators are:
+	DCDC_REG1, DCDC_REG2, DCDC_REG3, LDO_REG1, LDO_REG2, LDO_REG3, LDO_REG4.
+
+Example:
+
+	pmic {
+		compatible = "active-semi,act8945a-regulator";
+		active-semi,vsel-high;
+
+		regulators {
+			vdd_1v35_reg: DCDC_REG1 {
+				regulator-name = "VDD_1V35";
+				regulator-min-microvolt = <1350000>;
+				regulator-max-microvolt = <1350000>;
+				regulator-always-on;
+			};
+
+			vdd_1v2_reg: DCDC_REG2 {
+				regulator-name = "VDD_1V2";
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-always-on;
+			};
+
+			vdd_3v3_reg: DCDC_REG3 {
+				regulator-name = "VDD_3V3";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vdd_fuse_reg: LDO_REG1 {
+				regulator-name = "VDD_FUSE";
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-always-on;
+			};
+
+			vdd_3v3_lp_reg: LDO_REG2 {
+				regulator-name = "VDD_3V3_LP";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vdd_led_reg: LDO_REG3 {
+				regulator-name = "VDD_LED";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vdd_sdhc_1v8_reg: LDO_REG4 {
+				regulator-name = "VDD_SDHC_1V8";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+			};
+		};
+	};
-- 
1.7.9.5

Re: [PATCH 2/2] regulator: add documentation for ACT8945A's regulator DT bindings

[Rob Herring]

On Fri, Jan 08, 2016 at 10:08:58AM +0800, Wenyou Yang wrote:
> This patch adds documentation for the DT bindings of the regulator
> subdevice of ACT8945A MFD.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> 
>  .../bindings/regulator/act8945a-regulator.txt      |   78 ++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/act8945a-regulator.txt

Acked-by: Rob Herring <robh@kernel.org>

RE: [PATCH 2/2] regulator: add documentation for ACT8945A's regulator DT bindings

[Yang, Wenyou]

Hi Rob,

Thank you for your acked.

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 2016年1月9日 23:36
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>;
> Pawel Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 2/2] regulator: add documentation for ACT8945A's regulator
> DT bindings
> 
> On Fri, Jan 08, 2016 at 10:08:58AM +0800, Wenyou Yang wrote:
> > This patch adds documentation for the DT bindings of the regulator
> > subdevice of ACT8945A MFD.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > ---
> >
> >  .../bindings/regulator/act8945a-regulator.txt      |   78 ++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/regulator/act8945a-regulator.txt
> 
> Acked-by: Rob Herring <robh@kernel.org>


Best Regards,
Wenyou Yang

Re: [PATCH 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Mark Brown]

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

On Fri, Jan 08, 2016 at 10:08:57AM +0800, Wenyou Yang wrote:

> +	np = of_get_child_by_name(pdev->dev.of_node, "regulators");
> +	if (!np) {
> +		dev_err(&pdev->dev, "regulator node not found\n");
> +		return -EINVAL;
> +	}
> +
> +	matches = act8945a_matches;
> +	num_matches = ARRAY_SIZE(act8945a_matches);
> +
> +	ret = of_regulator_match(&pdev->dev, np, matches, num_matches);

Don't open code this, use the core regulators_node and of_match
implementation instead.

> +		dev_info(act8945a_dev->dev, "%s regulator registered\n",
> +			desc->name);

This is just noise, please remove it - let the core handle notifying
users.

> +static int __init act8945a_pmic_init(void)
> +{
> +	return platform_driver_register(&act8945a_pmic_driver);
> +}
> +subsys_initcall(act8945a_pmic_init);

Please use module_platform_driver() unless there is a strong reason to
do otherwise.

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

Re: [PATCH 2/2] regulator: add documentation for ACT8945A's regulator DT bindings

[Mark Brown]

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

On Fri, Jan 08, 2016 at 10:08:58AM +0800, Wenyou Yang wrote:

> +Optional properties:
> +- active-semi,vsel-high: Indicates if the VSEL pin is set to logic-high.
> +  If this property is missing, assume the VSEL pin is set to logic-low.

Wouldn't such a pin normally be connected to a GPIO for runtime control
- why is it expected to be tied statically all the time?

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

RE: [PATCH 2/2] regulator: add documentation for ACT8945A's regulator DT bindings

[Yang, Wenyou]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 2016年1月16日 1:59
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>;
> Pawel Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 2/2] regulator: add documentation for ACT8945A's regulator
> DT bindings
> 
> On Fri, Jan 08, 2016 at 10:08:58AM +0800, Wenyou Yang wrote:
> 
> > +Optional properties:
> > +- active-semi,vsel-high: Indicates if the VSEL pin is set to logic-high.
> > +  If this property is missing, assume the VSEL pin is set to logic-low.
> 
> Wouldn't such a pin normally be connected to a GPIO for runtime control
> - why is it expected to be tied statically all the time?

On the reference board, it is connected to the VSYS_5V statically.

Yes, it should be connected to a GPIO, it should be controllable at the necessary time.


Best Regards,
Wenyou Yang 

RE: [PATCH 1/2] regulator: act8945a: add regulator driver for ACT8945A

[Yang, Wenyou]

Hi Mark,

Thank you for your review.

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 2016年1月16日 1:58
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>;
> Pawel Moll <pawel.moll@arm.com>; Mark Rutland <mark.rutland@arm.com>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> Ferre, Nicolas <Nicolas.FERRE@atmel.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 1/2] regulator: act8945a: add regulator driver for ACT8945A
> 
> On Fri, Jan 08, 2016 at 10:08:57AM +0800, Wenyou Yang wrote:
> 
> > +	np = of_get_child_by_name(pdev->dev.of_node, "regulators");
> > +	if (!np) {
> > +		dev_err(&pdev->dev, "regulator node not found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	matches = act8945a_matches;
> > +	num_matches = ARRAY_SIZE(act8945a_matches);
> > +
> > +	ret = of_regulator_match(&pdev->dev, np, matches, num_matches);
> 
> Don't open code this, use the core regulators_node and of_match implementation
> instead.

Good. In the next version, I use the code implementation, and remove the redundant code.

Thank you for your advice.

> 
> > +		dev_info(act8945a_dev->dev, "%s regulator registered\n",
> > +			desc->name);
> 
> This is just noise, please remove it - let the core handle notifying users.

It is fixed in version 2.

> 
> > +static int __init act8945a_pmic_init(void) {
> > +	return platform_driver_register(&act8945a_pmic_driver);
> > +}
> > +subsys_initcall(act8945a_pmic_init);
> 
> Please use module_platform_driver() unless there is a strong reason to do
> otherwise.

It is fixed in version 2 too.


Best Regards,
Wenyou Yang