[Intel-gfx] [PATCH] i915: Fix opregion notifications
keithp at keithp.com
Wed Jul 13 00:20:23 CEST 2011
On Tue, 12 Jul 2011 17:51:36 -0400, Matthew Garrett <mjg at redhat.com> wrote:
> - keycode = KEY_SWITCHVIDEOMODE;
> + if (!acpi_notifier_call_chain(device, event, 0))
> + keycode = KEY_SWITCHVIDEOMODE;
Right, acpi_notify_call_chain returns -EINVAL when the underlying
function call returns NOTIFY_BAD.
> - acpi_notifier_call_chain(device, event, 0);
> + if (event != ACPI_VIDEO_NOTIFY_SWITCH)
> + acpi_notifier_call_chain(device, event, 0);
Not ideal to have two calls to this function only one of which will be
used. Could look like
bool check_call_chain = false;
acpi_bus_generate_proc_event(device, event, 0);
keycode = KEY_SWITCHVIDEOMODE;
check_call_chain = true
if (acpi_notifier_call_chain(device, event, 0) < 0)
keycode = 0;
Feel free to just call me for bikeshedding.
> + if (strcmp(event->device_class, ACPI_VIDEO_CLASS))
> + return NOTIFY_DONE;
I'm not entirely clear on accepted coding style here, but I'd much
rather this look like:
if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
I must have read this four times before I figured out that you weren't
early returning on all "video" devices...
> + if (event->type == 0x80 && !(acpi->cevt & 0x1))
> + ret = NOTIFY_BAD;
Seriously? It's some kind of magic non-switch switch event? And you can
tell by checking magic bits within the event?
How can I test this and know if it works?
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: not available
More information about the Intel-gfx