Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Jun,

Where's the 1/2 of this series?

On Fri, Dec 27, 2019 at 10:39:17AM +0000, Jun Li wrote:
> From: Li Jun <jun.li@nxp.com>
> 
> Since the typec port data role is separated from power role,
> so check the port data capability when setting data role.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 56fc356..1f0d82e 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -780,7 +780,7 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached,
>  			  enum typec_role role, enum typec_data_role data)
>  {
>  	enum typec_orientation orientation;
> -	enum usb_role usb_role;
> +	enum usb_role usb_role = USB_ROLE_NONE;
>  	int ret;
>  
>  	if (port->polarity == TYPEC_POLARITY_CC1)
> @@ -788,10 +788,20 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached,
>  	else
>  		orientation = TYPEC_ORIENTATION_REVERSE;
>  
> -	if (data == TYPEC_HOST)
> -		usb_role = USB_ROLE_HOST;
> -	else
> -		usb_role = USB_ROLE_DEVICE;
> +	if (port->typec_caps.data == TYPEC_PORT_DRD) {
> +		if (data == TYPEC_HOST)
> +			usb_role = USB_ROLE_HOST;
> +		else
> +			usb_role = USB_ROLE_DEVICE;
> +	} else if (port->typec_caps.data == TYPEC_PORT_DFP) {
> +		if (data == TYPEC_HOST)
> +			usb_role = USB_ROLE_HOST;
> +		data = TYPEC_HOST;

So if data != host, tcpc is told that data == host, but the mux is set
to USB_ROLE_NONE. So why tcpc needs to think the role is host in that
case?

Shouldn't this function actually return error if the port is DFP only,
and TYPEC_DEVICE is requested?

> +	} else {
> +		if (data == TYPEC_DEVICE)
> +			usb_role = USB_ROLE_DEVICE;
> +		data = TYPEC_DEVICE;
> +	}
>  
>  	ret = tcpm_mux_set(port, TYPEC_STATE_USB, usb_role, orientation);
>  	if (ret < 0)
> @@ -1817,7 +1827,7 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
>  		tcpm_set_state(port, SOFT_RESET, 0);
>  		break;
>  	case PD_CTRL_DR_SWAP:
> -		if (port->port_type != TYPEC_PORT_DRP) {
> +		if (port->typec_caps.data != TYPEC_PORT_DRD) {
>  			tcpm_queue_message(port, PD_MSG_CTRL_REJECT);
>  			break;
>  		}
> @@ -3969,7 +3979,7 @@ static int tcpm_dr_set(struct typec_port *p, enum typec_data_role data)
>  	mutex_lock(&port->swap_lock);
>  	mutex_lock(&port->lock);
>  
> -	if (port->port_type != TYPEC_PORT_DRP) {
> +	if (port->typec_caps.data != TYPEC_PORT_DRD) {
>  		ret = -EINVAL;
>  		goto port_unlock;
>  	}
> -- 
> 2.7.4

thanks,

-- 
heikki