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

Groups > linux.kernel > #1330476 > unrolled thread

[PATCH 1/3] usb: USB Type-C Connector Class

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-09 18:10 +0100
    Re: [PATCH 1/3] usb: USB Type-C Connector Class Greg KH <gregkh@linuxfoundation.org> - 2016-02-09 19:30 +0100
      Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-10 11:40 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Greg KH <gregkh@linuxfoundation.org> - 2016-02-10 18:30 +0100
          Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-11 15:10 +0100
    Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-10 12:00 +0100
      Re: [PATCH 1/3] usb: USB Type-C Connector Class Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-10 12:10 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-02-10 12:20 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-10 12:20 +0100
      Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-10 12:30 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-15 16:20 +0100
      Re: [PATCH 1/3] usb: USB Type-C Connector Class Felipe Balbi <balbi@kernel.org> - 2016-02-11 10:00 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-11 10:20 +0100
          Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-11 16:00 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-11 15:40 +0100
          Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-11 16:00 +0100
    Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-17 15:20 +0100
      Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 09:50 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-18 10:30 +0100
          Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 14:10 +0100
        Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-18 10:40 +0100
          Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 14:30 +0100
            Re: [PATCH 1/3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-02-18 14:50 +0100
              Re: [PATCH 1/3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-02-18 16:20 +0100

Page 1 of 2  [1] 2  Next page →

#1330476 — [PATCH 1/3] usb: USB Type-C Connector Class

The purpose of this class is to provide unified interface
for user space to get the status and basic information about
USB Type-C Connectors in the system, control data role
swapping, and when USB PD is available, also power role
swapping and Altenate Modes.

The class will export the following interfaces for every
USB Type-C Connector in the system to sysfs:

1. connected - Connection status of the connector
2. alternate_mode - The current Alternate Mode
3. alternate_modes - Lists all Alternate Modes the connector supports
4. partner_alt_modes - Lists partner's Alternate Modes when connected
5. partner_type - Can be USB, Charger, Alt Mode or Accessory
6. data_role - The current data role, host or device
7. data_roles - Data roles supported by the connector
8. power_role - Connector's current power role, source or sink
9. power_roles - Power roles supported by the connector
10. power_operation_mode - The current power level in use
11. usb_pd - yes if the connector supports USB PD.
12. audio_accessory - yes if the connector supports Audio Accessory
13. debug_accessory - yes if the connector supports Debug Accessory

The data_role, power_role and alternate_mode are also
writable and can be used for executing role swapping and
entering modes. When USB PD is not supported by the
connector or partner, power_role will reflect the value of
the data_role, and is not swappable independently.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/Kconfig         |   2 +
 drivers/usb/Makefile        |   2 +
 drivers/usb/type-c/Kconfig  |   7 +
 drivers/usb/type-c/Makefile |   1 +
 drivers/usb/type-c/typec.c  | 446 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h   | 114 +++++++++++
 6 files changed, 572 insertions(+)
 create mode 100644 drivers/usb/type-c/Kconfig
 create mode 100644 drivers/usb/type-c/Makefile
 create mode 100644 drivers/usb/type-c/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8ed451d..0c45547 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -151,6 +151,8 @@ source "drivers/usb/phy/Kconfig"
 
 source "drivers/usb/gadget/Kconfig"
 
+source "drivers/usb/type-c/Kconfig"
+
 config USB_LED_TRIG
 	bool "USB LED Triggers"
 	depends on LEDS_CLASS && USB_COMMON && LEDS_TRIGGERS
diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
index d5c57f1..4d712ee 100644
--- a/drivers/usb/Makefile
+++ b/drivers/usb/Makefile
@@ -61,3 +61,5 @@ obj-$(CONFIG_USB_GADGET)	+= gadget/
 obj-$(CONFIG_USB_COMMON)	+= common/
 
 obj-$(CONFIG_USBIP_CORE)	+= usbip/
+
+obj-$(CONFIG_TYPEC)		+= type-c/
diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
new file mode 100644
index 0000000..b229fb9
--- /dev/null
+++ b/drivers/usb/type-c/Kconfig
@@ -0,0 +1,7 @@
+
+menu "USB PD and Type-C drivers"
+
+config TYPEC
+	tristate
+
+endmenu
diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
new file mode 100644
index 0000000..1012a8b
--- /dev/null
+++ b/drivers/usb/type-c/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TYPEC)		+= typec.o
diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
new file mode 100644
index 0000000..e425955
--- /dev/null
+++ b/drivers/usb/type-c/typec.c
@@ -0,0 +1,446 @@
+/*
+ * USB Type-C class
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb/typec.h>
+
+#define to_typec_port(p) container_of(p, struct typec_port, dev)
+
+static DEFINE_IDA(typec_index_ida);
+
+/* -------------------------------- */
+
+int typec_connect(struct typec_port *port)
+{
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_connect);
+
+void typec_disconnect(struct typec_port *port)
+{
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+}
+EXPORT_SYMBOL_GPL(typec_disconnect);
+
+/* -------------------------------- */
+
+static ssize_t alternate_mode_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct typec_alt_mode alt_mode;
+	int ret;
+
+	if (!port->cap->set_alt_mode) {
+		dev_warn(dev, "entering Alternate Modes not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (sscanf(buf, "0x%hx,%u", &alt_mode.svid, &alt_mode.mid) != 2)
+		return -EINVAL;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->set_alt_mode(port, &alt_mode);
+	mutex_unlock(&port->lock);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t alternate_mode_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (!port->cur_alt_mode)
+		return sprintf(buf, "none\n");
+
+	/* REVISIT: SIDs in human readable form? */
+	return sprintf(buf, "0x%hx,%u\n", port->cur_alt_mode->svid,
+		       port->cur_alt_mode->mid);
+}
+static DEVICE_ATTR_RW(alternate_mode);
+
+static ssize_t alternate_modes_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct typec_alt_mode *alt_mode;
+	int len = 0;
+
+	if (!port->cap->alt_modes)
+		return sprintf(buf, "none\n");
+
+	/* REVISIT: SIDs in human readable form? */
+	for (alt_mode = port->cap->alt_modes; alt_mode->svid; alt_mode++)
+		len += sprintf(buf + len, "0x%hx,%u\n", alt_mode->svid,
+			       alt_mode->mid);
+
+	buf[len - 1] = '\0';
+	return len;
+}
+static DEVICE_ATTR_RO(alternate_modes);
+
+static ssize_t partner_alt_modes_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	struct typec_alt_mode *alt_mode;
+	int len = 0;
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (!port->partner_alt_modes)
+		return sprintf(buf, "none\n");
+
+	/* REVISIT: SIDs in human readable form? */
+	for (alt_mode = port->partner_alt_modes; alt_mode->svid; alt_mode++)
+		len += sprintf(buf + len, "0x%hx,%u\n", alt_mode->svid,
+			       alt_mode->mid);
+
+	buf[len - 1] = '\0';
+	return len;
+}
+static DEVICE_ATTR_RO(partner_alt_modes);
+
+static const char * const typec_partner_types[] = {
+	[TYPEC_PARTNER_NONE] = "unknown",
+	[TYPEC_PARTNER_USB] = "USB",
+	[TYPEC_PARTNER_CHARGER] = "Charger",
+	[TYPEC_PARTNER_ALTMODE] = "Alternate Mode",
+	[TYPEC_PARTNER_AUDIO] = "Audio Accessory",
+	[TYPEC_PARTNER_DEBUG] = "Debug Accessroy",
+};
+
+static ssize_t partner_type_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (!port->connected)
+		return -ENXIO;
+
+	return sprintf(buf, "%s\n", typec_partner_types[port->partner_type]);
+}
+static DEVICE_ATTR_RO(partner_type);
+
+static ssize_t data_role_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	enum typec_data_role role;
+	int ret;
+
+	if (port->cap->type != TYPEC_PORT_DRP) {
+		dev_dbg(dev, "data role swap only supported with DRP ports\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->cap->dr_swap) {
+		dev_warn(dev, "data role swapping not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (!strncmp(buf, "host", 4))
+		role = TYPEC_HOST;
+	else if (!strncmp(buf, "device", 6))
+		role = TYPEC_DEVICE;
+	else
+		return -EINVAL;
+
+	if (port->data_role == role)
+		goto out;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->dr_swap(port);
+	mutex_unlock(&port->lock);
+	if (ret)
+		return ret;
+out:
+	return size;
+}
+
+static ssize_t data_role_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	switch (port->cap->type) {
+	case TYPEC_PORT_DFP:
+		return sprintf(buf, "host\n");
+	case TYPEC_PORT_UFP:
+		return sprintf(buf, "device\n");
+	case TYPEC_PORT_DRP:
+		return sprintf(buf, "%s\n", port->data_role == TYPEC_HOST ?
+			       "host" : "device");
+	default:
+		return sprintf(buf, "unknown\n");
+	};
+}
+static DEVICE_ATTR_RW(data_role);
+
+static ssize_t data_roles_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (port->cap->type == TYPEC_PORT_DRP)
+		return sprintf(buf, "host, device\n");
+
+	return data_role_show(dev, attr, buf);
+}
+static DEVICE_ATTR_RO(data_roles);
+
+static ssize_t power_role_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	enum typec_pwr_role role;
+	int ret;
+
+	if (!port->cap->usb_pd) {
+		dev_dbg(dev, "power role swap only supported with USB PD\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->cap->pr_swap) {
+		dev_warn(dev, "power role swapping not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->connected)
+		return -ENXIO;
+
+	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
+		dev_dbg(dev, "partner unable to swap power role\n");
+		return -EIO;
+	}
+
+	if (!strncmp(buf, "source", 6))
+		role = TYPEC_PWR_SOURCE;
+	else if (!strncmp(buf, "sink", 4))
+		role = TYPEC_PWR_SINK;
+	else
+		return -EINVAL;
+
+	if (port->pwr_role == role)
+		return size;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->pr_swap(port);
+	mutex_unlock(&port->lock);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t power_role_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	switch (port->pwr_role) {
+	case TYPEC_PWR_SOURCE:
+		return sprintf(buf, "source\n");
+	case TYPEC_PWR_SINK:
+		return sprintf(buf, "sink\n");
+	default:
+		return sprintf(buf, "unknown\n");
+	};
+}
+static DEVICE_ATTR_RW(power_role);
+
+static ssize_t power_roles_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (port->cap->usb_pd || port->cap->type == TYPEC_PORT_DRP)
+		return sprintf(buf, "source, sink\n");
+
+	return power_role_show(dev, attr, buf);
+}
+static DEVICE_ATTR_RO(power_roles);
+
+static const char * const typec_pwr_opmodes[] = {
+	[TYPEC_PWR_MODE_USB] = "USB",
+	[TYPEC_PWR_MODE_BC1_2] = "BC1.2",
+	[TYPEC_PWR_MODE_1_5A] = "USB Type-C 1.5A",
+	[TYPEC_PWR_MODE_3_0A] = "USB Type-C 3.0A",
+	[TYPEC_PWR_MODE_PD] = "USB PD",
+};
+
+static ssize_t power_operation_mode_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n", typec_pwr_opmodes[port->pwr_opmode]);
+}
+static DEVICE_ATTR_RO(power_operation_mode);
+
+static ssize_t connected_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n", port->connected ? "yes" : "no");
+}
+static DEVICE_ATTR_RO(connected);
+
+static ssize_t usb_pd_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%ssupported\n", port->cap->usb_pd ? "" : "not ");
+}
+static DEVICE_ATTR_RO(usb_pd);
+
+static ssize_t audio_accessory_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%ssupported\n", port->cap->audio_accessory ?
+		       "" : "not ");
+}
+static DEVICE_ATTR_RO(audio_accessory);
+
+static ssize_t debug_accessory_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%ssupported\n", port->cap->debug_accessory ?
+		       "" : "not ");
+}
+static DEVICE_ATTR_RO(debug_accessory);
+
+/* REVISIT: Consider creating the partner dependent sysfs files at runtime. */
+static struct attribute *typec_attrs[] = {
+	&dev_attr_alternate_mode.attr,
+	&dev_attr_alternate_modes.attr,
+	&dev_attr_partner_alt_modes.attr,
+	&dev_attr_partner_type.attr,
+	&dev_attr_data_role.attr,
+	&dev_attr_data_roles.attr,
+	&dev_attr_power_role.attr,
+	&dev_attr_power_roles.attr,
+	&dev_attr_power_operation_mode.attr,
+	&dev_attr_connected.attr,
+	&dev_attr_usb_pd.attr,
+	&dev_attr_audio_accessory.attr,
+	&dev_attr_debug_accessory.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(typec);
+
+static int typec_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	int ret;
+
+	ret = add_uevent_var(env, "TYPEC_PORT=%s", dev_name(dev));
+	if (ret)
+		dev_err(dev, "failed to add uevent TYPEC_PORT\n");
+
+	return ret;
+}
+
+static void typec_release(struct device *dev)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	ida_simple_remove(&typec_index_ida, port->id);
+	kfree(port);
+}
+
+static struct class typec_class = {
+	.name = "type-c",
+	.dev_uevent = typec_uevent,
+	.dev_groups = typec_groups,
+	.dev_release = typec_release,
+};
+
+struct typec_port *typec_register_port(struct device *dev,
+				       struct typec_capability *cap)
+{
+	struct typec_port *port;
+	int ret;
+	int id;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return ERR_PTR(-ENOMEM);
+
+	id = ida_simple_get(&typec_index_ida, 0, 0, GFP_KERNEL);
+	if (id < 0) {
+		kfree(port);
+		return ERR_PTR(id);
+	}
+
+	port->id = id;
+	port->cap = cap;
+	port->dev.class = &typec_class;
+	port->dev.parent = dev;
+	dev_set_name(&port->dev, "usbc%d", id);
+	mutex_init(&port->lock);
+
+	ret = device_register(&port->dev);
+	if (ret) {
+		ida_simple_remove(&typec_index_ida, id);
+		put_device(&port->dev);
+		kfree(port);
+		return ERR_PTR(ret);
+	}
+
+	return port;
+}
+EXPORT_SYMBOL_GPL(typec_register_port);
+
+void typec_unregister_port(struct typec_port *port)
+{
+	device_unregister(&port->dev);
+}
+EXPORT_SYMBOL_GPL(typec_unregister_port);
+
+static int __init typec_init(void)
+{
+	return class_register(&typec_class);
+}
+subsys_initcall(typec_init);
+
+static void __exit typec_exit(void)
+{
+	return class_unregister(&typec_class);
+}
+module_exit(typec_exit);
+
+MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("USB Type-C Connector Class");
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
new file mode 100644
index 0000000..d6e562c
--- /dev/null
+++ b/include/linux/usb/typec.h
@@ -0,0 +1,114 @@
+
+#ifndef __LINUX_USB_TYPEC_H
+#define __LINUX_USB_TYPEC_H
+
+#include <linux/types.h>
+
+enum typec_port_type {
+	TYPEC_PORT_DFP,
+	TYPEC_PORT_UFP,
+	TYPEC_PORT_DRP,
+};
+
+enum typec_data_role {
+	TYPEC_DEVICE,
+	TYPEC_HOST,
+};
+
+enum typec_pwr_role {
+	TYPEC_PWR_SINK,
+	TYPEC_PWR_SOURCE,
+};
+
+enum typec_pwr_opmode {
+	TYPEC_PWR_MODE_USB,
+	TYPEC_PWR_MODE_BC1_2,
+	TYPEC_PWR_MODE_1_5A,
+	TYPEC_PWR_MODE_3_0A,
+	TYPEC_PWR_MODE_PD,
+};
+
+enum typec_partner_type {
+	TYPEC_PARTNER_NONE,
+	TYPEC_PARTNER_USB,
+	TYPEC_PARTNER_CHARGER,
+	TYPEC_PARTNER_ALTMODE,
+	TYPEC_PARTNER_AUDIO,
+	TYPEC_PARTNER_DEBUG,
+};
+
+struct typec_alt_mode {
+	u16 svid;
+	u32 mid;
+};
+
+struct typec_port;
+
+/*
+ * struct typec_capability - USB Type-C Port Capabilities
+ * @type: DFP (Host-only), UFP (Device-only) or DRP (Dual Role)
+ * @usb_pd: USB Power Delivery support
+ * @alt_modes: Alternate Modes the connector supports (null terminated)
+ * @audio_accessory: Audio Accessory Adapter Mode support
+ * @debug_accessory: Debug Accessory Mode support
+ * @dr_swap: Data Role Swap support
+ * @pr_swap: Power Role Swap support
+ * @set_alt_mode: Enter given Alternate Mode
+ *
+ * Static capabilities of a single USB Type-C port.
+ */
+struct typec_capability {
+	enum typec_port_type	type;
+	unsigned int		usb_pd:1;
+	struct typec_alt_mode	*alt_modes;
+	unsigned int		audio_accessory:1;
+	unsigned int		debug_accessory:1;
+
+	int			(*dr_swap)(struct typec_port *);
+	int			(*pr_swap)(struct typec_port *);
+	int			(*set_alt_mode)(struct typec_port *,
+						struct typec_alt_mode *);
+};
+
+/*
+ * struct typec_port - USB Type-C Port
+ * @id: port index
+ * @dev: struct device instance
+ * @lock: Lock to protect concurrent access
+ * @data_role: Current USB role - Host or Device
+ * @pwr_role: Current Power role - Source or Sink
+ * @pwr_opmode: The power level in use at the moment
+ * @cur_alt_mode: The Alternate Mode currently in use
+ * @connected: Connection status
+ * @partner_type: Port type of the partner
+ * @partner_alt_modes: Alternate Modes the partner supports (null terminated)
+ * @cap: Port Capabilities
+ *
+ * Current status of a USB Type-C port and relevant partner details when
+ * connected.
+ */
+struct typec_port {
+	unsigned int		id;
+	struct device		dev;
+	struct mutex		lock;
+
+	enum typec_data_role	data_role;
+	enum typec_pwr_role	pwr_role;
+	enum typec_pwr_opmode	pwr_opmode;
+	struct typec_alt_mode	*cur_alt_mode;
+
+	unsigned char		connected;
+	enum typec_partner_type	partner_type;
+	struct typec_alt_mode	*partner_alt_modes;
+
+	const struct typec_capability *cap;
+};
+
+struct typec_port *typec_register_port(struct device *dev,
+				       struct typec_capability *cap);
+void typec_unregister_port(struct typec_port *port);
+
+int typec_connect(struct typec_port *port);
+void typec_disconnect(struct typec_port *port);
+
+#endif /* __LINUX_USB_TYPEC_H */
-- 
2.7.0

[toc] | [next] | [standalone]

#1330565

On Tue, Feb 09, 2016 at 07:01:21PM +0200, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface
> for user space to get the status and basic information about
> USB Type-C Connectors in the system, control data role
> swapping, and when USB PD is available, also power role
> swapping and Altenate Modes.
> 
> The class will export the following interfaces for every
> USB Type-C Connector in the system to sysfs:
> 
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports
> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> 6. data_role - The current data role, host or device
> 7. data_roles - Data roles supported by the connector
> 8. power_role - Connector's current power role, source or sink
> 9. power_roles - Power roles supported by the connector
> 10. power_operation_mode - The current power level in use
> 11. usb_pd - yes if the connector supports USB PD.
> 12. audio_accessory - yes if the connector supports Audio Accessory
> 13. debug_accessory - yes if the connector supports Debug Accessory

You forgot to document these sysfs files in Documenataion/ABI :(

And what is userspace going to do with these files?  Why does it care?

> The data_role, power_role and alternate_mode are also
> writable and can be used for executing role swapping and
> entering modes. When USB PD is not supported by the
> connector or partner, power_role will reflect the value of
> the data_role, and is not swappable independently.

How does this implementation differ from those in other drivers that we
have seen, but not submitted for merging?  I'm referring to the code
from Fairchild for their USB Type C driver:
	https://github.com/gregkh/fusb30x
and the driver that is in the latest Nexus 6 Android release (don't have
the link to the android kernel tree at the moment sorry, but it's public
and I think Linaro is working on cleaning it up...)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1331050

On Tue, Feb 09, 2016 at 10:20:45AM -0800, Greg KH wrote:
> On Tue, Feb 09, 2016 at 07:01:21PM +0200, Heikki Krogerus wrote:
> > The purpose of this class is to provide unified interface
> > for user space to get the status and basic information about
> > USB Type-C Connectors in the system, control data role
> > swapping, and when USB PD is available, also power role
> > swapping and Altenate Modes.
> > 
> > The class will export the following interfaces for every
> > USB Type-C Connector in the system to sysfs:
> > 
> > 1. connected - Connection status of the connector
> > 2. alternate_mode - The current Alternate Mode
> > 3. alternate_modes - Lists all Alternate Modes the connector supports
> > 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> > 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> > 6. data_role - The current data role, host or device
> > 7. data_roles - Data roles supported by the connector
> > 8. power_role - Connector's current power role, source or sink
> > 9. power_roles - Power roles supported by the connector
> > 10. power_operation_mode - The current power level in use
> > 11. usb_pd - yes if the connector supports USB PD.
> > 12. audio_accessory - yes if the connector supports Audio Accessory
> > 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> You forgot to document these sysfs files in Documenataion/ABI :(

Man, it's almost like I enjoy making these stupid mistakes :(

> And what is userspace going to do with these files?  Why does it care?

The OS policy regarding USB Type-C ports needs to come from user
space. The user must be allowed to select the USB data role, and when
USB PD is supported, also the power role, and at the same time we need
to export all the relevant information about the USB Type-C ports to
the user space, like connection status, the type of partner etc. And
all that from a single interface.

I'm pretty sure that this is exactly what distributions like Ubuntu,
RedHat etc. want, an I know for fact that Chrome OS and Android will
expect the user to be in control over the roles and get that
information about the ports one way or the other.

> > The data_role, power_role and alternate_mode are also
> > writable and can be used for executing role swapping and
> > entering modes. When USB PD is not supported by the
> > connector or partner, power_role will reflect the value of
> > the data_role, and is not swappable independently.
> 
> How does this implementation differ from those in other drivers that we
> have seen, but not submitted for merging?  I'm referring to the code
> from Fairchild for their USB Type C driver:
> 	https://github.com/gregkh/fusb30x
> and the driver that is in the latest Nexus 6 Android release (don't have
> the link to the android kernel tree at the moment sorry, but it's public
> and I think Linaro is working on cleaning it up...)

That would be USB PD stack and driver for fusb30x USB Type-C/PD PHYs.
It's the second USB PD stack I've seen (and sadly also second driver
for fusb30x), but I just know there are more.

My class is just about exporting control of USB Type-C ports to the
user space, and note, USB Type-C *not* USB PD. I don't thing that my
little class and the USB PD stack, where ever it ends up coming from,
conflict with each other.

The only difference is that I'm clearly separating USB Type-C from USB
PD (and actually everything else), which is the correct thing to do.
USB Type-C is not the same thing as USB PD. USB Type-C does not always
come with USB PD.

I did not go through that code, but I'm guessing the guys have for
example exported similar role swapping controls to user space from
some part of their stack. So those guys would just need to register
their fusb30x with this class, let the class take care of exporting
those controls and probable continue using their USB PD stack exactly
like they have done before.

I hope I was able to explain myself.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1331351

On Wed, Feb 10, 2016 at 12:38:40PM +0200, Heikki Krogerus wrote:
> > And what is userspace going to do with these files?  Why does it care?
> 
> The OS policy regarding USB Type-C ports needs to come from user
> space.

What drives this "need"?

> The user must be allowed to select the USB data role, and when
> USB PD is supported, also the power role, and at the same time we need
> to export all the relevant information about the USB Type-C ports to
> the user space, like connection status, the type of partner etc. And
> all that from a single interface.

Again, what drives this "need" to be in a "single interface"?

> I'm pretty sure that this is exactly what distributions like Ubuntu,
> RedHat etc. want, an I know for fact that Chrome OS and Android will
> expect the user to be in control over the roles and get that
> information about the ports one way or the other.

Given that ChromeOS and Android already do this type of thing today, why
not work with those developers to ensure that this really is the
interface they want / expect?

> > > The data_role, power_role and alternate_mode are also
> > > writable and can be used for executing role swapping and
> > > entering modes. When USB PD is not supported by the
> > > connector or partner, power_role will reflect the value of
> > > the data_role, and is not swappable independently.
> > 
> > How does this implementation differ from those in other drivers that we
> > have seen, but not submitted for merging?  I'm referring to the code
> > from Fairchild for their USB Type C driver:
> > 	https://github.com/gregkh/fusb30x
> > and the driver that is in the latest Nexus 6 Android release (don't have
> > the link to the android kernel tree at the moment sorry, but it's public
> > and I think Linaro is working on cleaning it up...)
> 
> That would be USB PD stack and driver for fusb30x USB Type-C/PD PHYs.
> It's the second USB PD stack I've seen (and sadly also second driver
> for fusb30x), but I just know there are more.

Oh, there's more than just 2 drivers for that fusb30x chip floating
around.  My repo is not the latest version and it's a truly horrid piece
of code, never to be run on any hardware you actually care about power
as it doesn't care.

> My class is just about exporting control of USB Type-C ports to the
> user space, and note, USB Type-C *not* USB PD. I don't thing that my
> little class and the USB PD stack, where ever it ends up coming from,
> conflict with each other.

But we need to ensure that it doesn't conflict, and given that you are
already using the same directory those stacks are using, perhaps you can
look at them to see that you aren't duplicating any work?

> The only difference is that I'm clearly separating USB Type-C from USB
> PD (and actually everything else), which is the correct thing to do.
> USB Type-C is not the same thing as USB PD. USB Type-C does not always
> come with USB PD.

I agree, keeping them separate seems good, but I worry when you have to
do both how that is going to look.

> I did not go through that code, but I'm guessing the guys have for
> example exported similar role swapping controls to user space from
> some part of their stack. So those guys would just need to register
> their fusb30x with this class, let the class take care of exporting
> those controls and probable continue using their USB PD stack exactly
> like they have done before.

the fusb30x code does it all within kernel space, no userspace
interactions needed due to timing requirements (so they say).  I'm not
saying that this is a good idea / design, just that I'm getting
conflicting requirements from different camps at the moment and it's
really hard to sort it all out :(

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1331974

On Wed, Feb 10, 2016 at 09:26:19AM -0800, Greg KH wrote:
> On Wed, Feb 10, 2016 at 12:38:40PM +0200, Heikki Krogerus wrote:
> > > And what is userspace going to do with these files?  Why does it care?
> > 
> > The OS policy regarding USB Type-C ports needs to come from user
> > space.
> 
> What drives this "need"?
> 
> > The user must be allowed to select the USB data role, and when
> > USB PD is supported, also the power role, and at the same time we need
> > to export all the relevant information about the USB Type-C ports to
> > the user space, like connection status, the type of partner etc. And
> > all that from a single interface.
> 
> Again, what drives this "need" to be in a "single interface"?

Because the alternative is that every platform will have a custom way
of doing these things.. With the single interface I mean the class.
If we don't present USB Type-C ports to the userspace from a unified
interface, how can anybody create userspace software to support
control of USB Type-C ports? Or can we accept that we will always need
platform specific user space software if we want to control the USB
Type-C ports?

In case you are still skeptical about why userspace would need any
interaction with the USB Type-C ports, think about the following use
case: You plug what you think is something that will charge your
phone, nothing is printed on the screen - we don't now get any details
about the parter - but we must be charging, right? After a while you
realize that you are not charging, instead the thing you plugged in is
actually draining you phone of power? This will happen if the partner
happens to be for example..
1) Accessory
2) Alternate Mode that we enter successfully, so no Billboard Device.
3) Both our device and the partner are DRP and we end up being the
   host initially.

Nothing bad has happened in this example, but it's horrible user
experience.

> > I'm pretty sure that this is exactly what distributions like Ubuntu,
> > RedHat etc. want, an I know for fact that Chrome OS and Android will
> > expect the user to be in control over the roles and get that
> > information about the ports one way or the other.
> 
> Given that ChromeOS and Android already do this type of thing today, why
> not work with those developers to ensure that this really is the
> interface they want / expect?

I'm in contact with some Android developers, but Android is a bit
frustrating. I know the absolute minimum about the world of Android,
but the way it looks to me is that everybody is doing their own thing.
The expectations about things also vary from company to company.
Maybe there is a forum somewhere that I'm not aware of?

I'm pretty sure ChromeOS is going to be different. I'll try to make
some contacts there.

> > > > The data_role, power_role and alternate_mode are also
> > > > writable and can be used for executing role swapping and
> > > > entering modes. When USB PD is not supported by the
> > > > connector or partner, power_role will reflect the value of
> > > > the data_role, and is not swappable independently.
> > > 
> > > How does this implementation differ from those in other drivers that we
> > > have seen, but not submitted for merging?  I'm referring to the code
> > > from Fairchild for their USB Type C driver:
> > > 	https://github.com/gregkh/fusb30x
> > > and the driver that is in the latest Nexus 6 Android release (don't have
> > > the link to the android kernel tree at the moment sorry, but it's public
> > > and I think Linaro is working on cleaning it up...)
> > 
> > That would be USB PD stack and driver for fusb30x USB Type-C/PD PHYs.
> > It's the second USB PD stack I've seen (and sadly also second driver
> > for fusb30x), but I just know there are more.
> 
> Oh, there's more than just 2 drivers for that fusb30x chip floating
> around.  My repo is not the latest version and it's a truly horrid piece
> of code, never to be run on any hardware you actually care about power
> as it doesn't care.
> 
> > My class is just about exporting control of USB Type-C ports to the
> > user space, and note, USB Type-C *not* USB PD. I don't thing that my
> > little class and the USB PD stack, where ever it ends up coming from,
> > conflict with each other.
> 
> But we need to ensure that it doesn't conflict, and given that you are
> already using the same directory those stacks are using, perhaps you can
> look at them to see that you aren't duplicating any work?

Well, in case of that fusb30x driver, there definitely is no conflict
if there really is no interaction with the userspace. They would just
never register with this class.

But there really should never be any conflict between USB Type-C and
USB PD. For USB Type-C connectors USB PD is just an optional "library"
that can have the following functions and nothing else:

1) DR_Swap - Data Role swap
2) PR_Swap - Power Role swap
2) VCONN_Swap - VCONN Source swap
3) Discover SVIDs - Alternate Modes
3) Discover Modes - The actual modes under SVID
4) Enter Mode
4) Exit Mode

So those functions will always be the same. From Type-C point of view
it really makes no difference how they have actually been implemented
(in software, PD controller, firmware, etc).

> > The only difference is that I'm clearly separating USB Type-C from USB
> > PD (and actually everything else), which is the correct thing to do.
> > USB Type-C is not the same thing as USB PD. USB Type-C does not always
> > come with USB PD.
> 
> I agree, keeping them separate seems good, but I worry when you have to
> do both how that is going to look.

I'm fairly certain that we can make the Type-C part good. It's so
straight forward. I'm more worried about USB PD.

I'm a bit skeptical about whether it will even be possible to create a
stack that works for every type of USB PD thingy out there. For cases
where there is just the PHY like fusb30x, the PD stack will be made
completely in software. That we can (maybe) handle. Also cases where
we have a complex USB PD controller that handles all layers starting
from protocol, we will be able to handle, as with them the stack is not
needed at all. Even the simple USB PD controllers that take care of
protocol layer (completely) but leave the policy layers to the
software, we will be able to handle. The problem comes from the USB PD
controller and PHYs that fall somewhere in-between. The ones that
implement "partial" protocol layer and "partial" policy engine, or
consist of several components, or..

..I have to stop there. I just vomited in my mouth a little. I fear
USB PD can end up being a little bit like USB Charging (Felipe can
tell more about that) except many times worse! Maybe we need to start
thinking about the ground rules when somebody adds the PD stack, like
you can use only complete parts of stack and never try to mix your
oddities into it, otherwise you have to implement everything from
scratch for your particular platform, etc.

Perhaps things are not as bad as I fear, but let's just make sure
Type-C is always separate from USB PD!

> > I did not go through that code, but I'm guessing the guys have for
> > example exported similar role swapping controls to user space from
> > some part of their stack. So those guys would just need to register
> > their fusb30x with this class, let the class take care of exporting
> > those controls and probable continue using their USB PD stack exactly
> > like they have done before.
> 
> the fusb30x code does it all within kernel space, no userspace
> interactions needed due to timing requirements (so they say).  I'm not
> saying that this is a good idea / design, just that I'm getting
> conflicting requirements from different camps at the moment and it's
> really hard to sort it all out :(

I think there must have been some misunderstanding. We really have to
interact with the userspace with USB Type-C ports.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1331063

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface
> for user space to get the status and basic information about
> USB Type-C Connectors in the system, control data role
> swapping, and when USB PD is available, also power role
> swapping and Altenate Modes.
> 
> The class will export the following interfaces for every
> USB Type-C Connector in the system to sysfs:
> 
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports

These names are a bit problematic, as they are too similar.
How about

current_alternate_mode
potential_alternate_modes

> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> 6. data_role - The current data role, host or device
> 7. data_roles - Data roles supported by the connector
> 8. power_role - Connector's current power role, source or sink
> 9. power_roles - Power roles supported by the connector
> 10. power_operation_mode - The current power level in use
> 11. usb_pd - yes if the connector supports USB PD.
> 12. audio_accessory - yes if the connector supports Audio Accessory
> 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> The data_role, power_role and alternate_mode are also
> writable and can be used for executing role swapping and
> entering modes. When USB PD is not supported by the
> connector or partner, power_role will reflect the value of
> the data_role, and is not swappable independently.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/Kconfig         |   2 +
>  drivers/usb/Makefile        |   2 +
>  drivers/usb/type-c/Kconfig  |   7 +
>  drivers/usb/type-c/Makefile |   1 +
>  drivers/usb/type-c/typec.c  | 446 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h   | 114 +++++++++++
>  6 files changed, 572 insertions(+)
>  create mode 100644 drivers/usb/type-c/Kconfig
>  create mode 100644 drivers/usb/type-c/Makefile
>  create mode 100644 drivers/usb/type-c/typec.c
>  create mode 100644 include/linux/usb/typec.h
> 
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8ed451d..0c45547 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -151,6 +151,8 @@ source "drivers/usb/phy/Kconfig"
>  
>  source "drivers/usb/gadget/Kconfig"
>  
> +source "drivers/usb/type-c/Kconfig"
> +
>  config USB_LED_TRIG
>  	bool "USB LED Triggers"
>  	depends on LEDS_CLASS && USB_COMMON && LEDS_TRIGGERS
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index d5c57f1..4d712ee 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -61,3 +61,5 @@ obj-$(CONFIG_USB_GADGET)	+= gadget/
>  obj-$(CONFIG_USB_COMMON)	+= common/
>  
>  obj-$(CONFIG_USBIP_CORE)	+= usbip/
> +
> +obj-$(CONFIG_TYPEC)		+= type-c/
> diff --git a/drivers/usb/type-c/Kconfig b/drivers/usb/type-c/Kconfig
> new file mode 100644
> index 0000000..b229fb9
> --- /dev/null
> +++ b/drivers/usb/type-c/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB PD and Type-C drivers"
> +
> +config TYPEC
> +	tristate
> +
> +endmenu
> diff --git a/drivers/usb/type-c/Makefile b/drivers/usb/type-c/Makefile
> new file mode 100644
> index 0000000..1012a8b
> --- /dev/null
> +++ b/drivers/usb/type-c/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TYPEC)		+= typec.o
> diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
> new file mode 100644
> index 0000000..e425955
> --- /dev/null
> +++ b/drivers/usb/type-c/typec.c
> @@ -0,0 +1,446 @@
> +/*
> + * USB Type-C class
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb/typec.h>
> +
> +#define to_typec_port(p) container_of(p, struct typec_port, dev)
> +
> +static DEFINE_IDA(typec_index_ida);
> +
> +/* -------------------------------- */
> +
> +int typec_connect(struct typec_port *port)
> +{
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(typec_connect);
> +
> +void typec_disconnect(struct typec_port *port)
> +{
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +}
> +EXPORT_SYMBOL_GPL(typec_disconnect);
> +
> +/* -------------------------------- */
> +
> +static ssize_t alternate_mode_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	struct typec_alt_mode alt_mode;
> +	int ret;
> +
> +	if (!port->cap->set_alt_mode) {
> +		dev_warn(dev, "entering Alternate Modes not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->connected)
> +		return -ENXIO;

Doesn't this need locking?
And why wouldn't user space want to preselect a mode?

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1331069

On Wed, Feb 10, 2016 at 12:49 PM, Oliver Neukum <oneukum@suse.com> wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
>> The purpose of this class is to provide unified interface
>> for user space to get the status and basic information about
>> USB Type-C Connectors in the system, control data role
>> swapping, and when USB PD is available, also power role
>> swapping and Altenate Modes.
>>
>> The class will export the following interfaces for every
>> USB Type-C Connector in the system to sysfs:
>>
>> 1. connected - Connection status of the connector
>> 2. alternate_mode - The current Alternate Mode
>> 3. alternate_modes - Lists all Alternate Modes the connector supports
>
> These names are a bit problematic, as they are too similar.
> How about
>
> current_alternate_mode
> potential_alternate_modes

I would vote for supported_*

>> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
>> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
>> 6. data_role - The current data role, host or device
>> 7. data_roles - Data roles supported by the connector
>> 8. power_role - Connector's current power role, source or sink
>> 9. power_roles - Power roles supported by the connector
>> 10. power_operation_mode - The current power level in use
>> 11. usb_pd - yes if the connector supports USB PD.
>> 12. audio_accessory - yes if the connector supports Audio Accessory
>> 13. debug_accessory - yes if the connector supports Debug Accessory

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1331072

On Wed, Feb 10, 2016 at 1:11 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Wed, Feb 10, 2016 at 01:05:27PM +0200, Andy Shevchenko wrote:
>> On Wed, Feb 10, 2016 at 12:49 PM, Oliver Neukum <oneukum@suse.com> wrote:
>> > On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
>> >> The purpose of this class is to provide unified interface
>> >> for user space to get the status and basic information about
>> >> USB Type-C Connectors in the system, control data role
>> >> swapping, and when USB PD is available, also power role
>> >> swapping and Altenate Modes.
>> >>
>> >> The class will export the following interfaces for every
>> >> USB Type-C Connector in the system to sysfs:
>> >>
>> >> 1. connected - Connection status of the connector
>> >> 2. alternate_mode - The current Alternate Mode
>> >> 3. alternate_modes - Lists all Alternate Modes the connector supports
>> >
>> > These names are a bit problematic, as they are too similar.
>> > How about
>> >
>> > current_alternate_mode
>
> That works for me.
>
>> > potential_alternate_modes
>>
>> I would vote for supported_*
>
> How about connector_alternate_modes?

Would be fine as well.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1331075

On Wed, Feb 10, 2016 at 01:05:27PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 10, 2016 at 12:49 PM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> >> The purpose of this class is to provide unified interface
> >> for user space to get the status and basic information about
> >> USB Type-C Connectors in the system, control data role
> >> swapping, and when USB PD is available, also power role
> >> swapping and Altenate Modes.
> >>
> >> The class will export the following interfaces for every
> >> USB Type-C Connector in the system to sysfs:
> >>
> >> 1. connected - Connection status of the connector
> >> 2. alternate_mode - The current Alternate Mode
> >> 3. alternate_modes - Lists all Alternate Modes the connector supports
> >
> > These names are a bit problematic, as they are too similar.
> > How about
> >
> > current_alternate_mode

That works for me.

> > potential_alternate_modes
> 
> I would vote for supported_*

How about connector_alternate_modes?


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1331078

Hi Oliver,

> > +static ssize_t alternate_mode_store(struct device *dev,
> > +				    struct device_attribute *attr,
> > +				    const char *buf, size_t size)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	struct typec_alt_mode alt_mode;
> > +	int ret;
> > +
> > +	if (!port->cap->set_alt_mode) {
> > +		dev_warn(dev, "entering Alternate Modes not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (!port->connected)
> > +		return -ENXIO;
> 
> Doesn't this need locking?

Yes, I need to fix the locking.

> And why wouldn't user space want to preselect a mode?

That is tricky, as we would need to keep a list of the preselected
modes and for all SVIDs the connector supports. I don't think it would
be practical to do from this file as we would then use it differently
when connected and not connected, so the preselected modes would
probable be better to give from a separate file.

That is certainly doable, but is it really useful? I not really
against adding that support, but I would like to keep this interface
as simple as possible.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1334508

On Wed, 2016-02-10 at 13:23 +0200, Heikki Krogerus wrote:

> That is tricky, as we would need to keep a list of the preselected
> modes and for all SVIDs the connector supports. I don't think it would
> be practical to do from this file as we would then use it differently
> when connected and not connected, so the preselected modes would
> probable be better to give from a separate file.
> 
> That is certainly doable, but is it really useful? I not really
> against adding that support, but I would like to keep this interface
> as simple as possible.

Just for the record. Heikki is right. Preselecting an alternate mode
is stupid.

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1331740

[Multipart message — attachments visible in raw view] — view raw

Oliver Neukum <oneukum@suse.com> writes:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
>> The purpose of this class is to provide unified interface
>> for user space to get the status and basic information about
>> USB Type-C Connectors in the system, control data role
>> swapping, and when USB PD is available, also power role
>> swapping and Altenate Modes.
>> 
>> The class will export the following interfaces for every
>> USB Type-C Connector in the system to sysfs:
>> 
>> 1. connected - Connection status of the connector
>> 2. alternate_mode - The current Alternate Mode
>> 3. alternate_modes - Lists all Alternate Modes the connector supports
>
> These names are a bit problematic, as they are too similar.
> How about
>
> current_alternate_mode
> potential_alternate_modes

available_ ?

>> 4. partner_alt_modes - Lists partner's Alternate Modes when connected

partner_alternate_modes ? (it's a file name, we can spell it out)

>> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
>> 6. data_role - The current data role, host or device
>> 7. data_roles - Data roles supported by the connector

current_data_role
available_data_roles

>> 8. power_role - Connector's current power role, source or sink
>> 9. power_roles - Power roles supported by the connector

ditto...

>> 10. power_operation_mode - The current power level in use
>> 11. usb_pd - yes if the connector supports USB PD.

supports_usb_power_delivery ?

>> 12. audio_accessory - yes if the connector supports Audio Accessory

supports_audio_accessory

>> 13. debug_accessory - yes if the connector supports Debug Accessory

supports_debug_accessory

> Doesn't this need locking?
> And why wouldn't user space want to preselect a mode?

isn't USB always the default mode ?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1331765

On Thu, 2016-02-11 at 10:55 +0200, Felipe Balbi wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> > On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> >> The purpose of this class is to provide unified interface
> >> for user space to get the status and basic information about
> >> USB Type-C Connectors in the system, control data role
> >> swapping, and when USB PD is available, also power role
> >> swapping and Altenate Modes.
> >> 
> >> The class will export the following interfaces for every
> >> USB Type-C Connector in the system to sysfs:
> >> 
> >> 1. connected - Connection status of the connector
> >> 2. alternate_mode - The current Alternate Mode
> >> 3. alternate_modes - Lists all Alternate Modes the connector supports
> >
> > These names are a bit problematic, as they are too similar.
> > How about
> >
> > current_alternate_mode
> > potential_alternate_modes
> 
> available_ ?

Also good. Frankly I think it boils down to taste.
I just want to make sure that they not be mistaken for each other.

> >> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 
> partner_alternate_modes ? (it's a file name, we can spell it out)

True

[..]

> > Doesn't this need locking?
> > And why wouldn't user space want to preselect a mode?
> 
> isn't USB always the default mode ?

True for mode. But the question also applies to roles.
I would think that user space should be able to preselect
that we always want to be upstream or downstream or flexible.
And it should be able to preemptively disallow power delivery
even if no cable is present.

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1332128

On Thu, Feb 11, 2016 at 10:08:14AM +0100, Oliver Neukum wrote:
> I would think that user space should be able to preselect
> that we always want to be upstream or downstream or flexible.

Agreed.

> And it should be able to preemptively disallow power delivery
> even if no cable is present.

By power delivery you mean power sourcing, not USB PD, right? So
basically being able to preselect also power_role?

That is OK by me, but we need to remember that it's something that is
only possible when both our connector and also the partner support USB
PD. Otherwise the power role will be based on the data role. So when
USB PD is not supported, host is always the source and the device
always the sink.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1332088

Hi Felipe,

> >> 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 
> partner_alternate_modes ? (it's a file name, we can spell it out)
> 
> >> 5. partner_type - Can be USB, Charger, Alt Mode or Accessory
> >> 6. data_role - The current data role, host or device
> >> 7. data_roles - Data roles supported by the connector
> 
> current_data_role
> available_data_roles
> 
> >> 8. power_role - Connector's current power role, source or sink
> >> 9. power_roles - Power roles supported by the connector
> 
> ditto...
> 
> >> 10. power_operation_mode - The current power level in use
> >> 11. usb_pd - yes if the connector supports USB PD.
> 
> supports_usb_power_delivery ?
> 
> >> 12. audio_accessory - yes if the connector supports Audio Accessory
> 
> supports_audio_accessory
> 
> >> 13. debug_accessory - yes if the connector supports Debug Accessory
> 
> supports_debug_accessory

Those all make sense.

> > Doesn't this need locking?
> > And why wouldn't user space want to preselect a mode?
> 
> isn't USB always the default mode ?

Just in case there are no misunderstandings here, Oliver was
commenting on alternate_mode_store function, and USB of course is not
Alternate Mode.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1332118

On Thu, 2016-02-11 at 16:36 +0200, Heikki Krogerus wrote:
> Just in case there are no misunderstandings here, Oliver was
> commenting on alternate_mode_store function, and USB of course is not
> Alternate Mode.

Yes I was because that is the hardest case. In hindsight it is also the
most useless. If you see a billboard device you can still react.
But the point also applies to all "switchable" features,
most importantly PD.
I usually want to charge my phone if it is connected, but not
e.g. if my battery is at less than 60%. Such things belong
to user space and should ideally be setable before anything is
plugged in.

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1336469

On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> 1. connected - Connection status of the connector
> 2. alternate_mode - The current Alternate Mode
> 3. alternate_modes - Lists all Alternate Modes the connector supports
> 4. partner_alt_modes - Lists partner's Alternate Modes when connected

Now that I think about it, there's a gap.
Which SVIDs do we expose if we are UFP (slave)?

	Regards
		Oliver

[toc] | [prev] | [next] | [standalone]

#1337154

[Multipart message — attachments visible in raw view] — view raw

Hi Oliver,

On Wed, Feb 17, 2016 at 03:07:27PM +0100, Oliver Neukum wrote:
> On Tue, 2016-02-09 at 19:01 +0200, Heikki Krogerus wrote:
> > 1. connected - Connection status of the connector
> > 2. alternate_mode - The current Alternate Mode
> > 3. alternate_modes - Lists all Alternate Modes the connector supports
> > 4. partner_alt_modes - Lists partner's Alternate Modes when connected
> 
> Now that I think about it, there's a gap.
> Which SVIDs do we expose if we are UFP (slave)?

In the alternate_modes listing the connectors alt modes, we can not
have modes that the hardware can not support of course, and it is the
responsibility of the drivers registering the type-c ports with this
clss to make sure they are not part of the list.

In partner alternate modes, we will list all alternate modes the
partner supports, even the ones our connector doesn't.

The modes that can actually be selected have to be supported by both
the connector and the partner, and this is where I'm putting the ball
on the userspace at the moment. I'm not offering a list of
"possible_alternate_modes" where I list the combination, but instead
expect the userspace to be figure out that on it's own.

Do you think we should add "possible_alternate_modes" file?


P.S. That reminds me, here's my current draft for the
Documentation/ABI/. Could you take a look?


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1337185

On Thu, 2016-02-18 at 10:47 +0200, Heikki Krogerus wrote:

Hi,

> The modes that can actually be selected have to be supported by both
> the connector and the partner, and this is where I'm putting the ball
> on the userspace at the moment. I'm not offering a list of
> "possible_alternate_modes" where I list the combination, but instead
> expect the userspace to be figure out that on it's own.
> 
> Do you think we should add "possible_alternate_modes" file?

No, what do we answer to the DFP if we recieve "Discover SVIDs"?
I don't think that we always should answer with all we physically
can. If, for example, the hardware could do Thunderbolt, but the OS
is not prepared to handle it, we shouldn't offer it. So this is
a policy decision to be made in user space. Hence we need
an API to tell it to the kernel.

> P.S. That reminds me, here's my current draft for the
> Documentation/ABI/. Could you take a look?

OK

Here are my comments:

What:		/sys/class/type-c/usbcN/connected

		Connection status of the USB Type-C connector usbcN. "yes" when
		connected, otherwise "no".

Unnecessarily wordy. 0 and 1 would do

What:		/sys/class/type-c/usbcN/current_data_role

Again, 0 and 1 would do

What:		/sys/class/type-c/usbcN/partner_alternate_modes

You should say in which number base the values are given.

What:		/sys/class/type-c/usbcN/partner_type

That could be combined with "connected"

What:		/sys/class/type-c/usbcN/supported_data_roles

A connector can be both. How is that expressed?

What:		/sys/class/type-c/usbcN/supported_power_roles

Again, what if it can do both?

[toc] | [prev] | [next] | [standalone]

#1337339

On Thu, Feb 18, 2016 at 10:21:48AM +0100, Oliver Neukum wrote:
> On Thu, 2016-02-18 at 10:47 +0200, Heikki Krogerus wrote:
> 
> Hi,
> 
> > The modes that can actually be selected have to be supported by both
> > the connector and the partner, and this is where I'm putting the ball
> > on the userspace at the moment. I'm not offering a list of
> > "possible_alternate_modes" where I list the combination, but instead
> > expect the userspace to be figure out that on it's own.
> > 
> > Do you think we should add "possible_alternate_modes" file?
> 
> No, what do we answer to the DFP if we recieve "Discover SVIDs"?
> I don't think that we always should answer with all we physically
> can. If, for example, the hardware could do Thunderbolt, but the OS
> is not prepared to handle it, we shouldn't offer it. So this is
> a policy decision to be made in user space. Hence we need
> an API to tell it to the kernel.

OK. Makes sense.

> > P.S. That reminds me, here's my current draft for the
> > Documentation/ABI/. Could you take a look?
> 
> OK
> 
> Here are my comments:
> 
> What:		/sys/class/type-c/usbcN/connected
> 
> 		Connection status of the USB Type-C connector usbcN. "yes" when
> 		connected, otherwise "no".
> 
> Unnecessarily wordy. 0 and 1 would do

That works for me.

> What:		/sys/class/type-c/usbcN/current_data_role
> 
> Again, 0 and 1 would do

I disagree with this one. What would 0 mean and what would 1? It would
require us to make an agreement about the "index" of the role, which
creates a small risk of somebody getting it wrong, but for what
purpose?

Why couldn't it be human readable "host" or "device" so there is never
no confusion about it.

> What:		/sys/class/type-c/usbcN/partner_alternate_modes
> 
> You should say in which number base the values are given.
> 
> What:		/sys/class/type-c/usbcN/partner_type
> 
> That could be combined with "connected"

Hmm, so in practice getting rid of "connected" completely.. I guess
it's OK.

> What:		/sys/class/type-c/usbcN/supported_data_roles
> 
> A connector can be both. How is that expressed?

"host, device".

> What:		/sys/class/type-c/usbcN/supported_power_roles
> 
> Again, what if it can do both?

"source, sink".

So these last two are now listing the values that can be entered to
the current_data_role and current_power_role.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web