On 9/24/09, Ike Panhc <ike.pan@xxxxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
