Re: [PATCH v2 1/2] platform/chrome: cros_ec_typec: Enforce priority-based mode selection

[Tzung-Bi Shih]

On Thu, Oct 08, 2026 at 01:40:55PM +0000, Andrei Kuchynski wrote:
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
...
> +/* Delay between mode entry/exit attempts, ms */
> +static const unsigned int mode_selection_delay = 1000;
> +/* Timeout for a mode entry attempt, ms */
> +static const unsigned int mode_selection_timeout = 4000;

Couldn't they be macros?

> @@ -421,7 +427,6 @@ static int cros_typec_register_port_altmodes(struct cros_typec_data *typec,
>  		memset(&desc, 0, sizeof(desc));
>  		desc.svid = USB_TYPEC_TBT_SID;
>  		desc.mode = TBT_MODE;
> -		desc.inactive = true;

Why this is removed?  Per 1aede0698167 ("platform/chrome: cros_ec_typec:
Disable tbt on port"), it disables entering TBT mode automatically.

> @@ -940,6 +952,31 @@ static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_
>  		goto err_cleanup;
>  	}
>  
> +	/* Once all partner alt-modes are added, we should also trigger
> +	 * mode selection.
> +	 */

For multiple line comments:

    /*
     * Once ...

> +	if (is_partner && typec->ap_driven_altmode) {
> +		bool usb4_mode = (port->caps.usb_capability & USB_CAPABILITY_USB4) &&
> +			(PD_VDO_UFP_DEVCAP(port->p_identity.vdo[0]) & DEV_USB4_CAPABLE);
> +
> +		if (usb4_mode) {
> +			ret = cros_typec_enter_usb_mode(port->port, USB_MODE_USB4);
> +			if (ret < 0) {
> +				usb4_mode = false;
> +				dev_err(typec->dev,
> +					"Unable to activate USB4 mode, port: %d\n", port_num);
> +			}
> +		}
> +		if (!usb4_mode) {
> +			ret = typec_mode_selection_start(port->partner,
> +							 mode_selection_delay,
> +							 mode_selection_timeout);
> +			if (ret < 0)
> +				dev_err(typec->dev,
> +					"Unable to run mode selection, port: %d\n", port_num);

Wouldn't it need to go to err_cleanup and return the ret?  Or, if they
aren't considered as errors, dev_warn().

> diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> index 66c546bf89b53..116329dd113f8 100644
> --- a/drivers/platform/chrome/cros_typec_altmode.c
> +++ b/drivers/platform/chrome/cros_typec_altmode.c
> @@ -41,12 +41,16 @@ static void cros_typec_altmode_work(struct work_struct *work)
>  {
>  	struct cros_typec_altmode_data *data =
>  		container_of(work, struct cros_typec_altmode_data, work);
> +	int ret;
>  
>  	mutex_lock(&data->lock);
>  
> -	if (typec_altmode_vdm(data->alt, data->header, data->vdo_data,
> -			      data->vdo_size))
> +	ret = typec_altmode_vdm(data->alt, data->header, data->vdo_data,
> +				data->vdo_size);
> +	if (ret) {
>  		dev_err(&data->alt->dev, "VDM 0x%x failed\n", data->header);
> +		typec_altmode_state_update(data->port->partner, data->sid, ret);
> +	}

Wouldn't it need to update the state even if !ret?