On Tue, Jan 11, 2022 at 04:43:42AM +0000, Xu Yang wrote:
> Hi,
>
> >
> > I'm sorry, but I did not understand the subject line?
> >
> > On Thu, Jan 06, 2022 at 04:53:25PM +0800, Xu Yang wrote:
> > > With the AMS and Collision Avoidance, tcpm often needs to change the
> > CC's
> > > termination. When one CC line is souring Vconn, if we still change its
> > > termination, the voltage of the another CC line is likely to be fluctuant
> > > and unstable.
> > >
> > > Therefore, we should verify whether a CC line is soucing Vconn before
> > > changing its termination. And only changing the termination that is
> > > not a Vconn line. This can be done by reading the VCONN Present bit of
> > > POWER_ STATUS register. To determinate the polarity, we can read the
> > > Plug Orientation bit of TCPC_CONTROL register. Since only if Plug
> > > Orientation is set, Vconn can be sourced.
> > >
> > > Fixes: 0908c5aca31e ("usb: typec: tcpm: AMS and Collision Avoidance")
> > > cc: <stable@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > Okay, the commit message does explain what's this about, but could you
> > still change the subject to "..don't touch the CC line if it's VCONN
> > source" or something like that?
>
> Sorry for the puzzling title, I will change it in the next formal patch.
>
> >
> > > ---
> > > drivers/usb/typec/tcpm/tcpci.c | 27 +++++++++++++++++++++++++++
> > > drivers/usb/typec/tcpm/tcpci.h | 1 +
> > > 2 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpci.c
> > b/drivers/usb/typec/tcpm/tcpci.c
> > > index 35a1307349a2..0bf4cbfaa21c 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > > @@ -75,9 +75,26 @@ static int tcpci_write16(struct tcpci *tcpci, unsigned
> > int reg, u16 val)
> > > static int tcpci_set_cc(struct tcpc_dev *tcpc, enum typec_cc_status cc)
> > > {
> > > struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > > + bool vconn_pres = false;
> > > + enum typec_cc_polarity polarity = TYPEC_POLARITY_CC1;
> > > unsigned int reg;
> > > int ret;
> > >
> > > + ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS, ®);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + if (reg & TCPC_POWER_STATUS_VCONN_PRES) {
> >
> > Isn't that bit optional? tcpm.c already knows the vconn status, right?
> > If it does, then maybe it would be safer to change the API so that you
> > pass also the information about the vconn status to the .set_cc
> > callback? So in this case:
> >
> > static int tcpci_set_cc(struct tpcp_dev *tcpc, enum typec_cc_status cc, enum
> > typec_role vconn)
> >
> > That way the support would also be for all the other drivers too, so
> > not just for tcpci.c.
>
> Yeah, it's better to change the API in the tcpm.c. But from to my observation,
> other drivers already have their own implementation to set CC's termination.
>
> For fusb302:
> It use chip->cc_polarity to choose which CC line to be changed.
>
> For wcove:
> It only changes one CC's termination at one time.
>
> So, there is no such a problem for them with the AMS and Collision Avoidance.
> In tcpci, this problem appears because two CC's termination are changed at the
> same time even though Vconn enabled.
>
> Therefore, I'm not sure the API in tcpm should be changed or only tcpci driver
> should be changed at this case. Any suggestion for this?
Well, if this is only a problem for tcpci.c, then I guess API change
would not make much sense. Maybe we can just go with this for now.
thanks,
--
heikki
