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

Groups > linux.kernel > #1551893 > unrolled thread

[PATCHv14 0/3] USB Type-C Connector class

Back to article view | Back to linux.kernel

Contents

  [PATCHv14 0/3] USB Type-C Connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-05 12:10 +0100
    [PATCHv14 1/3] lib/string: add sysfs_match_string helper Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-05 12:10 +0100
    [PATCHv14 3/3] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-05 12:10 +0100
    Re: [PATCHv14 2/3] usb: USB Type-C connector class Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-01-05 17:00 +0100
      Re: [PATCHv14 2/3] usb: USB Type-C connector class Greg KH <gregkh@linuxfoundation.org> - 2017-01-05 17:50 +0100
      Re: [PATCHv14 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-06 12:10 +0100
        Re: [PATCHv14 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-01-06 16:50 +0100
        Re: [PATCHv14 2/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2017-01-10 11:20 +0100
          Re: [PATCHv14 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-11 09:00 +0100
            Re: [PATCHv14 2/3] usb: USB Type-C connector class Oliver Neukum <oneukum@suse.com> - 2017-01-11 10:20 +0100
    Re: [PATCHv14 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-01-09 18:00 +0100
      Re: [PATCHv14 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-10 10:00 +0100
        Re: [PATCHv14 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-01-10 15:00 +0100
          Re: [PATCHv14 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-10 15:50 +0100
            Re: [PATCHv14 2/3] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2017-01-10 18:40 +0100
              Re: [PATCHv14 2/3] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-01-11 12:10 +0100

#1551893 — [PATCHv14 0/3] USB Type-C Connector class

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 v13:
- New API. Everything is registered separately.

Changes since v12:
- Added prefer_role member to typec_capability structure as requested by Guenter

Changes since v11:
- The port drivers are responsible of removing the alternate
  modes (just like the documentation already said).

Changes since v10:
- Using ATTRIBUTE_GROUPS and DEVICE_ATTR marcos everywhere
- Moved sysfs_match_string to lib/string.c
- Rationalized uevents
- Calling ida_destroy

Changes since v9:
- Minor typec_wcove.c cleanup as proposed by Guenter Roeck. No
  function affect.

Changes since v8:
- checking sysfs_streq() result correctly in sysfs_strmatch
- fixed accessory check in supported_accessory_mode
- using "none" as the only string that can clear the preferred role

Changes since v7:
- Removed "type" attribute from partners
- Added supports_usb_power_delivery attribute for partner and cable

Changes since v6:
- current_vconn_role attr renamed to vconn_source (no API changes)
- Small documentation improvements proposed by Vincent Palatin

Changes since v5:
- Only updating the roles based on driver notifications
- Added MODULE_ALIAS for the WhiskeyCove module
- Including the patch that creates the actual platform device for the
  WhiskeyCove Type-C PHY in this series.

Changes since v4:
- Remove the port lock completely

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 (3):
  lib/string: add sysfs_match_string helper
  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 |  219 +++++
 Documentation/usb/typec.txt                 |  181 ++++
 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                   | 1190 +++++++++++++++++++++++++++
 drivers/usb/typec/typec_wcove.c             |  377 +++++++++
 include/linux/string.h                      |   10 +
 include/linux/usb/typec.h                   |  212 +++++
 lib/string.c                                |   26 +
 12 files changed, 2251 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.11.0

[toc] | [next] | [standalone]

#1551894 — [PATCHv14 1/3] lib/string: add sysfs_match_string helper

Make a simple helper for matching strings with sysfs
attribute files. In most parts the same as match_string(),
except sysfs_match_string() uses sysfs_streq() instead of
strcmp() for matching. This is more convenient when used
with sysfs attributes.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 include/linux/string.h | 10 ++++++++++
 lib/string.c           | 26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a66f83..c4011b28f3d8 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+
+/**
+ * sysfs_match_string - matches given string in an array
+ * @_a: array of strings
+ * @_s: string to match with
+ *
+ * Helper for __sysfs_match_string(). Calculates the size of @a automatically.
+ */
+#define sysfs_match_string(_a, _s) __sysfs_match_string(_a, ARRAY_SIZE(_a), _s)
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562a53ae..1a7d3fd52541 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, const char *string)
 }
 EXPORT_SYMBOL(match_string);
 
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+	const char *item;
+	int index;
+
+	for (index = 0; index < n; index++) {
+		item = array[index];
+		if (!item)
+			break;
+		if (sysfs_streq(item, str))
+			return index;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_match_string);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value
-- 
2.11.0

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

#1551896 — [PATCHv14 3/3] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

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 | 377 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 392 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 17792f9114c6..2abbcb021d1b 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB Power Delivery 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 1012a8bed6d5..b9cb862221af 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 000000000000..d5a7b21fa3f1
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,377 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY driver
+ *
+ * Copyright (C) 2017 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_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 irqreturn_t wcove_typec_irq(int irq, void *data)
+{
+	enum typec_role role = TYPEC_SINK;
+	struct typec_partner_desc partner;
+	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");
+		ret = regmap_write(wcove->regmap, USBC_IRQ1, cc_irq1);
+		if (ret)
+			goto err;
+	}
+
+	if (cc_irq2) {
+		ret = regmap_write(wcove->regmap, USBC_IRQ2, cc_irq2);
+		if (ret)
+			goto err;
+		/*
+		 * Ignoring any PD communication interrupts until the PD support
+		 * is available
+		 */
+		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->partner) {
+			typec_unregister_partner(wcove->partner);
+			wcove->partner = NULL;
+		}
+
+		wcove_typec_func(wcove, WCOVE_FUNC_ORIENTATION,
+				 WCOVE_ORIENTATION_NORMAL);
+
+		/* This makes sure the device controller is disconnected */
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
+
+		/* Port to default role */
+		typec_set_data_role(wcove->port, TYPEC_DEVICE);
+		typec_set_pwr_role(wcove->port, TYPEC_SINK);
+		typec_set_pwr_opmode(wcove->port, TYPEC_PWR_MODE_USB);
+
+		goto out;
+	}
+
+	if (wcove->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;
+	}
+
+	memset(&partner, 0, sizeof(partner));
+
+	switch (USBC_STATUS1_RSLT(status1)) {
+	case USBC_RSLT_SRC_DEFAULT:
+		typec_set_pwr_opmode(wcove->port, TYPEC_PWR_MODE_USB);
+		break;
+	case USBC_RSLT_SRC_1_5A:
+		typec_set_pwr_opmode(wcove->port, TYPEC_PWR_MODE_1_5A);
+		break;
+	case USBC_RSLT_SRC_3_0A:
+		typec_set_pwr_opmode(wcove->port, TYPEC_PWR_MODE_3_0A);
+		break;
+	case USBC_RSLT_SNK:
+		role = TYPEC_SOURCE;
+		break;
+	case USBC_RSLT_DEBUG_ACC:
+		partner.accessory = TYPEC_ACCESSORY_DEBUG;
+		break;
+	case USBC_RSLT_AUDIO_ACC:
+		partner.accessory = TYPEC_ACCESSORY_AUDIO;
+		break;
+	default:
+		dev_WARN(wcove->dev, "%s Undefined result\n", __func__);
+		goto err;
+	}
+
+	if (role == TYPEC_SINK) {
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_DEVICE);
+		typec_set_data_role(wcove->port, TYPEC_DEVICE);
+		typec_set_pwr_role(wcove->port, TYPEC_SINK);
+	} else {
+		wcove_typec_func(wcove, WCOVE_FUNC_ROLE, WCOVE_ROLE_HOST);
+		typec_set_pwr_role(wcove->port, TYPEC_SOURCE);
+		typec_set_data_role(wcove->port, TYPEC_HOST);
+	}
+
+	wcove->partner = typec_register_partner(wcove->port, &partner);
+	if (!wcove->partner)
+		dev_err(wcove->dev, "failed register partner\n");
+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;
+
+	if (!acpi_check_dsm(ACPI_HANDLE(&pdev->dev), uuid.b, 0, 0x1f)) {
+		dev_err(&pdev->dev, "Missing _DSM functions\n");
+		return -ENODEV;
+	}
+
+	wcove->cap.type = TYPEC_PORT_DRP;
+	wcove->cap.revision = USB_TYPEC_REV_1_1;
+	wcove->cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+
+	/* Make sure the PD PHY is disabled until USB PD is available */
+	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));
+
+	wcove->port = typec_register_port(&pdev->dev, &wcove->cap);
+	if (!wcove->port)
+		return -ENODEV;
+
+	/* 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_partner(wcove->partner);
+	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");
+MODULE_ALIAS("platform:bxt_wcove_usbc");
-- 
2.11.0

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

#1552077 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Thu, Jan 05, 2017 at 02:01:18PM +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.

Disclaimer: I'm not familiar with USB Type-C, so my comments are pretty
much limited to generic/stylistic issues.

Overall this looks good. Please find a couple of nitpicks below.

[snip]

> +++ b/Documentation/usb/typec.txt
> @@ -0,0 +1,181 @@
> +USB Type-C connector class
> +==========================

You might want to convert this to the new .rst format at some point.

[snip]

> +++ b/drivers/usb/typec/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB Power Delivery and Type-C drivers"

This could use a help text telling why the user wants to select this.

> +
> +config TYPEC
> +	tristate
> +
> +endmenu
> diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> new file mode 100644
> index 000000000000..1012a8bed6d5
> --- /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 000000000000..fe6571ca4d81
> --- /dev/null
> +++ b/drivers/usb/typec/typec.c
> @@ -0,0 +1,1190 @@
> +/*
> + * USB Type-C Connector Class
> + *
> + * Copyright (C) 2017, 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>
> +
> +/* XXX: Once we have a header for USB Power Delivery, this belongs there */
> +#define ALTMODE_MAX_N_MODES	7

ALTMODE_MAX_MODES looks better.

> +
> +struct typec_mode {
> +	int			index;
> +	u32			vdo;
> +	char			*desc;
> +	enum typec_port_type	roles;
> +
> +	struct typec_altmode	*alt_mode;
> +
> +	unsigned int		active:1;
> +
> +	char			group_name[6];
> +	struct attribute_group	group;
> +	struct attribute	*attrs[5];
> +	struct device_attribute vdo_attr;
> +	struct device_attribute desc_attr;
> +	struct device_attribute active_attr;
> +	struct device_attribute roles_attr;
> +};
> +
> +struct typec_altmode {
> +	struct device			dev;
> +	u16				svid;
> +	int				n_modes;
> +	struct typec_mode		modes[ALTMODE_MAX_N_MODES];
> +	const struct attribute_group	*mode_groups[ALTMODE_MAX_N_MODES];
> +};
> +
> +struct typec_plug {
> +	struct device			dev;
> +	enum typec_plug_index		index;
> +};
> +
> +struct typec_cable {
> +	struct device			dev;
> +	u16				pd_revision;
> +	enum typec_plug_type		type;
> +	u32				vdo;
> +	unsigned int			active:1;
> +};
> +
> +struct typec_partner {
> +	struct device			dev;
> +	u16				pd_revision;
> +	u32				vdo;
> +	enum typec_accessory		accessory;
> +};
> +
> +struct typec_port {
> +	unsigned int			id;
> +	struct device			dev;
> +
> +	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;
> +
> +	const struct typec_capability	*cap;
> +};
> +
> +#define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> +#define to_typec_plug(_dev_) container_of(_dev_, struct typec_plug, dev)
> +#define to_typec_cable(_dev_) container_of(_dev_, struct typec_cable, dev)
> +#define to_typec_partner(_dev_) container_of(_dev_, struct typec_partner, dev)
> +#define to_altmode(_dev_) container_of(_dev_, struct typec_altmode, dev)
> +
> +static const struct device_type typec_partner_dev_type;
> +static const struct device_type typec_cable_dev_type;
> +static const struct device_type typec_plug_dev_type;
> +static const struct device_type typec_port_dev_type;
> +
> +#define is_typec_partner(_dev_) (_dev_->type == &typec_partner_dev_type)
> +#define is_typec_cable(_dev_) (_dev_->type == &typec_cable_dev_type)
> +#define is_typec_plug(_dev_) (_dev_->type == &typec_plug_dev_type)
> +#define is_typec_port(_dev_) (_dev_->type == &typec_port_dev_type)
> +
> +static DEFINE_IDA(typec_index_ida);
> +static struct class *typec_class;
> +
> +/* Common attributes */
> +
> +static const char * const typec_accessory_modes[] = {
> +	[TYPEC_ACCESSORY_NONE]	= "None",
> +	[TYPEC_ACCESSORY_AUDIO]	= "Audio Adapter Accessory Mode",
> +	[TYPEC_ACCESSORY_DEBUG]	= "Debug Accessory Mode",
> +};
> +
> +static ssize_t usb_power_delivery_revision_show(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	u16 rev = 0;
> +
> +	if (is_typec_partner(dev)) {
> +		struct typec_partner *p = to_typec_partner(dev);
> +
> +		rev = p->pd_revision;
> +	} else if (is_typec_cable(dev)) {
> +		struct typec_cable *p = to_typec_cable(dev);
> +
> +		rev = p->pd_revision;
> +	} else if (is_typec_port(dev)) {
> +		struct typec_port *p = to_typec_port(dev);
> +
> +		rev = p->cap->pd_revision;
> +	}

Is it better to return -EINVAL or -ENODEV in case we do not find
supported type?

> +
> +	return sprintf(buf, "%d\n", (rev >> 8) & 0xff);
> +}
> +static DEVICE_ATTR_RO(usb_power_delivery_revision);
> +
> +static ssize_t
> +vdo_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	u32 vdo = 0;
> +
> +	if (is_typec_partner(dev)) {
> +		struct typec_partner *p = to_typec_partner(dev);
> +
> +		vdo = p->vdo;
> +	} else if (is_typec_cable(dev)) {
> +		struct typec_cable *p = to_typec_cable(dev);
> +
> +		vdo = p->vdo;
> +	}

Ditto.

> +
> +	return sprintf(buf, "0x%08x\n", vdo);
> +}
> +static DEVICE_ATTR_RO(vdo);
> +
> +/* ------------------------------------------------------------------------- */
> +/* Alternate Modes */
> +
> +/**
> + * typec_altmode_update_active - Report Enter/Exit mode
> + * @alt: Handle to the alternate mode
> + * @mode: Mode index
> + * @active: True when the mode has been entered
> + *
> + * If a partner or cable plug executes Enter/Exit Mode command successfully, the
> + * drivers use this routine to report the updated state of the mode.
> + */
> +void typec_altmode_update_active(struct typec_altmode *alt, int mode,
> +				 bool active)
> +{
> +	struct typec_mode *m = &alt->modes[mode];
> +	char dir[6];
> +
> +	if (m->active == active)
> +		return;
> +
> +	m->active = active;
> +	snprintf(dir, 6, "mode%d", mode);

Or instead of 6 use sizeof(dir). We are sure mode is never larger than
9, right?

> +	sysfs_notify(&alt->dev.kobj, dir, "active");
> +	kobject_uevent(&alt->dev.kobj, KOBJ_CHANGE);
> +}
> +EXPORT_SYMBOL_GPL(typec_altmode_update_active);
> +
> +/**
> + * typec_altmode2port - Alternate Mode to USB Type-C port
> + * @alt: The Alternate Mode
> + *
> + * Returns handle to the port that a cable plug or partner with @alt is
> + * connected to.
> + */
> +struct typec_port *typec_altmode2port(struct typec_altmode *alt)
> +{
> +	if (is_typec_plug(alt->dev.parent))
> +		return to_typec_port(alt->dev.parent->parent->parent);
> +	if (is_typec_partner(alt->dev.parent))
> +		return to_typec_port(alt->dev.parent->parent);
> +	if (is_typec_port(alt->dev.parent))
> +		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);
> +	int i;
> +
> +	for (i = 0; i < alt->n_modes; i++)
> +		kfree(alt->modes[i].desc);
> +	kfree(alt);
> +}
> +
> +static ssize_t
> +typec_altmode_vdo_show(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	struct typec_mode *mode = container_of(attr, struct typec_mode,
> +					       vdo_attr);
> +
> +	return sprintf(buf, "0x%08x\n", mode->vdo);
> +}
> +
> +static ssize_t
> +typec_altmode_desc_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct typec_mode *mode = container_of(attr, struct typec_mode,
> +					       desc_attr);
> +
> +	return sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
> +}
> +
> +static ssize_t
> +typec_altmode_active_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct typec_mode *mode = container_of(attr, struct typec_mode,
> +					       active_attr);
> +
> +	return sprintf(buf, "%d\n", mode->active);
> +}
> +
> +static 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;
> +
> +	if (!port->cap->activate_mode)
> +		return -EOPNOTSUPP;
> +
> +	ret = kstrtobool(buf, &activate);
> +	if (ret)
> +		return ret;
> +
> +	ret = port->cap->activate_mode(port->cap, mode->index, activate);
> +	if (ret)
> +		return ret;
> +
> +	return size;
> +}
> +
> +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);
> +	ssize_t ret;
> +
> +	switch (mode->roles) {
> +	case TYPEC_PORT_DFP:
> +		ret =  sprintf(buf, "source\n");

Extra space after '='.

> +		break;
> +	case TYPEC_PORT_UFP:
> +		ret = sprintf(buf, "sink\n");
> +		break;
> +	case TYPEC_PORT_DRP:
> +	default:
> +		ret = sprintf(buf, "source\nsink\n");

I wonder if "source sink" instead is better?  Along the lines of
/sys/power/state.

Then you can print "[source] sink" when source is selected and so on.

> +		break;
> +	}
> +	return ret;
> +}
> +
> +static inline void typec_init_modes(struct typec_altmode *alt,

inline, really?

> +				    struct typec_mode_desc *desc, bool is_port)
> +{
> +	int i;
> +
> +	for (i = 0; i < alt->n_modes; i++, desc++) {
> +		struct typec_mode *mode = &alt->modes[i];
> +
> +		/* Not considering the human readable description critical */
> +		mode->desc = kstrdup(desc->desc, GFP_KERNEL);
> +		if (desc->desc && !mode->desc)
> +			dev_err(&alt->dev, "failed to copy mode%d desc\n", i);
> +
> +		mode->alt_mode = alt;
> +		mode->vdo = desc->vdo;
> +		mode->roles = desc->roles;
> +		mode->index = desc->index;
> +		sprintf(mode->group_name, "mode%d", desc->index);
> +
> +		sysfs_attr_init(&mode->vdo_attr.attr);
> +		mode->vdo_attr.attr.name = "vdo";
> +		mode->vdo_attr.attr.mode = 0444;
> +		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 = 0444;
> +		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 = 0644;
> +		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 = 0444;
> +			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 struct typec_altmode
> +*typec_register_altmode(struct device *parent, struct typec_altmode_desc *desc)

Star belongs to the above line.

> +{
> +	struct typec_altmode *alt;
> +	int ret;
> +
> +	alt = kzalloc(sizeof(*alt), GFP_KERNEL);
> +	if (!alt)
> +		return NULL;
> +
> +	alt->svid = desc->svid;
> +	alt->n_modes = desc->n_modes;
> +	typec_init_modes(alt, desc->modes, is_typec_port(parent));
> +
> +	alt->dev.parent = parent;
> +	alt->dev.groups = alt->mode_groups;
> +	alt->dev.release = typec_altmode_release;
> +	dev_set_name(&alt->dev, "%s.svid:%04x", dev_name(parent), alt->svid);
> +
> +	ret = device_register(&alt->dev);
> +	if (ret) {
> +		int i;
> +
> +		dev_err(parent, "failed to register alternate mode (%d)\n",
> +			ret);
> +
> +		put_device(&alt->dev);
> +
> +		for (i = 0; i < alt->n_modes; i++)
> +			kfree(alt->modes[i].desc);
> +		kfree(alt);

Just checking: ->release() is not called when device_register() fails
and that's why you release memory here?

> +		return NULL;
> +	}
> +
> +	return alt;
> +}
> +
> +/**
> + * typec_unregister_altmode - Unregister Alternate Mode
> + * @alt: The alternate mode to be unregistered
> + *
> + * Unregister device created with typec_partner_register_altmode(),
> + * typec_plug_register_altmode() or typec_port_register_altmode().
> + */
> +void typec_unregister_altmode(struct typec_altmode *alt)
> +{
> +	if (alt)
> +		device_unregister(&alt->dev);
> +}
> +EXPORT_SYMBOL_GPL(typec_unregister_altmode);
> +
> +/* ------------------------------------------------------------------------- */
> +/* Type-C Partners */
> +
> +static ssize_t accessory_mode_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct typec_partner *p = to_typec_partner(dev);
> +
> +	if (p->accessory == TYPEC_ACCESSORY_NONE)
> +		return 0;
> +
> +	return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
> +}
> +static DEVICE_ATTR_RO(accessory_mode);
> +
> +static struct attribute *typec_partner_attrs[] = {
> +	&dev_attr_vdo.attr,
> +	&dev_attr_accessory_mode.attr,
> +	&dev_attr_usb_power_delivery_revision.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(typec_partner);
> +
> +static void typec_partner_release(struct device *dev)
> +{
> +	struct typec_partner *partner = to_typec_partner(dev);
> +
> +	kfree(partner);
> +}
> +
> +static const struct device_type typec_partner_dev_type = {
> +	.name = "typec_partner_device",
> +	.groups = typec_partner_groups,
> +	.release = typec_partner_release,
> +};
> +
> +/**
> + * typec_partner_register_altmode - Register USB Type-C Partner Alternate Mode
> + * @partner: USB Type-C Partner that supports the alternate mode
> + * @desc: Description of the alternate mode
> + *
> + * This routine is used to register each alternate mode individually that
> + * @partner has listed in response to Discover SVIDs command. The modes for a
> + * SVID listed in response to Discover Modes command need to be listed in an
> + * array in @desc.
> + *
> + * Returns handle to the alternate mode on success or NULL on failure.
> + */
> +struct typec_altmode
> +*typec_partner_register_altmode(struct typec_partner *partner,

Here also star belongs to the line above.

> +				struct typec_altmode_desc *desc)
> +{
> +	return typec_register_altmode(&partner->dev, desc);
> +}
> +EXPORT_SYMBOL_GPL(typec_partner_register_altmode);
> +
> +/**
> + * typec_register_partner - Register a USB Type-C Partner
> + * @port: The USB Type-C Port the partner is connected to
> + * @desc: Description of the partner
> + *
> + * Registers a device for USB Type-C Partner described in @desc.
> + *
> + * Returns handle to the partner on success or NULL on failure.
> + */
> +struct typec_partner *typec_register_partner(struct typec_port *port,
> +					     struct typec_partner_desc *desc)
> +{
> +	struct typec_partner *partner = NULL;
> +	int ret;
> +
> +	partner = kzalloc(sizeof(*partner), GFP_KERNEL);
> +	if (!partner)
> +		return NULL;
> +
> +	partner->vdo = desc->vdo;
> +	partner->accessory = desc->accessory;
> +	partner->pd_revision = desc->pd_revision;
> +
> +	partner->dev.class = typec_class;
> +	partner->dev.parent = &port->dev;
> +	partner->dev.type = &typec_partner_dev_type;
> +	dev_set_name(&partner->dev, "%s-partner", dev_name(&port->dev));
> +
> +	ret = device_register(&partner->dev);
> +	if (ret) {
> +		dev_err(&port->dev, "failed to register partner (%d)\n", ret);
> +		put_device(&partner->dev);
> +		kfree(partner);
> +		return NULL;
> +	}
> +
> +	return partner;
> +}
> +EXPORT_SYMBOL_GPL(typec_register_partner);
> +
> +/**
> + * typec_unregister_partner - Unregister a USB Type-C Partner
> + * @partner: The partner to be unregistered
> + *
> + * Unregister device created with typec_register_partner().
> + */
> +void typec_unregister_partner(struct typec_partner *partner)
> +{
> +	if (partner)
> +		device_unregister(&partner->dev);
> +}
> +EXPORT_SYMBOL_GPL(typec_unregister_partner);
> +
> +/* ------------------------------------------------------------------------- */
> +/* Type-C Cable Plugs */
> +
> +static void typec_plug_release(struct device *dev)
> +{
> +	struct typec_plug *plug = to_typec_plug(dev);
> +
> +	kfree(plug);
> +}
> +
> +static const struct device_type typec_plug_dev_type = {
> +	.name = "typec_plug_device",
> +	.release = typec_plug_release,
> +};
> +
> +/**
> + * typec_plug_register_altmode - Register USB Type-C Cable Plug Alternate Mode
> + * @plug: USB Type-C Cable Plug that supports the alternate mode
> + * @desc: Description of the alternate mode
> + *
> + * This routine is used to register each alternate mode individually that @plug
> + * has listed in response to Discover SVIDs command. The modes for a SVID that
> + * the plug lists in response to Discover Modes command need to be listed in an
> + * array in @desc.
> + *
> + * Returns handle to the alternate mode on success or NULL on failure.
> + */
> +struct typec_altmode
> +*typec_plug_register_altmode(struct typec_plug *plug,

Here also star belongs to the above line.

> +			     struct typec_altmode_desc *desc)
> +{
> +	return typec_register_altmode(&plug->dev, desc);
> +}
> +EXPORT_SYMBOL_GPL(typec_plug_register_altmode);
> +
> +/**
> + * typec_register_plug - Register a USB Type-C Cable Plug
> + * @cable: USB Type-C Cable with the plug
> + * @desc: Description of the cable plug
> + *
> + * Registers a device for USB Type-C Cable Plug described in @desc. A USB Type-C
> + * Cable Plug represents a plug with electronics in it that can response to USB
> + * Power Delivery SOP Prime or SOP Double Prime packages.
> + *
> + * Returns handle to the cable plug on success or NULL on failure.
> + */
> +struct typec_plug *typec_register_plug(struct typec_cable *cable,
> +				       struct typec_plug_desc *desc)
> +{
> +	struct typec_plug *plug = NULL;

No need to initialize plug.

> +	char name[8];
> +	int ret;
> +
> +	plug = kzalloc(sizeof(*plug), GFP_KERNEL);
> +	if (!plug)
> +		return NULL;
> +
> +	sprintf(name, "plug%d", desc->index);
> +
> +	plug->index = desc->index;
> +	plug->dev.class = typec_class;
> +	plug->dev.parent = &cable->dev;
> +	plug->dev.type = &typec_plug_dev_type;
> +	dev_set_name(&plug->dev, "%s-%s", dev_name(cable->dev.parent), name);
> +
> +	ret = device_register(&plug->dev);
> +	if (ret) {
> +		dev_err(&cable->dev, "failed to register plug (%d)\n", ret);
> +		put_device(&plug->dev);
> +		kfree(plug);
> +		return NULL;
> +	}
> +
> +	return plug;
> +}
> +EXPORT_SYMBOL_GPL(typec_register_plug);
> +
> +/**
> + * typec_unregister_plug - Unregister a USB Type-C Cable Plug
> + * @plug: The cable plug to be unregistered
> + *
> + * Unregister device created with typec_register_plug().
> + */
> +void typec_unregister_plug(struct typec_plug *plug)
> +{
> +	if (plug)
> +		device_unregister(&plug->dev);
> +}
> +EXPORT_SYMBOL_GPL(typec_unregister_plug);
> +
> +/* Type-C Cables */
> +
> +static ssize_t
> +active_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct typec_cable *cable = to_typec_cable(dev);
> +
> +	return sprintf(buf, "%d\n", cable->active);
> +}
> +static DEVICE_ATTR_RO(active);
> +
> +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 plug_type_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct typec_cable *cable = to_typec_cable(dev);
> +
> +	return sprintf(buf, "%s\n", typec_plug_types[cable->type]);
> +}
> +static DEVICE_ATTR_RO(plug_type);
> +
> +static struct attribute *typec_cable_attrs[] = {
> +	&dev_attr_active.attr,
> +	&dev_attr_plug_type.attr,
> +	&dev_attr_usb_power_delivery_revision.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(typec_cable);
> +
> +static void typec_cable_release(struct device *dev)
> +{
> +	struct typec_cable *cable = to_typec_cable(dev);
> +
> +	kfree(cable);
> +}
> +
> +static const struct device_type typec_cable_dev_type = {
> +	.name = "typec_cable_device",
> +	.groups = typec_cable_groups,
> +	.release = typec_cable_release,
> +};
> +
> +/**
> + * typec_register_cable - Register a USB Type-C Cable
> + * @port: The USB Type-C Port the cable is connected to
> + * @desc: Description of the cable
> + *
> + * Registers a device for USB Type-C Cable described in @desc. The cable will be
> + * parent for the optional cable plug devises.
> + *
> + * Returns handle to the cable on success or NULL on failure.
> + */
> +struct typec_cable *typec_register_cable(struct typec_port *port,
> +					 struct typec_cable_desc *desc)
> +{
> +	struct typec_cable *cable = NULL;

No need to initialize cable.

> +	int ret;
> +
> +	cable = kzalloc(sizeof(*cable), GFP_KERNEL);
> +	if (!cable)
> +		return NULL;
> +
> +	cable->type = desc->type;
> +	cable->vdo = desc->vdo;
> +	cable->active = desc->active;
> +	cable->pd_revision = desc->pd_revision;
> +
> +	cable->dev.class = typec_class;
> +	cable->dev.parent = &port->dev;
> +	cable->dev.type = &typec_cable_dev_type;
> +	dev_set_name(&cable->dev, "%s-cable", dev_name(&port->dev));
> +
> +	ret = device_register(&cable->dev);
> +	if (ret) {
> +		dev_err(&port->dev, "failed to register cable (%d)\n", ret);
> +		put_device(&cable->dev);
> +		kfree(cable);
> +		return NULL;
> +	}
> +
> +	return cable;
> +}

[snip]

> +/**
> + * typec_port_register_altmode - Register USB Type-C Port Alternate Mode
> + * @port: USB Type-C Port that supports the alternate mode
> + * @desc: Description of the alternate mode
> + *
> + * This routine is used to register an alternate mode that @port is capable of
> + * supporting.
> + *
> + * Returns handle to the alternate mode on success or NULL on failure.
> + */
> +struct typec_altmode
> +*typec_port_register_altmode(struct typec_port *port,

Again star.

> +			     struct typec_altmode_desc *desc)
> +{
> +	return typec_register_altmode(&port->dev, desc);
> +}
> +EXPORT_SYMBOL_GPL(typec_port_register_altmode);
> +
> +/**
> + * typec_register_port - Register a USB Type-C Port
> + * @parent: Parent device
> + * @cap: Description of the port
> + *
> + * Registers a device for USB Type-C Port described in @cap.
> + *
> + * Returns handle to the port on success or NULL on failure.
> + */
> +struct typec_port *typec_register_port(struct device *parent,
> +				       const struct typec_capability *cap)
> +{
> +	struct typec_port *port;
> +	enum typec_role role;
> +	int ret;
> +	int id;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return NULL;
> +
> +	id = ida_simple_get(&typec_index_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		kfree(port);
> +		return NULL;
> +	}
> +
> +	if (cap->type == TYPEC_PORT_DFP)
> +		role = TYPEC_SOURCE;
> +	else if (cap->type == TYPEC_PORT_UFP)
> +		role = TYPEC_SINK;
> +	else
> +		role = cap->prefer_role;
> +
> +	if (role == TYPEC_SOURCE) {
> +		port->data_role = TYPEC_HOST;
> +		port->pwr_role = TYPEC_SOURCE;
> +		port->vconn_role = TYPEC_SOURCE;
> +	} else {
> +		port->data_role = TYPEC_DEVICE;
> +		port->pwr_role = TYPEC_SINK;
> +		port->vconn_role = TYPEC_SINK;
> +	}
> +
> +	port->id = id;
> +	port->cap = cap;
> +	port->prefer_role = cap->prefer_role;
> +
> +	port->dev.type = &typec_port_dev_type;
> +	port->dev.class = typec_class;
> +	port->dev.parent = parent;
> +	dev_set_name(&port->dev, "port%d", id);
> +
> +	ret = device_register(&port->dev);
> +	if (ret) {
> +		dev_err(parent, "failed to register port (%d)\n", ret);
> +		ida_simple_remove(&typec_index_ida, id);
> +		put_device(&port->dev);
> +		kfree(port);
> +		return NULL;
> +	}
> +
> +	return port;
> +}
> +EXPORT_SYMBOL_GPL(typec_register_port);
> +
> +/**
> + * typec_unregister_port - Unregister a USB Type-C Port
> + * @port: The port to be unregistered
> + *
> + * Unregister device created with typec_register_port().
> + */
> +void typec_unregister_port(struct typec_port *port)
> +{
> +	if (port)
> +		device_unregister(&port->dev);
> +}
> +EXPORT_SYMBOL_GPL(typec_unregister_port);
> +
> +static int __init typec_init(void)
> +{
> +	typec_class = class_create(THIS_MODULE, "typec");
> +	if (IS_ERR(typec_class))
> +		return PTR_ERR(typec_class);
> +	return 0;
> +}
> +subsys_initcall(typec_init);
> +
> +static void __exit typec_exit(void)
> +{
> +	class_destroy(typec_class);
> +	ida_destroy(&typec_index_ida);
> +}
> +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 000000000000..37b996c8cb72
> --- /dev/null
> +++ b/include/linux/usb/typec.h
> @@ -0,0 +1,212 @@
> +
> +#ifndef __LINUX_USB_TYPEC_H
> +#define __LINUX_USB_TYPEC_H
> +
> +#include <linux/types.h>
> +
> +/* USB Type-C Specification releases */
> +#define USB_TYPEC_REV_1_0	0x100 /* 1.0 */
> +#define USB_TYPEC_REV_1_1	0x110 /* 1.1 */
> +#define USB_TYPEC_REV_1_2	0x120 /* 1.2 */
> +
> +struct typec_altmode;
> +struct typec_partner;
> +struct typec_cable;
> +struct typec_plug;
> +struct typec_port;
> +
> +enum typec_port_type {
> +	TYPEC_PORT_DFP,
> +	TYPEC_PORT_UFP,
> +	TYPEC_PORT_DRP,
> +};
> +
> +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,
> +};
> +
> +/*
> + * struct typec_mode_desc - Individual Mode of an Alternate Mode
> + * @index: Index of the Mode within the SVID
> + * @vdo: VDO returned by Discover Modes USB PD command
> + * @desc: Optional human readable description of the mode
> + * @roles: Only for ports. DRP if the mode is available in both roles
> + *
> + * Description of 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 has the mode being entered or not.
> + */
> +struct typec_mode_desc {
> +	int			index;
> +	u32			vdo;
> +	char			*desc;
> +	/* Only used with ports */
> +	enum typec_port_type	roles;
> +};
> +
> +/*
> + * struct typec_altmode_desc - USB Type-C Alternate Mode Descriptor
> + * @svid: Standard or Vendor ID
> + * @n_modes: Number of modes
> + * @modes: Array of modes supported by the Alternate Mode
> + *
> + * 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_desc {
> +	u16			svid;
> +	int			n_modes;
> +	struct typec_mode_desc	*modes;
> +};
> +
> +struct typec_altmode
> +*typec_partner_register_altmode(struct typec_partner *partner,

Again star.

> +				struct typec_altmode_desc *desc);
> +struct typec_altmode
> +*typec_plug_register_altmode(struct typec_plug *plug,

ditto.

> +			     struct typec_altmode_desc *desc);
> +struct typec_altmode
> +*typec_port_register_altmode(struct typec_port *port,

ditto.

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

#1552107 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Thu, Jan 05, 2017 at 05:54:02PM +0200, Mika Westerberg wrote:
> > +
> > +	ret = device_register(&alt->dev);
> > +	if (ret) {
> > +		int i;
> > +
> > +		dev_err(parent, "failed to register alternate mode (%d)\n",
> > +			ret);
> > +
> > +		put_device(&alt->dev);
> > +
> > +		for (i = 0; i < alt->n_modes; i++)
> > +			kfree(alt->modes[i].desc);
> > +		kfree(alt);
> 
> Just checking: ->release() is not called when device_register() fails
> and that's why you release memory here?

If so, that's wrong.  Please see the very good documentation for
device_register in the kernel itself, which says:

	* NOTE: _Never_ directly free @dev after calling this function, even
	* if it returned an error! Always use put_device() to give up the
	* reference initialized in this function instead.

Please read the rest of the documentation there as well, it should
answer all of these types of questions...

Sometimes I wonder why I even wrote that stuff if no one ever reads
it...

Heikki, did you get others at Intel to review this?  I don't see their
signed-off-by: on the patches.  Please use the internal Intel kernel
developer mailing list for this type of thing, I thought I said I was
going to require that before I would accept these patches after the last
round of review I did.

I'm not going to look at these until you do that, sorry.  You have
access to good resources, please use them and don't abuse the community
reviewers for basic things like this.

thanks,

greg k-h

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

#1552698 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

Hi guys,

On Thu, Jan 05, 2017 at 05:54:02PM +0200, Mika Westerberg wrote:
> > +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);
> > +	ssize_t ret;
> > +
> > +	switch (mode->roles) {
> > +	case TYPEC_PORT_DFP:
> > +		ret =  sprintf(buf, "source\n");
> 
> Extra space after '='.
> 
> > +		break;
> > +	case TYPEC_PORT_UFP:
> > +		ret = sprintf(buf, "sink\n");
> > +		break;
> > +	case TYPEC_PORT_DRP:
> > +	default:
> > +		ret = sprintf(buf, "source\nsink\n");
> 
> I wonder if "source sink" instead is better?  Along the lines of
> /sys/power/state.
> 
> Then you can print "[source] sink" when source is selected and so on.

That is more or less how I originally proposed how we list the roles
in general. I introduced the separate "current_*_role" and
"supported_*_roles" attribute files because somebody wanted them. I
don't remember the reason why they were preferred to be in separate
attribute files.

Oliver! Guenter! Do we really need to list the current and supported
roles in separate attribute files? Can't we just have the "power_role"
and "data_role" attribute files for the ports instead of the separate
"supported_*_roles" and "current_*_role", and show the current role
like Mika proposes? I definitely would prefer it that way because it
is similar style used in other places like Mike pointed out.

And since we are talking about the ABI, can we also change the listing
of the accessory mode back to just "audio" and "debug" like I
originally had it? I don't remember who and why wanted it to be
changed to "Audio Adapter Accessory Mode" and "Debug Accessory Mode",
but it differs from the style we list the other details.


Thanks,

-- 
heikki

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

#1552898 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Fri, Jan 06, 2017 at 12:54:05PM +0200, Heikki Krogerus wrote:
> Hi guys,
> 
> On Thu, Jan 05, 2017 at 05:54:02PM +0200, Mika Westerberg wrote:
> > > +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);
> > > +	ssize_t ret;
> > > +
> > > +	switch (mode->roles) {
> > > +	case TYPEC_PORT_DFP:
> > > +		ret =  sprintf(buf, "source\n");
> > 
> > Extra space after '='.
> > 
> > > +		break;
> > > +	case TYPEC_PORT_UFP:
> > > +		ret = sprintf(buf, "sink\n");
> > > +		break;
> > > +	case TYPEC_PORT_DRP:
> > > +	default:
> > > +		ret = sprintf(buf, "source\nsink\n");
> > 
> > I wonder if "source sink" instead is better?  Along the lines of
> > /sys/power/state.
> > 
> > Then you can print "[source] sink" when source is selected and so on.
> 
> That is more or less how I originally proposed how we list the roles
> in general. I introduced the separate "current_*_role" and
> "supported_*_roles" attribute files because somebody wanted them. I
> don't remember the reason why they were preferred to be in separate
> attribute files.
> 
> Oliver! Guenter! Do we really need to list the current and supported
> roles in separate attribute files? Can't we just have the "power_role"
> and "data_role" attribute files for the ports instead of the separate
> "supported_*_roles" and "current_*_role", and show the current role
> like Mika proposes? I definitely would prefer it that way because it
> is similar style used in other places like Mike pointed out.
> 
Consistency with other drivers/attribute should be preferrable,
but either way is ok with me.

> And since we are talking about the ABI, can we also change the listing
> of the accessory mode back to just "audio" and "debug" like I
> originally had it? I don't remember who and why wanted it to be
> changed to "Audio Adapter Accessory Mode" and "Debug Accessory Mode",
> but it differs from the style we list the other details.
> 

I prefer computer readable attributes over human readable,
so the change is fine with me.

Guenter

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

#1555104 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Fri, 2017-01-06 at 12:54 +0200, Heikki Krogerus wrote:
> Hi guys,
> 
> On Thu, Jan 05, 2017 at 05:54:02PM +0200, Mika Westerberg wrote:

> > I wonder if "source sink" instead is better?  Along the lines of
> > /sys/power/state.
> > 
> > Then you can print "[source] sink" when source is selected and so on.
> 
> That is more or less how I originally proposed how we list the roles
> in general. I introduced the separate "current_*_role" and
> "supported_*_roles" attribute files because somebody wanted them. I
> don't remember the reason why they were preferred to be in separate
> attribute files.

Neither do I.

> 
> Oliver! Guenter! Do we really need to list the current and supported
> roles in separate attribute files? Can't we just have the "power_role"
> and "data_role" attribute files for the ports instead of the separate
> "supported_*_roles" and "current_*_role", and show the current role
> like Mika proposes? I definitely would prefer it that way because it
> is similar style used in other places like Mike pointed out.

Either way would serve.

> And since we are talking about the ABI, can we also change the listing
> of the accessory mode back to just "audio" and "debug" like I
> originally had it? I don't remember who and why wanted it to be
> changed to "Audio Adapter Accessory Mode" and "Debug Accessory Mode",
> but it differs from the style we list the other details.

Yes, but can we differentiate analog and digital audio?

	Regards
		Oliver

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

#1556256 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Tue, Jan 10, 2017 at 11:08:51AM +0100, Oliver Neukum wrote:
> > And since we are talking about the ABI, can we also change the listing
> > of the accessory mode back to just "audio" and "debug" like I
> > originally had it? I don't remember who and why wanted it to be
> > changed to "Audio Adapter Accessory Mode" and "Debug Accessory Mode",
> > but it differs from the style we list the other details.
> 
> Yes, but can we differentiate analog and digital audio?

I guess we need to have values "analog_audio" and "digital_audio"
instead of just "audio". Is that OK?


Thanks,

-- 
heikki

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

#1556312 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Wed, 2017-01-11 at 09:57 +0200, Heikki Krogerus wrote:
> On Tue, Jan 10, 2017 at 11:08:51AM +0100, Oliver Neukum wrote:
> > > And since we are talking about the ABI, can we also change the listing
> > > of the accessory mode back to just "audio" and "debug" like I
> > > originally had it? I don't remember who and why wanted it to be
> > > changed to "Audio Adapter Accessory Mode" and "Debug Accessory Mode",
> > > but it differs from the style we list the other details.
> > 
> > Yes, but can we differentiate analog and digital audio?
> 
> I guess we need to have values "analog_audio" and "digital_audio"
> instead of just "audio". Is that OK?

Perfect.

	Regards
		Oliver

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

#1554493 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

Hello Heikki,

On Thu, Jan 05, 2017 at 02:01:18PM +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>
> ---
[ ... ]

> +
> +/**
> + * typec_register_partner - Register a USB Type-C Partner
> + * @port: The USB Type-C Port the partner is connected to
> + * @desc: Description of the partner
> + *
> + * Registers a device for USB Type-C Partner described in @desc.
> + *
> + * Returns handle to the partner on success or NULL on failure.
> + */
> +struct typec_partner *typec_register_partner(struct typec_port *port,
> +					     struct typec_partner_desc *desc)
> +{

With the changes to hide the actual partner structure, this looks at first
glance like a minor API change, but it is substantial.

Reason is that the vdo as required by typec_partner_desc is provided by a VDM
command reply, which is completely orthogonal to the PD registration process.
So far I was able to set the vdo later, after registering the connection,
and after (and if) the vdo was received.

Since the partner may not even respond to the DISCOVER_IDENT message, or not
support PD at all, this means that I would have to disconnect partner
registration from the PD protocol itself and tie it to the VDO message
exchange, with appropriate timeouts to register anyway even if the identity
was not received after some period of time or if the partner does not support
PD.

This in turn means that I'll have to re-implement and possibly re-architect
a substantial amount of code.

What is the reason for this change ? If I spend that time, I would like to
understand why it is necessary (compared to setting/updating the vdo if/when
it is available).

Thanks,
Guenter

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

#1555056 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

Hi Guenter,

On Mon, Jan 09, 2017 at 08:59:32AM -0800, Guenter Roeck wrote:
> > +/**
> > + * typec_register_partner - Register a USB Type-C Partner
> > + * @port: The USB Type-C Port the partner is connected to
> > + * @desc: Description of the partner
> > + *
> > + * Registers a device for USB Type-C Partner described in @desc.
> > + *
> > + * Returns handle to the partner on success or NULL on failure.
> > + */
> > +struct typec_partner *typec_register_partner(struct typec_port *port,
> > +					     struct typec_partner_desc *desc)
> > +{
> 
> With the changes to hide the actual partner structure, this looks at first
> glance like a minor API change, but it is substantial.
> 
> Reason is that the vdo as required by typec_partner_desc is provided by a VDM
> command reply, which is completely orthogonal to the PD registration process.
> So far I was able to set the vdo later, after registering the connection,
> and after (and if) the vdo was received.

If the identity vdo value is updated after the creation of the device,
then the user space needs to be notified separately.

> Since the partner may not even respond to the DISCOVER_IDENT message, or not
> support PD at all, this means that I would have to disconnect partner
> registration from the PD protocol itself and tie it to the VDO message
> exchange, with appropriate timeouts to register anyway even if the identity
> was not received after some period of time or if the partner does not support
> PD.
> 
> This in turn means that I'll have to re-implement and possibly re-architect
> a substantial amount of code.

We don't need to protect the structures like this, we can change this
back. But how about we introduce driver callback function for updating
the value instead, which would also notify the uses space?


Thanks,

-- 
heikki

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

#1555360 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On 01/10/2017 12:54 AM, Heikki Krogerus wrote:
> Hi Guenter,
>
> On Mon, Jan 09, 2017 at 08:59:32AM -0800, Guenter Roeck wrote:
>>> +/**
>>> + * typec_register_partner - Register a USB Type-C Partner
>>> + * @port: The USB Type-C Port the partner is connected to
>>> + * @desc: Description of the partner
>>> + *
>>> + * Registers a device for USB Type-C Partner described in @desc.
>>> + *
>>> + * Returns handle to the partner on success or NULL on failure.
>>> + */
>>> +struct typec_partner *typec_register_partner(struct typec_port *port,
>>> +					     struct typec_partner_desc *desc)
>>> +{
>>
>> With the changes to hide the actual partner structure, this looks at first
>> glance like a minor API change, but it is substantial.
>>
>> Reason is that the vdo as required by typec_partner_desc is provided by a VDM
>> command reply, which is completely orthogonal to the PD registration process.
>> So far I was able to set the vdo later, after registering the connection,
>> and after (and if) the vdo was received.
>
> If the identity vdo value is updated after the creation of the device,
> then the user space needs to be notified separately.
>
>> Since the partner may not even respond to the DISCOVER_IDENT message, or not
>> support PD at all, this means that I would have to disconnect partner
>> registration from the PD protocol itself and tie it to the VDO message
>> exchange, with appropriate timeouts to register anyway even if the identity
>> was not received after some period of time or if the partner does not support
>> PD.
>>
>> This in turn means that I'll have to re-implement and possibly re-architect
>> a substantial amount of code.
>
> We don't need to protect the structures like this, we can change this
> back. But how about we introduce driver callback function for updating
> the value instead, which would also notify the uses space?
>

That would work.

Thanks,
Guenter

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

#1555504 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Tue, Jan 10, 2017 at 05:50:04AM -0800, Guenter Roeck wrote:
> On 01/10/2017 12:54 AM, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Mon, Jan 09, 2017 at 08:59:32AM -0800, Guenter Roeck wrote:
> > > > +/**
> > > > + * typec_register_partner - Register a USB Type-C Partner
> > > > + * @port: The USB Type-C Port the partner is connected to
> > > > + * @desc: Description of the partner
> > > > + *
> > > > + * Registers a device for USB Type-C Partner described in @desc.
> > > > + *
> > > > + * Returns handle to the partner on success or NULL on failure.
> > > > + */
> > > > +struct typec_partner *typec_register_partner(struct typec_port *port,
> > > > +					     struct typec_partner_desc *desc)
> > > > +{
> > > 
> > > With the changes to hide the actual partner structure, this looks at first
> > > glance like a minor API change, but it is substantial.
> > > 
> > > Reason is that the vdo as required by typec_partner_desc is provided by a VDM
> > > command reply, which is completely orthogonal to the PD registration process.
> > > So far I was able to set the vdo later, after registering the connection,
> > > and after (and if) the vdo was received.
> > 
> > If the identity vdo value is updated after the creation of the device,
> > then the user space needs to be notified separately.
> > 
> > > Since the partner may not even respond to the DISCOVER_IDENT message, or not
> > > support PD at all, this means that I would have to disconnect partner
> > > registration from the PD protocol itself and tie it to the VDO message
> > > exchange, with appropriate timeouts to register anyway even if the identity
> > > was not received after some period of time or if the partner does not support
> > > PD.
> > > 
> > > This in turn means that I'll have to re-implement and possibly re-architect
> > > a substantial amount of code.
> > 
> > We don't need to protect the structures like this, we can change this
> > back. But how about we introduce driver callback function for updating
> > the value instead, which would also notify the uses space?
> > 
> 
> That would work.

OK, cool.

I guess we might as well then split the VDO into header, cert stat and
product parts. What do you think?

If it's OK, then should we change that file to "identity" and dump the
whole response from Discover Identity command in hex (minus VDM
Header), separate the parts in the output, or simply provide separate
attribute files for each part?

Just as a reminder, the user space can't rely on that attribute file.
We still can't get any of that information from UCSI or for example
the Thunderbolt controllers, which is annoying, but I guess it does
not matter.

An other question:
I would like to hide the attribute file(s) when the partner does not
support USB Power Delivery. Is it OK with you guys?


Thanks,

-- 
heikki

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

#1555757 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

On Tue, Jan 10, 2017 at 04:46:12PM +0200, Heikki Krogerus wrote:
> On Tue, Jan 10, 2017 at 05:50:04AM -0800, Guenter Roeck wrote:
> > On 01/10/2017 12:54 AM, Heikki Krogerus wrote:
> > > Hi Guenter,
> > > 
> > > On Mon, Jan 09, 2017 at 08:59:32AM -0800, Guenter Roeck wrote:
> > > > > +/**
> > > > > + * typec_register_partner - Register a USB Type-C Partner
> > > > > + * @port: The USB Type-C Port the partner is connected to
> > > > > + * @desc: Description of the partner
> > > > > + *
> > > > > + * Registers a device for USB Type-C Partner described in @desc.
> > > > > + *
> > > > > + * Returns handle to the partner on success or NULL on failure.
> > > > > + */
> > > > > +struct typec_partner *typec_register_partner(struct typec_port *port,
> > > > > +					     struct typec_partner_desc *desc)
> > > > > +{
> > > > 
> > > > With the changes to hide the actual partner structure, this looks at first
> > > > glance like a minor API change, but it is substantial.
> > > > 
> > > > Reason is that the vdo as required by typec_partner_desc is provided by a VDM
> > > > command reply, which is completely orthogonal to the PD registration process.
> > > > So far I was able to set the vdo later, after registering the connection,
> > > > and after (and if) the vdo was received.
> > > 
> > > If the identity vdo value is updated after the creation of the device,
> > > then the user space needs to be notified separately.
> > > 
> > > > Since the partner may not even respond to the DISCOVER_IDENT message, or not
> > > > support PD at all, this means that I would have to disconnect partner
> > > > registration from the PD protocol itself and tie it to the VDO message
> > > > exchange, with appropriate timeouts to register anyway even if the identity
> > > > was not received after some period of time or if the partner does not support
> > > > PD.
> > > > 
> > > > This in turn means that I'll have to re-implement and possibly re-architect
> > > > a substantial amount of code.
> > > 
> > > We don't need to protect the structures like this, we can change this
> > > back. But how about we introduce driver callback function for updating
> > > the value instead, which would also notify the uses space?
> > > 
> > 
> > That would work.
> 
> OK, cool.
> 
> I guess we might as well then split the VDO into header, cert stat and
> product parts. What do you think?
> 
> If it's OK, then should we change that file to "identity" and dump the
> whole response from Discover Identity command in hex (minus VDM
> Header), separate the parts in the output, or simply provide separate
> attribute files for each part?
> 
Makes me wonder what you expect in the vdo attribute today. Right now
I copy the value from the identity header into it.

Personally I would prefer separate attributes. identity, cert_stat,
product, maybe ?

> Just as a reminder, the user space can't rely on that attribute file.
> We still can't get any of that information from UCSI or for example
> the Thunderbolt controllers, which is annoying, but I guess it does
> not matter.
> 
> An other question:
> I would like to hide the attribute file(s) when the partner does not
> support USB Power Delivery. Is it OK with you guys?
> 
Ok with me.

Thanks,
Guenter

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

#1556402 — Re: [PATCHv14 2/3] usb: USB Type-C connector class

Hi Guenter,

On Tue, Jan 10, 2017 at 09:35:42AM -0800, Guenter Roeck wrote:
> > I guess we might as well then split the VDO into header, cert stat and
> > product parts. What do you think?
> > 
> > If it's OK, then should we change that file to "identity" and dump the
> > whole response from Discover Identity command in hex (minus VDM
> > Header), separate the parts in the output, or simply provide separate
> > attribute files for each part?
> > 
> Makes me wonder what you expect in the vdo attribute today. Right now
> I copy the value from the identity header into it.
> 
> Personally I would prefer separate attributes. identity, cert_stat,
> product, maybe ?

OK.

I'm sorry for bringing this up so late, but I'm still a bit concerned
about having those attributes because we can't provide a value for
them in every system even when USB power delivery is supported. This
will be the case with at least with UCSI systems.

So what I'll do is, I'll group them into separate directory that will
not be visible unless those values can actually be made available. We
will have "supports_usb_power_delivery" attribute out side that
directory.

We can name that directory "identity" or we can call it for example
"usb_power_delivery" and have something else usb pd specific in it as
well if you like, like attribute for capability. Let me know what you
think.

I proposed putting the usb pd details into separate directory already
at one point, but not everybody liked the idea if I remember
correctly. I don't think there were any real arguments against it, so
this time I'll add the group unless you disagree.


Thanks,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web