Hi Andrzej,
Thank you for the patch.
git am complained about
Applying: usb/gadget: f_uvc: convert f_uvc to new function interface
/home/laurent/src/kernel/media.git/.git/rebase-apply/patch:20: trailing
whitespace.
/home/laurent/src/kernel/media.git/.git/rebase-apply/patch:450: new blank line
at EOF.
+
/home/laurent/src/kernel/media.git/.git/rebase-apply/patch:494: new blank line
at EOF.
+
warning: 3 lines add whitespace errors.
Please see below for other comments.
On Friday 28 February 2014 10:32:26 Andrzej Pietrasiewicz wrote:
> Use the new function registration interface. It is required
> in order to integrate configfs support.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>
> ---
> drivers/usb/gadget/Kconfig | 3 +
> drivers/usb/gadget/Makefile | 2 +
> drivers/usb/gadget/f_uvc.c | 252 ++++++++++++++++++++++++++++++++---------
> drivers/usb/gadget/u_uvc.h | 37 +++++++
> drivers/usb/gadget/webcam.c | 1 +
> 5 files changed, 242 insertions(+), 53 deletions(-)
> create mode 100644 drivers/usb/gadget/u_uvc.h
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 46ba05f..fc9face 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -563,6 +563,9 @@ config USB_F_MASS_STORAGE
> config USB_F_FS
> tristate
>
> +config USB_F_UVC
> + tristate
> +
> choice
> tristate "USB Gadget Drivers"
> default USB_ETH
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index f73b48c..fb30084 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -64,6 +64,8 @@ usb_f_mass_storage-y := f_mass_storage.o
storage_common.o
> obj-$(CONFIG_USB_F_MASS_STORAGE)+= usb_f_mass_storage.o
> usb_f_fs-y := f_fs.o
> obj-$(CONFIG_USB_F_FS) += usb_f_fs.o
> +usb_f_uvc-y := f_uvc.o
> +obj-$(CONFIG_USB_F_UVC) += usb_f_uvc.o
>
> #
> # USB gadget drivers
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> index bd47af4..dfd8de33 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -11,6 +11,7 @@
> */
>
> #include <linux/kernel.h>
> +#include <linux/module.h>
> #include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/fs.h>
> @@ -27,6 +28,7 @@
> #include <media/v4l2-event.h>
>
> #include "uvc.h"
> +#include "u_uvc.h"
> #include "uvc_video.h"
> #include "uvc_v4l2.h"
>
> @@ -65,7 +67,7 @@ static struct usb_gadget_strings *uvc_function_strings[] =
> {
>
> #define UVC_STATUS_MAX_PACKET_SIZE 16 /* 16 bytes status */
>
> -static struct usb_interface_assoc_descriptor uvc_iad __initdata = {
> +static struct usb_interface_assoc_descriptor uvc_iad = {
> .bLength = sizeof(uvc_iad),
> .bDescriptorType = USB_DT_INTERFACE_ASSOCIATION,
> .bFirstInterface = 0,
> @@ -76,7 +78,7 @@ static struct usb_interface_assoc_descriptor uvc_iad
> __initdata = { .iFunction = 0,
> };
>
> -static struct usb_interface_descriptor uvc_control_intf __initdata = {
> +static struct usb_interface_descriptor uvc_control_intf = {
> .bLength = USB_DT_INTERFACE_SIZE,
> .bDescriptorType = USB_DT_INTERFACE,
> .bInterfaceNumber = UVC_INTF_VIDEO_CONTROL,
> @@ -88,7 +90,7 @@ static struct usb_interface_descriptor uvc_control_intf
> __initdata = { .iInterface = 0,
> };
>
> -static struct usb_endpoint_descriptor uvc_control_ep __initdata = {
> +static struct usb_endpoint_descriptor uvc_control_ep = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> .bEndpointAddress = USB_DIR_IN,
> @@ -97,7 +99,7 @@ static struct usb_endpoint_descriptor uvc_control_ep
> __initdata = { .bInterval = 8,
> };
>
> -static struct usb_ss_ep_comp_descriptor uvc_ss_control_comp __initdata = {
> +static struct usb_ss_ep_comp_descriptor uvc_ss_control_comp = {
> .bLength = sizeof(uvc_ss_control_comp),
> .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> /* The following 3 values can be tweaked if necessary. */
> @@ -106,14 +108,14 @@ static struct usb_ss_ep_comp_descriptor
> uvc_ss_control_comp __initdata = { .wBytesPerInterval =
> cpu_to_le16(UVC_STATUS_MAX_PACKET_SIZE),
> };
>
> -static struct uvc_control_endpoint_descriptor uvc_control_cs_ep __initdata
> = { +static struct uvc_control_endpoint_descriptor uvc_control_cs_ep = {
> .bLength = UVC_DT_CONTROL_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_CS_ENDPOINT,
> .bDescriptorSubType = UVC_EP_INTERRUPT,
> .wMaxTransferSize = cpu_to_le16(UVC_STATUS_MAX_PACKET_SIZE),
> };
>
> -static struct usb_interface_descriptor uvc_streaming_intf_alt0 __initdata =
> { +static struct usb_interface_descriptor uvc_streaming_intf_alt0 = {
> .bLength = USB_DT_INTERFACE_SIZE,
> .bDescriptorType = USB_DT_INTERFACE,
> .bInterfaceNumber = UVC_INTF_VIDEO_STREAMING,
> @@ -125,7 +127,7 @@ static struct usb_interface_descriptor
> uvc_streaming_intf_alt0 __initdata = { .iInterface = 0,
> };
>
> -static struct usb_interface_descriptor uvc_streaming_intf_alt1 __initdata =
> { +static struct usb_interface_descriptor uvc_streaming_intf_alt1 = {
> .bLength = USB_DT_INTERFACE_SIZE,
> .bDescriptorType = USB_DT_INTERFACE,
> .bInterfaceNumber = UVC_INTF_VIDEO_STREAMING,
> @@ -137,7 +139,7 @@ static struct usb_interface_descriptor
> uvc_streaming_intf_alt1 __initdata = { .iInterface = 0,
> };
>
> -static struct usb_endpoint_descriptor uvc_fs_streaming_ep __initdata = {
> +static struct usb_endpoint_descriptor uvc_fs_streaming_ep = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> .bEndpointAddress = USB_DIR_IN,
> @@ -148,7 +150,7 @@ static struct usb_endpoint_descriptor
> uvc_fs_streaming_ep __initdata = { */
> };
>
> -static struct usb_endpoint_descriptor uvc_hs_streaming_ep __initdata = {
> +static struct usb_endpoint_descriptor uvc_hs_streaming_ep = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> .bEndpointAddress = USB_DIR_IN,
> @@ -159,7 +161,7 @@ static struct usb_endpoint_descriptor
> uvc_hs_streaming_ep __initdata = { */
> };
>
> -static struct usb_endpoint_descriptor uvc_ss_streaming_ep __initdata = {
> +static struct usb_endpoint_descriptor uvc_ss_streaming_ep = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
>
> @@ -171,7 +173,7 @@ static struct usb_endpoint_descriptor
> uvc_ss_streaming_ep __initdata = { */
> };
>
> -static struct usb_ss_ep_comp_descriptor uvc_ss_streaming_comp __initdata =
> { +static struct usb_ss_ep_comp_descriptor uvc_ss_streaming_comp = {
> .bLength = sizeof(uvc_ss_streaming_comp),
> .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> /* The bMaxBurst, bmAttributes and wBytesPerInterval values will be
> @@ -198,6 +200,16 @@ static const struct usb_descriptor_header * const
> uvc_ss_streaming[] = { NULL,
> };
>
> +#ifndef USBF_UVC_INCLUDED
> +
> +void uvc_set_trace_param(unsigned int uvc_gadget_trace_param_webcam)
How about naming the function parameter just "trace" ?
> +{
> + uvc_gadget_trace_param = uvc_gadget_trace_param_webcam;
> +}
> +EXPORT_SYMBOL(uvc_set_trace_param);
> +
> +#endif
> +
> /* ------------------------------------------------------------------------
> * Control requests
> */
> @@ -424,7 +436,7 @@ uvc_register_video(struct uvc_device *uvc)
> } \
> } while (0)
>
> -static struct usb_descriptor_header ** __init
> +static struct usb_descriptor_header **
> uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed)
> {
> struct uvc_input_header_descriptor *uvc_streaming_header;
> @@ -544,29 +556,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed) return hdr;
> }
>
> -static void
> -uvc_function_unbind(struct usb_configuration *c, struct usb_function *f)
> -{
> - struct usb_composite_dev *cdev = c->cdev;
> - struct uvc_device *uvc = to_uvc(f);
> -
> - INFO(cdev, "uvc_function_unbind\n");
> -
> - video_unregister_device(uvc->vdev);
> - v4l2_device_unregister(&uvc->v4l2_dev);
> - uvc->control_ep->driver_data = NULL;
> - uvc->video.ep->driver_data = NULL;
> -
> - uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id = 0;
> - usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
> - kfree(uvc->control_buf);
> -
> - usb_free_all_descriptors(f);
> -
> - kfree(uvc);
> -}
> -
To minimize the diffstat and make the patch more readable, can't you leave the
function here and just add an #ifdef USBF_UVC_INCLUDED guard around it ?
> -static int __init
> +static int
> uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> {
> struct usb_composite_dev *cdev = c->cdev;
> @@ -574,10 +564,14 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f) unsigned int max_packet_mult;
> unsigned int max_packet_size;
> struct usb_ep *ep;
> +#ifndef USBF_UVC_INCLUDED
> + struct f_uvc_opts *opts;
> +#endif
> int ret = -EINVAL;
>
> INFO(cdev, "uvc_function_bind\n");
>
> +#ifdef USBF_UVC_INCLUDED
> /* Sanity check the streaming endpoint module parameters.
> */
> streaming_interval = clamp(streaming_interval, 1U, 16U);
> @@ -614,6 +608,46 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f) uvc_ss_streaming_comp.bMaxBurst = streaming_maxburst;
> uvc_ss_streaming_comp.wBytesPerInterval =
> max_packet_size * max_packet_mult * streaming_maxburst;
> +#else
> + opts = container_of(f->fi, struct f_uvc_opts, func_inst);
> + /* Sanity check the streaming endpoint module parameters.
> + */
> + opts->streaming_interval = clamp(opts->streaming_interval, 1U, 16U);
> + opts->streaming_maxpacket = clamp(opts->streaming_maxpacket, 1U, 3072U);
> + opts->streaming_maxburst = min(opts->streaming_maxburst, 15U);
> +
> + /* Fill in the FS/HS/SS Video Streaming specific descriptors from the
> + * module parameters.
> + *
> + * NOTE: We assume that the user knows what they are doing and won't
> + * give parameters that their UDC doesn't support.
> + */
> + if (opts->streaming_maxpacket <= 1024) {
> + max_packet_mult = 1;
> + max_packet_size = opts->streaming_maxpacket;
> + } else if (opts->streaming_maxpacket <= 2048) {
> + max_packet_mult = 2;
> + max_packet_size = opts->streaming_maxpacket / 2;
> + } else {
> + max_packet_mult = 3;
> + max_packet_size = opts->streaming_maxpacket / 3;
> + }
> +
> + uvc_fs_streaming_ep.wMaxPacketSize =
> + min(opts->streaming_maxpacket, 1023U);
> + uvc_fs_streaming_ep.bInterval = opts->streaming_interval;
> +
> + uvc_hs_streaming_ep.wMaxPacketSize = max_packet_size;
> + uvc_hs_streaming_ep.wMaxPacketSize |= ((max_packet_mult - 1) << 11);
> + uvc_hs_streaming_ep.bInterval = opts->streaming_interval;
> +
> + uvc_ss_streaming_ep.wMaxPacketSize = max_packet_size;
> + uvc_ss_streaming_ep.bInterval = opts->streaming_interval;
> + uvc_ss_streaming_comp.bmAttributes = max_packet_mult - 1;
> + uvc_ss_streaming_comp.bMaxBurst = opts->streaming_maxburst;
> + uvc_ss_streaming_comp.wBytesPerInterval =
> + max_packet_size * max_packet_mult * opts->streaming_maxburst;
> +#endif
>
> /* Allocate endpoints. */
> ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
> @@ -643,6 +677,23 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f) uvc_hs_streaming_ep.bEndpointAddress =
> uvc->video.ep->address;
> uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
>
> + /* String descriptors are global, we only need to allocate string IDs
> + * for the first UVC function. UVC functions beyond the first (if any)
> + * will reuse the same IDs.
> + */
> + if (uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id == 0) {
> + ret = usb_string_ids_tab(c->cdev, uvc_en_us_strings);
> + if (ret)
> + goto error;
> + uvc_iad.iFunction =
> + uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id;
> + uvc_control_intf.iInterface =
> + uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id;
> + ret = uvc_en_us_strings[UVC_STRING_STREAMING_IDX].id;
> + uvc_streaming_intf_alt0.iInterface = ret;
> + uvc_streaming_intf_alt1.iInterface = ret;
> + }
> +
> /* Allocate interface IDs. */
> if ((ret = usb_interface_id(c, f)) < 0)
> goto error;
> @@ -722,6 +773,30 @@ error:
> * USB gadget function
> */
>
> +#ifdef USBF_UVC_INCLUDED
> +
> +static void
> +uvc_old_function_unbind(struct usb_configuration *c, struct usb_function
> *f) +{
> + struct usb_composite_dev *cdev = c->cdev;
> + struct uvc_device *uvc = to_uvc(f);
> +
> + INFO(cdev, "uvc_function_unbind\n");
> +
> + video_unregister_device(uvc->vdev);
> + v4l2_device_unregister(&uvc->v4l2_dev);
> + uvc->control_ep->driver_data = NULL;
> + uvc->video.ep->driver_data = NULL;
> +
> + uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id = 0;
> + usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
> + kfree(uvc->control_buf);
> +
> + usb_free_all_descriptors(f);
> +
> + kfree(uvc);
> +}
> +
> /**
> * uvc_bind_config - add a UVC function to a configuration
> * @c: the configuration to support the UVC instance
> @@ -790,28 +865,11 @@ uvc_bind_config(struct usb_configuration *c,
> uvc->desc.hs_streaming = hs_streaming;
> uvc->desc.ss_streaming = ss_streaming;
>
> - /* String descriptors are global, we only need to allocate string IDs
> - * for the first UVC function. UVC functions beyond the first (if any)
> - * will reuse the same IDs.
> - */
> - if (uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id == 0) {
> - ret = usb_string_ids_tab(c->cdev, uvc_en_us_strings);
> - if (ret)
> - goto error;
> - uvc_iad.iFunction =
> - uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id;
> - uvc_control_intf.iInterface =
> - uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id;
> - ret = uvc_en_us_strings[UVC_STRING_STREAMING_IDX].id;
> - uvc_streaming_intf_alt0.iInterface = ret;
> - uvc_streaming_intf_alt1.iInterface = ret;
> - }
> -
> /* Register the function. */
> uvc->func.name = "uvc";
> uvc->func.strings = uvc_function_strings;
> uvc->func.bind = uvc_function_bind;
> - uvc->func.unbind = uvc_function_unbind;
> + uvc->func.unbind = uvc_old_function_unbind;
I don't think you introduce a uvc_function_unbind() function later in this
patch set, so to minimize the diffstat you could avoid renaming the function.
> uvc->func.get_alt = uvc_function_get_alt;
> uvc->func.set_alt = uvc_function_set_alt;
> uvc->func.disable = uvc_function_disable;
> @@ -828,4 +886,92 @@ error:
> return ret;
> }
>
> +#else
> +
> +static void uvc_free_inst(struct usb_function_instance *f)
> +{
> + struct f_uvc_opts *opts;
> +
> + opts = container_of(f, struct f_uvc_opts, func_inst);
You're using container_of in a few places. How about adding a
#define to_f_uvv_opts(f) container_of(f, struct f_uvc_opts, func_inst)
macro to u_uvc.h ? You could then fold the call into the opts variable
declaration line.
> +
> + kfree(opts);
> +}
> +
> +static struct usb_function_instance *uvc_alloc_inst(void)
> +{
> + struct f_uvc_opts *opts;
> +
> + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> + if (!opts)
> + return ERR_PTR(-ENOMEM);
> + opts->func_inst.free_func_inst = uvc_free_inst;
> +
> + return &opts->func_inst;
> +}
> +
> +static void uvc_free(struct usb_function *f)
> +{
> + struct uvc_device *uvc;
> +
> + uvc = to_uvc(f);
You can fold this line into the variable declaration
struct uvc_device *uvc = to_uvc(f);
> + kfree(uvc);
> +}
> +
> +static void uvc_unbind(struct usb_configuration *c, struct usb_function *f)
> +{
> + struct usb_composite_dev *cdev = c->cdev;
> + struct uvc_device *uvc = to_uvc(f);
> +
> + INFO(cdev, "uvc_function_unbind\n");
This should be uvc_unbind, or maybe "%s\n", __func__.
> +
> + video_unregister_device(uvc->vdev);
> + v4l2_device_unregister(&uvc->v4l2_dev);
> + uvc->control_ep->driver_data = NULL;
> + uvc->video.ep->driver_data = NULL;
> +
> + uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id = 0;
> + usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
> + kfree(uvc->control_buf);
> +
> + usb_free_all_descriptors(f);
> +}
> +
> +struct usb_function *uvc_alloc(struct usb_function_instance *fi)
> +{
> + struct uvc_device *uvc;
> + struct f_uvc_opts *opts;
> +
> + uvc = kzalloc(sizeof(*uvc), GFP_KERNEL);
> + if (uvc == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + uvc->state = UVC_STATE_DISCONNECTED;
> + opts = container_of(fi, struct f_uvc_opts, func_inst);
> +
> + 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;
> +
> + /* Register the function. */
> + uvc->func.name = "uvc";
> + uvc->func.strings = uvc_function_strings;
> + uvc->func.bind = uvc_function_bind;
> + uvc->func.unbind = uvc_unbind;
> + uvc->func.get_alt = uvc_function_get_alt;
> + uvc->func.set_alt = uvc_function_set_alt;
> + uvc->func.disable = uvc_function_disable;
> + uvc->func.setup = uvc_function_setup;
> + uvc->func.free_func = uvc_free;
> +
> + return &uvc->func;
> +}
> +
> +DECLARE_USB_FUNCTION_INIT(uvc, uvc_alloc_inst, uvc_alloc);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Laurent Pinchart");
> +
> +#endif
> +
>
> diff --git a/drivers/usb/gadget/u_uvc.h b/drivers/usb/gadget/u_uvc.h
> new file mode 100644
> index 0000000..1909ad5
> --- /dev/null
> +++ b/drivers/usb/gadget/u_uvc.h
> @@ -0,0 +1,37 @@
> +/*
> + * u_uvc.h
> + *
> + * Utility definitions for the uvc function
> + *
> + * Copyright (c) 2013 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 U_UVC_H
> +#define U_UVC_H
> +
> +#include <linux/usb/composite.h>
> +
> +struct f_uvc_opts {
> + struct usb_function_instance func_inst;
> + unsigned int uvc_gadget_trace_param;
> + unsigned int streaming_interval;
> + unsigned int streaming_maxpacket;
> + unsigned int streaming_maxburst;
> + const struct uvc_descriptor_header * const *fs_control;
> + const struct uvc_descriptor_header * const *ss_control;
> + const struct uvc_descriptor_header * const *fs_streaming;
> + const struct uvc_descriptor_header * const *hs_streaming;
> + const struct uvc_descriptor_header * const *ss_streaming;
> +};
> +
> +void uvc_set_trace_param(unsigned int uvc_gadget_trace_param_webcam);
> +
> +#endif /* U_UVC_H */
> +
> diff --git a/drivers/usb/gadget/webcam.c b/drivers/usb/gadget/webcam.c
> index dfc9729..14a6acc2f 100644
> --- a/drivers/usb/gadget/webcam.c
> +++ b/drivers/usb/gadget/webcam.c
> @@ -22,6 +22,7 @@
> * the runtime footprint, and giving us at least some parts of what
> * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
> */
> +#define USBF_UVC_INCLUDED
> #include "f_uvc.c"
>
> USB_GADGET_COMPOSITE_OPTIONS();
--
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
