[v6] input: touchscreen: Add generic driver for Silead touchscreens

Commit Message

Hans de Goede July 11, 2016, 3:51 p.m. UTC

From: Robert Dolca <[e-mail]>

This driver adds support for Silead touchscreens. It has been tested
with GSL1680 and GSL3680 touch panels.

It supports ACPI and device tree enumeration. Screen resolution,
the maximum number of fingers supported and firmware name are
configurable.

Signed-off-by: Robert Dolca <[e-mail]>
Signed-off-by: Daniel Jansen <[e-mail]>
Signed-off-by: Hans de Goede <[e-mail]>
---
Changes in v6:
-Default to 5 fingers if max-fingers is not specified
-Improve devicetree binding doc: improve wake-gpios description, use
 "See touchscreen.txt" where applicable
---
 .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
 drivers/input/touchscreen/Kconfig                  |  13 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
 4 files changed, 702 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
 create mode 100644 drivers/input/touchscreen/silead.c

Comments

Dmitry Torokhov July 12, 2016, 12:01 a.m. UTC | #1

Hi Hans,

On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
> From: Robert Dolca <[e-mail]>
> 
> This driver adds support for Silead touchscreens. It has been tested
> with GSL1680 and GSL3680 touch panels.
> 
> It supports ACPI and device tree enumeration. Screen resolution,
> the maximum number of fingers supported and firmware name are
> configurable.
> 
> Signed-off-by: Robert Dolca <[e-mail]>
> Signed-off-by: Daniel Jansen <[e-mail]>
> Signed-off-by: Hans de Goede <[e-mail]>
> ---
> Changes in v6:
> -Default to 5 fingers if max-fingers is not specified

From the conversations with Robert, IIRC, the number of touches is not
really property of a given controller, but rather firmware that is
currently loaded into it and theoretically can change. Given this fact,
can we simply expect up to 10 contacts and do not use the property at
all?

> -Improve devicetree binding doc: improve wake-gpios description, use
>  "See touchscreen.txt" where applicable
> ---
>  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
>  drivers/input/touchscreen/Kconfig                  |  13 +
>  drivers/input/touchscreen/Makefile                 |   1 +
>  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
>  4 files changed, 702 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>  create mode 100644 drivers/input/touchscreen/silead.c
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> new file mode 100644
> index 0000000..1e58d37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> @@ -0,0 +1,36 @@
> +* GSL 1680 touchscreen controller
> +
> +Required properties:
> +- compatible		  : "silead,gsl1680"
> +- 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 gsl1680 interrupt
> +- power-gpios		  : Specification for the pin connected to the gsl1680's
> +			    shutdown input. This needs to be driven high to take the
> +			    gsl1680 out of its low power state
> +- touchscreen-size-x	  : See touchscreen.txt
> +- touchscreen-size-y	  : See touchscreen.txt
> +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
> +
> +Optional properties:
> +- touchscreen-inverted-x  : See touchscreen.txt
> +- touchscreen-inverted-y  : See touchscreen.txt
> +- touchscreen-swapped-x-y : See touchscreen.txt
> +
> +Example:
> +
> +i2c@00000000 {
> +	gsl1680: touchscreen@40 {
> +		compatible = "silead,gsl1680";
> +		reg = <0x40>;
> +		interrupt-parent = <&pio>;
> +		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>;
> +		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
> +		touchscreen-size-x = <480>;
> +		touchscreen-size-y = <800>;
> +		touchscreen-max-fingers = <5>;
> +		touchscreen-inverted-x;
> +		touchscreen-swapped-x-y;
> +	};
> +};
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 8ecdc38..cdd7c4c 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1155,4 +1155,17 @@ config TOUCHSCREEN_ROHM_BU21023
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bu21023_ts.
>  
> +config TOUCHSCREEN_SILEAD
> +	tristate "Silead I2C touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have the Silead touchscreen connected to
> +	  your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called silead.
> +
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f42975e..8ffa0fd 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -95,3 +95,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_SILEAD)	+= silead.o
> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> new file mode 100644
> index 0000000..22d3546
> --- /dev/null
> +++ b/drivers/input/touchscreen/silead.c
> @@ -0,0 +1,652 @@
> +/* -------------------------------------------------------------------------
> + * Copyright (C) 2014-2015, Intel Corporation
> + *
> + * Derived from:
> + *  gslX68X.c
> + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> + *
> + *  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 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  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/i2c.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/pm.h>
> +#include <linux/irq.h>
> +
> +#define SILEAD_TS_NAME "silead_ts"
> +
> +#define SILEAD_REG_RESET	0xE0
> +#define SILEAD_REG_DATA		0x80
> +#define SILEAD_REG_TOUCH_NR	0x80
> +#define SILEAD_REG_POWER	0xBC
> +#define SILEAD_REG_CLOCK	0xE4
> +#define SILEAD_REG_STATUS	0xB0
> +#define SILEAD_REG_ID		0xFC
> +#define SILEAD_REG_MEM_CHECK	0xB0
> +
> +#define SILEAD_STATUS_OK	0x5A5A5A5A
> +#define SILEAD_TS_DATA_LEN	44
> +#define SILEAD_CLOCK		0x04
> +
> +#define SILEAD_CMD_RESET	0x88
> +#define SILEAD_CMD_START	0x00
> +
> +#define SILEAD_POINT_DATA_LEN	0x04
> +#define SILEAD_POINT_Y_OFF      0x00
> +#define SILEAD_POINT_Y_MSB_OFF	0x01
> +#define SILEAD_POINT_X_OFF	0x02
> +#define SILEAD_POINT_X_MSB_OFF	0x03
> +#define SILEAD_POINT_HSB_MASK	0x0F
> +#define SILEAD_TOUCH_ID_MASK	0xF0
> +
> +#define SILEAD_CMD_SLEEP_MIN	10000
> +#define SILEAD_CMD_SLEEP_MAX	20000
> +#define SILEAD_POWER_SLEEP	20
> +#define SILEAD_STARTUP_SLEEP	30
> +
> +#define SILEAD_MAX_FINGERS	10
> +#define SILEAD_MAX_X		4096
> +#define SILEAD_MAX_Y		4096
> +
> +enum silead_ts_power {
> +	SILEAD_POWER_ON  = 1,
> +	SILEAD_POWER_OFF = 0
> +};
> +
> +struct silead_ts_data {
> +	struct i2c_client *client;
> +	struct gpio_desc *gpio_power;
> +	struct input_dev *input;
> +	const char *custom_fw_name;
> +	char fw_name[I2C_NAME_SIZE];
> +	u32 x_max;
> +	u32 y_max;
> +	u32 max_fingers;
> +	bool x_invert;
> +	bool y_invert;
> +	bool xy_swap;
> +	u32 chip_id;
> +	struct input_mt_pos pos[SILEAD_MAX_FINGERS];
> +	int slots[SILEAD_MAX_FINGERS];
> +	int id[SILEAD_MAX_FINGERS];
> +};
> +
> +struct silead_fw_data {
> +	u32 offset;
> +	u32 val;
> +};
> +
> +static int silead_ts_request_input_dev(struct silead_ts_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	int error;
> +
> +	data->input = devm_input_allocate_device(dev);
> +	if (!data->input) {
> +		dev_err(dev,
> +			"Failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (!data->xy_swap) {
> +		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> +				     data->x_max, 0, 0);
> +		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> +				     data->y_max, 0, 0);
> +	} else {
> +		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> +				     data->y_max, 0, 0);
> +		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> +				     data->x_max, 0, 0);
> +	}
> +
> +	input_mt_init_slots(data->input, data->max_fingers,
> +			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
> +			    INPUT_MT_TRACK);
> +
> +	data->input->name = SILEAD_TS_NAME;
> +	data->input->phys = "input/ts";
> +	data->input->id.bustype = BUS_I2C;
> +
> +	error = input_register_device(data->input);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device: %d\n", error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> +				   u8 id)
> +{
> +	if (data->x_invert)
> +		x = data->x_max - x;
> +
> +	if (data->y_invert)
> +		y = data->y_max - y;
> +
> +	if (data->xy_swap)
> +		swap(x, y);
> +
> +	input_mt_slot(data->input, id);
> +	input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> +	input_report_abs(data->input, ABS_MT_POSITION_X, x);
> +	input_report_abs(data->input, ABS_MT_POSITION_Y, y);
> +}
> +
> +static void silead_ts_set_power(struct i2c_client *client,
> +				enum silead_ts_power state)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +
> +	if (data->gpio_power) {
> +		gpiod_set_value_cansleep(data->gpio_power, state);
> +		msleep(SILEAD_POWER_SLEEP);
> +	}
> +}
> +
> +static void silead_ts_read_data(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +	u8 buf[SILEAD_TS_DATA_LEN];
> +	int x, y, touch_nr, error, i, offset;
> +
> +	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> +					    SILEAD_TS_DATA_LEN, buf);
> +	if (error < 0) {
> +		dev_err(dev, "Data read error %d\n", error);
> +		return;
> +	}
> +
> +	touch_nr = buf[0];

What is touch NR? I.e. can we use it as slot ID instead of resorting to
in-kernel tracking?

> +
Extra blank line.

> +	if (touch_nr < 0)
> +		return;
> +
> +	if (touch_nr > data->max_fingers) {
> +		dev_warn(dev, "More touches reported then supported %d > %d\n",
> +			 touch_nr, data->max_fingers);
> +		touch_nr = data->max_fingers;
> +	}
> +
> +	dev_dbg(dev, "Touch number: %d\n", touch_nr);
> +
> +	for (i = 0; i < touch_nr; i++) {
> +		offset = (i + 1) * SILEAD_POINT_DATA_LEN;
> +
> +		/* Bits 4-7 are the touch id */
> +		data->id[i] = (buf[offset + SILEAD_POINT_X_MSB_OFF] &
> +			      SILEAD_TOUCH_ID_MASK) >> 4;
> +
> +		/* Bits 0-3 are MSB of X */
> +		buf[offset + SILEAD_POINT_X_MSB_OFF] =
> +					buf[offset + SILEAD_POINT_X_MSB_OFF] &
> +					SILEAD_POINT_HSB_MASK;
> +
> +		/* Bits 0-3 are MSB of Y */
> +		buf[offset + SILEAD_POINT_Y_MSB_OFF] =
> +					buf[offset + SILEAD_POINT_Y_MSB_OFF] &
> +					SILEAD_POINT_HSB_MASK;
> +
> +		y = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_Y_OFF));
> +		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> +
> +		data->pos[i].x = x;
> +		data->pos[i].y = y;
> +	}
> +
> +	input_mt_assign_slots(data->input, data->slots, data->pos, touch_nr, 0);
> +
> +	for (i = 0; i < touch_nr; i++) {
> +		silead_ts_report_touch(data, data->pos[i].x, data->pos[i].y,
> +				       data->slots[i]);
> +
> +		dev_dbg(dev, "x=%d y=%d hw_id=%d sw_id=%d\n", data->pos[i].x,
> +			data->pos[i].y, data->id[i], data->slots[i]);
> +	}
> +
> +	input_mt_sync_frame(data->input);
> +	input_sync(data->input);
> +}
> +
> +static int silead_ts_init(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	int error;
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> +					SILEAD_CMD_RESET);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> +					data->max_fingers);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
> +					  SILEAD_CLOCK);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> +					SILEAD_CMD_START);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	return 0;
> +
> +i2c_write_err:
> +	dev_err(&client->dev, "Registers clear error %d\n", error);
> +	return error;
> +}
> +
> +static int silead_ts_reset(struct i2c_client *client)
> +{
> +	int error;
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> +					SILEAD_CMD_RESET);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
> +					  SILEAD_CLOCK);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> +					SILEAD_CMD_START);
> +	if (error)
> +		goto i2c_write_err;
> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> +
> +	return 0;
> +
> +i2c_write_err:
> +	dev_err(&client->dev, "Chip reset error %d\n", error);
> +	return error;
> +}
> +
> +static int silead_ts_startup(struct i2c_client *client)
> +{
> +	int error;
> +
> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> +	if (error) {
> +		dev_err(&client->dev, "Startup error %d\n", error);
> +		return error;
> +	}
> +	msleep(SILEAD_STARTUP_SLEEP);
> +
> +	return 0;
> +}
> +
> +static int silead_ts_load_fw(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	unsigned int fw_size, i;
> +	const struct firmware *fw;
> +	struct silead_fw_data *fw_data;
> +	int error;
> +
> +	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> +
> +	if (data->custom_fw_name)
> +		error = request_firmware(&fw, data->custom_fw_name, dev);
> +	else
> +		error = request_firmware(&fw, data->fw_name, dev);
> +
> +	if (error) {
> +		dev_err(dev, "Firmware request error %d\n", error);
> +		return error;
> +	}
> +
> +	fw_size = fw->size / sizeof(*fw_data);
> +	fw_data = (struct silead_fw_data *)fw->data;
> +
> +	for (i = 0; i < fw_size; i++) {
> +		error = i2c_smbus_write_i2c_block_data(client,
> +						       fw_data[i].offset,
> +						       4,
> +						       (u8 *)&fw_data[i].val);
> +		if (error) {
> +			dev_err(dev, "Firmware load error %d\n", error);
> +			goto release_fw_err;
> +		}
> +	}
> +
> +	release_firmware(fw);
> +	return 0;
> +
> +release_fw_err:
> +	release_firmware(fw);
> +	return error;
> +}
> +
> +static u32 silead_ts_get_status(struct i2c_client *client)
> +{
> +	int error;
> +	u32 status;
> +
> +	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> +					    (u8 *)&status);
> +	if (error < 0) {
> +		dev_err(&client->dev, "Status read error %d\n", error);
> +		return error;
> +	}
> +
> +	return le32_to_cpu(status);
> +}
> +
> +static int silead_ts_get_id(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	int error;
> +
> +	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> +					    (u8 *)&data->chip_id);
> +
> +	data->chip_id = le32_to_cpu(data->chip_id);
> +
> +	if (error < 0) {
> +		dev_err(&client->dev, "Chip ID read error %d\n", error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int silead_ts_setup(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +	int error;
> +	u32 status;
> +
> +	silead_ts_set_power(client, SILEAD_POWER_OFF);
> +	silead_ts_set_power(client, SILEAD_POWER_ON);
> +
> +	error = silead_ts_get_id(client);
> +	if (error)
> +		return error;
> +	dev_info(dev, "Silead chip ID: 0x%8X", data->chip_id);
> +
> +	error = silead_ts_init(client);
> +	if (error)
> +		return error;
> +
> +	error = silead_ts_reset(client);
> +	if (error)
> +		return error;
> +
> +	error = silead_ts_load_fw(client);
> +	if (error)
> +		return error;
> +
> +	error = silead_ts_startup(client);
> +	if (error)
> +		return error;
> +
> +	status = silead_ts_get_status(client);
> +	if (status != SILEAD_STATUS_OK) {
> +		dev_err(dev, "Initialization error, status: 0x%X\n", status);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
> +{
> +	struct silead_ts_data *data = id;
> +	struct i2c_client *client = data->client;
> +
> +	silead_ts_read_data(client);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int silead_ts_read_props(struct i2c_client *client)
> +{
> +	struct silead_ts_data *data = i2c_get_clientdata(client);
> +	struct device *dev = &client->dev;
> +	int error;
> +
> +	error = device_property_read_u32(dev, "touchscreen-size-x",
> +					&data->x_max);
> +	if (error) {
> +		dev_dbg(dev, "Resolution X read error %d\n", error);
> +		data->x_max = SILEAD_MAX_X;
> +	}
> +	data->x_max--; /* Property contains size not max */
> +
> +	error = device_property_read_u32(dev, "touchscreen-size-y",
> +					&data->y_max);
> +	if (error) {
> +		dev_dbg(dev, "Resolution Y read error %d\n", error);
> +		data->y_max = SILEAD_MAX_Y;
> +	}
> +	data->y_max--; /* Property contains size not max */
> +
> +	error = device_property_read_u32(dev, "touchscreen-max-fingers",
> +					 &data->max_fingers);
> +	if (error) {
> +		dev_dbg(dev, "Max fingers read error %d\n", error);
> +		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
> +	}
> +
> +	error = device_property_read_string(dev, "touchscreen-fw-name",
> +					  &data->custom_fw_name);
> +	if (error)
> +		dev_dbg(dev, "Firmware file name read error. Using default.");

Why do we need separate variable for this?

> +
> +	data->x_invert = device_property_read_bool(dev,
> +						"touchscreen-inverted-x");
> +	data->y_invert = device_property_read_bool(dev,
> +						"touchscreen-inverted-y");
> +	data->xy_swap = device_property_read_bool(dev,
> +						"touchscreen-swapped-x-y");
> +
> +	dev_dbg(dev, "x_max = %d, y_max = %d, max_fingers = %d, x_invert = %d, y_invert = %d, xy_swap = %d",
> +		data->x_max, data->y_max, data->max_fingers, data->x_invert,
> +		data->y_invert, data->xy_swap);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_ACPI
> +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> +					 const struct i2c_device_id *id)
> +{
> +	const struct acpi_device_id *acpi_id;
> +	struct device *dev = &data->client->dev;
> +	int i;
> +
> +	if (ACPI_HANDLE(dev)) {
> +		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> +		if (!acpi_id)
> +			return -ENODEV;
> +
> +		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
> +			acpi_id->id);
> +
> +		for (i = 0; i < strlen(data->fw_name); i++)
> +			data->fw_name[i] = tolower(data->fw_name[i]);
> +	} else {
> +		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
> +			id->name);
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> +					 const struct i2c_device_id *id)
> +{
> +	snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw", id->name);
> +	return 0;
> +}
> +#endif
> +
> +static int silead_ts_probe(struct i2c_client *client,
> +			   const struct i2c_device_id *id)
> +{
> +	struct silead_ts_data *data;
> +	struct device *dev = &client->dev;
> +	int error;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> +				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> +		dev_err(dev, "I2C functionality check failed\n");
> +		return -ENXIO;
> +	}
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, data);
> +	data->client = client;
> +
> +	error = silead_ts_set_default_fw_name(data, id);
> +	if (error)
> +		return error;
> +
> +	/* We must have the IRQ provided by DT or ACPI subsytem */
> +	if (client->irq <= 0)
> +		return -ENODEV;
> +
> +	/* Power GPIO pin */
> +	data->gpio_power = devm_gpiod_get_index(dev, "power",
> +						0, GPIOD_OUT_LOW);

Why "get index" and not devm_gpiod_get_optional()?

> +	if (IS_ERR(data->gpio_power)) {
> +		dev_dbg(dev, "Shutdown GPIO request failed\n");
> +		data->gpio_power = NULL;

You want to abort on errors, especially if you get -EPROBE_DEFER.

> +	}
> +
> +	error = silead_ts_read_props(client);
> +	if (error)
> +		return error;
> +
> +	error = silead_ts_setup(client);
> +	if (error)
> +		return error;
> +
> +	error = silead_ts_request_input_dev(data);
> +	if (error)
> +		return error;
> +
> +	error = devm_request_threaded_irq(dev, client->irq, NULL,
> +					silead_ts_threaded_irq_handler,
> +					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,

Please just use IRQF_ONESHOT and rely on platform code (ACPI. DT, etc)
to specify interrupt trigger.

> +					client->name, data);
> +	if (error) {
> +		dev_err(dev, "IRQ request failed %d\n", error);
> +		return error;
> +	}
> +
> +	dev_dbg(dev, "Probing succeded\n");

You'd see if it failed... Just drop.

> +	return 0;
> +}
> +
> +static int __maybe_unused silead_ts_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	silead_ts_set_power(client, SILEAD_POWER_OFF);
> +	return 0;
> +}
> +
> +static int __maybe_unused silead_ts_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	int error, status;
> +
> +	silead_ts_set_power(client, SILEAD_POWER_ON);
> +
> +	error = silead_ts_reset(client);
> +	if (error)
> +		return error;
> +
> +	error = silead_ts_startup(client);
> +	if (error)
> +		return error;
> +
> +	status = silead_ts_get_status(client);
> +	if (status != SILEAD_STATUS_OK) {
> +		dev_err(dev, "Resume error, status: 0x%X\n", status);

"%#02X"

> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
> +
> +static const struct i2c_device_id silead_ts_id[] = {
> +	{ "gsl1680", 0 },
> +	{ "gsl1688", 0 },
> +	{ "gsl3670", 0 },
> +	{ "gsl3675", 0 },
> +	{ "gsl3692", 0 },
> +	{ "mssl1680", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id silead_ts_acpi_match[] = {
> +	{ "GSL1680", 0 },
> +	{ "GSL1688", 0 },
> +	{ "GSL3670", 0 },
> +	{ "GSL3675", 0 },
> +	{ "GSL3692", 0 },
> +	{ "MSSL1680", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
> +#endif
> +
> +static struct i2c_driver silead_ts_driver = {
> +	.probe = silead_ts_probe,
> +	.id_table = silead_ts_id,
> +	.driver = {
> +		.name = SILEAD_TS_NAME,
> +		.owner = THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> +		.pm = &silead_ts_pm,
> +	},
> +};
> +module_i2c_driver(silead_ts_driver);
> +
> +MODULE_AUTHOR("Robert Dolca <[e-mail]>");
> +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
> 

Thanks.

Hans de Goede July 12, 2016, 8:10 a.m. UTC | #2

Hi,

On 12-07-16 02:01, Dmitry Torokhov wrote:
> Hi Hans,
>
> On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
>> From: Robert Dolca <[e-mail]>
>>
>> This driver adds support for Silead touchscreens. It has been tested
>> with GSL1680 and GSL3680 touch panels.
>>
>> It supports ACPI and device tree enumeration. Screen resolution,
>> the maximum number of fingers supported and firmware name are
>> configurable.
>>
>> Signed-off-by: Robert Dolca <[e-mail]>
>> Signed-off-by: Daniel Jansen <[e-mail]>
>> Signed-off-by: Hans de Goede <[e-mail]>
>> ---
>> Changes in v6:
>> -Default to 5 fingers if max-fingers is not specified
>
> From the conversations with Robert, IIRC, the number of touches is not
> really property of a given controller, but rather firmware that is
> currently loaded into it and theoretically can change. Given this fact,
> can we simply expect up to 10 contacts and do not use the property at
> all?

We actually write the max-fingers value to a register in the
chip:

	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
					data->max_fingers);

So we need the property, because AFAIK we cannot just write 10 for
"firmware" which only supports 5.

Also note there really is not firmware, the "firmware" files contain
pin configuration data and lookup tables to go from analog pin
measurements to reported values. One can upgrade a touchscreen from
say 800x480 resolution to 1024x600 by using another "firmware" as long
as the 2 use the same pin config (same pins connected to the same
senselines of the touchscreen).



>
>> -Improve devicetree binding doc: improve wake-gpios description, use
>>  "See touchscreen.txt" where applicable
>> ---
>>  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
>>  drivers/input/touchscreen/Kconfig                  |  13 +
>>  drivers/input/touchscreen/Makefile                 |   1 +
>>  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
>>  4 files changed, 702 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>>  create mode 100644 drivers/input/touchscreen/silead.c
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> new file mode 100644
>> index 0000000..1e58d37
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> @@ -0,0 +1,36 @@
>> +* GSL 1680 touchscreen controller
>> +
>> +Required properties:
>> +- compatible		  : "silead,gsl1680"
>> +- 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 gsl1680 interrupt
>> +- power-gpios		  : Specification for the pin connected to the gsl1680's
>> +			    shutdown input. This needs to be driven high to take the
>> +			    gsl1680 out of its low power state
>> +- touchscreen-size-x	  : See touchscreen.txt
>> +- touchscreen-size-y	  : See touchscreen.txt
>> +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
>> +
>> +Optional properties:
>> +- touchscreen-inverted-x  : See touchscreen.txt
>> +- touchscreen-inverted-y  : See touchscreen.txt
>> +- touchscreen-swapped-x-y : See touchscreen.txt
>> +
>> +Example:
>> +
>> +i2c@00000000 {
>> +	gsl1680: touchscreen@40 {
>> +		compatible = "silead,gsl1680";
>> +		reg = <0x40>;
>> +		interrupt-parent = <&pio>;
>> +		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>;
>> +		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
>> +		touchscreen-size-x = <480>;
>> +		touchscreen-size-y = <800>;
>> +		touchscreen-max-fingers = <5>;
>> +		touchscreen-inverted-x;
>> +		touchscreen-swapped-x-y;
>> +	};
>> +};
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 8ecdc38..cdd7c4c 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -1155,4 +1155,17 @@ config TOUCHSCREEN_ROHM_BU21023
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called bu21023_ts.
>>
>> +config TOUCHSCREEN_SILEAD
>> +	tristate "Silead I2C touchscreen"
>> +	depends on I2C
>> +	help
>> +	  Say Y here if you have the Silead touchscreen connected to
>> +	  your system.
>> +
>> +	  If unsure, say N.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called silead.
>> +
>> +
>>  endif
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index f42975e..8ffa0fd 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -95,3 +95,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_SILEAD)	+= silead.o
>> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
>> new file mode 100644
>> index 0000000..22d3546
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/silead.c
>> @@ -0,0 +1,652 @@
>> +/* -------------------------------------------------------------------------
>> + * Copyright (C) 2014-2015, Intel Corporation
>> + *
>> + * Derived from:
>> + *  gslX68X.c
>> + *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
>> + *
>> + *  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 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  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/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/acpi.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>> +#include <linux/input.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/pm.h>
>> +#include <linux/irq.h>
>> +
>> +#define SILEAD_TS_NAME "silead_ts"
>> +
>> +#define SILEAD_REG_RESET	0xE0
>> +#define SILEAD_REG_DATA		0x80
>> +#define SILEAD_REG_TOUCH_NR	0x80
>> +#define SILEAD_REG_POWER	0xBC
>> +#define SILEAD_REG_CLOCK	0xE4
>> +#define SILEAD_REG_STATUS	0xB0
>> +#define SILEAD_REG_ID		0xFC
>> +#define SILEAD_REG_MEM_CHECK	0xB0
>> +
>> +#define SILEAD_STATUS_OK	0x5A5A5A5A
>> +#define SILEAD_TS_DATA_LEN	44
>> +#define SILEAD_CLOCK		0x04
>> +
>> +#define SILEAD_CMD_RESET	0x88
>> +#define SILEAD_CMD_START	0x00
>> +
>> +#define SILEAD_POINT_DATA_LEN	0x04
>> +#define SILEAD_POINT_Y_OFF      0x00
>> +#define SILEAD_POINT_Y_MSB_OFF	0x01
>> +#define SILEAD_POINT_X_OFF	0x02
>> +#define SILEAD_POINT_X_MSB_OFF	0x03
>> +#define SILEAD_POINT_HSB_MASK	0x0F
>> +#define SILEAD_TOUCH_ID_MASK	0xF0
>> +
>> +#define SILEAD_CMD_SLEEP_MIN	10000
>> +#define SILEAD_CMD_SLEEP_MAX	20000
>> +#define SILEAD_POWER_SLEEP	20
>> +#define SILEAD_STARTUP_SLEEP	30
>> +
>> +#define SILEAD_MAX_FINGERS	10
>> +#define SILEAD_MAX_X		4096
>> +#define SILEAD_MAX_Y		4096
>> +
>> +enum silead_ts_power {
>> +	SILEAD_POWER_ON  = 1,
>> +	SILEAD_POWER_OFF = 0
>> +};
>> +
>> +struct silead_ts_data {
>> +	struct i2c_client *client;
>> +	struct gpio_desc *gpio_power;
>> +	struct input_dev *input;
>> +	const char *custom_fw_name;
>> +	char fw_name[I2C_NAME_SIZE];
>> +	u32 x_max;
>> +	u32 y_max;
>> +	u32 max_fingers;
>> +	bool x_invert;
>> +	bool y_invert;
>> +	bool xy_swap;
>> +	u32 chip_id;
>> +	struct input_mt_pos pos[SILEAD_MAX_FINGERS];
>> +	int slots[SILEAD_MAX_FINGERS];
>> +	int id[SILEAD_MAX_FINGERS];
>> +};
>> +
>> +struct silead_fw_data {
>> +	u32 offset;
>> +	u32 val;
>> +};
>> +
>> +static int silead_ts_request_input_dev(struct silead_ts_data *data)
>> +{
>> +	struct device *dev = &data->client->dev;
>> +	int error;
>> +
>> +	data->input = devm_input_allocate_device(dev);
>> +	if (!data->input) {
>> +		dev_err(dev,
>> +			"Failed to allocate input device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (!data->xy_swap) {
>> +		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
>> +				     data->x_max, 0, 0);
>> +		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
>> +				     data->y_max, 0, 0);
>> +	} else {
>> +		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
>> +				     data->y_max, 0, 0);
>> +		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
>> +				     data->x_max, 0, 0);
>> +	}
>> +
>> +	input_mt_init_slots(data->input, data->max_fingers,
>> +			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
>> +			    INPUT_MT_TRACK);
>> +
>> +	data->input->name = SILEAD_TS_NAME;
>> +	data->input->phys = "input/ts";
>> +	data->input->id.bustype = BUS_I2C;
>> +
>> +	error = input_register_device(data->input);
>> +	if (error) {
>> +		dev_err(dev, "Failed to register input device: %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
>> +				   u8 id)
>> +{
>> +	if (data->x_invert)
>> +		x = data->x_max - x;
>> +
>> +	if (data->y_invert)
>> +		y = data->y_max - y;
>> +
>> +	if (data->xy_swap)
>> +		swap(x, y);
>> +
>> +	input_mt_slot(data->input, id);
>> +	input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
>> +	input_report_abs(data->input, ABS_MT_POSITION_X, x);
>> +	input_report_abs(data->input, ABS_MT_POSITION_Y, y);
>> +}
>> +
>> +static void silead_ts_set_power(struct i2c_client *client,
>> +				enum silead_ts_power state)
>> +{
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +
>> +	if (data->gpio_power) {
>> +		gpiod_set_value_cansleep(data->gpio_power, state);
>> +		msleep(SILEAD_POWER_SLEEP);
>> +	}
>> +}
>> +
>> +static void silead_ts_read_data(struct i2c_client *client)
>> +{
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +	struct device *dev = &client->dev;
>> +	u8 buf[SILEAD_TS_DATA_LEN];
>> +	int x, y, touch_nr, error, i, offset;
>> +
>> +	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
>> +					    SILEAD_TS_DATA_LEN, buf);
>> +	if (error < 0) {
>> +		dev_err(dev, "Data read error %d\n", error);
>> +		return;
>> +	}
>> +
>> +	touch_nr = buf[0];
>
> What is touch NR? I.e. can we use it as slot ID instead of resorting to
> in-kernel tracking?

On some models yes, the problem is we do not really know which models so
we're keeping this info here to try and enable using the hardware
tracking on later models and use in kernel tracking everywhere now.

>
>> +
> Extra blank line.

Will fix for the next version (which I will do when we've agreement on
how to deal with your other remarks).

>
>> +	if (touch_nr < 0)
>> +		return;
>> +
>> +	if (touch_nr > data->max_fingers) {
>> +		dev_warn(dev, "More touches reported then supported %d > %d\n",
>> +			 touch_nr, data->max_fingers);
>> +		touch_nr = data->max_fingers;
>> +	}
>> +
>> +	dev_dbg(dev, "Touch number: %d\n", touch_nr);
>> +
>> +	for (i = 0; i < touch_nr; i++) {
>> +		offset = (i + 1) * SILEAD_POINT_DATA_LEN;
>> +
>> +		/* Bits 4-7 are the touch id */
>> +		data->id[i] = (buf[offset + SILEAD_POINT_X_MSB_OFF] &
>> +			      SILEAD_TOUCH_ID_MASK) >> 4;
>> +
>> +		/* Bits 0-3 are MSB of X */
>> +		buf[offset + SILEAD_POINT_X_MSB_OFF] =
>> +					buf[offset + SILEAD_POINT_X_MSB_OFF] &
>> +					SILEAD_POINT_HSB_MASK;
>> +
>> +		/* Bits 0-3 are MSB of Y */
>> +		buf[offset + SILEAD_POINT_Y_MSB_OFF] =
>> +					buf[offset + SILEAD_POINT_Y_MSB_OFF] &
>> +					SILEAD_POINT_HSB_MASK;
>> +
>> +		y = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_Y_OFF));
>> +		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
>> +
>> +		data->pos[i].x = x;
>> +		data->pos[i].y = y;
>> +	}
>> +
>> +	input_mt_assign_slots(data->input, data->slots, data->pos, touch_nr, 0);
>> +
>> +	for (i = 0; i < touch_nr; i++) {
>> +		silead_ts_report_touch(data, data->pos[i].x, data->pos[i].y,
>> +				       data->slots[i]);
>> +
>> +		dev_dbg(dev, "x=%d y=%d hw_id=%d sw_id=%d\n", data->pos[i].x,
>> +			data->pos[i].y, data->id[i], data->slots[i]);
>> +	}
>> +
>> +	input_mt_sync_frame(data->input);
>> +	input_sync(data->input);
>> +}
>> +
>> +static int silead_ts_init(struct i2c_client *client)
>> +{
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +	int error;
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
>> +					SILEAD_CMD_RESET);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
>> +					data->max_fingers);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
>> +					  SILEAD_CLOCK);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
>> +					SILEAD_CMD_START);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	return 0;
>> +
>> +i2c_write_err:
>> +	dev_err(&client->dev, "Registers clear error %d\n", error);
>> +	return error;
>> +}
>> +
>> +static int silead_ts_reset(struct i2c_client *client)
>> +{
>> +	int error;
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
>> +					SILEAD_CMD_RESET);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
>> +					  SILEAD_CLOCK);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
>> +					SILEAD_CMD_START);
>> +	if (error)
>> +		goto i2c_write_err;
>> +	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
>> +
>> +	return 0;
>> +
>> +i2c_write_err:
>> +	dev_err(&client->dev, "Chip reset error %d\n", error);
>> +	return error;
>> +}
>> +
>> +static int silead_ts_startup(struct i2c_client *client)
>> +{
>> +	int error;
>> +
>> +	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
>> +	if (error) {
>> +		dev_err(&client->dev, "Startup error %d\n", error);
>> +		return error;
>> +	}
>> +	msleep(SILEAD_STARTUP_SLEEP);
>> +
>> +	return 0;
>> +}
>> +
>> +static int silead_ts_load_fw(struct i2c_client *client)
>> +{
>> +	struct device *dev = &client->dev;
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +	unsigned int fw_size, i;
>> +	const struct firmware *fw;
>> +	struct silead_fw_data *fw_data;
>> +	int error;
>> +
>> +	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
>> +
>> +	if (data->custom_fw_name)
>> +		error = request_firmware(&fw, data->custom_fw_name, dev);
>> +	else
>> +		error = request_firmware(&fw, data->fw_name, dev);
>> +
>> +	if (error) {
>> +		dev_err(dev, "Firmware request error %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	fw_size = fw->size / sizeof(*fw_data);
>> +	fw_data = (struct silead_fw_data *)fw->data;
>> +
>> +	for (i = 0; i < fw_size; i++) {
>> +		error = i2c_smbus_write_i2c_block_data(client,
>> +						       fw_data[i].offset,
>> +						       4,
>> +						       (u8 *)&fw_data[i].val);
>> +		if (error) {
>> +			dev_err(dev, "Firmware load error %d\n", error);
>> +			goto release_fw_err;
>> +		}
>> +	}
>> +
>> +	release_firmware(fw);
>> +	return 0;
>> +
>> +release_fw_err:
>> +	release_firmware(fw);
>> +	return error;
>> +}
>> +
>> +static u32 silead_ts_get_status(struct i2c_client *client)
>> +{
>> +	int error;
>> +	u32 status;
>> +
>> +	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
>> +					    (u8 *)&status);
>> +	if (error < 0) {
>> +		dev_err(&client->dev, "Status read error %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	return le32_to_cpu(status);
>> +}
>> +
>> +static int silead_ts_get_id(struct i2c_client *client)
>> +{
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +	int error;
>> +
>> +	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
>> +					    (u8 *)&data->chip_id);
>> +
>> +	data->chip_id = le32_to_cpu(data->chip_id);
>> +
>> +	if (error < 0) {
>> +		dev_err(&client->dev, "Chip ID read error %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int silead_ts_setup(struct i2c_client *client)
>> +{
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +	struct device *dev = &client->dev;
>> +	int error;
>> +	u32 status;
>> +
>> +	silead_ts_set_power(client, SILEAD_POWER_OFF);
>> +	silead_ts_set_power(client, SILEAD_POWER_ON);
>> +
>> +	error = silead_ts_get_id(client);
>> +	if (error)
>> +		return error;
>> +	dev_info(dev, "Silead chip ID: 0x%8X", data->chip_id);
>> +
>> +	error = silead_ts_init(client);
>> +	if (error)
>> +		return error;
>> +
>> +	error = silead_ts_reset(client);
>> +	if (error)
>> +		return error;
>> +
>> +	error = silead_ts_load_fw(client);
>> +	if (error)
>> +		return error;
>> +
>> +	error = silead_ts_startup(client);
>> +	if (error)
>> +		return error;
>> +
>> +	status = silead_ts_get_status(client);
>> +	if (status != SILEAD_STATUS_OK) {
>> +		dev_err(dev, "Initialization error, status: 0x%X\n", status);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
>> +{
>> +	struct silead_ts_data *data = id;
>> +	struct i2c_client *client = data->client;
>> +
>> +	silead_ts_read_data(client);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int silead_ts_read_props(struct i2c_client *client)
>> +{
>> +	struct silead_ts_data *data = i2c_get_clientdata(client);
>> +	struct device *dev = &client->dev;
>> +	int error;
>> +
>> +	error = device_property_read_u32(dev, "touchscreen-size-x",
>> +					&data->x_max);
>> +	if (error) {
>> +		dev_dbg(dev, "Resolution X read error %d\n", error);
>> +		data->x_max = SILEAD_MAX_X;
>> +	}
>> +	data->x_max--; /* Property contains size not max */
>> +
>> +	error = device_property_read_u32(dev, "touchscreen-size-y",
>> +					&data->y_max);
>> +	if (error) {
>> +		dev_dbg(dev, "Resolution Y read error %d\n", error);
>> +		data->y_max = SILEAD_MAX_Y;
>> +	}
>> +	data->y_max--; /* Property contains size not max */
>> +
>> +	error = device_property_read_u32(dev, "touchscreen-max-fingers",
>> +					 &data->max_fingers);
>> +	if (error) {
>> +		dev_dbg(dev, "Max fingers read error %d\n", error);
>> +		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
>> +	}
>> +
>> +	error = device_property_read_string(dev, "touchscreen-fw-name",
>> +					  &data->custom_fw_name);
>> +	if (error)
>> +		dev_dbg(dev, "Firmware file name read error. Using default.");
>
> Why do we need separate variable for this?

As said the firmware is not really firmware, but configuration data and
each different digitizer (or the same digitizer wired up differently)
needs different configuration data.

>> +
>> +	data->x_invert = device_property_read_bool(dev,
>> +						"touchscreen-inverted-x");
>> +	data->y_invert = device_property_read_bool(dev,
>> +						"touchscreen-inverted-y");
>> +	data->xy_swap = device_property_read_bool(dev,
>> +						"touchscreen-swapped-x-y");
>> +
>> +	dev_dbg(dev, "x_max = %d, y_max = %d, max_fingers = %d, x_invert = %d, y_invert = %d, xy_swap = %d",
>> +		data->x_max, data->y_max, data->max_fingers, data->x_invert,
>> +		data->y_invert, data->xy_swap);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_ACPI
>> +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
>> +					 const struct i2c_device_id *id)
>> +{
>> +	const struct acpi_device_id *acpi_id;
>> +	struct device *dev = &data->client->dev;
>> +	int i;
>> +
>> +	if (ACPI_HANDLE(dev)) {
>> +		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
>> +		if (!acpi_id)
>> +			return -ENODEV;
>> +
>> +		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
>> +			acpi_id->id);
>> +
>> +		for (i = 0; i < strlen(data->fw_name); i++)
>> +			data->fw_name[i] = tolower(data->fw_name[i]);
>> +	} else {
>> +		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
>> +			id->name);
>> +	}
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
>> +					 const struct i2c_device_id *id)
>> +{
>> +	snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw", id->name);
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static int silead_ts_probe(struct i2c_client *client,
>> +			   const struct i2c_device_id *id)
>> +{
>> +	struct silead_ts_data *data;
>> +	struct device *dev = &client->dev;
>> +	int error;
>> +
>> +	if (!i2c_check_functionality(client->adapter,
>> +				     I2C_FUNC_I2C |
>> +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK |
>> +				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
>> +		dev_err(dev, "I2C functionality check failed\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	i2c_set_clientdata(client, data);
>> +	data->client = client;
>> +
>> +	error = silead_ts_set_default_fw_name(data, id);
>> +	if (error)
>> +		return error;
>> +
>> +	/* We must have the IRQ provided by DT or ACPI subsytem */
>> +	if (client->irq <= 0)
>> +		return -ENODEV;
>> +
>> +	/* Power GPIO pin */
>> +	data->gpio_power = devm_gpiod_get_index(dev, "power",
>> +						0, GPIOD_OUT_LOW);
>
> Why "get index" and not devm_gpiod_get_optional()?
>
>> +	if (IS_ERR(data->gpio_power)) {
>> +		dev_dbg(dev, "Shutdown GPIO request failed\n");
>> +		data->gpio_power = NULL;
>
> You want to abort on errors, especially if you get -EPROBE_DEFER.
>
>> +	}
>> +
>> +	error = silead_ts_read_props(client);
>> +	if (error)
>> +		return error;
>> +
>> +	error = silead_ts_setup(client);
>> +	if (error)
>> +		return error;
>> +
>> +	error = silead_ts_request_input_dev(data);
>> +	if (error)
>> +		return error;
>> +
>> +	error = devm_request_threaded_irq(dev, client->irq, NULL,
>> +					silead_ts_threaded_irq_handler,
>> +					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
>
> Please just use IRQF_ONESHOT and rely on platform code (ACPI. DT, etc)
> to specify interrupt trigger.

Ack, will fix.

>> +					client->name, data);
>> +	if (error) {
>> +		dev_err(dev, "IRQ request failed %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	dev_dbg(dev, "Probing succeded\n");
>
> You'd see if it failed... Just drop.

Ack, will fix.

>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused silead_ts_suspend(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +
>> +	silead_ts_set_power(client, SILEAD_POWER_OFF);
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused silead_ts_resume(struct device *dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	int error, status;
>> +
>> +	silead_ts_set_power(client, SILEAD_POWER_ON);
>> +
>> +	error = silead_ts_reset(client);
>> +	if (error)
>> +		return error;
>> +
>> +	error = silead_ts_startup(client);
>> +	if (error)
>> +		return error;
>> +
>> +	status = silead_ts_get_status(client);
>> +	if (status != SILEAD_STATUS_OK) {
>> +		dev_err(dev, "Resume error, status: 0x%X\n", status);
>
> "%#02X"

Ack, will fix.

>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
>> +
>> +static const struct i2c_device_id silead_ts_id[] = {
>> +	{ "gsl1680", 0 },
>> +	{ "gsl1688", 0 },
>> +	{ "gsl3670", 0 },
>> +	{ "gsl3675", 0 },
>> +	{ "gsl3692", 0 },
>> +	{ "mssl1680", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, silead_ts_id);
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id silead_ts_acpi_match[] = {
>> +	{ "GSL1680", 0 },
>> +	{ "GSL1688", 0 },
>> +	{ "GSL3670", 0 },
>> +	{ "GSL3675", 0 },
>> +	{ "GSL3692", 0 },
>> +	{ "MSSL1680", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
>> +#endif
>> +
>> +static struct i2c_driver silead_ts_driver = {
>> +	.probe = silead_ts_probe,
>> +	.id_table = silead_ts_id,
>> +	.driver = {
>> +		.name = SILEAD_TS_NAME,
>> +		.owner = THIS_MODULE,
>> +		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
>> +		.pm = &silead_ts_pm,
>> +	},
>> +};
>> +module_i2c_driver(silead_ts_driver);
>> +
>> +MODULE_AUTHOR("Robert Dolca <[e-mail]>");
>> +MODULE_DESCRIPTION("Silead I2C touchscreen driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>>
>
> Thanks.

Please let me know if you're happy with my answers to your remarks /
questions which I've not marked as "will fix" and then I'll do a v7.

Regards,

Hans

Dmitry Torokhov July 12, 2016, 8:23 p.m. UTC | #3

On Tue, Jul 12, 2016 at 10:10:24AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-07-16 02:01, Dmitry Torokhov wrote:
> >Hi Hans,
> >
> >On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
> >>From: Robert Dolca <[e-mail]>
> >>
> >>This driver adds support for Silead touchscreens. It has been tested
> >>with GSL1680 and GSL3680 touch panels.
> >>
> >>It supports ACPI and device tree enumeration. Screen resolution,
> >>the maximum number of fingers supported and firmware name are
> >>configurable.
> >>
> >>Signed-off-by: Robert Dolca <[e-mail]>
> >>Signed-off-by: Daniel Jansen <[e-mail]>
> >>Signed-off-by: Hans de Goede <[e-mail]>
> >>---
> >>Changes in v6:
> >>-Default to 5 fingers if max-fingers is not specified
> >
> >From the conversations with Robert, IIRC, the number of touches is not
> >really property of a given controller, but rather firmware that is
> >currently loaded into it and theoretically can change. Given this fact,
> >can we simply expect up to 10 contacts and do not use the property at
> >all?
> 
> We actually write the max-fingers value to a register in the
> chip:
> 
> 	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> 					data->max_fingers);
> 
> So we need the property, because AFAIK we cannot just write 10 for
> "firmware" which only supports 5.

I see.

> 
> Also note there really is not firmware, the "firmware" files contain
> pin configuration data and lookup tables to go from analog pin
> measurements to reported values. One can upgrade a touchscreen from
> say 800x480 resolution to 1024x600 by using another "firmware" as long
> as the 2 use the same pin config (same pins connected to the same
> senselines of the touchscreen).

OK then.

> 
> 
> 
> >
> >>-Improve devicetree binding doc: improve wake-gpios description, use
> >> "See touchscreen.txt" where applicable
> >>---
> >> .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
> >> drivers/input/touchscreen/Kconfig                  |  13 +
> >> drivers/input/touchscreen/Makefile                 |   1 +
> >> drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
> >> 4 files changed, 702 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >> create mode 100644 drivers/input/touchscreen/silead.c
> >>
> >>diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>new file mode 100644
> >>index 0000000..1e58d37
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >>@@ -0,0 +1,36 @@
> >>+* GSL 1680 touchscreen controller
> >>+
> >>+Required properties:
> >>+- compatible		  : "silead,gsl1680"
> >>+- 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 gsl1680 interrupt
> >>+- power-gpios		  : Specification for the pin connected to the gsl1680's
> >>+			    shutdown input. This needs to be driven high to take the
> >>+			    gsl1680 out of its low power state
> >>+- touchscreen-size-x	  : See touchscreen.txt
> >>+- touchscreen-size-y	  : See touchscreen.txt
> >>+- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
> >>+
> >>+Optional properties:
> >>+- touchscreen-inverted-x  : See touchscreen.txt
> >>+- touchscreen-inverted-y  : See touchscreen.txt
> >>+- touchscreen-swapped-x-y : See touchscreen.txt
> >>+
> >>+Example:
> >>+
> >>+i2c@00000000 {
> >>+	gsl1680: touchscreen@40 {
> >>+		compatible = "silead,gsl1680";
> >>+		reg = <0x40>;
> >>+		interrupt-parent = <&pio>;
> >>+		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>;
> >>+		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
> >>+		touchscreen-size-x = <480>;
> >>+		touchscreen-size-y = <800>;
> >>+		touchscreen-max-fingers = <5>;
> >>+		touchscreen-inverted-x;
> >>+		touchscreen-swapped-x-y;
> >>+	};
> >>+};
> >>diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> >>index 8ecdc38..cdd7c4c 100644
> >>--- a/drivers/input/touchscreen/Kconfig
> >>+++ b/drivers/input/touchscreen/Kconfig
> >>@@ -1155,4 +1155,17 @@ config TOUCHSCREEN_ROHM_BU21023
> >> 	  To compile this driver as a module, choose M here: the
> >> 	  module will be called bu21023_ts.
> >>
> >>+config TOUCHSCREEN_SILEAD
> >>+	tristate "Silead I2C touchscreen"
> >>+	depends on I2C
> >>+	help
> >>+	  Say Y here if you have the Silead touchscreen connected to
> >>+	  your system.
> >>+
> >>+	  If unsure, say N.
> >>+
> >>+	  To compile this driver as a module, choose M here: the
> >>+	  module will be called silead.
> >>+
> >>+
> >> endif
> >>diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> >>index f42975e..8ffa0fd 100644
> >>--- a/drivers/input/touchscreen/Makefile
> >>+++ b/drivers/input/touchscreen/Makefile
> >>@@ -95,3 +95,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_SILEAD)	+= silead.o
> >>diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> >>new file mode 100644
> >>index 0000000..22d3546
> >>--- /dev/null
> >>+++ b/drivers/input/touchscreen/silead.c
> >>@@ -0,0 +1,652 @@
> >>+/* -------------------------------------------------------------------------
> >>+ * Copyright (C) 2014-2015, Intel Corporation
> >>+ *
> >>+ * Derived from:
> >>+ *  gslX68X.c
> >>+ *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
> >>+ *
> >>+ *  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 2 of the License, or
> >>+ *  (at your option) any later version.
> >>+ *
> >>+ *  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/i2c.h>
> >>+#include <linux/module.h>
> >>+#include <linux/acpi.h>
> >>+#include <linux/interrupt.h>
> >>+#include <linux/gpio/consumer.h>
> >>+#include <linux/delay.h>
> >>+#include <linux/firmware.h>
> >>+#include <linux/input.h>
> >>+#include <linux/input/mt.h>
> >>+#include <linux/pm.h>
> >>+#include <linux/irq.h>
> >>+
> >>+#define SILEAD_TS_NAME "silead_ts"
> >>+
> >>+#define SILEAD_REG_RESET	0xE0
> >>+#define SILEAD_REG_DATA		0x80
> >>+#define SILEAD_REG_TOUCH_NR	0x80
> >>+#define SILEAD_REG_POWER	0xBC
> >>+#define SILEAD_REG_CLOCK	0xE4
> >>+#define SILEAD_REG_STATUS	0xB0
> >>+#define SILEAD_REG_ID		0xFC
> >>+#define SILEAD_REG_MEM_CHECK	0xB0
> >>+
> >>+#define SILEAD_STATUS_OK	0x5A5A5A5A
> >>+#define SILEAD_TS_DATA_LEN	44
> >>+#define SILEAD_CLOCK		0x04
> >>+
> >>+#define SILEAD_CMD_RESET	0x88
> >>+#define SILEAD_CMD_START	0x00
> >>+
> >>+#define SILEAD_POINT_DATA_LEN	0x04
> >>+#define SILEAD_POINT_Y_OFF      0x00
> >>+#define SILEAD_POINT_Y_MSB_OFF	0x01
> >>+#define SILEAD_POINT_X_OFF	0x02
> >>+#define SILEAD_POINT_X_MSB_OFF	0x03
> >>+#define SILEAD_POINT_HSB_MASK	0x0F
> >>+#define SILEAD_TOUCH_ID_MASK	0xF0
> >>+
> >>+#define SILEAD_CMD_SLEEP_MIN	10000
> >>+#define SILEAD_CMD_SLEEP_MAX	20000
> >>+#define SILEAD_POWER_SLEEP	20
> >>+#define SILEAD_STARTUP_SLEEP	30
> >>+
> >>+#define SILEAD_MAX_FINGERS	10
> >>+#define SILEAD_MAX_X		4096
> >>+#define SILEAD_MAX_Y		4096
> >>+
> >>+enum silead_ts_power {
> >>+	SILEAD_POWER_ON  = 1,
> >>+	SILEAD_POWER_OFF = 0
> >>+};
> >>+
> >>+struct silead_ts_data {
> >>+	struct i2c_client *client;
> >>+	struct gpio_desc *gpio_power;
> >>+	struct input_dev *input;
> >>+	const char *custom_fw_name;
> >>+	char fw_name[I2C_NAME_SIZE];
> >>+	u32 x_max;
> >>+	u32 y_max;
> >>+	u32 max_fingers;
> >>+	bool x_invert;
> >>+	bool y_invert;
> >>+	bool xy_swap;
> >>+	u32 chip_id;
> >>+	struct input_mt_pos pos[SILEAD_MAX_FINGERS];
> >>+	int slots[SILEAD_MAX_FINGERS];
> >>+	int id[SILEAD_MAX_FINGERS];
> >>+};
> >>+
> >>+struct silead_fw_data {
> >>+	u32 offset;
> >>+	u32 val;
> >>+};
> >>+
> >>+static int silead_ts_request_input_dev(struct silead_ts_data *data)
> >>+{
> >>+	struct device *dev = &data->client->dev;
> >>+	int error;
> >>+
> >>+	data->input = devm_input_allocate_device(dev);
> >>+	if (!data->input) {
> >>+		dev_err(dev,
> >>+			"Failed to allocate input device\n");
> >>+		return -ENOMEM;
> >>+	}
> >>+
> >>+	if (!data->xy_swap) {
> >>+		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> >>+				     data->x_max, 0, 0);
> >>+		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> >>+				     data->y_max, 0, 0);
> >>+	} else {
> >>+		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
> >>+				     data->y_max, 0, 0);
> >>+		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
> >>+				     data->x_max, 0, 0);
> >>+	}
> >>+
> >>+	input_mt_init_slots(data->input, data->max_fingers,
> >>+			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
> >>+			    INPUT_MT_TRACK);
> >>+
> >>+	data->input->name = SILEAD_TS_NAME;
> >>+	data->input->phys = "input/ts";
> >>+	data->input->id.bustype = BUS_I2C;
> >>+
> >>+	error = input_register_device(data->input);
> >>+	if (error) {
> >>+		dev_err(dev, "Failed to register input device: %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
> >>+				   u8 id)
> >>+{
> >>+	if (data->x_invert)
> >>+		x = data->x_max - x;
> >>+
> >>+	if (data->y_invert)
> >>+		y = data->y_max - y;
> >>+
> >>+	if (data->xy_swap)
> >>+		swap(x, y);
> >>+
> >>+	input_mt_slot(data->input, id);
> >>+	input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
> >>+	input_report_abs(data->input, ABS_MT_POSITION_X, x);
> >>+	input_report_abs(data->input, ABS_MT_POSITION_Y, y);
> >>+}
> >>+
> >>+static void silead_ts_set_power(struct i2c_client *client,
> >>+				enum silead_ts_power state)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+
> >>+	if (data->gpio_power) {
> >>+		gpiod_set_value_cansleep(data->gpio_power, state);
> >>+		msleep(SILEAD_POWER_SLEEP);
> >>+	}
> >>+}
> >>+
> >>+static void silead_ts_read_data(struct i2c_client *client)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	struct device *dev = &client->dev;
> >>+	u8 buf[SILEAD_TS_DATA_LEN];
> >>+	int x, y, touch_nr, error, i, offset;
> >>+
> >>+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
> >>+					    SILEAD_TS_DATA_LEN, buf);
> >>+	if (error < 0) {
> >>+		dev_err(dev, "Data read error %d\n", error);
> >>+		return;
> >>+	}
> >>+
> >>+	touch_nr = buf[0];
> >
> >What is touch NR? I.e. can we use it as slot ID instead of resorting to
> >in-kernel tracking?
> 
> On some models yes, the problem is we do not really know which models so
> we're keeping this info here to try and enable using the hardware
> tracking on later models and use in kernel tracking everywhere now.

OK, I guess we can improve that later if we figure out more about
different models.

> 
> >
> >>+
> >Extra blank line.
> 
> Will fix for the next version (which I will do when we've agreement on
> how to deal with your other remarks).
> 
> >
> >>+	if (touch_nr < 0)
> >>+		return;
> >>+
> >>+	if (touch_nr > data->max_fingers) {
> >>+		dev_warn(dev, "More touches reported then supported %d > %d\n",
> >>+			 touch_nr, data->max_fingers);
> >>+		touch_nr = data->max_fingers;
> >>+	}
> >>+
> >>+	dev_dbg(dev, "Touch number: %d\n", touch_nr);
> >>+
> >>+	for (i = 0; i < touch_nr; i++) {
> >>+		offset = (i + 1) * SILEAD_POINT_DATA_LEN;
> >>+
> >>+		/* Bits 4-7 are the touch id */
> >>+		data->id[i] = (buf[offset + SILEAD_POINT_X_MSB_OFF] &
> >>+			      SILEAD_TOUCH_ID_MASK) >> 4;
> >>+
> >>+		/* Bits 0-3 are MSB of X */
> >>+		buf[offset + SILEAD_POINT_X_MSB_OFF] =
> >>+					buf[offset + SILEAD_POINT_X_MSB_OFF] &
> >>+					SILEAD_POINT_HSB_MASK;
> >>+
> >>+		/* Bits 0-3 are MSB of Y */
> >>+		buf[offset + SILEAD_POINT_Y_MSB_OFF] =
> >>+					buf[offset + SILEAD_POINT_Y_MSB_OFF] &
> >>+					SILEAD_POINT_HSB_MASK;

Why don't you mask off the result of le16_to_pcup() instread og mangling the
buffer?

> >>+
> >>+		y = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_Y_OFF));
> >>+		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
> >>+
> >>+		data->pos[i].x = x;
> >>+		data->pos[i].y = y;

I.e. do

		data->pos[i].x = x & 0x0fff;

> >>+	}
> >>+
> >>+	input_mt_assign_slots(data->input, data->slots, data->pos, touch_nr, 0);
> >>+
> >>+	for (i = 0; i < touch_nr; i++) {
> >>+		silead_ts_report_touch(data, data->pos[i].x, data->pos[i].y,
> >>+				       data->slots[i]);
> >>+
> >>+		dev_dbg(dev, "x=%d y=%d hw_id=%d sw_id=%d\n", data->pos[i].x,
> >>+			data->pos[i].y, data->id[i], data->slots[i]);
> >>+	}
> >>+
> >>+	input_mt_sync_frame(data->input);
> >>+	input_sync(data->input);
> >>+}
> >>+
> >>+static int silead_ts_init(struct i2c_client *client)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> >>+					SILEAD_CMD_RESET);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
> >>+					data->max_fingers);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
> >>+					  SILEAD_CLOCK);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> >>+					SILEAD_CMD_START);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	return 0;
> >>+
> >>+i2c_write_err:
> >>+	dev_err(&client->dev, "Registers clear error %d\n", error);
> >>+	return error;
> >>+}
> >>+
> >>+static int silead_ts_reset(struct i2c_client *client)
> >>+{
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
> >>+					SILEAD_CMD_RESET);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
> >>+					  SILEAD_CLOCK);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
> >>+					SILEAD_CMD_START);
> >>+	if (error)
> >>+		goto i2c_write_err;
> >>+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
> >>+
> >>+	return 0;
> >>+
> >>+i2c_write_err:
> >>+	dev_err(&client->dev, "Chip reset error %d\n", error);
> >>+	return error;
> >>+}
> >>+
> >>+static int silead_ts_startup(struct i2c_client *client)
> >>+{
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
> >>+	if (error) {
> >>+		dev_err(&client->dev, "Startup error %d\n", error);
> >>+		return error;
> >>+	}
> >>+	msleep(SILEAD_STARTUP_SLEEP);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static int silead_ts_load_fw(struct i2c_client *client)
> >>+{
> >>+	struct device *dev = &client->dev;
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	unsigned int fw_size, i;
> >>+	const struct firmware *fw;
> >>+	struct silead_fw_data *fw_data;
> >>+	int error;
> >>+
> >>+	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
> >>+
> >>+	if (data->custom_fw_name)
> >>+		error = request_firmware(&fw, data->custom_fw_name, dev);
> >>+	else
> >>+		error = request_firmware(&fw, data->fw_name, dev);
> >>+
> >>+	if (error) {
> >>+		dev_err(dev, "Firmware request error %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	fw_size = fw->size / sizeof(*fw_data);
> >>+	fw_data = (struct silead_fw_data *)fw->data;
> >>+
> >>+	for (i = 0; i < fw_size; i++) {
> >>+		error = i2c_smbus_write_i2c_block_data(client,
> >>+						       fw_data[i].offset,
> >>+						       4,
> >>+						       (u8 *)&fw_data[i].val);
> >>+		if (error) {
> >>+			dev_err(dev, "Firmware load error %d\n", error);
> >>+			goto release_fw_err;
> >>+		}
> >>+	}
> >>+
> >>+	release_firmware(fw);
> >>+	return 0;
> >>+
> >>+release_fw_err:
> >>+	release_firmware(fw);

Would love to see firmware being released in one place.

> >>+	return error;
> >>+}
> >>+
> >>+static u32 silead_ts_get_status(struct i2c_client *client)
> >>+{
> >>+	int error;
> >>+	u32 status;
> >>+
> >>+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
> >>+					    (u8 *)&status);
> >>+	if (error < 0) {
> >>+		dev_err(&client->dev, "Status read error %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	return le32_to_cpu(status);
> >>+}
> >>+
> >>+static int silead_ts_get_id(struct i2c_client *client)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	int error;
> >>+
> >>+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
> >>+					    (u8 *)&data->chip_id);
> >>+
> >>+	data->chip_id = le32_to_cpu(data->chip_id);
> >>+
> >>+	if (error < 0) {
> >>+		dev_err(&client->dev, "Chip ID read error %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static int silead_ts_setup(struct i2c_client *client)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	struct device *dev = &client->dev;
> >>+	int error;
> >>+	u32 status;
> >>+
> >>+	silead_ts_set_power(client, SILEAD_POWER_OFF);
> >>+	silead_ts_set_power(client, SILEAD_POWER_ON);
> >>+
> >>+	error = silead_ts_get_id(client);
> >>+	if (error)
> >>+		return error;
> >>+	dev_info(dev, "Silead chip ID: 0x%8X", data->chip_id);
> >>+
> >>+	error = silead_ts_init(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_reset(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_load_fw(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_startup(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	status = silead_ts_get_status(client);
> >>+	if (status != SILEAD_STATUS_OK) {
> >>+		dev_err(dev, "Initialization error, status: 0x%X\n", status);
> >>+		return -ENODEV;
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
> >>+{
> >>+	struct silead_ts_data *data = id;
> >>+	struct i2c_client *client = data->client;
> >>+
> >>+	silead_ts_read_data(client);
> >>+
> >>+	return IRQ_HANDLED;
> >>+}
> >>+
> >>+static int silead_ts_read_props(struct i2c_client *client)
> >>+{
> >>+	struct silead_ts_data *data = i2c_get_clientdata(client);
> >>+	struct device *dev = &client->dev;
> >>+	int error;
> >>+
> >>+	error = device_property_read_u32(dev, "touchscreen-size-x",
> >>+					&data->x_max);
> >>+	if (error) {
> >>+		dev_dbg(dev, "Resolution X read error %d\n", error);
> >>+		data->x_max = SILEAD_MAX_X;
> >>+	}
> >>+	data->x_max--; /* Property contains size not max */
> >>+
> >>+	error = device_property_read_u32(dev, "touchscreen-size-y",
> >>+					&data->y_max);
> >>+	if (error) {
> >>+		dev_dbg(dev, "Resolution Y read error %d\n", error);
> >>+		data->y_max = SILEAD_MAX_Y;
> >>+	}
> >>+	data->y_max--; /* Property contains size not max */
> >>+
> >>+	error = device_property_read_u32(dev, "touchscreen-max-fingers",
> >>+					 &data->max_fingers);
> >>+	if (error) {
> >>+		dev_dbg(dev, "Max fingers read error %d\n", error);
> >>+		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
> >>+	}
> >>+
> >>+	error = device_property_read_string(dev, "touchscreen-fw-name",
> >>+					  &data->custom_fw_name);
> >>+	if (error)
> >>+		dev_dbg(dev, "Firmware file name read error. Using default.");
> >
> >Why do we need separate variable for this?
> 
> As said the firmware is not really firmware, but configuration data and
> each different digitizer (or the same digitizer wired up differently)
> needs different configuration data.

Right, but can you store this info in data->fw_name? You will probably have to
move call to silead_ts_set_default_fw_name() after parsing DT and check for
data->fw_name being non-null... 

> 
> >>+
> >>+	data->x_invert = device_property_read_bool(dev,
> >>+						"touchscreen-inverted-x");
> >>+	data->y_invert = device_property_read_bool(dev,
> >>+						"touchscreen-inverted-y");
> >>+	data->xy_swap = device_property_read_bool(dev,
> >>+						"touchscreen-swapped-x-y");
> >>+
> >>+	dev_dbg(dev, "x_max = %d, y_max = %d, max_fingers = %d, x_invert = %d, y_invert = %d, xy_swap = %d",
> >>+		data->x_max, data->y_max, data->max_fingers, data->x_invert,
> >>+		data->y_invert, data->xy_swap);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+#ifdef CONFIG_ACPI
> >>+static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> >>+					 const struct i2c_device_id *id)
> >>+{
> >>+	const struct acpi_device_id *acpi_id;
> >>+	struct device *dev = &data->client->dev;
> >>+	int i;
> >>+
> >>+	if (ACPI_HANDLE(dev)) {
> >>+		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> >>+		if (!acpi_id)
> >>+			return -ENODEV;
> >>+
> >>+		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
> >>+			acpi_id->id);
> >>+
> >>+		for (i = 0; i < strlen(data->fw_name); i++)
> >>+			data->fw_name[i] = tolower(data->fw_name[i]);
> >>+	} else {
> >>+		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
> >>+			id->name);
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+#else
> >>+static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
> >>+					 const struct i2c_device_id *id)
> >>+{
> >>+	snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw", id->name);
> >>+	return 0;
> >>+}
> >>+#endif
> >>+
> >>+static int silead_ts_probe(struct i2c_client *client,
> >>+			   const struct i2c_device_id *id)
> >>+{
> >>+	struct silead_ts_data *data;
> >>+	struct device *dev = &client->dev;
> >>+	int error;
> >>+
> >>+	if (!i2c_check_functionality(client->adapter,
> >>+				     I2C_FUNC_I2C |
> >>+				     I2C_FUNC_SMBUS_READ_I2C_BLOCK |
> >>+				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> >>+		dev_err(dev, "I2C functionality check failed\n");
> >>+		return -ENXIO;
> >>+	}
> >>+
> >>+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >>+	if (!data)
> >>+		return -ENOMEM;
> >>+
> >>+	i2c_set_clientdata(client, data);
> >>+	data->client = client;
> >>+
> >>+	error = silead_ts_set_default_fw_name(data, id);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	/* We must have the IRQ provided by DT or ACPI subsytem */
> >>+	if (client->irq <= 0)
> >>+		return -ENODEV;
> >>+
> >>+	/* Power GPIO pin */
> >>+	data->gpio_power = devm_gpiod_get_index(dev, "power",
> >>+						0, GPIOD_OUT_LOW);
> >
> >Why "get index" and not devm_gpiod_get_optional()?
> >
> >>+	if (IS_ERR(data->gpio_power)) {
> >>+		dev_dbg(dev, "Shutdown GPIO request failed\n");
> >>+		data->gpio_power = NULL;
> >
> >You want to abort on errors, especially if you get -EPROBE_DEFER.
> >
> >>+	}
> >>+
> >>+	error = silead_ts_read_props(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_setup(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_request_input_dev(data);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = devm_request_threaded_irq(dev, client->irq, NULL,
> >>+					silead_ts_threaded_irq_handler,
> >>+					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
> >
> >Please just use IRQF_ONESHOT and rely on platform code (ACPI. DT, etc)
> >to specify interrupt trigger.
> 
> Ack, will fix.
> 
> >>+					client->name, data);
> >>+	if (error) {
> >>+		dev_err(dev, "IRQ request failed %d\n", error);
> >>+		return error;
> >>+	}
> >>+
> >>+	dev_dbg(dev, "Probing succeded\n");
> >
> >You'd see if it failed... Just drop.
> 
> Ack, will fix.
> 
> >>+	return 0;
> >>+}
> >>+
> >>+static int __maybe_unused silead_ts_suspend(struct device *dev)
> >>+{
> >>+	struct i2c_client *client = to_i2c_client(dev);
> >>+
> >>+	silead_ts_set_power(client, SILEAD_POWER_OFF);
> >>+	return 0;
> >>+}
> >>+
> >>+static int __maybe_unused silead_ts_resume(struct device *dev)
> >>+{
> >>+	struct i2c_client *client = to_i2c_client(dev);
> >>+	int error, status;
> >>+
> >>+	silead_ts_set_power(client, SILEAD_POWER_ON);
> >>+
> >>+	error = silead_ts_reset(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	error = silead_ts_startup(client);
> >>+	if (error)
> >>+		return error;
> >>+
> >>+	status = silead_ts_get_status(client);
> >>+	if (status != SILEAD_STATUS_OK) {
> >>+		dev_err(dev, "Resume error, status: 0x%X\n", status);
> >
> >"%#02X"
> 
> Ack, will fix.
> 
> >>+		return -ENODEV;
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
> >>+
> >>+static const struct i2c_device_id silead_ts_id[] = {
> >>+	{ "gsl1680", 0 },
> >>+	{ "gsl1688", 0 },
> >>+	{ "gsl3670", 0 },
> >>+	{ "gsl3675", 0 },
> >>+	{ "gsl3692", 0 },
> >>+	{ "mssl1680", 0 },
> >>+	{ }
> >>+};
> >>+MODULE_DEVICE_TABLE(i2c, silead_ts_id);
> >>+
> >>+#ifdef CONFIG_ACPI
> >>+static const struct acpi_device_id silead_ts_acpi_match[] = {
> >>+	{ "GSL1680", 0 },
> >>+	{ "GSL1688", 0 },
> >>+	{ "GSL3670", 0 },
> >>+	{ "GSL3675", 0 },
> >>+	{ "GSL3692", 0 },
> >>+	{ "MSSL1680", 0 },
> >>+	{ }
> >>+};
> >>+MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
> >>+#endif
> >>+
> >>+static struct i2c_driver silead_ts_driver = {
> >>+	.probe = silead_ts_probe,
> >>+	.id_table = silead_ts_id,
> >>+	.driver = {
> >>+		.name = SILEAD_TS_NAME,
> >>+		.owner = THIS_MODULE,
> >>+		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
> >>+		.pm = &silead_ts_pm,
> >>+	},
> >>+};
> >>+module_i2c_driver(silead_ts_driver);
> >>+
> >>+MODULE_AUTHOR("Robert Dolca <[e-mail]>");
> >>+MODULE_DESCRIPTION("Silead I2C touchscreen driver");
> >>+MODULE_LICENSE("GPL");
> >>--
> >>2.7.4
> >>
> >
> >Thanks.
> 
> Please let me know if you're happy with my answers to your remarks /
> questions which I've not marked as "will fix" and then I'll do a v7.

Yes I am for the most part, plus I had a couple of more comments above.

Thanks.

Rob Herring July 16, 2016, 5:56 p.m. UTC | #4

On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
> From: Robert Dolca <[e-mail]>
> 
> This driver adds support for Silead touchscreens. It has been tested
> with GSL1680 and GSL3680 touch panels.
> 
> It supports ACPI and device tree enumeration. Screen resolution,
> the maximum number of fingers supported and firmware name are
> configurable.
> 
> Signed-off-by: Robert Dolca <[e-mail]>
> Signed-off-by: Daniel Jansen <[e-mail]>
> Signed-off-by: Hans de Goede <[e-mail]>
> ---
> Changes in v6:
> -Default to 5 fingers if max-fingers is not specified
> -Improve devicetree binding doc: improve wake-gpios description, use
>  "See touchscreen.txt" where applicable
> ---
>  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
>  drivers/input/touchscreen/Kconfig                  |  13 +
>  drivers/input/touchscreen/Makefile                 |   1 +
>  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
>  4 files changed, 702 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>  create mode 100644 drivers/input/touchscreen/silead.c
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> new file mode 100644
> index 0000000..1e58d37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> @@ -0,0 +1,36 @@
> +* GSL 1680 touchscreen controller
> +
> +Required properties:
> +- compatible		  : "silead,gsl1680"
> +- 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 gsl1680 interrupt
> +- power-gpios		  : Specification for the pin connected to the gsl1680's
> +			    shutdown input. This needs to be driven high to take the
> +			    gsl1680 out of its low power state
> +- touchscreen-size-x	  : See touchscreen.txt
> +- touchscreen-size-y	  : See touchscreen.txt
> +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect

If you name it like a standard property, then it should be documented in 
touchscreen.txt.

> +Optional properties:
> +- touchscreen-inverted-x  : See touchscreen.txt
> +- touchscreen-inverted-y  : See touchscreen.txt
> +- touchscreen-swapped-x-y : See touchscreen.txt
> +
> +Example:
> +
> +i2c@00000000 {
> +	gsl1680: touchscreen@40 {
> +		compatible = "silead,gsl1680";
> +		reg = <0x40>;
> +		interrupt-parent = <&pio>;
> +		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>;
> +		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
> +		touchscreen-size-x = <480>;
> +		touchscreen-size-y = <800>;
> +		touchscreen-max-fingers = <5>;
> +		touchscreen-inverted-x;
> +		touchscreen-swapped-x-y;
> +	};
> +};

Dmitry Torokhov July 19, 2016, 6:44 p.m. UTC | #5

On Sat, Jul 16, 2016 at 12:56:05PM -0500, Rob Herring wrote:
> On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
> > From: Robert Dolca <[e-mail]>
> > 
> > This driver adds support for Silead touchscreens. It has been tested
> > with GSL1680 and GSL3680 touch panels.
> > 
> > It supports ACPI and device tree enumeration. Screen resolution,
> > the maximum number of fingers supported and firmware name are
> > configurable.
> > 
> > Signed-off-by: Robert Dolca <[e-mail]>
> > Signed-off-by: Daniel Jansen <[e-mail]>
> > Signed-off-by: Hans de Goede <[e-mail]>
> > ---
> > Changes in v6:
> > -Default to 5 fingers if max-fingers is not specified
> > -Improve devicetree binding doc: improve wake-gpios description, use
> >  "See touchscreen.txt" where applicable
> > ---
> >  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
> >  drivers/input/touchscreen/Kconfig                  |  13 +
> >  drivers/input/touchscreen/Makefile                 |   1 +
> >  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
> >  4 files changed, 702 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> >  create mode 100644 drivers/input/touchscreen/silead.c
> > 
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> > new file mode 100644
> > index 0000000..1e58d37
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> > @@ -0,0 +1,36 @@
> > +* GSL 1680 touchscreen controller
> > +
> > +Required properties:
> > +- compatible		  : "silead,gsl1680"
> > +- 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 gsl1680 interrupt
> > +- power-gpios		  : Specification for the pin connected to the gsl1680's
> > +			    shutdown input. This needs to be driven high to take the
> > +			    gsl1680 out of its low power state
> > +- touchscreen-size-x	  : See touchscreen.txt
> > +- touchscreen-size-y	  : See touchscreen.txt
> > +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
> 
> If you name it like a standard property, then it should be documented in 
> touchscreen.txt.

Actually this is very much Silead-specific wart, I'd rather we called it
"max-fingers" or "silead,max-fingers" to illustrate that it is not at
all a generic property.

Rob, will you be OK with it?

Thanks.

Dmitry Torokhov July 27, 2016, 9:35 p.m. UTC | #6

On Tue, Jul 19, 2016 at 11:44:25AM -0700, Dmitry Torokhov wrote:
> On Sat, Jul 16, 2016 at 12:56:05PM -0500, Rob Herring wrote:
> > On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
> > > From: Robert Dolca <[e-mail]>
> > > 
> > > This driver adds support for Silead touchscreens. It has been tested
> > > with GSL1680 and GSL3680 touch panels.
> > > 
> > > It supports ACPI and device tree enumeration. Screen resolution,
> > > the maximum number of fingers supported and firmware name are
> > > configurable.
> > > 
> > > Signed-off-by: Robert Dolca <[e-mail]>
> > > Signed-off-by: Daniel Jansen <[e-mail]>
> > > Signed-off-by: Hans de Goede <[e-mail]>
> > > ---
> > > Changes in v6:
> > > -Default to 5 fingers if max-fingers is not specified
> > > -Improve devicetree binding doc: improve wake-gpios description, use
> > >  "See touchscreen.txt" where applicable
> > > ---
> > >  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
> > >  drivers/input/touchscreen/Kconfig                  |  13 +
> > >  drivers/input/touchscreen/Makefile                 |   1 +
> > >  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
> > >  4 files changed, 702 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> > >  create mode 100644 drivers/input/touchscreen/silead.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> > > new file mode 100644
> > > index 0000000..1e58d37
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
> > > @@ -0,0 +1,36 @@
> > > +* GSL 1680 touchscreen controller
> > > +
> > > +Required properties:
> > > +- compatible		  : "silead,gsl1680"
> > > +- 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 gsl1680 interrupt
> > > +- power-gpios		  : Specification for the pin connected to the gsl1680's
> > > +			    shutdown input. This needs to be driven high to take the
> > > +			    gsl1680 out of its low power state
> > > +- touchscreen-size-x	  : See touchscreen.txt
> > > +- touchscreen-size-y	  : See touchscreen.txt
> > > +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
> > 
> > If you name it like a standard property, then it should be documented in 
> > touchscreen.txt.
> 
> Actually this is very much Silead-specific wart, I'd rather we called it
> "max-fingers" or "silead,max-fingers" to illustrate that it is not at
> all a generic property.
> 
> Rob, will you be OK with it?

Rob, ping? I'd like to get it into the tree and this is only outstanding
question.

Thanks!

Hans de Goede July 28, 2016, 3:42 p.m. UTC | #7

Hi,

On 27-07-16 23:35, Dmitry Torokhov wrote:
> On Tue, Jul 19, 2016 at 11:44:25AM -0700, Dmitry Torokhov wrote:
>> On Sat, Jul 16, 2016 at 12:56:05PM -0500, Rob Herring wrote:
>>> On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
>>>> From: Robert Dolca <[e-mail]>
>>>>
>>>> This driver adds support for Silead touchscreens. It has been tested
>>>> with GSL1680 and GSL3680 touch panels.
>>>>
>>>> It supports ACPI and device tree enumeration. Screen resolution,
>>>> the maximum number of fingers supported and firmware name are
>>>> configurable.
>>>>
>>>> Signed-off-by: Robert Dolca <[e-mail]>
>>>> Signed-off-by: Daniel Jansen <[e-mail]>
>>>> Signed-off-by: Hans de Goede <[e-mail]>
>>>> ---
>>>> Changes in v6:
>>>> -Default to 5 fingers if max-fingers is not specified
>>>> -Improve devicetree binding doc: improve wake-gpios description, use
>>>>  "See touchscreen.txt" where applicable
>>>> ---
>>>>  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
>>>>  drivers/input/touchscreen/Kconfig                  |  13 +
>>>>  drivers/input/touchscreen/Makefile                 |   1 +
>>>>  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
>>>>  4 files changed, 702 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>>>>  create mode 100644 drivers/input/touchscreen/silead.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>>>> new file mode 100644
>>>> index 0000000..1e58d37
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>>>> @@ -0,0 +1,36 @@
>>>> +* GSL 1680 touchscreen controller
>>>> +
>>>> +Required properties:
>>>> +- compatible		  : "silead,gsl1680"
>>>> +- 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 gsl1680 interrupt
>>>> +- power-gpios		  : Specification for the pin connected to the gsl1680's
>>>> +			    shutdown input. This needs to be driven high to take the
>>>> +			    gsl1680 out of its low power state
>>>> +- touchscreen-size-x	  : See touchscreen.txt
>>>> +- touchscreen-size-y	  : See touchscreen.txt
>>>> +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
>>>
>>> If you name it like a standard property, then it should be documented in
>>> touchscreen.txt.
>>
>> Actually this is very much Silead-specific wart, I'd rather we called it
>> "max-fingers" or "silead,max-fingers" to illustrate that it is not at
>> all a generic property.
>>
>> Rob, will you be OK with it?
>
> Rob, ping? I'd like to get it into the tree and this is only outstanding
> question.

I think that your suggestion to make this "silead,max-fingers" is a good idea,
I will do a v8 with this soon (or feel free to fixup v7 yourself).

Regards,

Hans

Rob Herring July 28, 2016, 6:32 p.m. UTC | #8

On Wed, Jul 27, 2016 at 4:35 PM, Dmitry Torokhov
<[e-mail]> wrote:
> On Tue, Jul 19, 2016 at 11:44:25AM -0700, Dmitry Torokhov wrote:
>> On Sat, Jul 16, 2016 at 12:56:05PM -0500, Rob Herring wrote:
>> > On Mon, Jul 11, 2016 at 05:51:44PM +0200, Hans de Goede wrote:
>> > > From: Robert Dolca <[e-mail]>
>> > >
>> > > This driver adds support for Silead touchscreens. It has been tested
>> > > with GSL1680 and GSL3680 touch panels.
>> > >
>> > > It supports ACPI and device tree enumeration. Screen resolution,
>> > > the maximum number of fingers supported and firmware name are
>> > > configurable.
>> > >
>> > > Signed-off-by: Robert Dolca <[e-mail]>
>> > > Signed-off-by: Daniel Jansen <[e-mail]>
>> > > Signed-off-by: Hans de Goede <[e-mail]>
>> > > ---
>> > > Changes in v6:
>> > > -Default to 5 fingers if max-fingers is not specified
>> > > -Improve devicetree binding doc: improve wake-gpios description, use
>> > >  "See touchscreen.txt" where applicable
>> > > ---
>> > >  .../bindings/input/touchscreen/silead_gsl1680.txt  |  36 ++
>> > >  drivers/input/touchscreen/Kconfig                  |  13 +
>> > >  drivers/input/touchscreen/Makefile                 |   1 +
>> > >  drivers/input/touchscreen/silead.c                 | 652 +++++++++++++++++++++
>> > >  4 files changed, 702 insertions(+)
>> > >  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> > >  create mode 100644 drivers/input/touchscreen/silead.c
>> > >
>> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> > > new file mode 100644
>> > > index 0000000..1e58d37
>> > > --- /dev/null
>> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>> > > @@ -0,0 +1,36 @@
>> > > +* GSL 1680 touchscreen controller
>> > > +
>> > > +Required properties:
>> > > +- compatible               : "silead,gsl1680"
>> > > +- 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 gsl1680 interrupt
>> > > +- power-gpios              : Specification for the pin connected to the gsl1680's
>> > > +                     shutdown input. This needs to be driven high to take the
>> > > +                     gsl1680 out of its low power state
>> > > +- touchscreen-size-x       : See touchscreen.txt
>> > > +- touchscreen-size-y       : See touchscreen.txt
>> > > +- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
>> >
>> > If you name it like a standard property, then it should be documented in
>> > touchscreen.txt.
>>
>> Actually this is very much Silead-specific wart, I'd rather we called it
>> "max-fingers" or "silead,max-fingers" to illustrate that it is not at
>> all a generic property.
>>
>> Rob, will you be OK with it?
>
> Rob, ping? I'd like to get it into the tree and this is only outstanding
> question.

Okay. With vendor prefix please.

Rob

Patch

diff --git a/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
new file mode 100644
index 0000000..1e58d37
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
@@ -0,0 +1,36 @@ 
+* GSL 1680 touchscreen controller
+
+Required properties:
+- compatible		  : "silead,gsl1680"
+- 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 gsl1680 interrupt
+- power-gpios		  : Specification for the pin connected to the gsl1680's
+			    shutdown input. This needs to be driven high to take the
+			    gsl1680 out of its low power state
+- touchscreen-size-x	  : See touchscreen.txt
+- touchscreen-size-y	  : See touchscreen.txt
+- touchscreen-max-fingers : maximum number of fingers the touchscreen can detect
+
+Optional properties:
+- touchscreen-inverted-x  : See touchscreen.txt
+- touchscreen-inverted-y  : See touchscreen.txt
+- touchscreen-swapped-x-y : See touchscreen.txt
+
+Example:
+
+i2c@00000000 {
+	gsl1680: touchscreen@40 {
+		compatible = "silead,gsl1680";
+		reg = <0x40>;
+		interrupt-parent = <&pio>;
+		interrupts = <6 11 IRQ_TYPE_EDGE_FALLING>;
+		power-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>;
+		touchscreen-size-x = <480>;
+		touchscreen-size-y = <800>;
+		touchscreen-max-fingers = <5>;
+		touchscreen-inverted-x;
+		touchscreen-swapped-x-y;
+	};
+};
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 8ecdc38..cdd7c4c 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1155,4 +1155,17 @@  config TOUCHSCREEN_ROHM_BU21023
 	  To compile this driver as a module, choose M here: the
 	  module will be called bu21023_ts.
 
+config TOUCHSCREEN_SILEAD
+	tristate "Silead I2C touchscreen"
+	depends on I2C
+	help
+	  Say Y here if you have the Silead touchscreen connected to
+	  your system.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called silead.
+
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index f42975e..8ffa0fd 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -95,3 +95,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_SILEAD)	+= silead.o
diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
new file mode 100644
index 0000000..22d3546
--- /dev/null
+++ b/drivers/input/touchscreen/silead.c
@@ -0,0 +1,652 @@ 
+/* -------------------------------------------------------------------------
+ * Copyright (C) 2014-2015, Intel Corporation
+ *
+ * Derived from:
+ *  gslX68X.c
+ *  Copyright (C) 2010-2015, Shanghai Sileadinc Co.Ltd
+ *
+ *  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 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  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/i2c.h>
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/pm.h>
+#include <linux/irq.h>
+
+#define SILEAD_TS_NAME "silead_ts"
+
+#define SILEAD_REG_RESET	0xE0
+#define SILEAD_REG_DATA		0x80
+#define SILEAD_REG_TOUCH_NR	0x80
+#define SILEAD_REG_POWER	0xBC
+#define SILEAD_REG_CLOCK	0xE4
+#define SILEAD_REG_STATUS	0xB0
+#define SILEAD_REG_ID		0xFC
+#define SILEAD_REG_MEM_CHECK	0xB0
+
+#define SILEAD_STATUS_OK	0x5A5A5A5A
+#define SILEAD_TS_DATA_LEN	44
+#define SILEAD_CLOCK		0x04
+
+#define SILEAD_CMD_RESET	0x88
+#define SILEAD_CMD_START	0x00
+
+#define SILEAD_POINT_DATA_LEN	0x04
+#define SILEAD_POINT_Y_OFF      0x00
+#define SILEAD_POINT_Y_MSB_OFF	0x01
+#define SILEAD_POINT_X_OFF	0x02
+#define SILEAD_POINT_X_MSB_OFF	0x03
+#define SILEAD_POINT_HSB_MASK	0x0F
+#define SILEAD_TOUCH_ID_MASK	0xF0
+
+#define SILEAD_CMD_SLEEP_MIN	10000
+#define SILEAD_CMD_SLEEP_MAX	20000
+#define SILEAD_POWER_SLEEP	20
+#define SILEAD_STARTUP_SLEEP	30
+
+#define SILEAD_MAX_FINGERS	10
+#define SILEAD_MAX_X		4096
+#define SILEAD_MAX_Y		4096
+
+enum silead_ts_power {
+	SILEAD_POWER_ON  = 1,
+	SILEAD_POWER_OFF = 0
+};
+
+struct silead_ts_data {
+	struct i2c_client *client;
+	struct gpio_desc *gpio_power;
+	struct input_dev *input;
+	const char *custom_fw_name;
+	char fw_name[I2C_NAME_SIZE];
+	u32 x_max;
+	u32 y_max;
+	u32 max_fingers;
+	bool x_invert;
+	bool y_invert;
+	bool xy_swap;
+	u32 chip_id;
+	struct input_mt_pos pos[SILEAD_MAX_FINGERS];
+	int slots[SILEAD_MAX_FINGERS];
+	int id[SILEAD_MAX_FINGERS];
+};
+
+struct silead_fw_data {
+	u32 offset;
+	u32 val;
+};
+
+static int silead_ts_request_input_dev(struct silead_ts_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int error;
+
+	data->input = devm_input_allocate_device(dev);
+	if (!data->input) {
+		dev_err(dev,
+			"Failed to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	if (!data->xy_swap) {
+		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
+				     data->x_max, 0, 0);
+		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
+				     data->y_max, 0, 0);
+	} else {
+		input_set_abs_params(data->input, ABS_MT_POSITION_X, 0,
+				     data->y_max, 0, 0);
+		input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0,
+				     data->x_max, 0, 0);
+	}
+
+	input_mt_init_slots(data->input, data->max_fingers,
+			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
+			    INPUT_MT_TRACK);
+
+	data->input->name = SILEAD_TS_NAME;
+	data->input->phys = "input/ts";
+	data->input->id.bustype = BUS_I2C;
+
+	error = input_register_device(data->input);
+	if (error) {
+		dev_err(dev, "Failed to register input device: %d\n", error);
+		return error;
+	}
+
+	return 0;
+}
+
+static void silead_ts_report_touch(struct silead_ts_data *data, u16 x, u16 y,
+				   u8 id)
+{
+	if (data->x_invert)
+		x = data->x_max - x;
+
+	if (data->y_invert)
+		y = data->y_max - y;
+
+	if (data->xy_swap)
+		swap(x, y);
+
+	input_mt_slot(data->input, id);
+	input_mt_report_slot_state(data->input, MT_TOOL_FINGER, true);
+	input_report_abs(data->input, ABS_MT_POSITION_X, x);
+	input_report_abs(data->input, ABS_MT_POSITION_Y, y);
+}
+
+static void silead_ts_set_power(struct i2c_client *client,
+				enum silead_ts_power state)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+
+	if (data->gpio_power) {
+		gpiod_set_value_cansleep(data->gpio_power, state);
+		msleep(SILEAD_POWER_SLEEP);
+	}
+}
+
+static void silead_ts_read_data(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	u8 buf[SILEAD_TS_DATA_LEN];
+	int x, y, touch_nr, error, i, offset;
+
+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_DATA,
+					    SILEAD_TS_DATA_LEN, buf);
+	if (error < 0) {
+		dev_err(dev, "Data read error %d\n", error);
+		return;
+	}
+
+	touch_nr = buf[0];
+
+	if (touch_nr < 0)
+		return;
+
+	if (touch_nr > data->max_fingers) {
+		dev_warn(dev, "More touches reported then supported %d > %d\n",
+			 touch_nr, data->max_fingers);
+		touch_nr = data->max_fingers;
+	}
+
+	dev_dbg(dev, "Touch number: %d\n", touch_nr);
+
+	for (i = 0; i < touch_nr; i++) {
+		offset = (i + 1) * SILEAD_POINT_DATA_LEN;
+
+		/* Bits 4-7 are the touch id */
+		data->id[i] = (buf[offset + SILEAD_POINT_X_MSB_OFF] &
+			      SILEAD_TOUCH_ID_MASK) >> 4;
+
+		/* Bits 0-3 are MSB of X */
+		buf[offset + SILEAD_POINT_X_MSB_OFF] =
+					buf[offset + SILEAD_POINT_X_MSB_OFF] &
+					SILEAD_POINT_HSB_MASK;
+
+		/* Bits 0-3 are MSB of Y */
+		buf[offset + SILEAD_POINT_Y_MSB_OFF] =
+					buf[offset + SILEAD_POINT_Y_MSB_OFF] &
+					SILEAD_POINT_HSB_MASK;
+
+		y = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_Y_OFF));
+		x = le16_to_cpup((__le16 *)(buf + offset + SILEAD_POINT_X_OFF));
+
+		data->pos[i].x = x;
+		data->pos[i].y = y;
+	}
+
+	input_mt_assign_slots(data->input, data->slots, data->pos, touch_nr, 0);
+
+	for (i = 0; i < touch_nr; i++) {
+		silead_ts_report_touch(data, data->pos[i].x, data->pos[i].y,
+				       data->slots[i]);
+
+		dev_dbg(dev, "x=%d y=%d hw_id=%d sw_id=%d\n", data->pos[i].x,
+			data->pos[i].y, data->id[i], data->slots[i]);
+	}
+
+	input_mt_sync_frame(data->input);
+	input_sync(data->input);
+}
+
+static int silead_ts_init(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	int error;
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
+					SILEAD_CMD_RESET);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_TOUCH_NR,
+					data->max_fingers);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
+					  SILEAD_CLOCK);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
+					SILEAD_CMD_START);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	return 0;
+
+i2c_write_err:
+	dev_err(&client->dev, "Registers clear error %d\n", error);
+	return error;
+}
+
+static int silead_ts_reset(struct i2c_client *client)
+{
+	int error;
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET,
+					SILEAD_CMD_RESET);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_CLOCK,
+					  SILEAD_CLOCK);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_POWER,
+					SILEAD_CMD_START);
+	if (error)
+		goto i2c_write_err;
+	usleep_range(SILEAD_CMD_SLEEP_MIN, SILEAD_CMD_SLEEP_MAX);
+
+	return 0;
+
+i2c_write_err:
+	dev_err(&client->dev, "Chip reset error %d\n", error);
+	return error;
+}
+
+static int silead_ts_startup(struct i2c_client *client)
+{
+	int error;
+
+	error = i2c_smbus_write_byte_data(client, SILEAD_REG_RESET, 0x00);
+	if (error) {
+		dev_err(&client->dev, "Startup error %d\n", error);
+		return error;
+	}
+	msleep(SILEAD_STARTUP_SLEEP);
+
+	return 0;
+}
+
+static int silead_ts_load_fw(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	unsigned int fw_size, i;
+	const struct firmware *fw;
+	struct silead_fw_data *fw_data;
+	int error;
+
+	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
+
+	if (data->custom_fw_name)
+		error = request_firmware(&fw, data->custom_fw_name, dev);
+	else
+		error = request_firmware(&fw, data->fw_name, dev);
+
+	if (error) {
+		dev_err(dev, "Firmware request error %d\n", error);
+		return error;
+	}
+
+	fw_size = fw->size / sizeof(*fw_data);
+	fw_data = (struct silead_fw_data *)fw->data;
+
+	for (i = 0; i < fw_size; i++) {
+		error = i2c_smbus_write_i2c_block_data(client,
+						       fw_data[i].offset,
+						       4,
+						       (u8 *)&fw_data[i].val);
+		if (error) {
+			dev_err(dev, "Firmware load error %d\n", error);
+			goto release_fw_err;
+		}
+	}
+
+	release_firmware(fw);
+	return 0;
+
+release_fw_err:
+	release_firmware(fw);
+	return error;
+}
+
+static u32 silead_ts_get_status(struct i2c_client *client)
+{
+	int error;
+	u32 status;
+
+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_STATUS, 4,
+					    (u8 *)&status);
+	if (error < 0) {
+		dev_err(&client->dev, "Status read error %d\n", error);
+		return error;
+	}
+
+	return le32_to_cpu(status);
+}
+
+static int silead_ts_get_id(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	int error;
+
+	error = i2c_smbus_read_i2c_block_data(client, SILEAD_REG_ID, 4,
+					    (u8 *)&data->chip_id);
+
+	data->chip_id = le32_to_cpu(data->chip_id);
+
+	if (error < 0) {
+		dev_err(&client->dev, "Chip ID read error %d\n", error);
+		return error;
+	}
+
+	return 0;
+}
+
+static int silead_ts_setup(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	int error;
+	u32 status;
+
+	silead_ts_set_power(client, SILEAD_POWER_OFF);
+	silead_ts_set_power(client, SILEAD_POWER_ON);
+
+	error = silead_ts_get_id(client);
+	if (error)
+		return error;
+	dev_info(dev, "Silead chip ID: 0x%8X", data->chip_id);
+
+	error = silead_ts_init(client);
+	if (error)
+		return error;
+
+	error = silead_ts_reset(client);
+	if (error)
+		return error;
+
+	error = silead_ts_load_fw(client);
+	if (error)
+		return error;
+
+	error = silead_ts_startup(client);
+	if (error)
+		return error;
+
+	status = silead_ts_get_status(client);
+	if (status != SILEAD_STATUS_OK) {
+		dev_err(dev, "Initialization error, status: 0x%X\n", status);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static irqreturn_t silead_ts_threaded_irq_handler(int irq, void *id)
+{
+	struct silead_ts_data *data = id;
+	struct i2c_client *client = data->client;
+
+	silead_ts_read_data(client);
+
+	return IRQ_HANDLED;
+}
+
+static int silead_ts_read_props(struct i2c_client *client)
+{
+	struct silead_ts_data *data = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	int error;
+
+	error = device_property_read_u32(dev, "touchscreen-size-x",
+					&data->x_max);
+	if (error) {
+		dev_dbg(dev, "Resolution X read error %d\n", error);
+		data->x_max = SILEAD_MAX_X;
+	}
+	data->x_max--; /* Property contains size not max */
+
+	error = device_property_read_u32(dev, "touchscreen-size-y",
+					&data->y_max);
+	if (error) {
+		dev_dbg(dev, "Resolution Y read error %d\n", error);
+		data->y_max = SILEAD_MAX_Y;
+	}
+	data->y_max--; /* Property contains size not max */
+
+	error = device_property_read_u32(dev, "touchscreen-max-fingers",
+					 &data->max_fingers);
+	if (error) {
+		dev_dbg(dev, "Max fingers read error %d\n", error);
+		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
+	}
+
+	error = device_property_read_string(dev, "touchscreen-fw-name",
+					  &data->custom_fw_name);
+	if (error)
+		dev_dbg(dev, "Firmware file name read error. Using default.");
+
+	data->x_invert = device_property_read_bool(dev,
+						"touchscreen-inverted-x");
+	data->y_invert = device_property_read_bool(dev,
+						"touchscreen-inverted-y");
+	data->xy_swap = device_property_read_bool(dev,
+						"touchscreen-swapped-x-y");
+
+	dev_dbg(dev, "x_max = %d, y_max = %d, max_fingers = %d, x_invert = %d, y_invert = %d, xy_swap = %d",
+		data->x_max, data->y_max, data->max_fingers, data->x_invert,
+		data->y_invert, data->xy_swap);
+
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
+					 const struct i2c_device_id *id)
+{
+	const struct acpi_device_id *acpi_id;
+	struct device *dev = &data->client->dev;
+	int i;
+
+	if (ACPI_HANDLE(dev)) {
+		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+		if (!acpi_id)
+			return -ENODEV;
+
+		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
+			acpi_id->id);
+
+		for (i = 0; i < strlen(data->fw_name); i++)
+			data->fw_name[i] = tolower(data->fw_name[i]);
+	} else {
+		snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw",
+			id->name);
+	}
+
+	return 0;
+}
+#else
+static int silead_ts_set_default_fw_name(struct silead_ts_data *data,
+					 const struct i2c_device_id *id)
+{
+	snprintf(data->fw_name, sizeof(data->fw_name), "%s.fw", id->name);
+	return 0;
+}
+#endif
+
+static int silead_ts_probe(struct i2c_client *client,
+			   const struct i2c_device_id *id)
+{
+	struct silead_ts_data *data;
+	struct device *dev = &client->dev;
+	int error;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_I2C |
+				     I2C_FUNC_SMBUS_READ_I2C_BLOCK |
+				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+		dev_err(dev, "I2C functionality check failed\n");
+		return -ENXIO;
+	}
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	error = silead_ts_set_default_fw_name(data, id);
+	if (error)
+		return error;
+
+	/* We must have the IRQ provided by DT or ACPI subsytem */
+	if (client->irq <= 0)
+		return -ENODEV;
+
+	/* Power GPIO pin */
+	data->gpio_power = devm_gpiod_get_index(dev, "power",
+						0, GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpio_power)) {
+		dev_dbg(dev, "Shutdown GPIO request failed\n");
+		data->gpio_power = NULL;
+	}
+
+	error = silead_ts_read_props(client);
+	if (error)
+		return error;
+
+	error = silead_ts_setup(client);
+	if (error)
+		return error;
+
+	error = silead_ts_request_input_dev(data);
+	if (error)
+		return error;
+
+	error = devm_request_threaded_irq(dev, client->irq, NULL,
+					silead_ts_threaded_irq_handler,
+					IRQF_ONESHOT | IRQ_TYPE_EDGE_RISING,
+					client->name, data);
+	if (error) {
+		dev_err(dev, "IRQ request failed %d\n", error);
+		return error;
+	}
+
+	dev_dbg(dev, "Probing succeded\n");
+	return 0;
+}
+
+static int __maybe_unused silead_ts_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+
+	silead_ts_set_power(client, SILEAD_POWER_OFF);
+	return 0;
+}
+
+static int __maybe_unused silead_ts_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int error, status;
+
+	silead_ts_set_power(client, SILEAD_POWER_ON);
+
+	error = silead_ts_reset(client);
+	if (error)
+		return error;
+
+	error = silead_ts_startup(client);
+	if (error)
+		return error;
+
+	status = silead_ts_get_status(client);
+	if (status != SILEAD_STATUS_OK) {
+		dev_err(dev, "Resume error, status: 0x%X\n", status);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(silead_ts_pm, silead_ts_suspend, silead_ts_resume);
+
+static const struct i2c_device_id silead_ts_id[] = {
+	{ "gsl1680", 0 },
+	{ "gsl1688", 0 },
+	{ "gsl3670", 0 },
+	{ "gsl3675", 0 },
+	{ "gsl3692", 0 },
+	{ "mssl1680", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, silead_ts_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id silead_ts_acpi_match[] = {
+	{ "GSL1680", 0 },
+	{ "GSL1688", 0 },
+	{ "GSL3670", 0 },
+	{ "GSL3675", 0 },
+	{ "GSL3692", 0 },
+	{ "MSSL1680", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, silead_ts_acpi_match);
+#endif
+
+static struct i2c_driver silead_ts_driver = {
+	.probe = silead_ts_probe,
+	.id_table = silead_ts_id,
+	.driver = {
+		.name = SILEAD_TS_NAME,
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(silead_ts_acpi_match),
+		.pm = &silead_ts_pm,
+	},
+};
+module_i2c_driver(silead_ts_driver);
+
+MODULE_AUTHOR("Robert Dolca <[e-mail]>");
+MODULE_DESCRIPTION("Silead I2C touchscreen driver");
+MODULE_LICENSE("GPL");