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

[Prashant Malani]

Hi Utkarsh,

On Mon, Aug 21, 2023 at 10:34 AM Patel, Utkarsh H
<utkarsh.h.patel@xxxxxxxxx> wrote:
>
> Hi Prashant,
>
> Thank you for the review and feedback.
>
> > -----Original Message-----
> > From: Prashant Malani <pmalani@xxxxxxxxxxxx>
>>
> > I don't understand why the conf VDO is being recreated here. cable_vdo
> > should already contain the necessary bits. Just use the cable_vdo that you get
> > from cros_typec_get_cable_vdo(); it will have all the bits set correctly already
> > (the EC should be doing that).
> >
> > The "if" condition should also be unnecessary.
> >
> > You are already doing something similar in the other patch for "active retimer
> > cable support" [1]
>
> "if" condition is necessary because there are cables (LRD Gen3) with DPSID but does not support DPAM 2.1 and in that case we need to check TBTSID of the cable and use the cable properties e.g. SPEED and Type.

Ok, then grab the two VDOs distinctly (cable_dp_vdo and cable_tbt_vdo).
Also, the explanation you gave above seems like a good candidate for a comment
above the "if" block.

> Since that information is already available in pd_ctrl, we are leveraging it.  I will remove the part where it's checking retimer cable as DP2.1 is anyway not supported.

As mentioned in earlier patches related to this, we want to avoid
using EC-specific data structures
as much as possible moving forward, especially if the information can
be gleaned from the
available DiscSVID VDOs. So, please use the required cable VDO
(whether it is DP or TBT SID).

>
> >
> > > +
> > > +   port->state.data = &dp21_data;
> > > +
> > > +   return typec_mux_set(port->mux, &port->state);
> >
> > Note that now you have reversed the order in which the muxes are set (which
> > leads to subtle timing issues with Burnside Bridge and other similar retimers).
> > So please don't do this.
>
> Are you suggesting to add same order here?

Specifically, breaking out a separate function for DP 2.1 and then
embedding that
in the overall enable_dp() function makes things unnecessarily complex.

Just craft the VDOs onew time, within enable_dp(), and then use the existing
locations where cros_retimer_set and typec_mux_set() are called.

This will become more clear once you rebase this commit on top of the changes
in [1]

Effectively cros_typec_enable_dp() should handle all DP cases, without needing
a special function broken out for DP 2.1

> > This entire feature isn't necessary. Regardless of whether dp2.1 is supported
> > or not, the port driver just needs to forward the cable_vdo it receives faithfully
> > to the mux driver, which can deal with internal details (based on whether *it*
> > supports DP 2.1 or not).
>
> I am Okay with that.
> We thought we can avoid unnecessary check for the cable_vdo for DP alternate mode on platform where DP2.1 is not supported.

The optimization is miniscule enough to not be worth it the added code
verbosity.