[PATCHv4] video: backlight: gpio-backlight: Add DT support.

[PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Denis Carikli]

Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
---
ChangeLog v3->v4:
- The default-brightness property is now optional, it defaults to 1 if not set.
- def_value int becomes an u32.
- gbl->def_value was set to pdata->def_value in pdata mode to avoid an extra
  check.
---
 .../bindings/video/backlight/gpio-backlight.txt    |   20 ++++++
 drivers/video/backlight/gpio_backlight.c           |   69 ++++++++++++++++++--
 2 files changed, 82 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt

diff --git a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
new file mode 100644
index 0000000..3474d4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
@@ -0,0 +1,20 @@
+gpio-backlight bindings
+
+Required properties:
+  - compatible: "gpio-backlight"
+  - gpios: describes the gpio that is used for enabling/disabling the backlight
+     (see GPIO binding[0] for more details).
+
+Optional properties:
+  - default-brightness-level: the default brightness level (can be 0(off) or
+      1(on) since GPIOs only support theses levels).
+
+[0]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+
+	backlight {
+		compatible = "gpio-backlight";
+		gpios = <&gpio3 4 0>;
+		default-brightness-level = <0>;
+	};
diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
index 81fb127..248124d 100644
--- a/drivers/video/backlight/gpio_backlight.c
+++ b/drivers/video/backlight/gpio_backlight.c
@@ -13,6 +13,8 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_data/gpio_backlight.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -23,6 +25,7 @@ struct gpio_backlight {
 
 	int gpio;
 	int active;
+	u32 def_value;
 };
 
 static int gpio_backlight_update_status(struct backlight_device *bl)
@@ -60,6 +63,41 @@ static const struct backlight_ops gpio_backlight_ops = {
 	.check_fb	= gpio_backlight_check_fb,
 };
 
+static int gpio_backlight_probe_dt(struct platform_device *pdev,
+				   struct gpio_backlight *gbl)
+{
+	struct device_node *np = pdev->dev.of_node;
+	enum of_gpio_flags gpio_flags;
+	int ret;
+
+	gbl->fbdev = NULL;
+	gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags);
+
+	gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+	if (gbl->gpio == -EPROBE_DEFER) {
+		return ERR_PTR(-EPROBE_DEFER);
+	} else if (gbl->gpio < 0) {
+		dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
+		return gbl->gpio;
+	}
+
+	ret = of_property_read_u32(np, "default-brightness-level",
+				   &gbl->def_value);
+	if (ret < 0) {
+		/* The property is optional. */
+		gbl->def_value = 1;
+	}
+
+	if (gbl->def_value > 1) {
+		dev_warn(&pdev->dev,
+			"Warning: Invalid default-brightness-level value. Its value can be either 0(off) or 1(on).\n");
+		gbl->def_value = 1;
+	}
+
+	return 0;
+}
+
 static int gpio_backlight_probe(struct platform_device *pdev)
 {
 	struct gpio_backlight_platform_data *pdata =
@@ -67,10 +105,12 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 	struct backlight_properties props;
 	struct backlight_device *bl;
 	struct gpio_backlight *gbl;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "failed to find platform data\n");
+	if (!pdata && !np) {
+		dev_err(&pdev->dev,
+			"failed to find platform data or device tree node.\n");
 		return -ENODEV;
 	}
 
@@ -79,14 +119,22 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	gbl->dev = &pdev->dev;
-	gbl->fbdev = pdata->fbdev;
-	gbl->gpio = pdata->gpio;
-	gbl->active = pdata->active_low ? 0 : 1;
+
+	if (np) {
+		ret = gpio_backlight_probe_dt(pdev, gbl);
+		if (ret)
+			return ret;
+	} else {
+		gbl->fbdev = pdata->fbdev;
+		gbl->gpio = pdata->gpio;
+		gbl->active = pdata->active_low ? 0 : 1;
+		gbl->def_value = pdata->def_value;
+	}
 
 	ret = devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT |
 				    (gbl->active ? GPIOF_INIT_LOW
 						 : GPIOF_INIT_HIGH),
-				    pdata->name);
+				    pdata ? pdata->name : "backlight");
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to request GPIO\n");
 		return ret;
@@ -103,17 +151,24 @@ static int gpio_backlight_probe(struct platform_device *pdev)
 		return PTR_ERR(bl);
 	}
 
-	bl->props.brightness = pdata->def_value;
+	bl->props.brightness = gbl->def_value;
+
 	backlight_update_status(bl);
 
 	platform_set_drvdata(pdev, bl);
 	return 0;
 }
 
+static struct of_device_id gpio_backlight_of_match[] = {
+	{ .compatible = "gpio-backlight" },
+	{ /* sentinel */ }
+};
+
 static struct platform_driver gpio_backlight_driver = {
 	.driver		= {
 		.name		= "gpio-backlight",
 		.owner		= THIS_MODULE,
+		.of_match_table = of_match_ptr(gpio_backlight_of_match),
 	},
 	.probe		= gpio_backlight_probe,
 };
-- 
1.7.9.5

--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Laurent Pinchart]

Hi Denis,

Thanks for the patch. Please see below for a couple of small comment.

On Monday 21 October 2013 11:13:33 Denis Carikli wrote:
> Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> ---
> ChangeLog v3->v4:
> - The default-brightness property is now optional, it defaults to 1 if not
> set. - def_value int becomes an u32.
> - gbl->def_value was set to pdata->def_value in pdata mode to avoid an extra
> check.
> ---
>  .../bindings/video/backlight/gpio-backlight.txt    |   20 ++++++
>  drivers/video/backlight/gpio_backlight.c           |   69 +++++++++++++++--
>  2 files changed, 82 insertions(+), 7 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt new
> file mode 100644
> index 0000000..3474d4a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> @@ -0,0 +1,20 @@
> +gpio-backlight bindings
> +
> +Required properties:
> +  - compatible: "gpio-backlight"
> +  - gpios: describes the gpio that is used for enabling/disabling the
> backlight
> +     (see GPIO binding[0] for more details).
> +
> +Optional properties:
> +  - default-brightness-level: the default brightness level (can be 0(off)
> or
> +      1(on) since GPIOs only support theses levels).

I believe you should specify what the default value is when the property isn't 
available.

> +
> +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +
> +	backlight {
> +		compatible = "gpio-backlight";
> +		gpios = <&gpio3 4 0>;
> +		default-brightness-level = <0>;
> +	};
> diff --git a/drivers/video/backlight/gpio_backlight.c
> b/drivers/video/backlight/gpio_backlight.c index 81fb127..248124d 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -13,6 +13,8 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_data/gpio_backlight.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -23,6 +25,7 @@ struct gpio_backlight {
> 
>  	int gpio;
>  	int active;
> +	u32 def_value;
>  };
> 
>  static int gpio_backlight_update_status(struct backlight_device *bl)
> @@ -60,6 +63,41 @@ static const struct backlight_ops gpio_backlight_ops = {
>  	.check_fb	= gpio_backlight_check_fb,
>  };
> 
> +static int gpio_backlight_probe_dt(struct platform_device *pdev,
> +				   struct gpio_backlight *gbl)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	enum of_gpio_flags gpio_flags;
> +	int ret;
> +
> +	gbl->fbdev = NULL;
> +	gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags);
> +
> +	gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;

I would move this line after the error check below.

> +
> +	if (gbl->gpio == -EPROBE_DEFER) {
> +		return ERR_PTR(-EPROBE_DEFER);

Any reason not to retrun -EPROBE_DEFER directly ?

> +	} else if (gbl->gpio < 0) {
> +		dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
> +		return gbl->gpio;
> +	}

Maybe you would do something like

	if (gbl->gpio < 0) {
		if (gbl->gpio == -EPROBE_DEFER)
			dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
		return gbl->gpio;
	}

> +
> +	ret = of_property_read_u32(np, "default-brightness-level",
> +				   &gbl->def_value);
> +	if (ret < 0) {
> +		/* The property is optional. */
> +		gbl->def_value = 1;
> +	}
> +
> +	if (gbl->def_value > 1) {
> +		dev_warn(&pdev->dev,
> +			"Warning: Invalid default-brightness-level value. Its value can 
be
> either 0(off) or 1(on).\n");

I believe a less verbose message (without the second sentence) would have 
done, but that's up to you.

> +		gbl->def_value = 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gpio_backlight_probe(struct platform_device *pdev)
>  {
>  	struct gpio_backlight_platform_data *pdata =
> @@ -67,10 +105,12 @@ static int gpio_backlight_probe(struct platform_device
> *pdev) struct backlight_properties props;
>  	struct backlight_device *bl;
>  	struct gpio_backlight *gbl;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;
> 
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "failed to find platform data\n");
> +	if (!pdata && !np) {
> +		dev_err(&pdev->dev,
> +			"failed to find platform data or device tree node.\n");
>  		return -ENODEV;
>  	}
> 
> @@ -79,14 +119,22 @@ static int gpio_backlight_probe(struct platform_device
> *pdev) return -ENOMEM;
> 
>  	gbl->dev = &pdev->dev;
> -	gbl->fbdev = pdata->fbdev;
> -	gbl->gpio = pdata->gpio;
> -	gbl->active = pdata->active_low ? 0 : 1;
> +
> +	if (np) {
> +		ret = gpio_backlight_probe_dt(pdev, gbl);
> +		if (ret)
> +			return ret;
> +	} else {
> +		gbl->fbdev = pdata->fbdev;
> +		gbl->gpio = pdata->gpio;
> +		gbl->active = pdata->active_low ? 0 : 1;
> +		gbl->def_value = pdata->def_value;
> +	}
> 
>  	ret = devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT |
>  				    (gbl->active ? GPIOF_INIT_LOW
> 
>  						 : GPIOF_INIT_HIGH),
> 
> -				    pdata->name);
> +				    pdata ? pdata->name : "backlight");
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "unable to request GPIO\n");
>  		return ret;
> @@ -103,17 +151,24 @@ static int gpio_backlight_probe(struct platform_device
> *pdev) return PTR_ERR(bl);
>  	}
> 
> -	bl->props.brightness = pdata->def_value;
> +	bl->props.brightness = gbl->def_value;
> +
>  	backlight_update_status(bl);
> 
>  	platform_set_drvdata(pdev, bl);
>  	return 0;
>  }
> 
> +static struct of_device_id gpio_backlight_of_match[] = {
> +	{ .compatible = "gpio-backlight" },
> +	{ /* sentinel */ }
> +};
> +
>  static struct platform_driver gpio_backlight_driver = {
>  	.driver		= {
>  		.name		= "gpio-backlight",
>  		.owner		= THIS_MODULE,
> +		.of_match_table = of_match_ptr(gpio_backlight_of_match),
>  	},
>  	.probe		= gpio_backlight_probe,
>  };
-- 
Regards,

Laurent Pinchart

--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: devicetree@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v3->v4:
> - The default-brightness property is now optional, it defaults to 1 if not set.
by default we set OFF not ON

do not actiate driver or properti by default you can not known to consequence
on the hw

Best Regards,
J.
> - def_value int becomes an u32.
> - gbl->def_value was set to pdata->def_value in pdata mode to avoid an extra
>   check.
> ---
>  .../bindings/video/backlight/gpio-backlight.txt    |   20 ++++++
>  drivers/video/backlight/gpio_backlight.c           |   69 ++++++++++++++++++--
>  2 files changed, 82 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> new file mode 100644
> index 0000000..3474d4a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> @@ -0,0 +1,20 @@
> +gpio-backlight bindings
> +
> +Required properties:
> +  - compatible: "gpio-backlight"
> +  - gpios: describes the gpio that is used for enabling/disabling the backlight
> +     (see GPIO binding[0] for more details).
> +
> +Optional properties:
> +  - default-brightness-level: the default brightness level (can be 0(off) or
> +      1(on) since GPIOs only support theses levels).
> +
> +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +
> +	backlight {
> +		compatible = "gpio-backlight";
> +		gpios = <&gpio3 4 0>;
> +		default-brightness-level = <0>;
> +	};
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index 81fb127..248124d 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -13,6 +13,8 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_data/gpio_backlight.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -23,6 +25,7 @@ struct gpio_backlight {
>  
>  	int gpio;
>  	int active;
> +	u32 def_value;
>  };
>  
>  static int gpio_backlight_update_status(struct backlight_device *bl)
> @@ -60,6 +63,41 @@ static const struct backlight_ops gpio_backlight_ops = {
>  	.check_fb	= gpio_backlight_check_fb,
>  };
>  
> +static int gpio_backlight_probe_dt(struct platform_device *pdev,
> +				   struct gpio_backlight *gbl)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	enum of_gpio_flags gpio_flags;
> +	int ret;
> +
> +	gbl->fbdev = NULL;
> +	gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags);
> +
> +	gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> +
> +	if (gbl->gpio == -EPROBE_DEFER) {
> +		return ERR_PTR(-EPROBE_DEFER);
> +	} else if (gbl->gpio < 0) {
> +		dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
> +		return gbl->gpio;
> +	}
> +
> +	ret = of_property_read_u32(np, "default-brightness-level",
> +				   &gbl->def_value);
> +	if (ret < 0) {
> +		/* The property is optional. */
> +		gbl->def_value = 1;
> +	}
> +
> +	if (gbl->def_value > 1) {
> +		dev_warn(&pdev->dev,
> +			"Warning: Invalid default-brightness-level value. Its value can be either 0(off) or 1(on).\n");
> +		gbl->def_value = 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gpio_backlight_probe(struct platform_device *pdev)
>  {
>  	struct gpio_backlight_platform_data *pdata =
> @@ -67,10 +105,12 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  	struct backlight_properties props;
>  	struct backlight_device *bl;
>  	struct gpio_backlight *gbl;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "failed to find platform data\n");
> +	if (!pdata && !np) {
> +		dev_err(&pdev->dev,
> +			"failed to find platform data or device tree node.\n");
>  		return -ENODEV;
>  	}
>  
> @@ -79,14 +119,22 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	gbl->dev = &pdev->dev;
> -	gbl->fbdev = pdata->fbdev;
> -	gbl->gpio = pdata->gpio;
> -	gbl->active = pdata->active_low ? 0 : 1;
> +
> +	if (np) {
> +		ret = gpio_backlight_probe_dt(pdev, gbl);
> +		if (ret)
> +			return ret;
> +	} else {
> +		gbl->fbdev = pdata->fbdev;
> +		gbl->gpio = pdata->gpio;
> +		gbl->active = pdata->active_low ? 0 : 1;
> +		gbl->def_value = pdata->def_value;
> +	}
>  
>  	ret = devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT |
>  				    (gbl->active ? GPIOF_INIT_LOW
>  						 : GPIOF_INIT_HIGH),
> -				    pdata->name);
> +				    pdata ? pdata->name : "backlight");
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "unable to request GPIO\n");
>  		return ret;
> @@ -103,17 +151,24 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  		return PTR_ERR(bl);
>  	}
>  
> -	bl->props.brightness = pdata->def_value;
> +	bl->props.brightness = gbl->def_value;
> +
>  	backlight_update_status(bl);
>  
>  	platform_set_drvdata(pdev, bl);
>  	return 0;
>  }
>  
> +static struct of_device_id gpio_backlight_of_match[] = {
> +	{ .compatible = "gpio-backlight" },
> +	{ /* sentinel */ }
> +};
> +
>  static struct platform_driver gpio_backlight_driver = {
>  	.driver		= {
>  		.name		= "gpio-backlight",
>  		.owner		= THIS_MODULE,
> +		.of_match_table = of_match_ptr(gpio_backlight_of_match),
>  	},
>  	.probe		= gpio_backlight_probe,
>  };
> -- 
> 1.7.9.5
> 

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jean-Christophe PLAGNIOL-VILLARD]

On 00:48 Tue 22 Oct     , Laurent Pinchart wrote:
> Hi Denis,
> 
> Thanks for the patch. Please see below for a couple of small comment.
> 
> On Monday 21 October 2013 11:13:33 Denis Carikli wrote:
> > Cc: Richard Purdie <rpurdie@rpsys.net>
> > Cc: Jingoo Han <jg1.han@samsung.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: devicetree@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Lothar Waßmann <LW@KARO-electronics.de>
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > ---
> > ChangeLog v3->v4:
> > - The default-brightness property is now optional, it defaults to 1 if not
> > set. - def_value int becomes an u32.
> > - gbl->def_value was set to pdata->def_value in pdata mode to avoid an extra
> > check.
> > ---
> >  .../bindings/video/backlight/gpio-backlight.txt    |   20 ++++++
> >  drivers/video/backlight/gpio_backlight.c           |   69 +++++++++++++++--
> >  2 files changed, 82 insertions(+), 7 deletions(-)
> >  create mode 100644
> > Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> > b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt new
> > file mode 100644
> > index 0000000..3474d4a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> > @@ -0,0 +1,20 @@
> > +gpio-backlight bindings
> > +
> > +Required properties:
> > +  - compatible: "gpio-backlight"
> > +  - gpios: describes the gpio that is used for enabling/disabling the
> > backlight
> > +     (see GPIO binding[0] for more details).
> > +
> > +Optional properties:
> > +  - default-brightness-level: the default brightness level (can be 0(off)
> > or
> > +      1(on) since GPIOs only support theses levels).
> 
> I believe you should specify what the default value is when the property isn't 
> available.
> 
> > +
> > +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
> > +
> > +Example:
> > +
> > +	backlight {
> > +		compatible = "gpio-backlight";
> > +		gpios = <&gpio3 4 0>;
> > +		default-brightness-level = <0>;
> > +	};
> > diff --git a/drivers/video/backlight/gpio_backlight.c
> > b/drivers/video/backlight/gpio_backlight.c index 81fb127..248124d 100644
> > --- a/drivers/video/backlight/gpio_backlight.c
> > +++ b/drivers/video/backlight/gpio_backlight.c
> > @@ -13,6 +13,8 @@
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/platform_data/gpio_backlight.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > @@ -23,6 +25,7 @@ struct gpio_backlight {
> > 
> >  	int gpio;
> >  	int active;
> > +	u32 def_value;
> >  };
> > 
> >  static int gpio_backlight_update_status(struct backlight_device *bl)
> > @@ -60,6 +63,41 @@ static const struct backlight_ops gpio_backlight_ops = {
> >  	.check_fb	= gpio_backlight_check_fb,
> >  };
> > 
> > +static int gpio_backlight_probe_dt(struct platform_device *pdev,
> > +				   struct gpio_backlight *gbl)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	enum of_gpio_flags gpio_flags;
> > +	int ret;
> > +
> > +	gbl->fbdev = NULL;
> > +	gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags);
> > +
> > +	gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> 
> I would move this line after the error check below.
> 
> > +
> > +	if (gbl->gpio == -EPROBE_DEFER) {
> > +		return ERR_PTR(-EPROBE_DEFER);
> 
> Any reason not to retrun -EPROBE_DEFER directly ?
> 
> > +	} else if (gbl->gpio < 0) {
> > +		dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
> > +		return gbl->gpio;
> > +	}
> 
> Maybe you would do something like
> 
> 	if (gbl->gpio < 0) {
> 		if (gbl->gpio == -EPROBE_DEFER)
> 			dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
it's not an error it's that the gpio driver is not yet probed

so it's need to be !=
> 		return gbl->gpio;
> 	}
> 
> > +
> > +	ret = of_property_read_u32(np, "default-brightness-level",
> > +				   &gbl->def_value);
> > +	if (ret < 0) {
> > +		/* The property is optional. */
> > +		gbl->def_value = 1;
> > +	}
> > +
> > +	if (gbl->def_value > 1) {
> > +		dev_warn(&pdev->dev,
> > +			"Warning: Invalid default-brightness-level value. Its value can 
> be
> > either 0(off) or 1(on).\n");
> 
> I believe a less verbose message (without the second sentence) would have 
> done, but that's up to you.

as it's only 0 or 1 why not just use a boolean
> 
> > +		gbl->def_value = 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int gpio_backlight_probe(struct platform_device *pdev)
> >  {
> >  	struct gpio_backlight_platform_data *pdata =
> > @@ -67,10 +105,12 @@ static int gpio_backlight_probe(struct platform_device
> > *pdev) struct backlight_properties props;
> >  	struct backlight_device *bl;
> >  	struct gpio_backlight *gbl;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	int ret;
> > 
> > -	if (!pdata) {
> > -		dev_err(&pdev->dev, "failed to find platform data\n");
> > +	if (!pdata && !np) {
> > +		dev_err(&pdev->dev,
> > +			"failed to find platform data or device tree node.\n");
> >  		return -ENODEV;
> >  	}
> > 
> > @@ -79,14 +119,22 @@ static int gpio_backlight_probe(struct platform_device
> > *pdev) return -ENOMEM;
> > 
> >  	gbl->dev = &pdev->dev;
> > -	gbl->fbdev = pdata->fbdev;
> > -	gbl->gpio = pdata->gpio;
> > -	gbl->active = pdata->active_low ? 0 : 1;
> > +
> > +	if (np) {
> > +		ret = gpio_backlight_probe_dt(pdev, gbl);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		gbl->fbdev = pdata->fbdev;
> > +		gbl->gpio = pdata->gpio;
> > +		gbl->active = pdata->active_low ? 0 : 1;
> > +		gbl->def_value = pdata->def_value;
> > +	}
> > 
> >  	ret = devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT |
> >  				    (gbl->active ? GPIOF_INIT_LOW
> > 
> >  						 : GPIOF_INIT_HIGH),
> > 
> > -				    pdata->name);
> > +				    pdata ? pdata->name : "backlight");
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "unable to request GPIO\n");
> >  		return ret;
> > @@ -103,17 +151,24 @@ static int gpio_backlight_probe(struct platform_device
> > *pdev) return PTR_ERR(bl);
> >  	}
> > 
> > -	bl->props.brightness = pdata->def_value;
> > +	bl->props.brightness = gbl->def_value;
> > +
> >  	backlight_update_status(bl);
> > 
> >  	platform_set_drvdata(pdev, bl);
> >  	return 0;
> >  }
> > 
> > +static struct of_device_id gpio_backlight_of_match[] = {
> > +	{ .compatible = "gpio-backlight" },
> > +	{ /* sentinel */ }
> > +};
> > +
> >  static struct platform_driver gpio_backlight_driver = {
> >  	.driver		= {
> >  		.name		= "gpio-backlight",
> >  		.owner		= THIS_MODULE,
> > +		.of_match_table = of_match_ptr(gpio_backlight_of_match),
> >  	},
> >  	.probe		= gpio_backlight_probe,
> >  };
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > ---
> > ChangeLog v3->v4:
> > - The default-brightness property is now optional, it defaults to 1 if not set.
> by default we set OFF not ON
> 
> do not actiate driver or properti by default you can not known to consequence
> on the hw

Turning on a backlight by default is what pretty much every backlight
driver does. I personally think that's the wrong default, I even tried
to get some discussion started recently about how we could change this.
However, given that this has been the case for possibly as long as the
subsystem has existed, suddenly changing it might cause quite a few of
our users to boot the new kernel and not see their display come up. As
with any other ABI, this isn't something we can just change without a
very good migration path.

In my opinion every backlight should be hooked up to a display panel,
and the display panel driver should be the one responsible for turning
the backlight on or off. That's the only way we can guarantee that the
backlight is turned on at the right moment so that glitches can be
avoided.

One possible migration path would be to update all display panel drivers
to cope with an associated backlight device, but even if somebody would
even find the time to write the code to do that, I can imagine that we'd
have a hard time getting this tested since a lot of the boards that rely
on these backlight drivers are legacy and probably no longer very
actively used.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > Cc: Richard Purdie <rpurdie@rpsys.net>
> > > Cc: Jingoo Han <jg1.han@samsung.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Rob Herring <rob.herring@calxeda.com>
> > > Cc: Pawel Moll <pawel.moll@arm.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > > Cc: devicetree@vger.kernel.org
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: Lothar Waßmann <LW@KARO-electronics.de>
> > > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > > Cc: Eric Bénard <eric@eukrea.com>
> > > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > > ---
> > > ChangeLog v3->v4:
> > > - The default-brightness property is now optional, it defaults to 1 if not set.
> > by default we set OFF not ON
> > 
> > do not actiate driver or properti by default you can not known to consequence
> > on the hw
> 
> Turning on a backlight by default is what pretty much every backlight
> driver does. I personally think that's the wrong default, I even tried
> to get some discussion started recently about how we could change this.
> However, given that this has been the case for possibly as long as the
> subsystem has existed, suddenly changing it might cause quite a few of
> our users to boot the new kernel and not see their display come up. As
> with any other ABI, this isn't something we can just change without a
> very good migration path.

I'm sorry but the blacklight descibe in DT have nothing to do with the common
pratice that the current driver have today

put on by default if wrong specially without the property define. Even put it
on by default it wrong as the bootloader may have set it already for splash
screen and to avoid glitch the drivers need to detect this.

For me this should not even be a property but handled by the driver them
selves in C.

Best Regards,
J.
> 
> In my opinion every backlight should be hooked up to a display panel,
> and the display panel driver should be the one responsible for turning
> the backlight on or off. That's the only way we can guarantee that the
> backlight is turned on at the right moment so that glitches can be
> avoided.
> 
> One possible migration path would be to update all display panel drivers
> to cope with an associated backlight device, but even if somebody would
> even find the time to write the code to do that, I can imagine that we'd
> have a hard time getting this tested since a lot of the boards that rely
> on these backlight drivers are legacy and probably no longer very
> actively used.
> 
> Thierry

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3655 bytes --]

On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> > On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > > > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > > > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > > ChangeLog v3->v4:
> > > > - The default-brightness property is now optional, it defaults to 1 if not set.
> > > by default we set OFF not ON
> > > 
> > > do not actiate driver or properti by default you can not known to consequence
> > > on the hw
> > 
> > Turning on a backlight by default is what pretty much every backlight
> > driver does. I personally think that's the wrong default, I even tried
> > to get some discussion started recently about how we could change this.
> > However, given that this has been the case for possibly as long as the
> > subsystem has existed, suddenly changing it might cause quite a few of
> > our users to boot the new kernel and not see their display come up. As
> > with any other ABI, this isn't something we can just change without a
> > very good migration path.
> 
> I'm sorry but the blacklight descibe in DT have nothing to do with the common
> pratice that the current driver have today

That's not at all what I said. What I said was that the majority of
backlight drivers currently default to turning the backlight on when
probed. Therefore I think it would be consistent if this driver did the
same.

I also said that I don't think it's a very good default, but at the same
time we can't just go and change the default behaviour at will because
people may rely on it.

> put on by default if wrong specially without the property define. Even put it
> on by default it wrong as the bootloader may have set it already for splash
> screen and to avoid glitch the drivers need to detect this.

I agree that would be preferable, but I don't know of any way to detect
what value the bootloader set a GPIO to. The GPIO API requires that you
call gpio_direction_output(), and that requires a value parameter which
will be used as the output level of the GPIO.

> For me this should not even be a property but handled by the driver them
> selves in C.

Agreed. There has been some discussion recently about whether devicetree
should be extended (or supplemented) to allow defining behaviour as well
(in addition to just hardware). But that's not immediately relevant here
at this time.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:01 Tue 22 Oct     , Thierry Reding wrote:
> On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> > > On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > > > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > > > > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > > > > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > ---
> > > > > ChangeLog v3->v4:
> > > > > - The default-brightness property is now optional, it defaults to 1 if not set.
> > > > by default we set OFF not ON
> > > > 
> > > > do not actiate driver or properti by default you can not known to consequence
> > > > on the hw
> > > 
> > > Turning on a backlight by default is what pretty much every backlight
> > > driver does. I personally think that's the wrong default, I even tried
> > > to get some discussion started recently about how we could change this.
> > > However, given that this has been the case for possibly as long as the
> > > subsystem has existed, suddenly changing it might cause quite a few of
> > > our users to boot the new kernel and not see their display come up. As
> > > with any other ABI, this isn't something we can just change without a
> > > very good migration path.
> > 
> > I'm sorry but the blacklight descibe in DT have nothing to do with the common
> > pratice that the current driver have today
> 
> That's not at all what I said. What I said was that the majority of
> backlight drivers currently default to turning the backlight on when
> probed. Therefore I think it would be consistent if this driver did the
> same.

This is a matter of C code not DT
> 
> I also said that I don't think it's a very good default, but at the same
> time we can't just go and change the default behaviour at will because
> people may rely on it.

so handle it in C not in DT

For me the only value a default-brightness would be with a pwn or a ragned
value but not on 0-1 for on vs off
> 
> > put on by default if wrong specially without the property define. Even put it
> > on by default it wrong as the bootloader may have set it already for splash
> > screen and to avoid glitch the drivers need to detect this.
> 
> I agree that would be preferable, but I don't know of any way to detect
> what value the bootloader set a GPIO to. The GPIO API requires that you
> call gpio_direction_output(), and that requires a value parameter which
> will be used as the output level of the GPIO.

For me gpio_get_value can return the value of a an output gpio but this need
to be checked with LinusW
> 
> > For me this should not even be a property but handled by the driver them
> > selves in C.
> 
> Agreed. There has been some discussion recently about whether devicetree
> should be extended (or supplemented) to allow defining behaviour as well
> (in addition to just hardware). But that's not immediately relevant here
> at this time.

yes

to conclued NACK on the default-brightness here


Best Regards,
J.
> 
> Thierry


--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Stephen Warren]

On 10/23/2013 02:42 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
...
> For me gpio_get_value can return the value of a an output gpio but this need
> to be checked with LinusW

That's certainly not true for all possible GPIO controllers; there are
at least some that can't read either:

* The value of the physical wire, if the GPIO is configured as an output.

* The value that the GPIO controller is driving as an output.
--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Stephen Warren]

On 10/22/2013 09:01 PM, Thierry Reding wrote:
> On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> PLAGNIOL-VILLARD wrote:
...
>> I'm sorry but the blacklight descibe in DT have nothing to do
>> with the common pratice that the current driver have today
> 
> That's not at all what I said. What I said was that the majority
> of backlight drivers currently default to turning the backlight on
> when probed. Therefore I think it would be consistent if this
> driver did the same.
> 
> I also said that I don't think it's a very good default, but at the
> same time we can't just go and change the default behaviour at will
> because people may rely on it.

It may well be reasonable to change the default behaviour for devices
instantiated from DT. If it's not possible to instantiate the device
from DT yet, then it's not possible for anyone to be relying on the
default behaviour yet, since there is none. So, perhaps the default
could be:

* If device instantiated from a board file, default to on, for
backwards-compatibility.

* If device instantiated from DT, there is no backwards compatibility
to be concerned with, since this is a new feature, hence default to
off, since we think that's the correct thing to do.
--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Wed, Oct 23, 2013 at 05:49:06PM +0100, Stephen Warren wrote:
> On 10/23/2013 02:42 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> ...
> > For me gpio_get_value can return the value of a an output gpio but this need
> > to be checked with LinusW
> 
> That's certainly not true for all possible GPIO controllers; there are
> at least some that can't read either:
> 
> * The value of the physical wire, if the GPIO is configured as an output.
> 
> * The value that the GPIO controller is driving as an output.

My point originally was that since it's an output pin, you need to
configure it as an output before you can use it. The way to do that in
Linux is to call gpio_direction_output(). But that will automatically
also force the output value to whatever you specify as the second
parameter.

I suppose that could be remedied by adding a separate function that
doesn't set the value, but as Stephen points out, reading the value of
an output pin may not be supported on all hardware.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

On Wed, Oct 23, 2013 at 05:51:13PM +0100, Stephen Warren wrote:
> On 10/22/2013 09:01 PM, Thierry Reding wrote:
> > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> > PLAGNIOL-VILLARD wrote:
> ...
> >> I'm sorry but the blacklight descibe in DT have nothing to do
> >> with the common pratice that the current driver have today
> > 
> > That's not at all what I said. What I said was that the majority
> > of backlight drivers currently default to turning the backlight on
> > when probed. Therefore I think it would be consistent if this
> > driver did the same.
> > 
> > I also said that I don't think it's a very good default, but at the
> > same time we can't just go and change the default behaviour at will
> > because people may rely on it.
> 
> It may well be reasonable to change the default behaviour for devices
> instantiated from DT. If it's not possible to instantiate the device
> from DT yet, then it's not possible for anyone to be relying on the
> default behaviour yet, since there is none. So, perhaps the default
> could be:
> 
> * If device instantiated from a board file, default to on, for
> backwards-compatibility.
> 
> * If device instantiated from DT, there is no backwards compatibility
> to be concerned with, since this is a new feature, hence default to
> off, since we think that's the correct thing to do.

I actually had a patch to do precisely that. However I then realized
that people have actually been using pwm-backlight in DT for a while
already and therefore may be relying on that behaviour as well.

It also isn't really an issue of DT vs. non-DT. The simple fact is that
besides the backlight driver there's usually no other code that enables
a backlight on boot. The only way to do so that I know of is using the
DRM panel patches that I've been working on.

That said, it is true that the number of DT users of the pwm-backlight
driver is smaller than the number of board file users, and it is much
more likely that people are still actively using them, so if we can get
everyone to agree on changing the default behaviour that might still be
possible.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

Hi Thierry,

On Wednesday 23 October 2013 22:20:12 Thierry Reding wrote:
> On Wed, Oct 23, 2013 at 05:51:13PM +0100, Stephen Warren wrote:
> > On 10/22/2013 09:01 PM, Thierry Reding wrote:
> > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> > 
> > > PLAGNIOL-VILLARD wrote:
> > ...
> > 
> > >> I'm sorry but the blacklight descibe in DT have nothing to do
> > >> with the common pratice that the current driver have today
> > > 
> > > That's not at all what I said. What I said was that the majority
> > > of backlight drivers currently default to turning the backlight on
> > > when probed. Therefore I think it would be consistent if this
> > > driver did the same.
> > > 
> > > I also said that I don't think it's a very good default, but at the
> > > same time we can't just go and change the default behaviour at will
> > > because people may rely on it.
> > 
> > It may well be reasonable to change the default behaviour for devices
> > instantiated from DT. If it's not possible to instantiate the device
> > from DT yet, then it's not possible for anyone to be relying on the
> > default behaviour yet, since there is none. So, perhaps the default
> > could be:
> > 
> > * If device instantiated from a board file, default to on, for
> > backwards-compatibility.
> > 
> > * If device instantiated from DT, there is no backwards compatibility
> > to be concerned with, since this is a new feature, hence default to
> > off, since we think that's the correct thing to do.
> 
> I actually had a patch to do precisely that. However I then realized
> that people have actually been using pwm-backlight in DT for a while
> already and therefore may be relying on that behaviour as well.
> 
> It also isn't really an issue of DT vs. non-DT. The simple fact is that
> besides the backlight driver there's usually no other code that enables
> a backlight on boot. The only way to do so that I know of is using the
> DRM panel patches that I've been working on.

I would very much welcome a refactoring of the backlight code that would 
remove the fbdev dependency and hook backlights to panel drivers. That's 
something I wanted to work on myself, but that I pushed back after CDF :-)

> That said, it is true that the number of DT users of the pwm-backlight
> driver is smaller than the number of board file users, and it is much
> more likely that people are still actively using them, so if we can get
> everyone to agree on changing the default behaviour that might still be
> possible.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

On Thu, Oct 24, 2013 at 12:38:59AM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Wednesday 23 October 2013 22:20:12 Thierry Reding wrote:
> > On Wed, Oct 23, 2013 at 05:51:13PM +0100, Stephen Warren wrote:
> > > On 10/22/2013 09:01 PM, Thierry Reding wrote:
> > > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> > > 
> > > > PLAGNIOL-VILLARD wrote:
> > > ...
> > > 
> > > >> I'm sorry but the blacklight descibe in DT have nothing to do
> > > >> with the common pratice that the current driver have today
> > > > 
> > > > That's not at all what I said. What I said was that the majority
> > > > of backlight drivers currently default to turning the backlight on
> > > > when probed. Therefore I think it would be consistent if this
> > > > driver did the same.
> > > > 
> > > > I also said that I don't think it's a very good default, but at the
> > > > same time we can't just go and change the default behaviour at will
> > > > because people may rely on it.
> > > 
> > > It may well be reasonable to change the default behaviour for devices
> > > instantiated from DT. If it's not possible to instantiate the device
> > > from DT yet, then it's not possible for anyone to be relying on the
> > > default behaviour yet, since there is none. So, perhaps the default
> > > could be:
> > > 
> > > * If device instantiated from a board file, default to on, for
> > > backwards-compatibility.
> > > 
> > > * If device instantiated from DT, there is no backwards compatibility
> > > to be concerned with, since this is a new feature, hence default to
> > > off, since we think that's the correct thing to do.
> > 
> > I actually had a patch to do precisely that. However I then realized
> > that people have actually been using pwm-backlight in DT for a while
> > already and therefore may be relying on that behaviour as well.
> > 
> > It also isn't really an issue of DT vs. non-DT. The simple fact is that
> > besides the backlight driver there's usually no other code that enables
> > a backlight on boot. The only way to do so that I know of is using the
> > DRM panel patches that I've been working on.
> 
> I would very much welcome a refactoring of the backlight code that would 
> remove the fbdev dependency and hook backlights to panel drivers. That's 
> something I wanted to work on myself, but that I pushed back after CDF :-)

Yeah, that would certainly be very welcome. But it's also a pretty
daunting job since there are a whole lot of devices out there that
aren't easy to test because, well, they're pretty old and chances
are that nobody that actually has one is around.

But I guess that we can always try to solve it on a best effort basis,
though. Perhaps things can even be done in a backwards-compatible way.
I'm thinking for instance that we could introduce a new property, say
.enable, but keep any of the others suhc as state, power, fb_blank for
backwards-compatibility. Perhaps even emulate them for a while until
we've gone and cleaned up all users.

Or is there still a reason to have more than a single bit for backlight
state? I don't know any hardware that actually makes a difference
between FB_BLANK_NORMAL, FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND
or FB_BLANK_POWERDOWN. Many drivers in drivers/video seem to be using
those, but for backlights I can see no use other than FB_BLANK_UNBLANK
meaning enabled and anything else meaning disabled.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 4002 bytes --]

Hi Thierry,

On Thursday 24 October 2013 13:05:25 Thierry Reding wrote:
> On Thu, Oct 24, 2013 at 12:38:59AM +0200, Laurent Pinchart wrote:
> > On Wednesday 23 October 2013 22:20:12 Thierry Reding wrote:
> > > On Wed, Oct 23, 2013 at 05:51:13PM +0100, Stephen Warren wrote:
> > > > On 10/22/2013 09:01 PM, Thierry Reding wrote:
> > > > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> > > > 
> > > > > PLAGNIOL-VILLARD wrote:
> > > > ...
> > > > 
> > > > >> I'm sorry but the blacklight descibe in DT have nothing to do
> > > > >> with the common pratice that the current driver have today
> > > > > 
> > > > > That's not at all what I said. What I said was that the majority
> > > > > of backlight drivers currently default to turning the backlight on
> > > > > when probed. Therefore I think it would be consistent if this
> > > > > driver did the same.
> > > > > 
> > > > > I also said that I don't think it's a very good default, but at the
> > > > > same time we can't just go and change the default behaviour at will
> > > > > because people may rely on it.
> > > > 
> > > > It may well be reasonable to change the default behaviour for devices
> > > > instantiated from DT. If it's not possible to instantiate the device
> > > > from DT yet, then it's not possible for anyone to be relying on the
> > > > default behaviour yet, since there is none. So, perhaps the default
> > > > could be:
> > > > 
> > > > * If device instantiated from a board file, default to on, for
> > > > backwards-compatibility.
> > > > 
> > > > * If device instantiated from DT, there is no backwards compatibility
> > > > to be concerned with, since this is a new feature, hence default to
> > > > off, since we think that's the correct thing to do.
> > > 
> > > I actually had a patch to do precisely that. However I then realized
> > > that people have actually been using pwm-backlight in DT for a while
> > > already and therefore may be relying on that behaviour as well.
> > > 
> > > It also isn't really an issue of DT vs. non-DT. The simple fact is that
> > > besides the backlight driver there's usually no other code that enables
> > > a backlight on boot. The only way to do so that I know of is using the
> > > DRM panel patches that I've been working on.
> > 
> > I would very much welcome a refactoring of the backlight code that would
> > remove the fbdev dependency and hook backlights to panel drivers. That's
> > something I wanted to work on myself, but that I pushed back after CDF :-)
> 
> Yeah, that would certainly be very welcome. But it's also a pretty
> daunting job since there are a whole lot of devices out there that
> aren't easy to test because, well, they're pretty old and chances
> are that nobody that actually has one is around.
>
> But I guess that we can always try to solve it on a best effort basis,
> though. Perhaps things can even be done in a backwards-compatible way.
> I'm thinking for instance that we could introduce a new property, say
> .enable, but keep any of the others suhc as state, power, fb_blank for
> backwards-compatibility. Perhaps even emulate them for a while until
> we've gone and cleaned up all users.

That would work with me. I don't think we need more than a best effort 
approach to porting existing backlight drivers to the new model. When it comes 
to compatibility with the current interface, I'd like to move the 
compatibility code to the core instead of leaving it in the individual drivers 
if possible.

> Or is there still a reason to have more than a single bit for backlight
> state? I don't know any hardware that actually makes a difference
> between FB_BLANK_NORMAL, FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND
> or FB_BLANK_POWERDOWN. Many drivers in drivers/video seem to be using
> those, but for backlights I can see no use other than FB_BLANK_UNBLANK
> meaning enabled and anything else meaning disabled.

I see no use case for the fine-grained state in backlights at the moment.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Grant Likely]

[-- Attachment #1: Type: text/plain, Size: 6439 bytes --]

On Mon, 21 Oct 2013 11:13:33 +0200, Denis Carikli <denis@eukrea.com> wrote:
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: devicetree@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v3->v4:
> - The default-brightness property is now optional, it defaults to 1 if not set.
> - def_value int becomes an u32.
> - gbl->def_value was set to pdata->def_value in pdata mode to avoid an extra
>   check.
> ---
>  .../bindings/video/backlight/gpio-backlight.txt    |   20 ++++++
>  drivers/video/backlight/gpio_backlight.c           |   69 ++++++++++++++++++--
>  2 files changed, 82 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> new file mode 100644
> index 0000000..3474d4a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> @@ -0,0 +1,20 @@
> +gpio-backlight bindings
> +
> +Required properties:
> +  - compatible: "gpio-backlight"
> +  - gpios: describes the gpio that is used for enabling/disabling the backlight
> +     (see GPIO binding[0] for more details).
> +
> +Optional properties:
> +  - default-brightness-level: the default brightness level (can be 0(off) or
> +      1(on) since GPIOs only support theses levels).

Seems odd to call it brightness since it can only be on or off. Why not
call it "default-state" or and empty bool named "default-on"?

Otherwise looks okay to me.

g.

> +
> +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +
> +	backlight {
> +		compatible = "gpio-backlight";
> +		gpios = <&gpio3 4 0>;
> +		default-brightness-level = <0>;
> +	};
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
> index 81fb127..248124d 100644
> --- a/drivers/video/backlight/gpio_backlight.c
> +++ b/drivers/video/backlight/gpio_backlight.c
> @@ -13,6 +13,8 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_data/gpio_backlight.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -23,6 +25,7 @@ struct gpio_backlight {
>  
>  	int gpio;
>  	int active;
> +	u32 def_value;
>  };
>  
>  static int gpio_backlight_update_status(struct backlight_device *bl)
> @@ -60,6 +63,41 @@ static const struct backlight_ops gpio_backlight_ops = {
>  	.check_fb	= gpio_backlight_check_fb,
>  };
>  
> +static int gpio_backlight_probe_dt(struct platform_device *pdev,
> +				   struct gpio_backlight *gbl)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	enum of_gpio_flags gpio_flags;
> +	int ret;
> +
> +	gbl->fbdev = NULL;
> +	gbl->gpio = of_get_gpio_flags(np, 0, &gpio_flags);
> +
> +	gbl->active = (gpio_flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
> +
> +	if (gbl->gpio == -EPROBE_DEFER) {
> +		return ERR_PTR(-EPROBE_DEFER);
> +	} else if (gbl->gpio < 0) {
> +		dev_err(&pdev->dev, "Error: gpios is a required parameter.\n");
> +		return gbl->gpio;
> +	}
> +
> +	ret = of_property_read_u32(np, "default-brightness-level",
> +				   &gbl->def_value);
> +	if (ret < 0) {
> +		/* The property is optional. */
> +		gbl->def_value = 1;
> +	}
> +
> +	if (gbl->def_value > 1) {
> +		dev_warn(&pdev->dev,
> +			"Warning: Invalid default-brightness-level value. Its value can be either 0(off) or 1(on).\n");
> +		gbl->def_value = 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gpio_backlight_probe(struct platform_device *pdev)
>  {
>  	struct gpio_backlight_platform_data *pdata =
> @@ -67,10 +105,12 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  	struct backlight_properties props;
>  	struct backlight_device *bl;
>  	struct gpio_backlight *gbl;
> +	struct device_node *np = pdev->dev.of_node;
>  	int ret;
>  
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "failed to find platform data\n");
> +	if (!pdata && !np) {
> +		dev_err(&pdev->dev,
> +			"failed to find platform data or device tree node.\n");
>  		return -ENODEV;
>  	}
>  
> @@ -79,14 +119,22 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	gbl->dev = &pdev->dev;
> -	gbl->fbdev = pdata->fbdev;
> -	gbl->gpio = pdata->gpio;
> -	gbl->active = pdata->active_low ? 0 : 1;
> +
> +	if (np) {
> +		ret = gpio_backlight_probe_dt(pdev, gbl);
> +		if (ret)
> +			return ret;
> +	} else {
> +		gbl->fbdev = pdata->fbdev;
> +		gbl->gpio = pdata->gpio;
> +		gbl->active = pdata->active_low ? 0 : 1;
> +		gbl->def_value = pdata->def_value;
> +	}
>  
>  	ret = devm_gpio_request_one(gbl->dev, gbl->gpio, GPIOF_DIR_OUT |
>  				    (gbl->active ? GPIOF_INIT_LOW
>  						 : GPIOF_INIT_HIGH),
> -				    pdata->name);
> +				    pdata ? pdata->name : "backlight");
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "unable to request GPIO\n");
>  		return ret;
> @@ -103,17 +151,24 @@ static int gpio_backlight_probe(struct platform_device *pdev)
>  		return PTR_ERR(bl);
>  	}
>  
> -	bl->props.brightness = pdata->def_value;
> +	bl->props.brightness = gbl->def_value;
> +
>  	backlight_update_status(bl);
>  
>  	platform_set_drvdata(pdev, bl);
>  	return 0;
>  }
>  
> +static struct of_device_id gpio_backlight_of_match[] = {
> +	{ .compatible = "gpio-backlight" },
> +	{ /* sentinel */ }
> +};
> +
>  static struct platform_driver gpio_backlight_driver = {
>  	.driver		= {
>  		.name		= "gpio-backlight",
>  		.owner		= THIS_MODULE,
> +		.of_match_table = of_match_ptr(gpio_backlight_of_match),
>  	},
>  	.probe		= gpio_backlight_probe,
>  };
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jingoo Han]

On Wednesday, October 23, 2013 5:02 AM, Thierry Reding wrote:
> On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> > > On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > > > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > > > > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > > > > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > ---
> > > > > ChangeLog v3->v4:
> > > > > - The default-brightness property is now optional, it defaults to 1 if not set.
> > > > by default we set OFF not ON
> > > >
> > > > do not actiate driver or properti by default you can not known to consequence
> > > > on the hw
> > >
> > > Turning on a backlight by default is what pretty much every backlight
> > > driver does. I personally think that's the wrong default, I even tried
> > > to get some discussion started recently about how we could change this.
> > > However, given that this has been the case for possibly as long as the
> > > subsystem has existed, suddenly changing it might cause quite a few of
> > > our users to boot the new kernel and not see their display come up. As
> > > with any other ABI, this isn't something we can just change without a
> > > very good migration path.
> >
> > I'm sorry but the blacklight descibe in DT have nothing to do with the common
> > pratice that the current driver have today
> 
> That's not at all what I said. What I said was that the majority of
> backlight drivers currently default to turning the backlight on when
> probed. Therefore I think it would be consistent if this driver did the
> same.
> 
> I also said that I don't think it's a very good default, but at the same
> time we can't just go and change the default behaviour at will because
> people may rely on it.

I agree with your opinion.
But, I can't decide how to change it.

> 
> > put on by default if wrong specially without the property define. Even put it
> > on by default it wrong as the bootloader may have set it already for splash
> > screen and to avoid glitch the drivers need to detect this.
> 
> I agree that would be preferable, but I don't know of any way to detect
> what value the bootloader set a GPIO to. The GPIO API requires that you
> call gpio_direction_output(), and that requires a value parameter which
> will be used as the output level of the GPIO.

Jean-Christophe's point is right.

We may need to discuss 'the way to detect what value the bootloader
set a GPIO to'.

> > For me this should not even be a property but handled by the driver them
> > selves in C.
> 
> Agreed. There has been some discussion recently about whether devicetree
> should be extended (or supplemented) to allow defining behaviour as well
> (in addition to just hardware). But that's not immediately relevant here
> at this time.

Agreed.

Best regards,
Jingoo Han

--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jingoo Han]

On Friday, October 25, 2013 10:58 PM, Laurent Pinchart wrote:
> On Thursday 24 October 2013 13:05:25 Thierry Reding wrote:
> > On Thu, Oct 24, 2013 at 12:38:59AM +0200, Laurent Pinchart wrote:
> > > On Wednesday 23 October 2013 22:20:12 Thierry Reding wrote:
> > > > On Wed, Oct 23, 2013 at 05:51:13PM +0100, Stephen Warren wrote:
> > > > > On 10/22/2013 09:01 PM, Thierry Reding wrote:
> > > > > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> > > > >
> > > > > > PLAGNIOL-VILLARD wrote:
> > > > > ...
> > > > >
> > > > > >> I'm sorry but the blacklight descibe in DT have nothing to do
> > > > > >> with the common pratice that the current driver have today
> > > > > >
> > > > > > That's not at all what I said. What I said was that the majority
> > > > > > of backlight drivers currently default to turning the backlight on
> > > > > > when probed. Therefore I think it would be consistent if this
> > > > > > driver did the same.
> > > > > >
> > > > > > I also said that I don't think it's a very good default, but at the
> > > > > > same time we can't just go and change the default behaviour at will
> > > > > > because people may rely on it.
> > > > >
> > > > > It may well be reasonable to change the default behaviour for devices
> > > > > instantiated from DT. If it's not possible to instantiate the device
> > > > > from DT yet, then it's not possible for anyone to be relying on the
> > > > > default behaviour yet, since there is none. So, perhaps the default
> > > > > could be:
> > > > >
> > > > > * If device instantiated from a board file, default to on, for
> > > > > backwards-compatibility.
> > > > >
> > > > > * If device instantiated from DT, there is no backwards compatibility
> > > > > to be concerned with, since this is a new feature, hence default to
> > > > > off, since we think that's the correct thing to do.
> > > >
> > > > I actually had a patch to do precisely that. However I then realized
> > > > that people have actually been using pwm-backlight in DT for a while
> > > > already and therefore may be relying on that behaviour as well.
> > > >
> > > > It also isn't really an issue of DT vs. non-DT. The simple fact is that
> > > > besides the backlight driver there's usually no other code that enables
> > > > a backlight on boot. The only way to do so that I know of is using the
> > > > DRM panel patches that I've been working on.
> > >
> > > I would very much welcome a refactoring of the backlight code that would
> > > remove the fbdev dependency and hook backlights to panel drivers. That's
> > > something I wanted to work on myself, but that I pushed back after CDF :-)
> >
> > Yeah, that would certainly be very welcome. But it's also a pretty
> > daunting job since there are a whole lot of devices out there that
> > aren't easy to test because, well, they're pretty old and chances
> > are that nobody that actually has one is around.
> >
> > But I guess that we can always try to solve it on a best effort basis,
> > though. Perhaps things can even be done in a backwards-compatible way.
> > I'm thinking for instance that we could introduce a new property, say
> > .enable, but keep any of the others suhc as state, power, fb_blank for
> > backwards-compatibility. Perhaps even emulate them for a while until
> > we've gone and cleaned up all users.
> 
> That would work with me. I don't think we need more than a best effort
> approach to porting existing backlight drivers to the new model. When it comes
> to compatibility with the current interface, I'd like to move the
> compatibility code to the core instead of leaving it in the individual drivers
> if possible.

I agree with you.
Moving the compatibility code to the core from the individual drivers
looks good. :-)

> 
> > Or is there still a reason to have more than a single bit for backlight
> > state? I don't know any hardware that actually makes a difference
> > between FB_BLANK_NORMAL, FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND
> > or FB_BLANK_POWERDOWN.

On Exynos side, I have never seen that FB_BLANK_VSYNC_SUSPEND,
FB_BLANK_HSYNC_SUSPEND are used for controlling display panels or
Display controllers.

However, I heard that FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND are
used for some monitors.

Best regards,
Jingoo Han

> > Many drivers in drivers/video seem to be using
> > those, but for backlights I can see no use other than FB_BLANK_UNBLANK
> > meaning enabled and anything else meaning disabled.
> I see no use case for the fine-grained state in backlights at the moment.
> 

--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4963 bytes --]

On Fri, Nov 01, 2013 at 08:44:48AM +0900, Jingoo Han wrote:
> On Friday, October 25, 2013 10:58 PM, Laurent Pinchart wrote:
> > On Thursday 24 October 2013 13:05:25 Thierry Reding wrote:
> > > On Thu, Oct 24, 2013 at 12:38:59AM +0200, Laurent Pinchart wrote:
> > > > On Wednesday 23 October 2013 22:20:12 Thierry Reding wrote:
> > > > > On Wed, Oct 23, 2013 at 05:51:13PM +0100, Stephen Warren wrote:
> > > > > > On 10/22/2013 09:01 PM, Thierry Reding wrote:
> > > > > > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe
> > > > > >
> > > > > > > PLAGNIOL-VILLARD wrote:
> > > > > > ...
> > > > > >
> > > > > > >> I'm sorry but the blacklight descibe in DT have nothing to do
> > > > > > >> with the common pratice that the current driver have today
> > > > > > >
> > > > > > > That's not at all what I said. What I said was that the majority
> > > > > > > of backlight drivers currently default to turning the backlight on
> > > > > > > when probed. Therefore I think it would be consistent if this
> > > > > > > driver did the same.
> > > > > > >
> > > > > > > I also said that I don't think it's a very good default, but at the
> > > > > > > same time we can't just go and change the default behaviour at will
> > > > > > > because people may rely on it.
> > > > > >
> > > > > > It may well be reasonable to change the default behaviour for devices
> > > > > > instantiated from DT. If it's not possible to instantiate the device
> > > > > > from DT yet, then it's not possible for anyone to be relying on the
> > > > > > default behaviour yet, since there is none. So, perhaps the default
> > > > > > could be:
> > > > > >
> > > > > > * If device instantiated from a board file, default to on, for
> > > > > > backwards-compatibility.
> > > > > >
> > > > > > * If device instantiated from DT, there is no backwards compatibility
> > > > > > to be concerned with, since this is a new feature, hence default to
> > > > > > off, since we think that's the correct thing to do.
> > > > >
> > > > > I actually had a patch to do precisely that. However I then realized
> > > > > that people have actually been using pwm-backlight in DT for a while
> > > > > already and therefore may be relying on that behaviour as well.
> > > > >
> > > > > It also isn't really an issue of DT vs. non-DT. The simple fact is that
> > > > > besides the backlight driver there's usually no other code that enables
> > > > > a backlight on boot. The only way to do so that I know of is using the
> > > > > DRM panel patches that I've been working on.
> > > >
> > > > I would very much welcome a refactoring of the backlight code that would
> > > > remove the fbdev dependency and hook backlights to panel drivers. That's
> > > > something I wanted to work on myself, but that I pushed back after CDF :-)
> > >
> > > Yeah, that would certainly be very welcome. But it's also a pretty
> > > daunting job since there are a whole lot of devices out there that
> > > aren't easy to test because, well, they're pretty old and chances
> > > are that nobody that actually has one is around.
> > >
> > > But I guess that we can always try to solve it on a best effort basis,
> > > though. Perhaps things can even be done in a backwards-compatible way.
> > > I'm thinking for instance that we could introduce a new property, say
> > > .enable, but keep any of the others suhc as state, power, fb_blank for
> > > backwards-compatibility. Perhaps even emulate them for a while until
> > > we've gone and cleaned up all users.
> > 
> > That would work with me. I don't think we need more than a best effort
> > approach to porting existing backlight drivers to the new model. When it comes
> > to compatibility with the current interface, I'd like to move the
> > compatibility code to the core instead of leaving it in the individual drivers
> > if possible.
> 
> I agree with you.
> Moving the compatibility code to the core from the individual drivers
> looks good. :-)
> 
> > 
> > > Or is there still a reason to have more than a single bit for backlight
> > > state? I don't know any hardware that actually makes a difference
> > > between FB_BLANK_NORMAL, FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND
> > > or FB_BLANK_POWERDOWN.
> 
> On Exynos side, I have never seen that FB_BLANK_VSYNC_SUSPEND,
> FB_BLANK_HSYNC_SUSPEND are used for controlling display panels or
> Display controllers.
> 
> However, I heard that FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND are
> used for some monitors.

I think that those may make sense in the context of fbdev, and looking
at some fbdev drivers seems to substantiate that.

However, I don't think backlights have any such capability. I mean they
are either on or off, right? There's no such thing as partially off, or
partially on. How would a backlight behave differently if the panel was
in HSYNC suspend mode or VSYNC suspend mode?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5517 bytes --]

On Fri, Nov 01, 2013 at 08:37:23AM +0900, Jingoo Han wrote:
> On Wednesday, October 23, 2013 5:02 AM, Thierry Reding wrote:
> > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> > > > On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > > > > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > > > > > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > > > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > > > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > > > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > > > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > > > > > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > > ---
> > > > > > ChangeLog v3->v4:
> > > > > > - The default-brightness property is now optional, it defaults to 1 if not set.
> > > > > by default we set OFF not ON
> > > > >
> > > > > do not actiate driver or properti by default you can not known to consequence
> > > > > on the hw
> > > >
> > > > Turning on a backlight by default is what pretty much every backlight
> > > > driver does. I personally think that's the wrong default, I even tried
> > > > to get some discussion started recently about how we could change this.
> > > > However, given that this has been the case for possibly as long as the
> > > > subsystem has existed, suddenly changing it might cause quite a few of
> > > > our users to boot the new kernel and not see their display come up. As
> > > > with any other ABI, this isn't something we can just change without a
> > > > very good migration path.
> > >
> > > I'm sorry but the blacklight descibe in DT have nothing to do with the common
> > > pratice that the current driver have today
> > 
> > That's not at all what I said. What I said was that the majority of
> > backlight drivers currently default to turning the backlight on when
> > probed. Therefore I think it would be consistent if this driver did the
> > same.
> > 
> > I also said that I don't think it's a very good default, but at the same
> > time we can't just go and change the default behaviour at will because
> > people may rely on it.
> 
> I agree with your opinion.
> But, I can't decide how to change it.

One solution that Stephen proposed was to make all DT-based setups use a
new default (off). An advantage of that is that such setups are still
fairly actively being worked on, so we can probably get early adopters
to cope with the new default.

Looking through some DTS files it seems that many use the pwm-backlight
driver. So if we can get some concensus from all the users that changing
the default would be okay, then I think we could reasonably change it.

Another solution perhaps would be to add a property to DT which encodes
the default state of the backlight on boot. This used to be impossible
because the general concensus was that DT should describe hardware only
and not software policy. During the kernel summit this requirement was
somewhat relaxed and it should now be okay to describe system
configuration data, and I think this would be a good match. If the
system is designed to keep the backlight off by default because some
other component (display panel driver) is meant to turn it on later,
that's system configuration data, right?

Perhaps a boolean property could be used:

	backlight {
		compatible = "pwm-backlight";

		...

		backlight-default-off;
	};

> > > put on by default if wrong specially without the property define. Even put it
> > > on by default it wrong as the bootloader may have set it already for splash
> > > screen and to avoid glitch the drivers need to detect this.
> > 
> > I agree that would be preferable, but I don't know of any way to detect
> > what value the bootloader set a GPIO to. The GPIO API requires that you
> > call gpio_direction_output(), and that requires a value parameter which
> > will be used as the output level of the GPIO.
> 
> Jean-Christophe's point is right.
> 
> We may need to discuss 'the way to detect what value the bootloader
> set a GPIO to'.

Since some GPIO hardware simply can't do it, I guess we could resolve to
passing such information via DT. That obviously won't work for non-DT
setups, but I guess that's something we could live with.

One possibility would be to supplement the backlight-default-off
property with another property (backlight-boot-on) that can be passed on
to the kernel from the bootloader to signal that it has turned the
backlight on.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Jingoo Han]

On Friday, November 01, 2013 6:58 PM, Thierry Reding wrote:
> On Fri, Nov 01, 2013 at 08:44:48AM +0900, Jingoo Han wrote:
>> On Friday, October 25, 2013 10:58 PM, Laurent Pinchart wrote:
>>> On Thursday 24 October 2013 13:05:25 Thierry Reding wrote:

[....]

>>>> Or is there still a reason to have more than a single bit for backlight
>>>> state? I don't know any hardware that actually makes a difference
>>>> between FB_BLANK_NORMAL, FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND
>>>> or FB_BLANK_POWERDOWN.
>>
>> On Exynos side, I have never seen that FB_BLANK_VSYNC_SUSPEND,
>> FB_BLANK_HSYNC_SUSPEND are used for controlling display panels or
>> Display controllers.
>>
>> However, I heard that FB_BLANK_VSYNC_SUSPEND, FB_BLANK_HSYNC_SUSPEND are
>> used for some monitors.
> 
> I think that those may make sense in the context of fbdev, and looking
> at some fbdev drivers seems to substantiate that.
> 
> However, I don't think backlights have any such capability. I mean they
> are either on or off, right? There's no such thing as partially off, or
> partially on. How would a backlight behave differently if the panel was
> in HSYNC suspend mode or VSYNC suspend mode?

Hi Thierry Reding.

I agree with you.
In the case of backlight, there is no need that a backlight behaves
differently in HSYNC suspend mode or VSYNC suspend mode.

Best regards,
Jingoo Han

--
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: [PATCHv4] video: backlight: gpio-backlight: Add DT support.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 6146 bytes --]

Hi Thierry,

On Friday 01 November 2013 11:13:47 Thierry Reding wrote:
> On Fri, Nov 01, 2013 at 08:37:23AM +0900, Jingoo Han wrote:
> > On Wednesday, October 23, 2013 5:02 AM, Thierry Reding wrote:
> > > On Tue, Oct 22, 2013 at 05:34:45PM +0200, Jean-Christophe PLAGNIOL-
VILLARD wrote:
> > > > On 09:23 Tue 22 Oct     , Thierry Reding wrote:
> > > > > On Tue, Oct 22, 2013 at 06:58:33AM +0200, Jean-Christophe PLAGNIOL-
VILLARD wrote:
> > > > > > On 11:13 Mon 21 Oct     , Denis Carikli wrote:
> > > > > > > Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> > > > > > > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > > > Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > > > > > > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > > > > > > Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
> > > > > > > Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > > > > > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > > > > > > Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
> > > > > > > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > > Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > > > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > > > > Cc: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
> > > > > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > > > > > > Cc: Eric Bénard <eric-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > > > Signed-off-by: Denis Carikli <denis-fO0SIAKYzcbQT0dZR+AlfA@public.gmane.org>
> > > > > > > ---
> > > > > > > ChangeLog v3->v4:
> > > > > > > - The default-brightness property is now optional, it defaults
> > > > > > > to 1 if not set.> > > > > 
> > > > > > by default we set OFF not ON
> > > > > > 
> > > > > > do not actiate driver or properti by default you can not known to
> > > > > > consequence on the hw
> > > > > 
> > > > > Turning on a backlight by default is what pretty much every
> > > > > backlight driver does. I personally think that's the wrong default,
> > > > > I even tried to get some discussion started recently about how we
> > > > > could change this. However, given that this has been the case for
> > > > > possibly as long as the subsystem has existed, suddenly changing it
> > > > > might cause quite a few of our users to boot the new kernel and not
> > > > > see their display come up. As with any other ABI, this isn't
> > > > > something we can just change without a very good migration path.
> > > > 
> > > > I'm sorry but the blacklight descibe in DT have nothing to do with the
> > > > common pratice that the current driver have today
> > > 
> > > That's not at all what I said. What I said was that the majority of
> > > backlight drivers currently default to turning the backlight on when
> > > probed. Therefore I think it would be consistent if this driver did the
> > > same.
> > > 
> > > I also said that I don't think it's a very good default, but at the same
> > > time we can't just go and change the default behaviour at will because
> > > people may rely on it.
> > 
> > I agree with your opinion.
> > But, I can't decide how to change it.
> 
> One solution that Stephen proposed was to make all DT-based setups use a
> new default (off). An advantage of that is that such setups are still
> fairly actively being worked on, so we can probably get early adopters
> to cope with the new default.
> 
> Looking through some DTS files it seems that many use the pwm-backlight
> driver. So if we can get some concensus from all the users that changing
> the default would be okay, then I think we could reasonably change it.
> 
> Another solution perhaps would be to add a property to DT which encodes
> the default state of the backlight on boot. This used to be impossible
> because the general concensus was that DT should describe hardware only
> and not software policy. During the kernel summit this requirement was
> somewhat relaxed and it should now be okay to describe system
> configuration data, and I think this would be a good match. If the
> system is designed to keep the backlight off by default because some
> other component (display panel driver) is meant to turn it on later,
> that's system configuration data, right?
> 
> Perhaps a boolean property could be used:
> 
> 	backlight {
> 		compatible = "pwm-backlight";
> 
> 		...
> 
> 		backlight-default-off;
> 	};
> 
> > > > put on by default if wrong specially without the property define. Even
> > > > put it on by default it wrong as the bootloader may have set it
> > > > already for splash screen and to avoid glitch the drivers need to
> > > > detect this.
> > > 
> > > I agree that would be preferable, but I don't know of any way to detect
> > > what value the bootloader set a GPIO to. The GPIO API requires that you
> > > call gpio_direction_output(), and that requires a value parameter which
> > > will be used as the output level of the GPIO.
> > 
> > Jean-Christophe's point is right.
> > 
> > We may need to discuss 'the way to detect what value the bootloader
> > set a GPIO to'.
> 
> Since some GPIO hardware simply can't do it, I guess we could resolve to
> passing such information via DT. That obviously won't work for non-DT
> setups, but I guess that's something we could live with.

For what it's worth, the pcf857x GPIO controllers suffer from this problem, 
and the solution was to use a DT property to specify the initial GPIOs state. 
The driver can thus implement gpio_get_value() properly and the hardware 
limitation is hidden from the clients.

https://lkml.org/lkml/2013/9/21/105

> One possibility would be to supplement the backlight-default-off property
> with another property (backlight-boot-on) that can be passed on to the
> kernel from the bootloader to signal that it has turned the backlight on.

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]