[PATCH 1/2] regulator: core: add debug print for regulator_resolve_supply

[PATCH 1/2] regulator: core: add debug print for regulator_resolve_supply

[Ahmad Fatoum]

Starting with commit 324bd9bbe7e8 ("regulator: recursively enable/disable
regulator dependency tree"), regulator operations may affect more than
just the one regulator being enabled. Place a debug print, so it's
easier to follow the dependency chain.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 drivers/regulator/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3eb4746fb757..97f359d6d29f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -141,6 +141,8 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (!supply_name)
 		return 0;
 
+	dev_dbg(rdev->dev, "resolving %s\n", supply_name);
+
 	supply = regulator_get(rdev->dev, supply_name);
 	if (IS_ERR(supply)) {
 		if (deep_probe_is_supported())
-- 
2.38.4

[PATCH 2/2] test: self: add basic regulator selftest

[Ahmad Fatoum]

This simple test verifies registration and always-on enabling and disabling
work as they should. It may be extended in future to support more
complex cases.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 test/self/Kconfig             |   5 +
 test/self/Makefile            |   1 +
 test/self/regulator.c         | 188 ++++++++++++++++++++++++++++++++++
 test/self/test_regulator.dtso |  43 ++++++++
 4 files changed, 237 insertions(+)
 create mode 100644 test/self/regulator.c
 create mode 100644 test/self/test_regulator.dtso

diff --git a/test/self/Kconfig b/test/self/Kconfig
index ce5048c70ec9..ea436aa34664 100644
--- a/test/self/Kconfig
+++ b/test/self/Kconfig
@@ -36,6 +36,7 @@ config SELFTEST_ENABLE_ALL
 	imply SELFTEST_FS_RAMFS
 	imply SELFTEST_TFTP
 	imply SELFTEST_JSON
+	imply SELFTEST_REGULATOR
 	help
 	  Selects all self-tests compatible with current configuration
 
@@ -69,4 +70,8 @@ config SELFTEST_JSON
 	bool "JSON selftest"
 	depends on JSMN
 
+config SELFTEST_REGULATOR
+	bool "Regulator selftest"
+	depends on REGULATOR && OFDEVICE
+
 endif
diff --git a/test/self/Makefile b/test/self/Makefile
index 98ebd1fd66c1..a5dc1531dc11 100644
--- a/test/self/Makefile
+++ b/test/self/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_SELFTEST_OF_MANIPULATION) += of_manipulation.o of_manipulation.dtb.
 obj-$(CONFIG_SELFTEST_ENVIRONMENT_VARIABLES) += envvar.o
 obj-$(CONFIG_SELFTEST_FS_RAMFS) += ramfs.o
 obj-$(CONFIG_SELFTEST_JSON) += json.o
+obj-$(CONFIG_SELFTEST_REGULATOR) += regulator.o test_regulator.dtbo.o
 
 clean-files := *.dtb *.dtb.S .*.dtc .*.pre .*.dts *.dtb.z
 clean-files += *.dtbo *.dtbo.S .*.dtso
diff --git a/test/self/regulator.c b/test/self/regulator.c
new file mode 100644
index 000000000000..cfbfd0490385
--- /dev/null
+++ b/test/self/regulator.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define DEBUG 1
+#include <common.h>
+#include <bselftest.h>
+#include <driver.h>
+#include <of.h>
+#include <regulator.h>
+#include <linux/regulator/of_regulator.h>
+
+struct test_regulator {
+	struct device *dev;
+};
+
+struct test_regulator_cfg {
+	struct regulator_desc desc;
+	struct regulator_dev rdev;
+};
+
+BSELFTEST_GLOBALS();
+
+static bool __ok(bool cond, const char *func, int line)
+{
+	total_tests++;
+	if (!cond) {
+		failed_tests++;
+		printf("%s:%d: assertion failure\n", func, line);
+		return false;
+	}
+
+	return true;
+}
+
+#define ok(cond) \
+	__ok(cond, __func__, __LINE__)
+
+static void test_regulator_selfref_always_on(struct device *dev)
+{
+	ok(1 == 1);
+}
+
+static int test_regulator_enable_noop(struct regulator_dev *rdev)
+{
+	dev_dbg(rdev->dev, "enabling %s-supply\n", rdev->desc->supply_name);
+	failed_tests--;
+	return 0;
+}
+
+static int test_regulator_disable_noop(struct regulator_dev *rdev)
+{
+	dev_dbg(rdev->dev, "disabling %s-supply\n", rdev->desc->supply_name);
+	failed_tests--;
+	return 0;
+}
+
+static const struct regulator_ops test_regulator_ops_range = {
+	.enable		= test_regulator_enable_noop,
+	.disable	= test_regulator_disable_noop,
+};
+
+enum {
+	/*
+	 * This is intentionally ordered that way to test registering
+	 * an always-on regulator before its sibling that it depends on
+	 */
+	TEST_LDO1,
+	TEST_BUCK,
+	TEST_LDO2,
+	TEST_REGULATORS_NUM
+};
+
+static struct test_regulator_cfg test_pmic_reg[] = {
+	[TEST_BUCK] = {{
+		 .supply_name = "buck",
+		 .ops = &test_regulator_ops_range,
+	}},
+	[TEST_LDO1] = {{
+		 .supply_name = "ldo1",
+		 .ops = &test_regulator_ops_range,
+	}},
+	[TEST_LDO2] = {{
+		.supply_name = "ldo2",
+		.ops = &test_regulator_ops_range,
+	}},
+};
+
+static struct of_regulator_match test_reg_matches[] = {
+	[TEST_BUCK] = { .name = "BUCK", .desc = &test_pmic_reg[TEST_BUCK].desc },
+	[TEST_LDO1] = { .name = "LDO1", .desc = &test_pmic_reg[TEST_LDO1].desc },
+	[TEST_LDO2] = { .name = "LDO2", .desc = &test_pmic_reg[TEST_LDO2].desc },
+};
+
+static int test_regulator_register(struct test_regulator *priv, int id,
+				    struct of_regulator_match *match,
+				    struct test_regulator_cfg *cfg)
+{
+	struct device *dev = priv->dev;
+	int ret;
+
+	if (!match->of_node) {
+		dev_warn(dev, "Skip missing DTB regulator %s\n", match->name);
+		return 0;
+	}
+
+	cfg->rdev.desc = &cfg->desc;
+	cfg->rdev.dev = dev;
+
+	ret = of_regulator_register(&cfg->rdev, match->of_node);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register %s regulator\n",
+				     match->name);
+
+	dev_dbg(dev, "registered %s\n", match->name);
+
+	return 0;
+}
+
+static int regulator_probe(struct device *dev)
+{
+	size_t nregulators = ARRAY_SIZE(test_pmic_reg);
+	struct device_node *np = dev->of_node;
+	struct test_regulator *priv;
+	int ret, i;
+
+	priv = xzalloc(sizeof(*priv));
+	priv->dev = dev;
+
+	total_tests += 2;
+	failed_tests += 2;
+
+	np = of_get_child_by_name(np, "regulators");
+	if (!np)
+		return -ENOENT;
+
+	ret = of_regulator_match(dev, np, test_reg_matches, nregulators);
+	if (ret < 0)
+		return ret;
+
+	ok(ret == TEST_REGULATORS_NUM);
+
+	for (i = 0; i < nregulators; i++) {
+		ret = test_regulator_register(priv, i, &test_reg_matches[i],
+					      &test_pmic_reg[i]);
+		ok(ret == 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id test_regulator_of_match[] = {
+	{ .compatible = "barebox,regulator-self-test" },
+	{ /* sentintel */ },
+};
+
+static struct driver regulator_test_driver = {
+	.name = "regulator-test",
+	.probe = regulator_probe,
+	.of_match_table = test_regulator_of_match,
+};
+
+static struct device_d *dev;
+
+static void test_regulator(void)
+{
+	extern char __dtbo_test_regulator_start[];
+	struct device_node *overlay;
+	int ret;
+
+	if (!dev) {
+		ret = platform_driver_register(&regulator_test_driver);
+		if (ret)
+			return;
+
+		overlay = of_unflatten_dtb(__dtbo_test_regulator_start, INT_MAX);
+		of_overlay_apply_tree(of_get_root_node(), overlay);
+		of_probe();
+
+		dev = of_find_device_by_node_path("/regulator-self-test-pmic");
+
+		ok(dev->driver != NULL);
+	}
+
+	test_regulator_selfref_always_on(dev);
+}
+bselftest(core, test_regulator);
diff --git a/test/self/test_regulator.dtso b/test/self/test_regulator.dtso
new file mode 100644
index 000000000000..65d2b130988d
--- /dev/null
+++ b/test/self/test_regulator.dtso
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	regulator_test_fixed: regulator-self-test-fixed {
+		compatible = "regulator-fixed";
+		regulator-name = "test_fixed";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+
+	regulator-self-test-pmic {
+		compatible = "barebox,regulator-self-test";
+
+		buck-supply = <&regulator_test_fixed>;
+		ldo1-supply = <&test_pmic_buck>;
+		ldo2-supply = <&test_pmic_buck>;
+
+		regulators {
+			test_pmic_buck: BUCK {
+				regulator-name = "buck";
+				regulator-min-microvolt = <330000>;
+				regulator-max-microvolt = <330000>;
+			};
+
+			test_pmic_ldo1: LDO1 {
+				regulator-name = "ldo1";
+				regulator-always-on;
+				regulator-boot-on;
+				regulator-min-microvolt = <180000>;
+				regulator-max-microvolt = <180000>;
+			};
+
+			test_pmic_ldo2: LDO2 {
+				regulator-name = "ldo2";
+				regulator-min-microvolt = <180000>;
+				regulator-max-microvolt = <180000>;
+			};
+		};
+	};
+};
-- 
2.38.4

Re: [PATCH 2/2] test: self: add basic regulator selftest

[Sascha Hauer]

On Mon, Apr 24, 2023 at 01:59:08PM +0200, Ahmad Fatoum wrote:
> This simple test verifies registration and always-on enabling and disabling
> work as they should. It may be extended in future to support more
> complex cases.
> 
> +static const struct of_device_id test_regulator_of_match[] = {
> +	{ .compatible = "barebox,regulator-self-test" },
> +	{ /* sentintel */ },
> +};
> +
> +static struct driver regulator_test_driver = {
> +	.name = "regulator-test",
> +	.probe = regulator_probe,
> +	.of_match_table = test_regulator_of_match,
> +};
> +
> +static struct device_d *dev;
> +
> +static void test_regulator(void)
> +{
> +	extern char __dtbo_test_regulator_start[];
> +	struct device_node *overlay;
> +	int ret;
> +
> +	if (!dev) {
> +		ret = platform_driver_register(&regulator_test_driver);
> +		if (ret)
> +			return;
> +
> +		overlay = of_unflatten_dtb(__dtbo_test_regulator_start, INT_MAX);
> +		of_overlay_apply_tree(of_get_root_node(), overlay);
> +		of_probe();

This needs CONFIG_OF_OVERLAY_LIVE enabled to work. Should we make sure
this option is enabled before running this test?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] fixup! test: self: add basic regulator selftest

[Ahmad Fatoum]

  - fix order, so test doesn't fail.
  - print debug message before registration, so early failure
    is easier to follow
  - don't define DEBUG by default

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 test/self/regulator.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/test/self/regulator.c b/test/self/regulator.c
index adb4d93c2114..1ccaae0ed332 100644
--- a/test/self/regulator.c
+++ b/test/self/regulator.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#define DEBUG 1
+
 #include <common.h>
 #include <bselftest.h>
 #include <driver.h>
@@ -60,12 +60,8 @@ static const struct regulator_ops test_regulator_ops_range = {
 };
 
 enum {
-	/*
-	 * This is intentionally ordered that way to test registering
-	 * an always-on regulator before its sibling that it depends on
-	 */
-	TEST_LDO1,
 	TEST_BUCK,
+	TEST_LDO1,
 	TEST_LDO2,
 	TEST_REGULATORS_NUM
 };
@@ -106,13 +102,12 @@ static int test_regulator_register(struct test_regulator *priv, int id,
 	cfg->rdev.desc = &cfg->desc;
 	cfg->rdev.dev = dev;
 
+	dev_dbg(dev, "registering %s\n", match->name);
+
 	ret = of_regulator_register(&cfg->rdev, match->of_node);
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to register %s regulator\n",
 				     match->name);
-
-	dev_dbg(dev, "registered %s\n", match->name);
-
 	return 0;
 }
 
-- 
2.38.4

Re: [PATCH 2/2] test: self: add basic regulator selftest

[Ahmad Fatoum]

Hello Sascha,

On 02.05.23 11:22, Sascha Hauer wrote:
> On Mon, Apr 24, 2023 at 01:59:08PM +0200, Ahmad Fatoum wrote:
>> This simple test verifies registration and always-on enabling and disabling
>> work as they should. It may be extended in future to support more
>> complex cases.
>>
>> +static const struct of_device_id test_regulator_of_match[] = {
>> +	{ .compatible = "barebox,regulator-self-test" },
>> +	{ /* sentintel */ },
>> +};
>> +
>> +static struct driver regulator_test_driver = {
>> +	.name = "regulator-test",
>> +	.probe = regulator_probe,
>> +	.of_match_table = test_regulator_of_match,
>> +};
>> +
>> +static struct device_d *dev;
>> +
>> +static void test_regulator(void)
>> +{
>> +	extern char __dtbo_test_regulator_start[];
>> +	struct device_node *overlay;
>> +	int ret;
>> +
>> +	if (!dev) {
>> +		ret = platform_driver_register(&regulator_test_driver);
>> +		if (ret)
>> +			return;
>> +
>> +		overlay = of_unflatten_dtb(__dtbo_test_regulator_start, INT_MAX);
>> +		of_overlay_apply_tree(of_get_root_node(), overlay);
>> +		of_probe();
> 
> This needs CONFIG_OF_OVERLAY_LIVE enabled to work. Should we make sure
> this option is enabled before running this test?

No, it doesn't. The overlay has no phandle references into the barebox
live tree, so it can be applied without symbols. This is similar to the
QEMU overlay, where the QEMU-provided DT also lacks symbols.

Cheers,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] test: self: add basic regulator selftest

[Sascha Hauer]

On Tue, May 02, 2023 at 12:04:04PM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 02.05.23 11:22, Sascha Hauer wrote:
> > On Mon, Apr 24, 2023 at 01:59:08PM +0200, Ahmad Fatoum wrote:
> >> This simple test verifies registration and always-on enabling and disabling
> >> work as they should. It may be extended in future to support more
> >> complex cases.
> >>
> >> +static const struct of_device_id test_regulator_of_match[] = {
> >> +	{ .compatible = "barebox,regulator-self-test" },
> >> +	{ /* sentintel */ },
> >> +};
> >> +
> >> +static struct driver regulator_test_driver = {
> >> +	.name = "regulator-test",
> >> +	.probe = regulator_probe,
> >> +	.of_match_table = test_regulator_of_match,
> >> +};
> >> +
> >> +static struct device_d *dev;
> >> +
> >> +static void test_regulator(void)
> >> +{
> >> +	extern char __dtbo_test_regulator_start[];
> >> +	struct device_node *overlay;
> >> +	int ret;
> >> +
> >> +	if (!dev) {
> >> +		ret = platform_driver_register(&regulator_test_driver);
> >> +		if (ret)
> >> +			return;
> >> +
> >> +		overlay = of_unflatten_dtb(__dtbo_test_regulator_start, INT_MAX);
> >> +		of_overlay_apply_tree(of_get_root_node(), overlay);
> >> +		of_probe();
> > 
> > This needs CONFIG_OF_OVERLAY_LIVE enabled to work. Should we make sure
> > this option is enabled before running this test?
> 
> No, it doesn't. The overlay has no phandle references into the barebox
> live tree, so it can be applied without symbols. This is similar to the
> QEMU overlay, where the QEMU-provided DT also lacks symbols.

Ok, I see. It still needs CONFIG_OF_OVERLAY though. Without it
of_overlay_apply_tree will be a stub.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |