[PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device

[PATCH 0/4] Refactor Qualcomm RPM regulator to single platform_device

[Bjorn Andersson]

Stephen Boyd pointed out that the current design of the Qualcomm RPM and
regulator driver consumes 12-20kB of ram just for the platform_device structs.

This series starts with a new revision of the dt binding documentation for the
rpm regulators, it then extend the of_parse_cb callback to support changing the
constraints flags during register of a regulator, refactor the
qcom_rpm-regulator driver to move all custom parse code to a function suitable
for usage as of_parse_cb. The final patch defines the tables of registers and
change the probe function to register the appropriate regulators based on pmic.

As Stephen pointed out in his PATCH/RFC/argument [1], this gives a more
accurate representation of input supplies, as they are now named as in the
specification.

Note that for platforms with multiple pmics (e.g. 8660 and 8974) will have
multiple regulator subnodes to the rpm node - something that will be clearer
with this binding than the previously suggested.

[1] https://lkml.org/lkml/2015/2/26/713

Bjorn Andersson (4):
  mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes
  regulator: core: Expose init_data to of_parse_cb
  regulator: qcom: Refactor of-parsing code
  regulator: qcom: Rework to single platform device

 Documentation/devicetree/bindings/mfd/qcom-rpm.txt | 201 +++++++++++++-
 drivers/regulator/max77686.c                       |   3 +-
 drivers/regulator/of_regulator.c                   |   2 +-
 drivers/regulator/qcom_rpm-regulator.c             | 290 ++++++++++++++-------
 include/linux/regulator/driver.h                   |   3 +-
 5 files changed, 389 insertions(+), 110 deletions(-)

-- 
1.8.2.2

[PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Bjorn Andersson]

Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

As the previously accepted binding didn't cover any functionality but
registering the mfd core there is no users, hence it's fine to drop the
two required fields at this time.

 Documentation/devicetree/bindings/mfd/qcom-rpm.txt | 201 +++++++++++++++++++--
 1 file changed, 189 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
index 85e3198..92bcd19 100644
--- a/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom-rpm.txt
@@ -31,16 +31,6 @@ frequencies.
 	Value type: <string-array>
 	Definition: must be the three strings "ack", "err" and "wakeup", in order
 
-- #address-cells:
-	Usage: required
-	Value type: <u32>
-	Definition: must be 1
-
-- #size-cells:
-	Usage: required
-	Value type: <u32>
-	Definition: must be 0
-
 - qcom,ipc:
 	Usage: required
 	Value type: <prop-encoded-array>
@@ -52,6 +42,172 @@ frequencies.
 		    - u32 representing the ipc bit within the register
 
 
+= SUBNODES
+
+The RPM exposes resources to its subnodes. The below bindings specify the set
+of valid subnodes that can operate on these resources.
+
+== Regulators
+
+Regulator notes are identified by their compatible:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		    "qcom,rpm-pm8058-regulators"
+		    "qcom,rpm-pm8901-regulators"
+		    "qcom,rpm-pm8921-regulators"
+
+- vdd_l0_l1_lvs-supply:
+- vdd_l10-supply:
+- vdd_l13_l16-supply:
+- vdd_l14_l15-supply:
+- vdd_l17_l18-supply:
+- vdd_l19_l20-supply:
+- vdd_l21-supply:
+- vdd_l22-supply:
+- vdd_l23_l24_l25-supply:
+- vdd_l2_l11_l12-supply:
+- vdd_l3_l4_l5-supply:
+- vdd_l6_l7-supply:
+- vdd_l8-supply:
+- vdd_l9-supply:
+- vdd_ncp-supply:
+- vdd_s0-supply:
+- vdd_s1-supply:
+- vdd_s2-supply:
+- vdd_s3-supply:
+- vdd_s4-supply:
+	Usage: optional (pm8058 only)
+	Value type: <phandle>
+	Definition: reference to regulator supplying the input pin, as
+		    described in the data sheet
+
+- vdd_l0-supply:
+- vdd_l1-supply:
+- vdd_l2-supply:
+- vdd_l3-supply:
+- vdd_l4-supply:
+- vdd_l5-supply:
+- vdd_l6-supply:
+- vdd_s0-supply:
+- vdd_s1-supply:
+- vdd_s2-supply:
+- vdd_s3-supply:
+- vdd_s4-supply:
+- lvs0_in-supply:
+- lvs1_in-supply:
+- lvs2_in-supply:
+- lvs3_in-supply:
+- mvs_in-supply:
+	Usage: optional (pm8901 only)
+	Value type: <phandle>
+	Definition: reference to regulator supplying the input pin, as
+		    described in the data sheet
+
+- vin_l1_l2_l12_l18-supply:
+- vin_l24-supply:
+- vin_l25-supply:
+- vin_l27-supply:
+- vin_l28-supply:
+- vin_lvs_1_3_6-supply:
+- vin_lvs2-supply:
+- vin_lvs_4_5_7-supply:
+- vin_ncp-supply:
+	Usage: optional (pm8921 only)
+	Value type: <phandle>
+	Definition: reference to regulator supplying the input pin, as
+		    described in the data sheet
+
+The regulator node houses sub-nodes for each regulator within the device. Each
+sub-node is identified using the node's name, with valid values listed for each
+of the pmics below.
+
+pm8058:
+	l0, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11, l12, l13, l14, l15,
+	l16, l17, l18, l19, l20, l21, l22, l23, l24, l25, s0, s1, s2, s3, s4,
+	lvs0, lvs1, ncp
+
+pm8901:
+	l0, l1, l2, l3, l4, l5, l6, s0, s1, s2, s3, s4, lvs0, lvs1, lvs2, lvs3,
+	mvs
+
+pm8921:
+	s1, s2, s3, s4, s7, s8, l1, l2, l3, l4, l5, l6, l7, l8, l9, l10, l11,
+	l12, l14, l15, l16, l17, l18, l21, l22, l23, l24, l25, l26, l27, l28,
+	l29, lvs1, lvs2, lvs3, lvs4, lvs5, lvs6, lvs7, usb-switch, hdmi-switch,
+	ncp
+
+The content of each sub-node is defined by the standard binding for regulators -
+see regulator.txt - with additional custom properties described below:
+
+=== Switch-mode Power Supply regulator custom properties
+
+- bias-pull-down:
+	Usage: optional
+	Value type: <empty>
+	Definition: enable pull down of the regulator when inactive
+
+- qcom,switch-mode-frequency:
+	Usage: required
+	Value type: <u32>
+	Definition: Frequency (Hz) of the switch-mode power supply;
+		    must be one of:
+		    19200000, 9600000, 6400000, 4800000, 3840000, 3200000,
+		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
+		    1480000, 1370000, 1280000, 1200000
+
+- qcom,force-mode:
+	Usage: optional (default if no other qcom,force-mode is specified)
+	Value type: <u32>
+	Defintion: indicates that the regulator should be forced to a
+		   particular mode, valid values are:
+		   QCOM_RPM_FORCE_MODE_NONE - do not force any mode
+		   QCOM_RPM_FORCE_MODE_LPM - force into low power mode
+		   QCOM_RPM_FORCE_MODE_HPM - force into high power mode
+		   QCOM_RPM_FORCE_MODE_AUTO - allow regulator to automatically
+					      select its own mode based on
+					      realtime current draw, only for:
+					      pm8921 smps and ftsmps
+
+- qcom,power-mode-hysteretic:
+	Usage: optional
+	Value type: <empty>
+	Definition: select that the power supply should operate in hysteretic
+		    mode, instead of the default pwm mode
+
+=== Low-dropout regulator custom properties
+
+- bias-pull-down:
+	Usage: optional
+	Value type: <empty>
+	Definition: enable pull down of the regulator when inactive
+
+- qcom,force-mode:
+	Usage: optional
+	Value type: <u32>
+	Defintion: indicates that the regulator should not be forced to any
+		   particular mode, valid values are:
+		   QCOM_RPM_FORCE_MODE_NONE - do not force any mode
+		   QCOM_RPM_FORCE_MODE_LPM - force into low power mode
+		   QCOM_RPM_FORCE_MODE_HPM - force into high power mode
+		   QCOM_RPM_FORCE_MODE_BYPASS - set regulator to use bypass
+						mode, i.e.  to act as a switch
+						and not regulate, only for:
+						pm8921 pldo, nldo and nldo1200
+
+=== Negative Charge Pump custom properties
+
+- qcom,switch-mode-frequency:
+	Usage: required
+	Value type: <u32>
+	Definition: Frequency (Hz) of the swith mode power supply;
+		    must be one of:
+		    19200000, 9600000, 6400000, 4800000, 3840000, 3200000,
+		    2740000, 2400000, 2130000, 1920000, 1750000, 1600000,
+		    1480000, 1370000, 1280000, 1200000
+
 = EXAMPLE
 
 	#include <dt-bindings/mfd/qcom-rpm.h>
@@ -64,7 +220,28 @@ frequencies.
 		interrupts = <0 19 0>, <0 21 0>, <0 22 0>;
 		interrupt-names = "ack", "err", "wakeup";
 
-		#address-cells = <1>;
-		#size-cells = <0>;
+		regulators {
+			compatible = "qcom,rpm-pm8921-regulators";
+			vin_l1_l2_l12_l18-supply = <&pm8921_s4>;
+
+			s1 {
+				regulator-min-microvolt = <1225000>;
+				regulator-max-microvolt = <1225000>;
+
+				bias-pull-down;
+
+				qcom,switch-mode-frequency = <3200000>;
+			};
+
+			pm8921_s4: s4 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+
+				qcom,switch-mode-frequency = <1600000>;
+				bias-pull-down;
+
+				qcom,force-mode = <QCOM_RPM_FORCE_MODE_AUTO>;
+			};
+		};
 	};
 
-- 
1.8.2.2

[PATCH 2/4] regulator: core: Expose init_data to of_parse_cb

[Bjorn Andersson]

Expose the newly created init_data to the driver's parse callback so
that it can futher enhance it with e.g. constraints of the regulator.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

This is needed because calling regulator_register() with a of_node and
of_match will overwrite the passed config->init_data. An alternative way
would be to merge the parsed init_data with the driver provided one.

 drivers/regulator/max77686.c     | 3 ++-
 drivers/regulator/of_regulator.c | 2 +-
 include/linux/regulator/driver.h | 3 ++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 15fb141..87cbef2 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -259,7 +259,8 @@ static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 
 static int max77686_of_parse_cb(struct device_node *np,
 		const struct regulator_desc *desc,
-		struct regulator_config *config)
+		struct regulator_config *config,
+		struct regulator_init_data *init_data)
 {
 	struct max77686_data *max77686 = config->driver_data;
 
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 24e812c..f65934f 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -309,7 +309,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 		}
 
 		if (desc->of_parse_cb) {
-			if (desc->of_parse_cb(child, desc, config)) {
+			if (desc->of_parse_cb(child, desc, config, init_data)) {
 				dev_err(dev,
 					"driver callback failed to parse DT for regulator %s\n",
 					child->name);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 8a0165f..0f86a182 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -266,7 +266,8 @@ struct regulator_desc {
 	const char *regulators_node;
 	int (*of_parse_cb)(struct device_node *,
 			    const struct regulator_desc *,
-			    struct regulator_config *);
+			    struct regulator_config *,
+			    struct regulator_init_data *);
 	int id;
 	bool continuous_voltage_range;
 	unsigned n_voltages;
-- 
1.8.2.2

[PATCH 3/4] regulator: qcom: Refactor of-parsing code

[Bjorn Andersson]

Refactor out all custom property parsing code from the probe function
into a function suitable for regulator_desc->of_parse_cb usage.

Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---

This simply moves the parsing code out of probe(), no functional change.

 drivers/regulator/qcom_rpm-regulator.c | 147 +++++++++++++++++++--------------
 1 file changed, 85 insertions(+), 62 deletions(-)

diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index 15e07c2..1ec4b98 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -645,7 +645,9 @@ static int rpm_reg_set(struct qcom_rpm_reg *vreg,
 	return 0;
 }
 
-static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
+static int rpm_reg_of_parse_freq(struct device *dev,
+				 struct device_node *node,
+				 struct qcom_rpm_reg *vreg)
 {
 	static const int freq_table[] = {
 		19200000, 9600000, 6400000, 4800000, 3840000, 3200000, 2740000,
@@ -659,7 +661,7 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
 	int i;
 
 	key = "qcom,switch-mode-frequency";
-	ret = of_property_read_u32(dev->of_node, key, &freq);
+	ret = of_property_read_u32(node, key, &freq);
 	if (ret) {
 		dev_err(dev, "regulator requires %s property\n", key);
 		return -EINVAL;
@@ -676,88 +678,45 @@ static int rpm_reg_of_parse_freq(struct device *dev, struct qcom_rpm_reg *vreg)
 	return -EINVAL;
 }
 
-static int rpm_reg_probe(struct platform_device *pdev)
+static int rpm_reg_of_parse(struct device_node *node,
+			    const struct regulator_desc *desc,
+			    struct regulator_config *config,
+			    struct regulator_init_data *init_data)
 {
-	struct regulator_init_data *initdata;
-	const struct qcom_rpm_reg *template;
-	const struct of_device_id *match;
-	struct regulator_config config = { };
-	struct regulator_dev *rdev;
-	struct qcom_rpm_reg *vreg;
+	struct qcom_rpm_reg *vreg = config->driver_data;
+	struct device *dev = config->dev;
 	const char *key;
 	u32 force_mode;
 	bool pwm;
-	u32 val;
 	int ret;
-
-	match = of_match_device(rpm_of_match, &pdev->dev);
-	template = match->data;
-
-	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
-	if (!vreg) {
-		dev_err(&pdev->dev, "failed to allocate vreg\n");
-		return -ENOMEM;
-	}
-	memcpy(vreg, template, sizeof(*vreg));
-	mutex_init(&vreg->lock);
-	vreg->dev = &pdev->dev;
-	vreg->desc.id = -1;
-	vreg->desc.owner = THIS_MODULE;
-	vreg->desc.type = REGULATOR_VOLTAGE;
-	vreg->desc.name = pdev->dev.of_node->name;
-	vreg->desc.supply_name = "vin";
-
-	vreg->rpm = dev_get_drvdata(pdev->dev.parent);
-	if (!vreg->rpm) {
-		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
-		return -ENODEV;
-	}
-
-	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
-					      &vreg->desc);
-	if (!initdata)
-		return -EINVAL;
-
-	key = "reg";
-	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to read %s\n", key);
-		return ret;
-	}
-	vreg->resource = val;
-
-	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
-	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
-		dev_err(&pdev->dev, "no voltage specified for regulator\n");
-		return -EINVAL;
-	}
+	u32 val;
 
 	/* Regulators with ia property suppports drms */
 	if (vreg->parts->ia.mask)
-		initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
+		init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
 
 	key = "bias-pull-down";
-	if (of_property_read_bool(pdev->dev.of_node, key)) {
+	if (of_property_read_bool(node, key)) {
 		ret = rpm_reg_set(vreg, &vreg->parts->pd, 1);
 		if (ret) {
-			dev_err(&pdev->dev, "%s is invalid", key);
+			dev_err(dev, "%s is invalid", key);
 			return ret;
 		}
 	}
 
 	if (vreg->parts->freq.mask) {
-		ret = rpm_reg_of_parse_freq(&pdev->dev, vreg);
+		ret = rpm_reg_of_parse_freq(dev, node, vreg);
 		if (ret < 0)
 			return ret;
 	}
 
 	if (vreg->parts->pm.mask) {
 		key = "qcom,power-mode-hysteretic";
-		pwm = !of_property_read_bool(pdev->dev.of_node, key);
+		pwm = !of_property_read_bool(node, key);
 
 		ret = rpm_reg_set(vreg, &vreg->parts->pm, pwm);
 		if (ret) {
-			dev_err(&pdev->dev, "failed to set power mode\n");
+			dev_err(dev, "failed to set power mode\n");
 			return ret;
 		}
 	}
@@ -766,11 +725,11 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		force_mode = -1;
 
 		key = "qcom,force-mode";
-		ret = of_property_read_u32(pdev->dev.of_node, key, &val);
+		ret = of_property_read_u32(node, key, &val);
 		if (ret == -EINVAL) {
 			val = QCOM_RPM_FORCE_MODE_NONE;
 		} else if (ret < 0) {
-			dev_err(&pdev->dev, "failed to read %s\n", key);
+			dev_err(dev, "failed to read %s\n", key);
 			return ret;
 		}
 
@@ -805,21 +764,85 @@ static int rpm_reg_probe(struct platform_device *pdev)
 		}
 
 		if (force_mode == -1) {
-			dev_err(&pdev->dev, "invalid force mode\n");
+			dev_err(dev, "invalid force mode\n");
 			return -EINVAL;
 		}
 
 		ret = rpm_reg_set(vreg, &vreg->parts->fm, force_mode);
 		if (ret) {
-			dev_err(&pdev->dev, "failed to set force mode\n");
+			dev_err(dev, "failed to set force mode\n");
 			return ret;
 		}
 	}
 
+	return 0;
+}
+
+static int rpm_reg_probe(struct platform_device *pdev)
+{
+	struct regulator_init_data *initdata;
+	const struct qcom_rpm_reg *template;
+	const struct of_device_id *match;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	struct qcom_rpm_reg *vreg;
+	const char *key;
+	u32 val;
+	int ret;
+
+	match = of_match_device(rpm_of_match, &pdev->dev);
+	template = match->data;
+
+	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
+	if (!vreg) {
+		dev_err(&pdev->dev, "failed to allocate vreg\n");
+		return -ENOMEM;
+	}
+	memcpy(vreg, template, sizeof(*vreg));
+	mutex_init(&vreg->lock);
+	vreg->dev = &pdev->dev;
+	vreg->desc.id = -1;
+	vreg->desc.owner = THIS_MODULE;
+	vreg->desc.type = REGULATOR_VOLTAGE;
+	vreg->desc.name = pdev->dev.of_node->name;
+	vreg->desc.supply_name = "vin";
+
+	vreg->rpm = dev_get_drvdata(pdev->dev.parent);
+	if (!vreg->rpm) {
+		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
+		return -ENODEV;
+	}
+
+	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
+					      &vreg->desc);
+	if (!initdata)
+		return -EINVAL;
+
+	key = "reg";
+	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to read %s\n", key);
+		return ret;
+	}
+	vreg->resource = val;
+
+	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
+	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
+		dev_err(&pdev->dev, "no voltage specified for regulator\n");
+		return -EINVAL;
+	}
+
+
 	config.dev = &pdev->dev;
 	config.init_data = initdata;
 	config.driver_data = vreg;
 	config.of_node = pdev->dev.of_node;
+
+	ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc,
+			       &config, initdata);
+	if (ret)
+		return ret;
+
 	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
 	if (IS_ERR(rdev)) {
 		dev_err(&pdev->dev, "can't register regulator\n");
-- 
1.8.2.2

[PATCH 4/4] regulator: qcom: Rework to single platform device

[Bjorn Andersson]

Modeling the individual RPM resources as platform devices consumes at
least 12-15kb of RAM, just to hold the platform_device structs. Rework
this to instead have one device per pmic exposed by the RPM.

With this representation we can more accurately define the input pins on
the pmic and have the supply description match the data sheet.

Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
---
 drivers/regulator/qcom_rpm-regulator.c | 245 ++++++++++++++++++++++-----------
 1 file changed, 161 insertions(+), 84 deletions(-)

diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c
index 1ec4b98..5cdd568 100644
--- a/drivers/regulator/qcom_rpm-regulator.c
+++ b/drivers/regulator/qcom_rpm-regulator.c
@@ -607,31 +607,6 @@ static const struct qcom_rpm_reg smb208_smps = {
 	.supports_force_mode_bypass = false,
 };
 
-static const struct of_device_id rpm_of_match[] = {
-	{ .compatible = "qcom,rpm-pm8058-pldo",     .data = &pm8058_pldo },
-	{ .compatible = "qcom,rpm-pm8058-nldo",     .data = &pm8058_nldo },
-	{ .compatible = "qcom,rpm-pm8058-smps",     .data = &pm8058_smps },
-	{ .compatible = "qcom,rpm-pm8058-ncp",      .data = &pm8058_ncp },
-	{ .compatible = "qcom,rpm-pm8058-switch",   .data = &pm8058_switch },
-
-	{ .compatible = "qcom,rpm-pm8901-pldo",     .data = &pm8901_pldo },
-	{ .compatible = "qcom,rpm-pm8901-nldo",     .data = &pm8901_nldo },
-	{ .compatible = "qcom,rpm-pm8901-ftsmps",   .data = &pm8901_ftsmps },
-	{ .compatible = "qcom,rpm-pm8901-switch",   .data = &pm8901_switch },
-
-	{ .compatible = "qcom,rpm-pm8921-pldo",     .data = &pm8921_pldo },
-	{ .compatible = "qcom,rpm-pm8921-nldo",     .data = &pm8921_nldo },
-	{ .compatible = "qcom,rpm-pm8921-nldo1200", .data = &pm8921_nldo1200 },
-	{ .compatible = "qcom,rpm-pm8921-smps",     .data = &pm8921_smps },
-	{ .compatible = "qcom,rpm-pm8921-ftsmps",   .data = &pm8921_ftsmps },
-	{ .compatible = "qcom,rpm-pm8921-ncp",      .data = &pm8921_ncp },
-	{ .compatible = "qcom,rpm-pm8921-switch",   .data = &pm8921_switch },
-
-	{ .compatible = "qcom,rpm-smb208", .data = &smb208_smps },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, rpm_of_match);
-
 static int rpm_reg_set(struct qcom_rpm_reg *vreg,
 		       const struct request_member *req,
 		       const int value)
@@ -778,75 +753,177 @@ static int rpm_reg_of_parse(struct device_node *node,
 	return 0;
 }
 
-static int rpm_reg_probe(struct platform_device *pdev)
-{
-	struct regulator_init_data *initdata;
+struct rpm_regulator_data {
+	const char *name;
+	int resource;
 	const struct qcom_rpm_reg *template;
-	const struct of_device_id *match;
-	struct regulator_config config = { };
-	struct regulator_dev *rdev;
-	struct qcom_rpm_reg *vreg;
-	const char *key;
-	u32 val;
-	int ret;
+	const char *supply;
+};
+
+static const struct rpm_regulator_data rpm_pm8058_regulators[] = {
+	{ "l0",   QCOM_RPM_PM8058_LDO0,   &pm8058_nldo, "vdd_l0_l1_lvs"	},
+	{ "l1",   QCOM_RPM_PM8058_LDO1,   &pm8058_nldo, "vdd_l0_l1_lvs" },
+	{ "l2",   QCOM_RPM_PM8058_LDO2,   &pm8058_pldo, "vdd_l2_l11_l12" },
+	{ "l3",   QCOM_RPM_PM8058_LDO3,   &pm8058_pldo, "vdd_l3_l4_l5" },
+	{ "l4",   QCOM_RPM_PM8058_LDO4,   &pm8058_pldo, "vdd_l3_l4_l5" },
+	{ "l5",   QCOM_RPM_PM8058_LDO5,   &pm8058_pldo, "vdd_l3_l4_l5" },
+	{ "l6",   QCOM_RPM_PM8058_LDO6,   &pm8058_pldo, "vdd_l6_l7" },
+	{ "l7",   QCOM_RPM_PM8058_LDO7,   &pm8058_pldo, "vdd_l6_l7" },
+	{ "l8",   QCOM_RPM_PM8058_LDO8,   &pm8058_pldo, "vdd_l8" },
+	{ "l9",   QCOM_RPM_PM8058_LDO9,   &pm8058_pldo, "vdd_l9" },
+	{ "l10",  QCOM_RPM_PM8058_LDO10,  &pm8058_pldo, "vdd_l10" },
+	{ "l11",  QCOM_RPM_PM8058_LDO11,  &pm8058_pldo, "vdd_l2_l11_l12" },
+	{ "l12",  QCOM_RPM_PM8058_LDO12,  &pm8058_pldo, "vdd_l2_l11_l12" },
+	{ "l13",  QCOM_RPM_PM8058_LDO13,  &pm8058_pldo, "vdd_l13_l16" },
+	{ "l14",  QCOM_RPM_PM8058_LDO14,  &pm8058_pldo, "vdd_l14_l15" },
+	{ "l15",  QCOM_RPM_PM8058_LDO15,  &pm8058_pldo, "vdd_l14_l15" },
+	{ "l16",  QCOM_RPM_PM8058_LDO16,  &pm8058_pldo, "vdd_l13_l16" },
+	{ "l17",  QCOM_RPM_PM8058_LDO17,  &pm8058_pldo, "vdd_l17_l18" },
+	{ "l18",  QCOM_RPM_PM8058_LDO18,  &pm8058_pldo, "vdd_l17_l18" },
+	{ "l19",  QCOM_RPM_PM8058_LDO19,  &pm8058_pldo, "vdd_l19_l20" },
+	{ "l20",  QCOM_RPM_PM8058_LDO20,  &pm8058_pldo, "vdd_l19_l20" },
+	{ "l21",  QCOM_RPM_PM8058_LDO21,  &pm8058_nldo, "vdd_l21" },
+	{ "l22",  QCOM_RPM_PM8058_LDO22,  &pm8058_nldo, "vdd_l22" },
+	{ "l23",  QCOM_RPM_PM8058_LDO23,  &pm8058_nldo, "vdd_l23_l24_l25" },
+	{ "l24",  QCOM_RPM_PM8058_LDO24,  &pm8058_nldo, "vdd_l23_l24_l25" },
+	{ "l25",  QCOM_RPM_PM8058_LDO25,  &pm8058_nldo, "vdd_l23_l24_l25" },
+
+	{ "s0",   QCOM_RPM_PM8058_SMPS0,  &pm8058_smps, "vdd_s0" },
+	{ "s1",   QCOM_RPM_PM8058_SMPS1,  &pm8058_smps, "vdd_s1" },
+	{ "s2",   QCOM_RPM_PM8058_SMPS2,  &pm8058_smps, "vdd_s2" },
+	{ "s3",   QCOM_RPM_PM8058_SMPS3,  &pm8058_smps, "vdd_s3" },
+	{ "s4",   QCOM_RPM_PM8058_SMPS4,  &pm8058_smps, "vdd_s4" },
+
+	{ "lvs0", QCOM_RPM_PM8058_LVS0, &pm8058_switch, "vdd_l0_l1_lvs" },
+	{ "lvs1", QCOM_RPM_PM8058_LVS1, &pm8058_switch, "vdd_l0_l1_lvs" },
+
+	{ "ncp",  QCOM_RPM_PM8058_NCP, &pm8058_ncp, "vdd_ncp" },
+	{ }
+};
 
-	match = of_match_device(rpm_of_match, &pdev->dev);
-	template = match->data;
+static const struct rpm_regulator_data rpm_pm8901_regulators[] = {
+	{ "l0",   QCOM_RPM_PM8901_LDO0, &pm8901_nldo, "vdd_l0" },
+	{ "l1",   QCOM_RPM_PM8901_LDO1, &pm8901_pldo, "vdd_l1" },
+	{ "l2",   QCOM_RPM_PM8901_LDO2, &pm8901_pldo, "vdd_l2" },
+	{ "l3",   QCOM_RPM_PM8901_LDO3, &pm8901_pldo, "vdd_l3" },
+	{ "l4",   QCOM_RPM_PM8901_LDO4, &pm8901_pldo, "vdd_l4" },
+	{ "l5",   QCOM_RPM_PM8901_LDO5, &pm8901_pldo, "vdd_l5" },
+	{ "l6",   QCOM_RPM_PM8901_LDO6, &pm8901_pldo, "vdd_l6" },
 
-	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
-	if (!vreg) {
-		dev_err(&pdev->dev, "failed to allocate vreg\n");
-		return -ENOMEM;
-	}
-	memcpy(vreg, template, sizeof(*vreg));
-	mutex_init(&vreg->lock);
-	vreg->dev = &pdev->dev;
-	vreg->desc.id = -1;
-	vreg->desc.owner = THIS_MODULE;
-	vreg->desc.type = REGULATOR_VOLTAGE;
-	vreg->desc.name = pdev->dev.of_node->name;
-	vreg->desc.supply_name = "vin";
-
-	vreg->rpm = dev_get_drvdata(pdev->dev.parent);
-	if (!vreg->rpm) {
-		dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
-		return -ENODEV;
-	}
+	{ "s0",   QCOM_RPM_PM8901_SMPS0, &pm8901_ftsmps, "vdd_s0" },
+	{ "s1",   QCOM_RPM_PM8901_SMPS1, &pm8901_ftsmps, "vdd_s1" },
+	{ "s2",   QCOM_RPM_PM8901_SMPS2, &pm8901_ftsmps, "vdd_s2" },
+	{ "s3",   QCOM_RPM_PM8901_SMPS3, &pm8901_ftsmps, "vdd_s3" },
+	{ "s4",   QCOM_RPM_PM8901_SMPS4, &pm8901_ftsmps, "vdd_s4" },
 
-	initdata = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
-					      &vreg->desc);
-	if (!initdata)
-		return -EINVAL;
+	{ "lvs0", QCOM_RPM_PM8901_LVS0, &pm8901_switch, "lvs0_in" },
+	{ "lvs1", QCOM_RPM_PM8901_LVS1, &pm8901_switch, "lvs1_in" },
+	{ "lvs2", QCOM_RPM_PM8901_LVS2, &pm8901_switch, "lvs2_in" },
+	{ "lvs3", QCOM_RPM_PM8901_LVS3, &pm8901_switch, "lvs3_in" },
 
-	key = "reg";
-	ret = of_property_read_u32(pdev->dev.of_node, key, &val);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to read %s\n", key);
-		return ret;
-	}
-	vreg->resource = val;
+	{ "mvs", QCOM_RPM_PM8901_MVS, &pm8901_switch, "mvs_in" },
+	{ }
+};
 
-	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
-	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
-		dev_err(&pdev->dev, "no voltage specified for regulator\n");
-		return -EINVAL;
-	}
+static const struct rpm_regulator_data rpm_pm8921_regulators[] = {
+	{ "s1",  QCOM_RPM_PM8921_SMPS1, &pm8921_smps },
+	{ "s2",  QCOM_RPM_PM8921_SMPS2, &pm8921_smps },
+	{ "s3",  QCOM_RPM_PM8921_SMPS3, &pm8921_smps },
+	{ "s4",  QCOM_RPM_PM8921_SMPS4, &pm8921_smps },
+	{ "s7",  QCOM_RPM_PM8921_SMPS7, &pm8921_smps },
+	{ "s8",  QCOM_RPM_PM8921_SMPS8, &pm8921_smps },
+
+	{ "l1",  QCOM_RPM_PM8921_LDO1, &pm8921_nldo, "vin_l1_l2_l12_l18" },
+	{ "l2",  QCOM_RPM_PM8921_LDO2, &pm8921_nldo, "vin_l1_l2_l12_l18" },
+	{ "l3",  QCOM_RPM_PM8921_LDO3, &pm8921_pldo },
+	{ "l4",  QCOM_RPM_PM8921_LDO4, &pm8921_pldo },
+	{ "l5",  QCOM_RPM_PM8921_LDO5, &pm8921_pldo },
+	{ "l6",  QCOM_RPM_PM8921_LDO6, &pm8921_pldo },
+	{ "l7",  QCOM_RPM_PM8921_LDO7, &pm8921_pldo },
+	{ "l8",  QCOM_RPM_PM8921_LDO8, &pm8921_pldo },
+	{ "l9",  QCOM_RPM_PM8921_LDO9, &pm8921_pldo },
+	{ "l10", QCOM_RPM_PM8921_LDO10, &pm8921_pldo },
+	{ "l11", QCOM_RPM_PM8921_LDO11, &pm8921_pldo },
+	{ "l12", QCOM_RPM_PM8921_LDO12, &pm8921_nldo },
+	{ "l14", QCOM_RPM_PM8921_LDO14, &pm8921_pldo },
+	{ "l15", QCOM_RPM_PM8921_LDO15, &pm8921_pldo },
+	{ "l16", QCOM_RPM_PM8921_LDO16, &pm8921_pldo },
+	{ "l17", QCOM_RPM_PM8921_LDO17, &pm8921_pldo },
+	{ "l18", QCOM_RPM_PM8921_LDO18, &pm8921_nldo, "vin_l1_l2_l12_l18" },
+	{ "l21", QCOM_RPM_PM8921_LDO21, &pm8921_pldo },
+	{ "l22", QCOM_RPM_PM8921_LDO22, &pm8921_pldo },
+	{ "l23", QCOM_RPM_PM8921_LDO23, &pm8921_pldo },
+	{ "l24", QCOM_RPM_PM8921_LDO24, &pm8921_nldo1200, "vin_l24" },
+	{ "l25", QCOM_RPM_PM8921_LDO25, &pm8921_nldo1200, "vin_l25" },
+	{ "l26", QCOM_RPM_PM8921_LDO26, &pm8921_nldo1200 },
+	{ "l27", QCOM_RPM_PM8921_LDO27, &pm8921_nldo1200, "vin_l27" },
+	{ "l28", QCOM_RPM_PM8921_LDO28, &pm8921_nldo1200, "vin_l28" },
+	{ "l29", QCOM_RPM_PM8921_LDO29, &pm8921_pldo },
+
+	{ "lvs1", QCOM_RPM_PM8921_LVS1, &pm8921_switch, "vin_lvs_1_3_6" },
+	{ "lvs2", QCOM_RPM_PM8921_LVS2, &pm8921_switch, "vin_lvs2" },
+	{ "lvs3", QCOM_RPM_PM8921_LVS3, &pm8921_switch, "vin_lvs_1_3_6" },
+	{ "lvs4", QCOM_RPM_PM8921_LVS4, &pm8921_switch, "vin_lvs_4_5_7" },
+	{ "lvs5", QCOM_RPM_PM8921_LVS5, &pm8921_switch, "vin_lvs_4_5_7" },
+	{ "lvs6", QCOM_RPM_PM8921_LVS6, &pm8921_switch, "vin_lvs_1_3_6" },
+	{ "lvs7", QCOM_RPM_PM8921_LVS7, &pm8921_switch, "vin_lvs_4_5_7" },
+
+	{ "usb-switch", QCOM_RPM_USB_OTG_SWITCH, &pm8921_switch },
+	{ "hdmi-switch", QCOM_RPM_HDMI_SWITCH, &pm8921_switch },
+	{ "ncp", QCOM_RPM_PM8921_NCP, &pm8921_ncp, "vin_ncp" },
+	{ }
+};
 
+static const struct of_device_id rpm_of_match[] = {
+	{ .compatible = "qcom,rpm-pm8058-regulators", .data = &rpm_pm8058_regulators },
+	{ .compatible = "qcom,rpm-pm8901-regulators", .data = &rpm_pm8901_regulators },
+	{ .compatible = "qcom,rpm-pm8921-regulators", .data = &rpm_pm8921_regulators },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpm_of_match);
 
-	config.dev = &pdev->dev;
-	config.init_data = initdata;
-	config.driver_data = vreg;
-	config.of_node = pdev->dev.of_node;
+static int rpm_reg_probe(struct platform_device *pdev)
+{
+	const struct rpm_regulator_data *reg;
+	const struct of_device_id *match;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	struct qcom_rpm_reg *vreg;
 
-	ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc,
-			       &config, initdata);
-	if (ret)
-		return ret;
+	match = of_match_device(rpm_of_match, &pdev->dev);
+	for (reg = match->data; reg->name; reg++) {
+		vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
+		if (!vreg) {
+			dev_err(&pdev->dev, "failed to allocate vreg\n");
+			return -ENOMEM;
+		}
+		memcpy(vreg, reg->template, sizeof(*vreg));
+		mutex_init(&vreg->lock);
+
+		vreg->dev = &pdev->dev;
+		vreg->resource = reg->resource;
+
+		vreg->desc.id = -1;
+		vreg->desc.owner = THIS_MODULE;
+		vreg->desc.type = REGULATOR_VOLTAGE;
+		vreg->desc.name = reg->name;
+		vreg->desc.supply_name = reg->supply;
+		vreg->desc.of_match = reg->name;
+		vreg->desc.of_parse_cb = rpm_reg_of_parse;
+
+		vreg->rpm = dev_get_drvdata(pdev->dev.parent);
+		if (!vreg->rpm) {
+			dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
+			return -ENODEV;
+		}
 
-	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
-	if (IS_ERR(rdev)) {
-		dev_err(&pdev->dev, "can't register regulator\n");
-		return PTR_ERR(rdev);
+		config.dev = &pdev->dev;
+		config.driver_data = vreg;
+		rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "can't register regulator\n");
+			return PTR_ERR(rdev);
+		}
 	}
 
 	return 0;
-- 
1.8.2.2

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Mark Brown]

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

On Mon, Mar 02, 2015 at 08:25:37PM -0800, Bjorn Andersson wrote:

> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,rpm-pm8058-regulators"
> +		    "qcom,rpm-pm8901-regulators"
> +		    "qcom,rpm-pm8921-regulators"

Why do these subnodes have a compatible - do they ever appear except as
a child of a parent of the same type of device?

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

Re: [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb

[Mark Brown]

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

On Mon, Mar 02, 2015 at 08:25:38PM -0800, Bjorn Andersson wrote:

> Expose the newly created init_data to the driver's parse callback so
> that it can futher enhance it with e.g. constraints of the regulator.

Why would the driver need to do that?  I guess I'll see later on in the
series but the changelog should make that clear.  Drivers aren't
supposed to ever need to look at their init data, modifying the init
data from what the machine provied is usually a red flag.

> This is needed because calling regulator_register() with a of_node and
> of_match will overwrite the passed config->init_data. An alternative way
> would be to merge the parsed init_data with the driver provided one.

Put any explanation as to why the feature is needed in the changelog.

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

Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code

[Mark Brown]

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

On Mon, Mar 02, 2015 at 08:25:39PM -0800, Bjorn Andersson wrote:

> +	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
> +	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
> +		dev_err(&pdev->dev, "no voltage specified for regulator\n");
> +		return -EINVAL;
> +	}

OK, so it looks like we're using it because we can't read the initial
hardware state - can we introduce a specific interface for that perhaps?
A call to query the current constraints might do it.  Or possibly just
change your earlier patch to make sure the constraints are marked const
would do it, reading them isn't the problem that modifying them is.  I
might've missed some other usage somewhere though.

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

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Bjorn Andersson]

On Tue 03 Mar 04:47 PST 2015, Mark Brown wrote:

> On Mon, Mar 02, 2015 at 08:25:37PM -0800, Bjorn Andersson wrote:
> 
> > +- compatible:
> > +	Usage: required
> > +	Value type: <string>
> > +	Definition: must be one of:
> > +		    "qcom,rpm-pm8058-regulators"
> > +		    "qcom,rpm-pm8901-regulators"
> > +		    "qcom,rpm-pm8921-regulators"
> 
> Why do these subnodes have a compatible - do they ever appear except as
> a child of a parent of the same type of device?

The relationship with the parent node is 1:M;

in the case of 8960/8064 there is only one PMIC controlled by the RPM,
hence the dt will look like:

rpm {
	compatible = "qcom,rpm-apq8960";

	regulators {
		compatible = "qcom,rpm-pm8921-regulators";
		...
	};
};

But for 8660, and later for e.g. 8974 we have something like:

rpm {
	compatible = "qcom,rpm-msm8660";

	pm8058-regulators {
		compatible = "qcom,rpm-pm8058-regulators";

		vdd_xxx-supply = <&pm8058_s4>;
		...
	};

	pm8901-regulators {
		compatible = "qcom,rpm-pm8901-regulators";
		...
	};
};

I intended to match these by name, having one rpm-regulator device
instance and using desc->regulators_node to match regulators in the
right node - with of_node being the rpm node.

But this doesn't really map to reality, as supplies are a property of
pm8058 and pm8901 and not of the rpm. I ended up writing some custom
device registering code to instantiate the right number of regulator
children and give each of them the right of_node etc.


But as several other of-based platforms have a compatible in their
regulators node I consider that a viable and cleaner solution.

Regards,
Bjorn

Re: [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb

[Bjorn Andersson]

On Tue 03 Mar 04:50 PST 2015, Mark Brown wrote:

> On Mon, Mar 02, 2015 at 08:25:38PM -0800, Bjorn Andersson wrote:
> 
> > Expose the newly created init_data to the driver's parse callback so
> > that it can futher enhance it with e.g. constraints of the regulator.
> 
> Why would the driver need to do that?  I guess I'll see later on in the
> series but the changelog should make that clear.  Drivers aren't
> supposed to ever need to look at their init data, modifying the init
> data from what the machine provied is usually a red flag.
> 

I think what you're getting at is that the init_data should come from a
board file or device tree. With the reworkings done in patch 4 this
makes more sense than it did before (e.g. I no longer call
of_get_regulator_init_data()).

However, by calling register_regulator() with dev->of_node being
non-NULL and desc->of_match being something that will match a DT entry
all init_data is now coming from device tree, through:

regulator_register()
  regulator_of_get_init_data()
    of_get_regulator_init_data()
      of_get_regulation_constraints()

As this matches a regulator, there is no way for the driver (or anyone
else to affect the init_data).

The problem at hand is that there is nothing in this code path telling
the core that we can do DRMS - something we can figure out in the driver
by basically looking at the ops for the registering regulator.

> > This is needed because calling regulator_register() with a of_node and
> > of_match will overwrite the passed config->init_data. An alternative way
> > would be to merge the parsed init_data with the driver provided one.
> 
> Put any explanation as to why the feature is needed in the changelog.

Point taken.

Let me know if you think I'm on a sane path with the above reasoning and
I can resend the patch with a better description of the problem at hand.

Regards,
Bjorn

Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code

[Bjorn Andersson]

On Tue 03 Mar 06:13 PST 2015, Mark Brown wrote:

> On Mon, Mar 02, 2015 at 08:25:39PM -0800, Bjorn Andersson wrote:
> 
> > +	if ((vreg->parts->uV.mask || vreg->parts->mV.mask) &&
> > +	    (!initdata->constraints.min_uV || !initdata->constraints.max_uV)) {
> > +		dev_err(&pdev->dev, "no voltage specified for regulator\n");
> > +		return -EINVAL;
> > +	}
> 
> OK, so it looks like we're using it because we can't read the initial
> hardware state - can we introduce a specific interface for that perhaps?
> A call to query the current constraints might do it.  Or possibly just
> change your earlier patch to make sure the constraints are marked const
> would do it, reading them isn't the problem that modifying them is.  I
> might've missed some other usage somewhere though.

No, the reason for init_data is this snippet:

/* Regulators with ia property suppports drms */
if (vreg->parts->ia.mask)
	init_data->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;

This patch just moves the code around to make it possible to implement
patch 4 as cleanly as possible.


The quoted code checks to see that for regulators with voltage-nobs
(could have checked for set_voltage ops) the of-parser did find voltage
limits. So it's a sanity check that discards regulators with missing
dt parameters.

I did drop this in patch 4, as  it's not strictly needed. But the result
is that we will successfully register the regulator and the consumers
will be faced with errors upon trying to set voltage or enable it.

Regards,
Bjorn

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Stephen Boyd]

On 03/02/15 20:25, Bjorn Andersson wrote:
>
> += SUBNODES
> +
> +The RPM exposes resources to its subnodes. The below bindings specify the set
> +of valid subnodes that can operate on these resources.
> +
> +== Regulators
> +
> +Regulator notes are identified by their compatible:
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,rpm-pm8058-regulators"
> +		    "qcom,rpm-pm8901-regulators"
> +		    "qcom,rpm-pm8921-regulators"
> +
> +- vdd_l0_l1_lvs-supply:
> +- vdd_l10-supply:
> +- vdd_l13_l16-supply:
> +- vdd_l14_l15-supply:
> +- vdd_l17_l18-supply:
> +- vdd_l19_l20-supply:
> +- vdd_l21-supply:
> +- vdd_l22-supply:
> +- vdd_l23_l24_l25-supply:
> +- vdd_l2_l11_l12-supply:
> +- vdd_l3_l4_l5-supply:
> +- vdd_l6_l7-supply:
> +- vdd_l8-supply:
> +- vdd_l9-supply:
> +- vdd_ncp-supply:
> +- vdd_s0-supply:
> +- vdd_s1-supply:
> +- vdd_s2-supply:
> +- vdd_s3-supply:
> +- vdd_s4-supply:
> +	Usage: optional (pm8058 only)
> +	Value type: <phandle>
> +	Definition: reference to regulator supplying the input pin, as
> +		    described in the data sheet
> +
> +- vdd_l0-supply:
> +- vdd_l1-supply:
> +- vdd_l2-supply:
> +- vdd_l3-supply:
> +- vdd_l4-supply:
> +- vdd_l5-supply:
> +- vdd_l6-supply:
> +- vdd_s0-supply:
> +- vdd_s1-supply:
> +- vdd_s2-supply:
> +- vdd_s3-supply:
> +- vdd_s4-supply:
> +- lvs0_in-supply:
> +- lvs1_in-supply:
> +- lvs2_in-supply:
> +- lvs3_in-supply:
> +- mvs_in-supply:
> +	Usage: optional (pm8901 only)
> +	Value type: <phandle>
> +	Definition: reference to regulator supplying the input pin, as
> +		    described in the data sheet
> +
> +- vin_l1_l2_l12_l18-supply:
> +- vin_l24-supply:
> +- vin_l25-supply:
> +- vin_l27-supply:
> +- vin_l28-supply:
> +- vin_lvs_1_3_6-supply:
> +- vin_lvs2-supply:
> +- vin_lvs_4_5_7-supply:
> +- vin_ncp-supply:

Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I
left out? It looks like you've covered all the inputs for the other pmics.

> +	Usage: optional (pm8921 only)
> +	Value type: <phandle>
> +	Definition: reference to regulator supplying the input pin, as
> +		    described in the data sheet
> +
> +The regulator node houses sub-nodes for each regulator within the device. Each
> +sub-node is identified using the node's name, with valid values listed for each
> +of the pmics below.
> +


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code

[Stephen Boyd]

On 03/02/15 20:25, Bjorn Andersson wrote:
> +
> +static int rpm_reg_probe(struct platform_device *pdev)
> +{
> +	struct regulator_init_data *initdata;
> +	const struct qcom_rpm_reg *template;
> +	const struct of_device_id *match;
> +	struct regulator_config config = { };
> +	struct regulator_dev *rdev;
> +	struct qcom_rpm_reg *vreg;
> +	const char *key;
> +	u32 val;
> +	int ret;
> +
> +	match = of_match_device(rpm_of_match, &pdev->dev);
> +	template = match->data;
> +
> +	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
> +	if (!vreg) {
> +		dev_err(&pdev->dev, "failed to allocate vreg\n");

We don't need error messages on allocation failures.

> +		return -ENOMEM;
> +	}


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Bjorn Andersson]

On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote:

> On 03/02/15 20:25, Bjorn Andersson wrote:
[..]
> > +
> > +- vin_l1_l2_l12_l18-supply:
> > +- vin_l24-supply:
> > +- vin_l25-supply:
> > +- vin_l27-supply:
> > +- vin_l28-supply:
> > +- vin_lvs_1_3_6-supply:
> > +- vin_lvs2-supply:
> > +- vin_lvs_4_5_7-supply:
> > +- vin_ncp-supply:
> 
> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I
> left out? It looks like you've covered all the inputs for the other pmics.
> 

Sorry, I had problems finding the information in the rather sparse
documentation I have for the 8921, so I just assumed that I could steal
your list.

I finally managed to find the pinout diagram, so the correct list for
pm8921 seems to be:

- vdd_l1_2_12_18-supply
- vdd_l3_15_17-supply
- vdd_l5_8_16-supply
- vdd_l6_7-supply
- vdd_l9_11-supply
- vdd_l10_22-supply
- vdd_l21_23_29-supply
- vdd_l24-supply
- vdd_l25-supply
- vdd_l26-supply
- vdd_l27-supply
- vdd_l28-supply
- vdd_ncp-supply
- vdd_s1-supply
- vdd_s2-supply
- vdd_s4-supply
- vdd_s5-supply
- vdd_s6-supply
- vdd_s7-supply
- vdd_s8-supply
- vin_lvs1_3_6-supply
- vin_lvs2-supply
- vin_lvs4_5_7-supply

I will send out an updated patch with these.

Regards,
Bjorn

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Stephen Boyd]

On 03/03/15 13:54, Bjorn Andersson wrote:
> On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote:
>
>> On 03/02/15 20:25, Bjorn Andersson wrote:
> [..]
>>> +
>>> +- vin_l1_l2_l12_l18-supply:
>>> +- vin_l24-supply:
>>> +- vin_l25-supply:
>>> +- vin_l27-supply:
>>> +- vin_l28-supply:
>>> +- vin_lvs_1_3_6-supply:
>>> +- vin_lvs2-supply:
>>> +- vin_lvs_4_5_7-supply:
>>> +- vin_ncp-supply:
>> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I
>> left out? It looks like you've covered all the inputs for the other pmics.
>>
> Sorry, I had problems finding the information in the rather sparse
> documentation I have for the 8921, so I just assumed that I could steal
> your list.
>
> I finally managed to find the pinout diagram, so the correct list for
> pm8921 seems to be:
>
> - vdd_l1_2_12_18-supply
> - vdd_l3_15_17-supply
> - vdd_l5_8_16-supply

vin_l4-supply?

> - vdd_l6_7-supply
> - vdd_l9_11-supply
> - vdd_l10_22-supply
> - vdd_l21_23_29-supply

Also these seem to be vin_l21_l23_l29 where "l" precedes all numbers
(for all the LDOs).

> - vdd_l24-supply
> - vdd_l25-supply
> - vdd_l26-supply
> - vdd_l27-supply
> - vdd_l28-supply
> - vdd_ncp-supply
> - vdd_s1-supply
> - vdd_s2-supply
> - vdd_s4-supply
> - vdd_s5-supply
> - vdd_s6-supply
> - vdd_s7-supply
> - vdd_s8-supply
> - vin_lvs1_3_6-supply
> - vin_lvs2-supply
> - vin_lvs4_5_7-supply
>
> I will send out an updated patch with these.

s/vdd/vin/ seems to match my datasheet.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] regulator: qcom: Refactor of-parsing code

[Bjorn Andersson]

On Tue 03 Mar 10:56 PST 2015, Stephen Boyd wrote:

> On 03/02/15 20:25, Bjorn Andersson wrote:
> > +
> > +static int rpm_reg_probe(struct platform_device *pdev)
> > +{
> > +	struct regulator_init_data *initdata;
> > +	const struct qcom_rpm_reg *template;
> > +	const struct of_device_id *match;
> > +	struct regulator_config config = { };
> > +	struct regulator_dev *rdev;
> > +	struct qcom_rpm_reg *vreg;
> > +	const char *key;
> > +	u32 val;
> > +	int ret;
> > +
> > +	match = of_match_device(rpm_of_match, &pdev->dev);
> > +	template = match->data;
> > +
> > +	vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
> > +	if (!vreg) {
> > +		dev_err(&pdev->dev, "failed to allocate vreg\n");
> 
> We don't need error messages on allocation failures.
> 

Right, it's just that I wanted to keep these patches free from any
unrelated changes. I can add an extra patch at the end removing this and
moving the retrieval of rpm out of the for loop.

> > +		return -ENOMEM;
> > +	}

Regards,
Bjorn

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Stephen Boyd]

On 03/02/15 20:25, Bjorn Andersson wrote:
> -	config.of_node = pdev->dev.of_node;
> +static int rpm_reg_probe(struct platform_device *pdev)
> +{
> +	const struct rpm_regulator_data *reg;
> +	const struct of_device_id *match;
> +	struct regulator_config config = { };
> +	struct regulator_dev *rdev;
> +	struct qcom_rpm_reg *vreg;
>  
> -	ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc,
> -			       &config, initdata);
> -	if (ret)
> -		return ret;
> +	match = of_match_device(rpm_of_match, &pdev->dev);
> +	for (reg = match->data; reg->name; reg++) {

How does this work for the case where we may not want to add all the
regulators that a PMIC supports. I'm mostly thinking about the case
where we want to use the pm8xxx-regulator driver for a few regulators
and so we omit them from the DT for the RPM regulators.

-Stephen
> +		vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
> +		if (!vreg) {
> +			dev_err(&pdev->dev, "failed to allocate vreg\n");
> +			return -ENOMEM;
> +		}
> +		memcpy(vreg, reg->template, sizeof(*vreg));
> +		mutex_init(&vreg->lock);
> +
> +		vreg->dev = &pdev->dev;
> +		vreg->resource = reg->resource;
> +
> +		vreg->desc.id = -1;
> +		vreg->desc.owner = THIS_MODULE;
> +		vreg->desc.type = REGULATOR_VOLTAGE;
> +		vreg->desc.name = reg->name;
> +		vreg->desc.supply_name = reg->supply;
> +		vreg->desc.of_match = reg->name;
> +		vreg->desc.of_parse_cb = rpm_reg_of_parse;
> +
> +		vreg->rpm = dev_get_drvdata(pdev->dev.parent);
> +		if (!vreg->rpm) {
> +			dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
> +			return -ENODEV;
> +		}
>  
> -	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> -	if (IS_ERR(rdev)) {
> -		dev_err(&pdev->dev, "can't register regulator\n");
> -		return PTR_ERR(rdev);
> +		config.dev = &pdev->dev;
> +		config.driver_data = vreg;
> +		rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(&pdev->dev, "can't register regulator\n");
> +			return PTR_ERR(rdev);
> +		}
>  	}
>  
>  	return 0;


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Bjorn Andersson]

On Tue 03 Mar 14:02 PST 2015, Stephen Boyd wrote:

> On 03/03/15 13:54, Bjorn Andersson wrote:
> > On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote:
> >
> >> On 03/02/15 20:25, Bjorn Andersson wrote:
> > [..]
> >>> +
> >>> +- vin_l1_l2_l12_l18-supply:
> >>> +- vin_l24-supply:
> >>> +- vin_l25-supply:
> >>> +- vin_l27-supply:
> >>> +- vin_l28-supply:
> >>> +- vin_lvs_1_3_6-supply:
> >>> +- vin_lvs2-supply:
> >>> +- vin_lvs_4_5_7-supply:
> >>> +- vin_ncp-supply:
> >> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I
> >> left out? It looks like you've covered all the inputs for the other pmics.
> >>
> > Sorry, I had problems finding the information in the rather sparse
> > documentation I have for the 8921, so I just assumed that I could steal
> > your list.
> >
> > I finally managed to find the pinout diagram, so the correct list for
> > pm8921 seems to be:
> >
> > - vdd_l1_2_12_18-supply
> > - vdd_l3_15_17-supply
> > - vdd_l5_8_16-supply
> 
> vin_l4-supply?
> 

Ahh, right, I missed the vdd_l4_l14 pad.

> > - vdd_l6_7-supply
> > - vdd_l9_11-supply
> > - vdd_l10_22-supply
> > - vdd_l21_23_29-supply
> 
> Also these seem to be vin_l21_l23_l29 where "l" precedes all numbers
> (for all the LDOs).
> 

The other pmics name their inputs like that; should we just pick that
naming scheme for all these supplies and be happy?

> > - vdd_l24-supply
> > - vdd_l25-supply
> > - vdd_l26-supply
> > - vdd_l27-supply
> > - vdd_l28-supply
> > - vdd_ncp-supply
> > - vdd_s1-supply
> > - vdd_s2-supply
> > - vdd_s4-supply
> > - vdd_s5-supply
> > - vdd_s6-supply
> > - vdd_s7-supply
> > - vdd_s8-supply
> > - vin_lvs1_3_6-supply
> > - vin_lvs2-supply
> > - vin_lvs4_5_7-supply
> >
> > I will send out an updated patch with these.
> 
> s/vdd/vin/ seems to match my datasheet.
> 

For all of these? Same with the other pmics?

These are the documented pad names that I have, nice to see that they
are named differently in different documents.

Regards,
Bjorn

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Bjorn Andersson]

On Tue 03 Mar 14:09 PST 2015, Stephen Boyd wrote:

> On 03/02/15 20:25, Bjorn Andersson wrote:
> > -	config.of_node = pdev->dev.of_node;
> > +static int rpm_reg_probe(struct platform_device *pdev)
> > +{
> > +	const struct rpm_regulator_data *reg;
> > +	const struct of_device_id *match;
> > +	struct regulator_config config = { };
> > +	struct regulator_dev *rdev;
> > +	struct qcom_rpm_reg *vreg;
> >  
> > -	ret = rpm_reg_of_parse(pdev->dev.of_node, &vreg->desc,
> > -			       &config, initdata);
> > -	if (ret)
> > -		return ret;
> > +	match = of_match_device(rpm_of_match, &pdev->dev);
> > +	for (reg = match->data; reg->name; reg++) {
> 
> How does this work for the case where we may not want to add all the
> regulators that a PMIC supports. I'm mostly thinking about the case
> where we want to use the pm8xxx-regulator driver for a few regulators
> and so we omit them from the DT for the RPM regulators.
> 

An empty or non-existing regulator of_node will still be registered, but
without REGULATOR_CHANGE_STATUS nor REGULATOR_CHANGE_VOLTAGE; so any
operation on this regulator will fail with an -EPERM.

Regards,
Bjorn

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Stephen Boyd]

On 03/03/15 14:17, Bjorn Andersson wrote:
> On Tue 03 Mar 14:02 PST 2015, Stephen Boyd wrote:
>
>> On 03/03/15 13:54, Bjorn Andersson wrote:
>>> On Tue 03 Mar 10:53 PST 2015, Stephen Boyd wrote:
>>>
>>>> On 03/02/15 20:25, Bjorn Andersson wrote:
>>> [..]
>>>>> +
>>>>> +- vin_l1_l2_l12_l18-supply:
>>>>> +- vin_l24-supply:
>>>>> +- vin_l25-supply:
>>>>> +- vin_l27-supply:
>>>>> +- vin_l28-supply:
>>>>> +- vin_lvs_1_3_6-supply:
>>>>> +- vin_lvs2-supply:
>>>>> +- vin_lvs_4_5_7-supply:
>>>>> +- vin_ncp-supply:
>>>> Can you also include vin_s1,vin_s2, etc. for the rest of the supplies I
>>>> left out? It looks like you've covered all the inputs for the other pmics.
>>>>
>>> Sorry, I had problems finding the information in the rather sparse
>>> documentation I have for the 8921, so I just assumed that I could steal
>>> your list.
>>>
>>> I finally managed to find the pinout diagram, so the correct list for
>>> pm8921 seems to be:
>>>
>>> - vdd_l1_2_12_18-supply
>>> - vdd_l3_15_17-supply
>>> - vdd_l5_8_16-supply
>> vin_l4-supply?
>>
> Ahh, right, I missed the vdd_l4_l14 pad.
>
>>> - vdd_l6_7-supply
>>> - vdd_l9_11-supply
>>> - vdd_l10_22-supply
>>> - vdd_l21_23_29-supply
>> Also these seem to be vin_l21_l23_l29 where "l" precedes all numbers
>> (for all the LDOs).
>>
> The other pmics name their inputs like that; should we just pick that
> naming scheme for all these supplies and be happy?

Ok.

>>> - vdd_l24-supply
>>> - vdd_l25-supply
>>> - vdd_l26-supply
>>> - vdd_l27-supply
>>> - vdd_l28-supply
>>> - vdd_ncp-supply
>>> - vdd_s1-supply
>>> - vdd_s2-supply
>>> - vdd_s4-supply
>>> - vdd_s5-supply
>>> - vdd_s6-supply
>>> - vdd_s7-supply
>>> - vdd_s8-supply
>>> - vin_lvs1_3_6-supply
>>> - vin_lvs2-supply
>>> - vin_lvs4_5_7-supply
>>>
>>> I will send out an updated patch with these.
>> s/vdd/vin/ seems to match my datasheet.
>>
> For all of these? Same with the other pmics?
>
> These are the documented pad names that I have, nice to see that they
> are named differently in different documents.

Oh. I seem to have been looking at some pre-published doc that was still
laying around. vdd seems to match the official documentation so that
sounds good to me.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Mark Brown]

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

On Tue, Mar 03, 2015 at 02:32:35PM -0800, Bjorn Andersson wrote:
> On Tue 03 Mar 14:09 PST 2015, Stephen Boyd wrote:

> > How does this work for the case where we may not want to add all the
> > regulators that a PMIC supports. I'm mostly thinking about the case
> > where we want to use the pm8xxx-regulator driver for a few regulators
> > and so we omit them from the DT for the RPM regulators.

> An empty or non-existing regulator of_node will still be registered, but
> without REGULATOR_CHANGE_STATUS nor REGULATOR_CHANGE_VOLTAGE; so any
> operation on this regulator will fail with an -EPERM.

...but of course we'd never try any operations on it anyway as there
would be no consumers.

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

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Stephen Boyd]

On 03/03/15 15:52, Mark Brown wrote:
> On Tue, Mar 03, 2015 at 02:32:35PM -0800, Bjorn Andersson wrote:
>> On Tue 03 Mar 14:09 PST 2015, Stephen Boyd wrote:
>>> How does this work for the case where we may not want to add all the
>>> regulators that a PMIC supports. I'm mostly thinking about the case
>>> where we want to use the pm8xxx-regulator driver for a few regulators
>>> and so we omit them from the DT for the RPM regulators.
>> An empty or non-existing regulator of_node will still be registered, but
>> without REGULATOR_CHANGE_STATUS nor REGULATOR_CHANGE_VOLTAGE; so any
>> operation on this regulator will fail with an -EPERM.
> ...but of course we'd never try any operations on it anyway as there
> would be no consumers.

Yes sounds fine. The only concern is that we're probably wasting memory
with things that won't ever "match" something in DT.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Mark Brown]

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

On Tue, Mar 03, 2015 at 04:01:43PM -0800, Stephen Boyd wrote:
> On 03/03/15 15:52, Mark Brown wrote:

> > ...but of course we'd never try any operations on it anyway as there
> > would be no consumers.

> Yes sounds fine. The only concern is that we're probably wasting memory
> with things that won't ever "match" something in DT.

You still get diagnostic output about what the regulator state is which
is useful in itself and the amount of memory involved is really not
enough to worry about in the grand scheme of things.

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

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Stephen Boyd]

On 03/02/15 20:25, Bjorn Andersson wrote:
> +	match = of_match_device(rpm_of_match, &pdev->dev);
> +	for (reg = match->data; reg->name; reg++) {
> +		vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
> +		if (!vreg) {
> +			dev_err(&pdev->dev, "failed to allocate vreg\n");
> +			return -ENOMEM;
> +		}
> +		memcpy(vreg, reg->template, sizeof(*vreg));
> +		mutex_init(&vreg->lock);
> +
> +		vreg->dev = &pdev->dev;
> +		vreg->resource = reg->resource;
> +
> +		vreg->desc.id = -1;
> +		vreg->desc.owner = THIS_MODULE;
> +		vreg->desc.type = REGULATOR_VOLTAGE;
> +		vreg->desc.name = reg->name;
> +		vreg->desc.supply_name = reg->supply;
> +		vreg->desc.of_match = reg->name;
> +		vreg->desc.of_parse_cb = rpm_reg_of_parse;
> +
> +		vreg->rpm = dev_get_drvdata(pdev->dev.parent);
> +		if (!vreg->rpm) {
> +			dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
> +			return -ENODEV;
> +		}
>  
> -	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> -	if (IS_ERR(rdev)) {
> -		dev_err(&pdev->dev, "can't register regulator\n");
> -		return PTR_ERR(rdev);
> +		config.dev = &pdev->dev;
> +		config.driver_data = vreg;
> +		rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(&pdev->dev, "can't register regulator\n");
> +			return PTR_ERR(rdev);
> +		}
>  	}
>  
>  	return 0;

There's another problem with this of_parse_cb design. The regulator
framework requires supplies to be registered before consumers of the
supplies are registered. So when we register L23 we need to make sure
it's supply is already registered (S8 for example). If we used
of_regulator_match() we could sort the array by hand so that we always
register the supplies first. Or we could modify the regulator framework
to have a concept of "orphaned" supplies like the clock framework has so
that when a regulator is registered it waits until its supply is registered.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Bjorn Andersson]

On Wed 04 Mar 11:35 PST 2015, Stephen Boyd wrote:

> On 03/02/15 20:25, Bjorn Andersson wrote:
> > +	match = of_match_device(rpm_of_match, &pdev->dev);
> > +	for (reg = match->data; reg->name; reg++) {
> > +		vreg = devm_kmalloc(&pdev->dev, sizeof(*vreg), GFP_KERNEL);
> > +		if (!vreg) {
> > +			dev_err(&pdev->dev, "failed to allocate vreg\n");
> > +			return -ENOMEM;
> > +		}
> > +		memcpy(vreg, reg->template, sizeof(*vreg));
> > +		mutex_init(&vreg->lock);
> > +
> > +		vreg->dev = &pdev->dev;
> > +		vreg->resource = reg->resource;
> > +
> > +		vreg->desc.id = -1;
> > +		vreg->desc.owner = THIS_MODULE;
> > +		vreg->desc.type = REGULATOR_VOLTAGE;
> > +		vreg->desc.name = reg->name;
> > +		vreg->desc.supply_name = reg->supply;
> > +		vreg->desc.of_match = reg->name;
> > +		vreg->desc.of_parse_cb = rpm_reg_of_parse;
> > +
> > +		vreg->rpm = dev_get_drvdata(pdev->dev.parent);
> > +		if (!vreg->rpm) {
> > +			dev_err(&pdev->dev, "unable to retrieve handle to rpm\n");
> > +			return -ENODEV;
> > +		}
> >  
> > -	rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> > -	if (IS_ERR(rdev)) {
> > -		dev_err(&pdev->dev, "can't register regulator\n");
> > -		return PTR_ERR(rdev);
> > +		config.dev = &pdev->dev;
> > +		config.driver_data = vreg;
> > +		rdev = devm_regulator_register(&pdev->dev, &vreg->desc, &config);
> > +		if (IS_ERR(rdev)) {
> > +			dev_err(&pdev->dev, "can't register regulator\n");
> > +			return PTR_ERR(rdev);
> > +		}
> >  	}
> >  
> >  	return 0;
> 
> There's another problem with this of_parse_cb design. The regulator
> framework requires supplies to be registered before consumers of the
> supplies are registered.

You're right, didn't think of that.

The way I've ordered pm8921 it happens to work, but neither 8058 nor
8901 will pass probe by filling in the dependencies according to what we
have in the caf branch.

> So when we register L23 we need to make sure
> it's supply is already registered (S8 for example). If we used
> of_regulator_match() we could sort the array by hand so that we always
> register the supplies first.

Right, but that would mean that any block of regulators with internal
dependencies would miss out on the "standard" code paths through
regulator_register() and have to implement this on their own.

Just looking at the Qualcomm case, we would have to implement this
sorting mechanism in the rpm-regulator, pm8xxx-regulator, smd-regulator
and qpnp-regulator drivers.


I took a stab at implementing EPROBE_DEFER within qcom_rpm-regulator,
but as it's a mixture of internal and external dependencies (e.g.
<&pm8921_s4> vs <&pm8921_mpp 7>) this became quite messy. I started
looking at using the dt dependencies sort and iterate over the entries
in a way that adheres to their dependencies, but that's also a lot of
code.

But...

> Or we could modify the regulator framework
> to have a concept of "orphaned" supplies like the clock framework has so
> that when a regulator is registered it waits until its supply is registered.
> 

...as we're grouping all regulators into one device per PMIC we create
artificial dependencies that the hardware guys does not have. Further
more the fact that we can have some parts of the pmic exposed through
the rpm driver and others through the direct driver we have yet another
level of possible just adds to this.

It's perfectly fine to route the dependencies between two of our PMICs
in a way that on a device level we have a circular dependency.

So I think you're right, we should be able to alter the supply lookup
code to defer the EPROBE_DEFER until we actually need the supply to be
there; e.g. attempt to map supplies when an external consumer request
the regulator.
Some care needs to be taken with regards to e.g. always-on regulators.

Regards,
Bjorn

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Mark Brown]

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

On Wed, Mar 04, 2015 at 11:35:43AM -0800, Stephen Boyd wrote:

> There's another problem with this of_parse_cb design. The regulator
> framework requires supplies to be registered before consumers of the
> supplies are registered. So when we register L23 we need to make sure
> it's supply is already registered (S8 for example). If we used
> of_regulator_match() we could sort the array by hand so that we always
> register the supplies first. Or we could modify the regulator framework
> to have a concept of "orphaned" supplies like the clock framework has so
> that when a regulator is registered it waits until its supply is registered.

Dependency resolution isn't anything new, I'm not sure why you think
this is related to of_parse_cb()?  Open coding does exactly the same
thing and the ability to have device specific properties on is not
obviously related to dependency resolution so perhaps I'm
misunderstanding what you're talking about...

If you *are* talking about dependency resolution then just bulk
registering the regulators (which we should be doing anyway) and then
iterating the array until we stop making progress should do the trick
for most cases, normally there's only one PMIC in play, or have people
who care do a single struct device per regulator and then let the probe
deferral stuff sort it out.

I'm a bit nervous of the idea of having orphaning without core support,
it's not just inter regulator dependencies we need to worry about (they
use GPIOs and so on) and there's concerns about debuggability but it is
the kind of thing probe deferral was meant to be a cheap implementation
of.  There was a proposal quite recently from someone at Samsung Poland
to do something more coreish and basically split registrations in two,
one half registering the things needed for each resource and then a
second half which runs once the required resources are registered.
Pushing that along might be best, it's a more general approach.  The
component stuff Russell did has some similarities here.

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

Re: [PATCH 1/4] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Mark Brown]

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

On Tue, Mar 03, 2015 at 08:02:51AM -0800, Bjorn Andersson wrote:

> rpm {
> 	compatible = "qcom,rpm-apq8960";
> 
> 	regulators {
> 		compatible = "qcom,rpm-pm8921-regulators";

Oh, so what you're saying is that the pm8921 is not actually a MFD at
all?  Why name it like a MFD component then?

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

Re: [PATCH 2/4] regulator: core: Expose init_data to of_parse_cb

[Mark Brown]

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

On Tue, Mar 03, 2015 at 08:15:41AM -0800, Bjorn Andersson wrote:
> On Tue 03 Mar 04:50 PST 2015, Mark Brown wrote:

> > Why would the driver need to do that?  I guess I'll see later on in the
> > series but the changelog should make that clear.  Drivers aren't
> > supposed to ever need to look at their init data, modifying the init
> > data from what the machine provied is usually a red flag.

> I think what you're getting at is that the init_data should come from a
> board file or device tree. With the reworkings done in patch 4 this

Yes, that's the entire purpose of the init data.

> As this matches a regulator, there is no way for the driver (or anyone
> else to affect the init_data).

That's a goal not a problem.  There is an interface for the machine
description to configure the system integration which neither the
regulator driver nor the consumer driver should be a part of.

> The problem at hand is that there is nothing in this code path telling
> the core that we can do DRMS - something we can figure out in the driver
> by basically looking at the ops for the registering regulator.

No, that way lies people doing all the crap they usually do with putting
the default constraints for their reference design or the maximum limits
for their PMIC into the constraints and then getting upset when drivers
then go and use this to make their CPU catch fire or whatever.

You need to provide a way for the machine description to say DRMS (or
really setting the load which is what's actually happening here now we
have that op) is safe on a given system.  I think that means adding a
new permission to DT and then using that; it's more sensible to want to
use this feature in the context where we're working with an external
processor which also has a chance to have system tuning than when we're
working with the PMIC directly so a permission that enable load setting
seems good.

We could even kill the existing DRMS permission, there's one user in a
legacy STE platform.

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

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Mark Brown]

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

On Wed, Mar 04, 2015 at 03:51:46PM -0800, Bjorn Andersson wrote:

> I took a stab at implementing EPROBE_DEFER within qcom_rpm-regulator,
> but as it's a mixture of internal and external dependencies (e.g.
> <&pm8921_s4> vs <&pm8921_mpp 7>) this became quite messy. I started
> looking at using the dt dependencies sort and iterate over the entries
> in a way that adheres to their dependencies, but that's also a lot of
> code.

This is why I don't like trying to open code in subsystems, it's too
much work.

> So I think you're right, we should be able to alter the supply lookup
> code to defer the EPROBE_DEFER until we actually need the supply to be
> there; e.g. attempt to map supplies when an external consumer request
> the regulator.
> Some care needs to be taken with regards to e.g. always-on regulators.

I'm not sure why always on regulators would need special casing here?
Enabling is orthogonal to supply mapping.

Like I said in reply to Stephen's mail I'm more worried about
discoverability of problems with this approach and with interactions
with dependencies on other subsystems (mainly GPIOs).  Thinking about it
some more the other subsystems will probably sort themselves out but the
diagnosics are an issue.

I do like the idea of a general mechanism for registering dependency
resources and deferring completion until they're ready more - I'm not
sure it's even that much more work, especially if the first cut only
handles regulators as a dependency...  that feels like attacking the
right problem, there's other things people are raising with deferred
probe like your complaint about logging.

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

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Stephen Boyd]

On 03/04/15 16:30, Mark Brown wrote:
> On Wed, Mar 04, 2015 at 11:35:43AM -0800, Stephen Boyd wrote:
>
>> There's another problem with this of_parse_cb design. The regulator
>> framework requires supplies to be registered before consumers of the
>> supplies are registered. So when we register L23 we need to make sure
>> it's supply is already registered (S8 for example). If we used
>> of_regulator_match() we could sort the array by hand so that we always
>> register the supplies first. Or we could modify the regulator framework
>> to have a concept of "orphaned" supplies like the clock framework has so
>> that when a regulator is registered it waits until its supply is registered.
> Dependency resolution isn't anything new, I'm not sure why you think
> this is related to of_parse_cb()?  Open coding does exactly the same
> thing and the ability to have device specific properties on is not
> obviously related to dependency resolution so perhaps I'm
> misunderstanding what you're talking about...

I was just using of_parse_cb to indicate the difference from
of_regulator_match(). I could have said "between your design and my design".

>
> If you *are* talking about dependency resolution then just bulk
> registering the regulators (which we should be doing anyway) and then
> iterating the array until we stop making progress should do the trick
> for most cases, normally there's only one PMIC in play, or have people
> who care do a single struct device per regulator and then let the probe
> deferral stuff sort it out.

Yeah I think the brute force, keep trying approach will work for now if
we can't add orphan support to regulator core. I'm not aware of any
circular dependencies for these platforms.

>
> I'm a bit nervous of the idea of having orphaning without core support,
> it's not just inter regulator dependencies we need to worry about (they
> use GPIOs and so on) and there's concerns about debuggability but it is
> the kind of thing probe deferral was meant to be a cheap implementation
> of.  There was a proposal quite recently from someone at Samsung Poland
> to do something more coreish and basically split registrations in two,
> one half registering the things needed for each resource and then a
> second half which runs once the required resources are registered.
> Pushing that along might be best, it's a more general approach.  The
> component stuff Russell did has some similarities here.

Ah you're talking about the res track stuff[1]? That patchset seemed to
be doing a *lot* of different stuff where probe defer was just a part of
it. Frmo what I recall that still operates on the device level, where
here we want to be able to say that a particular regulator needs another
resource, but it's ok to register it's sibling regulator within this
device because that regulator doesn't need anything. Are you saying you
want the restrack stuff to work at the regulator level? If we went that
way we could do the same thing in the clock framework and get rid of the
orphan list and rely on the notifications from restrack to figure out
when a clock resource becomes fully available.

[1] https://lkml.org/lkml/2014/12/10/342

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] regulator: qcom: Rework to single platform device

[Mark Brown]

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

On Wed, Mar 04, 2015 at 05:46:25PM -0800, Stephen Boyd wrote:
> On 03/04/15 16:30, Mark Brown wrote:
> > On Wed, Mar 04, 2015 at 11:35:43AM -0800, Stephen Boyd wrote:

> > Dependency resolution isn't anything new, I'm not sure why you think
> > this is related to of_parse_cb()?  Open coding does exactly the same

> I was just using of_parse_cb to indicate the difference from
> of_regulator_match(). I could have said "between your design and my design".

Please do things like that - it makes things harder to follow if people
throw in random unrelated terms for things.

> > of.  There was a proposal quite recently from someone at Samsung Poland
> > to do something more coreish and basically split registrations in two,
> > one half registering the things needed for each resource and then a
> > second half which runs once the required resources are registered.
> > Pushing that along might be best, it's a more general approach.  The
> > component stuff Russell did has some similarities here.

> Ah you're talking about the res track stuff[1]? That patchset seemed to
> be doing a *lot* of different stuff where probe defer was just a part of
> it. Frmo what I recall that still operates on the device level, where
> here we want to be able to say that a particular regulator needs another
> resource, but it's ok to register it's sibling regulator within this
> device because that regulator doesn't need anything. Are you saying you
> want the restrack stuff to work at the regulator level? If we went that
> way we could do the same thing in the clock framework and get rid of the
> orphan list and rely on the notifications from restrack to figure out
> when a clock resource becomes fully available.

Yes, that's it.  It's not quite the same thing as registering individual
resources and there are definite issues to be addressed but I think it's
a promising approach for addressing the deferred probe problem in a more
elegant way.

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