[PATCH] regulator: qcom_spmi: do no register unused regulators

[PATCH] regulator: qcom_spmi: do no register unused regulators

[Dmitry Baryshkov]

Typically SPMI interface to PMIC regulators on Qualcomm platforms is
used to supplement RPM interface in cases where direct control is
required (e.g. for the APCC or GFX regulators). Registering all possible
regulators is thus not required and may be potentially harmfull if
somebody tries to setup those directly. Thus register only regulators
that are really used in the device tree and ignore all unused
regulators.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/regulator/qcom_spmi-regulator.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c
index 41424a3366d0..5e68c3829e50 100644
--- a/drivers/regulator/qcom_spmi-regulator.c
+++ b/drivers/regulator/qcom_spmi-regulator.c
@@ -2147,20 +2147,29 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 			dev_err(dev, "ERROR reading SAW regmap\n");
 	}
 
-	for (reg = match->data; reg->name; reg++) {
-
+	for_each_child_of_node(node, reg_node) {
 		if (saw_regmap) {
-			reg_node = of_get_child_by_name(node, reg->name);
 			reg_prop = of_find_property(reg_node, "qcom,saw-slave",
 						    &lenp);
-			of_node_put(reg_node);
 			if (reg_prop)
 				continue;
 		}
 
+		for (reg = match->data; reg->name; reg++) {
+			if (of_node_name_eq(reg_node, reg->name))
+				break;
+		}
+
+		if (!reg->name) {
+			dev_err(dev, "No regulator matches device node %pOF\n", reg_node);
+			continue;
+		}
+
 		vreg = devm_kzalloc(dev, sizeof(*vreg), GFP_KERNEL);
-		if (!vreg)
+		if (!vreg) {
+			of_node_put(reg_node);
 			return -ENOMEM;
+		}
 
 		vreg->dev = dev;
 		vreg->base = reg->base;
@@ -2211,6 +2220,7 @@ static int qcom_spmi_regulator_probe(struct platform_device *pdev)
 		rdev = devm_regulator_register(dev, &vreg->desc, &config);
 		if (IS_ERR(rdev)) {
 			dev_err(dev, "failed to register %s\n", name);
+			of_node_put(reg_node);
 			return PTR_ERR(rdev);
 		}
 
-- 
2.33.0

Re: [PATCH] regulator: qcom_spmi: do no register unused regulators

[Stephan Gerhold]

On Thu, Nov 04, 2021 at 03:43:51AM +0300, Dmitry Baryshkov wrote:
> Typically SPMI interface to PMIC regulators on Qualcomm platforms is
> used to supplement RPM interface in cases where direct control is
> required (e.g. for the APCC or GFX regulators). Registering all possible
> regulators is thus not required and may be potentially harmfull if
> somebody tries to setup those directly. Thus register only regulators
> that are really used in the device tree and ignore all unused
> regulators.
> 

Shouldn't the missing regulator constraints already take care of that?
If you don't setup any voltage etc in the device tree it should
effectively make the regulator read-only.

This is useful sometimes, I have used this for debugging before because
it lets you check the actual voltage applied by RPM (at least on MSM8916).
The voltage that Linux believes to have set through the RPM driver is
not necessarily the same that is really set at the end.

Thanks,
Stephan

Re: [PATCH] regulator: qcom_spmi: do no register unused regulators

[Mark Brown]

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

On Thu, Nov 04, 2021 at 09:23:03AM +0100, Stephan Gerhold wrote:
> On Thu, Nov 04, 2021 at 03:43:51AM +0300, Dmitry Baryshkov wrote:

> > Typically SPMI interface to PMIC regulators on Qualcomm platforms is
> > used to supplement RPM interface in cases where direct control is
> > required (e.g. for the APCC or GFX regulators). Registering all possible
> > regulators is thus not required and may be potentially harmfull if
> > somebody tries to setup those directly. Thus register only regulators
> > that are really used in the device tree and ignore all unused
> > regulators.

> Shouldn't the missing regulator constraints already take care of that?
> If you don't setup any voltage etc in the device tree it should
> effectively make the regulator read-only.

Right, we won't touch the hardware configuration unless there are
constraints explicitly permitting it.  Drivers should always register
any regulators that are physically present unconditionally, the core
will only make changes if the system integration permits it.  Open
coding this in drivers just causes code duplication and creates issues
if there's changes in the generic bindings.

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