Re: [PATCHv6 1/3] usb: USB Type-C connector class

[Guenter Roeck]

Hello Heikki,

On Mon, Aug 29, 2016 at 05:07:39PM +0300, Heikki Krogerus wrote:
> Hi Guenter,
> 
> > > Overall this is quite vague and, especially for chargers, most of the time
> > > misses the point.
> > > 
> > > I would really prefer if we could stay closer to the specification in this
> > > case, and not try to merge multiple orthogonal attributes into one.
> > 
> > OK. So what would you propose?
> 
> I'm actually only conserned about the accessory case, as there we are
> really not a source/sink/DRP, nor are we DPF/UFP/DRD. Should we use
> this attribute to only express if the type of the partner is "normal"
> or an accessory?
> 

We currently have three attributes to cover accessory modes.

supported_accessory_modes
	Lists the Accessory Modes, defined in the USB Type-C
	specification, the port supports.

	[ This is a bit vague. I think we should list the actual strings.
	  The modes are called "Audio Adapter Accessory Mode" and "Debug
	  Accessory Mode", yet the reported text is "Audio" and "Debug".
	  Also, "Digital Audio" isn't supported as of specification revision
	  1.2. So the strings doesn't exactly follow the specification. ]

accessory
	Shows the name of the Accessory Mode. The Accessory Modes are
	defined in USB Type-C Specification.

type
	Shows the type of the partner.

One of the possible accessory modes is TYPEC_ACCESSORY_NONE.

If you are only interested in accessory mode support, maybe we don't need
the 'type' attribute at all. We could make the 'accessory' attribute always
visible and display one of "none", "Audio", "Debug", or "Digital Audio".
It might also make sense to rename the attribute to "accessory_mode".

On a side note, while looking into this, I noticed the following:

+       if (port->cap->accessory)
+               for (accessory = port->cap->accessory, i = 0;
+                    i < port->cap->num_accessory; accessory++, i++)
+                       ret += sprintf(buf, "%s\n",
+                                      typec_accessory_modes[*accessory]);

This means the list of supported accessories always starts with ", ".

Thanks,
Guenter