Copyright issues, do not copy code and add your own copyrights

Copyright issues, do not copy code and add your own copyrights

[Manu Abraham]

Hi,

The subject line says it.

Please fix the offending Copyright header.

Offending one.
http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_proc.h

Original one.
http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_cfg.h


Regards,
Manu

Re: Copyright issues, do not copy code and add your own copyrights

[Hans de Goede]

Hi,

On 08/14/2012 11:10 AM, Manu Abraham wrote:
> Hi,
>
> The subject line says it.
>
> Please fix the offending Copyright header.
>
> Offending one.
> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_proc.h
>
> Original one.
> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_cfg.h

Or even better, get rid of the offending one and add a i2c_gate_ctrl parameters to the inline
functions defined in stb6100_cfg.h, as this seems a typical case of unnecessary code-duplication.

I would also like to point out that things like these are pretty much wrong:

   27         if (&fe->ops)
   28                 frontend_ops = &fe->ops;
   29         if (&frontend_ops->tuner_ops)
   30                 tuner_ops = &frontend_ops->tuner_ops;
   31         if (tuner_ops->get_state) {

The last check de-references tuner_ops, which only is non-NULL if
fe-ops and fe->ops->tuner_ops are non NULL. So either the last check
needs to be:
              if (tuner_ops && tuner_ops->get_state) {

Or we assume that fe-ops and fe->ops->tuner_ops are always non NULL
when this helper gets called and all the previous checks can be removed.

Regards,

Hans

Re: Copyright issues, do not copy code and add your own copyrights

[Manu Abraham]

Hi,

On Tue, Aug 14, 2012 at 2:51 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 08/14/2012 11:10 AM, Manu Abraham wrote:
>>
>> Hi,
>>
>> The subject line says it.
>>
>> Please fix the offending Copyright header.
>>
>> Offending one.
>>
>> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_proc.h
>>
>> Original one.
>>
>> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_cfg.h
>
>
> Or even better, get rid of the offending one and add a i2c_gate_ctrl
> parameters to the inline
> functions defined in stb6100_cfg.h, as this seems a typical case of
> unnecessary code-duplication.


i2c_gate_ctrl is not provided by stb6100 hardware, but by the demodulator
used in conjunction such as a stb0899 as can be seen.


1473                         /* enable tuner I/O */
1474                         stb0899_i2c_gate_ctrl(&state->frontend, 1);
1475
1476                         if (state->config->tuner_set_bandwidth)
1477
state->config->tuner_set_bandwidth(fe, (13 *
(stb0899_carr_width(state) + SearchRange)) / 10);
1478                         if (state->config->tuner_get_bandwidth)
1479
state->config->tuner_get_bandwidth(fe, &internal->tuner_bw);


A sleep for a jiffie is needed after the gate is enabled, but any real life
sleep is pointless and causes unnecessary delays, causing noise to bleed
into the demodulator.

This improves tuning performance slightly. The user (demodulator) of the tuner
needs to enable/disable the gate, in this case as seen in

http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb0899_drv.c


>
> I would also like to point out that things like these are pretty much wrong:
>
>   27         if (&fe->ops)
>   28                 frontend_ops = &fe->ops;
>   29         if (&frontend_ops->tuner_ops)
>   30                 tuner_ops = &frontend_ops->tuner_ops;
>   31         if (tuner_ops->get_state) {
>
> The last check de-references tuner_ops, which only is non-NULL if
> fe-ops and fe->ops->tuner_ops are non NULL. So either the last check
> needs to be:
>              if (tuner_ops && tuner_ops->get_state) {
>
> Or we assume that fe-ops and fe->ops->tuner_ops are always non NULL
> when this helper gets called and all the previous checks can be removed.


fe->ops is not NULL in any case, when we reach here, but that conditionality
check causes a slight additional delay. The additional check you proposed
presents no harm, though not bringing any new advantage/disadvantage.

Regards,

Manu

Re: Copyright issues, do not copy code and add your own copyrights

[Hans de Goede]

Hi,

On 08/14/2012 11:42 AM, Manu Abraham wrote:
> Hi,
>
> On Tue, Aug 14, 2012 at 2:51 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 08/14/2012 11:10 AM, Manu Abraham wrote:
>>>
>>> Hi,
>>>
>>> The subject line says it.
>>>
>>> Please fix the offending Copyright header.
>>>
>>> Offending one.
>>>
>>> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_proc.h
>>>
>>> Original one.
>>>
>>> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_cfg.h
>>
>>
>> Or even better, get rid of the offending one and add a i2c_gate_ctrl
>> parameters to the inline
>> functions defined in stb6100_cfg.h, as this seems a typical case of
>> unnecessary code-duplication.
>
>
> i2c_gate_ctrl is not provided by stb6100 hardware, but by the demodulator
> used in conjunction such as a stb0899 as can be seen.

Right, I was merely pointing out that the only difference between the
original function wrappers in stb6100_cfg.h and the ones in stb6100_proc.h,
is the calling of the i2c_gate_ctrl frontend-op if defined. So the 2 files
could be merged into one, with the wrappers getting an extra boolean parameter
making them call the frontend-op when that parameter is true.

Note that if the i2c_gate_ctrl frontend-op should always be called when
present then the extra parameter could be omitted.

<snip>

>> I would also like to point out that things like these are pretty much wrong:
>>
>>    27         if (&fe->ops)
>>    28                 frontend_ops = &fe->ops;
>>    29         if (&frontend_ops->tuner_ops)
>>    30                 tuner_ops = &frontend_ops->tuner_ops;
>>    31         if (tuner_ops->get_state) {
>>
>> The last check de-references tuner_ops, which only is non-NULL if
>> fe-ops and fe->ops->tuner_ops are non NULL. So either the last check
>> needs to be:
>>               if (tuner_ops && tuner_ops->get_state) {
>>
>> Or we assume that fe-ops and fe->ops->tuner_ops are always non NULL
>> when this helper gets called and all the previous checks can be removed.
>
>
> fe->ops is not NULL in any case, when we reach here, but that conditionality
> check causes a slight additional delay. The additional check you proposed
> presents no harm, though not bringing any new advantage/disadvantage.

Well if we know that fe->ops and fe->ops->tuner_ops are never NULL, then the
if (&fe->ops) and if (&frontend_ops->tuner_ops) are superfluous and should be
removed, on the other hand if we don't know that, then the get_state check should
be:
                if (tuner_ops && tuner_ops->get_state) {

Either know fe->ops and fe->ops->tuner_ops are never NULL and then all checks
should be removed, or we don't know and we should check them in *all* places
where they are used. What we've now is somewhat of the former, and then some of
the latter, which makes no sense at all.

Regards,

Hans




>
> Regards,
>
> Manu
>

Re: Copyright issues, do not copy code and add your own copyrights

[Mauro Carvalho Chehab]

Em 14-08-2012 09:35, Hans de Goede escreveu:
> Hi,
> 
> On 08/14/2012 11:42 AM, Manu Abraham wrote:
>> Hi,
>>
>> On Tue, Aug 14, 2012 at 2:51 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>>
>>> On 08/14/2012 11:10 AM, Manu Abraham wrote:
>>>>
>>>> Hi,
>>>>
>>>> The subject line says it.
>>>>
>>>> Please fix the offending Copyright header.
>>>>
>>>> Offending one.
>>>>
>>>> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_proc.h
>>>>
>>>> Original one.
>>>>
>>>> http://git.linuxtv.org/media_tree.git/blob/staging/for_v3.7:/drivers/media/dvb-frontends/stb6100_cfg.h

Yeah, they look pretty much the same code.

>>>
>>>
>>> Or even better, get rid of the offending one and add a i2c_gate_ctrl
>>> parameters to the inline
>>> functions defined in stb6100_cfg.h, as this seems a typical case of
>>> unnecessary code-duplication.
>>
>>
>> i2c_gate_ctrl is not provided by stb6100 hardware, but by the demodulator
>> used in conjunction such as a stb0899 as can be seen.
> 
> Right, I was merely pointing out that the only difference between the
> original function wrappers in stb6100_cfg.h and the ones in stb6100_proc.h,
> is the calling of the i2c_gate_ctrl frontend-op if defined. So the 2 files
> could be merged into one, with the wrappers getting an extra boolean parameter
> making them call the frontend-op when that parameter is true.
> 
> Note that if the i2c_gate_ctrl frontend-op should always be called when
> present then the extra parameter could be omitted.

It is up to the attach logic to fill i2c_gate_ctrl() callback or not.
So, it is safe to keep it there, as it will work with both drivers
that need and the ones that don't.

Manu,

Please write a patch merging them and fixing the copyright.

Thanks,
Mauro


> 
> <snip>
> 
>>> I would also like to point out that things like these are pretty much wrong:
>>>
>>>    27         if (&fe->ops)
>>>    28                 frontend_ops = &fe->ops;
>>>    29         if (&frontend_ops->tuner_ops)
>>>    30                 tuner_ops = &frontend_ops->tuner_ops;
>>>    31         if (tuner_ops->get_state) {
>>>
>>> The last check de-references tuner_ops, which only is non-NULL if
>>> fe-ops and fe->ops->tuner_ops are non NULL. So either the last check
>>> needs to be:
>>>               if (tuner_ops && tuner_ops->get_state) {
>>>
>>> Or we assume that fe-ops and fe->ops->tuner_ops are always non NULL
>>> when this helper gets called and all the previous checks can be removed.
>>
>>
>> fe->ops is not NULL in any case, when we reach here, but that conditionality
>> check causes a slight additional delay. The additional check you proposed
>> presents no harm, though not bringing any new advantage/disadvantage.
> 
> Well if we know that fe->ops and fe->ops->tuner_ops are never NULL, then the
> if (&fe->ops) and if (&frontend_ops->tuner_ops) are superfluous and should be
> removed, on the other hand if we don't know that, then the get_state check should
> be:
>                if (tuner_ops && tuner_ops->get_state) {
> 
> Either know fe->ops and fe->ops->tuner_ops are never NULL and then all checks
> should be removed, or we don't know and we should check them in *all* places
> where they are used. What we've now is somewhat of the former, and then some of
> the latter, which makes no sense at all.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>> Regards,
>>
>> Manu
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html