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 <andrze...@samsung.com>
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 264 +++
> drivers/usb/gadget/Kconfig | 11 +
> drivers/usb/gadget/function/Makefile | 2 +-
> drivers/usb/gadget/function/f_uvc.c | 122 +-
> drivers/usb/gadget/function/u_uvc.h | 23 +
> drivers/usb/gadget/function/uvc_configfs.c | 2439 ++++++++++++++++++
> drivers/usb/gadget/function/uvc_configfs.h | 23 +
> 7 files changed, 2879 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/ABI/testing/configfs-usb-gadget-uvc
> create mode 100644 drivers/usb/gadget/function/uvc_configfs.c
> create mode 100644 drivers/usb/gadget/function/uvc_configfs.h
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc new file mode 100644
> index 0000000..8f2825d
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -0,0 +1,264 @@
> +What: /config/usb-gadget/gadget/functions/uvc.name
> +Date: Nov 2014
> +KernelVersion: 3.18
This should be 3.19. My fault, sorry :-/
> +Description: UVC function directory
> +
> + streaming_maxburst - 0..15 (ss only)
> + streaming_maxpacket - 1..1023 (fs), 1..3072 (hs/ss)
> + streaming_interval - 1..16
> +
> +What: /config/usb-gadget/gadget/functions/uvc.name/control
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Control descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/class
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Class descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/class/ss
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Super speed control class descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/class/fs
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Full speed control class descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/terminal
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Terminal descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/control/terminal/output
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Output terminal descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/control/terminal/output
> /default +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Default output terminal descriptors
> +
> + All attributes read only:
> + iTerminal - index of string descriptor
> + bSourceID - id of the terminal to which this terminal
> + is connected
> + bAssocTerminal - id of the input terminal to which this output
> + terminal is associated
> + wTerminalType - terminal type
> + bTerminalID - a non-zero id of this terminal
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/control/terminal/camera
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Camera terminal descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/control/terminal/camera
> /default +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Default camera terminal descriptors
> +
> + All attributes read only:
> + bmControls - bitmap specifying which controls are
> + supported for the video stream
> + wOcularFocalLength - the value of Locular
> + wObjectiveFocalLengthMax- the value of Lmin
> + wObjectiveFocalLengthMin- the value of Lmax
> + iTerminal - index of string descriptor
> + bAssocTerminal - id of the output terminal to which
> + this terminal is connected
> + wTerminalType - terminal type
> + bTerminalID - a non-zero id of this terminal
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/processing
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Processing unit descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/control/processing/defa
> ult +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Default processing unit descriptors
> +
> + All attributes read only:
> + iProcessing - index of string descriptor
> + bmControls - bitmap specifying which controls are
> + supported for the video stream
> + wMaxMultiplier - maximum digital magnification x100
> + bSourceID - id of the terminal to which this unit is
> + connected
> + bUnitID - a non-zero id of this unit
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/header
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Control header descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/control/header/name
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Specific control header descriptors
> +
> +dwClockFrequency
> +bcdUVC
> +What: /config/usb-gadget/gadget/functions/uvc.name/streaming
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Streaming descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/streaming/class
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Streaming class descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/streaming/class/ss
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Super speed streaming class descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/streaming/class/hs
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: High speed streaming class descriptors
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/streaming/class/fs
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Full speed streaming class descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/color_matchin
> g +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Color matching descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/color_matchin
> g/default +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Default color matching descriptors
> +
> + All attributes read only:
> + bMatrixCoefficients - matrix used to compute luma and
> + chroma values from the color primaries
> + bTransferCharacteristics- optoelectronic transfer
> + characteristic of the source picutre,
> + also called the gamma function
> + bColorPrimaries - color primaries and the reference
> + white
> +
> +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" ?
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Specific MJPEG format descriptors
> +
> + All attributes read only,
> + except bmaControls and bDefaultFrameIndex:
> + bmaControls - this format's data for bmaControls in
> + the streaming header
> + bmInterfaceFlags - specifies interlace information,
> + read-only
> + bAspectRatioY - the X dimension of the picture aspect
> + ratio, read-only
> + bAspectRatioX - the Y dimension of the picture aspect
> + ratio, read-only
> + bmFlags - characteristics of this format
Should this be documented as read-only ?
> + bDefaultFrameIndex - optimum frame index for this stream
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/mjpeg/name/na
> me +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Specific MJPEG frame descriptors
> +
> + dwFrameInterval - indicates how frame interval can be
> + programmed; a number of values
> + separated by newline can be specified
> + dwDefaultFrameInterval - the frame interval the device would
> + like to use as default
> + dwMaxVideoFrameBufferSize- the maximum number of bytes the
> + compressor will produce for a video
> + frame or still image
> + dwMaxBitRate - the maximum bit rate at the shortest
> + frame interval in bps
> + dwMinBitRate - the minimum bit rate at the longest
> + frame interval in bps
> + wHeight - height of decoded bitmap frame in px
> + wWidth - width of decoded bitmam frame in px
> + bmCapabilities - still image support, fixed frame-rate
> + support
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/uncompressed
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Uncompressed format descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/uncompressed/
> name +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Specific uncompressed format descriptors
> +
> + bmaControls - this format's data for bmaControls in
> + the streaming header
> + bmInterfaceFlags - specifies interlace information,
> + read-only
> + bAspectRatioY - the X dimension of the picture aspect
> + ratio, read-only
> + bAspectRatioX - the Y dimension of the picture aspect
> + ratio, read-only
> + bDefaultFrameIndex - optimum frame index for this stream
> + bBitsPerPixel - number of bits per pixel used to
> + specify color in the decoded video
> + frame
> + guidFormat - globally unique id used to identify
> + stream-encoding format
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/uncompressed/
> name/name +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Specific uncompressed frame descriptors
> +
> + dwFrameInterval - indicates how frame interval can be
> + programmed; a number of values
> + separated by newline can be specified
> + dwDefaultFrameInterval - the frame interval the device would
> + like to use as default
> + dwMaxVideoFrameBufferSize- the maximum number of bytes the
> + compressor will produce for a video
> + frame or still image
> + dwMaxBitRate - the maximum bit rate at the shortest
> + frame interval in bps
> + dwMinBitRate - the minimum bit rate at the longest
> + frame interval in bps
> + wHeight - height of decoded bitmap frame in px
> + wWidth - width of decoded bitmam frame in px
> + bmCapabilities - still image support, fixed frame-rate
> + support
> +
> +What:
> /config/usb-gadget/gadget/functions/uvc.name/streaming/header
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Streaming header descriptors
> +
> +What: /config/usb-
gadget/gadget/functions/uvc.name/streaming/header/name
> +Date: Nov 2014
> +KernelVersion: 3.18
> +Description: Specific streaming header descriptors
> +
> + All attributes read only:
> + bTriggerUsage - how the host software will respond to
> + a hardware trigger interrupt event
> + bTriggerSupport - flag specifying if hardware
> + triggering is supported
> + bStillCaptureMethod - method of still image caputre
> + supported
> + bTerminalLink - id of the output terminal to which
> + the video endpoint of this interface
> + is connected
> + bmInfo - capabilities of this video streaming
> + interface
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index c4880fc..61b581f 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -362,6 +362,17 @@ config USB_CONFIGFS_F_FS
> implemented in kernel space (for instance Ethernet, serial or
> mass storage) and other are implemented in user space.
>
> +config USB_CONFIGFS_F_UVC
> + boolean "USB Webcam function"
> + depends on USB_CONFIGFS
> + depends on VIDEO_DEV
> + select VIDEOBUF2_VMALLOC
> + select USB_F_UVC
> + help
> + The Webcam function acts as a composite USB Audio and Video Class
> + device. It provides a userspace API to process UVC control requests
> + and stream video data to the host.
> +
> source "drivers/usb/gadget/legacy/Kconfig"
>
> endchoice
> diff --git a/drivers/usb/gadget/function/Makefile
> b/drivers/usb/gadget/function/Makefile index 90701aa..cc01497 100644
> --- a/drivers/usb/gadget/function/Makefile
> +++ b/drivers/usb/gadget/function/Makefile
> @@ -36,5 +36,5 @@ usb_f_uac1-y := f_uac1.o u_uac1.o
> obj-$(CONFIG_USB_F_UAC1) += usb_f_uac1.o
> usb_f_uac2-y := f_uac2.o
> obj-$(CONFIG_USB_F_UAC2) += usb_f_uac2.o
> -usb_f_uvc-y := f_uvc.o uvc_queue.o uvc_v4l2.o uvc_video.o
> +usb_f_uvc-y := f_uvc.o uvc_queue.o uvc_v4l2.o uvc_video.o
uvc_configfs.o
> obj-$(CONFIG_USB_F_UVC) += usb_f_uvc.o
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c index 71e5935..bebf708 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -31,6 +31,7 @@
> #include "uvc_v4l2.h"
> #include "uvc_video.h"
> #include "u_uvc.h"
> +#include "uvc_configfs.h"
Could you please keep headers sorted alphabetically ?
>
> unsigned int uvc_gadget_trace_param;
>
> @@ -474,6 +475,8 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed) uvc_streaming_std = uvc_fs_streaming;
> break;
> }
Please add a blank line here.
> + if (!uvc_control_desc || !uvc_streaming_cls)
> + return ERR_PTR(-ENODEV);
If I'm not mistaken this error occurs only when the user doesn't specify the
required descriptors, through code or configfs. As it's not restricted to
configfs, should it be split to a separate patch, or is an oops acceptable in
that case given that a bug is present in the uvc_alloc() caller ?
> /* Descriptors layout
> *
> @@ -666,10 +669,27 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
>
> /* Copy descriptors */
> f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> - if (gadget_is_dualspeed(cdev->gadget))
> + 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 (gadget_is_superspeed(c->cdev->gadget))
> + 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;
> + }
> + }
Isn't this a bug fix that should be split to a separate patch ?
> /* Preallocate control endpoint request. */
> uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0, GFP_KERNEL);
> @@ -729,7 +749,6 @@ error:
> /*
> --------------------------------------------------------------------------
> * USB gadget function
> */
> -
That's an unrelated change, and I think you can leave it out.
> static void uvc_free_inst(struct usb_function_instance *f)
> {
> struct f_uvc_opts *opts = fi_to_f_uvc_opts(f);
> @@ -740,12 +759,88 @@ static void uvc_free_inst(struct usb_function_instance
> *f) static struct usb_function_instance *uvc_alloc_inst(void)
> {
> struct f_uvc_opts *opts;
> + struct uvc_camera_terminal_descriptor *cd;
> + struct uvc_processing_unit_descriptor *pd;
> + struct uvc_output_terminal_descriptor *od;
> + struct uvc_color_matching_descriptor *md;
> + struct uvc_descriptor_header **ctl_cls;
>
> opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> if (!opts)
> return ERR_PTR(-ENOMEM);
> opts->func_inst.free_func_inst = uvc_free_inst;
> -
> + mutex_init(&opts->lock);
You're missing the corresponding mutex_destroy() call.
> + cd = &opts->uvc_camera_terminal;
> + cd->bLength = UVC_DT_CAMERA_TERMINAL_SIZE(3);
> + cd->bDescriptorType = USB_DT_CS_INTERFACE;
> + cd->bDescriptorSubType = UVC_VC_INPUT_TERMINAL;
> + cd->bTerminalID = 1;
> + cd->wTerminalType = cpu_to_le16(0x0201);
> + cd->bAssocTerminal = 0;
> + cd->iTerminal = 0;
> + cd->wObjectiveFocalLengthMin = cpu_to_le16(0);
> + cd->wObjectiveFocalLengthMax = cpu_to_le16(0);
> + cd->wOcularFocalLength = cpu_to_le16(0);
> + cd->bControlSize = 3;
> + cd->bmControls[0] = 2;
> + cd->bmControls[1] = 0;
> + cd->bmControls[2] = 0;
> +
> + pd = &opts->uvc_processing;
> + pd->bLength = UVC_DT_PROCESSING_UNIT_SIZE(2);
> + pd->bDescriptorType = USB_DT_CS_INTERFACE;
> + pd->bDescriptorSubType = UVC_VC_PROCESSING_UNIT;
> + pd->bUnitID = 2;
> + pd->bSourceID = 1;
> + pd->wMaxMultiplier = cpu_to_le16(16*1024);
> + pd->bControlSize = 2;
> + pd->bmControls[0] = 1;
> + pd->bmControls[1] = 0;
> + pd->iProcessing = 0;
> +
> + od = &opts->uvc_output_terminal;
> + od->bLength = UVC_DT_OUTPUT_TERMINAL_SIZE;
> + od->bDescriptorType = USB_DT_CS_INTERFACE;
> + od->bDescriptorSubType = UVC_VC_OUTPUT_TERMINAL;
> + od->bTerminalID = 3;
> + od->wTerminalType = cpu_to_le16(0x0101);
> + od->bAssocTerminal = 0;
> + od->bSourceID = 2;
> + od->iTerminal = 0;
> +
> + md = &opts->uvc_color_matching;
> + md->bLength = UVC_DT_COLOR_MATCHING_SIZE;
> + md->bDescriptorType = USB_DT_CS_INTERFACE;
> + md->bDescriptorSubType = UVC_VS_COLORFORMAT;
> + md->bColorPrimaries = 1;
> + md->bTransferCharacteristics = 1;
> + md->bMatrixCoefficients = 4;
> +
> + ctl_cls = opts->uvc_fs_control_cls;
> + ctl_cls[0] = ctl_cls[4] = NULL;
I find the code easier to read of you write a single assignment per line. It
would also be nice to add a small comment explaining how the code works, it's
getting pretty cryptic.
> + ctl_cls[1] = (struct uvc_descriptor_header *)cd;
> + ctl_cls[2] = (struct uvc_descriptor_header *)pd;
> + ctl_cls[3] = (struct uvc_descriptor_header *)od;
> + ctl_cls = opts->uvc_hs_control_cls;
> + ctl_cls[0] = ctl_cls[4] = NULL;
> + ctl_cls[1] = (struct uvc_descriptor_header *)cd;
> + ctl_cls[2] = (struct uvc_descriptor_header *)pd;
> + ctl_cls[3] = (struct uvc_descriptor_header *)od;
> + ctl_cls = opts->uvc_ss_control_cls;
> + ctl_cls[0] = ctl_cls[4] = NULL;
> + ctl_cls[1] = (struct uvc_descriptor_header *)cd;
> + ctl_cls[2] = (struct uvc_descriptor_header *)pd;
> + ctl_cls[3] = (struct uvc_descriptor_header *)od;
> + opts->fs_control =
> + (const struct uvc_descriptor_header * const *)ctl_cls;
> + opts->ss_control =
> + (const struct uvc_descriptor_header * const *)ctl_cls;
You're setting both opts->fs_control and opts->ss_control to opts-
>uvc_ss_control_cls. Is that correct ? opts->uvc_fs_control_cls and opts-
>uvc_hs_control_cls are both unused. uvc_hs_control_cls can probably be
removed, and opts->fs_control should be set to opts->uvc_fs_control_cls.
> +
> + opts->streaming_interval = 1;
> + opts->streaming_maxpacket = 1024;
> +
> + uvcg_attach_configfs(opts);
> return &opts->func_inst;
> }
>
> @@ -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;
> + }
> +
> 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);
>
> /* Register the function. */
> uvc->func.name = "uvc";
> diff --git a/drivers/usb/gadget/function/u_uvc.h
> b/drivers/usb/gadget/function/u_uvc.h index c0706a3..f655435 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -17,6 +17,7 @@
> #define U_UVC_H
>
> #include <linux/usb/composite.h>
> +#include <linux/usb/video.h>
>
> #define fi_to_f_uvc_opts(f) container_of(f, struct f_uvc_opts, func_inst)
>
> @@ -31,6 +32,28 @@ 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;
> +
> + /* default descriptors */
> + struct uvc_camera_terminal_descriptor uvc_camera_terminal;
> + struct uvc_processing_unit_descriptor uvc_processing;
> + struct uvc_output_terminal_descriptor uvc_output_terminal;
> + struct uvc_color_matching_descriptor uvc_color_matching;
> +
> + struct uvc_descriptor_header *uvc_fs_control_cls[5];
> + struct uvc_descriptor_header *uvc_hs_control_cls[5];
> + struct uvc_descriptor_header *uvc_ss_control_cls[5];
> + struct uvc_descriptor_header **uvc_fs_streaming_cls;
> + struct uvc_descriptor_header **uvc_hs_streaming_cls;
> + struct uvc_descriptor_header **uvc_ss_streaming_cls;
This is introducing multiple levels of indirection, making the code pretty
hard to understand. Could you please add comments to explain why they're
needed and how they're used ?
> + /*
> + * 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.
> + */
> + struct mutex lock;
> + int refcnt;
refcnt is never modified.
> };
>
> void uvc_set_trace_param(unsigned int trace);
> 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]
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h
> b/drivers/usb/gadget/function/uvc_configfs.h new file mode 100644
> index 0000000..53ca3cc
> --- /dev/null
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -0,0 +1,23 @@
> +/*
> + * uvc_configfs.h
> + *
> + * Configfs support for the uvc function.
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Author: Andrzej Pietrasiewicz <andrze...@samsung.com>
> + *
> + * 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
> +
> +#include <linux/configfs.h>
> +#include "u_uvc.h"
There's not need to include those two headers here. You can just add a forward
declaration:
struct f_uvc_opts;
> +
> +int uvcg_attach_configfs(struct f_uvc_opts *opts);
> +
> +#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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
