[PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

MT6360 not support for TCPC command to control source and sink.
Uses external 5V vbus regulator as the vbus source control.

Also adds the capability to report vsafe0v.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
index f1bd9e0..0edf4b6 100644
--- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
+++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/usb/tcpm.h>
 
 #include "tcpci.h"
@@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
 	struct tcpci_data tdata;
 	struct tcpci *tcpci;
 	struct device *dev;
+	struct regulator *vbus;
 	int irq;
 };
 
@@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
 	return regmap_raw_write(regmap, reg, &val, sizeof(u16));
 }
 
+static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
+{
+	struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
+	int ret;
+
+	/* To correctly handle the already enabled vbus and disable its supply first */
+	if (regulator_is_enabled(mti->vbus)) {
+		ret = regulator_disable(mti->vbus);
+		if (ret)
+			return ret;
+	}
+
+	if (src) {
+		ret = regulator_enable(mti->vbus);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
 {
 	struct regmap *regmap = tdata->regmap;
@@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
 	if (mti->irq < 0)
 		return mti->irq;
 
+	mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
+	if (IS_ERR(mti->vbus))
+		return PTR_ERR(mti->vbus);
+
 	mti->tdata.init = mt6360_tcpc_init;
+	mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
+	mti->tdata.vbus_vsafe0v = 1;
 	mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
 	if (IS_ERR(mti->tcpci)) {
 		dev_err(&pdev->dev, "Failed to register tcpci port\n");
-- 
2.7.4

[PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add external vbus source into dt-binding description.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
index 1e8e1c2..b8d842b 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
+++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
@@ -26,6 +26,11 @@ properties:
     items:
       - const: PD_IRQB
 
+  vbus-supply:
+    description:
+      Vbus source supply regulator.
+    maxItems: 1
+
   connector:
     type: object
     $ref: ../connector/usb-connector.yaml#
@@ -38,6 +43,7 @@ required:
   - compatible
   - interrupts
   - interrupt-names
+  - vbus-supply
 
 examples:
   - |
@@ -54,6 +60,7 @@ examples:
           compatible = "mediatek,mt6360-tcpc";
           interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
           interrupt-names = "PD_IRQB";
+          vbus-supply = <&otg_vbus>;
 
           connector {
             compatible = "usb-c-connector";
-- 
2.7.4

Re: [PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description

[Rob Herring]

On Fri, 15 Jan 2021 22:13:21 +0800, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add external vbus source into dt-binding description.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml: properties:vbus-supply: 'maxItems' is not one of ['description', 'deprecated']
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml: ignoring, error in schema: properties: vbus-supply
warning: no schema found in file: ./Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml

See https://patchwork.ozlabs.org/patch/1427073

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description

[ChiYuan Huang]

Rob Herring <robh@kernel.org> 於 2021年1月17日 週日 下午11:46寫道：
>
> On Fri, 15 Jan 2021 22:13:21 +0800, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add external vbus source into dt-binding description.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml: properties:vbus-supply: 'maxItems' is not one of ['description', 'deprecated']
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml: ignoring, error in schema: properties: vbus-supply
> warning: no schema found in file: ./Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
>
> See https://patchwork.ozlabs.org/patch/1427073
>
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit.
>
Thanks, after I re-installed the yamlint, the error can be seen.
Refer to https://www.kernel.org/doc/Documentation/devicetree/bindings/example-schema.yaml
*-supply is only a phandle

In next series patch, I'll remove the maxItems in vbus-supply.
I already checked the below change. make dt_binding_check can be passed

   vbus-supply:
     description:
       Vbus source supply regulator.
-    maxItems: 1

   connector:
     type: object

Re: [PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description

[Rob Herring]

On Fri, Jan 15, 2021 at 10:13:21PM +0800, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add external vbus source into dt-binding description.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> index 1e8e1c2..b8d842b 100644
> --- a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> @@ -26,6 +26,11 @@ properties:
>      items:
>        - const: PD_IRQB
>  
> +  vbus-supply:
> +    description:
> +      Vbus source supply regulator.
> +    maxItems: 1

vbus-supply is already in the 'connector' node, you don't need it here.

> +
>    connector:
>      type: object
>      $ref: ../connector/usb-connector.yaml#
> @@ -38,6 +43,7 @@ required:
>    - compatible
>    - interrupts
>    - interrupt-names
> +  - vbus-supply
>  
>  examples:
>    - |
> @@ -54,6 +60,7 @@ examples:
>            compatible = "mediatek,mt6360-tcpc";
>            interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
>            interrupt-names = "PD_IRQB";
> +          vbus-supply = <&otg_vbus>;
>  
>            connector {
>              compatible = "usb-c-connector";
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] usb typec: tcpci: mt6360: Add vbus supply into dt-binding description

[ChiYuan Huang]

Rob Herring <robh@kernel.org> 於 2021年1月20日 週三 上午7:11寫道：
>
> On Fri, Jan 15, 2021 at 10:13:21PM +0800, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add external vbus source into dt-binding description.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > index 1e8e1c2..b8d842b 100644
> > --- a/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,mt6360-tcpc.yaml
> > @@ -26,6 +26,11 @@ properties:
> >      items:
> >        - const: PD_IRQB
> >
> > +  vbus-supply:
> > +    description:
> > +      Vbus source supply regulator.
> > +    maxItems: 1
>
> vbus-supply is already in the 'connector' node, you don't need it here.
>
If not put here, 'regulator_get' only can follow the legacy way to get
vbus regulator.
Currently, there's no one to use the 'vbus-supply' property.
From my understanding, the 'vbus-supply' is the chip level property,
not connector type property.

> > +
> >    connector:
> >      type: object
> >      $ref: ../connector/usb-connector.yaml#
> > @@ -38,6 +43,7 @@ required:
> >    - compatible
> >    - interrupts
> >    - interrupt-names
> > +  - vbus-supply
> >
> >  examples:
> >    - |
> > @@ -54,6 +60,7 @@ examples:
> >            compatible = "mediatek,mt6360-tcpc";
> >            interrupts-extended = <&gpio26 3 IRQ_TYPE_LEVEL_LOW>;
> >            interrupt-names = "PD_IRQB";
> > +          vbus-supply = <&otg_vbus>;
> >
> >            connector {
> >              compatible = "usb-c-connector";
> > --
> > 2.7.4
> >

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[Guenter Roeck]

On 1/15/21 6:13 AM, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> MT6360 not support for TCPC command to control source and sink.

does not

> Uses external 5V vbus regulator as the vbus source control.
> 
Use

> Also adds the capability to report vsafe0v.
> 
add

So far this driver works without regulator. Unless I am missing something,
this patch makes regulator support mandatory, meaning existing code will fail.
I am not sure if that is appropriate/acceptable. Can we be sure that this will
work for existing users of this driver ?

Thanks,
Guenter

> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> index f1bd9e0..0edf4b6 100644
> --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/usb/tcpm.h>
>  
>  #include "tcpci.h"
> @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
>  	struct tcpci_data tdata;
>  	struct tcpci *tcpci;
>  	struct device *dev;
> +	struct regulator *vbus;
>  	int irq;
>  };
>  
> @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
>  	return regmap_raw_write(regmap, reg, &val, sizeof(u16));
>  }
>  
> +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> +{
> +	struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> +	int ret;
> +
> +	/* To correctly handle the already enabled vbus and disable its supply first */
> +	if (regulator_is_enabled(mti->vbus)) {
> +		ret = regulator_disable(mti->vbus);
> +		if (ret)
> +			return ret;
> +	}

Is it really a good idea to disable vbus if it happens to be already enabled
and there is (another ?) request to enable it ?

> +
> +	if (src) {
> +		ret = regulator_enable(mti->vbus);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
>  {
>  	struct regmap *regmap = tdata->regmap;
> @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
>  	if (mti->irq < 0)
>  		return mti->irq;
>  
> +	mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> +	if (IS_ERR(mti->vbus))
> +		return PTR_ERR(mti->vbus);
> +
>  	mti->tdata.init = mt6360_tcpc_init;
> +	mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> +	mti->tdata.vbus_vsafe0v = 1;
>  	mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
>  	if (IS_ERR(mti->tcpci)) {
>  		dev_err(&pdev->dev, "Failed to register tcpci port\n");
> 

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[ChiYuan Huang]

Guenter Roeck <linux@roeck-us.net> 於 2021年1月18日 週一 上午1:43寫道：
>
> On 1/15/21 6:13 AM, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > MT6360 not support for TCPC command to control source and sink.
>
> does not
>
Ack
> > Uses external 5V vbus regulator as the vbus source control.
> >
> Use
>
Ack
> > Also adds the capability to report vsafe0v.
> >
> add
>
Ack
> So far this driver works without regulator. Unless I am missing something,
> this patch makes regulator support mandatory, meaning existing code will fail.
> I am not sure if that is appropriate/acceptable. Can we be sure that this will
> work for existing users of this driver ?
>
Yes, I already checked all the src/snk functionality based on  the
latest typec code.
It'll be common for our TCPC. It didn't support for TCPC command.
From the recent patches, actually, I have the local change to test the
src capability.
But I didn't submit it. It's almost the same to add set_vbus callback.
That's why I submit this change after tcpci 'set_vbus callback' is added.

> Thanks,
> Guenter
>
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > index f1bd9e0..0edf4b6 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/usb/tcpm.h>
> >
> >  #include "tcpci.h"
> > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> >       struct tcpci_data tdata;
> >       struct tcpci *tcpci;
> >       struct device *dev;
> > +     struct regulator *vbus;
> >       int irq;
> >  };
> >
> > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> >       return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> >  }
> >
> > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > +{
> > +     struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > +     int ret;
> > +
> > +     /* To correctly handle the already enabled vbus and disable its supply first */
> > +     if (regulator_is_enabled(mti->vbus)) {
> > +             ret = regulator_disable(mti->vbus);
> > +             if (ret)
> > +                     return ret;
> > +     }
>
> Is it really a good idea to disable vbus if it happens to be already enabled
> and there is (another ?) request to enable it ?
>
Yes, for  the state change from src_attach_wait to src_attach,
It need to meet the requirement that  the vbus is at vsafe0v.
So to disable it first is needed.
And to prevent other users from enabling/disabling external vbus
regulator in any case.
I think we may change regulator_get  to 'regulator_get_exclusive'.
From the design, 5v regulator only can be controlled via typec framework.
If other user touch it, it'll affect the typec state transition.
> > +
> > +     if (src) {
> > +             ret = regulator_enable(mti->vbus);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> >  {
> >       struct regmap *regmap = tdata->regmap;
> > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> >       if (mti->irq < 0)
> >               return mti->irq;
> >
> > +     mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > +     if (IS_ERR(mti->vbus))
> > +             return PTR_ERR(mti->vbus);
> > +
> >       mti->tdata.init = mt6360_tcpc_init;
> > +     mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > +     mti->tdata.vbus_vsafe0v = 1;
> >       mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> >       if (IS_ERR(mti->tcpci)) {
> >               dev_err(&pdev->dev, "Failed to register tcpci port\n");
> >
>

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[Chunfeng Yun]

On Mon, 2021-01-18 at 16:28 +0800, ChiYuan Huang wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2021年1月18日 週一 上午1:43寫道：
> >
> > On 1/15/21 6:13 AM, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > MT6360 not support for TCPC command to control source and sink.
> >
> > does not
> >
> Ack
> > > Uses external 5V vbus regulator as the vbus source control.
> > >
> > Use
> >
> Ack
> > > Also adds the capability to report vsafe0v.
> > >
> > add
> >
> Ack
> > So far this driver works without regulator. Unless I am missing something,
> > this patch makes regulator support mandatory, meaning existing code will fail.
> > I am not sure if that is appropriate/acceptable. Can we be sure that this will
> > work for existing users of this driver ?
> >
> Yes, I already checked all the src/snk functionality based on  the
> latest typec code.
> It'll be common for our TCPC. It didn't support for TCPC command.
> From the recent patches, actually, I have the local change to test the
> src capability.
> But I didn't submit it. It's almost the same to add set_vbus callback.
> That's why I submit this change after tcpci 'set_vbus callback' is added.
> 
> > Thanks,
> > Guenter
> >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > index f1bd9e0..0edf4b6 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > >  #include <linux/usb/tcpm.h>
> > >
> > >  #include "tcpci.h"
> > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > >       struct tcpci_data tdata;
> > >       struct tcpci *tcpci;
> > >       struct device *dev;
> > > +     struct regulator *vbus;
> > >       int irq;
> > >  };
> > >
> > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> > >       return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > >  }
> > >
> > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > > +{
> > > +     struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > > +     int ret;
> > > +
> > > +     /* To correctly handle the already enabled vbus and disable its supply first */
> > > +     if (regulator_is_enabled(mti->vbus)) {
> > > +             ret = regulator_disable(mti->vbus);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> >
> > Is it really a good idea to disable vbus if it happens to be already enabled
> > and there is (another ?) request to enable it ?
> >
> Yes, for  the state change from src_attach_wait to src_attach,
> It need to meet the requirement that  the vbus is at vsafe0v.
> So to disable it first is needed.
> And to prevent other users from enabling/disabling external vbus
> regulator in any case.
> I think we may change regulator_get  to 'regulator_get_exclusive'.
> From the design, 5v regulator only can be controlled via typec framework.
> If other user touch it, it'll affect the typec state transition.
How about to process the case that even switch usb controller to device
mode, platform also need to keep vbus on? e.g. Iphone Carplay


> > > +
> > > +     if (src) {
> > > +             ret = regulator_enable(mti->vbus);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > >  {
> > >       struct regmap *regmap = tdata->regmap;
> > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> > >       if (mti->irq < 0)
> > >               return mti->irq;
> > >
> > > +     mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > +     if (IS_ERR(mti->vbus))
> > > +             return PTR_ERR(mti->vbus);
> > > +
> > >       mti->tdata.init = mt6360_tcpc_init;
> > > +     mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > +     mti->tdata.vbus_vsafe0v = 1;
> > >       mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > >       if (IS_ERR(mti->tcpci)) {
> > >               dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > >
> >

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[ChiYuan Huang]

Chunfeng Yun <chunfeng.yun@mediatek.com> 於 2021年1月19日 週二 下午3:38寫道：
>
> On Mon, 2021-01-18 at 16:28 +0800, ChiYuan Huang wrote:
> > Guenter Roeck <linux@roeck-us.net> 於 2021年1月18日 週一 上午1:43寫道：
> > >
> > > On 1/15/21 6:13 AM, cy_huang wrote:
> > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > MT6360 not support for TCPC command to control source and sink.
> > >
> > > does not
> > >
> > Ack
> > > > Uses external 5V vbus regulator as the vbus source control.
> > > >
> > > Use
> > >
> > Ack
> > > > Also adds the capability to report vsafe0v.
> > > >
> > > add
> > >
> > Ack
> > > So far this driver works without regulator. Unless I am missing something,
> > > this patch makes regulator support mandatory, meaning existing code will fail.
> > > I am not sure if that is appropriate/acceptable. Can we be sure that this will
> > > work for existing users of this driver ?
> > >
> > Yes, I already checked all the src/snk functionality based on  the
> > latest typec code.
> > It'll be common for our TCPC. It didn't support for TCPC command.
> > From the recent patches, actually, I have the local change to test the
> > src capability.
> > But I didn't submit it. It's almost the same to add set_vbus callback.
> > That's why I submit this change after tcpci 'set_vbus callback' is added.
> >
> > > Thanks,
> > > Guenter
> > >
> > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > ---
> > > >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> > > >  1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > index f1bd9e0..0edf4b6 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > @@ -11,6 +11,7 @@
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/regmap.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >  #include <linux/usb/tcpm.h>
> > > >
> > > >  #include "tcpci.h"
> > > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > > >       struct tcpci_data tdata;
> > > >       struct tcpci *tcpci;
> > > >       struct device *dev;
> > > > +     struct regulator *vbus;
> > > >       int irq;
> > > >  };
> > > >
> > > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> > > >       return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > > >  }
> > > >
> > > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > > > +{
> > > > +     struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > > > +     int ret;
> > > > +
> > > > +     /* To correctly handle the already enabled vbus and disable its supply first */
> > > > +     if (regulator_is_enabled(mti->vbus)) {
> > > > +             ret = regulator_disable(mti->vbus);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > >
> > > Is it really a good idea to disable vbus if it happens to be already enabled
> > > and there is (another ?) request to enable it ?
> > >
> > Yes, for  the state change from src_attach_wait to src_attach,
> > It need to meet the requirement that  the vbus is at vsafe0v.
> > So to disable it first is needed.
> > And to prevent other users from enabling/disabling external vbus
> > regulator in any case.
> > I think we may change regulator_get  to 'regulator_get_exclusive'.
> > From the design, 5v regulator only can be controlled via typec framework.
> > If other user touch it, it'll affect the typec state transition.
> How about to process the case that even switch usb controller to device
> mode, platform also need to keep vbus on? e.g. Iphone Carplay
>
>
It must be processed by USBPD data role swap.

Type C only decide the initial role (SNK: power snk and ufp; SRC:
power src and DFP).
Only USBPD can change the power/data/vconn role individually.

> > > > +
> > > > +     if (src) {
> > > > +             ret = regulator_enable(mti->vbus);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > > >  {
> > > >       struct regmap *regmap = tdata->regmap;
> > > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> > > >       if (mti->irq < 0)
> > > >               return mti->irq;
> > > >
> > > > +     mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > > +     if (IS_ERR(mti->vbus))
> > > > +             return PTR_ERR(mti->vbus);
> > > > +
> > > >       mti->tdata.init = mt6360_tcpc_init;
> > > > +     mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > > +     mti->tdata.vbus_vsafe0v = 1;
> > > >       mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > > >       if (IS_ERR(mti->tcpci)) {
> > > >               dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > > >
> > >
>

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[ChiYuan Huang]

HI, reviews:

ChiYuan Huang <u0084500@gmail.com> 於 2021年1月19日 週二 下午4:23寫道：
>
> Chunfeng Yun <chunfeng.yun@mediatek.com> 於 2021年1月19日 週二 下午3:38寫道：
> >
> > On Mon, 2021-01-18 at 16:28 +0800, ChiYuan Huang wrote:
> > > Guenter Roeck <linux@roeck-us.net> 於 2021年1月18日 週一 上午1:43寫道：
> > > >
> > > > On 1/15/21 6:13 AM, cy_huang wrote:
> > > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > > >
> > > > > MT6360 not support for TCPC command to control source and sink.
> > > >
> > > > does not
> > > >
> > > Ack
> > > > > Uses external 5V vbus regulator as the vbus source control.
> > > > >
> > > > Use
> > > >
> > > Ack
> > > > > Also adds the capability to report vsafe0v.
> > > > >
> > > > add
> > > >
> > > Ack
> > > > So far this driver works without regulator. Unless I am missing something,
> > > > this patch makes regulator support mandatory, meaning existing code will fail.
> > > > I am not sure if that is appropriate/acceptable. Can we be sure that this will
> > > > work for existing users of this driver ?
> > > >
> > > Yes, I already checked all the src/snk functionality based on  the
> > > latest typec code.
> > > It'll be common for our TCPC. It didn't support for TCPC command.
> > > From the recent patches, actually, I have the local change to test the
> > > src capability.
> > > But I didn't submit it. It's almost the same to add set_vbus callback.
> > > That's why I submit this change after tcpci 'set_vbus callback' is added.
> > >
> > > > Thanks,
> > > > Guenter
> > > >
> > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > > ---
> > > > >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> > > > >  1 file changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > index f1bd9e0..0edf4b6 100644
> > > > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > > > @@ -11,6 +11,7 @@
> > > > >  #include <linux/of.h>
> > > > >  #include <linux/platform_device.h>
> > > > >  #include <linux/regmap.h>
> > > > > +#include <linux/regulator/consumer.h>
> > > > >  #include <linux/usb/tcpm.h>
> > > > >
> > > > >  #include "tcpci.h"
> > > > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > > > >       struct tcpci_data tdata;
> > > > >       struct tcpci *tcpci;
> > > > >       struct device *dev;
> > > > > +     struct regulator *vbus;
> > > > >       int irq;
> > > > >  };
> > > > >
> > > > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> > > > >       return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > > > >  }
> > > > >
> > > > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > > > > +{
> > > > > +     struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > > > > +     int ret;
> > > > > +
> > > > > +     /* To correctly handle the already enabled vbus and disable its supply first */
> > > > > +     if (regulator_is_enabled(mti->vbus)) {
> > > > > +             ret = regulator_disable(mti->vbus);
> > > > > +             if (ret)
> > > > > +                     return ret;
> > > > > +     }
> > > >
> > > > Is it really a good idea to disable vbus if it happens to be already enabled
> > > > and there is (another ?) request to enable it ?
> > > >
> > > Yes, for  the state change from src_attach_wait to src_attach,
> > > It need to meet the requirement that  the vbus is at vsafe0v.
> > > So to disable it first is needed.
> > > And to prevent other users from enabling/disabling external vbus
> > > regulator in any case.
> > > I think we may change regulator_get  to 'regulator_get_exclusive'.
> > > From the design, 5v regulator only can be controlled via typec framework.
> > > If other user touch it, it'll affect the typec state transition.
> > How about to process the case that even switch usb controller to device
> > mode, platform also need to keep vbus on? e.g. Iphone Carplay
> >
> >
> It must be processed by USBPD data role swap.
>
> Type C only decide the initial role (SNK: power snk and ufp; SRC:
> power src and DFP).
> Only USBPD can change the power/data/vconn role individually.
>
I'm  not sure the status about this patch.
But I'm trying to figure out the problems about some TCPCs.

Not all TCPCs can support the source_vbus/sink_vbus command.
I'm trying to add this patch make our mt6360 tcpc work.

Originally, I add some proprietary code in usb to make the otg vbus
output due to no set_vbus callback handled by vendor ops.

Eventually, I found the patch
b9358a068490 usb: typec: tcpci: Add set_vbus tcpci callback

Hope to get any response from you.
> > > > > +
> > > > > +     if (src) {
> > > > > +             ret = regulator_enable(mti->vbus);
> > > > > +             if (ret)
> > > > > +                     return ret;
> > > > > +     }
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > > > >  {
> > > > >       struct regmap *regmap = tdata->regmap;
> > > > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> > > > >       if (mti->irq < 0)
> > > > >               return mti->irq;
> > > > >
> > > > > +     mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > > > +     if (IS_ERR(mti->vbus))
> > > > > +             return PTR_ERR(mti->vbus);
> > > > > +
> > > > >       mti->tdata.init = mt6360_tcpc_init;
> > > > > +     mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > > > +     mti->tdata.vbus_vsafe0v = 1;
> > > > >       mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > > > >       if (IS_ERR(mti->tcpci)) {
> > > > >               dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > > > >
> > > >
> >

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[Chunfeng Yun]

On Sun, 2021-01-17 at 09:43 -0800, Guenter Roeck wrote:
> On 1/15/21 6:13 AM, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> > 
> > MT6360 not support for TCPC command to control source and sink.
> 
> does not
> 
> > Uses external 5V vbus regulator as the vbus source control.
> > 
> Use
> 
> > Also adds the capability to report vsafe0v.
> > 
> add
> 
> So far this driver works without regulator. Unless I am missing something,
> this patch makes regulator support mandatory, meaning existing code will fail.
If don't provide vbus-supply in DTS, regulator framework will provide a
dummy regulator, so the code will not fail.
> I am not sure if that is appropriate/acceptable. Can we be sure that this will
> work for existing users of this driver ?

> 
> Thanks,
> Guenter
> 
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > index f1bd9e0..0edf4b6 100644
> > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/usb/tcpm.h>
> >  
> >  #include "tcpci.h"
> > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> >  	struct tcpci_data tdata;
> >  	struct tcpci *tcpci;
> >  	struct device *dev;
> > +	struct regulator *vbus;
> >  	int irq;
> >  };
> >  
> > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> >  	return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> >  }
> >  
> > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > +{
> > +	struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > +	int ret;
> > +
> > +	/* To correctly handle the already enabled vbus and disable its supply first */
> > +	if (regulator_is_enabled(mti->vbus)) {
> > +		ret = regulator_disable(mti->vbus);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Is it really a good idea to disable vbus if it happens to be already enabled
> and there is (another ?) request to enable it ?
> 
> > +
> > +	if (src) {
> > +		ret = regulator_enable(mti->vbus);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> >  {
> >  	struct regmap *regmap = tdata->regmap;
> > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> >  	if (mti->irq < 0)
> >  		return mti->irq;
> >  
> > +	mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > +	if (IS_ERR(mti->vbus))
> > +		return PTR_ERR(mti->vbus);
> > +
> >  	mti->tdata.init = mt6360_tcpc_init;
> > +	mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > +	mti->tdata.vbus_vsafe0v = 1;
> >  	mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> >  	if (IS_ERR(mti->tcpci)) {
> >  		dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > 
> 

Re: [PATCH 1/2] usb typec: tcpci: mt6360: Add vsafe0v support and external vbus supply control

[ChiYuan Huang]

Chunfeng Yun <chunfeng.yun@mediatek.com> 於 2021年1月19日 週二 下午3:33寫道：
>
> On Sun, 2021-01-17 at 09:43 -0800, Guenter Roeck wrote:
> > On 1/15/21 6:13 AM, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > MT6360 not support for TCPC command to control source and sink.
> >
> > does not
> >
> > > Uses external 5V vbus regulator as the vbus source control.
> > >
> > Use
> >
> > > Also adds the capability to report vsafe0v.
> > >
> > add
> >
> > So far this driver works without regulator. Unless I am missing something,
> > this patch makes regulator support mandatory, meaning existing code will fail.
> If don't provide vbus-supply in DTS, regulator framework will provide a
> dummy regulator, so the code will not fail.
 In the last reply, I will change from regulator_get to
regulator_get_exclusive, it will return -ENODEV.
The IS_ERR can catch this situation, no dummy regulator will be returned.

And assume no vbus 5v for source & snk attached, It will cause typec
state machine repeated from
drp -> src_attach_wait -> src_attached -> PD_T_PS_SOURCE_on timeout.
It will be stuck in the loop until snk detached.

> > I am not sure if that is appropriate/acceptable. Can we be sure that this will
> > work for existing users of this driver ?
>
> >
> > Thanks,
> > Guenter
> >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > >  drivers/usb/typec/tcpm/tcpci_mt6360.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/usb/typec/tcpm/tcpci_mt6360.c b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > index f1bd9e0..0edf4b6 100644
> > > --- a/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > +++ b/drivers/usb/typec/tcpm/tcpci_mt6360.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > >  #include <linux/usb/tcpm.h>
> > >
> > >  #include "tcpci.h"
> > > @@ -36,6 +37,7 @@ struct mt6360_tcpc_info {
> > >     struct tcpci_data tdata;
> > >     struct tcpci *tcpci;
> > >     struct device *dev;
> > > +   struct regulator *vbus;
> > >     int irq;
> > >  };
> > >
> > > @@ -51,6 +53,27 @@ static inline int mt6360_tcpc_write16(struct regmap *regmap,
> > >     return regmap_raw_write(regmap, reg, &val, sizeof(u16));
> > >  }
> > >
> > > +static int mt6360_tcpc_set_vbus(struct tcpci *tcpci, struct tcpci_data *data, bool src, bool snk)
> > > +{
> > > +   struct mt6360_tcpc_info *mti = container_of(data, struct mt6360_tcpc_info, tdata);
> > > +   int ret;
> > > +
> > > +   /* To correctly handle the already enabled vbus and disable its supply first */
> > > +   if (regulator_is_enabled(mti->vbus)) {
> > > +           ret = regulator_disable(mti->vbus);
> > > +           if (ret)
> > > +                   return ret;
> > > +   }
> >
> > Is it really a good idea to disable vbus if it happens to be already enabled
> > and there is (another ?) request to enable it ?
> >
> > > +
> > > +   if (src) {
> > > +           ret = regulator_enable(mti->vbus);
> > > +           if (ret)
> > > +                   return ret;
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >  static int mt6360_tcpc_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> > >  {
> > >     struct regmap *regmap = tdata->regmap;
> > > @@ -138,7 +161,13 @@ static int mt6360_tcpc_probe(struct platform_device *pdev)
> > >     if (mti->irq < 0)
> > >             return mti->irq;
> > >
> > > +   mti->vbus = devm_regulator_get(&pdev->dev, "vbus");
> > > +   if (IS_ERR(mti->vbus))
> > > +           return PTR_ERR(mti->vbus);
> > > +
> > >     mti->tdata.init = mt6360_tcpc_init;
> > > +   mti->tdata.set_vbus = mt6360_tcpc_set_vbus;
> > > +   mti->tdata.vbus_vsafe0v = 1;
> > >     mti->tcpci = tcpci_register_port(&pdev->dev, &mti->tdata);
> > >     if (IS_ERR(mti->tcpci)) {
> > >             dev_err(&pdev->dev, "Failed to register tcpci port\n");
> > >
> >
>