Input: add support for HiDeep touchscreen

The HiDeep touchscreen device is a capacitive multi-touch controller
mainly for multi-touch supported devices use. It use I2C interface for
communication to IC and provide axis X, Y, Z locations for ten finger
touch through input event interface to userspace.

It support the Crimson and the Lime two type IC. They are different
the number of channel supported and FW size. But the working protocol
is same.

Signed-off-by: Anthony Kim <anthony.kim@hideep.com>
---
 .../bindings/input/touchscreen/hideep.txt          |   39 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 drivers/input/touchscreen/Kconfig                  |   11 +
 drivers/input/touchscreen/Makefile                 |    1 +
 drivers/input/touchscreen/hideep_core.c            | 1294 ++++++++++++++++++++
 drivers/input/touchscreen/hideep_core.h            |  221 ++++
 6 files changed, 1567 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hideep.txt
 create mode 100644 drivers/input/touchscreen/hideep_core.c
 create mode 100644 drivers/input/touchscreen/hideep_core.h

On Tue, Aug 22, 2017 at 06:03:38PM +0900, Anthony Kim wrote:
> The HiDeep touchscreen device is a capacitive multi-touch controller
> mainly for multi-touch supported devices use. It use I2C interface for
> communication to IC and provide axis X, Y, Z locations for ten finger
> touch through input event interface to userspace.
> 
> It support the Crimson and the Lime two type IC. They are different
> the number of channel supported and FW size. But the working protocol
> is same.
> 
> Signed-off-by: Anthony Kim <anthony.kim@hideep.com>
> ---
>  .../bindings/input/touchscreen/hideep.txt          |   39 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/input/touchscreen/Kconfig                  |   11 +
>  drivers/input/touchscreen/Makefile                 |    1 +
>  drivers/input/touchscreen/hideep_core.c            | 1294 ++++++++++++++++++++
>  drivers/input/touchscreen/hideep_core.h            |  221 ++++
>  6 files changed, 1567 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hideep.txt
>  create mode 100644 drivers/input/touchscreen/hideep_core.c
>  create mode 100644 drivers/input/touchscreen/hideep_core.h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Anthony,

On Tue, Aug 22, 2017 at 06:03:38PM +0900, Anthony Kim wrote:
> The HiDeep touchscreen device is a capacitive multi-touch controller
> mainly for multi-touch supported devices use. It use I2C interface for
> communication to IC and provide axis X, Y, Z locations for ten finger
> touch through input event interface to userspace.
> 
> It support the Crimson and the Lime two type IC. They are different
> the number of channel supported and FW size. But the working protocol
> is same.
> 
> Signed-off-by: Anthony Kim <anthony.kim@hideep.com>
> ---
>  .../bindings/input/touchscreen/hideep.txt          |   39 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  drivers/input/touchscreen/Kconfig                  |   11 +
>  drivers/input/touchscreen/Makefile                 |    1 +
>  drivers/input/touchscreen/hideep_core.c            | 1294 ++++++++++++++++++++
>  drivers/input/touchscreen/hideep_core.h            |  221 ++++

You have only one .c file now. Maybe call it hideep.c and fold
hideep_core.h into it?

>  6 files changed, 1567 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/hideep.txt
>  create mode 100644 drivers/input/touchscreen/hideep_core.c
>  create mode 100644 drivers/input/touchscreen/hideep_core.h
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/hideep.txt b/Documentation/devicetree/bindings/input/touchscreen/hideep.txt
> new file mode 100644
> index 0000000..7a5b9a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/hideep.txt
> @@ -0,0 +1,39 @@
> +* HiDeep Finger and Stylus touchscreen controller
> +
> +Required properties:
> +- compatible		: must be "hideep,hideep-crimson"
> +					or "hideep,hideep-lime".
> +- reg			: I2C slave address, (e.g. 0x6C).
> +- interrupt-parent : Interrupt controller to which the chip is connected.
> +- interrupts : Interrupt to which the chip is connected.
> +
> +Optional properties:
> +- vdd-supply	: It is the controller supply for controlling
> +					 main voltage(3.3V) through the regulator.
> +- vid-supply	: It is the controller supply for controlling
> +					IO voltage(1.8V) through the regulator.
> +- reset-gpios	: Define for reset gpio pin.
> +						It is to use for reset IC.
> +- touchscreen-size-x	: X axis size of touchscreen
> +- touchscreen-size-y	: Y axis size of touchscreen
> +- touchkey-use	: Using touchkey in system.

Instead of a flah and a hard-coded keycodes, I'd prefer we used
optional "linux,keycodes" property for that.  See
drivers/input/keyboard/mpr121_touchkey.c in my 'next' branch for
example of parsing.

> +
> +Example:
> +
> +i2c@00000000 {
> +
> +	/* ... */
> +
> +	touchscreen@6c {
> +		compatible = "hideep,hideep-lime";
> +		reg = <0x6c>;
> +		interrupt-parent = <&gpx1>;
> +		interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&ldo15_reg>";
> +		vid-supply = <&ldo18_reg>;
> +		reset-gpios = <&gpx1 5 0>;
> +		touchscreen-size-x = 1079;
> +		touchscreen-size-y = 1919;
> +		touchkey-use;
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index c03d201..aa2a301 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -131,6 +131,7 @@ gw	Gateworks Corporation
>  hannstar	HannStar Display Corporation
>  haoyu	Haoyu Microelectronic Co. Ltd.
>  hardkernel	Hardkernel Co., Ltd
> +hideep	HiDeep Inc.
>  himax	Himax Technologies, Inc.
>  hisilicon	Hisilicon Limited.
>  hit	Hitachi Ltd.
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 64b30fe..13e11c7 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1246,4 +1246,15 @@ config TOUCHSCREEN_ROHM_BU21023
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bu21023_ts.
>  
> +config TOUCHSCREEN_HIDEEP
> +	tristate "HiDeep Touch IC"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a touchscreen using HiDeep.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a moudle, choose M here : the
> +	  module will be called hideep_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 6badce8..03ec3bb 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -103,3 +103,4 @@ obj-$(CONFIG_TOUCHSCREEN_ZET6223)	+= zet6223.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_HIDEEP) += hideep_core.o

Please try inserting in [loosely] alphabetical order.

> diff --git a/drivers/input/touchscreen/hideep_core.c b/drivers/input/touchscreen/hideep_core.c
> new file mode 100644
> index 0000000..1d5887d
> --- /dev/null
> +++ b/drivers/input/touchscreen/hideep_core.c
> @@ -0,0 +1,1294 @@
> +/*
> + * Copyright (C) 2012-2017 Hideep, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foudation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/firmware.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/sysfs.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <asm/unaligned.h>
> +
> +#include "hideep_core.h"
> +
> +static void hideep_reset_ic(struct hideep_ts *ts);
> +
> +static int hideep_pgm_w_mem(struct hideep_ts *ts, unsigned int addr,
> +	struct pgm_packet *packet, unsigned int len)
> +{
> +	int ret = 0;

No need to initialize here. If anything it simply masks use of
uninitialized variable. This applies to entire driver.

> +	int i;
> +
> +	if ((len % 4) != 0)
> +		return -1;

-EINVAL

> +
> +	mutex_lock(&ts->i2c_mutex);

Could you explain why you need this mute? I would expect you'd have a
higher-level mutexes, for example one protecting entire device during
firmware update, but need to protect individual i2c operations is not
clear to me.

> +
> +	put_unaligned_be32((0x80 | (len / 4 - 1)), &packet->header.w[0]);
> +	put_unaligned_be32(addr, &packet->header.w[1]);
> +
> +	for (i = 0; i < len / sizeof(unsigned int); i++)

sizeof(u32)

> +		put_unaligned_be32(packet->payload[i], &packet->payload[i]);
> +
> +	ret = i2c_master_send(ts->client, &packet->header.b[3],
> +		(len + 5));
> +
> +	if (ret < 0)
> +		goto err;
> +
> +err:
> +	mutex_unlock(&ts->i2c_mutex);
> +	return ret;
> +}
> +
> +static int hideep_pgm_r_mem(struct hideep_ts *ts, unsigned int addr,
> +	struct pgm_packet *packet, unsigned int len)
> +{
> +	int ret = 0;
> +	int i;
> +	unsigned char buff[len];
> +
> +	if ((len % 4) != 0)
> +		return -1;
> +
> +	mutex_lock(&ts->i2c_mutex);
> +
> +	put_unaligned_be32((0x00 | (len / 4 - 1)), &packet->header.w[0]);
> +	put_unaligned_be32(addr, &packet->header.w[1]);
> +
> +	ret = i2c_master_send(ts->client, &packet->header.b[3], 5);
> +
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = i2c_master_recv(ts->client, buff,	len);
> +
> +	if (ret < 0)
> +		goto err;

Does this have to be 2 separate transactions?

> +
> +	for (i = 0; i < len / 4; i++)

len / sizeof(u32)

> +		packet->payload[i] = get_unaligned_be32(&buff[i * 4]);

i * sizeof(u32)

> +
> +err:
> +	mutex_unlock(&ts->i2c_mutex);
> +	return ret;
> +}
> +
> +static int hideep_pgm_r_reg(struct hideep_ts *ts, unsigned int addr,
> +	unsigned int *val)
> +{
> +	int ret = 0;
> +	struct pgm_packet packet;
> +
> +	put_unaligned_be32(0x00, &packet.header.w[0]);
> +	put_unaligned_be32(addr, &packet.header.w[1]);
> +
> +	ret = hideep_pgm_r_mem(ts, addr, &packet, 4);
> +
> +	if (ret < 0)
> +		goto err;
> +
> +	*val = packet.payload[0];
> +
> +err:
> +	return ret;
> +}
> +
> +static int hideep_pgm_w_reg(struct hideep_ts *ts, unsigned int addr,
> +	unsigned int data)
> +{
> +	int ret = 0;
> +	struct pgm_packet packet;
> +
> +	put_unaligned_be32(0x80, &packet.header.w[0]);
> +	put_unaligned_be32(addr, &packet.header.w[1]);
> +	packet.payload[0] = data;
> +
> +	ret = hideep_pgm_w_mem(ts, addr, &packet, 4);
> +
> +	return ret;
> +}
> +
> +#define SW_RESET_IN_PGM(CLK) \
> +{ \
> +	hideep_pgm_w_reg(ts, SYSCON_WDT_CNT, CLK); \
> +	hideep_pgm_w_reg(ts, SYSCON_WDT_CON, 0x03); \
> +	hideep_pgm_w_reg(ts, SYSCON_WDT_CON, 0x01); \
> +}
> +
> +#define SET_FLASH_PIO(CE) \
> +	hideep_pgm_w_reg(ts, FLASH_CON, 0x01 | ((CE) << 1))
> +#define SET_PIO_SIG(X, Y) \
> +	hideep_pgm_w_reg(ts, FLASH_BASE + PIO_SIG + (X), Y)
> +#define SET_FLASH_HWCONTROL() \
> +	hideep_pgm_w_reg(ts, FLASH_CON, 0x00000000)
> +
> +#define NVM_W_SFR(x, y) \
> +{ \
> +	SET_FLASH_PIO(1); \
> +	SET_PIO_SIG(x, y); \
> +	SET_FLASH_PIO(0); \
> +}
> +
> +static void get_dwz_from_binary(unsigned char *pres,
> +	struct dwz_info *dwz_info)
> +{
> +	memcpy(dwz_info, pres + HIDEEP_DWZ_INFO_OFS,
> +		sizeof(struct dwz_info));
> +}
> +
> +static void hideep_sw_reset(struct hideep_ts *ts, unsigned int food)
> +{
> +	SW_RESET_IN_PGM(food);
> +}
> +
> +static int hideep_enter_pgm(struct hideep_ts *ts)
> +{
> +	int ret = 0;
> +	int retry_count = 10;
> +	int retry = 0;
> +	unsigned int status;
> +	unsigned int pattern = 0xDF9DAF39;
> +
> +	while (retry < retry_count) {
> +		i2c_master_send(ts->client, (unsigned char *)&pattern, 4);

Error handling?


> +		mdelay(1);
> +
> +		/* flush invalid Tx load register */
> +		hideep_pgm_w_reg(ts, ESI_TX_INVALID, 0x01);

And here?

> +
> +		hideep_pgm_r_reg(ts, SYSCON_PGM_ID, &status);

And here...

> +
> +		if (pattern != get_unaligned_be32(&status)) {
> +			retry++;
> +			dev_err(&ts->client->dev, "enter_pgm : error(%08x):",
> +				get_unaligned_be32(&status));
> +		} else {
> +			dev_dbg(&ts->client->dev, "found magic code");
> +			break;
> +		}
> +	}
> +
> +	if (retry < retry_count) {
> +		hideep_pgm_w_reg(ts, SYSCON_WDT_CON, 0x00);
> +		hideep_pgm_w_reg(ts, SYSCON_SPC_CON, 0x00);
> +		hideep_pgm_w_reg(ts, SYSCON_CLK_ENA, 0xFF);
> +		hideep_pgm_w_reg(ts, SYSCON_CLK_CON, 0x01);
> +		hideep_pgm_w_reg(ts, SYSCON_PWR_CON, 0x01);
> +		hideep_pgm_w_reg(ts, FLASH_TIM, 0x03);
> +		hideep_pgm_w_reg(ts, FLASH_CACHE_CFG, 0x00);
> +		hideep_pgm_w_reg(ts, FLASH_CACHE_CFG, 0x02);
> +
> +		mdelay(1);
> +	} else {
> +		ret = -1;
> +	}
> +
> +	return ret;
> +}
> +static void hideep_nvm_unlock(struct hideep_ts *ts)
> +{
> +	unsigned int unmask_code = 0;
> +
> +	hideep_pgm_w_reg(ts, FLASH_CFG, NVM_SFR_RPAGE);
> +
> +	hideep_pgm_r_reg(ts, 0x0000000C, &unmask_code);
> +
> +	hideep_pgm_w_reg(ts, FLASH_CFG, NVM_DEFAULT_PAGE);
> +
> +	/* make it unprotected code */
> +	unmask_code &= (~_PROT_MODE);
> +
> +	/* compare unmask code */
> +	if (unmask_code != ts->nvm_mask)
> +		dev_dbg(&ts->client->dev, "read mask code different 0x%x",
> +			unmask_code);
> +
> +	hideep_pgm_w_reg(ts, FLASH_CFG, NVM_SFR_WPAGE);
> +	SET_FLASH_PIO(0);
> +
> +	NVM_W_SFR(NVM_MASK_OFS, ts->nvm_mask);
> +	SET_FLASH_HWCONTROL();
> +	hideep_pgm_w_reg(ts, FLASH_CFG, NVM_DEFAULT_PAGE);
> +}
> +
> +static int hideep_program_page(struct hideep_ts *ts,
> +	unsigned int addr, struct pgm_packet *packet_w)
> +{
> +	int ret = 0;
> +	unsigned int pio_cmd = WRONLY;
> +	unsigned int pio_cmd_page_erase = PERASE;
> +	unsigned int status;
> +	int time_out = 0;
> +	unsigned int end_flag = 124;
> +
> +	hideep_pgm_r_reg(ts, FLASH_STA, &status);
> +	ret = (status == 0) ? -1:0;
> +
> +	addr = addr & ~(NVM_PAGE_SIZE - 1);
> +
> +	SET_FLASH_PIO(0);
> +	SET_FLASH_PIO(1);
> +
> +	/* first erase */
> +	SET_PIO_SIG(pio_cmd_page_erase  + addr, 0xFFFFFFFF);
> +
> +	SET_FLASH_PIO(0);
> +	time_out = 0;
> +
> +	while (1) {
> +		mdelay(1);
> +		hideep_pgm_r_reg(ts, FLASH_STA, &status);
> +		if ((status) != 0)
> +			break;
> +		if (time_out++ > 100)
> +			break;
> +	}
> +	SET_FLASH_PIO(1);
> +	/* first erase end*/
> +
> +	SET_PIO_SIG(pio_cmd + addr, get_unaligned_be32(&packet_w->payload[0]));
> +
> +	hideep_pgm_w_mem(ts, (FLASH_BASE + 0x400000) + pio_cmd,
> +		packet_w, NVM_PAGE_SIZE);
> +
> +	SET_PIO_SIG(end_flag, get_unaligned_be32(&packet_w->payload[31]));
> +
> +	SET_FLASH_PIO(0);
> +
> +	mdelay(1);
> +
> +	while (1) {
> +		hideep_pgm_r_reg(ts, FLASH_STA, &status);
> +		if ((status) != 0)
> +			break;
> +	}
> +	/* write routine end */
> +
> +	SET_FLASH_HWCONTROL();
> +
> +	return ret;
> +}
> +
> +static int hideep_program_nvm(struct hideep_ts *ts, const unsigned char *ucode,
> +	int len, int offset, unsigned char *old_fw)
> +{
> +	int i;
> +	int ret = 0;
> +	int len_r;
> +	int len_w;
> +	int addr = 0;
> +	unsigned int pages;
> +
> +	struct pgm_packet packet_w;
> +
> +	ret = hideep_enter_pgm(ts);

What if the above returns error?

> +
> +	hideep_nvm_unlock(ts);
> +
> +	pages = (len + NVM_PAGE_SIZE - 1) / NVM_PAGE_SIZE;

DIV_ROUND_UP()

> +	addr = offset;
> +	len_r = len;
> +	len_w = len_r;
> +
> +	dev_dbg(&ts->client->dev, "pages : %d", pages);
> +	for (i = 0; i < pages; i++) {
> +		if (len_r >= NVM_PAGE_SIZE)
> +			len_w = NVM_PAGE_SIZE;
> +
> +		/* compare */
> +		if (old_fw != NULL)

		if (!old_fw)

> +			ret = memcmp(&ucode[addr], &old_fw[addr], len_w);
> +
> +		if (ret != 0 || old_fw == NULL) {
> +			/* write page */
> +			memcpy(packet_w.payload, &(ucode[addr]), len_w);

No need for parentheses around ucode[addr].

> +
> +			ret = hideep_program_page(ts, addr, &packet_w);
> +			mdelay(1);
> +			if (ret < 0)
> +				dev_err(&ts->client->dev,
> +					"hideep_program_nvm : error(%08x):",
> +					addr);
> +		}
> +
> +		addr += NVM_PAGE_SIZE;
> +		len_r -= NVM_PAGE_SIZE;
> +		len_w = len_r;
> +	}
> +
> +	return ret;
> +}
> +
> +static int hideep_verify_nvm(struct hideep_ts *ts, const unsigned char *ucode,
> +	int len, int offset)
> +{
> +	int i, j;
> +	int ret = 0;
> +	unsigned char page_chk = 0;
> +	unsigned int addr = offset;
> +	unsigned int pages = (len + NVM_PAGE_SIZE - 1) / NVM_PAGE_SIZE;
> +	int len_r = len;
> +	int len_v = len_r;
> +
> +	struct pgm_packet packet_r;
> +
> +	for (i = 0; i < pages; i++) {
> +		if (len_r >= NVM_PAGE_SIZE)
> +			len_v = NVM_PAGE_SIZE;
> +
> +		hideep_pgm_r_mem(ts, 0x00000000 + addr, &packet_r,
> +			NVM_PAGE_SIZE);
> +
> +		page_chk = memcmp(&(ucode[addr]), packet_r.payload, len_v);
> +
> +		if (page_chk != 0) {
> +			u8 *read = (u8 *)packet_r.payload;
> +
> +			for (j = 0; j < NVM_PAGE_SIZE; j++)
> +				dev_err(&ts->client->dev, "%02x : %02x",
> +						ucode[addr+j], read[j]);
> +
> +			dev_err(&ts->client->dev, "verify : error(addr : %d)",
> +				addr);
> +
> +			ret = -1;
> +		}
> +
> +		addr += NVM_PAGE_SIZE;
> +		len_r -= NVM_PAGE_SIZE;
> +		len_v = len_r;
> +	}
> +
> +	return ret;
> +}
> +
> +static void hideep_read_nvm(struct hideep_ts *ts, unsigned char *data, int len,
> +	int offset)
> +{
> +	int ret = 0;
> +	int pages, i;
> +	int len_r, len_v;
> +	int addr = offset;
> +
> +	struct pgm_packet packet_r;
> +
> +	pages = (len + NVM_PAGE_SIZE - 1) / NVM_PAGE_SIZE;
> +	len_r = len;
> +	len_v = len_r;
> +
> +	hideep_reset_ic(ts);
> +
> +	ret = hideep_enter_pgm(ts);
> +
> +	hideep_nvm_unlock(ts);
> +
> +	for (i = 0; i < pages; i++) {
> +		if (len_r >= NVM_PAGE_SIZE)
> +			len_v = NVM_PAGE_SIZE;
> +
> +		hideep_pgm_r_mem(ts, 0x00000000 + addr, &packet_r,
> +			NVM_PAGE_SIZE);
> +
> +		memcpy(&(data[i * NVM_PAGE_SIZE]), &packet_r.payload[0], len_v);
> +
> +		addr += NVM_PAGE_SIZE;
> +		len_r -= NVM_PAGE_SIZE;
> +		len_v = len_r;
> +	}
> +
> +	hideep_sw_reset(ts, 1000);
> +}
> +
> +static int hideep_fw_verify_run(struct hideep_ts *ts, unsigned char *fw,
> +	size_t len, int offset)
> +{
> +	int ret = 0;
> +	int retry = 3;
> +
> +	while (retry--) {
> +		ret = hideep_verify_nvm(ts, fw, len, offset);
> +		if (ret == 0) {
> +			dev_dbg(&ts->client->dev, "update success");
> +			break;
> +		}
> +		dev_err(&ts->client->dev, "download fw failed(%d)", retry);
> +	}
> +
> +	ret = (retry == 0) ? -1:0;
> +
> +	return ret;
> +}
> +
> +static int hideep_wr_firmware(struct hideep_ts *ts, unsigned char *code,
> +	int len, int offset, bool mode)
> +{
> +	int ret = 0;
> +	int firm_len;
> +	unsigned char *ic_fw;
> +	unsigned char *dwz_info;
> +
> +	ic_fw = kmalloc(ts->fw_size, GFP_KERNEL);
> +	dwz_info = kmalloc(sizeof(struct dwz_info) + 64, GFP_KERNEL);

Error handing.

> +
> +	memset(dwz_info, 0x0, sizeof(struct dwz_info) + 64);
> +
> +	firm_len = len;
> +	dev_dbg(&ts->client->dev, "fw len : %d, define size : %d",
> +		firm_len, ts->fw_size);
> +
> +	if (firm_len > ts->fw_size)
> +		firm_len = ts->fw_size;
> +
> +	dev_dbg(&ts->client->dev, "enter");
> +	/* memory dump of target IC */
> +	hideep_read_nvm(ts, ic_fw, firm_len, offset);
> +	/* comparing & programming each page, if the memory of specified
> +	 * page is exactly same, no need to update.
> +	 */
> +	ret = hideep_program_nvm(ts, code, firm_len, offset, ic_fw);
> +
> +	hideep_fw_verify_run(ts, code, firm_len, offset);
> +	if (ret < 0) {
> +		if (mode == true) {
> +			/* clear dwz version, it will be store once again
> +			 * after update success
> +			 */
> +			ts->dwz_info.release_ver = 0;
> +			memcpy(&dwz_info[64], &ts->dwz_info,
> +				sizeof(struct dwz_info));
> +			ret = hideep_program_nvm(ts, dwz_info, HIDEEP_DWZ_LEN,
> +				0x280, NULL);
> +		}
> +	}
> +
> +	get_dwz_from_binary(code, &ts->dwz_info);
> +
> +	hideep_sw_reset(ts, 1000);
> +
> +	kfree(ic_fw);
> +	kfree(dwz_info);
> +
> +	return ret;
> +}
> +
> +static int hideep_update_firmware(struct hideep_ts *ts, const char *fn)
> +{
> +	int ret = 0;
> +	const struct firmware *fw_entry;
> +	unsigned char *fw_buf;
> +	unsigned int fw_length;
> +
> +	dev_dbg(&ts->client->dev, "enter");
> +	ret = request_firmware(&fw_entry, fn, &ts->client->dev);
> +
> +	if (ret != 0) {
> +		dev_err(&ts->client->dev, "request_firmware : fail(%d)", ret);
> +		return ret;
> +	}
> +
> +	fw_buf = (unsigned char *)fw_entry->data;
> +	fw_length = (unsigned int)fw_entry->size;
> +
> +	/* chip specific code for flash fuse */
> +	mutex_lock(&ts->dev_mutex);
> +
> +	ts->dev_state = state_updating;
> +
> +	ret = hideep_wr_firmware(ts, fw_buf, fw_length, 0, true);
> +
> +	ts->dev_state = state_normal;
> +
> +	mutex_unlock(&ts->dev_mutex);
> +
> +	release_firmware(fw_entry);
> +
> +	return ret;
> +}
> +
> +static int hideep_load_dwz(struct hideep_ts *ts)
> +{
> +	int ret = 0;
> +	struct pgm_packet packet_r;
> +
> +	ret = hideep_enter_pgm(ts);
> +	if (ret < 0)
> +		return ret;
> +
> +	mdelay(50);
> +
> +	ret = hideep_pgm_r_mem(ts, HIDEEP_DWZ_INFO_OFS, &packet_r,
> +		sizeof(struct dwz_info));
> +
> +	memcpy(&ts->dwz_info, packet_r.payload,
> +		sizeof(struct dwz_info));
> +	hideep_sw_reset(ts, 10);
> +
> +	if (get_unaligned_le16(&ts->dwz_info.product_code) & 0x40) {
> +		/* Crimson IC */
> +		ts->fw_size = 1024 * 48;
> +		ts->nvm_mask = 0x00310000;
> +	} else {
> +		/* Lime fw size */
> +		ts->fw_size = 1024 * 64;
> +		ts->nvm_mask = 0x0030027B;
> +	}
> +
> +	dev_dbg(&ts->client->dev, "firmware release version : %04x",
> +		get_unaligned_le16(&ts->dwz_info.release_ver));
> +
> +	mdelay(50);
> +
> +	return ret;
> +}
> +
> +static int hideep_i2c_read(struct hideep_ts *ts, unsigned short addr,
> +	unsigned short len, unsigned char *buf)
> +{
> +	int ret = -1;
> +	unsigned short r_len, raddr;
> +	int length;
> +
> +	length = len;
> +	raddr = addr;
> +
> +	dev_dbg(&ts->client->dev, "addr = 0x%02x, len = %d", addr, len);
> +
> +	mutex_lock(&ts->i2c_mutex);
> +
> +	do {
> +		if (length > MAX_I2C_BUFFER_SIZE)
> +			r_len = MAX_I2C_BUFFER_SIZE;
> +		else
> +			r_len = length;
> +
> +		dev_dbg(&ts->client->dev, "addr = 0x%02x, len = %d",
> +			raddr, r_len);
> +
> +		ret = i2c_master_send(ts->client, (char *)&raddr, 2);
> +
> +		if (ret < 0)
> +			goto i2c_err;
> +
> +		ret = i2c_master_recv(ts->client, (char *)buf, r_len);

Can it be a single i2c_transfer() with 2 messages?

> +		length -= MAX_I2C_BUFFER_SIZE;
> +		buf += MAX_I2C_BUFFER_SIZE;
> +		raddr += MAX_I2C_BUFFER_SIZE;
> +
> +		if (ret < 0)
> +			goto i2c_err;
> +	} while (length > 0);
> +
> +	mutex_unlock(&ts->i2c_mutex);
> +
> +	return  0;
> +i2c_err:
> +	mutex_unlock(&ts->i2c_mutex);
> +	return -1;
> +}
> +
> +static int hideep_i2c_write(struct hideep_ts *ts, unsigned short addr,
> +	unsigned short len, unsigned char *buf)
> +{
> +	int ret = -1;
> +	unsigned char data[len + 2];

How big is len? It is not a good style to have potentially unbound
variables on stack. Could we allocate the buffer or use scratch buffer?

> +
> +	dev_dbg(&ts->client->dev, "addr = 0x%02x, len = %d", addr, len);
> +
> +	mutex_lock(&ts->i2c_mutex);
> +
> +	// data mangling..
> +	data[0] = (addr >> 0) & 0xFF;
> +	data[1] = (addr >> 8) & 0xFF;

	put_unaligned_le16().

> +	memcpy(&data[2], buf, len);
> +
> +	ret = i2c_master_send(ts->client, data, len + 2);
> +
> +	if (ret < 0)
> +		goto i2c_err;
> +
> +	mutex_unlock(&ts->i2c_mutex);
> +	return  0;
> +
> +i2c_err:
> +	mutex_unlock(&ts->i2c_mutex);
> +	return -1;

Why do you clobber return value of i2c_master_send() with -1?

> +}
> +
> +static void hideep_reset_ic(struct hideep_ts *ts)
> +{
> +	unsigned char cmd = 0x01;
> +
> +	if (!IS_ERR(ts->reset_gpio)) {

You should check for NULL for optional GPIO, not error. Error would
result in probe() aborting.

> +		dev_dbg(&ts->client->dev, "hideep:enable the reset_gpio");
> +		gpiod_set_value(ts->reset_gpio, 0);
> +		mdelay(20);
> +		gpiod_set_value(ts->reset_gpio, 1);

This seems inverted. You want to activate GPIO, wait, then release it.
The actual polarity is handled by GPIOLIB, here we are dealing with
logical state of GPIO.

> +	} else {
> +		hideep_i2c_write(ts, HIDEEP_RESET_CMD, 1, &cmd);
> +	}
> +	mdelay(50);
> +}
> +
> +static int hideep_pwr_on(struct hideep_ts *ts)
> +{
> +	int ret = 0;
> +
> +	if (!IS_ERR(ts->vcc_vdd)) {

You do not need this check. If there was an error getting regulator
probe() would have aborted.

> +		dev_dbg(&ts->client->dev, "hideep:vcc_vdd is enable");
> +		ret = regulator_enable(ts->vcc_vdd);
> +		if (ret)
> +			dev_err(&ts->client->dev,
> +				"Regulator vdd enable failed ret=%d", ret);
> +	}
> +	usleep_range(999, 1000);
> +
> +	if (!IS_ERR(ts->vcc_vid)) {

Same here.

> +		dev_dbg(&ts->client->dev, "hideep:vcc_vid is enable");
> +		ret = regulator_enable(ts->vcc_vid);
> +		if (ret)
> +			dev_err(&ts->client->dev,
> +				"Regulator vcc_vid enable failed ret=%d", ret);
> +	}
> +	usleep_range(2999, 3000);
> +
> +	return ret;
> +}
> +
> +static void hideep_pwr_off(void *data)
> +{
> +	struct hideep_ts *ts = data;
> +
> +	if (!IS_ERR(ts->reset_gpio))
> +		gpiod_set_value(ts->reset_gpio, 0);
> +
> +	if (!IS_ERR(ts->vcc_vid))
> +		regulator_disable(ts->vcc_vid);
> +
> +	if (!IS_ERR(ts->vcc_vdd))
> +		regulator_disable(ts->vcc_vdd);
> +}
> +
> +#define __GET_MT_TOOL_TYPE(X) ((X == 0x01) ? MT_TOOL_FINGER : MT_TOOL_PEN)
> +
> +static void pops_mt(struct hideep_ts *ts)
> +{
> +	int id;
> +	int i;
> +	int offset = sizeof(struct hideep_event);
> +	struct hideep_event *event;
> +
> +	for (i = 0; i < ts->tch_count; i++) {
> +		event = (struct hideep_event *)&ts->touch_event[i * offset];
> +		id = (event->index >> 0) & 0x0F;
> +		input_mt_slot(ts->input_dev, id);
> +		input_mt_report_slot_state(ts->input_dev,
> +			__GET_MT_TOOL_TYPE(event->type), false);
> +		input_report_key(ts->input_dev, BTN_TOUCH, false);

I do not think you need to report release of BTN_TOUCH explicitly. In
fact, I believe it is wrong as one of contacts can still be active.

> +	}
> +}
> +
> +static void push_mt(struct hideep_ts *ts)
> +{
> +	int id;
> +	int i;
> +	bool btn_up = 0;
> +	bool btn_dn = 0;
> +	bool btn_mv = 0;
> +	int evt = 0;
> +	int offset = sizeof(struct hideep_event);
> +	struct hideep_event *event;
> +
> +	/* load multi-touch event to input system */
> +	for (i = 0; i < ts->tch_count; i++) {
> +		event = (struct hideep_event *)&ts->touch_event[i * offset];
> +		id = (event->index >> 0) & 0x0F;
> +		btn_up = (event->flag >> HIDEEP_MT_RELEASED) & 0x01;
> +		btn_dn = (event->flag >> HIDEEP_MT_FIRST_CONTACT)
> +			& 0x01;
> +		btn_mv = (event->flag >> HIDEEP_MT_DRAG_MOVE) & 0x01;
> +
> +		if (btn_up)
> +			clear_bit(id, &ts->tch_bit);
> +		else
> +			__set_bit(id, &ts->tch_bit);
> +
> +		dev_dbg(&ts->client->dev,
> +			"type = %d, id = %d, i = %d, x = %d, y = %d, z = %d",
> +			event->type, event->index, i,
> +			get_unaligned_le16(&event->x),
> +			get_unaligned_le16(&event->y),
> +			get_unaligned_le16(&event->z));
> +
> +		input_mt_slot(ts->input_dev, id);
> +		input_mt_report_slot_state(ts->input_dev,
> +			__GET_MT_TOOL_TYPE(event->type),
> +			(btn_up == 0));
> +
> +		if (btn_up == 0) {
> +			input_report_abs(ts->input_dev, ABS_MT_POSITION_X,
> +				get_unaligned_le16(&event->x));
> +			input_report_abs(ts->input_dev, ABS_MT_POSITION_Y,
> +				get_unaligned_le16(&event->y));
> +			input_report_abs(ts->input_dev, ABS_MT_PRESSURE,
> +				get_unaligned_le16(&event->z));
> +			input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
> +				event->w);
> +			evt++;
> +		}
> +	}
> +
> +	if (ts->tch_bit == 0)
> +		evt = 0;
> +
> +	input_report_key(ts->input_dev, BTN_TOUCH, evt);

You do not need to emit BTN_TOUCH yourself, input_mt_sync_frame() calls
input_mt_report_pointer_emulation(), which will do that for you.

> +	input_mt_sync_frame(ts->input_dev);
> +}
> +
> +static void pops_ky(struct hideep_ts *ts)
> +{
> +	input_report_key(ts->input_dev, KEY_HOME, false);
> +	input_report_key(ts->input_dev, KEY_MENU, false);
> +	input_report_key(ts->input_dev, KEY_BACK, false);
> +}
> +
> +static void push_ky(struct hideep_ts *ts)
> +{
> +	int i;
> +	int pressed;
> +	int key;
> +	int status;
> +	int code;
> +
> +	for (i = 0; i < ts->key_count; i++) {
> +		key = ts->key_event[i + i * 2] & 0x0F;
> +		status = ts->key_event[i + i * 2] & 0xF0;
> +		code = 0;
> +		pressed = false;
> +
> +		if (status & HIDEEP_KEY_PRESSED_MASK)
> +			pressed = true;
> +		else
> +			pressed = false;
> +
> +		switch (key) {
> +		case 0:
> +			code = KEY_HOME;
> +			break;
> +		case 1:
> +			code = KEY_MENU;
> +			break;
> +		case 2:
> +			code = KEY_BACK;
> +			break;
> +		}
> +		input_report_key(ts->input_dev, code, pressed);

Just use
		input_report_key(ts->input_dev, code,
				 status & HIDEEP_KEY_PRESSED_MASK);

as it will normalize the value (to 0/1) for you.

> +	}
> +}
> +
> +static void hideep_put_event(struct hideep_ts *ts)
> +{
> +	/* mangling touch information */
> +	if (ts->tch_count > 0)
> +		push_mt(ts);
> +
> +	if (ts->key_count > 0)
> +		push_ky(ts);
> +
> +	input_sync(ts->input_dev);
> +}
> +
> +static int hideep_get_event(struct hideep_ts *ts)
> +{
> +	int ret;
> +	int touch_count;
> +	int event_size;
> +
> +	/* get touch event count */
> +	dev_dbg(&ts->client->dev, "mt = %d, key = %d, lpm = %02x",
> +		ts->tch_count, ts->key_count, ts->lpm_count);
> +
> +	/* get touch event information */
> +	if (ts->tch_count > HIDEEP_MT_MAX)
> +		ts->tch_count = 0;
> +
> +	if (ts->key_count > HIDEEP_KEY_MAX)
> +		ts->key_count = 0;
> +
> +	touch_count = ts->tch_count + ts->key_count;
> +
> +	if (ts->tch_count > 0) {
> +		event_size = ts->tch_count *
> +			sizeof(struct hideep_event);
> +		ret = hideep_i2c_read(ts, HIDEEP_TOUCH_DATA_ADDR,
> +			event_size, ts->touch_event);
> +
> +		if (ret < 0) {
> +			dev_err(&ts->client->dev, "read I2C error.");
> +			return -1;

Do not clobber return values, pass them on to the higher layers.

> +		}
> +	}
> +
> +	if (ts->key_count > 0) {
> +		event_size = ts->key_count * 2;
> +		ret = hideep_i2c_read(ts, HIDEEP_KEY_DATA_ADDR,
> +			event_size, ts->key_event);
> +		if (ret < 0) {
> +			dev_err(&ts->client->dev, "read I2C error.");
> +			return -1;
> +		}
> +	}
> +
> +	return touch_count;
> +}
> +
> +static irqreturn_t hideep_irq_task(int irq, void *handle)
> +{
> +	unsigned char buff[2];
> +	int ret;
> +
> +	struct hideep_ts *ts = (struct hideep_ts *) handle;

No need to cast void pointers.

> +
> +	dev_dbg(&ts->client->dev, "state = 0x%x", ts->dev_state);
> +
> +	if (ts->dev_state == state_normal) {
> +		ret = hideep_i2c_read(ts, HIDEEP_EVENT_COUNT_ADDR,
> +			2, buff);
> +		if (ret < 0) {
> +			disable_irq(ts->client->irq);
> +			ts->interrupt_state = 0;
> +			return IRQ_HANDLED;
> +		}
> +
> +		ts->tch_count = buff[0];
> +		ts->key_count = buff[1] & 0x0f;
> +		ts->lpm_count = buff[1] & 0xf0;
> +
> +		ret = hideep_get_event(ts);
> +
> +		if (ret >= 0)
> +			hideep_put_event(ts);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int hideep_capability(struct hideep_ts *ts)
> +{
> +	int ret;
> +
> +	ts->input_dev->name = HIDEEP_TS_NAME;
> +	ts->input_dev->id.bustype = BUS_I2C;
> +
> +	input_set_capability(ts->input_dev, EV_KEY, BTN_TOUCH);

Does not need to be set explicitly, since you are using
input_mt_init_slots() with INPUT_MT_DIRECT.

> +
> +	if (ts->key_use) {
> +		input_set_capability(ts->input_dev, EV_KEY, KEY_HOME);
> +		input_set_capability(ts->input_dev, EV_KEY, KEY_MENU);
> +		input_set_capability(ts->input_dev, EV_KEY, KEY_BACK);
> +	}
> +
> +	input_set_abs_params(ts->input_dev, ABS_X, 0, ts->prop.max_x, 0, 0);
> +	input_set_abs_params(ts->input_dev, ABS_Y, 0, ts->prop.max_y, 0, 0);
> +	input_set_abs_params(ts->input_dev, ABS_PRESSURE, 0,
> +		65535, 0, 0);
> +
> +	ret = input_mt_init_slots(ts->input_dev,
> +		HIDEEP_MT_MAX, INPUT_MT_DIRECT);
> +
> +	if (ret)
> +		return ret;
> +
> +	input_set_abs_params(ts->input_dev,
> +		ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> +	input_set_abs_params(ts->input_dev,
> +		ABS_MT_POSITION_X, 0, ts->prop.max_x, 0, 0);
> +	input_set_abs_params(ts->input_dev,
> +		ABS_MT_POSITION_Y, 0, ts->prop.max_y, 0, 0);
> +	input_set_abs_params(ts->input_dev,
> +		ABS_MT_PRESSURE, 0, 65535, 0, 0);
> +	input_set_abs_params(ts->input_dev,
> +		ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);


Please set the MT parameters first and then call input_mt_init_slots().
Do not set ST parameters explicitly as input_mt_init_slots() will do
that for you.

> +
> +	return 0;
> +}
> +
> +static void hideep_get_info(struct hideep_ts *ts)
> +{
> +	unsigned char val[4];
> +
> +	if (ts->prop.max_x == 0 || ts->prop.max_y == 0) {
> +		hideep_i2c_read(ts, 0x28, 4, val);
> +
> +		ts->prop.max_x = get_unaligned_le16(&val[2]);
> +		ts->prop.max_y = get_unaligned_le16(&val[0]);
> +
> +		dev_info(&ts->client->dev, "X : %d, Y : %d",
> +			ts->prop.max_x, ts->prop.max_y);
> +	}
> +}
> +
> +static ssize_t hideep_update_fw(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct hideep_ts *ts = dev_get_drvdata(dev);
> +	int mode, ret;
> +	char *fw_name;
> +
> +	ret = kstrtoint(buf, 8, &mode);
> +	if (ret)
> +		return ret;
> +
> +	if (mode == 1) {

Why do we check the value? Any write should do.

> +		disable_irq(ts->client->irq);
> +
> +		ts->dev_state = state_updating;
> +		fw_name = kasprintf(GFP_KERNEL, "hideep_ts_%04x.bin",
> +			get_unaligned_le16(&ts->dwz_info.product_id));
> +		ret = hideep_update_firmware(ts, fw_name);
> +
> +		kfree(fw_name);
> +
> +		enable_irq(ts->client->irq);
> +
> +		ts->dev_state = state_normal;
> +		if (ret != 0)
> +			dev_err(dev, "The firmware update failed(%d)", ret);
> +	}
> +
> +	return count;
> +}
> +
> +static ssize_t hideep_fw_version_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	int len = 0;
> +	struct hideep_ts *ts = dev_get_drvdata(dev);
> +
> +	dev_info(dev, "release version : %04x",
> +		get_unaligned_le16(&ts->dwz_info.release_ver));
> +
> +	mutex_lock(&ts->dev_mutex);
> +	len = scnprintf(buf, PAGE_SIZE,
> +		"%04x\n", get_unaligned_le16(&ts->dwz_info.release_ver));
> +	mutex_unlock(&ts->dev_mutex);
> +
> +	return len;
> +}
> +
> +static ssize_t hideep_product_id_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	int len = 0;
> +	struct hideep_ts *ts = dev_get_drvdata(dev);
> +
> +	dev_info(dev, "product id : %04x",
> +		get_unaligned_le16(&ts->dwz_info.product_id));
> +
> +	mutex_lock(&ts->dev_mutex);
> +	len = scnprintf(buf, PAGE_SIZE,
> +		"%04x\n", get_unaligned_le16(&ts->dwz_info.product_id));
> +	mutex_unlock(&ts->dev_mutex);
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR(version, 0664, hideep_fw_version_show, NULL);
> +static DEVICE_ATTR(product_id, 0664, hideep_product_id_show, NULL);
> +static DEVICE_ATTR(update_fw, 0664, NULL, hideep_update_fw);
> +
> +static struct attribute *hideep_ts_sysfs_entries[] = {
> +	&dev_attr_version.attr,
> +	&dev_attr_product_id.attr,
> +	&dev_attr_update_fw.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group hideep_ts_attr_group = {
> +	.attrs = hideep_ts_sysfs_entries,
> +};
> +
> +static int hideep_sysfs_init(struct hideep_ts *ts)
> +{
> +	int ret;
> +	struct i2c_client *client = ts->client;
> +
> +	/* Create the files associated with this kobject */
> +	ret = sysfs_create_group(&client->dev.kobj, &hideep_ts_attr_group);
> +
> +	dev_info(&ts->client->dev, "device : %s ", client->dev.kobj.name);
> +
> +	if (ret)
> +		dev_err(&ts->client->dev, "%s: Fail create link error = %d\n",
> +			 __func__, ret);
> +
> +	return ret;
> +}
> +
> +static void hideep_sysfs_exit(void *data)
> +{
> +	struct hideep_ts *ts = data;
> +
> +	sysfs_remove_group(&ts->client->dev.kobj, &hideep_ts_attr_group);
> +}
> +
> +static int __maybe_unused hideep_resume(struct device *dev)
> +{
> +	struct hideep_ts *ts = dev_get_drvdata(dev);
> +	unsigned char sleep_cmd = 0x00;
> +
> +	mutex_lock(&ts->dev_mutex);
> +
> +	if (ts->dev_state == state_normal)
> +		goto hideep_resume_exit;
> +
> +	dev_dbg(dev, "not waiting.");
> +	ts->dev_state = state_normal;
> +
> +	hideep_i2c_write(ts, HIDEEP_SLEEP_CMD, 1, &sleep_cmd);
> +	enable_irq(ts->client->irq);
> +	ts->interrupt_state = 1;
> +
> +hideep_resume_exit:
> +	mdelay(10);
> +	hideep_reset_ic(ts);
> +
> +	mutex_unlock(&ts->dev_mutex);
> +	return 0;
> +}
> +
> +static int __maybe_unused hideep_suspend(struct device *dev)
> +{
> +	struct hideep_ts *ts = dev_get_drvdata(dev);
> +	unsigned char sleep_cmd = 0x01;
> +
> +	mutex_lock(&ts->dev_mutex);
> +	if (ts->dev_state == state_sleep)

How can this be?

> +		goto hideep_suspend_exit;
> +
> +	dev_dbg(dev, "not waiting.");
> +	ts->dev_state = state_sleep;
> +
> +	/* send sleep command.. */
> +	pops_mt(ts);
> +	if (ts->key_use)
> +		pops_ky(ts);

I am pretty sure input core releases keys/contacts for you.

> +	input_sync(ts->input_dev);
> +
> +	/* default deep sleep */
> +	hideep_i2c_write(ts, HIDEEP_SLEEP_CMD, 1, &sleep_cmd);
> +	disable_irq(ts->client->irq);
> +	ts->interrupt_state = 0;
> +
> +hideep_suspend_exit:
> +	mutex_unlock(&ts->dev_mutex);
> +	return 0;
> +}
> +
> +
> +#ifdef CONFIG_OF

Do not guard it for OF systems only - it is all applicable to ACPI
systems and can even be used on legacy boards.

> +static int hideep_parse_dts(struct hideep_ts *ts)
> +{
> +	/* device tree information get */
> +	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +							GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->reset_gpio))
> +		return -EINVAL;

No, do not clobber return values. Your driver will fail to handle device
if you get a deferral here. Use


		return PTR_ERR(pdata->reset_gpio);


> +
> +	ts->vcc_vdd = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(ts->vcc_vdd))
> +		return -EINVAL;

Same here.

> +
> +	ts->vcc_vid = devm_regulator_get(dev, "vid");
> +	if (IS_ERR(ts->vcc_vid))
> +		return -EINVAL;

And here.

> +
> +	ts->key_use = device_property_read_bool(&ts->client->dev,
> +		"touchkey-use");
> +
> +	return 0;
> +}
> +#else
> +static int hideep_parse_dts(struct hideep_ts *ts)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +static int hideep_probe(struct i2c_client *client,
> +	const struct i2c_device_id *id)
> +{
> +	int ret = 0;
> +	struct hideep_ts *ts;
> +
> +	/* check i2c bus */
> +	if (!i2c_check_functionality(client->adapter,
> +		I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "check i2c device error");
> +		return -ENODEV;
> +	}
> +
> +	/* init hideep_ts */
> +	ts = devm_kzalloc(&client->dev,
> +		sizeof(struct hideep_ts), GFP_KERNEL);

sizeof(*ts) is preferred.

> +	if (!ts)
> +		return -ENOMEM;
> +
> +
> +	if (client->dev.of_node) {

Call it unconditionally.

> +		ret = hideep_parse_dts(ts);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ts->client = client;
> +
> +	i2c_set_clientdata(client, ts);
> +
> +	mutex_init(&ts->i2c_mutex);
> +	mutex_init(&ts->dev_mutex);
> +
> +	/* power on */
> +	ret = hideep_pwr_on(ts);
> +	if (ret) {
> +		dev_err(&ts->client->dev, "power on failed");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action(&ts->client->dev, hideep_pwr_off, ts);
> +	if (ret) {
> +		hideep_pwr_off(ts);
> +		return ret;
> +	}
> +
> +	ts->dev_state = state_init;
> +	mdelay(30);
> +
> +	/* ic reset */
> +	hideep_reset_ic(ts);
> +
> +	/* read info */
> +	ret = hideep_load_dwz(ts);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "fail to load dwz, ret = 0x%x", ret);
> +		return ret;
> +	}
> +
> +	/* init input device */
> +	ts->input_dev = devm_input_allocate_device(&client->dev);
> +	if (!ts->input_dev) {
> +		dev_err(&client->dev, "can't allocate memory for input_dev");
> +		return -ENOMEM;
> +	}
> +
> +	touchscreen_parse_properties(ts->input_dev, true, &ts->prop);
> +	hideep_get_info(ts);
> +
> +	ret = hideep_capability(ts);
> +	if (ret) {
> +		dev_err(&client->dev, "can't init input properties");
> +		return ret;
> +	}
> +
> +	ret = input_register_device(ts->input_dev);
> +	if (ret) {
> +		dev_err(&client->dev, "can't register input_dev");
> +		return ret;
> +	}
> +
> +	input_set_drvdata(ts->input_dev, ts);
> +
> +	dev_info(&ts->client->dev, "ts irq: %d", ts->client->irq);
> +	if (IS_ERR(&ts->client->irq)) {
> +		dev_err(&client->dev, "can't be assigned irq");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_request_threaded_irq(&client->dev, ts->client->irq,
> +		NULL, hideep_irq_task, IRQF_ONESHOT,
> +		ts->client->name, ts);
> +
> +	disable_irq(ts->client->irq);
> +	ts->interrupt_state = 0;
> +
> +	if (ret < 0) {
> +		dev_err(&client->dev, "fail to get irq, ret = 0x%08x",
> +			ret);
> +		return ret;
> +	}
> +
> +	ts->dev_state = state_normal;
> +	enable_irq(ts->client->irq);
> +	ts->interrupt_state = 1;
> +
> +	ret = hideep_sysfs_init(ts);
> +	if (ret) {
> +		dev_err(&client->dev, "fail init sys, ret = 0x%x", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action(&ts->client->dev, hideep_sysfs_exit, ts);

We have devm_device_add_group() now.

> +	if (ret) {
> +		hideep_sysfs_exit(ts);
> +		return ret;
> +	}
> +
> +	dev_info(&client->dev, "probe is ok!");

Not needed.

> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM

Drop this guard.

> +static SIMPLE_DEV_PM_OPS(hideep_pm_ops, hideep_suspend, hideep_resume);
> +#endif
> +
> +static const struct i2c_device_id hideep_dev_idtable[] = {
> +	{ HIDEEP_I2C_NAME, 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, hideep_dev_idtable);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id hideep_acpi_id[] = {
> +	{ "HIDP0001", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, hideep_acpi_id);
> +#endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id hideep_match_table[] = {
> +	{ .compatible = "hideep,hideep-lime" },
> +	{ .compatible = "hideep,hideep-crimson" },

Why do we need 2 compatibles when we can detect the kind of device we
are dealing with by querying it?

> +	{},

This is  entinel, no need for the trailing comma as nothing can be added
past it.

> +};
> +MODULE_DEVICE_TABLE(of, hideep_match_table);
> +#endif
> +
> +static struct i2c_driver hideep_driver = {
> +	.probe = hideep_probe,
> +	.id_table = hideep_dev_idtable,
> +	.driver = {
> +		.name = HIDEEP_I2C_NAME,
> +		.of_match_table = of_match_ptr(hideep_match_table),
> +		.acpi_match_table = ACPI_PTR(hideep_acpi_id),
> +		.pm = &hideep_pm_ops,
> +	},
> +};
> +
> +module_i2c_driver(hideep_driver);
> +
> +MODULE_DESCRIPTION("Driver for HiDeep Touchscreen Controller");
> +MODULE_AUTHOR("anthony.kim@hideep.com");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/input/touchscreen/hideep_core.h b/drivers/input/touchscreen/hideep_core.h
> new file mode 100644
> index 0000000..be6f3da
> --- /dev/null
> +++ b/drivers/input/touchscreen/hideep_core.h
> @@ -0,0 +1,221 @@
> +/*
> + * Copyright (C) 2012-2017 Hideep, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foudation.
> + */
> +
> +#ifndef _LINUX_HIDEEP_CORE_H
> +#define _LINUX_HIDEEP_CORE_H
> +/*************************************************************************
> + * Buffer size
> + *************************************************************************/
> +#define FRAME_HEADER_SIZE				8
> +/* if size of system i2c buffer is smaller than this value, need to modify */
> +#define MAX_I2C_BUFFER_SIZE				512
> +
> +/*************************************************************************
> + * board porting config
> + *************************************************************************/
> +#define HIDEEP_TS_NAME					"HiDeep Touchscreen"
> +#define HIDEEP_I2C_NAME					"hideep_ts"
> +
> +/*************************************************************************
> + * register addr
> + *************************************************************************/
> +/* Touch & key event */
> +#define HIDEEP_EVENT_COUNT_ADDR			0x240
> +#define HIDEEP_TOUCH_DATA_ADDR			0x242
> +#define HIDEEP_KEY_DATA_ADDR			0x2A6
> +#define HIDEEP_RAW_DATA_ADDR			0x1000
> +
> +/* command list */
> +#define HIDEEP_RESET_CMD				0x9800
> +#define HIDEEP_INTCLR_CMD				0x9802
> +#define HIDEEP_OPMODE_CMD				0x9804
> +#define HIDEEP_SWTICH_CMD				0x9805
> +#define HIDEEP_SLEEP_CMD				0x980D
> +
> +/*************************************************************************
> + * multi-touch & key definitions.
> + *************************************************************************/
> +#define HIDEEP_MT_MAX					10
> +#define HIDEEP_KEY_MAX					3
> +
> +/* multi touch event bit */
> +#define HIDEEP_MT_ALWAYS_REPORT			0
> +#define HIDEEP_MT_TOUCHED				1
> +#define HIDEEP_MT_FIRST_CONTACT			2
> +#define HIDEEP_MT_DRAG_MOVE				3
> +#define HIDEEP_MT_RELEASED				4
> +#define HIDEEP_MT_PINCH					5
> +#define HIDEEP_MT_PRESSURE				6
> +
> +/* key event bit */
> +#define HIDEEP_KEY_RELEASED				0x20
> +#define HIDEEP_KEY_PRESSED				0x40
> +#define HIDEEP_KEY_FIRST_PRESSED		0x80
> +#define HIDEEP_KEY_PRESSED_MASK			0xC0
> +
> +/*************************************************************************
> + * HiDeep Event
> + *************************************************************************/
> +struct hideep_event {
> +	__le16 x;
> +	__le16 y;
> +	__le16 z;
> +	unsigned char w;
> +	unsigned char flag;
> +	unsigned char type;
> +	unsigned char index;
> +} __packed;
> +
> +
> +/*************************************************************************
> + * IC State
> + *************************************************************************/
> +enum e_dev_state {
> +	state_init = 1,
> +	state_normal,
> +	state_sleep,
> +	state_updating,
> +	state_debugging,
> +};
> +
> +/*************************************************************************
> + * Firmware info
> + *************************************************************************/
> +struct dwz_info {
> +	__le32	code_start;
> +	unsigned char code_crc[12];
> +
> +	__le32 c_code_start;
> +	__le16 c_code_len;
> +	__le16 gen_ver;
> +
> +	__le32 vr_start;
> +	__le16 vr_len;
> +	__le16 rsv0;
> +
> +	__le32 ft_start;
> +	__le16 ft_len;
> +	__le16 vr_version;
> +
> +	__le16 boot_ver;
> +	__le16 core_ver;
> +	__le16 custom_ver;
> +	__le16 release_ver;
> +
> +	unsigned char factory_id;
> +	unsigned char panel_type;
> +	unsigned char model_name[6];
> +	__le16 product_code;
> +	__le16 extra_option;
> +
> +	__le16 product_id;
> +	__le16 vendor_id;
> +} __packed;
> +
> +struct hideep_ts {
> +	struct i2c_client *client;
> +	struct input_dev *input_dev;
> +
> +	struct touchscreen_properties prop;
> +
> +	struct gpio_desc *reset_gpio;
> +
> +	struct regulator *vcc_vdd;
> +	struct regulator *vcc_vid;
> +
> +	struct mutex dev_mutex;
> +	struct mutex i2c_mutex;
> +
> +	bool suspended;
> +	enum e_dev_state dev_state;
> +
> +	long tch_bit;
> +	unsigned int tch_count;
> +	unsigned int key_count;
> +	unsigned int lpm_count;
> +
> +	unsigned char touch_event[HIDEEP_MT_MAX * 10];
> +	unsigned char key_event[HIDEEP_KEY_MAX * 2];
> +	bool key_use;
> +
> +	struct dwz_info dwz_info;
> +
> +	int interrupt_state;
> +	int fw_size;
> +	int nvm_mask;
> +} __packed;
> +
> +#define NVM_DEFAULT_PAGE		0
> +#define NVM_SFR_WPAGE			1
> +#define NVM_SFR_RPAGE			2
> +
> +#define PIO_SIG					0x00400000
> +#define _PROT_MODE				0x03400000
> +
> +#define NVM_PAGE_SIZE			128
> +
> +#define HIDEEP_NVM_DOWNLOAD		0x10000000
> +
> +/*************************************************************************
> + * register map
> + *************************************************************************/
> +#define YRAM_BASE				0x40000000
> +#define PERIPHERAL_BASE			0x50000000
> +#define ESI_BASE				(PERIPHERAL_BASE + 0x00000000)
> +#define FLASH_BASE				(PERIPHERAL_BASE + 0x01000000)
> +#define SYSCON_BASE				(PERIPHERAL_BASE + 0x02000000)
> +
> +#define SYSCON_MOD_CON			(SYSCON_BASE + 0x0000)
> +#define SYSCON_SPC_CON			(SYSCON_BASE + 0x0004)
> +#define SYSCON_CLK_CON			(SYSCON_BASE + 0x0008)
> +#define SYSCON_CLK_ENA			(SYSCON_BASE + 0x000C)
> +#define SYSCON_RST_CON			(SYSCON_BASE + 0x0010)
> +#define SYSCON_WDT_CON			(SYSCON_BASE + 0x0014)
> +#define SYSCON_WDT_CNT			(SYSCON_BASE + 0x0018)
> +#define SYSCON_PWR_CON			(SYSCON_BASE + 0x0020)
> +#define SYSCON_PGM_ID			(SYSCON_BASE + 0x00F4)
> +
> +#define FLASH_CON				(FLASH_BASE + 0x0000)
> +#define FLASH_STA				(FLASH_BASE + 0x0004)
> +#define FLASH_CFG				(FLASH_BASE + 0x0008)
> +#define FLASH_TIM				(FLASH_BASE + 0x000C)
> +#define FLASH_CACHE_CFG			(FLASH_BASE + 0x0010)
> +
> +#define ESI_TX_INVALID			(ESI_BASE + 0x0008)
> +
> +/*************************************************************************
> + * flash commands
> + *************************************************************************/
> +#define MERASE					0x00010000
> +#define SERASE					0x00020000
> +#define PERASE					0x00040000
> +#define PROG					0x00080000
> +#define WRONLY					0x00100000
> +
> +#define NVM_MASK_OFS			0x0000000C
> +
> +/*************************************************************************
> + * DWZ info
> + *************************************************************************/
> +#define HIDEEP_BOOT_SECTION		0x00000400
> +#define HIDEEP_BOOT_LEN			0x00000400
> +#define HIDEEP_DWZ_SECTION		0x00000280
> +#define HIDEEP_DWZ_INFO_OFS		0x000002C0
> +#define HIDEEP_DWZ_LEN			(HIDEEP_BOOT_SECTION \
> +							- HIDEEP_DWZ_SECTION)
> +
> +struct pgm_packet {
> +	union {
> +		unsigned char b[8];
> +		__be32 w[2];
> +	} header;
> +
> +	__be32 payload[NVM_PAGE_SIZE / sizeof(unsigned int)];
> +};
> +
> +#endif
> -- 
> 2.7.4
> 

Thanks.