Hi Laurent,
thank you for your review.
I generally agree with your comments but please see inline
for some explanations.
W dniu 28.11.2014 o 00:40, Laurent Pinchart pisze:
Hi Andrzej,
Thank you for the patch.
On Wednesday 24 September 2014 15:26:43 Andrzej Pietrasiewicz wrote:
Add support for using the uvc function as a component of USB gadgets
composed with configfs.
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
---
<snip>
This should be 3.19. My fault, sorry :-/
I'm glad you have managed to review.
Now it looks like
all other functions which are available as separate f_xyz.c files are
already converted, so it would be nice if uvc made its way into 3.20.
<snip>
+What: /config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg
+Date: Nov 2014
+KernelVersion: 3.18
+Description: MJPEG format descriptors
+
+What: /config/usb-gadget/gadget/functions/uvc.name/streaming/mjpeg/name
I'm not too familiar with configfs, should the attribute description specify
what values are recommended and/or acceptable for "name" ?
Anything? The video format is determined by the parent directory name
(mjpeg in this case). The name can be pretty much anything a filesystem
permits. The association of formats is done with symbolic links.
<snip>
@@ -778,6 +873,7 @@ static struct usb_function *uvc_alloc(struct
usb_function_instance *fi) {
struct uvc_device *uvc;
struct f_uvc_opts *opts;
+ struct uvc_descriptor_header **strm_cls;
uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
if (uvc == NULL)
@@ -786,11 +882,29 @@ static struct usb_function *uvc_alloc(struct
usb_function_instance *fi) uvc->state = UVC_STATE_DISCONNECTED;
opts = fi_to_f_uvc_opts(fi);
+ mutex_lock(&opts->lock);
What does the mutex protect against ? Modifications of opts-
uvc_*_streaming_cls by configfs ? You only copy pointers around, what
prevents the descriptors from being modified after you unlock the mutex ?
+ if (opts->uvc_fs_streaming_cls) {
+ strm_cls = opts->uvc_fs_streaming_cls;
+ opts->fs_streaming =
+ (const struct uvc_descriptor_header * const *)strm_cls;
+ }
+ if (opts->uvc_hs_streaming_cls) {
+ strm_cls = opts->uvc_hs_streaming_cls;
+ opts->hs_streaming =
+ (const struct uvc_descriptor_header * const *)strm_cls;
+ }
+ if (opts->uvc_ss_streaming_cls) {
+ strm_cls = opts->uvc_ss_streaming_cls;
+ opts->ss_streaming =
+ (const struct uvc_descriptor_header * const *)strm_cls;
+ }
+
Also modifications of opts->fs_streaming & friends.
<snip>
diff --git a/drivers/usb/gadget/function/uvc_configfs.c
b/drivers/usb/gadget/function/uvc_configfs.c new file mode 100644
index 0000000..33d92ab
--- /dev/null
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -0,0 +1,2439 @@
I can't really review this in a reasonable time frame at the moment I'm afraid
:-/ We'll have to fix bugs as we go.
[snip]
<snip>
You never know with a company what you are assigned to, but I'm really
hoping to stick around usb gadgets next year so I intend to fix any bugs
that are discovered, especially if they are found in my code ;-) So I
think your suggestion seems reasonable, given the amount of time which
has passed since we first talked in New Orleans about the idea of adding
configfs support to the uvc function.
AP
--
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
