* [PATCH] staging: pi433: fix a precedence bug
@ 2017-07-19 9:51 Dan Carpenter
2017-07-19 10:11 ` walter harms
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Dan Carpenter @ 2017-07-19 9:51 UTC (permalink / raw)
To: kernel-janitors
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;
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: pi433: fix a precedence bug
2017-07-19 9:51 [PATCH] staging: pi433: fix a precedence bug Dan Carpenter
@ 2017-07-19 10:11 ` walter harms
2017-07-19 10:26 ` Dan Carpenter
2017-07-19 17:45 ` Marcus Wolf
2 siblings, 0 replies; 4+ messages in thread
From: walter harms @ 2017-07-19 10:11 UTC (permalink / raw)
To: kernel-janitors
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: pi433: fix a precedence bug
2017-07-19 9:51 [PATCH] staging: pi433: fix a precedence bug Dan Carpenter
2017-07-19 10:11 ` walter harms
@ 2017-07-19 10:26 ` Dan Carpenter
2017-07-19 17:45 ` Marcus Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2017-07-19 10:26 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: pi433: fix a precedence bug
2017-07-19 9:51 [PATCH] staging: pi433: fix a precedence bug Dan Carpenter
2017-07-19 10:11 ` walter harms
2017-07-19 10:26 ` Dan Carpenter
@ 2017-07-19 17:45 ` Marcus Wolf
2 siblings, 0 replies; 4+ messages in thread
From: Marcus Wolf @ 2017-07-19 17:45 UTC (permalink / raw)
To: kernel-janitors
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-07-19 17:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-19 9:51 [PATCH] staging: pi433: fix a precedence bug Dan Carpenter
2017-07-19 10:11 ` walter harms
2017-07-19 10:26 ` Dan Carpenter
2017-07-19 17:45 ` Marcus Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.