[PATCH 1/6] dt-bindings: regulator: Add regulator-external-output property

[PATCH 1/6] dt-bindings: regulator: Add regulator-external-output property

[Zev Weiss]

Some regulators do not provide power to anything within the system
described by a device tree, and simply supply an external output.  The
regulator-external-output property can now be used to mark such
regulators.

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

diff --git a/Documentation/devicetree/bindings/regulator/regulator.yaml b/Documentation/devicetree/bindings/regulator/regulator.yaml
index a9b66ececccf..0e418e68b0e6 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/regulator.yaml
@@ -226,6 +226,12 @@ properties:
     description: Maximum difference between current and target voltages
       that can be changed safely in a single step.
 
+  regulator-external-output:
+    description: The regulator's output is external to the system
+      described by the device-tree; no devices within the system are
+      downstream of it.
+    type: boolean
+
 patternProperties:
   ".*-supply$":
     description: Input supply phandle(s) for this node
-- 
2.36.0

[PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Zev Weiss]

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

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

diff --git a/Documentation/devicetree/bindings/regulator/reg-external-output.yaml b/Documentation/devicetree/bindings/regulator/reg-external-output.yaml
new file mode 100644
index 000000000000..c00bee5bd9f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/reg-external-output.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/regulator/reg-external-output.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Regulator external output
+
+maintainers:
+  - Zev Weiss <zev@bewilderbeest.net>
+
+description: |
+  This describes an external output supplied by a regulator, such as
+  a power outlet on a power distribution unit (PDU).
+
+properties:
+  compatible:
+    const: reg-external-output
+
+  vout-supply:
+    description:
+      Phandle of the regulator supplying the output, which should have
+      the regulator-external-output property.
+
+required:
+  - compatible
+  - vout-supply
+
+additionalProperties: false
+
+examples:
+  - |
+      output {
+          compatible = "reg-external-output";
+          vout-supply = <&output_reg>;
+      };
-- 
2.36.0

[PATCH 3/6] regulator: core: Add error flags to sysfs attributes

[Zev Weiss]

If a regulator provides a get_error_flags() operation, its sysfs
attributes will now include an entry for each defined
REGULATOR_ERROR_* flag.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../ABI/testing/sysfs-class-regulator         | 81 +++++++++++++++++++
 drivers/regulator/core.c                      | 45 +++++++++++
 2 files changed, 126 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-regulator b/Documentation/ABI/testing/sysfs-class-regulator
index 8516f08806dd..475b9a372657 100644
--- a/Documentation/ABI/testing/sysfs-class-regulator
+++ b/Documentation/ABI/testing/sysfs-class-regulator
@@ -370,3 +370,84 @@ Description:
 
 		'unknown' means software cannot determine the state, or
 		the reported state is invalid.
+
+What:		/sys/class/regulator/.../under_voltage
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		under_voltage.  This indicates if the device reports an
+		under-voltage fault (1) or not (0).
+
+What:		/sys/class/regulator/.../over_current
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		over_current.  This indicates if the device reports an
+		over-current fault (1) or not (0).
+
+What:		/sys/class/regulator/.../regulation_out
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		regulation_out.  This indicates if the device reports an
+		out-of-regulation fault (1) or not (0).
+
+What:		/sys/class/regulator/.../fail
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		fail.  This indicates if the device reports an output failure
+		(1) or not (0).
+
+What:		/sys/class/regulator/.../over_temp
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		over_temp.  This indicates if the device reports an
+		over-temperature fault (1) or not (0).
+
+What:		/sys/class/regulator/.../under_voltage_warn
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		under_voltage_warn.  This indicates if the device reports an
+		under-voltage warning (1) or not (0).
+
+What:		/sys/class/regulator/.../over_current_warn
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		over_current_warn.  This indicates if the device reports an
+		over-current warning (1) or not (0).
+
+What:		/sys/class/regulator/.../over_voltage_warn
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		over_voltage_warn.  This indicates if the device reports an
+		over-voltage warning (1) or not (0).
+
+What:		/sys/class/regulator/.../over_temp_warn
+Date:		April 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:
+		Some regulator directories will contain a field called
+		over_temp_warn.  This indicates if the device reports an
+		over-temperature warning (1) or not (0).
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d2553970a67b..9094bd8772e5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -83,6 +83,7 @@ struct regulator_supply_alias {
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
 static int _regulator_disable(struct regulator *regulator);
+static int _regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
 static int _notifier_call_chain(struct regulator_dev *rdev,
@@ -911,6 +912,30 @@ static ssize_t bypass_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(bypass);
 
+#define REGULATOR_ERROR_ATTR(name, bit)							\
+	static ssize_t name##_show(struct device *dev, struct device_attribute *attr,	\
+				   char *buf)						\
+	{										\
+		int ret;								\
+		unsigned int flags;							\
+		struct regulator_dev *rdev = dev_get_drvdata(dev);			\
+		ret = _regulator_get_error_flags(rdev, &flags);				\
+		if (ret)								\
+			return ret;							\
+		return sysfs_emit(buf, "%d\n", !!(flags & (bit)));			\
+	}										\
+	static DEVICE_ATTR_RO(name)
+
+REGULATOR_ERROR_ATTR(under_voltage, REGULATOR_ERROR_UNDER_VOLTAGE);
+REGULATOR_ERROR_ATTR(over_current, REGULATOR_ERROR_OVER_CURRENT);
+REGULATOR_ERROR_ATTR(regulation_out, REGULATOR_ERROR_REGULATION_OUT);
+REGULATOR_ERROR_ATTR(fail, REGULATOR_ERROR_FAIL);
+REGULATOR_ERROR_ATTR(over_temp, REGULATOR_ERROR_OVER_TEMP);
+REGULATOR_ERROR_ATTR(under_voltage_warn, REGULATOR_ERROR_UNDER_VOLTAGE_WARN);
+REGULATOR_ERROR_ATTR(over_current_warn, REGULATOR_ERROR_OVER_CURRENT_WARN);
+REGULATOR_ERROR_ATTR(over_voltage_warn, REGULATOR_ERROR_OVER_VOLTAGE_WARN);
+REGULATOR_ERROR_ATTR(over_temp_warn, REGULATOR_ERROR_OVER_TEMP_WARN);
+
 /* Calculate the new optimum regulator operating mode based on the new total
  * consumer load. All locks held by caller
  */
@@ -4971,6 +4996,15 @@ static struct attribute *regulator_dev_attrs[] = {
 	&dev_attr_max_microvolts.attr,
 	&dev_attr_min_microamps.attr,
 	&dev_attr_max_microamps.attr,
+	&dev_attr_under_voltage.attr,
+	&dev_attr_over_current.attr,
+	&dev_attr_regulation_out.attr,
+	&dev_attr_fail.attr,
+	&dev_attr_over_temp.attr,
+	&dev_attr_under_voltage_warn.attr,
+	&dev_attr_over_current_warn.attr,
+	&dev_attr_over_voltage_warn.attr,
+	&dev_attr_over_temp_warn.attr,
 	&dev_attr_suspend_standby_state.attr,
 	&dev_attr_suspend_mem_state.attr,
 	&dev_attr_suspend_disk_state.attr,
@@ -5026,6 +5060,17 @@ static umode_t regulator_attr_is_visible(struct kobject *kobj,
 	if (attr == &dev_attr_bypass.attr)
 		return ops->get_bypass ? mode : 0;
 
+	if (attr == &dev_attr_under_voltage.attr ||
+	    attr == &dev_attr_over_current.attr ||
+	    attr == &dev_attr_regulation_out.attr ||
+	    attr == &dev_attr_fail.attr ||
+	    attr == &dev_attr_over_temp.attr ||
+	    attr == &dev_attr_under_voltage_warn.attr ||
+	    attr == &dev_attr_over_current_warn.attr ||
+	    attr == &dev_attr_over_voltage_warn.attr ||
+	    attr == &dev_attr_over_temp_warn.attr)
+		return ops->get_error_flags ? mode : 0;
+
 	/* constraints need specific supporting methods */
 	if (attr == &dev_attr_min_microvolts.attr ||
 	    attr == &dev_attr_max_microvolts.attr)
-- 
2.36.0

[PATCH 4/6] regulator: core: Add external-output support

[Zev Weiss]

Regulators feeding external outputs (i.e. supplying devices we don't
control) can be switched on and off by userspace via the 'state' sysfs
attribute, which is now (conditionally) writable.  They are also not
automatically disabled in regulator_late_cleanup(), since we have no
way of knowing that they're not actually in use.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../ABI/testing/sysfs-class-regulator         |  4 ++
 drivers/regulator/core.c                      | 41 ++++++++++++++++---
 drivers/regulator/of_regulator.c              |  2 +
 include/linux/regulator/machine.h             |  1 +
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-regulator b/Documentation/ABI/testing/sysfs-class-regulator
index 475b9a372657..1c0451730df2 100644
--- a/Documentation/ABI/testing/sysfs-class-regulator
+++ b/Documentation/ABI/testing/sysfs-class-regulator
@@ -23,6 +23,10 @@ Description:
 		'unknown' means software cannot determine the state, or
 		the reported state is invalid.
 
+		For regulators supplying external outputs, 'enabled'
+		or 'disabled' can be written to this file to turn the
+		regulator on and off.
+
 		NOTE: this field can be used in conjunction with microvolts
 		or microamps to determine configured regulator output levels.
 
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9094bd8772e5..b7617926336f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -83,6 +83,8 @@ struct regulator_supply_alias {
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
 static int _regulator_disable(struct regulator *regulator);
+static int _regulator_do_enable(struct regulator_dev *rdev);
+static int _regulator_do_disable(struct regulator_dev *rdev);
 static int _regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
@@ -667,7 +669,33 @@ static ssize_t state_show(struct device *dev,
 
 	return ret;
 }
-static DEVICE_ATTR_RO(state);
+
+static ssize_t state_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct regulator_dev *rdev = dev_get_drvdata(dev);
+	bool enabled;
+	ssize_t ret = 0;
+
+	if (sysfs_streq(buf, "enabled"))
+		enabled = true;
+	else if (sysfs_streq(buf, "disabled"))
+		enabled = false;
+	else
+		return -EINVAL;
+
+	regulator_lock(rdev);
+	if (enabled != _regulator_is_enabled(rdev)) {
+		if (enabled)
+			ret = _regulator_do_enable(rdev);
+		else
+			ret = _regulator_do_disable(rdev);
+	}
+	regulator_unlock(rdev);
+
+	return ret ? : count;
+}
+static DEVICE_ATTR_RW(state);
 
 static ssize_t status_show(struct device *dev,
 			   struct device_attribute *attr, char *buf)
@@ -5051,8 +5079,11 @@ static umode_t regulator_attr_is_visible(struct kobject *kobj,
 	if (attr == &dev_attr_opmode.attr)
 		return ops->get_mode ? mode : 0;
 
-	if (attr == &dev_attr_state.attr)
+	if (attr == &dev_attr_state.attr) {
+		if (!(rdev->constraints && rdev->constraints->external_output))
+			mode &= ~0222;
 		return (rdev->ena_pin || ops->is_enabled) ? mode : 0;
+	}
 
 	if (attr == &dev_attr_status.attr)
 		return ops->get_status ? mode : 0;
@@ -6062,7 +6093,7 @@ static int regulator_late_cleanup(struct device *dev, void *data)
 	struct regulation_constraints *c = rdev->constraints;
 	int ret;
 
-	if (c && c->always_on)
+	if (c && (c->always_on || c->external_output))
 		return 0;
 
 	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
@@ -6114,8 +6145,8 @@ static void regulator_init_complete_work_function(struct work_struct *work)
 
 	/* If we have a full configuration then disable any regulators
 	 * we have permission to change the status for and which are
-	 * not in use or always_on.  This is effectively the default
-	 * for DT and ACPI as they have full constraints.
+	 * not in use, always_on, or external_output.  This is effectively
+	 * the default for DT and ACPI as they have full constraints.
 	 */
 	class_for_each_device(&regulator_class, NULL, NULL,
 			      regulator_late_cleanup);
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index f54d4f176882..f48e6ea2b97e 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -96,6 +96,8 @@ static int of_get_regulation_constraints(struct device *dev,
 
 	constraints->name = of_get_property(np, "regulator-name", NULL);
 
+	constraints->external_output = of_property_read_bool(np, "regulator-external-output");
+
 	if (!of_property_read_u32(np, "regulator-min-microvolt", &pval))
 		constraints->min_uV = pval;
 
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 621b7f4a3639..a38e7db9f82e 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -219,6 +219,7 @@ struct regulation_constraints {
 	unsigned over_voltage_detection:1; /* notify on over voltage */
 	unsigned under_voltage_detection:1; /* notify on under voltage */
 	unsigned over_temp_detection:1; /* notify on over temperature */
+	unsigned external_output:1; /* output supplies only external devices */
 };
 
 /**
-- 
2.36.0

[PATCH 5/6] regulator: core: Add external get type

[Zev Weiss]

EXTERNAL_GET is similar to EXCLUSIVE_GET, but requires opt-in
agreement from the supply (whose constraints must designate it as
external_output).  It is intended for use only within the regulator
subsystem, and hence is not exposed in the public headers.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/regulator/core.c     | 16 +++++++++++++---
 drivers/regulator/devres.c   |  7 +++++++
 drivers/regulator/internal.h |  3 +++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b7617926336f..d873606eb41f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2087,6 +2087,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
 	struct device_link *link;
+	bool is_external;
 	int ret;
 
 	if (get_type >= MAX_GET_TYPE) {
@@ -2129,8 +2130,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 			break;
 
 		case EXCLUSIVE_GET:
+		case EXTERNAL_GET:
 			dev_warn(dev,
-				 "dummy supplies not allowed for exclusive requests\n");
+				 "dummy supplies not allowed for exclusive or external requests\n");
 			fallthrough;
 
 		default:
@@ -2144,12 +2146,20 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 		return regulator;
 	}
 
-	if (get_type == EXCLUSIVE_GET && rdev->open_count) {
+	if ((get_type == EXCLUSIVE_GET || get_type == EXTERNAL_GET) && rdev->open_count) {
 		regulator = ERR_PTR(-EBUSY);
 		put_device(&rdev->dev);
 		return regulator;
 	}
 
+	/* EXTERNAL_GET is valid if and only if the regulator is designated for external output */
+	is_external = rdev->constraints && rdev->constraints->external_output;
+	if ((get_type == EXTERNAL_GET) != is_external) {
+		regulator = ERR_PTR(-EINVAL);
+		put_device(&rdev->dev);
+		return regulator;
+	}
+
 	mutex_lock(&regulator_list_mutex);
 	ret = (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled);
 	mutex_unlock(&regulator_list_mutex);
@@ -2182,7 +2192,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 	}
 
 	rdev->open_count++;
-	if (get_type == EXCLUSIVE_GET) {
+	if (get_type == EXCLUSIVE_GET || get_type == EXTERNAL_GET) {
 		rdev->exclusive = 1;
 
 		ret = _regulator_is_enabled(rdev);
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 9113233f41cd..36df9e9ff175 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -70,6 +70,13 @@ struct regulator *devm_regulator_get_exclusive(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_regulator_get_exclusive);
 
+/* For regulator-core internal use only */
+struct regulator *devm_regulator_get_external(struct device *dev,
+					      const char *id)
+{
+	return _devm_regulator_get(dev, id, EXTERNAL_GET);
+}
+
 /**
  * devm_regulator_get_optional - Resource managed regulator_get_optional()
  * @dev: device to supply
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 1e9c71642143..c176a416c571 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -116,10 +116,13 @@ static inline bool of_check_coupling_data(struct regulator_dev *rdev)
 enum regulator_get_type {
 	NORMAL_GET,
 	EXCLUSIVE_GET,
+	EXTERNAL_GET,
 	OPTIONAL_GET,
 	MAX_GET_TYPE
 };
 
 struct regulator *_regulator_get(struct device *dev, const char *id,
 				 enum regulator_get_type get_type);
+struct regulator *devm_regulator_get_external(struct device *dev,
+					      const char *id);
 #endif
-- 
2.36.0

[PATCH 6/6] regulator: core: Add external-consumer driver

[Zev Weiss]

An external consumer is a dummy device representing a device external
to the system that is supplied by a regulator designated for external
output.  It is purely a virtual placeholder to instantiate a regulator
by calling regulator_get() on it from its probe function.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/regulator/Kconfig |  9 +++++++++
 drivers/regulator/core.c  | 28 ++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 5ef2306fce04..5ec390687a81 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -56,6 +56,15 @@ config REGULATOR_USERSPACE_CONSUMER
 
 	  If unsure, say no.
 
+config REGULATOR_EXTERNAL_CONSUMER
+	bool "External regulator consumer support"
+	help
+	  Some regulators supply devices that are entirely external to
+	  the system.  This driver provides a placeholder consumer
+	  representing such devices.
+
+	  If unsure, say no.
+
 config REGULATOR_88PG86X
 	tristate "Marvell 88PG86X voltage regulators"
 	depends on I2C
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d873606eb41f..4f348b3d5290 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/consumer.h>
@@ -6070,6 +6071,33 @@ static int regulator_summary_show(struct seq_file *s, void *data)
 DEFINE_SHOW_ATTRIBUTE(regulator_summary);
 #endif /* CONFIG_DEBUG_FS */
 
+#ifdef CONFIG_REGULATOR_EXTERNAL_CONSUMER
+static int regulator_external_output_probe(struct platform_device *pdev)
+{
+	struct regulator *reg;
+
+	reg = devm_regulator_get_external(&pdev->dev, "vout");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	return 0;
+}
+
+static const struct of_device_id reg_external_consumer_of_match_table[] = {
+	{ .compatible = "regulator-external-output" },
+	{ },
+};
+
+static struct platform_driver reg_external_consumer_driver = {
+	.driver = {
+		.name = "reg-external-consumer",
+		.of_match_table = reg_external_consumer_of_match_table,
+	},
+	.probe = regulator_external_output_probe,
+};
+builtin_platform_driver(reg_external_consumer_driver);
+#endif /* CONFIG_REGULATOR_EXTERNAL_CONSUMER */
+
 static int __init regulator_init(void)
 {
 	int ret;
-- 
2.36.0

Re: [PATCH 1/6] dt-bindings: regulator: Add regulator-external-output property

[Mark Brown]

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

On Tue, May 03, 2022 at 11:52:47PM -0700, Zev Weiss wrote:
> Some regulators do not provide power to anything within the system
> described by a device tree, and simply supply an external output.  The
> regulator-external-output property can now be used to mark such
> regulators.

Why not just add a device representing this output?  Presumably it has
some other properties (eg, labelling for a connector) and may need some
integration with some form of control mechanism.

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

Re: [PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Mark Brown]

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

On Tue, May 03, 2022 at 11:52:48PM -0700, Zev Weiss wrote:
> This describes an external output supplied by a regulator, such as a
> power outlet on a power distribution unit (PDU).

OK, so this is that represnetation of the connection - which raises the
question about why the regulator needs a property?

> +description: |
> +  This describes an external output supplied by a regulator, such as
> +  a power outlet on a power distribution unit (PDU).
> +
> +properties:
> +  compatible:
> +    const: reg-external-output
> +
> +  vout-supply:
> +    description:
> +      Phandle of the regulator supplying the output, which should have
> +      the regulator-external-output property.
> +
> +required:

I think at a minimum anything like this would need some sort of
representation of how the output physically appears so that people can
work out how outputs are mapped to the hardware they see.  However we
already have a subsystem for external connectors - extcon.  Perhaps this
should be a regulator client in the extcon API?  It's common for
connectors to include some sort of power provision so it seems like this
would fit right in.

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

Re: [PATCH 4/6] regulator: core: Add external-output support

[Mark Brown]

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

On Tue, May 03, 2022 at 11:52:50PM -0700, Zev Weiss wrote:
> Regulators feeding external outputs (i.e. supplying devices we don't
> control) can be switched on and off by userspace via the 'state' sysfs
> attribute, which is now (conditionally) writable.  They are also not
> automatically disabled in regulator_late_cleanup(), since we have no
> way of knowing that they're not actually in use.

No, this is an abstraction failure.  Enabling and disabling a regulator
is something that should be handled in consumer drivers, just as with
every other use case there's no reason why the regulator should be
offering direct control to userspace.  Putting this directly in the
regulator will cause problems as soon as you for example have multiple
outputs supplied from a single regulator and can't be integrated with
any other control mechanisms.

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

Re: [PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Zev Weiss]

On Wed, May 04, 2022 at 05:55:53AM PDT, Mark Brown wrote:
>On Tue, May 03, 2022 at 11:52:48PM -0700, Zev Weiss wrote:
>> This describes an external output supplied by a regulator, such as a
>> power outlet on a power distribution unit (PDU).
>
>OK, so this is that represnetation of the connection - which raises the
>question about why the regulator needs a property?
>
>> +description: |
>> +  This describes an external output supplied by a regulator, such as
>> +  a power outlet on a power distribution unit (PDU).
>> +
>> +properties:
>> +  compatible:
>> +    const: reg-external-output
>> +
>> +  vout-supply:
>> +    description:
>> +      Phandle of the regulator supplying the output, which should have
>> +      the regulator-external-output property.
>> +
>> +required:
>
>I think at a minimum anything like this would need some sort of
>representation of how the output physically appears so that people can
>work out how outputs are mapped to the hardware they see.

I don't quite understand what you're describing here -- could you 
elaborate on what you mean by "how the output physically appears", and 
what that might look like in a DT binding?

>However we
>already have a subsystem for external connectors - extcon.  Perhaps this
>should be a regulator client in the extcon API?  It's common for
>connectors to include some sort of power provision so it seems like this
>would fit right in.

Interesting -- I wasn't previously aware of the extcon subsystem, thanks 
for the pointer.  However, after looking at it a bit, I'm not sure I see 
how it'd be applicable here, since it looks like it's built to handle 
hardware that's at least sophisticated enough for software to tell 
whether or not something's plugged in, which isn't the case here.  The 
connector is just a ground pin and +12VDC pin, no presence-detection 
mechanism or anything else.  Outside of the regulator itself there's 
really no "device" there for software to talk to or otherwise interact 
with at all.


Thanks,
Zev

Re: [PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Mark Brown]

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

On Wed, May 04, 2022 at 01:33:58PM -0700, Zev Weiss wrote:
> On Wed, May 04, 2022 at 05:55:53AM PDT, Mark Brown wrote:

> > I think at a minimum anything like this would need some sort of
> > representation of how the output physically appears so that people can
> > work out how outputs are mapped to the hardware they see.

> I don't quite understand what you're describing here -- could you elaborate
> on what you mean by "how the output physically appears", and what that might
> look like in a DT binding?

For example if the output comes out on a socket then that socket should
be described.

> > However we
> > already have a subsystem for external connectors - extcon.  Perhaps this
> > should be a regulator client in the extcon API?  It's common for
> > connectors to include some sort of power provision so it seems like this
> > would fit right in.

> Interesting -- I wasn't previously aware of the extcon subsystem, thanks for
> the pointer.  However, after looking at it a bit, I'm not sure I see how
> it'd be applicable here, since it looks like it's built to handle hardware
> that's at least sophisticated enough for software to tell whether or not
> something's plugged in, which isn't the case here.  The connector is just a
> ground pin and +12VDC pin, no presence-detection mechanism or anything else.
> Outside of the regulator itself there's really no "device" there for
> software to talk to or otherwise interact with at all.

Sure, but there's no reason why it can't scale down to something
simpler.  It's easier to support something simpler than have to extend
to support something more complicated.

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

Re: (subset) [PATCH 1/6] dt-bindings: regulator: Add regulator-external-output property

[Mark Brown]

On Tue, 3 May 2022 23:52:47 -0700, Zev Weiss wrote:
> Some regulators do not provide power to anything within the system
> described by a device tree, and simply supply an external output.  The
> regulator-external-output property can now be used to mark such
> regulators.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[3/6] regulator: core: Add error flags to sysfs attributes
      commit: 0f2d636e7d1fd76f704dd3ea5089ce29a8aee049

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Zev Weiss]

[Adding extcon maintainers...]

On Wed, May 04, 2022 at 01:49:12PM PDT, Mark Brown wrote:
>On Wed, May 04, 2022 at 01:33:58PM -0700, Zev Weiss wrote:
>> On Wed, May 04, 2022 at 05:55:53AM PDT, Mark Brown wrote:
>
>> > I think at a minimum anything like this would need some sort of
>> > representation of how the output physically appears so that people can
>> > work out how outputs are mapped to the hardware they see.
>
>> I don't quite understand what you're describing here -- could you elaborate
>> on what you mean by "how the output physically appears", and what that might
>> look like in a DT binding?
>
>For example if the output comes out on a socket then that socket should
>be described.
>

Okay -- in the case of an Open19 power shelf like the ahe-50dc, there 
are 50 instances of this, 48 of which are in four ganged connectors each 
with 12 pairs of pins, but two of which have their own dedicated little 
individual sockets.  The connectors are physically different, but 
they're all identical as far as software is concerned, so I'm not clear 
on why it would need to be expressed in any DT properties or the like.  
Or did you just mean explanatory free-form text in the description 
field?

>> > However we
>> > already have a subsystem for external connectors - extcon.  Perhaps this
>> > should be a regulator client in the extcon API?  It's common for
>> > connectors to include some sort of power provision so it seems like this
>> > would fit right in.
>
>> Interesting -- I wasn't previously aware of the extcon subsystem, thanks for
>> the pointer.  However, after looking at it a bit, I'm not sure I see how
>> it'd be applicable here, since it looks like it's built to handle hardware
>> that's at least sophisticated enough for software to tell whether or not
>> something's plugged in, which isn't the case here.  The connector is just a
>> ground pin and +12VDC pin, no presence-detection mechanism or anything else.
>> Outside of the regulator itself there's really no "device" there for
>> software to talk to or otherwise interact with at all.
>
>Sure, but there's no reason why it can't scale down to something
>simpler.  It's easier to support something simpler than have to extend
>to support something more complicated.

Alright, so would you suggest creating something like 
drivers/extcon/extcon-regulator-output.c, and just having its extcon 
functionality be something of a stub for now?


Thanks,
Zev

Re: [PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Krzysztof Kozlowski]

On 04/05/2022 22:33, Zev Weiss wrote:
> On Wed, May 04, 2022 at 05:55:53AM PDT, Mark Brown wrote:
>> On Tue, May 03, 2022 at 11:52:48PM -0700, Zev Weiss wrote:
>>> This describes an external output supplied by a regulator, such as a
>>> power outlet on a power distribution unit (PDU).
>>
>> OK, so this is that represnetation of the connection - which raises the
>> question about why the regulator needs a property?

This remained unanswered... the binding looks like approach the problem
from wrong side. It's like defining consumers of clock inside clock
controller. It does not scale, it's not in Devicetree style/intentions.

You should have a dedicated port or connector which takes a regulator as
a supply (and for supply we already have bindings).

We already have only one connector:
git grep connector -- Documentation/devicetree/


(extcon is not the best choice for bindings, it's tied to Linux
implementation, not to hardware)

Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: regulator: Add reg-external-output binding

[Mark Brown]

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

On Wed, May 04, 2022 at 02:35:08PM -0700, Zev Weiss wrote:
> On Wed, May 04, 2022 at 01:49:12PM PDT, Mark Brown wrote:

> > For example if the output comes out on a socket then that socket should
> > be described.

> Okay -- in the case of an Open19 power shelf like the ahe-50dc, there are 50
> instances of this, 48 of which are in four ganged connectors each with 12
> pairs of pins, but two of which have their own dedicated little individual
> sockets.  The connectors are physically different, but they're all identical
> as far as software is concerned, so I'm not clear on why it would need to be
> expressed in any DT properties or the like.  Or did you just mean
> explanatory free-form text in the description field?

Explanatory description would be one example of something that should be
described, yes - if there is control for switching the supplies on and
off the user is going to need to be able to figure out which software
control corresponds to which physical connection.  Other devices may
have more things that need autoamtion (eg, jack detection).

> > Sure, but there's no reason why it can't scale down to something
> > simpler.  It's easier to support something simpler than have to extend
> > to support something more complicated.

> Alright, so would you suggest creating something like
> drivers/extcon/extcon-regulator-output.c, and just having its extcon
> functionality be something of a stub for now?

I'm not super familiar with extcon but that seems plausible.

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