OK. Agree! From: Axel Lin [mailto:axel.lin@xxxxxxxxxx] Sent: Thursday, December 20, 2012 11:10 AM To: Shreshtha Kumar SAHU Cc: Milo(Woogyom) Kim; Richard Purdie; linux-leds@xxxxxxxxxxxxxxx; Bryan Wu Subject: Re: [PATCH] leds: lm3530: Ensure drvdata->enable has correct status if regulator_disable fails 2012/12/20 Shreshtha Kumar SAHU <shreshthakumar.sahu@xxxxxxxxxxxxxx> Agree! So wouldn't this be enough instead of moving regulator_enable/disable to a separate helper function --- a/drivers/leds/leds-lm3530.c +++ b/drivers/leds/leds-lm3530.c @@ -295,13 +295,15 @@ static void lm3530_brightness_set(struct led_classdev *led_cdev, drvdata->brightness = brt_val; if (brt_val == 0) { + drvdata->enable = false; err = regulator_disable(drvdata->regulator); - if (err) + if (err) { dev_err(&drvdata->client->dev, "Disable regulator failed\n"); + drvdata->enable = true; + } - drvdata->enable = false; } break; case LM3530_BL_MODE_ALS: Hmm, I think a simple fix can be just add "return;" after dev_err. But I feel adding lm3530_led_enable() and lm3530_led_disable() helper functions can make the code better in readability. With above helper function, we can then get rid of the "if (drvdata->enable)" or "if (!drvdata->enable)" checking before every regulator_enable()/regulator_disable() call. Just call lm3530_led_enable() and lm3530_led_disable() instead. Regards, Axel -- 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
