| Message ID | 1382346813-8449-1-git-send-email-denis@eukrea.com |
|---|---|
| State | New |
| Headers | show |
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"); 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, > };
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 >
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 >
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. 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
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
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@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 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
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@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 > > 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
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.
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.
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
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
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.
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
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.
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@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 > > 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
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. >
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
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@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 > > > > 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
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
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@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 > > > > > > 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.
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, };
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