| Message ID | 20190125131519.88416-5-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device connection: Add support for device graphs | expand |
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; > }
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,
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!
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; } /**
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(-)