Message-ID: <aD6e0JLntUC6BdH7@pie.lan>
Date: Tue, 3 Jun 2025 07:05:52 +0000
From: Yao Zi <ziyao@...root.org>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Jianmin Lv <lvjianmin@...ngson.cn>, WANG Xuerui <kernel@...0n.name>,
	linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev,
	Mingcong Bai <jeffbai@...c.io>, Kexy Biscuit <kexybiscuit@...c.io>
Subject: Re: [PATCH 2/2] platform/loongarch: laptop: Support backlight power
 control

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@...root.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@...root.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
> >
> >
> 

Powered by blists - more mailing lists