| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZqqRgnxZBW0nGTpF@google.com>
Date: Wed, 31 Jul 2024 12:33:22 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Oleh Kuzhylnyi <kuzhylol@...il.com>
Cc: linux-input@...r.kernel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, jeff@...undy.com,
neil.armstrong@...aro.org, hdegoede@...hat.com,
artur.serhiienko@...il.com, igor.opaniuk@...il.com
Subject: Re: [PATCH v3 2/2] input: add driver for Hynitron CST816X touchscreen
Hi Oleh,
On Sun, May 26, 2024 at 11:24:42PM +0200, Oleh Kuzhylnyi wrote:
> Introduce support for the Hynitron CST816X touchscreen controller
> used for 240×240 1.28-inch Round LCD Display Module manufactured
> by Waveshare Electronics. The driver is designed based on an Arduino
> implementation marked as under MIT License. This driver is written
> for a particular round display based on the CST816S controller, which
> is not compatiable with existing driver for Hynitron controllers.
>
> Signed-off-by: Oleh Kuzhylnyi <kuzhylol@...il.com>
> ---
>
> Changes in v3:
> - Drop timer and delayed work
> - Process touch in threaded IRQ context
> - Fix chip reset sequence
> - Move input_register() before devm_request_threaded_irq()
> - Re-arrange and document input reporting
> - Set u16 data type for event_code
> - Remove double tap event to prevent continuous double side sliding
>
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/hynitron-cst816x.c | 257 +++++++++++++++++++
> 3 files changed, 270 insertions(+)
> create mode 100644 drivers/input/touchscreen/hynitron-cst816x.c
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c821fe3ee794..02f40d0fbac0 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -481,6 +481,18 @@ config TOUCHSCREEN_HYNITRON_CSTXXX
> To compile this driver as a module, choose M here: the
> module will be called hynitron-cstxxx.
>
> +config TOUCHSCREEN_HYNITRON_CST816X
> + tristate "Hynitron CST816X touchscreen support"
> + depends on I2C
> + help
> + Say Y here if you have a touchscreen using a Hynitron
> + CST816X touchscreen controller.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called hynitron-cst816x.
> +
> config TOUCHSCREEN_ILI210X
> tristate "Ilitek ILI210X based touchscreen"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index a81cb5aa21a5..a92a87417a97 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> +obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CST816X) += hynitron-cst816x.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> diff --git a/drivers/input/touchscreen/hynitron-cst816x.c b/drivers/input/touchscreen/hynitron-cst816x.c
> new file mode 100644
> index 000000000000..5bb85ec1d60e
> --- /dev/null
> +++ b/drivers/input/touchscreen/hynitron-cst816x.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for I2C connected Hynitron CST816X Touchscreen
> + *
> + * Copyright (C) 2024 Oleh Kuzhylnyi <kuzhylol@...il.com>
> + */
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +
> +enum cst816x_registers {
> + CST816X_FRAME = 0x01,
> + CST816X_MOTION = 0xEC,
> +};
> +
> +enum cst816x_gestures {
> + CST816X_SWIPE_UP = 0x01,
> + CST816X_SWIPE_DOWN = 0x02,
> + CST816X_SWIPE_LEFT = 0x03,
> + CST816X_SWIPE_RIGHT = 0x04,
> + CST816X_SINGLE_TAP = 0x05,
> + CST816X_LONG_PRESS = 0x0C,
> +};
> +
> +struct cst816x_touch_info {
> + u8 gesture;
> + u8 touch;
> + u8 abs_x;
Is this the right size? You are doing:
priv->info.abs_x = ((raw[2] & 0x0F) << 8) | raw[3];
Which suggests it should be u16?
> + u8 abs_y;
> +} __packed;
Why does this need to be __packed?
> +
> +struct cst816x_priv {
> + struct device *dev;
> + struct i2c_client *client;
> + struct gpio_desc *reset;
> + struct input_dev *input;
> + struct cst816x_touch_info info;
Why do you need to have this here? It is tiny and does not need to be
kept around, just use on stack variable.
> +
> + u8 rxtx[8];
> +};
> +
> +struct cst816x_event_mapping {
> + enum cst816x_gestures gesture;
> + u16 event_code;
> +};
> +
> +static const struct cst816x_event_mapping cst816x_event_map[] = {
> + {CST816X_SWIPE_UP, BTN_FORWARD},
> + {CST816X_SWIPE_DOWN, BTN_BACK},
> + {CST816X_SWIPE_LEFT, BTN_LEFT},
> + {CST816X_SWIPE_RIGHT, BTN_RIGHT},
> + {CST816X_SINGLE_TAP, BTN_TOUCH},
> + {CST816X_LONG_PRESS, BTN_TOOL_TRIPLETAP}
> +};
> +
> +static int cst816x_i2c_read_register(struct cst816x_priv *priv, u8 reg)
Maybe supply buffer and size as arguments?
> +{
> + struct i2c_client *client;
> + struct i2c_msg xfer[2];
> + int rc;
> +
> + client = priv->client;
> +
> + xfer[0].addr = client->addr;
> + xfer[0].flags = 0;
> + xfer[0].len = sizeof(reg);
> + xfer[0].buf = ®
> +
> + xfer[1].addr = client->addr;
> + xfer[1].flags = I2C_M_RD;
> + xfer[1].len = sizeof(priv->rxtx);
> + xfer[1].buf = priv->rxtx;
> +
> + rc = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> + if (rc != ARRAY_SIZE(xfer)) {
error = rc >= 0 ? -EIO : rc;
dev_err(...);
return error;
}
return 0;
> + if (rc >= 0)
> + rc = -EIO;
> + } else {
> + rc = 0;
> + }
> +
> + if (rc < 0)
> + dev_err(&client->dev, "i2c rx err: %d\n", rc);
> +
> + return rc;
> +}
> +
> +static int cst816x_process_touch(struct cst816x_priv *priv)
> +{
> + u8 *raw;
> + int rc;
> +
> + rc = cst816x_i2c_read_register(priv, CST816X_FRAME);
> + if (!rc) {
> + raw = priv->rxtx;
> +
> + priv->info.gesture = raw[0];
> + priv->info.touch = raw[1];
> + priv->info.abs_x = ((raw[2] & 0x0F) << 8) | raw[3];
> + priv->info.abs_y = ((raw[4] & 0x0F) << 8) | raw[5];
> +
> + dev_dbg(priv->dev, "x: %d, y: %d, t: %d, g: 0x%x\n",
> + priv->info.abs_x, priv->info.abs_y, priv->info.touch,
> + priv->info.gesture);
> + }
> +
> + return rc;
> +}
> +
> +static int cst816x_register_input(struct cst816x_priv *priv)
> +{
> + priv->input = devm_input_allocate_device(priv->dev);
> + if (!priv->input)
> + return -ENOMEM;
> +
> + priv->input->name = "Hynitron CST816X Touchscreen";
> + priv->input->phys = "input/ts";
> + priv->input->id.bustype = BUS_I2C;
> + input_set_drvdata(priv->input, priv);
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(cst816x_event_map); i++) {
> + input_set_capability(priv->input, EV_KEY,
> + cst816x_event_map[i].event_code);
> + }
> +
> + input_set_abs_params(priv->input, ABS_X, 0, 240, 0, 0);
> + input_set_abs_params(priv->input, ABS_Y, 0, 240, 0, 0);
> +
> + return input_register_device(priv->input);
> +}
> +
> +static void cst816x_reset(struct cst816x_priv *priv)
> +{
> + gpiod_set_value_cansleep(priv->reset, 1);
> + msleep(50);
> + gpiod_set_value_cansleep(priv->reset, 0);
> + msleep(100);
> +}
> +
> +static void report_gesture_event(const struct cst816x_priv *priv,
> + enum cst816x_gestures gesture, bool touch)
> +{
> + for (unsigned int i = 0; i < ARRAY_SIZE(cst816x_event_map); i++) {
> + if (cst816x_event_map[i].gesture == gesture) {
> + input_report_key(priv->input,
> + cst816x_event_map[i].event_code,
> + touch);
If you make cst816x_event_map an array of 16 entries with unised
positions mapped to KEY_RESERVED you can do:
key = cst816x_event_map[raw[0] & 0x0f];
if (key != KEY_RESERVED)
input_report_key(priv->input, key, touch);
and you do not need to loop.
Thanks.
--
Dmitry
Powered by blists - more mailing lists