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