Hello,
On Fri, Dec 16, 2022 at 02:06:48PM +0000, Dan Scally wrote:
> 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.
I agree with Kieran, uvcg_cmd isn't good. uvcg_color_matching ?
> >> 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); \
And you could name the variable color_match.
> >> 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));
You can write
struct uvcg_cmd *cmd = to_uvcg_cmd(to_config_group(item));
although it would need to become
struct uvcg_color_matching *color_match =
to_uvcg_color_matching(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
Sounds good to me.
> >> +
> >> + 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,
--
Regards,
Laurent Pinchart
