[PATCH] staging: pi433: fix a precedence bug

[PATCH] staging: pi433: fix a precedence bug

[Dan Carpenter]

We had intended to do the mask first and then the shift.  Shift has
higher precedence so we need to add parenthesis.

Fixes: 874bcba65f9a ("staging: pi433: New driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e391ce777bc7..5089449bf775 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
 
 	currentValue = READ_REG(REG_LNA);
 
-	switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3)  // improvement: change 3 to define
+	switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 3)  // improvement: change 3 to define
 	{
 	case LNA_GAIN_AUTO:	    return automatic;
 	case LNA_GAIN_MAX:	    return max;

Re: [PATCH] staging: pi433: fix a precedence bug

[walter harms]

Am 19.07.2017 11:51, schrieb Dan Carpenter:
> We had intended to do the mask first and then the shift.  Shift has
> higher precedence so we need to add parenthesis.
> 
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..5089449bf775 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
>  
>  	currentValue = READ_REG(REG_LNA);


	is currentValue used again later ?

	iff not (IMHO) it would be more readable to use

	currentValue &= MASK_LNA_CURRENT_GAIN

	here.

	and
         currentValue >>3

	inside the switch()
>  
> -	switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3)  // improvement: change 3 to define
> +	switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 3)  // improvement: change 3 to define
>  	{
>  	case LNA_GAIN_AUTO:	    return automatic;
>  	case LNA_GAIN_MAX:	    return max;

note:
and instead to add an artificial define for 3, it would be simpler to change the  LNA_GAIN_AUTO/MAX
to the "real" values and drop the >>3 for good.

just my 2 cents,

re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] staging: pi433: fix a precedence bug

[Dan Carpenter]

Actually, please drop this patch.  I looked at this one the same as I
did the other one, but I didn't look closely enough.  It's the same
situation where we have two bugs that probably cancel each other out
somewhat.  It maybe needs to be fixed by someone who can test the code.

In rf69_set_lna_gain() we should be left shifting << 3 but we don't.

regards,
dan carpenter

Re: [PATCH] staging: pi433: fix a precedence bug

[Marcus Wolf]

Once again in plain text, only - sorry for frogetting to set the switch in my
mailtool....
 
Hi Dan, Hi Walter,
 
thank you for your investigation.
 
rf69_get_modulation, rf69_get_lna_gain, rf69_set_lna_gain: Seems like I messed
up a few things :-/
 
In the beginning, the defines were left-bound "real"  values, like you can see
for the LNA_GAIN values. Later on, I deceided not to use left-bound "real"
values, but to leave the bits at the position, they have in the register. These
two impelemtation types are mixed up here. Sorry.
 
Result: The shift is obsolete and the LNA_GAIN defines are old-style and need to
be reviesed. I will send a patch with a proposal in approx. half an hour.
 
rf69_set_deviation: Yes Dan, bitwise negate was intended.
 
Once again, thanks a lot for your investigation,
 
Marcus
 
 
Thanks a lot,
 
Marcus
 

> Dan Carpenter <dan.carpenter@oracle.com> hat am 19. Juli 2017 um 12:26
> geschrieben:
>
>
> Actually, please drop this patch. I looked at this one the same as I
> did the other one, but I didn't look closely enough. It's the same
> situation where we have two bugs that probably cancel each other out
> somewhat. It maybe needs to be fixed by someone who can test the code.
>
> In rf69_set_lna_gain() we should be left shifting << 3 but we don't.
>
> regards,
> dan carpenter
>
>