Hans Verkuil wrote:
> On Monday 22 October 2007 22:03:03 [EMAIL PROTECTED] 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 <[EMAIL PROTECTED]>
>
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
_______________________________________________
linux-dvb mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
