Re: [PATCH] input:Adding support for Wacom Stylus with I2c interface

Hi Tatsunosuke,

On Tue, Oct 04, 2011 at 04:44:23PM +0900, Tatsunosuke Tobita wrote:

So we have a timer and a workqueue. Why not use delayed_work?

+	return HRTIMER_NORESTART;
+}
+
+static int wacom_i2c_input_open(struct input_dev *dev)
+{
+	struct wacom_i2c *wac_i2c = input_get_drvdata(dev);
+
+	mutex_lock(&wac_i2c->lock);
+	wac_i2c->dev->open = true;

Did you try actually compiling this?

+	mutex_unlock(&wac_i2c->lock);
+
+	return 0;
+}
+
+static void wacom_i2c_input_close(struct input_dev *dev)
+{
+	struct wacom_i2c *wac_i2c = input_get_drvdata(dev);
+
+	mutex_lock(&wac_i2c->lock);
+	wac_i2c->dev->open = false;
+	mutex_unlock(&wac_i2c->lock);
+}
+
+static void wacom_i2c_set_input_values(struct i2c_client *client,
+				       struct wacom_i2c *wac_i2c, struct input_dev *dev)
+{
+	dev->name = "WACOM_I2C_DIGITIZER";
+	dev->id.bustype = BUS_I2C;
+	dev->id.vendor = 0x56a;
+	dev->dev.parent = &client->dev;
+	dev->open = wacom_i2c_input_open;
+	dev->close = wacom_i2c_input_close;
+	dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+
+	__set_bit(ABS_X, dev->absbit);
+	__set_bit(ABS_Y, dev->absbit);
+	__set_bit(ABS_PRESSURE, dev->absbit);
+	__set_bit(BTN_TOOL_PEN, dev->keybit);
+	__set_bit(BTN_TOOL_RUBBER, dev->keybit);
+	__set_bit(BTN_STYLUS, dev->keybit);
+	__set_bit(BTN_STYLUS2, dev->keybit);
+	__set_bit(BTN_TOUCH, dev->keybit);

Just put this into probe().

+}
+
+static int wacom_i2c_probe(struct i2c_client *client,
+			   const struct i2c_device_id *id)
+{
+	struct wacom_i2c *wac_i2c;
+	int i, ret;
+	i = ret = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		goto err2;
+
+	wac_i2c = kzalloc(sizeof(struct wacom_i2c), GFP_KERNEL);
+	wac_i2c->wac_feature = wacom_feature_EMR;

Should we get this from i2c_device_id, platform data, device tree or
something else instead of hard-coding?

+
+	mutex_init(&wac_i2c->lock);
+
+	wac_i2c->dev = input_allocate_device();
+	if (wac_i2c == NULL || wac_i2c->dev == NULL)
+		goto fail;
+	wacom_i2c_set_input_values(client, wac_i2c, wac_i2c->dev);
+
+	wac_i2c->client = client;
+
+	INIT_WORK(&wac_i2c->workq, wacom_i2c_workqueue);
+
+	ret = wacom_send_query(wac_i2c);
+	if (ret < 0)
+		goto err1;
+
+	wac_i2c->dev->id.version = wac_i2c->wac_feature.fw_version;
+	input_set_abs_params(wac_i2c->dev, ABS_X, 0,
+			     wac_i2c->wac_feature.x_max, 0, 0);
+	input_set_abs_params(wac_i2c->dev, ABS_Y, 0,
+			     wac_i2c->wac_feature.y_max, 0, 0);
+	input_set_abs_params(wac_i2c->dev, ABS_PRESSURE, 0,
+			     wac_i2c->wac_feature.pressure_max, 0, 0);
+	input_set_drvdata(wac_i2c->dev, wac_i2c);
+
+	i2c_set_clientdata(client, wac_i2c);
+	if (input_register_device(wac_i2c->dev))
+		goto err1;

You should report error returned by input_register_device().

+
+	hrtimer_init(&wac_i2c->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	wac_i2c->timer.function = wacom_i2c_timer;
+	hrtimer_start(&wac_i2c->timer, ktime_set(1, 0), HRTIMER_MODE_REL);

Why start timer if nobody has the device opened yet?

+	printk(KERN_INFO "WACOM_I2C_DIGITIZER: initialized and started \n");
+
+	return 0;
+
+ err2:
+	printk(KERN_ERR "wacom_i2c:No I2C functionality found\n");
+	return -ENODEV;
+
+ err1:
+	printk(KERN_ERR "wacom_i2c:err1 occured(num:%d)\n", ret);
+	input_free_device(wac_i2c->dev);
+	wac_i2c->dev = NULL;
+	return -EIO;
+
+ fail:
+	printk(KERN_ERR "wacom_i2c:fail occured\n");
+	return -ENOMEM;
+}
+
+static int wacom_i2c_remove(struct i2c_client *client)
+{
+	struct wacom_i2c *wac_i2c = i2c_get_clientdata(client);
+
+	input_unregister_device(wac_i2c->dev);

Timer fires here -> *BOOM*. You should cancel the timer in close().

Also, polling-only mode is hardly useful for a touchscreen driver. Is
there a chance you could switch to interrupt-driven mode instead?

+	hrtimer_cancel(&wac_i2c->timer);
+	kfree(wac_i2c);
+
+	return 0;
+}
+
+static int wacom_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	int ret;
+	struct wacom_i2c *wac_i2c = i2c_get_clientdata(client);
+
+	hrtimer_cancel(&wac_i2c->timer);
+	cancel_work_sync(&wac_i2c->workq);
+	ret = wac_i2c->power(0);
+
+	return 0;
+}
+
+static int wacom_i2c_resume(struct i2c_client *client)
+{
+	struct wacom_i2c *wac_i2c = i2c_get_clientdata(client);
+
+	hrtimer_start(&wac_i2c->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
+
+	return 0;
+}
+
+static const struct i2c_device_id wacom_i2c_id[] = {
+	{WACNAME, 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, wacom_i2c_id);
+
+static struct i2c_driver wacom_i2c_driver = {
+	.driver = {
+		.name = "wacom_i2c",
+	},
+
+	.probe = wacom_i2c_probe,
+	.remove = wacom_i2c_remove,
+	.suspend = wacom_i2c_suspend,
+	.resume = wacom_i2c_resume,

Please use dev_pm_ops.

+	.id_table = wacom_i2c_id,
+};
+
+static int __init wacom_i2c_init()
+{
+
+	wacom_i2c_wq = create_singlethread_workqueue("wacom_i2c_wq");
+	if (!wacom_i2c_wq)
+		return -ENOMEM;

There is no reason for dedicated workqueue in mainline.

+
+	return i2c_add_driver(&wacom_i2c_driver);
+}
+
+static void __exit wacom_i2c_exit()
+{
+	if (wacom_i2c_wq)

The test is useless.

There shouldn't be static variables in header files.

Why is is split? Also, what license does this code have?

+int wacom_send_query(struct wacom_i2c *wac_i2c)
+{
+	int ret;
+	u8 cmd[4] = {CMD_QUERY0, CMD_QUERY1, CMD_QUERY2, CMD_QUERY3};
+	u8 data[COM_COORD_NUM];
+
+	ret = i2c_master_send(wac_i2c->client, cmd, sizeof(cmd));
+	if (ret < 0) {
+		printk(KERN_ERR "wacom_i2c: Sending Query failed \n");
+		return -1;
+	}
+
+	cmd[0] = CMD_THROW0; cmd[1] = CMD_THROW1;
+	ret = i2c_master_send(wac_i2c->client, cmd, 2);
+	if (ret < 0) {
+		printk(KERN_ERR "wacom_i2c: Sending Query failed \n");
+		return -1;
+	}
+
+	ret = i2c_master_recv(wac_i2c->client, data, QUERY_SIZE);
+	if (ret < 0) {
+		printk(KERN_ERR "wacom_i2c: Receiving Query failed\n");
+		return -1;
+	}
+
+	wac_i2c->wac_feature.x_max = get_unaligned_le16(&data[3]);
+	wac_i2c->wac_feature.y_max = get_unaligned_le16(&data[5]);
+	wac_i2c->wac_feature.pressure_max = get_unaligned_le16(&data[11]);
+	wac_i2c->wac_feature.fw_version = get_unaligned_le16(&data[13]);
+
+	printk(KERN_INFO "wacom_i2c: x_max:%d, y_max:%d, pressure:%d, fw:%d\n",
+	       wac_i2c->wac_feature.x_max, wac_i2c->wac_feature.y_max,
+	       wac_i2c->wac_feature.pressure_max, wac_i2c->wac_feature.fw_version);
+

Please use dev_xxx() for reporting.

Thanks.

-- 
Dmitry