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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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?