[PATCH 1/2] lp855x_bl: use generic PWM functions

[PATCH 1/2] lp855x_bl: use generic PWM functions

[Kim, Milo]

 The LP855x family devices support the PWM input for the backlight control.
 Period of the PWM is configurable in the platform side.
 Platform specific functions are unnecessary anymore because
 generic PWM functions are used inside the driver.

 (PWM input mode)
 To set the brightness, new lp855x_pwm_ctrl() is used.
 If a PWM device is not allocated, devm_pwm_get() is called.
 The PWM consumer name is from the chip name such as 'lp8550' and 'lp8556'.
 To get the brightness value, no additional handling is required.
 Just the value of 'props.brightness' is returned.

 If the PWM driver is not ready while initializing the LP855x driver, it's OK.
 The PWM device can be retrieved later, when the brightness value is changed.

 Documentation is updated with an example.

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 Documentation/backlight/lp855x-driver.txt |   10 ++-------
 drivers/video/backlight/lp855x_bl.c       |   34 +++++++++++++++++++----------
 include/linux/platform_data/lp855x.h      |    9 ++------
 3 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/Documentation/backlight/lp855x-driver.txt b/Documentation/backlight/lp855x-driver.txt
index f5e4caa..1529394 100644
--- a/Documentation/backlight/lp855x-driver.txt
+++ b/Documentation/backlight/lp855x-driver.txt
@@ -35,11 +35,8 @@ For supporting platform specific data, the lp855x platform data can be used.
 * mode : Brightness control mode. PWM or register based.
 * device_control : Value of DEVICE CONTROL register.
 * initial_brightness : Initial value of backlight brightness.
-* pwm_data : Platform specific pwm generation functions.
+* period_ns : Platform specific PWM period value. unit is nano.
 	     Only valid when brightness is pwm input mode.
-	     Functions should be implemented by PWM driver.
-	     - pwm_set_intensity() : set duty of PWM
-	     - pwm_get_intensity() : get current duty of PWM
 * load_new_rom_data :
 	0 : use default configuration data
 	1 : update values of eeprom or eprom registers on loading driver
@@ -71,8 +68,5 @@ static struct lp855x_platform_data lp8556_pdata = {
 	.mode = PWM_BASED,
 	.device_control = PWM_CONFIG(LP8556),
 	.initial_brightness = INITIAL_BRT,
-	.pwm_data = {
-		     .pwm_set_intensity = platform_pwm_set_intensity,
-		     .pwm_get_intensity = platform_pwm_get_intensity,
-		     },
+	.period_ns = 1000000,
 };
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index aa6d4f7..c4fd72f 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -15,6 +15,7 @@
 #include <linux/backlight.h>
 #include <linux/err.h>
 #include <linux/platform_data/lp855x.h>
+#include <linux/pwm.h>
 
 /* Registers */
 #define BRIGHTNESS_CTRL		0x00
@@ -36,6 +37,7 @@ struct lp855x {
 	struct device *dev;
 	struct mutex xfer_lock;
 	struct lp855x_platform_data *pdata;
+	struct pwm_device *pwm;
 };
 
 static int lp855x_read_byte(struct lp855x *lp, u8 reg, u8 *data)
@@ -121,6 +123,25 @@ static int lp855x_init_registers(struct lp855x *lp)
 	return ret;
 }
 
+static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
+{
+	unsigned int period = lp->pdata->period_ns;
+	unsigned int duty = br * period / max_br;
+	struct pwm_device *pwm;
+
+	/* request pwm device with the consumer name */
+	if (!lp->pwm) {
+		pwm = devm_pwm_get(lp->dev, lp->chipname);
+		if (IS_ERR(pwm))
+			return;
+
+		lp->pwm = pwm;
+	}
+
+	pwm_config(lp->pwm, duty, period);
+	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);
+}
+
 static int lp855x_bl_update_status(struct backlight_device *bl)
 {
 	struct lp855x *lp = bl_get_data(bl);
@@ -130,12 +151,10 @@ static int lp855x_bl_update_status(struct backlight_device *bl)
 		bl->props.brightness = 0;
 
 	if (mode == PWM_BASED) {
-		struct lp855x_pwm_data *pd = &lp->pdata->pwm_data;
 		int br = bl->props.brightness;
 		int max_br = bl->props.max_brightness;
 
-		if (pd->pwm_set_intensity)
-			pd->pwm_set_intensity(br, max_br);
+		lp855x_pwm_ctrl(lp, br, max_br);
 
 	} else if (mode == REGISTER_BASED) {
 		u8 val = bl->props.brightness;
@@ -150,14 +169,7 @@ static int lp855x_bl_get_brightness(struct backlight_device *bl)
 	struct lp855x *lp = bl_get_data(bl);
 	enum lp855x_brightness_ctrl_mode mode = lp->pdata->mode;
 
-	if (mode == PWM_BASED) {
-		struct lp855x_pwm_data *pd = &lp->pdata->pwm_data;
-		int max_br = bl->props.max_brightness;
-
-		if (pd->pwm_get_intensity)
-			bl->props.brightness = pd->pwm_get_intensity(max_br);
-
-	} else if (mode == REGISTER_BASED) {
+	if (mode == REGISTER_BASED) {
 		u8 val = 0;
 
 		lp855x_read_byte(lp, BRIGHTNESS_CTRL, &val);
diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h
index 761f317..e81f62d 100644
--- a/include/linux/platform_data/lp855x.h
+++ b/include/linux/platform_data/lp855x.h
@@ -89,11 +89,6 @@ enum lp8556_brightness_source {
 	LP8556_COMBINED2,	/* pwm + i2c after the shaper block */
 };
 
-struct lp855x_pwm_data {
-	void (*pwm_set_intensity) (int brightness, int max_brightness);
-	int (*pwm_get_intensity) (int max_brightness);
-};
-
 struct lp855x_rom_data {
 	u8 addr;
 	u8 val;
@@ -105,7 +100,7 @@ struct lp855x_rom_data {
  * @mode : brightness control by pwm or lp855x register
  * @device_control : value of DEVICE CONTROL register
  * @initial_brightness : initial value of backlight brightness
- * @pwm_data : platform specific pwm generation functions.
+ * @period_ns : platform specific pwm period value. unit is nano.
 		Only valid when mode is PWM_BASED.
  * @load_new_rom_data :
 	0 : use default configuration data
@@ -118,7 +113,7 @@ struct lp855x_platform_data {
 	enum lp855x_brightness_ctrl_mode mode;
 	u8 device_control;
 	int initial_brightness;
-	struct lp855x_pwm_data pwm_data;
+	unsigned int period_ns;
 	u8 load_new_rom_data;
 	int size_program;
 	struct lp855x_rom_data *rom_data;
-- 
1.7.9.5


Best Regards,
Milo

Re: [PATCH 1/2] lp855x_bl: use generic PWM functions

[Thierry Reding]

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

On Fri, Oct 19, 2012 at 08:11:50AM +0000, Kim, Milo wrote:
>  The LP855x family devices support the PWM input for the backlight control.
>  Period of the PWM is configurable in the platform side.
>  Platform specific functions are unnecessary anymore because
>  generic PWM functions are used inside the driver.
> 
>  (PWM input mode)
>  To set the brightness, new lp855x_pwm_ctrl() is used.
>  If a PWM device is not allocated, devm_pwm_get() is called.
>  The PWM consumer name is from the chip name such as 'lp8550' and 'lp8556'.
>  To get the brightness value, no additional handling is required.
>  Just the value of 'props.brightness' is returned.
> 
>  If the PWM driver is not ready while initializing the LP855x driver, it's OK.
>  The PWM device can be retrieved later, when the brightness value is changed.
> 
>  Documentation is updated with an example.
> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>

Generally this looks good. Obviously you'll need to update any users of
this driver as well. It might make sense to include those changes in
this patch to avoid interim build failures.

Other than that I have just one smaller comment below.

> @@ -121,6 +123,25 @@ static int lp855x_init_registers(struct lp855x *lp)
>  	return ret;
>  }
>  
> +static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> +{
> +	unsigned int period = lp->pdata->period_ns;
> +	unsigned int duty = br * period / max_br;
> +	struct pwm_device *pwm;
> +
> +	/* request pwm device with the consumer name */
> +	if (!lp->pwm) {
> +		pwm = devm_pwm_get(lp->dev, lp->chipname);
> +		if (IS_ERR(pwm))
> +			return;
> +
> +		lp->pwm = pwm;
> +	}
> +
> +	pwm_config(lp->pwm, duty, period);
> +	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);

This is really ugly and should be written explicitly:

	if (duty == 0)
		pwm_disable(lp->pwm);
	else
		pwm_enable(lp->pwm);

Thierry

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

RE: [PATCH 1/2] lp855x_bl: use generic PWM functions

[Kim, Milo]

> Generally this looks good. Obviously you'll need to update any users of
> this driver as well. It might make sense to include those changes in
> this patch to avoid interim build failures.

Thanks for your review.
So far no usages for this driver in the mainline.
I've tested it in my own development environment instead.

> Other than that I have just one smaller comment below.
> 
> > +	pwm_config(lp->pwm, duty, period);
> > +	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);
> 
> This is really ugly and should be written explicitly:
> 
> 	if (duty == 0)
> 		pwm_disable(lp->pwm);
> 	else
> 		pwm_enable(lp->pwm);

Oh, I prefer using '?' to if-sentence because it looks clear to me.
But if it's difficult to read/understand, I'll fix it.
I'd like to have others' opinion.

Thank you

Best Regards,
Milo 

Re: [PATCH 1/2] lp855x_bl: use generic PWM functions

[Andrew Morton]

On Fri, 19 Oct 2012 08:45:14 +0000
"Kim, Milo" <Milo.Kim@ti.com> wrote:

> > Generally this looks good. Obviously you'll need to update any users of
> > this driver as well. It might make sense to include those changes in
> > this patch to avoid interim build failures.
> 
> Thanks for your review.
> So far no usages for this driver in the mainline.
> I've tested it in my own development environment instead.
> 
> > Other than that I have just one smaller comment below.
> > 
> > > +	pwm_config(lp->pwm, duty, period);
> > > +	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);
> > 
> > This is really ugly and should be written explicitly:
> > 
> > 	if (duty == 0)
> > 		pwm_disable(lp->pwm);
> > 	else
> > 		pwm_enable(lp->pwm);
> 
> Oh, I prefer using '?' to if-sentence because it looks clear to me.
> But if it's difficult to read/understand, I'll fix it.
> I'd like to have others' opinion.
> 

Hey, it's better than

	(*(duty ? pwm_enable : pwm_disable))(lp->pwm);

!


But yes, the original code is unusual and I think most kernel people
would have to stare at it for a bit longer than necessary to see
exactly what it's doing.

--- a/drivers/video/backlight/lp855x_bl.c~drivers-video-backlight-lp855x_blc-use-generic-pwm-functions-fix
+++ a/drivers/video/backlight/lp855x_bl.c
@@ -139,7 +139,10 @@ static void lp855x_pwm_ctrl(struct lp855
 	}
 
 	pwm_config(lp->pwm, duty, period);
-	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);
+	if (duty)
+		pwm_enable(lp->pwm);
+	else
+		pwm_disable(lp->pwm);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)

Re: [PATCH 1/2] lp855x_bl: use generic PWM functions

[Thierry Reding]

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

On Mon, Oct 22, 2012 at 03:30:12PM -0700, Andrew Morton wrote:
> On Fri, 19 Oct 2012 08:45:14 +0000
> "Kim, Milo" <Milo.Kim@ti.com> wrote:
> 
> > > Generally this looks good. Obviously you'll need to update any users of
> > > this driver as well. It might make sense to include those changes in
> > > this patch to avoid interim build failures.
> > 
> > Thanks for your review.
> > So far no usages for this driver in the mainline.
> > I've tested it in my own development environment instead.
> > 
> > > Other than that I have just one smaller comment below.
> > > 
> > > > +	pwm_config(lp->pwm, duty, period);
> > > > +	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);
> > > 
> > > This is really ugly and should be written explicitly:
> > > 
> > > 	if (duty == 0)
> > > 		pwm_disable(lp->pwm);
> > > 	else
> > > 		pwm_enable(lp->pwm);
> > 
> > Oh, I prefer using '?' to if-sentence because it looks clear to me.
> > But if it's difficult to read/understand, I'll fix it.
> > I'd like to have others' opinion.
> > 
> 
> Hey, it's better than
> 
> 	(*(duty ? pwm_enable : pwm_disable))(lp->pwm);
> 
> !

Indeed. Fortunately there don't seem to be overly many of those. Anyway,
thanks for taking these patches.

Thierry

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