From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Refactor the handling of supported formats in the RZ/G2L CRU driver by
moving the `rzg2l_cru_ip_format` struct to the common header to allow
reuse across multiple files and adding pixelformat and bpp members to it.
This change centralizes format handling, making it easier to manage and
extend.
- Moved the `rzg2l_cru_ip_format` struct to `rzg2l-cru.h` for better
accessibility.
- Added format, datatype and bpp members to `rzg2l_cru_ip_format` struct
- Dropped rzg2l_cru_formats
- Introduced helper functions `rzg2l_cru_ip_code_to_fmt()`,
`rzg2l_cru_ip_format_to_fmt()`, and
`rzg2l_cru_ip_index_to_fmt()` to streamline format lookups.
- Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
to utilize the new helpers.
Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
.../platform/renesas/rzg2l-cru/rzg2l-cru.h | 20 +++++-
.../platform/renesas/rzg2l-cru/rzg2l-ip.c | 36 ++++++++--
.../platform/renesas/rzg2l-cru/rzg2l-video.c | 67 ++++++-------------
3 files changed, 69 insertions(+), 54 deletions(-)
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 4fe24bdde5b2..39296a59b3da 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -62,6 +62,20 @@ struct rzg2l_cru_ip {
struct v4l2_subdev *remote;
};
+/**
+ * struct rzg2l_cru_ip_format - CRU IP format
+ * @code: Media bus code
+ * @format: 4CC format identifier (V4L2_PIX_FMT_*)
+ * @datatype: MIPI CSI2 data type
+ * @bpp: bytes per pixel
+ */
+struct rzg2l_cru_ip_format {
+ u32 code;
+ u32 format;
+ u32 datatype;
+ u8 bpp;
+};
+
/**
* struct rzg2l_cru_dev - Renesas CRU device structure
* @dev: (OF) device
@@ -144,10 +158,12 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru);
void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru);
irqreturn_t rzg2l_cru_irq(int irq, void *data);
-const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format);
-
int rzg2l_cru_ip_subdev_register(struct rzg2l_cru_dev *cru);
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);
+const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(u32 format);
+const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index);
+
#endif
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 7b006a0bfaae..fde6f4781cfb 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -6,17 +6,21 @@
*/
#include <linux/delay.h>
-#include "rzg2l-cru.h"
-struct rzg2l_cru_ip_format {
- u32 code;
-};
+#include <media/mipi-csi2.h>
+
+#include "rzg2l-cru.h"
static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
- { .code = MEDIA_BUS_FMT_UYVY8_1X16, },
+ {
+ .code = MEDIA_BUS_FMT_UYVY8_1X16,
+ .format = V4L2_PIX_FMT_UYVY,
+ .datatype = MIPI_CSI2_DT_YUV422_8B,
+ .bpp = 2,
+ },
};
-static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
+const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
{
unsigned int i;
@@ -27,6 +31,26 @@ static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int c
return NULL;
}
+const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(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];
+ }
+
+ return NULL;
+}
+
+const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index)
+{
+ if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
+ return NULL;
+
+ return &rzg2l_cru_ip_formats[index];
+}
+
struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru)
{
struct v4l2_subdev_state *state;
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index de88c0fab961..ceb9012c9d70 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -300,21 +300,10 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
}
-static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
- struct v4l2_mbus_framefmt *ip_sd_fmt, u8 csi_vc)
+static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, u8 csi_vc,
+ u32 csi2_datatype)
{
- u32 icnmc;
-
- switch (ip_sd_fmt->code) {
- case MEDIA_BUS_FMT_UYVY8_1X16:
- icnmc = ICnMC_INF(MIPI_CSI2_DT_YUV422_8B);
- *input_is_yuv = true;
- break;
- default:
- *input_is_yuv = false;
- icnmc = ICnMC_INF(MIPI_CSI2_DT_USER_DEFINED(0));
- break;
- }
+ u32 icnmc = ICnMC_INF(csi2_datatype);
icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
@@ -328,17 +317,20 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
struct v4l2_mbus_framefmt *ip_sd_fmt,
u8 csi_vc)
{
- bool output_is_yuv = false;
- bool input_is_yuv = false;
+ const struct v4l2_format_info *src_finfo, *dst_finfo;
+ const struct rzg2l_cru_ip_format *cru_ip_fmt;
u32 icndmr;
- rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt, csi_vc);
+ cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
+ rzg2l_cru_csi2_setup(cru, csi_vc, cru_ip_fmt->datatype);
+
+ src_finfo = v4l2_format_info(cru_ip_fmt->format);
+ dst_finfo = v4l2_format_info(cru->format.pixelformat);
/* Output format */
switch (cru->format.pixelformat) {
case V4L2_PIX_FMT_UYVY:
icndmr = ICnDMR_YCMODE_UYVY;
- output_is_yuv = true;
break;
default:
dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
@@ -347,7 +339,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
}
/* If input and output use same colorspace, do bypass mode */
- if (output_is_yuv == input_is_yuv)
+ if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))
rzg2l_cru_write(cru, ICnMC,
rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
else
@@ -810,35 +802,15 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
/* -----------------------------------------------------------------------------
* V4L2 stuff
*/
-
-static const struct v4l2_format_info rzg2l_cru_formats[] = {
- {
- .format = V4L2_PIX_FMT_UYVY,
- .bpp[0] = 2,
- },
-};
-
-const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)
-{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(rzg2l_cru_formats); i++)
- if (rzg2l_cru_formats[i].format == format)
- return rzg2l_cru_formats + i;
-
- return NULL;
-}
-
static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
{
- const struct v4l2_format_info *fmt;
-
- fmt = rzg2l_cru_format_from_pixel(pix->pixelformat);
+ const struct rzg2l_cru_ip_format *fmt;
+ fmt = rzg2l_cru_ip_format_to_fmt(pix->pixelformat);
if (WARN_ON(!fmt))
- return -EINVAL;
+ return 0;
- return pix->width * fmt->bpp[0];
+ return pix->width * fmt->bpp;
}
static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
@@ -849,7 +821,7 @@ static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
struct v4l2_pix_format *pix)
{
- if (!rzg2l_cru_format_from_pixel(pix->pixelformat))
+ if (!rzg2l_cru_ip_format_to_fmt(pix->pixelformat))
pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
switch (pix->field) {
@@ -941,10 +913,13 @@ static int rzg2l_cru_g_fmt_vid_cap(struct file *file, void *priv,
static int rzg2l_cru_enum_fmt_vid_cap(struct file *file, void *priv,
struct v4l2_fmtdesc *f)
{
- if (f->index >= ARRAY_SIZE(rzg2l_cru_formats))
+ const struct rzg2l_cru_ip_format *fmt;
+
+ fmt = rzg2l_cru_ip_index_to_fmt(f->index);
+ if (!fmt)
return -EINVAL;
- f->pixelformat = rzg2l_cru_formats[f->index].format;
+ f->pixelformat = fmt->format;
return 0;
}
--
2.43.0Hi Prabhakar,
Thank you for the patch.
On Mon, Oct 07, 2024 at 07:48:32PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Refactor the handling of supported formats in the RZ/G2L CRU driver by
> moving the `rzg2l_cru_ip_format` struct to the common header to allow
> reuse across multiple files and adding pixelformat and bpp members to it.
> This change centralizes format handling, making it easier to manage and
> extend.
>
> - Moved the `rzg2l_cru_ip_format` struct to `rzg2l-cru.h` for better
> accessibility.
> - Added format, datatype and bpp members to `rzg2l_cru_ip_format` struct
> - Dropped rzg2l_cru_formats
> - Introduced helper functions `rzg2l_cru_ip_code_to_fmt()`,
> `rzg2l_cru_ip_format_to_fmt()`, and
> `rzg2l_cru_ip_index_to_fmt()` to streamline format lookups.
> - Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
> to utilize the new helpers.
The general rule is once change, one patch. Bundling multiple changes
together makes review more difficult. A bullet list of changes in a
commit message is a sign you're bundling too many changed together.
You can still group related changes together when it makes sensor. For
instance moving rzg2l_cru_ip_format to rzg2l-cru.h and adding the
rzg2l_cru_ip_code_to_fmt() & co helper functions can be one patch.
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 20 +++++-
> .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 36 ++++++++--
> .../platform/renesas/rzg2l-cru/rzg2l-video.c | 67 ++++++-------------
> 3 files changed, 69 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 4fe24bdde5b2..39296a59b3da 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -62,6 +62,20 @@ struct rzg2l_cru_ip {
> struct v4l2_subdev *remote;
> };
>
> +/**
> + * struct rzg2l_cru_ip_format - CRU IP format
> + * @code: Media bus code
> + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> + * @datatype: MIPI CSI2 data type
> + * @bpp: bytes per pixel
> + */
> +struct rzg2l_cru_ip_format {
> + u32 code;
> + u32 format;
> + u32 datatype;
> + u8 bpp;
> +};
> +
> /**
> * struct rzg2l_cru_dev - Renesas CRU device structure
> * @dev: (OF) device
> @@ -144,10 +158,12 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru);
> void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru);
> irqreturn_t rzg2l_cru_irq(int irq, void *data);
>
> -const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format);
> -
> int rzg2l_cru_ip_subdev_register(struct rzg2l_cru_dev *cru);
> 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);
> +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(u32 format);
> +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index);
> +
> #endif
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index 7b006a0bfaae..fde6f4781cfb 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -6,17 +6,21 @@
> */
>
> #include <linux/delay.h>
> -#include "rzg2l-cru.h"
>
> -struct rzg2l_cru_ip_format {
> - u32 code;
> -};
> +#include <media/mipi-csi2.h>
> +
> +#include "rzg2l-cru.h"
>
> static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
> - { .code = MEDIA_BUS_FMT_UYVY8_1X16, },
> + {
> + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> + .format = V4L2_PIX_FMT_UYVY,
> + .datatype = MIPI_CSI2_DT_YUV422_8B,
> + .bpp = 2,
> + },
> };
>
> -static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> {
> unsigned int i;
>
> @@ -27,6 +31,26 @@ static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int c
> return NULL;
> }
>
> +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(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];
> + }
> +
> + return NULL;
> +}
> +
> +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index)
> +{
> + if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> + return NULL;
> +
> + return &rzg2l_cru_ip_formats[index];
> +}
> +
> struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru)
> {
> struct v4l2_subdev_state *state;
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index de88c0fab961..ceb9012c9d70 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -300,21 +300,10 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
> rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
> }
>
> -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> - struct v4l2_mbus_framefmt *ip_sd_fmt, u8 csi_vc)
> +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, u8 csi_vc,
> + u32 csi2_datatype)
I would pass the rzg2l_cru_ip_format pointer (make it const) to this
function instead of csi2_datatype.
> {
> - u32 icnmc;
> -
> - switch (ip_sd_fmt->code) {
> - case MEDIA_BUS_FMT_UYVY8_1X16:
> - icnmc = ICnMC_INF(MIPI_CSI2_DT_YUV422_8B);
> - *input_is_yuv = true;
> - break;
> - default:
> - *input_is_yuv = false;
> - icnmc = ICnMC_INF(MIPI_CSI2_DT_USER_DEFINED(0));
> - break;
> - }
> + u32 icnmc = ICnMC_INF(csi2_datatype);
>
> icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
>
> @@ -328,17 +317,20 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> struct v4l2_mbus_framefmt *ip_sd_fmt,
> u8 csi_vc)
> {
> - bool output_is_yuv = false;
> - bool input_is_yuv = false;
> + const struct v4l2_format_info *src_finfo, *dst_finfo;
> + const struct rzg2l_cru_ip_format *cru_ip_fmt;
> u32 icndmr;
>
> - rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt, csi_vc);
> + cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> + rzg2l_cru_csi2_setup(cru, csi_vc, cru_ip_fmt->datatype);
> +
> + src_finfo = v4l2_format_info(cru_ip_fmt->format);
> + dst_finfo = v4l2_format_info(cru->format.pixelformat);
>
> /* Output format */
> switch (cru->format.pixelformat) {
> case V4L2_PIX_FMT_UYVY:
> icndmr = ICnDMR_YCMODE_UYVY;
> - output_is_yuv = true;
> break;
> default:
> dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> @@ -347,7 +339,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> }
>
> /* If input and output use same colorspace, do bypass mode */
> - if (output_is_yuv == input_is_yuv)
> + if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))
I think this should be
if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
> rzg2l_cru_write(cru, ICnMC,
> rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
> else
> @@ -810,35 +802,15 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
> /* -----------------------------------------------------------------------------
> * V4L2 stuff
> */
> -
> -static const struct v4l2_format_info rzg2l_cru_formats[] = {
> - {
> - .format = V4L2_PIX_FMT_UYVY,
> - .bpp[0] = 2,
> - },
> -};
> -
> -const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)
> -{
> - unsigned int i;
> -
> - for (i = 0; i < ARRAY_SIZE(rzg2l_cru_formats); i++)
> - if (rzg2l_cru_formats[i].format == format)
> - return rzg2l_cru_formats + i;
> -
> - return NULL;
> -}
> -
> static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
> {
> - const struct v4l2_format_info *fmt;
> -
> - fmt = rzg2l_cru_format_from_pixel(pix->pixelformat);
> + const struct rzg2l_cru_ip_format *fmt;
>
> + fmt = rzg2l_cru_ip_format_to_fmt(pix->pixelformat);
> if (WARN_ON(!fmt))
> - return -EINVAL;
> + return 0;
This change isn't described in the commit message.
>
> - return pix->width * fmt->bpp[0];
> + return pix->width * fmt->bpp;
> }
>
> static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> @@ -849,7 +821,7 @@ static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
> struct v4l2_pix_format *pix)
> {
> - if (!rzg2l_cru_format_from_pixel(pix->pixelformat))
> + if (!rzg2l_cru_ip_format_to_fmt(pix->pixelformat))
Here you're calling rzg2l_cru_ip_format_to_fmt(), and just below the
function calls rzg2l_cru_format_bytesperline(), which calls
rzg2l_cru_format_from_pixel() again. Store the pointer here, drop the
rzg2l_cru_format_bytesperline() function, and just write
pix->bytesperline = pix->width * fmt->bpp;
below. I would also inline rzg2l_cru_format_sizeimage() in this function
as there's a single caller.
> pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
>
> switch (pix->field) {
> @@ -941,10 +913,13 @@ static int rzg2l_cru_g_fmt_vid_cap(struct file *file, void *priv,
> static int rzg2l_cru_enum_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_fmtdesc *f)
> {
> - if (f->index >= ARRAY_SIZE(rzg2l_cru_formats))
> + const struct rzg2l_cru_ip_format *fmt;
> +
> + fmt = rzg2l_cru_ip_index_to_fmt(f->index);
> + if (!fmt)
> return -EINVAL;
>
> - f->pixelformat = rzg2l_cru_formats[f->index].format;
> + f->pixelformat = fmt->format;
>
> return 0;
> }
--
Regards,
Laurent Pinchart
Hi Laurent,
Thank you for the review.
On Mon, Oct 7, 2024 at 9:21 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Mon, Oct 07, 2024 at 07:48:32PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Refactor the handling of supported formats in the RZ/G2L CRU driver by
> > moving the `rzg2l_cru_ip_format` struct to the common header to allow
> > reuse across multiple files and adding pixelformat and bpp members to it.
> > This change centralizes format handling, making it easier to manage and
> > extend.
> >
> > - Moved the `rzg2l_cru_ip_format` struct to `rzg2l-cru.h` for better
> > accessibility.
> > - Added format, datatype and bpp members to `rzg2l_cru_ip_format` struct
> > - Dropped rzg2l_cru_formats
> > - Introduced helper functions `rzg2l_cru_ip_code_to_fmt()`,
> > `rzg2l_cru_ip_format_to_fmt()`, and
> > `rzg2l_cru_ip_index_to_fmt()` to streamline format lookups.
> > - Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
> > to utilize the new helpers.
>
> The general rule is once change, one patch. Bundling multiple changes
> together makes review more difficult. A bullet list of changes in a
> commit message is a sign you're bundling too many changed together.
>
Agreed, I will henceforth take care.
> You can still group related changes together when it makes sensor. For
> instance moving rzg2l_cru_ip_format to rzg2l-cru.h and adding the
> rzg2l_cru_ip_code_to_fmt() & co helper functions can be one patch.
>
Agreed, I will split this up.
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 20 +++++-
> > .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 36 ++++++++--
> > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 67 ++++++-------------
> > 3 files changed, 69 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index 4fe24bdde5b2..39296a59b3da 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -62,6 +62,20 @@ struct rzg2l_cru_ip {
> > struct v4l2_subdev *remote;
> > };
> >
> > +/**
> > + * struct rzg2l_cru_ip_format - CRU IP format
> > + * @code: Media bus code
> > + * @format: 4CC format identifier (V4L2_PIX_FMT_*)
> > + * @datatype: MIPI CSI2 data type
> > + * @bpp: bytes per pixel
> > + */
> > +struct rzg2l_cru_ip_format {
> > + u32 code;
> > + u32 format;
> > + u32 datatype;
> > + u8 bpp;
> > +};
> > +
> > /**
> > * struct rzg2l_cru_dev - Renesas CRU device structure
> > * @dev: (OF) device
> > @@ -144,10 +158,12 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru);
> > void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru);
> > irqreturn_t rzg2l_cru_irq(int irq, void *data);
> >
> > -const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format);
> > -
> > int rzg2l_cru_ip_subdev_register(struct rzg2l_cru_dev *cru);
> > 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);
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(u32 format);
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index);
> > +
> > #endif
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > index 7b006a0bfaae..fde6f4781cfb 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> > @@ -6,17 +6,21 @@
> > */
> >
> > #include <linux/delay.h>
> > -#include "rzg2l-cru.h"
> >
> > -struct rzg2l_cru_ip_format {
> > - u32 code;
> > -};
> > +#include <media/mipi-csi2.h>
> > +
> > +#include "rzg2l-cru.h"
> >
> > static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
> > - { .code = MEDIA_BUS_FMT_UYVY8_1X16, },
> > + {
> > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > + .format = V4L2_PIX_FMT_UYVY,
> > + .datatype = MIPI_CSI2_DT_YUV422_8B,
> > + .bpp = 2,
> > + },
> > };
> >
> > -static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
> > {
> > unsigned int i;
> >
> > @@ -27,6 +31,26 @@ static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int c
> > return NULL;
> > }
> >
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_format_to_fmt(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];
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +const struct rzg2l_cru_ip_format *rzg2l_cru_ip_index_to_fmt(u32 index)
> > +{
> > + if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> > + return NULL;
> > +
> > + return &rzg2l_cru_ip_formats[index];
> > +}
> > +
> > struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru)
> > {
> > struct v4l2_subdev_state *state;
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index de88c0fab961..ceb9012c9d70 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -300,21 +300,10 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
> > rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
> > }
> >
> > -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> > - struct v4l2_mbus_framefmt *ip_sd_fmt, u8 csi_vc)
> > +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, u8 csi_vc,
> > + u32 csi2_datatype)
>
> I would pass the rzg2l_cru_ip_format pointer (make it const) to this
> function instead of csi2_datatype.
>
OK.
> > {
> > - u32 icnmc;
> > -
> > - switch (ip_sd_fmt->code) {
> > - case MEDIA_BUS_FMT_UYVY8_1X16:
> > - icnmc = ICnMC_INF(MIPI_CSI2_DT_YUV422_8B);
> > - *input_is_yuv = true;
> > - break;
> > - default:
> > - *input_is_yuv = false;
> > - icnmc = ICnMC_INF(MIPI_CSI2_DT_USER_DEFINED(0));
> > - break;
> > - }
> > + u32 icnmc = ICnMC_INF(csi2_datatype);
> >
> > icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
> >
> > @@ -328,17 +317,20 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > struct v4l2_mbus_framefmt *ip_sd_fmt,
> > u8 csi_vc)
> > {
> > - bool output_is_yuv = false;
> > - bool input_is_yuv = false;
> > + const struct v4l2_format_info *src_finfo, *dst_finfo;
> > + const struct rzg2l_cru_ip_format *cru_ip_fmt;
> > u32 icndmr;
> >
> > - rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt, csi_vc);
> > + cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
> > + rzg2l_cru_csi2_setup(cru, csi_vc, cru_ip_fmt->datatype);
> > +
> > + src_finfo = v4l2_format_info(cru_ip_fmt->format);
> > + dst_finfo = v4l2_format_info(cru->format.pixelformat);
> >
> > /* Output format */
> > switch (cru->format.pixelformat) {
> > case V4L2_PIX_FMT_UYVY:
> > icndmr = ICnDMR_YCMODE_UYVY;
> > - output_is_yuv = true;
> > break;
> > default:
> > dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> > @@ -347,7 +339,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
> > }
> >
> > /* If input and output use same colorspace, do bypass mode */
> > - if (output_is_yuv == input_is_yuv)
> > + if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))
>
> I think this should be
>
> if (v4l2_is_format_yuv(src_finfo) == v4l2_is_format_yuv(dst_finfo))
>
Agreed.
> > rzg2l_cru_write(cru, ICnMC,
> > rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
> > else
> > @@ -810,35 +802,15 @@ int rzg2l_cru_dma_register(struct rzg2l_cru_dev *cru)
> > /* -----------------------------------------------------------------------------
> > * V4L2 stuff
> > */
> > -
> > -static const struct v4l2_format_info rzg2l_cru_formats[] = {
> > - {
> > - .format = V4L2_PIX_FMT_UYVY,
> > - .bpp[0] = 2,
> > - },
> > -};
> > -
> > -const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)
> > -{
> > - unsigned int i;
> > -
> > - for (i = 0; i < ARRAY_SIZE(rzg2l_cru_formats); i++)
> > - if (rzg2l_cru_formats[i].format == format)
> > - return rzg2l_cru_formats + i;
> > -
> > - return NULL;
> > -}
> > -
> > static u32 rzg2l_cru_format_bytesperline(struct v4l2_pix_format *pix)
> > {
> > - const struct v4l2_format_info *fmt;
> > -
> > - fmt = rzg2l_cru_format_from_pixel(pix->pixelformat);
> > + const struct rzg2l_cru_ip_format *fmt;
> >
> > + fmt = rzg2l_cru_ip_format_to_fmt(pix->pixelformat);
> > if (WARN_ON(!fmt))
> > - return -EINVAL;
> > + return 0;
>
> This change isn't described in the commit message.
>
I'll make this as a separate patch.
> >
> > - return pix->width * fmt->bpp[0];
> > + return pix->width * fmt->bpp;
> > }
> >
> > static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> > @@ -849,7 +821,7 @@ static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> > static void rzg2l_cru_format_align(struct rzg2l_cru_dev *cru,
> > struct v4l2_pix_format *pix)
> > {
> > - if (!rzg2l_cru_format_from_pixel(pix->pixelformat))
> > + if (!rzg2l_cru_ip_format_to_fmt(pix->pixelformat))
>
> Here you're calling rzg2l_cru_ip_format_to_fmt(), and just below the
> function calls rzg2l_cru_format_bytesperline(), which calls
> rzg2l_cru_format_from_pixel() again. Store the pointer here, drop the
> rzg2l_cru_format_bytesperline() function, and just write
>
> pix->bytesperline = pix->width * fmt->bpp;
>
Agreed, I'll update it as mentioned above.
> below. I would also inline rzg2l_cru_format_sizeimage() in this function
> as there's a single caller.
>
OK, I'll update it as above.
Cheers,
Prabhakar
© 2016 - 2024 Red Hat, Inc.