Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation
On Sun, Jun 29, 2014 at 10:29:49AM +0100, Robert Jarzmik wrote: > Mark Rutland writes: > > > On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote: > >> > The name of the clock input doesn't make sense. > >> I don't understand. With [1] does it make any more sense ? If not you'll > >> have to > >> expand a bit more the "doesn't make sense". > > > > My concern is that clock-names is supposed to describe the name of the > > input clock line from the view of the IP block. "pxa27x-udc" doesn't > > sound like the name of a clock input line from the view of the UDC > > block. > > > > I assume the clock input line you care about has a more specific name > > than "pxa27x-udc"? > Not as far as I know. The technical reference manual call it "udc clock", so > it's even "less" specific ... Not from the point of view of the device. The clock-names are namespaced to the particular binding, so they're equally specific. Given the above I'd recommend naming the clock "udc" or "udc_clk". That doesn't pretend to be overly specific, and matches the TRM. > Marvell engineers have probably the internal schematics and the name of the > clock, but outsiders like me only have "udc" ... Sure, not having the full specs is always a pain. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation
Mark Rutland writes: > On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote: >> > The name of the clock input doesn't make sense. >> I don't understand. With [1] does it make any more sense ? If not you'll >> have to >> expand a bit more the "doesn't make sense". > > My concern is that clock-names is supposed to describe the name of the > input clock line from the view of the IP block. "pxa27x-udc" doesn't > sound like the name of a clock input line from the view of the UDC > block. > > I assume the clock input line you care about has a more specific name > than "pxa27x-udc"? Not as far as I know. The technical reference manual call it "udc clock", so it's even "less" specific ... Marvell engineers have probably the internal schematics and the name of the clock, but outsiders like me only have "udc" ... Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation
On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote:
> Mark Rutland writes:
>
> > On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
> >> index 79729a9..0e4863f 100644
> >> --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
> >> +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
> >> @@ -29,3 +29,23 @@ Example:
> >>marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
> >>};
> >>
> >> +UDC
> >> +
> >> +Required properties:
> >> + - compatible: Should be "marvell,pxa27x-udc" for USB controllers
> >> + used in device mode.
> >
> > We typically don't like wildcard compatible strings in DT.
> Same as in the other patch, "marvell,pxa270-udc".
> >
> >> +
> >> +Optional properties:
> >> + - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
> >> + - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
> >
> > Use the GPIO bindings.
> OK. I'm presuming in this case you think of something like :
> udc-pullup-gpio = <&gpio 22 GPIO_ACTIVE_LOW>
> Which translates into:
> "udc-pullup-gpio" - gpio activated to control the USB D+ pullup (see
> gpio.txt)
Something like that, yes.
> >
> >> +
> >> +Example:
> >> +
> >> + pxa27x_udc: udc@4060 {
> >> + compatible = "marvell,pxa27x-udc";
> >> + reg = <0x4060 0x1>;
> >> + interrupts = <11>;
> >> + clocks = <&pxa2xx_clks 11>;
> >> + clock-names = "pxa27x-udc";
> >
> > Neither of these were mentioned above.
> Ah you mean I shall describe the standard reg, interrupts as mandatory
> standard
> options I take it. OK.
Yes. While the properties are standard, their precise meanings (e.g.
which interrupt or address space region), and whether a particular
binding uses them is not.
> > The name of the clock input doesn't make sense.
> I don't understand. With [1] does it make any more sense ? If not you'll have
> to
> expand a bit more the "doesn't make sense".
My concern is that clock-names is supposed to describe the name of the
input clock line from the view of the IP block. "pxa27x-udc" doesn't
sound like the name of a clock input line from the view of the UDC
block.
I assume the clock input line you care about has a more specific name
than "pxa27x-udc"?
Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation
Mark Rutland writes:
> On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
>> index 79729a9..0e4863f 100644
>> --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
>> @@ -29,3 +29,23 @@ Example:
>> marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
>> };
>>
>> +UDC
>> +
>> +Required properties:
>> + - compatible: Should be "marvell,pxa27x-udc" for USB controllers
>> + used in device mode.
>
> We typically don't like wildcard compatible strings in DT.
Same as in the other patch, "marvell,pxa270-udc".
>
>> +
>> +Optional properties:
>> + - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
>> + - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
>
> Use the GPIO bindings.
OK. I'm presuming in this case you think of something like :
udc-pullup-gpio = <&gpio 22 GPIO_ACTIVE_LOW>
Which translates into:
"udc-pullup-gpio" - gpio activated to control the USB D+ pullup (see
gpio.txt)
>
>> +
>> +Example:
>> +
>> +pxa27x_udc: udc@4060 {
>> +compatible = "marvell,pxa27x-udc";
>> +reg = <0x4060 0x1>;
>> +interrupts = <11>;
>> +clocks = <&pxa2xx_clks 11>;
>> +clock-names = "pxa27x-udc";
>
> Neither of these were mentioned above.
Ah you mean I shall describe the standard reg, interrupts as mandatory standard
options I take it. OK.
>
> The name of the clock input doesn't make sense.
I don't understand. With [1] does it make any more sense ? If not you'll have to
expand a bit more the "doesn't make sense".
Cheers.
--
Robert
[1] http://www.spinics.net/lists/arm-kernel/msg337522.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation
On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
> Add documentation for device-tree binding of arm PXA 27x udc (usb
> device) driver.
>
> Signed-off-by: Robert Jarzmik
> Cc: devicet...@vger.kernel.org
>
> ---
> Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
> This is a consequence of other DT reviews on the marvell
> namings.
> ---
> Documentation/devicetree/bindings/usb/pxa-usb.txt | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/pxa-usb.txt
> b/Documentation/devicetree/bindings/usb/pxa-usb.txt
> index 79729a9..0e4863f 100644
> --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
> @@ -29,3 +29,23 @@ Example:
> marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
> };
>
> +UDC
> +
> +Required properties:
> + - compatible: Should be "marvell,pxa27x-udc" for USB controllers
> + used in device mode.
We typically don't like wildcard compatible strings in DT.
> +
> +Optional properties:
> + - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
> + - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
Use the GPIO bindings.
> +
> +Example:
> +
> + pxa27x_udc: udc@4060 {
> + compatible = "marvell,pxa27x-udc";
> + reg = <0x4060 0x1>;
> + interrupts = <11>;
> + clocks = <&pxa2xx_clks 11>;
> + clock-names = "pxa27x-udc";
Neither of these were mentioned above.
The name of the clock input doesn't make sense.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation
Add documentation for device-tree binding of arm PXA 27x udc (usb
device) driver.
Signed-off-by: Robert Jarzmik
Cc: devicet...@vger.kernel.org
---
Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
This is a consequence of other DT reviews on the marvell
namings.
---
Documentation/devicetree/bindings/usb/pxa-usb.txt | 20
1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/pxa-usb.txt
b/Documentation/devicetree/bindings/usb/pxa-usb.txt
index 79729a9..0e4863f 100644
--- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
+++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
@@ -29,3 +29,23 @@ Example:
marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
};
+UDC
+
+Required properties:
+ - compatible: Should be "marvell,pxa27x-udc" for USB controllers
+ used in device mode.
+
+Optional properties:
+ - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
+ - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
+
+Example:
+
+ pxa27x_udc: udc@4060 {
+ compatible = "marvell,pxa27x-udc";
+ reg = <0x4060 0x1>;
+ interrupts = <11>;
+ clocks = <&pxa2xx_clks 11>;
+ clock-names = "pxa27x-udc";
+ udc-pullup-gpio = <22>;
+ };
--
2.0.0.rc2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
