[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

On Mon, Mar 18, 2019 at 10:59:31AM +0000, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: 2019年3月13日 17:36
> > To: Rob Herring <robh@kernel.org>
> > Cc: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> > andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> > devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> > via GPIO
> > 
> > On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> > > On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > > > Some typec super speed active channel switch can be controlled via a
> > > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > > and the remote endpoint of its consumer.
> > > >
> > > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > > > ++++++++++++++++++++++
> > > >  1 file changed, 30 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > new file mode 100644
> > > > index 0000000..4ef76cf
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > > @@ -0,0 +1,30 @@
> > > > +Typec orientation switch via a GPIO
> > > > +-----------------------------------
> > > > +
> > > > +Required properties:
> > > > +- compatible: should be set one of following:
> > > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > > +
> > > > +- gpios: the GPIO used to switch the super speed active channel,
> > >
> > > Perhaps switch-gpios in case there are other gpios needed.
> > >
> > > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > > +- orientation-switch: must be present.
> > >
> > > Why is this needed? The compatible can't imply this?
> > 
> > I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c,
> > so I'm to blame here. If we can handle this with the compatible like I guess we can,
> > I'm happy.
> 
> Hi Heikki
> 
> Can I just remove the original bool property check? i.e, match OK if the remote
> parent node is in switch_list.

No. If typec_switch_get() is called before the mux device is
registered, we need to return -EPROBE_DEFER. That means we need to be
able to identify the mux device node.

I think we should just use the compatible like Rob suggested. The
Type-C muxes should probable have their own bindings file where it's
defined for these muxes.


thanks,

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: 2019年3月13日 17:36
> To: Rob Herring <robh@kernel.org>
> Cc: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; hdegoede@redhat.com;
> andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> > On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > > ++++++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 0000000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +-----------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> >
> > Perhaps switch-gpios in case there are other gpios needed.
> >
> > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> >
> > Why is this needed? The compatible can't imply this?
> 
> I think Jun Li is added that based on the comment I put to drivers/usb/typec/mux.c,
> so I'm to blame here. If we can handle this with the compatible like I guess we can,
> I'm happy.

Hi Heikki

Can I just remove the original bool property check? i.e, match OK if the remote
parent node is in switch_list.

Thanks
Li Jun
> 
> thanks,
> 
> --
> heikki

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2019年3月12日 22:45
> To: Jun Li <jun.li@nxp.com>
> Cc: heikki.krogerus@linux.intel.com; gregkh@linuxfoundation.org;
> hdegoede@redhat.com; andy.shevchenko@gmail.com; linux-usb@vger.kernel.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via a
> > GPIO, this binding can be used to specify the switch node by a GPIO
> > and the remote endpoint of its consumer.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> 
> Perhaps switch-gpios in case there are other gpios needed.

OK, I will change it to be switch-gpios.

> 
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Why is this needed? The compatible can't imply this?

I will try to remove this bool property and use a general compatible string.

> 
> > +
> > +Required sub-node:
> > +- port: specify the remote endpoint of typec switch consumer.
> > +
> > +Example:
> > +
> > +ptn36043 {
> > +	compatible = "nxp,ptn36043";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +	orientation-switch;
> > +
> > +	port {
> > +		usb3_data_ss: endpoint {
> > +			remote-endpoint = <&typec_con_ss>;
> > +		};
> > +	};
> > +};
> > --
> > 2.7.4
> >

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 2019年3月11日 19:12
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> Hi,
> 
> On 11-03-19 12:02, Hans de Goede wrote:
> > Hi,
> >
> > On 11-03-19 11:40, Jun Li wrote:
> >> Some typec super speed active channel switch can be controlled via a
> >> GPIO, this binding can be used to specify the switch node by a GPIO
> >> and the remote endpoint of its consumer.
> >>
> >> Signed-off-by: Li Jun <jun.li@nxp.com>
> >> ---
> >>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> >> ++++++++++++++++++++++
> >>   1 file changed, 30 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> new file mode 100644
> >> index 0000000..4ef76cf
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> >> @@ -0,0 +1,30 @@
> >> +Typec orientation switch via a GPIO
> >> +-----------------------------------
> >> +
> >> +Required properties:
> >> +- compatible: should be set one of following:
> >> +    - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> 
> Hmm, it seems that this binding should work fine with other orientation-switches as
> well, so I think this needs a generic compatible string.
> 
> >> +
> >> +- gpios: the GPIO used to switch the super speed active channel,
> >> +        GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> >> +        GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> >> +- orientation-switch: must be present.
> >
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch  ?
> 
> Also perhaps it would be better to use an additional compatible string for this, rather
> then a boolean property, because what you are trying to say is that this device is
> compatible with some (to be written) generic usb-c-orientation-switch binding.
> 
> So I think you may want to use an extra compatible for this and describe the
> port/graph usage linking the usb-c-connector port and the port on the
> orientation-switch together in a new usb-c-orientation-switch binding document.
> 

This patch was to only cover one kind of *typical* typec switch: done by
GPIO toggle, as I don't know how other typec switch may be implemented,
I will try to change this to be a *common* typec switch by using a generic
compatible(type-c-orientation-switch), which will for now only support
switch-gpios.

> This new binding will then document the port usage which is mostly undocumented
> in your typec-switch-gpio.txt binding and this port usage documentation can then be
> re-used for other orientation-switch bindings.

Port usage should be the same as I gave the example:
https://www.spinics.net/lists/devicetree/msg278042.html

thanks
Li Jun
> 
> Regards,
> 
> Hans
> 
> 
> >
> >> +
> >> +Required sub-node:
> >> +- port: specify the remote endpoint of typec switch consumer.
> >> +
> >> +Example:
> >> +
> >> +ptn36043 {
> >> +    compatible = "nxp,ptn36043";
> >> +    pinctrl-names = "default";
> >> +    pinctrl-0 = <&pinctrl_ss_sel>;
> >> +    gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> >> +    orientation-switch;
> >> +
> >> +    port {
> >> +        usb3_data_ss: endpoint {
> >> +            remote-endpoint = <&typec_con_ss>;
> >
> >
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the
> > other way around?  Both will work in the end. but to me it feels more
> > natural to group all the info about the type-c connector together in
> > the "usb-c-connector" compatible port
> >
> > Regards,
> >
> > Hans
> >

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

On Tue, Mar 12, 2019 at 09:45:27AM -0500, Rob Herring wrote:
> On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via
> > a GPIO, this binding can be used to specify the switch node by
> > a GPIO and the remote endpoint of its consumer.
> > 
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> 
> Perhaps switch-gpios in case there are other gpios needed.
> 
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Why is this needed? The compatible can't imply this?

I think Jun Li is added that based on the comment I put to
drivers/usb/typec/mux.c, so I'm to blame here. If we can handle this
with the compatible like I guess we can, I'm happy.

thanks,

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Rob Herring]

On Mon, Mar 11, 2019 at 10:40:09AM +0000, Jun Li wrote:
> Some typec super speed active channel switch can be controlled via
> a GPIO, this binding can be used to specify the switch node by
> a GPIO and the remote endpoint of its consumer.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>  .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> new file mode 100644
> index 0000000..4ef76cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> @@ -0,0 +1,30 @@
> +Typec orientation switch via a GPIO
> +-----------------------------------
> +
> +Required properties:
> +- compatible: should be set one of following:
> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> +
> +- gpios: the GPIO used to switch the super speed active channel,

Perhaps switch-gpios in case there are other gpios needed.

> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> +- orientation-switch: must be present.

Why is this needed? The compatible can't imply this?

> +
> +Required sub-node:
> +- port: specify the remote endpoint of typec switch consumer.
> +
> +Example:
> +
> +ptn36043 {
> +	compatible = "nxp,ptn36043";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ss_sel>;
> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +	orientation-switch;
> +
> +	port {
> +		usb3_data_ss: endpoint {
> +			remote-endpoint = <&typec_con_ss>;
> +		};
> +	};
> +};
> -- 
> 2.7.4
>

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 12-03-19 11:45, Heikki Krogerus wrote:
> Hi,
> 
> On Tue, Mar 12, 2019 at 10:32:00AM +0000, Jun Li wrote:
>> Hi Hans
>>> -----Original Message-----
>>> From: Hans de Goede <hdegoede@redhat.com>
>>> Sent: 2019年3月11日 19:03
>>> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
>>> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
>>> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
>>> <linux-imx@nxp.com>
>>> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
>>> via GPIO
>>>
>>> Hi,
>>>
>>> On 11-03-19 11:40, Jun Li wrote:
>>>> Some typec super speed active channel switch can be controlled via a
>>>> GPIO, this binding can be used to specify the switch node by a GPIO
>>>> and the remote endpoint of its consumer.
>>>>
>>>> Signed-off-by: Li Jun <jun.li@nxp.com>
>>>> ---
>>>>    .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
>>> ++++++++++++++++++++++
>>>>    1 file changed, 30 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> new file mode 100644
>>>> index 0000000..4ef76cf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>>>> @@ -0,0 +1,30 @@
>>>> +Typec orientation switch via a GPIO
>>>> +-----------------------------------
>>>> +
>>>> +Required properties:
>>>> +- compatible: should be set one of following:
>>>> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
>>>> +
>>>> +- gpios: the GPIO used to switch the super speed active channel,
>>>> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
>>>> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
>>>> +- orientation-switch: must be present.
>>>
>>> Shouldn't this have usb-c in the propery name, e.g.:
>>> usb-c-orientation-switch  ?
>>
>> This is decided by drivers/usb/typec/mux.c:36
>> /*
>>   * With OF graph the mux node must have a boolean device property named
>>   * "orientation-switch".
>>   */
> 
> Yes, but it's still OK to change it. It's not documented anywhere yet.
> 
>>>> +
>>>> +Required sub-node:
>>>> +- port: specify the remote endpoint of typec switch consumer.
>>>> +
>>>> +Example:
>>>> +
>>>> +ptn36043 {
>>>> +	compatible = "nxp,ptn36043";
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&pinctrl_ss_sel>;
>>>> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>>>> +	orientation-switch;
>>>> +
>>>> +	port {
>>>> +		usb3_data_ss: endpoint {
>>>> +			remote-endpoint = <&typec_con_ss>;
>>>
>>>
>>> Isn't this the wrong way around, shouldn't the "usb-c-connector"
>>> compatible port be pointing to the orientation switch, rather then the other way
>>> around?
> 
> Hans, in OF graph both endpoints will have a remote-endpoint pointing
> to each other..
> 
>> I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
>> yes, it is pointing to the orientation switch provider(i.e, this example node).
>>
>>> Both will work in the end. but to me it feels more natural to group all the
>>> info about the type-c connector together in the "usb-c-connector" compatible port
>>>
>>
>>          ptn36043 {
>>                  compatible = "nxp,ptn36043";
>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&pinctrl_ss_sel>;
>>                  gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>>                  orientation-switch;
>>
>>                  port {
>>                          usb3_data_ss: endpoint {
>>                                  remote-endpoint = <&typec_con_ss>;
>>                          };
>>                  };
>>          };
>>
>> 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_data_ss>;
>> 			};
>> 		};
>> 	};
>> };
> 
> So like that.

Ah I see, thank you for clarifying that.

Regards,

Hans

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Heikki Krogerus]

Hi,

On Tue, Mar 12, 2019 at 10:32:00AM +0000, Jun Li wrote:
> Hi Hans
> > -----Original Message-----
> > From: Hans de Goede <hdegoede@redhat.com>
> > Sent: 2019年3月11日 19:03
> > To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> > Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> > linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> > via GPIO
> > 
> > Hi,
> > 
> > On 11-03-19 11:40, Jun Li wrote:
> > > Some typec super speed active channel switch can be controlled via a
> > > GPIO, this binding can be used to specify the switch node by a GPIO
> > > and the remote endpoint of its consumer.
> > >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> > ++++++++++++++++++++++
> > >   1 file changed, 30 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > new file mode 100644
> > > index 0000000..4ef76cf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > > @@ -0,0 +1,30 @@
> > > +Typec orientation switch via a GPIO
> > > +-----------------------------------
> > > +
> > > +Required properties:
> > > +- compatible: should be set one of following:
> > > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > > +
> > > +- gpios: the GPIO used to switch the super speed active channel,
> > > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > > +- orientation-switch: must be present.
> > 
> > Shouldn't this have usb-c in the propery name, e.g.:
> > usb-c-orientation-switch  ?
> 
> This is decided by drivers/usb/typec/mux.c:36
> /*
>  * With OF graph the mux node must have a boolean device property named
>  * "orientation-switch".
>  */

Yes, but it's still OK to change it. It's not documented anywhere yet.

> > > +
> > > +Required sub-node:
> > > +- port: specify the remote endpoint of typec switch consumer.
> > > +
> > > +Example:
> > > +
> > > +ptn36043 {
> > > +	compatible = "nxp,ptn36043";
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > > +	orientation-switch;
> > > +
> > > +	port {
> > > +		usb3_data_ss: endpoint {
> > > +			remote-endpoint = <&typec_con_ss>;
> > 
> > 
> > Isn't this the wrong way around, shouldn't the "usb-c-connector"
> > compatible port be pointing to the orientation switch, rather then the other way
> > around?  

Hans, in OF graph both endpoints will have a remote-endpoint pointing
to each other..

> I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
> yes, it is pointing to the orientation switch provider(i.e, this example node).
> 
> >Both will work in the end. but to me it feels more natural to group all the
> > info about the type-c connector together in the "usb-c-connector" compatible port
> >
> 
>         ptn36043 {
>                 compatible = "nxp,ptn36043";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_ss_sel>;
>                 gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>                 orientation-switch;
> 
>                 port {
>                         usb3_data_ss: endpoint {
>                                 remote-endpoint = <&typec_con_ss>;
>                         };
>                 };
>         };
> 
> 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_data_ss>;
> 			};
> 		};
> 	};
> };

So like that.

thanks,

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

Hi Hans
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: 2019年3月11日 19:03
> To: Jun Li <jun.li@nxp.com>; robh+dt@kernel.org; heikki.krogerus@linux.intel.com
> Cc: gregkh@linuxfoundation.org; andy.shevchenko@gmail.com;
> linux-usb@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/2] dt-bindings: usb: add documentation for typec switch
> via GPIO
> 
> Hi,
> 
> On 11-03-19 11:40, Jun Li wrote:
> > Some typec super speed active channel switch can be controlled via a
> > GPIO, this binding can be used to specify the switch node by a GPIO
> > and the remote endpoint of its consumer.
> >
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> > ---
> >   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30
> ++++++++++++++++++++++
> >   1 file changed, 30 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > new file mode 100644
> > index 0000000..4ef76cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> > @@ -0,0 +1,30 @@
> > +Typec orientation switch via a GPIO
> > +-----------------------------------
> > +
> > +Required properties:
> > +- compatible: should be set one of following:
> > +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> > +
> > +- gpios: the GPIO used to switch the super speed active channel,
> > +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> > +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> > +- orientation-switch: must be present.
> 
> Shouldn't this have usb-c in the propery name, e.g.:
> usb-c-orientation-switch  ?

This is decided by drivers/usb/typec/mux.c:36
/*
 * With OF graph the mux node must have a boolean device property named
 * "orientation-switch".
 */
> 
> > +
> > +Required sub-node:
> > +- port: specify the remote endpoint of typec switch consumer.
> > +
> > +Example:
> > +
> > +ptn36043 {
> > +	compatible = "nxp,ptn36043";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_ss_sel>;
> > +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> > +	orientation-switch;
> > +
> > +	port {
> > +		usb3_data_ss: endpoint {
> > +			remote-endpoint = <&typec_con_ss>;
> 
> 
> Isn't this the wrong way around, shouldn't the "usb-c-connector"
> compatible port be pointing to the orientation switch, rather then the other way
> around?  

I am not sure I am getting your point, "usb-c-connector" is the user of typec switch,
yes, it is pointing to the orientation switch provider(i.e, this example node).

>Both will work in the end. but to me it feels more natural to group all the
> info about the type-c connector together in the "usb-c-connector" compatible port
>

        ptn36043 {
                compatible = "nxp,ptn36043";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_ss_sel>;
                gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
                orientation-switch;

                port {
                        usb3_data_ss: endpoint {
                                remote-endpoint = <&typec_con_ss>;
                        };
                };
        };

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

Regards
Jun
> Regards,
> 
> Hans

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 11-03-19 12:02, Hans de Goede wrote:
> Hi,
> 
> On 11-03-19 11:40, Jun Li wrote:
>> Some typec super speed active channel switch can be controlled via
>> a GPIO, this binding can be used to specify the switch node by
>> a GPIO and the remote endpoint of its consumer.
>>
>> Signed-off-by: Li Jun <jun.li@nxp.com>
>> ---
>>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>> new file mode 100644
>> index 0000000..4ef76cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
>> @@ -0,0 +1,30 @@
>> +Typec orientation switch via a GPIO
>> +-----------------------------------
>> +
>> +Required properties:
>> +- compatible: should be set one of following:
>> +    - "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.

Hmm, it seems that this binding should work fine with
other orientation-switches as well, so I think this needs
a generic compatible string.

>> +
>> +- gpios: the GPIO used to switch the super speed active channel,
>> +        GPIO_ACTIVE_HIGH: GPIO state high for cc1;
>> +        GPIO_ACTIVE_LOW:  GPIO state low for cc1.
>> +- orientation-switch: must be present.
> 
> Shouldn't this have usb-c in the propery name, e.g.:
> usb-c-orientation-switch  ?

Also perhaps it would be better to use an additional compatible
string for this, rather then a boolean property, because what you
are trying to say is that this device is compatible with some
(to be written) generic usb-c-orientation-switch binding.

So I think you may want to use an extra compatible for this
and describe the port/graph usage linking the usb-c-connector port
and the port on the orientation-switch together in a new
usb-c-orientation-switch binding document.

This new binding will then document the port usage which is
mostly undocumented in your typec-switch-gpio.txt binding and
this port usage documentation can then be re-used for other
orientation-switch bindings.

Regards,

Hans


> 
>> +
>> +Required sub-node:
>> +- port: specify the remote endpoint of typec switch consumer.
>> +
>> +Example:
>> +
>> +ptn36043 {
>> +    compatible = "nxp,ptn36043";
>> +    pinctrl-names = "default";
>> +    pinctrl-0 = <&pinctrl_ss_sel>;
>> +    gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
>> +    orientation-switch;
>> +
>> +    port {
>> +        usb3_data_ss: endpoint {
>> +            remote-endpoint = <&typec_con_ss>;
> 
> 
> Isn't this the wrong way around, shouldn't the "usb-c-connector"
> compatible port be pointing to the orientation switch, rather then
> the other way around?  Both will work in the end. but to me it
> feels more natural to group all the info about the type-c connector
> together in the "usb-c-connector" compatible port
> 
> Regards,
> 
> Hans
>

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Hans de Goede]

Hi,

On 11-03-19 11:40, Jun Li wrote:
> Some typec super speed active channel switch can be controlled via
> a GPIO, this binding can be used to specify the switch node by
> a GPIO and the remote endpoint of its consumer.
> 
> Signed-off-by: Li Jun <jun.li@nxp.com>
> ---
>   .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> new file mode 100644
> index 0000000..4ef76cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
> @@ -0,0 +1,30 @@
> +Typec orientation switch via a GPIO
> +-----------------------------------
> +
> +Required properties:
> +- compatible: should be set one of following:
> +	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
> +
> +- gpios: the GPIO used to switch the super speed active channel,
> +		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
> +		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
> +- orientation-switch: must be present.

Shouldn't this have usb-c in the propery name, e.g.:
usb-c-orientation-switch  ?

> +
> +Required sub-node:
> +- port: specify the remote endpoint of typec switch consumer.
> +
> +Example:
> +
> +ptn36043 {
> +	compatible = "nxp,ptn36043";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ss_sel>;
> +	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
> +	orientation-switch;
> +
> +	port {
> +		usb3_data_ss: endpoint {
> +			remote-endpoint = <&typec_con_ss>;


Isn't this the wrong way around, shouldn't the "usb-c-connector"
compatible port be pointing to the orientation switch, rather then
the other way around?  Both will work in the end. but to me it
feels more natural to group all the info about the type-c connector
together in the "usb-c-connector" compatible port

Regards,

Hans

[v3,1/2] dt-bindings: usb: add documentation for typec switch via GPIO

[Jun Li]

Some typec super speed active channel switch can be controlled via
a GPIO, this binding can be used to specify the switch node by
a GPIO and the remote endpoint of its consumer.

Signed-off-by: Li Jun <jun.li@nxp.com>
---
 .../devicetree/bindings/usb/typec-switch-gpio.txt  | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
new file mode 100644
index 0000000..4ef76cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/typec-switch-gpio.txt
@@ -0,0 +1,30 @@
+Typec orientation switch via a GPIO
+-----------------------------------
+
+Required properties:
+- compatible: should be set one of following:
+	- "nxp,ptn36043" for NXP Type-C SuperSpeed active switch.
+
+- gpios: the GPIO used to switch the super speed active channel,
+		GPIO_ACTIVE_HIGH: GPIO state high for cc1;
+		GPIO_ACTIVE_LOW:  GPIO state low for cc1.
+- orientation-switch: must be present.
+
+Required sub-node:
+- port: specify the remote endpoint of typec switch consumer.
+
+Example:
+
+ptn36043 {
+	compatible = "nxp,ptn36043";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_ss_sel>;
+	gpios = <&gpio3 15 GPIO_ACTIVE_HIGH>;
+	orientation-switch;
+
+	port {
+		usb3_data_ss: endpoint {
+			remote-endpoint = <&typec_con_ss>;
+		};
+	};
+};