Re: [PATCH] input: qt602240 - Add ATMEL QT602240 touchscreen driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Joonyoung,

On Wed, Jun 23, 2010 at 10:16:07PM +0900, Joonyoung Shim wrote:
> +
> +static void qt602240_input_read(struct qt602240_data *data)
> +{
> +	struct qt602240_message message;
> +	struct qt602240_object *object;
> +	struct device *dev = &data->client->dev;
> +	struct input_dev *input_dev = data->input_dev;
> +	u8 reportid;
> +	u8 max_reportid;
> +	u8 min_reportid;
> +
> +repeat:
> +	if (qt602240_read_message(data, &message)) {
> +		dev_err(dev, "Failed to read message\n");
> +		return;
> +	}
> +
> +	reportid = message.reportid;
> +
> +	/* Check it remains the message to process */
> +	if (reportid == 0xff)
> +		return;
> +
> +	/* whether reportid is thing of QT602240_TOUCH_MULTI */
> +	object = qt602240_get_object(data, QT602240_TOUCH_MULTI);
> +	if (!object)
> +		return;
> +
> +	max_reportid = object->max_reportid;
> +	min_reportid = max_reportid - object->num_report_ids + 1;
> +
> +	if ((reportid >= min_reportid) && (reportid <= max_reportid)) {
> +		u8 id;
> +		u8 status;
> +		int x;
> +		int y;
> +		int area;
> +		int finger_num = 0;
> +
> +		id = reportid - min_reportid;
> +		status = message.message[0];
> +
> +		/* Check the touch is present on the screen */
> +		if (!(status & QT602240_DETECT))
> +			goto release;
> +
> +		/* Check only AMP detection */
> +		if (!(status & (QT602240_PRESS | QT602240_MOVE)))
> +			goto repeat;
> +
> +		x = (message.message[1] << 2) |
> +			((message.message[3] & ~0x3f) >> 6);
> +		y = (message.message[2] << 2) |
> +			((message.message[3] & ~0xf3) >> 2);
> +		area = message.message[4];
> +
> +		dev_dbg(dev, "[%d] %s x: %d, y: %d, area: %d\n", id,
> +				status & QT602240_MOVE ?  "moved" : "pressed",
> +				x, y, area);
> +
> +		data->finger[id].status = status & QT602240_MOVE ?
> +			QT602240_MOVE : QT602240_PRESS;
> +		data->finger[id].x = x;
> +		data->finger[id].y = y;
> +		data->finger[id].area = area;
> +
> +		input_report_key(input_dev, BTN_TOUCH, 1);
> +		input_report_abs(input_dev, ABS_X, x);
> +		input_report_abs(input_dev, ABS_Y, y);
> +
> +		goto mt_report;
> +
> +release:
> +		if (status & QT602240_RELEASE) {
> +			dev_dbg(dev, "[%d] released\n", id);
> +
> +			data->finger[id].status = QT602240_RELEASE;
> +			data->finger[id].area = 0;
> +		}
> +
> +mt_report:
> +		for (id = 0; id < QT602240_MAX_FINGER; id++) {
> +			if (!data->finger[id].status)
> +				continue;
> +
> +			input_report_abs(input_dev, ABS_MT_TRACKING_ID, id);
> +			input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> +					data->finger[id].area);
> +
> +			if (data->finger[id].status == QT602240_RELEASE)
> +				data->finger[id].status = 0;
> +			else {
> +				input_report_abs(input_dev, ABS_MT_POSITION_X,
> +						data->finger[id].x);
> +				input_report_abs(input_dev, ABS_MT_POSITION_Y,
> +						data->finger[id].y);
> +				finger_num++;
> +			}
> +
> +			input_mt_sync(input_dev);
> +		}
> +
> +		if (!finger_num)
> +			input_report_key(input_dev, BTN_TOUCH, 0);
> +		input_sync(input_dev);
> +	} else {
> +		qt602240_dump_message(dev, &message);
> +		qt602240_check_config_error(data, &message, reportid);
> +	}
> +
> +	goto repeat;

I really dislike gotos that implement logic flow control (i.e. for me
gotos are acceptable in error path and in one-off cases when you restart
processing, like the CRC error in one of the functions above). Please do
not use old Fortran stylein kernel.

> +}
> +
> +static void qt602240_irq_worker(struct work_struct *work)
> +{
> +	struct qt602240_data *data = container_of(work,
> +			struct qt602240_data, ts_event_work);
> +
> +	qt602240_input_read(data);
> +}
> +
> +static void qt602240_disable_worker(struct work_struct *work)
> +{
> +	struct qt602240_data *data = container_of(work,
> +			struct qt602240_data, ts_disable_work);
> +
> +	/* Soft reset */
> +	qt602240_write_object(data, QT602240_GEN_COMMAND,
> +			QT602240_COMMAND_RESET, 1);
> +
> +	msleep(QT602240_RESET_TIME);
> +
> +	/* Touch enable */
> +	qt602240_write_object(data, QT602240_TOUCH_MULTI, QT602240_TOUCH_CTRL,
> +			0x83);
> +}
> +
> +static irqreturn_t qt602240_interrupt(int irq, void *dev_id)
> +{
> +	struct qt602240_data *data = dev_id;
> +
> +	if (!work_pending(&data->ts_event_work))
> +		schedule_work(&data->ts_event_work);
> +

Thios begs for use of threaded IRQs.

> +	return IRQ_HANDLED;
> +}
> +
> +
> +static int qt602240_initialize(struct qt602240_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	struct qt602240_info *info;
> +	int i;
> +	int ret;
> +	u16 reg;
> +	u8 buf[QT602240_OBJECT_SIZE];
> +	u8 reportid = 0;
> +
> +	info = data->info = kzalloc(sizeof(struct qt602240_info), GFP_KERNEL);

Why do you allocate it separately instead of embedding (entire
structure) into qt602240_data?

> +
> +	if (!info) {
> +		dev_err(&data->client->dev, "Failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +
> +		pos += frame_size;
> +
> +		dev_info(dev, "Updated %zd bytes / %zd bytes\n", pos, fw->size);

dev_dbg() maybe?

> +	}
> +
> +err_fw:
> +	/* Change to slave address of application */
> +	if (data->client->addr == QT602240_BOOT_LOW)
> +		data->client->addr = QT602240_APP_LOW;
> +	else
> +		data->client->addr = QT602240_APP_HIGH;
> +
> +	return ret;
> +}
> +
> +static ssize_t qt602240_update_fw_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct qt602240_data *data = dev_get_drvdata(dev);
> +	unsigned int version;
> +	int ret;
> +
> +	if (sscanf(buf, "%u", &version) != 1) {
> +		dev_err(dev, "Invalid values\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Firmware update supports from version 21 */
> +	if ((data->info->version < QT602240_VER_21) ||
> +			(version < QT602240_VER_21)) {
> +		dev_err(dev, "Firmware update supports from version 21\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Interrupt disable */
> +	disable_irq(data->irq);
> +
> +	ret = qt602240_load_fw(dev, QT602240_FW_NAME);
> +	if (ret < 0)
> +		dev_err(dev, "The firmware update failed(%d)\n", ret);
> +	else {
> +		dev_info(dev, "The firmware update successed\n");

dev_dbg() as well.

> +
> +		/* Wait for reset */
> +		msleep(QT602240_FWRESET_TIME);
> +
> +		kfree(data->object_table);
> +		kfree(data->info);
> +
> +		qt602240_initialize(data);
> +	}
> +
> +	/* Interrupt enable */
> +	enable_irq(data->irq);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(object, 0444, qt602240_object_show, NULL);
> +static DEVICE_ATTR(update_fw, 0664, NULL, qt602240_update_fw_store);
> +
> +static struct attribute *qt602240_attrs[] = {
> +	&dev_attr_object.attr,
> +	&dev_attr_update_fw.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group qt602240_attr_group = {
> +	.attrs = qt602240_attrs,
> +};
> +
> +static int __devinit qt602240_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct qt602240_data *data;
> +	struct input_dev *input_dev = NULL;

No need to initialize to NULL.

> +	int ret;
> +
> +	if (!client->dev.platform_data)
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(struct qt602240_data), GFP_KERNEL);
> +
> +	input_dev = input_allocate_device();
> +	if (!data || !input_dev) {
> +		dev_err(&client->dev, "Failed to allocate memory\n");
> +		ret = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	input_dev->name = "AT42QT602240/ATMXT224 Touchscreen";
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = &client->dev;
> +
> +	__set_bit(EV_ABS, input_dev->evbit);
> +	__set_bit(EV_KEY, input_dev->evbit);
> +	__set_bit(BTN_TOUCH, input_dev->keybit);
> +
> +	/* For single touch */
> +	input_set_abs_params(input_dev, ABS_X, 0, QT602240_MAX_XC, 0,
> +			0);
> +	input_set_abs_params(input_dev, ABS_Y, 0, QT602240_MAX_YC, 0,
> +			0);
> +
> +	/* For multi touch */
> +	input_set_abs_params(input_dev, ABS_MT_TRACKING_ID, 0,
> +			QT602240_MAX_ID, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> +			QT602240_MAX_AREA, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
> +			QT602240_MAX_XC, 0, 0);
> +	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
> +			QT602240_MAX_YC, 0, 0);
> +
> +	input_set_drvdata(input_dev, data);
> +
> +	INIT_WORK(&data->ts_event_work, qt602240_irq_worker);
> +	INIT_WORK(&data->ts_disable_work, qt602240_disable_worker);
> +	data->client = client;
> +	data->input_dev = input_dev;
> +	data->pdata = client->dev.platform_data;
> +	data->irq = client->irq;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	ret = qt602240_initialize(data);
> +	if (ret < 0)
> +		goto err_free_object;
> +
> +	ret = request_irq(client->irq, qt602240_interrupt, IRQF_TRIGGER_FALLING,
> +			client->dev.driver->name, data);
> +
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to register interrupt\n");
> +		goto err_free_object;
> +	}
> +
> +	ret = input_register_device(input_dev);
> +	if (ret < 0)
> +		goto err_free_irq;
> +
> +	ret = sysfs_create_group(&client->dev.kobj, &qt602240_attr_group);
> +	if (ret)
> +		goto err_free_irq;

After input_register_device() succeeded you need to call
input_unregister_device().

> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(client->irq, data);
> +err_free_object:
> +	kfree(data->object_table);
> +	kfree(data->info);
> +err_free_mem:
> +	input_free_device(input_dev);
> +	kfree(data);
> +	return ret;
> +}
> +
> +static int __devexit qt602240_remove(struct i2c_client *client)
> +{
> +	struct qt602240_data *data = i2c_get_clientdata(client);
> +
> +	free_irq(data->irq, data);
> +	cancel_work_sync(&data->ts_event_work);
> +	cancel_work_sync(&data->ts_disable_work);
> +	input_unregister_device(data->input_dev);
> +	kfree(data->object_table);
> +	kfree(data->info);
> +	kfree(data);
> +
> +	sysfs_remove_group(&client->dev.kobj, &qt602240_attr_group);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int qt602240_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	struct qt602240_data *data = i2c_get_clientdata(client);
> +
> +	/* Touch disable */
> +	qt602240_write_object(data, QT602240_TOUCH_MULTI, QT602240_TOUCH_CTRL,
> +			0);
> +
> +	qt602240_write_object(data, QT602240_GEN_POWER,
> +			QT602240_POWER_IDLEACQINT, 0);
> +	qt602240_write_object(data, QT602240_GEN_POWER,
> +			QT602240_POWER_ACTVACQINT, 0);
> +
> +	return 0;
> +}
> +
> +static int qt602240_resume(struct i2c_client *client)
> +{
> +	struct qt602240_data *data = i2c_get_clientdata(client);
> +
> +	schedule_work(&data->ts_disable_work);

Hmm, what happens if you go through suspend and resume process very
quickly? Won't your suspend fight with pending resume work? Also, name
of the work is pretty confusing.. Why is it ts_disable_work?

Also, what about open() and close() methods for the input device?

Andplease CC Henrik Rydberg <rydberg@euromail.se> so he can take a look
at the detail of MT protocol.

Thanks!

-- 
Dmitry