Thanks for your kind advice.
Please review my comments below.
I will submit the [V2] fixed code soon.
On Mon, Jan 11, 2016 at 12:37:20AM -0800, Dmitry Torokhov wrote:
> Hi,
>
> On Sun, Jan 10, 2016 at 02:52:20PM +0900, Sangwon Jee wrote:
> > This is an input driver for MELFAS MIP4 Touchscreen devices,
> > made by the original vendor, MELFAS.
> >
> > MIP4 means MELFAS Interface Protocol Version 4.
> > MELFAS MMS400, MMS500, MCS8000, MIT200, MIT300, MIT400, MFS10 and other
> > recent touchscreen devices are using MIP4 and supported by this driver.
> >
> > There are two MELFAS touchscreen drivers(mcs5000_ts, mms114) in the kernel.
> > But those drivers are for discontinued models and not supported by MELFAS.
>
> Thank you for you submission. Please find my comments from the 1st pass
> at the code below. Please fix the issues that I pointed out and resubmit
> for the next round of reviews.
>
> >
> > Signed-off-by: Sangwon Jee <jeesw@xxxxxxxxxx>
> > ---
> > drivers/input/touchscreen/Kconfig | 11 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/melfas_mip4.c | 2516 +++++++++++++++++++++++++++++++
> > include/linux/input/melfas_mip4.h | 33 +
> > 4 files changed, 2561 insertions(+)
> > create mode 100644 drivers/input/touchscreen/melfas_mip4.c
> > create mode 100644 include/linux/input/melfas_mip4.h
> >
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 53a97b3..f193049 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -491,6 +491,17 @@ config TOUCHSCREEN_MMS114
> > To compile this driver as a module, choose M here: the
> > module will be called mms114.
> >
> > +config TOUCHSCREEN_MELFAS_MIP4
> > + tristate "MELFAS MIP4 Touchscreen"
> > + depends on I2C
> > + help
> > + Say Y here if you have a MELFAS MIP4 Touchscreen device.
> > +
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here:
> > + the module will be called melfas_mip4.
> > +
> > config TOUCHSCREEN_MTOUCH
> > tristate "MicroTouch serial touchscreens"
> > select SERIO
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 968ff12..4b518c7 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_LPC32XX) += lpc32xx_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_MAX11801) += max11801_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_MC13783) += mc13783_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_MCS5000) += mcs5000_ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_MELFAS_MIP4) += melfas_mip4.o
> > obj-$(CONFIG_TOUCHSCREEN_MIGOR) += migor_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_MMS114) += mms114.o
> > obj-$(CONFIG_TOUCHSCREEN_MTOUCH) += mtouch.o
> > diff --git a/drivers/input/touchscreen/melfas_mip4.c b/drivers/input/touchscreen/melfas_mip4.c
> > new file mode 100644
> > index 0000000..5e42105
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/melfas_mip4.c
> > @@ -0,0 +1,2516 @@
> > +/*
> > + * MELFAS MIP4 Touchscreen
> > + *
> > + * Copyright (C) 2016 MELFAS Inc.
> > + *
> > + * Author : Sangwon Jee <jeesw@xxxxxxxxxx>
> > + *
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +/*****************************************************************
> > + * Include
> > + *****************************************************************/
> > +
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/firmware.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/slab.h>
> > +#include <linux/cdev.h>
> > +#include <linux/uaccess.h>
> > +#ifdef CONFIG_OF
> > +#include <linux/of_gpio.h>
> > +#endif
> > +#ifdef CONFIG_ACPI
> > +#include <linux/acpi.h>
> > +#endif
> > +
> > +/*****************************************************************
> > + * Platform
> > + *****************************************************************/
> > +
> > +#ifdef CONFIG_OF
> > +#define MIP_USE_DEVICETREE 1
> > +#else
> > +#define MIP_USE_DEVICETREE 0
> > +#endif
> > +
> > +#ifdef CONFIG_ACPI
> > +#define MIP_USE_ACPI 1
> > +#else
> > +#define MIP_USE_ACPI 0
> > +#endif
>
> Why do we need these defines? Simply use #ifdef CONFIG_* in your code.
Removed.
>
> > +
> > +#define MIP_DEVICE_NAME "mip4_ts"
> > +#define ACPI_ID "MLFS0000"
> > +
> > +/*****************************************************************
> > + * Protocol
> > + * Version : MIP 4.0 Rev 4.2
> > + *****************************************************************/
> > +
> > +/* Address */
> > +#define MIP_R0_BOOT 0x00
> > +#define MIP_R1_BOOT_MODE 0x01
> > +#define MIP_R1_BOOT_BUF_ADDR 0x10
> > +#define MIP_R1_BOOT_STATUS 0x20
> > +#define MIP_R1_BOOT_CMD 0x30
> > +#define MIP_R1_BOOT_TARGET_ADDR 0x40
> > +#define MIP_R1_BOOT_SIZE 0x44
> > +
> > +#define MIP_R0_INFO 0x01
> > +#define MIP_R1_INFO_PRODUCT_NAME 0x00
> > +#define MIP_R1_INFO_RESOLUTION_X 0x10
> > +#define MIP_R1_INFO_RESOLUTION_Y 0x12
> > +#define MIP_R1_INFO_NODE_NUM_X 0x14
> > +#define MIP_R1_INFO_NODE_NUM_Y 0x15
> > +#define MIP_R1_INFO_KEY_NUM 0x16
> > +#define MIP_R1_INFO_VERSION_BOOT 0x20
> > +#define MIP_R1_INFO_VERSION_CORE 0x22
> > +#define MIP_R1_INFO_VERSION_APP 0x24
> > +#define MIP_R1_INFO_VERSION_PARAM 0x26
> > +#define MIP_R1_INFO_SECT_BOOT_START 0x30
> > +#define MIP_R1_INFO_SECT_BOOT_END 0x31
> > +#define MIP_R1_INFO_SECT_CORE_START 0x32
> > +#define MIP_R1_INFO_SECT_CORE_END 0x33
> > +#define MIP_R1_INFO_SECT_APP_START 0x34
> > +#define MIP_R1_INFO_SECT_APP_END 0x35
> > +#define MIP_R1_INFO_SECT_PARAM_START 0x36
> > +#define MIP_R1_INFO_SECT_PARAM_END 0x37
> > +#define MIP_R1_INFO_BUILD_DATE 0x40
> > +#define MIP_R1_INFO_BUILD_TIME 0x44
> > +#define MIP_R1_INFO_CHECKSUM_PRECALC 0x48
> > +#define MIP_R1_INFO_CHECKSUM_REALTIME 0x4A
> > +#define MIP_R1_INFO_PROTOCOL_NAME 0x50
> > +#define MIP_R1_INFO_PROTOCOL_VERSION 0x58
> > +#define MIP_R1_INFO_IC_ID 0x70
> > +#define MIP_R1_INFO_IC_NAME 0x71
> > +#define MIP_R1_INFO_IC_VENDOR_ID 0x75
> > +#define MIP_R1_INFO_IC_HW_CATEGORY 0x77
> > +#define MIP_R1_INFO_CONTACT_THD_SCR 0x78
> > +#define MIP_R1_INFO_CONTACT_THD_KEY 0x7A
> > +
> > +#define MIP_R0_EVENT 0x02
> > +#define MIP_R1_EVENT_SUPPORTED_FUNC 0x00
> > +#define MIP_R1_EVENT_FORMAT 0x04
> > +#define MIP_R1_EVENT_SIZE 0x06
> > +#define MIP_R1_EVENT_PACKET_INFO 0x10
> > +#define MIP_R1_EVENT_PACKET_DATA 0x11
> > +
> > +#define MIP_R0_CTRL 0x06
> > +#define MIP_R1_CTRL_READY_STATUS 0x00
> > +#define MIP_R1_CTRL_EVENT_READY 0x01
> > +#define MIP_R1_CTRL_MODE 0x10
> > +#define MIP_R1_CTRL_EVENT_TRIGGER_TYPE 0x11
> > +#define MIP_R1_CTRL_RECALIBRATE 0x12
> > +#define MIP_R1_CTRL_POWER_STATE 0x13
> > +#define MIP_R1_CTRL_GESTURE_TYPE 0x14
> > +#define MIP_R1_CTRL_DISABLE_ESD_ALERT 0x18
> > +#define MIP_R1_CTRL_CHARGER_MODE 0x19
> > +#define MIP_R1_CTRL_HIGH_SENS_MODE 0x1A
> > +#define MIP_R1_CTRL_WINDOW_MODE 0x1B
> > +#define MIP_R1_CTRL_PALM_REJECTION 0x1C
> > +#define MIP_R1_CTRL_EDGE_CORRECTION 0x1D
> > +#define MIP_R1_CTRL_ENTER_GLOVE_MODE 0x1E
> > +#define MIP_R1_CTRL_I2C_ON_LPM 0x1F
> > +#define MIP_R1_CTRL_GESTURE_DEBUG 0x20
> > +#define MIP_R1_CTRL_PALM_EVENT 0x22
> > +#define MIP_R1_CTRL_PROXIMITY_SENSING 0x23
> > +
> > +#define MIP_R0_PARAM 0x08
> > +#define MIP_R1_PARAM_BUFFER_ADDR 0x00
> > +#define MIP_R1_PARAM_PROTOCOL 0x04
> > +#define MIP_R1_PARAM_MODE 0x10
> > +
> > +#define MIP_R0_TEST 0x0A
> > +#define MIP_R1_TEST_BUF_ADDR 0x00
> > +#define MIP_R1_TEST_PROTOCOL 0x02
> > +#define MIP_R1_TEST_TYPE 0x10
> > +#define MIP_R1_TEST_DATA_FORMAT 0x20
> > +#define MIP_R1_TEST_ROW_NUM 0x20
> > +#define MIP_R1_TEST_COL_NUM 0x21
> > +#define MIP_R1_TEST_BUFFER_COL_NUM 0x22
> > +#define MIP_R1_TEST_COL_AXIS 0x23
> > +#define MIP_R1_TEST_KEY_NUM 0x24
> > +#define MIP_R1_TEST_DATA_TYPE 0x25
> > +
> > +#define MIP_R0_IMAGE 0x0C
> > +#define MIP_R1_IMAGE_BUF_ADDR 0x00
> > +#define MIP_R1_IMAGE_PROTOCOL_ID 0x04
> > +#define MIP_R1_IMAGE_TYPE 0x10
> > +#define MIP_R1_IMAGE_DATA_FORMAT 0x20
> > +#define MIP_R1_IMAGE_ROW_NUM 0x20
> > +#define MIP_R1_IMAGE_COL_NUM 0x21
> > +#define MIP_R1_IMAGE_BUFFER_COL_NUM 0x22
> > +#define MIP_R1_IMAGE_COL_AXIS 0x23
> > +#define MIP_R1_IMAGE_KEY_NUM 0x24
> > +#define MIP_R1_IMAGE_DATA_TYPE 0x25
> > +#define MIP_R1_IMAGE_FINGER_NUM 0x30
> > +#define MIP_R1_IMAGE_FINGER_AREA 0x31
> > +
> > +#define MIP_R0_VENDOR 0x0E
> > +#define MIP_R1_VENDOR_INFO 0x00
> > +
> > +#define MIP_R0_LOG 0x10
> > +#define MIP_R1_LOG_TRIGGER 0x14
> > +
> > +/* Value */
> > +#define MIP_BOOT_MODE_BOOT 0x01
> > +#define MIP_BOOT_MODE_APP 0x02
> > +
> > +#define MIP_BOOT_STATUS_BUSY 0x05
> > +#define MIP_BOOT_STATUS_ERROR 0x0E
> > +#define MIP_BOOT_STATUS_DONE 0xA0
> > +
> > +#define MIP_BOOT_CMD_MASS_ERASE 0x15
> > +#define MIP_BOOT_CMD_PROGRAM 0x54
> > +#define MIP_BOOT_CMD_ERASE 0x8F
> > +#define MIP_BOOT_CMD_WRITE 0xA5
> > +#define MIP_BOOT_CMD_READ 0xC2
> > +
> > +#define MIP_EVENT_INPUT_TYPE_KEY 0
> > +#define MIP_EVENT_INPUT_TYPE_SCREEN 1
> > +#define MIP_EVENT_INPUT_TYPE_PROXIMITY 2
> > +
> > +#define MIP_EVENT_GESTURE_C 1
> > +#define MIP_EVENT_GESTURE_W 2
> > +#define MIP_EVENT_GESTURE_V 3
> > +#define MIP_EVENT_GESTURE_M 4
> > +#define MIP_EVENT_GESTURE_S 5
> > +#define MIP_EVENT_GESTURE_Z 6
> > +#define MIP_EVENT_GESTURE_O 7
> > +#define MIP_EVENT_GESTURE_E 8
> > +#define MIP_EVENT_GESTURE_V_90 9
> > +#define MIP_EVENT_GESTURE_V_180 10
> > +#define MIP_EVENT_GESTURE_FLICK_RIGHT 20
> > +#define MIP_EVENT_GESTURE_FLICK_DOWN 21
> > +#define MIP_EVENT_GESTURE_FLICK_LEFT 22
> > +#define MIP_EVENT_GESTURE_FLICK_UP 23
> > +#define MIP_EVENT_GESTURE_DOUBLE_TAP 24
> > +#define MIP_EVENT_GESTURE_ALL 0xFFFFFFFF
>
> Do we need these gestures defines? We generally do not use firmware
> gestures in Linux.
Unused defines are removed.
>
> > +
> > +#define MIP_ALERT_ESD 1
> > +#define MIP_ALERT_WAKEUP 2
> > +#define MIP_ALERT_INPUT_TYPE 3
> > +#define MIP_ALERT_IMAGE 4
> > +#define MIP_ALERT_F1 0xF1
> > +
> > +#define MIP_CTRL_STATUS_NONE 0x05
> > +#define MIP_CTRL_STATUS_READY 0xA0
> > +#define MIP_CTRL_STATUS_LOG 0x77
> > +
> > +#define MIP_CTRL_MODE_NORMAL 0
> > +#define MIP_CTRL_MODE_PARAM 1
> > +#define MIP_CTRL_MODE_TEST 2
> > +
> > +#define MIP_CTRL_TRIGGER_NONE 0
> > +#define MIP_CTRL_TRIGGER_INTR 1
> > +#define MIP_CTRL_TRIGGER_REG 2
> > +
> > +#define MIP_CTRL_POWER_ACTIVE 0
> > +#define MIP_CTRL_POWER_LOW 1
> > +
> > +#define MIP_TEST_TYPE_NONE 0
> > +#define MIP_TEST_TYPE_CM_DELTA 1
> > +#define MIP_TEST_TYPE_CM_ABS 2
> > +#define MIP_TEST_TYPE_CM_JITTER 3
> > +#define MIP_TEST_TYPE_SHORT 4
> > +#define MIP_TEST_TYPE_GPIO_LOW 5
> > +#define MIP_TEST_TYPE_GPIO_HIGH 6
> > +#define MIP_TEST_TYPE_SHORT2 7
> > +#define MIP_TEST_TYPE_OPEN 8
> > +#define MIP_TEST_TYPE_SELF_CP 20
> > +#define MIP_TEST_TYPE_SELF_DIFF_HOR 21
> > +#define MIP_TEST_TYPE_SELF_DIFF_VER 22
> > +#define MIP_TEST_TYPE_SELF_JITTER 23
> > +#define MIP_TEST_TYPE_HSELF_CP 24
> > +#define MIP_TEST_TYPE_HSELF_JITTER 25
> > +#define MIP_TEST_TYPE_HSELF_DIFF_HOR 26
> > +#define MIP_TEST_TYPE_HSELF_DIFF_VER 27
> > +
> > +#define MIP_IMG_TYPE_NONE 0
> > +#define MIP_IMG_TYPE_INTENSITY 1
> > +#define MIP_IMG_TYPE_RAWDATA 2
> > +#define MIP_IMG_TYPE_GESTURE 5
> > +#define MIP_IMG_TYPE_HSELF_RAWDATA 6
> > +#define MIP_IMG_TYPE_HSELF_INTENSITY 7
> > +
> > +#define MIP_LOG_MODE_NONE 0
> > +#define MIP_LOG_MODE_TRIG 1
> > +
> > +/*****************************************************************
> > + * Header
> > + *****************************************************************/
> > +
> > +/* Supported chip models */
> > +#define CHIP_NONE 000000
> > +#define CHIP_MMS427 104270
> > +#define CHIP_MMS438 104380
> > +#define CHIP_MMS449 104490
> > +#define CHIP_MMS458 104580
> > +#define CHIP_MMS500 105000
> > +#define CHIP_MCS8040L 280401
> > +#define CHIP_MIT200 302000
> > +#define CHIP_MIT300 303000
> > +#define CHIP_MIT400 304000
> > +#define CHIP_MFS10 400100
> > +
> > +/* Select a chip model */
> > +#define CHIP_MODEL CHIP_NONE
> > +
> > +/* Chip model info */
> > +#if CHIP_MODEL == CHIP_NONE
> > +#define CHIP_NAME "MIP4"
> > +#define FW_CHIP_CODE ""
> > +#endif
> > +#if CHIP_MODEL == CHIP_MMS427
> > +#define CHIP_NAME "MMS427"
> > +#define FW_CHIP_CODE "M4H2"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MMS438
> > +#define CHIP_NAME "MMS438"
> > +#define FW_CHIP_CODE "M4H0"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MMS449
> > +#define CHIP_NAME "MMS449"
> > +#define FW_CHIP_CODE "M4HP"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MMS458
> > +#define CHIP_NAME "MMS458"
> > +#define FW_CHIP_CODE "M4HP"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MMS500
> > +#define CHIP_NAME "MMS500"
> > +#define FW_CHIP_CODE "M5H0"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MCS8040L
> > +#define CHIP_NAME "MCS8040L"
> > +#define FW_CHIP_CODE "M8TL"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MIT200
> > +#define CHIP_NAME "MIT200"
> > +#define FW_CHIP_CODE "T2H0"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MIT300
> > +#define CHIP_NAME "MIT300"
> > +#define FW_CHIP_CODE "T3H0"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MIT400
> > +#define CHIP_NAME "MIT400"
> > +#define FW_CHIP_CODE "T4H0"
> > +#endif
> > +#if CHIP_MODEL == CHIP_MFS10
> > +#define CHIP_NAME "MFS10"
> > +#define FW_CHIP_CODE "MF10"
> > +#endif
>
> This should be runtime configurable, not compile time.
Removed.
>
> > +
> > +/* Config driver */
> > +#define MIP_USE_INPUT_OPEN_CLOSE 0 /* 0 (default) or 1 */
>
> Why would we not use open/close?
Removed.
>
> > +#define MIP_INPUT_REPORT_TYPE 1 /* 0 or 1 (default) */
> > +#define MIP_AUTOSET_RESOLUTION 1 /* 0 or 1 (default) */
> > +#define MIP_AUTOSET_EVENT_FORMAT 1 /* 0 or 1 (default) */
> > +#define I2C_RETRY_COUNT 3 /* 2~ */
> > +#define RESET_ON_I2C_ERROR 1 /* 0 or 1 (default) */
> > +#define RESET_ON_EVENT_ERROR 0 /* 0 (default) or 1 */
> > +#define ESD_COUNT_FOR_DISABLE 7 /* 7~ */
> > +
> > +/* Driver features */
> > +
> > +/* Module features */
> > +
> > +/* Input value */
> > +#define MAX_FINGER_NUM 10
> > +#define MAX_KEY_NUM 4
> > +#define INPUT_TOUCH_MAJOR_MIN 0
> > +#define INPUT_TOUCH_MAJOR_MAX 255
> > +#define INPUT_TOUCH_MINOR_MIN 0
> > +#define INPUT_TOUCH_MINOR_MAX 255
> > +#define INPUT_PRESSURE_MIN 0
> > +#define INPUT_PRESSURE_MAX 255
> > +
> > +/* Firmware update */
> > +#define FW_PATH_INTERNAL "melfas_mip4.fw"
> > +
> > +#define MIP_USE_AUTO_FW_UPDATE 0 /* 0 (default) or 1 */
>
> Why would we want this configurable?
Removed.
>
> > +#define MIP_FW_MAX_SECT_NUM 4
> > +#define MIP_FW_UPDATE_DEBUG 0 /* 0 (default) or 1 */
> > +#define MIP_FW_UPDATE_SECTION 0 /* 0 (default) or 1 */
> > +
> > +/*
> > +* Firmware update error code
> > +*/
> > +enum fw_update_errno {
> > + fw_err_file_read = -4,
> > + fw_err_file_open = -3,
> > + fw_err_file_type = -2,
> > + fw_err_download = -1,
> > + fw_err_none = 0,
> > + fw_err_uptodate = 1,
> > +};
> > +
> > +/*
> > +* Firmware file location
> > +*/
> > +enum fw_bin_source {
> > + fw_bin_source_kernel = 1,
> > + fw_bin_source_external = 2,
> > +};
> > +
> > +/*
> > +* Device info structure
> > +*/
> > +struct mip_ts_info {
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + struct melfas_mip4_platform_data *pdata;
> > + char phys[32];
> > + dev_t mip_dev;
> > + struct class *class;
> > + struct mutex lock;
> > + int irq;
> > + int gpio_intr;
> > + int gpio_ce;
> > + int gpio_vd33_en;
> > + struct regulator *regulator_vd33;
> > + struct pinctrl *pinctrl;
> > + struct pinctrl_state *pins_enable;
> > + struct pinctrl_state *pins_disable;
> > + bool init;
> > + bool enabled;
> > + bool irq_enabled;
> > + int power;
> > + char *fw_name;
> > + u8 product_name[16];
> > + int max_x;
> > + int max_y;
> > + u8 node_x;
> > + u8 node_y;
> > + u8 node_key;
> > + u8 fw_version[8];
> > + u8 event_size;
> > + int event_format;
> > + u8 touch_state[MAX_FINGER_NUM];
> > + bool key_enable;
> > + int key_num;
> > + int key_code[MAX_KEY_NUM];
> > + u8 gesture_wakeup_mode;
> > + u8 glove_mode;
> > + u8 charger_mode;
> > + u8 cover_mode;
> > + u8 esd_cnt;
> > + bool disable_esd;
> > + u8 *print_buf;
> > + u8 *debug_buf;
> > + int *image_buf;
> > + bool test_busy;
>
> Not used?
Removed.
>
> > + bool cmd_busy;
>
>
> Not used?
Removed.
>
> > + bool dev_busy;
>
> Same here?
Removed.
>
> > + struct cdev cdev;
> > + u8 *dev_fs_buf;
> > +};
> > +
> > +/*
> > +* Function declarations
> > +*/
> > +static int mip_get_fw_version(struct mip_ts_info *info, u8 *ver_buf);
> > +static int mip_get_fw_version_u16(struct mip_ts_info *info, u16 *ver_buf_u16);
> > +static int mip_power_on(struct mip_ts_info *info);
> > +static int mip_power_off(struct mip_ts_info *info);
> > +
> > +/*****************************************************************
> > + * Main
> > + *****************************************************************/
> > +
> > +/*
> > +* Reboot chip
> > +*/
> > +static void mip_reboot(struct mip_ts_info *info)
> > +{
> > + struct i2c_adapter *adapter = to_i2c_adapter(info->client->dev.parent);
> > +
> > + i2c_lock_adapter(adapter);
>
> Why do we need to lock entire bus segment for this?
Removed.
>
> > +
> > + mip_power_off(info);
> > + mip_power_on(info);
> > +
> > + i2c_unlock_adapter(adapter);
> > +}
> > +
> > +/*
> > +* I2C Read
> > +*/
> > +static int mip_i2c_read(struct mip_ts_info *info, char *write_buf,
> > + unsigned int write_len, char *read_buf, unsigned int read_len)
> > +{
> > + int retry = I2C_RETRY_COUNT;
> > + int res;
> > +
> > + struct i2c_msg msg[] = {
> > + {
> > + .addr = info->client->addr,
> > + .flags = 0,
> > + .buf = write_buf,
> > + .len = write_len,
> > + }, {
> > + .addr = info->client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = read_buf,
> > + .len = read_len,
> > + },
> > + };
> > +
> > + while (retry--) {
> > + res = i2c_transfer(info->client->adapter, msg, ARRAY_SIZE(msg));
> > +
> > + if (res == ARRAY_SIZE(msg))
> > + goto DONE;
>
> Please do not use all CAPS for labels.
Modified.
>
> > + else if (res < 0)
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_transfer - errno[%d]\n",
> > + __func__, res);
> > + else if (res != ARRAY_SIZE(msg))
>
> Given condition above (res == ARRAY_SIZE(msg)) if we get here this
> condition will always evaluate as true.
Modified.
>
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_transfer - size[%lu] result[%d]\n",
> > + __func__, ARRAY_SIZE(msg), res);
> > + else
>
> and this branch will never be taken.
Fixed.
>
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] unknown error [%d]\n",
> > + __func__, res);
> > + }
> > +
> > + goto ERROR_REBOOT;
> > +
> > +ERROR_REBOOT:
> > +#if RESET_ON_I2C_ERROR
> > + mip_reboot(info);
> > +#endif
> > + return 1;
> > +
> > +DONE:
> > + return 0;
> > +}
> > +
> > +/*
> > +* I2C Write
> > +*/
> > +static int mip_i2c_write(struct mip_ts_info *info, char *write_buf,
> > + unsigned int write_len)
> > +{
> > + int retry = I2C_RETRY_COUNT;
> > + int res;
> > +
> > + while (retry--) {
> > + res = i2c_master_send(info->client, write_buf, write_len);
> > +
> > + if (res == write_len)
> > + goto DONE;
> > + else if (res < 0)
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send - errno [%d]\n",
> > + __func__, res);
> > + else if (res != write_len)
>
> Same comment as in read.
Modified.
>
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] length mismatch - write[%d] result[%d]\n",
> > + __func__, write_len, res);
> > + else
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] unknown error [%d]\n",
> > + __func__, res);
> > + }
> > +
> > + goto ERROR_REBOOT;
> > +
> > +ERROR_REBOOT:
> > +#if RESET_ON_I2C_ERROR
> > + mip_reboot(info);
> > +#endif
> > + return 1;
> > +
> > +DONE:
> > + return 0;
> > +}
> > +
> > +/*****************************************************************
> > + * Platform dependent
> > + *****************************************************************/
> > +
> > +/*
> > +* Turn off power supply
> > +*/
> > +static int mip_power_off(struct mip_ts_info *info)
> > +{
> > + /* Control CE pin */
> > + /* gpio_direction_output(info->gpio_ce, 0); */
> > +
> > + /* Control VD33_EN */
> > + /* gpio_direction_output(info->gpio_vd33_en, 0); */
>
> Why is this all commented out?
Modified.
>
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Turn on power supply
> > +*/
> > +static int mip_power_on(struct mip_ts_info *info)
> > +{
> > + /* Control VD33_EN */
> > + /* gpio_direction_output(info->gpio_vd33_en, 1); */
> > +
> > + /* Control CE pin */
> > + /* gpio_direction_output(info->gpio_ce, 1); */
> > +
> > + /* Booting delay : 200~300ms */
> > + usleep_range(200 * 1000, 300 * 1000);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Clear touch input event status
> > +*/
> > +static void mip_clear_input(struct mip_ts_info *info)
> > +{
> > + int i;
> > +
> > + /* Screen */
> > + for (i = 0; i < MAX_FINGER_NUM; i++) {
> > + input_mt_slot(info->input_dev, i);
> > + input_mt_report_slot_state(info->input_dev,
> > + MT_TOOL_FINGER, 0);
> > +
> > + input_report_key(info->input_dev, BTN_TOUCH, 0);
> > +
> > + info->touch_state[i] = 0;
> > + }
> > +
> > + /* Key */
> > + if (info->key_enable == true) {
> > + for (i = 0; i < info->key_num; i++)
> > + input_report_key(info->input_dev, info->key_code[i], 0);
> > + }
> > +
> > + input_sync(info->input_dev);
> > +}
> > +
> > +/*
> > +* Input event handler - Report input event
> > +*/
> > +static void mip_input_event_handler(struct mip_ts_info *info, u8 sz, u8 *buf)
> > +{
> > + int i;
> > + int type;
> > + int id;
> > + int hover = 0;
> > + int palm = 0;
> > + int state = 0;
> > + int x, y, z;
> > + int size = 0;
> > + int pressure_stage = 0;
> > + int pressure = 0;
> > + int touch_major = 0;
> > + int touch_minor = 0;
> > + int finger_id = 0;
> > + int finger_cnt = 0;
> > +
> > + for (i = 0; i < sz; i += info->event_size) {
> > + u8 *packet = &buf[i];
> > +
> > + /* Event format & type */
> > + if ((info->event_format == 0) || (info->event_format == 1)) {
> > + type = (packet[0] & 0x40) >> 6;
> > + } else if (info->event_format == 3) {
> > + type = (packet[0] & 0xF0) >> 4;
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] Unknown event format[%d]\n",
> > + __func__, info->event_format);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev, "%s - Type[%d]\n",
> > + __func__, type);
> > +
> > + /* Report input event */
> > + if (type == MIP_EVENT_INPUT_TYPE_KEY) {
>
> This whole function could benefit from using "switch" statement.
Modified to "switch".
>
> > + /* Key event */
> > + if ((info->event_format == 0) ||
> > + (info->event_format == 1)) {
> > + id = packet[0] & 0x0F;
> > + state = (packet[0] & 0x80) >> 7;
> > + } else if (info->event_format == 3) {
> > + id = packet[0] & 0x0F;
> > + state = (packet[1] & 0x01);
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] Unknown event format[%d]\n",
> > + __func__, info->event_format);
> > + goto ERROR;
> > + }
> > +
> > + /* Report key event */
> > + if ((id >= 1) && (id <= info->key_num)) {
> > + int keycode = info->key_code[id - 1];
> > +
> > + input_report_key(info->input_dev,
> > + keycode, state);
> > +
> > + dev_dbg(&info->client->dev,
> > + "%s - Key : ID[%d] Code[%d] Event[%d]\n",
> > + __func__, id, keycode, state);
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] Unknown Key ID[%d]\n",
> > + __func__, id);
> > + continue;
> > + }
> > + } else if (type == MIP_EVENT_INPUT_TYPE_SCREEN) {
> > + /* Screen event */
> > + if (info->event_format == 0) {
> > + /* Touch only */
> > + state = (packet[0] & 0x80) >> 7;
> > + hover = (packet[0] & 0x20) >> 5;
> > + palm = (packet[0] & 0x10) >> 4;
> > + id = (packet[0] & 0x0F) - 1;
> > + x = ((packet[1] & 0x0F) << 8) | packet[2];
> > + y = (((packet[1] >> 4) & 0x0F) << 8) |
> > + packet[3];
> > + pressure = packet[4];
> > + size = packet[5];
> > + touch_major = packet[5];
> > + touch_minor = packet[5];
> > + } else if (info->event_format == 1) {
> > + /* Touch only */
> > + state = (packet[0] & 0x80) >> 7;
> > + hover = (packet[0] & 0x20) >> 5;
> > + palm = (packet[0] & 0x10) >> 4;
> > + id = (packet[0] & 0x0F) - 1;
> > + x = ((packet[1] & 0x0F) << 8) | packet[2];
> > + y = (((packet[1] >> 4) & 0x0F) << 8) |
> > + packet[3];
> > + pressure = packet[4];
> > + size = packet[5];
> > + touch_major = packet[6];
> > + touch_minor = packet[7];
>
> It looks like you can collapse these 2 branches with touch
> major/minor additionally filled when event format is 1.
I want to use this structure for the readability of
each event packet format. Is it OK?
>
> > + } else if (info->event_format == 3) {
> > + /* Touch + Force(Pressure) */
> > + id = (packet[0] & 0x0F) - 1;
> > + hover = (packet[1] & 0x04) >> 2;
> > + palm = (packet[1] & 0x02) >> 1;
> > + state = (packet[1] & 0x01);
> > + x = ((packet[2] & 0x0F) << 8) | packet[3];
> > + y = (((packet[2] >> 4) & 0x0F) << 8) |
> > + packet[4];
> > + z = packet[5];
> > + size = packet[6];
> > + pressure_stage = (packet[7] & 0xF0) >> 4;
> > + pressure = ((packet[7] & 0x0F) << 8) |
> > + packet[8];
> > + touch_major = packet[9];
> > + touch_minor = packet[10];
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] Unknown event format[%d]\n",
> > + __func__, info->event_format);
> > + goto ERROR;
> > + }
> > +
> > + /* Report screen event */
> > + if (state == 1) {
> > + /* Press or Move event */
> > + input_mt_slot(info->input_dev, id);
> > + input_mt_report_slot_state(info->input_dev,
> > + MT_TOOL_FINGER, true);
> > + input_report_abs(info->input_dev,
> > + ABS_MT_POSITION_X, x);
> > + input_report_abs(info->input_dev,
> > + ABS_MT_POSITION_Y, y);
> > + input_report_abs(info->input_dev,
> > + ABS_MT_PRESSURE, pressure);
> > + input_report_abs(info->input_dev,
> > + ABS_MT_TOUCH_MAJOR, touch_major);
> > + input_report_abs(info->input_dev,
> > + ABS_MT_TOUCH_MINOR, touch_minor);
> > +
> > + info->touch_state[id] = 1;
> > +
> > + /* First press event */
> > + finger_cnt = 0;
> > + for (finger_id = 0; finger_id < MAX_FINGER_NUM;
> > + finger_id++) {
> > + if (info->touch_state[finger_id] != 0) {
> > + finger_cnt++;
> > + break;
> > + }
> > + }
> > + if (finger_cnt == 1) {
> > + input_report_key(info->input_dev,
> > + BTN_TOUCH, 1);
> > + dev_dbg(&info->client->dev,
> > + "%s - Screen : Press\n",
> > + __func__);
> > + }
> > +
> > + dev_dbg(&info->client->dev,
> > + "%s - Screen : ID[%d] X[%d] Y[%d] Z[%d]\n",
> > + __func__, id, x, y, pressure);
> > + } else if (state == 0) {
> > + /* Release event */
> > + input_mt_slot(info->input_dev, id);
> > + input_mt_report_slot_state(info->input_dev,
> > + MT_TOOL_FINGER, 0);
> > +
> > + info->touch_state[id] = 0;
> > +
> > + dev_dbg(&info->client->dev,
> > + "%s - Screen : ID[%d] Release\n",
> > + __func__, id);
> > +
> > + /* Final release event */
> > + finger_cnt = 0;
> > + for (finger_id = 0; finger_id < MAX_FINGER_NUM;
> > + finger_id++) {
> > + if (info->touch_state[finger_id] != 0) {
> > + finger_cnt++;
> > + break;
> > + }
> > + }
> > + if (finger_cnt == 0) {
> > + input_report_key(info->input_dev,
> > + BTN_TOUCH, 0);
> > + dev_dbg(&info->client->dev,
> > + "%s - Screen : Release\n",
> > + __func__);
> > + }
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] Unknown event state[%d]\n",
> > + __func__, state);
> > + goto ERROR;
> > + }
> > + } else if (type == MIP_EVENT_INPUT_TYPE_PROXIMITY) {
> > + /* Proximity event */
> > + state = (packet[1] & 0x01);
> > + z = packet[5];
> > + dev_dbg(&info->client->dev,
> > + "%s - Proximity : State[%d] Value[%d]\n",
> > + __func__, state, z);
>
> Why not reporting it as ABS_DISTANCE?
Event type removed.
>
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] Unknown event type[%d]\n",
> > + __func__, type);
> > + goto ERROR;
> > + }
> > + }
> > +
> > + goto EXIT;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > +EXIT:
> > + input_sync(info->input_dev);
> > +}
> > +
> > +#if MIP_USE_DEVICETREE
> > +/*
> > +* Parse device tree
> > +*/
> > +static int mip_parse_devicetree(struct device *dev, struct mip_ts_info *info)
> > +{
> > + struct device_node *np = dev->of_node;
> > + int ret;
> > +
> > + /* Get GPIO */
> > + ret = of_get_named_gpio(np, MIP_DEVICE_NAME",irq-gpio", 0);
> > + if (!gpio_is_valid(ret)) {
> > + dev_err(dev,
> > + "%s [ERROR] of_get_named_gpio : irq-gpio\n",
> > + __func__);
> > + goto ERROR;
> > + } else{
> > + info->gpio_intr = ret;
> > + }
> > +
> > + /* Request GPIO */
> > + ret = gpio_request(info->gpio_intr, "irq-gpio");
> > + if (ret < 0) {
> > + dev_err(dev,
> > + "%s [ERROR] gpio_request : irq-gpio\n",
> > + __func__);
> > + goto ERROR;
> > + }
> > + gpio_direction_input(info->gpio_intr);
> > +
> > + /* Set IRQ */
> > + info->client->irq = gpio_to_irq(info->gpio_intr);
> > + dev_dbg(dev, "%s - gpio_to_irq : irq[%d]\n",
> > + __func__, info->client->irq);
>
>
> Why do you have it all here? I2C core will take care of setting up
> interrupt in client structure and the rest of platform code is
> responsible for setting up gpio as interrupt.
Removed.
>
> > +
> > + return 0;
> > +
> > +ERROR:
> > + dev_err(dev, "%s [ERROR]\n", __func__);
> > + return 1;
> > +}
> > +#endif
> > +
> > +/*
> > +* Config input device
> > +*/
> > +static void mip_config_input(struct mip_ts_info *info)
> > +{
> > + struct input_dev *input_dev = info->input_dev;
> > +
> > + set_bit(EV_SYN, input_dev->evbit);
> > + set_bit(EV_ABS, input_dev->evbit);
> > + set_bit(EV_KEY, input_dev->evbit);
> > + set_bit(BTN_TOUCH, input_dev->keybit);
> > +
> > + /* Screen */
> > + set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>
> The 5 set_bit()s above are not needed since you are calling
> input_mt_init_slots() with appropriate flag.
Removed.
>
> > +
> > + input_mt_init_slots(input_dev, MAX_FINGER_NUM, INPUT_MT_DIRECT);
>
> But you want to call it after you set up your axes.
Removed.
>
> > +
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
> > + info->max_x, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
> > + info->max_y, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0,
> > + INPUT_PRESSURE_MAX, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> > + INPUT_TOUCH_MAJOR_MAX, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0,
> > + INPUT_TOUCH_MINOR_MAX, 0, 0);
> > +
> > + /* Key */
> > + set_bit(KEY_BACK, input_dev->keybit);
> > + set_bit(KEY_MENU, input_dev->keybit);
> > +
> > + info->key_code[0] = KEY_BACK;
> > + info->key_code[1] = KEY_MENU;
>
> Isn't this platform-specific?
Removed.
>
> > +}
> > +
> > +/*****************************************************************
> > + * Firmware update
> > + *****************************************************************/
> > +
> > +/* Firmware Info */
> > +#define BL_PAGE_SIZE 512 /* 512 */
> > +#define BL_PACKET_SIZE 512 /* 512, 256, 128, 64, 32, 16, ... */
> > +
> > +/*
> > +* Firmware binary tail info
> > +*/
> > +struct mip_bin_tail {
> > + u8 tail_mark[4];
> > + char chip_name[4];
> > + u32 bin_start_addr;
> > + u32 bin_length;
> > +
> > + u16 ver_boot;
> > + u16 ver_core;
> > + u16 ver_app;
> > + u16 ver_param;
> > + u8 boot_start;
> > + u8 boot_end;
> > + u8 core_start;
> > + u8 core_end;
> > + u8 app_start;
> > + u8 app_end;
> > + u8 param_start;
> > + u8 param_end;
> > +
> > + u8 checksum_type;
> > + u8 hw_category;
> > + u16 param_id;
> > + u32 param_length;
> > + u32 build_date;
> > + u32 build_time;
> > +
> > + u32 reserved1;
> > + u32 reserved2;
> > + u16 reserved3;
> > + u16 tail_size;
> > + u32 crc;
> > +} __packed;
> > +
> > +#define MIP_BIN_TAIL_MARK {0x4D, 0x42, 0x54, 0x01} /* M B T 0x01 */
> > +#define MIP_BIN_TAIL_SIZE 64
> > +
> > +/*
> > +* Bootloader - Read status
> > +*/
> > +static int mip_bl_read_status(struct mip_ts_info *info)
> > +{
> > + u8 write_buf[2];
> > + u8 result = 0;
> > + int cnt = 1000;
> > + int ret = 0;
> > +
> > + struct i2c_msg msg[2] = {
> > + {
> > + .addr = info->client->addr,
> > + .flags = 0,
> > + .buf = write_buf,
> > + .len = 2,
> > + }, {
> > + .addr = info->client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = &result,
> > + .len = 1,
> > + },
> > + };
> > +
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_STATUS;
> > +
> > + do {
> > + if (i2c_transfer(info->client->adapter, msg, ARRAY_SIZE(msg))
> > + != ARRAY_SIZE(msg)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_transfer\n", __func__);
> > + ret = -1;
> > + goto ERROR;
> > + }
> > +
> > + if (result == MIP_BOOT_STATUS_DONE) {
>
> "switch"
Modified.
>
> > + dev_dbg(&info->client->dev, "%s - Done\n", __func__);
> > + ret = 0;
> > + break;
> > + } else if (result == MIP_BOOT_STATUS_BUSY) {
> > + dev_dbg(&info->client->dev, "%s - Busy\n", __func__);
> > + ret = -1;
>
> -EBUSY
Modified.
>
> > + usleep_range(1000, 2000);
> > + } else if (result == MIP_BOOT_STATUS_ERROR) {
> > + dev_dbg(&info->client->dev, "%s - Error\n", __func__);
> > + ret = -1;
>
> -EIO
Modified.
>
> > + goto ERROR;
> > + } else {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] wrong value [0x%02X]\n",
> > + __func__, result);
> > + ret = -1;
>
> -EINVAL
Modified.
>
> > + usleep_range(1000, 2000);
> > + }
> > + } while (--cnt);
> > +
> > + if (!cnt) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] count overflow - cnt[%d] status[0x%02X]\n",
> > + __func__, cnt, result);
> > + goto ERROR;
> > + }
> > +
> > + return ret;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return ret;
> > +}
> > +
> > +/*
> > +* Bootloader - Change mode
> > +*/
> > +static int mip_bl_change_mode(struct mip_ts_info *info, u8 mode)
> > +{
> > + u8 write_buf[8];
> > + u8 read_buf[8];
> > + int cnt = 10;
> > + int ret = 0;
> > +
> > + struct i2c_msg msg[2] = {
> > + {
> > + .addr = info->client->addr,
> > + .flags = 0,
> > + .buf = write_buf,
> > + .len = 2,
> > + }, {
> > + .addr = info->client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = read_buf,
> > + .len = 1,
> > + },
> > + };
> > +
> > + do {
> > + /* Write */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_MODE;
> > + write_buf[2] = mode;
> > + if (i2c_master_send(info->client, write_buf, 3) != 3) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev,
> > + "%s - Write : Mode [%d]\n", __func__, mode);
> > +
> > + /* Wait */
> > + msleep(1000);
> > +
> > + /* Read */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_MODE;
> > + if (i2c_transfer(info->client->adapter, msg, ARRAY_SIZE(msg))
> > + != ARRAY_SIZE(msg)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_transfer\n", __func__);
> > + ret = -1;
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev, "%s - Read : Mode [%d]\n",
> > + __func__, read_buf[0]);
> > +
> > + if (read_buf[0] == mode)
> > + break;
> > +
> > + } while (--cnt);
> > +
> > + if (!cnt) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] count overflow - cnt [%d]\n",
> > + __func__, cnt);
> > + goto ERROR;
> > + }
> > +
> > + return ret;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return ret;
> > +}
> > +
> > +/*
> > +* Bootloader - Read info
> > +*/
> > +static int mip_bl_read_info(struct mip_ts_info *info, u16 *buf_addr)
> > +{
> > + u8 write_buf[8];
> > + u8 read_buf[8];
> > + int ret = 0;
> > +
> > + struct i2c_msg msg[2] = {
> > + {
> > + .addr = info->client->addr,
> > + .flags = 0,
> > + .buf = write_buf,
> > + .len = 2,
> > + }, {
> > + .addr = info->client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = read_buf,
> > + .len = 2,
> > + },
> > + };
> > +
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_BUF_ADDR;
> > + if (i2c_transfer(info->client->adapter, msg, ARRAY_SIZE(msg))
> > + != ARRAY_SIZE(msg)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_transfer\n", __func__);
> > + ret = -1;
> > + goto ERROR;
> > + }
> > +
> > + *buf_addr = (u16)((read_buf[1] << 8) | read_buf[0]);
> > + dev_dbg(&info->client->dev,
> > + "%s - Buf Addr [0x%04X]\n", __func__, *buf_addr);
> > +
> > + return ret;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return ret;
> > +}
> > +
> > +/*
> > +* Bootloader - Program Page
> > +*/
> > +static int mip_bl_program_page(struct mip_ts_info *info, int offset,
> > + const u8 *data, int length, int buf_addr)
> > +{
> > + u8 write_buf[2 + BL_PAGE_SIZE];
> > + int buf_offset = 0;
> > +
> > + if (length > BL_PAGE_SIZE) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] page length overflow\n", __func__);
> > + goto ERROR;
> > + }
> > +
> > + /* Addr */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_TARGET_ADDR;
> > + write_buf[2] = (u8)(offset & 0xFF);
> > + write_buf[3] = (u8)((offset >> 8) & 0xFF);
> > + write_buf[4] = (u8)((offset >> 16) & 0xFF);
> > + write_buf[5] = (u8)((offset >> 24) & 0xFF);
>
> put_unaligned_le32()
Modified.
>
> > + if (i2c_master_send(info->client, write_buf, 6) != 6) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev,
> > + "%s - Addr [0x%06X]\n", __func__, offset);
> > +
> > + /* Size */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_SIZE;
> > + write_buf[2] = (u8)(length & 0xFF);
> > + write_buf[3] = (u8)((length >> 8) & 0xFF);
> > + write_buf[4] = (u8)((length >> 16) & 0xFF);
> > + write_buf[5] = (u8)((length >> 24) & 0xFF);
> > + if (i2c_master_send(info->client, write_buf, 6) != 6) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev,
> > + "%s - Size [%d]\n", __func__, length);
> > +
> > + /* Data */
> > + for (buf_offset = 0; buf_offset < length;
> > + buf_offset += BL_PACKET_SIZE) {
> > + write_buf[0] = (u8)(((buf_addr + buf_offset) >> 8) & 0xFF);
> > + write_buf[1] = (u8)((buf_addr + buf_offset) & 0xFF);
> > + memcpy(&write_buf[2], &data[buf_offset], BL_PACKET_SIZE);
> > + if (i2c_master_send(info->client, write_buf,
> > + (2 + BL_PACKET_SIZE))
> > + != (2 + BL_PACKET_SIZE)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev,
> > + "%s - PacketSize[%d] BufOffset[0x%04X]\n",
> > + __func__, BL_PACKET_SIZE, buf_offset);
> > + }
> > +
> > + /* Command */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_CMD;
> > + write_buf[2] = MIP_BOOT_CMD_PROGRAM;
> > + if (i2c_master_send(info->client, write_buf, 3) != 3) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > +
> > + /* Status */
> > + if (mip_bl_read_status(info) != 0)
> > + goto ERROR;
> > +
> > + return 0;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return -1;
> > +}
> > +
> > +/*
> > +* Bootloader - Read Page
> > +*/
> > +static int mip_bl_read_page(struct mip_ts_info *info, int offset,
> > + u8 *data, int length, int buf_addr)
> > +{
> > + u8 write_buf[8];
> > + u8 read_buf[BL_PACKET_SIZE];
> > + int buf_offset = 0;
> > +
> > + struct i2c_msg msg[2] = {
> > + {
> > + .addr = info->client->addr,
> > + .flags = 0,
> > + .buf = write_buf,
> > + .len = 2,
> > + }, {
> > + .addr = info->client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = read_buf,
> > + .len = BL_PACKET_SIZE,
> > + },
> > + };
> > +
> > + /* Addr */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_TARGET_ADDR;
> > + write_buf[2] = (u8)(offset & 0xFF);
> > + write_buf[3] = (u8)((offset >> 8) & 0xFF);
> > + write_buf[4] = (u8)((offset >> 16) & 0xFF);
> > + write_buf[5] = (u8)((offset >> 24) & 0xFF);
> > + if (i2c_master_send(info->client, write_buf, 6) != 6) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev, "%s - Addr [0x%06X]\n",
> > + __func__, offset);
> > +
> > + /* Size */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_SIZE;
> > + write_buf[2] = (u8)(length & 0xFF);
> > + write_buf[3] = (u8)((length >> 8) & 0xFF);
> > + write_buf[4] = (u8)((length >> 16) & 0xFF);
> > + write_buf[5] = (u8)((length >> 24) & 0xFF);
> > + if (i2c_master_send(info->client, write_buf, 6) != 6) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > + dev_dbg(&info->client->dev,
> > + "%s - Size [%d]\n", __func__, length);
> > +
> > + /* Command */
> > + write_buf[0] = MIP_R0_BOOT;
> > + write_buf[1] = MIP_R1_BOOT_CMD;
> > + write_buf[2] = MIP_BOOT_CMD_READ;
> > + if (i2c_master_send(info->client, write_buf, 3) != 3) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_master_send\n", __func__);
> > + goto ERROR;
> > + }
> > +
> > + /* Status */
> > + if (mip_bl_read_status(info) != 0)
> > + goto ERROR;
> > +
> > + /* Read */
> > + for (buf_offset = 0; buf_offset < length;
> > + buf_offset += BL_PACKET_SIZE) {
> > + write_buf[0] = (u8)(((buf_addr + buf_offset) >> 8) & 0xFF);
> > + write_buf[1] = (u8)((buf_addr + buf_offset) & 0xFF);
> > + if (i2c_transfer(info->client->adapter, msg, ARRAY_SIZE(msg))
> > + != ARRAY_SIZE(msg)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] i2c_transfer\n", __func__);
> > + goto ERROR;
> > + }
> > + memcpy(&data[buf_offset], read_buf, BL_PACKET_SIZE);
> > + dev_dbg(&info->client->dev,
> > + "%s - PacketSize[%d] BufOffset [0x%04X]\n",
> > + __func__, BL_PACKET_SIZE, buf_offset);
> > + }
> > +
> > + return 0;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return -1;
> > +}
> > +
> > +/*
> > +* Bootloader - Start bootloader mode
> > +*/
> > +static int mip_bl_enter(struct mip_ts_info *info)
> > +{
> > + if (mip_bl_change_mode(info, MIP_BOOT_MODE_BOOT) != 0)
> > + goto ERROR;
> > +
> > + return 0;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return -1;
> > +}
> > +
> > +/*
> > +* Bootloader - Exit bootloader mode
> > +*/
> > +static int mip_bl_exit(struct mip_ts_info *info)
> > +{
> > + if (mip_bl_change_mode(info, MIP_BOOT_MODE_APP) != 0)
> > + goto ERROR;
> > +
> > + return 0;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return -1;
> > +}
> > +
> > +/*
> > +* Flash chip firmware
> > +*/
> > +static int mip_flash_fw(struct mip_ts_info *info, const u8 *fw_data,
> > + size_t fw_size, bool force, bool section)
> > +{
> > + struct i2c_client *client = info->client;
> > + struct mip_bin_tail *bin_info;
> > + int ret = 0;
> > + int retry = 3;
> > + u8 rbuf[BL_PAGE_SIZE];
> > + int offset = 0;
> > + u32 offset_start = 0;
> > + u32 offset_end = 0;
> > + int bin_size = 0;
> > + u8 *bin_data;
> > + u16 tail_size = 0;
> > + u8 tail_mark[4] = MIP_BIN_TAIL_MARK;
> > + u16 ver_chip[MIP_FW_MAX_SECT_NUM];
> > + u16 buf_addr = 0;
> > +
> > + /* Check tail size */
> > + tail_size = (fw_data[fw_size - 5] << 8) | fw_data[fw_size - 6];
> > + if (tail_size != MIP_BIN_TAIL_SIZE) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] wrong tail size [%d]\n",
> > + __func__, tail_size);
> > + ret = fw_err_file_type;
> > + goto ERROR_FILE;
> > + }
> > +
> > + /* Check bin format */
> > + if (memcmp(&fw_data[fw_size - tail_size], tail_mark, 4)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] wrong tail mark\n", __func__);
> > + ret = fw_err_file_type;
> > + goto ERROR_FILE;
> > + }
> > +
> > + /* Read bin info */
> > + bin_info = (struct mip_bin_tail *)&fw_data[fw_size - tail_size];
> > +
> > +#if MIP_FW_UPDATE_DEBUG
> > + print_hex_dump(KERN_ERR, MIP_DEVICE_NAME " Bin Info : ",
> > + DUMP_PREFIX_OFFSET, 16, 1, bin_info, tail_size, false);
> > +#endif
> > +
> > + /* Check chip code */
> > + if (memcmp(bin_info->chip_name, FW_CHIP_CODE, 4)) {
> > + dev_info(&client->dev,
> > + "F/W file is not for %s\n", CHIP_NAME);
> > + ret = fw_err_file_type;
> > + goto ERROR_FILE;
> > + }
> > +
> > + /* Check F/W version */
> > + dev_info(&client->dev,
> > + "F/W file version [0x%04X 0x%04X 0x%04X 0x%04X]\n",
> > + bin_info->ver_boot, bin_info->ver_core,
> > + bin_info->ver_app, bin_info->ver_param);
> > +
> > + if (force == true) {
> > + /* Force update */
> > + dev_info(&client->dev,
> > + "Skip chip firmware version check\n");
> > + } else {
> > + /* Read firmware version from chip */
> > + while (retry--) {
> > + if (mip_get_fw_version_u16(info, ver_chip))
> > + mip_reboot(info);
> > + else
> > + break;
> > + }
> > + if (retry < 0) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] Unknown chip firmware version\n",
> > + __func__);
> > + offset_start = 0;
> > + } else {
> > + dev_info(&client->dev,
> > + "Chip firmware version [0x%04X 0x%04X 0x%04X 0x%04X]\n",
> > + ver_chip[0], ver_chip[1],
> > + ver_chip[2], ver_chip[3]);
> > +
> > + /* Compare version */
> > + if ((ver_chip[0] == bin_info->ver_boot) &&
> > + (ver_chip[1] == bin_info->ver_core) &&
> > + (ver_chip[2] == bin_info->ver_app) &&
> > + (ver_chip[3] == bin_info->ver_param)) {
> > + dev_info(&client->dev,
> > + "Chip firmware is already up-to-date\n");
> > + ret = fw_err_uptodate;
> > + goto UPTODATE;
> > + }
> > + }
> > + }
> > +
> > + /* Read bin data */
> > + bin_size = bin_info->bin_length;
> > + bin_data = kzalloc(sizeof(u8) * (bin_size), GFP_KERNEL);
> > + memcpy(bin_data, fw_data, bin_size);
> > +
> > + /* Enter bootloader mode */
> > + dev_dbg(&client->dev,
> > + "%s - Enter bootloader mode\n", __func__);
> > +
> > + if (mip_bl_enter(info) != 0) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_bl_enter\n", __func__);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > +
> > + /* Read info */
> > + if (mip_bl_read_info(info, &buf_addr)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_bl_read_info\n", __func__);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > + dev_dbg(&client->dev,
> > + "%s - Buffer Addr [0x%04X]\n", __func__, buf_addr);
> > +
> > + /* Program & Verify */
> > + dev_dbg(&client->dev,
> > + "%s - Program & Verify\n", __func__);
> > +
> > + dev_dbg(&client->dev,
> > + "%s - Size : Page[%d] Packet[%d]\n",
> > + __func__, BL_PAGE_SIZE, BL_PACKET_SIZE);
> > +
> > + offset_start = bin_info->bin_start_addr;
> > + offset_end = bin_size - BL_PAGE_SIZE;
> > + dev_dbg(&client->dev,
> > + "%s - Offset : Start[0x%08X] End[0x%08X]\n",
> > + __func__, offset_start, offset_end);
> > +
> > + for (offset = offset_start; offset < bin_size;
> > + offset += BL_PAGE_SIZE) {
> > + /* Program */
> > + if (mip_bl_program_page(info, offset, &bin_data[offset],
> > + BL_PAGE_SIZE, buf_addr)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_bl_program_page : offset[0x%08X]\n",
> > + __func__, offset);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > + dev_dbg(&client->dev,
> > + "%s - mip_bl_program_page : offset[0x%08X]\n",
> > + __func__, offset);
> > +
> > + /* Verify */
> > + if (mip_bl_read_page
> > + (info, offset, rbuf, BL_PAGE_SIZE, buf_addr)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_bl_read_page : offset[0x%08X]\n",
> > + __func__, offset);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > + dev_dbg(&client->dev,
> > + "%s - mip_bl_read_page : offset[0x%08X]\n",
> > + __func__, offset);
> > +
> > +#if MIP_FW_UPDATE_DEBUG
> > + print_hex_dump(KERN_ERR, MIP_DEVICE_NAME " F/W File : ",
> > + DUMP_PREFIX_OFFSET, 16, 1, &bin_data[offset],
> > + BL_PAGE_SIZE, false);
> > + print_hex_dump(KERN_ERR, MIP_DEVICE_NAME " F/W Chip : ",
> > + DUMP_PREFIX_OFFSET, 16, 1, rbuf,
> > + BL_PAGE_SIZE, false);
> > +#endif
> > +
> > + if (memcmp(rbuf, &bin_data[offset], BL_PAGE_SIZE)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] Verify failed : offset[0x%08X]\n",
> > + __func__, offset);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > + }
> > +
> > + /* Exit bootloader mode */
> > + dev_dbg(&client->dev,
> > + "%s - Exit bootloader mode\n", __func__);
> > +
> > + if (mip_bl_exit(info) != 0) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_bl_exit\n", __func__);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > +
> > + /* Reset chip */
> > + mip_reboot(info);
> > +
> > + /* Check chip firmware version */
> > + if (mip_get_fw_version_u16(info, ver_chip)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] Unknown chip firmware version\n",
> > + __func__);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + } else {
> > + if ((ver_chip[1] == bin_info->ver_core) &&
> > + (ver_chip[2] == bin_info->ver_app) &&
> > + (ver_chip[3] == bin_info->ver_param)) {
> > + dev_dbg(&client->dev,
> > + "%s - Version check OK\n", __func__);
> > + } else {
> > + dev_err(&client->dev,
> > + "Version mismatch after flash.\n");
> > + dev_err(&client->dev,
> > + "Chip[0x%04X 0x%04X 0x%04X 0x%04X]\n",
> > + ver_chip[0], ver_chip[1],
> > + ver_chip[2], ver_chip[3]);
> > + dev_err(&client->dev,
> > + "File[0x%04X 0x%04X 0x%04X 0x%04X]\n",
> > + bin_info->ver_boot, bin_info->ver_core,
> > + bin_info->ver_app, bin_info->ver_param);
> > + ret = fw_err_download;
> > + goto ERROR_UPDATE;
> > + }
> > + }
> > +
> > + kfree(bin_data);
> > +
> > +UPTODATE:
> > + goto EXIT;
> > +
> > +ERROR_UPDATE:
> > + kfree(bin_data);
> > +
> > + /* Reset chip */
> > + mip_reboot(info);
> > +
> > +ERROR_FILE:
> > + dev_err(&client->dev, "%s [ERROR]\n", __func__);
> > +
> > +EXIT:
> > + return ret;
> > +}
> > +
> > +/*****************************************************************
> > + * Development
> > + *****************************************************************/
> > +
> > +/*
> > +* Dev node output to user
> > +*/
> > +static ssize_t mip_dev_fs_read(struct file *fp, char *rbuf,
> > + size_t cnt, loff_t *fpos)
> > +{
> > + struct mip_ts_info *info = fp->private_data;
> > + int ret = 0;
> > +
> > + ret = copy_to_user(rbuf, info->dev_fs_buf, cnt);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > +* Dev node input from user
> > +*/
> > +static ssize_t mip_dev_fs_write(struct file *fp, const char *wbuf,
> > + size_t cnt, loff_t *fpos)
> > +{
> > + struct mip_ts_info *info = fp->private_data;
> > + u8 *buf;
> > + int ret = 0;
> > + int cmd = 0;
> > +
> > + buf = kzalloc(cnt + 1, GFP_KERNEL);
> > +
> > + if ((buf == NULL) || copy_from_user(buf, wbuf, cnt)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] copy_from_user\n", __func__);
> > + ret = -EIO;
> > + goto EXIT;
> > + }
> > +
> > + cmd = buf[cnt - 1];
> > +
> > + if (cmd == 1) {
> > + if (mip_i2c_read
> > + (info, buf, (cnt - 2), info->dev_fs_buf, buf[cnt - 2]))
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] mip_i2c_read\n", __func__);
> > + } else if (cmd == 2) {
> > + if (mip_i2c_write(info, buf, (cnt - 1)))
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] mip_i2c_write\n", __func__);
> > + } else {
> > + goto EXIT;
> > + }
> > +
> > +EXIT:
> > + kfree(buf);
> > + return ret;
> > +}
> > +
> > +/*
> > +* Open dev node
> > +*/
> > +static int mip_dev_fs_open(struct inode *node, struct file *fp)
> > +{
> > + struct mip_ts_info *info = container_of(node->i_cdev,
> > + struct mip_ts_info, cdev);
> > +
> > + fp->private_data = info;
> > +
> > + info->dev_fs_buf = kzalloc(1024 * 4, GFP_KERNEL);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Close dev node
> > +*/
> > +static int mip_dev_fs_release(struct inode *node, struct file *fp)
> > +{
> > + struct mip_ts_info *info = fp->private_data;
> > +
> > + kfree(info->dev_fs_buf);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Dev node info
> > +*/
> > +static const struct file_operations mip_dev_fops = {
> > + .owner = THIS_MODULE,
> > + .open = mip_dev_fs_open,
> > + .release = mip_dev_fs_release,
> > + .read = mip_dev_fs_read,
> > + .write = mip_dev_fs_write,
> > +};
> > +
> > +/*
> > +* Create dev node
> > +*/
> > +static int mip_dev_create(struct mip_ts_info *info)
> > +{
> > + struct i2c_client *client = info->client;
> > + int ret = 0;
> > +
> > + if (alloc_chrdev_region(&info->mip_dev, 0, 1, MIP_DEVICE_NAME)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] alloc_chrdev_region\n", __func__);
> > + ret = -ENOMEM;
> > + goto ERROR;
> > + }
> > +
> > + cdev_init(&info->cdev, &mip_dev_fops);
> > + info->cdev.owner = THIS_MODULE;
> > +
> > + if (cdev_add(&info->cdev, info->mip_dev, 1)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] cdev_add\n", __func__);
> > + ret = -EIO;
> > + goto ERROR;
> > + }
> > +
> > + return 0;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return 0;
> > +}
>
> Please remove custom device interface. If this is purely for
> development of the board to check if device responds you can access it
> form userspace (via i2c dev interface).
Removed.
>
> > +
> > +/*
> > +* Print chip firmware version
> > +*/
> > +static ssize_t mip_sys_fw_version(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct mip_ts_info *info = dev_get_drvdata(dev);
> > + u8 data[255];
> > + int ret;
> > + u8 rbuf[16];
> > +
> > + memset(info->print_buf, 0, PAGE_SIZE);
> > +
> > + if (mip_get_fw_version(info, rbuf)) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] mip_get_fw_version\n", __func__);
> > +
> > + sprintf(data, "F/W Version : ERROR\n");
> > + goto ERROR;
> > + }
> > +
> > + sprintf(data,
> > + "F/W Version : %02X.%02X/%02X.%02X/%02X.%02X/%02X.%02X\n",
> > + rbuf[0], rbuf[1], rbuf[2], rbuf[3],
> > + rbuf[4], rbuf[5], rbuf[6], rbuf[7]);
> > +
> > +ERROR:
> > + strcat(info->print_buf, data);
> > +
> > + ret = snprintf(buf, PAGE_SIZE, "%s\n", info->print_buf);
> > + return ret;
> > +}
> > +
> > +/*
> > +* Sysfs functions
> > +*/
> > +static DEVICE_ATTR(fw_version, 0660, mip_sys_fw_version, NULL);
> > +
> > +/*
> > +* Sysfs attr list info
> > +*/
> > +static struct attribute *mip_sys_attr[] = {
> > + &dev_attr_fw_version.attr,
> > + NULL,
> > +};
> > +
> > +/*
> > +* Sysfs attr group info
> > +*/
> > +static const struct attribute_group mip_sys_attr_group = {
> > + .attrs = mip_sys_attr,
> > +};
> > +
> > +/*
> > +* Create sysfs test functions
> > +*/
> > +static int mip_sysfs_create(struct mip_ts_info *info)
> > +{
> > + struct i2c_client *client = info->client;
> > +
> > + if (sysfs_create_group(&client->dev.kobj, &mip_sys_attr_group)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] sysfs_create_group\n", __func__);
> > + return -EAGAIN;
> > + }
> > +
> > + info->print_buf = kzalloc(sizeof(u8) * PAGE_SIZE, GFP_KERNEL);
>
> I do not believe you need to allocate whole page for this. In fact, in
> most places you use print_buf you already have buffer of sufficient size
> you can use directly.
Removed.
>
> > + info->debug_buf = kzalloc(sizeof(u8) * PAGE_SIZE, GFP_KERNEL);
>
> I do not see debug_buf being used.
Removed.
>
> > + info->image_buf = kzalloc(sizeof(int) * PAGE_SIZE, GFP_KERNEL);
>
> I do not see it being used either.
Removed.
>
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Remove sysfs test functions
> > +*/
> > +static void mip_sysfs_remove(struct mip_ts_info *info)
> > +{
> > + sysfs_remove_group(&info->client->dev.kobj, &mip_sys_attr_group);
> > +
> > + kfree(info->print_buf);
> > + kfree(info->debug_buf);
> > + kfree(info->image_buf);
> > +}
> > +
> > +/*****************************************************************
> > + * Main
> > + *****************************************************************/
> > +
> > +/*
> > +* Enable device
> > +*/
> > +static int mip_enable(struct mip_ts_info *info)
> > +{
> > + if (info->enabled) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] device already enabled\n", __func__);
> > + goto EXIT;
> > + }
> > +
> > + mip_power_on(info);
> > +
> > + mutex_lock(&info->lock);
> > +
> > + if (info->irq_enabled == false) {
> > + enable_irq(info->client->irq);
> > + info->irq_enabled = true;
> > + }
> > +
> > + info->enabled = true;
> > +
> > + mutex_unlock(&info->lock);
> > +
> > +EXIT:
> > + dev_info(&info->client->dev, MIP_DEVICE_NAME" - Enabled\n");
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Disable device
> > +*/
> > +static int mip_disable(struct mip_ts_info *info)
> > +{
> > + if (!info->enabled) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] device already disabled\n", __func__);
> > + goto EXIT;
> > + }
> > +
> > + mutex_lock(&info->lock);
> > +
> > + disable_irq(info->client->irq);
> > + info->irq_enabled = false;
> > +
> > + mutex_unlock(&info->lock);
> > +
> > + mip_power_off(info);
> > +
> > + mip_clear_input(info);
> > +
> > + info->enabled = false;
> > +
> > +EXIT:
> > + dev_info(&info->client->dev, MIP_DEVICE_NAME" - Disabled\n");
> > +
> > + return 0;
> > +}
> > +
> > +#if MIP_USE_INPUT_OPEN_CLOSE
> > +/*
> > +* Open input device
> > +*/
> > +static int mip_input_open(struct input_dev *dev)
> > +{
> > + struct mip_ts_info *info = input_get_drvdata(dev);
> > +
> > + if (info->init == true)
> > + info->init = false;
>
> No, please shut off the controller in probe() and then power it up here
> unconditionally.
Removed.
>
> > + else
> > + mip_enable(info);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Close input device
> > +*/
> > +static void mip_input_close(struct input_dev *dev)
> > +{
> > + struct mip_ts_info *info = input_get_drvdata(dev);
> > +
> > + mip_disable(info);
> > +}
> > +#endif
> > +
> > +/*
> > +* Read chip firmware version
> > +*/
> > +static int mip_get_fw_version(struct mip_ts_info *info, u8 *ver_buf)
> > +{
> > + u8 rbuf[8];
> > + u8 wbuf[2];
> > + int i;
> > +
> > + wbuf[0] = MIP_R0_INFO;
> > + wbuf[1] = MIP_R1_INFO_VERSION_BOOT;
> > + if (mip_i2c_read(info, wbuf, 2, rbuf, 8)) {
> > + goto ERROR;
> > + };
> > +
> > + for (i = 0; i < MIP_FW_MAX_SECT_NUM; i++) {
> > + ver_buf[0 + i * 2] = rbuf[1 + i * 2];
> > + ver_buf[1 + i * 2] = rbuf[0 + i * 2];
> > + }
> > +
> > + return 0;
> > +
> > +ERROR:
> > + memset(ver_buf, 0xFF, 8);
> > +
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return 1;
> > +}
> > +
> > +/*
> > +* Read chip firmware version for u16
> > +*/
> > +static int mip_get_fw_version_u16(struct mip_ts_info *info, u16 *ver_buf_u16)
> > +{
> > + u8 rbuf[8];
> > + int i;
> > +
> > + if (mip_get_fw_version(info, rbuf))
> > + goto ERROR;
> > +
> > + for (i = 0; i < MIP_FW_MAX_SECT_NUM; i++)
> > + ver_buf_u16[i] = (rbuf[0 + i * 2] << 8) | rbuf[1 + i * 2];
> > +
> > + return 0;
> > +
> > +ERROR:
> > + memset(ver_buf_u16, 0xFFFF, 4);
> > +
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return 1;
> > +}
> > +
> > +/*
> > +* Interrupt handler
> > +*/
> > +static irqreturn_t mip_interrupt(int irq, void *dev_id)
> > +{
> > + struct mip_ts_info *info = dev_id;
> > + struct i2c_client *client = info->client;
> > + u8 wbuf[8];
> > + u8 rbuf[256];
> > + unsigned int size = 0;
> > + u8 category = 0;
> > + u8 alert_type = 0;
> > +
> > + /* Read packet info */
> > + wbuf[0] = MIP_R0_EVENT;
> > + wbuf[1] = MIP_R1_EVENT_PACKET_INFO;
> > + if (mip_i2c_read(info, wbuf, 2, rbuf, 1)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] Read packet info\n", __func__);
> > + goto ERROR;
> > + }
> > +
> > + size = (rbuf[0] & 0x7F);
> > + category = ((rbuf[0] >> 7) & 0x1);
> > + dev_dbg(&client->dev,
> > + "%s - packet info : size[%d] category[%d]\n",
> > + __func__, size, category);
> > +
> > + /* Check size */
> > + if (size <= 0) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] Packet size [%d]\n",
> > + __func__, size);
> > + goto EXIT;
> > + }
> > +
> > + /* Read packet data */
> > + wbuf[0] = MIP_R0_EVENT;
> > + wbuf[1] = MIP_R1_EVENT_PACKET_DATA;
> > + if (mip_i2c_read(info, wbuf, 2, rbuf, size)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] Read packet data\n",
> > + __func__);
> > + goto ERROR;
> > + }
> > +
> > + /* Event handler */
> > + if (category == 0) {
> > + /* Touch event */
> > + info->esd_cnt = 0;
> > +
> > + mip_input_event_handler(info, size, rbuf);
> > + } else {
> > + /* Alert event */
> > + alert_type = rbuf[0];
> > +
> > + dev_dbg(&client->dev,
> > + "%s - alert type [%d]\n",
> > + __func__, alert_type);
> > + }
> > +
> > +EXIT:
> > + return IRQ_HANDLED;
> > +
> > +ERROR:
> > + if (RESET_ON_EVENT_ERROR) {
> > + dev_info(&client->dev,
> > + "%s - Reset on error\n", __func__);
> > +
> > + mip_disable(info);
>
> If I remember correctly you disable IRQ in mip_disable(). This will not
> work from interrupt handler. If you really want to reset on error (by
> the way, most drivers do not) you are likely have to do this from a
> workqueue.
Removed.
>
> > + mip_clear_input(info);
> > + mip_enable(info);
> > + }
> > +
> > + dev_err(&client->dev, "%s [ERROR]\n", __func__);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > +* Initial config
> > +*/
> > +static int mip_init_config(struct mip_ts_info *info)
> > +{
> > + u8 wbuf[8];
> > + u8 rbuf[64];
> > +
> > + /* Product name */
> > + wbuf[0] = MIP_R0_INFO;
> > + wbuf[1] = MIP_R1_INFO_PRODUCT_NAME;
> > + mip_i2c_read(info, wbuf, 2, rbuf, 16);
> > + memcpy(info->product_name, rbuf, 16);
> > + dev_dbg(&info->client->dev,
> > + "%s - product_name[%s]\n",
> > + __func__, info->product_name);
> > +
> > + /* Firmware version */
> > + mip_get_fw_version(info, rbuf);
> > + memcpy(info->fw_version, rbuf, 8);
> > + dev_info(&info->client->dev,
> > + "%s - F/W Version : %02X.%02X %02X.%02X %02X.%02X %02X.%02X\n",
> > + __func__, info->fw_version[0], info->fw_version[1],
> > + info->fw_version[2], info->fw_version[3], info->fw_version[4],
> > + info->fw_version[5], info->fw_version[6], info->fw_version[7]);
> > +
> > + /* Resolution */
> > + wbuf[0] = MIP_R0_INFO;
> > + wbuf[1] = MIP_R1_INFO_RESOLUTION_X;
> > + mip_i2c_read(info, wbuf, 2, rbuf, 7);
> > +
> > +#if MIP_AUTOSET_RESOLUTION
>
> Why would we not autoset it?
#if is removed.
>
> > + /* Set resolution by firmware info */
> > + info->max_x = (rbuf[0]) | (rbuf[1] << 8);
>
> get_unaligned_le16()?
Modified.
>
> > + info->max_y = (rbuf[2]) | (rbuf[3] << 8);
>
> get_unaligned_le16()?
Modified.
>
>
> > +#else
> > + info->max_x = 1920;
> > + info->max_y = 1080;
> > +#endif
> > + dev_dbg(&info->client->dev, "%s - max_x[%d] max_y[%d]\n",
> > + __func__, info->max_x, info->max_y);
> > +
> > + /* Node info */
> > + info->node_x = rbuf[4];
> > + info->node_y = rbuf[5];
> > + info->node_key = rbuf[6];
> > + dev_dbg(&info->client->dev,
> > + "%s - node_x[%d] node_y[%d] node_key[%d]\n",
> > + __func__, info->node_x, info->node_y, info->node_key);
>
> What is node?
Node is the number of sensing channels.
node_x, node_y removed.
>
> > +
> > + /* Key info */
> > + if (info->node_key > 0) {
> > + /* Enable touchkey */
> > + info->key_enable = true;
> > + info->key_num = info->node_key;
> > + }
> > +
> > + /* Protocol */
> > +#if MIP_AUTOSET_EVENT_FORMAT
>
> Again, why would we not want to do this?
#if is removed.
>
> > + wbuf[0] = MIP_R0_EVENT;
> > + wbuf[1] = MIP_R1_EVENT_SUPPORTED_FUNC;
> > + mip_i2c_read(info, wbuf, 2, rbuf, 7);
> > + info->event_format = (rbuf[4]) | (rbuf[5] << 8);
> > + info->event_size = rbuf[6];
> > +#else
> > + info->event_format = 0;
> > + info->event_size = 6;
> > +#endif
> > + dev_dbg(&info->client->dev,
> > + "%s - event_format[%d] event_size[%d]\n",
> > + __func__, info->event_format, info->event_size);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Update firmware
> > +*/
> > +static int mip_fw_update(struct mip_ts_info *info)
> > +{
> > + const char *fw_name = FW_PATH_INTERNAL;
> > + const struct firmware *fw;
> > + int retires = 3;
> > + int ret = fw_err_none;
> > +
> > + /* Disable IRQ */
> > + mutex_lock(&info->lock);
> > + disable_irq(info->client->irq);
> > +
> > + /* Get firmware */
> > + request_firmware(&fw, fw_name, &info->client->dev);
> > +
> > + if (!fw) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] request_firmware\n", __func__);
> > + goto ERROR;
> > + }
> > +
> > + /* Update firmware */
> > + do {
> > + ret = mip_flash_fw(info, fw->data, fw->size, false, true);
> > + if (ret >= fw_err_none)
> > + break;
> > + } while (--retires);
> > +
> > + if (!retires) {
> > + dev_err(&info->client->dev,
> > + "%s [ERROR] mip_flash_fw failed\n", __func__);
> > + ret = fw_err_download;
> > + }
> > +
> > + release_firmware(fw);
> > +
> > + /* Enable IRQ */
> > + enable_irq(info->client->irq);
> > + mutex_unlock(&info->lock);
> > +
> > + if (ret < fw_err_none)
> > + goto ERROR;
> > +
> > + return ret;
> > +
> > +ERROR:
> > + dev_err(&info->client->dev, "%s [ERROR]\n", __func__);
> > + return ret;
> > +}
> > +
> > +static ssize_t mip_sys_fw_update(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct mip_ts_info *info = i2c_get_clientdata(client);
> > + int result = 0;
> > + u8 data[255];
> > + int ret = 0;
> > +
> > + memset(info->print_buf, 0, PAGE_SIZE);
> > +
> > + /* Update firmware */
> > + ret = mip_fw_update(info);
> > +
> > + switch (ret) {
> > + case fw_err_none:
> > + sprintf(data, "F/W update success.\n");
> > + break;
> > + case fw_err_uptodate:
> > + sprintf(data, "F/W is already up-to-date.\n");
> > + break;
> > + case fw_err_download:
> > + sprintf(data, "F/W update failed : Download error\n");
> > + break;
> > + case fw_err_file_type:
> > + sprintf(data, "F/W update failed : File type error\n");
> > + break;
> > + case fw_err_file_open:
> > + sprintf(data, "F/W update failed : File open error\n");
> > + break;
> > + case fw_err_file_read:
> > + sprintf(data, "F/W update failed : File read error\n");
> > + break;
> > + default:
> > + sprintf(data, "F/W update failed.\n");
> > + break;
> > + }
> > +
> > + /* Re-initialize driver */
> > + mip_init_config(info);
> > + mip_config_input(info);
> > +
> > + strcat(info->print_buf, data);
> > + result = snprintf(buf, PAGE_SIZE, "%s\n", info->print_buf);
> > + return result;
> > +}
> > +static DEVICE_ATTR(fw_update, 0660, mip_sys_fw_update, NULL);
> > +
> > +/*
> > +* Sysfs attr info
> > +*/
> > +static struct attribute *mip_attrs[] = {
> > + &dev_attr_fw_update.attr,
> > + NULL,
> > +};
> > +
> > +/*
> > +* Sysfs attr group info
> > +*/
> > +static const struct attribute_group mip_attr_group = {
> > + .attrs = mip_attrs,
> > +};
>
> Why do you have 2 separate unnamed attribute groups?
Merged to one group.
>
> > +
> > +/*
> > +* Initialize driver
> > +*/
> > +static int mip_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +{
> > + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > + struct mip_ts_info *info;
> > + struct input_dev *input_dev;
> > +#if MIP_USE_ACPI
> > + struct acpi_device *adev;
> > + acpi_status a_status;
> > +#endif
> > + int ret = 0;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] i2c_check_functionality\n",
> > + __func__);
> > + ret = -EIO;
> > + goto ERROR_I2C;
> > + }
> > +
> > + /* Init info data */
> > + info = kzalloc(sizeof(struct mip_ts_info), GFP_KERNEL);
> > + input_dev = input_allocate_device();
> > + if (!info || !input_dev) {
> > + dev_err(&client->dev,
> > + "%s [ERROR]\n", __func__);
> > + ret = -ENOMEM;
> > + goto ERROR_INFO;
> > + }
> > +
> > + info->client = client;
> > + info->input_dev = input_dev;
> > + info->irq = -1;
> > + info->init = true;
> > + info->power = -1;
> > + info->irq_enabled = false;
> > +
> > + mutex_init(&info->lock);
> > +
> > + /* Set platform data */
> > +#if MIP_USE_ACPI
> > + a_status = acpi_bus_get_device(ACPI_HANDLE(&client->dev), &adev);
> > + if (ACPI_SUCCESS(a_status)) {
> > + if (strncmp(dev_name(&adev->dev), ACPI_ID, 8) != 0) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] ACPI_ID mismatch [%s]\n",
> > + __func__, dev_name(&adev->dev));
> > + goto ERROR_INFO;
> > + }
> > + }
> > +#else
> > + if (client->dev.of_node) {
> > + ret = mip_parse_devicetree(&client->dev, info);
> > + if (ret) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_parse_dt\n", __func__);
> > + goto ERROR_INFO;
> > + }
> > + } else {
> > + info->pdata = client->dev.platform_data;
> > + if (info->pdata == NULL) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] dev.platform_data is null\n",
> > + __func__);
> > + ret = -EINVAL;
> > + goto ERROR_INFO;
> > + } else {
> > + info->gpio_intr = info->pdata->gpio_intr;
> > + info->gpio_ce = info->pdata->gpio_ce;
> > + info->gpio_vd33_en = info->pdata->gpio_vd33_en;
> > + }
> > + }
> > +#endif
>
> Please used devm_gpiod_* interface, then you do not need separate code
> for ACPI, OF and platform data, at least where GPIOs are concerned.
Modified to devm_gpiod_*.
>
> > +
> > + /* Set input dev */
> > + info->input_dev->name = "MELFAS " CHIP_NAME " Touchscreen";
> > + info->input_dev->phys = info->phys;
> > + info->input_dev->id.bustype = BUS_I2C;
> > + info->input_dev->dev.parent = &client->dev;
> > +
> > +#if MIP_USE_INPUT_OPEN_CLOSE
> > + info->input_dev->open = mip_input_open;
> > + info->input_dev->close = mip_input_close;
> > +#endif
> > +
> > + /* Set info data */
> > + input_set_drvdata(input_dev, info);
> > + i2c_set_clientdata(client, info);
> > +
> > + /* Power on */
> > + mip_power_on(info);
> > +
> > + /* Firmware update */
> > +#if MIP_USE_AUTO_FW_UPDATE
> > + ret = mip_fw_update_from_kernel(info);
> > + if (ret)
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_fw_update_from_kernel\n",
> > + __func__);
> > +#endif
> > +
> > + /* Initial config */
> > + mip_init_config(info);
> > +
> > + /* Config input interface */
> > + mip_config_input(info);
> > +
> > + /* Register input device */
> > + ret = input_register_device(input_dev);
> > + if (ret) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] input_register_device\n",
> > + __func__);
> > + ret = -EIO;
> > + goto ERROR_DEVICE;
> > + }
> > +
> > + /* Set interrupt handler */
> > + ret = request_threaded_irq(client->irq, NULL, mip_interrupt,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, MIP_DEVICE_NAME, info);
>
> Let's have platform set up interrupt flags for us (level or edge
> trigger), so just have IRQF_ONESHOT here.
Modified.
>
> > +
> > + if (ret) {
> > + dev_err(&client->dev,
> > + "%s [ERROR] request_threaded_irq\n", __func__);
> > + goto ERROR_IRQ;
> > + }
> > +
> > + disable_irq(client->irq);
> > + info->irq = client->irq;
> > +
> > + /* Enable device */
> > + mip_enable(info);
> > +
> > + /* Create dev */
> > + if (mip_dev_create(info))
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_dev_create\n", __func__);
> > +
> > + info->class = class_create(THIS_MODULE, MIP_DEVICE_NAME);
> > + device_create(info->class, NULL, info->mip_dev, NULL, MIP_DEVICE_NAME);
>
> Why do we need a driver-private class?
Removed.
>
> > +
> > + /* Create sysfs */
> > + if (mip_sysfs_create(info))
> > + dev_err(&client->dev,
> > + "%s [ERROR] mip_sysfs_create\n", __func__);
> > +
> > + if (sysfs_create_group(&client->dev.kobj, &mip_attr_group))
> > + dev_err(&client->dev,
> > + "%s [ERROR] sysfs_create_group\n", __func__);
> > +
> > + dev_info(&client->dev,
> > + "MELFAS MIP4 Touchscreen is initialized successfully.\n");
>
> We already have message from input core, we do not need this one.
Removed.
>
> > + return 0;
> > +
> > +ERROR_IRQ:
> > + free_irq(info->irq, info);
> > +ERROR_DEVICE:
> > + input_unregister_device(info->input_dev);
> > +ERROR_INFO:
> > + kfree(info);
> > +ERROR_I2C:
> > + dev_dbg(&client->dev, "%s [ERROR]\n", __func__);
> > + dev_err(&client->dev,
> > + "MELFAS MIP4 Touchscreen initialization failed.\n");
>
> Driver core will print message when probe fails, no need for this one.
Removed.
>
> > + return ret;
> > +}
> > +
> > +/*
> > +* Remove driver
> > +*/
> > +static int mip_remove(struct i2c_client *client)
> > +{
> > + struct mip_ts_info *info = i2c_get_clientdata(client);
> > +
> > + if (info->irq >= 0)
> > + free_irq(info->irq, info);
>
> Why do we need this check? probe will fail without valid irq.
Removed and changed to devm_*.
>
> > +
> > + mip_sysfs_remove(info);
> > + sysfs_remove_group(&info->client->dev.kobj, &mip_attr_group);
> > + sysfs_remove_link(NULL, MIP_DEVICE_NAME);
>
> What creates this link and why?
Removed.
>
> > + kfree(info->print_buf);
> > +
> > + device_destroy(info->class, info->mip_dev);
> > + class_destroy(info->class);
> > +
> > + input_unregister_device(info->input_dev);
> > +
> > + kfree(info);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
>
> Please drop #ifdef
Removed.
>
> > +/*
> > +* Device suspend event handler
> > +*/
> > +int mip_suspend(struct device *dev)
>
> static int __maybe_unused mip_suspend()
Modified.
>
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct mip_ts_info *info = i2c_get_clientdata(client);
> > +
> > + mip_disable(info);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > +* Device resume event handler
> > +*/
> > +int mip_resume(struct device *dev)
>
> static int __maybe_unused mip_resume()
Modified.
>
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct mip_ts_info *info = i2c_get_clientdata(client);
> > +
>
> You need to check if there are users of the input device before enabling
> it.
Modified.
>
> > + mip_enable(info);
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_PM
> > +/*
> > +* PM info
> > +*/
> > +const struct dev_pm_ops mip_pm_ops = {
> > + .suspend = mip_suspend,
> > + .resume = mip_resume,
> > +};
> > +#endif
>
> use SIMPLE_DEV_PM_OPS instead.
Modified.
>
> > +
> > +#if MIP_USE_DEVICETREE
> > +/*
> > +* Device tree match table
> > +*/
> > +static const struct of_device_id mip_match_table[] = {
> > + {.compatible = "melfas,"MIP_DEVICE_NAME,},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, mip_match_table);
> > +#endif
> > +
> > +#if MIP_USE_ACPI
> > +static const struct acpi_device_id mip_acpi_match_table[] = {
> > + {ACPI_ID, 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, mip_acpi_match_table);
> > +#endif
> > +
> > +/*
> > +* I2C Device ID
> > +*/
> > +static const struct i2c_device_id mip_id[] = {
> > + {MIP_DEVICE_NAME, 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(i2c, mip_id);
> > +
> > +/*
> > +* I2C driver info
> > +*/
> > +static struct i2c_driver mip_driver = {
> > + .id_table = mip_id,
> > + .probe = mip_probe,
> > + .remove = mip_remove,
> > + .driver = {
> > + .name = MIP_DEVICE_NAME,
> > + .owner = THIS_MODULE,
> > +#if MIP_USE_DEVICETREE
> > + .of_match_table = mip_match_table,
> > +#endif
>
> .of_match_table = of_match_ptr(mip_match_table),
>
> and drop ifdef.
Modified.
>
> > +#if MIP_USE_ACPI
> > + .acpi_match_table = ACPI_PTR(mip_acpi_match_table),
> > +#endif
>
> No need for this ifdef
Modified.
>
> > +#ifdef CONFIG_PM
> > + .pm = &mip_pm_ops,
> > +#endif
>
> No need for ifdef with SIMPLE_DEV_PM_OPS.
Modified.
>
> > + },
> > +};
> > +
> > +/*
> > +* Init driver
> > +*/
> > +static int __init mip_init(void)
> > +{
> > + return i2c_add_driver(&mip_driver);
> > +}
> > +
> > +/*
> > +* Exit driver
> > +*/
> > +static void __exit mip_exit(void)
> > +{
> > + i2c_del_driver(&mip_driver);
> > +}
> > +
> > +module_init(mip_init);
> > +module_exit(mip_exit);
>
> module_i2c_driver().
Modified.
>
> > +
> > +MODULE_DESCRIPTION("MELFAS MIP4 Touchscreen");
> > +MODULE_VERSION("2016.01.08");
> > +MODULE_AUTHOR("Sangwon Jee <jeesw@xxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/input/melfas_mip4.h b/include/linux/input/melfas_mip4.h
> > new file mode 100644
> > index 0000000..3523e81
> > --- /dev/null
> > +++ b/include/linux/input/melfas_mip4.h
> > @@ -0,0 +1,33 @@
> > +/*
> > + * MELFAS MIP4 Touchscreen
> > + *
> > + * Copyright (C) 2016 MELFAS Inc.
> > + *
> > + * Author : Sangwon Jee <jeesw@xxxxxxxxxx>
> > + *
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#ifndef _LINUX_MELFAS_MIP4_TOUCHSCREEN_H
> > +#define _LINUX_MELFAS_MIP4_TOUCHSCREEN_H
> > +
> > +/*
> > +* Platform Data
> > +*/
> > +struct melfas_mip4_platform_data {
> > + int gpio_intr; /* Interrupt - Required */
> > + int gpio_ce; /* Chip Enable/Reset - Optional */
> > + int gpio_vd33_en; /* Power(VD33) control - Optional */
> > +};
> > +#endif
> > +
> > --
> > 1.9.1
> >
>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
