[v5 PATCH 0/6] add USB Type-B GPIO connector driver

[v5 PATCH 0/6] add USB Type-B GPIO 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, typically
using an input GPIO to detect USB ID pin.
This patch series introduce a Type-B GPIO connector driver and try
to replace the function provided by extcon-usb-gpio driver.

v5 changes:
  1. remove linux/of.h and put usb_role_switch when error happens,
     suggested by Biju
  2. treat Type-B connector as USB controller's child, but not as
     a virtual device, suggested by Rob
  3. provide and use generic property "usb-role-switch", see [1],
     suggested by Rob

Note: this series still depends on [2]

[1]: [v3] dt-binding: usb: add usb-role-switch property
      https://patchwork.kernel.org/patch/10934835/
[2]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
      https://patchwork.kernel.org/patch/10909971/

v4 changes:
  1. use switch_fwnode_match() to find fwnode suggested by Heikki
  2. assign fwnode member of usb_role_switch struct suggested by Heikki
  3. make [4/6] depend on [2]
  3. remove linux/gpio.h suggested by Linus
  4. put node when error happens

  [4/6] usb: roles: add API to get usb_role_switch by node
  [2] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
    https://patchwork.kernel.org/patch/10909971/

v3 changes:
  1. add GPIO direction, and use fixed-regulator for GPIO controlled
    VBUS regulator suggested by Rob;
  2. rebuild fwnode_usb_role_switch_get() suggested by Andy and Heikki
  3. treat the type-B connector as a virtual device;
  4. change file name of driver again
  5. select USB_ROLE_SWITCH in mtu3/Kconfig suggested by Heikki
  6. rename ssusb_mode_manual_switch() to ssusb_mode_switch()

v2 changes:
 1. make binding clear, and add a extra compatible suggested by Hans

Chunfeng Yun (6):
  dt-bindings: connector: add optional properties for Type-B
  dt-bindings: usb: add binding for Type-B GPIO connector driver
  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 GPIO connector driver
  usb: mtu3: register a USB Role Switch for dual role mode

 .../bindings/connector/usb-connector.txt      |  14 +
 .../devicetree/bindings/usb/mediatek,mtu3.txt |  10 +
 .../bindings/usb/typeb-conn-gpio.txt          |  42 +++
 drivers/usb/mtu3/Kconfig                      |   1 +
 drivers/usb/mtu3/mtu3.h                       |   5 +
 drivers/usb/mtu3/mtu3_debugfs.c               |   4 +-
 drivers/usb/mtu3/mtu3_dr.c                    |  48 ++-
 drivers/usb/mtu3/mtu3_dr.h                    |   6 +-
 drivers/usb/mtu3/mtu3_plat.c                  |   3 +-
 drivers/usb/roles/Kconfig                     |  11 +
 drivers/usb/roles/Makefile                    |   1 +
 drivers/usb/roles/class.c                     |  24 ++
 drivers/usb/roles/typeb-conn-gpio.c           | 295 ++++++++++++++++++
 include/linux/usb/role.h                      |   8 +
 14 files changed, 465 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
 create mode 100644 drivers/usb/roles/typeb-conn-gpio.c

-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 1/6] 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v5 no changes
v4 no changes

v3 changes:
 1. add GPIO direction, and use fixed-regulator for GPIO controlled
    VBUS regulator suggested by Rob;

v2 changes:
  1. describe more clear for vbus-gpios and vbus-supply suggested by Hans
---
 .../bindings/connector/usb-connector.txt           | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
index a9a2f2fc44f2..e8f9e854fd11 100644
--- a/Documentation/devicetree/bindings/connector/usb-connector.txt
+++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
@@ -17,6 +17,20 @@ 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: an input gpio for USB ID pin.
+- vbus-gpios: an input gpio for USB VBUS pin, used to detect presence of
+  VBUS 5V.
+  see gpio/gpio.txt.
+- vbus-supply: a phandle to the regulator for USB VBUS if needed when host
+  mode or dual role mode is supported.
+  Particularly, if use an output GPIO to control a VBUS regulator, should
+  model it as a regulator.
+  see regulator/fixed-regulator.yaml
+- 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.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 2/6] dt-bindings: usb: add binding for Type-B GPIO connector driver

[Chunfeng Yun]

It's used to support dual role switch via GPIO when use Type-B
receptacle, typically the USB ID pin is connected to an input
GPIO pin

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v5 changes:
 1. treat type-B connector as child device of USB controller's, but not
    as a separate virtual device, suggested by Rob
 2. put connector's port node under connector node, suggested by Rob

v4 no changes

v3 changes:
 1. treat type-B connector as a virtual device, but not child device of
    USB controller's

v2 changes:
  1. new patch to make binding clear suggested by Hans
---
 .../bindings/usb/typeb-conn-gpio.txt          | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt

diff --git a/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
new file mode 100644
index 000000000000..20dd3499a348
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
@@ -0,0 +1,42 @@
+USB Type-B GPIO Connector
+
+This is used to switch dual role mode from the USB ID pin connected to
+an input GPIO pin.
+
+Required properties:
+- compatible : should include "linux,typeb-conn-gpio" and "usb-b-connector".
+- id-gpios, vbus-gpios : either one of them must be present, and both
+	can be present as well.
+- vbus-supply : can be present if needed when supports dual role mode or
+	host mode.
+	see connector/usb-connector.txt
+
+Sub-nodes:
+- port : should be present.
+	see graph.txt
+
+Example:
+
+&mtu3 {
+	status = "okay";
+
+	connector {
+		compatible = "linux,typeb-conn-gpio", "usb-b-connector";
+		label = "micro-USB";
+		type = "micro";
+		id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
+		vbus-supply = <&usb_p0_vbus>;
+
+		port {
+			bconn_ep: endpoint@0 {
+				remote-endpoint = <&usb_role_sw>;
+			};
+		};
+	};
+
+	port {
+		usb_role_sw: endpoint@0 {
+			remote-endpoint = <&bconn_ep>;
+		};
+	};
+};
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 3/6] dt-bindings: usb: mtu3: add properties about USB Role Switch

[Chunfeng Yun]

Now the USB Role Switch is supported, so add properties about it,
and modify some description related.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v5 changes
 1. modify decription about extcon and vbus-supply properties
 2. make this patch depend on [1]

 [1]: [v3] dt-binding: usb: add usb-role-switch property
      https://patchwork.kernel.org/patch/10934835/

v4: no changes
v3: no changes

v2 changes:
  1. fix typo
  2. refer new binding about connector property
---
 .../devicetree/bindings/usb/mediatek,mtu3.txt          | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt b/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
index 3382b5cb471d..3a8300205cdb 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtu3.txt
@@ -28,8 +28,13 @@ Optional properties:
 	parent's address space
  - extcon : external connector for vbus and idpin changes detection, needed
 	when supports dual-role mode.
+	it's considered valid for compatibility reasons, not allowed for
+	new bindings, and use "usb-role-switch" property instead.
  - vbus-supply : reference to the VBUS regulator, needed when supports
 	dual-role mode.
+	it's considered valid for compatibility reasons, not allowed for
+	new bindings, and put into a usb-connector node.
+	see connector/usb-connector.txt.
  - pinctrl-names : a pinctrl state named "default" is optional, and need be
 	defined if auto drd switch is enabled, that means the property dr_mode
 	is set as "otg", and meanwhile the property "mediatek,enable-manual-drd"
@@ -39,6 +44,8 @@ Optional properties:
 
  - maximum-speed : valid arguments are "super-speed", "high-speed" and
 	"full-speed"; refer to usb/generic.txt
+ - usb-role-switch : use USB Role Switch to support dual-role switch, but
+	not extcon; see usb/generic.txt.
  - enable-manual-drd : supports manual dual-role switch via debugfs; usually
 	used when receptacle is TYPE-A and also wants to support dual-role
 	mode.
@@ -61,6 +68,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 port would be added as subnode if use "usb-role-switch" property.
+	see graph.txt
+
 Example:
 ssusb: usb@11271000 {
 	compatible = "mediatek,mt8173-mtu3";
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

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

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
Tested-by: Biju Das <biju.das@bp.renesas.com>
---
v5 changes:
 1. remove linux/of.h suggested by Biju
 2. add tested by Biju

Note: still depends on [1]
 [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
      https://patchwork.kernel.org/patch/10909971/

v4 changes:
  1. use switch_fwnode_match() to find fwnode suggested by Heikki
  2. this patch now depends on [1]

 [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
    https://patchwork.kernel.org/patch/10909971/

v3 changes:
  1. use fwnodes instead of node suggested by Andy
  2. rebuild the API suggested by Heikki

v2 no changes
---
 drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
 include/linux/usb/role.h  |  8 ++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..4a1f09a41ec0 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get);
 
+/**
+ * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
+ * @fwnode: The fwnode that register USB role switch
+ *
+ * Finds and returns role switch registered by @fwnode. The reference count
+ * for the found switch is incremented.
+ */
+struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+	struct usb_role_switch *sw;
+	struct device *dev;
+
+	dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
+	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(fwnode_usb_role_switch_get);
+
 /**
  * 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 da2b9641b877..35d460f9ec40 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -48,6 +48,8 @@ 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(struct device *dev);
 void usb_role_switch_put(struct usb_role_switch *sw);
+struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);
 
 struct usb_role_switch *
 usb_role_switch_register(struct device *parent,
@@ -72,6 +74,12 @@ static inline struct usb_role_switch *usb_role_switch_get(struct device *dev)
 
 static inline void usb_role_switch_put(struct usb_role_switch *sw) { }
 
+static inline struct usb_role_switch *
+fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct usb_role_switch *
 usb_role_switch_register(struct device *parent,
 			 const struct usb_role_switch_desc *desc)
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 5/6] usb: roles: add USB Type-B GPIO 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 introduces a driver of Type-B connector which typically
uses an input GPIO to detect USB ID pin, and try to replace the
function provided by extcon-usb-gpio driver

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v5 changes:
  1. put usb_role_switch when error happens suggested by Biju
  2. don't treat bype-B connector as a virtual device suggested by Rob

v4 changes:
  1. remove linux/gpio.h suggested by Linus
  2. put node when error happens

v3 changes:
  1. treat bype-B connector as a virtual device;
  2. change file name again

v2 changes:
  1. file name is changed
  2. use new compatible
---
 drivers/usb/roles/Kconfig           |  11 ++
 drivers/usb/roles/Makefile          |   1 +
 drivers/usb/roles/typeb-conn-gpio.c | 295 ++++++++++++++++++++++++++++
 3 files changed, 307 insertions(+)
 create mode 100644 drivers/usb/roles/typeb-conn-gpio.c

diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
index f8b31aa67526..d1156e18a81a 100644
--- a/drivers/usb/roles/Kconfig
+++ b/drivers/usb/roles/Kconfig
@@ -26,4 +26,15 @@ 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 TYPEB_CONN_GPIO
+	tristate "USB Type-B GPIO Connector"
+	depends on GPIOLIB
+	help
+	  The driver supports USB role switch between host and device via GPIO
+	  based USB cable detection, used typically if an input GPIO is used
+	  to detect USB ID pin.
+
+	  To compile the driver as a module, choose M here: the module will
+	  be called typeb-conn-gpio.ko
+
 endif # USB_ROLE_SWITCH
diff --git a/drivers/usb/roles/Makefile b/drivers/usb/roles/Makefile
index 757a7d2797eb..5d5620d9d113 100644
--- a/drivers/usb/roles/Makefile
+++ b/drivers/usb/roles/Makefile
@@ -3,3 +3,4 @@
 obj-$(CONFIG_USB_ROLE_SWITCH)		+= roles.o
 roles-y					:= class.o
 obj-$(CONFIG_USB_ROLES_INTEL_XHCI)	+= intel-xhci-usb-role-switch.o
+obj-$(CONFIG_TYPEB_CONN_GPIO)		+= typeb-conn-gpio.o
diff --git a/drivers/usb/roles/typeb-conn-gpio.c b/drivers/usb/roles/typeb-conn-gpio.c
new file mode 100644
index 000000000000..988ecf565f33
--- /dev/null
+++ b/drivers/usb/roles/typeb-conn-gpio.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB Type-B GPIO 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/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/pinctrl/consumer.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 *node = dev->of_node;
+	struct device_node *remote_node;
+	struct usb_conn_info *info;
+	int ret = 0;
+
+	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);
+	}
+
+	remote_node = of_graph_get_remote_node(node, -1, 0);
+	if (!remote_node) {
+		dev_err(dev, "failed to get remote node\n");
+		return -ENODEV;
+	}
+
+	info->role_sw =
+		fwnode_usb_role_switch_get(of_fwnode_handle(remote_node));
+	of_node_put(remote_node);
+	if (IS_ERR(info->role_sw)) {
+		dev_err(dev, "failed to get role switch\n");
+		return PTR_ERR(info->role_sw);
+	}
+
+	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");
+			ret = info->id_irq;
+			goto put_role_sw;
+		}
+
+		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");
+			goto put_role_sw;
+		}
+	}
+
+	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");
+			ret = info->vbus_irq;
+			goto put_role_sw;
+		}
+
+		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");
+			goto put_role_sw;
+		}
+	}
+
+	platform_set_drvdata(pdev, info);
+
+	/* Perform initial detection */
+	usb_conn_queue_dwork(info, 0);
+
+	return 0;
+
+put_role_sw:
+	usb_role_switch_put(info->role_sw);
+	return ret;
+}
+
+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 = "linux,typeb-conn-gpio", },
+	{ }
+};
+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	= "typeb-conn-gpio",
+		.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 Type-B GPIO connector driver");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 6/6] usb: mtu3: register a USB Role Switch for dual role mode

[Chunfeng Yun]

Because 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>
---
v5 no change

v4 changes:
  1. assign fwnode member of usb_role_switch struct suggested by Heikki

v3 changes:
  1. select USB_ROLE_SWITCH in Kconfig suggested by Heikki
  2. rename ssusb_mode_manual_switch() to ssusb_mode_switch()

v2 no change
---
 drivers/usb/mtu3/Kconfig        |  1 +
 drivers/usb/mtu3/mtu3.h         |  5 ++++
 drivers/usb/mtu3/mtu3_debugfs.c |  4 +--
 drivers/usb/mtu3/mtu3_dr.c      | 48 ++++++++++++++++++++++++++++++++-
 drivers/usb/mtu3/mtu3_dr.h      |  6 ++---
 drivers/usb/mtu3/mtu3_plat.c    |  3 ++-
 6 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/mtu3/Kconfig b/drivers/usb/mtu3/Kconfig
index bcc23486c4ed..88e3db7b3016 100644
--- a/drivers/usb/mtu3/Kconfig
+++ b/drivers/usb/mtu3/Kconfig
@@ -43,6 +43,7 @@ config USB_MTU3_DUAL_ROLE
 	bool "Dual Role mode"
 	depends on ((USB=y || USB=USB_MTU3) && (USB_GADGET=y || USB_GADGET=USB_MTU3))
 	depends on (EXTCON=y || EXTCON=USB_MTU3)
+	select USB_ROLE_SWITCH
 	help
 	  This is the default mode of working of MTU3 controller where
 	  both host and gadget features are enabled.
diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
index 76ecf12fdf62..6087be236a35 100644
--- a/drivers/usb/mtu3/mtu3.h
+++ b/drivers/usb/mtu3/mtu3.h
@@ -199,6 +199,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.
@@ -212,6 +215,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_debugfs.c b/drivers/usb/mtu3/mtu3_debugfs.c
index b7c86ccd50b4..3ed666f94dd9 100644
--- a/drivers/usb/mtu3/mtu3_debugfs.c
+++ b/drivers/usb/mtu3/mtu3_debugfs.c
@@ -453,9 +453,9 @@ static ssize_t ssusb_mode_write(struct file *file, const char __user *ubuf,
 		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;
diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
index 5fcb71af875a..08e18448e8b8 100644
--- a/drivers/usb/mtu3/mtu3_dr.c
+++ b/drivers/usb/mtu3/mtu3_dr.c
@@ -7,6 +7,8 @@
  * Author: Chunfeng Yun <chunfeng.yun@mediatek.com>
  */
 
+#include <linux/usb/role.h>
+
 #include "mtu3.h"
 #include "mtu3_dr.h"
 #include "mtu3_debug.h"
@@ -280,7 +282,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.
  */
-void ssusb_mode_manual_switch(struct ssusb_mtk *ssusb, int to_host)
+void ssusb_mode_switch(struct ssusb_mtk *ssusb, int to_host)
 {
 	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
 
@@ -318,6 +320,47 @@ 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;
+	role_sx_desc.fwnode = dev_fwnode(ssusb->dev);
+	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;
@@ -328,6 +371,8 @@ int ssusb_otg_switch_init(struct ssusb_mtk *ssusb)
 
 	if (otg_sx->manual_drd_enabled)
 		ssusb_dr_debugfs_init(ssusb);
+	else if (otg_sx->role_sw_used)
+		ret = ssusb_role_sw_register(otg_sx);
 	else
 		ret = ssusb_extcon_register(otg_sx);
 
@@ -340,4 +385,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_dr.h b/drivers/usb/mtu3/mtu3_dr.h
index ba6fe357ce29..5e58c4dbd54a 100644
--- a/drivers/usb/mtu3/mtu3_dr.h
+++ b/drivers/usb/mtu3/mtu3_dr.h
@@ -71,7 +71,7 @@ static inline void ssusb_gadget_exit(struct ssusb_mtk *ssusb)
 #if IS_ENABLED(CONFIG_USB_MTU3_DUAL_ROLE)
 int ssusb_otg_switch_init(struct ssusb_mtk *ssusb);
 void ssusb_otg_switch_exit(struct ssusb_mtk *ssusb);
-void ssusb_mode_manual_switch(struct ssusb_mtk *ssusb, int to_host);
+void ssusb_mode_switch(struct ssusb_mtk *ssusb, int to_host);
 int ssusb_set_vbus(struct otg_switch_mtk *otg_sx, int is_on);
 void ssusb_set_force_mode(struct ssusb_mtk *ssusb,
 			  enum mtu3_dr_force_mode mode);
@@ -86,8 +86,8 @@ static inline int ssusb_otg_switch_init(struct ssusb_mtk *ssusb)
 static inline void ssusb_otg_switch_exit(struct ssusb_mtk *ssusb)
 {}
 
-static inline void
-ssusb_mode_manual_switch(struct ssusb_mtk *ssusb, int to_host) {}
+static inline void ssusb_mode_switch(struct ssusb_mtk *ssusb, int to_host)
+{}
 
 static inline int ssusb_set_vbus(struct otg_switch_mtk *otg_sx, int is_on)
 {
diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
index fd0f6c5dfbc1..9c256ea3cdf5 100644
--- a/drivers/usb/mtu3/mtu3_plat.c
+++ b/drivers/usb/mtu3/mtu3_plat.c
@@ -299,8 +299,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.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/6] dt-bindings: usb: add binding for Type-B GPIO connector driver

[Rob Herring]

On Tue, May 14, 2019 at 04:47:19PM +0800, Chunfeng Yun wrote:
> It's used to support dual role switch via GPIO when use Type-B
> receptacle, typically the USB ID pin is connected to an input
> GPIO pin
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v5 changes:
>  1. treat type-B connector as child device of USB controller's, but not
>     as a separate virtual device, suggested by Rob
>  2. put connector's port node under connector node, suggested by Rob
> 
> v4 no changes
> 
> v3 changes:
>  1. treat type-B connector as a virtual device, but not child device of
>     USB controller's
> 
> v2 changes:
>   1. new patch to make binding clear suggested by Hans
> ---
>  .../bindings/usb/typeb-conn-gpio.txt          | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> new file mode 100644
> index 000000000000..20dd3499a348
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> @@ -0,0 +1,42 @@
> +USB Type-B GPIO Connector
> +
> +This is used to switch dual role mode from the USB ID pin connected to
> +an input GPIO pin.
> +
> +Required properties:
> +- compatible : should include "linux,typeb-conn-gpio" and "usb-b-connector".

I don't think we need "linux,typeb-conn-gpio". A driver can decide to 
handle GPIO lines if they present or we assume the parent device handles 
ID and/or Vbus if they are not present.

> +- id-gpios, vbus-gpios : either one of them must be present, and both
> +	can be present as well.

Please clarify that vbus-gpios is an input to sense Vbus presence as an 
output it should be modelled as a regulator only.

These should be added to usb-connector.txt.

The result of all this is you don't need this file. Just additions to 
usb-connector.txt.

> +- vbus-supply : can be present if needed when supports dual role mode or
> +	host mode.
> +	see connector/usb-connector.txt
> +
> +Sub-nodes:
> +- port : should be present.
> +	see graph.txt
> +
> +Example:
> +
> +&mtu3 {
> +	status = "okay";

Don't show status in examples.

> +
> +	connector {
> +		compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> +		label = "micro-USB";
> +		type = "micro";
> +		id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> +		vbus-supply = <&usb_p0_vbus>;
> +
> +		port {
> +			bconn_ep: endpoint@0 {
> +				remote-endpoint = <&usb_role_sw>;
> +			};
> +		};
> +	};
> +
> +	port {
> +		usb_role_sw: endpoint@0 {
> +			remote-endpoint = <&bconn_ep>;
> +		};
> +	};

When the host controller is the parent of the connector, you don't need 
the graph unless you're describing the alternate modes in Type-C.

> +};
> -- 
> 2.21.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 3/6] dt-bindings: usb: mtu3: add properties about USB Role Switch

[Rob Herring]

On Tue, 14 May 2019 16:47:20 +0800, Chunfeng Yun wrote:
> Now the USB Role Switch is supported, so add properties about it,
> and modify some description related.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v5 changes
>  1. modify decription about extcon and vbus-supply properties
>  2. make this patch depend on [1]
> 
>  [1]: [v3] dt-binding: usb: add usb-role-switch property
>       https://patchwork.kernel.org/patch/10934835/
> 
> v4: no changes
> v3: no changes
> 
> v2 changes:
>   1. fix typo
>   2. refer new binding about connector property
> ---
>  .../devicetree/bindings/usb/mediatek,mtu3.txt          | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/6] dt-bindings: usb: add binding for Type-B GPIO connector driver

[Chunfeng Yun]

On Tue, 2019-05-14 at 13:12 -0500, Rob Herring wrote:
> On Tue, May 14, 2019 at 04:47:19PM +0800, Chunfeng Yun wrote:
> > It's used to support dual role switch via GPIO when use Type-B
> > receptacle, typically the USB ID pin is connected to an input
> > GPIO pin
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v5 changes:
> >  1. treat type-B connector as child device of USB controller's, but not
> >     as a separate virtual device, suggested by Rob
> >  2. put connector's port node under connector node, suggested by Rob
> > 
> > v4 no changes
> > 
> > v3 changes:
> >  1. treat type-B connector as a virtual device, but not child device of
> >     USB controller's
> > 
> > v2 changes:
> >   1. new patch to make binding clear suggested by Hans
> > ---
> >  .../bindings/usb/typeb-conn-gpio.txt          | 42 +++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> > new file mode 100644
> > index 000000000000..20dd3499a348
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typeb-conn-gpio.txt
> > @@ -0,0 +1,42 @@
> > +USB Type-B GPIO Connector
> > +
> > +This is used to switch dual role mode from the USB ID pin connected to
> > +an input GPIO pin.
> > +
> > +Required properties:
> > +- compatible : should include "linux,typeb-conn-gpio" and "usb-b-connector".
> 
> I don't think we need "linux,typeb-conn-gpio". 
Not all usb-b-connector child node need bind this driver, by adding the
new compatible can avoid unnecessary binding.

> A driver can decide to 
> handle GPIO lines if they present
Yes, the driver, e.g. USB controller driver can do it, but here I want
to provide a common driver to handle this special case, like
extcon-usb-gpio driver does, and try to keep transparency from USB
controller driver. 

>  or we assume the parent device handles 
> ID and/or Vbus if they are not present.
Yes, it will
> 
> > +- id-gpios, vbus-gpios : either one of them must be present, and both
> > +	can be present as well.
> 
> Please clarify that vbus-gpios is an input to sense Vbus presence as an 
> output it should be modelled as a regulator only.
Ok, will add more description.
> 
> These should be added to usb-connector.txt.
Already add them in [1/6].
> 
> The result of all this is you don't need this file. Just additions to 
> usb-connector.txt.
Here add more constrains for id-gpios and vbus-gpios, at least one
should be present, although they are both optional, this is not true for
some cases, so not suitable to add into usb-connector.txt.
> 
> > +- vbus-supply : can be present if needed when supports dual role mode or
> > +	host mode.
> > +	see connector/usb-connector.txt
> > +
> > +Sub-nodes:
> > +- port : should be present.
> > +	see graph.txt
> > +
> > +Example:
> > +
> > +&mtu3 {
> > +	status = "okay";
> 
> Don't show status in examples.
Ok, will drop it.
> 
> > +
> > +	connector {
> > +		compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> > +		label = "micro-USB";
> > +		type = "micro";
> > +		id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> > +		vbus-supply = <&usb_p0_vbus>;
> > +
> > +		port {
> > +			bconn_ep: endpoint@0 {
> > +				remote-endpoint = <&usb_role_sw>;
> > +			};
> > +		};
> > +	};
> > +
> > +	port {
> > +		usb_role_sw: endpoint@0 {
> > +			remote-endpoint = <&bconn_ep>;
> > +		};
> > +	};
> 
> When the host controller is the parent of the connector, you don't need 
> the graph unless you're describing the alternate modes in Type-C.
Ok, got it.

Thanks a lot.

> 
> > +};
> > -- 
> > 2.21.0
> > 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> Add fwnode_usb_role_switch_get() to make easier to get
> usb_role_switch by fwnode which register it.
> It's useful when there is not device_connection registered
> between two drivers and only knows the fwnode which register
> usb_role_switch.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Tested-by: Biju Das <biju.das@bp.renesas.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> v5 changes:
>  1. remove linux/of.h suggested by Biju
>  2. add tested by Biju
> 
> Note: still depends on [1]
>  [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
>       https://patchwork.kernel.org/patch/10909971/
> 
> v4 changes:
>   1. use switch_fwnode_match() to find fwnode suggested by Heikki
>   2. this patch now depends on [1]
> 
>  [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
>     https://patchwork.kernel.org/patch/10909971/
> 
> v3 changes:
>   1. use fwnodes instead of node suggested by Andy
>   2. rebuild the API suggested by Heikki
> 
> v2 no changes
> ---
>  drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
>  include/linux/usb/role.h  |  8 ++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f45d8df5cfb8..4a1f09a41ec0 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_get);
>  
> +/**
> + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> + * @fwnode: The fwnode that register USB role switch
> + *
> + * Finds and returns role switch registered by @fwnode. The reference count
> + * for the found switch is incremented.
> + */
> +struct usb_role_switch *
> +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> +{
> +	struct usb_role_switch *sw;
> +	struct device *dev;
> +
> +	dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> +	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(fwnode_usb_role_switch_get);
> +
>  /**
>   * 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 da2b9641b877..35d460f9ec40 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -48,6 +48,8 @@ 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(struct device *dev);
>  void usb_role_switch_put(struct usb_role_switch *sw);
> +struct usb_role_switch *
> +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode);
>  
>  struct usb_role_switch *
>  usb_role_switch_register(struct device *parent,
> @@ -72,6 +74,12 @@ static inline struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  
>  static inline void usb_role_switch_put(struct usb_role_switch *sw) { }
>  
> +static inline struct usb_role_switch *
> +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
>  static inline struct usb_role_switch *
>  usb_role_switch_register(struct device *parent,
>  			 const struct usb_role_switch_desc *desc)
> -- 
> 2.21.0

thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

Hi,

On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > Add fwnode_usb_role_switch_get() to make easier to get
> > usb_role_switch by fwnode which register it.
> > It's useful when there is not device_connection registered
> > between two drivers and only knows the fwnode which register
> > usb_role_switch.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Tested-by: Biju Das <biju.das@bp.renesas.com>
> 
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Hold on. I just noticed Rob's comment on patch 2/6, where he points out
that you don't need to use device graph since the controller is the
parent of the connector. Doesn't that mean you don't really need this
API?

> > ---
> > v5 changes:
> >  1. remove linux/of.h suggested by Biju
> >  2. add tested by Biju
> > 
> > Note: still depends on [1]
> >  [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> >       https://patchwork.kernel.org/patch/10909971/
> > 
> > v4 changes:
> >   1. use switch_fwnode_match() to find fwnode suggested by Heikki
> >   2. this patch now depends on [1]
> > 
> >  [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> >     https://patchwork.kernel.org/patch/10909971/
> > 
> > v3 changes:
> >   1. use fwnodes instead of node suggested by Andy
> >   2. rebuild the API suggested by Heikki
> > 
> > v2 no changes
> > ---
> >  drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> >  include/linux/usb/role.h  |  8 ++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index f45d8df5cfb8..4a1f09a41ec0 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_role_switch_get);
> >  
> > +/**
> > + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> > + * @fwnode: The fwnode that register USB role switch
> > + *
> > + * Finds and returns role switch registered by @fwnode. The reference count
> > + * for the found switch is incremented.
> > + */
> > +struct usb_role_switch *
> > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> > +{
> > +	struct usb_role_switch *sw;
> > +	struct device *dev;
> > +
> > +	dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> > +	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(fwnode_usb_role_switch_get);

This function only basically converts the fwnode to usb_role_switch,
but I would actually prefer that we walked through the device graph
here instead of expecting the caller to do that.

So this function should probable be called fwnode_to_usb_role_switch()
and not fwnode_usb_role_switch_get(), but I guess you don't need it
at all, right?


thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi,
On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > Add fwnode_usb_role_switch_get() to make easier to get
> > > usb_role_switch by fwnode which register it.
> > > It's useful when there is not device_connection registered
> > > between two drivers and only knows the fwnode which register
> > > usb_role_switch.
> > > 
> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > 
> > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Hold on. I just noticed Rob's comment on patch 2/6, where he points out
> that you don't need to use device graph since the controller is the
> parent of the connector. Doesn't that mean you don't really need this
> API?
No, I still need it. 
The change is about the way how to get fwnode;
when use device graph, get fwnode by of_graph_get_remote_node();
but now will get fwnode by of_get_parent();

> 
> > > ---
> > > v5 changes:
> > >  1. remove linux/of.h suggested by Biju
> > >  2. add tested by Biju
> > > 
> > > Note: still depends on [1]
> > >  [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > >       https://patchwork.kernel.org/patch/10909971/
> > > 
> > > v4 changes:
> > >   1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > >   2. this patch now depends on [1]
> > > 
> > >  [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > >     https://patchwork.kernel.org/patch/10909971/
> > > 
> > > v3 changes:
> > >   1. use fwnodes instead of node suggested by Andy
> > >   2. rebuild the API suggested by Heikki
> > > 
> > > v2 no changes
> > > ---
> > >  drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > >  include/linux/usb/role.h  |  8 ++++++++
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > > index f45d8df5cfb8..4a1f09a41ec0 100644
> > > --- a/drivers/usb/roles/class.c
> > > +++ b/drivers/usb/roles/class.c
> > > @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > >  
> > > +/**
> > > + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> > > + * @fwnode: The fwnode that register USB role switch
> > > + *
> > > + * Finds and returns role switch registered by @fwnode. The reference count
> > > + * for the found switch is incremented.
> > > + */
> > > +struct usb_role_switch *
> > > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> > > +{
> > > +	struct usb_role_switch *sw;
> > > +	struct device *dev;
> > > +
> > > +	dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> > > +	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(fwnode_usb_role_switch_get);
> 
> This function only basically converts the fwnode to usb_role_switch,
> but I would actually prefer that we walked through the device graph
> here instead of expecting the caller to do that.
> 
> So this function should probable be called fwnode_to_usb_role_switch()
> and not fwnode_usb_role_switch_get(), but I guess you don't need it
> at all, right?
> 
> 
> thanks,
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> Hi,
> On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > usb_role_switch by fwnode which register it.
> > > > It's useful when there is not device_connection registered
> > > > between two drivers and only knows the fwnode which register
> > > > usb_role_switch.
> > > > 
> > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > 
> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > Hold on. I just noticed Rob's comment on patch 2/6, where he points out
> > that you don't need to use device graph since the controller is the
> > parent of the connector. Doesn't that mean you don't really need this
> > API?
> No, I still need it. 
> The change is about the way how to get fwnode;
> when use device graph, get fwnode by of_graph_get_remote_node();
> but now will get fwnode by of_get_parent();

OK, I get that, but I'm still not convinced about if something like
this function is needed at all. I also have concerns regarding how you
are using the function. I'll explain in comment to the patch 5/6 in
this series...

> > > > ---
> > > > v5 changes:
> > > >  1. remove linux/of.h suggested by Biju
> > > >  2. add tested by Biju
> > > > 
> > > > Note: still depends on [1]
> > > >  [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > > >       https://patchwork.kernel.org/patch/10909971/
> > > > 
> > > > v4 changes:
> > > >   1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > > >   2. this patch now depends on [1]
> > > > 
> > > >  [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in role.h
> > > >     https://patchwork.kernel.org/patch/10909971/
> > > > 
> > > > v3 changes:
> > > >   1. use fwnodes instead of node suggested by Andy
> > > >   2. rebuild the API suggested by Heikki
> > > > 
> > > > v2 no changes
> > > > ---
> > > >  drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > > >  include/linux/usb/role.h  |  8 ++++++++
> > > >  2 files changed, 32 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > > > index f45d8df5cfb8..4a1f09a41ec0 100644
> > > > --- a/drivers/usb/roles/class.c
> > > > +++ b/drivers/usb/roles/class.c
> > > > @@ -135,6 +135,30 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > > >  
> > > > +/**
> > > > + * fwnode_usb_role_switch_get - Find USB role switch by it's parent fwnode
> > > > + * @fwnode: The fwnode that register USB role switch
> > > > + *
> > > > + * Finds and returns role switch registered by @fwnode. The reference count
> > > > + * for the found switch is incremented.
> > > > + */
> > > > +struct usb_role_switch *
> > > > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> > > > +{
> > > > +	struct usb_role_switch *sw;
> > > > +	struct device *dev;
> > > > +
> > > > +	dev = class_find_device(role_class, NULL, fwnode, switch_fwnode_match);
> > > > +	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(fwnode_usb_role_switch_get);
> > 
> > This function only basically converts the fwnode to usb_role_switch,
> > but I would actually prefer that we walked through the device graph
> > here instead of expecting the caller to do that.
> > 
> > So this function should probable be called fwnode_to_usb_role_switch()
> > and not fwnode_usb_role_switch_get(), but I guess you don't need it
> > at all, right?
> > 
> > 
> > thanks,
> > 
> 

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Biju Das]

Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, May 20, 2019 9:04 AM
> To: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Rob Herring <robh+dt@kernel.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Mark Rutland <mark.rutland@arm.com>;
> Matthias Brugger <matthias.bgg@gmail.com>; Adam Thomson
> <Adam.Thomson.Opensource@diasemi.com>; Li Jun <jun.li@nxp.com>;
> Badhri Jagan Sridharan <badhri@google.com>; Hans de Goede
> <hdegoede@redhat.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; Biju Das <biju.das@bp.renesas.com>; Linus
> Walleij <linus.walleij@linaro.org>
> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
> 
> On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > Hi,
> > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > usb_role_switch by fwnode which register it.
> > > > > It's useful when there is not device_connection registered
> > > > > between two drivers and only knows the fwnode which register
> > > > > usb_role_switch.
> > > > >
> > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > >
> > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >
> > > Hold on. I just noticed Rob's comment on patch 2/6, where he points
> > > out that you don't need to use device graph since the controller is
> > > the parent of the connector. Doesn't that mean you don't really need
> > > this API?
> > No, I still need it.
> > The change is about the way how to get fwnode; when use device graph,
> > get fwnode by of_graph_get_remote_node(); but now will get fwnode by
> > of_get_parent();
> 
> OK, I get that, but I'm still not convinced about if something like this function
> is needed at all. I also have concerns regarding how you are using the
> function. I'll explain in comment to the patch 5/6 in this series...

FYI, Currently  I am also using this api in my patch series.
https://patchwork.kernel.org/patch/10944637/

regards,
Biju

> > > > > ---
> > > > > v5 changes:
> > > > >  1. remove linux/of.h suggested by Biju  2. add tested by Biju
> > > > >
> > > > > Note: still depends on [1]
> > > > >  [1]: [v6,08/13] usb: roles: Introduce stubs for the exiting functions in
> role.h
> > > > >       https://patchwork.kernel.org/patch/10909971/
> > > > >
> > > > > v4 changes:
> > > > >   1. use switch_fwnode_match() to find fwnode suggested by Heikki
> > > > >   2. this patch now depends on [1]
> > > > >
> > > > >  [1] [v6,08/13] usb: roles: Introduce stubs for the exiting functions in
> role.h
> > > > >     https://patchwork.kernel.org/patch/10909971/
> > > > >
> > > > > v3 changes:
> > > > >   1. use fwnodes instead of node suggested by Andy
> > > > >   2. rebuild the API suggested by Heikki
> > > > >
> > > > > v2 no changes
> > > > > ---
> > > > >  drivers/usb/roles/class.c | 24 ++++++++++++++++++++++++
> > > > > include/linux/usb/role.h  |  8 ++++++++
> > > > >  2 files changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/roles/class.c
> > > > > b/drivers/usb/roles/class.c index f45d8df5cfb8..4a1f09a41ec0
> > > > > 100644
> > > > > --- a/drivers/usb/roles/class.c
> > > > > +++ b/drivers/usb/roles/class.c
> > > > > @@ -135,6 +135,30 @@ struct usb_role_switch
> > > > > *usb_role_switch_get(struct device *dev)  }
> > > > > EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > > > >
> > > > > +/**
> > > > > + * fwnode_usb_role_switch_get - Find USB role switch by it's
> > > > > +parent fwnode
> > > > > + * @fwnode: The fwnode that register USB role switch
> > > > > + *
> > > > > + * Finds and returns role switch registered by @fwnode. The
> > > > > +reference count
> > > > > + * for the found switch is incremented.
> > > > > + */
> > > > > +struct usb_role_switch *
> > > > > +fwnode_usb_role_switch_get(struct fwnode_handle *fwnode) {
> > > > > +	struct usb_role_switch *sw;
> > > > > +	struct device *dev;
> > > > > +
> > > > > +	dev = class_find_device(role_class, NULL, fwnode,
> switch_fwnode_match);
> > > > > +	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(fwnode_usb_role_switch_get);
> > >
> > > This function only basically converts the fwnode to usb_role_switch,
> > > but I would actually prefer that we walked through the device graph
> > > here instead of expecting the caller to do that.
> > >
> > > So this function should probable be called
> > > fwnode_to_usb_role_switch() and not fwnode_usb_role_switch_get(),
> > > but I guess you don't need it at all, right?
> > >
> > >
> > > thanks,
> > >
> >
> 
> --
> heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/6] usb: roles: add USB Type-B GPIO connector driver

[Heikki Krogerus]

On Tue, May 14, 2019 at 04:47:22PM +0800, Chunfeng Yun wrote:
> +static int usb_conn_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *remote_node;
> +	struct usb_conn_info *info;
> +	int ret = 0;
> +
> +	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);
> +	}
> +
> +	remote_node = of_graph_get_remote_node(node, -1, 0);

This is really not ideal. In practice this code will only work if
there is only one endpoint described for this device, or if the first
endpoint is always the one we are looking for. There is no way to
guarantee that.

The code really has to walk through the entire graph, and identify the
remote endpoint it's looking for (and for that we have the boolean
device property).

> +	if (!remote_node) {
> +		dev_err(dev, "failed to get remote node\n");
> +		return -ENODEV;
> +	}
> +
> +	info->role_sw =
> +		fwnode_usb_role_switch_get(of_fwnode_handle(remote_node));

So fwnode_usb_role_switch_get() needs be the one that walks through
the graph, not the drivers. Otherwise every driver will do the same
exact steps (boilerplate). Here you need to be able to just pass the
node of this device, not the remote endpoint:

        info->role_sw = fwnode_usb_role_switch_get(dev_fwnode(&client->dev));

But why do you need that function at all? Why wouldn't
usb_role_switch_get() work?

        info->role_sw = usb_role_switch_get(&client->dev);

> +	of_node_put(remote_node);
> +	if (IS_ERR(info->role_sw)) {
> +		dev_err(dev, "failed to get role switch\n");
> +		return PTR_ERR(info->role_sw);
> +	}
> +
> +	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");
> +			ret = info->id_irq;
> +			goto put_role_sw;
> +		}
> +
> +		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");
> +			goto put_role_sw;
> +		}
> +	}
> +
> +	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");
> +			ret = info->vbus_irq;
> +			goto put_role_sw;
> +		}
> +
> +		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");
> +			goto put_role_sw;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +
> +	/* Perform initial detection */
> +	usb_conn_queue_dwork(info, 0);
> +
> +	return 0;
> +
> +put_role_sw:
> +	usb_role_switch_put(info->role_sw);
> +	return ret;
> +}

thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> Hi Heikki,
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Monday, May 20, 2019 9:04 AM
> > To: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Cc: Rob Herring <robh+dt@kernel.org>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Mark Rutland <mark.rutland@arm.com>;
> > Matthias Brugger <matthias.bgg@gmail.com>; Adam Thomson
> > <Adam.Thomson.Opensource@diasemi.com>; Li Jun <jun.li@nxp.com>;
> > Badhri Jagan Sridharan <badhri@google.com>; Hans de Goede
> > <hdegoede@redhat.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; Biju Das <biju.das@bp.renesas.com>; Linus
> > Walleij <linus.walleij@linaro.org>
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > Hi,
> > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > Hi,
> > > >
> > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > usb_role_switch by fwnode which register it.
> > > > > > It's useful when there is not device_connection registered
> > > > > > between two drivers and only knows the fwnode which register
> > > > > > usb_role_switch.
> > > > > >
> > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > >
> > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > >
> > > > Hold on. I just noticed Rob's comment on patch 2/6, where he points
> > > > out that you don't need to use device graph since the controller is
> > > > the parent of the connector. Doesn't that mean you don't really need
> > > > this API?
> > > No, I still need it.
> > > The change is about the way how to get fwnode; when use device graph,
> > > get fwnode by of_graph_get_remote_node(); but now will get fwnode by
> > > of_get_parent();
> > 
> > OK, I get that, but I'm still not convinced about if something like this function
> > is needed at all. I also have concerns regarding how you are using the
> > function. I'll explain in comment to the patch 5/6 in this series...
> 
> FYI, Currently  I am also using this api in my patch series.
> https://patchwork.kernel.org/patch/10944637/

Yes, and I have the same question for you I jusb asked in comment I
added to the patch 5/6 of this series. Why isn't usb_role_switch_get()
enough?

thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Biju Das]

Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
> 
> On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > Hi Heikki,
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > Hi,
> > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > It's useful when there is not device_connection registered
> > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > usb_role_switch.
> > > > > > >
> > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > >
> > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > >
> > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > points out that you don't need to use device graph since the
> > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > don't really need this API?
> > > > No, I still need it.
> > > > The change is about the way how to get fwnode; when use device
> > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > fwnode by of_get_parent();
> > >
> > > OK, I get that, but I'm still not convinced about if something like
> > > this function is needed at all. I also have concerns regarding how
> > > you are using the function. I'll explain in comment to the patch 5/6 in this
> series...
> >
> > FYI, Currently  I am also using this api in my patch series.
> > https://patchwork.kernel.org/patch/10944637/
> 
> Yes, and I have the same question for you I jusb asked in comment I added
> to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?

Currently no issue. It will work with this api as well, since the port node is part of controller node.
For eg:-
https://patchwork.kernel.org/patch/10944627/

However if any one adds port node inside the connector node, then this api may won't work as expected.
Currently I get below error

[    2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47

For eg:-

	hd3ss3220@47 {
		compatible = "ti,hd3ss3220";
		...
		....
		usb_con: connector {
                                     ....
                                     ....
			port {
				hd3ss3220_ep: endpoint@0 {
					reg = <0>;
					remote-endpoint = <&usb3peri_role_switch>;
				};
			};
		};
	};

Regards,
Biju

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi,
On Mon, 2019-05-20 at 09:45 +0000, Biju Das wrote:
> 
> Hi Heikki,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > Hi,
> > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > usb_role_switch.
> > > > > > > >
> > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > >
> > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > >
> > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > points out that you don't need to use device graph since the
> > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > don't really need this API?
> > > > > No, I still need it.
> > > > > The change is about the way how to get fwnode; when use device
> > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > fwnode by of_get_parent();
> > > >
> > > > OK, I get that, but I'm still not convinced about if something like
> > > > this function is needed at all. I also have concerns regarding how
> > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > series...
> > >
> > > FYI, Currently  I am also using this api in my patch series.
> > > https://patchwork.kernel.org/patch/10944637/
> > 
> > Yes, and I have the same question for you I jusb asked in comment I added
> > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> 
> Currently no issue. It will work with this api as well, since the port node is part of controller node.
> For eg:-
> https://patchwork.kernel.org/patch/10944627/
> 
> However if any one adds port node inside the connector node, then this api may won't work as expected.
> Currently I get below error
> 
> [    2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47
> 
> For eg:-
> 
> 	hd3ss3220@47 {
> 		compatible = "ti,hd3ss3220";
> 		...
> 		....
> 		usb_con: connector {
>                                      ....
>                                      ....
> 			port {
> 				hd3ss3220_ep: endpoint@0 {
> 					reg = <0>;
> 					remote-endpoint = <&usb3peri_role_switch>;
> 				};
> 			};
> 		};
> 	};
> 
> Regards,
> Biju

I tested 3 cases:

case 1:

connector {
    compatible = "linux,typeb-conn-gpio", "usb-b-connector";
    label = "micro-USB";
    type = "micro";
    id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
    vbus-supply = <&usb_p0_vbus>;

    port {
        bconn_ep: endpoint@0 {
            remote-endpoint = <&usb_role_sw>;
        };
    };
};

&mtu3 {
    usb-role-switch;

    port {
        usb_role_sw: endpoint@0 {
            remote-endpoint = <&bconn_ep>;
        };
    };
};

the driver of connector could use usb_role_switch_get(dev) to get
mtu3's USB Role Switch. (dev is the device of connector)

case 2:

&mtu3 {
    usb-role-switch;

    connector {
        compatible = "linux,typeb-conn-gpio", "usb-b-connector";
        label = "micro-USB";
        type = "micro";
        id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
        vbus-supply = <&usb_p0_vbus>;
    };
};

the driver of connector using usb_role_switch_get(dev) failed to get
mtu3's USB Role Switch.
error log:
#OF: graph: no port node found in /usb@11271000/connector
this is because connector hasn't child node connected to remote
endpoint which register USB Role Switch

case 3:

rsw_iddig: role_sw_iddig {
    compatible = "linux,typeb-conn-gpio";
    status = "okay";

    connector {
        compatible = "usb-b-connector";
        label = "micro-USB";
        type = "micro";
        id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
        vbus-supply = <&usb_p0_vbus>;

        port {
            bconn_ep: endpoint@0 {
                remote-endpoint = <&usb_role_sw>;
            };
        };
    };
};

&mtu3 {
    usb-role-switch;

    port {
        usb_role_sw: endpoint@0 {
            remote-endpoint = <&bconn_ep>;
        };
    };
};


the driver of connector using usb_role_switch_get(dev) also failed to
get mtu3's USB Role Switch. Because usb_role_switch_get() only search
its child nodes (connector node), but not child's child (port node)
This case is the same as Biju's

Usually type-c is similar with case 3;
the next version v6 of this series will use case 2 as Rob suggested,
see [v5, 2/6]

for case 2, will need the new API fwnode_usb_role_switch_get();
for case 3, use the new API, or need modify usb_role_switch_get();



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/6] usb: roles: add USB Type-B GPIO connector driver

[Chunfeng Yun]

Hi,

On Mon, 2019-05-20 at 11:31 +0300, Heikki Krogerus wrote:
> On Tue, May 14, 2019 at 04:47:22PM +0800, Chunfeng Yun wrote:
> > +static int usb_conn_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct device_node *remote_node;
> > +	struct usb_conn_info *info;
> > +	int ret = 0;
> > +
> > +	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);
> > +	}
> > +
> > +	remote_node = of_graph_get_remote_node(node, -1, 0);
> 
> This is really not ideal. In practice this code will only work if
> there is only one endpoint described for this device, or if the first
> endpoint is always the one we are looking for. There is no way to
> guarantee that.
Yes, it is.
I'll modify it as case 2, see reply [v5, 4/6] in this series.

> 
> The code really has to walk through the entire graph, and identify the
> remote endpoint it's looking for (and for that we have the boolean
> device property).
> 
> > +	if (!remote_node) {
> > +		dev_err(dev, "failed to get remote node\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	info->role_sw =
> > +		fwnode_usb_role_switch_get(of_fwnode_handle(remote_node));
> 
> So fwnode_usb_role_switch_get() needs be the one that walks through
> the graph, not the drivers. Otherwise every driver will do the same
> exact steps (boilerplate). Here you need to be able to just pass the
> node of this device, not the remote endpoint:
> 
>         info->role_sw = fwnode_usb_role_switch_get(dev_fwnode(&client->dev));
> 
> But why do you need that function at all? Why wouldn't
> usb_role_switch_get() work?
> 
>         info->role_sw = usb_role_switch_get(&client->dev);
> 
see reply [v5, 4/6] in this series

Thanks a lot.

> > +	of_node_put(remote_node);
> > +	if (IS_ERR(info->role_sw)) {
> > +		dev_err(dev, "failed to get role switch\n");
> > +		return PTR_ERR(info->role_sw);
> > +	}
> > +
> > +	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");
> > +			ret = info->id_irq;
> > +			goto put_role_sw;
> > +		}
> > +
> > +		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");
> > +			goto put_role_sw;
> > +		}
> > +	}
> > +
> > +	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");
> > +			ret = info->vbus_irq;
> > +			goto put_role_sw;
> > +		}
> > +
> > +		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");
> > +			goto put_role_sw;
> > +		}
> > +	}
> > +
> > +	platform_set_drvdata(pdev, info);
> > +
> > +	/* Perform initial detection */
> > +	usb_conn_queue_dwork(info, 0);
> > +
> > +	return 0;
> > +
> > +put_role_sw:
> > +	usb_role_switch_put(info->role_sw);
> > +	return ret;
> > +}
> 
> thanks,
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> 
> 
> Hi Heikki,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > Hi,
> > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > usb_role_switch.
> > > > > > > >
> > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > >
> > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > >
> > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > points out that you don't need to use device graph since the
> > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > don't really need this API?
> > > > > No, I still need it.
> > > > > The change is about the way how to get fwnode; when use device
> > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > fwnode by of_get_parent();
> > > >
> > > > OK, I get that, but I'm still not convinced about if something like
> > > > this function is needed at all. I also have concerns regarding how
> > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > series...
> > >
> > > FYI, Currently  I am also using this api in my patch series.
> > > https://patchwork.kernel.org/patch/10944637/
> > 
> > Yes, and I have the same question for you I jusb asked in comment I added
> > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> 
> Currently no issue. It will work with this api as well, since the port node is part of controller node.
> For eg:-
> https://patchwork.kernel.org/patch/10944627/
> 
> However if any one adds port node inside the connector node, then this api may won't work as expected.
> Currently I get below error
> 
> [    2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47

We need to understand why is that happening?

It looks like we have an issue somewhere in the code, and instead of
fixing that, you are working around it. Let's not do that.

> For eg:-
> 
> 	hd3ss3220@47 {
> 		compatible = "ti,hd3ss3220";
> 		...
> 		....
> 		usb_con: connector {
>                                      ....
>                                      ....
> 			port {
> 				hd3ss3220_ep: endpoint@0 {
> 					reg = <0>;
> 					remote-endpoint = <&usb3peri_role_switch>;
> 				};
> 			};
> 		};
> 	};
> 
> Regards,
> Biju

thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Tue, May 21, 2019 at 03:35:04PM +0800, Chunfeng Yun wrote:
> Hi,
> On Mon, 2019-05-20 at 09:45 +0000, Biju Das wrote:
> > 
> > Hi Heikki,
> > 
> > Thanks for the feedback.
> > 
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > node
> > > 
> > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > Hi,
> > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > > usb_role_switch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > >
> > > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > > >
> > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > points out that you don't need to use device graph since the
> > > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > > don't really need this API?
> > > > > > No, I still need it.
> > > > > > The change is about the way how to get fwnode; when use device
> > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > > fwnode by of_get_parent();
> > > > >
> > > > > OK, I get that, but I'm still not convinced about if something like
> > > > > this function is needed at all. I also have concerns regarding how
> > > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > > series...
> > > >
> > > > FYI, Currently  I am also using this api in my patch series.
> > > > https://patchwork.kernel.org/patch/10944637/
> > > 
> > > Yes, and I have the same question for you I jusb asked in comment I added
> > > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> > 
> > Currently no issue. It will work with this api as well, since the port node is part of controller node.
> > For eg:-
> > https://patchwork.kernel.org/patch/10944627/
> > 
> > However if any one adds port node inside the connector node, then this api may won't work as expected.
> > Currently I get below error
> > 
> > [    2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47
> > 
> > For eg:-
> > 
> > 	hd3ss3220@47 {
> > 		compatible = "ti,hd3ss3220";
> > 		...
> > 		....
> > 		usb_con: connector {
> >                                      ....
> >                                      ....
> > 			port {
> > 				hd3ss3220_ep: endpoint@0 {
> > 					reg = <0>;
> > 					remote-endpoint = <&usb3peri_role_switch>;
> > 				};
> > 			};
> > 		};
> > 	};
> > 
> > Regards,
> > Biju
> 
> I tested 3 cases:
> 
> case 1:
> 
> connector {
>     compatible = "linux,typeb-conn-gpio", "usb-b-connector";
>     label = "micro-USB";
>     type = "micro";
>     id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
>     vbus-supply = <&usb_p0_vbus>;
> 
>     port {
>         bconn_ep: endpoint@0 {
>             remote-endpoint = <&usb_role_sw>;
>         };
>     };
> };
> 
> &mtu3 {
>     usb-role-switch;
> 
>     port {
>         usb_role_sw: endpoint@0 {
>             remote-endpoint = <&bconn_ep>;
>         };
>     };
> };
> 
> the driver of connector could use usb_role_switch_get(dev) to get
> mtu3's USB Role Switch. (dev is the device of connector)
> 
> case 2:
> 
> &mtu3 {
>     usb-role-switch;
> 
>     connector {
>         compatible = "linux,typeb-conn-gpio", "usb-b-connector";
>         label = "micro-USB";
>         type = "micro";
>         id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
>         vbus-supply = <&usb_p0_vbus>;
>     };
> };
> 
> the driver of connector using usb_role_switch_get(dev) failed to get
> mtu3's USB Role Switch.
> error log:
> #OF: graph: no port node found in /usb@11271000/connector
> this is because connector hasn't child node connected to remote
> endpoint which register USB Role Switch
> 
> case 3:
> 
> rsw_iddig: role_sw_iddig {
>     compatible = "linux,typeb-conn-gpio";
>     status = "okay";
> 
>     connector {
>         compatible = "usb-b-connector";
>         label = "micro-USB";
>         type = "micro";
>         id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
>         vbus-supply = <&usb_p0_vbus>;
> 
>         port {
>             bconn_ep: endpoint@0 {
>                 remote-endpoint = <&usb_role_sw>;
>             };
>         };
>     };
> };
> 
> &mtu3 {
>     usb-role-switch;
> 
>     port {
>         usb_role_sw: endpoint@0 {
>             remote-endpoint = <&bconn_ep>;
>         };
>     };
> };
> 
> 
> the driver of connector using usb_role_switch_get(dev) also failed to
> get mtu3's USB Role Switch. Because usb_role_switch_get() only search
> its child nodes (connector node), but not child's child (port node)
> This case is the same as Biju's
> 
> Usually type-c is similar with case 3;
> the next version v6 of this series will use case 2 as Rob suggested,
> see [v5, 2/6]
> 
> for case 2, will need the new API fwnode_usb_role_switch_get();

Thanks for the explanation.

In this case, if I understood this correctly, the USB controller, which
is also the role switch, is the parent of the connector. So shouldn't
we simply consider that in the current API?

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..2f898167b99a 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -125,6 +125,13 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 {
        struct usb_role_switch *sw;

+       /*
+        * Simplest case is that a connector is looking for the controller,
+        * which is its parent.
+        */
+       if (device_property_present(dev->parent, "usb-role-switch"))
+               return to_role_switch(dev->parent);
+
        sw = device_connection_find_match(dev, "usb-role-switch", NULL,
                                          usb_role_switch_match);


> for case 3, use the new API, or need modify usb_role_switch_get();

I did not completely understand this case, but isn't it the same as
case 2 in the end, after you change it as Rob suggested?


thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

On Tue, 2019-05-21 at 13:33 +0300, Heikki Krogerus wrote:
> On Tue, May 21, 2019 at 03:35:04PM +0800, Chunfeng Yun wrote:
> > Hi,
> > On Mon, 2019-05-20 at 09:45 +0000, Biju Das wrote:
> > > 
> > > Hi Heikki,
> > > 
> > > Thanks for the feedback.
> > > 
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > > node
> > > > 
> > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > Hi,
> > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun wrote:
> > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > > It's useful when there is not device_connection registered
> > > > > > > > > > between two drivers and only knows the fwnode which register
> > > > > > > > > > usb_role_switch.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > >
> > > > > > > > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > > > >
> > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > > points out that you don't need to use device graph since the
> > > > > > > > controller is the parent of the connector. Doesn't that mean you
> > > > > > > > don't really need this API?
> > > > > > > No, I still need it.
> > > > > > > The change is about the way how to get fwnode; when use device
> > > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will get
> > > > > > > fwnode by of_get_parent();
> > > > > >
> > > > > > OK, I get that, but I'm still not convinced about if something like
> > > > > > this function is needed at all. I also have concerns regarding how
> > > > > > you are using the function. I'll explain in comment to the patch 5/6 in this
> > > > series...
> > > > >
> > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > https://patchwork.kernel.org/patch/10944637/
> > > > 
> > > > Yes, and I have the same question for you I jusb asked in comment I added
> > > > to the patch 5/6 of this series. Why isn't usb_role_switch_get() enough?
> > > 
> > > Currently no issue. It will work with this api as well, since the port node is part of controller node.
> > > For eg:-
> > > https://patchwork.kernel.org/patch/10944627/
> > > 
> > > However if any one adds port node inside the connector node, then this api may won't work as expected.
> > > Currently I get below error
> > > 
> > > [    2.299703] OF: graph: no port node found in /soc/i2c@e6500000/hd3ss3220@47
> > > 
> > > For eg:-
> > > 
> > > 	hd3ss3220@47 {
> > > 		compatible = "ti,hd3ss3220";
> > > 		...
> > > 		....
> > > 		usb_con: connector {
> > >                                      ....
> > >                                      ....
> > > 			port {
> > > 				hd3ss3220_ep: endpoint@0 {
> > > 					reg = <0>;
> > > 					remote-endpoint = <&usb3peri_role_switch>;
> > > 				};
> > > 			};
> > > 		};
> > > 	};
> > > 
> > > Regards,
> > > Biju
> > 
> > I tested 3 cases:
> > 
> > case 1:
> > 
> > connector {
> >     compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> >     label = "micro-USB";
> >     type = "micro";
> >     id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> >     vbus-supply = <&usb_p0_vbus>;
> > 
> >     port {
> >         bconn_ep: endpoint@0 {
> >             remote-endpoint = <&usb_role_sw>;
> >         };
> >     };
> > };
> > 
> > &mtu3 {
> >     usb-role-switch;
> > 
> >     port {
> >         usb_role_sw: endpoint@0 {
> >             remote-endpoint = <&bconn_ep>;
> >         };
> >     };
> > };
> > 
> > the driver of connector could use usb_role_switch_get(dev) to get
> > mtu3's USB Role Switch. (dev is the device of connector)
> > 
> > case 2:
> > 
> > &mtu3 {
> >     usb-role-switch;
> > 
> >     connector {
> >         compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> >         label = "micro-USB";
> >         type = "micro";
> >         id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> >         vbus-supply = <&usb_p0_vbus>;
> >     };
> > };
> > 
> > the driver of connector using usb_role_switch_get(dev) failed to get
> > mtu3's USB Role Switch.
> > error log:
> > #OF: graph: no port node found in /usb@11271000/connector
> > this is because connector hasn't child node connected to remote
> > endpoint which register USB Role Switch
> > 
> > case 3:
> > 
> > rsw_iddig: role_sw_iddig {
> >     compatible = "linux,typeb-conn-gpio";
> >     status = "okay";
> > 
> >     connector {
> >         compatible = "usb-b-connector";
> >         label = "micro-USB";
> >         type = "micro";
> >         id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> >         vbus-supply = <&usb_p0_vbus>;
> > 
> >         port {
> >             bconn_ep: endpoint@0 {
> >                 remote-endpoint = <&usb_role_sw>;
> >             };
> >         };
> >     };
> > };
> > 
> > &mtu3 {
> >     usb-role-switch;
> > 
> >     port {
> >         usb_role_sw: endpoint@0 {
> >             remote-endpoint = <&bconn_ep>;
> >         };
> >     };
> > };
> > 
> > 
> > the driver of connector using usb_role_switch_get(dev) also failed to
> > get mtu3's USB Role Switch. Because usb_role_switch_get() only search
> > its child nodes (connector node), but not child's child (port node)
> > This case is the same as Biju's
> > 
> > Usually type-c is similar with case 3;
> > the next version v6 of this series will use case 2 as Rob suggested,
> > see [v5, 2/6]
> > 
> > for case 2, will need the new API fwnode_usb_role_switch_get();
> 
> Thanks for the explanation.
> 
> In this case, if I understood this correctly, the USB controller, which
> is also the role switch, is the parent of the connector. So shouldn't
> we simply consider that in the current API?
It's better if can be added into the current API.
I'll try it.
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f45d8df5cfb8..2f898167b99a 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -125,6 +125,13 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  {
>         struct usb_role_switch *sw;
> 
> +       /*
> +        * Simplest case is that a connector is looking for the controller,
> +        * which is its parent.
> +        */
> +       if (device_property_present(dev->parent, "usb-role-switch"))
> +               return to_role_switch(dev->parent);
> +
>         sw = device_connection_find_match(dev, "usb-role-switch", NULL,
>                                           usb_role_switch_match);
> 
> 
> > for case 3, use the new API, or need modify usb_role_switch_get();
> 
> I did not completely understand this case, but isn't it the same as
> case 2 in the end, after you change it as Rob suggested?
I'm afraid not, their bindings are different, see
connector/usb-connector.txt
> 
> 
> thanks,
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Biju Das]

Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
> 
> On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> >
> >
> > Hi Heikki,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > Hi,
> > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> wrote:
> > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > registered between two drivers and only knows the fwnode
> > > > > > > > > which register usb_role_switch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > >
> > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > >
> > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > points out that you don't need to use device graph since the
> > > > > > > controller is the parent of the connector. Doesn't that mean
> > > > > > > you don't really need this API?
> > > > > > No, I still need it.
> > > > > > The change is about the way how to get fwnode; when use device
> > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will
> > > > > > get fwnode by of_get_parent();
> > > > >
> > > > > OK, I get that, but I'm still not convinced about if something
> > > > > like this function is needed at all. I also have concerns
> > > > > regarding how you are using the function. I'll explain in
> > > > > comment to the patch 5/6 in this
> > > series...
> > > >
> > > > FYI, Currently  I am also using this api in my patch series.
> > > > https://patchwork.kernel.org/patch/10944637/
> > >
> > > Yes, and I have the same question for you I jusb asked in comment I
> > > added to the patch 5/6 of this series. Why isn't usb_role_switch_get()
> enough?
> >
> > Currently no issue. It will work with this api as well, since the port node is
> part of controller node.
> > For eg:-
> > https://patchwork.kernel.org/patch/10944627/
> >
> > However if any one adds port node inside the connector node, then this
> api may won't work as expected.
> > Currently I get below error
> >
> > [    2.299703] OF: graph: no port node found in
> /soc/i2c@e6500000/hd3ss3220@47
> 
> We need to understand why is that happening?
> 

Form the stack trace  the parent node is "parent_node=hd3ss3220@47" , instead of the "connector" node.
That is the reason for the above error.

[    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
[    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
[    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
[    2.458374]  device_connection_find_match+0x74/0x1a0
[    2.463399]  usb_role_switch_get+0x20/0x28
[    2.467542]  hd3ss3220_probe+0xc4/0x218

The use case is

&i2c0 {
	hd3ss3220@47 {                                                           
                 	compatible = "ti,hd3ss3220"; 
                                   
                 	usb_con: connector {                                             
                          		compatible = "usb-c-connector";                                                      
                         		port {                                                   
                                		 hd3ss3220_ep: endpoint {                         
                                        			remote-endpoint = <&usb3_role_switch>;   
                                		};                                               
                         		};                                                       
                	 };                                                               
	 }; 
};
   
&usb3_peri0 {                                                                    
         companion = <&xhci0>;                                                    
         usb-role-switch;                                                         
                                                                                  
         port {                                                                   
                usb3_role_switch: endpoint {                                     
                        remote-endpoint = <&hd3ss3220_ep>;                       
                 };                                                               
         };                                                                       
};   

Q1) How do we modify the usb_role_switch_get() function to search 
Child(connector) and child's endpoint?

> It looks like we have an issue somewhere in the code, and instead of fixing
> that, you are working around it. Let's not do that.

OK.

Regards,
Biju


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi Biju,
On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> Hi Heikki,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > >
> > >
> > > Hi Heikki,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > Hi,
> > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus wrote:
> > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > wrote:
> > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to get
> > > > > > > > > > usb_role_switch by fwnode which register it.
> > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > registered between two drivers and only knows the fwnode
> > > > > > > > > > which register usb_role_switch.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > >
> > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > >
> > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6, where he
> > > > > > > > points out that you don't need to use device graph since the
> > > > > > > > controller is the parent of the connector. Doesn't that mean
> > > > > > > > you don't really need this API?
> > > > > > > No, I still need it.
> > > > > > > The change is about the way how to get fwnode; when use device
> > > > > > > graph, get fwnode by of_graph_get_remote_node(); but now will
> > > > > > > get fwnode by of_get_parent();
> > > > > >
> > > > > > OK, I get that, but I'm still not convinced about if something
> > > > > > like this function is needed at all. I also have concerns
> > > > > > regarding how you are using the function. I'll explain in
> > > > > > comment to the patch 5/6 in this
> > > > series...
> > > > >
> > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > https://patchwork.kernel.org/patch/10944637/
> > > >
> > > > Yes, and I have the same question for you I jusb asked in comment I
> > > > added to the patch 5/6 of this series. Why isn't usb_role_switch_get()
> > enough?
> > >
> > > Currently no issue. It will work with this api as well, since the port node is
> > part of controller node.
> > > For eg:-
> > > https://patchwork.kernel.org/patch/10944627/
> > >
> > > However if any one adds port node inside the connector node, then this
> > api may won't work as expected.
> > > Currently I get below error
> > >
> > > [    2.299703] OF: graph: no port node found in
> > /soc/i2c@e6500000/hd3ss3220@47
> > 
> > We need to understand why is that happening?
> > 
> 
> Form the stack trace  the parent node is "parent_node=hd3ss3220@47" , instead of the "connector" node.
> That is the reason for the above error.
> 
> [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> [    2.458374]  device_connection_find_match+0x74/0x1a0
> [    2.463399]  usb_role_switch_get+0x20/0x28
> [    2.467542]  hd3ss3220_probe+0xc4/0x218
> 
> The use case is
> 
> &i2c0 {
> 	hd3ss3220@47 {                                                           
>                  	compatible = "ti,hd3ss3220"; 
>                                    
>                  	usb_con: connector {                                             
>                           		compatible = "usb-c-connector";                                                      
>                          		port {                                                   
>                                 		 hd3ss3220_ep: endpoint {                         
>                                         			remote-endpoint = <&usb3_role_switch>;   
>                                 		};                                               
>                          		};                                                       
>                 	 };                                                               
> 	 }; 
> };
>    
> &usb3_peri0 {                                                                    
>          companion = <&xhci0>;                                                    
>          usb-role-switch;                                                         
>                                                                                   
>          port {                                                                   
>                 usb3_role_switch: endpoint {                                     
>                         remote-endpoint = <&hd3ss3220_ep>;                       
>                  };                                                               
>          };                                                                       
> };   
> 
> Q1) How do we modify the usb_role_switch_get() function to search 
> Child(connector) and child's endpoint?
How about firstly finding connector node in fwnode_graph_devcon_match(),
then search each endpoint?

> 
> > It looks like we have an issue somewhere in the code, and instead of fixing
> > that, you are working around it. Let's not do that.
> 
> OK.
> 
> Regards,
> Biju
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Biju Das]

Hi Chunfeng Yun,

Thanks for the feedback.

> Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
> 
> Hi Biju,
> On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > Hi Heikki,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > >
> > > >
> > > > Hi Heikki,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > Hi,
> > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> wrote:
> > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > wrote:
> > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > >
> > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > >
> > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > No, I still need it.
> > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > >
> > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > something like this function is needed at all. I also have
> > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > explain in comment to the patch 5/6 in this
> > > > > series...
> > > > > >
> > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > >
> > > > > Yes, and I have the same question for you I jusb asked in
> > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > usb_role_switch_get()
> > > enough?
> > > >
> > > > Currently no issue. It will work with this api as well, since the
> > > > port node is
> > > part of controller node.
> > > > For eg:-
> > > > https://patchwork.kernel.org/patch/10944627/
> > > >
> > > > However if any one adds port node inside the connector node, then
> > > > this
> > > api may won't work as expected.
> > > > Currently I get below error
> > > >
> > > > [    2.299703] OF: graph: no port node found in
> > > /soc/i2c@e6500000/hd3ss3220@47
> > >
> > > We need to understand why is that happening?
> > >
> >
> > Form the stack trace  the parent node is "parent_node=hd3ss3220@47" ,
> instead of the "connector" node.
> > That is the reason for the above error.
> >
> > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > [    2.463399]  usb_role_switch_get+0x20/0x28
> > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> >
> > The use case is
> >
> > &i2c0 {
> > 	hd3ss3220@47 {
> >                  	compatible = "ti,hd3ss3220";
> >
> >                  	usb_con: connector {
> >                           		compatible = "usb-c-connector";
> >                          		port {
> >                                 		 hd3ss3220_ep: endpoint {
> >                                         			remote-endpoint =
> <&usb3_role_switch>;
> >                                 		};
> >                          		};
> >                 	 };
> > 	 };
> > };
> >
> > &usb3_peri0 {
> >          companion = <&xhci0>;
> >          usb-role-switch;
> >
> >          port {
> >                 usb3_role_switch: endpoint {
> >                         remote-endpoint = <&hd3ss3220_ep>;
> >                  };
> >          };
> > };
> >
> > Q1) How do we modify the usb_role_switch_get() function to search
> > Child(connector) and child's endpoint?
> How about firstly finding connector node in fwnode_graph_devcon_match(),
> then search each endpoint?

 I have done a quick prototyping with the changes you suggested and it works.

-       struct fwnode_handle *ep;
+       struct fwnode_handle *ep,*child,*tmp = fwnode; 
 
-       fwnode_graph_for_each_endpoint(fwnode, ep) {
+       child = fwnode_get_named_child_node(fwnode, "connector");
+       if (child)
+               tmp = child;
+
+       fwnode_graph_for_each_endpoint(tmp, ep) {

Form the stack trace  the parent node is "parent_node= connector" .

[    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
[    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
[    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
[    2.456866]  device_connection_find_match+0x84/0x1c0
[    2.461888]  usb_role_switch_get+0x20/0x28

Heikki, 
Are you ok  with the above changes?

Regards,
Biju
> >
> > > It looks like we have an issue somewhere in the code, and instead of
> > > fixing that, you are working around it. Let's not do that.
> >
> > OK.
> >
> > Regards,
> > Biju
> >
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

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

On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> Hi Chunfeng Yun,
> 
> Thanks for the feedback.
> 
> > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > Hi Biju,
> > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > >
> > > > >
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > Hi,
> > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > wrote:
> > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > wrote:
> > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > >
> > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > >
> > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > No, I still need it.
> > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > >
> > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > something like this function is needed at all. I also have
> > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > series...
> > > > > > >
> > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > >
> > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > usb_role_switch_get()
> > > > enough?
> > > > >
> > > > > Currently no issue. It will work with this api as well, since the
> > > > > port node is
> > > > part of controller node.
> > > > > For eg:-
> > > > > https://patchwork.kernel.org/patch/10944627/
> > > > >
> > > > > However if any one adds port node inside the connector node, then
> > > > > this
> > > > api may won't work as expected.
> > > > > Currently I get below error
> > > > >
> > > > > [    2.299703] OF: graph: no port node found in
> > > > /soc/i2c@e6500000/hd3ss3220@47
> > > >
> > > > We need to understand why is that happening?
> > > >
> > >
> > > Form the stack trace  the parent node is "parent_node=hd3ss3220@47" ,
> > instead of the "connector" node.
> > > That is the reason for the above error.
> > >
> > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > >
> > > The use case is
> > >
> > > &i2c0 {
> > > 	hd3ss3220@47 {
> > >                  	compatible = "ti,hd3ss3220";
> > >
> > >                  	usb_con: connector {
> > >                           		compatible = "usb-c-connector";
> > >                          		port {
> > >                                 		 hd3ss3220_ep: endpoint {
> > >                                         			remote-endpoint =
> > <&usb3_role_switch>;
> > >                                 		};
> > >                          		};
> > >                 	 };
> > > 	 };
> > > };
> > >
> > > &usb3_peri0 {
> > >          companion = <&xhci0>;
> > >          usb-role-switch;
> > >
> > >          port {
> > >                 usb3_role_switch: endpoint {
> > >                         remote-endpoint = <&hd3ss3220_ep>;
> > >                  };
> > >          };
> > > };
> > >
> > > Q1) How do we modify the usb_role_switch_get() function to search
> > > Child(connector) and child's endpoint?
> > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > then search each endpoint?
> 
>  I have done a quick prototyping with the changes you suggested and it works.
> 
> -       struct fwnode_handle *ep;
> +       struct fwnode_handle *ep,*child,*tmp = fwnode; 
>  
> -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> +       child = fwnode_get_named_child_node(fwnode, "connector");
> +       if (child)
> +               tmp = child;
> +
> +       fwnode_graph_for_each_endpoint(tmp, ep) {
> 
> Form the stack trace  the parent node is "parent_node= connector" .
> 
> [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> [    2.456866]  device_connection_find_match+0x84/0x1c0
> [    2.461888]  usb_role_switch_get+0x20/0x28
> 
> Heikki, 
> Are you ok  with the above changes?

Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
I proposed, there is no problem? You just find the "connector" child
node in your driver, and pass that to fwnode_usb_role_switch_get():

        struct fwnode_handle *connector;
        ...
        connector = device_get_named_child_node(&client->dev, "connector");
        if (IS_ERR(connector))
                <do something>

        hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
        ...

The difference is that instead of just converting a device node of an
usb role switch to the usb role switch, it works just like
usb_role_switch_get(), just taking fwnode instead of device entry as
parameter.

I prepared the patches implementing fwnode_usb_role_switch_get() the
way I though it needs to work for my own tests. Please find the
patches attached.

thanks,

-- 
heikki

[-- Attachment #2: 0001-device-connection-Add-fwnode_connection_find_match.patch --]
[-- Type: text/plain, Size: 3386 bytes --]

From 18a15ef54f2d88b04ce851d9428cda6697a66fb2 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 22 May 2019 16:43:41 +0300
Subject: [PATCH 1/2] device connection: Add fwnode_connection_find_match()

The fwnode_connection_find_match() function is exactly the
same as device_connection_find_match(), except it takes
struct fwnode_handle as parameter instead of struct device.
That allows locating device connections before the device
entries have been created.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/devcon.c  | 33 +++++++++++++++++++++++++--------
 include/linux/device.h | 10 +++++++---
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 04db9ae235e4..8311b70bbca2 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -12,9 +12,6 @@
 static DEFINE_MUTEX(devcon_lock);
 static LIST_HEAD(devcon_list);
 
-typedef void *(*devcon_match_fn_t)(struct device_connection *con, int ep,
-				   void *data);
-
 static void *
 fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 			  void *data, devcon_match_fn_t match)
@@ -38,6 +35,28 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 	return NULL;
 }
 
+/**
+ * fwnode_connection_find_match - Find connection from a device node
+ * @fwnode: Device node with the connection
+ * @con_id: Identifier for the connection
+ * @data: Data for the match function
+ * @match: Function to check and convert the connection description
+ *
+ * Find a connection with unique identifier @con_id between @fwnode and another
+ * device node. @match will be used to convert the connection description to
+ * data the caller is expecting to be returned.
+ */
+void *fwnode_connection_find_match(struct fwnode_handle *fwnode,
+				   const char *con_id, void *data,
+				   devcon_match_fn_t match)
+{
+	if (!fwnode || !match)
+		return NULL;
+
+	return fwnode_graph_devcon_match(fwnode, con_id, data, match);
+}
+EXPORT_SYMBOL_GPL(fwnode_connection_find_match);
+
 /**
  * device_connection_find_match - Find physical connection to a device
  * @dev: Device with the connection
@@ -61,11 +80,9 @@ void *device_connection_find_match(struct device *dev, const char *con_id,
 	if (!match)
 		return NULL;
 
-	if (fwnode) {
-		ret = fwnode_graph_devcon_match(fwnode, con_id, data, match);
-		if (ret)
-			return ret;
-	}
+	ret = fwnode_connection_find_match(fwnode, con_id, data, match);
+	if (ret)
+		return ret;
 
 	mutex_lock(&devcon_lock);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 72a6260f2b4d..04ca902d24dc 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -772,10 +772,14 @@ struct device_connection {
 	struct list_head	list;
 };
 
+typedef void *(*devcon_match_fn_t)(struct device_connection *con, int ep,
+				   void *data);
+
+void *fwnode_connection_find_match(struct fwnode_handle *fwnode,
+				   const char *con_id, void *data,
+				   devcon_match_fn_t match);
 void *device_connection_find_match(struct device *dev, const char *con_id,
-				void *data,
-				void *(*match)(struct device_connection *con,
-					       int ep, void *data));
+				   void *data, devcon_match_fn_t match);
 
 struct device *device_connection_find(struct device *dev, const char *con_id);
 
-- 
2.20.1


[-- Attachment #3: 0002-usb-roles-Add-fwnode_usb_role_switch_get-function.patch --]
[-- Type: text/plain, Size: 2227 bytes --]

From 9eaa446c507780a6f9d0a1cc12c04e87a3bafcf3 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 22 May 2019 16:43:41 +0300
Subject: [PATCH 2/2] usb: roles: Add fwnode_usb_role_switch_get() function

The fwnode_usb_role_switch_get() function is exactly the
same as usb_role_switch_get(), except that it takes struct
fwnode_handle as parameter instead of struct device.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/roles/class.c | 20 ++++++++++++++++++++
 include/linux/usb/role.h  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index f45d8df5cfb8..aab795b54c7f 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -135,6 +135,26 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get);
 
+/**
+ * fwnode_usb_role_switch_get - Find USB role switch linked with the caller
+ * @fwnode: The caller device node
+ *
+ * This is similar to the usb_role_switch_get() function above, but it searches
+ * the switch using fwnode instead of device entry.
+ */
+struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
+{
+	struct usb_role_switch *sw;
+
+	sw = fwnode_connection_find_match(fwnode, "usb-role-switch", NULL,
+					  usb_role_switch_match);
+	if (!IS_ERR_OR_NULL(sw))
+		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+
+	return sw;
+}
+EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
+
 /**
  * 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 c05ffa6abda9..6abb04df255c 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -45,6 +45,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(struct device *dev);
+struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *node);
 void usb_role_switch_put(struct usb_role_switch *sw);
 
 struct usb_role_switch *
-- 
2.20.1


[-- Attachment #4: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Biju Das]

Hi Heikki,

Thanks for the patch

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
> 
> On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > Hi Chunfeng Yun,
> >
> > Thanks for the feedback.
> >
> > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > usb_role_switch by node
> > >
> > > Hi Biju,
> > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > >
> > > > > >
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun
> wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki
> > > > > > > > > > > Krogerus
> > > wrote:
> > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng
> > > > > > > > > > > > Yun
> > > > > wrote:
> > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier
> > > > > > > > > > > > > to get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > registered between two drivers and only knows
> > > > > > > > > > > > > the fwnode which register usb_role_switch.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > >
> > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > where he points out that you don't need to use
> > > > > > > > > > > device graph since the controller is the parent of
> > > > > > > > > > > the connector. Doesn't that mean you don't really need
> this API?
> > > > > > > > > > No, I still need it.
> > > > > > > > > > The change is about the way how to get fwnode; when
> > > > > > > > > > use device graph, get fwnode by
> > > > > > > > > > of_graph_get_remote_node(); but now will get fwnode by
> > > > > > > > > > of_get_parent();
> > > > > > > > >
> > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > something like this function is needed at all. I also
> > > > > > > > > have concerns regarding how you are using the function.
> > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > series...
> > > > > > > >
> > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > >
> > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > usb_role_switch_get()
> > > > > enough?
> > > > > >
> > > > > > Currently no issue. It will work with this api as well, since
> > > > > > the port node is
> > > > > part of controller node.
> > > > > > For eg:-
> > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > >
> > > > > > However if any one adds port node inside the connector node,
> > > > > > then this
> > > > > api may won't work as expected.
> > > > > > Currently I get below error
> > > > > >
> > > > > > [    2.299703] OF: graph: no port node found in
> > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > >
> > > > > We need to understand why is that happening?
> > > > >
> > > >
> > > > Form the stack trace  the parent node is
> > > > "parent_node=hd3ss3220@47" ,
> > > instead of the "connector" node.
> > > > That is the reason for the above error.
> > > >
> > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > >
> > > > The use case is
> > > >
> > > > &i2c0 {
> > > > 	hd3ss3220@47 {
> > > >                  	compatible = "ti,hd3ss3220";
> > > >
> > > >                  	usb_con: connector {
> > > >                           		compatible = "usb-c-connector";
> > > >                          		port {
> > > >                                 		 hd3ss3220_ep: endpoint {
> > > >                                         			remote-endpoint =
> > > <&usb3_role_switch>;
> > > >                                 		};
> > > >                          		};
> > > >                 	 };
> > > > 	 };
> > > > };
> > > >
> > > > &usb3_peri0 {
> > > >          companion = <&xhci0>;
> > > >          usb-role-switch;
> > > >
> > > >          port {
> > > >                 usb3_role_switch: endpoint {
> > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > >                  };
> > > >          };
> > > > };
> > > >
> > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > Child(connector) and child's endpoint?
> > > How about firstly finding connector node in
> > > fwnode_graph_devcon_match(), then search each endpoint?
> >
> >  I have done a quick prototyping with the changes you suggested and it
> works.
> >
> > -       struct fwnode_handle *ep;
> > +       struct fwnode_handle *ep,*child,*tmp = fwnode;
> >
> > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > +       if (child)
> > +               tmp = child;
> > +
> > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> >
> > Form the stack trace  the parent node is "parent_node= connector" .
> >
> > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > [    2.461888]  usb_role_switch_get+0x20/0x28
> >
> > Heikki,
> > Are you ok  with the above changes?
> 
> Doesn't that mean that if we made fwnode_usb_role_switch_get() the way I
> proposed, there is no problem? You just find the "connector" child node in
> your driver, and pass that to fwnode_usb_role_switch_get():

Yes, That is correct.

>         struct fwnode_handle *connector;
>         ...
>         connector = device_get_named_child_node(&client->dev, "connector");
>         if (IS_ERR(connector))
>                 <do something>
> 
>         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
>         ...
> 
> The difference is that instead of just converting a device node of an usb role
> switch to the usb role switch, it works just like usb_role_switch_get(), just
> taking fwnode instead of device entry as parameter.
> 
> I prepared the patches implementing fwnode_usb_role_switch_get() the
> way I though it needs to work for my own tests. Please find the patches
> attached.

I have tested  this patches and conform it works. 
Do you plan to post this patches to ML? 

Regards,
Biju

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

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

Hi Heikki,
On Wed, 2019-05-22 at 17:26 +0300, Heikki Krogerus wrote:
> On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > Hi Chunfeng Yun,
> > 
> > Thanks for the feedback.
> > 
> > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > node
> > > 
> > > Hi Biju,
> > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > Hi Heikki,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > >
> > > > > >
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > > wrote:
> > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > > wrote:
> > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > >
> > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > > No, I still need it.
> > > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > > >
> > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > something like this function is needed at all. I also have
> > > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > > series...
> > > > > > > >
> > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > >
> > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > usb_role_switch_get()
> > > > > enough?
> > > > > >
> > > > > > Currently no issue. It will work with this api as well, since the
> > > > > > port node is
> > > > > part of controller node.
> > > > > > For eg:-
> > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > >
> > > > > > However if any one adds port node inside the connector node, then
> > > > > > this
> > > > > api may won't work as expected.
> > > > > > Currently I get below error
> > > > > >
> > > > > > [    2.299703] OF: graph: no port node found in
> > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > >
> > > > > We need to understand why is that happening?
> > > > >
> > > >
> > > > Form the stack trace  the parent node is "parent_node=hd3ss3220@47" ,
> > > instead of the "connector" node.
> > > > That is the reason for the above error.
> > > >
> > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > >
> > > > The use case is
> > > >
> > > > &i2c0 {
> > > > 	hd3ss3220@47 {
> > > >                  	compatible = "ti,hd3ss3220";
> > > >
> > > >                  	usb_con: connector {
> > > >                           		compatible = "usb-c-connector";
> > > >                          		port {
> > > >                                 		 hd3ss3220_ep: endpoint {
> > > >                                         			remote-endpoint =
> > > <&usb3_role_switch>;
> > > >                                 		};
> > > >                          		};
> > > >                 	 };
> > > > 	 };
> > > > };
> > > >
> > > > &usb3_peri0 {
> > > >          companion = <&xhci0>;
> > > >          usb-role-switch;
> > > >
> > > >          port {
> > > >                 usb3_role_switch: endpoint {
> > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > >                  };
> > > >          };
> > > > };
> > > >
> > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > Child(connector) and child's endpoint?
> > > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > > then search each endpoint?
> > 
> >  I have done a quick prototyping with the changes you suggested and it works.
> > 
> > -       struct fwnode_handle *ep;
> > +       struct fwnode_handle *ep,*child,*tmp = fwnode; 
> >  
> > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > +       if (child)
> > +               tmp = child;
> > +
> > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> > 
> > Form the stack trace  the parent node is "parent_node= connector" .
> > 
> > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > [    2.461888]  usb_role_switch_get+0x20/0x28
> > 
> > Heikki, 
> > Are you ok  with the above changes?
> 
> Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
> I proposed, there is no problem? You just find the "connector" child
> node in your driver, and pass that to fwnode_usb_role_switch_get():
> 
>         struct fwnode_handle *connector;
>         ...
>         connector = device_get_named_child_node(&client->dev, "connector");
>         if (IS_ERR(connector))
>                 <do something>
> 
>         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
>         ...
> 
> The difference is that instead of just converting a device node of an
> usb role switch to the usb role switch, it works just like
> usb_role_switch_get(), just taking fwnode instead of device entry as
> parameter.
> 
> I prepared the patches implementing fwnode_usb_role_switch_get() the
> way I though it needs to work for my own tests. Please find the
> patches attached.
I tested these patches, it didn't work for case as following:

case 2:

&mtu3 {
    usb-role-switch;

    connector {
        compatible = "linux,typeb-conn-gpio", "usb-b-connector";
        label = "micro-USB";
        type = "micro";
        id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
        vbus-supply = <&usb_p0_vbus>;
    };
};

so I consider this case in the function fwnode_graph_devcon_match()
(based on your patches)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 8311b70..1dae8b7 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -18,9 +18,11 @@
 {
        struct device_connection con = { .id = con_id };
        struct fwnode_handle *ep;
+       int ep_count = 0;
        void *ret;
 
        fwnode_graph_for_each_endpoint(fwnode, ep) {
+               ep_count++;
                con.fwnode = fwnode_graph_get_remote_port_parent(ep);
                if (!fwnode_device_is_available(con.fwnode))
                        continue;
@@ -32,6 +34,19 @@
                        return ret;
                }
        }
+
+       /* if the connector has no remote endpoint, check its parent */
+       if (!ep_count) {
+               con.fwnode = fwnode_get_parent(fwnode);
+               if (!con.fwnode)
+                       return NULL;
+
+               ret = match(&con, -1, data);
+               fwnode_handle_put(con.fwnode);
+               if (ret)
+                       return ret;
+       }
+
        return NULL;
 }

see attached patch.

then both usb_role_switch_get(dev) or fwnode_usb_role_switch_get(fwnode)
work well;

but I don't know which way is better when consider this specific case,
put into class.c as you suggested before or put into devcon.c like
above. 

Thanks a lot

> 
> thanks,
> 


[-- Attachment #2: 0001-device-connection-test-only.patch --]
[-- Type: text/x-patch, Size: 1364 bytes --]

From bd291df4d0247fce5c11da746ae0761420054cf7 Mon Sep 17 00:00:00 2001
From: Chunfeng Yun <chunfeng.yun@mediatek.com>
Date: Thu, 23 May 2019 18:04:28 +0800
Subject: [PATCH] device connection: test only

Change-Id: Ie1b4b6304dfd7a89516fa3578aa5a5b1be924212
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/base/devcon.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 8311b70bbca2..1dae8b77562d 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -18,9 +18,11 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 {
 	struct device_connection con = { .id = con_id };
 	struct fwnode_handle *ep;
+	int ep_count = 0;
 	void *ret;
 
 	fwnode_graph_for_each_endpoint(fwnode, ep) {
+		ep_count++;
 		con.fwnode = fwnode_graph_get_remote_port_parent(ep);
 		if (!fwnode_device_is_available(con.fwnode))
 			continue;
@@ -32,6 +34,19 @@ fwnode_graph_devcon_match(struct fwnode_handle *fwnode, const char *con_id,
 			return ret;
 		}
 	}
+
+	/* if the connector has no remote endpoint, check its parent */
+	if (!ep_count) {
+		con.fwnode = fwnode_get_parent(fwnode);
+		if (!con.fwnode)
+			return NULL;
+
+		ret = match(&con, -1, data);
+		fwnode_handle_put(con.fwnode);
+		if (ret)
+			return ret;
+	}
+
 	return NULL;
 }
 
-- 
2.21.0


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Thu, May 23, 2019 at 06:16:10PM +0800, Chunfeng Yun wrote:
> Hi Heikki,
> On Wed, 2019-05-22 at 17:26 +0300, Heikki Krogerus wrote:
> > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > Hi Chunfeng Yun,
> > > 
> > > Thanks for the feedback.
> > > 
> > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > > node
> > > > 
> > > > Hi Biju,
> > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > usb_role_switch by node
> > > > > > > > > >
> > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > > > wrote:
> > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > > > wrote:
> > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > > > No, I still need it.
> > > > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > > > >
> > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > something like this function is needed at all. I also have
> > > > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > > > series...
> > > > > > > > >
> > > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > >
> > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > usb_role_switch_get()
> > > > > > enough?
> > > > > > >
> > > > > > > Currently no issue. It will work with this api as well, since the
> > > > > > > port node is
> > > > > > part of controller node.
> > > > > > > For eg:-
> > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > >
> > > > > > > However if any one adds port node inside the connector node, then
> > > > > > > this
> > > > > > api may won't work as expected.
> > > > > > > Currently I get below error
> > > > > > >
> > > > > > > [    2.299703] OF: graph: no port node found in
> > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > >
> > > > > > We need to understand why is that happening?
> > > > > >
> > > > >
> > > > > Form the stack trace  the parent node is "parent_node=hd3ss3220@47" ,
> > > > instead of the "connector" node.
> > > > > That is the reason for the above error.
> > > > >
> > > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > > >
> > > > > The use case is
> > > > >
> > > > > &i2c0 {
> > > > > 	hd3ss3220@47 {
> > > > >                  	compatible = "ti,hd3ss3220";
> > > > >
> > > > >                  	usb_con: connector {
> > > > >                           		compatible = "usb-c-connector";
> > > > >                          		port {
> > > > >                                 		 hd3ss3220_ep: endpoint {
> > > > >                                         			remote-endpoint =
> > > > <&usb3_role_switch>;
> > > > >                                 		};
> > > > >                          		};
> > > > >                 	 };
> > > > > 	 };
> > > > > };
> > > > >
> > > > > &usb3_peri0 {
> > > > >          companion = <&xhci0>;
> > > > >          usb-role-switch;
> > > > >
> > > > >          port {
> > > > >                 usb3_role_switch: endpoint {
> > > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > > >                  };
> > > > >          };
> > > > > };
> > > > >
> > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > Child(connector) and child's endpoint?
> > > > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > > > then search each endpoint?
> > > 
> > >  I have done a quick prototyping with the changes you suggested and it works.
> > > 
> > > -       struct fwnode_handle *ep;
> > > +       struct fwnode_handle *ep,*child,*tmp = fwnode; 
> > >  
> > > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > > +       if (child)
> > > +               tmp = child;
> > > +
> > > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> > > 
> > > Form the stack trace  the parent node is "parent_node= connector" .
> > > 
> > > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > > [    2.461888]  usb_role_switch_get+0x20/0x28
> > > 
> > > Heikki, 
> > > Are you ok  with the above changes?
> > 
> > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
> > I proposed, there is no problem? You just find the "connector" child
> > node in your driver, and pass that to fwnode_usb_role_switch_get():
> > 
> >         struct fwnode_handle *connector;
> >         ...
> >         connector = device_get_named_child_node(&client->dev, "connector");
> >         if (IS_ERR(connector))
> >                 <do something>
> > 
> >         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> >         ...
> > 
> > The difference is that instead of just converting a device node of an
> > usb role switch to the usb role switch, it works just like
> > usb_role_switch_get(), just taking fwnode instead of device entry as
> > parameter.
> > 
> > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > way I though it needs to work for my own tests. Please find the
> > patches attached.
> I tested these patches, it didn't work for case as following:
> 
> case 2:
> 
> &mtu3 {
>     usb-role-switch;
> 
>     connector {
>         compatible = "linux,typeb-conn-gpio", "usb-b-connector";
>         label = "micro-USB";
>         type = "micro";
>         id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
>         vbus-supply = <&usb_p0_vbus>;
>     };
> };
> 
> so I consider this case in the function fwnode_graph_devcon_match()
> (based on your patches)

IMO that case should be handled in USB role switch API initially, not
in the device connection API. If there is another user for it one day,
then we can move it to device connection API, but not before that.

How about this on top of the two patches I sent:

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index aab795b54c7f..36ac9d181e09 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -114,6 +114,19 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
        return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }

+static struct usb_role_switch *
+usb_role_switch_is_parent(struct fwnode_handle *parent)
+{
+       struct device *dev;
+
+       if (!parent || !fwnode_property_present(parent, "usb-role-switch"))
+               return NULL;
+
+       dev = class_find_device(role_class, NULL, parent, switch_fwnode_match);
+
+       return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
+}
+
 /**
  * usb_role_switch_get - Find USB role switch linked with the caller
  * @dev: The caller device
@@ -125,6 +138,10 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 {
        struct usb_role_switch *sw;

+       sw = usb_role_switch_is_parent(fwnode_get_parent(dev_fwnode(dev)));
+       if (sw)
+               return sw;
+
        sw = device_connection_find_match(dev, "usb-role-switch", NULL,
                                          usb_role_switch_match);

@@ -146,6 +163,10 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
 {
        struct usb_role_switch *sw;

+       sw = usb_role_switch_is_parent(fwnode_get_parent(fwnode));
+       if (sw)
+               return sw;
+
        sw = fwnode_connection_find_match(fwnode, "usb-role-switch", NULL,
                                          usb_role_switch_match);
        if (!IS_ERR_OR_NULL(sw))


thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

On Wed, May 22, 2019 at 02:57:33PM +0000, Biju Das wrote:
> Hi Heikki,
> 
> Thanks for the patch
> 
> > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > node
> > 
> > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > Hi Chunfeng Yun,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > Hi Biju,
> > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > Hi Heikki,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > usb_role_switch by node
> > > > > > > > > >
> > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun
> > wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki
> > > > > > > > > > > > Krogerus
> > > > wrote:
> > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng
> > > > > > > > > > > > > Yun
> > > > > > wrote:
> > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier
> > > > > > > > > > > > > > to get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > registered between two drivers and only knows
> > > > > > > > > > > > > > the fwnode which register usb_role_switch.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > > >
> > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > where he points out that you don't need to use
> > > > > > > > > > > > device graph since the controller is the parent of
> > > > > > > > > > > > the connector. Doesn't that mean you don't really need
> > this API?
> > > > > > > > > > > No, I still need it.
> > > > > > > > > > > The change is about the way how to get fwnode; when
> > > > > > > > > > > use device graph, get fwnode by
> > > > > > > > > > > of_graph_get_remote_node(); but now will get fwnode by
> > > > > > > > > > > of_get_parent();
> > > > > > > > > >
> > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > something like this function is needed at all. I also
> > > > > > > > > > have concerns regarding how you are using the function.
> > > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > > series...
> > > > > > > > >
> > > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > >
> > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > usb_role_switch_get()
> > > > > > enough?
> > > > > > >
> > > > > > > Currently no issue. It will work with this api as well, since
> > > > > > > the port node is
> > > > > > part of controller node.
> > > > > > > For eg:-
> > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > >
> > > > > > > However if any one adds port node inside the connector node,
> > > > > > > then this
> > > > > > api may won't work as expected.
> > > > > > > Currently I get below error
> > > > > > >
> > > > > > > [    2.299703] OF: graph: no port node found in
> > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > >
> > > > > > We need to understand why is that happening?
> > > > > >
> > > > >
> > > > > Form the stack trace  the parent node is
> > > > > "parent_node=hd3ss3220@47" ,
> > > > instead of the "connector" node.
> > > > > That is the reason for the above error.
> > > > >
> > > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > > >
> > > > > The use case is
> > > > >
> > > > > &i2c0 {
> > > > > 	hd3ss3220@47 {
> > > > >                  	compatible = "ti,hd3ss3220";
> > > > >
> > > > >                  	usb_con: connector {
> > > > >                           		compatible = "usb-c-connector";
> > > > >                          		port {
> > > > >                                 		 hd3ss3220_ep: endpoint {
> > > > >                                         			remote-endpoint =
> > > > <&usb3_role_switch>;
> > > > >                                 		};
> > > > >                          		};
> > > > >                 	 };
> > > > > 	 };
> > > > > };
> > > > >
> > > > > &usb3_peri0 {
> > > > >          companion = <&xhci0>;
> > > > >          usb-role-switch;
> > > > >
> > > > >          port {
> > > > >                 usb3_role_switch: endpoint {
> > > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > > >                  };
> > > > >          };
> > > > > };
> > > > >
> > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > Child(connector) and child's endpoint?
> > > > How about firstly finding connector node in
> > > > fwnode_graph_devcon_match(), then search each endpoint?
> > >
> > >  I have done a quick prototyping with the changes you suggested and it
> > works.
> > >
> > > -       struct fwnode_handle *ep;
> > > +       struct fwnode_handle *ep,*child,*tmp = fwnode;
> > >
> > > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > > +       if (child)
> > > +               tmp = child;
> > > +
> > > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> > >
> > > Form the stack trace  the parent node is "parent_node= connector" .
> > >
> > > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > > [    2.461888]  usb_role_switch_get+0x20/0x28
> > >
> > > Heikki,
> > > Are you ok  with the above changes?
> > 
> > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way I
> > proposed, there is no problem? You just find the "connector" child node in
> > your driver, and pass that to fwnode_usb_role_switch_get():
> 
> Yes, That is correct.
> 
> >         struct fwnode_handle *connector;
> >         ...
> >         connector = device_get_named_child_node(&client->dev, "connector");
> >         if (IS_ERR(connector))
> >                 <do something>
> > 
> >         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> >         ...
> > 
> > The difference is that instead of just converting a device node of an usb role
> > switch to the usb role switch, it works just like usb_role_switch_get(), just
> > taking fwnode instead of device entry as parameter.
> > 
> > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > way I though it needs to work for my own tests. Please find the patches
> > attached.
> 
> I have tested  this patches and conform it works. 
> Do you plan to post this patches to ML? 

Could make them part of this series?


thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

On Fri, 2019-05-24 at 14:40 +0300, Heikki Krogerus wrote:
> On Thu, May 23, 2019 at 06:16:10PM +0800, Chunfeng Yun wrote:
> > Hi Heikki,
> > On Wed, 2019-05-22 at 17:26 +0300, Heikki Krogerus wrote:
> > > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > > Hi Chunfeng Yun,
> > > > 
> > > > Thanks for the feedback.
> > > > 
> > > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > > > node
> > > > > 
> > > > > Hi Biju,
> > > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > Thanks for the feedback.
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > > Hi Heikki,
> > > > > > > > > >
> > > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > > usb_role_switch by node
> > > > > > > > > > >
> > > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki Krogerus
> > > > > wrote:
> > > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng Yun
> > > > > > > wrote:
> > > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier to
> > > > > > > > > > > > > > > get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > > registered between two drivers and only knows the
> > > > > > > > > > > > > > > fwnode which register usb_role_switch.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > > where he points out that you don't need to use device
> > > > > > > > > > > > > graph since the controller is the parent of the
> > > > > > > > > > > > > connector. Doesn't that mean you don't really need this API?
> > > > > > > > > > > > No, I still need it.
> > > > > > > > > > > > The change is about the way how to get fwnode; when use
> > > > > > > > > > > > device graph, get fwnode by of_graph_get_remote_node();
> > > > > > > > > > > > but now will get fwnode by of_get_parent();
> > > > > > > > > > >
> > > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > > something like this function is needed at all. I also have
> > > > > > > > > > > concerns regarding how you are using the function. I'll
> > > > > > > > > > > explain in comment to the patch 5/6 in this
> > > > > > > > > series...
> > > > > > > > > >
> > > > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > > >
> > > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > > usb_role_switch_get()
> > > > > > > enough?
> > > > > > > >
> > > > > > > > Currently no issue. It will work with this api as well, since the
> > > > > > > > port node is
> > > > > > > part of controller node.
> > > > > > > > For eg:-
> > > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > > >
> > > > > > > > However if any one adds port node inside the connector node, then
> > > > > > > > this
> > > > > > > api may won't work as expected.
> > > > > > > > Currently I get below error
> > > > > > > >
> > > > > > > > [    2.299703] OF: graph: no port node found in
> > > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > > >
> > > > > > > We need to understand why is that happening?
> > > > > > >
> > > > > >
> > > > > > Form the stack trace  the parent node is "parent_node=hd3ss3220@47" ,
> > > > > instead of the "connector" node.
> > > > > > That is the reason for the above error.
> > > > > >
> > > > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > > > >
> > > > > > The use case is
> > > > > >
> > > > > > &i2c0 {
> > > > > > 	hd3ss3220@47 {
> > > > > >                  	compatible = "ti,hd3ss3220";
> > > > > >
> > > > > >                  	usb_con: connector {
> > > > > >                           		compatible = "usb-c-connector";
> > > > > >                          		port {
> > > > > >                                 		 hd3ss3220_ep: endpoint {
> > > > > >                                         			remote-endpoint =
> > > > > <&usb3_role_switch>;
> > > > > >                                 		};
> > > > > >                          		};
> > > > > >                 	 };
> > > > > > 	 };
> > > > > > };
> > > > > >
> > > > > > &usb3_peri0 {
> > > > > >          companion = <&xhci0>;
> > > > > >          usb-role-switch;
> > > > > >
> > > > > >          port {
> > > > > >                 usb3_role_switch: endpoint {
> > > > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > > > >                  };
> > > > > >          };
> > > > > > };
> > > > > >
> > > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > > Child(connector) and child's endpoint?
> > > > > How about firstly finding connector node in fwnode_graph_devcon_match(),
> > > > > then search each endpoint?
> > > > 
> > > >  I have done a quick prototyping with the changes you suggested and it works.
> > > > 
> > > > -       struct fwnode_handle *ep;
> > > > +       struct fwnode_handle *ep,*child,*tmp = fwnode; 
> > > >  
> > > > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > > > +       if (child)
> > > > +               tmp = child;
> > > > +
> > > > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> > > > 
> > > > Form the stack trace  the parent node is "parent_node= connector" .
> > > > 
> > > > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > > > [    2.461888]  usb_role_switch_get+0x20/0x28
> > > > 
> > > > Heikki, 
> > > > Are you ok  with the above changes?
> > > 
> > > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way
> > > I proposed, there is no problem? You just find the "connector" child
> > > node in your driver, and pass that to fwnode_usb_role_switch_get():
> > > 
> > >         struct fwnode_handle *connector;
> > >         ...
> > >         connector = device_get_named_child_node(&client->dev, "connector");
> > >         if (IS_ERR(connector))
> > >                 <do something>
> > > 
> > >         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > >         ...
> > > 
> > > The difference is that instead of just converting a device node of an
> > > usb role switch to the usb role switch, it works just like
> > > usb_role_switch_get(), just taking fwnode instead of device entry as
> > > parameter.
> > > 
> > > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > > way I though it needs to work for my own tests. Please find the
> > > patches attached.
> > I tested these patches, it didn't work for case as following:
> > 
> > case 2:
> > 
> > &mtu3 {
> >     usb-role-switch;
> > 
> >     connector {
> >         compatible = "linux,typeb-conn-gpio", "usb-b-connector";
> >         label = "micro-USB";
> >         type = "micro";
> >         id-gpios = <&pio 12 GPIO_ACTIVE_HIGH>;
> >         vbus-supply = <&usb_p0_vbus>;
> >     };
> > };
> > 
> > so I consider this case in the function fwnode_graph_devcon_match()
> > (based on your patches)
> 
> IMO that case should be handled in USB role switch API initially, not
> in the device connection API. If there is another user for it one day,
> then we can move it to device connection API, but not before that.
Ok
> 
> How about this on top of the two patches I sent:
I just test it, it works well.
I'll prepare a patch and put into this series.

> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index aab795b54c7f..36ac9d181e09 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -114,6 +114,19 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
>         return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
>  }
> 
> +static struct usb_role_switch *
> +usb_role_switch_is_parent(struct fwnode_handle *parent)
> +{
> +       struct device *dev;
> +
> +       if (!parent || !fwnode_property_present(parent, "usb-role-switch"))
> +               return NULL;
> +
> +       dev = class_find_device(role_class, NULL, parent, switch_fwnode_match);
> +
> +       return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> +}
> +
>  /**
>   * usb_role_switch_get - Find USB role switch linked with the caller
>   * @dev: The caller device
> @@ -125,6 +138,10 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  {
>         struct usb_role_switch *sw;
> 
> +       sw = usb_role_switch_is_parent(fwnode_get_parent(dev_fwnode(dev)));
> +       if (sw)
> +               return sw;
Do we also need to get parent module before return?

Thanks.
> +
>         sw = device_connection_find_match(dev, "usb-role-switch", NULL,
>                                           usb_role_switch_match);
> 
> @@ -146,6 +163,10 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
>  {
>         struct usb_role_switch *sw;
> 
> +       sw = usb_role_switch_is_parent(fwnode_get_parent(fwnode));
> +       if (sw)
> +               return sw;
> +
>         sw = fwnode_connection_find_match(fwnode, "usb-role-switch", NULL,
>                                           usb_role_switch_match);
>         if (!IS_ERR_OR_NULL(sw))
> 
> 
> thanks,
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi Heikki & Biju,
On Fri, 2019-05-24 at 15:44 +0300, Heikki Krogerus wrote:
> On Wed, May 22, 2019 at 02:57:33PM +0000, Biju Das wrote:
> > Hi Heikki,
> > 
> > Thanks for the patch
> > 
> > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> > > node
> > > 
> > > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > > Hi Chunfeng Yun,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > > > usb_role_switch by node
> > > > >
> > > > > Hi Biju,
> > > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > > Hi Heikki,
> > > > > >
> > > > > > Thanks for the feedback.
> > > > > >
> > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > usb_role_switch by node
> > > > > > >
> > > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Heikki,
> > > > > > > >
> > > > > > > > Thanks for the feedback.
> > > > > > > >
> > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > usb_role_switch by node
> > > > > > > > >
> > > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > > Hi Heikki,
> > > > > > > > > >
> > > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > > usb_role_switch by node
> > > > > > > > > > >
> > > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng Yun
> > > wrote:
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300, Heikki
> > > > > > > > > > > > > Krogerus
> > > > > wrote:
> > > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800, Chunfeng
> > > > > > > > > > > > > > Yun
> > > > > > > wrote:
> > > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make easier
> > > > > > > > > > > > > > > to get usb_role_switch by fwnode which register it.
> > > > > > > > > > > > > > > It's useful when there is not device_connection
> > > > > > > > > > > > > > > registered between two drivers and only knows
> > > > > > > > > > > > > > > the fwnode which register usb_role_switch.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > > > Tested-by: Biju Das <biju.das@bp.renesas.com>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch 2/6,
> > > > > > > > > > > > > where he points out that you don't need to use
> > > > > > > > > > > > > device graph since the controller is the parent of
> > > > > > > > > > > > > the connector. Doesn't that mean you don't really need
> > > this API?
> > > > > > > > > > > > No, I still need it.
> > > > > > > > > > > > The change is about the way how to get fwnode; when
> > > > > > > > > > > > use device graph, get fwnode by
> > > > > > > > > > > > of_graph_get_remote_node(); but now will get fwnode by
> > > > > > > > > > > > of_get_parent();
> > > > > > > > > > >
> > > > > > > > > > > OK, I get that, but I'm still not convinced about if
> > > > > > > > > > > something like this function is needed at all. I also
> > > > > > > > > > > have concerns regarding how you are using the function.
> > > > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > > > series...
> > > > > > > > > >
> > > > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > > >
> > > > > > > > > Yes, and I have the same question for you I jusb asked in
> > > > > > > > > comment I added to the patch 5/6 of this series. Why isn't
> > > > > > > > > usb_role_switch_get()
> > > > > > > enough?
> > > > > > > >
> > > > > > > > Currently no issue. It will work with this api as well, since
> > > > > > > > the port node is
> > > > > > > part of controller node.
> > > > > > > > For eg:-
> > > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > > >
> > > > > > > > However if any one adds port node inside the connector node,
> > > > > > > > then this
> > > > > > > api may won't work as expected.
> > > > > > > > Currently I get below error
> > > > > > > >
> > > > > > > > [    2.299703] OF: graph: no port node found in
> > > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > > >
> > > > > > > We need to understand why is that happening?
> > > > > > >
> > > > > >
> > > > > > Form the stack trace  the parent node is
> > > > > > "parent_node=hd3ss3220@47" ,
> > > > > instead of the "connector" node.
> > > > > > That is the reason for the above error.
> > > > > >
> > > > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > > > >
> > > > > > The use case is
> > > > > >
> > > > > > &i2c0 {
> > > > > > 	hd3ss3220@47 {
> > > > > >                  	compatible = "ti,hd3ss3220";
> > > > > >
> > > > > >                  	usb_con: connector {
> > > > > >                           		compatible = "usb-c-connector";
> > > > > >                          		port {
> > > > > >                                 		 hd3ss3220_ep: endpoint {
> > > > > >                                         			remote-endpoint =
> > > > > <&usb3_role_switch>;
> > > > > >                                 		};
> > > > > >                          		};
> > > > > >                 	 };
> > > > > > 	 };
> > > > > > };
> > > > > >
> > > > > > &usb3_peri0 {
> > > > > >          companion = <&xhci0>;
> > > > > >          usb-role-switch;
> > > > > >
> > > > > >          port {
> > > > > >                 usb3_role_switch: endpoint {
> > > > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > > > >                  };
> > > > > >          };
> > > > > > };
> > > > > >
> > > > > > Q1) How do we modify the usb_role_switch_get() function to search
> > > > > > Child(connector) and child's endpoint?
> > > > > How about firstly finding connector node in
> > > > > fwnode_graph_devcon_match(), then search each endpoint?
> > > >
> > > >  I have done a quick prototyping with the changes you suggested and it
> > > works.
> > > >
> > > > -       struct fwnode_handle *ep;
> > > > +       struct fwnode_handle *ep,*child,*tmp = fwnode;
> > > >
> > > > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > > > +       if (child)
> > > > +               tmp = child;
> > > > +
> > > > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> > > >
> > > > Form the stack trace  the parent node is "parent_node= connector" .
> > > >
> > > > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > > > [    2.461888]  usb_role_switch_get+0x20/0x28
> > > >
> > > > Heikki,
> > > > Are you ok  with the above changes?
> > > 
> > > Doesn't that mean that if we made fwnode_usb_role_switch_get() the way I
> > > proposed, there is no problem? You just find the "connector" child node in
> > > your driver, and pass that to fwnode_usb_role_switch_get():
> > 
> > Yes, That is correct.
> > 
> > >         struct fwnode_handle *connector;
> > >         ...
> > >         connector = device_get_named_child_node(&client->dev, "connector");
> > >         if (IS_ERR(connector))
> > >                 <do something>
> > > 
> > >         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > >         ...
> > > 
> > > The difference is that instead of just converting a device node of an usb role
> > > switch to the usb role switch, it works just like usb_role_switch_get(), just
> > > taking fwnode instead of device entry as parameter.
> > > 
> > > I prepared the patches implementing fwnode_usb_role_switch_get() the
> > > way I though it needs to work for my own tests. Please find the patches
> > > attached.
> > 
> > I have tested  this patches and conform it works. 
> > Do you plan to post this patches to ML? 
> 
> Could make them part of this series?
I'll do it, thanks
> 
> 
> thanks,
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Heikki Krogerus]

Hi,

> > IMO that case should be handled in USB role switch API initially, not
> > in the device connection API. If there is another user for it one day,
> > then we can move it to device connection API, but not before that.
> Ok
> > 
> > How about this on top of the two patches I sent:
> I just test it, it works well.
> I'll prepare a patch and put into this series.
> 
> > 
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index aab795b54c7f..36ac9d181e09 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -114,6 +114,19 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
> >         return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> >  }
> > 
> > +static struct usb_role_switch *
> > +usb_role_switch_is_parent(struct fwnode_handle *parent)
> > +{
> > +       struct device *dev;
> > +
> > +       if (!parent || !fwnode_property_present(parent, "usb-role-switch"))
> > +               return NULL;
> > +
> > +       dev = class_find_device(role_class, NULL, parent, switch_fwnode_match);
> > +
> > +       return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
> > +}
> > +
> >  /**
> >   * usb_role_switch_get - Find USB role switch linked with the caller
> >   * @dev: The caller device
> > @@ -125,6 +138,10 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> >  {
> >         struct usb_role_switch *sw;
> > 
> > +       sw = usb_role_switch_is_parent(fwnode_get_parent(dev_fwnode(dev)));
> > +       if (sw)
> > +               return sw;
> Do we also need to get parent module before return?

Yes.

thanks,

-- 
heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Biju Das]

Hi Chunfeng Yun,

+ Chen Yu

Thanks for the feedback.

> Subject: Re: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by
> node
> 
> Hi Heikki & Biju,
> On Fri, 2019-05-24 at 15:44 +0300, Heikki Krogerus wrote:
> > On Wed, May 22, 2019 at 02:57:33PM +0000, Biju Das wrote:
> > > Hi Heikki,
> > >
> > > Thanks for the patch
> > >
> > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > usb_role_switch by node
> > > >
> > > > On Wed, May 22, 2019 at 10:55:17AM +0000, Biju Das wrote:
> > > > > Hi Chunfeng Yun,
> > > > >
> > > > > Thanks for the feedback.
> > > > >
> > > > > > Subject: RE: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > usb_role_switch by node
> > > > > >
> > > > > > Hi Biju,
> > > > > > On Wed, 2019-05-22 at 08:05 +0000, Biju Das wrote:
> > > > > > > Hi Heikki,
> > > > > > >
> > > > > > > Thanks for the feedback.
> > > > > > >
> > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > usb_role_switch by node
> > > > > > > >
> > > > > > > > On Mon, May 20, 2019 at 09:45:46AM +0000, Biju Das wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi Heikki,
> > > > > > > > >
> > > > > > > > > Thanks for the feedback.
> > > > > > > > >
> > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to get
> > > > > > > > > > usb_role_switch by node
> > > > > > > > > >
> > > > > > > > > > On Mon, May 20, 2019 at 08:06:41AM +0000, Biju Das wrote:
> > > > > > > > > > > Hi Heikki,
> > > > > > > > > > >
> > > > > > > > > > > > Subject: Re: [PATCH v5 4/6] usb: roles: add API to
> > > > > > > > > > > > get usb_role_switch by node
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, May 20, 2019 at 10:39:11AM +0800, Chunfeng
> > > > > > > > > > > > Yun
> > > > wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > On Fri, 2019-05-17 at 16:05 +0300, Heikki Krogerus
> wrote:
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, May 17, 2019 at 01:37:36PM +0300,
> > > > > > > > > > > > > > Heikki Krogerus
> > > > > > wrote:
> > > > > > > > > > > > > > > On Tue, May 14, 2019 at 04:47:21PM +0800,
> > > > > > > > > > > > > > > Chunfeng Yun
> > > > > > > > wrote:
> > > > > > > > > > > > > > > > Add fwnode_usb_role_switch_get() to make
> > > > > > > > > > > > > > > > easier to get usb_role_switch by fwnode which
> register it.
> > > > > > > > > > > > > > > > It's useful when there is not
> > > > > > > > > > > > > > > > device_connection registered between two
> > > > > > > > > > > > > > > > drivers and only knows the fwnode which register
> usb_role_switch.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Signed-off-by: Chunfeng Yun
> > > > > > > > > > > > > > > > <chunfeng.yun@mediatek.com>
> > > > > > > > > > > > > > > > Tested-by: Biju Das
> > > > > > > > > > > > > > > > <biju.das@bp.renesas.com>
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Acked-by: Heikki Krogerus
> > > > > > > > > > > > > > > <heikki.krogerus@linux.intel.com>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hold on. I just noticed Rob's comment on patch
> > > > > > > > > > > > > > 2/6, where he points out that you don't need
> > > > > > > > > > > > > > to use device graph since the controller is
> > > > > > > > > > > > > > the parent of the connector. Doesn't that mean
> > > > > > > > > > > > > > you don't really need
> > > > this API?
> > > > > > > > > > > > > No, I still need it.
> > > > > > > > > > > > > The change is about the way how to get fwnode;
> > > > > > > > > > > > > when use device graph, get fwnode by
> > > > > > > > > > > > > of_graph_get_remote_node(); but now will get
> > > > > > > > > > > > > fwnode by of_get_parent();
> > > > > > > > > > > >
> > > > > > > > > > > > OK, I get that, but I'm still not convinced about
> > > > > > > > > > > > if something like this function is needed at all.
> > > > > > > > > > > > I also have concerns regarding how you are using the
> function.
> > > > > > > > > > > > I'll explain in comment to the patch 5/6 in this
> > > > > > > > > > series...
> > > > > > > > > > >
> > > > > > > > > > > FYI, Currently  I am also using this api in my patch series.
> > > > > > > > > > > https://patchwork.kernel.org/patch/10944637/
> > > > > > > > > >
> > > > > > > > > > Yes, and I have the same question for you I jusb asked
> > > > > > > > > > in comment I added to the patch 5/6 of this series.
> > > > > > > > > > Why isn't
> > > > > > > > > > usb_role_switch_get()
> > > > > > > > enough?
> > > > > > > > >
> > > > > > > > > Currently no issue. It will work with this api as well,
> > > > > > > > > since the port node is
> > > > > > > > part of controller node.
> > > > > > > > > For eg:-
> > > > > > > > > https://patchwork.kernel.org/patch/10944627/
> > > > > > > > >
> > > > > > > > > However if any one adds port node inside the connector
> > > > > > > > > node, then this
> > > > > > > > api may won't work as expected.
> > > > > > > > > Currently I get below error
> > > > > > > > >
> > > > > > > > > [    2.299703] OF: graph: no port node found in
> > > > > > > > /soc/i2c@e6500000/hd3ss3220@47
> > > > > > > >
> > > > > > > > We need to understand why is that happening?
> > > > > > > >
> > > > > > >
> > > > > > > Form the stack trace  the parent node is
> > > > > > > "parent_node=hd3ss3220@47" ,
> > > > > > instead of the "connector" node.
> > > > > > > That is the reason for the above error.
> > > > > > >
> > > > > > > [    2.442429]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > > > [    2.447889]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > > > [    2.453267]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > > > [    2.458374]  device_connection_find_match+0x74/0x1a0
> > > > > > > [    2.463399]  usb_role_switch_get+0x20/0x28
> > > > > > > [    2.467542]  hd3ss3220_probe+0xc4/0x218
> > > > > > >
> > > > > > > The use case is
> > > > > > >
> > > > > > > &i2c0 {
> > > > > > > 	hd3ss3220@47 {
> > > > > > >                  	compatible = "ti,hd3ss3220";
> > > > > > >
> > > > > > >                  	usb_con: connector {
> > > > > > >                           		compatible = "usb-c-connector";
> > > > > > >                          		port {
> > > > > > >                                 		 hd3ss3220_ep: endpoint {
> > > > > > >                                         			remote-endpoint =
> > > > > > <&usb3_role_switch>;
> > > > > > >                                 		};
> > > > > > >                          		};
> > > > > > >                 	 };
> > > > > > > 	 };
> > > > > > > };
> > > > > > >
> > > > > > > &usb3_peri0 {
> > > > > > >          companion = <&xhci0>;
> > > > > > >          usb-role-switch;
> > > > > > >
> > > > > > >          port {
> > > > > > >                 usb3_role_switch: endpoint {
> > > > > > >                         remote-endpoint = <&hd3ss3220_ep>;
> > > > > > >                  };
> > > > > > >          };
> > > > > > > };
> > > > > > >
> > > > > > > Q1) How do we modify the usb_role_switch_get() function to
> > > > > > > search
> > > > > > > Child(connector) and child's endpoint?
> > > > > > How about firstly finding connector node in
> > > > > > fwnode_graph_devcon_match(), then search each endpoint?
> > > > >
> > > > >  I have done a quick prototyping with the changes you suggested
> > > > > and it
> > > > works.
> > > > >
> > > > > -       struct fwnode_handle *ep;
> > > > > +       struct fwnode_handle *ep,*child,*tmp = fwnode;
> > > > >
> > > > > -       fwnode_graph_for_each_endpoint(fwnode, ep) {
> > > > > +       child = fwnode_get_named_child_node(fwnode, "connector");
> > > > > +       if (child)
> > > > > +               tmp = child;
> > > > > +
> > > > > +       fwnode_graph_for_each_endpoint(tmp, ep) {
> > > > >
> > > > > Form the stack trace  the parent node is "parent_node= connector" .
> > > > >
> > > > > [    2.440922]  of_graph_get_next_endpoint.part.0+0x28/0x168
> > > > > [    2.446381]  of_fwnode_graph_get_next_endpoint+0x5c/0xb0
> > > > > [    2.451758]  fwnode_graph_get_next_endpoint+0x20/0x30
> > > > > [    2.456866]  device_connection_find_match+0x84/0x1c0
> > > > > [    2.461888]  usb_role_switch_get+0x20/0x28
> > > > >
> > > > > Heikki,
> > > > > Are you ok  with the above changes?
> > > >
> > > > Doesn't that mean that if we made fwnode_usb_role_switch_get() the
> > > > way I proposed, there is no problem? You just find the "connector"
> > > > child node in your driver, and pass that to
> fwnode_usb_role_switch_get():
> > >
> > > Yes, That is correct.
> > >
> > > >         struct fwnode_handle *connector;
> > > >         ...
> > > >         connector = device_get_named_child_node(&client->dev,
> "connector");
> > > >         if (IS_ERR(connector))
> > > >                 <do something>
> > > >
> > > >         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > > >         ...
> > > >
> > > > The difference is that instead of just converting a device node of
> > > > an usb role switch to the usb role switch, it works just like
> > > > usb_role_switch_get(), just taking fwnode instead of device entry as
> parameter.
> > > >
> > > > I prepared the patches implementing fwnode_usb_role_switch_get()
> > > > the way I though it needs to work for my own tests. Please find
> > > > the patches attached.
> > >
> > > I have tested  this patches and conform it works.
> > > Do you plan to post this patches to ML?
> >
> > Could make them part of this series?
> I'll do it, thanks

Just a suggestion, Do you think, is it worth to add the below  patch[1] also part of this series? So that we have all common patches in this series.

"usb: roles: Introduce stubs for the exiting functions in role.h."
[1] https://patchwork.kernel.org/patch/10909971/

Regards,
Biju
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5 4/6] usb: roles: add API to get usb_role_switch by node

[Chunfeng Yun]

Hi Biju & Yu,

On Tue, 2019-05-28 at 06:52 +0000, Biju Das wrote:
> Hi Chunfeng Yun,
> 
> + Chen Yu
> 
> Thanks for the feedback.
[...]
> 
> Just a suggestion, Do you think, is it worth to add the below  patch[1] also part of this series? So that we have all common patches in this series.
> 
Or resend it as a single patch?

> "usb: roles: Introduce stubs for the exiting functions in role.h."
> [1] https://patchwork.kernel.org/patch/10909971/
> 
 
> Regards,
> Biju



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel