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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Oleh,

On Thu, Aug 29, 2024 at 11:20:14PM +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@gmail.com>
> ---
> 
> 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

Thank you for making the changes, a few more comments/suggestions:

> +
> +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},
> +};
> +
> +static int cst816x_i2c_read_register(struct cst816x_priv *priv, u8 reg,
> +				     void *buf, size_t len)
> +{
> +	struct i2c_client *client;

Whenever reasonable combine declaration and initialization:

	struct i2c_client *client = priv->client;

> +	struct i2c_msg xfer[2];

	struct i2c_msg xfer[] = {
		{
			.addr = client->addr,
			.buf = &reg,
			.len = sizeof(reg),

		},
		{
			.addr = client->addr,
			.flags = I2C_M_RD,
			.buf = buf,
			.len = len,
		},
	};

> +	int rc;
> +
> +	client = priv->client;
> +
> +	xfer[0].addr = client->addr;
> +	xfer[0].flags = 0;
> +	xfer[0].buf = &reg;
> +	xfer[0].len = sizeof(reg);
> +
> +	xfer[1].addr = client->addr;
> +	xfer[1].flags = I2C_M_RD;
> +	xfer[1].buf = buf;
> +	xfer[1].len = len;
> +
> +	rc = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> +	if (rc != ARRAY_SIZE(xfer)) {
> +		if (rc >= 0)
> +			rc = -EIO;
		rc = rc < 0 ? rc : -EIO;
		dev_err(...);
		return rc;
> +	} else {
> +		rc = 0;
> +	}
> +
> +	if (rc < 0)
> +		dev_err(&client->dev, "i2c rx err: %d\n", rc);
> +
> +	return rc;

Explicitly returning 0 on success is preferred if code looks reasonable.

	return 0;

> +}
> +
> +static int cst816x_process_touch(struct cst816x_priv *priv,
> +				 struct cst816x_touch_info *info)
> +{
> +	u8 raw[8];
> +	int rc;

	int error;

> +
> +	rc = cst816x_i2c_read_register(priv, CST816X_FRAME, raw, sizeof(raw));
> +	if (!rc) {

	error = cst816x_i2c_read_register(...);
	if (error)
		return error;


> +		info->gesture = raw[0];
> +		info->touch = raw[1];
> +		info->abs_x = ((raw[2] & 0x0F) << 8) | raw[3];

I think it can be written as

	info->abs_x = get_unaligned_le16(&raw[2]) & GENMASK(11, 0);

> +		info->abs_y = ((raw[4] & 0x0F) << 8) | raw[5];
> +
> +		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 rc;

	return 0;

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

I believe the reset line should be optional, and so check for non-NULL
here before trying to execute the reset sequence.

> +	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)
> +{
> +	u16 key = event_map[gesture & 0x0F].code;
> +
> +	if (key != KEY_RESERVED)
> +		input_report_key(priv->input, key, touch);
> +
> +	if (!touch)
> +		input_report_key(priv->input, BTN_TOUCH, 0);

This chunk does not belong here but rather where you report the rest of
the touch state.

> +}
> +
> +/*
> + * 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
> + */
> +static irqreturn_t cst816x_irq_cb(int irq, void *cookie)
> +{
> +	struct cst816x_priv *priv = (struct cst816x_priv *)cookie;

No need to cast void pointers.

> +	struct cst816x_touch_info info;
> +
> +	if (!cst816x_process_touch(priv, &info)) {

This makes it appear cst816x_process_touch() returning boolean.

	error = cst816x_process_touch(priv, &info);
	if (error)
		goto out;

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

		} else {
			input_report_key(priv->input, BTN_TOUCH, 0);
		}

> +
> +		if (info.gesture)
> +			report_gesture_event(priv, info.gesture, info.touch);
> +
> +		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 rc;

	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(dev, "reset", GPIOD_OUT_HIGH);

Please make reset optional, it does not have to be handled by the driver
if something else (firmware) will be doing power sequencing.

> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "reset gpio not found\n");
> +
> +	cst816x_reset(priv);
> +
> +	rc = cst816x_register_input(priv);
> +	if (rc)
> +		return dev_err_probe(dev, rc, "input register failed\n");
> +
> +	rc = devm_request_threaded_irq(dev, client->irq, NULL, cst816x_irq_cb,
> +				       IRQF_ONESHOT, dev->driver->name, priv);
> +	if (rc)
> +		return dev_err_probe(dev, rc, "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@gmail.com>");
> +MODULE_DESCRIPTION("Hynitron CST816X Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Thanks.

-- 
Dmitry