Re: [PATCH v3 3/3] Input: novatek-nvt-ts: add support for NT36672A touchscreen

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans de Goede,

On 5/27/24 03:42, Hans de Goede wrote:
Hi Joel,

On 5/27/24 5:26 AM, Joel Selvaraj via B4 Relay wrote:
From: Joel Selvaraj <joelselvaraj.oss@xxxxxxxxx>

---
  drivers/input/touchscreen/novatek-nvt-ts.c | 78 +++++++++++++++++++++++++++---
  1 file changed, 72 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
index 224fd112b25a9..7a82a1b09f9d5 100644
--- a/drivers/input/touchscreen/novatek-nvt-ts.c
+++ b/drivers/input/touchscreen/novatek-nvt-ts.c
@@ -139,9 +143,23 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id)
  	return IRQ_HANDLED;
  }
+static void nvt_ts_disable_regulators(void *_data)
+{
+	struct nvt_ts_data *data = _data;
+
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
  static int nvt_ts_start(struct input_dev *dev)
  {
  	struct nvt_ts_data *data = input_get_drvdata(dev);
+	int error;
+
+	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);
+	if (error) {
+		dev_err(&data->client->dev, "failed to enable regulators\n");
+		return error;
+	}

This is weird, you already enable the regulators in probe() and
those get disabled again on remove() by the devm action you add.

So there is no need to enable / disable the regulators on start/stop .

If you want the regulators to only be enabled when the touchscreen
is on then you should disable the regulators again in probe()
after the nvt_ts_read_data() call there (and drop the devm action).

Yes, I want the regulators to be enabled only when the touchscreen is on/active. I will disable the regulators in probe and remove the devm action in v4.

@@ -277,8 +324,26 @@ static int nvt_ts_probe(struct i2c_client *client)
  	return 0;
  }
+static const struct nvt_ts_i2c_chip_data nvt_nt11205_ts_data = {
+	.wake_type = 0x05,
+	.chip_id = 0x05,
+};
+
+static const struct nvt_ts_i2c_chip_data nvt_nt36672a_ts_data = {
+	.wake_type = 0x01,
+	.chip_id = 0x08,
+};
+
+static const struct of_device_id nvt_ts_of_match[] = {
+	{ .compatible = "novatek,nt11205-ts", .data = &nvt_nt11205_ts_data },
+	{ .compatible = "novatek,nt36672a-ts", .data = &nvt_nt36672a_ts_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nvt_ts_of_match);
+
  static const struct i2c_device_id nvt_ts_i2c_id[] = {
-	{ "NT11205-ts" },
+	{ "NT11205-ts", (unsigned long) &nvt_nt11205_ts_data },
+	{ "NT36672A-ts", (unsigned long) &nvt_nt36672a_ts_data },

The i2c-subsystem will also match of compatible strings to i2c_device_ids
by looking at the partof the compatible after the ',', so for a compatible
of e.g. "novatek,nt36672a-ts" will match an i2c_device_id of "nt36672a-ts".

So if you change these to lower-case:

	{ "nt11205-ts", (unsigned long) &nvt_nt11205_ts_data },
	{ "nt36672a-ts", (unsigned long) &nvt_nt36672a_ts_data },

Then you can drop the nvt_ts_of_match table since that is not necessary
then.

Hmm I just realized that this will break module auto-loading though since that
does require of modaliases .
So maybe this is not such a good idea after all. Still switching to lowercase
i2c_device_id-s would be good for consistency and you need to respin
the patch-set for the regulator issue anyways.

Ok. I will change it to lowercase i2c device id in v4.

Regards,
Joel Selvaraj




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux