Messages in this thread | | | Date | Sat, 11 Jul 2015 05:52:22 +0900 | From | "Kim, Milo" <> | Subject | Re: [PATCH v2] backlight: lp855x: use private data for regulator control |
| |
Hi Paul,
On 7/11/2015 5:49 AM, Sean Paul wrote: > On Fri, Jul 10, 2015 at 4:43 PM, Kim, Milo <milo.kim@ti.com> wrote: >> Hi Paul, >> >> >> On 7/11/2015 12:01 AM, Sean Paul wrote: >>> >>> On Fri, Jul 10, 2015 at 4:26 AM, Milo Kim <milo.kim@ti.com> wrote: >>>> >>>> LP855x backlight device can be enabled by external VDD input. >>>> The 'supply' data is used for this purpose. >>>> It's kind of private data which runs internally, so there is no reason to >>>> expose to the platform data. >>>> >>>> And devm_regulator_get() is moved from _parse_dt() to _probe(). >>>> Regulator consumer(lp855x) can control regulator not only from DT but >>>> also >>>> from platform data configuration in a source file such like board-*.c. >>>> >>>> If 'power' regulator driver is not ready, lp855x should continue to work >>>> because the power supply control is optional. So -EPROBE_DEFER return >>>> code >>>> is removed. >>>> >>>> v1->v2: >>>> Keeps optional property '<name>-supply' in LP855x DT binding. >>>> >>>> Cc: Sean Paul <seanpaul@chromium.org> >>>> Cc: Jingoo Han <jingoohan1@gmail.com> >>>> Cc: Lee Jones <lee.jones@linaro.org> >>>> Cc: linux-kernel@vger.kernel.org >>>> Signed-off-by: Milo Kim <milo.kim@ti.com> >>>> --- >>>> drivers/video/backlight/lp855x_bl.c | 20 +++++++++----------- >>>> include/linux/platform_data/lp855x.h | 2 -- >>>> 2 files changed, 9 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/video/backlight/lp855x_bl.c >>>> b/drivers/video/backlight/lp855x_bl.c >>>> index a26d3bb..277d5ca 100644 >>>> --- a/drivers/video/backlight/lp855x_bl.c >>>> +++ b/drivers/video/backlight/lp855x_bl.c >>>> @@ -73,6 +73,7 @@ struct lp855x { >>>> struct device *dev; >>>> struct lp855x_platform_data *pdata; >>>> struct pwm_device *pwm; >>>> + struct regulator *supply; /* regulator for VDD input */ >>>> }; >>>> >>>> static int lp855x_write_byte(struct lp855x *lp, u8 reg, u8 data) >>>> @@ -384,13 +385,6 @@ static int lp855x_parse_dt(struct lp855x *lp) >>>> pdata->rom_data = &rom[0]; >>>> } >>>> >>>> - pdata->supply = devm_regulator_get(dev, "power"); >>>> - if (IS_ERR(pdata->supply)) { >>>> - if (PTR_ERR(pdata->supply) == -EPROBE_DEFER) >>>> - return -EPROBE_DEFER; >>>> - pdata->supply = NULL; >>>> - } >>>> - >>>> lp->pdata = pdata; >>>> >>>> return 0; >>>> @@ -431,8 +425,12 @@ static int lp855x_probe(struct i2c_client *cl, const >>>> struct i2c_device_id *id) >>>> else >>>> lp->mode = REGISTER_BASED; >>>> >>>> - if (lp->pdata->supply) { >>>> - ret = regulator_enable(lp->pdata->supply); >>>> + lp->supply = devm_regulator_get(lp->dev, "power"); >>>> + if (IS_ERR(lp->supply)) >>>> + lp->supply = NULL; >>> >>> >>> >>> Hi Milo, >>> You removed the probe deferral handling on the regulator and broke >>> probe deferral in cases where the regulator isn't ready. >> >> >> This power supply is optional. Even if lp855x can not get regulator driver, >> it should work. (And I saw same comment in the DT. The 'power-supply' >> property is optional). So -EPORBE_DEFER is not necessary in _probe(). >> > > I respectfully disagree. devm_regulator_get can return EPROBE_DEFER if > the regulator is valid (and specified in the dt), but not ready to be > used yet. In this case, your patch will assume it doesn't exist and > will never use it. This Is Bad.
So do you think this power supply should be mandatory in this driver?
Best regards, Milo
| |