RE: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

[Jun Li <jun.li@nxp.com>]

Hi
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: 2018年9月14日 1:35
> To: Angus Ainslie <angus@akkea.ca>
> Cc: Peter Chen <hzpeterchen@gmail.com>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; lkml
> <linux-kernel@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; Jun Li
> <jun.li@nxp.com>
> Subject: Re: [PATCH v3] usb: typec: get the vbus source and charge values from the
> devicetree
> 
> On Thu, Sep 13, 2018 at 05:10:09AM -0600, Angus Ainslie wrote:
> > >
> > >staging: typec: don't do vbus source disable for dead battery
> > >
> > >In PTN5110 design, DisableSourceVBUS command also disables the sink
> > >enable signal because the EN_SNK can be used to source higher
> > >voltage, and, there is only one TCPC command to disable sourcing
> > >voltage without telling whether to disable 5V or the high voltage,
> > >and to keep the design simple they designed the PTN5110 to disable
> > >both. with this fact, we use the flag drive_vbus to check if the
> > >source vbus enable was issued, if yes we then do vbus source disable,
> > >in dead battery case, we never did vbus source enable, so will not
> > >issue vbus source disable command.
> > >
> >
> > Thanks Peter, this sounds like the missing piece of information and I
> > think some form of the code below will fix that.
> >
> > There is still the issue that my board will need some way of
> > controlling the initial state of vbus-sink.
> >
> > @Guenter: would my initial patch be acceptable to set the default
> > state of vbus-source and vbus-sink. Would you like some code to sanity
> > check that both were not enabled at the same time ?
> >
> Seems to me this is indeed a chip specific problem. I don't think a fix belongs into the tcpm
> code. As mentioned before, the current status (ie drive_vbus) should be readable from the
> chip. I am not sure though if we should add a
> PTN5110 specific quirk to the driver to handle the situation. I am concerned that fixing this
> as suggested below for PTN5110 may cause trouble with other chips. Maybe not, but I'd
> rather be cautious.

Yes, this is a chip specific problem, the reason DisableSourceVBUS command also disables
the sink enable signal because the EN_SNK of PTN5110 can be used to source higher voltage
And, there is only one TCPC command to disable sourcing voltage without telling whether to
disable 5V or the high voltage, and to keep the design simple, TPN5110 is designed to disable
both.

The patch below is to not issue a vbus disable command if it was not enabled,
from my point view it should be OK and has the benefit to save one command.

Li Jun
> 
> Thanks,
> Guenter
> 
> > >Acked-by: Peter Chen <peter.chen@nxp.com>
> > >Signed-off-by: Li Jun <jun.li@nxp.com>
> > >---
> > > drivers/staging/typec/tcpci.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/drivers/staging/typec/tcpci.c
> > >b/drivers/staging/typec/tcpci.c index 2d4fbb8aac5e..7352207224b5
> > >100644
> > >--- a/drivers/staging/typec/tcpci.c
> > >+++ b/drivers/staging/typec/tcpci.c
> > >@@ -381,9 +381,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc,
> > >bool source, bool sink)
> > >  struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > >  int ret;
> > >
> > >- /* Disable both source and sink first before enabling anything */
> > >-
> > >- if (!source) {
> > >+ /* Only disable source if it was enabled */ if (!source &&
> > >+ tcpci->drive_vbus) {
> > >  ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> > >     TCPC_CMD_DISABLE_SRC_VBUS);
> > >  if (ret < 0)
> >
> > The version of struct tcpci doesn't have a drive_vbus. Where should
> > drive_vbus get set and cleared ?
> >
> > Is this a more complete version of what you intended ?
> >
> > diff --git a/drivers/usb/typec/tcpci.c b/drivers/usb/typec/tcpci.c
> > index ac6b418b15f1..d6168163df7b 100644
> > --- a/drivers/usb/typec/tcpci.c
> > +++ b/drivers/usb/typec/tcpci.c
> > @@ -28,6 +28,7 @@ struct tcpci {
> >         struct regmap *regmap;
> >
> >         bool controls_vbus;
> > +       bool drive_vbus;
> >
> >         struct tcpc_dev tcpc;
> >         struct tcpci_data *data;
> > @@ -277,7 +278,9 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc,
> > bool source, bool sink)
> >
> >         /* Disable both source and sink first before enabling anything
> > */
> >
> > -       if (!source) {
> > +       if (!source && tcpci->drive_vbus) {
> > +               tcpci->drive_vbus = false;
> > +
> >                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >                                    TCPC_CMD_DISABLE_SRC_VBUS);
> >                 if (ret < 0)
> > @@ -292,6 +295,8 @@ static int tcpci_set_vbus(struct tcpc_dev *tcpc,
> > bool source, bool sink)
> >         }
> >
> >         if (source) {
> > +               tcpci->drive_vbus = true;
> > +
> >                 ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> >                                    TCPC_CMD_SRC_VBUS_DEFAULT);
> >                 if (ret < 0)
> > @@ -503,6 +508,7 @@ struct tcpci *tcpci_register_port(struct device
> > *dev, struct tcpci_data *data)
> >         tcpci->dev = dev;
> >         tcpci->data = data;
> >         tcpci->regmap = data->regmap;
> > +       tcpci->drive_vbus = false;
> >
> >         tcpci->tcpc.init = tcpci_init;
> >         tcpci->tcpc.get_vbus = tcpci_get_vbus;
> >
> >
> >