> -----Original Message----- > From: Bryan Wu [mailto:cooloney@xxxxxxxxx] > Sent: Thursday, January 03, 2013 10:23 AM > To: Kim, Milo > Cc: Shreshtha Kumar SAHU; Axel Lin; Richard Purdie; linux- > leds@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] leds: lm3530: Ensure drvdata->enable has correct > status if regulator_disable fails > > 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? > Ah, I missed it. You're right. regulator_is_enabled() is unnecessary. And no private data is needed, either. Thanks, 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
