| Message ID | 20220823195429.1243516-4-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | typec orientation switch support via mux controller | expand |
Hi! 2022-08-23 at 21:54, Xu Yang wrote: > Some dedicated mux block can use existing mux controller as a mux > provider, typec port as a consumer to select channel for orientation > switch, this can be an alternate way to control typec orientation switch. > Also, one mux controller could cover highspeed, superspeed and sideband > use case one time in this implementation. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > Changes since v1: > - add build dependence (select MULTIPLEXER) > - improve Multiplexer control logic > > drivers/usb/typec/Kconfig | 1 + > drivers/usb/typec/mux.c | 76 ++++++++++++++++++++++++++++++++++- > include/linux/usb/typec_mux.h | 7 +--- > 3 files changed, 78 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > index 5defdfead653..73d4976d8148 100644 > --- a/drivers/usb/typec/Kconfig > +++ b/drivers/usb/typec/Kconfig > @@ -2,6 +2,7 @@ > > menuconfig TYPEC > tristate "USB Type-C Support" > + select MULTIPLEXER > help > USB Type-C Specification defines a cable and connector for USB where > only one type of plug is supported on both ends, i.e. there will not > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > index 464330776cd6..05e6ed217620 100644 > --- a/drivers/usb/typec/mux.c > +++ b/drivers/usb/typec/mux.c > @@ -13,6 +13,7 @@ > #include <linux/mutex.h> > #include <linux/property.h> > #include <linux/slab.h> > +#include <linux/mux/consumer.h> > > #include "class.h" > #include "mux.h" > @@ -22,6 +23,11 @@ > struct typec_switch { > struct typec_switch_dev *sw_devs[TYPEC_MUX_MAX_DEVS]; > unsigned int num_sw_devs; > + > + /* Could handle HighSpeed, SuperSpeed, Sideband switch one time */ > + struct mux_control *mux_switch; > + /* 3 state correspond to NONE, NORMAL, REVERSE for all switches */ > + int mux_states[3]; > }; > > static int switch_fwnode_match(struct device *dev, const void *fwnode) > @@ -117,6 +123,58 @@ struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode) > } > EXPORT_SYMBOL_GPL(fwnode_typec_switch_get); > > +static struct typec_switch *mux_control_typec_switch_get(struct device *dev) > +{ > + struct typec_switch *sw; > + struct mux_control *mux; > + int ret; > + > + if (!device_property_present(dev, "mux-controls")) > + return NULL; > + > + sw = kzalloc(sizeof(*sw), GFP_KERNEL); > + if (!sw) > + return ERR_PTR(-ENOMEM); > + > + mux = mux_control_get(dev, NULL); > + if (!IS_ERR(mux)) { > + sw->mux_switch = mux; > + ret = device_property_read_u32_array(dev, > + "typec-switch-states", sw->mux_states, 3); > + if (ret) { > + kfree(sw); > + return ERR_PTR(ret); > + } > + } else { > + kfree(sw); > + return ERR_CAST(mux); > + } > + > + return sw; > +} > + > +/** > + * typec_switch_get - Find USB Type-C orientation switch > + * @dev: The device using switch > + * > + * Finds a switch used by @dev. Returns a reference to the switch on > + * success, NULL if no matching connection was found, or > + * ERR_PTR(-EPROBE_DEFER) when a connection was found but the switch > + * has not been enumerated yet, or ERR_PTR with a negative errno. > + */ > +struct typec_switch *typec_switch_get(struct device *dev) > +{ > + struct typec_switch *sw; > + > + sw = fwnode_typec_switch_get(dev_fwnode(dev)); > + if (!sw) > + /* Try get switch based on mux control */ > + sw = mux_control_typec_switch_get(dev); > + > + return sw; > +} > +EXPORT_SYMBOL_GPL(typec_switch_get); > + > /** > * typec_switch_put - Release USB Type-C orientation switch > * @sw: USB Type-C orientation switch > @@ -137,6 +195,10 @@ void typec_switch_put(struct typec_switch *sw) > module_put(sw_dev->dev.parent->driver->owner); > put_device(&sw_dev->dev); > } > + > + if (sw->mux_switch) > + mux_control_put(sw->mux_switch); > + > kfree(sw); > } > EXPORT_SYMBOL_GPL(typec_switch_put); > @@ -204,6 +266,7 @@ int typec_switch_set(struct typec_switch *sw, > enum typec_orientation orientation) > { > struct typec_switch_dev *sw_dev; > + struct mux_control *mux; > unsigned int i; > int ret; > > @@ -218,7 +281,18 @@ int typec_switch_set(struct typec_switch *sw, > return ret; > } > > - return 0; > + mux = sw->mux_switch; > + if (!mux) > + return 0; > + > + ret = mux_control_select(mux, sw->mux_states[orientation]); > + if (ret) > + return ret; > + > + /* Keep it as it is since idle_state is MUX_IDLE_AS_IS */ > + ret = mux_control_deselect(mux); No, this is also broken. You cannot, in any client driver, rely on a mux keeping its state unless you keep it selected. As soon as you deselect it, it might be selected by some other driver. Sure, you might know that there are no other users of the mux in question, and you might also know that the idles state is "as-is". But the driver does not see the bigger picture and has no way of knowing that. So, it needs to keep the mux selected. Cheers, Peter > + > + return ret; > } > EXPORT_SYMBOL_GPL(typec_switch_set); > > diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h > index 9292f0e07846..2287e5a5f591 100644 > --- a/include/linux/usb/typec_mux.h > +++ b/include/linux/usb/typec_mux.h > @@ -24,16 +24,13 @@ struct typec_switch_desc { > void *drvdata; > }; > > + > +struct typec_switch *typec_switch_get(struct device *dev); > struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode); > void typec_switch_put(struct typec_switch *sw); > int typec_switch_set(struct typec_switch *sw, > enum typec_orientation orientation); > > -static inline struct typec_switch *typec_switch_get(struct device *dev) > -{ > - return fwnode_typec_switch_get(dev_fwnode(dev)); > -} > - > struct typec_switch_dev * > typec_switch_register(struct device *parent, > const struct typec_switch_desc *desc);
Hi, On Wed, Aug 24, 2022 at 03:54:28AM +0800, Xu Yang wrote: > Some dedicated mux block can use existing mux controller as a mux > provider, typec port as a consumer to select channel for orientation > switch, this can be an alternate way to control typec orientation switch. > Also, one mux controller could cover highspeed, superspeed and sideband > use case one time in this implementation. > > Reported-by: kernel test robot <lkp@intel.com> Drop that Reported-by tag. > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > Changes since v1: > - add build dependence (select MULTIPLEXER) > - improve Multiplexer control logic > > drivers/usb/typec/Kconfig | 1 + > drivers/usb/typec/mux.c | 76 ++++++++++++++++++++++++++++++++++- > include/linux/usb/typec_mux.h | 7 +--- > 3 files changed, 78 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > index 5defdfead653..73d4976d8148 100644 > --- a/drivers/usb/typec/Kconfig > +++ b/drivers/usb/typec/Kconfig > @@ -2,6 +2,7 @@ > > menuconfig TYPEC > tristate "USB Type-C Support" > + select MULTIPLEXER Whoa, do not tie TYPEC to another subsystem like that! You probable want to make your driver depend on that instead. > help > USB Type-C Specification defines a cable and connector for USB where > only one type of plug is supported on both ends, i.e. there will not > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > index 464330776cd6..05e6ed217620 100644 > --- a/drivers/usb/typec/mux.c > +++ b/drivers/usb/typec/mux.c > @@ -13,6 +13,7 @@ > #include <linux/mutex.h> > #include <linux/property.h> > #include <linux/slab.h> > +#include <linux/mux/consumer.h> > > #include "class.h" > #include "mux.h" > @@ -22,6 +23,11 @@ > struct typec_switch { > struct typec_switch_dev *sw_devs[TYPEC_MUX_MAX_DEVS]; > unsigned int num_sw_devs; > + > + /* Could handle HighSpeed, SuperSpeed, Sideband switch one time */ > + struct mux_control *mux_switch; That does not belong here... > + /* 3 state correspond to NONE, NORMAL, REVERSE for all switches */ > + int mux_states[3]; > }; > > static int switch_fwnode_match(struct device *dev, const void *fwnode) > @@ -117,6 +123,58 @@ struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode) > } > EXPORT_SYMBOL_GPL(fwnode_typec_switch_get); > > +static struct typec_switch *mux_control_typec_switch_get(struct device *dev) > +{ > + struct typec_switch *sw; > + struct mux_control *mux; > + int ret; > + > + if (!device_property_present(dev, "mux-controls")) > + return NULL; > + > + sw = kzalloc(sizeof(*sw), GFP_KERNEL); > + if (!sw) > + return ERR_PTR(-ENOMEM); > + > + mux = mux_control_get(dev, NULL); > + if (!IS_ERR(mux)) { > + sw->mux_switch = mux; > + ret = device_property_read_u32_array(dev, > + "typec-switch-states", sw->mux_states, 3); > + if (ret) { > + kfree(sw); > + return ERR_PTR(ret); > + } > + } else { > + kfree(sw); > + return ERR_CAST(mux); > + } > + > + return sw; > +} That code is broken - you return a switch that has never been registered - but never mind that... You need to register the switch from the place/driver that actually handles the orientation. If you really think that you have to have a generic multiplexer class wrapper layer for these switches, then you need to propose a multiplexer class wrapper driver for that. Though, I'm not sure you need that. There is a GPIO driver proposal for these switches, so can you take a look if that covers your case as well: https://lore.kernel.org/linux-usb/20220810204750.3672362-3-bjorn.andersson@linaro.org/#t In any case, you can't mix that code into the device class code itself like you do above. That kind of coupling is always going to be fragile (as we can see above), but more importantly, it forces the dependency on every Type-C driver, and that is completely wrong. So please keep the subsystems themselves independent of each other and handle things in the device drivers. thanks,
Hi Peter, > -----Original Message----- > From: Peter Rosin <peda@axentia.se> > Sent: Wednesday, August 24, 2022 7:51 PM > To: Xu Yang <xu.yang_2@nxp.com>; heikki.krogerus@linux.intel.com; robh+dt@kernel.org; shawnguo@kernel.org > Cc: gregkh@linuxfoundation.org; linux@roeck-us.net; Jun Li <jun.li@nxp.com>; linux-usb@vger.kernel.org; dl-linux-imx > <linux-imx@nxp.com>; devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: [EXT] Re: [PATCH v2 3/4] usb: typec: mux: add typec orientation switch support via mux controller > > Caution: EXT Email > > Hi! > > 2022-08-23 at 21:54, Xu Yang wrote: > > Some dedicated mux block can use existing mux controller as a mux > > provider, typec port as a consumer to select channel for orientation > > switch, this can be an alternate way to control typec orientation switch. > > Also, one mux controller could cover highspeed, superspeed and sideband > > use case one time in this implementation. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > > > --- > > Changes since v1: > > - add build dependence (select MULTIPLEXER) > > - improve Multiplexer control logic > > > > drivers/usb/typec/Kconfig | 1 + > > drivers/usb/typec/mux.c | 76 ++++++++++++++++++++++++++++++++++- > > include/linux/usb/typec_mux.h | 7 +--- > > 3 files changed, 78 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > > index 5defdfead653..73d4976d8148 100644 > > --- a/drivers/usb/typec/Kconfig > > +++ b/drivers/usb/typec/Kconfig > > @@ -2,6 +2,7 @@ > > > > menuconfig TYPEC > > tristate "USB Type-C Support" > > + select MULTIPLEXER > > help > > USB Type-C Specification defines a cable and connector for USB where > > only one type of plug is supported on both ends, i.e. there will not > > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c > > index 464330776cd6..05e6ed217620 100644 > > --- a/drivers/usb/typec/mux.c > > +++ b/drivers/usb/typec/mux.c > > @@ -13,6 +13,7 @@ > > #include <linux/mutex.h> > > #include <linux/property.h> > > #include <linux/slab.h> > > +#include <linux/mux/consumer.h> > > > > #include "class.h" > > #include "mux.h" > > @@ -22,6 +23,11 @@ > > struct typec_switch { > > struct typec_switch_dev *sw_devs[TYPEC_MUX_MAX_DEVS]; > > unsigned int num_sw_devs; > > + > > + /* Could handle HighSpeed, SuperSpeed, Sideband switch one time */ > > + struct mux_control *mux_switch; > > + /* 3 state correspond to NONE, NORMAL, REVERSE for all switches */ > > + int mux_states[3]; > > }; > > > > static int switch_fwnode_match(struct device *dev, const void *fwnode) > > @@ -117,6 +123,58 @@ struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode) > > } > > EXPORT_SYMBOL_GPL(fwnode_typec_switch_get); > > > > +static struct typec_switch *mux_control_typec_switch_get(struct device *dev) > > +{ > > + struct typec_switch *sw; > > + struct mux_control *mux; > > + int ret; > > + > > + if (!device_property_present(dev, "mux-controls")) > > + return NULL; > > + > > + sw = kzalloc(sizeof(*sw), GFP_KERNEL); > > + if (!sw) > > + return ERR_PTR(-ENOMEM); > > + > > + mux = mux_control_get(dev, NULL); > > + if (!IS_ERR(mux)) { > > + sw->mux_switch = mux; > > + ret = device_property_read_u32_array(dev, > > + "typec-switch-states", sw->mux_states, 3); > > + if (ret) { > > + kfree(sw); > > + return ERR_PTR(ret); > > + } > > + } else { > > + kfree(sw); > > + return ERR_CAST(mux); > > + } > > + > > + return sw; > > +} > > + > > +/** > > + * typec_switch_get - Find USB Type-C orientation switch > > + * @dev: The device using switch > > + * > > + * Finds a switch used by @dev. Returns a reference to the switch on > > + * success, NULL if no matching connection was found, or > > + * ERR_PTR(-EPROBE_DEFER) when a connection was found but the switch > > + * has not been enumerated yet, or ERR_PTR with a negative errno. > > + */ > > +struct typec_switch *typec_switch_get(struct device *dev) > > +{ > > + struct typec_switch *sw; > > + > > + sw = fwnode_typec_switch_get(dev_fwnode(dev)); > > + if (!sw) > > + /* Try get switch based on mux control */ > > + sw = mux_control_typec_switch_get(dev); > > + > > + return sw; > > +} > > +EXPORT_SYMBOL_GPL(typec_switch_get); > > + > > /** > > * typec_switch_put - Release USB Type-C orientation switch > > * @sw: USB Type-C orientation switch > > @@ -137,6 +195,10 @@ void typec_switch_put(struct typec_switch *sw) > > module_put(sw_dev->dev.parent->driver->owner); > > put_device(&sw_dev->dev); > > } > > + > > + if (sw->mux_switch) > > + mux_control_put(sw->mux_switch); > > + > > kfree(sw); > > } > > EXPORT_SYMBOL_GPL(typec_switch_put); > > @@ -204,6 +266,7 @@ int typec_switch_set(struct typec_switch *sw, > > enum typec_orientation orientation) > > { > > struct typec_switch_dev *sw_dev; > > + struct mux_control *mux; > > unsigned int i; > > int ret; > > > > @@ -218,7 +281,18 @@ int typec_switch_set(struct typec_switch *sw, > > return ret; > > } > > > > - return 0; > > + mux = sw->mux_switch; > > + if (!mux) > > + return 0; > > + > > + ret = mux_control_select(mux, sw->mux_states[orientation]); > > + if (ret) > > + return ret; > > + > > + /* Keep it as it is since idle_state is MUX_IDLE_AS_IS */ > > + ret = mux_control_deselect(mux); > > No, this is also broken. You cannot, in any client driver, rely on a > mux keeping its state unless you keep it selected. As soon as you > deselect it, it might be selected by some other driver. Sure, you > might know that there are no other users of the mux in question, and > you might also know that the idles state is "as-is". But the driver > does not see the bigger picture and has no way of knowing that. So, > it needs to keep the mux selected. > Understood. May be a flag is needed like mdio-mux-multiplexer does. I will improve it in next version. Thanks, Xu Yang > Cheers, > Peter > > > + > > + return ret; > > } > > EXPORT_SYMBOL_GPL(typec_switch_set); > > > > diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h > > index 9292f0e07846..2287e5a5f591 100644 > > --- a/include/linux/usb/typec_mux.h > > +++ b/include/linux/usb/typec_mux.h > > @@ -24,16 +24,13 @@ struct typec_switch_desc { > > void *drvdata; > > }; > > > > + > > +struct typec_switch *typec_switch_get(struct device *dev); > > struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode); > > void typec_switch_put(struct typec_switch *sw); > > int typec_switch_set(struct typec_switch *sw, > > enum typec_orientation orientation); > > > > -static inline struct typec_switch *typec_switch_get(struct device *dev) > > -{ > > - return fwnode_typec_switch_get(dev_fwnode(dev)); > > -} > > - > > struct typec_switch_dev * > > typec_switch_register(struct device *parent, > > const struct typec_switch_desc *desc);
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig index 5defdfead653..73d4976d8148 100644 --- a/drivers/usb/typec/Kconfig +++ b/drivers/usb/typec/Kconfig @@ -2,6 +2,7 @@ menuconfig TYPEC tristate "USB Type-C Support" + select MULTIPLEXER help USB Type-C Specification defines a cable and connector for USB where only one type of plug is supported on both ends, i.e. there will not diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c index 464330776cd6..05e6ed217620 100644 --- a/drivers/usb/typec/mux.c +++ b/drivers/usb/typec/mux.c @@ -13,6 +13,7 @@ #include <linux/mutex.h> #include <linux/property.h> #include <linux/slab.h> +#include <linux/mux/consumer.h> #include "class.h" #include "mux.h" @@ -22,6 +23,11 @@ struct typec_switch { struct typec_switch_dev *sw_devs[TYPEC_MUX_MAX_DEVS]; unsigned int num_sw_devs; + + /* Could handle HighSpeed, SuperSpeed, Sideband switch one time */ + struct mux_control *mux_switch; + /* 3 state correspond to NONE, NORMAL, REVERSE for all switches */ + int mux_states[3]; }; static int switch_fwnode_match(struct device *dev, const void *fwnode) @@ -117,6 +123,58 @@ struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fwnode_typec_switch_get); +static struct typec_switch *mux_control_typec_switch_get(struct device *dev) +{ + struct typec_switch *sw; + struct mux_control *mux; + int ret; + + if (!device_property_present(dev, "mux-controls")) + return NULL; + + sw = kzalloc(sizeof(*sw), GFP_KERNEL); + if (!sw) + return ERR_PTR(-ENOMEM); + + mux = mux_control_get(dev, NULL); + if (!IS_ERR(mux)) { + sw->mux_switch = mux; + ret = device_property_read_u32_array(dev, + "typec-switch-states", sw->mux_states, 3); + if (ret) { + kfree(sw); + return ERR_PTR(ret); + } + } else { + kfree(sw); + return ERR_CAST(mux); + } + + return sw; +} + +/** + * typec_switch_get - Find USB Type-C orientation switch + * @dev: The device using switch + * + * Finds a switch used by @dev. Returns a reference to the switch on + * success, NULL if no matching connection was found, or + * ERR_PTR(-EPROBE_DEFER) when a connection was found but the switch + * has not been enumerated yet, or ERR_PTR with a negative errno. + */ +struct typec_switch *typec_switch_get(struct device *dev) +{ + struct typec_switch *sw; + + sw = fwnode_typec_switch_get(dev_fwnode(dev)); + if (!sw) + /* Try get switch based on mux control */ + sw = mux_control_typec_switch_get(dev); + + return sw; +} +EXPORT_SYMBOL_GPL(typec_switch_get); + /** * typec_switch_put - Release USB Type-C orientation switch * @sw: USB Type-C orientation switch @@ -137,6 +195,10 @@ void typec_switch_put(struct typec_switch *sw) module_put(sw_dev->dev.parent->driver->owner); put_device(&sw_dev->dev); } + + if (sw->mux_switch) + mux_control_put(sw->mux_switch); + kfree(sw); } EXPORT_SYMBOL_GPL(typec_switch_put); @@ -204,6 +266,7 @@ int typec_switch_set(struct typec_switch *sw, enum typec_orientation orientation) { struct typec_switch_dev *sw_dev; + struct mux_control *mux; unsigned int i; int ret; @@ -218,7 +281,18 @@ int typec_switch_set(struct typec_switch *sw, return ret; } - return 0; + mux = sw->mux_switch; + if (!mux) + return 0; + + ret = mux_control_select(mux, sw->mux_states[orientation]); + if (ret) + return ret; + + /* Keep it as it is since idle_state is MUX_IDLE_AS_IS */ + ret = mux_control_deselect(mux); + + return ret; } EXPORT_SYMBOL_GPL(typec_switch_set); diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h index 9292f0e07846..2287e5a5f591 100644 --- a/include/linux/usb/typec_mux.h +++ b/include/linux/usb/typec_mux.h @@ -24,16 +24,13 @@ struct typec_switch_desc { void *drvdata; }; + +struct typec_switch *typec_switch_get(struct device *dev); struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode); void typec_switch_put(struct typec_switch *sw); int typec_switch_set(struct typec_switch *sw, enum typec_orientation orientation); -static inline struct typec_switch *typec_switch_get(struct device *dev) -{ - return fwnode_typec_switch_get(dev_fwnode(dev)); -} - struct typec_switch_dev * typec_switch_register(struct device *parent, const struct typec_switch_desc *desc);
Some dedicated mux block can use existing mux controller as a mux provider, typec port as a consumer to select channel for orientation switch, this can be an alternate way to control typec orientation switch. Also, one mux controller could cover highspeed, superspeed and sideband use case one time in this implementation. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- Changes since v1: - add build dependence (select MULTIPLEXER) - improve Multiplexer control logic drivers/usb/typec/Kconfig | 1 + drivers/usb/typec/mux.c | 76 ++++++++++++++++++++++++++++++++++- include/linux/usb/typec_mux.h | 7 +--- 3 files changed, 78 insertions(+), 6 deletions(-)