[linux-dvb] [RFC] tuner-refactor-phase-2

mkrufky at linuxtv.org mkrufky at linuxtv.org
Mon Oct 22 23:25:07 CEST 2007


Hans Verkuil wrote:
> On Monday 22 October 2007 22:03:03 mkrufky at linuxtv.org wrote:
>   
>> http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2
> Hi Mike,
>
> Looks good!
>
>   
Hans,

Thank you for the review!
> Just some nitpick things:
>
> 1)
>
> tuner-core.c: things like:
>
> -               if (t->ops.tuner_status)
> -                       t->ops.tuner_status(t);
> +               if ((ops) && (ops->tuner_status))
> +                       ops->tuner_status(t);
>
> A bit too many parenthesis, 'if (ops && ops->tuner_status)' works just 
> as well and is more readable IMHO.
>   
Good point -- I've cleaned that up.
> 2)
>
> Please comment who IS responsible for freeing analog_demod_priv, 
> also 'kfree(t->fe.' should be 'kfree(fe->' to keep the comment in sync 
> with the code changes.
>
> +static void fe_release(struct dvb_frontend *fe)
> +{
> +       if (fe->ops.tuner_ops.release)
> +               fe->ops.tuner_ops.release(fe);
> +
> +       fe->ops.analog_demod_ops = NULL;
> +       /* DO NOT kfree(t->fe.analog_demod_priv) */
> +       fe->analog_demod_priv = NULL;
> +}
>   
Done.  I realize that this could be confusing.  This should be clearer 
with the new comments / better explanation.
> 3)
>
> Personally I don't see the need for changeset 6214 (clean up ops 
> checking in tuner_status function): I thought the original was easier 
> to read. Just remove the '{' '}' checkpatch complains about and it's 
> fine.
>   
I disagree with you here.  In reality, either way should be OK...  The 
way it is right now simplifies matters by checking (ops) once.  If it is 
NULL, then neither .has_signal nor .is_stereo will be checked.  I'd 
prefer to keep it this way.
> Reviewed-by: Hans Verkuil <hverkuil at xs4all.nl>
>   
Thanks.  I will add your reviewed-by tag to all changesets EXCEPT for 
#6214 as mentioned in (3).  I'm going to give it a few hours first, to 
see if anybody else has any comments.
> Regards,
>
> 	Hans
>   

Thanks again for the review at such short notice.

Regards,

Mike Krufky





More information about the linux-dvb mailing list