Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 13 Sep 2018 10:34:52 -0700
From:   Guenter Roeck <linux@roeck-us.net>
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@nxp.com, jun.li@nxp.com
Subject: Re: [PATCH v3] usb: typec: get the vbus source and charge values
 from  the devicetree
Message-ID: <20180913173452.GA10115@roeck-us.net>
References: <20180906192644.24587-1-angus@akkea.ca>
 <20180911145931.32441-1-angus@akkea.ca>
 <8A418EC6-62A4-4354-8928-7693696409D1@gmail.com>
 <9d7431e51aa069f288dd4bf39e9db9f1@www.akkea.ca>
 <20180912163259.GC3300@roeck-us.net>
 <CAL411-qmQ23=hfXX3s5Zo100rSASrByMWR70qLZ8cWqt9hPgSg@mail.gmail.com>
 <aa08abf87b945dc5c8268fab3cb55cbb@www.akkea.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aa08abf87b945dc5c8268fab3cb55cbb@www.akkea.ca>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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;
> 
> 
>