Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752072AbcCJSrv (ORCPT ); Thu, 10 Mar 2016 13:47:51 -0500 Received: from mail-pf0-f177.google.com ([209.85.192.177]:34471 "EHLO mail-pf0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750929AbcCJSrm (ORCPT ); Thu, 10 Mar 2016 13:47:42 -0500 Date: Thu, 10 Mar 2016 10:47:38 -0800 From: Dmitry Torokhov To: "jeffrey.lin" Cc: rydberg@euromail.se, grant.likely@linaro.org, robh+dt@kernel.org, jeesw@melfas.com, bleung@chromium.org, scott.liu@emc.com.tw, jeffrey.lin@rad-ic.com, roger.yang@rad-ic.com, KP.li@rad-ic.com, albert.shieh@rad-ic.com, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver Message-ID: <20160310184738.GA485@dtor-ws> References: <1456987331-28049-1-git-send-email-jeffrey.lin@rad-ic.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1456987331-28049-1-git-send-email-jeffrey.lin@rad-ic.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 30580 Lines: 1136 Hi Jeffrey, On Thu, Mar 03, 2016 at 02:42:11PM +0800, jeffrey.lin wrote: > Raydium I2C touch driver. > > Signed-off-by: jeffrey.lin > --- > 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? > + 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include Why do you need this include? > +#include > +#include > +#include > +#include > +#include > +#include I do not think you need this one. > +#include > +#include > +#include > + > +/* 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. > + 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. > + 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