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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Heikki,

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Sent: 2020年1月20日 21:51
> To: Jun Li <jun.li@xxxxxxx>
> Cc: linux@xxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx>
> > > Sent: 2020年1月9日 19:52
> > > To: Jun Li <jun.li@xxxxxxx>
> > > Cc: linux@xxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; dl-linux-imx
> > > <linux-imx@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> > > 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@xxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > ---
> > > >  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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux