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
