Hi Stuart,
On 8/25/23 18:42, Stuart wrote:
> 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?
Either way is fine, just pick which way you prefer.
Regards,
Hans
>> 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.
>>
>
