[PATCH v3 00/17] media: platform: rzg2l-cru: CSI-2 and CRU enhancements

[PATCH v3 00/17] media: platform: rzg2l-cru: CSI-2 and CRU enhancements

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series aims to add the below:
- Retrieve virtual channel from remote subdev
- Support to capture 8bit Bayer formats.

v2->v3
- Added MUST_CONNECT flag for source pads
- Used ARRAY_SIZE() instead of NR_OF_RZG2L_CSI2_PAD
- Implemented rzg2l_cru_ip_format_to_fmt() and rzg2l_cru_ip_index_to_fmt()
- Dropped checking fmt in rzg2l_cru_initialize_image_conv()
- Dropped fse->index checks
- Implemented link_validate for video node
- Re-used rzg2l_cru_ip_format_to_fmt() to fetch icndmr details
- Moved register definitions to separate header file
- Updated subject lines and commit messages
- Collected RB tag

v1->v2
- Fixed retrieving VC from subdev
- Fixed review comments pointed by Laurent
  * Refactored supported CRU formats
  * Added MUST_CONNECT flag wherever required
  * Dropped `channel` member from `struct

v1:
Link: https://lore.kernel.org/all/20240906173947.282402-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (17):
  media: rzg2l-cru: Use RZG2L_CRU_IP_SINK/SOURCE enum entries
  media: rzg2l-cru: Mark sink and source pad with MUST_CONNECT flag
  media: rzg2l-cru: csi2: Mark sink and source pad with MUST_CONNECT
    flag
  media: rzg2l-cru: csi2: Use ARRAY_SIZE() in media_entity_pads_init()
  media: rzg2l-cru: csi2: Implement .get_frame_desc()
  media: rzg2l-cru: Retrieve virtual channel information
  media: rzg2l-cru: Remove `channel` member from `struct rzg2l_cru_csi`
  media: rzg2l-cru: Use MIPI CSI-2 data types for ICnMC_INF definitions
  media: rzg2l-cru: Remove unused fields from rzg2l_cru_ip_format struct
  media: rzg2l-cru: Simplify handling of supported formats
  media: rzg2l-cru: Use `rzg2l_cru_ip_formats` array in enum_frame_size
  media: rzg2l-cru: csi2: Remove unused field from rzg2l_csi2_format
  media: rzg2l-cru: video: Implement .link_validate() callback
  media: rzg2l-cru: csi2: Use rzg2l_csi2_formats array in
    enum_frame_size
  media: rzg2l-cru: Refactor ICnDMR register configuration
  media: rzg2l-cru: Add support to capture 8bit raw sRGB
  media: rzg2l-cru: Move register definitions to a separate file

 .../platform/renesas/rzg2l-cru/rzg2l-core.c   |   3 +-
 .../renesas/rzg2l-cru/rzg2l-cru-regs.h        |  79 +++++
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  30 +-
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   |  39 ++-
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     |  79 ++++-
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 289 ++++++++----------
 6 files changed, 325 insertions(+), 194 deletions(-)
 create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h

-- 
2.43.0

[PATCH v3 01/17] media: rzg2l-cru: Use RZG2L_CRU_IP_SINK/SOURCE enum entries

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Use enum values (`RZG2L_CRU_IP_SINK` and `RZG2L_CRU_IP_SOURCE`) instead
of hardcoded array indices.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index ac8ebae4ed07..9f0ea1de50da 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -217,8 +217,8 @@ int rzg2l_cru_ip_subdev_register(struct rzg2l_cru_dev *cru)
 	ip->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	ip->subdev.entity.ops = &rzg2l_cru_ip_entity_ops;
 
-	ip->pads[0].flags = MEDIA_PAD_FL_SINK;
-	ip->pads[1].flags = MEDIA_PAD_FL_SOURCE;
+	ip->pads[RZG2L_CRU_IP_SINK].flags = MEDIA_PAD_FL_SINK;
+	ip->pads[RZG2L_CRU_IP_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	ret = media_entity_pads_init(&ip->subdev.entity, 2, ip->pads);
 	if (ret)
-- 
2.43.0

[PATCH v3 02/17] media: rzg2l-cru: Mark sink and source pad with MUST_CONNECT flag

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Mark the sink and source pad with the MEDIA_PAD_FL_MUST_CONNECT flag to
ensure pipeline validation fails if it is not connected.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line and commit message
- Added MUST_CONNECT flag for source pad
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c | 2 +-
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c   | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
index 19e0ba9596c9..69cd45b26951 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
@@ -209,7 +209,7 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru)
 	const struct of_device_id *match;
 	int ret;
 
-	cru->pad.flags = MEDIA_PAD_FL_SINK;
+	cru->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
 	ret = media_entity_pads_init(&cru->vdev.entity, 1, &cru->pad);
 	if (ret)
 		return ret;
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 9f0ea1de50da..700d8b704689 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -217,8 +217,10 @@ int rzg2l_cru_ip_subdev_register(struct rzg2l_cru_dev *cru)
 	ip->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	ip->subdev.entity.ops = &rzg2l_cru_ip_entity_ops;
 
-	ip->pads[RZG2L_CRU_IP_SINK].flags = MEDIA_PAD_FL_SINK;
-	ip->pads[RZG2L_CRU_IP_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	ip->pads[RZG2L_CRU_IP_SINK].flags = MEDIA_PAD_FL_SINK |
+					    MEDIA_PAD_FL_MUST_CONNECT;
+	ip->pads[RZG2L_CRU_IP_SOURCE].flags = MEDIA_PAD_FL_SOURCE |
+					      MEDIA_PAD_FL_MUST_CONNECT;
 
 	ret = media_entity_pads_init(&ip->subdev.entity, 2, ip->pads);
 	if (ret)
-- 
2.43.0

[PATCH v3 03/17] media: rzg2l-cru: csi2: Mark sink and source pad with MUST_CONNECT flag

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Mark the sink and source pad with the MEDIA_PAD_FL_MUST_CONNECT flag to
ensure pipeline validation fails if it is not connected.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line and commit message
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index c7fdee347ac8..2f4c87ede8bf 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -795,13 +795,15 @@ static int rzg2l_csi2_probe(struct platform_device *pdev)
 	csi2->subdev.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
 	csi2->subdev.entity.ops = &rzg2l_csi2_entity_ops;
 
-	csi2->pads[RZG2L_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
+	csi2->pads[RZG2L_CSI2_SINK].flags = MEDIA_PAD_FL_SINK |
+					    MEDIA_PAD_FL_MUST_CONNECT;
 	/*
 	 * TODO: RZ/G2L CSI2 supports 4 virtual channels, as virtual
 	 * channels should be implemented by streams API which is under
 	 * development lets hardcode to VC0 for now.
 	 */
-	csi2->pads[RZG2L_CSI2_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	csi2->pads[RZG2L_CSI2_SOURCE].flags = MEDIA_PAD_FL_SOURCE |
+					      MEDIA_PAD_FL_MUST_CONNECT;
 	ret = media_entity_pads_init(&csi2->subdev.entity, 2, csi2->pads);
 	if (ret)
 		goto error_pm;
-- 
2.43.0

[PATCH v3 04/17] media: rzg2l-cru: csi2: Use ARRAY_SIZE() in media_entity_pads_init()

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The media_entity_pads_init() function was previously hardcoded to use a
magic number for the number of pads. Replace the magic number with the
ARRAY_SIZE() macro to determine the number of pads dynamically.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line and commit message
- Used ARRAY_SIZE() instead of NR_OF_RZG2L_CSI2_PAD
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index 2f4c87ede8bf..abacf53b944c 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -804,7 +804,8 @@ static int rzg2l_csi2_probe(struct platform_device *pdev)
 	 */
 	csi2->pads[RZG2L_CSI2_SOURCE].flags = MEDIA_PAD_FL_SOURCE |
 					      MEDIA_PAD_FL_MUST_CONNECT;
-	ret = media_entity_pads_init(&csi2->subdev.entity, 2, csi2->pads);
+	ret = media_entity_pads_init(&csi2->subdev.entity, ARRAY_SIZE(csi2->pads),
+				     csi2->pads);
 	if (ret)
 		goto error_pm;
 
-- 
2.43.0

[PATCH v3 05/17] media: rzg2l-cru: csi2: Implement .get_frame_desc()

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The RZ/G2L CRU requires information about which VCx to process data from,
among the four available VCs. To obtain this information, the
.get_frame_desc() routine is implemented. This routine, in turn, calls
.get_frame_desc() on the remote sensor connected to the CSI2 bridge.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v2->v3
- Updated subject line

v1->v2
- Collected RB tag
---
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index abacf53b944c..3fd0be6a3b65 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -582,6 +582,25 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				     struct v4l2_mbus_frame_desc *fd)
+{
+	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
+	struct media_pad *remote_pad;
+
+	if (!csi2->remote_source)
+		return -ENODEV;
+
+	remote_pad = media_pad_remote_pad_unique(&csi2->pads[RZG2L_CSI2_SINK]);
+	if (IS_ERR(remote_pad)) {
+		dev_err(csi2->dev, "can't get source pad of %s (%ld)\n",
+			csi2->remote_source->name, PTR_ERR(remote_pad));
+		return PTR_ERR(remote_pad);
+	}
+	return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc,
+				remote_pad->index, fd);
+}
+
 static const struct v4l2_subdev_video_ops rzg2l_csi2_video_ops = {
 	.s_stream = rzg2l_csi2_s_stream,
 	.pre_streamon = rzg2l_csi2_pre_streamon,
@@ -593,6 +612,7 @@ static const struct v4l2_subdev_pad_ops rzg2l_csi2_pad_ops = {
 	.enum_frame_size = rzg2l_csi2_enum_frame_size,
 	.set_fmt = rzg2l_csi2_set_format,
 	.get_fmt = v4l2_subdev_get_fmt,
+	.get_frame_desc = rzg2l_csi2_get_frame_desc,
 };
 
 static const struct v4l2_subdev_ops rzg2l_csi2_subdev_ops = {
-- 
2.43.0

[PATCH v3 06/17] media: rzg2l-cru: Retrieve virtual channel information

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
virtual channel should be processed from the four available VCs. To
retrieve this information from the connected subdevice, the
.get_frame_desc() function is called.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
v2->v3
- Updated subject line

v1->v2
- Collected RB tag
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  5 +++
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     |  5 ---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 34 +++++++++++++++++++
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 174760239548..8fbd45c43763 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -31,6 +31,11 @@
 #define RZG2L_CRU_MIN_INPUT_HEIGHT	240
 #define RZG2L_CRU_MAX_INPUT_HEIGHT	4095
 
+enum rzg2l_csi2_pads {
+	RZG2L_CRU_IP_SINK = 0,
+	RZG2L_CRU_IP_SOURCE,
+};
+
 /**
  * enum rzg2l_cru_dma_state - DMA states
  * @RZG2L_CRU_DMA_STOPPED:   No operation in progress
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 700d8b704689..aee7d4ba70b0 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -18,11 +18,6 @@ static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
 	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
 };
 
-enum rzg2l_csi2_pads {
-	RZG2L_CRU_IP_SINK = 0,
-	RZG2L_CRU_IP_SOURCE,
-};
-
 static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
 {
 	unsigned int i;
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index bbf4674f888d..7cd33eb1939c 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -433,12 +433,46 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
+static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
+{
+	struct v4l2_mbus_frame_desc fd = { };
+	struct media_pad *remote_pad;
+	int ret;
+
+	remote_pad = media_pad_remote_pad_unique(&cru->ip.pads[RZG2L_CRU_IP_SINK]);
+	ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc, remote_pad->index, &fd);
+	if (ret < 0 && ret != -ENOIOCTLCMD) {
+		dev_err(cru->dev, "get_frame_desc failed on IP remote subdev\n");
+		return ret;
+	}
+	/* If remote subdev does not implement .get_frame_desc default to VC0. */
+	if (ret == -ENOIOCTLCMD)
+		return 0;
+
+	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
+		dev_err(cru->dev, "get_frame_desc returned invalid bus type %d\n", fd.type);
+		return -EINVAL;
+	}
+
+	if (!fd.num_entries) {
+		dev_err(cru->dev, "get_frame_desc returned zero entries\n");
+		return -EINVAL;
+	}
+
+	return fd.entry[0].bus.csi2.vc;
+}
+
 int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 {
 	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
 	unsigned long flags;
 	int ret;
 
+	ret = rzg2l_cru_get_virtual_channel(cru);
+	if (ret < 0)
+		return ret;
+	cru->csi.channel = ret;
+
 	spin_lock_irqsave(&cru->qlock, flags);
 
 	/* Select a video input */
-- 
2.43.0

[PATCH v3 07/17] media: rzg2l-cru: Remove `channel` member from `struct rzg2l_cru_csi`

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Remove the CSI virtual channel number from `struct rzg2l_cru_csi`.
Instead, pass the CSI virtual channel number as an argument to
`rzg2l_cru_csi2_setup()`.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Collected RB tag

v1->v2
- New patch
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-core.c  |  1 -
 .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h   |  1 -
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c | 14 ++++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
index 69cd45b26951..b21a66e2ce5c 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
@@ -72,7 +72,6 @@ static int rzg2l_cru_group_notify_complete(struct v4l2_async_notifier *notifier)
 			source->name, sink->name);
 		return ret;
 	}
-	cru->csi.channel = 0;
 	cru->ip.remote = cru->csi.subdev;
 
 	/* Create media device link between CRU IP <-> CRU OUTPUT */
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 8fbd45c43763..4fe24bdde5b2 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -53,7 +53,6 @@ enum rzg2l_cru_dma_state {
 struct rzg2l_cru_csi {
 	struct v4l2_async_connection *asd;
 	struct v4l2_subdev *subdev;
-	u32 channel;
 };
 
 struct rzg2l_cru_ip {
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 7cd33eb1939c..9ab7ef33c9da 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -301,7 +301,7 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
 }
 
 static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
-				 struct v4l2_mbus_framefmt *ip_sd_fmt)
+				 struct v4l2_mbus_framefmt *ip_sd_fmt, u8 csi_vc)
 {
 	u32 icnmc;
 
@@ -319,19 +319,20 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
 	icnmc |= (rzg2l_cru_read(cru, ICnMC) & ~ICnMC_INF_MASK);
 
 	/* Set virtual channel CSI2 */
-	icnmc |= ICnMC_VCSEL(cru->csi.channel);
+	icnmc |= ICnMC_VCSEL(csi_vc);
 
 	rzg2l_cru_write(cru, ICnMC, icnmc);
 }
 
 static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
-					   struct v4l2_mbus_framefmt *ip_sd_fmt)
+					   struct v4l2_mbus_framefmt *ip_sd_fmt,
+					   u8 csi_vc)
 {
 	bool output_is_yuv = false;
 	bool input_is_yuv = false;
 	u32 icndmr;
 
-	rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt);
+	rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt, csi_vc);
 
 	/* Output format */
 	switch (cru->format.pixelformat) {
@@ -466,12 +467,13 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 {
 	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
 	unsigned long flags;
+	u8 csi_vc;
 	int ret;
 
 	ret = rzg2l_cru_get_virtual_channel(cru);
 	if (ret < 0)
 		return ret;
-	cru->csi.channel = ret;
+	csi_vc = ret;
 
 	spin_lock_irqsave(&cru->qlock, flags);
 
@@ -489,7 +491,7 @@ int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 	rzg2l_cru_initialize_axi(cru);
 
 	/* Initialize image convert */
-	ret = rzg2l_cru_initialize_image_conv(cru, fmt);
+	ret = rzg2l_cru_initialize_image_conv(cru, fmt, csi_vc);
 	if (ret) {
 		spin_unlock_irqrestore(&cru->qlock, flags);
 		return ret;
-- 
2.43.0

[PATCH v3 08/17] media: rzg2l-cru: Use MIPI CSI-2 data types for ICnMC_INF definitions

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The INF field in the ICnMC register accepts data type codes as specified
in the MIPI CSI-2 v2.1 specification. This patch introduces the
`ICnMC_INF()` macro to use the MIPI CSI-2 data types, which are available
in the `media/mipi-csi2.h` header.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 9ab7ef33c9da..de88c0fab961 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
 
+#include <media/mipi-csi2.h>
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-dma-contig.h>
 
@@ -77,8 +78,7 @@
 /* CRU Image Processing Main Control Register */
 #define ICnMC				0x208
 #define ICnMC_CSCTHR			BIT(5)
-#define ICnMC_INF_YUV8_422		(0x1e << 16)
-#define ICnMC_INF_USER			(0x30 << 16)
+#define ICnMC_INF(x)			((x) << 16)
 #define ICnMC_VCSEL(x)			((x) << 22)
 #define ICnMC_INF_MASK			GENMASK(21, 16)
 
@@ -307,12 +307,12 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
 
 	switch (ip_sd_fmt->code) {
 	case MEDIA_BUS_FMT_UYVY8_1X16:
-		icnmc = ICnMC_INF_YUV8_422;
+		icnmc = ICnMC_INF(MIPI_CSI2_DT_YUV422_8B);
 		*input_is_yuv = true;
 		break;
 	default:
 		*input_is_yuv = false;
-		icnmc = ICnMC_INF_USER;
+		icnmc = ICnMC_INF(MIPI_CSI2_DT_USER_DEFINED(0));
 		break;
 	}
 
-- 
2.43.0

[PATCH v3 09/17] media: rzg2l-cru: Remove unused fields from rzg2l_cru_ip_format struct

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Simplified the `rzg2l_cru_ip_format` struct by removing the unused
`datatype` and `bpp` fields from the structure in `rzg2l-ip.c`.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index aee7d4ba70b0..7b006a0bfaae 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -10,12 +10,10 @@
 
 struct rzg2l_cru_ip_format {
 	u32 code;
-	unsigned int datatype;
-	unsigned int bpp;
 };
 
 static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
-	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, },
 };
 
 static const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
-- 
2.43.0

[PATCH v3 10/17] media: rzg2l-cru: Simplify handling of supported formats

[Prabhakar]

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>
---
v2->v3
- Updated subject line and commit message
- Implemented rzg2l_cru_ip_format_to_fmt() and rzg2l_cru_ip_index_to_fmt()
- Dropped checking fmt in rzg2l_cru_initialize_image_conv()

v1->v2
- New patch
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 ++++++++--
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 67 ++++++-------------
 3 files changed, 68 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..12aac9d6cb4b 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,25 @@ 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.0

Re: [PATCH v3 10/17] media: rzg2l-cru: Simplify handling of supported formats

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

Just one minor comment below.

On Tue, Oct 01, 2024 at 03:09:12PM +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.
> 
> 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
> - Implemented rzg2l_cru_ip_format_to_fmt() and rzg2l_cru_ip_index_to_fmt()
> - Dropped checking fmt in rzg2l_cru_initialize_image_conv()
> 
> v1->v2
> - New patch
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 ++++++++--
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 67 ++++++-------------
>  3 files changed, 68 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..12aac9d6cb4b 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,25 @@ 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];

	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];
	}

Sakari can probably handle this when applying the series to his tree.

> +
> +	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.0
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 10/17] media: rzg2l-cru: Simplify handling of supported formats

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Oct 3, 2024 at 3:25 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> Just one minor comment below.
>
Missed Reviewed-by?

> On Tue, Oct 01, 2024 at 03:09:12PM +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.
> >
> > 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
> > - Implemented rzg2l_cru_ip_format_to_fmt() and rzg2l_cru_ip_index_to_fmt()
> > - Dropped checking fmt in rzg2l_cru_initialize_image_conv()
> >
> > v1->v2
> > - New patch
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 ++++++++--
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 67 ++++++-------------
> >  3 files changed, 68 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..12aac9d6cb4b 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,25 @@ 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];
>
>         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];
>         }
>
> Sakari can probably handle this when applying the series to his tree.
>
As there will be v4 anyway for fixing the link_validate(), I'll
address it for v4.

Cheers,
Prabhakar

[PATCH v3 11/17] media: rzg2l-cru: Use `rzg2l_cru_ip_formats` array in enum_frame_size

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Use the `rzg2l_cru_ip_formats` array in `rzg2l_cru_ip_enum_frame_size()`
to validate the format code.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line and commit message
- Dropped fse->index check
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index 12aac9d6cb4b..6ce077ab42e2 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -165,7 +165,7 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index != 0)
 		return -EINVAL;
 
-	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
+	if (!rzg2l_cru_ip_code_to_fmt(fse->code))
 		return -EINVAL;
 
 	fse->min_width = RZG2L_CRU_MIN_INPUT_WIDTH;
-- 
2.43.0

[PATCH v3 12/17] media: rzg2l-cru: csi2: Remove unused field from rzg2l_csi2_format

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Remove the unused `datatype` field from the `rzg2l_csi2_format` struct and
update the `rzg2l_csi2_formats[]` array to reflect the updated structure.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index 3fd0be6a3b65..7b76d495cfe4 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -183,12 +183,11 @@ static const struct rzg2l_csi2_timings rzg2l_csi2_global_timings[] = {
 
 struct rzg2l_csi2_format {
 	u32 code;
-	unsigned int datatype;
 	unsigned int bpp;
 };
 
 static const struct rzg2l_csi2_format rzg2l_csi2_formats[] = {
-	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16 },
 };
 
 static inline struct rzg2l_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
-- 
2.43.0

[PATCH v3 13/17] media: rzg2l-cru: video: Implement .link_validate() callback

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Implement the `.link_validate()` callback for the video node and move the
format checking into this function. This change allows the removal of
`rzg2l_cru_mc_validate_format()`.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
- New patch
---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 99 ++++++++++---------
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index ceb9012c9d70..c6c82b9b130a 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb)
 	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
-static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
-					struct v4l2_subdev *sd,
-					struct media_pad *pad)
-{
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-
-	fmt.pad = pad->index;
-	if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt))
-		return -EPIPE;
-
-	switch (fmt.format.code) {
-	case MEDIA_BUS_FMT_UYVY8_1X16:
-		break;
-	default:
-		return -EPIPE;
-	}
-
-	switch (fmt.format.field) {
-	case V4L2_FIELD_TOP:
-	case V4L2_FIELD_BOTTOM:
-	case V4L2_FIELD_NONE:
-	case V4L2_FIELD_INTERLACED_TB:
-	case V4L2_FIELD_INTERLACED_BT:
-	case V4L2_FIELD_INTERLACED:
-	case V4L2_FIELD_SEQ_TB:
-	case V4L2_FIELD_SEQ_BT:
-		break;
-	default:
-		return -EPIPE;
-	}
-
-	if (fmt.format.width != cru->format.width ||
-	    fmt.format.height != cru->format.height)
-		return -EPIPE;
-
-	return 0;
-}
-
 static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
 				    int slot, dma_addr_t addr)
 {
@@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
 		return stream_off_ret;
 	}
 
-	ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
-	if (ret)
-		return ret;
-
 	pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
 	ret = video_device_pipeline_start(&cru->vdev, pipe);
 	if (ret)
@@ -995,6 +951,60 @@ static const struct v4l2_file_operations rzg2l_cru_fops = {
 	.read		= vb2_fop_read,
 };
 
+/* -----------------------------------------------------------------------------
+ * Media entity operations
+ */
+
+static int rzg2l_cru_video_link_validate(struct media_link *link)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_subdev *subdev;
+	struct media_entity *entity;
+	struct rzg2l_cru_dev *cru;
+	struct media_pad *remote;
+	int ret;
+
+	entity = link->sink->entity;
+	remote = link->source;
+
+	subdev = media_entity_to_v4l2_subdev(remote->entity);
+	fmt.pad = remote->index;
+	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+	if (ret < 0)
+		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+
+	if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
+		return -EPIPE;
+
+	switch (fmt.format.field) {
+	case V4L2_FIELD_TOP:
+	case V4L2_FIELD_BOTTOM:
+	case V4L2_FIELD_NONE:
+	case V4L2_FIELD_INTERLACED_TB:
+	case V4L2_FIELD_INTERLACED_BT:
+	case V4L2_FIELD_INTERLACED:
+	case V4L2_FIELD_SEQ_TB:
+	case V4L2_FIELD_SEQ_BT:
+		break;
+	default:
+		return -EPIPE;
+	}
+
+	cru = container_of(media_entity_to_video_device(entity),
+			   struct rzg2l_cru_dev, vdev);
+	if (fmt.format.width != cru->format.width ||
+	    fmt.format.height != cru->format.height)
+		return -EPIPE;
+
+	return 0;
+}
+
+static const struct media_entity_operations rzg2l_cru_video_media_ops = {
+	.link_validate = rzg2l_cru_video_link_validate,
+};
+
 static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru)
 {
 	struct video_device *vdev = &cru->vdev;
@@ -1006,6 +1016,7 @@ static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru)
 	vdev->lock = &cru->lock;
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
 	vdev->device_caps |= V4L2_CAP_IO_MC;
+	vdev->entity.ops = &rzg2l_cru_video_media_ops;
 	vdev->fops = &rzg2l_cru_fops;
 	vdev->ioctl_ops = &rzg2l_cru_ioctl_ops;
 
-- 
2.43.0

Re: [PATCH v3 13/17] media: rzg2l-cru: video: Implement .link_validate() callback

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Oct 01, 2024 at 03:09:15PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Implement the `.link_validate()` callback for the video node and move the
> format checking into this function. This change allows the removal of
> `rzg2l_cru_mc_validate_format()`.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - New patch
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 99 ++++++++++---------
>  1 file changed, 55 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index ceb9012c9d70..c6c82b9b130a 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb)
>  	spin_unlock_irqrestore(&cru->qlock, flags);
>  }
>  
> -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
> -					struct v4l2_subdev *sd,
> -					struct media_pad *pad)
> -{
> -	struct v4l2_subdev_format fmt = {
> -		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> -	};
> -
> -	fmt.pad = pad->index;
> -	if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt))
> -		return -EPIPE;
> -
> -	switch (fmt.format.code) {
> -	case MEDIA_BUS_FMT_UYVY8_1X16:
> -		break;
> -	default:
> -		return -EPIPE;
> -	}
> -
> -	switch (fmt.format.field) {
> -	case V4L2_FIELD_TOP:
> -	case V4L2_FIELD_BOTTOM:
> -	case V4L2_FIELD_NONE:
> -	case V4L2_FIELD_INTERLACED_TB:
> -	case V4L2_FIELD_INTERLACED_BT:
> -	case V4L2_FIELD_INTERLACED:
> -	case V4L2_FIELD_SEQ_TB:
> -	case V4L2_FIELD_SEQ_BT:
> -		break;
> -	default:
> -		return -EPIPE;
> -	}
> -
> -	if (fmt.format.width != cru->format.width ||
> -	    fmt.format.height != cru->format.height)
> -		return -EPIPE;
> -
> -	return 0;
> -}
> -
>  static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
>  				    int slot, dma_addr_t addr)
>  {
> @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
>  		return stream_off_ret;
>  	}
>  
> -	ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
> -	if (ret)
> -		return ret;
> -
>  	pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
>  	ret = video_device_pipeline_start(&cru->vdev, pipe);
>  	if (ret)
> @@ -995,6 +951,60 @@ static const struct v4l2_file_operations rzg2l_cru_fops = {
>  	.read		= vb2_fop_read,
>  };
>  
> +/* -----------------------------------------------------------------------------
> + * Media entity operations
> + */
> +
> +static int rzg2l_cru_video_link_validate(struct media_link *link)
> +{
> +	struct v4l2_subdev_format fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	struct v4l2_subdev *subdev;
> +	struct media_entity *entity;
> +	struct rzg2l_cru_dev *cru;
> +	struct media_pad *remote;
> +	int ret;
> +
> +	entity = link->sink->entity;
> +	remote = link->source;
> +
> +	subdev = media_entity_to_v4l2_subdev(remote->entity);
> +	fmt.pad = remote->index;
> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> +	if (ret < 0)
> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> +
> +	if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
> +		return -EPIPE;

Here you should check that the format on the subdev matches the format
on the video device.

> +
> +	switch (fmt.format.field) {
> +	case V4L2_FIELD_TOP:
> +	case V4L2_FIELD_BOTTOM:
> +	case V4L2_FIELD_NONE:
> +	case V4L2_FIELD_INTERLACED_TB:
> +	case V4L2_FIELD_INTERLACED_BT:
> +	case V4L2_FIELD_INTERLACED:
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
> +		break;
> +	default:
> +		return -EPIPE;
> +	}

Instead of checking the field here, shouldn't it be forced to a valid
value in the subdev .set_fmt() function ? The link validation handler is
responsible for validating that the configuration of the two sides of
the link (IP subdev and video device) match. The driver shouldn't allow
setting formats that can't be supported.

What you should check here is that the field of the subdev and the
field of the video device match.

> +
> +	cru = container_of(media_entity_to_video_device(entity),

You can drop the local entity variable and write

	cru = container_of(media_entity_to_video_device(link->sink->entity),

> +			   struct rzg2l_cru_dev, vdev);
> +	if (fmt.format.width != cru->format.width ||
> +	    fmt.format.height != cru->format.height)
> +		return -EPIPE;
> +
> +	return 0;
> +}
> +
> +static const struct media_entity_operations rzg2l_cru_video_media_ops = {
> +	.link_validate = rzg2l_cru_video_link_validate,
> +};
> +
>  static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru)
>  {
>  	struct video_device *vdev = &cru->vdev;
> @@ -1006,6 +1016,7 @@ static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru)
>  	vdev->lock = &cru->lock;
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>  	vdev->device_caps |= V4L2_CAP_IO_MC;
> +	vdev->entity.ops = &rzg2l_cru_video_media_ops;
>  	vdev->fops = &rzg2l_cru_fops;
>  	vdev->ioctl_ops = &rzg2l_cru_ioctl_ops;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 13/17] media: rzg2l-cru: video: Implement .link_validate() callback

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Oct 3, 2024 at 3:51 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Oct 01, 2024 at 03:09:15PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Implement the `.link_validate()` callback for the video node and move the
> > format checking into this function. This change allows the removal of
> > `rzg2l_cru_mc_validate_format()`.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > - New patch
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 99 ++++++++++---------
> >  1 file changed, 55 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index ceb9012c9d70..c6c82b9b130a 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb)
> >       spin_unlock_irqrestore(&cru->qlock, flags);
> >  }
> >
> > -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
> > -                                     struct v4l2_subdev *sd,
> > -                                     struct media_pad *pad)
> > -{
> > -     struct v4l2_subdev_format fmt = {
> > -             .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > -     };
> > -
> > -     fmt.pad = pad->index;
> > -     if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt))
> > -             return -EPIPE;
> > -
> > -     switch (fmt.format.code) {
> > -     case MEDIA_BUS_FMT_UYVY8_1X16:
> > -             break;
> > -     default:
> > -             return -EPIPE;
> > -     }
> > -
> > -     switch (fmt.format.field) {
> > -     case V4L2_FIELD_TOP:
> > -     case V4L2_FIELD_BOTTOM:
> > -     case V4L2_FIELD_NONE:
> > -     case V4L2_FIELD_INTERLACED_TB:
> > -     case V4L2_FIELD_INTERLACED_BT:
> > -     case V4L2_FIELD_INTERLACED:
> > -     case V4L2_FIELD_SEQ_TB:
> > -     case V4L2_FIELD_SEQ_BT:
> > -             break;
> > -     default:
> > -             return -EPIPE;
> > -     }
> > -
> > -     if (fmt.format.width != cru->format.width ||
> > -         fmt.format.height != cru->format.height)
> > -             return -EPIPE;
> > -
> > -     return 0;
> > -}
> > -
> >  static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
> >                                   int slot, dma_addr_t addr)
> >  {
> > @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
> >               return stream_off_ret;
> >       }
> >
> > -     ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
> > -     if (ret)
> > -             return ret;
> > -
> >       pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
> >       ret = video_device_pipeline_start(&cru->vdev, pipe);
> >       if (ret)
> > @@ -995,6 +951,60 @@ static const struct v4l2_file_operations rzg2l_cru_fops = {
> >       .read           = vb2_fop_read,
> >  };
> >
> > +/* -----------------------------------------------------------------------------
> > + * Media entity operations
> > + */
> > +
> > +static int rzg2l_cru_video_link_validate(struct media_link *link)
> > +{
> > +     struct v4l2_subdev_format fmt = {
> > +             .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +     };
> > +     struct v4l2_subdev *subdev;
> > +     struct media_entity *entity;
> > +     struct rzg2l_cru_dev *cru;
> > +     struct media_pad *remote;
> > +     int ret;
> > +
> > +     entity = link->sink->entity;
> > +     remote = link->source;
> > +
> > +     subdev = media_entity_to_v4l2_subdev(remote->entity);
> > +     fmt.pad = remote->index;
> > +     ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
> > +     if (ret < 0)
> > +             return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> > +
> > +     if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
> > +             return -EPIPE;
>
> Here you should check that the format on the subdev matches the format
> on the video device.
>
OK, I'll use the return value from rzg2l_cru_ip_code_to_fmt() and
match it with the video device node.

> > +
> > +     switch (fmt.format.field) {
> > +     case V4L2_FIELD_TOP:
> > +     case V4L2_FIELD_BOTTOM:
> > +     case V4L2_FIELD_NONE:
> > +     case V4L2_FIELD_INTERLACED_TB:
> > +     case V4L2_FIELD_INTERLACED_BT:
> > +     case V4L2_FIELD_INTERLACED:
> > +     case V4L2_FIELD_SEQ_TB:
> > +     case V4L2_FIELD_SEQ_BT:
> > +             break;
> > +     default:
> > +             return -EPIPE;
> > +     }
>
> Instead of checking the field here, shouldn't it be forced to a valid
> value in the subdev .set_fmt() function ? The link validation handler is
> responsible for validating that the configuration of the two sides of
> the link (IP subdev and video device) match. The driver shouldn't allow
> setting formats that can't be supported.
>
Agreed, this is already taken care of in .set_fmt(), so I'll drop this
check here.

> What you should check here is that the field of the subdev and the
> field of the video device match.
>
OK, I'll add a check for this.

> > +
> > +     cru = container_of(media_entity_to_video_device(entity),
>
> You can drop the local entity variable and write
>
OK.

Cheers,
Prabhakar

[PATCH v3 14/17] media: rzg2l-cru: csi2: Use rzg2l_csi2_formats array in enum_frame_size

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Make use of `rzg2l_csi2_formats` array in rzg2l_csi2_enum_frame_size() to
validate if the `fse->code` is supported.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Dropped fse->index check
- Collected RB tag

v1->v2
- New patch
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index 7b76d495cfe4..e21142d3b67d 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -573,6 +573,9 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index != 0)
 		return -EINVAL;
 
+	if (!rzg2l_csi2_code_to_fmt(fse->code))
+		return -EINVAL;
+
 	fse->min_width = RZG2L_CSI2_MIN_WIDTH;
 	fse->min_height = RZG2L_CSI2_MIN_HEIGHT;
 	fse->max_width = RZG2L_CSI2_MAX_WIDTH;
-- 
2.43.0

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

[Prabhakar]

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;
 
 	/* If input and output use same colorspace, do bypass mode */
 	if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))
-- 
2.43.0

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

[Laurent Pinchart]

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

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

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Oct 3, 2024 at 3:34 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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);
>
OK, I'll get rid of the local var.

Cheers,
Prabhakar

> 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

[PATCH v3 16/17] media: rzg2l-cru: Add support to capture 8bit raw sRGB

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add support to capture 8bit Bayer formats.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v3
- Updated subject line
- Collected RB tag

v1->v2
- Used the simplified version of format handling
---
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   |  4 +++
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 28 +++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index e21142d3b67d..84230926875a 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -188,6 +188,10 @@ struct rzg2l_csi2_format {
 
 static const struct rzg2l_csi2_format rzg2l_csi2_formats[] = {
 	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
 };
 
 static inline struct rzg2l_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index f14ac949cc64..7e3f725eeece 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -19,6 +19,34 @@ static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
 		.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,
+	},
 };
 
 const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code)
-- 
2.43.0

[PATCH v3 17/17] media: rzg2l-cru: Move register definitions to a separate file

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Move the RZ/G2L CRU register definitions from `rzg2l-video.c` to a
dedicated header file, `rzg2l-cru-regs.h`. Separating these definitions
into their own file improves the readability of the code.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3
- New patch
---
 .../renesas/rzg2l-cru/rzg2l-cru-regs.h        | 79 +++++++++++++++++++
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 +---------------
 2 files changed, 80 insertions(+), 68 deletions(-)
 create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
new file mode 100644
index 000000000000..458f7452e5d3
--- /dev/null
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * rzg2l-cru-regs.h--RZ/G2L (and alike SoCs) CRU Registers Definitions
+ *
+ * Copyright (C) 2024 Renesas Electronics Corp.
+ */
+
+#ifndef __RZG2L_CRU_REGS_H__
+#define __RZG2L_CRU_REGS_H__
+
+/* HW CRU Registers Definition */
+
+/* CRU Control Register */
+#define CRUnCTRL			0x0
+#define CRUnCTRL_VINSEL(x)		((x) << 0)
+
+/* CRU Interrupt Enable Register */
+#define CRUnIE				0x4
+#define CRUnIE_EFE			BIT(17)
+
+/* CRU Interrupt Status Register */
+#define CRUnINTS			0x8
+#define CRUnINTS_SFS			BIT(16)
+
+/* CRU Reset Register */
+#define CRUnRST				0xc
+#define CRUnRST_VRESETN			BIT(0)
+
+/* Memory Bank Base Address (Lower) Register for CRU Image Data */
+#define AMnMBxADDRL(x)			(0x100 + ((x) * 8))
+
+/* Memory Bank Base Address (Higher) Register for CRU Image Data */
+#define AMnMBxADDRH(x)			(0x104 + ((x) * 8))
+
+/* Memory Bank Enable Register for CRU Image Data */
+#define AMnMBVALID			0x148
+#define AMnMBVALID_MBVALID(x)		GENMASK(x, 0)
+
+/* Memory Bank Status Register for CRU Image Data */
+#define AMnMBS				0x14c
+#define AMnMBS_MBSTS			0x7
+
+/* AXI Master Transfer Setting Register for CRU Image Data */
+#define AMnAXIATTR			0x158
+#define AMnAXIATTR_AXILEN_MASK		GENMASK(3, 0)
+#define AMnAXIATTR_AXILEN		(0xf)
+
+/* AXI Master FIFO Pointer Register for CRU Image Data */
+#define AMnFIFOPNTR			0x168
+#define AMnFIFOPNTR_FIFOWPNTR		GENMASK(7, 0)
+#define AMnFIFOPNTR_FIFORPNTR_Y		GENMASK(23, 16)
+
+/* AXI Master Transfer Stop Register for CRU Image Data */
+#define AMnAXISTP			0x174
+#define AMnAXISTP_AXI_STOP		BIT(0)
+
+/* AXI Master Transfer Stop Status Register for CRU Image Data */
+#define AMnAXISTPACK			0x178
+#define AMnAXISTPACK_AXI_STOP_ACK	BIT(0)
+
+/* CRU Image Processing Enable Register */
+#define ICnEN				0x200
+#define ICnEN_ICEN			BIT(0)
+
+/* CRU Image Processing Main Control Register */
+#define ICnMC				0x208
+#define ICnMC_CSCTHR			BIT(5)
+#define ICnMC_INF(x)			((x) << 16)
+#define ICnMC_VCSEL(x)			((x) << 22)
+#define ICnMC_INF_MASK			GENMASK(21, 16)
+
+/* CRU Module Status Register */
+#define ICnMS				0x254
+#define ICnMS_IA			BIT(2)
+
+/* CRU Data Output Mode Register */
+#define ICnDMR				0x26c
+
+#endif /* __RZG2L_CRU_REGS_H__ */
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index c3d10b001b7c..d7c82c7b9044 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -20,74 +20,7 @@
 #include <media/videobuf2-dma-contig.h>
 
 #include "rzg2l-cru.h"
-
-/* HW CRU Registers Definition */
-
-/* CRU Control Register */
-#define CRUnCTRL			0x0
-#define CRUnCTRL_VINSEL(x)		((x) << 0)
-
-/* CRU Interrupt Enable Register */
-#define CRUnIE				0x4
-#define CRUnIE_EFE			BIT(17)
-
-/* CRU Interrupt Status Register */
-#define CRUnINTS			0x8
-#define CRUnINTS_SFS			BIT(16)
-
-/* CRU Reset Register */
-#define CRUnRST				0xc
-#define CRUnRST_VRESETN			BIT(0)
-
-/* Memory Bank Base Address (Lower) Register for CRU Image Data */
-#define AMnMBxADDRL(x)			(0x100 + ((x) * 8))
-
-/* Memory Bank Base Address (Higher) Register for CRU Image Data */
-#define AMnMBxADDRH(x)			(0x104 + ((x) * 8))
-
-/* Memory Bank Enable Register for CRU Image Data */
-#define AMnMBVALID			0x148
-#define AMnMBVALID_MBVALID(x)		GENMASK(x, 0)
-
-/* Memory Bank Status Register for CRU Image Data */
-#define AMnMBS				0x14c
-#define AMnMBS_MBSTS			0x7
-
-/* AXI Master Transfer Setting Register for CRU Image Data */
-#define AMnAXIATTR			0x158
-#define AMnAXIATTR_AXILEN_MASK		GENMASK(3, 0)
-#define AMnAXIATTR_AXILEN		(0xf)
-
-/* AXI Master FIFO Pointer Register for CRU Image Data */
-#define AMnFIFOPNTR			0x168
-#define AMnFIFOPNTR_FIFOWPNTR		GENMASK(7, 0)
-#define AMnFIFOPNTR_FIFORPNTR_Y		GENMASK(23, 16)
-
-/* AXI Master Transfer Stop Register for CRU Image Data */
-#define AMnAXISTP			0x174
-#define AMnAXISTP_AXI_STOP		BIT(0)
-
-/* AXI Master Transfer Stop Status Register for CRU Image Data */
-#define AMnAXISTPACK			0x178
-#define AMnAXISTPACK_AXI_STOP_ACK	BIT(0)
-
-/* CRU Image Processing Enable Register */
-#define ICnEN				0x200
-#define ICnEN_ICEN			BIT(0)
-
-/* CRU Image Processing Main Control Register */
-#define ICnMC				0x208
-#define ICnMC_CSCTHR			BIT(5)
-#define ICnMC_INF(x)			((x) << 16)
-#define ICnMC_VCSEL(x)			((x) << 22)
-#define ICnMC_INF_MASK			GENMASK(21, 16)
-
-/* CRU Module Status Register */
-#define ICnMS				0x254
-#define ICnMS_IA			BIT(2)
-
-/* CRU Data Output Mode Register */
-#define ICnDMR				0x26c
+#include "rzg2l-cru-regs.h"
 
 #define RZG2L_TIMEOUT_MS		100
 #define RZG2L_RETRIES			10
-- 
2.43.0

Re: [PATCH v3 17/17] media: rzg2l-cru: Move register definitions to a separate file

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Oct 01, 2024 at 03:09:19PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Move the RZ/G2L CRU register definitions from `rzg2l-video.c` to a
> dedicated header file, `rzg2l-cru-regs.h`. Separating these definitions
> into their own file improves the readability of the code.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - New patch
> ---
>  .../renesas/rzg2l-cru/rzg2l-cru-regs.h        | 79 +++++++++++++++++++
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 +---------------
>  2 files changed, 80 insertions(+), 68 deletions(-)
>  create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> new file mode 100644
> index 000000000000..458f7452e5d3
> --- /dev/null
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * rzg2l-cru-regs.h--RZ/G2L (and alike SoCs) CRU Registers Definitions
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#ifndef __RZG2L_CRU_REGS_H__
> +#define __RZG2L_CRU_REGS_H__
> +
> +/* HW CRU Registers Definition */
> +
> +/* CRU Control Register */
> +#define CRUnCTRL			0x0
> +#define CRUnCTRL_VINSEL(x)		((x) << 0)
> +
> +/* CRU Interrupt Enable Register */
> +#define CRUnIE				0x4
> +#define CRUnIE_EFE			BIT(17)
> +
> +/* CRU Interrupt Status Register */
> +#define CRUnINTS			0x8
> +#define CRUnINTS_SFS			BIT(16)
> +
> +/* CRU Reset Register */
> +#define CRUnRST				0xc
> +#define CRUnRST_VRESETN			BIT(0)
> +
> +/* Memory Bank Base Address (Lower) Register for CRU Image Data */
> +#define AMnMBxADDRL(x)			(0x100 + ((x) * 8))
> +
> +/* Memory Bank Base Address (Higher) Register for CRU Image Data */
> +#define AMnMBxADDRH(x)			(0x104 + ((x) * 8))
> +
> +/* Memory Bank Enable Register for CRU Image Data */
> +#define AMnMBVALID			0x148
> +#define AMnMBVALID_MBVALID(x)		GENMASK(x, 0)
> +
> +/* Memory Bank Status Register for CRU Image Data */
> +#define AMnMBS				0x14c
> +#define AMnMBS_MBSTS			0x7
> +
> +/* AXI Master Transfer Setting Register for CRU Image Data */
> +#define AMnAXIATTR			0x158
> +#define AMnAXIATTR_AXILEN_MASK		GENMASK(3, 0)
> +#define AMnAXIATTR_AXILEN		(0xf)
> +
> +/* AXI Master FIFO Pointer Register for CRU Image Data */
> +#define AMnFIFOPNTR			0x168
> +#define AMnFIFOPNTR_FIFOWPNTR		GENMASK(7, 0)
> +#define AMnFIFOPNTR_FIFORPNTR_Y		GENMASK(23, 16)
> +
> +/* AXI Master Transfer Stop Register for CRU Image Data */
> +#define AMnAXISTP			0x174
> +#define AMnAXISTP_AXI_STOP		BIT(0)
> +
> +/* AXI Master Transfer Stop Status Register for CRU Image Data */
> +#define AMnAXISTPACK			0x178
> +#define AMnAXISTPACK_AXI_STOP_ACK	BIT(0)
> +
> +/* CRU Image Processing Enable Register */
> +#define ICnEN				0x200
> +#define ICnEN_ICEN			BIT(0)
> +
> +/* CRU Image Processing Main Control Register */
> +#define ICnMC				0x208
> +#define ICnMC_CSCTHR			BIT(5)
> +#define ICnMC_INF(x)			((x) << 16)
> +#define ICnMC_VCSEL(x)			((x) << 22)
> +#define ICnMC_INF_MASK			GENMASK(21, 16)
> +
> +/* CRU Module Status Register */
> +#define ICnMS				0x254
> +#define ICnMS_IA			BIT(2)
> +
> +/* CRU Data Output Mode Register */
> +#define ICnDMR				0x26c

The ICnDMR_YCMODE_UYVY macro from
drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h should also be
moved here. With that,

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

> +
> +#endif /* __RZG2L_CRU_REGS_H__ */
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index c3d10b001b7c..d7c82c7b9044 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -20,74 +20,7 @@
>  #include <media/videobuf2-dma-contig.h>
>  
>  #include "rzg2l-cru.h"
> -
> -/* HW CRU Registers Definition */
> -
> -/* CRU Control Register */
> -#define CRUnCTRL			0x0
> -#define CRUnCTRL_VINSEL(x)		((x) << 0)
> -
> -/* CRU Interrupt Enable Register */
> -#define CRUnIE				0x4
> -#define CRUnIE_EFE			BIT(17)
> -
> -/* CRU Interrupt Status Register */
> -#define CRUnINTS			0x8
> -#define CRUnINTS_SFS			BIT(16)
> -
> -/* CRU Reset Register */
> -#define CRUnRST				0xc
> -#define CRUnRST_VRESETN			BIT(0)
> -
> -/* Memory Bank Base Address (Lower) Register for CRU Image Data */
> -#define AMnMBxADDRL(x)			(0x100 + ((x) * 8))
> -
> -/* Memory Bank Base Address (Higher) Register for CRU Image Data */
> -#define AMnMBxADDRH(x)			(0x104 + ((x) * 8))
> -
> -/* Memory Bank Enable Register for CRU Image Data */
> -#define AMnMBVALID			0x148
> -#define AMnMBVALID_MBVALID(x)		GENMASK(x, 0)
> -
> -/* Memory Bank Status Register for CRU Image Data */
> -#define AMnMBS				0x14c
> -#define AMnMBS_MBSTS			0x7
> -
> -/* AXI Master Transfer Setting Register for CRU Image Data */
> -#define AMnAXIATTR			0x158
> -#define AMnAXIATTR_AXILEN_MASK		GENMASK(3, 0)
> -#define AMnAXIATTR_AXILEN		(0xf)
> -
> -/* AXI Master FIFO Pointer Register for CRU Image Data */
> -#define AMnFIFOPNTR			0x168
> -#define AMnFIFOPNTR_FIFOWPNTR		GENMASK(7, 0)
> -#define AMnFIFOPNTR_FIFORPNTR_Y		GENMASK(23, 16)
> -
> -/* AXI Master Transfer Stop Register for CRU Image Data */
> -#define AMnAXISTP			0x174
> -#define AMnAXISTP_AXI_STOP		BIT(0)
> -
> -/* AXI Master Transfer Stop Status Register for CRU Image Data */
> -#define AMnAXISTPACK			0x178
> -#define AMnAXISTPACK_AXI_STOP_ACK	BIT(0)
> -
> -/* CRU Image Processing Enable Register */
> -#define ICnEN				0x200
> -#define ICnEN_ICEN			BIT(0)
> -
> -/* CRU Image Processing Main Control Register */
> -#define ICnMC				0x208
> -#define ICnMC_CSCTHR			BIT(5)
> -#define ICnMC_INF(x)			((x) << 16)
> -#define ICnMC_VCSEL(x)			((x) << 22)
> -#define ICnMC_INF_MASK			GENMASK(21, 16)
> -
> -/* CRU Module Status Register */
> -#define ICnMS				0x254
> -#define ICnMS_IA			BIT(2)
> -
> -/* CRU Data Output Mode Register */
> -#define ICnDMR				0x26c
> +#include "rzg2l-cru-regs.h"
>  
>  #define RZG2L_TIMEOUT_MS		100
>  #define RZG2L_RETRIES			10

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 17/17] media: rzg2l-cru: Move register definitions to a separate file

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Oct 3, 2024 at 3:36 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Oct 01, 2024 at 03:09:19PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Move the RZ/G2L CRU register definitions from `rzg2l-video.c` to a
> > dedicated header file, `rzg2l-cru-regs.h`. Separating these definitions
> > into their own file improves the readability of the code.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3
> > - New patch
> > ---
> >  .../renesas/rzg2l-cru/rzg2l-cru-regs.h        | 79 +++++++++++++++++++
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 69 +---------------
> >  2 files changed, 80 insertions(+), 68 deletions(-)
> >  create mode 100644 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> > new file mode 100644
> > index 000000000000..458f7452e5d3
> > --- /dev/null
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru-regs.h
> > @@ -0,0 +1,79 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * rzg2l-cru-regs.h--RZ/G2L (and alike SoCs) CRU Registers Definitions
> > + *
> > + * Copyright (C) 2024 Renesas Electronics Corp.
> > + */
> > +
> > +#ifndef __RZG2L_CRU_REGS_H__
> > +#define __RZG2L_CRU_REGS_H__
> > +
> > +/* HW CRU Registers Definition */
> > +
> > +/* CRU Control Register */
> > +#define CRUnCTRL                     0x0
> > +#define CRUnCTRL_VINSEL(x)           ((x) << 0)
> > +
> > +/* CRU Interrupt Enable Register */
> > +#define CRUnIE                               0x4
> > +#define CRUnIE_EFE                   BIT(17)
> > +
> > +/* CRU Interrupt Status Register */
> > +#define CRUnINTS                     0x8
> > +#define CRUnINTS_SFS                 BIT(16)
> > +
> > +/* CRU Reset Register */
> > +#define CRUnRST                              0xc
> > +#define CRUnRST_VRESETN                      BIT(0)
> > +
> > +/* Memory Bank Base Address (Lower) Register for CRU Image Data */
> > +#define AMnMBxADDRL(x)                       (0x100 + ((x) * 8))
> > +
> > +/* Memory Bank Base Address (Higher) Register for CRU Image Data */
> > +#define AMnMBxADDRH(x)                       (0x104 + ((x) * 8))
> > +
> > +/* Memory Bank Enable Register for CRU Image Data */
> > +#define AMnMBVALID                   0x148
> > +#define AMnMBVALID_MBVALID(x)                GENMASK(x, 0)
> > +
> > +/* Memory Bank Status Register for CRU Image Data */
> > +#define AMnMBS                               0x14c
> > +#define AMnMBS_MBSTS                 0x7
> > +
> > +/* AXI Master Transfer Setting Register for CRU Image Data */
> > +#define AMnAXIATTR                   0x158
> > +#define AMnAXIATTR_AXILEN_MASK               GENMASK(3, 0)
> > +#define AMnAXIATTR_AXILEN            (0xf)
> > +
> > +/* AXI Master FIFO Pointer Register for CRU Image Data */
> > +#define AMnFIFOPNTR                  0x168
> > +#define AMnFIFOPNTR_FIFOWPNTR                GENMASK(7, 0)
> > +#define AMnFIFOPNTR_FIFORPNTR_Y              GENMASK(23, 16)
> > +
> > +/* AXI Master Transfer Stop Register for CRU Image Data */
> > +#define AMnAXISTP                    0x174
> > +#define AMnAXISTP_AXI_STOP           BIT(0)
> > +
> > +/* AXI Master Transfer Stop Status Register for CRU Image Data */
> > +#define AMnAXISTPACK                 0x178
> > +#define AMnAXISTPACK_AXI_STOP_ACK    BIT(0)
> > +
> > +/* CRU Image Processing Enable Register */
> > +#define ICnEN                                0x200
> > +#define ICnEN_ICEN                   BIT(0)
> > +
> > +/* CRU Image Processing Main Control Register */
> > +#define ICnMC                                0x208
> > +#define ICnMC_CSCTHR                 BIT(5)
> > +#define ICnMC_INF(x)                 ((x) << 16)
> > +#define ICnMC_VCSEL(x)                       ((x) << 22)
> > +#define ICnMC_INF_MASK                       GENMASK(21, 16)
> > +
> > +/* CRU Module Status Register */
> > +#define ICnMS                                0x254
> > +#define ICnMS_IA                     BIT(2)
> > +
> > +/* CRU Data Output Mode Register */
> > +#define ICnDMR                               0x26c
>
> The ICnDMR_YCMODE_UYVY macro from
> drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h should also be
> moved here. With that,
>
Agreed, I'll move ICnDMR_YCMODE_UYVY macro here.

Cheers,
Prabhakar

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> > +
> > +#endif /* __RZG2L_CRU_REGS_H__ */
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index c3d10b001b7c..d7c82c7b9044 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -20,74 +20,7 @@
> >  #include <media/videobuf2-dma-contig.h>
> >
> >  #include "rzg2l-cru.h"
> > -
> > -/* HW CRU Registers Definition */
> > -
> > -/* CRU Control Register */
> > -#define CRUnCTRL                     0x0
> > -#define CRUnCTRL_VINSEL(x)           ((x) << 0)
> > -
> > -/* CRU Interrupt Enable Register */
> > -#define CRUnIE                               0x4
> > -#define CRUnIE_EFE                   BIT(17)
> > -
> > -/* CRU Interrupt Status Register */
> > -#define CRUnINTS                     0x8
> > -#define CRUnINTS_SFS                 BIT(16)
> > -
> > -/* CRU Reset Register */
> > -#define CRUnRST                              0xc
> > -#define CRUnRST_VRESETN                      BIT(0)
> > -
> > -/* Memory Bank Base Address (Lower) Register for CRU Image Data */
> > -#define AMnMBxADDRL(x)                       (0x100 + ((x) * 8))
> > -
> > -/* Memory Bank Base Address (Higher) Register for CRU Image Data */
> > -#define AMnMBxADDRH(x)                       (0x104 + ((x) * 8))
> > -
> > -/* Memory Bank Enable Register for CRU Image Data */
> > -#define AMnMBVALID                   0x148
> > -#define AMnMBVALID_MBVALID(x)                GENMASK(x, 0)
> > -
> > -/* Memory Bank Status Register for CRU Image Data */
> > -#define AMnMBS                               0x14c
> > -#define AMnMBS_MBSTS                 0x7
> > -
> > -/* AXI Master Transfer Setting Register for CRU Image Data */
> > -#define AMnAXIATTR                   0x158
> > -#define AMnAXIATTR_AXILEN_MASK               GENMASK(3, 0)
> > -#define AMnAXIATTR_AXILEN            (0xf)
> > -
> > -/* AXI Master FIFO Pointer Register for CRU Image Data */
> > -#define AMnFIFOPNTR                  0x168
> > -#define AMnFIFOPNTR_FIFOWPNTR                GENMASK(7, 0)
> > -#define AMnFIFOPNTR_FIFORPNTR_Y              GENMASK(23, 16)
> > -
> > -/* AXI Master Transfer Stop Register for CRU Image Data */
> > -#define AMnAXISTP                    0x174
> > -#define AMnAXISTP_AXI_STOP           BIT(0)
> > -
> > -/* AXI Master Transfer Stop Status Register for CRU Image Data */
> > -#define AMnAXISTPACK                 0x178
> > -#define AMnAXISTPACK_AXI_STOP_ACK    BIT(0)
> > -
> > -/* CRU Image Processing Enable Register */
> > -#define ICnEN                                0x200
> > -#define ICnEN_ICEN                   BIT(0)
> > -
> > -/* CRU Image Processing Main Control Register */
> > -#define ICnMC                                0x208
> > -#define ICnMC_CSCTHR                 BIT(5)
> > -#define ICnMC_INF(x)                 ((x) << 16)
> > -#define ICnMC_VCSEL(x)                       ((x) << 22)
> > -#define ICnMC_INF_MASK                       GENMASK(21, 16)
> > -
> > -/* CRU Module Status Register */
> > -#define ICnMS                                0x254
> > -#define ICnMS_IA                     BIT(2)
> > -
> > -/* CRU Data Output Mode Register */
> > -#define ICnDMR                               0x26c
> > +#include "rzg2l-cru-regs.h"
> >
> >  #define RZG2L_TIMEOUT_MS             100
> >  #define RZG2L_RETRIES                        10
>
> --
> Regards,
>
> Laurent Pinchart