* [PATCH v2 0/2] extcon: Add extcon-regulator driver @ 2022-05-05 23:25 Zev Weiss 2022-05-05 23:25 ` [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding Zev Weiss 2022-05-05 23:25 ` [PATCH v2 2/2] extcon: Add extcon-regulator driver Zev Weiss 0 siblings, 2 replies; 10+ messages in thread From: Zev Weiss @ 2022-05-05 23:25 UTC (permalink / raw) To: Mark Brown, Liam Girdwood, MyungJoo Ham, Chanwoo Choi Cc: Zev Weiss, linux-kernel, Greg Kroah-Hartman, openbmc, Rob Herring, Krzysztof Kozlowski, devicetree 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding 2022-05-05 23:25 [PATCH v2 0/2] extcon: Add extcon-regulator driver Zev Weiss @ 2022-05-05 23:25 ` Zev Weiss 2022-05-17 0:15 ` Rob Herring 2022-05-05 23:25 ` [PATCH v2 2/2] extcon: Add extcon-regulator driver Zev Weiss 1 sibling, 1 reply; 10+ messages in thread From: Zev Weiss @ 2022-05-05 23:25 UTC (permalink / raw) To: Mark Brown, Liam Girdwood, MyungJoo Ham, Chanwoo Choi, Rob Herring, Krzysztof Kozlowski, devicetree Cc: Zev Weiss, linux-kernel, Greg Kroah-Hartman, openbmc 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding 2022-05-05 23:25 ` [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding Zev Weiss @ 2022-05-17 0:15 ` Rob Herring 0 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2022-05-17 0:15 UTC (permalink / raw) To: Zev Weiss Cc: linux-kernel, openbmc, Chanwoo Choi, Rob Herring, Liam Girdwood, Mark Brown, Krzysztof Kozlowski, MyungJoo Ham, devicetree, Greg Kroah-Hartman 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-05 23:25 [PATCH v2 0/2] extcon: Add extcon-regulator driver Zev Weiss 2022-05-05 23:25 ` [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding Zev Weiss @ 2022-05-05 23:25 ` Zev Weiss 2022-05-09 12:24 ` Chanwoo Choi 1 sibling, 1 reply; 10+ messages in thread From: Zev Weiss @ 2022-05-05 23:25 UTC (permalink / raw) To: Mark Brown, Liam Girdwood, MyungJoo Ham, Chanwoo Choi Cc: Zev Weiss, linux-kernel, Greg Kroah-Hartman, openbmc 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-05 23:25 ` [PATCH v2 2/2] extcon: Add extcon-regulator driver Zev Weiss @ 2022-05-09 12:24 ` Chanwoo Choi 2022-05-09 15:24 ` Mark Brown 0 siblings, 1 reply; 10+ messages in thread From: Chanwoo Choi @ 2022-05-09 12:24 UTC (permalink / raw) To: Zev Weiss, Mark Brown, Liam Girdwood, MyungJoo Ham, Chanwoo Choi Cc: linux-kernel, Greg Kroah-Hartman, openbmc 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"); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-09 12:24 ` Chanwoo Choi @ 2022-05-09 15:24 ` Mark Brown 2022-05-17 1:03 ` Zev Weiss 0 siblings, 1 reply; 10+ messages in thread From: Mark Brown @ 2022-05-09 15:24 UTC (permalink / raw) To: Chanwoo Choi Cc: Zev Weiss, Liam Girdwood, MyungJoo Ham, Chanwoo Choi, linux-kernel, Greg Kroah-Hartman, openbmc [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-09 15:24 ` Mark Brown @ 2022-05-17 1:03 ` Zev Weiss 2022-05-17 3:15 ` Chanwoo Choi 0 siblings, 1 reply; 10+ messages in thread From: Zev Weiss @ 2022-05-17 1:03 UTC (permalink / raw) To: Mark Brown Cc: Chanwoo Choi, Liam Girdwood, MyungJoo Ham, Chanwoo Choi, linux-kernel, Greg Kroah-Hartman, openbmc, Sebastian Reichel [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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-17 1:03 ` Zev Weiss @ 2022-05-17 3:15 ` Chanwoo Choi 2022-05-17 10:07 ` Zev Weiss 0 siblings, 1 reply; 10+ messages in thread From: Chanwoo Choi @ 2022-05-17 3:15 UTC (permalink / raw) To: Zev Weiss, Mark Brown Cc: Chanwoo Choi, Liam Girdwood, MyungJoo Ham, linux-kernel, Greg Kroah-Hartman, openbmc, Sebastian Reichel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-17 3:15 ` Chanwoo Choi @ 2022-05-17 10:07 ` Zev Weiss 2022-05-17 18:15 ` Chanwoo Choi 0 siblings, 1 reply; 10+ messages in thread From: Zev Weiss @ 2022-05-17 10:07 UTC (permalink / raw) To: Chanwoo Choi Cc: Mark Brown, Chanwoo Choi, Liam Girdwood, MyungJoo Ham, linux-kernel, Greg Kroah-Hartman, openbmc, Sebastian Reichel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] extcon: Add extcon-regulator driver 2022-05-17 10:07 ` Zev Weiss @ 2022-05-17 18:15 ` Chanwoo Choi 0 siblings, 0 replies; 10+ messages in thread From: Chanwoo Choi @ 2022-05-17 18:15 UTC (permalink / raw) To: Zev Weiss, Chanwoo Choi Cc: Mark Brown, Liam Girdwood, MyungJoo Ham, linux-kernel, Greg Kroah-Hartman, openbmc, Sebastian Reichel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-05-17 18:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-05 23:25 [PATCH v2 0/2] extcon: Add extcon-regulator driver Zev Weiss 2022-05-05 23:25 ` [PATCH v2 1/2] dt-bindings: connector: Add regulator-connector binding Zev Weiss 2022-05-17 0:15 ` Rob Herring 2022-05-05 23:25 ` [PATCH v2 2/2] extcon: Add extcon-regulator driver Zev Weiss 2022-05-09 12:24 ` Chanwoo Choi 2022-05-09 15:24 ` Mark Brown 2022-05-17 1:03 ` Zev Weiss 2022-05-17 3:15 ` Chanwoo Choi 2022-05-17 10:07 ` Zev Weiss 2022-05-17 18:15 ` Chanwoo Choi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).