diff mbox series

[3/4] media: platform: rzg2l-cru: Use v4l2_fill_pixfmt()

Message ID 20240920124115.375748-4-dan.scally@ideasonboard.com (mailing list archive)
State New
Headers show
Series Extend RAW format support for rzg2l-cru driver | expand

Commit Message

Dan Scally Sept. 20, 2024, 12:41 p.m. UTC 
From: Daniel Scally <dan.scally+renesas@ideasonboard.com>

Rather than open-code a calculation of the format's bytesperline
and sizeimage, use the v4l2_fill_pixfmt() helper. This makes it
easier to support the CRU packed pixel formats without over
complicating the driver.

This change makes the .bpp member of struct rzg2l_cru_ip_format and
the rzg2l_cru_ip_pix_fmt_to_bpp() function superfluous - remove them
both.

Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 ---
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 16 --------------
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 21 ++-----------------
 3 files changed, 2 insertions(+), 38 deletions(-)

Comments

Jacopo Mondi Sept. 24, 2024, 2:08 p.m. UTC | #1 
Hi Dan

On Fri, Sep 20, 2024 at 01:41:14PM GMT, Daniel Scally wrote:
> From: Daniel Scally <dan.scally+renesas@ideasonboard.com>
>
> Rather than open-code a calculation of the format's bytesperline
> and sizeimage, use the v4l2_fill_pixfmt() helper. This makes it
> easier to support the CRU packed pixel formats without over
> complicating the driver.
>
> This change makes the .bpp member of struct rzg2l_cru_ip_format and
> the rzg2l_cru_ip_pix_fmt_to_bpp() function superfluous - remove them
> both.
>
> Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 ---
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 16 --------------
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 21 ++-----------------
>  3 files changed, 2 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index dc50a5feb3de..858098b8a13f 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -68,14 +68,12 @@ struct rzg2l_cru_ip {
>   * @code: Media bus code
>   * @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;
>  };
>
> @@ -169,7 +167,6 @@ void rzg2l_cru_ip_subdev_unregister(struct rzg2l_cru_dev *cru);
>  struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
>
>  const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code);
> -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
>  int rzg2l_cru_ip_index_to_pix_fmt(u32 index);
>  int rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 9bb192655f25..f2fea3a63444 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -16,35 +16,30 @@ static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
>  		.code = MEDIA_BUS_FMT_UYVY8_1X16,
>  		.format = V4L2_PIX_FMT_UYVY,
>  		.datatype = MIPI_CSI2_DT_YUV422_8B,
> -		.bpp = 2,
>  		.icndmr = ICnDMR_YCMODE_UYVY,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
>  		.format = V4L2_PIX_FMT_SBGGR8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
>  		.format = V4L2_PIX_FMT_SGBRG8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
>  		.format = V4L2_PIX_FMT_SGRBG8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
>  		.format = V4L2_PIX_FMT_SRGGB8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  };
> @@ -60,17 +55,6 @@ const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
>  	return NULL;
>  }
>
> -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
> -		if (rzg2l_cru_ip_formats[i].format == format)
> -			return rzg2l_cru_ip_formats[i].bpp;
> -
> -	return 0;
> -}
> -
>  int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
>  {
>  	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 61e2f23053ee..01b39a2395df 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -800,27 +800,11 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
>   * V4L2 stuff
>   */
>
> -static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
> -{
> -	u8 bpp;
> -
> -	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
> -
> -	if (WARN_ON(!bpp))
> -		return 0;
> -
> -	return pix->width * bpp;
> -}
> -
> -static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> -{
> -	return pix->bytesperline * pix->height;
> -}
>
>  static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
>  				   struct v4l2_pix_format *pix)
>  {
> -	if (!rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat))
> +	if (rzg2l_cru_ip_pix_fmt_to_icndmr(pix->pixelformat) < 0)
>  		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
>
>  	switch (pix->field) {
> @@ -840,8 +824,7 @@ static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
>  	v4l_bound_align_image(&pix->width, 320, RZG2L_CRU_MAX_INPUT_WIDTH, 1,
>  			      &pix->height, 240, RZG2L_CRU_MAX_INPUT_HEIGHT, 2, 0);
>
> -	pix->bytesperline = rzg2l_cru_format_bytesperline(pix);
> -	pix->sizeimage = rzg2l_cru_format_sizeimage(pix);
> +	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);

Looks good to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
>  	dev_dbg(cru->dev, "Format %ux%u bpl: %u size: %u\n",
>  		pix->width, pix->height, pix->bytesperline, pix->sizeimage);
> --
> 2.34.1
>
>
Laurent Pinchart Sept. 24, 2024, 2:46 p.m. UTC | #2 
Hi Dan,

Thank you for the patch.

On Fri, Sep 20, 2024 at 01:41:14PM +0100, Daniel Scally wrote:
> From: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> 
> Rather than open-code a calculation of the format's bytesperline
> and sizeimage, use the v4l2_fill_pixfmt() helper. This makes it
> easier to support the CRU packed pixel formats without over
> complicating the driver.
> 
> This change makes the .bpp member of struct rzg2l_cru_ip_format and
> the rzg2l_cru_ip_pix_fmt_to_bpp() function superfluous - remove them
> both.
> 
> Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>

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

> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 ---
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 16 --------------
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 21 ++-----------------
>  3 files changed, 2 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index dc50a5feb3de..858098b8a13f 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -68,14 +68,12 @@ struct rzg2l_cru_ip {
>   * @code: Media bus code
>   * @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;
>  };
>  
> @@ -169,7 +167,6 @@ void rzg2l_cru_ip_subdev_unregister(struct rzg2l_cru_dev *cru);
>  struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
>  
>  const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code);
> -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
>  int rzg2l_cru_ip_index_to_pix_fmt(u32 index);
>  int rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
>  
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 9bb192655f25..f2fea3a63444 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -16,35 +16,30 @@ static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
>  		.code = MEDIA_BUS_FMT_UYVY8_1X16,
>  		.format = V4L2_PIX_FMT_UYVY,
>  		.datatype = MIPI_CSI2_DT_YUV422_8B,
> -		.bpp = 2,
>  		.icndmr = ICnDMR_YCMODE_UYVY,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
>  		.format = V4L2_PIX_FMT_SBGGR8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
>  		.format = V4L2_PIX_FMT_SGBRG8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
>  		.format = V4L2_PIX_FMT_SGRBG8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
>  		.format = V4L2_PIX_FMT_SRGGB8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  };
> @@ -60,17 +55,6 @@ const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
>  	return NULL;
>  }
>  
> -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
> -		if (rzg2l_cru_ip_formats[i].format == format)
> -			return rzg2l_cru_ip_formats[i].bpp;
> -
> -	return 0;
> -}
> -
>  int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
>  {
>  	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 61e2f23053ee..01b39a2395df 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -800,27 +800,11 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
>   * V4L2 stuff
>   */
>  
> -static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
> -{
> -	u8 bpp;
> -
> -	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
> -
> -	if (WARN_ON(!bpp))
> -		return 0;
> -
> -	return pix->width * bpp;
> -}
> -
> -static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> -{
> -	return pix->bytesperline * pix->height;
> -}
>  
>  static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
>  				   struct v4l2_pix_format *pix)
>  {
> -	if (!rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat))
> +	if (rzg2l_cru_ip_pix_fmt_to_icndmr(pix->pixelformat) < 0)
>  		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
>  
>  	switch (pix->field) {
> @@ -840,8 +824,7 @@ static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
>  	v4l_bound_align_image(&pix->width, 320, RZG2L_CRU_MAX_INPUT_WIDTH, 1,
>  			      &pix->height, 240, RZG2L_CRU_MAX_INPUT_HEIGHT, 2, 0);
>  
> -	pix->bytesperline = rzg2l_cru_format_bytesperline(pix);
> -	pix->sizeimage = rzg2l_cru_format_sizeimage(pix);
> +	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);
>  
>  	dev_dbg(cru->dev, "Format %ux%u bpl: %u size: %u\n",
>  		pix->width, pix->height, pix->bytesperline, pix->sizeimage);
Lad Prabhakar Sept. 27, 2024, 12:51 p.m. UTC | #3 
Hi Daniel,

Thank you for the patch.


> From: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> 
> Rather than open-code a calculation of the format's bytesperline and
> sizeimage, use the v4l2_fill_pixfmt() helper. This makes it easier to
> support the CRU packed pixel formats without over complicating the driver.
> 
> This change makes the .bpp member of struct rzg2l_cru_ip_format and the
> rzg2l_cru_ip_pix_fmt_to_bpp() function superfluous - remove them both.
> 
> Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 ---
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 16 --------------
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 21 ++-----------------
>  3 files changed, 2 insertions(+), 38 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

This patch doesn't apply cleanly on top of media-stage + [0]

[0] https://lore.kernel.org/all/20240910175357.229075-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar
 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index dc50a5feb3de..858098b8a13f 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -68,14 +68,12 @@ struct rzg2l_cru_ip {
>   * @code: Media bus code
>   * @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;
>  };
> 
> @@ -169,7 +167,6 @@ void rzg2l_cru_ip_subdev_unregister(struct
> rzg2l_cru_dev *cru);  struct v4l2_mbus_framefmt
> *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
> 
>  const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int
> code);
> -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);  int
> rzg2l_cru_ip_index_to_pix_fmt(u32 index);  int
> rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 9bb192655f25..f2fea3a63444 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -16,35 +16,30 @@ static const struct rzg2l_cru_ip_format
> rzg2l_cru_ip_formats[] = {
>  		.code = MEDIA_BUS_FMT_UYVY8_1X16,
>  		.format = V4L2_PIX_FMT_UYVY,
>  		.datatype = MIPI_CSI2_DT_YUV422_8B,
> -		.bpp = 2,
>  		.icndmr = ICnDMR_YCMODE_UYVY,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
>  		.format = V4L2_PIX_FMT_SBGGR8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
>  		.format = V4L2_PIX_FMT_SGBRG8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
>  		.format = V4L2_PIX_FMT_SGRBG8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  	{
>  		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
>  		.format = V4L2_PIX_FMT_SRGGB8,
>  		.datatype = MIPI_CSI2_DT_RAW8,
> -		.bpp = 1,
>  		.icndmr = 0,
>  	},
>  };
> @@ -60,17 +55,6 @@ const struct rzg2l_cru_ip_format
> *rzg2l_cru_ip_code_to_fmt(unsigned int code)
>  	return NULL;
>  }
> 
> -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format) -{
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
> -		if (rzg2l_cru_ip_formats[i].format == format)
> -			return rzg2l_cru_ip_formats[i].bpp;
> -
> -	return 0;
> -}
> -
>  int rzg2l_cru_ip_index_to_pix_fmt(u32 index)  {
>  	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats)) diff --git
> a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 61e2f23053ee..01b39a2395df 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -800,27 +800,11 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev
> *cru)
>   * V4L2 stuff
>   */
> 
> -static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix) -{
> -	u8 bpp;
> -
> -	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
> -
> -	if (WARN_ON(!bpp))
> -		return 0;
> -
> -	return pix->width * bpp;
> -}
> -
> -static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix) -{
> -	return pix->bytesperline * pix->height;
> -}
> 
>  static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
>  				   struct v4l2_pix_format *pix)
>  {
> -	if (!rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat))
> +	if (rzg2l_cru_ip_pix_fmt_to_icndmr(pix->pixelformat) < 0)
>  		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
> 
>  	switch (pix->field) {
> @@ -840,8 +824,7 @@ static void rzg2l_cru_format_align(struct
> rzg2l_cru_dev *cru,
>  	v4l_bound_align_image(&pix->width, 320, RZG2L_CRU_MAX_INPUT_WIDTH,
> 1,
>  			      &pix->height, 240, RZG2L_CRU_MAX_INPUT_HEIGHT, 2,
> 0);
> 
> -	pix->bytesperline = rzg2l_cru_format_bytesperline(pix);
> -	pix->sizeimage = rzg2l_cru_format_sizeimage(pix);
> +	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);
> 
>  	dev_dbg(cru->dev, "Format %ux%u bpl: %u size: %u\n",
>  		pix->width, pix->height, pix->bytesperline, pix->sizeimage);
> --
> 2.34.1
Laurent Pinchart Sept. 27, 2024, 11:27 p.m. UTC | #4 
On Fri, Sep 27, 2024 at 12:51:24PM +0000, Prabhakar Mahadev Lad wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> 
> > From: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> > 
> > Rather than open-code a calculation of the format's bytesperline and
> > sizeimage, use the v4l2_fill_pixfmt() helper. This makes it easier to
> > support the CRU packed pixel formats without over complicating the driver.
> > 
> > This change makes the .bpp member of struct rzg2l_cru_ip_format and the
> > rzg2l_cru_ip_pix_fmt_to_bpp() function superfluous - remove them both.
> > 
> > Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 16 --------------
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 21 ++-----------------
> >  3 files changed, 2 insertions(+), 38 deletions(-)
> >
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> This patch doesn't apply cleanly on top of media-stage + [0]
> 
> [0] https://lore.kernel.org/all/20240910175357.229075-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Is it fine with you to wait for v3 of your series and rebase this one on
top ?

> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index dc50a5feb3de..858098b8a13f 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -68,14 +68,12 @@ struct rzg2l_cru_ip {
> >   * @code: Media bus code
> >   * @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;
> >  };
> > 
> > @@ -169,7 +167,6 @@ void rzg2l_cru_ip_subdev_unregister(struct
> > rzg2l_cru_dev *cru);  struct v4l2_mbus_framefmt
> > *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
> > 
> >  const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int
> > code);
> > -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);  int
> > rzg2l_cru_ip_index_to_pix_fmt(u32 index);  int
> > rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
> > 
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > index 9bb192655f25..f2fea3a63444 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > @@ -16,35 +16,30 @@ static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
> >  		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> >  		.format = V4L2_PIX_FMT_UYVY,
> >  		.datatype = MIPI_CSI2_DT_YUV422_8B,
> > -		.bpp = 2,
> >  		.icndmr = ICnDMR_YCMODE_UYVY,
> >  	},
> >  	{
> >  		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
> >  		.format = V4L2_PIX_FMT_SBGGR8,
> >  		.datatype = MIPI_CSI2_DT_RAW8,
> > -		.bpp = 1,
> >  		.icndmr = 0,
> >  	},
> >  	{
> >  		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
> >  		.format = V4L2_PIX_FMT_SGBRG8,
> >  		.datatype = MIPI_CSI2_DT_RAW8,
> > -		.bpp = 1,
> >  		.icndmr = 0,
> >  	},
> >  	{
> >  		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
> >  		.format = V4L2_PIX_FMT_SGRBG8,
> >  		.datatype = MIPI_CSI2_DT_RAW8,
> > -		.bpp = 1,
> >  		.icndmr = 0,
> >  	},
> >  	{
> >  		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
> >  		.format = V4L2_PIX_FMT_SRGGB8,
> >  		.datatype = MIPI_CSI2_DT_RAW8,
> > -		.bpp = 1,
> >  		.icndmr = 0,
> >  	},
> >  };
> > @@ -60,17 +55,6 @@ const struct rzg2l_cru_ip_format
> > *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> >  	return NULL;
> >  }
> > 
> > -u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format) -{
> > -	unsigned int i;
> > -
> > -	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
> > -		if (rzg2l_cru_ip_formats[i].format == format)
> > -			return rzg2l_cru_ip_formats[i].bpp;
> > -
> > -	return 0;
> > -}
> > -
> >  int rzg2l_cru_ip_index_to_pix_fmt(u32 index)  {
> >  	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats)) diff --git
> > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index 61e2f23053ee..01b39a2395df 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -800,27 +800,11 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev
> > *cru)
> >   * V4L2 stuff
> >   */
> > 
> > -static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix) -{
> > -	u8 bpp;
> > -
> > -	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
> > -
> > -	if (WARN_ON(!bpp))
> > -		return 0;
> > -
> > -	return pix->width * bpp;
> > -}
> > -
> > -static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix) -{
> > -	return pix->bytesperline * pix->height;
> > -}
> > 
> >  static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
> >  				   struct v4l2_pix_format *pix)
> >  {
> > -	if (!rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat))
> > +	if (rzg2l_cru_ip_pix_fmt_to_icndmr(pix->pixelformat) < 0)
> >  		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
> > 
> >  	switch (pix->field) {
> > @@ -840,8 +824,7 @@ static void rzg2l_cru_format_align(struct
> > rzg2l_cru_dev *cru,
> >  	v4l_bound_align_image(&pix->width, 320, RZG2L_CRU_MAX_INPUT_WIDTH,
> > 1,
> >  			      &pix->height, 240, RZG2L_CRU_MAX_INPUT_HEIGHT, 2,
> > 0);
> > 
> > -	pix->bytesperline = rzg2l_cru_format_bytesperline(pix);
> > -	pix->sizeimage = rzg2l_cru_format_sizeimage(pix);
> > +	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);
> > 
> >  	dev_dbg(cru->dev, "Format %ux%u bpl: %u size: %u\n",
> >  		pix->width, pix->height, pix->bytesperline, pix->sizeimage);
Lad, Prabhakar Sept. 30, 2024, 7:59 a.m. UTC | #5 
Hi Laurent,

On Sat, Sep 28, 2024 at 12:27 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Sep 27, 2024 at 12:51:24PM +0000, Prabhakar Mahadev Lad wrote:
> > Hi Daniel,
> >
> > Thank you for the patch.
> >
> >
> > > From: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> > >
> > > Rather than open-code a calculation of the format's bytesperline and
> > > sizeimage, use the v4l2_fill_pixfmt() helper. This makes it easier to
> > > support the CRU packed pixel formats without over complicating the driver.
> > >
> > > This change makes the .bpp member of struct rzg2l_cru_ip_format and the
> > > rzg2l_cru_ip_pix_fmt_to_bpp() function superfluous - remove them both.
> > >
> > > Signed-off-by: Daniel Scally <dan.scally+renesas@ideasonboard.com>
> > > ---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  3 ---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 16 --------------
> > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 21 ++-----------------
> > >  3 files changed, 2 insertions(+), 38 deletions(-)
> > >
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > This patch doesn't apply cleanly on top of media-stage + [0]
> >
> > [0] https://lore.kernel.org/all/20240910175357.229075-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
>
> Is it fine with you to wait for v3 of your series and rebase this one on
> top ?
>
Yep, sounds good to me.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index dc50a5feb3de..858098b8a13f 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -68,14 +68,12 @@  struct rzg2l_cru_ip {
  * @code: Media bus code
  * @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;
 };
 
@@ -169,7 +167,6 @@  void rzg2l_cru_ip_subdev_unregister(struct rzg2l_cru_dev *cru);
 struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
 
 const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code);
-u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
 int rzg2l_cru_ip_index_to_pix_fmt(u32 index);
 int rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
 
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 9bb192655f25..f2fea3a63444 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -16,35 +16,30 @@  static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
 		.code = MEDIA_BUS_FMT_UYVY8_1X16,
 		.format = V4L2_PIX_FMT_UYVY,
 		.datatype = MIPI_CSI2_DT_YUV422_8B,
-		.bpp = 2,
 		.icndmr = ICnDMR_YCMODE_UYVY,
 	},
 	{
 		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
 		.format = V4L2_PIX_FMT_SBGGR8,
 		.datatype = MIPI_CSI2_DT_RAW8,
-		.bpp = 1,
 		.icndmr = 0,
 	},
 	{
 		.code = MEDIA_BUS_FMT_SGBRG8_1X8,
 		.format = V4L2_PIX_FMT_SGBRG8,
 		.datatype = MIPI_CSI2_DT_RAW8,
-		.bpp = 1,
 		.icndmr = 0,
 	},
 	{
 		.code = MEDIA_BUS_FMT_SGRBG8_1X8,
 		.format = V4L2_PIX_FMT_SGRBG8,
 		.datatype = MIPI_CSI2_DT_RAW8,
-		.bpp = 1,
 		.icndmr = 0,
 	},
 	{
 		.code = MEDIA_BUS_FMT_SRGGB8_1X8,
 		.format = V4L2_PIX_FMT_SRGGB8,
 		.datatype = MIPI_CSI2_DT_RAW8,
-		.bpp = 1,
 		.icndmr = 0,
 	},
 };
@@ -60,17 +55,6 @@  const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
 	return NULL;
 }
 
-u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format)
-{
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
-		if (rzg2l_cru_ip_formats[i].format == format)
-			return rzg2l_cru_ip_formats[i].bpp;
-
-	return 0;
-}
-
 int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
 {
 	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 61e2f23053ee..01b39a2395df 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -800,27 +800,11 @@  int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
  * V4L2 stuff
  */
 
-static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
-{
-	u8 bpp;
-
-	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
-
-	if (WARN_ON(!bpp))
-		return 0;
-
-	return pix->width * bpp;
-}
-
-static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
-{
-	return pix->bytesperline * pix->height;
-}
 
 static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
 				   struct v4l2_pix_format *pix)
 {
-	if (!rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat))
+	if (rzg2l_cru_ip_pix_fmt_to_icndmr(pix->pixelformat) < 0)
 		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
 
 	switch (pix->field) {
@@ -840,8 +824,7 @@  static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
 	v4l_bound_align_image(&pix->width, 320, RZG2L_CRU_MAX_INPUT_WIDTH, 1,
 			      &pix->height, 240, RZG2L_CRU_MAX_INPUT_HEIGHT, 2, 0);
 
-	pix->bytesperline = rzg2l_cru_format_bytesperline(pix);
-	pix->sizeimage = rzg2l_cru_format_sizeimage(pix);
+	v4l2_fill_pixfmt(pix, pix->pixelformat, pix->width, pix->height);
 
 	dev_dbg(cru->dev, "Format %ux%u bpl: %u size: %u\n",
 		pix->width, pix->height, pix->bytesperline, pix->sizeimage);