Re: [PATCH] Input: add driver for Neonode zForce based touchscreens

["Heiko Stübner" <heiko@sntech.de>]

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@sntech.de>
> > ---
> > 
> >  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@sntech.de>
> > + *
> > + * based in parts on Nook zforce driver
> > + *
> > + * Copyright (C) 2010 Barnes & Noble, Inc.
> > + * Author: Pieter Truter<ptruter@intrinsyc.com>
> > + *
> > + * 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