linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Jun Li <jun.li@nxp.com>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: "robh+dt@kernel.org" <robh+dt@kernel.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux@roeck-us.net" <linux@roeck-us.net>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"peda@axentia.se" <peda@axentia.se>,
	Hans de Goede <hdegoede@redhat.com>
Subject: RE: [PATCH 3/4] usb: typec: add typec orientation switch support via mux controller
Date: Mon, 31 May 2021 11:58:48 +0000	[thread overview]
Message-ID: <VI1PR04MB5935CF87B5DB6BC62BD69B10893F9@VI1PR04MB5935.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <YK4R4q1cZqQDS1qy@kuha.fi.intel.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

  reply	other threads:[~2021-05-31 12:00 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-19  7:14 [PATCH 0/4] typec switch " Li Jun
2021-05-19  7:14 ` [PATCH 1/4] dt-bindings: connector: Add typec orientation switch properties Li Jun
2021-05-21  1:30   ` Rob Herring
2021-05-25 11:48     ` Jun Li
2021-05-19  7:14 ` [PATCH 2/4] usb: typec: use typec cap fwnode's of_node for typec port Li Jun
2021-05-20 12:38   ` Heikki Krogerus
2021-05-19  7:14 ` [PATCH 3/4] usb: typec: add typec orientation switch support via mux controller Li Jun
2021-05-20 12:33   ` Heikki Krogerus
2021-05-21  8:37     ` Heikki Krogerus
2021-05-25 11:46       ` Jun Li
2021-05-26  9:16         ` Heikki Krogerus
2021-05-31 11:58           ` Jun Li [this message]
2021-05-21 13:02     ` Jun Li
2021-05-19  7:14 ` [PATCH 4/4] arm64: dts: imx8mp-evk: enable usb0 with typec connector Li Jun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR04MB5935CF87B5DB6BC62BD69B10893F9@VI1PR04MB5935.eurprd04.prod.outlook.com \
    --to=jun.li@nxp.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=peda@axentia.se \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --subject='RE: [PATCH 3/4] usb: typec: add typec orientation switch support via mux controller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).