| Message ID | 1604403610-16577-1-git-send-email-jun.li@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,1/4] dt-bindings: usb: add documentation for typec switch simple driver | expand |
On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote: > 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> > --- > No changes for v5. > > 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. > > previous 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. What does active mean? There isn't really an active and inactive state, right? It's more a mux selecting 0 or 1 input? I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an inverter in the middle. > + 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>; The data goes from the connector to here and then where? You need a connection to the USB host controller. > + }; > + }; > + }; > -- > 2.7.4 >
> -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Friday, November 6, 2020 6:26 AM > To: Jun Li <jun.li@nxp.com> > Cc: heikki.krogerus@linux.intel.com; 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>; Peter Chen > <peter.chen@nxp.com> > Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec > switch simple driver > > On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote: > > 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> > > --- > > No changes for v5. > > > > 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. > > > > previous discussion: > > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch > > > work.ozlabs.org%2Fpatch%2F1054342%2F&data=04%7C01%7Cjun.li%40nxp.c > > > om%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c5c3016 > > > 35%7C0%7C0%7C637402119664101856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > > > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata= > > 8TY%2BPRIui6HhxhYE1%2BLmwWL38Vp7SY1Ceb5rGG%2B4DUo%3D&reserved=0 > > > > .../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: > > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fschemas%2Fusb%2Ftypec-switch-simple.yaml%23&data=04% > > > +7C01%7Cjun.li%40nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3 > > > +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWF > > > +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 > > > +Mn0%3D%7C1000&sdata=HjWfKlDLyqb%2FKLlL6vdnyPe%2BnB8pSllhokIXQ%2Bw > > +yyw8%3D&reserved=0 > > +$schema: > > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=04%7C01%7Cjun.li%40 > > > +nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c > > > +5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWFpbGZsb3d8eyJWIjo > > > +iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&am > > > +p;sdata=z0bO47QVl5gw0UE%2Bx3a5E27ALgz568zp%2Bf4suGlch%2Fo%3D&rese > > +rved=0 > > + > > +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. > > What does active mean? There isn't really an active and inactive state, right? > It's more a mux selecting 0 or 1 input? Yes, I will change the description: gpio specifier to select the target channel of mux. > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an inverter > in the middle. This depends on the switch IC design and board design, leave 2 flags (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases. NXP has 2 diff IC parts for this: 1. PTN36043(used on iMX8MQ) Output selection control When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor. When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor. Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1, so GPIO_ACTIVE_HIGH 2. CBTU02043(used on iMX8MP) SEL Function -------------------------------------- Low A to B ports and vice versa High A to C ports and vice versa Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW Therefore, we need 2 flags. > > > + 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>; > > The data goes from the connector to here and then where? You need a connection > to the USB host controller. The orientation switch only need interact with type-c, no any interaction with USB controller, do we still need a connection to it? Thanks Li Jun > > > + }; > > + }; > > + }; > > -- > > 2.7.4 > >
On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote: > > > > > -----Original Message----- > > From: Rob Herring <robh@kernel.org> > > Sent: Friday, November 6, 2020 6:26 AM > > To: Jun Li <jun.li@nxp.com> > > Cc: heikki.krogerus@linux.intel.com; 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>; Peter Chen > > <peter.chen@nxp.com> > > Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec > > switch simple driver > > > > On Tue, Nov 03, 2020 at 07:40:07PM +0800, Li Jun wrote: > > > 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> > > > --- > > > No changes for v5. > > > > > > 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. > > > > > > previous discussion: > > > > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch > > > > > work.ozlabs.org%2Fpatch%2F1054342%2F&data=04%7C01%7Cjun.li%40nxp.c > > > > > om%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c5c3016 > > > > > 35%7C0%7C0%7C637402119664101856%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > > > > > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata= > > > 8TY%2BPRIui6HhxhYE1%2BLmwWL38Vp7SY1Ceb5rGG%2B4DUo%3D&reserved=0 > > > > > > .../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: > > > > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > > > +cetree.org%2Fschemas%2Fusb%2Ftypec-switch-simple.yaml%23&data=04% > > > > > +7C01%7Cjun.li%40nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3 > > > > > +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWF > > > > > +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 > > > > > +Mn0%3D%7C1000&sdata=HjWfKlDLyqb%2FKLlL6vdnyPe%2BnB8pSllhokIXQ%2Bw > > > +yyw8%3D&reserved=0 > > > +$schema: > > > > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi > > > > > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=04%7C01%7Cjun.li%40 > > > > > +nxp.com%7C78ca5205695149e2743208d881d9c753%7C686ea1d3bc2b4c6fa92cd99c > > > > > +5c301635%7C0%7C0%7C637402119664111854%7CUnknown%7CTWFpbGZsb3d8eyJWIjo > > > > > +iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&am > > > > > +p;sdata=z0bO47QVl5gw0UE%2Bx3a5E27ALgz568zp%2Bf4suGlch%2Fo%3D&rese > > > +rved=0 > > > + > > > +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. > > > > What does active mean? There isn't really an active and inactive state, right? > > It's more a mux selecting 0 or 1 input? > > Yes, I will change the description: > gpio specifier to select the target channel of mux. I wonder if the existing mux bindings should be used here. > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an inverter > > in the middle. > > This depends on the switch IC design and board design, leave 2 flags > (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases. > > NXP has 2 diff IC parts for this: > 1. PTN36043(used on iMX8MQ) > Output selection control > When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and > RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor. > When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and > RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor. > > Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1, > so GPIO_ACTIVE_HIGH > > 2. CBTU02043(used on iMX8MP) > SEL Function > -------------------------------------- > Low A to B ports and vice versa > High A to C ports and vice versa > > Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW > > Therefore, we need 2 flags. I'm not saying you don't. Just that the description is a bit odd. Please expand the description for how one decides how to set the flags. > > > > > > + 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>; > > > > The data goes from the connector to here and then where? You need a connection > > to the USB host controller. > > The orientation switch only need interact with type-c, no any interaction > with USB controller, do we still need a connection to it? If you have 2 USB hosts and 2 connectors (and 2 muxes), how would you describe which connector goes with which host? Rob
On Tue, Nov 03, 2020 at 07:40:10PM +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. > > Signed-off-by: Li Jun <jun.li@nxp.com> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Changes for v5: > - A few changes address Andy's comment, remove gpio check as it's > optional, add module name for Kconfig, use correct header files, > and other minor changes. > - Remove the mutex lock as it's not required currently. > > 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 | 10 ++++ > drivers/usb/typec/mux/Makefile | 1 + > drivers/usb/typec/mux/switch-simple.c | 100 ++++++++++++++++++++++++++++++++++ > 3 files changed, 111 insertions(+) > > diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig > index a4dbd11..11320d7 100644 > --- a/drivers/usb/typec/mux/Kconfig > +++ b/drivers/usb/typec/mux/Kconfig > @@ -18,4 +18,14 @@ config TYPEC_MUX_INTEL_PMC > 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. > + > + To compile this driver as a module, choose M here: the > + module will be called switch-simple. > + > 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..8707703 > --- /dev/null > +++ b/drivers/usb/typec/mux/switch-simple.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Type-C switch simple control driver > + * > + * Copyright 2020 NXP > + * Author: Jun Li <jun.li@nxp.com> > + */ > + > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/mutex.h> > +#include <linux/platform_device.h> > +#include <linux/usb/typec_mux.h> > + > +struct typec_switch_simple { > + struct typec_switch *sw; > + 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); > + > + switch (orientation) { > + case TYPEC_ORIENTATION_NORMAL: > + gpiod_set_value_cansleep(typec_sw->sel_gpio, 1); > + break; > + case TYPEC_ORIENTATION_REVERSE: > + gpiod_set_value_cansleep(typec_sw->sel_gpio, 0); > + break; > + case TYPEC_ORIENTATION_NONE: > + break; > + } > + > + return 0; > +} > + > +static int typec_switch_simple_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct typec_switch_desc sw_desc; > + struct typec_switch_simple *typec_sw; > + > + 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; > + > + /* 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 thanks, -- heikki
On Mon, Nov 9, 2020 at 6:24 AM Jun Li <jun.li@nxp.com> wrote: > > From: Rob Herring <robh@kernel.org> > > On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote: > > > > From: Rob Herring <robh@kernel.org> > > > > > +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. > > > > > > > > What does active mean? There isn't really an active and inactive state, > > right? > > > > It's more a mux selecting 0 or 1 input? > > > > > > Yes, I will change the description: > > > gpio specifier to select the target channel of mux. > > > > I wonder if the existing mux bindings should be used here. > > If only consider typec switch via "gpio", existing "mux-gpio" > binding may be used with property "mux-control-names" to be > "typec-xxx" for match, but we still need create typec stuff at > mux driver to hook to typec system, so little benefit, considering > this binding is going to be for a generic typec orientation switch > simple driver, I think a new typec binding make sense. You can instantiate drivers without a compatible. That's just the easy way. However, using the mux binding doesn't necessarily mean you have to use 'mux-gpio'. Consider if you need to do more control than just the GPIO line. For example, the chips you mentioned may have a s/w controlled power supply or reset. Also, consider what the mux would look like with different control interfaces. That could be I2C or some sub-block in a PMIC or ??? I'm sure we already have some examples. I'm not happy with these piecemeal additions to TypeC related bindings that don't consider more than 1 h/w possibility. > > > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's an > > > > inverter in the middle. > > > > > > This depends on the switch IC design and board design, leave 2 flags > > > (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible cases. > > > > > > NXP has 2 diff IC parts for this: > > > 1. PTN36043(used on iMX8MQ) > > > Output selection control > > > When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, and > > > RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor. > > > When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, and > > > RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor. > > > > > > Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1, so > > > GPIO_ACTIVE_HIGH > > > > > > 2. CBTU02043(used on iMX8MP) > > > SEL Function > > > -------------------------------------- > > > Low A to B ports and vice versa > > > High A to C ports and vice versa > > > > > > Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW > > > > > > Therefore, we need 2 flags. > > > > I'm not saying you don't. Just that the description is a bit odd. > > Please expand the description for how one decides how to set the flags. > > Misunderstood your point, OK, I thought the "how to set the flags" was > simple and clear enough: > Use GPIO_ACTIVE_HIGH if GPIO physical state high is for cc1; or > Use GPIO_ACTIVE_LOW if GPIO physical state low is for cc1. Okay. > > > > > +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>; > > > > > > > > The data goes from the connector to here and then where? You need a > > > > connection to the USB host controller. > > > > > > The orientation switch only need interact with type-c, no any > > > interaction with USB controller, do we still need a connection to it? > > > > If you have 2 USB hosts and 2 connectors (and 2 muxes), how would you describe > > which connector goes with which host? > > One instance of typec orientation switch defined by this binding only for > One typec connector. With that, my understanding is > Whether a connection need be described depends on if the connector > (typec driver) need notify the host controller driver to do something > (e.g. role switch need a connection between controller node and connector > node for controller driver to swap usb role). If the mux/switch control is > transparent to usb host controller (e.g. my case, usb controller drivers > normally don't need do anything for orientation change), there is no need > to describe connection between orientation switch node and host controller > node. There can be several reasons you need to know the association. When writing the DT you can't assume what information is or isn't needed. That may vary by h/w or can evolve in an OS and the DT shouldn't change. Rob
Hi Rob > -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Monday, November 9, 2020 10:38 PM > To: Jun Li <jun.li@nxp.com> > Cc: heikki.krogerus@linux.intel.com; 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>; Peter Chen > <peter.chen@nxp.com> > Subject: Re: [PATCH v5 1/4] dt-bindings: usb: add documentation for typec > switch simple driver > > On Mon, Nov 9, 2020 at 6:24 AM Jun Li <jun.li@nxp.com> wrote: > > > From: Rob Herring <robh@kernel.org> > > > On Fri, Nov 6, 2020 at 5:07 AM Jun Li <jun.li@nxp.com> wrote: > > > > > From: Rob Herring <robh@kernel.org> > > > > > > > +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. > > > > > > > > > > What does active mean? There isn't really an active and inactive > > > > > state, > > > right? > > > > > It's more a mux selecting 0 or 1 input? > > > > > > > > Yes, I will change the description: > > > > gpio specifier to select the target channel of mux. > > > > > > I wonder if the existing mux bindings should be used here. > > > > If only consider typec switch via "gpio", existing "mux-gpio" > > binding may be used with property "mux-control-names" to be > > "typec-xxx" for match, but we still need create typec stuff at mux > > driver to hook to typec system, so little benefit, considering this > > binding is going to be for a generic typec orientation switch simple > > driver, I think a new typec binding make sense. > > You can instantiate drivers without a compatible. That's just the easy way. > However, using the mux binding doesn't necessarily mean you have to use > 'mux-gpio'. Consider if you need to do more control than just the GPIO line. > For example, the chips you mentioned may have a s/w controlled power supply > or reset. > > Also, consider what the mux would look like with different control interfaces. > That could be I2C or some sub-block in a PMIC or ??? I'm sure we already > have some examples. I'm not happy with these piecemeal additions to TypeC > related bindings that don't consider more than 1 h/w possibility. As typec class sub system already has its own interface(and users) to do switch/mux control, using existing mux bindings means typec class will add switch/mux control through another approach(mux chip/controller interface), this makes the typec mux looks having 2 similar way for the same function. so I was hesitating to use it. After more check, I agree creating typec specific bindings will duplicate much existing mux bindings, the mux controller based bindings can be a good way used for various typec switch/mux, so I will try typec orientation compatible with mux controller bindings. > > > > > > I think you want flags 0 (aka GPIO_ACTIVE_HIGH) unless there's > > > > > an inverter in the middle. > > > > > > > > This depends on the switch IC design and board design, leave 2 > > > > flags (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW) can cover all possible > cases. > > > > > > > > NXP has 2 diff IC parts for this: > > > > 1. PTN36043(used on iMX8MQ) > > > > Output selection control > > > > When SEL=0, RX_AP_*/TX_AP_* are connected to RX_CON_2*/TX_CON_2*, > > > > and > > > > RX_CON_1*/TX_CON_1* are connected to VDD thru low ohmic resistor. > > > > When SEL=1, RX_AP_*/TX_AP_* are connected to RX_CON_1*/TX_CON_1*, > > > > and > > > > RX_CON_2*/TX_CON_2* are connected to VDD thru low ohmic resistor. > > > > > > > > Board design connects RX_CON_1*/TX_CON_1* to typec connector CC1, > > > > so GPIO_ACTIVE_HIGH > > > > > > > > 2. CBTU02043(used on iMX8MP) > > > > SEL Function > > > > -------------------------------------- > > > > Low A to B ports and vice versa > > > > High A to C ports and vice versa > > > > > > > > Board design connects B to typec connector CC1, so GPIO_ACTIVE_LOW > > > > > > > > Therefore, we need 2 flags. > > > > > > I'm not saying you don't. Just that the description is a bit odd. > > > Please expand the description for how one decides how to set the flags. > > > > Misunderstood your point, OK, I thought the "how to set the flags" was > > simple and clear enough: > > Use GPIO_ACTIVE_HIGH if GPIO physical state high is for cc1; or Use > > GPIO_ACTIVE_LOW if GPIO physical state low is for cc1. > > Okay. > > > > > > > +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>; > > > > > > > > > > The data goes from the connector to here and then where? You > > > > > need a connection to the USB host controller. > > > > > > > > The orientation switch only need interact with type-c, no any > > > > interaction with USB controller, do we still need a connection to it? > > > > > > If you have 2 USB hosts and 2 connectors (and 2 muxes), how would > > > you describe which connector goes with which host? > > > > One instance of typec orientation switch defined by this binding only > > for One typec connector. With that, my understanding is Whether a > > connection need be described depends on if the connector (typec > > driver) need notify the host controller driver to do something (e.g. > > role switch need a connection between controller node and connector > > node for controller driver to swap usb role). If the mux/switch > > control is transparent to usb host controller (e.g. my case, usb > > controller drivers normally don't need do anything for orientation > > change), there is no need to describe connection between orientation > > switch node and host controller node. > > There can be several reasons you need to know the association. When writing > the DT you can't assume what information is or isn't needed. > That may vary by h/w or can evolve in an OS and the DT shouldn't change. Okay, I will add the connection to usb controller in example. Thanks Li Jun > > Rob
> -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Monday, November 9, 2020 4:36 PM > To: Jun Li <jun.li@nxp.com> > Cc: 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>; Peter Chen > <peter.chen@nxp.com> > Subject: Re: [PATCH v5 4/4] usb: typec: mux: add typec switch simple driver > > On Tue, Nov 03, 2020 at 07:40:10PM +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. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Hi, Heikki, I have to drop your A-b tag as the driver updated to use mux bindings (drivers/mux/), see v6: https://patchwork.kernel.org/project/linux-usb/patch/1606140096-1382-6-git-send-email-jun.li@nxp.com/ thanks Li Jun
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> --- No changes for v5. 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. previous discussion: http://patchwork.ozlabs.org/patch/1054342/ .../bindings/usb/typec-switch-simple.yaml | 69 ++++++++++++++++++++++ 1 file changed, 69 insertions(+)