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

Groups > linux.kernel > #1464491 > unrolled thread

[PATCH v5 0/2] USB Type-C Connector class

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/2] USB Type-C Connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-17 12:40 +0200
    [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-17 12:40 +0200
      Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-08-17 15:00 +0200
        Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove  PMIC USB Type-C PHY Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-17 15:40 +0200
        Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove  PMIC USB Type-C PHY Peter Chen <hzpeterchen@gmail.com> - 2016-08-18 02:20 +0200
          Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-08-18 08:40 +0200
            Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC  USB Type-C PHY Guenter Roeck <linux@roeck-us.net> - 2016-08-18 15:40 +0200
    Re: [PATCH v5 1/2] usb: USB Type-C connector class Frans Klaver <fransklaver@gmail.com> - 2016-08-17 15:20 +0200
      Re: [PATCH v5 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-17 16:00 +0200
        Re: [PATCH v5 1/2] usb: USB Type-C connector class Frans Klaver <fransklaver@gmail.com> - 2016-08-17 16:10 +0200
    Re: [PATCH v5 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-17 20:00 +0200
      Re: [PATCH v5 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-18 12:50 +0200
    Re: [PATCH v5 1/2] usb: USB Type-C connector class Guenter Roeck <linux@roeck-us.net> - 2016-08-17 20:00 +0200
      Re: [PATCH v5 1/2] usb: USB Type-C connector class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-08-18 12:50 +0200

#1464491 — [PATCH v5 0/2] USB Type-C Connector class

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 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 (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 |  199 +++++
 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                   | 1104 +++++++++++++++++++++++++++
 drivers/usb/typec/typec_wcove.c             |  371 +++++++++
 include/linux/usb/typec.h                   |  260 +++++++
 10 files changed, 2073 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

[toc] | [next] | [standalone]

#1464495 — [PATCH v5 2/2] 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 | 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

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

#1464564 — Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

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

Hi,

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:

[...]

> +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");

you need to add:

MODULE_ALIAS("platform:bxt_wcove_usbc");

otherwise this won't probe() automagically.

-- 
balbi

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

#1464585 — Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

On Wed, Aug 17, 2016 at 03:53:24PM +0300, Felipe Balbi wrote:
> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> 
> [...]
> 
> > +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");
> 
> you need to add:
> 
> MODULE_ALIAS("platform:bxt_wcove_usbc");
> 
> otherwise this won't probe() automagically.

I'll fix it.


Thanks,

-- 
heikki

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

#1464855 — Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

On Wed, Aug 17, 2016 at 03:53:24PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
> 
> [...]
> 
> > +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");
> 
> you need to add:
> 
> MODULE_ALIAS("platform:bxt_wcove_usbc");
> 
> otherwise this won't probe() automagically.
> 

What do you mean, Felipe? If there is a platform device named
"bxt_wcove_usbc", why it will not be probed?

-- 

Best Regards,
Peter Chen

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

#1464940 — Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

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

Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
> On Wed, Aug 17, 2016 at 03:53:24PM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
>> 
>> [...]
>> 
>> > +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");
>> 
>> you need to add:
>> 
>> MODULE_ALIAS("platform:bxt_wcove_usbc");
>> 
>> otherwise this won't probe() automagically.
>> 
>
> What do you mean, Felipe? If there is a platform device named
> "bxt_wcove_usbc", why it will not be probed?

IIRC default module alias is added based on file name, not
platform_driver name, so this would be
MODULE_ALIAS("platform:typec_wcove") by default... I could be wrong,
really don't remember the details of  that now.

-- 
balbi

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

#1465349 — Re: [PATCH v5 2/2] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

On 08/17/2016 11:33 PM, Felipe Balbi wrote:
>
> Hi,
>
> Peter Chen <hzpeterchen@gmail.com> writes:
>> On Wed, Aug 17, 2016 at 03:53:24PM +0300, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:
>>>
>>> [...]
>>>
>>>> +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");
>>>
>>> you need to add:
>>>
>>> MODULE_ALIAS("platform:bxt_wcove_usbc");
>>>
>>> otherwise this won't probe() automagically.
>>>
>>
>> What do you mean, Felipe? If there is a platform device named
>> "bxt_wcove_usbc", why it will not be probed?
>
> IIRC default module alias is added based on file name, not
> platform_driver name, so this would be
> MODULE_ALIAS("platform:typec_wcove") by default... I could be wrong,
> really don't remember the details of  that now.
>
AFAIK one either needs MODULE_ALIAS or MODULE_DEVICE_TABLE for platform
device autoloading. I have seen many patches along the line of "Add missing
MODULE_ALIAS", so I'd be a bit surprised if that was no longer needed.

Guenter

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

#1464576 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

On Wed, Aug 17, 2016 at 12:34 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> 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 |  199 +++++
>  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                   | 1104 +++++++++++++++++++++++++++
>  include/linux/usb/typec.h                   |  260 +++++++
>  9 files changed, 1687 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..e6179d3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,199 @@
> +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.
> +
> +
> +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
> +
> +
> +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 45c9848..9e64ac2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12305,6 +12305,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..52a0431
> --- /dev/null
> +++ b/drivers/usb/typec/typec.c
> @@ -0,0 +1,1104 @@
> +/*
> + * 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;
> +
> +       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",
> +};
> +
> +static int sysfs_strmatch(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;
> +}
> +
> +/* ------------------------------------------------------------------------- */
> +/* 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,
> +};

Why not use DEVICE_ATTR_RO() for this?

> +
> +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,
> +};

And this

Thanks,
Frans

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

#1464595 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

Hi,

On Wed, Aug 17, 2016 at 03:14:03PM +0200, Frans Klaver wrote:
> On Wed, Aug 17, 2016 at 12:34 PM, Heikki Krogerus
> > +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,
> > +};
> 
> Why not use DEVICE_ATTR_RO() for this?

Because I don't want to tie the attribute names to the function names
in this case. There are other *type* attributes being created in the
driver, so type_show() is not good, and we can't name the attribute
"partner_type". The attribute will be placed in group named "partner".

> > +
> > +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,
> > +};
> 
> And this

Ditto.


Thanks,

-- 
heikki

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

#1464597 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

On Wed, Aug 17, 2016 at 3:53 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> Hi,
>
> On Wed, Aug 17, 2016 at 03:14:03PM +0200, Frans Klaver wrote:
>> On Wed, Aug 17, 2016 at 12:34 PM, Heikki Krogerus
>> > +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,
>> > +};
>>
>> Why not use DEVICE_ATTR_RO() for this?
>
> Because I don't want to tie the attribute names to the function names
> in this case. There are other *type* attributes being created in the
> driver, so type_show() is not good, and we can't name the attribute
> "partner_type". The attribute will be placed in group named "partner".
>

Ah, makes sense.

Thanks,
Frans

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

#1464694 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

On Wed, Aug 17, 2016 at 01:34:40PM +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>
> ---
[ ... ]

> +
> +static ssize_t current_power_role_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	enum typec_role role;
> +	int ret = size;
> +
> +	if (!port->cap->usb_pd) {
> +		dev_dbg(dev, "power role swap only supported with USB PD\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->cap->pr_set) {
> +		dev_dbg(dev, "power role swapping not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> +		dev_dbg(dev, "partner unable to swap power role\n");
> +		return -EIO;
> +	}
> +
> +	if (!port->connected)
> +		return size;
> +
> +	ret = sysfs_strmatch(typec_roles, ARRAY_SIZE(typec_roles), buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	role = ret;
> +
> +	ret = port->cap->pr_set(port->cap, role);

	if (ret)
missing.

> +		return ret;

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

#1465120 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

On Wed, Aug 17, 2016 at 10:58:40AM -0700, Guenter Roeck wrote:
> On Wed, Aug 17, 2016 at 01:34:40PM +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>
> > ---
> [ ... ]
> 
> > +
> > +static ssize_t current_power_role_store(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf, size_t size)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	enum typec_role role;
> > +	int ret = size;
> > +
> > +	if (!port->cap->usb_pd) {
> > +		dev_dbg(dev, "power role swap only supported with USB PD\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (!port->cap->pr_set) {
> > +		dev_dbg(dev, "power role swapping not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
> > +		dev_dbg(dev, "partner unable to swap power role\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (!port->connected)
> > +		return size;
> > +
> > +	ret = sysfs_strmatch(typec_roles, ARRAY_SIZE(typec_roles), buf);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	role = ret;
> > +
> > +	ret = port->cap->pr_set(port->cap, role);
> 
> 	if (ret)
> missing.

It also seems to be missing from current_vconn_role_store().. How have
I managed to do that?

Thanks for catching this.

-- 
heikki

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

#1464695 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

On Wed, Aug 17, 2016 at 01:34:40PM +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>
> ---
[ ... ]
> +
> +static ssize_t
> +current_data_role_store(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t size)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +	enum typec_role role;
> +	int ret;
> +
> +	if (port->cap->type != TYPEC_PORT_DRP) {
> +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->cap->dr_set) {
> +		dev_dbg(dev, "data role swapping not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->connected)
> +		return size;

I don't think this check should be here. The connection status can change after
the connection status was checked. We should leave it up to the driver to
perform the necessary checks.

This also applies to the other role change store functions.

Thanks,
Guenter

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

#1465122 — Re: [PATCH v5 1/2] usb: USB Type-C connector class

On Wed, Aug 17, 2016 at 10:53:11AM -0700, Guenter Roeck wrote:
> On Wed, Aug 17, 2016 at 01:34:40PM +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>
> > ---
> [ ... ]
> > +
> > +static ssize_t
> > +current_data_role_store(struct device *dev, struct device_attribute *attr,
> > +			const char *buf, size_t size)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +	enum typec_role role;
> > +	int ret;
> > +
> > +	if (port->cap->type != TYPEC_PORT_DRP) {
> > +		dev_dbg(dev, "data role swap only supported with DRP ports\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (!port->cap->dr_set) {
> > +		dev_dbg(dev, "data role swapping not supported\n");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (!port->connected)
> > +		return size;
> 
> I don't think this check should be here. The connection status can change after
> the connection status was checked. We should leave it up to the driver to
> perform the necessary checks.
> 
> This also applies to the other role change store functions.

OK.

Thanks,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web