diff mbox series

[RESEND,v5,1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle.

Message ID 20180216174034.15936-1-enric.balletbo@collabora.com
State Superseded
Headers show
Series [RESEND,v5,1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle. | expand

Commit Message

Enric Balletbo i Serra Feb. 16, 2018, 5:40 p.m. UTC 
Before this patch the enable signal was set before the PWM signal and
vice-versa on power off. This sequence is wrong, at least, it is on
the different panels datasheets that I checked, so I inverted the sequence
to follow the specs.

For reference the following panels have the mentioned sequence:
  - N133HSE-EA1 (Innolux)
  - N116BGE (Innolux)
  - N156BGE-L21 (Innolux)
  - B101EAN0 (Auo)
  - B101AW03 (Auo)
  - LTN101NT05 (Samsung)
  - CLAA101WA01A (Chunghwa)

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Acked-by: Jingoo Han <jingoohan1@gmail.com>
---
Changes since v4:
 - Rebase on top of mainline.
 - Add the acks from Daniel Thompson and Jingoo Han.
Changes since v3:
 - List the part numbers for the panel checked (Daniel Thompson)
Changes since v2:
 - Add this as a separate patch (Thierry Reding)
Changes since v1:
 - None

 drivers/video/backlight/pwm_bl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Thierry Reding March 19, 2018, 4:58 p.m. UTC | #1 
On Fri, Feb 16, 2018 at 06:40:30PM +0100, Enric Balletbo i Serra wrote:
> Before this patch the enable signal was set before the PWM signal and
> vice-versa on power off. This sequence is wrong, at least, it is on
> the different panels datasheets that I checked, so I inverted the sequence
> to follow the specs.
> 
> For reference the following panels have the mentioned sequence:
>   - N133HSE-EA1 (Innolux)
>   - N116BGE (Innolux)
>   - N156BGE-L21 (Innolux)
>   - B101EAN0 (Auo)
>   - B101AW03 (Auo)
>   - LTN101NT05 (Samsung)
>   - CLAA101WA01A (Chunghwa)
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Acked-by: Jingoo Han <jingoohan1@gmail.com>
> ---
> Changes since v4:
>  - Rebase on top of mainline.
>  - Add the acks from Daniel Thompson and Jingoo Han.
> Changes since v3:
>  - List the part numbers for the panel checked (Daniel Thompson)
> Changes since v2:
>  - Add this as a separate patch (Thierry Reding)
> Changes since v1:
>  - None
> 
>  drivers/video/backlight/pwm_bl.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

I'm surprised that panels even care about this. Do you see actual
breakage if these are the other way around?

Anyway, this seems to me as legit as the other way around, so:

Acked-by: Thierry Reding <thierry.reding@gmail.com>
Enric Balletbo i Serra March 19, 2018, 6:28 p.m. UTC | #2 
Hi Thierry,

Thanks for your ack.

On 19/03/18 17:58, Thierry Reding wrote:
> On Fri, Feb 16, 2018 at 06:40:30PM +0100, Enric Balletbo i Serra wrote:
>> Before this patch the enable signal was set before the PWM signal and
>> vice-versa on power off. This sequence is wrong, at least, it is on
>> the different panels datasheets that I checked, so I inverted the sequence
>> to follow the specs.
>>
>> For reference the following panels have the mentioned sequence:
>>   - N133HSE-EA1 (Innolux)
>>   - N116BGE (Innolux)
>>   - N156BGE-L21 (Innolux)
>>   - B101EAN0 (Auo)
>>   - B101AW03 (Auo)
>>   - LTN101NT05 (Samsung)
>>   - CLAA101WA01A (Chunghwa)
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
>> Acked-by: Jingoo Han <jingoohan1@gmail.com>
>> ---
>> Changes since v4:
>>  - Rebase on top of mainline.
>>  - Add the acks from Daniel Thompson and Jingoo Han.
>> Changes since v3:
>>  - List the part numbers for the panel checked (Daniel Thompson)
>> Changes since v2:
>>  - Add this as a separate patch (Thierry Reding)
>> Changes since v1:
>>  - None
>>
>>  drivers/video/backlight/pwm_bl.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> I'm surprised that panels even care about this. Do you see actual
> breakage if these are the other way around?
> 

At least the B116XTN02 requires enable first the PWM, wait 10ms and then BL_EN
to avoid garbage. I did not observe the issue on other panels but the datasheets
I checked specifies this sequence as correct.

> Anyway, this seems to me as legit as the other way around, so:
> 
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1c2289ddd555..698ec68bcdc9 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_cansleep(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_cansleep(pb->enable_gpio, 0);
 
+	pwm_config(pb->pwm, 0, pb->period);
+	pwm_disable(pb->pwm);
+
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }