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
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 <[email protected]>
---
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
Adding new connections with for the muxes with new
identifiers. The old connection are left in for now.
Signed-off-by: Heikki Krogerus <[email protected]>
---
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
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 <[email protected]>
---
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
This will prepare the device connection API for connections
described in firmware.
Signed-off-by: Heikki Krogerus <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
<[email protected]> 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
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 <[email protected]>
I'm slightly less familiar with the material in patches 4-9, so they
are:
Acked-by: Hans de Goede <[email protected]>
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(-)
>
On Wed, Jan 30, 2019 at 06:51:56PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 30, 2019 at 6:03 PM Heikki Krogerus
> <[email protected]> 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
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 <[email protected]>
>
> I'm slightly less familiar with the material in patches 4-9, so they
> are:
>
> Acked-by: Hans de Goede <[email protected]>
>
> I will also add this series to my personal git tree for testing.
Cool!
Thanks Hans!
--
heikki
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
> > <[email protected]> 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
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
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
On Mon, Feb 11, 2019 at 10:39 AM Heikki Krogerus
<[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
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
> -----Original Message-----
> From: Heikki Krogerus <[email protected]>
> Sent: 2019??2??11?? 18:46
> To: Jun Li <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Andy Shevchenko
> <[email protected]>; Chen Yu <[email protected]>; Hans de
> Goede <[email protected]>; [email protected];
> [email protected]
> 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&data=02%7C01%7Cjun.li%40nx
> p.com
> > %7C8c2d40d5e5d246da34ad08d6900e31cf%7C686ea1d3bc2b4c6fa92cd99c5
> c301635
> > %7C0%7C0%7C636854788040965224&sdata=db4gvXKc9InWiltsweetxXYr
> tPbtfX
> > jshPh%2FnvA24ig%3D&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
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
Hi
> -----Original Message-----
> From: Heikki Krogerus <[email protected]>
> Sent: 2019??2??12?? 16:51
> To: Jun Li <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Andy Shevchenko
> <[email protected]>; Chen Yu <[email protected]>; Hans de
> Goede <[email protected]>; [email protected];
> [email protected]
> 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
Hi
> -----Original Message-----
> From: Heikki Krogerus <[email protected]>
> Sent: 2019??1??31?? 0:03
> To: Greg Kroah-Hartman <[email protected]>
> Cc: Andy Shevchenko <[email protected]>; Chen Yu
> <[email protected]>; Jun Li <[email protected]>; Hans de Goede
> <[email protected]>; [email protected];
> [email protected]
> 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&data=02%7C01%7Cjun.li%40nxp.co
> m%7C2fd7c8c28d67434354be08d686cc6b55%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C636844609858846167&sdata=AWDD9WaO%2BXxM
> Izlli6GUNEq%2FqUpa5hSyLbBsjICdLIo%3D&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 <[email protected]>
Tested-by: Jun Li <[email protected]>
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
On Tue, Feb 12, 2019 at 10:44:35AM +0000, Jun Li wrote:
> Hi
> > -----Original Message-----
> > From: Heikki Krogerus <[email protected]>
> > Sent: 2019年1月31日 0:03
> > To: Greg Kroah-Hartman <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>; Chen Yu
> > <[email protected]>; Jun Li <[email protected]>; Hans de Goede
> > <[email protected]>; [email protected];
> > [email protected]
> > 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&data=02%7C01%7Cjun.li%40nxp.co
> > m%7C2fd7c8c28d67434354be08d686cc6b55%7C686ea1d3bc2b4c6fa92cd99c5c
> > 301635%7C0%7C0%7C636844609858846167&sdata=AWDD9WaO%2BXxM
> > Izlli6GUNEq%2FqUpa5hSyLbBsjICdLIo%3D&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 <[email protected]>
> Tested-by: Jun Li <[email protected]>
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