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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Daniel.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
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.
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 <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
> ---
> 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.
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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
> >
> > Reviewed-by: Daniel Thompson <[email protected]>
> >
> >
> > > ---
> > > 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.
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 <[email protected]>
>
> Reviewed-by: Daniel Thompson <[email protected]>
>
>
> > ---
> > 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.
>
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 <[email protected]>
>
> Reviewed-by: Daniel Thompson <[email protected]>
>
> 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.
>
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 <[email protected]>
> >
> > Reviewed-by: Daniel Thompson <[email protected]>
> >
> > 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.
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 [李琼斯]