Hi Michael,
Thank you for the patch.
On Mon, May 31, 2021 at 12:22:37AM +0200, Michael Grzeschik wrote:
> The functions and structs of the configfs interface should also be used
> by the uvc gadget driver. This patch prepares the stack by moving the
> common structs and functions to the common header file.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 111 -------------------
> drivers/usb/gadget/function/uvc_configfs.h | 119 +++++++++++++++++++++
> 2 files changed, 119 insertions(+), 111 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 77d64031aa9c2..86463bb2639ed 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -19,8 +19,6 @@
> * Global Utility Structures and Macros
> */
>
> -#define UVCG_STREAMING_CONTROL_SIZE 1
> -
> #define UVC_ATTR(prefix, cname, aname) \
> static struct configfs_attribute prefix##attr_##cname = { \
> .ca_name = __stringify(aname), \
> @@ -49,12 +47,6 @@ static int uvcg_config_compare_u32(const void *l, const void *r)
> return li < ri ? -1 : li == ri ? 0 : 1;
> }
>
> -static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
> -{
> - return container_of(to_config_group(item), struct f_uvc_opts,
> - func_inst.group);
> -}
> -
> struct uvcg_config_group_type {
> struct config_item_type type;
> const char *name;
> @@ -125,19 +117,6 @@ static void uvcg_config_remove_children(struct config_group *group)
> * control/header
> */
>
> -DECLARE_UVC_HEADER_DESCRIPTOR(1);
> -
> -struct uvcg_control_header {
> - struct config_item item;
> - struct UVC_HEADER_DESCRIPTOR(1) desc;
> - unsigned linked;
> -};
> -
> -static struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
> -{
> - return container_of(item, struct uvcg_control_header, item);
> -}
> -
> #define UVCG_CTRL_HDR_ATTR(cname, aname, bits, limit) \
> static ssize_t uvcg_control_header_##cname##_show( \
> struct config_item *item, char *page) \
> @@ -764,29 +743,6 @@ static const struct uvcg_config_group_type uvcg_control_grp_type = {
> * streaming/mjpeg
> */
>
> -static const char * const uvcg_format_names[] = {
> - "uncompressed",
> - "mjpeg",
> -};
> -
> -enum uvcg_format_type {
> - UVCG_UNCOMPRESSED = 0,
> - UVCG_MJPEG,
> -};
> -
> -struct uvcg_format {
> - struct config_group group;
> - enum uvcg_format_type type;
> - unsigned linked;
> - unsigned num_frames;
> - __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> -};
> -
> -static struct uvcg_format *to_uvcg_format(struct config_item *item)
> -{
> - return container_of(to_config_group(item), struct uvcg_format, group);
> -}
> -
> static ssize_t uvcg_format_bma_controls_show(struct uvcg_format *f, char *page)
> {
> struct f_uvc_opts *opts;
> @@ -845,29 +801,11 @@ static ssize_t uvcg_format_bma_controls_store(struct uvcg_format *ch,
> return ret;
> }
>
> -struct uvcg_format_ptr {
> - struct uvcg_format *fmt;
> - struct list_head entry;
> -};
> -
> /* -----------------------------------------------------------------------------
> * streaming/header/<NAME>
> * streaming/header
> */
>
> -struct uvcg_streaming_header {
> - struct config_item item;
> - struct uvc_input_header_descriptor desc;
> - unsigned linked;
> - struct list_head formats;
> - unsigned num_fmt;
> -};
> -
> -static struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
> -{
> - return container_of(item, struct uvcg_streaming_header, item);
> -}
> -
> static void uvcg_format_set_indices(struct config_group *fmt);
>
> static int uvcg_streaming_header_allow_link(struct config_item *src,
> @@ -1059,31 +997,6 @@ static const struct uvcg_config_group_type uvcg_streaming_header_grp_type = {
> * streaming/<mode>/<format>/<NAME>
> */
>
> -struct uvcg_frame {
> - struct config_item item;
> - enum uvcg_format_type fmt_type;
> - struct {
> - u8 b_length;
> - u8 b_descriptor_type;
> - u8 b_descriptor_subtype;
> - u8 b_frame_index;
> - u8 bm_capabilities;
> - u16 w_width;
> - u16 w_height;
> - u32 dw_min_bit_rate;
> - u32 dw_max_bit_rate;
> - u32 dw_max_video_frame_buffer_size;
> - u32 dw_default_frame_interval;
> - u8 b_frame_interval_type;
> - } __attribute__((packed)) frame;
> - u32 *dw_frame_interval;
> -};
> -
> -static struct uvcg_frame *to_uvcg_frame(struct config_item *item)
> -{
> - return container_of(item, struct uvcg_frame, item);
> -}
> -
> #define UVCG_FRAME_ATTR(cname, aname, bits) \
> static ssize_t uvcg_frame_##cname##_show(struct config_item *item, char *page)\
> { \
> @@ -1420,18 +1333,6 @@ static void uvcg_format_set_indices(struct config_group *fmt)
> * streaming/uncompressed/<NAME>
> */
>
> -struct uvcg_uncompressed {
> - struct uvcg_format fmt;
> - struct uvc_format_uncompressed desc;
> -};
> -
> -static struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
> -{
> - return container_of(
> - container_of(to_config_group(item), struct uvcg_format, group),
> - struct uvcg_uncompressed, fmt);
> -}
> -
> static struct configfs_group_operations uvcg_uncompressed_group_ops = {
> .make_item = uvcg_frame_make,
> .drop_item = uvcg_frame_drop,
> @@ -1669,18 +1570,6 @@ static const struct uvcg_config_group_type uvcg_uncompressed_grp_type = {
> * streaming/mjpeg/<NAME>
> */
>
> -struct uvcg_mjpeg {
> - struct uvcg_format fmt;
> - struct uvc_format_mjpeg desc;
> -};
> -
> -static struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
> -{
> - return container_of(
> - container_of(to_config_group(item), struct uvcg_format, group),
> - struct uvcg_mjpeg, fmt);
> -}
> -
> static struct configfs_group_operations uvcg_mjpeg_group_ops = {
> .make_item = uvcg_frame_make,
> .drop_item = uvcg_frame_drop,
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index 7e1d7ca29bf21..f905d29570eb4 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -16,4 +16,123 @@ struct f_uvc_opts;
>
> int uvcg_attach_configfs(struct f_uvc_opts *opts);
Function declarations are traditionally placed after structure
definitions, I would thus move this line to the end of the file.
>
> +static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item)
Headers should be self-contained as much as possible. This means that
you need forward declarations and/or #include statements corresponding
to the structure and functions used herein. A very simple way to test
this is to apply the following change:
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index cd28dec837dd..5303b790911b 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -10,10 +10,11 @@
* Author: Andrzej Pietrasiewicz <andrzejtp2010@xxxxxxxxx>
*/
+#include "uvc_configfs.h"
+
#include <linux/sort.h>
#include "u_uvc.h"
-#include "uvc_configfs.h"
/* -----------------------------------------------------------------------------
* Global Utility Structures and Macros
Compilation will fail if uvc_configfs.h isn't self-contained. Feel free
to squash the above diff in this patch.
> +{
> + return container_of(to_config_group(item), struct f_uvc_opts,
> + func_inst.group);
> +}
> +
> +#define UVCG_STREAMING_CONTROL_SIZE 1
> +
> +DECLARE_UVC_HEADER_DESCRIPTOR(1);
> +
> +struct uvcg_control_header {
> + struct config_item item;
> + struct UVC_HEADER_DESCRIPTOR(1) desc;
> + unsigned linked;
> +};
> +
> +static inline struct uvcg_control_header *to_uvcg_control_header(struct config_item *item)
> +{
> + return container_of(item, struct uvcg_control_header, item);
> +}
> +
> +static const char * const uvcg_format_names[] = {
> + "uncompressed",
> + "mjpeg",
> +};
> +
> +enum uvcg_format_type {
> + UVCG_UNCOMPRESSED = 0,
> + UVCG_MJPEG,
> +};
> +
> +struct uvcg_format {
> + struct config_group group;
> + enum uvcg_format_type type;
> + unsigned linked;
> + unsigned num_frames;
> + __u8 bmaControls[UVCG_STREAMING_CONTROL_SIZE];
> +};
> +
> +struct uvcg_format_ptr {
> + struct uvcg_format *fmt;
> + struct list_head entry;
> +};
> +
> +static inline struct uvcg_format *to_uvcg_format(struct config_item *item)
> +{
> + return container_of(to_config_group(item), struct uvcg_format, group);
> +}
> +
> +struct uvcg_streaming_header {
> + struct config_item item;
> + struct uvc_input_header_descriptor desc;
> + unsigned linked;
> + struct list_head formats;
> + unsigned num_fmt;
> +};
> +
> +static inline struct uvcg_streaming_header *to_uvcg_streaming_header(struct config_item *item)
> +{
> + return container_of(item, struct uvcg_streaming_header, item);
> +}
> +
> +struct uvcg_frame {
> + struct config_item item;
> + enum uvcg_format_type fmt_type;
> + struct {
> + u8 b_length;
> + u8 b_descriptor_type;
> + u8 b_descriptor_subtype;
> + u8 b_frame_index;
> + u8 bm_capabilities;
> + u16 w_width;
> + u16 w_height;
> + u32 dw_min_bit_rate;
> + u32 dw_max_bit_rate;
> + u32 dw_max_video_frame_buffer_size;
> + u32 dw_default_frame_interval;
> + u8 b_frame_interval_type;
> + } __attribute__((packed)) frame;
> + u32 *dw_frame_interval;
> +};
> +
> +static inline struct uvcg_frame *to_uvcg_frame(struct config_item *item)
> +{
> + return container_of(item, struct uvcg_frame, item);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * streaming/uncompressed/<NAME>
> + */
> +
> +struct uvcg_uncompressed {
> + struct uvcg_format fmt;
> + struct uvc_format_uncompressed desc;
> +};
> +
> +static inline struct uvcg_uncompressed *to_uvcg_uncompressed(struct config_item *item)
> +{
> + return container_of(
> + container_of(to_config_group(item), struct uvcg_format, group),
> + struct uvcg_uncompressed, fmt);
While at it, how about writing this
return container_of(to_uvcg_format(item), struct uvcg_uncompressed, fmt);
?
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * streaming/mjpeg/<NAME>
> + */
> +
> +struct uvcg_mjpeg {
> + struct uvcg_format fmt;
> + struct uvc_format_mjpeg desc;
> +};
> +
> +static inline struct uvcg_mjpeg *to_uvcg_mjpeg(struct config_item *item)
> +{
> + return container_of(
> + container_of(to_config_group(item), struct uvcg_format, group),
> + struct uvcg_mjpeg, fmt);
Same here with
return container_of(to_uvcg_format(item), struct uvcg_mjpeg, fmt);
With these issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> +}
> +
> #endif /* UVC_CONFIGFS_H */
--
Regards,
Laurent Pinchart
