Hello Mauro,
Thank you for reviewing. See comments inline.
On Thu, Mar 12, 2009 at 6:06 AM, Mauro Carvalho Chehab
<mchehab@xxxxxxxxxxxxx> wrote:
> +static int vidioc_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct au0828_fh *fh = priv;
> + struct au0828_dev *dev = fh->dev;
> +
> + memset(cap, 0, sizeof(*cap));
>
> Please remove all memsets for input/output arguments on vidioc_foo at au0828-video.c.
> The V4L2 core warrants that the non-input fields are zeroed.
Ok.
> +static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_fmtdesc *f)
> +{
> + if(f->index)
> + return -EINVAL;
> +
> + memset(f, 0, sizeof(*f));
> + f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + strcpy(f->description, "Packed YUV2");
> +
> + f->flags = 0;
> + f->pixelformat = V4L2_PIX_FMT_UYVY;
> +
> + memset(f->reserved, 0, sizeof(f->reserved));
> + return 0;
> +}
>
> 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.
> 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.
> +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.
> + switch(AUVI_INPUT(index).type) {
> + case AU0828_VMUX_SVIDEO:
> + {
> + dev->input_type = AU0828_VMUX_SVIDEO;
> + break;
> + }
> + case AU0828_VMUX_COMPOSITE:
> + {
> + dev->input_type = AU0828_VMUX_COMPOSITE;
> + break;
> + }
> + case AU0828_VMUX_TELEVISION:
> + {
> + dev->input_type = AU0828_VMUX_TELEVISION;
> + break;
> + }
> + default:
> + ;
> + }
>
> You don't need all those braces.
I will remove the braces.
>
> Also, the default rule is missing. I don't see why you would preserve the same
> dev->input_type if the user selects an undefined entry, or a DVB or a debug.
I will fix that.
> 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".
I looked at the list and all of these issues are easy to fix, and I
will do that tonight.
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.
Thanks,
Devin
--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller
--
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
