Re: [RFC PATCH 1/2] Input: add msi-wmi driver to support hotkeys in MSI Windtop AE1900-WT

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

Hi Anisse,

On Wed, Dec 02, 2009 at 07:26:03PM +0100, Anisse Astier wrote:
> +
> +	if (jiffies_to_msecs(get_jiffies_64() - msi_wmi_time_last_press)
> +			> pression_timeout) {

Why don't you use time_after() instead of manual computation?

Also, what is the point of this? If you are trying to debounce the
buttons this will not quite work. To do debouncing properly you need to
store the value you just read and fire up a timer. When timer fires -
that's the stable value.

> +		printk(KERN_DEBUG
> +				"MSI WMI: event correctly received: %llu\n",
> +				obj->integer.value);

This is way too noisy for the mainline kernel, pr_debug() perhaps?

> +		msi_wmi_time_last_press = get_jiffies_64();
> +		key = msi_wmi_get_entry_by_scancode(obj->integer.value);
> +		input_report_key(msi_wmi_input_dev, key->keycode, 1);
> +		input_sync(msi_wmi_input_dev);
> +		input_report_key(msi_wmi_input_dev, key->keycode, 0);
> +		input_sync(msi_wmi_input_dev);
> +	}
> +}
> +
> +static int msi_wmi_getkeycode(struct input_dev *dev, int scancode, int *keycode)
> +{
> +	struct key_entry *key = msi_wmi_get_entry_by_scancode(scancode);
> +
> +	if (key && key->code) {
> +		*keycode = key->keycode;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int msi_wmi_setkeycode(struct input_dev *dev, int scancode, int keycode)
> +{
> +	struct key_entry *key;
> +	int old_keycode;
> +
> +	if (keycode < 0 || keycode > KEY_MAX)
> +		return -EINVAL;
> +
> +	key = msi_wmi_get_entry_by_scancode(scancode);
> +	if (key && key->code) {
> +		old_keycode = key->keycode;
> +		key->keycode = keycode;
> +		set_bit(keycode, dev->keybit);
> +		if (!msi_wmi_get_entry_by_keycode(old_keycode))
> +			clear_bit(old_keycode, dev->keybit);
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int __init msi_wmi_input_setup(void)
> +{
> +	struct key_entry *key;
> +	int err;
> +
> +	msi_wmi_input_dev = input_allocate_device();
> +
> +	msi_wmi_input_dev->name = "MSI WMI hotkeys";
> +	msi_wmi_input_dev->phys = "wmi/input0";
> +	msi_wmi_input_dev->id.bustype = BUS_HOST;
> +	msi_wmi_input_dev->getkeycode = msi_wmi_getkeycode;
> +	msi_wmi_input_dev->setkeycode = msi_wmi_setkeycode;
> +
> +	for (key = msi_wmi_keymap; key->code; key++) {
> +		set_bit(EV_KEY, msi_wmi_input_dev->evbit);
> +		set_bit(key->keycode, msi_wmi_input_dev->keybit);
> +	}
> +
> +	err = input_register_device(msi_wmi_input_dev);
> +
> +	if (err)
> +		input_free_device(msi_wmi_input_dev);
> +
> +	return err;
> +}
> +
> +static int __init msi_wmi_init(void)
> +{
> +	int err;
> +
> +	msi_wmi_time_last_press = get_jiffies_64();
> +	if (!wmi_has_guid(MSIWMI_EVENT_GUID)) {
> +		printk(KERN_ERR
> +		       "This machine doesn't have MSI-hotkeys through WMI\n");
> +		goto load_error;
> +	}
> +	err = wmi_install_notify_handler(MSIWMI_EVENT_GUID,
> +					 msi_wmi_notify, NULL);
> +	if (err) {
> +		printk(KERN_ERR
> +		       "MSI WMI: Error while installing notify handler\n");
> +		goto load_error;
> +	}
> +
> +	msi_wmi_input_setup();


You need to handle errors returned by msi_wmi_input_setup() as well.

> +
> +	return 0;
> +load_error:
> +	return -ENODEV;
> +}
> +
> +static void __exit msi_wmi_exit(void)
> +{
> +	if (wmi_has_guid(MSIWMI_EVENT_GUID)) {
> +		wmi_remove_notify_handler(MSIWMI_EVENT_GUID);
> +		input_unregister_device(msi_wmi_input_dev);
> +	}
> +}
> +
> +module_init(msi_wmi_init);
> +module_exit(msi_wmi_exit);
> +module_param(pression_timeout, uint, 0);
> +
> +MODULE_AUTHOR("Jérôme Pouiller <jezz@sysmic.org>");
> +MODULE_AUTHOR("Michael Bouchaud <michael@substantiel.fr");
> +MODULE_AUTHOR("Anisse Astier <anisse@astier.eu");
> +MODULE_DESCRIPTION("MSI all-in-one WMI hotkeys driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("wmi:" MSIWMI_EVENT_GUID);
> +/*This one is useless if there is wmi-autoloading*/
> +MODULE_ALIAS("dmi:bvnMICRO-STARINTERNATIONALCO.,LTD:*:svnMICRO-STARINTERNATIO"
> +	     "NALCO.,LTD:pnMS-6638:*:rnMS-7438:*:cvnMICRO-STARINTERNATIONALCO."
> +	     ",LTD:*");
> +MODULE_PARM_DESC(pression_timeout,
> +		 "How much time interrupts are ignored between each pression");

This is not the best option name:

MODULE_PARM_DESC(debounce_interval,
		 "Controls how long driver will wait for button to debounce");

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html