Received-SPF: pass (google.com: domain of linux-kernel+bounces-197613-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <a2f68c56-e6d6-4626-8d05-b5e808da60da@gmail.com>
Date: Fri, 31 May 2024 19:10:20 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 3/3] Input: novatek-nvt-ts: add support for NT36672A
 touchscreen
To: Hans de Goede <hdegoede@redhat.com>,
 Dmitry Torokhov <dmitry.torokhov@gmail.com>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
 <conor+dt@kernel.org>, =?UTF-8?Q?Ilpo_J=C3=A4rvinen?=
 <ilpo.jarvinen@linux.intel.com>
Cc: linux-input@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org
References: <20240526-nvt-ts-devicetree-regulator-support-v3-0-aa88d10ccd9a@gmail.com>
 <20240526-nvt-ts-devicetree-regulator-support-v3-3-aa88d10ccd9a@gmail.com>
 <55272a3b-575d-4212-a40b-7245beed5d80@redhat.com>
Content-Language: en-US
From: Joel Selvaraj <joelselvaraj.oss@gmail.com>
In-Reply-To: <55272a3b-575d-4212-a40b-7245beed5d80@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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@gmail.com>
>>
>> ---
>>   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