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

[Jun Li <jun.li@nxp.com>]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Wednesday, May 26, 2021 5:16 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; gregkh@linuxfoundation.org;
> linux@roeck-us.net; linux-usb@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 3/4] usb: typec: add typec orientation switch support
> via mux controller
> 
> On Tue, May 25, 2021 at 11:46:18AM +0000, Jun Li wrote:
> > Hi Heikki,
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Friday, May 21, 2021 4:38 PM
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: robh+dt@kernel.org; shawnguo@kernel.org;
> > > gregkh@linuxfoundation.org; linux@roeck-us.net;
> > > linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > > Subject: Re: [PATCH 3/4] usb: typec: add typec orientation switch
> > > support via mux controller
> > >
> > > Hi,
> > >
> > > On Thu, May 20, 2021 at 03:33:36PM +0300, Heikki Krogerus wrote:
> > > > Why not just do that inside fwnode_typec_switch_get() and handle
> > > > the whole thing in drivers/usb/typec/mux.c (or in its own file if
> > > > you prefer)?
> > > >
> > > > You'll just need to register a "wrapper" Type-C switch object for
> > > > the OF mux controller, but that should not be a problem. That way
> > > > you don't need to export any new functions, touch this file or
> > > > anything else.
> > >
> > > I wrote a bit of code just to see how that would look. I'm attaching
> > > you the hack I made. I guess something like that would not be too bad.
> > > A wrapper is probable always a bit clumsy, but I'm not sure that in
> > > this case it's a huge problem. Of course if there are any better
> > > ideas, let's here them :-)
> >
> > Thanks for your patch, I am pasting the patch as below.
> >
> > seems we need consider more than that.
> >
> > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> > index a0adb8947a301..d85231b2fe10b 100644
> > --- a/drivers/usb/typec/Makefile
> > +++ b/drivers/usb/typec/Makefile
> > @@ -1,6 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_TYPEC)		+= typec.o
> >  typec-y				:= class.o mux.o bus.o port-mapper.o
> > +typec-$(MULTIPLEXER)		+= of_mux.o
> >  obj-$(CONFIG_TYPEC)		+= altmodes/
> >  obj-$(CONFIG_TYPEC_TCPM)	+= tcpm/
> >  obj-$(CONFIG_TYPEC_UCSI)	+= ucsi/
> > diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c index
> > 9da22ae3006c9..282622276d97b 100644
> > --- a/drivers/usb/typec/mux.c
> > +++ b/drivers/usb/typec/mux.c
> > @@ -63,6 +63,9 @@ struct typec_switch *fwnode_typec_switch_get(struct
> > fwnode_handle *fwnode)
> >
> >  	sw = fwnode_connection_find_match(fwnode, "orientation-switch", NULL,
> >  					  typec_switch_match);
> > +	if (!sw)
> > +		sw = of_switch_register(fwnode);
> > +
> >
> > How to support multiple typec_switch_get() for of_mux case?
> > the second call to fwnode_typec_switch_get() will get the switch via
> > fwnode_connection_find_match()? This means we still need a property
> > "orientation-switch" for mux controller node, this seems not the
> > expected way for a mux consumer, even with this property, we will get
> > a EPROBE_DEFER for the first call.
> >
> > If we use mux_control_get() for multiple caller/consumer, then we need
> > some other device as input.
> >
> > typec_switch object looks to me is a provider, if we create and
> > maintain it in consumer side, I have not come out a better way:-).
> 
> Sorry, but can we rewind a bit: Why can't you just register the orientation
> switch from your mux driver and be done with it? You should then be able
> to use OF graph, and no special bindings should be needed, no?

So we still need a special property for OF graph per discussion on another
thread(use device type other than device name for match), and this has
to be a mux controller core binding for possible different mux chips
(GPIO/MMIO...), register a typec switch if this property exist, but this
is the user specific thing from mux controller point view, I feed this
is again against DT binding's expectation.

> 
> If you want to reuse a mux-controller driver, then you do need to modify
> it (but only a little), and what ever mux-controller specific bindings there
> are, you will not use those when the mux supplies the orientation switching
> function, instead you'll use the OF graph for that. But surely that is not
> a problem?
> 
> The mux-controller framework expects the "consumers" of the muxes to
> understand the final function that the mux is used for. The Type-C "mux"
> framework (I should not even talk about muxes with those) works the other
> way around. 

Fully agree.

> The driver for the component that supplies the orientation switch
> function must understand that it is handling that function, and there is
> a good reason for doing it that way with the USB Type-C switches. 

I understand yes if the switch is only part function of the driver.
  
> The
> orientation switch for example quite simply is _not_ always a mux. In fact,
> it's seems to be rarely a mux these days. With USB4 for example the orientation
> is handled almost always by the first on-board retimer.

If the mux is only part function of a new driver, use the tyepc
"mux" framework and create new binding for the new driver is fine.

But if the typec switch control need a dedicated driver to handle,
on DT platforms, now mux-controller is the only proposed way to go
from binding point view. I am not sure if my case is a normal HW
design, but I guess I should not the only user of this kind of
situation.

> 
> There are actually also some technical reasons why Hans failed to get the
> mux-controller thing to work, which is the original reason why we introduced
> the dedicated framework for the Type-C "muxes" (I really should stop talking
> about muxes), but I don't remember what was the reason.

I checked the patches Hans did, that was mainly to address non-DT
platform, I don't see a clear reason why it can't fit DT platform,
maybe I missed something.

+Hans, It would be great if you can comment on this, thanks.

> 
> In any case, to summarise: the orientation switch is a function. A mux is
> a device that can supply that function, and if it does, then the driver for
> it really needs to register the dedicated orientation switch.

Understand your point, if register the dedicated orientation switch is a must,
I feel using general mux control can't make much sense.

Thanks
Li Jun 
> 
> thanks,
> 
> --
> heikki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel