[PATCH 0/3] ideapad: hotkey enablement

[PATCH 0/3] ideapad: hotkey enablement

[Ike Panhc]

Here are the enablement patches for hotkey events on ideapads and these patches
are made against current checkout of mainline kernel.

These patches are available in the git repository at:
  git://kernel.ubuntu.com/ikepanhc/ideapad-laptop.git ideapad-laptop

Ike Panhc (3):
  ideapad: add platform driver for ideapad
  ideapad: let camera power control entry under platform driver
  ideapad: add hotkey support

 drivers/platform/x86/ideapad-laptop.c |  174 ++++++++++++++++++++++++++-------
 1 files changed, 140 insertions(+), 34 deletions(-)

[PATCH 1/3] ideapad: add platform driver for ideapad

[Ike Panhc]

Create /sys/devices/platform/Ideapad for nodes of ideapad landing.

Signed-off-by: Ike Panhc <ike.pan@canonical.com>
---
 drivers/platform/x86/ideapad-laptop.c |   59 ++++++++++++++++++++++++++------
 1 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 5ff1220..26edbdb 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -27,6 +27,7 @@
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 #include <linux/rfkill.h>
+#include <linux/platform_device.h>
 
 #define IDEAPAD_DEV_CAMERA	0
 #define IDEAPAD_DEV_WLAN	1
@@ -37,6 +38,7 @@
 struct ideapad_private {
 	acpi_handle handle;
 	struct rfkill *rfk[5];
+	struct platform_device *platform_device;
 } *ideapad_priv;
 
 static struct {
@@ -277,6 +279,35 @@ static void ideapad_unregister_rfkill(struct acpi_device *adevice, int dev)
 	rfkill_destroy(priv->rfk[dev]);
 }
 
+/*
+ * Platform device
+ */
+static int ideapad_platform_init(void)
+{
+	int result;
+
+	ideapad_priv->platform_device = platform_device_alloc("Ideapad", -1);
+	if (!ideapad_priv->platform_device)
+		return -ENOMEM;
+	platform_set_drvdata(ideapad_priv->platform_device, ideapad_priv);
+
+	result = platform_device_add(ideapad_priv->platform_device);
+	if (result)
+		goto fail_platform_device;
+
+	return 0;
+
+fail_platform_device:
+	platform_device_put(ideapad_priv->platform_device);
+	return result;
+}
+
+static void ideapad_platform_exit(void)
+{
+	platform_device_unregister(ideapad_priv->platform_device);
+}
+/* the above is platform device */
+
 static const struct acpi_device_id ideapad_device_ids[] = {
 	{ "VPC2004", 0},
 	{ "", 0},
@@ -285,7 +316,7 @@ MODULE_DEVICE_TABLE(acpi, ideapad_device_ids);
 
 static int ideapad_acpi_add(struct acpi_device *adevice)
 {
-	int i, cfg;
+	int ret, i, cfg;
 	int devs_present[5];
 	struct ideapad_private *priv;
 
@@ -305,18 +336,20 @@ static int ideapad_acpi_add(struct acpi_device *adevice)
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	ideapad_priv = priv;
+
+	ret = ideapad_platform_init();
+	if (ret)
+		goto platform_failed;
 
 	if (devs_present[IDEAPAD_DEV_CAMERA]) {
-		int ret = device_create_file(&adevice->dev, &dev_attr_camera_power);
-		if (ret) {
-			kfree(priv);
-			return ret;
-		}
+		ret = device_create_file(&adevice->dev, &dev_attr_camera_power);
+		if (ret)
+			goto camera_failed;
 	}
 
 	priv->handle = adevice->handle;
 	dev_set_drvdata(&adevice->dev, priv);
-	ideapad_priv = priv;
 	for (i = IDEAPAD_DEV_WLAN; i <= IDEAPAD_DEV_KILLSW; i++) {
 		if (!devs_present[i])
 			continue;
@@ -325,6 +358,12 @@ static int ideapad_acpi_add(struct acpi_device *adevice)
 	}
 	ideapad_sync_rfk_state(adevice);
 	return 0;
+
+camera_failed:
+	ideapad_platform_exit();
+platform_failed:
+	kfree(priv);
+	return ret;
 }
 
 static int ideapad_acpi_remove(struct acpi_device *adevice, int type)
@@ -337,6 +376,7 @@ static int ideapad_acpi_remove(struct acpi_device *adevice, int type)
 	for (i = IDEAPAD_DEV_WLAN; i <= IDEAPAD_DEV_KILLSW; i++)
 		ideapad_unregister_rfkill(adevice, i);
 
+	ideapad_platform_exit();
 	dev_set_drvdata(&adevice->dev, NULL);
 	kfree(priv);
 	return 0;
@@ -374,16 +414,13 @@ static struct acpi_driver ideapad_acpi_driver = {
 
 static int __init ideapad_acpi_module_init(void)
 {
-	acpi_bus_register_driver(&ideapad_acpi_driver);
-
-	return 0;
+	return acpi_bus_register_driver(&ideapad_acpi_driver);
 }
 
 
 static void __exit ideapad_acpi_module_exit(void)
 {
 	acpi_bus_unregister_driver(&ideapad_acpi_driver);
-
 }
 
 MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
-- 
1.7.1

[PATCH 2/3] ideapad: let camera power control entry under platform driver

[Ike Panhc]

The entry was at /sys/devices/LNXSYSTM:00/../VPC2004:00/camera_power
move to /sys/devices/platform/Ideapad/camera_power

Signed-off-by: Ike Panhc <ike.pan@canonical.com>
---
 drivers/platform/x86/ideapad-laptop.c |   55 +++++++++++++++------------------
 1 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 26edbdb..d75c21f 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -282,6 +282,15 @@ static void ideapad_unregister_rfkill(struct acpi_device *adevice, int dev)
 /*
  * Platform device
  */
+static struct attribute *ideapad_attributes[] = {
+	&dev_attr_camera_power.attr,
+	NULL
+};
+
+static struct attribute_group ideapad_attribute_group = {
+	.attrs = ideapad_attributes
+};
+
 static int ideapad_platform_init(void)
 {
 	int result;
@@ -295,8 +304,14 @@ static int ideapad_platform_init(void)
 	if (result)
 		goto fail_platform_device;
 
+	result = sysfs_create_group(&ideapad_priv->platform_device->dev.kobj,
+				    &ideapad_attribute_group);
+	if (result)
+		goto fail_sysfs;
 	return 0;
 
+fail_sysfs:
+	platform_device_del(ideapad_priv->platform_device);
 fail_platform_device:
 	platform_device_put(ideapad_priv->platform_device);
 	return result;
@@ -304,6 +319,8 @@ fail_platform_device:
 
 static void ideapad_platform_exit(void)
 {
+	sysfs_remove_group(&ideapad_priv->platform_device->dev.kobj,
+			   &ideapad_attribute_group);
 	platform_device_unregister(ideapad_priv->platform_device);
 }
 /* the above is platform device */
@@ -317,50 +334,30 @@ MODULE_DEVICE_TABLE(acpi, ideapad_device_ids);
 static int ideapad_acpi_add(struct acpi_device *adevice)
 {
 	int ret, i, cfg;
-	int devs_present[5];
 	struct ideapad_private *priv;
 
 	if (read_method_int(adevice->handle, "_CFG", &cfg))
 		return -ENODEV;
 
-	for (i = IDEAPAD_DEV_CAMERA; i < IDEAPAD_DEV_KILLSW; i++) {
-		if (test_bit(ideapad_rfk_data[i].cfgbit, (unsigned long *)&cfg))
-			devs_present[i] = 1;
-		else
-			devs_present[i] = 0;
-	}
-
-	/* The hardware switch is always present */
-	devs_present[IDEAPAD_DEV_KILLSW] = 1;
-
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 	ideapad_priv = priv;
+	priv->handle = adevice->handle;
+	dev_set_drvdata(&adevice->dev, priv);
 
 	ret = ideapad_platform_init();
 	if (ret)
 		goto platform_failed;
 
-	if (devs_present[IDEAPAD_DEV_CAMERA]) {
-		ret = device_create_file(&adevice->dev, &dev_attr_camera_power);
-		if (ret)
-			goto camera_failed;
-	}
-
-	priv->handle = adevice->handle;
-	dev_set_drvdata(&adevice->dev, priv);
-	for (i = IDEAPAD_DEV_WLAN; i <= IDEAPAD_DEV_KILLSW; i++) {
-		if (!devs_present[i])
-			continue;
-
-		ideapad_register_rfkill(adevice, i);
+	for (i = IDEAPAD_DEV_WLAN; i < IDEAPAD_DEV_KILLSW; i++) {
+		if (test_bit(ideapad_rfk_data[i].cfgbit, (unsigned long *)&cfg))
+			ideapad_register_rfkill(adevice, i);
 	}
 	ideapad_sync_rfk_state(adevice);
+
 	return 0;
 
-camera_failed:
-	ideapad_platform_exit();
 platform_failed:
 	kfree(priv);
 	return ret;
@@ -371,14 +368,12 @@ static int ideapad_acpi_remove(struct acpi_device *adevice, int type)
 	struct ideapad_private *priv = dev_get_drvdata(&adevice->dev);
 	int i;
 
-	device_remove_file(&adevice->dev, &dev_attr_camera_power);
-
-	for (i = IDEAPAD_DEV_WLAN; i <= IDEAPAD_DEV_KILLSW; i++)
+	for (i = IDEAPAD_DEV_WLAN; i < IDEAPAD_DEV_KILLSW; i++)
 		ideapad_unregister_rfkill(adevice, i);
-
 	ideapad_platform_exit();
 	dev_set_drvdata(&adevice->dev, NULL);
 	kfree(priv);
+
 	return 0;
 }
 
-- 
1.7.1

[PATCH 3/3] ideapad: add hotkey support

[Ike Panhc]

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)
+{
+	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;
+	return 0;
+
+err_free_keymap:
+	sparse_keymap_free(inputdev);
+err_free_dev:
+	input_free_device(inputdev);
+	return error;
+}
+
+static void ideapad_input_exit(void)
+{
+	if (ideapad_priv->inputdev) {
+		sparse_keymap_free(ideapad_priv->inputdev);
+		input_unregister_device(ideapad_priv->inputdev);
+	}
+	ideapad_priv->inputdev = NULL;
+}
+
+static void ideapad_input_report(unsigned long scancode)
+{
+	sparse_keymap_report_event(ideapad_priv->inputdev, scancode, 1, true);
+}
+/* the above is input device */
+
 static const struct acpi_device_id ideapad_device_ids[] = {
 	{ "VPC2004", 0},
 	{ "", 0},
@@ -350,6 +415,10 @@ static int ideapad_acpi_add(struct acpi_device *adevice)
 	if (ret)
 		goto platform_failed;
 
+	ret = ideapad_input_init();
+	if (ret)
+		goto input_failed;
+
 	for (i = IDEAPAD_DEV_WLAN; i < IDEAPAD_DEV_KILLSW; i++) {
 		if (test_bit(ideapad_rfk_data[i].cfgbit, (unsigned long *)&cfg))
 			ideapad_register_rfkill(adevice, i);
@@ -358,6 +427,8 @@ static int ideapad_acpi_add(struct acpi_device *adevice)
 
 	return 0;
 
+input_failed:
+	ideapad_platform_exit();
 platform_failed:
 	kfree(priv);
 	return ret;
@@ -370,6 +441,7 @@ static int ideapad_acpi_remove(struct acpi_device *adevice, int type)
 
 	for (i = IDEAPAD_DEV_WLAN; i < IDEAPAD_DEV_KILLSW; i++)
 		ideapad_unregister_rfkill(adevice, i);
+	ideapad_input_exit();
 	ideapad_platform_exit();
 	dev_set_drvdata(&adevice->dev, NULL);
 	kfree(priv);
@@ -392,6 +464,8 @@ static void ideapad_acpi_notify(struct acpi_device *adevice, u32 event)
 		if (test_bit(vpc_bit, &vpc1)) {
 			if (vpc_bit == 9)
 				ideapad_sync_rfk_state(adevice);
+			else
+				ideapad_input_report(vpc_bit);
 		}
 	}
 }
-- 
1.7.1

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

[Dmitry Torokhov]

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

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

[Corentin Chary]

On Wed, Nov 17, 2010 at 10:29 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.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@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?

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

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

[Ike Panhc]

On 11/18/2010 03:35 PM, Corentin Chary wrote:
> On Wed, Nov 17, 2010 at 10:29 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.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@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?
> 
> That may allow to remove the global later, like we did on
> eeepc-laptop/asus-laptop.

I am ok to go though the driver and try to avoid using global variable.

Other feedbacks is easy done, so I will fix them first..

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

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

[Dave Hansen]

On Wed, 2010-11-17 at 15:00 +0800, Ike Panhc wrote:
> +static void ideapad_input_report(unsigned long scancode)
> +{
> +       sparse_keymap_report_event(ideapad_priv->inputdev, scancode, 1, true);
> +} 

I got a little build error about some unresolved symbols in the module
while compiling:
        
        ERROR: "sparse_keymap_setup" [drivers/platform/x86/ideapad-laptop.ko] undefined!
        ERROR: "sparse_keymap_free" [drivers/platform/x86/ideapad-laptop.ko] undefined!
        ERROR: "sparse_keymap_report_event" [drivers/platform/x86/ideapad-laptop.ko] undefined!
        
You can get it to go away by setting CONFIG_INPUT_SPARSEKMAP=m or =y
in .config.  But, I think your patches need this:

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index faec777..879d266 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -226,6 +226,7 @@ config IDEAPAD_LAPTOP
 	tristate "Lenovo IdeaPad Laptop Extras"
 	depends on ACPI
 	depends on RFKILL
+	select INPUT_SPARSEKMAP
 	help
 	  This is a driver for the rfkill switches on Lenovo IdeaPad netbooks.
 

Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>

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

[Ike Panhc]

On 12/03/2010 07:41 AM, Dave Hansen wrote:
> On Wed, 2010-11-17 at 15:00 +0800, Ike Panhc wrote:
>> +static void ideapad_input_report(unsigned long scancode)
>> +{
>> +       sparse_keymap_report_event(ideapad_priv->inputdev, scancode, 1, true);
>> +} 
> 
> I got a little build error about some unresolved symbols in the module
> while compiling:
>         
>         ERROR: "sparse_keymap_setup" [drivers/platform/x86/ideapad-laptop.ko] undefined!
>         ERROR: "sparse_keymap_free" [drivers/platform/x86/ideapad-laptop.ko] undefined!
>         ERROR: "sparse_keymap_report_event" [drivers/platform/x86/ideapad-laptop.ko] undefined!
>         
> You can get it to go away by setting CONFIG_INPUT_SPARSEKMAP=m or =y
> in .config.  But, I think your patches need this:
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index faec777..879d266 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -226,6 +226,7 @@ config IDEAPAD_LAPTOP
>  	tristate "Lenovo IdeaPad Laptop Extras"
>  	depends on ACPI
>  	depends on RFKILL
> +	select INPUT_SPARSEKMAP
>  	help
>  	  This is a driver for the rfkill switches on Lenovo IdeaPad netbooks.
>  
> 
> Signed-off-by: Dave Hansen <dave@linux.vnet.ibm.com>
> 

Thanks, you are right