Re: [PATCH 2/6] usb: gadget: uvc: Add struct for color matching in configs

[Dan Scally <dan.scally@xxxxxxxxxxxxxxxx>]

Hi Kieran

On 15/12/2022 11:45, Kieran Bingham wrote:
> Quoting Daniel Scally (2022-12-13 08:37:32)
> > 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_cmd (for "color matching descriptor"). Use the same
> Hrm .. I can't see 'cmd' and not think 'command' ... but longer names
> are longer ...


Yeah. Naming things was never my strong suit...I couldn't think of a 
name of intermediate length that wasn't rubbish so it was either this or 
"uvcg_color_matching_descriptor" which is loooong.

> > 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>
> > ---
> >   drivers/usb/gadget/function/uvc_configfs.c | 55 ++++++++++++++++------
> >   drivers/usb/gadget/function/uvc_configfs.h |  8 ++++
> >   2 files changed, 49 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> > index 26d092790f12..9918e7b6a023 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.c
> > +++ b/drivers/usb/gadget/function/uvc_configfs.c
> > @@ -1788,20 +1788,19 @@ static ssize_t uvcg_color_matching_##cname##_show(                      \
> >          struct config_item *item, char *page)                           \
> >   {                                                                      \
> >          struct config_group *group = to_config_group(item);             \
> > +       struct uvcg_cmd *cmd = to_uvcg_cmd(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(cmd->desc.aname));\
> >          mutex_unlock(&opts->lock);                                      \
> >                                                                          \
> >          mutex_unlock(su_mutex);                                         \
> > @@ -1823,29 +1822,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_cmd *cmd;
> > +
> > +       cmd = to_uvcg_cmd(to_config_group(item));
> > +       kfree(cmd);
> > +}
> > +
> > +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_cmd *cmd;
> > +
> > +       cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> > +       if (!cmd)
> > +               return -ENOMEM;
> > +
> > +       cmd->desc.bLength = UVC_DT_COLOR_MATCHING_SIZE;
> > +       cmd->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> > +       cmd->desc.bDescriptorSubType = UVC_VS_COLORFORMAT;
> > +       cmd->desc.bColorPrimaries = 1;
> > +       cmd->desc.bTransferCharacteristics = 1;
> > +       cmd->desc.bMatrixCoefficients = 4;
> I realise these values were taken directly as existing code, but
> particularly in regards to how these values will be set from userspace -
> is it easy enough to have some common definitions in a preceeding patch
> that state the supported values here from the spec, to avoid 'magic
> values' here ...
>
> A header with defines or an enum isn't going to be usable from a bash
> script configuring configfs, but at least a compiled program could use
> the definitions.


Yes, I think probably it's a candidate for 
include/uapi/linux/usb/video.h...unless anyone thinks it's better elsewhere

> > +
> > +       config_group_init_type_name(&cmd->group, "default",
> > +                                   &uvcg_color_matching_type);
> > +       configfs_add_default_group(&cmd->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..f990739838d5 100644
> > --- a/drivers/usb/gadget/function/uvc_configfs.h
> > +++ b/drivers/usb/gadget/function/uvc_configfs.h
> > @@ -37,6 +37,14 @@ static inline struct uvcg_control_header *to_uvcg_control_header(struct config_i
> >          return container_of(item, struct uvcg_control_header, item);
> >   }
> >   
> > +struct uvcg_cmd {
> > +       struct config_group group;
> > +       struct uvc_color_matching_descriptor desc;
> > +};
> > +
> > +#define to_uvcg_cmd(group_ptr) \
> > +container_of(group_ptr, struct uvcg_cmd, group)
> > +
> >   enum uvcg_format_type {
> >          UVCG_UNCOMPRESSED = 0,
> >          UVCG_MJPEG,
> > --
> > 2.34.1