analogdevicesinc / no-OS

Software drivers in C for systems without an operating system
http://analogdevicesinc.github.io/no-OS/
Other
967 stars 1.66k forks source link

Incorrect formula in ad9361_gc_update #39

Closed GardenerWilly closed 6 years ago

GardenerWilly commented 6 years ago

Comment in function says:

    /*
     * AGC Attack Delay (us)=ceiling((((0.2+Delay_LNA)*ClkRF+14))/(2*ClkRF))+1
     * ClkRF in MHz, delay in us
     */

but code for delay claculation looks like that: reg = (200 * delay_lna) / 2 + (14000000UL / (clkrf / 500U)); Seems it must be: reg = (200 + delay_lna) / 2 + (14000000UL / (clkrf / 500U));

mhennerich commented 6 years ago

No sure what you mean.

/*
 * AGC Attack Delay (us)=ceiling((((0.2+Delay_LNA)*ClkRF+14))/(2*ClkRF))+1
 * ClkRF in MHz, delay in us
 */

reg = (200 * delay_lna) / 2 + (14000000UL / (clkrf / 500U));
reg = DIV_ROUND_UP(reg, 1000UL) +
    phy->pdata->gain_ctrl.agc_attack_delay_extra_margin_us;
reg = clamp_t(u8, reg, 0U, 31U);
ret = ad9361_spi_writef(spi, REG_AGC_ATTACK_DELAY,
          AGC_ATTACK_DELAY(~0), reg);

The formula in the comment expects MHz and us. While the code uses Hz and ns. The divide by 1000 is done by the DIV_ROUND_UP kernel macro.

-Michael

GardenerWilly commented 6 years ago

I can't see that summ: (0.2 (us) + Delay_LNA (us)) I see strange multiplication: (200 (ns) * delay_lna (ns))

mhennerich commented 6 years ago

When I verified this a few minutes ago I correctly used + And didn't notice the difference in your quoted corrected code....

I wrote that stuff 5 years ago - and must have been blind today and 5 years back. You're absolutely right it must be +. That's a typo!

Thanks for pointing out. Will be fixed ASAP.

mhennerich commented 6 years ago

Fixed on Linux - No-OS driver will pick this up shortly: https://github.com/analogdevicesinc/linux/commit/c876b7a9b5549252f894936f5daac69ea0b841f1

dbogdan commented 6 years ago

Fixed. Thanks for let us know about this.