diff mbox series

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

Message ID 1603098195-9923-1-git-send-email-jun.li@nxp.com
State Superseded
Headers show
Series [v4,1/4] dt-bindings: usb: add documentation for typec switch simple driver | expand

Checks

Context Check Description
robh/dt-meta-schema success
robh/checkpatch success

Commit Message

Jun Li Oct. 19, 2020, 9:03 a.m. UTC
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(+)

Comments

Andy Shevchenko Oct. 19, 2020, 12:25 p.m. UTC | #1
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.
Andy Shevchenko Oct. 19, 2020, 12:31 p.m. UTC | #2
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);
Peter Chen Oct. 20, 2020, 6:05 a.m. UTC | #3
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
>
Jun Li Oct. 20, 2020, 11:13 a.m. UTC | #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
>
Andy Shevchenko Oct. 20, 2020, 12:06 p.m. UTC | #5
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!
Jun Li Oct. 20, 2020, 12:48 p.m. UTC | #6
> -----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
>
Jun Li Oct. 20, 2020, 1:02 p.m. UTC | #7
> -----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
Peter Chen Oct. 21, 2020, 12:16 a.m. UTC | #8
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
Jun Li Oct. 21, 2020, 2:43 a.m. UTC | #9
> -----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
Dmitry Torokhov Oct. 21, 2020, 6:55 p.m. UTC | #10
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 Torokhov Oct. 21, 2020, 7:55 p.m. UTC | #11
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.
Jun Li Oct. 22, 2020, 11:03 a.m. UTC | #12
> -----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
Jun Li Oct. 22, 2020, 11:52 a.m. UTC | #13
> -----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 mbox series

Patch

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