[PATCH 0/5] add USB Type-B connector driver

[PATCH 0/5] add USB Type-B connector driver

[Chunfeng Yun]

Because the USB Connector is introduced and the requirement of
usb-connector.txt binding, the old way using extcon to support
USB Dual-Role switch is now deprecated, meanwhile there is no
available common driver when use Type-B connector.
This patch series introduce a Type-B connector driver and try
to replace the function provided by extcon-usb-gpio driver.
The main purpose of the patches is also to solve the Type-B
connector problem encountered in [1].

[1]: https://patchwork.kernel.org/patch/10819377/

Chunfeng Yun (5):
  dt-bindings: connector: add optional properties for Type-B
  dt-bindings: usb: mtu3: add properties about USB Role Switch
  usb: roles: add API to get usb_role_switch by node
  usb: roles: add USB Type-B connector driver
  usb: mtu3: register a USB Role Switch for Dual-Role mode

 .../bindings/connector/usb-connector.txt      |  10 +
 .../devicetree/bindings/usb/mediatek,mtu3.txt |  10 +-
 drivers/usb/mtu3/mtu3.h                       |   5 +
 drivers/usb/mtu3/mtu3_dr.c                    |  50 ++-
 drivers/usb/mtu3/mtu3_plat.c                  |   3 +-
 drivers/usb/roles/Kconfig                     |  14 +
 drivers/usb/roles/Makefile                    |   1 +
 drivers/usb/roles/class.c                     |  30 ++
 drivers/usb/roles/usb-b-connector.c           | 285 ++++++++++++++++++
 include/linux/usb/role.h                      |   1 +
 10 files changed, 403 insertions(+), 6 deletions(-)
 create mode 100644 drivers/usb/roles/usb-b-connector.c

-- 
2.20.1

[PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
usb-b-connector

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
index a9a2f2fc44f2..7a07b0f4f973 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.txt
+++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
@@ -17,6 +17,16 @@ Optional properties:
 - self-powered: Set this property if the usb device that has its own power
   source.
 
+Optional properties for usb-b-connector:
+- id-gpios: gpio for USB ID pin.
+- vbus-gpios: gpio for USB VBUS pin.
+  see gpio/gpio.txt.
+- vbus-supply: reference to the VBUS regulator, needed when supports
+  dual-role mode.
+- pinctrl-names : a pinctrl state named "default" is optional
+- pinctrl-0 : pin control group
+  see pinctrl/pinctrl-bindings.txt
+
 Optional properties for usb-c-connector:
 - power-role: should be one of "source", "sink" or "dual"(DRP) if typec
   connector has power support.
-- 
2.20.1

[PATCH 2/5] dt-bindings: usb: mtu3: add properties about USB Role Switch

[Chunfeng Yun]

Now the USB Role Switch is supported, so add properties about it

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 .../devicetree/bindings/usb/mediatek,mtu3.txt          | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt b/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
index 3382b5cb471d..97239913a7bd 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
@@ -27,7 +27,9 @@ Optional properties:
  - ranges : allows valid 1:1 translation between child's address space and
 	parent's address space
  - extcon : external connector for vbus and idpin changes detection, needed
-	when supports dual-role mode.
+	when supports dual-role mode; it's consiedered valid for compatibility
+	reasons, and not allowed for new bindings, use the property
+	usb-role-switch instead.
  - vbus-supply : reference to the VBUS regulator, needed when supports
 	dual-role mode.
  - pinctrl-names : a pinctrl state named "default" is optional, and need be
@@ -36,7 +38,8 @@ Optional properties:
 	is not set.
  - pinctrl-0 : pin control group
 	See: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
-
+ - usb-role-switch : use USB Role Switch to support dual-role siwtch, but
+	not extcon
  - maximum-speed : valid arguments are "super-speed", "high-speed" and
 	"full-speed"; refer to usb/generic.txt
  - enable-manual-drd : supports manual dual-role switch via debugfs; usually
@@ -61,6 +64,9 @@ The xhci should be added as subnode to mtu3 as shown in the following example
 if host mode is enabled. The DT binding details of xhci can be found in:
 Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt
 
+The connector would be added as subnode if receptacle is Micro, see
+connector/usb-connector.txt
+
 Example:
 ssusb: usb@11271000 {
 	compatible = "mediatek,mt8173-mtu3";
-- 
2.20.1

[PATCH 3/5] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Add usb_role_switch_get_by_node() to make easier to get
usb_role_switch by node which register it.
It's useful when there is not device_connection registered
between two drivers and only knows the node which register
usb_role_switch.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/roles/class.c | 30 ++++++++++++++++++++++++++++++
 include/linux/usb/role.h  |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 99116af07f1d..284b19856dc4 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -11,6 +11,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 
 static struct class *role_class;
@@ -121,6 +122,35 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get);
 
+static int __switch_match_node(struct device *dev, const void *node)
+{
+	return dev->parent->of_node == (const struct device_node *)node;
+}
+
+/**
+ * usb_role_switch_get_by_node - Find USB role switch by it's parent node
+ * @node: The node that register USB role switch
+ *
+ * Finds and returns role switch registered by @node. The reference count
+ * for the found switch is incremented.
+ */
+struct usb_role_switch *usb_role_switch_get_by_node(struct device_node *node)
+{
+	struct usb_role_switch *sw;
+	struct device *dev;
+
+	dev = class_find_device(role_class, NULL, node,
+				__switch_match_node);
+	if (!dev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	sw = to_role_switch(dev);
+	WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+
+	return sw;
+}
+EXPORT_SYMBOL_GPL(usb_role_switch_get_by_node);
+
 /**
  * usb_role_switch_put - Release handle to a switch
  * @sw: USB Role Switch
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index edc51be4a77c..056498b83dee 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -42,6 +42,7 @@ struct usb_role_switch_desc {
 
 int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role);
 enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw);
+struct usb_role_switch *usb_role_switch_get_by_node(struct device_node *node);
 struct usb_role_switch *usb_role_switch_get(struct device *dev);
 void usb_role_switch_put(struct usb_role_switch *sw);
 
-- 
2.20.1

[PATCH 4/5] usb: roles: add USB Type-B connector driver

[Chunfeng Yun]

Due to the requirement of usb-connector.txt binding, the old way
using extcon to support USB Dual-Role switch is now deprecated
when use Type-B connector.
This patch introduce a Type-B connector driver and try to replace
the function provided by extcon-usb-gpio driver

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/roles/Kconfig           |  14 ++
 drivers/usb/roles/Makefile          |   1 +
 drivers/usb/roles/usb-b-connector.c | 285 ++++++++++++++++++++++++++++
 3 files changed, 300 insertions(+)
 create mode 100644 drivers/usb/roles/usb-b-connector.c

diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
index e4194ac94510..e267132917c2 100644
--- a/drivers/usb/roles/Kconfig
+++ b/drivers/usb/roles/Kconfig
@@ -24,4 +24,18 @@ config USB_ROLES_INTEL_XHCI
 	  To compile the driver as a module, choose M here: the module will
 	  be called intel-xhci-usb-role-switch.
 
+config USB_B_CONNECTOR
+	tristate "USB Type-B Connector"
+	depends on GPIOLIB
+	help
+	  The driver supports USB role switch between host and device by GPIO
+	  based USB cable detection, used typically if GPIO is used for USB ID
+	  pin detection. And it's used to replace the function provided by
+	  drivers/extcon/extcon-usb-gpio.c driver due to the requirement by
+	  connector/usb-connector.txt
+
+	  Say Y here if your USB Type-B Connector supports Dual-Role mode.
+	  To compile the driver as a module, choose M here: the module will
+	  be called usb-b-connector.ko
+
 endif # USB_ROLE_SWITCH
diff --git a/drivers/usb/roles/Makefile b/drivers/usb/roles/Makefile
index c02873206fc1..ff8be33253a4 100644
--- a/drivers/usb/roles/Makefile
+++ b/drivers/usb/roles/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_USB_ROLE_SWITCH)		+= roles.o
 roles-y					:= class.o
+obj-$(CONFIG_USB_B_CONNECTOR)		+= usb-b-connector.o
 obj-$(CONFIG_USB_ROLES_INTEL_XHCI)	+= intel-xhci-usb-role-switch.o
diff --git a/drivers/usb/roles/usb-b-connector.c b/drivers/usb/roles/usb-b-connector.c
new file mode 100644
index 000000000000..debd0c6500cf
--- /dev/null
+++ b/drivers/usb/roles/usb-b-connector.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB Type-B Connector Driver
+ *
+ * Copyright (C) 2019 MediaTek Inc.
+ *
+ * Author: Chunfeng Yun <chunfeng.yun@mediatek.com>
+ *
+ * Some code borrowed from drivers/extcon/extcon-usb-gpio.c
+ */
+
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/role.h>
+
+#define USB_GPIO_DEB_MS		20	/* ms */
+#define USB_GPIO_DEB_US		((USB_GPIO_DEB_MS) * 1000)	/* us */
+
+#define USB_CONN_IRQF	\
+	(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT)
+
+struct usb_conn_info {
+	struct device *dev;
+	struct usb_role_switch *role_sw;
+	enum usb_role last_role;
+	struct regulator *vbus;
+	struct delayed_work dw_det;
+	unsigned long debounce_jiffies;
+
+	struct gpio_desc *id_gpiod;
+	struct gpio_desc *vbus_gpiod;
+	int id_irq;
+	int vbus_irq;
+};
+
+/**
+ * "DEVICE" = VBUS and "HOST" = !ID, so we have:
+ * Both "DEVICE" and "HOST" can't be set as active at the same time
+ * so if "HOST" is active (i.e. ID is 0)  we keep "DEVICE" inactive
+ * even if VBUS is on.
+ *
+ *  Role          |   ID  |  VBUS
+ * ------------------------------------
+ *  [1] DEVICE    |   H   |   H
+ *  [2] NONE      |   H   |   L
+ *  [3] HOST      |   L   |   H
+ *  [4] HOST      |   L   |   L
+ *
+ * In case we have only one of these signals:
+ * - VBUS only - we want to distinguish between [1] and [2], so ID is always 1
+ * - ID only - we want to distinguish between [1] and [4], so VBUS = ID
+ */
+static void usb_conn_detect_cable(struct work_struct *work)
+{
+	struct usb_conn_info *info;
+	enum usb_role role;
+	int id, vbus, ret;
+
+	info = container_of(to_delayed_work(work),
+			    struct usb_conn_info, dw_det);
+
+	/* check ID and VBUS */
+	id = info->id_gpiod ?
+		gpiod_get_value_cansleep(info->id_gpiod) : 1;
+	vbus = info->vbus_gpiod ?
+		gpiod_get_value_cansleep(info->vbus_gpiod) : id;
+
+	if (!id)
+		role = USB_ROLE_HOST;
+	else if (vbus)
+		role = USB_ROLE_DEVICE;
+	else
+		role = USB_ROLE_NONE;
+
+	dev_dbg(info->dev, "role %d/%d, gpios: id %d, vbus %d\n",
+		info->last_role, role, id, vbus);
+
+	if (info->last_role == role) {
+		dev_warn(info->dev, "repeated role: %d\n", role);
+		return;
+	}
+
+	if (info->last_role == USB_ROLE_HOST)
+		regulator_disable(info->vbus);
+
+	ret = usb_role_switch_set_role(info->role_sw, role);
+	if (ret)
+		dev_err(info->dev, "failed to set role: %d\n", ret);
+
+	if (role == USB_ROLE_HOST) {
+		ret = regulator_enable(info->vbus);
+		if (ret)
+			dev_err(info->dev, "enable vbus regulator failed\n");
+	}
+
+	info->last_role = role;
+
+	dev_dbg(info->dev, "vbus regulator is %s\n",
+		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
+}
+
+static void usb_conn_queue_dwork(struct usb_conn_info *info,
+				 unsigned long delay)
+{
+	queue_delayed_work(system_power_efficient_wq, &info->dw_det, delay);
+}
+
+static irqreturn_t usb_conn_isr(int irq, void *dev_id)
+{
+	struct usb_conn_info *info = dev_id;
+
+	usb_conn_queue_dwork(info, info->debounce_jiffies);
+
+	return IRQ_HANDLED;
+}
+
+static int usb_conn_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *parent;
+	struct usb_conn_info *info;
+	int ret = 0;
+
+	if (!np)
+		return -EINVAL;
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = dev;
+	info->id_gpiod = devm_gpiod_get_optional(dev, "id", GPIOD_IN);
+	if (IS_ERR(info->id_gpiod))
+		return PTR_ERR(info->id_gpiod);
+
+	info->vbus_gpiod = devm_gpiod_get_optional(dev, "vbus", GPIOD_IN);
+	if (IS_ERR(info->vbus_gpiod))
+		return PTR_ERR(info->vbus_gpiod);
+
+	if (!info->id_gpiod && !info->vbus_gpiod) {
+		dev_err(dev, "failed to get gpios\n");
+		return -ENODEV;
+	}
+
+	if (info->id_gpiod)
+		ret = gpiod_set_debounce(info->id_gpiod, USB_GPIO_DEB_US);
+	if (!ret && info->vbus_gpiod)
+		ret = gpiod_set_debounce(info->vbus_gpiod, USB_GPIO_DEB_US);
+	if (ret < 0)
+		info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEB_MS);
+
+	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
+
+	info->vbus = devm_regulator_get(dev, "vbus");
+	if (IS_ERR(info->vbus)) {
+		dev_err(dev, "failed to get vbus\n");
+		return PTR_ERR(info->vbus);
+	}
+
+	parent = of_get_parent(np);
+	info->role_sw = usb_role_switch_get_by_node(parent);
+	if (IS_ERR_OR_NULL(info->role_sw)) {
+		dev_err(dev, "failed to get role switch\n");
+		return PTR_ERR(info->role_sw);
+	}
+
+	of_node_put(parent);
+
+	if (info->id_gpiod) {
+		info->id_irq = gpiod_to_irq(info->id_gpiod);
+		if (info->id_irq < 0) {
+			dev_err(dev, "failed to get ID IRQ\n");
+			return info->id_irq;
+		}
+
+		ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
+						usb_conn_isr, USB_CONN_IRQF,
+						pdev->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request ID IRQ\n");
+			return ret;
+		}
+	}
+
+	if (info->vbus_gpiod) {
+		info->vbus_irq = gpiod_to_irq(info->vbus_gpiod);
+		if (info->vbus_irq < 0) {
+			dev_err(dev, "failed to get VBUS IRQ\n");
+			return info->vbus_irq;
+		}
+
+		ret = devm_request_threaded_irq(dev, info->vbus_irq, NULL,
+						usb_conn_isr, USB_CONN_IRQF,
+						pdev->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request VBUS IRQ\n");
+			return ret;
+		}
+	}
+
+	platform_set_drvdata(pdev, info);
+
+	/* Perform initial detection */
+	usb_conn_queue_dwork(info, 0);
+
+	return 0;
+}
+
+static int usb_conn_remove(struct platform_device *pdev)
+{
+	struct usb_conn_info *info = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&info->dw_det);
+
+	if (info->last_role == USB_ROLE_HOST)
+		regulator_disable(info->vbus);
+
+	usb_role_switch_put(info->role_sw);
+
+	return 0;
+}
+
+static int __maybe_unused usb_conn_suspend(struct device *dev)
+{
+	struct usb_conn_info *info = dev_get_drvdata(dev);
+
+	if (info->id_gpiod)
+		disable_irq(info->id_irq);
+	if (info->vbus_gpiod)
+		disable_irq(info->vbus_irq);
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int __maybe_unused usb_conn_resume(struct device *dev)
+{
+	struct usb_conn_info *info = dev_get_drvdata(dev);
+
+	pinctrl_pm_select_default_state(dev);
+
+	if (info->id_gpiod)
+		enable_irq(info->id_irq);
+	if (info->vbus_gpiod)
+		enable_irq(info->vbus_irq);
+
+	usb_conn_queue_dwork(info, 0);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(usb_conn_pm_ops,
+			 usb_conn_suspend, usb_conn_resume);
+
+#define DEV_PMS_OPS (IS_ENABLED(CONFIG_PM_SLEEP) ? &usb_conn_pm_ops : NULL)
+
+static const struct of_device_id usb_conn_dt_match[] = {
+	{ .compatible = "usb-b-connector", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, usb_conn_dt_match);
+
+static struct platform_driver usb_conn_driver = {
+	.probe		= usb_conn_probe,
+	.remove		= usb_conn_remove,
+	.driver		= {
+		.name	= "usb_b_conn",
+		.pm	= DEV_PMS_OPS,
+		.of_match_table = usb_conn_dt_match,
+	},
+};
+
+module_platform_driver(usb_conn_driver);
+
+MODULE_AUTHOR("Chunfeng Yun <chunfeng.yun@mediatek.com>");
+MODULE_DESCRIPTION("USB GPIO conn driver");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH 5/5] usb: mtu3: register a USB Role Switch for Dual-Role mode

[Chunfeng Yun]

Due to extcon is not allowed for new bindings, and the
Dual-Role switch is supported by USB Role Switch,
especially for Type-C drivers, so register a USB Role
Switch to support the new way

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3.h      |  5 ++++
 drivers/usb/mtu3/mtu3_dr.c   | 50 +++++++++++++++++++++++++++++++++---
 drivers/usb/mtu3/mtu3_plat.c |  3 ++-
 3 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
index 87823ac0d120..8f9da03255b6 100644
--- a/drivers/usb/mtu3/mtu3.h
+++ b/drivers/usb/mtu3/mtu3.h
@@ -202,6 +202,9 @@ struct mtu3_gpd_ring {
 * @id_nb : notifier for iddig(idpin) detection
 * @id_work : work of iddig detection notifier
 * @id_event : event of iddig detecion notifier
+* @role_sw : use USB Role Switch to support dual-role switch, can't use
+*		extcon at the same time, and extcon is deprecated.
+* @role_sw_used : true when the USB Role Switch is used.
 * @is_u3_drd: whether port0 supports usb3.0 dual-role device or not
 * @manual_drd_enabled: it's true when supports dual-role device by debugfs
 *		to switch host/device modes depending on user input.
@@ -215,6 +218,8 @@ struct otg_switch_mtk {
 	struct notifier_block id_nb;
 	struct work_struct id_work;
 	unsigned long id_event;
+	struct usb_role_switch *role_sw;
+	bool role_sw_used;
 	bool is_u3_drd;
 	bool manual_drd_enabled;
 };
diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
index ac60e9c8564e..b9272295fe2b 100644
--- a/drivers/usb/mtu3/mtu3_dr.c
+++ b/drivers/usb/mtu3/mtu3_dr.c
@@ -14,6 +14,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
+#include <linux/usb/role.h>
 
 #include "mtu3.h"
 #include "mtu3_dr.h"
@@ -266,7 +267,7 @@ static int ssusb_extcon_register(struct otg_switch_mtk *otg_sx)
  * This is useful in special cases, such as uses TYPE-A receptacle but also
  * wants to support dual-role mode.
  */
-static void ssusb_mode_manual_switch(struct ssusb_mtk *ssusb, int to_host)
+static void ssusb_mode_switch(struct ssusb_mtk *ssusb, int to_host)
 {
 	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
 
@@ -308,9 +309,9 @@ static ssize_t ssusb_mode_write(struct file *file,
 		return -EFAULT;
 
 	if (!strncmp(buf, "host", 4) && !ssusb->is_host) {
-		ssusb_mode_manual_switch(ssusb, 1);
+		ssusb_mode_switch(ssusb, 1);
 	} else if (!strncmp(buf, "device", 6) && ssusb->is_host) {
-		ssusb_mode_manual_switch(ssusb, 0);
+		ssusb_mode_switch(ssusb, 0);
 	} else {
 		dev_err(ssusb->dev, "wrong or duplicated setting\n");
 		return -EINVAL;
@@ -412,6 +413,46 @@ void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
 	mtu3_writel(ssusb->ippc_base, SSUSB_U2_CTRL(0), value);
 }
 
+static int ssusb_role_sw_set(struct device *dev, enum usb_role role)
+{
+	struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
+	bool to_host = false;
+
+	if (role == USB_ROLE_HOST)
+		to_host = true;
+
+	if (to_host ^ ssusb->is_host)
+		ssusb_mode_switch(ssusb, to_host);
+
+	return 0;
+}
+
+static enum usb_role ssusb_role_sw_get(struct device *dev)
+{
+	struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
+	enum usb_role role;
+
+	role = ssusb->is_host ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+
+	return role;
+}
+
+static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
+{
+	struct usb_role_switch_desc role_sx_desc = { 0 };
+	struct ssusb_mtk *ssusb =
+		container_of(otg_sx, struct ssusb_mtk, otg_switch);
+
+	if (!otg_sx->role_sw_used)
+		return 0;
+
+	role_sx_desc.set = ssusb_role_sw_set;
+	role_sx_desc.get = ssusb_role_sw_get;
+	otg_sx->role_sw = usb_role_switch_register(ssusb->dev, &role_sx_desc);
+
+	return PTR_ERR_OR_ZERO(otg_sx->role_sw);
+}
+
 int ssusb_otg_switch_init(struct ssusb_mtk *ssusb)
 {
 	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
@@ -421,6 +462,8 @@ int ssusb_otg_switch_init(struct ssusb_mtk *ssusb)
 
 	if (otg_sx->manual_drd_enabled)
 		ssusb_debugfs_init(ssusb);
+	else if (otg_sx->role_sw_used)
+		ssusb_role_sw_register(otg_sx);
 	else
 		ssusb_extcon_register(otg_sx);
 
@@ -436,4 +479,5 @@ void ssusb_otg_switch_exit(struct ssusb_mtk *ssusb)
 
 	cancel_work_sync(&otg_sx->id_work);
 	cancel_work_sync(&otg_sx->vbus_work);
+	usb_role_switch_unregister(otg_sx->role_sw);
 }
diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
index e086630e41a9..02c288a85989 100644
--- a/drivers/usb/mtu3/mtu3_plat.c
+++ b/drivers/usb/mtu3/mtu3_plat.c
@@ -313,8 +313,9 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
 	otg_sx->is_u3_drd = of_property_read_bool(node, "mediatek,usb3-drd");
 	otg_sx->manual_drd_enabled =
 		of_property_read_bool(node, "enable-manual-drd");
+	otg_sx->role_sw_used = of_property_read_bool(node, "usb-role-switch");
 
-	if (of_property_read_bool(node, "extcon")) {
+	if (!otg_sx->role_sw_used && of_property_read_bool(node, "extcon")) {
 		otg_sx->edev = extcon_get_edev_by_phandle(ssusb->dev, 0);
 		if (IS_ERR(otg_sx->edev)) {
 			dev_err(ssusb->dev, "couldn't get extcon device\n");
-- 
2.20.1

Re: [PATCH 3/5] usb: roles: add API to get usb_role_switch by node

[Andy Shevchenko]

On Fri, Mar 8, 2019 at 8:14 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> Add usb_role_switch_get_by_node() to make easier to get
> usb_role_switch by node which register it.
> It's useful when there is not device_connection registered
> between two drivers and only knows the node which register
> usb_role_switch.

> +static int __switch_match_node(struct device *dev, const void *node)
> +{
> +       return dev->parent->of_node == (const struct device_node *)node;
> +}

Hmm... Shouldn't be slightly better to compare fwnode instead?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Hans de Goede]

Hi,

On 08-03-19 07:13, Chunfeng Yun wrote:
> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> usb-b-connector
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>   .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> index a9a2f2fc44f2..7a07b0f4f973 100644
> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -17,6 +17,16 @@ Optional properties:
>   - self-powered: Set this property if the usb device that has its own power
>     source.
>   
> +Optional properties for usb-b-connector:
> +- id-gpios: gpio for USB ID pin.

What about boards where the ID pin is *not* connected to a GPIO,
but e.g. to a special pin on the PMIC which can also detect
an ACA adapter ? Currently this case is handled by extcon
drivers, but we have no way to set e.g. vbus-supply for the
connector. Maybe in this case the usb-connector node should
be a child of the PMIC node ?

And in many cases there also is a mux to switch the datalines
between the host and device(gadget) controllers, how should
that be described in this model?  See the new usb-role-switch
code under drivers/usb/roles

In some cases the mux is controlled through a gpio, so we
may want to add a "mux-gpios" here in which case we also
need to define what 0/1 means.

> +- vbus-gpios: gpio for USB VBUS pin.
> +  see gpio/gpio.txt.
> +- vbus-supply: reference to the VBUS regulator, needed when supports
> +  dual-role mode.

I think this needs some text that there can be either a vbus-gpio or
a vbus-supply. Oh wait reading:

https://patchwork.kernel.org/patch/10819377/

I see that this GPIO is for detecting vbus presence, not for driving/enabling
5v to Vbus from the board, that needs to be described more clearly.

> +- pinctrl-names : a pinctrl state named "default" is optional
> +- pinctrl-0 : pin control group
> +  see pinctrl/pinctrl-bindings.txt
> +
>   Optional properties for usb-c-connector:
>   - power-role: should be one of "source", "sink" or "dual"(DRP) if typec
>     connector has power support.
> 


Regards,

Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Hi,

On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-03-19 07:13, Chunfeng Yun wrote:
> > Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> > usb-b-connector
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >   .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > index a9a2f2fc44f2..7a07b0f4f973 100644
> > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > @@ -17,6 +17,16 @@ Optional properties:
> >   - self-powered: Set this property if the usb device that has its own power
> >     source.
> >   
> > +Optional properties for usb-b-connector:
> > +- id-gpios: gpio for USB ID pin.
> 
> What about boards where the ID pin is *not* connected to a GPIO,
> but e.g. to a special pin on the PMIC which can also detect
> an ACA adapter ? Currently this case is handled by extcon
> drivers, but we have no way to set e.g. vbus-supply for the
> connector. Maybe in this case the usb-connector node should
> be a child of the PMIC node ?
Yes, it would be, PMIC is in charger of detecting the status of ID pin
> 
> And in many cases there also is a mux to switch the datalines
> between the host and device(gadget) controllers, how should
> that be described in this model?  See the new usb-role-switch
> code under drivers/usb/roles
> 
> In some cases the mux is controlled through a gpio, so we
> may want to add a "mux-gpios" here in which case we also
> need to define what 0/1 means.
I'm not sure, the mux seems not belong to this connector,
and may need another driver to register usb-role-switch,
similar to:

[v2,2/2] usb: typec: add typec switch via GPIO control
https://patchwork.kernel.org/patch/10834327/


> 
> > +- vbus-gpios: gpio for USB VBUS pin.
> > +  see gpio/gpio.txt.
> > +- vbus-supply: reference to the VBUS regulator, needed when supports
> > +  dual-role mode.
> 
> I think this needs some text that there can be either a vbus-gpio or
> a vbus-supply. Oh wait reading:
> 
> https://patchwork.kernel.org/patch/10819377/
> 
> I see that this GPIO is for detecting vbus presence, not for driving/enabling
> 5v to Vbus from the board, that needs to be described more clearly.
Ok

Thanks a lot
> 
> > +- pinctrl-names : a pinctrl state named "default" is optional
> > +- pinctrl-0 : pin control group
> > +  see pinctrl/pinctrl-bindings.txt
> > +
> >   Optional properties for usb-c-connector:
> >   - power-role: should be one of "source", "sink" or "dual"(DRP) if typec
> >     connector has power support.
> > 
> 
> 
> Regards,
> 
> Hans

Re: [PATCH 3/5] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi,

On Fri, 2019-03-08 at 08:52 +0200, Andy Shevchenko wrote:
> On Fri, Mar 8, 2019 at 8:14 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > Add usb_role_switch_get_by_node() to make easier to get
> > usb_role_switch by node which register it.
> > It's useful when there is not device_connection registered
> > between two drivers and only knows the node which register
> > usb_role_switch.
> 
> > +static int __switch_match_node(struct device *dev, const void *node)
> > +{
> > +       return dev->parent->of_node == (const struct device_node *)node;
> > +}
> 
> Hmm... Shouldn't be slightly better to compare fwnode instead?
> 
Using fwnode is indeed suitable for more cases,
I find that there are many functions named xx_by_node using node, but
not fwnode, is there any rules about choice between device_node and
fwnode_handle?

Thanks

RE: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Jun Li]

> -----Original Message-----
> From: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Sent: 2019年3月11日 13:33
> To: Hans de Goede <hdegoede@redhat.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Mark Rutland <mark.rutland@arm.com>;
> Matthias Brugger <matthias.bgg@gmail.com>; Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com>; Jun Li <jun.li@nxp.com>; Badhri
> Jagan Sridharan <badhri@google.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Min Guo <min.guo@mediatek.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-usb@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-mediatek@lists.infradead.org
> Subject: Re: [PATCH 1/5] dt-bindings: connector: add optional properties for
> Type-B
> 
> Hi,
> 
> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 08-03-19 07:13, Chunfeng Yun wrote:
> > > Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> > > usb-b-connector
> > >
> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > ---
> > >   .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > index a9a2f2fc44f2..7a07b0f4f973 100644
> > > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > @@ -17,6 +17,16 @@ Optional properties:
> > >   - self-powered: Set this property if the usb device that has its own power
> > >     source.
> > >
> > > +Optional properties for usb-b-connector:
> > > +- id-gpios: gpio for USB ID pin.
> >
> > What about boards where the ID pin is *not* connected to a GPIO, but
> > e.g. to a special pin on the PMIC which can also detect an ACA adapter
> > ? Currently this case is handled by extcon drivers, but we have no way
> > to set e.g. vbus-supply for the connector. Maybe in this case the
> > usb-connector node should be a child of the PMIC node ?
> Yes, it would be, PMIC is in charger of detecting the status of ID pin
> >
> > And in many cases there also is a mux to switch the datalines between
> > the host and device(gadget) controllers, how should that be described
> > in this model?  See the new usb-role-switch code under
> > drivers/usb/roles
> >
> > In some cases the mux is controlled through a gpio, so we may want to
> > add a "mux-gpios" here in which case we also need to define what 0/1
> > means.
> I'm not sure, the mux seems not belong to this connector, and may need another
> driver to register usb-role-switch, similar to:
> 
> [v2,2/2] usb: typec: add typec switch via GPIO control
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork
> .kernel.org%2Fpatch%2F10834327%2F&amp;data=02%7C01%7Cjun.li%40nxp.co
> m%7C963df62e15ed4bedb14d08d6a5e30de2%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C636878791953122760&amp;sdata=grPIs2MbdaarTa17dr
> pASVkGpyW7TAexB24igOJopGU%3D&amp;reserved=0
> 

No, this is not for usb role switch, this is a typec switch driver to select the super speed
active channel by orientation(CC1/CC2).

Li Jun
> 
> >
> > > +- vbus-gpios: gpio for USB VBUS pin.
> > > +  see gpio/gpio.txt.
> > > +- vbus-supply: reference to the VBUS regulator, needed when
> > > +supports
> > > +  dual-role mode.
> >
> > I think this needs some text that there can be either a vbus-gpio or a
> > vbus-supply. Oh wait reading:
> >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.kernel.org%2Fpatch%2F10819377%2F&amp;data=02%7C01%7Cjun.li%4
> 0nx
> >
> p.com%7C963df62e15ed4bedb14d08d6a5e30de2%7C686ea1d3bc2b4c6fa92cd9
> 9c5c3
> >
> 01635%7C0%7C0%7C636878791953122760&amp;sdata=Judz7gdwQTOC7Jh84
> 57N4x21a
> > fWci%2FEH79ARqWZzbX8%3D&amp;reserved=0
> >
> > I see that this GPIO is for detecting vbus presence, not for
> > driving/enabling 5v to Vbus from the board, that needs to be described more
> clearly.
> Ok
> 
> Thanks a lot
> >
> > > +- pinctrl-names : a pinctrl state named "default" is optional
> > > +- pinctrl-0 : pin control group
> > > +  see pinctrl/pinctrl-bindings.txt
> > > +
> > >   Optional properties for usb-c-connector:
> > >   - power-role: should be one of "source", "sink" or "dual"(DRP) if typec
> > >     connector has power support.
> > >
> >
> >
> > Regards,
> >
> > Hans
> 

RE: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Hi Jun,
On Mon, 2019-03-11 at 06:06 +0000, Jun Li wrote:
> 
> > -----Original Message-----
> > From: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Sent: 2019年3月11日 13:33
> > To: Hans de Goede <hdegoede@redhat.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Heikki Krogerus
> > <heikki.krogerus@linux.intel.com>; Mark Rutland <mark.rutland@arm.com>;
> > Matthias Brugger <matthias.bgg@gmail.com>; Adam Thomson
> > <Adam.Thomson.Opensource@diasemi.com>; Jun Li <jun.li@nxp.com>; Badhri
> > Jagan Sridharan <badhri@google.com>; Andy Shevchenko
> > <andy.shevchenko@gmail.com>; Min Guo <min.guo@mediatek.com>;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-usb@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-mediatek@lists.infradead.org
> > Subject: Re: [PATCH 1/5] dt-bindings: connector: add optional properties for
> > Type-B
> > 
> > Hi,
> > 
> > On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 08-03-19 07:13, Chunfeng Yun wrote:
> > > > Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> > > > usb-b-connector
> > > >
> > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > ---
> > > >   .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> > > >   1 file changed, 10 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > index a9a2f2fc44f2..7a07b0f4f973 100644
> > > > --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > > > @@ -17,6 +17,16 @@ Optional properties:
> > > >   - self-powered: Set this property if the usb device that has its own power
> > > >     source.
> > > >
> > > > +Optional properties for usb-b-connector:
> > > > +- id-gpios: gpio for USB ID pin.
> > >
> > > What about boards where the ID pin is *not* connected to a GPIO, but
> > > e.g. to a special pin on the PMIC which can also detect an ACA adapter
> > > ? Currently this case is handled by extcon drivers, but we have no way
> > > to set e.g. vbus-supply for the connector. Maybe in this case the
> > > usb-connector node should be a child of the PMIC node ?
> > Yes, it would be, PMIC is in charger of detecting the status of ID pin
> > >
> > > And in many cases there also is a mux to switch the datalines between
> > > the host and device(gadget) controllers, how should that be described
> > > in this model?  See the new usb-role-switch code under
> > > drivers/usb/roles
> > >
> > > In some cases the mux is controlled through a gpio, so we may want to
> > > add a "mux-gpios" here in which case we also need to define what 0/1
> > > means.
> > I'm not sure, the mux seems not belong to this connector, and may need another
> > driver to register usb-role-switch, similar to:
> > 
> > [v2,2/2] usb: typec: add typec switch via GPIO control
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork
> > .kernel.org%2Fpatch%2F10834327%2F&amp;data=02%7C01%7Cjun.li%40nxp.co
> > m%7C963df62e15ed4bedb14d08d6a5e30de2%7C686ea1d3bc2b4c6fa92cd99c5
> > c301635%7C0%7C0%7C636878791953122760&amp;sdata=grPIs2MbdaarTa17dr
> > pASVkGpyW7TAexB24igOJopGU%3D&amp;reserved=0
> > 
> 
> No, this is not for usb role switch, this is a typec switch driver to select the super speed
> active channel by orientation(CC1/CC2).
Got it, it's just an analogy:)
> 
> Li Jun
> > 
> > >
> > > > +- vbus-gpios: gpio for USB VBUS pin.
> > > > +  see gpio/gpio.txt.
> > > > +- vbus-supply: reference to the VBUS regulator, needed when
> > > > +supports
> > > > +  dual-role mode.
> > >
> > > I think this needs some text that there can be either a vbus-gpio or a
> > > vbus-supply. Oh wait reading:
> > >
> > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> > >
> > chwork.kernel.org%2Fpatch%2F10819377%2F&amp;data=02%7C01%7Cjun.li%4
> > 0nx
> > >
> > p.com%7C963df62e15ed4bedb14d08d6a5e30de2%7C686ea1d3bc2b4c6fa92cd9
> > 9c5c3
> > >
> > 01635%7C0%7C0%7C636878791953122760&amp;sdata=Judz7gdwQTOC7Jh84
> > 57N4x21a
> > > fWci%2FEH79ARqWZzbX8%3D&amp;reserved=0
> > >
> > > I see that this GPIO is for detecting vbus presence, not for
> > > driving/enabling 5v to Vbus from the board, that needs to be described more
> > clearly.
> > Ok
> > 
> > Thanks a lot
> > >
> > > > +- pinctrl-names : a pinctrl state named "default" is optional
> > > > +- pinctrl-0 : pin control group
> > > > +  see pinctrl/pinctrl-bindings.txt
> > > > +
> > > >   Optional properties for usb-c-connector:
> > > >   - power-role: should be one of "source", "sink" or "dual"(DRP) if typec
> > > >     connector has power support.
> > > >
> > >
> > >
> > > Regards,
> > >
> > > Hans
> > 
> 

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Hans de Goede]

Hi,

On 11-03-19 06:33, Chunfeng Yun wrote:
> Hi,
> 
> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 08-03-19 07:13, Chunfeng Yun wrote:
>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
>>> usb-b-connector
>>>
>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>> ---
>>>    .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> index a9a2f2fc44f2..7a07b0f4f973 100644
>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> @@ -17,6 +17,16 @@ Optional properties:
>>>    - self-powered: Set this property if the usb device that has its own power
>>>      source.
>>>    
>>> +Optional properties for usb-b-connector:
>>> +- id-gpios: gpio for USB ID pin.
>>
>> What about boards where the ID pin is *not* connected to a GPIO,
>> but e.g. to a special pin on the PMIC which can also detect
>> an ACA adapter ? Currently this case is handled by extcon
>> drivers, but we have no way to set e.g. vbus-supply for the
>> connector. Maybe in this case the usb-connector node should
>> be a child of the PMIC node ?
> Yes, it would be, PMIC is in charger of detecting the status of ID pin

Ok, then I think this should be documented too.

>> And in many cases there also is a mux to switch the datalines
>> between the host and device(gadget) controllers, how should
>> that be described in this model?  See the new usb-role-switch
>> code under drivers/usb/roles
>>
>> In some cases the mux is controlled through a gpio, so we
>> may want to add a "mux-gpios" here in which case we also
>> need to define what 0/1 means.
> I'm not sure, the mux seems not belong to this connector,
> and may need another driver to register usb-role-switch,
> similar to:
> 
> [v2,2/2] usb: typec: add typec switch via GPIO control
> https://patchwork.kernel.org/patch/10834327/

Right the mux/role-switch will need a driver, but the "owner"
of the usb_connector, e.g. the PMIC or the owner of the
id GPIO pin needs to know which device is the role-switch so
that it can set the role correctly based on the id-pin.

Your binding already contains Vbus info, allowing the owner
of the usb_connector to enable/disable Vbus based on the id-pin,
but the owner will also be responsible for setting the role-switch.

Note we cannot simply assume there will be only one role-switch,
we really need some link from the usb_connector to the role-switch
(or if it is a GPIO driven role-switch simply a role-switch-gpios
member in the usb_connector).

Regards,

Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Hans de Goede]

Hi,

On 11-03-19 09:06, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 06:33, Chunfeng Yun wrote:
>> Hi,
>>
>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 08-03-19 07:13, Chunfeng Yun wrote:
>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
>>>> usb-b-connector
>>>>
>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>>> ---
>>>>    .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> @@ -17,6 +17,16 @@ Optional properties:
>>>>    - self-powered: Set this property if the usb device that has its own power
>>>>      source.
>>>> +Optional properties for usb-b-connector:
>>>> +- id-gpios: gpio for USB ID pin.
>>>
>>> What about boards where the ID pin is *not* connected to a GPIO,
>>> but e.g. to a special pin on the PMIC which can also detect
>>> an ACA adapter ? Currently this case is handled by extcon
>>> drivers, but we have no way to set e.g. vbus-supply for the
>>> connector. Maybe in this case the usb-connector node should
>>> be a child of the PMIC node ?
>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
> 
> Ok, then I think this should be documented too.
> 
>>> And in many cases there also is a mux to switch the datalines
>>> between the host and device(gadget) controllers, how should
>>> that be described in this model?  See the new usb-role-switch
>>> code under drivers/usb/roles
>>>
>>> In some cases the mux is controlled through a gpio, so we
>>> may want to add a "mux-gpios" here in which case we also
>>> need to define what 0/1 means.
>> I'm not sure, the mux seems not belong to this connector,
>> and may need another driver to register usb-role-switch,
>> similar to:
>>
>> [v2,2/2] usb: typec: add typec switch via GPIO control
>> https://patchwork.kernel.org/patch/10834327/
> 
> Right the mux/role-switch will need a driver, but the "owner"
> of the usb_connector, e.g. the PMIC or the owner of the
> id GPIO pin needs to know which device is the role-switch so
> that it can set the role correctly based on the id-pin.
> 
> Your binding already contains Vbus info, allowing the owner
> of the usb_connector to enable/disable Vbus based on the id-pin,
> but the owner will also be responsible for setting the role-switch.
> 
> Note we cannot simply assume there will be only one role-switch,
> we really need some link from the usb_connector to the role-switch
> (or if it is a GPIO driven role-switch simply a role-switch-gpios
> member in the usb_connector).

I see now in your "[PATCH v3 1/2] dt-bindings: usb: add documentation for
typec switch via GPIO" that you plan to use a
Documentation/devicetree/bindings/graph.txt style binding for this,
adding a remote-endpoint to the orientation-switch (in that patch), or
to the role-switch.

But a Type-C port can have both an orientation-switch and a role-switch
(and a mux if it supports e.g. DP-altmode), how is the driver driving
the device which has the actual usb_c_connecter child-node to which
the remote-endpoints for both the orientation- and the role-switch points
supposed to figure out which is which ?

Also it feels the wrong-way around to me to have the orientation-switch
point to the usb_c_connector, to me it would make more sense to have
the usb_c_connector point to a port on the orientation-switch.

Regards,

Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Hans de Goede]

Hi,

On 11-03-19 12:00, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 09:06, Hans de Goede wrote:
>> Hi,
>>
>> On 11-03-19 06:33, Chunfeng Yun wrote:
>>> Hi,
>>>
>>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 08-03-19 07:13, Chunfeng Yun wrote:
>>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
>>>>> usb-b-connector
>>>>>
>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>>>> ---
>>>>>    .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
>>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>> @@ -17,6 +17,16 @@ Optional properties:
>>>>>    - self-powered: Set this property if the usb device that has its own power
>>>>>      source.
>>>>> +Optional properties for usb-b-connector:
>>>>> +- id-gpios: gpio for USB ID pin.
>>>>
>>>> What about boards where the ID pin is *not* connected to a GPIO,
>>>> but e.g. to a special pin on the PMIC which can also detect
>>>> an ACA adapter ? Currently this case is handled by extcon
>>>> drivers, but we have no way to set e.g. vbus-supply for the
>>>> connector. Maybe in this case the usb-connector node should
>>>> be a child of the PMIC node ?
>>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
>>
>> Ok, then I think this should be documented too.
>>
>>>> And in many cases there also is a mux to switch the datalines
>>>> between the host and device(gadget) controllers, how should
>>>> that be described in this model?  See the new usb-role-switch
>>>> code under drivers/usb/roles
>>>>
>>>> In some cases the mux is controlled through a gpio, so we
>>>> may want to add a "mux-gpios" here in which case we also
>>>> need to define what 0/1 means.
>>> I'm not sure, the mux seems not belong to this connector,
>>> and may need another driver to register usb-role-switch,
>>> similar to:
>>>
>>> [v2,2/2] usb: typec: add typec switch via GPIO control
>>> https://patchwork.kernel.org/patch/10834327/
>>
>> Right the mux/role-switch will need a driver, but the "owner"
>> of the usb_connector, e.g. the PMIC or the owner of the
>> id GPIO pin needs to know which device is the role-switch so
>> that it can set the role correctly based on the id-pin.
>>
>> Your binding already contains Vbus info, allowing the owner
>> of the usb_connector to enable/disable Vbus based on the id-pin,
>> but the owner will also be responsible for setting the role-switch.
>>
>> Note we cannot simply assume there will be only one role-switch,
>> we really need some link from the usb_connector to the role-switch
>> (or if it is a GPIO driven role-switch simply a role-switch-gpios
>> member in the usb_connector).
> 
> I see now in your "[PATCH v3 1/2] dt-bindings: usb: add documentation for
> typec switch via GPIO" that you plan to use a
> Documentation/devicetree/bindings/graph.txt style binding for this,
> adding a remote-endpoint to the orientation-switch (in that patch), or
> to the role-switch.
> 
> But a Type-C port can have both an orientation-switch and a role-switch
> (and a mux if it supports e.g. DP-altmode), how is the driver driving
> the device which has the actual usb_c_connecter child-node to which
> the remote-endpoints for both the orientation- and the role-switch points
> supposed to figure out which is which ?
> 
> Also it feels the wrong-way around to me to have the orientation-switch
> point to the usb_c_connector, to me it would make more sense to have
> the usb_c_connector point to a port on the orientation-switch.

I just noticed you put an "orientation-switch" bool property in the device
which has the orientation-switch property, if we do something similar for
the role-switch then that should solve this.

Regards,

Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Hi Jun,

   Please pay attention to Hans' comments for your patch

On Mon, 2019-03-11 at 12:04 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 12:00, Hans de Goede wrote:
> > Hi,
> > 
> > On 11-03-19 09:06, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 11-03-19 06:33, Chunfeng Yun wrote:
> >>> Hi,
> >>>
> >>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 08-03-19 07:13, Chunfeng Yun wrote:
> >>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> >>>>> usb-b-connector
> >>>>>
> >>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>>>> ---
> >>>>>    .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> >>>>>    1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
> >>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>> @@ -17,6 +17,16 @@ Optional properties:
> >>>>>    - self-powered: Set this property if the usb device that has its own power
> >>>>>      source.
> >>>>> +Optional properties for usb-b-connector:
> >>>>> +- id-gpios: gpio for USB ID pin.
> >>>>
> >>>> What about boards where the ID pin is *not* connected to a GPIO,
> >>>> but e.g. to a special pin on the PMIC which can also detect
> >>>> an ACA adapter ? Currently this case is handled by extcon
> >>>> drivers, but we have no way to set e.g. vbus-supply for the
> >>>> connector. Maybe in this case the usb-connector node should
> >>>> be a child of the PMIC node ?
> >>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
> >>
> >> Ok, then I think this should be documented too.
> >>
> >>>> And in many cases there also is a mux to switch the datalines
> >>>> between the host and device(gadget) controllers, how should
> >>>> that be described in this model?  See the new usb-role-switch
> >>>> code under drivers/usb/roles
> >>>>
> >>>> In some cases the mux is controlled through a gpio, so we
> >>>> may want to add a "mux-gpios" here in which case we also
> >>>> need to define what 0/1 means.
> >>> I'm not sure, the mux seems not belong to this connector,
> >>> and may need another driver to register usb-role-switch,
> >>> similar to:
> >>>
> >>> [v2,2/2] usb: typec: add typec switch via GPIO control
> >>> https://patchwork.kernel.org/patch/10834327/
> >>
> >> Right the mux/role-switch will need a driver, but the "owner"
> >> of the usb_connector, e.g. the PMIC or the owner of the
> >> id GPIO pin needs to know which device is the role-switch so
> >> that it can set the role correctly based on the id-pin.
> >>
> >> Your binding already contains Vbus info, allowing the owner
> >> of the usb_connector to enable/disable Vbus based on the id-pin,
> >> but the owner will also be responsible for setting the role-switch.
> >>
> >> Note we cannot simply assume there will be only one role-switch,
> >> we really need some link from the usb_connector to the role-switch
> >> (or if it is a GPIO driven role-switch simply a role-switch-gpios
> >> member in the usb_connector).
> > 
> > I see now in your "[PATCH v3 1/2] dt-bindings: usb: add documentation for
> > typec switch via GPIO" that you plan to use a
> > Documentation/devicetree/bindings/graph.txt style binding for this,
> > adding a remote-endpoint to the orientation-switch (in that patch), or
> > to the role-switch.
> > 
> > But a Type-C port can have both an orientation-switch and a role-switch
> > (and a mux if it supports e.g. DP-altmode), how is the driver driving
> > the device which has the actual usb_c_connecter child-node to which
> > the remote-endpoints for both the orientation- and the role-switch points
> > supposed to figure out which is which ?
> > 
> > Also it feels the wrong-way around to me to have the orientation-switch
> > point to the usb_c_connector, to me it would make more sense to have
> > the usb_c_connector point to a port on the orientation-switch.
> 
> I just noticed you put an "orientation-switch" bool property in the device
> which has the orientation-switch property, if we do something similar for
> the role-switch then that should solve this.
> 
> Regards,
> 
> Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Hi,
On Mon, 2019-03-11 at 09:06 +0100, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 06:33, Chunfeng Yun wrote:
> > Hi,
> > 
> > On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 08-03-19 07:13, Chunfeng Yun wrote:
> >>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> >>> usb-b-connector
> >>>
> >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>> ---
> >>>    .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> >>>    1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>> index a9a2f2fc44f2..7a07b0f4f973 100644
> >>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>> @@ -17,6 +17,16 @@ Optional properties:
> >>>    - self-powered: Set this property if the usb device that has its own power
> >>>      source.
> >>>    
> >>> +Optional properties for usb-b-connector:
> >>> +- id-gpios: gpio for USB ID pin.
> >>
> >> What about boards where the ID pin is *not* connected to a GPIO,
> >> but e.g. to a special pin on the PMIC which can also detect
> >> an ACA adapter ? Currently this case is handled by extcon
> >> drivers, but we have no way to set e.g. vbus-supply for the
> >> connector. Maybe in this case the usb-connector node should
> >> be a child of the PMIC node ?
> > Yes, it would be, PMIC is in charger of detecting the status of ID pin
> 
> Ok, then I think this should be documented too.
> 
> >> And in many cases there also is a mux to switch the datalines
> >> between the host and device(gadget) controllers, how should
> >> that be described in this model?  See the new usb-role-switch
> >> code under drivers/usb/roles
> >>
> >> In some cases the mux is controlled through a gpio, so we
> >> may want to add a "mux-gpios" here in which case we also
> >> need to define what 0/1 means.
> > I'm not sure, the mux seems not belong to this connector,
> > and may need another driver to register usb-role-switch,
> > similar to:
> > 
> > [v2,2/2] usb: typec: add typec switch via GPIO control
> > https://patchwork.kernel.org/patch/10834327/
> 
> Right the mux/role-switch will need a driver, but the "owner"
> of the usb_connector, e.g. the PMIC or the owner of the
> id GPIO pin needs to know which device is the role-switch so
> that it can set the role correctly based on the id-pin.
> 
> Your binding already contains Vbus info, allowing the owner
> of the usb_connector to enable/disable Vbus based on the id-pin,
> but the owner will also be responsible for setting the role-switch.
In this patch series, I make usb_connector driver enable/disable Vbus,
but not the parent(USB controller) of usb_connector which registers a
usb-role-switch, which way do you think it is better? 

Thanks
> 
> Note we cannot simply assume there will be only one role-switch,
> we really need some link from the usb_connector to the role-switch
> (or if it is a GPIO driven role-switch simply a role-switch-gpios
> member in the usb_connector).
> 
> Regards,
> 
> Hans
> 

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Hans de Goede]

Hi,

On 12-03-19 04:18, Chunfeng Yun wrote:
> Hi,
> On Mon, 2019-03-11 at 09:06 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11-03-19 06:33, Chunfeng Yun wrote:
>>> Hi,
>>>
>>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 08-03-19 07:13, Chunfeng Yun wrote:
>>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
>>>>> usb-b-connector
>>>>>
>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>>>> ---
>>>>>     .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
>>>>>     1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
>>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>> @@ -17,6 +17,16 @@ Optional properties:
>>>>>     - self-powered: Set this property if the usb device that has its own power
>>>>>       source.
>>>>>     
>>>>> +Optional properties for usb-b-connector:
>>>>> +- id-gpios: gpio for USB ID pin.
>>>>
>>>> What about boards where the ID pin is *not* connected to a GPIO,
>>>> but e.g. to a special pin on the PMIC which can also detect
>>>> an ACA adapter ? Currently this case is handled by extcon
>>>> drivers, but we have no way to set e.g. vbus-supply for the
>>>> connector. Maybe in this case the usb-connector node should
>>>> be a child of the PMIC node ?
>>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
>>
>> Ok, then I think this should be documented too.
>>
>>>> And in many cases there also is a mux to switch the datalines
>>>> between the host and device(gadget) controllers, how should
>>>> that be described in this model?  See the new usb-role-switch
>>>> code under drivers/usb/roles
>>>>
>>>> In some cases the mux is controlled through a gpio, so we
>>>> may want to add a "mux-gpios" here in which case we also
>>>> need to define what 0/1 means.
>>> I'm not sure, the mux seems not belong to this connector,
>>> and may need another driver to register usb-role-switch,
>>> similar to:
>>>
>>> [v2,2/2] usb: typec: add typec switch via GPIO control
>>> https://patchwork.kernel.org/patch/10834327/
>>
>> Right the mux/role-switch will need a driver, but the "owner"
>> of the usb_connector, e.g. the PMIC or the owner of the
>> id GPIO pin needs to know which device is the role-switch so
>> that it can set the role correctly based on the id-pin.
>>
>> Your binding already contains Vbus info, allowing the owner
>> of the usb_connector to enable/disable Vbus based on the id-pin,
>> but the owner will also be responsible for setting the role-switch.
> In this patch series, I make usb_connector driver enable/disable Vbus,
> but not the parent(USB controller) of usb_connector which registers a
> usb-role-switch, which way do you think it is better?

IMHO there should not be a driver for the usb_connector child-node,
only for the parent-device of that child-node.

There are going to be too many specific setups surrounding PMICs to
be able to do a single generic driver for the connector.

Regards,

Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Hi,
On Tue, 2019-03-12 at 13:45 +0100, Hans de Goede wrote:
> Hi,
> On 12-03-19 04:18, Chunfeng Yun wrote:
> > Hi,
> > On Mon, 2019-03-11 at 09:06 +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 11-03-19 06:33, Chunfeng Yun wrote:
> >>> Hi,
> >>>
> >>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 08-03-19 07:13, Chunfeng Yun wrote:
> >>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> >>>>> usb-b-connector
> >>>>>
> >>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>>>> ---
> >>>>>     .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> >>>>>     1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
> >>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>> @@ -17,6 +17,16 @@ Optional properties:
> >>>>>     - self-powered: Set this property if the usb device that has its own power
> >>>>>       source.
> >>>>>     
> >>>>> +Optional properties for usb-b-connector:
> >>>>> +- id-gpios: gpio for USB ID pin.
> >>>>
> >>>> What about boards where the ID pin is *not* connected to a GPIO,
> >>>> but e.g. to a special pin on the PMIC which can also detect
> >>>> an ACA adapter ? Currently this case is handled by extcon
> >>>> drivers, but we have no way to set e.g. vbus-supply for the
> >>>> connector. Maybe in this case the usb-connector node should
> >>>> be a child of the PMIC node ?
> >>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
> >>
> >> Ok, then I think this should be documented too.
> >>
> >>>> And in many cases there also is a mux to switch the datalines
> >>>> between the host and device(gadget) controllers, how should
> >>>> that be described in this model?  See the new usb-role-switch
> >>>> code under drivers/usb/roles
> >>>>
> >>>> In some cases the mux is controlled through a gpio, so we
> >>>> may want to add a "mux-gpios" here in which case we also
> >>>> need to define what 0/1 means.
> >>> I'm not sure, the mux seems not belong to this connector,
> >>> and may need another driver to register usb-role-switch,
> >>> similar to:
> >>>
> >>> [v2,2/2] usb: typec: add typec switch via GPIO control
> >>> https://patchwork.kernel.org/patch/10834327/
> >>
> >> Right the mux/role-switch will need a driver, but the "owner"
> >> of the usb_connector, e.g. the PMIC or the owner of the
> >> id GPIO pin needs to know which device is the role-switch so
> >> that it can set the role correctly based on the id-pin.
> >>
> >> Your binding already contains Vbus info, allowing the owner
> >> of the usb_connector to enable/disable Vbus based on the id-pin,
> >> but the owner will also be responsible for setting the role-switch.
> > In this patch series, I make usb_connector driver enable/disable Vbus,
> > but not the parent(USB controller) of usb_connector which registers a
> > usb-role-switch, which way do you think it is better?
> 
> IMHO there should not be a driver for the usb_connector child-node,
> only for the parent-device of that child-node.
If so, each USB controller driver should process this special case by
itself when use a gpio to detect the status of ID pin, it's not a
friendly way. And it's easy by using extcon-usb-gpio driver before, so
we also want to provide a simple way when support usb_connector, no
matter what method we choose.

Ideally, the only one thing that USB controller driver need to do, just
register a usb-role-switch, and leave decision when switch the role to
other drivers, such as type-C, PMIC/charger, also include gpio

> 
> There are going to be too many specific setups surrounding PMICs to
> be able to do a single generic driver for the connector.
> 
Fortunately, these patches only focus on the simplest gpio driven role
switch :)

Thanks

> Regards,
> 
> Hans
> 

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Hans de Goede]

Hi,

On 3/13/19 11:15 AM, Chunfeng Yun wrote:
> Hi,
> On Tue, 2019-03-12 at 13:45 +0100, Hans de Goede wrote:
>> Hi,
>> On 12-03-19 04:18, Chunfeng Yun wrote:
>>> Hi,
>>> On Mon, 2019-03-11 at 09:06 +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 11-03-19 06:33, Chunfeng Yun wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 08-03-19 07:13, Chunfeng Yun wrote:
>>>>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
>>>>>>> usb-b-connector
>>>>>>>
>>>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>>>>>> ---
>>>>>>>      .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
>>>>>>>      1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
>>>>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>>>> @@ -17,6 +17,16 @@ Optional properties:
>>>>>>>      - self-powered: Set this property if the usb device that has its own power
>>>>>>>        source.
>>>>>>>      
>>>>>>> +Optional properties for usb-b-connector:
>>>>>>> +- id-gpios: gpio for USB ID pin.
>>>>>>
>>>>>> What about boards where the ID pin is *not* connected to a GPIO,
>>>>>> but e.g. to a special pin on the PMIC which can also detect
>>>>>> an ACA adapter ? Currently this case is handled by extcon
>>>>>> drivers, but we have no way to set e.g. vbus-supply for the
>>>>>> connector. Maybe in this case the usb-connector node should
>>>>>> be a child of the PMIC node ?
>>>>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
>>>>
>>>> Ok, then I think this should be documented too.
>>>>
>>>>>> And in many cases there also is a mux to switch the datalines
>>>>>> between the host and device(gadget) controllers, how should
>>>>>> that be described in this model?  See the new usb-role-switch
>>>>>> code under drivers/usb/roles
>>>>>>
>>>>>> In some cases the mux is controlled through a gpio, so we
>>>>>> may want to add a "mux-gpios" here in which case we also
>>>>>> need to define what 0/1 means.
>>>>> I'm not sure, the mux seems not belong to this connector,
>>>>> and may need another driver to register usb-role-switch,
>>>>> similar to:
>>>>>
>>>>> [v2,2/2] usb: typec: add typec switch via GPIO control
>>>>> https://patchwork.kernel.org/patch/10834327/
>>>>
>>>> Right the mux/role-switch will need a driver, but the "owner"
>>>> of the usb_connector, e.g. the PMIC or the owner of the
>>>> id GPIO pin needs to know which device is the role-switch so
>>>> that it can set the role correctly based on the id-pin.
>>>>
>>>> Your binding already contains Vbus info, allowing the owner
>>>> of the usb_connector to enable/disable Vbus based on the id-pin,
>>>> but the owner will also be responsible for setting the role-switch.
>>> In this patch series, I make usb_connector driver enable/disable Vbus,
>>> but not the parent(USB controller) of usb_connector which registers a
>>> usb-role-switch, which way do you think it is better?
>>
>> IMHO there should not be a driver for the usb_connector child-node,
>> only for the parent-device of that child-node.
> If so, each USB controller driver should process this special case by
> itself when use a gpio to detect the status of ID pin, it's not a
> friendly way. And it's easy by using extcon-usb-gpio driver before, so
> we also want to provide a simple way when support usb_connector, no
> matter what method we choose.
> 
> Ideally, the only one thing that USB controller driver need to do, just
> register a usb-role-switch, and leave decision when switch the role to
> other drivers, such as type-C, PMIC/charger, also include gpio
> 
>>
>> There are going to be too many specific setups surrounding PMICs to
>> be able to do a single generic driver for the connector.
>>
> Fortunately, these patches only focus on the simplest gpio driven role
> switch :)

In that case there should be an extra compatible added to the
usb_b_connector node to which the "simple gpio" driver binds,
so that not all devicetree-s declaring a usb_b_connector child-node
automatically get that driver bound, and the gpio / vbus
properties should be mandatory properties for usb_b_connector
child nodes declaring the extra compatible, rather then being
optional for the generic compatible.

Regards,

Hans

Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun]

Hi,
On Wed, 2019-03-13 at 11:18 +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/13/19 11:15 AM, Chunfeng Yun wrote:
> > Hi,
> > On Tue, 2019-03-12 at 13:45 +0100, Hans de Goede wrote:
> >> Hi,
> >> On 12-03-19 04:18, Chunfeng Yun wrote:
> >>> Hi,
> >>> On Mon, 2019-03-11 at 09:06 +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 11-03-19 06:33, Chunfeng Yun wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 08-03-19 07:13, Chunfeng Yun wrote:
> >>>>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> >>>>>>> usb-b-connector
> >>>>>>>
> >>>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>>>>>> ---
> >>>>>>>      .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> >>>>>>>      1 file changed, 10 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>>>> @@ -17,6 +17,16 @@ Optional properties:
> >>>>>>>      - self-powered: Set this property if the usb device that has its own power
> >>>>>>>        source.
> >>>>>>>      
> >>>>>>> +Optional properties for usb-b-connector:
> >>>>>>> +- id-gpios: gpio for USB ID pin.
> >>>>>>
> >>>>>> What about boards where the ID pin is *not* connected to a GPIO,
> >>>>>> but e.g. to a special pin on the PMIC which can also detect
> >>>>>> an ACA adapter ? Currently this case is handled by extcon
> >>>>>> drivers, but we have no way to set e.g. vbus-supply for the
> >>>>>> connector. Maybe in this case the usb-connector node should
> >>>>>> be a child of the PMIC node ?
> >>>>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
> >>>>
> >>>> Ok, then I think this should be documented too.
> >>>>
> >>>>>> And in many cases there also is a mux to switch the datalines
> >>>>>> between the host and device(gadget) controllers, how should
> >>>>>> that be described in this model?  See the new usb-role-switch
> >>>>>> code under drivers/usb/roles
> >>>>>>
> >>>>>> In some cases the mux is controlled through a gpio, so we
> >>>>>> may want to add a "mux-gpios" here in which case we also
> >>>>>> need to define what 0/1 means.
> >>>>> I'm not sure, the mux seems not belong to this connector,
> >>>>> and may need another driver to register usb-role-switch,
> >>>>> similar to:
> >>>>>
> >>>>> [v2,2/2] usb: typec: add typec switch via GPIO control
> >>>>> https://patchwork.kernel.org/patch/10834327/
> >>>>
> >>>> Right the mux/role-switch will need a driver, but the "owner"
> >>>> of the usb_connector, e.g. the PMIC or the owner of the
> >>>> id GPIO pin needs to know which device is the role-switch so
> >>>> that it can set the role correctly based on the id-pin.
> >>>>
> >>>> Your binding already contains Vbus info, allowing the owner
> >>>> of the usb_connector to enable/disable Vbus based on the id-pin,
> >>>> but the owner will also be responsible for setting the role-switch.
> >>> In this patch series, I make usb_connector driver enable/disable Vbus,
> >>> but not the parent(USB controller) of usb_connector which registers a
> >>> usb-role-switch, which way do you think it is better?
> >>
> >> IMHO there should not be a driver for the usb_connector child-node,
> >> only for the parent-device of that child-node.
> > If so, each USB controller driver should process this special case by
> > itself when use a gpio to detect the status of ID pin, it's not a
> > friendly way. And it's easy by using extcon-usb-gpio driver before, so
> > we also want to provide a simple way when support usb_connector, no
> > matter what method we choose.
> > 
> > Ideally, the only one thing that USB controller driver need to do, just
> > register a usb-role-switch, and leave decision when switch the role to
> > other drivers, such as type-C, PMIC/charger, also include gpio
> > 
> >>
> >> There are going to be too many specific setups surrounding PMICs to
> >> be able to do a single generic driver for the connector.
> >>
> > Fortunately, these patches only focus on the simplest gpio driven role
> > switch :)
> 
> In that case there should be an extra compatible added to the
> usb_b_connector node to which the "simple gpio" driver binds,
> so that not all devicetree-s declaring a usb_b_connector child-node
> automatically get that driver bound, 
Yes
> and the gpio / vbus
> properties should be mandatory properties for usb_b_connector
> child nodes declaring the extra compatible, rather then being
> optional for the generic compatible.
At least one of id-gpios and vbus-gpios should exist, so both of them
are optional, of course if only vbus-gpios exists, then only device mode
is supported, in this case vbus is also optional.

> 
> Regards,
> 
> Hans
>