Re: RFC - unclear change in "[media] DiBxxxx: Codingstype updates"

[Nicholas Mc Guire]

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@xxxxxxx> 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