* re: iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers
@ 2013-05-22 20:25 Dan Carpenter
2013-05-23 8:23 ` Michael Hennerich
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2013-05-22 20:25 UTC (permalink / raw)
To: michael.hennerich; +Cc: linux-iio
Hello Michael Hennerich,
The patch e31166f0fd48: "iio: frequency: New driver for Analog
Devices ADF4350/ADF4351 Wideband Synthesizers" from May 29, 2012,
leads to the following warning:
"drivers/iio/frequency/adf4350.c:212 adf4350_set_freq()
warn: 0x13c001fc0 is larger than 32 bits"
I have been messing with Smatch recently and this is not the right
warning... :/
drivers/iio/frequency/adf4350.c
207 st->regs[ADF4350_REG2] =
208 ADF4350_REG2_10BIT_R_CNT(r_cnt) |
209 ADF4350_REG2_DOUBLE_BUFF_EN |
210 (pdata->ref_doubler_en ? ADF4350_REG2_RMULT2_EN : 0) |
211 (pdata->ref_div2_en ? ADF4350_REG2_RDIV2_EN : 0) |
212 (pdata->r2_user_settings & (ADF4350_REG2_PD_POLARITY_POS |
213 ADF4350_REG2_LDP_6ns | ADF4350_REG2_LDF_INT_N |
214 ADF4350_REG2_CHARGE_PUMP_CURR_uA(5000) |
215 ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x9)));
^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^
0x7 and 0x9 are signed int so when we do "<< 29" it wraps or has a sign
extention. Also we are doing a bitwise AND with 32 bit unsigned values
so they get truncated that way too.
It's not clear what was intended here.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers
2013-05-22 20:25 iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers Dan Carpenter
@ 2013-05-23 8:23 ` Michael Hennerich
2013-05-23 9:15 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Michael Hennerich @ 2013-05-23 8:23 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-iio
On 05/22/2013 10:25 PM, Dan Carpenter wrote:
> Hello Michael Hennerich,
>
> The patch e31166f0fd48: "iio: frequency: New driver for Analog
> Devices ADF4350/ADF4351 Wideband Synthesizers" from May 29, 2012,
> leads to the following warning:
> "drivers/iio/frequency/adf4350.c:212 adf4350_set_freq()
> warn: 0x13c001fc0 is larger than 32 bits"
>
> I have been messing with Smatch recently and this is not the right
> warning... :/
>
> drivers/iio/frequency/adf4350.c
> 207 st->regs[ADF4350_REG2] =
> 208 ADF4350_REG2_10BIT_R_CNT(r_cnt) |
> 209 ADF4350_REG2_DOUBLE_BUFF_EN |
> 210 (pdata->ref_doubler_en ? ADF4350_REG2_RMULT2_EN : 0) |
> 211 (pdata->ref_div2_en ? ADF4350_REG2_RDIV2_EN : 0) |
> 212 (pdata->r2_user_settings & (ADF4350_REG2_PD_POLARITY_POS |
> 213 ADF4350_REG2_LDP_6ns | ADF4350_REG2_LDF_INT_N |
> 214 ADF4350_REG2_CHARGE_PUMP_CURR_uA(5000) |
> 215 ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x9)));
> ^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 0x7 and 0x9 are signed int so when we do "<< 29" it wraps or has a sign
> extention. Also we are doing a bitwise AND with 32 bit unsigned values
> so they get truncated that way too.
>
> It's not clear what was intended here.
>
> regards,
> dan carpenter
>
>
Hi Dan,
Thanks for the reminder.
We actually fixed the typo in the mask some time ago,
0x9 should be 0x3, we send a patch shortly.
7 or 3 being signed by default shouldn't matter. The whole term
at the end is just a big mask used to reject some bits being
set in r2_user_settings.
--
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers
2013-05-23 8:23 ` Michael Hennerich
@ 2013-05-23 9:15 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2013-05-23 9:15 UTC (permalink / raw)
To: Michael Hennerich; +Cc: linux-iio
On Thu, May 23, 2013 at 10:23:32AM +0200, Michael Hennerich wrote:
> Thanks for the reminder.
> We actually fixed the typo in the mask some time ago,
> 0x9 should be 0x3, we send a patch shortly.
>
> 7 or 3 being signed by default shouldn't matter. The whole term
> at the end is just a big mask used to reject some bits being
> set in r2_user_settings.
It's truncated away, yes. But sign extension from int literals is
an easy thing for the static checkers to spot and it's upsets and
worries them. Let's fix that as well, I would do it but it's an
awkward thing because I have to wait for your first patch and then
write a patch on top of that...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-05-23 9:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-22 20:25 iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers Dan Carpenter
2013-05-23 8:23 ` Michael Hennerich
2013-05-23 9:15 ` Dan Carpenter
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.