[PATCH v4 3/5] leds-lm3530: support pwm input mode

[PATCH v4 3/5] leds-lm3530: support pwm input mode

[Kim, Milo]

* add 'struct lm3530_pwm_data' in the platform data
  The pwm data is the platform specific functions which generate the pwm.
  The pwm data is only valid when brightness is pwm input mode.
  Functions should be implemented by the pwm driver.
  pwm_set_intensity() : set duty of pwm.
  pwm_get_intensity() : get current the brightness.

* brightness control by pwm
  If the control mode is pwm, then brightness is changed by the duty of pwm.
  So pwm platform function should be called in lm3530_brightness_set().

* do not update brightness register when pwm input mode
  In pwm input mode, brightness register is not used.
  If any value is updated in this register, then the led will be off.

* when input mode is changed, set duty of pwm to 0 if unnecessary.

* patch base version : kernel 3.2.4

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/leds/leds-lm3530.c |   30 ++++++++++++++++++++++--------
 include/linux/led-lm3530.h |    9 +++++++++
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
index 8886959..8ec7b2b 100644
--- a/drivers/leds/leds-lm3530.c
+++ b/drivers/leds/leds-lm3530.c
@@ -157,6 +157,7 @@ static int lm3530_init_registers(struct lm3530_data *drvdata)
 	u32 als_vmin, als_vmax, als_vstep;
 	struct lm3530_platform_data *pltfm = drvdata->pdata;
 	struct i2c_client *client = drvdata->client;
+	struct lm3530_pwm_data *pwm = &pltfm->pwm_data;
 
 	gen_config = (pltfm->brt_ramp_law << LM3530_RAMP_LAW_SHIFT) |
 			((pltfm->max_current & 7) << LM3530_MAX_CURR_SHIFT);
@@ -240,6 +241,15 @@ static int lm3530_init_registers(struct lm3530_data *drvdata)
 	}
 
 	for (i = 0; i < LM3530_REG_MAX; i++) {
+		/* do not update brightness register when pwm mode */
+		if (lm3530_reg[i] == LM3530_BRT_CTRL_REG &&
+		    drvdata->mode == LM3530_BL_MODE_PWM) {
+			if (pwm->pwm_set_intensity)
+				pwm->pwm_set_intensity(reg_val[i],
+					drvdata->led_dev.max_brightness);
+			continue;
+		}
+
 		ret = i2c_smbus_write_byte_data(client,
 				lm3530_reg[i], reg_val[i]);
 		if (ret)
@@ -255,6 +265,9 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
 	int err;
 	struct lm3530_data *drvdata =
 	    container_of(led_cdev, struct lm3530_data, led_dev);
+	struct lm3530_platform_data *pdata = drvdata->pdata;
+	struct lm3530_pwm_data *pwm = &pdata->pwm_data;
+	u8 max_brightness = led_cdev->max_brightness;
 
 	switch (drvdata->mode) {
 	case LM3530_BL_MODE_MANUAL:
@@ -288,6 +301,8 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev,
 	case LM3530_BL_MODE_ALS:
 		break;
 	case LM3530_BL_MODE_PWM:
+		if (pwm->pwm_set_intensity)
+			pwm->pwm_set_intensity(brt_val, max_brightness);
 		break;
 	default:
 		break;
@@ -319,6 +334,8 @@ static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct lm3530_data *drvdata =
 	    container_of(led_cdev, struct lm3530_data, led_dev);
+	struct lm3530_pwm_data *pwm = &drvdata->pdata->pwm_data;
+	u8 max_brightness = led_cdev->max_brightness;
 	int mode, err;
 
 	mode = lm3530_get_mode_from_str(buf);
@@ -327,14 +344,11 @@ static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
 		return -EINVAL;
 	}
 
-	if (mode == LM3530_BL_MODE_MANUAL)
-		drvdata->mode = LM3530_BL_MODE_MANUAL;
-	else if (mode == LM3530_BL_MODE_ALS)
-		drvdata->mode = LM3530_BL_MODE_ALS;
-	else if (mode == LM3530_BL_MODE_PWM) {
-		dev_err(dev, "PWM mode not supported\n");
-		return -EINVAL;
-	}
+	drvdata->mode = mode;
+
+	/* set pwm to low if unnecessary */
+	if (mode != LM3530_BL_MODE_PWM && pwm->pwm_set_intensity)
+		pwm->pwm_set_intensity(0, max_brightness);
 
 	err = lm3530_init_registers(drvdata);
 	if (err) {
diff --git a/include/linux/led-lm3530.h b/include/linux/led-lm3530.h
index 8eb1235..eeae6e7 100644
--- a/include/linux/led-lm3530.h
+++ b/include/linux/led-lm3530.h
@@ -72,6 +72,12 @@ enum lm3530_als_mode {
 	LM3530_INPUT_CEIL,	/* Max of ALS1 and ALS2 */
 };
 
+/* PWM Platform Specific Data */
+struct lm3530_pwm_data {
+	void (*pwm_set_intensity) (int brightness, int max_brightness);
+	int (*pwm_get_intensity) (int max_brightness);
+};
+
 /**
  * struct lm3530_platform_data
  * @mode: mode of operation i.e. Manual, ALS or PWM
@@ -87,6 +93,7 @@ enum lm3530_als_mode {
  * @als_vmin: als input voltage calibrated for max brightness in mV
  * @als_vmax: als input voltage calibrated for min brightness in mV
  * @brt_val: brightness value (0-255)
+ * @pwm_data: PWM control functions (only valid when the mode is PWM)
  */
 struct lm3530_platform_data {
 	enum lm3530_mode mode;
@@ -107,6 +114,8 @@ struct lm3530_platform_data {
 	u32 als_vmax;
 
 	u8 brt_val;
+
+	struct lm3530_pwm_data pwm_data;
 };
 
 #endif	/* _LINUX_LED_LM3530_H__ */
-- 
1.7.4.1


Best Regards,
Milo (Woogyom) Kim
Texas Instruments Incorporated

Re: [PATCH v4 3/5] leds-lm3530: support pwm input mode

[Andrew Morton]

On Mon, 6 Feb 2012 21:14:21 -0800
"Kim, Milo" <Milo.Kim@ti.com> wrote:

> * add 'struct lm3530_pwm_data' in the platform data
>   The pwm data is the platform specific functions which generate the pwm.
>   The pwm data is only valid when brightness is pwm input mode.
>   Functions should be implemented by the pwm driver.
>   pwm_set_intensity() : set duty of pwm.
>   pwm_get_intensity() : get current the brightness.
> 
> * brightness control by pwm
>   If the control mode is pwm, then brightness is changed by the duty of pwm.
>   So pwm platform function should be called in lm3530_brightness_set().
> 
> * do not update brightness register when pwm input mode
>   In pwm input mode, brightness register is not used.
>   If any value is updated in this register, then the led will be off.
> 
> * when input mode is changed, set duty of pwm to 0 if unnecessary.
> 
> * patch base version : kernel 3.2.4
> 
>
> ...
>
> @@ -327,14 +344,11 @@ static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
>  		return -EINVAL;
>  	}
>  
> -	if (mode == LM3530_BL_MODE_MANUAL)
> -		drvdata->mode = LM3530_BL_MODE_MANUAL;
> -	else if (mode == LM3530_BL_MODE_ALS)
> -		drvdata->mode = LM3530_BL_MODE_ALS;
> -	else if (mode == LM3530_BL_MODE_PWM) {
> -		dev_err(dev, "PWM mode not supported\n");
> -		return -EINVAL;
> -	}
> +	drvdata->mode = mode;
> +
> +	/* set pwm to low if unnecessary */
> +	if (mode != LM3530_BL_MODE_PWM && pwm->pwm_set_intensity)
> +		pwm->pwm_set_intensity(0, max_brightness);

lm3530_mode_set() has no local variable "pwm" and has no local variable
"brightness".

I see now that you prepared the patches against 3.2.4, which might
contribute to this problem, but I don't see how the patches could work
in 3.2 either.  Perhaps 3.2.4 is significantly different from 3.2.

Anyway, I think I will drop all the patches again until we can get this
fully sorted out.  Please don't prepare patches against ancient old
kernels like 3.2.4!  They should be prepared and tested against the
latest Linus tree, from
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git.  Or
against the latest linux-next from 
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

RE: [PATCH v4 3/5] leds-lm3530: support pwm input mode

[Kim, Milo]

> -----Original Message-----
> From: Andrew Morton [mailto:akpm@linux-foundation.org]
> Sent: Wednesday, February 08, 2012 7:55 AM
> To: Kim, Milo
> Cc: Linus Walleij; shreshthakumar.sahu@stericsson.com;
> rpurdie@rpsys.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4 3/5] leds-lm3530: support pwm input mode
> 
> On Mon, 6 Feb 2012 21:14:21 -0800
> "Kim, Milo" <Milo.Kim@ti.com> wrote:
> 
> > * add 'struct lm3530_pwm_data' in the platform data
> >   The pwm data is the platform specific functions which generate the
> pwm.
> >   The pwm data is only valid when brightness is pwm input mode.
> >   Functions should be implemented by the pwm driver.
> >   pwm_set_intensity() : set duty of pwm.
> >   pwm_get_intensity() : get current the brightness.
> >
> > * brightness control by pwm
> >   If the control mode is pwm, then brightness is changed by the duty
> of pwm.
> >   So pwm platform function should be called in
> lm3530_brightness_set().
> >
> > * do not update brightness register when pwm input mode
> >   In pwm input mode, brightness register is not used.
> >   If any value is updated in this register, then the led will be off.
> >
> > * when input mode is changed, set duty of pwm to 0 if unnecessary.
> >
> > * patch base version : kernel 3.2.4
> >
> >
> > ...
> >
> > @@ -327,14 +344,11 @@ static ssize_t lm3530_mode_set(struct device
> *dev, struct device_attribute
> >  		return -EINVAL;
> >  	}
> >
> > -	if (mode == LM3530_BL_MODE_MANUAL)
> > -		drvdata->mode = LM3530_BL_MODE_MANUAL;
> > -	else if (mode == LM3530_BL_MODE_ALS)
> > -		drvdata->mode = LM3530_BL_MODE_ALS;
> > -	else if (mode == LM3530_BL_MODE_PWM) {
> > -		dev_err(dev, "PWM mode not supported\n");
> > -		return -EINVAL;
> > -	}
> > +	drvdata->mode = mode;
> > +
> > +	/* set pwm to low if unnecessary */
> > +	if (mode != LM3530_BL_MODE_PWM && pwm->pwm_set_intensity)
> > +		pwm->pwm_set_intensity(0, max_brightness);
> 
> lm3530_mode_set() has no local variable "pwm" and has no local variable
> "brightness".

Local variable "pwm" was added in this patch.
[PATCH v4 3/5] leds-lm3530: support pwm input mode

@@ -255,6 +265,9 @@ static void lm3530_brightness_set(struct
 	int err;
 	struct lm3530_data *drvdata =
 	    container_of(led_cdev, struct lm3530_data, led_dev);
+	struct lm3530_platform_data *pdata = drvdata->pdata;
+	struct lm3530_pwm_data *pwm = &pdata->pwm_data;
+	u8 max_brightness = led_cdev->max_brightness;

And I can see the same code in your mm tree mail.

> 
> I see now that you prepared the patches against 3.2.4, which might
> contribute to this problem, but I don't see how the patches could work
> in 3.2 either.  Perhaps 3.2.4 is significantly different from 3.2.
> 

For the leds-lm3530 driver, there is no difference between 3.2 and 3.2.4.
I'm curious what happened to the patch. Can I have any message from your working tree ?

Thanks & BR
Milo -

Re: [PATCH v4 3/5] leds-lm3530: support pwm input mode

[Andrew Morton]

On Tue, 7 Feb 2012 16:32:14 -0800 "Kim, Milo" <Milo.Kim@ti.com> wrote:

> > lm3530_mode_set() has no local variable "pwm" and has no local variable
> > "brightness".
> 
> Local variable "pwm" was added in this patch.
> [PATCH v4 3/5] leds-lm3530: support pwm input mode
> 

hm, so it was.  Sorry, I must have lost that bit when fixing rejects.

I shall fix things up...