Hi Laurent, > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Friday, August 03, 2012 1:50 PM > To: Bhupesh SHARMA > Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx > Subject: Re: [PATCH V2 2/2] usb: gadget/uvc: Add support for > 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command > > 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 :-) Ok :) > > 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. > Ok. Agreed :) I will send a V3 after I receive your review comments on 1/2 patch of this pathcset (so that I can accommodate review comments on 1/2 also, if any, in V3). Regards, Bhupesh -- 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
