* [PATCH 0/4] Various fixes for the lm3630a backlight driver
@ 2024-02-19 23:11 Luca Weiss
2024-02-19 23:11 ` [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init Luca Weiss
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Luca Weiss @ 2024-02-19 23:11 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maximilian Weigand
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree,
Luca Weiss
On the MSM8974 Nexus 5 and OnePlus One phones (latter doesn't have
display upstream) the display backlight was turning off whenever you
would write a brightness to sysfs since a recent commit to the driver
(kernel v6.5).
backlight: lm3630a: Turn off both led strings when display is blank
Turns out, backlight_is_blank() thought the display was blanked because
the props variable is was checking was never actually initialized so it
was just reading some value that was left before.
The first commit in this series fixes this, and the others are some
cleanups / fixes I noticed while working on this.
As last commit, we can finally hook up the panel and backlight on the
Nexus 5 so blanking the screen actually turns off the backlight.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Luca Weiss (4):
backlight: lm3630a: Initialize backlight_properties on init
backlight: lm3630a: Don't set bl->props.brightness in get_brightness
backlight: lm3630a: Use backlight_get_brightness helper in update_status
ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
.../qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 ++-
drivers/video/backlight/lm3630a_bl.c | 29 ++++++++++------------
2 files changed, 16 insertions(+), 17 deletions(-)
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20240219-lm3630a-fixups-8a9359e5a8ce
Best regards,
--
Luca Weiss <luca@z3ntu.xyz>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
@ 2024-02-19 23:11 ` Luca Weiss
2024-02-20 14:00 ` Daniel Thompson
2024-02-20 14:07 ` Konrad Dybcio
2024-02-19 23:11 ` [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness Luca Weiss
` (4 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Luca Weiss @ 2024-02-19 23:11 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maximilian Weigand
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree,
Luca Weiss
The backlight_properties struct should be initialized to zero before
using, otherwise there will be some random values in the struct.
Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
drivers/video/backlight/lm3630a_bl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index a3412c936ca2..8e275275b808 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -343,6 +343,7 @@ static int lm3630a_backlight_register(struct lm3630a_chip *pchip)
struct backlight_properties props;
const char *label;
+ memset(&props, 0, sizeof(struct backlight_properties));
props.type = BACKLIGHT_RAW;
if (pdata->leda_ctrl != LM3630A_LEDA_DISABLE) {
props.brightness = pdata->leda_init_brt;
--
2.43.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
2024-02-19 23:11 ` [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init Luca Weiss
@ 2024-02-19 23:11 ` Luca Weiss
2024-02-20 14:03 ` Daniel Thompson
2024-02-19 23:11 ` [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status Luca Weiss
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Luca Weiss @ 2024-02-19 23:11 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maximilian Weigand
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree,
Luca Weiss
There's no need to set bl->props.brightness, the get_brightness function
is just supposed to return the current brightness and not touch the
struct.
With that done we can also remove the 'goto out' and just return the
value.
Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
drivers/video/backlight/lm3630a_bl.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 8e275275b808..26ff4178cc16 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -233,7 +233,7 @@ static int lm3630a_bank_a_get_brightness(struct backlight_device *bl)
if (rval < 0)
goto out_i2c_err;
brightness |= rval;
- goto out;
+ return brightness;
}
/* disable sleep */
@@ -244,11 +244,8 @@ static int lm3630a_bank_a_get_brightness(struct backlight_device *bl)
rval = lm3630a_read(pchip, REG_BRT_A);
if (rval < 0)
goto out_i2c_err;
- brightness = rval;
+ return rval;
-out:
- bl->props.brightness = brightness;
- return bl->props.brightness;
out_i2c_err:
dev_err(pchip->dev, "i2c failed to access register\n");
return 0;
@@ -310,7 +307,7 @@ static int lm3630a_bank_b_get_brightness(struct backlight_device *bl)
if (rval < 0)
goto out_i2c_err;
brightness |= rval;
- goto out;
+ return brightness;
}
/* disable sleep */
@@ -321,11 +318,8 @@ static int lm3630a_bank_b_get_brightness(struct backlight_device *bl)
rval = lm3630a_read(pchip, REG_BRT_B);
if (rval < 0)
goto out_i2c_err;
- brightness = rval;
+ return rval;
-out:
- bl->props.brightness = brightness;
- return bl->props.brightness;
out_i2c_err:
dev_err(pchip->dev, "i2c failed to access register\n");
return 0;
--
2.43.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
2024-02-19 23:11 ` [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init Luca Weiss
2024-02-19 23:11 ` [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness Luca Weiss
@ 2024-02-19 23:11 ` Luca Weiss
2024-02-20 14:11 ` Daniel Thompson
2024-02-19 23:11 ` [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight Luca Weiss
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Luca Weiss @ 2024-02-19 23:11 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maximilian Weigand
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree,
Luca Weiss
As per documentation "drivers are expected to use this function in their
update_status() operation to get the brightness value.".
With this we can also drop the manual backlight_is_blank() handling
since backlight_get_brightness() is already handling this correctly.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
drivers/video/backlight/lm3630a_bl.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 26ff4178cc16..e6c0916ec88b 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -189,10 +189,11 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
int ret;
struct lm3630a_chip *pchip = bl_get_data(bl);
enum lm3630a_pwm_ctrl pwm_ctrl = pchip->pdata->pwm_ctrl;
+ int brightness = backlight_get_brightness(bl);
/* pwm control */
if ((pwm_ctrl & LM3630A_PWM_BANK_A) != 0)
- return lm3630a_pwm_ctrl(pchip, bl->props.brightness,
+ return lm3630a_pwm_ctrl(pchip, brightness,
bl->props.max_brightness);
/* disable sleep */
@@ -201,9 +202,9 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
goto out_i2c_err;
usleep_range(1000, 2000);
/* minimum brightness is 0x04 */
- ret = lm3630a_write(pchip, REG_BRT_A, bl->props.brightness);
+ ret = lm3630a_write(pchip, REG_BRT_A, brightness);
- if (backlight_is_blank(bl) || (backlight_get_brightness(bl) < 0x4))
+ if (brightness < 0x4)
/* turn the string off */
ret |= lm3630a_update(pchip, REG_CTRL, LM3630A_LEDA_ENABLE, 0);
else
@@ -263,10 +264,11 @@ static int lm3630a_bank_b_update_status(struct backlight_device *bl)
int ret;
struct lm3630a_chip *pchip = bl_get_data(bl);
enum lm3630a_pwm_ctrl pwm_ctrl = pchip->pdata->pwm_ctrl;
+ int brightness = backlight_get_brightness(bl);
/* pwm control */
if ((pwm_ctrl & LM3630A_PWM_BANK_B) != 0)
- return lm3630a_pwm_ctrl(pchip, bl->props.brightness,
+ return lm3630a_pwm_ctrl(pchip, brightness,
bl->props.max_brightness);
/* disable sleep */
@@ -275,9 +277,9 @@ static int lm3630a_bank_b_update_status(struct backlight_device *bl)
goto out_i2c_err;
usleep_range(1000, 2000);
/* minimum brightness is 0x04 */
- ret = lm3630a_write(pchip, REG_BRT_B, bl->props.brightness);
+ ret = lm3630a_write(pchip, REG_BRT_B, brightness);
- if (backlight_is_blank(bl) || (backlight_get_brightness(bl) < 0x4))
+ if (brightness < 0x4)
/* turn the string off */
ret |= lm3630a_update(pchip, REG_CTRL, LM3630A_LEDB_ENABLE, 0);
else
--
2.43.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
` (2 preceding siblings ...)
2024-02-19 23:11 ` [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status Luca Weiss
@ 2024-02-19 23:11 ` Luca Weiss
2024-02-20 14:12 ` Daniel Thompson
2024-02-23 17:02 ` (subset) [PATCH 0/4] Various fixes for the lm3630a backlight driver Lee Jones
2024-05-28 3:32 ` Bjorn Andersson
5 siblings, 1 reply; 17+ messages in thread
From: Luca Weiss @ 2024-02-19 23:11 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maximilian Weigand
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree,
Luca Weiss
Connect the panel with the backlight nodes so that the backlight can be
turned off when the display is blanked.
Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
index 4aaae8537a3f..8eaa5b162815 100644
--- a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -182,7 +182,7 @@ &blsp2_i2c5 {
status = "okay";
clock-frequency = <355000>;
- led-controller@38 {
+ backlight: led-controller@38 {
compatible = "ti,lm3630a";
status = "okay";
reg = <0x38>;
@@ -272,6 +272,8 @@ panel: panel@0 {
reg = <0>;
compatible = "lg,acx467akm-7";
+ backlight = <&backlight>;
+
pinctrl-names = "default";
pinctrl-0 = <&panel_pin>;
--
2.43.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init
2024-02-19 23:11 ` [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init Luca Weiss
@ 2024-02-20 14:00 ` Daniel Thompson
2024-02-20 14:07 ` Konrad Dybcio
1 sibling, 0 replies; 17+ messages in thread
From: Daniel Thompson @ 2024-02-20 14:00 UTC (permalink / raw
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 12:11:19AM +0100, Luca Weiss wrote:
> The backlight_properties struct should be initialized to zero before
> using, otherwise there will be some random values in the struct.
>
> Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness
2024-02-19 23:11 ` [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness Luca Weiss
@ 2024-02-20 14:03 ` Daniel Thompson
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Thompson @ 2024-02-20 14:03 UTC (permalink / raw
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 12:11:20AM +0100, Luca Weiss wrote:
> There's no need to set bl->props.brightness, the get_brightness function
> is just supposed to return the current brightness and not touch the
> struct.
>
> With that done we can also remove the 'goto out' and just return the
> value.
>
> Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init
2024-02-19 23:11 ` [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init Luca Weiss
2024-02-20 14:00 ` Daniel Thompson
@ 2024-02-20 14:07 ` Konrad Dybcio
2024-02-20 15:41 ` Daniel Thompson
1 sibling, 1 reply; 17+ messages in thread
From: Konrad Dybcio @ 2024-02-20 14:07 UTC (permalink / raw
To: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Maximilian Weigand
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree
On 20.02.2024 00:11, Luca Weiss wrote:
> The backlight_properties struct should be initialized to zero before
> using, otherwise there will be some random values in the struct.
>
> Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
> drivers/video/backlight/lm3630a_bl.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index a3412c936ca2..8e275275b808 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -343,6 +343,7 @@ static int lm3630a_backlight_register(struct lm3630a_chip *pchip)
> struct backlight_properties props;
> const char *label;
>
> + memset(&props, 0, sizeof(struct backlight_properties));
You can zero-initialize it instead
Konrad
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
2024-02-19 23:11 ` [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status Luca Weiss
@ 2024-02-20 14:11 ` Daniel Thompson
2024-02-20 16:43 ` Luca Weiss
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Thompson @ 2024-02-20 14:11 UTC (permalink / raw
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 12:11:21AM +0100, Luca Weiss wrote:
> As per documentation "drivers are expected to use this function in their
> update_status() operation to get the brightness value.".
>
> With this we can also drop the manual backlight_is_blank() handling
> since backlight_get_brightness() is already handling this correctly.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
However...
> ---
> /* disable sleep */
> @@ -201,9 +202,9 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
> goto out_i2c_err;
> usleep_range(1000, 2000);
> /* minimum brightness is 0x04 */
> - ret = lm3630a_write(pchip, REG_BRT_A, bl->props.brightness);
> + ret = lm3630a_write(pchip, REG_BRT_A, brightness);
... then handling of the minimum brightness looks weird in this driver.
The range of the backlight is 0..max_brightness. Sadly the drivers
are inconsistant regarding whether zero means off or just minimum,
however three certainly isn't supposed to mean off! In other words the
offsetting should be handled by driver rather than hoping userspace has
some magic LM3630A mode.
You didn't introduce this so this patch still has my R-b ...
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
2024-02-19 23:11 ` [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight Luca Weiss
@ 2024-02-20 14:12 ` Daniel Thompson
2024-02-20 16:45 ` Luca Weiss
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Thompson @ 2024-02-20 14:12 UTC (permalink / raw
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 12:11:22AM +0100, Luca Weiss wrote:
> Connect the panel with the backlight nodes so that the backlight can be
> turned off when the display is blanked.
>
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
> arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> index 4aaae8537a3f..8eaa5b162815 100644
> --- a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> +++ b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> @@ -182,7 +182,7 @@ &blsp2_i2c5 {
> status = "okay";
> clock-frequency = <355000>;
>
> - led-controller@38 {
> + backlight: led-controller@38 {
Again... a minor nit regarding existing problems but this node doesn't
follow the generic naming recommendations:
https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#generic-names-recommendation
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init
2024-02-20 14:07 ` Konrad Dybcio
@ 2024-02-20 15:41 ` Daniel Thompson
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Thompson @ 2024-02-20 15:41 UTC (permalink / raw
To: Konrad Dybcio
Cc: Luca Weiss, ~postmarketos/upstreaming, phone-devel, Lee Jones,
Jingoo Han, Helge Deller, Andrew Morton, G.Shark Jeong,
Bjorn Andersson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 03:07:54PM +0100, Konrad Dybcio wrote:
> On 20.02.2024 00:11, Luca Weiss wrote:
> > The backlight_properties struct should be initialized to zero before
> > using, otherwise there will be some random values in the struct.
> >
> > Fixes: 0c2a665a648e ("backlight: add Backlight driver for lm3630 chip")
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> > drivers/video/backlight/lm3630a_bl.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> > index a3412c936ca2..8e275275b808 100644
> > --- a/drivers/video/backlight/lm3630a_bl.c
> > +++ b/drivers/video/backlight/lm3630a_bl.c
> > @@ -343,6 +343,7 @@ static int lm3630a_backlight_register(struct lm3630a_chip *pchip)
> > struct backlight_properties props;
> > const char *label;
> >
> > + memset(&props, 0, sizeof(struct backlight_properties));
>
> You can zero-initialize it instead
I don't object to either approach but memset() dominates backlight
implementations currently.
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
2024-02-20 14:11 ` Daniel Thompson
@ 2024-02-20 16:43 ` Luca Weiss
2024-02-21 11:20 ` Daniel Thompson
0 siblings, 1 reply; 17+ messages in thread
From: Luca Weiss @ 2024-02-20 16:43 UTC (permalink / raw
To: Daniel Thompson
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Dienstag, 20. Februar 2024 15:11:07 CET Daniel Thompson wrote:
> On Tue, Feb 20, 2024 at 12:11:21AM +0100, Luca Weiss wrote:
> > As per documentation "drivers are expected to use this function in their
> > update_status() operation to get the brightness value.".
> >
> > With this we can also drop the manual backlight_is_blank() handling
> > since backlight_get_brightness() is already handling this correctly.
> >
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>
> However...
>
> > ---
> > /* disable sleep */
> > @@ -201,9 +202,9 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
> > goto out_i2c_err;
> > usleep_range(1000, 2000);
> > /* minimum brightness is 0x04 */
> > - ret = lm3630a_write(pchip, REG_BRT_A, bl->props.brightness);
> > + ret = lm3630a_write(pchip, REG_BRT_A, brightness);
>
> ... then handling of the minimum brightness looks weird in this driver.
>
> The range of the backlight is 0..max_brightness. Sadly the drivers
> are inconsistant regarding whether zero means off or just minimum,
> however three certainly isn't supposed to mean off! In other words the
> offsetting should be handled by driver rather than hoping userspace has
> some magic LM3630A mode.
I could also try and fix that..
1. Treat 1..4 as 4, so have backlight on at that minimum level? Probably
wouldn't be noticable that brightness 1=2=3=4. And the backlight will be
on compared to off as it is now.
2. Decrease max_brightness by 4 values, so probably 0..251 and shift the
values up in the driver so we get 4..255?
Or would you have some other idea here?
Regards
Luca
>
> You didn't introduce this so this patch still has my R-b ...
>
>
> Daniel.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
2024-02-20 14:12 ` Daniel Thompson
@ 2024-02-20 16:45 ` Luca Weiss
2024-02-20 17:07 ` Daniel Thompson
0 siblings, 1 reply; 17+ messages in thread
From: Luca Weiss @ 2024-02-20 16:45 UTC (permalink / raw
To: Daniel Thompson
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Dienstag, 20. Februar 2024 15:12:10 CET Daniel Thompson wrote:
> On Tue, Feb 20, 2024 at 12:11:22AM +0100, Luca Weiss wrote:
> > Connect the panel with the backlight nodes so that the backlight can be
> > turned off when the display is blanked.
> >
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>
>
> > ---
> > arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > index 4aaae8537a3f..8eaa5b162815 100644
> > --- a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > +++ b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > @@ -182,7 +182,7 @@ &blsp2_i2c5 {
> > status = "okay";
> > clock-frequency = <355000>;
> >
> > - led-controller@38 {
> > + backlight: led-controller@38 {
>
> Again... a minor nit regarding existing problems but this node doesn't
> follow the generic naming recommendations:
> https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#generic-names-recommendation
"led-controller" is listed on that page, or do you mean something else?
>
>
> Daniel.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
2024-02-20 16:45 ` Luca Weiss
@ 2024-02-20 17:07 ` Daniel Thompson
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Thompson @ 2024-02-20 17:07 UTC (permalink / raw
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 05:45:32PM +0100, Luca Weiss wrote:
> On Dienstag, 20. Februar 2024 15:12:10 CET Daniel Thompson wrote:
> > On Tue, Feb 20, 2024 at 12:11:22AM +0100, Luca Weiss wrote:
> > > Connect the panel with the backlight nodes so that the backlight can be
> > > turned off when the display is blanked.
> > >
> > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> >
> > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> >
> >
> > > ---
> > > arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > index 4aaae8537a3f..8eaa5b162815 100644
> > > --- a/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > +++ b/arch/arm/boot/dts/qcom/qcom-msm8974-lge-nexus5-hammerhead.dts
> > > @@ -182,7 +182,7 @@ &blsp2_i2c5 {
> > > status = "okay";
> > > clock-frequency = <355000>;
> > >
> > > - led-controller@38 {
> > > + backlight: led-controller@38 {
> >
> > Again... a minor nit regarding existing problems but this node doesn't
> > follow the generic naming recommendations:
> > https://devicetree-specification.readthedocs.io/en/stable/devicetree-basics.html#generic-names-recommendation
>
> "led-controller" is listed on that page, or do you mean something else?
That's the point ;-). It is supposed to be called backlight@38!
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
2024-02-20 16:43 ` Luca Weiss
@ 2024-02-21 11:20 ` Daniel Thompson
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Thompson @ 2024-02-21 11:20 UTC (permalink / raw
To: Luca Weiss
Cc: ~postmarketos/upstreaming, phone-devel, Lee Jones, Jingoo Han,
Helge Deller, Andrew Morton, G.Shark Jeong, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Maximilian Weigand, dri-devel, linux-fbdev, linux-kernel,
linux-arm-msm, devicetree
On Tue, Feb 20, 2024 at 05:43:32PM +0100, Luca Weiss wrote:
> On Dienstag, 20. Februar 2024 15:11:07 CET Daniel Thompson wrote:
> > On Tue, Feb 20, 2024 at 12:11:21AM +0100, Luca Weiss wrote:
> > > As per documentation "drivers are expected to use this function in their
> > > update_status() operation to get the brightness value.".
> > >
> > > With this we can also drop the manual backlight_is_blank() handling
> > > since backlight_get_brightness() is already handling this correctly.
> > >
> > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> >
> > Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> >
> > However...
> >
> > > ---
> > > /* disable sleep */
> > > @@ -201,9 +202,9 @@ static int lm3630a_bank_a_update_status(struct backlight_device *bl)
> > > goto out_i2c_err;
> > > usleep_range(1000, 2000);
> > > /* minimum brightness is 0x04 */
> > > - ret = lm3630a_write(pchip, REG_BRT_A, bl->props.brightness);
> > > + ret = lm3630a_write(pchip, REG_BRT_A, brightness);
> >
> > ... then handling of the minimum brightness looks weird in this driver.
> >
> > The range of the backlight is 0..max_brightness. Sadly the drivers
> > are inconsistant regarding whether zero means off or just minimum,
> > however three certainly isn't supposed to mean off! In other words the
> > offsetting should be handled by driver rather than hoping userspace has
> > some magic LM3630A mode.
>
> I could also try and fix that..
>
> 1. Treat 1..4 as 4, so have backlight on at that minimum level? Probably
> wouldn't be noticable that brightness 1=2=3=4. And the backlight will be
> on compared to off as it is now.
>
> 2. Decrease max_brightness by 4 values, so probably 0..251 and shift the
> values up in the driver so we get 4..255?
>
> Or would you have some other idea here?
I think #2 is the right option but shouldn't it be decrease max_brightness
by 3, yielding 0..252 .
Only nitpicking on that because, given how old this driver is I'd like
to be conservative. I don't expect there to be userspaces with magic
LM3630A modes but there could be some that assume #0 is off! Hence I
wanted to make sure we are on the same page.
Daniel.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: (subset) [PATCH 0/4] Various fixes for the lm3630a backlight driver
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
` (3 preceding siblings ...)
2024-02-19 23:11 ` [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight Luca Weiss
@ 2024-02-23 17:02 ` Lee Jones
2024-05-28 3:32 ` Bjorn Andersson
5 siblings, 0 replies; 17+ messages in thread
From: Lee Jones @ 2024-02-23 17:02 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maximilian Weigand, Luca Weiss
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree
On Tue, 20 Feb 2024 00:11:18 +0100, Luca Weiss wrote:
> On the MSM8974 Nexus 5 and OnePlus One phones (latter doesn't have
> display upstream) the display backlight was turning off whenever you
> would write a brightness to sysfs since a recent commit to the driver
> (kernel v6.5).
>
> backlight: lm3630a: Turn off both led strings when display is blank
>
> [...]
Applied, thanks!
[1/4] backlight: lm3630a: Initialize backlight_properties on init
commit: 4602c7615989e6e7052e317995a66014eb318082
[2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness
commit: ebb3b9a65b56e9b21841ab9a15b946407cd6b104
[3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status
commit: 3c40590fafd4cc2447fb482a640c450e1a58ffa1
--
Lee Jones [李琼斯]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: (subset) [PATCH 0/4] Various fixes for the lm3630a backlight driver
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
` (4 preceding siblings ...)
2024-02-23 17:02 ` (subset) [PATCH 0/4] Various fixes for the lm3630a backlight driver Lee Jones
@ 2024-05-28 3:32 ` Bjorn Andersson
5 siblings, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2024-05-28 3:32 UTC (permalink / raw
To: ~postmarketos/upstreaming, phone-devel, Lee Jones,
Daniel Thompson, Jingoo Han, Helge Deller, Andrew Morton,
G.Shark Jeong, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Maximilian Weigand, Luca Weiss
Cc: dri-devel, linux-fbdev, linux-kernel, linux-arm-msm, devicetree
On Tue, 20 Feb 2024 00:11:18 +0100, Luca Weiss wrote:
> On the MSM8974 Nexus 5 and OnePlus One phones (latter doesn't have
> display upstream) the display backlight was turning off whenever you
> would write a brightness to sysfs since a recent commit to the driver
> (kernel v6.5).
>
> backlight: lm3630a: Turn off both led strings when display is blank
>
> [...]
Applied, thanks!
[4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight
commit: e23dfb4ee30a120a947abb94a718ccc1eb5f87ff
Best regards,
--
Bjorn Andersson <andersson@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-05-28 3:32 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-19 23:11 [PATCH 0/4] Various fixes for the lm3630a backlight driver Luca Weiss
2024-02-19 23:11 ` [PATCH 1/4] backlight: lm3630a: Initialize backlight_properties on init Luca Weiss
2024-02-20 14:00 ` Daniel Thompson
2024-02-20 14:07 ` Konrad Dybcio
2024-02-20 15:41 ` Daniel Thompson
2024-02-19 23:11 ` [PATCH 2/4] backlight: lm3630a: Don't set bl->props.brightness in get_brightness Luca Weiss
2024-02-20 14:03 ` Daniel Thompson
2024-02-19 23:11 ` [PATCH 3/4] backlight: lm3630a: Use backlight_get_brightness helper in update_status Luca Weiss
2024-02-20 14:11 ` Daniel Thompson
2024-02-20 16:43 ` Luca Weiss
2024-02-21 11:20 ` Daniel Thompson
2024-02-19 23:11 ` [PATCH 4/4] ARM: dts: qcom: msm8974-hammerhead: Hook up backlight Luca Weiss
2024-02-20 14:12 ` Daniel Thompson
2024-02-20 16:45 ` Luca Weiss
2024-02-20 17:07 ` Daniel Thompson
2024-02-23 17:02 ` (subset) [PATCH 0/4] Various fixes for the lm3630a backlight driver Lee Jones
2024-05-28 3:32 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).