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.
