Re: [PATCH] Driver for AUO In-Cell touchscreens using pixcir ICs

On Mon, 2011-12-05 at 09:20 +0100, Heiko Stübner wrote:

Hi Christoph,

thanks for taking the time to look it over.

comments below

Heiko
Am Sonntag, 4. Dezember 2011, 14:31:41 schrieb Christoph Fritz:

quoted

Hi Heiko,

 you can find some annotations below.

Thanks,
 -- chf

On Sat, 2011-12-03 at 15:09 +0100, Heiko Stübner wrote:

quoted

Some displays from AUO have a so called in-cell touchscreen, meaning it
is built directly into the display unit.

Touchdata is gathered through PIXCIR Tango-ICs and processed in an
Atmel ATmega168P with custom firmware. Communication between the host
system and ATmega is done via I2C.

Devices using this touch solution include the Dell Streak5 and the family
of Qisda ebook readers.

The driver reports single- and multi-touch events including touch area
values.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---

 drivers/input/touchscreen/Kconfig         |   14 +
 drivers/input/touchscreen/Makefile        |    1 +
 drivers/input/touchscreen/auo-pixcir-ts.c |  657
 +++++++++++++++++++++++++++++ include/linux/input/auo-pixcir-ts.h      
 |   57 +++
 4 files changed, 729 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/touchscreen/auo-pixcir-ts.c
 create mode 100644 include/linux/input/auo-pixcir-ts.h

[...]

quoted

quoted

+
+static int auo_pixcir_int_config(struct auo_pixcir_ts *ts, int
int_setting) +{
+	struct i2c_client *client = ts->client;
+	struct auo_pixcir_ts_platdata *pdata = client->dev.platform_data;
+	uint8_t value;
+
+	value = i2c_smbus_read_byte_data(client, AUO_PIXCIR_REG_INT_SETTING);
+	if (value < 0) {

value is an unsigned int and can't be less than zero. An int value would
be more appropriate.

you're right of course and I will fix the type

quoted

quoted

+		dev_err(&client->dev, "unable to read reg %Xh, %d\n",
+			AUO_PIXCIR_REG_INT_SETTING, value);
+		return value;
+	}
+
+	value &= (~AUO_PIXCIR_INT_MODE_MASK);
+	value |= int_setting;
+	value |= AUO_PIXCIR_INT_POL_HIGH; /* always use high for interrupts 

*/

quoted

quoted

+
+	value = i2c_smbus_write_byte_data(client, AUO_PIXCIR_REG_INT_SETTING,
+					  value);
+	if (value < 0) {

the same here

here too

[...]

quoted

quoted

+static int __devinit auo_pixcir_probe(struct i2c_client *client,
+				      const struct i2c_device_id *id)
+{
+	struct auo_pixcir_ts *ts;
+	struct auo_pixcir_ts_platdata *pdata = client->dev.platform_data;
+	struct input_dev *input_dev;
+	int ret;
+
+	if (!pdata)
+		return -EINVAL;
+
+	ts = kzalloc(sizeof(struct auo_pixcir_ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	ret = gpio_request(pdata->gpio_int, "auo_pixcir_ts_int");
+	if (ret) {
+		dev_err(&client->dev, "request of gpio %d failed, %d\n",
+			pdata->gpio_int, ret);
+		goto err_gpio_int;
+	}
+
+	ret = gpio_request(pdata->gpio_rst, "auo_pixcir_ts_rst");
+	if (ret) {
+		dev_err(&client->dev, "request of gpio %d failed, %d\n",
+			pdata->gpio_int, ret);
+		goto err_gpio_rst;
+	}
+
+	if (pdata->init_hw)
+		pdata->init_hw();
+
+	ts->client = client;
+	ts->touch_ind_mode = 0;
+	init_waitqueue_head(&ts->wait);
+
+	snprintf(ts->phys, sizeof(ts->phys),
+		 "%s/input0", dev_name(&client->dev));
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		dev_err(&client->dev, "could not allocate input device\n");
+		goto err_input_alloc;
+	}
+
+	ts->input = input_dev;
+
+	input_dev->name = "auo_pixcir_ts";
+	input_dev->phys = ts->phys;
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = &client->dev;
+
+	input_dev->open = auo_pixcir_input_open;
+	input_dev->close = auo_pixcir_input_close;
+
+	__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, pdata->x_max, 0, 0);
+	input_set_abs_params(input_dev, ABS_Y, 0, pdata->y_max, 0, 0);
+
+	/* For multi touch */
+	input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
+			     pdata->x_max, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
+			     pdata->y_max, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
+			     AUO_PIXCIR_MAX_AREA, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0,
+			     AUO_PIXCIR_MAX_AREA, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_ORIENTATION, 0, 1, 0, 0);
+
+	ret = i2c_smbus_read_byte_data(client, AUO_PIXCIR_REG_VERSION);
+	if (ret < 0)
+		goto err_fw_vers;
+	dev_info(&client->dev, "firmware version 0x%X\n", ret);
+
+	ret = auo_pixcir_int_config(ts, pdata->int_setting);
+	if (ret)
+		goto err_fw_vers;
+
+	/* disable irqs on the device side before irq-request */
+	auo_pixcir_int_enable(ts, 0);
+	ts->stopped = true;
+	ts->is_open = false;
+
+	ret = request_threaded_irq(client->irq, NULL, auo_pixcir_interrupt,
+				   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+				   input_dev->name, ts);
+	if (ret) {
+		dev_err(&client->dev, "irq %d requested failed\n", client->irq);
+		goto err_fw_vers;
+	}
+
+	/* stop device and put it into deep sleep until it is opened */
+	ret = auo_pixcir_stop(ts);
+	if (ret < 0)
+		goto err_input_register;
+
+	ret = input_register_device(input_dev);
+	if (ret) {
+		dev_err(&client->dev, "could not register input device\n");
+		goto err_input_register;
+	}
+
+	input_set_drvdata(ts->input, ts);
+
+	i2c_set_clientdata(client, ts);
+
+	return 0;
+
+err_input_register:
+	free_irq(client->irq, ts);
+err_fw_vers:
+	input_free_device(input_dev);
+err_input_alloc:
+	if (pdata->exit_hw)
+		pdata->exit_hw();
+	gpio_free(pdata->gpio_rst);
+err_gpio_rst:
+	gpio_free(pdata->gpio_int);
+err_gpio_int:
+	kfree(ts);
+
+	return ret;
+}
+
+static int __devexit auo_pixcir_remove(struct i2c_client *client)
+{
+	struct auo_pixcir_ts *ts = (struct auo_pixcir_ts *)
+						i2c_get_clientdata(client);
+	struct auo_pixcir_ts_platdata *pdata = client->dev.platform_data;
+
+	auo_pixcir_stop(ts);
+
+	free_irq(client->irq, ts);
+
+	input_unregister_device(ts->input);
+	input_free_device(ts->input);

to quote input.c:

/**
 * input_free_device - free memory occupied by input_dev structure
 * @dev: input device to free
 *
 * This function should only be used if input_register_device()
 * was not called yet or if it failed. Once device was registered
 * use input_unregister_device() and memory will be freed once last
 * reference to the device is dropped.
 *
 * Device should be allocated by input_allocate_device().
 *
 * NOTE: If there are references to the input device then memory
 * will not be freed until last reference is dropped.
 */

thanks for this quote and I will remove the input_free_device call.

quoted

the same applies to the error path in auo_pixcir_probe()

according to the docs your quoted this wouldn't be the same, as 
input_free_device is called in _probe either before the input_register_device 
call or if the call to it failed.

Yes, it's done right.

 Thanks,
   -- chf


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html