[PATCH] staging:iio:ade7854 surround complex defines in parentheses

[PATCH] staging:iio:ade7854 surround complex defines in parentheses

[daniel watson]

Error found by checkpatch.pl

Signed-off-by: daniel watson <ozzloy@challenge-bot.com>
---
 drivers/staging/iio/meter/ade7854.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index a51e6e3183d38..afb13e21002e1 100644
--- a/drivers/staging/iio/meter/ade7854.h
+++ b/drivers/staging/iio/meter/ade7854.h
@@ -139,9 +139,9 @@
 #define ADE7854_MAX_RX    7
 #define ADE7854_STARTUP_DELAY 1000
 
-#define ADE7854_SPI_SLOW	(u32)(300 * 1000)
-#define ADE7854_SPI_BURST	(u32)(1000 * 1000)
-#define ADE7854_SPI_FAST	(u32)(2000 * 1000)
+#define ADE7854_SPI_SLOW	((u32)(300 * 1000))
+#define ADE7854_SPI_BURST	((u32)(1000 * 1000))
+#define ADE7854_SPI_FAST	((u32)(2000 * 1000))
 
 /**
  * struct ade7854_state - device instance specific data
-- 
2.30.2

Re: [PATCH] staging:iio:ade7854 surround complex defines in parentheses

[Greg Kroah-Hartman]

On Sat, Aug 14, 2021 at 07:31:15PM -0700, daniel watson wrote:
> Error found by checkpatch.pl

What error?

> 
> Signed-off-by: daniel watson <ozzloy@challenge-bot.com>

Capitalize your name?

> ---
>  drivers/staging/iio/meter/ade7854.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
> index a51e6e3183d38..afb13e21002e1 100644
> --- a/drivers/staging/iio/meter/ade7854.h
> +++ b/drivers/staging/iio/meter/ade7854.h
> @@ -139,9 +139,9 @@
>  #define ADE7854_MAX_RX    7
>  #define ADE7854_STARTUP_DELAY 1000
>  
> -#define ADE7854_SPI_SLOW	(u32)(300 * 1000)
> -#define ADE7854_SPI_BURST	(u32)(1000 * 1000)
> -#define ADE7854_SPI_FAST	(u32)(2000 * 1000)
> +#define ADE7854_SPI_SLOW	((u32)(300 * 1000))
> +#define ADE7854_SPI_BURST	((u32)(1000 * 1000))
> +#define ADE7854_SPI_FAST	((u32)(2000 * 1000))

This is not a real change that is needed, just look at the code to
verify that.

thanks,

greg k-h

Re: [PATCH] staging:iio:ade7854 surround complex defines in parentheses

[daniel watson]

If this is a false positive from checkpatch, I can submit it as an
example to the checkpatch maintainers.  Do you think I should?

On Sun, Aug 15, 2021 at 08:25:51AM +0200, Greg Kroah-Hartman wrote:
> On Sat, Aug 14, 2021 at 07:31:15PM -0700, daniel watson wrote:
> > Error found by checkpatch.pl
> 
> What error?

$ git checkout 36a21d5172 drivers/staging/iio/meter/ade7854.h # before
Updated 1 path from 638ccd1543654

$ ./scripts/checkpatch.pl --terse --types COMPLEX_MACRO \
drivers/staging/iio/meter/ade7854.h
drivers/staging/iio/meter/ade7854.h:142: ERROR: Macros with complex
values should be enclosed in parentheses
drivers/staging/iio/meter/ade7854.h:143: ERROR: Macros with complex
values should be enclosed in parentheses
drivers/staging/iio/meter/ade7854.h:144: ERROR: Macros with complex
values should be enclosed in parentheses
total: 3 errors, 0 warnings, 0 checks, 173 lines checked

$ git checkout 143b51a80978 drivers/staging/iio/meter/ade7854.h # after
Updated 1 path from 21c208a36476a

$ ./scripts/checkpatch.pl --terse --types COMPLEX_MACRO \
drivers/staging/iio/meter/ade7854.h

$


> > Signed-off-by: daniel watson <ozzloy@challenge-bot.com>
> 
> Capitalize your name?

I can remake this patch with my name capitalized if the patch is worth
remaking.  I'll be sure to capitalize in future sign-off lines for
Linux.


> This is not a real change that is needed, just look at the code to
> verify that.

Agreed, this is not a huge change.

I thought small changes were acceptable, if they get rid of errors from
checkpatch.  I got that impression from this video

Write and Submit your first Linux kernel Patch
https://youtu.be/LLBrBBImJt4

At around 15 minutes, you create a patch which removes curly braces
from an if-else.  That seemed comparable to the change in this
patch.  That video was posted over a decade ago, so I would understand
if things are different now.

> 
> thanks,
> 
> greg k-h

You're welcome!  Thank you too, that was a super fast response!  I am
happy to get a direct response from you!

Re: [PATCH] staging:iio:ade7854 surround complex defines in parentheses

[Greg Kroah-Hartman]

On Mon, Aug 16, 2021 at 09:30:38PM -0700, daniel watson wrote:
> 
> If this is a false positive from checkpatch, I can submit it as an
> example to the checkpatch maintainers.  Do you think I should?

checkpatch is a perl script that does pattern matching, it is really
hard for it to determine for things like this specific example, that it
is not needed.

Try finding other valid checkpatch cleanups if you wish to contribute in
this way, there are loads of others under drivers/staging/ that should
be easy to find.

thanks,

greg k-h

Re: [PATCH] staging:iio:ade7854 surround complex defines in parentheses

[Joe Perches]

On Tue, 2021-08-17 at 08:12 +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 16, 2021 at 09:30:38PM -0700, daniel watson wrote:
> > 
> > If this is a false positive from checkpatch, I can submit it as an
> > example to the checkpatch maintainers.  Do you think I should?
> 
> checkpatch is a perl script that does pattern matching, it is really
> hard for it to determine for things like this specific example, that it
> is not needed.
> 
> Try finding other valid checkpatch cleanups if you wish to contribute in
> this way, there are loads of others under drivers/staging/ that should
> be easy to find.

These macros, and several other '#define ADE7854_<FOO> <value>' instances,
are apparently unused and could possibly be removed instead.

Re: [PATCH] staging:iio:ade7854 surround complex defines in parentheses

[Fabio M. De Francesco]

Hi Daniel,

Welcome to drivers/staging.

On Tuesday, August 17, 2021 6:30:38 AM CEST daniel watson wrote:
> If this is a false positive from checkpatch, I can submit it as an
> example to the checkpatch maintainers.  Do you think I should?
> 
> On Sun, Aug 15, 2021 at 08:25:51AM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Aug 14, 2021 at 07:31:15PM -0700, daniel watson wrote:
> > > Error found by checkpatch.pl
> > 
> > What error?

Please, understand that Greg K-H is very busy, so he often is quite terse in 
his responses. That does not absolutely mean he doesn't care.

You'll read from him questions like "Why did you do this?" or "What error?" or 
other similar ones. I think that while working here during the past four 
months I've become able to "decode" :) the meaning of the above questions (at 
least, I hope).

First of all you should read and follow the guidelines about how to submit 
patches: "Submitting patches: the essential guide to getting your code into 
the kernel" at https://www.kernel.org/doc/html/latest/process/submitting-patches.html

"What error?" may mean that you have not sufficiently described the underlying 
problem that motivated you to do this work. Don't assume that the reviewers 
can and want to deduce what the error is by just reading the "Subject" line.

The "Subject" should summarize in few words the content of the commit message, 
and the commit message should never be written with the assumption that the  
readers want to compose the whole picture by merging "Subject" and "Commit 
message" together: they have different purposes and they should be self-
contained entities.  "Error found by checkpatch.pl" does say nothing about 
what error you're addressing.

Furthermore, the above-mentioned question may also mean: "Are you sure that 
there are *real* errors/problems?". In this case they are just false 
positives.

An example of a real problem that should be worked out by adding parenthesis 
is the following one...

#define SQRT(x) 	(x * x)

What happens if some code uses that macro like the following example?

int a = 3;
int b = SQRT(a + 2)

Would the result differ if we put parentheses around arguments? 

#define SQRT(x) 	((x) * (x))

As Greg wrote in another message of this thread: "checkpatch is a perl script 
that does pattern matching, it is really hard for it to determine for things 
like this specific example, that it is not needed.".

The lesson is: we expect you're smarter than checkpatch.pl! :)

> $ git checkout 36a21d5172 drivers/staging/iio/meter/ade7854.h # before
> Updated 1 path from 638ccd1543654
> 
> $ ./scripts/checkpatch.pl --terse --types COMPLEX_MACRO \
> drivers/staging/iio/meter/ade7854.h
> drivers/staging/iio/meter/ade7854.h:142: ERROR: Macros with complex
> values should be enclosed in parentheses
> drivers/staging/iio/meter/ade7854.h:143: ERROR: Macros with complex
> values should be enclosed in parentheses
> drivers/staging/iio/meter/ade7854.h:144: ERROR: Macros with complex
> values should be enclosed in parentheses
> total: 3 errors, 0 warnings, 0 checks, 173 lines checked
> 
> $ git checkout 143b51a80978 drivers/staging/iio/meter/ade7854.h # after
> Updated 1 path from 21c208a36476a

OK, this is one of many common ways to explain what errors you're addressing. 
Unfortunately, in this case they are false positives (so you should have 
ignored them and move on to the next problem).
 
> $ ./scripts/checkpatch.pl --terse --types COMPLEX_MACRO \
> drivers/staging/iio/meter/ade7854.h
> 
> $
> 
> > > Signed-off-by: daniel watson <ozzloy@challenge-bot.com>
> > 
> > Capitalize your name?
> 
> I can remake this patch with my name capitalized if the patch is worth
> remaking.  I'll be sure to capitalize in future sign-off lines for
> Linux.
>
> > This is not a real change that is needed, just look at the code to
> > verify that.
> 
> Agreed, this is not a huge change.

Minor changes are very well welcome here. Move on to something else and fix 
only what really needs to be fixed. 

> I thought small changes were acceptable, if they get rid of errors from
> checkpatch.  I got that impression from this video
> 
> Write and Submit your first Linux kernel Patch
> https://youtu.be/LLBrBBImJt4
> 
> At around 15 minutes, you create a patch which removes curly braces
> from an if-else.  That seemed comparable to the change in this
> patch.  That video was posted over a decade ago, so I would understand
> if things are different now.

Read again the last sentence I wrote above. I can confirm that the removal of 
really unneeded curly braces is wanted and welcome. 

Have a nice time with kernel hacking.

Thanks,

Fabio
> > thanks,
> > 
> > greg k-h
> 
> You're welcome!  Thank you too, that was a super fast response!  I am
> happy to get a direct response from you!