* [PATCH 0/2] Input: Add Cypress Gen5 Touchscreen driver @ 2017-05-29 14:45 Mylène Josserand 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand 2017-05-29 14:45 ` [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 Mylène Josserand 0 siblings, 2 replies; 18+ messages in thread From: Mylène Josserand @ 2017-05-29 14:45 UTC (permalink / raw) To: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, fery-+wT8y+m8/X5BDgjK7y7TUQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8 Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-input-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8 Hi everyone, Here is a V1 serie to add the driver of the touchscreen Cypress, TrueTouch Generation 5. Based on last linux-next tag (next-20170529), last commit 62d5d7921010. This touchscreen is similar to Cypress generation 4 but the registers are different. This driver uses regmap as it is possible to use I2C or SPI busses. Currently, only the I2C support are available in the driver. This touchscreen can handle some multitouch event as buttons using some defined zone. That is why the driver handles buttons and multitouch events. Patch 01: Add the basis of the driver for Cypress Gen5 Touchscreen. Patch 02: Add the binding documentation for this driver. If you have any remarks, let me know. Thank you in advance, Mylène Mylène Josserand (2): Input: Add driver for Cypress Generation 5 touchscreen Documentation: DT: bindings: input: Add documentation for cyttsp5 .../bindings/input/touchscreen/cyttsp5.txt | 55 + drivers/input/touchscreen/Kconfig | 15 + drivers/input/touchscreen/Makefile | 1 + drivers/input/touchscreen/cyttsp5.c | 1174 ++++++++++++++++++++ 4 files changed, 1245 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt create mode 100644 drivers/input/touchscreen/cyttsp5.c -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-05-29 14:45 [PATCH 0/2] Input: Add Cypress Gen5 Touchscreen driver Mylène Josserand @ 2017-05-29 14:45 ` Mylène Josserand 2017-05-29 17:16 ` kbuild test robot ` (4 more replies) 2017-05-29 14:45 ` [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 Mylène Josserand 1 sibling, 5 replies; 18+ messages in thread From: Mylène Josserand @ 2017-05-29 14:45 UTC (permalink / raw) To: dmitry.torokhov, fery, robh+dt, mark.rutland Cc: linux-kernel, linux-input, devicetree, mylene.josserand, thomas.petazzoni, maxime.ripard This is the basic driver for the Cypress TrueTouch Gen5 touchscreen controllers. This driver supports only the I2C bus but it uses regmap so SPI support could be added later. The touchscreen can retrieve some defined zone that are handled as buttons (according to the hardware). That is why it handles button and multitouch events. Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> --- drivers/input/touchscreen/Kconfig | 15 + drivers/input/touchscreen/Makefile | 1 + drivers/input/touchscreen/cyttsp5.c | 1174 +++++++++++++++++++++++++++++++++++ 3 files changed, 1190 insertions(+) create mode 100644 drivers/input/touchscreen/cyttsp5.c diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index cf26ca49ae6d..8e9e25cd0249 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -238,6 +238,21 @@ config TOUCHSCREEN_CYTTSP4_SPI To compile this driver as a module, choose M here: the module will be called cyttsp4_spi. +config TOUCHSCREEN_CYTTSP5 + tristate "Cypress TrueTouch Gen5 Touchscreen Driver" + depends on OF + select REGMAP_I2C + help + Driver for Parade TrueTouch Standard Product + Generation 5 touchscreen controllers. + I2C bus interface support only. + + Say Y here if you have a Cypress Gen5 touchscreen. + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called cyttsp5. + config TOUCHSCREEN_DA9034 tristate "Touchscreen support for Dialog Semiconductor DA9034" depends on PMIC_DA903X diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile index 18e476948e44..a9ac134d8b7b 100644 --- a/drivers/input/touchscreen/Makefile +++ b/drivers/input/touchscreen/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_CORE) += cyttsp4_core.o obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_I2C) += cyttsp4_i2c.o cyttsp_i2c_common.o obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_SPI) += cyttsp4_spi.o +obj-$(CONFIG_TOUCHSCREEN_CYTTSP5) += cyttsp5.o obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.o obj-$(CONFIG_TOUCHSCREEN_DYNAPRO) += dynapro.o diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c new file mode 100644 index 000000000000..d472d6ed83dc --- /dev/null +++ b/drivers/input/touchscreen/cyttsp5.c @@ -0,0 +1,1174 @@ +/* + * Parade TrueTouch(TM) Standard Product V5 Module. + * For use with Parade touchscreen controllers. + * + * Copyright (C) 2015 Parade Technologies + * Copyright (C) 2012-2015 Cypress Semiconductor + * Copyright (C) 2017 Free Electrons + * + * Author: Mylène Josserand <mylene.josserand@free-electrons.com> + * + * 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. + * + */ + +#include <asm/unaligned.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio.h> +#include <linux/input/mt.h> +#include <linux/input/touchscreen.h> +#include <linux/interrupt.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/regmap.h> + +#define CYTTSP5_NAME "cyttsp5" +#define CY_I2C_DATA_SIZE (2 * 256) +#define HID_VERSION 0x0100 +#define CY_MAX_INPUT 512 +#define CYTTSP5_PREALLOCATED_CMD_BUFFER 32 +#define CY_BITS_PER_BTN 1 +#define CY_NUM_BTN_EVENT_ID ((1 << CY_BITS_PER_BTN) - 1) + +#define MAX_AREA 255 +#define HID_OUTPUT_BL_SOP 0x1 +#define HID_OUTPUT_BL_EOP 0x17 +#define HID_OUTPUT_BL_LAUNCH_APP 0x3B +#define HID_OUTPUT_BL_LAUNCH_APP_SIZE 11 +#define HID_OUTPUT_GET_SYSINFO 0x2 +#define HID_OUTPUT_GET_SYSINFO_SIZE 5 + +#define HID_DESC_REG 0x1 +#define HID_OUTPUT_REG 0x4 + +#define REPORT_ID_TOUCH 0x1 +#define REPORT_ID_BTN 0x3 +#define REPORT_SIZE_5 5 +#define REPORT_SIZE_8 8 +#define REPORT_SIZE_16 16 + +/* Touch reports offsets */ +/* Header offsets */ +#define TOUCH_REPORT_DESC_HDR_CONTACTCOUNT 16 +/* Record offsets */ +#define TOUCH_REPORT_DESC_CONTACTID 8 +#define TOUCH_REPORT_DESC_X 16 +#define TOUCH_REPORT_DESC_Y 32 +#define TOUCH_REPORT_DESC_P 48 +#define TOUCH_REPORT_DESC_MAJ 56 +#define TOUCH_REPORT_DESC_MIN 64 + +/* HID */ +#define HID_TOUCH_REPORT_ID 0x1 +#define HID_BTN_REPORT_ID 0x3 +#define HID_APP_RESPONSE_REPORT_ID 0x1F +#define HID_APP_OUTPUT_REPORT_ID 0x2F +#define HID_BL_RESPONSE_REPORT_ID 0x30 +#define HID_BL_OUTPUT_REPORT_ID 0x40 + +#define HID_OUTPUT_RESPONSE_REPORT_OFFSET 2 +#define HID_OUTPUT_RESPONSE_CMD_OFFSET 4 +#define HID_OUTPUT_RESPONSE_CMD_MASK 0x7F + +#define HID_SYSINFO_SENSING_OFFSET 33 +#define HID_SYSINFO_BTN_OFFSET 48 +#define HID_SYSINFO_BTN_MASK 0xFF +#define HID_SYSINFO_MAX_BTN 8 + +/* Timeout in ms */ +#define CY_HID_OUTPUT_TIMEOUT 200 +#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT 3000 +#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT 4000 + +/* maximum number of concurrent tracks */ +#define TOUCH_REPORT_SIZE 10 +#define TOUCH_INPUT_HEADER_SIZE 7 +#define BTN_REPORT_SIZE 9 +#define BTN_INPUT_HEADER_SIZE 5 + +/* All usage pages for Touch Report */ +#define TOUCH_REPORT_USAGE_PG_X 0x00010030 +#define TOUCH_REPORT_USAGE_PG_Y 0x00010031 +#define TOUCH_REPORT_USAGE_PG_P 0x000D0030 +#define TOUCH_REPORT_USAGE_PG_CONTACTID 0x000D0051 +#define TOUCH_REPORT_USAGE_PG_CONTACTCOUNT 0x000D0054 +#define TOUCH_REPORT_USAGE_PG_MAJ 0xFF010062 +#define TOUCH_REPORT_USAGE_PG_MIN 0xFF010063 +#define TOUCH_COL_USAGE_PG 0x000D0022 + +/* helpers */ +#define IS_TMO(t) ((t) == 0) +#define HI_BYTE(x) (u8)(((x) >> 8) & 0xFF) +#define LOW_BYTE(x) (u8)((x) & 0xFF) + +/* System Information interface definitions */ +struct cyttsp5_sensing_conf_data_dev { + u8 electrodes_x; + u8 electrodes_y; + __le16 len_x; + __le16 len_y; + __le16 res_x; + __le16 res_y; + __le16 max_z; + u8 origin_x; + u8 origin_y; + u8 btn; + u8 scan_mode; + u8 max_num_of_tch_per_refresh_cycle; +} __packed; + +struct cyttsp5_sensing_conf_data { + u16 res_x; + u16 res_y; + u16 max_z; + u16 len_x; + u16 len_y; + u8 origin_x; + u8 origin_y; + u8 max_tch; +}; + +enum cyttsp5_tch_abs { /* for ordering within the extracted touch data array */ + CY_TCH_X, /* X */ + CY_TCH_Y, /* Y */ + CY_TCH_P, /* P (Z) */ + CY_TCH_T, /* TOUCH ID */ + CY_TCH_MAJ, /* TOUCH_MAJOR */ + CY_TCH_MIN, /* TOUCH_MINOR */ + CY_TCH_NUM_ABS, +}; + +struct cyttsp5_tch_abs_params { + size_t ofs; /* abs byte offset */ + size_t size; /* size in bits */ + size_t min; /* min value */ + size_t max; /* max value */ + size_t bofs; /* bit offset */ +}; + +struct cyttsp5_touch { + int hdr; + int abs[CY_TCH_NUM_ABS]; +}; + +struct cyttsp5_btn { + bool enabled; + int key_code; +}; + +struct cyttsp5_sysinfo { + struct cyttsp5_sensing_conf_data sensing_conf_data; + int num_btns; + struct cyttsp5_btn *btn; + struct cyttsp5_tch_abs_params tch_hdr; + struct cyttsp5_tch_abs_params tch_abs[CY_TCH_NUM_ABS]; + u8 *xy_mode; + u8 *xy_data; +}; + +struct cyttsp5_hid_desc { + __le16 hid_desc_len; + u8 packet_id; + u8 reserved_byte; + __le16 bcd_version; + __le16 report_desc_len; + __le16 report_desc_register; + __le16 input_register; + __le16 max_input_len; + __le16 output_register; + __le16 max_output_len; + __le16 command_register; + __le16 data_register; + __le16 vendor_id; + __le16 product_id; + __le16 version_id; + u8 reserved[4]; +} __packed; + +struct cyttsp5 { + struct device *dev; + struct mutex system_lock; + struct mutex btn_lock; + struct mutex mt_lock; + wait_queue_head_t wait_q; + int irq; + struct cyttsp5_sysinfo sysinfo; + int hid_cmd_state; + struct cyttsp5_hid_desc hid_desc; + u8 cmd_buf[CYTTSP5_PREALLOCATED_CMD_BUFFER]; + u8 input_buf[CY_MAX_INPUT]; + u8 response_buf[CY_MAX_INPUT]; + struct gpio_desc *reset_gpio; + struct input_dev *input; + char phys[NAME_MAX]; + int num_prv_rec; + struct regmap *regmap; +}; + +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max) +{ + int rc; + u32 size; + + if (!buf) + return -EINVAL; + + /* Read the frame to retrieve the size */ + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); + if (rc < 0) + return rc; + + size = get_unaligned_le16(&buf[0]); + if (!size || size == 2) + return 0; + + if (size > max) + return -EINVAL; + + /* Get the real value */ + return regmap_bulk_read(ts->regmap, 0, buf, size); +} + +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data, + size_t size) +{ + u8 cmd[size + 1]; + + /* High bytes of register address needed as first byte of cmd */ + cmd[0] = HI_BYTE(reg); + /* Copy the rest of the data */ + memcpy(&cmd[1], data, size); + + return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1); +} + +static void cyttsp5_final_sync(struct input_dev *input, int max_slots, + unsigned long *ids) +{ + int t; + + for (t = 0; t < max_slots; t++) { + if (test_bit(t, ids)) + continue; + input_mt_slot(input, t); + input_mt_report_slot_state(input, MT_TOOL_FINGER, false); + } + + input_sync(input); +} + +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots) +{ + int t; + + if (ts->num_prv_rec == 0) + return; + + for (t = 0; t < max_slots; t++) { + input_mt_slot(ts->input, t); + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false); + } +} + +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int max = si->tch_abs[CY_TCH_T].max; + + if (ts->num_prv_rec != 0) { + cyttsp5_report_slot_liftoff(ts, max); + input_sync(ts->input); + ts->num_prv_rec = 0; + } +} + +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data, + int bofs) +{ + int nbyte; + int next; + + for (nbyte = 0, *axis = 0, next = 0; nbyte < size; nbyte++) { + *axis = *axis + ((xy_data[next] >> bofs) << (nbyte * 8)); + next++; + } + + *axis &= max - 1; +} + +static void cyttsp5_get_touch_record(struct cyttsp5 *ts, + struct cyttsp5_touch *touch, u8 *xy_data) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + enum cyttsp5_tch_abs abs; + + for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) { + cyttsp5_get_touch_axis(&touch->abs[abs], + si->tch_abs[abs].size, + si->tch_abs[abs].max, + xy_data + si->tch_abs[abs].ofs, + si->tch_abs[abs].bofs); + } +} + +static void cyttsp5_mt_process_touch(struct cyttsp5 *ts, + struct cyttsp5_touch *touch) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int tmp; + + tmp = touch->abs[CY_TCH_X]; + touch->abs[CY_TCH_X] = touch->abs[CY_TCH_Y]; + touch->abs[CY_TCH_Y] = tmp; + + /* Convert MAJOR/MINOR from mm to resolution */ + tmp = touch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x; + touch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x; + tmp = touch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x; + touch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x; +} + +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts, + struct cyttsp5_touch *tch, int num_cur_tch) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int i, t = 0; + DECLARE_BITMAP(ids, si->tch_abs[CY_TCH_T].max); + u8 *tch_addr; + + bitmap_zero(ids, si->tch_abs[CY_TCH_T].max); + memset(tch->abs, 0, sizeof(tch->abs)); + + for (i = 0; i < num_cur_tch; i++) { + tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE); + cyttsp5_get_touch_record(ts, tch, tch_addr); + + /* Process touch */ + cyttsp5_mt_process_touch(ts, tch); + + t = tch->abs[CY_TCH_T]; + input_mt_slot(ts->input, t); + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true); + __set_bit(t, ids); + + /* position and pressure fields */ + input_report_abs(ts->input, ABS_MT_POSITION_X, tch->abs[CY_TCH_X]); + input_report_abs(ts->input, ABS_MT_POSITION_Y, tch->abs[CY_TCH_Y]); + input_report_abs(ts->input, ABS_MT_PRESSURE, tch->abs[CY_TCH_P]); + + /* Get the extended touch fields */ + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, tch->abs[CY_TCH_MAJ]); + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR, tch->abs[CY_TCH_MIN]); + } + + cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids); + + ts->num_prv_rec = num_cur_tch; +} + +/* read xy_data for all current touches */ +static int cyttsp5_xy_worker(struct cyttsp5 *ts) +{ + struct device *dev = ts->dev; + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int max_tch = si->sensing_conf_data.max_tch; + struct cyttsp5_touch tch; + u8 num_cur_tch; + + cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size, + si->tch_hdr.max, + si->xy_mode + 3 + si->tch_hdr.ofs, + si->tch_hdr.bofs); + + num_cur_tch = tch.hdr; + if (num_cur_tch > max_tch) { + dev_err(dev, "%s: Num touch err detected (n=%d)\n", + __func__, num_cur_tch); + num_cur_tch = max_tch; + } + + if (num_cur_tch == 0 && ts->num_prv_rec == 0) + return 0; + + /* extract xy_data for all currently reported touches */ + if (num_cur_tch) + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch); + else + cyttsp5_mt_lift_all(ts); + + return 0; +} + +static int cyttsp5_mt_attention(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int rc; + + if (si->xy_mode[2] != HID_TOUCH_REPORT_ID) + return 0; + + /* core handles handshake */ + mutex_lock(&ts->mt_lock); + rc = cyttsp5_xy_worker(ts); + mutex_unlock(&ts->mt_lock); + if (rc < 0) + dev_err(dev, "%s: xy_worker error r=%d\n", __func__, rc); + + return rc; +} + +static int cyttsp5_setup_input_device(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int max_x, max_y, max_p; + int max_x_tmp, max_y_tmp; + int rc; + + __set_bit(EV_ABS, ts->input->evbit); + __set_bit(EV_REL, ts->input->evbit); + __set_bit(EV_KEY, ts->input->evbit); + + max_x_tmp = si->sensing_conf_data.res_x; + max_y_tmp = si->sensing_conf_data.res_y; + max_x = max_y_tmp - 1; + max_y = max_x_tmp - 1; + max_p = si->sensing_conf_data.max_z; + + input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0); + + __set_bit(ABS_MT_POSITION_X, ts->input->absbit); + __set_bit(ABS_MT_POSITION_Y, ts->input->absbit); + __set_bit(ABS_MT_PRESSURE, ts->input->absbit); + + input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0); + input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0); + input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0); + + input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0); + input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0); + + rc = input_register_device(ts->input); + if (rc < 0) + dev_err(dev, "%s: Error, failed register input device r=%d\n", + __func__, rc); + + return rc; +} + +static int cyttsp5_parse_dt_key_code(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + struct device_node *np, *pp; + int rc, i; + + np = dev->of_node; + if (!np) + return -EINVAL; + + if (si->num_btns) { + si->btn = devm_kzalloc(dev, + si->num_btns * sizeof(struct cyttsp5_btn), + GFP_KERNEL); + if (!si->btn) + return -ENOMEM; + + /* Initialized the button to RESERVED */ + for (i = 0; i < si->num_btns; i++) { + struct cyttsp5_btn *btns = &si->btn[i]; + btns->key_code = KEY_RESERVED; + btns->enabled = true; + } + } + + i = 0; + for_each_child_of_node(np, pp) { + struct cyttsp5_btn *btns = &si->btn[i]; + + rc = of_property_read_u32(pp, "linux,code", &btns->key_code); + if (rc) { + dev_err(dev, "%s: Inval linux,code prop\n", pp->name); + return -EINVAL; + } + btns->enabled = true; + + i++; + } + + return i; +} + +static int cyttsp5_btn_attention(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int cur_btn; + int cur_btn_state; + + if (si->xy_mode[2] != HID_BTN_REPORT_ID) + return 0; + + /* core handles handshake */ + mutex_lock(&ts->btn_lock); + + /* extract button press/release touch information */ + if (si->num_btns > 0) { + for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) { + /* Get current button state */ + cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN)) + & CY_NUM_BTN_EVENT_ID; + + if (!si->btn[cur_btn].enabled) + continue; + + input_report_key(ts->input, si->btn[cur_btn].key_code, + cur_btn_state); + input_sync(ts->input); + } + } + + mutex_unlock(&ts->btn_lock); + + return 0; +} + +static const u16 crc_table[16] = { + 0x0000, 0x1021, 0x2042, 0x3063, + 0x4084, 0x50a5, 0x60c6, 0x70e7, + 0x8108, 0x9129, 0xa14a, 0xb16b, + 0xc18c, 0xd1ad, 0xe1ce, 0xf1ef, +}; + +static u16 cyttsp5_compute_crc(u8 *buf, u32 size) +{ + u16 remainder = 0xFFFF; + u16 xor_mask = 0x0000; + u32 index; + u32 byte_value; + u32 table_index; + u32 crc_bit_width = sizeof(u16) * 8; + + /* Divide the message by polynomial, via the table. */ + for (index = 0; index < size; index++) { + byte_value = buf[index]; + table_index = ((byte_value >> 4) & 0x0F) + ^ (remainder >> (crc_bit_width - 4)); + remainder = crc_table[table_index] ^ (remainder << 4); + table_index = (byte_value & 0x0F) + ^ (remainder >> (crc_bit_width - 4)); + remainder = crc_table[table_index] ^ (remainder << 4); + } + + /* Perform the final remainder CRC. */ + return remainder ^ xor_mask; +} + +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code) +{ + u16 size, crc; + u8 status, offset; + int command_code; + + size = get_unaligned_le16(&ts->response_buf[0]); + + if (!size) + return 0; + + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET]; + + if (offset == HID_BL_RESPONSE_REPORT_ID) { + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) { + dev_err(ts->dev, "%s: HID output response, wrong SOP\n", + __func__); + return -EPROTO; + } + + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) { + dev_err(ts->dev, "%s: HID output response, wrong EOP\n", + __func__); + return -EPROTO; + } + + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7); + if (ts->response_buf[size - 3] != LOW_BYTE(crc) + || ts->response_buf[size - 2] != HI_BYTE(crc)) { + dev_err(ts->dev, "%s: HID output response, wrong CRC 0x%X\n", + __func__, crc); + return -EPROTO; + } + + status = ts->response_buf[5]; + if (status) { + dev_err(ts->dev, "%s: HID output response, ERROR:%d\n", + __func__, status); + return -EPROTO; + } + } + + if (offset == HID_APP_RESPONSE_REPORT_ID) { + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET] + & HID_OUTPUT_RESPONSE_CMD_MASK; + if (command_code != code) { + dev_err(ts->dev, + "%s: HID output response, wrong command_code:%X\n", + __func__, command_code); + return -EPROTO; + } + } + + return 0; +} + +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int i; + int num_btns = 0; + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] + & HID_SYSINFO_BTN_MASK; + + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { + if (btns & (1 << i)) + num_btns++; + } + si->num_btns = num_btns; +} + +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) +{ + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; + struct cyttsp5_sensing_conf_data_dev *scd_dev = + (struct cyttsp5_sensing_conf_data_dev *) + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; + struct cyttsp5_sysinfo *si = &ts->sysinfo; + + cyttsp5_si_get_btn_data(ts); + + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; + scd->res_x = get_unaligned_le16(&scd_dev->res_x); + scd->res_y = get_unaligned_le16(&scd_dev->res_y); + scd->max_z = get_unaligned_le16(&scd_dev->max_z); + scd->len_x = get_unaligned_le16(&scd_dev->len_x); + scd->len_y = get_unaligned_le16(&scd_dev->len_y); + + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, + GFP_KERNEL); + if (!si->xy_data) + return -ENOMEM; + + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL); + if (!si->xy_mode) { + devm_kfree(ts->dev, si->xy_data); + return -ENOMEM; + } + + return 0; +} + +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) +{ + int rc, t; + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; + + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; + mutex_unlock(&ts->system_lock); + + /* HI bytes of Output register address */ + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); + cmd[2] = HID_APP_OUTPUT_REPORT_ID; + cmd[3] = 0x0; /* Reserved */ + cmd[4] = HID_OUTPUT_GET_SYSINFO; + + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, + HID_OUTPUT_GET_SYSINFO_SIZE); + if (rc) { + dev_err(ts->dev, "%s: Failed to write command %d", + __func__, rc); + goto error; + } + + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); + if (IS_TMO(t)) { + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", + __func__); + rc = -ETIME; + goto error; + } + + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); + goto exit; + +error: + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = 0; + mutex_unlock(&ts->system_lock); + return rc; +exit: + rc = cyttsp5_get_sysinfo_regs(ts); + + return rc; +} + +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) +{ + int rc, t; + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; + u16 crc; + + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; + mutex_unlock(&ts->system_lock); + + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); + cmd[2] = HID_BL_OUTPUT_REPORT_ID; + cmd[3] = 0x0; /* Reserved */ + cmd[4] = HID_OUTPUT_BL_SOP; + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; + cmd[6] = 0x0; /* Low bytes of data */ + cmd[7] = 0x0; /* Hi bytes of data */ + crc = cyttsp5_compute_crc(&cmd[4], 4); + cmd[8] = LOW_BYTE(crc); + cmd[9] = HI_BYTE(crc); + cmd[10] = HID_OUTPUT_BL_EOP; + + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, + HID_OUTPUT_BL_LAUNCH_APP_SIZE); + if (rc) { + dev_err(ts->dev, "%s: Failed to write command %d", + __func__, rc); + goto error; + } + + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT)); + if (IS_TMO(t)) { + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", + __func__); + rc = -ETIME; + goto error; + } + + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP); + goto exit; + +error: + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = 0; + mutex_unlock(&ts->system_lock); +exit: + return rc; +} + +static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts, + struct cyttsp5_hid_desc *desc) +{ + struct device *dev = ts->dev; + __le16 hid_desc_register = HID_DESC_REG; + int rc; + int t; + u8 cmd[2]; + + /* Read HID descriptor length and version */ + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = 1; + mutex_unlock(&ts->system_lock); + + /* Set HID descriptor register */ + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register)); + + regmap_write(ts->regmap, HID_DESC_REG, cmd[0]); + rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]); + if (rc < 0) { + dev_err(dev, "%s: failed to get HID descriptor, rc=%d\n", + __func__, rc); + goto error; + } + + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT)); + if (IS_TMO(t)) { + dev_err(ts->dev, "%s: HID get descriptor timed out\n", + __func__); + rc = -ETIME; + goto error; + } + + memcpy((u8 *)desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc)); + + /* Check HID descriptor length and version */ + if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) || + le16_to_cpu(desc->bcd_version) != HID_VERSION) { + dev_err(dev, "%s: Unsupported HID version\n", __func__); + return -ENODEV; + } + + goto exit; + +error: + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = 0; + mutex_unlock(&ts->system_lock); +exit: + return rc; +} + +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size, + int offset) +{ + tch_abs->ofs = offset / 8; + tch_abs->size = report_size / 8; + if (report_size % 8) + tch_abs->size += 1; + tch_abs->min = 0; + tch_abs->max = 1 << report_size; + tch_abs->bofs = offset - (tch_abs->ofs << 3); + + return 0; +} + +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) +{ + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE); + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE], + BTN_REPORT_SIZE); + + return 0; +} + +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) +{ + int max_tch = si->sensing_conf_data.max_tch; + int num_cur_tch; + int length; + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr; + + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE); + + cyttsp5_get_touch_axis(&num_cur_tch, tch->size, + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs); + if (unlikely(num_cur_tch > max_tch)) + num_cur_tch = max_tch; + + length = num_cur_tch * TOUCH_REPORT_SIZE; + + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length); + return 0; +} + +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle) +{ + struct cyttsp5 *ts = handle; + int report_id; + int size; + int rc; + + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT); + if (!rc) { + size = get_unaligned_le16(&ts->input_buf[0]); + + /* check reset */ + if (size == 0) { + memcpy(ts->response_buf, ts->input_buf, 2); + + ts->hid_cmd_state = 0; + wake_up(&ts->wait_q); + mutex_unlock(&ts->system_lock); + return IRQ_HANDLED; + } + + report_id = ts->input_buf[2]; + + if (report_id == HID_TOUCH_REPORT_ID) { + move_touch_data(ts, &ts->sysinfo); + cyttsp5_mt_attention(ts->dev); + } else if (report_id == HID_BTN_REPORT_ID) { + move_button_data(ts, &ts->sysinfo); + cyttsp5_btn_attention(ts->dev); + } else { + /* It is not an input but a command response */ + memcpy(ts->response_buf, ts->input_buf, size); + + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = 0; + mutex_unlock(&ts->system_lock); + wake_up(&ts->wait_q); + } + } + + return IRQ_HANDLED; +} + +static int cyttsp5_deassert_int(struct cyttsp5 *ts) +{ + u16 size; + u8 buf[2]; + int rc; + + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); + if (rc < 0) + return rc; + + size = get_unaligned_le16(&buf[0]); + if (size == 2 || size == 0) + return 0; + + return -EINVAL; +} + +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + + fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16, + TOUCH_REPORT_DESC_X); + fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16, + TOUCH_REPORT_DESC_Y); + fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8, + TOUCH_REPORT_DESC_P); + fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5, + TOUCH_REPORT_DESC_CONTACTID); + fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5, + TOUCH_REPORT_DESC_HDR_CONTACTCOUNT); + fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8, + TOUCH_REPORT_DESC_MAJ); + fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8, + TOUCH_REPORT_DESC_MIN); + + return 0; +} + +static int cyttsp5_startup(struct cyttsp5 *ts) +{ + int rc; + + rc = cyttsp5_deassert_int(ts); + if (rc) { + dev_err(ts->dev, "%s: Error on deassert int r=%d\n", + __func__, rc); + return -ENODEV; + } + + /* + * Launch the application as the device starts in bootloader mode + * because of a power-on-reset + */ + rc = cyttsp5_hid_output_bl_launch_app(ts); + if (rc < 0) { + dev_err(ts->dev, "%s: Error on launch app r=%d\n", + __func__, rc); + goto exit; + } + + rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc); + if (rc < 0) { + dev_err(ts->dev, "%s: Error on getting HID descriptor r=%d\n", + __func__, rc); + goto exit; + } + + rc = cyttsp5_fill_all_touch(ts); + if (rc < 0) { + dev_err(ts->dev, "%s: Error on getting report descriptor r=%d\n", + __func__, rc); + goto exit; + } + + rc = cyttsp5_hid_output_get_sysinfo(ts); + if (rc) { + dev_err(ts->dev, "%s: Error on getting sysinfo r=%d\n", + __func__, rc); + goto exit; + } + +exit: + + return rc; +} + +static const struct of_device_id cyttsp5_of_match[] = { + { .compatible = "cypress,cyttsp5", }, + { } +}; +MODULE_DEVICE_TABLE(of, cyttsp5_of_match); + +static const struct i2c_device_id cyttsp5_i2c_id[] = { + { CYTTSP5_NAME, 0, }, + { } +}; +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id); + +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq, + const char *name) +{ + struct cyttsp5 *ts; + struct cyttsp5_sysinfo *si; + int rc = 0, i; + + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); + if (!ts) { + rc = -ENOMEM; + goto error_alloc_data; + } + + /* Initialize device info */ + ts->regmap = regmap; + ts->dev = dev; + si = &ts->sysinfo; + dev_set_drvdata(dev, ts); + + /* Initialize mutexes and spinlocks */ + mutex_init(&ts->system_lock); + mutex_init(&ts->mt_lock); + mutex_init(&ts->btn_lock); + + /* Initialize wait queue */ + init_waitqueue_head(&ts->wait_q); + + /* Call platform init function */ + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(ts->reset_gpio)) { + rc = PTR_ERR(ts->reset_gpio); + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); + return rc; + } + + /* Need a delay to have device up */ + msleep(20); + + ts->irq = irq; + rc = devm_request_threaded_irq(dev, ts->irq, NULL, cyttsp5_handle_irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + name, ts); + if (rc) { + dev_err(dev, "unable to request IRQ\n"); + goto error_setup_irq; + } + + rc = cyttsp5_startup(ts); + if (rc) { + dev_err(ts->dev, "%s: Fail initial startup r=%d\n", + __func__, rc); + goto error_startup; + } + + rc = cyttsp5_parse_dt_key_code(dev); + if (rc < 0) { + dev_err(ts->dev, "%s: Error while parsing dts\n", __func__); + goto error_startup; + } + + ts->input = input_allocate_device(); + if (!ts->input) { + dev_err(dev, "%s: Error, failed to allocate input device\n", + __func__); + rc = -ENODEV; + goto error_startup; + } + + ts->input->name = "cyttsp5"; + scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0); + ts->input->phys = ts->phys; + ts->input->dev.parent = ts->dev; + input_set_drvdata(ts->input, ts); + + touchscreen_parse_properties(ts->input, true, NULL); + + if (si) { + __set_bit(EV_KEY, ts->input->evbit); + for (i = 0; i < si->num_btns; i++) + __set_bit(si->btn[i].key_code, ts->input->keybit); + + rc = cyttsp5_setup_input_device(dev); + if (rc) + goto error_init_input; + } + + return 0; + +error_init_input: + input_free_device(ts->input); +error_startup: +error_setup_irq: + dev_set_drvdata(dev, NULL); +error_alloc_data: + dev_err(dev, "%s failed.\n", __func__); + if (dev->of_node) + of_node_put(dev->of_node); + + return rc; +} + +static int cyttsp5_i2c_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct regmap *regmap; + static const struct regmap_config config = { + .reg_bits = 8, + .val_bits = 8, + }; + + regmap = devm_regmap_init_i2c(client, &config); + if (IS_ERR(regmap)) { + dev_err(&client->dev, "%s: regmap allocation failed: %ld\n", + __func__, PTR_ERR(regmap)); + return PTR_ERR(regmap); + } + + return cyttsp5_probe(&client->dev, regmap, client->irq, client->name); +} + +static int cyttsp5_remove(struct device *dev) +{ + const struct of_device_id *match; + struct cyttsp5 *ts = dev_get_drvdata(dev); + + input_unregister_device(ts->input); + + dev_set_drvdata(dev, NULL); + + match = of_match_device(of_match_ptr(cyttsp5_of_match), dev); + if (match && dev->of_node) + of_node_put(dev->of_node); + + return 0; +} + +static int cyttsp5_i2c_remove(struct i2c_client *client) +{ + struct device *dev = &client->dev; + + return cyttsp5_remove(dev); +} + +static struct i2c_driver cyttsp5_i2c_driver = { + .driver = { + .name = CYTTSP5_NAME, + .owner = THIS_MODULE, + .of_match_table = cyttsp5_of_match, + }, + .probe = cyttsp5_i2c_probe, + .remove = cyttsp5_i2c_remove, + .id_table = cyttsp5_i2c_id, +}; + +module_i2c_driver(cyttsp5_i2c_driver); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product"); +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@free-electrons.com>"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand @ 2017-05-29 17:16 ` kbuild test robot 2017-05-29 17:16 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot ` (3 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2017-05-29 17:16 UTC (permalink / raw) Cc: kbuild-all, dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, mylene.josserand, thomas.petazzoni, maxime.ripard Hi Mylène, [auto build test WARNING on input/next] [also build test WARNING on v4.12-rc3 next-20170529] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/Input-Add-Cypress-Gen5-Touchscreen-driver/20170529-230006 base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next coccinelle warnings: (new ones prefixed by >>) >> drivers/input/touchscreen/cyttsp5.c:1162:3-8: No need to set .owner here. The core will do it. Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] Input: fix platform_no_drv_owner.cocci warnings 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand 2017-05-29 17:16 ` kbuild test robot @ 2017-05-29 17:16 ` kbuild test robot 2017-05-30 8:02 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Maxime Ripard ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2017-05-29 17:16 UTC (permalink / raw) Cc: kbuild-all, dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, mylene.josserand, thomas.petazzoni, maxime.ripard drivers/input/touchscreen/cyttsp5.c:1162:3-8: No need to set .owner here. The core will do it. Remove .owner field if calls are used which set it automatically Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci CC: Mylène Josserand <mylene.josserand@free-electrons.com> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- cyttsp5.c | 1 - 1 file changed, 1 deletion(-) --- a/drivers/input/touchscreen/cyttsp5.c +++ b/drivers/input/touchscreen/cyttsp5.c @@ -1159,7 +1159,6 @@ static int cyttsp5_i2c_remove(struct i2c static struct i2c_driver cyttsp5_i2c_driver = { .driver = { .name = CYTTSP5_NAME, - .owner = THIS_MODULE, .of_match_table = cyttsp5_of_match, }, .probe = cyttsp5_i2c_probe, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand 2017-05-29 17:16 ` kbuild test robot 2017-05-29 17:16 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot @ 2017-05-30 8:02 ` Maxime Ripard 2017-06-06 8:03 ` Mylene Josserand 2017-05-30 8:11 ` Thomas Petazzoni [not found] ` <20170529144538.29187-2-mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 4 siblings, 1 reply; 18+ messages in thread From: Maxime Ripard @ 2017-05-30 8:02 UTC (permalink / raw) To: Mylène Josserand Cc: dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, thomas.petazzoni [-- Attachment #1: Type: text/plain, Size: 30272 bytes --] Hi Mylene, On Mon, May 29, 2017 at 04:45:37PM +0200, Mylène Josserand wrote: > +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max) > +{ > + int rc; > + u32 size; > + > + if (!buf) > + return -EINVAL; > + > + /* Read the frame to retrieve the size */ > + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); > + if (rc < 0) > + return rc; You should probably use a temporary buffer here, instead of the caller one. Otherwise, you might return an error (a bit below, in the (size > max) test), while you modified the caller buffer, which seems a bit odd. > + size = get_unaligned_le16(&buf[0]); isn't &buf[0] the same thing than buf here ? > + if (!size || size == 2) > + return 0; > + > + if (size > max) > + return -EINVAL; > + > + /* Get the real value */ > + return regmap_bulk_read(ts->regmap, 0, buf, size); > +} > + > +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data, > + size_t size) > +{ > + u8 cmd[size + 1]; > + > + /* High bytes of register address needed as first byte of cmd */ > + cmd[0] = HI_BYTE(reg); > + /* Copy the rest of the data */ > + memcpy(&cmd[1], data, size); > + > + return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1); The way we need to access the buffers here is definitely not trivial, I guess some comment about how the hardware expects the commands to be sent would be welcome. > +} > + > +static void cyttsp5_final_sync(struct input_dev *input, int max_slots, > + unsigned long *ids) > +{ > + int t; > + > + for (t = 0; t < max_slots; t++) { > + if (test_bit(t, ids)) > + continue; > + input_mt_slot(input, t); > + input_mt_report_slot_state(input, MT_TOOL_FINGER, false); > + } > + > + input_sync(input); > +} > + > +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots) > +{ > + int t; > + > + if (ts->num_prv_rec == 0) > + return; > + > + for (t = 0; t < max_slots; t++) { > + input_mt_slot(ts->input, t); > + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false); > + } > +} > + > +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int max = si->tch_abs[CY_TCH_T].max; > + > + if (ts->num_prv_rec != 0) { > + cyttsp5_report_slot_liftoff(ts, max); > + input_sync(ts->input); > + ts->num_prv_rec = 0; > + } > +} > + > +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data, > + int bofs) > +{ > + int nbyte; > + int next; > + > + for (nbyte = 0, *axis = 0, next = 0; nbyte < size; nbyte++) { > + *axis = *axis + ((xy_data[next] >> bofs) << (nbyte * 8)); > + next++; Isn't nbyte and next the same thing here ? > + } > + > + *axis &= max - 1; > +} > + > +static void cyttsp5_get_touch_record(struct cyttsp5 *ts, > + struct cyttsp5_touch *touch, u8 *xy_data) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + enum cyttsp5_tch_abs abs; > + > + for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) { > + cyttsp5_get_touch_axis(&touch->abs[abs], > + si->tch_abs[abs].size, > + si->tch_abs[abs].max, > + xy_data + si->tch_abs[abs].ofs, > + si->tch_abs[abs].bofs); > + } > +} > + > +static void cyttsp5_mt_process_touch(struct cyttsp5 *ts, > + struct cyttsp5_touch *touch) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int tmp; > + > + tmp = touch->abs[CY_TCH_X]; > + touch->abs[CY_TCH_X] = touch->abs[CY_TCH_Y]; > + touch->abs[CY_TCH_Y] = tmp; Why do you need to swap the values here? Instead of rolling your own implementation, you can use swap() > + /* Convert MAJOR/MINOR from mm to resolution */ > + tmp = touch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x; > + touch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x; > + tmp = touch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x; > + touch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x; > +} > + > +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts, > + struct cyttsp5_touch *tch, int num_cur_tch) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int i, t = 0; > + DECLARE_BITMAP(ids, si->tch_abs[CY_TCH_T].max); > + u8 *tch_addr; > + > + bitmap_zero(ids, si->tch_abs[CY_TCH_T].max); > + memset(tch->abs, 0, sizeof(tch->abs)); > + > + for (i = 0; i < num_cur_tch; i++) { > + tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE); > + cyttsp5_get_touch_record(ts, tch, tch_addr); > + > + /* Process touch */ > + cyttsp5_mt_process_touch(ts, tch); Maybe you should just make that function return a structure, or fill one directly passed as an argument. Filling only one field of a particular structure passed as an argument to exchange data doesn't seem very natural. Especially since you don't really use tch in that function. > + > + t = tch->abs[CY_TCH_T]; > + input_mt_slot(ts->input, t); > + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true); > + __set_bit(t, ids); > + > + /* position and pressure fields */ > + input_report_abs(ts->input, ABS_MT_POSITION_X, tch->abs[CY_TCH_X]); > + input_report_abs(ts->input, ABS_MT_POSITION_Y, tch->abs[CY_TCH_Y]); > + input_report_abs(ts->input, ABS_MT_PRESSURE, tch->abs[CY_TCH_P]); > + > + /* Get the extended touch fields */ > + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, tch->abs[CY_TCH_MAJ]); > + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR, tch->abs[CY_TCH_MIN]); > + } > + > + cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids); > + > + ts->num_prv_rec = num_cur_tch; > +} > + > +/* read xy_data for all current touches */ > +static int cyttsp5_xy_worker(struct cyttsp5 *ts) > +{ > + struct device *dev = ts->dev; > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int max_tch = si->sensing_conf_data.max_tch; > + struct cyttsp5_touch tch; > + u8 num_cur_tch; > + > + cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size, > + si->tch_hdr.max, > + si->xy_mode + 3 + si->tch_hdr.ofs, > + si->tch_hdr.bofs); > + > + num_cur_tch = tch.hdr; > + if (num_cur_tch > max_tch) { > + dev_err(dev, "%s: Num touch err detected (n=%d)\n", > + __func__, num_cur_tch); > + num_cur_tch = max_tch; > + } > + > + if (num_cur_tch == 0 && ts->num_prv_rec == 0) > + return 0; > + > + /* extract xy_data for all currently reported touches */ > + if (num_cur_tch) > + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch); > + else > + cyttsp5_mt_lift_all(ts); > + > + return 0; > +} > + > +static int cyttsp5_mt_attention(struct device *dev) > +{ > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int rc; > + > + if (si->xy_mode[2] != HID_TOUCH_REPORT_ID) > + return 0; > + > + /* core handles handshake */ > + mutex_lock(&ts->mt_lock); > + rc = cyttsp5_xy_worker(ts); > + mutex_unlock(&ts->mt_lock); > + if (rc < 0) > + dev_err(dev, "%s: xy_worker error r=%d\n", __func__, rc); > + > + return rc; > +} > + > +static int cyttsp5_setup_input_device(struct device *dev) > +{ > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int max_x, max_y, max_p; > + int max_x_tmp, max_y_tmp; > + int rc; > + > + __set_bit(EV_ABS, ts->input->evbit); > + __set_bit(EV_REL, ts->input->evbit); > + __set_bit(EV_KEY, ts->input->evbit); > + > + max_x_tmp = si->sensing_conf_data.res_x; > + max_y_tmp = si->sensing_conf_data.res_y; > + max_x = max_y_tmp - 1; > + max_y = max_x_tmp - 1; > + max_p = si->sensing_conf_data.max_z; > + > + input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0); > + > + __set_bit(ABS_MT_POSITION_X, ts->input->absbit); > + __set_bit(ABS_MT_POSITION_Y, ts->input->absbit); > + __set_bit(ABS_MT_PRESSURE, ts->input->absbit); > + > + input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0); > + input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0); > + input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0); > + > + input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0); > + input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0); > + > + rc = input_register_device(ts->input); > + if (rc < 0) > + dev_err(dev, "%s: Error, failed register input device r=%d\n", > + __func__, rc); > + > + return rc; > +} > + > +static int cyttsp5_parse_dt_key_code(struct device *dev) > +{ > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + struct device_node *np, *pp; > + int rc, i; > + > + np = dev->of_node; > + if (!np) > + return -EINVAL; > + > + if (si->num_btns) { > + si->btn = devm_kzalloc(dev, > + si->num_btns * sizeof(struct cyttsp5_btn), > + GFP_KERNEL); > + if (!si->btn) > + return -ENOMEM; > + > + /* Initialized the button to RESERVED */ > + for (i = 0; i < si->num_btns; i++) { > + struct cyttsp5_btn *btns = &si->btn[i]; > + btns->key_code = KEY_RESERVED; > + btns->enabled = true; > + } > + } > + > + i = 0; > + for_each_child_of_node(np, pp) { > + struct cyttsp5_btn *btns = &si->btn[i]; > + > + rc = of_property_read_u32(pp, "linux,code", &btns->key_code); > + if (rc) { > + dev_err(dev, "%s: Inval linux,code prop\n", pp->name); > + return -EINVAL; > + } > + btns->enabled = true; > + > + i++; > + } > + > + return i; > +} > + > +static int cyttsp5_btn_attention(struct device *dev) > +{ > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int cur_btn; > + int cur_btn_state; > + > + if (si->xy_mode[2] != HID_BTN_REPORT_ID) > + return 0; > + > + /* core handles handshake */ > + mutex_lock(&ts->btn_lock); > + > + /* extract button press/release touch information */ > + if (si->num_btns > 0) { > + for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) { > + /* Get current button state */ > + cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN)) > + & CY_NUM_BTN_EVENT_ID; > + > + if (!si->btn[cur_btn].enabled) > + continue; > + > + input_report_key(ts->input, si->btn[cur_btn].key_code, > + cur_btn_state); > + input_sync(ts->input); > + } > + } > + > + mutex_unlock(&ts->btn_lock); > + > + return 0; > +} > + > +static const u16 crc_table[16] = { > + 0x0000, 0x1021, 0x2042, 0x3063, > + 0x4084, 0x50a5, 0x60c6, 0x70e7, > + 0x8108, 0x9129, 0xa14a, 0xb16b, > + 0xc18c, 0xd1ad, 0xe1ce, 0xf1ef, > +}; This looks like a shorter version of crc_itu_t_table. > +static u16 cyttsp5_compute_crc(u8 *buf, u32 size) > +{ > + u16 remainder = 0xFFFF; > + u16 xor_mask = 0x0000; > + u32 index; > + u32 byte_value; > + u32 table_index; > + u32 crc_bit_width = sizeof(u16) * 8; > + > + /* Divide the message by polynomial, via the table. */ > + for (index = 0; index < size; index++) { > + byte_value = buf[index]; > + table_index = ((byte_value >> 4) & 0x0F) > + ^ (remainder >> (crc_bit_width - 4)); > + remainder = crc_table[table_index] ^ (remainder << 4); > + table_index = (byte_value & 0x0F) > + ^ (remainder >> (crc_bit_width - 4)); > + remainder = crc_table[table_index] ^ (remainder << 4); > + } > + > + /* Perform the final remainder CRC. */ > + return remainder ^ xor_mask; > +} Could it be that it's just using the CRC ITU-T algorithm? > +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code) > +{ > + u16 size, crc; > + u8 status, offset; > + int command_code; > + > + size = get_unaligned_le16(&ts->response_buf[0]); > + > + if (!size) > + return 0; > + > + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET]; > + > + if (offset == HID_BL_RESPONSE_REPORT_ID) { > + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) { > + dev_err(ts->dev, "%s: HID output response, wrong SOP\n", > + __func__); > + return -EPROTO; > + } > + > + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) { > + dev_err(ts->dev, "%s: HID output response, wrong EOP\n", > + __func__); > + return -EPROTO; > + } > + > + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7); > + if (ts->response_buf[size - 3] != LOW_BYTE(crc) > + || ts->response_buf[size - 2] != HI_BYTE(crc)) { > + dev_err(ts->dev, "%s: HID output response, wrong CRC 0x%X\n", > + __func__, crc); > + return -EPROTO; > + } > + > + status = ts->response_buf[5]; > + if (status) { > + dev_err(ts->dev, "%s: HID output response, ERROR:%d\n", > + __func__, status); > + return -EPROTO; > + } > + } > + > + if (offset == HID_APP_RESPONSE_REPORT_ID) { > + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET] > + & HID_OUTPUT_RESPONSE_CMD_MASK; > + if (command_code != code) { > + dev_err(ts->dev, > + "%s: HID output response, wrong command_code:%X\n", > + __func__, command_code); > + return -EPROTO; > + } > + } > + > + return 0; > +} > + > +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int i; > + int num_btns = 0; > + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] > + & HID_SYSINFO_BTN_MASK; > + > + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { > + if (btns & (1 << i)) > + num_btns++; > + } > + si->num_btns = num_btns; > +} > + > +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) > +{ > + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; > + struct cyttsp5_sensing_conf_data_dev *scd_dev = > + (struct cyttsp5_sensing_conf_data_dev *) > + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + > + cyttsp5_si_get_btn_data(ts); > + > + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; > + scd->res_x = get_unaligned_le16(&scd_dev->res_x); > + scd->res_y = get_unaligned_le16(&scd_dev->res_y); > + scd->max_z = get_unaligned_le16(&scd_dev->max_z); > + scd->len_x = get_unaligned_le16(&scd_dev->len_x); > + scd->len_y = get_unaligned_le16(&scd_dev->len_y); > + > + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, > + GFP_KERNEL); > + if (!si->xy_data) > + return -ENOMEM; > + > + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL); > + if (!si->xy_mode) { > + devm_kfree(ts->dev, si->xy_data); > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) > +{ > + int rc, t; > + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; > + > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; > + mutex_unlock(&ts->system_lock); > + > + /* HI bytes of Output register address */ > + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); > + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); > + cmd[2] = HID_APP_OUTPUT_REPORT_ID; > + cmd[3] = 0x0; /* Reserved */ > + cmd[4] = HID_OUTPUT_GET_SYSINFO; > + > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, > + HID_OUTPUT_GET_SYSINFO_SIZE); > + if (rc) { > + dev_err(ts->dev, "%s: Failed to write command %d", > + __func__, rc); > + goto error; > + } > + > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); > + if (IS_TMO(t)) { > + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", > + __func__); > + rc = -ETIME; > + goto error; > + } > + > + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); > + goto exit; > + > +error: > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = 0; > + mutex_unlock(&ts->system_lock); > + return rc; > +exit: > + rc = cyttsp5_get_sysinfo_regs(ts); > + > + return rc; > +} > + > +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) > +{ > + int rc, t; > + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; > + u16 crc; > + > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; > + mutex_unlock(&ts->system_lock); > + > + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); > + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); > + cmd[2] = HID_BL_OUTPUT_REPORT_ID; > + cmd[3] = 0x0; /* Reserved */ > + cmd[4] = HID_OUTPUT_BL_SOP; > + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; > + cmd[6] = 0x0; /* Low bytes of data */ > + cmd[7] = 0x0; /* Hi bytes of data */ > + crc = cyttsp5_compute_crc(&cmd[4], 4); > + cmd[8] = LOW_BYTE(crc); > + cmd[9] = HI_BYTE(crc); > + cmd[10] = HID_OUTPUT_BL_EOP; > + > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, > + HID_OUTPUT_BL_LAUNCH_APP_SIZE); It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here, once as setup in cyttsp5_write, and once in the buffer you want to send. Is that on purpose? > + if (rc) { > + dev_err(ts->dev, "%s: Failed to write command %d", > + __func__, rc); > + goto error; > + } > + > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT)); > + if (IS_TMO(t)) { > + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", > + __func__); > + rc = -ETIME; > + goto error; > + } > + > + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP); > + goto exit; > + > +error: > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = 0; > + mutex_unlock(&ts->system_lock); > +exit: > + return rc; > +} > + > +static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts, > + struct cyttsp5_hid_desc *desc) > +{ > + struct device *dev = ts->dev; > + __le16 hid_desc_register = HID_DESC_REG; > + int rc; > + int t; > + u8 cmd[2]; > + > + /* Read HID descriptor length and version */ > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = 1; > + mutex_unlock(&ts->system_lock); > + > + /* Set HID descriptor register */ > + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register)); > + > + regmap_write(ts->regmap, HID_DESC_REG, cmd[0]); > + rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]); > + if (rc < 0) { > + dev_err(dev, "%s: failed to get HID descriptor, rc=%d\n", > + __func__, rc); > + goto error; > + } > + > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT)); > + if (IS_TMO(t)) { > + dev_err(ts->dev, "%s: HID get descriptor timed out\n", > + __func__); > + rc = -ETIME; > + goto error; > + } > + > + memcpy((u8 *)desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc)); > + > + /* Check HID descriptor length and version */ > + if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) || > + le16_to_cpu(desc->bcd_version) != HID_VERSION) { > + dev_err(dev, "%s: Unsupported HID version\n", __func__); > + return -ENODEV; > + } > + > + goto exit; > + > +error: > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = 0; > + mutex_unlock(&ts->system_lock); > +exit: > + return rc; > +} > + > +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size, > + int offset) > +{ > + tch_abs->ofs = offset / 8; > + tch_abs->size = report_size / 8; > + if (report_size % 8) > + tch_abs->size += 1; > + tch_abs->min = 0; > + tch_abs->max = 1 << report_size; > + tch_abs->bofs = offset - (tch_abs->ofs << 3); > + > + return 0; > +} > + > +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) > +{ > + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE); > + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE], > + BTN_REPORT_SIZE); > + > + return 0; > +} > + > +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) > +{ > + int max_tch = si->sensing_conf_data.max_tch; > + int num_cur_tch; > + int length; > + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr; > + > + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE); > + > + cyttsp5_get_touch_axis(&num_cur_tch, tch->size, > + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs); > + if (unlikely(num_cur_tch > max_tch)) > + num_cur_tch = max_tch; > + > + length = num_cur_tch * TOUCH_REPORT_SIZE; > + > + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length); > + return 0; > +} > + > +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle) > +{ > + struct cyttsp5 *ts = handle; > + int report_id; > + int size; > + int rc; > + > + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT); > + if (!rc) { > + size = get_unaligned_le16(&ts->input_buf[0]); > + > + /* check reset */ > + if (size == 0) { > + memcpy(ts->response_buf, ts->input_buf, 2); > + > + ts->hid_cmd_state = 0; > + wake_up(&ts->wait_q); > + mutex_unlock(&ts->system_lock); It doesn't seem like you've taken that mutex anywhere in the interrupt. Where is it taken? Note that taking a mutex in some code path, and releasing it in some other is pretty confusing, and locks are complicated enough to not introduce any confusion. > + return IRQ_HANDLED; > + } > + > + report_id = ts->input_buf[2]; > + > + if (report_id == HID_TOUCH_REPORT_ID) { > + move_touch_data(ts, &ts->sysinfo); > + cyttsp5_mt_attention(ts->dev); > + } else if (report_id == HID_BTN_REPORT_ID) { > + move_button_data(ts, &ts->sysinfo); > + cyttsp5_btn_attention(ts->dev); > + } else { > + /* It is not an input but a command response */ > + memcpy(ts->response_buf, ts->input_buf, size); > + > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = 0; > + mutex_unlock(&ts->system_lock); And you take and release that lock here, which is even weirder :) > + wake_up(&ts->wait_q); > + } > + } > + > + return IRQ_HANDLED; > +} > + > +static int cyttsp5_deassert_int(struct cyttsp5 *ts) > +{ > + u16 size; > + u8 buf[2]; > + int rc; > + > + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); > + if (rc < 0) > + return rc; > + > + size = get_unaligned_le16(&buf[0]); > + if (size == 2 || size == 0) > + return 0; > + > + return -EINVAL; > +} > + > +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + > + fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16, > + TOUCH_REPORT_DESC_X); > + fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16, > + TOUCH_REPORT_DESC_Y); > + fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8, > + TOUCH_REPORT_DESC_P); > + fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5, > + TOUCH_REPORT_DESC_CONTACTID); > + fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5, > + TOUCH_REPORT_DESC_HDR_CONTACTCOUNT); > + fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8, > + TOUCH_REPORT_DESC_MAJ); > + fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8, > + TOUCH_REPORT_DESC_MIN); > + > + return 0; > +} > + > +static int cyttsp5_startup(struct cyttsp5 *ts) > +{ > + int rc; > + > + rc = cyttsp5_deassert_int(ts); > + if (rc) { > + dev_err(ts->dev, "%s: Error on deassert int r=%d\n", > + __func__, rc); > + return -ENODEV; > + } > + > + /* > + * Launch the application as the device starts in bootloader mode > + * because of a power-on-reset > + */ > + rc = cyttsp5_hid_output_bl_launch_app(ts); > + if (rc < 0) { > + dev_err(ts->dev, "%s: Error on launch app r=%d\n", > + __func__, rc); > + goto exit; > + } > + > + rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc); > + if (rc < 0) { > + dev_err(ts->dev, "%s: Error on getting HID descriptor r=%d\n", > + __func__, rc); > + goto exit; > + } > + > + rc = cyttsp5_fill_all_touch(ts); > + if (rc < 0) { > + dev_err(ts->dev, "%s: Error on getting report descriptor r=%d\n", > + __func__, rc); > + goto exit; > + } > + > + rc = cyttsp5_hid_output_get_sysinfo(ts); > + if (rc) { > + dev_err(ts->dev, "%s: Error on getting sysinfo r=%d\n", > + __func__, rc); > + goto exit; > + } > + > +exit: > + > + return rc; > +} > + > +static const struct of_device_id cyttsp5_of_match[] = { > + { .compatible = "cypress,cyttsp5", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, cyttsp5_of_match); You probably want to make that conditional to CONFIG_OF (and use of_match_ptr on the pointer to that table in your struct driver); > + > +static const struct i2c_device_id cyttsp5_i2c_id[] = { > + { CYTTSP5_NAME, 0, }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id); > + > +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq, > + const char *name) > +{ > + struct cyttsp5 *ts; > + struct cyttsp5_sysinfo *si; > + int rc = 0, i; > + > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); > + if (!ts) { > + rc = -ENOMEM; > + goto error_alloc_data; > + } > + > + /* Initialize device info */ > + ts->regmap = regmap; > + ts->dev = dev; > + si = &ts->sysinfo; > + dev_set_drvdata(dev, ts); > + > + /* Initialize mutexes and spinlocks */ > + mutex_init(&ts->system_lock); > + mutex_init(&ts->mt_lock); > + mutex_init(&ts->btn_lock); > + > + /* Initialize wait queue */ > + init_waitqueue_head(&ts->wait_q); > + > + /* Call platform init function */ > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(ts->reset_gpio)) { > + rc = PTR_ERR(ts->reset_gpio); > + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); > + return rc; > + } > + > + /* Need a delay to have device up */ > + msleep(20); In the case where the device has already been out of reset, this means that this alone is not sufficient to reset it and bring it out of reset, which also means that you do not have any guarantee on the current state of the device. I'm not sure how it would impact the proper operations though. > + > + ts->irq = irq; You don't seem to use ts->irq anywhere else, you can probably just keep it as a local variable. > + rc = devm_request_threaded_irq(dev, ts->irq, NULL, cyttsp5_handle_irq, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + name, ts); > + if (rc) { > + dev_err(dev, "unable to request IRQ\n"); > + goto error_setup_irq; > + } > + > + rc = cyttsp5_startup(ts); > + if (rc) { > + dev_err(ts->dev, "%s: Fail initial startup r=%d\n", > + __func__, rc); > + goto error_startup; > + } > + > + rc = cyttsp5_parse_dt_key_code(dev); > + if (rc < 0) { > + dev_err(ts->dev, "%s: Error while parsing dts\n", __func__); > + goto error_startup; > + } > + > + ts->input = input_allocate_device(); > + if (!ts->input) { > + dev_err(dev, "%s: Error, failed to allocate input device\n", > + __func__); > + rc = -ENODEV; > + goto error_startup; > + } In error_startup, you never uninitialize the device. Given your comment in cyttsp5_startup, this means that you might end up in a situation where your driver probes again with the device initialized and no longer in a bootloader mode, which is likely to cause some error. Putting the call to cyttsp5_startup later will make you less likely to fail (especially because of the DT parsing), and putting the device back into reset will probably address the issue once and for all. > + ts->input->name = "cyttsp5"; > + scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0); > + ts->input->phys = ts->phys; > + ts->input->dev.parent = ts->dev; > + input_set_drvdata(ts->input, ts); > + > + touchscreen_parse_properties(ts->input, true, NULL); > + > + if (si) { > + __set_bit(EV_KEY, ts->input->evbit); > + for (i = 0; i < si->num_btns; i++) > + __set_bit(si->btn[i].key_code, ts->input->keybit); > + > + rc = cyttsp5_setup_input_device(dev); > + if (rc) > + goto error_init_input; > + } > + > + return 0; > + > +error_init_input: > + input_free_device(ts->input); > +error_startup: > +error_setup_irq: > + dev_set_drvdata(dev, NULL); > +error_alloc_data: > + dev_err(dev, "%s failed.\n", __func__); I'm not sure using those __func__ everywhere is worth it, and given your code, this error log is redundant with the "real" error log you used before the goto. > + if (dev->of_node) > + of_node_put(dev->of_node); I haven't seen any of_node_get, or any function that would do it, called in your driver so far, why do you need it? > + > + return rc; > +} > + > +static int cyttsp5_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct regmap *regmap; > + static const struct regmap_config config = { > + .reg_bits = 8, > + .val_bits = 8, > + }; > + > + regmap = devm_regmap_init_i2c(client, &config); > + if (IS_ERR(regmap)) { > + dev_err(&client->dev, "%s: regmap allocation failed: %ld\n", > + __func__, PTR_ERR(regmap)); > + return PTR_ERR(regmap); > + } > + > + return cyttsp5_probe(&client->dev, regmap, client->irq, client->name); > +} > + > +static int cyttsp5_remove(struct device *dev) > +{ > + const struct of_device_id *match; > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + > + input_unregister_device(ts->input); > + > + dev_set_drvdata(dev, NULL); > + > + match = of_match_device(of_match_ptr(cyttsp5_of_match), dev); > + if (match && dev->of_node) > + of_node_put(dev->of_node); > + > + return 0; > +} > + > +static int cyttsp5_i2c_remove(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + > + return cyttsp5_remove(dev); > +} > + > +static struct i2c_driver cyttsp5_i2c_driver = { > + .driver = { > + .name = CYTTSP5_NAME, > + .owner = THIS_MODULE, > + .of_match_table = cyttsp5_of_match, > + }, > + .probe = cyttsp5_i2c_probe, > + .remove = cyttsp5_i2c_remove, > + .id_table = cyttsp5_i2c_id, > +}; > + > +module_i2c_driver(cyttsp5_i2c_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product"); > +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@free-electrons.com>"); Good work cleaning it up! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-05-30 8:02 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Maxime Ripard @ 2017-06-06 8:03 ` Mylene Josserand 2017-06-06 12:04 ` Maxime Ripard 0 siblings, 1 reply; 18+ messages in thread From: Mylene Josserand @ 2017-06-06 8:03 UTC (permalink / raw) To: Maxime Ripard Cc: dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, thomas.petazzoni Hello Maxime, Thank you for the review! On 30/05/2017 10:02, Maxime Ripard wrote: > Hi Mylene, > > On Mon, May 29, 2017 at 04:45:37PM +0200, Mylène Josserand wrote: >> +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max) >> +{ >> + int rc; >> + u32 size; >> + >> + if (!buf) >> + return -EINVAL; >> + >> + /* Read the frame to retrieve the size */ >> + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); >> + if (rc < 0) >> + return rc; > > You should probably use a temporary buffer here, instead of the caller > one. Otherwise, you might return an error (a bit below, in the (size > > max) test), while you modified the caller buffer, which seems a bit > odd. Okay. > >> + size = get_unaligned_le16(&buf[0]); > > isn't &buf[0] the same thing than buf here ? Yep > >> + if (!size || size == 2) >> + return 0; >> + >> + if (size > max) >> + return -EINVAL; >> + >> + /* Get the real value */ >> + return regmap_bulk_read(ts->regmap, 0, buf, size); >> +} >> + >> +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data, >> + size_t size) >> +{ >> + u8 cmd[size + 1]; >> + >> + /* High bytes of register address needed as first byte of cmd */ >> + cmd[0] = HI_BYTE(reg); >> + /* Copy the rest of the data */ >> + memcpy(&cmd[1], data, size); >> + >> + return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1); > > The way we need to access the buffers here is definitely not trivial, > I guess some comment about how the hardware expects the commands to be > sent would be welcome. Yes, it is not trivial, I will add a comment. > >> +} >> + >> +static void cyttsp5_final_sync(struct input_dev *input, int max_slots, >> + unsigned long *ids) >> +{ >> + int t; >> + >> + for (t = 0; t < max_slots; t++) { >> + if (test_bit(t, ids)) >> + continue; >> + input_mt_slot(input, t); >> + input_mt_report_slot_state(input, MT_TOOL_FINGER, false); >> + } >> + >> + input_sync(input); >> +} >> + >> +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots) >> +{ >> + int t; >> + >> + if (ts->num_prv_rec == 0) >> + return; >> + >> + for (t = 0; t < max_slots; t++) { >> + input_mt_slot(ts->input, t); >> + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false); >> + } >> +} >> + >> +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int max = si->tch_abs[CY_TCH_T].max; >> + >> + if (ts->num_prv_rec != 0) { >> + cyttsp5_report_slot_liftoff(ts, max); >> + input_sync(ts->input); >> + ts->num_prv_rec = 0; >> + } >> +} >> + >> +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data, >> + int bofs) >> +{ >> + int nbyte; >> + int next; >> + >> + for (nbyte = 0, *axis = 0, next = 0; nbyte < size; nbyte++) { >> + *axis = *axis + ((xy_data[next] >> bofs) << (nbyte * 8)); >> + next++; > > Isn't nbyte and next the same thing here ? Yes, thanks! > >> + } >> + >> + *axis &= max - 1; >> +} >> + >> +static void cyttsp5_get_touch_record(struct cyttsp5 *ts, >> + struct cyttsp5_touch *touch, u8 *xy_data) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + enum cyttsp5_tch_abs abs; >> + >> + for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) { >> + cyttsp5_get_touch_axis(&touch->abs[abs], >> + si->tch_abs[abs].size, >> + si->tch_abs[abs].max, >> + xy_data + si->tch_abs[abs].ofs, >> + si->tch_abs[abs].bofs); >> + } >> +} >> + >> +static void cyttsp5_mt_process_touch(struct cyttsp5 *ts, >> + struct cyttsp5_touch *touch) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int tmp; >> + >> + tmp = touch->abs[CY_TCH_X]; >> + touch->abs[CY_TCH_X] = touch->abs[CY_TCH_Y]; >> + touch->abs[CY_TCH_Y] = tmp; > > Why do you need to swap the values here? The X/Y axis must be swapped otherwise, touchscreen's events are not correct (inversion between X and Y axis). I noticed that there is a "touchscreen-swapped-x-y" property that exists in the touchscreen's device tree documentation. http://elixir.free-electrons.com/linux/v4.12-rc4/source/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt#L22 I will update the driver to use this property instead of hard-coding the swap. > > Instead of rolling your own implementation, you can use swap() > >> + /* Convert MAJOR/MINOR from mm to resolution */ >> + tmp = touch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x; >> + touch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x; >> + tmp = touch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x; >> + touch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x; >> +} >> + >> +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts, >> + struct cyttsp5_touch *tch, int num_cur_tch) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int i, t = 0; >> + DECLARE_BITMAP(ids, si->tch_abs[CY_TCH_T].max); >> + u8 *tch_addr; >> + >> + bitmap_zero(ids, si->tch_abs[CY_TCH_T].max); >> + memset(tch->abs, 0, sizeof(tch->abs)); >> + >> + for (i = 0; i < num_cur_tch; i++) { >> + tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE); >> + cyttsp5_get_touch_record(ts, tch, tch_addr); >> + >> + /* Process touch */ >> + cyttsp5_mt_process_touch(ts, tch); > > Maybe you should just make that function return a structure, or fill > one directly passed as an argument. Filling only one field of a > particular structure passed as an argument to exchange data doesn't > seem very natural. Especially since you don't really use tch in that > function. Okay, I will rework it or, even, remove it because this function is small now. > >> + >> + t = tch->abs[CY_TCH_T]; >> + input_mt_slot(ts->input, t); >> + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true); >> + __set_bit(t, ids); >> + >> + /* position and pressure fields */ >> + input_report_abs(ts->input, ABS_MT_POSITION_X, tch->abs[CY_TCH_X]); >> + input_report_abs(ts->input, ABS_MT_POSITION_Y, tch->abs[CY_TCH_Y]); >> + input_report_abs(ts->input, ABS_MT_PRESSURE, tch->abs[CY_TCH_P]); >> + >> + /* Get the extended touch fields */ >> + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, tch->abs[CY_TCH_MAJ]); >> + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR, tch->abs[CY_TCH_MIN]); >> + } >> + >> + cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids); >> + >> + ts->num_prv_rec = num_cur_tch; >> +} >> + >> +/* read xy_data for all current touches */ >> +static int cyttsp5_xy_worker(struct cyttsp5 *ts) >> +{ >> + struct device *dev = ts->dev; >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int max_tch = si->sensing_conf_data.max_tch; >> + struct cyttsp5_touch tch; >> + u8 num_cur_tch; >> + >> + cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size, >> + si->tch_hdr.max, >> + si->xy_mode + 3 + si->tch_hdr.ofs, >> + si->tch_hdr.bofs); >> + >> + num_cur_tch = tch.hdr; >> + if (num_cur_tch > max_tch) { >> + dev_err(dev, "%s: Num touch err detected (n=%d)\n", >> + __func__, num_cur_tch); >> + num_cur_tch = max_tch; >> + } >> + >> + if (num_cur_tch == 0 && ts->num_prv_rec == 0) >> + return 0; >> + >> + /* extract xy_data for all currently reported touches */ >> + if (num_cur_tch) >> + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch); >> + else >> + cyttsp5_mt_lift_all(ts); >> + >> + return 0; >> +} >> + >> +static int cyttsp5_mt_attention(struct device *dev) >> +{ >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int rc; >> + >> + if (si->xy_mode[2] != HID_TOUCH_REPORT_ID) >> + return 0; >> + >> + /* core handles handshake */ >> + mutex_lock(&ts->mt_lock); >> + rc = cyttsp5_xy_worker(ts); >> + mutex_unlock(&ts->mt_lock); >> + if (rc < 0) >> + dev_err(dev, "%s: xy_worker error r=%d\n", __func__, rc); >> + >> + return rc; >> +} >> + >> +static int cyttsp5_setup_input_device(struct device *dev) >> +{ >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int max_x, max_y, max_p; >> + int max_x_tmp, max_y_tmp; >> + int rc; >> + >> + __set_bit(EV_ABS, ts->input->evbit); >> + __set_bit(EV_REL, ts->input->evbit); >> + __set_bit(EV_KEY, ts->input->evbit); >> + >> + max_x_tmp = si->sensing_conf_data.res_x; >> + max_y_tmp = si->sensing_conf_data.res_y; >> + max_x = max_y_tmp - 1; >> + max_y = max_x_tmp - 1; >> + max_p = si->sensing_conf_data.max_z; >> + >> + input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0); >> + >> + __set_bit(ABS_MT_POSITION_X, ts->input->absbit); >> + __set_bit(ABS_MT_POSITION_Y, ts->input->absbit); >> + __set_bit(ABS_MT_PRESSURE, ts->input->absbit); >> + >> + input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0); >> + input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0); >> + input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0); >> + >> + input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0); >> + input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0); >> + >> + rc = input_register_device(ts->input); >> + if (rc < 0) >> + dev_err(dev, "%s: Error, failed register input device r=%d\n", >> + __func__, rc); >> + >> + return rc; >> +} >> + >> +static int cyttsp5_parse_dt_key_code(struct device *dev) >> +{ >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + struct device_node *np, *pp; >> + int rc, i; >> + >> + np = dev->of_node; >> + if (!np) >> + return -EINVAL; >> + >> + if (si->num_btns) { >> + si->btn = devm_kzalloc(dev, >> + si->num_btns * sizeof(struct cyttsp5_btn), >> + GFP_KERNEL); >> + if (!si->btn) >> + return -ENOMEM; >> + >> + /* Initialized the button to RESERVED */ >> + for (i = 0; i < si->num_btns; i++) { >> + struct cyttsp5_btn *btns = &si->btn[i]; >> + btns->key_code = KEY_RESERVED; >> + btns->enabled = true; >> + } >> + } >> + >> + i = 0; >> + for_each_child_of_node(np, pp) { >> + struct cyttsp5_btn *btns = &si->btn[i]; >> + >> + rc = of_property_read_u32(pp, "linux,code", &btns->key_code); >> + if (rc) { >> + dev_err(dev, "%s: Inval linux,code prop\n", pp->name); >> + return -EINVAL; >> + } >> + btns->enabled = true; >> + >> + i++; >> + } >> + >> + return i; >> +} >> + >> +static int cyttsp5_btn_attention(struct device *dev) >> +{ >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int cur_btn; >> + int cur_btn_state; >> + >> + if (si->xy_mode[2] != HID_BTN_REPORT_ID) >> + return 0; >> + >> + /* core handles handshake */ >> + mutex_lock(&ts->btn_lock); >> + >> + /* extract button press/release touch information */ >> + if (si->num_btns > 0) { >> + for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) { >> + /* Get current button state */ >> + cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN)) >> + & CY_NUM_BTN_EVENT_ID; >> + >> + if (!si->btn[cur_btn].enabled) >> + continue; >> + >> + input_report_key(ts->input, si->btn[cur_btn].key_code, >> + cur_btn_state); >> + input_sync(ts->input); >> + } >> + } >> + >> + mutex_unlock(&ts->btn_lock); >> + >> + return 0; >> +} >> + >> +static const u16 crc_table[16] = { >> + 0x0000, 0x1021, 0x2042, 0x3063, >> + 0x4084, 0x50a5, 0x60c6, 0x70e7, >> + 0x8108, 0x9129, 0xa14a, 0xb16b, >> + 0xc18c, 0xd1ad, 0xe1ce, 0xf1ef, >> +}; > > This looks like a shorter version of crc_itu_t_table. Yes, true > >> +static u16 cyttsp5_compute_crc(u8 *buf, u32 size) >> +{ >> + u16 remainder = 0xFFFF; >> + u16 xor_mask = 0x0000; >> + u32 index; >> + u32 byte_value; >> + u32 table_index; >> + u32 crc_bit_width = sizeof(u16) * 8; >> + >> + /* Divide the message by polynomial, via the table. */ >> + for (index = 0; index < size; index++) { >> + byte_value = buf[index]; >> + table_index = ((byte_value >> 4) & 0x0F) >> + ^ (remainder >> (crc_bit_width - 4)); >> + remainder = crc_table[table_index] ^ (remainder << 4); >> + table_index = (byte_value & 0x0F) >> + ^ (remainder >> (crc_bit_width - 4)); >> + remainder = crc_table[table_index] ^ (remainder << 4); >> + } >> + >> + /* Perform the final remainder CRC. */ >> + return remainder ^ xor_mask; >> +} > > Could it be that it's just using the CRC ITU-T algorithm? The datasheet says it is using a CCITT algorithm with polynomial (x^16 + x^12 + x^5 + 1) whereas the ITU-T has a polynomial of (x^16 + x^12 + x^15 + 1). I guess I can just use the itu table, then. > >> +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code) >> +{ >> + u16 size, crc; >> + u8 status, offset; >> + int command_code; >> + >> + size = get_unaligned_le16(&ts->response_buf[0]); >> + >> + if (!size) >> + return 0; >> + >> + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET]; >> + >> + if (offset == HID_BL_RESPONSE_REPORT_ID) { >> + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) { >> + dev_err(ts->dev, "%s: HID output response, wrong SOP\n", >> + __func__); >> + return -EPROTO; >> + } >> + >> + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) { >> + dev_err(ts->dev, "%s: HID output response, wrong EOP\n", >> + __func__); >> + return -EPROTO; >> + } >> + >> + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7); >> + if (ts->response_buf[size - 3] != LOW_BYTE(crc) >> + || ts->response_buf[size - 2] != HI_BYTE(crc)) { >> + dev_err(ts->dev, "%s: HID output response, wrong CRC 0x%X\n", >> + __func__, crc); >> + return -EPROTO; >> + } >> + >> + status = ts->response_buf[5]; >> + if (status) { >> + dev_err(ts->dev, "%s: HID output response, ERROR:%d\n", >> + __func__, status); >> + return -EPROTO; >> + } >> + } >> + >> + if (offset == HID_APP_RESPONSE_REPORT_ID) { >> + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET] >> + & HID_OUTPUT_RESPONSE_CMD_MASK; >> + if (command_code != code) { >> + dev_err(ts->dev, >> + "%s: HID output response, wrong command_code:%X\n", >> + __func__, command_code); >> + return -EPROTO; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int i; >> + int num_btns = 0; >> + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] >> + & HID_SYSINFO_BTN_MASK; >> + >> + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { >> + if (btns & (1 << i)) >> + num_btns++; >> + } >> + si->num_btns = num_btns; >> +} >> + >> +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) >> +{ >> + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; >> + struct cyttsp5_sensing_conf_data_dev *scd_dev = >> + (struct cyttsp5_sensing_conf_data_dev *) >> + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + >> + cyttsp5_si_get_btn_data(ts); >> + >> + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; >> + scd->res_x = get_unaligned_le16(&scd_dev->res_x); >> + scd->res_y = get_unaligned_le16(&scd_dev->res_y); >> + scd->max_z = get_unaligned_le16(&scd_dev->max_z); >> + scd->len_x = get_unaligned_le16(&scd_dev->len_x); >> + scd->len_y = get_unaligned_le16(&scd_dev->len_y); >> + >> + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, >> + GFP_KERNEL); >> + if (!si->xy_data) >> + return -ENOMEM; >> + >> + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL); >> + if (!si->xy_mode) { >> + devm_kfree(ts->dev, si->xy_data); >> + return -ENOMEM; >> + } >> + >> + return 0; >> +} >> + >> +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) >> +{ >> + int rc, t; >> + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; >> + >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; >> + mutex_unlock(&ts->system_lock); >> + >> + /* HI bytes of Output register address */ >> + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); >> + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); >> + cmd[2] = HID_APP_OUTPUT_REPORT_ID; >> + cmd[3] = 0x0; /* Reserved */ >> + cmd[4] = HID_OUTPUT_GET_SYSINFO; >> + >> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, >> + HID_OUTPUT_GET_SYSINFO_SIZE); >> + if (rc) { >> + dev_err(ts->dev, "%s: Failed to write command %d", >> + __func__, rc); >> + goto error; >> + } >> + >> + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), >> + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); >> + if (IS_TMO(t)) { >> + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", >> + __func__); >> + rc = -ETIME; >> + goto error; >> + } >> + >> + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); >> + goto exit; >> + >> +error: >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = 0; >> + mutex_unlock(&ts->system_lock); >> + return rc; >> +exit: >> + rc = cyttsp5_get_sysinfo_regs(ts); >> + >> + return rc; >> +} >> + >> +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) >> +{ >> + int rc, t; >> + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; >> + u16 crc; >> + >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; >> + mutex_unlock(&ts->system_lock); >> + >> + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); >> + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); >> + cmd[2] = HID_BL_OUTPUT_REPORT_ID; >> + cmd[3] = 0x0; /* Reserved */ >> + cmd[4] = HID_OUTPUT_BL_SOP; >> + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; >> + cmd[6] = 0x0; /* Low bytes of data */ >> + cmd[7] = 0x0; /* Hi bytes of data */ >> + crc = cyttsp5_compute_crc(&cmd[4], 4); >> + cmd[8] = LOW_BYTE(crc); >> + cmd[9] = HI_BYTE(crc); >> + cmd[10] = HID_OUTPUT_BL_EOP; >> + >> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, >> + HID_OUTPUT_BL_LAUNCH_APP_SIZE); > > It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here, > once as setup in cyttsp5_write, and once in the buffer you want to > send. Is that on purpose? I am not sure to see the second time it is sent. What do you mean by "as setup in cyttsp5_write"? Anyway, the size is needed in the buffer of the frame. > >> + if (rc) { >> + dev_err(ts->dev, "%s: Failed to write command %d", >> + __func__, rc); >> + goto error; >> + } >> + >> + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), >> + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT)); >> + if (IS_TMO(t)) { >> + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", >> + __func__); >> + rc = -ETIME; >> + goto error; >> + } >> + >> + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP); >> + goto exit; >> + >> +error: >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = 0; >> + mutex_unlock(&ts->system_lock); >> +exit: >> + return rc; >> +} >> + >> +static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts, >> + struct cyttsp5_hid_desc *desc) >> +{ >> + struct device *dev = ts->dev; >> + __le16 hid_desc_register = HID_DESC_REG; >> + int rc; >> + int t; >> + u8 cmd[2]; >> + >> + /* Read HID descriptor length and version */ >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = 1; >> + mutex_unlock(&ts->system_lock); >> + >> + /* Set HID descriptor register */ >> + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register)); >> + >> + regmap_write(ts->regmap, HID_DESC_REG, cmd[0]); >> + rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]); >> + if (rc < 0) { >> + dev_err(dev, "%s: failed to get HID descriptor, rc=%d\n", >> + __func__, rc); >> + goto error; >> + } >> + >> + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), >> + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT)); >> + if (IS_TMO(t)) { >> + dev_err(ts->dev, "%s: HID get descriptor timed out\n", >> + __func__); >> + rc = -ETIME; >> + goto error; >> + } >> + >> + memcpy((u8 *)desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc)); >> + >> + /* Check HID descriptor length and version */ >> + if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) || >> + le16_to_cpu(desc->bcd_version) != HID_VERSION) { >> + dev_err(dev, "%s: Unsupported HID version\n", __func__); >> + return -ENODEV; >> + } >> + >> + goto exit; >> + >> +error: >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = 0; >> + mutex_unlock(&ts->system_lock); >> +exit: >> + return rc; >> +} >> + >> +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size, >> + int offset) >> +{ >> + tch_abs->ofs = offset / 8; >> + tch_abs->size = report_size / 8; >> + if (report_size % 8) >> + tch_abs->size += 1; >> + tch_abs->min = 0; >> + tch_abs->max = 1 << report_size; >> + tch_abs->bofs = offset - (tch_abs->ofs << 3); >> + >> + return 0; >> +} >> + >> +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) >> +{ >> + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE); >> + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE], >> + BTN_REPORT_SIZE); >> + >> + return 0; >> +} >> + >> +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) >> +{ >> + int max_tch = si->sensing_conf_data.max_tch; >> + int num_cur_tch; >> + int length; >> + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr; >> + >> + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE); >> + >> + cyttsp5_get_touch_axis(&num_cur_tch, tch->size, >> + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs); >> + if (unlikely(num_cur_tch > max_tch)) >> + num_cur_tch = max_tch; >> + >> + length = num_cur_tch * TOUCH_REPORT_SIZE; >> + >> + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length); >> + return 0; >> +} >> + >> +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle) >> +{ >> + struct cyttsp5 *ts = handle; >> + int report_id; >> + int size; >> + int rc; >> + >> + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT); >> + if (!rc) { >> + size = get_unaligned_le16(&ts->input_buf[0]); >> + >> + /* check reset */ >> + if (size == 0) { >> + memcpy(ts->response_buf, ts->input_buf, 2); >> + >> + ts->hid_cmd_state = 0; >> + wake_up(&ts->wait_q); >> + mutex_unlock(&ts->system_lock); > > It doesn't seem like you've taken that mutex anywhere in the > interrupt. Where is it taken? I think it is a removal from the clean-up, thanks. > > Note that taking a mutex in some code path, and releasing it in some > other is pretty confusing, and locks are complicated enough to not > introduce any confusion. Sure, I will pay attention next time. > >> + return IRQ_HANDLED; >> + } >> + >> + report_id = ts->input_buf[2]; >> + >> + if (report_id == HID_TOUCH_REPORT_ID) { >> + move_touch_data(ts, &ts->sysinfo); >> + cyttsp5_mt_attention(ts->dev); >> + } else if (report_id == HID_BTN_REPORT_ID) { >> + move_button_data(ts, &ts->sysinfo); >> + cyttsp5_btn_attention(ts->dev); >> + } else { >> + /* It is not an input but a command response */ >> + memcpy(ts->response_buf, ts->input_buf, size); >> + >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = 0; >> + mutex_unlock(&ts->system_lock); > > And you take and release that lock here, which is even weirder :) > >> + wake_up(&ts->wait_q); >> + } >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int cyttsp5_deassert_int(struct cyttsp5 *ts) >> +{ >> + u16 size; >> + u8 buf[2]; >> + int rc; >> + >> + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); >> + if (rc < 0) >> + return rc; >> + >> + size = get_unaligned_le16(&buf[0]); >> + if (size == 2 || size == 0) >> + return 0; >> + >> + return -EINVAL; >> +} >> + >> +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + >> + fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16, >> + TOUCH_REPORT_DESC_X); >> + fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16, >> + TOUCH_REPORT_DESC_Y); >> + fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8, >> + TOUCH_REPORT_DESC_P); >> + fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5, >> + TOUCH_REPORT_DESC_CONTACTID); >> + fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5, >> + TOUCH_REPORT_DESC_HDR_CONTACTCOUNT); >> + fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8, >> + TOUCH_REPORT_DESC_MAJ); >> + fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8, >> + TOUCH_REPORT_DESC_MIN); >> + >> + return 0; >> +} >> + >> +static int cyttsp5_startup(struct cyttsp5 *ts) >> +{ >> + int rc; >> + >> + rc = cyttsp5_deassert_int(ts); >> + if (rc) { >> + dev_err(ts->dev, "%s: Error on deassert int r=%d\n", >> + __func__, rc); >> + return -ENODEV; >> + } >> + >> + /* >> + * Launch the application as the device starts in bootloader mode >> + * because of a power-on-reset >> + */ >> + rc = cyttsp5_hid_output_bl_launch_app(ts); >> + if (rc < 0) { >> + dev_err(ts->dev, "%s: Error on launch app r=%d\n", >> + __func__, rc); >> + goto exit; >> + } >> + >> + rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc); >> + if (rc < 0) { >> + dev_err(ts->dev, "%s: Error on getting HID descriptor r=%d\n", >> + __func__, rc); >> + goto exit; >> + } >> + >> + rc = cyttsp5_fill_all_touch(ts); >> + if (rc < 0) { >> + dev_err(ts->dev, "%s: Error on getting report descriptor r=%d\n", >> + __func__, rc); >> + goto exit; >> + } >> + >> + rc = cyttsp5_hid_output_get_sysinfo(ts); >> + if (rc) { >> + dev_err(ts->dev, "%s: Error on getting sysinfo r=%d\n", >> + __func__, rc); >> + goto exit; >> + } >> + >> +exit: >> + >> + return rc; >> +} >> + >> +static const struct of_device_id cyttsp5_of_match[] = { >> + { .compatible = "cypress,cyttsp5", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, cyttsp5_of_match); > > You probably want to make that conditional to CONFIG_OF (and use > of_match_ptr on the pointer to that table in your struct driver); Yep! > >> + >> +static const struct i2c_device_id cyttsp5_i2c_id[] = { >> + { CYTTSP5_NAME, 0, }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id); >> + >> +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq, >> + const char *name) >> +{ >> + struct cyttsp5 *ts; >> + struct cyttsp5_sysinfo *si; >> + int rc = 0, i; >> + >> + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); >> + if (!ts) { >> + rc = -ENOMEM; >> + goto error_alloc_data; >> + } >> + >> + /* Initialize device info */ >> + ts->regmap = regmap; >> + ts->dev = dev; >> + si = &ts->sysinfo; >> + dev_set_drvdata(dev, ts); >> + >> + /* Initialize mutexes and spinlocks */ >> + mutex_init(&ts->system_lock); >> + mutex_init(&ts->mt_lock); >> + mutex_init(&ts->btn_lock); >> + >> + /* Initialize wait queue */ >> + init_waitqueue_head(&ts->wait_q); >> + >> + /* Call platform init function */ >> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); >> + if (IS_ERR(ts->reset_gpio)) { >> + rc = PTR_ERR(ts->reset_gpio); >> + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); >> + return rc; >> + } >> + >> + /* Need a delay to have device up */ >> + msleep(20); > > In the case where the device has already been out of reset, this means > that this alone is not sufficient to reset it and bring it out of > reset, which also means that you do not have any guarantee on the > current state of the device. I'm not sure how it would impact the > proper operations though. Okay. A reset frame can be sent to perform a "software reset". Should I add it on startup to be in a "known behavior"? > >> + >> + ts->irq = irq; > > You don't seem to use ts->irq anywhere else, you can probably just > keep it as a local variable. Yes, removed for v2. > >> + rc = devm_request_threaded_irq(dev, ts->irq, NULL, cyttsp5_handle_irq, >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> + name, ts); >> + if (rc) { >> + dev_err(dev, "unable to request IRQ\n"); >> + goto error_setup_irq; >> + } >> + >> + rc = cyttsp5_startup(ts); >> + if (rc) { >> + dev_err(ts->dev, "%s: Fail initial startup r=%d\n", >> + __func__, rc); >> + goto error_startup; >> + } >> + >> + rc = cyttsp5_parse_dt_key_code(dev); >> + if (rc < 0) { >> + dev_err(ts->dev, "%s: Error while parsing dts\n", __func__); >> + goto error_startup; >> + } >> + >> + ts->input = input_allocate_device(); >> + if (!ts->input) { >> + dev_err(dev, "%s: Error, failed to allocate input device\n", >> + __func__); >> + rc = -ENODEV; >> + goto error_startup; >> + } > > In error_startup, you never uninitialize the device. Given your > comment in cyttsp5_startup, this means that you might end up in a > situation where your driver probes again with the device initialized > and no longer in a bootloader mode, which is likely to cause some > error. > > Putting the call to cyttsp5_startup later will make you less likely to > fail (especially because of the DT parsing), and putting the device > back into reset will probably address the issue once and for all. Hm, I am not sure to understand what you want to say, here. Could you explain me more what you have in mind? Notice that the DT parsing uses a sysinfo's variable (si->num_btns) which is retrieved in the startup function (thanks to get_sysinfo). So, currently, it is not possible to move the startup function after the DT parsing. > >> + ts->input->name = "cyttsp5"; >> + scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0); >> + ts->input->phys = ts->phys; >> + ts->input->dev.parent = ts->dev; >> + input_set_drvdata(ts->input, ts); >> + >> + touchscreen_parse_properties(ts->input, true, NULL); >> + >> + if (si) { >> + __set_bit(EV_KEY, ts->input->evbit); >> + for (i = 0; i < si->num_btns; i++) >> + __set_bit(si->btn[i].key_code, ts->input->keybit); >> + >> + rc = cyttsp5_setup_input_device(dev); >> + if (rc) >> + goto error_init_input; >> + } >> + >> + return 0; >> + >> +error_init_input: >> + input_free_device(ts->input); >> +error_startup: >> +error_setup_irq: >> + dev_set_drvdata(dev, NULL); >> +error_alloc_data: >> + dev_err(dev, "%s failed.\n", __func__); > > I'm not sure using those __func__ everywhere is worth it, and given > your code, this error log is redundant with the "real" error log you > used before the goto. Sure, the vendor's driver had these dev_err prints and, actually, I did not modify them. > >> + if (dev->of_node) >> + of_node_put(dev->of_node); > > I haven't seen any of_node_get, or any function that would do it, > called in your driver so far, why do you need it? I forgot to remove it from the initial driver, thanks. > >> + >> + return rc; >> +} >> + >> +static int cyttsp5_i2c_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct regmap *regmap; >> + static const struct regmap_config config = { >> + .reg_bits = 8, >> + .val_bits = 8, >> + }; >> + >> + regmap = devm_regmap_init_i2c(client, &config); >> + if (IS_ERR(regmap)) { >> + dev_err(&client->dev, "%s: regmap allocation failed: %ld\n", >> + __func__, PTR_ERR(regmap)); >> + return PTR_ERR(regmap); >> + } >> + >> + return cyttsp5_probe(&client->dev, regmap, client->irq, client->name); >> +} >> + >> +static int cyttsp5_remove(struct device *dev) >> +{ >> + const struct of_device_id *match; >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + >> + input_unregister_device(ts->input); >> + >> + dev_set_drvdata(dev, NULL); >> + >> + match = of_match_device(of_match_ptr(cyttsp5_of_match), dev); >> + if (match && dev->of_node) >> + of_node_put(dev->of_node); >> + >> + return 0; >> +} >> + >> +static int cyttsp5_i2c_remove(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + >> + return cyttsp5_remove(dev); >> +} >> + >> +static struct i2c_driver cyttsp5_i2c_driver = { >> + .driver = { >> + .name = CYTTSP5_NAME, >> + .owner = THIS_MODULE, >> + .of_match_table = cyttsp5_of_match, >> + }, >> + .probe = cyttsp5_i2c_probe, >> + .remove = cyttsp5_i2c_remove, >> + .id_table = cyttsp5_i2c_id, >> +}; >> + >> +module_i2c_driver(cyttsp5_i2c_driver); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product"); >> +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@free-electrons.com>"); > > Good work cleaning it up! Thank you! Best regards, -- Mylène Josserand, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-06-06 8:03 ` Mylene Josserand @ 2017-06-06 12:04 ` Maxime Ripard 2017-06-06 14:32 ` Mylene Josserand 0 siblings, 1 reply; 18+ messages in thread From: Maxime Ripard @ 2017-06-06 12:04 UTC (permalink / raw) To: Mylene Josserand Cc: dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, thomas.petazzoni [-- Attachment #1: Type: text/plain, Size: 8993 bytes --] On Tue, Jun 06, 2017 at 10:03:48AM +0200, Mylene Josserand wrote: > > > +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code) > > > +{ > > > + u16 size, crc; > > > + u8 status, offset; > > > + int command_code; > > > + > > > + size = get_unaligned_le16(&ts->response_buf[0]); > > > + > > > + if (!size) > > > + return 0; > > > + > > > + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET]; > > > + > > > + if (offset == HID_BL_RESPONSE_REPORT_ID) { > > > + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) { > > > + dev_err(ts->dev, "%s: HID output response, wrong SOP\n", > > > + __func__); > > > + return -EPROTO; > > > + } > > > + > > > + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) { > > > + dev_err(ts->dev, "%s: HID output response, wrong EOP\n", > > > + __func__); > > > + return -EPROTO; > > > + } > > > + > > > + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7); > > > + if (ts->response_buf[size - 3] != LOW_BYTE(crc) > > > + || ts->response_buf[size - 2] != HI_BYTE(crc)) { > > > + dev_err(ts->dev, "%s: HID output response, wrong CRC 0x%X\n", > > > + __func__, crc); > > > + return -EPROTO; > > > + } > > > + > > > + status = ts->response_buf[5]; > > > + if (status) { > > > + dev_err(ts->dev, "%s: HID output response, ERROR:%d\n", > > > + __func__, status); > > > + return -EPROTO; > > > + } > > > + } > > > + > > > + if (offset == HID_APP_RESPONSE_REPORT_ID) { > > > + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET] > > > + & HID_OUTPUT_RESPONSE_CMD_MASK; > > > + if (command_code != code) { > > > + dev_err(ts->dev, > > > + "%s: HID output response, wrong command_code:%X\n", > > > + __func__, command_code); > > > + return -EPROTO; > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) > > > +{ > > > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > > > + int i; > > > + int num_btns = 0; > > > + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] > > > + & HID_SYSINFO_BTN_MASK; > > > + > > > + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { > > > + if (btns & (1 << i)) > > > + num_btns++; > > > + } > > > + si->num_btns = num_btns; > > > +} > > > + > > > +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) > > > +{ > > > + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; > > > + struct cyttsp5_sensing_conf_data_dev *scd_dev = > > > + (struct cyttsp5_sensing_conf_data_dev *) > > > + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; > > > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > > > + > > > + cyttsp5_si_get_btn_data(ts); > > > + > > > + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; > > > + scd->res_x = get_unaligned_le16(&scd_dev->res_x); > > > + scd->res_y = get_unaligned_le16(&scd_dev->res_y); > > > + scd->max_z = get_unaligned_le16(&scd_dev->max_z); > > > + scd->len_x = get_unaligned_le16(&scd_dev->len_x); > > > + scd->len_y = get_unaligned_le16(&scd_dev->len_y); > > > + > > > + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, > > > + GFP_KERNEL); > > > + if (!si->xy_data) > > > + return -ENOMEM; > > > + > > > + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL); > > > + if (!si->xy_mode) { > > > + devm_kfree(ts->dev, si->xy_data); > > > + return -ENOMEM; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) > > > +{ > > > + int rc, t; > > > + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; > > > + > > > + mutex_lock(&ts->system_lock); > > > + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; > > > + mutex_unlock(&ts->system_lock); > > > + > > > + /* HI bytes of Output register address */ > > > + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); > > > + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); > > > + cmd[2] = HID_APP_OUTPUT_REPORT_ID; > > > + cmd[3] = 0x0; /* Reserved */ > > > + cmd[4] = HID_OUTPUT_GET_SYSINFO; > > > + > > > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, > > > + HID_OUTPUT_GET_SYSINFO_SIZE); > > > + if (rc) { > > > + dev_err(ts->dev, "%s: Failed to write command %d", > > > + __func__, rc); > > > + goto error; > > > + } > > > + > > > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > > > + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); > > > + if (IS_TMO(t)) { > > > + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", > > > + __func__); > > > + rc = -ETIME; > > > + goto error; > > > + } > > > + > > > + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); > > > + goto exit; > > > + > > > +error: > > > + mutex_lock(&ts->system_lock); > > > + ts->hid_cmd_state = 0; > > > + mutex_unlock(&ts->system_lock); > > > + return rc; > > > +exit: > > > + rc = cyttsp5_get_sysinfo_regs(ts); > > > + > > > + return rc; > > > +} > > > + > > > +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) > > > +{ > > > + int rc, t; > > > + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; > > > + u16 crc; > > > + > > > + mutex_lock(&ts->system_lock); > > > + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; > > > + mutex_unlock(&ts->system_lock); > > > + > > > + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); > > > + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); > > > + cmd[2] = HID_BL_OUTPUT_REPORT_ID; > > > + cmd[3] = 0x0; /* Reserved */ > > > + cmd[4] = HID_OUTPUT_BL_SOP; > > > + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; > > > + cmd[6] = 0x0; /* Low bytes of data */ > > > + cmd[7] = 0x0; /* Hi bytes of data */ > > > + crc = cyttsp5_compute_crc(&cmd[4], 4); > > > + cmd[8] = LOW_BYTE(crc); > > > + cmd[9] = HI_BYTE(crc); > > > + cmd[10] = HID_OUTPUT_BL_EOP; > > > + > > > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, > > > + HID_OUTPUT_BL_LAUNCH_APP_SIZE); > > > > It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here, > > once as setup in cyttsp5_write, and once in the buffer you want to > > send. Is that on purpose? > > I am not sure to see the second time it is sent. What do you mean by "as > setup in cyttsp5_write"? Anyway, the size is needed in the buffer of the > frame. Nevermind, I confused the HID_OUTPUT_BL_LAUNCH_APP_SIZE with the register, my bad. > > > + /* Call platform init function */ > > > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > > > + if (IS_ERR(ts->reset_gpio)) { > > > + rc = PTR_ERR(ts->reset_gpio); > > > + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); > > > + return rc; > > > + } > > > + > > > + /* Need a delay to have device up */ > > > + msleep(20); > > > > In the case where the device has already been out of reset, this means > > that this alone is not sufficient to reset it and bring it out of > > reset, which also means that you do not have any guarantee on the > > current state of the device. I'm not sure how it would impact the > > proper operations though. > > Okay. A reset frame can be sent to perform a "software > reset". Should I add it on startup to be in a "known behavior"? I guess you'd be safer just getting the GPIO with the low level by default, and then changing to high. That way you know that you went through a reset state, before bringing up the device. > > > + ts->input = input_allocate_device(); > > > + if (!ts->input) { > > > + dev_err(dev, "%s: Error, failed to allocate input device\n", > > > + __func__); > > > + rc = -ENODEV; > > > + goto error_startup; > > > + } > > > > In error_startup, you never uninitialize the device. Given your > > comment in cyttsp5_startup, this means that you might end up in a > > situation where your driver probes again with the device initialized > > and no longer in a bootloader mode, which is likely to cause some > > error. > > > > Putting the call to cyttsp5_startup later will make you less likely to > > fail (especially because of the DT parsing), and putting the device > > back into reset will probably address the issue once and for all. > > Hm, I am not sure to understand what you want to say, here. > Could you explain me more what you have in mind? I meant to say that there was still an issue with the reset here, and moving the DT parsing code further would ease the reset operation. > Notice that the DT parsing uses a sysinfo's variable (si->num_btns) > which is retrieved in the startup function (thanks to > get_sysinfo). So, currently, it is not possible to move the startup > function after the DT parsing. Can't that be made the other way around? You parse the number of buttons in the DT, and check the consistency with the hardware? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-06-06 12:04 ` Maxime Ripard @ 2017-06-06 14:32 ` Mylene Josserand [not found] ` <0232a641-c741-1f95-9086-840b31f5d055-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 0 siblings, 1 reply; 18+ messages in thread From: Mylene Josserand @ 2017-06-06 14:32 UTC (permalink / raw) To: Maxime Ripard Cc: dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, thomas.petazzoni Maxime, On 06/06/2017 14:04, Maxime Ripard wrote: > On Tue, Jun 06, 2017 at 10:03:48AM +0200, Mylene Josserand wrote: >>>> +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) >>>> +{ >>>> + int rc, t; >>>> + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; >>>> + u16 crc; >>>> + >>>> + mutex_lock(&ts->system_lock); >>>> + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; >>>> + mutex_unlock(&ts->system_lock); >>>> + >>>> + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); >>>> + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); >>>> + cmd[2] = HID_BL_OUTPUT_REPORT_ID; >>>> + cmd[3] = 0x0; /* Reserved */ >>>> + cmd[4] = HID_OUTPUT_BL_SOP; >>>> + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; >>>> + cmd[6] = 0x0; /* Low bytes of data */ >>>> + cmd[7] = 0x0; /* Hi bytes of data */ >>>> + crc = cyttsp5_compute_crc(&cmd[4], 4); >>>> + cmd[8] = LOW_BYTE(crc); >>>> + cmd[9] = HI_BYTE(crc); >>>> + cmd[10] = HID_OUTPUT_BL_EOP; >>>> + >>>> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, >>>> + HID_OUTPUT_BL_LAUNCH_APP_SIZE); >>> >>> It seems like you'll send HID_OUTPUT_BL_LAUNCH_APP_SIZE twice here, >>> once as setup in cyttsp5_write, and once in the buffer you want to >>> send. Is that on purpose? >> >> I am not sure to see the second time it is sent. What do you mean by "as >> setup in cyttsp5_write"? Anyway, the size is needed in the buffer of the >> frame. > > Nevermind, I confused the HID_OUTPUT_BL_LAUNCH_APP_SIZE with the > register, my bad. > No problem. >>>> + /* Call platform init function */ >>>> + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); >>>> + if (IS_ERR(ts->reset_gpio)) { >>>> + rc = PTR_ERR(ts->reset_gpio); >>>> + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); >>>> + return rc; >>>> + } >>>> + >>>> + /* Need a delay to have device up */ >>>> + msleep(20); >>> >>> In the case where the device has already been out of reset, this means >>> that this alone is not sufficient to reset it and bring it out of >>> reset, which also means that you do not have any guarantee on the >>> current state of the device. I'm not sure how it would impact the >>> proper operations though. >> >> Okay. A reset frame can be sent to perform a "software >> reset". Should I add it on startup to be in a "known behavior"? > > I guess you'd be safer just getting the GPIO with the low level by > default, and then changing to high. That way you know that you went > through a reset state, before bringing up the device. Okay, I will update the driver, thanks! >>>> + ts->input = input_allocate_device(); >>>> + if (!ts->input) { >>>> + dev_err(dev, "%s: Error, failed to allocate input device\n", >>>> + __func__); >>>> + rc = -ENODEV; >>>> + goto error_startup; >>>> + } >>> >>> In error_startup, you never uninitialize the device. Given your >>> comment in cyttsp5_startup, this means that you might end up in a >>> situation where your driver probes again with the device initialized >>> and no longer in a bootloader mode, which is likely to cause some >>> error. >>> >>> Putting the call to cyttsp5_startup later will make you less likely to >>> fail (especially because of the DT parsing), and putting the device >>> back into reset will probably address the issue once and for all. >> >> Hm, I am not sure to understand what you want to say, here. >> Could you explain me more what you have in mind? > > I meant to say that there was still an issue with the reset here, and > moving the DT parsing code further would ease the reset operation. Okay > >> Notice that the DT parsing uses a sysinfo's variable (si->num_btns) >> which is retrieved in the startup function (thanks to >> get_sysinfo). So, currently, it is not possible to move the startup >> function after the DT parsing. > > Can't that be made the other way around? You parse the number of > buttons in the DT, and check the consistency with the hardware? It could be possible but currently, it was designed to enable every button from hardware's configuration and DT binding was just a way to customize button's keycode (default is KEY_RESERVED). If I base it on the DT, it means that some buttons could never be reported (such as deactivated) because they will not be present in the DT. I am not sure if it is the behavior we want. What do you think? Best regards, -- Mylène Josserand, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <0232a641-c741-1f95-9086-840b31f5d055-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen [not found] ` <0232a641-c741-1f95-9086-840b31f5d055-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-07 9:16 ` Maxime Ripard 0 siblings, 0 replies; 18+ messages in thread From: Maxime Ripard @ 2017-06-07 9:16 UTC (permalink / raw) To: Mylene Josserand Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, fery-+wT8y+m8/X5BDgjK7y7TUQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-input-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8 [-- Attachment #1: Type: text/plain, Size: 1093 bytes --] On Tue, Jun 06, 2017 at 04:32:46PM +0200, Mylene Josserand wrote: > > > Notice that the DT parsing uses a sysinfo's variable (si->num_btns) > > > which is retrieved in the startup function (thanks to > > > get_sysinfo). So, currently, it is not possible to move the startup > > > function after the DT parsing. > > > > Can't that be made the other way around? You parse the number of > > buttons in the DT, and check the consistency with the hardware? > > It could be possible but currently, it was designed to enable every button > from hardware's configuration and DT binding was just a way to customize > button's keycode (default is KEY_RESERVED). > If I base it on the DT, it means that some buttons could never be reported > (such as deactivated) because they will not be present in the DT. I am not > sure if it is the behavior we want. What do you think? I don't really know. Both look reasonable, and it seems to be more a matter of policy. Dmitry? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand ` (2 preceding siblings ...) 2017-05-30 8:02 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Maxime Ripard @ 2017-05-30 8:11 ` Thomas Petazzoni [not found] ` <20170529144538.29187-2-mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 4 siblings, 0 replies; 18+ messages in thread From: Thomas Petazzoni @ 2017-05-30 8:11 UTC (permalink / raw) To: Mylène Josserand Cc: dmitry.torokhov, fery, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, maxime.ripard Hello, Couple more comments in addition to the more extensive review done by Maxime. On Mon, 29 May 2017 16:45:37 +0200, Mylène Josserand wrote: > +static int cyttsp5_startup(struct cyttsp5 *ts) > +{ > + int rc; > + > + rc = cyttsp5_deassert_int(ts); > + if (rc) { > + dev_err(ts->dev, "%s: Error on deassert int r=%d\n", > + __func__, rc); I don't think it's very common to put __func__ in error messages, especially when dev_err() already prints the name of the dvice. > + return -ENODEV; > + } > + > + /* > + * Launch the application as the device starts in bootloader mode > + * because of a power-on-reset > + */ > + rc = cyttsp5_hid_output_bl_launch_app(ts); > + if (rc < 0) { > + dev_err(ts->dev, "%s: Error on launch app r=%d\n", > + __func__, rc); > + goto exit; Replace "goto exit" by "return rc;" since anyway the exit label doesn't do anything. > +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq, > + const char *name) > +{ > + struct cyttsp5 *ts; > + struct cyttsp5_sysinfo *si; > + int rc = 0, i; > + > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); > + if (!ts) { > + rc = -ENOMEM; > + goto error_alloc_data; Just return directly here (because the of_node_put is useless, see below). > + } > + > + /* Initialize device info */ > + ts->regmap = regmap; > + ts->dev = dev; > + si = &ts->sysinfo; So "si" is never NULL. > + if (si) { And therefore you can do this unconditionally. > + __set_bit(EV_KEY, ts->input->evbit); > + for (i = 0; i < si->num_btns; i++) > + __set_bit(si->btn[i].key_code, ts->input->keybit); > + > + rc = cyttsp5_setup_input_device(dev); > + if (rc) > + goto error_init_input; > + } > + > + return 0; > + > +error_init_input: > + input_free_device(ts->input); What about using devm_input_allocate_device() ? > +error_startup: > +error_setup_irq: > + dev_set_drvdata(dev, NULL); > +error_alloc_data: > + dev_err(dev, "%s failed.\n", __func__); > + if (dev->of_node) > + of_node_put(dev->of_node); As Maxime said, this is not necessary. > +static int cyttsp5_remove(struct device *dev) > +{ > + const struct of_device_id *match; > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + > + input_unregister_device(ts->input); > + > + dev_set_drvdata(dev, NULL); > + > + match = of_match_device(of_match_ptr(cyttsp5_of_match), dev); > + if (match && dev->of_node) > + of_node_put(dev->of_node); Ditto. I'm also not sure that doing a dev_set_drvdata(dev, NULL) is really needed in ->remove() and in the ->probe() exit path. Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <20170529144538.29187-2-mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen [not found] ` <20170529144538.29187-2-mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-05-30 8:43 ` Thomas Petazzoni [not found] ` <20170530104302.6db658f0-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 0 siblings, 1 reply; 18+ messages in thread From: Thomas Petazzoni @ 2017-05-30 8:43 UTC (permalink / raw) To: Mylène Josserand Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, fery-+wT8y+m8/X5BDgjK7y7TUQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-input-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8 Hello, A few more comments :) On Mon, 29 May 2017 16:45:37 +0200, Mylène Josserand wrote: > +struct cyttsp5 { > + struct device *dev; > + struct mutex system_lock; > + struct mutex btn_lock; > + struct mutex mt_lock; Three mutexes for such a driver is probably excessive, especially when they are in fact used "sequentially". > +static int cyttsp5_parse_dt_key_code(struct device *dev) > +{ > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + struct device_node *np, *pp; > + int rc, i; > + > + np = dev->of_node; > + if (!np) > + return -EINVAL; > + > + if (si->num_btns) { Invert the test, and remove one indentation level: if (!si->num_btns) return 0; > + si->btn = devm_kzalloc(dev, > + si->num_btns * sizeof(struct cyttsp5_btn), > + GFP_KERNEL); > + if (!si->btn) > + return -ENOMEM; > + > + /* Initialized the button to RESERVED */ Initialized -> Initialize > + for (i = 0; i < si->num_btns; i++) { > + struct cyttsp5_btn *btns = &si->btn[i]; > + btns->key_code = KEY_RESERVED; > + btns->enabled = true; Does it make sense to have the button as "enabled" when it in fact isn't really? Also setting btns->enabled = true is done again below. > + } > + } > + > + i = 0; > + for_each_child_of_node(np, pp) { > + struct cyttsp5_btn *btns = &si->btn[i]; > + > + rc = of_property_read_u32(pp, "linux,code", &btns->key_code); > + if (rc) { > + dev_err(dev, "%s: Inval linux,code prop\n", pp->name); > + return -EINVAL; Propagate "rc". > + } Add blank line here. > + btns->enabled = true; > + > + i++; > + } si->num_btns comes from the HW (calculated in function cyttsp5_si_get_btn_data), but then you are trusting the DT to have the exact same number of buttons. If the DT has more button that si->num_btns, then you have a buffer overflow here because you for_each_child_of_node() loop will loop after the end of si->btn[]. > +static int cyttsp5_btn_attention(struct device *dev) > +{ > + struct cyttsp5 *ts = dev_get_drvdata(dev); > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int cur_btn; > + int cur_btn_state; > + > + if (si->xy_mode[2] != HID_BTN_REPORT_ID) > + return 0; > + > + /* core handles handshake */ > + mutex_lock(&ts->btn_lock); > + > + /* extract button press/release touch information */ > + if (si->num_btns > 0) { Invert this test: if (!si->num_btns) return 0; and then you save one indentation level. Of course, move the test outside the mutex lock/unlock section. > +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) > +{ > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + int i; > + int num_btns = 0; > + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] > + & HID_SYSINFO_BTN_MASK; > + > + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { > + if (btns & (1 << i)) Replace (1 << i) by BIT(i) > + num_btns++; > + } > + si->num_btns = num_btns; Does it really make sense to have a temporary variable, rather than using si->num_btns directly? > +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) > +{ > + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; > + struct cyttsp5_sensing_conf_data_dev *scd_dev = > + (struct cyttsp5_sensing_conf_data_dev *) > + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; > + struct cyttsp5_sysinfo *si = &ts->sysinfo; > + > + cyttsp5_si_get_btn_data(ts); > + > + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; > + scd->res_x = get_unaligned_le16(&scd_dev->res_x); > + scd->res_y = get_unaligned_le16(&scd_dev->res_y); > + scd->max_z = get_unaligned_le16(&scd_dev->max_z); > + scd->len_x = get_unaligned_le16(&scd_dev->len_x); > + scd->len_y = get_unaligned_le16(&scd_dev->len_y); > + > + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, > + GFP_KERNEL); > + if (!si->xy_data) > + return -ENOMEM; > + > + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL); > + if (!si->xy_mode) { > + devm_kfree(ts->dev, si->xy_data); The point of devm_kzalloc() is to not need to do a devm_kfree(). Of course, check you're doing this only in the ->probe() but it seems to be the case. > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) > +{ > + int rc, t; > + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; > + > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; > + mutex_unlock(&ts->system_lock); This is called only from the _startup() function, itself called from the _probe() function. So taking a mutex is useless here, there is no concurrency. This hid_cmd_state thing is a bit strange. It's set to the following values: + ts->hid_cmd_state = 1; + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; and when the interrupt occurs it's set back to 0. So: why those values? Why this "1" and then those magic values + 1 ? Do you need something else than BUSY/DONE ? What about: enum { HID_CMD_DONE, HID_CMD_BUSY } hid_cmd_state; > + > + /* HI bytes of Output register address */ > + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); > + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); > + cmd[2] = HID_APP_OUTPUT_REPORT_ID; > + cmd[3] = 0x0; /* Reserved */ > + cmd[4] = HID_OUTPUT_GET_SYSINFO; > + > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, > + HID_OUTPUT_GET_SYSINFO_SIZE); > + if (rc) { > + dev_err(ts->dev, "%s: Failed to write command %d", > + __func__, rc); > + goto error; > + } > + > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); > + if (IS_TMO(t)) { I don't think this IS_TMO() macro makes sense. And you case use "rc" as the return value. > + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", > + __func__); > + rc = -ETIME; > + goto error; > + } > + > + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); > + goto exit; It's really weird for the "normal" case to jump to an "exit" label. Why don't you handle the normal case here? Also, this code means that you're currently ignoring the return value of cyttsp5_validate_cmd_response(). > + > +error: > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = 0; > + mutex_unlock(&ts->system_lock); > + return rc; > +exit: > + rc = cyttsp5_get_sysinfo_regs(ts); > + > + return rc; > +} > + > +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) > +{ > + int rc, t; > + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; > + u16 crc; > + > + mutex_lock(&ts->system_lock); > + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; > + mutex_unlock(&ts->system_lock); > + > + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); > + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); > + cmd[2] = HID_BL_OUTPUT_REPORT_ID; > + cmd[3] = 0x0; /* Reserved */ > + cmd[4] = HID_OUTPUT_BL_SOP; > + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; > + cmd[6] = 0x0; /* Low bytes of data */ > + cmd[7] = 0x0; /* Hi bytes of data */ > + crc = cyttsp5_compute_crc(&cmd[4], 4); > + cmd[8] = LOW_BYTE(crc); > + cmd[9] = HI_BYTE(crc); > + cmd[10] = HID_OUTPUT_BL_EOP; > + > + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, > + HID_OUTPUT_BL_LAUNCH_APP_SIZE); > + if (rc) { > + dev_err(ts->dev, "%s: Failed to write command %d", > + __func__, rc); > + goto error; > + } > + > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT)); > + if (IS_TMO(t)) { > + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", > + __func__); > + rc = -ETIME; > + goto error; > + } > + > + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP); > + goto exit; Same comments as for the previous function. > + /* Set HID descriptor register */ > + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register)); > + > + regmap_write(ts->regmap, HID_DESC_REG, cmd[0]); Return value? > + rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]); Why are you sending these through two regmap_write() calls and not a regmap_bulk_write() ? > + if (rc < 0) { > + dev_err(dev, "%s: failed to get HID descriptor, rc=%d\n", > + __func__, rc); > + goto error; > + } > + > + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), > + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT)); > + if (IS_TMO(t)) { Same comment as above: don't use IS_TMO(), use "rc" instead of "t". > + dev_err(ts->dev, "%s: HID get descriptor timed out\n", > + __func__); > + rc = -ETIME; > + goto error; > + } > + > + memcpy((u8 *)desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc)); I don't see why the u8* cast would be needed here. > +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size, > + int offset) > +{ > + tch_abs->ofs = offset / 8; > + tch_abs->size = report_size / 8; > + if (report_size % 8) > + tch_abs->size += 1; > + tch_abs->min = 0; > + tch_abs->max = 1 << report_size; > + tch_abs->bofs = offset - (tch_abs->ofs << 3); > + > + return 0; > +} > + > +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) > +{ > + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE); > + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE], > + BTN_REPORT_SIZE); > + > + return 0; > +} > + > +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) > +{ > + int max_tch = si->sensing_conf_data.max_tch; > + int num_cur_tch; > + int length; > + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr; > + > + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE); > + > + cyttsp5_get_touch_axis(&num_cur_tch, tch->size, > + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs); > + if (unlikely(num_cur_tch > max_tch)) > + num_cur_tch = max_tch; > + > + length = num_cur_tch * TOUCH_REPORT_SIZE; > + > + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length); Blank line here. > + return 0; > +} > + > +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle) > +{ > + struct cyttsp5 *ts = handle; > + int report_id; > + int size; > + int rc; > + > + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT); > + if (!rc) { Same as usual: to remove one indentation level, invert the test, by doing: if (rc) return IRQ_HANDLED; > +static int cyttsp5_deassert_int(struct cyttsp5 *ts) > +{ > + u16 size; > + u8 buf[2]; > + int rc; > + > + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); Which register are you reading here? > + scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0); Why use %d if the value is always 0 ? Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <20170530104302.6db658f0-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen [not found] ` <20170530104302.6db658f0-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2017-06-06 13:20 ` Mylene Josserand 0 siblings, 0 replies; 18+ messages in thread From: Mylene Josserand @ 2017-06-06 13:20 UTC (permalink / raw) To: Thomas Petazzoni Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, fery-+wT8y+m8/X5BDgjK7y7TUQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-input-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8 Hello Thomas, On 30/05/2017 10:43, Thomas Petazzoni wrote: > Hello, > > A few more comments :) Thank you for the review! > > On Mon, 29 May 2017 16:45:37 +0200, Mylène Josserand wrote: >> +struct cyttsp5 { >> + struct device *dev; >> + struct mutex system_lock; >> + struct mutex btn_lock; >> + struct mutex mt_lock; > > Three mutexes for such a driver is probably excessive, especially when > they are in fact used "sequentially". Yes, I will have a look at these tree mutexes to see if there are necessary or not. > >> +static int cyttsp5_parse_dt_key_code(struct device *dev) >> +{ >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + struct device_node *np, *pp; >> + int rc, i; >> + >> + np = dev->of_node; >> + if (!np) >> + return -EINVAL; >> + >> + if (si->num_btns) { > > Invert the test, and remove one indentation level: Thanks! > > if (!si->num_btns) > return 0; > >> + si->btn = devm_kzalloc(dev, >> + si->num_btns * sizeof(struct cyttsp5_btn), >> + GFP_KERNEL); >> + if (!si->btn) >> + return -ENOMEM; >> + >> + /* Initialized the button to RESERVED */ > > Initialized -> Initialize > >> + for (i = 0; i < si->num_btns; i++) { >> + struct cyttsp5_btn *btns = &si->btn[i]; >> + btns->key_code = KEY_RESERVED; >> + btns->enabled = true; > > Does it make sense to have the button as "enabled" when it in fact > isn't really? Also setting btns->enabled = true is done again below. No, it was used in the initial driver. I can remove it now. > >> + } >> + } >> + >> + i = 0; >> + for_each_child_of_node(np, pp) { >> + struct cyttsp5_btn *btns = &si->btn[i]; >> + >> + rc = of_property_read_u32(pp, "linux,code", &btns->key_code); >> + if (rc) { >> + dev_err(dev, "%s: Inval linux,code prop\n", pp->name); >> + return -EINVAL; > > Propagate "rc". > >> + } > > Add blank line here. > >> + btns->enabled = true; >> + >> + i++; >> + } > > si->num_btns comes from the HW (calculated in function > cyttsp5_si_get_btn_data), but then you are trusting the DT to have the > exact same number of buttons. > > If the DT has more button that si->num_btns, then you have a buffer > overflow here because you for_each_child_of_node() loop will loop after > the end of si->btn[]. Oh, thanks for the catch! > >> +static int cyttsp5_btn_attention(struct device *dev) >> +{ >> + struct cyttsp5 *ts = dev_get_drvdata(dev); >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int cur_btn; >> + int cur_btn_state; >> + >> + if (si->xy_mode[2] != HID_BTN_REPORT_ID) >> + return 0; >> + >> + /* core handles handshake */ >> + mutex_lock(&ts->btn_lock); >> + >> + /* extract button press/release touch information */ >> + if (si->num_btns > 0) { > > Invert this test: > > if (!si->num_btns) > return 0; > > and then you save one indentation level. > > Of course, move the test outside the mutex lock/unlock section. ACK > > >> +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) >> +{ >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + int i; >> + int num_btns = 0; >> + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] >> + & HID_SYSINFO_BTN_MASK; >> + >> + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { >> + if (btns & (1 << i)) > > Replace (1 << i) by BIT(i) ACK > >> + num_btns++; >> + } >> + si->num_btns = num_btns; > > Does it really make sense to have a temporary variable, rather than > using si->num_btns directly? Not at all :) > >> +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) >> +{ >> + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; >> + struct cyttsp5_sensing_conf_data_dev *scd_dev = >> + (struct cyttsp5_sensing_conf_data_dev *) >> + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; >> + struct cyttsp5_sysinfo *si = &ts->sysinfo; >> + >> + cyttsp5_si_get_btn_data(ts); >> + >> + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; >> + scd->res_x = get_unaligned_le16(&scd_dev->res_x); >> + scd->res_y = get_unaligned_le16(&scd_dev->res_y); >> + scd->max_z = get_unaligned_le16(&scd_dev->max_z); >> + scd->len_x = get_unaligned_le16(&scd_dev->len_x); >> + scd->len_y = get_unaligned_le16(&scd_dev->len_y); >> + >> + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, >> + GFP_KERNEL); >> + if (!si->xy_data) >> + return -ENOMEM; >> + >> + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, GFP_KERNEL); >> + if (!si->xy_mode) { >> + devm_kfree(ts->dev, si->xy_data); > > The point of devm_kzalloc() is to not need to do a devm_kfree(). Of > course, check you're doing this only in the ->probe() but it seems to > be the case. Okay > >> + return -ENOMEM; >> + } >> + >> + return 0; >> +} >> + >> +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) >> +{ >> + int rc, t; >> + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; >> + >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; >> + mutex_unlock(&ts->system_lock); > > This is called only from the _startup() function, itself called from > the _probe() function. So taking a mutex is useless here, there is no > concurrency. > > This hid_cmd_state thing is a bit strange. It's set to the following > values: > > + ts->hid_cmd_state = 1; > + ts->hid_cmd_state = HID_OUTPUT_GET_SYSINFO + 1; > + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; > > and when the interrupt occurs it's set back to 0. > > So: why those values? Why this "1" and then those magic values + 1 ? Do > you need something else than BUSY/DONE ? > > What about: > > enum { HID_CMD_DONE, HID_CMD_BUSY } hid_cmd_state; > The initial driver was doing it like this, I missed this refactoring during my clean-up. Thank you for the remark, it is much more understandable like that. >> + >> + /* HI bytes of Output register address */ >> + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); >> + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); >> + cmd[2] = HID_APP_OUTPUT_REPORT_ID; >> + cmd[3] = 0x0; /* Reserved */ >> + cmd[4] = HID_OUTPUT_GET_SYSINFO; >> + >> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, >> + HID_OUTPUT_GET_SYSINFO_SIZE); >> + if (rc) { >> + dev_err(ts->dev, "%s: Failed to write command %d", >> + __func__, rc); >> + goto error; >> + } >> + >> + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), >> + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); >> + if (IS_TMO(t)) { > > I don't think this IS_TMO() macro makes sense. And you case use "rc" as > the return value. Yep, it is true. > >> + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", >> + __func__); >> + rc = -ETIME; >> + goto error; >> + } >> + >> + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); >> + goto exit; > > It's really weird for the "normal" case to jump to an "exit" label. Why > don't you handle the normal case here? Also, this code means that > you're currently ignoring the return value of > cyttsp5_validate_cmd_response(). Fixed for V2 > >> + >> +error: >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = 0; >> + mutex_unlock(&ts->system_lock); >> + return rc; >> +exit: >> + rc = cyttsp5_get_sysinfo_regs(ts); >> + >> + return rc; >> +} >> + >> +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) >> +{ >> + int rc, t; >> + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; >> + u16 crc; >> + >> + mutex_lock(&ts->system_lock); >> + ts->hid_cmd_state = HID_OUTPUT_BL_LAUNCH_APP + 1; >> + mutex_unlock(&ts->system_lock); >> + >> + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); >> + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); >> + cmd[2] = HID_BL_OUTPUT_REPORT_ID; >> + cmd[3] = 0x0; /* Reserved */ >> + cmd[4] = HID_OUTPUT_BL_SOP; >> + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; >> + cmd[6] = 0x0; /* Low bytes of data */ >> + cmd[7] = 0x0; /* Hi bytes of data */ >> + crc = cyttsp5_compute_crc(&cmd[4], 4); >> + cmd[8] = LOW_BYTE(crc); >> + cmd[9] = HI_BYTE(crc); >> + cmd[10] = HID_OUTPUT_BL_EOP; >> + >> + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, >> + HID_OUTPUT_BL_LAUNCH_APP_SIZE); >> + if (rc) { >> + dev_err(ts->dev, "%s: Failed to write command %d", >> + __func__, rc); >> + goto error; >> + } >> + >> + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), >> + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT)); >> + if (IS_TMO(t)) { >> + dev_err(ts->dev, "%s: HID output cmd execution timed out\n", >> + __func__); >> + rc = -ETIME; >> + goto error; >> + } >> + >> + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP); >> + goto exit; > > Same comments as for the previous function. > >> + /* Set HID descriptor register */ >> + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register)); >> + >> + regmap_write(ts->regmap, HID_DESC_REG, cmd[0]); > > Return value? > >> + rc = regmap_write(ts->regmap, HID_DESC_REG, cmd[1]); > > Why are you sending these through two regmap_write() calls and not a > regmap_bulk_write() ? You are right. I will even update the cyttysp5_write function to be more generic to use it in that case (ie send the register without any data). > >> + if (rc < 0) { >> + dev_err(dev, "%s: failed to get HID descriptor, rc=%d\n", >> + __func__, rc); >> + goto error; >> + } >> + >> + t = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == 0), >> + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT)); >> + if (IS_TMO(t)) { > > Same comment as above: don't use IS_TMO(), use "rc" instead of "t". ACK > >> + dev_err(ts->dev, "%s: HID get descriptor timed out\n", >> + __func__); >> + rc = -ETIME; >> + goto error; >> + } >> + >> + memcpy((u8 *)desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc)); > > I don't see why the u8* cast would be needed here. ACK > >> +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size, >> + int offset) >> +{ >> + tch_abs->ofs = offset / 8; >> + tch_abs->size = report_size / 8; >> + if (report_size % 8) >> + tch_abs->size += 1; >> + tch_abs->min = 0; >> + tch_abs->max = 1 << report_size; >> + tch_abs->bofs = offset - (tch_abs->ofs << 3); >> + >> + return 0; >> +} >> + >> +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) >> +{ >> + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE); >> + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE], >> + BTN_REPORT_SIZE); >> + >> + return 0; >> +} >> + >> +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) >> +{ >> + int max_tch = si->sensing_conf_data.max_tch; >> + int num_cur_tch; >> + int length; >> + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr; >> + >> + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE); >> + >> + cyttsp5_get_touch_axis(&num_cur_tch, tch->size, >> + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs); >> + if (unlikely(num_cur_tch > max_tch)) >> + num_cur_tch = max_tch; >> + >> + length = num_cur_tch * TOUCH_REPORT_SIZE; >> + >> + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length); > > Blank line here. > >> + return 0; >> +} >> + >> +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle) >> +{ >> + struct cyttsp5 *ts = handle; >> + int report_id; >> + int size; >> + int rc; >> + >> + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT); >> + if (!rc) { > > Same as usual: to remove one indentation level, invert the test, by > doing: > > if (rc) > return IRQ_HANDLED; > > ACK >> +static int cyttsp5_deassert_int(struct cyttsp5 *ts) >> +{ >> + u16 size; >> + u8 buf[2]; >> + int rc; >> + >> + rc = regmap_bulk_read(ts->regmap, 0, buf, 2); > > Which register are you reading here? In fact, for the read part, it is using an Input Report Register and it does not care about the register address. The only frame to send to perform a read is a frame with the I2C device address. It is possible to send anything as register address, it will still work. I guess that for some consistency, I will set it to the Input Register (0x3) but, for what I understood, it does not mean anything to the device. I will also add a comment to explain this specificity in the code. > >> + scnprintf(ts->phys, sizeof(ts->phys), "%s/input%d", dev_name(dev), 0); > > Why use %d if the value is always 0 ? > No explanation, it is a rest of the old driver :) I will update it. Thanks! Best regards, -- Mylène Josserand, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 2017-05-29 14:45 [PATCH 0/2] Input: Add Cypress Gen5 Touchscreen driver Mylène Josserand 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand @ 2017-05-29 14:45 ` Mylène Josserand 2017-06-07 20:26 ` Rob Herring 1 sibling, 1 reply; 18+ messages in thread From: Mylène Josserand @ 2017-05-29 14:45 UTC (permalink / raw) To: dmitry.torokhov, fery, robh+dt, mark.rutland Cc: linux-kernel, linux-input, devicetree, mylene.josserand, thomas.petazzoni, maxime.ripard Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings documentation. It can use I2C or SPI bus. This touchscreen can handle some defined zone that are designed and sent as button. To be able to customize the keycode sent, the "linux,code" property in a "button" sub-node can be used. Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> --- .../bindings/input/touchscreen/cyttsp5.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt diff --git a/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt new file mode 100644 index 000000000000..713a377b5039 --- /dev/null +++ b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt @@ -0,0 +1,55 @@ +* Cypress cyttsp touchscreen controller, generation 5 + +Required properties: + - compatible : must be "cypress,cyttsp5" + - reg : Device I2C address or SPI chip select number + - interrupt-parent : the phandle for the gpio controller + (see interrupt binding[0]). + - interrupts : (gpio) interrupt to which the chip is connected + (see interrupt binding[0]). + +Optional properties (many of them coming from touchscreen binding[1]): + - reset-gpios : the reset gpio the chip is connected to + (see GPIO binding[2] for more details). + - touchscreen-size-x : horizontal resolution of touchscreen (in pixels) + - touchscreen-size-y : vertical resolution of touchscreen (in pixels) + - touchscreen-fuzz-x : horizontal noise value of the absolute input device + (in pixels) + - touchscreen-fuzz-y : vertical noise value of the absolute input device + (in pixels) + +This touchscreen can handle some buttons that are touchscreen's defined zones. +Each button's event can be customized using a sub-node properties: + - linux,code: Keycode to emit. + +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt +[2]: Documentation/devicetree/bindings/gpio/gpio.txt + +Example: +&i2c0 { + [...] + + tsc@24 { + compatible = "cypress,cyttsp5"; + reg = <0x24>; + + pinctrl-names = "default"; + pinctrl-0 = <&tp_reset_ds203>; + interrupt-parent = <&pio>; + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>; + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; + + button@0 { + linux,code = <KEY_HOMEPAGE>; + }; + + button@1 { + linux,code = <KEY_MENU>; + }; + + button@2 { + linux,code = <KEY_BACK>; + }; + }; +}; -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 2017-05-29 14:45 ` [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 Mylène Josserand @ 2017-06-07 20:26 ` Rob Herring 2017-06-07 20:40 ` Dmitry Torokhov 2017-06-09 11:11 ` Mylene Josserand 0 siblings, 2 replies; 18+ messages in thread From: Rob Herring @ 2017-06-07 20:26 UTC (permalink / raw) To: Mylène Josserand Cc: dmitry.torokhov, fery, mark.rutland, linux-kernel, linux-input, devicetree, thomas.petazzoni, maxime.ripard On Mon, May 29, 2017 at 04:45:38PM +0200, Mylène Josserand wrote: > Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings > documentation. It can use I2C or SPI bus. > This touchscreen can handle some defined zone that are designed and > sent as button. To be able to customize the keycode sent, the > "linux,code" property in a "button" sub-node can be used. "documentation" twice in the subject makes for a long subject. The preferred subject prefix is "dt-bindings: input: ..." > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> > --- > .../bindings/input/touchscreen/cyttsp5.txt | 55 ++++++++++++++++++++++ cypress,cyttsp5.txt matching the compatible is preferred. > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt > new file mode 100644 > index 000000000000..713a377b5039 > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt > @@ -0,0 +1,55 @@ > +* Cypress cyttsp touchscreen controller, generation 5 > + > +Required properties: > + - compatible : must be "cypress,cyttsp5" > + - reg : Device I2C address or SPI chip select number > + - interrupt-parent : the phandle for the gpio controller > + (see interrupt binding[0]). > + - interrupts : (gpio) interrupt to which the chip is connected > + (see interrupt binding[0]). > + > +Optional properties (many of them coming from touchscreen binding[1]): > + - reset-gpios : the reset gpio the chip is connected to > + (see GPIO binding[2] for more details). > + - touchscreen-size-x : horizontal resolution of touchscreen (in pixels) Just "see ./touchscreen.txt" is enough description. > + - touchscreen-size-y : vertical resolution of touchscreen (in pixels) > + - touchscreen-fuzz-x : horizontal noise value of the absolute input device > + (in pixels) > + - touchscreen-fuzz-y : vertical noise value of the absolute input device > + (in pixels) > + > +This touchscreen can handle some buttons that are touchscreen's defined zones. > +Each button's event can be customized using a sub-node properties: > + - linux,code: Keycode to emit. > + > +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt > +[2]: Documentation/devicetree/bindings/gpio/gpio.txt > + > +Example: > +&i2c0 { > + [...] > + > + tsc@24 { touchscreen@24 > + compatible = "cypress,cyttsp5"; > + reg = <0x24>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&tp_reset_ds203>; > + interrupt-parent = <&pio>; > + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>; > + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; > + > + button@0 { unit addresses need a reg property. If 0,1,2 are meaningful numbers for the hardware, then it makes sense to add here. > + linux,code = <KEY_HOMEPAGE>; > + }; > + > + button@1 { > + linux,code = <KEY_MENU>; > + }; > + > + button@2 { > + linux,code = <KEY_BACK>; > + }; > + }; > +}; > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 2017-06-07 20:26 ` Rob Herring @ 2017-06-07 20:40 ` Dmitry Torokhov 2017-06-09 11:32 ` Mylene Josserand 2017-06-09 11:11 ` Mylene Josserand 1 sibling, 1 reply; 18+ messages in thread From: Dmitry Torokhov @ 2017-06-07 20:40 UTC (permalink / raw) To: Rob Herring Cc: Mylène Josserand, fery, mark.rutland, linux-kernel, linux-input, devicetree, thomas.petazzoni, maxime.ripard On Wed, Jun 07, 2017 at 03:26:03PM -0500, Rob Herring wrote: > On Mon, May 29, 2017 at 04:45:38PM +0200, Mylène Josserand wrote: > > Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings > > documentation. It can use I2C or SPI bus. > > This touchscreen can handle some defined zone that are designed and > > sent as button. To be able to customize the keycode sent, the > > "linux,code" property in a "button" sub-node can be used. > > "documentation" twice in the subject makes for a long subject. > The preferred subject prefix is "dt-bindings: input: ..." > > > > > Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> > > --- > > .../bindings/input/touchscreen/cyttsp5.txt | 55 ++++++++++++++++++++++ > > cypress,cyttsp5.txt matching the compatible is preferred. > > > 1 file changed, 55 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt > > new file mode 100644 > > index 000000000000..713a377b5039 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt > > @@ -0,0 +1,55 @@ > > +* Cypress cyttsp touchscreen controller, generation 5 > > + > > +Required properties: > > + - compatible : must be "cypress,cyttsp5" > > + - reg : Device I2C address or SPI chip select number > > + - interrupt-parent : the phandle for the gpio controller > > + (see interrupt binding[0]). > > + - interrupts : (gpio) interrupt to which the chip is connected > > + (see interrupt binding[0]). > > + > > +Optional properties (many of them coming from touchscreen binding[1]): > > + - reset-gpios : the reset gpio the chip is connected to > > + (see GPIO binding[2] for more details). > > + - touchscreen-size-x : horizontal resolution of touchscreen (in pixels) > > Just "see ./touchscreen.txt" is enough description. > > > + - touchscreen-size-y : vertical resolution of touchscreen (in pixels) > > + - touchscreen-fuzz-x : horizontal noise value of the absolute input device > > + (in pixels) > > + - touchscreen-fuzz-y : vertical noise value of the absolute input device > > + (in pixels) > > + > > +This touchscreen can handle some buttons that are touchscreen's defined zones. > > +Each button's event can be customized using a sub-node properties: > > + - linux,code: Keycode to emit. > > + > > +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > > +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt > > +[2]: Documentation/devicetree/bindings/gpio/gpio.txt > > + > > +Example: > > +&i2c0 { > > + [...] > > + > > + tsc@24 { > > touchscreen@24 > > > + compatible = "cypress,cyttsp5"; > > + reg = <0x24>; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&tp_reset_ds203>; > > + interrupt-parent = <&pio>; > > + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>; > > + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; > > + > > + button@0 { > > unit addresses need a reg property. If 0,1,2 are meaningful numbers for > the hardware, then it makes sense to add here. Another option would be just say: linux,keycodes = <KEY_HOMEPAGE>, <KEY_MENU>, <KEY_BACK>; I am wondering though: you read number of button supported by the device from HID_SYSINFO_BTN_OFFSET, can you also get button assignment form the device as well? And the biggest question of all: since you refer to HID descriptors in your driver, is it a HID device and should it be driven by HID susbystem instead of relying on a custom driver? > > > + linux,code = <KEY_HOMEPAGE>; > > + }; > > + > > + button@1 { > > + linux,code = <KEY_MENU>; > > + }; > > + > > + button@2 { > > + linux,code = <KEY_BACK>; > > + }; > > + }; > > +}; > > -- > > 2.11.0 > > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 2017-06-07 20:40 ` Dmitry Torokhov @ 2017-06-09 11:32 ` Mylene Josserand 0 siblings, 0 replies; 18+ messages in thread From: Mylene Josserand @ 2017-06-09 11:32 UTC (permalink / raw) To: Dmitry Torokhov, Rob Herring Cc: fery-+wT8y+m8/X5BDgjK7y7TUQ, mark.rutland-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-input-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8 Hi Dmitry, Thank you for the review! On 07/06/2017 22:40, Dmitry Torokhov wrote: > On Wed, Jun 07, 2017 at 03:26:03PM -0500, Rob Herring wrote: >> On Mon, May 29, 2017 at 04:45:38PM +0200, Mylène Josserand wrote: >>> Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings >>> documentation. It can use I2C or SPI bus. >>> This touchscreen can handle some defined zone that are designed and >>> sent as button. To be able to customize the keycode sent, the >>> "linux,code" property in a "button" sub-node can be used. >> >> "documentation" twice in the subject makes for a long subject. >> The preferred subject prefix is "dt-bindings: input: ..." >> >>> >>> Signed-off-by: Mylène Josserand <mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> >>> --- >>> .../bindings/input/touchscreen/cyttsp5.txt | 55 ++++++++++++++++++++++ >> >> cypress,cyttsp5.txt matching the compatible is preferred. >> >>> 1 file changed, 55 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt >>> >>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt >>> new file mode 100644 >>> index 000000000000..713a377b5039 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt >>> @@ -0,0 +1,55 @@ >>> +* Cypress cyttsp touchscreen controller, generation 5 >>> + >>> +Required properties: >>> + - compatible : must be "cypress,cyttsp5" >>> + - reg : Device I2C address or SPI chip select number >>> + - interrupt-parent : the phandle for the gpio controller >>> + (see interrupt binding[0]). >>> + - interrupts : (gpio) interrupt to which the chip is connected >>> + (see interrupt binding[0]). >>> + >>> +Optional properties (many of them coming from touchscreen binding[1]): >>> + - reset-gpios : the reset gpio the chip is connected to >>> + (see GPIO binding[2] for more details). >>> + - touchscreen-size-x : horizontal resolution of touchscreen (in pixels) >> >> Just "see ./touchscreen.txt" is enough description. >> >>> + - touchscreen-size-y : vertical resolution of touchscreen (in pixels) >>> + - touchscreen-fuzz-x : horizontal noise value of the absolute input device >>> + (in pixels) >>> + - touchscreen-fuzz-y : vertical noise value of the absolute input device >>> + (in pixels) >>> + >>> +This touchscreen can handle some buttons that are touchscreen's defined zones. >>> +Each button's event can be customized using a sub-node properties: >>> + - linux,code: Keycode to emit. >>> + >>> +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >>> +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt >>> +[2]: Documentation/devicetree/bindings/gpio/gpio.txt >>> + >>> +Example: >>> +&i2c0 { >>> + [...] >>> + >>> + tsc@24 { >> >> touchscreen@24 >> >>> + compatible = "cypress,cyttsp5"; >>> + reg = <0x24>; >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&tp_reset_ds203>; >>> + interrupt-parent = <&pio>; >>> + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>; >>> + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; >>> + >>> + button@0 { >> >> unit addresses need a reg property. If 0,1,2 are meaningful numbers for >> the hardware, then it makes sense to add here. > > Another option would be just say: > > linux,keycodes = <KEY_HOMEPAGE>, <KEY_MENU>, <KEY_BACK>; Sure. I noticed the "linux,code" property so I used that one but I did not see the "linux,keycodes". I guess it is possible to group all the keycodes using that property. > > I am wondering though: you read number of button supported by the device > from HID_SYSINFO_BTN_OFFSET, can you also get button assignment form > the device as well? For what I know, it is not possible. We can only retrieve the number of buttons configured for one device but not keycodes. > > And the biggest question of all: since you refer to HID descriptors in > your driver, is it a HID device and should it be driven by HID susbystem > instead of relying on a custom driver? I am not familiar with HID subsytem but I looked to other HID drivers and read the kernel documentation. In the datasheet, there is no reference to the HID specification and, for example, there is no GET/SET_REPORT requests. There is one "HID descriptor register" but that is all. I guess that it is not a HID device but do not hesitate to give me hints to look at it, if you want me to confirm it. > >> >>> + linux,code = <KEY_HOMEPAGE>; >>> + }; >>> + >>> + button@1 { >>> + linux,code = <KEY_MENU>; >>> + }; >>> + >>> + button@2 { >>> + linux,code = <KEY_BACK>; >>> + }; >>> + }; >>> +}; >>> -- >>> 2.11.0 >>> > > Thanks. > Best regards, -- Mylène Josserand, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 2017-06-07 20:26 ` Rob Herring 2017-06-07 20:40 ` Dmitry Torokhov @ 2017-06-09 11:11 ` Mylene Josserand 1 sibling, 0 replies; 18+ messages in thread From: Mylene Josserand @ 2017-06-09 11:11 UTC (permalink / raw) To: Rob Herring Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-input-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA, thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8 Hi Rob, Thank you for the review. On 07/06/2017 22:26, Rob Herring wrote: > On Mon, May 29, 2017 at 04:45:38PM +0200, Mylène Josserand wrote: >> Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings >> documentation. It can use I2C or SPI bus. >> This touchscreen can handle some defined zone that are designed and >> sent as button. To be able to customize the keycode sent, the >> "linux,code" property in a "button" sub-node can be used. > > "documentation" twice in the subject makes for a long subject. > The preferred subject prefix is "dt-bindings: input: ..." > Noted, thanks. >> >> Signed-off-by: Mylène Josserand <mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> >> --- >> .../bindings/input/touchscreen/cyttsp5.txt | 55 ++++++++++++++++++++++ > > cypress,cyttsp5.txt matching the compatible is preferred. ACK > >> 1 file changed, 55 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt >> >> diff --git a/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt >> new file mode 100644 >> index 000000000000..713a377b5039 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/input/touchscreen/cyttsp5.txt >> @@ -0,0 +1,55 @@ >> +* Cypress cyttsp touchscreen controller, generation 5 >> + >> +Required properties: >> + - compatible : must be "cypress,cyttsp5" >> + - reg : Device I2C address or SPI chip select number >> + - interrupt-parent : the phandle for the gpio controller >> + (see interrupt binding[0]). >> + - interrupts : (gpio) interrupt to which the chip is connected >> + (see interrupt binding[0]). >> + >> +Optional properties (many of them coming from touchscreen binding[1]): >> + - reset-gpios : the reset gpio the chip is connected to >> + (see GPIO binding[2] for more details). >> + - touchscreen-size-x : horizontal resolution of touchscreen (in pixels) > > Just "see ./touchscreen.txt" is enough description. Okay > >> + - touchscreen-size-y : vertical resolution of touchscreen (in pixels) >> + - touchscreen-fuzz-x : horizontal noise value of the absolute input device >> + (in pixels) >> + - touchscreen-fuzz-y : vertical noise value of the absolute input device >> + (in pixels) >> + >> +This touchscreen can handle some buttons that are touchscreen's defined zones. >> +Each button's event can be customized using a sub-node properties: >> + - linux,code: Keycode to emit. >> + >> +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> +[1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt >> +[2]: Documentation/devicetree/bindings/gpio/gpio.txt >> + >> +Example: >> +&i2c0 { >> + [...] >> + >> + tsc@24 { > > touchscreen@24 ACK > >> + compatible = "cypress,cyttsp5"; >> + reg = <0x24>; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&tp_reset_ds203>; >> + interrupt-parent = <&pio>; >> + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>; >> + reset-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; >> + >> + button@0 { > > unit addresses need a reg property. If 0,1,2 are meaningful numbers for > the hardware, then it makes sense to add here. No, 0,1,2 do not mean anything to the driver. > >> + linux,code = <KEY_HOMEPAGE>; >> + }; >> + >> + button@1 { >> + linux,code = <KEY_MENU>; >> + }; >> + >> + button@2 { >> + linux,code = <KEY_BACK>; >> + }; >> + }; >> +}; >> -- >> 2.11.0 >> Thanks! Best regards, -- Mylène Josserand, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/2] Input: Add driver for Cypress Generation 5 touchscreen @ 2017-12-01 15:39 Mylène Josserand 2017-12-03 14:29 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot 0 siblings, 1 reply; 18+ messages in thread From: Mylène Josserand @ 2017-12-01 15:39 UTC (permalink / raw) To: dmitry.torokhov, robh+dt, mark.rutland Cc: linux-kernel, linux-input, devicetree, mylene.josserand, thomas.petazzoni, maxime.ripard This is the basic driver for the Cypress TrueTouch Gen5 touchscreen controllers. This driver supports only the I2C bus but it uses regmap so SPI support could be added later. The touchscreen can retrieve some defined zone that are handled as buttons (according to the hardware). That is why it handles button and multitouch events. Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com> --- drivers/input/touchscreen/Kconfig | 16 + drivers/input/touchscreen/Makefile | 1 + drivers/input/touchscreen/cyttsp5.c | 1110 +++++++++++++++++++++++++++++++++++ 3 files changed, 1127 insertions(+) create mode 100644 drivers/input/touchscreen/cyttsp5.c diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index 0cfdb7cb610e..28eea6d5f1bb 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -238,6 +238,22 @@ config TOUCHSCREEN_CYTTSP4_SPI To compile this driver as a module, choose M here: the module will be called cyttsp4_spi. +config TOUCHSCREEN_CYTTSP5 + tristate "Cypress TrueTouch Gen5 Touchscreen Driver" + depends on OF + select REGMAP_I2C + select CRC_ITU_T + help + Driver for Parade TrueTouch Standard Product + Generation 5 touchscreen controllers. + I2C bus interface support only. + + Say Y here if you have a Cypress Gen5 touchscreen. + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called cyttsp5. + config TOUCHSCREEN_DA9034 tristate "Touchscreen support for Dialog Semiconductor DA9034" depends on PMIC_DA903X diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile index d2a2b3b7af27..e7d124901dd9 100644 --- a/drivers/input/touchscreen/Makefile +++ b/drivers/input/touchscreen/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_TOUCHSCREEN_CYTTSP_SPI) += cyttsp_spi.o obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_CORE) += cyttsp4_core.o obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_I2C) += cyttsp4_i2c.o cyttsp_i2c_common.o obj-$(CONFIG_TOUCHSCREEN_CYTTSP4_SPI) += cyttsp4_spi.o +obj-$(CONFIG_TOUCHSCREEN_CYTTSP5) += cyttsp5.o obj-$(CONFIG_TOUCHSCREEN_DA9034) += da9034-ts.o obj-$(CONFIG_TOUCHSCREEN_DA9052) += da9052_tsi.o obj-$(CONFIG_TOUCHSCREEN_DYNAPRO) += dynapro.o diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c new file mode 100644 index 000000000000..a41feea2cd5a --- /dev/null +++ b/drivers/input/touchscreen/cyttsp5.c @@ -0,0 +1,1110 @@ +/* + * Parade TrueTouch(TM) Standard Product V5 Module. + * For use with Parade touchscreen controllers. + * + * Copyright (C) 2015 Parade Technologies + * Copyright (C) 2012-2015 Cypress Semiconductor + * Copyright (C) 2017 Free Electrons + * + * Author: Mylène Josserand <mylene.josserand@free-electrons.com> + * + * 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. + * + */ + +#include <asm/unaligned.h> +#include <linux/crc-itu-t.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/gpio.h> +#include <linux/input/mt.h> +#include <linux/input/touchscreen.h> +#include <linux/interrupt.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/regmap.h> + +#define CYTTSP5_NAME "cyttsp5" +#define CY_I2C_DATA_SIZE (2 * 256) +#define HID_VERSION 0x0100 +#define CY_MAX_INPUT 512 +#define CYTTSP5_PREALLOCATED_CMD_BUFFER 32 +#define CY_BITS_PER_BTN 1 +#define CY_NUM_BTN_EVENT_ID ((1 << CY_BITS_PER_BTN) - 1) + +#define MAX_AREA 255 +#define HID_OUTPUT_BL_SOP 0x1 +#define HID_OUTPUT_BL_EOP 0x17 +#define HID_OUTPUT_BL_LAUNCH_APP 0x3B +#define HID_OUTPUT_BL_LAUNCH_APP_SIZE 11 +#define HID_OUTPUT_GET_SYSINFO 0x2 +#define HID_OUTPUT_GET_SYSINFO_SIZE 5 + +#define HID_DESC_REG 0x1 +#define HID_INPUT_REG 0x3 +#define HID_OUTPUT_REG 0x4 + +#define REPORT_ID_TOUCH 0x1 +#define REPORT_ID_BTN 0x3 +#define REPORT_SIZE_5 5 +#define REPORT_SIZE_8 8 +#define REPORT_SIZE_16 16 + +/* Touch reports offsets */ +/* Header offsets */ +#define TOUCH_REPORT_DESC_HDR_CONTACTCOUNT 16 +/* Record offsets */ +#define TOUCH_REPORT_DESC_CONTACTID 8 +#define TOUCH_REPORT_DESC_X 16 +#define TOUCH_REPORT_DESC_Y 32 +#define TOUCH_REPORT_DESC_P 48 +#define TOUCH_REPORT_DESC_MAJ 56 +#define TOUCH_REPORT_DESC_MIN 64 + +/* HID */ +#define HID_TOUCH_REPORT_ID 0x1 +#define HID_BTN_REPORT_ID 0x3 +#define HID_APP_RESPONSE_REPORT_ID 0x1F +#define HID_APP_OUTPUT_REPORT_ID 0x2F +#define HID_BL_RESPONSE_REPORT_ID 0x30 +#define HID_BL_OUTPUT_REPORT_ID 0x40 + +#define HID_OUTPUT_RESPONSE_REPORT_OFFSET 2 +#define HID_OUTPUT_RESPONSE_CMD_OFFSET 4 +#define HID_OUTPUT_RESPONSE_CMD_MASK 0x7F + +#define HID_SYSINFO_SENSING_OFFSET 33 +#define HID_SYSINFO_BTN_OFFSET 48 +#define HID_SYSINFO_BTN_MASK 0xFF +#define HID_SYSINFO_MAX_BTN 8 + +/* Timeout in ms */ +#define CY_HID_OUTPUT_TIMEOUT 200 +#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT 3000 +#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT 4000 + +/* maximum number of concurrent tracks */ +#define TOUCH_REPORT_SIZE 10 +#define TOUCH_INPUT_HEADER_SIZE 7 +#define BTN_REPORT_SIZE 9 +#define BTN_INPUT_HEADER_SIZE 5 + +/* All usage pages for Touch Report */ +#define TOUCH_REPORT_USAGE_PG_X 0x00010030 +#define TOUCH_REPORT_USAGE_PG_Y 0x00010031 +#define TOUCH_REPORT_USAGE_PG_P 0x000D0030 +#define TOUCH_REPORT_USAGE_PG_CONTACTID 0x000D0051 +#define TOUCH_REPORT_USAGE_PG_CONTACTCOUNT 0x000D0054 +#define TOUCH_REPORT_USAGE_PG_MAJ 0xFF010062 +#define TOUCH_REPORT_USAGE_PG_MIN 0xFF010063 +#define TOUCH_COL_USAGE_PG 0x000D0022 + +/* helpers */ +#define HI_BYTE(x) (u8)(((x) >> 8) & 0xFF) +#define LOW_BYTE(x) (u8)((x) & 0xFF) + +/* System Information interface definitions */ +struct cyttsp5_sensing_conf_data_dev { + u8 electrodes_x; + u8 electrodes_y; + __le16 len_x; + __le16 len_y; + __le16 res_x; + __le16 res_y; + __le16 max_z; + u8 origin_x; + u8 origin_y; + u8 btn; + u8 scan_mode; + u8 max_num_of_tch_per_refresh_cycle; +} __packed; + +struct cyttsp5_sensing_conf_data { + u16 res_x; + u16 res_y; + u16 max_z; + u16 len_x; + u16 len_y; + u8 origin_x; + u8 origin_y; + u8 max_tch; +}; + +enum { HID_CMD_DONE, HID_CMD_BUSY } hid_cmd_state; + +enum cyttsp5_tch_abs { /* for ordering within the extracted touch data array */ + CY_TCH_X, /* X */ + CY_TCH_Y, /* Y */ + CY_TCH_P, /* P (Z) */ + CY_TCH_T, /* TOUCH ID */ + CY_TCH_MAJ, /* TOUCH_MAJOR */ + CY_TCH_MIN, /* TOUCH_MINOR */ + CY_TCH_NUM_ABS, +}; + +struct cyttsp5_tch_abs_params { + size_t ofs; /* abs byte offset */ + size_t size; /* size in bits */ + size_t min; /* min value */ + size_t max; /* max value */ + size_t bofs; /* bit offset */ +}; + +struct cyttsp5_touch { + int hdr; + int abs[CY_TCH_NUM_ABS]; +}; + +struct cyttsp5_sysinfo { + struct cyttsp5_sensing_conf_data sensing_conf_data; + int num_btns; + struct cyttsp5_tch_abs_params tch_hdr; + struct cyttsp5_tch_abs_params tch_abs[CY_TCH_NUM_ABS]; + u32 key_code[HID_SYSINFO_MAX_BTN]; + u8 *xy_mode; + u8 *xy_data; +}; + +struct cyttsp5_hid_desc { + __le16 hid_desc_len; + u8 packet_id; + u8 reserved_byte; + __le16 bcd_version; + __le16 report_desc_len; + __le16 report_desc_register; + __le16 input_register; + __le16 max_input_len; + __le16 output_register; + __le16 max_output_len; + __le16 command_register; + __le16 data_register; + __le16 vendor_id; + __le16 product_id; + __le16 version_id; + u8 reserved[4]; +} __packed; + +struct cyttsp5 { + struct device *dev; + struct mutex system_lock; + wait_queue_head_t wait_q; + struct cyttsp5_sysinfo sysinfo; + int hid_cmd_state; + struct cyttsp5_hid_desc hid_desc; + u8 cmd_buf[CYTTSP5_PREALLOCATED_CMD_BUFFER]; + u8 input_buf[CY_MAX_INPUT]; + u8 response_buf[CY_MAX_INPUT]; + struct gpio_desc *reset_gpio; + struct input_dev *input; + char phys[NAME_MAX]; + int num_prv_rec; + struct regmap *regmap; +}; + +/* + * For what understood in the datasheet, the register does not + * matter. For consistency, used the Input Register address + * but it does mean anything to the device. The important data + * to send is the I2C address + */ +static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max) +{ + int rc; + u32 size; + u8 temp[2]; + + if (!buf) + return -EINVAL; + + /* Read the frame to retrieve the size */ + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, 2); + if (rc < 0) + return rc; + + size = get_unaligned_le16(temp); + if (!size || size == 2) + return 0; + + if (size > max) + return -EINVAL; + + /* Get the real value */ + return regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, size); +} + +static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data, + size_t size) +{ + u8 cmd[size + 1]; + + /* High bytes of register address needed as first byte of cmd */ + cmd[0] = HI_BYTE(reg); + + /* Copy the rest of the data */ + if (data) + memcpy(&cmd[1], data, size); + + /* The hardware wants to receive a frame with the address register + * contains in the first two bytes. As the regmap_write function + * add the register adresse in the frame, we use the LOW_BYTE as + * first frame byte for the address register and the first + * data byte is the high register + left of the cmd to send + */ + return regmap_bulk_write(ts->regmap, LOW_BYTE(reg), cmd, size + 1); +} + +static void cyttsp5_final_sync(struct input_dev *input, int max_slots, + unsigned long *ids) +{ + int t; + + for (t = 0; t < max_slots; t++) { + if (test_bit(t, ids)) + continue; + input_mt_slot(input, t); + input_mt_report_slot_state(input, MT_TOOL_FINGER, false); + } + + input_sync(input); +} + +static void cyttsp5_report_slot_liftoff(struct cyttsp5 *ts, int max_slots) +{ + int t; + + if (ts->num_prv_rec == 0) + return; + + for (t = 0; t < max_slots; t++) { + input_mt_slot(ts->input, t); + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, false); + } +} + +static void cyttsp5_mt_lift_all(struct cyttsp5 *ts) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int max = si->tch_abs[CY_TCH_T].max; + + if (ts->num_prv_rec != 0) { + cyttsp5_report_slot_liftoff(ts, max); + input_sync(ts->input); + ts->num_prv_rec = 0; + } +} + +static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data, + int bofs) +{ + int nbyte; + int next; + + for (nbyte = 0, *axis = 0, next = 0; nbyte < size; nbyte++) + *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8)); + + *axis &= max - 1; +} + +static void cyttsp5_get_touch_record(struct cyttsp5 *ts, + struct cyttsp5_touch *touch, u8 *xy_data) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + enum cyttsp5_tch_abs abs; + + for (abs = CY_TCH_X; abs < CY_TCH_NUM_ABS; abs++) { + cyttsp5_get_touch_axis(&touch->abs[abs], + si->tch_abs[abs].size, + si->tch_abs[abs].max, + xy_data + si->tch_abs[abs].ofs, + si->tch_abs[abs].bofs); + } +} + +static void cyttsp5_get_mt_touches(struct cyttsp5 *ts, + struct cyttsp5_touch *tch, int num_cur_tch) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int i, t = 0; + DECLARE_BITMAP(ids, si->tch_abs[CY_TCH_T].max); + u8 *tch_addr; + int tmp; + + bitmap_zero(ids, si->tch_abs[CY_TCH_T].max); + memset(tch->abs, 0, sizeof(tch->abs)); + + for (i = 0; i < num_cur_tch; i++) { + tch_addr = si->xy_data + (i * TOUCH_REPORT_SIZE); + cyttsp5_get_touch_record(ts, tch, tch_addr); + + /* Convert MAJOR/MINOR from mm to resolution */ + tmp = tch->abs[CY_TCH_MAJ] * 100 * si->sensing_conf_data.res_x; + tch->abs[CY_TCH_MAJ] = tmp / si->sensing_conf_data.len_x; + tmp = tch->abs[CY_TCH_MIN] * 100 * si->sensing_conf_data.res_x; + tch->abs[CY_TCH_MIN] = tmp / si->sensing_conf_data.len_x; + + t = tch->abs[CY_TCH_T]; + input_mt_slot(ts->input, t); + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER, true); + __set_bit(t, ids); + + /* position and pressure fields */ + input_report_abs(ts->input, ABS_MT_POSITION_X, + tch->abs[CY_TCH_X]); + input_report_abs(ts->input, ABS_MT_POSITION_Y, + tch->abs[CY_TCH_Y]); + input_report_abs(ts->input, ABS_MT_PRESSURE, + tch->abs[CY_TCH_P]); + + /* Get the extended touch fields */ + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR, + tch->abs[CY_TCH_MAJ]); + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR, + tch->abs[CY_TCH_MIN]); + } + + cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids); + + ts->num_prv_rec = num_cur_tch; +} + +/* read xy_data for all current touches */ +static int cyttsp5_xy_worker(struct cyttsp5 *ts) +{ + struct device *dev = ts->dev; + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int max_tch = si->sensing_conf_data.max_tch; + struct cyttsp5_touch tch; + u8 num_cur_tch; + + cyttsp5_get_touch_axis(&tch.hdr, si->tch_hdr.size, + si->tch_hdr.max, + si->xy_mode + 3 + si->tch_hdr.ofs, + si->tch_hdr.bofs); + + num_cur_tch = tch.hdr; + if (num_cur_tch > max_tch) { + dev_err(dev, "Num touch err detected (n=%d)\n", num_cur_tch); + num_cur_tch = max_tch; + } + + if (num_cur_tch == 0 && ts->num_prv_rec == 0) + return 0; + + /* extract xy_data for all currently reported touches */ + if (num_cur_tch) + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch); + else + cyttsp5_mt_lift_all(ts); + + return 0; +} + +static int cyttsp5_mt_attention(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int rc; + + if (si->xy_mode[2] != HID_TOUCH_REPORT_ID) + return 0; + + /* core handles handshake */ + rc = cyttsp5_xy_worker(ts); + if (rc < 0) + dev_err(dev, "xy_worker error r=%d\n", rc); + + return rc; +} + +static int cyttsp5_setup_input_device(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int max_x, max_y, max_p; + int max_x_tmp, max_y_tmp; + int rc; + + __set_bit(EV_ABS, ts->input->evbit); + __set_bit(EV_REL, ts->input->evbit); + __set_bit(EV_KEY, ts->input->evbit); + + max_x_tmp = si->sensing_conf_data.res_x; + max_y_tmp = si->sensing_conf_data.res_y; + max_x = max_y_tmp - 1; + max_y = max_x_tmp - 1; + max_p = si->sensing_conf_data.max_z; + + input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0); + + __set_bit(ABS_MT_POSITION_X, ts->input->absbit); + __set_bit(ABS_MT_POSITION_Y, ts->input->absbit); + __set_bit(ABS_MT_PRESSURE, ts->input->absbit); + + input_set_abs_params(ts->input, ABS_MT_POSITION_X, 0, max_x, 0, 0); + input_set_abs_params(ts->input, ABS_MT_POSITION_Y, 0, max_y, 0, 0); + input_set_abs_params(ts->input, ABS_MT_PRESSURE, 0, max_p, 0, 0); + + input_set_abs_params(ts->input, ABS_MT_TOUCH_MAJOR, 0, MAX_AREA, 0, 0); + input_set_abs_params(ts->input, ABS_MT_TOUCH_MINOR, 0, MAX_AREA, 0, 0); + + rc = input_register_device(ts->input); + if (rc < 0) + dev_err(dev, "Error, failed register input device r=%d\n", rc); + + return rc; +} + +#ifdef CONFIG_OF +static int cyttsp5_parse_dt_key_code(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + struct device_node *np; + int i; + + np = dev->of_node; + if (!np) + return -EINVAL; + + if (!si->num_btns) + return 0; + + /* Initialize the button to RESERVED */ + for (i = 0; i < si->num_btns; i++) + si->key_code[i] = KEY_RESERVED; + + return of_property_read_u32_array(np, "linux,keycodes", + si->key_code, si->num_btns); +} +#else +static int cyttsp5_parse_dt_key_code(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int i; + + if (!si->num_btns) + return 0; + + /* Initialize the button to RESERVED */ + for (i = 0; i < si->num_btns; i++) + si->key_code[i] = KEY_RESERVED; + + return 0; +} +#endif + +static int cyttsp5_btn_attention(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int cur_btn; + int cur_btn_state; + + if (si->xy_mode[2] != HID_BTN_REPORT_ID || !si->num_btns) + return 0; + + /* extract button press/release touch information */ + for (cur_btn = 0; cur_btn < si->num_btns; cur_btn++) { + /* Get current button state */ + cur_btn_state = (si->xy_data[0] >> (cur_btn * CY_BITS_PER_BTN)) + & CY_NUM_BTN_EVENT_ID; + + input_report_key(ts->input, si->key_code[cur_btn], + cur_btn_state); + input_sync(ts->input); + } + + return 0; +} + +static u16 cyttsp5_compute_crc(u8 *buf, u32 size) +{ + u16 remainder = 0xFFFF; + u16 xor_mask = 0x0000; + u32 index; + u32 byte_value; + u32 table_index; + u32 crc_bit_width = sizeof(u16) * 8; + + /* Divide the message by polynomial, via the table. */ + for (index = 0; index < size; index++) { + byte_value = buf[index]; + table_index = ((byte_value >> 4) & 0x0F) + ^ (remainder >> (crc_bit_width - 4)); + remainder = crc_itu_t_table[table_index] + ^ (remainder << 4); + table_index = (byte_value & 0x0F) + ^ (remainder >> (crc_bit_width - 4)); + remainder = crc_itu_t_table[table_index] + ^ (remainder << 4); + } + + /* Perform the final remainder CRC. */ + return remainder ^ xor_mask; +} + +static int cyttsp5_validate_cmd_response(struct cyttsp5 *ts, u8 code) +{ + u16 size, crc; + u8 status, offset; + int command_code; + + size = get_unaligned_le16(&ts->response_buf[0]); + + if (!size) + return 0; + + offset = ts->response_buf[HID_OUTPUT_RESPONSE_REPORT_OFFSET]; + + if (offset == HID_BL_RESPONSE_REPORT_ID) { + if (ts->response_buf[4] != HID_OUTPUT_BL_SOP) { + dev_err(ts->dev, "HID output response, wrong SOP\n"); + return -EPROTO; + } + + if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) { + dev_err(ts->dev, "HID output response, wrong EOP\n"); + return -EPROTO; + } + + crc = cyttsp5_compute_crc(&ts->response_buf[4], size - 7); + if (ts->response_buf[size - 3] != LOW_BYTE(crc) || + ts->response_buf[size - 2] != HI_BYTE(crc)) { + dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n", + crc); + return -EPROTO; + } + + status = ts->response_buf[5]; + if (status) { + dev_err(ts->dev, "HID output response, ERROR:%d\n", + status); + return -EPROTO; + } + } + + if (offset == HID_APP_RESPONSE_REPORT_ID) { + command_code = ts->response_buf[HID_OUTPUT_RESPONSE_CMD_OFFSET] + & HID_OUTPUT_RESPONSE_CMD_MASK; + if (command_code != code) { + dev_err(ts->dev, + "HID output response, wrong command_code:%X\n", + command_code); + return -EPROTO; + } + } + + return 0; +} + +static void cyttsp5_si_get_btn_data(struct cyttsp5 *ts) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + int i; + unsigned int btns = ts->response_buf[HID_SYSINFO_BTN_OFFSET] + & HID_SYSINFO_BTN_MASK; + + si->num_btns = 0; + for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) { + if (btns & BIT(i)) + si->num_btns++; + } +} + +static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts) +{ + struct cyttsp5_sensing_conf_data *scd = &ts->sysinfo.sensing_conf_data; + struct cyttsp5_sensing_conf_data_dev *scd_dev = + (struct cyttsp5_sensing_conf_data_dev *) + &ts->response_buf[HID_SYSINFO_SENSING_OFFSET]; + struct cyttsp5_sysinfo *si = &ts->sysinfo; + + cyttsp5_si_get_btn_data(ts); + + scd->max_tch = scd_dev->max_num_of_tch_per_refresh_cycle; + scd->res_x = get_unaligned_le16(&scd_dev->res_x); + scd->res_y = get_unaligned_le16(&scd_dev->res_y); + scd->max_z = get_unaligned_le16(&scd_dev->max_z); + scd->len_x = get_unaligned_le16(&scd_dev->len_x); + scd->len_y = get_unaligned_le16(&scd_dev->len_y); + + si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE, + GFP_KERNEL); + if (!si->xy_data) + return -ENOMEM; + + si->xy_mode = devm_kzalloc(ts->dev, TOUCH_INPUT_HEADER_SIZE, + GFP_KERNEL); + if (!si->xy_mode) + return -ENOMEM; + + return 0; +} + +static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts) +{ + int rc; + u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE]; + + ts->hid_cmd_state = HID_CMD_BUSY; + + /* HI bytes of Output register address */ + cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); + cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE); + cmd[2] = HID_APP_OUTPUT_REPORT_ID; + cmd[3] = 0x0; /* Reserved */ + cmd[4] = HID_OUTPUT_GET_SYSINFO; + + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, + HID_OUTPUT_GET_SYSINFO_SIZE); + if (rc) { + dev_err(ts->dev, "Failed to write command %d", rc); + goto error; + } + + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE), + msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT)); + if (!rc) { + dev_err(ts->dev, "HID output cmd execution timed out\n"); + rc = -ETIME; + goto error; + } + + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_GET_SYSINFO); + if (rc) { + dev_err(ts->dev, "Validation of the response failed\n"); + goto error; + } + + return cyttsp5_get_sysinfo_regs(ts); + +error: + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_DONE; + mutex_unlock(&ts->system_lock); + return rc; +} + +static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts) +{ + int rc; + u8 cmd[HID_OUTPUT_BL_LAUNCH_APP]; + u16 crc; + + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_BUSY; + mutex_unlock(&ts->system_lock); + + cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); + cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE); + cmd[2] = HID_BL_OUTPUT_REPORT_ID; + cmd[3] = 0x0; /* Reserved */ + cmd[4] = HID_OUTPUT_BL_SOP; + cmd[5] = HID_OUTPUT_BL_LAUNCH_APP; + cmd[6] = 0x0; /* Low bytes of data */ + cmd[7] = 0x0; /* Hi bytes of data */ + crc = cyttsp5_compute_crc(&cmd[4], 4); + cmd[8] = LOW_BYTE(crc); + cmd[9] = HI_BYTE(crc); + cmd[10] = HID_OUTPUT_BL_EOP; + + rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd, + HID_OUTPUT_BL_LAUNCH_APP_SIZE); + if (rc) { + dev_err(ts->dev, "Failed to write command %d", rc); + goto error; + } + + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE), + msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT)); + if (!rc) { + dev_err(ts->dev, "HID output cmd execution timed out\n"); + rc = -ETIME; + goto error; + } + + rc = cyttsp5_validate_cmd_response(ts, HID_OUTPUT_BL_LAUNCH_APP); + if (rc) { + dev_err(ts->dev, "Validation of the response failed\n"); + goto error; + } + + return rc; + +error: + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_DONE; + mutex_unlock(&ts->system_lock); + + return rc; +} + +static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts, + struct cyttsp5_hid_desc *desc) +{ + struct device *dev = ts->dev; + __le16 hid_desc_register = HID_DESC_REG; + int rc; + u8 cmd[2]; + + /* Read HID descriptor length and version */ + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_BUSY; + mutex_unlock(&ts->system_lock); + + /* Set HID descriptor register */ + memcpy(cmd, &hid_desc_register, sizeof(hid_desc_register)); + + rc = cyttsp5_write(ts, HID_DESC_REG, NULL, 0); + if (rc < 0) { + dev_err(dev, "Failed to get HID descriptor, rc=%d\n", rc); + goto error; + } + + rc = wait_event_timeout(ts->wait_q, (ts->hid_cmd_state == HID_CMD_DONE), + msecs_to_jiffies(CY_HID_GET_HID_DESCRIPTOR_TIMEOUT)); + if (!rc) { + dev_err(ts->dev, "HID get descriptor timed out\n"); + rc = -ETIME; + goto error; + } + + memcpy(desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc)); + + /* Check HID descriptor length and version */ + if (le16_to_cpu(desc->hid_desc_len) != sizeof(*desc) || + le16_to_cpu(desc->bcd_version) != HID_VERSION) { + dev_err(dev, "Unsupported HID version\n"); + return -ENODEV; + } + + goto exit; + +error: + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_DONE; + mutex_unlock(&ts->system_lock); +exit: + return rc; +} + +static int fill_tch_abs(struct cyttsp5_tch_abs_params *tch_abs, int report_size, + int offset) +{ + tch_abs->ofs = offset / 8; + tch_abs->size = report_size / 8; + if (report_size % 8) + tch_abs->size += 1; + tch_abs->min = 0; + tch_abs->max = 1 << report_size; + tch_abs->bofs = offset - (tch_abs->ofs << 3); + + return 0; +} + +static int move_button_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) +{ + memcpy(si->xy_mode, ts->input_buf, BTN_INPUT_HEADER_SIZE); + memcpy(si->xy_data, &ts->input_buf[BTN_INPUT_HEADER_SIZE], + BTN_REPORT_SIZE); + + return 0; +} + +static int move_touch_data(struct cyttsp5 *ts, struct cyttsp5_sysinfo *si) +{ + int max_tch = si->sensing_conf_data.max_tch; + int num_cur_tch; + int length; + struct cyttsp5_tch_abs_params *tch = &si->tch_hdr; + + memcpy(si->xy_mode, ts->input_buf, TOUCH_INPUT_HEADER_SIZE); + + cyttsp5_get_touch_axis(&num_cur_tch, tch->size, + tch->max, si->xy_mode + 3 + tch->ofs, tch->bofs); + if (unlikely(num_cur_tch > max_tch)) + num_cur_tch = max_tch; + + length = num_cur_tch * TOUCH_REPORT_SIZE; + + memcpy(si->xy_data, &ts->input_buf[TOUCH_INPUT_HEADER_SIZE], length); + + return 0; +} + +static irqreturn_t cyttsp5_handle_irq(int irq, void *handle) +{ + struct cyttsp5 *ts = handle; + int report_id; + int size; + int rc; + + rc = cyttsp5_read(ts, ts->input_buf, CY_MAX_INPUT); + if (rc) + return IRQ_HANDLED; + + size = get_unaligned_le16(&ts->input_buf[0]); + + /* check reset */ + if (size == 0) { + memcpy(ts->response_buf, ts->input_buf, 2); + + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_DONE; + mutex_unlock(&ts->system_lock); + wake_up(&ts->wait_q); + return IRQ_HANDLED; + } + + report_id = ts->input_buf[2]; + + if (report_id == HID_TOUCH_REPORT_ID) { + move_touch_data(ts, &ts->sysinfo); + cyttsp5_mt_attention(ts->dev); + } else if (report_id == HID_BTN_REPORT_ID) { + move_button_data(ts, &ts->sysinfo); + cyttsp5_btn_attention(ts->dev); + } else { + /* It is not an input but a command response */ + memcpy(ts->response_buf, ts->input_buf, size); + + mutex_lock(&ts->system_lock); + ts->hid_cmd_state = HID_CMD_DONE; + mutex_unlock(&ts->system_lock); + wake_up(&ts->wait_q); + } + + return IRQ_HANDLED; +} + +static int cyttsp5_deassert_int(struct cyttsp5 *ts) +{ + u16 size; + u8 buf[2]; + int rc; + + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2); + if (rc < 0) + return rc; + + size = get_unaligned_le16(&buf[0]); + if (size == 2 || size == 0) + return 0; + + return -EINVAL; +} + +static int cyttsp5_fill_all_touch(struct cyttsp5 *ts) +{ + struct cyttsp5_sysinfo *si = &ts->sysinfo; + + fill_tch_abs(&si->tch_abs[CY_TCH_X], REPORT_SIZE_16, + TOUCH_REPORT_DESC_X); + fill_tch_abs(&si->tch_abs[CY_TCH_Y], REPORT_SIZE_16, + TOUCH_REPORT_DESC_Y); + fill_tch_abs(&si->tch_abs[CY_TCH_P], REPORT_SIZE_8, + TOUCH_REPORT_DESC_P); + fill_tch_abs(&si->tch_abs[CY_TCH_T], REPORT_SIZE_5, + TOUCH_REPORT_DESC_CONTACTID); + fill_tch_abs(&si->tch_hdr, REPORT_SIZE_5, + TOUCH_REPORT_DESC_HDR_CONTACTCOUNT); + fill_tch_abs(&si->tch_abs[CY_TCH_MAJ], REPORT_SIZE_8, + TOUCH_REPORT_DESC_MAJ); + fill_tch_abs(&si->tch_abs[CY_TCH_MIN], REPORT_SIZE_8, + TOUCH_REPORT_DESC_MIN); + + return 0; +} + +static int cyttsp5_startup(struct cyttsp5 *ts) +{ + int rc; + + rc = cyttsp5_deassert_int(ts); + if (rc) { + dev_err(ts->dev, "Error on deassert int r=%d\n", rc); + return -ENODEV; + } + + /* + * Launch the application as the device starts in bootloader mode + * because of a power-on-reset + */ + rc = cyttsp5_hid_output_bl_launch_app(ts); + if (rc < 0) { + dev_err(ts->dev, "Error on launch app r=%d\n", rc); + return rc; + } + + rc = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc); + if (rc < 0) { + dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", rc); + return rc; + } + + rc = cyttsp5_fill_all_touch(ts); + if (rc < 0) { + dev_err(ts->dev, "Error on report descriptor r=%d\n", rc); + return rc; + } + + rc = cyttsp5_hid_output_get_sysinfo(ts); + if (rc) { + dev_err(ts->dev, "Error on getting sysinfo r=%d\n", rc); + return rc; + } + + return rc; +} + +#ifdef CONFIG_OF +static const struct of_device_id cyttsp5_of_match[] = { + { .compatible = "cypress,cyttsp5", }, + { } +}; +MODULE_DEVICE_TABLE(of, cyttsp5_of_match); +#endif + +static const struct i2c_device_id cyttsp5_i2c_id[] = { + { CYTTSP5_NAME, 0, }, + { } +}; +MODULE_DEVICE_TABLE(i2c, cyttsp5_i2c_id); + +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq, + const char *name) +{ + struct cyttsp5 *ts; + struct cyttsp5_sysinfo *si; + int rc = 0, i; + + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); + if (!ts) + return -ENOMEM; + + /* Initialize device info */ + ts->regmap = regmap; + ts->dev = dev; + si = &ts->sysinfo; + dev_set_drvdata(dev, ts); + + /* Initialize mutexes and spinlocks */ + mutex_init(&ts->system_lock); + + /* Initialize wait queue */ + init_waitqueue_head(&ts->wait_q); + + /* Reset the gpio to be in a reset state */ + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(ts->reset_gpio)) { + rc = PTR_ERR(ts->reset_gpio); + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); + return rc; + } + gpiod_set_value(ts->reset_gpio, 1); + + /* Need a delay to have device up */ + msleep(20); + + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + name, ts); + if (rc) { + dev_err(dev, "unable to request IRQ\n"); + return rc; + } + + rc = cyttsp5_startup(ts); + if (rc) { + dev_err(ts->dev, "Fail initial startup r=%d\n", rc); + return rc; + } + + rc = cyttsp5_parse_dt_key_code(dev); + if (rc < 0) { + dev_err(ts->dev, "Error while parsing dts %d\n", rc); + return rc; + } + + ts->input = devm_input_allocate_device(dev); + if (!ts->input) { + dev_err(dev, "Error, failed to allocate input device\n"); + return -ENODEV; + } + + ts->input->name = "cyttsp5"; + scnprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev)); + ts->input->phys = ts->phys; + ts->input->dev.parent = ts->dev; + input_set_drvdata(ts->input, ts); + + touchscreen_parse_properties(ts->input, true, NULL); + + __set_bit(EV_KEY, ts->input->evbit); + for (i = 0; i < si->num_btns; i++) + __set_bit(si->key_code[i], ts->input->keybit); + + return cyttsp5_setup_input_device(dev); +} + +static int cyttsp5_i2c_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct regmap *regmap; + static const struct regmap_config config = { + .reg_bits = 8, + .val_bits = 8, + }; + + regmap = devm_regmap_init_i2c(client, &config); + if (IS_ERR(regmap)) { + dev_err(&client->dev, "regmap allocation failed: %ld\n", + PTR_ERR(regmap)); + return PTR_ERR(regmap); + } + + return cyttsp5_probe(&client->dev, regmap, client->irq, client->name); +} + +static int cyttsp5_remove(struct device *dev) +{ + struct cyttsp5 *ts = dev_get_drvdata(dev); + + input_unregister_device(ts->input); + + return 0; +} + +static int cyttsp5_i2c_remove(struct i2c_client *client) +{ + struct device *dev = &client->dev; + + return cyttsp5_remove(dev); +} + +static struct i2c_driver cyttsp5_i2c_driver = { + .driver = { + .name = CYTTSP5_NAME, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(cyttsp5_of_match), + }, + .probe = cyttsp5_i2c_probe, + .remove = cyttsp5_i2c_remove, + .id_table = cyttsp5_i2c_id, +}; + +module_i2c_driver(cyttsp5_i2c_driver); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Touchscreen driver for Cypress TrueTouch Gen 5 Product"); +MODULE_AUTHOR("Mylène Josserand <mylene.josserand@free-electrons.com>"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] Input: fix platform_no_drv_owner.cocci warnings 2017-12-01 15:39 [PATCH v4 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand @ 2017-12-03 14:29 ` kbuild test robot 0 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2017-12-03 14:29 UTC (permalink / raw) Cc: kbuild-all, dmitry.torokhov, robh+dt, mark.rutland, linux-kernel, linux-input, devicetree, mylene.josserand, thomas.petazzoni, maxime.ripard drivers/input/touchscreen/cyttsp5.c:1098:3-8: No need to set .owner here. The core will do it. Remove .owner field if calls are used which set it automatically Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci Fixes: 68d0bd1e4815 ("Input: Add driver for Cypress Generation 5 touchscreen") CC: Mylène Josserand <mylene.josserand@free-electrons.com> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- cyttsp5.c | 1 - 1 file changed, 1 deletion(-) --- a/drivers/input/touchscreen/cyttsp5.c +++ b/drivers/input/touchscreen/cyttsp5.c @@ -1095,7 +1095,6 @@ static int cyttsp5_i2c_remove(struct i2c static struct i2c_driver cyttsp5_i2c_driver = { .driver = { .name = CYTTSP5_NAME, - .owner = THIS_MODULE, .of_match_table = of_match_ptr(cyttsp5_of_match), }, .probe = cyttsp5_i2c_probe, ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-12-03 14:29 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-29 14:45 [PATCH 0/2] Input: Add Cypress Gen5 Touchscreen driver Mylène Josserand 2017-05-29 14:45 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand 2017-05-29 17:16 ` kbuild test robot 2017-05-29 17:16 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot 2017-05-30 8:02 ` [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Maxime Ripard 2017-06-06 8:03 ` Mylene Josserand 2017-06-06 12:04 ` Maxime Ripard 2017-06-06 14:32 ` Mylene Josserand [not found] ` <0232a641-c741-1f95-9086-840b31f5d055-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-07 9:16 ` Maxime Ripard 2017-05-30 8:11 ` Thomas Petazzoni [not found] ` <20170529144538.29187-2-mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-05-30 8:43 ` Thomas Petazzoni [not found] ` <20170530104302.6db658f0-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2017-06-06 13:20 ` Mylene Josserand 2017-05-29 14:45 ` [PATCH 2/2] Documentation: DT: bindings: input: Add documentation for cyttsp5 Mylène Josserand 2017-06-07 20:26 ` Rob Herring 2017-06-07 20:40 ` Dmitry Torokhov 2017-06-09 11:32 ` Mylene Josserand 2017-06-09 11:11 ` Mylene Josserand 2017-12-01 15:39 [PATCH v4 1/2] Input: Add driver for Cypress Generation 5 touchscreen Mylène Josserand 2017-12-03 14:29 ` [PATCH] Input: fix platform_no_drv_owner.cocci warnings kbuild test robot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).