[PATCH 0/3] iwlwifi: cosmetic fixes

[PATCH 0/3] iwlwifi: cosmetic fixes

[Christoph Böhmwalder]

Fix several code style issues, some of which were reported by checkpatch.pl.

The changes are:
* One instance of an `int` variable being used in a boolean context, chaned to
  use the more appropriate `bool` type.
* One very minor fix, removing a newline between a function definition and its
  associated `static` keyword
* One fix wrapping a macro in curly braces


Christoph Böhmwalder (3):
  wireless: iwlwifi: use bool instead of int
  wireless: iwlwifi: function definition cosmetic fix
  wireless: iwlwifi: wrap macro into braces

 drivers/net/wireless/intel/iwlwifi/iwl-io.c     |  2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 16 +++++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)

-- 
2.13.5

[PATCH 1/3] wireless: iwlwifi: use bool instead of int

[Christoph Böhmwalder]

Change a usage of int in a boolean context to use the bool type instead, as it
makes the intent of the function clearer and helps clarify its semantics.

Also eliminate the if/else and just return the boolean result directly,
making the code more readable.

Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
---
 drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
index b7cd813ba70f..0eb815ae97e8 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
@@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db *phy_db,
 }
 IWL_EXPORT_SYMBOL(iwl_phy_db_set_section);
 
-static int is_valid_channel(u16 ch_id)
+static bool is_valid_channel(u16 ch_id)
 {
-	if (ch_id <= 14 ||
-	    (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
-	    (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
-	    (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1))
-		return 1;
-	return 0;
+	return (ch_id <= 14 ||
+	       (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
+	       (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
+	       (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1));
 }
 
 static u8 ch_id_to_ch_index(u16 ch_id)
-- 
2.13.5

Re: [PATCH 1/3] wireless: iwlwifi: use bool instead of int

[Luciano Coelho]

On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote:
> Change a usage of int in a boolean context to use the bool type
> instead, as it
> makes the intent of the function clearer and helps clarify its
> semantics.
> 
> Also eliminate the if/else and just return the boolean result
> directly,
> making the code more readable.
> 
> Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
> ---
>  drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> index b7cd813ba70f..0eb815ae97e8 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db
> *phy_db,
>  }
>  IWL_EXPORT_SYMBOL(iwl_phy_db_set_section);
>  
> -static int is_valid_channel(u16 ch_id)
> +static bool is_valid_channel(u16 ch_id)
>  {
> -	if (ch_id <= 14 ||
> -	    (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
> -	    (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
> -	    (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1))
> -		return 1;
> -	return 0;
> +	return (ch_id <= 14 ||
> +	       (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
> +	       (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
> +	       (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1));
>  }
>  
>  static u8 ch_id_to_ch_index(u16 ch_id)

This actually makes some sense, and I would probably apply it if it
were part of a patchset that actually does something useful.

--
Luca.

Re: [PATCH 1/3] wireless: iwlwifi: use bool instead of int

[Joe Perches]

On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote:
> Change a usage of int in a boolean context to use the bool type instead, as it
> makes the intent of the function clearer and helps clarify its semantics.
> 
> Also eliminate the if/else and just return the boolean result directly,
> making the code more readable.
> 
> Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
> ---
>  drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> index b7cd813ba70f..0eb815ae97e8 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db *phy_db,
>  }
>  IWL_EXPORT_SYMBOL(iwl_phy_db_set_section);
>  
> -static int is_valid_channel(u16 ch_id)
> +static bool is_valid_channel(u16 ch_id)
>  {
> -	if (ch_id <= 14 ||
> -	    (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
> -	    (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
> -	    (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1))
> -		return 1;
> -	return 0;
> +	return (ch_id <= 14 ||
> +	       (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
> +	       (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
> +	       (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1));
>  }

This might be more intelligble as separate tests

static bool is_valid_channel(u16 ch_id)
{
	if (ch_id <= 14)
		return true;

	if ((ch_id % 4 == 0) &&
	    ((ch_id >= 36 && ch_id <= 64) ||
	     (ch_id >= 100 && ch_id <= 140)))
		return true;

	if ((ch_id % 4 == 1) &&
	    (chid >= 145 && ch_id <= 165))
		return true;

	return false;
}

The compiler should produce the same object code.

Re: [PATCH 1/3] wireless: iwlwifi: use bool instead of int

[Luciano Coelho]

On Wed, 2017-10-04 at 09:26 -0700, Joe Perches wrote:
> On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote:
> > Change a usage of int in a boolean context to use the bool type
> > instead, as it
> > makes the intent of the function clearer and helps clarify its
> > semantics.
> > 
> > Also eliminate the if/else and just return the boolean result
> > directly,
> > making the code more readable.
> > 
> > Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
> > ---
> >  drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> > b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> > index b7cd813ba70f..0eb815ae97e8 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> > @@ -267,14 +267,12 @@ int iwl_phy_db_set_section(struct iwl_phy_db
> > *phy_db,
> >  }
> >  IWL_EXPORT_SYMBOL(iwl_phy_db_set_section);
> >  
> > -static int is_valid_channel(u16 ch_id)
> > +static bool is_valid_channel(u16 ch_id)
> >  {
> > -	if (ch_id <= 14 ||
> > -	    (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
> > -	    (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
> > -	    (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1))
> > -		return 1;
> > -	return 0;
> > +	return (ch_id <= 14 ||
> > +	       (36 <= ch_id && ch_id <= 64 && ch_id % 4 == 0) ||
> > +	       (100 <= ch_id && ch_id <= 140 && ch_id % 4 == 0) ||
> > +	       (145 <= ch_id && ch_id <= 165 && ch_id % 4 == 1));
> >  }
> 
> This might be more intelligble as separate tests
> 
> static bool is_valid_channel(u16 ch_id)
> {
> 	if (ch_id <= 14)
> 		return true;
> 
> 	if ((ch_id % 4 == 0) &&
> 	    ((ch_id >= 36 && ch_id <= 64) ||
> 	     (ch_id >= 100 && ch_id <= 140)))
> 		return true;
> 
> 	if ((ch_id % 4 == 1) &&
> 	    (chid >= 145 && ch_id <= 165))
> 		return true;
> 
> 	return false;
> }
> 
> The compiler should produce the same object code.

Yeah, it may be a bit easier to read, but I don't want to start getting
"fixes" to working and reasonable code.  There's nothing wrong with the
existing function (except maybe for the int vs. boolean) so let's not
change it.

A good time to change this would be the next time someone adds yet
another range of valid channels here. ;)

--
Luca.

Re: [PATCH 1/3] wireless: iwlwifi: use bool instead of int

[Joe Perches]

On Wed, 2017-10-04 at 19:39 +0300, Luciano Coelho wrote:
> On Wed, 2017-10-04 at 09:26 -0700, Joe Perches wrote:
[]
> > This might be more intelligble as separate tests
> > 
> > static bool is_valid_channel(u16 ch_id)
> > {
> > 	if (ch_id <= 14)
> > 		return true;
> > 
> > 	if ((ch_id % 4 == 0) &&
> > 	    ((ch_id >= 36 && ch_id <= 64) ||
> > 	     (ch_id >= 100 && ch_id <= 140)))
> > 		return true;
> > 
> > 	if ((ch_id % 4 == 1) &&
> > 	    (chid >= 145 && ch_id <= 165))
> > 		return true;
> > 
> > 	return false;
> > }
> > 
> > The compiler should produce the same object code.
> 
> Yeah, it may be a bit easier to read, but I don't want to start getting
> "fixes" to working and reasonable code.  There's nothing wrong with the
> existing function (except maybe for the int vs. boolean) so let's not
> change it.
> 
> A good time to change this would be the next time someone adds yet
> another range of valid channels here. ;)

<shrug>  Your choice.

I like code I can read and understand at a glance.

At case somebody needs to add channels, likely nobody
would do the change suggested but would just add
another test to the already odd looking block.

And constants should be on the right side of the tests.

Re: [PATCH 1/3] wireless: iwlwifi: use bool instead of int

[Luciano Coelho]

On Wed, 2017-10-04 at 10:55 -0700, Joe Perches wrote:
> On Wed, 2017-10-04 at 19:39 +0300, Luciano Coelho wrote:
> > On Wed, 2017-10-04 at 09:26 -0700, Joe Perches wrote:
> 
> []
> > > This might be more intelligble as separate tests
> > > 
> > > static bool is_valid_channel(u16 ch_id)
> > > {
> > > 	if (ch_id <= 14)
> > > 		return true;
> > > 
> > > 	if ((ch_id % 4 == 0) &&
> > > 	    ((ch_id >= 36 && ch_id <= 64) ||
> > > 	     (ch_id >= 100 && ch_id <= 140)))
> > > 		return true;
> > > 
> > > 	if ((ch_id % 4 == 1) &&
> > > 	    (chid >= 145 && ch_id <= 165))
> > > 		return true;
> > > 
> > > 	return false;
> > > }
> > > 
> > > The compiler should produce the same object code.
> > 
> > Yeah, it may be a bit easier to read, but I don't want to start
> > getting
> > "fixes" to working and reasonable code.  There's nothing wrong with
> > the
> > existing function (except maybe for the int vs. boolean) so let's
> > not
> > change it.
> > 
> > A good time to change this would be the next time someone adds yet
> > another range of valid channels here. ;)
> 
> <shrug>  Your choice.
> 
> I like code I can read and understand at a glance.

I do too, but I don't think the original is that hard to read, really. 
Each "if" you add is already corresponding to one separate line in the
original code...


> At case somebody needs to add channels, likely nobody
> would do the change suggested but would just add
> another test to the already odd looking block.

Yeah, that would most likely be the case, but if I saw that and thought
there was a better way to write it, believe me, I would definitely
nitpick the patch and ask the author to reorg the code so it would look
nicer.


> And constants should be on the right side of the tests.

Sure, in a new patch, we would definitely pay attention to that.  But
now, is it worth having one more patch go through the entire machinery
to change a relatively clear, extremely simple function just because
you could write it in a different way? My answer is a resounding no,
sorry.

--
Luca.

[PATCH 2/3] wireless: iwlwifi: function definition cosmetic fix

[Christoph Böhmwalder]

Separate the function from the previous definition with a newline and
put the `static` keyword on the same line, as it just looks nicer.

Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
---
 drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
index 0eb815ae97e8..249ee1c7b02f 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
@@ -325,8 +325,8 @@ static u16 channel_id_to_txp(struct iwl_phy_db *phy_db, u16 ch_id)
 	}
 	return 0xff;
 }
-static
-int iwl_phy_db_get_section_data(struct iwl_phy_db *phy_db,
+
+static int iwl_phy_db_get_section_data(struct iwl_phy_db *phy_db,
 				u32 type, u8 **data, u16 *size, u16 ch_id)
 {
 	struct iwl_phy_db_entry *entry;
-- 
2.13.5

Re: [PATCH 2/3] wireless: iwlwifi: function definition cosmetic fix

[Luciano Coelho]

On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote:
> Separate the function from the previous definition with a newline and
> put the `static` keyword on the same line, as it just looks nicer.
> 
> Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
> ---
>  drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> index 0eb815ae97e8..249ee1c7b02f 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c
> @@ -325,8 +325,8 @@ static u16 channel_id_to_txp(struct iwl_phy_db *phy_db, u16 ch_id)
>  	}
>  	return 0xff;
>  }
> -static
> -int iwl_phy_db_get_section_data(struct iwl_phy_db *phy_db,
> +
> +static int iwl_phy_db_get_section_data(struct iwl_phy_db *phy_db,
>  				u32 type, u8 **data, u16 *size, u16 ch_id)
>  {
>  	struct iwl_phy_db_entry *entry;

Sorry, but this now looks much uglier because the second line is not
even aligned to the parenthesis.

NACK.

--
Luca.

[PATCH 3/3] wireless: iwlwifi: wrap macro into braces

[Christoph Böhmwalder]

Macros should always be wrapped in braces, so fix this instance.

Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
---
 drivers/net/wireless/intel/iwlwifi/iwl-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-io.c b/drivers/net/wireless/intel/iwlwifi/iwl-io.c
index efb1998dcabd..0211963b3e71 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-io.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-io.c
@@ -252,7 +252,7 @@ IWL_EXPORT_SYMBOL(iwl_force_nmi);
 
 static const char *get_rfh_string(int cmd)
 {
-#define IWL_CMD(x) case x: return #x
+#define IWL_CMD(x) { case x: return #x; }
 #define IWL_CMD_MQ(arg, reg, q) { if (arg == reg(q)) return #reg; }
 
 	int i;
-- 
2.13.5

Re: [PATCH 3/3] wireless: iwlwifi: wrap macro into braces

[Johannes Berg]

Please don't send obviously broken patches.

johannes

Re: [PATCH 3/3] wireless: iwlwifi: wrap macro into braces

[Joe Perches]

On Wed, 2017-10-04 at 17:57 +0200, Christoph Böhmwalder wrote:
> Macros should always be wrapped in braces, so fix this instance.
> 
> Signed-off-by: Christoph Böhmwalder <christoph@boehmwalder.at>
> ---
>  drivers/net/wireless/intel/iwlwifi/iwl-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-io.c b/drivers/net/wireless/intel/iwlwifi/iwl-io.c
> index efb1998dcabd..0211963b3e71 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-io.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-io.c
> @@ -252,7 +252,7 @@ IWL_EXPORT_SYMBOL(iwl_force_nmi);
>  
>  static const char *get_rfh_string(int cmd)
>  {
> -#define IWL_CMD(x) case x: return #x
> +#define IWL_CMD(x) { case x: return #x; }

I think this unnecessary.  Maybe:
http://lists.infradead.org/pipermail/ath10k/2017-February/009335.html

>  #define IWL_CMD_MQ(arg, reg, q) { if (arg == reg(q)) return #reg; }

But this should use do { ... } while (0)

Re: [PATCH 0/3] iwlwifi: cosmetic fixes

[Luciano Coelho]

On Wed, 2017-10-04 at 17:56 +0200, Christoph Böhmwalder wrote:
> Fix several code style issues, some of which were reported by
> checkpatch.pl.
> 
> The changes are:
> * One instance of an `int` variable being used in a boolean context,
> chaned to
>   use the more appropriate `bool` type.
> * One very minor fix, removing a newline between a function
> definition and its
>   associated `static` keyword
> * One fix wrapping a macro in curly braces
> 
> 
> Christoph Böhmwalder (3):
>   wireless: iwlwifi: use bool instead of int
>   wireless: iwlwifi: function definition cosmetic fix
>   wireless: iwlwifi: wrap macro into braces
> 
>  drivers/net/wireless/intel/iwlwifi/iwl-io.c     |  2 +-
>  drivers/net/wireless/intel/iwlwifi/iwl-phy-db.c | 16 +++++++------
> ---
>  2 files changed, 8 insertions(+), 10 deletions(-)

Sorry, but this kind of series just generates churn.  Especially when 2
out of 3 patches are broken.  I applied your previous patch because it
was really trivial, but I really don't want to encourage this kind of
drive-by "fixes" that only cause additional work.

I generally only accept this kind of changes when people are changing
code close or related to it.

--
Luca.