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 <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Acked-by: Jingoo Han <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Changes since v5:
- Add Acked-by: Thierry Reding <[email protected]>
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(-)
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;
}
--
2.16.2
For veyron the binding should provide both PWM timings, the delay between
you enable the PWM and set the enable signal, and the delay between you
disable the PWM signal and clear the enable signal. Update the binding
accordingly, in this case the panels connected to the veyron boards have
a symmetric power sequence, hence the same value is used.
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
Changes since v5:
- None.
Changes since v4:
- Rebased on top of mainline.
Changes since v3:
- Use new -ms names for proprieties.
Changes since v2:
- Use new names for proprieties.
Changes since v1:
- Add this new patch to fix current binding on veyron.
---
arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi b/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
index d752a315f884..5a8c7f3abb38 100644
--- a/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
@@ -96,7 +96,8 @@
pinctrl-names = "default";
pinctrl-0 = <&bl_en>;
pwms = <&pwm0 0 1000000 0>;
- pwm-delay-us = <10000>;
+ post-pwm-on-delay-ms = <10>;
+ pwm-off-delay-ms = <10>;
};
gpio-charger {
--
2.16.2
Hardware needs a delay between setting an initial (non-zero) PWM and
enabling the backlight using GPIO. The post-pwm-on-delay-ms specifies
this delay in milli seconds. Hardware also needs a delay between disabing
the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-ms
is this delay in milli seconds.
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Acked-by: Pavel Machek <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Based on the original Huang Lin <[email protected]> work.
Changes since v5:
- Add Acked-by: Thierry Reding <[email protected]>
- CC DT mainling list (for some reason dropped at some point, my
fault)
Changes since v4:
- Rebase on top of mainline.
Changes since v3:
- Replace us for ms.
- Add Acked-by: Pavel Machek <[email protected]>
Changes since v2:
- Use separate properties (Rob Herring)
Changes since v1:
- As suggested by Daniel Thompson
- Do not assume power-on delay and power-off delay will be the same
---
Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 764db86d441a..310810906613 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -17,6 +17,10 @@ Optional properties:
"pwms" property (see PWM binding[0])
- enable-gpios: contains a single GPIO specifier for the GPIO which enables
and disables the backlight (see GPIO binding[1])
+ - post-pwm-on-delay-ms: Delay in ms between setting an initial (non-zero) PWM
+ and enabling the backlight using GPIO.
+ - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
+ and setting PWM value to 0.
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
[1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -32,4 +36,6 @@ Example:
power-supply = <&vdd_bl_reg>;
enable-gpios = <&gpio 58 0>;
+ post-pwm-on-delay-ms = <10>;
+ pwm-off-delay-ms = <10>;
};
--
2.16.2
The minnie devices comes with an AUO B101EAN01 panel which is different
from default veyron devices, thus the power on/off timing sequence is
slightly different. The datasheet specifies a pwm delay of 200 ms, so
update the PMW delay proprieties accordingly.
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
Changes since v5:
- None.
Changes since v4:
- Rebase on top of mainline.
Changes since v3:
- Use new -ms names for proprieties.
- Fix the delay, should be 200ms instead of 20ms (Pavel)
Changes since v2:
- Use new names for proprieties.
Changes since v1:
- Add this new patch as minnie has differents timings
---
arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
index 544de6027aaa..4c5307e62001 100644
--- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
@@ -123,6 +123,8 @@
240 241 242 243 244 245 246 247
248 249 250 251 252 253 254 255>;
power-supply = <&backlight_regulator>;
+ post-pwm-on-delay-ms = <200>;
+ pwm-off-delay-ms = <200>;
};
&emmc {
--
2.16.2
Some panels (i.e. N116BGE-L41), in their power sequence specifications,
request a delay between set the PWM signal and enable the backlight and
between clear the PWM signal and disable the backlight. Add support for
the new post-pwm-on-delay-ms and pwm-off-delay-ms proprieties to meet
the timings.
Signed-off-by: Enric Balletbo i Serra <[email protected]>
Acked-by: Pavel Machek <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
Acked-by: Jingoo Han <[email protected]>
Acked-by: Thierry Reding <[email protected]>
---
Changes since v5:
- Add Acked-by: Thierry Reding <[email protected]>
Changes since v4:
- Rebased on top of mainline.
- Added the acks from Pavel Machek, Daniel thompson and Jingoo Han
Changes since v3:
- Use two named members instead of pwm_delay[] (Daniel and Pavel)
- Use msleep instead of usleep_range. (Pavel)
Changes since v2:
- Move the pwm/enable sequence to another patch (Thierry Reding)
Changes since v1:
- As suggested by Daniel Thompson
- Do not assume power-on delay and power-off delay will be the same
- Move the check of dt property to the parse dt function.
---
drivers/video/backlight/pwm_bl.c | 19 +++++++++++++++++++
include/linux/pwm_backlight.h | 2 ++
2 files changed, 21 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 698ec68bcdc9..8e3f1245f5c5 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -10,6 +10,7 @@
* published by the Free Software Foundation.
*/
+#include <linux/delay.h>
#include <linux/gpio/consumer.h>
#include <linux/gpio.h>
#include <linux/module.h>
@@ -35,6 +36,8 @@ struct pwm_bl_data {
struct gpio_desc *enable_gpio;
unsigned int scale;
bool legacy;
+ unsigned int post_pwm_on_delay;
+ unsigned int pwm_off_delay;
int (*notify)(struct device *,
int brightness);
void (*notify_after)(struct device *,
@@ -56,6 +59,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
pwm_enable(pb->pwm);
+ if (pb->post_pwm_on_delay)
+ msleep(pb->post_pwm_on_delay);
+
if (pb->enable_gpio)
gpiod_set_value_cansleep(pb->enable_gpio, 1);
@@ -70,6 +76,9 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
if (pb->enable_gpio)
gpiod_set_value_cansleep(pb->enable_gpio, 0);
+ if (pb->pwm_off_delay)
+ msleep(pb->pwm_off_delay);
+
pwm_config(pb->pwm, 0, pb->period);
pwm_disable(pb->pwm);
@@ -178,6 +187,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
+ /*
+ * These values are optional and set as 0 by default, the out values
+ * are modified only if a valid u32 value can be decoded.
+ */
+ of_property_read_u32(node, "post-pwm-on-delay-ms",
+ &data->post_pwm_on_delay);
+ of_property_read_u32(node, "pwm-off-delay-ms", &data->pwm_off_delay);
+
data->enable_gpio = -EINVAL;
return 0;
}
@@ -276,6 +293,8 @@ static int pwm_backlight_probe(struct platform_device *pdev)
pb->exit = data->exit;
pb->dev = &pdev->dev;
pb->enabled = false;
+ pb->post_pwm_on_delay = data->post_pwm_on_delay;
+ pb->pwm_off_delay = data->pwm_off_delay;
pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
GPIOD_ASIS);
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index e8afbd71a140..8ea265a022fd 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -14,6 +14,8 @@ struct platform_pwm_backlight_data {
unsigned int lth_brightness;
unsigned int pwm_period_ns;
unsigned int *levels;
+ unsigned int post_pwm_on_delay;
+ unsigned int pwm_off_delay;
/* TODO remove once all users are switched to gpiod_* API */
int enable_gpio;
int (*init)(struct device *dev);
--
2.16.2
On Mon 2018-03-26 11:52:09, 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)
Ok, but this changes behaviour for other panels, too. Are you sure you
are not breaking one of those?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
2018-03-26 12:42 GMT+02:00 Pavel Machek <[email protected]>:
> On Mon 2018-03-26 11:52:09, 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)
>
> Ok, but this changes behaviour for other panels, too. Are you sure you
> are not breaking one of those?
I can't say that I am 100% sure because I didn't find all the
datasheets of all the panels supported in the kernel. But all the
datasheets I checked specifies this sequence as valid. In general I
think that doesn't really matter, but I know that at least the
B116XTN02 panel requires enable first the PWM, wait 10ms and then
enable BL_EN to avoid garbage. So the other way around is not valid
for this panel. That's the reason for this patchset.
If anyone knows a panel that says the contrary, please let me know and
I'll try to rework the patches to satisfy both requirements.
Regards,
Enric
> Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Mar 26, 2018 at 11:52:10AM +0200, Enric Balletbo i Serra wrote:
> Hardware needs a delay between setting an initial (non-zero) PWM and
> enabling the backlight using GPIO. The post-pwm-on-delay-ms specifies
> this delay in milli seconds. Hardware also needs a delay between disabing
> the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-ms
> is this delay in milli seconds.
>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
> Acked-by: Thierry Reding <[email protected]>
Acked-by: Daniel Thompson <[email protected]>
> ---
> Based on the original Huang Lin <[email protected]> work.
> Changes since v5:
> - Add Acked-by: Thierry Reding <[email protected]>
> - CC DT mainling list (for some reason dropped at some point, my
> fault)
> Changes since v4:
> - Rebase on top of mainline.
> Changes since v3:
> - Replace us for ms.
> - Add Acked-by: Pavel Machek <[email protected]>
> Changes since v2:
> - Use separate properties (Rob Herring)
> Changes since v1:
> - As suggested by Daniel Thompson
> - Do not assume power-on delay and power-off delay will be the same
> ---
> Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86d441a..310810906613 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,10 @@ Optional properties:
> "pwms" property (see PWM binding[0])
> - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> and disables the backlight (see GPIO binding[1])
> + - post-pwm-on-delay-ms: Delay in ms between setting an initial (non-zero) PWM
> + and enabling the backlight using GPIO.
> + - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
> + and setting PWM value to 0.
>
> [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -32,4 +36,6 @@ Example:
>
> power-supply = <&vdd_bl_reg>;
> enable-gpios = <&gpio 58 0>;
> + post-pwm-on-delay-ms = <10>;
> + pwm-off-delay-ms = <10>;
> };
> --
> 2.16.2
>
On Mon, Mar 26, 2018 at 04:40:21PM +0200, Enric Balletbo Serra wrote:
> Hi Pavel,
>
> 2018-03-26 12:42 GMT+02:00 Pavel Machek <[email protected]>:
> > On Mon 2018-03-26 11:52:09, 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)
> >
> > Ok, but this changes behaviour for other panels, too. Are you sure you
> > are not breaking one of those?
>
> I can't say that I am 100% sure because I didn't find all the
> datasheets of all the panels supported in the kernel. But all the
> datasheets I checked specifies this sequence as valid. In general I
> think that doesn't really matter, but I know that at least the
> B116XTN02 panel requires enable first the PWM, wait 10ms and then
> enable BL_EN to avoid garbage. So the other way around is not valid
> for this panel. That's the reason for this patchset.
This is certainly a patch that could cause regressions... but it would
be a very odd panel that *likes* to be exposed to all the weird edges
that might occur whilst the PWM stablizes and a panel that *needs* to
see weird edges to work seems even less likely (since it could act
different with each SoC).
So whilst the patch is not absolutely cast iron guaranteed free of
risk, I think it is well enough argued for.
Naturally I may change my position *very* quickly on receipt of the
first bug report ;-)
Daniel.
On Mon, Mar 26, 2018 at 11:52:10AM +0200, Enric Balletbo i Serra wrote:
> Hardware needs a delay between setting an initial (non-zero) PWM and
> enabling the backlight using GPIO. The post-pwm-on-delay-ms specifies
> this delay in milli seconds. Hardware also needs a delay between disabing
> the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-ms
> is this delay in milli seconds.
>
> Signed-off-by: Enric Balletbo i Serra <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
> Acked-by: Thierry Reding <[email protected]>
> ---
> Based on the original Huang Lin <[email protected]> work.
> Changes since v5:
> - Add Acked-by: Thierry Reding <[email protected]>
> - CC DT mainling list (for some reason dropped at some point, my
> fault)
> Changes since v4:
> - Rebase on top of mainline.
> Changes since v3:
> - Replace us for ms.
> - Add Acked-by: Pavel Machek <[email protected]>
> Changes since v2:
> - Use separate properties (Rob Herring)
> Changes since v1:
> - As suggested by Daniel Thompson
> - Do not assume power-on delay and power-off delay will be the same
> ---
> Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Rob Herring <[email protected]>
On Mon, 26 Mar 2018, 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 <[email protected]>
> Acked-by: Daniel Thompson <[email protected]>
> Acked-by: Jingoo Han <[email protected]>
> Acked-by: Thierry Reding <[email protected]>
> ---
> Changes since v5:
> - Add Acked-by: Thierry Reding <[email protected]>
> 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(-)
Enric,
Please collect all of your Acks and submit a RESEND.
I will then apply the set.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog