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.
