Re: [PATCH v5 1/1] input: ideapad_slidebar: new input driver

Hi Andrey,

On Mon, Aug 12, 2013 at 02:23:23PM +0400, Andrey Moiseev wrote:

A blank line between variables and code is appreciated.

+	spin_lock_irqsave(&sio_lock, flags);
+	outb(0xf4, 0xff29);
+	outb(0xbf, 0xff2a);
+	res = inb(0xff2b);
+	spin_unlock_irqrestore(&sio_lock, flags);
+	return res;
+}
+
+static unsigned char slidebar_mode_get(void)
+{
+	int res;
+	unsigned long flags;
+	spin_lock_irqsave(&sio_lock, flags);
+	outb(0xf7, 0xff29);
+	outb(0x8b, 0xff2a);
+	res = inb(0xff2b);
+	spin_unlock_irqrestore(&sio_lock, flags);
+	return res;
+}
+
+static void slidebar_mode_set(unsigned char mode)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&sio_lock, flags);
+	outb(0xf7, 0xff29);
+	outb(0x8b, 0xff2a);
+	outb(mode, 0xff2b);
+	spin_unlock_irqrestore(&sio_lock, flags);
+}
+
+/* Listening the keyboard (i8042 filter) */
+static bool slidebar_i8042_filter(unsigned char data, unsigned char str,
+				struct serio *port)
+{
+	static bool extended = false, touched = false;
+
+	/* Scancodes: e03b on move, e0bb on release */
+	if (unlikely(data == 0xe0)) {
+		extended = true;
+		return false;
+	} else if (unlikely(extended && (data == 0x3b))) {
+		extended = false;
+		if (!touched)
+			input_report_key(slidebar_input_dev, BTN_TOUCH, 1);
+		touched = true;
+		input_report_abs(slidebar_input_dev, ABS_X, slidebar_pos_get());
+		input_sync(slidebar_input_dev);
+		return false;
+	} else if (unlikely(extended && (data == 0xbb))) {
+		touched = false;
+		input_report_key(slidebar_input_dev, BTN_TOUCH, 0);
+		input_sync(slidebar_input_dev);
+	}
+
+	return false;
+}
+
+/* Input device */
+static int setup_input_dev(void)
+{
+	int err;
+
+	slidebar_input_dev = input_allocate_device();
+	if (!slidebar_input_dev) {
+		pr_err("ideapad_slidebar: Not enough memory\n");
+		return -ENOMEM;
+	}
+
+	slidebar_input_dev->name = "IdeaPad Slidebar";
+	slidebar_input_dev->id.bustype = BUS_HOST;
+	slidebar_input_dev->dev.parent = &slidebar_platform_dev->dev;
+	input_set_capability(slidebar_input_dev, EV_KEY, BTN_TOUCH);
+	input_set_capability(slidebar_input_dev, EV_ABS, ABS_X);
+	input_alloc_absinfo(slidebar_input_dev);

This call is not needed, it will be done for you.

+	input_set_abs_params(slidebar_input_dev, ABS_X, 0, 0xff, 0, 0);
+
+	err = input_register_device(slidebar_input_dev);
+	if (err) {
+		pr_err("ideapad_slidebar: Failed to register device\n");
+		goto err_free_dev;
+	}
+
+	err = i8042_install_filter(slidebar_i8042_filter);
+	if (err) {
+		pr_err("ideapad_slidebar: Can't install i8042 filter \n");
+		goto err_unregister_dev;
+	}
+	return 0;
+
+err_unregister_dev:
+	input_unregister_device(slidebar_input_dev);
+err_free_dev:
+	input_free_device(slidebar_input_dev);

No free after unregister for input devices. Consider swapping
registering device and installing the filter (allocated but not
registered input device can take events emitted by the filter just
fine).

+	return err;
+}
+
+static void remove_input_dev(void)
+{
+	i8042_remove_filter(slidebar_i8042_filter);
+	input_unregister_device(slidebar_input_dev);
+	input_free_device(slidebar_input_dev);

No free after unregister for input devices.

+}
+
+/* Sysfs slidebar_mode interface */
+static ssize_t show_slidebar_mode(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "%x\n", slidebar_mode_get());
+}
+
+static ssize_t store_slidebar_mode(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	int mode;
+	if (!count)
+		return 0;
+
+	if (sscanf(buf, "%x", &mode) != 1)
+		return -EINVAL;
+
+	slidebar_mode_set(mode);
+	return count;
+}
+
+static DEVICE_ATTR(slidebar_mode, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH,
+				show_slidebar_mode, store_slidebar_mode);
+
+static struct attribute *ideapad_attributes[] = {
+	&dev_attr_slidebar_mode.attr,
+	NULL
+};
+
+static struct attribute_group ideapad_attribute_group = {
+	.attrs = ideapad_attributes
+};
+
+static int setup_slidebar_mode_dev(void)
+{
+	return sysfs_create_group(&slidebar_platform_dev->dev.kobj,
+				  &ideapad_attribute_group);

To avoid races between creating device and attributes you want to simply
assign your group to the devices group pointer and then it will be
created for you by device core.

+}
+
+static void remove_slidebar_mode_dev(void)
+{
+	return sysfs_remove_group(&slidebar_platform_dev->dev.kobj,
+				  &ideapad_attribute_group);
+}
+
+/* Platform device */
+static int setup_platform_dev(void)
+{
+	int err;
+	slidebar_platform_dev = platform_device_alloc("ideapad_slidebar", -1);
+	if (!slidebar_platform_dev) {
+		pr_err("ideapad_slidebar: Not enough memory\n");
+		return -ENOMEM;
+	}
+
+	err = platform_device_add(slidebar_platform_dev);
+	if (err) {
+		pr_err("ideapad_slidebar: Failed to register plarform device\n");
+		goto err_free_platform_device;
+	}
+	return 0;
+
+err_free_platform_device:
+	platform_device_put(slidebar_platform_dev);
+	return err;
+}
+
+static void remove_platform_dev(void)
+{
+	platform_device_del(slidebar_platform_dev);
+	platform_device_put(slidebar_platform_dev);

I do nto understand why you need all these wrappers when you can simply
call platform_device_unregister().

+}
+
+/* Platform driver */
+static struct platform_driver slidebar_drv = {
+	.driver = {
+		.name = "ideapad_slidebar",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int register_platform_drv(void)
+{
+	int err;
+	err = platform_driver_register(&slidebar_drv);
+	if (err)
+		pr_err("ideapad_slidebar: Failed to register platform driver\n");
+
+	return err;
+}
+
+static void unregister_platform_drv(void)
+{
+	platform_driver_unregister(&slidebar_drv);
+}
+
+/* DMI */
+static int ideapad_dmi_check(const struct dmi_system_id *id)
+{
+	pr_info("ideapad_slidebar: Laptop model '%s'\n", id->ident);
+	return 1;
+}
+
+static struct dmi_system_id ideapad_dmi_table[] __initconst = {
+	{
+		.ident = "Lenovo IdeaPad Y550",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "20017"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo IdeaPad Y550")
+		},
+		.callback = ideapad_dmi_check
+	},
+	{
+		.ident = "Lenovo IdeaPad Y550P",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "20035"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo IdeaPad Y550P")
+		},
+		.callback = ideapad_dmi_check
+	},
+	{ }
+};
+
+/* Init and cleanup */
+static int __init slidebar_init(void)
+{
+	int err;
+
+	if (!force && !dmi_check_system(ideapad_dmi_table))
+		return -ENODEV;
+
+	err = setup_platform_dev();
+	if (err)
+		return err;
+
+	err = register_platform_drv();
+	if (err)
+		goto err_remove_platform_dev;
+
+	err = setup_input_dev();
+	if (err)
+		goto err_unregister_platform_drv;
+	
+	err = setup_slidebar_mode_dev();
+	if (err)
+		goto err_remove_input_dev;
+	return 0;
+
+err_remove_input_dev:
+	remove_input_dev();
+err_unregister_platform_drv:
+	unregister_platform_drv();
+err_remove_platform_dev:
+	remove_platform_dev();
+	return err;
+}
+
+static void __exit slidebar_exit(void)
+{
+	remove_slidebar_mode_dev();
+	remove_input_dev();
+	remove_platform_dev();
+	unregister_platform_drv();
+}
+
+module_init(slidebar_init);
+module_exit(slidebar_exit);
+
+MODULE_AUTHOR("Andrey Moiseev [off-list ref]");
+MODULE_DESCRIPTION("Slidebar input support for some Lenovo IdeaPad laptops");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("0.1");
+
+MODULE_ALIAS("dmi:*:svnLENOVO:pn20017:pvrLenovoIdeaPadY550:*");
+MODULE_ALIAS("dmi:*:svnLENOVO:pn20035:pvrLenovoIdeaPadY550P:*");

Why not create MODULE_DEVICE_TABLE from dmi table instead of doing it by
hand?

Thanks.

-- 
Dmitry