Re: [PATCH v3 15/17] media: rzg2l-cru: Refactor ICnDMR register configuration

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Oct 01, 2024 at 03:09:17PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Refactor the ICnDMR register configuration in
> `rzg2l_cru_initialize_image_conv()` by adding a new member `icndmr` in the
> `rzg2l_cru_ip_format` structure.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Updated subject line and commit message
> - Re-used rzg2l_cru_ip_format_to_fmt() to fetch icndmr details
> - Collected RB tag
> 
> v1->v2
> - New patch
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h   |  4 ++++
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c    |  1 +
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 10 ++++------
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 39296a59b3da..51206373b7fe 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -31,6 +31,8 @@
>  #define RZG2L_CRU_MIN_INPUT_HEIGHT	240
>  #define RZG2L_CRU_MAX_INPUT_HEIGHT	4095
>  
> +#define ICnDMR_YCMODE_UYVY		(1 << 4)
> +
>  enum rzg2l_csi2_pads {
>  	RZG2L_CRU_IP_SINK = 0,
>  	RZG2L_CRU_IP_SOURCE,
> @@ -68,12 +70,14 @@ struct rzg2l_cru_ip {
>   * @format: 4CC format identifier (V4L2_PIX_FMT_*)
>   * @datatype: MIPI CSI2 data type
>   * @bpp: bytes per pixel
> + * @icndmr: ICnDMR register value
>   */
>  struct rzg2l_cru_ip_format {
>  	u32 code;
>  	u32 format;
>  	u32 datatype;
>  	u8 bpp;
> +	u32 icndmr;
>  };
>  
>  /**
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 6ce077ab42e2..f14ac949cc64 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -17,6 +17,7 @@ static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
>  		.format = V4L2_PIX_FMT_UYVY,
>  		.datatype = MIPI_CSI2_DT_YUV422_8B,
>  		.bpp = 2,
> +		.icndmr = ICnDMR_YCMODE_UYVY,
>  	},
>  };
>  
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index c6c82b9b130a..c3d10b001b7c 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -88,7 +88,6 @@
>  
>  /* CRU Data Output Mode Register */
>  #define ICnDMR				0x26c
> -#define ICnDMR_YCMODE_UYVY		(1 << 4)
>  
>  #define RZG2L_TIMEOUT_MS		100
>  #define RZG2L_RETRIES			10
> @@ -278,6 +277,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  					   u8 csi_vc)
>  {
>  	const struct v4l2_format_info *src_finfo, *dst_finfo;
> +	const struct rzg2l_cru_ip_format *cru_video_fmt;
>  	const struct rzg2l_cru_ip_format *cru_ip_fmt;
>  	u32 icndmr;
>  
> @@ -288,15 +288,13 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  	dst_finfo = v4l2_format_info(cru->format.pixelformat);
>  
>  	/* Output format */
> -	switch (cru->format.pixelformat) {
> -	case V4L2_PIX_FMT_UYVY:
> -		icndmr = ICnDMR_YCMODE_UYVY;
> -		break;
> -	default:
> +	cru_video_fmt = rzg2l_cru_ip_format_to_fmt(cru->format.pixelformat);
> +	if (!cru_video_fmt) {
>  		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
>  			cru->format.pixelformat);
>  		return -EINVAL;
>  	}
> +	icndmr = cru_video_fmt->icndmr;

I think you can drop the icndmr local variable and write below

	/* Set output data format */
	rzg2l_cru_write(cru, ICnDMR, cru_video_fmt->icndmr);

With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>  
>  	/* If input and output use same colorspace, do bypass mode */
>  	if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))

-- 
Regards,

Laurent Pinchart