[PATCH v2 0/9] device connection: Add support for device graphs

[PATCH v2 0/9] device connection: Add support for device graphs

[Heikki Krogerus]

Hi,

This is the second version of this series. On top the two code style
improvements requested by Andy, I also renamed the connection
identifiers used with the USB Type-C muxes for something that I felt
are better, especially after we start using them to name reference
device properties in fwnodes. That's why the first patch is now split
in two, 1/9 and 3/9.

Hans! Please note that there is no functional change. The alt mode
device is still getting a handle to the mux, just like before.
That was actually happening also in the first version of the series.

The commit message from v1:

This series adds support for OF and ACPI device graph parsing to the
device connection API.

Handling the graph is straightforward, but because I'm adding that
fwnode member to struct device_connection, I had to make sure all the
existing users consider it.

The plan is to only support matching with fwnode in the future, so no
more device name matching. The software fwnodes that we now have in
kernel should make that possible, once we add support for references
to them.

The original RFC:
https://lkml.org/lkml/2018/10/24/619

thanks,

Heikki Krogerus (9):
  platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme
  usb: typec: Rationalize the API for the muxes
  platform/x86: intel_cht_int33fe: Remove old style mux connections
  device connection: Add fwnode member to struct device_connection
  usb: typec: mux: Find the muxes by also matching against the device
    node
  usb: roles: Find the muxes by also matching against the device node
  usb: typec: Find the ports by also matching against the device node
  device connection: Prepare support for firmware described connections
  device connection: Find device connections also from device graphs

 drivers/base/devcon.c                    | 62 ++++++++++++++-
 drivers/platform/x86/intel_cht_int33fe.c | 15 ++--
 drivers/usb/roles/class.c                | 21 +++++-
 drivers/usb/typec/class.c                | 31 ++++++--
 drivers/usb/typec/mux.c                  | 96 ++++++++++++++++++++----
 include/linux/device.h                   |  6 ++
 include/linux/usb/role.h                 |  1 +
 include/linux/usb/typec_mux.h            |  3 +-
 8 files changed, 195 insertions(+), 40 deletions(-)

-- 
2.20.1

[PATCH v2 1/9] platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme

[Heikki Krogerus]

Adding new connections with for the muxes with new
identifiers. The old connection are left in for now.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 02bc74608cf3..295fe19ad4c2 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -32,7 +32,7 @@ struct cht_int33fe_data {
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
 	/* Contain a list-head must be per device */
-	struct device_connection connections[5];
+	struct device_connection connections[7];
 };
 
 /*
@@ -184,6 +184,12 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	data->connections[3].endpoint[0] = "i2c-fusb302";
 	data->connections[3].endpoint[1] = "intel_xhci_usb_sw-role-switch";
 	data->connections[3].id = "usb-role-switch";
+	data->connections[4].endpoint[0] = "port0";
+	data->connections[4].endpoint[1] = "i2c-pi3usb30532";
+	data->connections[4].id = "orientation-switch";
+	data->connections[5].endpoint[0] = "port0";
+	data->connections[5].endpoint[1] = "i2c-pi3usb30532";
+	data->connections[5].id = "mode-switch";
 
 	device_connections_add(data->connections);
 
-- 
2.20.1

[v2,1/9] platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme

[Heikki Krogerus]

Adding new connections with for the muxes with new
identifiers. The old connection are left in for now.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 02bc74608cf3..295fe19ad4c2 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -32,7 +32,7 @@ struct cht_int33fe_data {
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
 	/* Contain a list-head must be per device */
-	struct device_connection connections[5];
+	struct device_connection connections[7];
 };
 
 /*
@@ -184,6 +184,12 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	data->connections[3].endpoint[0] = "i2c-fusb302";
 	data->connections[3].endpoint[1] = "intel_xhci_usb_sw-role-switch";
 	data->connections[3].id = "usb-role-switch";
+	data->connections[4].endpoint[0] = "port0";
+	data->connections[4].endpoint[1] = "i2c-pi3usb30532";
+	data->connections[4].id = "orientation-switch";
+	data->connections[5].endpoint[0] = "port0";
+	data->connections[5].endpoint[1] = "i2c-pi3usb30532";
+	data->connections[5].id = "mode-switch";
 
 	device_connections_add(data->connections);
 

[PATCH v2 2/9] usb: typec: Rationalize the API for the muxes

[Heikki Krogerus]

Since with accessory modes there is no need for additional
identification when requesting a handle to the mux, we can
replace the second parameter that is passed to the
typec_mux_get() function with a pointer to alternate mode
description structure, and simply passing NULL with
accessory modes.

This change means the naming of the mux device connections
can be updated. Alternate and Accessory Modes will both be
handled with muxes named "mode-switch", and the orientation
switches will be named "orientation-switch".

Future identification of the alternate modes will be later
done using device property "svid" of the mux.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c     |  7 ++-----
 drivers/usb/typec/mux.c       | 10 ++++++----
 include/linux/usb/typec_mux.h |  3 ++-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 5db0593ca0bd..f50278dbef59 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1496,11 +1496,8 @@ typec_port_register_altmode(struct typec_port *port,
 {
 	struct typec_altmode *adev;
 	struct typec_mux *mux;
-	char id[10];
 
-	sprintf(id, "id%04xm%02x", desc->svid, desc->mode);
-
-	mux = typec_mux_get(&port->dev, id);
+	mux = typec_mux_get(&port->dev, desc);
 	if (IS_ERR(mux))
 		return ERR_CAST(mux);
 
@@ -1593,7 +1590,7 @@ struct typec_port *typec_register_port(struct device *parent,
 		return ERR_CAST(port->sw);
 	}
 
-	port->mux = typec_mux_get(&port->dev, "typec-mux");
+	port->mux = typec_mux_get(&port->dev, NULL);
 	if (IS_ERR(port->mux)) {
 		put_device(&port->dev);
 		return ERR_CAST(port->mux);
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index d990aa510fab..8975f58e1d60 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -48,7 +48,7 @@ struct typec_switch *typec_switch_get(struct device *dev)
 	struct typec_switch *sw;
 
 	mutex_lock(&switch_lock);
-	sw = device_connection_find_match(dev, "typec-switch", NULL,
+	sw = device_connection_find_match(dev, "orientation-switch", NULL,
 					  typec_switch_match);
 	if (!IS_ERR_OR_NULL(sw)) {
 		WARN_ON(!try_module_get(sw->dev->driver->owner));
@@ -128,19 +128,21 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 /**
  * typec_mux_get - Find USB Type-C Multiplexer
  * @dev: The caller device
- * @name: Mux identifier
+ * @desc: Alt Mode description
  *
  * Finds a mux linked to the caller. This function is primarily meant for the
  * Type-C drivers. Returns a reference to the mux on success, NULL if no
  * matching connection was found, or ERR_PTR(-EPROBE_DEFER) when a connection
  * was found but the mux has not been enumerated yet.
  */
-struct typec_mux *typec_mux_get(struct device *dev, const char *name)
+struct typec_mux *typec_mux_get(struct device *dev,
+				const struct typec_altmode_desc *desc)
 {
 	struct typec_mux *mux;
 
 	mutex_lock(&mux_lock);
-	mux = device_connection_find_match(dev, name, NULL, typec_mux_match);
+	mux = device_connection_find_match(dev, "mode-switch", (void *)desc,
+					   typec_mux_match);
 	if (!IS_ERR_OR_NULL(mux)) {
 		WARN_ON(!try_module_get(mux->dev->driver->owner));
 		get_device(mux->dev);
diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index 79293f630ee1..43f40685e53c 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -47,7 +47,8 @@ void typec_switch_put(struct typec_switch *sw);
 int typec_switch_register(struct typec_switch *sw);
 void typec_switch_unregister(struct typec_switch *sw);
 
-struct typec_mux *typec_mux_get(struct device *dev, const char *name);
+struct typec_mux *
+typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc);
 void typec_mux_put(struct typec_mux *mux);
 int typec_mux_register(struct typec_mux *mux);
 void typec_mux_unregister(struct typec_mux *mux);
-- 
2.20.1

[v2,2/9] usb: typec: Rationalize the API for the muxes

[Heikki Krogerus]

Since with accessory modes there is no need for additional
identification when requesting a handle to the mux, we can
replace the second parameter that is passed to the
typec_mux_get() function with a pointer to alternate mode
description structure, and simply passing NULL with
accessory modes.

This change means the naming of the mux device connections
can be updated. Alternate and Accessory Modes will both be
handled with muxes named "mode-switch", and the orientation
switches will be named "orientation-switch".

Future identification of the alternate modes will be later
done using device property "svid" of the mux.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c     |  7 ++-----
 drivers/usb/typec/mux.c       | 10 ++++++----
 include/linux/usb/typec_mux.h |  3 ++-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 5db0593ca0bd..f50278dbef59 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1496,11 +1496,8 @@ typec_port_register_altmode(struct typec_port *port,
 {
 	struct typec_altmode *adev;
 	struct typec_mux *mux;
-	char id[10];
 
-	sprintf(id, "id%04xm%02x", desc->svid, desc->mode);
-
-	mux = typec_mux_get(&port->dev, id);
+	mux = typec_mux_get(&port->dev, desc);
 	if (IS_ERR(mux))
 		return ERR_CAST(mux);
 
@@ -1593,7 +1590,7 @@ struct typec_port *typec_register_port(struct device *parent,
 		return ERR_CAST(port->sw);
 	}
 
-	port->mux = typec_mux_get(&port->dev, "typec-mux");
+	port->mux = typec_mux_get(&port->dev, NULL);
 	if (IS_ERR(port->mux)) {
 		put_device(&port->dev);
 		return ERR_CAST(port->mux);
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index d990aa510fab..8975f58e1d60 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -48,7 +48,7 @@ struct typec_switch *typec_switch_get(struct device *dev)
 	struct typec_switch *sw;
 
 	mutex_lock(&switch_lock);
-	sw = device_connection_find_match(dev, "typec-switch", NULL,
+	sw = device_connection_find_match(dev, "orientation-switch", NULL,
 					  typec_switch_match);
 	if (!IS_ERR_OR_NULL(sw)) {
 		WARN_ON(!try_module_get(sw->dev->driver->owner));
@@ -128,19 +128,21 @@ static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 /**
  * typec_mux_get - Find USB Type-C Multiplexer
  * @dev: The caller device
- * @name: Mux identifier
+ * @desc: Alt Mode description
  *
  * Finds a mux linked to the caller. This function is primarily meant for the
  * Type-C drivers. Returns a reference to the mux on success, NULL if no
  * matching connection was found, or ERR_PTR(-EPROBE_DEFER) when a connection
  * was found but the mux has not been enumerated yet.
  */
-struct typec_mux *typec_mux_get(struct device *dev, const char *name)
+struct typec_mux *typec_mux_get(struct device *dev,
+				const struct typec_altmode_desc *desc)
 {
 	struct typec_mux *mux;
 
 	mutex_lock(&mux_lock);
-	mux = device_connection_find_match(dev, name, NULL, typec_mux_match);
+	mux = device_connection_find_match(dev, "mode-switch", (void *)desc,
+					   typec_mux_match);
 	if (!IS_ERR_OR_NULL(mux)) {
 		WARN_ON(!try_module_get(mux->dev->driver->owner));
 		get_device(mux->dev);
diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h
index 79293f630ee1..43f40685e53c 100644
--- a/include/linux/usb/typec_mux.h
+++ b/include/linux/usb/typec_mux.h
@@ -47,7 +47,8 @@ void typec_switch_put(struct typec_switch *sw);
 int typec_switch_register(struct typec_switch *sw);
 void typec_switch_unregister(struct typec_switch *sw);
 
-struct typec_mux *typec_mux_get(struct device *dev, const char *name);
+struct typec_mux *
+typec_mux_get(struct device *dev, const struct typec_altmode_desc *desc);
 void typec_mux_put(struct typec_mux *mux);
 int typec_mux_register(struct typec_mux *mux);
 void typec_mux_unregister(struct typec_mux *mux);

[PATCH v2 3/9] platform/x86: intel_cht_int33fe: Remove old style mux connections

[Heikki Krogerus]

The new mux connection naming scheme is now in use, so
dropping the connections still using the old names. From now
on the same connection description named "mode-switch" is
used with both the port and the alternate modes, so on CHT
the DP alt mode will use the same connection as the port to
get a handle to the mux device.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 295fe19ad4c2..6fa3cced6f8e 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -32,7 +32,7 @@ struct cht_int33fe_data {
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
 	/* Contain a list-head must be per device */
-	struct device_connection connections[7];
+	struct device_connection connections[4];
 };
 
 /*
@@ -174,22 +174,13 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	data->connections[0].endpoint[0] = "port0";
 	data->connections[0].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[0].id = "typec-switch";
+	data->connections[0].id = "orientation-switch";
 	data->connections[1].endpoint[0] = "port0";
 	data->connections[1].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[1].id = "typec-mux";
-	data->connections[2].endpoint[0] = "port0";
-	data->connections[2].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[2].id = "idff01m01";
-	data->connections[3].endpoint[0] = "i2c-fusb302";
-	data->connections[3].endpoint[1] = "intel_xhci_usb_sw-role-switch";
-	data->connections[3].id = "usb-role-switch";
-	data->connections[4].endpoint[0] = "port0";
-	data->connections[4].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[4].id = "orientation-switch";
-	data->connections[5].endpoint[0] = "port0";
-	data->connections[5].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[5].id = "mode-switch";
+	data->connections[1].id = "mode-switch";
+	data->connections[2].endpoint[0] = "i2c-fusb302";
+	data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
+	data->connections[2].id = "usb-role-switch";
 
 	device_connections_add(data->connections);
 
-- 
2.20.1

[v2,3/9] platform/x86: intel_cht_int33fe: Remove old style mux connections

[Heikki Krogerus]

The new mux connection naming scheme is now in use, so
dropping the connections still using the old names. From now
on the same connection description named "mode-switch" is
used with both the port and the alternate modes, so on CHT
the DP alt mode will use the same connection as the port to
get a handle to the mux device.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/x86/intel_cht_int33fe.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 295fe19ad4c2..6fa3cced6f8e 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -32,7 +32,7 @@ struct cht_int33fe_data {
 	struct i2c_client *fusb302;
 	struct i2c_client *pi3usb30532;
 	/* Contain a list-head must be per device */
-	struct device_connection connections[7];
+	struct device_connection connections[4];
 };
 
 /*
@@ -174,22 +174,13 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 
 	data->connections[0].endpoint[0] = "port0";
 	data->connections[0].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[0].id = "typec-switch";
+	data->connections[0].id = "orientation-switch";
 	data->connections[1].endpoint[0] = "port0";
 	data->connections[1].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[1].id = "typec-mux";
-	data->connections[2].endpoint[0] = "port0";
-	data->connections[2].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[2].id = "idff01m01";
-	data->connections[3].endpoint[0] = "i2c-fusb302";
-	data->connections[3].endpoint[1] = "intel_xhci_usb_sw-role-switch";
-	data->connections[3].id = "usb-role-switch";
-	data->connections[4].endpoint[0] = "port0";
-	data->connections[4].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[4].id = "orientation-switch";
-	data->connections[5].endpoint[0] = "port0";
-	data->connections[5].endpoint[1] = "i2c-pi3usb30532";
-	data->connections[5].id = "mode-switch";
+	data->connections[1].id = "mode-switch";
+	data->connections[2].endpoint[0] = "i2c-fusb302";
+	data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
+	data->connections[2].id = "usb-role-switch";
 
 	device_connections_add(data->connections);
 

[PATCH v2 4/9] device connection: Add fwnode member to struct device_connection

[Heikki Krogerus]

This will prepare the device connection API for connections
described in firmware.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/device.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 5663003a95eb..1fb077f5a936 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -757,11 +757,17 @@ struct device_dma_parameters {
 
 /**
  * struct device_connection - Device Connection Descriptor
+ * @fwnode: The device node of the connected device
  * @endpoint: The names of the two devices connected together
  * @id: Unique identifier for the connection
  * @list: List head, private, for internal use only
+ *
+ * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
+ * platform firmware defines the connection. When the connection is registered
+ * with device_connection_add() @endpoint is used instead.
  */
 struct device_connection {
+	struct fwnode_handle	*fwnode;
 	const char		*endpoint[2];
 	const char		*id;
 	struct list_head	list;
-- 
2.20.1

[v2,4/9] device connection: Add fwnode member to struct device_connection

[Heikki Krogerus]

This will prepare the device connection API for connections
described in firmware.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/device.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 5663003a95eb..1fb077f5a936 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -757,11 +757,17 @@ struct device_dma_parameters {
 
 /**
  * struct device_connection - Device Connection Descriptor
+ * @fwnode: The device node of the connected device
  * @endpoint: The names of the two devices connected together
  * @id: Unique identifier for the connection
  * @list: List head, private, for internal use only
+ *
+ * NOTE: @fwnode is not used together with @endpoint. @fwnode is used when
+ * platform firmware defines the connection. When the connection is registered
+ * with device_connection_add() @endpoint is used instead.
  */
 struct device_connection {
+	struct fwnode_handle	*fwnode;
 	const char		*endpoint[2];
 	const char		*id;
 	struct list_head	list;

[PATCH v2 5/9] usb: typec: mux: Find the muxes by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/mux.c | 86 +++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 8975f58e1d60..a5947d98824d 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -11,6 +11,8 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/slab.h>
 #include <linux/usb/typec_mux.h>
 
 static DEFINE_MUTEX(switch_lock);
@@ -23,15 +25,25 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 {
 	struct typec_switch *sw;
 
-	list_for_each_entry(sw, &switch_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
-			return sw;
+	if (!con->fwnode) {
+		list_for_each_entry(sw, &switch_list, entry)
+			if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
+				return sw;
+		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the switch to show up.
+	 * With OF graph the mux node must have a boolean device property named
+	 * "orientation-switch".
 	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	if (con->id && !fwnode_property_present(con->fwnode, con->id))
+		return NULL;
+
+	list_for_each_entry(sw, &switch_list, entry)
+		if (dev_fwnode(sw->dev) == con->fwnode)
+			return sw;
+
+	return con->id ? ERR_PTR(-EPROBE_DEFER) : NULL;
 }
 
 /**
@@ -112,17 +124,67 @@ EXPORT_SYMBOL_GPL(typec_switch_unregister);
 
 static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 {
+	const struct typec_altmode_desc *desc = data;
 	struct typec_mux *mux;
+	size_t nval;
+	bool match;
+	u16 *val;
+	int i;
 
-	list_for_each_entry(mux, &mux_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
-			return mux;
+	if (!con->fwnode) {
+		list_for_each_entry(mux, &mux_list, entry)
+			if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
+				return mux;
+		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the switch to show up.
+	 * Check has the identifier already been "consumed". If it
+	 * has, no need to do any extra connection identification.
 	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	match = !con->id;
+	if (match)
+		goto find_mux;
+
+	/* Accessory Mode muxes */
+	if (!desc) {
+		match = fwnode_property_present(con->fwnode, "accessory");
+		if (match)
+			goto find_mux;
+		return NULL;
+	}
+
+	/* Alternate Mode muxes */
+	nval = fwnode_property_read_u16_array(con->fwnode, "svid", NULL, 0);
+	if (nval <= 0)
+		return NULL;
+
+	val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
+	if (!val)
+		return ERR_PTR(-ENOMEM);
+
+	nval = fwnode_property_read_u16_array(con->fwnode, "svid", val, nval);
+	if (nval < 0) {
+		kfree(val);
+		return ERR_PTR(nval);
+	}
+
+	for (i = 0; i < nval; i++) {
+		match = val[i] == desc->svid;
+		if (match) {
+			kfree(val);
+			goto find_mux;
+		}
+	}
+	kfree(val);
+	return NULL;
+
+find_mux:
+	list_for_each_entry(mux, &mux_list, entry)
+		if (dev_fwnode(mux->dev) == con->fwnode)
+			return mux;
+
+	return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
 }
 
 /**
-- 
2.20.1

[v2,5/9] usb: typec: mux: Find the muxes by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/mux.c | 86 +++++++++++++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 8975f58e1d60..a5947d98824d 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -11,6 +11,8 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/slab.h>
 #include <linux/usb/typec_mux.h>
 
 static DEFINE_MUTEX(switch_lock);
@@ -23,15 +25,25 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 {
 	struct typec_switch *sw;
 
-	list_for_each_entry(sw, &switch_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
-			return sw;
+	if (!con->fwnode) {
+		list_for_each_entry(sw, &switch_list, entry)
+			if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
+				return sw;
+		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the switch to show up.
+	 * With OF graph the mux node must have a boolean device property named
+	 * "orientation-switch".
 	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	if (con->id && !fwnode_property_present(con->fwnode, con->id))
+		return NULL;
+
+	list_for_each_entry(sw, &switch_list, entry)
+		if (dev_fwnode(sw->dev) == con->fwnode)
+			return sw;
+
+	return con->id ? ERR_PTR(-EPROBE_DEFER) : NULL;
 }
 
 /**
@@ -112,17 +124,67 @@ EXPORT_SYMBOL_GPL(typec_switch_unregister);
 
 static void *typec_mux_match(struct device_connection *con, int ep, void *data)
 {
+	const struct typec_altmode_desc *desc = data;
 	struct typec_mux *mux;
+	size_t nval;
+	bool match;
+	u16 *val;
+	int i;
 
-	list_for_each_entry(mux, &mux_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
-			return mux;
+	if (!con->fwnode) {
+		list_for_each_entry(mux, &mux_list, entry)
+			if (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
+				return mux;
+		return ERR_PTR(-EPROBE_DEFER);
+	}
 
 	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the switch to show up.
+	 * Check has the identifier already been "consumed". If it
+	 * has, no need to do any extra connection identification.
 	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	match = !con->id;
+	if (match)
+		goto find_mux;
+
+	/* Accessory Mode muxes */
+	if (!desc) {
+		match = fwnode_property_present(con->fwnode, "accessory");
+		if (match)
+			goto find_mux;
+		return NULL;
+	}
+
+	/* Alternate Mode muxes */
+	nval = fwnode_property_read_u16_array(con->fwnode, "svid", NULL, 0);
+	if (nval <= 0)
+		return NULL;
+
+	val = kcalloc(nval, sizeof(*val), GFP_KERNEL);
+	if (!val)
+		return ERR_PTR(-ENOMEM);
+
+	nval = fwnode_property_read_u16_array(con->fwnode, "svid", val, nval);
+	if (nval < 0) {
+		kfree(val);
+		return ERR_PTR(nval);
+	}
+
+	for (i = 0; i < nval; i++) {
+		match = val[i] == desc->svid;
+		if (match) {
+			kfree(val);
+			goto find_mux;
+		}
+	}
+	kfree(val);
+	return NULL;
+
+find_mux:
+	list_for_each_entry(mux, &mux_list, entry)
+		if (dev_fwnode(mux->dev) == con->fwnode)
+			return mux;
+
+	return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
 }
 
 /**

[PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

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

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 99116af07f1d..f45d8df5cfb8 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/usb/role.h>
+#include <linux/property.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
 
-static int __switch_match(struct device *dev, const void *name)
+static int switch_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static int switch_name_match(struct device *dev, const void *name)
 {
 	return !strcmp((const char *)name, dev_name(dev));
 }
@@ -94,8 +100,16 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
 {
 	struct device *dev;
 
-	dev = class_find_device(role_class, NULL, con->endpoint[ep],
-				__switch_match);
+	if (con->fwnode) {
+		if (!fwnode_property_present(con->fwnode, con->id))
+			return NULL;
+
+		dev = class_find_device(role_class, NULL, con->fwnode,
+					switch_fwnode_match);
+	} else {
+		dev = class_find_device(role_class, NULL, con->endpoint[ep],
+					switch_name_match);
+	}
 
 	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
@@ -266,6 +280,7 @@ usb_role_switch_register(struct device *parent,
 	sw->get = desc->get;
 
 	sw->dev.parent = parent;
+	sw->dev.fwnode = desc->fwnode;
 	sw->dev.class = role_class;
 	sw->dev.type = &usb_role_dev_type;
 	dev_set_name(&sw->dev, "%s-role-switch", dev_name(parent));
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index edc51be4a77c..9684a8734757 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
  * usb_role_switch_register() before registering the switch.
  */
 struct usb_role_switch_desc {
+	struct fwnode_handle *fwnode;
 	struct device *usb2_port;
 	struct device *usb3_port;
 	struct device *udc;
-- 
2.20.1

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

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

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index 99116af07f1d..f45d8df5cfb8 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/usb/role.h>
+#include <linux/property.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
 
-static int __switch_match(struct device *dev, const void *name)
+static int switch_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static int switch_name_match(struct device *dev, const void *name)
 {
 	return !strcmp((const char *)name, dev_name(dev));
 }
@@ -94,8 +100,16 @@ static void *usb_role_switch_match(struct device_connection *con, int ep,
 {
 	struct device *dev;
 
-	dev = class_find_device(role_class, NULL, con->endpoint[ep],
-				__switch_match);
+	if (con->fwnode) {
+		if (!fwnode_property_present(con->fwnode, con->id))
+			return NULL;
+
+		dev = class_find_device(role_class, NULL, con->fwnode,
+					switch_fwnode_match);
+	} else {
+		dev = class_find_device(role_class, NULL, con->endpoint[ep],
+					switch_name_match);
+	}
 
 	return dev ? to_role_switch(dev) : ERR_PTR(-EPROBE_DEFER);
 }
@@ -266,6 +280,7 @@ usb_role_switch_register(struct device *parent,
 	sw->get = desc->get;
 
 	sw->dev.parent = parent;
+	sw->dev.fwnode = desc->fwnode;
 	sw->dev.class = role_class;
 	sw->dev.type = &usb_role_dev_type;
 	dev_set_name(&sw->dev, "%s-role-switch", dev_name(parent));
diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
index edc51be4a77c..9684a8734757 100644
--- a/include/linux/usb/role.h
+++ b/include/linux/usb/role.h
@@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct device *dev);
  * usb_role_switch_register() before registering the switch.
  */
 struct usb_role_switch_desc {
+	struct fwnode_handle *fwnode;
 	struct device *usb2_port;
 	struct device *usb3_port;
 	struct device *udc;

RE: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

Hi Heikki,

> @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> usb_role_switch *sw)  }  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> 
> -static int __switch_match(struct device *dev, const void *name)
> +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> +{
> +	return dev_fwnode(dev) == fwnode;

You missed the comment
https://lkml.org/lkml/2019/1/22/437

return dev_fwnode(dev->parent) == fwnode;

Jun

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

Hi Heikki,

> @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> usb_role_switch *sw)  }  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> 
> -static int __switch_match(struct device *dev, const void *name)
> +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> +{
> +	return dev_fwnode(dev) == fwnode;

You missed the comment
https://lkml.org/lkml/2019/1/22/437

return dev_fwnode(dev->parent) == fwnode;

Jun

Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

On Mon, Feb 11, 2019 at 09:58:04AM +0000, Jun Li wrote:
> Hi Heikki,
> 
> > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > usb_role_switch *sw)  }  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > 
> > -static int __switch_match(struct device *dev, const void *name)
> > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > +{
> > +	return dev_fwnode(dev) == fwnode;
> 
> You missed the comment
> https://lkml.org/lkml/2019/1/22/437
> 
> return dev_fwnode(dev->parent) == fwnode;

That's actually not the case. struct usb_role_switch_desc has a member
for fwnode, and that's what we use with the actual mux device. Check
usb_role_switch_register():

        ...
        sw->dev.fwnode = desc->fwnode;
        ...

Sorry for not realizing it before.


thanks,

-- 
heikki

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

On Mon, Feb 11, 2019 at 09:58:04AM +0000, Jun Li wrote:
> Hi Heikki,
> 
> > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > usb_role_switch *sw)  }  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > 
> > -static int __switch_match(struct device *dev, const void *name)
> > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > +{
> > +	return dev_fwnode(dev) == fwnode;
> 
> You missed the comment
> https://lkml.org/lkml/2019/1/22/437
> 
> return dev_fwnode(dev->parent) == fwnode;

That's actually not the case. struct usb_role_switch_desc has a member
for fwnode, and that's what we use with the actual mux device. Check
usb_role_switch_register():

        ...
        sw->dev.fwnode = desc->fwnode;
        ...

Sorry for not realizing it before.


thanks,

Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

On Mon, Feb 11, 2019 at 12:46:29PM +0200, Heikki Krogerus wrote:
> On Mon, Feb 11, 2019 at 09:58:04AM +0000, Jun Li wrote:
> > Hi Heikki,
> > 
> > > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > > usb_role_switch *sw)  }  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > > 
> > > -static int __switch_match(struct device *dev, const void *name)
> > > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > > +{
> > > +	return dev_fwnode(dev) == fwnode;
> > 
> > You missed the comment
> > https://lkml.org/lkml/2019/1/22/437
> > 
> > return dev_fwnode(dev->parent) == fwnode;
> 
> That's actually not the case. struct usb_role_switch_desc has a member
> for fwnode, and that's what we use with the actual mux device. Check
> usb_role_switch_register():
> 
>         ...
>         sw->dev.fwnode = desc->fwnode;
>         ...
> 
> Sorry for not realizing it before.

Just to clarify. The current patch is OK. No changes needed.


thanks,

-- 
heikki

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

On Mon, Feb 11, 2019 at 12:46:29PM +0200, Heikki Krogerus wrote:
> On Mon, Feb 11, 2019 at 09:58:04AM +0000, Jun Li wrote:
> > Hi Heikki,
> > 
> > > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > > usb_role_switch *sw)  }  EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > > 
> > > -static int __switch_match(struct device *dev, const void *name)
> > > +static int switch_fwnode_match(struct device *dev, const void *fwnode)
> > > +{
> > > +	return dev_fwnode(dev) == fwnode;
> > 
> > You missed the comment
> > https://lkml.org/lkml/2019/1/22/437
> > 
> > return dev_fwnode(dev->parent) == fwnode;
> 
> That's actually not the case. struct usb_role_switch_desc has a member
> for fwnode, and that's what we use with the actual mux device. Check
> usb_role_switch_register():
> 
>         ...
>         sw->dev.fwnode = desc->fwnode;
>         ...
> 
> Sorry for not realizing it before.

Just to clarify. The current patch is OK. No changes needed.


thanks,

RE: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年2月11日 18:46
> To: Jun Li <jun.li@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Chen Yu <chenyu56@huawei.com>; Hans de
> Goede <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the
> device node
> 
> On Mon, Feb 11, 2019 at 09:58:04AM +0000, Jun Li wrote:
> > Hi Heikki,
> >
> > > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > > usb_role_switch *sw)  }
> > > EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > >
> > > -static int __switch_match(struct device *dev, const void *name)
> > > +static int switch_fwnode_match(struct device *dev, const void
> > > +*fwnode) {
> > > +	return dev_fwnode(dev) == fwnode;
> >
> > You missed the comment
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >
> l.org%2Flkml%2F2019%2F1%2F22%2F437&amp;data=02%7C01%7Cjun.li%40nx
> p.com
> > %7C8c2d40d5e5d246da34ad08d6900e31cf%7C686ea1d3bc2b4c6fa92cd99c5
> c301635
> > %7C0%7C0%7C636854788040965224&amp;sdata=db4gvXKc9InWiltsweetxXYr
> tPbtfX
> > jshPh%2FnvA24ig%3D&amp;reserved=0
> >
> > return dev_fwnode(dev->parent) == fwnode;
> 
> That's actually not the case. struct usb_role_switch_desc has a member for fwnode,
> and that's what we use with the actual mux device. Check
> usb_role_switch_register():
> 
>         ...
>         sw->dev.fwnode = desc->fwnode;
>         ...
> 
> Sorry for not realizing it before.

So desc->fwnode should be initialized before do usb_role_switch_register()?
But seems usb_role_switch_desc is a read-only object so can't be set at runtime.

usb_controller_node {
	...
	usb-role-switch;

	port {
		sw_provider_node: endpoint {
			remote-endpoint = <&sw_consumer_node>;
		};
	};
};

typec_node {
	...
	port {
		sw_consumer_node: endpoint {
			remote-endpoint = <&sw_provider_node>;
		};
	};
};

Is my understanding correct?

Thanks
Jun
> 
> 
> thanks,
> 
> --
> heikki

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年2月11日 18:46
> To: Jun Li <jun.li@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Chen Yu <chenyu56@huawei.com>; Hans de
> Goede <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the
> device node
> 
> On Mon, Feb 11, 2019 at 09:58:04AM +0000, Jun Li wrote:
> > Hi Heikki,
> >
> > > @@ -84,7 +85,12 @@ enum usb_role usb_role_switch_get_role(struct
> > > usb_role_switch *sw)  }
> > > EXPORT_SYMBOL_GPL(usb_role_switch_get_role);
> > >
> > > -static int __switch_match(struct device *dev, const void *name)
> > > +static int switch_fwnode_match(struct device *dev, const void
> > > +*fwnode) {
> > > +	return dev_fwnode(dev) == fwnode;
> >
> > You missed the comment
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >
> l.org%2Flkml%2F2019%2F1%2F22%2F437&amp;data=02%7C01%7Cjun.li%40nx
> p.com
> > %7C8c2d40d5e5d246da34ad08d6900e31cf%7C686ea1d3bc2b4c6fa92cd99c5
> c301635
> > %7C0%7C0%7C636854788040965224&amp;sdata=db4gvXKc9InWiltsweetxXYr
> tPbtfX
> > jshPh%2FnvA24ig%3D&amp;reserved=0
> >
> > return dev_fwnode(dev->parent) == fwnode;
> 
> That's actually not the case. struct usb_role_switch_desc has a member for fwnode,
> and that's what we use with the actual mux device. Check
> usb_role_switch_register():
> 
>         ...
>         sw->dev.fwnode = desc->fwnode;
>         ...
> 
> Sorry for not realizing it before.

So desc->fwnode should be initialized before do usb_role_switch_register()?
But seems usb_role_switch_desc is a read-only object so can't be set at runtime.

usb_controller_node {
	...
	usb-role-switch;

	port {
		sw_provider_node: endpoint {
			remote-endpoint = <&sw_consumer_node>;
		};
	};
};

typec_node {
	...
	port {
		sw_consumer_node: endpoint {
			remote-endpoint = <&sw_provider_node>;
		};
	};
};

Is my understanding correct?

Thanks
Jun
> 
> 
> thanks,
> 
> --
> heikki

Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

Hi,

On Tue, Feb 12, 2019 at 06:03:42AM +0000, Jun Li wrote:
> > > return dev_fwnode(dev->parent) == fwnode;
> > 
> > That's actually not the case. struct usb_role_switch_desc has a member for fwnode,
> > and that's what we use with the actual mux device. Check
> > usb_role_switch_register():
> > 
> >         ...
> >         sw->dev.fwnode = desc->fwnode;
> >         ...
> > 
> > Sorry for not realizing it before.
> 
> So desc->fwnode should be initialized before do usb_role_switch_register()?
> But seems usb_role_switch_desc is a read-only object so can't be set at runtime.

It can. Even though usb_role_switch_register() takes read-only
parameter, nothing's preventing you from passing data even from the
stack (the content of the descriptor is copied in any case).

Expecting the descriptor to be read-only just means it can be
read-only, but it does not have to be.

> usb_controller_node {
> 	...
> 	usb-role-switch;
> 
> 	port {
> 		sw_provider_node: endpoint {
> 			remote-endpoint = <&sw_consumer_node>;
> 		};
> 	};
> };
> 
> typec_node {
> 	...
> 	port {
> 		sw_consumer_node: endpoint {
> 			remote-endpoint = <&sw_provider_node>;
> 		};
> 	};
> };

That looks roughly correct to me.


thanks,

-- 
heikki

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

Hi,

On Tue, Feb 12, 2019 at 06:03:42AM +0000, Jun Li wrote:
> > > return dev_fwnode(dev->parent) == fwnode;
> > 
> > That's actually not the case. struct usb_role_switch_desc has a member for fwnode,
> > and that's what we use with the actual mux device. Check
> > usb_role_switch_register():
> > 
> >         ...
> >         sw->dev.fwnode = desc->fwnode;
> >         ...
> > 
> > Sorry for not realizing it before.
> 
> So desc->fwnode should be initialized before do usb_role_switch_register()?
> But seems usb_role_switch_desc is a read-only object so can't be set at runtime.

It can. Even though usb_role_switch_register() takes read-only
parameter, nothing's preventing you from passing data even from the
stack (the content of the descriptor is copied in any case).

Expecting the descriptor to be read-only just means it can be
read-only, but it does not have to be.

> usb_controller_node {
> 	...
> 	usb-role-switch;
> 
> 	port {
> 		sw_provider_node: endpoint {
> 			remote-endpoint = <&sw_consumer_node>;
> 		};
> 	};
> };
> 
> typec_node {
> 	...
> 	port {
> 		sw_consumer_node: endpoint {
> 			remote-endpoint = <&sw_provider_node>;
> 		};
> 	};
> };

That looks roughly correct to me.


thanks,

RE: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

Hi
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年2月12日 16:51
> To: Jun Li <jun.li@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Chen Yu <chenyu56@huawei.com>; Hans de
> Goede <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the
> device node
> 
> Hi,
> 
> On Tue, Feb 12, 2019 at 06:03:42AM +0000, Jun Li wrote:
> > > > return dev_fwnode(dev->parent) == fwnode;
> > >
> > > That's actually not the case. struct usb_role_switch_desc has a
> > > member for fwnode, and that's what we use with the actual mux
> > > device. Check
> > > usb_role_switch_register():
> > >
> > >         ...
> > >         sw->dev.fwnode = desc->fwnode;
> > >         ...
> > >
> > > Sorry for not realizing it before.
> >
> > So desc->fwnode should be initialized before do usb_role_switch_register()?
> > But seems usb_role_switch_desc is a read-only object so can't be set at runtime.
> 
> It can. Even though usb_role_switch_register() takes read-only parameter, nothing's
> preventing you from passing data even from the stack (the content of the descriptor
> is copied in any case).
> 
> Expecting the descriptor to be read-only just means it can be read-only, but it does
> not have to be.

Understood, thanks.

> @@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct
> device *dev);
>   * usb_role_switch_register() before registering the switch.
>   */
>  struct usb_role_switch_desc {
> +	struct fwnode_handle *fwnode;
You may add some description for this new member
/**
 * struct usb_role_switch_desc - USB Role Switch Descriptor
 * @ fwnode 

> 
> > usb_controller_node {
> > 	...
> > 	usb-role-switch;
> >
> > 	port {
> > 		sw_provider_node: endpoint {
> > 			remote-endpoint = <&sw_consumer_node>;
> > 		};
> > 	};
> > };
> >
> > typec_node {
> > 	...
> > 	port {
> > 		sw_consumer_node: endpoint {
> > 			remote-endpoint = <&sw_provider_node>;
> > 		};
> > 	};
> > };
> 
> That looks roughly correct to me.
> 
> 
> thanks,
> 
> --
> heikki

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Jun Li]

Hi
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年2月12日 16:51
> To: Jun Li <jun.li@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Chen Yu <chenyu56@huawei.com>; Hans de
> Goede <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the
> device node
> 
> Hi,
> 
> On Tue, Feb 12, 2019 at 06:03:42AM +0000, Jun Li wrote:
> > > > return dev_fwnode(dev->parent) == fwnode;
> > >
> > > That's actually not the case. struct usb_role_switch_desc has a
> > > member for fwnode, and that's what we use with the actual mux
> > > device. Check
> > > usb_role_switch_register():
> > >
> > >         ...
> > >         sw->dev.fwnode = desc->fwnode;
> > >         ...
> > >
> > > Sorry for not realizing it before.
> >
> > So desc->fwnode should be initialized before do usb_role_switch_register()?
> > But seems usb_role_switch_desc is a read-only object so can't be set at runtime.
> 
> It can. Even though usb_role_switch_register() takes read-only parameter, nothing's
> preventing you from passing data even from the stack (the content of the descriptor
> is copied in any case).
> 
> Expecting the descriptor to be read-only just means it can be read-only, but it does
> not have to be.

Understood, thanks.

> @@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct
> device *dev);
>   * usb_role_switch_register() before registering the switch.
>   */
>  struct usb_role_switch_desc {
> +	struct fwnode_handle *fwnode;
You may add some description for this new member
/**
 * struct usb_role_switch_desc - USB Role Switch Descriptor
 * @ fwnode 

> 
> > usb_controller_node {
> > 	...
> > 	usb-role-switch;
> >
> > 	port {
> > 		sw_provider_node: endpoint {
> > 			remote-endpoint = <&sw_consumer_node>;
> > 		};
> > 	};
> > };
> >
> > typec_node {
> > 	...
> > 	port {
> > 		sw_consumer_node: endpoint {
> > 			remote-endpoint = <&sw_provider_node>;
> > 		};
> > 	};
> > };
> 
> That looks roughly correct to me.
> 
> 
> thanks,
> 
> --
> heikki

Re: [PATCH v2 6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

Hi,

On Tue, Feb 12, 2019 at 10:41:28AM +0000, Jun Li wrote:
> > @@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct
> > device *dev);
> >   * usb_role_switch_register() before registering the switch.
> >   */
> >  struct usb_role_switch_desc {
> > +	struct fwnode_handle *fwnode;
> You may add some description for this new member
> /**
>  * struct usb_role_switch_desc - USB Role Switch Descriptor
>  * @ fwnode 

You are correct. I need to fix that one.

thanks,

-- 
heikki

[v2,6/9] usb: roles: Find the muxes by also matching against the device node

[Heikki Krogerus]

Hi,

On Tue, Feb 12, 2019 at 10:41:28AM +0000, Jun Li wrote:
> > @@ -32,6 +32,7 @@ typedef enum usb_role (*usb_role_switch_get_t)(struct
> > device *dev);
> >   * usb_role_switch_register() before registering the switch.
> >   */
> >  struct usb_role_switch_desc {
> > +	struct fwnode_handle *fwnode;
> You may add some description for this new member
> /**
>  * struct usb_role_switch_desc - USB Role Switch Descriptor
>  * @ fwnode 

You are correct. I need to fix that one.

thanks,

[PATCH v2 7/9] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index f50278dbef59..88adf0eaad34 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 #include "bus.h"
@@ -204,15 +205,32 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 	put_device(&adev->dev);
 }
 
-static int __typec_port_match(struct device *dev, const void *name)
+static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static int typec_port_name_match(struct device *dev, const void *name)
 {
 	return !strcmp((const char *)name, dev_name(dev));
 }
 
 static void *typec_port_match(struct device_connection *con, int ep, void *data)
 {
-	return class_find_device(typec_class, NULL, con->endpoint[ep],
-				 __typec_port_match);
+	struct device *dev;
+
+	/*
+	 * FIXME: Check does the fwnode supports the requested SVID. If it does
+	 * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
+	 */
+	if (con->fwnode)
+		return class_find_device(typec_class, NULL, con->fwnode,
+					 typec_port_fwnode_match);
+
+	dev = class_find_device(typec_class, NULL, con->endpoint[ep],
+				typec_port_name_match);
+
+	return dev ? dev : ERR_PTR(-EPROBE_DEFER);
 }
 
 struct typec_altmode *
-- 
2.20.1

[v2,7/9] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device, and the endpoint will not be used at all in that
case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index f50278dbef59..88adf0eaad34 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 #include "bus.h"
@@ -204,15 +205,32 @@ static void typec_altmode_put_partner(struct altmode *altmode)
 	put_device(&adev->dev);
 }
 
-static int __typec_port_match(struct device *dev, const void *name)
+static int typec_port_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static int typec_port_name_match(struct device *dev, const void *name)
 {
 	return !strcmp((const char *)name, dev_name(dev));
 }
 
 static void *typec_port_match(struct device_connection *con, int ep, void *data)
 {
-	return class_find_device(typec_class, NULL, con->endpoint[ep],
-				 __typec_port_match);
+	struct device *dev;
+
+	/*
+	 * FIXME: Check does the fwnode supports the requested SVID. If it does
+	 * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
+	 */
+	if (con->fwnode)
+		return class_find_device(typec_class, NULL, con->fwnode,
+					 typec_port_fwnode_match);
+
+	dev = class_find_device(typec_class, NULL, con->endpoint[ep],
+				typec_port_name_match);
+
+	return dev ? dev : ERR_PTR(-EPROBE_DEFER);
 }
 
 struct typec_altmode *

Re: [PATCH v2 7/9] usb: typec: Find the ports by also matching against the device node

[Andy Shevchenko]

On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> When the connections are defined in firmware, struct
> device_connection will have the fwnode member pointing to
> the device node (struct fwnode_handle) of the requested
> device, and the endpoint will not be used at all in that
> case.

> +       /*
> +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> +        */
> +       if (con->fwnode)
> +               return class_find_device(typec_class, NULL, con->fwnode,
> +                                        typec_port_fwnode_match);
> +
> +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> +                               typec_port_name_match);
> +
> +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);

Just  to be clear, this one takes a reference on dev. Is it taken into account?

-- 
With Best Regards,
Andy Shevchenko

[v2,7/9] usb: typec: Find the ports by also matching against the device node

[Andy Shevchenko]

On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> When the connections are defined in firmware, struct
> device_connection will have the fwnode member pointing to
> the device node (struct fwnode_handle) of the requested
> device, and the endpoint will not be used at all in that
> case.

> +       /*
> +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> +        */
> +       if (con->fwnode)
> +               return class_find_device(typec_class, NULL, con->fwnode,
> +                                        typec_port_fwnode_match);
> +
> +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> +                               typec_port_name_match);
> +
> +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);

Just  to be clear, this one takes a reference on dev. Is it taken into account?

Re: [PATCH v2 7/9] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > When the connections are defined in firmware, struct
> > device_connection will have the fwnode member pointing to
> > the device node (struct fwnode_handle) of the requested
> > device, and the endpoint will not be used at all in that
> > case.
> 
> > +       /*
> > +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> > +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> > +        */
> > +       if (con->fwnode)
> > +               return class_find_device(typec_class, NULL, con->fwnode,
> > +                                        typec_port_fwnode_match);
> > +
> > +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> > +                               typec_port_name_match);
> > +
> > +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);
> 
> Just  to be clear, this one takes a reference on dev. Is it taken into account?

Yes. That is what we want it to do.

thanks,

-- 
heikki

[v2,7/9] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > When the connections are defined in firmware, struct
> > device_connection will have the fwnode member pointing to
> > the device node (struct fwnode_handle) of the requested
> > device, and the endpoint will not be used at all in that
> > case.
> 
> > +       /*
> > +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> > +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> > +        */
> > +       if (con->fwnode)
> > +               return class_find_device(typec_class, NULL, con->fwnode,
> > +                                        typec_port_fwnode_match);
> > +
> > +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> > +                               typec_port_name_match);
> > +
> > +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);
> 
> Just  to be clear, this one takes a reference on dev. Is it taken into account?

Yes. That is what we want it to do.

thanks,

Re: [PATCH v2 7/9] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

Hi Andy,

On Thu, Jan 31, 2019 at 03:35:37PM +0200, Heikki Krogerus wrote:
> On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > When the connections are defined in firmware, struct
> > > device_connection will have the fwnode member pointing to
> > > the device node (struct fwnode_handle) of the requested
> > > device, and the endpoint will not be used at all in that
> > > case.
> > 
> > > +       /*
> > > +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> > > +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> > > +        */
> > > +       if (con->fwnode)
> > > +               return class_find_device(typec_class, NULL, con->fwnode,
> > > +                                        typec_port_fwnode_match);
> > > +
> > > +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> > > +                               typec_port_name_match);
> > > +
> > > +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);
> > 
> > Just  to be clear, this one takes a reference on dev. Is it taken into account?
> 
> Yes. That is what we want it to do.

Gentle ping. Is the series OK?

thanks,

-- 
heikki

[v2,7/9] usb: typec: Find the ports by also matching against the device node

[Heikki Krogerus]

Hi Andy,

On Thu, Jan 31, 2019 at 03:35:37PM +0200, Heikki Krogerus wrote:
> On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > When the connections are defined in firmware, struct
> > > device_connection will have the fwnode member pointing to
> > > the device node (struct fwnode_handle) of the requested
> > > device, and the endpoint will not be used at all in that
> > > case.
> > 
> > > +       /*
> > > +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> > > +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> > > +        */
> > > +       if (con->fwnode)
> > > +               return class_find_device(typec_class, NULL, con->fwnode,
> > > +                                        typec_port_fwnode_match);
> > > +
> > > +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> > > +                               typec_port_name_match);
> > > +
> > > +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);
> > 
> > Just  to be clear, this one takes a reference on dev. Is it taken into account?
> 
> Yes. That is what we want it to do.

Gentle ping. Is the series OK?

thanks,

Re: [PATCH v2 7/9] usb: typec: Find the ports by also matching against the device node

[Andy Shevchenko]

On Mon, Feb 11, 2019 at 10:39 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Thu, Jan 31, 2019 at 03:35:37PM +0200, Heikki Krogerus wrote:
> > On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> > > On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > When the connections are defined in firmware, struct
> > > > device_connection will have the fwnode member pointing to
> > > > the device node (struct fwnode_handle) of the requested
> > > > device, and the endpoint will not be used at all in that
> > > > case.
> > >
> > > > +       /*
> > > > +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> > > > +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> > > > +        */
> > > > +       if (con->fwnode)
> > > > +               return class_find_device(typec_class, NULL, con->fwnode,
> > > > +                                        typec_port_fwnode_match);
> > > > +
> > > > +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> > > > +                               typec_port_name_match);
> > > > +
> > > > +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);
> > >
> > > Just  to be clear, this one takes a reference on dev. Is it taken into account?
> >
> > Yes. That is what we want it to do.
>
> Gentle ping. Is the series OK?

From my prospective it's okay, so, FWIW
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


-- 
With Best Regards,
Andy Shevchenko

[v2,7/9] usb: typec: Find the ports by also matching against the device node

[Andy Shevchenko]

On Mon, Feb 11, 2019 at 10:39 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Thu, Jan 31, 2019 at 03:35:37PM +0200, Heikki Krogerus wrote:
> > On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> > > On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > When the connections are defined in firmware, struct
> > > > device_connection will have the fwnode member pointing to
> > > > the device node (struct fwnode_handle) of the requested
> > > > device, and the endpoint will not be used at all in that
> > > > case.
> > >
> > > > +       /*
> > > > +        * FIXME: Check does the fwnode supports the requested SVID. If it does
> > > > +        * we need to return ERR_PTR(-PROBE_DEFER) when there is no device.
> > > > +        */
> > > > +       if (con->fwnode)
> > > > +               return class_find_device(typec_class, NULL, con->fwnode,
> > > > +                                        typec_port_fwnode_match);
> > > > +
> > > > +       dev = class_find_device(typec_class, NULL, con->endpoint[ep],
> > > > +                               typec_port_name_match);
> > > > +
> > > > +       return dev ? dev : ERR_PTR(-EPROBE_DEFER);
> > >
> > > Just  to be clear, this one takes a reference on dev. Is it taken into account?
> >
> > Yes. That is what we want it to do.
>
> Gentle ping. Is the series OK?

From my prospective it's okay, so, FWIW
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

[PATCH v2 8/9] device connection: Prepare support for firmware described connections

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device. The endpoint member for the device names will not be
used at all in that case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/devcon.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index d427e806cd73..858b8d2f6ef8 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -75,12 +75,36 @@ static struct bus_type *generic_match_buses[] = {
 	NULL,
 };
 
+static int device_fwnode_match(struct device *dev, void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static void *device_connection_fwnode_match(struct device_connection *con)
+{
+	struct bus_type *bus;
+	struct device *dev;
+
+	for (bus = generic_match_buses[0]; bus; bus++) {
+		dev = bus_find_device(bus, NULL, (void *)con->fwnode,
+				      device_fwnode_match);
+		if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
+			return dev;
+
+		put_device(dev);
+	}
+	return NULL;
+}
+
 /* This tries to find the device from the most common bus types by name. */
 static void *generic_match(struct device_connection *con, int ep, void *data)
 {
 	struct bus_type *bus;
 	struct device *dev;
 
+	if (con->fwnode)
+		return device_connection_fwnode_match(con);
+
 	for (bus = generic_match_buses[0]; bus; bus++) {
 		dev = bus_find_device_by_name(bus, NULL, con->endpoint[ep]);
 		if (dev)
-- 
2.20.1

[v2,8/9] device connection: Prepare support for firmware described connections

[Heikki Krogerus]

When the connections are defined in firmware, struct
device_connection will have the fwnode member pointing to
the device node (struct fwnode_handle) of the requested
device. The endpoint member for the device names will not be
used at all in that case.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/devcon.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index d427e806cd73..858b8d2f6ef8 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -75,12 +75,36 @@ static struct bus_type *generic_match_buses[] = {
 	NULL,
 };
 
+static int device_fwnode_match(struct device *dev, void *fwnode)
+{
+	return dev_fwnode(dev) == fwnode;
+}
+
+static void *device_connection_fwnode_match(struct device_connection *con)
+{
+	struct bus_type *bus;
+	struct device *dev;
+
+	for (bus = generic_match_buses[0]; bus; bus++) {
+		dev = bus_find_device(bus, NULL, (void *)con->fwnode,
+				      device_fwnode_match);
+		if (dev && !strncmp(dev_name(dev), con->id, strlen(con->id)))
+			return dev;
+
+		put_device(dev);
+	}
+	return NULL;
+}
+
 /* This tries to find the device from the most common bus types by name. */
 static void *generic_match(struct device_connection *con, int ep, void *data)
 {
 	struct bus_type *bus;
 	struct device *dev;
 
+	if (con->fwnode)
+		return device_connection_fwnode_match(con);
+
 	for (bus = generic_match_buses[0]; bus; bus++) {
 		dev = bus_find_device_by_name(bus, NULL, con->endpoint[ep]);
 		if (dev)

[PATCH v2 9/9] device connection: Find device connections also from device graphs

[Heikki Krogerus]

If connections between devices are described in OF graph or
ACPI device graph, we can find them by using the
fwnode_graph_*() functions.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/devcon.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 858b8d2f6ef8..04db9ae235e4 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,10 +7,37 @@
  */
 
 #include <linux/device.h>
+#include <linux/property.h>
 
 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)
+{
+	struct device_connection con = { .id = con_id };
+	struct fwnode_handle *ep;
+	void *ret;
+
+	fwnode_graph_for_each_endpoint(fwnode, ep) {
+		con.fwnode = fwnode_graph_get_remote_port_parent(ep);
+		if (!fwnode_device_is_available(con.fwnode))
+			continue;
+
+		ret = match(&con, -1, data);
+		fwnode_handle_put(con.fwnode);
+		if (ret) {
+			fwnode_handle_put(ep);
+			return ret;
+		}
+	}
+	return NULL;
+}
+
 /**
  * device_connection_find_match - Find physical connection to a device
  * @dev: Device with the connection
@@ -23,10 +50,9 @@ static LIST_HEAD(devcon_list);
  * caller is expecting to be returned.
  */
 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 fwnode_handle *fwnode = dev_fwnode(dev);
 	const char *devname = dev_name(dev);
 	struct device_connection *con;
 	void *ret = NULL;
@@ -35,6 +61,12 @@ 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;
+	}
+
 	mutex_lock(&devcon_lock);
 
 	list_for_each_entry(con, &devcon_list, list) {
-- 
2.20.1

[v2,9/9] device connection: Find device connections also from device graphs

[Heikki Krogerus]

If connections between devices are described in OF graph or
ACPI device graph, we can find them by using the
fwnode_graph_*() functions.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/devcon.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 858b8d2f6ef8..04db9ae235e4 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,10 +7,37 @@
  */
 
 #include <linux/device.h>
+#include <linux/property.h>
 
 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)
+{
+	struct device_connection con = { .id = con_id };
+	struct fwnode_handle *ep;
+	void *ret;
+
+	fwnode_graph_for_each_endpoint(fwnode, ep) {
+		con.fwnode = fwnode_graph_get_remote_port_parent(ep);
+		if (!fwnode_device_is_available(con.fwnode))
+			continue;
+
+		ret = match(&con, -1, data);
+		fwnode_handle_put(con.fwnode);
+		if (ret) {
+			fwnode_handle_put(ep);
+			return ret;
+		}
+	}
+	return NULL;
+}
+
 /**
  * device_connection_find_match - Find physical connection to a device
  * @dev: Device with the connection
@@ -23,10 +50,9 @@ static LIST_HEAD(devcon_list);
  * caller is expecting to be returned.
  */
 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 fwnode_handle *fwnode = dev_fwnode(dev);
 	const char *devname = dev_name(dev);
 	struct device_connection *con;
 	void *ret = NULL;
@@ -35,6 +61,12 @@ 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;
+	}
+
 	mutex_lock(&devcon_lock);
 
 	list_for_each_entry(con, &devcon_list, list) {

Re: [PATCH v2 0/9] device connection: Add support for device graphs

[Hans de Goede]

Hi,

On 30-01-19 17:02, Heikki Krogerus wrote:
> Hi,
> 
> This is the second version of this series. On top the two code style
> improvements requested by Andy, I also renamed the connection
> identifiers used with the USB Type-C muxes for something that I felt
> are better, especially after we start using them to name reference
> device properties in fwnodes. That's why the first patch is now split
> in two, 1/9 and 3/9.
> 
> Hans! Please note that there is no functional change. The alt mode
> device is still getting a handle to the mux, just like before.

Ack.

I've reviewed the entire series and it looks good to me.

Patches 1-3 are:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I'm slightly less familiar with the material in patches 4-9, so they
are:

Acked-by: Hans de Goede <hdegoede@redhat.com>

I will also add this series to my personal git tree for testing.

Regards,

Hans




> That was actually happening also in the first version of the series.
> 
> The commit message from v1:
> 
> This series adds support for OF and ACPI device graph parsing to the
> device connection API.
> 
> Handling the graph is straightforward, but because I'm adding that
> fwnode member to struct device_connection, I had to make sure all the
> existing users consider it.
> 
> The plan is to only support matching with fwnode in the future, so no
> more device name matching. The software fwnodes that we now have in
> kernel should make that possible, once we add support for references
> to them.
> 
> The original RFC:
> https://lkml.org/lkml/2018/10/24/619
> 
> thanks,
> 
> Heikki Krogerus (9):
>    platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme
>    usb: typec: Rationalize the API for the muxes
>    platform/x86: intel_cht_int33fe: Remove old style mux connections
>    device connection: Add fwnode member to struct device_connection
>    usb: typec: mux: Find the muxes by also matching against the device
>      node
>    usb: roles: Find the muxes by also matching against the device node
>    usb: typec: Find the ports by also matching against the device node
>    device connection: Prepare support for firmware described connections
>    device connection: Find device connections also from device graphs
> 
>   drivers/base/devcon.c                    | 62 ++++++++++++++-
>   drivers/platform/x86/intel_cht_int33fe.c | 15 ++--
>   drivers/usb/roles/class.c                | 21 +++++-
>   drivers/usb/typec/class.c                | 31 ++++++--
>   drivers/usb/typec/mux.c                  | 96 ++++++++++++++++++++----
>   include/linux/device.h                   |  6 ++
>   include/linux/usb/role.h                 |  1 +
>   include/linux/usb/typec_mux.h            |  3 +-
>   8 files changed, 195 insertions(+), 40 deletions(-)
> 

Re: [PATCH v2 0/9] device connection: Add support for device graphs

[Heikki Krogerus]

On Thu, Jan 31, 2019 at 11:06:29AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-01-19 17:02, Heikki Krogerus wrote:
> > Hi,
> > 
> > This is the second version of this series. On top the two code style
> > improvements requested by Andy, I also renamed the connection
> > identifiers used with the USB Type-C muxes for something that I felt
> > are better, especially after we start using them to name reference
> > device properties in fwnodes. That's why the first patch is now split
> > in two, 1/9 and 3/9.
> > 
> > Hans! Please note that there is no functional change. The alt mode
> > device is still getting a handle to the mux, just like before.
> 
> Ack.
> 
> I've reviewed the entire series and it looks good to me.
> 
> Patches 1-3 are:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> I'm slightly less familiar with the material in patches 4-9, so they
> are:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> I will also add this series to my personal git tree for testing.

Cool!

Thanks Hans!

-- 
heikki

RE: [PATCH v2 0/9] device connection: Add support for device graphs

[Jun Li]

Hi
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年1月31日 0:03
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Chen Yu
> <chenyu56@huawei.com>; Jun Li <jun.li@nxp.com>; Hans de Goede
> <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: [PATCH v2 0/9] device connection: Add support for device graphs
> 
> Hi,
> 
> This is the second version of this series. On top the two code style improvements
> requested by Andy, I also renamed the connection identifiers used with the USB
> Type-C muxes for something that I felt are better, especially after we start using
> them to name reference device properties in fwnodes. That's why the first patch is
> now split in two, 1/9 and 3/9.
> 
> Hans! Please note that there is no functional change. The alt mode device is still
> getting a handle to the mux, just like before.
> That was actually happening also in the first version of the series.
> 
> The commit message from v1:
> 
> This series adds support for OF and ACPI device graph parsing to the device
> connection API.
> 
> Handling the graph is straightforward, but because I'm adding that fwnode member
> to struct device_connection, I had to make sure all the existing users consider it.
> 
> The plan is to only support matching with fwnode in the future, so no more device
> name matching. The software fwnodes that we now have in kernel should make that
> possible, once we add support for references to them.
> 
> The original RFC:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> 2Flkml%2F2018%2F10%2F24%2F619&amp;data=02%7C01%7Cjun.li%40nxp.co
> m%7C2fd7c8c28d67434354be08d686cc6b55%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C636844609858846167&amp;sdata=AWDD9WaO%2BXxM
> Izlli6GUNEq%2FqUpa5hSyLbBsjICdLIo%3D&amp;reserved=0
> 
> thanks,
> 
> Heikki Krogerus (9):
>   platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme
>   usb: typec: Rationalize the API for the muxes
>   platform/x86: intel_cht_int33fe: Remove old style mux connections
>   device connection: Add fwnode member to struct device_connection
>   usb: typec: mux: Find the muxes by also matching against the device
>     node
>   usb: roles: Find the muxes by also matching against the device node
>   usb: typec: Find the ports by also matching against the device node
>   device connection: Prepare support for firmware described connections
>   device connection: Find device connections also from device graphs
> 
>  drivers/base/devcon.c                    | 62 ++++++++++++++-
>  drivers/platform/x86/intel_cht_int33fe.c | 15 ++--
>  drivers/usb/roles/class.c                | 21 +++++-
>  drivers/usb/typec/class.c                | 31 ++++++--
>  drivers/usb/typec/mux.c                  | 96 ++++++++++++++++++++----
>  include/linux/device.h                   |  6 ++
>  include/linux/usb/role.h                 |  1 +
>  include/linux/usb/typec_mux.h            |  3 +-
>  8 files changed, 195 insertions(+), 40 deletions(-)
> 
> --
> 2.20.1

I tested this series on dwc3+typec, for usb role switch and typec switch common part
Reviewed-by: Jun Li <jun.li@nxp.com>
Tested-by: Jun Li <jun.li@nxp.com>

Re: [PATCH v2 0/9] device connection: Add support for device graphs

[Heikki Krogerus]

On Tue, Feb 12, 2019 at 10:44:35AM +0000, Jun Li wrote:
> Hi
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: 2019年1月31日 0:03
> > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; Chen Yu
> > <chenyu56@huawei.com>; Jun Li <jun.li@nxp.com>; Hans de Goede
> > <hdegoede@redhat.com>; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: [PATCH v2 0/9] device connection: Add support for device graphs
> > 
> > Hi,
> > 
> > This is the second version of this series. On top the two code style improvements
> > requested by Andy, I also renamed the connection identifiers used with the USB
> > Type-C muxes for something that I felt are better, especially after we start using
> > them to name reference device properties in fwnodes. That's why the first patch is
> > now split in two, 1/9 and 3/9.
> > 
> > Hans! Please note that there is no functional change. The alt mode device is still
> > getting a handle to the mux, just like before.
> > That was actually happening also in the first version of the series.
> > 
> > The commit message from v1:
> > 
> > This series adds support for OF and ACPI device graph parsing to the device
> > connection API.
> > 
> > Handling the graph is straightforward, but because I'm adding that fwnode member
> > to struct device_connection, I had to make sure all the existing users consider it.
> > 
> > The plan is to only support matching with fwnode in the future, so no more device
> > name matching. The software fwnodes that we now have in kernel should make that
> > possible, once we add support for references to them.
> > 
> > The original RFC:
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > 2Flkml%2F2018%2F10%2F24%2F619&amp;data=02%7C01%7Cjun.li%40nxp.co
> > m%7C2fd7c8c28d67434354be08d686cc6b55%7C686ea1d3bc2b4c6fa92cd99c5c
> > 301635%7C0%7C0%7C636844609858846167&amp;sdata=AWDD9WaO%2BXxM
> > Izlli6GUNEq%2FqUpa5hSyLbBsjICdLIo%3D&amp;reserved=0
> > 
> > thanks,
> > 
> > Heikki Krogerus (9):
> >   platform/x86: intel_cht_int33fe: Prepare for better mux naming scheme
> >   usb: typec: Rationalize the API for the muxes
> >   platform/x86: intel_cht_int33fe: Remove old style mux connections
> >   device connection: Add fwnode member to struct device_connection
> >   usb: typec: mux: Find the muxes by also matching against the device
> >     node
> >   usb: roles: Find the muxes by also matching against the device node
> >   usb: typec: Find the ports by also matching against the device node
> >   device connection: Prepare support for firmware described connections
> >   device connection: Find device connections also from device graphs
> > 
> >  drivers/base/devcon.c                    | 62 ++++++++++++++-
> >  drivers/platform/x86/intel_cht_int33fe.c | 15 ++--
> >  drivers/usb/roles/class.c                | 21 +++++-
> >  drivers/usb/typec/class.c                | 31 ++++++--
> >  drivers/usb/typec/mux.c                  | 96 ++++++++++++++++++++----
> >  include/linux/device.h                   |  6 ++
> >  include/linux/usb/role.h                 |  1 +
> >  include/linux/usb/typec_mux.h            |  3 +-
> >  8 files changed, 195 insertions(+), 40 deletions(-)
> > 
> > --
> > 2.20.1
> 
> I tested this series on dwc3+typec, for usb role switch and typec switch common part
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Tested-by: Jun Li <jun.li@nxp.com>

Thanks Jun.

I'm going to send one more version, and fix the description of the
struct usb_role_switch_desc like you proposed.

Since the fix will only add one comment line to the code, I'm going to
take the liberty to add your Reviewed-by and Tested-by tags this
time (I don't usually like to carry them to new versions of patches).
I'll do the same with the others.


thanks,

-- 
heikki