[PATCH v2 0/2] extcon: Add extcon-regulator driver

[PATCH v2 0/2] extcon: Add extcon-regulator driver

[Zev Weiss]

Hello,

This is a restructured v2 of a series adding support for
regulator-supplied power output connectors.

Changes since v1 [0, 1]:
 - Dropped regulator-external-output DT property [Mark]
 - Moved DT binding to connector/regulator-connector.yaml [Krzysztof]
 - Dropped EXTERNAL_GET type and other regulator-core changes

This driver aims to support hardware where a regulator supplies a
power output connector, such as a power outlet on a DC power
distribution unit (PDU).  (The specific system I'm targeting at the
moment is the Delta AHE-50DC Open19 power shelf [2], for which I'm
working on a port of OpenBMC.)

Patch 1 adds the regulator-connector DT binding, with a single
property specifying the regulator supplying the connector's output
power, and patch 2 adds an extcon driver (extcon-regulator).  The
driver currently doesn't actually do much extcon-wise, because the
AHE-50DC hardware is quite minimal and doesn't really offer much for
it to do, but if other similar devices in the future do have
additional functionality to support (such as a presence-detection
mechanism) the driver can be extended to add that.


Thanks,
Zev

[0] https://lore.kernel.org/openbmc/20220504065041.6718-1-zev@bewilderbeest.net/
[1] https://lore.kernel.org/openbmc/20220504065252.6955-1-zev@bewilderbeest.net/
[2] https://www.open19.org/marketplace/delta-16kw-power-shelf/

Zev Weiss (2):
  dt-bindings: connector: Add regulator-connector binding
  extcon: Add extcon-regulator driver

 .../ABI/testing/sysfs-driver-extcon-regulator |   8 ++
 .../connector/regulator-connector.yaml        |  38 +++++
 MAINTAINERS                                   |   8 ++
 drivers/extcon/Kconfig                        |   8 ++
 drivers/extcon/Makefile                       |   1 +
 drivers/extcon/extcon-regulator.c             | 133 ++++++++++++++++++
 6 files changed, 196 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-extcon-regulator
 create mode 100644 Documentation/devicetree/bindings/connector/regulator-connector.yaml
 create mode 100644 drivers/extcon/extcon-regulator.c

-- 
2.36.0

[PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding

[Zev Weiss]

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

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

diff --git a/Documentation/devicetree/bindings/connector/regulator-connector.yaml b/Documentation/devicetree/bindings/connector/regulator-connector.yaml
new file mode 100644
index 000000000000..96825b6f608a
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/regulator-connector.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/connector/regulator-connector.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Regulator output connector
+
+maintainers:
+  - Zev Weiss <zev@bewilderbeest.net>
+
+description: |
+  This describes a power 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-connector
+
+  vout-supply:
+    description:
+      Phandle of the regulator supplying the connector.
+
+required:
+  - compatible
+  - vout-supply
+
+additionalProperties: false
+
+examples:
+  - |
+      output {
+          compatible = "regulator-connector";
+          vout-supply = <&output_reg>;
+      };
-- 
2.36.0

[PATCH v2 2/2] extcon: Add extcon-regulator driver

[Zev Weiss]

This driver supports power connectors supplied by a regulator, such as
outlets on a power distribution unit (PDU).

Its extcon functionality is currently quite limited, since the
hardware it's initially targeting is very simple and doesn't really
provide anything for the driver to interact with, but it can be
extended as required for hardware that might offer more for it to do
(e.g. a presence-detection mechanism).

Its sole feature is a read/write sysfs attribute allowing userspace to
switch the output on and off by enabling and disabling the supply
regulator.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../ABI/testing/sysfs-driver-extcon-regulator |   8 ++
 MAINTAINERS                                   |   8 ++
 drivers/extcon/Kconfig                        |   8 ++
 drivers/extcon/Makefile                       |   1 +
 drivers/extcon/extcon-regulator.c             | 133 ++++++++++++++++++
 5 files changed, 158 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-extcon-regulator
 create mode 100644 drivers/extcon/extcon-regulator.c

diff --git a/Documentation/ABI/testing/sysfs-driver-extcon-regulator b/Documentation/ABI/testing/sysfs-driver-extcon-regulator
new file mode 100644
index 000000000000..b2f3141a1c49
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-extcon-regulator
@@ -0,0 +1,8 @@
+What:		/sys/bus/platform/drivers/extcon-regulator/*/state
+Date:		May 2022
+KernelVersion:	5.18
+Contact:	Zev Weiss <zev@bewilderbeest.net>
+Description:	When read, provides the current power state of the connector,
+		either "on" or "off".  Either string may also be written to
+		set the power state of the connector.
+Users:		OpenBMC <openbmc@lists.ozlabs.org>
diff --git a/MAINTAINERS b/MAINTAINERS
index edc96cdb85e8..c30b6cf95ff1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16740,6 +16740,14 @@ F:	Documentation/devicetree/bindings/regmap/
 F:	drivers/base/regmap/
 F:	include/linux/regmap.h
 
+REGULATOR EXTCON DRIVER
+M:	Zev Weiss <zev@bewilderbeest.net>
+L:	openbmc@lists.ozlabs.org
+S:	Maintained
+F:	Documentation/ABI/testing/sysfs-driver-extcon-regulator
+F:	Documentation/devicetree/bindings/connector/regulator-connector.yaml
+F:	drivers/extcon/extcon-regulator.c
+
 REISERFS FILE SYSTEM
 L:	reiserfs-devel@vger.kernel.org
 S:	Supported
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 0d42e49105dd..19fe76da6c75 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -143,6 +143,14 @@ config EXTCON_QCOM_SPMI_MISC
 	  Say Y here to enable SPMI PMIC based USB cable detection
 	  support on Qualcomm PMICs such as PM8941.
 
+config EXTCON_REGULATOR
+	tristate "Regulator output extcon support"
+	depends on REGULATOR
+	help
+	  Say y here to enable support for regulator-supplied external
+	  power output connections, such as the outlets of a power
+	  distribution unit (PDU).
+
 config EXTCON_RT8973A
 	tristate "Richtek RT8973A EXTCON support"
 	depends on I2C
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 1b390d934ca9..1a1c32d4b23e 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
 obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
+obj-$(CONFIG_EXTCON_REGULATOR)	+= extcon-regulator.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
 obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
diff --git a/drivers/extcon/extcon-regulator.c b/drivers/extcon/extcon-regulator.c
new file mode 100644
index 000000000000..eec1bb3f4c09
--- /dev/null
+++ b/drivers/extcon/extcon-regulator.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * extcon-regulator: extcon driver for regulator-supplied external power
+ * output connectors
+ *
+ * Copyright (C) 2022 Zev Weiss <zev@bewilderbeest.net>
+ */
+
+#include <linux/extcon-provider.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+struct regulator_extcon_data {
+	struct extcon_dev *edev;
+	struct regulator *reg;
+	struct mutex lock;
+	unsigned int extcon_id;
+};
+
+static ssize_t state_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct regulator_extcon_data *data = dev_get_drvdata(dev);
+	int status = regulator_is_enabled(data->reg);
+
+	if (status < 0)
+		return status;
+
+	return sysfs_emit(buf, "%s\n", status ? "on" : "off");
+}
+
+static ssize_t state_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			   size_t count)
+{
+	int status, wantstate;
+	struct regulator_extcon_data *data = dev_get_drvdata(dev);
+	struct regulator *reg = data->reg;
+
+	if (sysfs_streq(buf, "on"))
+		wantstate = 1;
+	else if (sysfs_streq(buf, "off"))
+		wantstate = 0;
+	else
+		return -EINVAL;
+
+	mutex_lock(&data->lock);
+
+	status = regulator_is_enabled(reg);
+
+	/*
+	 * We need to ensure our enable/disable calls don't get imbalanced, so
+	 * bail if we can't determine the current state.
+	 */
+	if (status < 0)
+		goto out;
+
+	/* Nothing further needed if we're already in the desired state */
+	if (!!status == wantstate) {
+		status = 0;
+		goto out;
+	}
+
+	if (wantstate)
+		status = regulator_enable(reg);
+	else
+		status = regulator_disable(reg);
+
+out:
+	mutex_unlock(&data->lock);
+
+	return status ? : count;
+}
+
+static DEVICE_ATTR_RW(state);
+
+static struct attribute *regulator_extcon_attrs[] = {
+	&dev_attr_state.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(regulator_extcon);
+
+static int regulator_extcon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct regulator_extcon_data *data;
+	struct device *dev = &pdev->dev;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->reg = devm_regulator_get_exclusive(&pdev->dev, "vout");
+	if (IS_ERR(data->reg))
+		return PTR_ERR(data->reg);
+
+	mutex_init(&data->lock);
+
+	/* No cables currently supported */
+	data->extcon_id = EXTCON_NONE;
+
+	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
+	if (IS_ERR(data->edev))
+		return PTR_ERR(data->edev);
+
+	ret = devm_extcon_dev_register(dev, data->edev);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static const struct of_device_id regulator_extcon_of_match_table[] = {
+	{ .compatible = "regulator-connector" },
+	{ },
+};
+
+static struct platform_driver regulator_extcon_driver = {
+	.driver = {
+		.name = "extcon-regulator",
+		.of_match_table = of_match_ptr(regulator_extcon_of_match_table),
+		.dev_groups = regulator_extcon_groups,
+	},
+	.probe = regulator_extcon_probe,
+};
+module_platform_driver(regulator_extcon_driver);
+
+MODULE_AUTHOR("Zev Weiss <zev@bewilderbeest.net>");
+MODULE_DESCRIPTION("Regulator extcon driver");
+MODULE_LICENSE("GPL");
-- 
2.36.0

Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver

[Chanwoo Choi]

Hi Zev,

I checked this patch. But, it doesn't look like the extcon provider
driver. Because basically, extcon provider driver need the circuit
in order to detect the kind of external connector. But, there are
no any code for detection. Just add the specific sysfs attribute
for only this driver. It is not standard interface.

Thanks,
Chanwoo Choi

On 22. 5. 6. 08:25, Zev Weiss wrote:
> This driver supports power connectors supplied by a regulator, such as
> outlets on a power distribution unit (PDU).
> 
> Its extcon functionality is currently quite limited, since the
> hardware it's initially targeting is very simple and doesn't really
> provide anything for the driver to interact with, but it can be
> extended as required for hardware that might offer more for it to do
> (e.g. a presence-detection mechanism).
> 
> Its sole feature is a read/write sysfs attribute allowing userspace to
> switch the output on and off by enabling and disabling the supply
> regulator.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>   .../ABI/testing/sysfs-driver-extcon-regulator |   8 ++
>   MAINTAINERS                                   |   8 ++
>   drivers/extcon/Kconfig                        |   8 ++
>   drivers/extcon/Makefile                       |   1 +
>   drivers/extcon/extcon-regulator.c             | 133 ++++++++++++++++++
>   5 files changed, 158 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-driver-extcon-regulator
>   create mode 100644 drivers/extcon/extcon-regulator.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-extcon-regulator b/Documentation/ABI/testing/sysfs-driver-extcon-regulator
> new file mode 100644
> index 000000000000..b2f3141a1c49
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-extcon-regulator
> @@ -0,0 +1,8 @@
> +What:		/sys/bus/platform/drivers/extcon-regulator/*/state
> +Date:		May 2022
> +KernelVersion:	5.18
> +Contact:	Zev Weiss <zev@bewilderbeest.net>
> +Description:	When read, provides the current power state of the connector,
> +		either "on" or "off".  Either string may also be written to
> +		set the power state of the connector.
> +Users:		OpenBMC <openbmc@lists.ozlabs.org>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index edc96cdb85e8..c30b6cf95ff1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16740,6 +16740,14 @@ F:	Documentation/devicetree/bindings/regmap/
>   F:	drivers/base/regmap/
>   F:	include/linux/regmap.h
>   
> +REGULATOR EXTCON DRIVER
> +M:	Zev Weiss <zev@bewilderbeest.net>
> +L:	openbmc@lists.ozlabs.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-driver-extcon-regulator
> +F:	Documentation/devicetree/bindings/connector/regulator-connector.yaml
> +F:	drivers/extcon/extcon-regulator.c
> +
>   REISERFS FILE SYSTEM
>   L:	reiserfs-devel@vger.kernel.org
>   S:	Supported
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 0d42e49105dd..19fe76da6c75 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -143,6 +143,14 @@ config EXTCON_QCOM_SPMI_MISC
>   	  Say Y here to enable SPMI PMIC based USB cable detection
>   	  support on Qualcomm PMICs such as PM8941.
>   
> +config EXTCON_REGULATOR
> +	tristate "Regulator output extcon support"
> +	depends on REGULATOR
> +	help
> +	  Say y here to enable support for regulator-supplied external
> +	  power output connections, such as the outlets of a power
> +	  distribution unit (PDU).
> +
>   config EXTCON_RT8973A
>   	tristate "Richtek RT8973A EXTCON support"
>   	depends on I2C
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 1b390d934ca9..1a1c32d4b23e 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>   obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>   obj-$(CONFIG_EXTCON_PTN5150)	+= extcon-ptn5150.o
>   obj-$(CONFIG_EXTCON_QCOM_SPMI_MISC) += extcon-qcom-spmi-misc.o
> +obj-$(CONFIG_EXTCON_REGULATOR)	+= extcon-regulator.o
>   obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>   obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
>   obj-$(CONFIG_EXTCON_USB_GPIO)	+= extcon-usb-gpio.o
> diff --git a/drivers/extcon/extcon-regulator.c b/drivers/extcon/extcon-regulator.c
> new file mode 100644
> index 000000000000..eec1bb3f4c09
> --- /dev/null
> +++ b/drivers/extcon/extcon-regulator.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * extcon-regulator: extcon driver for regulator-supplied external power
> + * output connectors
> + *
> + * Copyright (C) 2022 Zev Weiss <zev@bewilderbeest.net>
> + */
> +
> +#include <linux/extcon-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct regulator_extcon_data {
> +	struct extcon_dev *edev;
> +	struct regulator *reg;
> +	struct mutex lock;
> +	unsigned int extcon_id;
> +};
> +
> +static ssize_t state_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct regulator_extcon_data *data = dev_get_drvdata(dev);
> +	int status = regulator_is_enabled(data->reg);
> +
> +	if (status < 0)
> +		return status;
> +
> +	return sysfs_emit(buf, "%s\n", status ? "on" : "off");
> +}
> +
> +static ssize_t state_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			   size_t count)
> +{
> +	int status, wantstate;
> +	struct regulator_extcon_data *data = dev_get_drvdata(dev);
> +	struct regulator *reg = data->reg;
> +
> +	if (sysfs_streq(buf, "on"))
> +		wantstate = 1;
> +	else if (sysfs_streq(buf, "off"))
> +		wantstate = 0;
> +	else
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +
> +	status = regulator_is_enabled(reg);
> +
> +	/*
> +	 * We need to ensure our enable/disable calls don't get imbalanced, so
> +	 * bail if we can't determine the current state.
> +	 */
> +	if (status < 0)
> +		goto out;
> +
> +	/* Nothing further needed if we're already in the desired state */
> +	if (!!status == wantstate) {
> +		status = 0;
> +		goto out;
> +	}
> +
> +	if (wantstate)
> +		status = regulator_enable(reg);
> +	else
> +		status = regulator_disable(reg);
> +
> +out:
> +	mutex_unlock(&data->lock);
> +
> +	return status ? : count;
> +}
> +
> +static DEVICE_ATTR_RW(state);
> +
> +static struct attribute *regulator_extcon_attrs[] = {
> +	&dev_attr_state.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(regulator_extcon);
> +
> +static int regulator_extcon_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct regulator_extcon_data *data;
> +	struct device *dev = &pdev->dev;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->reg = devm_regulator_get_exclusive(&pdev->dev, "vout");
> +	if (IS_ERR(data->reg))
> +		return PTR_ERR(data->reg);
> +
> +	mutex_init(&data->lock);
> +
> +	/* No cables currently supported */
> +	data->extcon_id = EXTCON_NONE;
> +
> +	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
> +	if (IS_ERR(data->edev))
> +		return PTR_ERR(data->edev);
> +
> +	ret = devm_extcon_dev_register(dev, data->edev);
> +	if (ret < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id regulator_extcon_of_match_table[] = {
> +	{ .compatible = "regulator-connector" },
> +	{ },
> +};
> +
> +static struct platform_driver regulator_extcon_driver = {
> +	.driver = {
> +		.name = "extcon-regulator",
> +		.of_match_table = of_match_ptr(regulator_extcon_of_match_table),
> +		.dev_groups = regulator_extcon_groups,
> +	},
> +	.probe = regulator_extcon_probe,
> +};
> +module_platform_driver(regulator_extcon_driver);
> +
> +MODULE_AUTHOR("Zev Weiss <zev@bewilderbeest.net>");
> +MODULE_DESCRIPTION("Regulator extcon driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver

[Mark Brown]

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

On Mon, May 09, 2022 at 09:24:39PM +0900, Chanwoo Choi wrote:
> Hi Zev,
> 
> I checked this patch. But, it doesn't look like the extcon provider
> driver. Because basically, extcon provider driver need the circuit
> in order to detect the kind of external connector. But, there are
> no any code for detection. Just add the specific sysfs attribute
> for only this driver. It is not standard interface.

OTOH it's something where if I look at the physical system with the
hardware there's a clearly visible external connector that I can point
to - it just happens to not support hotplug.  It's not clear what other
system it would sit in, and it seems like an application that displays
external connections on a system in a UI would be able to do something
sensible with it.

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

Re: [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding

[Rob Herring]

On Thu, 05 May 2022 16:25:56 -0700, Zev Weiss wrote:
> This describes a power connector supplied by a regulator, such as a
> power outlet on a power distribution unit (PDU).
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  .../connector/regulator-connector.yaml        | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/connector/regulator-connector.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver

[Zev Weiss]

[Adding Sebastian for drivers/power discussion]

On Mon, May 09, 2022 at 08:24:16AM PDT, Mark Brown wrote:
> On Mon, May 09, 2022 at 09:24:39PM +0900, Chanwoo Choi wrote:
> > Hi Zev,
> > 
> > I checked this patch. But, it doesn't look like the extcon provider
> > driver. Because basically, extcon provider driver need the circuit
> > in order to detect the kind of external connector. But, there are
> > no any code for detection. Just add the specific sysfs attribute
> > for only this driver. It is not standard interface.
> 
> OTOH it's something where if I look at the physical system with the
> hardware there's a clearly visible external connector that I can point
> to - it just happens to not support hotplug.  It's not clear what other
> system it would sit in, and it seems like an application that displays
> external connections on a system in a UI would be able to do something
> sensible with it.

Chanwoo, any further thoughts on Mark's reasoning above?

I certainly understand the reluctance to add an extcon driver that 
doesn't really do anything with the extcon API, and I have no idea when 
we might end up enhancing it to do something more meaningful with that 
API (I don't know of any hardware at the moment that would need it).

That said, as Mark points out, the hardware *is* ultimately an "external 
connector" (if a very simplistic one).

Do you have any other ideas for where this functionality could go?  Greg 
wasn't enthusiastic about a previous revision that had it in 
drivers/misc -- though now a fair amount of what was in that version is 
now going to be in the regulator core, so maybe that could be 
reconsidered?

Or maybe something under drivers/power, though it's not really a supply 
or a reset device...drivers/power/output.c or something?

Personally I don't have any terribly strong opinions on this, I'd just 
like to reach a mutually-agreeable consensus on a place for it to live.


Thanks,
Zev

Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver

[Chanwoo Choi]

Hi Mark, Zev,

On 5/17/22 10:03 AM, Zev Weiss wrote:
> [Adding Sebastian for drivers/power discussion]
> 
> On Mon, May 09, 2022 at 08:24:16AM PDT, Mark Brown wrote:
>> On Mon, May 09, 2022 at 09:24:39PM +0900, Chanwoo Choi wrote:
>>> Hi Zev,
>>>
>>> I checked this patch. But, it doesn't look like the extcon provider
>>> driver. Because basically, extcon provider driver need the circuit
>>> in order to detect the kind of external connector. But, there are
>>> no any code for detection. Just add the specific sysfs attribute
>>> for only this driver. It is not standard interface.
>>
>> OTOH it's something where if I look at the physical system with the
>> hardware there's a clearly visible external connector that I can point
>> to - it just happens to not support hotplug.  It's not clear what other
>> system it would sit in, and it seems like an application that displays
>> external connections on a system in a UI would be able to do something
>> sensible with it.
> 
> Chanwoo, any further thoughts on Mark's reasoning above?
> 
> I certainly understand the reluctance to add an extcon driver that 
> doesn't really do anything with the extcon API, and I have no idea when 
> we might end up enhancing it to do something more meaningful with that 
> API (I don't know of any hardware at the moment that would need it).
> 
> That said, as Mark points out, the hardware *is* ultimately an "external 
> connector" (if a very simplistic one).
> 
> Do you have any other ideas for where this functionality could go?  Greg 
> wasn't enthusiastic about a previous revision that had it in 
> drivers/misc -- though now a fair amount of what was in that version is 
> now going to be in the regulator core, so maybe that could be 
> reconsidered?
> 
> Or maybe something under drivers/power, though it's not really a supply 
> or a reset device...drivers/power/output.c or something?
> 
> Personally I don't have any terribly strong opinions on this, I'd just 
> like to reach a mutually-agreeable consensus on a place for it to live.
> 

After Mark's reply, I considered extcon provider driver without hotplug
feature. I think that extcon need to support the following something:

1. Need to specify the type of external connector instead of EXTCON_NONE.
2. extcon subsystem provides the standard sysfs interface
   for non-hotplug extcon provider driver.
3. User can control the state of external connector via
   the standard extcon sysfs attributes.


For example of extcon provider driver,
static const unsigned int supported_cables[] = {
	EXTCON_USB,
	EXTCON_NONE,
};

int extcon_usb_callback(int connector_id, int property_id, int state, void *data) {
	struct extcon_dev *edev = data;

	if (id == EXTCON_USB && property_id == EXTCON_NOT_HOTPLUG) {
		regulator_enable() or regulator_disable()
	}

	return 0;
}

int extcon_provider_probe(...) {
	edev = devm_extcon_dev_allocate(dev, supported_cables);

	devm_extcon_dev_register(dev, edev);

	extcon_set_property_capability(edev, EXTCON_USB, EXTCON_NOT_HOTPLUG);
	extcon_set_property_callback(edev, EXTCON_USB, extcon_usb_callback);

	...
}

And then user can change the state of each external connector
via '/sys/class/extcon/extcon0/cable.0/state' 
if cable.0 contains the EXTCON_NOT_HOTPLUG property.

I'm designing this approach. But it has not yet decided
because try to check that this approach is right or not.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver

[Zev Weiss]

On Mon, May 16, 2022 at 08:15:31PM PDT, Chanwoo Choi wrote:
> Hi Mark, Zev,
> 
> On 5/17/22 10:03 AM, Zev Weiss wrote:
> > [Adding Sebastian for drivers/power discussion]
> > 
> > On Mon, May 09, 2022 at 08:24:16AM PDT, Mark Brown wrote:
> >> On Mon, May 09, 2022 at 09:24:39PM +0900, Chanwoo Choi wrote:
> >>> Hi Zev,
> >>>
> >>> I checked this patch. But, it doesn't look like the extcon provider
> >>> driver. Because basically, extcon provider driver need the circuit
> >>> in order to detect the kind of external connector. But, there are
> >>> no any code for detection. Just add the specific sysfs attribute
> >>> for only this driver. It is not standard interface.
> >>
> >> OTOH it's something where if I look at the physical system with the
> >> hardware there's a clearly visible external connector that I can point
> >> to - it just happens to not support hotplug.  It's not clear what other
> >> system it would sit in, and it seems like an application that displays
> >> external connections on a system in a UI would be able to do something
> >> sensible with it.
> > 
> > Chanwoo, any further thoughts on Mark's reasoning above?
> > 
> > I certainly understand the reluctance to add an extcon driver that 
> > doesn't really do anything with the extcon API, and I have no idea when 
> > we might end up enhancing it to do something more meaningful with that 
> > API (I don't know of any hardware at the moment that would need it).
> > 
> > That said, as Mark points out, the hardware *is* ultimately an "external 
> > connector" (if a very simplistic one).
> > 
> > Do you have any other ideas for where this functionality could go?  Greg 
> > wasn't enthusiastic about a previous revision that had it in 
> > drivers/misc -- though now a fair amount of what was in that version is 
> > now going to be in the regulator core, so maybe that could be 
> > reconsidered?
> > 
> > Or maybe something under drivers/power, though it's not really a supply 
> > or a reset device...drivers/power/output.c or something?
> > 
> > Personally I don't have any terribly strong opinions on this, I'd just 
> > like to reach a mutually-agreeable consensus on a place for it to live.
> > 
> 
> After Mark's reply, I considered extcon provider driver without hotplug
> feature. I think that extcon need to support the following something:
> 
> 1. Need to specify the type of external connector instead of EXTCON_NONE.
> 2. extcon subsystem provides the standard sysfs interface
>    for non-hotplug extcon provider driver.
> 3. User can control the state of external connector via
>    the standard extcon sysfs attributes.
> 
> 
> For example of extcon provider driver,
> static const unsigned int supported_cables[] = {
> 	EXTCON_USB,
> 	EXTCON_NONE,
> };
> 
> int extcon_usb_callback(int connector_id, int property_id, int state, void *data) {
> 	struct extcon_dev *edev = data;
> 
> 	if (id == EXTCON_USB && property_id == EXTCON_NOT_HOTPLUG) {
> 		regulator_enable() or regulator_disable()
> 	}
> 
> 	return 0;
> }
> 
> int extcon_provider_probe(...) {
> 	edev = devm_extcon_dev_allocate(dev, supported_cables);
> 
> 	devm_extcon_dev_register(dev, edev);
> 
> 	extcon_set_property_capability(edev, EXTCON_USB, EXTCON_NOT_HOTPLUG);
> 	extcon_set_property_callback(edev, EXTCON_USB, extcon_usb_callback);
> 
> 	...
> }
> 
> And then user can change the state of each external connector
> via '/sys/class/extcon/extcon0/cable.0/state' 
> if cable.0 contains the EXTCON_NOT_HOTPLUG property.
> 
> I'm designing this approach. But it has not yet decided
> because try to check that this approach is right or not.
> 

Okay, so if I'm understanding correctly we'd be using the extcon 
subsystem's existing attached/detached state to model (and control) the 
on/off state of the power output?

That could work for the particular hardware I'm dealing with at the 
moment, though I'd be a bit concerned that conflating the two might 
constrain things in the future if there's some similar but slightly more 
sophisticated hardware we'd want to extend the same driver to support.  
For example on a power connector with some capability for presence 
detection, we might want to be able to support "attached but powered 
off" as a valid state for it to be in -- would the above approach be 
able to do that?


Thanks,
Zev

Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver

[Chanwoo Choi]

On 22. 5. 17. 19:07, Zev Weiss wrote:
> On Mon, May 16, 2022 at 08:15:31PM PDT, Chanwoo Choi wrote:
>> Hi Mark, Zev,
>>
>> On 5/17/22 10:03 AM, Zev Weiss wrote:
>>> [Adding Sebastian for drivers/power discussion]
>>>
>>> On Mon, May 09, 2022 at 08:24:16AM PDT, Mark Brown wrote:
>>>> On Mon, May 09, 2022 at 09:24:39PM +0900, Chanwoo Choi wrote:
>>>>> Hi Zev,
>>>>>
>>>>> I checked this patch. But, it doesn't look like the extcon provider
>>>>> driver. Because basically, extcon provider driver need the circuit
>>>>> in order to detect the kind of external connector. But, there are
>>>>> no any code for detection. Just add the specific sysfs attribute
>>>>> for only this driver. It is not standard interface.
>>>>
>>>> OTOH it's something where if I look at the physical system with the
>>>> hardware there's a clearly visible external connector that I can point
>>>> to - it just happens to not support hotplug.  It's not clear what other
>>>> system it would sit in, and it seems like an application that displays
>>>> external connections on a system in a UI would be able to do something
>>>> sensible with it.
>>>
>>> Chanwoo, any further thoughts on Mark's reasoning above?
>>>
>>> I certainly understand the reluctance to add an extcon driver that 
>>> doesn't really do anything with the extcon API, and I have no idea when 
>>> we might end up enhancing it to do something more meaningful with that 
>>> API (I don't know of any hardware at the moment that would need it).
>>>
>>> That said, as Mark points out, the hardware *is* ultimately an "external 
>>> connector" (if a very simplistic one).
>>>
>>> Do you have any other ideas for where this functionality could go?  Greg 
>>> wasn't enthusiastic about a previous revision that had it in 
>>> drivers/misc -- though now a fair amount of what was in that version is 
>>> now going to be in the regulator core, so maybe that could be 
>>> reconsidered?
>>>
>>> Or maybe something under drivers/power, though it's not really a supply 
>>> or a reset device...drivers/power/output.c or something?
>>>
>>> Personally I don't have any terribly strong opinions on this, I'd just 
>>> like to reach a mutually-agreeable consensus on a place for it to live.
>>>
>>
>> After Mark's reply, I considered extcon provider driver without hotplug
>> feature. I think that extcon need to support the following something:
>>
>> 1. Need to specify the type of external connector instead of EXTCON_NONE.
>> 2. extcon subsystem provides the standard sysfs interface
>>    for non-hotplug extcon provider driver.
>> 3. User can control the state of external connector via
>>    the standard extcon sysfs attributes.
>>
>>
>> For example of extcon provider driver,
>> static const unsigned int supported_cables[] = {
>> 	EXTCON_USB,
>> 	EXTCON_NONE,
>> };
>>
>> int extcon_usb_callback(int connector_id, int property_id, int state, void *data) {
>> 	struct extcon_dev *edev = data;
>>
>> 	if (id == EXTCON_USB && property_id == EXTCON_NOT_HOTPLUG) {
>> 		regulator_enable() or regulator_disable()
>> 	}
>>
>> 	return 0;
>> }
>>
>> int extcon_provider_probe(...) {
>> 	edev = devm_extcon_dev_allocate(dev, supported_cables);
>>
>> 	devm_extcon_dev_register(dev, edev);
>>
>> 	extcon_set_property_capability(edev, EXTCON_USB, EXTCON_NOT_HOTPLUG);
>> 	extcon_set_property_callback(edev, EXTCON_USB, extcon_usb_callback);
>>
>> 	...
>> }
>>
>> And then user can change the state of each external connector
>> via '/sys/class/extcon/extcon0/cable.0/state' 
>> if cable.0 contains the EXTCON_NOT_HOTPLUG property.
>>
>> I'm designing this approach. But it has not yet decided
>> because try to check that this approach is right or not.
>>
> 
> Okay, so if I'm understanding correctly we'd be using the extcon 
> subsystem's existing attached/detached state to model (and control) the 
> on/off state of the power output?

The extcon provides the sysfs interface to control the state of each cable
and then passes the extra role to each extcon provider driver with using
the registered callback. The extcon core don't care the detailed operation
in registered callback. Just provide the interface from sysfs interface
to extcon provider driver and then handle the state of external connector.

In your case, you might enable/disable the regulator on the registered callback
like extcon_usb_callback in example.

> 
> That could work for the particular hardware I'm dealing with at the 
> moment, though I'd be a bit concerned that conflating the two might 
> constrain things in the future if there's some similar but slightly more 
> sophisticated hardware we'd want to extend the same driver to support.  
> For example on a power connector with some capability for presence 
> detection, we might want to be able to support "attached but powered 
> off" as a valid state for it to be in -- would the above approach be 
> able to do that?

Yes. As I mentioned above, the regulator control is extra role
for the specific extcon provider driver. Any extcon provider driver
without hotplug feature might control the audio volume in the registered
callback like extcon_usb_callback in example. The extra job depends
on the extcon provider driver.

> 
> 
> Thanks,
> Zev
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi