[PATCH] pwm-backlight: Avoid backlight flicker when probed from DT

[Philipp Zabel]

If the driver is probed from the device tree, and there is a phandle
property set on it, and the enable GPIO is already configured as output,
and the backlight is currently disabled, keep it disabled.
If all these conditions are met, assume there will be some other driver
that can enable the backlight at the appropriate time.

Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
Reviewed-by: Christian Gmeiner <christia...@gmail.com>
---
Changes since v2:
- Leave GPIO configuration as is (GPIOD_ASIS) when requesting,
so we can actually check the initial state.
---
drivers/video/backlight/pwm_bl.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index ae3c6b6..8e9c261 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -199,6 +199,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
struct backlight_properties props;
struct backlight_device *bl;
struct pwm_bl_data *pb;
+ int initial_blank = FB_BLANK_UNBLANK;
+ bool phandle;
int ret;

if (!data) {
@@ -242,7 +244,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enabled = false;

pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
- GPIOD_OUT_HIGH);
+ GPIOD_ASIS);
if (IS_ERR(pb->enable_gpio)) {
ret = PTR_ERR(pb->enable_gpio);
goto err_alloc;
@@ -264,12 +266,32 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}

+ phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;
+
+ if (pb->enable_gpio) {
+ /*
+ * If the driver is probed from the device tree and there is a
+ * phandle link pointing to the backlight node, it is safe to
+ * assume that another driver will enable the backlight at the
+ * appropriate time. Therefore, if it is disabled, keep it so.
+ */
+ if (phandle &&
+ gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
+ gpiod_get_value(pb->enable_gpio) == 0)
+ initial_blank = FB_BLANK_POWERDOWN;
+ else
+ gpiod_direction_output(pb->enable_gpio, 1);
+ }
+
pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
ret = PTR_ERR(pb->power_supply);
goto err_alloc;
}

+ if (phandle && !regulator_is_enabled(pb->power_supply))
+ initial_blank = FB_BLANK_POWERDOWN;
+
pb->pwm = devm_pwm_get(&pdev->dev, NULL);
if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
&& !pdev->dev.of_node) {
@@ -320,6 +342,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
}

bl->props.brightness = data->dft_brightness;
+ bl->props.power = initial_blank;
backlight_update_status(bl);

platform_set_drvdata(pdev, bl);
--
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Lee Jones]

This is a little ugly.

If this is the only way to identify the probedness of a device then
you probably want to neaten it up a little. I suggest something along
the lines of:

if (of_find_property(pdev->dev.of_node, "phandle", NULL))
already_probed = true;

Also, do you also want to check for "linux,phandle", as this is always
catered for in drivers/of/*

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

[Philipp Zabel]

Hi Lee,

Am Freitag, den 30.10.2015, 18:21 +0000 schrieb Lee Jones:
> > @@ -264,12 +266,32 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> > pb->enable_gpio = gpio_to_desc(data->enable_gpio);
> > }
> >
> > + phandle = of_find_property(pdev->dev.of_node, "phandle", NULL) != NULL;
>
> This is a little ugly.
>
> If this is the only way to identify the probedness of a device then
> you probably want to neaten it up a little. I suggest something along
> the lines of:
>
> if (of_find_property(pdev->dev.of_node, "phandle", NULL))
> already_probed = true;
>
> Also, do you also want to check for "linux,phandle", as this is always
> catered for in drivers/of/*

Thanks for pointing this out. I suppose I should just check
pdev->dev.of_node->phandle != 0 instead. That will already be
initialized by unflatten_dt_node if either property was found.

regards
Philipp

[Philipp Zabel]

If the driver is probed from the device tree, and there is a phandle
property set on it, and the enable GPIO is already configured as output,
and the backlight is currently disabled, keep it disabled.
If all these conditions are met, assume there will be some other driver
that can enable the backlight at the appropriate time.

Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
Reviewed-by: Christian Gmeiner <christia...@gmail.com>
---

Changes since v3:
- No need to call of_find_property(of_node, "phandle", NULL),
just use of_node->phandle.
- Rebased onto v4.3
---
drivers/video/backlight/pwm_bl.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..65e57da 100644

--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -199,6 +199,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
struct backlight_properties props;
struct backlight_device *bl;
struct pwm_bl_data *pb;

+ phandle phandle = pdev->dev.of_node->phandle;

+ int initial_blank = FB_BLANK_UNBLANK;

int ret;

if (!data) {
@@ -242,7 +244,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->enabled = false;

pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
- GPIOD_OUT_HIGH);
+ GPIOD_ASIS);
if (IS_ERR(pb->enable_gpio)) {
ret = PTR_ERR(pb->enable_gpio);
goto err_alloc;

@@ -264,12 +266,30 @@ static int pwm_backlight_probe(struct platform_device *pdev)

pb->enable_gpio = gpio_to_desc(data->enable_gpio);
}

+ if (pb->enable_gpio) {
+ /*
+ * If the driver is probed from the device tree and there is a
+ * phandle link pointing to the backlight node, it is safe to
+ * assume that another driver will enable the backlight at the
+ * appropriate time. Therefore, if it is disabled, keep it so.
+ */
+ if (phandle &&
+ gpiod_get_direction(pb->enable_gpio) == GPIOF_DIR_OUT &&
+ gpiod_get_value(pb->enable_gpio) == 0)
+ initial_blank = FB_BLANK_POWERDOWN;
+ else
+ gpiod_direction_output(pb->enable_gpio, 1);
+ }
+
pb->power_supply = devm_regulator_get(&pdev->dev, "power");
if (IS_ERR(pb->power_supply)) {
ret = PTR_ERR(pb->power_supply);
goto err_alloc;
}

+ if (phandle && !regulator_is_enabled(pb->power_supply))
+ initial_blank = FB_BLANK_POWERDOWN;
+
pb->pwm = devm_pwm_get(&pdev->dev, NULL);

if (IS_ERR(pb->pwm)) {
ret = PTR_ERR(pb->pwm);
@@ -321,6 +341,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)

}

bl->props.brightness = data->dft_brightness;
+ bl->props.power = initial_blank;
backlight_update_status(bl);

platform_set_drvdata(pdev, bl);
--

2.6.1

[Heiko Stuebner]

Hi Philipp,

Am Montag, 2. November 2015, 17:55:56 schrieb Philipp Zabel:
> If the driver is probed from the device tree, and there is a phandle
> property set on it, and the enable GPIO is already configured as output,
> and the backlight is currently disabled, keep it disabled.
> If all these conditions are met, assume there will be some other driver
> that can enable the backlight at the appropriate time.
>
> Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
> Reviewed-by: Christian Gmeiner <christia...@gmail.com>

this patch improves the white screen when booting a veyron chromebook
a lot. I still see a small white flash, but that can probably come from
the WIP edp driver.

Tested-by: Heiko Stuebner <he...@sntech.de>

[Thierry Reding]

On Tue, Nov 10, 2015 at 03:18:10PM +0100, Heiko Stuebner wrote:
> Hi Philipp,
>
> Am Montag, 2. November 2015, 17:55:56 schrieb Philipp Zabel:
> > If the driver is probed from the device tree, and there is a phandle
> > property set on it, and the enable GPIO is already configured as output,
> > and the backlight is currently disabled, keep it disabled.
> > If all these conditions are met, assume there will be some other driver
> > that can enable the backlight at the appropriate time.
> >
> > Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
> > Reviewed-by: Christian Gmeiner <christia...@gmail.com>
>
> this patch improves the white screen when booting a veyron chromebook
> a lot. I still see a small white flash, but that can probably come from
> the WIP edp driver.

Some panels require a couple of frames before they actually enable. You
may want to look at the datasheet of your panel to see if it has some
specific requirement and update the panel driver with that.

From a high-level view the way that this is supposed to work is that
your encoder driver (e.g. eDP) "prepares" the panel, then starts sending
frames and finally "enables" the panel. With something like the simple
panel driver you can influence this by setting the delay.enable field in
the panel descriptor.

See struct panel_desc in drivers/gpu/drm/panel/panel-simple.c

Thierry

[Philipp Zabel]

If the driver is probed from the device tree, and there is a phandle
property set on it, and the enable GPIO is already configured as output,
and the backlight is currently disabled, keep it disabled.
If all these conditions are met, assume there will be some other driver
that can enable the backlight at the appropriate time.

Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
Reviewed-by: Christian Gmeiner <christia...@gmail.com>

Tested-by: Heiko Stuebner <he...@sntech.de>
---
- Rebased onto v4.4-rc1
- Added Heiko's Tested-by

---
drivers/video/backlight/pwm_bl.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c

index ae3c6b6..3daf9cc 100644

if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER
&& !pdev->dev.of_node) {

@@ -320,6 +340,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)

}

bl->props.brightness = data->dft_brightness;
+ bl->props.power = initial_blank;
backlight_update_status(bl);

platform_set_drvdata(pdev, bl);
--

2.6.2

[Lee Jones]

On Wed, 18 Nov 2015, Philipp Zabel wrote:

> If the driver is probed from the device tree, and there is a phandle
> property set on it, and the enable GPIO is already configured as output,
> and the backlight is currently disabled, keep it disabled.
> If all these conditions are met, assume there will be some other driver
> that can enable the backlight at the appropriate time.
>
> Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
> Reviewed-by: Christian Gmeiner <christia...@gmail.com>
> Tested-by: Heiko Stuebner <he...@sntech.de>
> ---
> - Rebased onto v4.4-rc1
> - Added Heiko's Tested-by
> ---
> drivers/video/backlight/pwm_bl.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)

Applied, thanks.

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

[Heiko Stübner]

Hi Thierry,

Thanks for the pointers :-)

Just to follow up a little bit, after moving to Yakir's Analogix-dp driver
from the chromeos-one I was using till now, that actually works nicely for me
and even the short flash is gone.


Heiko

[Daniel Kurtz]

Hi Lee,

On Wed, Nov 25, 2015 at 12:26 AM, Lee Jones <lee....@linaro.org> wrote:
> On Wed, 18 Nov 2015, Philipp Zabel wrote:
>
>> If the driver is probed from the device tree, and there is a phandle
>> property set on it, and the enable GPIO is already configured as output,
>> and the backlight is currently disabled, keep it disabled.
>> If all these conditions are met, assume there will be some other driver
>> that can enable the backlight at the appropriate time.
>>
>> Signed-off-by: Philipp Zabel <p.z...@pengutronix.de>
>> Reviewed-by: Christian Gmeiner <christia...@gmail.com>
>> Tested-by: Heiko Stuebner <he...@sntech.de>
>> ---
>> - Rebased onto v4.4-rc1
>> - Added Heiko's Tested-by
>> ---
>> drivers/video/backlight/pwm_bl.c | 23 ++++++++++++++++++++++-
>> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> Applied, thanks.

Hmm, where was this applied? I do not see it in:
* v4.4-rc4
* linux-next next-20151203 (dcccebc - Add linux-next specific files
for 20151203)

Nor do I see it in any branch in:
* git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight
Is this the right git?
By the way this repository has a git tag called "HEAD", which is a bit
annoying since every time I "git remote update" from here, git gets
confused until I manually delete it (git tag -d HEAD).

Thanks,
-Dan

[Lee Jones]

Yes, that's right. When I say "applied", it means I applied it to my
local repository. I am careful not to allude to the patch being
"pushed". There is no guarantee when this will happen, since it
completly depends how busy the subsystem is. For Backlight, as the
churn is low, the impetus for pushing is pretty low.

It has been "pushed" now though.

> By the way this repository has a git tag called "HEAD", which is a bit
> annoying since every time I "git remote update" from here, git gets
> confused until I manually delete it (git tag -d HEAD).

Thanks for letting me know. I have now removed this tag.

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