Messages in this thread |  | | Date | Sun, 16 Oct 2016 13:55:28 +0000 | From | Nicholas Mc Guire <> | Subject | Re: RFC - unclear change in "[media] DiBxxxx: Codingstype updates" |
| |
On Mon, Oct 10, 2016 at 08:31:12AM +0200, Patrick Boettcher wrote: > Hi, der Herr Hofrat ;-) > > On Sat, 8 Oct 2016 13:57:14 +0000 > Nicholas Mc Guire <der.herr@hofr.at> wrote: > > - lo6 |= (1 << 2) | 2; > > - else > > - lo6 |= (1 << 2) | 1; > > + lo6 |= (1 << 2) | 2; //SigmaDelta and Dither > > + else { > > + if (state->identity.in_soc) > > + lo6 |= (1 << 2) | 2; //SigmaDelta and > > Dither > > + else > > + lo6 |= (1 << 2) | 2; //SigmaDelta and > > Dither > > + } > > > > resulting in the current code-base of: > > > > if (Rest > 0) { > > if (state->config->analog_output) > > lo6 |= (1 << 2) | 2; > > else { > > if (state->identity.in_soc) > > lo6 |= (1 << 2) | 2; > > else > > lo6 |= (1 << 2) | 2; > > } > > Den = 255; > > } > > > > The problem now is that the if and the else(if/else) are > > all the same and thus the conditions have no effect. Further > > the origninal code actually had different if/else - so I > > wonder if this is a cut&past bug here ? > > I may answer on behalf of Olivier (didn't his address bounce?). > > I don't remember the details, this patch must date from 2011 or > before, but at that time we generated the linux-driver from our/their > internal sources. > > Updates in this area were achieved by a lot of thinking + a mix of trial > and error (after hours/days/weeks of RF hardware validation). > > This logic above has 3 possibilities: > > - we use the analog-output, or > - we are using the digital one, then there is whether we are being in > a SoC or not (SIP or sinlge chip). > > At some point in time all values have been different. In the end, they > aren't anymore, but in case someone wants to try a different value, > there are placeholders in the code to easily inject these values. > > Now the device is stable, maybe even obsolete. We could remove all the > branches resulting in the same value for lo6. > ok - so as the value for lo6 effectively is the same for all conditions
given (1 << 2) | 2 == 6
this might be simplified to and commented as: if (Rest > 0) { /* Based on trial and error */ lo6 |= 6; Den = 255; }
let me know if that sounds resonable - just plugging in a magic number sounds like a bad idea - even if this comment might not be wildly enlightening it atleast indicates that it is known "magic".
thx! Der Herr Hofrat
|  |