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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Wei-Shih Lin <frank101417@gmail.com>,
	dmitry.torokhov@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org
Cc: linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] Input: Add driver for Novatek NT519XX series touchscreen devices
Date: Sat, 28 Oct 2023 10:52:18 +0200	[thread overview]
Message-ID: <c66bb1fe-152f-492f-ada6-274162515c58@linaro.org> (raw)
In-Reply-To: <20231025082054.1190-3-Weishih_Lin@novatek.com.tw>

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