Re: [PATCH v6 2/2] input: add driver for Hynitron CST816X touchscreen

From: Jeff LaBundy
Date: Tue Sep 10 2024 - 11:47:58 EST

Hi Oleh,

Great work! Just a few comments throughout.

On Tue, Sep 10, 2024 at 01:51:58PM +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@xxxxxxxxx>
> ---
>
> Changes in v6:
> - No code changes
>
> Changes in v5:
> - Update commit based on Dmitry's feedback:
> - Make GPIO reset optional
> - Combine declaration and initialization for i2c_xfer
> - Return 0 explicitly where possible
> - Rename rc (return code) to error
> - Make Touch processing call return boolean
> - Improve error handling for i2c_transfer
> - Use get_unaligned_be16 for getting coordinates
> - Move touch event completeness upper to irq callback
>
> Changes in v4:
> - Update commit based on Dmitry's feedback:
> - Move abs_x and abs_y to u16
> - Remove __packed qualifier for touch_info struct
> - Hide tiny touch irq context to stack
> - Extend cst816x_i2c_read_register() with buf and buf_size
> - Remove loop from event lookup
>
> 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
>
> Changes in v2:
> - Apply dev_err_probe() for better error handling
> - Remove redundant printing, remove dev_warn() message spamming
> - Get rid of PM since the touchscreen goes into sleep mode automatically
> - Get rid of IRQ control and IRQF_NO_AUTOEN flag
> - Reduce timer timeout up to 10ms to handle touch events faster
> - Skip registering of non-gesture CST816X_SWIPE event
> - Shift input_register_device() as a final call in probe() callback
> - Specify name of i2c_device_id explicitly
> - Update module description and fix typo
> - Add necessary spaces between lines
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/hynitron-cst816x.c | 259 +++++++++++++++++++
> 3 files changed, 272 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..3886617e6a71
> --- /dev/null
> +++ b/drivers/input/touchscreen/hynitron-cst816x.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for I2C connected Hynitron CST816X Touchscreen
> + *
> + * Copyright (C) 2024 Oleh Kuzhylnyi <kuzhylol@xxxxxxxxx>
> + */
> +#include <asm/unaligned.h>
> +#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,
> + CST816X_RESERVED = 0xFF,
> +};
> +
> +struct cst816x_touch_info {
> + u8 gesture;
> + u8 touch;
> + u16 abs_x;
> + u16 abs_y;
> +};
> +
> +struct cst816x_priv {
> + struct device *dev;
> + struct i2c_client *client;

I don't see any value in storing both the device and i2c_client pointers
in the private struct; I think you can simply drop the former.

> + struct gpio_desc *reset;
> + struct input_dev *input;
> +};
> +
> +struct cst816x_event_mapping {
> + enum cst816x_gestures gesture;
> + u16 code;
> +};
> +
> +static const struct cst816x_event_mapping event_map[16] = {
> + {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},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> + {CST816X_RESERVED, KEY_RESERVED},
> +};

These really should be configurable via device tree and not hard coded
in the driver. At the very least, if the touchscreen is installed 180
degrees for certain platforms, the concept of "left" and "right" changes.

> +
> +static int cst816x_i2c_read_register(struct cst816x_priv *priv, u8 reg,
> + void *buf, size_t len)
> +{
> + int rc;
> + struct i2c_msg xfer[] = {
> + {
> + .addr = priv->client->addr,
> + .flags = 0,
> + .buf = &reg,
> + .len = sizeof(reg),
> + },
> + {
> + .addr = priv->client->addr,
> + .flags = I2C_M_RD,
> + .buf = buf,
> + .len = len,
> + },
> + };
> +
> + rc = i2c_transfer(priv->client->adapter, xfer, ARRAY_SIZE(xfer));
> + if (rc != ARRAY_SIZE(xfer)) {
> + rc = rc < 0 ? rc : -EIO;
> + dev_err(&priv->client->dev, "i2c rx err: %d\n", rc);

Case in point: you already have priv->dev, but it's not being used.

> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +static bool cst816x_process_touch(struct cst816x_priv *priv,
> + struct cst816x_touch_info *info)
> +{
> + u8 raw[8];
> +
> + if (cst816x_i2c_read_register(priv, CST816X_FRAME, raw, sizeof(raw)))
> + return false;
> +
> + info->gesture = raw[0];
> + info->touch = raw[1];
> + info->abs_x = get_unaligned_be16(&raw[2]) & GENMASK(11, 0);
> + info->abs_y = get_unaligned_be16(&raw[4]) & GENMASK(11, 0);

This seems like a good case to make cst816x_touch_info a __packed struct
with abs_x and abs_y declared as__be16 members. You can then read into an
instance of this struct and unpack as necessary, as opposed to essentially
having two buffers and copying one into the other manually.

> +
> + dev_dbg(priv->dev, "x: %d, y: %d, t: %d, g: 0x%x\n", info->abs_x,
> + info->abs_y, info->touch, info->gesture);
> +
> + return true;
> +}
> +
> +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(event_map); i++)
> + input_set_capability(priv->input, EV_KEY, event_map[i].code);

Nit: it's much more common in kernel code for iterators to be of type int.

> +
> + input_set_abs_params(priv->input, ABS_X, 0, 240, 0, 0);
> + input_set_abs_params(priv->input, ABS_Y, 0, 240, 0, 0);

I see the binding includes the touchscreen helper binding, but you're not
actually using any of the helpers. Assuming that's intentional, please drop
the reference to touchscreen.yaml in the binding.

> +
> + return input_register_device(priv->input);
> +}
> +
> +static void cst816x_reset(struct cst816x_priv *priv)
> +{
> + if (priv->reset) {
> + gpiod_set_value_cansleep(priv->reset, 1);
> + msleep(50);
> + gpiod_set_value_cansleep(priv->reset, 0);
> + msleep(100);
> + }

This is a style choice, but you can save some indentation by doing
the following:

if (!priv->reset)
return;

gpiod_set_value_cansleep(...);

> +}
> +
> +static void report_gesture_event(const struct cst816x_priv *priv,
> + enum cst816x_gestures gesture, bool touch)
> +{
> + u16 key = event_map[gesture & 0x0F].code;
> +
> + if (key != KEY_RESERVED)
> + input_report_key(priv->input, key, touch);

There is no need to manually filter KEY_RESERVED; the input core
automatically marks this key code as unsupported upon registration.

> +}
> +
> +/*
> + * Supports five gestures: TOUCH, LEFT, RIGHT, FORWARD, BACK, and LONG_PRESS.
> + * Reports surface interaction, sliding coordinates and finger detachment.
> + *
> + * 1. TOUCH Gesture Scenario:
> + *
> + * [x/y] [touch] [gesture] [Action] [Report ABS] [Report Key]
> + * x y true 0x00 Touch ABS_X_Y BTN_TOUCH
> + * x y true 0x00 Slide ABS_X_Y
> + * x y false 0x05 Gesture BTN_TOUCH
> + *
> + * 2. LEFT, RIGHT, FORWARD, BACK, and LONG_PRESS Gestures Scenario:
> + *
> + * [x/y] [touch] [gesture] [Action] [Report ABS] [Report Key]
> + * x y true 0x00 Touch ABS_X_Y BTN_TOUCH
> + * x y true 0x01 Gesture ABS_X_Y BTN_FORWARD
> + * x y true 0x01 Slide ABS_X_Y
> + * x y false 0x01 Detach BTN_FORWARD | BTN_TOUCH
> + */

This is one very specific implementation, and too restrictive to be
hard coded into the driver. As mentioned above, please consider making
gesture key codes configurable by way of the device tree.

> +static irqreturn_t cst816x_irq_cb(int irq, void *cookie)
> +{
> + struct cst816x_priv *priv = cookie;
> + struct cst816x_touch_info info;
> +
> + if (!cst816x_process_touch(priv, &info))
> + goto out;

This is mostly a style choice, but it would also be fine to simply
return directly here since there is no clean-up to do.

> +
> + if (info.touch) {
> + input_report_abs(priv->input, ABS_X, info.abs_x);
> + input_report_abs(priv->input, ABS_Y, info.abs_y);
> + input_report_key(priv->input, BTN_TOUCH, 1);
> + }
> +
> + if (info.gesture) {
> + report_gesture_event(priv, info.gesture, info.touch);
> +
> + if (!info.touch)
> + input_report_key(priv->input, BTN_TOUCH, 0);
> + }
> +
> + input_sync(priv->input);
> +
> +out:
> + return IRQ_HANDLED;
> +}
> +
> +static int cst816x_probe(struct i2c_client *client)
> +{
> + struct cst816x_priv *priv;
> + struct device *dev = &client->dev;
> + int error;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> + priv->client = client;
> +
> + priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(priv->reset))
> + return dev_err_probe(dev, PTR_ERR(priv->reset),
> + "reset gpio not found\n");
> +
> + cst816x_reset(priv);
> +
> + error = cst816x_register_input(priv);
> + if (error)
> + return dev_err_probe(dev, error, "input register failed\n");
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, cst816x_irq_cb,
> + IRQF_ONESHOT, dev->driver->name, priv);
> + if (error)
> + return dev_err_probe(dev, error, "irq request failed\n");
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id cst816x_id[] = {
> + { .name = "cst816s", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, cst816x_id);
> +
> +static const struct of_device_id cst816x_of_match[] = {
> + { .compatible = "hynitron,cst816s", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, cst816x_of_match);
> +
> +static struct i2c_driver cst816x_driver = {
> + .driver = {
> + .name = "cst816x",
> + .of_match_table = cst816x_of_match,
> + },
> + .id_table = cst816x_id,
> + .probe = cst816x_probe,
> +};
> +
> +module_i2c_driver(cst816x_driver);
> +
> +MODULE_AUTHOR("Oleh Kuzhylnyi <kuzhylol@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Hynitron CST816X Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>

Kind regards,
Jeff LaBundy