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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
