Re: [PATCH 2/15] usb: typec: Add parameter for the VDO to typec_altmode_enter()

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

+Badhri

Sun, Jan 25, 2026 at 11:54:22AM +0800, Andy Yan wrote:
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 56fc356bc55c..f3087ef8265c 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -1475,16 +1475,16 @@ static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
> >  	return 0;
> >  }
> >  
> > -static int tcpm_altmode_enter(struct typec_altmode *altmode)
> > +static int tcpm_altmode_enter(struct typec_altmode *altmode, u32 *vdo)
> >  {
> >  	struct tcpm_port *port = typec_altmode_get_drvdata(altmode);
> >  	u32 header;
> >  
> >  	mutex_lock(&port->lock);
> > -	header = VDO(altmode->svid, 1, CMD_ENTER_MODE);
> > +	header = VDO(altmode->svid, vdo ? 2 : 1, CMD_ENTER_MODE);
> >  	header |= VDO_OPOS(altmode->mode);
> 
> Sorry to bother you. I happened to come across this piece of code and
> I'm not quite clear about this part: vdo ? 2 : 1
> 
> According to the definition of the VDO macro(pd_vdo.h), the type here is
> only 1 bit wide, located at bit 15. If the type value is set to 2 here,
> wouldn’t that overwrite bit 16?

That looks like just an obvious bug to me.

thanks,

> /*
>  * VDM header
>  * ----------
>  * <31:16>  :: SVID
>  * <15>     :: VDM type ( 1b == structured, 0b == unstructured )
>  * <14:13>  :: Structured VDM version
>  * <12:11>  :: reserved
>  * <10:8>   :: object position (1-7 valid ... used for enter/exit mode only)
>  * <7:6>    :: command type (SVDM only?)
>  * <5>      :: reserved (SVDM), command type (UVDM)
>  * <4:0>    :: command
>  */
> #define VDO(vid, type, ver, custom)			\
> 	(((vid) << 16) |				\
> 	 ((type) << 15) |				\
> 	 ((ver) << 13) |				\
> 	 ((custom) & 0x7FFF))
> 
> 
> >-	tcpm_queue_vdm(port, header, NULL, 0);
> >+	tcpm_queue_vdm(port, header, vdo, vdo ? 1 : 0);
> > 	mod_delayed_work(port->wq, &port->vdm_state_machine, 0);

-- 
heikki