[PATCH v2] backlight: lp855x: use private data for regulator control

[PATCH v2] backlight: lp855x: use private data for regulator control

[Milo Kim]

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;
+
+	if (lp->supply) {
+		ret = regulator_enable(lp->supply);
 		if (ret < 0) {
 			dev_err(&cl->dev, "failed to enable supply: %d\n", ret);
 			return ret;
@@ -470,8 +468,8 @@ static int lp855x_remove(struct i2c_client *cl)
 
 	lp->bl->props.brightness = 0;
 	backlight_update_status(lp->bl);
-	if (lp->pdata->supply)
-		regulator_disable(lp->pdata->supply);
+	if (lp->supply)
+		regulator_disable(lp->supply);
 	sysfs_remove_group(&lp->dev->kobj, &lp855x_attr_group);
 
 	return 0;
diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h
index 9c7fd1e..1b2ba24 100644
--- a/include/linux/platform_data/lp855x.h
+++ b/include/linux/platform_data/lp855x.h
@@ -136,7 +136,6 @@ struct lp855x_rom_data {
 		Only valid when mode is PWM_BASED.
  * @size_program : total size of lp855x_rom_data
  * @rom_data : list of new eeprom/eprom registers
- * @supply : regulator that supplies 3V input
  */
 struct lp855x_platform_data {
 	const char *name;
@@ -145,7 +144,6 @@ struct lp855x_platform_data {
 	unsigned int period_ns;
 	int size_program;
 	struct lp855x_rom_data *rom_data;
-	struct regulator *supply;
 };
 
 #endif
-- 
1.9.1

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Sean Paul]

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.

Please re-introduce the error handling you removed from parse_dt.

Sean

> +
> +       if (lp->supply) {
> +               ret = regulator_enable(lp->supply);
>                 if (ret < 0) {
>                         dev_err(&cl->dev, "failed to enable supply: %d\n", ret);
>                         return ret;
> @@ -470,8 +468,8 @@ static int lp855x_remove(struct i2c_client *cl)
>
>         lp->bl->props.brightness = 0;
>         backlight_update_status(lp->bl);
> -       if (lp->pdata->supply)
> -               regulator_disable(lp->pdata->supply);
> +       if (lp->supply)
> +               regulator_disable(lp->supply);
>         sysfs_remove_group(&lp->dev->kobj, &lp855x_attr_group);
>
>         return 0;
> diff --git a/include/linux/platform_data/lp855x.h b/include/linux/platform_data/lp855x.h
> index 9c7fd1e..1b2ba24 100644
> --- a/include/linux/platform_data/lp855x.h
> +++ b/include/linux/platform_data/lp855x.h
> @@ -136,7 +136,6 @@ struct lp855x_rom_data {
>                 Only valid when mode is PWM_BASED.
>   * @size_program : total size of lp855x_rom_data
>   * @rom_data : list of new eeprom/eprom registers
> - * @supply : regulator that supplies 3V input
>   */
>  struct lp855x_platform_data {
>         const char *name;
> @@ -145,7 +144,6 @@ struct lp855x_platform_data {
>         unsigned int period_ns;
>         int size_program;
>         struct lp855x_rom_data *rom_data;
> -       struct regulator *supply;
>  };
>
>  #endif
> --
> 1.9.1
>

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

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().

Best regards,
Milo

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Sean Paul]

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.

Sean

> Best regards,
> Milo

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

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

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Kim, Milo]

Hi Paul,

On 7/11/2015 5:52 AM, Kim, Milo wrote:
> 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?

The devm_regulator_get_optinonal() seems the right API for this case.
Let me take a look at and get back to you.

Best regards,
Milo

Re: [PATCH v2] backlight: lp855x: use private data for regulator control

[Jingoo Han]

On Saturday, July 11, 2015 6:11 AM, Kim, Milo wrote:
> On 7/11/2015 5:52 AM, Kim, Milo wrote:
> > 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(-)
> >>>>>

[.....]

> >>>>
> >>>>
> >>>> 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?
> 
> The devm_regulator_get_optinonal() seems the right API for this case.
> Let me take a look at and get back to you.

The devm_regulator_get_optinonal() looks better. But, I am not sure.
Please review devm_regulator_get_optinonal() carefully, before sending
the V3 patch.

Best regards,
Jingoo Han

> 
> Best regards,
> Milo