Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Thierry Reding 
On Tue, Mar 05, 2013 at 01:59:06PM +0900, Alex Courbot wrote:
> On 03/05/2013 01:48 PM, Andrew Chew wrote:
> >I sent out a new patch that enables/disables the backlight enable gpio.
> >
> >>On 03/05/2013 01:00 PM, Andrew Chew wrote:
> >>>I did come to the same conclusion regarding the platform data breakage.
> >>>I'm expecting that the use of platform data will go away, at least on
> >>>ARM, since we are all aggressively moving what used to be in platform
> >>>data into the device tree.  Do other platforms use this driver?
> >>
> >>I can see at least 29 users of platform_pwm_backlight_data, all ARM with the
> >>exception of one unicore32. I guess at least for the foreseeable future
> >>platform data will remain.
> >
> >I'm not sure how to solve this, then.  Any suggestions?
> 
> In one of my (many) attempts to add power sequencing to
> pwm-backlight, I just added a boolean to the platform data that must
> be explicitly set in order to enable control by GPIO. I.e.
> 
>   booluse_enable_gpio
>   int enable_gpio;
>   unsigned intenable_gpio_flags;
> 
> enable_gpio and enable_gpio_flags would then only be considered if
> use_enable_gpio is true. Granted, it's not the best solution here
> but that's the only way to handle this correctly with integer GPIOS,
> and it does not pollute the DT anyway (use_enable_gpio will only be
> set by pwm_backlight_parse_dt() if of_get_named_gpio() returned a
> valid GPIO. Btw, you also want to check if the enable-gpio property
> exists first because otherwise probe() will fails if no GPIO is
> specified).

There are two more options that I can see. The first involves updating
all users to initialize the GPIO to -1 in the platform data, which will
remove the requirement for an extra flag field. Another option would be
to make this feature DT only, so that the GPIO can always be assumed to
be -1 for non-DT and DT without an enable-gpio property.

There's a third alternative, namely using a regulator for this, which
has better lookup support and therefore should be easier to make
optional. And as Alex mentioned it already has support for always-on
functionality and such.

Thierry


pgpng2b93uGqI.pgp
Description: PGP signature

Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 01:59 PM, Alex Courbot wrote:

Btw, you also want to check if the enable-gpio property exists first
because otherwise probe() will fails if no GPIO is specified).


That's actually not true - I overlooked the fact that probe() checks for 
the GPIO validity before requesting it. My bad.


Alex.


--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 01:48 PM, Andrew Chew wrote:

I sent out a new patch that enables/disables the backlight enable gpio.


On 03/05/2013 01:00 PM, Andrew Chew wrote:

I did come to the same conclusion regarding the platform data breakage.
I'm expecting that the use of platform data will go away, at least on
ARM, since we are all aggressively moving what used to be in platform
data into the device tree.  Do other platforms use this driver?


I can see at least 29 users of platform_pwm_backlight_data, all ARM with the
exception of one unicore32. I guess at least for the foreseeable future
platform data will remain.


I'm not sure how to solve this, then.  Any suggestions?


In one of my (many) attempts to add power sequencing to pwm-backlight, I 
just added a boolean to the platform data that must be explicitly set in 
order to enable control by GPIO. I.e.


booluse_enable_gpio
int enable_gpio;
unsigned intenable_gpio_flags;

enable_gpio and enable_gpio_flags would then only be considered if 
use_enable_gpio is true. Granted, it's not the best solution here but 
that's the only way to handle this correctly with integer GPIOS, and it 
does not pollute the DT anyway (use_enable_gpio will only be set by 
pwm_backlight_parse_dt() if of_get_named_gpio() returned a valid GPIO. 
Btw, you also want to check if the enable-gpio property exists first 
because otherwise probe() will fails if no GPIO is specified).



Yes, actually I am doing the GPIO rework. If you are not too much in a hurry
you might want for it to happen (should not be too long now that the core
has been reworked). At the same time, GPIO descriptors will also enable the
power sequences, so if you wait even longer (or help me with it), this patch
might not even be needed at all. Of course if you want to support this
*now*, this is still the shortest path.


Sadly, I do need this now, and I'd rather do it as cleanly as possible rather
than maintaining a hack.  The project I am working on is very pedantic.


Well, if you can get this right and make the GPIO optional, I think this 
is a reasonable feature to have in pwm-backlight, until a more generic 
powerseq-backlight driver takes over. ;)



To answer your last question, yes, this single patch does allow me to
enable the backlight on some boards (in particular, the one I'm working

on).

Cool - may I ask which one? All the NV boards I tried to far required more
complex sequences for their panels.


This is for t114-dalmore.  There may be other gpios that are needed that I'm
not aware of off the top of my head.  For the backlight itself, this seems to
be the only one.


I don't know the details of Dalmore but would think there must also be 
at least one regulator involved. If it is set to be always on in the DT, 
then your solution of using an enable GPIO should work then, even if not 
necessarily optimal wrt power usage.


Alex.

--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Andrew Chew 
I sent out a new patch that enables/disables the backlight enable gpio.

> On 03/05/2013 01:00 PM, Andrew Chew wrote:
> > I did come to the same conclusion regarding the platform data breakage.
> > I'm expecting that the use of platform data will go away, at least on
> > ARM, since we are all aggressively moving what used to be in platform
> > data into the device tree.  Do other platforms use this driver?
> 
> I can see at least 29 users of platform_pwm_backlight_data, all ARM with the
> exception of one unicore32. I guess at least for the foreseeable future
> platform data will remain.

I'm not sure how to solve this, then.  Any suggestions?

> > I remember hearing that there is some work in progress to encapsulate
> > gpios into a struct, rather than passing it around as a bare integer,
> > so when that happens, we can use NULL for no-gpio, which should take
> > care of the platform data issue as well.  It's kind of difficult to
> > work around this problem otherwise.
> 
> Yes, actually I am doing the GPIO rework. If you are not too much in a hurry
> you might want for it to happen (should not be too long now that the core
> has been reworked). At the same time, GPIO descriptors will also enable the
> power sequences, so if you wait even longer (or help me with it), this patch
> might not even be needed at all. Of course if you want to support this
> *now*, this is still the shortest path.

Sadly, I do need this now, and I'd rather do it as cleanly as possible rather
than maintaining a hack.  The project I am working on is very pedantic.

> > I agree that we should be turning on and off the backlight enable gpio
> > as needed to save power.  I just haven't gotten there yet.  I can try
> > to modify this patch if that's your preference, or I can follow up
> > with a patch to add this in the very near future.
> 
> That's ultimately for Thierry to say, but submitting a new revision makes
> more sense IMHO - it is not a big change and there are other issues to
> address (uninitialized GPIO in platform data) anyway.

Done.

> > To answer your last question, yes, this single patch does allow me to
> > enable the backlight on some boards (in particular, the one I'm working
> on).
> 
> Cool - may I ask which one? All the NV boards I tried to far required more
> complex sequences for their panels.

This is for t114-dalmore.  There may be other gpios that are needed that I'm
not aware of off the top of my head.  For the backlight itself, this seems to
be the only one.
--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 01:00 PM, Andrew Chew wrote:

I did come to the same conclusion regarding the platform data breakage.
I'm expecting that the use of platform data will go away, at least on ARM,
since we are all aggressively moving what used to be in platform data into
the device tree.  Do other platforms use this driver?


I can see at least 29 users of platform_pwm_backlight_data, all ARM with 
the exception of one unicore32. I guess at least for the foreseeable 
future platform data will remain.



I remember hearing that there is some work in progress to encapsulate
gpios into a struct, rather than passing it around as a bare integer, so when
that happens, we can use NULL for no-gpio, which should take care of the
platform data issue as well.  It's kind of difficult to work around this problem
otherwise.


Yes, actually I am doing the GPIO rework. If you are not too much in a 
hurry you might want for it to happen (should not be too long now that 
the core has been reworked). At the same time, GPIO descriptors will 
also enable the power sequences, so if you wait even longer (or help me 
with it), this patch might not even be needed at all. Of course if you 
want to support this *now*, this is still the shortest path.



I agree that we should be turning on and off the backlight enable gpio as
needed to save power.  I just haven't gotten there yet.  I can try to modify
this patch if that's your preference, or I can follow up with a patch to add
this in the very near future.


That's ultimately for Thierry to say, but submitting a new revision 
makes more sense IMHO - it is not a big change and there are other 
issues to address (uninitialized GPIO in platform data) anyway.



To answer your last question, yes, this single patch does allow me to enable
the backlight on some boards (in particular, the one I'm working on).


Cool - may I ask which one? All the NV boards I tried to far required 
more complex sequences for their panels.


Alex.

--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Andrew Chew 
> From: Alex Courbot
> Sent: Monday, March 04, 2013 7:00 PM
> To: Thierry Reding
> Cc: Andrew Chew; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
> 
> On 03/05/2013 07:46 AM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> >
> > On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:
> >> The backlight enable GPIO is specified in the device tree node for
> >> backlight.
> >>
> >> Signed-off-by: Andrew Chew 
> >> ---
> >>   .../bindings/video/backlight/pwm-backlight.txt |2 ++
> >>   drivers/video/backlight/pwm_bl.c   |   32 
> >> +--
> -
> >>   include/linux/pwm_backlight.h  |2 ++
> >>   3 files changed, 32 insertions(+), 4 deletions(-)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt
> >> b/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt
> >> index 1e4fc72..1ed4f0f 100644
> >> ---
> >> a/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight.txt
> >> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-
> backlight
> >> +++ .txt
> >> @@ -14,6 +14,8 @@ Required properties:
> >>   Optional properties:
> >> - pwm-names: a list of names for the PWM devices specified in the
> >>  "pwms" property (see PWM binding[0])
> >> +  - enable-gpio: a GPIO that needs to be used to enable the
> >> + backlight
> >> +  - enable-gpio-active-high: polarity of GPIO is active high
> >> + (default is low)
> >>
> >>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> >>
> >> diff --git a/drivers/video/backlight/pwm_bl.c
> >> b/drivers/video/backlight/pwm_bl.c
> >> index 069983c..f29f9c7 100644
> >> --- a/drivers/video/backlight/pwm_bl.c
> >> +++ b/drivers/video/backlight/pwm_bl.c
> >> @@ -20,10 +20,13 @@
> >>   #include 
> >>   #include 
> >>   #include 
> >> +#include 
> >>
> >>   struct pwm_bl_data {
> >>struct pwm_device   *pwm;
> >>struct device   *dev;
> >> +  int enable_gpio;
> >> +  unsigned intenable_gpio_flags;
> >>unsigned intperiod;
> >>unsigned intlth_brightness;
> >>unsigned int*levels;
> >> @@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
> >>}
> >>
> >>/*
> >> -   * TODO: Most users of this driver use a number of GPIOs to control
> >> -   *   backlight power. Support for specifying these needs to be
> >> -   *   added.
> >> +   * If "enable-gpio" is present, use that GPIO to enable the backlight.
> >> +   * The presence (or not) of "enable-gpio-active-high" will determine
> >> +   * the value of the GPIO.
> >> */
> >> +  data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
> >> +  if (of_property_read_bool(node, "enable-gpio-active-high"))
> >> +  data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
> >> +  else
> >> +  data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;
> >>
> >>return 0;
> >>   }
> >> @@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct
> platform_device *pdev)
> >>} else
> >>max = data->max_brightness;
> >>
> >> +  pb->enable_gpio = data->enable_gpio;
> >> +  pb->enable_gpio_flags = data->enable_gpio_flags;
> >>pb->notify = data->notify;
> >>pb->notify_after = data->notify_after;
> >>pb->check_fb = data->check_fb;
> >>pb->exit = data->exit;
> >>pb->dev = >dev;
> >>
> >> +  if (gpio_is_valid(pb->enable_gpio)) {
> >> +  ret = gpio_request_one(pb->enable_gpio,
> >> +  GPIOF_DIR_OUT | pb->enable_gpio_flags, "bl_en");
> >> +  if (ret) {
> >> +  dev_err(>dev, "failed to allocate bl_en gpio");
> >> +  goto err_alloc;
> >> +  }
> >> +  }
> >> +
> >>pb->pwm = devm_pwm_get(>dev, NULL);
> >>if (IS_ERR(pb->pwm)) {
> >>dev_err(>dev, "unable t

Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 07:46 AM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:

The backlight enable GPIO is specified in the device tree node for
backlight.

Signed-off-by: Andrew Chew 
---
  .../bindings/video/backlight/pwm-backlight.txt |2 ++
  drivers/video/backlight/pwm_bl.c   |   32 +---
  include/linux/pwm_backlight.h  |2 ++
  3 files changed, 32 insertions(+), 4 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
  Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
 "pwms" property (see PWM binding[0])
+  - enable-gpio: a GPIO that needs to be used to enable the backlight
+  - enable-gpio-active-high: polarity of GPIO is active high (default is low)

  [0]: Documentation/devicetree/bindings/pwm/pwm.txt

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
  #include 
  #include 
  #include 
+#include 

  struct pwm_bl_data {
struct pwm_device   *pwm;
struct device   *dev;
+   int enable_gpio;
+   unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}

/*
-* TODO: Most users of this driver use a number of GPIOs to control
-*   backlight power. Support for specifying these needs to be
-*   added.
+* If "enable-gpio" is present, use that GPIO to enable the backlight.
+* The presence (or not) of "enable-gpio-active-high" will determine
+* the value of the GPIO.
 */
+   data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
+   if (of_property_read_bool(node, "enable-gpio-active-high"))
+   data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+   else
+   data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;

return 0;
  }
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
} else
max = data->max_brightness;

+   pb->enable_gpio = data->enable_gpio;
+   pb->enable_gpio_flags = data->enable_gpio_flags;
pb->notify = data->notify;
pb->notify_after = data->notify_after;
pb->check_fb = data->check_fb;
pb->exit = data->exit;
pb->dev = >dev;

+   if (gpio_is_valid(pb->enable_gpio)) {
+   ret = gpio_request_one(pb->enable_gpio,
+   GPIOF_DIR_OUT | pb->enable_gpio_flags, "bl_en");
+   if (ret) {
+   dev_err(>dev, "failed to allocate bl_en gpio");
+   goto err_alloc;
+   }
+   }
+
pb->pwm = devm_pwm_get(>dev, NULL);
if (IS_ERR(pb->pwm)) {
dev_err(>dev, "unable to request PWM, trying legacy 
API\n");
@@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(pb->pwm)) {
dev_err(>dev, "unable to request legacy PWM\n");
ret = PTR_ERR(pb->pwm);
-   goto err_alloc;
+   goto err_gpio;
}
}

@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;

+err_gpio:
+   if (gpio_is_valid(data->enable_gpio))
+   gpio_free(data->enable_gpio);
  err_alloc:
if (data->exit)
data->exit(>dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device 
*pdev)
backlight_device_unregister(bl);
pwm_config(pb->pwm, 0, pb->period);
pwm_disable(pb->pwm);
+   if (gpio_is_valid(pb->enable_gpio))
+   gpio_free(pb->enable_gpio);
if (pb->exit)
pb->exit(>dev);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@

  struct platform_pwm_backlight_data {
int pwm_id;
+   int enable_gpio;
+   unsigned int enable_gpio_flags;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;


Hi Andrew,

I'm Cc'ing Alexandre

Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Thierry Reding 
On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:
> The backlight enable GPIO is specified in the device tree node for
> backlight.
> 
> Signed-off-by: Andrew Chew 
> ---
>  .../bindings/video/backlight/pwm-backlight.txt |2 ++
>  drivers/video/backlight/pwm_bl.c   |   32 
> +---
>  include/linux/pwm_backlight.h  |2 ++
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
> b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> index 1e4fc72..1ed4f0f 100644
> --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> @@ -14,6 +14,8 @@ Required properties:
>  Optional properties:
>- pwm-names: a list of names for the PWM devices specified in the
> "pwms" property (see PWM binding[0])
> +  - enable-gpio: a GPIO that needs to be used to enable the backlight
> +  - enable-gpio-active-high: polarity of GPIO is active high (default is low)
>  
>  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>  
> diff --git a/drivers/video/backlight/pwm_bl.c 
> b/drivers/video/backlight/pwm_bl.c
> index 069983c..f29f9c7 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -20,10 +20,13 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct pwm_bl_data {
>   struct pwm_device   *pwm;
>   struct device   *dev;
> + int enable_gpio;
> + unsigned intenable_gpio_flags;
>   unsigned intperiod;
>   unsigned intlth_brightness;
>   unsigned int*levels;
> @@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
>   }
>  
>   /*
> -  * TODO: Most users of this driver use a number of GPIOs to control
> -  *   backlight power. Support for specifying these needs to be
> -  *   added.
> +  * If "enable-gpio" is present, use that GPIO to enable the backlight.
> +  * The presence (or not) of "enable-gpio-active-high" will determine
> +  * the value of the GPIO.
>*/
> + data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
> + if (of_property_read_bool(node, "enable-gpio-active-high"))
> + data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
> + else
> + data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;
>  
>   return 0;
>  }
> @@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device 
> *pdev)
>   } else
>   max = data->max_brightness;
>  
> + pb->enable_gpio = data->enable_gpio;
> + pb->enable_gpio_flags = data->enable_gpio_flags;
>   pb->notify = data->notify;
>   pb->notify_after = data->notify_after;
>   pb->check_fb = data->check_fb;
>   pb->exit = data->exit;
>   pb->dev = >dev;
>  
> + if (gpio_is_valid(pb->enable_gpio)) {
> + ret = gpio_request_one(pb->enable_gpio,
> + GPIOF_DIR_OUT | pb->enable_gpio_flags, "bl_en");
> + if (ret) {
> + dev_err(>dev, "failed to allocate bl_en gpio");
> + goto err_alloc;
> + }
> + }
> +
>   pb->pwm = devm_pwm_get(>dev, NULL);
>   if (IS_ERR(pb->pwm)) {
>   dev_err(>dev, "unable to request PWM, trying legacy 
> API\n");
> @@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(pb->pwm)) {
>   dev_err(>dev, "unable to request legacy PWM\n");
>   ret = PTR_ERR(pb->pwm);
> - goto err_alloc;
> + goto err_gpio;
>   }
>   }
>  
> @@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device 
> *pdev)
>   platform_set_drvdata(pdev, bl);
>   return 0;
>  
> +err_gpio:
> + if (gpio_is_valid(data->enable_gpio))
> + gpio_free(data->enable_gpio);
>  err_alloc:
>   if (data->exit)
>   data->exit(>dev);
> @@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device 
> *pdev)
>   backlight_device_unregister(bl);
>   pwm_config(pb->pwm, 0, pb->period);
>   pwm_disable(pb->pwm);
> + if (gpio_is_valid(pb->enable_gpio))
> + gpio_free(pb->enable_gpio);
>   if (pb->exit)
>   pb->exit(>dev);
>   return 0;
> diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
> index 56f4a86..2706805 100644
> --- a/include/linux/pwm_backlight.h
> +++ b/include/linux/pwm_backlight.h
> @@ -8,6 +8,8 @@
>  
>  struct platform_pwm_backlight_data {
>   int pwm_id;
> + int enable_gpio;
> + unsigned int enable_gpio_flags;
>   unsigned int max_brightness;
>   unsigned int dft_brightness;
>   unsigned int

[PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Andrew Chew 
The backlight enable GPIO is specified in the device tree node for
backlight.

Signed-off-by: Andrew Chew 
---
 .../bindings/video/backlight/pwm-backlight.txt |2 ++
 drivers/video/backlight/pwm_bl.c   |   32 +---
 include/linux/pwm_backlight.h  |2 ++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
"pwms" property (see PWM binding[0])
+  - enable-gpio: a GPIO that needs to be used to enable the backlight
+  - enable-gpio-active-high: polarity of GPIO is active high (default is low)
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
 #include 
 #include 
 #include 
+#include 
 
 struct pwm_bl_data {
struct pwm_device   *pwm;
struct device   *dev;
+   int enable_gpio;
+   unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
 
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-*   backlight power. Support for specifying these needs to be
-*   added.
+* If "enable-gpio" is present, use that GPIO to enable the backlight.
+* The presence (or not) of "enable-gpio-active-high" will determine
+* the value of the GPIO.
 */
+   data->enable_gpio = of_get_named_gpio(node, "enable-gpio", 0);
+   if (of_property_read_bool(node, "enable-gpio-active-high"))
+   data->enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+   else
+   data->enable_gpio_flags = GPIOF_OUT_INIT_LOW;
 
return 0;
 }
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
} else
max = data->max_brightness;
 
+   pb->enable_gpio = data->enable_gpio;
+   pb->enable_gpio_flags = data->enable_gpio_flags;
pb->notify = data->notify;
pb->notify_after = data->notify_after;
pb->check_fb = data->check_fb;
pb->exit = data->exit;
pb->dev = >dev;
 
+   if (gpio_is_valid(pb->enable_gpio)) {
+   ret = gpio_request_one(pb->enable_gpio,
+   GPIOF_DIR_OUT | pb->enable_gpio_flags, "bl_en");
+   if (ret) {
+   dev_err(>dev, "failed to allocate bl_en gpio");
+   goto err_alloc;
+   }
+   }
+
pb->pwm = devm_pwm_get(>dev, NULL);
if (IS_ERR(pb->pwm)) {
dev_err(>dev, "unable to request PWM, trying legacy 
API\n");
@@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(pb->pwm)) {
dev_err(>dev, "unable to request legacy PWM\n");
ret = PTR_ERR(pb->pwm);
-   goto err_alloc;
+   goto err_gpio;
}
}
 
@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;
 
+err_gpio:
+   if (gpio_is_valid(data->enable_gpio))
+   gpio_free(data->enable_gpio);
 err_alloc:
if (data->exit)
data->exit(>dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device 
*pdev)
backlight_device_unregister(bl);
pwm_config(pb->pwm, 0, pb->period);
pwm_disable(pb->pwm);
+   if (gpio_is_valid(pb->enable_gpio))
+   gpio_free(pb->enable_gpio);
if (pb->exit)
pb->exit(>dev);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@
 
 struct platform_pwm_backlight_data {
int pwm_id;
+   int enable_gpio;
+   unsigned int enable_gpio_flags;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;
-- 
1.7.9.5

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

[PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Andrew Chew 
The backlight enable GPIO is specified in the device tree node for
backlight.

Signed-off-by: Andrew Chew ac...@nvidia.com
---
 .../bindings/video/backlight/pwm-backlight.txt |2 ++
 drivers/video/backlight/pwm_bl.c   |   32 +---
 include/linux/pwm_backlight.h  |2 ++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
pwms property (see PWM binding[0])
+  - enable-gpio: a GPIO that needs to be used to enable the backlight
+  - enable-gpio-active-high: polarity of GPIO is active high (default is low)
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
 #include linux/pwm.h
 #include linux/pwm_backlight.h
 #include linux/slab.h
+#include linux/of_gpio.h
 
 struct pwm_bl_data {
struct pwm_device   *pwm;
struct device   *dev;
+   int enable_gpio;
+   unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}
 
/*
-* TODO: Most users of this driver use a number of GPIOs to control
-*   backlight power. Support for specifying these needs to be
-*   added.
+* If enable-gpio is present, use that GPIO to enable the backlight.
+* The presence (or not) of enable-gpio-active-high will determine
+* the value of the GPIO.
 */
+   data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
+   if (of_property_read_bool(node, enable-gpio-active-high))
+   data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+   else
+   data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;
 
return 0;
 }
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
} else
max = data-max_brightness;
 
+   pb-enable_gpio = data-enable_gpio;
+   pb-enable_gpio_flags = data-enable_gpio_flags;
pb-notify = data-notify;
pb-notify_after = data-notify_after;
pb-check_fb = data-check_fb;
pb-exit = data-exit;
pb-dev = pdev-dev;
 
+   if (gpio_is_valid(pb-enable_gpio)) {
+   ret = gpio_request_one(pb-enable_gpio,
+   GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
+   if (ret) {
+   dev_err(pdev-dev, failed to allocate bl_en gpio);
+   goto err_alloc;
+   }
+   }
+
pb-pwm = devm_pwm_get(pdev-dev, NULL);
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request PWM, trying legacy 
API\n);
@@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request legacy PWM\n);
ret = PTR_ERR(pb-pwm);
-   goto err_alloc;
+   goto err_gpio;
}
}
 
@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;
 
+err_gpio:
+   if (gpio_is_valid(data-enable_gpio))
+   gpio_free(data-enable_gpio);
 err_alloc:
if (data-exit)
data-exit(pdev-dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device 
*pdev)
backlight_device_unregister(bl);
pwm_config(pb-pwm, 0, pb-period);
pwm_disable(pb-pwm);
+   if (gpio_is_valid(pb-enable_gpio))
+   gpio_free(pb-enable_gpio);
if (pb-exit)
pb-exit(pdev-dev);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@
 
 struct platform_pwm_backlight_data {
int pwm_id;
+   int enable_gpio;
+   unsigned int enable_gpio_flags;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int lth_brightness;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to

Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Thierry Reding 
On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:
 The backlight enable GPIO is specified in the device tree node for
 backlight.
 
 Signed-off-by: Andrew Chew ac...@nvidia.com
 ---
  .../bindings/video/backlight/pwm-backlight.txt |2 ++
  drivers/video/backlight/pwm_bl.c   |   32 
 +---
  include/linux/pwm_backlight.h  |2 ++
  3 files changed, 32 insertions(+), 4 deletions(-)
 
 diff --git 
 a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
 b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
 index 1e4fc72..1ed4f0f 100644
 --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
 +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
 @@ -14,6 +14,8 @@ Required properties:
  Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
 pwms property (see PWM binding[0])
 +  - enable-gpio: a GPIO that needs to be used to enable the backlight
 +  - enable-gpio-active-high: polarity of GPIO is active high (default is low)
  
  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
  
 diff --git a/drivers/video/backlight/pwm_bl.c 
 b/drivers/video/backlight/pwm_bl.c
 index 069983c..f29f9c7 100644
 --- a/drivers/video/backlight/pwm_bl.c
 +++ b/drivers/video/backlight/pwm_bl.c
 @@ -20,10 +20,13 @@
  #include linux/pwm.h
  #include linux/pwm_backlight.h
  #include linux/slab.h
 +#include linux/of_gpio.h
  
  struct pwm_bl_data {
   struct pwm_device   *pwm;
   struct device   *dev;
 + int enable_gpio;
 + unsigned intenable_gpio_flags;
   unsigned intperiod;
   unsigned intlth_brightness;
   unsigned int*levels;
 @@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
   }
  
   /*
 -  * TODO: Most users of this driver use a number of GPIOs to control
 -  *   backlight power. Support for specifying these needs to be
 -  *   added.
 +  * If enable-gpio is present, use that GPIO to enable the backlight.
 +  * The presence (or not) of enable-gpio-active-high will determine
 +  * the value of the GPIO.
*/
 + data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
 + if (of_property_read_bool(node, enable-gpio-active-high))
 + data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
 + else
 + data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;
  
   return 0;
  }
 @@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device 
 *pdev)
   } else
   max = data-max_brightness;
  
 + pb-enable_gpio = data-enable_gpio;
 + pb-enable_gpio_flags = data-enable_gpio_flags;
   pb-notify = data-notify;
   pb-notify_after = data-notify_after;
   pb-check_fb = data-check_fb;
   pb-exit = data-exit;
   pb-dev = pdev-dev;
  
 + if (gpio_is_valid(pb-enable_gpio)) {
 + ret = gpio_request_one(pb-enable_gpio,
 + GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
 + if (ret) {
 + dev_err(pdev-dev, failed to allocate bl_en gpio);
 + goto err_alloc;
 + }
 + }
 +
   pb-pwm = devm_pwm_get(pdev-dev, NULL);
   if (IS_ERR(pb-pwm)) {
   dev_err(pdev-dev, unable to request PWM, trying legacy 
 API\n);
 @@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device 
 *pdev)
   if (IS_ERR(pb-pwm)) {
   dev_err(pdev-dev, unable to request legacy PWM\n);
   ret = PTR_ERR(pb-pwm);
 - goto err_alloc;
 + goto err_gpio;
   }
   }
  
 @@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device 
 *pdev)
   platform_set_drvdata(pdev, bl);
   return 0;
  
 +err_gpio:
 + if (gpio_is_valid(data-enable_gpio))
 + gpio_free(data-enable_gpio);
  err_alloc:
   if (data-exit)
   data-exit(pdev-dev);
 @@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device 
 *pdev)
   backlight_device_unregister(bl);
   pwm_config(pb-pwm, 0, pb-period);
   pwm_disable(pb-pwm);
 + if (gpio_is_valid(pb-enable_gpio))
 + gpio_free(pb-enable_gpio);
   if (pb-exit)
   pb-exit(pdev-dev);
   return 0;
 diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
 index 56f4a86..2706805 100644
 --- a/include/linux/pwm_backlight.h
 +++ b/include/linux/pwm_backlight.h
 @@ -8,6 +8,8 @@
  
  struct platform_pwm_backlight_data {
   int pwm_id;
 + int enable_gpio;
 + unsigned int enable_gpio_flags;
   unsigned int max_brightness;
   unsigned int dft_brightness;
   unsigned int lth_brightness;

Hi Andrew,

I'm Cc'ing Alexandre Courbot, who has been working on

Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 07:46 AM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:

The backlight enable GPIO is specified in the device tree node for
backlight.

Signed-off-by: Andrew Chew ac...@nvidia.com
---
  .../bindings/video/backlight/pwm-backlight.txt |2 ++
  drivers/video/backlight/pwm_bl.c   |   32 +---
  include/linux/pwm_backlight.h  |2 ++
  3 files changed, 32 insertions(+), 4 deletions(-)

diff --git 
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt 
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..1ed4f0f 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,8 @@ Required properties:
  Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
 pwms property (see PWM binding[0])
+  - enable-gpio: a GPIO that needs to be used to enable the backlight
+  - enable-gpio-active-high: polarity of GPIO is active high (default is low)

  [0]: Documentation/devicetree/bindings/pwm/pwm.txt

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..f29f9c7 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
  #include linux/pwm.h
  #include linux/pwm_backlight.h
  #include linux/slab.h
+#include linux/of_gpio.h

  struct pwm_bl_data {
struct pwm_device   *pwm;
struct device   *dev;
+   int enable_gpio;
+   unsigned intenable_gpio_flags;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device *dev,
}

/*
-* TODO: Most users of this driver use a number of GPIOs to control
-*   backlight power. Support for specifying these needs to be
-*   added.
+* If enable-gpio is present, use that GPIO to enable the backlight.
+* The presence (or not) of enable-gpio-active-high will determine
+* the value of the GPIO.
 */
+   data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
+   if (of_property_read_bool(node, enable-gpio-active-high))
+   data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
+   else
+   data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;

return 0;
  }
@@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct platform_device 
*pdev)
} else
max = data-max_brightness;

+   pb-enable_gpio = data-enable_gpio;
+   pb-enable_gpio_flags = data-enable_gpio_flags;
pb-notify = data-notify;
pb-notify_after = data-notify_after;
pb-check_fb = data-check_fb;
pb-exit = data-exit;
pb-dev = pdev-dev;

+   if (gpio_is_valid(pb-enable_gpio)) {
+   ret = gpio_request_one(pb-enable_gpio,
+   GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
+   if (ret) {
+   dev_err(pdev-dev, failed to allocate bl_en gpio);
+   goto err_alloc;
+   }
+   }
+
pb-pwm = devm_pwm_get(pdev-dev, NULL);
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request PWM, trying legacy 
API\n);
@@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (IS_ERR(pb-pwm)) {
dev_err(pdev-dev, unable to request legacy PWM\n);
ret = PTR_ERR(pb-pwm);
-   goto err_alloc;
+   goto err_gpio;
}
}

@@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, bl);
return 0;

+err_gpio:
+   if (gpio_is_valid(data-enable_gpio))
+   gpio_free(data-enable_gpio);
  err_alloc:
if (data-exit)
data-exit(pdev-dev);
@@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct platform_device 
*pdev)
backlight_device_unregister(bl);
pwm_config(pb-pwm, 0, pb-period);
pwm_disable(pb-pwm);
+   if (gpio_is_valid(pb-enable_gpio))
+   gpio_free(pb-enable_gpio);
if (pb-exit)
pb-exit(pdev-dev);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..2706805 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -8,6 +8,8 @@

  struct platform_pwm_backlight_data {
int pwm_id;
+   int enable_gpio;
+   unsigned int enable_gpio_flags;
unsigned int max_brightness;
unsigned int dft_brightness;
unsigned int

RE: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Andrew Chew 
 From: Alex Courbot
 Sent: Monday, March 04, 2013 7:00 PM
 To: Thierry Reding
 Cc: Andrew Chew; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO
 
 On 03/05/2013 07:46 AM, Thierry Reding wrote:
  * PGP Signed by an unknown key
 
  On Mon, Mar 04, 2013 at 01:49:49PM -0800, Andrew Chew wrote:
  The backlight enable GPIO is specified in the device tree node for
  backlight.
 
  Signed-off-by: Andrew Chew ac...@nvidia.com
  ---
.../bindings/video/backlight/pwm-backlight.txt |2 ++
drivers/video/backlight/pwm_bl.c   |   32 
  +--
 -
include/linux/pwm_backlight.h  |2 ++
3 files changed, 32 insertions(+), 4 deletions(-)
 
  diff --git
  a/Documentation/devicetree/bindings/video/backlight/pwm-
 backlight.txt
  b/Documentation/devicetree/bindings/video/backlight/pwm-
 backlight.txt
  index 1e4fc72..1ed4f0f 100644
  ---
  a/Documentation/devicetree/bindings/video/backlight/pwm-
 backlight.txt
  +++ b/Documentation/devicetree/bindings/video/backlight/pwm-
 backlight
  +++ .txt
  @@ -14,6 +14,8 @@ Required properties:
Optional properties:
  - pwm-names: a list of names for the PWM devices specified in the
   pwms property (see PWM binding[0])
  +  - enable-gpio: a GPIO that needs to be used to enable the
  + backlight
  +  - enable-gpio-active-high: polarity of GPIO is active high
  + (default is low)
 
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
  diff --git a/drivers/video/backlight/pwm_bl.c
  b/drivers/video/backlight/pwm_bl.c
  index 069983c..f29f9c7 100644
  --- a/drivers/video/backlight/pwm_bl.c
  +++ b/drivers/video/backlight/pwm_bl.c
  @@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
  +#include linux/of_gpio.h
 
struct pwm_bl_data {
 struct pwm_device   *pwm;
 struct device   *dev;
  +  int enable_gpio;
  +  unsigned intenable_gpio_flags;
 unsigned intperiod;
 unsigned intlth_brightness;
 unsigned int*levels;
  @@ -146,10 +149,15 @@ static int pwm_backlight_parse_dt(struct device
 *dev,
 }
 
 /*
  -   * TODO: Most users of this driver use a number of GPIOs to control
  -   *   backlight power. Support for specifying these needs to be
  -   *   added.
  +   * If enable-gpio is present, use that GPIO to enable the backlight.
  +   * The presence (or not) of enable-gpio-active-high will determine
  +   * the value of the GPIO.
  */
  +  data-enable_gpio = of_get_named_gpio(node, enable-gpio, 0);
  +  if (of_property_read_bool(node, enable-gpio-active-high))
  +  data-enable_gpio_flags = GPIOF_OUT_INIT_HIGH;
  +  else
  +  data-enable_gpio_flags = GPIOF_OUT_INIT_LOW;
 
 return 0;
}
  @@ -207,12 +215,23 @@ static int pwm_backlight_probe(struct
 platform_device *pdev)
 } else
 max = data-max_brightness;
 
  +  pb-enable_gpio = data-enable_gpio;
  +  pb-enable_gpio_flags = data-enable_gpio_flags;
 pb-notify = data-notify;
 pb-notify_after = data-notify_after;
 pb-check_fb = data-check_fb;
 pb-exit = data-exit;
 pb-dev = pdev-dev;
 
  +  if (gpio_is_valid(pb-enable_gpio)) {
  +  ret = gpio_request_one(pb-enable_gpio,
  +  GPIOF_DIR_OUT | pb-enable_gpio_flags, bl_en);
  +  if (ret) {
  +  dev_err(pdev-dev, failed to allocate bl_en gpio);
  +  goto err_alloc;
  +  }
  +  }
  +
 pb-pwm = devm_pwm_get(pdev-dev, NULL);
 if (IS_ERR(pb-pwm)) {
 dev_err(pdev-dev, unable to request PWM, trying legacy
  API\n); @@ -221,7 +240,7 @@ static int pwm_backlight_probe(struct
 platform_device *pdev)
 if (IS_ERR(pb-pwm)) {
 dev_err(pdev-dev, unable to request legacy
 PWM\n);
 ret = PTR_ERR(pb-pwm);
  -  goto err_alloc;
  +  goto err_gpio;
 }
 }
 
  @@ -255,6 +274,9 @@ static int pwm_backlight_probe(struct
 platform_device *pdev)
 platform_set_drvdata(pdev, bl);
 return 0;
 
  +err_gpio:
  +  if (gpio_is_valid(data-enable_gpio))
  +  gpio_free(data-enable_gpio);
err_alloc:
 if (data-exit)
 data-exit(pdev-dev);
  @@ -269,6 +291,8 @@ static int pwm_backlight_remove(struct
 platform_device *pdev)
 backlight_device_unregister(bl);
 pwm_config(pb-pwm, 0, pb-period);
 pwm_disable(pb-pwm);
  +  if (gpio_is_valid(pb-enable_gpio))
  +  gpio_free(pb-enable_gpio);
 if (pb-exit)
 pb-exit(pdev-dev);
 return 0;
  diff --git a/include/linux/pwm_backlight.h
  b/include/linux/pwm_backlight.h index 56f4a86..2706805 100644
  --- a/include/linux/pwm_backlight.h
  +++ b/include/linux/pwm_backlight.h
  @@ -8,6 +8,8 @@
 
struct platform_pwm_backlight_data {
 int pwm_id;
  +  int

Re: [PATCH 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 01:00 PM, Andrew Chew wrote:

I did come to the same conclusion regarding the platform data breakage.
I'm expecting that the use of platform data will go away, at least on ARM,
since we are all aggressively moving what used to be in platform data into
the device tree.  Do other platforms use this driver?


I can see at least 29 users of platform_pwm_backlight_data, all ARM with 
the exception of one unicore32. I guess at least for the foreseeable 
future platform data will remain.



I remember hearing that there is some work in progress to encapsulate
gpios into a struct, rather than passing it around as a bare integer, so when
that happens, we can use NULL for no-gpio, which should take care of the
platform data issue as well.  It's kind of difficult to work around this problem
otherwise.


Yes, actually I am doing the GPIO rework. If you are not too much in a 
hurry you might want for it to happen (should not be too long now that 
the core has been reworked). At the same time, GPIO descriptors will 
also enable the power sequences, so if you wait even longer (or help me 
with it), this patch might not even be needed at all. Of course if you 
want to support this *now*, this is still the shortest path.



I agree that we should be turning on and off the backlight enable gpio as
needed to save power.  I just haven't gotten there yet.  I can try to modify
this patch if that's your preference, or I can follow up with a patch to add
this in the very near future.


That's ultimately for Thierry to say, but submitting a new revision 
makes more sense IMHO - it is not a big change and there are other 
issues to address (uninitialized GPIO in platform data) anyway.



To answer your last question, yes, this single patch does allow me to enable
the backlight on some boards (in particular, the one I'm working on).


Cool - may I ask which one? All the NV boards I tried to far required 
more complex sequences for their panels.


Alex.

--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Andrew Chew 
I sent out a new patch that enables/disables the backlight enable gpio.

 On 03/05/2013 01:00 PM, Andrew Chew wrote:
  I did come to the same conclusion regarding the platform data breakage.
  I'm expecting that the use of platform data will go away, at least on
  ARM, since we are all aggressively moving what used to be in platform
  data into the device tree.  Do other platforms use this driver?
 
 I can see at least 29 users of platform_pwm_backlight_data, all ARM with the
 exception of one unicore32. I guess at least for the foreseeable future
 platform data will remain.

I'm not sure how to solve this, then.  Any suggestions?

  I remember hearing that there is some work in progress to encapsulate
  gpios into a struct, rather than passing it around as a bare integer,
  so when that happens, we can use NULL for no-gpio, which should take
  care of the platform data issue as well.  It's kind of difficult to
  work around this problem otherwise.
 
 Yes, actually I am doing the GPIO rework. If you are not too much in a hurry
 you might want for it to happen (should not be too long now that the core
 has been reworked). At the same time, GPIO descriptors will also enable the
 power sequences, so if you wait even longer (or help me with it), this patch
 might not even be needed at all. Of course if you want to support this
 *now*, this is still the shortest path.

Sadly, I do need this now, and I'd rather do it as cleanly as possible rather
than maintaining a hack.  The project I am working on is very pedantic.

  I agree that we should be turning on and off the backlight enable gpio
  as needed to save power.  I just haven't gotten there yet.  I can try
  to modify this patch if that's your preference, or I can follow up
  with a patch to add this in the very near future.
 
 That's ultimately for Thierry to say, but submitting a new revision makes
 more sense IMHO - it is not a big change and there are other issues to
 address (uninitialized GPIO in platform data) anyway.

Done.

  To answer your last question, yes, this single patch does allow me to
  enable the backlight on some boards (in particular, the one I'm working
 on).
 
 Cool - may I ask which one? All the NV boards I tried to far required more
 complex sequences for their panels.

This is for t114-dalmore.  There may be other gpios that are needed that I'm
not aware of off the top of my head.  For the backlight itself, this seems to
be the only one.
--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 01:48 PM, Andrew Chew wrote:

I sent out a new patch that enables/disables the backlight enable gpio.


On 03/05/2013 01:00 PM, Andrew Chew wrote:

I did come to the same conclusion regarding the platform data breakage.
I'm expecting that the use of platform data will go away, at least on
ARM, since we are all aggressively moving what used to be in platform
data into the device tree.  Do other platforms use this driver?


I can see at least 29 users of platform_pwm_backlight_data, all ARM with the
exception of one unicore32. I guess at least for the foreseeable future
platform data will remain.


I'm not sure how to solve this, then.  Any suggestions?


In one of my (many) attempts to add power sequencing to pwm-backlight, I 
just added a boolean to the platform data that must be explicitly set in 
order to enable control by GPIO. I.e.


booluse_enable_gpio
int enable_gpio;
unsigned intenable_gpio_flags;

enable_gpio and enable_gpio_flags would then only be considered if 
use_enable_gpio is true. Granted, it's not the best solution here but 
that's the only way to handle this correctly with integer GPIOS, and it 
does not pollute the DT anyway (use_enable_gpio will only be set by 
pwm_backlight_parse_dt() if of_get_named_gpio() returned a valid GPIO. 
Btw, you also want to check if the enable-gpio property exists first 
because otherwise probe() will fails if no GPIO is specified).



Yes, actually I am doing the GPIO rework. If you are not too much in a hurry
you might want for it to happen (should not be too long now that the core
has been reworked). At the same time, GPIO descriptors will also enable the
power sequences, so if you wait even longer (or help me with it), this patch
might not even be needed at all. Of course if you want to support this
*now*, this is still the shortest path.


Sadly, I do need this now, and I'd rather do it as cleanly as possible rather
than maintaining a hack.  The project I am working on is very pedantic.


Well, if you can get this right and make the GPIO optional, I think this 
is a reasonable feature to have in pwm-backlight, until a more generic 
powerseq-backlight driver takes over. ;)



To answer your last question, yes, this single patch does allow me to
enable the backlight on some boards (in particular, the one I'm working

on).

Cool - may I ask which one? All the NV boards I tried to far required more
complex sequences for their panels.


This is for t114-dalmore.  There may be other gpios that are needed that I'm
not aware of off the top of my head.  For the backlight itself, this seems to
be the only one.


I don't know the details of Dalmore but would think there must also be 
at least one regulator involved. If it is set to be always on in the DT, 
then your solution of using an enable GPIO should work then, even if not 
necessarily optimal wrt power usage.


Alex.

--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Alex Courbot 

On 03/05/2013 01:59 PM, Alex Courbot wrote:

Btw, you also want to check if the enable-gpio property exists first
because otherwise probe() will fails if no GPIO is specified).


That's actually not true - I overlooked the fact that probe() checks for 
the GPIO validity before requesting it. My bad.


Alex.


--
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 1/1] pwm_bl: Add support for backlight enable GPIO

2013-03-04 Thread Thierry Reding 
On Tue, Mar 05, 2013 at 01:59:06PM +0900, Alex Courbot wrote:
 On 03/05/2013 01:48 PM, Andrew Chew wrote:
 I sent out a new patch that enables/disables the backlight enable gpio.
 
 On 03/05/2013 01:00 PM, Andrew Chew wrote:
 I did come to the same conclusion regarding the platform data breakage.
 I'm expecting that the use of platform data will go away, at least on
 ARM, since we are all aggressively moving what used to be in platform
 data into the device tree.  Do other platforms use this driver?
 
 I can see at least 29 users of platform_pwm_backlight_data, all ARM with the
 exception of one unicore32. I guess at least for the foreseeable future
 platform data will remain.
 
 I'm not sure how to solve this, then.  Any suggestions?
 
 In one of my (many) attempts to add power sequencing to
 pwm-backlight, I just added a boolean to the platform data that must
 be explicitly set in order to enable control by GPIO. I.e.
 
   booluse_enable_gpio
   int enable_gpio;
   unsigned intenable_gpio_flags;
 
 enable_gpio and enable_gpio_flags would then only be considered if
 use_enable_gpio is true. Granted, it's not the best solution here
 but that's the only way to handle this correctly with integer GPIOS,
 and it does not pollute the DT anyway (use_enable_gpio will only be
 set by pwm_backlight_parse_dt() if of_get_named_gpio() returned a
 valid GPIO. Btw, you also want to check if the enable-gpio property
 exists first because otherwise probe() will fails if no GPIO is
 specified).

There are two more options that I can see. The first involves updating
all users to initialize the GPIO to -1 in the platform data, which will
remove the requirement for an extra flag field. Another option would be
to make this feature DT only, so that the GPIO can always be assumed to
be -1 for non-DT and DT without an enable-gpio property.

There's a third alternative, namely using a regulator for this, which
has better lookup support and therefore should be easier to make
optional. And as Alex mentioned it already has support for always-on
functionality and such.

Thierry


pgpng2b93uGqI.pgp
Description: PGP signature