On Thu, Dec 20, 2012 at 3:42 PM, Kim, Milo <Milo.Kim@xxxxxx> wrote: > Hi Shreshtha, > >> -----Original Message----- >> From: Shreshtha Kumar SAHU [mailto:shreshthakumar.sahu@xxxxxxxxxxxxxx] >> Sent: Thursday, December 20, 2012 2:28 PM >> To: Axel Lin >> Cc: Kim, Milo; Richard Purdie; linux-leds@xxxxxxxxxxxxxxx; Bryan Wu >> Subject: RE: [PATCH] leds: lm3530: Ensure drvdata->enable has correct >> status if regulator_disable fails >> >> 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: >> > If private data, 'enable' is used for the status of 'vin', I would suggest using > regulator_is_enabled() rather than private data. > > Best Regards, > Milo Good point, Milo. But I found regulator_enable() will check _regulator_is_enabled() itself, do we still need this private data drvdata->enable to do that? I think we can simply remove this! Axel and Shreshtha, how do you think about this? -Bryan -- 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
