Hi Dan,
Thank you for the patch.
On Mon, Nov 21, 2022 at 09:25:10AM +0000, Daniel Scally wrote:
> the __uvcg_*frm_intrv() helper functions can be helpful when adding
s/the/The/
> support for similar attributes. Generalise the function names and
> move them higher in the file for better coverage. We also need copies
> of the functions for different sized targets, so refactor them to
> a macro with configurable bit size.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
>
> - none
>
> drivers/usb/gadget/function/uvc_configfs.c | 109 +++++++++++----------
> 1 file changed, 56 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index af4258120f9a..d542dd251633 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -594,6 +594,60 @@ static const struct uvcg_config_group_type uvcg_terminal_grp_type = {
> },
> };
>
> +static inline int __uvcg_count_item_entries(char *buf, void *priv)
> +{
> + ++*((int *)priv);
> + return 0;
> +}
> +
> +#define UVCG_ITEM_ENTRY_FUNCS(bits) \
> +static inline int __uvcg_fill_item_entries_u##bits(char *buf, void *priv)\
> +{ \
> + u##bits num, **interv; \
I'd rename interv to values as the function is now generic.
> + int ret; \
> + \
> + ret = kstrtou##bits(buf, 0, &num); \
> + if (ret) \
> + return ret; \
> + \
> + interv = priv; \
> + **interv = num; \
> + ++*interv; \
> + \
> + return 0; \
> +} \
> + \
> +static int __uvcg_iter_item_entries_u##bits(const char *page, size_t len,\
> + int (*fun)(char *, void *), void *priv)\
> +{ \
> + /* sign, base 2 representation, newline, terminator */ \
> + char buf[1 + sizeof(u##bits) * 8 + 1 + 1]; \
As the only dependence on bits is a size, how about passing the size to
the function as a parameter instead of duplicating the whole
implementation ?
> + const char *pg = page; \
> + int i, ret; \
> + \
> + if (!fun) \
> + return -EINVAL; \
> + \
> + while (pg - page < len) { \
> + i = 0; \
> + while (i < sizeof(buf) && (pg - page < len) && \
> + *pg != '\0' && *pg != '\n') \
> + buf[i++] = *pg++; \
> + if (i == sizeof(buf)) \
> + return -EINVAL; \
> + while ((pg - page < len) && (*pg == '\0' || *pg == '\n'))\
> + ++pg; \
> + buf[i] = '\0'; \
> + ret = fun(buf, priv); \
> + if (ret) \
> + return ret; \
> + } \
> + \
> + return 0; \
> +}
> +
> +UVCG_ITEM_ENTRY_FUNCS(32)
> +
> /* -----------------------------------------------------------------------------
> * control/class/{fs|ss}
> */
> @@ -1186,57 +1240,6 @@ static ssize_t uvcg_frame_dw_frame_interval_show(struct config_item *item,
> return result;
> }
>
> -static inline int __uvcg_count_frm_intrv(char *buf, void *priv)
> -{
> - ++*((int *)priv);
> - return 0;
> -}
> -
> -static inline int __uvcg_fill_frm_intrv(char *buf, void *priv)
> -{
> - u32 num, **interv;
> - int ret;
> -
> - ret = kstrtou32(buf, 0, &num);
> - if (ret)
> - return ret;
> -
> - interv = priv;
> - **interv = num;
> - ++*interv;
> -
> - return 0;
> -}
> -
> -static int __uvcg_iter_frm_intrv(const char *page, size_t len,
> - int (*fun)(char *, void *), void *priv)
> -{
> - /* sign, base 2 representation, newline, terminator */
> - char buf[1 + sizeof(u32) * 8 + 1 + 1];
> - const char *pg = page;
> - int i, ret;
> -
> - if (!fun)
> - return -EINVAL;
> -
> - while (pg - page < len) {
> - i = 0;
> - while (i < sizeof(buf) && (pg - page < len) &&
> - *pg != '\0' && *pg != '\n')
> - buf[i++] = *pg++;
> - if (i == sizeof(buf))
> - return -EINVAL;
> - while ((pg - page < len) && (*pg == '\0' || *pg == '\n'))
> - ++pg;
> - buf[i] = '\0';
> - ret = fun(buf, priv);
> - if (ret)
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> static ssize_t uvcg_frame_dw_frame_interval_store(struct config_item *item,
> const char *page, size_t len)
> {
> @@ -1260,7 +1263,7 @@ static ssize_t uvcg_frame_dw_frame_interval_store(struct config_item *item,
> goto end;
> }
>
> - ret = __uvcg_iter_frm_intrv(page, len, __uvcg_count_frm_intrv, &n);
> + ret = __uvcg_iter_item_entries_u32(page, len, __uvcg_count_item_entries, &n);
A wrapper macro could be nice, to be written
ret = uvcg_count_item_entries(u32, page, len, &n);
> if (ret)
> goto end;
>
> @@ -1270,7 +1273,7 @@ static ssize_t uvcg_frame_dw_frame_interval_store(struct config_item *item,
> goto end;
> }
>
> - ret = __uvcg_iter_frm_intrv(page, len, __uvcg_fill_frm_intrv, &tmp);
> + ret = __uvcg_iter_item_entries_u32(page, len, __uvcg_fill_item_entries_u32, &tmp);
And
ret = uvcg_fill_item_entries(u32, page, len, &tmp);
This in particular would make sure that the __uvcg_fill_item_entries_*()
variant always matches the size passed to
__uvcg_iter_item_entries_u32().
This is probably a candidate for a separate patch on top of this one.
> if (ret) {
> kfree(frm_intrv);
> goto end;
--
Regards,
Laurent Pinchart
