[PATCH v2] pwm-backlight: fix the panel power sequence

In order to match the panel power sequence, disable the enable_gpio
in the probe function. Also, reorder the code in the power_on and
power_off function to match the timing.

Signed-off-by: YH Huang <redacted>
---
Change in v2:
Fix the build error.
---
 drivers/video/backlight/pwm_bl.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index eff379b..d8c9c62 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c

@@ -54,10 +54,11 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
+	pwm_enable(pb->pwm);
+
 	if (pb->enable_gpio)
 		gpiod_set_value(pb->enable_gpio, 1);
 
-	pwm_enable(pb->pwm);
 	pb->enabled = true;
 }
 

@@ -66,12 +67,12 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	if (!pb->enabled)
 		return;
 
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-
 	if (pb->enable_gpio)
 		gpiod_set_value(pb->enable_gpio, 0);
 
+	pwm_config(pb->pwm, 0, pb->period);
+	pwm_disable(pb->pwm);
+
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }

@@ -242,7 +243,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,6 +265,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		pb->enable_gpio = gpio_to_desc(data->enable_gpio);
 	}
 
+	if (pb->enable_gpio)
+		gpiod_direction_output(pb->enable_gpio, 0);
+
 	pb->power_supply = devm_regulator_get(&pdev->dev, "power");
 	if (IS_ERR(pb->power_supply)) {
 		ret = PTR_ERR(pb->power_supply);

-- 
1.7.9.5

Am Freitag, den 16.10.2015, 17:17 +0800 schrieb YH Huang:

In order to match the panel power sequence, disable the enable_gpio
in the probe function. Also, reorder the code in the power_on and
power_off function to match the timing.

Could you also have a look at the "pwm-backlight: Avoid backlight
flicker when probed from DT" patch?
I think in case of a panel left enabled by the bootloader, your patch
will disable the backlight for a short time.

best regards
Philipp

On Fri, 2015-10-16 at 11:36 +0200, Philipp Zabel wrote:

Am Freitag, den 16.10.2015, 17:17 +0800 schrieb YH Huang:

quoted

In order to match the panel power sequence, disable the enable_gpio
in the probe function. Also, reorder the code in the power_on and
power_off function to match the timing.

Could you also have a look at the "pwm-backlight: Avoid backlight
flicker when probed from DT" patch?
I think in case of a panel left enabled by the bootloader, your patch
will disable the backlight for a short time.

best regards
Philipp

In the case of the panel disabled by the bootloader,
your patch still has the following code and always enables the backlight
in the probe function.
pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
-						  GPIOD_OUT_HIGH);

What do you think if I remove these two lines in my patch?
if (pb->enable_gpio)
	gpiod_direction_output(pb->enable_gpio, 0);


Regards,
YH Huang

Hi YH,

Am Donnerstag, den 22.10.2015, 23:12 +0800 schrieb YH Huang:

In the case of the panel disabled by the bootloader,
your patch still has the following code and always enables the backlight
in the probe function.
pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
-						  GPIOD_OUT_HIGH);

You are right.

What do you think if I remove these two lines in my patch?
if (pb->enable_gpio)
	gpiod_direction_output(pb->enable_gpio, 0);

That won't work if the gpio is still configured as input. How about I
add the GPIOD_ASIS change to my patch you remove that and the above from
yours?

best regards
Philipp

Hi Hpilipp,

On Thu, 2015-10-29 at 16:40 +0100, Philipp Zabel wrote:

Hi YH,

Am Donnerstag, den 22.10.2015, 23:12 +0800 schrieb YH Huang:

quoted

In the case of the panel disabled by the bootloader,
your patch still has the following code and always enables the backlight
in the probe function.
pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
-						  GPIOD_OUT_HIGH);

You are right.

quoted

What do you think if I remove these two lines in my patch?
if (pb->enable_gpio)
	gpiod_direction_output(pb->enable_gpio, 0);

That won't work if the gpio is still configured as input. How about I
add the GPIOD_ASIS change to my patch you remove that and the above from
yours?

I revise these two lines 
if (pb->enable_gpio)
	gpiod_direction_output(pb->enable_gpio, 0);
into
if (pb->enable_gpio) {
	if(gpiod_get_value(pb->enable_gpio) = 0)
		gpiod_direction_output(pb->enable_gpio, 0);
	else
		gpiod_direction_output(pb->enable_gpio, 1);
}

I am not sure what "phandle" is working for.
My change is like your patch but remove the "phandle related" and
"gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT".
Do we really need these?

Also, do you think it is right that I do the "pwm_enable(pb->pwm);"
before "gpiod_set_value(pb->enable_gpio, 1);" in power on function?

Regards,
YH Huang

Am Freitag, den 30.10.2015, 15:41 +0800 schrieb YH Huang:

quoted

That won't work if the gpio is still configured as input. How about I
add the GPIOD_ASIS change to my patch you remove that and the above from
yours?

I revise these two lines 
if (pb->enable_gpio)
	gpiod_direction_output(pb->enable_gpio, 0);
into
if (pb->enable_gpio) {
	if(gpiod_get_value(pb->enable_gpio) = 0)
		gpiod_direction_output(pb->enable_gpio, 0);
	else
		gpiod_direction_output(pb->enable_gpio, 1);
}

If the GPIO is still configured as an input, the return value of
gpiod_get_value could be random.

I am not sure what "phandle" is working for.

The reasoning is that devices where there is no phandle link pointing to
the backlight (for example from a simple-panel node), we should keep the
current default behaviour (enable during probe).

My change is like your patch but remove the "phandle related" and
"gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT".
Do we really need these?

See above, I wanted to be careful not to change existing behavior in
unexpected ways.

Also, do you think it is right that I do the "pwm_enable(pb->pwm);"
before "gpiod_set_value(pb->enable_gpio, 1);" in power on function?

That is unclear to me, because I'm sure that depends on the actual
constant current LED driver used.
I suspect EN and PWM pins are completely independent on a lot of them,
so enabling EN last should be ok to work around the problem that the PWM
pin state might not be well defined when the PWM is disabled.

For example, the STCS1A datasheet doesn't mention any order for enabling
EN and PWM. In the LP8545 datasheet, the modes of operation state
diagram suggests that enabling EN first is the proper way, on the other
hand the PWM interface characteristics table also lists a startup delay
for a rising EN edge while PWM is already active.

But I'm not sure that there aren't any LED drivers that somehow have a
problem with this order.

regards
Philipp

On Fri, 2015-10-30 at 11:34 +0100, Philipp Zabel wrote:

Am Freitag, den 30.10.2015, 15:41 +0800 schrieb YH Huang:

quoted

quoted

That won't work if the gpio is still configured as input. How about I
add the GPIOD_ASIS change to my patch you remove that and the above from
yours?

I revise these two lines 
if (pb->enable_gpio)
	gpiod_direction_output(pb->enable_gpio, 0);
into
if (pb->enable_gpio) {
	if(gpiod_get_value(pb->enable_gpio) = 0)
		gpiod_direction_output(pb->enable_gpio, 0);
	else
		gpiod_direction_output(pb->enable_gpio, 1);
}

If the GPIO is still configured as an input, the return value of
gpiod_get_value could be random.

quoted

I am not sure what "phandle" is working for.

The reasoning is that devices where there is no phandle link pointing to
the backlight (for example from a simple-panel node), we should keep the
current default behaviour (enable during probe).

I have a little problem for the current default behaviour.
Should we enable during probe?

Before this patch ( http://patchwork.ozlabs.org/patch/324690/ ),
we disable "enable-gpio" in the probe function.

Do you have any idea of this?

Regards,
YH Huang

Hi YH,

Am Dienstag, den 03.11.2015, 16:11 +0800 schrieb YH Huang:

quoted

The reasoning is that devices where there is no phandle link pointing to
the backlight (for example from a simple-panel node), we should keep the
current default behaviour (enable during probe).

I have a little problem for the current default behaviour.
Should we enable during probe?

Here I mean enabling the backlight (at the end of the probe function),
not enabling the GPIO already when requesting it.

Before this patch ( http://patchwork.ozlabs.org/patch/324690/ ),
we disable "enable-gpio" in the probe function.

While before this patch the GPIO would be initialized in the disabled
state, the call to backlight_update_status at the end of the probe
function would still enable the backlight afterwards.

regards
Philipp

On Tue, 2015-11-03 at 12:08 +0100, Philipp Zabel wrote:

Hi YH,

Am Dienstag, den 03.11.2015, 16:11 +0800 schrieb YH Huang:

quoted

quoted

The reasoning is that devices where there is no phandle link pointing to
the backlight (for example from a simple-panel node), we should keep the
current default behaviour (enable during probe).

I have a little problem for the current default behaviour.
Should we enable during probe?

Here I mean enabling the backlight (at the end of the probe function),
not enabling the GPIO already when requesting it.

quoted

Before this patch ( http://patchwork.ozlabs.org/patch/324690/ ),
we disable "enable-gpio" in the probe function.

While before this patch the GPIO would be initialized in the disabled
state, the call to backlight_update_status at the end of the probe
function would still enable the backlight afterwards.

Based on this, could we disable it initially and update in the
backlight_update_status function?

Like this,

if (pb->enable_gpio) {
	if (phandle &&
	    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
	    gpiod_get_value(pb->enable_gpio) = 1)
		gpiod_direction_output(pb->enable_gpio, 1);
	else
		gpiod_direction_output(pb->enable_gpio, 0);
}

And then update with props.brightness in backlight_update_status.
I am not sure, maybe I miss something.

Regards,
YH Huang

Am Mittwoch, den 04.11.2015, 09:47 +0800 schrieb YH Huang:

On Tue, 2015-11-03 at 12:08 +0100, Philipp Zabel wrote:

quoted

Hi YH,

Am Dienstag, den 03.11.2015, 16:11 +0800 schrieb YH Huang:

quoted

quoted

The reasoning is that devices where there is no phandle link pointing to
the backlight (for example from a simple-panel node), we should keep the
current default behaviour (enable during probe).

I have a little problem for the current default behaviour.
Should we enable during probe?

Here I mean enabling the backlight (at the end of the probe function),
not enabling the GPIO already when requesting it.

quoted

Before this patch ( http://patchwork.ozlabs.org/patch/324690/ ),
we disable "enable-gpio" in the probe function.

While before this patch the GPIO would be initialized in the disabled
state, the call to backlight_update_status at the end of the probe
function would still enable the backlight afterwards.

Based on this, could we disable it initially and update in the
backlight_update_status function?

Like this,

if (pb->enable_gpio) {
	if (phandle &&
	    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
	    gpiod_get_value(pb->enable_gpio) = 1)
		gpiod_direction_output(pb->enable_gpio, 1);

The gpiod_direction_output call is a no-op, since the direction is
already output and the value is already 1.
Also, I propose to set initial blanking to FB_BLANK_POWERDOWN in this
case, and wait for the panel driver to enable the backlight at the
appropriate time.

regards
Philipp

On Thu, 2015-11-05 at 10:40 +0100, Philipp Zabel wrote:

quoted

Based on this, could we disable it initially and update in the
backlight_update_status function?

Like this,

if (pb->enable_gpio) {
	if (phandle &&
	    gpiod_get_direction(pb->enable_gpio) = GPIOF_DIR_OUT &&
	    gpiod_get_value(pb->enable_gpio) = 1)
		gpiod_direction_output(pb->enable_gpio, 1);

The gpiod_direction_output call is a no-op, since the direction is
already output and the value is already 1.
Also, I propose to set initial blanking to FB_BLANK_POWERDOWN in this
case, and wait for the panel driver to enable the backlight at the
appropriate time.

Thanks your kindly reply.
Your patch looks good to me.

Regards,
YH Huang