Message-ID: <CAAhV-H4yt2Rdu5-8QsDyXpLo5v8jHSDmBB0nBO+4-pKzfbYJHA@mail.gmail.com>
Date: Wed, 4 Jun 2025 21:57:52 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Yao Zi <ziyao@...root.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 3, 2025 at 3:06 PM Yao Zi <ziyao@...root.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@...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.
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
> > >
> > >
> >
>

Powered by blists - more mailing lists