Disclaimer: most of my comments are mostly small styles issue, and you
can probably
ignore them. But I'd like eeepc-wmi style to match asus-laptop and
eeepc-laptop, this way
it will be easier to identify common code pattern.
In fact while reviewing this patch, comparing eeepc_wmi_input_exit,
eeepc_input_exit and asus_input_exit
I found that kfree(eeepc->keymap); should not be here and sparse_keymap is not
freed (will add a patch for that soon).
If you don't [want /have the time] to do it, I'll will submit patchs
for that later, but it's better if it's done early.
On Fri, Apr 9, 2010 at 5:04 PM, Yong Wang <yong.y.wang@xxxxxxxxxxxxxxx> wrote:
> Add an eeepc_wmi context structure to manage all the sub-devices
> that will be implemented later on. Put input device into it first.
>
> Signed-off-by: Yong Wang <yong.y.wang@xxxxxxxxx>
> ---
> drivers/platform/x86/eeepc-wmi.c | 65 +++++++++++++++++++++++++++-----------
> 1 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/platform/x86/eeepc-wmi.c b/drivers/platform/x86/eeepc-wmi.c
> index 9f88226..f030410 100644
> --- a/drivers/platform/x86/eeepc-wmi.c
> +++ b/drivers/platform/x86/eeepc-wmi.c
> @@ -58,10 +58,13 @@ static const struct key_entry eeepc_wmi_keymap[] = {
> { KE_END, 0},
> };
>
> -static struct input_dev *eeepc_wmi_input_dev;
> +struct eeepc_wmi {
> + struct input_dev *input_dev;
We use "inputdev" in asus-laptop and eeepc-laptop.
> +};
>
> static void eeepc_wmi_notify(u32 value, void *context)
> {
> + struct eeepc_wmi *eeepc_wmi = (struct eeepc_wmi *)context;
Explicit cast is not needed here.
Also In eeepc-laptop laptop Alan named the context "eeepc',
in asus-laptop I choose "asus". Maybe eeepc_wmi is a little too long
and you could also use "eeepc".
> struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> union acpi_object *obj;
> acpi_status status;
> @@ -83,7 +86,7 @@ static void eeepc_wmi_notify(u32 value, void *context)
> else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
> code = NOTIFY_BRNDOWN_MIN;
>
> - if (!sparse_keymap_report_event(eeepc_wmi_input_dev,
> + if (!sparse_keymap_report_event(eeepc_wmi->input_dev,
> code, 1, true))
> pr_info("EEEPC WMI: Unknown key %x pressed\n", code);
> }
> @@ -91,37 +94,48 @@ static void eeepc_wmi_notify(u32 value, void *context)
> kfree(obj);
> }
>
> -static int eeepc_wmi_input_setup(void)
> +static int eeepc_wmi_input_init(struct eeepc_wmi *eeepc_wmi)
> {
> int err;
>
> - eeepc_wmi_input_dev = input_allocate_device();
> - if (!eeepc_wmi_input_dev)
> + eeepc_wmi->input_dev = input_allocate_device();
> + if (!eeepc_wmi->input_dev)
> return -ENOMEM;
>
> - eeepc_wmi_input_dev->name = "Eee PC WMI hotkeys";
> - eeepc_wmi_input_dev->phys = "wmi/input0";
> - eeepc_wmi_input_dev->id.bustype = BUS_HOST;
> + eeepc_wmi->input_dev->name = "Eee PC WMI hotkeys";
> + eeepc_wmi->input_dev->phys = "wmi/input0";
> + eeepc_wmi->input_dev->id.bustype = BUS_HOST;
>
> - err = sparse_keymap_setup(eeepc_wmi_input_dev, eeepc_wmi_keymap, NULL);
> + err = sparse_keymap_setup(eeepc_wmi->input_dev, eeepc_wmi_keymap, NULL);
> if (err)
> goto err_free_dev;
>
> - err = input_register_device(eeepc_wmi_input_dev);
> + err = input_register_device(eeepc_wmi->input_dev);
> if (err)
> goto err_free_keymap;
>
> return 0;
>
> err_free_keymap:
> - sparse_keymap_free(eeepc_wmi_input_dev);
> + sparse_keymap_free(eeepc_wmi->input_dev);
> err_free_dev:
> - input_free_device(eeepc_wmi_input_dev);
> + input_free_device(eeepc_wmi->input_dev);
> return err;
> }
>
> +static void eeepc_wmi_input_exit(struct eeepc_wmi *eeepc_wmi)
> +{
> + if (eeepc_wmi->input_dev) {
> + sparse_keymap_free(eeepc_wmi->input_dev);
> + input_unregister_device(eeepc_wmi->input_dev);
> + }
> +
> + eeepc_wmi->input_dev = NULL;
> +}
> +
> static int __init eeepc_wmi_init(void)
> {
> + static struct eeepc_wmi *eeepc_wmi;
> int err;
> acpi_status status;
>
> @@ -130,17 +144,27 @@ static int __init eeepc_wmi_init(void)
> return -ENODEV;
> }
>
> - err = eeepc_wmi_input_setup();
> - if (err)
> + eeepc_wmi = kzalloc(sizeof(struct eeepc_wmi), GFP_KERNEL);
> + if (!eeepc_wmi) {
> + pr_warning("EEEPC WMI: Unable to allocate eeepc_wmi device\n");
Is this message really needed ? Anyway we now this will never happen,
and modprobe will probably print the same thing.
In fact, another noisy message could also be removed:
pr_warning("EEEPC WMI: No known WMI GUID found\n");
And speaking of messages, you cand use the pr_fmt macro (see
eeepc-laptop for and example)
instead of adding "EEEPC WMI:" as a prefix for all pr_ calls.
> + return -ENOMEM;
> + }
> +
> + wmi_set_driver_data(EEEPC_WMI_EVENT_GUID, eeepc_wmi);
> +
> + err = eeepc_wmi_input_init(eeepc_wmi);
> + if (err) {
> + kfree(eeepc_wmi);
> return err;
> + }
>
> status = wmi_install_notify_handler(EEEPC_WMI_EVENT_GUID,
> - eeepc_wmi_notify, NULL);
> + eeepc_wmi_notify, eeepc_wmi);
> if (ACPI_FAILURE(status)) {
> - sparse_keymap_free(eeepc_wmi_input_dev);
> - input_unregister_device(eeepc_wmi_input_dev);
> pr_err("EEEPC WMI: Unable to register notify handler - %d\n",
> status);
> + eeepc_wmi_input_exit(eeepc_wmi);
> + kfree(eeepc_wmi);
> return -ENODEV;
> }
>
> @@ -149,9 +173,12 @@ static int __init eeepc_wmi_init(void)
>
> static void __exit eeepc_wmi_exit(void)
> {
> + static struct eeepc_wmi *eeepc_wmi;
> +
> + eeepc_wmi = wmi_get_driver_data(EEEPC_WMI_EVENT_GUID);
> wmi_remove_notify_handler(EEEPC_WMI_EVENT_GUID);
> - sparse_keymap_free(eeepc_wmi_input_dev);
> - input_unregister_device(eeepc_wmi_input_dev);
> + eeepc_wmi_input_exit(eeepc_wmi);
> + kfree(eeepc_wmi);
> }
>
> module_init(eeepc_wmi_init);
> --
> 1.5.5.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Corentin Chary
http://xf.iksaif.net
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
