linux-kernel - Re: [PATCH v2 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:   Fri, 8 Sep 2023 17:03:10 +0000
From:   Prashant Malani <pmalani@...omium.org>
To:     "Patel, Utkarsh H" <utkarsh.h.patel@...el.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>,
        "chrome-platform@...ts.linux.dev" <chrome-platform@...ts.linux.dev>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "bleung@...omium.org" <bleung@...omium.org>
Subject: Re: [PATCH v2 4/5] platform/chrome: cros_ec_typec: Add Displayport
 Alternatemode 2.1 Support

Hi Utkarsh,

Just a minor thing you can fix for the next version (since it looks
like there will be one).

On Aug 31 15:24, Patel, Utkarsh H wrote:
> Hello,
> 
> >  drivers/platform/chrome/cros_ec_typec.c | 31
> > +++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index d0b4d3fc40ed..8372f13052a8 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -492,6 +492,8 @@ static int cros_typec_enable_dp(struct
> > cros_typec_data *typec,  {
> >  	struct cros_typec_port *port = typec->ports[port_num];
> >  	struct typec_displayport_data dp_data;
> > +	u32 cable_tbt_vdo;
> > +	u32 cable_dp_vdo;
> >  	int ret;
> > 
> >  	if (typec->pd_ctrl_ver < 2) {
> > @@ -524,6 +526,35 @@ static int cros_typec_enable_dp(struct
> > cros_typec_data *typec,
> >  	port->state.data = &dp_data;
> >  	port->state.mode = TYPEC_MODAL_STATE(ffs(pd_ctrl->dp_mode));
> > 
> > +	/* Get cable VDO for cables with DPSID to check DPAM2.1 is
> > supported */
> > +	cable_dp_vdo = cros_typec_get_cable_vdo(port,
> > USB_TYPEC_DP_SID);
> > +
> > +	/**
> > +	 * Get cable VDO for thunderbolt cables and cables with DPSID but
> > does not
> > +	 * support DPAM2.1.
> > +	 */
> > +	cable_tbt_vdo = cros_typec_get_cable_vdo(port,
> > USB_TYPEC_TBT_SID);
> > +
> > +	if (cable_dp_vdo & DP_CAP_DPAM_VERSION) {
> > +		dp_data.conf |= cable_dp_vdo;
> > +	} else if (cable_tbt_vdo) {
> > +		u8 cable_speed = TBT_CABLE_SPEED(cable_tbt_vdo);
Can we declare this variable at the top? That is the style in this
file and quite commonly seen elsewhere.

Or better yet, just inline this and get rid of the extra variable altogether:

	dp_data.conf |= TBT_CABLE_SPEED(...) << DP_CONF_SIGNALLING_SHIFT;

> > +
> > +		dp_data.conf |= cable_speed <<
> > 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) {
> > +		u8 cable_speed = VDO_CABLE_SPEED(port-
> > >c_identity.vdo[0]);
Same here, you can inline this without affecting readability too much.


BR,

-Prashant