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

[Heikki Krogerus]

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?


Thanks,

-- 
heikki