Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1253902 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] pwm-backlight: fix the panel power sequence YH Huang <yh.huang@mediatek.com> - 2015-10-22 17:20 +0200
    Re: [PATCH v2] pwm-backlight: fix the panel power sequence Philipp Zabel <p.zabel@pengutronix.de> - 2015-10-29 16:50 +0100
      Re: [PATCH v2] pwm-backlight: fix the panel power sequence YH Huang <yh.huang@mediatek.com> - 2015-10-30 08:50 +0100
        Re: [PATCH v2] pwm-backlight: fix the panel power sequence Philipp Zabel <p.zabel@pengutronix.de> - 2015-10-30 11:40 +0100

#1253902 — Re: [PATCH v2] pwm-backlight: fix the panel power sequence

On Fri, 2015-10-16 at 11:36 +0200, Philipp Zabel wrote:
> 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
> 
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

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

[toc] | [next] | [standalone]

#1258872

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

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

[toc] | [prev] | [next] | [standalone]

#1259278

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

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

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

[toc] | [prev] | [next] | [standalone]

#1259382

Am Freitag, den 30.10.2015, 15:41 +0800 schrieb YH Huang:
> > 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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web