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@xxxxxxxxxxxxxxxxxx>");
Good work cleaning it up!
Thank you!
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
