| Message ID | 1603098195-9923-4-git-send-email-jun.li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/4] dt-bindings: usb: add documentation for typec switch simple driver | expand |
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);
> -----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 >
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 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/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>");
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(+)