Hi Peter,
> -----Original Message-----
> From: Peter Rosin <peda@xxxxxxxxxx>
> Sent: Wednesday, August 24, 2022 7:51 PM
> To: Xu Yang <xu.yang_2@xxxxxxx>; heikki.krogerus@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; Jun Li <jun.li@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > ---
> > 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);
