Re: [PATCH 3/3] ideapad: add hotkey support

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@canonical.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?

> +	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.

-- 
Dmitry