Message ID | 1603098195-9923-1-git-send-email-jun.li@nxp.com |
---|---|
State | Superseded, archived |
Headers | show |
Series | [v4,1/4] dt-bindings: usb: add documentation for typec switch simple driver | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success |
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.
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);
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 >
> -----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 >
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!
> -----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 >
> -----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
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
> -----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
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.
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.
> -----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
> -----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
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>; + }; + }; + };
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(+)