Re: [PATCH 01/10] pwm-backlight: Refactor backlight power on/off

2013-10-01 Thread Thierry Reding 
On Tue, Oct 01, 2013 at 12:26:07PM -0600, Stephen Warren wrote:
> On 09/23/2013 03:40 PM, Thierry Reding wrote:
> > In preparation for adding an optional regulator and enable GPIO to the
> > driver, split the power on and power off sequences into separate
> > functions to reduce code duplication at the multiple call sites.
> 
> > diff --git a/drivers/video/backlight/pwm_bl.c 
> > b/drivers/video/backlight/pwm_bl.c
> 
> > +static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> > +{
> > +   pwm_disable(pb->pwm);
> 
> Both the call-sites you're replacing do the following before pwm_disable():
> 
>   pwm_config(pb->pwm, 0, pb->period);
> 
> While I agree that probably shouldn't be necessary, I think it's at
> least worth mentioning that in the commit description just to make it
> obvious that it was a deliberate change. Splitting that change into a
> separate patch might be reasonable in order to keep refactoring and
> functional changes separate, although perhaps it's not worth it.

Actually I'll add that back. I'm not sure exactly why I dropped it (it
may just have been oversight) and there have been reports that some HW
actually requires pwm_config(..., 0, ...) before pwm_disable() to turn
off properly.

> There are also a couple unrelated whitespace changes thrown in here.

I think they increase readability, but I can certainly move them into a
separate patch.

Thierry


pgpctxOW5l0O1.pgp
Description: PGP signature

Re: [PATCH 01/10] pwm-backlight: Refactor backlight power on/off

2013-10-01 Thread Stephen Warren 
On 09/23/2013 03:40 PM, Thierry Reding wrote:
> In preparation for adding an optional regulator and enable GPIO to the
> driver, split the power on and power off sequences into separate
> functions to reduce code duplication at the multiple call sites.

> diff --git a/drivers/video/backlight/pwm_bl.c 
> b/drivers/video/backlight/pwm_bl.c

> +static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> +{
> + pwm_disable(pb->pwm);

Both the call-sites you're replacing do the following before pwm_disable():

pwm_config(pb->pwm, 0, pb->period);

While I agree that probably shouldn't be necessary, I think it's at
least worth mentioning that in the commit description just to make it
obvious that it was a deliberate change. Splitting that change into a
separate patch might be reasonable in order to keep refactoring and
functional changes separate, although perhaps it's not worth it.

There are also a couple unrelated whitespace changes thrown in here.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/10] pwm-backlight: Refactor backlight power on/off

2013-09-23 Thread Thierry Reding 
In preparation for adding an optional regulator and enable GPIO to the
driver, split the power on and power off sequences into separate
functions to reduce code duplication at the multiple call sites.

Signed-off-by: Thierry Reding 
---
 drivers/video/backlight/pwm_bl.c | 59 
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 36db5d9..8a49b30 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -35,6 +35,30 @@ struct pwm_bl_data {
void(*exit)(struct device *);
 };
 
+static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness,
+  int max)
+{
+   int duty_cycle, err;
+
+   if (pb->levels) {
+   duty_cycle = pb->levels[brightness];
+   max = pb->levels[max];
+   } else {
+   duty_cycle = brightness;
+   }
+
+   duty_cycle = (duty_cycle * (pb->period - pb->lth_brightness) / max) +
+pb->lth_brightness;
+
+   pwm_config(pb->pwm, duty_cycle, pb->period);
+   pwm_enable(pb->pwm);
+}
+
+static void pwm_backlight_power_off(struct pwm_bl_data *pb)
+{
+   pwm_disable(pb->pwm);
+}
+
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
struct pwm_bl_data *pb = bl_get_data(bl);
@@ -49,24 +73,10 @@ static int pwm_backlight_update_status(struct 
backlight_device *bl)
if (pb->notify)
brightness = pb->notify(pb->dev, brightness);
 
-   if (brightness == 0) {
-   pwm_config(pb->pwm, 0, pb->period);
-   pwm_disable(pb->pwm);
-   } else {
-   int duty_cycle;
-
-   if (pb->levels) {
-   duty_cycle = pb->levels[brightness];
-   max = pb->levels[max];
-   } else {
-   duty_cycle = brightness;
-   }
-
-   duty_cycle = pb->lth_brightness +
-(duty_cycle * (pb->period - pb->lth_brightness) / max);
-   pwm_config(pb->pwm, duty_cycle, pb->period);
-   pwm_enable(pb->pwm);
-   }
+   if (brightness > 0)
+   pwm_backlight_power_on(pb, brightness, max);
+   else
+   pwm_backlight_power_off(pb);
 
if (pb->notify_after)
pb->notify_after(pb->dev, brightness);
@@ -267,10 +277,11 @@ static int pwm_backlight_remove(struct platform_device 
*pdev)
struct pwm_bl_data *pb = bl_get_data(bl);
 
backlight_device_unregister(bl);
-   pwm_config(pb->pwm, 0, pb->period);
-   pwm_disable(pb->pwm);
+   pwm_backlight_power_off(pb);
+
if (pb->exit)
pb->exit(&pdev->dev);
+
return 0;
 }
 
@@ -282,10 +293,12 @@ static int pwm_backlight_suspend(struct device *dev)
 
if (pb->notify)
pb->notify(pb->dev, 0);
-   pwm_config(pb->pwm, 0, pb->period);
-   pwm_disable(pb->pwm);
+
+   pwm_backlight_power_off(pb);
+
if (pb->notify_after)
pb->notify_after(pb->dev, 0);
+
return 0;
 }
 
@@ -294,6 +307,7 @@ static int pwm_backlight_resume(struct device *dev)
struct backlight_device *bl = dev_get_drvdata(dev);
 
backlight_update_status(bl);
+
return 0;
 }
 #endif
@@ -317,4 +331,3 @@ module_platform_driver(pwm_backlight_driver);
 MODULE_DESCRIPTION("PWM based Backlight Driver");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:pwm-backlight");
-
-- 
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html