lkml.org 
[lkml]   [2018]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
    On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote:
    > Remove some unnecessary comments by giving appropriate name to macros.
    > Therefore, add _REG suffix for control registers. Also, align function
    > arguments to match open parentheses using tabs.
    > Group the control register and register field macros together.
    >
    > Blank line before some returns improves code readability.
    >
    > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>

    The most obvious response is that this needs to be broken up into
    multiple patches.

    I think I liked almost all the comments...

    > -/* Output, power supply */
    > -#define ADIS16201_SUPPLY_OUT 0x02
    > +#define ADIS16201_SUPPLY_OUT_REG 0x02

    To me it seems useful to know that we're talking about the power supply.

    Adding _REG to the name seems not terribly useful and it makes the name
    longer so we're going to run into the 80 character limit. I just
    checked and this patch does add some checkpatch warnings.

    But aligning the arguments is fine, deleting unused macros is fine,
    adding blank lines is fine.

    > static int adis16201_probe(struct spi_device *spi)
    > {
    > - int ret;
    > - struct adis *st;
    > struct iio_dev *indio_dev;
    > + struct adis *st;
    > + int ret;

    Making this reverse Christmas tree is fine. But these things should all
    be done in separate patches.

    regards,
    dan carpenter

    \
     
     \ /
      Last update: 2018-02-12 13:58    [W:4.143 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site