| Message ID | 1444987060-48202-1-git-send-email-yh.huang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: > > 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: > > 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
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
On Fri, 2015-10-30 at 11:34 +0100, Philipp Zabel wrote: > 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). 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: > > 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: > > > 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. 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: > > Hi YH, > > > > Am Dienstag, den 03.11.2015, 16:11 +0800 schrieb YH Huang: > > > > 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. > > 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: > > > > 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
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);
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 <yh.huang@mediatek.com> --- Change in v2: Fix the build error. --- drivers/video/backlight/pwm_bl.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)