Re: [PATCH v4 7/7] leds: leds-pwm: Add device tree bindings

[Grant Likely]

Hi Peter,

Thanks for this work. Comments below...

On Wed, 12 Dec 2012 10:04:52 +0100, Peter Ujfalusi <peter.ujfalusi@xxxxxx> wrote:
> Support for device tree booted kernel.
> 
> For usage see:
> Documentation/devicetree/bindings/leds/leds-pwm.txt
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxx>

About commit text: Remember that commit text is the first thing people
are going to see when looking over what changed in a given subsystem. It
is also where maintainers like me look first when trying to decide if a
patch makes sense before applying it. When commit text is bare-bones
like the above and the patch is non-trivial, then I end up trying to
reconstruct your though process or rational for a patch.

So, please, if it is anything beyond a trivial patch then tell us more
than simply "support for device tree booted kernel". What platform did
you make the change for? How was it tested? Are there any notable design
decisions that you made when adding this support? Are there any missing
pieces or possible bugs?

I'm picking on you at the moment, but this is a general comment. The
commit text is where you get to convince me that the patch is needed and
tell me about how it was designed. This is really important information;
particularly for poor random user, bisecting his broken kernel and
landing on your commit. In this small way, you can make her Christmas
merrier this year.


> ---
>  .../devicetree/bindings/leds/leds-pwm.txt          |  48 +++++++++
>  drivers/leds/leds-pwm.c                            | 112 +++++++++++++++++----
>  2 files changed, 140 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> new file mode 100644
> index 0000000..7297107
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> @@ -0,0 +1,48 @@
> +LED connected to PWM
> +
> +Required properties:
> +- compatible : should be "pwm-leds".
> +
> +Each LED is represented as a sub-node of the pwm-leds device.  Each
> +node's name represents the name of the corresponding LED.
> +
> +LED sub-node properties:
> +- pwms : PWM property to point to the PWM device (phandle)/port (id) and to
> +  specify the period time to be used: <&phandle id period_ns>;
> +- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device
> +  For the pwms and pwm-names property please refer to:
> +  Documentation/devicetree/bindings/pwm/pwm.txt
> +- max-brightness : Maximum brightness possible for the LED
> +- label :  (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger :  (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +twl_pwm: pwm {
> +	/* provides two PWMs (id 0, 1 for PWM1 and PWM2) */
> +	compatible = "ti,twl6030-pwm";
> +	#pwm-cells = <2>;
> +};
> +
> +twl_pwmled: pwmled {
> +	/* provides one PWM (id 0 for Charing indicator LED) */
> +	compatible = "ti,twl6030-pwmled";
> +	#pwm-cells = <2>;
> +};
> +
> +pwmleds {
> +	compatible = "pwm-leds";
> +	kpad {
> +		label = "omap4::keypad";
> +		pwms = <&twl_pwm 0 7812500>;
> +		max-brightness = <127>;
> +	};
> +
> +	charging {
> +		label = "omap4:green:chrg";
> +		pwms = <&twl_pwmled 0 7812500>;
> +		max-brightness = <255>;
> +	};

Acked-by: Grant Likely <grant.likely@xxxxxxxxxxxx>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/