[PATCH 1/2] backlight/lp855x: Refactor dt parsing code

[PATCH 1/2] backlight/lp855x: Refactor dt parsing code

[Sean Paul]

This patch refactors the dt parsing code to avoid setting platform_data,
instead just setting lp->pdata directly. This facilitates easier
probe deferral since the current scheme would require us to clear out
dev->platform_data before deferring.

Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/video/backlight/lp855x_bl.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 25fb8e3..257b3ba 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -341,8 +341,10 @@ static const struct attribute_group lp855x_attr_group = {
 };
 
 #ifdef CONFIG_OF
-static int lp855x_parse_dt(struct device *dev, struct device_node *node)
+static int lp855x_parse_dt(struct lp855x *lp)
 {
+	struct device *dev = lp->dev;
+	struct device_node *node = dev->of_node;
 	struct lp855x_platform_data *pdata;
 	int rom_length;
 
@@ -381,12 +383,12 @@ static int lp855x_parse_dt(struct device *dev, struct device_node *node)
 		pdata->rom_data = &rom[0];
 	}
 
-	dev->platform_data = pdata;
+	lp->pdata = pdata;
 
 	return 0;
 }
 #else
-static int lp855x_parse_dt(struct device *dev, struct device_node *node)
+static int lp855x_parse_dt(struct lp855x *lp)
 {
 	return -EINVAL;
 }
@@ -395,18 +397,8 @@ static int lp855x_parse_dt(struct device *dev, struct device_node *node)
 static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
 {
 	struct lp855x *lp;
-	struct lp855x_platform_data *pdata = dev_get_platdata(&cl->dev);
-	struct device_node *node = cl->dev.of_node;
 	int ret;
 
-	if (!pdata) {
-		ret = lp855x_parse_dt(&cl->dev, node);
-		if (ret < 0)
-			return ret;
-
-		pdata = dev_get_platdata(&cl->dev);
-	}
-
 	if (!i2c_check_functionality(cl->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
 		return -EIO;
 
@@ -414,16 +406,23 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
 	if (!lp)
 		return -ENOMEM;
 
-	if (pdata->period_ns > 0)
-		lp->mode = PWM_BASED;
-	else
-		lp->mode = REGISTER_BASED;
-
 	lp->client = cl;
 	lp->dev = &cl->dev;
-	lp->pdata = pdata;
 	lp->chipname = id->name;
 	lp->chip_id = id->driver_data;
+	lp->pdata = dev_get_platdata(&cl->dev);
+
+	if (!lp->pdata) {
+		ret = lp855x_parse_dt(lp);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (lp->pdata->period_ns > 0)
+		lp->mode = PWM_BASED;
+	else
+		lp->mode = REGISTER_BASED;
+
 	i2c_set_clientdata(cl, lp);
 
 	ret = lp855x_configure(lp);
-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] backlight/lp855x: Add supply regulator to lp855x

[Sean Paul]

This patch adds a supply regulator to the lp855x platform data to facilitate
powering on/off the 3V rail attached to the controller.

Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Aaron Durbin <adurbin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 .../devicetree/bindings/video/backlight/lp855x.txt      |  2 ++
 drivers/video/backlight/lp855x_bl.c                     | 17 +++++++++++++++++
 include/linux/platform_data/lp855x.h                    |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
index 96e83a5..0a3ecbc 100644
--- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt
+++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
@@ -12,6 +12,7 @@ Optional properties:
   - pwm-period: PWM period value. Set only PWM input mode used (u32)
   - rom-addr: Register address of ROM area to be updated (u8)
   - rom-val: Register value to be updated (u8)
+  - power-supply: Regulator which controls the 3V rail
 
 Example:
 
@@ -56,6 +57,7 @@ Example:
 	backlight@2c {
 		compatible = "ti,lp8557";
 		reg = <0x2c>;
+		power-supply = <&backlight_vdd>;
 
 		dev-ctrl = /bits/ 8 <0x41>;
 		init-brt = /bits/ 8 <0x0a>;
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 257b3ba..8021009 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/platform_data/lp855x.h>
 #include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
 
 /* LP8550/1/2/3/6 Registers */
 #define LP855X_BRIGHTNESS_CTRL		0x00
@@ -383,6 +384,13 @@ 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;
@@ -423,6 +431,14 @@ 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);
+		if (ret < 0) {
+			dev_err(&cl->dev, "failed to enable supply: %d\n", ret);
+			return ret;
+		}
+	}
+
 	i2c_set_clientdata(cl, lp);
 
 	ret = lp855x_configure(lp);
@@ -454,6 +470,7 @@ static int lp855x_remove(struct i2c_client *cl)
 
 	lp->bl->props.brightness = 0;
 	backlight_update_status(lp->bl);
+	regulator_disable(lp->pdata->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 1b2ba24..9c7fd1e 100644
--- a/include/linux/platform_data/lp855x.h
+++ b/include/linux/platform_data/lp855x.h
@@ -136,6 +136,7 @@ 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;
@@ -144,6 +145,7 @@ struct lp855x_platform_data {
 	unsigned int period_ns;
 	int size_program;
 	struct lp855x_rom_data *rom_data;
+	struct regulator *supply;
 };
 
 #endif
-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] backlight/lp855x: Add supply regulator to lp855x

[Kim, Milo]

(Looping backlight class subsystem maintainers)

Hi Sean,

Please see my comments below. Thanks!
On 11/27/2014 4:11 AM, Sean Paul wrote:
> This patch adds a supply regulator to the lp855x platform data to facilitate
> powering on/off the 3V rail attached to the controller.
>
> Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Aaron Durbin <adurbin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>   .../devicetree/bindings/video/backlight/lp855x.txt      |  2 ++
>   drivers/video/backlight/lp855x_bl.c                     | 17 +++++++++++++++++
>   include/linux/platform_data/lp855x.h                    |  2 ++
>   3 files changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
> index 96e83a5..0a3ecbc 100644
> --- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt
> +++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
> @@ -12,6 +12,7 @@ Optional properties:
>     - pwm-period: PWM period value. Set only PWM input mode used (u32)
>     - rom-addr: Register address of ROM area to be updated (u8)
>     - rom-val: Register value to be updated (u8)
> +  - power-supply: Regulator which controls the 3V rail
>
>   Example:
>
> @@ -56,6 +57,7 @@ Example:
>   	backlight@2c {
>   		compatible = "ti,lp8557";
>   		reg = <0x2c>;
> +		power-supply = <&backlight_vdd>;
>
>   		dev-ctrl = /bits/ 8 <0x41>;
>   		init-brt = /bits/ 8 <0x0a>;
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 257b3ba..8021009 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -17,6 +17,7 @@
>   #include <linux/of.h>
>   #include <linux/platform_data/lp855x.h>
>   #include <linux/pwm.h>
> +#include <linux/regulator/consumer.h>
>
>   /* LP8550/1/2/3/6 Registers */
>   #define LP855X_BRIGHTNESS_CTRL		0x00
> @@ -383,6 +384,13 @@ 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;

This optional data, 'supply' is set to NULL.
Please refer to my comments below.

> +	}
> +
>   	lp->pdata = pdata;
>
>   	return 0;
> @@ -423,6 +431,14 @@ 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);
> +		if (ret < 0) {
> +			dev_err(&cl->dev, "failed to enable supply: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>   	i2c_set_clientdata(cl, lp);
>
>   	ret = lp855x_configure(lp);
> @@ -454,6 +470,7 @@ static int lp855x_remove(struct i2c_client *cl)
>
>   	lp->bl->props.brightness = 0;
>   	backlight_update_status(lp->bl);
> +	regulator_disable(lp->pdata->supply);

The 'lp->pdata->supply' is optional, what happens if it is NULL?
This will cause the kernel OOPS inside regulator_disable() API.

int regulator_disable(struct regulator *regulator)
{
	struct regulator_dev *rdev = regulator->rdev;
...
}

>   	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 1b2ba24..9c7fd1e 100644
> --- a/include/linux/platform_data/lp855x.h
> +++ b/include/linux/platform_data/lp855x.h
> @@ -136,6 +136,7 @@ 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;
> @@ -144,6 +145,7 @@ struct lp855x_platform_data {
>   	unsigned int period_ns;
>   	int size_program;
>   	struct lp855x_rom_data *rom_data;
> +	struct regulator *supply;
>   };
>
>   #endif
>

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] backlight/lp855x: Add supply regulator to lp855x

[Sean Paul]

On Wed, Nov 26, 2014 at 5:32 PM, Kim, Milo <milo.kim-l0cyMroinI0@public.gmane.org> wrote:
> (Looping backlight class subsystem maintainers)
>
> Hi Sean,
>
> Please see my comments below. Thanks!
>
> On 11/27/2014 4:11 AM, Sean Paul wrote:
>>
>> This patch adds a supply regulator to the lp855x platform data to
>> facilitate
>> powering on/off the 3V rail attached to the controller.
>>
>> Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Cc: Aaron Durbin <adurbin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>   .../devicetree/bindings/video/backlight/lp855x.txt      |  2 ++
>>   drivers/video/backlight/lp855x_bl.c                     | 17
>> +++++++++++++++++
>>   include/linux/platform_data/lp855x.h                    |  2 ++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt
>> b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
>> index 96e83a5..0a3ecbc 100644
>> --- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt
>> +++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
>> @@ -12,6 +12,7 @@ Optional properties:
>>     - pwm-period: PWM period value. Set only PWM input mode used (u32)
>>     - rom-addr: Register address of ROM area to be updated (u8)
>>     - rom-val: Register value to be updated (u8)
>> +  - power-supply: Regulator which controls the 3V rail
>>
>>   Example:
>>
>> @@ -56,6 +57,7 @@ Example:
>>         backlight@2c {
>>                 compatible = "ti,lp8557";
>>                 reg = <0x2c>;
>> +               power-supply = <&backlight_vdd>;
>>
>>                 dev-ctrl = /bits/ 8 <0x41>;
>>                 init-brt = /bits/ 8 <0x0a>;
>> diff --git a/drivers/video/backlight/lp855x_bl.c
>> b/drivers/video/backlight/lp855x_bl.c
>> index 257b3ba..8021009 100644
>> --- a/drivers/video/backlight/lp855x_bl.c
>> +++ b/drivers/video/backlight/lp855x_bl.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/of.h>
>>   #include <linux/platform_data/lp855x.h>
>>   #include <linux/pwm.h>
>> +#include <linux/regulator/consumer.h>
>>
>>   /* LP8550/1/2/3/6 Registers */
>>   #define LP855X_BRIGHTNESS_CTRL                0x00
>> @@ -383,6 +384,13 @@ 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;
>
>
> This optional data, 'supply' is set to NULL.
> Please refer to my comments below.
>
>> +       }
>> +
>>         lp->pdata = pdata;
>>
>>         return 0;
>> @@ -423,6 +431,14 @@ 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);
>> +               if (ret < 0) {
>> +                       dev_err(&cl->dev, "failed to enable supply: %d\n",
>> ret);
>> +                       return ret;
>> +               }
>> +       }
>> +
>>         i2c_set_clientdata(cl, lp);
>>
>>         ret = lp855x_configure(lp);
>> @@ -454,6 +470,7 @@ static int lp855x_remove(struct i2c_client *cl)
>>
>>         lp->bl->props.brightness = 0;
>>         backlight_update_status(lp->bl);
>> +       regulator_disable(lp->pdata->supply);
>
>
> The 'lp->pdata->supply' is optional, what happens if it is NULL?
> This will cause the kernel OOPS inside regulator_disable() API.
>

bleh. Thanks for catching this, I'll re-spin and send a new version next week.

Sean



> int regulator_disable(struct regulator *regulator)
> {
>         struct regulator_dev *rdev = regulator->rdev;
> ...
> }
>
>>         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 1b2ba24..9c7fd1e 100644
>> --- a/include/linux/platform_data/lp855x.h
>> +++ b/include/linux/platform_data/lp855x.h
>> @@ -136,6 +136,7 @@ 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;
>> @@ -144,6 +145,7 @@ struct lp855x_platform_data {
>>         unsigned int period_ns;
>>         int size_program;
>>         struct lp855x_rom_data *rom_data;
>> +       struct regulator *supply;
>>   };
>>
>>   #endif
>>
>
> Best regards,
> Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x

[Sean Paul]

This patch adds a supply regulator to the lp855x platform data to facilitate
powering on/off the 3V rail attached to the controller.

Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Aaron Durbin <adurbin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v2:
	- Add NULL check in lp855x_remove


 .../devicetree/bindings/video/backlight/lp855x.txt     |  2 ++
 drivers/video/backlight/lp855x_bl.c                    | 18 ++++++++++++++++++
 include/linux/platform_data/lp855x.h                   |  2 ++
 3 files changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/backlight/lp855x.txt b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
index 96e83a5..0a3ecbc 100644
--- a/Documentation/devicetree/bindings/video/backlight/lp855x.txt
+++ b/Documentation/devicetree/bindings/video/backlight/lp855x.txt
@@ -12,6 +12,7 @@ Optional properties:
   - pwm-period: PWM period value. Set only PWM input mode used (u32)
   - rom-addr: Register address of ROM area to be updated (u8)
   - rom-val: Register value to be updated (u8)
+  - power-supply: Regulator which controls the 3V rail
 
 Example:
 
@@ -56,6 +57,7 @@ Example:
 	backlight@2c {
 		compatible = "ti,lp8557";
 		reg = <0x2c>;
+		power-supply = <&backlight_vdd>;
 
 		dev-ctrl = /bits/ 8 <0x41>;
 		init-brt = /bits/ 8 <0x0a>;
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 257b3ba..a26d3bb 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/platform_data/lp855x.h>
 #include <linux/pwm.h>
+#include <linux/regulator/consumer.h>
 
 /* LP8550/1/2/3/6 Registers */
 #define LP855X_BRIGHTNESS_CTRL		0x00
@@ -383,6 +384,13 @@ 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;
@@ -423,6 +431,14 @@ 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);
+		if (ret < 0) {
+			dev_err(&cl->dev, "failed to enable supply: %d\n", ret);
+			return ret;
+		}
+	}
+
 	i2c_set_clientdata(cl, lp);
 
 	ret = lp855x_configure(lp);
@@ -454,6 +470,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);
 	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 1b2ba24..9c7fd1e 100644
--- a/include/linux/platform_data/lp855x.h
+++ b/include/linux/platform_data/lp855x.h
@@ -136,6 +136,7 @@ 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;
@@ -144,6 +145,7 @@ struct lp855x_platform_data {
 	unsigned int period_ns;
 	int size_program;
 	struct lp855x_rom_data *rom_data;
+	struct regulator *supply;
 };
 
 #endif
-- 
2.2.0.rc0.207.ga3a616c

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x

[Kim, Milo]

On 12/2/2014 6:07 AM, Sean Paul wrote:
> This patch adds a supply regulator to the lp855x platform data to facilitate
> powering on/off the 3V rail attached to the controller.
>
> Cc: Stéphane Marchesin<marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Aaron Durbin<adurbin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Sean Paul<seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: Milo Kim <milo.kim-l0cyMroinI0@public.gmane.org>

For effective power consumption, regulator could be enabled/disabled 
based on backlight control flow.
For example, when the brightness is 0, then call regulator_disable()
when the brightness is > 0 and regulator is off, then call 
regulator_enable().
However, I'd like to send ACK to this patch.

Sean,
Could you send the patch-set with the first one - 'backlight/lp855x: 
Refactor dt parsing code'? I'll send my ACK as well.
For the backlight maintainers, it would be better to apply them together.

Thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x

[Sean Paul]

On Tue, Dec 2, 2014 at 4:54 PM, Kim, Milo <milo.kim-l0cyMroinI0@public.gmane.org> wrote:
> On 12/2/2014 6:07 AM, Sean Paul wrote:
>>
>> This patch adds a supply regulator to the lp855x platform data to
>> facilitate
>> powering on/off the 3V rail attached to the controller.
>>
>> Cc: Stéphane Marchesin<marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Cc: Aaron Durbin<adurbin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Sean Paul<seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
>
> Acked-by: Milo Kim <milo.kim-l0cyMroinI0@public.gmane.org>
>
> For effective power consumption, regulator could be enabled/disabled based
> on backlight control flow.
> For example, when the brightness is 0, then call regulator_disable()
> when the brightness is > 0 and regulator is off, then call
> regulator_enable().
> However, I'd like to send ACK to this patch.
>
> Sean,
> Could you send the patch-set with the first one - 'backlight/lp855x:
> Refactor dt parsing code'? I'll send my ACK as well.

Hi Milo,
Thanks for looking at these. There aren't any changes to PATCH 1/2, is
there any value in resending the same patches?

Sean



> For the backlight maintainers, it would be better to apply them together.
>
> Thanks!
>
> Best regards,
> Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x

[Kim, Milo]

Hi Sean,

On 12/3/2014 10:01 AM, Sean Paul wrote:
> There aren't any changes to PATCH 1/2, is
> there any value in resending the same patches?

No. Title change is only required.
Please submit two patches in series like below.

	[PATCH v2 1/2] backlight/lp855x: Refactor dt parsing code
	[PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x

Thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x

[Bryan Wu]

On Tue, Dec 2, 2014 at 5:15 PM, Kim, Milo <milo.kim-l0cyMroinI0@public.gmane.org> wrote:
> Hi Sean,
>
> On 12/3/2014 10:01 AM, Sean Paul wrote:
>>
>> There aren't any changes to PATCH 1/2, is
>> there any value in resending the same patches?
>
>
> No. Title change is only required.
> Please submit two patches in series like below.
>
>         [PATCH v2 1/2] backlight/lp855x: Refactor dt parsing code
>         [PATCH v2 2/2] backlight/lp855x: Add supply regulator to lp855x
>

It looks good to me too. Please follow Milo's advice and I will give
my Ack then.

Thanks,
-Bryan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] backlight/lp855x: Refactor dt parsing code

[Kim, Milo]

(Looping backlight class subsystem maintainers)

Hi Sean,

Thanks for checking this. I'd like to describe why the original code is 
preferred.

The original code is copying the values of the lp855x platform data from 
the DT in advance of allocating lp855x data.
It guarantees returning quickly in case of invalid platform data.
In other words, no memory allocation of lp855x proceeds if parsing the 
DT gets failed.
However, this patch allocates the lp855x first and checking the DT.
Of course, devm_kzalloc() will free allocated memory later but original 
code does NOT try to allocate it.
So I'd prefer to keep this way.

Best regards,
Milo

On 11/27/2014 4:11 AM, Sean Paul wrote:
> This patch refactors the dt parsing code to avoid setting platform_data,
> instead just setting lp->pdata directly. This facilitates easier
> probe deferral since the current scheme would require us to clear out
> dev->platform_data before deferring.
>
> Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>   drivers/video/backlight/lp855x_bl.c | 37 ++++++++++++++++++-------------------
>   1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 25fb8e3..257b3ba 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -341,8 +341,10 @@ static const struct attribute_group lp855x_attr_group = {
>   };
>
>   #ifdef CONFIG_OF
> -static int lp855x_parse_dt(struct device *dev, struct device_node *node)
> +static int lp855x_parse_dt(struct lp855x *lp)
>   {
> +	struct device *dev = lp->dev;
> +	struct device_node *node = dev->of_node;
>   	struct lp855x_platform_data *pdata;
>   	int rom_length;
>
> @@ -381,12 +383,12 @@ static int lp855x_parse_dt(struct device *dev, struct device_node *node)
>   		pdata->rom_data = &rom[0];
>   	}
>
> -	dev->platform_data = pdata;
> +	lp->pdata = pdata;
>
>   	return 0;
>   }
>   #else
> -static int lp855x_parse_dt(struct device *dev, struct device_node *node)
> +static int lp855x_parse_dt(struct lp855x *lp)
>   {
>   	return -EINVAL;
>   }
> @@ -395,18 +397,8 @@ static int lp855x_parse_dt(struct device *dev, struct device_node *node)
>   static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
>   {
>   	struct lp855x *lp;
> -	struct lp855x_platform_data *pdata = dev_get_platdata(&cl->dev);
> -	struct device_node *node = cl->dev.of_node;
>   	int ret;
>
> -	if (!pdata) {
> -		ret = lp855x_parse_dt(&cl->dev, node);
> -		if (ret < 0)
> -			return ret;
> -
> -		pdata = dev_get_platdata(&cl->dev);
> -	}
> -
>   	if (!i2c_check_functionality(cl->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
>   		return -EIO;
>
> @@ -414,16 +406,23 @@ static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
>   	if (!lp)
>   		return -ENOMEM;
>
> -	if (pdata->period_ns > 0)
> -		lp->mode = PWM_BASED;
> -	else
> -		lp->mode = REGISTER_BASED;
> -
>   	lp->client = cl;
>   	lp->dev = &cl->dev;
> -	lp->pdata = pdata;
>   	lp->chipname = id->name;
>   	lp->chip_id = id->driver_data;
> +	lp->pdata = dev_get_platdata(&cl->dev);
> +
> +	if (!lp->pdata) {
> +		ret = lp855x_parse_dt(lp);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (lp->pdata->period_ns > 0)
> +		lp->mode = PWM_BASED;
> +	else
> +		lp->mode = REGISTER_BASED;
> +
>   	i2c_set_clientdata(cl, lp);
>
>   	ret = lp855x_configure(lp);
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] backlight/lp855x: Refactor dt parsing code

[Sean Paul]

On Wed, Nov 26, 2014 at 5:32 PM, Kim, Milo <milo.kim-l0cyMroinI0@public.gmane.org> wrote:
> (Looping backlight class subsystem maintainers)
>
> Hi Sean,
>
> Thanks for checking this. I'd like to describe why the original code is
> preferred.
>
> The original code is copying the values of the lp855x platform data from the
> DT in advance of allocating lp855x data.
> It guarantees returning quickly in case of invalid platform data.
> In other words, no memory allocation of lp855x proceeds if parsing the DT
> gets failed.
> However, this patch allocates the lp855x first and checking the DT.
> Of course, devm_kzalloc() will free allocated memory later but original code
> does NOT try to allocate it.
> So I'd prefer to keep this way.
>

Hi Milo,
Thanks for the quick response. The reason I'm changing the existing
code is that it causes problems in the probe deferral case.

A concrete example: In my follow-on patch, I call devm_regulator_get
in the parse_dt function. Assume that something later on in probe()
returns -EPROBEDEFER. If we didn't have this patch, dev->platform_data
would continue to be set the next time probe() was called. This would
causes us to skip the parse_dt() function (because pdata is non-null
from last time) and we would have an invalid pointer to the regulator
we got the last time.

Sean



> Best regards,
> Milo
>
>
> On 11/27/2014 4:11 AM, Sean Paul wrote:
>>
>> This patch refactors the dt parsing code to avoid setting platform_data,
>> instead just setting lp->pdata directly. This facilitates easier
>> probe deferral since the current scheme would require us to clear out
>> dev->platform_data before deferring.
>>
>> Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>   drivers/video/backlight/lp855x_bl.c | 37
>> ++++++++++++++++++-------------------
>>   1 file changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/video/backlight/lp855x_bl.c
>> b/drivers/video/backlight/lp855x_bl.c
>> index 25fb8e3..257b3ba 100644
>> --- a/drivers/video/backlight/lp855x_bl.c
>> +++ b/drivers/video/backlight/lp855x_bl.c
>> @@ -341,8 +341,10 @@ static const struct attribute_group lp855x_attr_group
>> = {
>>   };
>>
>>   #ifdef CONFIG_OF
>> -static int lp855x_parse_dt(struct device *dev, struct device_node *node)
>> +static int lp855x_parse_dt(struct lp855x *lp)
>>   {
>> +       struct device *dev = lp->dev;
>> +       struct device_node *node = dev->of_node;
>>         struct lp855x_platform_data *pdata;
>>         int rom_length;
>>
>> @@ -381,12 +383,12 @@ static int lp855x_parse_dt(struct device *dev,
>> struct device_node *node)
>>                 pdata->rom_data = &rom[0];
>>         }
>>
>> -       dev->platform_data = pdata;
>> +       lp->pdata = pdata;
>>
>>         return 0;
>>   }
>>   #else
>> -static int lp855x_parse_dt(struct device *dev, struct device_node *node)
>> +static int lp855x_parse_dt(struct lp855x *lp)
>>   {
>>         return -EINVAL;
>>   }
>> @@ -395,18 +397,8 @@ static int lp855x_parse_dt(struct device *dev, struct
>> device_node *node)
>>   static int lp855x_probe(struct i2c_client *cl, const struct
>> i2c_device_id *id)
>>   {
>>         struct lp855x *lp;
>> -       struct lp855x_platform_data *pdata = dev_get_platdata(&cl->dev);
>> -       struct device_node *node = cl->dev.of_node;
>>         int ret;
>>
>> -       if (!pdata) {
>> -               ret = lp855x_parse_dt(&cl->dev, node);
>> -               if (ret < 0)
>> -                       return ret;
>> -
>> -               pdata = dev_get_platdata(&cl->dev);
>> -       }
>> -
>>         if (!i2c_check_functionality(cl->adapter,
>> I2C_FUNC_SMBUS_I2C_BLOCK))
>>                 return -EIO;
>>
>> @@ -414,16 +406,23 @@ static int lp855x_probe(struct i2c_client *cl, const
>> struct i2c_device_id *id)
>>         if (!lp)
>>                 return -ENOMEM;
>>
>> -       if (pdata->period_ns > 0)
>> -               lp->mode = PWM_BASED;
>> -       else
>> -               lp->mode = REGISTER_BASED;
>> -
>>         lp->client = cl;
>>         lp->dev = &cl->dev;
>> -       lp->pdata = pdata;
>>         lp->chipname = id->name;
>>         lp->chip_id = id->driver_data;
>> +       lp->pdata = dev_get_platdata(&cl->dev);
>> +
>> +       if (!lp->pdata) {
>> +               ret = lp855x_parse_dt(lp);
>> +               if (ret < 0)
>> +                       return ret;
>> +       }
>> +
>> +       if (lp->pdata->period_ns > 0)
>> +               lp->mode = PWM_BASED;
>> +       else
>> +               lp->mode = REGISTER_BASED;
>> +
>>         i2c_set_clientdata(cl, lp);
>>
>>         ret = lp855x_configure(lp);
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html