Re: [PATCH 5/6] usb: gadget: uvc: Make color matching attributes read/write

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Daniel Scally (2022-12-13 08:37:35)
> In preparation for allowing more than the default color matching
> descriptor, make the color matching attributes writeable.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  .../ABI/testing/configfs-usb-gadget-uvc       |  2 +-
>  drivers/usb/gadget/function/uvc_configfs.c    | 32 ++++++++++++++++++-
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 611b23e6488d..3512f4899fe3 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -165,7 +165,7 @@ Date:               Dec 2014
>  KernelVersion: 4.0
>  Description:   Default color matching descriptors
>  
> -               All attributes read only:
> +               All attributes read/write:

Do we need to specify here what acceptable values can now be written at
all?

>  
>                 ========================  ======================================
>                 bMatrixCoefficients       matrix used to compute luma and
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 6f5932c9f09c..4fbc42d738a4 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1845,7 +1845,37 @@ static ssize_t uvcg_color_matching_##cname##_show(                       \
>         return result;                                                  \
>  }                                                                      \
>                                                                         \
> -UVC_ATTR_RO(uvcg_color_matching_, cname, aname)
> +static ssize_t uvcg_color_matching_##cname##_store(                    \
> +       struct config_item *item, const char *page, size_t len)         \
> +{                                                                      \
> +       struct config_group *group = to_config_group(item);             \
> +       struct mutex *su_mutex = &group->cg_subsys->su_mutex;           \
> +       struct uvcg_cmd *cmd = to_uvcg_cmd(group);                      \
> +       struct f_uvc_opts *opts;                                        \
> +       struct config_item *opts_item;                                  \
> +       int ret;                                                        \
> +       u##bits num;                                                    \
> +                                                                       \
> +       ret = kstrtou##bits(page, 0, &num);                             \
> +       if (ret)                                                        \
> +               return ret;                                             \

I don't know how horrible it would be - or if there's any other
precendence, but I'm weary that setting '1', or '4' in here from
userspace is fairly meaningless.

Of course - the user doing so would have to know from the spec perhaps
what they are configuring - but it makes me wonder if we should support
string matching in here to also convert say "BT.709" to the appropriate
integer value (if a non-integer was set).

It may depend on how 'most' other configfs entries that would be similar
to this would expect to operate.

> +                                                                       \
> +       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);                                \
> +                                                                       \
> +       mutex_lock(&opts->lock);                                        \
> +                                                                       \
> +       cmd->desc.aname = num;                                          \
> +       ret = len;                                                      \
> +                                                                       \
> +       mutex_unlock(&opts->lock);                                      \
> +       mutex_unlock(su_mutex);                                         \
> +                                                                       \
> +       return ret;                                                     \
> +}                                                                      \
> +UVC_ATTR(uvcg_color_matching_, cname, aname)
>  
>  UVCG_COLOR_MATCHING_ATTR(b_color_primaries, bColorPrimaries, 8);
>  UVCG_COLOR_MATCHING_ATTR(b_transfer_characteristics, bTransferCharacteristics, 8);
> -- 
> 2.34.1
>