| Message ID | 1450807792-4980-4-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Tue, 22 Dec 2015, Hans de Goede wrote: > Use the new acpi_video_handles_brightness_key_presses function to check > if we should report brightness key-presses. > > This makes the code both easier to read and makes it properly report > key-presses when acpi-video is not reporting them for reasons other > then the backlight type being vendor. If this new function will return false when acpi video is not reporting keypresses *BUT* still allowing any sort of brightness changes (e.g. through sysfs), I don't think it is safe in thinkpad-acpi's case. So, will it return false in any situation whether acpi-video attached to the backlight class?
Hi, On 28-12-15 00:08, Henrique de Moraes Holschuh wrote: > On Tue, 22 Dec 2015, Hans de Goede wrote: >> Use the new acpi_video_handles_brightness_key_presses function to check >> if we should report brightness key-presses. >> >> This makes the code both easier to read and makes it properly report >> key-presses when acpi-video is not reporting them for reasons other >> then the backlight type being vendor. > > If this new function will return false when acpi video is not reporting > keypresses *BUT* still allowing any sort of brightness changes (e.g. through > sysfs), I don't think it is safe in thinkpad-acpi's case. Have you closely looked at the patch? It only applies to this bit of thinkpad-acpi code: /* Do not issue duplicate brightness change events to * userspace. tpacpi_detect_brightness_capabilities() must have * been called before this point */ if (acpi_video_handles_brightness_key_presses()) { pr_info("This ThinkPad has standard ACPI backlight " "brightness control, supported by the ACPI " "video driver\n"); pr_notice("Disabling thinkpad-acpi brightness events " "by default...\n"); /* 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_unmap(TP_ACPI_HOTKEYSCAN_FNHOME); hotkey_unmap(TP_ACPI_HOTKEYSCAN_FNEND); } So it only disables hotkey press reporting, not to the thinkpad_acpi backlight handling code, that still is using acpi_video_get_backlight_type() : if (acpi_video_get_backlight_type() != acpi_backlight_vendor) { if (brightness_enable > 1) { pr_info("Standard ACPI backlight interface " "available, not loading native one\n"); return 1; } else if (brightness_enable == 1) { pr_warn("Cannot enable backlight brightness support, " "ACPI is already handling it. Refer to the " "acpi_backlight kernel parameter.\n"); return 1; } } else if (tp_features.bright_acpimode && brightness_enable > 1) { pr_notice("Standard ACPI backlight interface not " "available, thinkpad_acpi native " "brightness control enabled\n"); } > So, will it return false in any situation whether acpi-video attached to the > backlight class? When the new video.report_key_events kernel cmdline option gets manually set to a non default value then yes it may report false while acpi-video is attached to the backlight class. By default, no it will never return false when acpi-video is attached to the backlight class. This should not matter though, as said this new function is only used to suppress the reporting of key-presses. I realize that doing so will also cause tpacpi_driver_event() to get called on brightness hotkey presses, but that has: if (ibm_backlight_device) { switch (hkey_event) { case TP_HKEY_EV_BRGHT_UP: case TP_HKEY_EV_BRGHT_DOWN: tpacpi_brightness_notify_change(); } } And the creating of ibm_backlight_device is still protected by if (acpi_video_get_backlight_type() != acpi_backlight_vendor) return 1; So this new check really does nothing other then NOT suppress the hotkey presses reported by thinkpad-acpi when for some reason a user has asked acpi-video to suppress its reporting of hotkey presses, which is exactly what it should do. IMHO this only shows that having a separate function to detect if acpi-video is reporting keypresses is the right thing to do. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 29, 2015, at 10:27, Hans de Goede wrote: > So it only disables hotkey press reporting, not to the thinkpad_acpi > backlight handling code, that still is using > acpi_video_get_backlight_type() : ... > This should not matter though, as said this new function is only used to > suppress > the reporting of key-presses. I realize that doing so will also cause > tpacpi_driver_event() to get called on brightness hotkey presses, but > that has: > > if (ibm_backlight_device) { > switch (hkey_event) { > case TP_HKEY_EV_BRGHT_UP: > case TP_HKEY_EV_BRGHT_DOWN: > tpacpi_brightness_notify_change(); > } > } > > And the creating of ibm_backlight_device is still protected by > > if (acpi_video_get_backlight_type() != acpi_backlight_vendor) return 1; Indeed. I have no objections to the patch, then. Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 0bed473..f453d5d 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -3488,7 +3488,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm) /* Do not issue duplicate brightness change events to * userspace. tpacpi_detect_brightness_capabilities() must have * been called before this point */ - if (acpi_video_get_backlight_type() != acpi_backlight_vendor) { + if (acpi_video_handles_brightness_key_presses()) { pr_info("This ThinkPad has standard ACPI backlight " "brightness control, supported by the ACPI " "video driver\n");
Use the new acpi_video_handles_brightness_key_presses function to check if we should report brightness key-presses. This makes the code both easier to read and makes it properly report key-presses when acpi-video is not reporting them for reasons other then the backlight type being vendor. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/thinkpad_acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)