Re: [patch 6/6] usb: gadget: uvc: cleanup UVCG_FRAME_ATTR macro

[Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>]

W dniu 14.01.2015 o 22:06, Dan Carpenter pisze:
> 1) Change "conv" an "vnoc" to "to_cpu_endian" to "to_little_endian".
> 2) No need to check the "limit" because that is already handled in
>     kstrtoXX so delete that parameter along with the check.
> 3) By using a "bits" parameter, we can combine the "uxx" parameter and
>     the "str2u" parameters.
> 4) The kstrtou##bits() conversion does not need to be done under the
>     mutex so move it to the start of the function.
> 5) Change the name of "identity_conv" to "noop_conversion".
>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

Acked-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>

 ---
> This file has a couple pages of Sparse endian warnings.
> http://lwn.net/Articles/205624/
> It's hard to review the static checker warnings when there are so many.
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index a0443a2..87beb8c 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1032,7 +1032,7 @@ static struct configfs_item_operations uvcg_frame_item_ops = {
>   	.store_attribute	= uvcg_frame_attr_store,
>   };
>
> -#define UVCG_FRAME_ATTR(cname, aname, conv, str2u, uxx, vnoc, limit)	\
> +#define UVCG_FRAME_ATTR(cname, aname, to_cpu_endian, to_little_endian, bits) \
>   static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char *page)\
>   {									\
>   	struct f_uvc_opts *opts;					\
> @@ -1046,7 +1046,7 @@ static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char *page)\
>   	opts = to_f_uvc_opts(opts_item);				\
>   									\
>   	mutex_lock(&opts->lock);					\
> -	result = sprintf(page, "%d\n", conv(f->frame.cname));		\
> +	result = sprintf(page, "%d\n", to_cpu_endian(f->frame.cname));	\
>   	mutex_unlock(&opts->lock);					\
>   									\
>   	mutex_unlock(su_mutex);						\
> @@ -1061,7 +1061,11 @@ static ssize_t  uvcg_frame_##cname##_store(struct uvcg_frame *f,	\
>   	struct uvcg_format *fmt;					\
>   	struct mutex *su_mutex = &f->item.ci_group->cg_subsys->su_mutex;\
>   	int ret;							\
> -	uxx num;							\
> +	u##bits num;							\
> +									\
> +	ret = kstrtou##bits(page, 0, &num);				\
> +	if (ret)							\
> +		return ret;						\
>   									\
>   	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
>   									\
> @@ -1075,15 +1079,7 @@ static ssize_t  uvcg_frame_##cname##_store(struct uvcg_frame *f,	\
>   		goto end;						\
>   	}								\
>   									\
> -	ret = str2u(page, 0, &num);					\
> -	if (ret)							\
> -		goto end;						\
> -									\
> -	if (num > limit) {						\
> -		ret = -EINVAL;						\
> -		goto end;						\
> -	}								\
> -	f->frame.cname = vnoc(num);					\
> +	f->frame.cname = to_little_endian(num);				\
>   	ret = len;							\
>   end:									\
>   	mutex_unlock(&opts->lock);					\
> @@ -1097,24 +1093,20 @@ static struct uvcg_frame_attribute					\
>   			uvcg_frame_##cname##_show,			\
>   			uvcg_frame_##cname##_store)
>
> -#define identity_conv(x) (x)
> +#define noop_conversion(x) (x)
>
> -UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, identity_conv, kstrtou8, u8,
> -		identity_conv, 0xFF);
> -UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
> -		0xFFFF);
> -UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
> -		0xFFFF);
> -UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, kstrtou32, u32,
> -		cpu_to_le32, 0xFFFFFFFF);
> -UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, kstrtou32, u32,
> -		cpu_to_le32, 0xFFFFFFFF);
> +UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
> +		noop_conversion, 8);
> +UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
> +UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
> +UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
> +UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, cpu_to_le32, 32);
>   UVCG_FRAME_ATTR(dw_max_video_frame_buffer_size, dwMaxVideoFrameBufferSize,
> -		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
> +		le32_to_cpu, cpu_to_le32, 32);
>   UVCG_FRAME_ATTR(dw_default_frame_interval, dwDefaultFrameInterval,
> -		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
> +		le32_to_cpu, cpu_to_le32, 32);
>
> -#undef identity_conv
> +#undef noop_conversion
>
>   #undef UVCG_FRAME_ATTR
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html