Message-ID: <20161114123235.GD22706@kuha.fi.intel.com>
Date:   Mon, 14 Nov 2016 14:32:35 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Oliver Neukum <oneukum@...e.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Bin Gao <bin.gao@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATHCv10 1/2] usb: USB Type-C connector class

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> On Mon, Sep 19, 2016 at 02:16:56PM +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.
> > 
> > Reviewed-by: Guenter Roeck <linux@...ck-us.net>
> > Tested-by: Guenter Roeck <linux@...ck-us.net>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  218 ++++++
> >  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                   | 1075 +++++++++++++++++++++++++++
> >  include/linux/usb/typec.h                   |  252 +++++++
> >  9 files changed, 1669 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
> 
> Overall, this looks good, just a few minor comments that will require
> you to respin this...
> 
> 
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > new file mode 100644
> > index 0000000..dcca6bd
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -0,0 +1,218 @@
> > +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> > +
> > +What:		/sys/class/typec/<port>/current_data_role
> > +Date:		June 2016
> 
> It's no longer June.  I don't know why we have these dates, sorry, just
> make it be December and you should be fine.

OK

> > --- /dev/null
> > +++ b/Documentation/usb/typec.txt
> 
> We want to use .rst formats now, but this should be fine as-is for now.
> 
> > +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;
> > +}
> 
> should we make this a core sysfs function?

I already have the patch. I was planning on proposing that separately
after this series. This turned out to be something that can be used in
many drivers, so I was thinking about proposing it for at least few
other drivers.

But if you prefer, I can also introduce the function alone as new
sysfs core function in this series, and just use it in this driver.

> > +
> > +/* ------------------------------------------------------------------------- */
> > +/* Type-C Partners */
> > +
> > +static void typec_dev_release(struct device *dev)
> > +{
> > +}
> 
> Yeah, thanks to the in-kernel documentation, I now get to make fun of
> you!!!!
> 
> Please, NEVER DO THIS EVER!!!  You are trying to tell the kernel "hey,
> shut up for your stupid warning about an empty release function!"  Did
> you ever think about _why_ I made the kernel warn about that?  Don't
> think you are smarter than the kernel, you will always loose that bet...

Point taken.

> > +
> > +static ssize_t partner_usb_pd_show(struct device *dev,
> > +				   struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> > +
> > +	return sprintf(buf, "%d\n", p->usb_pd);
> > +}
> > +
> > +static struct device_attribute dev_attr_partner_usb_pd = {
> > +	.attr = {
> > +		.name = "supports_usb_power_delivery",
> > +		.mode = S_IRUGO,
> > +	},
> > +	.show = partner_usb_pd_show,
> > +};
> 
> DEVICE_ATTR_RO()?

I'm using the same attribute names with different types of devises. It
felt more wrong to use shared functions for some of the attributes
(but not all), and try to identify the device type in them, then to
simply ignore the macros and name the functions how ever I wanted. At
least it made the driver look less messy for sure.

But I'll try change these.

> > +
> > +static ssize_t
> > +partner_accessory_mode_show(struct device *dev, struct device_attribute *attr,
> > +			    char *buf)
> > +{
> > +	struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> > +
> > +	return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
> > +}
> > +
> > +static struct device_attribute dev_attr_partner_accessory = {
> > +	.attr = {
> > +		.name = "accessory_mode",
> > +		.mode = S_IRUGO,
> > +	},
> > +	.show = partner_accessory_mode_show,
> > +};
> 
> DEVICE_ATTR_RO()?
> 
> > +
> > +static struct attribute *typec_partner_attrs[] = {
> > +	&dev_attr_partner_accessory.attr,
> > +	&dev_attr_partner_usb_pd.attr,
> > +	NULL
> > +};
> > +
> > +static struct attribute_group typec_partner_group = {
> > +	.attrs = typec_partner_attrs,
> > +};
> > +
> > +static const struct attribute_group *typec_partner_groups[] = {
> > +	&typec_partner_group,
> > +	NULL
> > +};
> 
> ATTRIBUTE_GROUPS()?
> 
> 
> > +
> > +static struct device_type typec_partner_dev_type = {
> > +	.name = "typec_partner_device",
> > +	.groups = typec_partner_groups,
> > +	.release = typec_dev_release,
> > +};
> > +
> > +static int
> > +typec_add_partner(struct typec_port *port, struct typec_partner *partner)
> > +{
> > +	struct device *dev = &partner->dev;
> > +	int ret;
> > +
> > +	dev->class = &typec_class;
> > +	dev->parent = &port->dev;
> > +	dev->type = &typec_partner_dev_type;
> > +	dev_set_name(dev, "%s-partner", dev_name(&port->dev));
> > +
> > +	ret = device_register(dev);
> > +	if (ret) {
> > +		put_device(dev);
> > +		return ret;
> > +	}
> > +
> > +	port->partner = partner;
> > +	return 0;
> > +}
> > +
> > +static void typec_remove_partner(struct typec_port *port)
> > +{
> > +	WARN_ON(port->partner->alt_modes);
> 
> how can this happen?  What would a user do if it does?

I'll get rid of that.

> > +	device_unregister(&port->partner->dev);
> > +}
> > +
> > +/* ------------------------------------------------------------------------- */
> > +/* Type-C Cable Plugs */
> > +
> > +static struct device_type typec_plug_dev_type = {
> > +	.name = "typec_plug_device",
> > +	.release = typec_dev_release,
> > +};
> > +
> > +static int
> > +typec_add_plug(struct typec_port *port, struct typec_plug *plug)
> > +{
> > +	struct device *dev = &plug->dev;
> > +	char name[8];
> > +	int ret;
> > +
> > +	sprintf(name, "plug%d", plug->index);
> > +
> > +	dev->class = &typec_class;
> > +	dev->parent = &port->cable->dev;
> > +	dev->type = &typec_plug_dev_type;
> > +	dev_set_name(dev, "%s-%s", dev_name(&port->dev), name);
> > +
> > +	ret = device_register(dev);
> > +	if (ret) {
> > +		put_device(dev);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void typec_remove_plug(struct typec_plug *plug)
> > +{
> > +	WARN_ON(plug->alt_modes);
> 
> again, what can someone do with this?

I'll get rid of that too.

> > +	device_unregister(&plug->dev);
> > +}
> > +
> > +/* Type-C Cables */
> > +
> > +static ssize_t
> > +cable_active_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
> > +
> > +	return sprintf(buf, "%d\n", cable->active);
> > +}
> > +
> > +static struct device_attribute dev_attr_cable_active = {
> > +	.attr = {
> > +		.name = "active",
> > +		.mode = S_IRUGO,
> > +	},
> > +	.show = cable_active_show,
> > +};
> 
> DEVICE_ATTR_RO()
> 
> > +
> > +static ssize_t cable_usb_pd_show(struct device *dev,
> > +				 struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
> > +
> > +	return sprintf(buf, "%d\n", cable->usb_pd);
> > +}
> > +
> > +static struct device_attribute dev_attr_cable_usb_pd = {
> > +	.attr = {
> > +		.name = "supports_usb_power_delivery",
> > +		.mode = S_IRUGO,
> > +	},
> > +	.show = cable_usb_pd_show,
> > +};
> 
> DEVICE_ATTR_RO()
> 
> > +
> > +static const char * const typec_plug_types[] = {
> > +	[USB_PLUG_NONE]		= "unknown",
> > +	[USB_PLUG_TYPE_A]	= "Type-A",
> > +	[USB_PLUG_TYPE_B]	= "Type-B",
> > +	[USB_PLUG_TYPE_C]	= "Type-C",
> > +	[USB_PLUG_CAPTIVE]	= "Captive",
> > +};
> > +
> > +static ssize_t cable_plug_type_show(struct device *dev,
> > +				    struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_cable *cable = container_of(dev, struct typec_cable, dev);
> > +
> > +	return sprintf(buf, "%s\n", typec_plug_types[cable->type]);
> > +}
> > +
> > +static struct device_attribute dev_attr_plug_type = {
> > +	.attr = {
> > +		.name = "plug_type",
> > +		.mode = S_IRUGO,
> > +	},
> > +	.show = cable_plug_type_show,
> > +};
> 
> And so on...
> 
> 
> > +
> > +static struct attribute *typec_cable_attrs[] = {
> > +	&dev_attr_cable_active.attr,
> > +	&dev_attr_cable_usb_pd.attr,
> > +	&dev_attr_plug_type.attr,
> > +	NULL
> > +};
> > +
> > +static struct attribute_group typec_cable_group = {
> > +	.attrs = typec_cable_attrs,
> > +};
> > +
> > +static const struct attribute_group *typec_cable_groups[] = {
> > +	&typec_cable_group,
> > +	NULL
> > +};
> 
> ATTRIBUTE_GROUPS()?
> 
> 
> > +
> > +static struct device_type typec_cable_dev_type = {
> > +	.name = "typec_cable_device",
> > +	.groups = typec_cable_groups,
> > +	.release = typec_dev_release,
> > +};
> > +
> > +static int typec_add_cable(struct typec_port *port, struct typec_cable *cable)
> > +{
> > +	struct device *dev = &cable->dev;
> > +	int ret;
> > +
> > +	dev->class = &typec_class;
> > +	dev->parent = &port->dev;
> > +	dev->type = &typec_cable_dev_type;
> > +	dev_set_name(dev, "%s-cable", dev_name(&port->dev));
> > +
> > +	ret = device_register(dev);
> > +	if (ret) {
> > +		put_device(dev);
> > +		return ret;
> > +	}
> > +
> > +	/* Plug1 */
> > +	if (!cable->usb_pd)
> > +		return 0;
> > +
> > +	cable->plug[0].index = 1;
> > +	ret = typec_add_plug(port, &cable->plug[0]);
> > +	if (ret) {
> > +		device_unregister(dev);
> > +		return ret;
> > +	}
> > +
> > +	/* Plug2 */
> > +	if (!cable->active || !cable->sop_pp_controller)
> > +		return 0;
> > +
> > +	cable->plug[1].index = 2;
> > +	ret = typec_add_plug(port, &cable->plug[1]);
> > +	if (ret) {
> > +		typec_remove_plug(&cable->plug[0]);
> > +		device_unregister(dev);
> > +		return ret;
> > +	}
> > +
> > +	port->cable = cable;
> > +	return 0;
> > +}
> > +
> > +static void typec_remove_cable(struct typec_port *port)
> > +{
> > +	if (port->cable->active) {
> > +		typec_remove_plug(&port->cable->plug[0]);
> > +		if (port->cable->sop_pp_controller)
> > +			typec_remove_plug(&port->cable->plug[1]);
> > +	}
> > +	device_unregister(&port->cable->dev);
> > +}
> > +
> > +/* ------------------------------------------------------------------------- */
> > +/* API for the port drivers */
> > +
> > +static void typec_init_roles(struct typec_port *port)
> > +{
> > +	if (port->prefer_role < 0)
> > +		return;
> > +
> > +	if (port->prefer_role == TYPEC_SOURCE) {
> > +		port->data_role = TYPEC_HOST;
> > +		port->pwr_role = TYPEC_SOURCE;
> > +		port->vconn_role = TYPEC_SOURCE;
> > +	} else {
> > +		/* Device mode as default also by default with DRP ports */
> > +		port->data_role = TYPEC_DEVICE;
> > +		port->pwr_role = TYPEC_SINK;
> > +		port->vconn_role = TYPEC_SINK;
> > +	}
> > +}
> > +
> > +int typec_connect(struct typec_port *port, struct typec_connection *con)
> > +{
> > +	int ret;
> > +
> > +	if (!con->partner && !con->cable)
> > +		return -EINVAL;
> > +
> > +	port->connected = 1;
> > +	port->data_role = con->data_role;
> > +	port->pwr_role = con->pwr_role;
> > +	port->vconn_role = con->vconn_role;
> > +	port->pwr_opmode = con->pwr_opmode;
> > +
> > +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> 
> This worries me.  Who is listening for it?  What will you do with it?
> Shouldn't you just poll on an attribute file instead?

Oliver! Did you need this or can we remove it?

I remember I removed the "connected" attribute because you did not see
any use for it at one point. I don't remember the reason exactly why?

> > +
> > +	if (con->cable) {
> > +		ret = typec_add_cable(port, con->cable);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (con->partner) {
> > +		ret = typec_add_partner(port, con->partner);
> > +		if (ret) {
> > +			if (con->cable)
> > +				typec_remove_cable(port);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_connect);
> > +
> > +void typec_disconnect(struct typec_port *port)
> > +{
> > +	if (port->partner)
> > +		typec_remove_partner(port);
> > +
> > +	if (port->cable)
> > +		typec_remove_cable(port);
> > +
> > +	port->connected = 0;
> > +	port->partner = NULL;
> > +	port->cable = NULL;
> > +
> > +	port->pwr_opmode = TYPEC_PWR_MODE_USB;
> > +
> > +	typec_init_roles(port);
> > +
> > +	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
> > +}
> > +EXPORT_SYMBOL_GPL(typec_disconnect);
> > +
> > +/* --------------------------------------- */
> > +/* Driver callbacks to report role updates */
> > +
> > +void typec_set_data_role(struct typec_port *port, enum typec_data_role role)
> > +{
> > +	port->data_role = role;
> > +	sysfs_notify(&port->dev.kobj, NULL, "current_data_role");
> > +}
> > +EXPORT_SYMBOL(typec_set_data_role);
> 
> EXPORT_SYMBOL_GPL() everywhere please, don't mix and match for no good
> reason.

I'll fix these.

> > +static void typec_init_modes(struct typec_altmode *alt, int is_port)
> > +{
> > +	struct typec_mode *mode = alt->modes;
> > +	int i;
> > +
> > +	for (i = 0; i < alt->n_modes; i++, mode++) {
> > +		mode->alt_mode = alt;
> > +		mode->index = i;
> > +		sprintf(mode->group_name, "mode%d", i);
> > +
> > +		sysfs_attr_init(&mode->vdo_attr.attr);
> > +		mode->vdo_attr.attr.name = "vdo";
> > +		mode->vdo_attr.attr.mode = S_IRUGO;
> > +		mode->vdo_attr.show = typec_altmode_vdo_show;
> > +
> > +		sysfs_attr_init(&mode->desc_attr.attr);
> > +		mode->desc_attr.attr.name = "description";
> > +		mode->desc_attr.attr.mode = S_IRUGO;
> > +		mode->desc_attr.show = typec_altmode_desc_show;
> > +
> > +		sysfs_attr_init(&mode->active_attr.attr);
> > +		mode->active_attr.attr.name = "active";
> > +		mode->active_attr.attr.mode = S_IWUSR | S_IRUGO;
> > +		mode->active_attr.show = typec_altmode_active_show;
> > +		mode->active_attr.store = typec_altmode_active_store;
> > +
> > +		mode->attrs[0] = &mode->vdo_attr.attr;
> > +		mode->attrs[1] = &mode->desc_attr.attr;
> > +		mode->attrs[2] = &mode->active_attr.attr;
> > +
> > +		/* With ports, list the roles that the mode is supported with */
> > +		if (is_port) {
> > +			sysfs_attr_init(&mode->roles_attr.attr);
> > +			mode->roles_attr.attr.name = "supported_roles";
> > +			mode->roles_attr.attr.mode = S_IRUGO;
> > +			mode->roles_attr.show = typec_altmode_roles_show;
> > +
> > +			mode->attrs[3] = &mode->roles_attr.attr;
> > +		}
> > +
> > +		mode->group.attrs = mode->attrs;
> > +		mode->group.name = mode->group_name;
> > +
> > +		alt->mode_groups[i] = &mode->group;
> > +	}
> > +}
> 
> Ugh, dynamic attributes on the fly?  why?  Why not just use the callback
> to determine if the attribute should be shown or not?  Much simpler and
> you don't have to clean up after yourself.  Are you sure you cleaned up
> properly from these?

I'll change that.

> > +static ssize_t current_power_role_show(struct device *dev,
> > +				       struct device_attribute *attr, char *buf)
> > +{
> > +	struct typec_port *port = to_typec_port(dev);
> > +
> > +	return sprintf(buf, "%s\n", typec_roles[port->pwr_role]);
> > +}
> > +static DEVICE_ATTR_RW(current_power_role);
> 
> Nice, see, you use the macros here, be consistant please...
> 
> > +static const struct attribute_group typec_group = {
> > +	.attrs = typec_attrs,
> > +};
> > +
> > +static const struct attribute_group *typec_groups[] = {
> > +	&typec_group,
> > +	NULL,
> > +};
> 
> ATTRIBUTE_GROUP() please.
> 
> > +static int __init typec_init(void)
> > +{
> > +	return class_register(&typec_class);
> > +}
> > +subsys_initcall(typec_init);
> > +
> > +static void __exit typec_exit(void)
> > +{
> > +	class_unregister(&typec_class);
> 
> You forgot to clean up your idr :(

Sorry, what idr? The port ids get removed in typec_release().


Thanks,

-- 
heikki

Powered by blists - more mailing lists