[PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Li Jun]

Some platforms need a simple driver to do some controls according to
typec orientation, this can be extended to be a generic driver with
compatible with "typec-orientation-switch".

Signed-off-by: Li Jun <jun.li@nxp.com>
---
No changes for v5.

changes on v4:
- Use compatible instead of bool property for switch matching.
- Change switch GPIO to be switch simple.
- Change the active channel selection GPIO to be optional.

previous discussion:
http://patchwork.ozlabs.org/patch/1054342/

 .../bindings/usb/typec-switch-simple.yaml          | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
new file mode 100644
index 0000000..244162d
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/typec-switch-simple.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Typec Orientation Switch Simple Solution Bindings
+
+maintainers:
+  - Li Jun <jun.li@nxp.com>
+
+description: |-
+  USB SuperSpeed (SS) lanes routing to which side of typec connector is
+  decided by orientation, this maybe achieved by some simple control like
+  GPIO toggle.
+
+properties:
+  compatible:
+    const: typec-orientation-switch
+
+  switch-gpios:
+    description: |
+      gpio specifier to switch the super speed active channel,
+      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
+      GPIO_ACTIVE_LOW:  GPIO state low for cc1.
+    maxItems: 1
+
+  port:
+    type: object
+    additionalProperties: false
+    description: -|
+      Connection to the remote endpoint using OF graph bindings that model SS
+      data bus to typec connector.
+
+    properties:
+      endpoint:
+        type: object
+        additionalProperties: false
+
+        properties:
+          remote-endpoint: true
+
+        required:
+          - remote-endpoint
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    ptn36043 {
+        compatible = "typec-orientation-switch";
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_ss_sel>;
+        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
+
+        port {
+                usb3_data_ss: endpoint {
+                        remote-endpoint = <&typec_con_ss>;
+                };
+        };
+    };
-- 
2.7.4

[PATCH v5 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers

[Li Jun]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Since there are also some ACPI platforms where the
"compatible" property is used, introducing a generic helper
function fwnode_is_compatible() that can be used with
DT, ACPI and swnodes, and a wrapper function
device_is_compatible() with it.

The function calls of_device_is_comaptible() with OF nodes,
and with ACPI and swnodes it matches the given string
against the "compatible" string property array.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Li Jun <jun.li@nxp.com>
---
change for v5
- A minor change to move the function return valure description
  to a new paragraph.

New patch for v4.

 drivers/base/property.c  | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 41 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index d58aa98..d1c1f30 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1184,3 +1184,42 @@ const void *device_get_match_data(struct device *dev)
 	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
 }
 EXPORT_SYMBOL_GPL(device_get_match_data);
+
+/**
+ * fwnode_is_compatible - Check does fwnode have the given compatible string
+ * @fwnode: fwnode with the "compatible" property
+ * @compat: The compatible string
+ *
+ * Match the compatible strings of @fwnode against @compat.
+ *
+ * Returns positive value on match, and 0 when no matching compatible
+ * string is found.
+ */
+int fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compat)
+{
+	int ret;
+
+	if (is_of_node(fwnode))
+		return of_device_is_compatible(to_of_node(fwnode), compat);
+
+	ret = fwnode_property_match_string(fwnode, "compatible", compat);
+
+	return ret < 0 ? 0 : 1;
+}
+EXPORT_SYMBOL_GPL(fwnode_is_compatible);
+
+/**
+ * device_is_compatible - Check does a device have the given compatible string
+ * @dev: Device with the "compatible" property
+ * @compat: The compatible string
+ *
+ * Match the compatible strings of @dev against @compat.
+ *
+ * Returns positive value on match, and 0 when no matching compatible
+ * string is found.
+ */
+int device_is_compatible(struct device *dev, const char *compat)
+{
+	return fwnode_is_compatible(dev_fwnode(dev), compat);
+}
+EXPORT_SYMBOL_GPL(device_is_compatible);
diff --git a/include/linux/property.h b/include/linux/property.h
index 9f805c4..42c1d99 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -48,6 +48,7 @@ int device_property_read_string(struct device *dev, const char *propname,
 				const char **val);
 int device_property_match_string(struct device *dev,
 				 const char *propname, const char *string);
+int device_is_compatible(struct device *dev, const char *compat);
 
 bool fwnode_device_is_available(const struct fwnode_handle *fwnode);
 bool fwnode_property_present(const struct fwnode_handle *fwnode,
@@ -117,6 +118,7 @@ struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 int fwnode_irq_get(struct fwnode_handle *fwnode, unsigned int index);
+int fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compat);
 
 unsigned int device_get_child_node_count(struct device *dev);
 
-- 
2.7.4

[PATCH v5 3/4] usb: typec: mux: add "compatible" property for switch match

[Li Jun]

For those need a dedicated typec switch simple solution driver,
use compatible property for matching.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
No changes for v5
New patch for v4

 drivers/usb/typec/mux.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
index 52ad277..3da17d1 100644
--- a/drivers/usb/typec/mux.c
+++ b/drivers/usb/typec/mux.c
@@ -39,7 +39,8 @@ static void *typec_switch_match(struct device_connection *con, int ep,
 {
 	struct device *dev;
 
-	if (con->id && !fwnode_property_present(con->fwnode, con->id))
+	if (con->id && !fwnode_is_compatible(con->fwnode, con->id) &&
+		       !fwnode_property_present(con->fwnode, con->id))
 		return NULL;
 
 	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
@@ -61,8 +62,8 @@ struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode)
 {
 	struct typec_switch *sw;
 
-	sw = fwnode_connection_find_match(fwnode, "orientation-switch", NULL,
-					  typec_switch_match);
+	sw = fwnode_connection_find_match(fwnode, "typec-orientation-switch",
+					  NULL, typec_switch_match);
 	if (!IS_ERR_OR_NULL(sw))
 		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
 
-- 
2.7.4

Re: [PATCH v5 3/4] usb: typec: mux: add "compatible" property for switch match

[Heikki Krogerus]

On Tue, Nov 03, 2020 at 07:40:09PM +0800, Li Jun wrote:
> For those need a dedicated typec switch simple solution driver,
> use compatible property for matching.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> No changes for v5
> New patch for v4
> 
>  drivers/usb/typec/mux.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c
> index 52ad277..3da17d1 100644
> --- a/drivers/usb/typec/mux.c
> +++ b/drivers/usb/typec/mux.c
> @@ -39,7 +39,8 @@ static void *typec_switch_match(struct device_connection *con, int ep,
>  {
>  	struct device *dev;
>  
> -	if (con->id && !fwnode_property_present(con->fwnode, con->id))
> +	if (con->id && !fwnode_is_compatible(con->fwnode, con->id) &&
> +		       !fwnode_property_present(con->fwnode, con->id))
>  		return NULL;
>  
>  	dev = class_find_device(&typec_mux_class, NULL, con->fwnode,
> @@ -61,8 +62,8 @@ struct typec_switch *fwnode_typec_switch_get(struct fwnode_handle *fwnode)
>  {
>  	struct typec_switch *sw;
>  
> -	sw = fwnode_connection_find_match(fwnode, "orientation-switch", NULL,
> -					  typec_switch_match);
> +	sw = fwnode_connection_find_match(fwnode, "typec-orientation-switch",
> +					  NULL, typec_switch_match);
>  	if (!IS_ERR_OR_NULL(sw))
>  		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
>  
> -- 
> 2.7.4

thanks,

-- 
heikki

[PATCH v5 4/4] usb: typec: mux: add typec switch simple driver

[Li Jun]

This patch adds a simple typec switch driver for cases which only
needs some simple operations but a dedicated driver is required,
current driver only supports GPIO toggle to switch the super speed
active channel according to typec orientation.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
Changes for v5:
- A few changes address Andy's comment, remove gpio check as it's
  optional, add module name for Kconfig, use correct header files,
  and other minor changes.
- Remove the mutex lock as it's not required currently.

Changes for v4:
- Change driver name to be switch simple from switch GPIO, to make it
  generic for possible extention.
- Use compatiable "typec-orientation-switch" instead of bool property
  for switch matching.
- Make acitve channel selection GPIO to be optional.
- Remove Andy's R-b tag since the driver changes a lot.

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         |  10 ++++
 drivers/usb/typec/mux/Makefile        |   1 +
 drivers/usb/typec/mux/switch-simple.c | 100 ++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index a4dbd11..11320d7 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -18,4 +18,14 @@ config TYPEC_MUX_INTEL_PMC
 	  control the USB role switch and also the multiplexer/demultiplexer
 	  switches used with USB Type-C Alternate Modes.
 
+config TYPEC_SWITCH_SIMPLE
+	tristate "Type-C orientation switch simple driver"
+	depends on GPIOLIB
+	help
+	  Say Y or M if your system need a simple driver for typec switch
+	  control, like use GPIO to select active channel.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called switch-simple.
+
 endmenu
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index 280a6f5..712d0ad 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
 obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
+obj-$(CONFIG_TYPEC_SWITCH_SIMPLE)	+= switch-simple.o
diff --git a/drivers/usb/typec/mux/switch-simple.c b/drivers/usb/typec/mux/switch-simple.c
new file mode 100644
index 0000000..8707703
--- /dev/null
+++ b/drivers/usb/typec/mux/switch-simple.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Type-C switch simple control driver
+ *
+ * Copyright 2020 NXP
+ * Author: Jun Li <jun.li@nxp.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/usb/typec_mux.h>
+
+struct typec_switch_simple {
+	struct typec_switch *sw;
+	struct gpio_desc *sel_gpio;
+};
+
+static int typec_switch_simple_set(struct typec_switch *sw,
+				   enum typec_orientation orientation)
+{
+	struct typec_switch_simple *typec_sw = typec_switch_get_drvdata(sw);
+
+	switch (orientation) {
+	case TYPEC_ORIENTATION_NORMAL:
+		gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
+		break;
+	case TYPEC_ORIENTATION_NONE:
+		break;
+	}
+
+	return 0;
+}
+
+static int typec_switch_simple_probe(struct platform_device *pdev)
+{
+	struct device			*dev = &pdev->dev;
+	struct typec_switch_desc	sw_desc;
+	struct typec_switch_simple	*typec_sw;
+
+	typec_sw = devm_kzalloc(dev, sizeof(*typec_sw), GFP_KERNEL);
+	if (!typec_sw)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, typec_sw);
+
+	sw_desc.drvdata = typec_sw;
+	sw_desc.fwnode = dev->fwnode;
+	sw_desc.set = typec_switch_simple_set;
+
+	/* Get the super speed active channel selection GPIO */
+	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch", GPIOD_OUT_LOW);
+	if (IS_ERR(typec_sw->sel_gpio))
+		return PTR_ERR(typec_sw->sel_gpio);
+
+	typec_sw->sw = typec_switch_register(dev, &sw_desc);
+	if (IS_ERR(typec_sw->sw)) {
+		dev_err(dev, "Error registering typec switch: %ld\n",
+			PTR_ERR(typec_sw->sw));
+		return PTR_ERR(typec_sw->sw);
+	}
+
+	return 0;
+}
+
+static int typec_switch_simple_remove(struct platform_device *pdev)
+{
+	struct typec_switch_simple *typec_sw = platform_get_drvdata(pdev);
+
+	typec_switch_unregister(typec_sw->sw);
+
+	return 0;
+}
+
+static const struct of_device_id of_typec_switch_simple_match[] = {
+	{ .compatible = "typec-orientation-switch" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_typec_switch_simple_match);
+
+static struct platform_driver typec_switch_simple_driver = {
+	.probe		= typec_switch_simple_probe,
+	.remove		= typec_switch_simple_remove,
+	.driver		= {
+		.name	= "typec-switch-simple",
+		.of_match_table = of_typec_switch_simple_match,
+	},
+};
+
+module_platform_driver(typec_switch_simple_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TypeC Orientation Switch Simple driver");
+MODULE_AUTHOR("Jun Li <jun.li@nxp.com>");
-- 
2.7.4

Re: [PATCH v5 4/4] usb: typec: mux: add typec switch simple driver

[Heikki Krogerus]

On Tue, Nov 03, 2020 at 07:40:10PM +0800, Li Jun wrote:
> This patch adds a simple typec switch driver for cases which only
> needs some simple operations but a dedicated driver is required,
> current driver only supports GPIO toggle to switch the super speed
> active channel according to typec orientation.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Changes for v5:
> - A few changes address Andy's comment, remove gpio check as it's
>   optional, add module name for Kconfig, use correct header files,
>   and other minor changes.
> - Remove the mutex lock as it's not required currently.
> 
> Changes for v4:
> - Change driver name to be switch simple from switch GPIO, to make it
>   generic for possible extention.
> - Use compatiable "typec-orientation-switch" instead of bool property
>   for switch matching.
> - Make acitve channel selection GPIO to be optional.
> - Remove Andy's R-b tag since the driver changes a lot.
> 
> 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         |  10 ++++
>  drivers/usb/typec/mux/Makefile        |   1 +
>  drivers/usb/typec/mux/switch-simple.c | 100 ++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index a4dbd11..11320d7 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -18,4 +18,14 @@ config TYPEC_MUX_INTEL_PMC
>  	  control the USB role switch and also the multiplexer/demultiplexer
>  	  switches used with USB Type-C Alternate Modes.
>  
> +config TYPEC_SWITCH_SIMPLE
> +	tristate "Type-C orientation switch simple driver"
> +	depends on GPIOLIB
> +	help
> +	  Say Y or M if your system need a simple driver for typec switch
> +	  control, like use GPIO to select active channel.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called switch-simple.
> +
>  endmenu
> diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
> index 280a6f5..712d0ad 100644
> --- a/drivers/usb/typec/mux/Makefile
> +++ b/drivers/usb/typec/mux/Makefile
> @@ -2,3 +2,4 @@
>  
>  obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
>  obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
> +obj-$(CONFIG_TYPEC_SWITCH_SIMPLE)	+= switch-simple.o
> diff --git a/drivers/usb/typec/mux/switch-simple.c b/drivers/usb/typec/mux/switch-simple.c
> new file mode 100644
> index 0000000..8707703
> --- /dev/null
> +++ b/drivers/usb/typec/mux/switch-simple.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Type-C switch simple control driver
> + *
> + * Copyright 2020 NXP
> + * Author: Jun Li <jun.li@nxp.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/typec_mux.h>
> +
> +struct typec_switch_simple {
> +	struct typec_switch *sw;
> +	struct gpio_desc *sel_gpio;
> +};
> +
> +static int typec_switch_simple_set(struct typec_switch *sw,
> +				   enum typec_orientation orientation)
> +{
> +	struct typec_switch_simple *typec_sw = typec_switch_get_drvdata(sw);
> +
> +	switch (orientation) {
> +	case TYPEC_ORIENTATION_NORMAL:
> +		gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> +		break;
> +	case TYPEC_ORIENTATION_NONE:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int typec_switch_simple_probe(struct platform_device *pdev)
> +{
> +	struct device			*dev = &pdev->dev;
> +	struct typec_switch_desc	sw_desc;
> +	struct typec_switch_simple	*typec_sw;
> +
> +	typec_sw = devm_kzalloc(dev, sizeof(*typec_sw), GFP_KERNEL);
> +	if (!typec_sw)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, typec_sw);
> +
> +	sw_desc.drvdata = typec_sw;
> +	sw_desc.fwnode = dev->fwnode;
> +	sw_desc.set = typec_switch_simple_set;
> +
> +	/* Get the super speed active channel selection GPIO */
> +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch", GPIOD_OUT_LOW);
> +	if (IS_ERR(typec_sw->sel_gpio))
> +		return PTR_ERR(typec_sw->sel_gpio);
> +
> +	typec_sw->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(typec_sw->sw)) {
> +		dev_err(dev, "Error registering typec switch: %ld\n",
> +			PTR_ERR(typec_sw->sw));
> +		return PTR_ERR(typec_sw->sw);
> +	}
> +
> +	return 0;
> +}
> +
> +static int typec_switch_simple_remove(struct platform_device *pdev)
> +{
> +	struct typec_switch_simple *typec_sw = platform_get_drvdata(pdev);
> +
> +	typec_switch_unregister(typec_sw->sw);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_typec_switch_simple_match[] = {
> +	{ .compatible = "typec-orientation-switch" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_typec_switch_simple_match);
> +
> +static struct platform_driver typec_switch_simple_driver = {
> +	.probe		= typec_switch_simple_probe,
> +	.remove		= typec_switch_simple_remove,
> +	.driver		= {
> +		.name	= "typec-switch-simple",
> +		.of_match_table = of_typec_switch_simple_match,
> +	},
> +};
> +
> +module_platform_driver(typec_switch_simple_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("TypeC Orientation Switch Simple driver");
> +MODULE_AUTHOR("Jun Li <jun.li@nxp.com>");
> -- 
> 2.7.4

thanks,

-- 
heikki

RE: [PATCH v5 4/4] usb: typec: mux: add typec switch simple driver

[Jun Li]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Monday, November 9, 2020 4:36 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: robh+dt@kernel.org; rafael@kernel.org; gregkh@linuxfoundation.org;
> andriy.shevchenko@linux.intel.com; hdegoede@redhat.com;
> lee.jones@linaro.org; mika.westerberg@linux.intel.com;
> dmitry.torokhov@gmail.com; prabhakar.mahadev-lad.rj@bp.renesas.com;
> laurent.pinchart+renesas@ideasonboard.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>
> Subject: Re: [PATCH v5 4/4] usb: typec: mux: add typec switch simple driver
> 
> On Tue, Nov 03, 2020 at 07:40:10PM +0800, Li Jun wrote:
> > This patch adds a simple typec switch driver for cases which only
> > needs some simple operations but a dedicated driver is required,
> > current driver only supports GPIO toggle to switch the super speed
> > active channel according to typec orientation.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> 
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Hi, Heikki,
 
I have to drop your A-b tag as the driver updated to use mux bindings
(drivers/mux/), see v6:

https://patchwork.kernel.org/project/linux-usb/patch/1606140096-1382-6-git-send-email-jun.li@nxp.com/

thanks
Li Jun

Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Rob Herring]

On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote:
> Some platforms need a simple driver to do some controls according to
> typec orientation, this can be extended to be a generic driver with
> compatible with "typec-orientation-switch".
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
> No changes for v5.
> 
> changes on v4:
> - Use compatible instead of bool property for switch matching.
> - Change switch GPIO to be switch simple.
> - Change the active channel selection GPIO to be optional.
> 
> previous discussion:
> http://patchwork.ozlabs.org/patch/1054342/
> 
>  .../bindings/usb/typec-switch-simple.yaml          | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> new file mode 100644
> index 0000000..244162d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/typec-switch-simple.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Typec Orientation Switch Simple Solution Bindings
> +
> +maintainers:
> +  - Li Jun <jun.li@nxp.com>
> +
> +description: |-
> +  USB SuperSpeed (SS) lanes routing to which side of typec connector is
> +  decided by orientation, this maybe achieved by some simple control like
> +  GPIO toggle.
> +
> +properties:
> +  compatible:
> +    const: typec-orientation-switch
> +
> +  switch-gpios:
> +    description: |
> +      gpio specifier to switch the super speed active channel,
> +      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +      GPIO_ACTIVE_LOW:  GPIO state low for cc1.

What does active mean? There isn't really an active and inactive state, 
right? It's more a mux selecting 0 or 1 input?

I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an 
inverter in the middle.

> +    maxItems: 1
> +
> +  port:
> +    type: object
> +    additionalProperties: false
> +    description: -|
> +      Connection to the remote endpoint using OF graph bindings that model SS
> +      data bus to typec connector.
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        additionalProperties: false
> +
> +        properties:
> +          remote-endpoint: true
> +
> +        required:
> +          - remote-endpoint
> +
> +    required:
> +      - endpoint
> +
> +required:
> +  - compatible
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    ptn36043 {
> +        compatible = "typec-orientation-switch";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_ss_sel>;
> +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +
> +        port {
> +                usb3_data_ss: endpoint {
> +                        remote-endpoint = <&typec_con_ss>;

The data goes from the connector to here and then where? You need a 
connection to the USB host controller.

> +                };
> +        };
> +    };
> -- 
> 2.7.4
> 

RE: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Jun Li]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, November 6, 2020 6:26 AM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; rafael@kernel.org;
> gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> hdegoede@redhat.com; lee.jones@linaro.org;
> mika.westerberg@linux.intel.com; dmitry.torokhov@gmail.com;
> prabhakar.mahadev-lad.rj@bp.renesas.com;
> laurent.pinchart+renesas@ideasonboard.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>
> Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec
> switch simple driver
> 
> On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote:
> > Some platforms need a simple driver to do some controls according to
> > typec orientation, this can be extended to be a generic driver with
> > compatible with "typec-orientation-switch".
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> > No changes for v5.
> >
> > changes on v4:
> > - Use compatible instead of bool property for switch matching.
> > - Change switch GPIO to be switch simple.
> > - Change the active channel selection GPIO to be optional.
> >
> > previous discussion:
> >
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> work.ozlabs.org%2Fpatch%2F1054342%2F&amp;data=04%7C01%7Cjun.li%40nxp.c
> >
> om%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c5c3016
> >
> 35%7C0%7C0%7C637402119664101856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> >
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=
> > 8TY%2BPRIui6HhxhYE1%2BLmwWL38Vp7SY1Ceb5rGG%2B4DUo%3D&amp;reserved=0
> >
> >  .../bindings/usb/typec-switch-simple.yaml          | 69
> ++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > new file mode 100644
> > index 0000000..244162d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> >
> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fusb%2Ftypec-switch-simple.yaml%23&amp;data=04%
> >
> +7C01%7Cjun.li%40nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3
> >
> +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWF
> >
> +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> >
> +Mn0%3D%7C1000&amp;sdata=HjWfKlDLyqb%2FKLlL6vdnyPe%2BnB8pSllhokIXQ%2Bw
> > +yyw8%3D&amp;reserved=0
> > +$schema:
> >
> +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cjun.li%40
> >
> +nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c
> >
> +5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWFpbGZsb3d8eyJWIjo
> >
> +iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&am
> >
> +p;sdata=z0bO47QVl5gw0UE%2Bx3a5E27ALgz568zp%2Bf4suGlch%2Fo%3D&amp;rese
> > +rved=0
> > +
> > +title: Typec Orientation Switch Simple Solution Bindings
> > +
> > +maintainers:
> > +  - Li Jun <jun.li@nxp.com>
> > +
> > +description: |-
> > +  USB SuperSpeed (SS) lanes routing to which side of typec connector
> > +is
> > +  decided by orientation, this maybe achieved by some simple control
> > +like
> > +  GPIO toggle.
> > +
> > +properties:
> > +  compatible:
> > +    const: typec-orientation-switch
> > +
> > +  switch-gpios:
> > +    description: |
> > +      gpio specifier to switch the super speed active channel,
> > +      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +      GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> 
> What does active mean? There isn't really an active and inactive state, right?
> It's more a mux selecting 0 or 1 input?

Yes, I will change the description:
gpio specifier to select the target channel of mux.

> 
> I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an inverter
> in the middle.

This depends on the switch IC design and board design, leave 2 flags
(GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases.

NXP has 2 diff IC parts for this:
1. PTN36043(used on iMX8MQ)
Output selection control
When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and
RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor.
When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and
RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor.

Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1,
so GPIO_ACTIVE_HIGH

2. CBTU02043(used on iMX8MP)
SEL        Function
--------------------------------------
Low        A to B ports and vice versa
High       A to C ports and vice versa

Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW

Therefore, we need 2 flags.

> 
> > +    maxItems: 1
> > +
> > +  port:
> > +    type: object
> > +    additionalProperties: false
> > +    description: -|
> > +      Connection to the remote endpoint using OF graph bindings that model
> SS
> > +      data bus to typec connector.
> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
> > +        additionalProperties: false
> > +
> > +        properties:
> > +          remote-endpoint: true
> > +
> > +        required:
> > +          - remote-endpoint
> > +
> > +    required:
> > +      - endpoint
> > +
> > +required:
> > +  - compatible
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    ptn36043 {
> > +        compatible = "typec-orientation-switch";
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&pinctrl_ss_sel>;
> > +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +
> > +        port {
> > +                usb3_data_ss: endpoint {
> > +                        remote-endpoint = <&typec_con_ss>;
> 
> The data goes from the connector to here and then where? You need a connection
> to the USB host controller.

The orientation switch only need interact with type-c, no any interaction
with USB controller, do we still need a connection to it?

Thanks
Li Jun
> 
> > +                };
> > +        };
> > +    };
> > --
> > 2.7.4
> >

Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Rob Herring]

On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Friday, November 6, 2020 6:26 AM
> > To: Jun Li <jun.li@nxp.com>
> > Cc: heikki.krogerus@linux.intel.com; rafael@kernel.org;
> > gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> > hdegoede@redhat.com; lee.jones@linaro.org;
> > mika.westerberg@linux.intel.com; dmitry.torokhov@gmail.com;
> > prabhakar.mahadev-lad.rj@bp.renesas.com;
> > laurent.pinchart+renesas@ideasonboard.com; linux-usb@vger.kernel.org;
> > devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> > <peter.chen@nxp.com>
> > Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec
> > switch simple driver
> >
> > On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote:
> > > Some platforms need a simple driver to do some controls according to
> > > typec orientation, this can be extended to be a generic driver with
> > > compatible with "typec-orientation-switch".
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > > No changes for v5.
> > >
> > > changes on v4:
> > > - Use compatible instead of bool property for switch matching.
> > > - Change switch GPIO to be switch simple.
> > > - Change the active channel selection GPIO to be optional.
> > >
> > > previous discussion:
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> > >
> > work.ozlabs.org%2Fpatch%2F1054342%2F&amp;data=04%7C01%7Cjun.li%40nxp.c
> > >
> > om%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > >
> > 35%7C0%7C0%7C637402119664101856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> > >
> > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=
> > > 8TY%2BPRIui6HhxhYE1%2BLmwWL38Vp7SY1Ceb5rGG%2B4DUo%3D&amp;reserved=0
> > >
> > >  .../bindings/usb/typec-switch-simple.yaml          | 69
> > ++++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > > b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > > new file mode 100644
> > > index 0000000..244162d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id:
> > >
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > >
> > +cetree.org%2Fschemas%2Fusb%2Ftypec-switch-simple.yaml%23&amp;data=04%
> > >
> > +7C01%7Cjun.li%40nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3
> > >
> > +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWF
> > >
> > +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > >
> > +Mn0%3D%7C1000&amp;sdata=HjWfKlDLyqb%2FKLlL6vdnyPe%2BnB8pSllhokIXQ%2Bw
> > > +yyw8%3D&amp;reserved=0
> > > +$schema:
> > >
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > >
> > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cjun.li%40
> > >
> > +nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c
> > >
> > +5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWFpbGZsb3d8eyJWIjo
> > >
> > +iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&am
> > >
> > +p;sdata=z0bO47QVl5gw0UE%2Bx3a5E27ALgz568zp%2Bf4suGlch%2Fo%3D&amp;rese
> > > +rved=0
> > > +
> > > +title: Typec Orientation Switch Simple Solution Bindings
> > > +
> > > +maintainers:
> > > +  - Li Jun <jun.li@nxp.com>
> > > +
> > > +description: |-
> > > +  USB SuperSpeed (SS) lanes routing to which side of typec connector
> > > +is
> > > +  decided by orientation, this maybe achieved by some simple control
> > > +like
> > > +  GPIO toggle.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: typec-orientation-switch
> > > +
> > > +  switch-gpios:
> > > +    description: |
> > > +      gpio specifier to switch the super speed active channel,
> > > +      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +      GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> >
> > What does active mean? There isn't really an active and inactive state, right?
> > It's more a mux selecting 0 or 1 input?
>
> Yes, I will change the description:
> gpio specifier to select the target channel of mux.

I wonder if the existing mux bindings should be used here.

> > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an inverter
> > in the middle.
>
> This depends on the switch IC design and board design, leave 2 flags
> (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases.
>
> NXP has 2 diff IC parts for this:
> 1. PTN36043(used on iMX8MQ)
> Output selection control
> When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and
> RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor.
> When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and
> RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor.
>
> Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1,
> so GPIO_ACTIVE_HIGH
>
> 2. CBTU02043(used on iMX8MP)
> SEL        Function
> --------------------------------------
> Low        A to B ports and vice versa
> High       A to C ports and vice versa
>
> Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW
>
> Therefore, we need 2 flags.

I'm not saying you don't. Just that the description is a bit odd.
Please expand the description for how one decides how to set the
flags.

>
> >
> > > +    maxItems: 1
> > > +
> > > +  port:
> > > +    type: object
> > > +    additionalProperties: false
> > > +    description: -|
> > > +      Connection to the remote endpoint using OF graph bindings that model
> > SS
> > > +      data bus to typec connector.
> > > +
> > > +    properties:
> > > +      endpoint:
> > > +        type: object
> > > +        additionalProperties: false
> > > +
> > > +        properties:
> > > +          remote-endpoint: true
> > > +
> > > +        required:
> > > +          - remote-endpoint
> > > +
> > > +    required:
> > > +      - endpoint
> > > +
> > > +required:
> > > +  - compatible
> > > +  - port
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    ptn36043 {
> > > +        compatible = "typec-orientation-switch";
> > > +        pinctrl-names = "default";
> > > +        pinctrl-0 = <&pinctrl_ss_sel>;
> > > +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > +
> > > +        port {
> > > +                usb3_data_ss: endpoint {
> > > +                        remote-endpoint = <&typec_con_ss>;
> >
> > The data goes from the connector to here and then where? You need a connection
> > to the USB host controller.
>
> The orientation switch only need interact with type-c, no any interaction
> with USB controller, do we still need a connection to it?

If you have 2 USB hosts and 2 connectors (and 2 muxes), how would you
describe which connector goes with which host?

Rob

RE: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Jun Li]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, November 6, 2020 10:24 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; rafael@kernel.org;
> gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> hdegoede@redhat.com; lee.jones@linaro.org;
> mika.westerberg@linux.intel.com; dmitry.torokhov@gmail.com;
> prabhakar.mahadev-lad.rj@bp.renesas.com;
> laurent.pinchart+renesas@ideasonboard.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>
> Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec
> switch simple driver
> 
> On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Friday, November 6, 2020 6:26 AM
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: heikki.krogerus@linux.intel.com; rafael@kernel.org;
> > > gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> > > hdegoede@redhat.com; lee.jones@linaro.org;
> > > mika.westerberg@linux.intel.com; dmitry.torokhov@gmail.com;
> > > prabhakar.mahadev-lad.rj@bp.renesas.com;
> > > laurent.pinchart+renesas@ideasonboard.com;
> > > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> > > <linux-imx@nxp.com>; Peter Chen <peter.chen@nxp.com>
> > > Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for
> > > typec switch simple driver
> > >
> > > On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote:
> > > > Some platforms need a simple driver to do some controls according
> > > > to typec orientation, this can be extended to be a generic driver
> > > > with compatible with "typec-orientation-switch".
> > > >
> > > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > > ---
> > > > No changes for v5.
> > > >
> > > > changes on v4:
> > > > - Use compatible instead of bool property for switch matching.
> > > > - Change switch GPIO to be switch simple.
> > > > - Change the active channel selection GPIO to be optional.
> > > >
> > > > previous discussion:
> > > >
> > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpat
> > > ch
> > > >
> > > work.ozlabs.org%2Fpatch%2F1054342%2F&amp;data=04%7C01%7Cjun.li%40nxp
> > > .c
> > > >
> > > om%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c5c30
> > > 16
> > > >
> > > 35%7C0%7C0%7C637402119664101856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> > > Lj
> > > >
> > > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdat
> > > a=
> > > > 8TY%2BPRIui6HhxhYE1%2BLmwWL38Vp7SY1Ceb5rGG%2B4DUo%3D&amp;reserved=
> > > > 0
> > > >
> > > >  .../bindings/usb/typec-switch-simple.yaml          | 69
> > > ++++++++++++++++++++++
> > > >  1 file changed, 69 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > > > b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml
> > > > new file mode 100644
> > > > index 0000000..244162d
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.ya
> > > > +++ ml
> > > > @@ -0,0 +1,69 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML
> > > > +1.2
> > > > +---
> > > > +$id:
> > > >
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > >
> > > +cetree.org%2Fschemas%2Fusb%2Ftypec-switch-simple.yaml%23&amp;data=0
> > > +4%
> > > >
> > > +7C01%7Cjun.li%40nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1
> > > +d3
> > > >
> > > +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CT
> > > +WF
> > > >
> > > +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> > > +I6
> > > >
> > > +Mn0%3D%7C1000&amp;sdata=HjWfKlDLyqb%2FKLlL6vdnyPe%2BnB8pSllhokIXQ%2
> > > +Bw
> > > > +yyw8%3D&amp;reserved=0
> > > > +$schema:
> > > >
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > >
> > > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cjun.li%
> > > +40
> > > >
> > > +nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd9
> > > +9c
> > > >
> > > +5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWFpbGZsb3d8eyJWI
> > > +jo
> > > >
> > > +iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> > > +am
> > > >
> > > +p;sdata=z0bO47QVl5gw0UE%2Bx3a5E27ALgz568zp%2Bf4suGlch%2Fo%3D&amp;re
> > > +se
> > > > +rved=0
> > > > +
> > > > +title: Typec Orientation Switch Simple Solution Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Li Jun <jun.li@nxp.com>
> > > > +
> > > > +description: |-
> > > > +  USB SuperSpeed (SS) lanes routing to which side of typec
> > > > +connector is
> > > > +  decided by orientation, this maybe achieved by some simple
> > > > +control like
> > > > +  GPIO toggle.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: typec-orientation-switch
> > > > +
> > > > +  switch-gpios:
> > > > +    description: |
> > > > +      gpio specifier to switch the super speed active channel,
> > > > +      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > > +      GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > >
> > > What does active mean? There isn't really an active and inactive state,
> right?
> > > It's more a mux selecting 0 or 1 input?
> >
> > Yes, I will change the description:
> > gpio specifier to select the target channel of mux.
> 
> I wonder if the existing mux bindings should be used here.

If only consider typec switch via "gpio", existing "mux-gpio"
binding may be used with property "mux-control-names" to be
"typec-xxx" for match, but we still need create typec stuff at
mux driver to hook to typec system, so little benefit, considering
this binding is going to be for a generic typec orientation switch
simple driver, I think a new typec binding make sense.  

> 
> > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an
> > > inverter in the middle.
> >
> > This depends on the switch IC design and board design, leave 2 flags
> > (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases.
> >
> > NXP has 2 diff IC parts for this:
> > 1. PTN36043(used on iMX8MQ)
> > Output selection control
> > When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and
> > RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor.
> > When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and
> > RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor.
> >
> > Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1, so
> > GPIO_ACTIVE_HIGH
> >
> > 2. CBTU02043(used on iMX8MP)
> > SEL        Function
> > --------------------------------------
> > Low        A to B ports and vice versa
> > High       A to C ports and vice versa
> >
> > Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW
> >
> > Therefore, we need 2 flags.
> 
> I'm not saying you don't. Just that the description is a bit odd.
> Please expand the description for how one decides how to set the flags.

Misunderstood your point, OK, I thought the "how to set the flags" was
simple and clear enough:
Use GPIO_ACTIVE_HIGH if GPIO physical state high is for cc1; or
Use GPIO_ACTIVE_LOW if GPIO physical state low is for cc1.

> 
> >
> > >
> > > > +    maxItems: 1
> > > > +
> > > > +  port:
> > > > +    type: object
> > > > +    additionalProperties: false
> > > > +    description: -|
> > > > +      Connection to the remote endpoint using OF graph bindings
> > > > + that model
> > > SS
> > > > +      data bus to typec connector.
> > > > +
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +        additionalProperties: false
> > > > +
> > > > +        properties:
> > > > +          remote-endpoint: true
> > > > +
> > > > +        required:
> > > > +          - remote-endpoint
> > > > +
> > > > +    required:
> > > > +      - endpoint
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - port
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > +    ptn36043 {
> > > > +        compatible = "typec-orientation-switch";
> > > > +        pinctrl-names = "default";
> > > > +        pinctrl-0 = <&pinctrl_ss_sel>;
> > > > +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > > +
> > > > +        port {
> > > > +                usb3_data_ss: endpoint {
> > > > +                        remote-endpoint = <&typec_con_ss>;
> > >
> > > The data goes from the connector to here and then where? You need a
> > > connection to the USB host controller.
> >
> > The orientation switch only need interact with type-c, no any
> > interaction with USB controller, do we still need a connection to it?
> 
> If you have 2 USB hosts and 2 connectors (and 2 muxes), how would you describe
> which connector goes with which host?

One instance of typec orientation switch defined by this binding only for
One typec connector. With that, my understanding is
Whether a connection need be described depends on if the connector
(typec driver) need notify the host controller driver to do something
(e.g. role switch need a connection between controller node and connector
node for controller driver to swap usb role). If the mux/switch control is
transparent to usb host controller (e.g. my case, usb controller drivers
normally don't need do anything for orientation change), there is no need
to describe connection between orientation switch node and host controller
node.

Thanks
Li Jun
> 
> Rob

Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Rob Herring]

On Mon, Nov 9, 2020 at 6:24 AM Jun Li <jun.li@nxp.com> wrote:
> > From: Rob Herring <robh@kernel.org>
> > On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote:
> > > > From: Rob Herring <robh@kernel.org>

> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: typec-orientation-switch
> > > > > +
> > > > > +  switch-gpios:
> > > > > +    description: |
> > > > > +      gpio specifier to switch the super speed active channel,
> > > > > +      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > > > +      GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > >
> > > > What does active mean? There isn't really an active and inactive state,
> > right?
> > > > It's more a mux selecting 0 or 1 input?
> > >
> > > Yes, I will change the description:
> > > gpio specifier to select the target channel of mux.
> >
> > I wonder if the existing mux bindings should be used here.
>
> If only consider typec switch via "gpio", existing "mux-gpio"
> binding may be used with property "mux-control-names" to be
> "typec-xxx" for match, but we still need create typec stuff at
> mux driver to hook to typec system, so little benefit, considering
> this binding is going to be for a generic typec orientation switch
> simple driver, I think a new typec binding make sense.

You can instantiate drivers without a compatible. That's just the easy
way. However, using the mux binding doesn't necessarily mean you have
to use 'mux-gpio'. Consider if you need to do more control than just
the GPIO line. For example, the chips you mentioned may have a s/w
controlled power supply or reset.

Also, consider what the mux would look like with different control
interfaces. That could be I2C or some sub-block in a PMIC or ??? I'm
sure we already have some examples. I'm not happy with these piecemeal
additions to TypeC related bindings that don't consider more than 1
h/w possibility.

> > > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an
> > > > inverter in the middle.
> > >
> > > This depends on the switch IC design and board design, leave 2 flags
> > > (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases.
> > >
> > > NXP has 2 diff IC parts for this:
> > > 1. PTN36043(used on iMX8MQ)
> > > Output selection control
> > > When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and
> > > RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor.
> > > When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and
> > > RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor.
> > >
> > > Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1, so
> > > GPIO_ACTIVE_HIGH
> > >
> > > 2. CBTU02043(used on iMX8MP)
> > > SEL        Function
> > > --------------------------------------
> > > Low        A to B ports and vice versa
> > > High       A to C ports and vice versa
> > >
> > > Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW
> > >
> > > Therefore, we need 2 flags.
> >
> > I'm not saying you don't. Just that the description is a bit odd.
> > Please expand the description for how one decides how to set the flags.
>
> Misunderstood your point, OK, I thought the "how to set the flags" was
> simple and clear enough:
> Use GPIO_ACTIVE_HIGH if GPIO physical state high is for cc1; or
> Use GPIO_ACTIVE_LOW if GPIO physical state low is for cc1.

Okay.

> > > > > +examples:
> > > > > +  - |
> > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > +    ptn36043 {
> > > > > +        compatible = "typec-orientation-switch";
> > > > > +        pinctrl-names = "default";
> > > > > +        pinctrl-0 = <&pinctrl_ss_sel>;
> > > > > +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > > > +
> > > > > +        port {
> > > > > +                usb3_data_ss: endpoint {
> > > > > +                        remote-endpoint = <&typec_con_ss>;
> > > >
> > > > The data goes from the connector to here and then where? You need a
> > > > connection to the USB host controller.
> > >
> > > The orientation switch only need interact with type-c, no any
> > > interaction with USB controller, do we still need a connection to it?
> >
> > If you have 2 USB hosts and 2 connectors (and 2 muxes), how would you describe
> > which connector goes with which host?
>
> One instance of typec orientation switch defined by this binding only for
> One typec connector. With that, my understanding is
> Whether a connection need be described depends on if the connector
> (typec driver) need notify the host controller driver to do something
> (e.g. role switch need a connection between controller node and connector
> node for controller driver to swap usb role). If the mux/switch control is
> transparent to usb host controller (e.g. my case, usb controller drivers
> normally don't need do anything for orientation change), there is no need
> to describe connection between orientation switch node and host controller
> node.

There can be several reasons you need to know the association. When
writing the DT you can't assume what information is or isn't needed.
That may vary by h/w or can evolve in an OS and the DT shouldn't
change.

Rob

RE: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec switch simple driver

[Jun Li]

Hi Rob

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Monday, November 9, 2020 10:38 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; rafael@kernel.org;
> gregkh@linuxfoundation.org; andriy.shevchenko@linux.intel.com;
> hdegoede@redhat.com; lee.jones@linaro.org;
> mika.westerberg@linux.intel.com; dmitry.torokhov@gmail.com;
> prabhakar.mahadev-lad.rj@bp.renesas.com;
> laurent.pinchart+renesas@ideasonboard.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter Chen
> <peter.chen@nxp.com>
> Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec
> switch simple driver
> 
> On Mon, Nov 9, 2020 at 6:24 AM Jun Li <jun.li@nxp.com> wrote:
> > > From: Rob Herring <robh@kernel.org>
> > > On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote:
> > > > > From: Rob Herring <robh@kernel.org>
> 
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    const: typec-orientation-switch
> > > > > > +
> > > > > > +  switch-gpios:
> > > > > > +    description: |
> > > > > > +      gpio specifier to switch the super speed active channel,
> > > > > > +      GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > > > > +      GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > > >
> > > > > What does active mean? There isn't really an active and inactive
> > > > > state,
> > > right?
> > > > > It's more a mux selecting 0 or 1 input?
> > > >
> > > > Yes, I will change the description:
> > > > gpio specifier to select the target channel of mux.
> > >
> > > I wonder if the existing mux bindings should be used here.
> >
> > If only consider typec switch via "gpio", existing "mux-gpio"
> > binding may be used with property "mux-control-names" to be
> > "typec-xxx" for match, but we still need create typec stuff at mux
> > driver to hook to typec system, so little benefit, considering this
> > binding is going to be for a generic typec orientation switch simple
> > driver, I think a new typec binding make sense.
> 
> You can instantiate drivers without a compatible. That's just the easy way.
> However, using the mux binding doesn't necessarily mean you have to use
> 'mux-gpio'. Consider if you need to do more control than just the GPIO line.
> For example, the chips you mentioned may have a s/w controlled power supply
> or reset.
> 
> Also, consider what the mux would look like with different control interfaces.
> That could be I2C or some sub-block in a PMIC or ??? I'm sure we already
> have some examples. I'm not happy with these piecemeal additions to TypeC
> related bindings that don't consider more than 1 h/w possibility.

As typec class sub system already has its own interface(and users)
to do switch/mux control, using existing mux bindings means typec class will
add switch/mux control through another approach(mux chip/controller interface),
this makes the typec mux looks having 2 similar way for the same function.
so I was hesitating to use it.

After more check, I agree creating typec specific bindings will duplicate
much existing mux bindings, the mux controller based bindings can be a good
way used for various typec switch/mux, so I will try typec orientation
compatible with mux controller bindings.

> 
> > > > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's
> > > > > an inverter in the middle.
> > > >
> > > > This depends on the switch IC design and board design, leave 2
> > > > flags (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible
> cases.
> > > >
> > > > NXP has 2 diff IC parts for this:
> > > > 1. PTN36043(used on iMX8MQ)
> > > > Output selection control
> > > > When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*,
> > > > and
> > > > RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor.
> > > > When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*,
> > > > and
> > > > RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor.
> > > >
> > > > Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1,
> > > > so GPIO_ACTIVE_HIGH
> > > >
> > > > 2. CBTU02043(used on iMX8MP)
> > > > SEL        Function
> > > > --------------------------------------
> > > > Low        A to B ports and vice versa
> > > > High       A to C ports and vice versa
> > > >
> > > > Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW
> > > >
> > > > Therefore, we need 2 flags.
> > >
> > > I'm not saying you don't. Just that the description is a bit odd.
> > > Please expand the description for how one decides how to set the flags.
> >
> > Misunderstood your point, OK, I thought the "how to set the flags" was
> > simple and clear enough:
> > Use GPIO_ACTIVE_HIGH if GPIO physical state high is for cc1; or Use
> > GPIO_ACTIVE_LOW if GPIO physical state low is for cc1.
> 
> Okay.
> 
> > > > > > +examples:
> > > > > > +  - |
> > > > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > > > +    ptn36043 {
> > > > > > +        compatible = "typec-orientation-switch";
> > > > > > +        pinctrl-names = "default";
> > > > > > +        pinctrl-0 = <&pinctrl_ss_sel>;
> > > > > > +        switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > > > > +
> > > > > > +        port {
> > > > > > +                usb3_data_ss: endpoint {
> > > > > > +                        remote-endpoint = <&typec_con_ss>;
> > > > >
> > > > > The data goes from the connector to here and then where? You
> > > > > need a connection to the USB host controller.
> > > >
> > > > The orientation switch only need interact with type-c, no any
> > > > interaction with USB controller, do we still need a connection to it?
> > >
> > > If you have 2 USB hosts and 2 connectors (and 2 muxes), how would
> > > you describe which connector goes with which host?
> >
> > One instance of typec orientation switch defined by this binding only
> > for One typec connector. With that, my understanding is Whether a
> > connection need be described depends on if the connector (typec
> > driver) need notify the host controller driver to do something (e.g.
> > role switch need a connection between controller node and connector
> > node for controller driver to swap usb role). If the mux/switch
> > control is transparent to usb host controller (e.g. my case, usb
> > controller drivers normally don't need do anything for orientation
> > change), there is no need to describe connection between orientation
> > switch node and host controller node.
> 
> There can be several reasons you need to know the association. When writing
> the DT you can't assume what information is or isn't needed.
> That may vary by h/w or can evolve in an OS and the DT shouldn't change.

Okay, I will add the connection to usb controller in example.

Thanks
Li Jun
> 
> Rob