On Wed, Apr 18, 2012 at 9:21 PM, Daniel Kurtz <[email protected]> wrote:
>
> This patchset cleans up the atmel_mxt_ts touchscreen driver.
> In particular, v3 implements the following:
>
> 1) sysfs
> ? a) fw_update only by root
> ? b) use sncprintf()
> 2) faster initialization
> ? a) read/write sets of registers using i2c block transactions
> ? b) fetch object descriptor table as a single i2c transaction
> ? c) fetch objects ?as a set of i2c reads, one per object
> ? d) write config data as a set of i2c writes, one per object
> 3) faster interrupt processing & initialization times
> ? a) cache important values at init instead of computing in isr
> ? b) don't read message checksum byte (which isn't even enabled in fw)
> 4) more correct MT-B support
> ? a) send all (changed) contacts in a single EV_SYN/SYN_REPORT
>
> v3:
> ?* Address Henrik's feedback:
> ? ?1) removed some more stack allocations
> ? ?2) use sncprintf() in sysfs handler
>
>
> The patches were tested using an MXT224E.
> They should apply cleanly to input/next.
>
> Daniel Kurtz (14):
> ?Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
> ?Input: atmel_mxt_ts - only allow root to update firmware
> ?Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
> ?Input: atmel_mxt_ts - verify object size in mxt_write_object
> ?Input: atmel_mxt_ts - do not read extra (checksum) byte
> ?Input: atmel_mxt_ts - dump each message on just 1 line
> ?Input: atmel_mxt_ts - refactor mxt_object_show
> ?Input: atmel_mxt_ts - optimize writing of object table entries
> ?Input: atmel_mxt_ts - refactor get info
> ?Input: atmel_mxt_ts - simplify event reporting
> ?Input: atmel_mxt_ts - cache T9 reportid range when reading object
> ? ?table
> ?Input: atmel_mxt_ts - parse vector field of data packets
> ?Input: atmel_mxt_ts - send all MT-B slots in one input report
> ?Input: atmel_mxt_ts - parse T6 reports
Thank you to Henrik for reviewing again, and ACK'ing patch 3.
Could I get a review for the rest of the set?
There will actually be quite a few more patches that follow these.
-Daniel
Hi Daniel,
> Thank you to Henrik for reviewing again, and ACK'ing patch 3.
Reading it again, I do have some more comments, actually.
> Could I get a review for the rest of the set?
> There will actually be quite a few more patches that follow these.
I think that is part of the problem. What you want to achieve is all
good, but something else is not quite right. Reading through these
patches felt like a lot of work, although it should not really be that
much. A closer look suggests the patches are on average 20% too large,
the rest being irrelevant changes. That may look small, but apparently
it is off-putting enough. The less work it is to accept your patches,
the more likely they are to be processed quickly.
Please find brief notes below.
> > Daniel Kurtz (14):
> > ?Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
Seems to clash with current mainline.
> > ?Input: atmel_mxt_ts - only allow root to update firmware
OK.
> > ?Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
The return value change should be split out in a separate patch,
subject to stable as well. Also, there is no real benefit in changing
the name from __mxt to mxt. It only makes the patch longer.
> > ?Input: atmel_mxt_ts - verify object size in mxt_write_object
OK, also stable material.
> > ?Input: atmel_mxt_ts - do not read extra (checksum) byte
OK.
> > ?Input: atmel_mxt_ts - dump each message on just 1 line
OK.
> > ?Input: atmel_mxt_ts - refactor mxt_object_show
Start of for loop does not need to change. The buf_end - buf is less
clear than the existing PAGE_SIZE - count. The realloc feels clunky,
could it not allocate the max size once instead?
> > ?Input: atmel_mxt_ts - optimize writing of object table entries
Seems the index variable could be kept, no real need to move the bject
deklaration around, small things like that.
> > ?Input: atmel_mxt_ts - refactor get info
Why not keep mxt_get_info(), just using the smaller implementation?
Why change the formatting of the debug messages?
> > ?Input: atmel_mxt_ts - simplify event reporting
Why change formatting of function, why reformat status initialization,
why new name for pressure, why change the shift functions, why change
the debug message.
> > ?Input: atmel_mxt_ts - cache T9 reportid range when reading object
> > ? ?table
Why change touchevent() function name and arguments, why not reuse the
reportid variables. Why reformat the object assignment. Aren't
T9_reportid values zero already.
> > ?Input: atmel_mxt_ts - parse vector field of data packets
These could be deferred until they are actually used.
> > ?Input: atmel_mxt_ts - send all MT-B slots in one input report
OK.
> > ?Input: atmel_mxt_ts - parse T6 reports
Aren't T6_reportid values zero already.
Hope this helps.
Thanks.
Henrik
On Sat, May 5, 2012 at 8:16 PM, Henrik Rydberg <[email protected]> wrote:
> Hi Daniel,
>
>> Thank you to Henrik for reviewing again, and ACK'ing patch 3.
>
> Reading it again, I do have some more comments, actually.
>
>> Could I get a review for the rest of the set?
>> There will actually be quite a few more patches that follow these.
>
> I think that is part of the problem. ?What you want to achieve is all
> good, but something else is not quite right. ?Reading through these
> patches felt like a lot of work, although it should not really be that
> much. A closer look suggests the patches are on average 20% too large,
> the rest being irrelevant changes. That may look small, but apparently
> it is off-putting enough. The less work it is to accept your patches,
> the more likely they are to be processed quickly.
>
> Please find brief notes below.
Hi Henrik,
Once again, thanks for all of your time and effort reviewing these patches.
I'll send a smaller friendlier set soon, but first, some brief notes
on your notes, below.
>
>> > Daniel Kurtz (14):
>> > ?Input: atmel_mxt_ts - use CONFIG_PM_SLEEP
>
> Seems to clash with current mainline.
I don't understand this comment. What is clashing with what, exactly?
>
>> > ?Input: atmel_mxt_ts - only allow root to update firmware
>
> OK.
>
>> > ?Input: atmel_mxt_ts - refactor mxt_read/write_reg to take a length
>
> The return value change should be split out in a separate patch,
> subject to stable as well. Also, there is no real benefit in changing
> the name from __mxt to mxt. It only makes the patch longer.
>
>> > ?Input: atmel_mxt_ts - verify object size in mxt_write_object
>
> OK, also stable material.
What does "stable material" mean?
>
>> > ?Input: atmel_mxt_ts - do not read extra (checksum) byte
>
> OK.
>
>> > ?Input: atmel_mxt_ts - dump each message on just 1 line
>
> OK.
>
>> > ?Input: atmel_mxt_ts - refactor mxt_object_show
>
> Start of for loop does not need to change. The buf_end - buf is less
> clear than the existing PAGE_SIZE - count. ?The realloc feels clunky,
> could it not allocate the max size once instead?
>
>> > ?Input: atmel_mxt_ts - optimize writing of object table entries
>
> Seems the index variable could be kept, no real need to move the bject
> deklaration around, small things like that.
>
>> > ?Input: atmel_mxt_ts - refactor get info
>
> Why not keep mxt_get_info(), just using the smaller implementation?
> Why change the formatting of the debug messages?
>
>> > ?Input: atmel_mxt_ts - simplify event reporting
>
> Why change formatting of function, why reformat status initialization,
> why new name for pressure, why change the shift functions, why change
> the debug message.
>
>> > ?Input: atmel_mxt_ts - cache T9 reportid range when reading object
>> > ? ?table
>
> Why change touchevent() function name and arguments, why not reuse the
> reportid variables. Why reformat the object assignment. Aren't
> T9_reportid values zero already.
It is true that the reportid values are zero init'ed on driver probe().
However, mxt_get_object_table() gets called both at probe, and after a
firmware update.
Thus, these values are zero'ed here to clear out the old values that
might be present from the old firmware.
In particular, the old values would linger on if, for example, there
was an error while reading the object table, or the new object table
doesn't have a T9 object.
That is too subtle, I guess, though, and the zero'ing could be moved
to the firmware update code to make it more clear why.
-Dan
>
>> > ?Input: atmel_mxt_ts - parse vector field of data packets
>
> These could be deferred until they are actually used.
>
>> > ?Input: atmel_mxt_ts - send all MT-B slots in one input report
>
> OK.
>
>> > ?Input: atmel_mxt_ts - parse T6 reports
>
> Aren't T6_reportid values zero already.
>
> Hope this helps.
>
> Thanks.
> Henrik
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
Hi Henrik,
On Sat, May 05, 2012 at 02:16:26PM +0200, Henrik Rydberg wrote:
>
> > > ?Input: atmel_mxt_ts - verify object size in mxt_write_object
>
> OK, also stable material.
No, I don't think we need it in stable as it is not exposed to
unprivileged userspace and we did not have reports of it causing
issues...
Daniel,
I applied patches 1, 2, 4, 5, and 6. The rest seem to need more
discussion.
Thanks.
--
Dmitry