linux-kernel - Re: [PATCH v5 2/2] input: zinitix: Add touchkey support

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] [day] [month] [year] [list]

Message-ID: <b0b86f2f6f315d791e7e0391142720b9@trvn.ru>
Date: Sat, 20 Jul 2024 11:57:44 +0500
From: Nikita Travkin <nikita@...n.ru>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Michael Srba <Michael.Srba@...nam.cz>, Linus Walleij
 <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, linux-input@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] input: zinitix: Add touchkey support

Dmitry Torokhov писал(а) 20.07.2024 05:29:
> On Wed, Jul 17, 2024 at 06:55:34PM +0500, Nikita Travkin wrote:
>> Zinitix touch controllers can use some of the sense lines for virtual
>> keys (like those found on many phones). Add support for those keys.
>>
>> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
>> Signed-off-by: Nikita Travkin <nikita@...n.ru>
> 
> Applied, thank you. However:
> 
>> -->
>> +	if (le16_to_cpu(touch_event.status) & BIT_ICON_EVENT) {
>> +		error = zinitix_read_data(bt541->client, ZINITIX_ICON_STATUS_REG,
>> +					  &icon_events, sizeof(icon_events));
>> +		if (error) {
>> +			dev_err(&client->dev, "Failed to read icon events\n");
>> +			goto out;
>> +		}
> 
> I wonder, would it make sense (and be more efficient) to issue a single
> read of size sizeof(struct touch_event) + sizeof(icon_events) and the
> parse the data based on touch_event.status?

Maybe, but I would be really hesitant to such a change: Original driver
also makes a dedicated read for the "icon" data and per my understanding,
those "register reads" may also not be really "register" based but rather
kind of "command" based, where controller will start streaming the data
based on the request for the specific "register". In this case i'd prefer
to not accidentally confuse the touch firmware by over-reading the data,
if its somehow firmware-version-defined.

Thanks for giving it a look and picking this up!
Nikita

> 
> Thanks.