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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Sep 2023 15:54:40 +0000
From:   "Patel, Utkarsh H" <utkarsh.h.patel@...el.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
        "pmalani@...omium.org" <pmalani@...omium.org>,
        "chrome-platform@...ts.linux.dev" <chrome-platform@...ts.linux.dev>,
        "bleung@...omium.org" <bleung@...omium.org>
Subject: RE: [PATCH v4 4/5] platform/chrome: cros_ec_typec: Add Displayport
 Alternatemode 2.1 Support

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.