[PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

Some typec super speed active channel switch can be controlled via
a GPIO, this binding can be used to specify the switch node by
a GPIO and the remote endpoint of its consumer.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
new file mode 100644
index 0000000..4ef76cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
@@ -0,0 +1,30 @@
+Typec orientation switch via a GPIO
+-----------------------------------
+
+Required properties:
+- compatible: should be set one of following:
+	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
+
+- gpios: the GPIO used to switch the super speed active channel,
+		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
+		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
+- orientation-switch: must be present.
+
+Required sub-node:
+- port: specify the remote endpoint of typec switch consumer.
+
+Example:
+
+ptn36043 {
+	compatible = "nxp,ptn36043";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_ss_sel>;
+	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
+	orientation-switch;
+
+	port {
+		usb3_data_ss: endpoint {
+			remote-endpoint = <&typec_con_ss>;
+		};
+	};
+};
-- 
2.7.4

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

Some typec super speed active channel switch can be controlled via
a GPIO, this binding can be used to specify the switch node by
a GPIO and the remote endpoint of its consumer.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
new file mode 100644
index 0000000..4ef76cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
@@ -0,0 +1,30 @@
+Typec orientation switch via a GPIO
+-----------------------------------
+
+Required properties:
+- compatible: should be set one of following:
+	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
+
+- gpios: the GPIO used to switch the super speed active channel,
+		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
+		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
+- orientation-switch: must be present.
+
+Required sub-node:
+- port: specify the remote endpoint of typec switch consumer.
+
+Example:
+
+ptn36043 {
+	compatible = "nxp,ptn36043";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_ss_sel>;
+	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
+	orientation-switch;
+
+	port {
+		usb3_data_ss: endpoint {
+			remote-endpoint = <&typec_con_ss>;
+		};
+	};
+};

[PATCH v3 2/2] usb: typec: add typec switch via GPIO control

[Jun Li]

This patch adds a simple typec switch driver which only needs
a GPIO to switch the super speed active channel according to
typec orientation.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---

Change for v3:
- Remove file name in driver description.
- Add Andy Shevchenko's Reviewed-by tag.

Changes for v2:
- Use the correct head files for gpio api and of_device_id:
  #include <linux/gpio/consumer.h>
  #include <linux/mod_devicetable.h>
- Add driver dependency on GPIOLIB

 drivers/usb/typec/mux/Kconfig       |   7 +++
 drivers/usb/typec/mux/Makefile      |   1 +
 drivers/usb/typec/mux/gpio-switch.c | 105 ++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index 01ed0d5..27120e6 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -9,4 +9,11 @@ config TYPEC_MUX_PI3USB30532
 	  Say Y or M if your system has a Pericom PI3USB30532 Type-C cross
 	  switch / mux chip found on some devices with a Type-C port.
 
+config TYPEC_SWITCH_GPIO
+	tristate "Simple Super Speed Active Switch via GPIO"
+	depends on GPIOLIB
+	help
+	  Say Y or M if your system has a typec super speed channel
+	  switch via a simple GPIO control.
+
 endmenu
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index 1332e46..e29377c 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
+obj-$(CONFIG_TYPEC_SWITCH_GPIO)		+= gpio-switch.o
diff --git a/drivers/usb/typec/mux/gpio-switch.c b/drivers/usb/typec/mux/gpio-switch.c
new file mode 100644
index 0000000..9ccfefe
--- /dev/null
+++ b/drivers/usb/typec/mux/gpio-switch.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Typec switch via a simple GPIO control driver.
+ *
+ * Copyright 2019 NXP
+ * Author: Jun Li <jun.li@nxp.com>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/usb/typec_mux.h>
+
+struct gpio_typec_switch {
+	struct typec_switch sw;
+	struct mutex lock;
+	struct gpio_desc *ss_sel;
+};
+
+static int switch_gpio_set(struct typec_switch *sw,
+			   enum typec_orientation orientation)
+{
+	struct gpio_typec_switch *gpio_sw = container_of(sw,
+				struct gpio_typec_switch, sw);
+
+	mutex_lock(&gpio_sw->lock);
+
+	switch (orientation) {
+	case TYPEC_ORIENTATION_NORMAL:
+		gpiod_set_value_cansleep(gpio_sw->ss_sel, 1);
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		gpiod_set_value_cansleep(gpio_sw->ss_sel, 0);
+		break;
+	case TYPEC_ORIENTATION_NONE:
+		break;
+	}
+
+	mutex_unlock(&gpio_sw->lock);
+
+	return 0;
+}
+
+static int typec_switch_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_typec_switch	*gpio_sw;
+	struct device			*dev = &pdev->dev;
+	int				ret;
+
+	gpio_sw = devm_kzalloc(dev, sizeof(*gpio_sw), GFP_KERNEL);
+	if (!gpio_sw)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, gpio_sw);
+
+	gpio_sw->sw.dev = dev;
+	gpio_sw->sw.set = switch_gpio_set;
+	mutex_init(&gpio_sw->lock);
+
+	/* Get the super speed active channel selection GPIO */
+	gpio_sw->ss_sel = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(gpio_sw->ss_sel))
+		return PTR_ERR(gpio_sw->ss_sel);
+
+	ret = typec_switch_register(&gpio_sw->sw);
+	if (ret) {
+		dev_err(dev, "Error registering typec switch: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int typec_switch_gpio_remove(struct platform_device *pdev)
+{
+	struct gpio_typec_switch *gpio_sw = platform_get_drvdata(pdev);
+
+	typec_switch_unregister(&gpio_sw->sw);
+
+	return 0;
+}
+
+static const struct of_device_id of_typec_switch_gpio_match[] = {
+	{ .compatible = "nxp,ptn36043" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_typec_switch_gpio_match);
+
+static struct platform_driver typec_switch_gpio_driver = {
+	.probe		= typec_switch_gpio_probe,
+	.remove		= typec_switch_gpio_remove,
+	.driver		= {
+		.name	= "typec-switch-gpio",
+		.of_match_table = of_typec_switch_gpio_match,
+	},
+};
+
+module_platform_driver(typec_switch_gpio_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TypeC Super Speed Switch GPIO driver");
+MODULE_AUTHOR("Jun Li <jun.li@nxp.com>");
-- 
2.7.4

[v3,2/2] usb: typec: add typec switch via GPIO control

[Jun Li]

This patch adds a simple typec switch driver which only needs
a GPIO to switch the super speed active channel according to
typec orientation.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---

Change for v3:
- Remove file name in driver description.
- Add Andy Shevchenko's Reviewed-by tag.

Changes for v2:
- Use the correct head files for gpio api and of_device_id:
  #include <linux/gpio/consumer.h>
  #include <linux/mod_devicetable.h>
- Add driver dependency on GPIOLIB

 drivers/usb/typec/mux/Kconfig       |   7 +++
 drivers/usb/typec/mux/Makefile      |   1 +
 drivers/usb/typec/mux/gpio-switch.c | 105 ++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index 01ed0d5..27120e6 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -9,4 +9,11 @@ config TYPEC_MUX_PI3USB30532
 	  Say Y or M if your system has a Pericom PI3USB30532 Type-C cross
 	  switch / mux chip found on some devices with a Type-C port.
 
+config TYPEC_SWITCH_GPIO
+	tristate "Simple Super Speed Active Switch via GPIO"
+	depends on GPIOLIB
+	help
+	  Say Y or M if your system has a typec super speed channel
+	  switch via a simple GPIO control.
+
 endmenu
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index 1332e46..e29377c 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
+obj-$(CONFIG_TYPEC_SWITCH_GPIO)		+= gpio-switch.o
diff --git a/drivers/usb/typec/mux/gpio-switch.c b/drivers/usb/typec/mux/gpio-switch.c
new file mode 100644
index 0000000..9ccfefe
--- /dev/null
+++ b/drivers/usb/typec/mux/gpio-switch.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Typec switch via a simple GPIO control driver.
+ *
+ * Copyright 2019 NXP
+ * Author: Jun Li <jun.li@nxp.com>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/usb/typec_mux.h>
+
+struct gpio_typec_switch {
+	struct typec_switch sw;
+	struct mutex lock;
+	struct gpio_desc *ss_sel;
+};
+
+static int switch_gpio_set(struct typec_switch *sw,
+			   enum typec_orientation orientation)
+{
+	struct gpio_typec_switch *gpio_sw = container_of(sw,
+				struct gpio_typec_switch, sw);
+
+	mutex_lock(&gpio_sw->lock);
+
+	switch (orientation) {
+	case TYPEC_ORIENTATION_NORMAL:
+		gpiod_set_value_cansleep(gpio_sw->ss_sel, 1);
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		gpiod_set_value_cansleep(gpio_sw->ss_sel, 0);
+		break;
+	case TYPEC_ORIENTATION_NONE:
+		break;
+	}
+
+	mutex_unlock(&gpio_sw->lock);
+
+	return 0;
+}
+
+static int typec_switch_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_typec_switch	*gpio_sw;
+	struct device			*dev = &pdev->dev;
+	int				ret;
+
+	gpio_sw = devm_kzalloc(dev, sizeof(*gpio_sw), GFP_KERNEL);
+	if (!gpio_sw)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, gpio_sw);
+
+	gpio_sw->sw.dev = dev;
+	gpio_sw->sw.set = switch_gpio_set;
+	mutex_init(&gpio_sw->lock);
+
+	/* Get the super speed active channel selection GPIO */
+	gpio_sw->ss_sel = devm_gpiod_get(dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(gpio_sw->ss_sel))
+		return PTR_ERR(gpio_sw->ss_sel);
+
+	ret = typec_switch_register(&gpio_sw->sw);
+	if (ret) {
+		dev_err(dev, "Error registering typec switch: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int typec_switch_gpio_remove(struct platform_device *pdev)
+{
+	struct gpio_typec_switch *gpio_sw = platform_get_drvdata(pdev);
+
+	typec_switch_unregister(&gpio_sw->sw);
+
+	return 0;
+}
+
+static const struct of_device_id of_typec_switch_gpio_match[] = {
+	{ .compatible = "nxp,ptn36043" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_typec_switch_gpio_match);
+
+static struct platform_driver typec_switch_gpio_driver = {
+	.probe		= typec_switch_gpio_probe,
+	.remove		= typec_switch_gpio_remove,
+	.driver		= {
+		.name	= "typec-switch-gpio",
+		.of_match_table = of_typec_switch_gpio_match,
+	},
+};
+
+module_platform_driver(typec_switch_gpio_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TypeC Super Speed Switch GPIO driver");
+MODULE_AUTHOR("Jun Li <jun.li@nxp.com>");

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 11-03-19 11:40, Jun Li wrote:
> Some typec super speed active channel switch can be controlled via
> a GPIO, this binding can be used to specify the switch node by
> a GPIO and the remote endpoint of its consumer.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> new file mode 100644
> index 0000000..4ef76cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> @@ -0,0 +1,30 @@
> +Typec orientation switch via a GPIO
> +-----------------------------------
> +
> +Required properties:
> +- compatible: should be set one of following:
> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> +
> +- gpios: the GPIO used to switch the super speed active channel,
> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> +- orientation-switch: must be present.

Shouldn't this have usb-c in the propery name, e.g.:
usb-c-orientation-switch  ?

> +
> +Required sub-node:
> +- port: specify the remote endpoint of typec switch consumer.
> +
> +Example:
> +
> +ptn36043 {
> +	compatible = "nxp,ptn36043";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ss_sel>;
> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +	orientation-switch;
> +
> +	port {
> +		usb3_data_ss: endpoint {
> +			remote-endpoint = <&typec_con_ss>;


Isn't this the wrong way around, shouldn't the "usb-c-connector"
compatible port be pointing to the orientation switch, rather then
the other way around?  Both will work in the end. but to me it
feels more natural to group all the info about the type-c connector
together in the "usb-c-connector" compatible port

Regards,

Hans

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 11-03-19 11:40, Jun Li wrote:
> Some typec super speed active channel switch can be controlled via
> a GPIO, this binding can be used to specify the switch node by
> a GPIO and the remote endpoint of its consumer.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> new file mode 100644
> index 0000000..4ef76cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> @@ -0,0 +1,30 @@
> +Typec orientation switch via a GPIO
> +-----------------------------------
> +
> +Required properties:
> +- compatible: should be set one of following:
> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> +
> +- gpios: the GPIO used to switch the super speed active channel,
> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> +- orientation-switch: must be present.

Shouldn't this have usb-c in the propery name, e.g.:
usb-c-orientation-switch  ?

> +
> +Required sub-node:
> +- port: specify the remote endpoint of typec switch consumer.
> +
> +Example:
> +
> +ptn36043 {
> +	compatible = "nxp,ptn36043";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ss_sel>;
> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +	orientation-switch;
> +
> +	port {
> +		usb3_data_ss: endpoint {
> +			remote-endpoint = <&typec_con_ss>;


Isn't this the wrong way around, shouldn't the "usb-c-connector"
compatible port be pointing to the orientation switch, rather then
the other way around?  Both will work in the end. but to me it
feels more natural to group all the info about the type-c connector
together in the "usb-c-connector" compatible port

Regards,

Hans

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 11-03-19 12:02, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 11:40, Jun Li wrote:
>> Some typec super speed active channel switch can be controlled via
>> a GPIO, this binding can be used to specify the switch node by
>> a GPIO and the remote endpoint of its consumer.
>>
>> Signed-off-by: Li Jun <jun.li@nxp.com>
>> ---
>>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>> new file mode 100644
>> index 0000000..4ef76cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>> @@ -0,0 +1,30 @@
>> +Typec orientation switch via a GPIO
>> +-----------------------------------
>> +
>> +Required properties:
>> +- compatible: should be set one of following:
>> +    - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.

Hmm, it seems that this binding should work fine with
other orientation-switches as well, so I think this needs
a generic compatible string.

>> +
>> +- gpios: the GPIO used to switch the super speed active channel,
>> +        GPIO_ACTIVE_HIGH: GPIO state high for cc1;
>> +        GPIO_ACTIVE_LOW:  GPIO state low for cc1.
>> +- orientation-switch: must be present.
> 
> Shouldn't this have usb-c in the propery name, e.g.:
> usb-c-orientation-switch  ?

Also perhaps it would be better to use an additional compatible
string for this, rather then a boolean property, because what you
are trying to say is that this device is compatible with some
(to be written) generic usb-c-orientation-switch binding.

So I think you may want to use an extra compatible for this
and describe the port/graph usage linking the usb-c-connector port
and the port on the orientation-switch together in a new
usb-c-orientation-switch binding document.

This new binding will then document the port usage which is
mostly undocumented in your typec-switch-gpio.txt binding and
this port usage documentation can then be re-used for other
orientation-switch bindings.

Regards,

Hans


> 
>> +
>> +Required sub-node:
>> +- port: specify the remote endpoint of typec switch consumer.
>> +
>> +Example:
>> +
>> +ptn36043 {
>> +    compatible = "nxp,ptn36043";
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&pinctrl_ss_sel>;
>> +    gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>> +    orientation-switch;
>> +
>> +    port {
>> +        usb3_data_ss: endpoint {
>> +            remote-endpoint = <&typec_con_ss>;
> 
> 
> Isn't this the wrong way around, shouldn't the "usb-c-connector"
> compatible port be pointing to the orientation switch, rather then
> the other way around?  Both will work in the end. but to me it
> feels more natural to group all the info about the type-c connector
> together in the "usb-c-connector" compatible port
> 
> Regards,
> 
> Hans
> 

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 11-03-19 12:02, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 11:40, Jun Li wrote:
>> Some typec super speed active channel switch can be controlled via
>> a GPIO, this binding can be used to specify the switch node by
>> a GPIO and the remote endpoint of its consumer.
>>
>> Signed-off-by: Li Jun <jun.li@nxp.com>
>> ---
>>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>> new file mode 100644
>> index 0000000..4ef76cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>> @@ -0,0 +1,30 @@
>> +Typec orientation switch via a GPIO
>> +-----------------------------------
>> +
>> +Required properties:
>> +- compatible: should be set one of following:
>> +    - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.

Hmm, it seems that this binding should work fine with
other orientation-switches as well, so I think this needs
a generic compatible string.

>> +
>> +- gpios: the GPIO used to switch the super speed active channel,
>> +        GPIO_ACTIVE_HIGH: GPIO state high for cc1;
>> +        GPIO_ACTIVE_LOW:  GPIO state low for cc1.
>> +- orientation-switch: must be present.
> 
> Shouldn't this have usb-c in the propery name, e.g.:
> usb-c-orientation-switch  ?

Also perhaps it would be better to use an additional compatible
string for this, rather then a boolean property, because what you
are trying to say is that this device is compatible with some
(to be written) generic usb-c-orientation-switch binding.

So I think you may want to use an extra compatible for this
and describe the port/graph usage linking the usb-c-connector port
and the port on the orientation-switch together in a new
usb-c-orientation-switch binding document.

This new binding will then document the port usage which is
mostly undocumented in your typec-switch-gpio.txt binding and
this port usage documentation can then be re-used for other
orientation-switch bindings.

Regards,

Hans


> 
>> +
>> +Required sub-node:
>> +- port: specify the remote endpoint of typec switch consumer.
>> +
>> +Example:
>> +
>> +ptn36043 {
>> +    compatible = "nxp,ptn36043";
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&pinctrl_ss_sel>;
>> +    gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>> +    orientation-switch;
>> +
>> +    port {
>> +        usb3_data_ss: endpoint {
>> +            remote-endpoint = <&typec_con_ss>;
> 
> 
> Isn't this the wrong way around, shouldn't the "usb-c-connector"
> compatible port be pointing to the orientation switch, rather then
> the other way around?  Both will work in the end. but to me it
> feels more natural to group all the info about the type-c connector
> together in the "usb-c-connector" compatible port
> 
> Regards,
> 
> Hans
>

RE: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

Hi Hans
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 2019年3月11日 19:03
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> Hi,
> 
> On 11-03-19 11:40, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via a
> > GPIO, this binding can be used to specify the switch node by a GPIO
> > and the remote endpoint of its consumer.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> ++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Shouldn't this have usb-c in the propery name, e.g.:
> usb-c-orientation-switch  ?

This is decided by drivers/usb/typec/mux.c:36
/*
 * With OF graph the mux node must have a boolean device property named
 * "orientation-switch".
 */
> 
> > +
> > +Required sub-node:
> > +- port: specify the remote endpoint of typec switch consumer.
> > +
> > +Example:
> > +
> > +ptn36043 {
> > +	compatible = "nxp,ptn36043";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +	orientation-switch;
> > +
> > +	port {
> > +		usb3_data_ss: endpoint {
> > +			remote-endpoint = <&typec_con_ss>;
> 
> 
> Isn't this the wrong way around, shouldn't the "usb-c-connector"
> compatible port be pointing to the orientation switch, rather then the other way
> around?  

I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
yes, it is pointing to the orientation switch provider(i.e, this example node).

>Both will work in the end. but to me it feels more natural to group all the
> info about the type-c connector together in the "usb-c-connector" compatible port
>

        ptn36043 {
                compatible = "nxp,ptn36043";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_ss_sel>;
                gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
                orientation-switch;

                port {
                        usb3_data_ss: endpoint {
                                remote-endpoint = <&typec_con_ss>;
                        };
                };
        };

usb_con: connector {
	compatible = "usb-c-connector";
	...
	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@1 {
			reg = <1>;
			typec_con_ss: endpoint {
				remote-endpoint = <&usb3_data_ss>;
			};
		};
	};
};

Regards
Jun
> Regards,
> 
> Hans

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

Hi Hans
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 2019年3月11日 19:03
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> Hi,
> 
> On 11-03-19 11:40, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via a
> > GPIO, this binding can be used to specify the switch node by a GPIO
> > and the remote endpoint of its consumer.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> ++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Shouldn't this have usb-c in the propery name, e.g.:
> usb-c-orientation-switch  ?

This is decided by drivers/usb/typec/mux.c:36
/*
 * With OF graph the mux node must have a boolean device property named
 * "orientation-switch".
 */
> 
> > +
> > +Required sub-node:
> > +- port: specify the remote endpoint of typec switch consumer.
> > +
> > +Example:
> > +
> > +ptn36043 {
> > +	compatible = "nxp,ptn36043";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +	orientation-switch;
> > +
> > +	port {
> > +		usb3_data_ss: endpoint {
> > +			remote-endpoint = <&typec_con_ss>;
> 
> 
> Isn't this the wrong way around, shouldn't the "usb-c-connector"
> compatible port be pointing to the orientation switch, rather then the other way
> around?  

I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
yes, it is pointing to the orientation switch provider(i.e, this example node).

>Both will work in the end. but to me it feels more natural to group all the
> info about the type-c connector together in the "usb-c-connector" compatible port
>

        ptn36043 {
                compatible = "nxp,ptn36043";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_ss_sel>;
                gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
                orientation-switch;

                port {
                        usb3_data_ss: endpoint {
                                remote-endpoint = <&typec_con_ss>;
                        };
                };
        };

usb_con: connector {
	compatible = "usb-c-connector";
	...
	ports {
		#address-cells = <1>;
		#size-cells = <0>;

		port@1 {
			reg = <1>;
			typec_con_ss: endpoint {
				remote-endpoint = <&usb3_data_ss>;
			};
		};
	};
};

Regards
Jun
> Regards,
> 
> Hans

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

Hi,

On Tue, Mar 12, 2019 at 10:32:00AM +0000, Jun Li wrote:
> Hi Hans
> > -----Original Message-----
> > From: Hans de Goede <hdegoede@redhat.com>
> > Sent: 2019年3月11日 19:03
> > To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> > Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> > via GPIO
> > 
> > Hi,
> > 
> > On 11-03-19 11:40, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > ++++++++++++++++++++++
> > >   1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 0000000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +-----------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> > 
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch  ?
> 
> This is decided by drivers/usb/typec/mux.c:36
> /*
>  * With OF graph the mux node must have a boolean device property named
>  * "orientation-switch".
>  */

Yes, but it's still OK to change it. It's not documented anywhere yet.

> > > +
> > > +Required sub-node:
> > > +- port: specify the remote endpoint of typec switch consumer.
> > > +
> > > +Example:
> > > +
> > > +ptn36043 {
> > > +	compatible = "nxp,ptn36043";
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > +	orientation-switch;
> > > +
> > > +	port {
> > > +		usb3_data_ss: endpoint {
> > > +			remote-endpoint = <&typec_con_ss>;
> > 
> > 
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the other way
> > around?  

Hans, in OF graph both endpoints will have a remote-endpoint pointing
to each other..

> I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
> yes, it is pointing to the orientation switch provider(i.e, this example node).
> 
> >Both will work in the end. but to me it feels more natural to group all the
> > info about the type-c connector together in the "usb-c-connector" compatible port
> >
> 
>         ptn36043 {
>                 compatible = "nxp,ptn36043";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_ss_sel>;
>                 gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>                 orientation-switch;
> 
>                 port {
>                         usb3_data_ss: endpoint {
>                                 remote-endpoint = <&typec_con_ss>;
>                         };
>                 };
>         };
> 
> usb_con: connector {
> 	compatible = "usb-c-connector";
> 	...
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@1 {
> 			reg = <1>;
> 			typec_con_ss: endpoint {
> 				remote-endpoint = <&usb3_data_ss>;
> 			};
> 		};
> 	};
> };

So like that.

thanks,

-- 
heikki

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

Hi,

On Tue, Mar 12, 2019 at 10:32:00AM +0000, Jun Li wrote:
> Hi Hans
> > -----Original Message-----
> > From: Hans de Goede <hdegoede@redhat.com>
> > Sent: 2019年3月11日 19:03
> > To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> > Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> > via GPIO
> > 
> > Hi,
> > 
> > On 11-03-19 11:40, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > ++++++++++++++++++++++
> > >   1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 0000000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +-----------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> > 
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch  ?
> 
> This is decided by drivers/usb/typec/mux.c:36
> /*
>  * With OF graph the mux node must have a boolean device property named
>  * "orientation-switch".
>  */

Yes, but it's still OK to change it. It's not documented anywhere yet.

> > > +
> > > +Required sub-node:
> > > +- port: specify the remote endpoint of typec switch consumer.
> > > +
> > > +Example:
> > > +
> > > +ptn36043 {
> > > +	compatible = "nxp,ptn36043";
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > +	orientation-switch;
> > > +
> > > +	port {
> > > +		usb3_data_ss: endpoint {
> > > +			remote-endpoint = <&typec_con_ss>;
> > 
> > 
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the other way
> > around?  

Hans, in OF graph both endpoints will have a remote-endpoint pointing
to each other..

> I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
> yes, it is pointing to the orientation switch provider(i.e, this example node).
> 
> >Both will work in the end. but to me it feels more natural to group all the
> > info about the type-c connector together in the "usb-c-connector" compatible port
> >
> 
>         ptn36043 {
>                 compatible = "nxp,ptn36043";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_ss_sel>;
>                 gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>                 orientation-switch;
> 
>                 port {
>                         usb3_data_ss: endpoint {
>                                 remote-endpoint = <&typec_con_ss>;
>                         };
>                 };
>         };
> 
> usb_con: connector {
> 	compatible = "usb-c-connector";
> 	...
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@1 {
> 			reg = <1>;
> 			typec_con_ss: endpoint {
> 				remote-endpoint = <&usb3_data_ss>;
> 			};
> 		};
> 	};
> };

So like that.

thanks,

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 12-03-19 11:45, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Mar 12, 2019 at 10:32:00AM +0000, Jun Li wrote:
>> Hi Hans
>>> -----Original Message-----
>>> From: Hans de Goede <hdegoede@redhat.com>
>>> Sent: 2019年3月11日 19:03
>>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
>>> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
>>> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
>>> <linux-imx@nxp.com>
>>> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
>>> via GPIO
>>>
>>> Hi,
>>>
>>> On 11-03-19 11:40, Jun Li wrote:
>>>> Some typec super speed active channel switch can be controlled via a
>>>> GPIO, this binding can be used to specify the switch node by a GPIO
>>>> and the remote endpoint of its consumer.
>>>>
>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>> ---
>>>>    .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
>>> ++++++++++++++++++++++
>>>>    1 file changed, 30 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> new file mode 100644
>>>> index 0000000..4ef76cf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> @@ -0,0 +1,30 @@
>>>> +Typec orientation switch via a GPIO
>>>> +-----------------------------------
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be set one of following:
>>>> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
>>>> +
>>>> +- gpios: the GPIO used to switch the super speed active channel,
>>>> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
>>>> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
>>>> +- orientation-switch: must be present.
>>>
>>> Shouldn't this have usb-c in the propery name, e.g.:
>>> usb-c-orientation-switch  ?
>>
>> This is decided by drivers/usb/typec/mux.c:36
>> /*
>>   * With OF graph the mux node must have a boolean device property named
>>   * "orientation-switch".
>>   */
> 
> Yes, but it's still OK to change it. It's not documented anywhere yet.
> 
>>>> +
>>>> +Required sub-node:
>>>> +- port: specify the remote endpoint of typec switch consumer.
>>>> +
>>>> +Example:
>>>> +
>>>> +ptn36043 {
>>>> +	compatible = "nxp,ptn36043";
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&pinctrl_ss_sel>;
>>>> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>>>> +	orientation-switch;
>>>> +
>>>> +	port {
>>>> +		usb3_data_ss: endpoint {
>>>> +			remote-endpoint = <&typec_con_ss>;
>>>
>>>
>>> Isn't this the wrong way around, shouldn't the "usb-c-connector"
>>> compatible port be pointing to the orientation switch, rather then the other way
>>> around?
> 
> Hans, in OF graph both endpoints will have a remote-endpoint pointing
> to each other..
> 
>> I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
>> yes, it is pointing to the orientation switch provider(i.e, this example node).
>>
>>> Both will work in the end. but to me it feels more natural to group all the
>>> info about the type-c connector together in the "usb-c-connector" compatible port
>>>
>>
>>          ptn36043 {
>>                  compatible = "nxp,ptn36043";
>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&pinctrl_ss_sel>;
>>                  gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>>                  orientation-switch;
>>
>>                  port {
>>                          usb3_data_ss: endpoint {
>>                                  remote-endpoint = <&typec_con_ss>;
>>                          };
>>                  };
>>          };
>>
>> usb_con: connector {
>> 	compatible = "usb-c-connector";
>> 	...
>> 	ports {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		port@1 {
>> 			reg = <1>;
>> 			typec_con_ss: endpoint {
>> 				remote-endpoint = <&usb3_data_ss>;
>> 			};
>> 		};
>> 	};
>> };
> 
> So like that.

Ah I see, thank you for clarifying that.

Regards,

Hans

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 12-03-19 11:45, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Mar 12, 2019 at 10:32:00AM +0000, Jun Li wrote:
>> Hi Hans
>>> -----Original Message-----
>>> From: Hans de Goede <hdegoede@redhat.com>
>>> Sent: 2019年3月11日 19:03
>>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
>>> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
>>> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
>>> <linux-imx@nxp.com>
>>> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
>>> via GPIO
>>>
>>> Hi,
>>>
>>> On 11-03-19 11:40, Jun Li wrote:
>>>> Some typec super speed active channel switch can be controlled via a
>>>> GPIO, this binding can be used to specify the switch node by a GPIO
>>>> and the remote endpoint of its consumer.
>>>>
>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>> ---
>>>>    .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
>>> ++++++++++++++++++++++
>>>>    1 file changed, 30 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> new file mode 100644
>>>> index 0000000..4ef76cf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> @@ -0,0 +1,30 @@
>>>> +Typec orientation switch via a GPIO
>>>> +-----------------------------------
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be set one of following:
>>>> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
>>>> +
>>>> +- gpios: the GPIO used to switch the super speed active channel,
>>>> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
>>>> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
>>>> +- orientation-switch: must be present.
>>>
>>> Shouldn't this have usb-c in the propery name, e.g.:
>>> usb-c-orientation-switch  ?
>>
>> This is decided by drivers/usb/typec/mux.c:36
>> /*
>>   * With OF graph the mux node must have a boolean device property named
>>   * "orientation-switch".
>>   */
> 
> Yes, but it's still OK to change it. It's not documented anywhere yet.
> 
>>>> +
>>>> +Required sub-node:
>>>> +- port: specify the remote endpoint of typec switch consumer.
>>>> +
>>>> +Example:
>>>> +
>>>> +ptn36043 {
>>>> +	compatible = "nxp,ptn36043";
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&pinctrl_ss_sel>;
>>>> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>>>> +	orientation-switch;
>>>> +
>>>> +	port {
>>>> +		usb3_data_ss: endpoint {
>>>> +			remote-endpoint = <&typec_con_ss>;
>>>
>>>
>>> Isn't this the wrong way around, shouldn't the "usb-c-connector"
>>> compatible port be pointing to the orientation switch, rather then the other way
>>> around?
> 
> Hans, in OF graph both endpoints will have a remote-endpoint pointing
> to each other..
> 
>> I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
>> yes, it is pointing to the orientation switch provider(i.e, this example node).
>>
>>> Both will work in the end. but to me it feels more natural to group all the
>>> info about the type-c connector together in the "usb-c-connector" compatible port
>>>
>>
>>          ptn36043 {
>>                  compatible = "nxp,ptn36043";
>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&pinctrl_ss_sel>;
>>                  gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>>                  orientation-switch;
>>
>>                  port {
>>                          usb3_data_ss: endpoint {
>>                                  remote-endpoint = <&typec_con_ss>;
>>                          };
>>                  };
>>          };
>>
>> usb_con: connector {
>> 	compatible = "usb-c-connector";
>> 	...
>> 	ports {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		port@1 {
>> 			reg = <1>;
>> 			typec_con_ss: endpoint {
>> 				remote-endpoint = <&usb3_data_ss>;
>> 			};
>> 		};
>> 	};
>> };
> 
> So like that.

Ah I see, thank you for clarifying that.

Regards,

Hans

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Rob Herring]

On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> Some typec super speed active channel switch can be controlled via
> a GPIO, this binding can be used to specify the switch node by
> a GPIO and the remote endpoint of its consumer.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> new file mode 100644
> index 0000000..4ef76cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> @@ -0,0 +1,30 @@
> +Typec orientation switch via a GPIO
> +-----------------------------------
> +
> +Required properties:
> +- compatible: should be set one of following:
> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> +
> +- gpios: the GPIO used to switch the super speed active channel,

Perhaps switch-gpios in case there are other gpios needed.

> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> +- orientation-switch: must be present.

Why is this needed? The compatible can't imply this?

> +
> +Required sub-node:
> +- port: specify the remote endpoint of typec switch consumer.
> +
> +Example:
> +
> +ptn36043 {
> +	compatible = "nxp,ptn36043";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ss_sel>;
> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +	orientation-switch;
> +
> +	port {
> +		usb3_data_ss: endpoint {
> +			remote-endpoint = <&typec_con_ss>;
> +		};
> +	};
> +};
> -- 
> 2.7.4
> 

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Rob Herring]

On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> Some typec super speed active channel switch can be controlled via
> a GPIO, this binding can be used to specify the switch node by
> a GPIO and the remote endpoint of its consumer.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> new file mode 100644
> index 0000000..4ef76cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> @@ -0,0 +1,30 @@
> +Typec orientation switch via a GPIO
> +-----------------------------------
> +
> +Required properties:
> +- compatible: should be set one of following:
> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> +
> +- gpios: the GPIO used to switch the super speed active channel,

Perhaps switch-gpios in case there are other gpios needed.

> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> +- orientation-switch: must be present.

Why is this needed? The compatible can't imply this?

> +
> +Required sub-node:
> +- port: specify the remote endpoint of typec switch consumer.
> +
> +Example:
> +
> +ptn36043 {
> +	compatible = "nxp,ptn36043";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ss_sel>;
> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +	orientation-switch;
> +
> +	port {
> +		usb3_data_ss: endpoint {
> +			remote-endpoint = <&typec_con_ss>;
> +		};
> +	};
> +};
> -- 
> 2.7.4
>

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via
> > a GPIO, this binding can be used to specify the switch node by
> > a GPIO and the remote endpoint of its consumer.
> > 
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> 
> Perhaps switch-gpios in case there are other gpios needed.
> 
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Why is this needed? The compatible can't imply this?

I think Jun Li is added that based on the comment I put to
drivers/usb/typec/mux.c, so I'm to blame here. If we can handle this
with the compatible like I guess we can, I'm happy.

thanks,

-- 
heikki

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via
> > a GPIO, this binding can be used to specify the switch node by
> > a GPIO and the remote endpoint of its consumer.
> > 
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> 
> Perhaps switch-gpios in case there are other gpios needed.
> 
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Why is this needed? The compatible can't imply this?

I think Jun Li is added that based on the comment I put to
drivers/usb/typec/mux.c, so I'm to blame here. If we can handle this
with the compatible like I guess we can, I'm happy.

thanks,

RE: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 2019年3月11日 19:12
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> Hi,
> 
> On 11-03-19 12:02, Hans de Goede wrote:
> > Hi,
> >
> > On 11-03-19 11:40, Jun Li wrote:
> >> Some typec super speed active channel switch can be controlled via a
> >> GPIO, this binding can be used to specify the switch node by a GPIO
> >> and the remote endpoint of its consumer.
> >>
> >> Signed-off-by: Li Jun <jun.li@nxp.com>
> >> ---
> >>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> >> ++++++++++++++++++++++
> >>   1 file changed, 30 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> new file mode 100644
> >> index 0000000..4ef76cf
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> @@ -0,0 +1,30 @@
> >> +Typec orientation switch via a GPIO
> >> +-----------------------------------
> >> +
> >> +Required properties:
> >> +- compatible: should be set one of following:
> >> +    - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> 
> Hmm, it seems that this binding should work fine with other orientation-switches as
> well, so I think this needs a generic compatible string.
> 
> >> +
> >> +- gpios: the GPIO used to switch the super speed active channel,
> >> +        GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> >> +        GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> >> +- orientation-switch: must be present.
> >
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch  ?
> 
> Also perhaps it would be better to use an additional compatible string for this, rather
> then a boolean property, because what you are trying to say is that this device is
> compatible with some (to be written) generic usb-c-orientation-switch binding.
> 
> So I think you may want to use an extra compatible for this and describe the
> port/graph usage linking the usb-c-connector port and the port on the
> orientation-switch together in a new usb-c-orientation-switch binding document.
> 

This patch was to only cover one kind of *typical* typec switch: done by
GPIO toggle, as I don't know how other typec switch may be implemented,
I will try to change this to be a *common* typec switch by using a generic
compatible(type-c-orientation-switch), which will for now only support
switch-gpios.

> This new binding will then document the port usage which is mostly undocumented
> in your typec-switch-gpio.txt binding and this port usage documentation can then be
> re-used for other orientation-switch bindings.

Port usage should be the same as I gave the example:
https://www.spinics.net/lists/devicetree/msg278042.html

thanks
Li Jun
> 
> Regards,
> 
> Hans
> 
> 
> >
> >> +
> >> +Required sub-node:
> >> +- port: specify the remote endpoint of typec switch consumer.
> >> +
> >> +Example:
> >> +
> >> +ptn36043 {
> >> +    compatible = "nxp,ptn36043";
> >> +    pinctrl-names = "default";
> >> +    pinctrl-0 = <&pinctrl_ss_sel>;
> >> +    gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> >> +    orientation-switch;
> >> +
> >> +    port {
> >> +        usb3_data_ss: endpoint {
> >> +            remote-endpoint = <&typec_con_ss>;
> >
> >
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the
> > other way around?  Both will work in the end. but to me it feels more
> > natural to group all the info about the type-c connector together in
> > the "usb-c-connector" compatible port
> >
> > Regards,
> >
> > Hans
> >

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 2019年3月11日 19:12
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> Hi,
> 
> On 11-03-19 12:02, Hans de Goede wrote:
> > Hi,
> >
> > On 11-03-19 11:40, Jun Li wrote:
> >> Some typec super speed active channel switch can be controlled via a
> >> GPIO, this binding can be used to specify the switch node by a GPIO
> >> and the remote endpoint of its consumer.
> >>
> >> Signed-off-by: Li Jun <jun.li@nxp.com>
> >> ---
> >>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> >> ++++++++++++++++++++++
> >>   1 file changed, 30 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> new file mode 100644
> >> index 0000000..4ef76cf
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> @@ -0,0 +1,30 @@
> >> +Typec orientation switch via a GPIO
> >> +-----------------------------------
> >> +
> >> +Required properties:
> >> +- compatible: should be set one of following:
> >> +    - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> 
> Hmm, it seems that this binding should work fine with other orientation-switches as
> well, so I think this needs a generic compatible string.
> 
> >> +
> >> +- gpios: the GPIO used to switch the super speed active channel,
> >> +        GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> >> +        GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> >> +- orientation-switch: must be present.
> >
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch  ?
> 
> Also perhaps it would be better to use an additional compatible string for this, rather
> then a boolean property, because what you are trying to say is that this device is
> compatible with some (to be written) generic usb-c-orientation-switch binding.
> 
> So I think you may want to use an extra compatible for this and describe the
> port/graph usage linking the usb-c-connector port and the port on the
> orientation-switch together in a new usb-c-orientation-switch binding document.
> 

This patch was to only cover one kind of *typical* typec switch: done by
GPIO toggle, as I don't know how other typec switch may be implemented,
I will try to change this to be a *common* typec switch by using a generic
compatible(type-c-orientation-switch), which will for now only support
switch-gpios.

> This new binding will then document the port usage which is mostly undocumented
> in your typec-switch-gpio.txt binding and this port usage documentation can then be
> re-used for other orientation-switch bindings.

Port usage should be the same as I gave the example:
https://www.spinics.net/lists/devicetree/msg278042.html

thanks
Li Jun
> 
> Regards,
> 
> Hans
> 
> 
> >
> >> +
> >> +Required sub-node:
> >> +- port: specify the remote endpoint of typec switch consumer.
> >> +
> >> +Example:
> >> +
> >> +ptn36043 {
> >> +    compatible = "nxp,ptn36043";
> >> +    pinctrl-names = "default";
> >> +    pinctrl-0 = <&pinctrl_ss_sel>;
> >> +    gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> >> +    orientation-switch;
> >> +
> >> +    port {
> >> +        usb3_data_ss: endpoint {
> >> +            remote-endpoint = <&typec_con_ss>;
> >
> >
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the
> > other way around?  Both will work in the end. but to me it feels more
> > natural to group all the info about the type-c connector together in
> > the "usb-c-connector" compatible port
> >
> > Regards,
> >
> > Hans
> >

RE: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2019年3月12日 22:45
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> hdegoede@redhat.com; andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via a
> > GPIO, this binding can be used to specify the switch node by a GPIO
> > and the remote endpoint of its consumer.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> 
> Perhaps switch-gpios in case there are other gpios needed.

OK, I will change it to be switch-gpios.

> 
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Why is this needed? The compatible can't imply this?

I will try to remove this bool property and use a general compatible string.

> 
> > +
> > +Required sub-node:
> > +- port: specify the remote endpoint of typec switch consumer.
> > +
> > +Example:
> > +
> > +ptn36043 {
> > +	compatible = "nxp,ptn36043";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +	orientation-switch;
> > +
> > +	port {
> > +		usb3_data_ss: endpoint {
> > +			remote-endpoint = <&typec_con_ss>;
> > +		};
> > +	};
> > +};
> > --
> > 2.7.4
> >

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2019年3月12日 22:45
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> hdegoede@redhat.com; andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via a
> > GPIO, this binding can be used to specify the switch node by a GPIO
> > and the remote endpoint of its consumer.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> 
> Perhaps switch-gpios in case there are other gpios needed.

OK, I will change it to be switch-gpios.

> 
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Why is this needed? The compatible can't imply this?

I will try to remove this bool property and use a general compatible string.

> 
> > +
> > +Required sub-node:
> > +- port: specify the remote endpoint of typec switch consumer.
> > +
> > +Example:
> > +
> > +ptn36043 {
> > +	compatible = "nxp,ptn36043";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +	orientation-switch;
> > +
> > +	port {
> > +		usb3_data_ss: endpoint {
> > +			remote-endpoint = <&typec_con_ss>;
> > +		};
> > +	};
> > +};
> > --
> > 2.7.4
> >

RE: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年3月13日 17:36
> To: Rob Herring <robh@kernel.org>
> Cc: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> > On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > > ++++++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 0000000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +-----------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> >
> > Perhaps switch-gpios in case there are other gpios needed.
> >
> > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> >
> > Why is this needed? The compatible can't imply this?
> 
> I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c,
> so I'm to blame here. If we can handle this with the compatible like I guess we can,
> I'm happy.

Hi Heikki

Can I just remove the original bool property check? i.e, match OK if the remote
parent node is in switch_list.

Thanks
Li Jun
> 
> thanks,
> 
> --
> heikki

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年3月13日 17:36
> To: Rob Herring <robh@kernel.org>
> Cc: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> > On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > > ++++++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 0000000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +-----------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> >
> > Perhaps switch-gpios in case there are other gpios needed.
> >
> > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> >
> > Why is this needed? The compatible can't imply this?
> 
> I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c,
> so I'm to blame here. If we can handle this with the compatible like I guess we can,
> I'm happy.

Hi Heikki

Can I just remove the original bool property check? i.e, match OK if the remote
parent node is in switch_list.

Thanks
Li Jun
> 
> thanks,
> 
> --
> heikki

Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

On Mon, Mar 18, 2019 at 10:59:31AM +0000, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: 2019年3月13日 17:36
> > To: Rob Herring <robh@kernel.org>
> > Cc: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> > andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> > devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> > via GPIO
> > 
> > On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> > > On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > > > Some typec super speed active channel switch can be controlled via a
> > > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > > and the remote endpoint of its consumer.
> > > >
> > > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > > > ++++++++++++++++++++++
> > > >  1 file changed, 30 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > new file mode 100644
> > > > index 0000000..4ef76cf
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > @@ -0,0 +1,30 @@
> > > > +Typec orientation switch via a GPIO
> > > > +-----------------------------------
> > > > +
> > > > +Required properties:
> > > > +- compatible: should be set one of following:
> > > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > > +
> > > > +- gpios: the GPIO used to switch the super speed active channel,
> > >
> > > Perhaps switch-gpios in case there are other gpios needed.
> > >
> > > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > > +- orientation-switch: must be present.
> > >
> > > Why is this needed? The compatible can't imply this?
> > 
> > I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c,
> > so I'm to blame here. If we can handle this with the compatible like I guess we can,
> > I'm happy.
> 
> Hi Heikki
> 
> Can I just remove the original bool property check? i.e, match OK if the remote
> parent node is in switch_list.

No. If typec_switch_get() is called before the mux device is
registered, we need to return -EPROBE_DEFER. That means we need to be
able to identify the mux device node.

I think we should just use the compatible like Rob suggested. The
Type-C muxes should probable have their own bindings file where it's
defined for these muxes.


thanks,

-- 
heikki

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

On Mon, Mar 18, 2019 at 10:59:31AM +0000, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: 2019年3月13日 17:36
> > To: Rob Herring <robh@kernel.org>
> > Cc: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> > andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> > devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> > via GPIO
> > 
> > On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> > > On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > > > Some typec super speed active channel switch can be controlled via a
> > > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > > and the remote endpoint of its consumer.
> > > >
> > > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > > > ++++++++++++++++++++++
> > > >  1 file changed, 30 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > new file mode 100644
> > > > index 0000000..4ef76cf
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > @@ -0,0 +1,30 @@
> > > > +Typec orientation switch via a GPIO
> > > > +-----------------------------------
> > > > +
> > > > +Required properties:
> > > > +- compatible: should be set one of following:
> > > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > > +
> > > > +- gpios: the GPIO used to switch the super speed active channel,
> > >
> > > Perhaps switch-gpios in case there are other gpios needed.
> > >
> > > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > > +- orientation-switch: must be present.
> > >
> > > Why is this needed? The compatible can't imply this?
> > 
> > I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c,
> > so I'm to blame here. If we can handle this with the compatible like I guess we can,
> > I'm happy.
> 
> Hi Heikki
> 
> Can I just remove the original bool property check? i.e, match OK if the remote
> parent node is in switch_list.

No. If typec_switch_get() is called before the mux device is
registered, we need to return -EPROBE_DEFER. That means we need to be
able to identify the mux device node.

I think we should just use the compatible like Rob suggested. The
Type-C muxes should probable have their own bindings file where it's
defined for these muxes.


thanks,