[RFC 0/4] Input: edt-ft5506 - Adding support for FT5506

[RFC 0/4] Input: edt-ft5506 - Adding support for FT5506

[fcooper]

From: Franklin S Cooper Jr <fcooper@ti.com>

This series adds support for the FT5506 tsc. The biggest difference
between this tsc vs the ones currently supported by the driver is the
ability to handle upto 10 touch points.

The FT5506 tsc that I currently have seems to be based on the M09
firmware which I have documentation for. However, I haven't been able
to find documentation for the tsc that use the M06 firmware. Therefore,
the calculation I made to determine the amount of data to read for the M06
is at best an educated guess.

If anyone has the doc for the old firmware please send it to me if
possible or double check the code I use to determine the amount of bytes
to read.

Also I had to work around a weird bug that I'm only seeing on the FT5506.
I tested this patch set on another board (AM437x SK) that uses the FT5306
and didn't see any problems.

This patchset is built on top of linux-input master

Franklin S Cooper Jr (4):
  Input: edt-ft5x06 - Use max support points to determine how much to
    read
  Input: edt-ft5x06 - Add support for different max support points
  Input: edt-ft5x06 - Add support for FT5506
  Input: edt-ft5x06 - Work around FT5506 firmware bug

 .../bindings/input/touchscreen/edt-ft5x06.txt      |  2 +
 drivers/input/touchscreen/edt-ft5x06.c             | 69 ++++++++++++++++++----
 2 files changed, 59 insertions(+), 12 deletions(-)

-- 
2.6.1

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

[fcooper]

From: Franklin S Cooper Jr <fcooper@ti.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@ti.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;
 
 	default:
-- 
2.6.1

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

[Dmitry Torokhov]

On Wed, Oct 07, 2015 at 07:21:38AM -0500, fcooper@ti.com wrote:
> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.

I also wonder why we need extra 1 byte in M06 case.

Lothar?

Thanks.

-- 
Dmitry

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

[Franklin S Cooper Jr.]

On 10/14/2015 06:39 PM, Dmitry Torokhov wrote:
> On Wed, Oct 07, 2015 at 07:21:38AM -0500, fcooper@ti.com wrote:
>> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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).
We need +1 simply for the fact that we read the register at location
cmd.

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.
>

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

[Dmitry Torokhov]

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@ti.com wrote:
> >> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.

> 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.

Can we unify the calculation to be:

	datalen = tplen * MAX_SUPPORT_POINTS + offset + crc_len;

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.

-- 
Dmitry

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

[Franklin S Cooper Jr.]

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@ti.com wrote:
>>>> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.

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.
>

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

[Franklin S Cooper Jr.]

On 10/15/2015 07:43 PM, Franklin S Cooper Jr. 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@ti.com wrote:
>>>>> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.
>
> 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?
Forgot to add. I'm using the M09 version.
>>
>>> 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.
>>

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

[Dmitry Torokhov]

On Thu, Oct 15, 2015 at 5:43 PM, Franklin S Cooper Jr. <fcooper@ti.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@ti.com wrote:
>>>>> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.

>
> 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.

-- 
Dmitry

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

[Franklin S Cooper Jr.]

On 10/15/2015 07:47 PM, Dmitry Torokhov wrote:
> On Thu, Oct 15, 2015 at 5:43 PM, Franklin S Cooper Jr. <fcooper@ti.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@ti.com wrote:
>>>>>> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.
>

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

[Dmitry Torokhov]

On October 15, 2015 5:54:14 PM PDT, "Franklin S Cooper Jr." <fcooper@ti.com> wrote:
>
>
>
>On 10/15/2015 07:47 PM, Dmitry Torokhov wrote:
>> On Thu, Oct 15, 2015 at 5:43 PM, Franklin S Cooper Jr.
><fcooper@ti.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@ti.com wrote:
>>>>>>> From: Franklin S Cooper Jr <fcooper@ti.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@ti.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.

No, I think I'm good with the direction you are going. Please resubmit and I should be able to apply it.


Thanks.

-- 
Dmitry

[RFC 2/4] Input: edt-ft5x06 - Add support for different max support points

[fcooper]

From: Franklin S Cooper Jr <fcooper@ti.com>

Update the code so that the maximum supported points aren't hard coded but
can be changed.

Set the maximum support points based on the data passed along side the
compatible field.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 56 ++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 1e0ed6e..8031152 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -38,8 +38,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
-
-#define MAX_SUPPORT_POINTS		5
+#include <linux/of_device.h>
 
 #define WORK_REGISTER_THRESHOLD		0x00
 #define WORK_REGISTER_REPORT_RATE	0x08
@@ -105,6 +104,7 @@ struct edt_ft5x06_ts_data {
 	int gain;
 	int offset;
 	int report_rate;
+	int max_support_points;
 
 	char name[EDT_NAME_LEN];
 
@@ -112,6 +112,10 @@ struct edt_ft5x06_ts_data {
 	enum edt_ver version;
 };
 
+struct edt_i2c_chip_data {
+	int  max_support_points;
+};
+
 static int edt_ft5x06_ts_readwrite(struct i2c_client *client,
 				   u16 wr_len, u8 *wr_buf,
 				   u16 rd_len, u8 *rd_buf)
@@ -180,14 +184,14 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 		tplen = 4;  /* data comes in so called frames */
 
 		/* how many bytes to listen for */
-		datalen = tplen * MAX_SUPPORT_POINTS + offset + 1;
+		datalen = tplen * tsdata->max_support_points + offset + 1;
 		break;
 
 	case M09:
 		cmd = 0x02;
 		offset = 1;
 		tplen = 6;
-		datalen = tplen * MAX_SUPPORT_POINTS - cmd + 1;
+		datalen = tplen * tsdata->max_support_points - cmd + 1;
 		break;
 
 	default:
@@ -219,7 +223,7 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 			goto out;
 	}
 
-	for (i = 0; i < MAX_SUPPORT_POINTS; i++) {
+	for (i = 0; i < tsdata->max_support_points; i++) {
 		u8 *buf = &rdbuf[i * tplen + offset];
 		bool down;
 
@@ -872,6 +876,32 @@ edt_ft5x06_ts_set_regs(struct edt_ft5x06_ts_data *tsdata)
 	}
 }
 
+#ifdef CONFIG_OF
+
+static const struct of_device_id edt_ft5x06_of_match[];
+
+static void edt_ft5x06_ts_set_max_support_points(struct device *dev,
+					struct edt_ft5x06_ts_data *tsdata)
+{
+	struct edt_i2c_chip_data *chip_data;
+	const struct of_device_id *match;
+
+	match = of_match_device(of_match_ptr(edt_ft5x06_of_match), dev);
+
+	if (match) {
+		chip_data = (struct edt_i2c_chip_data *)match->data;
+		tsdata->max_support_points = chip_data->max_support_points;
+	} else {
+		tsdata->max_support_points = 5;
+	}
+}
+#else
+static void edt_ft5x06_ts_set_max_support_points(struct device *dev,
+					struct edt_ft5x06_ts_data *tsdata)
+{
+}
+#endif
+
 static int edt_ft5x06_ts_probe(struct i2c_client *client,
 					 const struct i2c_device_id *id)
 {
@@ -889,6 +919,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
+	edt_ft5x06_ts_set_max_support_points(&client->dev, tsdata);
+
 	tsdata->reset_gpio = devm_gpiod_get_optional(&client->dev,
 						     "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(tsdata->reset_gpio)) {
@@ -954,7 +986,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 
 	touchscreen_parse_properties(input, true);
 
-	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
+	error = input_mt_init_slots(input, tsdata->max_support_points,
+				INPUT_MT_DIRECT);
 	if (error) {
 		dev_err(&client->dev, "Unable to init MT slots.\n");
 		return error;
@@ -1039,10 +1072,15 @@ static const struct i2c_device_id edt_ft5x06_ts_id[] = {
 MODULE_DEVICE_TABLE(i2c, edt_ft5x06_ts_id);
 
 #ifdef CONFIG_OF
+
+static const struct edt_i2c_chip_data edt_ft5x06_data = {
+	.max_support_points = 5,
+};
+
 static const struct of_device_id edt_ft5x06_of_match[] = {
-	{ .compatible = "edt,edt-ft5206", },
-	{ .compatible = "edt,edt-ft5306", },
-	{ .compatible = "edt,edt-ft5406", },
+	{ .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data},
+	{ .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data},
+	{ .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, edt_ft5x06_of_match);
-- 
2.6.1

[RFC 3/4] Input: edt-ft5x06 - Add support for FT5506

[fcooper]

From: Franklin S Cooper Jr <fcooper@ti.com>

FT5506 is essentially the same as other FT5x06 devices other than
supporting 10 support points.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt | 2 ++
 drivers/input/touchscreen/edt-ft5x06.c                             | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index bedd7dd..f99528d 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -5,6 +5,7 @@ There are 3 variants of the chip for various touch panel sizes
 FT5206GE1  2.8" .. 3.8"
 FT5306DE4  4.3" .. 7"
 FT5406EE8  7"   .. 8.9"
+FT5506EEG  7"   .. 8.9"
 
 The software interface is identical for all those chips, so that
 currently there is no need for the driver to distinguish between the
@@ -17,6 +18,7 @@ Required properties:
  - compatible:  "edt,edt-ft5206"
            or:  "edt,edt-ft5306"
            or:  "edt,edt-ft5406"
+           or:  "edt,edt-ft5506"
 
  - reg:         I2C slave address of the chip (0x38)
  - interrupt-parent: a phandle pointing to the interrupt controller
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 8031152..752da69 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -172,7 +172,7 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 	struct edt_ft5x06_ts_data *tsdata = dev_id;
 	struct device *dev = &tsdata->client->dev;
 	u8 cmd;
-	u8 rdbuf[29];
+	u8 rdbuf[59];
 	int i, type, x, y, id;
 	int offset, tplen, datalen;
 	int error;
@@ -1073,6 +1073,10 @@ MODULE_DEVICE_TABLE(i2c, edt_ft5x06_ts_id);
 
 #ifdef CONFIG_OF
 
+static const struct edt_i2c_chip_data edt_ft5506_data = {
+	.max_support_points = 10,
+};
+
 static const struct edt_i2c_chip_data edt_ft5x06_data = {
 	.max_support_points = 5,
 };
@@ -1081,6 +1085,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
 	{ .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data},
 	{ .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data},
 	{ .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data},
+	{ .compatible = "edt,edt-ft5506", .data = &edt_ft5506_data},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, edt_ft5x06_of_match);
-- 
2.6.1

[RFC 4/4] Input: edt-ft5x06 - Work around FT5506 firmware bug

[fcooper]

From: Franklin S Cooper Jr <fcooper@ti.com>

In the touchscreen controller ISR, reading the tsc starting from
register 0x2 causes the tsc to infrequently update the detected
finger's x and y coordinate. The irq pin toggles at a fast rate to
indicate touch events are happening. However, the tsc on average
updates the touch point's x and y value every ~100 ms which is much
slower than the advertised rate of 100+ Hz. This leads to multiple reads
within this ~100 ms time window returning the same value.

Example:
X: 10 , Y: 30
X: 10 , Y: 30
X: 10,  Y: 30
..
// After 100 ms
X: 300, Y: 300
X: 300, y: 300
..
// After 100 ms
X: 1743, Y: 621
X: 1743, Y: 621

For some reason if instead of starting to read at register 0x2 you
start reading at register 0x0 this issue isn't seen. This seems like
a quirk only seen in the EDT FT5506 so to fix this issue simply
adjust the code to start reading from 0x0. Technically this isn't wrong
so no regressions should be seen with other touchscreen controllers
supported by this driver.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 752da69..b3ed631 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -172,7 +172,7 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 	struct edt_ft5x06_ts_data *tsdata = dev_id;
 	struct device *dev = &tsdata->client->dev;
 	u8 cmd;
-	u8 rdbuf[59];
+	u8 rdbuf[61];
 	int i, type, x, y, id;
 	int offset, tplen, datalen;
 	int error;
@@ -188,8 +188,8 @@ static irqreturn_t edt_ft5x06_ts_isr(int irq, void *dev_id)
 		break;
 
 	case M09:
-		cmd = 0x02;
-		offset = 1;
+		cmd = 0x0;
+		offset = 3;
 		tplen = 6;
 		datalen = tplen * tsdata->max_support_points - cmd + 1;
 		break;
-- 
2.6.1