[PATCH 0/3] regulator: Add DT support for regulator-output connectors

[PATCH 0/3] regulator: Add DT support for regulator-output connectors

[Zev Weiss]

Hello,

This series is another attempt at implementing support for
userspace-controlled regulator-supplied power outputs.  This is an
important feature for some kinds of BMC (baseboard management
controller) systems where the BMC provides an operator with manual
control of a set of DC power outputs.

As in a broadly similar patchset that was recently almost merged [0],
this takes the approach of providing support by extending the existing
userspace-consumer regulator driver.  A couple questions about the
userspace-consumer driver came up along the way, however.

First, how (if at all) is it currently being used?  It appears the
last in-tree use of it was removed a bit over two years ago in commit
9d3239147d6d ("ARM: pxa: remove Compulab pxa2xx boards").  Aside from
just adding DT support I've made a couple small tweaks to the driver
in patch 3 that I hope are compatible with any current usage, but
without any extant examples to look at it's kind of hard to say.

Second, how critical is its support for controlling multiple
regulators?  (i.e. its use of regulator_bulk_data and friends instead
of a single struct regulator.)  As far as I can see every in-tree use
of it that's ever existed has used num_supplies = 1.  If it's not
important to retain, patch 1 of this series could be supplanted by one
that instead simplifies the driver slightly by removing that
functionality.

The DT binding added in patch 2 is very similar to one I posted in a
previous patchset that had an R-B from Rob [1], but has had some minor
rewording and gained one new (optional) property.

Laxman, Naresh, Patrick -- please let me know if there are any aspects
of this implementation that would be incompatible with your needs.


Thanks,
Zev

[0] https://lore.kernel.org/all/20220707081826.953449-4-Naresh.Solanki@9elements.com/
[1] https://lore.kernel.org/linux-kernel/20220505232557.10936-2-zev@bewilderbeest.net/

Zev Weiss (3):
  regulator: devres: Add devm_regulator_bulk_get_exclusive()
  dt-bindings: regulator: Add regulator-output binding
  regulator: userspace-consumer: Handle regulator-output DT nodes

 .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++
 drivers/regulator/core.c                      | 42 +++++++-----
 drivers/regulator/devres.c                    | 66 ++++++++++++++-----
 drivers/regulator/internal.h                  |  2 +
 drivers/regulator/userspace-consumer.c        | 43 ++++++++++--
 include/linux/regulator/consumer.h            |  2 +
 include/linux/regulator/userspace-consumer.h  |  1 +
 7 files changed, 162 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml

-- 
2.37.3

[PATCH 0/3] regulator: Add DT support for regulator-output connectors

[Zev Weiss]

Hello,

This series is another attempt at implementing support for
userspace-controlled regulator-supplied power outputs.  This is an
important feature for some kinds of BMC (baseboard management
controller) systems where the BMC provides an operator with manual
control of a set of DC power outputs.

As in a broadly similar patchset that was recently almost merged [0],
this takes the approach of providing support by extending the existing
userspace-consumer regulator driver.  A couple questions about the
userspace-consumer driver came up along the way, however.

First, how (if at all) is it currently being used?  It appears the
last in-tree use of it was removed a bit over two years ago in commit
9d3239147d6d ("ARM: pxa: remove Compulab pxa2xx boards").  Aside from
just adding DT support I've made a couple small tweaks to the driver
in patch 3 that I hope are compatible with any current usage, but
without any extant examples to look at it's kind of hard to say.

Second, how critical is its support for controlling multiple
regulators?  (i.e. its use of regulator_bulk_data and friends instead
of a single struct regulator.)  As far as I can see every in-tree use
of it that's ever existed has used num_supplies = 1.  If it's not
important to retain, patch 1 of this series could be supplanted by one
that instead simplifies the driver slightly by removing that
functionality.

The DT binding added in patch 2 is very similar to one I posted in a
previous patchset that had an R-B from Rob [1], but has had some minor
rewording and gained one new (optional) property.

Laxman, Naresh, Patrick -- please let me know if there are any aspects
of this implementation that would be incompatible with your needs.


Thanks,
Zev

[0] https://lore.kernel.org/all/20220707081826.953449-4-Naresh.Solanki@9elements.com/
[1] https://lore.kernel.org/linux-kernel/20220505232557.10936-2-zev@bewilderbeest.net/

Zev Weiss (3):
  regulator: devres: Add devm_regulator_bulk_get_exclusive()
  dt-bindings: regulator: Add regulator-output binding
  regulator: userspace-consumer: Handle regulator-output DT nodes

 .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++
 drivers/regulator/core.c                      | 42 +++++++-----
 drivers/regulator/devres.c                    | 66 ++++++++++++++-----
 drivers/regulator/internal.h                  |  2 +
 drivers/regulator/userspace-consumer.c        | 43 ++++++++++--
 include/linux/regulator/consumer.h            |  2 +
 include/linux/regulator/userspace-consumer.h  |  1 +
 7 files changed, 162 insertions(+), 41 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml

-- 
2.37.3

[PATCH 1/3] regulator: devres: Add devm_regulator_bulk_get_exclusive()

[Zev Weiss]

We had an exclusive variant of the devm_regulator_get() API, but no
corresponding variant for the bulk API; let's add one now.  We add a
generalized version of the existing regulator_bulk_get() function that
additionally takes a get_type parameter and redefine
regulator_bulk_get() in terms of it, then do similarly with
devm_regulator_bulk_get(), and finally add the new
devm_regulator_bulk_get_exclusive().

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/regulator/core.c           | 42 +++++++++++--------
 drivers/regulator/devres.c         | 66 ++++++++++++++++++++++--------
 drivers/regulator/internal.h       |  2 +
 include/linux/regulator/consumer.h |  2 +
 4 files changed, 76 insertions(+), 36 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d3e8dc32832d..1988859d8d02 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4762,22 +4762,8 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
 	return blocking_notifier_call_chain(&rdev->notifier, event, data);
 }
 
-/**
- * regulator_bulk_get - get multiple regulator consumers
- *
- * @dev:           Device to supply
- * @num_consumers: Number of consumers to register
- * @consumers:     Configuration of consumers; clients are stored here.
- *
- * @return 0 on success, an errno on failure.
- *
- * This helper function allows drivers to get several regulator
- * consumers in one operation.  If any of the regulators cannot be
- * acquired then any regulators that were allocated will be freed
- * before returning to the caller.
- */
-int regulator_bulk_get(struct device *dev, int num_consumers,
-		       struct regulator_bulk_data *consumers)
+int _regulator_bulk_get(struct device *dev, int num_consumers,
+			struct regulator_bulk_data *consumers, enum regulator_get_type get_type)
 {
 	int i;
 	int ret;
@@ -4786,8 +4772,8 @@ int regulator_bulk_get(struct device *dev, int num_consumers,
 		consumers[i].consumer = NULL;
 
 	for (i = 0; i < num_consumers; i++) {
-		consumers[i].consumer = regulator_get(dev,
-						      consumers[i].supply);
+		consumers[i].consumer = _regulator_get(dev,
+						       consumers[i].supply, get_type);
 		if (IS_ERR(consumers[i].consumer)) {
 			ret = dev_err_probe(dev, PTR_ERR(consumers[i].consumer),
 					    "Failed to get supply '%s'",
@@ -4814,6 +4800,26 @@ int regulator_bulk_get(struct device *dev, int num_consumers,
 
 	return ret;
 }
+
+/**
+ * regulator_bulk_get - get multiple regulator consumers
+ *
+ * @dev:           Device to supply
+ * @num_consumers: Number of consumers to register
+ * @consumers:     Configuration of consumers; clients are stored here.
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation.  If any of the regulators cannot be
+ * acquired then any regulators that were allocated will be freed
+ * before returning to the caller.
+ */
+int regulator_bulk_get(struct device *dev, int num_consumers,
+		       struct regulator_bulk_data *consumers)
+{
+	return _regulator_bulk_get(dev, num_consumers, consumers, NORMAL_GET);
+}
 EXPORT_SYMBOL_GPL(regulator_bulk_get);
 
 static void regulator_bulk_enable_async(void *data, async_cookie_t cookie)
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 32823a87fd40..e32344829b4f 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -127,23 +127,9 @@ static void devm_regulator_bulk_release(struct device *dev, void *res)
 	regulator_bulk_free(devres->num_consumers, devres->consumers);
 }
 
-/**
- * devm_regulator_bulk_get - managed get multiple regulator consumers
- *
- * @dev:           device to supply
- * @num_consumers: number of consumers to register
- * @consumers:     configuration of consumers; clients are stored here.
- *
- * @return 0 on success, an errno on failure.
- *
- * This helper function allows drivers to get several regulator
- * consumers in one operation with management, the regulators will
- * automatically be freed when the device is unbound.  If any of the
- * regulators cannot be acquired then any regulators that were
- * allocated will be freed before returning to the caller.
- */
-int devm_regulator_bulk_get(struct device *dev, int num_consumers,
-			    struct regulator_bulk_data *consumers)
+static int _devm_regulator_bulk_get(struct device *dev, int num_consumers,
+				    struct regulator_bulk_data *consumers,
+				    enum regulator_get_type get_type)
 {
 	struct regulator_bulk_devres *devres;
 	int ret;
@@ -153,7 +139,7 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 	if (!devres)
 		return -ENOMEM;
 
-	ret = regulator_bulk_get(dev, num_consumers, consumers);
+	ret = _regulator_bulk_get(dev, num_consumers, consumers, get_type);
 	if (!ret) {
 		devres->consumers = consumers;
 		devres->num_consumers = num_consumers;
@@ -164,8 +150,52 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 
 	return ret;
 }
+
+/**
+ * devm_regulator_bulk_get - managed get multiple regulator consumers
+ *
+ * @dev:           device to supply
+ * @num_consumers: number of consumers to register
+ * @consumers:     configuration of consumers; clients are stored here.
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation with management, the regulators will
+ * automatically be freed when the device is unbound.  If any of the
+ * regulators cannot be acquired then any regulators that were
+ * allocated will be freed before returning to the caller.
+ */
+int devm_regulator_bulk_get(struct device *dev, int num_consumers,
+			    struct regulator_bulk_data *consumers)
+{
+	return _devm_regulator_bulk_get(dev, num_consumers, consumers, NORMAL_GET);
+}
 EXPORT_SYMBOL_GPL(devm_regulator_bulk_get);
 
+/**
+ * devm_regulator_bulk_get_exclusive - managed exclusive get of multiple
+ * regulator consumers
+ *
+ * @dev:           device to supply
+ * @num_consumers: number of consumers to register
+ * @consumers:     configuration of consumers; clients are stored here.
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to exclusively get several
+ * regulator consumers in one operation with management, the regulators
+ * will automatically be freed when the device is unbound.  If any of
+ * the regulators cannot be acquired then any regulators that were
+ * allocated will be freed before returning to the caller.
+ */
+int devm_regulator_bulk_get_exclusive(struct device *dev, int num_consumers,
+				      struct regulator_bulk_data *consumers)
+{
+	return _devm_regulator_bulk_get(dev, num_consumers, consumers, EXCLUSIVE_GET);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_get_exclusive);
+
 /**
  * devm_regulator_bulk_get_const - devm_regulator_bulk_get() w/ const data
  *
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 1e9c71642143..fb4433068d29 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -122,4 +122,6 @@ enum regulator_get_type {
 
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
+int _regulator_bulk_get(struct device *dev, int num_consumers,
+			struct regulator_bulk_data *consumers, enum regulator_get_type get_type);
 #endif
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index bc6cda706d1f..e123cdc91d24 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -244,6 +244,8 @@ int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
 				    struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get(struct device *dev, int num_consumers,
 					 struct regulator_bulk_data *consumers);
+int __must_check devm_regulator_bulk_get_exclusive(struct device *dev, int num_consumers,
+						   struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get_const(
 	struct device *dev, int num_consumers,
 	const struct regulator_bulk_data *in_consumers,
-- 
2.37.3

[PATCH 1/3] regulator: devres: Add devm_regulator_bulk_get_exclusive()

[Zev Weiss]

We had an exclusive variant of the devm_regulator_get() API, but no
corresponding variant for the bulk API; let's add one now.  We add a
generalized version of the existing regulator_bulk_get() function that
additionally takes a get_type parameter and redefine
regulator_bulk_get() in terms of it, then do similarly with
devm_regulator_bulk_get(), and finally add the new
devm_regulator_bulk_get_exclusive().

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/regulator/core.c           | 42 +++++++++++--------
 drivers/regulator/devres.c         | 66 ++++++++++++++++++++++--------
 drivers/regulator/internal.h       |  2 +
 include/linux/regulator/consumer.h |  2 +
 4 files changed, 76 insertions(+), 36 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d3e8dc32832d..1988859d8d02 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4762,22 +4762,8 @@ static int _notifier_call_chain(struct regulator_dev *rdev,
 	return blocking_notifier_call_chain(&rdev->notifier, event, data);
 }
 
-/**
- * regulator_bulk_get - get multiple regulator consumers
- *
- * @dev:           Device to supply
- * @num_consumers: Number of consumers to register
- * @consumers:     Configuration of consumers; clients are stored here.
- *
- * @return 0 on success, an errno on failure.
- *
- * This helper function allows drivers to get several regulator
- * consumers in one operation.  If any of the regulators cannot be
- * acquired then any regulators that were allocated will be freed
- * before returning to the caller.
- */
-int regulator_bulk_get(struct device *dev, int num_consumers,
-		       struct regulator_bulk_data *consumers)
+int _regulator_bulk_get(struct device *dev, int num_consumers,
+			struct regulator_bulk_data *consumers, enum regulator_get_type get_type)
 {
 	int i;
 	int ret;
@@ -4786,8 +4772,8 @@ int regulator_bulk_get(struct device *dev, int num_consumers,
 		consumers[i].consumer = NULL;
 
 	for (i = 0; i < num_consumers; i++) {
-		consumers[i].consumer = regulator_get(dev,
-						      consumers[i].supply);
+		consumers[i].consumer = _regulator_get(dev,
+						       consumers[i].supply, get_type);
 		if (IS_ERR(consumers[i].consumer)) {
 			ret = dev_err_probe(dev, PTR_ERR(consumers[i].consumer),
 					    "Failed to get supply '%s'",
@@ -4814,6 +4800,26 @@ int regulator_bulk_get(struct device *dev, int num_consumers,
 
 	return ret;
 }
+
+/**
+ * regulator_bulk_get - get multiple regulator consumers
+ *
+ * @dev:           Device to supply
+ * @num_consumers: Number of consumers to register
+ * @consumers:     Configuration of consumers; clients are stored here.
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation.  If any of the regulators cannot be
+ * acquired then any regulators that were allocated will be freed
+ * before returning to the caller.
+ */
+int regulator_bulk_get(struct device *dev, int num_consumers,
+		       struct regulator_bulk_data *consumers)
+{
+	return _regulator_bulk_get(dev, num_consumers, consumers, NORMAL_GET);
+}
 EXPORT_SYMBOL_GPL(regulator_bulk_get);
 
 static void regulator_bulk_enable_async(void *data, async_cookie_t cookie)
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 32823a87fd40..e32344829b4f 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -127,23 +127,9 @@ static void devm_regulator_bulk_release(struct device *dev, void *res)
 	regulator_bulk_free(devres->num_consumers, devres->consumers);
 }
 
-/**
- * devm_regulator_bulk_get - managed get multiple regulator consumers
- *
- * @dev:           device to supply
- * @num_consumers: number of consumers to register
- * @consumers:     configuration of consumers; clients are stored here.
- *
- * @return 0 on success, an errno on failure.
- *
- * This helper function allows drivers to get several regulator
- * consumers in one operation with management, the regulators will
- * automatically be freed when the device is unbound.  If any of the
- * regulators cannot be acquired then any regulators that were
- * allocated will be freed before returning to the caller.
- */
-int devm_regulator_bulk_get(struct device *dev, int num_consumers,
-			    struct regulator_bulk_data *consumers)
+static int _devm_regulator_bulk_get(struct device *dev, int num_consumers,
+				    struct regulator_bulk_data *consumers,
+				    enum regulator_get_type get_type)
 {
 	struct regulator_bulk_devres *devres;
 	int ret;
@@ -153,7 +139,7 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 	if (!devres)
 		return -ENOMEM;
 
-	ret = regulator_bulk_get(dev, num_consumers, consumers);
+	ret = _regulator_bulk_get(dev, num_consumers, consumers, get_type);
 	if (!ret) {
 		devres->consumers = consumers;
 		devres->num_consumers = num_consumers;
@@ -164,8 +150,52 @@ int devm_regulator_bulk_get(struct device *dev, int num_consumers,
 
 	return ret;
 }
+
+/**
+ * devm_regulator_bulk_get - managed get multiple regulator consumers
+ *
+ * @dev:           device to supply
+ * @num_consumers: number of consumers to register
+ * @consumers:     configuration of consumers; clients are stored here.
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to get several regulator
+ * consumers in one operation with management, the regulators will
+ * automatically be freed when the device is unbound.  If any of the
+ * regulators cannot be acquired then any regulators that were
+ * allocated will be freed before returning to the caller.
+ */
+int devm_regulator_bulk_get(struct device *dev, int num_consumers,
+			    struct regulator_bulk_data *consumers)
+{
+	return _devm_regulator_bulk_get(dev, num_consumers, consumers, NORMAL_GET);
+}
 EXPORT_SYMBOL_GPL(devm_regulator_bulk_get);
 
+/**
+ * devm_regulator_bulk_get_exclusive - managed exclusive get of multiple
+ * regulator consumers
+ *
+ * @dev:           device to supply
+ * @num_consumers: number of consumers to register
+ * @consumers:     configuration of consumers; clients are stored here.
+ *
+ * @return 0 on success, an errno on failure.
+ *
+ * This helper function allows drivers to exclusively get several
+ * regulator consumers in one operation with management, the regulators
+ * will automatically be freed when the device is unbound.  If any of
+ * the regulators cannot be acquired then any regulators that were
+ * allocated will be freed before returning to the caller.
+ */
+int devm_regulator_bulk_get_exclusive(struct device *dev, int num_consumers,
+				      struct regulator_bulk_data *consumers)
+{
+	return _devm_regulator_bulk_get(dev, num_consumers, consumers, EXCLUSIVE_GET);
+}
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_get_exclusive);
+
 /**
  * devm_regulator_bulk_get_const - devm_regulator_bulk_get() w/ const data
  *
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 1e9c71642143..fb4433068d29 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -122,4 +122,6 @@ enum regulator_get_type {
 
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
+int _regulator_bulk_get(struct device *dev, int num_consumers,
+			struct regulator_bulk_data *consumers, enum regulator_get_type get_type);
 #endif
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index bc6cda706d1f..e123cdc91d24 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -244,6 +244,8 @@ int __must_check regulator_bulk_get(struct device *dev, int num_consumers,
 				    struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get(struct device *dev, int num_consumers,
 					 struct regulator_bulk_data *consumers);
+int __must_check devm_regulator_bulk_get_exclusive(struct device *dev, int num_consumers,
+						   struct regulator_bulk_data *consumers);
 int __must_check devm_regulator_bulk_get_const(
 	struct device *dev, int num_consumers,
 	const struct regulator_bulk_data *in_consumers,
-- 
2.37.3

[PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Zev Weiss]

This describes a power output supplied by a regulator, such as a
power outlet on a power distribution unit (PDU).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml

diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
new file mode 100644
index 000000000000..40953ec48e9e
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Regulator output connector
+
+maintainers:
+  - Zev Weiss <zev@bewilderbeest.net>
+
+description: |
+  This describes a power output connector supplied by a regulator,
+  such as a power outlet on a power distribution unit (PDU).  The
+  connector may be standalone or merely one channel or set of pins
+  within a ganged physical connector carrying multiple independent
+  power outputs.
+
+properties:
+  compatible:
+    const: regulator-output
+
+  vout-supply:
+    description:
+      Phandle of the regulator supplying the output.
+
+  regulator-leave-on:
+    description: |
+      If the regulator is enabled when software relinquishes control
+      of it (such as when shutting down) it should be left enabled
+      instead of being turned off.
+    type: boolean
+
+required:
+  - compatible
+  - vout-supply
+
+additionalProperties: false
+
+examples:
+  - |
+      output {
+          compatible = "regulator-output";
+          vout-supply = <&output_reg>;
+          regulator-leave-on;
+      };
-- 
2.37.3

[PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Zev Weiss]

This describes a power output supplied by a regulator, such as a
power outlet on a power distribution unit (PDU).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml

diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
new file mode 100644
index 000000000000..40953ec48e9e
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Regulator output connector
+
+maintainers:
+  - Zev Weiss <zev@bewilderbeest.net>
+
+description: |
+  This describes a power output connector supplied by a regulator,
+  such as a power outlet on a power distribution unit (PDU).  The
+  connector may be standalone or merely one channel or set of pins
+  within a ganged physical connector carrying multiple independent
+  power outputs.
+
+properties:
+  compatible:
+    const: regulator-output
+
+  vout-supply:
+    description:
+      Phandle of the regulator supplying the output.
+
+  regulator-leave-on:
+    description: |
+      If the regulator is enabled when software relinquishes control
+      of it (such as when shutting down) it should be left enabled
+      instead of being turned off.
+    type: boolean
+
+required:
+  - compatible
+  - vout-supply
+
+additionalProperties: false
+
+examples:
+  - |
+      output {
+          compatible = "regulator-output";
+          vout-supply = <&output_reg>;
+          regulator-leave-on;
+      };
-- 
2.37.3

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Rob Herring]

On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
> This describes a power output supplied by a regulator, such as a
> power outlet on a power distribution unit (PDU).
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
> new file mode 100644
> index 000000000000..40953ec48e9e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Regulator output connector
> +
> +maintainers:
> +  - Zev Weiss <zev@bewilderbeest.net>
> +
> +description: |
> +  This describes a power output connector supplied by a regulator,
> +  such as a power outlet on a power distribution unit (PDU).  The
> +  connector may be standalone or merely one channel or set of pins
> +  within a ganged physical connector carrying multiple independent
> +  power outputs.
> +
> +properties:
> +  compatible:
> +    const: regulator-output
> +
> +  vout-supply:
> +    description:
> +      Phandle of the regulator supplying the output.
> +
> +  regulator-leave-on:
> +    description: |
> +      If the regulator is enabled when software relinquishes control
> +      of it (such as when shutting down) it should be left enabled
> +      instead of being turned off.
> +    type: boolean

I'm not too sure about this one as there could be various times when 
control is relinquished. It is userspace closing its access? 
driver unbind? module unload? Does a bootloader pay attention to this?

Rob

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Rob Herring]

On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
> This describes a power output supplied by a regulator, such as a
> power outlet on a power distribution unit (PDU).
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
> new file mode 100644
> index 000000000000..40953ec48e9e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Regulator output connector
> +
> +maintainers:
> +  - Zev Weiss <zev@bewilderbeest.net>
> +
> +description: |
> +  This describes a power output connector supplied by a regulator,
> +  such as a power outlet on a power distribution unit (PDU).  The
> +  connector may be standalone or merely one channel or set of pins
> +  within a ganged physical connector carrying multiple independent
> +  power outputs.
> +
> +properties:
> +  compatible:
> +    const: regulator-output
> +
> +  vout-supply:
> +    description:
> +      Phandle of the regulator supplying the output.
> +
> +  regulator-leave-on:
> +    description: |
> +      If the regulator is enabled when software relinquishes control
> +      of it (such as when shutting down) it should be left enabled
> +      instead of being turned off.
> +    type: boolean

I'm not too sure about this one as there could be various times when 
control is relinquished. It is userspace closing its access? 
driver unbind? module unload? Does a bootloader pay attention to this?

Rob

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Zev Weiss]

On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>> This describes a power output supplied by a regulator, such as a
>> power outlet on a power distribution unit (PDU).
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>> new file mode 100644
>> index 000000000000..40953ec48e9e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
>> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Regulator output connector
>> +
>> +maintainers:
>> +  - Zev Weiss <zev@bewilderbeest.net>
>> +
>> +description: |
>> +  This describes a power output connector supplied by a regulator,
>> +  such as a power outlet on a power distribution unit (PDU).  The
>> +  connector may be standalone or merely one channel or set of pins
>> +  within a ganged physical connector carrying multiple independent
>> +  power outputs.
>> +
>> +properties:
>> +  compatible:
>> +    const: regulator-output
>> +
>> +  vout-supply:
>> +    description:
>> +      Phandle of the regulator supplying the output.
>> +
>> +  regulator-leave-on:
>> +    description: |
>> +      If the regulator is enabled when software relinquishes control
>> +      of it (such as when shutting down) it should be left enabled
>> +      instead of being turned off.
>> +    type: boolean
>
>I'm not too sure about this one as there could be various times when
>control is relinquished. It is userspace closing its access?
>driver unbind? module unload? Does a bootloader pay attention to this?
>
>Rob

Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to 
approach that, and this may well not be the right answer.  What I'm 
really aiming for is an appropriate way to express that regulator on/off 
state should only ever be changed by explicit (external, e.g. userspace) 
request, never as any sort of default/automatic action.  The two obvious 
things to guard against there seem to be automatic enablement during 
initialization and automatic disablement on de-init (shutdown, unbind, 
etc.).  The former I think can be avoided by simply not setting 
regulator-boot-on, so I added this as a corresponding property to avoid 
the latter.

I'm definitely open to suggestions for a better approach though.


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Zev Weiss]

On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>> This describes a power output supplied by a regulator, such as a
>> power outlet on a power distribution unit (PDU).
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>> new file mode 100644
>> index 000000000000..40953ec48e9e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
>> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Regulator output connector
>> +
>> +maintainers:
>> +  - Zev Weiss <zev@bewilderbeest.net>
>> +
>> +description: |
>> +  This describes a power output connector supplied by a regulator,
>> +  such as a power outlet on a power distribution unit (PDU).  The
>> +  connector may be standalone or merely one channel or set of pins
>> +  within a ganged physical connector carrying multiple independent
>> +  power outputs.
>> +
>> +properties:
>> +  compatible:
>> +    const: regulator-output
>> +
>> +  vout-supply:
>> +    description:
>> +      Phandle of the regulator supplying the output.
>> +
>> +  regulator-leave-on:
>> +    description: |
>> +      If the regulator is enabled when software relinquishes control
>> +      of it (such as when shutting down) it should be left enabled
>> +      instead of being turned off.
>> +    type: boolean
>
>I'm not too sure about this one as there could be various times when
>control is relinquished. It is userspace closing its access?
>driver unbind? module unload? Does a bootloader pay attention to this?
>
>Rob

Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to 
approach that, and this may well not be the right answer.  What I'm 
really aiming for is an appropriate way to express that regulator on/off 
state should only ever be changed by explicit (external, e.g. userspace) 
request, never as any sort of default/automatic action.  The two obvious 
things to guard against there seem to be automatic enablement during 
initialization and automatic disablement on de-init (shutdown, unbind, 
etc.).  The former I think can be avoided by simply not setting 
regulator-boot-on, so I added this as a corresponding property to avoid 
the latter.

I'm definitely open to suggestions for a better approach though.


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Zev Weiss]

On Thu, Sep 29, 2022 at 02:27:20PM PDT, Zev Weiss wrote:
>On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>>On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>>>This describes a power output supplied by a regulator, such as a
>>>power outlet on a power distribution unit (PDU).
>>>
>>>Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>>---
>>> .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>> 1 file changed, 47 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>
>>>diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>new file mode 100644
>>>index 000000000000..40953ec48e9e
>>>--- /dev/null
>>>+++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>@@ -0,0 +1,47 @@
>>>+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>+%YAML 1.2
>>>+---
>>>+
>>>+$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>>>+$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>+
>>>+title: Regulator output connector
>>>+
>>>+maintainers:
>>>+  - Zev Weiss <zev@bewilderbeest.net>
>>>+
>>>+description: |
>>>+  This describes a power output connector supplied by a regulator,
>>>+  such as a power outlet on a power distribution unit (PDU).  The
>>>+  connector may be standalone or merely one channel or set of pins
>>>+  within a ganged physical connector carrying multiple independent
>>>+  power outputs.
>>>+
>>>+properties:
>>>+  compatible:
>>>+    const: regulator-output
>>>+
>>>+  vout-supply:
>>>+    description:
>>>+      Phandle of the regulator supplying the output.
>>>+
>>>+  regulator-leave-on:
>>>+    description: |
>>>+      If the regulator is enabled when software relinquishes control
>>>+      of it (such as when shutting down) it should be left enabled
>>>+      instead of being turned off.
>>>+    type: boolean
>>
>>I'm not too sure about this one as there could be various times when
>>control is relinquished. It is userspace closing its access?
>>driver unbind? module unload? Does a bootloader pay attention to this?
>>
>>Rob
>
>Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to 
>approach that, and this may well not be the right answer.  What I'm 
>really aiming for is an appropriate way to express that regulator 
>on/off state should only ever be changed by explicit (external, e.g. 
>userspace) request, never as any sort of default/automatic action.  
>The two obvious things to guard against there seem to be automatic 
>enablement during initialization and automatic disablement on de-init 
>(shutdown, unbind, etc.).  The former I think can be avoided by simply 
>not setting regulator-boot-on, so I added this as a corresponding 
>property to avoid the latter.
>
>I'm definitely open to suggestions for a better approach though.
>

Ping...

Would something like this be preferable as a more direct description of 
that?

   regulator-manually-controlled:
     description: |
       The regulator should never be enabled or disabled automatically,
       only when explicitly requested by an external actor (e.g.  
       userspace).
     type: boolean

That would leave the question of which property takes priority if both 
regulator-manually-controlled and regulator-boot-on are set -- should 
the binding document the answer to that?  (I personally don't have a 
strong opinion on which it should be.)

Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Zev Weiss]

On Thu, Sep 29, 2022 at 02:27:20PM PDT, Zev Weiss wrote:
>On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>>On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>>>This describes a power output supplied by a regulator, such as a
>>>power outlet on a power distribution unit (PDU).
>>>
>>>Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>>---
>>> .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>> 1 file changed, 47 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>
>>>diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>new file mode 100644
>>>index 000000000000..40953ec48e9e
>>>--- /dev/null
>>>+++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>@@ -0,0 +1,47 @@
>>>+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>+%YAML 1.2
>>>+---
>>>+
>>>+$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>>>+$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>+
>>>+title: Regulator output connector
>>>+
>>>+maintainers:
>>>+  - Zev Weiss <zev@bewilderbeest.net>
>>>+
>>>+description: |
>>>+  This describes a power output connector supplied by a regulator,
>>>+  such as a power outlet on a power distribution unit (PDU).  The
>>>+  connector may be standalone or merely one channel or set of pins
>>>+  within a ganged physical connector carrying multiple independent
>>>+  power outputs.
>>>+
>>>+properties:
>>>+  compatible:
>>>+    const: regulator-output
>>>+
>>>+  vout-supply:
>>>+    description:
>>>+      Phandle of the regulator supplying the output.
>>>+
>>>+  regulator-leave-on:
>>>+    description: |
>>>+      If the regulator is enabled when software relinquishes control
>>>+      of it (such as when shutting down) it should be left enabled
>>>+      instead of being turned off.
>>>+    type: boolean
>>
>>I'm not too sure about this one as there could be various times when
>>control is relinquished. It is userspace closing its access?
>>driver unbind? module unload? Does a bootloader pay attention to this?
>>
>>Rob
>
>Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to 
>approach that, and this may well not be the right answer.  What I'm 
>really aiming for is an appropriate way to express that regulator 
>on/off state should only ever be changed by explicit (external, e.g. 
>userspace) request, never as any sort of default/automatic action.  
>The two obvious things to guard against there seem to be automatic 
>enablement during initialization and automatic disablement on de-init 
>(shutdown, unbind, etc.).  The former I think can be avoided by simply 
>not setting regulator-boot-on, so I added this as a corresponding 
>property to avoid the latter.
>
>I'm definitely open to suggestions for a better approach though.
>

Ping...

Would something like this be preferable as a more direct description of 
that?

   regulator-manually-controlled:
     description: |
       The regulator should never be enabled or disabled automatically,
       only when explicitly requested by an external actor (e.g.  
       userspace).
     type: boolean

That would leave the question of which property takes priority if both 
regulator-manually-controlled and regulator-boot-on are set -- should 
the binding document the answer to that?  (I personally don't have a 
strong opinion on which it should be.)

Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Krzysztof Kozlowski]

On 27/10/2022 14:42, Zev Weiss wrote:
> On Thu, Sep 29, 2022 at 02:27:20PM PDT, Zev Weiss wrote:
>> On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>>> On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>>>> This describes a power output supplied by a regulator, such as a
>>>> power outlet on a power distribution unit (PDU).
>>>>
>>>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>>> ---
>>>> .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>>> 1 file changed, 47 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>> new file mode 100644
>>>> index 000000000000..40953ec48e9e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>> @@ -0,0 +1,47 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +
>>>> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Regulator output connector
>>>> +
>>>> +maintainers:
>>>> +  - Zev Weiss <zev@bewilderbeest.net>
>>>> +
>>>> +description: |
>>>> +  This describes a power output connector supplied by a regulator,
>>>> +  such as a power outlet on a power distribution unit (PDU).  The
>>>> +  connector may be standalone or merely one channel or set of pins
>>>> +  within a ganged physical connector carrying multiple independent
>>>> +  power outputs.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: regulator-output
>>>> +
>>>> +  vout-supply:
>>>> +    description:
>>>> +      Phandle of the regulator supplying the output.
>>>> +
>>>> +  regulator-leave-on:
>>>> +    description: |
>>>> +      If the regulator is enabled when software relinquishes control
>>>> +      of it (such as when shutting down) it should be left enabled
>>>> +      instead of being turned off.
>>>> +    type: boolean
>>>
>>> I'm not too sure about this one as there could be various times when
>>> control is relinquished. It is userspace closing its access?
>>> driver unbind? module unload? Does a bootloader pay attention to this?
>>>
>>> Rob
>>
>> Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to 
>> approach that, and this may well not be the right answer.  What I'm 
>> really aiming for is an appropriate way to express that regulator 
>> on/off state should only ever be changed by explicit (external, e.g. 
>> userspace) request, never as any sort of default/automatic action.  
>> The two obvious things to guard against there seem to be automatic 
>> enablement during initialization and automatic disablement on de-init 
>> (shutdown, unbind, etc.).  The former I think can be avoided by simply 
>> not setting regulator-boot-on, so I added this as a corresponding 
>> property to avoid the latter.
>>
>> I'm definitely open to suggestions for a better approach though.
>>
> 
> Ping...
> 
> Would something like this be preferable as a more direct description of 
> that?
> 
>    regulator-manually-controlled:
>      description: |
>        The regulator should never be enabled or disabled automatically,
>        only when explicitly requested by an external actor (e.g.  
>        userspace).
>      type: boolean

This looks like putting policy and OS behavior into DT. I guess it easy
to understand in case of Linux which disables unclaimed regulators
during. But what if other system/firmware does not behave like that?

And what is the "external actor"? OS is not an external actor?

I could not get the problem you want to solve with this property - I
looked at cover letter and at commit msg.

I can only imagine that you want to keep regulator on, after last its
user disappears... but for what purpose? Do you expect that after system
shutdown the pin will stay high so regulator will be also on? If so, you
need hardware design, e.g. with some pull up (if control is over GPIO).

> 
> That would leave the question of which property takes priority if both 
> regulator-manually-controlled and regulator-boot-on are set -- should 
> the binding document the answer to that?  (I personally don't have a 
> strong opinion on which it should be.)


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Zev Weiss]

On Thu, Oct 27, 2022 at 06:22:10PM PDT, Krzysztof Kozlowski wrote:
>On 27/10/2022 14:42, Zev Weiss wrote:
>> On Thu, Sep 29, 2022 at 02:27:20PM PDT, Zev Weiss wrote:
>>> On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>>>> On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>>>>> This describes a power output supplied by a regulator, such as a
>>>>> power outlet on a power distribution unit (PDU).
>>>>>
>>>>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>>>> ---
>>>>> .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>>>> 1 file changed, 47 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..40953ec48e9e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>> @@ -0,0 +1,47 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +
>>>>> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Regulator output connector
>>>>> +
>>>>> +maintainers:
>>>>> +  - Zev Weiss <zev@bewilderbeest.net>
>>>>> +
>>>>> +description: |
>>>>> +  This describes a power output connector supplied by a regulator,
>>>>> +  such as a power outlet on a power distribution unit (PDU).  The
>>>>> +  connector may be standalone or merely one channel or set of pins
>>>>> +  within a ganged physical connector carrying multiple independent
>>>>> +  power outputs.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: regulator-output
>>>>> +
>>>>> +  vout-supply:
>>>>> +    description:
>>>>> +      Phandle of the regulator supplying the output.
>>>>> +
>>>>> +  regulator-leave-on:
>>>>> +    description: |
>>>>> +      If the regulator is enabled when software relinquishes control
>>>>> +      of it (such as when shutting down) it should be left enabled
>>>>> +      instead of being turned off.
>>>>> +    type: boolean
>>>>
>>>> I'm not too sure about this one as there could be various times when
>>>> control is relinquished. It is userspace closing its access?
>>>> driver unbind? module unload? Does a bootloader pay attention to this?
>>>>
>>>> Rob
>>>
>>> Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to
>>> approach that, and this may well not be the right answer.  What I'm
>>> really aiming for is an appropriate way to express that regulator
>>> on/off state should only ever be changed by explicit (external, e.g.
>>> userspace) request, never as any sort of default/automatic action.
>>> The two obvious things to guard against there seem to be automatic
>>> enablement during initialization and automatic disablement on de-init
>>> (shutdown, unbind, etc.).  The former I think can be avoided by simply
>>> not setting regulator-boot-on, so I added this as a corresponding
>>> property to avoid the latter.
>>>
>>> I'm definitely open to suggestions for a better approach though.
>>>
>>
>> Ping...
>>
>> Would something like this be preferable as a more direct description of
>> that?
>>
>>    regulator-manually-controlled:
>>      description: |
>>        The regulator should never be enabled or disabled automatically,
>>        only when explicitly requested by an external actor (e.g.
>>        userspace).
>>      type: boolean
>
>This looks like putting policy and OS behavior into DT.

I can see why it might look that way, but I'd argue it's actually not.  
The systems this is intended to support provide power to entirely 
separate external devices -- think of a power distribution unit that 
might have arbitrary things plugged into it.  It seems to me like a 
property of the hardware that those things shouldn't have their power 
supply turned off (or on) just because a controller in the PDU rebooted.

>I guess it easy
>to understand in case of Linux which disables unclaimed regulators
>during. But what if other system/firmware does not behave like that?
>

In this case, then no change would be needed -- a system that (unlike 
Linux) doesn't twiddle regulator state on its own would just continue to 
not do that.

>And what is the "external actor"? OS is not an external actor?

It's admittedly a bit vague, but I couldn't think of a clearer way to 
express what is a sort of nebulous concept -- essentially, some entity 
outside the "driver" (or analogous software component) using the 
information in the device-tree.  In many common cases this would 
essentially mean "a human user", since in the PDU-like systems I'm 
targeting here the only thing that should ever be deciding to turn the 
regulator on or off is an operator logged in to the system to manually 
enable or disable an outlet.  I was aiming to leave the wording a bit 
more general though, since in some other context I could imagine some 
other piece of software toggling things automatedly (e.g. lights getting 
turned on and off on a schedule or something, if that's what happens to 
be plugged in).

>
>I could not get the problem you want to solve with this property - I
>looked at cover letter and at commit msg.
>

The problem is that a driver deciding on its own to enable or disable 
the regulator (e.g. during boot or shutdown) would be a critical failure 
for the kind of systems I'm aiming to support.

>I can only imagine that you want to keep regulator on, after last its
>user disappears... but for what purpose? Do you expect that after system
>shutdown the pin will stay high so regulator will be also on? If so, you
>need hardware design, e.g. with some pull up (if control is over GPIO).
>

As described above, the regulators involved here (in these sorts of 
PDU-like systems) provide power for external systems and devices.  It is 
critical that the controller's boot and shutdown sequences not alter the 
state of the regulator.

If some additional concrete details would help clarify, the particular 
system I'm working on at the moment is the Delta Power AHE-50DC [0].  It 
has two redundant BMCs controlling 50 power outputs, each of which is 
managed by an LM25066 [1] attached to the controllers via I2C.  The 
LM25066s maintain their power state independently of the controllers 
booting or shutting down, and it's very important that if one controller 
reboots (for a firmware update, say) that it not send I2C commands to 
all the LM25066s telling them to turn off their outputs.

[0] https://www.open19.org/marketplace/delta-16kw-power-shelf/
[1] https://www.ti.com/lit/ds/symlink/lm25066.pdf


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Zev Weiss]

On Thu, Oct 27, 2022 at 06:22:10PM PDT, Krzysztof Kozlowski wrote:
>On 27/10/2022 14:42, Zev Weiss wrote:
>> On Thu, Sep 29, 2022 at 02:27:20PM PDT, Zev Weiss wrote:
>>> On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:
>>>> On Sun, Sep 25, 2022 at 03:03:18PM -0700, Zev Weiss wrote:
>>>>> This describes a power output supplied by a regulator, such as a
>>>>> power outlet on a power distribution unit (PDU).
>>>>>
>>>>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>>>> ---
>>>>> .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++++++++
>>>>> 1 file changed, 47 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/regulator/regulator-output.yaml b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..40953ec48e9e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/regulator/regulator-output.yaml
>>>>> @@ -0,0 +1,47 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +
>>>>> +$id: http://devicetree.org/schemas/regulator/regulator-output.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Regulator output connector
>>>>> +
>>>>> +maintainers:
>>>>> +  - Zev Weiss <zev@bewilderbeest.net>
>>>>> +
>>>>> +description: |
>>>>> +  This describes a power output connector supplied by a regulator,
>>>>> +  such as a power outlet on a power distribution unit (PDU).  The
>>>>> +  connector may be standalone or merely one channel or set of pins
>>>>> +  within a ganged physical connector carrying multiple independent
>>>>> +  power outputs.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: regulator-output
>>>>> +
>>>>> +  vout-supply:
>>>>> +    description:
>>>>> +      Phandle of the regulator supplying the output.
>>>>> +
>>>>> +  regulator-leave-on:
>>>>> +    description: |
>>>>> +      If the regulator is enabled when software relinquishes control
>>>>> +      of it (such as when shutting down) it should be left enabled
>>>>> +      instead of being turned off.
>>>>> +    type: boolean
>>>>
>>>> I'm not too sure about this one as there could be various times when
>>>> control is relinquished. It is userspace closing its access?
>>>> driver unbind? module unload? Does a bootloader pay attention to this?
>>>>
>>>> Rob
>>>
>>> Thanks for the feedback, Rob -- I'll admit I was a bit unsure how to
>>> approach that, and this may well not be the right answer.  What I'm
>>> really aiming for is an appropriate way to express that regulator
>>> on/off state should only ever be changed by explicit (external, e.g.
>>> userspace) request, never as any sort of default/automatic action.
>>> The two obvious things to guard against there seem to be automatic
>>> enablement during initialization and automatic disablement on de-init
>>> (shutdown, unbind, etc.).  The former I think can be avoided by simply
>>> not setting regulator-boot-on, so I added this as a corresponding
>>> property to avoid the latter.
>>>
>>> I'm definitely open to suggestions for a better approach though.
>>>
>>
>> Ping...
>>
>> Would something like this be preferable as a more direct description of
>> that?
>>
>>    regulator-manually-controlled:
>>      description: |
>>        The regulator should never be enabled or disabled automatically,
>>        only when explicitly requested by an external actor (e.g.
>>        userspace).
>>      type: boolean
>
>This looks like putting policy and OS behavior into DT.

I can see why it might look that way, but I'd argue it's actually not.  
The systems this is intended to support provide power to entirely 
separate external devices -- think of a power distribution unit that 
might have arbitrary things plugged into it.  It seems to me like a 
property of the hardware that those things shouldn't have their power 
supply turned off (or on) just because a controller in the PDU rebooted.

>I guess it easy
>to understand in case of Linux which disables unclaimed regulators
>during. But what if other system/firmware does not behave like that?
>

In this case, then no change would be needed -- a system that (unlike 
Linux) doesn't twiddle regulator state on its own would just continue to 
not do that.

>And what is the "external actor"? OS is not an external actor?

It's admittedly a bit vague, but I couldn't think of a clearer way to 
express what is a sort of nebulous concept -- essentially, some entity 
outside the "driver" (or analogous software component) using the 
information in the device-tree.  In many common cases this would 
essentially mean "a human user", since in the PDU-like systems I'm 
targeting here the only thing that should ever be deciding to turn the 
regulator on or off is an operator logged in to the system to manually 
enable or disable an outlet.  I was aiming to leave the wording a bit 
more general though, since in some other context I could imagine some 
other piece of software toggling things automatedly (e.g. lights getting 
turned on and off on a schedule or something, if that's what happens to 
be plugged in).

>
>I could not get the problem you want to solve with this property - I
>looked at cover letter and at commit msg.
>

The problem is that a driver deciding on its own to enable or disable 
the regulator (e.g. during boot or shutdown) would be a critical failure 
for the kind of systems I'm aiming to support.

>I can only imagine that you want to keep regulator on, after last its
>user disappears... but for what purpose? Do you expect that after system
>shutdown the pin will stay high so regulator will be also on? If so, you
>need hardware design, e.g. with some pull up (if control is over GPIO).
>

As described above, the regulators involved here (in these sorts of 
PDU-like systems) provide power for external systems and devices.  It is 
critical that the controller's boot and shutdown sequences not alter the 
state of the regulator.

If some additional concrete details would help clarify, the particular 
system I'm working on at the moment is the Delta Power AHE-50DC [0].  It 
has two redundant BMCs controlling 50 power outputs, each of which is 
managed by an LM25066 [1] attached to the controllers via I2C.  The 
LM25066s maintain their power state independently of the controllers 
booting or shutting down, and it's very important that if one controller 
reboots (for a firmware update, say) that it not send I2C commands to 
all the LM25066s telling them to turn off their outputs.

[0] https://www.open19.org/marketplace/delta-16kw-power-shelf/
[1] https://www.ti.com/lit/ds/symlink/lm25066.pdf


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Mark Brown]

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

On Thu, Oct 27, 2022 at 09:12:22PM -0700, Zev Weiss wrote:

> I can see why it might look that way, but I'd argue it's actually not.  The
> systems this is intended to support provide power to entirely separate
> external devices -- think of a power distribution unit that might have
> arbitrary things plugged into it.  It seems to me like a property of the
> hardware that those things shouldn't have their power supply turned off (or
> on) just because a controller in the PDU rebooted.

We don't turn things off on reboot?  We don't do anything in particular
on reboot...

> > I guess it easy
> > to understand in case of Linux which disables unclaimed regulators
> > during. But what if other system/firmware does not behave like that?

> In this case, then no change would be needed -- a system that (unlike Linux)
> doesn't twiddle regulator state on its own would just continue to not do
> that.

We don't turn unclaimed regulators off until userspace has had a chance
to start, if there's some problem with system integrators arranging to
do this we can look into how that works, for example making the delay
tunable.  I don't think this is really meaningfully different from a
driver deciding to turn things off from a binding point of view.

> > And what is the "external actor"? OS is not an external actor?

> It's admittedly a bit vague, but I couldn't think of a clearer way to
> express what is a sort of nebulous concept -- essentially, some entity
> outside the "driver" (or analogous software component) using the information
> in the device-tree.  In many common cases this would essentially mean "a
> human user", since in the PDU-like systems I'm targeting here the only thing
> that should ever be deciding to turn the regulator on or off is an operator
> logged in to the system to manually enable or disable an outlet.  I was
> aiming to leave the wording a bit more general though, since in some other
> context I could imagine some other piece of software toggling things
> automatedly (e.g. lights getting turned on and off on a schedule or
> something, if that's what happens to be plugged in).

This is policy stuff, it doesn't translate into DTs at all.

> > I could not get the problem you want to solve with this property - I
> > looked at cover letter and at commit msg.

> The problem is that a driver deciding on its own to enable or disable the
> regulator (e.g. during boot or shutdown) would be a critical failure for the
> kind of systems I'm aiming to support.

If the driver is doing something like this it should be addressed in the
driver.

> > I can only imagine that you want to keep regulator on, after last its
> > user disappears... but for what purpose? Do you expect that after system
> > shutdown the pin will stay high so regulator will be also on? If so, you
> > need hardware design, e.g. with some pull up (if control is over GPIO).

> As described above, the regulators involved here (in these sorts of PDU-like
> systems) provide power for external systems and devices.  It is critical
> that the controller's boot and shutdown sequences not alter the state of the
> regulator.

This really sounds like a full stack system integration problem, not
something that can be resolved with one software component.

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

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Mark Brown]

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

On Thu, Oct 27, 2022 at 09:12:22PM -0700, Zev Weiss wrote:

> I can see why it might look that way, but I'd argue it's actually not.  The
> systems this is intended to support provide power to entirely separate
> external devices -- think of a power distribution unit that might have
> arbitrary things plugged into it.  It seems to me like a property of the
> hardware that those things shouldn't have their power supply turned off (or
> on) just because a controller in the PDU rebooted.

We don't turn things off on reboot?  We don't do anything in particular
on reboot...

> > I guess it easy
> > to understand in case of Linux which disables unclaimed regulators
> > during. But what if other system/firmware does not behave like that?

> In this case, then no change would be needed -- a system that (unlike Linux)
> doesn't twiddle regulator state on its own would just continue to not do
> that.

We don't turn unclaimed regulators off until userspace has had a chance
to start, if there's some problem with system integrators arranging to
do this we can look into how that works, for example making the delay
tunable.  I don't think this is really meaningfully different from a
driver deciding to turn things off from a binding point of view.

> > And what is the "external actor"? OS is not an external actor?

> It's admittedly a bit vague, but I couldn't think of a clearer way to
> express what is a sort of nebulous concept -- essentially, some entity
> outside the "driver" (or analogous software component) using the information
> in the device-tree.  In many common cases this would essentially mean "a
> human user", since in the PDU-like systems I'm targeting here the only thing
> that should ever be deciding to turn the regulator on or off is an operator
> logged in to the system to manually enable or disable an outlet.  I was
> aiming to leave the wording a bit more general though, since in some other
> context I could imagine some other piece of software toggling things
> automatedly (e.g. lights getting turned on and off on a schedule or
> something, if that's what happens to be plugged in).

This is policy stuff, it doesn't translate into DTs at all.

> > I could not get the problem you want to solve with this property - I
> > looked at cover letter and at commit msg.

> The problem is that a driver deciding on its own to enable or disable the
> regulator (e.g. during boot or shutdown) would be a critical failure for the
> kind of systems I'm aiming to support.

If the driver is doing something like this it should be addressed in the
driver.

> > I can only imagine that you want to keep regulator on, after last its
> > user disappears... but for what purpose? Do you expect that after system
> > shutdown the pin will stay high so regulator will be also on? If so, you
> > need hardware design, e.g. with some pull up (if control is over GPIO).

> As described above, the regulators involved here (in these sorts of PDU-like
> systems) provide power for external systems and devices.  It is critical
> that the controller's boot and shutdown sequences not alter the state of the
> regulator.

This really sounds like a full stack system integration problem, not
something that can be resolved with one software component.

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

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Zev Weiss]

On Fri, Oct 28, 2022 at 08:51:54AM PDT, Mark Brown wrote:
>On Thu, Oct 27, 2022 at 09:12:22PM -0700, Zev Weiss wrote:
>
>> I can see why it might look that way, but I'd argue it's actually not.  The
>> systems this is intended to support provide power to entirely separate
>> external devices -- think of a power distribution unit that might have
>> arbitrary things plugged into it.  It seems to me like a property of the
>> hardware that those things shouldn't have their power supply turned off (or
>> on) just because a controller in the PDU rebooted.
>
>We don't turn things off on reboot?  We don't do anything in particular
>on reboot...
>

Okay, perhaps not on reboot specifically, but the userspace-consumer 
driver has a regulator_bulk_disable() in its .remove function, so it 
would be triggered at least by a module unload (which is sort of why I 
ended up with the "when software relinquishes control" wording in the 
patch).  If we're going to continue with the plan of using that driver 
for this functionality (which seems overall quite reasonable to me), we 
need a way to express that that must not happen on this hardware.


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Zev Weiss]

On Fri, Oct 28, 2022 at 08:51:54AM PDT, Mark Brown wrote:
>On Thu, Oct 27, 2022 at 09:12:22PM -0700, Zev Weiss wrote:
>
>> I can see why it might look that way, but I'd argue it's actually not.  The
>> systems this is intended to support provide power to entirely separate
>> external devices -- think of a power distribution unit that might have
>> arbitrary things plugged into it.  It seems to me like a property of the
>> hardware that those things shouldn't have their power supply turned off (or
>> on) just because a controller in the PDU rebooted.
>
>We don't turn things off on reboot?  We don't do anything in particular
>on reboot...
>

Okay, perhaps not on reboot specifically, but the userspace-consumer 
driver has a regulator_bulk_disable() in its .remove function, so it 
would be triggered at least by a module unload (which is sort of why I 
ended up with the "when software relinquishes control" wording in the 
patch).  If we're going to continue with the plan of using that driver 
for this functionality (which seems overall quite reasonable to me), we 
need a way to express that that must not happen on this hardware.


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Mark Brown]

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

On Fri, Oct 28, 2022 at 12:44:50PM -0700, Zev Weiss wrote:
> On Fri, Oct 28, 2022 at 08:51:54AM PDT, Mark Brown wrote:

> > We don't turn things off on reboot?  We don't do anything in particular
> > on reboot...

> Okay, perhaps not on reboot specifically, but the userspace-consumer driver
> has a regulator_bulk_disable() in its .remove function, so it would be
> triggered at least by a module unload (which is sort of why I ended up with
> the "when software relinquishes control" wording in the patch).  If we're
> going to continue with the plan of using that driver for this functionality
> (which seems overall quite reasonable to me), we need a way to express that
> that must not happen on this hardware.

Ah, that would be the test driver not intended to be used in production
then...  That shouldn't be a blocker for the DT binding, and if there's
a different compatible string for this application then we can either
make the userspace consumer do something different based on that
compatible string or have a new driver which does something more
sensible and perhaps has a better userspace ABI.  Either way so long as
we can tell the thing being described is a BMC output from the DT
binding I think we can leave it up to the OS to do something constructive
with that rather than trying to control the specific behaviour in the
binding.

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

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Mark Brown]

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

On Fri, Oct 28, 2022 at 12:44:50PM -0700, Zev Weiss wrote:
> On Fri, Oct 28, 2022 at 08:51:54AM PDT, Mark Brown wrote:

> > We don't turn things off on reboot?  We don't do anything in particular
> > on reboot...

> Okay, perhaps not on reboot specifically, but the userspace-consumer driver
> has a regulator_bulk_disable() in its .remove function, so it would be
> triggered at least by a module unload (which is sort of why I ended up with
> the "when software relinquishes control" wording in the patch).  If we're
> going to continue with the plan of using that driver for this functionality
> (which seems overall quite reasonable to me), we need a way to express that
> that must not happen on this hardware.

Ah, that would be the test driver not intended to be used in production
then...  That shouldn't be a blocker for the DT binding, and if there's
a different compatible string for this application then we can either
make the userspace consumer do something different based on that
compatible string or have a new driver which does something more
sensible and perhaps has a better userspace ABI.  Either way so long as
we can tell the thing being described is a BMC output from the DT
binding I think we can leave it up to the OS to do something constructive
with that rather than trying to control the specific behaviour in the
binding.

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

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Zev Weiss]

On Mon, Oct 31, 2022 at 08:45:34AM PDT, Mark Brown wrote:
>On Fri, Oct 28, 2022 at 12:44:50PM -0700, Zev Weiss wrote:
>> On Fri, Oct 28, 2022 at 08:51:54AM PDT, Mark Brown wrote:
>
>> > We don't turn things off on reboot?  We don't do anything in particular
>> > on reboot...
>
>> Okay, perhaps not on reboot specifically, but the userspace-consumer driver
>> has a regulator_bulk_disable() in its .remove function, so it would be
>> triggered at least by a module unload (which is sort of why I ended up with
>> the "when software relinquishes control" wording in the patch).  If we're
>> going to continue with the plan of using that driver for this functionality
>> (which seems overall quite reasonable to me), we need a way to express that
>> that must not happen on this hardware.
>
>Ah, that would be the test driver not intended to be used in production
>then...  That shouldn't be a blocker for the DT binding, and if there's
>a different compatible string for this application then we can either
>make the userspace consumer do something different based on that
>compatible string or have a new driver which does something more
>sensible and perhaps has a better userspace ABI.  Either way so long as
>we can tell the thing being described is a BMC output from the DT
>binding I think we can leave it up to the OS to do something constructive
>with that rather than trying to control the specific behaviour in the
>binding.

Ah, alright -- that seems like a nice (obvious in retrospect, of course) 
solution that should work well I think.  I'll post a v2 with that 
approach soon.


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output bindingg

[Zev Weiss]

On Mon, Oct 31, 2022 at 08:45:34AM PDT, Mark Brown wrote:
>On Fri, Oct 28, 2022 at 12:44:50PM -0700, Zev Weiss wrote:
>> On Fri, Oct 28, 2022 at 08:51:54AM PDT, Mark Brown wrote:
>
>> > We don't turn things off on reboot?  We don't do anything in particular
>> > on reboot...
>
>> Okay, perhaps not on reboot specifically, but the userspace-consumer driver
>> has a regulator_bulk_disable() in its .remove function, so it would be
>> triggered at least by a module unload (which is sort of why I ended up with
>> the "when software relinquishes control" wording in the patch).  If we're
>> going to continue with the plan of using that driver for this functionality
>> (which seems overall quite reasonable to me), we need a way to express that
>> that must not happen on this hardware.
>
>Ah, that would be the test driver not intended to be used in production
>then...  That shouldn't be a blocker for the DT binding, and if there's
>a different compatible string for this application then we can either
>make the userspace consumer do something different based on that
>compatible string or have a new driver which does something more
>sensible and perhaps has a better userspace ABI.  Either way so long as
>we can tell the thing being described is a BMC output from the DT
>binding I think we can leave it up to the OS to do something constructive
>with that rather than trying to control the specific behaviour in the
>binding.

Ah, alright -- that seems like a nice (obvious in retrospect, of course) 
solution that should work well I think.  I'll post a v2 with that 
approach soon.


Thanks,
Zev

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Mark Brown]

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

On Thu, Sep 29, 2022 at 02:27:19PM -0700, Zev Weiss wrote:
> On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:

> never as any sort of default/automatic action.  The two obvious things to
> guard against there seem to be automatic enablement during initialization
> and automatic disablement on de-init (shutdown, unbind, etc.).  The former I
> think can be avoided by simply not setting regulator-boot-on, so I added
> this as a corresponding property to avoid the latter.

> I'm definitely open to suggestions for a better approach though.

regulator-boot-on mainly exists for handover of state from the
bootloader where we can't read back the state of the hardware rather
than as a control for boot purposes.

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

Re: [PATCH 2/3] dt-bindings: regulator: Add regulator-output binding

[Mark Brown]

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

On Thu, Sep 29, 2022 at 02:27:19PM -0700, Zev Weiss wrote:
> On Thu, Sep 29, 2022 at 02:07:14PM PDT, Rob Herring wrote:

> never as any sort of default/automatic action.  The two obvious things to
> guard against there seem to be automatic enablement during initialization
> and automatic disablement on de-init (shutdown, unbind, etc.).  The former I
> think can be avoided by simply not setting regulator-boot-on, so I added
> this as a corresponding property to avoid the latter.

> I'm definitely open to suggestions for a better approach though.

regulator-boot-on mainly exists for handover of state from the
bootloader where we can't read back the state of the hardware rather
than as a control for boot purposes.

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

[PATCH 3/3] regulator: userspace-consumer: Handle regulator-output DT nodes

[Zev Weiss]

In addition to adding some fairly simple OF support code, we make some
slight adjustments to the userspace-consumer driver to properly
support use with regulator-output hardware:

 - We now do an exclusive get of the supply regulators so as to
   prevent regulator_init_complete_work from automatically disabling
   them.

 - Instead of assuming that the supply is initially disabled, we now
   query its state to determine the initial value of drvdata->enabled.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/regulator/userspace-consumer.c       | 43 +++++++++++++++++---
 include/linux/regulator/userspace-consumer.h |  1 +
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
index 8ca28664776e..81752458f75e 100644
--- a/drivers/regulator/userspace-consumer.c
+++ b/drivers/regulator/userspace-consumer.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/userspace-consumer.h>
@@ -24,6 +25,7 @@ struct userspace_consumer_data {
 
 	struct mutex lock;
 	bool enabled;
+	bool leave_on;
 
 	int num_supplies;
 	struct regulator_bulk_data *supplies;
@@ -102,13 +104,32 @@ static const struct attribute_group attr_group = {
 
 static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 {
+	struct regulator_userspace_consumer_data tmpdata;
 	struct regulator_userspace_consumer_data *pdata;
 	struct userspace_consumer_data *drvdata;
 	int ret;
 
 	pdata = dev_get_platdata(&pdev->dev);
-	if (!pdata)
+	if (!pdata) {
+		if (!pdev->dev.of_node)
+			return -EINVAL;
+
+		pdata = &tmpdata;
+		memset(pdata, 0, sizeof(*pdata));
+
+		pdata->num_supplies = 1;
+		pdata->supplies = devm_kzalloc(&pdev->dev, sizeof(*pdata->supplies), GFP_KERNEL);
+		if (!pdata->supplies)
+			return -ENOMEM;
+		pdata->supplies[0].supply = "vout";
+
+		pdata->leave_on = of_property_read_bool(pdev->dev.of_node, "regulator-leave-on");
+	}
+
+	if (pdata->num_supplies < 1) {
+		dev_err(&pdev->dev, "At least one supply required\n");
 		return -EINVAL;
+	}
 
 	drvdata = devm_kzalloc(&pdev->dev,
 			       sizeof(struct userspace_consumer_data),
@@ -119,11 +140,12 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 	drvdata->name = pdata->name;
 	drvdata->num_supplies = pdata->num_supplies;
 	drvdata->supplies = pdata->supplies;
+	drvdata->leave_on = pdata->leave_on;
 
 	mutex_init(&drvdata->lock);
 
-	ret = devm_regulator_bulk_get(&pdev->dev, drvdata->num_supplies,
-				      drvdata->supplies);
+	ret = devm_regulator_bulk_get_exclusive(&pdev->dev, drvdata->num_supplies,
+						drvdata->supplies);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to get supplies: %d\n", ret);
 		return ret;
@@ -143,7 +165,12 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 		}
 	}
 
-	drvdata->enabled = pdata->init_on;
+	ret = regulator_is_enabled(pdata->supplies[0].consumer);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to get regulator status\n");
+		goto err_enable;
+	}
+	drvdata->enabled = !!ret;
 	platform_set_drvdata(pdev, drvdata);
 
 	return 0;
@@ -160,17 +187,23 @@ static int regulator_userspace_consumer_remove(struct platform_device *pdev)
 
 	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
 
-	if (data->enabled)
+	if (data->enabled && !data->leave_on)
 		regulator_bulk_disable(data->num_supplies, data->supplies);
 
 	return 0;
 }
 
+static const struct of_device_id regulator_userspace_consumer_of_match[] = {
+	{ .compatible = "regulator-output", },
+	{},
+};
+
 static struct platform_driver regulator_userspace_consumer_driver = {
 	.probe		= regulator_userspace_consumer_probe,
 	.remove		= regulator_userspace_consumer_remove,
 	.driver		= {
 		.name		= "reg-userspace-consumer",
+		.of_match_table = regulator_userspace_consumer_of_match,
 	},
 };
 
diff --git a/include/linux/regulator/userspace-consumer.h b/include/linux/regulator/userspace-consumer.h
index b5dba0628951..7eac57ede8ad 100644
--- a/include/linux/regulator/userspace-consumer.h
+++ b/include/linux/regulator/userspace-consumer.h
@@ -21,6 +21,7 @@ struct regulator_userspace_consumer_data {
 	struct regulator_bulk_data *supplies;
 
 	bool init_on;
+	bool leave_on;
 };
 
 #endif /* __REGULATOR_PLATFORM_CONSUMER_H_ */
-- 
2.37.3

[PATCH 3/3] regulator: userspace-consumer: Handle regulator-output DT nodes

[Zev Weiss]

In addition to adding some fairly simple OF support code, we make some
slight adjustments to the userspace-consumer driver to properly
support use with regulator-output hardware:

 - We now do an exclusive get of the supply regulators so as to
   prevent regulator_init_complete_work from automatically disabling
   them.

 - Instead of assuming that the supply is initially disabled, we now
   query its state to determine the initial value of drvdata->enabled.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/regulator/userspace-consumer.c       | 43 +++++++++++++++++---
 include/linux/regulator/userspace-consumer.h |  1 +
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
index 8ca28664776e..81752458f75e 100644
--- a/drivers/regulator/userspace-consumer.c
+++ b/drivers/regulator/userspace-consumer.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/userspace-consumer.h>
@@ -24,6 +25,7 @@ struct userspace_consumer_data {
 
 	struct mutex lock;
 	bool enabled;
+	bool leave_on;
 
 	int num_supplies;
 	struct regulator_bulk_data *supplies;
@@ -102,13 +104,32 @@ static const struct attribute_group attr_group = {
 
 static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 {
+	struct regulator_userspace_consumer_data tmpdata;
 	struct regulator_userspace_consumer_data *pdata;
 	struct userspace_consumer_data *drvdata;
 	int ret;
 
 	pdata = dev_get_platdata(&pdev->dev);
-	if (!pdata)
+	if (!pdata) {
+		if (!pdev->dev.of_node)
+			return -EINVAL;
+
+		pdata = &tmpdata;
+		memset(pdata, 0, sizeof(*pdata));
+
+		pdata->num_supplies = 1;
+		pdata->supplies = devm_kzalloc(&pdev->dev, sizeof(*pdata->supplies), GFP_KERNEL);
+		if (!pdata->supplies)
+			return -ENOMEM;
+		pdata->supplies[0].supply = "vout";
+
+		pdata->leave_on = of_property_read_bool(pdev->dev.of_node, "regulator-leave-on");
+	}
+
+	if (pdata->num_supplies < 1) {
+		dev_err(&pdev->dev, "At least one supply required\n");
 		return -EINVAL;
+	}
 
 	drvdata = devm_kzalloc(&pdev->dev,
 			       sizeof(struct userspace_consumer_data),
@@ -119,11 +140,12 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 	drvdata->name = pdata->name;
 	drvdata->num_supplies = pdata->num_supplies;
 	drvdata->supplies = pdata->supplies;
+	drvdata->leave_on = pdata->leave_on;
 
 	mutex_init(&drvdata->lock);
 
-	ret = devm_regulator_bulk_get(&pdev->dev, drvdata->num_supplies,
-				      drvdata->supplies);
+	ret = devm_regulator_bulk_get_exclusive(&pdev->dev, drvdata->num_supplies,
+						drvdata->supplies);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to get supplies: %d\n", ret);
 		return ret;
@@ -143,7 +165,12 @@ static int regulator_userspace_consumer_probe(struct platform_device *pdev)
 		}
 	}
 
-	drvdata->enabled = pdata->init_on;
+	ret = regulator_is_enabled(pdata->supplies[0].consumer);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to get regulator status\n");
+		goto err_enable;
+	}
+	drvdata->enabled = !!ret;
 	platform_set_drvdata(pdev, drvdata);
 
 	return 0;
@@ -160,17 +187,23 @@ static int regulator_userspace_consumer_remove(struct platform_device *pdev)
 
 	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
 
-	if (data->enabled)
+	if (data->enabled && !data->leave_on)
 		regulator_bulk_disable(data->num_supplies, data->supplies);
 
 	return 0;
 }
 
+static const struct of_device_id regulator_userspace_consumer_of_match[] = {
+	{ .compatible = "regulator-output", },
+	{},
+};
+
 static struct platform_driver regulator_userspace_consumer_driver = {
 	.probe		= regulator_userspace_consumer_probe,
 	.remove		= regulator_userspace_consumer_remove,
 	.driver		= {
 		.name		= "reg-userspace-consumer",
+		.of_match_table = regulator_userspace_consumer_of_match,
 	},
 };
 
diff --git a/include/linux/regulator/userspace-consumer.h b/include/linux/regulator/userspace-consumer.h
index b5dba0628951..7eac57ede8ad 100644
--- a/include/linux/regulator/userspace-consumer.h
+++ b/include/linux/regulator/userspace-consumer.h
@@ -21,6 +21,7 @@ struct regulator_userspace_consumer_data {
 	struct regulator_bulk_data *supplies;
 
 	bool init_on;
+	bool leave_on;
 };
 
 #endif /* __REGULATOR_PLATFORM_CONSUMER_H_ */
-- 
2.37.3

Re: [PATCH 0/3] regulator: Add DT support for regulator-output connectors

[Patrick Rudolph]

Hi Zev,
Thanks for picking this up.
I cannot answer the first question, but the second:
It's sufficient for us to just have one supply per userspace-consumer instance.

The optional property is fine and could be useful in the future.

Regards,
Patrick

On Mon, Sep 26, 2022 at 12:04 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> Hello,
>
> This series is another attempt at implementing support for
> userspace-controlled regulator-supplied power outputs.  This is an
> important feature for some kinds of BMC (baseboard management
> controller) systems where the BMC provides an operator with manual
> control of a set of DC power outputs.
>
> As in a broadly similar patchset that was recently almost merged [0],
> this takes the approach of providing support by extending the existing
> userspace-consumer regulator driver.  A couple questions about the
> userspace-consumer driver came up along the way, however.
>
> First, how (if at all) is it currently being used?  It appears the
> last in-tree use of it was removed a bit over two years ago in commit
> 9d3239147d6d ("ARM: pxa: remove Compulab pxa2xx boards").  Aside from
> just adding DT support I've made a couple small tweaks to the driver
> in patch 3 that I hope are compatible with any current usage, but
> without any extant examples to look at it's kind of hard to say.
>
> Second, how critical is its support for controlling multiple
> regulators?  (i.e. its use of regulator_bulk_data and friends instead
> of a single struct regulator.)  As far as I can see every in-tree use
> of it that's ever existed has used num_supplies = 1.  If it's not
> important to retain, patch 1 of this series could be supplanted by one
> that instead simplifies the driver slightly by removing that
> functionality.
>
> The DT binding added in patch 2 is very similar to one I posted in a
> previous patchset that had an R-B from Rob [1], but has had some minor
> rewording and gained one new (optional) property.
>
> Laxman, Naresh, Patrick -- please let me know if there are any aspects
> of this implementation that would be incompatible with your needs.
>
>
> Thanks,
> Zev
>
> [0] https://lore.kernel.org/all/20220707081826.953449-4-Naresh.Solanki@9elements.com/
> [1] https://lore.kernel.org/linux-kernel/20220505232557.10936-2-zev@bewilderbeest.net/
>
> Zev Weiss (3):
>   regulator: devres: Add devm_regulator_bulk_get_exclusive()
>   dt-bindings: regulator: Add regulator-output binding
>   regulator: userspace-consumer: Handle regulator-output DT nodes
>
>  .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++
>  drivers/regulator/core.c                      | 42 +++++++-----
>  drivers/regulator/devres.c                    | 66 ++++++++++++++-----
>  drivers/regulator/internal.h                  |  2 +
>  drivers/regulator/userspace-consumer.c        | 43 ++++++++++--
>  include/linux/regulator/consumer.h            |  2 +
>  include/linux/regulator/userspace-consumer.h  |  1 +
>  7 files changed, 162 insertions(+), 41 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>
> --
> 2.37.3
>

Re: [PATCH 0/3] regulator: Add DT support for regulator-output connectors

[Patrick Rudolph]

Hi Zev,
Thanks for picking this up.
I cannot answer the first question, but the second:
It's sufficient for us to just have one supply per userspace-consumer instance.

The optional property is fine and could be useful in the future.

Regards,
Patrick

On Mon, Sep 26, 2022 at 12:04 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> Hello,
>
> This series is another attempt at implementing support for
> userspace-controlled regulator-supplied power outputs.  This is an
> important feature for some kinds of BMC (baseboard management
> controller) systems where the BMC provides an operator with manual
> control of a set of DC power outputs.
>
> As in a broadly similar patchset that was recently almost merged [0],
> this takes the approach of providing support by extending the existing
> userspace-consumer regulator driver.  A couple questions about the
> userspace-consumer driver came up along the way, however.
>
> First, how (if at all) is it currently being used?  It appears the
> last in-tree use of it was removed a bit over two years ago in commit
> 9d3239147d6d ("ARM: pxa: remove Compulab pxa2xx boards").  Aside from
> just adding DT support I've made a couple small tweaks to the driver
> in patch 3 that I hope are compatible with any current usage, but
> without any extant examples to look at it's kind of hard to say.
>
> Second, how critical is its support for controlling multiple
> regulators?  (i.e. its use of regulator_bulk_data and friends instead
> of a single struct regulator.)  As far as I can see every in-tree use
> of it that's ever existed has used num_supplies = 1.  If it's not
> important to retain, patch 1 of this series could be supplanted by one
> that instead simplifies the driver slightly by removing that
> functionality.
>
> The DT binding added in patch 2 is very similar to one I posted in a
> previous patchset that had an R-B from Rob [1], but has had some minor
> rewording and gained one new (optional) property.
>
> Laxman, Naresh, Patrick -- please let me know if there are any aspects
> of this implementation that would be incompatible with your needs.
>
>
> Thanks,
> Zev
>
> [0] https://lore.kernel.org/all/20220707081826.953449-4-Naresh.Solanki@9elements.com/
> [1] https://lore.kernel.org/linux-kernel/20220505232557.10936-2-zev@bewilderbeest.net/
>
> Zev Weiss (3):
>   regulator: devres: Add devm_regulator_bulk_get_exclusive()
>   dt-bindings: regulator: Add regulator-output binding
>   regulator: userspace-consumer: Handle regulator-output DT nodes
>
>  .../bindings/regulator/regulator-output.yaml  | 47 +++++++++++++
>  drivers/regulator/core.c                      | 42 +++++++-----
>  drivers/regulator/devres.c                    | 66 ++++++++++++++-----
>  drivers/regulator/internal.h                  |  2 +
>  drivers/regulator/userspace-consumer.c        | 43 ++++++++++--
>  include/linux/regulator/consumer.h            |  2 +
>  include/linux/regulator/userspace-consumer.h  |  1 +
>  7 files changed, 162 insertions(+), 41 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/regulator-output.yaml
>
> --
> 2.37.3
>