Re: [PATCH] usb: typec: altmodes/displayport: Update active state

[Prashant Malani]

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