[PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Alban Bedel]

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 .../bindings/regulator/constrained-supply.txt      | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/constrained-supply.txt

diff --git a/Documentation/devicetree/bindings/regulator/constrained-supply.txt b/Documentation/devicetree/bindings/regulator/constrained-supply.txt
new file mode 100644
index 0000000..3c85430
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/constrained-supply.txt
@@ -0,0 +1,29 @@
+Constrained Supply Regulator
+
+This binding allow creating a virtual regulator that constrain its
+supply to provides the requested voltage. This is to allow using
+simple drivers that don't explicitly request a voltage on boards
+that have adjustable hardware regulators.
+
+Required properties:
+- compatible : Must be "regulator-constrained-supply".
+- vin-supply : phandle to the parent supply/regulator node
+
+Optional properties:
+- any property defined in regulator.txt
+
+Example:
+
+  /* Adjustable regulator for extension boards */
+  vdd_ext: regulator@0 {
+  	regulator-min-microvolt = <1000000>;
+  	regulator-max-microvolt = <10000000>;
+  };
+
+  /* An extension board that need 3.3V */
+  vdd_ext_board1: regulator@1 {
+	compatible = "regulator-constrained-supply";
+  	regulator-min-microvolt = <3300000>;
+  	regulator-max-microvolt = <3300000>;
+	vin-supply = <&vdd_ext>;
+  };
-- 
2.1.3

[PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Alban Bedel]

Signed-off-by: Alban Bedel <alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 .../bindings/regulator/constrained-supply.txt      | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/constrained-supply.txt

diff --git a/Documentation/devicetree/bindings/regulator/constrained-supply.txt b/Documentation/devicetree/bindings/regulator/constrained-supply.txt
new file mode 100644
index 0000000..3c85430
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/constrained-supply.txt
@@ -0,0 +1,29 @@
+Constrained Supply Regulator
+
+This binding allow creating a virtual regulator that constrain its
+supply to provides the requested voltage. This is to allow using
+simple drivers that don't explicitly request a voltage on boards
+that have adjustable hardware regulators.
+
+Required properties:
+- compatible : Must be "regulator-constrained-supply".
+- vin-supply : phandle to the parent supply/regulator node
+
+Optional properties:
+- any property defined in regulator.txt
+
+Example:
+
+  /* Adjustable regulator for extension boards */
+  vdd_ext: regulator@0 {
+  	regulator-min-microvolt = <1000000>;
+  	regulator-max-microvolt = <10000000>;
+  };
+
+  /* An extension board that need 3.3V */
+  vdd_ext_board1: regulator@1 {
+	compatible = "regulator-constrained-supply";
+  	regulator-min-microvolt = <3300000>;
+  	regulator-max-microvolt = <3300000>;
+	vin-supply = <&vdd_ext>;
+  };
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] regulator: add a regulator that constrain its supply

[Alban Bedel]

This driver is meant for devices that are supplied by a settable
regulator but that don't set their supply voltage explicitly.
With this reglator those simple driver can just get and enable the
regulator and they will get the correct voltage.

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 drivers/regulator/Kconfig              |   8 +++
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/constrained-supply.c | 126 +++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+)
 create mode 100644 drivers/regulator/constrained-supply.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c8767b7..ebfd9a7 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -54,6 +54,14 @@ config REGULATOR_USERSPACE_CONSUMER
 
 	  If unsure, say no.
 
+config REGULATOR_CONSTRAINED_SUPPLY
+	tristate "Voltage regulator that constrain its supply"
+	depends on OF
+	help
+	  This driver provides a virtual regulator that constrains its supply.
+	  This is mostly useful to use drivers that don't explicitly set a
+	  voltage on boards that use variable regulators.
+
 config REGULATOR_88PM800
 	tristate "Marvell 88PM800 Power regulators"
 	depends on MFD_88PM800
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 37bb4b7..99c7979 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
+obj-$(CONFIG_REGULATOR_CONSTRAINED_SUPPLY) += constrained-supply.o
 
 obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
diff --git a/drivers/regulator/constrained-supply.c b/drivers/regulator/constrained-supply.c
new file mode 100644
index 0000000..1ab94b0
--- /dev/null
+++ b/drivers/regulator/constrained-supply.c
@@ -0,0 +1,126 @@
+/*
+ * Regulator driver that constrain its supply
+ *
+ * Copyright (C) 2014 - Alban Bedel
+ *
+ * Author: Alban Bedel <alban.bedel@avionic-design.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+/* Here we can't use rdev->supply as these functions are also
+ * called before the supply is set.
+ */
+static int constrained_supply_set_voltage(struct regulator_dev *rdev,
+					int min_uV, int max_uV,
+					unsigned *selector)
+{
+	struct regulator *supply = rdev_get_drvdata(rdev);
+
+	return regulator_set_voltage(supply, min_uV, max_uV);
+}
+
+static int constrained_supply_get_voltage(struct regulator_dev *rdev)
+{
+	struct regulator *supply = rdev_get_drvdata(rdev);
+
+	return regulator_get_voltage(supply);
+}
+
+static int constrained_supply_regulator_enable(struct regulator_dev *rdev)
+{
+	return 0;
+}
+
+static int constrained_supply_regulator_disable(struct regulator_dev *rdev)
+{
+	return 0;
+}
+
+static int constrained_supply_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	return rdev->use_count > 0;
+}
+
+static struct regulator_ops constrained_supply_regulator_ops = {
+	.get_voltage	= constrained_supply_get_voltage,
+	.set_voltage	= constrained_supply_set_voltage,
+	.enable		= constrained_supply_regulator_enable,
+	.disable	= constrained_supply_regulator_disable,
+	.is_enabled	= constrained_supply_regulator_is_enabled,
+};
+
+static const struct regulator_desc constrained_supply_regulator_desc = {
+	.name		= "constrained-supply",
+	.ops		= &constrained_supply_regulator_ops,
+	.type		= REGULATOR_VOLTAGE,
+	.owner		= THIS_MODULE,
+	.supply_name    = "vin",
+};
+
+static int constrained_supply_regulator_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regulator_config config = {};
+	struct regulator_dev *regulator;
+	struct regulator *supply;
+
+	if (!np)
+		return -EINVAL;
+
+	/* We need the supply to be available */
+	supply = devm_regulator_get(&pdev->dev,
+				constrained_supply_regulator_desc.supply_name);
+	if (IS_ERR(supply)) {
+		dev_err(&pdev->dev, "Failed to get supply regulator\n");
+		return PTR_ERR(supply);
+	}
+
+	config.init_data = of_get_regulator_init_data(&pdev->dev, np);
+	if (!config.init_data)
+		return -ENOMEM;
+
+	config.of_node = np;
+	config.dev = &pdev->dev;
+	config.driver_data = supply;
+
+	regulator = devm_regulator_register(
+		&pdev->dev, &constrained_supply_regulator_desc, &config);
+	if (IS_ERR(regulator)) {
+		dev_err(&pdev->dev, "Failed to register regulator\n");
+		return PTR_ERR(regulator);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id constrained_supply_of_match[] = {
+	{ .compatible = "regulator-constrained-supply" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, constrained_supply_of_match);
+
+static struct platform_driver constrained_supply_regulator_driver = {
+	.driver = {
+		.name		= "constrained-supply-regulator",
+		.owner		= THIS_MODULE,
+		.of_match_table = of_match_ptr(constrained_supply_of_match),
+	},
+	.probe = constrained_supply_regulator_probe,
+};
+module_platform_driver(constrained_supply_regulator_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alban Bedel <alban.bedel@avionic-design.de>");
+MODULE_DESCRIPTION("Constrained Supply Regulator Driver");
+MODULE_ALIAS("platform:-regulator");
-- 
2.1.3

[PATCH 3/4] devicetree: add a binding for a group of regulator

[Alban Bedel]

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 .../devicetree/bindings/regulator/group.txt        | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/group.txt

diff --git a/Documentation/devicetree/bindings/regulator/group.txt b/Documentation/devicetree/bindings/regulator/group.txt
new file mode 100644
index 0000000..5f811cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/group.txt
@@ -0,0 +1,26 @@
+Regulator Group
+
+This binding allow creating a group of regulators for use with simple
+drivers that only expect a single power supply. Additionally it is
+possible to enforce the enable ordering to create simple power up
+sequences.
+
+Required properties:
+- compatible : Must be "regulator-group".
+- regulator-supplies : List of the supplies names.
+- <name>-supply : One entry for each supply defined in regulator-supplies.
+
+Optional properties:
+- ordered-supplies : set if the supplies should be enabled in order,
+   otherwise they are all enable or disabled in parallel.
+- any property defined in regulator.txt
+
+Example:
+
+  regulator {
+	compatible = "regulator-group";
+	regulator-supplies = "vcc_a", "vcc_b";
+	vcc_a-supply = <&vcc_a>;
+	vcc_b-supply = <&vcc_b>;
+	ordered-supplies;
+  };
-- 
2.1.3

[PATCH 4/4] regulator: add a regulator group driver

[Alban Bedel]

This driver allow using simple driver that expect a single regulator
on hardware that need to enable several regulators. Optionally the
driver can enforce the enable and disable order to provide a simple
power sequencing.

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 drivers/regulator/Kconfig  |   8 +++
 drivers/regulator/Makefile |   1 +
 drivers/regulator/group.c  | 161 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 drivers/regulator/group.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index ebfd9a7..895adc6 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -62,6 +62,14 @@ config REGULATOR_CONSTRAINED_SUPPLY
 	  This is mostly useful to use drivers that don't explicitly set a
 	  voltage on boards that use variable regulators.
 
+config REGULATOR_GROUP
+	tristate "Regulator group"
+	depends on OF
+	help
+	  This driver allow grouping regulators to use simple drivers that
+	  expect a single regulator on hardware where several regulator
+	  are required.
+
 config REGULATOR_88PM800
 	tristate "Marvell 88PM800 Power regulators"
 	depends on MFD_88PM800
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 99c7979..263ce6a 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
 obj-$(CONFIG_REGULATOR_CONSTRAINED_SUPPLY) += constrained-supply.o
+obj-$(CONFIG_REGULATOR_GROUP) += group.o
 
 obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
diff --git a/drivers/regulator/group.c b/drivers/regulator/group.c
new file mode 100644
index 0000000..8373430
--- /dev/null
+++ b/drivers/regulator/group.c
@@ -0,0 +1,161 @@
+/*
+ * Regulator driver that group several supplies together
+ *
+ * Copyright (C) 2014 - Alban Bedel
+ *
+ * Author: Alban Bedel <alban.bedel@avionic-design.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/of_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+struct regulator_group {
+	bool ordered;
+	unsigned int supply_count;
+	struct regulator_bulk_data supply[];
+};
+
+static int regulator_group_enable(struct regulator_dev *rdev)
+{
+	struct regulator_group *group = rdev_get_drvdata(rdev);
+	int i, err = 0;
+
+	/* If no ordering is needed just use bulk enable */
+	if (!group->ordered)
+		return regulator_bulk_enable(
+			group->supply_count, group->supply);
+
+	/* Otherwise do it ourself */
+	for (i = 0; !err && i < group->supply_count; i++)
+		err = regulator_enable(group->supply[i].consumer);
+
+	/* Rollback in case of error */
+	if (err)
+		for (i--; i >= 0; i--)
+			regulator_disable(group->supply[i].consumer);
+
+	return err;
+}
+
+static int regulator_group_disable(struct regulator_dev *rdev)
+{
+	struct regulator_group *group = rdev_get_drvdata(rdev);
+	int i, r, err = 0;
+
+	/* If no ordering is needed just use bulk disable */
+	if (!group->ordered)
+		return regulator_bulk_disable(
+			group->supply_count, group->supply);
+
+	/* Otherwise do it ourself */
+	for (i = group->supply_count - 1; i >= 0; i--)
+		regulator_disable(group->supply[i].consumer);
+
+	/* Rollback in case of error */
+	if (err)
+		for (i++; i < group->supply_count; i++) {
+			r = regulator_enable(group->supply[i].consumer);
+			if (r)
+				dev_err(&rdev->dev,
+					"Failed to reenable suplly %s: %d\n",
+					group->supply[i].supply, r);
+		}
+
+	return err;
+}
+
+static int regulator_group_is_enabled(struct regulator_dev *rdev)
+{
+	return rdev->use_count > 0;
+}
+
+static struct regulator_ops regulator_group_ops = {
+	.enable		= regulator_group_enable,
+	.disable	= regulator_group_disable,
+	.is_enabled	= regulator_group_is_enabled,
+};
+
+static const struct regulator_desc regulator_group_desc = {
+	.name		= "group",
+	.ops		= &regulator_group_ops,
+	.type		= REGULATOR_VOLTAGE,
+	.owner		= THIS_MODULE,
+};
+
+static int regulator_group_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regulator_config config = {};
+	struct regulator_dev *regulator;
+	struct regulator_group *group;
+	int i, count, err;
+
+	if (!np)
+		return -EINVAL;
+
+	count = of_property_count_strings(np, "regulator-supplies");
+	if (count < 0)
+		return count;
+
+	config.init_data = of_get_regulator_init_data(&pdev->dev, np);
+	if (!config.init_data)
+		return -ENOMEM;
+
+	group = devm_kzalloc(&pdev->dev,
+		sizeof(*group) + count * sizeof(*group->supply), GFP_KERNEL);
+	if (!group)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		err = of_property_read_string_index(
+			np, "regulator-supplies", i,
+			&group->supply[i].supply);
+		if (err)
+			return err;
+		group->supply_count++;
+	}
+
+	group->ordered = of_property_read_bool(np, "ordered-supplies");
+
+	err = devm_regulator_bulk_get(
+		&pdev->dev, group->supply_count, group->supply);
+	if (err)
+		return err;
+
+	config.of_node = np;
+	config.dev = &pdev->dev;
+	config.driver_data = group;
+
+	regulator = devm_regulator_register(
+		&pdev->dev, &regulator_group_desc, &config);
+	return IS_ERR(regulator) ? PTR_ERR(regulator) : 0;
+}
+
+static const struct of_device_id regulator_group_of_match[] = {
+	{ .compatible = "regulator-group" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, regulator_group_of_match);
+
+static struct platform_driver regulator_group_driver = {
+	.driver = {
+		.name		= "group-regulator",
+		.owner		= THIS_MODULE,
+		.of_match_table = of_match_ptr(regulator_group_of_match),
+	},
+	.probe = regulator_group_probe,
+};
+module_platform_driver(regulator_group_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alban Bedel <alban.bedel@avionic-design.de>");
+MODULE_DESCRIPTION("Regulator Group Driver");
+MODULE_ALIAS("platform:group-regulator");
-- 
2.1.3

Re: [PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Mark Brown]

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

On Mon, Nov 24, 2014 at 02:02:00PM +0100, Alban Bedel wrote:

> +Constrained Supply Regulator
> +
> +This binding allow creating a virtual regulator that constrain its
> +supply to provides the requested voltage. This is to allow using
> +simple drivers that don't explicitly request a voltage on boards
> +that have adjustable hardware regulators.
> +
> +Required properties:
> +- compatible : Must be "regulator-constrained-supply".
> +- vin-supply : phandle to the parent supply/regulator node

Reading this description I'm at a loss to explain what this is intended
to achieve - it's obviously not describing hardware which is a rather
large alarm bell and I don't really understand why the constraints
wouldn't be set on the parent regulator.

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

Re: [PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Mark Brown]

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

On Mon, Nov 24, 2014 at 02:02:00PM +0100, Alban Bedel wrote:

> +Constrained Supply Regulator
> +
> +This binding allow creating a virtual regulator that constrain its
> +supply to provides the requested voltage. This is to allow using
> +simple drivers that don't explicitly request a voltage on boards
> +that have adjustable hardware regulators.
> +
> +Required properties:
> +- compatible : Must be "regulator-constrained-supply".
> +- vin-supply : phandle to the parent supply/regulator node

Reading this description I'm at a loss to explain what this is intended
to achieve - it's obviously not describing hardware which is a rather
large alarm bell and I don't really understand why the constraints
wouldn't be set on the parent regulator.

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

Re: [PATCH 3/4] devicetree: add a binding for a group of regulator

[Mark Brown]

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

On Mon, Nov 24, 2014 at 02:02:02PM +0100, Alban Bedel wrote:

> +This binding allow creating a group of regulators for use with simple
> +drivers that only expect a single power supply. Additionally it is
> +possible to enforce the enable ordering to create simple power up
> +sequences.

Absoutely not, this sort of scripting is not sensible - if the consumer
device has multiple supplies the consumer device should be working with
them independently and if the consumer has ordering constraints it needs
to enforce them itself.  Trying to solve this problem with a bodge in
the regulator API just isn't the right place, leaving aside the above
most power sequences involve things other than regulators like clocks
and reset signals so just doing things purely at the regulator API level
isn't ging to solve the problem.  

Please look for the generic power sequence stuff that was getting
discussed a while back and try to resurrect that if you feel there's a
compelling reason to have this functionality without doing it for
drivers.

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

Re: [PATCH 3/4] devicetree: add a binding for a group of regulator

[Mark Brown]

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

On Mon, Nov 24, 2014 at 02:02:02PM +0100, Alban Bedel wrote:

> +This binding allow creating a group of regulators for use with simple
> +drivers that only expect a single power supply. Additionally it is
> +possible to enforce the enable ordering to create simple power up
> +sequences.

Absoutely not, this sort of scripting is not sensible - if the consumer
device has multiple supplies the consumer device should be working with
them independently and if the consumer has ordering constraints it needs
to enforce them itself.  Trying to solve this problem with a bodge in
the regulator API just isn't the right place, leaving aside the above
most power sequences involve things other than regulators like clocks
and reset signals so just doing things purely at the regulator API level
isn't ging to solve the problem.  

Please look for the generic power sequence stuff that was getting
discussed a while back and try to resurrect that if you feel there's a
compelling reason to have this functionality without doing it for
drivers.

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

Re: [PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Alban Bedel]

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

On Mon, 24 Nov 2014 15:15:27 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Nov 24, 2014 at 02:02:00PM +0100, Alban Bedel wrote:
> 
> > +Constrained Supply Regulator
> > +
> > +This binding allow creating a virtual regulator that constrain its
> > +supply to provides the requested voltage. This is to allow using
> > +simple drivers that don't explicitly request a voltage on boards
> > +that have adjustable hardware regulators.
> > +
> > +Required properties:
> > +- compatible : Must be "regulator-constrained-supply".
> > +- vin-supply : phandle to the parent supply/regulator node
> 
> Reading this description I'm at a loss to explain what this is intended
> to achieve - it's obviously not describing hardware which is a rather
> large alarm bell and I don't really understand why the constraints
> wouldn't be set on the parent regulator.

Yes, it is not real hardware. The use case is a generic base board with
one regulator for add-on boards that has a settable voltage output. In
the base board DTS the regulator constraints represent the whole range
that the regulator is capable of, for example 1V to 12V.

Using this driver the DTS for the add-on board can further constrain
this supply to get the voltage it need, for example 3.3V. I understand
that normally the drivers for the consumers on the add-on board should
do this, however I don't really see how that is possible with generic
drivers like simple-panel. Such driver target a large range of
hardware, so just setting an  arbitrary voltage doesn't make much sense.
Furthermore even driver that target a specific hardware usually don't
set a voltage and in many case they couldn't do it without some
knowledge of the board design.

I saw 3 ways to solve this problem:

1) This, use a "filter" regulator
2) Extend each and every driver that use a voltage regulator to
   optionally set the voltage
3) Include the required range in the phandle reference

#1 was the easiest :) #2 doesn't seems practical to me, #3 seems pretty
good but I'm really not sure if that would be acceptable.

Alban

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

Re: [PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Alban Bedel]

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

On Mon, 24 Nov 2014 15:15:27 +0000
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Mon, Nov 24, 2014 at 02:02:00PM +0100, Alban Bedel wrote:
> 
> > +Constrained Supply Regulator
> > +
> > +This binding allow creating a virtual regulator that constrain its
> > +supply to provides the requested voltage. This is to allow using
> > +simple drivers that don't explicitly request a voltage on boards
> > +that have adjustable hardware regulators.
> > +
> > +Required properties:
> > +- compatible : Must be "regulator-constrained-supply".
> > +- vin-supply : phandle to the parent supply/regulator node
> 
> Reading this description I'm at a loss to explain what this is intended
> to achieve - it's obviously not describing hardware which is a rather
> large alarm bell and I don't really understand why the constraints
> wouldn't be set on the parent regulator.

Yes, it is not real hardware. The use case is a generic base board with
one regulator for add-on boards that has a settable voltage output. In
the base board DTS the regulator constraints represent the whole range
that the regulator is capable of, for example 1V to 12V.

Using this driver the DTS for the add-on board can further constrain
this supply to get the voltage it need, for example 3.3V. I understand
that normally the drivers for the consumers on the add-on board should
do this, however I don't really see how that is possible with generic
drivers like simple-panel. Such driver target a large range of
hardware, so just setting an  arbitrary voltage doesn't make much sense.
Furthermore even driver that target a specific hardware usually don't
set a voltage and in many case they couldn't do it without some
knowledge of the board design.

I saw 3 ways to solve this problem:

1) This, use a "filter" regulator
2) Extend each and every driver that use a voltage regulator to
   optionally set the voltage
3) Include the required range in the phandle reference

#1 was the easiest :) #2 doesn't seems practical to me, #3 seems pretty
good but I'm really not sure if that would be acceptable.

Alban

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

Re: [PATCH 3/4] devicetree: add a binding for a group of regulator

[Alban Bedel]

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

On Mon, 24 Nov 2014 15:24:33 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Nov 24, 2014 at 02:02:02PM +0100, Alban Bedel wrote:
> 
> > +This binding allow creating a group of regulators for use with simple
> > +drivers that only expect a single power supply. Additionally it is
> > +possible to enforce the enable ordering to create simple power up
> > +sequences.
> 
> Absoutely not, this sort of scripting is not sensible - if the consumer
> device has multiple supplies the consumer device should be working with
> them independently and if the consumer has ordering constraints it needs
> to enforce them itself.  Trying to solve this problem with a bodge in
> the regulator API just isn't the right place, leaving aside the above
> most power sequences involve things other than regulators like clocks
> and reset signals so just doing things purely at the regulator API level
> isn't ging to solve the problem.  
> 
> Please look for the generic power sequence stuff that was getting
> discussed a while back and try to resurrect that if you feel there's a
> compelling reason to have this functionality without doing it for
> drivers.

Honestly my primary aim wasn't the sequencing, but rather to increase
the usefulness of generic drivers. Generic driver generally only
manipulate a single supply, however many hardware might have more,
and won't need any specific power up ordering. Having to write a full
new driver just because of an extra supply doesn't seems to make much
sense to me.

As alternative solution to this problem I though about allowing a list
of regulator for the supplies:

 vin-supply = <&reg1>, <&reg2>;

The API could still return a single consumer but it would operate on
all the regulators in the list instead of just one. Would that be a
better solution?

Alban

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

Re: [PATCH 3/4] devicetree: add a binding for a group of regulator

[Alban Bedel]

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

On Mon, 24 Nov 2014 15:24:33 +0000
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Mon, Nov 24, 2014 at 02:02:02PM +0100, Alban Bedel wrote:
> 
> > +This binding allow creating a group of regulators for use with simple
> > +drivers that only expect a single power supply. Additionally it is
> > +possible to enforce the enable ordering to create simple power up
> > +sequences.
> 
> Absoutely not, this sort of scripting is not sensible - if the consumer
> device has multiple supplies the consumer device should be working with
> them independently and if the consumer has ordering constraints it needs
> to enforce them itself.  Trying to solve this problem with a bodge in
> the regulator API just isn't the right place, leaving aside the above
> most power sequences involve things other than regulators like clocks
> and reset signals so just doing things purely at the regulator API level
> isn't ging to solve the problem.  
> 
> Please look for the generic power sequence stuff that was getting
> discussed a while back and try to resurrect that if you feel there's a
> compelling reason to have this functionality without doing it for
> drivers.

Honestly my primary aim wasn't the sequencing, but rather to increase
the usefulness of generic drivers. Generic driver generally only
manipulate a single supply, however many hardware might have more,
and won't need any specific power up ordering. Having to write a full
new driver just because of an extra supply doesn't seems to make much
sense to me.

As alternative solution to this problem I though about allowing a list
of regulator for the supplies:

 vin-supply = <&reg1>, <&reg2>;

The API could still return a single consumer but it would operate on
all the regulators in the list instead of just one. Would that be a
better solution?

Alban

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

Re: [PATCH 3/4] devicetree: add a binding for a group of regulator

[Mark Brown]

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

On Mon, Nov 24, 2014 at 06:32:04PM +0100, Alban Bedel wrote:

> Honestly my primary aim wasn't the sequencing, but rather to increase
> the usefulness of generic drivers. Generic driver generally only
> manipulate a single supply, however many hardware might have more,
> and won't need any specific power up ordering. Having to write a full
> new driver just because of an extra supply doesn't seems to make much
> sense to me.

I'm having a really hard time following the above - you say "generic
driver generally only manipulate a single supply" but that's absolutely
not the case.  A driver should control exactly as many supplies as the
device it is controlling does.  

The nearest I can get to something that I think I can understand is a
device variant that has some changes in supplies but varaints aren't
something that we need to write entirely new drivers for, just new
device IDs and a few lines of conditional code.

It's possible that I'm missing something but I'm really struggling to
see the problem that you're trying to solve here or why this is an
abstraction that makes sense.

> As alternative solution to this problem I though about allowing a list
> of regulator for the supplies:

>  vin-supply = <&reg1>, <&reg2>;

> The API could still return a single consumer but it would operate on
> all the regulators in the list instead of just one. Would that be a
> better solution?

No, that's even worse - this is just hacking around whatever problem
you're facing.  The device tree should accurately describe the hardware
not some random thing that vaguely looks like the hardware because it
happens to let us shoehorn things onto it.

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

Re: [PATCH 3/4] devicetree: add a binding for a group of regulator

[Mark Brown]

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

On Mon, Nov 24, 2014 at 06:32:04PM +0100, Alban Bedel wrote:

> Honestly my primary aim wasn't the sequencing, but rather to increase
> the usefulness of generic drivers. Generic driver generally only
> manipulate a single supply, however many hardware might have more,
> and won't need any specific power up ordering. Having to write a full
> new driver just because of an extra supply doesn't seems to make much
> sense to me.

I'm having a really hard time following the above - you say "generic
driver generally only manipulate a single supply" but that's absolutely
not the case.  A driver should control exactly as many supplies as the
device it is controlling does.  

The nearest I can get to something that I think I can understand is a
device variant that has some changes in supplies but varaints aren't
something that we need to write entirely new drivers for, just new
device IDs and a few lines of conditional code.

It's possible that I'm missing something but I'm really struggling to
see the problem that you're trying to solve here or why this is an
abstraction that makes sense.

> As alternative solution to this problem I though about allowing a list
> of regulator for the supplies:

>  vin-supply = <&reg1>, <&reg2>;

> The API could still return a single consumer but it would operate on
> all the regulators in the list instead of just one. Would that be a
> better solution?

No, that's even worse - this is just hacking around whatever problem
you're facing.  The device tree should accurately describe the hardware
not some random thing that vaguely looks like the hardware because it
happens to let us shoehorn things onto it.

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

Re: [PATCH 1/4] devicetree: add a binding for a regulator that constrains its supply

[Mark Brown]

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

On Mon, Nov 24, 2014 at 06:12:33PM +0100, Alban Bedel wrote:

> Using this driver the DTS for the add-on board can further constrain
> this supply to get the voltage it need, for example 3.3V. I understand

> I saw 3 ways to solve this problem:

> 1) This, use a "filter" regulator
> 2) Extend each and every driver that use a voltage regulator to
>    optionally set the voltage
> 3) Include the required range in the phandle reference

> #1 was the easiest :) #2 doesn't seems practical to me, #3 seems pretty
> good but I'm really not sure if that would be acceptable.

The DT overlay for the add on board should be setting the constraints
for the parent regulator.  I wouldn't expect the final DT to look any
different to how it looks with no hardware modularity unless the actual
module is interesting enough to have a driver of its own.

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