> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
> Sent: Tuesday, September 26, 2017 9:20 PM
> To: Jun Li <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Peter Chen
> <[email protected]>; A.s. Dong <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 09/12] usb: typec: tcpm: only drives the connected cc line
> when attached
>
> On 09/26/2017 02:53 AM, Jun Li wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> >> Roeck
> >> Sent: Tuesday, September 26, 2017 3:17 PM
> >> To: Jun Li <[email protected]>; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]
> >> Cc: [email protected]; [email protected]; Peter Chen
> >> <[email protected]>; A.s. Dong <[email protected]>; linux-
> >> [email protected]; [email protected]
> >> Subject: Re: [PATCH 09/12] usb: typec: tcpm: only drives the
> >> connected cc line when attached
> >>
> >> On 09/25/2017 05:45 PM, Li Jun wrote:
> >>> As we should only drive connected cc line to be the right state when
> >>> attached, and keeps the other one to be open, so update the set_cc
> >>> interface for that.
> >>>
> >>> Signed-off-by: Li Jun <[email protected]>
> >>> ---
> >>> drivers/usb/typec/tcpm.c | 12 +++++++++++-
> >>> include/linux/usb/tcpm.h | 3 ++-
> >>> 2 files changed, 13 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> >>> index 38a6223..c9966ee 100644
> >>> --- a/drivers/usb/typec/tcpm.c
> >>> +++ b/drivers/usb/typec/tcpm.c
> >>> @@ -1857,9 +1857,15 @@ static bool tcpm_start_drp_toggling(struct
> >>> tcpm_port *port,
> >>>
> >>> static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status
> >>> cc)
> >>> {
> >>> + bool attached;
> >>> +
> >>> tcpm_log(port, "cc:=%d", cc);
> >>> port->cc_req = cc;
> >>> - port->tcpc->set_cc(port->tcpc, cc);
> >>> + if (port->state == SRC_ATTACHED || port->state == SNK_ATTACHED)
> >>> + attached = true;
> >>> + else
> >>> + attached = false;
> >>> + port->tcpc->set_cc(port->tcpc, cc, attached, port->polarity);
> >>
> >> Polarity should be set with set_polarity(). Is it necessary to duplicate
> >> that ?
> >
> > In tcpci case, set_polarity only sets Plug Orientation bit:
> > " 0b: When Vconn is enabled, apply it to the CC2 pin. Monitor the CC1
> > pin for BMC communications if PD messaging is enabled.
> > 1b: When Vconn is enabled, apply it to the CC1 pin. Monitor the CC2
> > pin for BMC communications if PD messaging is enabled."
> >
> Yes, but the driver should know about the flag already.
>
> I have two more concerns:
>
> Setting CC is logically independent from the state machine "active" state.
> I don't recall that the USB PD state machine talks about CC pin changes upon
> entering and exiting READY states. Why is it necessary to pass the state to
> the
> driver ?
Here the CC pin changes only for un-touched one between look for connection
and attached, so when attach(before entering READY), the un-touch cc pin
should be changed from Pd/Rp to open(e.g. see typec spec Table 4-6 Source
Perspective). as my question below, I can adding it into tcpci_set_polarity()
for tcpci case if it's reasonable, with that tcpm API don't need change.
>
> Also, your patch changes the API, but you don't change the driver, meaning
> there should be compile failures after this patch. You need to change the
> calling
> code in the same patch to keep the build clean.
Sorry, I didn’t realize there is already a user of tcpm, I will update the
calling
driver accordingly for hanged tcpm API in next version.
Li Jun
>
> Guenter
>
> > There is no duplication for tcpci, or you think I should do this in
> > set_polarity of tcpci driver internally? looks better from my point
> > view as I may don't need change tcpm set_cc interface.
> >
> > thanks
> > Li Jun
> >>
> >>> }
> >>>
> >>> static int tcpm_init_vbus(struct tcpm_port *port) @@ -1913,6
> >>> +1919,8 @@ static int tcpm_src_attach(struct tcpm_port *port)
> >>> if (ret < 0)
> >>> return ret;
> >>>
> >>> + tcpm_set_cc(port, tcpm_rp_cc(port));
> >>> +
> >>> ret = tcpm_set_roles(port, true, TYPEC_SOURCE, TYPEC_HOST);
> >>> if (ret < 0)
> >>> return ret;
> >>> @@ -2028,6 +2036,8 @@ static int tcpm_snk_attach(struct tcpm_port
> *port)
> >>> if (ret < 0)
> >>> return ret;
> >>>
> >>> + tcpm_set_cc(port, TYPEC_CC_RD);
> >>> +
> >>> ret = tcpm_set_roles(port, true, TYPEC_SINK, TYPEC_DEVICE);
> >>> if (ret < 0)
> >>> return ret;
> >>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> >>> index
> >>> a67cf77..a007e2a1 100644
> >>> --- a/include/linux/usb/tcpm.h
> >>> +++ b/include/linux/usb/tcpm.h
> >>> @@ -159,7 +159,8 @@ struct tcpc_dev {
> >>> int (*init)(struct tcpc_dev *dev);
> >>> int (*get_vbus)(struct tcpc_dev *dev);
> >>> int (*get_current_limit)(struct tcpc_dev *dev);
> >>> - int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc);
> >>> + int (*set_cc)(struct tcpc_dev *dev, enum typec_cc_status cc,
> >>> + bool attached, enum typec_cc_polarity polarity);
> >>> int (*get_cc)(struct tcpc_dev *dev, enum typec_cc_status *cc1,
> >>> enum typec_cc_status *cc2);
> >>> int (*set_polarity)(struct tcpc_dev *dev,
> >>>
> >
