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@xxxxxxx>
> >Signed-off-by: Li Jun <jun.li@xxxxxxx>
> >---
> > 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;
>
>
>
