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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 12 Feb 2018 17:57:31 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:
> > 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 ?  
> 
> If you're changing a line of code and you fix a white space issue on
> that same line, then that's fine.  If it's just in the same function,
> then do it in a separate patch.  In other words, adding tabs when you're
> moving around macros is fine, but adding it to the arguments is
> unrelated.
> 
> This patch was honestly pretty tricky to review.
> 
> Jonathan assumes reviewers have the datasheet in front of them and I
> assume that that they don't.  He's probably right...  But especially
> comments like this:

Actually I don't.  I like the code to be very clear without the datasheet.

That is one of the reasons I always advocate making it very clear what
is a register and what is a field. The _REG postfix is useful to my mind
for that reason.

What I really don't like is needing comments to tell you what a register
is for when the name of the define should make it clear.  Obviously
there are sometimes places you can't do this because the meaning cannot
be explained in a short enough name but they are fairly rare.

I agree it is a trade off on whether the naming is sufficiently clear
or not and your example of the power supply one is a classic.
That register has a stupid name on the datasheet given how easy
it would have been to make it clear in the name choice that it was
measuring the power supply.

The naming things _OUT on this datasheet is particularly nasty as
it adds nothing other than confusion.  However, the question arises
on whether it makes sense to get rid of that in the driver and
make it harder to read with the datasheet.


> 
> 		*val2 = 220000; /* 1.22 mV */
> 
> They seem really helpful to me.
This isn't about the data sheet, it is about knowledge of IIO.

That one is perhaps debatable as the base units for voltage are
less than ideal (I really wish I had been a stickler for SI units
throughout from the first - this came about through trying to maintain
compatibility with hwmon which with hind sight was a bad idea).

> 
> regards,
> dan carpenter
>