Re: [PATCHv14 2/3] usb: USB Type-C connector class

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

Hi guys,

On Thu, Jan 05, 2017 at 05:54:02PM +0200, Mika Westerberg wrote:
> > +static ssize_t
> > +typec_altmode_roles_show(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct typec_mode *mode = container_of(attr, struct typec_mode,
> > +					       roles_attr);
> > +	ssize_t ret;
> > +
> > +	switch (mode->roles) {
> > +	case TYPEC_PORT_DFP:
> > +		ret =  sprintf(buf, "source\n");
> 
> Extra space after '='.
> 
> > +		break;
> > +	case TYPEC_PORT_UFP:
> > +		ret = sprintf(buf, "sink\n");
> > +		break;
> > +	case TYPEC_PORT_DRP:
> > +	default:
> > +		ret = sprintf(buf, "source\nsink\n");
> 
> I wonder if "source sink" instead is better?  Along the lines of
> /sys/power/state.
> 
> Then you can print "[source] sink" when source is selected and so on.

That is more or less how I originally proposed how we list the roles
in general. I introduced the separate "current_*_role" and
"supported_*_roles" attribute files because somebody wanted them. I
don't remember the reason why they were preferred to be in separate
attribute files.

Oliver! Guenter! Do we really need to list the current and supported
roles in separate attribute files? Can't we just have the "power_role"
and "data_role" attribute files for the ports instead of the separate
"supported_*_roles" and "current_*_role", and show the current role
like Mika proposes? I definitely would prefer it that way because it
is similar style used in other places like Mike pointed out.

And since we are talking about the ABI, can we also change the listing
of the accessory mode back to just "audio" and "debug" like I
originally had it? I don't remember who and why wanted it to be
changed to "Audio Adapter Accessory Mode" and "Debug Accessory Mode",
but it differs from the style we list the other details.


Thanks,

-- 
heikki