On Thu, Jan 19, 2023 at 1:26 AM Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> wrote: > > Hi Prashant, > > On Wed, Jan 18, 2023 at 10:26:21AM -0800, Prashant Malani wrote: > > Hi Heikki, > > > > Thanks for reviewing the patch. > > > > On Wed, Jan 18, 2023 at 1:39 AM Heikki Krogerus > > <heikki.krogerus@xxxxxxxxxxxxxxx> wrote: > > > > > > On Wed, Jan 18, 2023 at 03:15:15AM +0000, Prashant Malani wrote: > > FWIW, I think we can make the typec_altmode_update_active() calls from > > our (cros-ec-typec) port driver too, but displayport.c is parsing the header > > anyway, so it seemed repetitive. Just wanted to clarify the intention here. > > The alt modes may have been entered even if there are no drivers for > them, if for example the PD controller handles the mode entry. In > those cases the port driver needs to update the active state of the > partner alt mode. Ack. Thanks for explaining the rationale here. > > Since the port drivers have to handle that in some cases, for the sake > of consistency I thought that they might as well take care of it in > every case. > > On the other hand, it should be safe to do it in both the port driver > and the altmode driver. > > If you prefer that the altmode drivers always do this, I'm not against > it. But in that case could you patch tcpm.c while at it - in the same > series: Sure, I will send out a v2 with the below diff as Patch 2/2 (I will mark you as "Suggested-by" but as always LMK if you prefer another way to denote attribution). > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 904c7b4ce2f0c..0f5a9d4db105a 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1693,14 +1693,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev, > } > break; > case CMD_ENTER_MODE: > - if (adev && pdev) { > - typec_altmode_update_active(pdev, true); > + if (adev && pdev) > *adev_action = ADEV_QUEUE_VDM_SEND_EXIT_MODE_ON_FAIL; > - } > return 0; > case CMD_EXIT_MODE: > if (adev && pdev) { > - typec_altmode_update_active(pdev, false); > /* Back to USB Operation */ > *adev_action = ADEV_NOTIFY_USB_AND_QUEUE_VDM; > return 0; > > That's the only driver that will definitely always requires the > altmode drivers, so perhaps it would be good to drop the calls > from it at the same time. > > thanks, > > -- > heikki