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

[Krzysztof Kozlowski <krzk@kernel.org>]

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