Message-ID: <20180509224152.GB91762@dtor-ws>
Date:   Wed, 9 May 2018 15:41:52 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Philipp Puschmann <pp@...ix.com>
Cc:     robh+dt@...nel.org, mark.rutland@....com, rydberg@...math.org,
        andi@...zian.org, linux-input@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: ili251x - add support for Ilitek ILI251x
 touchscreens

Hi Philipp,

On Mon, May 07, 2018 at 03:18:23PM +0200, Philipp Puschmann wrote:
> The driver supports at least the ili2511 chipset but may support other
> Ilitek chipsets using Ilitek i2c protocol v3.x.
> 
> The tested ili2511-based touchscreen delivers garbage for more than 6
> fingers while it should support up to 10 fingers. The reason is still
> unclear and this remains a FIXME in the driver for now.
> 
> The usage of pressure is optional. Touchscreens may deliver constant
> and so useless pressure data.

Is it dependent on model or what? I would much rather we did not have DT
property for this.

> 
> Signed-off-by: Philipp Puschmann <pp@...ix.com>
> ---
>  .../bindings/input/touchscreen/ili251x.txt    |  35 ++
>  drivers/input/touchscreen/Kconfig             |  12 +
>  drivers/input/touchscreen/Makefile            |   1 +
>  drivers/input/touchscreen/ili251x.c           | 350 ++++++++++++++++++
>  4 files changed, 398 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ili251x.txt
>  create mode 100644 drivers/input/touchscreen/ili251x.c
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ili251x.txt b/Documentation/devicetree/bindings/input/touchscreen/ili251x.txt
> new file mode 100644
> index 000000000000..f21ad93d3bdd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ili251x.txt
> @@ -0,0 +1,35 @@
> +Ilitek ili251x touchscreen driver
> +
> +This driver uses protocol version 3 and should be compatible with other
> +Ilitek touch controllers that use protocol 3.x
> +
> +Required properties:
> + - compatible:  "ili251x"
> + - reg:         I2C slave address of the chip (0x41)
> + - interrupt-parent: a phandle pointing to the interrupt controller
> +                     serving the interrupt for this chip
> + - interrupts:       interrupt specification for the touchdetect
> +                     interrupt
> +
> +Optional properties:
> + - reset-gpios: GPIO specification for the RESET input
> +
> + - pinctrl-names: should be "default"
> + - pinctrl-0:   a phandle pointing to the pin settings for the
> +                control gpios
> + - max-fingers: the maximum number of fingers to handle
> + - pressure: support pressure data
> + - generic options	   : See touchscreen.txt
> +
> +Example:
> +
> +	ili251x@41 {
> +		compatible = "ili251x";
> +		reg = <0x41>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_touchpanel>;
> +		interrupt-parent = <&gpio5>;
> +		interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> +		reset-gpios = <&gpio5 18 GPIO_ACTIVE_HIGH>;
> +		max-fingers = <6>;
> +	};
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 4f15496fec8b..569528834d48 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -380,6 +380,18 @@ config TOUCHSCREEN_ILI210X
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ili210x.
>  
> +config TOUCHSCREEN_ILI251X
> +	tristate "Ilitek ILI251X based touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a ILI251X based touchscreen
> +	  controller. This driver supports ILI2511.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ili251x.
> +
>  config TOUCHSCREEN_IPROC
>  	tristate "IPROC touch panel driver support"
>  	depends on ARCH_BCM_IPROC || COMPILE_TEST
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index dddae7973436..e795b62e5f64 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_GOODIX)	+= goodix.o
>  obj-$(CONFIG_TOUCHSCREEN_HIDEEP)	+= hideep.o
>  obj-$(CONFIG_TOUCHSCREEN_ILI210X)	+= ili210x.o
> +obj-$(CONFIG_TOUCHSCREEN_ILI251X)	+= ili251x.o
>  obj-$(CONFIG_TOUCHSCREEN_IMX6UL_TSC)	+= imx6ul_tsc.o
>  obj-$(CONFIG_TOUCHSCREEN_INEXIO)	+= inexio.o
>  obj-$(CONFIG_TOUCHSCREEN_IPROC)		+= bcm_iproc_tsc.o
> diff --git a/drivers/input/touchscreen/ili251x.c b/drivers/input/touchscreen/ili251x.c
> new file mode 100644
> index 000000000000..203367b59902
> --- /dev/null
> +++ b/drivers/input/touchscreen/ili251x.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, emlix GmbH. All rights reserved. */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/irq.h>
> +
> +#define MAX_FINGERS		10
> +#define REG_TOUCHDATA		0x10
> +#define TOUCHDATA_FINGERS	6
> +#define REG_TOUCHDATA2		0x14
> +#define TOUCHDATA2_FINGERS	4
> +#define REG_PANEL_INFO		0x20
> +#define REG_FIRMWARE_VERSION	0x40
> +#define REG_PROTO_VERSION	0x42
> +#define REG_CALIBRATE		0xcc
> +
> +struct finger {
> +	u8 x_high:6;
> +	u8 dummy:1;
> +	u8 status:1;

This does not work on BE as the order of bits will be reversed. Please
use masks, shifts, etc instead.

> +	u8 x_low;
> +	u8 y_high;
> +	u8 y_low;
> +	u8 pressure;
> +} __packed;
> +
> +struct touchdata {
> +	u8 status;
> +	struct finger fingers[MAX_FINGERS];
> +} __packed;
> +
> +struct panel_info {
> +	u8 x_low;
> +	u8 x_high;

__le16?

> +	u8 y_low;
> +	u8 y_high;

__le16

> +	u8 xchannel_num;
> +	u8 ychannel_num;
> +	u8 max_fingers;
> +} __packed;

This does not need to be packed.

> +
> +struct firmware_version {
> +	u8 id;
> +	u8 major;
> +	u8 minor;
> +} __packed;
> +
> +struct protocol_version {
> +	u8 major;
> +	u8 minor;
> +} __packed;
> +
> +struct ili251x_data {
> +	struct i2c_client *client;
> +	struct input_dev *input;
> +	unsigned int max_fingers;
> +	bool use_pressure;
> +	struct gpio_desc *reset_gpio;
> +};
> +
> +static int ili251x_read_reg(struct i2c_client *client, u8 reg, void *buf,
> +			    size_t len)
> +{
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr	= client->addr,
> +			.flags	= 0,
> +			.len	= 1,
> +			.buf	= &reg,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= len,
> +			.buf	= buf,
> +		}
> +	};
> +
> +	if (i2c_transfer(client->adapter, msg, 2) != 2) {
> +		dev_err(&client->dev, "i2c transfer failed\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ili251x_report_events(struct ili251x_data *data,
> +				  const struct touchdata *touchdata)
> +{
> +	struct input_dev *input = data->input;
> +	unsigned int i;
> +	bool touch;
> +	unsigned int x, y;
> +	const struct finger *finger;
> +	unsigned int reported_fingers = 0;
> +
> +	/* the touch chip does not count the real fingers but switches between
> +	 * 0, 6 and 10 reported fingers *
> +	 *
> +	 * FIXME: With a tested ili2511 we received only garbage for fingers
> +	 *        6-9. As workaround we add a device tree option to limit the
> +	 *        handled number of fingers
> +	 */
> +	if (touchdata->status == 1)
> +		reported_fingers = 6;
> +	else if (touchdata->status == 2)
> +		reported_fingers = 10;
> +
> +	for (i = 0; i < reported_fingers && i < data->max_fingers; i++) {
> +		input_mt_slot(input, i);
> +
> +		finger = &touchdata->fingers[i];
> +
> +		touch = finger->status;
> +		input_mt_report_slot_state(input, MT_TOOL_FINGER, touch);
> +		x = finger->x_low | (finger->x_high << 8);
> +		y = finger->y_low | (finger->y_high << 8);
> +
> +		if (touch) {
> +			input_report_abs(input, ABS_MT_POSITION_X, x);
> +			input_report_abs(input, ABS_MT_POSITION_Y, y);
> +			if (data->use_pressure)
> +				input_report_abs(input, ABS_MT_PRESSURE,
> +						 finger->pressure);
> +
> +		}
> +	}
> +
> +	input_mt_report_pointer_emulation(input, false);
> +	input_sync(input);
> +}
> +
> +static irqreturn_t ili251x_irq(int irq, void *irq_data)
> +{
> +	struct ili251x_data *data = irq_data;
> +	struct i2c_client *client = data->client;
> +	struct touchdata touchdata;
> +	int error;
> +
> +	error = ili251x_read_reg(client, REG_TOUCHDATA,
> +				 &touchdata,
> +				 sizeof(touchdata) -
> +					sizeof(struct finger)*TOUCHDATA2_FINGERS);
> +
> +	if (!error && touchdata.status == 2 && data->max_fingers > 6)
> +		error = ili251x_read_reg(client, REG_TOUCHDATA2,
> +					 &touchdata.fingers[TOUCHDATA_FINGERS],
> +					 sizeof(struct finger)*TOUCHDATA2_FINGERS);
> +
> +	if (!error)
> +		ili251x_report_events(data, &touchdata);
> +	else
> +		dev_err(&client->dev,
> +			"Unable to get touchdata, err = %d\n", error);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void ili251x_reset(struct ili251x_data *data)
> +{
> +	if (data->reset_gpio) {
> +		gpiod_set_value(data->reset_gpio, 1);
> +		usleep_range(50, 100);
> +		gpiod_set_value(data->reset_gpio, 0);
> +		msleep(100);
> +	}
> +}
> +
> +static int ili251x_i2c_probe(struct i2c_client *client,
> +				       const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct ili251x_data *data;
> +	struct input_dev *input;
> +	struct panel_info panel;
> +	struct device_node *np = dev->of_node;
> +	struct firmware_version firmware;
> +	struct protocol_version protocol;
> +	int xmax, ymax;
> +	int error;
> +
> +	dev_dbg(dev, "Probing for ili251x I2C Touschreen driver");
> +
> +	if (client->irq <= 0) {
> +		dev_err(dev, "No IRQ!\n");
> +		return -EINVAL;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	input = devm_input_allocate_device(dev);
> +	if (!data || !input)

Please use separate tests here.

> +		return -ENOMEM;
> +
> +	data->client = client;
> +	data->input = input;
> +	data->use_pressure = false;
> +
> +	data->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio)) {
> +		error = PTR_ERR(data->reset_gpio);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(dev,
> +				"Failed to get reset GPIO: %d\n", error);
> +		return error;
> +	}
> +
> +	ili251x_reset(data);
> +
> +	error = ili251x_read_reg(client, REG_FIRMWARE_VERSION,
> +				 &firmware, sizeof(firmware));
> +	if (error) {
> +		dev_err(dev, "Failed to get firmware version, err: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	error = ili251x_read_reg(client, REG_PROTO_VERSION,
> +				 &protocol, sizeof(protocol));
> +	if (error) {
> +		dev_err(dev, "Failed to get protocol version, err: %d\n",
> +			error);
> +		return error;
> +	}
> +	if (protocol.major != 3) {
> +		dev_err(dev, "This driver expects protocol version 3.x, Chip uses: %d\n",
> +				protocol.major);
> +		return -EINVAL;
> +	}
> +
> +	error = ili251x_read_reg(client, REG_PANEL_INFO, &panel, sizeof(panel));
> +	if (error) {
> +		dev_err(dev, "Failed to get panel information, err: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	data->max_fingers = panel.max_fingers;
> +	if (np) {
> +		int max_fingers;
> +
> +		error = of_property_read_u32(np, "max-fingers", &max_fingers);
> +		if (!error && max_fingers < data->max_fingers)
> +			data->max_fingers = max_fingers;
> +
> +		if (of_property_read_bool(np, "pressure"))
> +			data->use_pressure = true;
> +	}
> +
> +	xmax = panel.x_low | (panel.x_high << 8);
> +	ymax = panel.y_low | (panel.y_high << 8);
> +
> +	/* Setup input device */
> +	input->name = "ili251x Touchscreen";
> +	input->id.bustype = BUS_I2C;
> +	input->dev.parent = dev;

This is done by devm_input_allocate_device(), please drop.

> +
> +	__set_bit(EV_SYN, input->evbit);
> +	__set_bit(EV_KEY, input->evbit);
> +	__set_bit(EV_ABS, input->evbit);
> +	__set_bit(BTN_TOUCH, input->keybit);
> +
> +	/* Single touch */
> +	input_set_abs_params(input, ABS_X, 0, xmax, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, ymax, 0, 0);
> +
> +	/* Multi touch */
> +	input_mt_init_slots(input, data->max_fingers, 0);

Error handling please.

The touchscreens should be marked as INPUT_MT_DIRECT.

> +	input_set_abs_params(input, ABS_MT_POSITION_X, 0, xmax, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, ymax, 0, 0);
> +	if (data->use_pressure)
> +		input_set_abs_params(input, ABS_MT_PRESSURE, 0, U8_MAX, 0, 0);

You want to set up absolute axes before you call input_mt_init_slots()
so that single-touch emulation is set up properly.

> +
> +	i2c_set_clientdata(client, data);
> +
> +	error = devm_request_threaded_irq(dev, client->irq, NULL, ili251x_irq,
> +				 IRQF_ONESHOT, client->name, data);
> +
> +	if (error) {
> +		dev_err(dev, "Unable to request touchscreen IRQ, err: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	error = input_register_device(data->input);
> +	if (error) {
> +		dev_err(dev, "Cannot register input device, err: %d\n", error);
> +		return error;
> +	}
> +
> +	device_init_wakeup(dev, 1);
> +
> +	dev_info(dev,
> +		"ili251x initialized (IRQ: %d), firmware version %d.%d.%d fingers %d",
> +		client->irq, firmware.id, firmware.major, firmware.minor,
> +		data->max_fingers);

dve_dbg() if you really need this, but I would rather drop.

> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ili251x_i2c_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (device_may_wakeup(&client->dev))
> +		enable_irq_wake(client->irq);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ili251x_i2c_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (device_may_wakeup(&client->dev))
> +		disable_irq_wake(client->irq);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ili251x_i2c_pm,
> +			 ili251x_i2c_suspend, ili251x_i2c_resume);

All this boilerplate can be removed if you use dev_pm_set_wake_irq() in
probe().

> +
> +static const struct i2c_device_id ili251x_i2c_id[] = {
> +	{ "ili251x", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ili251x_i2c_id);
> +
> +static struct i2c_driver ili251x_ts_driver = {
> +	.driver = {
> +		.name = "ili251x_i2c",
> +		.pm = &ili251x_i2c_pm,
> +	},
> +	.id_table = ili251x_i2c_id,
> +	.probe = ili251x_i2c_probe,
> +};
> +
> +module_i2c_driver(ili251x_ts_driver);
> +
> +MODULE_AUTHOR("Philipp Puschmann <pp@...ix.com>");
> +MODULE_DESCRIPTION("ili251x I2C Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.17.0
> 

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists