RE: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver

[Jeffrey Lin (林義章) <Jeffrey.Lin@xxxxxxxxxx>]

Hi Dmitry:
Thanks for your response. I'll fixed issues you remarked in the mail.

Best Regards
----------------------------------------------------------------------
Jeffrey Lin,林義章
瑞鼎科技
Raydium Semiconductor Corporation
Tel:(03)666-1818 Ext.4163
Fax:(03)666-1919

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] 
Sent: Friday, March 11, 2016 2:48 AM
To: jeffrey.lin
Cc: rydberg@xxxxxxxxxxx; grant.likely@xxxxxxxxxx; robh+dt@xxxxxxxxxx; jeesw@xxxxxxxxxx; bleung@xxxxxxxxxxxx; scott.liu@xxxxxxxxxx; Jeffrey Lin (林義章); Roger Yang (楊鎮瑋); KP Li (李昆倍); Albert Shieh (謝欣瑋); linux-kernel@xxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver

Hi Jeffrey,

On Thu, Mar 03, 2016 at 02:42:11PM +0800, jeffrey.lin wrote:
> Raydium I2C touch driver.
> 
> Signed-off-by: jeffrey.lin <jeffrey.lin@xxxxxxxxxx>
> ---
>  drivers/input/touchscreen/Kconfig          |  13 +
>  drivers/input/touchscreen/Makefile         |   1 +
>  drivers/input/touchscreen/raydium_i2c_ts.c | 953 
> +++++++++++++++++++++++++++++
>  3 files changed, 967 insertions(+)
>  create mode 100644 drivers/input/touchscreen/raydium_i2c_ts.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig 
> b/drivers/input/touchscreen/Kconfig
> index 3f3f6ee..9adacf6 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -915,6 +915,19 @@ config TOUCHSCREEN_PCAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pcap_ts.
>  
> +config TOUCHSCREEN_RM_TS

Can we call it TOUCHSCREEN_RAYDIUM_RM31100? Do you have other models?
Maybe RM31XXX or similar?

Or TOUCHSCREEN_RAYDIUM_I2C?

I just want to make a general i2c driver for our several raydium models. So I think TOUCHSCREEN_RAYDIUM_I2C be better.
> +	tristate "Raydium I2C Touchscreen"
> +	depends on I2C
> +	help
> +	  Say Y here if you have Raydium series I2C touchscreen,
> +	  such as RM31100 , connected to your system.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called raydium_i2c_ts.
> +
> +

Extra empty line.

>  config TOUCHSCREEN_ST1232
>  	tristate "Sitronix ST1232 touchscreen controllers"
>  	depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile 
> b/drivers/input/touchscreen/Makefile
> index 4941f2d..99e08cf 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -55,6 +55,7 @@ obj-$(CONFIG_TOUCHSCREEN_USB_COMPOSITE)	+= usbtouchscreen.o
>  obj-$(CONFIG_TOUCHSCREEN_PCAP)		+= pcap_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_PENMOUNT)	+= penmount.o
>  obj-$(CONFIG_TOUCHSCREEN_PIXCIR)	+= pixcir_i2c_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_RM_TS)		+= raydium_i2c_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_S3C2410)	+= s3c2410_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ST1232)	+= st1232.o
>  obj-$(CONFIG_TOUCHSCREEN_STMPE)		+= stmpe-ts.o
> diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c 
> b/drivers/input/touchscreen/raydium_i2c_ts.c
> new file mode 100644
> index 0000000..7ba681e
> --- /dev/null
> +++ b/drivers/input/touchscreen/raydium_i2c_ts.c
> @@ -0,0 +1,953 @@
> +/*
> + * Raydium touchscreen I2C driver.
> + *
> + * Copyright (C) 2012-2014, Raydium Semiconductor Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2, and only version 2, as published by the
> + * Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Raydium reserves the right to make changes without further notice
> + * to the materials described herein. Raydium does not assume any
> + * liability arising out of the application described herein.
> + *
> + * Contact Raydium Semiconductor Corporation at www.rad-ic.com  */
> +
> +#include <linux/module.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/uaccess.h>
> +#include <linux/buffer_head.h>

Why do you need this include?

> +#include <linux/slab.h>
> +#include <linux/firmware.h>
> +#include <linux/input/mt.h>
> +#include <linux/acpi.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>

I do not think you need this one.

> +#include <linux/regulator/consumer.h> #include <asm/unaligned.h> 
> +#include <linux/gpio/consumer.h>
> +
> +/* Device, Driver information */
> +#define DEVICE_NAME	"raydium_i2c"
> +
> +/* Slave I2C mode*/
> +#define RM_BOOT_BLDR	0x02
> +#define RM_BOOT_MAIN	0x03
> +
> +/*I2C command */
> +#define CMD_QUERY_BANK		0x2B
> +#define CMD_DATA_BANK		0x4D
> +#define CMD_ENTER_SLEEP		0x4E
> +#define CMD_BOOT_ACK		0x0A
> +#define CMD_BOOT_WRT		0x5B
> +#define CMD_BOOT_CHK		0x0C
> +#define CMD_BANK_SWITCH		0xAA
> +
> +/* Touch relative info */
> +#define MAX_RETRIES		3
> +#define MAX_FW_UPDATE_RETRIES	30
> +#define MAX_TOUCH_NUM		10
> +#define MAX_PACKET_SIZE		60
> +#define BOOT_DELAY_MS	100
> +
> +#define RAYDIUM_FW_PAGESIZE	128
> +#define RAYDIUM_POWERON_DELAY_USEC	500
> +#define RAYDIUM_RESET_DELAY_MSEC	50
> +
> +#define ADDR_INDEX		0x03
> +#define HEADER_SIZE		4
> +
> +enum raydium_boot_mode {
> +	RAYDIUM_TS_MAIN,
> +	RAYDIUM_TS_BLDR,
> +};
> +
> +struct raydium_info {
> +	u32 hw_ver;

This seems to be coming directly form the wire, you need to annotate it as little- or big-endian and use appropriate accessors to convert to CPU endianness.

Because of many CPU endianness are different, so next I'll use buffer or point to replace them and use le16(or le32)_to_cpu().
> +	u8 main_ver;
> +	u8 sub_ver;
> +	u16 ft_ver;

Same here.

> +	u8 x_num;
> +	u8 y_num;
> +	u16 x_max;
> +	u16 y_max;

And here.

> +	u8 x_res;		/* units/mm */
> +	u8 y_res;		/* units/mm */
> +};
> +
> +struct raydium_abs_info {
> +	u8 state;/*1:touch, 0:no touch*/
> +	u8 x_pos_lsb;
> +	u8 x_pos_msb;
> +	u8 y_pos_lsb;
> +	u8 y_pos_msb;
> +	u8 pressure;
> +	u8 x_width;
> +	u8 y_width;
> +};

I'd rather define offsets and use get_unaligned_le16() to fetch x/y data.

> +
> +struct raydium_object {
> +	u32 data_bank_addr;

This likely needs to be converted to cpu-endianness as well.

> +	u8 pkg_size;
> +};
> +
> +/* struct raydium_data - represents state of Raydium touchscreen 
> +device */ struct raydium_data {
> +	struct i2c_client *client;
> +	struct input_dev *input;
> +
> +	struct regulator *vcc33;
> +	struct regulator *vccio;
> +	struct gpio_desc *reset_gpio;
> +
> +	u32 query_bank_info;
> +
> +	struct raydium_info info;
> +	struct raydium_object obj;
> +	struct raydium_abs_info finger;

This is not used.

> +
> +	enum raydium_boot_mode boot_mode;
> +
> +	struct mutex sysfs_mutex;
> +
> +	u8 cmd_resp[HEADER_SIZE];
> +	struct completion cmd_done;
> +
> +	u8 buf[MAX_PACKET_SIZE];
> +
> +	bool wake_irq_enabled;
> +};
> +
> +static int raydium_i2c_send(struct i2c_client *client,
> +	u8 addr, u8 *data, size_t len)
> +{
> +	u8 buf[MAX_PACKET_SIZE + 1];
> +	int tries = 0;
> +
> +	if (len > MAX_PACKET_SIZE)
> +		return -EINVAL;
> +
> +	buf[0] = addr;
> +	memcpy(&buf[1], data, len);
> +
> +	do {
> +		if (i2c_master_send(client, buf, len + 1) == (len + 1))
> +			return 0;
> +		msleep(20);
> +	} while (++tries < MAX_RETRIES);
> +
> +	dev_err(&client->dev, "%s: i2c send failed\n", __func__);
> +
> +	return -EIO;
> +}
> +
> +static int raydium_i2c_read(struct i2c_client *client,
> +	u8 addr, size_t len, void *data)
> +{
> +	struct i2c_msg xfer[2];
> +
> +	/* Write register */
> +	xfer[0].addr = client->addr;
> +	xfer[0].flags = 0;
> +	xfer[0].len = 1;
> +	xfer[0].buf = &addr;
> +
> +	/* Read data */
> +	xfer[1].addr = client->addr;
> +	xfer[1].flags = I2C_M_RD;
> +	xfer[1].len = len;
> +	xfer[1].buf = data;
> +
> +	if (i2c_transfer(client->adapter, xfer, 2) == 2)

ARRAY_SIZE()

> +		return 0;
> +
> +	dev_err(&client->dev, "%s: i2c transfer failed\n", __func__);
> +
> +	return -EIO;

Do not clobber errors returned by i2c_transfer.

> +}
> +
> +static int raydium_i2c_read_message(struct i2c_client *client,
> +	u32 addr, size_t len, void *data)
> +{
> +	u16 pkg_size, use_len;
> +	u8 buf[HEADER_SIZE], idx_i, idx_j;
> +	int error;
> +
> +	use_len = len;
> +	idx_j = 0;
> +	while (use_len > 0) {
> +		pkg_size = (use_len < MAX_PACKET_SIZE) ?
> +			use_len : MAX_PACKET_SIZE;
> +		for (idx_i = 0; idx_i < HEADER_SIZE; idx_i++)
> +			buf[idx_i] = addr >> (HEADER_SIZE - 1 - idx_i)*8;
> +
> +		/*set data bank*/
> +		error = raydium_i2c_send(client, CMD_BANK_SWITCH,
> +			(u8 *)buf, HEADER_SIZE);
> +		/*read potints data*/
> +		if (!error)
> +			error = raydium_i2c_read(client, buf[ADDR_INDEX],
> +				pkg_size,
> +				(void *)(data + idx_j*MAX_PACKET_SIZE));

You do not need to convert to void * pointers.

> +
> +		pkg_size += MAX_PACKET_SIZE;
> +		addr += MAX_PACKET_SIZE;
> +		use_len = (use_len < MAX_PACKET_SIZE) ?
> +			0 : (use_len - MAX_PACKET_SIZE);
> +		idx_j++;
> +	}
> +
> +	return error;
> +}
> +
> +static int raydium_i2c_send_message(struct i2c_client *client,
> +	size_t len, void *data)
> +{
> +	int error;
> +	u8 buf[HEADER_SIZE], ii;
> +
> +	for (ii = 0; ii < HEADER_SIZE; ii++)
> +		buf[ii] = ((u8 *)data)[3 - ii];

This looks like cpu_to_be32().

> +	/*set data bank*/
> +	error = raydium_i2c_send(client, CMD_BANK_SWITCH, (u8 *)buf,
> +		HEADER_SIZE);
> +
> +	/*send message*/
> +	if (!error)
> +		error = raydium_i2c_send(client, buf[ADDR_INDEX], buf, len);
> +
> +	return error;
> +}
> +
> +static int raydium_i2c_sw_reset(struct i2c_client *client) {
> +	static const u8 soft_rst_cmd[] = { 0x01, 0x04, 0x00, 0x00, 0x40};
> +	int error;
> +
> +	error = raydium_i2c_send_message(client, 1, (void *)soft_rst_cmd);

This is wrong. You cast away constness. I also confused how it works.
You have 5 bytes of command, but I think you send first 4 and then fist
1 in raydium_i2c_send_message; you never get to the 5th byte.

Sorry. I sent the wrong length.

> +	if (error) {
> +		dev_err(&client->dev, "software reset failed: %d\n", error);
> +		return error;
> +	}
> +
> +	msleep(RAYDIUM_RESET_DELAY_MSEC);
> +
> +	return 0;
> +}
> +
> +static int raydium_i2c_query_ts_info(struct raydium_data *ts) {
> +	struct i2c_client *client = ts->client;
> +	int error, retry_cnt;
> +
> +	for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> +		error = raydium_i2c_read(client, CMD_DATA_BANK,
> +			sizeof(ts->obj), (void *)&ts->obj);
> +		if (!error) {
> +			error = raydium_i2c_read(client, CMD_QUERY_BANK,
> +				sizeof(ts->query_bank_info),
> +				(void *)&ts->query_bank_info);
> +			if (!error) {
> +				error = raydium_i2c_read_message(client,
> +					ts->query_bank_info, sizeof(ts->info),
> +					(void *)&ts->info);
> +			}
> +
> +			if (!error)
> +				return 0;
> +		}
> +	}
> +	dev_err(&client->dev, "get data bank failed: %d\n", error);
> +
> +	return -EINVAL;
> +}
> +
> +static int raydium_i2c_fastboot(struct i2c_client *client) {
> +	static const u8 boot_cmd[] = { 0x20, 0x06, 0x00, 0x50 };
> +	u8 buf[HEADER_SIZE];
> +	int error;
> +
> +	error = raydium_i2c_read_message(client,
> +		get_unaligned_be32(boot_cmd),
> +		sizeof(boot_cmd), buf);
> +
> +	if (!error) {
> +		if (buf[0] == RM_BOOT_BLDR) {
> +			dev_dbg(&client->dev, "boot in fastboot mode\n");
> +			return -EINVAL;
> +		}
> +		dev_dbg(&client->dev, "boot success -- 0x%x\n", client->addr);
> +		return 0;
> +	}
> +
> +	dev_err(&client->dev, "boot failed: %d\n", error);
> +
> +	return error;
> +}
> +
> +static int raydium_i2c_initialize(struct raydium_data *ts) {
> +	struct i2c_client *client = ts->client;
> +	int error, retry_cnt;
> +	static const u8 recov_packet[] = { 0x04, 0x81 };
> +	u8 buf[HEADER_SIZE];
> +
> +	for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> +		error = raydium_i2c_fastboot(client);
> +		if (error) {
> +			/* Continue initializing if it's the last try */
> +			if (retry_cnt < MAX_RETRIES - 1)
> +				continue;
> +		}
> +		/* Wait for Hello packet */
> +		msleep(BOOT_DELAY_MS);
> +
> +		error = raydium_i2c_read(client, recov_packet[0], 1,
> +			(void *)buf);
> +		if (error) {
> +			dev_err(&client->dev,
> +				"failed to read 'hello' packet: %d\n", error);
> +		} else if (buf[0] == recov_packet[1]) {
> +			ts->boot_mode = RAYDIUM_TS_MAIN;
> +			break;
> +		}
> +	}
> +
> +	if (error)
> +		ts->boot_mode = RAYDIUM_TS_BLDR;
> +	else
> +		raydium_i2c_query_ts_info(ts);
> +
> +	return error;
> +}
> +
> +static int raydium_i2c_fw_write_page(struct i2c_client *client,
> +				    const void *page)
> +{
> +	static const u8 ack_ok[] = { 0x55, 0xAA };
> +	u8 buf[2];
> +	int retry;
> +	int error;
> +
> +	for (retry = 0; retry < MAX_FW_UPDATE_RETRIES; retry++) {
> +		error = raydium_i2c_send(client, CMD_BOOT_WRT,
> +			(u8 *)page, RAYDIUM_FW_PAGESIZE);
> +		if (error) {
> +			dev_err(&client->dev,
> +				"BLDR Write Page failed: %d\n", error);
> +			continue;
> +		}
> +
> +		error = raydium_i2c_read(client, CMD_BOOT_CHK, sizeof(ack_ok),
> +			(void *)buf);
> +		if (error) {
> +			dev_err(&client->dev,
> +				"BLDR Ack read failed: %d\n", error);
> +			return error;
> +		}
> +
> +		if (!memcmp(buf, ack_ok, sizeof(ack_ok)))
> +			return 0;
> +
> +		error = -EIO;
> +		dev_err(&client->dev,
> +			"BLDR Get Ack Error [%02x:%02x]\n", buf[0], buf[1]);
> +	}
> +
> +	return error;
> +}
> +
> +static int raydium_i2c_do_update_firmware(struct i2c_client *client,
> +					 const struct firmware *fw,
> +					 bool force)
> +{
> +	static const u8 boot_cmd[] = { RM_BOOT_BLDR, 0x20, 0x06, 0x00, 0x50 };
> +	static const u8 main_cmd[] = { RM_BOOT_MAIN, 0x20, 0x06, 0x00, 0x50 };
> +	static const u8 boot_ack[] = { 0x55, 0xAA, 0x00, 0xFF };
> +	u8 buf[HEADER_SIZE];
> +	int page, n_fw_pages;
> +	int error;
> +
> +	/* Recovery mode detection! */
> +	if (!force) {
> +		/* Start boot loader Procedure */
> +		dev_dbg(&client->dev, "Normal BLDR procedure\n");
> +		/* switch to mode */
> +		error = raydium_i2c_send_message(client, 1, (void *)boot_cmd);
> +		if (error)
> +			dev_err(&client->dev, "failed to send boot cmd: %d\n",
> +				error);
> +		msleep(60);
> +		raydium_i2c_sw_reset(client);
> +		msleep(RAYDIUM_RESET_DELAY_MSEC);
> +		error = raydium_i2c_send_message(client, 1, (void *)boot_cmd);
> +	}
> +
> +	if (error) {
> +		dev_err(&client->dev, "failed to enter fastboot mode: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	msleep(20);
> +
> +	/* check fastboot state */
> +	error = raydium_i2c_read(client, CMD_BOOT_ACK,
> +		sizeof(boot_ack), (void *)buf);
> +	if (error) {
> +		dev_err(&client->dev,
> +			"failed to read boot ack: %d\n", error);
> +		return error;
> +	}
> +
> +	if (memcmp(buf, boot_ack, sizeof(boot_ack))) {
> +		dev_err(&client->dev,
> +			"failed to enter fastboot: %*ph (expected %*ph)\n",
> +			(int)sizeof(buf), buf, (int)sizeof(boot_ack), boot_ack);
> +		return -EIO;
> +	}
> +
> +	dev_info(&client->dev, "successfully entered fastboot mode");
> +
> +	n_fw_pages = fw->size / RAYDIUM_FW_PAGESIZE;
> +	dev_dbg(&client->dev, "BLDR Pages = %d\n", n_fw_pages);
> +
> +	for (page = 0; page < n_fw_pages; page++) {
> +		error = raydium_i2c_fw_write_page(client,
> +					fw->data + page * RAYDIUM_FW_PAGESIZE);
> +		if (error) {
> +			dev_err(&client->dev,
> +				"failed to write FW page %d: %d\n",
> +				page, error);
> +			return error;
> +		}
> +	}
> +	error = raydium_i2c_send_message(client, 1, (void *)main_cmd);
> +	msleep(20);
> +	raydium_i2c_sw_reset(client);
> +	dev_info(&client->dev, "firmware update completed\n");
> +
> +	return 0;
> +}
> +
> +static int raydium_i2c_fw_update(struct raydium_data *ts) {
> +	struct i2c_client *client = ts->client;
> +	const struct firmware *fw;
> +	char *fw_name;
> +	int error;
> +
> +	fw_name = kasprintf(GFP_KERNEL, "raydium_i2c_%04x.bin",
> +		ts->info.hw_ver & 0xFFFF);
> +	if (!fw_name)
> +		return -ENOMEM;
> +
> +	dev_info(&client->dev, "requesting fw name = %s\n", fw_name);
> +	error = request_firmware(&fw, fw_name, &client->dev);
> +	kfree(fw_name);
> +	if (error) {
> +		dev_err(&client->dev, "failed to request firmware: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	if (fw->size % RAYDIUM_FW_PAGESIZE) {
> +		dev_err(&client->dev, "invalid firmware length: %zu\n",
> +			fw->size);
> +		error = -EINVAL;
> +		goto out;
> +	}
> +
> +	disable_irq(client->irq);
> +	error = raydium_i2c_do_update_firmware(client, fw,
> +					ts->boot_mode == RAYDIUM_TS_BLDR);
> +	if (error) {
> +		dev_err(&client->dev, "firmware update failed: %d\n", error);
> +		ts->boot_mode = RAYDIUM_TS_BLDR;
> +		goto out_enable_irq;
> +	}
> +	error = raydium_i2c_initialize(ts);
> +	if (error) {
> +		dev_err(&client->dev,
> +			"failed to initialize device after firmware update: %d\n",
> +			error);
> +		ts->boot_mode = RAYDIUM_TS_BLDR;
> +		goto out_enable_irq;
> +	}
> +
> +	ts->boot_mode = RAYDIUM_TS_MAIN;
> +
> +out_enable_irq:
> +	enable_irq(client->irq);
> +	msleep(100);
> +out:
> +	release_firmware(fw);
> +	return error;
> +}
> +
> +static void raydium_mt_event(struct raydium_data *ts) {
> +	struct raydium_abs_info *data;
> +	int error, i, x, y;
> +	u8 f_state;
> +	u8 touch_count;
> +	u8 tp_info_size;
> +
> +	error = raydium_i2c_read_message(ts->client, ts->obj.data_bank_addr,
> +		ts->obj.pkg_size, (void *)&ts->buf);
> +
> +	if (error < 0) {
> +		dev_err(&ts->client->dev, "%s: failed to read data: %d\n",
> +			__func__, error);
> +		return;
> +	}
> +
> +	touch_count = 0;
> +	tp_info_size = sizeof(ts->finger);
> +
> +	for (i = 0; i < MAX_TOUCH_NUM; i++) {
> +		data = (struct raydium_abs_info *)(ts->buf + i * tp_info_size);
> +
> +		f_state = data->state & 0x03;
> +
> +		input_mt_slot(ts->input, i);
> +		input_mt_report_slot_state(ts->input,
> +				MT_TOOL_FINGER, f_state != 0);
> +
> +		if (!f_state)
> +			continue;
> +
> +		x = (data->x_pos_msb << 8) | (data->x_pos_lsb);
> +		y = (data->y_pos_msb << 8) | (data->y_pos_lsb);
> +
> +		input_report_key(ts->input, BTN_TOUCH, 1);
> +		input_report_key(ts->input, BTN_TOOL_FINGER, 1);

Drop these 2.

> +		input_report_abs(ts->input, ABS_MT_POSITION_X, x);
> +		input_report_abs(ts->input, ABS_MT_POSITION_Y, y);
> +		input_report_abs(ts->input, ABS_MT_PRESSURE, data->pressure);
> +		input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR,
> +			max(data->x_width, data->y_width));
> +		input_report_abs(ts->input, ABS_MT_TOUCH_MINOR,
> +			min(data->x_width, data->y_width));
> +		touch_count++;
> +	}
> +
> +	input_report_key(ts->input, BTN_TOUCH, touch_count > 0);


> +	input_report_key(ts->input, BTN_TOOL_FINGER, ts->input > 0);

What does ts->input > 0 means?

> +	input_mt_report_pointer_emulation(ts->input, false);

Please call input_mt_sync_frame() instead.

> +	input_sync(ts->input);
> +}
> +
> +static irqreturn_t raydium_i2c_irq(int irq, void *_dev) {
> +	struct raydium_data *ts = _dev;
> +
> +	if (ts->boot_mode != RAYDIUM_TS_BLDR)
> +		raydium_mt_event(ts);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static ssize_t write_update_fw(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct raydium_data *ts = dev_get_drvdata(dev);
> +	int error;
> +
> +	error = mutex_lock_interruptible(&ts->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	error = raydium_i2c_fw_update(ts);
> +	dev_dbg(dev, "firmware update result: %d\n", error);
> +
> +	mutex_unlock(&ts->sysfs_mutex);
> +	return error ?: count;
> +}
> +
> +static ssize_t raydium_bootmode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf) {
> +	struct raydium_data *ts = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", ts->boot_mode == RAYDIUM_TS_MAIN ?
> +		"Normal" : "Recovery");
> +}
> +
> +static ssize_t raydium_fw_ver_show(struct device *dev,
> +			struct device_attribute *attr, char *buf) {
> +	struct raydium_data *ts = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "Release Version %d.%d\n",
> +		ts->info.main_ver, ts->info.sub_ver);

Do not use strings in sysfs attributes. Just use %d.%d or, better yet, %02x.%02x.

> +}
> +
> +static ssize_t raydium_hw_ver_show(struct device *dev,
> +			struct device_attribute *attr, char *buf) {
> +	struct raydium_data *ts = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "Hardware version 0x%x\n",
> +		ts->info.hw_ver & 0xFFFF);

Same here, just use %04x.

> +}
> +
> +static DEVICE_ATTR(fw_version, S_IRUGO, raydium_fw_ver_show, NULL); 
> +static DEVICE_ATTR(hw_version, S_IRUGO, raydium_hw_ver_show, NULL); 
> +static DEVICE_ATTR(boot_mode, S_IRUGO, raydium_bootmode_show, NULL); 
> +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, write_update_fw);
> +
> +static struct attribute *raydium_attributes[] = {
> +	&dev_attr_update_fw.attr,
> +	&dev_attr_boot_mode.attr,
> +	&dev_attr_fw_version.attr,
> +	&dev_attr_hw_version.attr,
> +	NULL
> +};
> +
> +static struct attribute_group raydium_attribute_group = {
> +	.attrs = raydium_attributes,
> +};
> +
> +static void raydium_i2c_remove_sysfs_group(void *_data) {
> +	struct raydium_data *ts = _data;
> +
> +	sysfs_remove_group(&ts->client->dev.kobj, &raydium_attribute_group); 
> +}
> +
> +static int raydium_i2c_power_on(struct raydium_data *ts) {
> +	int error;
> +
> +	if (IS_ERR_OR_NULL(ts->reset_gpio))
> +		return 0;
> +
> +	gpiod_set_value_cansleep(ts->reset_gpio, 1);
> +
> +	error = regulator_enable(ts->vcc33);
> +	if (error) {
> +		dev_err(&ts->client->dev,
> +			"failed to enable vcc33 regulator: %d\n", error);
> +		goto release_reset_gpio;
> +	}
> +
> +	error = regulator_enable(ts->vccio);
> +	if (error) {
> +		regulator_disable(ts->vcc33);
> +		dev_err(&ts->client->dev,
> +			"failed to enable vccio regulator: %d\n", error);
> +		goto release_reset_gpio;
> +	}
> +
> +	udelay(RAYDIUM_POWERON_DELAY_USEC);
> +
> +release_reset_gpio:
> +	gpiod_set_value_cansleep(ts->reset_gpio, 0);
> +
> +	if (error)
> +		return error;
> +
> +	msleep(RAYDIUM_RESET_DELAY_MSEC);
> +
> +	return 0;
> +}
> +
> +static void raydium_i2c_power_off(void *_data) {
> +	struct raydium_data *ts = _data;
> +
> +	if (!IS_ERR_OR_NULL(ts->reset_gpio)) {
> +		gpiod_set_value_cansleep(ts->reset_gpio, 1);
> +		regulator_disable(ts->vccio);
> +		regulator_disable(ts->vcc33);
> +	}
> +}
> +
> +static int raydium_i2c_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *id) {
> +	union i2c_smbus_data dummy;
> +	struct raydium_data *ts;
> +	unsigned long irqflags;
> +	int error;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev,
> +			"%s: i2c check functionality error\n", DEVICE_NAME);
> +		return -ENXIO;
> +	}
> +
> +	ts = devm_kzalloc(&client->dev, sizeof(struct raydium_data),
> +		GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	mutex_init(&ts->sysfs_mutex);
> +	init_completion(&ts->cmd_done);
> +
> +	ts->client = client;
> +	i2c_set_clientdata(client, ts);
> +
> +	ts->vcc33 = devm_regulator_get(&client->dev, "avdd");

If regulator is called avdd then I'd rather have ts->avdd as well.

> +	if (IS_ERR(ts->vcc33)) {
> +		error = PTR_ERR(ts->vcc33);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(&client->dev,
> +				"Failed to get 'vcc33' regulator: %d\n", error);
> +		return error;
> +	}
> +
> +	ts->vccio = devm_regulator_get(&client->dev, "vdd");

ts->vccio -> ts->vdd please.

You also need to add binding documentation to Documentation/devicetree/bindings/input/...

> +	if (IS_ERR(ts->vccio)) {
> +		error = PTR_ERR(ts->vccio);
> +		if (error != -EPROBE_DEFER)
> +			dev_err(&client->dev,
> +				"Failed to get 'vccio' regulator: %d\n", error);
> +		return error;
> +	}
> +
> +

Extra empty line.

> +	ts->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +		GPIOD_OUT_LOW);
> +	if (IS_ERR(ts->reset_gpio)) {
> +		error = PTR_ERR(ts->reset_gpio);
> +		if (error != -EPROBE_DEFER) {
> +			dev_err(&client->dev,
> +				"failed to get reset gpio: %d\n", error);
> +			return error;
> +		}
> +	}
> +
> +	error = raydium_i2c_power_on(ts);
> +	if (error)
> +		return error;
> +
> +	error = devm_add_action(&client->dev, raydium_i2c_power_off, ts);
> +	if (error) {
> +		dev_err(&client->dev,
> +			"failed to install power off action: %d\n", error);
> +		raydium_i2c_power_off(ts);
> +		return error;
> +	}
> +
> +	/* Make sure there is something at this address */
> +	if (i2c_smbus_xfer(client->adapter, client->addr, 0,
> +			I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy) < 0) {
> +		dev_err(&client->dev, "nothing at this address\n");
> +		return -ENXIO;
> +	}
> +
> +	error = raydium_i2c_initialize(ts);
> +	if (error) {
> +		dev_err(&client->dev, "failed to initialize: %d\n", error);
> +		return error;
> +	}
> +
> +	ts->input = devm_input_allocate_device(&client->dev);
> +	if (!ts->input) {
> +		dev_err(&client->dev, "Failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ts->input->name = "Raydium Touchscreen";
> +	ts->input->id.bustype = BUS_I2C;
> +
> +	__set_bit(BTN_TOUCH, ts->input->keybit);
> +	__set_bit(EV_ABS, ts->input->evbit);
> +	__set_bit(EV_KEY, ts->input->evbit);
> +
> +	/* Single touch input params setup */
> +	input_set_abs_params(ts->input, ABS_X, 0, ts->info.x_max, 0, 0);
> +	input_set_abs_params(ts->input, ABS_Y, 0, ts->info.y_max, 0, 0);
> +	input_set_abs_params(ts->input, ABS_PRESSURE, 0, 255, 0, 0);
> +	input_abs_set_res(ts->input, ABS_X, ts->info.x_res);
> +	input_abs_set_res(ts->input, ABS_Y, ts->info.y_res);
> +
> +	/* Multitouch input params setup */
> +	error = input_mt_init_slots(ts->input, MAX_TOUCH_NUM,
> +				    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +	if (error) {
> +		dev_err(&client->dev,
> +			"failed to initialize MT slots: %d\n", error);
> +		return error;
> +	}
> +
> +	input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0,
> +		ts->info.x_max, 0, 0);
> +	input_set_abs_params(ts->input, ABS_MT_POSITION_Y,
> +		0, ts->info.y_max, 0, 0);
> +	input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +	input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +	input_abs_set_res(ts->input, ABS_MT_POSITION_X, ts->info.x_res);
> +	input_abs_set_res(ts->input, ABS_MT_POSITION_Y, ts->info.y_res);
> +
> +	input_mt_init_slots(ts->input, MAX_TOUCH_NUM, 0);

You already called this once a few lines above. But what you actually want is to:

1. Set MT axies parameters.
2. Call  input_mt_init_slots(ts->input, MAX_TOUCH_NUM,
		INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED)
   so that it will finish MT initialization and will copy MT parameters
   over to ST.

> +
> +	input_set_drvdata(ts->input, ts);
> +
> +	error = input_register_device(ts->input);
> +	if (error) {
> +		dev_err(&client->dev,
> +			"unable to register input device: %d\n", error);
> +		return error;
> +	}
> +
> +	error = devm_request_threaded_irq(&client->dev, client->irq,
> +			NULL, raydium_i2c_irq, irqflags | IRQF_ONESHOT,
> +			client->name, ts);
> +	if (error) {
> +		dev_err(&client->dev, "Failed to register interrupt\n");
> +		return error;
> +	}
> +
> +	if (!client->dev.of_node)
> +		device_init_wakeup(&client->dev, true);

Please drop and instead make ACPI platform code mark i2c device as wakeup capable when registering it.

> +
> +	error = sysfs_create_group(&client->dev.kobj, &raydium_attribute_group);
> +	if (error) {
> +		dev_err(&client->dev, "failed to create sysfs attributes: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	error = devm_add_action(&client->dev,
> +				raydium_i2c_remove_sysfs_group, ts);
> +	if (error) {
> +		raydium_i2c_remove_sysfs_group(ts);
> +		dev_err(&client->dev,
> +			"Failed to add sysfs cleanup action: %d\n", error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static int raydium_i2c_remove(struct i2c_client *client) {
> +	struct raydium_data *ts = i2c_get_clientdata(client);
> +
> +	if (ts->input)
> +		input_unregister_device(ts->input);

In what cases input device will not be present?

> +
> +	device_init_wakeup(&client->dev, false);
> +
> +	devm_free_irq(&client->dev, client->irq, ts);
> +
> +	mutex_destroy(&ts->sysfs_mutex);
> +
> +	devm_remove_action(&client->dev, raydium_i2c_remove_sysfs_group, 
> +ts);
> +
> +	devm_remove_action(&client->dev, raydium_i2c_power_off, ts);

The point of using devm_* API is not to free resources manually. Please remove devm_* calls from raydium_i2c_remove(), and see if the whole routine can be removed.

> +
> +	return 0;
> +}
> +
> +static void __maybe_unused raydium_enter_sleep(struct i2c_client 
> +*client) {
> +	static const u8 sleep_cmd[] = { 0x5A, 0xff, 0x00, 0x0f };
> +	int error;
> +
> +	error = raydium_i2c_send(client, CMD_ENTER_SLEEP, (u8 *)sleep_cmd,
> +		sizeof(sleep_cmd));
> +	if (error)
> +		dev_err(&client->dev,
> +			"Send sleep failed: %d\n", error); }
> +
> +static int __maybe_unused raydium_i2c_suspend(struct device *dev) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct raydium_data *ts = i2c_get_clientdata(client);
> +
> +	/* Command not support in BLDR recovery mode */
> +	if (ts->boot_mode != RAYDIUM_TS_MAIN)
> +		return -EBUSY;
> +
> +	disable_irq(client->irq);
> +
> +	if (device_may_wakeup(dev)) {
> +		raydium_enter_sleep(client);
> +
> +		ts->wake_irq_enabled = (enable_irq_wake(client->irq) == 0);
> +	} else {
> +		raydium_i2c_power_off(ts);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused raydium_i2c_resume(struct device *dev) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct raydium_data *ts = i2c_get_clientdata(client);
> +
> +	if (device_may_wakeup(dev)) {
> +		if (ts->wake_irq_enabled)
> +			disable_irq_wake(client->irq);
> +		raydium_i2c_sw_reset(client);
> +	} else {
> +		raydium_i2c_power_on(ts);
> +		raydium_i2c_initialize(ts);
> +	}
> +
> +	enable_irq(client->irq);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(raydium_i2c_pm_ops,
> +			 raydium_i2c_suspend, raydium_i2c_resume);
> +
> +static const struct i2c_device_id raydium_i2c_id[] = {
> +	{ DEVICE_NAME, 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, raydium_i2c_id);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id raydium_acpi_id[] = {
> +	{ "RMTS_0001", 0 },

As far as I know it is not a valid ACPI HID entry. In valid ACPI HIDs both vendor and product IDs are 4 characters long and consist of symbols in [A-Z0-9] range. This HID is 9 characters long and has invalid character (underscore).

What product uses this HID?

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, raydium_acpi_id); #endif
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id raydium_of_match[] = {
> +	{ .compatible = "raydium,rm-ts",},

We also prefer real model number in OF binding, so raydium,rm31100 would be better. For module autoloading you also want to add rm31100 entry to raydium_i2c_id.

Please add "raydium" entry to
Documentation/devicetree/bindings/vendor-prefixes.txt

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, raydium_of_match); #endif
> +
> +static struct i2c_driver raydium_i2c_driver = {
> +	.probe = raydium_i2c_probe,
> +	.remove = raydium_i2c_remove,
> +	.id_table = raydium_i2c_id,
> +	.driver = {
> +		.name = "raydium_ts",
> +		.pm = &raydium_i2c_pm_ops,
> +		.acpi_match_table = ACPI_PTR(raydium_acpi_id),
> +		.of_match_table = of_match_ptr(raydium_of_match),
> +	},
> +};
> +
> +module_i2c_driver(raydium_i2c_driver);
> +
> +MODULE_AUTHOR("Raydium");
> +MODULE_DESCRIPTION("Raydium I2c Touchscreen driver"); 
> +MODULE_LICENSE("GPL");

"GPL v2" since license notice at the top of the file specifies that only
v2 can be used.

Thanks.

--
Dmitry
?頨{.n?????%??橆??w?{.n????z筊{孎??雰}?笙??j:+v??茶庫全?2??霅??腄冠嗓??z蹂z嫡?+???▏?w噮f