On Thu, 12 Mar 2009 10:24:02 -0400
Devin Heitmueller <devin.heitmueller@xxxxxxxxx> wrote:
> > hmm.. you are cleaning up f->reserved three times: at v4l2-ioctl, at the
> > memset(f) and at memset(f->reserved).
> >
> > You really wanted to make sure that you've cleaned it, don't you? ;)
>
> Well, I wanted to be *extra* sure. ;-) Yeah, I'll yank the duplicate code.
LOL
> > hmm...
> >
> > +#ifdef VBI_NOT_YET_WORKING
> > + .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
> > + .vidioc_try_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
> > + .vidioc_s_fmt_vbi_cap = vidioc_s_fmt_vbi_cap,
> > +#endif
> >
> > I don't see any reference of this macro. If VBI is working, please cleanup the
> > driver. Btw, your logic seems to be inverted on some cases. Why are you adding
> > VBI macros, if it is not working yet?
> >
> > On the other hand, if VBI is broken we'll need some rules for removing vbi code
> > from upstream, at gentree.pl.
>
> Here's the situation with VBI: I did all the groundwork, but it
> doesn't work yet. I am hoping on getting it working over the next
> couple of weeks. I believed that #ifdef'ing out the code was the
> safest way to ensure that the code does not get called, while not
> having to remove it from the tree entirely.
>
> If this is important to you that the code not appear in the source
> tree at all until it works entirely, then I will remove the VBI code
> entirely.
Hmm... So, I understood just the opposite ;)
you wrote "if VBI_IS_NOT_WORKING", when you should have written "if
VBI_IS_WORKING".
Just rename it. I'll add it at gentree.pl to remove this symbol.
>
> > +enum au0828_itype {
> > + AU0828_VMUX_UNDEFINED = 0,
> > + AU0828_VMUX_COMPOSITE,
> > + AU0828_VMUX_SVIDEO,
> > + AU0828_VMUX_CABLE,
> > + AU0828_VMUX_TELEVISION,
> > + AU0828_VMUX_DVB,
> > + AU0828_VMUX_DEBUG
> > +};
> >
> > ...
> >
> > +static int vidioc_enum_input(struct file *file, void *priv,
> > + struct v4l2_input *input)
> > +{
> > + struct au0828_fh *fh = priv;
> > + struct au0828_dev *dev = fh->dev;
> > + unsigned int tmp;
> > +
> > + static const char *inames[] = {
> > + [AU0828_VMUX_COMPOSITE] = "Composite",
> > + [AU0828_VMUX_SVIDEO] = "S-Video",
> > + [AU0828_VMUX_CABLE] = "Cable TV",
> > + [AU0828_VMUX_TELEVISION] = "Television",
> > + [AU0828_VMUX_DVB] = "DVB",
> > + [AU0828_VMUX_DEBUG] = "tv debug"
> > + };
> >
> > If the user enumerates an entry marked as UNDEFINED, it will print NULL. Is it
> > what you really wanted? I would, instead, assign another value for
> > AU0828_VMUX_UNDEFINED, like -1.
>
> Yeah, printing "NULL" is bad and I can obviously fix that. The real
> reason for the addition of the "UNDEFINED" entry is I use that to
> detect if there are *any* analog inputs defined, which dictates
> whether the analog subsystem is initialized. Because the .input
> section is a member of the au0828_dev struct, and not a pointer, I
> needed some way to tell if it was populated with anything.
if you attribute it to -1, the userspace calls will never set it to undefined.
You should take some care to avoid reading outside some array though.
> > Ah, finally, there are a number of CodingStyle fun. I've enclosed what it got,
> > from the final code. Please, always use make checkpatch before committing a patch.
>
> I rather foolishly assumed that "make commit" ran "make checkpatch".
It runs, and outputs its result at the commit comments.
> I looked at the list and all of these issues are easy to fix, and I
> will do that tonight.
Ok.
> Please let me know if you have any other concerns (and what you want
> me to do regarding the VBI stuff), since I would like to avoid having
> do redo the tree again.
No, just the above. Please, instead of redo the tree, just add some patches
fixing those issues. This allows me to review faster your series.
Cheers,
Mauro
--
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
