Thread (22 messages) 22 messages, 7 authors, 2021-12-23

Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen

From: Andy Shevchenko <hidden>
Date: 2021-12-23 13:49:24
Also in: linux-arm-kernel, linux-input, lkml

On Wed, Nov 3, 2021 at 1:49 PM Alistair Francis [off-list ref] wrote:

...
Message-Id: [ref]
What is this?

...
+config TOUCHSCREEN_CYTTSP5
+       tristate "Cypress TrueTouch Gen5 Touchscreen Driver"
+       depends on OF && I2C
Is it really OF dependent?
quoted hunk ↗ jump to hunk
+       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.
Utilize lines better, i.e. increase density of the words on them.

...
quoted hunk ↗ jump to hunk
+/*
+ * Parade TrueTouch(TM) Standard Product V5 Module.
+ *
+ * Copyright (C) 2015 Parade Technologies
+ * Copyright (C) 2012-2015 Cypress Semiconductor
+ * Copyright (C) 2018 Bootlin
+ *
+ * Authors: Mylène Josserand <mylene.josserand@bootlin.com>
+ *                Alistair Francis <alistair@alistair23.me>
+ *
Redundant.
+ */
...
+#include <asm/unaligned.h>
Can you move it after linux/* ones?
quoted hunk ↗ jump to hunk
+#include <linux/crc-itu-t.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
This is wrong. New code shouldn't include this header.
quoted hunk ↗ jump to hunk
+#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 CY_BITS_PER_BTN                        1
+#define CY_NUM_BTN_EVENT_ID                    ((1 << CY_BITS_PER_BTN) - 1)
Seems like a mask depending on the amount of bits.
Perhaps GENMASK(1, 0)?

...
+#define HID_OUTPUT_RESPONSE_CMD_MASK           0x7F
GENMASK()

...
+#define HID_SYSINFO_BTN_MASK                   0xFF
Ditto.

...
+/*  Timeout in ms */
Drop this comment and use _MS suffix in the definitions.
quoted hunk ↗ jump to hunk
+#define CY_HID_OUTPUT_TIMEOUT                  200
+#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT      3000
+#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT      4000
...
quoted hunk ↗ jump to hunk
+/* helpers */
+#define HI_BYTE(x)                             ((u8)(((x) >> 8) & 0xFF))
+#define LOW_BYTE(x)                            ((u8)((x) & 0xFF))
Why are these needed? Just use them directly.

...
quoted hunk ↗ jump to hunk
+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,
If it is a terminator, drop the comma.
+};
...
+/*
+ * For what understood in the datasheet, the register does not
is understood
+ * matter. For consistency, used the Input Register address
use
quoted hunk ↗ jump to hunk
+ * but it does mean anything to the device. The important data
+ * to send is the I2C address
+ */
+       /* The hardware wants to receive a frame with the address register
Be consistent with multi-line comments. Here you need to add a leading
empty line.
+        * contains in the first two bytes. As the regmap_write function
contained
quoted hunk ↗ jump to hunk
+        * 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
+        */
...
+       for (nbyte = 0, *axis = 0; nbyte < size; nbyte++)
+               *axis = *axis + ((xy_data[nbyte] >> bofs) << (nbyte * 8));
+=

...
+#ifdef CONFIG_OF
Why? You may easily combine the two.
quoted hunk ↗ jump to hunk
+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;
quoted hunk ↗ jump to hunk
+       np = dev->of_node;
+       if (!np)
+               return -EINVAL;
Redundant. The of_property_*() will do it anyway.
quoted hunk ↗ jump to hunk
+       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;
memset32() ?
quoted hunk ↗ jump to hunk
+       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;
+}
Combine with the above and get rid of ugly ifdeffery.
+#endif
...
+       size = get_unaligned_le16(&ts->response_buf[0]);
+
Redundant blank line.
+       if (!size)
+               return 0;
...
quoted hunk ↗ jump to hunk
+               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)) {
What's wrong with a direct comparison with _le16 value?
quoted hunk ↗ jump to hunk
+                       dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n",
+                               crc);
+                       return -EPROTO;
+               }
...
quoted hunk ↗ jump to hunk
+       si->num_btns = 0;
+       for (i = 0; i < HID_SYSINFO_MAX_BTN; i++) {
+               if (btns & BIT(i))
+                       si->num_btns++;
+       }
NIH for_each_set_bit(), but do one more step to see that this is
actually a Hamming weight. We have functions. Hence, make it one line
with one simple call.

...
+       struct cyttsp5_sensing_conf_data_dev *scd_dev =
+               (struct cyttsp5_sensing_conf_data_dev *)
Why casting?
+               &ts->response_buf[HID_SYSINFO_SENSING_OFFSET];
...
+       si->xy_data = devm_kzalloc(ts->dev, scd->max_tch * TOUCH_REPORT_SIZE,
+                                  GFP_KERNEL);
Use _kcalloc().
+       if (!si->xy_data)
+               return -ENOMEM;
...
+       cmd[0] = LOW_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
+       cmd[1] = HI_BYTE(HID_OUTPUT_GET_SYSINFO_SIZE);
Use put_unaligned_le16() instead.

...
quoted hunk ↗ jump to hunk
+       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;
What is this supposed to mean?
+               goto error;
+       }
...
+       cmd[0] = LOW_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
+       cmd[1] = HI_BYTE(HID_OUTPUT_BL_LAUNCH_APP_SIZE);
put_unaligned_le16()

...
+       cmd[6] = 0x0; /* Low bytes of data */
+       cmd[7] = 0x0; /* Hi bytes of data */
Ditto.

...
quoted hunk ↗ jump to hunk
+       crc = cyttsp5_compute_crc(&cmd[4], 4);
+       cmd[8] = LOW_BYTE(crc);
+       cmd[9] = HI_BYTE(crc);
Ditto.

...
quoted hunk ↗ jump to hunk
+       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;
+       }
...
+       memcpy(desc, ts->response_buf, sizeof(struct cyttsp5_hid_desc));
sizeof(*desc) ?

...
+       rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2);
sizeof(buf)
+       if (rc < 0)
+               return rc;
...
+#ifdef CONFIG_OF
Drop this. It usually brings more pain than helps.
quoted hunk ↗ jump to hunk
+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);
Move both closer to their users (below).

...
+               dev_set_drvdata(dev, NULL);
No need to repeat what device core does for everybody for the last 10+ years.

...
+               dev_set_drvdata(dev, NULL);
Ditto.

...
quoted hunk ↗ jump to hunk
+       /* 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;
+       }
No minimum reset timeout?
+       gpiod_set_value(ts->reset_gpio, 1);
Or what does this all mean? Shouldn't you simply use _OUT_HIGH above?
+       /* Need a delay to have device up */
+       msleep(20);
...
quoted hunk ↗ jump to hunk
+static struct i2c_driver cyttsp5_i2c_driver = {
+       .driver = {
+               .name = CYTTSP5_NAME,
+               .of_match_table = of_match_ptr(cyttsp5_of_match),
Drop of_match_ptr() as well.
quoted hunk ↗ jump to hunk
+       },
+       .probe = cyttsp5_i2c_probe,
+       .remove = cyttsp5_i2c_remove,
+       .id_table = cyttsp5_i2c_id,
+};
+
Redundant blank line.
+module_i2c_driver(cyttsp5_i2c_driver);
--
With Best Regards,
Andy Shevchenko
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help