Hi Simon,
> This is a driver for the EDT "Polytouch" family of touch controllers
> based on the FocalTech FT5x06 line of chips.
>
> Signed-off-by: Simon Budig <simon.budig@xxxxxxxxxxxxxxxxx>
> ---
Thank you for the thorough set of changes. Some minor comments below.
> +#define MIN(a, b) (((a) < (b)) ? (a) : (b))
We have min() defined in kernel.h.
> +static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
> +{
> + struct edt_ft5x06_ts_data *tsdata = dev_id;
> + struct device *dev = &tsdata->client->dev;
> + u8 cmd = 0xf9;
> + u8 rdbuf[26];
> + u8 crc;
> + int i, type, x, y, id;
> + int error;
> +
> + memset(rdbuf, 0, sizeof(rdbuf));
> +
> + error = edt_ft5x06_ts_readwrite(tsdata->client,
> + sizeof(cmd), &cmd,
> + sizeof(rdbuf), rdbuf);
> + if (error) {
> + dev_err_ratelimited(dev, "Unable to fetch data, error: %d\n",
> + error);
> + goto out;
> + }
> +
> + if (rdbuf[0] != 0xaa || rdbuf[1] != 0xaa || rdbuf[2] != 26) {
> + dev_err_ratelimited(dev, "Unexpected header: %02x%02x%02x!\n",
> + rdbuf[0], rdbuf[1], rdbuf[2]);
> + goto out;
> + }
> +
> + crc = 0;
> + for (i = 0; i < 25; i++)
> + crc ^= rdbuf[i];
> + if (crc != rdbuf[25]) {
A separate function for the crc check would be nice.
> + dev_err_ratelimited(dev,
> + "crc error: 0x%02x expected, got 0x%02x\n",
> + crc, rdbuf[25]);
It is alright to let the string exceed 80 characters, even by checkpatch standards.
> + goto out;
> + }
> +
> + for (i = 0; i < MAX_SUPPORT_POINTS; i++) {
> + u8 *buf = &rdbuf[i * 4];
> + bool down;
> +
> + type = buf[5] >> 6;
> + /* ignore Reserved events */
> + if (type == TOUCH_EVENT_RESERVED)
> + continue;
> +
> + x = ((buf[5] << 8) | buf[6]) & 0x0fff;
> + y = ((buf[7] << 8) | buf[8]) & 0x0fff;
> + id = (buf[7] >> 4) & 0x0f;
> + down = (type != TOUCH_EVENT_UP);
> +
> + input_mt_slot(tsdata->input, id);
> + input_mt_report_slot_state(tsdata->input, MT_TOOL_FINGER, down);
> +
> + if (!down)
> + continue;
> +
> + input_report_abs(tsdata->input, ABS_MT_POSITION_X, x);
> + input_report_abs(tsdata->input, ABS_MT_POSITION_Y, y);
> + }
> +
> + input_mt_report_pointer_emulation(tsdata->input, true);
> + input_sync(tsdata->input);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
> + char *buf,
> + size_t count,
> + loff_t *off)
> +{
> + struct edt_ft5x06_ts_data *tsdata = file->private_data;
> + int retries = EDT_RAW_DATA_RETRIES;
> + int ret = 0, error;
> + int colbytes;
> + int pos, endpos, start_off;
> + char wrbuf[3];
> + char *rdbuf;
> +
> + colbytes = tsdata->num_y * 2;
> +
> + if (*off < 0 || *off >= tsdata->num_x * colbytes)
> + return 0;
> +
> + rdbuf = kmalloc(colbytes, GFP_KERNEL);
> + if (!rdbuf)
> + return -ENOMEM;
> +
> + mutex_lock(&tsdata->mutex);
> +
> + if (!tsdata->factory_mode) {
> + error = -EIO;
> + goto out;
> + }
> +
> + error = edt_ft5x06_register_write(tsdata, 0x08, 0x01);
> + if (error) {
> + dev_dbg(&tsdata->client->dev,
> + "failed to write 0x08 register, error %d\n",
> + error);
> + goto out;
> + }
> +
> + do {
> + msleep(EDT_RAW_DATA_DELAY);
> + ret = edt_ft5x06_register_read(tsdata, 0x08);
> + if (ret < 1)
> + break;
> + } while (--retries > 0);
> +
> + if (ret < 0) {
> + error = ret;
> + dev_dbg(&tsdata->client->dev,
> + "failed to read 0x08 register, error %d\n",
> + error);
> + goto out;
> + }
> +
> + if (retries == 0) {
> + dev_dbg(&tsdata->client->dev,
> + "timed out waiting for register to settle\n");
> + error = -ETIMEDOUT;
> + goto out;
> + }
> +
> + endpos = MIN(*off + count, colbytes * tsdata->num_x);
> +
> + wrbuf[0] = 0xf5;
> + wrbuf[1] = 0x0e;
> + for (pos = *off; pos < endpos; pos += colbytes) {
> + wrbuf[2] = pos / colbytes; /* column index */
> + error = edt_ft5x06_ts_readwrite(tsdata->client,
> + sizeof(wrbuf), wrbuf,
> + colbytes, rdbuf);
> + if (error)
> + goto out;
> +
> + start_off = pos % colbytes;
> + error = copy_to_user(buf + pos - *off, rdbuf + start_off,
> + MIN(colbytes - start_off, endpos - pos));
Quite a few variables in play here... it looks correct, but a)
breaking out parts of this function would not hurt, and b) I bet the
column-by column copy-to-user algorithm could be slightly less
involved.
> + if (error)
> + goto out;
> +
> + pos -= start_off;
> + }
> +
> + ret = endpos - *off;
> + if (!error)
> + *off += ret;
> +out:
> + mutex_unlock(&tsdata->mutex);
> + kfree(rdbuf);
> + return error ?: ret;
> +};
> +static int __devinit edt_ft5x06_ts_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> +
> + const struct edt_ft5x06_platform_data *pdata =
> + client->dev.platform_data;
> + struct edt_ft5x06_ts_data *tsdata;
> + struct input_dev *input;
> + int error;
> + char fw_version[EDT_NAME_LEN];
> +
> + dev_dbg(&client->dev, "probing for EDT FT5x06 I2C\n");
> +
> + if (!pdata) {
> + dev_err(&client->dev, "no platform data?\n");
> + return -EINVAL;
> + }
> +
> + error = edt_ft5x06_ts_reset(client, pdata->reset_pin);
> + if (error)
> + return error;
> +
> + if (gpio_is_valid(pdata->irq_pin)) {
> + error = gpio_request_one(pdata->irq_pin,
> + GPIOF_IN, "edt-ft5x06 irq");
> + if (error) {
> + dev_err(&client->dev,
> + "Failed to request GPIO %d, error %d\n",
> + pdata->irq_pin, error);
> + return error;
> + }
> + }
> +
> + tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
> + input = input_allocate_device();
> + if (!tsdata || !input) {
> + dev_err(&client->dev, "failed to allocate driver data.\n");
> + error = -ENOMEM;
> + goto err_free_mem;
> + }
> +
> + mutex_init(&tsdata->mutex);
> + tsdata->client = client;
> + tsdata->input = input;
> + tsdata->factory_mode = false;
> +
> + error = edt_ft5x06_ts_identify(client, tsdata->name, fw_version);
> + if (error) {
> + dev_err(&client->dev, "touchscreen probe failed\n");
> + goto err_free_mem;
> + }
> +
> + if (pdata->use_parameters) {
> + /* pick up defaults from the platform data */
> + if (pdata->threshold >= edt_ft5x06_attr_threshold.limit_low &&
> + pdata->threshold <= edt_ft5x06_attr_threshold.limit_high)
> + edt_ft5x06_register_write(tsdata,
> + WORK_REGISTER_THRESHOLD,
> + pdata->threshold);
> + if (pdata->gain >= edt_ft5x06_attr_gain.limit_low &&
> + pdata->gain <= edt_ft5x06_attr_gain.limit_high)
> + edt_ft5x06_register_write(tsdata,
> + WORK_REGISTER_GAIN,
> + pdata->gain);
> + if (pdata->offset >= edt_ft5x06_attr_offset.limit_low &&
> + pdata->offset <= edt_ft5x06_attr_offset.limit_high)
> + edt_ft5x06_register_write(tsdata,
> + WORK_REGISTER_OFFSET,
> + pdata->offset);
> + if (pdata->report_rate
> + >= edt_ft5x06_attr_report_rate.limit_low &&
> + pdata->report_rate
> + <= edt_ft5x06_attr_report_rate.limit_high)
> + edt_ft5x06_register_write(tsdata,
> + WORK_REGISTER_REPORT_RATE,
> + pdata->report_rate);
> + }
Putting this in a function, perhaps?
> +
> + tsdata->threshold = edt_ft5x06_register_read(tsdata,
> + WORK_REGISTER_THRESHOLD);
> + tsdata->gain = edt_ft5x06_register_read(tsdata, WORK_REGISTER_GAIN);
> + tsdata->offset = edt_ft5x06_register_read(tsdata, WORK_REGISTER_OFFSET);
> + tsdata->report_rate = edt_ft5x06_register_read(tsdata,
> + WORK_REGISTER_REPORT_RATE);
> + tsdata->num_x = edt_ft5x06_register_read(tsdata, WORK_REGISTER_NUM_X);
> + tsdata->num_y = edt_ft5x06_register_read(tsdata, WORK_REGISTER_NUM_Y);
And this?
> +
> + dev_dbg(&client->dev,
> + "Model \"%s\", Rev. \"%s\", %dx%d sensors\n",
> + tsdata->name, fw_version, tsdata->num_x, tsdata->num_y);
> +
> + input->name = tsdata->name;
> + input->id.bustype = BUS_I2C;
> + input->dev.parent = &client->dev;
> +
> + __set_bit(EV_SYN, input->evbit);
> + __set_bit(EV_KEY, input->evbit);
> + __set_bit(EV_ABS, input->evbit);
> + __set_bit(BTN_TOUCH, input->keybit);
> + input_set_abs_params(input, ABS_X, 0, tsdata->num_x * 64 - 1, 0, 0);
> + input_set_abs_params(input, ABS_Y, 0, tsdata->num_y * 64 - 1, 0, 0);
> + input_set_abs_params(input, ABS_MT_POSITION_X,
> + 0, tsdata->num_x * 64 - 1, 0, 0);
> + input_set_abs_params(input, ABS_MT_POSITION_Y,
> + 0, tsdata->num_y * 64 - 1, 0, 0);
> + error = input_mt_init_slots(input, MAX_SUPPORT_POINTS);
> + if (error) {
> + dev_err(&client->dev, "Unable to init MT slots.\n");
> + goto err_free_mem;
> + }
> +
> + input_set_drvdata(input, tsdata);
> + i2c_set_clientdata(client, tsdata);
> +
> + error = request_threaded_irq(client->irq, NULL, edt_ft5x06_ts_isr,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + client->name, tsdata);
> + if (error) {
> + dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> + goto err_free_mem;
> + }
> +
> + error = sysfs_create_group(&client->dev.kobj, &edt_ft5x06_attr_group);
> + if (error)
> + goto err_free_irq;
> +
> + error = input_register_device(input);
> + if (error)
> + goto err_remove_attrs;
> +
> +#if defined(CONFIG_DEBUG_FS)
> + tsdata->debug_dir = debugfs_create_dir(dev_driver_string(&client->dev),
> + NULL);
> +
> + if (tsdata->debug_dir) {
> + debugfs_create_u16("num_x", S_IRUSR, tsdata->debug_dir,
> + &tsdata->num_x);
> + debugfs_create_u16("num_y", S_IRUSR, tsdata->debug_dir,
> + &tsdata->num_y);
> + debugfs_create_file("mode", S_IRUSR | S_IWUSR,
> + tsdata->debug_dir, tsdata,
> + &debugfs_mode_fops);
> + debugfs_create_file("raw_data", S_IRUSR,
> + tsdata->debug_dir, tsdata,
> + &debugfs_raw_data_fops);
> + }
> +#endif
And this?
> +
> + device_init_wakeup(&client->dev, 1);
> +
> + dev_dbg(&tsdata->client->dev,
> + "EDT FT5x06 initialized: IRQ pin %d, Reset pin %d.\n",
> + pdata->irq_pin, pdata->reset_pin);
> +
> + return 0;
> +
> +err_remove_attrs:
> + sysfs_remove_group(&client->dev.kobj, &edt_ft5x06_attr_group);
> +err_free_irq:
> + free_irq(client->irq, tsdata);
> +err_free_mem:
> + input_free_device(input);
> + kfree(tsdata);
> +
> + if (gpio_is_valid(pdata->irq_pin))
> + gpio_free(pdata->irq_pin);
> +
> + return error;
> +}
Thanks,
Henrik
--
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
