usb: typec: Add a sysfs node to manage port type

PATCH Superseded Badhri Jagan Sridharan 3 participants 5 messages May 22, 2017 – May 23, 2017

Subsystems (2)

USB USB Type-C class

lore

Share & tools

lore

Badhri Jagan Sridharan

author

2017-05-22 20:06 UTC #0 lore raw

PATCH 1/1 usb: typec: Add a sysfs node to manage port type

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 <[email protected]>

---
 Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
 drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
 include/linux/usb/typec.h                   |  4 +++
 3 files changed, 57 insertions(+)

Documentation/ABI/testing/sysfs-class-typec +13 -0

index d4a3d23eb09c..853b2ef73641 100644
@@ -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 <[email protected]>

drivers/usb/typec/typec.c +40 -0

index 89e540bb7ff3..684a13bb744d 100644
@@ -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",
+};
+
 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);
+
 static const char * const typec_pwr_opmodes[] = {
 	[TYPEC_PWR_MODE_USB]	= "default",
 	[TYPEC_PWR_MODE_1_5A]	= "1.5A",

@@ -1035,6 +1074,7 @@ static struct attribute *typec_attrs[] = {
 	&dev_attr_usb_power_delivery_revision.attr,
 	&dev_attr_usb_typec_revision.attr,
 	&dev_attr_vconn_source.attr,
+	&dev_attr_port_type.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(typec);

include/linux/usb/typec.h +4 -0

index ec78204964ab..5badf6f66479 100644
@@ -190,6 +190,7 @@ struct typec_partner_desc {
  * @pr_set: Set Power Role
  * @vconn_set: Set VCONN Role
  * @activate_mode: Enter/exit given Alternate Mode
+ * @port_type_set: Set port type
  *
  * Static capabilities of a single USB Type-C port.
  */

@@ -214,6 +215,9 @@ struct typec_capability {
 
 	int		(*activate_mode)(const struct typec_capability *,
 					 int mode, int activate);
+
+	int		(*port_type_set)(const struct typec_capability *,
+					enum typec_port_type);
 };
 
 /* Specific to try_role(). Indicates the user want's to clear the preference. */

Signature · 1 line

2.13.0.303.g4ebf302169-goog

Heikki Krogerus

maintainer

2017-05-23 10:48 UTC #1 lore raw

Hi,

Quoted 41 lines · Badhri Jagan Sridharan

Signed-off-by Badhri Jagan Sridharan <[email protected]>

---
 Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
 drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
 include/linux/usb/typec.h                   |  4 +++
 3 files changed, 57 insertions(+)

Documentation/ABI/testing/sysfs-class-typec +12 -0

index d4a3d23eb09c..853b2ef73641 100644
@@ -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 <[email protected]>

drivers/usb/typec/typec.c +0 -0

index 89e540bb7ff3..684a13bb744d 100644

drivers/usb/typec/typec.c static const char * const typec_data_roles[] = { · line 796

Quoted 8 lines

drivers/usb/typec/typec.c +5 -0

@@ -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?

Quoted 29 lines

drivers/usb/typec/typec.c +22 -0

 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;
+}

drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 966

Quoted 10 lines

drivers/usb/typec/typec.c +10 -0

+
+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,

Signature · 1 line

heikki

Guenter Roeck

2017-05-23 13:17 UTC #2 lore raw

Quoted 97 lines · Heikki Krogerus

Hi,

Quoted 41 lines · Badhri Jagan Sridharan

Signed-off-by Badhri Jagan Sridharan <[email protected]>

---
  Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
  drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
  include/linux/usb/typec.h                   |  4 +++
  3 files changed, 57 insertions(+)

Documentation/ABI/testing/sysfs-class-typec +12 -0

index d4a3d23eb09c..853b2ef73641 100644
@@ -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 <[email protected]>

drivers/usb/typec/typec.c +0 -0

index 89e540bb7ff3..684a13bb744d 100644

drivers/usb/typec/typec.c static const char * const typec_data_roles[] = { · line 796

Quoted 8 lines

drivers/usb/typec/typec.c +5 -0

@@ -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",
+};

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

Quoted 36 lines

drivers/usb/typec/typec.c +29 -0

  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);
+

Quoted 9 lines

drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 966
Quoted 3 lines
drivers/usb/typec/typec.c +3 -0
+	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 ?

Quoted 3 lines

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

Heikki Krogerus

maintainer

2017-05-23 13:43 UTC #3 lore raw

Quoted 94 lines · Guenter Roeck

Quoted 93 lines · Heikki Krogerus

Hi,

Quoted 39 lines · Badhri Jagan Sridharan

Signed-off-by Badhri Jagan Sridharan <[email protected]>

---
  Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
  drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
  include/linux/usb/typec.h                   |  4 +++
  3 files changed, 57 insertions(+)

Documentation/ABI/testing/sysfs-class-typec +12 -0

index d4a3d23eb09c..853b2ef73641 100644
@@ -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 <[email protected]>

drivers/usb/typec/typec.c +0 -0

index 89e540bb7ff3..684a13bb744d 100644

drivers/usb/typec/typec.c static const char * const typec_data_roles[] = { · line 795

Quoted 7 lines

drivers/usb/typec/typec.c +5 -0

@@ -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",
+};

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

Quoted 35 lines

drivers/usb/typec/typec.c +29 -0

  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);
+

Quoted 8 lines

Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 966
Quoted 3 lines
drivers/usb/typec/typec.c +3 -0
+	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.

Sorry, not the API..

Quoted 3 lines

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 mean, we should not overwrite the type member in typec_capability. DRP capable port is still DRP capable even if we fix it to DFP or UFP. So the active, fixed role, should be stored to struct typec_port.

Thanks,

Signature · 1 line

heikki

Guenter Roeck

2017-05-23 14:06 UTC #4 lore raw

Quoted 106 lines · Heikki Krogerus

Quoted 94 lines · Guenter Roeck
Quoted 93 lines · Heikki Krogerus
Hi,
Quoted 39 lines · Badhri Jagan Sridharan
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 <[email protected]>
---
   Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
   drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
   include/linux/usb/typec.h                   |  4 +++
   3 files changed, 57 insertions(+)
Documentation/ABI/testing/sysfs-class-typec +12 -0
index d4a3d23eb09c..853b2ef73641 100644
@@ -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 <[email protected]>
drivers/usb/typec/typec.c +0 -0
index 89e540bb7ff3..684a13bb744d 100644
drivers/usb/typec/typec.c static const char * const typec_data_roles[] = { · line 795
Quoted 7 lines
drivers/usb/typec/typec.c +5 -0
@@ -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?
Quoted 35 lines
drivers/usb/typec/typec.c +29 -0
   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);
+
Quoted 8 lines
Quoted 8 lines
drivers/usb/typec/typec.c static ssize_t power_role_show(struct device *dev, · line 966
Quoted 3 lines
drivers/usb/typec/typec.c +3 -0
+	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.
Sorry, not the API..

Quoted 8 lines

Quoted 3 lines
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 mean, we should not overwrite the type member in typec_capability. DRP capable port is still DRP capable even if we fix it to DFP or UFP. So the active, fixed role, should be stored to struct typec_port.

Yes, you are right. Makes sense.

Thanks,
Guenter