linux-kernel - Re: [PATCH 3/3] ideapad: add hotkey support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTim1DKZkCqB1mLg5=7UTd2szCoSS_DSO8v3jHTmy@mail.gmail.com>
Date:	Thu, 18 Nov 2010 08:35:58 +0100
From:	Corentin Chary <corentin.chary@...il.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Ike Panhc <ike.pan@...onical.com>, linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org,
	Matthew Garrett <mjg@...hat.com>,
	David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH 3/3] ideapad: add hotkey support

On Wed, Nov 17, 2010 at 10:29 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Wed, Nov 17, 2010 at 03:00:38PM +0800, Ike Panhc wrote:
>> Hotkey enabled by this patch:
>>   Fn+F3: Video mode switch
>>   Fn+F5: software rfkill for wifi
>>
>> For some ideapad when push Fn+F3, hardware generates Super-P keys, those key
>> will not be enabled by this patch.
>>
>> Signed-off-by: Ike Panhc <ike.pan@...onical.com>
>> ---
>>  drivers/platform/x86/ideapad-laptop.c |   74 +++++++++++++++++++++++++++++++++
>>  1 files changed, 74 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> index d75c21f..d397034 100644
>> --- a/drivers/platform/x86/ideapad-laptop.c
>> +++ b/drivers/platform/x86/ideapad-laptop.c
>> @@ -28,6 +28,8 @@
>>  #include <acpi/acpi_drivers.h>
>>  #include <linux/rfkill.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/input.h>
>> +#include <linux/input/sparse-keymap.h>
>>
>>  #define IDEAPAD_DEV_CAMERA   0
>>  #define IDEAPAD_DEV_WLAN     1
>> @@ -39,6 +41,7 @@ struct ideapad_private {
>>       acpi_handle handle;
>>       struct rfkill *rfk[5];
>>       struct platform_device *platform_device;
>> +     struct input_dev *inputdev;
>>  } *ideapad_priv;
>>
>>  static struct {
>> @@ -325,6 +328,68 @@ static void ideapad_platform_exit(void)
>>  }
>>  /* the above is platform device */
>>
>> +/*
>> + * input device
>> + */
>> +static const struct key_entry ideapad_keymap[] = {
>> +     { KE_KEY, 0x06, { KEY_SWITCHVIDEOMODE } },
>> +     { KE_KEY, 0x0D, { KEY_WLAN } },
>> +     { KE_END, 0 },
>> +};
>> +
>> +static int ideapad_input_init(void)
>
> __devinit?
>
>> +{
>> +     struct input_dev *inputdev;
>> +     int error;
>> +
>> +     inputdev = input_allocate_device();
>> +     if (!inputdev) {
>> +             pr_info("Unable to allocate input device\n");
>> +             return -ENOMEM;
>> +     }
>> +
>> +     inputdev->name = "Ideapad extra buttons";
>> +     inputdev->phys = "ideapad/input0";
>> +     inputdev->id.bustype = BUS_HOST;
>> +     inputdev->dev.parent = &ideapad_priv->platform_device->dev;
>> +
>> +     error = sparse_keymap_setup(inputdev, ideapad_keymap, NULL);
>> +     if (error) {
>> +             pr_err("Unable to setup input device keymap\n");
>> +             goto err_free_dev;
>> +     }
>> +
>> +     error = input_register_device(inputdev);
>> +     if (error) {
>> +             pr_err("Unable to register input device\n");
>> +             goto err_free_keymap;
>> +     }
>> +
>> +     ideapad_priv->inputdev = inputdev;
>
> Why don't you pass ideapad_priv in as an argument instead of relying on
> global. I know that there can only be one, but then why do you have a
> structure?

That may allow to remove the global later, like we did on
eeepc-laptop/asus-laptop.

>> +     return 0;
>> +
>> +err_free_keymap:
>> +     sparse_keymap_free(inputdev);
>> +err_free_dev:
>> +     input_free_device(inputdev);
>> +     return error;
>> +}
>> +
>> +static void ideapad_input_exit(void)
>
> __devexit? The rest of the driver might user these markups as well.
>
>> +{
>> +     if (ideapad_priv->inputdev) {
>> +             sparse_keymap_free(ideapad_priv->inputdev);
>> +             input_unregister_device(ideapad_priv->inputdev);
>> +     }
>> +     ideapad_priv->inputdev = NULL;
>
> You fail driver initialization when ideapad_input_init() fails so there
> is no point in checking whether ideapad_priv->inputdev is null or not.
>
> Otherwise looks good from input POV.
>
> Thanks.



-- 
Corentin Chary
http://xf.iksaif.net
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/