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

[PATCH v4 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>
---
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.

previsou 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 v4 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>
---
New patch for v4.

 drivers/base/property.c  | 35 +++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 38 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index d58aa98..32e3a3e 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1184,3 +1184,38 @@ 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..4a84325 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -418,6 +418,9 @@ fwnode_graph_get_endpoint_by_id(const struct fwnode_handle *fwnode,
 int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 				struct fwnode_endpoint *endpoint);
 
+int fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compat);
+int device_is_compatible(struct device *dev, const char *compat);
+
 /* -------------------------------------------------------------------------- */
 /* Software fwnode support - when HW description is incomplete or missing */
 
-- 
2.7.4

[PATCH v4 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>
---
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

[PATCH v4 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 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         |   6 ++
 drivers/usb/typec/mux/Makefile        |   1 +
 drivers/usb/typec/mux/switch-simple.c | 110 ++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index a4dbd11..2942588 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -17,5 +17,11 @@ config TYPEC_MUX_INTEL_PMC
 	  Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
 	  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.
 
 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..98f4b49
--- /dev/null
+++ b/drivers/usb/typec/mux/switch-simple.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * switch-simple.c - typec switch simple control.
+ *
+ * Copyright 2020 NXP
+ * Author: Jun Li <jun.li@nxp.com>
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/usb/typec_mux.h>
+
+struct typec_switch_simple {
+	struct typec_switch *sw;
+	struct mutex lock;
+	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);
+
+	mutex_lock(&typec_sw->lock);
+
+	switch (orientation) {
+	case TYPEC_ORIENTATION_NORMAL:
+		if (typec_sw->sel_gpio)
+			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		if (typec_sw->sel_gpio)
+			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
+		break;
+	case TYPEC_ORIENTATION_NONE:
+		break;
+	}
+
+	mutex_unlock(&typec_sw->lock);
+
+	return 0;
+}
+
+static int typec_switch_simple_probe(struct platform_device *pdev)
+{
+	struct typec_switch_simple	*typec_sw;
+	struct device			*dev = &pdev->dev;
+	struct typec_switch_desc sw_desc;
+
+	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;
+	mutex_init(&typec_sw->lock);
+
+	/* 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 v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers

[Andy Shevchenko]

On Mon, Oct 19, 2020 at 05:03:13PM +0800, Li Jun wrote:
> 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.

...

> + * Match the compatible strings of @fwnode against @compat. Returns positive
> + * value on match, and 0 when no matching compatible string is found.

Please move Returns... to a separate paragraph.

Btw, this is not true...

> +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;

...and this is at least strange after all.

> +}

> + * Match the compatible strings of @dev against @compat. Returns positive value
> + * on match, and 0 when no matching compatible string is found.

So does this.

...

> +int fwnode_is_compatible(struct fwnode_handle *fwnode, const char *compat);
> +int device_is_compatible(struct device *dev, const char *compat);

Please, group them rather device_is_compatible() with device_* and fwnode_*
ones respectively.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Oct 19, 2020 at 05:03:15PM +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.

...

>  	  Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
>  	  control the USB role switch and also the multiplexer/demultiplexer
>  	  switches used with USB Type-C Alternate Modes.

Missed blank line.

> +config TYPEC_SWITCH_SIMPLE
> +	tristate "Type-c orientation switch Simple driver"

Type-c -> Type-C

Simple -> simple


> +	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.

Driver name?

...

> +/**

Is it kernel doc?!

> + * switch-simple.c - typec switch simple control.

Remove file name from the file.

> + *
> + * Copyright 2020 NXP
> + * Author: Jun Li <jun.li@nxp.com>

> + *

Redundant blank line.

> + */

...

> +#include <linux/of.h>
> +#include <linux/of_gpio.h>

No evidence of use of these headers, but
mod_devicetable.h along with gpio/consumer.h are missed.


...

> +	switch (orientation) {
> +	case TYPEC_ORIENTATION_NORMAL:
> +		if (typec_sw->sel_gpio)

Optional GPIO does not require these checks. Drop them and learn what "optional" means.

> +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		if (typec_sw->sel_gpio)
> +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> +		break;
> +	case TYPEC_ORIENTATION_NONE:
> +		break;
> +	}

...

> +	struct typec_switch_simple	*typec_sw;
> +	struct device			*dev = &pdev->dev;
> +	struct typec_switch_desc sw_desc;

Be consistent with indentation.

...

> +	/* Get the super speed active channel selection GPIO */
> +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch",
> +						     GPIOD_OUT_LOW);

It can be one line.

> +	if (IS_ERR(typec_sw->sel_gpio))
> +		return PTR_ERR(typec_sw->sel_gpio);

-- 
With Best Regards,
Andy Shevchenko

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

[Peter Chen]

On 20-10-19 17:03:14, 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>
> ---
> 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;

If property is not present, should not fwnode_is_compatible return NULL?

Peter

>  
>  	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,
Peter Chen

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

[Jun Li]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, October 19, 2020 8:25 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; robh+dt@kernel.org;
> rafael@kernel.org; gregkh@linuxfoundation.org; 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 v4 2/4] device property: Add fwnode_is_compatible() and
> device_is_compatible() helpers
> 
> On Mon, Oct 19, 2020 at 05:03:13PM +0800, Li Jun wrote:
> > 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.
> 
> ...
> 
> > + * Match the compatible strings of @fwnode against @compat. Returns
> > + positive
> > + * value on match, and 0 when no matching compatible string is found.
> 
> Please move Returns... to a separate paragraph.

OK, will change.

> 
> Btw, this is not true...
> 
> > +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;
> 
> ...and this is at least strange after all.

of_device_is_compatible() will return positive value on match, and 0
when no matching,
fwnode_property_match_string() will return 0 if the property was found
(success); and negative error code on failure. so the return conversion
of fwnode_property_match_string () should work here.

> 
> > +}
> 
> > + * Match the compatible strings of @dev against @compat. Returns
> > + positive value
> > + * on match, and 0 when no matching compatible string is found.
> 
> So does this.

Yes, will change.

> 
> ...
> 
> > +int fwnode_is_compatible(struct fwnode_handle *fwnode, const char
> > +*compat); int device_is_compatible(struct device *dev, const char
> > +*compat);
> 
> Please, group them rather device_is_compatible() with device_* and fwnode_*
> ones respectively.

Ok, will move them in next version.

Thanks
Li Jun

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

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

[Andy Shevchenko]

On Tue, Oct 20, 2020 at 2:35 PM Jun Li <jun.li@nxp.com> wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Monday, October 19, 2020 8:25 PM
> > On Mon, Oct 19, 2020 at 05:03:13PM +0800, Li Jun wrote:

...

> of_device_is_compatible() will return positive value on match, and 0
> when no matching,
> fwnode_property_match_string() will return 0 if the property was found
> (success); and negative error code on failure.

Thanks for checking this!

-- 
With Best Regards,
Andy Shevchenko

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

[Jun Li]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, October 19, 2020 8:31 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; robh+dt@kernel.org;
> rafael@kernel.org; gregkh@linuxfoundation.org; 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 v4 4/4] usb: typec: mux: add typec switch simple driver
> 
> On Mon, Oct 19, 2020 at 05:03:15PM +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.
> 
> ...
> 
> >  	  Driver for USB muxes controlled by Intel PMC FW. Intel PMC FW can
> >  	  control the USB role switch and also the multiplexer/demultiplexer
> >  	  switches used with USB Type-C Alternate Modes.
> 
> Missed blank line.

Will add.

> 
> > +config TYPEC_SWITCH_SIMPLE
> > +	tristate "Type-c orientation switch Simple driver"
> 
> Type-c -> Type-C
> 
> Simple -> simple

Will change.

> 
> 
> > +	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.
> 
> Driver name?

Will add the driver module name.

> 
> ...
> 
> > +/**
> 
> Is it kernel doc?!

Will change to "/*"

> 
> > + * switch-simple.c - typec switch simple control.
> 
> Remove file name from the file.

Will remove it.
> 
> > + *
> > + * Copyright 2020 NXP
> > + * Author: Jun Li <jun.li@nxp.com>
> 
> > + *
> 
> Redundant blank line.

Will remove.

> 
> > + */
> 
> ...
> 
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> 
> No evidence of use of these headers, but mod_devicetable.h along with
> gpio/consumer.h are missed.

Thanks, will change.

> 
> 
> ...
> 
> > +	switch (orientation) {
> > +	case TYPEC_ORIENTATION_NORMAL:
> > +		if (typec_sw->sel_gpio)
> 
> Optional GPIO does not require these checks. Drop them and learn what
> "optional" means.

Checked, will drop those checks.

> 
> > +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> > +		break;
> > +	case TYPEC_ORIENTATION_REVERSE:
> > +		if (typec_sw->sel_gpio)
> > +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> > +		break;
> > +	case TYPEC_ORIENTATION_NONE:
> > +		break;
> > +	}
> 
> ...
> 
> > +	struct typec_switch_simple	*typec_sw;
> > +	struct device			*dev = &pdev->dev;
> > +	struct typec_switch_desc sw_desc;
> 
> Be consistent with indentation.

Wil change.

> 
> ...
> 
> > +	/* Get the super speed active channel selection GPIO */
> > +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch",
> > +						     GPIOD_OUT_LOW);
> 
> It can be one line.

Will change.

Thanks
Li Jun
> 
> > +	if (IS_ERR(typec_sw->sel_gpio))
> > +		return PTR_ERR(typec_sw->sel_gpio);
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

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

[Jun Li]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: Tuesday, October 20, 2020 2:05 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; 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>
> Subject: Re: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for
> switch match
> 
> On 20-10-19 17:03:14, 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>
> > ---
> > 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;
> 
> If property is not present, should not fwnode_is_compatible return NULL?

Do you want to say:

If property is not present, should not *typec_switch_match* return NULL?

Li Jun
> 
> Peter
> 
> >
> >  	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,
> Peter Chen

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

[Peter Chen]

On 20-10-20 13:02:30, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Peter Chen <peter.chen@nxp.com>
> > Sent: Tuesday, October 20, 2020 2:05 PM
> > To: Jun Li <jun.li@nxp.com>
> > Cc: heikki.krogerus@linux.intel.com; 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>
> > Subject: Re: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for
> > switch match
> > 
> > On 20-10-19 17:03:14, 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>
> > > ---
> > > 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;
> > 
> > If property is not present, should not fwnode_is_compatible return NULL?
> 
> Do you want to say:
> 
> If property is not present, should not *typec_switch_match* return NULL?

If fwnode_is_compatible returns true, it should mean the fwnode_property_present is
true too. But if fwnode_is_compatible is false, the fwnode_property_present may return
true, the code does not return NULL. Then, with this new patch, what changes compared
to original logic?

Peter

> > 
> > >
> > >  	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,
> > Peter Chen

-- 

Thanks,
Peter Chen

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

[Jun Li]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: Wednesday, October 21, 2020 8:16 AM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; 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>
> Subject: Re: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for
> switch match
> 
> On 20-10-20 13:02:30, Jun Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Peter Chen <peter.chen@nxp.com>
> > > Sent: Tuesday, October 20, 2020 2:05 PM
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: heikki.krogerus@linux.intel.com; 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>
> > > Subject: Re: [PATCH v4 3/4] usb: typec: mux: add "compatible"
> > > property for switch match
> > >
> > > On 20-10-19 17:03:14, 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>
> > > > ---
> > > > 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;
> > >
> > > If property is not present, should not fwnode_is_compatible return NULL?
> >
> > Do you want to say:
> >
> > If property is not present, should not *typec_switch_match* return NULL?
> 
> If fwnode_is_compatible returns true, it should mean the
> fwnode_property_present is true too. 

Nope.

fwnode_is_compatible():
        property name: compatible
fwnode_property_present():
        property name: typec-orientation-switch

> But if fwnode_is_compatible is false,
> the fwnode_property_present may return true, the code does not return NULL.

Yes. Original matching via bool property is still allowed.

> Then, with this new patch, what changes compared to original logic?

One more way to match typec switch via compatible.

> 
> Peter
> 
> > >
> > > >
> > > >  	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,
> > > Peter Chen
> 
> --
> 
> Thanks,
> Peter Chen

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

[dmitry.torokhov]

On Tue, Oct 20, 2020 at 11:13:47AM +0000, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: Monday, October 19, 2020 8:25 PM
> > To: Jun Li <jun.li@nxp.com>
> > Cc: heikki.krogerus@linux.intel.com; robh+dt@kernel.org;
> > rafael@kernel.org; gregkh@linuxfoundation.org; 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 v4 2/4] device property: Add fwnode_is_compatible() and
> > device_is_compatible() helpers
> > 
> > On Mon, Oct 19, 2020 at 05:03:13PM +0800, Li Jun wrote:
> > > 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.
> > 
> > ...
> > 
> > > + * Match the compatible strings of @fwnode against @compat. Returns
> > > + positive
> > > + * value on match, and 0 when no matching compatible string is found.
> > 
> > Please move Returns... to a separate paragraph.
> 
> OK, will change.
> 
> > 
> > Btw, this is not true...
> > 
> > > +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;
> > 
> > ...and this is at least strange after all.
> 
> of_device_is_compatible() will return positive value on match, and 0
> when no matching,
> fwnode_property_match_string() will return 0 if the property was found
> (success); and negative error code on failure. so the return conversion
> of fwnode_property_match_string () should work here.

Yes, but please make the function return bool instead of int.
of_device_is_compatible() returns "score", so it is int, but here we
only want yes/no.

So

	return fwnode_property_match_string(...) == 0;

Thanks.

-- 
Dmitry

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

[dmitry.torokhov]

Hi,

On Mon, Oct 19, 2020 at 05:03:15PM +0800, Li Jun wrote:
> +
> +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);
> +
> +	mutex_lock(&typec_sw->lock);

Why is this lock needed? It looks like we are passing requests directly
to gpiochip which I expect would take care of this.

> +
> +	switch (orientation) {
> +	case TYPEC_ORIENTATION_NORMAL:
> +		if (typec_sw->sel_gpio)
> +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		if (typec_sw->sel_gpio)
> +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> +		break;
> +	case TYPEC_ORIENTATION_NONE:
> +		break;
> +	}
> +
> +	mutex_unlock(&typec_sw->lock);
> +
> +	return 0;
> +}
> +
> +static int typec_switch_simple_probe(struct platform_device *pdev)
> +{
> +	struct typec_switch_simple	*typec_sw;
> +	struct device			*dev = &pdev->dev;
> +	struct typec_switch_desc sw_desc;
> +
> +	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;
> +	mutex_init(&typec_sw->lock);
> +
> +	/* Get the super speed active channel selection GPIO */
> +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch",
> +						     GPIOD_OUT_LOW);

Does this driver make sense without the GPIO? Should it be made
mandatory?

Thanks.

-- 
Dmitry

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

[Jun Li]

> -----Original Message-----
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
> Sent: Thursday, October 22, 2020 2:56 AM
> To: Jun Li <jun.li@nxp.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> heikki.krogerus@linux.intel.com; robh+dt@kernel.org; rafael@kernel.org;
> gregkh@linuxfoundation.org; hdegoede@redhat.com; lee.jones@linaro.org;
> mika.westerberg@linux.intel.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 v4 2/4] device property: Add fwnode_is_compatible() and
> device_is_compatible() helpers
> 
> On Tue, Oct 20, 2020 at 11:13:47AM +0000, Jun Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Sent: Monday, October 19, 2020 8:25 PM
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: heikki.krogerus@linux.intel.com; robh+dt@kernel.org;
> > > rafael@kernel.org; gregkh@linuxfoundation.org; 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 v4 2/4] device property: Add
> > > fwnode_is_compatible() and
> > > device_is_compatible() helpers
> > >
> > > On Mon, Oct 19, 2020 at 05:03:13PM +0800, Li Jun wrote:
> > > > 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.
> > >
> > > ...
> > >
> > > > + * Match the compatible strings of @fwnode against @compat.
> > > > + Returns positive
> > > > + * value on match, and 0 when no matching compatible string is found.
> > >
> > > Please move Returns... to a separate paragraph.
> >
> > OK, will change.
> >
> > >
> > > Btw, this is not true...
> > >
> > > > +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;
> > >
> > > ...and this is at least strange after all.
> >
> > of_device_is_compatible() will return positive value on match, and 0
> > when no matching,
> > fwnode_property_match_string() will return 0 if the property was found
> > (success); and negative error code on failure. so the return
> > conversion of fwnode_property_match_string () should work here.
> 
> Yes, but please make the function return bool instead of int.
> of_device_is_compatible() returns "score", so it is int, but here we only
> want yes/no.
> 
> So
> 
> 	return fwnode_property_match_string(...) == 0;

My understanding this is to keep this new API fwnode_is_compatible() consistent
with of_device_is_compatible(). I would wait patch author Heikki to comment this.

thanks
Li Jun 
> 
> Thanks.
> 
> --
> Dmitry

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

[Jun Li]

> -----Original Message-----
> From: dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
> Sent: Thursday, October 22, 2020 3:56 AM
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; 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;
> 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 v4 4/4] usb: typec: mux: add typec switch simple driver
> 
> Hi,
> 
> On Mon, Oct 19, 2020 at 05:03:15PM +0800, Li Jun wrote:
> > +
> > +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);
> > +
> > +	mutex_lock(&typec_sw->lock);
> 
> Why is this lock needed? It looks like we are passing requests directly to
> gpiochip which I expect would take care of this.

Checked some gpiochips, looks like with only GPIO, yes, this lock is not required,
I will remove it.

> 
> > +
> > +	switch (orientation) {
> > +	case TYPEC_ORIENTATION_NORMAL:
> > +		if (typec_sw->sel_gpio)
> > +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 1);
> > +		break;
> > +	case TYPEC_ORIENTATION_REVERSE:
> > +		if (typec_sw->sel_gpio)
> > +			gpiod_set_value_cansleep(typec_sw->sel_gpio, 0);
> > +		break;
> > +	case TYPEC_ORIENTATION_NONE:
> > +		break;
> > +	}
> > +
> > +	mutex_unlock(&typec_sw->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int typec_switch_simple_probe(struct platform_device *pdev) {
> > +	struct typec_switch_simple	*typec_sw;
> > +	struct device			*dev = &pdev->dev;
> > +	struct typec_switch_desc sw_desc;
> > +
> > +	typec_sw = devm_kzalloc(dev, sizeof(*typec_sw), GFP_KERNEL);
> > +	if (!typec_sw)
> > +		return -ENOMEM;
> > +q
> > +	platform_set_drvdata(pdev, typec_sw);
> > +
> > +	sw_desc.drvdata = typec_sw;
> > +	sw_desc.fwnode = dev->fwnode;
> > +	sw_desc.set = typec_switch_simple_set;
> > +	mutex_init(&typec_sw->lock);
> > +
> > +	/* Get the super speed active channel selection GPIO */
> > +	typec_sw->sel_gpio = devm_gpiod_get_optional(dev, "switch",
> > +						     GPIOD_OUT_LOW);
> 
> Does this driver make sense without the GPIO? Should it be made mandatory?

My old version made it to be mandatory, I change it to be optional in this version
for possible extend to other simple typec switch design which do not use GPIO as
this is going to be a generic typec switch driver. yes, for current implementation,
this driver will only register a typec switch in sys if without GPIO provided.

Li Jun

> 
> Thanks.
> 
> --
> Dmitry