lkml.org 
[lkml]   [2012]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 00/14 v3] cleanup atmel_mxt_ts
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-05-05 14:21    [W:0.034 / U:0.808 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site