Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

On Thu, Jun 02, 2016 at 09:12:19AM -0700, Guenter Roeck wrote:
> On Thu, Jun 02, 2016 at 01:18:53PM +0300, Heikki Krogerus wrote:
> > On Wed, Jun 01, 2016 at 04:29:26PM -0700, Guenter Roeck wrote:
> > > On Wed, Jun 01, 2016 at 11:26:09AM +0200, Oliver Neukum wrote:
> > > > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > > > > Just noticed that the "active" file is for now read only, but it needs
> > > > > to be changed to writable. That file will of course provide means for
> > > > > the userspace to Exit and Enter modes. But please note that the
> > > > > responsibility of the dependencies between the modes, say, if a plug
> > > > > needs to be in one mode or the other in order for the partner to enter
> > > > > some specific mode, will fall on the Alternate Mode specific drivers
> > > > > once we have the altmode bus. I remember there were concerns about
> > > > > this in the original thread.
> > > > 
> > > > There's one thing we haven't touched upon yet. And I cannot really find
> > > > an answer in the spec.
> > > > 
> > > > What do we do if we return from S4 or S3? I think we need to restore
> > > > the ALternate Mode because our display may be running over that
> > > > Alternate Mode.
> > > > If we want to support USB persist we also need to restore data role
> > > > after S4.
> > > > 
> > > I don't have an answer ... but another interesting question.
> > > 
> > > How do we distinguish between alternate modes supported by a host vs.
> > > alternate modes supported by a sink ? typec_capability includes a pointer
> > > to alternate modes supportedf by the connector, but it is not clear if
> > > those are alternate modes supported as host, or alternate modes supported
> > > as device, or alternate modes supported by both.
> > > 
> > > This doesn't matter much if only a fixed role is supported, but it does matter
> > > for dual role ports. A laptop will typically only support DisplayPort as host,
> > > for example.
> > 
> > The DP alternate mode spec actually separates the display role from
> > Type-C role. A laptop most likely would only support the modes for
> > display host roles, but if the port was DRP port then it would still
> > do so in both Type-C roles.
> > 
> > So basically, even if the display was Type-C host, it would still work
> > as a display when attached to the laptop.
> > 
> > > Any idea ?
> > 
> > I'm actually not sure this is a problem.
> > 
> Yes, this was a bad example, since the DisplayPort mode vdo includes a flag
> indicating if the port supports source, sink, or both.

I meant that in case of DP alternate mode, there should not be a
problem.

> Let's use a different example:
> Google devices (such as power adapters) have mode '1' for firmware upgrades.
> Obviously hosts will support that, but what should the host advertise if it
> is configured as sink ?
> 
> Maybe this is just my personal confusion, and there is no real problem.
> It might as well be that the Google mode VDO _should_ include a flag
> indicating if the port supports updating the partner, and/or if it supports
> being updated. For now I'll just assume that this is the case.

Well, do you think we can rely on always being able to get this detail
from VDO?

> Something else, which goes back into the symlink question. If I create the
> alternate mode devices before calling typec_register_port(), the devices won't
> have a parent and don't show up in the class directory. You previously solved
> that with the symlink. I am trying to solve it in my current code by calling
> typec_register_altmodes() from typec_register_port() - primarily because I
> don't really want to duplicate all the device creation code in my driver.
> 
> In my test case, this gives me
>     /sys/class/type-c/usbc0/
> 	usbc0.svid:18d1
> 	usbc0.svid:18d1/mode0
> 	usbc0.svid:18d1/mode0/vdo
> 	usbc0.svid:18d1/mode0/description
> 	usbc0.svid:18d1/mode0/active
> 	...
> 	usbc0.svid:ff01
> 	usbc0.svid:ff01/mode0/vdo
> 	usbc0.svid:ff01/mode0/description
> 	usbc0.svid:ff01/mode0/active
> 
> in addition to
>     /sys/class/type-c/usbc0/
> 	usbc0-partner/usbc0-partner.svid:05ac
> 	usbc0-partner/usbc0-partner.svid:05ac/mode0
> 	usbc0-partner/usbc0-partner.svid:05ac/mode0/vdo
> 	usbc0-partner/usbc0-partner.svid:05ac/mode0/description
> 	usbc0-partner/usbc0-partner.svid:05ac/mode0/active
> 	usbc0-partner/usbc0-partner.svid:05ac/mode1
> 	usbc0-partner/usbc0-partner.svid:05ac/mode1/vdo
> 	usbc0-partner/usbc0-partner.svid:05ac/mode1/description
> 	usbc0-partner/usbc0-partner.svid:05ac/mode1/active
> 	...
> 	usbc0-partner/usbc0-partner.svid:ff01
> 	usbc0-partner/usbc0-partner.svid:ff01/mode0
> 	usbc0-partner/usbc0-partner.svid:ff01/mode0/vdo
> 	usbc0-partner/usbc0-partner.svid:ff01/mode0/description
> 	usbc0-partner/usbc0-partner.svid:ff01/mode0/active
> 
> (when connecting the Apple adapter), which is exactly what I would expect to see.
> 
> Is this sensible ? Do we have a reason for expecting the alternate mode
> _devices_ to be created (without parent) when calling typec_register_port() ?

So if you would prefer that the class code takes care of creating the
alternate modes when typec_register_port() is called, I'm fine with
that too. Let's make it so.


Thanks,

-- 
heikki