RE: [EXT] Re: [PATCH 3/4] usb: typec: mux: add typec orientation switch support via mux controller

[Xu Yang <xu.yang_2@nxp.com>]

Hi Peter,

> -----Original Message-----
> From: Peter Rosin <peda@axentia.se>
> Sent: Tuesday, August 23, 2022 2:13 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 3/4] usb: typec: mux: add typec orientation switch
> support via mux controller
> 
> Caution: EXT Email
> 
> Hi!
> 
> 2022-08-22 at 17:35, 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.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/typec/mux.c       | 74
> +++++++++++++++++++++++++++++++++++
> >  include/linux/usb/typec_mux.h |  7 +---
> >  2 files changed, 76 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> > index 464330776cd6..5ee960fb668d 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,6 +281,17 @@ int typec_switch_set(struct typec_switch *sw,
> >                       return ret;
> >       }
> >
> > +     mux = sw->mux_switch;
> > +     if (mux) {
> > +             ret = mux_control_deselect(mux);
> 
> This is broken. Please read the documentation for mux_control_select and
> mux_control_deselect. Every call to mux_control_deselect *must* be paired
> with a *successful* call to mux_control_select. Here, mux_control_deselect
> is called unconditionally (as long as a mux is configured).

Okay, I will improve it in v2.

> 
> Yes, agreed, that is indeed awkward (and fragile). But those are the rules.
> (the mux interface was not designed for long-time selects like this)
> 

Understood. I'll follow the rules. 

Thanks,
Xu Yang

> Cheers,
> Peter
> 
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = mux_control_select(mux, sw->mux_states[orientation]);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >  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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel