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

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

[Heikki Krogerus]

Hi,

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 (8):
  platform/x86: intel_cht_int33fe: Remove connection for the alt mode
    mux
  usb: typec: Rationalize the API for the muxes
  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                    | 63 ++++++++++++++++-
 drivers/platform/x86/intel_cht_int33fe.c | 11 ++-
 drivers/usb/roles/class.c                | 21 +++++-
 drivers/usb/typec/class.c                | 31 ++++++---
 drivers/usb/typec/mux.c                  | 86 +++++++++++++++++++-----
 include/linux/device.h                   |  6 ++
 include/linux/usb/role.h                 |  1 +
 include/linux/usb/typec_mux.h            |  3 +-
 8 files changed, 185 insertions(+), 37 deletions(-)

-- 
2.20.1

[PATCH 1/8] platform/x86: intel_cht_int33fe: Remove connection for the alt mode mux

[Heikki Krogerus]

Driver for fusb302 does not support alternate modes, so the
connection is not really needed for now. Removing that
connection description allows us to improve the USB Type-C
mux API.

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

diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index 02bc74608cf3..fbd24daa7f8d 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[4];
 };
 
 /*
@@ -178,12 +178,9 @@ static int cht_int33fe_probe(struct platform_device *pdev)
 	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[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

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

[Heikki Krogerus]

We can replace the second parameter that is passed to the
typec_mux_get() function with alt mode description
structure, and simply pass NULL with accessory modes.

With accessory modes there is no need for additional
identification (in practice the accessory mode can only be
Audio Accessory if muxing is needed), only with alternate
modes we need to identify the exact alternate mode.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c     | 7 ++-----
 drivers/usb/typec/mux.c       | 8 +++++---
 include/linux/usb/typec_mux.h | 3 ++-
 3 files changed, 9 insertions(+), 9 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..c7b09cbcd45e 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -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, "typec-mux", (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

[PATCH 3/8] 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

[PATCH 4/8] 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 | 78 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index c7b09cbcd45e..eea29024da25 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,22 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 {
 	struct typec_switch *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);
+	}
+
+	if (con->id &&
+	    !fwnode_property_present(con->fwnode, "orientation-switch"))
+		return NULL;
+
 	list_for_each_entry(sw, &switch_list, entry)
-		if (!strcmp(con->endpoint[ep], dev_name(sw->dev)))
+		if (dev_fwnode(sw->dev) == con->fwnode)
 			return sw;
 
-	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the switch to show up.
-	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	return con->id ? ERR_PTR(-EPROBE_DEFER) : NULL;
 }
 
 /**
@@ -112,17 +121,62 @@ 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;
+	bool match = !con->id;
+	size_t nval;
+	u16 *val;
+	int i;
+
+	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);
+	}
+
+	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 (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
+		if (dev_fwnode(mux->dev) == con->fwnode)
 			return mux;
 
-	/*
-	 * We only get called if a connection was found, tell the caller to
-	 * wait for the switch to show up.
-	 */
-	return ERR_PTR(-EPROBE_DEFER);
+	return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
 }
 
 /**
-- 
2.20.1

[PATCH 5/8] 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

[PATCH 6/8] 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

[PATCH 7/8] 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

[PATCH 8/8] 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 | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
index 858b8d2f6ef8..1c20eceff2b0 100644
--- a/drivers/base/devcon.c
+++ b/drivers/base/devcon.c
@@ -7,10 +7,38 @@
  */
 
 #include <linux/device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
 
 static DEFINE_MUTEX(devcon_lock);
 static LIST_HEAD(devcon_list);
 
+typedef void *(*devcon_match_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_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 +51,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_t match)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	const char *devname = dev_name(dev);
 	struct device_connection *con;
 	void *ret = NULL;
@@ -35,6 +62,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

Re: [PATCH 1/8] platform/x86: intel_cht_int33fe: Remove connection for the alt mode mux

[Andy Shevchenko]

On Fri, Jan 25, 2019 at 3:17 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Driver for fusb302 does not support alternate modes, so the
> connection is not really needed for now. Removing that
> connection description allows us to improve the USB Type-C
> mux API.
>

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
supposed to go via USB tree.

> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/platform/x86/intel_cht_int33fe.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> index 02bc74608cf3..fbd24daa7f8d 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[4];
>  };
>
>  /*
> @@ -178,12 +178,9 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>         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[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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/8] usb: typec: Rationalize the API for the muxes

[Andy Shevchenko]

On Fri, Jan 25, 2019 at 3:18 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> We can replace the second parameter that is passed to the
> typec_mux_get() function with alt mode description
> structure, and simply pass NULL with accessory modes.
>
> With accessory modes there is no need for additional
> identification (in practice the accessory mode can only be
> Audio Accessory if muxing is needed), only with alternate
> modes we need to identify the exact alternate mode.

> +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, "typec-mux", (void *)desc,
> +                                          typec_mux_match);

Not related to this series, but shouldn't
device_connection_find_match() be improved to avoid dropping const
qualifier?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/8] usb: typec: mux: Find the muxes by also matching against the device node

[Andy Shevchenko]

On Fri, Jan 25, 2019 at 3:17 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.

>  static void *typec_mux_match(struct device_connection *con, int ep, void *data)
>  {
> +       const struct typec_altmode_desc *desc = data;
>         struct typec_mux *mux;

> +       bool match = !con->id;

I don't see how this assignment is used.

> +       size_t nval;
> +       u16 *val;
> +       int i;
> +
> +       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);
> +       }
> +
> +       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 (!strcmp(con->endpoint[ep], dev_name(mux->dev)))
> +               if (dev_fwnode(mux->dev) == con->fwnode)
>                         return mux;
>
> -       /*
> -        * We only get called if a connection was found, tell the caller to
> -        * wait for the switch to show up.
> -        */
> -       return ERR_PTR(-EPROBE_DEFER);
> +       return match ? ERR_PTR(-EPROBE_DEFER) : NULL;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/8] device connection: Find device connections also from device graphs

[Andy Shevchenko]

On Fri, Jan 25, 2019 at 3:17 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> If connections between devices are described in OF graph or
> ACPI device graph, we can find them by using the
> fwnode_graph_*() functions.

> +typedef void *(*devcon_match_t)(struct device_connection *con, int ep,
> +                               void *data);

devcon_match_fn ?
_t sounds to me more like a struct or a redefined POD.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/8] platform/x86: intel_cht_int33fe: Remove connection for the alt mode mux

[Hans de Goede]

Hi,

On 28-01-19 10:45, Andy Shevchenko wrote:
> On Fri, Jan 25, 2019 at 3:17 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
>>
>> Driver for fusb302 does not support alternate modes, so the
>> connection is not really needed for now. Removing that
>> connection description allows us to improve the USB Type-C
>> mux API.
>>
> 
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> supposed to go via USB tree.

I missed the original posting of this, so let me reply here:

Nack to this change, I've a patch-set in the works to
make display-port over type-c work with 2 devices with a fusb302
mux and that needs this connection.

Regards,

Hans



> 
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel_cht_int33fe.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
>> index 02bc74608cf3..fbd24daa7f8d 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[4];
>>   };
>>
>>   /*
>> @@ -178,12 +178,9 @@ static int cht_int33fe_probe(struct platform_device *pdev)
>>          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[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
>>
> 
> 

Re: [PATCH 1/8] platform/x86: intel_cht_int33fe: Remove connection for the alt mode mux

[Heikki Krogerus]

Hi Hans,

On Mon, Jan 28, 2019 at 11:44:29AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 28-01-19 10:45, Andy Shevchenko wrote:
> > On Fri, Jan 25, 2019 at 3:17 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > > 
> > > Driver for fusb302 does not support alternate modes, so the
> > > connection is not really needed for now. Removing that
> > > connection description allows us to improve the USB Type-C
> > > mux API.
> > > 
> > 
> > Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > supposed to go via USB tree.
> 
> I missed the original posting of this, so let me reply here:
> 
> Nack to this change, I've a patch-set in the works to
> make display-port over type-c work with 2 devices with a fusb302
> mux and that needs this connection.

I can add the connections back in this series after the API
modification patches, but should the connections be added back only
after we actually support the alt mode in the driver?

Btw. I'm preparing patches where I remove struct tcpc_config
completely. We can do that by taking advantage of the software fwnodes
(I'll send the patches RFC to give you an idea what I'm talking about).

That's related as we don't need struct tcpc_config for anything else
except for alternate modes (which no driver supports currently) after
that series, and even with the alt modes, it's only a question of
supplying DT bindings that define the appropriate device properties.

Also, as a "heads-up": As I explained in the cover-letter, my plan is
to take advantage of the software fwnodes also with the connections.
By adding support for reference handling to the software nodes, we
don't need to maintain the list of connections as we do today. And
more importantly, we don't need to match using device names, which is
always fragile.

That means we will change the connection registration, actually,
remove connection registration :-). The connections after that can
always be described in the fwnode for the device.


thanks,

-- 
heikki

Re: [PATCH 2/8] usb: typec: Rationalize the API for the muxes

[Heikki Krogerus]

On Mon, Jan 28, 2019 at 11:50:31AM +0200, Andy Shevchenko wrote:
> On Fri, Jan 25, 2019 at 3:18 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > We can replace the second parameter that is passed to the
> > typec_mux_get() function with alt mode description
> > structure, and simply pass NULL with accessory modes.
> >
> > With accessory modes there is no need for additional
> > identification (in practice the accessory mode can only be
> > Audio Accessory if muxing is needed), only with alternate
> > modes we need to identify the exact alternate mode.
> 
> > +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, "typec-mux", (void *)desc,
> > +                                          typec_mux_match);
> 
> Not related to this series, but shouldn't
> device_connection_find_match() be improved to avoid dropping const
> qualifier?

I'll see if we can do that already.

thanks,

-- 
heikki

Re: [PATCH 4/8] usb: typec: mux: Find the muxes by also matching against the device node

[Heikki Krogerus]

On Mon, Jan 28, 2019 at 11:53:54AM +0200, Andy Shevchenko wrote:
> On Fri, Jan 25, 2019 at 3:17 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.
> 
> >  static void *typec_mux_match(struct device_connection *con, int ep, void *data)
> >  {
> > +       const struct typec_altmode_desc *desc = data;
> >         struct typec_mux *mux;
> 
> > +       bool match = !con->id;
> 
> I don't see how this assignment is used.

That definitely deserves a comment at least.

This series adds support for the "device graph" to the device
connection API, but it is also possible to describe connections with
normal references (fwnode_property_get_reference_args()). With normal
references we don't need to do any extra identification like we do
with device graph.

The idea is to supply the connection id only if it has not been
"consumed" already (device graph case). If it has been "consumed", we
know there is no need for any extra connection identification (normal
references), and we can jump straight to the next stage -> find the
mux..

> > +       size_t nval;
> > +       u16 *val;
> > +       int i;
> > +
> > +       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);
> > +       }
> > +
> > +       if (match)
> > +               goto find_mux;

..here.

I'll change this so that the match variable is introduced without
setting default value. Instead, how about this:

        /*
         * Check has the identifier already been "consumed". If it
         * has, no need to do any extra connection identification.
         */
        match = !con->id;
        if (match)
                goto find_mux;

thanks,

-- 
heikki

Re: [PATCH 4/8] usb: typec: mux: Find the muxes by also matching against the device node

[Andy Shevchenko]

On Mon, Jan 28, 2019 at 5:46 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Mon, Jan 28, 2019 at 11:53:54AM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 25, 2019 at 3:17 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:

> > > +       bool match = !con->id;
> >
> > I don't see how this assignment is used.
>
> That definitely deserves a comment at least.
>
> This series adds support for the "device graph" to the device
> connection API, but it is also possible to describe connections with
> normal references (fwnode_property_get_reference_args()). With normal
> references we don't need to do any extra identification like we do
> with device graph.
>
> The idea is to supply the connection id only if it has not been
> "consumed" already (device graph case). If it has been "consumed", we
> know there is no need for any extra connection identification (normal
> references), and we can jump straight to the next stage -> find the
> 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);
> > > +       }
> > > +
> > > +       if (match)
> > > +               goto find_mux;
>
> ..here.
>
> I'll change this so that the match variable is introduced without
> setting default value. Instead, how about this:
>
>         /*
>          * Check has the identifier already been "consumed". If it
>          * has, no need to do any extra connection identification.
>          */
>         match = !con->id;
>         if (match)
>                 goto find_mux;

Cool, thanks!


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/8] platform/x86: intel_cht_int33fe: Remove connection for the alt mode mux

[Hans de Goede]

Hi,

On 28-01-19 16:27, Heikki Krogerus wrote:
> Hi Hans,
> 
> On Mon, Jan 28, 2019 at 11:44:29AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 28-01-19 10:45, Andy Shevchenko wrote:
>>> On Fri, Jan 25, 2019 at 3:17 PM Heikki Krogerus
>>> <heikki.krogerus@linux.intel.com> wrote:
>>>>
>>>> Driver for fusb302 does not support alternate modes, so the
>>>> connection is not really needed for now. Removing that
>>>> connection description allows us to improve the USB Type-C
>>>> mux API.
>>>>
>>>
>>> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> supposed to go via USB tree.
>>
>> I missed the original posting of this, so let me reply here:
>>
>> Nack to this change, I've a patch-set in the works to
>> make display-port over type-c work with 2 devices with a fusb302
>> mux and that needs this connection.
> 
> I can add the connections back in this series after the API
> modification patches, but should the connections be added back only
> after we actually support the alt mode in the driver?
> 
> Btw. I'm preparing patches where I remove struct tcpc_config
> completely. We can do that by taking advantage of the software fwnodes
> (I'll send the patches RFC to give you an idea what I'm talking about).
> 
> That's related as we don't need struct tcpc_config for anything else
> except for alternate modes (which no driver supports currently) after
> that series, and even with the alt modes, it's only a question of
> supplying DT bindings that define the appropriate device properties.
> 
> Also, as a "heads-up": As I explained in the cover-letter, my plan is
> to take advantage of the software fwnodes also with the connections.
> By adding support for reference handling to the software nodes, we
> don't need to maintain the list of connections as we do today. And
> more importantly, we don't need to match using device names, which is
> always fragile.
> 
> That means we will change the connection registration, actually,
> remove connection registration :-). The connections after that can
> always be described in the fwnode for the device.

I see that you've posted a v2 series now and that you've kept the dev_name
matching for platforms where there are no fwnodes to match on, thanks.

I've just reviewed the v2 series and it looks good to me, I will reply
there.

Regards,

Hans