> > From: Axel Lin [mailto:axel.lin@xxxxxxxxxx] > > Sent: Thursday, January 03, 2013 11:01 AM > > To: Bryan Wu > > Cc: Kim, Milo; Shreshtha Kumar SAHU; Richard Purdie; linux- > > leds@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH] leds: lm3530: Ensure drvdata->enable has correct > > status if regulator_disable fails > > > > This driver uses regulator_get() rather than > regulator_get_exclusive(). > > So I think we need the drvdata->enable to determinate if this driver > > needs to call regulator_enable/disable, which will add reference > count > > if the regulator is already enabled by other driver. > > > > Calls to regulator_enable() must be balanced with calls to > > regulator_disable(). > > Having a drvdata->enable flag in this driver also ensures we have > > balance calls > > for regulator_enable/regulator_disable. > > I think no need to consider the use_count in regulator-consumer side, > but I'll update if after testing in the real board. Here is an update. The conclusion is maintaining private data, 'enable' is better. It's for not only balancing the use_count of 'vin' regulator but also simple handling of initializing registers. Balance the use_count of 'vin' : In LM3530 driver, the regulator is enabled when registers are initialized. On the other hand, this regulator is disabled when the brightness is set to 0. What if setting the brightness to 0 and unloading the driver? Then regulator_disable() is called twice. Here is another case. If the platform data, 'brt_val' is configured as non-zero integer value, 'vin' regulator turns on while loading the driver. And then the brightness is changed by an LED application, registers should not be initialized because this job is already done in _probe(). To guarantee this condition, 'enable' data is used in the driver. (please refer to the code below) static void lm3530_brightness_set(struct led_classdev *led_cdev, enum led_brightness brt_val) { ... switch (drvdata->mode) { case LM3530_BL_MODE_MANUAL: if (!drvdata->enable) { err = lm3530_init_registers(drvdata); if (err) { dev_err(&drvdata->client->dev, "Register Init failed: %d\n", err); break; } } ... if (brt_val == 0) lm3530_led_disable(drvdata); We may think 'drvdata->enable' can be replaced with regulator API, regulator_is_enabled(), but it has a problem. If the platform data, 'brt_val' is 0, then the regulator stays off on loading the driver. Then lm3530_init_registers() should be called to enable the regulator, but it never happens because regulator_is_enabled() is always false. It sounds like the question of which came first, the chicken or the egg ;) Therefore, maintaining 'enable' private data is more simpler way than changing the flow. I'll do ACK on previous Axel's patch. Best Regards, Milo -- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
