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

[Yao Zi <ziyao@disroot.org>]

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
> > > >
> > > >
>