Hi Rob > -----Original Message----- > From: Jun Li > Sent: Tuesday, December 1, 2020 11:13 AM > To: Rob Herring <robh@xxxxxxxxxx> > Cc: heikki.krogerus@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; > hdegoede@xxxxxxxxxx; lee.jones@xxxxxxxxxx; > mika.westerberg@xxxxxxxxxxxxxxx; dmitry.torokhov@xxxxxxxxx; > prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx; > laurent.pinchart+renesas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Peter Chen > <peter.chen@xxxxxxx> > Subject: RE: [PATCH v6 1/6] dt-bindings: usb: add documentation for typec > switch simple driver > > > > > -----Original Message----- > > From: Rob Herring <robh@xxxxxxxxxx> > > Sent: Tuesday, December 1, 2020 5:47 AM > > To: Jun Li <jun.li@xxxxxxx> > > Cc: heikki.krogerus@xxxxxxxxxxxxxxx; rafael@xxxxxxxxxx; > > gregkh@xxxxxxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; > > hdegoede@xxxxxxxxxx; lee.jones@xxxxxxxxxx; > > mika.westerberg@xxxxxxxxxxxxxxx; dmitry.torokhov@xxxxxxxxx; > > prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx; > > laurent.pinchart+renesas@xxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > > devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Peter > > Chen <peter.chen@xxxxxxx> > > Subject: Re: [PATCH v6 1/6] dt-bindings: usb: add documentation for > > typec switch simple driver > > > > On Mon, Nov 23, 2020 at 10:01:31PM +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@xxxxxxx> > > > --- > > > changes for v6: > > > - Use general mux bindings for typec switch, one typec switch > > > may have 1 or 2 GPIOs for channel selection, if 1 GPIO, only > > > can be used to select cc1 or cc2; if 2 GPIOs, the second GPIO > > > can be used to deselect both channels or keep normal operations. > > > - Add one more connection to usb controller. > > > > > > 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%7C4436142512ac45fef86f08d8957983c2%7C686ea1d3bc2b4c6fa92cd99c5c3016 > > > > > > 35%7C0%7C1%7C637423696442152522%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj > > > > > > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata= > > > JCV9OtFkfLv4bH2LIxfnk0lVIO6j830%2F%2Bx2OMOi%2Fs6o%3D&reserved=0 > > > > > > .../bindings/usb/typec-switch-simple.yaml | 122 > > +++++++++++++++++++++ > > > 1 file changed, 122 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..030ade5 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-simple.yaml > > > @@ -0,0 +1,122 @@ > > > +# 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%7C4436142512ac45fef86f08d8957983c2%7C686ea1d3 > > > > > > +bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637423696442152522%7CUnknown%7CTWF > > > > > > +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 > > > > > > +Mn0%3D%7C3000&sdata=vSLcoGYWCgwpsi4LWbKo2VTyCVgN%2BA7zjHfE8QCS4rg > > > +%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%7C4436142512ac45fef86f08d8957983c2%7C686ea1d3bc2b4c6fa92cd99c > > > > > > +5c301635%7C0%7C1%7C637423696442152522%7CUnknown%7CTWFpbGZsb3d8eyJWIjo > > > > > > +iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am > > > > > > +p;sdata=0fd3wjAtKwLCQAY3NG2Be4qNA9FiFhtO0HIZmCK7AQs%3D&reserved=0 > > > + > > > +title: Typec Orientation Switch Simple Solution Bindings > > > + > > > +maintainers: > > > + - Li Jun <jun.li@xxxxxxx> > > > + > > > +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 > > > + > > > + '#mux-control-cells': > > > + const: 0 > > > > Who is the mux-control consumer? > > Typec(controller) > > ptn5110: tcpc@50 { > compatible = "nxp,ptn5110"; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_typec>; > reg = <0x50>; > interrupt-parent = <&gpio4>; > interrupts = <19 8>; > mux-controls = <&typec_switch>; > mux-control-names = "mux-typec-switch"; > > usb_con: connector { > compatible = "usb-c-connector"; > ... ... > ports { > #address-cells = <1>; > #size-cells = <0>; > > port@1 { > reg = <1>; > typec_con_ss: endpoint { > remote-endpoint = > <&usb3_orien_sel>; > }; > }; > }; > }; > }; > > > > > > + > > > + switch-gpios: > > > + description: | > > > + GPIO specifiers to select the target channel of mux. > > > + The first GPIO is for cc1 and cc2 selection, the GPIO flag 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. > > > + The second gpio is to deselect any channles by places all channels > > > + in high-impedance state to reduce current consumption, the > > > + gpio > > flag > > > + use GPIO_ACTIVE_HIGH if GPIO physical state high is for > > > + high-impedance state (so low for normal operations); or Use > > > + GPIO_ACTIVE_LOW if GPIO physical state low is for high-impedance > > > + state(so high for normal operations). > > > + minItems: 1 > > > + maxItems: 2 > > > + > > > + idle-state: > > > + description: -| > > > + For TYPEC_ORIENTATION_NONE by deselect both channels if supported. > > > + const: 2 > > > + > > > + # Standard properties described in # > > > + Documentation/devicetree/bindings/mux/mux-controller.txt > > > + > > > + port: > > > + type: object > > > + additionalProperties: false > > > + description: -| > > > + Connection to the remote endpoint using OF graph bindings that > model > > > + SS data bus to typec connector. > > > + > > > + properties: > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > + endpoint@0: > > > + type: object > > > + description: Endpoint connected to typec connector. > > > + additionalProperties: false > > > + > > > + properties: > > > + reg: > > > + const: 0 > > > + remote-endpoint: true > > > + > > > + required: > > > + - remote-endpoint > > > + > > > + endpoint@1: > > > + type: object > > > + description: Endpoint connected to usb controller. > > > + additionalProperties: false > > > + > > > + properties: > > > + reg: > > > + const: 1 > > > + remote-endpoint: true > > > + > > > + required: > > > + - remote-endpoint > > > + > > > + required: > > > + - endpoint@0 > > > + > > > +required: > > > + - compatible > > > + - port > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/gpio/gpio.h> > > > + typec_switch: mux-controller { > > > + compatible = "typec-orientation-switch"; > > > + #mux-control-cells = <0>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_ss_sel>; > > > + switch-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>, > > > + <&gpio2 20 GPIO_ACTIVE_HIGH>; > > > + idle-state = <2>; > > > + > > > + port { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + usb3_orien_sel: endpoint@0 { > > > + reg = <0>; > > > + remote-endpoint = <&typec_con_ss>; > > > + }; > > > + > > > + usb3_con_data: endpoint@1 { > > > + reg = <1>; > > > + remote-endpoint = <&dwc3_0>; > > > + }; > > > + }; > > > > This still seems unnecessarily complicated. What I had in mind is just > > something like this: > > > > mux: mux-controller { > > compatible = "gpio-mux"; > > #mux-control-cells = <0>; > > mux-gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>, > > <&gpio2 20 GPIO_ACTIVE_HIGH>; > > idle-state = <2>; > > }; > > > > connector { > > compatible = "usb-c-connector"; > > mux-controls = <&mux>; > > ... > > }; > > > > Then you aren't re-implementing the gpio-mux driver. > > > > This should work for *any* mux implementation as long as the mux > > states always have the same meanings from the perspective of the connector. > > > > Of course, I have little visibility into the possible h/w > > implementations for Type-C and maybe this isn't sufficient. So input > > from Type-C experts would be helpful. More thinking of this, I have a proposal like this: 34 mux: mux-controller { 33 compatible = "gpio-mux"; 32 #mux-control-cells = <0>; 29 switch-gpios = <&gpio4 20 GPIO_ACTIVE_LOW>, 28 <&gpio2 20 GPIO_ACTIVE_HIGH>; 27 idle-state = <2>; 26 }; 25 Then I don't need re-implement the gpio-mux driver. 24 typec_switch { 23 compatible = "typec-orientation-switch"; 22 mux-controls = <&mux>; 21 mux-control-names = "mux-typec-switch"; 20 19 port { 18 usb3_data_ss: endpoint@0 { 17 remote-endpoint = <&typec_con_ss>; 16 }; 15 }; 14 }; The simple typec switch driver as the consumer of mux-control, and also as the provider of typec_switch to typec class, this can be extended to use other mux solutions. 31 usb_con: connector { 30 compatible = "usb-c-connector"; 29 label = "USB-C"; 20 ... ... 19 ports { 18 #address-cells = <1>; 17 #size-cells = <0>; 16 15 port@1 { 14 reg = <1>; 13 typec_con_ss: endpoint { 12 remote-endpoint = <&usb3_data_ss>; 11 }; 10 }; 9 }; 8 }; Then existing typec mux interface can be kept, is this making sense to you? Thanks Li Jun > > From h/w point view, this can work if only consider my(GPIO) case. > But I still need register a typec switch in provider side to make it under > the unified typec switch interface: > > 7 struct typec_switch { > 6 struct device dev; > 5 typec_switch_set_fn_t set; > 4 struct mux_control *mux_ctrl; > 3 }; > > typec_switch->dev.parent = mux_chip->dev; > > So either I put the typec switch creation in exiting mux-gpio driver; or > I create a new typec switch driver with gpio mux code duplication. > > If I go directly with mux control w/o(out of) typec_switch, then I don't > need any driver change as you expect, but I guess this is not preferred, > @Heikki, any comments? > > Thanks > Li Jun > > > > > Rob
