Re: [PATCH v4 7/7] leds: leds-pwm: Add device tree bindings
Hi Peter,
Thanks for this work. Comments below...
On Wed, 12 Dec 2012 10:04:52 +0100, Peter Ujfalusi
wrote:
> Support for device tree booted kernel.
>
> For usage see:
> Documentation/devicetree/bindings/leds/leds-pwm.txt
>
> Signed-off-by: Peter Ujfalusi
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 000..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: < 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 = <_pwm 0 7812500>;
> + max-brightness = <127>;
> + };
> +
> + charging {
> + label = "omap4:green:chrg";
> + pwms = <_pwmled 0 7812500>;
> + max-brightness = <255>;
> + };
Acked-by: Grant Likely
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 7/7] leds: leds-pwm: Add device tree bindings
Hi Peter,
Thanks for this work. Comments below...
On Wed, 12 Dec 2012 10:04:52 +0100, Peter Ujfalusi peter.ujfal...@ti.com
wrote:
Support for device tree booted kernel.
For usage see:
Documentation/devicetree/bindings/leds/leds-pwm.txt
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
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 000..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.lik...@secretlab.ca
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 7/7] leds: leds-pwm: Add device tree bindings
Support for device tree booted kernel.
For usage see:
Documentation/devicetree/bindings/leds/leds-pwm.txt
Signed-off-by: Peter Ujfalusi
---
.../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 000..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: < 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 = <_pwm 0 7812500>;
+ max-brightness = <127>;
+ };
+
+ charging {
+ label = "omap4:green:chrg";
+ pwms = <_pwmled 0 7812500>;
+ max-brightness = <255>;
+ };
+};
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 46f4e44..a1ea5f6 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -58,46 +59,110 @@ static inline size_t sizeof_pwm_leds_priv(int num_leds)
(sizeof(struct led_pwm_data) * num_leds);
}
-static int led_pwm_probe(struct platform_device *pdev)
+static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev)
{
- struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
+ struct device_node *node = pdev->dev.of_node;
+ struct device_node *child;
struct led_pwm_priv *priv;
- int i, ret = 0;
+ int count, ret;
- if (!pdata)
- return -EBUSY;
+ /* count LEDs in this device, so we know how much to allocate */
+ count = of_get_child_count(node);
+ if (!count)
+ return NULL;
- priv = devm_kzalloc(>dev, sizeof_pwm_leds_priv(pdata->num_leds),
+ priv = devm_kzalloc(>dev, sizeof_pwm_leds_priv(count),
GFP_KERNEL);
if (!priv)
- return -ENOMEM;
+ return NULL;
- for (i = 0; i < pdata->num_leds; i++) {
- struct led_pwm *cur_led = >leds[i];
- struct led_pwm_data *led_dat = >leds[i];
+ for_each_child_of_node(node, child) {
+ struct led_pwm_data *led_dat = >leds[priv->num_leds];
- led_dat->pwm = devm_pwm_get(>dev, cur_led->name);
+ led_dat->cdev.name = of_get_property(child, "label",
+NULL) ? : child->name;
+
+ led_dat->pwm = devm_of_pwm_get(>dev, child, NULL);
if (IS_ERR(led_dat->pwm)) {
- ret = PTR_ERR(led_dat->pwm);
dev_err(>dev, "unable to request PWM for %s\n",
- cur_led->name);
+ led_dat->cdev.name);
goto err;
}
+ /* Get the period from PWM core when n*/
+ led_dat->period = pwm_get_period(led_dat->pwm);
+
+ led_dat->cdev.default_trigger = of_get_property(child,
+ "linux,default-trigger", NULL);
+ of_property_read_u32(child, "max-brightness",
+_dat->cdev.max_brightness);
- led_dat->cdev.name = cur_led->name;
- led_dat->cdev.default_trigger = cur_led->default_trigger;
- led_dat->active_low = cur_led->active_low;
- led_dat->period = cur_led->pwm_period_ns;
led_dat->cdev.brightness_set = led_pwm_set;
[PATCH v4 7/7] leds: leds-pwm: Add device tree bindings
Support for device tree booted kernel.
For usage see:
Documentation/devicetree/bindings/leds/leds-pwm.txt
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
.../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 000..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;
+ };
+};
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 46f4e44..a1ea5f6 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -16,6 +16,7 @@
#include linux/kernel.h
#include linux/init.h
#include linux/platform_device.h
+#include linux/of_platform.h
#include linux/fb.h
#include linux/leds.h
#include linux/err.h
@@ -58,46 +59,110 @@ static inline size_t sizeof_pwm_leds_priv(int num_leds)
(sizeof(struct led_pwm_data) * num_leds);
}
-static int led_pwm_probe(struct platform_device *pdev)
+static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev)
{
- struct led_pwm_platform_data *pdata = pdev-dev.platform_data;
+ struct device_node *node = pdev-dev.of_node;
+ struct device_node *child;
struct led_pwm_priv *priv;
- int i, ret = 0;
+ int count, ret;
- if (!pdata)
- return -EBUSY;
+ /* count LEDs in this device, so we know how much to allocate */
+ count = of_get_child_count(node);
+ if (!count)
+ return NULL;
- priv = devm_kzalloc(pdev-dev, sizeof_pwm_leds_priv(pdata-num_leds),
+ priv = devm_kzalloc(pdev-dev, sizeof_pwm_leds_priv(count),
GFP_KERNEL);
if (!priv)
- return -ENOMEM;
+ return NULL;
- for (i = 0; i pdata-num_leds; i++) {
- struct led_pwm *cur_led = pdata-leds[i];
- struct led_pwm_data *led_dat = priv-leds[i];
+ for_each_child_of_node(node, child) {
+ struct led_pwm_data *led_dat = priv-leds[priv-num_leds];
- led_dat-pwm = devm_pwm_get(pdev-dev, cur_led-name);
+ led_dat-cdev.name = of_get_property(child, label,
+NULL) ? : child-name;
+
+ led_dat-pwm = devm_of_pwm_get(pdev-dev, child, NULL);
if (IS_ERR(led_dat-pwm)) {
- ret = PTR_ERR(led_dat-pwm);
dev_err(pdev-dev, unable to request PWM for %s\n,
- cur_led-name);
+ led_dat-cdev.name);
goto err;
}
+ /* Get the period from PWM core when n*/
+ led_dat-period = pwm_get_period(led_dat-pwm);
+
+ led_dat-cdev.default_trigger = of_get_property(child,
+ linux,default-trigger, NULL);
+ of_property_read_u32(child, max-brightness,
+led_dat-cdev.max_brightness);
- led_dat-cdev.name = cur_led-name;
- led_dat-cdev.default_trigger = cur_led-default_trigger;
- led_dat-active_low = cur_led-active_low;
-
