leds: lm3530: fix handling of already enabled regulators

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.

---
 drivers/leds/leds-lm3530.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 968fd5f..859bf04 100644
@@ -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

2012/4/23 Mark Brown <[email protected]>:

The regulator_enable() is called in lm3530_init_registers().

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

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 -

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

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.

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 -

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