On Sat, May 5, 2012 at 8:16 PM, Henrik Rydberg [off-list ref] wrote:

Hi Daniel,

quoted

Thank you to Henrik for reviewing again, and ACK'ing patch 3.

Reading it again, I do have some more comments, actually.

quoted

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.

quoted

quoted

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?

quoted

quoted

 Input: atmel_mxt_ts - only allow root to update firmware

OK.

quoted

quoted

 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.

quoted

quoted

 Input: atmel_mxt_ts - verify object size in mxt_write_object

OK, also stable material.

What does "stable material" mean?

quoted

quoted

 Input: atmel_mxt_ts - do not read extra (checksum) byte

OK.

quoted

quoted

 Input: atmel_mxt_ts - dump each message on just 1 line

OK.

quoted

quoted

 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?

quoted

quoted

 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.

quoted

quoted

 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?

quoted

quoted

 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.

quoted

quoted

 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

quoted

quoted

 Input: atmel_mxt_ts - parse vector field of data packets

These could be deferred until they are actually used.

quoted

quoted

 Input: atmel_mxt_ts - send all MT-B slots in one input report

OK.

quoted

quoted

 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html