From: Hans de Goede <hdegoede@redhat.com>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Andy Shevchenko <andy@kernel.org>,
Mark Pearson <mpearson-lenovo@squebb.ca>,
Henrique de Moraes Holschuh <hmh@hmh.eng.br>,
Vishnu Sankar <vishnuocv@gmail.com>,
Nitin Joshi <njoshi1@lenovo.com>,
ibm-acpi-devel@lists.sourceforge.net,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v2 16/24] platform/x86: thinkpad_acpi: Change hotkey_reserved_mask initialization
Date: Mon, 29 Apr 2024 11:52:33 +0200 [thread overview]
Message-ID: <b00d7c9c-db55-4337-84f1-fd1d19c00859@redhat.com> (raw)
In-Reply-To: <d56986cf-e400-4f8f-d2aa-0fb1bba297cf@linux.intel.com>
Hi Ilpo,
Thank you for reviewing this series.
On 4/25/24 11:14 AM, Ilpo Järvinen wrote:
> On Wed, 24 Apr 2024, Hans de Goede wrote:
>
>> Change the hotkey_reserved_mask initialization to hardcode the list
>> of reserved keys. There are only a few reserved keys and the code to
>> iterate over the keymap will be removed when moving to sparse-keymaps.
>
> Hi,
>
> Consider extending this explanation. It's hard to see how the old and new
> code are identical because there are more KEY_RESERVED entries in the
> array than in the new code. I can see the list of keys in the new code
> matches to those set using tpacpi_hotkey_driver_mask_set() but it's hard
> to connect the dots here.
Right, this is basically the same question as which Mark asked. I've added
the following to the commit message while merging this series to clarify this:
"""
Note only the 32 original hotkeys are affected by any of the hotkey_*_mask
values, note:
if (i < sizeof(hotkey_reserved_mask)*8)
hotkey_reserved_mask |= 1 << i;
The (i < sizeof(hotkey_reserved_mask)*8) condition translates to (i < 32)
so this code only ever set bits in hotkey_reserved_mask for the 32 original
hotkeys. Therefor this patch does not set any bits in hotkey_reserved_mask
for the KEY_RESERVED mappings for the adaptive keyboard scancodes.
"""
>
>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/platform/x86/thinkpad_acpi.c | 21 +++++++++++++++------
>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index 952bac635a18..cf5c741d1343 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -3545,6 +3545,19 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>> dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY,
>> "using keymap number %lu\n", keymap_id);
>>
>> + /* Keys which should be reserved on both IBM and Lenovo models */
>> + hotkey_reserved_mask = TP_ACPI_HKEY_KBD_LIGHT_MASK |
>> + TP_ACPI_HKEY_VOLUP_MASK |
>> + TP_ACPI_HKEY_VOLDWN_MASK |
>> + TP_ACPI_HKEY_MUTE_MASK;
>> + /*
>> + * Reserve brightness up/down unconditionally on IBM models, on Lenovo
>> + * models these are disabled based on acpi_video_get_backlight_type().
>> + */
>> + if (keymap_id == TPACPI_KEYMAP_IBM_GENERIC)
>> + hotkey_reserved_mask |= TP_ACPI_HKEY_BRGHTUP_MASK |
>> + TP_ACPI_HKEY_BRGHTDWN_MASK;
>> +
>> hotkey_keycode_map = kmemdup(&tpacpi_keymaps[keymap_id],
>> TPACPI_HOTKEY_MAP_SIZE, GFP_KERNEL);
>> if (!hotkey_keycode_map) {
>> @@ -3560,9 +3573,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>> if (hotkey_keycode_map[i] != KEY_RESERVED) {
>> input_set_capability(tpacpi_inputdev, EV_KEY,
>> hotkey_keycode_map[i]);
>> - } else {
>> - if (i < sizeof(hotkey_reserved_mask)*8)
>> - hotkey_reserved_mask |= 1 << i;
>> }
>> }
>>
>> @@ -3587,9 +3597,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>> /* Disable brightness up/down on Lenovo thinkpads when
>> * ACPI is handling them, otherwise it is plain impossible
>> * for userspace to do something even remotely sane */
>> - hotkey_reserved_mask |=
>> - (1 << TP_ACPI_HOTKEYSCAN_FNHOME)
>> - | (1 << TP_ACPI_HOTKEYSCAN_FNEND);
>> + hotkey_reserved_mask |= TP_ACPI_HKEY_BRGHTUP_MASK |
>> + TP_ACPI_HKEY_BRGHTDWN_MASK;
>
> This is a simple define replace that would belong to own patch?
This makes the code style identical to how the brightness keys are added
to the mask above. I guess this could have been in a separate patch, but
since it is less work for me to just leave it here I'm going to leave it here.
Regards,
Hans
next prev parent reply other threads:[~2024-04-29 9:52 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 12:28 [PATCH v2 00/24] platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys Hans de Goede
2024-04-24 12:28 ` [PATCH v2 01/24] platform/x86: thinkpad_acpi: Take hotkey_mutex during hotkey_exit() Hans de Goede
2024-04-24 12:28 ` [PATCH v2 02/24] platform/x86: thinkpad_acpi: Provide hotkey_poll_stop_sync() dummy Hans de Goede
2024-04-24 12:28 ` [PATCH v2 03/24] platform/x86: thinkpad_acpi: Drop setting send_/ignore_acpi_ev defaults twice Hans de Goede
2024-04-24 12:28 ` [PATCH v2 04/24] platform/x86: thinkpad_acpi: Drop ignore_acpi_ev Hans de Goede
2024-04-25 7:13 ` Ilpo Järvinen
2024-04-29 9:34 ` Hans de Goede
2024-04-24 12:28 ` [PATCH v2 05/24] platform/x86: thinkpad_acpi: Use tpacpi_input_send_key() in adaptive kbd code Hans de Goede
2024-04-24 12:28 ` [PATCH v2 06/24] platform/x86: thinkpad_acpi: Do hkey to scancode translation later Hans de Goede
2024-04-24 12:28 ` [PATCH v2 07/24] platform/x86: thinkpad_acpi: Make tpacpi_driver_event() return if it handled the event Hans de Goede
2024-04-24 12:28 ` [PATCH v2 08/24] platform/x86: thinkpad_acpi: Move adaptive kbd event handling to tpacpi_driver_event() Hans de Goede
2024-04-24 12:28 ` [PATCH v2 09/24] platform/x86: thinkpad_acpi: Move special original hotkeys handling out of switch-case Hans de Goede
2024-04-24 12:28 ` [PATCH v2 10/24] platform/x86: thinkpad_acpi: Move hotkey_user_mask check to tpacpi_input_send_key() Hans de Goede
2024-04-24 12:28 ` [PATCH v2 11/24] platform/x86: thinkpad_acpi: Always call tpacpi_driver_event() for hotkeys Hans de Goede
2024-04-24 12:28 ` [PATCH v2 12/24] platform/x86: thinkpad_acpi: Drop tpacpi_input_send_key_masked() and hotkey_driver_event() Hans de Goede
2024-04-24 12:28 ` [PATCH v2 13/24] platform/x86: thinkpad_acpi: Move hkey > scancode mapping to tpacpi_input_send_key() Hans de Goede
2024-04-24 12:28 ` [PATCH v2 14/24] platform/x86: thinkpad_acpi: Move tpacpi_driver_event() call " Hans de Goede
2024-04-24 12:28 ` [PATCH v2 15/24] platform/x86: thinkpad_acpi: Do not send ACPI netlink events for unknown hotkeys Hans de Goede
2024-04-25 8:51 ` Ilpo Järvinen
2024-04-24 12:28 ` [PATCH v2 16/24] platform/x86: thinkpad_acpi: Change hotkey_reserved_mask initialization Hans de Goede
2024-04-24 14:17 ` Mark Pearson
2024-04-24 14:47 ` Hans de Goede
2024-04-24 15:11 ` Mark Pearson
2024-04-25 9:14 ` Ilpo Järvinen
2024-04-29 9:52 ` Hans de Goede [this message]
2024-04-29 10:06 ` Ilpo Järvinen
2024-04-24 12:28 ` [PATCH v2 17/24] platform/x86: thinkpad_acpi: Use correct keycodes for volume and brightness keys Hans de Goede
2024-04-24 12:28 ` [PATCH v2 18/24] platform/x86: thinkpad_acpi: Drop KEY_RESERVED special handling Hans de Goede
2024-04-24 12:28 ` [PATCH v2 19/24] platform/x86: thinkpad_acpi: Switch to using sparse-keymap helpers Hans de Goede
2024-04-24 12:28 ` [PATCH v2 20/24] platform/x86: thinkpad_acpi: Add mappings for adaptive kbd clipping-tool and cloud keys Hans de Goede
2024-04-24 12:28 ` [PATCH v2 21/24] platform/x86: thinkpad_acpi: Simplify known_ev handling Hans de Goede
2024-04-24 12:28 ` [PATCH v2 22/24] platform/x86: thinkpad_acpi: Support for trackpoint doubletap Hans de Goede
2024-04-24 18:19 ` Mark Pearson
2024-04-29 9:57 ` Hans de Goede
2024-04-29 12:40 ` Mark Pearson
2024-04-24 12:28 ` [PATCH v2 23/24] platform/x86: thinkpad_acpi: Support for system debug info hotkey Hans de Goede
2024-04-24 12:28 ` [PATCH v2 24/24] platform/x86: thinkpad_acpi: Support hotkey to disable trackpoint doubletap Hans de Goede
2024-04-24 18:28 ` [PATCH v2 00/24] platform/x86: thinkpad_acpi: Refactor hotkey handling and add support for some new hotkeys Mark Pearson
2024-04-25 9:31 ` Ilpo Järvinen
2024-04-29 10:03 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b00d7c9c-db55-4337-84f1-fd1d19c00859@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy@kernel.org \
--cc=hmh@hmh.eng.br \
--cc=ibm-acpi-devel@lists.sourceforge.net \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=mpearson-lenovo@squebb.ca \
--cc=njoshi1@lenovo.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=vishnuocv@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.