Re: [PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm

[Peter Chen (CIX)]

On 26-03-19 17:57:15, Xu Yang wrote:
> In current design, we expect 2 ci_irq() to handle ID and VBUS events in
> usb role switch, like what ci_extcon_wakeup_int() does. Now we only call
> ci_irq() once. However, this won't bring any issues in low power mode,
> because ci_irq() just take the device out of low power mode, and then
> ci_extcon_wakeup_int() will call ci_irq() twice. If the device is not in
> suspend state, the device mode will not work properly because VBUS event
> won'tbe handled (ID event has higher priority) at all.

%s/won'tbe/won't be

Is it possible change ci_irq_handler and handle both events?

> 
> Although 2 consecutive ci_irq() can work around the issue, do not do it
> because ci_usb_role_switch_set() may or not be in low power context which
> make the ci_irq() purpose not unique here. Because the final processing
> is in ci_otg_work(), just directly queue an otg work. This also refine
> the logic for more clarity and not set changed flag.
> 
> Fixes: e1b5d2bed67c ("usb: chipidea: core: handle usb role switch in a common way")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Jun Li <jun.li@xxxxxxx>
> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> ---
>  drivers/usb/chipidea/core.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index fac11f20cf0a..1bd231a852a1 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -618,30 +618,22 @@ static int ci_usb_role_switch_set(struct usb_role_switch *sw,
>  	struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
>  	struct ci_hdrc_cable *cable;
>  
> -	if (role == USB_ROLE_HOST) {
> -		cable = &ci->platdata->id_extcon;
> -		cable->changed = true;
> +	cable = &ci->platdata->id_extcon;
> +	if (role == USB_ROLE_HOST)
>  		cable->connected = true;
> -		cable = &ci->platdata->vbus_extcon;
> -		cable->changed = true;
> -		cable->connected = false;
> -	} else if (role == USB_ROLE_DEVICE) {
> -		cable = &ci->platdata->id_extcon;
> -		cable->changed = true;
> +	else
>  		cable->connected = false;
> -		cable = &ci->platdata->vbus_extcon;
> -		cable->changed = true;
> +
> +	cable = &ci->platdata->vbus_extcon;
> +	if (role == USB_ROLE_DEVICE)
>  		cable->connected = true;
> -	} else {
> -		cable = &ci->platdata->id_extcon;
> -		cable->changed = true;
> -		cable->connected = false;
> -		cable = &ci->platdata->vbus_extcon;
> -		cable->changed = true;
> +	else
>  		cable->connected = false;
> -	}
>  
> -	ci_irq(ci);
> +	ci->id_event = true;
> +	ci->b_sess_valid_event = true;

Why both ID and VBUS event are set as true unconditionally?

-- 

Best regards,
Peter