[PATCH 0/2] Backlight control improvements for loongson_laptop

[PATCH 0/2] Backlight control improvements for loongson_laptop

[Yao Zi]

I've observed strange screen brightness changes on TongFang L860-T2
3A5000 laptop after resumption. It's found that a brightness value that
doesn't match the hardware state before suspension is "restored" for the
EC, causing the strange behavior.

This series fixes the behavior by obtaining EC's brightness setting
instead of using a constant as current brightness on probe.

While digging through the code, I've found some unused,
power-control-related functions in the driver. They're cleaned up and
used to support power control of backlight in the second patch as well.

Yao Zi (2):
  platform/loongarch: laptop: Get brightness setting from EC on probe
  platform/loongarch: laptop: Support backlight power control

 drivers/platform/loongarch/loongson-laptop.c | 55 +++++++-------------
 1 file changed, 20 insertions(+), 35 deletions(-)

-- 
2.49.0

[PATCH 1/2] platform/loongarch: laptop: Get brightness setting from EC on probe

[Yao Zi]

Previously 1 is unconditionally taken as current brightness value. This
causes problems since it's required to restore brightness settings on
resumption, and a value that doesn't match EC's state before suspension
will cause surprising changes of screen brightness.

Let's get brightness from EC and take it as the current brightness on
probe of the laptop driver to avoid the surprising behavior. Tested on
TongFang L860-T2 3A5000 laptop.

Cc: stable@vger.kernel.org
Fixes: 6246ed09111f ("LoongArch: Add ACPI-based generic laptop driver")
Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/platform/loongarch/loongson-laptop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
index 99203584949d..828bd62e3596 100644
--- a/drivers/platform/loongarch/loongson-laptop.c
+++ b/drivers/platform/loongarch/loongson-laptop.c
@@ -392,7 +392,7 @@ static int laptop_backlight_register(void)
 	if (!acpi_evalf(hotkey_handle, &status, "ECLL", "d"))
 		return -EIO;
 
-	props.brightness = 1;
+	props.brightness = ec_get_brightness();
 	props.max_brightness = status;
 	props.type = BACKLIGHT_PLATFORM;
 
-- 
2.49.0

Re: [PATCH 1/2] platform/loongarch: laptop: Get brightness setting from EC on probe

[Huacai Chen]

On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
>
> Previously 1 is unconditionally taken as current brightness value. This
> causes problems since it's required to restore brightness settings on
> resumption, and a value that doesn't match EC's state before suspension
> will cause surprising changes of screen brightness.
laptop_backlight_register() isn't called at resuming, so I think your
problem has nothing to do with suspend (S3).

But there is really a problem about hibernation (S4): the brightness
is 1 during booting, but when switching to the target kernel, the
brightness may jump to the old value.

If the above case is what you meet, please update the commit message.

Huacai

>
> Let's get brightness from EC and take it as the current brightness on
> probe of the laptop driver to avoid the surprising behavior. Tested on
> TongFang L860-T2 3A5000 laptop.
>
> Cc: stable@vger.kernel.org
> Fixes: 6246ed09111f ("LoongArch: Add ACPI-based generic laptop driver")
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  drivers/platform/loongarch/loongson-laptop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> index 99203584949d..828bd62e3596 100644
> --- a/drivers/platform/loongarch/loongson-laptop.c
> +++ b/drivers/platform/loongarch/loongson-laptop.c
> @@ -392,7 +392,7 @@ static int laptop_backlight_register(void)
>         if (!acpi_evalf(hotkey_handle, &status, "ECLL", "d"))
>                 return -EIO;
>
> -       props.brightness = 1;
> +       props.brightness = ec_get_brightness();
>         props.max_brightness = status;
>         props.type = BACKLIGHT_PLATFORM;
>
> --
> 2.49.0
>
>

Re: [PATCH 1/2] platform/loongarch: laptop: Get brightness setting from EC on probe

[Yao Zi]

On Tue, Jun 03, 2025 at 12:11:48PM +0800, Huacai Chen wrote:
> On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
> >
> > Previously 1 is unconditionally taken as current brightness value. This
> > causes problems since it's required to restore brightness settings on
> > resumption, and a value that doesn't match EC's state before suspension
> > will cause surprising changes of screen brightness.
> laptop_backlight_register() isn't called at resuming, so I think your
> problem has nothing to do with suspend (S3).

It does have something to do with it. In loongson_hotkey_resume() which
is called when leaving S3 (suspension), the brightness is restored
according to props.brightness,

        bd = backlight_device_get_by_type(BACKLIGHT_PLATFORM);
        if (bd) {
                loongson_laptop_backlight_update(bd) ?
                pr_warn("Loongson_backlight: resume brightness failed") :
                pr_info("Loongson_backlight: resume brightness %d\n", bd->props
.brightness);
        }

and without this patch, props.brightness is always set to 1 when the
driver probes, but actually (at least with the firmware on my laptop)
the screen brightness is set to 80 instead of 1 on cold boot, IOW, a
brightness value that doesn't match hardware state is set to
props.brightness.

On resumption, loongson_hotkey_resume() restores the brightness
settings according to props.brightness. But as the value isn't what is
used by hardware before suspension. the screen brightness will look very
different (1 v.s. 80) comparing to the brightness before suspension.

Some dmesg proves this as well, without this patch it says

	loongson_laptop: Loongson_backlight: resume brightness 1

but before suspension, reading
/sys/class/backlight/loongson3_laptop/actual_brightness yields 80.

> But there is really a problem about hibernation (S4): the brightness
> is 1 during booting, but when switching to the target kernel, the
> brightness may jump to the old value.
> 
> If the above case is what you meet, please update the commit message.
> 
> Huacai

Thanks,
Yao Zi

> >
> > Let's get brightness from EC and take it as the current brightness on
> > probe of the laptop driver to avoid the surprising behavior. Tested on
> > TongFang L860-T2 3A5000 laptop.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 6246ed09111f ("LoongArch: Add ACPI-based generic laptop driver")
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  drivers/platform/loongarch/loongson-laptop.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> > index 99203584949d..828bd62e3596 100644
> > --- a/drivers/platform/loongarch/loongson-laptop.c
> > +++ b/drivers/platform/loongarch/loongson-laptop.c
> > @@ -392,7 +392,7 @@ static int laptop_backlight_register(void)
> >         if (!acpi_evalf(hotkey_handle, &status, "ECLL", "d"))
> >                 return -EIO;
> >
> > -       props.brightness = 1;
> > +       props.brightness = ec_get_brightness();
> >         props.max_brightness = status;
> >         props.type = BACKLIGHT_PLATFORM;
> >
> > --
> > 2.49.0
> >
> >

Re: [PATCH 1/2] platform/loongarch: laptop: Get brightness setting from EC on probe

[Huacai Chen]

On Tue, Jun 3, 2025 at 2:44 PM Yao Zi <ziyao@disroot.org> wrote:
>
> On Tue, Jun 03, 2025 at 12:11:48PM +0800, Huacai Chen wrote:
> > On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
> > >
> > > Previously 1 is unconditionally taken as current brightness value. This
> > > causes problems since it's required to restore brightness settings on
> > > resumption, and a value that doesn't match EC's state before suspension
> > > will cause surprising changes of screen brightness.
> > laptop_backlight_register() isn't called at resuming, so I think your
> > problem has nothing to do with suspend (S3).
>
> It does have something to do with it. In loongson_hotkey_resume() which
> is called when leaving S3 (suspension), the brightness is restored
> according to props.brightness,
>
>         bd = backlight_device_get_by_type(BACKLIGHT_PLATFORM);
>         if (bd) {
>                 loongson_laptop_backlight_update(bd) ?
>                 pr_warn("Loongson_backlight: resume brightness failed") :
>                 pr_info("Loongson_backlight: resume brightness %d\n", bd->props
> .brightness);
>         }
>
> and without this patch, props.brightness is always set to 1 when the
> driver probes, but actually (at least with the firmware on my laptop)
> the screen brightness is set to 80 instead of 1 on cold boot, IOW, a
> brightness value that doesn't match hardware state is set to
> props.brightness.
>
> On resumption, loongson_hotkey_resume() restores the brightness
> settings according to props.brightness. But as the value isn't what is
> used by hardware before suspension. the screen brightness will look very
> different (1 v.s. 80) comparing to the brightness before suspension.
>
> Some dmesg proves this as well, without this patch it says
>
>         loongson_laptop: Loongson_backlight: resume brightness 1
>
> but before suspension, reading
> /sys/class/backlight/loongson3_laptop/actual_brightness yields 80.
OK, that makes sense. But the commit message can still be improved, at
least replace suspension/resumption with suspend/resume. You can grep
them at Documentation/power.

Huacai

>
> > But there is really a problem about hibernation (S4): the brightness
> > is 1 during booting, but when switching to the target kernel, the
> > brightness may jump to the old value.
> >
> > If the above case is what you meet, please update the commit message.
> >
> > Huacai
>
> Thanks,
> Yao Zi
>
> > >
> > > Let's get brightness from EC and take it as the current brightness on
> > > probe of the laptop driver to avoid the surprising behavior. Tested on
> > > TongFang L860-T2 3A5000 laptop.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 6246ed09111f ("LoongArch: Add ACPI-based generic laptop driver")
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > >  drivers/platform/loongarch/loongson-laptop.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> > > index 99203584949d..828bd62e3596 100644
> > > --- a/drivers/platform/loongarch/loongson-laptop.c
> > > +++ b/drivers/platform/loongarch/loongson-laptop.c
> > > @@ -392,7 +392,7 @@ static int laptop_backlight_register(void)
> > >         if (!acpi_evalf(hotkey_handle, &status, "ECLL", "d"))
> > >                 return -EIO;
> > >
> > > -       props.brightness = 1;
> > > +       props.brightness = ec_get_brightness();
> > >         props.max_brightness = status;
> > >         props.type = BACKLIGHT_PLATFORM;
> > >
> > > --
> > > 2.49.0
> > >
> > >

Re: [PATCH 1/2] platform/loongarch: laptop: Get brightness setting from EC on probe

[Yao Zi]

On Wed, Jun 04, 2025 at 09:56:20PM +0800, Huacai Chen wrote:
> On Tue, Jun 3, 2025 at 2:44 PM Yao Zi <ziyao@disroot.org> wrote:
> >
> > On Tue, Jun 03, 2025 at 12:11:48PM +0800, Huacai Chen wrote:
> > > On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
> > > >
> > > > Previously 1 is unconditionally taken as current brightness value. This
> > > > causes problems since it's required to restore brightness settings on
> > > > resumption, and a value that doesn't match EC's state before suspension
> > > > will cause surprising changes of screen brightness.
> > > laptop_backlight_register() isn't called at resuming, so I think your
> > > problem has nothing to do with suspend (S3).
> >
> > It does have something to do with it. In loongson_hotkey_resume() which
> > is called when leaving S3 (suspension), the brightness is restored
> > according to props.brightness,
> >
> >         bd = backlight_device_get_by_type(BACKLIGHT_PLATFORM);
> >         if (bd) {
> >                 loongson_laptop_backlight_update(bd) ?
> >                 pr_warn("Loongson_backlight: resume brightness failed") :
> >                 pr_info("Loongson_backlight: resume brightness %d\n", bd->props
> > .brightness);
> >         }
> >
> > and without this patch, props.brightness is always set to 1 when the
> > driver probes, but actually (at least with the firmware on my laptop)
> > the screen brightness is set to 80 instead of 1 on cold boot, IOW, a
> > brightness value that doesn't match hardware state is set to
> > props.brightness.
> >
> > On resumption, loongson_hotkey_resume() restores the brightness
> > settings according to props.brightness. But as the value isn't what is
> > used by hardware before suspension. the screen brightness will look very
> > different (1 v.s. 80) comparing to the brightness before suspension.
> >
> > Some dmesg proves this as well, without this patch it says
> >
> >         loongson_laptop: Loongson_backlight: resume brightness 1
> >
> > but before suspension, reading
> > /sys/class/backlight/loongson3_laptop/actual_brightness yields 80.
> OK, that makes sense. But the commit message can still be improved, at
> least replace suspension/resumption with suspend/resume. You can grep
> them at Documentation/power.

Oops, thanks for the hint. Seems suspend/resume are wider used, and I
will reword the commit message in v2 :)

> Huacai

Regards,
Yao Zi

> >
> > > But there is really a problem about hibernation (S4): the brightness
> > > is 1 during booting, but when switching to the target kernel, the
> > > brightness may jump to the old value.
> > >
> > > If the above case is what you meet, please update the commit message.
> > >
> > > Huacai
> >
> > Thanks,
> > Yao Zi
> >
> > > >
> > > > Let's get brightness from EC and take it as the current brightness on
> > > > probe of the laptop driver to avoid the surprising behavior. Tested on
> > > > TongFang L860-T2 3A5000 laptop.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 6246ed09111f ("LoongArch: Add ACPI-based generic laptop driver")
> > > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > > ---
> > > >  drivers/platform/loongarch/loongson-laptop.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> > > > index 99203584949d..828bd62e3596 100644
> > > > --- a/drivers/platform/loongarch/loongson-laptop.c
> > > > +++ b/drivers/platform/loongarch/loongson-laptop.c
> > > > @@ -392,7 +392,7 @@ static int laptop_backlight_register(void)
> > > >         if (!acpi_evalf(hotkey_handle, &status, "ECLL", "d"))
> > > >                 return -EIO;
> > > >
> > > > -       props.brightness = 1;
> > > > +       props.brightness = ec_get_brightness();
> > > >         props.max_brightness = status;
> > > >         props.type = BACKLIGHT_PLATFORM;
> > > >
> > > > --
> > > > 2.49.0
> > > >
> > > >
> 

[PATCH 2/2] platform/loongarch: laptop: Support backlight power control

[Yao Zi]

loongson_laptop_turn_{on,off}_backlight() are designed for controlling
power of the backlight, but they aren't really used in the driver
previously.

Unify these two functions since they only differ in arguments passed to
ACPI method, and wire up loongson_laptop_backlight_update() to update
power state of the backlight as well. Tested on TongFang L860-T2 3A5000
laptop.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/platform/loongarch/loongson-laptop.c | 53 +++++++-------------
 1 file changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
index 828bd62e3596..f01e53b1c84d 100644
--- a/drivers/platform/loongarch/loongson-laptop.c
+++ b/drivers/platform/loongarch/loongson-laptop.c
@@ -56,8 +56,6 @@ static struct input_dev *generic_inputdev;
 static acpi_handle hotkey_handle;
 static struct key_entry hotkey_keycode_map[GENERIC_HOTKEY_MAP_MAX];
 
-int loongson_laptop_turn_on_backlight(void);
-int loongson_laptop_turn_off_backlight(void);
 static int loongson_laptop_backlight_update(struct backlight_device *bd);
 
 /* 2. ACPI Helpers and device model */
@@ -354,6 +352,22 @@ static int ec_backlight_level(u8 level)
 	return level;
 }
 
+static int ec_backlight_set_power(bool state)
+{
+	int status;
+	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
+	struct acpi_object_list args = { 1, &arg0 };
+
+	arg0.integer.value = state;
+	status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_info("Loongson lvds error: 0x%x\n", status);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int loongson_laptop_backlight_update(struct backlight_device *bd)
 {
 	int lvl = ec_backlight_level(bd->props.brightness);
@@ -363,6 +377,8 @@ static int loongson_laptop_backlight_update(struct backlight_device *bd)
 	if (ec_set_brightness(lvl))
 		return -EIO;
 
+	ec_backlight_set_power(bd->props.power == BACKLIGHT_POWER_ON ? true : false);
+
 	return 0;
 }
 
@@ -394,6 +410,7 @@ static int laptop_backlight_register(void)
 
 	props.brightness = ec_get_brightness();
 	props.max_brightness = status;
+	props.power = BACKLIGHT_POWER_ON;
 	props.type = BACKLIGHT_PLATFORM;
 
 	backlight_device_register("loongson_laptop",
@@ -402,38 +419,6 @@ static int laptop_backlight_register(void)
 	return 0;
 }
 
-int loongson_laptop_turn_on_backlight(void)
-{
-	int status;
-	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
-	struct acpi_object_list args = { 1, &arg0 };
-
-	arg0.integer.value = 1;
-	status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
-	if (ACPI_FAILURE(status)) {
-		pr_info("Loongson lvds error: 0x%x\n", status);
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
-int loongson_laptop_turn_off_backlight(void)
-{
-	int status;
-	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
-	struct acpi_object_list args = { 1, &arg0 };
-
-	arg0.integer.value = 0;
-	status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
-	if (ACPI_FAILURE(status)) {
-		pr_info("Loongson lvds error: 0x%x\n", status);
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
 static int __init event_init(struct generic_sub_driver *sub_driver)
 {
 	int ret;
-- 
2.49.0

Re: [PATCH 2/2] platform/loongarch: laptop: Support backlight power control

[Huacai Chen]

On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
>
> loongson_laptop_turn_{on,off}_backlight() are designed for controlling
> power of the backlight, but they aren't really used in the driver
> previously.
>
> Unify these two functions since they only differ in arguments passed to
> ACPI method, and wire up loongson_laptop_backlight_update() to update
> power state of the backlight as well. Tested on TongFang L860-T2 3A5000
> laptop.
>
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  drivers/platform/loongarch/loongson-laptop.c | 53 +++++++-------------
>  1 file changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> index 828bd62e3596..f01e53b1c84d 100644
> --- a/drivers/platform/loongarch/loongson-laptop.c
> +++ b/drivers/platform/loongarch/loongson-laptop.c
> @@ -56,8 +56,6 @@ static struct input_dev *generic_inputdev;
>  static acpi_handle hotkey_handle;
>  static struct key_entry hotkey_keycode_map[GENERIC_HOTKEY_MAP_MAX];
>
> -int loongson_laptop_turn_on_backlight(void);
> -int loongson_laptop_turn_off_backlight(void);
>  static int loongson_laptop_backlight_update(struct backlight_device *bd);
>
>  /* 2. ACPI Helpers and device model */
> @@ -354,6 +352,22 @@ static int ec_backlight_level(u8 level)
>         return level;
>  }
>
> +static int ec_backlight_set_power(bool state)
> +{
> +       int status;
> +       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> +       struct acpi_object_list args = { 1, &arg0 };
> +
> +       arg0.integer.value = state;
> +       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> +       if (ACPI_FAILURE(status)) {
> +               pr_info("Loongson lvds error: 0x%x\n", status);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
>  static int loongson_laptop_backlight_update(struct backlight_device *bd)
>  {
>         int lvl = ec_backlight_level(bd->props.brightness);
> @@ -363,6 +377,8 @@ static int loongson_laptop_backlight_update(struct backlight_device *bd)
>         if (ec_set_brightness(lvl))
>                 return -EIO;
>
> +       ec_backlight_set_power(bd->props.power == BACKLIGHT_POWER_ON ? true : false);
It is better to check the status before setting, because the EC
firmware may not be as robust as needed, a checking can reduce
interactions between kernel and EC.

There is an example: dp_aux_backlight_update_status() in
drivers/gpu/drm/display/drm_dp_helper.c.

> +
>         return 0;
>  }
>
> @@ -394,6 +410,7 @@ static int laptop_backlight_register(void)
>
>         props.brightness = ec_get_brightness();
>         props.max_brightness = status;
> +       props.power = BACKLIGHT_POWER_ON;
>         props.type = BACKLIGHT_PLATFORM;
>
>         backlight_device_register("loongson_laptop",
> @@ -402,38 +419,6 @@ static int laptop_backlight_register(void)
>         return 0;
>  }
>
> -int loongson_laptop_turn_on_backlight(void)
> -{
> -       int status;
> -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> -       struct acpi_object_list args = { 1, &arg0 };
> -
> -       arg0.integer.value = 1;
> -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> -       if (ACPI_FAILURE(status)) {
> -               pr_info("Loongson lvds error: 0x%x\n", status);
> -               return -ENODEV;
> -       }
> -
> -       return 0;
> -}
> -
> -int loongson_laptop_turn_off_backlight(void)
> -{
> -       int status;
> -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> -       struct acpi_object_list args = { 1, &arg0 };
> -
> -       arg0.integer.value = 0;
> -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> -       if (ACPI_FAILURE(status)) {
> -               pr_info("Loongson lvds error: 0x%x\n", status);
> -               return -ENODEV;
> -       }
> -
> -       return 0;
> -}
I prefer to keep them, in downstream kernels there are users of them,
I don't want to add them back if one day those users are upstream.

Huacai

> -
>  static int __init event_init(struct generic_sub_driver *sub_driver)
>  {
>         int ret;
> --
> 2.49.0
>
>

Re: [PATCH 2/2] platform/loongarch: laptop: Support backlight power control

[WANG Xuerui]

On 6/3/25 12:16, Huacai Chen wrote:
> On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
>>
>> loongson_laptop_turn_{on,off}_backlight() are designed for controlling
>> power of the backlight, but they aren't really used in the driver
>> previously.
>>
>> Unify these two functions since they only differ in arguments passed to
>> ACPI method, and wire up loongson_laptop_backlight_update() to update
>> power state of the backlight as well. Tested on TongFang L860-T2 3A5000
>> laptop.
>>
>> Signed-off-by: Yao Zi <ziyao@disroot.org>
>> ---
>>   drivers/platform/loongarch/loongson-laptop.c | 53 +++++++-------------
>>   1 file changed, 19 insertions(+), 34 deletions(-)
>>
>> [snip]
>>
>> -int loongson_laptop_turn_on_backlight(void)
>> -{
>> -       int status;
>> -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>> -       struct acpi_object_list args = { 1, &arg0 };
>> -
>> -       arg0.integer.value = 1;
>> -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
>> -       if (ACPI_FAILURE(status)) {
>> -               pr_info("Loongson lvds error: 0x%x\n", status);
>> -               return -ENODEV;
>> -       }
>> -
>> -       return 0;
>> -}
>> -
>> -int loongson_laptop_turn_off_backlight(void)
>> -{
>> -       int status;
>> -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>> -       struct acpi_object_list args = { 1, &arg0 };
>> -
>> -       arg0.integer.value = 0;
>> -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
>> -       if (ACPI_FAILURE(status)) {
>> -               pr_info("Loongson lvds error: 0x%x\n", status);
>> -               return -ENODEV;
>> -       }
>> -
>> -       return 0;
>> -}
> I prefer to keep them, in downstream kernels there are users of them,
> I don't want to add them back if one day those users are upstream.

Then these symbols should be properly EXPORT_SYMBOL_GPL marked?

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

Re: [PATCH 2/2] platform/loongarch: laptop: Support backlight power control

[Xi Ruoyao]

On Tue, 2025-06-03 at 13:44 +0800, WANG Xuerui wrote:
> On 6/3/25 12:16, Huacai Chen wrote:
> > On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
> > > 
> > > loongson_laptop_turn_{on,off}_backlight() are designed for controlling
> > > power of the backlight, but they aren't really used in the driver
> > > previously.
> > > 
> > > Unify these two functions since they only differ in arguments passed to
> > > ACPI method, and wire up loongson_laptop_backlight_update() to update
> > > power state of the backlight as well. Tested on TongFang L860-T2 3A5000
> > > laptop.
> > > 
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > >   drivers/platform/loongarch/loongson-laptop.c | 53 +++++++-------------
> > >   1 file changed, 19 insertions(+), 34 deletions(-)
> > > 
> > > [snip]
> > > 
> > > -int loongson_laptop_turn_on_backlight(void)
> > > -{
> > > -       int status;
> > > -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > > -       struct acpi_object_list args = { 1, &arg0 };
> > > -
> > > -       arg0.integer.value = 1;
> > > -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > > -       if (ACPI_FAILURE(status)) {
> > > -               pr_info("Loongson lvds error: 0x%x\n", status);
> > > -               return -ENODEV;
> > > -       }
> > > -
> > > -       return 0;
> > > -}
> > > -
> > > -int loongson_laptop_turn_off_backlight(void)
> > > -{
> > > -       int status;
> > > -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > > -       struct acpi_object_list args = { 1, &arg0 };
> > > -
> > > -       arg0.integer.value = 0;
> > > -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > > -       if (ACPI_FAILURE(status)) {
> > > -               pr_info("Loongson lvds error: 0x%x\n", status);
> > > -               return -ENODEV;
> > > -       }
> > > -
> > > -       return 0;
> > > -}
> > I prefer to keep them, in downstream kernels there are users of them,
> > I don't want to add them back if one day those users are upstream.
> 
> Then these symbols should be properly EXPORT_SYMBOL_GPL marked?

I guess those downstream works are kernel forks, not separated modules,
thus an export is not needed.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH 2/2] platform/loongarch: laptop: Support backlight power control

[Yao Zi]

On Tue, Jun 03, 2025 at 12:16:57PM +0800, Huacai Chen wrote:
> On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
> >
> > loongson_laptop_turn_{on,off}_backlight() are designed for controlling
> > power of the backlight, but they aren't really used in the driver
> > previously.
> >
> > Unify these two functions since they only differ in arguments passed to
> > ACPI method, and wire up loongson_laptop_backlight_update() to update
> > power state of the backlight as well. Tested on TongFang L860-T2 3A5000
> > laptop.
> >
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  drivers/platform/loongarch/loongson-laptop.c | 53 +++++++-------------
> >  1 file changed, 19 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> > index 828bd62e3596..f01e53b1c84d 100644
> > --- a/drivers/platform/loongarch/loongson-laptop.c
> > +++ b/drivers/platform/loongarch/loongson-laptop.c
> > @@ -56,8 +56,6 @@ static struct input_dev *generic_inputdev;
> >  static acpi_handle hotkey_handle;
> >  static struct key_entry hotkey_keycode_map[GENERIC_HOTKEY_MAP_MAX];
> >
> > -int loongson_laptop_turn_on_backlight(void);
> > -int loongson_laptop_turn_off_backlight(void);
> >  static int loongson_laptop_backlight_update(struct backlight_device *bd);
> >
> >  /* 2. ACPI Helpers and device model */
> > @@ -354,6 +352,22 @@ static int ec_backlight_level(u8 level)
> >         return level;
> >  }
> >
> > +static int ec_backlight_set_power(bool state)
> > +{
> > +       int status;
> > +       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > +       struct acpi_object_list args = { 1, &arg0 };
> > +
> > +       arg0.integer.value = state;
> > +       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > +       if (ACPI_FAILURE(status)) {
> > +               pr_info("Loongson lvds error: 0x%x\n", status);
> > +               return -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int loongson_laptop_backlight_update(struct backlight_device *bd)
> >  {
> >         int lvl = ec_backlight_level(bd->props.brightness);
> > @@ -363,6 +377,8 @@ static int loongson_laptop_backlight_update(struct backlight_device *bd)
> >         if (ec_set_brightness(lvl))
> >                 return -EIO;
> >
> > +       ec_backlight_set_power(bd->props.power == BACKLIGHT_POWER_ON ? true : false);
> It is better to check the status before setting, because the EC
> firmware may not be as robust as needed, a checking can reduce
> interactions between kernel and EC.
> 
> There is an example: dp_aux_backlight_update_status() in
> drivers/gpu/drm/display/drm_dp_helper.c.

It's reasonable and I'll take it.

> > +
> >         return 0;
> >  }
> >
> > @@ -394,6 +410,7 @@ static int laptop_backlight_register(void)
> >
> >         props.brightness = ec_get_brightness();
> >         props.max_brightness = status;
> > +       props.power = BACKLIGHT_POWER_ON;
> >         props.type = BACKLIGHT_PLATFORM;
> >
> >         backlight_device_register("loongson_laptop",
> > @@ -402,38 +419,6 @@ static int laptop_backlight_register(void)
> >         return 0;
> >  }
> >
> > -int loongson_laptop_turn_on_backlight(void)
> > -{
> > -       int status;
> > -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > -       struct acpi_object_list args = { 1, &arg0 };
> > -
> > -       arg0.integer.value = 1;
> > -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > -       if (ACPI_FAILURE(status)) {
> > -               pr_info("Loongson lvds error: 0x%x\n", status);
> > -               return -ENODEV;
> > -       }
> > -
> > -       return 0;
> > -}
> > -
> > -int loongson_laptop_turn_off_backlight(void)
> > -{
> > -       int status;
> > -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > -       struct acpi_object_list args = { 1, &arg0 };
> > -
> > -       arg0.integer.value = 0;
> > -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > -       if (ACPI_FAILURE(status)) {
> > -               pr_info("Loongson lvds error: 0x%x\n", status);
> > -               return -ENODEV;
> > -       }
> > -
> > -       return 0;
> > -}
> I prefer to keep them, in downstream kernels there are users of them,
> I don't want to add them back if one day those users are upstream.

These two functions are mostly identical, and I think unifying them
together should be the right way to go. If this makes sense, users
introduced in the future should also adapt it, instead of keeping
redundant code in the current mainline kernel.

If there're new users of the API out of the loongson3_laptop module in
the future, it's still easy to rename ec_backlight_set_power() and
export it.

For these two points, I disagree on keeping these two symbols.

> Huacai

Thanks,
Yao Zi

> > -
> >  static int __init event_init(struct generic_sub_driver *sub_driver)
> >  {
> >         int ret;
> > --
> > 2.49.0
> >
> >
> 

Re: [PATCH 2/2] platform/loongarch: laptop: Support backlight power control

[Huacai Chen]

On Tue, Jun 3, 2025 at 3:06 PM Yao Zi <ziyao@disroot.org> wrote:
>
> On Tue, Jun 03, 2025 at 12:16:57PM +0800, Huacai Chen wrote:
> > On Sat, May 31, 2025 at 7:39 PM Yao Zi <ziyao@disroot.org> wrote:
> > >
> > > loongson_laptop_turn_{on,off}_backlight() are designed for controlling
> > > power of the backlight, but they aren't really used in the driver
> > > previously.
> > >
> > > Unify these two functions since they only differ in arguments passed to
> > > ACPI method, and wire up loongson_laptop_backlight_update() to update
> > > power state of the backlight as well. Tested on TongFang L860-T2 3A5000
> > > laptop.
> > >
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > >  drivers/platform/loongarch/loongson-laptop.c | 53 +++++++-------------
> > >  1 file changed, 19 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/platform/loongarch/loongson-laptop.c b/drivers/platform/loongarch/loongson-laptop.c
> > > index 828bd62e3596..f01e53b1c84d 100644
> > > --- a/drivers/platform/loongarch/loongson-laptop.c
> > > +++ b/drivers/platform/loongarch/loongson-laptop.c
> > > @@ -56,8 +56,6 @@ static struct input_dev *generic_inputdev;
> > >  static acpi_handle hotkey_handle;
> > >  static struct key_entry hotkey_keycode_map[GENERIC_HOTKEY_MAP_MAX];
> > >
> > > -int loongson_laptop_turn_on_backlight(void);
> > > -int loongson_laptop_turn_off_backlight(void);
> > >  static int loongson_laptop_backlight_update(struct backlight_device *bd);
> > >
> > >  /* 2. ACPI Helpers and device model */
> > > @@ -354,6 +352,22 @@ static int ec_backlight_level(u8 level)
> > >         return level;
> > >  }
> > >
> > > +static int ec_backlight_set_power(bool state)
> > > +{
> > > +       int status;
> > > +       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > > +       struct acpi_object_list args = { 1, &arg0 };
> > > +
> > > +       arg0.integer.value = state;
> > > +       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > > +       if (ACPI_FAILURE(status)) {
> > > +               pr_info("Loongson lvds error: 0x%x\n", status);
> > > +               return -EIO;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int loongson_laptop_backlight_update(struct backlight_device *bd)
> > >  {
> > >         int lvl = ec_backlight_level(bd->props.brightness);
> > > @@ -363,6 +377,8 @@ static int loongson_laptop_backlight_update(struct backlight_device *bd)
> > >         if (ec_set_brightness(lvl))
> > >                 return -EIO;
> > >
> > > +       ec_backlight_set_power(bd->props.power == BACKLIGHT_POWER_ON ? true : false);
> > It is better to check the status before setting, because the EC
> > firmware may not be as robust as needed, a checking can reduce
> > interactions between kernel and EC.
> >
> > There is an example: dp_aux_backlight_update_status() in
> > drivers/gpu/drm/display/drm_dp_helper.c.
>
> It's reasonable and I'll take it.
>
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -394,6 +410,7 @@ static int laptop_backlight_register(void)
> > >
> > >         props.brightness = ec_get_brightness();
> > >         props.max_brightness = status;
> > > +       props.power = BACKLIGHT_POWER_ON;
> > >         props.type = BACKLIGHT_PLATFORM;
> > >
> > >         backlight_device_register("loongson_laptop",
> > > @@ -402,38 +419,6 @@ static int laptop_backlight_register(void)
> > >         return 0;
> > >  }
> > >
> > > -int loongson_laptop_turn_on_backlight(void)
> > > -{
> > > -       int status;
> > > -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > > -       struct acpi_object_list args = { 1, &arg0 };
> > > -
> > > -       arg0.integer.value = 1;
> > > -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > > -       if (ACPI_FAILURE(status)) {
> > > -               pr_info("Loongson lvds error: 0x%x\n", status);
> > > -               return -ENODEV;
> > > -       }
> > > -
> > > -       return 0;
> > > -}
> > > -
> > > -int loongson_laptop_turn_off_backlight(void)
> > > -{
> > > -       int status;
> > > -       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > > -       struct acpi_object_list args = { 1, &arg0 };
> > > -
> > > -       arg0.integer.value = 0;
> > > -       status = acpi_evaluate_object(NULL, "\\BLSW", &args, NULL);
> > > -       if (ACPI_FAILURE(status)) {
> > > -               pr_info("Loongson lvds error: 0x%x\n", status);
> > > -               return -ENODEV;
> > > -       }
> > > -
> > > -       return 0;
> > > -}
> > I prefer to keep them, in downstream kernels there are users of them,
> > I don't want to add them back if one day those users are upstream.
>
> These two functions are mostly identical, and I think unifying them
> together should be the right way to go. If this makes sense, users
> introduced in the future should also adapt it, instead of keeping
> redundant code in the current mainline kernel.
>
> If there're new users of the API out of the loongson3_laptop module in
> the future, it's still easy to rename ec_backlight_set_power() and
> export it.
>
> For these two points, I disagree on keeping these two symbols.
OK, please add a check before communicating with EC, and then send V2
as soon as possible.

Huacai

>
> > Huacai
>
> Thanks,
> Yao Zi
>
> > > -
> > >  static int __init event_init(struct generic_sub_driver *sub_driver)
> > >  {
> > >         int ret;
> > > --
> > > 2.49.0
> > >
> > >
> >
>