[PATCH v3 0/4] leds: leds-pwm: Device tree support

[PATCH v3 0/4] leds: leds-pwm: Device tree support

[Peter Ujfalusi]

Hello,

Changes since v2:
- rebased on top of linux-next
- DT bindings now alligned with Grant's request
- exporting of_pwm_request() from PWM core to allow clean DT bindings
- DT binding documentation changed to reflect the changes

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 (4):
  leds: leds-pwm: Convert to use devm_get_pwm
  leds: leds-pwm: Preparing the driver for device tree support
  pwm: core: Export of_pwm_request() so client drivers can also use it
  leds: leds-pwm: Add device tree bindings

 .../devicetree/bindings/leds/leds-pwm.txt          |  46 ++++++
 drivers/leds/leds-pwm.c                            | 158 ++++++++++++++++-----
 drivers/pwm/core.c                                 |   2 +-
 include/linux/leds_pwm.h                           |   2 +-
 include/linux/pwm.h                                |   7 +
 5 files changed, 175 insertions(+), 40 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt

-- 
1.8.0

[PATCH v3 1/4] 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 2157524..351257c 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 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

Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Mon, Dec 10, 2012 at 11:00:34AM +0100, Peter Ujfalusi 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 2157524..351257c 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;
>  		}

The commit message says that legacy mode is used as fallback if
devm_get_pwm() (that should really be devm_pwm_get() btw) fails but I
don't see where pwm_request() is called.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

[-- Attachment #1.1: Type: text/plain, Size: 1765 bytes --]

On Tuesday 11 December 2012 08:03:07 Thierry Reding wrote:
> On Mon, Dec 10, 2012 at 11:00:34AM +0100, Peter Ujfalusi 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-l0cyMroinI0@public.gmane.org>
> > ---
> > 
> >  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 2157524..351257c 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;
> >  		
> >  		}
> 
> The commit message says that legacy mode is used as fallback if
> devm_get_pwm() (that should really be devm_pwm_get() btw) fails but I
> don't see where pwm_request() is called.

Oh, true. The fallback has been removed based on earlier comment from Bryan 
Wu. I will correct the commit message.

--
Péter

[-- Attachment #1.2: Type: text/html, Size: 9369 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2033 bytes --]

On Tue, Dec 11, 2012 at 09:29:21AM +0100, Peter Ujfalusi wrote:
> On Tuesday 11 December 2012 08:03:07 Thierry Reding wrote:
> > On Mon, Dec 10, 2012 at 11:00:34AM +0100, Peter Ujfalusi 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-l0cyMroinI0@public.gmane.org>
> > > ---
> > > 
> > >  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 2157524..351257c 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;
> > >  		
> > >  		}
> > 
> > The commit message says that legacy mode is used as fallback if
> > devm_get_pwm() (that should really be devm_pwm_get() btw) fails but I
> > don't see where pwm_request() is called.
> 
> Oh, true. The fallback has been removed based on earlier comment from Bryan 
> Wu. I will correct the commit message.

I'm not so sure that's a good idea. After all there may still be users
who rely on the legacy behaviour.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Re: Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

[-- Attachment #1.1: Type: text/plain, Size: 1290 bytes --]

On Tuesday 11 December 2012 09:36:35 Thierry Reding wrote:
> > > The commit message says that legacy mode is used as fallback if
> > > devm_get_pwm() (that should really be devm_pwm_get() btw) fails but I
> > > don't see where pwm_request() is called.
> > 
> > Oh, true. The fallback has been removed based on earlier comment from
> > Bryan
> > Wu. I will correct the commit message.
> 
> I'm not so sure that's a good idea. After all there may still be users
> who rely on the legacy behaviour.

The only user of leds_pwm is arch/arm/mach-omap2/board-4430sdp.c. Even that is 
not working because it uses wrong ID number to request the pwm.
The legacy request type needs the id number of the PWM it want to use. But if 
you have multiple PWM devices there is not way to guess the ID for the PWM. 
Using the pwm_get() along with pwm_lookup is the only way to get thing 
working.
For example with twl4030 we have 2 PWM chips, both provides 2 PWMs. The ID 
number of the PWMs will depend on the load order of the drivers. If they are 
built in they are going to be in the same order most of the time, but if they 
are modules they can be loaded in different order.

I think we should not use fallback in leds-pwm to avoid new boards to use the 
legacy API.

--
Péter

[-- Attachment #1.2: Type: text/html, Size: 4537 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: Re: Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

On Tue, Dec 11, 2012 at 09:57:51AM +0100, Peter Ujfalusi wrote:
> On Tuesday 11 December 2012 09:36:35 Thierry Reding wrote:
> > > > The commit message says that legacy mode is used as fallback if
> > > > devm_get_pwm() (that should really be devm_pwm_get() btw) fails but I
> > > > don't see where pwm_request() is called.
> > > 
> > > Oh, true. The fallback has been removed based on earlier comment from
> > > Bryan
> > > Wu. I will correct the commit message.
> > 
> > I'm not so sure that's a good idea. After all there may still be users
> > who rely on the legacy behaviour.
> 
> The only user of leds_pwm is arch/arm/mach-omap2/board-4430sdp.c. Even that is 
> not working because it uses wrong ID number to request the pwm.
> The legacy request type needs the id number of the PWM it want to use. But if 
> you have multiple PWM devices there is not way to guess the ID for the PWM. 
> Using the pwm_get() along with pwm_lookup is the only way to get thing 
> working.
> For example with twl4030 we have 2 PWM chips, both provides 2 PWMs. The ID 
> number of the PWMs will depend on the load order of the drivers. If they are 
> built in they are going to be in the same order most of the time, but if they 
> are modules they can be loaded in different order.
> 
> I think we should not use fallback in leds-pwm to avoid new boards to use the 
> legacy API.

Okay, if there are no intree users that may be broken, then it should be
fine to remove it. In that case you might want to remove the pwm_id
field as well instead of deprecating it in this patch.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

On 12/11/2012 10:31 AM, Thierry Reding wrote:
> On Tue, Dec 11, 2012 at 09:57:51AM +0100, Peter Ujfalusi wrote:
> Okay, if there are no intree users that may be broken, then it should be
> fine to remove it. In that case you might want to remove the pwm_id
> field as well instead of deprecating it in this patch.

The reason I marked the pwm_id as deprecated is to signal to out of tree users
(if any) that they should stop using it since it is going to go away in the
next cycle.
If we remove it right away the sdp4030 board file will not going to compile in
subsystem trees, only in linux-next.

-- 
Péter

Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On Tue, Dec 11, 2012 at 10:39:53AM +0100, Peter Ujfalusi wrote:
> On 12/11/2012 10:31 AM, Thierry Reding wrote:
> > On Tue, Dec 11, 2012 at 09:57:51AM +0100, Peter Ujfalusi wrote:
> > Okay, if there are no intree users that may be broken, then it should be
> > fine to remove it. In that case you might want to remove the pwm_id
> > field as well instead of deprecating it in this patch.
> 
> The reason I marked the pwm_id as deprecated is to signal to out of tree users
> (if any) that they should stop using it since it is going to go away in the
> next cycle.
> If we remove it right away the sdp4030 board file will not going to compile in
> subsystem trees, only in linux-next.

Okay, go ahead then. As long as the field will be removed eventually
that's fine with me.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Peter Ujfalusi]

On 12/11/2012 10:48 AM, Thierry Reding wrote:
> On Tue, Dec 11, 2012 at 10:39:53AM +0100, Peter Ujfalusi wrote:
>> On 12/11/2012 10:31 AM, Thierry Reding wrote:
>>> On Tue, Dec 11, 2012 at 09:57:51AM +0100, Peter Ujfalusi wrote:
>>> Okay, if there are no intree users that may be broken, then it should be
>>> fine to remove it. In that case you might want to remove the pwm_id
>>> field as well instead of deprecating it in this patch.
>>
>> The reason I marked the pwm_id as deprecated is to signal to out of tree users
>> (if any) that they should stop using it since it is going to go away in the
>> next cycle.
>> If we remove it right away the sdp4030 board file will not going to compile in
>> subsystem trees, only in linux-next.
> 
> Okay, go ahead then. As long as the field will be removed eventually
> that's fine with me.

Thank you and yes, it will be removed.

Probably it would be a good thing to check other places for legacy
pwm_request() users and prepare them to move to (devm_)pwm_get gracefully over
coming kernel releases.

-- 
Péter

Re: [PATCH v3 1/4] leds: leds-pwm: Convert to use devm_get_pwm

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

On Tue, Dec 11, 2012 at 10:53:11AM +0100, Peter Ujfalusi wrote:
> On 12/11/2012 10:48 AM, Thierry Reding wrote:
> > On Tue, Dec 11, 2012 at 10:39:53AM +0100, Peter Ujfalusi wrote:
> >> On 12/11/2012 10:31 AM, Thierry Reding wrote:
> >>> On Tue, Dec 11, 2012 at 09:57:51AM +0100, Peter Ujfalusi wrote:
> >>> Okay, if there are no intree users that may be broken, then it should be
> >>> fine to remove it. In that case you might want to remove the pwm_id
> >>> field as well instead of deprecating it in this patch.
> >>
> >> The reason I marked the pwm_id as deprecated is to signal to out of tree users
> >> (if any) that they should stop using it since it is going to go away in the
> >> next cycle.
> >> If we remove it right away the sdp4030 board file will not going to compile in
> >> subsystem trees, only in linux-next.
> > 
> > Okay, go ahead then. As long as the field will be removed eventually
> > that's fine with me.
> 
> Thank you and yes, it will be removed.
> 
> Probably it would be a good thing to check other places for legacy
> pwm_request() users and prepare them to move to (devm_)pwm_get gracefully over
> coming kernel releases.

Yes, I have a local patch that deprecates pwm_request() and pwm_free()
to make it easy to spot them (though git grep does a better job actually
as it doesn't require compilation). I originally planned to replace all
pwm_request()/pwm_free() usage by pwm_get()/pwm_put() already for 3.8
but got side-tracked with other stuff but I hope I can make it for 3.9.
If that works out we could remove pwm_request() and pwm_free()
completely for 3.10.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 2/4] 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-l0cyMroinI0@public.gmane.org>
---
 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 351257c..02f0c0c 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 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

Re: [PATCH v3 2/4] leds: leds-pwm: Preparing the driver for device tree support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2205 bytes --]

On Mon, Dec 10, 2012 at 11:00:35AM +0100, Peter Ujfalusi wrote:
> 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-l0cyMroinI0@public.gmane.org>
> ---
>  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 351257c..02f0c0c 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[];
> +};

I think you want leds[0] here. Otherwise your structure is too large by
sizeof(struct led_pwm_data *).

> +
>  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)

Perhaps this should return size_t?

> +{
> +	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);

I'm not sure if sizeof_pwm_leds_priv() requires to be a separate
function. You could make it shorter by doing something like:

	size_t extra = sizeof(*led_dat) * pdata->num_leds;

	priv = devm_kzalloc(&pdev->dev, sizeof(*priv) + extra, GFP_KERNEL);

But that's really just a matter of taste, so no further objections if
you want to keep the inline function.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH v3 3/4] pwm: core: Export of_pwm_request() so client drivers can also use it

[Peter Ujfalusi]

Allow client driver to use of_pwm_request() to get the pwm they need. This
is needed for drivers which handle more than one pwm separately, like
leds-pwm driver which have:

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>;
	};
};

in the dts files.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/pwm/core.c  | 2 +-
 include/linux/pwm.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 903138b..3a7ebcc 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -486,7 +486,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
  * becomes mandatory for devices that look up the PWM device via the con_id
  * parameter.
  */
-static struct pwm_device *of_pwm_request(struct device_node *np,
+struct pwm_device *of_pwm_request(struct device_node *np,
 					 const char *con_id)
 {
 	struct pwm_device *pwm = NULL;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 6d661f3..d70ffe3 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -175,6 +175,7 @@ struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
 		const struct of_phandle_args *args);
 
 struct pwm_device *pwm_get(struct device *dev, const char *consumer);
+struct pwm_device *of_pwm_request(struct device_node *np, const char *con_id);
 void pwm_put(struct pwm_device *pwm);
 
 struct pwm_device *devm_pwm_get(struct device *dev, const char *consumer);
@@ -213,6 +214,12 @@ static inline struct pwm_device *pwm_get(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
+static inline struct pwm_device *of_pwm_request(struct device_node *np,
+						const char *con_id)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline void pwm_put(struct pwm_device *pwm)
 {
 }
-- 
1.8.0

Re: [PATCH v3 3/4] pwm: core: Export of_pwm_request() so client drivers can also use it

[Grant Likely]

On Mon, 10 Dec 2012 11:00:36 +0100, Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org> wrote:
> Allow client driver to use of_pwm_request() to get the pwm they need. This
> is needed for drivers which handle more than one pwm separately, like
> leds-pwm driver which have:
> 
> 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>;
> 	};
> };
> 
> in the dts files.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>

Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

> ---
>  drivers/pwm/core.c  | 2 +-
>  include/linux/pwm.h | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 903138b..3a7ebcc 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -486,7 +486,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
>   * becomes mandatory for devices that look up the PWM device via the con_id
>   * parameter.
>   */
> -static struct pwm_device *of_pwm_request(struct device_node *np,
> +struct pwm_device *of_pwm_request(struct device_node *np,
>  					 const char *con_id)
>  {
>  	struct pwm_device *pwm = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 6d661f3..d70ffe3 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -175,6 +175,7 @@ struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
>  		const struct of_phandle_args *args);
>  
>  struct pwm_device *pwm_get(struct device *dev, const char *consumer);
> +struct pwm_device *of_pwm_request(struct device_node *np, const char *con_id);
>  void pwm_put(struct pwm_device *pwm);
>  
>  struct pwm_device *devm_pwm_get(struct device *dev, const char *consumer);
> @@ -213,6 +214,12 @@ static inline struct pwm_device *pwm_get(struct device *dev,
>  	return ERR_PTR(-ENODEV);
>  }
>  
> +static inline struct pwm_device *of_pwm_request(struct device_node *np,
> +						const char *con_id)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
>  static inline void pwm_put(struct pwm_device *pwm)
>  {
>  }
> -- 
> 1.8.0
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH v3 3/4] pwm: core: Export of_pwm_request() so client drivers can also use it

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Mon, Dec 10, 2012 at 11:00:36AM +0100, Peter Ujfalusi wrote:
> Allow client driver to use of_pwm_request() to get the pwm they need. This
> is needed for drivers which handle more than one pwm separately, like
> leds-pwm driver which have:

Hi Peter,

I really was hoping that we didn't have to export this function, but I
can't think of any other way to solve the problem at hand either. I'd
prefer to rename the function to of_pwm_get() at the same time to keep
consistent with other subsystems that provide similar functionality.
Also, please use all-caps for PWM in prose. And while at it, you can
drop the "core:" and "so client drivers can also use it" from the
subject line.

> 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>;
> 	};
> };
> 
> in the dts files.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/pwm/core.c  | 2 +-
>  include/linux/pwm.h | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 903138b..3a7ebcc 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -486,7 +486,7 @@ static struct pwm_chip *of_node_to_pwmchip(struct device_node *np)
>   * becomes mandatory for devices that look up the PWM device via the con_id
>   * parameter.
>   */
> -static struct pwm_device *of_pwm_request(struct device_node *np,
> +struct pwm_device *of_pwm_request(struct device_node *np,
>  					 const char *con_id)
>  {
>  	struct pwm_device *pwm = NULL;

This is missing an EXPORT_SYMBOL_GPL.

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 6d661f3..d70ffe3 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -175,6 +175,7 @@ struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
>  		const struct of_phandle_args *args);
>  
>  struct pwm_device *pwm_get(struct device *dev, const char *consumer);
> +struct pwm_device *of_pwm_request(struct device_node *np, const char *con_id);

While at it, maybe rename the con_id parameter as well to match
pwm_get().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 4/4] leds: leds-pwm: Add device tree bindings

[Peter Ujfalusi]

Support for device tree booted kernel.

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          |  46 ++++++++
 drivers/leds/leds-pwm.c                            | 124 +++++++++++++++++----
 2 files changed, 149 insertions(+), 21 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..abdff60
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
@@ -0,0 +1,46 @@
+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, please refer to: 
+  Documentation/devicetree/bindings/pwm/pwm.txt
+- pwm-names : (optional) Name to be used by the PWM subsystem for the PWM device
+- 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 02f0c0c..a57ac27 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,115 @@ static inline int 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_each_child_of_node(node, child) {
+		struct led_pwm_data *led_dat = &priv->leds[priv->num_leds];
 
-	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->cdev.name = of_get_property(child, "label",
+						     NULL) ? : child->name;
 
-		led_dat->pwm = devm_pwm_get(&pdev->dev, cur_led->name);
+		led_dat->pwm = of_pwm_request(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;
-		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)
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to register for %s\n",
+				led_dat->cdev.name);
+			of_node_put(child);
+			pwm_put(led_dat->pwm);
 			goto err;
+		}
+		priv->num_leds++;
+	}
+
+	return priv;
+err:
+	if (priv->num_leds > 0) {
+		for (count = priv->num_leds - 1; count >= 0; count--) {
+			led_classdev_unregister(&priv->leds[count].cdev);
+			pwm_put(priv->leds[count].pwm);
+		}
+	}
+
+	return NULL;
+}
+
+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 && 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;
+		}
+		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);
 
@@ -114,21 +184,33 @@ err:
 
 static int led_pwm_remove(struct platform_device *pdev)
 {
+	struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
 	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
 	int i;
 
-	for (i = 0; i < priv->num_leds; i++)
+	for (i = 0; i < priv->num_leds; i++) {
 		led_classdev_unregister(&priv->leds[i].cdev);
+		if (!pdata)
+			pwm_put(priv->leds[i].pwm);
+	}
 
 	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		= 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 v3 4/4] leds: leds-pwm: Add device tree bindings

[Grant Likely]

On Mon, 10 Dec 2012 11:00:37 +0100, Peter Ujfalusi <peter.ujfalusi@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.ujfalusi@ti.com>

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

The other patches in this series look good to.

g.

Re: [PATCH v3 4/4] leds: leds-pwm: Add device tree bindings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Mon, Dec 10, 2012 at 11:00:37AM +0100, Peter Ujfalusi wrote:
[...]
> +LED sub-node properties:
> +- pwms : PWM property, please refer to: 
> +  Documentation/devicetree/bindings/pwm/pwm.txt

Instead of only referring to the generic PWM binding document, this
should probably explain what the PWM device is used for.

> +err:
> +	if (priv->num_leds > 0) {
> +		for (count = priv->num_leds - 1; count >= 0; count--) {
> +			led_classdev_unregister(&priv->leds[count].cdev);
> +			pwm_put(priv->leds[count].pwm);
> +		}
> +	}

Can this not be written more simply as follows?

	while (priv->num_leds--) {
		...
	}

>  static int led_pwm_remove(struct platform_device *pdev)
>  {
> +	struct led_pwm_platform_data *pdata = pdev->dev.platform_data;
>  	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
>  	int i;
>  
> -	for (i = 0; i < priv->num_leds; i++)
> +	for (i = 0; i < priv->num_leds; i++) {
>  		led_classdev_unregister(&priv->leds[i].cdev);
> +		if (!pdata)
> +			pwm_put(priv->leds[i].pwm);
> +	}

Perhaps while at it we can add devm_of_pwm_get() along with exporting
of_pwm_get() so that you don't have to special-case this?

> +static const struct of_device_id of_pwm_leds_match[] = {
> +	{ .compatible = "pwm-leds", },
> +	{},
> +};

Doesn't this cause a compiler warning for !OF builds?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 4/4] leds: leds-pwm: Add device tree bindings

[Peter Ujfalusi]

On 12/11/2012 08:25 AM, Thierry Reding wrote:
>> +static const struct of_device_id of_pwm_leds_match[] = {
>> +	{ .compatible = "pwm-leds", },
>> +	{},
>> +};
> 
> Doesn't this cause a compiler warning for !OF builds?

This is not causing any compiler warnings.

-- 
Péter