[PATCH v2 0/3] leds: leds-pwm: Device tree support

[PATCH v2 0/3] leds: leds-pwm: Device tree support

[Peter Ujfalusi]

Hello,

The patch:
https://lkml.org/lkml/2012/11/7/187
grown to be a series to add DeviceTree support for the leds-pwm driver.

Changes since v1:
- As suggested by Bryan Wu: the legacy pwm_request() has been removed from
  patch 1
- Device tree bindings added for leds-pwm driver.

When we boot with Device tree we handle one LED per device to be more aligned
with PWM core's DT implementation.
An example of the DT usage is provided in the new DT binding documentation for
leds-pwm.

Tested on OMAP4 Blaze (SDP) with legacy and DT boot.

Regards,
Peter
---
Peter Ujfalusi (3):
  leds: leds-pwm: Convert to use devm_get_pwm
  leds: leds-pwm: Preparing the driver for device tree support
  leds: leds-pwm: Add device tree bindings

 .../devicetree/bindings/leds/leds-pwm.txt          |  34 +++++
 drivers/leds/leds-pwm.c                            | 161 ++++++++++++++-------
 include/linux/leds_pwm.h                           |   2 +-
 3 files changed, 146 insertions(+), 51 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt

-- 
1.8.0

[PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

Update the driver to use the new API for requesting pwm so we can take
advantage of the pwm_lookup table to find the correct pwm to be used for the
LED functionality.
If the devm_get_pwm fails we fall back to legacy mode to try to get the pwm.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/leds/leds-pwm.c  | 19 ++++++-------------
 include/linux/leds_pwm.h |  2 +-
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index f2e44c7..c953c75 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -67,12 +67,11 @@ static int led_pwm_probe(struct platform_device *pdev)
 		cur_led = &pdata->leds[i];
 		led_dat = &leds_data[i];
 
-		led_dat->pwm = pwm_request(cur_led->pwm_id,
-				cur_led->name);
+		led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
 		if (IS_ERR(led_dat->pwm)) {
 			ret = PTR_ERR(led_dat->pwm);
-			dev_err(&pdev->dev, "unable to request PWM %d\n",
-					cur_led->pwm_id);
+			dev_err(&pdev->dev, "unable to request PWM for %s\n",
+				cur_led->name);
 			goto err;
 		}
 
@@ -86,10 +85,8 @@ static int led_pwm_probe(struct platform_device *pdev)
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 
 		ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-		if (ret < 0) {
-			pwm_free(led_dat->pwm);
+		if (ret < 0)
 			goto err;
-		}
 	}
 
 	platform_set_drvdata(pdev, leds_data);
@@ -98,10 +95,8 @@ static int led_pwm_probe(struct platform_device *pdev)
 
 err:
 	if (i > 0) {
-		for (i = i - 1; i >= 0; i--) {
+		for (i = i - 1; i >= 0; i--)
 			led_classdev_unregister(&leds_data[i].cdev);
-			pwm_free(leds_data[i].pwm);
-		}
 	}
 
 	return ret;
@@ -115,10 +110,8 @@ static int __devexit led_pwm_remove(struct platform_device *pdev)
 
 	leds_data = platform_get_drvdata(pdev);
 
-	for (i = 0; i < pdata->num_leds; i++) {
+	for (i = 0; i < pdata->num_leds; i++)
 		led_classdev_unregister(&leds_data[i].cdev);
-		pwm_free(leds_data[i].pwm);
-	}
 
 	return 0;
 }
diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
index 33a0711..a65e964 100644
--- a/include/linux/leds_pwm.h
+++ b/include/linux/leds_pwm.h
@@ -7,7 +7,7 @@
 struct led_pwm {
 	const char	*name;
 	const char	*default_trigger;
-	unsigned	pwm_id;
+	unsigned	pwm_id __deprecated;
 	u8 		active_low;
 	unsigned 	max_brightness;
 	unsigned	pwm_period_ns;
-- 
1.8.0

[PATCH v2 2/3] leds: leds-pwm: Preparing the driver for device tree support

[Peter Ujfalusi]

In order to be able to add device tree support for leds-pwm driver we need
to rearrange the data structures used by the drivers.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/leds/leds-pwm.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index c953c75..b219ea9 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -30,6 +30,11 @@ struct led_pwm_data {
 	unsigned int		period;
 };
 
+struct led_pwm_priv {
+	int num_leds;
+	struct led_pwm_data leds[];
+};
+
 static void led_pwm_set(struct led_classdev *led_cdev,
 	enum led_brightness brightness)
 {
@@ -47,25 +52,29 @@ static void led_pwm_set(struct led_classdev *led_cdev,
 	}
 }
 
+static inline int sizeof_pwm_leds_priv(int num_leds)
+{
+	return sizeof(struct led_pwm_priv) +
+		      (sizeof(struct led_pwm_data) * num_leds);
+}
+
 static int led_pwm_probe(struct platform_device *pdev)
 {
 	struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
-	struct led_pwm *cur_led;
-	struct led_pwm_data *leds_data, *led_dat;
+	struct led_pwm_priv *priv;
 	int i, ret = 0;
 
 	if (!pdata)
 		return -EBUSY;
 
-	leds_data = devm_kzalloc(&pdev->dev,
-			sizeof(struct led_pwm_data) * pdata->num_leds,
-				GFP_KERNEL);
-	if (!leds_data)
+	priv = devm_kzalloc(&pdev->dev, sizeof_pwm_leds_priv(pdata->num_leds),
+			    GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
 	for (i = 0; i < pdata->num_leds; i++) {
-		cur_led = &pdata->leds[i];
-		led_dat = &leds_data[i];
+		struct led_pwm *cur_led = &pdata->leds[i];
+		struct led_pwm_data *led_dat = &priv->leds[i];
 
 		led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
 		if (IS_ERR(led_dat->pwm)) {
@@ -88,15 +97,16 @@ static int led_pwm_probe(struct platform_device *pdev)
 		if (ret < 0)
 			goto err;
 	}
+	priv->num_leds = pdata->num_leds;
 
-	platform_set_drvdata(pdev, leds_data);
+	platform_set_drvdata(pdev, priv);
 
 	return 0;
 
 err:
 	if (i > 0) {
 		for (i = i - 1; i >= 0; i--)
-			led_classdev_unregister(&leds_data[i].cdev);
+			led_classdev_unregister(&priv->leds[i].cdev);
 	}
 
 	return ret;
@@ -104,14 +114,11 @@ err:
 
 static int __devexit led_pwm_remove(struct platform_device *pdev)
 {
+	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
 	int i;
-	struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
-	struct led_pwm_data *leds_data;
-
-	leds_data = platform_get_drvdata(pdev);
 
-	for (i = 0; i < pdata->num_leds; i++)
-		led_classdev_unregister(&leds_data[i].cdev);
+	for (i = 0; i < priv->num_leds; i++)
+		led_classdev_unregister(&priv->leds[i].cdev);
 
 	return 0;
 }
-- 
1.8.0

[PATCH v2 3/3] leds: leds-pwm: Add device tree bindings

[Peter Ujfalusi]

Support for device tree booted kernel.
When the kernel is booted with DeviceTree blob we support one led per
leds-pwm device to have cleaner integration with the PWM subsystem.

For usage see:
Documentation/devicetree/bindings/leds/leds-pwm.txt

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../devicetree/bindings/leds/leds-pwm.txt          |  34 ++++++
 drivers/leds/leds-pwm.c                            | 125 +++++++++++++++------
 2 files changed, 127 insertions(+), 32 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..9fe3040
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
@@ -0,0 +1,34 @@
+LED connected to PWM
+
+Required properties:
+- compatible : should be "pwm-leds".
+- pwms : PWM property, please refer to: 
+  Documentation/devicetree/bindings/pwm/pwm.txt
+- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device
+- label : (optional) The label for this LED.  If omitted, the label is
+  taken from the node name (excluding the unit address).
+- max-brightness : Maximum brightness possible for the LED
+- linux,default-trigger :  (optional) This parameter, if present, is a
+  string defining the trigger assigned to the LED.  Current triggers are:
+    "backlight" - LED will act as a back-light, controlled by the framebuffer
+		  system
+    "default-on" - LED will turn on, but see "default-state" below
+    "heartbeat" - LED "double" flashes at a load average based rate
+    "ide-disk" - LED indicates disk activity
+    "timer" - LED flashes at a fixed, configurable rate
+
+Example:
+
+twl_pwm: pwm {
+	compatible = "ti,twl6030-pwm";
+	#pwm-cells = <2>;
+};
+
+pwmled_keypad {
+	compatible = "pwm-leds";
+	pwms = <&twl_pwm 0 7812500>;
+	pwm-names = "Keypad LED";
+
+	label = "omap4::keypad";
+	max-brightness = <127>;
+};
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index b219ea9..333c942 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,98 @@ static inline int sizeof_pwm_leds_priv(int num_leds)
 		      (sizeof(struct led_pwm_data) * num_leds);
 }
 
+static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct led_pwm_priv *priv;
+	struct led_pwm_data *led_dat;
+	int ret;
+
+	/* With DT boot we support one LED per leds-pwm device */
+	priv = devm_kzalloc(&pdev->dev, sizeof_pwm_leds_priv(1),
+			    GFP_KERNEL);
+	if (!priv)
+		return NULL;
+
+	led_dat = &priv->leds[priv->num_leds];
+	led_dat->cdev.name = of_get_property(node, "label", NULL) ? : node->name;
+	led_dat->pwm = devm_pwm_get(&pdev->dev, NULL);
+	if (IS_ERR(led_dat->pwm)) {
+		dev_err(&pdev->dev, "unable to request PWM for %s\n",
+			led_dat->cdev.name);
+		return NULL;
+	}
+	/* 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(node,
+							"linux,default-trigger",
+							NULL);
+	of_property_read_u32(node, "max-brightness",
+			     &led_dat->cdev.max_brightness);
+
+	led_dat->cdev.brightness_set = led_pwm_set;
+	led_dat->cdev.brightness = LED_OFF;
+	led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+
+	ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register for %s\n",
+			led_dat->cdev.name);
+		of_node_put(node);
+		return NULL;
+	}
+
+	priv->num_leds = 1;
+
+	return priv;
+}
+
 static int led_pwm_probe(struct platform_device *pdev)
 {
 	struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
 	struct led_pwm_priv *priv;
 	int i, ret = 0;
 
-	if (!pdata)
-		return -EBUSY;
-
-	priv = devm_kzalloc(&pdev->dev, sizeof_pwm_leds_priv(pdata->num_leds),
-			    GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	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];
-
-		led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
-		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);
-			goto err;
+	if (pdata && pdata->num_leds) {
+		priv = devm_kzalloc(&pdev->dev,
+				    sizeof_pwm_leds_priv(pdata->num_leds),
+				    GFP_KERNEL);
+		if (!priv)
+			return -ENOMEM;
+
+		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];
+
+			led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
+			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);
+				goto err;
+			}
+
+			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;
+			led_dat->cdev.brightness = LED_OFF;
+			led_dat->cdev.max_brightness = cur_led->max_brightness;
+			led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
+
+			ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
+			if (ret < 0)
+				goto err;
 		}
-
-		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;
-		led_dat->cdev.brightness = LED_OFF;
-		led_dat->cdev.max_brightness = cur_led->max_brightness;
-		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
-
-		ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
-		if (ret < 0)
-			goto err;
+		priv->num_leds = pdata->num_leds;
+	} else {
+		priv = led_pwm_create_of(pdev);
+		if (!priv)
+			return -ENODEV;
 	}
-	priv->num_leds = pdata->num_leds;
 
 	platform_set_drvdata(pdev, priv);
 
@@ -123,12 +176,20 @@ static int __devexit led_pwm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id of_pwm_leds_match[] = {
+	{ .compatible = "pwm-leds", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_pwm_leds_match);
+
 static struct platform_driver led_pwm_driver = {
 	.probe		= led_pwm_probe,
 	.remove		= __devexit_p(led_pwm_remove),
 	.driver		= {
 		.name	= "leds_pwm",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(of_pwm_leds_match),
 	},
 };
 
-- 
1.8.0

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Bryan Wu]

On Mon, Nov 12, 2012 at 6:41 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Update the driver to use the new API for requesting pwm so we can take
> advantage of the pwm_lookup table to find the correct pwm to be used for the
> LED functionality.
> If the devm_get_pwm fails we fall back to legacy mode to try to get the pwm.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/leds/leds-pwm.c  | 19 ++++++-------------
>  include/linux/leds_pwm.h |  2 +-
>  2 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index f2e44c7..c953c75 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -67,12 +67,11 @@ static int led_pwm_probe(struct platform_device *pdev)
>                 cur_led = &pdata->leds[i];
>                 led_dat = &leds_data[i];
>
> -               led_dat->pwm = pwm_request(cur_led->pwm_id,
> -                               cur_led->name);
> +               led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
>                 if (IS_ERR(led_dat->pwm)) {
>                         ret = PTR_ERR(led_dat->pwm);
> -                       dev_err(&pdev->dev, "unable to request PWM %d\n",
> -                                       cur_led->pwm_id);
> +                       dev_err(&pdev->dev, "unable to request PWM for %s\n",
> +                               cur_led->name);
>                         goto err;
>                 }
>
> @@ -86,10 +85,8 @@ static int led_pwm_probe(struct platform_device *pdev)
>                 led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>
>                 ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
> -               if (ret < 0) {
> -                       pwm_free(led_dat->pwm);
> +               if (ret < 0)
>                         goto err;
> -               }
>         }
>
>         platform_set_drvdata(pdev, leds_data);
> @@ -98,10 +95,8 @@ static int led_pwm_probe(struct platform_device *pdev)
>
>  err:
>         if (i > 0) {
> -               for (i = i - 1; i >= 0; i--) {
> +               for (i = i - 1; i >= 0; i--)
>                         led_classdev_unregister(&leds_data[i].cdev);
> -                       pwm_free(leds_data[i].pwm);
> -               }
>         }
>
>         return ret;
> @@ -115,10 +110,8 @@ static int __devexit led_pwm_remove(struct platform_device *pdev)
>
>         leds_data = platform_get_drvdata(pdev);
>
> -       for (i = 0; i < pdata->num_leds; i++) {
> +       for (i = 0; i < pdata->num_leds; i++)
>                 led_classdev_unregister(&leds_data[i].cdev);
> -               pwm_free(leds_data[i].pwm);
> -       }
>
>         return 0;
>  }
> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
> index 33a0711..a65e964 100644
> --- a/include/linux/leds_pwm.h
> +++ b/include/linux/leds_pwm.h
> @@ -7,7 +7,7 @@
>  struct led_pwm {
>         const char      *name;
>         const char      *default_trigger;
> -       unsigned        pwm_id;
> +       unsigned        pwm_id __deprecated;

I suggest we remove this later, we can provide patches from this from
platform data of board file. And I think this patch is good for me to
merge, will do it soon.

Thanks,
-Bryan

>         u8              active_low;
>         unsigned        max_brightness;
>         unsigned        pwm_period_ns;
> --
> 1.8.0
>

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

Hi Bryan,

On 11/14/2012 02:14 AM, Bryan Wu wrote:
>> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
>> index 33a0711..a65e964 100644
>> --- a/include/linux/leds_pwm.h
>> +++ b/include/linux/leds_pwm.h
>> @@ -7,7 +7,7 @@
>>  struct led_pwm {
>>         const char      *name;
>>         const char      *default_trigger;
>> -       unsigned        pwm_id;
>> +       unsigned        pwm_id __deprecated;
> 
> I suggest we remove this later, we can provide patches from this from
> platform data of board file. And I think this patch is good for me to
> merge, will do it soon.

Yes, we can remove it in 3.9. I marked it as deprecated for now to avoid
breakage - if any - in 3.8.

Thank you,
Péter

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

Hi Bryan,

On 11/14/2012 02:14 AM, Bryan Wu wrote:
> On Mon, Nov 12, 2012 at 6:41 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> Update the driver to use the new API for requesting pwm so we can take
>> advantage of the pwm_lookup table to find the correct pwm to be used for the
>> LED functionality.
>> If the devm_get_pwm fails we fall back to legacy mode to try to get the pwm.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/leds/leds-pwm.c  | 19 ++++++-------------
>>  include/linux/leds_pwm.h |  2 +-
>>  2 files changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>> index f2e44c7..c953c75 100644
>> --- a/drivers/leds/leds-pwm.c
>> +++ b/drivers/leds/leds-pwm.c
>> @@ -67,12 +67,11 @@ static int led_pwm_probe(struct platform_device *pdev)
>>                 cur_led = &pdata->leds[i];
>>                 led_dat = &leds_data[i];
>>
>> -               led_dat->pwm = pwm_request(cur_led->pwm_id,
>> -                               cur_led->name);
>> +               led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
>>                 if (IS_ERR(led_dat->pwm)) {
>>                         ret = PTR_ERR(led_dat->pwm);
>> -                       dev_err(&pdev->dev, "unable to request PWM %d\n",
>> -                                       cur_led->pwm_id);
>> +                       dev_err(&pdev->dev, "unable to request PWM for %s\n",
>> +                               cur_led->name);
>>                         goto err;
>>                 }
>>
>> @@ -86,10 +85,8 @@ static int led_pwm_probe(struct platform_device *pdev)
>>                 led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>>
>>                 ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
>> -               if (ret < 0) {
>> -                       pwm_free(led_dat->pwm);
>> +               if (ret < 0)
>>                         goto err;
>> -               }
>>         }
>>
>>         platform_set_drvdata(pdev, leds_data);
>> @@ -98,10 +95,8 @@ static int led_pwm_probe(struct platform_device *pdev)
>>
>>  err:
>>         if (i > 0) {
>> -               for (i = i - 1; i >= 0; i--) {
>> +               for (i = i - 1; i >= 0; i--)
>>                         led_classdev_unregister(&leds_data[i].cdev);
>> -                       pwm_free(leds_data[i].pwm);
>> -               }
>>         }
>>
>>         return ret;
>> @@ -115,10 +110,8 @@ static int __devexit led_pwm_remove(struct platform_device *pdev)
>>
>>         leds_data = platform_get_drvdata(pdev);
>>
>> -       for (i = 0; i < pdata->num_leds; i++) {
>> +       for (i = 0; i < pdata->num_leds; i++)
>>                 led_classdev_unregister(&leds_data[i].cdev);
>> -               pwm_free(leds_data[i].pwm);
>> -       }
>>
>>         return 0;
>>  }
>> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
>> index 33a0711..a65e964 100644
>> --- a/include/linux/leds_pwm.h
>> +++ b/include/linux/leds_pwm.h
>> @@ -7,7 +7,7 @@
>>  struct led_pwm {
>>         const char      *name;
>>         const char      *default_trigger;
>> -       unsigned        pwm_id;
>> +       unsigned        pwm_id __deprecated;
> 
> I suggest we remove this later, we can provide patches from this from
> platform data of board file. And I think this patch is good for me to
> merge, will do it soon.

I have marked the pwm_id as deprecated for now to allow one kernel cycle for
external (?) drivers to adopt. We can remove it for 3.9 I think.

I just checked linux-next today and this series is still not there. Do you
want me to resend it for you? I can rebase the patches on top of linux-next or
if you have preference on which tree should I use please let me know.

> 
> Thanks,
> -Bryan
> 
>>         u8              active_low;
>>         unsigned        max_brightness;
>>         unsigned        pwm_period_ns;
>> --
>> 1.8.0
>>


Thanks,
Péter

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Bryan Wu]

On Mon, Dec 3, 2012 at 6:13 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Hi Bryan,
>
> On 11/14/2012 02:14 AM, Bryan Wu wrote:
>> On Mon, Nov 12, 2012 at 6:41 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>> Update the driver to use the new API for requesting pwm so we can take
>>> advantage of the pwm_lookup table to find the correct pwm to be used for the
>>> LED functionality.
>>> If the devm_get_pwm fails we fall back to legacy mode to try to get the pwm.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>>  drivers/leds/leds-pwm.c  | 19 ++++++-------------
>>>  include/linux/leds_pwm.h |  2 +-
>>>  2 files changed, 7 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>>> index f2e44c7..c953c75 100644
>>> --- a/drivers/leds/leds-pwm.c
>>> +++ b/drivers/leds/leds-pwm.c
>>> @@ -67,12 +67,11 @@ static int led_pwm_probe(struct platform_device *pdev)
>>>                 cur_led = &pdata->leds[i];
>>>                 led_dat = &leds_data[i];
>>>
>>> -               led_dat->pwm = pwm_request(cur_led->pwm_id,
>>> -                               cur_led->name);
>>> +               led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
>>>                 if (IS_ERR(led_dat->pwm)) {
>>>                         ret = PTR_ERR(led_dat->pwm);
>>> -                       dev_err(&pdev->dev, "unable to request PWM %d\n",
>>> -                                       cur_led->pwm_id);
>>> +                       dev_err(&pdev->dev, "unable to request PWM for %s\n",
>>> +                               cur_led->name);
>>>                         goto err;
>>>                 }
>>>
>>> @@ -86,10 +85,8 @@ static int led_pwm_probe(struct platform_device *pdev)
>>>                 led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>>>
>>>                 ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
>>> -               if (ret < 0) {
>>> -                       pwm_free(led_dat->pwm);
>>> +               if (ret < 0)
>>>                         goto err;
>>> -               }
>>>         }
>>>
>>>         platform_set_drvdata(pdev, leds_data);
>>> @@ -98,10 +95,8 @@ static int led_pwm_probe(struct platform_device *pdev)
>>>
>>>  err:
>>>         if (i > 0) {
>>> -               for (i = i - 1; i >= 0; i--) {
>>> +               for (i = i - 1; i >= 0; i--)
>>>                         led_classdev_unregister(&leds_data[i].cdev);
>>> -                       pwm_free(leds_data[i].pwm);
>>> -               }
>>>         }
>>>
>>>         return ret;
>>> @@ -115,10 +110,8 @@ static int __devexit led_pwm_remove(struct platform_device *pdev)
>>>
>>>         leds_data = platform_get_drvdata(pdev);
>>>
>>> -       for (i = 0; i < pdata->num_leds; i++) {
>>> +       for (i = 0; i < pdata->num_leds; i++)
>>>                 led_classdev_unregister(&leds_data[i].cdev);
>>> -               pwm_free(leds_data[i].pwm);
>>> -       }
>>>
>>>         return 0;
>>>  }
>>> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
>>> index 33a0711..a65e964 100644
>>> --- a/include/linux/leds_pwm.h
>>> +++ b/include/linux/leds_pwm.h
>>> @@ -7,7 +7,7 @@
>>>  struct led_pwm {
>>>         const char      *name;
>>>         const char      *default_trigger;
>>> -       unsigned        pwm_id;
>>> +       unsigned        pwm_id __deprecated;
>>
>> I suggest we remove this later, we can provide patches from this from
>> platform data of board file. And I think this patch is good for me to
>> merge, will do it soon.
>
> I have marked the pwm_id as deprecated for now to allow one kernel cycle for
> external (?) drivers to adopt. We can remove it for 3.9 I think.
>
> I just checked linux-next today and this series is still not there. Do you
> want me to resend it for you? I can rebase the patches on top of linux-next or
> if you have preference on which tree should I use please let me know.
>

Actually, I'm waiting for some feedback from DT maintainers about this
new binding. But it looks find to me.
I plan to put this series into my -devel branch and target for 3.9, is
that OK for you? Right now, it's -rc7. It's better to put this new
thing for next cycle.

-Bryan

>>
>> Thanks,
>> -Bryan
>>
>>>         u8              active_low;
>>>         unsigned        max_brightness;
>>>         unsigned        pwm_period_ns;
>>> --
>>> 1.8.0
>>>
>
>
> Thanks,
> Péter

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

Hi Bryan,

On 12/03/2012 07:32 PM, Bryan Wu wrote:
> On Mon, Dec 3, 2012 at 6:13 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Actually, I'm waiting for some feedback from DT maintainers about this
> new binding. But it looks find to me.

Would it be possible to have the first (or even better the first two) patch
going for 3.8 and we can leave the DT bindings for 3.9?
I would like to clean up, fix some of the OMAP related board files for 3.9
which would need the patches for the leds-pwm driver itself.

Thank you,
Péter

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Bryan Wu]

On Tue, Dec 4, 2012 at 12:11 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Hi Bryan,
>
> On 12/03/2012 07:32 PM, Bryan Wu wrote:
>> On Mon, Dec 3, 2012 at 6:13 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> Actually, I'm waiting for some feedback from DT maintainers about this
>> new binding. But it looks find to me.
>
> Would it be possible to have the first (or even better the first two) patch
> going for 3.8 and we can leave the DT bindings for 3.9?
> I would like to clean up, fix some of the OMAP related board files for 3.9
> which would need the patches for the leds-pwm driver itself.
>

What's best thing I can do is I can merge these patchset into my
-devel branch, after 3.8 merge window close I will move this -devel
branch to my for-next branch for 3.9 merge window.

Does this make sense to you?

Thanks,
-Bryan

Re: [PATCH v2 1/3] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

On 12/04/2012 06:34 PM, Bryan Wu wrote:
> What's best thing I can do is I can merge these patchset into my
> -devel branch, after 3.8 merge window close I will move this -devel
> branch to my for-next branch for 3.9 merge window.
> 
> Does this make sense to you?

Not sure if I can get changes related to leds-pwm through linux-omap in this
way. The practice is that l-o should be 'in a working condition' alone -
without linux-next.

But I'll find something else while waiting for 3.10 to clean up the omap board
files related to this.

-- 
Péter

Re: [PATCH v2 3/3] leds: leds-pwm: Add device tree bindings

[Grant Likely]

On Mon, 12 Nov 2012 15:41:10 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Support for device tree booted kernel.
> When the kernel is booted with DeviceTree blob we support one led per
> leds-pwm device to have cleaner integration with the PWM subsystem.
> 
> For usage see:
> Documentation/devicetree/bindings/leds/leds-pwm.txt
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  .../devicetree/bindings/leds/leds-pwm.txt          |  34 ++++++
>  drivers/leds/leds-pwm.c                            | 125 +++++++++++++++------
>  2 files changed, 127 insertions(+), 32 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..9fe3040
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> @@ -0,0 +1,34 @@
> +LED connected to PWM
> +
> +Required properties:
> +- compatible : should be "pwm-leds".
> +- pwms : PWM property, please refer to: 
> +  Documentation/devicetree/bindings/pwm/pwm.txt
> +- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device
> +- label : (optional) The label for this LED.  If omitted, the label is
> +  taken from the node name (excluding the unit address).
> +- max-brightness : Maximum brightness possible for the LED
> +- linux,default-trigger :  (optional) This parameter, if present, is a
> +  string defining the trigger assigned to the LED.  Current triggers are:
> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
> +		  system
> +    "default-on" - LED will turn on, but see "default-state" below
> +    "heartbeat" - LED "double" flashes at a load average based rate
> +    "ide-disk" - LED indicates disk activity
> +    "timer" - LED flashes at a fixed, configurable rate

The binding mostly looks good. However, it seems to be gratuitously
different from the gpio-leds binding and it duplicates property
definitions. Please match the gpio-leds behaviour with each led defined
as a sub node of the pwm-leds node.

Also, please reference the common properties in bindings/leds/common.txt
(This is a new file in linux-next. See how leds-gpio references it).

Otherwise the binding looks okay to me.

Re: [PATCH v2 3/3] leds: leds-pwm: Add device tree bindings

[Peter Ujfalusi]

Hi Grant,

On 12/06/2012 11:00 AM, Grant Likely wrote:
> On Mon, 12 Nov 2012 15:41:10 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> Support for device tree booted kernel.
>> When the kernel is booted with DeviceTree blob we support one led per
>> leds-pwm device to have cleaner integration with the PWM subsystem.
>>
>> For usage see:
>> Documentation/devicetree/bindings/leds/leds-pwm.txt
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  .../devicetree/bindings/leds/leds-pwm.txt          |  34 ++++++
>>  drivers/leds/leds-pwm.c                            | 125 +++++++++++++++------
>>  2 files changed, 127 insertions(+), 32 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..9fe3040
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> @@ -0,0 +1,34 @@
>> +LED connected to PWM
>> +
>> +Required properties:
>> +- compatible : should be "pwm-leds".
>> +- pwms : PWM property, please refer to: 
>> +  Documentation/devicetree/bindings/pwm/pwm.txt
>> +- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device
>> +- label : (optional) The label for this LED.  If omitted, the label is
>> +  taken from the node name (excluding the unit address).
>> +- max-brightness : Maximum brightness possible for the LED
>> +- linux,default-trigger :  (optional) This parameter, if present, is a
>> +  string defining the trigger assigned to the LED.  Current triggers are:
>> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
>> +		  system
>> +    "default-on" - LED will turn on, but see "default-state" below
>> +    "heartbeat" - LED "double" flashes at a load average based rate
>> +    "ide-disk" - LED indicates disk activity
>> +    "timer" - LED flashes at a fixed, configurable rate
> 
> The binding mostly looks good. However, it seems to be gratuitously
> different from the gpio-leds binding and it duplicates property
> definitions. Please match the gpio-leds behaviour with each led defined
> as a sub node of the pwm-leds node.

The GPIO and PWM bindings are substantially different. For start in pwm we do
not have of_get_pwm* helpers. To get the PWM itself we need to use wpm_get()
which uses the pwms = <>; pwm-names = <>; properties on the device's main node.
This is what I could do at the moment:

twl_pwm: pwm {
	compatible = "ti,twl4030-pwm";
	#pwm-cells = <2>;
};

twl_led: pwmled {
	compatible = "ti,twl4030-pwmled";
	#pwm-cells = <2>;
};

pwmleds {
	compatible = "pwm-leds";
	pwms = <&twl_pwm 0 7812500
		&twl_pwmled 0 7812500>;
	pwm-names = "omap4::keypad",
		    "omap4:green:chrg";
	kpad {
		label = "omap4::keypad";
		max-brightness = <127>;
	};

	charging {
		label = "omap4:green:chrg";
		max-brightness = <255>;
	};


};

The string in pwm-names and under the led section's label must match in order
to be able to get the correct PWM for the led to drive it.
If this is good, I can make the change and resend the series soon.

> Also, please reference the common properties in bindings/leds/common.txt
> (This is a new file in linux-next. See how leds-gpio references it).

Will do that.

-- 
Péter

Re: [PATCH v2 3/3] leds: leds-pwm: Add device tree bindings

[Grant Likely]

On Thu, 6 Dec 2012 13:36:11 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Hi Grant,
> 
> On 12/06/2012 11:00 AM, Grant Likely wrote:
> > On Mon, 12 Nov 2012 15:41:10 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> >> Support for device tree booted kernel.
> >> When the kernel is booted with DeviceTree blob we support one led per
> >> leds-pwm device to have cleaner integration with the PWM subsystem.
> >>
> >> For usage see:
> >> Documentation/devicetree/bindings/leds/leds-pwm.txt
> >>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> ---
> >>  .../devicetree/bindings/leds/leds-pwm.txt          |  34 ++++++
> >>  drivers/leds/leds-pwm.c                            | 125 +++++++++++++++------
> >>  2 files changed, 127 insertions(+), 32 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..9fe3040
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> >> @@ -0,0 +1,34 @@
> >> +LED connected to PWM
> >> +
> >> +Required properties:
> >> +- compatible : should be "pwm-leds".
> >> +- pwms : PWM property, please refer to: 
> >> +  Documentation/devicetree/bindings/pwm/pwm.txt
> >> +- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device
> >> +- label : (optional) The label for this LED.  If omitted, the label is
> >> +  taken from the node name (excluding the unit address).
> >> +- max-brightness : Maximum brightness possible for the LED
> >> +- linux,default-trigger :  (optional) This parameter, if present, is a
> >> +  string defining the trigger assigned to the LED.  Current triggers are:
> >> +    "backlight" - LED will act as a back-light, controlled by the framebuffer
> >> +		  system
> >> +    "default-on" - LED will turn on, but see "default-state" below
> >> +    "heartbeat" - LED "double" flashes at a load average based rate
> >> +    "ide-disk" - LED indicates disk activity
> >> +    "timer" - LED flashes at a fixed, configurable rate
> > 
> > The binding mostly looks good. However, it seems to be gratuitously
> > different from the gpio-leds binding and it duplicates property
> > definitions. Please match the gpio-leds behaviour with each led defined
> > as a sub node of the pwm-leds node.
> 
> The GPIO and PWM bindings are substantially different. For start in pwm we do
> not have of_get_pwm* helpers. To get the PWM itself we need to use wpm_get()

That's just the implementation. I'm talking about the binding. :-)

Implementation should follow binding design, not the other way around.
Both bindings use the same pattern. There isn't a of_get_pwm helper now,
but there is nothing preventing one from being created if you need it.

in the controller:
	#gpio-cells  vs. #pwm-cells
in the user:
	gpios = <[gpio specifier]> vs. pwms = <pwm specifier>

The PWM binding states that pwm-names is optional.

> which uses the pwms = <>; pwm-names = <>; properties on the device's main node.
> This is what I could do at the moment:
> 
> twl_pwm: pwm {
> 	compatible = "ti,twl4030-pwm";
> 	#pwm-cells = <2>;
> };
> 
> twl_led: pwmled {
> 	compatible = "ti,twl4030-pwmled";
> 	#pwm-cells = <2>;
> };
> 
> pwmleds {
> 	compatible = "pwm-leds";
> 	pwms = <&twl_pwm 0 7812500
> 		&twl_pwmled 0 7812500>;
> 	pwm-names = "omap4::keypad",
> 		    "omap4:green:chrg";
> 	kpad {
> 		label = "omap4::keypad";
> 		max-brightness = <127>;
> 	};
> 
> 	charging {
> 		label = "omap4:green:chrg";
> 		max-brightness = <255>;
> 	};

That's just a goofy extra layer of indirection and still doesn't follow
the lead of the gpio-leds pattern. That makes it worse that your
original binding, not better.

It really needs to look like this:

pwmleds {
	compatible = "pwm-leds";
	kpad {
		label = "omap4::keypad";
		max-brightness = <127>;
		pwms = <&twl_pwm 0 7812500>;
	};
	charging {
		label = "omap4:green:chrg";
		max-brightness = <255>;
		pwms = <&twl_pwmled 0 7812500>;
	};
};

g.

Re: [PATCH v2 3/3] leds: leds-pwm: Add device tree bindings

[Peter Ujfalusi]

On 12/07/2012 02:34 PM, Grant Likely wrote:
> It really needs to look like this:
> 
> pwmleds {
> 	compatible = "pwm-leds";
> 	kpad {
> 		label = "omap4::keypad";
> 		max-brightness = <127>;
> 		pwms = <&twl_pwm 0 7812500>;
> 	};
> 	charging {
> 		label = "omap4:green:chrg";
> 		max-brightness = <255>;
> 		pwms = <&twl_pwmled 0 7812500>;
> 	};
> };

OK.
So What we need to do is export the of_pwm_request() from drivers/pwm/core.c
Client drivers us of_pwm_request(node, NULL); to get the pwm when booted with DT.
Works fine on my setup this way and the DTS section looks just as you have
described.
Now I need to clean them up and resubmit.

-- 
Péter

Re: [PATCH v2 3/3] leds: leds-pwm: Add device tree bindings

[Grant Likely]

On Fri, 7 Dec 2012 15:04:51 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 12/07/2012 02:34 PM, Grant Likely wrote:
> > It really needs to look like this:
> > 
> > pwmleds {
> > 	compatible = "pwm-leds";
> > 	kpad {
> > 		label = "omap4::keypad";
> > 		max-brightness = <127>;
> > 		pwms = <&twl_pwm 0 7812500>;
> > 	};
> > 	charging {
> > 		label = "omap4:green:chrg";
> > 		max-brightness = <255>;
> > 		pwms = <&twl_pwmled 0 7812500>;
> > 	};
> > };
> 
> OK.
> So What we need to do is export the of_pwm_request() from drivers/pwm/core.c
> Client drivers us of_pwm_request(node, NULL); to get the pwm when booted with DT.
> Works fine on my setup this way and the DTS section looks just as you have
> described.
> Now I need to clean them up and resubmit.

Cool, thanks.

g.