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

Groups > linux.kernel > #1403714 > unrolled thread

[RFC PATCHv2] usb: USB Type-C Connector Class

Back to article view | Back to linux.kernel

Contents

  [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-19 14:50 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-19 17:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Greg KH <gregkh@linuxfoundation.org> - 2016-05-19 17:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 13:00 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-19 17:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 13:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-20 15:50 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-21 08:00 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-21 08:50 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-22 18:00 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 07:40 +0200
                  Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 15:30 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 16:10 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 16:50 +0200
                        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 18:00 +0200
                          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 19:00 +0200
                            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 12:10 +0200
                              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 12:30 +0200
                                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 13:10 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-19 20:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 12:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-20 19:10 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-23 11:30 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-20 16:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-23 12:00 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 13:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 19:10 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 11:10 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 11:40 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 15:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 17:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 09:40 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-24 15:50 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:40 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 15:20 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-24 21:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 14:00 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 15:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 16:10 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-25 16:30 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 17:10 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 09:30 +0200
    [RFC PATCH] usb: typec: Various API updates and fixes Guenter Roeck <linux@roeck-us.net> - 2016-05-25 20:40 +0200
      Re: [RFC PATCH] usb: typec: Various API updates and fixes Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 10:00 +0200
        Re: [RFC PATCH] usb: typec: Various API updates and fixes Guenter Roeck <linux@roeck-us.net> - 2016-05-27 16:10 +0200
          Re: [RFC PATCH] usb: typec: Various API updates and fixes Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-30 14:50 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-30 15:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-30 16:10 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-31 10:40 +0200

Page 1 of 3  [1] 2 3  Next page →

#1403714 — [RFC PATCHv2] 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 Alternate Modes.

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  | 957 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/typec.h   | 230 +++++++++++
 6 files changed, 1199 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

Hi,

Like I've told some of you guys, I'm trying to implement a bus for
the Alternate Modes, but I'm still nowhere near finished with that
one, so let's just get the class ready now. The altmode bus should in
any case not affect the userspace interface proposed in this patch.

As you can see, the Alternate Modes are handled completely differently
compared to the original proposal. Every Alternate Mode will have
their own device instance (which will be then later bound to an
Alternate Mode specific driver once we have the bus), but also every
partner, cable and cable plug will have their own device instances
representing them.

An other change is that the data role is now handled in two ways.
The current_data_role file will represent static mode of the port, and
it will use the names for the roles as they are defined in the spec:
DFP, UFP and DRP. This file should be used if the port needs to be
fixed to one specific role with DRP ports. So this approach will
replace the suggestions for "preferred" data role we had. The
current_usb_data_role will use values "host" and "device" and it will
be used for data role swapping when already connected.

The tree of devices that will be populated when the cable is active
and when the cable has controller on both plug, will look as
following:

usbc0
|- usbc0-cable
|   |- usbc0-plug0
|   |   |- usbc0-plug.svid:xxx
|   |   |   |-mode0
|   |   |   |   |- vdo
|   |   |   |   |- desc
|   |   |   |   |- active
...
|   |- usbc0-plug1
|   |   |-usbc0-partner
|   |   |   |- usbc0-partner.svid:xxxx
|   |   |   |	|-mode0
|   |   |   |	|   |- vdo
|   |   |   |	|   |- desc
|   |   |   |	|   |- active
|   |   |   |	|-mode1
...
|   |   |- usbc0-plug1.svid:xxx
|   |   |   |-mode0
|   |   |   |   |- vdo
...

If there is no active cable, the partner will be directly attached to
the port, but symlink to the partner is now always added to the port
folder in any case. I'm not sure about this approach. There is a
question about it in the code. Please check it.

Just noticed that the "active" file is for now read only, but it needs
to be changed to writable. That file will of course provide means for
the userspace to Exit and Enter modes. But please note that the
responsibility of the dependencies between the modes, say, if a plug
needs to be in one mode or the other in order for the partner to enter
some specific mode, will fall on the Alternate Mode specific drivers
once we have the altmode bus. I remember there were concerns about
this in the original thread.

There were quite a few suggestions for the file names. I'm using the
most popular ones from the first thread now.

In case somebody is wondering, I'm not adding ABI documentation until
we know how the final version will look like.

I Hope I remembered to CC everybody interested.


Thanks,


diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8689dcb..4ef9b03 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -150,6 +150,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 dca7856..02d26bf 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..8028b7d
--- /dev/null
+++ b/drivers/usb/type-c/typec.c
@@ -0,0 +1,957 @@
+/*
+ * USB Type-C Connector 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>
+
+struct typec_port {
+	unsigned int		id;
+	struct device		dev;
+	struct mutex		lock; /* FIXME: Not in use yet. */
+
+	enum typec_usb_role	usb_role;
+	enum typec_pwr_role	pwr_role;
+	enum typec_pwr_role	vconn_role;
+	enum typec_pwr_opmode	pwr_opmode;
+
+	struct typec_partner	*partner;
+	struct typec_cable	*cable;
+
+	unsigned int		connected:1;
+
+	int			n_altmode;
+
+	enum typec_data_role	fixed_role;
+	const struct typec_capability *cap;
+};
+
+#define to_typec_port(p) container_of(p, struct typec_port, dev)
+
+static DEFINE_IDA(typec_index_ida);
+
+static struct class typec_class = {
+	.name = "type-c",
+};
+
+/* -------------------------------- */
+/* Type-C Partners */
+
+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_AUDIO] = "Audio Accessory",
+	[TYPEC_PARTNER_DEBUG] = "Debug Accessory",
+};
+
+static ssize_t partner_type_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct typec_partner *partner = container_of(dev, struct typec_partner,
+						     dev);
+
+	return sprintf(buf, "%s\n", typec_partner_types[partner->type]);
+}
+
+static struct device_attribute dev_attr_partner_type = {
+	.attr = {
+		.name = "type",
+		.mode = S_IRUGO,
+	},
+	.show = partner_type_show,
+};
+
+static struct attribute *typec_partner_attrs[] = {
+	&dev_attr_partner_type.attr,
+	NULL
+};
+
+static struct attribute_group typec_partner_group = {
+	.attrs = typec_partner_attrs,
+};
+
+static const struct attribute_group *typec_partner_groups[] = {
+	&typec_partner_group,
+	NULL
+};
+
+static struct device_type typec_partner_dev_type = {
+	.name = "typec_partner_device",
+	.groups = typec_partner_groups,
+	.release = typec_dev_release,
+};
+
+static int
+typec_add_partner(struct typec_port *port, struct typec_partner *partner)
+{
+	struct device *dev = &partner->dev;
+	struct device *parent;
+	int ret;
+
+	/*
+	 * REVISIT: Maybe it would be better to make the port always as the
+	 * parent of the partner? Or not even that. Would it be enough to just
+	 * create the symlink to the partner like we do below in any case?
+	 */
+	if (port->cable) {
+		if (port->cable->active) {
+			if (port->cable->sop_pp_controller)
+				parent = &port->cable->plug[1].dev;
+			else
+				parent = &port->cable->plug[0].dev;
+		} else {
+			parent = &port->cable->dev;
+		}
+	} else {
+		parent = &port->dev;
+	}
+
+	dev->class = &typec_class;
+	dev->parent = parent;
+	dev->type = &typec_partner_dev_type;
+	dev_set_name(dev, "%s-partner", dev_name(&port->dev));
+
+	ret = device_register(dev);
+	if (ret) {
+		put_device(dev);
+		return ret;
+	}
+
+	ret = typec_register_altmodes(dev, partner->alt_modes);
+	if (ret) {
+		device_unregister(dev);
+		return ret;
+	}
+
+	/* REVISIT: Creating symlink for the port device for now. */
+	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, "partner");
+	if (ret)
+		dev_WARN(&port->dev, "failed to create link to %s (%d)\n",
+			 dev_name(dev), ret);
+
+	port->partner = partner;
+	return 0;
+}
+
+static void typec_remove_partner(struct typec_port *port)
+{
+	sysfs_remove_link(&port->dev.kobj, "partner");
+	typec_unregister_altmodes(port->partner->alt_modes);
+	device_unregister(&port->partner->dev);
+}
+
+/* -------------------------------- */
+/* Type-C Cable Plugs */
+
+static struct device_type typec_plug_dev_type = {
+	.name = "type_plug_device",
+	.release = typec_dev_release,
+};
+
+static int
+typec_add_plug(struct typec_port *port, struct typec_plug *plug)
+{
+	struct device *dev = &plug->dev;
+	char name[6];
+	int ret;
+
+	sprintf(name, "plug%d", plug->index);
+
+	dev->class = &typec_class;
+	dev->parent = &port->cable->dev;
+	dev->type = &typec_plug_dev_type;
+	dev_set_name(dev, "%s-%s", dev_name(&port->dev), name);
+
+	ret = device_register(dev);
+	if (ret) {
+		put_device(dev);
+		return ret;
+	}
+
+	ret = typec_register_altmodes(dev, plug->alt_modes);
+	if (ret) {
+		device_unregister(dev);
+		return ret;
+	}
+
+	/* REVISIT: Is this useful? */
+	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, name);
+	if (ret)
+		dev_WARN(&port->dev, "failed to create link to %s (%d)\n",
+			 dev_name(dev), ret);
+
+	return 0;
+}
+
+static void typec_remove_plug(struct typec_plug *plug)
+{
+	struct typec_port *port = to_typec_port(plug->dev.parent->parent);
+	char name[6];
+
+	sprintf(name, "plug%d", plug->index);
+	sysfs_remove_link(&port->dev.kobj, name);
+	typec_unregister_altmodes(plug->alt_modes);
+	device_unregister(&plug->dev);
+}
+
+static ssize_t
+active_cable_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
+
+	return sprintf(buf, "%d\n", cable->active);
+}
+
+static struct device_attribute dev_attr_active_cable = {
+	.attr = {
+		.name = "active",
+		.mode = S_IRUGO,
+	},
+	.show = active_cable_show,
+};
+
+static const char * const typec_plug_types[] = {
+	[USB_PLUG_NONE] = "unknown",
+	[USB_PLUG_TYPE_A] = "Type-A",
+	[USB_PLUG_TYPE_B] = "Type-B",
+	[USB_PLUG_TYPE_C] = "Type-C",
+	[USB_PLUG_CAPTIVE] = "Captive",
+};
+
+static ssize_t
+cable_plug_type_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
+
+	return sprintf(buf, "%s\n", typec_plug_types[cable->type]);
+}
+
+static struct device_attribute dev_attr_plug_type = {
+	.attr = {
+		.name = "plug_type",
+		.mode = S_IRUGO,
+	},
+	.show = cable_plug_type_show,
+};
+
+static struct attribute *typec_cable_attrs[] = {
+	&dev_attr_active_cable.attr,
+	&dev_attr_plug_type.attr,
+	NULL
+};
+
+static struct attribute_group typec_cable_group = {
+	.attrs = typec_cable_attrs,
+};
+
+static const struct attribute_group *typec_cable_groups[] = {
+	&typec_cable_group,
+	NULL
+};
+
+static struct device_type typec_cable_dev_type = {
+	.name = "typec_cable_device",
+	.groups = typec_cable_groups,
+	.release = typec_dev_release,
+};
+
+static int typec_add_cable(struct typec_port *port, struct typec_cable *cable)
+{
+	struct device *dev = &cable->dev;
+	int ret;
+
+	dev->class = &typec_class;
+	/* REVISIT: We could have just the symlink also for the cable. */
+	dev->parent = &port->dev;
+	dev->type = &typec_cable_dev_type;
+	dev_set_name(dev, "%s-cable", dev_name(&port->dev));
+
+	ret = device_register(dev);
+	if (ret) {
+		put_device(dev);
+		return ret;
+	}
+
+	/* Plug1 */
+	if (!cable->active)
+		return 0;
+
+	cable->plug[0].index = 1;
+	ret = typec_add_plug(port, &cable->plug[0]);
+	if (ret) {
+		device_unregister(dev);
+		return ret;
+	}
+
+	/* Plug2 */
+	if (!cable->sop_pp_controller)
+		return 0;
+
+	cable->plug[1].index = 2;
+	ret = typec_add_plug(port, &cable->plug[1]);
+	if (ret) {
+		typec_remove_plug(&cable->plug[0]);
+		device_unregister(dev);
+		return ret;
+	}
+
+	port->cable = cable;
+	return 0;
+}
+
+static void typec_remove_cable(struct typec_port *port)
+{
+	if (port->cable->active) {
+		typec_remove_plug(&port->cable->plug[0]);
+		if (port->cable->sop_pp_controller)
+			typec_remove_plug(&port->cable->plug[1]);
+	}
+	device_unregister(&port->cable->dev);
+}
+
+/* -------------------------------- */
+
+int typec_connect(struct typec_port *port, struct typec_connection *con)
+{
+	int ret;
+
+	/* FIXME: bus_type for typec? Note that we will in any case have bus for
+	 * the alternate modes. typec bus would be only dealing with the cable
+	 * and partner. */
+
+	if (!con->partner && !con->cable)
+		return -EINVAL;
+
+	port->connected = 1;
+	port->usb_role = con->usb_role;
+	port->pwr_role = con->pwr_role;
+	port->vconn_role = con->vconn_role;
+	port->pwr_opmode = con->pwr_opmode;
+
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+
+	if (con->cable) {
+		ret = typec_add_cable(port, con->cable);
+		if (ret)
+			return ret;
+	}
+
+	if (con->partner) {
+		ret = typec_add_partner(port, con->partner);
+		if (ret) {
+			if (con->cable)
+				typec_remove_cable(port);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(typec_connect);
+
+void typec_disconnect(struct typec_port *port)
+{
+	if (port->partner)
+		typec_remove_partner(port);
+
+	if (port->cable)
+		typec_remove_cable(port);
+
+	port->connected = 0;
+	port->partner = NULL;
+	port->cable = NULL;
+
+	port->pwr_opmode = TYPEC_PWR_MODE_USB;
+
+	if (port->fixed_role == TYPEC_PORT_DFP) {
+		port->usb_role = TYPEC_HOST;
+		port->pwr_role = TYPEC_PWR_SOURCE;
+		port->vconn_role = TYPEC_PWR_SOURCE;
+	} else {
+		/* Device mode as default also with DRP ports */
+		port->usb_role = TYPEC_DEVICE;
+		port->pwr_role = TYPEC_PWR_SINK;
+		port->vconn_role = TYPEC_PWR_SINK;
+	}
+
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+}
+EXPORT_SYMBOL_GPL(typec_disconnect);
+
+struct device *typec_port2dev(struct typec_port *port)
+{
+	return &port->dev;
+}
+EXPORT_SYMBOL_GPL(typec_port2dev);
+
+struct typec_port *typec_dev2port(struct device *dev)
+{
+	return to_typec_port(dev);
+}
+EXPORT_SYMBOL_GPL(typec_dev2port);
+
+/* -------------------------------- */
+/* Alternate Modes */
+
+/*
+ * typec_altmode2port - Alternate Mode to USB Type-C port
+ * @alt: The Alternate Mode
+ *
+ * Returns the port that the cable plug or partner with @alt is connected to.
+ * The function is helper only for cable plug and partner Alternate Modes. With
+ * Type-C port Alternate Modes the function returns NULL.
+ */
+struct typec_port *typec_altmode2port(struct typec_altmode *alt)
+{
+	if (alt->dev.parent->type == &typec_plug_dev_type)
+		return to_typec_port(alt->dev.parent->parent->parent);
+	if (alt->dev.parent->type == &typec_partner_dev_type)
+		return to_typec_port(alt->dev.parent->parent);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(typec_altmode2port);
+
+static void typec_altmode_release(struct device *dev)
+{
+	struct typec_altmode *alt = to_altmode(dev);
+
+	kfree(alt->mode_groups);
+}
+
+static ssize_t
+typec_altmode_vdo_show(struct device *dev, struct device_attribute *attr,
+		       char *buf)
+{
+	struct typec_mode *mode = container_of(attr, struct typec_mode,
+					       vdo_attr);
+
+	return sprintf(buf, "0x%08x\n", mode->vdo);
+}
+
+static ssize_t
+typec_altmode_desc_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct typec_mode *mode = container_of(attr, struct typec_mode,
+					       desc_attr);
+
+	return sprintf(buf, "%s\n", mode->desc);
+}
+
+static ssize_t
+typec_altmode_active_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct typec_mode *mode = container_of(attr, struct typec_mode,
+					       active_attr);
+
+	return sprintf(buf, "%d\n", mode->active);
+}
+
+static void typec_init_modes(struct typec_altmode *alt)
+{
+	struct typec_mode *mode = alt->modes;
+	int i;
+
+	for (i = 0; i < alt->n_modes; i++, mode++) {
+		mode->index = i;
+		sprintf(mode->group_name, "mode%d", i);
+
+		sysfs_attr_init(&mode->vdo_attr.attr);
+		mode->vdo_attr.attr.name = "vdo";
+		mode->vdo_attr.attr.mode = S_IRUGO;
+		mode->vdo_attr.show = typec_altmode_vdo_show;
+
+		sysfs_attr_init(&mode->desc_attr.attr);
+		mode->desc_attr.attr.name = "description";
+		mode->desc_attr.attr.mode = S_IRUGO;
+		mode->desc_attr.show = typec_altmode_desc_show;
+
+		sysfs_attr_init(&mode->active_attr.attr);
+		mode->active_attr.attr.name = "active";
+		mode->active_attr.attr.mode = S_IRUGO;
+		mode->active_attr.show = typec_altmode_active_show;
+
+		mode->attrs[0] = &mode->vdo_attr.attr;
+		mode->attrs[1] = &mode->desc_attr.attr;
+		mode->attrs[2] = &mode->active_attr.attr;
+
+		mode->group.attrs = mode->attrs;
+		mode->group.name = mode->group_name;
+
+		alt->mode_groups[i] = &mode->group;
+	}
+}
+
+static int typec_add_altmode(struct device *parent, struct typec_altmode *alt)
+{
+	struct device *dev = &alt->dev;
+	int ret;
+
+	alt->mode_groups = kcalloc(alt->n_modes + 1,
+				   sizeof(struct attibute_group *), GFP_KERNEL);
+	if (!alt->mode_groups)
+		return -ENOMEM;
+
+	typec_init_modes(alt);
+
+	dev->groups = alt->mode_groups;
+	dev->release = typec_altmode_release;
+
+	dev->parent = parent;
+	/* TODO: dev->bus = &typec_altmode_bus; */
+
+	if (alt->name)
+		dev_set_name(dev, "%s.%s", dev_name(parent), alt->name);
+	else
+		dev_set_name(dev, "%s.svid:%04x", dev_name(parent), alt->svid);
+
+	ret = device_register(dev);
+	if (ret) {
+		put_device(dev);
+		kfree(alt->mode_groups);
+		return ret;
+	}
+
+	return 0;
+}
+
+int typec_register_altmodes(struct device *parent,
+			    struct typec_altmode *alt_modes)
+{
+	struct typec_altmode *alt;
+	int index;
+	int ret;
+
+	if (!alt_modes)
+		return 0;
+
+	for (alt = alt_modes, index = 0; alt->svid; alt++, index++) {
+		ret = typec_add_altmode(parent, alt);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	for (alt = alt_modes + index; index; alt--, index--)
+		device_unregister(&alt->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(typec_register_altmodes);
+
+void typec_unregister_altmodes(struct typec_altmode *alt_modes)
+{
+	struct typec_altmode *alt;
+
+	for (alt = alt_modes; alt->svid; alt++)
+		device_unregister(&alt->dev);
+}
+EXPORT_SYMBOL_GPL(typec_unregister_altmodes);
+
+/* -------------------------------- */
+
+static ssize_t
+current_usb_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_usb_role role;
+	int ret;
+
+	if (port->cap->role != 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 (!strncmp(buf, "host", 4))
+		role = TYPEC_HOST;
+	else if (!strncmp(buf, "device", 6))
+		role = TYPEC_DEVICE;
+	else
+		return -EINVAL;
+
+	if (port->usb_role == role || !port->partner)
+		return size;
+
+	ret = port->cap->dr_swap(port);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t
+current_usb_data_role_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	if (port->usb_role == TYPEC_DEVICE)
+		return sprintf(buf, "device\n");
+
+	return sprintf(buf, "host\n");
+}
+static DEVICE_ATTR_RW(current_usb_data_role);
+
+static const char * const typec_data_roles[] = {
+	[TYPEC_PORT_DFP] = "DFP",
+	[TYPEC_PORT_UFP] = "UFP",
+	[TYPEC_PORT_DRP] = "DRP",
+};
+
+static ssize_t supported_data_roles_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n", typec_data_roles[port->cap->role]);
+}
+static DEVICE_ATTR_RO(supported_data_roles);
+
+static ssize_t
+current_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->role != TYPEC_PORT_DRP)
+		return -EOPNOTSUPP;
+
+	if (!port->cap->fix_role)
+		return -EOPNOTSUPP;
+
+	if (!strcmp(buf, "DFP"))
+		role = TYPEC_PORT_DFP;
+	else if (!strcmp(buf, "UFP"))
+		role = TYPEC_PORT_UFP;
+	else if (!strcmp(buf, "DRP"))
+		role = TYPEC_PORT_DRP;
+	else
+		return -EINVAL;
+
+	if (port->fixed_role == role)
+		return size;
+
+	ret = port->cap->fix_role(port, role);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t
+current_data_role_show(struct device *dev, struct device_attribute *attr,
+		       char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n", typec_data_roles[port->fixed_role]);
+}
+static DEVICE_ATTR_RW(current_data_role);
+
+static ssize_t current_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->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 || !port->partner)
+		return size;
+
+	ret = port->cap->pr_swap(port);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t current_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(current_power_role);
+
+static ssize_t supported_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->role == TYPEC_PORT_DRP)
+		return sprintf(buf, "source, sink\n");
+
+	return current_power_role_show(dev, attr, buf);
+}
+static DEVICE_ATTR_RO(supported_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 Power Delivery",
+};
+
+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 supports_audio_accessory_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%d\n", port->cap->audio_accessory);
+}
+static DEVICE_ATTR_RO(supports_audio_accessory);
+
+static ssize_t supports_debug_accessory_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%d\n", port->cap->debug_accessory);
+}
+static DEVICE_ATTR_RO(supports_debug_accessory);
+
+static ssize_t supports_usb_power_delivery_show(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%d\n", port->cap->usb_pd);
+}
+static DEVICE_ATTR_RO(supports_usb_power_delivery);
+
+static struct attribute *typec_attrs[] = {
+	&dev_attr_current_data_role.attr,
+	&dev_attr_current_power_role.attr,
+	&dev_attr_current_usb_data_role.attr,
+	&dev_attr_power_operation_mode.attr,
+	&dev_attr_supported_data_roles.attr,
+	&dev_attr_supported_power_roles.attr,
+	&dev_attr_supports_audio_accessory.attr,
+	&dev_attr_supports_debug_accessory.attr,
+	&dev_attr_supports_usb_power_delivery.attr,
+	NULL,
+};
+
+static const struct attribute_group typec_group = {
+	.attrs = typec_attrs,
+};
+
+static ssize_t number_of_alternate_modes_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	return sprintf(buf, "%d\n", port->n_altmode);
+}
+static DEVICE_ATTR_RO(number_of_alternate_modes);
+
+static struct attribute *altmode_attrs[] = {
+	&dev_attr_number_of_alternate_modes.attr,
+	NULL,
+};
+
+static const struct attribute_group altmode_group = {
+	.name = "supported_alternate_modes",
+	.attrs = altmode_attrs,
+};
+
+static const struct attribute_group *typec_groups[] = {
+	&typec_group,
+	&altmode_group,
+	NULL,
+};
+
+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 device_type typec_port_dev_type = {
+	.name = "typec_port",
+	.groups = typec_groups,
+	.uevent = typec_uevent,
+	.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.type = &typec_port_dev_type;
+	port->dev.class = &typec_class;
+	port->dev.parent = dev;
+	dev_set_name(&port->dev, "usbc%d", id);
+	mutex_init(&port->lock);
+
+	port->fixed_role = port->cap->role;
+
+	ret = device_register(&port->dev);
+	if (ret) {
+		ida_simple_remove(&typec_index_ida, id);
+		put_device(&port->dev);
+		kfree(port);
+		return ERR_PTR(ret);
+	}
+
+	/*
+	 * The alternate modes that the port supports must be created before
+	 * registering the port. They are just linked to the port here.
+	 */
+	if (cap->alt_modes) {
+		struct typec_altmode *alt;
+
+		for (alt = cap->alt_modes; alt->svid; alt++) {
+			ret = sysfs_add_link_to_group(&port->dev.kobj,
+						"supported_alternate_modes",
+						&alt->dev.kobj,
+						alt->name ? alt->name :
+						dev_name(&alt->dev));
+			if (ret) {
+				dev_WARN(&port->dev,
+					 "failed to create sysfs symlink\n");
+			} else {
+				port->n_altmode++;
+			}
+		}
+	}
+
+	return port;
+}
+EXPORT_SYMBOL_GPL(typec_register_port);
+
+void typec_unregister_port(struct typec_port *port)
+{
+	if (port->connected)
+		typec_disconnect(port);
+
+	if (port->cap->alt_modes) {
+		struct typec_altmode *alt;
+
+		for (alt = port->cap->alt_modes; alt->svid; alt++)
+			sysfs_remove_link_from_group(&port->dev.kobj,
+						     "alternate_modes",
+						     alt->name ? alt->name :
+						     dev_name(&alt->dev));
+	}
+	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)
+{
+	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..86e5c86
--- /dev/null
+++ b/include/linux/usb/typec.h
@@ -0,0 +1,230 @@
+
+#ifndef __LINUX_USB_TYPEC_H
+#define __LINUX_USB_TYPEC_H
+
+#include <linux/types.h>
+
+struct typec_port;
+
+enum typec_data_role {
+	TYPEC_PORT_DFP,
+	TYPEC_PORT_UFP,
+	TYPEC_PORT_DRP,
+};
+
+enum typec_partner_type {
+	TYPEC_PARTNER_USB,
+	TYPEC_PARTNER_CHARGER,
+	TYPEC_PARTNER_ALTMODE,
+	TYPEC_PARTNER_AUDIO,
+	TYPEC_PARTNER_DEBUG,
+};
+
+enum typec_plug_type {
+	USB_PLUG_NONE,
+	USB_PLUG_TYPE_A,
+	USB_PLUG_TYPE_B,
+	USB_PLUG_TYPE_C,
+	USB_PLUG_CAPTIVE,
+};
+
+enum typec_usb_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,
+};
+
+/*
+ * struct typec_mode - Individual Mode of an Alternate Mode
+ * @vdo: VDO returned by Discover Modes USB PD command
+ * @desc: Mode description
+ * @active: Tells if the mode is currently entered or not
+ * @index: Index of the mode
+ * @group_name: Name for the sysfs folder in form "mode<index>"
+ * @group: The sysfs group (folder) for the mode
+ * @attrs: The attributes for the sysfs group
+ * @vdo_attr: Device attribute to expose the VDO of the mode
+ * @desc_attr: Device attribute to expose the description of the mode
+ * @acctive_attr: Device attribute to expose active of the mode
+ *
+ * Details about a mode of an Alternate Mode which a connector, cable plug or
+ * partner supports. Every mode will have it's own sysfs group. The details are
+ * the VDO returned by discover modes command, description for the mode and
+ * active flag telling is the mode currently active or not.
+ */
+struct typec_mode {
+	u32			vdo;
+	char			*desc;
+	unsigned int		active:1;
+
+	int			index;
+	char			group_name[8];
+	struct attribute_group	group;
+	struct attribute	*attrs[4];
+	struct device_attribute vdo_attr;
+	struct device_attribute desc_attr;
+	struct device_attribute active_attr;
+};
+
+/*
+ * struct typec_altmode - USB Type-C Alternate Mode
+ * @dev: struct device instance
+ * @name: Name for the Alternate Mode (optional)
+ * @svid: Standard or Vendor ID
+ * @n_modes: Number of modes
+ * @modes: Array of modes supported by the Alternat Mode
+ * @mode_groups: The modes as attribute groups to be exposed in sysfs
+ *
+ * Representation of an Alternate Mode that has SVID assigned by USB-IF. The
+ * array of modes will list the modes of a particular SVID that are supported by
+ * a connector, partner of a cable plug.
+ */
+struct typec_altmode {
+	struct device		dev;
+	char			*name;
+
+	u16			svid;
+	int			n_modes;
+	struct typec_mode	*modes;
+
+	const struct attribute_group **mode_groups;
+};
+
+#define to_altmode(d) container_of(d, struct typec_altmode, dev)
+
+struct typec_port *typec_altmode2port(struct typec_altmode *);
+
+int typec_register_altmodes(struct device *, struct typec_altmode *);
+void typec_unregister_altmodes(struct typec_altmode *);
+
+/*
+ * struct typec_plug - USB Type-C Cable Plug
+ * @dev: struct device instance
+ * @index: 1 for the plug connected to DFP and 2 for the plug connected to UFP
+ * @alt_modes: Alternate Modes the cable plug supports (null terminated)
+ *
+ * Represents USB Type-C Cable Plug.
+ */
+struct typec_plug {
+	struct device		dev;
+	int			index;
+	struct typec_altmode	*alt_modes;
+};
+
+/*
+ * struct typec_cable - USB Type-C Cable
+ * @dev: struct device instance
+ * @type: The plug type from USB PD Cable VDO
+ * @active: Is the cable active or passive
+ * @sop_pp_controller: Tells whether both cable plugs are configurable or not
+ * @plug: The two plugs in the cable.
+ *
+ * Represents USB Type-C Cable attached to USB Type-C port. Two plugs are
+ * created if the cable has SOP Double Prime controller as defined in USB PD
+ * specification. Otherwise only one will be created if the cable is active. For
+ * passive cables no plugs are created.
+ */
+struct typec_cable {
+	struct device		dev;
+	enum typec_plug_type	type;
+	unsigned int		active:1;
+	unsigned int		sop_pp_controller:1;
+	/* REVISIT: What else needed? */
+
+	struct typec_plug	plug[2];
+};
+
+/*
+ * struct typec_partner - USB Type-C Partner
+ * @dev: struct device instance
+ * @type: Normal USB device, charger, Alternate Mode or Accessory
+ * @alt_modes: Alternate Modes the partner supports (null terminated)
+ *
+ * Details about a partner that is attached to USB Type-C port.
+ */
+struct typec_partner {
+	struct device		dev;
+	enum typec_partner_type	type;
+	struct typec_altmode	*alt_modes;
+};
+
+/*
+ * 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
+ * @alt_modes: Alternate Modes the connector supports (null terminated)
+ * @audio_accessory: Audio Accessory Adapter Mode support
+ * @debug_accessory: Debug Accessory Mode support
+ * @fix_role: Set a fixed data role for DRP port
+ * @dr_swap: Data Role Swap support
+ * @pr_swap: Power Role Swap support
+ * @vconn_swap: VCONN Swap support
+ * @activate_mode: Enter/exit given Alternate Mode
+ *
+ * Static capabilities of a single USB Type-C port.
+ */
+struct typec_capability {
+	enum typec_data_role	role;
+	unsigned int		usb_pd:1;
+	struct typec_altmode	*alt_modes;
+	unsigned int		audio_accessory:1;
+	unsigned int		debug_accessory:1;
+
+	int			(*fix_role)(struct typec_port *,
+					    enum typec_data_role);
+
+	int			(*dr_swap)(struct typec_port *);
+	int			(*pr_swap)(struct typec_port *);
+	int			(*vconn_swap)(struct typec_port *);
+
+	int			(*activate_mode)(struct typec_altmode *,
+						 int mode, int activate);
+};
+
+/*
+ * struct typec_connection - Details about USB Type-C port connection event
+ * @partner: The attached partner
+ * @cable: The attached cable
+ * @usb_role: Initial USB data role (host or device)
+ * @pwr_role: Initial Power role (source or sink)
+ * @vconn_role: Initial VCONN role (source or sink)
+ * @pwr_opmode: The power mode of the connection
+ *
+ * All the relevant details about a connection event. Wrapper that is passed to
+ * typec_connect(). The context is copied when typec_connect() is called and the
+ * structure is not used for anything else.
+ */
+struct typec_connection {
+	struct typec_partner	*partner;
+	struct typec_cable	*cable;
+
+	enum typec_usb_role	usb_role;
+	enum typec_pwr_role	pwr_role;
+	enum typec_pwr_role	vconn_role;
+	enum typec_pwr_opmode	pwr_opmode;
+};
+
+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, struct typec_connection *con);
+void typec_disconnect(struct typec_port *port);
+
+/* REVISIT: are these needed? */
+struct device *typec_port2dev(struct typec_port *port);
+struct typec_port *typec_dev2port(struct device *dev);
+
+#endif /* __LINUX_USB_TYPEC_H */
-- 
2.8.1

[toc] | [next] | [standalone]

#1403798

On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:

> +	dev->class = &typec_class;
> +	dev->parent = parent;
> +	dev->type = &typec_partner_dev_type;
> +	dev_set_name(dev, "%s-partner", dev_name(&port->dev));
> +
> +	ret = device_register(dev);
> +	if (ret) {
> +		put_device(dev);
> +		return ret;
> +	}

This looks like a race condition.

> +	ret = typec_register_altmodes(dev, partner->alt_modes);
> +	if (ret) {
> +		device_unregister(dev);
> +		return ret;
> +	}
> +
> +	/* REVISIT: Creating symlink for the port device for now. */
> +	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, "partner");
> +	if (ret)
> +		dev_WARN(&port->dev, "failed to create link to %s (%d)\n",
> +			 dev_name(dev), ret);

The attributes should be present as soon as the device is announced.

	Regards
		Oliver

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

#1403818

On Thu, May 19, 2016 at 04:48:46PM +0200, Oliver Neukum wrote:
> On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> 
> > +	dev->class = &typec_class;
> > +	dev->parent = parent;
> > +	dev->type = &typec_partner_dev_type;
> > +	dev_set_name(dev, "%s-partner", dev_name(&port->dev));
> > +
> > +	ret = device_register(dev);
> > +	if (ret) {
> > +		put_device(dev);
> > +		return ret;
> > +	}
> 
> This looks like a race condition.

Really?  How?

> > +	ret = typec_register_altmodes(dev, partner->alt_modes);
> > +	if (ret) {
> > +		device_unregister(dev);
> > +		return ret;
> > +	}
> > +
> > +	/* REVISIT: Creating symlink for the port device for now. */
> > +	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, "partner");
> > +	if (ret)
> > +		dev_WARN(&port->dev, "failed to create link to %s (%d)\n",
> > +			 dev_name(dev), ret);
> 
> The attributes should be present as soon as the device is announced.

Yes, this is wrong and racy (hint, if you have to drop down to a sysfs
call within a driver, almost always something is wrong...)

thanks,

greg k-h

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

#1404338

On Thu, May 19, 2016 at 08:43:27AM -0700, Greg KH wrote:
> On Thu, May 19, 2016 at 04:48:46PM +0200, Oliver Neukum wrote:
> > On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:
> > > +	ret = typec_register_altmodes(dev, partner->alt_modes);
> > > +	if (ret) {
> > > +		device_unregister(dev);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* REVISIT: Creating symlink for the port device for now. */
> > > +	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, "partner");
> > > +	if (ret)
> > > +		dev_WARN(&port->dev, "failed to create link to %s (%d)\n",
> > > +			 dev_name(dev), ret);
> > 
> > The attributes should be present as soon as the device is announced.
> 
> Yes, this is wrong and racy (hint, if you have to drop down to a sysfs
> call within a driver, almost always something is wrong...)

I'll remove those links.

I'm hoping to get comments about the attributes in general. Do we
have everything needed to describe the state and capabilities of USB
Type-C ports with them, and are they in reasonable places. I'm playing
with these links in couple of places in the code, and I've put
questions in the comments on some of them. I'm really not planning on
leaving them as they are.

So if you guys could provide your opinions on those questions, I would
much appreciate.


Thanks a lot,

-- 
heikki

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

#1403801

On Thu, 2016-05-19 at 15:44 +0300, 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 Alternate Modes.
> 
> 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  | 957 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h   | 230 +++++++++++
>  6 files changed, 1199 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
> 
> Hi,
> 
> Like I've told some of you guys, I'm trying to implement a bus for
> the Alternate Modes, but I'm still nowhere near finished with that
> one, so let's just get the class ready now. The altmode bus should in
> any case not affect the userspace interface proposed in this patch.
> 
> As you can see, the Alternate Modes are handled completely differently
> compared to the original proposal. Every Alternate Mode will have
> their own device instance (which will be then later bound to an
> Alternate Mode specific driver once we have the bus), but also every
> partner, cable and cable plug will have their own device instances
> representing them.

That raises a question. If I read the standard correctly, alternate
modes could be combinable. So how do you represent that.

> An other change is that the data role is now handled in two ways.
> The current_data_role file will represent static mode of the port, and
> it will use the names for the roles as they are defined in the spec:
> DFP, UFP and DRP. This file should be used if the port needs to be
> fixed to one specific role with DRP ports. So this approach will
> replace the suggestions for "preferred" data role we had. The
> current_usb_data_role will use values "host" and "device" and it will
> be used for data role swapping when already connected.

Please explain. How does that express DRP but prefered master?

> I Hope I remembered to CC everybody interested.

Alternate modes can be left involuntarily. So we need a method of
notification.

	Regards
		Oliver

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

#1404352

On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
> On Thu, 2016-05-19 at 15:44 +0300, 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 Alternate Modes.
> > 
> > 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  | 957 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/typec.h   | 230 +++++++++++
> >  6 files changed, 1199 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
> > 
> > Hi,
> > 
> > Like I've told some of you guys, I'm trying to implement a bus for
> > the Alternate Modes, but I'm still nowhere near finished with that
> > one, so let's just get the class ready now. The altmode bus should in
> > any case not affect the userspace interface proposed in this patch.
> > 
> > As you can see, the Alternate Modes are handled completely differently
> > compared to the original proposal. Every Alternate Mode will have
> > their own device instance (which will be then later bound to an
> > Alternate Mode specific driver once we have the bus), but also every
> > partner, cable and cable plug will have their own device instances
> > representing them.
> 
> That raises a question. If I read the standard correctly, alternate
> modes could be combinable. So how do you represent that.
> 
> > An other change is that the data role is now handled in two ways.
> > The current_data_role file will represent static mode of the port, and
> > it will use the names for the roles as they are defined in the spec:
> > DFP, UFP and DRP. This file should be used if the port needs to be
> > fixed to one specific role with DRP ports. So this approach will
> > replace the suggestions for "preferred" data role we had. The
> > current_usb_data_role will use values "host" and "device" and it will
> > be used for data role swapping when already connected.
> 
> Please explain. How does that express DRP but prefered master?

Sorry but I'm not sure what you mean here. If the port is capable of
being used as dual role port (DRP in the supported_data_roles file),
that is the only case where you can select the role with this file. So
I would imagine that in your case you want to make the port act as
DFP only, right? But if the port is capable of acting only as UFP, you
are stuck with that role.

> > I Hope I remembered to CC everybody interested.
> 
> Alternate modes can be left involuntarily. So we need a method of
> notification.

Yes, that is missing indeed.


Thanks,

-- 
heikki

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

#1404439

On Fri, 2016-05-20 at 14:24 +0300, Heikki Krogerus wrote:
> On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
> >
> > Please explain. How does that express DRP but prefered master?
> 
> Sorry but I'm not sure what you mean here. If the port is capable of
> being used as dual role port (DRP in the supported_data_roles file),
> that is the only case where you can select the role with this file. So
> I would imagine that in your case you want to make the port act as
> DFP only, right? But if the port is capable of acting only as UFP, you
> are stuck with that role.

How do I trigger that Try.SRC is to be used on a port?

	Regards
		Oliver

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

#1404815

On 05/20/2016 06:37 AM, Oliver Neukum wrote:
> On Fri, 2016-05-20 at 14:24 +0300, Heikki Krogerus wrote:
>> On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
>>>
>>> Please explain. How does that express DRP but prefered master?
>>
>> Sorry but I'm not sure what you mean here. If the port is capable of
>> being used as dual role port (DRP in the supported_data_roles file),
>> that is the only case where you can select the role with this file. So
>> I would imagine that in your case you want to make the port act as
>> DFP only, right? But if the port is capable of acting only as UFP, you
>> are stuck with that role.
>
> How do I trigger that Try.SRC is to be used on a port?
>

This would be part of the USB PD protocol, ie probably outside the scope
of the class code. In my implementation, I enable Try.SRC or Try.SNK based
on the platform's preferred role.

Guenter

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

#1404818

On Fri, 2016-05-20 at 22:51 -0700, Guenter Roeck wrote:
> On 05/20/2016 06:37 AM, Oliver Neukum wrote:
> > On Fri, 2016-05-20 at 14:24 +0300, Heikki Krogerus wrote:
> >> On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
> >>>
> >>> Please explain. How does that express DRP but prefered master?
> >>
> >> Sorry but I'm not sure what you mean here. If the port is capable of
> >> being used as dual role port (DRP in the supported_data_roles file),
> >> that is the only case where you can select the role with this file. So
> >> I would imagine that in your case you want to make the port act as
> >> DFP only, right? But if the port is capable of acting only as UFP, you
> >> are stuck with that role.
> >
> > How do I trigger that Try.SRC is to be used on a port?
> >
> 
> This would be part of the USB PD protocol, ie probably outside the scope
> of the class code. In my implementation, I enable Try.SRC or Try.SNK based
> on the platform's preferred role.

Hi,

from a purely formal point of view that makes sense.

From a usability viewpoint I'd ideally want all controls
for role at one place. And possibly the controls for modes
at the same place.

I think part of the problem here is that we lack a statement
of mission. What are these controls for?
Merely for controlling modes and providing information
about modes? And to use neutral terms only for the "master"
side or both sides?

	Regards
		Oliver

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

#1405020

Hi Oliver,

On 05/20/2016 11:43 PM, Oliver Neukum wrote:
> On Fri, 2016-05-20 at 22:51 -0700, Guenter Roeck wrote:
>> On 05/20/2016 06:37 AM, Oliver Neukum wrote:
>>> On Fri, 2016-05-20 at 14:24 +0300, Heikki Krogerus wrote:
>>>> On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
>>>>>
>>>>> Please explain. How does that express DRP but prefered master?
>>>>
>>>> Sorry but I'm not sure what you mean here. If the port is capable of
>>>> being used as dual role port (DRP in the supported_data_roles file),
>>>> that is the only case where you can select the role with this file. So
>>>> I would imagine that in your case you want to make the port act as
>>>> DFP only, right? But if the port is capable of acting only as UFP, you
>>>> are stuck with that role.
>>>
>>> How do I trigger that Try.SRC is to be used on a port?
>>>
>>
>> This would be part of the USB PD protocol, ie probably outside the scope
>> of the class code. In my implementation, I enable Try.SRC or Try.SNK based
>> on the platform's preferred role.
>
> Hi,
>
> from a purely formal point of view that makes sense.
>
>>From a usability viewpoint I'd ideally want all controls
> for role at one place. And possibly the controls for modes
> at the same place.
>
> I think part of the problem here is that we lack a statement
> of mission. What are these controls for?
> Merely for controlling modes and providing information
> about modes? And to use neutral terms only for the "master"
> side or both sides?
>

Good question. I originally added a sysfs attribute 'preferred-mode' to
my code, but then concluded that this is supposed to be provided
by the platform and added it as platform data instead, with (currently)
no means to report it to user space.

Heikki's current code doesn't really match the semantics of a 'preferred'
mode, at least not as I read it. In my understanding, 'preferred mode'
means "this is a DRP port which prefers to be a source/sink". In Heikki's
code, one can fix the mode to source or sink, but that doesn't support
situations such as "this port prefers to be a source, but is currently
a sink because the link partner happens to be a charger or refuses to act
as sink for some other reason".

Given that, my working assumption is that preferred mode handling is supposed
to be outside the scope of the infrastructure. I am happy to be corrected,
though.

On a side note, are you working on a USB-PD implementation as well ?

Thanks,
Guenter

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

#1405099

On Sun, 2016-05-22 at 08:54 -0700, Guenter Roeck wrote:
> Hi Oliver,
> 
> On 05/20/2016 11:43 PM, Oliver Neukum wrote:
> > On Fri, 2016-05-20 at 22:51 -0700, Guenter Roeck wrote:
> >> On 05/20/2016 06:37 AM, Oliver Neukum wrote:
> >>> On Fri, 2016-05-20 at 14:24 +0300, Heikki Krogerus wrote:
> >>>> On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
> >>>>>
> >>>>> Please explain. How does that express DRP but prefered master?
> >>>>
> >>>> Sorry but I'm not sure what you mean here. If the port is capable of
> >>>> being used as dual role port (DRP in the supported_data_roles file),
> >>>> that is the only case where you can select the role with this file. So
> >>>> I would imagine that in your case you want to make the port act as
> >>>> DFP only, right? But if the port is capable of acting only as UFP, you
> >>>> are stuck with that role.
> >>>
> >>> How do I trigger that Try.SRC is to be used on a port?
> >>>
> >>
> >> This would be part of the USB PD protocol, ie probably outside the scope
> >> of the class code. In my implementation, I enable Try.SRC or Try.SNK based
> >> on the platform's preferred role.
> >
> > Hi,
> >
> > from a purely formal point of view that makes sense.
> >
> >>From a usability viewpoint I'd ideally want all controls
> > for role at one place. And possibly the controls for modes
> > at the same place.
> >
> > I think part of the problem here is that we lack a statement
> > of mission. What are these controls for?
> > Merely for controlling modes and providing information
> > about modes? And to use neutral terms only for the "master"
> > side or both sides?
> >
> 
> Good question. I originally added a sysfs attribute 'preferred-mode' to
> my code, but then concluded that this is supposed to be provided
> by the platform and added it as platform data instead, with (currently)
> no means to report it to user space.

Mode or role? I would say that the choice of alternate modes belongs to
user space. THere's no point in giving a preference to the kernel.
Role is different. Try.SRC is part of the standard (and Try.SNK has been
added). It needs to be in kernel space.

Now, if you say that this API is for selecting modes only, then I say it
lacks support for cable resets and notifications thereof. Yes, that is a
PD feature, but that doesn't help us. It changes the mode.

> Heikki's current code doesn't really match the semantics of a 'preferred'
> mode, at least not as I read it. In my understanding, 'preferred mode'
> means "this is a DRP port which prefers to be a source/sink". In Heikki's
> code, one can fix the mode to source or sink, but that doesn't support
> situations such as "this port prefers to be a source, but is currently
> a sink because the link partner happens to be a charger or refuses to act
> as sink for some other reason".

Yes. That is why we need a mission statement. We may be talking
a rice cooker not preparing tea here.

> Given that, my working assumption is that preferred mode handling is supposed
> to be outside the scope of the infrastructure. I am happy to be corrected,
> though.
> 
> On a side note, are you working on a USB-PD implementation as well ?

No, one of my task is to to make use of USB PD (and alternate modes)
in a distribution.

	Regards
		Oliver

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

#1405365

On 05/22/2016 10:34 PM, Oliver Neukum wrote:
> On Sun, 2016-05-22 at 08:54 -0700, Guenter Roeck wrote:
>> Hi Oliver,
>>
>> On 05/20/2016 11:43 PM, Oliver Neukum wrote:
>>> On Fri, 2016-05-20 at 22:51 -0700, Guenter Roeck wrote:
>>>> On 05/20/2016 06:37 AM, Oliver Neukum wrote:
>>>>> On Fri, 2016-05-20 at 14:24 +0300, Heikki Krogerus wrote:
>>>>>> On Thu, May 19, 2016 at 04:47:17PM +0200, Oliver Neukum wrote:
>>>>>>>
>>>>>>> Please explain. How does that express DRP but prefered master?
>>>>>>
>>>>>> Sorry but I'm not sure what you mean here. If the port is capable of
>>>>>> being used as dual role port (DRP in the supported_data_roles file),
>>>>>> that is the only case where you can select the role with this file. So
>>>>>> I would imagine that in your case you want to make the port act as
>>>>>> DFP only, right? But if the port is capable of acting only as UFP, you
>>>>>> are stuck with that role.
>>>>>
>>>>> How do I trigger that Try.SRC is to be used on a port?
>>>>>
>>>>
>>>> This would be part of the USB PD protocol, ie probably outside the scope
>>>> of the class code. In my implementation, I enable Try.SRC or Try.SNK based
>>>> on the platform's preferred role.
>>>
>>> Hi,
>>>
>>> from a purely formal point of view that makes sense.
>>>
>>> >From a usability viewpoint I'd ideally want all controls
>>> for role at one place. And possibly the controls for modes
>>> at the same place.
>>>
>>> I think part of the problem here is that we lack a statement
>>> of mission. What are these controls for?
>>> Merely for controlling modes and providing information
>>> about modes? And to use neutral terms only for the "master"
>>> side or both sides?
>>>
>>
>> Good question. I originally added a sysfs attribute 'preferred-mode' to
>> my code, but then concluded that this is supposed to be provided
>> by the platform and added it as platform data instead, with (currently)
>> no means to report it to user space.
>
> Mode or role? I would say that the choice of alternate modes belongs to

s/preferred mode/preferred role/g

> user space. THere's no point in giving a preference to the kernel.
> Role is different. Try.SRC is part of the standard (and Try.SNK has been
> added). It needs to be in kernel space.
>
> Now, if you say that this API is for selecting modes only, then I say it
> lacks support for cable resets and notifications thereof. Yes, that is a
> PD feature, but that doesn't help us. It changes the mode.
>
>> Heikki's current code doesn't really match the semantics of a 'preferred'
>> mode, at least not as I read it. In my understanding, 'preferred mode'
>> means "this is a DRP port which prefers to be a source/sink". In Heikki's
>> code, one can fix the mode to source or sink, but that doesn't support
>> situations such as "this port prefers to be a source, but is currently
>> a sink because the link partner happens to be a charger or refuses to act
>> as sink for some other reason".
>
> Yes. That is why we need a mission statement. We may be talking
> a rice cooker not preparing tea here.
>
>> Given that, my working assumption is that preferred mode handling is supposed
>> to be outside the scope of the infrastructure. I am happy to be corrected,
>> though.
>>
>> On a side note, are you working on a USB-PD implementation as well ?
>
> No, one of my task is to to make use of USB PD (and alternate modes)
> in a distribution.
>
> 	Regards
> 		Oliver
>
>
>

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

#1405400

On Mon, 2016-05-23 at 06:27 -0700, Guenter Roeck wrote:
> >> Good question. I originally added a sysfs attribute
> 'preferred-mode' to
> >> my code, but then concluded that this is supposed to be provided
> >> by the platform and added it as platform data instead, with
> (currently)
> >> no means to report it to user space.
> >
> > Mode or role? I would say that the choice of alternate modes belongs
> to
> 
> s/preferred mode/preferred role/g

Now I am confused. Are you saying that the choice of Alternate Mode does
not belong into user space?

	Regards
		Oliver

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

#1405419

On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> On Mon, 2016-05-23 at 06:27 -0700, Guenter Roeck wrote:
>>>> Good question. I originally added a sysfs attribute
>> 'preferred-mode' to
>>>> my code, but then concluded that this is supposed to be provided
>>>> by the platform and added it as platform data instead, with
>> (currently)
>>>> no means to report it to user space.
>>>
>>> Mode or role? I would say that the choice of alternate modes belongs
>> to
>>
>> s/preferred mode/preferred role/g
>
> Now I am confused. Are you saying that the choice of Alternate Mode does
> not belong into user space?
>

No; sorry for the confusion. The above was meant to apply to my use
of "preferred mode", not yours. I was trying to say that the choice of
preferred roles (which determines if Try.SRC or Try.SNK is enabled)
should belong primarily into the kernel, to be determined by the platform
(presumably via ACPI, devicetree data, or platform data). If it should
be possible to override it by user space is a different question. That
might be useful, at least for testing. If so, does such an override
belong into the class or into the PD driver ? Good question. I am fine
either way.

I don't really have a strong opinion about alternate mode selection. I would
think that there should be a kernel (platform) default, possibly determined
by the alternate mode itself, but I also think that it should be selectable
by user space. Question is if that should be done through the alternate mode
driver or through the class (example: alternate modes used for firmware
upgrades and other device specific functionality). I don't really have
an answer for that at this point. I'll probably have a better idea after
I implemented the alternate mode driver for Google devices.

Again, sorry for creating confusion between preferred role and preferred
(alternate) mode selection.

Thanks,
Guenter

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

#1405473

On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> On 05/23/2016 06:58 AM, Oliver Neukum wrote:

> > Now I am confused. Are you saying that the choice of Alternate Mode does
> > not belong into user space?
> >
> 
> No; sorry for the confusion. The above was meant to apply to my use
> of "preferred mode", not yours. I was trying to say that the choice of
> preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> should belong primarily into the kernel, to be determined by the platform
> (presumably via ACPI, devicetree data, or platform data). If it should

Why on earth? That is most clearly a policy decision.

> be possible to override it by user space is a different question. That
> might be useful, at least for testing. If so, does such an override
> belong into the class or into the PD driver ? Good question. I am fine
> either way.

Well, if platform data has a default, I suppose we ought to use it.

> I don't really have a strong opinion about alternate mode selection. I would
> think that there should be a kernel (platform) default, possibly determined
> by the alternate mode itself, but I also think that it should be selectable
> by user space. Question is if that should be done through the alternate mode
> driver or through the class (example: alternate modes used for firmware

I would say that the ought to be a driver for type C which controls
alternate modes and roles.

	Regards
		Oliver

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

#1405516

On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> 
> > > Now I am confused. Are you saying that the choice of Alternate Mode does
> > > not belong into user space?
> > >
> > 
> > No; sorry for the confusion. The above was meant to apply to my use
> > of "preferred mode", not yours. I was trying to say that the choice of
> > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > should belong primarily into the kernel, to be determined by the platform
> > (presumably via ACPI, devicetree data, or platform data). If it should
> 
> Why on earth? That is most clearly a policy decision.
> 

The question is not that much if it is policy (it is), but if the policy
should be driven by the platform or by user space. I think there needs
to be at least a default driven by the platform. As already mentioned,
I am ok with a means to override this platform default from user space.
But if user space doesn't say, there still needs to be a default.

Thanks,
Guenter

> > be possible to override it by user space is a different question. That
> > might be useful, at least for testing. If so, does such an override
> > belong into the class or into the PD driver ? Good question. I am fine
> > either way.
> 
> Well, if platform data has a default, I suppose we ought to use it.
> 
> > I don't really have a strong opinion about alternate mode selection. I would
> > think that there should be a kernel (platform) default, possibly determined
> > by the alternate mode itself, but I also think that it should be selectable
> > by user space. Question is if that should be done through the alternate mode
> > driver or through the class (example: alternate modes used for firmware
> 
> I would say that the ought to be a driver for type C which controls
> alternate modes and roles.
> 
> 	Regards
> 		Oliver
> 
> 

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

#1406026

Hi Guenter,

On Mon, May 23, 2016 at 09:52:12AM -0700, Guenter Roeck wrote:
> On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> > On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> > 
> > > > Now I am confused. Are you saying that the choice of Alternate Mode does
> > > > not belong into user space?
> > > >
> > > 
> > > No; sorry for the confusion. The above was meant to apply to my use
> > > of "preferred mode", not yours. I was trying to say that the choice of
> > > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > > should belong primarily into the kernel, to be determined by the platform
> > > (presumably via ACPI, devicetree data, or platform data). If it should
> > 
> > Why on earth? That is most clearly a policy decision.
> > 
> 
> The question is not that much if it is policy (it is), but if the policy
> should be driven by the platform or by user space. I think there needs
> to be at least a default driven by the platform. As already mentioned,
> I am ok with a means to override this platform default from user space.
> But if user space doesn't say, there still needs to be a default.

I don't completely agree with that. The platform should not, and
actually in most cases with ACPI AFAIK, will not provide any
"preferences" to the OS about anything. The platform should only
provide the OS the physical capabilities and nothing else. So if for
example the platform is capable of acting as only source with a Type-C
port, that is what it needs to tell to the OS so possibly the PHY can
be programmed accordingly, etc.

So IMO, just like with any decision related to what the system will
ultimately be used for, decision about the preferred role really
belongs to the userspace.


Thanks,

-- 
heikki

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

#1406038

On Tue, 2016-05-24 at 13:08 +0300, Heikki Krogerus wrote:
> Hi Guenter,
> 
> On Mon, May 23, 2016 at 09:52:12AM -0700, Guenter Roeck wrote:
> > On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> > > On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > > > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> > > 
> > > > > Now I am confused. Are you saying that the choice of Alternate Mode does
> > > > > not belong into user space?
> > > > >
> > > > 
> > > > No; sorry for the confusion. The above was meant to apply to my use
> > > > of "preferred mode", not yours. I was trying to say that the choice of
> > > > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > > > should belong primarily into the kernel, to be determined by the platform
> > > > (presumably via ACPI, devicetree data, or platform data). If it should
> > > 
> > > Why on earth? That is most clearly a policy decision.
> > > 
> > 
> > The question is not that much if it is policy (it is), but if the policy
> > should be driven by the platform or by user space. I think there needs
> > to be at least a default driven by the platform. As already mentioned,
> > I am ok with a means to override this platform default from user space.
> > But if user space doesn't say, there still needs to be a default.
> 
> I don't completely agree with that. The platform should not, and
> actually in most cases with ACPI AFAIK, will not provide any
> "preferences" to the OS about anything. The platform should only
> provide the OS the physical capabilities and nothing else. So if for
> example the platform is capable of acting as only source with a Type-C
> port, that is what it needs to tell to the OS so possibly the PHY can
> be programmed accordingly, etc.

May I suggest that the point is moot as long as we agree that user space
needs to be able to set a policy? The OS cannot ignore the port before
user space tells it what to do. So a default will be needed.

The OS must act conservatively, which means it cannot deactivate
hardware. So if a port can do DRP, that must be the default.
I would go so far that I would suggest that we add a module parameter
for Try.SRC and Try.SNK to avoid trouble during boot.

> So IMO, just like with any decision related to what the system will
> ultimately be used for, decision about the preferred role really
> belongs to the userspace.

Yes, but ought the APIs for role, mode and PD be separate or not.
Sorry to sound like a broken record, but you need to provide some
higher level planning here.

	Regards
		Oliver

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

#1406068

On Tue, May 24, 2016 at 12:18:38PM +0200, Oliver Neukum wrote:
> On Tue, 2016-05-24 at 13:08 +0300, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Mon, May 23, 2016 at 09:52:12AM -0700, Guenter Roeck wrote:
> > > On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> > > > On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > > > > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> > > > 
> > > > > > Now I am confused. Are you saying that the choice of Alternate Mode does
> > > > > > not belong into user space?
> > > > > >
> > > > > 
> > > > > No; sorry for the confusion. The above was meant to apply to my use
> > > > > of "preferred mode", not yours. I was trying to say that the choice of
> > > > > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > > > > should belong primarily into the kernel, to be determined by the platform
> > > > > (presumably via ACPI, devicetree data, or platform data). If it should
> > > > 
> > > > Why on earth? That is most clearly a policy decision.
> > > > 
> > > 
> > > The question is not that much if it is policy (it is), but if the policy
> > > should be driven by the platform or by user space. I think there needs
> > > to be at least a default driven by the platform. As already mentioned,
> > > I am ok with a means to override this platform default from user space.
> > > But if user space doesn't say, there still needs to be a default.
> > 
> > I don't completely agree with that. The platform should not, and
> > actually in most cases with ACPI AFAIK, will not provide any
> > "preferences" to the OS about anything. The platform should only
> > provide the OS the physical capabilities and nothing else. So if for
> > example the platform is capable of acting as only source with a Type-C
> > port, that is what it needs to tell to the OS so possibly the PHY can
> > be programmed accordingly, etc.
> 
> May I suggest that the point is moot as long as we agree that user space
> needs to be able to set a policy? The OS cannot ignore the port before
> user space tells it what to do. So a default will be needed.
> 
> The OS must act conservatively, which means it cannot deactivate
> hardware. So if a port can do DRP, that must be the default.

So exactly my point.

> I would go so far that I would suggest that we add a module parameter
> for Try.SRC and Try.SNK to avoid trouble during boot.

I would be fine with that, but apparently module parameters are so 90s
:(

> > So IMO, just like with any decision related to what the system will
> > ultimately be used for, decision about the preferred role really
> > belongs to the userspace.
> 
> Yes, but ought the APIs for role, mode and PD be separate or not.
> Sorry to sound like a broken record, but you need to provide some
> higher level planning here.

I was kinda hoping that we could formulate the plan together in this
thread.


Thanks,

-- 
heikki

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

#1403885

Hello Heikki,

On Thu, May 19, 2016 at 03:44:54PM +0300, 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 Alternate Modes.
> 
> 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  | 957 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/typec.h   | 230 +++++++++++
>  6 files changed, 1199 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
> 
> Hi,
> 
> Like I've told some of you guys, I'm trying to implement a bus for
> the Alternate Modes, but I'm still nowhere near finished with that
> one, so let's just get the class ready now. The altmode bus should in
> any case not affect the userspace interface proposed in this patch.
> 
> As you can see, the Alternate Modes are handled completely differently
> compared to the original proposal. Every Alternate Mode will have
> their own device instance (which will be then later bound to an
> Alternate Mode specific driver once we have the bus), but also every
> partner, cable and cable plug will have their own device instances
> representing them.
> 
> An other change is that the data role is now handled in two ways.
> The current_data_role file will represent static mode of the port, and
> it will use the names for the roles as they are defined in the spec:
> DFP, UFP and DRP. This file should be used if the port needs to be
> fixed to one specific role with DRP ports. So this approach will
> replace the suggestions for "preferred" data role we had. The
> current_usb_data_role will use values "host" and "device" and it will
> be used for data role swapping when already connected.
> 

What I am missing completely is a means to handle role and alternate mode
changes triggered by the partner. The need for those should be obvious,
unless I am really missing something (just consider two devices supporting
this code connected to each other).

Also, I am not sure where the policy engine is supposed to reside.
I understand that some policy changes (eg unsolicited requests to switch roles)
can be triggered from user space. However, role change requests triggered from
the partner need to be evaluated quickly (typically within 15 ms), so user
space can not get involved. Maybe it would help to have some text describing
where the policy engine is expected to reside and how it is involved
in the decision making process. This includes the initial decision making
process, when it needs to be decided if role changes should be requested
or if one or multiple alternate modes should be entered after the initial
connection has been established.

On top of that, I am concerned about synchronization problems with role
changes triggered from user space. The driver is not told about the
desired role, only that it shall perform a role change. If a role change
triggered by the partner is ongoing at the time a role change request is
made from user space, it may well happen that the dual role change results
in a revert to the original role. Some synchronization primitives as well
as an API change might resolve that, but essentially it would mean that
drivers have to implement at least part of the policy engine. It might
make more sense to have that code in the infrastructure.

On disconnect, a port reverts to the default role. However, on port
registration, the port roles are left at 0, meaning they always
default to device/sink independent of the port's capabilities. Is this
on purpose ?

current_data_role_store() lets user space set a fixed port role. However,
the port role variables are not updated. How is this supposed to be handled ?
The same is true for other role change attributes - I don't see any code
to update the role variables. Presumably this would have to be done in the
class code, since the port data structure is private.

Overall, I am quite concerned by the lack of synchronization primitives
between the class code and port drivers, but also in the class code
itself. For example, nothing prevents multiple user space processes
from writing into the same (or different) attribute(s) repeatedly.

Thanks,
Guenter

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web