Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus]

Hi guys,

I'm attaching a diff instead of full v3. I'm not yet adding attributes
for the reset and cable_reset. I still don't understand what is the
case where the userspace would need to be able to tricker reset? Why
isn't it enough for the userspace to be able to enter/exit modes?
Oliver! Can you please comment?

Guenter, I removed the driver_data you proposed and changed the API
so that the struct typec_capability is passed to the function pointers
instead of struct typec_port. The driver_data pointer felt a bit
clumsy to me, as it was something extra that always had to be passed
to every function. Let me know if it's not OK for you.

I also made a change to the partner devices so that they always have
the port as their parent. That will have an effect on the location
where the partner device is exposed in sysfs (so now always under the
port). And because of that, I would like to get an OK from you guys.

I'm a bit concerned about the current API for adding the alternate
modes. Since we are passing the parent for an alternate mode as
struct device, it makes it possible for the caller to place it into
some wrong place. But I guess we can change the API even later.

We also need to decide how the alternate modes a port support are
exposed to the userspace. Do we just assume the port drivers will
create them as devices under the port device itself, just like
alternate modes of partners and cable plugs are exposed under the
partners and cable plugs? That works for me, but again, the class does
not have any effect on that, and it will be just a guideline. Maybe
we can add some kind of helpers and force the port drivers to use
them.

And finally, mostly as a reminder for myself. I didn't yet add
attributes for Try.SRC/SNK. So can we make it something like
"preferred_role" like I think you proposed Guenter? The
"current_data_role" I'm dropping.

So to summarize. There are still open questions regarding the required
attributes and attribute names and locations. Let's agree on those
quickly and then we can polish the patch.


Thanks,

-- 
heikki
diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
index 6836e97..41ad955 100644
--- a/drivers/usb/type-c/typec.c
+++ b/drivers/usb/type-c/typec.c
@@ -27,8 +27,6 @@ struct typec_port {
 	struct typec_partner	*partner;
 	struct typec_cable	*cable;
 
-	void			*driver_data;
-
 	unsigned int		connected:1;
 
 	int			n_altmode;
@@ -101,29 +99,10 @@ static int
 typec_add_partner(struct typec_port *port, struct typec_partner *partner)
 {
 	struct device *dev = &partner->dev;
-	struct device *parent;
 	int ret;
 
-	/*
-	 * REVISIT: Maybe it would be better to make the port always as the
-	 * parent of the partner? Or not even that. Would it be enough to just
-	 * create the symlink to the partner like we do below in any case?
-	 */
-	if (port->cable) {
-		if (port->cable->active) {
-			if (port->cable->sop_pp_controller)
-				parent = &port->cable->plug[1].dev;
-			else
-				parent = &port->cable->plug[0].dev;
-		} else {
-			parent = &port->cable->dev;
-		}
-	} else {
-		parent = &port->dev;
-	}
-
 	dev->class = &typec_class;
-	dev->parent = parent;
+	dev->parent = &port->dev;
 	dev->type = &typec_partner_dev_type;
 	dev_set_name(dev, "%s-partner", dev_name(&port->dev));
 
@@ -133,18 +112,6 @@ typec_add_partner(struct typec_port *port, struct typec_partner *partner)
 		return ret;
 	}
 
-	ret = typec_register_altmodes(dev, partner->alt_modes);
-	if (ret) {
-		device_unregister(dev);
-		return ret;
-	}
-
-	/* REVISIT: Creating symlink for the port device for now. */
-	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, "partner");
-	if (ret)
-		dev_WARN(&port->dev, "failed to create link to %s (%d)\n",
-			 dev_name(dev), ret);
-
 	port->partner = partner;
 	return 0;
 }
@@ -184,12 +151,6 @@ typec_add_plug(struct typec_port *port, struct typec_plug *plug)
 		return ret;
 	}
 
-	ret = typec_register_altmodes(dev, plug->alt_modes);
-	if (ret) {
-		device_unregister(dev);
-		return ret;
-	}
-
 	/* REVISIT: Is this useful? */
 	ret = sysfs_create_link(&port->dev.kobj, &dev->kobj, name);
 	if (ret)
@@ -278,7 +239,6 @@ static int typec_add_cable(struct typec_port *port, struct typec_cable *cable)
 	int ret;
 
 	dev->class = &typec_class;
-	/* REVISIT: We could have just the symlink also for the cable. */
 	dev->parent = &port->dev;
 	dev->type = &typec_cable_dev_type;
 	dev_set_name(dev, "%s-cable", dev_name(&port->dev));
@@ -631,7 +591,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
 	else
 		return -EINVAL;
 
-	ret = port->cap->dr_set(port, port->driver_data, role);
+	ret = port->cap->dr_set(port->cap, role);
 	if (ret)
 		return ret;
 
@@ -667,46 +627,6 @@ static ssize_t supported_data_roles_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(supported_data_roles);
 
-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_data_role role;
-	int ret;
-
-	if (port->cap->role != TYPEC_PORT_DRP)
-		return -EOPNOTSUPP;
-
-	if (!port->cap->fix_role)
-		return -EOPNOTSUPP;
-
-	if (!strcmp(buf, "DFP"))
-		role = TYPEC_PORT_DFP;
-	else if (!strcmp(buf, "UFP"))
-		role = TYPEC_PORT_UFP;
-	else if (!strcmp(buf, "DRP"))
-		role = TYPEC_PORT_DRP;
-	else
-		return -EINVAL;
-
-	ret = port->cap->fix_role(port, port->driver_data, role);
-	if (ret)
-		return ret;
-
-	return size;
-}
-
-static ssize_t
-current_data_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_data_roles[port->fixed_role]);
-}
-static DEVICE_ATTR_RW(current_data_role);
-
 static ssize_t current_power_role_store(struct device *dev,
 					struct device_attribute *attr,
 					const char *buf, size_t size)
@@ -737,7 +657,7 @@ static ssize_t current_power_role_store(struct device *dev,
 	else
 		return -EINVAL;
 
-	ret = port->cap->pr_set(port, port->driver_data, role);
+	ret = port->cap->pr_set(port->cap, role);
 	if (ret)
 		return ret;
 
@@ -816,7 +736,7 @@ static ssize_t current_vconn_role_store(struct device *dev,
 	else
 		return -EINVAL;
 
-	ret = port->cap->vconn_set(port, port->driver_data, role);
+	ret = port->cap->vconn_set(port->cap, role);
 	if (ret)
 		return ret;
 
@@ -870,7 +790,6 @@ static ssize_t supports_usb_power_delivery_show(struct device *dev,
 static DEVICE_ATTR_RO(supports_usb_power_delivery);
 
 static struct attribute *typec_attrs[] = {
-	&dev_attr_current_data_role.attr,
 	&dev_attr_current_power_role.attr,
 	&dev_attr_current_vconn_role.attr,
 	&dev_attr_current_usb_data_role.attr,
@@ -902,6 +821,10 @@ static struct attribute *altmode_attrs[] = {
 	NULL,
 };
 
+/*
+ * FIXME: Now this group is useless, but is there any use for the
+ * number_of_alternate_modes attribute?
+ */
 static const struct attribute_group altmode_group = {
 	.name = "supported_alternate_modes",
 	.attrs = altmode_attrs,
@@ -940,8 +863,7 @@ static struct device_type typec_port_dev_type = {
 };
 
 struct typec_port *typec_register_port(struct device *dev,
-				       struct typec_capability *cap,
-				       void *driver_data)
+				       const struct typec_capability *cap)
 {
 	struct typec_port *port;
 	int ret;
@@ -959,7 +881,6 @@ struct typec_port *typec_register_port(struct device *dev,
 
 	port->id = id;
 	port->cap = cap;
-	port->driver_data = driver_data;
 	port->dev.type = &typec_port_dev_type;
 	port->dev.class = &typec_class;
 	port->dev.parent = dev;
@@ -978,28 +899,6 @@ struct typec_port *typec_register_port(struct device *dev,
 		return ERR_PTR(ret);
 	}
 
-	/*
-	 * The alternate modes that the port supports must be created before
-	 * registering the port. They are just linked to the port here.
-	 */
-	if (cap->alt_modes) {
-		struct typec_altmode *alt;
-
-		for (alt = cap->alt_modes; alt->svid; alt++) {
-			ret = sysfs_add_link_to_group(&port->dev.kobj,
-						"supported_alternate_modes",
-						&alt->dev.kobj,
-						alt->name ? alt->name :
-						dev_name(&alt->dev));
-			if (ret) {
-				dev_WARN(&port->dev,
-					 "failed to create sysfs symlink\n");
-			} else {
-				port->n_altmode++;
-			}
-		}
-	}
-
 	return port;
 }
 EXPORT_SYMBOL_GPL(typec_register_port);
@@ -1009,15 +908,6 @@ void typec_unregister_port(struct typec_port *port)
 	if (port->connected)
 		typec_disconnect(port);
 
-	if (port->cap->alt_modes) {
-		struct typec_altmode *alt;
-
-		for (alt = port->cap->alt_modes; alt->svid; alt++)
-			sysfs_remove_link_from_group(&port->dev.kobj,
-						     "alternate_modes",
-						     alt->name ? alt->name :
-						     dev_name(&alt->dev));
-	}
 	device_unregister(&port->dev);
 }
 EXPORT_SYMBOL_GPL(typec_unregister_port);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index d16a38d..390e0e4 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -182,14 +182,18 @@ struct typec_capability {
 	unsigned int		audio_accessory:1;
 	unsigned int		debug_accessory:1;
 
-	int			(*fix_role)(struct typec_port *, void *,
+	/* FIXME: change to prefer_role? */
+	int			(*fix_role)(const struct typec_capability *,
 					    enum typec_data_role);
 
-	int			(*dr_set)(struct typec_port *, void *, enum typec_usb_role);
-	int			(*pr_set)(struct typec_port *, void *, enum typec_pwr_role);
-	int			(*vconn_set)(struct typec_port *, void *, enum typec_pwr_role);
+	int			(*dr_set)(const struct typec_capability *,
+					  enum typec_usb_role);
+	int			(*pr_set)(const struct typec_capability *,
+					  enum typec_pwr_role);
+	int			(*vconn_set)(const struct typec_capability *,
+					     enum typec_pwr_role);
 
-	int			(*activate_mode)(struct typec_altmode *, void *,
+	int			(*activate_mode)(struct typec_altmode *,
 						 int mode, int activate);
 };
 
@@ -217,8 +221,7 @@ struct typec_connection {
 };
 
 struct typec_port *typec_register_port(struct device *dev,
-				       struct typec_capability *cap,
-				       void *driver_data);
+				       const struct typec_capability *cap);
 void typec_unregister_port(struct typec_port *port);
 
 int typec_connect(struct typec_port *port, struct typec_connection *con);