[PATCH] leds: lm3530: fix handling of already enabled regulators

[PATCH] leds: lm3530: fix handling of already enabled regulators

[Axel Lin]

It is possible that the regulator has been previously enabled by bootloader or
kernel board initialization code. Thus, don't assume the regulator is disabled
by default. Get the correct status by regulator_is_enabled().

The leds-lm3530 driver was ignoring the initial status of the
regulator; this resulted in rdev->use_count being incremented to 2 after
calling lm3530_init_registers() in the .probe method when a regulator
was already enabled at insmod time, which made it impossible to ever
disable the regulator.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 drivers/leds/leds-lm3530.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/leds-lm3530.c b/drivers/leds/leds-lm3530.c
index 968fd5f..859bf04 100644
--- a/drivers/leds/leds-lm3530.c
+++ b/drivers/leds/leds-lm3530.c
@@ -397,7 +397,6 @@ static int __devinit lm3530_probe(struct i2c_client *client,
 	drvdata->client = client;
 	drvdata->pdata = pdata;
 	drvdata->brightness = LED_OFF;
-	drvdata->enable = false;
 	drvdata->led_dev.name = LM3530_LED_DEV;
 	drvdata->led_dev.brightness_set = lm3530_brightness_set;
 	drvdata->led_dev.max_brightness = MAX_BRIGHTNESS;
@@ -411,6 +410,7 @@ static int __devinit lm3530_probe(struct i2c_client *client,
 		drvdata->regulator = NULL;
 		goto err_regulator_get;
 	}
+	drvdata->enable = regulator_is_enabled(drvdata->regulator);
 
 	if (drvdata->pdata->brt_val) {
 		err = lm3530_init_registers(drvdata);
-- 
1.7.5.4

Re: [PATCH] leds: lm3530: fix handling of already enabled regulators

[Mark Brown]

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

On Sun, Apr 22, 2012 at 11:11:48PM +0800, Axel Lin wrote:

>  		drvdata->regulator = NULL;
>  		goto err_regulator_get;
>  	}
> +	drvdata->enable = regulator_is_enabled(drvdata->regulator);
>  

This isn't sufficient for what the driver is doing - it also needs to
enable the regulator.  Regulator enables are reference counted (since
the regulator can be shared by many device supplies) so if this driver
doesn't reference the regulator when it's needed then it could get
powered off.

If this regulator is the actual LED then the current default of assuming
the regulator is off on boot isn't ideal either as it means that if the
regulator is enabled on boot then the LED may be left on even though the
driver thinks it's off.

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

Re: [PATCH] leds: lm3530: fix handling of already enabled regulators

[Axel Lin]

2012/4/23 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Sun, Apr 22, 2012 at 11:11:48PM +0800, Axel Lin wrote:
>
>>               drvdata->regulator = NULL;
>>               goto err_regulator_get;
>>       }
>> +     drvdata->enable = regulator_is_enabled(drvdata->regulator);
>>
>
> This isn't sufficient for what the driver is doing - it also needs to
> enable the regulator.  Regulator enables are reference counted (since
> the regulator can be shared by many device supplies) so if this driver
> doesn't reference the regulator when it's needed then it could get
> powered off.

The regulator_enable() is called in lm3530_init_registers().

>
> If this regulator is the actual LED then the current default of assuming
> the regulator is off on boot isn't ideal either as it means that if the
> regulator is enabled on boot then the LED may be left on even though the
> driver thinks it's off.

Hi Shreshtha,
I think without this patch you can see the symptom Mark mentioned if the
regulator is enabled on boot.
Any chance to test if this patch works or not?

Regards,
Axel

RE: [PATCH] leds: lm3530: fix handling of already enabled regulators

[Kim, Milo]

> 
> > On Sun, Apr 22, 2012 at 11:11:48PM +0800, Axel Lin wrote:
> >
> >>               drvdata->regulator = NULL;
> >>               goto err_regulator_get;
> >>       }
> >> +     drvdata->enable = regulator_is_enabled(drvdata->regulator);
> >>
> >
> > This isn't sufficient for what the driver is doing - it also needs to
> > enable the regulator.  Regulator enables are reference counted (since
> > the regulator can be shared by many device supplies) so if this
> driver
> > doesn't reference the regulator when it's needed then it could get
> > powered off.
> 
> The regulator_enable() is called in lm3530_init_registers().

I think this patch has a mismatched condition between the value of variable and actual status of the regulator.

Let's suppose that the regulator (named "vin") is disabled after the lm3530 driver is probed.

Step 1. The 'vin' regulator is on

Step 2. lm3530 driver is probed
lm3530_probe()
{
	/* drvdata->enabled is true at this moment */
	drvdata->enable = regulator_is_enabled(drvdata->regulator);
	...
}

Step 3. lm3530_init_registers() is called
lm3530_init_registers()
{
	/* skip enabling the regulator because the condition is true */
	if (!drvdata->enable) {
		ret = regulator_enable(drvdata->regulator);
		...
	}
}

Step 4. The regulator is disabled by another driver.
	(It is possible because the regulator reference count is not incremented yet by the lm3530 driver)

Step 5. If the brightness mode is changed from the ALS to manual, lm3530_init_registers() is called again.

lm3530_init_registers()
{
	/* In this case, regulator should be on,
	   but it will skip again because the variable is still true */
	if (!drvdata->enable) {
		ret = regulator_enable(drvdata->regulator);
		...
	}
}

The lm3530_probe() is called once.
But lm3530_init_registers() can be called whenever the brightness mode is changed.
(e.g from ALS to manual or manual to PWM, and so on)
So the status of regulator can be mismatched.

In my opinion, current source code is the simplest way.

	1. The variable 'drvdata->enabled' is initialized as false.
	2. The regulator is enabled and the variable is updated when the driver is probed.
	   (If the regulator is already on, it's ok - keep going ...)
	3. The variable is updated when regulator_enable()/regulator_disable() is called by the lm3530 driver.
	   (If another driver tries to turn off the regulator, it can't be allowed
	    because the reference count was incremented by the lm3530 driver.)

Thanks & BR
Milo -

Re: [PATCH] leds: lm3530: fix handling of already enabled regulators

[Axel Lin]

> In my opinion, current source code is the simplest way.
>
>        1. The variable 'drvdata->enabled' is initialized as false.
>        2. The regulator is enabled and the variable is updated when the driver is probed.
>           (If the regulator is already on, it's ok - keep going ...)
>        3. The variable is updated when regulator_enable()/regulator_disable() is called by the lm3530 driver.
>           (If another driver tries to turn off the regulator, it can't be allowed
>            because the reference count was incremented by the lm3530 driver.)

No. Current code is buggy.

Let me put it this way: Assume only this driver is using the regulator.
If the regulator is enabled on boot, which means the reference count
is init to 1.

In probe, the driver assumes the enable variable is false by default and it
calls regulator_enable() in lm3530_init_registers(). This increases
the reference count to 2.

Then if we set brt_val to 0 in lm3530_brightness_set, which will then call
regulator_disable(). In this case, the reference count becomes 1, note that
the reference count is not 0 so the LED may be left on even though the driver
thinks it's off.

Regards,
Axel

RE: [PATCH] leds: lm3530: fix handling of already enabled regulators

[Kim, Milo]

> 
> Let me put it this way: Assume only this driver is using the regulator.
> If the regulator is enabled on boot, which means the reference count
> is init to 1.
> 

Could you explain what 'enabled on boot' means ?
1) The regulator_enable() is called in initial time
or
2) boot_on = 1 in regulator_init_data ?

If you mean 'boot_on = 1' in regulator_init_data, then no use_count increase.

> Then if we set brt_val to 0 in lm3530_brightness_set, which will then
> call
> regulator_disable(). 

It looks buggy code in lm3530_brightness_set().
Even the regulator_disable() is failed, drvdata->enable is set to false.
I think drvdata->enable is changed to false only when the regulator is off successfully.

Thanks & BR
Milo -

Re: [PATCH] leds: lm3530: fix handling of already enabled regulators

[Axel Lin]

> Could you explain what 'enabled on boot' means ?
> 1) The regulator_enable() is called in initial time
> or
> 2) boot_on = 1 in regulator_init_data ?
>
> If you mean 'boot_on = 1' in regulator_init_data, then no use_count increase.

I mean boot_on = 1 in regulator_init_data.

I just checked the regulator core, and you are right that
no use_count increase if boot_on = 1 in this case.

use_count will be increased if boot_on = 1 && using regulator_get_exclusive.

Regards,
Axel