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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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.