Hi Bhupesh, On Wednesday 01 August 2012 21:24:58 Bhupesh SHARMA wrote: > On Wednesday, August 01, 2012 6:40 PM Laurent Pinchart wrote; > > On Tuesday 31 July 2012 06:24:52 Bhupesh Sharma wrote: [snip] > > > diff --git a/drivers/usb/gadget/uvc_v4l2.c > > > b/drivers/usb/gadget/uvc_v4l2.c > > > index 134bfe5..b47e0f7 100644 > > > --- a/drivers/usb/gadget/uvc_v4l2.c > > > +++ b/drivers/usb/gadget/uvc_v4l2.c > > > @@ -245,7 +245,20 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned > > > int cmd, void *arg) if (*type != video->queue.queue.type) > > > > > > return -EINVAL; > > > > > > - return uvc_video_enable(video, 1); > > > + /* enable uvc video. */ > > > > Please start comments with a capital letter and spell UVC in uppercase. > > :) > > Regarding the first letter as a capital letter, I don't find any such > guideline in the Documentation Style :), It's not in those guidelines, but that's how all other comments in the driver are written, so let's keep the coding style consistent :-) > though I agree on spelling UVC in capitals. > > > > + ret = uvc_video_enable(video, 1); > > > + if (ret < 0) > > > + return ret; > > > + > > > + /* > > > + * since the real video device has now started streaming > > > + * its safe now to complete the status phase of the > > > + * set_interface (alt setting 1) > > > + */ > > > > I would change this comment, as not all use cases of the UVC gadget > > driver involve a real video capture device. What about something like > > > > "Complete the alternate setting selection setup phase now that > > userspace is ready to provide video frames." > > Not really, even your dummy UVC test application provides the video frames > but it doesn't need to wait for data coming from a real video source. > > This change is required only for real video sources which are placed over > slow busses like I2C/SPI and may take time to start streaming (and hence > outputting 1st useful video frame). > > Dummy sources can always keep videobuffers queued at the UVC side (as > the data is anyways fake) at the very start, so they can immediately return > the status stage of set-alt(1). This is different from the real video > sources as there you will have to wait for a real videobuffer filled with > data before queuing the same at the UVC end and then completing the status > phase of the set-alt(1) > > So, in my view the part "userspace is ready to provide video frames" doesn't > explain the exact use case. > > Pls feel free to disagree though :) My point is that the will not always be a "real video device", so the comment should not blindly refer to one, especially given that nothing else in the driver refers to a real video device. To a newcomer I believe the comment would be confusing. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
