[PATCH v1 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[PATCH v1 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[Radu Vele]

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Add the pr_set and dr_set typec_operations to registered typec ports.
This enables sysfs to control power and data role when the port is
capable of doing so.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Co-developed-by: Radu Vele <raduvele@google.com>
Signed-off-by: Radu Vele <raduvele@google.com>
---
 drivers/platform/chrome/cros_ec_typec.c | 77 ++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 7678e3d05fd3..289429ef959f 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -58,8 +58,83 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
 			  &req, sizeof(req), NULL, 0);
 }
 
+static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
+{
+	struct cros_typec_port *port = typec_get_drvdata(tc_port);
+	struct cros_typec_data *data = port->typec_data;
+	struct ec_response_usb_pd_control_v2 resp;
+	struct ec_params_usb_pd_control req;
+	int role, ret;
+
+	/* Must be at least v1 to support role swap. */
+	if (!data->pd_ctrl_ver)
+		return -EOPNOTSUPP;
+
+	/* First query the state */
+	req.port = port->port_num;
+	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
+	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
+	req.swap = USB_PD_CTRL_SWAP_NONE;
+
+	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
+				&req, sizeof(req), &resp, sizeof(resp));
+	if (ret < 0)
+		return ret;
+
+	switch (swap_type) {
+	case USB_PD_CTRL_SWAP_DATA:
+		role = (resp.role & PD_CTRL_RESP_ROLE_DATA) ? TYPEC_HOST :
+						TYPEC_DEVICE;
+		break;
+	case USB_PD_CTRL_SWAP_POWER:
+		role = (resp.role & PD_CTRL_RESP_ROLE_POWER) ? TYPEC_SOURCE :
+						TYPEC_SINK;
+		break;
+	default:
+		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);
+		return -EOPNOTSUPP;
+	}
+
+	if (role == target_role)
+		return 0;
+
+	req.swap = swap_type;
+	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
+				&req, sizeof(req), &resp, sizeof(resp));
+
+	if (ret < 0)
+		return ret;
+
+	switch (swap_type) {
+	case USB_PD_CTRL_SWAP_DATA:
+		typec_set_data_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_DATA ?
+									TYPEC_HOST :
+									TYPEC_DEVICE);
+		break;
+	case USB_PD_CTRL_SWAP_POWER:
+		typec_set_pwr_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_POWER ?
+									TYPEC_SOURCE :
+									TYPEC_SINK);
+		break;
+	}
+
+	return 0;
+}
+
+static int cros_typec_dr_swap(struct typec_port *port, enum typec_data_role role)
+{
+	return cros_typec_perform_role_swap(port, role, USB_PD_CTRL_SWAP_DATA);
+}
+
+static int cros_typec_pr_swap(struct typec_port *port, enum typec_role role)
+{
+	return cros_typec_perform_role_swap(port, role, USB_PD_CTRL_SWAP_POWER);
+}
+
 static const struct typec_operations cros_typec_usb_mode_ops = {
-	.enter_usb_mode = cros_typec_enter_usb_mode
+	.enter_usb_mode = cros_typec_enter_usb_mode,
+	.dr_set = cros_typec_dr_swap,
+	.pr_set = cros_typec_pr_swap
 };
 
 static int cros_typec_parse_port_props(struct typec_capability *cap,
-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v1 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Radu Vele]

Add a lock associated to each port to protect port data against
concurrent access. Concurrency may result from sysfs commands
and ec events.

Signed-off-by: Radu Vele <raduvele@google.com>
---
 drivers/platform/chrome/cros_ec_typec.c | 38 +++++++++++++++++++------
 drivers/platform/chrome/cros_ec_typec.h |  3 ++
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 289429ef959f..bbb9e02624b8 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -8,6 +8,7 @@
 
 #include <linux/acpi.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_usbpd_notify.h>
@@ -54,8 +55,11 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
 		.mode_to_enter = CROS_EC_ALTMODE_USB4
 	};
 
-	return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
+	mutex_lock(&port->lock);
+	int ret = cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
 			  &req, sizeof(req), NULL, 0);
+	mutex_unlock(&port->lock);
+	return ret;
 }
 
 static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
@@ -70,6 +74,8 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
 	if (!data->pd_ctrl_ver)
 		return -EOPNOTSUPP;
 
+	mutex_lock(&port->lock);
+
 	/* First query the state */
 	req.port = port->port_num;
 	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
@@ -79,7 +85,7 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
 	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
 				&req, sizeof(req), &resp, sizeof(resp));
 	if (ret < 0)
-		return ret;
+		goto unlock;
 
 	switch (swap_type) {
 	case USB_PD_CTRL_SWAP_DATA:
@@ -92,18 +98,21 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
 		break;
 	default:
 		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		goto unlock;
 	}
 
-	if (role == target_role)
-		return 0;
+	if (role == target_role) {
+		ret = 0;
+		goto unlock;
+	}
 
 	req.swap = swap_type;
 	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
 				&req, sizeof(req), &resp, sizeof(resp));
 
 	if (ret < 0)
-		return ret;
+		goto unlock;
 
 	switch (swap_type) {
 	case USB_PD_CTRL_SWAP_DATA:
@@ -117,8 +126,11 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
 									TYPEC_SINK);
 		break;
 	}
+	ret = 0;
 
-	return 0;
+unlock:
+	mutex_unlock(&port->lock);
+	return ret;
 }
 
 static int cros_typec_dr_swap(struct typec_port *port, enum typec_data_role role)
@@ -370,6 +382,7 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
 		if (!typec->ports[i])
 			continue;
 
+		mutex_lock(&typec->ports[i]->lock);
 		cros_typec_remove_partner(typec, i);
 		cros_typec_remove_cable(typec, i);
 
@@ -378,6 +391,8 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
 		typec_mux_put(typec->ports[i]->mux);
 		cros_typec_unregister_port_altmodes(typec->ports[i]);
 		typec_unregister_port(typec->ports[i]->port);
+		mutex_unlock(&typec->ports[i]->lock);
+		mutex_destroy(&typec->ports[i]->lock);
 	}
 }
 
@@ -472,6 +487,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
 			goto unregister_ports;
 		}
 
+		mutex_init(&cros_port->lock);
 		cros_port->port_num = port_num;
 		cros_port->typec_data = typec;
 		typec->ports[port_num] = cros_port;
@@ -1232,6 +1248,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 		return -EINVAL;
 	}
 
+	mutex_lock(&typec->ports[port_num]->lock);
 	req.port = port_num;
 	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
 	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
@@ -1241,7 +1258,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 			  EC_CMD_USB_PD_CONTROL, &req, sizeof(req),
 			  &resp, sizeof(resp));
 	if (ret < 0)
-		return ret;
+		goto unlock;
 
 	/* Update the switches if they exist, according to requested state */
 	ret = cros_typec_configure_mux(typec, port_num, &resp);
@@ -1263,7 +1280,10 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
 	if (typec->typec_cmd_supported)
 		cros_typec_handle_status(typec, port_num);
 
-	return 0;
+	ret = 0;
+unlock:
+	mutex_unlock(&typec->ports[port_num]->lock);
+	return ret;
 }
 
 static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
index f9c31f04c102..d0a8a12ec91a 100644
--- a/drivers/platform/chrome/cros_ec_typec.h
+++ b/drivers/platform/chrome/cros_ec_typec.h
@@ -82,6 +82,9 @@ struct cros_typec_port {
 	struct usb_power_delivery_capabilities *partner_sink_caps;
 
 	struct cros_typec_data *typec_data;
+
+	/* Mutex to protect port data against concurrent access */
+	struct mutex lock;
 };
 
 #endif /*  __CROS_EC_TYPEC__ */
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v1 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[Benson Leung]

Hi Radu and Abhishek,

Thanks for this change!

On Wed, Jul 09, 2025 at 01:22:31PM +0000, Radu Vele wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> Add the pr_set and dr_set typec_operations to registered typec ports.
> This enables sysfs to control power and data role when the port is
> capable of doing so.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Co-developed-by: Radu Vele <raduvele@google.com>
> Signed-off-by: Radu Vele <raduvele@google.com>

Reviewed-by: Benson Leung <bleung@chromium.org>


> ---
>  drivers/platform/chrome/cros_ec_typec.c | 77 ++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 7678e3d05fd3..289429ef959f 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -58,8 +58,83 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
>  			  &req, sizeof(req), NULL, 0);
>  }
>  
> +static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
> +{
> +	struct cros_typec_port *port = typec_get_drvdata(tc_port);
> +	struct cros_typec_data *data = port->typec_data;
> +	struct ec_response_usb_pd_control_v2 resp;
> +	struct ec_params_usb_pd_control req;
> +	int role, ret;
> +
> +	/* Must be at least v1 to support role swap. */
> +	if (!data->pd_ctrl_ver)
> +		return -EOPNOTSUPP;
> +
> +	/* First query the state */
> +	req.port = port->port_num;
> +	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
> +	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
> +	req.swap = USB_PD_CTRL_SWAP_NONE;
> +
> +	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
> +				&req, sizeof(req), &resp, sizeof(resp));
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (swap_type) {
> +	case USB_PD_CTRL_SWAP_DATA:
> +		role = (resp.role & PD_CTRL_RESP_ROLE_DATA) ? TYPEC_HOST :
> +						TYPEC_DEVICE;
> +		break;
> +	case USB_PD_CTRL_SWAP_POWER:
> +		role = (resp.role & PD_CTRL_RESP_ROLE_POWER) ? TYPEC_SOURCE :
> +						TYPEC_SINK;
> +		break;
> +	default:
> +		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (role == target_role)
> +		return 0;
> +
> +	req.swap = swap_type;
> +	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
> +				&req, sizeof(req), &resp, sizeof(resp));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (swap_type) {
> +	case USB_PD_CTRL_SWAP_DATA:
> +		typec_set_data_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_DATA ?
> +									TYPEC_HOST :
> +									TYPEC_DEVICE);
> +		break;
> +	case USB_PD_CTRL_SWAP_POWER:
> +		typec_set_pwr_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_POWER ?
> +									TYPEC_SOURCE :
> +									TYPEC_SINK);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_typec_dr_swap(struct typec_port *port, enum typec_data_role role)
> +{
> +	return cros_typec_perform_role_swap(port, role, USB_PD_CTRL_SWAP_DATA);
> +}
> +
> +static int cros_typec_pr_swap(struct typec_port *port, enum typec_role role)
> +{
> +	return cros_typec_perform_role_swap(port, role, USB_PD_CTRL_SWAP_POWER);
> +}
> +
>  static const struct typec_operations cros_typec_usb_mode_ops = {
> -	.enter_usb_mode = cros_typec_enter_usb_mode
> +	.enter_usb_mode = cros_typec_enter_usb_mode,
> +	.dr_set = cros_typec_dr_swap,
> +	.pr_set = cros_typec_pr_swap
>  };
>  
>  static int cros_typec_parse_port_props(struct typec_capability *cap,
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v1 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Benson Leung]

Hi Radu,


On Wed, Jul 09, 2025 at 01:22:32PM +0000, Radu Vele wrote:
> Add a lock associated to each port to protect port data against
> concurrent access. Concurrency may result from sysfs commands
> and ec events.
> 
> Signed-off-by: Radu Vele <raduvele@google.com>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 38 +++++++++++++++++++------
>  drivers/platform/chrome/cros_ec_typec.h |  3 ++
>  2 files changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 289429ef959f..bbb9e02624b8 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_usbpd_notify.h>
> @@ -54,8 +55,11 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
>  		.mode_to_enter = CROS_EC_ALTMODE_USB4
>  	};
>  
> -	return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> +	mutex_lock(&port->lock);
> +	int ret = cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
>  			  &req, sizeof(req), NULL, 0);

Please declare "int ret" at the top of the block, as per other examples of it
in this file.

You have it in the middle of a block right now.

> +	mutex_unlock(&port->lock);
> +	return ret;
>  }
>  
>  static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
> @@ -70,6 +74,8 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  	if (!data->pd_ctrl_ver)
>  		return -EOPNOTSUPP;
>  
> +	mutex_lock(&port->lock);
> +
>  	/* First query the state */
>  	req.port = port->port_num;
>  	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
> @@ -79,7 +85,7 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
>  				&req, sizeof(req), &resp, sizeof(resp));
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>  
>  	switch (swap_type) {
>  	case USB_PD_CTRL_SWAP_DATA:
> @@ -92,18 +98,21 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  		break;
>  	default:
>  		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		goto unlock;
>  	}
>  
> -	if (role == target_role)
> -		return 0;
> +	if (role == target_role) {
> +		ret = 0;
> +		goto unlock;
> +	}
>  
>  	req.swap = swap_type;
>  	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
>  				&req, sizeof(req), &resp, sizeof(resp));
>  
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>  
>  	switch (swap_type) {
>  	case USB_PD_CTRL_SWAP_DATA:
> @@ -117,8 +126,11 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  									TYPEC_SINK);
>  		break;
>  	}
> +	ret = 0;
>  
> -	return 0;
> +unlock:
> +	mutex_unlock(&port->lock);
> +	return ret;
>  }
>  
>  static int cros_typec_dr_swap(struct typec_port *port, enum typec_data_role role)
> @@ -370,6 +382,7 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  		if (!typec->ports[i])
>  			continue;
>  
> +		mutex_lock(&typec->ports[i]->lock);
>  		cros_typec_remove_partner(typec, i);
>  		cros_typec_remove_cable(typec, i);
>  
> @@ -378,6 +391,8 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  		typec_mux_put(typec->ports[i]->mux);
>  		cros_typec_unregister_port_altmodes(typec->ports[i]);
>  		typec_unregister_port(typec->ports[i]->port);
> +		mutex_unlock(&typec->ports[i]->lock);
> +		mutex_destroy(&typec->ports[i]->lock);
>  	}
>  }
>  
> @@ -472,6 +487,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  			goto unregister_ports;
>  		}
>  
> +		mutex_init(&cros_port->lock);
>  		cros_port->port_num = port_num;
>  		cros_port->typec_data = typec;
>  		typec->ports[port_num] = cros_port;
> @@ -1232,6 +1248,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&typec->ports[port_num]->lock);
>  	req.port = port_num;
>  	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
>  	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
> @@ -1241,7 +1258,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  			  EC_CMD_USB_PD_CONTROL, &req, sizeof(req),
>  			  &resp, sizeof(resp));
>  	if (ret < 0)
> -		return ret;
> +		goto unlock;
>  
>  	/* Update the switches if they exist, according to requested state */
>  	ret = cros_typec_configure_mux(typec, port_num, &resp);
> @@ -1263,7 +1280,10 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  	if (typec->typec_cmd_supported)
>  		cros_typec_handle_status(typec, port_num);
>  
> -	return 0;
> +	ret = 0;
> +unlock:
> +	mutex_unlock(&typec->ports[port_num]->lock);
> +	return ret;
>  }
>  
>  static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
> diff --git a/drivers/platform/chrome/cros_ec_typec.h b/drivers/platform/chrome/cros_ec_typec.h
> index f9c31f04c102..d0a8a12ec91a 100644
> --- a/drivers/platform/chrome/cros_ec_typec.h
> +++ b/drivers/platform/chrome/cros_ec_typec.h
> @@ -82,6 +82,9 @@ struct cros_typec_port {
>  	struct usb_power_delivery_capabilities *partner_sink_caps;
>  
>  	struct cros_typec_data *typec_data;
> +
> +	/* Mutex to protect port data against concurrent access */
> +	struct mutex lock;
>  };
>  
>  #endif /*  __CROS_EC_TYPEC__ */
> -- 
> 2.50.0.727.gbf7dc18ff4-goog
> 

Re: [PATCH v1 1/2] platform/chrome: cros_ec_typec: Add role swap ops

[Tzung-Bi Shih]

On Wed, Jul 09, 2025 at 01:22:31PM +0000, Radu Vele wrote:
> +static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
> +{
> [...]
> +	switch (swap_type) {
> +	case USB_PD_CTRL_SWAP_DATA:
> +		role = (resp.role & PD_CTRL_RESP_ROLE_DATA) ? TYPEC_HOST :
> +						TYPEC_DEVICE;
> +		break;
> +	case USB_PD_CTRL_SWAP_POWER:
> +		role = (resp.role & PD_CTRL_RESP_ROLE_POWER) ? TYPEC_SOURCE :
> +						TYPEC_SINK;
> +		break;
> +	default:
> +		dev_warn(data->dev, "Unsupported role swap type %d", swap_type);

Append a newline at the end of message?

> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (role == target_role)
> +		return 0;
> +
> +	req.swap = swap_type;
> +	ret = cros_ec_cmd(data->ec, data->pd_ctrl_ver, EC_CMD_USB_PD_CONTROL,
> +				&req, sizeof(req), &resp, sizeof(resp));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (swap_type) {
> +	case USB_PD_CTRL_SWAP_DATA:
> +		typec_set_data_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_DATA ?
> +									TYPEC_HOST :
> +									TYPEC_DEVICE);

To improve the readability a bit, how about re-use `role` variable here:

        role = resp.role & ... ? ... : ...;

Also I'm wondering doesn't it need to assert
(resp.role & ... ? ... : ...) == target_role here?  Otherwise, could it just
use `target_role` as the parameter for typec_set_data_role()?


> +		break;
> +	case USB_PD_CTRL_SWAP_POWER:
> +		typec_set_pwr_role(tc_port, resp.role & PD_CTRL_RESP_ROLE_POWER ?
> +									TYPEC_SOURCE :
> +									TYPEC_SINK);

Similar here.

> +		break;
> +	}

It already checks in previous code block.  Not sure if some checkers would
find the swtich-case block doesn't have a default clause.  How about still
add a default case, and have a comment to remind that should never execute?

>  static const struct typec_operations cros_typec_usb_mode_ops = {
> -	.enter_usb_mode = cros_typec_enter_usb_mode
> +	.enter_usb_mode = cros_typec_enter_usb_mode,
> +	.dr_set = cros_typec_dr_swap,
> +	.pr_set = cros_typec_pr_swap

Leave a comma "," at the end of line?

Re: [PATCH v1 2/2] platform/chrome: cros_ec_typec: Add lock per-port

[Tzung-Bi Shih]

On Wed, Jul 09, 2025 at 01:22:32PM +0000, Radu Vele wrote:
> @@ -8,6 +8,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_usbpd_notify.h>

Please consider to include linux/cleanup.h and use guard() to simplify the
patch.

> @@ -54,8 +55,11 @@ static int cros_typec_enter_usb_mode(struct typec_port *tc_port, enum usb_mode m
>  		.mode_to_enter = CROS_EC_ALTMODE_USB4
>  	};
>  
> -	return cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
> +	mutex_lock(&port->lock);
> +	int ret = cros_ec_cmd(port->typec_data->ec, 0, EC_CMD_TYPEC_CONTROL,
>  			  &req, sizeof(req), NULL, 0);
> +	mutex_unlock(&port->lock);
> +	return ret;

E.g.:
        guard(mutex)(&port->lock);
        return cros_ec_cmd(...);

>  static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_role, u8 swap_type)
> @@ -70,6 +74,8 @@ static int cros_typec_perform_role_swap(struct typec_port *tc_port, int target_r
>  	if (!data->pd_ctrl_ver)
>  		return -EOPNOTSUPP;
>  
> +	mutex_lock(&port->lock);

It can drop all the following diff hunks in the function if using:
        guard(mutex)(&port->lock);

> @@ -1232,6 +1248,7 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&typec->ports[port_num]->lock);

It can drop all the following diff hunks in the function if using:
        guard(mutex)(&typec->ports[port_num]->lock);