On 17/05/2024 11:27, 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.
>
...
> +static int cst816x_process_touch(struct cst816x_priv *priv)
> +{
> + u8 *raw;
> + int rc;
> +
> + rc = cst816x_i2c_read_reg(priv, CST816X_FRAME);
> + if (!rc) {
> + raw = priv->rxtx;
> +
> + priv->info.gesture = raw[0];
> + priv->info.x = ((raw[2] & 0x0F) << 8) | raw[3];
> + priv->info.y = ((raw[4] & 0x0F) << 8) | raw[5];
> +
> + dev_dbg(priv->dev, "x: %d, y: %d, gesture: 0x%x\n",
> + priv->info.x, priv->info.y, priv->info.gesture);
> + } else {
> + dev_warn(priv->dev, "request was dropped\n");
Not a warn. First, it feels like really spamming the log, second,
drivers should be moderately quiet.
> + }
> +
> + return rc;
> +}
> +
> +static int cst816x_register_input(struct cst816x_priv *priv)
> +{
> + int rc;
> +
> + priv->input = devm_input_allocate_device(priv->dev);
> + if (!priv->input) {
> + rc = -ENOMEM;
> + dev_err(priv->dev, "input device alloc err: %d\n", rc);
Memory allocation errors are *never* printk'ed by drivers.
> + goto err;
> + }
...
> +static int cst816x_resume(struct device *dev)
> +{
> + struct cst816x_priv *priv = i2c_get_clientdata(to_i2c_client(dev));
> + int rc;
> +
> + cst816x_reset(priv);
> + rc = cst816x_regs_setup(priv);
> + if (!rc)
> + enable_irq(priv->irq);
> +
> + return rc;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(cst816x_pm_ops, cst816x_suspend, cst816x_resume);
> +
> +static int cst816x_probe(struct i2c_client *client)
> +{
> + struct cst816x_priv *priv;
> + struct device *dev = &client->dev;
> + int rc;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + rc = -ENOMEM;
No, just return.
> + dev_err(dev, "devm alloc failed: %d\n", rc);
No, drop. This is some ancient, downstream code. Do you see anything
like this anywhere else?
> + goto err;
> + }
> +
> + INIT_DELAYED_WORK(&priv->dw, cst816x_dw_cb);
> + timer_setup(&priv->timer, cst816x_timer_cb, 0);
> +
> + priv->dev = dev;
> + priv->client = client;
> +
> + priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (priv->reset == NULL) {
> + rc = -EIO;
Syntax is return dev_err_probe().
Same everywhere else.
Best regards,
Krzysztof
