From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mylene Josserand Subject: Re: [PATCH 1/2] Input: Add driver for Cypress Generation 5 touchscreen Date: Tue, 6 Jun 2017 10:03:48 +0200 Message-ID: References: <20170529144538.29187-1-mylene.josserand@free-electrons.com> <20170529144538.29187-2-mylene.josserand@free-electrons.com> <20170530080225.rgvy5too2cy5m2zp@flea.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20170530080225.rgvy5too2cy5m2zp@flea.lan> Sender: linux-input-owner@vger.kernel.org To: Maxime Ripard Cc: dmitry.torokhov@gmail.com, fery@cypress.com, robh+dt@kernel.org, mark.rutland@arm.com, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, devicetree@vger.kernel.org, thomas.petazzoni@free-electrons.com List-Id: devicetree@vger.kernel.org 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 "); > > Good work cleaning it up! Thank you! Best regards, -- Mylène Josserand, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com