[PATCH] Input: novatek-nvt-ts - support being panel follower

[PATCH] Input: novatek-nvt-ts - support being panel follower

[Gianluca Boiano]

From: Alexey Minnekhanov <alexeymin@minlexx.ru>

Sometimes Novatek touchscreen is paired together with Novatek panel,
and they both need to be powered together in sync.

Add support for nvt-ts driver to operate in panel follower mode, in
which touchscreen isn't in full control of its lifecycle. This is
managed using callbacks from panel, to get informed when the panel is
powered on and off. From there we can match the nvt-ts device's power
state to that of the panel.

Without this probing touchscreen before panel sometimes randomly fails
with I2C read errors.

Split some code from probe function to a separate
initial_power_on_and_register_inputdev function, which is called once
from panel_prepared callback, or from probe function, depending on
mode.

No Kconfig dependency on DRM is needed: drm_panel.h provides inline
stubs when CONFIG_DRM_PANEL is off, so the driver remains usable as a
standalone I2C touchscreen without DRM.

In panel-follower mode, nvt_ts_stop() can be called twice for a single
nvt_ts_start(): first from on_novatek_panel_unpreparing() when the
panel powers off, and again from input_close_device() when userspace
(e.g. systemd) closes the evdev file descriptor. The symmetric problem
exists for nvt_ts_start() on panel re-prepare. Guard both with a
'running' flag to make them idempotent. The flag is safe without
additional locking as all call paths already hold input->mutex.

Signed-off-by: Alexey Minnekhanov <alexeymin@minlexx.ru>
Co-developed-by: Gianluca Boiano <morf3089@gmail.com>
Signed-off-by: Gianluca Boiano <morf3089@gmail.com>
---
 drivers/input/touchscreen/novatek-nvt-ts.c | 122 +++++++++++++++++----
 1 file changed, 103 insertions(+), 19 deletions(-)

diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
index 0f771f681952..8b290f75a8e4 100644
--- a/drivers/input/touchscreen/novatek-nvt-ts.c
+++ b/drivers/input/touchscreen/novatek-nvt-ts.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
  */
 
+#include <drm/drm_panel.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
@@ -59,6 +60,13 @@ struct nvt_ts_data {
 	struct touchscreen_properties prop;
 	int max_touches;
 	u8 buf[NVT_TS_TOUCH_SIZE * NVT_TS_MAX_TOUCHES];
+	/*
+	 * Sometimes Novatek touchscreen is paired together with Novatek panel,
+	 * and they need to be powered together in sync.
+	 */
+	struct drm_panel_follower panel_follower;
+	bool is_panel_follower;
+	bool running;
 };
 
 static int nvt_ts_read_data(struct i2c_client *client, u8 reg, u8 *data, int count)
@@ -95,6 +103,9 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id)
 	bool active;
 	u8 *touch;
 
+	if (!data->input)
+		return IRQ_HANDLED;
+
 	error = nvt_ts_read_data(data->client, NVT_TS_TOUCH_START, data->buf,
 				 data->max_touches * NVT_TS_TOUCH_SIZE);
 	if (error)
@@ -145,6 +156,9 @@ static int nvt_ts_start(struct input_dev *dev)
 	struct nvt_ts_data *data = input_get_drvdata(dev);
 	int error;
 
+	if (data->running)
+		return 0;
+
 	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);
 	if (error) {
 		dev_err(&data->client->dev, "failed to enable regulators\n");
@@ -154,6 +168,7 @@ static int nvt_ts_start(struct input_dev *dev)
 	enable_irq(data->client->irq);
 	gpiod_set_value_cansleep(data->reset_gpio, 0);
 
+	data->running = true;
 	return 0;
 }
 
@@ -161,15 +176,23 @@ static void nvt_ts_stop(struct input_dev *dev)
 {
 	struct nvt_ts_data *data = input_get_drvdata(dev);
 
+	if (!data->running)
+		return;
+
 	disable_irq(data->client->irq);
 	gpiod_set_value_cansleep(data->reset_gpio, 1);
 	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+
+	data->running = false;
 }
 
 static int nvt_ts_suspend(struct device *dev)
 {
 	struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
 
+	if (data->is_panel_follower)
+		return 0;
+
 	guard(mutex)(&data->input->mutex);
 
 	if (input_device_enabled(data->input))
@@ -182,6 +205,9 @@ static int nvt_ts_resume(struct device *dev)
 {
 	struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
 
+	if (data->is_panel_follower)
+		return 0;
+
 	guard(mutex)(&data->input->mutex);
 
 	if (input_device_enabled(data->input))
@@ -192,30 +218,17 @@ static int nvt_ts_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(nvt_ts_pm_ops, nvt_ts_suspend, nvt_ts_resume);
 
-static int nvt_ts_probe(struct i2c_client *client)
+static int nvt_ts_initial_power_on_and_register_inputdev(struct nvt_ts_data *data)
 {
-	struct device *dev = &client->dev;
+	struct device *dev = &data->client->dev;
 	int error, width, height, irq_type;
-	struct nvt_ts_data *data;
 	const struct nvt_ts_i2c_chip_data *chip;
 	struct input_dev *input;
 
-	if (!client->irq) {
-		dev_err(dev, "Error no irq specified\n");
-		return -EINVAL;
-	}
-
-	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	chip = device_get_match_data(&client->dev);
+	chip = device_get_match_data(dev);
 	if (!chip)
 		return -EINVAL;
 
-	data->client = client;
-	i2c_set_clientdata(client, data);
-
 	/*
 	 * VCC is the analog voltage supply
 	 * IOVCC is the digital voltage supply
@@ -275,7 +288,7 @@ static int nvt_ts_probe(struct i2c_client *client)
 	if (!input)
 		return -ENOMEM;
 
-	input->name = client->name;
+	input->name = data->client->name;
 	input->id.bustype = BUS_I2C;
 	input->open = nvt_ts_start;
 	input->close = nvt_ts_stop;
@@ -292,10 +305,11 @@ static int nvt_ts_probe(struct i2c_client *client)
 	data->input = input;
 	input_set_drvdata(input, data);
 
-	error = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
+	error = devm_request_threaded_irq(dev, data->client->irq, NULL,
+					  nvt_ts_irq,
 					  IRQF_ONESHOT | IRQF_NO_AUTOEN |
 						nvt_ts_irq_type[irq_type],
-					  client->name, data);
+					  data->client->name, data);
 	if (error) {
 		dev_err(dev, "failed to request irq: %d\n", error);
 		return error;
@@ -310,6 +324,76 @@ static int nvt_ts_probe(struct i2c_client *client)
 	return 0;
 }
 
+static int on_novatek_panel_prepared(struct drm_panel_follower *follower)
+{
+	struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
+	int ret;
+
+	/* Is this the first power on? */
+	if (!data->input) {
+		ret = nvt_ts_initial_power_on_and_register_inputdev(data);
+		if (ret)
+			return ret;
+	}
+
+	guard(mutex)(&data->input->mutex);
+
+	if (input_device_enabled(data->input))
+		nvt_ts_start(data->input);
+
+	return 0;
+}
+
+static int on_novatek_panel_unpreparing(struct drm_panel_follower *follower)
+{
+	struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
+
+	guard(mutex)(&data->input->mutex);
+
+	if (input_device_enabled(data->input))
+		nvt_ts_stop(data->input);
+
+	return 0;
+}
+
+static const struct drm_panel_follower_funcs nvt_ts_follower_funcs = {
+	.panel_prepared = on_novatek_panel_prepared,
+	.panel_unpreparing = on_novatek_panel_unpreparing,
+};
+
+static int nvt_ts_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct nvt_ts_data *data;
+
+	if (!client->irq) {
+		dev_err(dev, "Error no irq specified\n");
+		return -EINVAL;
+	}
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	i2c_set_clientdata(client, data);
+
+	/* Check if "panel = <&...>" is set in DT */
+	if (drm_is_panel_follower(dev)) {
+		data->is_panel_follower = true;
+		data->panel_follower.funcs = &nvt_ts_follower_funcs;
+		/*
+		 * In this mode, we defer everything to the panel callbacks.
+		 * The initial power on and input device registration will
+		 * happen in on_novatek_panel_prepared().
+		 */
+		return devm_drm_panel_add_follower(dev, &data->panel_follower);
+	}
+
+	return nvt_ts_initial_power_on_and_register_inputdev(data);
+}
+
+
 static const struct nvt_ts_i2c_chip_data nvt_nt11205_ts_data = {
 	.chip_id = 0x05,
 };
-- 
2.55.0

Re: [PATCH] Input: novatek-nvt-ts - support being panel follower

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional dereference of `data->input` in `on_novatek_panel_unpreparing` causes a kernel crash if initialization failed.
- [High] Devres ordering inversion causes a Use-After-Free of the input device and invalid IRQ access during driver unbind.
- [High] Late `devm_*` allocations in a retryable panel callback cause permanent `-EBUSY` errors on initialization retries.
- [High] Independent userspace `open()` defeats the panel follower power sequencing, unconditionally powering the touchscreen.
- [Medium] Premature assignment of `data->input` without rollback on error leaves a dangling pointer that breaks initialization retries.
--

--- Patch [1]: [PATCH] Input: novatek-nvt-ts - support being panel follower ---
commit 13c26acf74fd7e860a0a8efe083b34d9bd7e6937
Author: Alexey Minnekhanov <alexeymin@minlexx.ru>

    Input: novatek-nvt-ts - support being panel follower

This commit adds support for the novatek-nvt-ts driver to operate in panel
follower mode, where the touchscreen and panel are powered together in sync.
It splits the input device registration out of the probe function, deferring
it until the panel prepares via callbacks.

> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
> index 0f771f6819528..8b290f75a8e48 100644
> --- a/drivers/input/touchscreen/novatek-nvt-ts.c
> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c

[ ... ]

> @@ -145,6 +156,9 @@ static int nvt_ts_start(struct input_dev *dev)
>  	struct nvt_ts_data *data = input_get_drvdata(dev);
>  	int error;
>  
> +	if (data->running)
> +		return 0;
> +
>  	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);

[Severity: High]
Does this bypass the panel follower power sequencing if userspace opens the
device independently?

Once the panel is prepared and the input device is registered, userspace can
open the input node (e.g. /dev/input/eventX) even if the panel has since been
powered off.

When that happens, input_dev->open() calls nvt_ts_start(), which will
unconditionally enable the regulators and IRQ without checking if
data->is_panel_follower restricts power control.

Should this check if the panel is currently prepared before forcing the
hardware on?

[ ... ]

> @@ -292,10 +305,11 @@ static int nvt_ts_probe(struct i2c_client *client)
>  	data->input = input;
>  	input_set_drvdata(input, data);
>  
> -	error = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
> +	error = devm_request_threaded_irq(dev, data->client->irq, NULL,
> +					  nvt_ts_irq,
>  					  IRQF_ONESHOT | IRQF_NO_AUTOEN |
>  						nvt_ts_irq_type[irq_type],
> -					  client->name, data);
> +					  data->client->name, data);
>  	if (error) {
>  		dev_err(dev, "failed to request irq: %d\n", error);
>  		return error;
>  	}

[Severity: Medium]
Does an error here leave data->input as a dangling pointer?

Because data->input = input was already assigned above, if
devm_request_threaded_irq() or input_register_device() fails, the function
returns an error but leaves data->input populated.

When the panel retries initialization in on_novatek_panel_prepared(), it checks
if (!data->input). Since it's no longer NULL, it will skip initialization and
attempt to interact with an unregistered or broken input device.

Should data->input be set back to NULL in these error paths, or assigned
only after everything succeeds?

[ ... ]

> @@ -310,6 +324,76 @@ static int nvt_ts_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int on_novatek_panel_prepared(struct drm_panel_follower *follower)
> +{
> +	struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
> +	int ret;
> +
> +	/* Is this the first power on? */
> +	if (!data->input) {
> +		ret = nvt_ts_initial_power_on_and_register_inputdev(data);
> +		if (ret)
> +			return ret;

[Severity: High]
Does calling devm allocation functions from a retryable callback cause permanent
errors on failure?

If nvt_ts_initial_power_on_and_register_inputdev() fails partway through
(e.g., an I2C timeout), it will return an error, but any devm resources it
already acquired (like devm_gpiod_get) will remain attached to the device.

When the panel power-on is retried later, it will execute again and attempt
to acquire the same GPIO and regulators, which will fail with -EBUSY.

Should these devm allocations be moved back to the probe function, or cleaned
up manually on error?

> +	}
> +
> +	guard(mutex)(&data->input->mutex);
> +
> +	if (input_device_enabled(data->input))
> +		nvt_ts_start(data->input);
> +
> +	return 0;
> +}
> +
> +static int on_novatek_panel_unpreparing(struct drm_panel_follower *follower)
> +{
> +	struct nvt_ts_data *data = container_of(follower, struct nvt_ts_data, panel_follower);
> +
> +	guard(mutex)(&data->input->mutex);

[Severity: Critical]
Is it possible for data->input to be NULL here?

If nvt_ts_initial_power_on_and_register_inputdev() fails during
on_novatek_panel_prepared() (for instance, due to an I2C error), data->input
will remain NULL.

Since the follower is still registered, the DRM framework will eventually call
on_novatek_panel_unpreparing(), and this guard(mutex) will dereference the NULL
pointer.

Should this check if data->input is valid before attempting to lock its mutex?

[Severity: High]
Could the devm lifecycle ordering cause a use-after-free here during unbind?

When nvt_ts_probe() calls devm_drm_panel_add_follower(), it registers the
follower cleanup action. Later, on_novatek_panel_prepared() calls
nvt_ts_initial_power_on_and_register_inputdev(), which allocates the input
device via devm_input_allocate_device() and the IRQ via
devm_request_threaded_irq().

Because devres cleans up in LIFO order, on unbind:
1. The IRQ and input device are freed first.
2. The follower is removed, which invokes on_novatek_panel_unpreparing().

This means on_novatek_panel_unpreparing() will try to lock the mutex of an
already freed input device, and nvt_ts_stop() will attempt to disable an
already freed IRQ.

> +
> +	if (input_device_enabled(data->input))
> +		nvt_ts_stop(data->input);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718135316.785467-1-morf3089@gmail.com?part=1