linux-kernel - Re: [RFC 1/4] Input: edt-ft5x06 - Use max support points to determine how much to read

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]

Message-ID: <56204AB6.6080306@ti.com>
Date:	Thu, 15 Oct 2015 19:54:14 -0500
From:	"Franklin S Cooper Jr." <fcooper@...com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Lothar Waßmann <lw@...o-electronics.de>,
	Jingoo Han <jg1.han@...sung.com>,
	Asaf Vertz <asaf.vertz@...demg.com>
Subject: Re: [RFC 1/4] Input: edt-ft5x06 - Use max support points to determine
 how much to read




On 10/15/2015 07:47 PM, Dmitry Torokhov wrote:
> On Thu, Oct 15, 2015 at 5:43 PM, Franklin S Cooper Jr. <fcooper@...com> wrote:
>>
>> On 10/15/2015 07:16 PM, Dmitry Torokhov wrote:
>>> On Wed, Oct 14, 2015 at 08:58:32PM -0500, Franklin S Cooper Jr. wrote:
>>>> On 10/14/2015 06:39 PM, Dmitry Torokhov wrote:
>>>>> On Wed, Oct 07, 2015 at 07:21:38AM -0500, fcooper@...com wrote:
>>>>>> From: Franklin S Cooper Jr <fcooper@...com>
>>>>>>
>>>>>> Calculate the amount of data that needs to be read for the specified max
>>>>>> number of support points. If the maximum number of support points changes
>>>>>> then the amount that is read from the touch screen controller should
>>>>>> reflect this.
>>>>>>
>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@...com>
>>>>>> ---
>>>>>>  drivers/input/touchscreen/edt-ft5x06.c | 6 ++++--
>>>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>> index 7239c31..1e0ed6e 100644
>>>>>> --- a/drivers/input/touchscreen/edt-ft5x06.c
>>>>>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
>>>>>> @@ -178,14 +178,16 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
>>>>>>            cmd = 0xf9; /* tell the controller to send touch data */
>>>>>>            offset = 5; /* where the actual touch data starts */
>>>>>>            tplen = 4;  /* data comes in so called frames */
>>>>>> -          datalen = 26; /* how much bytes to listen for */
>>>>>> +
>>>>>> +          /* how many bytes to listen for */
>>>>>> +          datalen = tplen * MAX_SUPPORT_POINTS + offset + 1;
>>>>>>            break;
>>>>>>
>>>>>>    case M09:
>>>>>>            cmd = 0x02;
>>>>>>            offset = 1;
>>>>>>            tplen = 6;
>>>>>> -          datalen = 29;
>>>>>> +          datalen = tplen * MAX_SUPPORT_POINTS - cmd + 1;
>>>>>>            break;
>>>>> Hmm, why would formulae for datalen be different depending on the
>>>>> firmware? And I think original 29 it too low: we need 30 bytes for 5
>>>>> contacts + 1 to account for offset.
>>>> So based on the current ISR we don't care about the touch weight and
>>>> which are the last two registers for each touch point. So for the last
>>>> touchpoint we really don't need to read the extra two registers (-2).
>>> This is really not obvious. I do not think we'd see any performance
>>> degradation if we actually read the whole last touchpoint.
>> Yeah that shouldn't be a problem. I'll fix that.
>>>> We need +1 simply for the fact that we read the register at location
>>>> cmd.
>>> I am not sure I follow this. We do not reference anything past
>>> rdbuf[(MAX_SUPPORT_POINTS - 1) * tplen + offset] and
>>> our offset takes care of the start position, so why exactly we need the
>>> +1? Ah, CRC is in the extra byte.
>> Sorry your right the +1 isn't needed.
>>> Can we unify the calculation to be:
>>>
>>>       datalen = tplen * MAX_SUPPORT_POINTS + offset + crc_len;
>> Why do we need the crc_len? M06 is the only one that uses the CRC
>> and the offset insures we are reading the necessary crc registers.
> CRC is at buf[datalen - 1] position, so it is the last byte after last
> contact. That is why we have +1 for M06. For M09 crc_len will be 0.
Ok I get it now.

I can submit a v2 patchset with these changes. Are you ok with this patchset
as a whole other than these changes or should I give you more time to review
the rest before sending out a v2 and remove the RFC.

>
>> Unless I'm missing something it would simply be:
>>
>> datalen = tplen * MAX_SUPPORT_POINTS + offset
>>
>>> By the way, what version of firmware you tested your changes with?
>>>
>>>> So 6 * 5 - 2 + 1 which is how we get to 29. The formula looks slightly
>>>> different because the registers we are reading are very close to zero
>>>> so the math works out to equal the equation I used for M09.
>>>>
>>>> M06 since tplen = 4 then all four registers are used in the ISR per touch
>>>> point. Plus the offset and plus 1 again to account for the fact we are reading
>>>> the cmd register. But once again it would be nice if someone can confirm this.
>>>>> I also wonder why we need extra 1 byte in M06 case.
>>>>>
>>>>> Lothar?
>>>>>
>>>>> Thanks.
>>>>>
>>> Thanks.
>>>
> Thanks.
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/