| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f99671f46a7c534f017fbe8912acde95e3186407.camel@rong.moe>
Date: Wed, 06 Aug 2025 21:22:43 +0800
From: Rong Zhang <i@...g.moe>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Ike Panhc <ikepanhc@...il.com>, "Derek J. Clark"
<derekjohn.clark@...il.com>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
Hans de Goede <hansg@...nel.org>, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] platform/x86: ideapad-laptop: Decouple HW & cdev
brightness for kbd backlight
Hi Ilpo,
On Wed, 2025-08-06 at 14:19 +0300, Ilpo Järvinen wrote:
> On Tue, 5 Aug 2025, Rong Zhang wrote:
>
> > Some recent models come with an ambient light sensor (ALS). On these
> > models, their EC will automatically set the keyboard backlight to an
> > appropriate brightness when the effective "HW brightness" is 3. The term
> > "HW brightness" means that it cannot be perfectly mapped to an LED
> > classdev brightness, but the EC does use this predefined brightness
> > value to represent such a mode.
> >
> > Currently, the code processing keyboard backlight is coupled with LED
> > classdev, making it hard to expose the auto brightness (ALS) mode to the
> > userspace.
> >
> > As the first step toward the goal, decouple HW brightness from LED
> > classdev brightness, and update comments about corresponding backlight
> > modes.
> >
> > Signed-off-by: Rong Zhang <i@...g.moe>
> > ---
> > drivers/platform/x86/lenovo/ideapad-laptop.c | 125 +++++++++++++------
> > 1 file changed, 90 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > index fcebfbaf0460..5014c1d0b633 100644
> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > @@ -119,17 +119,40 @@ enum {
> > };
> >
> > /*
> > - * These correspond to the number of supported states - 1
> > - * Future keyboard types may need a new system, if there's a collision
> > - * KBD_BL_TRISTATE_AUTO has no way to report or set the auto state
> > - * so it effectively has 3 states, but needs to handle 4
> > + * The enumeration has two purposes:
> > + * - as an internal identifier for all known types of keyboard backlight
> > + * - as a mandatory parameter of the KBLC command
> > + *
> > + * For each type, the HW brightness values are defined as follows:
> > + * +--------------------------+----------+-----+------+------+
> > + * | HW brightness | 0 | 1 | 2 | 3 |
> > + * | Type | | | | |
> > + * +--------------------------+----------+-----+------+------+
> > + * | KBD_BL_STANDARD | off | on | N/A | N/A |
> > + * +--------------------------+----------+-----+------+------+
> > + * | KBD_BL_TRISTATE | off | low | high | N/A |
> > + * +--------------------------+----------+-----+------+------+
> > + * | KBD_BL_TRISTATE_AUTO | off | low | high | auto |
> > + * +--------------------------+----------+-----+------+------+
> > + *
> > + * We map LED classdev brightness for KBD_BL_TRISTATE_AUTO as follows:
> > + * +--------------------------+----------+-----+------+
> > + * | LED classdev brightness | 0 | 1 | 2 |
> > + * | Operation | | | |
> > + * +--------------------------+----------+-----+------+
> > + * | Read | off/auto | low | high |
> > + * +--------------------------+----------+-----+------+
> > + * | Write | off | low | high |
> > + * +--------------------------+----------+-----+------+
> > */
> > enum {
> > - KBD_BL_STANDARD = 1,
> > - KBD_BL_TRISTATE = 2,
> > - KBD_BL_TRISTATE_AUTO = 3,
> > + KBD_BL_STANDARD = 1,
> > + KBD_BL_TRISTATE = 2,
> > + KBD_BL_TRISTATE_AUTO = 3,
>
> Pure space change for no reason. Please leave as is.
Sure. Will remove the change in v2.
> > };
> >
> > +#define KBD_BL_AUTO_MODE_HW_BRIGHTNESS 3
> > +
> > #define KBD_BL_QUERY_TYPE 0x1
> > #define KBD_BL_TRISTATE_TYPE 0x5
> > #define KBD_BL_TRISTATE_AUTO_TYPE 0x7
> > @@ -1559,7 +1582,25 @@ static int ideapad_kbd_bl_check_tristate(int type)
> > return (type == KBD_BL_TRISTATE) || (type == KBD_BL_TRISTATE_AUTO);
> > }
> >
> > -static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> > +static int ideapad_kbd_bl_brightness_parse(struct ideapad_private *priv,
> > + unsigned int hw_brightness)
> > +{
> > + /* Off, low or high */
> > + if (hw_brightness <= priv->kbd_bl.led.max_brightness)
> > + return hw_brightness;
> > +
> > + /* Auto (controlled by EC according to ALS), report as off */
> > + if (priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO &&
> > + hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS)
> > + return 0;
>
> There seems to be code move/function refactoring (split to get+parse)
> and changes mixed up in here as this doesn't match to what was there
> beforehand AFAICT.
>
> Could you please try to separate the pure function refactoring from the
> changes to make the real changes easier to understand/see.
Sure. Will move real changes into [PATCH v2 2/2]. Besides, the macro
KBD_BL_AUTO_MODE_HW_BRIGHTNESS will then be unused in [PATCH v2 1/2],
so I will also move it into [PATCH v2 2/2].
> > + /* Unknown value */
> > + dev_warn(&priv->platform_device->dev,
> > + "Unknown keyboard backlight value: %u", hw_brightness);
> > + return -EINVAL;
> > +}
> > +
> > +static int ideapad_kbd_bl_hw_brightness_get(struct ideapad_private *priv)
> > {
> > unsigned long value;
> > int err;
> > @@ -1573,21 +1614,7 @@ static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> > if (err)
> > return err;
> >
> > - /* Convert returned value to brightness level */
> > - value = FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
> > -
> > - /* Off, low or high */
> > - if (value <= priv->kbd_bl.led.max_brightness)
> > - return value;
> > -
> > - /* Auto, report as off */
> > - if (value == priv->kbd_bl.led.max_brightness + 1)
> > - return 0;
> > -
> > - /* Unknown value */
> > - dev_warn(&priv->platform_device->dev,
> > - "Unknown keyboard backlight value: %lu", value);
> > - return -EINVAL;
> > + return FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
> > }
> >
> > err = eval_hals(priv->adev->handle, &value);
> > @@ -1597,6 +1624,16 @@ static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> > return !!test_bit(HALS_KBD_BL_STATE_BIT, &value);
> > }
> >
> > +static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> > +{
> > + int hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> > +
> > + if (hw_brightness < 0)
> > + return hw_brightness;
> > +
> > + return ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
> > +}
> > +
> > static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
> > {
> > struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> > @@ -1604,24 +1641,37 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
> > return ideapad_kbd_bl_brightness_get(priv);
> > }
> >
> > -static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> > +static int ideapad_kbd_bl_hw_brightness_set(struct ideapad_private *priv,
> > + unsigned int hw_brightness)
> > {
> > int err;
> > unsigned long value;
> > int type = priv->kbd_bl.type;
> >
> > if (ideapad_kbd_bl_check_tristate(type)) {
> > - if (brightness > priv->kbd_bl.led.max_brightness)
> > - return -EINVAL;
> > -
> > - value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, brightness) |
> > + value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, hw_brightness) |
> > FIELD_PREP(KBD_BL_COMMAND_TYPE, type) |
> > KBD_BL_COMMAND_SET;
> > err = exec_kblc(priv->adev->handle, value);
> > } else {
> > - err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> > + err = exec_sals(priv->adev->handle,
> > + hw_brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> > }
> >
> > + if (err)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > +static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> > +{
> > + int err;
> > +
> > + if (brightness > priv->kbd_bl.led.max_brightness)
> > + return -EINVAL;
> > +
> > + err = ideapad_kbd_bl_hw_brightness_set(priv, brightness);
> > if (err)
> > return err;
> >
> > @@ -1638,6 +1688,16 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
> > return ideapad_kbd_bl_brightness_set(priv, brightness);
> > }
> >
> > +static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned int brightness)
> > +{
> > + if (brightness == priv->kbd_bl.last_brightness)
> > + return;
> > +
> > + priv->kbd_bl.last_brightness = brightness;
> > +
> > + led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> > +}
> > +
> > static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> > {
> > int brightness;
> > @@ -1649,12 +1709,7 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> > if (brightness < 0)
> > return;
> >
> > - if (brightness == priv->kbd_bl.last_brightness)
> > - return;
> > -
> > - priv->kbd_bl.last_brightness = brightness;
> > -
> > - led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> > + ideapad_kbd_bl_notify_known(priv, brightness);
> > }
> >
> > static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> >
Thanks for your review,
Rong
Powered by blists - more mailing lists