Thanks for the quick review, I've addressed most of the comments for
V2, do you want that submitted in this thread, or should I create a
new one?
> Could these bits too be named with defines, it would be helpful for those
> reading the code?
> (If you can add the names to all these other bits too, it should be put
> into own patch and not into this one.)
Sorry, I have no idea about the other events. I can do this one if
you'd like, or leave a comment for the future if you'd rather they all
be done together.
On Fri, Aug 25, 2023 at 2:01 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, 25 Aug 2023, Stuart Hayhurst wrote:
>
> > Newer Lenovo laptops seem to use the KBLC symbol to control the backlight
> > Add support for handling the keyboard backlight on these devices
> >
> > Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@xxxxxxxxx>
> > ---
> >
> > This has been tested on both new types of keyboard backlight being supported.
> > KBD_BL_TRISTATE_AUTO is used for keyboards that support automatic brightness.
> > AUTO is reported as '0' with this patch, as it doesn't fit numerically, I'm not sure how else to
> > report AUTO, hopefully someone has some insight :)
> >
> > ---
> > drivers/platform/x86/ideapad-laptop.c | 107 ++++++++++++++++++++++++--
> > 1 file changed, 100 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> > index d2fee9a3e239..0e4cdd471a4a 100644
> > --- a/drivers/platform/x86/ideapad-laptop.c
> > +++ b/drivers/platform/x86/ideapad-laptop.c
> > @@ -85,6 +85,21 @@ enum {
> > SALS_FNLOCK_OFF = 0xf,
> > };
> >
> > +/*
> > + * 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
> > + */
> > +enum {
> > + KBD_BL_STANDARD = 1,
> > + KBD_BL_TRISTATE = 2,
> > + KBD_BL_TRISTATE_AUTO = 3,
> > +};
> > +
> > +#define KBD_BL_COMMAND_GET 0x2
> > +#define KBD_BL_COMMAND_SET 0x3
> > +
> > struct ideapad_dytc_priv {
> > enum platform_profile_option current_profile;
> > struct platform_profile_handler pprof;
> > @@ -122,6 +137,7 @@ struct ideapad_private {
> > } features;
> > struct {
> > bool initialized;
> > + int type;
> > struct led_classdev led;
> > unsigned int last_brightness;
> > } kbd_bl;
> > @@ -242,6 +258,16 @@ static int exec_sals(acpi_handle handle, unsigned long arg)
> > return exec_simple_method(handle, "SALS", arg);
> > }
> >
> > +static int exec_kblc(acpi_handle handle, unsigned long arg)
> > +{
> > + return exec_simple_method(handle, "KBLC", arg);
> > +}
> > +
> > +static int eval_kblc(acpi_handle handle, unsigned long cmd, unsigned long *res)
> > +{
> > + return eval_int_with_arg(handle, "KBLC", cmd, res);
> > +}
> > +
> > static int eval_dytc(acpi_handle handle, unsigned long cmd, unsigned long *res)
> > {
> > return eval_int_with_arg(handle, "DYTC", cmd, res);
> > @@ -1272,14 +1298,42 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
> > */
> > static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> > {
> > - unsigned long hals;
> > + unsigned long value;
> > int err;
> >
> > - err = eval_hals(priv->adev->handle, &hals);
> > + if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> > + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
> > + err = eval_kblc(priv->adev->handle,
> > + (priv->kbd_bl.type << 4) | KBD_BL_COMMAND_GET,
> > + &value);
> > +
> > + if (err)
> > + return err;
> > +
> > + /* Convert returned value to brightness level */
> > + value = (value & 0xFFFF) >> 1;
>
> You should define a field for thiswith GENMASK() and use FIELD_GET()
> instead of manual masking and shifting.
>
> > +
> > + if (value >= 0 && value <= 2) {
>
> How can unsigned long be < 0??
>
> If that 2 is the same as priv->kbd_bl.led.max_brightness, it would make
> sense to use it rather than literal.
>
> > + /* Off, low or high */
> > + return value;
> > + } else if (value == 3) {
> > + /* Auto, report as off */
> > + return 0;
> > + } else {
>
> Since those blocks above return, the elses are unnecessary.
>
> > + /* Unknown value */
> > + dev_warn(&priv->platform_device->dev,
> > + "Unknown keyboard backlight value: %i",
> > + value);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > +
>
> Remove one of the newlines.
>
> > + err = eval_hals(priv->adev->handle, &value);
> > if (err)
> > return err;
> >
> > - return !!test_bit(HALS_KBD_BL_STATE_BIT, &hals);
> > + return !!test_bit(HALS_KBD_BL_STATE_BIT, &value);
> > }
> >
> > static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
> > @@ -1291,13 +1345,27 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
> >
> > static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> > {
> > - int err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> > + int err;
> > + unsigned long value;
> > + int type = priv->kbd_bl.type;
> > +
> > + if (type == KBD_BL_TRISTATE ||
> > + type == KBD_BL_TRISTATE_AUTO) {
> > + if (brightness >= 0 && brightness <= 2) {
>
> Brightness is unsigned int so no need for < 0 check.
>
> Reverse the logic here:
>
> if (brightness > 2)
> return -EINVAL;
>
> ...as it avoid the need to use else.
>
> Consider using .max_brightness here too.
>
> > + value = (brightness << 16) | (type << 4) | KBD_BL_COMMAND_SET;
>
> There would also be a readability benefit for these if you define these
> as fields and use FIELD_PREP().
>
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + err = exec_kblc(priv->adev->handle, value);
> > + } else {
> > + err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> > + }
> >
> > if (err)
> > return err;
> >
> > priv->kbd_bl.last_brightness = brightness;
> > -
>
> Stray change.
>
> > return 0;
> > }
> >
> > @@ -1344,8 +1412,14 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> >
> > priv->kbd_bl.last_brightness = brightness;
> >
> > + if (priv->kbd_bl.type == KBD_BL_TRISTATE ||
> > + priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO) {
>
> Please add a helper for this check as it seems to appear multiple times in
> this patch.
>
> > + priv->kbd_bl.led.max_brightness = 2;
> > + } else {
> > + priv->kbd_bl.led.max_brightness = 1;
> > + }
> > +
> > priv->kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
> > - priv->kbd_bl.led.max_brightness = 1;
> > priv->kbd_bl.led.brightness_get = ideapad_kbd_bl_led_cdev_brightness_get;
> > priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
> > priv->kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED;
> > @@ -1456,6 +1530,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
> > case 2:
> > ideapad_backlight_notify_power(priv);
> > break;
> > + case 12:
>
> Could these bits too be named with defines, it would be helpful for those
> reading the code?
>
> (If you can add the names to all these other bits too, it should be put
> into own patch and not into this one.)
>
> > case 1:
> > /*
> > * Some IdeaPads report event 1 every ~20
> > @@ -1557,13 +1632,31 @@ static void ideapad_check_features(struct ideapad_private *priv)
> > if (test_bit(HALS_FNLOCK_SUPPORT_BIT, &val))
> > priv->features.fn_lock = true;
> >
> > - if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val))
> > + if (test_bit(HALS_KBD_BL_SUPPORT_BIT, &val)) {
> > priv->features.kbd_bl = true;
> > + priv->kbd_bl.type = KBD_BL_STANDARD;
> > + }
> >
> > if (test_bit(HALS_USB_CHARGING_SUPPORT_BIT, &val))
> > priv->features.usb_charging = true;
> > }
> > }
> > +
> > + if (acpi_has_method(handle, "KBLC")) {
> > + if (!eval_kblc(priv->adev->handle, 0x1, &val)) {
> > + if (val == 0x5) {
> > + priv->features.kbd_bl = true;
> > + priv->kbd_bl.type = KBD_BL_TRISTATE;
> > + } else if (val == 0x7) {
>
> Name these three literals with defines.
>
> > + priv->features.kbd_bl = true;
> > + priv->kbd_bl.type = KBD_BL_TRISTATE_AUTO;
> > + } else {
> > + dev_warn(&priv->platform_device->dev,
> > + "Unknown keyboard type: %i",
> > + val);
> > + }
> > + }
> > + }
> > }
> >
> > #if IS_ENABLED(CONFIG_ACPI_WMI)
> >
>
> --
> i.
>
