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

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

Hi Guenter,

On Mon, Aug 29, 2016 at 11:50:49AM -0700, Guenter Roeck wrote:
> 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. ]

I'm fine if we want to use more precise strings.

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

That works for me.

How about if I add the "supports_usb_power_delivery" attribute for the
partners instead to give some details about them. Any objections?

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

Where does it print ", "?

I'm not sure what is wrong here, but I'll update this code in any
case. I'll change the accessory member in typec_capability into fixed
size array to make it easier to deal with for now.


Thanks,

-- 
heikki