Re: [PATCH] ACPI: New driver for Lenovo SL laptops

[Alan Jenkins <alan-jenkins@tuffmail.co.uk>]

On 9/24/09, Ike Panhc <ike.pan@canonical.com> wrote:
> lenovo-sl-laptop is a new driver that provides support for hotkeys,
> bluetooth,
> LenovoCare LEDs and fan speed on the Lenovo ThinkPad SL series laptops. The
> original author is Alexandre Rostovtsev. [1] In February 2009 Alexandre has
> posted the driver on the linux-acpi mailing list and and there was some
> feedback suggesting further modifications. [2] I would like to see Linux
> working properly on these laptops. I was encouraged to push this driver
> again
> with the modifications that where suggested in the responses to the initial
> post in order to allow me and others interested in that driver to improve it
> and hopefully get it included upstream.
>
> [1] homepage : http://github.com/tetromino/lenovo-sl-laptop/tree/master
> [2] http://patchwork.kernel.org/patch/7427/
>
> Following the suggestions when last time the origin author has posted on the
> linux-acpi mailing list. The major modification of this driver is listed
> below.
>  - Remove backlight control
>  - Remove procfs EC debug
>  - Remove fan control function
>  - Using generic debugging infrastructure
>  - Support for lastest rfkill infrastructure (by Alexandre)
>  - Register query function into EC for detecting hotkey event
>
> Patch against current checkout of linux-acpi 2.6.31 is below.
>
>

Hi again!  Here are a few comments on the non-rfkill bits of the driver.  It's not a comprehensive review, just a few nit-picks I noticed.


> +static const struct acpi_device_id hkey_ids[] = {
> +	{"LEN0014", 0},
> +	{"", 0},
> +};
> +
> +static struct acpi_driver hkey_driver = {
> +	.name = "lenovo-sl-laptop-hotkey",
> +	.class = "lenovo",
> +	.ids = hkey_ids,
> +	.ops = {
> +		.add = hkey_add,
> +		.remove = hkey_remove,
> +	},

I recently discovered a neglected .owner field in this struct.  Please set the .owner field to THIS_MODULE to get correct information under "/sys/module/leveno-sl-laptop/drivers"

> +};
> +
> +static void hkey_inputdev_exit(void)
> +{
> +	if (hkey_inputdev) {
> +		input_unregister_device(hkey_inputdev);
> +		input_free_device(hkey_inputdev);
> +		hkey_inputdev = NULL;
> +	}
> +}
> +
> +static int hkey_inputdev_init(void)
> +{
> +	int result;
> +	struct key_entry *key;
> +
> +	hkey_inputdev = input_allocate_device();
> +	if (!hkey_inputdev) {
> +		pr_err("Failed to allocate hotkey input device\n");
> +		return -ENODEV;
> +	}
> +	hkey_inputdev->name = "Lenovo ThinkPad SL Series extra buttons";
> +	hkey_inputdev->phys = LENSL_HKEY_FILE "/input0";
> +	hkey_inputdev->uniq = LENSL_HKEY_FILE;
> +	hkey_inputdev->id.bustype = BUS_HOST;
> +	hkey_inputdev->id.vendor = PCI_VENDOR_ID_LENOVO;

I never have any idea what these accomplish, but Dmitry didn't object to the values so I hope they're sane enough :-).  Anyway, how about adding

	hkey_inputdev->dev.parent = &lensl_pdev->dev;

It should make the input device show up under "/sys/devices/platform/lenovo-sl-laptop", instead of /sys/devices/virtual.

> +	set_bit(EV_KEY, hkey_inputdev->evbit);
> +
> +	for (key = ec_keymap; key->type != KE_END; key++)
> +		set_bit(key->keycode, hkey_inputdev->keybit);
> +
> +	result = input_register_device(hkey_inputdev);
> +	if (result) {
> +		pr_err("Failed to register hotkey input device\n");
> +		input_free_device(hkey_inputdev);
> +		hkey_inputdev = NULL;
> +		return -ENODEV;
> +	}
> +	pr_devel("Initialized hotkey subdriver\n");
> +	return 0;
> +}

...

> +static void __exit lenovo_sl_laptop_exit(void)
> +{
> +	hwmon_exit();
> +	led_exit();
> +	hkey_unregister_notify();
> +
> +	radio_exit(LENSL_UWB);
> +	radio_exit(LENSL_WWAN);
> +	radio_exit(LENSL_BLUETOOTH);
> +
> +	if (lensl_pdev)
> +		platform_device_unregister(lensl_pdev);
> +	destroy_workqueue(lensl_wq);
> +
> +	pr_info("Unloaded Lenovo ThinkPad SL Series driver\n");

What purpose does this message serve?

> +}
> +
> +MODULE_ALIAS("dmi:bvnLENOVO:*:svnLENOVO*:*:pvrThinkPad SL*:rvnLENOVO:*");
> +MODULE_ALIAS("dmi:bvnLENOVO:*:svnLENOVO*:*:pvrThinkPadSL*:rvnLENOVO:*");

Why can't you use

MODULE_DEVICE_TABLE(acpi, hkey_ids);

instead?

> +module_init(lenovo_sl_laptop_init);
> +module_exit(lenovo_sl_laptop_exit);

Best wishes
Alan