Re: [PATCHv6 1/3] usb: USB Type-C connector class
From: Heikki Krogerus
Date: Tue Aug 30 2016 - 09:11:36 EST
Hi,
On Tue, Aug 30, 2016 at 02:49:50PM +0300, Heikki Krogerus wrote:
> On Tue, Aug 30, 2016 at 01:16:46PM +0200, Oliver Neukum wrote:
> > Error reporting does not require a synchronous operation. Reporting
> > it in the next read() or write() and making it pollable is perfectly
> > viable. It just must not be silently dropped.
>
> OK, I think I got it. I need to document that. I'll also add
> get_pr/dr/vconn hooks to the API for getting the status.
Would the attached diff do the trick? It also includes the other
suggestions from Guenter.
Thanks,
--
heikki
diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index 53fdd11..9f249b2 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -6,10 +6,12 @@ Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Description:
The current USB data role the port is operating in. This
attribute can be used for requesting data role swapping on the
- port. Swapping is only supported as an asynchronous operation
- and requires polling of the attribute in order to know the
- result, so successful write operation does not mean successful
- swap.
+ port. Swapping is only supported as an asynchronous operation,
+ so successful write operation does not mean successful swap.
+ The attribute can be polled with poll() or select() to get
+ notification on finished operation. In case of failures, the
+ following read/write after finished operation will return error
+ code identifying the cause of the failure.
Valid values:
- host
@@ -22,9 +24,11 @@ Description:
The current power role of the port. This attribute can be used
to request power role swap on the port when the port supports
USB Power Delivery. Swapping is only supported as an
- asynchronous operation and requires polling of the attribute in
- order to know the result, so successful write operation does not
- mean successful swap.
+ asynchronous operation, so successful write operation does not
+ mean successful swap. The attribute can be polled with poll() or
+ select() to get notification on finished operation. In case of
+ failures, the following read/write after finished operation will
+ return error code identifying the cause of the failure.
Valid values:
- source
@@ -37,6 +41,12 @@ Description:
Shows is the port VCONN Source. This attribute can be used to
request VCONN swap to change the VCONN Source during connection
when both the port and the partner support USB Power Delivery.
+ Swapping is only supported as an asynchronous operation, so
+ successful write operation does not mean successful swap. The
+ attribute can be polled with poll() or select() to get
+ notification on finished operation. In case of failures, the
+ following read/write after finished operation will return error
+ code identifying the cause of the failure.
Valid values are:
- 0 when the port is not the VCONN Source
@@ -113,28 +123,21 @@ Description:
USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
-What: /sys/class/typec/<port>-partner/accessory
+What: /sys/class/typec/<port>-partner/accessory_mode
Date: June 2016
Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Description:
- The attribute is visible only when the partner's type is
- "Accessory". The type can be read from its own attribute.
+ Shows the Accessory Mode name or "no" when the partner is not an
+ Accesory Mode. The Accessory Modes are defined in USB Type-C
+ Specification.
- Shows the name of the Accessory Mode. The Accessory Modes are
- defined in USB Type-C Specification.
-
-What: /sys/class/typec/<port>-partner/type
+What: /sys/class/typec/<port>-partner/supports_usb_power_delivery
Date: June 2016
Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
Description:
- Shows the type of the partner. Can be one of the following:
- - USB - When the partner is normal USB host/peripheral.
- - Charger - When the partner has been identified as dedicated
- charger.
- - Alternate Mode - When the partner supports Alternate Modes.
- - Accessory - When the partner is one of the accessories with
- specific Accessory Mode defined in USB Type-C
- specification.
+ Shows if the partner supports USB Power Delivery.
+ - 0 when USB Power Delivery is not supported
+ - 1 when USB Power Delivery is supported
USB Type-C cable devices (eg. /sys/class/typec/usbc0-cable/)
@@ -177,7 +180,13 @@ Description:
Shows if the mode is active or not. The attribute can be used
for entering/exiting the mode with partners and cable plugs, and
with the port alternate modes it can be used for disabling
- support for specific alternate modes.
+ support for specific alternate modes. Entering/exiting modes is
+ only supported as an asynchronous operation, so successful write
+ operation does not mean successful swap. The attribute can be
+ polled with poll() or select() to get notification on finished
+ operation. In case of failures, the following read/write after
+ finished operation will return error code identifying the cause
+ of the failure.
What: /sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/description
Date: June 2016
diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
index 366a04c..6e549f4 100644
--- a/drivers/usb/typec/typec.c
+++ b/drivers/usb/typec/typec.c
@@ -42,10 +42,9 @@ static struct class typec_class = {
};
static const char * const typec_accessory_modes[] = {
- [TYPEC_ACCESSORY_NONE] = "none",
- [TYPEC_ACCESSORY_AUDIO] = "Audio",
- [TYPEC_ACCESSORY_DEBUG] = "Debug",
- [TYPEC_ACCESSORY_DAUDIO] = "Digital Audio",
+ [TYPEC_ACCESSORY_NONE] = "no",
+ [TYPEC_ACCESSORY_AUDIO] = "Audio Adapter Accessory Mode",
+ [TYPEC_ACCESSORY_DEBUG] = "Debug Accessory Mode",
};
/* ------------------------------------------------------------------------- */
@@ -55,43 +54,34 @@ static void typec_dev_release(struct device *dev)
{
}
-static const char * const typec_partner_types[] = {
- [TYPEC_PARTNER_USB] = "USB",
- [TYPEC_PARTNER_CHARGER] = "Charger",
- [TYPEC_PARTNER_ALTMODE] = "Alternate Mode",
- [TYPEC_PARTNER_ACCESSORY] = "Accessory",
-};
-
-static ssize_t
-partner_type_show(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t partner_usb_pd_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
- struct typec_partner *partner = container_of(dev, struct typec_partner,
- dev);
+ struct typec_partner *p = container_of(dev, struct typec_partner, dev);
- return sprintf(buf, "%s\n", typec_partner_types[partner->type]);
+ return sprintf(buf, "%d\n", p->usb_pd);
}
-static struct device_attribute dev_attr_partner_type = {
+static struct device_attribute dev_attr_partner_usb_pd = {
.attr = {
- .name = "type",
+ .name = "supports_usb_power_delivery",
.mode = S_IRUGO,
},
- .show = partner_type_show,
+ .show = partner_usb_pd_show,
};
static ssize_t
partner_accessory_mode_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct typec_partner *partner = container_of(dev, struct typec_partner,
- dev);
+ struct typec_partner *p = container_of(dev, struct typec_partner, dev);
- return sprintf(buf, "%s\n", typec_accessory_modes[partner->accessory]);
+ return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
}
static struct device_attribute dev_attr_partner_accessory = {
.attr = {
- .name = "accessory",
+ .name = "accessory_mode",
.mode = S_IRUGO,
},
.show = partner_accessory_mode_show,
@@ -99,27 +89,12 @@ static struct device_attribute dev_attr_partner_accessory = {
static struct attribute *typec_partner_attrs[] = {
&dev_attr_partner_accessory.attr,
- &dev_attr_partner_type.attr,
+ &dev_attr_partner_usb_pd.attr,
NULL
};
-static umode_t
-partner_is_visible(struct kobject *kobj, struct attribute *attr, int n)
-{
- struct device *dev = container_of(kobj, struct device, kobj);
- struct typec_partner *partner = container_of(dev, struct typec_partner,
- dev);
-
- if (attr == &dev_attr_partner_accessory.attr &&
- partner->type != TYPEC_PARTNER_ACCESSORY)
- return 0;
-
- return attr->mode;
-}
-
static struct attribute_group typec_partner_group = {
.attrs = typec_partner_attrs,
- .is_visible = partner_is_visible,
};
static const struct attribute_group *typec_partner_groups[] = {
@@ -495,6 +470,12 @@ typec_altmode_active_show(struct device *dev, struct device_attribute *attr,
{
struct typec_mode *mode = container_of(attr, struct typec_mode,
active_attr);
+ struct typec_port *port = typec_altmode2port(mode->alt_mode);
+ int ret;
+
+ ret = port->cap->mode_status(mode->alt_mode, mode->index);
+ if (ret)
+ return ret;
return sprintf(buf, "%d\n", mode->active);
}
@@ -509,6 +490,10 @@ typec_altmode_active_store(struct device *dev, struct device_attribute *attr,
bool activate;
int ret;
+ ret = port->cap->mode_status(mode->alt_mode, mode->index);
+ if (ret)
+ return ret;
+
ret = kstrtobool(buf, &activate);
if (ret)
return ret;
@@ -768,6 +753,11 @@ current_data_role_store(struct device *dev, struct device_attribute *attr,
return -EOPNOTSUPP;
}
+ /* Get status from previous operation */
+ ret = port->cap->dr_get(port->cap);
+ if (ret < 0)
+ return ret;
+
ret = sysfs_strmatch(typec_data_roles, buf);
if (ret < 0)
return ret;
@@ -786,6 +776,11 @@ current_data_role_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct typec_port *port = to_typec_port(dev);
+ int ret;
+
+ ret = port->cap->dr_get(port->cap);
+ if (ret < 0)
+ return ret;
return sprintf(buf, "%s\n", typec_data_roles[port->data_role]);
}
@@ -827,6 +822,11 @@ static ssize_t current_power_role_store(struct device *dev,
return -EIO;
}
+ /* Get status from previous operation */
+ ret = port->cap->pr_get(port->cap);
+ if (ret < 0)
+ return ret;
+
ret = sysfs_strmatch(typec_roles, buf);
if (ret < 0)
return ret;
@@ -844,6 +844,11 @@ static ssize_t current_power_role_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct typec_port *port = to_typec_port(dev);
+ int ret;
+
+ ret = port->cap->pr_get(port->cap);
+ if (ret < 0)
+ return ret;
return sprintf(buf, "%s\n", typec_roles[port->pwr_role]);
}
@@ -897,13 +902,17 @@ static ssize_t vconn_source_store(struct device *dev,
return -EOPNOTSUPP;
}
- if (sysfs_streq(buf, "1"))
- role = TYPEC_SOURCE;
- else if (sysfs_streq(buf, "0"))
+ if (sysfs_streq(buf, "0"))
role = TYPEC_SINK;
+ else if (sysfs_streq(buf, "1"))
+ role = TYPEC_SOURCE;
else
return -EINVAL;
+ ret = port->cap->vconn_get(port->cap);
+ if (ret < 0)
+ return ret;
+
ret = port->cap->vconn_set(port->cap, role);
if (ret)
return ret;
@@ -915,6 +924,11 @@ static ssize_t vconn_source_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct typec_port *port = to_typec_port(dev);
+ int ret;
+
+ ret = port->cap->vconn_get(port->cap);
+ if (ret < 0)
+ return ret;
return sprintf(buf, "%d\n", port->vconn_role == TYPEC_SOURCE ? 1 : 0);
}
@@ -925,15 +939,15 @@ static ssize_t supported_accessory_modes_show(struct device *dev,
char *buf)
{
struct typec_port *port = to_typec_port(dev);
- enum typec_accessory *accessory;
ssize_t ret = 0;
int i;
- if (port->cap->accessory)
- for (accessory = port->cap->accessory, i = 0;
- i < port->cap->num_accessory; accessory++, i++)
- ret += sprintf(buf, "%s\n",
- typec_accessory_modes[*accessory]);
+ if (!port->cap->accessory[0])
+ return 0;
+
+ for (i = 0; port->cap->accessory[i]; i++)
+ ret += sprintf(buf + ret, "%s\n",
+ typec_accessory_modes[port->cap->accessory[i]]);
return ret;
}
static DEVICE_ATTR_RO(supported_accessory_modes);
@@ -1003,6 +1017,12 @@ struct typec_port *typec_register_port(struct device *dev,
int ret;
int id;
+ /* Forcing the drivers to report status */
+ BUG_ON(cap->dr_set && !cap->dr_get);
+ BUG_ON(cap->pr_set && !cap->pr_get);
+ BUG_ON(cap->vconn_set && !cap->vconn_get);
+ BUG_ON(cap->activate_mode && !cap->mode_status);
+
port = kzalloc(sizeof(*port), GFP_KERNEL);
if (!port)
return ERR_PTR(-ENOMEM);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index eda6747..60a7376 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -13,13 +13,6 @@ enum typec_port_type {
TYPEC_PORT_DRP,
};
-enum typec_partner_type {
- TYPEC_PARTNER_USB,
- TYPEC_PARTNER_CHARGER,
- TYPEC_PARTNER_ALTMODE,
- TYPEC_PARTNER_ACCESSORY,
-};
-
enum typec_plug_type {
USB_PLUG_NONE,
USB_PLUG_TYPE_A,
@@ -49,7 +42,6 @@ enum typec_accessory {
TYPEC_ACCESSORY_NONE,
TYPEC_ACCESSORY_AUDIO,
TYPEC_ACCESSORY_DEBUG,
- TYPEC_ACCESSORY_DAUDIO,
};
/*
@@ -177,7 +169,6 @@ struct typec_cable {
*/
struct typec_partner {
struct device dev;
- enum typec_partner_type type;
unsigned int usb_pd:1;
u32 vdo;
enum typec_accessory accessory;
@@ -188,14 +179,17 @@ struct typec_partner {
* struct typec_capability - USB Type-C Port Capabilities
* @role: DFP (Host-only), UFP (Device-only) or DRP (Dual Role)
* @usb_pd: USB Power Delivery support
- * @accessory: Supported Accessory Modes
- * @num_accessory: Number of supported Accessory Modes
+ * @accessory: Supported Accessory Modes (NULL terminated array)
* @alt_modes: Alternate Modes the connector supports (null terminated)
* @try_role: Set a fixed data role for DRP port
* @dr_set: Set Data Role
+ * @dr_get: Get status from previous dr_set (mandatory with dr_set)
* @pr_set: Set Power Role
+ * @pr_get: Get status from previous pr_get (mandatory with pr_set)
* @vconn_set: Set VCONN Role
+ * @vconn_get: Get status from previous vconn_set (mandatory with vconn_set)
* @activate_mode: Enter/exit given Alternate Mode
+ * @mode_status: Status from last activate_mode (mandatory with acivate_mode)
*
* Static capabilities of a single USB Type-C port.
*/
@@ -203,7 +197,6 @@ struct typec_capability {
enum typec_port_type type;
unsigned int usb_pd:1;
enum typec_accessory *accessory;
- unsigned int num_accessory;
struct typec_altmode *alt_modes;
int (*try_role)(const struct typec_capability *,
@@ -211,13 +204,18 @@ struct typec_capability {
int (*dr_set)(const struct typec_capability *,
enum typec_data_role);
+ int (*dr_get)(const struct typec_capability *);
int (*pr_set)(const struct typec_capability *,
enum typec_role);
+ int (*pr_get)(const struct typec_capability *);
int (*vconn_set)(const struct typec_capability *,
enum typec_role);
+ int (*vconn_get)(const struct typec_capability *);
int (*activate_mode)(struct typec_altmode *,
int mode, int activate);
+ int (*mode_status)(struct typec_altmode *,
+ int mode);
};
/*