RE: [PATCH v3 3/4] platform/chrome: cros_ec_typec: Add Displayport Alternatemode 2.1 Support

[Patel, Utkarsh H]

Hi Prashant,

Thank you for the review.

> -----Original Message-----
> From: Prashant Malani <pmalani@xxxxxxxxxxxx>
> Sent: Monday, September 11, 2023 6:18 PM
> To: Patel, Utkarsh H <utkarsh.h.patel@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> heikki.krogerus@xxxxxxxxxxxxxxx; chrome-platform@xxxxxxxxxxxxxxx;
> andriy.shevchenko@xxxxxxxxxxxxxxx; bleung@xxxxxxxxxxxx
> Subject: Re: [PATCH v3 3/4] platform/chrome: cros_ec_typec: Add Displayport
> Alternatemode 2.1 Support
> 
> Hi Utkarsh,
> 
> On Mon, Sep 11, 2023 at 5:58 PM Utkarsh Patel <utkarsh.h.patel@xxxxxxxxx>
> wrote:
> >
> > 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.
> > Added a helper macro to get the Type C cable speed when provided the
> > cable VDO.
> >
> > Signed-off-by: Utkarsh Patel <utkarsh.h.patel@xxxxxxxxx>
> 
> Thank you for addressing the comments. This LGTM; I have one minor
> suggestion, but I'll leave it to USB maintainers for the final call on that
> comment, so:
> 
> Acked-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> 
> > ---
> > Changes in v3:
> > - Removed use of variable cable_seepd.
> > - Added helper macro of pd_vdo.h in this patch as cros_ec_typec is the user.
> >
> > 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 | 28
> +++++++++++++++++++++++++
> >  include/linux/usb/pd_vdo.h              |  1 +
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index d0b4d3fc40ed..cca913400b39 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,32 @@ 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) {
> > +               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;
> > +       }
> > +
> >         ret = cros_typec_retimer_set(port->retimer, port->state);
> >         if (!ret)
> >                 ret = typec_mux_set(port->mux, &port->state); diff
> > --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h index
> > b057250704e8..3a747938cdab 100644
> > --- a/include/linux/usb/pd_vdo.h
> > +++ b/include/linux/usb/pd_vdo.h
> > @@ -376,6 +376,7 @@
> >          | ((vbm) & 0x3) << 9 | (sbu) << 8 | (sbut) << 7 | ((cur) & 0x3) << 5   \
> >          | (vbt) << 4 | (sopp) << 3 | ((spd) & 0x7))
> >
> > +#define VDO_TYPEC_CABLE_SPEED(vdo)     ((vdo) & 0x7)
> 
> I would suggest putting this header modification in a separate patch; if for
> some reason we have to revert the Chrome part of the change, then we won't
> rip this part out too (some other driver down the road may use the macro and
> would break if it were to be removed). But I'll leave it to Heikki to determine
> whether that is preferred.
> 
Heikki,  What's your preference here?

Sincerely,
Utkarsh Patel.