| Message ID | 20220208031944.3444-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | typec: mux: Introduce support for multiple TypeC muxes | expand |
On Mon, Feb 07, 2022 at 07:19:42PM -0800, Bjorn Andersson wrote: > In the Qualcomm platforms the USB/DP PHY handles muxing and orientation > switching of the SuperSpeed lines, but the SBU lines needs to be > connected and switched by external (to the SoC) hardware. > > It's therefor necessary to be able to have the TypeC controller operate > multiple TypeC muxes and switches. Use the newly introduced indirection > object to handle this, to avoid having to taint the TypeC controllers > with knowledge about the downstream hardware configuration. > > The max number of devs per indirection is set to 3, which account for > being able to mux/switch the USB HS, SS and SBU lines, as per defined > defined in the usb-c-connector binding. This number could be grown if > need arrises at a later point in time. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > > Changes since v1: > - Improved the motivation for the 3 in the commit message. > - kfree sw and mux in error paths > > drivers/usb/typec/mux.c | 128 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 102 insertions(+), 26 deletions(-) > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > index d0b42c297aca..cf2347dd1663 100644 > --- a/drivers/usb/typec/mux.c > +++ b/drivers/usb/typec/mux.c > @@ -17,8 +17,11 @@ > #include "class.h" > #include "mux.h" > > +#define TYPEC_MUX_MAX_DEVS 3 > + > struct typec_switch { > - struct typec_switch_dev *sw_dev; > + struct typec_switch_dev *sw_devs[TYPEC_MUX_MAX_DEVS]; > + unsigned int num_sw_devs; > }; > > static int switch_fwnode_match(struct device *dev, const void *fwnode) > @@ -67,25 +70,50 @@ static void *typec_switch_match(struct fwnode_handle *fwnode, const char *id, > */ > struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode) > { > - struct typec_switch_dev *sw_dev; > + struct typec_switch_dev *sw_devs[TYPEC_MUX_MAX_DEVS]; > struct typec_switch *sw; > + int count; > + int err; > + int i; > > sw = kzalloc(sizeof(*sw), GFP_KERNEL); > if (!sw) > return ERR_PTR(-ENOMEM); > > - sw_dev = fwnode_connection_find_match(fwnode, "orientation-switch", NULL, > - typec_switch_match); > - if (IS_ERR_OR_NULL(sw_dev)) { > + count = fwnode_connection_find_matches(fwnode, "orientation-switch", NULL, > + typec_switch_match, > + (void **)sw_devs, > + ARRAY_SIZE(sw_devs)); > + if (count <= 0) { > kfree(sw); > - return ERR_CAST(sw_dev); > + return NULL; > } > > - WARN_ON(!try_module_get(sw_dev->dev.parent->driver->owner)); > + for (i = 0; i < count; i++) { > + if (IS_ERR(sw_devs[i])) { > + err = PTR_ERR(sw_devs[i]); > + goto put_sw_devs; > + } > + } > > - sw->sw_dev = sw_dev; > + for (i = 0; i < count; i++) { > + WARN_ON(!try_module_get(sw_devs[i]->dev.parent->driver->owner)); > + sw->sw_devs[i] = sw_devs[i]; > + } > + > + sw->num_sw_devs = count; > > return sw; > + > +put_sw_devs: > + for (i = 0; i < count; i++) { > + if (!IS_ERR(sw_devs[i])) > + put_device(&sw_devs[i]->dev); > + } > + > + kfree(sw); > + > + return ERR_PTR(err); > } > EXPORT_SYMBOL_GPL(fwnode_typec_switch_get); > > @@ -98,14 +126,17 @@ EXPORT_SYMBOL_GPL(fwnode_typec_switch_get); > void typec_switch_put(struct typec_switch *sw) > { > struct typec_switch_dev *sw_dev; > + unsigned int i; > > if (IS_ERR_OR_NULL(sw)) > return; > > - sw_dev = sw->sw_dev; > + for (i = 0; i < sw->num_sw_devs; i++) { > + sw_dev = sw->sw_devs[i]; > > - module_put(sw_dev->dev.parent->driver->owner); > - put_device(&sw_dev->dev); > + module_put(sw_dev->dev.parent->driver->owner); > + put_device(&sw_dev->dev); > + } > kfree(sw); > } > EXPORT_SYMBOL_GPL(typec_switch_put); > @@ -170,13 +201,21 @@ int typec_switch_set(struct typec_switch *sw, > enum typec_orientation orientation) > { > struct typec_switch_dev *sw_dev; > + unsigned int i; > + int ret; > > if (IS_ERR_OR_NULL(sw)) > return 0; > > - sw_dev = sw->sw_dev; > + for (i = 0; i < sw->num_sw_devs; i++) { > + sw_dev = sw->sw_devs[i]; > + > + ret = sw_dev->set(sw_dev, orientation); > + if (ret) > + return ret; > + } > > - return sw_dev->set(sw_dev, orientation); > + return 0; > } > EXPORT_SYMBOL_GPL(typec_switch_set); > > @@ -208,7 +247,8 @@ EXPORT_SYMBOL_GPL(typec_switch_get_drvdata); > /* ------------------------------------------------------------------------- */ > > struct typec_mux { > - struct typec_mux_dev *mux_dev; > + struct typec_mux_dev *mux_devs[TYPEC_MUX_MAX_DEVS]; > + unsigned int num_mux_devs; > }; > > static int mux_fwnode_match(struct device *dev, const void *fwnode) > @@ -291,25 +331,50 @@ static void *typec_mux_match(struct fwnode_handle *fwnode, const char *id, > struct typec_mux *fwnode_typec_mux_get(struct fwnode_handle *fwnode, > const struct typec_altmode_desc *desc) > { > - struct typec_mux_dev *mux_dev; > + struct typec_mux_dev *mux_devs[TYPEC_MUX_MAX_DEVS]; > struct typec_mux *mux; > + int count; > + int err; > + int i; > > mux = kzalloc(sizeof(*mux), GFP_KERNEL); > if (!mux) > return ERR_PTR(-ENOMEM); > > - mux_dev = fwnode_connection_find_match(fwnode, "mode-switch", (void *)desc, > - typec_mux_match); > - if (IS_ERR_OR_NULL(mux_dev)) { > + count = fwnode_connection_find_matches(fwnode, "mode-switch", > + (void *)desc, typec_mux_match, > + (void **)mux_devs, > + ARRAY_SIZE(mux_devs)); > + if (count <= 0) { > kfree(mux); > - return ERR_CAST(mux_dev); > + return NULL; > } > > - WARN_ON(!try_module_get(mux_dev->dev.parent->driver->owner)); > + for (i = 0; i < count; i++) { > + if (IS_ERR(mux_devs[i])) { > + err = PTR_ERR(mux_devs[i]); > + goto put_mux_devs; > + } > + } > + > + for (i = 0; i < count; i++) { > + WARN_ON(!try_module_get(mux_devs[i]->dev.parent->driver->owner)); > + mux->mux_devs[i] = mux_devs[i]; > + } > > - mux->mux_dev = mux_dev; > + mux->num_mux_devs = count; > > return mux; > + > +put_mux_devs: > + for (i = 0; i < count; i++) { > + if (!IS_ERR(mux_devs[i])) > + put_device(&mux_devs[i]->dev); > + } > + > + kfree(mux); > + > + return ERR_PTR(err); > } > EXPORT_SYMBOL_GPL(fwnode_typec_mux_get); > > @@ -322,13 +387,16 @@ EXPORT_SYMBOL_GPL(fwnode_typec_mux_get); > void typec_mux_put(struct typec_mux *mux) > { > struct typec_mux_dev *mux_dev; > + unsigned int i; > > if (IS_ERR_OR_NULL(mux)) > return; > > - mux_dev = mux->mux_dev; > - module_put(mux_dev->dev.parent->driver->owner); > - put_device(&mux_dev->dev); > + for (i = 0; i < mux->num_mux_devs; i++) { > + mux_dev = mux->mux_devs[i]; > + module_put(mux_dev->dev.parent->driver->owner); > + put_device(&mux_dev->dev); > + } > kfree(mux); > } > EXPORT_SYMBOL_GPL(typec_mux_put); > @@ -336,13 +404,21 @@ EXPORT_SYMBOL_GPL(typec_mux_put); > int typec_mux_set(struct typec_mux *mux, struct typec_mux_state *state) > { > struct typec_mux_dev *mux_dev; > + unsigned int i; > + int ret; > > if (IS_ERR_OR_NULL(mux)) > return 0; > > - mux_dev = mux->mux_dev; > + for (i = 0; i < mux->num_mux_devs; i++) { > + mux_dev = mux->mux_devs[i]; > + > + ret = mux_dev->set(mux_dev, state); > + if (ret) > + return ret; > + } > > - return mux_dev->set(mux_dev, state); > + return 0; > } > EXPORT_SYMBOL_GPL(typec_mux_set); > > -- > 2.33.1
On Mon 21 Feb 09:19 PST 2022, Andy Shevchenko wrote: > On Sun, Feb 20, 2022 at 08:55:10PM -0800, Bjorn Andersson wrote: > > On Sun 20 Feb 03:16 PST 2022, Andy Shevchenko wrote: > > > On Fri, Feb 18, 2022 at 11:00:45AM -0800, Bjorn Andersson wrote: > > > > On Wed 09 Feb 04:30 PST 2022, Andy Shevchenko wrote: > > > > > On Mon, Feb 07, 2022 at 07:19:39PM -0800, Bjorn Andersson wrote: > > ... > > > > > > > +int fwnode_connection_find_matches(struct fwnode_handle *fwnode, > > > > > > + const char *con_id, void *data, > > > > > > + devcon_match_fn_t match, > > > > > > + void **matches, unsigned int matches_len) > > > > > > +{ > > > > > > + unsigned int count; > > > > > > + > > > > > > + if (!fwnode || !match || !matches) > > > > > > > > > > !matches case may be still useful to get the count and allocate memory by > > > > > caller. Please, consider this case. > > > > > > > > As discussed in previous version, and described in the commit message, > > > > the returned value of "match" is a opaque pointer to something which > > > > has to be passed back to the caller in order to be cleaned up. > > > > > > > > E.g. the typec mux code returns a pointer to a typec_mux/switch object > > > > with a refcounted struct device within, or an ERR_PTR(). > > > > > > > > So unfortunately we can must gather the results into matches and pass it > > > > back to the caller to take consume or clean up. > > > > > > It's fine. You have **matches, means pointer of an opaque pointer. > > > What I'm talking about is memory allocation for and array of _pointers_. > > > That's what caller very much aware of and can allocate on heap. So, please > > > consider this case. > > > > I'm sorry, but I'm not sure what you're looking for. > > > > > > I still interpret your comment as that it would be nice to be able to do > > something like: > > > > count = fwnode_connection_find_matches(fwnode, "orientation-switch", > > NULL, typec_switch_match, NULL, 0); > > > > based on the returned value the caller could allocate an array of > > "count" pointers and then call the function again to actually fill out > > the count elements. > > Yes, that's what I want from the generic fwnode APIs. > (Keyword: generic) > > > The problem with this is that, typec_switch_match() does: > > As you stated, the problem is in the typec_switch_match(). So, it's not related > to the fwnode, but how you are using it. > > > void *typec_switch_match(fwnode, id, data) { > > struct device *dev = find_struct_device(fwnode, id); > > if (!dev) > > return NULL; > > get_device(dev); > > return container_of(dev, struct typec_switch, dev); > > } > > > > So if we call the match function and if that finds a "dev" it will > > return a struct typec_switch with a refcounted struct device within. > > fwnode (as being an abstraction on top of the others) has no knowledge > about this. And more important should not know that. > > > We can see if that's NULL or not and will be able to return a "count", > > but we have no way of releasing the reference acquired - we must return > > the void pointer back to the client, so that it can release it. > > The caller (if it wants to!) may create different callbacks for count and real > matching, no? > Ahh, yeah you're right, we can shift this responsibility onto the caller and thereby allow them to implement the count as well. Makes sense! Thanks, Bjorn > > My claim is that this is not a problem, because this works fine with any > > reasonable size of fwnode graphs we might run into - and the client will > > in general have a sense of the worst case number of matches (in this > > series its 3, as there's 3 types of lanes that can be switched/muxed > > coming out of a USB connector). > > -- > With Best Regards, > Andy Shevchenko > >