[PATCH] staging: vt6656: Refactor the vnt_ofdm_min_rate function

[PATCH] staging: vt6656: Refactor the vnt_ofdm_min_rate function

[Oscar Carter]

Replace the for loop by a ternary operator whose condition is an AND
bitmask against the priv->basic_rates variable.

The purpose of the for loop was to check if any of bits from RATE_54M to
RATE_6M was set, but it's not necessary to check every individual bit.
The same result can be achieved using only one single mask which
comprises all the commented bits.

This way avoid the iteration over an unnecessary for loop.

Also change the return type to bool because it's the type that this
function returns.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/card.c | 11 ++---------
 drivers/staging/vt6656/card.h |  2 +-
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
index 9bd37e57c727..adaf93cf653a 100644
--- a/drivers/staging/vt6656/card.c
+++ b/drivers/staging/vt6656/card.c
@@ -248,16 +248,9 @@ void vnt_update_top_rates(struct vnt_private *priv)
 	priv->top_cck_basic_rate = top_cck;
 }

-int vnt_ofdm_min_rate(struct vnt_private *priv)
+bool vnt_ofdm_min_rate(struct vnt_private *priv)
 {
-	int ii;
-
-	for (ii = RATE_54M; ii >= RATE_6M; ii--) {
-		if ((priv->basic_rates) & ((u16)BIT(ii)))
-			return true;
-	}
-
-	return false;
+	return priv->basic_rates & GENMASK(RATE_54M, RATE_6M) ? true : false;
 }

 u8 vnt_get_pkt_type(struct vnt_private *priv)
diff --git a/drivers/staging/vt6656/card.h b/drivers/staging/vt6656/card.h
index 75cd340c0cce..eaa15d0c291a 100644
--- a/drivers/staging/vt6656/card.h
+++ b/drivers/staging/vt6656/card.h
@@ -29,7 +29,7 @@ void vnt_set_channel(struct vnt_private *priv, u32 connection_channel);
 void vnt_set_rspinf(struct vnt_private *priv, u8 bb_type);
 void vnt_update_ifs(struct vnt_private *priv);
 void vnt_update_top_rates(struct vnt_private *priv);
-int vnt_ofdm_min_rate(struct vnt_private *priv);
+bool vnt_ofdm_min_rate(struct vnt_private *priv);
 void vnt_adjust_tsf(struct vnt_private *priv, u8 rx_rate,
 		    u64 time_stamp, u64 local_tsf);
 bool vnt_get_current_tsf(struct vnt_private *priv, u64 *current_tsf);
--
2.20.1

Re: [PATCH] staging: vt6656: Refactor the vnt_ofdm_min_rate function

[Stefano Brivio]

Hi Oscar,

On Sat, 18 Apr 2020 15:45:53 +0200
Oscar Carter <oscar.carter@gmx.com> wrote:

> Replace the for loop by a ternary operator whose condition is an AND
> bitmask against the priv->basic_rates variable.
> 
> The purpose of the for loop was to check if any of bits from RATE_54M to
> RATE_6M was set, but it's not necessary to check every individual bit.
> The same result can be achieved using only one single mask which
> comprises all the commented bits.
> 
> This way avoid the iteration over an unnecessary for loop.
> 
> Also change the return type to bool because it's the type that this
> function returns.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  drivers/staging/vt6656/card.c | 11 ++---------
>  drivers/staging/vt6656/card.h |  2 +-
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
> index 9bd37e57c727..adaf93cf653a 100644
> --- a/drivers/staging/vt6656/card.c
> +++ b/drivers/staging/vt6656/card.c
> @@ -248,16 +248,9 @@ void vnt_update_top_rates(struct vnt_private *priv)
>  	priv->top_cck_basic_rate = top_cck;
>  }
> 
> -int vnt_ofdm_min_rate(struct vnt_private *priv)
> +bool vnt_ofdm_min_rate(struct vnt_private *priv)
>  {
> -	int ii;
> -
> -	for (ii = RATE_54M; ii >= RATE_6M; ii--) {
> -		if ((priv->basic_rates) & ((u16)BIT(ii)))
> -			return true;
> -	}
> -
> -	return false;
> +	return priv->basic_rates & GENMASK(RATE_54M, RATE_6M) ? true : false;

priv->basic_rates & GENMASK(RATE_54M, RATE_6M) is already true if
non-zero and false otherwise. Note that I haven't checked if the
rest is correct.

-- 
Stefano

Re: [PATCH] staging: vt6656: Refactor the vnt_ofdm_min_rate function

[Malcolm Priestley]

Hi all

On 19/04/2020 18:55, Stefano Brivio wrote:
> Hi Oscar,
> 
> On Sat, 18 Apr 2020 15:45:53 +0200
> Oscar Carter <oscar.carter@gmx.com> wrote:
> 
>> Replace the for loop by a ternary operator whose condition is an AND
>> bitmask against the priv->basic_rates variable.
>>
>> The purpose of the for loop was to check if any of bits from RATE_54M to
>> RATE_6M was set, but it's not necessary to check every individual bit.
>> The same result can be achieved using only one single mask which
>> comprises all the commented bits.
<snip>

>>
>> -int vnt_ofdm_min_rate(struct vnt_private *priv)
>> +bool vnt_ofdm_min_rate(struct vnt_private *priv)
>>   {
>> -	int ii;
>> -
>> -	for (ii = RATE_54M; ii >= RATE_6M; ii--) {
>> -		if ((priv->basic_rates) & ((u16)BIT(ii)))
>> -			return true;
>> -	}
>> -
>> -	return false;
>> +	return priv->basic_rates & GENMASK(RATE_54M, RATE_6M) ? true : false;
> 
> priv->basic_rates & GENMASK(RATE_54M, RATE_6M) is already true if
> non-zero and false otherwise. Note that I haven't checked if the
> rest is correct.
> 
Yes only 1 or more needs to be true and it is false when none present.

I have run-time checked the patch and it does function as before.

Regards

Malcolm.