Received-SPF: pass (google.com: domain of linux-kernel+bounces-208474-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 10 Jun 2024 18:20:46 +0300
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Ricardo Ribalda <ribalda@chromium.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/5] media: uvcvideo: Refactor Power Line Frequency limit
 selection
Message-ID: <20240610152046.GJ26663@pendragon.ideasonboard.com>
References: <20240318-billion-v1-0-2f7bc0ee2030@chromium.org>
 <20240318-billion-v1-2-2f7bc0ee2030@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240318-billion-v1-2-2f7bc0ee2030@chromium.org>

Hi Ricardo,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:55:24PM +0000, Ricardo Ribalda wrote:
> Move the PLF mapping logic to its own function. This patch does not
> introduce any new functionality to the logic, it is just a preparation
> patch.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 93 ++++++++++++++++++++++++----------------
>  1 file changed, 55 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 3e939b4fbaaaf..67522143c6c85 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -459,6 +459,56 @@ static void uvc_ctrl_set_rel_speed(struct uvc_control_mapping *mapping,
>  	data[first+1] = min_t(int, abs(value), 0xff);
>  }
>  
> +const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> +	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> +	.entity		= UVC_GUID_UVC_PROCESSING,
> +	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +	.size		= 2,
> +	.offset		= 0,
> +	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> +				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
> +};
> +
> +const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
> +	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> +	.entity		= UVC_GUID_UVC_PROCESSING,
> +	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +	.size		= 2,
> +	.offset		= 0,
> +	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> +				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> +};
> +
> +static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> +	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> +	.entity		= UVC_GUID_UVC_PROCESSING,
> +	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +	.size		= 2,
> +	.offset		= 0,
> +	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> +	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> +	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO,
> +				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> +};
> +
> +static int __uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> +	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping);

I wonder if we could avoid the forward declaration by turning the
.add_mapping() operation into a .filter_mapping() (name to be
bikshedded) that would return a replacement mapping instead of adding
it. The caller (the __uvc_ctrl_add_custom_mapping() function) would then
call __uvc_ctrl_add_mapping() unconditionally. You could actually call
the new operation directly in __uvc_ctrl_add_custom_mapping() without
having to add a new __uvc_ctrl_add_custom_mapping() function. What do
you think, would that be simpler and more redable ?

> +
> +static int uvc_ctrl_add_plf_mapping(struct uvc_video_chain *chain,
> +	struct uvc_control *ctrl, const struct uvc_control_mapping *mapping)
> +{
> +	if (chain->dev->uvc_version < 0x150)
> +		return __uvc_ctrl_add_mapping(chain, ctrl,
> +					      &uvc_ctrl_power_line_mapping_uvc11);
> +
> +	return __uvc_ctrl_add_mapping(chain, ctrl,
> +				      &uvc_ctrl_power_line_mapping_uvc15);
> +}
> +
>  static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  	{
>  		.id		= V4L2_CID_BRIGHTNESS,
> @@ -748,51 +798,18 @@ static const struct uvc_control_mapping uvc_ctrl_mappings[] = {
>  		.v4l2_type	= V4L2_CTRL_TYPE_BOOLEAN,
>  		.data_type	= UVC_CTRL_DATA_TYPE_BOOLEAN,
>  	},
> -};
> -
> -const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited = {
> -	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -	.entity		= UVC_GUID_UVC_PROCESSING,
> -	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> -	.size		= 2,
> -	.offset		= 0,
> -	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> -	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> -	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> -				  V4L2_CID_POWER_LINE_FREQUENCY_50HZ),
> -};
> -
> -const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc11 = {
> -	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -	.entity		= UVC_GUID_UVC_PROCESSING,
> -	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> -	.size		= 2,
> -	.offset		= 0,
> -	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> -	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> -	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_60HZ,
> -				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> +	{
> +		.entity		= UVC_GUID_UVC_PROCESSING,
> +		.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> +		.add_mapping	= uvc_ctrl_add_plf_mapping,
> +	},
>  };
>  
>  static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc11[] = {
> -	&uvc_ctrl_power_line_mapping_uvc11,
>  	NULL, /* Sentinel */
>  };
>  
> -static const struct uvc_control_mapping uvc_ctrl_power_line_mapping_uvc15 = {
> -	.id		= V4L2_CID_POWER_LINE_FREQUENCY,
> -	.entity		= UVC_GUID_UVC_PROCESSING,
> -	.selector	= UVC_PU_POWER_LINE_FREQUENCY_CONTROL,
> -	.size		= 2,
> -	.offset		= 0,
> -	.v4l2_type	= V4L2_CTRL_TYPE_MENU,
> -	.data_type	= UVC_CTRL_DATA_TYPE_ENUM,
> -	.menu_mask	= GENMASK(V4L2_CID_POWER_LINE_FREQUENCY_AUTO,
> -				  V4L2_CID_POWER_LINE_FREQUENCY_DISABLED),
> -};
> -
>  static const struct uvc_control_mapping *uvc_ctrl_mappings_uvc15[] = {
> -	&uvc_ctrl_power_line_mapping_uvc15,
>  	NULL, /* Sentinel */
>  };
>  

-- 
Regards,

Laurent Pinchart