Re: [PATCH 1/2] Add DT bindings documentation for Synopsys UDC driver

2016-12-06 Thread Raviteja Garimella 
Hi Rob,

On Tue, Dec 6, 2016 at 4:34 AM, Rob Herring  wrote:
> On Wed, Nov 30, 2016 at 11:35:09AM +0530, Raviteja Garimella wrote:
>> This patch adds documentation for Synopsis Designware Cores AHB
>> Subsystem Device Controller (UDC).
>>
>> Signed-off-by: Raviteja Garimella 
>> ---
>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt| 29 
>> ++
>>  1 file changed, 29 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt 
>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> new file mode 100644
>> index 000..64e1fbf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> @@ -0,0 +1,29 @@
>> +Synopsys USB Device controller.
>> +
>> +The device node is used for Synopsys Designware Cores AHB
>> +Subsystem Device Controller (UDC).
>> +
>> +Required properties:
>> + - compatible: should be "snps,dw-ahbudc"
>
> Needs an SoC specific compatible string.

This will be changed. I am working on using amd5536udc.c driver
which's already in Kernel tree and can be used for this UDC(as per
review comments from Felipe/John).
>
>> + - reg: Offset and length of UDC register set
>> + - interrupts: description of interrupt line
>> + - phys: phandle to phy node.
>> + - phy-names: name of phy node. Must be usb2drd.
>
> A name is pointless when there's only 1 phy. Is this a device or dual
> role device(DRD)?

This is DRD phy that's is connected to a Host Controller and a Device
Controller.
>
>> + - extcon: phandle to the extcon device
>
> I don't think extcon should be required. If this is UDC only, I'm not
> sure why you'd need it.

This Phy will be initialized in Host/Device mode based on the external
connector that's plugged in. That's reason for having extcon node.
>
>> +
>> +Example:
>> +
>> + usbdrd_phy: phy@6501c000 {
>> + #phy-cells = <0>;
>> + compatible = "brcm,ns2-drd-phy";
>> + reg = <0x6600 0x1000>,
>> + }
>> +
>> + udc_dwc: usb@664e {
>> + compatible = "snps,dw-ahb-udc";
>
> Doesn't match above.

This will be changed.
>
>> + reg = <0x664e 0x2000>;
>> + interrupts = ;
>> + phys = <_phy>;
>> + phy-names = "usb2drd";
>> + extcon = <_phy>";
>
> You are already describing the phy connection, you shouldn't need both.

"extcon_get_edev_by_phandle" requires extcon phandle. I see that's the
only way to get the extcon device, since generic Phy device doesn't
have any extcon member.
The current driver will not go as-is after the suggestion I got in UDC
driver review (to use amd5536udc driver). I will work on the changes
and submit the patches once again.

Thanks,
Ravi
>
>> + };
>> --
>> 2.1.0
>>

Re: [PATCH 1/2] Add DT bindings documentation for Synopsys UDC driver

2016-12-06 Thread Raviteja Garimella 
Hi Rob,

On Tue, Dec 6, 2016 at 4:34 AM, Rob Herring  wrote:
> On Wed, Nov 30, 2016 at 11:35:09AM +0530, Raviteja Garimella wrote:
>> This patch adds documentation for Synopsis Designware Cores AHB
>> Subsystem Device Controller (UDC).
>>
>> Signed-off-by: Raviteja Garimella 
>> ---
>>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt| 29 
>> ++
>>  1 file changed, 29 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt 
>> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> new file mode 100644
>> index 000..64e1fbf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
>> @@ -0,0 +1,29 @@
>> +Synopsys USB Device controller.
>> +
>> +The device node is used for Synopsys Designware Cores AHB
>> +Subsystem Device Controller (UDC).
>> +
>> +Required properties:
>> + - compatible: should be "snps,dw-ahbudc"
>
> Needs an SoC specific compatible string.

This will be changed. I am working on using amd5536udc.c driver
which's already in Kernel tree and can be used for this UDC(as per
review comments from Felipe/John).
>
>> + - reg: Offset and length of UDC register set
>> + - interrupts: description of interrupt line
>> + - phys: phandle to phy node.
>> + - phy-names: name of phy node. Must be usb2drd.
>
> A name is pointless when there's only 1 phy. Is this a device or dual
> role device(DRD)?

This is DRD phy that's is connected to a Host Controller and a Device
Controller.
>
>> + - extcon: phandle to the extcon device
>
> I don't think extcon should be required. If this is UDC only, I'm not
> sure why you'd need it.

This Phy will be initialized in Host/Device mode based on the external
connector that's plugged in. That's reason for having extcon node.
>
>> +
>> +Example:
>> +
>> + usbdrd_phy: phy@6501c000 {
>> + #phy-cells = <0>;
>> + compatible = "brcm,ns2-drd-phy";
>> + reg = <0x6600 0x1000>,
>> + }
>> +
>> + udc_dwc: usb@664e {
>> + compatible = "snps,dw-ahb-udc";
>
> Doesn't match above.

This will be changed.
>
>> + reg = <0x664e 0x2000>;
>> + interrupts = ;
>> + phys = <_phy>;
>> + phy-names = "usb2drd";
>> + extcon = <_phy>";
>
> You are already describing the phy connection, you shouldn't need both.

"extcon_get_edev_by_phandle" requires extcon phandle. I see that's the
only way to get the extcon device, since generic Phy device doesn't
have any extcon member.
The current driver will not go as-is after the suggestion I got in UDC
driver review (to use amd5536udc driver). I will work on the changes
and submit the patches once again.

Thanks,
Ravi
>
>> + };
>> --
>> 2.1.0
>>

Re: [PATCH 1/2] Add DT bindings documentation for Synopsys UDC driver

2016-12-05 Thread Rob Herring 
On Wed, Nov 30, 2016 at 11:35:09AM +0530, Raviteja Garimella wrote:
> This patch adds documentation for Synopsis Designware Cores AHB
> Subsystem Device Controller (UDC).
> 
> Signed-off-by: Raviteja Garimella 
> ---
>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt| 29 
> ++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt 
> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> new file mode 100644
> index 000..64e1fbf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> @@ -0,0 +1,29 @@
> +Synopsys USB Device controller.
> +
> +The device node is used for Synopsys Designware Cores AHB
> +Subsystem Device Controller (UDC).
> +
> +Required properties:
> + - compatible: should be "snps,dw-ahbudc"

Needs an SoC specific compatible string.

> + - reg: Offset and length of UDC register set
> + - interrupts: description of interrupt line
> + - phys: phandle to phy node.
> + - phy-names: name of phy node. Must be usb2drd.

A name is pointless when there's only 1 phy. Is this a device or dual 
role device(DRD)?

> + - extcon: phandle to the extcon device

I don't think extcon should be required. If this is UDC only, I'm not 
sure why you'd need it.

> +
> +Example:
> +
> + usbdrd_phy: phy@6501c000 {
> + #phy-cells = <0>;
> + compatible = "brcm,ns2-drd-phy";
> + reg = <0x6600 0x1000>,
> + }
> +
> + udc_dwc: usb@664e {
> + compatible = "snps,dw-ahb-udc";

Doesn't match above.

> + reg = <0x664e 0x2000>;
> + interrupts = ;
> + phys = <_phy>;
> + phy-names = "usb2drd";
> + extcon = <_phy>";

You are already describing the phy connection, you shouldn't need both.

> + };
> -- 
> 2.1.0
>

Re: [PATCH 1/2] Add DT bindings documentation for Synopsys UDC driver

2016-12-05 Thread Rob Herring 
On Wed, Nov 30, 2016 at 11:35:09AM +0530, Raviteja Garimella wrote:
> This patch adds documentation for Synopsis Designware Cores AHB
> Subsystem Device Controller (UDC).
> 
> Signed-off-by: Raviteja Garimella 
> ---
>  .../devicetree/bindings/usb/snps,dw-ahb-udc.txt| 29 
> ++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt 
> b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> new file mode 100644
> index 000..64e1fbf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/snps,dw-ahb-udc.txt
> @@ -0,0 +1,29 @@
> +Synopsys USB Device controller.
> +
> +The device node is used for Synopsys Designware Cores AHB
> +Subsystem Device Controller (UDC).
> +
> +Required properties:
> + - compatible: should be "snps,dw-ahbudc"

Needs an SoC specific compatible string.

> + - reg: Offset and length of UDC register set
> + - interrupts: description of interrupt line
> + - phys: phandle to phy node.
> + - phy-names: name of phy node. Must be usb2drd.

A name is pointless when there's only 1 phy. Is this a device or dual 
role device(DRD)?

> + - extcon: phandle to the extcon device

I don't think extcon should be required. If this is UDC only, I'm not 
sure why you'd need it.

> +
> +Example:
> +
> + usbdrd_phy: phy@6501c000 {
> + #phy-cells = <0>;
> + compatible = "brcm,ns2-drd-phy";
> + reg = <0x6600 0x1000>,
> + }
> +
> + udc_dwc: usb@664e {
> + compatible = "snps,dw-ahb-udc";

Doesn't match above.

> + reg = <0x664e 0x2000>;
> + interrupts = ;
> + phys = <_phy>;
> + phy-names = "usb2drd";
> + extcon = <_phy>";

You are already describing the phy connection, you shouldn't need both.

> + };
> -- 
> 2.1.0
>