Re: [PATCH] usb: typec: Add a sysfs node to manage port type

[Heikki Krogerus]

Hi,

On Mon, May 22, 2017 at 01:05:42PM -0700, Badhri Jagan Sridharan wrote:
> User space applications in some cases have the need to enforce a
> specific port type(DFP/UFP/DRP). This change allows userspace to
> attempt setting the desired port type. Low level drivers can
> however reject the request if the specific port type is not supported.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
> ---
>  Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
>  drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
>  include/linux/usb/typec.h                   |  4 +++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index d4a3d23eb09c..853b2ef73641 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -73,6 +73,19 @@ Description:
>  
>  		Valid values: source, sink, none (to remove preference)
>  
> +What:           /sys/class/typec/<port>/port_type
> +Date:           May 2017
> +Description:
> +		Indicates the type of the port. This attribute can be used for
> +		requesting a change in the port type. Port type change is
> +		supported as a synchronous operation, so write(2) to the
> +		attribute will not return until the operation has finished.
> +
> +		Valid values:
> +		- DRP
> +		- DFP
> +		- UFP
> 
>  What:		/sys/class/typec/<port>/supported_accessory_modes
>  Date:		April 2017
>  Contact:	Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> index 89e540bb7ff3..684a13bb744d 100644
> --- a/drivers/usb/typec/typec.c
> +++ b/drivers/usb/typec/typec.c
> @@ -789,6 +789,12 @@ static const char * const typec_data_roles[] = {
>  	[TYPEC_HOST]	= "host",
>  };
>  
> +static const char * const typec_port_types[] = {
> +	[TYPEC_PORT_DFP] = "dfp",
> +	[TYPEC_PORT_UFP] = "ufp",
> +	[TYPEC_PORT_DRP] = "drp",
> +};

The meaning of those abbreviations has changed in every version of the
spec since v1.0 which makes me a bit uncomfortable using them with the
attributes. In USB Type-C specification v1.2, DRP now means
Dual-Role-Power, but DFP and UFP are used with USB data operation.

I would prefer "source, "sink" and "drp". Actually, I don't even like
"drp". How about "dual" instead?

>  static ssize_t
>  preferred_role_store(struct device *dev, struct device_attribute *attr,
>  		     const char *buf, size_t size)
> @@ -926,6 +932,39 @@ static ssize_t power_role_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(power_role);
>  
> +static ssize_t
> +port_type_store(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	int ret;
> +
> +	if (!port->cap->port_type_set) {
> +		dev_dbg(dev, "changing port type not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = sysfs_match_string(typec_port_types, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = port->cap->port_type_set(port->cap, ret);
> +	if (ret)
> +		return ret;
> +
> +	return size;
> +}
> +
> +static ssize_t
> +port_type_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +
> +	return sprintf(buf, "%s\n", typec_port_types[port->cap->type]);
> +}
> +static DEVICE_ATTR_RW(port_type);

This doesn't tell the user the capabilities of the port. All the
supported roles should be listed here like with the other attributes,
the active one in brackets. This probable means we need a small
addition/change to the API too.

I do like the idea of being able to fix the role, assuming others are
OK with it too.


Thanks,

-- 
heikki