Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Fri, Jul 01, 2016 at 10:38:03AM +0300, Heikki Krogerus wrote:
> On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> > On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > > The purpose of USB Type-C connector class is to provide
> > > unified interface for the user space to get the status and
> > > basic information about USB Type-C connectors on a system,
> > > control over data role swapping, and when the port supports
> > > USB Power Delivery, also control over power role swapping
> > > and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > ---
> > [ ... ]
> > 
> > > +
> > > +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> > > +Date:		June 2016
> > > +Contact:	Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > +Description:
> > > +		Shows if the partner supports USB Power Delivery.
> > > +		- 1 if USB Power Delivery is supported
> > > +		- 0 when it's not
> > > +
> > > +
> > > +What:		/sys/class/typec/<port>-partner/id_header_vdo
> > > +Date:		June 2016
> > > +Contact:	Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > +Description:
> > > +		If the partner supports USB Power Deliver, shows the VDO
> > > +		returned from Discover Identity USB Power Delivery command.
> > > +
> > > +		If the partner does not support USB Power Delivery, the
> > > +		attribute is hidden.
> > > +
> > On second thought, and after merging the code (and realizing that I don't get
> > the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> > is that useful here. It also doesn't provide all information either.
> 
> Yeah, I don't think it's available with UCSI either..
> 
> > Would it make sense to split it into multiple decoded attributes ?
> > 
> > - vendor-id
> >   [bit 0..15 of ID header VDO]
> > - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
> > 	passive/active for cable plugs)
> >   Might map into typec_partner_type, but I don't see a 1:1 match.
> >   [bit 27..29 of ID header VDO]
> > - alternate-mode-supported
> >   [bit 26 of ID header VDO]
> > - capabilities (ufp, dfp, drp, none (?))
> >   [bit 30/31 of ID header VDO]
> > - product-id
> >   [bit 16..31 of Product VDO]
> > 
> > Does this make any sense ?
> 
> I feel a bit uncomfortable exposing so many attribute like that which
> will give details that we can only know when both ends support USB
> PD...
> 
> Here's my proposal for this:
> There has to be a special group for these devices, partners and
> cables, a directory named "pd" or "power_deliver" (or something like
> that), which exposes those. That group will not be responsibility of
> this class, but instead the PD framework that you are working on
> (right?).
> 
> So basically, in this class we will not expose those attributes, and
> we'll also get rid of the "supports_usb_power_delivery" attribute (the
> "pd" groups should only exist when USB PD is actually supported).
> 
> Initially that group needs to be assigned to the "groups" member of
> struct device of the partners in the drivers before they are
> registered. That member is meant for optional attribute groups, but we
> can later think of something better, a way perhaps to bind that group
> the device types "groups" member for partners, cables, etc. or
> something similar in case using the "groups" member of struct device
> is not ideal. But initially we can use that.
> 
> How would that sound to you?
> 
Quite frankly I don't know. At first glance it sounds like a bad idea - it means
that PD specific atttributes would not be standardized, which would reduce the
valus of the class code substantially. I'll have to think about this.

At the same time I'll have to find a somewhat working solution for the locking
issues. I'll have to focus on that for the time being, because it affects
the core architecture of our code. So I don't really have the time to look
into attribute support right now.

Guenter