Messages in this thread Patch in this message | | | Date | Wed, 14 Aug 2013 12:58:38 +0200 | From | Thierry Reding <> | Subject | Re: [PATCH v2 4/4] Documentation: dt: bindings: add LP3943 bindings |
| |
On Tue, Jul 30, 2013 at 12:42:29AM +0000, Kim, Milo wrote: [...] > diff --git a/Documentation/devicetree/bindings/gpio/gpio-lp3943.txt b/Documentation/devicetree/bindings/gpio/gpio-lp3943.txt [...] > +Required properties: > + - compatible: "ti,lp3943-gpio" > + - gpio-controller: device node as a GPIO controller
"Marks the device node as a GPIO controller"?
> + - #gpio-cells: Should be two.
"Should be 2"?
[...] > + gpios = <&gpioex 9 1>; /* active-low */ [...] > + gpios = <&gpioex 10 1>; /* active-low */
If you use the defines as Lee suggested, then you don't need these comments either.
> diff --git a/Documentation/devicetree/bindings/mfd/lp3943.txt b/Documentation/devicetree/bindings/mfd/lp3943.txt > new file mode 100644 > index 0000000..ef62784 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/lp3943.txt > @@ -0,0 +1,33 @@ > +TI/National Semiconductor LP3943 MFD driver > + > +Required properties: > + - compatible: "ti,lp3943" > + - reg: I2C slave address (u8). 0x60 ~ 0x67 u8 is a Linux specific and implementation detail. I think it's safe to assume that everybody knows that I2C slave addresses are 7 bits (and you even provide a valid range), so just drop the "(u8)".
> +lp3943@60 { > + compatible = "ti,lp3943"; > + reg = <0x60>; > + > + gpioex: gpio { > + compatible = "ti,lp3943-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + > + pwm3943: pwm { > + compatible = "ti,lp3943-pwm"; > + #pwm-cells = <3>; > + ti,pwm0 = < 8 9 10 >;
This should probably be written as "<8 9 10>" for consistency.
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-lp3943.txt b/Documentation/devicetree/bindings/pwm/pwm-lp3943.txt [...] > +Required properties: > + - compatible: "ti,lp3943-pwm" > + - #pwm-cells: Should be three.
"Should be three"? Although since as I mentioned the chip doesn't support polarity inversion, this should now be "2".
> + 1) index of PWM controller > + 0 = PWM 0, 1 = PWM 1 > + 2) PWM signal period in nanoseconds > + min = 6250, max = 1600000 > + 3) bitmask of optional PWM polarity flags > + 0 = normal, 1 = inversed
All of this can probably go away. The new way of doing this is to reference the standard bindings document using something like the below:
- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of the cells format.
Since you have a restriction on the minimum and maximum period lengths, you could add something like:
Note that this hardware limits the period length to the range 6250-1600000. > + - ti,pwm0 or ti,pwm1: Output pin number(s) for PWM port 0 or 1.
"PWM port" -> "PWM channel"
> + 0 = invalid > + 1 = output 0 > + 2 = output 1 > + . > + . > + 16 = output 15
I thought I read that this is now a 1:1 mapping?
> +Example: > +PWM 0 is for RGB LED brightness control > +PWM 1 is for brightness control of LP8557 backlight device > + > +&i2c3 { > + lp3943@60 { > + compatible = "ti,lp3943"; > + reg = <0x60>; > + > + /* > + * PWM 0 : output 7, 8 and 9 > + * PWM 1 : output 15 > + */ > + pwm3943: pwm { > + compatible = "ti,lp3943-pwm"; > + #pwm-cells = <3>; > + ti,pwm0 = < 8 9 10 >;
And here as well: "< 8 9 10 >" -> "<8 9 10>".
Thierry [unhandled content-type:application/pgp-signature] | |