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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 1 Jun 2024 16:07:01 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Joel Selvaraj <joelselvaraj.oss@...il.com>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>
Cc: linux-input@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v3 3/3] Input: novatek-nvt-ts: add support for NT36672A
 touchscreen

Hi,

On 6/1/24 2:10 AM, Joel Selvaraj wrote:
> 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@...il.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.

Sounds good.

It is great to see people working on getting mainline
kernels to work on phones like the work you are doing here.
Thank you for doing this work!

Regards,

Hans