[PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Joonyoung Shim]

The MELPAS MCS-5000 is the touchscreen controller. The overview of this
controller can see at the following website:

http://www.melfas.com/product/product01.asp?k_r=eng_

This patch supports only the i2c interface.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/input/touchscreen/Kconfig      |   12 +
 drivers/input/touchscreen/Makefile     |    1 +
 drivers/input/touchscreen/mcs5000_ts.c |  377 ++++++++++++++++++++++++++++++++
 include/linux/i2c/mcs5000_ts.h         |   11 +
 4 files changed, 401 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/touchscreen/mcs5000_ts.c
 create mode 100644 include/linux/i2c/mcs5000_ts.h

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 82c388e..ac5ebb4 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -481,4 +481,16 @@ config TOUCHSCREEN_TSC2007
 	  To compile this driver as a module, choose M here: the
 	  module will be called tsc2007.
 
+config TOUCHSCREEN_MCS5000
+	tristate "MELFAS MCS-5000 touchscreen"
+	depends on I2C
+	help
+	  Say Y here if you have the MELFAS MCS-5000 touchscreen controller
+	  chip in your system.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called mcs5000_ts.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index bef7415..5e4a9dd 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
 obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
 obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
 obj-$(CONFIG_TOUCHSCREEN_INEXIO)	+= inexio.o
+obj-$(CONFIG_TOUCHSCREEN_MCS5000)	+= mcs5000_ts.o
 obj-$(CONFIG_TOUCHSCREEN_MIGOR)		+= migor_ts.o
 obj-$(CONFIG_TOUCHSCREEN_MTOUCH)	+= mtouch.o
 obj-$(CONFIG_TOUCHSCREEN_MK712)		+= mk712.o
diff --git a/drivers/input/touchscreen/mcs5000_ts.c b/drivers/input/touchscreen/mcs5000_ts.c
new file mode 100644
index 0000000..c29c072
--- /dev/null
+++ b/drivers/input/touchscreen/mcs5000_ts.c
@@ -0,0 +1,377 @@
+/*
+ * mcs5000_ts.c - Touch screen driver for MELFAS MCS-5000 controller
+ *
+ * Copyright (C) 2008 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * Based on wm97xx-core.c
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/i2c/mcs5000_ts.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/irq.h>
+#include <linux/workqueue.h>
+
+#define DEFAULT_PRESSURE		0x100
+
+/* Registers */
+#define MCS5000_TS_STATUS		0x00
+#define STATUS_OFFSET			0
+#define STATUS_NO			(0 << STATUS_OFFSET)
+#define STATUS_INIT			(1 << STATUS_OFFSET)
+#define STATUS_SENSING			(2 << STATUS_OFFSET)
+#define STATUS_COORD			(3 << STATUS_OFFSET)
+#define STATUS_GESTURE			(4 << STATUS_OFFSET)
+#define ERROR_OFFSET			4
+#define ERROR_NO			(0 << ERROR_OFFSET)
+#define ERROR_POWER_ON_RESET		(1 << ERROR_OFFSET)
+#define ERROR_INT_RESET			(2 << ERROR_OFFSET)
+#define ERROR_EXT_RESET			(3 << ERROR_OFFSET)
+#define ERROR_INVALID_REG_ADDRESS	(8 << ERROR_OFFSET)
+#define ERROR_INVALID_REG_VALUE		(9 << ERROR_OFFSET)
+
+#define MCS5000_TS_OP_MODE		0x01
+#define RESET_OFFSET			0
+#define RESET_NO			(0 << RESET_OFFSET)
+#define RESET_EXT_SOFT			(1 << RESET_OFFSET)
+#define OP_MODE_OFFSET			1
+#define OP_MODE_SLEEP			(0 << OP_MODE_OFFSET)
+#define OP_MODE_ACTIVE			(1 << OP_MODE_OFFSET)
+#define GESTURE_OFFSET			4
+#define GESTURE_DISABLE			(0 << GESTURE_OFFSET)
+#define GESTURE_ENABLE			(1 << GESTURE_OFFSET)
+#define PROXIMITY_OFFSET		5
+#define PROXIMITY_DISABLE		(0 << PROXIMITY_OFFSET)
+#define PROXIMITY_ENABLE		(1 << PROXIMITY_OFFSET)
+#define SCAN_MODE_OFFSET		6
+#define SCAN_MODE_INTERRUPT		(0 << SCAN_MODE_OFFSET)
+#define SCAN_MODE_POLLING		(1 << SCAN_MODE_OFFSET)
+#define REPORT_RATE_OFFSET		7
+#define REPORT_RATE_40			(0 << REPORT_RATE_OFFSET)
+#define REPORT_RATE_80			(1 << REPORT_RATE_OFFSET)
+
+#define MCS5000_TS_SENS_CTL		0x02
+#define MCS5000_TS_FILTER_CTL		0x03
+#define PRI_FILTER_OFFSET		0
+#define SEC_FILTER_OFFSET		4
+
+#define MCS5000_TS_X_SIZE_UPPER		0x08
+#define MCS5000_TS_X_SIZE_LOWER		0x09
+#define MCS5000_TS_Y_SIZE_UPPER		0x0A
+#define MCS5000_TS_Y_SIZE_LOWER		0x0B
+
+#define MCS5000_TS_INPUT_INFO		0x10
+#define INPUT_TYPE_OFFSET		0
+#define INPUT_TYPE_NONTOUCH		(0 << INPUT_TYPE_OFFSET)
+#define INPUT_TYPE_SINGLE		(1 << INPUT_TYPE_OFFSET)
+#define INPUT_TYPE_DUAL			(2 << INPUT_TYPE_OFFSET)
+#define INPUT_TYPE_PALM			(3 << INPUT_TYPE_OFFSET)
+#define INPUT_TYPE_PROXIMITY		(7 << INPUT_TYPE_OFFSET)
+#define GESTURE_CODE_OFFSET		3
+#define GESTURE_CODE_NO			(0 << GESTURE_CODE_OFFSET)
+
+#define MCS5000_TS_X_POS_UPPER		0x11
+#define MCS5000_TS_X_POS_LOWER		0x12
+#define MCS5000_TS_Y_POS_UPPER		0x13
+#define MCS5000_TS_Y_POS_LOWER		0x14
+#define MCS5000_TS_Z_POS		0x15
+#define MCS5000_TS_WIDTH		0x16
+#define MCS5000_TS_GESTURE_VAL		0x17
+#define MCS5000_TS_MODULE_REV		0x20
+#define MCS5000_TS_FIRMWARE_VER		0x21
+
+enum mcs5000_ts_read_offset {
+	READ_INPUT_INFO,
+	READ_X_POS_UPPER,
+	READ_X_POS_LOWER,
+	READ_Y_POS_UPPER,
+	READ_Y_POS_LOWER,
+	READ_BLOCK_SIZE,
+};
+
+/* Touchscreen absolute values */
+static int abs_x[3] = {0, 1024, 0};
+module_param_array(abs_x, int, NULL, 0);
+MODULE_PARM_DESC(abs_x, "Touchscreen absolute X min, max, fuzz");
+
+static int abs_y[3] = {0, 1024, 0};
+module_param_array(abs_y, int, NULL, 0);
+MODULE_PARM_DESC(abs_y, "Touchscreen absolute Y min, max, fuzz");
+
+static int abs_p[3] = {0, 256, 0};
+module_param_array(abs_p, int, NULL, 0);
+MODULE_PARM_DESC(abs_p, "Touchscreen absolute Pressure min, max, fuzz");
+
+/* Each client has this additional data */
+struct mcs5000_ts_data {
+	struct i2c_client *client;
+	struct input_dev *input_dev;
+	struct work_struct ts_event_work;
+	struct mcs5000_ts_platform_data *platform_data;
+
+	unsigned int irq;
+};
+
+static struct i2c_driver mcs5000_ts_driver;
+
+static void mcs5000_ts_input_read(struct mcs5000_ts_data *data)
+{
+	struct i2c_client *client = data->client;
+	u8 buffer[READ_BLOCK_SIZE];
+	int err;
+	int x;
+	int y;
+
+	err = i2c_smbus_read_i2c_block_data(client, MCS5000_TS_INPUT_INFO,
+			READ_BLOCK_SIZE, buffer);
+	if (err < 0) {
+		dev_err(&client->dev, "%s, err[%d]\n", __func__, err);
+		return;
+	}
+
+	switch (buffer[READ_INPUT_INFO]) {
+	case INPUT_TYPE_NONTOUCH:
+		input_report_abs(data->input_dev, ABS_PRESSURE, 0);
+		input_report_key(data->input_dev, BTN_TOUCH, 0);
+		input_sync(data->input_dev);
+		break;
+	case INPUT_TYPE_SINGLE:
+		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
+		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
+
+		input_report_key(data->input_dev, BTN_TOUCH, 1);
+		input_report_abs(data->input_dev, ABS_X, x);
+		input_report_abs(data->input_dev, ABS_Y, y);
+		input_report_abs(data->input_dev, ABS_PRESSURE,
+				DEFAULT_PRESSURE);
+		input_sync(data->input_dev);
+		break;
+	case INPUT_TYPE_DUAL:
+		/* TODO */
+		break;
+	case INPUT_TYPE_PALM:
+		/* TODO */
+		break;
+	case INPUT_TYPE_PROXIMITY:
+		/* TODO */
+		break;
+	default:
+		dev_err(&client->dev, "Unknown ts input type %d\n",
+				buffer[READ_INPUT_INFO]);
+		break;
+	}
+}
+
+static void mcs5000_ts_irq_worker(struct work_struct *work)
+{
+	struct mcs5000_ts_data *data = container_of(work,
+			struct mcs5000_ts_data, ts_event_work);
+
+	mcs5000_ts_input_read(data);
+
+	enable_irq(data->irq);
+}
+
+static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id)
+{
+	struct mcs5000_ts_data *data = dev_id;
+
+	if (!work_pending(&data->ts_event_work)) {
+		disable_irq_nosync(data->irq);
+		schedule_work(&data->ts_event_work);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int mcs5000_ts_input_init(struct mcs5000_ts_data *data)
+{
+	struct input_dev *input_dev;
+	int ret = 0;
+
+	INIT_WORK(&data->ts_event_work, mcs5000_ts_irq_worker);
+
+	data->input_dev = input_allocate_device();
+	if (data->input_dev == NULL) {
+		ret = -ENOMEM;
+		goto err_input;
+	}
+
+	input_dev = data->input_dev;
+	input_dev->name = "MELPAS MCS-5000 Touchscreen";
+	set_bit(EV_ABS, input_dev->evbit);
+	set_bit(ABS_X, input_dev->absbit);
+	set_bit(ABS_Y, input_dev->absbit);
+	set_bit(ABS_PRESSURE, input_dev->absbit);
+	input_set_abs_params(input_dev, ABS_X, abs_x[0], abs_x[1], abs_x[2], 0);
+	input_set_abs_params(input_dev, ABS_Y, abs_y[0], abs_y[1], abs_y[2], 0);
+	input_set_abs_params(input_dev, ABS_PRESSURE, abs_p[0], abs_p[1],
+			abs_p[2], 0);
+	set_bit(EV_KEY, input_dev->evbit);
+	set_bit(BTN_TOUCH, input_dev->keybit);
+
+	ret = input_register_device(data->input_dev);
+	if (ret < 0)
+		goto err_register;
+
+	if (request_irq(data->irq, mcs5000_ts_interrupt,
+			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
+		dev_err(&data->client->dev, "Failed to register interrupt\n");
+		ret = -EINVAL;
+		goto err_irq;
+	}
+
+	input_set_drvdata(input_dev, data);
+
+	return 0;
+err_irq:
+	input_unregister_device(data->input_dev);
+err_register:
+	input_free_device(data->input_dev);
+err_input:
+	return ret;
+}
+
+static void mcs5000_ts_phys_init(struct mcs5000_ts_data *data)
+{
+	struct i2c_client *client = data->client;
+	struct mcs5000_ts_platform_data *platform_data = data->platform_data;
+
+	/* Touch reset & sleep mode */
+	i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE,
+			RESET_EXT_SOFT | OP_MODE_SLEEP);
+
+	/* Touch size */
+	i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_UPPER,
+			platform_data->x_size >> 8);
+	i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_LOWER,
+			platform_data->x_size & 0xff);
+	i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_UPPER,
+			platform_data->y_size >> 8);
+	i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_LOWER,
+			platform_data->y_size & 0xff);
+
+	/* Touch active mode & 80 report rate */
+	i2c_smbus_write_byte_data(data->client, MCS5000_TS_OP_MODE,
+			OP_MODE_ACTIVE | REPORT_RATE_80);
+}
+
+static int mcs5000_ts_probe(struct i2c_client *client,
+		const struct i2c_device_id *idp)
+{
+	struct mcs5000_ts_data *data;
+	int ret;
+
+	data = kzalloc(sizeof(struct mcs5000_ts_data), GFP_KERNEL);
+	if (!data)
+		dev_err(&client->dev, "Failed to allocate driver data\n");
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	data->client = client;
+	data->irq = client->irq;
+	data->platform_data = client->dev.platform_data;
+
+	i2c_set_clientdata(client, data);
+
+	if (data->platform_data->set_pin)
+		data->platform_data->set_pin();
+
+	ret = mcs5000_ts_input_init(data);
+	if (ret)
+		goto exit_free;
+
+	mcs5000_ts_phys_init(data);
+
+	return 0;
+
+exit_free:
+	kfree(data);
+	i2c_set_clientdata(client, NULL);
+exit:
+	return ret;
+}
+
+static int mcs5000_ts_remove(struct i2c_client *client)
+{
+	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
+
+	cancel_work_sync(&data->ts_event_work);
+	free_irq(data->irq, data);
+	input_unregister_device(data->input_dev);
+	input_free_device(data->input_dev);
+	kfree(data);
+
+	i2c_set_clientdata(client, NULL);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int mcs5000_ts_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	/* Touch sleep mode */
+	i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE, OP_MODE_SLEEP);
+
+	return 0;
+}
+
+static int mcs5000_ts_resume(struct i2c_client *client)
+{
+	struct mcs5000_ts_data *data;
+
+	data = i2c_get_clientdata(client);
+	mcs5000_ts_phys_init(data);
+
+	return 0;
+}
+#else
+#define mcs5000_ts_suspend	NULL
+#define mcs5000_ts_resume	NULL
+#endif
+
+static const struct i2c_device_id mcs5000_ts_id[] = {
+	{ "mcs5000_ts", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mcs5000_ts_id);
+
+static struct i2c_driver mcs5000_ts_driver = {
+	.driver = {
+		.name = "mcs5000_ts",
+	},
+	.probe		= mcs5000_ts_probe,
+	.remove		= mcs5000_ts_remove,
+	.suspend	= mcs5000_ts_suspend,
+	.resume		= mcs5000_ts_resume,
+	.id_table	= mcs5000_ts_id,
+};
+
+static int __init mcs5000_ts_init(void)
+{
+	return i2c_add_driver(&mcs5000_ts_driver);
+}
+
+static void __exit mcs5000_ts_exit(void)
+{
+	i2c_del_driver(&mcs5000_ts_driver);
+}
+
+module_init(mcs5000_ts_init);
+module_exit(mcs5000_ts_exit);
+
+/* Module information */
+MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
+MODULE_DESCRIPTION("Touch screen driver for MELFAS MCS-5000 controller");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c/mcs5000_ts.h b/include/linux/i2c/mcs5000_ts.h
new file mode 100644
index 0000000..69d92b6
--- /dev/null
+++ b/include/linux/i2c/mcs5000_ts.h
@@ -0,0 +1,11 @@
+#ifndef __LINUX_MCS5000_TS_H
+#define __LINUX_MCS5000_TS_H
+
+/* platform data for the MELFAS MCS5000 touchscreen driver */
+struct mcs5000_ts_platform_data {
+	void (*set_pin)(void);
+	int x_size;
+	int y_size;
+};
+
+#endif	/* __LINUX_MCS5000_TS_H */
-- 
1.6.0.4

Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Dmitry Torokhov]

Hi,

On Mon, May 11, 2009 at 09:28:30PM +0900, Joonyoung Shim wrote:
> The MELPAS MCS-5000 is the touchscreen controller. The overview of this
> controller can see at the following website:
> 
> http://www.melfas.com/product/product01.asp?k_r=eng_
> 
> This patch supports only the i2c interface.
> 

Thank you for the patch. Some comments below.

> +
> +/* Touchscreen absolute values */
> +static int abs_x[3] = {0, 1024, 0};
> +module_param_array(abs_x, int, NULL, 0);
> +MODULE_PARM_DESC(abs_x, "Touchscreen absolute X min, max, fuzz");
> +
> +static int abs_y[3] = {0, 1024, 0};
> +module_param_array(abs_y, int, NULL, 0);
> +MODULE_PARM_DESC(abs_y, "Touchscreen absolute Y min, max, fuzz");
> +

Do we need to have these as module parameters give the fact that we can
set them dynamically with EVIOCSABS? I'd =say they should go.

> +static int abs_p[3] = {0, 256, 0};
> +module_param_array(abs_p, int, NULL, 0);
> +MODULE_PARM_DESC(abs_p, "Touchscreen absolute Pressure min, max, fuzz");
> +
> +/* Each client has this additional data */
> +struct mcs5000_ts_data {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +	struct work_struct ts_event_work;
> +	struct mcs5000_ts_platform_data *platform_data;
> +
> +	unsigned int irq;
> +};
> +
> +static struct i2c_driver mcs5000_ts_driver;
> +
> +static void mcs5000_ts_input_read(struct mcs5000_ts_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 buffer[READ_BLOCK_SIZE];
> +	int err;
> +	int x;
> +	int y;
> +
> +	err = i2c_smbus_read_i2c_block_data(client, MCS5000_TS_INPUT_INFO,
> +			READ_BLOCK_SIZE, buffer);
> +	if (err < 0) {
> +		dev_err(&client->dev, "%s, err[%d]\n", __func__, err);
> +		return;
> +	}
> +
> +	switch (buffer[READ_INPUT_INFO]) {
> +	case INPUT_TYPE_NONTOUCH:
> +		input_report_abs(data->input_dev, ABS_PRESSURE, 0);
> +		input_report_key(data->input_dev, BTN_TOUCH, 0);
> +		input_sync(data->input_dev);
> +		break;
> +	case INPUT_TYPE_SINGLE:
> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
> +
> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
> +		input_report_abs(data->input_dev, ABS_X, x);
> +		input_report_abs(data->input_dev, ABS_Y, y);
> +		input_report_abs(data->input_dev, ABS_PRESSURE,
> +				DEFAULT_PRESSURE);

If the hardware does not support pressure reading don't fake it.
BTN_TOUCH should be enough to signal touch.

> +		input_sync(data->input_dev);
> +		break;
> +	case INPUT_TYPE_DUAL:
> +		/* TODO */
> +		break;
> +	case INPUT_TYPE_PALM:
> +		/* TODO */
> +		break;
> +	case INPUT_TYPE_PROXIMITY:
> +		/* TODO */
> +		break;
> +	default:
> +		dev_err(&client->dev, "Unknown ts input type %d\n",
> +				buffer[READ_INPUT_INFO]);
> +		break;
> +	}
> +}
> +
> +static void mcs5000_ts_irq_worker(struct work_struct *work)
> +{
> +	struct mcs5000_ts_data *data = container_of(work,
> +			struct mcs5000_ts_data, ts_event_work);
> +
> +	mcs5000_ts_input_read(data);
> +
> +	enable_irq(data->irq);
> +}
> +
> +static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id)
> +{
> +	struct mcs5000_ts_data *data = dev_id;
> +
> +	if (!work_pending(&data->ts_event_work)) {
> +		disable_irq_nosync(data->irq);
> +		schedule_work(&data->ts_event_work);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mcs5000_ts_input_init(struct mcs5000_ts_data *data)
> +{
> +	struct input_dev *input_dev;
> +	int ret = 0;
> +
> +	INIT_WORK(&data->ts_event_work, mcs5000_ts_irq_worker);
> +
> +	data->input_dev = input_allocate_device();
> +	if (data->input_dev == NULL) {
> +		ret = -ENOMEM;
> +		goto err_input;
> +	}
> +
> +	input_dev = data->input_dev;
> +	input_dev->name = "MELPAS MCS-5000 Touchscreen";

input_dev->id.bustype = BUS_I2C;
input_dev->dev.parent = <i2c device>;


> +	set_bit(EV_ABS, input_dev->evbit);
> +	set_bit(ABS_X, input_dev->absbit);
> +	set_bit(ABS_Y, input_dev->absbit);
> +	set_bit(ABS_PRESSURE, input_dev->absbit);
> +	input_set_abs_params(input_dev, ABS_X, abs_x[0], abs_x[1], abs_x[2], 0);
> +	input_set_abs_params(input_dev, ABS_Y, abs_y[0], abs_y[1], abs_y[2], 0);
> +	input_set_abs_params(input_dev, ABS_PRESSURE, abs_p[0], abs_p[1],
> +			abs_p[2], 0);

This line should go.

> +	set_bit(EV_KEY, input_dev->evbit);
> +	set_bit(BTN_TOUCH, input_dev->keybit);
> +
> +	ret = input_register_device(data->input_dev);
> +	if (ret < 0)
> +		goto err_register;
> +
> +	if (request_irq(data->irq, mcs5000_ts_interrupt,
> +			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
> +		dev_err(&data->client->dev, "Failed to register interrupt\n");
> +		ret = -EINVAL;

Why EINVAL and not EBUSY? Or better yet, why don't you propagate what
reqiest_irq returned?

> +		goto err_irq;
> +	}
> +
> +	input_set_drvdata(input_dev, data);
> +
> +	return 0;
> +err_irq:
> +	input_unregister_device(data->input_dev);
> +err_register:
> +	input_free_device(data->input_dev);

Do not call input_free_device() after input_unregister_device(), it will
cause double-free. EIther rearrange initialization order or do
data->input_dev = NULL; right after calling input_unregister_device().

> +err_input:
> +	return ret;
> +}
> +
> +static void mcs5000_ts_phys_init(struct mcs5000_ts_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	struct mcs5000_ts_platform_data *platform_data = data->platform_data;
> +
> +	/* Touch reset & sleep mode */
> +	i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE,
> +			RESET_EXT_SOFT | OP_MODE_SLEEP);
> +
> +	/* Touch size */
> +	i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_UPPER,
> +			platform_data->x_size >> 8);
> +	i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_LOWER,
> +			platform_data->x_size & 0xff);
> +	i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_UPPER,
> +			platform_data->y_size >> 8);
> +	i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_LOWER,
> +			platform_data->y_size & 0xff);
> +
> +	/* Touch active mode & 80 report rate */
> +	i2c_smbus_write_byte_data(data->client, MCS5000_TS_OP_MODE,
> +			OP_MODE_ACTIVE | REPORT_RATE_80);
> +}
> +
> +static int mcs5000_ts_probe(struct i2c_client *client,
> +		const struct i2c_device_id *idp)

This should be marked __devinit.

> +{
> +	struct mcs5000_ts_data *data;
> +	int ret;
> +
> +	data = kzalloc(sizeof(struct mcs5000_ts_data), GFP_KERNEL);
> +	if (!data)
> +		dev_err(&client->dev, "Failed to allocate driver data\n");
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	data->client = client;
> +	data->irq = client->irq;
> +	data->platform_data = client->dev.platform_data;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	if (data->platform_data->set_pin)
> +		data->platform_data->set_pin();
> +
> +	ret = mcs5000_ts_input_init(data);
> +	if (ret)
> +		goto exit_free;
> +
> +	mcs5000_ts_phys_init(data);
> +
> +	return 0;
> +
> +exit_free:
> +	kfree(data);
> +	i2c_set_clientdata(client, NULL);
> +exit:
> +	return ret;
> +}
> +
> +static int mcs5000_ts_remove(struct i2c_client *client)

This should be marked __devexit.

> +{
> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
> +
> +	cancel_work_sync(&data->ts_event_work);

There is a race here, IRQ handler may resubmit work after
cancel_work_sync() returns. You need to  make sure that
IRQ handler does not resubmit work while device is being shut down.

> +	free_irq(data->irq, data);
> +	input_unregister_device(data->input_dev);
> +	input_free_device(data->input_dev);

Just input_unregister_device() is enough.

> +	kfree(data);
> +
> +	i2c_set_clientdata(client, NULL);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int mcs5000_ts_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	/* Touch sleep mode */
> +	i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE, OP_MODE_SLEEP);
> +
> +	return 0;
> +}
> +
> +static int mcs5000_ts_resume(struct i2c_client *client)
> +{
> +	struct mcs5000_ts_data *data;
> +
> +	data = i2c_get_clientdata(client);
> +	mcs5000_ts_phys_init(data);
> +
> +	return 0;
> +}
> +#else
> +#define mcs5000_ts_suspend	NULL
> +#define mcs5000_ts_resume	NULL
> +#endif
> +
> +static const struct i2c_device_id mcs5000_ts_id[] = {
> +	{ "mcs5000_ts", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, mcs5000_ts_id);
> +
> +static struct i2c_driver mcs5000_ts_driver = {
> +	.driver = {
> +		.name = "mcs5000_ts",
> +	},
> +	.probe		= mcs5000_ts_probe,
> +	.remove		= mcs5000_ts_remove,

__devexit_p()

#ifdef CONFIG_PM
> +	.suspend	= mcs5000_ts_suspend,
> +	.resume		= mcs5000_ts_resume,
#endif

> +	.id_table	= mcs5000_ts_id,
> +};
> +

Thanks.

-- 
Dmitry

Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Joonyoung Shim]

Hi,

Thank you for your review.

> Hi,
> 
> On Mon, May 11, 2009 at 09:28:30PM +0900, Joonyoung Shim wrote:
>> The MELPAS MCS-5000 is the touchscreen controller. The overview of this
>> controller can see at the following website:
>>
>> http://www.melfas.com/product/product01.asp?k_r=eng_
>>
>> This patch supports only the i2c interface.
>>
> 
> Thank you for the patch. Some comments below.
> 
>> +
>> +/* Touchscreen absolute values */
>> +static int abs_x[3] = {0, 1024, 0};
>> +module_param_array(abs_x, int, NULL, 0);
>> +MODULE_PARM_DESC(abs_x, "Touchscreen absolute X min, max, fuzz");
>> +
>> +static int abs_y[3] = {0, 1024, 0};
>> +module_param_array(abs_y, int, NULL, 0);
>> +MODULE_PARM_DESC(abs_y, "Touchscreen absolute Y min, max, fuzz");
>> +
> 
> Do we need to have these as module parameters give the fact that we can
> set them dynamically with EVIOCSABS? I'd =say they should go.

I think that this part is unnecessary, we can get resolution of
touchscreen from platform_data or i will use the defined values.

> 
>> +static int abs_p[3] = {0, 256, 0};
>> +module_param_array(abs_p, int, NULL, 0);
>> +MODULE_PARM_DESC(abs_p, "Touchscreen absolute Pressure min, max, fuzz");
>> +
>> +/* Each client has this additional data */
>> +struct mcs5000_ts_data {
>> +	struct i2c_client *client;
>> +	struct input_dev *input_dev;
>> +	struct work_struct ts_event_work;
>> +	struct mcs5000_ts_platform_data *platform_data;
>> +
>> +	unsigned int irq;
>> +};
>> +
>> +static struct i2c_driver mcs5000_ts_driver;
>> +
>> +static void mcs5000_ts_input_read(struct mcs5000_ts_data *data)
>> +{
>> +	struct i2c_client *client = data->client;
>> +	u8 buffer[READ_BLOCK_SIZE];
>> +	int err;
>> +	int x;
>> +	int y;
>> +
>> +	err = i2c_smbus_read_i2c_block_data(client, MCS5000_TS_INPUT_INFO,
>> +			READ_BLOCK_SIZE, buffer);
>> +	if (err < 0) {
>> +		dev_err(&client->dev, "%s, err[%d]\n", __func__, err);
>> +		return;
>> +	}
>> +
>> +	switch (buffer[READ_INPUT_INFO]) {
>> +	case INPUT_TYPE_NONTOUCH:
>> +		input_report_abs(data->input_dev, ABS_PRESSURE, 0);
>> +		input_report_key(data->input_dev, BTN_TOUCH, 0);
>> +		input_sync(data->input_dev);
>> +		break;
>> +	case INPUT_TYPE_SINGLE:
>> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
>> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
>> +
>> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
>> +		input_report_abs(data->input_dev, ABS_X, x);
>> +		input_report_abs(data->input_dev, ABS_Y, y);
>> +		input_report_abs(data->input_dev, ABS_PRESSURE,
>> +				DEFAULT_PRESSURE);
> 
> If the hardware does not support pressure reading don't fake it.
> BTN_TOUCH should be enough to signal touch.

MCS-5000 supports pressure reading, but the value of pressure is unstable in
my target, so i used the static value defined.
I will add pressure reading after more test.

signal touch? Do you mean single touch?

> 
>> +		input_sync(data->input_dev);
>> +		break;
>> +	case INPUT_TYPE_DUAL:
>> +		/* TODO */
>> +		break;
>> +	case INPUT_TYPE_PALM:
>> +		/* TODO */
>> +		break;
>> +	case INPUT_TYPE_PROXIMITY:
>> +		/* TODO */
>> +		break;
>> +	default:
>> +		dev_err(&client->dev, "Unknown ts input type %d\n",
>> +				buffer[READ_INPUT_INFO]);
>> +		break;
>> +	}
>> +}
>> +
>> +static void mcs5000_ts_irq_worker(struct work_struct *work)
>> +{
>> +	struct mcs5000_ts_data *data = container_of(work,
>> +			struct mcs5000_ts_data, ts_event_work);
>> +
>> +	mcs5000_ts_input_read(data);
>> +
>> +	enable_irq(data->irq);
>> +}
>> +
>> +static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id)
>> +{
>> +	struct mcs5000_ts_data *data = dev_id;
>> +
>> +	if (!work_pending(&data->ts_event_work)) {
>> +		disable_irq_nosync(data->irq);
>> +		schedule_work(&data->ts_event_work);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int mcs5000_ts_input_init(struct mcs5000_ts_data *data)
>> +{
>> +	struct input_dev *input_dev;
>> +	int ret = 0;
>> +
>> +	INIT_WORK(&data->ts_event_work, mcs5000_ts_irq_worker);
>> +
>> +	data->input_dev = input_allocate_device();
>> +	if (data->input_dev == NULL) {
>> +		ret = -ENOMEM;
>> +		goto err_input;
>> +	}
>> +
>> +	input_dev = data->input_dev;
>> +	input_dev->name = "MELPAS MCS-5000 Touchscreen";
> 
> input_dev->id.bustype = BUS_I2C;
> input_dev->dev.parent = <i2c device>;

ok, i will add it.

> 
> 
>> +	set_bit(EV_ABS, input_dev->evbit);
>> +	set_bit(ABS_X, input_dev->absbit);
>> +	set_bit(ABS_Y, input_dev->absbit);
>> +	set_bit(ABS_PRESSURE, input_dev->absbit);
>> +	input_set_abs_params(input_dev, ABS_X, abs_x[0], abs_x[1], abs_x[2], 0);
>> +	input_set_abs_params(input_dev, ABS_Y, abs_y[0], abs_y[1], abs_y[2], 0);
>> +	input_set_abs_params(input_dev, ABS_PRESSURE, abs_p[0], abs_p[1],
>> +			abs_p[2], 0);
> 
> This line should go.
> 
>> +	set_bit(EV_KEY, input_dev->evbit);
>> +	set_bit(BTN_TOUCH, input_dev->keybit);
>> +
>> +	ret = input_register_device(data->input_dev);
>> +	if (ret < 0)
>> +		goto err_register;
>> +
>> +	if (request_irq(data->irq, mcs5000_ts_interrupt,
>> +			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
>> +		dev_err(&data->client->dev, "Failed to register interrupt\n");
>> +		ret = -EINVAL;
> 
> Why EINVAL and not EBUSY? Or better yet, why don't you propagate what
> reqiest_irq returned?

Hmm, EINVAL is used in wm97xx-core.c file, but you are right.
I will fix it using the latter.

> 
>> +		goto err_irq;
>> +	}
>> +
>> +	input_set_drvdata(input_dev, data);
>> +
>> +	return 0;
>> +err_irq:
>> +	input_unregister_device(data->input_dev);
>> +err_register:
>> +	input_free_device(data->input_dev);
> 
> Do not call input_free_device() after input_unregister_device(), it will
> cause double-free. EIther rearrange initialization order or do
> data->input_dev = NULL; right after calling input_unregister_device().

ok, i will fix it.

> 
>> +err_input:
>> +	return ret;
>> +}
>> +
>> +static void mcs5000_ts_phys_init(struct mcs5000_ts_data *data)
>> +{
>> +	struct i2c_client *client = data->client;
>> +	struct mcs5000_ts_platform_data *platform_data = data->platform_data;
>> +
>> +	/* Touch reset & sleep mode */
>> +	i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE,
>> +			RESET_EXT_SOFT | OP_MODE_SLEEP);
>> +
>> +	/* Touch size */
>> +	i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_UPPER,
>> +			platform_data->x_size >> 8);
>> +	i2c_smbus_write_byte_data(client, MCS5000_TS_X_SIZE_LOWER,
>> +			platform_data->x_size & 0xff);
>> +	i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_UPPER,
>> +			platform_data->y_size >> 8);
>> +	i2c_smbus_write_byte_data(client, MCS5000_TS_Y_SIZE_LOWER,
>> +			platform_data->y_size & 0xff);
>> +
>> +	/* Touch active mode & 80 report rate */
>> +	i2c_smbus_write_byte_data(data->client, MCS5000_TS_OP_MODE,
>> +			OP_MODE_ACTIVE | REPORT_RATE_80);
>> +}
>> +
>> +static int mcs5000_ts_probe(struct i2c_client *client,
>> +		const struct i2c_device_id *idp)
> 
> This should be marked __devinit.

ok, i will add it.

> 
>> +{
>> +	struct mcs5000_ts_data *data;
>> +	int ret;
>> +
>> +	data = kzalloc(sizeof(struct mcs5000_ts_data), GFP_KERNEL);
>> +	if (!data)
>> +		dev_err(&client->dev, "Failed to allocate driver data\n");
>> +		ret = -ENOMEM;
>> +		goto exit;
>> +	}
>> +
>> +	data->client = client;
>> +	data->irq = client->irq;
>> +	data->platform_data = client->dev.platform_data;
>> +
>> +	i2c_set_clientdata(client, data);
>> +
>> +	if (data->platform_data->set_pin)
>> +		data->platform_data->set_pin();
>> +
>> +	ret = mcs5000_ts_input_init(data);
>> +	if (ret)
>> +		goto exit_free;
>> +
>> +	mcs5000_ts_phys_init(data);
>> +
>> +	return 0;
>> +
>> +exit_free:
>> +	kfree(data);
>> +	i2c_set_clientdata(client, NULL);
>> +exit:
>> +	return ret;
>> +}
>> +
>> +static int mcs5000_ts_remove(struct i2c_client *client)
> 
> This should be marked __devexit.

ok, i will add it.

> 
>> +{
>> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
>> +
>> +	cancel_work_sync(&data->ts_event_work);
> 
> There is a race here, IRQ handler may resubmit work after
> cancel_work_sync() returns. You need to  make sure that
> IRQ handler does not resubmit work while device is being shut down.

ok, how about doing free_irq() before cancel_work_sync() call?


> 
>> +	free_irq(data->irq, data);
>> +	input_unregister_device(data->input_dev);
>> +	input_free_device(data->input_dev);
> 
> Just input_unregister_device() is enough.

ok.

> 
>> +	kfree(data);
>> +
>> +	i2c_set_clientdata(client, NULL);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int mcs5000_ts_suspend(struct i2c_client *client, pm_message_t mesg)
>> +{
>> +	/* Touch sleep mode */
>> +	i2c_smbus_write_byte_data(client, MCS5000_TS_OP_MODE, OP_MODE_SLEEP);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mcs5000_ts_resume(struct i2c_client *client)
>> +{
>> +	struct mcs5000_ts_data *data;
>> +
>> +	data = i2c_get_clientdata(client);
>> +	mcs5000_ts_phys_init(data);
>> +
>> +	return 0;
>> +}
>> +#else
>> +#define mcs5000_ts_suspend	NULL
>> +#define mcs5000_ts_resume	NULL
>> +#endif
>> +
>> +static const struct i2c_device_id mcs5000_ts_id[] = {
>> +	{ "mcs5000_ts", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, mcs5000_ts_id);
>> +
>> +static struct i2c_driver mcs5000_ts_driver = {
>> +	.driver = {
>> +		.name = "mcs5000_ts",
>> +	},
>> +	.probe		= mcs5000_ts_probe,
>> +	.remove		= mcs5000_ts_remove,
> 
> __devexit_p()

ok.

> 
> #ifdef CONFIG_PM
>> +	.suspend	= mcs5000_ts_suspend,
>> +	.resume		= mcs5000_ts_resume,
> #endif

ok, i will add it.

> 
>> +	.id_table	= mcs5000_ts_id,
>> +};
>> +
> 
> Thanks.
> 

Thanks, too.

Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Dmitry Torokhov]

On Thu, May 14, 2009 at 09:52:41PM +0900, Joonyoung Shim wrote:
> Hi,
> 
> Thank you for your review.
> 
> > Hi,
> > 
> > On Mon, May 11, 2009 at 09:28:30PM +0900, Joonyoung Shim wrote:
> >> The MELPAS MCS-5000 is the touchscreen controller. The overview of this
> >> controller can see at the following website:
> >>
> >> http://www.melfas.com/product/product01.asp?k_r=eng_
> >>
> >> This patch supports only the i2c interface.
> >>
> > 
> > Thank you for the patch. Some comments below.
> > 
> >> +
> >> +/* Touchscreen absolute values */
> >> +static int abs_x[3] = {0, 1024, 0};
> >> +module_param_array(abs_x, int, NULL, 0);
> >> +MODULE_PARM_DESC(abs_x, "Touchscreen absolute X min, max, fuzz");
> >> +
> >> +static int abs_y[3] = {0, 1024, 0};
> >> +module_param_array(abs_y, int, NULL, 0);
> >> +MODULE_PARM_DESC(abs_y, "Touchscreen absolute Y min, max, fuzz");
> >> +
> > 
> > Do we need to have these as module parameters give the fact that we can
> > set them dynamically with EVIOCSABS? I'd =say they should go.
> 
> I think that this part is unnecessary, we can get resolution of
> touchscreen from platform_data or i will use the defined values.
> 

Ok, that would work as well.

> >> +	case INPUT_TYPE_SINGLE:
> >> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
> >> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
> >> +
> >> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
> >> +		input_report_abs(data->input_dev, ABS_X, x);
> >> +		input_report_abs(data->input_dev, ABS_Y, y);
> >> +		input_report_abs(data->input_dev, ABS_PRESSURE,
> >> +				DEFAULT_PRESSURE);
> > 
> > If the hardware does not support pressure reading don't fake it.
> > BTN_TOUCH should be enough to signal touch.
> 
> MCS-5000 supports pressure reading, but the value of pressure is unstable in
> my target, so i used the static value defined.
> I will add pressure reading after more test.

OK. Alternatively you may indicate in the platform data if pressure
reading is supported and set ABS_PRESSURE bit and report pressure only
when you know it works well.
> 
> signal touch? Do you mean single touch?

I meant "signal" as "convey", "indicate".

> >> +
> >> +	if (request_irq(data->irq, mcs5000_ts_interrupt,
> >> +			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
> >> +		dev_err(&data->client->dev, "Failed to register interrupt\n");
> >> +		ret = -EINVAL;
> > 
> > Why EINVAL and not EBUSY? Or better yet, why don't you propagate what
> > reqiest_irq returned?
> 
> Hmm, EINVAL is used in wm97xx-core.c file, but you are right.

I am always taking patches ;))

> 
> > 
> >> +{
> >> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
> >> +
> >> +	cancel_work_sync(&data->ts_event_work);
> > 
> > There is a race here, IRQ handler may resubmit work after
> > cancel_work_sync() returns. You need to  make sure that
> > IRQ handler does not resubmit work while device is being shut down.
> 
> ok, how about doing free_irq() before cancel_work_sync() call?
> 

Then there is a risk that your work will try to enable_irq() on irq that
was freed. I am not sure if IRQ core will be happy with it,

-- 
Dmitry

Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Joonyoung Shim]

>>>> +	case INPUT_TYPE_SINGLE:
>>>> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
>>>> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
>>>> +
>>>> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
>>>> +		input_report_abs(data->input_dev, ABS_X, x);
>>>> +		input_report_abs(data->input_dev, ABS_Y, y);
>>>> +		input_report_abs(data->input_dev, ABS_PRESSURE,
>>>> +				DEFAULT_PRESSURE);
>>> If the hardware does not support pressure reading don't fake it.
>>> BTN_TOUCH should be enough to signal touch.
>> MCS-5000 supports pressure reading, but the value of pressure is unstable in
>> my target, so i used the static value defined.
>> I will add pressure reading after more test.
> 
> OK. Alternatively you may indicate in the platform data if pressure
> reading is supported and set ABS_PRESSURE bit and report pressure only
> when you know it works well.

OK, i will add the thing about pressure in the platform data.

>> signal touch? Do you mean single touch?
> 
> I meant "signal" as "convey", "indicate".

I see :)

> 
>>>> +
>>>> +	if (request_irq(data->irq, mcs5000_ts_interrupt,
>>>> +			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
>>>> +		dev_err(&data->client->dev, "Failed to register interrupt\n");
>>>> +		ret = -EINVAL;
>>> Why EINVAL and not EBUSY? Or better yet, why don't you propagate what
>>> reqiest_irq returned?
>> Hmm, EINVAL is used in wm97xx-core.c file, but you are right.
> 
> I am always taking patches ;))

OK, i will send the patch about it.

> 
>>>> +{
>>>> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
>>>> +
>>>> +	cancel_work_sync(&data->ts_event_work);
>>> There is a race here, IRQ handler may resubmit work after
>>> cancel_work_sync() returns. You need to  make sure that
>>> IRQ handler does not resubmit work while device is being shut down.
>> ok, how about doing free_irq() before cancel_work_sync() call?
>>
> 
> Then there is a risk that your work will try to enable_irq() on irq that
> was freed. I am not sure if IRQ core will be happy with it,

Oh, i didn't think about that, then, how about the following patch?
The work handler decides trying to enable_irq() by whether irq is NULL or
not.

@@ -179,7 +179,8 @@ static void mcs5000_ts_irq_worker(struct work_struct *work)
 
 	mcs5000_ts_input_read(data);
 
-	enable_irq(data->irq);
+	if (data->irq)
+		enable_irq(data->irq);
 }
 
 static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id)
@@ -307,8 +308,9 @@ static int mcs5000_ts_remove(struct i2c_client *client)
 {
 	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
 
-	cancel_work_sync(&data->ts_event_work);
 	free_irq(data->irq, data);
+	data->irq = 0;
+	cancel_work_sync(&data->ts_event_work);
 	input_unregister_device(data->input_dev);
 	input_free_device(data->input_dev);
 	kfree(data);

Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Dmitry Torokhov]

On Thursday 14 May 2009 17:49:40 Joonyoung Shim wrote:
> >>>> +	case INPUT_TYPE_SINGLE:
> >>>> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
> >>>> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
> >>>> +
> >>>> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
> >>>> +		input_report_abs(data->input_dev, ABS_X, x);
> >>>> +		input_report_abs(data->input_dev, ABS_Y, y);
> >>>> +		input_report_abs(data->input_dev, ABS_PRESSURE,
> >>>> +				DEFAULT_PRESSURE);
> >>>
> >>> If the hardware does not support pressure reading don't fake it.
> >>> BTN_TOUCH should be enough to signal touch.
> >>
> >> MCS-5000 supports pressure reading, but the value of pressure is
> >> unstable in my target, so i used the static value defined.
> >> I will add pressure reading after more test.
> >
> > OK. Alternatively you may indicate in the platform data if pressure
> > reading is supported and set ABS_PRESSURE bit and report pressure only
> > when you know it works well.
>
> OK, i will add the thing about pressure in the platform data.
>
> >> signal touch? Do you mean single touch?
> >
> > I meant "signal" as "convey", "indicate".
>
> I see :)
>
> >>>> +
> >>>> +	if (request_irq(data->irq, mcs5000_ts_interrupt,
> >>>> +			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
> >>>> +		dev_err(&data->client->dev, "Failed to register interrupt\n");
> >>>> +		ret = -EINVAL;
> >>>
> >>> Why EINVAL and not EBUSY? Or better yet, why don't you propagate what
> >>> reqiest_irq returned?
> >>
> >> Hmm, EINVAL is used in wm97xx-core.c file, but you are right.
> >
> > I am always taking patches ;))
>
> OK, i will send the patch about it.
>
> >>>> +{
> >>>> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
> >>>> +
> >>>> +	cancel_work_sync(&data->ts_event_work);
> >>>
> >>> There is a race here, IRQ handler may resubmit work after
> >>> cancel_work_sync() returns. You need to  make sure that
> >>> IRQ handler does not resubmit work while device is being shut down.
> >>
> >> ok, how about doing free_irq() before cancel_work_sync() call?
> >
> > Then there is a risk that your work will try to enable_irq() on irq that
> > was freed. I am not sure if IRQ core will be happy with it,
>
> Oh, i didn't think about that, then, how about the following patch?
> The work handler decides trying to enable_irq() by whether irq is NULL or
> not.
>
> @@ -179,7 +179,8 @@ static void mcs5000_ts_irq_worker(struct work_struct
> *work)
>
>  	mcs5000_ts_input_read(data);
>
> -	enable_irq(data->irq);
> +	if (data->irq)
> +		enable_irq(data->irq);
>  }
>
>  static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id)
> @@ -307,8 +308,9 @@ static int mcs5000_ts_remove(struct i2c_client *client)
>  {
>  	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
>
> -	cancel_work_sync(&data->ts_event_work);
>  	free_irq(data->irq, data);
> +	data->irq = 0;

Not if you want to enable IRQ 0 every once in a while. I think a separate 
flag, the check in the IRQ handler as opposed to workqueue handler and a 
memory barrier should work though.

> +	cancel_work_sync(&data->ts_event_work);
>  	input_unregister_device(data->input_dev);
>  	input_free_device(data->input_dev);
>  	kfree(data);

-- 
Dmitry

Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

[Joonyoung Shim]

Hi, sorry for long time no reponse.

>>>> +	case INPUT_TYPE_SINGLE:
>>>> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
>>>> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
>>>> +
>>>> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
>>>> +		input_report_abs(data->input_dev, ABS_X, x);
>>>> +		input_report_abs(data->input_dev, ABS_Y, y);
>>>> +		input_report_abs(data->input_dev, ABS_PRESSURE,
>>>> +				DEFAULT_PRESSURE);
>>> If the hardware does not support pressure reading don't fake it.
>>> BTN_TOUCH should be enough to signal touch.
>> MCS-5000 supports pressure reading, but the value of pressure is unstable in
>> my target, so i used the static value defined.
>> I will add pressure reading after more test.
> 
> OK. Alternatively you may indicate in the platform data if pressure
> reading is supported and set ABS_PRESSURE bit and report pressure only
> when you know it works well.

This is my mistake, the MCS-5000 does not support pressure. I confused pressure
and proximity.

>>>> +{
>>>> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
>>>> +
>>>> +	cancel_work_sync(&data->ts_event_work);
>>> There is a race here, IRQ handler may resubmit work after
>>> cancel_work_sync() returns. You need to  make sure that
>>> IRQ handler does not resubmit work while device is being shut down.
>> ok, how about doing free_irq() before cancel_work_sync() call?
>>
> 
> Then there is a risk that your work will try to enable_irq() on irq that
> was freed. I am not sure if IRQ core will be happy with it,
> 

It is no problem to try to enable_irq() on irq that was freed, but a new
problem is balance of disable_irq() and enable_irq().
I refered drivers/net/phy/phy.c file.

I will post patch v2.

thanks.