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

Groups > linux.kernel > #1403714 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-19 14:50 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-19 17:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Greg KH <gregkh@linuxfoundation.org> - 2016-05-19 17:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 13:00 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-19 17:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 13:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-20 15:50 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-21 08:00 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-21 08:50 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-22 18:00 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 07:40 +0200
                  Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 15:30 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 16:10 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 16:50 +0200
                        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 18:00 +0200
                          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 19:00 +0200
                            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 12:10 +0200
                              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 12:30 +0200
                                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 13:10 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-19 20:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-20 12:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-20 19:10 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-23 11:30 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-20 16:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-23 12:00 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-23 13:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-23 19:10 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 11:10 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-24 11:40 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-24 15:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 17:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 09:40 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-24 15:50 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:40 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 15:20 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-24 21:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 14:00 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 15:30 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 16:10 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-25 16:30 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-25 17:10 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 09:30 +0200
    [RFC PATCH] usb: typec: Various API updates and fixes Guenter Roeck <linux@roeck-us.net> - 2016-05-25 20:40 +0200
      Re: [RFC PATCH] usb: typec: Various API updates and fixes Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 10:00 +0200
        Re: [RFC PATCH] usb: typec: Various API updates and fixes Guenter Roeck <linux@roeck-us.net> - 2016-05-27 16:10 +0200
          Re: [RFC PATCH] usb: typec: Various API updates and fixes Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-30 14:50 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-30 15:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-30 16:10 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-31 10:40 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-05-31 11:00 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-31 14:10 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-31 14:50 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-31 19:30 +0200
                  Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-01 10:30 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-01 10:40 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-01 11:10 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-01 15:40 +0200
                        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-02 08:30 +0200
                          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-02 08:40 +0200
                            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-02 09:50 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-05-31 19:20 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-01 11:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-02 01:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-02 08:40 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-02 10:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-02 12:20 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-02 18:20 +0200
            Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-03 15:30 +0200
              Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-03 16:00 +0200
                Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-03 17:20 +0200
                  Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-03 20:50 +0200
                    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-06 15:30 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-06 15:40 +0200
                        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-07 10:30 +0200
                      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-07 19:00 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-02 10:10 +0200
    Re: [RFC PATCHv2] usb: USB Type-C Connector Class Pavel Machek <pavel@ucw.cz> - 2016-06-03 22:30 +0200
      Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-06 15:50 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Greg KH <gregkh@linuxfoundation.org> - 2016-06-06 16:50 +0200
          Re: [RFC PATCHv2] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-07 10:30 +0200
        Re: [RFC PATCHv2] usb: USB Type-C Connector Class Guenter Roeck <linux@roeck-us.net> - 2016-06-06 18:10 +0200
    [RFC PATCHv3] usb: USB Type-C Connector Class Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-06-10 16:40 +0200
      Re: [RFC PATCHv3] usb: USB Type-C Connector Class Oliver Neukum <oneukum@suse.com> - 2016-06-11 09:10 +0200

Page 3 of 5 — ← Prev page 1 2 [3] 4 5  Next page →

#1406946

On Wed, 2016-05-25 at 17:04 +0300, Heikki Krogerus wrote:

> I'm not against leaving the responsibility of registering the alternate
> modes to the drivers. I'm a little bit worried about relying then on
> the drivers to also handle the unregistering accordingly, but I can
> live with that. But we just shouldn't share the responsibility of
> un/registering them between the class and the drivers, so the driver
> should then handle the registration always.
> 
> Oliver, what do you think?

Either will do for me. Registration by the drivers is a bit better.
But it has to be the one or the other. Mixing is indeed bad.

	Regards
		Oliver

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

#1406996

On Wed, May 25, 2016 at 04:20:56PM +0200, Oliver Neukum wrote:
> On Wed, 2016-05-25 at 17:04 +0300, Heikki Krogerus wrote:
> 
> > I'm not against leaving the responsibility of registering the alternate
> > modes to the drivers. I'm a little bit worried about relying then on
> > the drivers to also handle the unregistering accordingly, but I can
> > live with that. But we just shouldn't share the responsibility of
> > un/registering them between the class and the drivers, so the driver
> > should then handle the registration always.
> > 
> > Oliver, what do you think?
> 
> Either will do for me. Registration by the drivers is a bit better.
> But it has to be the one or the other. Mixing is indeed bad.
> 
Same here. I don't have any problems handling unregistering
from the driver. I just have to keep track of the state and call
typec_unregister_altmodes() before calling typec_disconnect().

Having to wait for mode discovery to complete before calling
typec_connect() is much more complicated, at least with my current
code.

Guenter

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

#1407868

On Wed, May 25, 2016 at 07:59:57AM -0700, Guenter Roeck wrote:
> On Wed, May 25, 2016 at 04:20:56PM +0200, Oliver Neukum wrote:
> > On Wed, 2016-05-25 at 17:04 +0300, Heikki Krogerus wrote:
> > 
> > > I'm not against leaving the responsibility of registering the alternate
> > > modes to the drivers. I'm a little bit worried about relying then on
> > > the drivers to also handle the unregistering accordingly, but I can
> > > live with that. But we just shouldn't share the responsibility of
> > > un/registering them between the class and the drivers, so the driver
> > > should then handle the registration always.
> > > 
> > > Oliver, what do you think?
> > 
> > Either will do for me. Registration by the drivers is a bit better.
> > But it has to be the one or the other. Mixing is indeed bad.
> > 
> Same here. I don't have any problems handling unregistering
> from the driver. I just have to keep track of the state and call
> typec_unregister_altmodes() before calling typec_disconnect().
> 
> Having to wait for mode discovery to complete before calling
> typec_connect() is much more complicated, at least with my current
> code.

OK, so we'll change this and make the driver take care of the
registration.


-- 
heikki

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

#1407158 — [RFC PATCH] usb: typec: Various API updates and fixes

From: Guenter Roeck <groeck@chromium.org>

New API functions (calls into class code)
	typec_set_usb_role()
	typec_set_pwr_role()
	typec_set_vconn_role()
	typec_set_pwr_opmode()

Modified API functions (calls into class code):
	typec_register_port(dev, cap) ->
			typec_register_port(dev, cap, driver_data)

Modified callback functions:
	dr_swap(port) -> dr_set(port, driver_data, role);
	pr_swap(port) -> pr_set(port, driver_data, role);
	vconn_swap(port) -> vconn_set(port, driver_data, role);
	fix_role(port) -> fix_role(port, driver_data, role);
	activate_mode(...) -> activate_mode(..., driver_data, ...);

New sysfs attribute:
	current_vconn_role

Other:
- Extract role initialization to new function typec_init_roles()
- Call driver code unconditionally on role changes
- Add NULL check in typec_unregister_altmodes()
- If an alternate mode description pointer is NULL, display
  an empty string.

Signed-off-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
This patch applies on top of '[RFC PATCHv2] usb: USB Type-C Connector Class'
from Heikki Krogerus. It provided the changes I made to get the code
operational.

 drivers/usb/type-c/typec.c | 134 ++++++++++++++++++++++++++++++++++++---------
 include/linux/usb/typec.h  |  26 ++++++---
 2 files changed, 125 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
index 8028b7df0951..6836e972b681 100644
--- a/drivers/usb/type-c/typec.c
+++ b/drivers/usb/type-c/typec.c
@@ -27,6 +27,8 @@ struct typec_port {
 	struct typec_partner	*partner;
 	struct typec_cable	*cable;
 
+	void			*driver_data;
+
 	unsigned int		connected:1;
 
 	int			n_altmode;
@@ -324,6 +326,20 @@ static void typec_remove_cable(struct typec_port *port)
 	device_unregister(&port->cable->dev);
 }
 
+static void typec_init_roles(struct typec_port *port)
+{
+	if (port->fixed_role == TYPEC_PORT_DFP) {
+		port->usb_role = TYPEC_HOST;
+		port->pwr_role = TYPEC_PWR_SOURCE;
+		port->vconn_role = TYPEC_PWR_SOURCE;
+	} else {
+		/* Device mode as default also with DRP ports */
+		port->usb_role = TYPEC_DEVICE;
+		port->pwr_role = TYPEC_PWR_SINK;
+		port->vconn_role = TYPEC_PWR_SINK;
+	}
+}
+
 /* -------------------------------- */
 
 int typec_connect(struct typec_port *port, struct typec_connection *con)
@@ -378,16 +394,7 @@ void typec_disconnect(struct typec_port *port)
 
 	port->pwr_opmode = TYPEC_PWR_MODE_USB;
 
-	if (port->fixed_role == TYPEC_PORT_DFP) {
-		port->usb_role = TYPEC_HOST;
-		port->pwr_role = TYPEC_PWR_SOURCE;
-		port->vconn_role = TYPEC_PWR_SOURCE;
-	} else {
-		/* Device mode as default also with DRP ports */
-		port->usb_role = TYPEC_DEVICE;
-		port->pwr_role = TYPEC_PWR_SINK;
-		port->vconn_role = TYPEC_PWR_SINK;
-	}
+	typec_init_roles(port);
 
 	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
 }
@@ -405,6 +412,34 @@ struct typec_port *typec_dev2port(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(typec_dev2port);
 
+/* --------------------------------------- */
+/* Driver callbacks to report role updates */
+
+void typec_set_usb_role(struct typec_port *port, enum typec_usb_role role)
+{
+	port->usb_role = role;
+}
+EXPORT_SYMBOL(typec_set_usb_role);
+
+void typec_set_pwr_role(struct typec_port *port, enum typec_pwr_role role)
+{
+	port->pwr_role = role;
+}
+EXPORT_SYMBOL(typec_set_pwr_role);
+
+void typec_set_vconn_role(struct typec_port *port, enum typec_pwr_role role)
+{
+	port->vconn_role = role;
+}
+EXPORT_SYMBOL(typec_set_vconn_role);
+
+void typec_set_pwr_opmode(struct typec_port *port,
+			  enum typec_pwr_opmode opmode)
+{
+	port->pwr_opmode = opmode;
+}
+EXPORT_SYMBOL(typec_set_pwr_opmode);
+
 /* -------------------------------- */
 /* Alternate Modes */
 
@@ -451,7 +486,7 @@ typec_altmode_desc_show(struct device *dev, struct device_attribute *attr,
 	struct typec_mode *mode = container_of(attr, struct typec_mode,
 					       desc_attr);
 
-	return sprintf(buf, "%s\n", mode->desc);
+	return sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
 }
 
 static ssize_t
@@ -561,6 +596,9 @@ void typec_unregister_altmodes(struct typec_altmode *alt_modes)
 {
 	struct typec_altmode *alt;
 
+	if (!alt_modes)
+		return;
+
 	for (alt = alt_modes; alt->svid; alt++)
 		device_unregister(&alt->dev);
 }
@@ -581,7 +619,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->dr_swap) {
+	if (!port->cap->dr_set) {
 		dev_warn(dev, "data role swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -593,10 +631,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
 	else
 		return -EINVAL;
 
-	if (port->usb_role == role || !port->partner)
-		return size;
-
-	ret = port->cap->dr_swap(port);
+	ret = port->cap->dr_set(port, port->driver_data, role);
 	if (ret)
 		return ret;
 
@@ -655,10 +690,7 @@ current_data_role_store(struct device *dev, struct device_attribute *attr,
 	else
 		return -EINVAL;
 
-	if (port->fixed_role == role)
-		return size;
-
-	ret = port->cap->fix_role(port, role);
+	ret = port->cap->fix_role(port, port->driver_data, role);
 	if (ret)
 		return ret;
 
@@ -688,7 +720,7 @@ static ssize_t current_power_role_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->pr_swap) {
+	if (!port->cap->pr_set) {
 		dev_warn(dev, "power role swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -705,10 +737,7 @@ static ssize_t current_power_role_store(struct device *dev,
 	else
 		return -EINVAL;
 
-	if (port->pwr_role == role || !port->partner)
-		return size;
-
-	ret = port->cap->pr_swap(port);
+	ret = port->cap->pr_set(port, port->driver_data, role);
 	if (ret)
 		return ret;
 
@@ -762,6 +791,54 @@ static ssize_t power_operation_mode_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(power_operation_mode);
 
+static ssize_t current_vconn_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_pwr_role role;
+	int ret;
+
+	if (!port->cap->usb_pd) {
+		dev_dbg(dev, "vconn swap only supported with USB PD\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!port->cap->vconn_set) {
+		dev_warn(dev, "vconn swapping not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (!strncmp(buf, "source", 6))
+		role = TYPEC_PWR_SOURCE;
+	else if (!strncmp(buf, "sink", 4))
+		role = TYPEC_PWR_SINK;
+	else
+		return -EINVAL;
+
+	ret = port->cap->vconn_set(port, port->driver_data, role);
+	if (ret)
+		return ret;
+
+	return size;
+}
+
+static ssize_t current_vconn_role_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct typec_port *port = to_typec_port(dev);
+
+	switch (port->vconn_role) {
+	case TYPEC_PWR_SOURCE:
+		return sprintf(buf, "source\n");
+	case TYPEC_PWR_SINK:
+		return sprintf(buf, "sink\n");
+	default:
+		return sprintf(buf, "unknown\n");
+	};
+}
+static DEVICE_ATTR_RW(current_vconn_role);
+
 static ssize_t supports_audio_accessory_show(struct device *dev,
 					     struct device_attribute *attr,
 					     char *buf)
@@ -795,6 +872,7 @@ 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,
 	&dev_attr_power_operation_mode.attr,
 	&dev_attr_supported_data_roles.attr,
@@ -862,7 +940,8 @@ static struct device_type typec_port_dev_type = {
 };
 
 struct typec_port *typec_register_port(struct device *dev,
-				       struct typec_capability *cap)
+				       struct typec_capability *cap,
+				       void *driver_data)
 {
 	struct typec_port *port;
 	int ret;
@@ -880,6 +959,7 @@ 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;
@@ -888,6 +968,8 @@ struct typec_port *typec_register_port(struct device *dev,
 
 	port->fixed_role = port->cap->role;
 
+	typec_init_roles(port);
+
 	ret = device_register(&port->dev);
 	if (ret) {
 		ida_simple_remove(&typec_index_ida, id);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 86e5c867800b..d16a38de57ac 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -168,9 +168,9 @@ struct typec_partner {
  * @audio_accessory: Audio Accessory Adapter Mode support
  * @debug_accessory: Debug Accessory Mode support
  * @fix_role: Set a fixed data role for DRP port
- * @dr_swap: Data Role Swap support
- * @pr_swap: Power Role Swap support
- * @vconn_swap: VCONN Swap support
+ * @dr_set: Set Data Role
+ * @pr_set: Set Power Role
+ * @vconn_set: Set VCONN Role
  * @activate_mode: Enter/exit given Alternate Mode
  *
  * Static capabilities of a single USB Type-C port.
@@ -182,14 +182,14 @@ struct typec_capability {
 	unsigned int		audio_accessory:1;
 	unsigned int		debug_accessory:1;
 
-	int			(*fix_role)(struct typec_port *,
+	int			(*fix_role)(struct typec_port *, void *,
 					    enum typec_data_role);
 
-	int			(*dr_swap)(struct typec_port *);
-	int			(*pr_swap)(struct typec_port *);
-	int			(*vconn_swap)(struct typec_port *);
+	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			(*activate_mode)(struct typec_altmode *,
+	int			(*activate_mode)(struct typec_altmode *, void *,
 						 int mode, int activate);
 };
 
@@ -217,7 +217,8 @@ struct typec_connection {
 };
 
 struct typec_port *typec_register_port(struct device *dev,
-				       struct typec_capability *cap);
+				       struct typec_capability *cap,
+				       void *driver_data);
 void typec_unregister_port(struct typec_port *port);
 
 int typec_connect(struct typec_port *port, struct typec_connection *con);
@@ -227,4 +228,11 @@ void typec_disconnect(struct typec_port *port);
 struct device *typec_port2dev(struct typec_port *port);
 struct typec_port *typec_dev2port(struct device *dev);
 
+/* Callbacks from driver */
+
+void typec_set_usb_role(struct typec_port *, enum typec_usb_role);
+void typec_set_pwr_role(struct typec_port *, enum typec_pwr_role);
+void typec_set_vconn_role(struct typec_port *, enum typec_pwr_role);
+void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
+
 #endif /* __LINUX_USB_TYPEC_H */
-- 
2.5.0

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

#1407891 — Re: [RFC PATCH] usb: typec: Various API updates and fixes

Hi,

On Wed, May 25, 2016 at 11:35:07AM -0700, Guenter Roeck wrote:
> From: Guenter Roeck <groeck@chromium.org>
> 
> New API functions (calls into class code)
> 	typec_set_usb_role()
> 	typec_set_pwr_role()
> 	typec_set_vconn_role()
> 	typec_set_pwr_opmode()
> 
> Modified API functions (calls into class code):
> 	typec_register_port(dev, cap) ->
> 			typec_register_port(dev, cap, driver_data)
> 
> Modified callback functions:
> 	dr_swap(port) -> dr_set(port, driver_data, role);
> 	pr_swap(port) -> pr_set(port, driver_data, role);
> 	vconn_swap(port) -> vconn_set(port, driver_data, role);
> 	fix_role(port) -> fix_role(port, driver_data, role);
> 	activate_mode(...) -> activate_mode(..., driver_data, ...);
> 
> New sysfs attribute:
> 	current_vconn_role
> 
> Other:
> - Extract role initialization to new function typec_init_roles()
> - Call driver code unconditionally on role changes
> - Add NULL check in typec_unregister_altmodes()
> - If an alternate mode description pointer is NULL, display
>   an empty string.
> 
> Signed-off-by: Guenter Roeck <groeck@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> This patch applies on top of '[RFC PATCHv2] usb: USB Type-C Connector Class'
> from Heikki Krogerus. It provided the changes I made to get the code
> operational.
> 
>  drivers/usb/type-c/typec.c | 134 ++++++++++++++++++++++++++++++++++++---------
>  include/linux/usb/typec.h  |  26 ++++++---
>  2 files changed, 125 insertions(+), 35 deletions(-)

Thanks Guenter. After a quick glance looks good to me.

I think I'll create a development branch to my github tree for this
class and add this on top of the original v2 patch as is. Just to have
some history for as long we develop this thing. Though I'm not sure
how useful it is in this case. I think we need to put this together
fast. I'm thinking we need to target v4.8 with this.

I'll merge this into any case to v3, and I'll send on Monday.


> diff --git a/drivers/usb/type-c/typec.c b/drivers/usb/type-c/typec.c
> index 8028b7df0951..6836e972b681 100644
> --- a/drivers/usb/type-c/typec.c
> +++ b/drivers/usb/type-c/typec.c
> @@ -27,6 +27,8 @@ struct typec_port {
>  	struct typec_partner	*partner;
>  	struct typec_cable	*cable;
>  
> +	void			*driver_data;
> +
>  	unsigned int		connected:1;
>  
>  	int			n_altmode;
> @@ -324,6 +326,20 @@ static void typec_remove_cable(struct typec_port *port)
>  	device_unregister(&port->cable->dev);
>  }
>  
> +static void typec_init_roles(struct typec_port *port)
> +{
> +	if (port->fixed_role == TYPEC_PORT_DFP) {
> +		port->usb_role = TYPEC_HOST;
> +		port->pwr_role = TYPEC_PWR_SOURCE;
> +		port->vconn_role = TYPEC_PWR_SOURCE;
> +	} else {
> +		/* Device mode as default also with DRP ports */
> +		port->usb_role = TYPEC_DEVICE;
> +		port->pwr_role = TYPEC_PWR_SINK;
> +		port->vconn_role = TYPEC_PWR_SINK;
> +	}
> +}
> +
>  /* -------------------------------- */
>  
>  int typec_connect(struct typec_port *port, struct typec_connection *con)
> @@ -378,16 +394,7 @@ void typec_disconnect(struct typec_port *port)
>  
>  	port->pwr_opmode = TYPEC_PWR_MODE_USB;
>  
> -	if (port->fixed_role == TYPEC_PORT_DFP) {
> -		port->usb_role = TYPEC_HOST;
> -		port->pwr_role = TYPEC_PWR_SOURCE;
> -		port->vconn_role = TYPEC_PWR_SOURCE;
> -	} else {
> -		/* Device mode as default also with DRP ports */
> -		port->usb_role = TYPEC_DEVICE;
> -		port->pwr_role = TYPEC_PWR_SINK;
> -		port->vconn_role = TYPEC_PWR_SINK;
> -	}
> +	typec_init_roles(port);
>  
>  	kobject_uevent(&port->dev.kobj, KOBJ_CHANGE);
>  }
> @@ -405,6 +412,34 @@ struct typec_port *typec_dev2port(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(typec_dev2port);
>  
> +/* --------------------------------------- */
> +/* Driver callbacks to report role updates */
> +
> +void typec_set_usb_role(struct typec_port *port, enum typec_usb_role role)
> +{
> +	port->usb_role = role;
> +}
> +EXPORT_SYMBOL(typec_set_usb_role);
> +
> +void typec_set_pwr_role(struct typec_port *port, enum typec_pwr_role role)
> +{
> +	port->pwr_role = role;
> +}
> +EXPORT_SYMBOL(typec_set_pwr_role);
> +
> +void typec_set_vconn_role(struct typec_port *port, enum typec_pwr_role role)
> +{
> +	port->vconn_role = role;
> +}
> +EXPORT_SYMBOL(typec_set_vconn_role);
> +
> +void typec_set_pwr_opmode(struct typec_port *port,
> +			  enum typec_pwr_opmode opmode)
> +{
> +	port->pwr_opmode = opmode;
> +}
> +EXPORT_SYMBOL(typec_set_pwr_opmode);
> +
>  /* -------------------------------- */
>  /* Alternate Modes */
>  
> @@ -451,7 +486,7 @@ typec_altmode_desc_show(struct device *dev, struct device_attribute *attr,
>  	struct typec_mode *mode = container_of(attr, struct typec_mode,
>  					       desc_attr);
>  
> -	return sprintf(buf, "%s\n", mode->desc);
> +	return sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
>  }
>  
>  static ssize_t
> @@ -561,6 +596,9 @@ void typec_unregister_altmodes(struct typec_altmode *alt_modes)
>  {
>  	struct typec_altmode *alt;
>  
> +	if (!alt_modes)
> +		return;
> +
>  	for (alt = alt_modes; alt->svid; alt++)
>  		device_unregister(&alt->dev);
>  }
> @@ -581,7 +619,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (!port->cap->dr_swap) {
> +	if (!port->cap->dr_set) {
>  		dev_warn(dev, "data role swapping not supported\n");
>  		return -EOPNOTSUPP;
>  	}
> @@ -593,10 +631,7 @@ current_usb_data_role_store(struct device *dev, struct device_attribute *attr,
>  	else
>  		return -EINVAL;
>  
> -	if (port->usb_role == role || !port->partner)
> -		return size;
> -
> -	ret = port->cap->dr_swap(port);
> +	ret = port->cap->dr_set(port, port->driver_data, role);
>  	if (ret)
>  		return ret;
>  
> @@ -655,10 +690,7 @@ current_data_role_store(struct device *dev, struct device_attribute *attr,
>  	else
>  		return -EINVAL;
>  
> -	if (port->fixed_role == role)
> -		return size;
> -
> -	ret = port->cap->fix_role(port, role);
> +	ret = port->cap->fix_role(port, port->driver_data, role);
>  	if (ret)
>  		return ret;
>  
> @@ -688,7 +720,7 @@ static ssize_t current_power_role_store(struct device *dev,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (!port->cap->pr_swap) {
> +	if (!port->cap->pr_set) {
>  		dev_warn(dev, "power role swapping not supported\n");
>  		return -EOPNOTSUPP;
>  	}
> @@ -705,10 +737,7 @@ static ssize_t current_power_role_store(struct device *dev,
>  	else
>  		return -EINVAL;
>  
> -	if (port->pwr_role == role || !port->partner)
> -		return size;
> -
> -	ret = port->cap->pr_swap(port);
> +	ret = port->cap->pr_set(port, port->driver_data, role);
>  	if (ret)
>  		return ret;
>  
> @@ -762,6 +791,54 @@ static ssize_t power_operation_mode_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(power_operation_mode);
>  
> +static ssize_t current_vconn_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_pwr_role role;
> +	int ret;
> +
> +	if (!port->cap->usb_pd) {
> +		dev_dbg(dev, "vconn swap only supported with USB PD\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!port->cap->vconn_set) {
> +		dev_warn(dev, "vconn swapping not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!strncmp(buf, "source", 6))
> +		role = TYPEC_PWR_SOURCE;
> +	else if (!strncmp(buf, "sink", 4))
> +		role = TYPEC_PWR_SINK;
> +	else
> +		return -EINVAL;
> +
> +	ret = port->cap->vconn_set(port, port->driver_data, role);
> +	if (ret)
> +		return ret;
> +
> +	return size;
> +}
> +
> +static ssize_t current_vconn_role_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct typec_port *port = to_typec_port(dev);
> +
> +	switch (port->vconn_role) {
> +	case TYPEC_PWR_SOURCE:
> +		return sprintf(buf, "source\n");
> +	case TYPEC_PWR_SINK:
> +		return sprintf(buf, "sink\n");
> +	default:
> +		return sprintf(buf, "unknown\n");
> +	};
> +}
> +static DEVICE_ATTR_RW(current_vconn_role);
> +
>  static ssize_t supports_audio_accessory_show(struct device *dev,
>  					     struct device_attribute *attr,
>  					     char *buf)
> @@ -795,6 +872,7 @@ 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,
>  	&dev_attr_power_operation_mode.attr,
>  	&dev_attr_supported_data_roles.attr,
> @@ -862,7 +940,8 @@ static struct device_type typec_port_dev_type = {
>  };
>  
>  struct typec_port *typec_register_port(struct device *dev,
> -				       struct typec_capability *cap)
> +				       struct typec_capability *cap,
> +				       void *driver_data)
>  {
>  	struct typec_port *port;
>  	int ret;
> @@ -880,6 +959,7 @@ 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;
> @@ -888,6 +968,8 @@ struct typec_port *typec_register_port(struct device *dev,
>  
>  	port->fixed_role = port->cap->role;
>  
> +	typec_init_roles(port);
> +
>  	ret = device_register(&port->dev);
>  	if (ret) {
>  		ida_simple_remove(&typec_index_ida, id);
> diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> index 86e5c867800b..d16a38de57ac 100644
> --- a/include/linux/usb/typec.h
> +++ b/include/linux/usb/typec.h
> @@ -168,9 +168,9 @@ struct typec_partner {
>   * @audio_accessory: Audio Accessory Adapter Mode support
>   * @debug_accessory: Debug Accessory Mode support
>   * @fix_role: Set a fixed data role for DRP port
> - * @dr_swap: Data Role Swap support
> - * @pr_swap: Power Role Swap support
> - * @vconn_swap: VCONN Swap support
> + * @dr_set: Set Data Role
> + * @pr_set: Set Power Role
> + * @vconn_set: Set VCONN Role
>   * @activate_mode: Enter/exit given Alternate Mode
>   *
>   * Static capabilities of a single USB Type-C port.
> @@ -182,14 +182,14 @@ struct typec_capability {
>  	unsigned int		audio_accessory:1;
>  	unsigned int		debug_accessory:1;
>  
> -	int			(*fix_role)(struct typec_port *,
> +	int			(*fix_role)(struct typec_port *, void *,
>  					    enum typec_data_role);
>  
> -	int			(*dr_swap)(struct typec_port *);
> -	int			(*pr_swap)(struct typec_port *);
> -	int			(*vconn_swap)(struct typec_port *);
> +	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			(*activate_mode)(struct typec_altmode *,
> +	int			(*activate_mode)(struct typec_altmode *, void *,
>  						 int mode, int activate);
>  };
>  
> @@ -217,7 +217,8 @@ struct typec_connection {
>  };
>  
>  struct typec_port *typec_register_port(struct device *dev,
> -				       struct typec_capability *cap);
> +				       struct typec_capability *cap,
> +				       void *driver_data);
>  void typec_unregister_port(struct typec_port *port);
>  
>  int typec_connect(struct typec_port *port, struct typec_connection *con);
> @@ -227,4 +228,11 @@ void typec_disconnect(struct typec_port *port);
>  struct device *typec_port2dev(struct typec_port *port);
>  struct typec_port *typec_dev2port(struct device *dev);
>  
> +/* Callbacks from driver */
> +
> +void typec_set_usb_role(struct typec_port *, enum typec_usb_role);
> +void typec_set_pwr_role(struct typec_port *, enum typec_pwr_role);
> +void typec_set_vconn_role(struct typec_port *, enum typec_pwr_role);
> +void typec_set_pwr_opmode(struct typec_port *, enum typec_pwr_opmode);
> +
>  #endif /* __LINUX_USB_TYPEC_H */
> -- 
> 2.5.0

-- 
heikki

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

#1408095 — Re: [RFC PATCH] usb: typec: Various API updates and fixes

On 05/27/2016 12:55 AM, Heikki Krogerus wrote:
> Hi,
>

[ ... ]

>> ---
>> This patch applies on top of '[RFC PATCHv2] usb: USB Type-C Connector Class'
>> from Heikki Krogerus. It provided the changes I made to get the code
>> operational.
>>
>>   drivers/usb/type-c/typec.c | 134 ++++++++++++++++++++++++++++++++++++---------
>>   include/linux/usb/typec.h  |  26 ++++++---
>>   2 files changed, 125 insertions(+), 35 deletions(-)
>
> Thanks Guenter. After a quick glance looks good to me.
>
My pleasure.

> I think I'll create a development branch to my github tree for this
> class and add this on top of the original v2 patch as is. Just to have
> some history for as long we develop this thing. Though I'm not sure
> how useful it is in this case. I think we need to put this together
> fast. I'm thinking we need to target v4.8 with this.
>

Yes, agreed.

> I'll merge this into any case to v3, and I'll send on Monday.
>
Sounds good.

Couple of additional comments.

I don't really know what to do with the 'desc' field in struct typec_mode
for modes received from the partner. I wonder if it makes sense to display
it for partner modes.

Also, there is currently no attribute to show the partner identity (data
received from the Discover Identity command). Would it make sense to add
an 'identity' attribute to the partner device (plus an associated API
function to set it) ?

Thanks,
Guenter

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

#1409074 — Re: [RFC PATCH] usb: typec: Various API updates and fixes

Hi guys,

On Fri, May 27, 2016 at 07:06:41AM -0700, Guenter Roeck wrote:
> On 05/27/2016 12:55 AM, Heikki Krogerus wrote:
> > I'll merge this into any case to v3, and I'll send on Monday.
> > 
> Sounds good.
> 
> Couple of additional comments.
> 
> I don't really know what to do with the 'desc' field in struct typec_mode
> for modes received from the partner. I wonder if it makes sense to display
> it for partner modes.

The idea comes straight from Billboard class where every mode has an
index to an _optional_ string. Those desc should ultimately come from
the altmode bus driver, and I think never from the port drivers.

> Also, there is currently no attribute to show the partner identity (data
> received from the Discover Identity command). Would it make sense to add
> an 'identity' attribute to the partner device (plus an associated API
> function to set it) ?

The problem is that we can only use it when the partner and our port
are both PD capable. Details about PDUSB devices that are attached are
out side the scope of this interface IMO, and need to be in any case
handled in USB core by using the PD Class specific descriptors etc.,
and possible with the help of the future PD stack that you are working
on. It's an other task. So this interface IMO should expose only the
Type-C specific details about the ports and partners. How about if we
just add an attribute "usb_communication_capable" for the partners?
That is something that we should be able to always determine, even
without USB PD.


Thanks,

-- 
heikki

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

#1409108

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

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

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

#1409146

On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> 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?

1. Because we need error handling.
   Devices crash. Cables will crash. We will get out of sync.
   You never put yourself in a place where you cannot handle an
   IO error.
2. Because it is in the spec. We do not second guess the spec.
   We implement it.

> 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.

It is not very important. i am fine with it.

> 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.

What are the alternatives?

> 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.

That sounds good.

	Regards
		Oliver

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

#1409990

Hi Oliver,

On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > 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?
> 
> 1. Because we need error handling.
>    Devices crash. Cables will crash. We will get out of sync.
>    You never put yourself in a place where you cannot handle an
>    IO error.
> 2. Because it is in the spec. We do not second guess the spec.
>    We implement it.

Error conditions and crashes are the responsibility of the USB PD
stack, not userspace. In those cases the stack can not wait for a
command from the userspace. So for example if a timer like
NoResponseTimer times out, the stack an its state machines will have
to take care of the reset quite independently.

If you get out of sync with an alternate mode, you reset that specific
alternate mode by exiting and re-entering it, and you do not reset the
entire PD connection, port, partner or cable.

The resets from userspace would be purely unsolicited. What would the
cases where we would need to tricker a reset like that?

I want to be careful with exposing reset to userspace. Reset in USB PD
is not just an IO related thing. When you tricker a reset with USB PD,
even if it's a soft reset, it may lead into hard reset, which may
potentially lead into sudden voltage and current drop, which may lead
into the entire system crashing. We really need to understand the
cases where it would be necessary to tricker a reset from userspace.
Right now I don't see any.

> > 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.
> 
> It is not very important. i am fine with it.
> 
> > 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.
> 
> What are the alternatives?

Can we make a group for them under the port device somehow? Like the
supported_alternate_modes I proposed. I guess it's not possible to add
devices to a specific group in sysfs. And would it even be useful.


Thanks,

-- 
heikki

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

#1410009

On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> Hi Oliver,
> 
> On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > 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?
> > 
> > 1. Because we need error handling.
> >    Devices crash. Cables will crash. We will get out of sync.
> >    You never put yourself in a place where you cannot handle an
> >    IO error.
> > 2. Because it is in the spec. We do not second guess the spec.
> >    We implement it.
> 
> Error conditions and crashes are the responsibility of the USB PD
> stack, not userspace. In those cases the stack can not wait for a

Those are not exclusive conditions.

> command from the userspace. So for example if a timer like
> NoResponseTimer times out, the stack an its state machines will have
> to take care of the reset quite independently.

Yes. But somebody needs to handle high level errors.

> If you get out of sync with an alternate mode, you reset that specific
> alternate mode by exiting and re-entering it, and you do not reset the
> entire PD connection, port, partner or cable.

That would be the first step. If that doesn't work you will at that
point either give up or use the next largest hammer.
In principle you could do that in kernel space, but that implies
that the kernel can detect all failures. That is unlikely.

> The resets from userspace would be purely unsolicited. What would the
> cases where we would need to tricker a reset like that?
> 
> I want to be careful with exposing reset to userspace. Reset in USB PD
> is not just an IO related thing. When you tricker a reset with USB PD,
> even if it's a soft reset, it may lead into hard reset, which may
> potentially lead into sudden voltage and current drop, which may lead
> into the entire system crashing. We really need to understand the
> cases where it would be necessary to tricker a reset from userspace.
> Right now I don't see any.

User space can call reboot. Actually that does not help.
Reset is an operation that is intended for error handling.
If all else fails, we will need to use it.
Its bad consequences apply whether you trigger this from kernel or
user space. In fact, an operation that may potentially crash the system
should involve user space.

[..]
> > > 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.
> > 
> > What are the alternatives?
> 
> Can we make a group for them under the port device somehow? Like the
> supported_alternate_modes I proposed. I guess it's not possible to add
> devices to a specific group in sysfs. And would it even be useful.

Please explain. It is not clear to me what you are proposing here.

	Regards
		Oliver

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

#1410234

On Tue, May 31, 2016 at 10:48:29AM +0200, Oliver Neukum wrote:
> On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> > Hi Oliver,
> > 
> > On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > > 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?
> > > 
> > > 1. Because we need error handling.
> > >    Devices crash. Cables will crash. We will get out of sync.
> > >    You never put yourself in a place where you cannot handle an
> > >    IO error.
> > > 2. Because it is in the spec. We do not second guess the spec.
> > >    We implement it.
> > 
> > Error conditions and crashes are the responsibility of the USB PD
> > stack, not userspace. In those cases the stack can not wait for a
> 
> Those are not exclusive conditions.
> 
> > command from the userspace. So for example if a timer like
> > NoResponseTimer times out, the stack an its state machines will have
> > to take care of the reset quite independently.
> 
> Yes. But somebody needs to handle high level errors.
> 
> > If you get out of sync with an alternate mode, you reset that specific
> > alternate mode by exiting and re-entering it, and you do not reset the
> > entire PD connection, port, partner or cable.
> 
> That would be the first step. If that doesn't work you will at that
> point either give up or use the next largest hammer.
> In principle you could do that in kernel space, but that implies
> that the kernel can detect all failures. That is unlikely.

Any PD communication failures the kernel has to be able to detect, so
I guess you mean failures with the alternate modes themselves, right?

In that case, surely exiting the mode is enough to "reset" it? When it
is re-entered, it has to be completely re-configured in any case. I
don't see how resetting the whole port or cable would guarantee that a
mode would become any more functional in case of failures? It will
however make also the other active modes to de-activate even if they
are functioning fine.

> > The resets from userspace would be purely unsolicited. What would the
> > cases where we would need to tricker a reset like that?
> > 
> > I want to be careful with exposing reset to userspace. Reset in USB PD
> > is not just an IO related thing. When you tricker a reset with USB PD,
> > even if it's a soft reset, it may lead into hard reset, which may
> > potentially lead into sudden voltage and current drop, which may lead
> > into the entire system crashing. We really need to understand the
> > cases where it would be necessary to tricker a reset from userspace.
> > Right now I don't see any.
> 
> User space can call reboot. Actually that does not help.
> Reset is an operation that is intended for error handling.
> If all else fails, we will need to use it.
> Its bad consequences apply whether you trigger this from kernel or
> user space. In fact, an operation that may potentially crash the system
> should involve user space.
> 
> > > > 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.
> > > 
> > > What are the alternatives?
> > 
> > Can we make a group for them under the port device somehow? Like the
> > supported_alternate_modes I proposed. I guess it's not possible to add
> > devices to a specific group in sysfs. And would it even be useful.
> 
> Please explain. It is not clear to me what you are proposing here.

So is it possible to have a folder in sysfs for the alternate modes
a port supports?

So instead of:

        /sysfs/class/type-c/usbc0/svid:xxx1/
        /sysfs/class/type-c/usbc0/svid:xxx2/
        /sysfs/class/type-c/usbc0/svid:xxx3/
        ...

We would have something like:

        /sysfs/class/type-c/usbc0/supported_alternate_modes/svid:xxx1/
        /sysfs/class/type-c/usbc0/supported_alternate_modes/svid:xxx2/
        /sysfs/class/type-c/usbc0/supported_alternate_modes/svid:xxx3/
        ...


Thanks,

-- 
heikki

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

#1410266

On Tue, May 31, 2016 at 03:09:01PM +0300, Heikki Krogerus wrote:
> On Tue, May 31, 2016 at 10:48:29AM +0200, Oliver Neukum wrote:
> > On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> > > Hi Oliver,
> > > 
> > > On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > > > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > > > 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?
> > > > 
> > > > 1. Because we need error handling.
> > > >    Devices crash. Cables will crash. We will get out of sync.
> > > >    You never put yourself in a place where you cannot handle an
> > > >    IO error.
> > > > 2. Because it is in the spec. We do not second guess the spec.
> > > >    We implement it.
> > > 
> > > Error conditions and crashes are the responsibility of the USB PD
> > > stack, not userspace. In those cases the stack can not wait for a
> > 
> > Those are not exclusive conditions.
> > 
> > > command from the userspace. So for example if a timer like
> > > NoResponseTimer times out, the stack an its state machines will have
> > > to take care of the reset quite independently.
> > 
> > Yes. But somebody needs to handle high level errors.
> > 
> > > If you get out of sync with an alternate mode, you reset that specific
> > > alternate mode by exiting and re-entering it, and you do not reset the
> > > entire PD connection, port, partner or cable.
> > 
> > That would be the first step. If that doesn't work you will at that
> > point either give up or use the next largest hammer.
> > In principle you could do that in kernel space, but that implies
> > that the kernel can detect all failures. That is unlikely.
> 
> Any PD communication failures the kernel has to be able to detect, so
> I guess you mean failures with the alternate modes themselves, right?
> 
> In that case, surely exiting the mode is enough to "reset" it? When it
> is re-entered, it has to be completely re-configured in any case. I
> don't see how resetting the whole port or cable would guarantee that a
> mode would become any more functional in case of failures? It will
> however make also the other active modes to de-activate even if they
> are functioning fine.

Forget about it, I'll just add the reset attributes. I'm still not
clear about their usefulness, but instead they will just create a small
risk, but I can live with that.


Cheers,

-- 
heikki

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

#1410469

On Tue, May 31, 2016 at 03:43:56PM +0300, Heikki Krogerus wrote:
> On Tue, May 31, 2016 at 03:09:01PM +0300, Heikki Krogerus wrote:
> > On Tue, May 31, 2016 at 10:48:29AM +0200, Oliver Neukum wrote:
> > > On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> > > > Hi Oliver,
> > > > 
> > > > On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > > > > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > > > > 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?
> > > > > 
> > > > > 1. Because we need error handling.
> > > > >    Devices crash. Cables will crash. We will get out of sync.
> > > > >    You never put yourself in a place where you cannot handle an
> > > > >    IO error.
> > > > > 2. Because it is in the spec. We do not second guess the spec.
> > > > >    We implement it.
> > > > 
> > > > Error conditions and crashes are the responsibility of the USB PD
> > > > stack, not userspace. In those cases the stack can not wait for a
> > > 
> > > Those are not exclusive conditions.
> > > 
> > > > command from the userspace. So for example if a timer like
> > > > NoResponseTimer times out, the stack an its state machines will have
> > > > to take care of the reset quite independently.
> > > 
> > > Yes. But somebody needs to handle high level errors.
> > > 
> > > > If you get out of sync with an alternate mode, you reset that specific
> > > > alternate mode by exiting and re-entering it, and you do not reset the
> > > > entire PD connection, port, partner or cable.
> > > 
> > > That would be the first step. If that doesn't work you will at that
> > > point either give up or use the next largest hammer.
> > > In principle you could do that in kernel space, but that implies
> > > that the kernel can detect all failures. That is unlikely.
> > 
> > Any PD communication failures the kernel has to be able to detect, so
> > I guess you mean failures with the alternate modes themselves, right?
> > 
> > In that case, surely exiting the mode is enough to "reset" it? When it
> > is re-entered, it has to be completely re-configured in any case. I
> > don't see how resetting the whole port or cable would guarantee that a
> > mode would become any more functional in case of failures? It will
> > however make also the other active modes to de-activate even if they
> > are functioning fine.
> 
> Forget about it, I'll just add the reset attributes. I'm still not
> clear about their usefulness, but instead they will just create a small
> risk, but I can live with that.
> 

Given my experience over the last few weeks, I think the added risk
may not just be small, and I think the added benefit is questionable.
Reset handling is not well implemented in all devices, and manually
triggered resets in an unexpected state may make the situation worse.

Can you make it optional ? I may choose not to support it to avoid
the risk.

Thanks,
Guenter

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

#1410934

On Tue, May 31, 2016 at 10:20:34AM -0700, Guenter Roeck wrote:
> On Tue, May 31, 2016 at 03:43:56PM +0300, Heikki Krogerus wrote:
> > On Tue, May 31, 2016 at 03:09:01PM +0300, Heikki Krogerus wrote:
> > > On Tue, May 31, 2016 at 10:48:29AM +0200, Oliver Neukum wrote:
> > > > On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> > > > > Hi Oliver,
> > > > > 
> > > > > On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > > > > > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > > > > > 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?
> > > > > > 
> > > > > > 1. Because we need error handling.
> > > > > >    Devices crash. Cables will crash. We will get out of sync.
> > > > > >    You never put yourself in a place where you cannot handle an
> > > > > >    IO error.
> > > > > > 2. Because it is in the spec. We do not second guess the spec.
> > > > > >    We implement it.
> > > > > 
> > > > > Error conditions and crashes are the responsibility of the USB PD
> > > > > stack, not userspace. In those cases the stack can not wait for a
> > > > 
> > > > Those are not exclusive conditions.
> > > > 
> > > > > command from the userspace. So for example if a timer like
> > > > > NoResponseTimer times out, the stack an its state machines will have
> > > > > to take care of the reset quite independently.
> > > > 
> > > > Yes. But somebody needs to handle high level errors.
> > > > 
> > > > > If you get out of sync with an alternate mode, you reset that specific
> > > > > alternate mode by exiting and re-entering it, and you do not reset the
> > > > > entire PD connection, port, partner or cable.
> > > > 
> > > > That would be the first step. If that doesn't work you will at that
> > > > point either give up or use the next largest hammer.
> > > > In principle you could do that in kernel space, but that implies
> > > > that the kernel can detect all failures. That is unlikely.
> > > 
> > > Any PD communication failures the kernel has to be able to detect, so
> > > I guess you mean failures with the alternate modes themselves, right?
> > > 
> > > In that case, surely exiting the mode is enough to "reset" it? When it
> > > is re-entered, it has to be completely re-configured in any case. I
> > > don't see how resetting the whole port or cable would guarantee that a
> > > mode would become any more functional in case of failures? It will
> > > however make also the other active modes to de-activate even if they
> > > are functioning fine.
> > 
> > Forget about it, I'll just add the reset attributes. I'm still not
> > clear about their usefulness, but instead they will just create a small
> > risk, but I can live with that.
> > 
> 
> Given my experience over the last few weeks, I think the added risk
> may not just be small, and I think the added benefit is questionable.
> Reset handling is not well implemented in all devices, and manually
> triggered resets in an unexpected state may make the situation worse.
> 
> Can you make it optional ? I may choose not to support it to avoid
> the risk.

Maybe I gave up on this too hastily... I changing my mind about this,
I'm not going to add them. Having them optional is not enough. It
changes nothing when they are implemented. I think there is a change
that we would actually end up having to remove the attributes, which
would be really bad.

I think we can still add them later if they are still seen as
necessity later on, tough I seriously doubt it. It would not be
ideal, but adding an attribute should not really break anything,
right? Removing would.


Thanks,

-- 
heikki

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

#1410952

On Wed, 2016-06-01 at 11:23 +0300, Heikki Krogerus wrote:
> On Tue, May 31, 2016 at 10:20:34AM -0700, Guenter Roeck wrote:
> > On Tue, May 31, 2016 at 03:43:56PM +0300, Heikki Krogerus wrote:
> > > On Tue, May 31, 2016 at 03:09:01PM +0300, Heikki Krogerus wrote:
> > > > On Tue, May 31, 2016 at 10:48:29AM +0200, Oliver Neukum wrote:
> > > > > On Tue, 2016-05-31 at 11:31 +0300, Heikki Krogerus wrote:
> > > > > > Hi Oliver,
> > > > > > 
> > > > > > On Mon, May 30, 2016 at 03:59:27PM +0200, Oliver Neukum wrote:
> > > > > > > On Mon, 2016-05-30 at 16:19 +0300, Heikki Krogerus wrote:
> > > > > > > > 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?
> > > > > > > 
> > > > > > > 1. Because we need error handling.
> > > > > > >    Devices crash. Cables will crash. We will get out of sync.
> > > > > > >    You never put yourself in a place where you cannot handle an
> > > > > > >    IO error.
> > > > > > > 2. Because it is in the spec. We do not second guess the spec.
> > > > > > >    We implement it.
> > > > > > 
> > > > > > Error conditions and crashes are the responsibility of the USB PD
> > > > > > stack, not userspace. In those cases the stack can not wait for a
> > > > > 
> > > > > Those are not exclusive conditions.
> > > > > 
> > > > > > command from the userspace. So for example if a timer like
> > > > > > NoResponseTimer times out, the stack an its state machines will have
> > > > > > to take care of the reset quite independently.
> > > > > 
> > > > > Yes. But somebody needs to handle high level errors.
> > > > > 
> > > > > > If you get out of sync with an alternate mode, you reset that specific
> > > > > > alternate mode by exiting and re-entering it, and you do not reset the
> > > > > > entire PD connection, port, partner or cable.
> > > > > 
> > > > > That would be the first step. If that doesn't work you will at that
> > > > > point either give up or use the next largest hammer.
> > > > > In principle you could do that in kernel space, but that implies
> > > > > that the kernel can detect all failures. That is unlikely.
> > > > 
> > > > Any PD communication failures the kernel has to be able to detect, so
> > > > I guess you mean failures with the alternate modes themselves, right?
> > > > 
> > > > In that case, surely exiting the mode is enough to "reset" it? When it
> > > > is re-entered, it has to be completely re-configured in any case. I
> > > > don't see how resetting the whole port or cable would guarantee that a
> > > > mode would become any more functional in case of failures? It will
> > > > however make also the other active modes to de-activate even if they
> > > > are functioning fine.
> > > 
> > > Forget about it, I'll just add the reset attributes. I'm still not
> > > clear about their usefulness, but instead they will just create a small
> > > risk, but I can live with that.
> > > 
> > 
> > Given my experience over the last few weeks, I think the added risk
> > may not just be small, and I think the added benefit is questionable.
> > Reset handling is not well implemented in all devices, and manually
> > triggered resets in an unexpected state may make the situation worse.
> > 
> > Can you make it optional ? I may choose not to support it to avoid
> > the risk.
> 
> Maybe I gave up on this too hastily... I changing my mind about this,
> I'm not going to add them. Having them optional is not enough. It
> changes nothing when they are implemented. I think there is a change
> that we would actually end up having to remove the attributes, which
> would be really bad.
> 
> I think we can still add them later if they are still seen as
> necessity later on, tough I seriously doubt it. It would not be
> ideal, but adding an attribute should not really break anything,
> right? Removing would.

That is true. So let's leave it out for now. I still think sane
error handling will require it eventually, but that will be in the
future.

	Regards
		Oliver

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

#1411008

On Wed, 2016-06-01 at 11:23 +0300, Heikki Krogerus wrote:
> I think we can still add them later if they are still seen as
> necessity later on, tough I seriously doubt it. It would not be
> ideal, but adding an attribute should not really break anything,
> right? Removing would.

However, how do we learn that the other side has triggered a reset?

	Regards
		Oliver

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

#1411228

On 06/01/2016 02:04 AM, Oliver Neukum wrote:
> On Wed, 2016-06-01 at 11:23 +0300, Heikki Krogerus wrote:
>> I think we can still add them later if they are still seen as
>> necessity later on, tough I seriously doubt it. It would not be
>> ideal, but adding an attribute should not really break anything,
>> right? Removing would.
>
> However, how do we learn that the other side has triggered a reset?
>

USB PD specification, section 6.8.2.2 (Modal Operation and Hard Reset):

A Hard Reset shall cause all Active Modes to be exited by both Port Partners
and any Cable Plugs (see Section 6.4.4.3.4).

Section 6.4.4.3.4 (Enter Mode Command):

The following events shall also cause the Port Partners and Cable Plug(s) to exit all Active Modes:
- A PD Hard Reset
- The Port Partners or Cable Plug(s) are Detached
- A Cable Reset (only exits the Cable Plug’s Active Modes)

The class code would not explicitly learn about the reset,
but it would be informed about the exited modes.

Guenter

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

#1411882

On Wed, 2016-06-01 at 06:34 -0700, Guenter Roeck wrote:
> The class code would not explicitly learn about the reset,
> but it would be informed about the exited modes.

That has drawbacks

- it doesn't tell you what caused the mode to be left (if you
  UFP, it may be the regular command)
- it is a race against your own command
- it does not work if you are in basic USB mode

	Regards
		Oliver

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

#1411891

On 06/01/2016 11:24 PM, Oliver Neukum wrote:
> On Wed, 2016-06-01 at 06:34 -0700, Guenter Roeck wrote:
>> The class code would not explicitly learn about the reset,
>> but it would be informed about the exited modes.
>
> That has drawbacks
>

Playing devils advocate a bit here

> - it doesn't tell you what caused the mode to be left (if you
>    UFP, it may be the regular command)

Does it matter ?

> - it is a race against your own command

It is my understanding that races have to be resolved by the drivers,
since the typec code does not do any locking. This is quite similar
to handling, say, a request to change the vconn source or to change
the power role. Am I missing something ?

> - it does not work if you are in basic USB mode
>
Would alternate modes be active in that case ?

Thanks,
Guenter

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

Page 3 of 5 — ← Prev page 1 2 [3] 4 5  Next page →

Back to top | Article view | linux.kernel

csiph-web