Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1648029

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

Show all headers | View raw

On 05/23/2017 03:46 AM, Heikki Krogerus wrote:
> 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@google.com>
>> ---
>>   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@linux.intel.com>
>> 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.
> 
typec_capability already lists the port type. Presumably it can be
restricted to TYPEC_PORT_DFP or TYPEC_PORT_UFP only if it is reported
as TYPEC_PORT_DRP. Am I missing something ?

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

I am definitely ok with it.

Thanks,
Guenter

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan <badhri@google.com> - 2017-05-22 22:10 +0200
  Re: [PATCH] usb: typec: Add a sysfs node to manage port type Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-05-23 12:50 +0200
    Re: [PATCH] usb: typec: Add a sysfs node to manage port type Guenter Roeck <linux@roeck-us.net> - 2017-05-23 15:20 +0200
      Re: [PATCH] usb: typec: Add a sysfs node to manage port type Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-05-23 15:50 +0200
        Re: [PATCH] usb: typec: Add a sysfs node to manage port type Guenter Roeck <linux@roeck-us.net> - 2017-05-23 16:10 +0200

csiph-web