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

[Andrei Kuchynski <akuchynski@chromium.org>]

On Thu, Oct 15, 2026 at 2:15 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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?

It is implemented as variables in cros_ec_ucsi.c, but they could be macros.
I will change this in v3.

>
> > @@ -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.

If the ap_driven_altmode parameter is enabled, automatic activation of
both TBT and DP will be disabled via the mode_selection field. Consequently,
deactivating TBT via the inactive field is no longer necessary.
Alternative mode drivers within cros_ec_typec are no longer needed to
trigger activation.

>
> > @@ -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 ...
>

Thanks, I will fix the comment formatting.

> > +     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().

dev_warn() then. We definitely don't need an error path here.

>
> > 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?

No. The typec_altmode_state_update() reports whether the activation of
altmode has been completed or has failed.
If successful, typec_altmode_state_update() will be called from within
cros_typec_configure_mux().

Thanks,
Andrei