linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
@ 2019-12-27 10:39 Jun Li
  2020-01-09 11:52 ` Heikki Krogerus
  0 siblings, 1 reply; 5+ messages in thread
From: Jun Li @ 2019-12-27 10:39 UTC (permalink / raw)
  To: linux, heikki.krogerus; +Cc: gregkh, Jun Li, dl-linux-imx, linux-usb

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;
+	} 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
  2019-12-27 10:39 [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD Jun Li
@ 2020-01-09 11:52 ` Heikki Krogerus
  2020-01-10 10:41   ` Jun Li
  0 siblings, 1 reply; 5+ messages in thread
From: Heikki Krogerus @ 2020-01-09 11:52 UTC (permalink / raw)
  To: Jun Li; +Cc: linux, gregkh, dl-linux-imx, linux-usb

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
  2020-01-09 11:52 ` Heikki Krogerus
@ 2020-01-10 10:41   ` Jun Li
  2020-01-20 13:51     ` Heikki Krogerus
  0 siblings, 1 reply; 5+ messages in thread
From: Jun Li @ 2020-01-10 10:41 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux, gregkh, dl-linux-imx, linux-usb



> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2020年1月9日 19:52
> To: Jun Li <jun.li@nxp.com>
> Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
> 
> Hi Jun,
> 
> Where's the 1/2 of this series?

1/2 patch is for controller driver change, so not TO or CC you in mail list.
Will pay attention this to avoid confuse.

> 
> 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?

enum usb_role {
	USB_ROLE_NONE,
	USB_ROLE_HOST,
	USB_ROLE_DEVICE,
};

enum typec_data_role {
	TYPEC_DEVICE,
	TYPEC_HOST,
};

If the port only support DFP(host), I think we should never tell tcpc the data
is TYPEC_DEVICE, so TYPEC_HOST. 

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

Current TCPM use one API to set both power and data role, doesn't consider
the case of dual power role but single data role. Return error in tcpm_set_roles()
may lose the setting for power role, I think the current change is use correct
data role value when call to tcpm_set_roles(). 
For simple, I didn't change the caller places of tcpm_set_roles(), so just override the
data and usb_role to be reasonable value here.

thanks
Jun
> 
> > +	} 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
  2020-01-10 10:41   ` Jun Li
@ 2020-01-20 13:51     ` Heikki Krogerus
  2020-01-21  4:01       ` Jun Li
  0 siblings, 1 reply; 5+ messages in thread
From: Heikki Krogerus @ 2020-01-20 13:51 UTC (permalink / raw)
  To: Jun Li; +Cc: linux, gregkh, dl-linux-imx, linux-usb

Hi Jun,

On Fri, Jan 10, 2020 at 10:41:31AM +0000, Jun Li wrote:
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: 2020年1月9日 19:52
> > To: Jun Li <jun.li@nxp.com>
> > Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; dl-linux-imx
> > <linux-imx@nxp.com>; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
> > 
> > Hi Jun,
> > 
> > Where's the 1/2 of this series?
> 
> 1/2 patch is for controller driver change, so not TO or CC you in mail list.
> Will pay attention this to avoid confuse.
> 
> > 
> > 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?
> 
> enum usb_role {
> 	USB_ROLE_NONE,
> 	USB_ROLE_HOST,
> 	USB_ROLE_DEVICE,
> };
> 
> enum typec_data_role {
> 	TYPEC_DEVICE,
> 	TYPEC_HOST,
> };
> 
> If the port only support DFP(host), I think we should never tell tcpc the data
> is TYPEC_DEVICE, so TYPEC_HOST. 

But we should also not have to "lie" and force the role into something
that works.

> > Shouldn't this function actually return error if the port is DFP only, and
> > TYPEC_DEVICE is requested?
> 
> Current TCPM use one API to set both power and data role, doesn't consider
> the case of dual power role but single data role. Return error in tcpm_set_roles()
> may lose the setting for power role, I think the current change is use correct
> data role value when call to tcpm_set_roles().
> For simple, I didn't change the caller places of tcpm_set_roles(), so just override the
> data and usb_role to be reasonable value here.

I think the correct thing to do would be to fix the places where the
function is called, and here return error if the unsupported role is
attempted. I hate to be picky, but this looks like a workaround that
may potentially hide real issues in the code.


> > > +	} 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
  2020-01-20 13:51     ` Heikki Krogerus
@ 2020-01-21  4:01       ` Jun Li
  0 siblings, 0 replies; 5+ messages in thread
From: Jun Li @ 2020-01-21  4:01 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux, gregkh, dl-linux-imx, linux-usb

Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2020年1月20日 21:51
> To: Jun Li <jun.li@nxp.com>
> Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD
> 
> Hi Jun,
> 
> On Fri, Jan 10, 2020 at 10:41:31AM +0000, Jun Li wrote:
> > > -----Original Message-----
> > > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > Sent: 2020年1月9日 19:52
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; dl-linux-imx
> > > <linux-imx@nxp.com>; linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH 2/2] usb: typec: tcpm: set correct data role for
> > > non-DRD
> > >
> > > Hi Jun,
> > >
> > > Where's the 1/2 of this series?
> >
> > 1/2 patch is for controller driver change, so not TO or CC you in mail list.
> > Will pay attention this to avoid confuse.
> >
> > >
> > > 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?
> >
> > enum usb_role {
> > 	USB_ROLE_NONE,
> > 	USB_ROLE_HOST,
> > 	USB_ROLE_DEVICE,
> > };
> >
> > enum typec_data_role {
> > 	TYPEC_DEVICE,
> > 	TYPEC_HOST,
> > };
> >
> > If the port only support DFP(host), I think we should never tell tcpc
> > the data is TYPEC_DEVICE, so TYPEC_HOST.
> 
> But we should also not have to "lie" and force the role into something that
> works.
> 
> > > Shouldn't this function actually return error if the port is DFP
> > > only, and TYPEC_DEVICE is requested?
> >
> > Current TCPM use one API to set both power and data role, doesn't
> > consider the case of dual power role but single data role. Return
> > error in tcpm_set_roles() may lose the setting for power role, I think
> > the current change is use correct data role value when call to
> tcpm_set_roles().
> > For simple, I didn't change the caller places of tcpm_set_roles(), so
> > just override the data and usb_role to be reasonable value here.
> 
> I think the correct thing to do would be to fix the places where the function is
> called, and here return error if the unsupported role is attempted. I hate to be
> picky, but this looks like a workaround that may potentially hide real issues in
> the code.

OK, understood, I will send v2.

Thanks
Li Jun
> 
> 
> > > > +	} 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-01-21  4:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-27 10:39 [PATCH 2/2] usb: typec: tcpm: set correct data role for non-DRD Jun Li
2020-01-09 11:52 ` Heikki Krogerus
2020-01-10 10:41   ` Jun Li
2020-01-20 13:51     ` Heikki Krogerus
2020-01-21  4:01       ` Jun Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).