Hi Andrzej,
Thank you for the patch.
git am complains with
Applying: usb/gadget: f_uvc: add configfs support
/home/laurent/src/kernel/media.git/.git/rebase-apply/patch:40: trailing
whitespace.
boolean "USB Webcam function"
warning: 1 line adds whitespace errors.
Could you please fix that ?
On Friday 28 February 2014 10:32:30 Andrzej Pietrasiewicz wrote:
> Add support for using uvc as a component of a composite gadget
> set up with configfs.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 11 +
> drivers/usb/gadget/Kconfig | 11 +
> drivers/usb/gadget/Makefile | 2 +-
> drivers/usb/gadget/f_uvc.c | 97 +-
> drivers/usb/gadget/u_uvc.h | 19 +
> drivers/usb/gadget/uvc_configfs.c | 2928 ++++++++++++++++++
> drivers/usb/gadget/uvc_configfs.h | 283 ++
> 7 files changed, 3348 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/ABI/testing/configfs-usb-gadget-uvc
> create mode 100644 drivers/usb/gadget/uvc_configfs.c
> create mode 100644 drivers/usb/gadget/uvc_configfs.h
[snip]
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> index bbadc0a..31e8bed 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -31,6 +31,7 @@
> #include "u_uvc.h"
> #include "uvc_video.h"
> #include "uvc_v4l2.h"
> +#include "uvc_configfs.h"
Please keep the headers sorted alphabetically.
> unsigned int uvc_gadget_trace_param;
>
> @@ -467,6 +468,9 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed) break;
> }
>
> + if (!uvc_control_desc || !uvc_streaming_cls)
> + return ERR_PTR(-ENODEV);
> +
Is this a configfs-specific problem ? If not it should be moved to a separate
patch.
> /* Descriptors layout
> *
> * uvc_iad
> @@ -658,10 +662,25 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
>
> /* Copy descriptors */
> f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> + if (IS_ERR(f->fs_descriptors)) {
> + ret = PTR_ERR(f->fs_descriptors);
> + f->fs_descriptors = NULL;
> + goto error;
> + }
> if (gadget_is_dualspeed(cdev->gadget))
> f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
> + if (IS_ERR(f->hs_descriptors)) {
> + ret = PTR_ERR(f->hs_descriptors);
> + f->hs_descriptors = NULL;
> + goto error;
> + }
> if (gadget_is_superspeed(c->cdev->gadget))
> f->ss_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_SUPER);
> + if (IS_ERR(f->ss_descriptors)) {
> + ret = PTR_ERR(f->ss_descriptors);
> + f->ss_descriptors = NULL;
> + goto error;
> + }
Same comment here. Should't you also handle the case where
uvc_copy_descriptors() returns NULL ? It seems a pretty fatal error too.
>
> /* Preallocate control endpoint request. */
> uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0, GFP_KERNEL);
> @@ -738,17 +757,88 @@ static struct usb_function_instance
> *uvc_alloc_inst(void) opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> if (!opts)
> return ERR_PTR(-ENOMEM);
> + mutex_init(&opts->lock);
> opts->func_inst.free_func_inst = uvc_free_inst;
>
> + config_group_init_type_name(&f_uvc_header_group, "header",
> + &f_uvc_header_type);
> + config_group_init_type_name(&f_uvc_processing_group, "processing",
> + &f_uvc_processing_type);
> + config_group_init_type_name(&f_uvc_class_fs_group, "fs",
> + &f_uvc_class_fs_type);
> + config_group_init_type_name(&f_uvc_class_ss_group, "ss",
> + &f_uvc_class_ss_type);
> + f_uvc_class_group.default_groups = f_uvc_class_default_groups;
> + config_group_init_type_name(&f_uvc_class_group, "class",
> + &f_uvc_class_type);
> + config_group_init_type_name(&f_uvc_camera_group, "camera",
> + &f_uvc_camera_type);
> + config_group_init_type_name(&f_uvc_output_group, "output",
> + &f_uvc_output_type);
> + f_uvc_terminal_group.default_groups = f_uvc_terminal_default_groups;
> + config_group_init_type_name(&f_uvc_terminal_group, "terminal",
> + &f_uvc_terminal_type);
> + f_uvc_control_group.group.default_groups = f_uvc_control_default_groups;
> + INIT_LIST_HEAD(&f_uvc_control_group.known_targets);
> + config_group_init_type_name(&f_uvc_control_group.group, "control",
> + &f_uvc_control_type);
> + config_group_init_type_name(&f_uvc_input_header_group, "input_header",
> + &f_uvc_input_header_type);
> + config_group_init_type_name(&f_uvc_color_matching_group,
"color_matching",
> + &f_uvc_color_matching_type);
> + config_group_init_type_name(&f_uvc_streaming_fs_group, "fs",
> + &f_uvc_streaming_fs_type);
> + config_group_init_type_name(&f_uvc_streaming_hs_group, "hs",
> + &f_uvc_streaming_hs_type);
> + config_group_init_type_name(&f_uvc_streaming_ss_group, "ss",
> + &f_uvc_streaming_ss_type);
> + f_uvc_streaming_class_group.default_groups =
> + f_uvc_streaming_class_default_groups;
> + config_group_init_type_name(&f_uvc_streaming_class_group, "class",
> + &f_uvc_streaming_class_type);
> + config_group_init_type_name(&f_uvc_frame_yuv_group, "yuv",
> + &f_uvc_frame_yuv_type);
> + config_group_init_type_name(&f_uvc_frame_mjpeg_group, "mjpeg",
> + &f_uvc_frame_mjpeg_type);
> + f_uvc_frame_group.default_groups = f_uvc_frame_default_groups;
> + config_group_init_type_name(&f_uvc_frame_group, "frame",
> + &f_uvc_frame_type);
> + config_group_init_type_name(&f_uvc_format_yuv_group, "yuv",
> + &f_uvc_format_yuv_type);
> + config_group_init_type_name(&f_uvc_format_mjpeg_group, "mjpeg",
> + &f_uvc_format_mjpeg_type);
> + f_uvc_format_group.group.default_groups = f_uvc_format_default_groups;
> + INIT_LIST_HEAD(&f_uvc_format_group.known_targets);
> + config_group_init_type_name(&f_uvc_format_group.group, "format",
> + &f_uvc_format_type);
> + f_uvc_streaming_group.group.default_groups =
> f_uvc_streaming_default_groups;
> + INIT_LIST_HEAD(&f_uvc_streaming_group.known_targets);
> + config_group_init_type_name(&f_uvc_streaming_group.group, "streaming",
> + &f_uvc_streaming_type);
> + opts->func_inst.group.default_groups = f_uvc_default_groups;
> + opts->fs_class = &f_uvc_class_fs_group.cg_item;
> + opts->ss_class = &f_uvc_class_ss_group.cg_item;
> + opts->fs_streaming_class = &f_uvc_streaming_fs_group.cg_item;
> + opts->hs_streaming_class = &f_uvc_streaming_hs_group.cg_item;
> + opts->ss_streaming_class = &f_uvc_streaming_ss_group.cg_item;
> + INIT_LIST_HEAD(&opts->known_targets);
> + config_group_init_type_name(&opts->func_inst.group, "",
> + &uvc_func_type);
> +
> return &opts->func_inst;
> }
>
> static void uvc_free(struct usb_function *f)
> {
> struct uvc_device *uvc;
> + struct f_uvc_opts *opts;
>
> uvc = to_uvc(f);
> + opts = container_of(f->fi, struct f_uvc_opts, func_inst);
> kfree(uvc);
> + mutex_lock(&opts->lock);
> + --opts->refcnt;
> + mutex_unlock(&opts->lock);
> }
>
> static void uvc_unbind(struct usb_configuration *c, struct usb_function *f)
> @@ -778,14 +868,17 @@ struct usb_function *uvc_alloc(struct
> usb_function_instance *fi) if (uvc == NULL)
> return ERR_PTR(-ENOMEM);
>
> - uvc->state = UVC_STATE_DISCONNECTED;
> opts = container_of(fi, struct f_uvc_opts, func_inst);
> -
> + mutex_lock(&opts->lock);
> + ++opts->refcnt;
> uvc->desc.fs_control = opts->fs_control;
> uvc->desc.ss_control = opts->ss_control;
> uvc->desc.fs_streaming = opts->fs_streaming;
> uvc->desc.hs_streaming = opts->hs_streaming;
> uvc->desc.ss_streaming = opts->ss_streaming;
> + mutex_unlock(&opts->lock);
> +
> + uvc->state = UVC_STATE_DISCONNECTED;
>
> /* Register the function. */
> uvc->func.name = "uvc";
> diff --git a/drivers/usb/gadget/u_uvc.h b/drivers/usb/gadget/u_uvc.h
> index 1909ad5..8277fdf 100644
> --- a/drivers/usb/gadget/u_uvc.h
> +++ b/drivers/usb/gadget/u_uvc.h
> @@ -29,6 +29,25 @@ struct f_uvc_opts {
> const struct uvc_descriptor_header * const *fs_streaming;
> const struct uvc_descriptor_header * const *hs_streaming;
> const struct uvc_descriptor_header * const *ss_streaming;
> +
> + /*
> + * Read/write access to configfs attributes is handled by configfs.
> + *
> + * This is to protect the data from concurrent access by read/write
> + * and create symlink/remove symlink.
This sounds like a pretty common problem to me, shouldn't it be handled by the
configfs core ?
> + */
> + struct mutex lock;
> + int refcnt;
> + /*
> + * In uvc function's configfs directory there will be symbolic links.
> + * The allowed targets are in the "known_targets" list.
> + */
> + struct list_head known_targets;
> + struct config_item *fs_class;
> + struct config_item *ss_class;
> + struct config_item *fs_streaming_class;
> + struct config_item *ss_streaming_class;
> + struct config_item *hs_streaming_class;
> };
>
> void uvc_set_trace_param(unsigned int uvc_gadget_trace_param_webcam);
> diff --git a/drivers/usb/gadget/uvc_configfs.c
> b/drivers/usb/gadget/uvc_configfs.c new file mode 100644
> index 0000000..2031114
> --- /dev/null
> +++ b/drivers/usb/gadget/uvc_configfs.c
> @@ -0,0 +1,2928 @@
> +/*
> + * uvc_configfs.c
> + *
> + * Configfs hierarchy definitions for the uvc function
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include "uvc_configfs.h"
> +#include "configfs.h"
> +#include "u_uvc.h"
Please sort the headers here too.
I'll have to learn about configfs to understand the code below, that will take
a bit more time. I propose sorting out the rest of the issues I've pointed out
for the patch set and work on getting patches 1/8 to 7/8 mainlined in the
meantime.
> diff --git a/drivers/usb/gadget/uvc_configfs.h
> b/drivers/usb/gadget/uvc_configfs.h new file mode 100644
> index 0000000..13f554f
> --- /dev/null
> +++ b/drivers/usb/gadget/uvc_configfs.h
> @@ -0,0 +1,283 @@
> +/*
> + * uvc_configfs.h
> + *
> + * Configfs hierarchy definitions for the uvc function
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef UVC_CONFIGFS_H
> +#define UVC_CONFIGFS_H
The UVC gadget headers are guarded by
_F_UVC_H_
U_UVC_H
_UVC_GADGET_H_
UVC_CONFIGFS_H
_UVC_QUEUE_H_
__UVC__V4L2__H__
__UVC__VIDEO__H__
I propose standardizing it to
__F_UVC_H__
__U_UVC_H__
__UVC_GADGET_H__
__UVC_CONFIGFS_H__
__UVC_QUEUE_H__
__UVC_V4L2_H__
__UVC_VIDEO_H__
or
_F_UVC_H_
_U_UVC_H_
_UVC_GADGET_H_
_UVC_CONFIGFS_H_
_UVC_QUEUE_H_
_UVC_V4L2_H_
_UVC_VIDEO_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/configfs.h>
> +#include <linux/usb/video.h>
Usual comment :-)
[snip]
> +
> +#endif /* UVC_CONFIGFS_H */
--
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
