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

[Joel Selvaraj <joelselvaraj.oss@gmail.com>]

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