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

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

[Nicholas Mc Guire]

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&past 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

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

[Patrick Boettcher]

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.

--
Patrick.

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