linux-kernel - Re: [PATCH 2/2] Input: Add driver for Novatek NT519XX series touchscreen devices

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c66bb1fe-152f-492f-ada6-274162515c58@linaro.org>
Date:   Sat, 28 Oct 2023 10:52:18 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Wei-Shih Lin <frank101417@...il.com>, dmitry.torokhov@...il.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org
Cc:     linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Input: Add driver for Novatek NT519XX series
 touchscreen devices

On 25/10/2023 10:20, Wei-Shih Lin wrote:
> This patch adds support for Novatek NT519XX series touchscreen devices.
> Existing Novatek touchscreen driver code developed for Acer Iconia One 7
> B1-750 tablet with Novatek IC NT11205 is novatek-nvt-ts.c in the path
> drivers/input/touchscreen/. However, this patch supports touch features
> for automotive display with Novatek TDDI NT519XX.
> 


> +
> +static int32_t nvt_ts_resume(struct device *dev)
> +{
> +	if (bTouchIsAwake) {
> +		NVT_LOG("Touch is already resume.\n");
> +		return 0;
> +	}
> +
> +	mutex_lock(&ts->lock);
> +
> +	NVT_LOG("start\n");

Sorry, you cannot have such silly debugs.

> +
> +	nvt_bootloader_reset();
> +	nvt_check_fw_reset_state(RESET_STATE_NORMAL_RUN);
> +
> +	nvt_irq_enable(true);
> +
> +	bTouchIsAwake = 1;
> +
> +	mutex_unlock(&ts->lock);
> +
> +	NVT_LOG("end\n");



....

> +#endif
> +
> +static struct i2c_driver nvt_i2c_driver = {
> +	.probe		= nvt_ts_probe,
> +	.remove		= nvt_ts_remove,
> +	.shutdown	= nvt_ts_shutdown,
> +	.id_table	= nvt_ts_id,
> +	.driver = {
> +		.name	= NVT_I2C_NAME,
> +		.owner	= THIS_MODULE,

Drop

> +#ifdef CONFIG_OF

Nope, drop

> +		.of_match_table = nvt_match_table,
> +#endif
> +	},
> +};
> +
> +static int32_t __init nvt_driver_init(void)
> +{
> +	int32_t ret = 0;
> +
> +	NVT_LOG("start\n");

Drop entire init. Open existing code and use it as template.

> +
> +	bTouchIsAwake = 0;
> +
> +	ret = i2c_add_driver(&nvt_i2c_driver);
> +	if (ret) {
> +		NVT_ERR("Failed to add i2c driver!");
> +		goto err_driver;
> +	}
> +
> +	NVT_LOG("end\n");
> +
> +err_driver:
> +	return ret;
> +}
> +
> +static void __exit nvt_driver_exit(void)
> +{
> +	i2c_del_driver(&nvt_i2c_driver);
> +}
> +
> +module_init(nvt_driver_init);
> +module_exit(nvt_driver_exit);
> +
> +MODULE_DESCRIPTION("Novatek Touchscreen Driver");
> +MODULE_LICENSE("GPL");

This driver has very poor quality. Pointing issues here would be even
too much work. Please start from scratch from existing, accepted and
reviewed driver and customize it for your needs. You will notice that it
does not have all this weird code you put here.

Best regards,
Krzysztof