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

Alan Jenkins
2009-09-25T16:54:40Z

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