lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170831065102.GA462@dtor-ws>
Date:   Wed, 30 Aug 2017 23:51:02 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Anthony Kim <anthony.kim@...eep.com>
Cc:     robh+dt@...nel.org, mark.rutland@....com, rydberg@...math.org,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH] Input: add support for HiDeep touchscreen

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@...eep.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@...00000 {
> +
> +	/* ... */
> +
> +	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@...eep.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.

-- 
Dmitry

Powered by blists - more mailing lists