Re: [PATCH] i915: Fix opregion notifications

[Keith Packard <keithp@keithp.com>]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

On Tue, 12 Jul 2011 17:51:36 -0400, Matthew Garrett <mjg@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;

        ...
        case ACPI_VIDEO_NOTIFY_SWITCH:
                acpi_bus_generate_proc_event(device, event, 0);
                keycode = KEY_SWITCHVIDEOMODE;
                check_call_chain = true
                break;
        ...

        if (acpi_notifier_call_chain(device, event, 0) < 0)
                if (check_call_chain)
                        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@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]