Hi Dan,
Thank you for the patch.
On Mon, Dec 19, 2022 at 02:43:12PM +0000, Daniel Scally wrote:
> Color matching descriptors are meant to be a per-format piece of data
> and we need to be able to support different descriptors for different
> formats. As a preliminary step towards that goal, switch the default
> color matching configfs functionality to point to an instance of a
> new struct uvcg_color_matching. Use the same default values for its
> attributes as the currently hard-coded ones so that the interface to
> userspace is consistent.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
>
> - Renamed uvcg_cmd to uvcg_color_matching plus the associated
> variables (Kieran, Laurent)
> - Added a refcnt member to struct uvcg_color_matching
>
> drivers/usb/gadget/function/uvc_configfs.c | 58 ++++++++++++++++------
> drivers/usb/gadget/function/uvc_configfs.h | 9 ++++
> 2 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index e28becd435bf..147d3def24dd 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -13,6 +13,7 @@
> #include "uvc_configfs.h"
>
> #include <linux/sort.h>
> +#include <uapi/linux/usb/video.h>
Drop "uapi/", the kernel should handle that automatically.
>
> /* -----------------------------------------------------------------------------
> * Global Utility Structures and Macros
> @@ -1788,20 +1789,21 @@ static ssize_t uvcg_color_matching_##cname##_show( \
> struct config_item *item, char *page) \
> { \
> struct config_group *group = to_config_group(item); \
> + struct uvcg_color_matching *color_match = \
> + to_uvcg_color_matching(group); \
> struct f_uvc_opts *opts; \
> struct config_item *opts_item; \
> struct mutex *su_mutex = &group->cg_subsys->su_mutex; \
> - struct uvc_color_matching_descriptor *cd; \
> int result; \
> \
> mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
> \
> opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
> opts = to_f_uvc_opts(opts_item); \
> - cd = &opts->uvc_color_matching; \
> \
> mutex_lock(&opts->lock); \
> - result = sprintf(page, "%u\n", le##bits##_to_cpu(cd->aname)); \
> + result = sprintf(page, "%u\n", \
> + le##bits##_to_cpu(color_match->desc.aname)); \
> mutex_unlock(&opts->lock); \
> \
> mutex_unlock(su_mutex); \
> @@ -1823,29 +1825,57 @@ static struct configfs_attribute *uvcg_color_matching_attrs[] = {
> NULL,
> };
>
> -static const struct uvcg_config_group_type uvcg_color_matching_type = {
> - .type = {
> - .ct_item_ops = &uvcg_config_item_ops,
> - .ct_attrs = uvcg_color_matching_attrs,
> - .ct_owner = THIS_MODULE,
> - },
> - .name = "default",
> +static void uvcg_color_matching_release(struct config_item *item)
> +{
> + struct uvcg_color_matching *color_match =
> + to_uvcg_color_matching(to_config_group(item));
> +
> + kfree(color_match);
> +}
> +
> +static struct configfs_item_operations uvcg_color_matching_item_ops = {
> + .release = uvcg_color_matching_release,
> +};
> +
> +static const struct config_item_type uvcg_color_matching_type = {
> + .ct_item_ops = &uvcg_color_matching_item_ops,
> + .ct_attrs = uvcg_color_matching_attrs,
> + .ct_owner = THIS_MODULE,
> };
>
> /* -----------------------------------------------------------------------------
> * streaming/color_matching
> */
>
> +static int uvcg_color_matching_create_children(struct config_group *parent)
> +{
> + struct uvcg_color_matching *color_match;
> +
> + color_match = kzalloc(sizeof(*color_match), GFP_KERNEL);
> + if (!color_match)
> + return -ENOMEM;
> +
> + color_match->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> + color_match->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> + color_match->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> + color_match->desc.bColorPrimaries = UVC_COLOR_PRIMARIES_BT_709_SRGB;
> + color_match->desc.bTransferCharacteristics = UVC_TRANSFER_CHARACTERISTICS_BT_709;
> + color_match->desc.bMatrixCoefficients = UVC_MATRIX_COEFFICIENTS_SMPTE_170M;
> +
> + config_group_init_type_name(&color_match->group, "default",
> + &uvcg_color_matching_type);
> + configfs_add_default_group(&color_match->group, parent);
> +
> + return 0;
> +}
> +
> static const struct uvcg_config_group_type uvcg_color_matching_grp_type = {
> .type = {
> .ct_item_ops = &uvcg_config_item_ops,
> .ct_owner = THIS_MODULE,
> },
> .name = "color_matching",
> - .children = (const struct uvcg_config_group_type*[]) {
> - &uvcg_color_matching_type,
> - NULL,
> - },
> + .create_children = uvcg_color_matching_create_children,
> };
>
> /* -----------------------------------------------------------------------------
> diff --git a/drivers/usb/gadget/function/uvc_configfs.h b/drivers/usb/gadget/function/uvc_configfs.h
> index ad2ec8c4c78c..c7392c9b840e 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.h
> +++ b/drivers/usb/gadget/function/uvc_configfs.h
> @@ -37,6 +37,15 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
> return container_of(item, struct uvcg_control_header, item);
> }
>
> +struct uvcg_color_matching {
> + struct config_group group;
> + struct uvc_color_matching_descriptor desc;
> + unsigned int refcnt;
As refcnt isn't used in this patch, I would add it to the structure in
the patch that uses it.
Conditionally-Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
with these small issues fixed.
> +};
> +
> +#define to_uvcg_color_matching(group_ptr) \
> +container_of(group_ptr, struct uvcg_color_matching, group)
> +
> enum uvcg_format_type {
> UVCG_UNCOMPRESSED = 0,
> UVCG_MJPEG,
--
Regards,
Laurent Pinchart
