Re: [PATCH] Input: add driver for Neonode zForce based touchscreens
From: Heiko Stübner
Date: Thu Aug 29 2013 - 19:37:48 EST
Hi Dmitry,
Am Donnerstag, 29. August 2013, 18:29:04 schrieb Dmitry Torokhov:
> Hi Heiko,
>
> On Fri, Aug 16, 2013 at 01:59:39PM +0200, Heiko Stübner wrote:
> > This adds a driver for touchscreens using the zforce infrared
> > technology from Neonode connected via i2c to the host system.
> >
> > It supports multitouch with up to two fingers and tracking of the
> > contacts in hardware.
>
> Generally looks good, just a few comments...
>
> > Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> > ---
> >
> > drivers/input/touchscreen/Kconfig | 13 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/zforce_ts.c | 837
> > +++++++++++++++++++++++++++++++++ include/linux/input/zforce_ts.h
> > | 26 +
> > 4 files changed, 877 insertions(+)
> > create mode 100644 drivers/input/touchscreen/zforce_ts.c
> > create mode 100644 include/linux/input/zforce_ts.h
> >
> > diff --git a/drivers/input/touchscreen/Kconfig
> > b/drivers/input/touchscreen/Kconfig index 3b9758b..ade11b7 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -919,4 +919,17 @@ config TOUCHSCREEN_TPS6507X
> >
> > To compile this driver as a module, choose M here: the
> > module will be called tps6507x_ts.
> >
> > +config TOUCHSCREEN_ZFORCE
> > + tristate "Neonode zForce infrared touchscreens"
> > + depends on I2C
> > + depends on GPIOLIB
> > + help
> > + Say Y here if you have a touchscreen using the zforce
> > + infraread technology from Neonode.
> > +
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called zforce_ts.
> > +
> >
> > endif
> >
> > diff --git a/drivers/input/touchscreen/Makefile
> > b/drivers/input/touchscreen/Makefile index f5216c1..7587883 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -75,3 +75,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE) +=
> > mainstone-wm97xx.o
> >
> > obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE) += zylonite-wm97xx.o
> > obj-$(CONFIG_TOUCHSCREEN_W90X900) += w90p910_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_TPS6507X) += tps6507x-ts.o
> >
> > +obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> > diff --git a/drivers/input/touchscreen/zforce_ts.c
> > b/drivers/input/touchscreen/zforce_ts.c new file mode 100644
> > index 0000000..92af632
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/zforce_ts.c
> > @@ -0,0 +1,837 @@
> > +/*
> > + * Copyright (C) 2012-2013 MundoReader S.L.
> > + * Author: Heiko Stuebner <heiko@xxxxxxxxx>
> > + *
> > + * based in parts on Nook zforce driver
> > + *
> > + * Copyright (C) 2010 Barnes & Noble, Inc.
> > + * Author: Pieter Truter<ptruter@xxxxxxxxxxxxx>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/slab.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/input/zforce_ts.h>
> > +#include <linux/input/mt.h>
> > +
> > +#define WAIT_TIMEOUT msecs_to_jiffies(1000)
> > +
> > +#define FRAME_START 0xee
> > +
> > +/* Offsets of the different parts of the payload the controller sends */
> > +#define PAYLOAD_HEADER 0
> > +#define PAYLOAD_LENGTH 1
> > +#define PAYLOAD_BODY 2
> > +
> > +/* Response offsets */
> > +#define RESPONSE_ID 0
> > +#define RESPONSE_DATA 1
> > +
> > +/* Commands */
> > +#define COMMAND_DEACTIVATE 0x00
> > +#define COMMAND_INITIALIZE 0x01
> > +#define COMMAND_RESOLUTION 0x02
> > +#define COMMAND_SETCONFIG 0x03
> > +#define COMMAND_DATAREQUEST 0x04
> > +#define COMMAND_SCANFREQ 0x08
> > +#define COMMAND_STATUS 0X1e
> > +
> > +/*
> > + * Responses the controller sends as a result of
> > + * command requests
> > + */
> > +#define RESPONSE_DEACTIVATE 0x00
> > +#define RESPONSE_INITIALIZE 0x01
> > +#define RESPONSE_RESOLUTION 0x02
> > +#define RESPONSE_SETCONFIG 0x03
> > +#define RESPONSE_SCANFREQ 0x08
> > +#define RESPONSE_STATUS 0X1e
> > +
> > +/*
> > + * Notifications are send by the touch controller without
> > + * being requested by the driver and include for example
> > + * touch indications
> > + */
> > +#define NOTIFICATION_TOUCH 0x04
> > +#define NOTIFICATION_BOOTCOMPLETE 0x07
> > +#define NOTIFICATION_OVERRUN 0x25
> > +#define NOTIFICATION_PROXIMITY 0x26
> > +#define NOTIFICATION_INVALID_COMMAND 0xfe
> > +
> > +#define ZFORCE_REPORT_POINTS 2
> > +#define ZFORCE_MAX_AREA 0xff
> > +
> > +#define STATE_DOWN 0
> > +#define STATE_MOVE 1
> > +#define STATE_UP 2
> > +
> > +#define SETCONFIG_DUALTOUCH (1 << 0)
> > +
> > +struct zforce_point {
> > + int coord_x;
> > + int coord_y;
> > + int state;
> > + int id;
> > + int area_major;
> > + int area_minor;
> > + int orientation;
> > + int pressure;
> > + int prblty;
> > +};
> > +
> > +/*
> > + * @client the i2c_client
> > + * @input the input device
> > + * @suspending in the process of going to suspend (don't emit
wakeup
> > + * events for commands executed to suspend the device)
> > + * @suspended device suspended
> > + * @access_mutex serialize i2c-access, to keep multipart reads together
> > + * @command_done completion to wait for the command result
> > + * @command_mutex serialize commands send to the ic
> > + * @command_waiting the id of the command that that is currently
waiting
> > + * for a result
> > + * @command_result returned result of the command
> > + */
> > +struct zforce_ts {
> > + struct i2c_client *client;
> > + struct input_dev *input;
> > + const struct zforce_ts_platdata *pdata;
> > + char phys[32];
> > +
> > + bool suspending;
> > + bool suspended;
> > + bool boot_complete;
> > +
> > + /* Firmware version information */
> > + u16 version_major;
> > + u16 version_minor;
> > + u16 version_build;
> > + u16 version_rev;
> > +
> > + struct mutex access_mutex;
> > +
> > + struct completion command_done;
> > + struct mutex command_mutex;
> > + int command_waiting;
> > + int command_result;
> > +};
> > +
> > +static int zforce_command(struct zforce_ts *ts, u8 cmd)
> > +{
> > + struct i2c_client *client = ts->client;
> > + char buf[3];
> > + int ret;
> > +
> > + dev_dbg(&client->dev, "%s: 0x%x\n", __func__, cmd);
> > +
> > + buf[0] = FRAME_START;
> > + buf[1] = 1; /* data size, command only */
> > + buf[2] = cmd;
> > +
> > + mutex_lock(&ts->access_mutex);
> > + ret = i2c_master_send(client, &buf[0], ARRAY_SIZE(buf));
> > + mutex_unlock(&ts->access_mutex);
>
> I am unsure why you need this lock. Doesn't i2c core already ensure
> necessary locking?
The access_lock is really only there to ensure the multi-reads stay together
and do not get split up. For individual reads the i2c core of course does all
the necessary things.
> Also, it does not look like zforec_command() will reace with your
> interrupt handler that does multi-reads...
I'm unsure :-) ... what about the following scenario:
- touch -> interrupt
- isr reads first packet
- user closes device -> stop command sent
- isr reads payload
I'll fix the rest of the issues in the next version too.
Thanks for the review
Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/