Re: [PATCH v1] Add support for the ektf2127 touchscreencontroller

[Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Siebren,

On Mon, Jul 04, 2016 at 11:29:05AM +0200, Siebren Vroegindeweij wrote:
> From: Siebren Vroegindeweij <siebren.vroegindeweij-PkbjNfxxIARBDgjK7y7TUQ@public.gmane.org>
> 
> The ELAN eKTF2127 driver is written for the ELAN series of
> touchscreencontrollers. This version is especially written for the
> eKTF2127 controller. The driver communicates to the controller over i2c.
> The additional screen specifications can be read from the devicetree file.
> The driver has also the ability to read the screen height and width from
> the controller. When the screen is pressed, a interrupt is generated.
> The interrupt wil be handled by a function that request a data string from
> the controller. This string is then modified so that the right number of touches
> and their x and y coordinates are available and given to userspace through
> the input subsystem.
> 
> Signed-off-by: Michel Verlaan <michel.verl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Siebren Vroegindeweij <siebren.vroegindeweij-PkbjNfxxIARBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../bindings/input/touchscreen/ektf2127.txt        |  40 +++
>  drivers/input/touchscreen/Kconfig                  |  11 +
>  drivers/input/touchscreen/Makefile                 |   1 +
>  drivers/input/touchscreen/ektf2127.c               | 351 +++++++++++++++++++++
>  4 files changed, 403 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ektf2127.txt
>  create mode 100644 drivers/input/touchscreen/ektf2127.c
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ektf2127.txt b/Documentation/devicetree/bindings/input/touchscreen/ektf2127.txt
> new file mode 100644
> index 0000000..a774336
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ektf2127.txt
> @@ -0,0 +1,40 @@
> +* Elan eKTF2127 I2C touchscreen controller
> +
> +Required properties:
> + - compatible		  : "elan,ektf2127"
> + - reg			  : I2C slave address of the chip (0x40)
> + - interrupt-parent	  : a phandle pointing to the interrupt controller
> +			    serving the interrupt for this chip
> + - interrupts		  : interrupt specification for the eKTF2127 interrupt
> + - wake-gpios		  : GPIO specification for the WAKE input
> +
> +Optional properties:
> + - touchscreen-size-x	  : horizontal resolution of touchscreen (in pixels)
> + - touchscreen-size-y	  : vertical resolution of touchscreen (in pixels)
> + - touchscreen-fuzz-x	  : horizontal noise value of the absolute input
> +			    device (in pixels)
> + - touchscreen-fuzz-y	  : vertical noise value of the absolute input
> +			    device (in pixels)
> + - touchscreen-inverted-x : X axis is inverted (boolean)
> + - touchscreen-inverted-y : Y axis is inverted (boolean)
> + - touchscreen-swapped-x-y	  : X and Y axis are swapped (boolean)
> +			    Swapping is done after inverting the axis
> +
> +Example:
> +
> +i2c@00000000 {
> +
> +	ektf2127: touchscreen@40 {
> +		compatible = "elan,ektf2127";
> +		reg = <0x40>;
> +		interrupt-parent = <&pio>;
> +		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>
> +		/* pinctrl-names = "default";
> +		pinctrl-0 = <&ts_wake_pin_p66>; */
> +		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
> +		touchscreen-inverted-x;
> +		touchscreen-swapped-x-y;
> +	};
> +
> +};
> +
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 66c6264..28fd425 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1133,4 +1133,15 @@ config TOUCHSCREEN_ROHM_BU21023
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bu21023_ts.
>  
> +config TOUCHSCREEN_EKTF2127
> +	tristate "ektf2127 I2C touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a touchscreen using ektf2127.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ektf2127.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 968ff12..bc0db43 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -93,3 +93,4 @@ obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)	+= rohm_bu21023.o
> +obj-$(CONFIG_TOUCHSCREEN_EKTF2127)	+= ektf2127.o
> diff --git a/drivers/input/touchscreen/ektf2127.c b/drivers/input/touchscreen/ektf2127.c
> new file mode 100644
> index 0000000..e57fbbd
> --- /dev/null
> +++ b/drivers/input/touchscreen/ektf2127.c
> @@ -0,0 +1,351 @@
> +/*
> + * Driver for ELAN eKTF2127 i2c touchscreen controller
> + *
> + * For this driver the layout of the Chipone icn8318 i2c
> + * touchscreencontroller is used.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.

As I mentioned the kernel is GPL v2, so please change it from 3 to 2 for
me to be able to accept the driver.

> + *
> + * Author:
> + * Michel Verlaan <michel.verl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * Siebren Vroegindeweij <siebren.vroegindeweij-PkbjNfxxIARBDgjK7y7TUQ@public.gmane.org>
> + *
> + * Original chipone_icn8318 driver:
> + * Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>
> +
> +#define EKTF2127_REG_POWER		4
> +#define EKTF2127_REG_TOUCHDATA		16
> +
> +#define EKTF2127_POWER_ACTIVE		0
> +#define EKTF2127_POWER_MONITOR		1
> +#define EKTF2127_POWER_HIBERNATE	2
> +
> +#define EKTF2127_MAX_TOUCHES		5
> +
> +/* The difference between 2 and 3 is unclear */
> +#define EKTF2127_EVENT_NO_DATA	1 /* No finger seen yet since wakeup */
> +#define EKTF2127_EVENT_UPDATE1	2 /* New or updated coordinates */
> +#define EKTF2127_EVENT_UPDATE2	3 /* New or updated coordinates */
> +#define EKTF2127_EVENT_END	4 /* Finger lifted */
> +
> +struct ektf2127_data {
> +	struct i2c_client *client;
> +	struct input_dev *input;
> +	struct gpio_desc *power_gpios;
> +	u32 max_x;
> +	u32 max_y;
> +	bool invert_x;
> +	bool invert_y;
> +	bool swap_x_y;
> +};
> +
> +static void retrieve_coordinates(struct input_mt_pos *touches,
> +				int touch_count, char *buf)
> +{
> +	int index = 0;
> +	int i = 0;
> +
> +	for (i = 0; i < touch_count; i++) {
> +		index = 2 + i * 3;
> +
> +		touches[i].x = (buf[index] & 0x0f);
> +		touches[i].x <<= 8;
> +		touches[i].x |= buf[index + 2];
> +
> +		touches[i].y = (buf[index] & 0xf0);
> +		touches[i].y <<= 4;
> +		touches[i].y |= buf[index + 1];
> +	}
> +}
> +
> +static irqreturn_t ektf2127_irq(int irq, void *dev_id)
> +{
> +	struct ektf2127_data *data = dev_id;
> +	struct device *dev = &data->client->dev;
> +	struct input_mt_pos touches[EKTF2127_MAX_TOUCHES];
> +	int touch_count;
> +	int slots[EKTF2127_MAX_TOUCHES];
> +	char buff[25];
> +	int i, ret;
> +
> +	ret = i2c_master_recv(data->client, buff, 25);
> +	if (ret != 25) {
> +		dev_err(dev, "Error reading touch data");
> +		return IRQ_HANDLED;
> +	}
> +
> +	touch_count = buff[1] & 0x07;
> +
> +	if (touch_count > EKTF2127_MAX_TOUCHES) {
> +		dev_err(dev, "Too many touches %d > %d\n",
> +			touch_count, EKTF2127_MAX_TOUCHES);
> +		touch_count = EKTF2127_MAX_TOUCHES;
> +	}
> +
> +	retrieve_coordinates(touches, touch_count, buff);
> +
> +	for (i = 0; i < touch_count; i++) {
> +
> +		input_mt_assign_slots(data->input, slots, touches,
> +			touch_count, 0);

You only want to assign the slots once, not repeat it every time you
report a slot.

> +
> +		input_mt_slot(data->input, slots[i]);
> +		input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> +
> +		input_event(data->input, EV_ABS, ABS_MT_POSITION_X,
> +			touches[i].x);
> +		input_event(data->input, EV_ABS, ABS_MT_POSITION_Y,
> +			touches[i].y);
> +	}
> +
> +	input_mt_sync_frame(data->input);
> +	input_sync(data->input);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ektf2127_start(struct input_dev *dev)
> +{
> +	struct ektf2127_data *data = input_get_drvdata(dev);
> +
> +	enable_irq(data->client->irq);
> +	gpiod_set_value_cansleep(data->power_gpios, 1);
> +
> +	return 0;
> +}
> +
> +static void ektf2127_stop(struct input_dev *dev)
> +{
> +	struct ektf2127_data *data = input_get_drvdata(dev);
> +
> +	disable_irq(data->client->irq);
> +	gpiod_set_value_cansleep(data->power_gpios, 0);
> +}
> +
> +static int ektf2127_suspend(struct device *dev)
> +{
> +	struct ektf2127_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	mutex_lock(&data->input->mutex);
> +	if (data->input->users)
> +		ektf2127_stop(data->input);
> +	mutex_unlock(&data->input->mutex);
> +
> +	return 0;
> +}
> +
> +static int ektf2127_resume(struct device *dev)
> +{
> +	struct ektf2127_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	mutex_lock(&data->input->mutex);
> +	if (data->input->users)
> +		ektf2127_start(data->input);
> +	mutex_unlock(&data->input->mutex);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ektf2127_pm_ops, ektf2127_suspend,
> +			ektf2127_resume);
> +
> +static int ektf2127_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct device_node *np = dev->of_node;
> +	struct ektf2127_data *data;
> +	struct input_dev *input;
> +
> +	u32 fuzz_x = 0, fuzz_y = 0;
> +	char buff[25];
> +	int error, ret = 0;
> +
> +	if (!client->irq) {
> +		dev_err(dev, "Error no irq specified\n");
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->power_gpios = devm_gpiod_get(dev, "power", GPIOD_OUT_HIGH);
> +	msleep(20);

Why do you need sleep here, before checking for error?

> +	if (IS_ERR(data->power_gpios)) {
> +		error = PTR_ERR(data->power_gpios);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(dev, "Error getting power gpio: %d\n", error);
> +		return error;
> +	}
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input)
> +		return -ENOMEM;
> +
> +	input->name = client->name;
> +	input->id.bustype = BUS_I2C;
> +	input->open = ektf2127_start;
> +	input->close = ektf2127_stop;
> +	input->dev.parent = dev;

Not needed if you allocated with devm_input_allocate_device().

> +
> +	data->client = client;
> +
> +	/* read hello */
> +	i2c_master_recv(data->client, buff, 4);
> +
> +	/* Read resolution from chip */
> +
> +	/* Request height */
> +	buff[0] = 0x53; /* REQUEST */
> +	buff[1] = 0x63; /* HEIGHT */
> +	buff[2] = 0x00;
> +	buff[3] = 0x00;
> +	ret = i2c_master_send(data->client, buff, 4);
> +	if (ret != 4) {
> +		dev_err(dev, "Error requesting height");
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	msleep(20);
> +
> +	/* Read response */
> +	ret = i2c_master_recv(data->client, buff, 4);
> +	if (ret != 4) {
> +		dev_err(dev, "Error receiving height");
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +
> +	if ((buff[0] == 0x52) && (buff[1] == 0x63)) {
> +		data->max_y = ((buff[3] & 0xf0) << 4) | buff[2];
> +	} else {
> +		dev_err(dev, "Error receiving height data from"
> +			" wrong register");
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	/* Request width */
> +	buff[0] = 0x53; /* REQUEST */
> +	buff[1] = 0x60; /* WIDTH */
> +	buff[2] = 0x00;
> +	buff[3] = 0x00;
> +	ret = i2c_master_send(data->client, buff, 4);
> +	if (ret != 4) {
> +		dev_err(dev, "Error requesting width");
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +	msleep(20);
> +
> +	/* Read response */
> +	ret = i2c_master_recv(data->client, buff, 4);
> +	if (ret != 4) {
> +		dev_err(dev, "Error receiving width");
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +
> +	if ((buff[0] == 0x52) && (buff[1] == 0x60)) {
> +		data->max_x = (((buff[3] & 0xf0) << 4) | buff[2]);
> +	} else {
> +		dev_err(dev, "Error receiving width data from"
> +			" wrong register");
> +		return ret < 0 ? ret : -EIO;
> +	}
> +
> +
> +	/* Touchscreen resolution can be overruled by devicetree*/
> +	of_property_read_u32(np, "touchscreen-size-x", &data->max_x);
> +	of_property_read_u32(np, "touchscreen-size-y", &data->max_y);
> +
> +	/* Optional */
> +	of_property_read_u32(np, "touchscreen-fuzz-x", &fuzz_x);
> +	of_property_read_u32(np, "touchscreen-fuzz-y", &fuzz_y);
> +	data->invert_x = of_property_read_bool(np, "touchscreen-inverted-x");
> +	data->invert_y = of_property_read_bool(np, "touchscreen-inverted-y");
> +	data->swap_x_y = of_property_read_bool(np, "touchscreen-swapped-x-y");

Please use touchscreen_parse_properties() with struct
touchscreen_properties *prop argument and then use
touchscreen_set_mt_pos() to aply the needed conversions (this is a  new
API) instead of doing it yourself by hand.



> +
> +	if (!data->swap_x_y) {
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0,
> +				     data->max_x, fuzz_x, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
> +				     data->max_y, fuzz_y, 0);
> +	} else {
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0,
> +				     data->max_y, fuzz_y, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0,
> +				     data->max_x, fuzz_x, 0);
> +	}
> +
> +	error = input_mt_init_slots(input, EKTF2127_MAX_TOUCHES,
> +				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED
> +				    | INPUT_MT_TRACK);
> +	if (error)
> +		return error;
> +
> +	data->input = input;
> +	input_set_drvdata(input, data);
> +
> +	error = devm_request_threaded_irq(dev, client->irq, NULL, ektf2127_irq,
> +					  IRQF_ONESHOT, client->name, data);
> +	if (error) {
> +		dev_err(dev, "Error requesting irq: %d\n", error);
> +		return error;
> +	}
> +
> +	/* Stop device till opened */
> +	ektf2127_stop(data->input);
> +
> +	error = input_register_device(input);
> +	if (error)
> +		return error;
> +
> +	i2c_set_clientdata(client, data);
> +
> +	return 0;
> +}
> +

#ifdef CONFIG_OF
> +static const struct of_device_id ektf2127_of_match[] = {
> +	{ .compatible = "elan,ektf2127" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ektf2127_of_match);
#endif

> +
> +/* This is useless for OF-enabled devices,
> + * but it is needed by I2C subsystem
> + */
> +static const struct i2c_device_id ektf2127_i2c_id[] = {
> +	{ },

I think you want to have "ektf2127" here.

> +};
> +MODULE_DEVICE_TABLE(i2c, ektf2127_i2c_id);
> +
> +static struct i2c_driver ektf2127_driver = {
> +	.driver = {
> +		.name	= "elan_ektf2127",
> +		.pm	= &ektf2127_pm_ops,
> +		.of_match_table = ektf2127_of_match,

of_match_ptr(ektf2127_of_match)

> +	},
> +	.probe = ektf2127_probe,
> +	.id_table = ektf2127_i2c_id,
> +};
> +
> +module_i2c_driver(ektf2127_driver);
> +
> +MODULE_DESCRIPTION("ELAN eKTF2127 I2C Touchscreen Driver");
> +MODULE_AUTHOR("Michel Verlaan");
> +MODULE_AUTHOR("Siebren Vroegindeweij");
> +MODULE_LICENSE("GPL");

Thanks.

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