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

[Patel, Utkarsh H]

Hi Prashant,

> > > 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).

Ack.

> 
> >
> > >
> > > > +
> > > > +   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

Ack.

> 
> > > 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.

Ack. I will remove it.

Sincerely,
Utkarsh Patel.