Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 23 Dec 2021 15:47:21 +0200 | Subject | Re: [PATCH v2 1/4] Input: Add driver for Cypress Generation 5 touchscreen |
| |
On Wed, Nov 3, 2021 at 1:49 PM Alistair Francis <alistair@alistair23.me> wrote:
...
> Message-Id: <20180703094309.18514-2-mylene.josserand@bootlin.com>
What is this?
...
> +config TOUCHSCREEN_CYTTSP5 > + tristate "Cypress TrueTouch Gen5 Touchscreen Driver"
> + depends on OF && I2C
Is it really OF dependent?
> + 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.
...
> +/* > + * 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?
> +#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.
> +#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.
> +#define CY_HID_OUTPUT_TIMEOUT 200 > +#define CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT 3000 > +#define CY_HID_GET_HID_DESCRIPTOR_TIMEOUT 4000
...
> +/* helpers */ > +#define HI_BYTE(x) ((u8)(((x) >> 8) & 0xFF)) > +#define LOW_BYTE(x) ((u8)((x) & 0xFF))
Why are these needed? Just use them directly.
...
> +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
> + * 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
> + * 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.
> +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;
Redundant. The of_property_*() will do it anyway.
> + 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() ?
> + 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;
...
> + 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?
> + dev_err(ts->dev, "HID output response, wrong CRC 0x%X\n", > + crc); > + return -EPROTO; > + }
...
> + 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.
...
> + 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.
...
> + crc = cyttsp5_compute_crc(&cmd[4], 4); > + cmd[8] = LOW_BYTE(crc); > + cmd[9] = HI_BYTE(crc);
Ditto.
...
> + 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.
> +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.
...
> + /* 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);
...
> +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.
> + }, > + .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
| |