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(-)
Create /sys/devices/platform/Ideapad for nodes of ideapad landing.
Signed-off-by: Ike Panhc <[email protected]>
---
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 <[email protected]>");
--
1.7.1
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Wed, Nov 17, 2010 at 10:29 PM, Dmitry Torokhov
<[email protected]> 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 <[email protected]>
>> ---
>> 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
On 11/18/2010 03:35 PM, Corentin Chary wrote:
> On Wed, Nov 17, 2010 at 10:29 PM, Dmitry Torokhov
> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
>
>
>
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 <[email protected]>
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 <[email protected]>
>
Thanks, you are right