linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/14] usb: typec: UCSI driver overhaul
@ 2019-09-26 10:07 Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 01/14] usb: typec: Copy everything from struct typec_capability during registration Heikki Krogerus
                   ` (15 more replies)
  0 siblings, 16 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Hi Ajay,

Here's the pretty much complete rewrite of the I/O handling that I was
talking about. The first seven patches are not actually related to
this stuff, but I'm including them here because the rest of the series
is made on top of them. I'm including also that fix patch I send you
earlier.

After this it should be easier to handle quirks. My idea how to handle
the multi-instance connector alt modes is that we "emulate" the PPM in
ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all.

We can now get the connector alternate modes that the actual
controller supplies during probe - before registering the ucsi
interface - and squash all alt modes with the same SVID into one that
we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES
command. Also other alt mode commands like SET_NEW_CAM can have
special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should
not be any problem with that anymore.

thanks,

Heikki Krogerus (14):
  usb: typec: Copy everything from struct typec_capability during
    registration
  usb: typec: Introduce typec_get_drvdata()
  usb: typec: Separate the operations vector
  usb: typec: tcpm: Start using struct typec_operations
  usb: typec: tps6598x: Start using struct typec_operations
  usb: typec: ucsi: Start using struct typec_operations
  usb: typec: Remove the callback members from struct typec_capability
  usb: typec: ucsi: ccg: Remove run_isr flag
  usb: typec: ucsi: Simplified interface registration and I/O API
  usb: typec: ucsi: acpi: Move to the new API
  usb: typec: ucsi: ccg: Move to the new API
  usb: typec: ucsi: Remove the old API
  usb: typec: ucsi: Remove struct ucsi_control
  usb: typec: ucsi: Remove all bit-fields

 drivers/usb/typec/class.c            | 125 +++---
 drivers/usb/typec/tcpm/tcpm.c        |  47 +--
 drivers/usb/typec/tps6598x.c         |  49 +--
 drivers/usb/typec/ucsi/displayport.c |  26 +-
 drivers/usb/typec/ucsi/trace.c       |  11 -
 drivers/usb/typec/ucsi/trace.h       |  79 +---
 drivers/usb/typec/ucsi/ucsi.c        | 592 ++++++++++++++-------------
 drivers/usb/typec/ucsi/ucsi.h        | 410 +++++++------------
 drivers/usb/typec/ucsi/ucsi_acpi.c   |  96 ++++-
 drivers/usb/typec/ucsi/ucsi_ccg.c    | 214 ++++------
 include/linux/usb/typec.h            |  38 +-
 11 files changed, 785 insertions(+), 902 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 01/14] usb: typec: Copy everything from struct typec_capability during registration
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 02/14] usb: typec: Introduce typec_get_drvdata() Heikki Krogerus
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Copying everything from struct typec_capability to struct
typec_port during port registration.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 55 +++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 94a3eda62add..3835e2d9fba6 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -46,8 +46,14 @@ struct typec_port {
 	enum typec_role			vconn_role;
 	enum typec_pwr_opmode		pwr_opmode;
 	enum typec_port_type		port_type;
+	enum typec_port_type		fixed_role;
+	enum typec_port_data		port_roles;
+	enum typec_accessory		accessory[TYPEC_MAX_ACCESSORY];
 	struct mutex			port_type_lock;
 
+	u16				revision;
+	u16				pd_revision;
+
 	enum typec_orientation		orientation;
 	struct typec_switch		*sw;
 	struct typec_mux		*mux;
@@ -950,7 +956,7 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 	int role;
 	int ret;
 
-	if (port->cap->type != TYPEC_PORT_DRP) {
+	if (port->fixed_role != TYPEC_PORT_DRP) {
 		dev_dbg(dev, "Preferred role only supported with DRP ports\n");
 		return -EOPNOTSUPP;
 	}
@@ -982,7 +988,7 @@ preferred_role_show(struct device *dev, struct device_attribute *attr,
 {
 	struct typec_port *port = to_typec_port(dev);
 
-	if (port->cap->type != TYPEC_PORT_DRP)
+	if (port->fixed_role != TYPEC_PORT_DRP)
 		return 0;
 
 	if (port->prefer_role < 0)
@@ -1009,7 +1015,7 @@ static ssize_t data_role_store(struct device *dev,
 		return ret;
 
 	mutex_lock(&port->port_type_lock);
-	if (port->cap->data != TYPEC_PORT_DRD) {
+	if (port->port_roles != TYPEC_PORT_DRD) {
 		ret = -EOPNOTSUPP;
 		goto unlock_and_ret;
 	}
@@ -1029,7 +1035,7 @@ static ssize_t data_role_show(struct device *dev,
 {
 	struct typec_port *port = to_typec_port(dev);
 
-	if (port->cap->data == TYPEC_PORT_DRD)
+	if (port->port_roles == TYPEC_PORT_DRD)
 		return sprintf(buf, "%s\n", port->data_role == TYPEC_HOST ?
 			       "[host] device" : "host [device]");
 
@@ -1044,7 +1050,7 @@ static ssize_t power_role_store(struct device *dev,
 	struct typec_port *port = to_typec_port(dev);
 	int ret;
 
-	if (!port->cap->pd_revision) {
+	if (!port->pd_revision) {
 		dev_dbg(dev, "USB Power Delivery not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1064,9 +1070,9 @@ static ssize_t power_role_store(struct device *dev,
 		return ret;
 
 	mutex_lock(&port->port_type_lock);
-	if (port->port_type != TYPEC_PORT_DRP) {
+	if (port->fixed_role != TYPEC_PORT_DRP) {
 		dev_dbg(dev, "port type fixed at \"%s\"",
-			     typec_port_power_roles[port->port_type]);
+			     typec_port_power_roles[port->fixed_role]);
 		ret = -EOPNOTSUPP;
 		goto unlock_and_ret;
 	}
@@ -1086,7 +1092,7 @@ static ssize_t power_role_show(struct device *dev,
 {
 	struct typec_port *port = to_typec_port(dev);
 
-	if (port->cap->type == TYPEC_PORT_DRP)
+	if (port->fixed_role == TYPEC_PORT_DRP)
 		return sprintf(buf, "%s\n", port->pwr_role == TYPEC_SOURCE ?
 			       "[source] sink" : "source [sink]");
 
@@ -1102,7 +1108,7 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 	int ret;
 	enum typec_port_type type;
 
-	if (!port->cap->port_type_set || port->cap->type != TYPEC_PORT_DRP) {
+	if (!port->cap->port_type_set || port->fixed_role != TYPEC_PORT_DRP) {
 		dev_dbg(dev, "changing port type not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1114,7 +1120,7 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 	type = ret;
 	mutex_lock(&port->port_type_lock);
 
-	if (port->port_type == type) {
+	if (port->fixed_role == type) {
 		ret = size;
 		goto unlock_and_ret;
 	}
@@ -1123,7 +1129,7 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		goto unlock_and_ret;
 
-	port->port_type = type;
+	port->fixed_role = type;
 	ret = size;
 
 unlock_and_ret:
@@ -1137,11 +1143,11 @@ port_type_show(struct device *dev, struct device_attribute *attr,
 {
 	struct typec_port *port = to_typec_port(dev);
 
-	if (port->cap->type == TYPEC_PORT_DRP)
+	if (port->fixed_role == TYPEC_PORT_DRP)
 		return sprintf(buf, "%s\n",
-			       typec_port_types_drp[port->port_type]);
+			       typec_port_types_drp[port->fixed_role]);
 
-	return sprintf(buf, "[%s]\n", typec_port_power_roles[port->cap->type]);
+	return sprintf(buf, "[%s]\n", typec_port_power_roles[port->fixed_role]);
 }
 static DEVICE_ATTR_RW(port_type);
 
@@ -1170,7 +1176,7 @@ static ssize_t vconn_source_store(struct device *dev,
 	bool source;
 	int ret;
 
-	if (!port->cap->pd_revision) {
+	if (!port->pd_revision) {
 		dev_dbg(dev, "VCONN swap depends on USB Power Delivery\n");
 		return -EOPNOTSUPP;
 	}
@@ -1209,10 +1215,10 @@ static ssize_t supported_accessory_modes_show(struct device *dev,
 	ssize_t ret = 0;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(port->cap->accessory); i++) {
-		if (port->cap->accessory[i])
+	for (i = 0; i < ARRAY_SIZE(port->accessory); i++) {
+		if (port->accessory[i])
 			ret += sprintf(buf + ret, "%s ",
-			       typec_accessory_modes[port->cap->accessory[i]]);
+			       typec_accessory_modes[port->accessory[i]]);
 	}
 
 	if (!ret)
@@ -1229,7 +1235,7 @@ static ssize_t usb_typec_revision_show(struct device *dev,
 				       char *buf)
 {
 	struct typec_port *port = to_typec_port(dev);
-	u16 rev = port->cap->revision;
+	u16 rev = port->revision;
 
 	return sprintf(buf, "%d.%d\n", (rev >> 8) & 0xff, (rev >> 4) & 0xf);
 }
@@ -1241,7 +1247,7 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
 {
 	struct typec_port *p = to_typec_port(dev);
 
-	return sprintf(buf, "%d\n", (p->cap->pd_revision >> 8) & 0xff);
+	return sprintf(buf, "%d\n", (p->pd_revision >> 8) & 0xff);
 }
 static DEVICE_ATTR_RO(usb_power_delivery_revision);
 
@@ -1532,6 +1538,7 @@ struct typec_port *typec_register_port(struct device *parent,
 	struct typec_port *port;
 	int ret;
 	int id;
+	int i;
 
 	port = kzalloc(sizeof(*port), GFP_KERNEL);
 	if (!port)
@@ -1581,8 +1588,16 @@ struct typec_port *typec_register_port(struct device *parent,
 	port->id = id;
 	port->cap = cap;
 	port->port_type = cap->type;
+	port->fixed_role = cap->type;
+	port->port_roles = cap->data;
 	port->prefer_role = cap->prefer_role;
 
+	port->revision = cap->revision;
+	port->pd_revision = cap->pd_revision;
+
+	for (i = 0; i < TYPEC_MAX_ACCESSORY; i++)
+		port->accessory[i] = cap->accessory[i];
+
 	device_initialize(&port->dev);
 	port->dev.class = typec_class;
 	port->dev.parent = parent;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 02/14] usb: typec: Introduce typec_get_drvdata()
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 01/14] usb: typec: Copy everything from struct typec_capability during registration Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 03/14] usb: typec: Separate the operations vector Heikki Krogerus
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Leaving the private driver_data pointer of the port device
to the port drivers.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 11 +++++++++++
 include/linux/usb/typec.h |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 3835e2d9fba6..9fab0be8f08c 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1492,6 +1492,16 @@ EXPORT_SYMBOL_GPL(typec_set_mode);
 
 /* --------------------------------------- */
 
+/**
+ * typec_get_drvdata - Return private driver data pointer
+ * @port: USB Type-C port
+ */
+void *typec_get_drvdata(struct typec_port *port)
+{
+	return dev_get_drvdata(&port->dev);
+}
+EXPORT_SYMBOL_GPL(typec_get_drvdata);
+
 /**
  * typec_port_register_altmode - Register USB Type-C Port Alternate Mode
  * @port: USB Type-C Port that supports the alternate mode
@@ -1604,6 +1614,7 @@ struct typec_port *typec_register_port(struct device *parent,
 	port->dev.fwnode = cap->fwnode;
 	port->dev.type = &typec_port_dev_type;
 	dev_set_name(&port->dev, "port%d", id);
+	dev_set_drvdata(&port->dev, cap->driver_data);
 
 	port->sw = typec_switch_get(&port->dev);
 	if (IS_ERR(port->sw)) {
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 7df4ecabc78a..8b90cd77331c 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -179,6 +179,7 @@ struct typec_partner_desc {
  * @sw: Cable plug orientation switch
  * @mux: Multiplexer switch for Alternate/Accessory Modes
  * @fwnode: Optional fwnode of the port
+ * @driver_data: Private pointer for driver specific info
  * @try_role: Set data role preference for DRP port
  * @dr_set: Set Data Role
  * @pr_set: Set Power Role
@@ -198,6 +199,7 @@ struct typec_capability {
 	struct typec_switch	*sw;
 	struct typec_mux	*mux;
 	struct fwnode_handle	*fwnode;
+	void			*driver_data;
 
 	int		(*try_role)(const struct typec_capability *,
 				    int role);
@@ -241,6 +243,8 @@ int typec_set_orientation(struct typec_port *port,
 enum typec_orientation typec_get_orientation(struct typec_port *port);
 int typec_set_mode(struct typec_port *port, int mode);
 
+void *typec_get_drvdata(struct typec_port *port);
+
 int typec_find_port_power_role(const char *name);
 int typec_find_power_role(const char *name);
 int typec_find_port_data_role(const char *name);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 03/14] usb: typec: Separate the operations vector
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 01/14] usb: typec: Copy everything from struct typec_capability during registration Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 02/14] usb: typec: Introduce typec_get_drvdata() Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 04/14] usb: typec: tcpm: Start using struct typec_operations Heikki Krogerus
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Introducing struct typec_operations which has the same
callbacks as struct typec_capability. The old callbacks are
kept for now, but after all users have been converted, they
will be removed.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 90 +++++++++++++++++++++++++--------------
 include/linux/usb/typec.h | 19 +++++++++
 2 files changed, 76 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 9fab0be8f08c..542be63795db 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -59,6 +59,7 @@ struct typec_port {
 	struct typec_mux		*mux;
 
 	const struct typec_capability	*cap;
+	const struct typec_operations	*ops;
 };
 
 #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
@@ -961,11 +962,6 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->try_role) {
-		dev_dbg(dev, "Setting preferred role not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	role = sysfs_match_string(typec_roles, buf);
 	if (role < 0) {
 		if (sysfs_streq(buf, "none"))
@@ -974,9 +970,18 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 			return -EINVAL;
 	}
 
-	ret = port->cap->try_role(port->cap, role);
-	if (ret)
-		return ret;
+	if (port->ops && port->ops->try_role) {
+		ret = port->ops->try_role(port, role);
+		if (ret)
+			return ret;
+	} else if (port->cap && port->cap->try_role) {
+		ret = port->cap->try_role(port->cap, role);
+		if (ret)
+			return ret;
+	} else {
+		dev_dbg(dev, "Setting preferred role not supported\n");
+		return -EOPNOTSUPP;
+	}
 
 	port->prefer_role = role;
 	return size;
@@ -1005,11 +1010,6 @@ static ssize_t data_role_store(struct device *dev,
 	struct typec_port *port = to_typec_port(dev);
 	int ret;
 
-	if (!port->cap->dr_set) {
-		dev_dbg(dev, "data role swapping not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	ret = sysfs_match_string(typec_data_roles, buf);
 	if (ret < 0)
 		return ret;
@@ -1020,9 +1020,19 @@ static ssize_t data_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->dr_set(port->cap, ret);
-	if (ret)
+	if (port->ops && port->ops->dr_set) {
+		ret = port->ops->dr_set(port, ret);
+		if (ret)
+			goto unlock_and_ret;
+	} else if (port->cap && port->cap->dr_set) {
+		ret = port->cap->dr_set(port->cap, ret);
+		if (ret)
+			goto unlock_and_ret;
+	} else {
+		dev_dbg(dev, "data role swapping not supported\n");
+		ret = -EOPNOTSUPP;
 		goto unlock_and_ret;
+	}
 
 	ret = size;
 unlock_and_ret:
@@ -1055,11 +1065,6 @@ static ssize_t power_role_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->pr_set) {
-		dev_dbg(dev, "power role swapping not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
 		dev_dbg(dev, "partner unable to swap power role\n");
 		return -EIO;
@@ -1077,11 +1082,21 @@ static ssize_t power_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->pr_set(port->cap, ret);
-	if (ret)
+	if (port->ops && port->ops->pr_set) {
+		ret = port->ops->pr_set(port, ret);
+		if (ret)
+			goto unlock_and_ret;
+	} else if (port->cap && port->cap->pr_set) {
+		ret = port->cap->pr_set(port->cap, ret);
+		if (ret)
+			goto unlock_and_ret;
+	} else {
+		dev_dbg(dev, "power role swapping not supported\n");
+		ret = -EOPNOTSUPP;
 		goto unlock_and_ret;
-
+	}
 	ret = size;
+
 unlock_and_ret:
 	mutex_unlock(&port->port_type_lock);
 	return ret;
@@ -1108,7 +1123,8 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 	int ret;
 	enum typec_port_type type;
 
-	if (!port->cap->port_type_set || port->fixed_role != TYPEC_PORT_DRP) {
+	if ((!port->ops || !port->ops->port_type_set) ||
+	    !port->cap->port_type_set || port->fixed_role != TYPEC_PORT_DRP) {
 		dev_dbg(dev, "changing port type not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1125,7 +1141,10 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->port_type_set(port->cap, type);
+	if (port->ops && port->ops->port_type_set)
+		ret = port->ops->port_type_set(port, type);
+	else
+		ret = port->cap->port_type_set(port->cap, type);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1181,18 +1200,22 @@ static ssize_t vconn_source_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->vconn_set) {
-		dev_dbg(dev, "VCONN swapping not supported\n");
-		return -EOPNOTSUPP;
-	}
-
 	ret = kstrtobool(buf, &source);
 	if (ret)
 		return ret;
 
-	ret = port->cap->vconn_set(port->cap, (enum typec_role)source);
-	if (ret)
-		return ret;
+	if (port->ops && port->ops->vconn_set) {
+		ret = port->ops->vconn_set(port, source);
+		if (ret)
+			return ret;
+	} else if (port->cap && port->cap->vconn_set) {
+		ret = port->cap->vconn_set(port->cap, (enum typec_role)source);
+		if (ret)
+			return ret;
+	} else {
+		dev_dbg(dev, "VCONN swapping not supported\n");
+		return -EOPNOTSUPP;
+	}
 
 	return size;
 }
@@ -1597,6 +1620,7 @@ struct typec_port *typec_register_port(struct device *parent,
 
 	port->id = id;
 	port->cap = cap;
+	port->ops = cap->ops;
 	port->port_type = cap->type;
 	port->fixed_role = cap->type;
 	port->port_roles = cap->data;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 8b90cd77331c..6c95a9ff43c6 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -168,6 +168,22 @@ struct typec_partner_desc {
 	struct usb_pd_identity	*identity;
 };
 
+/*
+ * struct typec_operations - USB Type-C Port Operations
+ * @try_role: Set data role preference for DRP port
+ * @dr_set: Set Data Role
+ * @pr_set: Set Power Role
+ * @vconn_set: Source VCONN
+ * @port_type_set: Set port type
+ */
+struct typec_operations {
+	int (*try_role)(struct typec_port *port, int role);
+	int (*dr_set)(struct typec_port *port, enum typec_data_role);
+	int (*pr_set)(struct typec_port *port, enum typec_role);
+	int (*vconn_set)(struct typec_port *port, bool source);
+	int (*port_type_set)(struct typec_port *port, enum typec_port_type);
+};
+
 /*
  * struct typec_capability - USB Type-C Port Capabilities
  * @type: Supported power role of the port
@@ -180,6 +196,7 @@ struct typec_partner_desc {
  * @mux: Multiplexer switch for Alternate/Accessory Modes
  * @fwnode: Optional fwnode of the port
  * @driver_data: Private pointer for driver specific info
+ * @ops: Port operations vector
  * @try_role: Set data role preference for DRP port
  * @dr_set: Set Data Role
  * @pr_set: Set Power Role
@@ -201,6 +218,8 @@ struct typec_capability {
 	struct fwnode_handle	*fwnode;
 	void			*driver_data;
 
+	const struct typec_operations	*ops;
+
 	int		(*try_role)(const struct typec_capability *,
 				    int role);
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 04/14] usb: typec: tcpm: Start using struct typec_operations
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (2 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 03/14] usb: typec: Separate the operations vector Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 05/14] usb: typec: tps6598x: " Heikki Krogerus
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 47 ++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 96562744101c..0fde7e042d5f 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -390,12 +390,6 @@ static enum tcpm_state tcpm_default_state(struct tcpm_port *port)
 	return SRC_UNATTACHED;
 }
 
-static inline
-struct tcpm_port *typec_cap_to_tcpm(const struct typec_capability *cap)
-{
-	return container_of(cap, struct tcpm_port, typec_caps);
-}
-
 static bool tcpm_port_is_disconnected(struct tcpm_port *port)
 {
 	return (!port->attached && port->cc1 == TYPEC_CC_OPEN &&
@@ -3970,10 +3964,9 @@ void tcpm_pd_hard_reset(struct tcpm_port *port)
 }
 EXPORT_SYMBOL_GPL(tcpm_pd_hard_reset);
 
-static int tcpm_dr_set(const struct typec_capability *cap,
-		       enum typec_data_role data)
+static int tcpm_dr_set(struct typec_port *p, enum typec_data_role data)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	int ret;
 
 	mutex_lock(&port->swap_lock);
@@ -4038,10 +4031,9 @@ static int tcpm_dr_set(const struct typec_capability *cap,
 	return ret;
 }
 
-static int tcpm_pr_set(const struct typec_capability *cap,
-		       enum typec_role role)
+static int tcpm_pr_set(struct typec_port *p, enum typec_role role)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	int ret;
 
 	mutex_lock(&port->swap_lock);
@@ -4082,10 +4074,9 @@ static int tcpm_pr_set(const struct typec_capability *cap,
 	return ret;
 }
 
-static int tcpm_vconn_set(const struct typec_capability *cap,
-			  enum typec_role role)
+static int tcpm_vconn_set(struct typec_port *p, bool source)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	int ret;
 
 	mutex_lock(&port->swap_lock);
@@ -4096,7 +4087,7 @@ static int tcpm_vconn_set(const struct typec_capability *cap,
 		goto port_unlock;
 	}
 
-	if (role == port->vconn_role) {
+	if (source == port->vconn_role) {
 		ret = 0;
 		goto port_unlock;
 	}
@@ -4122,9 +4113,9 @@ static int tcpm_vconn_set(const struct typec_capability *cap,
 	return ret;
 }
 
-static int tcpm_try_role(const struct typec_capability *cap, int role)
+static int tcpm_try_role(struct typec_port *p, int role)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 	struct tcpc_dev	*tcpc = port->tcpc;
 	int ret = 0;
 
@@ -4331,10 +4322,9 @@ static void tcpm_init(struct tcpm_port *port)
 	tcpm_set_state(port, PORT_RESET, 0);
 }
 
-static int tcpm_port_type_set(const struct typec_capability *cap,
-			      enum typec_port_type type)
+static int tcpm_port_type_set(struct typec_port *p, enum typec_port_type type)
 {
-	struct tcpm_port *port = typec_cap_to_tcpm(cap);
+	struct tcpm_port *port = typec_get_drvdata(p);
 
 	mutex_lock(&port->lock);
 	if (type == port->port_type)
@@ -4359,6 +4349,14 @@ static int tcpm_port_type_set(const struct typec_capability *cap,
 	return 0;
 }
 
+static const struct typec_operations tcpm_ops = {
+	.try_role = tcpm_try_role,
+	.dr_set = tcpm_dr_set,
+	.pr_set = tcpm_pr_set,
+	.vconn_set = tcpm_vconn_set,
+	.port_type_set = tcpm_port_type_set
+};
+
 void tcpm_tcpc_reset(struct tcpm_port *port)
 {
 	mutex_lock(&port->lock);
@@ -4770,11 +4768,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->typec_caps.fwnode = tcpc->fwnode;
 	port->typec_caps.revision = 0x0120;	/* Type-C spec release 1.2 */
 	port->typec_caps.pd_revision = 0x0300;	/* USB-PD spec release 3.0 */
-	port->typec_caps.dr_set = tcpm_dr_set;
-	port->typec_caps.pr_set = tcpm_pr_set;
-	port->typec_caps.vconn_set = tcpm_vconn_set;
-	port->typec_caps.try_role = tcpm_try_role;
-	port->typec_caps.port_type_set = tcpm_port_type_set;
+	port->typec_caps.driver_data = port;
+	port->typec_caps.ops = &tcpm_ops;
 
 	port->partner_desc.identity = &port->partner_ident;
 	port->port_type = port->typec_caps.type;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 05/14] usb: typec: tps6598x: Start using struct typec_operations
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (3 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 04/14] usb: typec: tcpm: Start using struct typec_operations Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 06/14] usb: typec: ucsi: " Heikki Krogerus
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration. After this there
is not need to keep the capabilities stored anywhere in the
driver.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/tps6598x.c | 49 +++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index a38d1409f15b..0698addd1185 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -94,7 +94,6 @@ struct tps6598x {
 	struct typec_port *port;
 	struct typec_partner *partner;
 	struct usb_pd_identity partner_identity;
-	struct typec_capability typec_cap;
 };
 
 /*
@@ -307,11 +306,10 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
 	return 0;
 }
 
-static int
-tps6598x_dr_set(const struct typec_capability *cap, enum typec_data_role role)
+static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
 {
-	struct tps6598x *tps = container_of(cap, struct tps6598x, typec_cap);
 	const char *cmd = (role == TYPEC_DEVICE) ? "SWUF" : "SWDF";
+	struct tps6598x *tps = typec_get_drvdata(port);
 	u32 status;
 	int ret;
 
@@ -338,11 +336,10 @@ tps6598x_dr_set(const struct typec_capability *cap, enum typec_data_role role)
 	return ret;
 }
 
-static int
-tps6598x_pr_set(const struct typec_capability *cap, enum typec_role role)
+static int tps6598x_pr_set(struct typec_port *port, enum typec_role role)
 {
-	struct tps6598x *tps = container_of(cap, struct tps6598x, typec_cap);
 	const char *cmd = (role == TYPEC_SINK) ? "SWSk" : "SWSr";
+	struct tps6598x *tps = typec_get_drvdata(port);
 	u32 status;
 	int ret;
 
@@ -369,6 +366,11 @@ tps6598x_pr_set(const struct typec_capability *cap, enum typec_role role)
 	return ret;
 }
 
+static const struct typec_operations tps6598x_ops = {
+	.dr_set = tps6598x_dr_set,
+	.pr_set = tps6598x_pr_set,
+};
+
 static irqreturn_t tps6598x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
@@ -448,6 +450,7 @@ static const struct regmap_config tps6598x_regmap_config = {
 
 static int tps6598x_probe(struct i2c_client *client)
 {
+	struct typec_capability typec_cap = { };
 	struct tps6598x *tps;
 	u32 status;
 	u32 conf;
@@ -492,40 +495,40 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	tps->typec_cap.revision = USB_TYPEC_REV_1_2;
-	tps->typec_cap.pd_revision = 0x200;
-	tps->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
-	tps->typec_cap.pr_set = tps6598x_pr_set;
-	tps->typec_cap.dr_set = tps6598x_dr_set;
+	typec_cap.revision = USB_TYPEC_REV_1_2;
+	typec_cap.pd_revision = 0x200;
+	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+	typec_cap.driver_data = tps;
+	typec_cap.ops = &tps6598x_ops;
 
 	switch (TPS_SYSCONF_PORTINFO(conf)) {
 	case TPS_PORTINFO_SINK_ACCESSORY:
 	case TPS_PORTINFO_SINK:
-		tps->typec_cap.type = TYPEC_PORT_SNK;
-		tps->typec_cap.data = TYPEC_PORT_UFP;
+		typec_cap.type = TYPEC_PORT_SNK;
+		typec_cap.data = TYPEC_PORT_UFP;
 		break;
 	case TPS_PORTINFO_DRP_UFP_DRD:
 	case TPS_PORTINFO_DRP_DFP_DRD:
-		tps->typec_cap.type = TYPEC_PORT_DRP;
-		tps->typec_cap.data = TYPEC_PORT_DRD;
+		typec_cap.type = TYPEC_PORT_DRP;
+		typec_cap.data = TYPEC_PORT_DRD;
 		break;
 	case TPS_PORTINFO_DRP_UFP:
-		tps->typec_cap.type = TYPEC_PORT_DRP;
-		tps->typec_cap.data = TYPEC_PORT_UFP;
+		typec_cap.type = TYPEC_PORT_DRP;
+		typec_cap.data = TYPEC_PORT_UFP;
 		break;
 	case TPS_PORTINFO_DRP_DFP:
-		tps->typec_cap.type = TYPEC_PORT_DRP;
-		tps->typec_cap.data = TYPEC_PORT_DFP;
+		typec_cap.type = TYPEC_PORT_DRP;
+		typec_cap.data = TYPEC_PORT_DFP;
 		break;
 	case TPS_PORTINFO_SOURCE:
-		tps->typec_cap.type = TYPEC_PORT_SRC;
-		tps->typec_cap.data = TYPEC_PORT_DFP;
+		typec_cap.type = TYPEC_PORT_SRC;
+		typec_cap.data = TYPEC_PORT_DFP;
 		break;
 	default:
 		return -ENODEV;
 	}
 
-	tps->port = typec_register_port(&client->dev, &tps->typec_cap);
+	tps->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(tps->port))
 		return PTR_ERR(tps->port);
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 06/14] usb: typec: ucsi: Start using struct typec_operations
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (4 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 05/14] usb: typec: tps6598x: " Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 07/14] usb: typec: Remove the callback members from struct typec_capability Heikki Krogerus
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Supplying the operation callbacks as part of a struct
typec_operations instead of as part of struct
typec_capability during port registration.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ba288b964dc8..edd722fb88b8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -17,9 +17,6 @@
 #include "ucsi.h"
 #include "trace.h"
 
-#define to_ucsi_connector(_cap_) container_of(_cap_, struct ucsi_connector, \
-					      typec_cap)
-
 /*
  * UCSI_TIMEOUT_MS - PPM communication timeout
  *
@@ -713,10 +710,9 @@ static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl)
 	return ret;
 }
 
-static int
-ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role)
+static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 {
-	struct ucsi_connector *con = to_ucsi_connector(cap);
+	struct ucsi_connector *con = typec_get_drvdata(port);
 	struct ucsi_control ctrl;
 	int ret = 0;
 
@@ -748,10 +744,9 @@ ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role)
 	return ret < 0 ? ret : 0;
 }
 
-static int
-ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role)
+static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
 {
-	struct ucsi_connector *con = to_ucsi_connector(cap);
+	struct ucsi_connector *con = typec_get_drvdata(port);
 	struct ucsi_control ctrl;
 	int ret = 0;
 
@@ -788,6 +783,11 @@ ucsi_pr_swap(const struct typec_capability *cap, enum typec_role role)
 	return ret;
 }
 
+static const struct typec_operations ucsi_ops = {
+	.dr_set = ucsi_dr_swap,
+	.pr_set = ucsi_pr_swap
+};
+
 static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
 {
 	struct fwnode_handle *fwnode;
@@ -843,8 +843,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		*accessory = TYPEC_ACCESSORY_DEBUG;
 
 	cap->fwnode = ucsi_find_fwnode(con);
-	cap->dr_set = ucsi_dr_swap;
-	cap->pr_set = ucsi_pr_swap;
+	cap->driver_data = con;
+	cap->ops = &ucsi_ops;
 
 	/* Register the connector */
 	con->port = typec_register_port(ucsi->dev, cap);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 07/14] usb: typec: Remove the callback members from struct typec_capability
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (5 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 06/14] usb: typec: ucsi: " Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 08/14] usb: typec: ucsi: ccg: Remove run_isr flag Heikki Krogerus
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

There are no more users for them.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/class.c | 65 +++++++++++++--------------------------
 include/linux/usb/typec.h | 17 ----------
 2 files changed, 22 insertions(+), 60 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 542be63795db..58e83fc54aa6 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -58,7 +58,6 @@ struct typec_port {
 	struct typec_switch		*sw;
 	struct typec_mux		*mux;
 
-	const struct typec_capability	*cap;
 	const struct typec_operations	*ops;
 };
 
@@ -970,19 +969,15 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 			return -EINVAL;
 	}
 
-	if (port->ops && port->ops->try_role) {
-		ret = port->ops->try_role(port, role);
-		if (ret)
-			return ret;
-	} else if (port->cap && port->cap->try_role) {
-		ret = port->cap->try_role(port->cap, role);
-		if (ret)
-			return ret;
-	} else {
+	if (!port->ops || !port->ops->try_role) {
 		dev_dbg(dev, "Setting preferred role not supported\n");
 		return -EOPNOTSUPP;
 	}
 
+	ret = port->ops->try_role(port, role);
+	if (ret)
+		return ret;
+
 	port->prefer_role = role;
 	return size;
 }
@@ -1020,20 +1015,16 @@ static ssize_t data_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	if (port->ops && port->ops->dr_set) {
-		ret = port->ops->dr_set(port, ret);
-		if (ret)
-			goto unlock_and_ret;
-	} else if (port->cap && port->cap->dr_set) {
-		ret = port->cap->dr_set(port->cap, ret);
-		if (ret)
-			goto unlock_and_ret;
-	} else {
+	if (!port->ops || !port->ops->dr_set) {
 		dev_dbg(dev, "data role swapping not supported\n");
 		ret = -EOPNOTSUPP;
 		goto unlock_and_ret;
 	}
 
+	ret = port->ops->dr_set(port, ret);
+	if (ret)
+		goto unlock_and_ret;
+
 	ret = size;
 unlock_and_ret:
 	mutex_unlock(&port->port_type_lock);
@@ -1082,21 +1073,17 @@ static ssize_t power_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	if (port->ops && port->ops->pr_set) {
-		ret = port->ops->pr_set(port, ret);
-		if (ret)
-			goto unlock_and_ret;
-	} else if (port->cap && port->cap->pr_set) {
-		ret = port->cap->pr_set(port->cap, ret);
-		if (ret)
-			goto unlock_and_ret;
-	} else {
+	if (!port->ops || !port->ops->pr_set) {
 		dev_dbg(dev, "power role swapping not supported\n");
 		ret = -EOPNOTSUPP;
 		goto unlock_and_ret;
 	}
 	ret = size;
 
+	ret = port->ops->pr_set(port, ret);
+	if (ret)
+		goto unlock_and_ret;
+
 unlock_and_ret:
 	mutex_unlock(&port->port_type_lock);
 	return ret;
@@ -1124,7 +1111,7 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 	enum typec_port_type type;
 
 	if ((!port->ops || !port->ops->port_type_set) ||
-	    !port->cap->port_type_set || port->fixed_role != TYPEC_PORT_DRP) {
+	    port->fixed_role != TYPEC_PORT_DRP) {
 		dev_dbg(dev, "changing port type not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1141,10 +1128,7 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 		goto unlock_and_ret;
 	}
 
-	if (port->ops && port->ops->port_type_set)
-		ret = port->ops->port_type_set(port, type);
-	else
-		ret = port->cap->port_type_set(port->cap, type);
+	ret = port->ops->port_type_set(port, type);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1204,19 +1188,15 @@ static ssize_t vconn_source_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (port->ops && port->ops->vconn_set) {
-		ret = port->ops->vconn_set(port, source);
-		if (ret)
-			return ret;
-	} else if (port->cap && port->cap->vconn_set) {
-		ret = port->cap->vconn_set(port->cap, (enum typec_role)source);
-		if (ret)
-			return ret;
-	} else {
+	if (!port->ops || !port->ops->vconn_set) {
 		dev_dbg(dev, "VCONN swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
 
+	ret = port->ops->vconn_set(port, source);
+	if (ret)
+		return ret;
+
 	return size;
 }
 
@@ -1619,7 +1599,6 @@ struct typec_port *typec_register_port(struct device *parent,
 	mutex_init(&port->port_type_lock);
 
 	port->id = id;
-	port->cap = cap;
 	port->ops = cap->ops;
 	port->port_type = cap->type;
 	port->fixed_role = cap->type;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 6c95a9ff43c6..e759668f8af9 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -197,11 +197,6 @@ struct typec_operations {
  * @fwnode: Optional fwnode of the port
  * @driver_data: Private pointer for driver specific info
  * @ops: Port operations vector
- * @try_role: Set data role preference for DRP port
- * @dr_set: Set Data Role
- * @pr_set: Set Power Role
- * @vconn_set: Set VCONN Role
- * @port_type_set: Set port type
  *
  * Static capabilities of a single USB Type-C port.
  */
@@ -219,18 +214,6 @@ struct typec_capability {
 	void			*driver_data;
 
 	const struct typec_operations	*ops;
-
-	int		(*try_role)(const struct typec_capability *,
-				    int role);
-
-	int		(*dr_set)(const struct typec_capability *,
-				  enum typec_data_role);
-	int		(*pr_set)(const struct typec_capability *,
-				  enum typec_role);
-	int		(*vconn_set)(const struct typec_capability *,
-				     enum typec_role);
-	int		(*port_type_set)(const struct typec_capability *,
-					 enum typec_port_type);
 };
 
 /* Specific to try_role(). Indicates the user want's to clear the preference. */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 08/14] usb: typec: ucsi: ccg: Remove run_isr flag
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (6 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 07/14] usb: typec: Remove the callback members from struct typec_capability Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 10:07 ` [PATCH 09/14] usb: typec: ucsi: Simplified interface registration and I/O API Heikki Krogerus
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

The "run_isr" flag is used for preventing the driver from
calling the interrupt service routine in its runtime resume
callback when the driver is expecting completion to a
command, but what that basically does is that it hides the
real problem. The real problem is that the controller is
allowed to suspend in the middle of command execution.

As a more appropriate fix for the problem, using autosuspend
delay time that matches UCSI_TIMEOUT_MS (5s). That prevents
the controller from suspending while still in the middle of
executing a command.

This fixes a potential deadlock. Both ccg_read() and
ccg_write() are called with the mutex already taken at least
from ccg_send_command(). In ccg_read() and ccg_write, the
mutex is only acquired so that run_isr flag can be set.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 42 +++----------------------------
 1 file changed, 4 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 907e20e1a71e..d772fce51905 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -195,7 +195,6 @@ struct ucsi_ccg {
 
 	/* fw build with vendor information */
 	u16 fw_build;
-	bool run_isr; /* flag to call ISR routine during resume */
 	struct work_struct pm_work;
 };
 
@@ -224,18 +223,6 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
-	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		/*
-		 * Do not schedule pm_work to run ISR in
-		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
-		 * since we are already in ISR path.
-		 */
-		uc->run_isr = false;
-		mutex_unlock(&uc->lock);
-	}
-
 	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
@@ -278,18 +265,6 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
-	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		/*
-		 * Do not schedule pm_work to run ISR in
-		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
-		 * since we are already in ISR path.
-		 */
-		uc->run_isr = false;
-		mutex_unlock(&uc->lock);
-	}
-
 	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
@@ -1130,7 +1105,6 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	uc->ppm.sync = ucsi_ccg_sync;
 	uc->dev = dev;
 	uc->client = client;
-	uc->run_isr = true;
 	mutex_init(&uc->lock);
 	INIT_WORK(&uc->work, ccg_update_firmware);
 	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
@@ -1188,6 +1162,8 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 
 	pm_runtime_set_active(uc->dev);
 	pm_runtime_enable(uc->dev);
+	pm_runtime_use_autosuspend(uc->dev);
+	pm_runtime_set_autosuspend_delay(uc->dev, 5000);
 	pm_runtime_idle(uc->dev);
 
 	return 0;
@@ -1229,7 +1205,6 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ucsi_ccg *uc = i2c_get_clientdata(client);
-	bool schedule = true;
 
 	/*
 	 * Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
@@ -1237,17 +1212,8 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
 	 * Schedule a work to call ISR as a workaround.
 	 */
 	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		if (!uc->run_isr) {
-			uc->run_isr = true;
-			schedule = false;
-		}
-		mutex_unlock(&uc->lock);
-
-		if (schedule)
-			schedule_work(&uc->pm_work);
-	}
+	    uc->fw_version <= CCG_OLD_FW_VERSION)
+		schedule_work(&uc->pm_work);
 
 	return 0;
 }
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 09/14] usb: typec: ucsi: Simplified interface registration and I/O API
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (7 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 08/14] usb: typec: ucsi: ccg: Remove run_isr flag Heikki Krogerus
@ 2019-09-26 10:07 ` Heikki Krogerus
  2019-09-26 14:25 ` [PATCH 10/14] usb: typec: ucsi: acpi: Move to the new API Heikki Krogerus
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 10:07 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Adding more simplified API for interface registration and
read and write operations.

The registration is split into separate creation and
registration phases. That allows the drivers to properly
initialize the interface before registering it if necessary.

The read and write operations are supplied in a completely
separate struct ucsi_operations that is passed to the
ucsi_register() function during registration. The new read
and write operations will work more traditionally so that
the read callback function reads a requested amount of data
from an offset, and the write callback functions write the
given data to the offset. The drivers will have to support
both non-blocking writing and blocking writing. In blocking
writing the driver itself is responsible of waiting for the
completion event.

The new API makes it possible for the drivers to perform
tasks also independently of the core ucsi.c, and that should
allow for example quirks to be handled completely in the
drivers without the need to touch ucsi.c.

The old API is kept until all drivers have been converted to
the new API.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 326 +++++++++++++++++++++++++++++++---
 drivers/usb/typec/ucsi/ucsi.h |  57 ++++++
 2 files changed, 354 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index edd722fb88b8..2ba890327b9d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -98,6 +98,98 @@ static int ucsi_ack(struct ucsi *ucsi, u8 ack)
 	return ret;
 }
 
+static int ucsi_acknowledge_command(struct ucsi *ucsi)
+{
+	u64 ctrl;
+
+	ctrl = UCSI_ACK_CC_CI;
+	ctrl |= UCSI_ACK_COMMAND_COMPLETE;
+
+	return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+}
+
+static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
+{
+	u64 ctrl;
+
+	ctrl = UCSI_ACK_CC_CI;
+	ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
+
+	return ucsi->ops->async_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+}
+
+static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
+
+static int ucsi_read_error(struct ucsi *ucsi)
+{
+	u16 error;
+	int ret;
+
+	/* Acknowlege the command that failed */
+	ret = ucsi_acknowledge_command(ucsi);
+	if (ret)
+		return ret;
+
+	ret = ucsi_exec_command(ucsi, UCSI_GET_ERROR_STATUS);
+	if (ret < 0)
+		return ret;
+
+	ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, &error, sizeof(error));
+	if (ret)
+		return ret;
+
+	switch (error) {
+	case UCSI_ERROR_INCOMPATIBLE_PARTNER:
+		return -EOPNOTSUPP;
+	case UCSI_ERROR_CC_COMMUNICATION_ERR:
+		return -ECOMM;
+	case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
+		return -EPROTO;
+	case UCSI_ERROR_DEAD_BATTERY:
+		dev_warn(ucsi->dev, "Dead battery condition!\n");
+		return -EPERM;
+	/* The following mean a bug in this driver */
+	case UCSI_ERROR_INVALID_CON_NUM:
+	case UCSI_ERROR_UNREGONIZED_CMD:
+	case UCSI_ERROR_INVALID_CMD_ARGUMENT:
+		dev_err(ucsi->dev, "possible UCSI driver bug (0x%x)\n", error);
+		return -EINVAL;
+	default:
+		dev_err(ucsi->dev, "%s: error without status\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
+{
+	u32 cci;
+	int ret;
+
+	ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
+	if (ret)
+		return ret;
+
+	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret)
+		return ret;
+
+	if (cci & UCSI_CCI_BUSY)
+		return -EBUSY;
+
+	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
+		return -EIO;
+
+	if (cci & UCSI_CCI_NOT_SUPPORTED)
+		return -EOPNOTSUPP;
+
+	if (cci & UCSI_CCI_ERROR)
+		return ucsi_read_error(ucsi);
+
+	return UCSI_CCI_LENGTH(cci);
+}
+
 static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 			    void *data, size_t size)
 {
@@ -106,6 +198,26 @@ static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 	u16 error;
 	int ret;
 
+	if (ucsi->ops) {
+		ret = ucsi_exec_command(ucsi, ctrl->raw_cmd);
+		if (ret < 0)
+			return ret;
+
+		data_length = ret;
+
+		if (data) {
+			ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size);
+			if (ret)
+				return ret;
+		}
+
+		ret = ucsi_acknowledge_command(ucsi);
+		if (ret)
+			return ret;
+
+		return data_length;
+	}
+
 	ret = ucsi_command(ucsi, ctrl);
 	if (ret)
 		goto err;
@@ -518,7 +630,7 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 		ucsi_altmode_update_active(con);
 }
 
-static void ucsi_connector_change(struct work_struct *work)
+static void ucsi_handle_connector_change(struct work_struct *work)
 {
 	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
 						  work);
@@ -580,7 +692,10 @@ static void ucsi_connector_change(struct work_struct *work)
 	if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
 		ucsi_partner_change(con);
 
-	ret = ucsi_ack(ucsi, UCSI_ACK_EVENT);
+	if (ucsi->ops)
+		ret = ucsi_acknowledge_connector_change(ucsi);
+	else
+		ret = ucsi_ack(ucsi, UCSI_ACK_EVENT);
 	if (ret)
 		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
 
@@ -591,6 +706,20 @@ static void ucsi_connector_change(struct work_struct *work)
 	mutex_unlock(&con->lock);
 }
 
+/**
+ * ucsi_connector_change - Process Connector Change Event
+ * @ucsi: UCSI Interface
+ * @num: Connector number
+ */
+void ucsi_connector_change(struct ucsi *ucsi, u8 num)
+{
+	struct ucsi_connector *con = &ucsi->connector[num - 1];
+
+	if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
+		schedule_work(&con->work);
+}
+EXPORT_SYMBOL_GPL(ucsi_connector_change);
+
 /**
  * ucsi_notify - PPM notification handler
  * @ucsi: Source UCSI Interface for the notifications
@@ -647,6 +776,39 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 	unsigned long tmo;
 	int ret;
 
+	if (ucsi->ops) {
+		u64 command = UCSI_PPM_RESET;
+		u32 cci;
+
+		ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
+					     sizeof(command));
+		if (ret < 0)
+			return ret;
+
+		tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+
+		do {
+			if (time_is_before_jiffies(tmo))
+				return -ETIMEDOUT;
+
+			ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+			if (ret)
+				return ret;
+
+			if (cci & ~UCSI_CCI_RESET_COMPLETE) {
+				ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL,
+							     &command,
+							     sizeof(command));
+				if (ret < 0)
+					return ret;
+			}
+
+			msleep(20);
+		} while (!(cci & UCSI_CCI_RESET_COMPLETE));
+
+		return 0;
+	}
+
 	ctrl.raw_cmd = 0;
 	ctrl.cmd.cmd = UCSI_PPM_RESET;
 	trace_ucsi_command(&ctrl);
@@ -807,7 +969,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	struct ucsi_control ctrl;
 	int ret;
 
-	INIT_WORK(&con->work, ucsi_connector_change);
+	INIT_WORK(&con->work, ucsi_handle_connector_change);
 	init_completion(&con->complete);
 	mutex_init(&con->lock);
 	con->num = index + 1;
@@ -898,9 +1060,14 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	return 0;
 }
 
-static void ucsi_init(struct work_struct *work)
+/**
+ * ucsi_init - Initialize UCSI interface
+ * @ucsi: UCSI to be initialized
+ *
+ * Registers all ports @ucsi has and enables all notification events.
+ */
+int ucsi_init(struct ucsi *ucsi)
 {
-	struct ucsi *ucsi = container_of(work, struct ucsi, work);
 	struct ucsi_connector *con;
 	struct ucsi_control ctrl;
 	int ret;
@@ -956,7 +1123,7 @@ static void ucsi_init(struct work_struct *work)
 
 	mutex_unlock(&ucsi->ppm_lock);
 
-	return;
+	return 0;
 
 err_unregister:
 	for (con = ucsi->connector; con->port; con++) {
@@ -970,49 +1137,106 @@ static void ucsi_init(struct work_struct *work)
 	ucsi_reset_ppm(ucsi);
 err:
 	mutex_unlock(&ucsi->ppm_lock);
-	dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ucsi_init);
+
+static void ucsi_init_work(struct work_struct *work)
+{
+	struct ucsi *ucsi = container_of(work, struct ucsi, work);
+	int ret;
+
+	ret = ucsi_init(ucsi);
+	if (ret)
+		dev_err(ucsi->dev, "PPM init failed (%d)\n", ret);
 }
 
 /**
- * ucsi_register_ppm - Register UCSI PPM Interface
- * @dev: Device interface to the PPM
- * @ppm: The PPM interface
- *
- * Allocates UCSI instance, associates it with @ppm and returns it to the
- * caller, and schedules initialization of the interface.
+ * ucsi_get_drvdata - Return private driver data pointer
+ * @ucsi: UCSI interface
  */
-struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
+void *ucsi_get_drvdata(struct ucsi *ucsi)
+{
+	return ucsi->driver_data;
+}
+EXPORT_SYMBOL_GPL(ucsi_get_drvdata);
+
+/**
+ * ucsi_get_drvdata - Assign private driver data pointer
+ * @ucsi: UCSI interface
+ * @data: Private data pointer
+ */
+void ucsi_set_drvdata(struct ucsi *ucsi, void *data)
+{
+	ucsi->driver_data = data;
+}
+EXPORT_SYMBOL_GPL(ucsi_set_drvdata);
+
+/**
+ * ucsi_create - Allocate UCSI instance
+ * @dev: Device interface to the PPM (Platform Policy Manager)
+ * @ops: I/O routines
+ */
+struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
 {
 	struct ucsi *ucsi;
 
+	if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
+		return ERR_PTR(-EINVAL);
+
 	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
 	if (!ucsi)
 		return ERR_PTR(-ENOMEM);
 
-	INIT_WORK(&ucsi->work, ucsi_init);
-	init_completion(&ucsi->complete);
+	INIT_WORK(&ucsi->work, ucsi_init_work);
 	mutex_init(&ucsi->ppm_lock);
-
 	ucsi->dev = dev;
-	ucsi->ppm = ppm;
+	ucsi->ops = ops;
+
+	return ucsi;
+}
+EXPORT_SYMBOL_GPL(ucsi_create);
+
+/**
+ * ucsi_destroy - Free UCSI instance
+ * @ucsi: UCSI instance to be freed
+ */
+void ucsi_destroy(struct ucsi *ucsi)
+{
+	kfree(ucsi);
+}
+EXPORT_SYMBOL_GPL(ucsi_destroy);
+
+/**
+ * ucsi_register - Register UCSI interface
+ * @ucsi: UCSI instance
+ */
+int ucsi_register(struct ucsi *ucsi)
+{
+	u16 version;
+	int ret;
+
+	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &version, sizeof(version));
+	if (ret)
+		return ret;
+
+	if (!version)
+		return -ENODEV;
 
-	/*
-	 * Communication with the PPM takes a lot of time. It is not reasonable
-	 * to initialize the driver here. Using a work for now.
-	 */
 	queue_work(system_long_wq, &ucsi->work);
 
-	return ucsi;
+	return 0;
 }
-EXPORT_SYMBOL_GPL(ucsi_register_ppm);
+EXPORT_SYMBOL_GPL(ucsi_register);
 
 /**
- * ucsi_unregister_ppm - Unregister UCSI PPM Interface
- * @ucsi: struct ucsi associated with the PPM
+ * ucsi_unregister - Unregister UCSI interface
+ * @ucsi: UCSI interface to be unregistered
  *
- * Unregister UCSI PPM that was created with ucsi_register().
+ * Unregister UCSI interface that was created with ucsi_register().
  */
-void ucsi_unregister_ppm(struct ucsi *ucsi)
+void ucsi_unregister(struct ucsi *ucsi)
 {
 	struct ucsi_control ctrl;
 	int i;
@@ -1035,7 +1259,51 @@ void ucsi_unregister_ppm(struct ucsi *ucsi)
 	ucsi_reset_ppm(ucsi);
 
 	kfree(ucsi->connector);
-	kfree(ucsi);
+}
+EXPORT_SYMBOL_GPL(ucsi_unregister);
+
+/**
+ * ucsi_register_ppm - Register UCSI PPM Interface
+ * @dev: Device interface to the PPM
+ * @ppm: The PPM interface
+ *
+ * Allocates UCSI instance, associates it with @ppm and returns it to the
+ * caller, and schedules initialization of the interface.
+ */
+struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
+{
+	struct ucsi *ucsi;
+
+	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
+	if (!ucsi)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_WORK(&ucsi->work, ucsi_init_work);
+	init_completion(&ucsi->complete);
+	mutex_init(&ucsi->ppm_lock);
+
+	ucsi->dev = dev;
+	ucsi->ppm = ppm;
+
+	/*
+	 * Communication with the PPM takes a lot of time. It is not reasonable
+	 * to initialize the driver here. Using a work for now.
+	 */
+	queue_work(system_long_wq, &ucsi->work);
+
+	return ucsi;
+}
+EXPORT_SYMBOL_GPL(ucsi_register_ppm);
+
+/**
+ * ucsi_unregister_ppm - Unregister UCSI PPM Interface
+ * @ucsi: struct ucsi associated with the PPM
+ *
+ * Unregister UCSI PPM that was created with ucsi_register().
+ */
+void ucsi_unregister_ppm(struct ucsi *ucsi)
+{
+	ucsi_unregister(ucsi);
 }
 EXPORT_SYMBOL_GPL(ucsi_unregister_ppm);
 
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index de87d0b8319d..3e9a4ba912e9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -10,6 +10,56 @@
 
 /* -------------------------------------------------------------------------- */
 
+struct ucsi;
+
+/* UCSI offsets (Bytes) */
+#define UCSI_VERSION			0
+#define UCSI_CCI			4
+#define UCSI_CONTROL			8
+#define UCSI_MESSAGE_IN			16
+#define UCSI_MESSAGE_OUT		32
+
+/* Command Status and Connector Change Indication (CCI) bits */
+#define UCSI_CCI_CONNECTOR(_c_)		(((_c_) & GENMASK(7, 0)) >> 1)
+#define UCSI_CCI_LENGTH(_c_)		(((_c_) & GENMASK(15, 8)) >> 8)
+#define UCSI_CCI_NOT_SUPPORTED		BIT(25)
+#define UCSI_CCI_CANCEL_COMPLETE	BIT(26)
+#define UCSI_CCI_RESET_COMPLETE		BIT(27)
+#define UCSI_CCI_BUSY			BIT(28)
+#define UCSI_CCI_ACK_COMPLETE		BIT(29)
+#define UCSI_CCI_ERROR			BIT(30)
+#define UCSI_CCI_COMMAND_COMPLETE	BIT(31)
+
+/**
+ * struct ucsi_operations - UCSI I/O operations
+ * @read: Read operation
+ * @sync_write: Blocking write operation
+ * @async_write: Non-blocking write operation
+ *
+ * Read and write routines for UCSI interface. @sync_write must wait for the
+ * Command Completion Event from the PPM before returning, and @async_write must
+ * return immediately after sending the data to the PPM.
+ */
+struct ucsi_operations {
+	int (*read)(struct ucsi *ucsi, unsigned int offset,
+		    void *val, size_t val_len);
+	int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
+			  const void *val, size_t val_len);
+	int (*async_write)(struct ucsi *ucsi, unsigned int offset,
+			   const void *val, size_t val_len);
+};
+
+struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
+void ucsi_destroy(struct ucsi *ucsi);
+int ucsi_register(struct ucsi *ucsi);
+void ucsi_unregister(struct ucsi *ucsi);
+void *ucsi_get_drvdata(struct ucsi *ucsi);
+void ucsi_set_drvdata(struct ucsi *ucsi, void *data);
+
+void ucsi_connector_change(struct ucsi *ucsi, u8 num);
+
+/* -------------------------------------------------------------------------- */
+
 /* Command Status and Connector Change Indication (CCI) data structure */
 struct ucsi_cci {
 	u8:1; /* reserved */
@@ -207,6 +257,10 @@ struct ucsi_control {
 #define UCSI_ACK_EVENT			1
 #define UCSI_ACK_CMD			2
 
+/* Bits for ACK CC or CI */
+#define UCSI_ACK_CONNECTOR_CHANGE		BIT(16)
+#define UCSI_ACK_COMMAND_COMPLETE		BIT(17)
+
 /* Bits for SET_NOTIFICATION_ENABLE command */
 #define UCSI_ENABLE_NTFY_CMD_COMPLETE		BIT(0)
 #define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE	BIT(1)
@@ -385,6 +439,9 @@ enum ucsi_status {
 struct ucsi {
 	struct device *dev;
 	struct ucsi_ppm *ppm;
+	struct driver_data *driver_data;
+
+	const struct ucsi_operations *ops;
 
 	enum ucsi_status status;
 	struct completion complete;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 10/14] usb: typec: ucsi: acpi: Move to the new API
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (8 preceding siblings ...)
  2019-09-26 10:07 ` [PATCH 09/14] usb: typec: ucsi: Simplified interface registration and I/O API Heikki Krogerus
@ 2019-09-26 14:25 ` Heikki Krogerus
  2019-09-26 14:25 ` [PATCH 11/14] usb: typec: ucsi: ccg: " Heikki Krogerus
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 14:25 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Replacing the old "cmd" and "sync" callbacks with an
implementation of struct ucsi_operations. The ACPI
notification (interrupt) handler will from now on read the
CCI (Command Status and Connector Change Indication)
register, and call ucsi_connector_change() function and/or
complete pending command completions based on it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 96 ++++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index a18112a83fae..876510acbf9f 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -19,7 +19,9 @@
 struct ucsi_acpi {
 	struct device *dev;
 	struct ucsi *ucsi;
-	struct ucsi_ppm ppm;
+	void __iomem *base;
+	struct completion complete;
+	unsigned long flags;
 	guid_t guid;
 };
 
@@ -39,27 +41,78 @@ static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
 	return 0;
 }
 
-static int ucsi_acpi_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
+static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int offset,
+			  void *val, size_t val_len)
 {
-	struct ucsi_acpi *ua = container_of(ppm, struct ucsi_acpi, ppm);
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+	int ret;
+
+	ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+	if (ret)
+		return ret;
 
-	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
+	memcpy(val, ua->base + offset, val_len);
+
+	return 0;
+}
+
+static int ucsi_acpi_async_write(struct ucsi *ucsi, unsigned int offset,
+				 const void *val, size_t val_len)
+{
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+
+	memcpy(ua->base + offset, val, val_len);
 
 	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_WRITE);
 }
 
-static int ucsi_acpi_sync(struct ucsi_ppm *ppm)
+static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
+				const void *val, size_t val_len)
 {
-	struct ucsi_acpi *ua = container_of(ppm, struct ucsi_acpi, ppm);
+	struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
+	int ret;
+
+	set_bit(COMMAND_PENDING, &ua->flags);
+
+	ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
+	if (ret)
+		goto out_clear_bit;
+
+	if (!wait_for_completion_timeout(&ua->complete, msecs_to_jiffies(5000)))
+		ret = -ETIMEDOUT;
 
-	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+out_clear_bit:
+	clear_bit(COMMAND_PENDING, &ua->flags);
+
+	return ret;
 }
 
+static const struct ucsi_operations ucsi_acpi_ops = {
+	.read = ucsi_acpi_read,
+	.sync_write = ucsi_acpi_sync_write,
+	.async_write = ucsi_acpi_async_write
+};
+
 static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ucsi_acpi *ua = data;
+	u32 cci;
+	int ret;
+
+	ret = ucsi_acpi_read(ua->ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret) {
+		dev_err(ua->dev, "failed to read CCI\n");
+		return;
+	}
+
+	if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
 
-	ucsi_notify(ua->ucsi);
+	if (test_bit(COMMAND_PENDING, &ua->flags) &&
+	    (cci & (UCSI_CCI_BUSY |
+		    UCSI_CCI_ACK_COMPLETE |
+		    UCSI_CCI_COMMAND_COMPLETE)))
+		complete(&ua->complete);
 }
 
 static int ucsi_acpi_probe(struct platform_device *pdev)
@@ -90,35 +143,39 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
 	 * it can not be requested here, and we can not use
 	 * devm_ioremap_resource().
 	 */
-	ua->ppm.data = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!ua->ppm.data)
+	ua->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!ua->base)
 		return -ENOMEM;
 
-	if (!ua->ppm.data->version)
-		return -ENODEV;
-
 	ret = guid_parse(UCSI_DSM_UUID, &ua->guid);
 	if (ret)
 		return ret;
 
-	ua->ppm.cmd = ucsi_acpi_cmd;
-	ua->ppm.sync = ucsi_acpi_sync;
+	init_completion(&ua->complete);
 	ua->dev = &pdev->dev;
 
+	ua->ucsi = ucsi_create(&pdev->dev, &ucsi_acpi_ops);
+	if (IS_ERR(ua->ucsi))
+		return PTR_ERR(ua->ucsi);
+
+	ucsi_set_drvdata(ua->ucsi, ua);
+
 	status = acpi_install_notify_handler(ACPI_HANDLE(&pdev->dev),
 					     ACPI_DEVICE_NOTIFY,
 					     ucsi_acpi_notify, ua);
 	if (ACPI_FAILURE(status)) {
 		dev_err(&pdev->dev, "failed to install notify handler\n");
+		ucsi_destroy(ua->ucsi);
 		return -ENODEV;
 	}
 
-	ua->ucsi = ucsi_register_ppm(&pdev->dev, &ua->ppm);
-	if (IS_ERR(ua->ucsi)) {
+	ret = ucsi_register(ua->ucsi);
+	if (ret) {
 		acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev),
 					   ACPI_DEVICE_NOTIFY,
 					   ucsi_acpi_notify);
-		return PTR_ERR(ua->ucsi);
+		ucsi_destroy(ua->ucsi);
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, ua);
@@ -130,7 +187,8 @@ static int ucsi_acpi_remove(struct platform_device *pdev)
 {
 	struct ucsi_acpi *ua = platform_get_drvdata(pdev);
 
-	ucsi_unregister_ppm(ua->ucsi);
+	ucsi_unregister(ua->ucsi);
+	ucsi_destroy(ua->ucsi);
 
 	acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev), ACPI_DEVICE_NOTIFY,
 				   ucsi_acpi_notify);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 11/14] usb: typec: ucsi: ccg: Move to the new API
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (9 preceding siblings ...)
  2019-09-26 14:25 ` [PATCH 10/14] usb: typec: ucsi: acpi: Move to the new API Heikki Krogerus
@ 2019-09-26 14:25 ` Heikki Krogerus
  2019-09-26 14:25 ` [PATCH 12/14] usb: typec: ucsi: Remove the old API Heikki Krogerus
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 14:25 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Replacing the old "cmd" and "sync" callbacks with an
implementation of struct ucsi_operations. The interrupt
handler will from now on read the CCI (Command Status and
Connector Change Indication) register, and call
ucsi_connector_change() function and/or complete pending
command completions based on it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 172 +++++++++++++++---------------
 1 file changed, 87 insertions(+), 85 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index d772fce51905..bbe87a71295c 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -176,8 +176,8 @@ struct ccg_resp {
 struct ucsi_ccg {
 	struct device *dev;
 	struct ucsi *ucsi;
-	struct ucsi_ppm ppm;
 	struct i2c_client *client;
+
 	struct ccg_dev_info info;
 	/* version info for boot, primary and secondary */
 	struct version_info version[FW2 + 1];
@@ -196,6 +196,8 @@ struct ucsi_ccg {
 	/* fw build with vendor information */
 	u16 fw_build;
 	struct work_struct pm_work;
+
+	struct completion complete;
 };
 
 static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
@@ -243,7 +245,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	return 0;
 }
 
-static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+static int ccg_write(struct ucsi_ccg *uc, u16 rab, const u8 *data, u32 len)
 {
 	struct i2c_client *client = uc->client;
 	unsigned char *buf;
@@ -317,88 +319,91 @@ static int ucsi_ccg_init(struct ucsi_ccg *uc)
 	return -ETIMEDOUT;
 }
 
-static int ucsi_ccg_send_data(struct ucsi_ccg *uc)
+static int ucsi_ccg_read(struct ucsi *ucsi, unsigned int offset,
+			 void *val, size_t val_len)
 {
-	u8 *ppm = (u8 *)uc->ppm.data;
-	int status;
-	u16 rab;
+	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
 
-	rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, message_out));
-	status = ccg_write(uc, rab, ppm +
-			   offsetof(struct ucsi_data, message_out),
-			   sizeof(uc->ppm.data->message_out));
-	if (status < 0)
-		return status;
-
-	rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, ctrl));
-	return ccg_write(uc, rab, ppm + offsetof(struct ucsi_data, ctrl),
-			 sizeof(uc->ppm.data->ctrl));
+	return ccg_read(ucsi_get_drvdata(ucsi), reg, val, val_len);
 }
 
-static int ucsi_ccg_recv_data(struct ucsi_ccg *uc)
+static int ucsi_ccg_async_write(struct ucsi *ucsi, unsigned int offset,
+				const void *val, size_t val_len)
 {
-	u8 *ppm = (u8 *)uc->ppm.data;
-	int status;
-	u16 rab;
+	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(offset);
 
-	rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, cci));
-	status = ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, cci),
-			  sizeof(uc->ppm.data->cci));
-	if (status < 0)
-		return status;
-
-	rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, message_in));
-	return ccg_read(uc, rab, ppm + offsetof(struct ucsi_data, message_in),
-			sizeof(uc->ppm.data->message_in));
+	return ccg_write(ucsi_get_drvdata(ucsi), reg, val, val_len);
 }
 
-static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc)
+static int ucsi_ccg_sync_write(struct ucsi *ucsi, unsigned int offset,
+			       const void *val, size_t val_len)
 {
-	int status;
-	unsigned char data;
+	struct ucsi_ccg *uc = ucsi_get_drvdata(ucsi);
+	int ret;
 
-	status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
-	if (status < 0)
-		return status;
+	mutex_lock(&uc->lock);
+	pm_runtime_get_sync(uc->dev);
+	set_bit(DEV_CMD_PENDING, &uc->flags);
 
-	return ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
-}
+	ret = ucsi_ccg_async_write(ucsi, offset, val, val_len);
+	if (ret)
+		goto err_clear_bit;
 
-static int ucsi_ccg_sync(struct ucsi_ppm *ppm)
-{
-	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
-	int status;
+	if (!wait_for_completion_timeout(&uc->complete, msecs_to_jiffies(5000)))
+		ret = -ETIMEDOUT;
 
-	status = ucsi_ccg_recv_data(uc);
-	if (status < 0)
-		return status;
+err_clear_bit:
+	clear_bit(DEV_CMD_PENDING, &uc->flags);
+	pm_runtime_put_sync(uc->dev);
+	mutex_unlock(&uc->lock);
 
-	/* ack interrupt to allow next command to run */
-	return ucsi_ccg_ack_interrupt(uc);
+	return ret;
 }
 
-static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
-{
-	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
-
-	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
-	return ucsi_ccg_send_data(uc);
-}
+static const struct ucsi_operations ucsi_ccg_ops = {
+	.read = ucsi_ccg_read,
+	.sync_write = ucsi_ccg_sync_write,
+	.async_write = ucsi_ccg_async_write
+};
 
 static irqreturn_t ccg_irq_handler(int irq, void *data)
 {
+	u16 reg = CCGX_RAB_UCSI_DATA_BLOCK(UCSI_CCI);
 	struct ucsi_ccg *uc = data;
+	u8 intr_reg;
+	u32 cci;
+	int ret;
+
+	ret = ccg_read(uc, CCGX_RAB_INTR_REG, &intr_reg, sizeof(intr_reg));
+	if (ret)
+		return ret;
+
+	ret = ccg_read(uc, reg, (void *)&cci, sizeof(cci));
+	if (ret) {
+		dev_err(uc->dev, "failed to read CCI\n");
+		goto err_clear_irq;
+	}
+
+	if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(uc->ucsi, UCSI_CCI_CONNECTOR(cci));
 
-	ucsi_notify(uc->ucsi);
+	if (test_bit(DEV_CMD_PENDING, &uc->flags) &&
+	    (cci & (UCSI_CCI_BUSY |
+		    UCSI_CCI_ACK_COMPLETE |
+		    UCSI_CCI_COMMAND_COMPLETE)))
+		complete(&uc->complete);
+
+err_clear_irq:
+	ret =  ccg_write(uc, CCGX_RAB_INTR_REG, &intr_reg, sizeof(intr_reg));
+	if (ret)
+		dev_err(uc->dev, "failed to clear interrupt\n");
 
 	return IRQ_HANDLED;
 }
 
 static void ccg_pm_workaround_work(struct work_struct *pm_work)
 {
-	struct ucsi_ccg *uc = container_of(pm_work, struct ucsi_ccg, pm_work);
-
-	ucsi_notify(uc->ucsi);
+	ccg_irq_handler(0, container_of(pm_work, struct ucsi_ccg, pm_work));
 }
 
 static int get_fw_info(struct ucsi_ccg *uc)
@@ -1027,10 +1032,10 @@ static int ccg_restart(struct ucsi_ccg *uc)
 		return status;
 	}
 
-	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
-	if (IS_ERR(uc->ucsi)) {
-		dev_err(uc->dev, "ucsi_register_ppm failed\n");
-		return PTR_ERR(uc->ucsi);
+	status = ucsi_register(uc->ucsi);
+	if (status) {
+		dev_err(uc->dev, "failed to register the interface\n");
+		return status;
 	}
 
 	return 0;
@@ -1047,7 +1052,7 @@ static void ccg_update_firmware(struct work_struct *work)
 		return;
 
 	if (flash_mode != FLASH_NOT_NEEDED) {
-		ucsi_unregister_ppm(uc->ucsi);
+		ucsi_unregister(uc->ucsi);
 		free_irq(uc->irq, uc);
 
 		ccg_fw_update(uc, flash_mode);
@@ -1091,21 +1096,15 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct ucsi_ccg *uc;
 	int status;
-	u16 rab;
 
 	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
 	if (!uc)
 		return -ENOMEM;
 
-	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data), GFP_KERNEL);
-	if (!uc->ppm.data)
-		return -ENOMEM;
-
-	uc->ppm.cmd = ucsi_ccg_cmd;
-	uc->ppm.sync = ucsi_ccg_sync;
 	uc->dev = dev;
 	uc->client = client;
 	mutex_init(&uc->lock);
+	init_completion(&uc->complete);
 	INIT_WORK(&uc->work, ccg_update_firmware);
 	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
 
@@ -1133,30 +1132,25 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	if (uc->info.mode & CCG_DEVINFO_PDPORTS_MASK)
 		uc->port_num++;
 
+	uc->ucsi = ucsi_create(dev, &ucsi_ccg_ops);
+	if (IS_ERR(uc->ucsi))
+		return PTR_ERR(uc->ucsi);
+
+	ucsi_set_drvdata(uc->ucsi, uc);
+
 	status = request_threaded_irq(client->irq, NULL, ccg_irq_handler,
 				      IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
 				      dev_name(dev), uc);
 	if (status < 0) {
 		dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
-		return status;
+		goto out_ucsi_destroy;
 	}
 
 	uc->irq = client->irq;
 
-	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
-	if (IS_ERR(uc->ucsi)) {
-		dev_err(uc->dev, "ucsi_register_ppm failed\n");
-		return PTR_ERR(uc->ucsi);
-	}
-
-	rab = CCGX_RAB_UCSI_DATA_BLOCK(offsetof(struct ucsi_data, version));
-	status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
-			  offsetof(struct ucsi_data, version),
-			  sizeof(uc->ppm.data->version));
-	if (status < 0) {
-		ucsi_unregister_ppm(uc->ucsi);
-		return status;
-	}
+	status = ucsi_register(uc->ucsi);
+	if (status)
+		goto out_free_irq;
 
 	i2c_set_clientdata(client, uc);
 
@@ -1167,6 +1161,13 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	pm_runtime_idle(uc->dev);
 
 	return 0;
+
+out_free_irq:
+	free_irq(uc->irq, uc);
+out_ucsi_destroy:
+	ucsi_destroy(uc->ucsi);
+
+	return status;
 }
 
 static int ucsi_ccg_remove(struct i2c_client *client)
@@ -1175,8 +1176,9 @@ static int ucsi_ccg_remove(struct i2c_client *client)
 
 	cancel_work_sync(&uc->pm_work);
 	cancel_work_sync(&uc->work);
-	ucsi_unregister_ppm(uc->ucsi);
 	pm_runtime_disable(uc->dev);
+	ucsi_unregister(uc->ucsi);
+	ucsi_destroy(uc->ucsi);
 	free_irq(uc->irq, uc);
 
 	return 0;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 12/14] usb: typec: ucsi: Remove the old API
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (10 preceding siblings ...)
  2019-09-26 14:25 ` [PATCH 11/14] usb: typec: ucsi: ccg: " Heikki Krogerus
@ 2019-09-26 14:25 ` Heikki Krogerus
  2019-09-26 14:25 ` [PATCH 13/14] usb: typec: ucsi: Remove struct ucsi_control Heikki Krogerus
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 14:25 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

The drivers now only use the new API, so removing the old one.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/displayport.c |   8 +-
 drivers/usb/typec/ucsi/trace.h       |  17 --
 drivers/usb/typec/ucsi/ucsi.c        | 346 +++------------------------
 drivers/usb/typec/ucsi/ucsi.h        |  41 ----
 4 files changed, 41 insertions(+), 371 deletions(-)

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 6c103697c582..132007cbb96c 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -48,8 +48,10 @@ struct ucsi_dp {
 static int ucsi_displayport_enter(struct typec_altmode *alt)
 {
 	struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
+	struct ucsi *ucsi = dp->con->ucsi;
 	struct ucsi_control ctrl;
 	u8 cur = 0;
+	u16 ver;
 	int ret;
 
 	mutex_lock(&dp->con->lock);
@@ -66,7 +68,11 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 	UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
 	ret = ucsi_send_command(dp->con->ucsi, &ctrl, &cur, sizeof(cur));
 	if (ret < 0) {
-		if (dp->con->ucsi->ppm->data->version > 0x0100) {
+		ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ver, sizeof(ver));
+		if (ret)
+			return ret;
+
+		if (ver > 0x0100) {
 			mutex_unlock(&dp->con->lock);
 			return ret;
 		}
diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
index 783ec9c72055..6e3d510b236e 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -75,23 +75,6 @@ DEFINE_EVENT(ucsi_log_command, ucsi_reset_ppm,
 	TP_ARGS(ctrl, ret)
 );
 
-DECLARE_EVENT_CLASS(ucsi_log_cci,
-	TP_PROTO(u32 cci),
-	TP_ARGS(cci),
-	TP_STRUCT__entry(
-		__field(u32, cci)
-	),
-	TP_fast_assign(
-		__entry->cci = cci;
-	),
-	TP_printk("CCI=%08x %s", __entry->cci, ucsi_cci_str(__entry->cci))
-);
-
-DEFINE_EVENT(ucsi_log_cci, ucsi_notify,
-	TP_PROTO(u32 cci),
-	TP_ARGS(cci)
-);
-
 DECLARE_EVENT_CLASS(ucsi_log_connector_status,
 	TP_PROTO(int port, struct ucsi_connector_status *status),
 	TP_ARGS(port, status),
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 2ba890327b9d..ea149a115834 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -36,68 +36,6 @@
  */
 #define UCSI_SWAP_TIMEOUT_MS	5000
 
-static inline int ucsi_sync(struct ucsi *ucsi)
-{
-	if (ucsi->ppm && ucsi->ppm->sync)
-		return ucsi->ppm->sync(ucsi->ppm);
-	return 0;
-}
-
-static int ucsi_command(struct ucsi *ucsi, struct ucsi_control *ctrl)
-{
-	int ret;
-
-	trace_ucsi_command(ctrl);
-
-	set_bit(COMMAND_PENDING, &ucsi->flags);
-
-	ret = ucsi->ppm->cmd(ucsi->ppm, ctrl);
-	if (ret)
-		goto err_clear_flag;
-
-	if (!wait_for_completion_timeout(&ucsi->complete,
-					 msecs_to_jiffies(UCSI_TIMEOUT_MS))) {
-		dev_warn(ucsi->dev, "PPM NOT RESPONDING\n");
-		ret = -ETIMEDOUT;
-	}
-
-err_clear_flag:
-	clear_bit(COMMAND_PENDING, &ucsi->flags);
-
-	return ret;
-}
-
-static int ucsi_ack(struct ucsi *ucsi, u8 ack)
-{
-	struct ucsi_control ctrl;
-	int ret;
-
-	trace_ucsi_ack(ack);
-
-	set_bit(ACK_PENDING, &ucsi->flags);
-
-	UCSI_CMD_ACK(ctrl, ack);
-	ret = ucsi->ppm->cmd(ucsi->ppm, &ctrl);
-	if (ret)
-		goto out_clear_bit;
-
-	/* Waiting for ACK with ACK CMD, but not with EVENT for now */
-	if (ack == UCSI_ACK_EVENT)
-		goto out_clear_bit;
-
-	if (!wait_for_completion_timeout(&ucsi->complete,
-					 msecs_to_jiffies(UCSI_TIMEOUT_MS)))
-		ret = -ETIMEDOUT;
-
-out_clear_bit:
-	clear_bit(ACK_PENDING, &ucsi->flags);
-
-	if (ret)
-		dev_err(ucsi->dev, "%s: failed\n", __func__);
-
-	return ret;
-}
-
 static int ucsi_acknowledge_command(struct ucsi *ucsi)
 {
 	u64 ctrl;
@@ -193,115 +131,26 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 			    void *data, size_t size)
 {
-	struct ucsi_control _ctrl;
-	u8 data_length;
-	u16 error;
+	u8 length;
 	int ret;
 
-	if (ucsi->ops) {
-		ret = ucsi_exec_command(ucsi, ctrl->raw_cmd);
-		if (ret < 0)
-			return ret;
-
-		data_length = ret;
+	ret = ucsi_exec_command(ucsi, ctrl->raw_cmd);
+	if (ret < 0)
+		return ret;
 
-		if (data) {
-			ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size);
-			if (ret)
-				return ret;
-		}
+	length = ret;
 
-		ret = ucsi_acknowledge_command(ucsi);
+	if (data) {
+		ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size);
 		if (ret)
 			return ret;
-
-		return data_length;
 	}
 
-	ret = ucsi_command(ucsi, ctrl);
+	ret = ucsi_acknowledge_command(ucsi);
 	if (ret)
-		goto err;
-
-	switch (ucsi->status) {
-	case UCSI_IDLE:
-		ret = ucsi_sync(ucsi);
-		if (ret)
-			dev_warn(ucsi->dev, "%s: sync failed\n", __func__);
-
-		if (data)
-			memcpy(data, ucsi->ppm->data->message_in, size);
-
-		data_length = ucsi->ppm->data->cci.data_length;
-
-		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
-		if (!ret)
-			ret = data_length;
-		break;
-	case UCSI_BUSY:
-		/* The caller decides whether to cancel or not */
-		ret = -EBUSY;
-		break;
-	case UCSI_ERROR:
-		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
-		if (ret)
-			break;
-
-		_ctrl.raw_cmd = 0;
-		_ctrl.cmd.cmd = UCSI_GET_ERROR_STATUS;
-		ret = ucsi_command(ucsi, &_ctrl);
-		if (ret) {
-			dev_err(ucsi->dev, "reading error failed!\n");
-			break;
-		}
-
-		memcpy(&error, ucsi->ppm->data->message_in, sizeof(error));
-
-		/* Something has really gone wrong */
-		if (WARN_ON(ucsi->status == UCSI_ERROR)) {
-			ret = -ENODEV;
-			break;
-		}
-
-		ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
-		if (ret)
-			break;
-
-		switch (error) {
-		case UCSI_ERROR_INCOMPATIBLE_PARTNER:
-			ret = -EOPNOTSUPP;
-			break;
-		case UCSI_ERROR_CC_COMMUNICATION_ERR:
-			ret = -ECOMM;
-			break;
-		case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
-			ret = -EPROTO;
-			break;
-		case UCSI_ERROR_DEAD_BATTERY:
-			dev_warn(ucsi->dev, "Dead battery condition!\n");
-			ret = -EPERM;
-			break;
-		/* The following mean a bug in this driver */
-		case UCSI_ERROR_INVALID_CON_NUM:
-		case UCSI_ERROR_UNREGONIZED_CMD:
-		case UCSI_ERROR_INVALID_CMD_ARGUMENT:
-			dev_warn(ucsi->dev,
-				 "%s: possible UCSI driver bug - error 0x%x\n",
-				 __func__, error);
-			ret = -EINVAL;
-			break;
-		default:
-			dev_warn(ucsi->dev,
-				 "%s: error without status\n", __func__);
-			ret = -EIO;
-			break;
-		}
-		break;
-	}
-
-err:
-	trace_ucsi_run_command(ctrl, ret);
+		return ret;
 
-	return ret;
+	return length;
 }
 
 int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
@@ -332,6 +181,7 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 {
 	const struct typec_altmode *altmode = NULL;
 	struct ucsi_control ctrl;
+	u16 version;
 	int ret;
 	u8 cur;
 	int i;
@@ -339,7 +189,9 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 	UCSI_CMD_GET_CURRENT_CAM(ctrl, con->num);
 	ret = ucsi_run_command(con->ucsi, &ctrl, &cur, sizeof(cur));
 	if (ret < 0) {
-		if (con->ucsi->ppm->data->version > 0x0100) {
+		ret = con->ucsi->ops->read(con->ucsi, UCSI_VERSION, &version,
+					   sizeof(version));
+		if (ret || version > 0x0100) {
 			dev_err(con->ucsi->dev,
 				"GET_CURRENT_CAM command failed\n");
 			return;
@@ -692,10 +544,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 	if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
 		ucsi_partner_change(con);
 
-	if (ucsi->ops)
-		ret = ucsi_acknowledge_connector_change(ucsi);
-	else
-		ret = ucsi_ack(ucsi, UCSI_ACK_EVENT);
+	ret = ucsi_acknowledge_connector_change(ucsi);
 	if (ret)
 		dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
 
@@ -720,45 +569,6 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 }
 EXPORT_SYMBOL_GPL(ucsi_connector_change);
 
-/**
- * ucsi_notify - PPM notification handler
- * @ucsi: Source UCSI Interface for the notifications
- *
- * Handle notifications from PPM of @ucsi.
- */
-void ucsi_notify(struct ucsi *ucsi)
-{
-	struct ucsi_cci *cci;
-
-	/* There is no requirement to sync here, but no harm either. */
-	ucsi_sync(ucsi);
-
-	cci = &ucsi->ppm->data->cci;
-
-	if (cci->error)
-		ucsi->status = UCSI_ERROR;
-	else if (cci->busy)
-		ucsi->status = UCSI_BUSY;
-	else
-		ucsi->status = UCSI_IDLE;
-
-	if (cci->cmd_complete && test_bit(COMMAND_PENDING, &ucsi->flags)) {
-		complete(&ucsi->complete);
-	} else if (cci->ack_complete && test_bit(ACK_PENDING, &ucsi->flags)) {
-		complete(&ucsi->complete);
-	} else if (cci->connector_change) {
-		struct ucsi_connector *con;
-
-		con = &ucsi->connector[cci->connector_change - 1];
-
-		if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
-			schedule_work(&con->work);
-	}
-
-	trace_ucsi_notify(ucsi->ppm->data->raw_cci);
-}
-EXPORT_SYMBOL_GPL(ucsi_notify);
-
 /* -------------------------------------------------------------------------- */
 
 static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
@@ -772,82 +582,39 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
 
 static int ucsi_reset_ppm(struct ucsi *ucsi)
 {
-	struct ucsi_control ctrl;
+	u64 command = UCSI_PPM_RESET;
 	unsigned long tmo;
+	u32 cci;
 	int ret;
 
-	if (ucsi->ops) {
-		u64 command = UCSI_PPM_RESET;
-		u32 cci;
-
-		ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
-					     sizeof(command));
-		if (ret < 0)
-			return ret;
-
-		tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
-
-		do {
-			if (time_is_before_jiffies(tmo))
-				return -ETIMEDOUT;
-
-			ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
-			if (ret)
-				return ret;
-
-			if (cci & ~UCSI_CCI_RESET_COMPLETE) {
-				ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL,
-							     &command,
-							     sizeof(command));
-				if (ret < 0)
-					return ret;
-			}
-
-			msleep(20);
-		} while (!(cci & UCSI_CCI_RESET_COMPLETE));
-
-		return 0;
-	}
-
-	ctrl.raw_cmd = 0;
-	ctrl.cmd.cmd = UCSI_PPM_RESET;
-	trace_ucsi_command(&ctrl);
-	ret = ucsi->ppm->cmd(ucsi->ppm, &ctrl);
-	if (ret)
-		goto err;
+	ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command,
+				     sizeof(command));
+	if (ret < 0)
+		return ret;
 
 	tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
 
 	do {
-		/* Here sync is critical. */
-		ret = ucsi_sync(ucsi);
-		if (ret)
-			goto err;
+		if (time_is_before_jiffies(tmo))
+			return -ETIMEDOUT;
 
-		if (ucsi->ppm->data->cci.reset_complete)
-			break;
+		ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+		if (ret)
+			return ret;
 
 		/* If the PPM is still doing something else, reset it again. */
-		if (ucsi->ppm->data->raw_cci) {
-			dev_warn_ratelimited(ucsi->dev,
-				"Failed to reset PPM! Trying again..\n");
-
-			trace_ucsi_command(&ctrl);
-			ret = ucsi->ppm->cmd(ucsi->ppm, &ctrl);
-			if (ret)
-				goto err;
+		if (cci & ~UCSI_CCI_RESET_COMPLETE) {
+			ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL,
+						     &command,
+						     sizeof(command));
+			if (ret < 0)
+				return ret;
 		}
 
-		/* Letting the PPM settle down. */
 		msleep(20);
+	} while (!(cci & UCSI_CCI_RESET_COMPLETE));
 
-		ret = -ETIMEDOUT;
-	} while (time_is_after_jiffies(tmo));
-
-err:
-	trace_ucsi_reset_ppm(&ctrl, ret);
-
-	return ret;
+	return 0;
 }
 
 static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl)
@@ -1262,51 +1029,6 @@ void ucsi_unregister(struct ucsi *ucsi)
 }
 EXPORT_SYMBOL_GPL(ucsi_unregister);
 
-/**
- * ucsi_register_ppm - Register UCSI PPM Interface
- * @dev: Device interface to the PPM
- * @ppm: The PPM interface
- *
- * Allocates UCSI instance, associates it with @ppm and returns it to the
- * caller, and schedules initialization of the interface.
- */
-struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
-{
-	struct ucsi *ucsi;
-
-	ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
-	if (!ucsi)
-		return ERR_PTR(-ENOMEM);
-
-	INIT_WORK(&ucsi->work, ucsi_init_work);
-	init_completion(&ucsi->complete);
-	mutex_init(&ucsi->ppm_lock);
-
-	ucsi->dev = dev;
-	ucsi->ppm = ppm;
-
-	/*
-	 * Communication with the PPM takes a lot of time. It is not reasonable
-	 * to initialize the driver here. Using a work for now.
-	 */
-	queue_work(system_long_wq, &ucsi->work);
-
-	return ucsi;
-}
-EXPORT_SYMBOL_GPL(ucsi_register_ppm);
-
-/**
- * ucsi_unregister_ppm - Unregister UCSI PPM Interface
- * @ucsi: struct ucsi associated with the PPM
- *
- * Unregister UCSI PPM that was created with ucsi_register().
- */
-void ucsi_unregister_ppm(struct ucsi *ucsi)
-{
-	ucsi_unregister(ucsi);
-}
-EXPORT_SYMBOL_GPL(ucsi_unregister_ppm);
-
 MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@linux.intel.com>");
 MODULE_LICENSE("GPL v2");
 MODULE_DESCRIPTION("USB Type-C Connector System Software Interface driver");
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 3e9a4ba912e9..bb1df6cb241b 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -398,53 +398,12 @@ struct ucsi_connector_status {
 
 /* -------------------------------------------------------------------------- */
 
-struct ucsi;
-
-struct ucsi_data {
-	u16 version;
-	u16 reserved;
-	union {
-		u32 raw_cci;
-		struct ucsi_cci cci;
-	};
-	struct ucsi_control ctrl;
-	u32 message_in[4];
-	u32 message_out[4];
-} __packed;
-
-/*
- * struct ucsi_ppm - Interface to UCSI Platform Policy Manager
- * @data: memory location to the UCSI data structures
- * @cmd: UCSI command execution routine
- * @sync: Refresh UCSI mailbox (the data structures)
- */
-struct ucsi_ppm {
-	struct ucsi_data *data;
-	int (*cmd)(struct ucsi_ppm *, struct ucsi_control *);
-	int (*sync)(struct ucsi_ppm *);
-};
-
-struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm);
-void ucsi_unregister_ppm(struct ucsi *ucsi);
-void ucsi_notify(struct ucsi *ucsi);
-
-/* -------------------------------------------------------------------------- */
-
-enum ucsi_status {
-	UCSI_IDLE = 0,
-	UCSI_BUSY,
-	UCSI_ERROR,
-};
-
 struct ucsi {
 	struct device *dev;
-	struct ucsi_ppm *ppm;
 	struct driver_data *driver_data;
 
 	const struct ucsi_operations *ops;
 
-	enum ucsi_status status;
-	struct completion complete;
 	struct ucsi_capability cap;
 	struct ucsi_connector *connector;
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 13/14] usb: typec: ucsi: Remove struct ucsi_control
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (11 preceding siblings ...)
  2019-09-26 14:25 ` [PATCH 12/14] usb: typec: ucsi: Remove the old API Heikki Krogerus
@ 2019-09-26 14:25 ` Heikki Krogerus
  2019-09-26 14:25 ` [PATCH 14/14] usb: typec: ucsi: Remove all bit-fields Heikki Krogerus
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 14:25 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

That data structure was used for constructing the commands
before executing them, but it was never really useful. Using
the structure just complicated the driver. The commands are
64-bit wide, so it is enough to simply fill a u64 variable.
No data structures needed.

This simplifies the driver considerable and makes it much
easier to for example add support for big endian systems
later on.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/displayport.c |  18 +--
 drivers/usb/typec/ucsi/trace.c       |  11 --
 drivers/usb/typec/ucsi/trace.h       |  50 +-----
 drivers/usb/typec/ucsi/ucsi.c        | 110 +++++++------
 drivers/usb/typec/ucsi/ucsi.h        | 231 +++++----------------------
 5 files changed, 118 insertions(+), 302 deletions(-)

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 132007cbb96c..f8d3503933d0 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -49,7 +49,7 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 {
 	struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
 	struct ucsi *ucsi = dp->con->ucsi;
-	struct ucsi_control ctrl;
+	u64 command;
 	u8 cur = 0;
 	u16 ver;
 	int ret;
@@ -65,8 +65,8 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 		return -EOPNOTSUPP;
 	}
 
-	UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
-	ret = ucsi_send_command(dp->con->ucsi, &ctrl, &cur, sizeof(cur));
+	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
+	ret = ucsi_send_command(dp->con->ucsi, command, &cur, sizeof(cur));
 	if (ret < 0) {
 		ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ver, sizeof(ver));
 		if (ret)
@@ -107,7 +107,7 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 static int ucsi_displayport_exit(struct typec_altmode *alt)
 {
 	struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
-	struct ucsi_control ctrl;
+	u64 command;
 	int ret = 0;
 
 	mutex_lock(&dp->con->lock);
@@ -121,8 +121,8 @@ static int ucsi_displayport_exit(struct typec_altmode *alt)
 		goto out_unlock;
 	}
 
-	ctrl.raw_cmd = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, dp->offset, 0);
-	ret = ucsi_send_command(dp->con->ucsi, &ctrl, NULL, 0);
+	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 0, dp->offset, 0);
+	ret = ucsi_send_command(dp->con->ucsi, command, NULL, 0);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -176,14 +176,14 @@ static int ucsi_displayport_status_update(struct ucsi_dp *dp)
 static int ucsi_displayport_configure(struct ucsi_dp *dp)
 {
 	u32 pins = DP_CONF_GET_PIN_ASSIGN(dp->data.conf);
-	struct ucsi_control ctrl;
+	u64 command;
 
 	if (!dp->override)
 		return 0;
 
-	ctrl.raw_cmd = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp->offset, pins);
+	command = UCSI_CMD_SET_NEW_CAM(dp->con->num, 1, dp->offset, pins);
 
-	return ucsi_send_command(dp->con->ucsi, &ctrl, NULL, 0);
+	return ucsi_send_command(dp->con->ucsi, command, NULL, 0);
 }
 
 static int ucsi_displayport_vdm(struct typec_altmode *alt,
diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c
index 1dabafb74320..48ad1dc1b1b2 100644
--- a/drivers/usb/typec/ucsi/trace.c
+++ b/drivers/usb/typec/ucsi/trace.c
@@ -33,17 +33,6 @@ const char *ucsi_cmd_str(u64 raw_cmd)
 	return ucsi_cmd_strs[(cmd >= ARRAY_SIZE(ucsi_cmd_strs)) ? 0 : cmd];
 }
 
-static const char * const ucsi_ack_strs[] = {
-	[0]				= "",
-	[UCSI_ACK_EVENT]		= "event",
-	[UCSI_ACK_CMD]			= "command",
-};
-
-const char *ucsi_ack_str(u8 ack)
-{
-	return ucsi_ack_strs[(ack >= ARRAY_SIZE(ucsi_ack_strs)) ? 0 : ack];
-}
-
 const char *ucsi_cci_str(u32 cci)
 {
 	if (cci & GENMASK(7, 0)) {
diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
index 6e3d510b236e..2262229dae8e 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -10,54 +10,18 @@
 #include <linux/usb/typec_altmode.h>
 
 const char *ucsi_cmd_str(u64 raw_cmd);
-const char *ucsi_ack_str(u8 ack);
 const char *ucsi_cci_str(u32 cci);
 const char *ucsi_recipient_str(u8 recipient);
 
-DECLARE_EVENT_CLASS(ucsi_log_ack,
-	TP_PROTO(u8 ack),
-	TP_ARGS(ack),
-	TP_STRUCT__entry(
-		__field(u8, ack)
-	),
-	TP_fast_assign(
-		__entry->ack = ack;
-	),
-	TP_printk("ACK %s", ucsi_ack_str(__entry->ack))
-);
-
-DEFINE_EVENT(ucsi_log_ack, ucsi_ack,
-	TP_PROTO(u8 ack),
-	TP_ARGS(ack)
-);
-
-DECLARE_EVENT_CLASS(ucsi_log_control,
-	TP_PROTO(struct ucsi_control *ctrl),
-	TP_ARGS(ctrl),
-	TP_STRUCT__entry(
-		__field(u64, ctrl)
-	),
-	TP_fast_assign(
-		__entry->ctrl = ctrl->raw_cmd;
-	),
-	TP_printk("control=%08llx (%s)", __entry->ctrl,
-		ucsi_cmd_str(__entry->ctrl))
-);
-
-DEFINE_EVENT(ucsi_log_control, ucsi_command,
-	TP_PROTO(struct ucsi_control *ctrl),
-	TP_ARGS(ctrl)
-);
-
 DECLARE_EVENT_CLASS(ucsi_log_command,
-	TP_PROTO(struct ucsi_control *ctrl, int ret),
-	TP_ARGS(ctrl, ret),
+	TP_PROTO(u64 command, int ret),
+	TP_ARGS(command, ret),
 	TP_STRUCT__entry(
 		__field(u64, ctrl)
 		__field(int, ret)
 	),
 	TP_fast_assign(
-		__entry->ctrl = ctrl->raw_cmd;
+		__entry->ctrl = command;
 		__entry->ret = ret;
 	),
 	TP_printk("%s -> %s (err=%d)", ucsi_cmd_str(__entry->ctrl),
@@ -66,13 +30,13 @@ DECLARE_EVENT_CLASS(ucsi_log_command,
 );
 
 DEFINE_EVENT(ucsi_log_command, ucsi_run_command,
-	TP_PROTO(struct ucsi_control *ctrl, int ret),
-	TP_ARGS(ctrl, ret)
+	TP_PROTO(u64 command, int ret),
+	TP_ARGS(command, ret)
 );
 
 DEFINE_EVENT(ucsi_log_command, ucsi_reset_ppm,
-	TP_PROTO(struct ucsi_control *ctrl, int ret),
-	TP_ARGS(ctrl, ret)
+	TP_PROTO(u64 command, int ret),
+	TP_ARGS(command, ret)
 );
 
 DECLARE_EVENT_CLASS(ucsi_log_connector_status,
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ea149a115834..6fd4ff46d728 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -128,13 +128,13 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	return UCSI_CCI_LENGTH(cci);
 }
 
-static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+static int ucsi_run_command(struct ucsi *ucsi, u64 command,
 			    void *data, size_t size)
 {
 	u8 length;
 	int ret;
 
-	ret = ucsi_exec_command(ucsi, ctrl->raw_cmd);
+	ret = ucsi_exec_command(ucsi, command);
 	if (ret < 0)
 		return ret;
 
@@ -153,13 +153,13 @@ static int ucsi_run_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 	return length;
 }
 
-int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+int ucsi_send_command(struct ucsi *ucsi, u64 command,
 		      void *retval, size_t size)
 {
 	int ret;
 
 	mutex_lock(&ucsi->ppm_lock);
-	ret = ucsi_run_command(ucsi, ctrl, retval, size);
+	ret = ucsi_run_command(ucsi, command, retval, size);
 	mutex_unlock(&ucsi->ppm_lock);
 
 	return ret;
@@ -168,11 +168,12 @@ EXPORT_SYMBOL_GPL(ucsi_send_command);
 
 int ucsi_resume(struct ucsi *ucsi)
 {
-	struct ucsi_control ctrl;
+	u64 command;
 
 	/* Restore UCSI notification enable mask after system resume */
-	UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_ALL);
-	return ucsi_send_command(ucsi, &ctrl, NULL, 0);
+	command = UCSI_SET_NOTIFICATION_ENABLE | UCSI_ENABLE_NTFY_ALL;
+
+	return ucsi_send_command(ucsi, command, NULL, 0);
 }
 EXPORT_SYMBOL_GPL(ucsi_resume);
 /* -------------------------------------------------------------------------- */
@@ -180,14 +181,14 @@ EXPORT_SYMBOL_GPL(ucsi_resume);
 void ucsi_altmode_update_active(struct ucsi_connector *con)
 {
 	const struct typec_altmode *altmode = NULL;
-	struct ucsi_control ctrl;
+	u64 command;
 	u16 version;
 	int ret;
 	u8 cur;
 	int i;
 
-	UCSI_CMD_GET_CURRENT_CAM(ctrl, con->num);
-	ret = ucsi_run_command(con->ucsi, &ctrl, &cur, sizeof(cur));
+	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_run_command(con->ucsi, command, &cur, sizeof(cur));
 	if (ret < 0) {
 		ret = con->ucsi->ops->read(con->ucsi, UCSI_VERSION, &version,
 					   sizeof(version));
@@ -307,7 +308,7 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 	int max_altmodes = UCSI_MAX_ALTMODES;
 	struct typec_altmode_desc desc;
 	struct ucsi_altmode alt[2];
-	struct ucsi_control ctrl;
+	u64 command;
 	int num = 1;
 	int ret;
 	int len;
@@ -325,8 +326,12 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 
 	for (i = 0; i < max_altmodes;) {
 		memset(alt, 0, sizeof(alt));
-		UCSI_CMD_GET_ALTERNATE_MODES(ctrl, recipient, con->num, i, 1);
-		len = ucsi_run_command(con->ucsi, &ctrl, alt, sizeof(alt));
+		command = UCSI_GET_ALTERNATE_MODES;
+		command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
+		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
+		command |= UCSI_GET_ALTMODE_OFFSET(i);
+		command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a time */
+		len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
 		if (len <= 0)
 			return len;
 
@@ -487,13 +492,14 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
 						  work);
 	struct ucsi *ucsi = con->ucsi;
-	struct ucsi_control ctrl;
+	u64 command;
 	int ret;
 
 	mutex_lock(&con->lock);
 
-	UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num);
-	ret = ucsi_send_command(ucsi, &ctrl, &con->status, sizeof(con->status));
+	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_send_command(ucsi, command, &con->status,
+				sizeof(con->status));
 	if (ret < 0) {
 		dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
 			__func__, ret);
@@ -537,8 +543,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 		 * Running GET_CAM_SUPPORTED command just to make sure the PPM
 		 * does not get stuck in case it assumes we do so.
 		 */
-		UCSI_CMD_GET_CAM_SUPPORTED(ctrl, con->num);
-		ucsi_run_command(con->ucsi, &ctrl, NULL, 0);
+		command = UCSI_GET_CAM_SUPPORTED;
+		command |= UCSI_CONNECTOR_NUMBER(con->num);
+		ucsi_run_command(con->ucsi, command, NULL, 0);
 	}
 
 	if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
@@ -573,11 +580,12 @@ EXPORT_SYMBOL_GPL(ucsi_connector_change);
 
 static int ucsi_reset_connector(struct ucsi_connector *con, bool hard)
 {
-	struct ucsi_control ctrl;
+	u64 command;
 
-	UCSI_CMD_CONNECTOR_RESET(ctrl, con, hard);
+	command = UCSI_CONNECTOR_RESET | UCSI_CONNECTOR_NUMBER(con->num);
+	command |= hard ? UCSI_CONNECTOR_RESET_HARD : 0;
 
-	return ucsi_send_command(con->ucsi, &ctrl, NULL, 0);
+	return ucsi_send_command(con->ucsi, command, NULL, 0);
 }
 
 static int ucsi_reset_ppm(struct ucsi *ucsi)
@@ -617,21 +625,21 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
 	return 0;
 }
 
-static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl)
+static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 {
 	int ret;
 
-	ret = ucsi_send_command(con->ucsi, ctrl, NULL, 0);
+	ret = ucsi_send_command(con->ucsi, command, NULL, 0);
 	if (ret == -ETIMEDOUT) {
-		struct ucsi_control c;
+		u64 c;
 
 		/* PPM most likely stopped responding. Resetting everything. */
 		mutex_lock(&con->ucsi->ppm_lock);
 		ucsi_reset_ppm(con->ucsi);
 		mutex_unlock(&con->ucsi->ppm_lock);
 
-		UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
-		ucsi_send_command(con->ucsi, &c, NULL, 0);
+		c = UCSI_SET_NOTIFICATION_ENABLE | UCSI_ENABLE_NTFY_ALL;
+		ucsi_send_command(con->ucsi, c, NULL, 0);
 
 		ucsi_reset_connector(con, true);
 	}
@@ -642,7 +650,7 @@ static int ucsi_role_cmd(struct ucsi_connector *con, struct ucsi_control *ctrl)
 static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 {
 	struct ucsi_connector *con = typec_get_drvdata(port);
-	struct ucsi_control ctrl;
+	u64 command;
 	int ret = 0;
 
 	mutex_lock(&con->lock);
@@ -658,8 +666,10 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 	     role == TYPEC_HOST))
 		goto out_unlock;
 
-	UCSI_CMD_SET_UOR(ctrl, con, role);
-	ret = ucsi_role_cmd(con, &ctrl);
+	command = UCSI_SET_UOR | UCSI_CONNECTOR_NUMBER(con->num);
+	command |= UCSI_SET_UOR_ROLE(role);
+	command |= UCSI_SET_UOR_ACCEPT_ROLE_SWAPS;
+	ret = ucsi_role_cmd(con, command);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -676,7 +686,7 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
 {
 	struct ucsi_connector *con = typec_get_drvdata(port);
-	struct ucsi_control ctrl;
+	u64 command;
 	int ret = 0;
 
 	mutex_lock(&con->lock);
@@ -689,8 +699,10 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
 	if (con->status.pwr_dir == role)
 		goto out_unlock;
 
-	UCSI_CMD_SET_PDR(ctrl, con, role);
-	ret = ucsi_role_cmd(con, &ctrl);
+	command = UCSI_SET_PDR | UCSI_CONNECTOR_NUMBER(con->num);
+	command |= UCSI_SET_PDR_ROLE(role);
+	command |= UCSI_SET_PDR_ACCEPT_ROLE_SWAPS;
+	ret = ucsi_role_cmd(con, command);
 	if (ret < 0)
 		goto out_unlock;
 
@@ -733,7 +745,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	struct ucsi_connector *con = &ucsi->connector[index];
 	struct typec_capability *cap = &con->typec_cap;
 	enum typec_accessory *accessory = cap->accessory;
-	struct ucsi_control ctrl;
+	u64 command;
 	int ret;
 
 	INIT_WORK(&con->work, ucsi_handle_connector_change);
@@ -743,8 +755,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	con->ucsi = ucsi;
 
 	/* Get connector capability */
-	UCSI_CMD_GET_CONNECTOR_CAPABILITY(ctrl, con->num);
-	ret = ucsi_run_command(ucsi, &ctrl, &con->cap, sizeof(con->cap));
+	command = UCSI_GET_CONNECTOR_CAPABILITY;
+	command |= UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_run_command(ucsi, command, &con->cap, sizeof(con->cap));
 	if (ret < 0)
 		return ret;
 
@@ -787,8 +800,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 			con->num);
 
 	/* Get the status */
-	UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num);
-	ret = ucsi_run_command(ucsi, &ctrl, &con->status, sizeof(con->status));
+	command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
+	ret = ucsi_run_command(ucsi, command, &con->status,
+			       sizeof(con->status));
 	if (ret < 0) {
 		dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
 		return 0;
@@ -836,7 +850,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 int ucsi_init(struct ucsi *ucsi)
 {
 	struct ucsi_connector *con;
-	struct ucsi_control ctrl;
+	u64 command;
 	int ret;
 	int i;
 
@@ -850,15 +864,15 @@ int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Enable basic notifications */
-	UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_CMD_COMPLETE |
-					UCSI_ENABLE_NTFY_ERROR);
-	ret = ucsi_run_command(ucsi, &ctrl, NULL, 0);
+	command = UCSI_SET_NOTIFICATION_ENABLE;
+	command |= UCSI_ENABLE_NTFY_CMD_COMPLETE | UCSI_ENABLE_NTFY_ERROR;
+	ret = ucsi_run_command(ucsi, command, NULL, 0);
 	if (ret < 0)
 		goto err_reset;
 
 	/* Get PPM capabilities */
-	UCSI_CMD_GET_CAPABILITY(ctrl);
-	ret = ucsi_run_command(ucsi, &ctrl, &ucsi->cap, sizeof(ucsi->cap));
+	command = UCSI_GET_CAPABILITY;
+	ret = ucsi_run_command(ucsi, command, &ucsi->cap, sizeof(ucsi->cap));
 	if (ret < 0)
 		goto err_reset;
 
@@ -883,8 +897,8 @@ int ucsi_init(struct ucsi *ucsi)
 	}
 
 	/* Enable all notifications */
-	UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_ALL);
-	ret = ucsi_run_command(ucsi, &ctrl, NULL, 0);
+	command = UCSI_SET_NOTIFICATION_ENABLE | UCSI_ENABLE_NTFY_ALL;
+	ret = ucsi_run_command(ucsi, command, NULL, 0);
 	if (ret < 0)
 		goto err_unregister;
 
@@ -1005,15 +1019,15 @@ EXPORT_SYMBOL_GPL(ucsi_register);
  */
 void ucsi_unregister(struct ucsi *ucsi)
 {
-	struct ucsi_control ctrl;
+	u64 command;
 	int i;
 
 	/* Make sure that we are not in the middle of driver initialization */
 	cancel_work_sync(&ucsi->work);
 
 	/* Disable everything except command complete notification */
-	UCSI_CMD_SET_NTFY_ENABLE(ctrl, UCSI_ENABLE_NTFY_CMD_COMPLETE)
-	ucsi_send_command(ucsi, &ctrl, NULL, 0);
+	command = UCSI_SET_NOTIFICATION_ENABLE | UCSI_ENABLE_NTFY_CMD_COMPLETE;
+	ucsi_send_command(ucsi, command, NULL, 0);
 
 	for (i = 0; i < ucsi->cap.num_connectors; i++) {
 		cancel_work_sync(&ucsi->connector[i].work);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index bb1df6cb241b..55ce41dd77f9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -60,178 +60,6 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 
 /* -------------------------------------------------------------------------- */
 
-/* Command Status and Connector Change Indication (CCI) data structure */
-struct ucsi_cci {
-	u8:1; /* reserved */
-	u8 connector_change:7;
-	u8 data_length;
-	u16:9; /* reserved */
-	u16 not_supported:1;
-	u16 cancel_complete:1;
-	u16 reset_complete:1;
-	u16 busy:1;
-	u16 ack_complete:1;
-	u16 error:1;
-	u16 cmd_complete:1;
-} __packed;
-
-/* Default fields in CONTROL data structure */
-struct ucsi_command {
-	u8 cmd;
-	u8 length;
-	u64 data:48;
-} __packed;
-
-/* ACK Command structure */
-struct ucsi_ack_cmd {
-	u8 cmd;
-	u8 length;
-	u8 cci_ack:1;
-	u8 cmd_ack:1;
-	u8:6; /* reserved */
-} __packed;
-
-/* Connector Reset Command structure */
-struct ucsi_con_rst {
-	u8 cmd;
-	u8 length;
-	u8 con_num:7;
-	u8 hard_reset:1;
-} __packed;
-
-/* Set USB Operation Mode Command structure */
-struct ucsi_uor_cmd {
-	u8 cmd;
-	u8 length;
-	u16 con_num:7;
-	u16 role:3;
-#define UCSI_UOR_ROLE_DFP			BIT(0)
-#define UCSI_UOR_ROLE_UFP			BIT(1)
-#define UCSI_UOR_ROLE_DRP			BIT(2)
-	u16:6; /* reserved */
-} __packed;
-
-/* Get Alternate Modes Command structure */
-struct ucsi_altmode_cmd {
-	u8 cmd;
-	u8 length;
-	u8 recipient;
-#define UCSI_RECIPIENT_CON			0
-#define UCSI_RECIPIENT_SOP			1
-#define UCSI_RECIPIENT_SOP_P			2
-#define UCSI_RECIPIENT_SOP_PP			3
-	u8 con_num;
-	u8 offset;
-	u8 num_altmodes;
-} __packed;
-
-struct ucsi_control {
-	union {
-		u64 raw_cmd;
-		struct ucsi_command cmd;
-		struct ucsi_uor_cmd uor;
-		struct ucsi_ack_cmd ack;
-		struct ucsi_con_rst con_rst;
-		struct ucsi_altmode_cmd alt;
-	};
-};
-
-#define __UCSI_CMD(_ctrl_, _cmd_)					\
-{									\
-	(_ctrl_).raw_cmd = 0;						\
-	(_ctrl_).cmd.cmd = _cmd_;					\
-}
-
-/* Helper for preparing ucsi_control for CONNECTOR_RESET command. */
-#define UCSI_CMD_CONNECTOR_RESET(_ctrl_, _con_, _hard_)			\
-{									\
-	__UCSI_CMD(_ctrl_, UCSI_CONNECTOR_RESET)			\
-	(_ctrl_).con_rst.con_num = (_con_)->num;			\
-	(_ctrl_).con_rst.hard_reset = _hard_;				\
-}
-
-/* Helper for preparing ucsi_control for ACK_CC_CI command. */
-#define UCSI_CMD_ACK(_ctrl_, _ack_)					\
-{									\
-	__UCSI_CMD(_ctrl_, UCSI_ACK_CC_CI)				\
-	(_ctrl_).ack.cci_ack = ((_ack_) == UCSI_ACK_EVENT);		\
-	(_ctrl_).ack.cmd_ack = ((_ack_) == UCSI_ACK_CMD);		\
-}
-
-/* Helper for preparing ucsi_control for SET_NOTIFY_ENABLE command. */
-#define UCSI_CMD_SET_NTFY_ENABLE(_ctrl_, _ntfys_)			\
-{									\
-	__UCSI_CMD(_ctrl_, UCSI_SET_NOTIFICATION_ENABLE)		\
-	(_ctrl_).cmd.data = _ntfys_;					\
-}
-
-/* Helper for preparing ucsi_control for GET_CAPABILITY command. */
-#define UCSI_CMD_GET_CAPABILITY(_ctrl_)					\
-{									\
-	__UCSI_CMD(_ctrl_, UCSI_GET_CAPABILITY)				\
-}
-
-/* Helper for preparing ucsi_control for GET_CONNECTOR_CAPABILITY command. */
-#define UCSI_CMD_GET_CONNECTOR_CAPABILITY(_ctrl_, _con_)		\
-{									\
-	__UCSI_CMD(_ctrl_, UCSI_GET_CONNECTOR_CAPABILITY)		\
-	(_ctrl_).cmd.data = _con_;					\
-}
-
-/* Helper for preparing ucsi_control for GET_ALTERNATE_MODES command. */
-#define UCSI_CMD_GET_ALTERNATE_MODES(_ctrl_, _r_, _con_num_, _o_, _num_)\
-{									\
-	__UCSI_CMD((_ctrl_), UCSI_GET_ALTERNATE_MODES)			\
-	_ctrl_.alt.recipient = (_r_);					\
-	_ctrl_.alt.con_num = (_con_num_);				\
-	_ctrl_.alt.offset = (_o_);					\
-	_ctrl_.alt.num_altmodes = (_num_) - 1;				\
-}
-
-/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */
-#define UCSI_CMD_GET_CAM_SUPPORTED(_ctrl_, _con_)			\
-{									\
-	__UCSI_CMD((_ctrl_), UCSI_GET_CAM_SUPPORTED)			\
-	_ctrl_.cmd.data = (_con_);					\
-}
-
-/* Helper for preparing ucsi_control for GET_CAM_SUPPORTED command. */
-#define UCSI_CMD_GET_CURRENT_CAM(_ctrl_, _con_)			\
-{									\
-	__UCSI_CMD((_ctrl_), UCSI_GET_CURRENT_CAM)			\
-	_ctrl_.cmd.data = (_con_);					\
-}
-
-/* Helper for preparing ucsi_control for GET_CONNECTOR_STATUS command. */
-#define UCSI_CMD_GET_CONNECTOR_STATUS(_ctrl_, _con_)			\
-{									\
-	__UCSI_CMD(_ctrl_, UCSI_GET_CONNECTOR_STATUS)			\
-	(_ctrl_).cmd.data = _con_;					\
-}
-
-#define __UCSI_ROLE(_ctrl_, _cmd_, _con_num_)				\
-{									\
-	__UCSI_CMD(_ctrl_, _cmd_)					\
-	(_ctrl_).uor.con_num = _con_num_;				\
-	(_ctrl_).uor.role = UCSI_UOR_ROLE_DRP;				\
-}
-
-/* Helper for preparing ucsi_control for SET_UOR command. */
-#define UCSI_CMD_SET_UOR(_ctrl_, _con_, _role_)				\
-{									\
-	__UCSI_ROLE(_ctrl_, UCSI_SET_UOR, (_con_)->num)		\
-	(_ctrl_).uor.role |= (_role_) == TYPEC_HOST ? UCSI_UOR_ROLE_DFP : \
-			  UCSI_UOR_ROLE_UFP;				\
-}
-
-/* Helper for preparing ucsi_control for SET_PDR command. */
-#define UCSI_CMD_SET_PDR(_ctrl_, _con_, _role_)			\
-{									\
-	__UCSI_ROLE(_ctrl_, UCSI_SET_PDR, (_con_)->num)		\
-	(_ctrl_).uor.role |= (_role_) == TYPEC_SOURCE ? UCSI_UOR_ROLE_DFP : \
-			UCSI_UOR_ROLE_UFP;				\
-}
-
 /* Commands */
 #define UCSI_PPM_RESET			0x01
 #define UCSI_CANCEL			0x02
@@ -253,28 +81,49 @@ struct ucsi_control {
 #define UCSI_GET_CONNECTOR_STATUS	0x12
 #define UCSI_GET_ERROR_STATUS		0x13
 
-/* ACK_CC_CI commands */
-#define UCSI_ACK_EVENT			1
-#define UCSI_ACK_CMD			2
+#define UCSI_CONNECTOR_NUMBER(_num_)		((_num_) << 16)
+
+/* CONNECTOR_RESET command bits */
+#define UCSI_CONNECTOR_RESET_HARD		BIT(23) /* Deprecated in v1.1 */
 
-/* Bits for ACK CC or CI */
+/* ACK_CC_CI bits */
 #define UCSI_ACK_CONNECTOR_CHANGE		BIT(16)
 #define UCSI_ACK_COMMAND_COMPLETE		BIT(17)
 
-/* Bits for SET_NOTIFICATION_ENABLE command */
-#define UCSI_ENABLE_NTFY_CMD_COMPLETE		BIT(0)
-#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE	BIT(1)
-#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE	BIT(2)
-#define UCSI_ENABLE_NTFY_CAP_CHANGE		BIT(5)
-#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE	BIT(6)
-#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE	BIT(7)
-#define UCSI_ENABLE_NTFY_CAM_CHANGE		BIT(8)
-#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE	BIT(9)
-#define UCSI_ENABLE_NTFY_PARTNER_CHANGE		BIT(11)
-#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE		BIT(12)
-#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE	BIT(14)
-#define UCSI_ENABLE_NTFY_ERROR			BIT(15)
-#define UCSI_ENABLE_NTFY_ALL			0xdbe7
+/* SET_NOTIFICATION_ENABLE command bits */
+#define UCSI_ENABLE_NTFY_CMD_COMPLETE		BIT(16)
+#define UCSI_ENABLE_NTFY_EXT_PWR_SRC_CHANGE	BIT(17)
+#define UCSI_ENABLE_NTFY_PWR_OPMODE_CHANGE	BIT(18)
+#define UCSI_ENABLE_NTFY_CAP_CHANGE		BIT(19)
+#define UCSI_ENABLE_NTFY_PWR_LEVEL_CHANGE	BIT(20)
+#define UCSI_ENABLE_NTFY_PD_RESET_COMPLETE	BIT(21)
+#define UCSI_ENABLE_NTFY_CAM_CHANGE		BIT(22)
+#define UCSI_ENABLE_NTFY_BAT_STATUS_CHANGE	BIT(23)
+#define UCSI_ENABLE_NTFY_PARTNER_CHANGE		BIT(24)
+#define UCSI_ENABLE_NTFY_PWR_DIR_CHANGE		BIT(25)
+#define UCSI_ENABLE_NTFY_CONNECTOR_CHANGE	BIT(26)
+#define UCSI_ENABLE_NTFY_ERROR			BIT(27)
+#define UCSI_ENABLE_NTFY_ALL			(0xdbe7 << 16)
+
+/* SET_UOR command bits */
+#define UCSI_SET_UOR_ROLE(_r_)		(((_r_) == TYPEC_HOST ? 1 : 2) << 23)
+#define UCSI_SET_UOR_ACCEPT_ROLE_SWAPS		BIT(25)
+
+/* SET_PDF command bits */
+#define UCSI_SET_PDR_ROLE(_r_)		(((_r_) == TYPEC_SOURCE ? 1 : 2) << 23)
+#define UCSI_SET_PDR_ACCEPT_ROLE_SWAPS		BIT(25)
+
+/* GET_ALTERNATE_MODES command bits */
+#define UCSI_GET_ALTMODE_RECIPIENT(_r_)		((u64)(_r_) << 16)
+#define   UCSI_RECIPIENT_CON			0
+#define   UCSI_RECIPIENT_SOP			1
+#define   UCSI_RECIPIENT_SOP_P			2
+#define   UCSI_RECIPIENT_SOP_PP			3
+#define UCSI_GET_ALTMODE_CONNECTOR_NUMBER(_r_)	((u64)(_r_) << 24)
+#define UCSI_GET_ALTMODE_OFFSET(_r_)		((u64)(_r_) << 32)
+#define UCSI_GET_ALTMODE_NUM_ALTMODES(_r_)	((u64)(_r_) << 40)
+
+/* -------------------------------------------------------------------------- */
 
 /* Error information returned by PPM in response to GET_ERROR_STATUS command. */
 #define UCSI_ERROR_UNREGONIZED_CMD		BIT(0)
@@ -442,7 +291,7 @@ struct ucsi_connector {
 	struct ucsi_connector_capability cap;
 };
 
-int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
+int ucsi_send_command(struct ucsi *ucsi, u64 command,
 		      void *retval, size_t size);
 
 void ucsi_altmode_update_active(struct ucsi_connector *con);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 14/14] usb: typec: ucsi: Remove all bit-fields
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (12 preceding siblings ...)
  2019-09-26 14:25 ` [PATCH 13/14] usb: typec: ucsi: Remove struct ucsi_control Heikki Krogerus
@ 2019-09-26 14:25 ` Heikki Krogerus
  2019-09-27  0:13 ` [PATCH 00/14] usb: typec: UCSI driver overhaul Ajay Gupta
  2019-10-01 18:36 ` Ajay Gupta
  15 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-26 14:25 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

We can't use bit fields with data that is received or send
to/from the device.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/trace.h | 12 ++---
 drivers/usb/typec/ucsi/ucsi.c  | 52 +++++++++++--------
 drivers/usb/typec/ucsi/ucsi.h  | 93 +++++++++++++++++-----------------
 3 files changed, 85 insertions(+), 72 deletions(-)

diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
index 2262229dae8e..a0d3a934d3d9 100644
--- a/drivers/usb/typec/ucsi/trace.h
+++ b/drivers/usb/typec/ucsi/trace.h
@@ -56,13 +56,13 @@ DECLARE_EVENT_CLASS(ucsi_log_connector_status,
 	TP_fast_assign(
 		__entry->port = port - 1;
 		__entry->change = status->change;
-		__entry->opmode = status->pwr_op_mode;
-		__entry->connected = status->connected;
-		__entry->pwr_dir = status->pwr_dir;
-		__entry->partner_flags = status->partner_flags;
-		__entry->partner_type = status->partner_type;
+		__entry->opmode = UCSI_CONSTAT_PWR_OPMODE(status->flags);
+		__entry->connected = !!(status->flags & UCSI_CONSTAT_CONNECTED);
+		__entry->pwr_dir = !!(status->flags & UCSI_CONSTAT_PWR_DIR);
+		__entry->partner_flags = UCSI_CONSTAT_PARTNER_FLAGS(status->flags);
+		__entry->partner_type = UCSI_CONSTAT_PARTNER_TYPE(status->flags);
 		__entry->request_data_obj = status->request_data_obj;
-		__entry->bc_status = status->bc_status;
+		__entry->bc_status = UCSI_CONSTAT_BC_STATUS(status->pwr_status);
 	),
 	TP_printk("port%d status: change=%04x, opmode=%x, connected=%d, "
 		"sourcing=%d, partner_flags=%x, partner_type=%x, "
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 6fd4ff46d728..accf54d987ad 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -393,7 +393,7 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 
 static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 {
-	switch (con->status.pwr_op_mode) {
+	switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
 	case UCSI_CONSTAT_PWR_OPMODE_PD:
 		typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
 		break;
@@ -411,6 +411,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
 
 static int ucsi_register_partner(struct ucsi_connector *con)
 {
+	u8 pwr_opmode = UCSI_CONSTAT_PWR_OPMODE(con->status.flags);
 	struct typec_partner_desc desc;
 	struct typec_partner *partner;
 
@@ -419,7 +420,7 @@ static int ucsi_register_partner(struct ucsi_connector *con)
 
 	memset(&desc, 0, sizeof(desc));
 
-	switch (con->status.partner_type) {
+	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 	case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
 		desc.accessory = TYPEC_ACCESSORY_DEBUG;
 		break;
@@ -430,7 +431,7 @@ static int ucsi_register_partner(struct ucsi_connector *con)
 		break;
 	}
 
-	desc.usb_pd = con->status.pwr_op_mode == UCSI_CONSTAT_PWR_OPMODE_PD;
+	desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
 
 	partner = typec_register_partner(con->port, &desc);
 	if (IS_ERR(partner)) {
@@ -462,7 +463,7 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 	if (!con->partner)
 		return;
 
-	switch (con->status.partner_type) {
+	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
 		typec_set_data_role(con->port, TYPEC_HOST);
 		break;
@@ -492,6 +493,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 	struct ucsi_connector *con = container_of(work, struct ucsi_connector,
 						  work);
 	struct ucsi *ucsi = con->ucsi;
+	enum typec_role role;
 	u64 command;
 	int ret;
 
@@ -506,11 +508,13 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 		goto out_unlock;
 	}
 
+	role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
+
 	if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE)
 		ucsi_pwr_opmode_change(con);
 
 	if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
-		typec_set_pwr_role(con->port, con->status.pwr_dir);
+		typec_set_pwr_role(con->port, role);
 
 		/* Complete pending power role swap */
 		if (!completion_done(&con->complete))
@@ -518,9 +522,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 	}
 
 	if (con->status.change & UCSI_CONSTAT_CONNECT_CHANGE) {
-		typec_set_pwr_role(con->port, con->status.pwr_dir);
+		typec_set_pwr_role(con->port, role);
 
-		switch (con->status.partner_type) {
+		switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 		case UCSI_CONSTAT_PARTNER_TYPE_UFP:
 			typec_set_data_role(con->port, TYPEC_HOST);
 			break;
@@ -531,7 +535,7 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 			break;
 		}
 
-		if (con->status.connected)
+		if (con->status.flags & UCSI_CONSTAT_CONNECTED)
 			ucsi_register_partner(con);
 		else
 			ucsi_unregister_partner(con);
@@ -650,6 +654,7 @@ static int ucsi_role_cmd(struct ucsi_connector *con, u64 command)
 static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 {
 	struct ucsi_connector *con = typec_get_drvdata(port);
+	u8 partner_type;
 	u64 command;
 	int ret = 0;
 
@@ -660,9 +665,10 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 		goto out_unlock;
 	}
 
-	if ((con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP &&
+	partner_type = UCSI_CONSTAT_PARTNER_TYPE(con->status.flags);
+	if ((partner_type == UCSI_CONSTAT_PARTNER_TYPE_DFP &&
 	     role == TYPEC_DEVICE) ||
-	    (con->status.partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP &&
+	    (partner_type == UCSI_CONSTAT_PARTNER_TYPE_UFP &&
 	     role == TYPEC_HOST))
 		goto out_unlock;
 
@@ -686,6 +692,7 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
 static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
 {
 	struct ucsi_connector *con = typec_get_drvdata(port);
+	enum typec_role cur_role;
 	u64 command;
 	int ret = 0;
 
@@ -696,7 +703,9 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
 		goto out_unlock;
 	}
 
-	if (con->status.pwr_dir == role)
+	cur_role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
+
+	if (cur_role == role)
 		goto out_unlock;
 
 	command = UCSI_SET_PDR | UCSI_CONNECTOR_NUMBER(con->num);
@@ -713,7 +722,8 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
 	}
 
 	/* Something has gone wrong while swapping the role */
-	if (con->status.pwr_op_mode != UCSI_CONSTAT_PWR_OPMODE_PD) {
+	if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) !=
+	    UCSI_CONSTAT_PWR_OPMODE_PD) {
 		ucsi_reset_connector(con, true);
 		ret = -EPROTO;
 	}
@@ -768,11 +778,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	else if (con->cap.op_mode & UCSI_CONCAP_OPMODE_UFP)
 		cap->data = TYPEC_PORT_UFP;
 
-	if (con->cap.provider && con->cap.consumer)
+	if ((con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER) &&
+	    (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER))
 		cap->type = TYPEC_PORT_DRP;
-	else if (con->cap.provider)
+	else if (con->cap.flags & UCSI_CONCAP_FLAG_PROVIDER)
 		cap->type = TYPEC_PORT_SRC;
-	else if (con->cap.consumer)
+	else if (con->cap.flags & UCSI_CONCAP_FLAG_CONSUMER)
 		cap->type = TYPEC_PORT_SNK;
 
 	cap->revision = ucsi->cap.typec_version;
@@ -808,10 +819,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		return 0;
 	}
 
-	ucsi_pwr_opmode_change(con);
-	typec_set_pwr_role(con->port, con->status.pwr_dir);
-
-	switch (con->status.partner_type) {
+	switch (UCSI_CONSTAT_PARTNER_TYPE(con->status.flags)) {
 	case UCSI_CONSTAT_PARTNER_TYPE_UFP:
 		typec_set_data_role(con->port, TYPEC_HOST);
 		break;
@@ -823,8 +831,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	}
 
 	/* Check if there is already something connected */
-	if (con->status.connected)
+	if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
+		typec_set_pwr_role(con->port,
+				  !!(con->status.flags & UCSI_CONSTAT_PWR_DIR));
+		ucsi_pwr_opmode_change(con);
 		ucsi_register_partner(con);
+	}
 
 	if (con->partner) {
 		ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 55ce41dd77f9..31d88f835f92 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -144,8 +144,8 @@ struct ucsi_capability {
 #define UCSI_CAP_ATTR_POWER_AC_SUPPLY		BIT(8)
 #define UCSI_CAP_ATTR_POWER_OTHER		BIT(10)
 #define UCSI_CAP_ATTR_POWER_VBUS		BIT(14)
-	u32 num_connectors:8;
-	u32 features:24;
+	u8 num_connectors;
+	u8 features;
 #define UCSI_CAP_SET_UOM			BIT(0)
 #define UCSI_CAP_SET_PDM			BIT(1)
 #define UCSI_CAP_ALT_MODE_DETAILS		BIT(2)
@@ -154,8 +154,9 @@ struct ucsi_capability {
 #define UCSI_CAP_CABLE_DETAILS			BIT(5)
 #define UCSI_CAP_EXT_SUPPLY_NOTIFICATIONS	BIT(6)
 #define UCSI_CAP_PD_RESET			BIT(7)
+	u16 reserved_1;
 	u8 num_alt_modes;
-	u8 reserved;
+	u8 reserved_2;
 	u16 bc_version;
 	u16 pd_version;
 	u16 typec_version;
@@ -172,9 +173,9 @@ struct ucsi_connector_capability {
 #define UCSI_CONCAP_OPMODE_USB2			BIT(5)
 #define UCSI_CONCAP_OPMODE_USB3			BIT(6)
 #define UCSI_CONCAP_OPMODE_ALT_MODE		BIT(7)
-	u8 provider:1;
-	u8 consumer:1;
-	u8:6; /* reserved */
+	u8 flags;
+#define UCSI_CONCAP_FLAG_PROVIDER		BIT(0)
+#define UCSI_CONCAP_FLAG_CONSUMER		BIT(1)
 } __packed;
 
 struct ucsi_altmode {
@@ -186,18 +187,17 @@ struct ucsi_altmode {
 struct ucsi_cable_property {
 	u16 speed_supported;
 	u8 current_capability;
-	u8 vbus_in_cable:1;
-	u8 active_cable:1;
-	u8 directionality:1;
-	u8 plug_type:2;
-#define UCSI_CABLE_PROPERTY_PLUG_TYPE_A		0
-#define UCSI_CABLE_PROPERTY_PLUG_TYPE_B		1
-#define UCSI_CABLE_PROPERTY_PLUG_TYPE_C		2
-#define UCSI_CABLE_PROPERTY_PLUG_OTHER		3
-	u8 mode_support:1;
-	u8:2; /* reserved */
-	u8 latency:4;
-	u8:4; /* reserved */
+	u8 flags;
+#define UCSI_CABLE_PROP_FLAG_VBUS_IN_CABLE	BIT(0)
+#define UCSI_CABLE_PROP_FLAG_ACTIVE_CABLE	BIT(1)
+#define UCSI_CABLE_PROP_FLAG_DIRECTIONALITY	BIT(2)
+#define UCSI_CABLE_PROP_FLAG_PLUG_TYPE(_f_)	((_f_) & GENMASK(3, 0))
+#define   UCSI_CABLE_PROPERTY_PLUG_TYPE_A	0
+#define   UCSI_CABLE_PROPERTY_PLUG_TYPE_B	1
+#define   UCSI_CABLE_PROPERTY_PLUG_TYPE_C	2
+#define   UCSI_CABLE_PROPERTY_PLUG_OTHER	3
+#define UCSI_CABLE_PROP_MODE_SUPPORT		BIT(5)
+	u8 latency;
 } __packed;
 
 /* Data structure filled by PPM in response to GET_CONNECTOR_STATUS command. */
@@ -214,35 +214,36 @@ struct ucsi_connector_status {
 #define UCSI_CONSTAT_POWER_DIR_CHANGE		BIT(12)
 #define UCSI_CONSTAT_CONNECT_CHANGE		BIT(14)
 #define UCSI_CONSTAT_ERROR			BIT(15)
-	u16 pwr_op_mode:3;
-#define UCSI_CONSTAT_PWR_OPMODE_NONE		0
-#define UCSI_CONSTAT_PWR_OPMODE_DEFAULT		1
-#define UCSI_CONSTAT_PWR_OPMODE_BC		2
-#define UCSI_CONSTAT_PWR_OPMODE_PD		3
-#define UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5	4
-#define UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0	5
-	u16 connected:1;
-	u16 pwr_dir:1;
-	u16 partner_flags:8;
-#define UCSI_CONSTAT_PARTNER_FLAG_USB		BIT(0)
-#define UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE	BIT(1)
-	u16 partner_type:3;
-#define UCSI_CONSTAT_PARTNER_TYPE_DFP		1
-#define UCSI_CONSTAT_PARTNER_TYPE_UFP		2
-#define UCSI_CONSTAT_PARTNER_TYPE_CABLE		3 /* Powered Cable */
-#define UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP	4 /* Powered Cable */
-#define UCSI_CONSTAT_PARTNER_TYPE_DEBUG		5
-#define UCSI_CONSTAT_PARTNER_TYPE_AUDIO		6
+	u16 flags;
+#define UCSI_CONSTAT_PWR_OPMODE(_f_)		((_f_) & GENMASK(2, 0))
+#define   UCSI_CONSTAT_PWR_OPMODE_NONE		0
+#define   UCSI_CONSTAT_PWR_OPMODE_DEFAULT	1
+#define   UCSI_CONSTAT_PWR_OPMODE_BC		2
+#define   UCSI_CONSTAT_PWR_OPMODE_PD		3
+#define   UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5	4
+#define   UCSI_CONSTAT_PWR_OPMODE_TYPEC3_0	5
+#define UCSI_CONSTAT_CONNECTED			BIT(3)
+#define UCSI_CONSTAT_PWR_DIR			BIT(4)
+#define UCSI_CONSTAT_PARTNER_FLAGS(_f_)		((_f_) & GENMASK(12, 5) >> 5)
+#define   UCSI_CONSTAT_PARTNER_FLAG_USB		1
+#define   UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE	2
+#define UCSI_CONSTAT_PARTNER_TYPE(_f_)		((_f_) & GENMASK(15, 13) >> 13)
+#define   UCSI_CONSTAT_PARTNER_TYPE_DFP		1
+#define   UCSI_CONSTAT_PARTNER_TYPE_UFP		2
+#define   UCSI_CONSTAT_PARTNER_TYPE_CABLE	3 /* Powered Cable */
+#define   UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP	4 /* Powered Cable */
+#define   UCSI_CONSTAT_PARTNER_TYPE_DEBUG	5
+#define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO	6
 	u32 request_data_obj;
-	u8 bc_status:2;
-#define UCSI_CONSTAT_BC_NOT_CHARGING		0
-#define UCSI_CONSTAT_BC_NOMINAL_CHARGING	1
-#define UCSI_CONSTAT_BC_SLOW_CHARGING		2
-#define UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
-	u8 provider_cap_limit_reason:4;
-#define UCSI_CONSTAT_CAP_PWR_LOWERED		0
-#define UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT	1
-	u8:2; /* reserved */
+	u8 pwr_status;
+#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_) & GENMASK(2, 0))
+#define   UCSI_CONSTAT_BC_NOT_CHARGING		0
+#define   UCSI_CONSTAT_BC_NOMINAL_CHARGING	1
+#define   UCSI_CONSTAT_BC_SLOW_CHARGING		2
+#define   UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
+#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)	((_p_) & GENMASK(6, 3) >> 3)
+#define   UCSI_CONSTAT_CAP_PWR_LOWERED		0
+#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT	1
 } __packed;
 
 /* -------------------------------------------------------------------------- */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* RE: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (13 preceding siblings ...)
  2019-09-26 14:25 ` [PATCH 14/14] usb: typec: ucsi: Remove all bit-fields Heikki Krogerus
@ 2019-09-27  0:13 ` Ajay Gupta
  2019-09-27  9:44   ` Heikki Krogerus
  2019-09-27 12:53   ` Heikki Krogerus
  2019-10-01 18:36 ` Ajay Gupta
  15 siblings, 2 replies; 24+ messages in thread
From: Ajay Gupta @ 2019-09-27  0:13 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi Heikki,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, September 26, 2019 3:07 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: linux-usb@vger.kernel.org
> Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> 
> Hi Ajay,
> 
> Here's the pretty much complete rewrite of the I/O handling that I was
> talking about. The first seven patches are not actually related to
> this stuff, but I'm including them here because the rest of the series
> is made on top of them. I'm including also that fix patch I send you
> earlier.
> 
> After this it should be easier to handle quirks. My idea how to handle
> the multi-instance connector alt modes is that we "emulate" the PPM in
> ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all.
> 
> We can now get the connector alternate modes that the actual
> controller supplies during probe - before registering the ucsi
> interface - and squash all alt modes with the same SVID into one that
> we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES
> command. Also other alt mode commands like SET_NEW_CAM can have
> special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should
> not be any problem with that anymore.
I took the changes and loaded on my GPU system and do not see
altmode devices under /sys/bus/typec/devices/*. Its empty.

Below error is seen
"ucsi_ccg 4-0008: con1: failed to register alternate modes"

ucsi_run_command() is returning -16.

I will review the ccg changes and try to debug above issue.

Thanks
> nvpublic
> 
> thanks,
> 
> Heikki Krogerus (14):
>   usb: typec: Copy everything from struct typec_capability during
>     registration
>   usb: typec: Introduce typec_get_drvdata()
>   usb: typec: Separate the operations vector
>   usb: typec: tcpm: Start using struct typec_operations
>   usb: typec: tps6598x: Start using struct typec_operations
>   usb: typec: ucsi: Start using struct typec_operations
>   usb: typec: Remove the callback members from struct typec_capability
>   usb: typec: ucsi: ccg: Remove run_isr flag
>   usb: typec: ucsi: Simplified interface registration and I/O API
>   usb: typec: ucsi: acpi: Move to the new API
>   usb: typec: ucsi: ccg: Move to the new API
>   usb: typec: ucsi: Remove the old API
>   usb: typec: ucsi: Remove struct ucsi_control
>   usb: typec: ucsi: Remove all bit-fields
> 
>  drivers/usb/typec/class.c            | 125 +++---
>  drivers/usb/typec/tcpm/tcpm.c        |  47 +--
>  drivers/usb/typec/tps6598x.c         |  49 +--
>  drivers/usb/typec/ucsi/displayport.c |  26 +-
>  drivers/usb/typec/ucsi/trace.c       |  11 -
>  drivers/usb/typec/ucsi/trace.h       |  79 +---
>  drivers/usb/typec/ucsi/ucsi.c        | 592 ++++++++++++++-------------
>  drivers/usb/typec/ucsi/ucsi.h        | 410 +++++++------------
>  drivers/usb/typec/ucsi/ucsi_acpi.c   |  96 ++++-
>  drivers/usb/typec/ucsi/ucsi_ccg.c    | 214 ++++------
>  include/linux/usb/typec.h            |  38 +-
>  11 files changed, 785 insertions(+), 902 deletions(-)
> 
> --
> 2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-09-27  0:13 ` [PATCH 00/14] usb: typec: UCSI driver overhaul Ajay Gupta
@ 2019-09-27  9:44   ` Heikki Krogerus
  2019-09-27 12:53   ` Heikki Krogerus
  1 sibling, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-27  9:44 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

On Fri, Sep 27, 2019 at 12:13:57AM +0000, Ajay Gupta wrote:
> Hi Heikki,
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, September 26, 2019 3:07 AM
> > To: Ajay Gupta <ajayg@nvidia.com>
> > Cc: linux-usb@vger.kernel.org
> > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> > 
> > Hi Ajay,
> > 
> > Here's the pretty much complete rewrite of the I/O handling that I was
> > talking about. The first seven patches are not actually related to
> > this stuff, but I'm including them here because the rest of the series
> > is made on top of them. I'm including also that fix patch I send you
> > earlier.
> > 
> > After this it should be easier to handle quirks. My idea how to handle
> > the multi-instance connector alt modes is that we "emulate" the PPM in
> > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all.
> > 
> > We can now get the connector alternate modes that the actual
> > controller supplies during probe - before registering the ucsi
> > interface - and squash all alt modes with the same SVID into one that
> > we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES
> > command. Also other alt mode commands like SET_NEW_CAM can have
> > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should
> > not be any problem with that anymore.
> I took the changes and loaded on my GPU system and do not see
> altmode devices under /sys/bus/typec/devices/*. Its empty.
> 
> Below error is seen
> "ucsi_ccg 4-0008: con1: failed to register alternate modes"
> 
> ucsi_run_command() is returning -16.
> 
> I will review the ccg changes and try to debug above issue.

There is at least on obvious bug. The number of alternate modes field
is wrong. The this:

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index accf54d987ad..506d963ecc6c 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -330,7 +330,7 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
                command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
                command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
                command |= UCSI_GET_ALTMODE_OFFSET(i);
-               command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a time */
+               command |= UCSI_GET_ALTMODE_NUM_ALTMODES(0); /* One at a time */
                len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
                if (len <= 0)
                        return len;

Alternatively you can also just drop two last patches of the series.

thanks,

-- 
heikki

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-09-27  0:13 ` [PATCH 00/14] usb: typec: UCSI driver overhaul Ajay Gupta
  2019-09-27  9:44   ` Heikki Krogerus
@ 2019-09-27 12:53   ` Heikki Krogerus
  2019-09-27 16:30     ` Ajay Gupta
  1 sibling, 1 reply; 24+ messages in thread
From: Heikki Krogerus @ 2019-09-27 12:53 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

On Fri, Sep 27, 2019 at 12:13:57AM +0000, Ajay Gupta wrote:
> Hi Heikki,
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, September 26, 2019 3:07 AM
> > To: Ajay Gupta <ajayg@nvidia.com>
> > Cc: linux-usb@vger.kernel.org
> > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> > 
> > Hi Ajay,
> > 
> > Here's the pretty much complete rewrite of the I/O handling that I was
> > talking about. The first seven patches are not actually related to
> > this stuff, but I'm including them here because the rest of the series
> > is made on top of them. I'm including also that fix patch I send you
> > earlier.
> > 
> > After this it should be easier to handle quirks. My idea how to handle
> > the multi-instance connector alt modes is that we "emulate" the PPM in
> > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all.
> > 
> > We can now get the connector alternate modes that the actual
> > controller supplies during probe - before registering the ucsi
> > interface - and squash all alt modes with the same SVID into one that
> > we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES
> > command. Also other alt mode commands like SET_NEW_CAM can have
> > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should
> > not be any problem with that anymore.
> I took the changes and loaded on my GPU system and do not see
> altmode devices under /sys/bus/typec/devices/*. Its empty.
> 
> Below error is seen
> "ucsi_ccg 4-0008: con1: failed to register alternate modes"
> 
> ucsi_run_command() is returning -16.
> 
> I will review the ccg changes and try to debug above issue.

I tested this series with the NVIDIA GPU PCI card that we have. The
controller is reporting BUSY as responce to the GET_ALTERNATE_MODES
command. I added a loop to ucsi_exec_command() where I read the CCI
until the there is no UCSI_CCI_BUSY bit set, and it seems to work.

Here are both changes:

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index accf54d987ad..d70ee8006c34 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -102,6 +102,7 @@ static int ucsi_read_error(struct ucsi *ucsi)

 static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 {
+       unsigned long timeout;
        u32 cci;
        int ret;

@@ -109,12 +110,15 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
        if (ret)
                return ret;

-       ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
-       if (ret)
-               return ret;
+       timeout = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+       do {
+               if (time_is_before_jiffies(timeout))
+                       return -ETIMEDOUT;

-       if (cci & UCSI_CCI_BUSY)
-               return -EBUSY;
+               ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+               if (ret)
+                       return ret;
+       } while (cci & UCSI_CCI_BUSY);

        if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
                return -EIO;
@@ -330,7 +334,7 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
                command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
                command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
                command |= UCSI_GET_ALTMODE_OFFSET(i);
-               command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a time */
+
                len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
                if (len <= 0)
                        return len;

Let me know if that works.

thanks,

-- 
heikki

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* RE: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-09-27 12:53   ` Heikki Krogerus
@ 2019-09-27 16:30     ` Ajay Gupta
  0 siblings, 0 replies; 24+ messages in thread
From: Ajay Gupta @ 2019-09-27 16:30 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Friday, September 27, 2019 5:53 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [PATCH 00/14] usb: typec: UCSI driver overhaul
> 
> On Fri, Sep 27, 2019 at 12:13:57AM +0000, Ajay Gupta wrote:
> > Hi Heikki,
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Thursday, September 26, 2019 3:07 AM
> > > To: Ajay Gupta <ajayg@nvidia.com>
> > > Cc: linux-usb@vger.kernel.org
> > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> > >
> > > Hi Ajay,
> > >
> > > Here's the pretty much complete rewrite of the I/O handling that I
> > > was talking about. The first seven patches are not actually related
> > > to this stuff, but I'm including them here because the rest of the
> > > series is made on top of them. I'm including also that fix patch I
> > > send you earlier.
> > >
> > > After this it should be easier to handle quirks. My idea how to
> > > handle the multi-instance connector alt modes is that we "emulate"
> > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not touched at
> all.
> > >
> > > We can now get the connector alternate modes that the actual
> > > controller supplies during probe - before registering the ucsi
> > > interface - and squash all alt modes with the same SVID into one
> > > that we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES
> > > command. Also other alt mode commands like SET_NEW_CAM can have
> > > special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should
> > > not be any problem with that anymore.
> > I took the changes and loaded on my GPU system and do not see altmode
> > devices under /sys/bus/typec/devices/*. Its empty.
> >
> > Below error is seen
> > "ucsi_ccg 4-0008: con1: failed to register alternate modes"
> >
> > ucsi_run_command() is returning -16.
> >
> > I will review the ccg changes and try to debug above issue.
> 
> I tested this series with the NVIDIA GPU PCI card that we have. The controller
> is reporting BUSY as responce to the GET_ALTERNATE_MODES command. I
> added a loop to ucsi_exec_command() where I read the CCI until the there is
> no UCSI_CCI_BUSY bit set, and it seems to work.
> 
> Here are both changes:
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index
> accf54d987ad..d70ee8006c34 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -102,6 +102,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
> 
>  static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)  {
> +       unsigned long timeout;
>         u32 cci;
>         int ret;
> 
> @@ -109,12 +110,15 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64
> cmd)
>         if (ret)
>                 return ret;
> 
> -       ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> -       if (ret)
> -               return ret;
> +       timeout = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
> +       do {
> +               if (time_is_before_jiffies(timeout))
> +                       return -ETIMEDOUT;
> 
> -       if (cci & UCSI_CCI_BUSY)
> -               return -EBUSY;
> +               ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +               if (ret)
> +                       return ret;
> +       } while (cci & UCSI_CCI_BUSY);
> 
>         if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
>                 return -EIO;
> @@ -330,7 +334,7 @@ static int ucsi_register_altmodes(struct
> ucsi_connector *con, u8 recipient)
>                 command |= UCSI_GET_ALTMODE_RECIPIENT(recipient);
>                 command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con-
> >num);
>                 command |= UCSI_GET_ALTMODE_OFFSET(i);
> -               command |= UCSI_GET_ALTMODE_NUM_ALTMODES(1); /* One at a
> time */
> +
>                 len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
>                 if (len <= 0)
>                         return len;
> 
> Let me know if that works.
It works now for me too. I will review rest of changes anyways and also need
to update my change on altmode squashing.

Thanks
>nvpublic
> 
> thanks,
> 
> --
> heikki

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
                   ` (14 preceding siblings ...)
  2019-09-27  0:13 ` [PATCH 00/14] usb: typec: UCSI driver overhaul Ajay Gupta
@ 2019-10-01 18:36 ` Ajay Gupta
  2019-10-03 14:24   ` Heikki Krogerus
  15 siblings, 1 reply; 24+ messages in thread
From: Ajay Gupta @ 2019-10-01 18:36 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi Heikki

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, September 26, 2019 3:07 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: linux-usb@vger.kernel.org
> Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> 
> Hi Ajay,
> 
> Here's the pretty much complete rewrite of the I/O handling that I was
> talking about. The first seven patches are not actually related to
> this stuff, but I'm including them here because the rest of the series
> is made on top of them. I'm including also that fix patch I send you
> earlier.
> 
> After this it should be easier to handle quirks. My idea how to handle
> the multi-instance connector alt modes is that we "emulate" the PPM in
> ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all.
> 
> We can now get the connector alternate modes that the actual
> controller supplies during probe - before registering the ucsi
> interface 
How can ucsi_ccg.c get the connector alternate modes before
registering ucsi interface? PPM reset, notification enable, etc. 
is done during ucsi registration. UCSI spec says:
" The only commands the PPM is required to process in the 
"PPM Idle (Notifications Disabled)" state are 
SET_NOTIFICATION_ENABLE and PPM_RESE"

Also, it doesn't look correct if ucsi_ccg.c has to replicate most 
of the stuff done in ucsi_init() of ucsi.c.

Thanks
> nvpublic
> - and squash all alt modes with the same SVID into one that
> we supply to the ucsi.c when ever it sends GET_ALTERNATE_MODES
> command. Also other alt mode commands like SET_NEW_CAM can have
> special processing in ucsi_ccg.c and ucsi_ccg.c alone. There should
> not be any problem with that anymore.
> 
> thanks,
> 
> Heikki Krogerus (14):
>   usb: typec: Copy everything from struct typec_capability during
>     registration
>   usb: typec: Introduce typec_get_drvdata()
>   usb: typec: Separate the operations vector
>   usb: typec: tcpm: Start using struct typec_operations
>   usb: typec: tps6598x: Start using struct typec_operations
>   usb: typec: ucsi: Start using struct typec_operations
>   usb: typec: Remove the callback members from struct typec_capability
>   usb: typec: ucsi: ccg: Remove run_isr flag
>   usb: typec: ucsi: Simplified interface registration and I/O API
>   usb: typec: ucsi: acpi: Move to the new API
>   usb: typec: ucsi: ccg: Move to the new API
>   usb: typec: ucsi: Remove the old API
>   usb: typec: ucsi: Remove struct ucsi_control
>   usb: typec: ucsi: Remove all bit-fields
> 
>  drivers/usb/typec/class.c            | 125 +++---
>  drivers/usb/typec/tcpm/tcpm.c        |  47 +--
>  drivers/usb/typec/tps6598x.c         |  49 +--
>  drivers/usb/typec/ucsi/displayport.c |  26 +-
>  drivers/usb/typec/ucsi/trace.c       |  11 -
>  drivers/usb/typec/ucsi/trace.h       |  79 +---
>  drivers/usb/typec/ucsi/ucsi.c        | 592 ++++++++++++++-------------
>  drivers/usb/typec/ucsi/ucsi.h        | 410 +++++++------------
>  drivers/usb/typec/ucsi/ucsi_acpi.c   |  96 ++++-
>  drivers/usb/typec/ucsi/ucsi_ccg.c    | 214 ++++------
>  include/linux/usb/typec.h            |  38 +-
>  11 files changed, 785 insertions(+), 902 deletions(-)
> 
> --
> 2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-10-01 18:36 ` Ajay Gupta
@ 2019-10-03 14:24   ` Heikki Krogerus
  2019-10-03 16:33     ` Ajay Gupta
  0 siblings, 1 reply; 24+ messages in thread
From: Heikki Krogerus @ 2019-10-03 14:24 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Hi Ajay,

On Tue, Oct 01, 2019 at 06:36:25PM +0000, Ajay Gupta wrote:
> Hi Heikki
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, September 26, 2019 3:07 AM
> > To: Ajay Gupta <ajayg@nvidia.com>
> > Cc: linux-usb@vger.kernel.org
> > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> > 
> > Hi Ajay,
> > 
> > Here's the pretty much complete rewrite of the I/O handling that I was
> > talking about. The first seven patches are not actually related to
> > this stuff, but I'm including them here because the rest of the series
> > is made on top of them. I'm including also that fix patch I send you
> > earlier.
> > 
> > After this it should be easier to handle quirks. My idea how to handle
> > the multi-instance connector alt modes is that we "emulate" the PPM in
> > ucsi_ccg.c in order to handle them, so ucsi.c is not touched at all.
> > 
> > We can now get the connector alternate modes that the actual
> > controller supplies during probe - before registering the ucsi
> > interface 
> How can ucsi_ccg.c get the connector alternate modes before
> registering ucsi interface? PPM reset, notification enable, etc. 
> is done during ucsi registration. UCSI spec says:
> " The only commands the PPM is required to process in the 
> "PPM Idle (Notifications Disabled)" state are 
> SET_NOTIFICATION_ENABLE and PPM_RESE"
> 
> Also, it doesn't look correct if ucsi_ccg.c has to replicate most 
> of the stuff done in ucsi_init() of ucsi.c.

How about if we split ucsi_init() into a function that first simply
constructs the struct ucsi and struct ucsi_connector instances without
registering anything, and into separate functions that then register
the ports, altmodes and what have you. I don't think that should be a
huge problem. It will make ucsi.c even more like a library, which is
probable a good thing. I can prepare patches for that too if you like?

After that you should be able to get the struct ucsi instance that
represents the "real" PPM without registering anything by calling
a single function, most likely ucsi_init(). And after getting that you
can construct the connector alternate modes that we actually register.
Finally you register the final interface which does not use
ucsi_ccg_ops, but instead something like ucsi_nvidia_ops.

How would this sound to you?

Br,

-- 
heikki

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-10-03 14:24   ` Heikki Krogerus
@ 2019-10-03 16:33     ` Ajay Gupta
  2019-10-10 17:51       ` Ajay Gupta
  0 siblings, 1 reply; 24+ messages in thread
From: Ajay Gupta @ 2019-10-03 16:33 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi Heikki,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, October 3, 2019 7:25 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: linux-usb@vger.kernel.org
> Subject: Re: [PATCH 00/14] usb: typec: UCSI driver overhaul
> 
> Hi Ajay,
> 
> On Tue, Oct 01, 2019 at 06:36:25PM +0000, Ajay Gupta wrote:
> > Hi Heikki
> >
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: Thursday, September 26, 2019 3:07 AM
> > > To: Ajay Gupta <ajayg@nvidia.com>
> > > Cc: linux-usb@vger.kernel.org
> > > Subject: [PATCH 00/14] usb: typec: UCSI driver overhaul
> > >
> > > Hi Ajay,
> > >
> > > Here's the pretty much complete rewrite of the I/O handling that I
> > > was talking about. The first seven patches are not actually related
> > > to this stuff, but I'm including them here because the rest of the
> > > series is made on top of them. I'm including also that fix patch I
> > > send you earlier.
> > >
> > > After this it should be easier to handle quirks. My idea how to
> > > handle the multi-instance connector alt modes is that we "emulate"
> > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not touched at
> all.
> > >
> > > We can now get the connector alternate modes that the actual
> > > controller supplies during probe - before registering the ucsi
> > > interface
> > How can ucsi_ccg.c get the connector alternate modes before
> > registering ucsi interface? PPM reset, notification enable, etc.
> > is done during ucsi registration. UCSI spec says:
> > " The only commands the PPM is required to process in the "PPM Idle
> > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and
> > PPM_RESE"
> >
> > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of
> > the stuff done in ucsi_init() of ucsi.c.
> 
> How about if we split ucsi_init() into a function that first simply constructs the
> struct ucsi and struct ucsi_connector instances without registering anything,
> and into separate functions that then register the ports, altmodes and what
> have you. I don't think that should be a huge problem. It will make ucsi.c even
> more like a library, which is probable a good thing. 
Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ?
a) ucsi_ccg.c calls first part of split ucsi_init()
b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes.
c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate altmode found.
d1) ucsi_ccg.c calls new method to register connector alternate modes. 
This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is expected
to send squashed alternate mode.  This will require changes in .read(), .sync_write() and 
.async_write() to make it appear as if the squashed data coming from the ppm.
OR
d2) ucsi_ccg.c calls new method to register squashed connector alternate modes. 
This method doesn't issue GET_ALTERNATE_MODES commands to PPM but simply 
registers the alternate mode values passed to this function.

If you mean the (a->b->c->d2) solution then it looks fine to me and would wait for patches 
from you. This solution would mean that GET_ALTERNATE_MODES for connector is done
only by each ucsi_xxx.c and not by ucsi.c

> I can prepare patches for that too if you like? 
> After that you should be able to get the struct ucsi instance that represents
> the "real" PPM without registering anything by calling a single function, most
> likely ucsi_init(). And after getting that you can construct the connector
> alternate modes that we actually register.
> Finally you register the final interface which does not use ucsi_ccg_ops, but
> instead something like ucsi_nvidia_ops.
I didn't understand this part. ucsi_ccg_ops has .read(), .sync_write() and
 .async_write() interface and they remain same for all ucsi_ccg controllers.

Thanks
>nvpublic
> 
> How would this sound to you?
> 
> Br,
> 
> --
> heikki

^ permalink raw reply	[flat|nested] 24+ messages in thread

* RE: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-10-03 16:33     ` Ajay Gupta
@ 2019-10-10 17:51       ` Ajay Gupta
  2019-10-11 10:37         ` Heikki Krogerus
  0 siblings, 1 reply; 24+ messages in thread
From: Ajay Gupta @ 2019-10-10 17:51 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi Heikki,

> > > > Hi Ajay,
> > > >
> > > > Here's the pretty much complete rewrite of the I/O handling that I
> > > > was talking about. The first seven patches are not actually
> > > > related to this stuff, but I'm including them here because the
> > > > rest of the series is made on top of them. I'm including also that
> > > > fix patch I send you earlier.
> > > >
> > > > After this it should be easier to handle quirks. My idea how to
> > > > handle the multi-instance connector alt modes is that we "emulate"
> > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not
> > > > touched at
> > all.
> > > >
> > > > We can now get the connector alternate modes that the actual
> > > > controller supplies during probe - before registering the ucsi
> > > > interface
> > > How can ucsi_ccg.c get the connector alternate modes before
> > > registering ucsi interface? PPM reset, notification enable, etc.
> > > is done during ucsi registration. UCSI spec says:
> > > " The only commands the PPM is required to process in the "PPM Idle
> > > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and
> > > PPM_RESE"
> > >
> > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of
> > > the stuff done in ucsi_init() of ucsi.c.
> >
> > How about if we split ucsi_init() into a function that first simply
> > constructs the struct ucsi and struct ucsi_connector instances without
> > registering anything, and into separate functions that then register
> > the ports, altmodes and what have you. I don't think that should be a
> > huge problem. It will make ucsi.c even more like a library, which is probable
> a good thing.
> Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ?
> a) ucsi_ccg.c calls first part of split ucsi_init()
> b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes.
> c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate
> altmode found.
> d1) ucsi_ccg.c calls new method to register connector alternate modes.
> This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is
> expected to send squashed alternate mode.  This will require changes in
> .read(), .sync_write() and
> .async_write() to make it appear as if the squashed data coming from the ppm.
> OR
> d2) ucsi_ccg.c calls new method to register squashed connector alternate
> modes.
> This method doesn't issue GET_ALTERNATE_MODES commands to PPM but
> simply registers the alternate mode values passed to this function.
> 
> If you mean the (a->b->c->d2) solution then it looks fine to me and would wait
> for patches from you. This solution would mean that GET_ALTERNATE_MODES
> for connector is done only by each ucsi_xxx.c and not by ucsi.c

I am waiting for your comments on this.

Thanks
> nvpublic
> > I can prepare patches for that too if you like?
> > After that you should be able to get the struct ucsi instance that
> > represents the "real" PPM without registering anything by calling a
> > single function, most likely ucsi_init(). And after getting that you
> > can construct the connector alternate modes that we actually register.
> > Finally you register the final interface which does not use
> > ucsi_ccg_ops, but instead something like ucsi_nvidia_ops.
> I didn't understand this part. ucsi_ccg_ops has .read(), .sync_write() and
>  .async_write() interface and they remain same for all ucsi_ccg controllers.
> 
> Thanks
> > How would this sound to you?
> >
> > Br,
> >
> > --
> > heikki

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 00/14] usb: typec: UCSI driver overhaul
  2019-10-10 17:51       ` Ajay Gupta
@ 2019-10-11 10:37         ` Heikki Krogerus
  0 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-10-11 10:37 UTC (permalink / raw)
  To: Ajay Gupta; +Cc: linux-usb

Hi,

On Thu, Oct 10, 2019 at 05:51:23PM +0000, Ajay Gupta wrote:
> Hi Heikki,
> 
> > > > > Hi Ajay,
> > > > >
> > > > > Here's the pretty much complete rewrite of the I/O handling that I
> > > > > was talking about. The first seven patches are not actually
> > > > > related to this stuff, but I'm including them here because the
> > > > > rest of the series is made on top of them. I'm including also that
> > > > > fix patch I send you earlier.
> > > > >
> > > > > After this it should be easier to handle quirks. My idea how to
> > > > > handle the multi-instance connector alt modes is that we "emulate"
> > > > > the PPM in ucsi_ccg.c in order to handle them, so ucsi.c is not
> > > > > touched at
> > > all.
> > > > >
> > > > > We can now get the connector alternate modes that the actual
> > > > > controller supplies during probe - before registering the ucsi
> > > > > interface
> > > > How can ucsi_ccg.c get the connector alternate modes before
> > > > registering ucsi interface? PPM reset, notification enable, etc.
> > > > is done during ucsi registration. UCSI spec says:
> > > > " The only commands the PPM is required to process in the "PPM Idle
> > > > (Notifications Disabled)" state are SET_NOTIFICATION_ENABLE and
> > > > PPM_RESE"
> > > >
> > > > Also, it doesn't look correct if ucsi_ccg.c has to replicate most of
> > > > the stuff done in ucsi_init() of ucsi.c.
> > >
> > > How about if we split ucsi_init() into a function that first simply
> > > constructs the struct ucsi and struct ucsi_connector instances without
> > > registering anything, and into separate functions that then register
> > > the ports, altmodes and what have you. I don't think that should be a
> > > huge problem. It will make ucsi.c even more like a library, which is probable
> > a good thing.
> > Do you mean the solution to follow steps (a->b->c->d1) or (a->b->c->d2) ?
> > a) ucsi_ccg.c calls first part of split ucsi_init()
> > b) ucsi_ccg.c uses ucsi_send_command() to collect actual alternate modes.
> > c) ucsi_ccg.c looks into actual alternate modes and squashes if duplicate
> > altmode found.
> > d1) ucsi_ccg.c calls new method to register connector alternate modes.
> > This method issues GET_ALTERNATE_MODES command again and ucsi_ccg.c is
> > expected to send squashed alternate mode.  This will require changes in
> > .read(), .sync_write() and
> > .async_write() to make it appear as if the squashed data coming from the ppm.
> > OR
> > d2) ucsi_ccg.c calls new method to register squashed connector alternate
> > modes.
> > This method doesn't issue GET_ALTERNATE_MODES commands to PPM but
> > simply registers the alternate mode values passed to this function.
> > 
> > If you mean the (a->b->c->d2) solution then it looks fine to me and would wait
> > for patches from you. This solution would mean that GET_ALTERNATE_MODES
> > for connector is done only by each ucsi_xxx.c and not by ucsi.c
> 
> I am waiting for your comments on this.

Sorry Ajay. I lost track of this thread.

The above is not exactly what I had in mind, but something like that.
But in any case, I'll start wringing the patches then.


thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-10-11 10:37 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 10:07 [PATCH 00/14] usb: typec: UCSI driver overhaul Heikki Krogerus
2019-09-26 10:07 ` [PATCH 01/14] usb: typec: Copy everything from struct typec_capability during registration Heikki Krogerus
2019-09-26 10:07 ` [PATCH 02/14] usb: typec: Introduce typec_get_drvdata() Heikki Krogerus
2019-09-26 10:07 ` [PATCH 03/14] usb: typec: Separate the operations vector Heikki Krogerus
2019-09-26 10:07 ` [PATCH 04/14] usb: typec: tcpm: Start using struct typec_operations Heikki Krogerus
2019-09-26 10:07 ` [PATCH 05/14] usb: typec: tps6598x: " Heikki Krogerus
2019-09-26 10:07 ` [PATCH 06/14] usb: typec: ucsi: " Heikki Krogerus
2019-09-26 10:07 ` [PATCH 07/14] usb: typec: Remove the callback members from struct typec_capability Heikki Krogerus
2019-09-26 10:07 ` [PATCH 08/14] usb: typec: ucsi: ccg: Remove run_isr flag Heikki Krogerus
2019-09-26 10:07 ` [PATCH 09/14] usb: typec: ucsi: Simplified interface registration and I/O API Heikki Krogerus
2019-09-26 14:25 ` [PATCH 10/14] usb: typec: ucsi: acpi: Move to the new API Heikki Krogerus
2019-09-26 14:25 ` [PATCH 11/14] usb: typec: ucsi: ccg: " Heikki Krogerus
2019-09-26 14:25 ` [PATCH 12/14] usb: typec: ucsi: Remove the old API Heikki Krogerus
2019-09-26 14:25 ` [PATCH 13/14] usb: typec: ucsi: Remove struct ucsi_control Heikki Krogerus
2019-09-26 14:25 ` [PATCH 14/14] usb: typec: ucsi: Remove all bit-fields Heikki Krogerus
2019-09-27  0:13 ` [PATCH 00/14] usb: typec: UCSI driver overhaul Ajay Gupta
2019-09-27  9:44   ` Heikki Krogerus
2019-09-27 12:53   ` Heikki Krogerus
2019-09-27 16:30     ` Ajay Gupta
2019-10-01 18:36 ` Ajay Gupta
2019-10-03 14:24   ` Heikki Krogerus
2019-10-03 16:33     ` Ajay Gupta
2019-10-10 17:51       ` Ajay Gupta
2019-10-11 10:37         ` Heikki Krogerus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).