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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
