[PATCH v3 00/18] usb: typec: API improvements

[PATCH v3 00/18] usb: typec: API improvements

[Heikki Krogerus]

Hi,

I modified ucsi_acpi.c so that the behavior matches exactly the
behaviour of the Connector Change Event handling before these patches.

The cover letter from v2:

There is now a check in ucsi_exec_command() that makes sure we do not
call ucsi_read_error() with UCSI_GET_ERROR_STATUS command. That should
prevent endless recursion from happening.

The original cover letter:

The first patches in this series (patches 1-8) introduce a small
change to the USB Type-C Connector Class API. Guenter was kind enough
to go over those already.

Patches 10-15 improve the ucsi driver API by introducing more
traditional read and write routines, and the rest is more generic
optimisations and improvements to the ucsi drivers.

Let me know if there is anything you want to be changed.

thanks,

Heikki Krogerus (18):
  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: hd3ss3220: Start using struct typec_operations
  usb: typec: Remove the callback members from struct typec_capability
  usb: typec: Remove unused members from struct typec_capability
  usb: typec: hd3ss3220: Give the connector fwnode to the port device
  usb: typec: ucsi: Simplified 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
  usb: typec: ucsi: New error codes
  usb: typec: ucsi: Optimise ucsi_unregister()

 drivers/usb/typec/class.c            |  42 +-
 drivers/usb/typec/hd3ss3220.c        |  36 +-
 drivers/usb/typec/tcpm/tcpm.c        |  45 +-
 drivers/usb/typec/tps6598x.c         |  49 ++-
 drivers/usb/typec/ucsi/displayport.c |  40 +-
 drivers/usb/typec/ucsi/trace.c       |  11 -
 drivers/usb/typec/ucsi/trace.h       |  79 +---
 drivers/usb/typec/ucsi/ucsi.c        | 609 ++++++++++++++-------------
 drivers/usb/typec/ucsi/ucsi.h        | 417 +++++++-----------
 drivers/usb/typec/ucsi/ucsi_acpi.c   |  93 +++-
 drivers/usb/typec/ucsi/ucsi_ccg.c    | 170 ++++----
 include/linux/usb/typec.h            |  41 +-
 12 files changed, 776 insertions(+), 856 deletions(-)

-- 
2.23.0

[PATCH v3 01/18] usb: typec: Copy everything from struct typec_capability during registration

[Heikki Krogerus]

Copying everything from struct typec_capability to struct
typec_port during port registration. This will make sure
that under no circumstances the driver can change the values
in the struct typec_capability that the port uses.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/class.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 94a3eda62add..7749933ffce5 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -52,6 +52,7 @@ struct typec_port {
 	struct typec_switch		*sw;
 	struct typec_mux		*mux;
 
+	const struct typec_capability	*orig_cap; /* to be removed */
 	const struct typec_capability	*cap;
 };
 
@@ -968,7 +969,7 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 			return -EINVAL;
 	}
 
-	ret = port->cap->try_role(port->cap, role);
+	ret = port->cap->try_role(port->orig_cap, role);
 	if (ret)
 		return ret;
 
@@ -1014,7 +1015,7 @@ static ssize_t data_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->dr_set(port->cap, ret);
+	ret = port->cap->dr_set(port->orig_cap, ret);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1071,7 +1072,7 @@ static ssize_t power_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->pr_set(port->cap, ret);
+	ret = port->cap->pr_set(port->orig_cap, ret);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1119,7 +1120,7 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->port_type_set(port->cap, type);
+	ret = port->cap->port_type_set(port->orig_cap, type);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1184,7 +1185,7 @@ static ssize_t vconn_source_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = port->cap->vconn_set(port->cap, (enum typec_role)source);
+	ret = port->cap->vconn_set(port->orig_cap, (enum typec_role)source);
 	if (ret)
 		return ret;
 
@@ -1278,6 +1279,7 @@ static void typec_release(struct device *dev)
 	ida_destroy(&port->mode_ids);
 	typec_switch_put(port->sw);
 	typec_mux_put(port->mux);
+	kfree(port->cap);
 	kfree(port);
 }
 
@@ -1579,7 +1581,7 @@ struct typec_port *typec_register_port(struct device *parent,
 	mutex_init(&port->port_type_lock);
 
 	port->id = id;
-	port->cap = cap;
+	port->orig_cap = cap;
 	port->port_type = cap->type;
 	port->prefer_role = cap->prefer_role;
 
@@ -1590,6 +1592,12 @@ struct typec_port *typec_register_port(struct device *parent,
 	port->dev.type = &typec_port_dev_type;
 	dev_set_name(&port->dev, "port%d", id);
 
+	port->cap = kmemdup(cap, sizeof(*cap), GFP_KERNEL);
+	if (!port->cap) {
+		put_device(&port->dev);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	port->sw = typec_switch_get(&port->dev);
 	if (IS_ERR(port->sw)) {
 		put_device(&port->dev);
-- 
2.23.0

[PATCH v3 02/18] usb: typec: Introduce typec_get_drvdata()

[Heikki Krogerus]

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

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 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 7749933ffce5..51154634c2c0 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1488,6 +1488,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
@@ -1591,6 +1601,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->cap = kmemdup(cap, sizeof(*cap), GFP_KERNEL);
 	if (!port->cap) {
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

[PATCH v3 03/18] usb: typec: Separate the operations vector

[Heikki Krogerus]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/class.c | 39 +++++++++++++++++++++++++++++----------
 include/linux/usb/typec.h | 20 ++++++++++++++++++++
 2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 51154634c2c0..b934a006535a 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -54,6 +54,7 @@ struct typec_port {
 
 	const struct typec_capability	*orig_cap; /* to be removed */
 	const struct typec_capability	*cap;
+	const struct typec_operations   *ops;
 };
 
 #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
@@ -956,7 +957,7 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->try_role) {
+	if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
 		dev_dbg(dev, "Setting preferred role not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -969,7 +970,10 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 			return -EINVAL;
 	}
 
-	ret = port->cap->try_role(port->orig_cap, role);
+	if (port->ops && port->ops->try_role)
+		ret = port->ops->try_role(port, role);
+	else
+		ret = port->cap->try_role(port->orig_cap, role);
 	if (ret)
 		return ret;
 
@@ -1000,7 +1004,7 @@ static ssize_t data_role_store(struct device *dev,
 	struct typec_port *port = to_typec_port(dev);
 	int ret;
 
-	if (!port->cap->dr_set) {
+	if (!port->cap->dr_set && (!port->ops || !port->ops->dr_set)) {
 		dev_dbg(dev, "data role swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1015,7 +1019,10 @@ static ssize_t data_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->dr_set(port->orig_cap, ret);
+	if (port->ops && port->ops->dr_set)
+		ret = port->ops->dr_set(port, ret);
+	else
+		ret = port->cap->dr_set(port->orig_cap, ret);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1050,7 +1057,7 @@ static ssize_t power_role_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->pr_set) {
+	if (!port->cap->pr_set && (!port->ops || !port->ops->pr_set)) {
 		dev_dbg(dev, "power role swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1072,7 +1079,10 @@ static ssize_t power_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->pr_set(port->orig_cap, ret);
+	if (port->ops && port->ops->dr_set)
+		ret = port->ops->pr_set(port, ret);
+	else
+		ret = port->cap->pr_set(port->orig_cap, ret);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1103,7 +1113,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->cap->type != TYPEC_PORT_DRP) {
+	if (port->cap->type != TYPEC_PORT_DRP || (!port->cap->port_type_set &&
+	    (!port->ops || !port->ops->port_type_set))) {
 		dev_dbg(dev, "changing port type not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1120,7 +1131,10 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 		goto unlock_and_ret;
 	}
 
-	ret = port->cap->port_type_set(port->orig_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->orig_cap, type);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1176,7 +1190,7 @@ static ssize_t vconn_source_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->vconn_set) {
+	if (!port->cap->vconn_set && (!port->ops || !port->ops->vconn_set)) {
 		dev_dbg(dev, "VCONN swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1185,7 +1199,11 @@ static ssize_t vconn_source_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = port->cap->vconn_set(port->orig_cap, (enum typec_role)source);
+	if (port->ops && port->ops->vconn_set)
+		ret = port->ops->vconn_set(port, (enum typec_role)source);
+	else
+		ret = port->cap->vconn_set(port->orig_cap,
+					   (enum typec_role)source);
 	if (ret)
 		return ret;
 
@@ -1591,6 +1609,7 @@ struct typec_port *typec_register_port(struct device *parent,
 	mutex_init(&port->port_type_lock);
 
 	port->id = id;
+	port->ops = cap->ops;
 	port->orig_cap = cap;
 	port->port_type = cap->type;
 	port->prefer_role = cap->prefer_role;
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 8b90cd77331c..c9bef128453b 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -168,6 +168,23 @@ 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 role);
+	int (*pr_set)(struct typec_port *port, enum typec_role role);
+	int (*vconn_set)(struct typec_port *port, enum typec_role role);
+	int (*port_type_set)(struct typec_port *port,
+			     enum typec_port_type type);
+};
+
 /*
  * struct typec_capability - USB Type-C Port Capabilities
  * @type: Supported power role of the port
@@ -180,6 +197,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 +219,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

[PATCH v3 04/18] usb: typec: tcpm: Start using struct typec_operations

[Heikki Krogerus]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/tcpm/tcpm.c | 45 ++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 5f61d9977a15..bc9edb4c013b 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, 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);
@@ -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);
@@ -4772,11 +4770,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

[PATCH v3 05/18] usb: typec: tps6598x: Start using struct typec_operations

[Heikki Krogerus]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 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

[PATCH v3 06/18] usb: typec: ucsi: Start using struct typec_operations

[Heikki Krogerus]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 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

[PATCH v3 07/18] usb: typec: hd3ss3220: Start using struct typec_operations

[Heikki Krogerus]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/hd3ss3220.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 8afaf5768a17..db09fa0d85f2 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -37,7 +37,6 @@ struct hd3ss3220 {
 	struct regmap *regmap;
 	struct usb_role_switch	*role_sw;
 	struct typec_port *port;
-	struct typec_capability typec_cap;
 };
 
 static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
@@ -73,11 +72,9 @@ static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220 *hd3ss3220)
 	return attached_state;
 }
 
-static int hd3ss3220_dr_set(const struct typec_capability *cap,
-			    enum typec_data_role role)
+static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role)
 {
-	struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
-						   typec_cap);
+	struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
 	enum usb_role role_val;
 	int pref, ret = 0;
 
@@ -98,6 +95,10 @@ static int hd3ss3220_dr_set(const struct typec_capability *cap,
 	return ret;
 }
 
+static const struct typec_operations hd3ss3220_ops = {
+	.dr_set = hd3ss3220_dr_set
+};
+
 static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
 {
 	enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
@@ -152,6 +153,7 @@ static const struct regmap_config config = {
 static int hd3ss3220_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
+	struct typec_capability typec_cap = { };
 	struct hd3ss3220 *hd3ss3220;
 	struct fwnode_handle *connector;
 	int ret;
@@ -180,13 +182,13 @@ static int hd3ss3220_probe(struct i2c_client *client,
 	if (IS_ERR(hd3ss3220->role_sw))
 		return PTR_ERR(hd3ss3220->role_sw);
 
-	hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
-	hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
-	hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
-	hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
+	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
+	typec_cap.driver_data = hd3ss3220;
+	typec_cap.type = TYPEC_PORT_DRP;
+	typec_cap.data = TYPEC_PORT_DRD;
+	typec_cap.ops = &hd3ss3220_ops;
 
-	hd3ss3220->port = typec_register_port(&client->dev,
-					      &hd3ss3220->typec_cap);
+	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(hd3ss3220->port)) {
 		ret = PTR_ERR(hd3ss3220->port);
 		goto err_put_role;
-- 
2.23.0

[PATCH v3 08/18] usb: typec: Remove the callback members from struct typec_capability

[Heikki Krogerus]

There are no more users for them.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/typec/class.c | 40 +++++++++++----------------------------
 include/linux/usb/typec.h | 17 -----------------
 2 files changed, 11 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index b934a006535a..7ece6ca6e690 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -52,7 +52,6 @@ struct typec_port {
 	struct typec_switch		*sw;
 	struct typec_mux		*mux;
 
-	const struct typec_capability	*orig_cap; /* to be removed */
 	const struct typec_capability	*cap;
 	const struct typec_operations   *ops;
 };
@@ -957,7 +956,7 @@ preferred_role_store(struct device *dev, struct device_attribute *attr,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
+	if (!port->ops || !port->ops->try_role) {
 		dev_dbg(dev, "Setting preferred role not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -970,10 +969,7 @@ 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);
-	else
-		ret = port->cap->try_role(port->orig_cap, role);
+	ret = port->ops->try_role(port, role);
 	if (ret)
 		return ret;
 
@@ -1004,7 +1000,7 @@ static ssize_t data_role_store(struct device *dev,
 	struct typec_port *port = to_typec_port(dev);
 	int ret;
 
-	if (!port->cap->dr_set && (!port->ops || !port->ops->dr_set)) {
+	if (!port->ops || !port->ops->dr_set) {
 		dev_dbg(dev, "data role swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1019,10 +1015,7 @@ 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);
-	else
-		ret = port->cap->dr_set(port->orig_cap, ret);
+	ret = port->ops->dr_set(port, ret);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1057,7 +1050,7 @@ static ssize_t power_role_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->pr_set && (!port->ops || !port->ops->pr_set)) {
+	if (!port->ops || !port->ops->pr_set) {
 		dev_dbg(dev, "power role swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1079,10 +1072,7 @@ static ssize_t power_role_store(struct device *dev,
 		goto unlock_and_ret;
 	}
 
-	if (port->ops && port->ops->dr_set)
-		ret = port->ops->pr_set(port, ret);
-	else
-		ret = port->cap->pr_set(port->orig_cap, ret);
+	ret = port->ops->pr_set(port, ret);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1113,8 +1103,8 @@ port_type_store(struct device *dev, struct device_attribute *attr,
 	int ret;
 	enum typec_port_type type;
 
-	if (port->cap->type != TYPEC_PORT_DRP || (!port->cap->port_type_set &&
-	    (!port->ops || !port->ops->port_type_set))) {
+	if (port->cap->type != TYPEC_PORT_DRP ||
+	    !port->ops || !port->ops->port_type_set) {
 		dev_dbg(dev, "changing port type not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1131,10 +1121,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->orig_cap, type);
+	ret = port->ops->port_type_set(port, type);
 	if (ret)
 		goto unlock_and_ret;
 
@@ -1190,7 +1177,7 @@ static ssize_t vconn_source_store(struct device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!port->cap->vconn_set && (!port->ops || !port->ops->vconn_set)) {
+	if (!port->ops || !port->ops->vconn_set) {
 		dev_dbg(dev, "VCONN swapping not supported\n");
 		return -EOPNOTSUPP;
 	}
@@ -1199,11 +1186,7 @@ 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, (enum typec_role)source);
-	else
-		ret = port->cap->vconn_set(port->orig_cap,
-					   (enum typec_role)source);
+	ret = port->ops->vconn_set(port, (enum typec_role)source);
 	if (ret)
 		return ret;
 
@@ -1610,7 +1593,6 @@ struct typec_port *typec_register_port(struct device *parent,
 
 	port->id = id;
 	port->ops = cap->ops;
-	port->orig_cap = cap;
 	port->port_type = cap->type;
 	port->prefer_role = cap->prefer_role;
 
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index c9bef128453b..894798084319 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -198,11 +198,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.
  */
@@ -220,18 +215,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

[PATCH v3 09/18] usb: typec: Remove unused members from struct typec_capability

[Heikki Krogerus]

The members for the muxes are not used, so dropping them.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/usb/typec.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index 894798084319..0f52723a11bd 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -209,8 +209,6 @@ struct typec_capability {
 	int			prefer_role;
 	enum typec_accessory	accessory[TYPEC_MAX_ACCESSORY];
 
-	struct typec_switch	*sw;
-	struct typec_mux	*mux;
 	struct fwnode_handle	*fwnode;
 	void			*driver_data;
 
-- 
2.23.0

[PATCH v3 10/18] usb: typec: hd3ss3220: Give the connector fwnode to the port device

[Heikki Krogerus]

The driver already finds the node in order to get reference
to the USB role switch.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Biju Das <biju.das@bp.renesas.com>
---
 drivers/usb/typec/hd3ss3220.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index db09fa0d85f2..323dfa8160ab 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -178,15 +178,17 @@ static int hd3ss3220_probe(struct i2c_client *client,
 		return -ENODEV;
 
 	hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
-	fwnode_handle_put(connector);
-	if (IS_ERR(hd3ss3220->role_sw))
-		return PTR_ERR(hd3ss3220->role_sw);
+	if (IS_ERR(hd3ss3220->role_sw)) {
+		ret = PTR_ERR(hd3ss3220->role_sw);
+		goto err_put_fwnode;
+	}
 
 	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
 	typec_cap.driver_data = hd3ss3220;
 	typec_cap.type = TYPEC_PORT_DRP;
 	typec_cap.data = TYPEC_PORT_DRD;
 	typec_cap.ops = &hd3ss3220_ops;
+	typec_cap.fwnode = connector;
 
 	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(hd3ss3220->port)) {
@@ -220,6 +222,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto err_unreg_port;
 
+	fwnode_handle_put(connector);
+
 	dev_info(&client->dev, "probed revision=0x%x\n", ret);
 
 	return 0;
@@ -227,6 +231,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
 	typec_unregister_port(hd3ss3220->port);
 err_put_role:
 	usb_role_switch_put(hd3ss3220->role_sw);
+err_put_fwnode:
+	fwnode_handle_put(connector);
 
 	return ret;
 }
-- 
2.23.0

Re: [PATCH v3 10/18] usb: typec: hd3ss3220: Give the connector fwnode to the port device

[Guenter Roeck]

On 10/25/19 1:23 AM, Heikki Krogerus wrote:
> The driver already finds the node in order to get reference
> to the USB role switch.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Tested-by: Biju Das <biju.das@bp.renesas.com>

The fusb302 driver retains the fwnode and calls fwnode_handle_put()
in the remove function. Assuming this isn't necessary here,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/hd3ss3220.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index db09fa0d85f2..323dfa8160ab 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -178,15 +178,17 @@ static int hd3ss3220_probe(struct i2c_client *client,
>   		return -ENODEV;
>   
>   	hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> -	fwnode_handle_put(connector);
> -	if (IS_ERR(hd3ss3220->role_sw))
> -		return PTR_ERR(hd3ss3220->role_sw);
> +	if (IS_ERR(hd3ss3220->role_sw)) {
> +		ret = PTR_ERR(hd3ss3220->role_sw);
> +		goto err_put_fwnode;
> +	}
>   
>   	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
>   	typec_cap.driver_data = hd3ss3220;
>   	typec_cap.type = TYPEC_PORT_DRP;
>   	typec_cap.data = TYPEC_PORT_DRD;
>   	typec_cap.ops = &hd3ss3220_ops;
> +	typec_cap.fwnode = connector;
>   
>   	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
>   	if (IS_ERR(hd3ss3220->port)) {
> @@ -220,6 +222,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
>   	if (ret < 0)
>   		goto err_unreg_port;
>   
> +	fwnode_handle_put(connector);
> +
>   	dev_info(&client->dev, "probed revision=0x%x\n", ret);
>   
>   	return 0;
> @@ -227,6 +231,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
>   	typec_unregister_port(hd3ss3220->port);
>   err_put_role:
>   	usb_role_switch_put(hd3ss3220->role_sw);
> +err_put_fwnode:
> +	fwnode_handle_put(connector);
>   
>   	return ret;
>   }
> 

RE: [PATCH v3 10/18] usb: typec: hd3ss3220: Give the connector fwnode to the port device

[Biju Das]

Hi Guenter,

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Saturday, November 2, 2019 4:11 PM
> To: Heikki Krogerus <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Cc: Ajay Gupta <ajayg@nvidia.com>; linux-usb@vger.kernel.org; Biju Das
> <biju.das@bp.renesas.com>
> Subject: Re: [PATCH v3 10/18] usb: typec: hd3ss3220: Give the connector
> fwnode to the port device
> 
> On 10/25/19 1:23 AM, Heikki Krogerus wrote:
> > The driver already finds the node in order to get reference to the USB
> > role switch.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Tested-by: Biju Das <biju.das@bp.renesas.com>
> 
> The fusb302 driver retains the fwnode and calls fwnode_handle_put() in the
> remove function. Assuming this isn't necessary here,

It is not required. The purpose is  to get the role switch handle associated with the connecter node.
Once you get the role switch handle,  you don't need the connector handle any more. 

Cheers,
Biju

> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> > ---
> >   drivers/usb/typec/hd3ss3220.c | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/hd3ss3220.c
> > b/drivers/usb/typec/hd3ss3220.c index db09fa0d85f2..323dfa8160ab
> > 100644
> > --- a/drivers/usb/typec/hd3ss3220.c
> > +++ b/drivers/usb/typec/hd3ss3220.c
> > @@ -178,15 +178,17 @@ static int hd3ss3220_probe(struct i2c_client
> *client,
> >   		return -ENODEV;
> >
> >   	hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
> > -	fwnode_handle_put(connector);
> > -	if (IS_ERR(hd3ss3220->role_sw))
> > -		return PTR_ERR(hd3ss3220->role_sw);
> > +	if (IS_ERR(hd3ss3220->role_sw)) {
> > +		ret = PTR_ERR(hd3ss3220->role_sw);
> > +		goto err_put_fwnode;
> > +	}
> >
> >   	typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> >   	typec_cap.driver_data = hd3ss3220;
> >   	typec_cap.type = TYPEC_PORT_DRP;
> >   	typec_cap.data = TYPEC_PORT_DRD;
> >   	typec_cap.ops = &hd3ss3220_ops;
> > +	typec_cap.fwnode = connector;
> >
> >   	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
> >   	if (IS_ERR(hd3ss3220->port)) {
> > @@ -220,6 +222,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
> >   	if (ret < 0)
> >   		goto err_unreg_port;
> >
> > +	fwnode_handle_put(connector);
> > +
> >   	dev_info(&client->dev, "probed revision=0x%x\n", ret);
> >
> >   	return 0;
> > @@ -227,6 +231,8 @@ static int hd3ss3220_probe(struct i2c_client *client,
> >   	typec_unregister_port(hd3ss3220->port);
> >   err_put_role:
> >   	usb_role_switch_put(hd3ss3220->role_sw);
> > +err_put_fwnode:
> > +	fwnode_handle_put(connector);
> >
> >   	return ret;
> >   }
> >

[PATCH v3 11/18] usb: typec: ucsi: Simplified registration and I/O API

[Heikki Krogerus]

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>
Tested-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 330 +++++++++++++++++++++++++++++++---
 drivers/usb/typec/ucsi/ucsi.h |  58 ++++++
 2 files changed, 359 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index edd722fb88b8..dffc2cf8db6f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -98,6 +98,101 @@ 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) {
+		if (cmd == UCSI_GET_ERROR_STATUS)
+			return -EIO;
+		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 +201,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 +633,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 +695,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 +709,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 +779,40 @@ 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 the PPM is still doing something else, reset it again. */
+			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 +973,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 +1064,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 +1127,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 +1141,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)
+{
+	int ret;
+
+	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
+			      sizeof(ucsi->version));
+	if (ret)
+		return ret;
+
+	if (!ucsi->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 +1263,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..d8a8e8f2f912 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)
@@ -383,8 +437,12 @@ enum ucsi_status {
 };
 
 struct ucsi {
+	u16 version;
 	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

Re: [PATCH v3 11/18] usb: typec: ucsi: Simplified registration and I/O API

[Guenter Roeck]

On 10/25/19 1:23 AM, Heikki Krogerus wrote:
> 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>
> Tested-by: Ajay Gupta <ajayg@nvidia.com>

I am still a bit concerned about log spam, eg if a battery is
dead for good, or if there is an actual driver bug, but I guess
we can address that later if needed.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/ucsi/ucsi.c | 330 +++++++++++++++++++++++++++++++---
>   drivers/usb/typec/ucsi/ucsi.h |  58 ++++++
>   2 files changed, 359 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index edd722fb88b8..dffc2cf8db6f 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -98,6 +98,101 @@ 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) {
> +		if (cmd == UCSI_GET_ERROR_STATUS)
> +			return -EIO;
> +		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 +201,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 +633,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 +695,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 +709,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 +779,40 @@ 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 the PPM is still doing something else, reset it again. */
> +			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 +973,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 +1064,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 +1127,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 +1141,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)
> +{
> +	int ret;
> +
> +	ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
> +			      sizeof(ucsi->version));
> +	if (ret)
> +		return ret;
> +
> +	if (!ucsi->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 +1263,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..d8a8e8f2f912 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)
> @@ -383,8 +437,12 @@ enum ucsi_status {
>   };
>   
>   struct ucsi {
> +	u16 version;
>   	struct device *dev;
>   	struct ucsi_ppm *ppm;
> +	struct driver_data *driver_data;
> +
> +	const struct ucsi_operations *ops;
>   
>   	enum ucsi_status status;
>   	struct completion complete;
> 

[PATCH v3 12/18] usb: typec: ucsi: acpi: Move to the new API

[Heikki Krogerus]

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 | 93 ++++++++++++++++++++++++------
 1 file changed, 74 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index a18112a83fae..88d2401e3e61 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,75 @@ 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;
+
+	memcpy(val, (const void __force *)(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);
 
-	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
+	memcpy((void __force *)(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;
 
-	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
+	if (!wait_for_completion_timeout(&ua->complete, msecs_to_jiffies(5000)))
+		ret = -ETIMEDOUT;
+
+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;
+	}
 
-	ucsi_notify(ua->ucsi);
+	if (test_bit(COMMAND_PENDING, &ua->flags) &&
+	    cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
+		complete(&ua->complete);
+	else if (UCSI_CCI_CONNECTOR(cci))
+		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
 }
 
 static int ucsi_acpi_probe(struct platform_device *pdev)
@@ -90,35 +140,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 +184,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

Re: [PATCH v3 12/18] usb: typec: ucsi: acpi: Move to the new API

[Guenter Roeck]

On 10/25/19 1:23 AM, Heikki Krogerus wrote:
> 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 | 93 ++++++++++++++++++++++++------
>   1 file changed, 74 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index a18112a83fae..88d2401e3e61 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,75 @@ 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;
> +
> +	memcpy(val, (const void __force *)(ua->base + offset), val_len);
> +

Would it be better to use memcpy_fromio() and memcpy_toio() if
ua->base indeed points to iomem ?

> +	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);
>   
> -	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> +	memcpy((void __force *)(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;
>   
> -	return ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ);
> +	if (!wait_for_completion_timeout(&ua->complete, msecs_to_jiffies(5000)))
> +		ret = -ETIMEDOUT;
> +
> +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");

If I follow the call chain correctly, this would be from ucsi_acpi_dsm(),
which already logs an error message.

> +		return;
> +	}
>   
> -	ucsi_notify(ua->ucsi);
> +	if (test_bit(COMMAND_PENDING, &ua->flags) &&
> +	    cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))
> +		complete(&ua->complete);
> +	else if (UCSI_CCI_CONNECTOR(cci))
> +		ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
>   }
>   
>   static int ucsi_acpi_probe(struct platform_device *pdev)
> @@ -90,35 +140,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 +184,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);
> 

Re: [PATCH v3 12/18] usb: typec: ucsi: acpi: Move to the new API

[Heikki Krogerus]

Hi,

On Sat, Nov 02, 2019 at 09:31:29AM -0700, Guenter Roeck wrote:
> > -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;
> > +
> > +	memcpy(val, (const void __force *)(ua->base + offset), val_len);
> > +
> 
> Would it be better to use memcpy_fromio() and memcpy_toio() if
> ua->base indeed points to iomem ?

No, it's not really iomem. It's just supplied to the driver as such.

thanks,

-- 
heikki

[PATCH v3 13/18] usb: typec: ucsi: ccg: Move to the new API

[Heikki Krogerus]

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>
Tested-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 170 +++++++++++++++---------------
 1 file changed, 85 insertions(+), 85 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index d772fce51905..43442580a13c 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,89 @@ 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_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 +1030,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 +1050,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 +1094,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 +1130,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 +1159,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 +1174,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

Re: [PATCH v3 13/18] usb: typec: ucsi: ccg: Move to the new API

[Guenter Roeck]

On 10/25/19 1:23 AM, Heikki Krogerus wrote:
> 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>
> Tested-by: Ajay Gupta <ajayg@nvidia.com>
> ---
>   drivers/usb/typec/ucsi/ucsi_ccg.c | 170 +++++++++++++++---------------
>   1 file changed, 85 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index d772fce51905..43442580a13c 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,89 @@ 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");

ccg_read() already logs an error message.

> +		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_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");

ccg_write() already logs an error message.

>   
>   	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 +1030,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 +1050,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 +1094,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 +1130,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 +1159,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 +1174,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;
> 

Re: [PATCH v3 13/18] usb: typec: ucsi: ccg: Move to the new API

[Heikki Krogerus]

On Sat, Nov 02, 2019 at 09:34:55AM -0700, Guenter Roeck wrote:
> >   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");
> 
> ccg_read() already logs an error message.
> 
> > +		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_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");
> 
> ccg_write() already logs an error message.

OK. I'll drop both errors.

> >   	return IRQ_HANDLED;
> >   }

thanks,

-- 
heikki

[PATCH v3 14/18] usb: typec: ucsi: Remove the old API

[Heikki Krogerus]

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

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/displayport.c |  24 +-
 drivers/usb/typec/ucsi/trace.h       |  17 --
 drivers/usb/typec/ucsi/ucsi.c        | 346 +++------------------------
 drivers/usb/typec/ucsi/ucsi.h        |  41 ----
 4 files changed, 43 insertions(+), 385 deletions(-)

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index d99700cb4dca..47424935bc81 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -48,6 +48,7 @@ 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;
 	int ret;
@@ -59,25 +60,21 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 
 		dev_warn(&p->dev,
 			 "firmware doesn't support alternate mode overriding\n");
-		mutex_unlock(&dp->con->lock);
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		goto err_unlock;
 	}
 
 	UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
-	ret = ucsi_send_command(dp->con->ucsi, &ctrl, &cur, sizeof(cur));
+	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
 	if (ret < 0) {
-		if (dp->con->ucsi->ppm->data->version > 0x0100) {
-			mutex_unlock(&dp->con->lock);
-			return ret;
-		}
+		if (ucsi->version > 0x0100)
+			goto err_unlock;
 		cur = 0xff;
 	}
 
 	if (cur != 0xff) {
-		mutex_unlock(&dp->con->lock);
-		if (dp->con->port_altmode[cur] == alt)
-			return 0;
-		return -EBUSY;
+		ret = dp->con->port_altmode[cur] == alt ? 0 : -EBUSY;
+		goto err_unlock;
 	}
 
 	/*
@@ -94,10 +91,11 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 	dp->vdo_size = 1;
 
 	schedule_work(&dp->work);
-
+	ret = 0;
+err_unlock:
 	mutex_unlock(&dp->con->lock);
 
-	return 0;
+	return ret;
 }
 
 static int ucsi_displayport_exit(struct typec_altmode *alt)
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 dffc2cf8db6f..6462dadd7540 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;
@@ -196,115 +134,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,
@@ -334,7 +183,7 @@ 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;
 	int ret;
 	u8 cur;
 	int i;
@@ -342,7 +191,7 @@ 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) {
+		if (con->ucsi->version > 0x0100) {
 			dev_err(con->ucsi->dev,
 				"GET_CURRENT_CAM command failed\n");
 			return;
@@ -695,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);
 
@@ -723,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)
@@ -775,83 +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 the PPM is still doing something else, reset it again. */
-			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)
@@ -1266,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 d8a8e8f2f912..29f9e7f0d212 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -398,54 +398,13 @@ 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 {
 	u16 version;
 	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

[PATCH v3 15/18] usb: typec: ucsi: Remove struct ucsi_control

[Heikki Krogerus]

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>
Tested-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/displayport.c |  16 +-
 drivers/usb/typec/ucsi/trace.c       |  11 --
 drivers/usb/typec/ucsi/trace.h       |  50 +-----
 drivers/usb/typec/ucsi/ucsi.c        | 107 +++++++------
 drivers/usb/typec/ucsi/ucsi.h        | 231 +++++----------------------
 5 files changed, 115 insertions(+), 300 deletions(-)

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 47424935bc81..d4d5189edfb8 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;
 	int ret;
 
@@ -64,7 +64,7 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
 		goto err_unlock;
 	}
 
-	UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
+	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
 	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
 	if (ret < 0) {
 		if (ucsi->version > 0x0100)
@@ -101,7 +101,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);
@@ -115,8 +115,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;
 
@@ -170,14 +170,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 6462dadd7540..373dd575c68d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -131,13 +131,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;
 
@@ -156,13 +156,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;
@@ -171,11 +171,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);
 /* -------------------------------------------------------------------------- */
@@ -188,8 +189,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
 	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) {
 		if (con->ucsi->version > 0x0100) {
 			dev_err(con->ucsi->dev,
@@ -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,11 @@ 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);
+		len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
 		if (len <= 0)
 			return len;
 
@@ -487,13 +491,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 +542,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 +579,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 +624,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 +649,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 +665,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 +685,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 +698,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 +744,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 +754,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 +799,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 +849,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 +863,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 +896,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 +1018,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 29f9e7f0d212..4f1409e06d22 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_)		((u64)(_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			0xdbe70000
+
+/* 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)
@@ -443,7 +292,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

Re: [PATCH v3 15/18] usb: typec: ucsi: Remove struct ucsi_control

[Guenter Roeck]

On 10/25/19 1:23 AM, Heikki Krogerus wrote:
> 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>
> Tested-by: Ajay Gupta <ajayg@nvidia.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/usb/typec/ucsi/displayport.c |  16 +-
>   drivers/usb/typec/ucsi/trace.c       |  11 --
>   drivers/usb/typec/ucsi/trace.h       |  50 +-----
>   drivers/usb/typec/ucsi/ucsi.c        | 107 +++++++------
>   drivers/usb/typec/ucsi/ucsi.h        | 231 +++++----------------------
>   5 files changed, 115 insertions(+), 300 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
> index 47424935bc81..d4d5189edfb8 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;
>   	int ret;
>   
> @@ -64,7 +64,7 @@ static int ucsi_displayport_enter(struct typec_altmode *alt)
>   		goto err_unlock;
>   	}
>   
> -	UCSI_CMD_GET_CURRENT_CAM(ctrl, dp->con->num);
> +	command = UCSI_GET_CURRENT_CAM | UCSI_CONNECTOR_NUMBER(dp->con->num);
>   	ret = ucsi_send_command(ucsi, command, &cur, sizeof(cur));
>   	if (ret < 0) {
>   		if (ucsi->version > 0x0100)
> @@ -101,7 +101,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);
> @@ -115,8 +115,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;
>   
> @@ -170,14 +170,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 6462dadd7540..373dd575c68d 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -131,13 +131,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;
>   
> @@ -156,13 +156,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;
> @@ -171,11 +171,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);
>   /* -------------------------------------------------------------------------- */
> @@ -188,8 +189,8 @@ void ucsi_altmode_update_active(struct ucsi_connector *con)
>   	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) {
>   		if (con->ucsi->version > 0x0100) {
>   			dev_err(con->ucsi->dev,
> @@ -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,11 @@ 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);
> +		len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
>   		if (len <= 0)
>   			return len;
>   
> @@ -487,13 +491,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 +542,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 +579,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 +624,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 +649,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 +665,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 +685,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 +698,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 +744,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 +754,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 +799,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 +849,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 +863,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 +896,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 +1018,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 29f9e7f0d212..4f1409e06d22 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_)		((u64)(_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			0xdbe70000
> +
> +/* 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)
> @@ -443,7 +292,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);
> 

[PATCH v3 16/18] usb: typec: ucsi: Remove all bit-fields

[Heikki Krogerus]

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>
Tested-by: Ajay Gupta <ajayg@nvidia.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 373dd575c68d..6c6def96a55b 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -392,7 +392,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;
@@ -410,6 +410,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;
 
@@ -418,7 +419,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;
@@ -429,7 +430,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)) {
@@ -461,7 +462,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;
@@ -491,6 +492,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;
 
@@ -505,11 +507,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))
@@ -517,9 +521,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;
@@ -530,7 +534,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);
@@ -649,6 +653,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;
 
@@ -659,9 +664,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;
 
@@ -685,6 +691,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;
 
@@ -695,7 +702,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);
@@ -712,7 +721,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;
 	}
@@ -767,11 +777,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;
@@ -807,10 +818,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;
@@ -822,8 +830,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 4f1409e06d22..bc2254e7a3a3 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

[PATCH v3 17/18] usb: typec: ucsi: New error codes

[Heikki Krogerus]

Adding new error codes to the driver that were introduced in
UCSI specification v1.1.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 25 ++++++++++++++++++++-----
 drivers/usb/typec/ucsi/ucsi.h |  6 ++++++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 6c6def96a55b..772f55c92ba3 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -86,18 +86,33 @@ static int ucsi_read_error(struct ucsi *ucsi)
 	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);
+		dev_err(ucsi->dev, "possible UCSI driver bug %u\n", error);
 		return -EINVAL;
+	case UCSI_ERROR_OVERCURRENT:
+		dev_warn(ucsi->dev, "Overcurrent condition\n");
+		break;
+	case UCSI_ERROR_PARTNER_REJECTED_SWAP:
+		dev_warn(ucsi->dev, "Partner rejected swap\n");
+		break;
+	case UCSI_ERROR_HARD_RESET:
+		dev_warn(ucsi->dev, "Hard reset occurred\n");
+		break;
+	case UCSI_ERROR_PPM_POLICY_CONFLICT:
+		dev_warn(ucsi->dev, "PPM Policy conflict\n");
+		break;
+	case UCSI_ERROR_SWAP_REJECTED:
+		dev_warn(ucsi->dev, "Swap rejected\n");
+		break;
+	case UCSI_ERROR_UNDEFINED:
 	default:
-		dev_err(ucsi->dev, "%s: error without status\n", __func__);
-		return -EIO;
+		dev_err(ucsi->dev, "unknown error %u\n", error);
+		break;
 	}
 
-	return 0;
+	return -EIO;
 }
 
 static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index bc2254e7a3a3..8569bbd3762f 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -133,6 +133,12 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
 #define UCSI_ERROR_CC_COMMUNICATION_ERR		BIT(4)
 #define UCSI_ERROR_DEAD_BATTERY			BIT(5)
 #define UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL	BIT(6)
+#define UCSI_ERROR_OVERCURRENT			BIT(7)
+#define UCSI_ERROR_UNDEFINED			BIT(8)
+#define UCSI_ERROR_PARTNER_REJECTED_SWAP	BIT(9)
+#define UCSI_ERROR_HARD_RESET			BIT(10)
+#define UCSI_ERROR_PPM_POLICY_CONFLICT		BIT(11)
+#define UCSI_ERROR_SWAP_REJECTED		BIT(12)
 
 /* Data structure filled by PPM in response to GET_CAPABILITY command. */
 struct ucsi_capability {
-- 
2.23.0

[PATCH v3 18/18] usb: typec: ucsi: Optimise ucsi_unregister()

[Heikki Krogerus]

There is no need to reset the PPM when the interface is
unregistered. Quietly silencing the notifications and then
unregistering everything is enough. This speeds up
ucsi_unregister() a lot.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 772f55c92ba3..4459bc68aa33 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1045,15 +1045,14 @@ EXPORT_SYMBOL_GPL(ucsi_register);
  */
 void ucsi_unregister(struct ucsi *ucsi)
 {
-	u64 command;
+	u64 cmd = UCSI_SET_NOTIFICATION_ENABLE;
 	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 */
-	command = UCSI_SET_NOTIFICATION_ENABLE | UCSI_ENABLE_NTFY_CMD_COMPLETE;
-	ucsi_send_command(ucsi, command, NULL, 0);
+	/* Disable notifications */
+	ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
 
 	for (i = 0; i < ucsi->cap.num_connectors; i++) {
 		cancel_work_sync(&ucsi->connector[i].work);
@@ -1063,8 +1062,6 @@ void ucsi_unregister(struct ucsi *ucsi)
 		typec_unregister_port(ucsi->connector[i].port);
 	}
 
-	ucsi_reset_ppm(ucsi);
-
 	kfree(ucsi->connector);
 }
 EXPORT_SYMBOL_GPL(ucsi_unregister);
-- 
2.23.0