[PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[Krzysztof Wilczynski]

Move the static keyword to the front of declaration of
bh1750_chip_info_tbl, and resolve the following compiler
warning that can be seen when building with warnings
enabled (W=1):

drivers/iio/light/bh1750.c:64:1: warning:
  ‘static’ is not at beginning of declaration [-Wold-style-declaration]

Signed-off-by: Krzysztof Wilczynski <kw@linux.com>
---
Related: https://lore.kernel.org/r/20190827233017.GK9987@google.com

 drivers/iio/light/bh1750.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
index 28347df78cff..460c0013f1a9 100644
--- a/drivers/iio/light/bh1750.c
+++ b/drivers/iio/light/bh1750.c
@@ -42,7 +42,7 @@ struct bh1750_data {
 	u16 mtreg;
 };
 
-struct bh1750_chip_info {
+static const struct bh1750_chip_info {
 	u16 mtreg_min;
 	u16 mtreg_max;
 	u16 mtreg_default;
@@ -59,9 +59,7 @@ struct bh1750_chip_info {
 
 	u16 int_time_low_mask;
 	u16 int_time_high_mask;
-}
-
-static const bh1750_chip_info_tbl[] = {
+} bh1750_chip_info_tbl[] = {
 	[BH1710] = { 140, 1022, 300, 400,  250000000, 2, 0x001F, 0x03E0 },
 	[BH1721] = { 140, 1020, 300, 400,  250000000, 2, 0x0010, 0x03E0 },
 	[BH1750] = { 31,  254,  69,  1740, 57500000,  1, 0x001F, 0x00E0 },
-- 
2.22.1

Re: [PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[Tomasz Duszynski]

On Mon, Sep 02, 2019 at 01:31:32PM +0200, Krzysztof Wilczynski wrote:
> Move the static keyword to the front of declaration of
> bh1750_chip_info_tbl, and resolve the following compiler
> warning that can be seen when building with warnings
> enabled (W=1):

Looks okay.
Acked-by: Tomasz Duszynski <tduszyns@gmail.com>

>
> drivers/iio/light/bh1750.c:64:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
>
> Signed-off-by: Krzysztof Wilczynski <kw@linux.com>
> ---
> Related: https://lore.kernel.org/r/20190827233017.GK9987@google.com
>
>  drivers/iio/light/bh1750.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> index 28347df78cff..460c0013f1a9 100644
> --- a/drivers/iio/light/bh1750.c
> +++ b/drivers/iio/light/bh1750.c
> @@ -42,7 +42,7 @@ struct bh1750_data {
>  	u16 mtreg;
>  };
>
> -struct bh1750_chip_info {
> +static const struct bh1750_chip_info {
>  	u16 mtreg_min;
>  	u16 mtreg_max;
>  	u16 mtreg_default;
> @@ -59,9 +59,7 @@ struct bh1750_chip_info {
>
>  	u16 int_time_low_mask;
>  	u16 int_time_high_mask;
> -}
> -
> -static const bh1750_chip_info_tbl[] = {
> +} bh1750_chip_info_tbl[] = {
>  	[BH1710] = { 140, 1022, 300, 400,  250000000, 2, 0x001F, 0x03E0 },
>  	[BH1721] = { 140, 1020, 300, 400,  250000000, 2, 0x0010, 0x03E0 },
>  	[BH1750] = { 31,  254,  69,  1740, 57500000,  1, 0x001F, 0x00E0 },
> --
> 2.22.1
>

Re: [PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[Jonathan Cameron]

On Mon,  2 Sep 2019 13:31:32 +0200
Krzysztof Wilczynski <kw@linux.com> wrote:

> Move the static keyword to the front of declaration of
> bh1750_chip_info_tbl, and resolve the following compiler
> warning that can be seen when building with warnings
> enabled (W=1):
> 
> drivers/iio/light/bh1750.c:64:1: warning:
>   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
> 
> Signed-off-by: Krzysztof Wilczynski <kw@linux.com>

This one has me confused.  The warning seems to be false as static
is at the beginning of the declaration....

Sure we "could" combine the declaration with the definition as you have
done here, but that has nothing much to do with the warning.

What am I missing?

Jonathan

> ---
> Related: https://lore.kernel.org/r/20190827233017.GK9987@google.com
> 
>  drivers/iio/light/bh1750.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/bh1750.c b/drivers/iio/light/bh1750.c
> index 28347df78cff..460c0013f1a9 100644
> --- a/drivers/iio/light/bh1750.c
> +++ b/drivers/iio/light/bh1750.c
> @@ -42,7 +42,7 @@ struct bh1750_data {
>  	u16 mtreg;
>  };
>  
> -struct bh1750_chip_info {
> +static const struct bh1750_chip_info {
>  	u16 mtreg_min;
>  	u16 mtreg_max;
>  	u16 mtreg_default;
> @@ -59,9 +59,7 @@ struct bh1750_chip_info {
>  
>  	u16 int_time_low_mask;
>  	u16 int_time_high_mask;
> -}
> -
> -static const bh1750_chip_info_tbl[] = {
Looks like the beginning fo the declaration to me!

Jonathan

> +} bh1750_chip_info_tbl[] = {
>  	[BH1710] = { 140, 1022, 300, 400,  250000000, 2, 0x001F, 0x03E0 },
>  	[BH1721] = { 140, 1020, 300, 400,  250000000, 2, 0x0010, 0x03E0 },
>  	[BH1750] = { 31,  254,  69,  1740, 57500000,  1, 0x001F, 0x00E0 },

Re: [PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[Krzysztof Wilczynski]

Hello Jonathan,

Thank you for feedback.

[...]
> > drivers/iio/light/bh1750.c:64:1: warning:
> >   ‘static’ is not at beginning of declaration [-Wold-style-declaration]
[...]
> This one has me confused.  The warning seems to be false as static
> is at the beginning of the declaration....
> 
> Sure we "could" combine the declaration with the definition as you have
> done here, but that has nothing much to do with the warning.
[...]

I only moved the "static const" at the front, I haven't changed the
code as it's already has been a declaration and definition.  There is
no semicolon there and the original author put a newline to separate
things which makes it look as if these were separate.

Simple example based on the existing code:

  https://godbolt.org/z/hV4HP7

I hope this helps to illustrate the change in the patch.  I apologise
if my approach was incorrect.

As part of the patch I removed the newline in an aim to make it less
confusing to anyone who will read the code in the future.  Especially,
since it makes it a bit awkward to read and when using things like
grep.

Krzysztof

Re: [PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[Jonathan Cameron]

On Sun, 8 Sep 2019 15:52:09 +0200
Krzysztof Wilczynski <kw@linux.com> wrote:

> Hello Jonathan,
> 
> Thank you for feedback.
> 
> [...]
> > > drivers/iio/light/bh1750.c:64:1: warning:
> > >   ‘static’ is not at beginning of declaration [-Wold-style-declaration]  
> [...]
> > This one has me confused.  The warning seems to be false as static
> > is at the beginning of the declaration....
> > 
> > Sure we "could" combine the declaration with the definition as you have
> > done here, but that has nothing much to do with the warning.  
> [...]
> 
> I only moved the "static const" at the front, I haven't changed the
> code as it's already has been a declaration and definition.  There is
> no semicolon there and the original author put a newline to separate
> things which makes it look as if these were separate.
> 
> Simple example based on the existing code:
> 
>   https://godbolt.org/z/hV4HP7
> 
> I hope this helps to illustrate the change in the patch.  I apologise
> if my approach was incorrect.
> 
> As part of the patch I removed the newline in an aim to make it less
> confusing to anyone who will read the code in the future.  Especially,
> since it makes it a bit awkward to read and when using things like
> grep.
> 
> Krzysztof

I get what you are trying to do, the issue is the code is currently:

struct bh1750_chip_info {
	u16 mtreg_min;
	u16 mtreg_max;
	u16 mtreg_default;
	int mtreg_to_usec;
	int mtreg_to_scale;

	/*
	 * For BH1710/BH1721 all possible integration time values won't fit
	 * into one page so displaying is limited to every second one.
	 * Note, that user can still write proper values which were not
	 * listed.
	 */
	int inc;

	u16 int_time_low_mask;
	u16 int_time_high_mask;
}

static const bh1750_chip_info_tbl[] = {
	[BH1710] = { 140, 1022, 300, 400,  250000000, 2, 0x001F, 0x03E0 },
	[BH1721] = { 140, 1020, 300, 400,  250000000, 2, 0x0010, 0x03E0 },
	[BH1750] = { 31,  254,  69,  1740, 57500000,  1, 0x001F, 0x00E0 },
};

That test is supposed to catch the second block being

const static bh1750_chip_info_tbl[] = {
...

Which it isn't.  So the issue here was never that the static keyword
wasn't at the front of the declaration but that we could save a tiny
bit code by using the pattern

static const struct bh1750_chip_info {
...
} bh1750_chip_info_tbl[] {
	[...] = ...
};

We can do that of course, but that's nothing to do with moving the static
keyword to the front of the declaration which is what the patch claims
to be doing.

Jonathan

Re: [PATCH] iio: light: bh1750: Move static keyword to the front of declaration

[Krzysztof Wilczynski]

Hello Jonathan,

Thank you for the feedback.  I really appreciate it.

[...]
> We can do that of course, but that's nothing to do with moving the static
> keyword to the front of the declaration which is what the patch claims
> to be doing.

I see your point.  I am going to send a v2 that hopefully will be much
cleaner and better worded.  Thank you!

Krzysztof