[PATCHv4 0/2] USB Type-C Connector class

[PATCHv4 0/2] USB Type-C Connector class

[Heikki Krogerus]

Hi,

The USB Type-C class is meant to provide unified interface to the
userspace to present the USB Type-C ports in a system.

Changes since v3:
- Documentation cleanup as proposed by Roger Quadros
- Setting partner altmodes member to NULL on removal and fixing a
  warning, as proposed by Guenter Roeck
- Added the following attributes for partners and cables:
  * supports_usb_power_delivery
  * id_header_vdo
- "id_header_vdo" is visible only when the partner or cable supports
  USB Power Delivery communication.
- Partner attribute "accessory" is hidden when the partner type is not
  "Accessory".

Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.



Heikki Krogerus (2):
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

 Documentation/ABI/testing/sysfs-class-typec |  236 +++++
 Documentation/usb/typec.txt                 |  103 +++
 MAINTAINERS                                 |    9 +
 drivers/usb/Kconfig                         |    2 +
 drivers/usb/Makefile                        |    2 +
 drivers/usb/typec/Kconfig                   |   21 +
 drivers/usb/typec/Makefile                  |    2 +
 drivers/usb/typec/typec.c                   | 1277 +++++++++++++++++++++++++++
 drivers/usb/typec/typec_wcove.c             |  371 ++++++++
 include/linux/usb/typec.h                   |  260 ++++++
 10 files changed, 2283 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.8.1

[PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

The purpose of USB Type-C connector class is to provide
unified interface for the user space to get the status and
basic information about USB Type-C connectors on a system,
control over data role swapping, and when the port supports
USB Power Delivery, also control over power role swapping
and Alternate Modes.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-class-typec |  236 +++++
 Documentation/usb/typec.txt                 |  103 +++
 MAINTAINERS                                 |    9 +
 drivers/usb/Kconfig                         |    2 +
 drivers/usb/Makefile                        |    2 +
 drivers/usb/typec/Kconfig                   |    7 +
 drivers/usb/typec/Makefile                  |    1 +
 drivers/usb/typec/typec.c                   | 1277 +++++++++++++++++++++++++++
 include/linux/usb/typec.h                   |  260 ++++++
 9 files changed, 1897 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
new file mode 100644
index 0000000..7cd40e5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,236 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:		/sys/class/typec/<port>/current_data_role
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The current USB data role the port is operating in. This
+		attribute can be used for requesting data role swapping on the
+		port.
+
+		Valid values:
+		- host
+		- device
+
+What:		/sys/class/typec/<port>/current_power_role
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The current power role of the port. This attribute can be used
+		to request power role swap on the port when the port supports
+		USB Power Delivery.
+
+		Valid values:
+		- source
+		- sink
+
+What:		/sys/class/typec/<port>/current_vconn_role
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows the current VCONN role of the port. This attribute can be
+		used to request VCONN role swap on the port when the port
+		supports USB Power Delivery.
+
+		Valid values are:
+		- source
+		- sink
+
+What:		/sys/class/typec/<port>/power_operation_mode
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows the current power operational mode the port is in.
+
+		Valid values:
+		- USB - Normal power levels defined in USB specifications
+		- BC1.2 - Power levels defined in Battery Charging Specification
+			  v1.2
+		- USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
+				    specification.
+		- USB Type-C 3.0A - Higher 3A current defined in USB Type-C
+				    specification.
+                - USB Power Delivery - The voltages and currents defined in USB
+				       Power Delivery specification
+
+What:		/sys/class/typec/<port>/preferred_role
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The user space can notify the driver about the preferred role.
+		It should be handled as enabling of Try.SRC or Try.SNK, as
+		defined in USB Type-C specification, in the port drivers. By
+		default there is no preferred role.
+
+		Valid values:
+		- host
+		- device
+		- For example "none" to remove preference (anything else except
+		  "host" or "device")
+
+What:		/sys/class/typec/<port>/supported_accessory_modes
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Lists the Accessory Modes, defined in the USB Type-C
+		specification, the port supports.
+
+What:		/sys/class/typec/<port>/supported_data_roles
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Lists the USB data roles the port is capable of supporting.
+
+		Valid values:
+		- device
+		- host
+		- device, host (DRD as defined in USB Type-C specification v1.2)
+
+What:		/sys/class/typec/<port>/supported_power_roles
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Lists the power roles the port is capable of supporting.
+
+		Valid values:
+		- source
+		- sink
+
+What:		/sys/class/typec/<port>/supports_usb_power_delivery
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows if the port supports USB Power Delivery.
+		- 1 if USB Power Delivery is supported
+		- 0 when it's not
+
+
+USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
+
+What:		/sys/class/typec/<port>-partner/accessory
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		The attribute is visible only when the partner's type is
+		"Accessory". The type can be read from its own attribute.
+
+		Shows the name of the Accessory Mode. The Accessory Modes are
+		defined in USB Type-C Specification.
+
+What:		/sys/class/typec/<port>-partner/type
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows the type of the partner. Can be one of the following:
+		- USB - When the partner is normal USB host/peripheral.
+		- Charger - When the partner has been identified as dedicated
+			    charger.
+		- Alternate Mode - When the partner supports Alternate Modes.
+		- Accessory - When the partner is one of the accessories with
+			      specific Accessory Mode defined in USB Type-C
+			      specification.
+
+What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows if the partner supports USB Power Delivery.
+		- 1 if USB Power Delivery is supported
+		- 0 when it's not
+
+
+What:		/sys/class/typec/<port>-partner/id_header_vdo
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		If the partner supports USB Power Deliver, shows the VDO
+		returned from Discover Identity USB Power Delivery command.
+
+		If the partner does not support USB Power Delivery, the
+		attribute is hidden.
+
+
+USB Type-C cable devices (eg. /sys/class/typec/usbc0-cable/)
+
+Note: Electronically Marked Cables will have a device also for one cable plug
+(eg. /sys/class/typec/usbc0-plug0). If the cable is active and has also SOP
+Double Prime controller (USB Power Deliver specification ch. 2.4) it will have
+second device also for the other plug. Both plugs may have their alternate modes
+as described in USB Type-C and USB Power Delivery specifications.
+
+What:		/sys/class/typec/<port>-cable/active
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows if the cable is active or passive.
+
+		Valid values:
+		- 0 when the cable is passive
+		- 1 when the cable is active
+
+What:		/sys/class/typec/<port>-cable/plug_type
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows type of the plug on the cable:
+		- Type-A - Standard A
+		- Type-B - Standard B
+		- Type-C - USB Type-C
+		- Captive - Non-standard
+
+What:		/sys/class/typec/<port>-cable/supports_usb_power_delivery
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows if the cable supports USB Power Delivery communication.
+		- 1 if USB Power Delivery is supported
+		- 0 when it's not
+
+What:		/sys/class/typec/<port>-cable/id_header_vdo
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		If the cable supports USB Power Deliver, shows the VDO
+		returned from Discover Identity USB Power Delivery command.
+
+		If the cable does not support USB Power Delivery, the
+		attribute is hidden.
+
+
+Alternate Mode devices (For example,
+/sys/class/typec/usbc0-partner/usbc0-partner.svid:xxxx/). The ports, partners
+and cable plugs can have alternate modes.
+
+What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/active
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows if the mode is active or not. The attribute can be used
+		for entering/exiting the mode with partners and cable plugs, and
+		with the port alternate modes it can be used for disabling
+		support for specific alternate modes.
+
+What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/description
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows description of the mode. The description is optional for
+		the drivers, just like with the Billboard Devices.
+
+What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/vdo
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows the VDO in hexadecimal returned from the Discover Modes
+		command.
+
+What:		/sys/class/typec/<port>/<port>.svid:<svid>/<mode>/supported_roles
+Date:		June 2016
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Shows the roles, source or sink, the mode is supported with.
+
+		This attribute is available for the devices describing the
+		alternate modes a port supports, and it will not be exposed with
+		the devices presenting the alternate modes the partners or cable
+		plugs support.
diff --git a/Documentation/usb/typec.txt b/Documentation/usb/typec.txt
new file mode 100644
index 0000000..dce5f07
--- /dev/null
+++ b/Documentation/usb/typec.txt
@@ -0,0 +1,103 @@
+USB Type-C connector class
+==========================
+
+Introduction
+------------
+The typec class is meant for describing the USB Type-C ports in a system to the
+user space in unified fashion. The class is designed to provide nothing else
+except the user space interface implementation in hope that it can be utilized
+on as many platforms as possible.
+
+The platforms are expected to register every USB Type-C port they have with the
+class. In a normal case the registration will be done by a USB Type-C or PD PHY
+driver, but it may be a driver for firmware interface such as UCSI, driver for
+USB PD controller or even driver for Thunderbolt3 controller. This document
+considers the component registering the USB Type-C ports with the class as "port
+driver".
+
+On top of showing the capabilities, the class also offer the user space control
+over the roles and alternate modes they support when the port driver is capable
+of supporting those features.
+
+The class provides an API for the port drivers described in this document. The
+attributes are described in Documentation/ABI/testing/sysfs-class-typec.
+
+
+Interface
+---------
+Every port will be presented as its own device under /sys/class/typec/. The
+first port will be named "usbc0", the second "usbc1" and so on.
+
+When connected, the partner will be presented also as its own device under
+/sys/class/typec/. The parent of the partner device will always be the port. The
+partner attached to port "usbc0" will be named "usbc0-partner". Full patch to
+the device would be /sys/class/typec/usb0/usb0-partner/.
+
+The cable and the two plugs on it may also be optionally presented as their own
+devices under /sys/class/typec/. The cable attached to the port "usbc0" port
+will be named usbc0-cable and the plug on the SOP Prime end (see USB Power
+Delivery Specification ch. 2.4) will be named "usbc-plug0" and on the SOP Double
+Prime end "usbc0-plug1". The parent of a cable will always be the port, and the
+parent of the cable plugs will always be the cable.
+
+If the port, partner or cable plug support Alternate Modes, every Alternate Mode
+SVID will have their own device describing them. The Alternate Modes will not be
+attached to the typec class. For the port's "usbc0" partner, the Alternate Modes
+would have devices presented under /sys/class/typec/usbc0-partner/. Every mode
+that is supported will have its own group under the Alternate Mode device named
+"mode<id>". For example /sys/class/typec/usbc0/usbc0.svid:xxxx/mode0/. The
+requests for entering/exiting the modes happens with the "active" attribute in
+that group.
+
+
+API
+---
+
+* Registering the ports
+
+The port drivers will describe every Type-C port they control with struct
+typec_capability data structure, and register them with the following API:
+
+struct typec_port *typec_register_port(struct device *dev,
+				       const struct typec_capability *cap);
+
+The class will provide handle to struct typec_port on success and ERR_PTR on
+failure. The un-registration of the port happens with the following API:
+
+void typec_unregister_port(struct typec_port *port);
+
+
+* Notifications
+
+When connection happens on a port, the port driver fills struct typec_connection
+which is passed to the class. The class provides the following API for reporting
+connection/disconnection:
+
+int typec_connect(struct typec_port *port, struct typec_connection *);
+void typec_disconnect(struct typec_port *);
+
+When the partner end has executed a role change, the port driver uses the
+following APIs to report it to the class:
+
+void typec_set_data_role(struct typec_port *, enum typec_data_role);
+void typec_set_pwr_role(struct typec_port *, enum typec_role);
+void typec_set_vconn_role(struct typec_port *, enum typec_role);
+void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
+
+
+* Alternate Modes
+
+After connection, the port drivers register the alternate modes the partner
+and/or cable plugs support. And before reporting disconnection, the port driver
+_must_ unregister all the alternate modes registered for the partner and cable
+plugs. The API takes the struct device of the partner or the cable plug as
+parameter:
+
+int typec_register_altmodes(struct device *, struct typec_altmode *);
+void typec_unregister_altmodes(struct device *);
+
+When the partner end enters or exits the modes, the port driver needs to notify
+the class with the following API:
+
+void typec_altmode_update_active(struct typec_altmode *alt, int mode,
+				 bool active);
diff --git a/MAINTAINERS b/MAINTAINERS
index 952fd2a..8fe0977 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11983,6 +11983,15 @@ F:	drivers/usb/
 F:	include/linux/usb.h
 F:	include/linux/usb/
 
+USB TYPEC SUBSYSTEM
+M:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+L:	linux-usb@vger.kernel.org
+S:	Maintained
+F:	Documentation/ABI/testing/sysfs-class-typec
+F:	Documentation/usb/typec.txt
+F:	drivers/usb/typec/
+F:	include/linux/usb/typec.h
+
 USB UHCI DRIVER
 M:	Alan Stern <stern@rowland.harvard.edu>
 L:	linux-usb@vger.kernel.org
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 8689dcb..f42a3d3 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/typec/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..51e381e 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)		+= typec/
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
new file mode 100644
index 0000000..b229fb9
--- /dev/null
+++ b/drivers/usb/typec/Kconfig
@@ -0,0 +1,7 @@
+
+menu "USB PD and Type-C drivers"
+
+config TYPEC
+	tristate
+
+endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
new file mode 100644
index 0000000..1012a8b
--- /dev/null
+++ b/drivers/usb/typec/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_TYPEC)		+= typec.o
diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
new file mode 100644
index 0000000..bbfd6e5
--- /dev/null
+++ b/drivers/usb/typec/typec.c
@@ -0,0 +1,1277 @@
+/*
+ * 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; /* device lock */
+
+	int			prefer_role;
+
+	enum typec_data_role	data_role;
+	enum typec_role		pwr_role;
+	enum typec_role		vconn_role;
+	enum typec_pwr_opmode	pwr_opmode;
+
+	struct typec_partner	*partner;
+	struct typec_cable	*cable;
+
+	unsigned int		connected:1;
+
+	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 = "typec",
+};
+
+static const char * const typec_accessory_modes[] = {
+	[TYPEC_ACCESSORY_NONE]		= "none",
+	[TYPEC_ACCESSORY_AUDIO]		= "Audio",
+	[TYPEC_ACCESSORY_DEBUG]		= "Debug",
+	[TYPEC_ACCESSORY_DAUDIO]	= "Digital Audio",
+};
+
+/* ------------------------------------------------------------------------- */
+/* 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_ACCESSORY]	= "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 ssize_t
+partner_accessory_mode_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct typec_partner *partner = container_of(dev, struct typec_partner,
+						     dev);
+
+	return sprintf(buf, "%s\n", typec_accessory_modes[partner->accessory]);
+}
+
+static struct device_attribute dev_attr_partner_accessory = {
+	.attr = {
+		.name = "accessory",
+		.mode = S_IRUGO,
+	},
+	.show = partner_accessory_mode_show,
+};
+
+static ssize_t partner_usb_pd_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct typec_partner *partner = container_of(dev, struct typec_partner,
+						     dev);
+
+	return sprintf(buf, "%d\n", partner->usb_pd);
+}
+
+static struct device_attribute dev_attr_partner_usb_pd = {
+	.attr = {
+		.name = "supports_usb_power_delivery",
+		.mode = S_IRUGO,
+	},
+	.show = partner_usb_pd_show,
+};
+
+static ssize_t partner_vdo_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct typec_partner *partner = container_of(dev, struct typec_partner,
+						     dev);
+
+	return sprintf(buf, "0x%08x\n", partner->vdo);
+}
+
+static struct device_attribute dev_attr_partner_vdo = {
+	.attr = {
+		.name = "id_header_vdo",
+		.mode = S_IRUGO,
+	},
+	.show = partner_vdo_show,
+};
+
+static struct attribute *typec_partner_attrs[] = {
+	&dev_attr_partner_accessory.attr,
+	&dev_attr_partner_type.attr,
+	&dev_attr_partner_usb_pd.attr,
+	&dev_attr_partner_vdo.attr,
+	NULL
+};
+
+static umode_t partner_is_visible(struct kobject *kobj, struct attribute *attr,
+				  int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct typec_partner *partner = container_of(dev, struct typec_partner,
+						     dev);
+
+	if (attr == &dev_attr_partner_accessory.attr &&
+	    partner->type != TYPEC_PARTNER_ACCESSORY)
+		return 0;
+
+	if (attr == &dev_attr_partner_vdo.attr && !partner->usb_pd)
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute_group typec_partner_group = {
+	.attrs = typec_partner_attrs,
+	.is_visible = partner_is_visible,
+};
+
+static const struct attribute_group *typec_partner_groups[] = {
+	&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;
+	int ret;
+
+	dev->class = &typec_class;
+	dev->parent = &port->dev;
+	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;
+	}
+
+	port->partner = partner;
+	return 0;
+}
+
+static void typec_remove_partner(struct typec_port *port)
+{
+	WARN_ON(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[8];
+	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;
+	}
+
+	return 0;
+}
+
+static void typec_remove_plug(struct typec_plug *plug)
+{
+	WARN_ON(plug->alt_modes);
+	device_unregister(&plug->dev);
+}
+
+/* Type-C Cables */
+
+static ssize_t
+cable_active_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_cable_active = {
+	.attr = {
+		.name = "active",
+		.mode = S_IRUGO,
+	},
+	.show = cable_active_show,
+};
+
+static ssize_t cable_usb_pd_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->usb_pd);
+}
+
+static struct device_attribute dev_attr_cable_usb_pd = {
+	.attr = {
+		.name = "supports_usb_power_delivery",
+		.mode = S_IRUGO,
+	},
+	.show = cable_usb_pd_show,
+};
+
+static ssize_t cable_vdo_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
+
+	return sprintf(buf, "0x%08x\n", cable->vdo);
+}
+
+static struct device_attribute dev_attr_cable_vdo = {
+	.attr = {
+		.name = "id_header_vdo",
+		.mode = S_IRUGO,
+	},
+	.show = cable_vdo_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_cable_active.attr,
+	&dev_attr_cable_usb_pd.attr,
+	&dev_attr_cable_vdo.attr,
+	&dev_attr_plug_type.attr,
+	NULL
+};
+
+static umode_t cable_is_visible(struct kobject *kobj, struct attribute *attr,
+				int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
+
+	if (attr == &dev_attr_cable_vdo.attr && !cable->usb_pd)
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute_group typec_cable_group = {
+	.attrs = typec_cable_attrs,
+	.is_visible = cable_is_visible,
+};
+
+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;
+	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->usb_pd)
+		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->active || !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);
+}
+
+/* ------------------------------------------------------------------------- */
+/* API for the port drivers */
+
+static void typec_init_roles(struct typec_port *port)
+{
+	if (port->prefer_role < 0)
+		return;
+
+	if (port->prefer_role == TYPEC_SOURCE) {
+		port->data_role = TYPEC_HOST;
+		port->pwr_role = TYPEC_SOURCE;
+		port->vconn_role = TYPEC_SOURCE;
+	} else {
+		/* Device mode as default also by default with DRP ports */
+		port->data_role = TYPEC_DEVICE;
+		port->pwr_role = TYPEC_SINK;
+		port->vconn_role = TYPEC_SINK;
+	}
+}
+
+int typec_connect(struct typec_port *port, struct typec_connection *con)
+{
+	int ret;
+
+	if (!con->partner && !con->cable)
+		return -EINVAL;
+
+	port->connected = 1;
+	port->data_role = con->data_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;
+
+	typec_init_roles(port);
+
+	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
+}
+EXPORT_SYMBOL_GPL(typec_disconnect);
+
+/* --------------------------------------- */
+/* Driver callbacks to report role updates */
+
+void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
+{
+	mutex_lock(&port->lock);
+	port->data_role = role;
+	sysfs_notify(&port->dev.kobj, NULL, "current_data_role");
+	mutex_unlock(&port->lock);
+}
+EXPORT_SYMBOL(typec_set_data_role);
+
+void typec_set_pwr_role(struct typec_port *port, enum typec_role role)
+{
+	mutex_lock(&port->lock);
+	port->pwr_role = role;
+	sysfs_notify(&port->dev.kobj, NULL, "current_power_role");
+	mutex_unlock(&port->lock);
+}
+EXPORT_SYMBOL(typec_set_pwr_role);
+
+void typec_set_vconn_role(struct typec_port *port, enum typec_role role)
+{
+	mutex_lock(&port->lock);
+	port->vconn_role = role;
+	sysfs_notify(&port->dev.kobj, NULL, "current_vconn_role");
+	mutex_unlock(&port->lock);
+}
+EXPORT_SYMBOL(typec_set_vconn_role);
+
+void typec_set_pwr_opmode(struct typec_port *port,
+			  enum typec_pwr_opmode opmode)
+{
+	mutex_lock(&port->lock);
+	port->pwr_opmode = opmode;
+	sysfs_notify(&port->dev.kobj, NULL, "power_operation_mode");
+	mutex_unlock(&port->lock);
+}
+EXPORT_SYMBOL(typec_set_pwr_opmode);
+
+/* -------------------------------- */
+/* Alternate Modes */
+
+/*
+ * typec_altmode_update_active - Notify about Enter/Exit mode
+ * @alt: Handle to the Alternate Mode
+ * @mode: Mode id
+ * @active: True when the mode has been enterred
+ */
+void typec_altmode_update_active(struct typec_altmode *alt, int mode,
+				 bool active)
+{
+	struct typec_port *port = typec_altmode2port(alt);
+	struct typec_mode *m = alt->modes + mode;
+	char dir[6];
+
+	mutex_lock(&port->lock);
+	m->active = active;
+	sprintf(dir, "mode%d", mode);
+	sysfs_notify(&alt->dev.kobj, dir, "active");
+	mutex_unlock(&port->lock);
+}
+EXPORT_SYMBOL(typec_altmode_update_active);
+
+static struct device_type typec_port_dev_type;
+
+/*
+ * 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.
+ */
+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);
+	if (alt->dev.parent->type == &typec_port_dev_type)
+		return to_typec_port(alt->dev.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);
+	struct typec_port *port = typec_altmode2port(mode->alt_mode);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "0x%08x\n", mode->vdo);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+
+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);
+	struct typec_port *port = typec_altmode2port(mode->alt_mode);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+
+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);
+	struct typec_port *port = typec_altmode2port(mode->alt_mode);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%d\n", mode->active);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+
+static ssize_t
+typec_altmode_active_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	struct typec_mode *mode = container_of(attr, struct typec_mode,
+					       active_attr);
+	struct typec_port *port = typec_altmode2port(mode->alt_mode);
+	bool activate;
+	int ret;
+
+	ret = kstrtobool(buf, &activate);
+	if (ret)
+		return ret;
+
+	if (activate > 1)
+		return -EINVAL;
+
+	mutex_lock(&port->lock);
+	ret = port->cap->activate_mode(mode->alt_mode, mode->index, activate);
+	if (!ret) {
+		mode->active = activate;
+		ret = size;
+	}
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+
+static ssize_t
+typec_altmode_roles_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct typec_mode *mode = container_of(attr, struct typec_mode,
+					       roles_attr);
+	struct typec_port *port = typec_altmode2port(mode->alt_mode);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	switch (mode->roles) {
+	case TYPEC_PORT_DFP:
+		ret =  sprintf(buf, "source\n");
+		break;
+	case TYPEC_PORT_UFP:
+		ret = sprintf(buf, "sink\n");
+		break;
+	case TYPEC_PORT_DRP:
+	default:
+		ret = sprintf(buf, "source, sink\n");
+		break;
+	}
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+
+static void typec_init_modes(struct typec_altmode *alt, int is_port)
+{
+	struct typec_mode *mode = alt->modes;
+	int i;
+
+	for (i = 0; i < alt->n_modes; i++, mode++) {
+		mode->alt_mode = alt;
+		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_IWUSR | S_IRUGO;
+		mode->active_attr.show = typec_altmode_active_show;
+		mode->active_attr.store = typec_altmode_active_store;
+
+		mode->attrs[0] = &mode->vdo_attr.attr;
+		mode->attrs[1] = &mode->desc_attr.attr;
+		mode->attrs[2] = &mode->active_attr.attr;
+
+		/* With ports, list the roles that the mode is supported with */
+		if (is_port) {
+			sysfs_attr_init(&mode->roles_attr.attr);
+			mode->roles_attr.attr.name = "supported_roles";
+			mode->roles_attr.attr.mode = S_IRUGO;
+			mode->roles_attr.show = typec_altmode_roles_show;
+
+			mode->attrs[3] = &mode->roles_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, int is_port)
+{
+	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, is_port);
+
+	dev->groups = alt->mode_groups;
+	dev->release = typec_altmode_release;
+	dev->parent = parent;
+	/* TODO: if (!is_port) dev->bus = &typec_altmode_bus; */
+
+	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;
+}
+
+static int __typec_register_altmodes(struct device *parent,
+				     struct typec_altmode *alt_modes,
+				     int is_port)
+{
+	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, is_port);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	for (alt = alt_modes + index; index; alt--, index--)
+		device_unregister(&alt->dev);
+
+	return ret;
+}
+
+int typec_register_altmodes(struct device *dev, struct typec_altmode *alt_modes)
+{
+	if (dev->type == &typec_partner_dev_type) {
+		struct typec_partner *p = container_of(dev,
+						       struct typec_partner,
+						       dev);
+		p->alt_modes = alt_modes;
+	} else if (dev->type == &typec_plug_dev_type) {
+		struct typec_plug *p = container_of(dev, struct typec_plug,
+						    dev);
+		p->alt_modes = alt_modes;
+	} else {
+		return -ENODEV;
+	}
+
+	return __typec_register_altmodes(dev, alt_modes, false);
+}
+EXPORT_SYMBOL_GPL(typec_register_altmodes);
+
+void typec_unregister_altmodes(struct device *dev)
+{
+	struct typec_altmode *alt_modes = NULL;
+	struct typec_altmode *alt;
+
+	if (dev->type == &typec_partner_dev_type) {
+		struct typec_partner *p = container_of(dev,
+						       struct typec_partner,
+						       dev);
+		alt_modes = p->alt_modes;
+		p->alt_modes = NULL;
+	} else if (dev->type == &typec_plug_dev_type) {
+		struct typec_plug *p = container_of(dev, struct typec_plug,
+						    dev);
+		alt_modes = p->alt_modes;
+		p->alt_modes = NULL;
+	}
+
+	if (!alt_modes)
+		return;
+
+	for (alt = alt_modes; alt->svid; alt++)
+		device_unregister(&alt->dev);
+}
+EXPORT_SYMBOL_GPL(typec_unregister_altmodes);
+
+/* ------------------------------------------------------------------------- */
+
+static const char * const typec_roles[] = {
+	[TYPEC_SINK]	= "sink",
+	[TYPEC_SOURCE]	= "source",
+};
+
+static const char * const typec_data_roles[] = {
+	[TYPEC_DEVICE]	= "device",
+	[TYPEC_HOST]	= "host",
+};
+
+static ssize_t
+preferred_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_role role;
+	int ret;
+
+	mutex_lock(&port->lock);
+
+	if (port->cap->type != TYPEC_PORT_DRP) {
+		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (!port->cap->try_role) {
+		dev_dbg(dev, "Setting preferred role not supported\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
+	if (ret < 0) {
+		port->prefer_role = -1;
+		ret = size;
+		goto out;
+	}
+
+	role = ret;
+
+	ret = port->cap->try_role(port->cap, role);
+	if (ret)
+		goto out;
+
+	port->prefer_role = role;
+	ret = size;
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+
+static ssize_t
+preferred_role_show(struct device *dev, struct device_attribute *attr,
+		    char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	ssize_t ret = 0;
+
+	mutex_lock(&port->lock);
+
+	if (port->prefer_role < 0)
+		goto out;
+
+	ret = sprintf(buf, "%s\n", typec_roles[port->prefer_role]);
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+static DEVICE_ATTR_RW(preferred_role);
+
+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);
+	int ret = size;
+
+	mutex_lock(&port->lock);
+
+	if (port->cap->type != TYPEC_PORT_DRP) {
+		dev_dbg(dev, "data role swap only supported with DRP ports\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (!port->cap->dr_set) {
+		dev_dbg(dev, "data role swapping not supported\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (!port->connected)
+		goto out;
+
+	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
+	if (ret < 0)
+		goto out;
+
+	ret = port->cap->dr_set(port->cap, ret);
+	if (ret)
+		goto out;
+
+	ret = size;
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+
+static ssize_t
+current_data_role_show(struct device *dev, struct device_attribute *attr,
+		       char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%s\n", typec_data_roles[port->data_role]);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(current_data_role);
+
+static ssize_t supported_data_roles_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	size_t ret;
+
+	mutex_lock(&port->lock);
+
+	if (port->cap->type == TYPEC_PORT_DRP) {
+		ret = sprintf(buf, "host, device\n");
+		goto out;
+	}
+
+	ret = sprintf(buf, "%s\n", typec_data_roles[port->data_role]);
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+static DEVICE_ATTR_RO(supported_data_roles);
+
+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);
+	int ret = size;
+
+	mutex_lock(&port->lock);
+
+	if (!port->cap->usb_pd) {
+		dev_dbg(dev, "power role swap only supported with USB PD\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (!port->cap->pr_set) {
+		dev_dbg(dev, "power role swapping not supported\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
+		dev_dbg(dev, "partner unable to swap power role\n");
+		ret = -EIO;
+		goto out;
+	}
+
+	if (!port->connected)
+		goto out;
+
+	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
+	if (ret < 0)
+		goto out;
+
+	ret = port->cap->pr_set(port->cap, ret);
+	if (ret)
+		goto out;
+
+	ret = size;
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+
+static ssize_t current_power_role_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%s\n", typec_roles[port->pwr_role]);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+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);
+	size_t ret;
+
+	mutex_lock(&port->lock);
+
+	if (port->cap->usb_pd || port->cap->type == TYPEC_PORT_DRP) {
+		ret = sprintf(buf, "source, sink\n");
+		goto out;
+	}
+
+	ret = sprintf(buf, "%s\n", typec_roles[port->pwr_role]);
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+static DEVICE_ATTR_RO(supported_power_roles);
+
+static const char * const typec_pwr_opmodes[] = {
+	[TYPEC_PWR_MODE_USB]	= "USB",
+	[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);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%s\n", typec_pwr_opmodes[port->pwr_opmode]);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(power_operation_mode);
+
+static ssize_t current_vconn_role_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t size)
+{
+	struct typec_port *port = to_typec_port(dev);
+	int ret;
+
+	mutex_lock(&port->lock);
+
+	if (!port->cap->usb_pd) {
+		dev_dbg(dev, "vconn swap only supported with USB PD\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	if (!port->cap->vconn_set) {
+		dev_dbg(dev, "vconn swapping not supported\n");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
+	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
+	if (ret < 0)
+		goto out;
+
+	ret = port->cap->vconn_set(port->cap, ret);
+	if (ret)
+		goto out;
+
+	ret = size;
+out:
+	mutex_unlock(&port->lock);
+	return ret;
+}
+
+static ssize_t current_vconn_role_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%s\n", typec_roles[port->vconn_role]);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RW(current_vconn_role);
+
+static ssize_t supported_accessory_modes_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+	enum typec_accessory *accessory;
+	ssize_t ret = 0;
+	int i;
+
+	mutex_lock(&port->lock);
+	if (port->cap->accessory)
+		for (accessory = port->cap->accessory, i = 0;
+		     i < port->cap->num_accessory; accessory++, i++)
+			ret += sprintf(buf, "%s\n",
+				       typec_accessory_modes[*accessory]);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(supported_accessory_modes);
+
+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);
+	ssize_t ret;
+
+	mutex_lock(&port->lock);
+	ret = sprintf(buf, "%d\n", port->cap->usb_pd);
+	mutex_unlock(&port->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(supports_usb_power_delivery);
+
+static struct attribute *typec_attrs[] = {
+	&dev_attr_current_power_role.attr,
+	&dev_attr_current_vconn_role.attr,
+	&dev_attr_current_data_role.attr,
+	&dev_attr_power_operation_mode.attr,
+	&dev_attr_preferred_role.attr,
+	&dev_attr_supported_accessory_modes.attr,
+	&dev_attr_supported_data_roles.attr,
+	&dev_attr_supported_power_roles.attr,
+	&dev_attr_supports_usb_power_delivery.attr,
+	NULL,
+};
+
+static const struct attribute_group typec_group = {
+	.attrs = typec_attrs,
+};
+
+static const struct attribute_group *typec_groups[] = {
+	&typec_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,
+				       const 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);
+	}
+
+	/* FIXME: a better approach for this */
+	port->prefer_role = -1;
+
+	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);
+
+	typec_init_roles(port);
+
+	ret = device_register(&port->dev);
+	if (ret)
+		goto reg_err;
+
+	ret = __typec_register_altmodes(&port->dev, cap->alt_modes, true);
+	if (ret)
+		goto alt_err;
+
+	return port;
+alt_err:
+	device_unregister(&port->dev);
+reg_err:
+	ida_simple_remove(&typec_index_ida, id);
+	put_device(&port->dev);
+	kfree(port);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(typec_register_port);
+
+void typec_unregister_port(struct typec_port *port)
+{
+	struct typec_altmode *alt;
+
+	WARN_ON(port->connected);
+
+	if (port->cap->alt_modes)
+		for (alt = port->cap->alt_modes; alt->svid; alt++)
+			device_unregister(&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..eda6747
--- /dev/null
+++ b/include/linux/usb/typec.h
@@ -0,0 +1,260 @@
+
+#ifndef __LINUX_USB_TYPEC_H
+#define __LINUX_USB_TYPEC_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+struct typec_port;
+
+enum typec_port_type {
+	TYPEC_PORT_DFP,
+	TYPEC_PORT_UFP,
+	TYPEC_PORT_DRP,
+};
+
+enum typec_partner_type {
+	TYPEC_PARTNER_USB,
+	TYPEC_PARTNER_CHARGER,
+	TYPEC_PARTNER_ALTMODE,
+	TYPEC_PARTNER_ACCESSORY,
+};
+
+enum typec_plug_type {
+	USB_PLUG_NONE,
+	USB_PLUG_TYPE_A,
+	USB_PLUG_TYPE_B,
+	USB_PLUG_TYPE_C,
+	USB_PLUG_CAPTIVE,
+};
+
+enum typec_data_role {
+	TYPEC_DEVICE,
+	TYPEC_HOST,
+};
+
+enum typec_role {
+	TYPEC_SINK,
+	TYPEC_SOURCE,
+};
+
+enum typec_pwr_opmode {
+	TYPEC_PWR_MODE_USB,
+	TYPEC_PWR_MODE_1_5A,
+	TYPEC_PWR_MODE_3_0A,
+	TYPEC_PWR_MODE_PD,
+};
+
+enum typec_accessory {
+	TYPEC_ACCESSORY_NONE,
+	TYPEC_ACCESSORY_AUDIO,
+	TYPEC_ACCESSORY_DEBUG,
+	TYPEC_ACCESSORY_DAUDIO,
+};
+
+/*
+ * 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
+ * @active_attr: Device attribute to expose active of the mode
+ * @roles: Only for ports. DRP if the mode is awailable in both roles
+ * @roles_attr: Device attribute, only for ports, to expose the supported roles
+ *
+ * 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;
+	/* Only for ports */
+	enum typec_port_type	roles;
+
+	struct typec_altmode	*alt_mode;
+
+	int			index;
+	char			group_name[8];
+	struct attribute_group	group;
+	struct attribute	*attrs[5];
+	struct device_attribute vdo_attr;
+	struct device_attribute desc_attr;
+	struct device_attribute active_attr;
+	/* Only for ports */
+	struct device_attribute roles_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 *);
+
+void typec_altmode_update_active(struct typec_altmode *alt, int mode,
+				 bool active);
+
+int typec_register_altmodes(struct device *, struct typec_altmode *);
+void typec_unregister_altmodes(struct device *);
+
+/*
+ * 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
+ * @usb_pd: Electronically Marked Cable
+ * @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;
+	u32			vdo;
+	unsigned int		usb_pd:1;
+	unsigned int		active:1;
+	unsigned int		sop_pp_controller:1;
+
+	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
+ * @usb_pd: USB Power Delivery support
+ * @vdo: VDO returned by Discover Identity USB PD command
+ * @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;
+	unsigned int		usb_pd:1;
+	u32			vdo;
+	enum typec_accessory	accessory;
+	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
+ * @accessory: Supported Accessory Modes
+ * @num_accessory: Number of supported Accessory Modes
+ * @alt_modes: Alternate Modes the connector supports (null terminated)
+ * @try_role: Set a fixed data role for DRP port
+ * @dr_set: Set Data Role
+ * @pr_set: Set Power Role
+ * @vconn_set: Set VCONN Role
+ * @activate_mode: Enter/exit 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;
+	enum typec_accessory	*accessory;
+	unsigned int		num_accessory;
+	struct typec_altmode	*alt_modes;
+
+	int			(*try_role)(const struct typec_capability *,
+					    enum typec_role);
+
+	int			(*dr_set)(const struct typec_capability *,
+					  enum typec_data_role);
+	int			(*pr_set)(const struct typec_capability *,
+					  enum typec_role);
+	int			(*vconn_set)(const struct typec_capability *,
+					     enum typec_role);
+
+	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
+ * @data_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_data_role	data_role;
+	enum typec_role		pwr_role;
+	enum typec_role		vconn_role;
+	enum typec_pwr_opmode	pwr_opmode;
+};
+
+struct typec_port *typec_register_port(struct device *dev,
+				       const 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);
+
+/* Callbacks from driver */
+
+void typec_set_data_role(struct typec_port *, enum typec_data_role);
+void typec_set_pwr_role(struct typec_port *, enum typec_role);
+void typec_set_vconn_role(struct typec_port *, enum typec_role);
+void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
+
+#endif /* __LINUX_USB_TYPEC_H */
-- 
2.8.1

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
[ ... ]

> +
> +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the partner supports USB Power Delivery.
> +		- 1 if USB Power Delivery is supported
> +		- 0 when it's not
> +
> +
> +What:		/sys/class/typec/<port>-partner/id_header_vdo
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		If the partner supports USB Power Deliver, shows the VDO
> +		returned from Discover Identity USB Power Delivery command.
> +
> +		If the partner does not support USB Power Delivery, the
> +		attribute is hidden.
> +
On second thought, and after merging the code (and realizing that I don't get
the raw data from the Type-C Port Manager), I am not sure if a raw attribute
is that useful here. It also doesn't provide all information either.

Would it make sense to split it into multiple decoded attributes ?

- vendor-id
  [bit 0..15 of ID header VDO]
- product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
	passive/active for cable plugs)
  Might map into typec_partner_type, but I don't see a 1:1 match.
  [bit 27..29 of ID header VDO]
- alternate-mode-supported
  [bit 26 of ID header VDO]
- capabilities (ufp, dfp, drp, none (?))
  [bit 30/31 of ID header VDO]
- product-id
  [bit 16..31 of Product VDO]

Does this make any sense ?

Thanks,
Guenter

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > The purpose of USB Type-C connector class is to provide
> > unified interface for the user space to get the status and
> > basic information about USB Type-C connectors on a system,
> > control over data role swapping, and when the port supports
> > USB Power Delivery, also control over power role swapping
> > and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> [ ... ]
> 
> > +
> > +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> > +Date:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:
> > +		Shows if the partner supports USB Power Delivery.
> > +		- 1 if USB Power Delivery is supported
> > +		- 0 when it's not
> > +
> > +
> > +What:		/sys/class/typec/<port>-partner/id_header_vdo
> > +Date:		June 2016
> > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:
> > +		If the partner supports USB Power Deliver, shows the VDO
> > +		returned from Discover Identity USB Power Delivery command.
> > +
> > +		If the partner does not support USB Power Delivery, the
> > +		attribute is hidden.
> > +
> On second thought, and after merging the code (and realizing that I don't get
> the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> is that useful here. It also doesn't provide all information either.

Yeah, I don't think it's available with UCSI either..

> Would it make sense to split it into multiple decoded attributes ?
> 
> - vendor-id
>   [bit 0..15 of ID header VDO]
> - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
> 	passive/active for cable plugs)
>   Might map into typec_partner_type, but I don't see a 1:1 match.
>   [bit 27..29 of ID header VDO]
> - alternate-mode-supported
>   [bit 26 of ID header VDO]
> - capabilities (ufp, dfp, drp, none (?))
>   [bit 30/31 of ID header VDO]
> - product-id
>   [bit 16..31 of Product VDO]
> 
> Does this make any sense ?

I feel a bit uncomfortable exposing so many attribute like that which
will give details that we can only know when both ends support USB
PD...

Here's my proposal for this:
There has to be a special group for these devices, partners and
cables, a directory named "pd" or "power_deliver" (or something like
that), which exposes those. That group will not be responsibility of
this class, but instead the PD framework that you are working on
(right?).

So basically, in this class we will not expose those attributes, and
we'll also get rid of the "supports_usb_power_delivery" attribute (the
"pd" groups should only exist when USB PD is actually supported).

Initially that group needs to be assigned to the "groups" member of
struct device of the partners in the drivers before they are
registered. That member is meant for optional attribute groups, but we
can later think of something better, a way perhaps to bind that group
the device types "groups" member for partners, cables, etc. or
something similar in case using the "groups" member of struct device
is not ideal. But initially we can use that.

How would that sound to you?


Thanks,

-- 
heikki

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Fri, Jul 01, 2016 at 10:38:03AM +0300, Heikki Krogerus wrote:
> On Thu, Jun 30, 2016 at 10:10:25AM -0700, Guenter Roeck wrote:
> > On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> > > The purpose of USB Type-C connector class is to provide
> > > unified interface for the user space to get the status and
> > > basic information about USB Type-C connectors on a system,
> > > control over data role swapping, and when the port supports
> > > USB Power Delivery, also control over power role swapping
> > > and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > [ ... ]
> > 
> > > +
> > > +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> > > +Date:		June 2016
> > > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +		Shows if the partner supports USB Power Delivery.
> > > +		- 1 if USB Power Delivery is supported
> > > +		- 0 when it's not
> > > +
> > > +
> > > +What:		/sys/class/typec/<port>-partner/id_header_vdo
> > > +Date:		June 2016
> > > +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > +Description:
> > > +		If the partner supports USB Power Deliver, shows the VDO
> > > +		returned from Discover Identity USB Power Delivery command.
> > > +
> > > +		If the partner does not support USB Power Delivery, the
> > > +		attribute is hidden.
> > > +
> > On second thought, and after merging the code (and realizing that I don't get
> > the raw data from the Type-C Port Manager), I am not sure if a raw attribute
> > is that useful here. It also doesn't provide all information either.
> 
> Yeah, I don't think it's available with UCSI either..
> 
> > Would it make sense to split it into multiple decoded attributes ?
> > 
> > - vendor-id
> >   [bit 0..15 of ID header VDO]
> > - product-type (undefined, hub, peripheral, alternate mode adapter for ufp;
> > 	passive/active for cable plugs)
> >   Might map into typec_partner_type, but I don't see a 1:1 match.
> >   [bit 27..29 of ID header VDO]
> > - alternate-mode-supported
> >   [bit 26 of ID header VDO]
> > - capabilities (ufp, dfp, drp, none (?))
> >   [bit 30/31 of ID header VDO]
> > - product-id
> >   [bit 16..31 of Product VDO]
> > 
> > Does this make any sense ?
> 
> I feel a bit uncomfortable exposing so many attribute like that which
> will give details that we can only know when both ends support USB
> PD...
> 
> Here's my proposal for this:
> There has to be a special group for these devices, partners and
> cables, a directory named "pd" or "power_deliver" (or something like
> that), which exposes those. That group will not be responsibility of
> this class, but instead the PD framework that you are working on
> (right?).
> 
> So basically, in this class we will not expose those attributes, and
> we'll also get rid of the "supports_usb_power_delivery" attribute (the
> "pd" groups should only exist when USB PD is actually supported).
> 
> Initially that group needs to be assigned to the "groups" member of
> struct device of the partners in the drivers before they are
> registered. That member is meant for optional attribute groups, but we
> can later think of something better, a way perhaps to bind that group
> the device types "groups" member for partners, cables, etc. or
> something similar in case using the "groups" member of struct device
> is not ideal. But initially we can use that.
> 
> How would that sound to you?
> 
Quite frankly I don't know. At first glance it sounds like a bad idea - it means
that PD specific atttributes would not be standardized, which would reduce the
valus of the class code substantially. I'll have to think about this.

At the same time I'll have to find a somewhat working solution for the locking
issues. I'll have to focus on that for the time being, because it affects
the core architecture of our code. So I don't really have the time to look
into attribute support right now.

Guenter

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Wed, Jun 29, 2016 at 04:38:37PM +0300, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  236 +++++
>  Documentation/usb/typec.txt                 |  103 +++
>  MAINTAINERS                                 |    9 +
>  drivers/usb/Kconfig                         |    2 +
>  drivers/usb/Makefile                        |    2 +
>  drivers/usb/typec/Kconfig                   |    7 +
>  drivers/usb/typec/Makefile                  |    1 +
>  drivers/usb/typec/typec.c                   | 1277 +++++++++++++++++++++++++++
>  include/linux/usb/typec.h                   |  260 ++++++
>  9 files changed, 1897 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>  create mode 100644 Documentation/usb/typec.txt
>  create mode 100644 drivers/usb/typec/Kconfig
>  create mode 100644 drivers/usb/typec/Makefile
>  create mode 100644 drivers/usb/typec/typec.c
>  create mode 100644 include/linux/usb/typec.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 0000000..7cd40e5
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,236 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What:		/sys/class/typec/<port>/current_data_role
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		The current USB data role the port is operating in. This
> +		attribute can be used for requesting data role swapping on the
> +		port.
> +
> +		Valid values:
> +		- host
> +		- device
> +
> +What:		/sys/class/typec/<port>/current_power_role
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		The current power role of the port. This attribute can be used
> +		to request power role swap on the port when the port supports
> +		USB Power Delivery.
> +
> +		Valid values:
> +		- source
> +		- sink
> +
> +What:		/sys/class/typec/<port>/current_vconn_role
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows the current VCONN role of the port. This attribute can be
> +		used to request VCONN role swap on the port when the port
> +		supports USB Power Delivery.
> +
> +		Valid values are:
> +		- source
> +		- sink
> +
> +What:		/sys/class/typec/<port>/power_operation_mode
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows the current power operational mode the port is in.
> +
> +		Valid values:
> +		- USB - Normal power levels defined in USB specifications
> +		- BC1.2 - Power levels defined in Battery Charging Specification
> +			  v1.2
> +		- USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
> +				    specification.
> +		- USB Type-C 3.0A - Higher 3A current defined in USB Type-C
> +				    specification.
> +                - USB Power Delivery - The voltages and currents defined in USB
> +				       Power Delivery specification
> +
> +What:		/sys/class/typec/<port>/preferred_role
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		The user space can notify the driver about the preferred role.
> +		It should be handled as enabling of Try.SRC or Try.SNK, as
> +		defined in USB Type-C specification, in the port drivers. By
> +		default there is no preferred role.
> +
> +		Valid values:
> +		- host
> +		- device
> +		- For example "none" to remove preference (anything else except
> +		  "host" or "device")
> +
> +What:		/sys/class/typec/<port>/supported_accessory_modes
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Lists the Accessory Modes, defined in the USB Type-C
> +		specification, the port supports.
> +
> +What:		/sys/class/typec/<port>/supported_data_roles
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Lists the USB data roles the port is capable of supporting.
> +
> +		Valid values:
> +		- device
> +		- host
> +		- device, host (DRD as defined in USB Type-C specification v1.2)
> +
> +What:		/sys/class/typec/<port>/supported_power_roles
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Lists the power roles the port is capable of supporting.
> +
> +		Valid values:
> +		- source
> +		- sink
> +
> +What:		/sys/class/typec/<port>/supports_usb_power_delivery
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the port supports USB Power Delivery.
> +		- 1 if USB Power Delivery is supported
> +		- 0 when it's not
> +
> +
> +USB Type-C partner devices (eg. /sys/class/typec/usbc0-partner/)
> +
> +What:		/sys/class/typec/<port>-partner/accessory
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		The attribute is visible only when the partner's type is
> +		"Accessory". The type can be read from its own attribute.
> +
> +		Shows the name of the Accessory Mode. The Accessory Modes are
> +		defined in USB Type-C Specification.
> +
> +What:		/sys/class/typec/<port>-partner/type
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows the type of the partner. Can be one of the following:
> +		- USB - When the partner is normal USB host/peripheral.
> +		- Charger - When the partner has been identified as dedicated
> +			    charger.
> +		- Alternate Mode - When the partner supports Alternate Modes.
> +		- Accessory - When the partner is one of the accessories with
> +			      specific Accessory Mode defined in USB Type-C
> +			      specification.
> +
> +What:		/sys/class/typec/<port>-partner/supports_usb_power_delivery
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the partner supports USB Power Delivery.
> +		- 1 if USB Power Delivery is supported
> +		- 0 when it's not
> +
> +
> +What:		/sys/class/typec/<port>-partner/id_header_vdo
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		If the partner supports USB Power Deliver, shows the VDO
> +		returned from Discover Identity USB Power Delivery command.
> +
> +		If the partner does not support USB Power Delivery, the
> +		attribute is hidden.
> +
> +
> +USB Type-C cable devices (eg. /sys/class/typec/usbc0-cable/)
> +
> +Note: Electronically Marked Cables will have a device also for one cable plug
> +(eg. /sys/class/typec/usbc0-plug0). If the cable is active and has also SOP
> +Double Prime controller (USB Power Deliver specification ch. 2.4) it will have
> +second device also for the other plug. Both plugs may have their alternate modes
> +as described in USB Type-C and USB Power Delivery specifications.
> +
> +What:		/sys/class/typec/<port>-cable/active
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the cable is active or passive.
> +
> +		Valid values:
> +		- 0 when the cable is passive
> +		- 1 when the cable is active
> +
> +What:		/sys/class/typec/<port>-cable/plug_type
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows type of the plug on the cable:
> +		- Type-A - Standard A
> +		- Type-B - Standard B
> +		- Type-C - USB Type-C
> +		- Captive - Non-standard
> +
> +What:		/sys/class/typec/<port>-cable/supports_usb_power_delivery
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the cable supports USB Power Delivery communication.
> +		- 1 if USB Power Delivery is supported
> +		- 0 when it's not
> +
> +What:		/sys/class/typec/<port>-cable/id_header_vdo
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		If the cable supports USB Power Deliver, shows the VDO
> +		returned from Discover Identity USB Power Delivery command.
> +
> +		If the cable does not support USB Power Delivery, the
> +		attribute is hidden.
> +
> +
> +Alternate Mode devices (For example,
> +/sys/class/typec/usbc0-partner/usbc0-partner.svid:xxxx/). The ports, partners
> +and cable plugs can have alternate modes.
> +
> +What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/active
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows if the mode is active or not. The attribute can be used
> +		for entering/exiting the mode with partners and cable plugs, and
> +		with the port alternate modes it can be used for disabling
> +		support for specific alternate modes.
> +
> +What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/description
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows description of the mode. The description is optional for
> +		the drivers, just like with the Billboard Devices.
> +
> +What:		/sys/class/typec/<dev>/<dev>.svid:<svid>/<mode>/vdo
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows the VDO in hexadecimal returned from the Discover Modes
> +		command.
> +
> +What:		/sys/class/typec/<port>/<port>.svid:<svid>/<mode>/supported_roles
> +Date:		June 2016
> +Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +Description:
> +		Shows the roles, source or sink, the mode is supported with.
> +
> +		This attribute is available for the devices describing the
> +		alternate modes a port supports, and it will not be exposed with
> +		the devices presenting the alternate modes the partners or cable
> +		plugs support.
> diff --git a/Documentation/usb/typec.txt b/Documentation/usb/typec.txt
> new file mode 100644
> index 0000000..dce5f07
> --- /dev/null
> +++ b/Documentation/usb/typec.txt
> @@ -0,0 +1,103 @@
> +USB Type-C connector class
> +==========================
> +
> +Introduction
> +------------
> +The typec class is meant for describing the USB Type-C ports in a system to the
> +user space in unified fashion. The class is designed to provide nothing else
> +except the user space interface implementation in hope that it can be utilized
> +on as many platforms as possible.
> +
> +The platforms are expected to register every USB Type-C port they have with the
> +class. In a normal case the registration will be done by a USB Type-C or PD PHY
> +driver, but it may be a driver for firmware interface such as UCSI, driver for
> +USB PD controller or even driver for Thunderbolt3 controller. This document
> +considers the component registering the USB Type-C ports with the class as "port
> +driver".
> +
> +On top of showing the capabilities, the class also offer the user space control
> +over the roles and alternate modes they support when the port driver is capable
> +of supporting those features.
> +
> +The class provides an API for the port drivers described in this document. The
> +attributes are described in Documentation/ABI/testing/sysfs-class-typec.
> +
> +
> +Interface
> +---------
> +Every port will be presented as its own device under /sys/class/typec/. The
> +first port will be named "usbc0", the second "usbc1" and so on.
> +
> +When connected, the partner will be presented also as its own device under
> +/sys/class/typec/. The parent of the partner device will always be the port. The
> +partner attached to port "usbc0" will be named "usbc0-partner". Full patch to
> +the device would be /sys/class/typec/usb0/usb0-partner/.
> +
> +The cable and the two plugs on it may also be optionally presented as their own
> +devices under /sys/class/typec/. The cable attached to the port "usbc0" port
> +will be named usbc0-cable and the plug on the SOP Prime end (see USB Power
> +Delivery Specification ch. 2.4) will be named "usbc-plug0" and on the SOP Double
> +Prime end "usbc0-plug1". The parent of a cable will always be the port, and the
> +parent of the cable plugs will always be the cable.
> +
> +If the port, partner or cable plug support Alternate Modes, every Alternate Mode
> +SVID will have their own device describing them. The Alternate Modes will not be
> +attached to the typec class. For the port's "usbc0" partner, the Alternate Modes
> +would have devices presented under /sys/class/typec/usbc0-partner/. Every mode
> +that is supported will have its own group under the Alternate Mode device named
> +"mode<id>". For example /sys/class/typec/usbc0/usbc0.svid:xxxx/mode0/. The
> +requests for entering/exiting the modes happens with the "active" attribute in
> +that group.
> +
> +
> +API
> +---
> +
> +* Registering the ports
> +
> +The port drivers will describe every Type-C port they control with struct
> +typec_capability data structure, and register them with the following API:
> +
> +struct typec_port *typec_register_port(struct device *dev,
> +				       const struct typec_capability *cap);
> +
> +The class will provide handle to struct typec_port on success and ERR_PTR on
> +failure. The un-registration of the port happens with the following API:
> +
> +void typec_unregister_port(struct typec_port *port);
> +
> +
> +* Notifications
> +
> +When connection happens on a port, the port driver fills struct typec_connection
> +which is passed to the class. The class provides the following API for reporting
> +connection/disconnection:
> +
> +int typec_connect(struct typec_port *port, struct typec_connection *);
> +void typec_disconnect(struct typec_port *);
> +
> +When the partner end has executed a role change, the port driver uses the
> +following APIs to report it to the class:
> +
> +void typec_set_data_role(struct typec_port *, enum typec_data_role);
> +void typec_set_pwr_role(struct typec_port *, enum typec_role);
> +void typec_set_vconn_role(struct typec_port *, enum typec_role);
> +void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
> +
> +
> +* Alternate Modes
> +
> +After connection, the port drivers register the alternate modes the partner
> +and/or cable plugs support. And before reporting disconnection, the port driver
> +_must_ unregister all the alternate modes registered for the partner and cable
> +plugs. The API takes the struct device of the partner or the cable plug as
> +parameter:
> +
> +int typec_register_altmodes(struct device *, struct typec_altmode *);
> +void typec_unregister_altmodes(struct device *);
> +
> +When the partner end enters or exits the modes, the port driver needs to notify
> +the class with the following API:
> +
> +void typec_altmode_update_active(struct typec_altmode *alt, int mode,
> +				 bool active);
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 952fd2a..8fe0977 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11983,6 +11983,15 @@ F:	drivers/usb/
>  F:	include/linux/usb.h
>  F:	include/linux/usb/
>  
> +USB TYPEC SUBSYSTEM
> +M:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> +L:	linux-usb@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-class-typec
> +F:	Documentation/usb/typec.txt
> +F:	drivers/usb/typec/
> +F:	include/linux/usb/typec.h
> +
>  USB UHCI DRIVER
>  M:	Alan Stern <stern@rowland.harvard.edu>
>  L:	linux-usb@vger.kernel.org
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 8689dcb..f42a3d3 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/typec/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..51e381e 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)		+= typec/
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> new file mode 100644
> index 0000000..b229fb9
> --- /dev/null
> +++ b/drivers/usb/typec/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB PD and Type-C drivers"
> +
> +config TYPEC
> +	tristate
> +
> +endmenu
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> new file mode 100644
> index 0000000..1012a8b
> --- /dev/null
> +++ b/drivers/usb/typec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TYPEC)		+= typec.o
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> new file mode 100644
> index 0000000..bbfd6e5
> --- /dev/null
> +++ b/drivers/usb/typec/typec.c
> @@ -0,0 +1,1277 @@
> +/*
> + * 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; /* device lock */
> +
> +	int			prefer_role;
> +
> +	enum typec_data_role	data_role;
> +	enum typec_role		pwr_role;
> +	enum typec_role		vconn_role;
> +	enum typec_pwr_opmode	pwr_opmode;
> +
> +	struct typec_partner	*partner;
> +	struct typec_cable	*cable;
> +
> +	unsigned int		connected:1;
> +
> +	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 = "typec",
> +};
> +
> +static const char * const typec_accessory_modes[] = {
> +	[TYPEC_ACCESSORY_NONE]		= "none",
> +	[TYPEC_ACCESSORY_AUDIO]		= "Audio",
> +	[TYPEC_ACCESSORY_DEBUG]		= "Debug",
> +	[TYPEC_ACCESSORY_DAUDIO]	= "Digital Audio",
> +};
> +
> +/* ------------------------------------------------------------------------- */
> +/* 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_ACCESSORY]	= "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 ssize_t
> +partner_accessory_mode_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct typec_partner *partner = container_of(dev, struct typec_partner,
> +						     dev);
> +
> +	return sprintf(buf, "%s\n", typec_accessory_modes[partner->accessory]);
> +}
> +
> +static struct device_attribute dev_attr_partner_accessory = {
> +	.attr = {
> +		.name = "accessory",
> +		.mode = S_IRUGO,
> +	},
> +	.show = partner_accessory_mode_show,
> +};
> +
> +static ssize_t partner_usb_pd_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct typec_partner *partner = container_of(dev, struct typec_partner,
> +						     dev);
> +
> +	return sprintf(buf, "%d\n", partner->usb_pd);
> +}
> +
> +static struct device_attribute dev_attr_partner_usb_pd = {
> +	.attr = {
> +		.name = "supports_usb_power_delivery",
> +		.mode = S_IRUGO,
> +	},
> +	.show = partner_usb_pd_show,
> +};
> +
> +static ssize_t partner_vdo_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct typec_partner *partner = container_of(dev, struct typec_partner,
> +						     dev);
> +
> +	return sprintf(buf, "0x%08x\n", partner->vdo);
> +}
> +
> +static struct device_attribute dev_attr_partner_vdo = {
> +	.attr = {
> +		.name = "id_header_vdo",
> +		.mode = S_IRUGO,
> +	},
> +	.show = partner_vdo_show,
> +};
> +
> +static struct attribute *typec_partner_attrs[] = {
> +	&dev_attr_partner_accessory.attr,
> +	&dev_attr_partner_type.attr,
> +	&dev_attr_partner_usb_pd.attr,
> +	&dev_attr_partner_vdo.attr,
> +	NULL
> +};
> +
> +static umode_t partner_is_visible(struct kobject *kobj, struct attribute *attr,
> +				  int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct typec_partner *partner = container_of(dev, struct typec_partner,
> +						     dev);
> +
> +	if (attr == &dev_attr_partner_accessory.attr &&
> +	    partner->type != TYPEC_PARTNER_ACCESSORY)
> +		return 0;
> +
> +	if (attr == &dev_attr_partner_vdo.attr && !partner->usb_pd)
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
> +static struct attribute_group typec_partner_group = {
> +	.attrs = typec_partner_attrs,
> +	.is_visible = partner_is_visible,
> +};
> +
> +static const struct attribute_group *typec_partner_groups[] = {
> +	&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;
> +	int ret;
> +
> +	dev->class = &typec_class;
> +	dev->parent = &port->dev;
> +	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;
> +	}
> +
> +	port->partner = partner;
> +	return 0;
> +}
> +
> +static void typec_remove_partner(struct typec_port *port)
> +{
> +	WARN_ON(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[8];
> +	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;
> +	}
> +
> +	return 0;
> +}
> +
> +static void typec_remove_plug(struct typec_plug *plug)
> +{
> +	WARN_ON(plug->alt_modes);
> +	device_unregister(&plug->dev);
> +}
> +
> +/* Type-C Cables */
> +
> +static ssize_t
> +cable_active_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_cable_active = {
> +	.attr = {
> +		.name = "active",
> +		.mode = S_IRUGO,
> +	},
> +	.show = cable_active_show,
> +};
> +
> +static ssize_t cable_usb_pd_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->usb_pd);
> +}
> +
> +static struct device_attribute dev_attr_cable_usb_pd = {
> +	.attr = {
> +		.name = "supports_usb_power_delivery",
> +		.mode = S_IRUGO,
> +	},
> +	.show = cable_usb_pd_show,
> +};
> +
> +static ssize_t cable_vdo_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
> +
> +	return sprintf(buf, "0x%08x\n", cable->vdo);
> +}
> +
> +static struct device_attribute dev_attr_cable_vdo = {
> +	.attr = {
> +		.name = "id_header_vdo",
> +		.mode = S_IRUGO,
> +	},
> +	.show = cable_vdo_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_cable_active.attr,
> +	&dev_attr_cable_usb_pd.attr,
> +	&dev_attr_cable_vdo.attr,
> +	&dev_attr_plug_type.attr,
> +	NULL
> +};
> +
> +static umode_t cable_is_visible(struct kobject *kobj, struct attribute *attr,
> +				int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
> +
> +	if (attr == &dev_attr_cable_vdo.attr && !cable->usb_pd)
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
> +static struct attribute_group typec_cable_group = {
> +	.attrs = typec_cable_attrs,
> +	.is_visible = cable_is_visible,
> +};
> +
> +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;
> +	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->usb_pd)
> +		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->active || !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);
> +}
> +
> +/* ------------------------------------------------------------------------- */
> +/* API for the port drivers */
> +
> +static void typec_init_roles(struct typec_port *port)
> +{
> +	if (port->prefer_role < 0)
> +		return;
> +
> +	if (port->prefer_role == TYPEC_SOURCE) {
> +		port->data_role = TYPEC_HOST;
> +		port->pwr_role = TYPEC_SOURCE;
> +		port->vconn_role = TYPEC_SOURCE;
> +	} else {
> +		/* Device mode as default also by default with DRP ports */
> +		port->data_role = TYPEC_DEVICE;
> +		port->pwr_role = TYPEC_SINK;
> +		port->vconn_role = TYPEC_SINK;
> +	}
> +}
> +
> +int typec_connect(struct typec_port *port, struct typec_connection *con)
> +{
> +	int ret;
> +
> +	if (!con->partner && !con->cable)
> +		return -EINVAL;
> +
> +	port->connected = 1;
> +	port->data_role = con->data_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;
> +
> +	typec_init_roles(port);
> +
> +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> +}
> +EXPORT_SYMBOL_GPL(typec_disconnect);
> +
> +/* --------------------------------------- */
> +/* Driver callbacks to report role updates */
> +
> +void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
> +{
> +	mutex_lock(&port->lock);
> +	port->data_role = role;
> +	sysfs_notify(&port->dev.kobj, NULL, "current_data_role");
> +	mutex_unlock(&port->lock);
> +}
> +EXPORT_SYMBOL(typec_set_data_role);
> +
> +void typec_set_pwr_role(struct typec_port *port, enum typec_role role)
> +{
> +	mutex_lock(&port->lock);
> +	port->pwr_role = role;
> +	sysfs_notify(&port->dev.kobj, NULL, "current_power_role");
> +	mutex_unlock(&port->lock);
> +}
> +EXPORT_SYMBOL(typec_set_pwr_role);
> +
> +void typec_set_vconn_role(struct typec_port *port, enum typec_role role)
> +{
> +	mutex_lock(&port->lock);
> +	port->vconn_role = role;
> +	sysfs_notify(&port->dev.kobj, NULL, "current_vconn_role");
> +	mutex_unlock(&port->lock);
> +}
> +EXPORT_SYMBOL(typec_set_vconn_role);
> +
> +void typec_set_pwr_opmode(struct typec_port *port,
> +			  enum typec_pwr_opmode opmode)
> +{
> +	mutex_lock(&port->lock);
> +	port->pwr_opmode = opmode;
> +	sysfs_notify(&port->dev.kobj, NULL, "power_operation_mode");
> +	mutex_unlock(&port->lock);
> +}
> +EXPORT_SYMBOL(typec_set_pwr_opmode);
> +
> +/* -------------------------------- */
> +/* Alternate Modes */
> +
> +/*
> + * typec_altmode_update_active - Notify about Enter/Exit mode
> + * @alt: Handle to the Alternate Mode
> + * @mode: Mode id
> + * @active: True when the mode has been enterred
> + */
> +void typec_altmode_update_active(struct typec_altmode *alt, int mode,
> +				 bool active)
> +{
> +	struct typec_port *port = typec_altmode2port(alt);
> +	struct typec_mode *m = alt->modes + mode;
> +	char dir[6];
> +
> +	mutex_lock(&port->lock);
> +	m->active = active;
> +	sprintf(dir, "mode%d", mode);
> +	sysfs_notify(&alt->dev.kobj, dir, "active");
> +	mutex_unlock(&port->lock);
> +}
> +EXPORT_SYMBOL(typec_altmode_update_active);
> +
> +static struct device_type typec_port_dev_type;
> +
> +/*
> + * 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.
> + */
> +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);
> +	if (alt->dev.parent->type == &typec_port_dev_type)
> +		return to_typec_port(alt->dev.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);
> +	struct typec_port *port = typec_altmode2port(mode->alt_mode);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "0x%08x\n", mode->vdo);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +
> +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);
> +	struct typec_port *port = typec_altmode2port(mode->alt_mode);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +
> +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);
> +	struct typec_port *port = typec_altmode2port(mode->alt_mode);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%d\n", mode->active);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t
> +typec_altmode_active_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t size)
> +{
> +	struct typec_mode *mode = container_of(attr, struct typec_mode,
> +					       active_attr);
> +	struct typec_port *port = typec_altmode2port(mode->alt_mode);
> +	bool activate;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &activate);
> +	if (ret)
> +		return ret;
> +
> +	if (activate > 1)
> +		return -EINVAL;
> +
> +	mutex_lock(&port->lock);
> +	ret = port->cap->activate_mode(mode->alt_mode, mode->index, activate);
> +	if (!ret) {
> +		mode->active = activate;
> +		ret = size;
> +	}
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +
> +static ssize_t
> +typec_altmode_roles_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct typec_mode *mode = container_of(attr, struct typec_mode,
> +					       roles_attr);
> +	struct typec_port *port = typec_altmode2port(mode->alt_mode);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	switch (mode->roles) {
> +	case TYPEC_PORT_DFP:
> +		ret =  sprintf(buf, "source\n");
> +		break;
> +	case TYPEC_PORT_UFP:
> +		ret = sprintf(buf, "sink\n");
> +		break;
> +	case TYPEC_PORT_DRP:
> +	default:
> +		ret = sprintf(buf, "source, sink\n");
> +		break;
> +	}
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +
> +static void typec_init_modes(struct typec_altmode *alt, int is_port)
> +{
> +	struct typec_mode *mode = alt->modes;
> +	int i;
> +
> +	for (i = 0; i < alt->n_modes; i++, mode++) {
> +		mode->alt_mode = alt;
> +		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_IWUSR | S_IRUGO;
> +		mode->active_attr.show = typec_altmode_active_show;
> +		mode->active_attr.store = typec_altmode_active_store;
> +
> +		mode->attrs[0] = &mode->vdo_attr.attr;
> +		mode->attrs[1] = &mode->desc_attr.attr;
> +		mode->attrs[2] = &mode->active_attr.attr;
> +
> +		/* With ports, list the roles that the mode is supported with */
> +		if (is_port) {
> +			sysfs_attr_init(&mode->roles_attr.attr);
> +			mode->roles_attr.attr.name = "supported_roles";
> +			mode->roles_attr.attr.mode = S_IRUGO;
> +			mode->roles_attr.show = typec_altmode_roles_show;
> +
> +			mode->attrs[3] = &mode->roles_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, int is_port)
> +{
> +	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, is_port);
> +
> +	dev->groups = alt->mode_groups;
> +	dev->release = typec_altmode_release;
> +	dev->parent = parent;
> +	/* TODO: if (!is_port) dev->bus = &typec_altmode_bus; */
> +
> +	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;
> +}
> +
> +static int __typec_register_altmodes(struct device *parent,
> +				     struct typec_altmode *alt_modes,
> +				     int is_port)
> +{
> +	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, is_port);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	return 0;
> +err:
> +	for (alt = alt_modes + index; index; alt--, index--)
> +		device_unregister(&alt->dev);
> +
> +	return ret;
> +}
> +
> +int typec_register_altmodes(struct device *dev, struct typec_altmode *alt_modes)
> +{
> +	if (dev->type == &typec_partner_dev_type) {
> +		struct typec_partner *p = container_of(dev,
> +						       struct typec_partner,
> +						       dev);
> +		p->alt_modes = alt_modes;
> +	} else if (dev->type == &typec_plug_dev_type) {
> +		struct typec_plug *p = container_of(dev, struct typec_plug,
> +						    dev);
> +		p->alt_modes = alt_modes;
> +	} else {
> +		return -ENODEV;
> +	}
> +
> +	return __typec_register_altmodes(dev, alt_modes, false);
> +}
> +EXPORT_SYMBOL_GPL(typec_register_altmodes);
> +
> +void typec_unregister_altmodes(struct device *dev)
> +{
> +	struct typec_altmode *alt_modes = NULL;
> +	struct typec_altmode *alt;
> +
> +	if (dev->type == &typec_partner_dev_type) {
> +		struct typec_partner *p = container_of(dev,
> +						       struct typec_partner,
> +						       dev);
> +		alt_modes = p->alt_modes;
> +		p->alt_modes = NULL;
> +	} else if (dev->type == &typec_plug_dev_type) {
> +		struct typec_plug *p = container_of(dev, struct typec_plug,
> +						    dev);
> +		alt_modes = p->alt_modes;
> +		p->alt_modes = NULL;
> +	}
> +
> +	if (!alt_modes)
> +		return;
> +
> +	for (alt = alt_modes; alt->svid; alt++)
> +		device_unregister(&alt->dev);
> +}
> +EXPORT_SYMBOL_GPL(typec_unregister_altmodes);
> +
> +/* ------------------------------------------------------------------------- */
> +
> +static const char * const typec_roles[] = {
> +	[TYPEC_SINK]	= "sink",
> +	[TYPEC_SOURCE]	= "source",
> +};
> +
> +static const char * const typec_data_roles[] = {
> +	[TYPEC_DEVICE]	= "device",
> +	[TYPEC_HOST]	= "host",
> +};
> +
> +static ssize_t
> +preferred_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_role role;
> +	int ret;
> +
> +	mutex_lock(&port->lock);
> +
> +	if (port->cap->type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (!port->cap->try_role) {
> +		dev_dbg(dev, "Setting preferred role not supported\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);

With this, 'echo "sink" > preferred_role' no longer works because
match_string() tries to match the entire string, including the newline
generated by the echo command. One has to use "echo -n" instead.
Is this on purpose ? It is quite unusual.

> +	if (ret < 0) {
> +		port->prefer_role = -1;
> +		ret = size;
> +		goto out;
> +	}
> +
> +	role = ret;
> +
> +	ret = port->cap->try_role(port->cap, role);
> +	if (ret)
> +		goto out;
> +
> +	port->prefer_role = role;
> +	ret = size;
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +
> +static ssize_t
> +preferred_role_show(struct device *dev, struct device_attribute *attr,
> +		    char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	ssize_t ret = 0;
> +
> +	mutex_lock(&port->lock);
> +
> +	if (port->prefer_role < 0)
> +		goto out;
> +
> +	ret = sprintf(buf, "%s\n", typec_roles[port->prefer_role]);
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(preferred_role);
> +
> +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);
> +	int ret = size;
> +
> +	mutex_lock(&port->lock);
> +
With this lock, the code in the driver can no longer call any state updates
during the call to dr_set(), because those (eg typec_set_data_role()) set
the lock as well. This means that the dr_set call and all other similar calls
now have to be asynchronous. As a result, those calls can not return an error
if the role change request fails. Is this on purpose ? I see it as a major
drawback, not only because errors can no longer be returned, but also because
I very much preferred the call to be synchronous.

> +	if (port->cap->type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (!port->cap->dr_set) {
> +		dev_dbg(dev, "data role swapping not supported\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (!port->connected)
> +		goto out;
> +
> +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = port->cap->dr_set(port->cap, ret);
> +	if (ret)
> +		goto out;
> +
> +	ret = size;
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +
> +static ssize_t
> +current_data_role_show(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%s\n", typec_data_roles[port->data_role]);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(current_data_role);
> +
> +static ssize_t supported_data_roles_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	size_t ret;
> +
> +	mutex_lock(&port->lock);
> +
> +	if (port->cap->type == TYPEC_PORT_DRP) {
> +		ret = sprintf(buf, "host, device\n");
> +		goto out;
> +	}
> +
> +	ret = sprintf(buf, "%s\n", typec_data_roles[port->data_role]);
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(supported_data_roles);
> +
> +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);
> +	int ret = size;
> +
> +	mutex_lock(&port->lock);
> +
> +	if (!port->cap->usb_pd) {
> +		dev_dbg(dev, "power role swap only supported with USB PD\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (!port->cap->pr_set) {
> +		dev_dbg(dev, "power role swapping not supported\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> +		dev_dbg(dev, "partner unable to swap power role\n");
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	if (!port->connected)
> +		goto out;
> +
> +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = port->cap->pr_set(port->cap, ret);
> +	if (ret)
> +		goto out;
> +
> +	ret = size;
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +
> +static ssize_t current_power_role_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%s\n", typec_roles[port->pwr_role]);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +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);
> +	size_t ret;
> +
> +	mutex_lock(&port->lock);
> +
> +	if (port->cap->usb_pd || port->cap->type == TYPEC_PORT_DRP) {
> +		ret = sprintf(buf, "source, sink\n");
> +		goto out;
> +	}
> +
> +	ret = sprintf(buf, "%s\n", typec_roles[port->pwr_role]);
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(supported_power_roles);
> +
> +static const char * const typec_pwr_opmodes[] = {
> +	[TYPEC_PWR_MODE_USB]	= "USB",
> +	[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);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%s\n", typec_pwr_opmodes[port->pwr_opmode]);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(power_operation_mode);
> +
> +static ssize_t current_vconn_role_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	int ret;
> +
> +	mutex_lock(&port->lock);
> +
> +	if (!port->cap->usb_pd) {
> +		dev_dbg(dev, "vconn swap only supported with USB PD\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	if (!port->cap->vconn_set) {
> +		dev_dbg(dev, "vconn swapping not supported\n");
> +		ret = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = port->cap->vconn_set(port->cap, ret);
> +	if (ret)
> +		goto out;
> +
> +	ret = size;
> +out:
> +	mutex_unlock(&port->lock);
> +	return ret;
> +}
> +
> +static ssize_t current_vconn_role_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%s\n", typec_roles[port->vconn_role]);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RW(current_vconn_role);
> +
> +static ssize_t supported_accessory_modes_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	enum typec_accessory *accessory;
> +	ssize_t ret = 0;
> +	int i;
> +
> +	mutex_lock(&port->lock);
> +	if (port->cap->accessory)
> +		for (accessory = port->cap->accessory, i = 0;
> +		     i < port->cap->num_accessory; accessory++, i++)
> +			ret += sprintf(buf, "%s\n",
> +				       typec_accessory_modes[*accessory]);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(supported_accessory_modes);
> +
> +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);
> +	ssize_t ret;
> +
> +	mutex_lock(&port->lock);
> +	ret = sprintf(buf, "%d\n", port->cap->usb_pd);
> +	mutex_unlock(&port->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(supports_usb_power_delivery);
> +
> +static struct attribute *typec_attrs[] = {
> +	&dev_attr_current_power_role.attr,
> +	&dev_attr_current_vconn_role.attr,
> +	&dev_attr_current_data_role.attr,
> +	&dev_attr_power_operation_mode.attr,
> +	&dev_attr_preferred_role.attr,
> +	&dev_attr_supported_accessory_modes.attr,
> +	&dev_attr_supported_data_roles.attr,
> +	&dev_attr_supported_power_roles.attr,
> +	&dev_attr_supports_usb_power_delivery.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group typec_group = {
> +	.attrs = typec_attrs,
> +};
> +
> +static const struct attribute_group *typec_groups[] = {
> +	&typec_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,
> +				       const 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);
> +	}
> +
> +	/* FIXME: a better approach for this */
> +	port->prefer_role = -1;
> +
> +	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);
> +
> +	typec_init_roles(port);
> +
> +	ret = device_register(&port->dev);
> +	if (ret)
> +		goto reg_err;
> +
> +	ret = __typec_register_altmodes(&port->dev, cap->alt_modes, true);
> +	if (ret)
> +		goto alt_err;
> +
> +	return port;
> +alt_err:
> +	device_unregister(&port->dev);
> +reg_err:
> +	ida_simple_remove(&typec_index_ida, id);
> +	put_device(&port->dev);
> +	kfree(port);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(typec_register_port);
> +
> +void typec_unregister_port(struct typec_port *port)
> +{
> +	struct typec_altmode *alt;
> +
> +	WARN_ON(port->connected);
> +
> +	if (port->cap->alt_modes)
> +		for (alt = port->cap->alt_modes; alt->svid; alt++)
> +			device_unregister(&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..eda6747
> --- /dev/null
> +++ b/include/linux/usb/typec.h
> @@ -0,0 +1,260 @@
> +
> +#ifndef __LINUX_USB_TYPEC_H
> +#define __LINUX_USB_TYPEC_H
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +
> +struct typec_port;
> +
> +enum typec_port_type {
> +	TYPEC_PORT_DFP,
> +	TYPEC_PORT_UFP,
> +	TYPEC_PORT_DRP,
> +};
> +
> +enum typec_partner_type {
> +	TYPEC_PARTNER_USB,
> +	TYPEC_PARTNER_CHARGER,
> +	TYPEC_PARTNER_ALTMODE,
> +	TYPEC_PARTNER_ACCESSORY,
> +};
> +
> +enum typec_plug_type {
> +	USB_PLUG_NONE,
> +	USB_PLUG_TYPE_A,
> +	USB_PLUG_TYPE_B,
> +	USB_PLUG_TYPE_C,
> +	USB_PLUG_CAPTIVE,
> +};
> +
> +enum typec_data_role {
> +	TYPEC_DEVICE,
> +	TYPEC_HOST,
> +};
> +
> +enum typec_role {
> +	TYPEC_SINK,
> +	TYPEC_SOURCE,
> +};
> +
> +enum typec_pwr_opmode {
> +	TYPEC_PWR_MODE_USB,
> +	TYPEC_PWR_MODE_1_5A,
> +	TYPEC_PWR_MODE_3_0A,
> +	TYPEC_PWR_MODE_PD,
> +};
> +
> +enum typec_accessory {
> +	TYPEC_ACCESSORY_NONE,
> +	TYPEC_ACCESSORY_AUDIO,
> +	TYPEC_ACCESSORY_DEBUG,
> +	TYPEC_ACCESSORY_DAUDIO,
> +};
> +
> +/*
> + * 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
> + * @active_attr: Device attribute to expose active of the mode
> + * @roles: Only for ports. DRP if the mode is awailable in both roles
> + * @roles_attr: Device attribute, only for ports, to expose the supported roles
> + *
> + * 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;
> +	/* Only for ports */
> +	enum typec_port_type	roles;
> +
> +	struct typec_altmode	*alt_mode;
> +
> +	int			index;
> +	char			group_name[8];
> +	struct attribute_group	group;
> +	struct attribute	*attrs[5];
> +	struct device_attribute vdo_attr;
> +	struct device_attribute desc_attr;
> +	struct device_attribute active_attr;
> +	/* Only for ports */
> +	struct device_attribute roles_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 *);
> +
> +void typec_altmode_update_active(struct typec_altmode *alt, int mode,
> +				 bool active);
> +
> +int typec_register_altmodes(struct device *, struct typec_altmode *);
> +void typec_unregister_altmodes(struct device *);
> +
> +/*
> + * 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
> + * @usb_pd: Electronically Marked Cable
> + * @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;
> +	u32			vdo;
> +	unsigned int		usb_pd:1;
> +	unsigned int		active:1;
> +	unsigned int		sop_pp_controller:1;
> +
> +	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
> + * @usb_pd: USB Power Delivery support
> + * @vdo: VDO returned by Discover Identity USB PD command
> + * @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;
> +	unsigned int		usb_pd:1;
> +	u32			vdo;
> +	enum typec_accessory	accessory;
> +	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
> + * @accessory: Supported Accessory Modes
> + * @num_accessory: Number of supported Accessory Modes
> + * @alt_modes: Alternate Modes the connector supports (null terminated)
> + * @try_role: Set a fixed data role for DRP port
> + * @dr_set: Set Data Role
> + * @pr_set: Set Power Role
> + * @vconn_set: Set VCONN Role
> + * @activate_mode: Enter/exit 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;
> +	enum typec_accessory	*accessory;
> +	unsigned int		num_accessory;
> +	struct typec_altmode	*alt_modes;
> +
> +	int			(*try_role)(const struct typec_capability *,
> +					    enum typec_role);
> +
> +	int			(*dr_set)(const struct typec_capability *,
> +					  enum typec_data_role);
> +	int			(*pr_set)(const struct typec_capability *,
> +					  enum typec_role);
> +	int			(*vconn_set)(const struct typec_capability *,
> +					     enum typec_role);
> +
> +	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
> + * @data_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_data_role	data_role;
> +	enum typec_role		pwr_role;
> +	enum typec_role		vconn_role;
> +	enum typec_pwr_opmode	pwr_opmode;
> +};
> +
> +struct typec_port *typec_register_port(struct device *dev,
> +				       const 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);
> +
> +/* Callbacks from driver */
> +
> +void typec_set_data_role(struct typec_port *, enum typec_data_role);
> +void typec_set_pwr_role(struct typec_port *, enum typec_role);
> +void typec_set_vconn_role(struct typec_port *, enum typec_role);
> +void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
> +
> +#endif /* __LINUX_USB_TYPEC_H */
> -- 
> 2.8.1
> 

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Guenter,

On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > +static ssize_t
> > +preferred_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_role role;
> > +	int ret;
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	if (!port->cap->try_role) {
> > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> 
> With this, 'echo "sink" > preferred_role' no longer works because
> match_string() tries to match the entire string, including the newline
> generated by the echo command. One has to use "echo -n" instead.
> Is this on purpose ? It is quite unusual.

For some reason I though 'echo -n is expected. I guess I'm still
living in the past in the days when the kernel version was still 2.4.
It did not use to be so unusual, and there are still plenty of sysfs
attributes that do expect 'echo -n..'.

But I'll fix this.

> > +	if (ret < 0) {
> > +		port->prefer_role = -1;
> > +		ret = size;
> > +		goto out;
> > +	}
> > +
> > +	role = ret;
> > +
> > +	ret = port->cap->try_role(port->cap, role);
> > +	if (ret)
> > +		goto out;
> > +
> > +	port->prefer_role = role;
> > +	ret = size;
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}
> > +
> > +static ssize_t
> > +preferred_role_show(struct device *dev, struct device_attribute *attr,
> > +		    char *buf)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	ssize_t ret = 0;
> > +
> > +	mutex_lock(&port->lock);
> > +
> > +	if (port->prefer_role < 0)
> > +		goto out;
> > +
> > +	ret = sprintf(buf, "%s\n", typec_roles[port->prefer_role]);
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RW(preferred_role);
> > +
> > +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);
> > +	int ret = size;
> > +
> > +	mutex_lock(&port->lock);
> > +
> With this lock, the code in the driver can no longer call any state updates
> during the call to dr_set(), because those (eg typec_set_data_role()) set
> the lock as well. This means that the dr_set call and all other similar calls
> now have to be asynchronous. As a result, those calls can not return an error
> if the role change request fails. Is this on purpose ? I see it as a major
> drawback, not only because errors can no longer be returned, but also because
> I very much preferred the call to be synchronous.

But the drivers should not call typec_set_data_role() in these
cases? That API the drivers should only use if the partners execute
the swaps.

So in this case, we need to set the role and notify sysfs here. This
function has to return successfully only if the role swap really
happened, and dr_set() of course can not return before the driver
actually knows the result of, for example, DR_swap.

> > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	if (!port->cap->dr_set) {
> > +		dev_dbg(dev, "data role swapping not supported\n");
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	if (!port->connected)
> > +		goto out;
> > +
> > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = port->cap->dr_set(port->cap, ret);
> > +	if (ret)
> > +		goto out;

I could have sworn I was setting the port->data_role here, but I guess
it was removed at some point...

Need to fix that.

> > +	ret = size;
> > +out:
> > +	mutex_unlock(&port->lock);
> > +	return ret;
> > +}


Thanks,

-- 
heikki

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Fri, Jul 01, 2016 at 10:13:48AM +0300, Heikki Krogerus wrote:
> Hi Guenter,
> 
> On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > > +static ssize_t
> > > +preferred_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_role role;
> > > +	int ret;
> > > +
> > > +	mutex_lock(&port->lock);
> > > +
> > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!port->cap->try_role) {
> > > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> > 
> > With this, 'echo "sink" > preferred_role' no longer works because
> > match_string() tries to match the entire string, including the newline
> > generated by the echo command. One has to use "echo -n" instead.
> > Is this on purpose ? It is quite unusual.
> 
> For some reason I though 'echo -n is expected. I guess I'm still
> living in the past in the days when the kernel version was still 2.4.
> It did not use to be so unusual, and there are still plenty of sysfs
> attributes that do expect 'echo -n..'.
> 
> But I'll fix this.

<snip>

> > > +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);
> > > +	int ret = size;
> > > +
> > > +	mutex_lock(&port->lock);
> > > +
> > With this lock, the code in the driver can no longer call any state updates
> > during the call to dr_set(), because those (eg typec_set_data_role()) set
> > the lock as well. This means that the dr_set call and all other similar calls
> > now have to be asynchronous. As a result, those calls can not return an error
> > if the role change request fails. Is this on purpose ? I see it as a major
> > drawback, not only because errors can no longer be returned, but also because
> > I very much preferred the call to be synchronous.
> 
> But the drivers should not call typec_set_data_role() in these
> cases? That API the drivers should only use if the partners execute
> the swaps.
> 
> So in this case, we need to set the role and notify sysfs here. This
> function has to return successfully only if the role swap really
> happened, and dr_set() of course can not return before the driver
> actually knows the result of, for example, DR_swap.
> 
> > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!port->cap->dr_set) {
> > > +		dev_dbg(dev, "data role swapping not supported\n");
> > > +		ret = -EOPNOTSUPP;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (!port->connected)
> > > +		goto out;
> > > +
> > > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > > +	if (ret < 0)
> > > +		goto out;
> > > +
> > > +	ret = port->cap->dr_set(port->cap, ret);
> > > +	if (ret)
> > > +		goto out;
> 
> I could have sworn I was setting the port->data_role here, but I guess
> it was removed at some point...
> 
> Need to fix that.

I've updated my github branch with a commit where both of these issues
should be fixed. Can you give it a try?

I've also removed the supports_usb_power_delivery and id_header_vdo
attributes from partners and cables. We really have to put all the USB
PD specific attributes to its own group, and that group can't be in
control of this class. We will simply not always have access to the
kind of USB PD specific details you want to show, for example details
that you get from discover identity command, even when USB PD is fully
supported. For example on systems that use UCSI.

The alternate mode VDO is the only that we can for fairly certainly
always get, and the only things purely tied to USB PD (the plugs are
also, but the only purpose for them is to expose the alternate modes
they have).

Man, I was always against coupling this thing too tightly with USB PD.
I'm slipping.


Thanks,

-- 
heikki

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> On Fri, Jul 01, 2016 at 10:13:48AM +0300, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Thu, Jun 30, 2016 at 03:02:20PM -0700, Guenter Roeck wrote:
> > > > +static ssize_t
> > > > +preferred_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_role role;
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&port->lock);
> > > > +
> > > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > > +		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->cap->try_role) {
> > > > +		dev_dbg(dev, "Setting preferred role not supported\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	ret = match_string(typec_roles, ARRAY_SIZE(typec_roles), buf);
> > > 
> > > With this, 'echo "sink" > preferred_role' no longer works because
> > > match_string() tries to match the entire string, including the newline
> > > generated by the echo command. One has to use "echo -n" instead.
> > > Is this on purpose ? It is quite unusual.
> > 
> > For some reason I though 'echo -n is expected. I guess I'm still
> > living in the past in the days when the kernel version was still 2.4.
> > It did not use to be so unusual, and there are still plenty of sysfs
> > attributes that do expect 'echo -n..'.
> > 
> > But I'll fix this.
> 
> <snip>
> 
> > > > +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);
> > > > +	int ret = size;
> > > > +
> > > > +	mutex_lock(&port->lock);
> > > > +
> > > With this lock, the code in the driver can no longer call any state updates
> > > during the call to dr_set(), because those (eg typec_set_data_role()) set
> > > the lock as well. This means that the dr_set call and all other similar calls
> > > now have to be asynchronous. As a result, those calls can not return an error
> > > if the role change request fails. Is this on purpose ? I see it as a major
> > > drawback, not only because errors can no longer be returned, but also because
> > > I very much preferred the call to be synchronous.
> > 
> > But the drivers should not call typec_set_data_role() in these
> > cases? That API the drivers should only use if the partners execute
> > the swaps.
> > 
> > So in this case, we need to set the role and notify sysfs here. This
> > function has to return successfully only if the role swap really
> > happened, and dr_set() of course can not return before the driver
> > actually knows the result of, for example, DR_swap.
> > 
> > > > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > > > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->cap->dr_set) {
> > > > +		dev_dbg(dev, "data role swapping not supported\n");
> > > > +		ret = -EOPNOTSUPP;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!port->connected)
> > > > +		goto out;
> > > > +
> > > > +	ret = match_string(typec_data_roles, ARRAY_SIZE(typec_data_roles), buf);
> > > > +	if (ret < 0)
> > > > +		goto out;
> > > > +
> > > > +	ret = port->cap->dr_set(port->cap, ret);
> > > > +	if (ret)
> > > > +		goto out;
> > 
> > I could have sworn I was setting the port->data_role here, but I guess
> > it was removed at some point...
> > 
> > Need to fix that.
> 
> I've updated my github branch with a commit where both of these issues
> should be fixed. Can you give it a try?
> 

This is going to be very difficult. So far, calls such as
typec_set_data_role() were independent (asynchronous) of role change
requests through sysfs, meaning they happened whenever lower level
confirmed that a role change was complete, for whatever reason. Now
I have to check if a role change request through the class code
is pending and not call typec_set_data_role() and friends in that case.

This becomes even more complicated in situations with parallel role
change requests both from the class code and from the partner.
In such cases the class code may have acquired the lock, and before it
calls the driver, a role change complete is reported. The call to
typec_set_XXX_role() will then get stuck. So I'll have to add another
flag before calling typec_set_data_role() and friends, and then reject
the call to dr_set and all other set requests with -EBUSY.
race conditions.

Thinking more ... ultimately, this means that I'll have to reject role
change requests from the class code whenever the state machine is busy,
because I never know if the state machine activity would result in a
call into the typec class code. At the same time, the protocol driver
will have to reject incoming role change requests while a locally
requested role change request is pending, even if that internal
role change request has not yet resulted in a PD message being sent.

Is this really necessary ? It creates a lot of interdependencies.

In summary, this means a substantial amount of code and testing, which
is going to take a while.

You might want to mention all this in the API documentation.

> I've also removed the supports_usb_power_delivery and id_header_vdo
> attributes from partners and cables. We really have to put all the USB
> PD specific attributes to its own group, and that group can't be in
> control of this class. We will simply not always have access to the
> kind of USB PD specific details you want to show, for example details
> that you get from discover identity command, even when USB PD is fully
> supported. For example on systems that use UCSI.
> 

I think we should have a single unified ABI, independent of the underlying
driver, that informs the user about the partner device. I really don't
quite understand why the class code should not be able to report what device
it is connected to (while at the same time being able to report the alternate
modes it supports).

Following your logic, one could argue that all alternate mode handling
should be taken out as well. After all, alternate modes are also tied
to PD protocol support, and discovering the partner identity is a
prerequisite of discovering the alternate modes it supports.
That really doesn't make any sense.

Guenter

> The alternate mode VDO is the only that we can for fairly certainly
> always get, and the only things purely tied to USB PD (the plugs are
> also, but the only purpose for them is to expose the alternate modes
> they have).
> 
> Man, I was always against coupling this thing too tightly with USB PD.
> I'm slipping.
> 
> 
> Thanks,
> 
> -- 
> heikki

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
> On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> > I've updated my github branch with a commit where both of these issues
> > should be fixed. Can you give it a try?
> > 
> 
> This is going to be very difficult. So far, calls such as
> typec_set_data_role() were independent (asynchronous) of role change
> requests through sysfs, meaning they happened whenever lower level
> confirmed that a role change was complete, for whatever reason. Now
> I have to check if a role change request through the class code
> is pending and not call typec_set_data_role() and friends in that case.

I'm sorry about the misunderstanding.

What you want is basically that we only support non-blocking mode with
this interface, and we can't do that.

Would you even be able to make it work? How would you report errors
for example?

> This becomes even more complicated in situations with parallel role
> change requests both from the class code and from the partner.
> In such cases the class code may have acquired the lock, and before it
> calls the driver, a role change complete is reported. The call to
> typec_set_XXX_role() will then get stuck. So I'll have to add another
> flag before calling typec_set_data_role() and friends, and then reject
> the call to dr_set and all other set requests with -EBUSY.
> race conditions.

Or we'll just export the port locks somehow, or provide separate API
for handling them.

..typec_get_port_lock(..
{
        return &port->lock;
}

or

typec_lock(..
typec_test_and_lock(..
typec_unlock(..
...

> Thinking more ... ultimately, this means that I'll have to reject role
> change requests from the class code whenever the state machine is busy,
> because I never know if the state machine activity would result in a
> call into the typec class code.

Yes, why is that a problem? The other option would be that you queue
the requests from users, and I'm pretty sure we want to avoid that. It
would mean you have to consider a lot of conditions to avoid any
races, and you would also have to make a lot of extra decisions. As an
example, what do you do if two role change requests are in the queue
for the same role? If the first requests is successful, you probable
can just report success to the second request also, _maybe_! But if
the first one fails, do you try again for the second request, and in
that case do you wait for the result of the second request before
reporting to the first, or do you just fail both of them? etc.

> At the same time, the protocol driver
> will have to reject incoming role change requests while a locally
> requested role change request is pending, even if that internal
> role change request has not yet resulted in a PD message being sent.

Why?

> Is this really necessary ? It creates a lot of interdependencies.
> 
> In summary, this means a substantial amount of code and testing, which
> is going to take a while.
> 
> You might want to mention all this in the API documentation.

OK.

> > I've also removed the supports_usb_power_delivery and id_header_vdo
> > attributes from partners and cables. We really have to put all the USB
> > PD specific attributes to its own group, and that group can't be in
> > control of this class. We will simply not always have access to the
> > kind of USB PD specific details you want to show, for example details
> > that you get from discover identity command, even when USB PD is fully
> > supported. For example on systems that use UCSI.
> > 
> 
> I think we should have a single unified ABI, independent of the underlying
> driver, that informs the user about the partner device. I really don't
> quite understand why the class code should not be able to report what device
> it is connected to (while at the same time being able to report the alternate
> modes it supports).

OK, let's plan this more then. Maybe we can make for example a layer
that creates the groups for the PD specific details to the class?

The problem is still that we can only provide results from for example
request identity command reliably when the PD protocol layer is
completely inside kernel, and that is not always the case. So we
really need to group those details in its own group, and that group
will basically indicate is the PD stack inside the kernel or not.

We should not forget also that the userspace can never rely on those
details because of the fact that they simply will not always be
available.


Thanks,

-- 
heikki

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On 07/03/2016 12:38 PM, Heikki Krogerus wrote:
> On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
>> On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
>>> I've updated my github branch with a commit where both of these issues
>>> should be fixed. Can you give it a try?
>>>
>>
>> This is going to be very difficult. So far, calls such as
>> typec_set_data_role() were independent (asynchronous) of role change
>> requests through sysfs, meaning they happened whenever lower level
>> confirmed that a role change was complete, for whatever reason. Now
>> I have to check if a role change request through the class code
>> is pending and not call typec_set_data_role() and friends in that case.
>
> I'm sorry about the misunderstanding.
>
> What you want is basically that we only support non-blocking mode with
> this interface, and we can't do that.
>

No, that is not what I said or asked for. The problems I am seeing are due
to locking in the class code. "Asynchronous" above referred to the state
machine vs. role change requests by the class code, which operates independent
of each other in my code. Set requests from the class code still wait for
the state machine to complete.

The problem is that the state machine now needs to know if the class code
has a set role request pending, because in that case it can no longer report
role changes directly to the class code. This includes role changes unrelated
to the actual set role request. That code is now much more complicated, especially
since each callback into the typec code is handled differently. For example,
typec_disconnect() does not require a lock (unless I missed it), but many
other functions do.

> Would you even be able to make it work? How would you report errors
> for example?
>

It worked just great when you had no locks. All I had to do is handle
role changes in the driver, and to implement the necessary locks there.
Actually, before you introduced the locks, you had suggested that this
was the way to go, which is why I implemented it that way.

>> This becomes even more complicated in situations with parallel role
>> change requests both from the class code and from the partner.
>> In such cases the class code may have acquired the lock, and before it
>> calls the driver, a role change complete is reported. The call to
>> typec_set_XXX_role() will then get stuck. So I'll have to add another
>> flag before calling typec_set_data_role() and friends, and then reject
>> the call to dr_set and all other set requests with -EBUSY.
>> race conditions.
>
> Or we'll just export the port locks somehow, or provide separate API
> for handling them.
>
> ..typec_get_port_lock(..
> {
>          return &port->lock;
> }
>
> or
>
> typec_lock(..
> typec_test_and_lock(..
> typec_unlock(..
> ...
>
>> Thinking more ... ultimately, this means that I'll have to reject role
>> change requests from the class code whenever the state machine is busy,
>> because I never know if the state machine activity would result in a
>> call into the typec class code.
>
> Yes, why is that a problem? The other option would be that you queue
> the requests from users, and I'm pretty sure we want to avoid that. It
> would mean you have to consider a lot of conditions to avoid any
> races, and you would also have to make a lot of extra decisions. As an
> example, what do you do if two role change requests are in the queue
> for the same role? If the first requests is successful, you probable
> can just report success to the second request also, _maybe_! But if
> the first one fails, do you try again for the second request, and in
> that case do you wait for the result of the second request before
> reporting to the first, or do you just fail both of them? etc.
>

No, that is not correct. Again, all I had to do was to implement a lock
in the driver which waited until the state machine was idle, and _then_
check if the set command needed to do anything. No queuing necessary,
no race conditions.

Keep in mind there are no role _change_ requests, the requests are to set
a specific role - and I can do that once I have the state machine lock.
The second request would again be a request to set a specific role. It waits
for the state machine lock, and after it is acquired decides if a role
change is necessary or not.

Sure, I can switch to your method, and I guess I'll have to.
I just think that the resulting -EBUSY responses are unnecessary.

I will reiterate, though, that this all worked just fine with the previous
version of your code, where sysfs role change requests did not require
or implement a lock in the class code.

>> At the same time, the protocol driver
>> will have to reject incoming role change requests while a locally
>> requested role change request is pending, even if that internal
>> role change request has not yet resulted in a PD message being sent.
>
> Why?
>
Example:

Class code requests data role change, remote partner requests power role
change. Both are handled in parallel by the state machine. State machine
tries to report power role change to class code and gets stuck because the
class code holds a lock due to the pending data role change.

Therefore, a role change request from the partner now has to be rejected
while a (different) role change request from the class code is pending.

>> Is this really necessary ? It creates a lot of interdependencies.
>>
>> In summary, this means a substantial amount of code and testing, which
>> is going to take a while.
>>
>> You might want to mention all this in the API documentation.
>
> OK.
>
>>> I've also removed the supports_usb_power_delivery and id_header_vdo
>>> attributes from partners and cables. We really have to put all the USB
>>> PD specific attributes to its own group, and that group can't be in
>>> control of this class. We will simply not always have access to the
>>> kind of USB PD specific details you want to show, for example details
>>> that you get from discover identity command, even when USB PD is fully
>>> supported. For example on systems that use UCSI.
>>>
>>
>> I think we should have a single unified ABI, independent of the underlying
>> driver, that informs the user about the partner device. I really don't
>> quite understand why the class code should not be able to report what device
>> it is connected to (while at the same time being able to report the alternate
>> modes it supports).
>
> OK, let's plan this more then. Maybe we can make for example a layer
> that creates the groups for the PD specific details to the class?
>
> The problem is still that we can only provide results from for example
> request identity command reliably when the PD protocol layer is
> completely inside kernel, and that is not always the case. So we
> really need to group those details in its own group, and that group
> will basically indicate is the PD stack inside the kernel or not.
>
> We should not forget also that the userspace can never rely on those
> details because of the fact that they simply will not always be
> available.
>
On the other side, not being able to rely on a well defined ABI makes the
ABI much less useful.

Guenter

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Guenter,

On Sun, Jul 03, 2016 at 02:28:44PM -0700, Guenter Roeck wrote:
> On 07/03/2016 12:38 PM, Heikki Krogerus wrote:
> > On Fri, Jul 01, 2016 at 07:33:12AM -0700, Guenter Roeck wrote:
> > > On Fri, Jul 01, 2016 at 03:05:35PM +0300, Heikki Krogerus wrote:
> > > > I've updated my github branch with a commit where both of these issues
> > > > should be fixed. Can you give it a try?
> > > > 
> > > 
> > > This is going to be very difficult. So far, calls such as
> > > typec_set_data_role() were independent (asynchronous) of role change
> > > requests through sysfs, meaning they happened whenever lower level
> > > confirmed that a role change was complete, for whatever reason. Now
> > > I have to check if a role change request through the class code
> > > is pending and not call typec_set_data_role() and friends in that case.
> > 
> > I'm sorry about the misunderstanding.
> > 
> > What you want is basically that we only support non-blocking mode with
> > this interface, and we can't do that.
> > 
> 
> No, that is not what I said or asked for. The problems I am seeing are due
> to locking in the class code. "Asynchronous" above referred to the state
> machine vs. role change requests by the class code, which operates independent
> of each other in my code. Set requests from the class code still wait for
> the state machine to complete.
> 
> The problem is that the state machine now needs to know if the class code
> has a set role request pending, because in that case it can no longer report
> role changes directly to the class code. This includes role changes unrelated
> to the actual set role request. That code is now much more complicated, especially
> since each callback into the typec code is handled differently. For example,
> typec_disconnect() does not require a lock (unless I missed it), but many
> other functions do.

It seems that I have misunderstood your whole point. I'm really sorry.
I was in a hurry to start my vacation.

So let's just fix the locking.

<snip>

> > > > I've also removed the supports_usb_power_delivery and id_header_vdo
> > > > attributes from partners and cables. We really have to put all the USB
> > > > PD specific attributes to its own group, and that group can't be in
> > > > control of this class. We will simply not always have access to the
> > > > kind of USB PD specific details you want to show, for example details
> > > > that you get from discover identity command, even when USB PD is fully
> > > > supported. For example on systems that use UCSI.
> > > > 
> > > 
> > > I think we should have a single unified ABI, independent of the underlying
> > > driver, that informs the user about the partner device. I really don't
> > > quite understand why the class code should not be able to report what device
> > > it is connected to (while at the same time being able to report the alternate
> > > modes it supports).
> > 
> > OK, let's plan this more then. Maybe we can make for example a layer
> > that creates the groups for the PD specific details to the class?
> > 
> > The problem is still that we can only provide results from for example
> > request identity command reliably when the PD protocol layer is
> > completely inside kernel, and that is not always the case. So we
> > really need to group those details in its own group, and that group
> > will basically indicate is the PD stack inside the kernel or not.
> > 
> > We should not forget also that the userspace can never rely on those
> > details because of the fact that they simply will not always be
> > available.
> > 
> On the other side, not being able to rely on a well defined ABI makes the
> ABI much less useful.

What do we do about this?


Thanks,

-- 
heikki

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On 07/04/2016 10:11 AM, Heikki Krogerus wrote:
[ ... ]

>>> We should not forget also that the userspace can never rely on those
>>> details because of the fact that they simply will not always be
>>> available.
>>>
>> On the other side, not being able to rely on a well defined ABI makes the
>> ABI much less useful.
>
> What do we do about this?
>
I'll have to think about it. Unfortunately, I'll have to put this on the
back-burner for the time being. My primary problem is that I need
a functional  driver _now_ (ie by the end of this week), to meet an
upcoming deadline. Due to to the locking problem, I can not rely on the
typec infrastructure ... meaning I have to take two steps back and get
my code working with the Android infrastructure (which, coincidentally,
does not require locking and thus doesn't have all the resulting
limitations and complexities).

Guenter

Re: [PATCHv4 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Mon, Jul 04, 2016 at 10:45:02AM -0700, Guenter Roeck wrote:
> On 07/04/2016 10:11 AM, Heikki Krogerus wrote:
> [ ... ]
> 
> > > > We should not forget also that the userspace can never rely on those
> > > > details because of the fact that they simply will not always be
> > > > available.
> > > > 
> > > On the other side, not being able to rely on a well defined ABI makes the
> > > ABI much less useful.
> > 
> > What do we do about this?
> > 
> I'll have to think about it. Unfortunately, I'll have to put this on the
> back-burner for the time being. My primary problem is that I need
> a functional  driver _now_ (ie by the end of this week), to meet an
> upcoming deadline. Due to to the locking problem, I can not rely on the
> typec infrastructure ... meaning I have to take two steps back and get
> my code working with the Android infrastructure (which, coincidentally,
> does not require locking and thus doesn't have all the resulting
> limitations and complexities).

That is unfortunate.

I added a commit where I remove the lock completely (at least for
now) to my github.


Cheers,

-- 
heikki

[PATCHv4 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

[Heikki Krogerus]

This adds driver for the USB Type-C PHY on Intel WhiskeyCove
PMIC which is available on some of the Intel Broxton SoC
based platforms.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/Kconfig       |  14 ++
 drivers/usb/typec/Makefile      |   1 +
 drivers/usb/typec/typec_wcove.c | 371 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 386 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index b229fb9..7a345a4 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB PD and Type-C drivers"
 config TYPEC
 	tristate
 
+config TYPEC_WCOVE
+	tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
+	depends on ACPI
+	depends on INTEL_SOC_PMIC
+	depends on INTEL_PMC_IPC
+	select TYPEC
+	help
+	  This driver adds support for USB Type-C detection on Intel Broxton
+	  platforms that have Intel Whiskey Cove PMIC. The driver can detect the
+	  role and cable orientation.
+
+	  To compile this driver as module, choose M here: the module will be
+	  called typec_wcove
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1012a8b..b9cb862 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)		+= typec.o
+obj-$(CONFIG_TYPEC_WCOVE)	+= typec_wcove.o
diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
new file mode 100644
index 0000000..c7c2d28
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,371 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY driver
+ *
+ * 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/acpi.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/usb/typec.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/intel_soc_pmic.h>
+
+/* Register offsets */
+#define WCOVE_CHGRIRQ0		0x4e09
+#define WCOVE_PHYCTRL		0x5e07
+
+#define USBC_CONTROL1		0x7001
+#define USBC_CONTROL2		0x7002
+#define USBC_CONTROL3		0x7003
+#define USBC_CC1_CTRL		0x7004
+#define USBC_CC2_CTRL		0x7005
+#define USBC_STATUS1		0x7007
+#define USBC_STATUS2		0x7008
+#define USBC_STATUS3		0x7009
+#define USBC_IRQ1		0x7015
+#define USBC_IRQ2		0x7016
+#define USBC_IRQMASK1		0x7017
+#define USBC_IRQMASK2		0x7018
+
+/* Register bits */
+
+#define USBC_CONTROL1_MODE_DRP(r)	((r & ~0x7) | 4)
+
+#define USBC_CONTROL2_UNATT_SNK		BIT(0)
+#define USBC_CONTROL2_UNATT_SRC		BIT(1)
+#define USBC_CONTROL2_DIS_ST		BIT(2)
+
+#define USBC_CONTROL3_PD_DIS		BIT(1)
+
+#define USBC_CC_CTRL_VCONN_EN		BIT(1)
+
+#define USBC_STATUS1_DET_ONGOING	BIT(6)
+#define USBC_STATUS1_RSLT(r)		(r & 0xf)
+#define USBC_RSLT_NOTHING		0
+#define USBC_RSLT_SRC_DEFAULT		1
+#define USBC_RSLT_SRC_1_5A		2
+#define USBC_RSLT_SRC_3_0A		3
+#define USBC_RSLT_SNK			4
+#define USBC_RSLT_DEBUG_ACC		5
+#define USBC_RSLT_AUDIO_ACC		6
+#define USBC_RSLT_UNDEF			15
+#define USBC_STATUS1_ORIENT(r)		((r >> 4) & 0x3)
+#define USBC_ORIENT_NORMAL		1
+#define USBC_ORIENT_REVERSE		2
+
+#define USBC_STATUS2_VBUS_REQ		BIT(5)
+
+#define USBC_IRQ1_ADCDONE1		BIT(2)
+#define USBC_IRQ1_OVERTEMP		BIT(1)
+#define USBC_IRQ1_SHORT			BIT(0)
+
+#define USBC_IRQ2_CC_CHANGE		BIT(7)
+#define USBC_IRQ2_RX_PD			BIT(6)
+#define USBC_IRQ2_RX_HR			BIT(5)
+#define USBC_IRQ2_RX_CR			BIT(4)
+#define USBC_IRQ2_TX_SUCCESS		BIT(3)
+#define USBC_IRQ2_TX_FAIL		BIT(2)
+
+#define USBC_IRQMASK1_ALL	(USBC_IRQ1_ADCDONE1 | USBC_IRQ1_OVERTEMP | \
+				 USBC_IRQ1_SHORT)
+
+#define USBC_IRQMASK2_ALL	(USBC_IRQ2_CC_CHANGE | USBC_IRQ2_RX_PD | \
+				 USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
+				 USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
+
+struct wcove_typec {
+	struct mutex lock; /* device lock */
+	struct device *dev;
+	struct regmap *regmap;
+	struct typec_port *port;
+	struct typec_capability cap;
+	struct typec_connection con;
+	struct typec_partner partner;
+};
+
+enum wcove_typec_func {
+	WCOVE_FUNC_DRIVE_VBUS = 1,
+	WCOVE_FUNC_ORIENTATION,
+	WCOVE_FUNC_ROLE,
+	WCOVE_FUNC_DRIVE_VCONN,
+};
+
+enum wcove_typec_orientation {
+	WCOVE_ORIENTATION_NORMAL,
+	WCOVE_ORIENTATION_REVERSE,
+};
+
+enum wcove_typec_role {
+	WCOVE_ROLE_HOST,
+	WCOVE_ROLE_DEVICE,
+};
+
+static uuid_le uuid = UUID_LE(0x482383f0, 0x2876, 0x4e49,
+			      0x86, 0x85, 0xdb, 0x66, 0x21, 0x1a, 0xf0, 0x37);
+
+static int wcove_typec_func(struct wcove_typec *wcove,
+			    enum wcove_typec_func func, int param)
+{
+	union acpi_object *obj;
+	union acpi_object tmp;
+	union acpi_object argv4 = ACPI_INIT_DSM_ARGV4(1, &tmp);
+
+	tmp.type = ACPI_TYPE_INTEGER;
+	tmp.integer.value = param;
+
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(wcove->dev), uuid.b, 1, func,
+				&argv4);
+	if (!obj) {
+		dev_err(wcove->dev, "%s: failed to evaluate _DSM\n", __func__);
+		return -EIO;
+	}
+
+	ACPI_FREE(obj);
+	return 0;
+}
+
+static void wcove_typec_device_mode(struct wcove_typec *wcove)
+{
+	wcove->partner.type = TYPEC_PARTNER_USB;
+	wcove->con.partner = &wcove->partner;
+	wcove->con.pwr_role = TYPEC_SINK;
+	wcove->con.vconn_role = TYPEC_SINK;
+	wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_DEVICE);
+	typec_connect(wcove->port, &wcove->con);
+}
+
+static irqreturn_t wcove_typec_irq(int irq, void *data)
+{
+	struct wcove_typec *wcove = data;
+	unsigned int cc1_ctrl;
+	unsigned int cc2_ctrl;
+	unsigned int cc_irq1;
+	unsigned int cc_irq2;
+	unsigned int status1;
+	unsigned int status2;
+	int ret;
+
+	mutex_lock(&wcove->lock);
+
+	ret = regmap_read(wcove->regmap, USBC_IRQ1, &cc_irq1);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_IRQ2, &cc_irq2);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_STATUS1, &status1);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_STATUS2, &status2);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_CC1_CTRL, &cc1_ctrl);
+	if (ret)
+		goto err;
+
+	ret = regmap_read(wcove->regmap, USBC_CC2_CTRL, &cc2_ctrl);
+	if (ret)
+		goto err;
+
+	if (cc_irq1) {
+		if (cc_irq1 & USBC_IRQ1_OVERTEMP)
+			dev_err(wcove->dev, "VCONN Switch Over Temperature!\n");
+		if (cc_irq1 & USBC_IRQ1_SHORT)
+			dev_err(wcove->dev, "VCONN Switch Short Circuit!\n");
+		regmap_write(wcove->regmap, USBC_IRQ1, cc_irq1);
+	}
+
+	if (cc_irq2) {
+		regmap_write(wcove->regmap, USBC_IRQ2, cc_irq2);
+		/*
+		 * Ingoring any PD communication interrupts until the PD stack
+		 * is in place
+		 */
+		if (cc_irq2 & ~USBC_IRQ2_CC_CHANGE) {
+			dev_WARN(wcove->dev, "USB PD handling missing\n");
+			goto err;
+		}
+	}
+
+	if (status1 & USBC_STATUS1_DET_ONGOING)
+		goto out;
+
+	if (USBC_STATUS1_RSLT(status1) == USBC_RSLT_NOTHING) {
+		if (wcove->con.partner) {
+			typec_disconnect(wcove->port);
+			memset(&wcove->con, 0, sizeof(wcove->con));
+			memset(&wcove->partner, 0, sizeof(wcove->partner));
+		}
+
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_NORMAL);
+		/* Host mode by default */
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
+		goto out;
+	}
+
+	if (wcove->con.partner)
+		goto out;
+
+	switch (USBC_STATUS1_ORIENT(status1)) {
+	case USBC_ORIENT_NORMAL:
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_NORMAL);
+		break;
+	case USBC_ORIENT_REVERSE:
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_REVERSE);
+	default:
+		break;
+	}
+
+	switch (USBC_STATUS1_RSLT(status1)) {
+	case USBC_RSLT_SRC_DEFAULT:
+		wcove->con.pwr_opmode = TYPEC_PWR_MODE_USB;
+		wcove_typec_device_mode(wcove);
+		break;
+	case USBC_RSLT_SRC_1_5A:
+		wcove->con.pwr_opmode = TYPEC_PWR_MODE_1_5A;
+		wcove_typec_device_mode(wcove);
+		break;
+	case USBC_RSLT_SRC_3_0A:
+		wcove->con.pwr_opmode = TYPEC_PWR_MODE_3_0A;
+		wcove_typec_device_mode(wcove);
+		break;
+	case USBC_RSLT_SNK:
+		wcove->partner.type = TYPEC_PARTNER_USB;
+		wcove->con.partner = &wcove->partner;
+		wcove->con.data_role = TYPEC_HOST;
+		wcove->con.pwr_role = TYPEC_SOURCE;
+		wcove->con.vconn_role = TYPEC_SOURCE;
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
+		typec_connect(wcove->port, &wcove->con);
+		break;
+	case USBC_RSLT_DEBUG_ACC:
+		wcove->partner.accessory = TYPEC_ACCESSORY_DEBUG;
+		wcove->partner.type = TYPEC_PARTNER_ACCESSORY;
+		wcove->con.partner = &wcove->partner;
+		typec_connect(wcove->port, &wcove->con);
+		break;
+	case USBC_RSLT_AUDIO_ACC:
+		wcove->partner.accessory = TYPEC_ACCESSORY_AUDIO;
+		wcove->partner.type = TYPEC_PARTNER_ACCESSORY;
+		wcove->con.partner = &wcove->partner;
+		typec_connect(wcove->port, &wcove->con);
+		break;
+	default:
+		dev_WARN(wcove->dev, "%s Undefined result\n", __func__);
+		goto err;
+	}
+out:
+	/* If either CC pins is requesting VCONN, we turn it on */
+	if ((cc1_ctrl & USBC_CC_CTRL_VCONN_EN) ||
+	    (cc2_ctrl &	USBC_CC_CTRL_VCONN_EN))
+		wcove_typec_func(wcove, WCOVE_FUNC_DRIVE_VCONN, true);
+	else
+		wcove_typec_func(wcove, WCOVE_FUNC_DRIVE_VCONN, false);
+
+	/* Relying on the FSM to know when we need to drive VBUS. */
+	wcove_typec_func(wcove, WCOVE_FUNC_DRIVE_VBUS,
+			 !!(status2 & USBC_STATUS2_VBUS_REQ));
+err:
+	/* REVISIT: Clear WhiskeyCove CHGR Type-C interrupt */
+	regmap_write(wcove->regmap, WCOVE_CHGRIRQ0, BIT(5));
+
+	mutex_unlock(&wcove->lock);
+	return IRQ_HANDLED;
+}
+
+static int wcove_typec_probe(struct platform_device *pdev)
+{
+	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
+	struct wcove_typec *wcove;
+	unsigned int val;
+	int ret;
+
+	wcove = devm_kzalloc(&pdev->dev, sizeof(*wcove), GFP_KERNEL);
+	if (!wcove)
+		return -ENOMEM;
+
+	mutex_init(&wcove->lock);
+	wcove->dev = &pdev->dev;
+	wcove->regmap = pmic->regmap;
+
+	ret = regmap_irq_get_virq(pmic->irq_chip_data_level2,
+				  platform_get_irq(pdev, 0));
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_threaded_irq(&pdev->dev, ret, NULL,
+					wcove_typec_irq, IRQF_ONESHOT,
+					"wcove_typec", wcove);
+	if (ret)
+		return ret;
+
+	wcove->cap.type = TYPEC_PORT_DRP;
+
+	wcove->port = typec_register_port(&pdev->dev, &wcove->cap);
+	if (IS_ERR(wcove->port))
+		return PTR_ERR(wcove->port);
+
+	if (!acpi_check_dsm(ACPI_HANDLE(&pdev->dev), uuid.b, 0, 0x1f)) {
+		dev_err(&pdev->dev, "Missing _DSM functions\n");
+		return -ENODEV;
+	}
+
+	/* Make sure the PD PHY is disabled until PD stack is ready */
+	regmap_read(wcove->regmap, USBC_CONTROL3, &val);
+	regmap_write(wcove->regmap, USBC_CONTROL3, val | USBC_CONTROL3_PD_DIS);
+
+	/* DRP mode without accessory support */
+	regmap_read(wcove->regmap, USBC_CONTROL1, &val);
+	regmap_write(wcove->regmap, USBC_CONTROL1, USBC_CONTROL1_MODE_DRP(val));
+
+	/* Unmask everything */
+	regmap_read(wcove->regmap, USBC_IRQMASK1, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK1, val & ~USBC_IRQMASK1_ALL);
+	regmap_read(wcove->regmap, USBC_IRQMASK2, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK2, val & ~USBC_IRQMASK2_ALL);
+
+	platform_set_drvdata(pdev, wcove);
+	return 0;
+}
+
+static int wcove_typec_remove(struct platform_device *pdev)
+{
+	struct wcove_typec *wcove = platform_get_drvdata(pdev);
+	unsigned int val;
+
+	/* Mask everything */
+	regmap_read(wcove->regmap, USBC_IRQMASK1, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK1, val | USBC_IRQMASK1_ALL);
+	regmap_read(wcove->regmap, USBC_IRQMASK2, &val);
+	regmap_write(wcove->regmap, USBC_IRQMASK2, val | USBC_IRQMASK2_ALL);
+
+	typec_unregister_port(wcove->port);
+	return 0;
+}
+
+static struct platform_driver wcove_typec_driver = {
+	.driver = {
+		.name		= "bxt_wcove_usbc",
+	},
+	.probe			= wcove_typec_probe,
+	.remove			= wcove_typec_remove,
+};
+
+module_platform_driver(wcove_typec_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("WhiskeyCove PMIC USB Type-C PHY driver");
-- 
2.8.1

Re: [PATCHv4 0/2] USB Type-C Connector class

[Greg KH]

On Wed, Jun 29, 2016 at 04:38:36PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> The USB Type-C class is meant to provide unified interface to the
> userspace to present the USB Type-C ports in a system.
> 
> Changes since v3:
> - Documentation cleanup as proposed by Roger Quadros
> - Setting partner altmodes member to NULL on removal and fixing a
>   warning, as proposed by Guenter Roeck
> - Added the following attributes for partners and cables:
>   * supports_usb_power_delivery
>   * id_header_vdo
> - "id_header_vdo" is visible only when the partner or cable supports
>   USB Power Delivery communication.
> - Partner attribute "accessory" is hidden when the partner type is not
>   "Accessory".
> 
> Changes since v2:
> - Notification on role and alternate mode changes
> - cleanups
> 
> Changes since v1:
> - Completely rewrote alternate mode support
> - Patners, cables and cable plugs presented as devices.

I'm not going to take this series until everyone agrees on it, sorry.
I'll wait for you and Guenter and Oliver to all come up with a solution
that works for everyone.

So I'm dropping this from my queue now, sorry.

greg k-h

Re: [PATCHv4 0/2] USB Type-C Connector class

[Guenter Roeck]

On Tue, Aug 09, 2016 at 04:01:53PM +0200, Greg KH wrote:
> On Wed, Jun 29, 2016 at 04:38:36PM +0300, Heikki Krogerus wrote:
> > Hi,
> > 
> > The USB Type-C class is meant to provide unified interface to the
> > userspace to present the USB Type-C ports in a system.
> > 
> > Changes since v3:
> > - Documentation cleanup as proposed by Roger Quadros
> > - Setting partner altmodes member to NULL on removal and fixing a
> >   warning, as proposed by Guenter Roeck
> > - Added the following attributes for partners and cables:
> >   * supports_usb_power_delivery
> >   * id_header_vdo
> > - "id_header_vdo" is visible only when the partner or cable supports
> >   USB Power Delivery communication.
> > - Partner attribute "accessory" is hidden when the partner type is not
> >   "Accessory".
> > 
> > Changes since v2:
> > - Notification on role and alternate mode changes
> > - cleanups
> > 
> > Changes since v1:
> > - Completely rewrote alternate mode support
> > - Patners, cables and cable plugs presented as devices.
> 
> I'm not going to take this series until everyone agrees on it, sorry.
> I'll wait for you and Guenter and Oliver to all come up with a solution
> that works for everyone.
> 

I have not heard from Heikko for a while. There have been some follow-up
patches in

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes

but I am not really sure where this is going. I've been wondering if I
can/should get more actively involved and start driving the series if
Heikki is busy. Thoughts, anyone ?

Guenter

Re: [PATCHv4 0/2] USB Type-C Connector class

[Oliver Neukum]

On Tue, 2016-08-09 at 09:23 -0700, Guenter Roeck wrote:
> > I'm not going to take this series until everyone agrees on it,
> sorry.
> > I'll wait for you and Guenter and Oliver to all come up with a
> solution
> > that works for everyone.
> > 
> 
> I have not heard from Heikko for a while. There have been some
> follow-up
> patches in
> 
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes
> 
> but I am not really sure where this is going. I've been wondering if I
> can/should get more actively involved and start driving the series if
> Heikki is busy. Thoughts, anyone ?

It is not clear to me where we were. It seems to me that we found
the API good but disagreed about locking. Does that sum it up?

	Regards
		Oliver

Re: [PATCHv4 0/2] USB Type-C Connector class

[Heikki Krogerus]

Hi guys,

Sorry for the long silence. I just returned from paternal leave.

On Wed, Aug 10, 2016 at 10:19:25AM +0200, Oliver Neukum wrote:
> On Tue, 2016-08-09 at 09:23 -0700, Guenter Roeck wrote:
> > > I'm not going to take this series until everyone agrees on it,
> > sorry.
> > > I'll wait for you and Guenter and Oliver to all come up with a
> > solution
> > > that works for everyone.
> > > 
> > 
> > I have not heard from Heikko for a while. There have been some
> > follow-up
> > patches in
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes
> > 
> > but I am not really sure where this is going. I've been wondering if I
> > can/should get more actively involved and start driving the series if
> > Heikki is busy. Thoughts, anyone ?
> 
> It is not clear to me where we were. It seems to me that we found
> the API good but disagreed about locking. Does that sum it up?

I'll remove the locks and resend.


Thanks,

-- 
heikki

Re: [PATCHv4 0/2] USB Type-C Connector class

[Guenter Roeck]

On Tue, Aug 16, 2016 at 10:38:26AM +0300, Heikki Krogerus wrote:
> Hi guys,
> 
> Sorry for the long silence. I just returned from paternal leave.
> 
> On Wed, Aug 10, 2016 at 10:19:25AM +0200, Oliver Neukum wrote:
> > On Tue, 2016-08-09 at 09:23 -0700, Guenter Roeck wrote:
> > > > I'm not going to take this series until everyone agrees on it,
> > > sorry.
> > > > I'll wait for you and Guenter and Oliver to all come up with a
> > > solution
> > > > that works for everyone.
> > > > 
> > > 
> > > I have not heard from Heikko for a while. There have been some
> > > follow-up
> > > patches in
> > > 
> > > https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes
> > > 
> > > but I am not really sure where this is going. I've been wondering if I
> > > can/should get more actively involved and start driving the series if
> > > Heikki is busy. Thoughts, anyone ?
> > 
> > It is not clear to me where we were. It seems to me that we found
> > the API good but disagreed about locking. Does that sum it up?
> 
> I'll remove the locks and resend.
> 
I have a new version of my patch set ready. Should I wait for your update
or send it now ?

Thanks,
Guenter

Re: [PATCHv4 0/2] USB Type-C Connector class

[Heikki Krogerus]

On Tue, Aug 16, 2016 at 09:40:16AM -0700, Guenter Roeck wrote:
> On Tue, Aug 16, 2016 at 10:38:26AM +0300, Heikki Krogerus wrote:
> > Hi guys,
> > 
> > Sorry for the long silence. I just returned from paternal leave.
> > 
> > On Wed, Aug 10, 2016 at 10:19:25AM +0200, Oliver Neukum wrote:
> > > On Tue, 2016-08-09 at 09:23 -0700, Guenter Roeck wrote:
> > > > > I'm not going to take this series until everyone agrees on it,
> > > > sorry.
> > > > > I'll wait for you and Guenter and Oliver to all come up with a
> > > > solution
> > > > > that works for everyone.
> > > > > 
> > > > 
> > > > I have not heard from Heikko for a while. There have been some
> > > > follow-up
> > > > patches in
> > > > 
> > > > https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes
> > > > 
> > > > but I am not really sure where this is going. I've been wondering if I
> > > > can/should get more actively involved and start driving the series if
> > > > Heikki is busy. Thoughts, anyone ?
> > > 
> > > It is not clear to me where we were. It seems to me that we found
> > > the API good but disagreed about locking. Does that sum it up?
> > 
> > I'll remove the locks and resend.
> > 
> I have a new version of my patch set ready. Should I wait for your update
> or send it now ?

Please wait. I'm going to send my update today.


Thanks,

-- 
heikki

Re: [PATCHv4 0/2] USB Type-C Connector class

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]


Hi,

Guenter Roeck <linux@roeck-us.net> writes:
> On Tue, Aug 09, 2016 at 04:01:53PM +0200, Greg KH wrote:
>> On Wed, Jun 29, 2016 at 04:38:36PM +0300, Heikki Krogerus wrote:
>> > Hi,
>> > 
>> > The USB Type-C class is meant to provide unified interface to the
>> > userspace to present the USB Type-C ports in a system.
>> > 
>> > Changes since v3:
>> > - Documentation cleanup as proposed by Roger Quadros
>> > - Setting partner altmodes member to NULL on removal and fixing a
>> >   warning, as proposed by Guenter Roeck
>> > - Added the following attributes for partners and cables:
>> >   * supports_usb_power_delivery
>> >   * id_header_vdo
>> > - "id_header_vdo" is visible only when the partner or cable supports
>> >   USB Power Delivery communication.
>> > - Partner attribute "accessory" is hidden when the partner type is not
>> >   "Accessory".
>> > 
>> > Changes since v2:
>> > - Notification on role and alternate mode changes
>> > - cleanups
>> > 
>> > Changes since v1:
>> > - Completely rewrote alternate mode support
>> > - Patners, cables and cable plugs presented as devices.
>> 
>> I'm not going to take this series until everyone agrees on it, sorry.
>> I'll wait for you and Guenter and Oliver to all come up with a solution
>> that works for everyone.
>> 
>
> I have not heard from Heikko for a while. There have been some follow-up
> patches in
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=bxt-typec-pd-fixes
>
> but I am not really sure where this is going. I've been wondering if I
> can/should get more actively involved and start driving the series if
> Heikki is busy. Thoughts, anyone ?

Heikki is away on patternity leave. He should be back next Monday. Sorry
this wasn't clear, we should have notified involved folks.

best

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]