On Sat, Nov 6, 2021 at 4:47 PM Dmitry Torokhov
[off-list ref] wrote:

Hi Alistair,

On Wed, Nov 03, 2021 at 09:48:27PM +1000, Alistair Francis wrote:

quoted

From: Mylčne Josserand <redacted>

...

quoted

+
+/*
+ * 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;

Please call this "error". Also elsewhere, variables that hold either
error code or success (0) should be called "error".

quoted

+     u32 size;
+     u8 temp[2];
+
+     if (!buf)
+             return -EINVAL;

This is an internal function. How can it be called with NULL buffer?

I have removed the check

quoted

+
+     /* Read the frame to retrieve the size */
+     rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, 2);

sizeof(temp)

quoted

+     if (rc < 0)

regmap_bulk_read() retirns 0 on success, so the check should be

        if (error)

quoted

+             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[HID_OUTPUT_MAX_CMD_SIZE];
+
+     if (size + 1 > HID_OUTPUT_MAX_CMD_SIZE) {
+             return -ENOMEM;

Maybe -E2BIG or -EINVAL.

quoted

+     }

Unnecessary curly braces.

quoted

+
+     /* 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;
+
+     for (nbyte = 0, *axis = 0; nbyte < size; nbyte++)
+             *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8));
+
+     *axis &= max - 1;

Can it be that max is not power of 2?

Not that I have seen.

quoted

+}
+
+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);
+     }

Unnecessary curly braces.

quoted

+}
+
+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, MAX_CY_TCH_T_IDS);
+     u8 *tch_addr;
+     int tmp;
+
+     bitmap_zero(ids, MAX_CY_TCH_T_IDS);
+     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]);
+
+             touchscreen_report_pos(ts->input, &ts->prop,
+                                    tch->abs[CY_TCH_X], tch->abs[CY_TCH_Y],
+                                    true);

I have no idea why you first report position manually (without applying
transformation) and then finally use touchscreen_report_pos() to report
is again, now properly.

Fixed!

quoted

+     }
+
+     cyttsp5_final_sync(ts->input, si->tch_abs[CY_TCH_T].max, ids);

I think if you use INPUT_MT_DROP_UNUSED when initializing slots and call
input_mt_sync_frame() then cyttsp5_final_sync() will not be needed.

Yep, I have removed cyttsp5_final_sync()

quoted

+
+     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,

I am struggling to understand why this member is called "hdr" and why it
is a part of cyttsp5_touch.

Good point, removed.

quoted

+                            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);

Not needed with INPUT_MT_DROP_UNUSED.

I tried INPUT_MT_DROP_UNUSED and I still need this function

quoted

+
+     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;

It is only ever called with this condition, why do we need to check
this again?

Removed.

quoted

+
+     /* 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);

input_set_abs_params() does this I believe.

Removed

quoted

+     __set_bit(EV_REL, ts->input->evbit);

I do not believe the driver emits any EV_REL events.

If I delete this my userspace program doesn't start.

quoted

+     __set_bit(EV_KEY, ts->input->evbit);

I think I saw it done elsewhere already.

Yep, removed.

quoted

+
+     max_x_tmp = si->sensing_conf_data.res_x;
+     max_y_tmp = si->sensing_conf_data.res_y;
+     max_x = max_x_tmp - 1;
+     max_y = max_y_tmp - 1;
+     max_p = si->sensing_conf_data.max_z;
+
+     input_mt_init_slots(ts->input, si->tch_abs[CY_TCH_T].max, 0);

Error handling. You also want to use INPUT_MT_POINTER and potentially
INPUT_MT_DROP_UNUSED.

Done!

quoted

+
+     __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);

These 3 are not needed.

quoted

+
+     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);

I would use device_property_read_u32_array() instead and did not make it
limited to OF.

quoted

+}
+#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;

Do we have matching implementation in lib/crc* by any chance?

Yep, it seems that we do.

quoted

+}
+
+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];

This is definitely not an "offset".

quoted

+
+     if (offset == HID_BL_RESPONSE_REPORT_ID) {

        switch (report_id) {

quoted

+             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++;
+     }

        hweight*()?

Fixed!

quoted

+}
+
+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);

What it the absolute maximum for tracked contacts? I wonder if it makes
sense to allocate this buffer separately.

I have removed this as well and just use the input_buf.

quoted

+     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;

Why do we need to allocate 7 bytes separately?

I have removed this.

quoted

+
+     return 0;
+}
+

...

quoted

+
+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);

Why do we need to do all these moves? Can we parse the date directly
from the input buffer?

We don't need the moves. I have removed them.

quoted

+
+     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);

Why is this lock needed exactly?

I don't think it is, I have removed it.

quoted

+             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];

I'd probably do:

        if (size == 0) {
                /* reset */
                report_id = HID_EMPTY_RESPONSE_ID; /* = 0 */
                size = 2;
        } else {
                report_id = ts->input_buf[2];
        }

quoted

+
+     if (report_id == HID_TOUCH_REPORT_ID) {

        switch (report_id) {

Good idea, fixed.

quoted

+             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;

If you were to use level interrupts this probably would not be needed.

I have tried with/without this and I still can't get level interrupts working.

quoted

+
+     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,tt21000", },
+     { }
+};
+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);
+
+     /* Power up the device */
+     ts->vdd = regulator_get(dev, "vdd");

Do not mix managed an unmanaged resources. You are leaking this
regulator.

I'm not clear what I should do differently here.

quoted

+     if (IS_ERR(ts->vdd)) {
+             rc = PTR_ERR(ts->vdd);
+             dev_set_drvdata(dev, NULL);
+             kfree(ts);
+             return rc;
+     }
+
+     rc = regulator_enable(ts->vdd);
+     if (rc) {
+             regulator_put(ts->vdd);
+             dev_set_drvdata(dev, NULL);
+             kfree(ts);
+             return rc;
+     }
+
+     /* 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,

Please do not override platform setup with hardcoded triggers. Also, it
is strongly recommended to use level interrupts for these peripherals.

This is also likely unsafe if controller is not completely shut off and
is capable of generating interrupts given input device is not yet
allocated.

I have dropped the `IRQF_TRIGGER_FALLING |`

I have tried to use level interrupts, but I can't get the device
working with them.

quoted

+                                    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;

No need to do this, devm_input_allocate_device() sets up parent.

quoted

+     input_set_drvdata(ts->input, ts);
+
+     touchscreen_parse_properties(ts->input, true, &ts->prop);
+
+     __set_bit(EV_KEY, ts->input->evbit);
+     __set_bit(ABS_X, ts->input->absbit);
+     __set_bit(ABS_Y, ts->input->absbit);
+     __set_bit(BTN_TOUCH, ts->input->keybit);

These 3 should not be needed.

quoted

+     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);

This is not needed as input device is devm managed. This entire function
can be dropped.

Removed

quoted

+
+     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,
+             .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 [off-list ref]");
--
2.31.1

Thanks.

Thanks for the in depth review.

I have addressed all of the issues that I didn't comment on.

Alistair

--
Dmitry