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

[PATCH v2 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@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.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

[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@chromium.org>
Cc: Aaron Durbin <adurbin@chromium.org>
Acked-by: Milo Kim <milo.kim@ti.com>
Signed-off-by: Sean Paul <seanpaul@chromium.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

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

[Kim, Milo]

On 12/3/2014 10:39 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@chromium.org>
> Cc: Aaron Durbin<adurbin@chromium.org>
> Acked-by: Milo Kim<milo.kim@ti.com>
> Signed-off-by: Sean Paul<seanpaul@chromium.org>

Acked-by: Milo Kim <milo.kim@ti.com>

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

[Jingoo Han]

On Wednesday, December 03, 2014 10:39 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@chromium.org>
> Cc: Aaron Durbin <adurbin@chromium.org>
> Acked-by: Milo Kim <milo.kim@ti.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Acked-by: Jingoo Han <jg1.han@samsung.com>

Best regards,
Jingoo Han

> ---
> 
> 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

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

[Bryan Wu]

On Tue, Dec 2, 2014 at 5:39 PM, Sean Paul <seanpaul@chromium.org> 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@chromium.org>
> Cc: Aaron Durbin <adurbin@chromium.org>
> Acked-by: Milo Kim <milo.kim@ti.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Acked-by: Bryan Wu <cooloney@gmail.com>

Thanks,
-Bryan

> ---
>
> 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
>

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

[Lee Jones]

On Tue, 02 Dec 2014, 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@chromium.org>
> Cc: Aaron Durbin <adurbin@chromium.org>
> Acked-by: Milo Kim <milo.kim@ti.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.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(+)

Applied with Acks, thanks.

> 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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Arnd Bergmann]

On Tuesday 02 December 2014 17:39:12 Sean Paul wrote:
> 
>  .../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(+)

While your two patches are both correct (and applied already), I took
a look at the platform_data header and noticed that all users of this
file have been converted to DT a while ago, so it would be nice to clean
it up by integrating the linux/platform_data/lp855x.h header file into the
drivers/video/backlight/lp855x_bl.c itself.

You can then merge struct lp855x_platform_data into lp855x to avoid the
extra dynamic allocation and remove all the 'pdata' references in the
driver.

	Arnd

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

[Sean Paul]

On Wed, Dec 3, 2014 at 6:11 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 02 December 2014 17:39:12 Sean Paul wrote:
>>
>>  .../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(+)
>
> While your two patches are both correct (and applied already), I took
> a look at the platform_data header and noticed that all users of this
> file have been converted to DT a while ago, so it would be nice to clean
> it up by integrating the linux/platform_data/lp855x.h header file into the
> drivers/video/backlight/lp855x_bl.c itself.
>
> You can then merge struct lp855x_platform_data into lp855x to avoid the
> extra dynamic allocation and remove all the 'pdata' references in the
> driver.
>

Hi Arnd,
Thanks for the suggestion. Indeed, that will be much cleaner, I'll
send a follow-up patch.

Sean

>         Arnd

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

[Kim, Milo]

Hi Arnd,

Good to talk to you again. It's been a long time since I met you in 
Linaro Connect HK few years ago :)

On 12/3/2014 11:11 PM, Arnd Bergmann wrote:
> While your two patches are both correct (and applied already), I took
> a look at the platform_data header and noticed that all users of this
> file have been converted to DT a while ago, so it would be nice to clean
> it up by integrating the linux/platform_data/lp855x.h header file into the
> drivers/video/backlight/lp855x_bl.c itself.

I like this cleanup but need to consider few things.

- Platform which does not support the DT
In this case, there is no way to configure options like backlight device 
control mode. Without the DT, only default options are set.
To change the settings, the driver modification is required.
I don't want to write project/platform dependency code inside the driver.

- Backward compatibility
Some customers have been using this platform data in their board-*.c 
(but this *.c has not been mainlined yet).
My concern is the backward compatibility issue may happen in their projects.

Best regards,
Milo

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

[Jingoo Han]

On Tuesday, December 09, 2014 12:04 PM, Kim, Milo wrote:
> 
> Hi Arnd,
> 
> Good to talk to you again. It's been a long time since I met you in
> Linaro Connect HK few years ago :)
> 
> On 12/3/2014 11:11 PM, Arnd Bergmann wrote:
> > While your two patches are both correct (and applied already), I took
> > a look at the platform_data header and noticed that all users of this
> > file have been converted to DT a while ago, so it would be nice to clean
> > it up by integrating the linux/platform_data/lp855x.h header file into the
> > drivers/video/backlight/lp855x_bl.c itself.
> 
> I like this cleanup but need to consider few things.
> 
> - Platform which does not support the DT
> In this case, there is no way to configure options like backlight device
> control mode. Without the DT, only default options are set.
> To change the settings, the driver modification is required.
> I don't want to write project/platform dependency code inside the driver.
> 
> - Backward compatibility
> Some customers have been using this platform data in their board-*.c
> (but this *.c has not been mainlined yet).
> My concern is the backward compatibility issue may happen in their projects.

I understand this situation. In the case of Exynos DisplayPort driver,
a similar issue happened. At that time, the conclusion was that
out-of-tree code (not mainlined yet) cannot be considered.

In this context, it is valuable to upstream the code to mainline kernel.
I think that one of the followings can be considered.

  1. Ask customers to upstream their machine code
  2. Ask customers to use DT

If they don't want to these things, someone (you or customers)
will need to make the patch for non-DT support, whenever new
kernel is released and is used by projects not supporting DT.

Best regards,
Jingoo Han

> 
> Best regards,
> Milo

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

[Arnd Bergmann]

On Tuesday 09 December 2014 12:46:15 Jingoo Han wrote:
> On Tuesday, December 09, 2014 12:04 PM, Kim, Milo wrote:
> > 
> > Hi Arnd,
> > 
> > Good to talk to you again. It's been a long time since I met you in
> > Linaro Connect HK few years ago 
> > 
> > On 12/3/2014 11:11 PM, Arnd Bergmann wrote:
> > > While your two patches are both correct (and applied already), I took
> > > a look at the platform_data header and noticed that all users of this
> > > file have been converted to DT a while ago, so it would be nice to clean
> > > it up by integrating the linux/platform_data/lp855x.h header file into the
> > > drivers/video/backlight/lp855x_bl.c itself.
> > 
> > I like this cleanup but need to consider few things.
> > 
> > - Platform which does not support the DT
> > In this case, there is no way to configure options like backlight device
> > control mode. Without the DT, only default options are set.
> > To change the settings, the driver modification is required.
> > I don't want to write project/platform dependency code inside the driver.
> > 
> > - Backward compatibility
> > Some customers have been using this platform data in their board-*.c
> > (but this *.c has not been mainlined yet).
> > My concern is the backward compatibility issue may happen in their projects.
> 
> I understand this situation. In the case of Exynos DisplayPort driver,
> a similar issue happened. At that time, the conclusion was that
> out-of-tree code (not mainlined yet) cannot be considered.

Right. Basically when the only reason to have code in the kernel is
to support someone with an out-of-tree patch set, they can be expected
to add the extra bit to their patch set, it doesn't add any extra burden
to them, and it makes our lives easier.

	Arnd

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

[Bryan Wu]

On Tue, Dec 2, 2014 at 5:39 PM, Sean Paul <seanpaul@chromium.org> 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.
>

Looks good to me,
Acked-by: Bryan Wu <cooloney@gmail.com>

Please also wait for Milo to ack. Lee, can you take this patch?

Thanks,
-Bryan

> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.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
>

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

[Kim, Milo]

On 12/3/2014 10:39 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@chromium.org>
> Signed-off-by: Sean Paul<seanpaul@chromium.org>

Acked-by: Milo Kim <milo.kim@ti.com>

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

[Jingoo Han]

On Wednesday, December 03, 2014 10:39 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@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Acked-by: Jingoo Han <jg1.han@samsung.com>

Best regards,
Jingoo Han

> ---
>  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

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

[Lee Jones]

On Tue, 02 Dec 2014, 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@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/video/backlight/lp855x_bl.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)

Applied with Acks, thanks.

> 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);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog