linux-kernel - Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180212143522.GA12142@himanshu-Vostro-3559>
Date:   Mon, 12 Feb 2018 20:05:22 +0530
From:   Himanshu Jha <himanshujha199640@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     21cnbao@...il.com, jic23@...nel.org, devel@...verdev.osuosl.org,
        lars@...afoo.de, Michael.Hennerich@...log.com,
        linux-iio@...r.kernel.org, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, pmeerw@...erw.net, knaack.h@....de
Subject: Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and
 add suitable suffix

Hi Dan,

On Mon, Feb 12, 2018 at 03:53:12PM +0300, Dan Carpenter wrote:
> 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@...il.com>
> 
> The most obvious response is that this needs to be broken up into
> multiple patches.
> 
> I think I liked almost all the comments...

Please note that all the changes are suggested by Joanathan in his TODO
https://marc.info/?l=linux-iio&m=151775804702998&w=2
I think it far more commenting as compared to other drivers in the
mainline IIO drivers. I grouped all the relevant control registers
together at one place with suitable comment.
For eg:

+/* Data Output Register Definitions */

The macro names are identical to the names in the datasheet if you can
look and one could easily refer to the datasheet easily. Also, I
grouped the control registers and their fields together make it more readable.
For eg:

+#define ADIS16201_MSC_CTRL_REG                 0x34
+#define ADIS16201_MSC_CTRL_SELF_TEST_EN                BIT(8)
+#define ADIS16201_MSC_CTRL_DATA_RDY_EN         BIT(2)

> > -/* 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.

For that purpose I already grouped data output register definitions.

> 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.

_REG prefix is used to differentiate between registers addresses and the
fields in the register as pointed in the above MSC_CTRL_REG and
MSC_CTRL_SELF_TEST_EN.

Yes by doing this it triggered 2 I think 80 character warning. For eg:

	ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 12),

But you know with 81 characters so I neglected it because it looked more
readable without breaking into lines IMHO.

> 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.

I am sometimes confused in dividing the patches. As per your advice I
should make separate patch for :

1. remove unnecessary comments.
2. add suitable suffix.
3. add tabs instead of space.
4. reverse christmas tree.

But these should be done when we have *more* instances.

For eg: 
I added a tab space in function static int adis16201_read_raw() argument
to match open parentheses in this patch. But I also added tabs while
removing and adding suitable suffix to the macros. So, should it also be
done in a separate patch ? Since there was only one instance of adding tabs
therefore I did it here without framing another patch for that purpose
while saving my time to plan 'what to include/exclude in the patch ?'

Also, given the above queries I was clueless as what to do first! Since
sometimes it happens that the patch series doesn't apply cleanly because
of incorrect ordering for framing the patches.

Thanks for taking your time to review the patch series.

-- 
Thanks
Himanshu Jha