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 + > Nicholas Mc Guirewrote: > > - 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 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
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 + > Nicholas Mc Guire 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 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
Re: RFC - unclear change in "[media] DiBxxxx: Codingstype updates"
Hi, der Herr Hofrat ;-) On Sat, 8 Oct 2016 13:57:14 + Nicholas Mc Guirewrote: > - 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 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. -- Patrick.
Re: RFC - unclear change in "[media] DiBxxxx: Codingstype updates"
Hi, der Herr Hofrat ;-) On Sat, 8 Oct 2016 13:57:14 + Nicholas Mc Guire 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 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. -- Patrick.
RFC - unclear change in "[media] DiBxxxx: Codingstype updates"
Hi Olivier ! in your commit 28fafca78797b ("[media] DiB0090: misc improvements") with commit message: This patch adds several performance improvements and prepares the usage of firmware-based devices. it seems you changed the logic of an if/else in dib0090_tune() in a way that I do not understand: - 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 bug here ? With no knowlege of the device providing a patch makes no sense as it would just be guessing - in any case this looks wrong (or atleast should have a comment if it actually is correct) What am I missing ? thx! hofrat
RFC - unclear change in "[media] DiBxxxx: Codingstype updates"
Hi Olivier ! in your commit 28fafca78797b ("[media] DiB0090: misc improvements") with commit message: This patch adds several performance improvements and prepares the usage of firmware-based devices. it seems you changed the logic of an if/else in dib0090_tune() in a way that I do not understand: - 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 bug here ? With no knowlege of the device providing a patch makes no sense as it would just be guessing - in any case this looks wrong (or atleast should have a comment if it actually is correct) What am I missing ? thx! hofrat