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
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
>
>
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
> >
> >
>
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
> > >
> > >
> >
>
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/
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
© 2016 - 2026 Red Hat, Inc.