Hans Verkuil wrote: > On Monday 22 October 2007 22:03:03 mkrufky@xxxxxxxxxxx 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@xxxxxxxxx> > 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 linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
