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

[Guenter Roeck]

On 06/03/2016 06:21 AM, Heikki Krogerus wrote:
> 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?

Not really. Like in the Google case, one end will implement sending the firmware,
the other end will implement receiving it and writing it to flash (or whatever).
Which is which isn't currently made visible to user space. I suspect that
other "intelligent" devices like Apple's multi-function adapter do the same,
though obviously I don't really know what the two VDO modes do on the apple
adapter.

I'll have to have some ABI to user space for the alternate mode, for example
to send the firmware file to the kernel. I am not there yet, though, so I
don't really know exactly how that will look like. Most likely it is going to be
added sysfs attributes in the mode device (eg usbc0.svid:18d1/mode0/firmware).

> > 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

Side note: I didn't provide a description/name for the modes, because that
would result in something like usbc0.DisplayPort/ instead of usbc0.svid:ff01/,
and I prefer a consistent ABI. Since this _is_ part of the ABI, would it make
sense to standardize on names for modes in sysfs ? For example, how should
a "Display Port" mode directory be named ? It doesn't sound good if I
use "usbc0.svid:ff01", someone else uses "usbc0.DisplayPort", and yet
someone else uses "usbc0.displayport".

Also, do we at some point need to standardize the ABI for the standard
alternate modes such as DisplayPort (if there are any - again I am not
there yet) ?

> > 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.

Sounds good to me. Many other subsystems do the same, ie create the subsystem
device(s) during registration with the subsystem, so this is in line with other
kernel code.

Should I send you a follow-up patch on top of yours ?

Thanks,
Guenter