| Message ID | 1379972467-11243-2-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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"); -
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 <treding@nvidia.com> --- drivers/video/backlight/pwm_bl.c | 59 ++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 23 deletions(-)