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 <[email protected]>
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
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 <[email protected]>
>
> 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
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:
*val2 = 220000; /* 1.22 mV */
They seem really helpful to me.
regards,
dan carpenter
On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter 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.
I will try and divide my patches next time :)
> This patch was honestly pretty tricky to review.
I am sorry for that. Might be easy for IIO reviewers ;)
> 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:
>
> *val2 = 220000; /* 1.22 mV */
They are pretty obvious as you can see from the return statements
just below that which is :
return IIO_VAL_INT_PLUS_MICRO;
These comments are obvious because we know 'val1' will be responsible
for Integer part(1.0) and 'val2' for the Micro part(220000 * 10^-6 = 0.22).
Isn't it ?
Although I am new to IIO please correct if I'm wrong.
--
Thanks
Himanshu Jha
On Mon, 12 Feb 2018 17:57:31 +0300
Dan Carpenter <[email protected]> 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
>
On Tue, 13 Feb 2018 01:16:05 +0530
Himanshu Jha <[email protected]> wrote:
> On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter 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.
>
> I will try and divide my patches next time :)
>
> > This patch was honestly pretty tricky to review.
>
> I am sorry for that. Might be easy for IIO reviewers ;)
>
> > 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:
> >
> > *val2 = 220000; /* 1.22 mV */
>
> They are pretty obvious as you can see from the return statements
> just below that which is :
>
> return IIO_VAL_INT_PLUS_MICRO;
>
> These comments are obvious because we know 'val1' will be responsible
> for Integer part(1.0) and 'val2' for the Micro part(220000 * 10^-6 = 0.22).
> Isn't it ?
>
> Although I am new to IIO please correct if I'm wrong.
>
The oddity here is that the base units (here mV) are inconsistent due
to some ancient attempts to align with other subsystems.
Dan is perhaps correct in that the comment might be helpful for that reason.
If so I would prefer to see it made clear what is going on.
/* Voltage base units are mV hence 1.22 mV */
Also should definitely have it's own line rather than being associated
with just the val2 line like it currently is.
Jonathan