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
------------------------------------------------------------------------------
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
