| 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: <ZtPtIdqm4G7SqYvd@google.com>
Date: Sat, 31 Aug 2024 21:27:13 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Oleh Kuzhylnyi <kuzhylol@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-input@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Conor Dooley <conor.dooley@...rochip.com>, igor.opaniuk@...il.com,
Neil Armstrong <neil.armstrong@...aro.org>,
Jeff LaBundy <jeff@...undy.com>
Subject: Re: [PATCH v4 2/2] input: add driver for Hynitron CST816X touchscreen
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@...il.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 = ®,
.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 = ®
> + 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@...il.com>");
> +MODULE_DESCRIPTION("Hynitron CST816X Touchscreen Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
Thanks.
--
Dmitry
Powered by blists - more mailing lists