Linux-USB Archive on lore.kernel.org
 help / color / 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; 3+ 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	[flat|nested] 3+ 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; 3+ 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] 3+ 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
  0 siblings, 0 replies; 3+ 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] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ 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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git