| Message ID | 20230830223950.1360865-5-utkarsh.h.patel@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Displayport Alternate Mode 2.1 Support | expand |
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); > + > + 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]); I have wrong macro name here, will correct it in next version. It should be VDO_TYPEC_CABLE_SPEED. Sincerely, Utkarsh Patel.
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
Hi Prashant, Thank you for the review and feedback. > -----Original Message----- > From: Prashant Malani <pmalani@chromium.org> > Sent: Friday, September 8, 2023 10:03 AM > To: Patel, Utkarsh H <utkarsh.h.patel@intel.com> > Cc: linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; > heikki.krogerus@linux.intel.com; chrome-platform@lists.linux.dev; > andriy.shevchenko@linux.intel.com; bleung@chromium.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; Ack. > > > > + > > > + 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. Ack. Sincerely, Utkarsh Patel.
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); + + 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]); + + dp_data.conf |= cable_speed << DP_CONF_SIGNALLING_SHIFT; + } + ret = cros_typec_retimer_set(port->retimer, port->state); if (!ret) ret = typec_mux_set(port->mux, &port->state);
Displayport Alternatemode 2.1 requires cable capabilities such as cable signalling, cable type, DPAM version which then will be used by mux driver for displayport configuration. These capabilities can be derived from the Cable VDO. Signed-off-by: Utkarsh Patel <utkarsh.h.patel@intel.com> --- Changes in v2: - Remvoed feature flag specifice to DP2.1. - Remvoed seperate function for DP2.1. - Removed use of EC host coammnd to get cable details. - TBT cable VDO is used to get cable details. - Using VDO_CABLE_SPEED macro to get passive cable speed. drivers/platform/chrome/cros_ec_typec.c | 31 +++++++++++++++++++++++++ 1 file changed, 31 insertions(+)