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:
> > Hi Heiko,
> >
> > you can find some annotations below.
> >
> > Thanks,
> > -- chf
> >
> > On Sat, 2011-12-03 at 15:09 +0100, Heiko Stübner wrote:
> > > 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@xxxxxxxxx>
> > > ---
> > >
> > > 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
>
> [...]
>
> > > +
> > > +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
>
> > > + 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
> */
> > > +
> > > + value = i2c_smbus_write_byte_data(client, AUO_PIXCIR_REG_INT_SETTING,
> > > + value);
> > > + if (value < 0) {
> >
> > the same here
> here too
>
> [...]
>
> > > +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.
>
>
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
