> -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Sent: Wednesday, May 26, 2021 5:16 PM > To: Jun Li <jun.li@xxxxxxx> > Cc: robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; > linux@xxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx> > > > Sent: Friday, May 21, 2021 4:38 PM > > > To: Jun Li <jun.li@xxxxxxx> > > > Cc: robh+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx; > > > gregkh@xxxxxxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx; > > > linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; > > > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > > > 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
