* [PATCH v4 1/4] dt-bindings: usb: add documentation for typec switch simple driver
@ 2020-10-19 9:03 Li Jun
2020-10-19 9:03 ` [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers Li Jun
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Li Jun @ 2020-10-19 9:03 UTC (permalink / raw)
To: heikki.krogerus, robh+dt, rafael
Cc: gregkh, andriy.shevchenko, hdegoede, lee.jones, mika.westerberg,
dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers
2020-10-19 9:03 [PATCH v4 1/4] dt-bindings: usb: add documentation for typec switch simple driver Li Jun
@ 2020-10-19 9:03 ` Li Jun
2020-10-19 12:25 ` Andy Shevchenko
2020-10-19 9:03 ` [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match Li Jun
2020-10-19 9:03 ` [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver Li Jun
2 siblings, 1 reply; 17+ messages in thread
From: Li Jun @ 2020-10-19 9:03 UTC (permalink / raw)
To: heikki.krogerus, robh+dt, rafael
Cc: gregkh, andriy.shevchenko, hdegoede, lee.jones, mika.westerberg,
dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match
2020-10-19 9:03 [PATCH v4 1/4] dt-bindings: usb: add documentation for typec switch simple driver Li Jun
2020-10-19 9:03 ` [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers Li Jun
@ 2020-10-19 9:03 ` Li Jun
2020-10-20 6:05 ` Peter Chen
2020-10-19 9:03 ` [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver Li Jun
2 siblings, 1 reply; 17+ messages in thread
From: Li Jun @ 2020-10-19 9:03 UTC (permalink / raw)
To: heikki.krogerus, robh+dt, rafael
Cc: gregkh, andriy.shevchenko, hdegoede, lee.jones, mika.westerberg,
dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
2020-10-19 9:03 [PATCH v4 1/4] dt-bindings: usb: add documentation for typec switch simple driver Li Jun
2020-10-19 9:03 ` [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers Li Jun
2020-10-19 9:03 ` [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match Li Jun
@ 2020-10-19 9:03 ` Li Jun
2020-10-19 12:31 ` Andy Shevchenko
2020-10-21 19:55 ` dmitry.torokhov
2 siblings, 2 replies; 17+ messages in thread
From: Li Jun @ 2020-10-19 9:03 UTC (permalink / raw)
To: heikki.krogerus, robh+dt, rafael
Cc: gregkh, andriy.shevchenko, hdegoede, lee.jones, mika.westerberg,
dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers
2020-10-19 9:03 ` [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers Li Jun
@ 2020-10-19 12:25 ` Andy Shevchenko
2020-10-20 11:13 ` Jun Li
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-19 12:25 UTC (permalink / raw)
To: Li Jun
Cc: heikki.krogerus, robh+dt, rafael, gregkh, hdegoede, lee.jones,
mika.westerberg, dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
2020-10-19 9:03 ` [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver Li Jun
@ 2020-10-19 12:31 ` Andy Shevchenko
2020-10-20 12:48 ` Jun Li
2020-10-21 19:55 ` dmitry.torokhov
1 sibling, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-19 12:31 UTC (permalink / raw)
To: Li Jun
Cc: heikki.krogerus, robh+dt, rafael, gregkh, hdegoede, lee.jones,
mika.westerberg, dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match
2020-10-19 9:03 ` [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match Li Jun
@ 2020-10-20 6:05 ` Peter Chen
2020-10-20 13:02 ` Jun Li
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-10-20 6:05 UTC (permalink / raw)
To: Jun Li
Cc: heikki.krogerus, robh+dt, rafael, gregkh, andriy.shevchenko,
hdegoede, lee.jones, mika.westerberg, dmitry.torokhov,
prabhakar.mahadev-lad.rj, laurent.pinchart+renesas, linux-usb,
devicetree, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers
2020-10-19 12:25 ` Andy Shevchenko
@ 2020-10-20 11:13 ` Jun Li
2020-10-20 12:06 ` Andy Shevchenko
2020-10-21 18:55 ` dmitry.torokhov
0 siblings, 2 replies; 17+ messages in thread
From: Jun Li @ 2020-10-20 11:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: heikki.krogerus, robh+dt, rafael, gregkh, hdegoede, lee.jones,
mika.westerberg, dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, dl-linux-imx,
Peter Chen
> -----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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers
2020-10-20 11:13 ` Jun Li
@ 2020-10-20 12:06 ` Andy Shevchenko
2020-10-21 18:55 ` dmitry.torokhov
1 sibling, 0 replies; 17+ messages in thread
From: Andy Shevchenko @ 2020-10-20 12:06 UTC (permalink / raw)
To: Jun Li
Cc: Andy Shevchenko, heikki.krogerus, robh+dt, rafael, gregkh,
hdegoede, lee.jones, mika.westerberg, dmitry.torokhov,
prabhakar.mahadev-lad.rj, laurent.pinchart+renesas, linux-usb,
devicetree, dl-linux-imx, Peter Chen
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
2020-10-19 12:31 ` Andy Shevchenko
@ 2020-10-20 12:48 ` Jun Li
0 siblings, 0 replies; 17+ messages in thread
From: Jun Li @ 2020-10-20 12:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: heikki.krogerus, robh+dt, rafael, gregkh, hdegoede, lee.jones,
mika.westerberg, dmitry.torokhov, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, dl-linux-imx,
Peter Chen
> -----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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match
2020-10-20 6:05 ` Peter Chen
@ 2020-10-20 13:02 ` Jun Li
2020-10-21 0:16 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Jun Li @ 2020-10-20 13:02 UTC (permalink / raw)
To: Peter Chen
Cc: heikki.krogerus, robh+dt, rafael, gregkh, andriy.shevchenko,
hdegoede, lee.jones, mika.westerberg, dmitry.torokhov,
prabhakar.mahadev-lad.rj, laurent.pinchart+renesas, linux-usb,
devicetree, dl-linux-imx
> -----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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match
2020-10-20 13:02 ` Jun Li
@ 2020-10-21 0:16 ` Peter Chen
2020-10-21 2:43 ` Jun Li
0 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2020-10-21 0:16 UTC (permalink / raw)
To: Jun Li
Cc: heikki.krogerus, robh+dt, rafael, gregkh, andriy.shevchenko,
hdegoede, lee.jones, mika.westerberg, dmitry.torokhov,
prabhakar.mahadev-lad.rj, laurent.pinchart+renesas, linux-usb,
devicetree, dl-linux-imx
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match
2020-10-21 0:16 ` Peter Chen
@ 2020-10-21 2:43 ` Jun Li
0 siblings, 0 replies; 17+ messages in thread
From: Jun Li @ 2020-10-21 2:43 UTC (permalink / raw)
To: Peter Chen
Cc: heikki.krogerus, robh+dt, rafael, gregkh, andriy.shevchenko,
hdegoede, lee.jones, mika.westerberg, dmitry.torokhov,
prabhakar.mahadev-lad.rj, laurent.pinchart+renesas, linux-usb,
devicetree, dl-linux-imx
> -----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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers
2020-10-20 11:13 ` Jun Li
2020-10-20 12:06 ` Andy Shevchenko
@ 2020-10-21 18:55 ` dmitry.torokhov
2020-10-22 11:03 ` Jun Li
1 sibling, 1 reply; 17+ messages in thread
From: dmitry.torokhov @ 2020-10-21 18:55 UTC (permalink / raw)
To: Jun Li
Cc: Andy Shevchenko, heikki.krogerus, robh+dt, rafael, gregkh,
hdegoede, lee.jones, mika.westerberg, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, dl-linux-imx,
Peter Chen
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
2020-10-19 9:03 ` [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver Li Jun
2020-10-19 12:31 ` Andy Shevchenko
@ 2020-10-21 19:55 ` dmitry.torokhov
2020-10-22 11:52 ` Jun Li
1 sibling, 1 reply; 17+ messages in thread
From: dmitry.torokhov @ 2020-10-21 19:55 UTC (permalink / raw)
To: Li Jun
Cc: heikki.krogerus, robh+dt, rafael, gregkh, andriy.shevchenko,
hdegoede, lee.jones, mika.westerberg, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, linux-imx,
peter.chen
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers
2020-10-21 18:55 ` dmitry.torokhov
@ 2020-10-22 11:03 ` Jun Li
0 siblings, 0 replies; 17+ messages in thread
From: Jun Li @ 2020-10-22 11:03 UTC (permalink / raw)
To: dmitry.torokhov
Cc: Andy Shevchenko, heikki.krogerus, robh+dt, rafael, gregkh,
hdegoede, lee.jones, mika.westerberg, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, dl-linux-imx,
Peter Chen
> -----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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver
2020-10-21 19:55 ` dmitry.torokhov
@ 2020-10-22 11:52 ` Jun Li
0 siblings, 0 replies; 17+ messages in thread
From: Jun Li @ 2020-10-22 11:52 UTC (permalink / raw)
To: dmitry.torokhov
Cc: heikki.krogerus, robh+dt, rafael, gregkh, andriy.shevchenko,
hdegoede, lee.jones, mika.westerberg, prabhakar.mahadev-lad.rj,
laurent.pinchart+renesas, linux-usb, devicetree, dl-linux-imx,
Peter Chen
> -----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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-10-22 11:53 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-19 9:03 [PATCH v4 1/4] dt-bindings: usb: add documentation for typec switch simple driver Li Jun
2020-10-19 9:03 ` [PATCH v4 2/4] device property: Add fwnode_is_compatible() and device_is_compatible() helpers Li Jun
2020-10-19 12:25 ` Andy Shevchenko
2020-10-20 11:13 ` Jun Li
2020-10-20 12:06 ` Andy Shevchenko
2020-10-21 18:55 ` dmitry.torokhov
2020-10-22 11:03 ` Jun Li
2020-10-19 9:03 ` [PATCH v4 3/4] usb: typec: mux: add "compatible" property for switch match Li Jun
2020-10-20 6:05 ` Peter Chen
2020-10-20 13:02 ` Jun Li
2020-10-21 0:16 ` Peter Chen
2020-10-21 2:43 ` Jun Li
2020-10-19 9:03 ` [PATCH v4 4/4] usb: typec: mux: add typec switch simple driver Li Jun
2020-10-19 12:31 ` Andy Shevchenko
2020-10-20 12:48 ` Jun Li
2020-10-21 19:55 ` dmitry.torokhov
2020-10-22 11:52 ` Jun Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).