[PATCH v3 1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle.

[PATCH v3 1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle.

[Enric Balletbo i Serra]

Before this patch the enable signal was set before the PWM signal and
vice-versa on power off. I guess that 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.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
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 002f1ce..909a686 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.9.3

[PATCH v3 1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle.

[Enric Balletbo i Serra]

Before this patch the enable signal was set before the PWM signal and
vice-versa on power off. I guess that 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.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
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 002f1ce..909a686 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.9.3

[PATCH v3 2/5] dt-bindings: pwm-backlight: add PWM delay proprieties.

[Enric Balletbo i Serra]

Hardware needs a delay between setting an initial (non-zero) PWM and
enabling the backlight using GPIO. The post-pwm-on-delay-us specifies
this delay in micro seconds. Hardware also needs a delay between disabing
the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-us
is this delay in micro seconds.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Based on the original Huang Lin <hl@rock-chips.com> work.

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 764db86..810527c 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-us: Delay in us between setting an initial (non-zero) PWM
+                          and enabling the backlight using GPIO.
+  - pwm-off-delay-us: Delay in us 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-us = <10000>;
+		pwm-off-delay-us = <10000>;
 	};
-- 
2.9.3

[PATCH v3 2/5] dt-bindings: pwm-backlight: add PWM delay proprieties.

[Enric Balletbo i Serra]

Hardware needs a delay between setting an initial (non-zero) PWM and
enabling the backlight using GPIO. The post-pwm-on-delay-us specifies
this delay in micro seconds. Hardware also needs a delay between disabing
the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-us
is this delay in micro seconds.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Based on the original Huang Lin <hl@rock-chips.com> work.

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 764db86..810527c 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-us: Delay in us between setting an initial (non-zero) PWM
+                          and enabling the backlight using GPIO.
+  - pwm-off-delay-us: Delay in us 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-us = <10000>;
+		pwm-off-delay-us = <10000>;
 	};
-- 
2.9.3

[PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Enric Balletbo i Serra]

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-us and pwm-off-delay-us proprieties to meet
the timings.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
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 | 17 +++++++++++++++++
 include/linux/pwm_backlight.h    |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 909a686..528155d 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,7 @@ struct pwm_bl_data {
 	struct gpio_desc	*enable_gpio;
 	unsigned int		scale;
 	bool			legacy;
+	unsigned int		pwm_delay[2];
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 
 	pwm_enable(pb->pwm);
 
+	if (pb->pwm_delay[0])
+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
+
 	if (pb->enable_gpio)
 		gpiod_set_value_cansleep(pb->enable_gpio, 1);
 
@@ -70,6 +75,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_delay[1])
+		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] * 2);
+
 	pwm_config(pb->pwm, 0, pb->period);
 	pwm_disable(pb->pwm);
 
@@ -175,6 +183,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-us",
+			     &data->pwm_delay[0]);
+	of_property_read_u32(node, "pwm-off-delay-us", &data->pwm_delay[1]);
+
 	data->enable_gpio = -EINVAL;
 	return 0;
 }
@@ -273,6 +289,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
+	memcpy(pb->pwm_delay, data->pwm_delay, sizeof(pb->pwm_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 efdd922..bf37131 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -13,6 +13,7 @@ struct platform_pwm_backlight_data {
 	unsigned int lth_brightness;
 	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	unsigned int pwm_delay[2];
 	/* TODO remove once all users are switched to gpiod_* API */
 	int enable_gpio;
 	int (*init)(struct device *dev);
-- 
2.9.3

[PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Enric Balletbo i Serra]

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-us and pwm-off-delay-us proprieties to meet
the timings.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
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 | 17 +++++++++++++++++
 include/linux/pwm_backlight.h    |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 909a686..528155d 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,7 @@ struct pwm_bl_data {
 	struct gpio_desc	*enable_gpio;
 	unsigned int		scale;
 	bool			legacy;
+	unsigned int		pwm_delay[2];
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 
 	pwm_enable(pb->pwm);
 
+	if (pb->pwm_delay[0])
+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
+
 	if (pb->enable_gpio)
 		gpiod_set_value_cansleep(pb->enable_gpio, 1);
 
@@ -70,6 +75,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_delay[1])
+		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] * 2);
+
 	pwm_config(pb->pwm, 0, pb->period);
 	pwm_disable(pb->pwm);
 
@@ -175,6 +183,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-us",
+			     &data->pwm_delay[0]);
+	of_property_read_u32(node, "pwm-off-delay-us", &data->pwm_delay[1]);
+
 	data->enable_gpio = -EINVAL;
 	return 0;
 }
@@ -273,6 +289,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
+	memcpy(pb->pwm_delay, data->pwm_delay, sizeof(pb->pwm_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 efdd922..bf37131 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -13,6 +13,7 @@ struct platform_pwm_backlight_data {
 	unsigned int lth_brightness;
 	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	unsigned int pwm_delay[2];
 	/* TODO remove once all users are switched to gpiod_* API */
 	int enable_gpio;
 	int (*init)(struct device *dev);
-- 
2.9.3

[PATCH v3 4/5] ARM: dts: rockchip: set PWM delay backlight settings for Veyron.

[Enric Balletbo i Serra]

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 <enric.balletbo@collabora.com>
---
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 d752a31..4cef71f 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-us = <10000>;
+		pwm-off-delay-us = <10000>;
 	};
 
 	gpio-charger {
-- 
2.9.3

[PATCH v3 4/5] ARM: dts: rockchip: set PWM delay backlight settings for Veyron.

[Enric Balletbo i Serra]

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 <enric.balletbo@collabora.com>
---
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 d752a31..4cef71f 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-us = <10000>;
+		pwm-off-delay-us = <10000>;
 	};
 
 	gpio-charger {
-- 
2.9.3

[PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Enric Balletbo i Serra]

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 <enric.balletbo@collabora.com>
---
Heiko,
 I'm not able to test this patch in a minnie device because I don't have
one, so could you do a quick try, please?

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 544de60..a5b7387 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-us = <20000>;
+	pwm-off-delay-us = <20000>;
 };
 
 &emmc {
-- 
2.9.3

[PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Enric Balletbo i Serra]

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 <enric.balletbo@collabora.com>
---
Heiko,
 I'm not able to test this patch in a minnie device because I don't have
one, so could you do a quick try, please?

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 544de60..a5b7387 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-us = <20000>;
+	pwm-off-delay-us = <20000>;
 };
 
 &emmc {
-- 
2.9.3

Re: [PATCH v3 1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle.

[Daniel Thompson]

On 17/07/17 22:28, Enric Balletbo i Serra wrote:
> Before this patch the enable signal was set before the PWM signal and
> vice-versa on power off. I guess that 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.

Could you list the part numbers for the panels you checked? Getting that 
in the git history would be really helpful for future archaeologists 
(including me).

Also whilst changing the header I'd also say that "I guess that" does 
not inspire much confidence. It sounds like you have done some homework 
here... surely you've moved past guess work!


Daniel.


> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 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 002f1ce..909a686 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;
>   }
> 

Re: [PATCH v3 1/5] pwm-backlight: enable/disable the PWM before/after LCD enable toggle.

[Daniel Thompson]

On 17/07/17 22:28, Enric Balletbo i Serra wrote:
> Before this patch the enable signal was set before the PWM signal and
> vice-versa on power off. I guess that 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.

Could you list the part numbers for the panels you checked? Getting that 
in the git history would be really helpful for future archaeologists 
(including me).

Also whilst changing the header I'd also say that "I guess that" does 
not inspire much confidence. It sounds like you have done some homework 
here... surely you've moved past guess work!


Daniel.


> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 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 002f1ce..909a686 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;
>   }
> 

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Daniel Thompson]

On 17/07/17 22:28, Enric Balletbo i Serra wrote:
> 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-us and pwm-off-delay-us proprieties to meet
> the timings.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 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 | 17 +++++++++++++++++
>   include/linux/pwm_backlight.h    |  1 +
>   2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 909a686..528155d 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,7 @@ struct pwm_bl_data {
>   	struct gpio_desc	*enable_gpio;
>   	unsigned int		scale;
>   	bool			legacy;
> +	unsigned int		pwm_delay[2];

Two named members would be better here (eliminating the "magic" 0 and 1).


>   	int			(*notify)(struct device *,
>   					  int brightness);
>   	void			(*notify_after)(struct device *,
> @@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>   
>   	pwm_enable(pb->pwm);
>   
> +	if (pb->pwm_delay[0])
> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> +
>   	if (pb->enable_gpio)
>   		gpiod_set_value_cansleep(pb->enable_gpio, 1);
>   
> @@ -70,6 +75,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_delay[1])
> +		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] * 2);
> +
>   	pwm_config(pb->pwm, 0, pb->period);
>   	pwm_disable(pb->pwm);
>   
> @@ -175,6 +183,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-us",
> +			     &data->pwm_delay[0]);
> +	of_property_read_u32(node, "pwm-off-delay-us", &data->pwm_delay[1]);
> +
>   	data->enable_gpio = -EINVAL;
>   	return 0;
>   }
> @@ -273,6 +289,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>   	pb->exit = data->exit;
>   	pb->dev = &pdev->dev;
>   	pb->enabled = false;
> +	memcpy(pb->pwm_delay, data->pwm_delay, sizeof(pb->pwm_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 efdd922..bf37131 100644
> --- a/include/linux/pwm_backlight.h
> +++ b/include/linux/pwm_backlight.h
> @@ -13,6 +13,7 @@ struct platform_pwm_backlight_data {
>   	unsigned int lth_brightness;
>   	unsigned int pwm_period_ns;
>   	unsigned int *levels;
> +	unsigned int pwm_delay[2];
>   	/* TODO remove once all users are switched to gpiod_* API */
>   	int enable_gpio;
>   	int (*init)(struct device *dev);
> 

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Daniel Thompson]

On 17/07/17 22:28, Enric Balletbo i Serra wrote:
> 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-us and pwm-off-delay-us proprieties to meet
> the timings.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 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 | 17 +++++++++++++++++
>   include/linux/pwm_backlight.h    |  1 +
>   2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 909a686..528155d 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,7 @@ struct pwm_bl_data {
>   	struct gpio_desc	*enable_gpio;
>   	unsigned int		scale;
>   	bool			legacy;
> +	unsigned int		pwm_delay[2];

Two named members would be better here (eliminating the "magic" 0 and 1).


>   	int			(*notify)(struct device *,
>   					  int brightness);
>   	void			(*notify_after)(struct device *,
> @@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>   
>   	pwm_enable(pb->pwm);
>   
> +	if (pb->pwm_delay[0])
> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> +
>   	if (pb->enable_gpio)
>   		gpiod_set_value_cansleep(pb->enable_gpio, 1);
>   
> @@ -70,6 +75,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_delay[1])
> +		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] * 2);
> +
>   	pwm_config(pb->pwm, 0, pb->period);
>   	pwm_disable(pb->pwm);
>   
> @@ -175,6 +183,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-us",
> +			     &data->pwm_delay[0]);
> +	of_property_read_u32(node, "pwm-off-delay-us", &data->pwm_delay[1]);
> +
>   	data->enable_gpio = -EINVAL;
>   	return 0;
>   }
> @@ -273,6 +289,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>   	pb->exit = data->exit;
>   	pb->dev = &pdev->dev;
>   	pb->enabled = false;
> +	memcpy(pb->pwm_delay, data->pwm_delay, sizeof(pb->pwm_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 efdd922..bf37131 100644
> --- a/include/linux/pwm_backlight.h
> +++ b/include/linux/pwm_backlight.h
> @@ -13,6 +13,7 @@ struct platform_pwm_backlight_data {
>   	unsigned int lth_brightness;
>   	unsigned int pwm_period_ns;
>   	unsigned int *levels;
> +	unsigned int pwm_delay[2];
>   	/* TODO remove once all users are switched to gpiod_* API */
>   	int enable_gpio;
>   	int (*init)(struct device *dev);
> 

Re: [PATCH v3 2/5] dt-bindings: pwm-backlight: add PWM delay proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Mon 2017-07-17 23:28:08, 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-us specifies
> this delay in micro seconds. Hardware also needs a delay between disabing
> the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-us
> is this delay in micro seconds.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 2/5] dt-bindings: pwm-backlight: add PWM delay proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Mon 2017-07-17 23:28:08, 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-us specifies
> this delay in micro seconds. Hardware also needs a delay between disabing
> the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-us
> is this delay in micro seconds.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Hi!

> >--- 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,7 @@ struct pwm_bl_data {
> >  	struct gpio_desc	*enable_gpio;
> >  	unsigned int		scale;
> >  	bool			legacy;
> >+	unsigned int		pwm_delay[2];
> 
> Two named members would be better here (eliminating the "magic" 0
>and 1).

My thought, too.

> >@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >  	pwm_enable(pb->pwm);
> >+	if (pb->pwm_delay[0])
> >+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);

Plus I'd just do the delay unconditionally :-).

Best regards,
								Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Hi!

> >--- 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,7 @@ struct pwm_bl_data {
> >  	struct gpio_desc	*enable_gpio;
> >  	unsigned int		scale;
> >  	bool			legacy;
> >+	unsigned int		pwm_delay[2];
> 
> Two named members would be better here (eliminating the "magic" 0
>and 1).

My thought, too.

> >@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >  	pwm_enable(pb->pwm);
> >+	if (pb->pwm_delay[0])
> >+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);

Plus I'd just do the delay unconditionally :-).

Best regards,
								Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
> 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.

Wait a wait a moment!

> --- 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-us = <20000>;

-us = <20 000>;

This is not 200 msec.

Plus, it is quite anti-social to do udelay(200 000).

Plus, it is very anti-socifal to use udelay_range(200msec,
400msec).

Whoever told you udelay_range is good thing to use -- it is not, and
it is certainly not worth making user wait 200msec more!

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
> 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.

Wait a wait a moment!

> --- 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-us = <20000>;

-us = <20 000>;

This is not 200 msec.

Plus, it is quite anti-social to do udelay(200 000).

Plus, it is very anti-socifal to use udelay_range(200msec,
400msec).

Whoever told you udelay_range is good thing to use -- it is not, and
it is certainly not worth making user wait 200msec more!

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Enric Balletbo Serra]

Pavel,

2017-07-20 10:10 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
>> 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.
>
> Wait a wait a moment!
>
>> --- 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-us = <20000>;
>
> -us = <20 000>;
>
> This is not 200 msec.
>

Oops, good catch.

> Plus, it is quite anti-social to do udelay(200 000).
>
> Plus, it is very anti-socifal to use udelay_range(200msec,
> 400msec).
>
> Whoever told you udelay_range is good thing to use -- it is not, and
> it is certainly not worth making user wait 200msec more!
>

Checked again some datasheets and seems that or not require a delay or
the delays are 10ms+, so according to [1] I'll use msleep instead.

[1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Thanks,
 Enric

>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Enric Balletbo Serra]

Pavel,

2017-07-20 10:10 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
>> 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.
>
> Wait a wait a moment!
>
>> --- 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-us = <20000>;
>
> -us = <20 000>;
>
> This is not 200 msec.
>

Oops, good catch.

> Plus, it is quite anti-social to do udelay(200 000).
>
> Plus, it is very anti-socifal to use udelay_range(200msec,
> 400msec).
>
> Whoever told you udelay_range is good thing to use -- it is not, and
> it is certainly not worth making user wait 200msec more!
>

Checked again some datasheets and seems that or not require a delay or
the delays are 10ms+, so according to [1] I'll use msleep instead.

[1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Thanks,
 Enric

>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

Hi!

> 2017-07-20 10:10 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> > On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
> >> 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.
> >
> > Wait a wait a moment!
> >
> >> --- 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-us = <20000>;
> >
> > -us = <20 000>;
> >
> > This is not 200 msec.
> >
> 
> Oops, good catch.
> 
> > Plus, it is quite anti-social to do udelay(200 000).
> >
> > Plus, it is very anti-socifal to use udelay_range(200msec,
> > 400msec).
> >
> > Whoever told you udelay_range is good thing to use -- it is not, and
> > it is certainly not worth making user wait 200msec more!
> >
> 
> Checked again some datasheets and seems that or not require a delay or
> the delays are 10ms+, so according to [1] I'll use msleep instead.
> 
> [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Ok, msleep makes sense.

But in such case, you probably want to specify delays in the
miliseconds in the device tree... or at least carefully round the
values up.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

Hi!

> 2017-07-20 10:10 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> > On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
> >> 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.
> >
> > Wait a wait a moment!
> >
> >> --- 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-us = <20000>;
> >
> > -us = <20 000>;
> >
> > This is not 200 msec.
> >
> 
> Oops, good catch.
> 
> > Plus, it is quite anti-social to do udelay(200 000).
> >
> > Plus, it is very anti-socifal to use udelay_range(200msec,
> > 400msec).
> >
> > Whoever told you udelay_range is good thing to use -- it is not, and
> > it is certainly not worth making user wait 200msec more!
> >
> 
> Checked again some datasheets and seems that or not require a delay or
> the delays are 10ms+, so according to [1] I'll use msleep instead.
> 
> [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Ok, msleep makes sense.

But in such case, you probably want to specify delays in the
miliseconds in the device tree... or at least carefully round the
values up.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Daniel Thompson]

On 20/07/17 09:06, Pavel Machek wrote:
> Hi!
> 
>>> --- 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,7 @@ struct pwm_bl_data {
>>>   	struct gpio_desc	*enable_gpio;
>>>   	unsigned int		scale;
>>>   	bool			legacy;
>>> +	unsigned int		pwm_delay[2];
>>
>> Two named members would be better here (eliminating the "magic" 0
>> and 1).
> 
> My thought, too.
> 
>>> @@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>>>   	pwm_enable(pb->pwm);
>>> +	if (pb->pwm_delay[0])
>>> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> 
> Plus I'd just do the delay unconditionally :-).

... does this still apply if this code is switched to msleep()?

msleep() has no wait avoidance[1] and if lots of drivers are reckless 
about sleeping for 10ms it soon starts to show up in the boot time 
(especially optimized ones).


Daniel.


[1] As it happens I can't see that many early bail out paths in
     usleep_range() either.

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Daniel Thompson]

On 20/07/17 09:06, Pavel Machek wrote:
> Hi!
> 
>>> --- 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,7 @@ struct pwm_bl_data {
>>>   	struct gpio_desc	*enable_gpio;
>>>   	unsigned int		scale;
>>>   	bool			legacy;
>>> +	unsigned int		pwm_delay[2];
>>
>> Two named members would be better here (eliminating the "magic" 0
>> and 1).
> 
> My thought, too.
> 
>>> @@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>>>   	pwm_enable(pb->pwm);
>>> +	if (pb->pwm_delay[0])
>>> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> 
> Plus I'd just do the delay unconditionally :-).

... does this still apply if this code is switched to msleep()?

msleep() has no wait avoidance[1] and if lots of drivers are reckless 
about sleeping for 10ms it soon starts to show up in the boot time 
(especially optimized ones).


Daniel.


[1] As it happens I can't see that many early bail out paths in
     usleep_range() either.

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Thu 2017-07-20 11:37:17, Daniel Thompson wrote:
> On 20/07/17 09:06, Pavel Machek wrote:
> >Hi!
> >
> >>>--- 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,7 @@ struct pwm_bl_data {
> >>>  	struct gpio_desc	*enable_gpio;
> >>>  	unsigned int		scale;
> >>>  	bool			legacy;
> >>>+	unsigned int		pwm_delay[2];
> >>
> >>Two named members would be better here (eliminating the "magic" 0
> >>and 1).
> >
> >My thought, too.
> >
> >>>@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >>>  	pwm_enable(pb->pwm);
> >>>+	if (pb->pwm_delay[0])
> >>>+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> >
> >Plus I'd just do the delay unconditionally :-).
> 
> ... does this still apply if this code is switched to msleep()?
> 
> msleep() has no wait avoidance[1] and if lots of drivers are reckless about
> sleeping for 10ms it soon starts to show up in the boot time (especially
> optimized ones).

No, not for msleep.

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Thu 2017-07-20 11:37:17, Daniel Thompson wrote:
> On 20/07/17 09:06, Pavel Machek wrote:
> >Hi!
> >
> >>>--- 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,7 @@ struct pwm_bl_data {
> >>>  	struct gpio_desc	*enable_gpio;
> >>>  	unsigned int		scale;
> >>>  	bool			legacy;
> >>>+	unsigned int		pwm_delay[2];
> >>
> >>Two named members would be better here (eliminating the "magic" 0
> >>and 1).
> >
> >My thought, too.
> >
> >>>@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >>>  	pwm_enable(pb->pwm);
> >>>+	if (pb->pwm_delay[0])
> >>>+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> >
> >Plus I'd just do the delay unconditionally :-).
> 
> ... does this still apply if this code is switched to msleep()?
> 
> msleep() has no wait avoidance[1] and if lots of drivers are reckless about
> sleeping for 10ms it soon starts to show up in the boot time (especially
> optimized ones).

No, not for msleep.

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On Thu 2017-07-20 11:37:17, Daniel Thompson wrote:
> On 20/07/17 09:06, Pavel Machek wrote:
> >Hi!
> >
> >>>--- 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,7 @@ struct pwm_bl_data {
> >>>  	struct gpio_desc	*enable_gpio;
> >>>  	unsigned int		scale;
> >>>  	bool			legacy;
> >>>+	unsigned int		pwm_delay[2];
> >>
> >>Two named members would be better here (eliminating the "magic" 0
> >>and 1).
> >
> >My thought, too.
> >
> >>>@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >>>  	pwm_enable(pb->pwm);
> >>>+	if (pb->pwm_delay[0])
> >>>+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> >
> >Plus I'd just do the delay unconditionally :-).
> 
> ... does this still apply if this code is switched to msleep()?

No.

> msleep() has no wait avoidance[1] and if lots of drivers are reckless about
> sleeping for 10ms it soon starts to show up in the boot time (especially
> optimized ones).
...
> [1] As it happens I can't see that many early bail out paths in
>     usleep_range() either.

Well, in usleep_range(1,2) should be fast enough operation, and
usleep_range(0,0) should be similar to usleep_range(1,2) at worst :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/5] pwm-backlight: add support for PWM delays proprieties.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On Thu 2017-07-20 11:37:17, Daniel Thompson wrote:
> On 20/07/17 09:06, Pavel Machek wrote:
> >Hi!
> >
> >>>--- 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,7 @@ struct pwm_bl_data {
> >>>  	struct gpio_desc	*enable_gpio;
> >>>  	unsigned int		scale;
> >>>  	bool			legacy;
> >>>+	unsigned int		pwm_delay[2];
> >>
> >>Two named members would be better here (eliminating the "magic" 0
> >>and 1).
> >
> >My thought, too.
> >
> >>>@@ -56,6 +58,9 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >>>  	pwm_enable(pb->pwm);
> >>>+	if (pb->pwm_delay[0])
> >>>+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> >
> >Plus I'd just do the delay unconditionally :-).
> 
> ... does this still apply if this code is switched to msleep()?

No.

> msleep() has no wait avoidance[1] and if lots of drivers are reckless about
> sleeping for 10ms it soon starts to show up in the boot time (especially
> optimized ones).
...
> [1] As it happens I can't see that many early bail out paths in
>     usleep_range() either.

Well, in usleep_range(1,2) should be fast enough operation, and
usleep_range(0,0) should be similar to usleep_range(1,2) at worst :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]