linux-kernel - Re: [PATCH v3] usb: typec: get the vbus source and charge values from the devicetree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180913173452.GA10115@roeck-us.net>
Date:   Thu, 13 Sep 2018 10:34:52 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Angus Ainslie <angus@...ea.ca>
Cc:     Peter Chen <hzpeterchen@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
        peter.chen@....com, jun.li@....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.

Thanks,
Guenter

> >Acked-by: Peter Chen <peter.chen@....com>
> >Signed-off-by: Li Jun <jun.li@....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;
> 
> 
>