RE: [PATCH v4 4/5] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support

[Patel, Utkarsh H]

Hi Andy,

Thank you for the review. 

> 
> ...
> 
> > +	/**
> 
> Are you sure?
> 
> > +	 * Get cable VDO for thunderbolt cables and cables with DPSID but
> does not
> > +	 * support DPAM2.1.
> > +	 */
> 
> ...

Yes, there are TBT3 cables which advertise DPSID but does not provide any DP capabilities in the DP discover mode VDO but does support UHBR.
In that case, need to use TBTSID and use capabilities from TBT discover mode VDO.

> 
> > +	if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
> > +		dp_data.conf |= cable_dp_vdo;
> > +	} else if (cable_tbt_vdo) {
> > +		dp_data.conf |=  TBT_CABLE_SPEED(cable_tbt_vdo) <<
> > +DP_CONF_SIGNALLING_SHIFT;
> > +
> > +		/* Cable Type */
> > +		if (cable_tbt_vdo & TBT_CABLE_OPTICAL)
> > +			dp_data.conf |= DP_CONF_CABLE_TYPE_OPTICAL <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > +		else if (cable_tbt_vdo & TBT_CABLE_RETIMER)
> > +			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_TIMER <<
> DP_CONF_CABLE_TYPE_SHIFT;
> > +		else if (cable_tbt_vdo & TBT_CABLE_ACTIVE_PASSIVE)
> > +			dp_data.conf |= DP_CONF_CABLE_TYPE_RE_DRIVER
> << DP_CONF_CABLE_TYPE_SHIFT;
> > +	} else if (PD_IDH_PTYPE(port->c_identity.id_header) ==
> IDH_PTYPE_PCABLE) {
> > +		dp_data.conf |= VDO_TYPEC_CABLE_SPEED(port-
> >c_identity.vdo[0]) <<
> > +				DP_CONF_SIGNALLING_SHIFT;
> > +	}
> 
> You can also make it a bit more readable with (use better names if you think it's
> needed)
> 
> 	u32 signalling = 0;
> 	u32 cable_type = 0;

In v2 version of this patch I had them but there was feedback to remove extra variables and use them inline. 


Sincerely,
Utkarsh Patel.