On Mon, 12 Feb 2018 17:57:31 +0300 Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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 > -- 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
