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

[PATCH v2 00/16] 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.

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 (16):
  media: platform: rzg2l-cru: rzg2l-ip: Use the RZG2L_CRU_IP_SINK/SOURCE
    enum entries
  media: platform: rzg2l-cru: Mark sink pads with MUST_CONNECT flag
  media: platform: rzg2l-cru: rzg2l-csi2: Mark sink and source pad with
    MUST_CONNECT flag
  media: platform: rzg2l-cru: rzg2l-csi2: Make use of
    NR_OF_RZG2L_CSI2_PAD
  media: platform: rzg2l-cru: rzg2l-csi2: Implement .get_frame_desc()
  media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel
    information
  media: platform: rzg2l-cru: Remove `channel` member from `struct
    rzg2l_cru_csi`
  media: platform: rzg2l-cru: rzg2l-video: Use MIPI CSI-2 data types for
    ICnMC_INF definitions
  media: platform: rzg2l-cru: Remove unused fields from
    rzg2l_cru_ip_format struct
  media: platform: rzg2l-cru: Simplify handling of supported formats
  media: platform: rzg2l-cru: rzg2l-ip: Use `rzg2l_cru_ip_formats` array
    in enum_frame_size callback
  media: platform: rzg2l-cru: rzg2l-csi2: Remove unused datatype field
    from rzg2l_csi2_format
  media: platform: rzg2l-cru: rzg2l-video: Use
    rzg2l_cru_ip_code_to_fmt() to validate format
  media: platform: rzg2l-cru: rzg2l-csi2: Make use of rzg2l_csi2_formats
    array in rzg2l_csi2_enum_frame_size()
  media: renesas: rzg2l-cru: Refactor ICnDMR register configuration
  media: platform: rzg2l-cru: Add support to capture 8bit raw sRGB

 .../platform/renesas/rzg2l-cru/rzg2l-core.c   |   3 +-
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  31 +++-
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   |  39 ++++-
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     |  91 +++++++++---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 133 ++++++++++--------
 5 files changed, 207 insertions(+), 90 deletions(-)

-- 
2.34.1

[PATCH v2 01/16] media: platform: rzg2l-cru: rzg2l-ip: Use the 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>
---
 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.34.1

Re: [PATCH v2 01/16] media: platform: rzg2l-cru: rzg2l-ip: Use the RZG2L_CRU_IP_SINK/SOURCE enum entries

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:42PM +0100, Prabhakar wrote:
> 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>

> ---
>  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)

-- 
Regards,

Laurent Pinchart

[PATCH v2 02/16] media: platform: rzg2l-cru: Mark sink pads with MUST_CONNECT flag

[Prabhakar]

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

Mark the sink pads 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>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c | 2 +-
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 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..db48118fc817 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -217,7 +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[RZG2L_CRU_IP_SINK].flags = MEDIA_PAD_FL_SINK;
+	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;
 
 	ret = media_entity_pads_init(&ip->subdev.entity, 2, ip->pads);
-- 
2.34.1

Re: [PATCH v2 02/16] media: platform: rzg2l-cru: Mark sink pads with MUST_CONNECT flag

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:43PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Mark the sink pads 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>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c | 2 +-
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c   | 3 ++-
>  2 files changed, 3 insertions(+), 2 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;

The corresponding link is ENABLED and IMMUTABLE, so this will not have
any effect. It shouldn't cause any issue though, and makes things more
explicit.

>  	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..db48118fc817 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -217,7 +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[RZG2L_CRU_IP_SINK].flags = MEDIA_PAD_FL_SINK;
> +	ip->pads[RZG2L_CRU_IP_SINK].flags = MEDIA_PAD_FL_SINK |
> +					    MEDIA_PAD_FL_MUST_CONNECT;

Same here. For this pad, the MUST_CONNECT flag will come handy when
adding support for the parallel input, as the link will not be immutable
anymore.

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

>  	ip->pads[RZG2L_CRU_IP_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>  
>  	ret = media_entity_pads_init(&ip->subdev.entity, 2, ip->pads);

-- 
Regards,

Laurent Pinchart

[PATCH v2 03/16] media: platform: rzg2l-cru: rzg2l-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>
---
 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.34.1

Re: [PATCH v2 03/16] media: platform: rzg2l-cru: rzg2l-csi2: Mark sink and source pad with MUST_CONNECT flag

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:44PM +0100, Prabhakar wrote:
> 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.

The MUST_CONNECT flag only affects sink pads. That's not documented
though, and it seems that most drivers using the flag sets it on both
sink and source pads. That's probably a good practice, and the fact that
the flag is only checked for sink pads is more of an implementation
detail. This patch is thus fine.

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

However, I think you should then set the flag on the source pad of the
IP entity in patch 02/16. You can keep my Rb.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/16] media: platform: rzg2l-cru: rzg2l-csi2: Mark sink and source pad with MUST_CONNECT flag

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Fri, Sep 27, 2024 at 11:24 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:53:44PM +0100, Prabhakar wrote:
> > 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.
>
> The MUST_CONNECT flag only affects sink pads. That's not documented
> though, and it seems that most drivers using the flag sets it on both
> sink and source pads. That's probably a good practice, and the fact that
> the flag is only checked for sink pads is more of an implementation
> detail. This patch is thus fine.
>
Agreed.

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> However, I think you should then set the flag on the source pad of the
> IP entity in patch 02/16. You can keep my Rb.
>
Sure, I'll add this flag in v3.

Cheers,
Prabhakar

[PATCH v2 04/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of NR_OF_RZG2L_CSI2_PAD

[Prabhakar]

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

Make use of NR_OF_RZG2L_CSI2_PAD enum entry in media_entity_pads_init()
instead of magic number.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 2 +-
 1 file changed, 1 insertion(+), 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..10b8b0c87c1f 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -804,7 +804,7 @@ 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, NR_OF_RZG2L_CSI2_PAD, csi2->pads);
 	if (ret)
 		goto error_pm;
 
-- 
2.34.1

Re: [PATCH v2 04/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of NR_OF_RZG2L_CSI2_PAD

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:45PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Make use of NR_OF_RZG2L_CSI2_PAD enum entry in media_entity_pads_init()
> instead of magic number.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 2 +-
>  1 file changed, 1 insertion(+), 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..10b8b0c87c1f 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -804,7 +804,7 @@ 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, NR_OF_RZG2L_CSI2_PAD, csi2->pads);

Better, I would use ARRAY_SIZE

	ret = media_entity_pads_init(&csi2->subdev.entity, ARRAY_SIZE(csi2->pads),
				     csi2->pads);

With this (and an updated commit message),

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

>  	if (ret)
>  		goto error_pm;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of NR_OF_RZG2L_CSI2_PAD

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Fri, Sep 27, 2024 at 11:26 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:53:45PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Make use of NR_OF_RZG2L_CSI2_PAD enum entry in media_entity_pads_init()
> > instead of magic number.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 2 +-
> >  1 file changed, 1 insertion(+), 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..10b8b0c87c1f 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -804,7 +804,7 @@ 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, NR_OF_RZG2L_CSI2_PAD, csi2->pads);
>
> Better, I would use ARRAY_SIZE
>
>         ret = media_entity_pads_init(&csi2->subdev.entity, ARRAY_SIZE(csi2->pads),
>                                      csi2->pads);
>
> With this (and an updated commit message),
>
Ok, I will update the code to above suggestion and update the commit
description.

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
And I will add the RB tag.

Cheers,
Prabhakar

[PATCH v2 05/16] media: platform: rzg2l-cru: rzg2l-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>
---
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index 10b8b0c87c1f..26953499f305 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -582,6 +582,24 @@ 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 +611,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.34.1

[PATCH v2 06/16] media: platform: rzg2l-cru: rzg2l-video: 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>
---
 .../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 db48118fc817..75c3c4f1d10b 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.34.1

[PATCH v2 07/16] media: platform: 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>
---
 .../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.34.1

Re: [PATCH v2 07/16] media: platform: rzg2l-cru: Remove `channel` member from `struct rzg2l_cru_csi`

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:48PM +0100, Prabhakar wrote:
> 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>

> ---
>  .../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;

-- 
Regards,

Laurent Pinchart

[PATCH v2 08/16] media: platform: rzg2l-cru: rzg2l-video: 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>
---
 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.34.1

Re: [PATCH v2 08/16] media: platform: rzg2l-cru: rzg2l-video: Use MIPI CSI-2 data types for ICnMC_INF definitions

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:49PM +0100, Prabhakar wrote:
> 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>

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

-- 
Regards,

Laurent Pinchart

[PATCH v2 09/16] media: platform: 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>
---
 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 75c3c4f1d10b..cc297e137f3e 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.34.1

Re: [PATCH v2 09/16] media: platform: rzg2l-cru: Remove unused fields from rzg2l_cru_ip_format struct

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:50PM +0100, Prabhakar wrote:
> 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>

> ---
>  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 75c3c4f1d10b..cc297e137f3e 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)

-- 
Regards,

Laurent Pinchart

[PATCH v2 10/16] media: platform: 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_pix_fmt_to_bpp()`, and
  `rzg2l_cru_ip_index_to_pix_fmt()` to streamline format lookups.
- Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
  to utilize the new helpers.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 +++++++--
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 71 +++++++------------
 3 files changed, 72 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..24097df14881 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);
+u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
+int rzg2l_cru_ip_index_to_pix_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 cc297e137f3e..2d3b985b7b0d 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;
 }
 
+u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
+		if (rzg2l_cru_ip_formats[i].format == format)
+			return rzg2l_cru_ip_formats[i].bpp;
+
+	return 0;
+}
+
+int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
+{
+	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
+		return -EINVAL;
+
+	return rzg2l_cru_ip_formats[index].format;
+}
+
 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..014c0ff2721b 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,23 @@ 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);
+	if (!cru_ip_fmt)
+		return -EINVAL;
+
+	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 +342,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 +805,16 @@ 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;
+	u8 bpp;
 
-	fmt = rzg2l_cru_format_from_pixel(pix->pixelformat);
+	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
 
-	if (WARN_ON(!fmt))
-		return -EINVAL;
+	if (WARN_ON(!bpp))
+		return 0;
 
-	return pix->width * fmt->bpp[0];
+	return pix->width * bpp;
 }
 
 static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
@@ -849,7 +825,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_pix_fmt_to_bpp(pix->pixelformat))
 		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
 
 	switch (pix->field) {
@@ -941,10 +917,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))
+	int ret;
+
+	ret = rzg2l_cru_ip_index_to_pix_fmt(f->index);
+	if (ret < 0)
 		return -EINVAL;
 
-	f->pixelformat = rzg2l_cru_formats[f->index].format;
+	f->pixelformat = ret;
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2 10/16] media: platform: rzg2l-cru: Simplify handling of supported formats

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:51PM +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_pix_fmt_to_bpp()`, and
>   `rzg2l_cru_ip_index_to_pix_fmt()` to streamline format lookups.
> - Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
>   to utilize the new helpers.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 +++++++--
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 71 +++++++------------
>  3 files changed, 72 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..24097df14881 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);
> +u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
> +int rzg2l_cru_ip_index_to_pix_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 cc297e137f3e..2d3b985b7b0d 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;
>  }
>  
> +u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
> +		if (rzg2l_cru_ip_formats[i].format == format)
> +			return rzg2l_cru_ip_formats[i].bpp;
> +
> +	return 0;
> +}

Instead of making this a ad-hoc 4cc -> bpp conversion, I would rename
the function to rzg2l_cru_ip_format_to_fmt() (or something similar) and
return a const struct rzg2l_cru_ip_format *. The caller can use the .bpp
field.

> +
> +int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
> +{
> +	if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> +		return -EINVAL;
> +
> +	return rzg2l_cru_ip_formats[index].format;

There's no guarantee the 4CC won't map to a negative 32-bit integer. I
would return a onst struct rzg2l_cru_ip_format * from this function, and
rename it accordingly. The call can then use the .format field.

> +}
> +
>  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..014c0ff2721b 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,23 @@ 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);
> +	if (!cru_ip_fmt)
> +		return -EINVAL;

I think you can drop this check, as the code is guaranteed to be valid.

> +
> +	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 +342,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 +805,16 @@ 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;
> +	u8 bpp;
>  
> -	fmt = rzg2l_cru_format_from_pixel(pix->pixelformat);
> +	bpp = rzg2l_cru_ip_pix_fmt_to_bpp(pix->pixelformat);
>  
> -	if (WARN_ON(!fmt))
> -		return -EINVAL;
> +	if (WARN_ON(!bpp))
> +		return 0;
>  
> -	return pix->width * fmt->bpp[0];
> +	return pix->width * bpp;
>  }
>  
>  static u32 rzg2l_cru_format_sizeimage(struct v4l2_pix_format *pix)
> @@ -849,7 +825,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_pix_fmt_to_bpp(pix->pixelformat))
>  		pix->pixelformat = RZG2L_CRU_DEFAULT_FORMAT;
>  
>  	switch (pix->field) {
> @@ -941,10 +917,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))
> +	int ret;
> +
> +	ret = rzg2l_cru_ip_index_to_pix_fmt(f->index);
> +	if (ret < 0)
>  		return -EINVAL;
>  
> -	f->pixelformat = rzg2l_cru_formats[f->index].format;
> +	f->pixelformat = ret;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 10/16] media: platform: rzg2l-cru: Simplify handling of supported formats

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Fri, Sep 27, 2024 at 11:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:53:51PM +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_pix_fmt_to_bpp()`, and
> >   `rzg2l_cru_ip_index_to_pix_fmt()` to streamline format lookups.
> > - Refactored the `rzg2l_cru_csi2_setup` and format alignment functions
> >   to utilize the new helpers.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    | 20 +++++-
> >  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     | 35 +++++++--
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 71 +++++++------------
> >  3 files changed, 72 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..24097df14881 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);
> > +u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
> > +int rzg2l_cru_ip_index_to_pix_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 cc297e137f3e..2d3b985b7b0d 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;
> >  }
> >
> > +u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format)
> > +{
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(rzg2l_cru_ip_formats); i++)
> > +             if (rzg2l_cru_ip_formats[i].format == format)
> > +                     return rzg2l_cru_ip_formats[i].bpp;
> > +
> > +     return 0;
> > +}
>
> Instead of making this a ad-hoc 4cc -> bpp conversion, I would rename
> the function to rzg2l_cru_ip_format_to_fmt() (or something similar) and
> return a const struct rzg2l_cru_ip_format *. The caller can use the .bpp
> field.
>
OK, I'll introduce rzg2l_cru_ip_format_to_fmt().

> > +
> > +int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
> > +{
> > +     if (index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
> > +             return -EINVAL;
> > +
> > +     return rzg2l_cru_ip_formats[index].format;
>
> There's no guarantee the 4CC won't map to a negative 32-bit integer. I
> would return a onst struct rzg2l_cru_ip_format * from this function, and
> rename it accordingly. The call can then use the .format field.
>
OK, I'll introduce rzg2l_cru_ip_index_to_fmt().

> > +}
> > +
> >  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..014c0ff2721b 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,23 @@ 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);
> > +     if (!cru_ip_fmt)
> > +             return -EINVAL;
>
> I think you can drop this check, as the code is guaranteed to be valid.
>
Agreed, I will drop this check.

Cheers,
Prabhakar

[PATCH v2 11/16] media: platform: rzg2l-cru: rzg2l-ip: Use `rzg2l_cru_ip_formats` array in enum_frame_size callback

[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 index range and format code.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 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 2d3b985b7b0d..c7bc82bf3f14 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -162,10 +162,10 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd,
 					struct v4l2_subdev_state *state,
 					struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->index != 0)
+	if (fse->index >= ARRAY_SIZE(rzg2l_cru_ip_formats))
 		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.34.1

Re: [PATCH v2 11/16] media: platform: rzg2l-cru: rzg2l-ip: Use `rzg2l_cru_ip_formats` array in enum_frame_size callback

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:52PM +0100, Prabhakar wrote:
> 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 index range and format code.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  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 2d3b985b7b0d..c7bc82bf3f14 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -162,10 +162,10 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd,
>  					struct v4l2_subdev_state *state,
>  					struct v4l2_subdev_frame_size_enum *fse)
>  {
> -	if (fse->index != 0)
> +	if (fse->index >= ARRAY_SIZE(rzg2l_cru_ip_formats))

This isn't right, the index is related to frame sizes, not frame
formats. You should keep the code as-is.

>  		return -EINVAL;
>  
> -	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
> +	if (!rzg2l_cru_ip_code_to_fmt(fse->code))
>  		return -EINVAL;

This is good.

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

>  
>  	fse->min_width = RZG2L_CRU_MIN_INPUT_WIDTH;

-- 
Regards,

Laurent Pinchart

[PATCH v2 12/16] media: platform: rzg2l-cru: rzg2l-csi2: Remove unused datatype 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>
---
 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 26953499f305..79d99d865c1f 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.34.1

Re: [PATCH v2 12/16] media: platform: rzg2l-cru: rzg2l-csi2: Remove unused datatype field from rzg2l_csi2_format

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:53PM +0100, Prabhakar wrote:
> 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>

> ---
>  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 26953499f305..79d99d865c1f 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)

-- 
Regards,

Laurent Pinchart

[PATCH v2 13/16] media: platform: rzg2l-cru: rzg2l-video: Use rzg2l_cru_ip_code_to_fmt() to validate format

[Prabhakar]

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

Utilize `rzg2l_cru_ip_code_to_fmt()` in `rzg2l_cru_mc_validate_format()`
to validate whether the format is supported. This change removes the need
to manually add new entries when a new format is added to the CRU driver,
simplifying the validation process.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 014c0ff2721b..c32608c557a3 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -201,12 +201,8 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
 	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:
+	if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
 		return -EPIPE;
-	}
 
 	switch (fmt.format.field) {
 	case V4L2_FIELD_TOP:
-- 
2.34.1

Re: [PATCH v2 13/16] media: platform: rzg2l-cru: rzg2l-video: Use rzg2l_cru_ip_code_to_fmt() to validate format

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:54PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Utilize `rzg2l_cru_ip_code_to_fmt()` in `rzg2l_cru_mc_validate_format()`
> to validate whether the format is supported. This change removes the need
> to manually add new entries when a new format is added to the CRU driver,
> simplifying the validation process.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 014c0ff2721b..c32608c557a3 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -201,12 +201,8 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
>  	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:
> +	if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
>  		return -EPIPE;
> -	}

This looks fine, but I think you should take it one step further and
perform format validation in .link_validate(). See
https://lore.kernel.org/all/20240826124106.3823-8-laurent.pinchart+renesas@ideasonboard.com/

>  
>  	switch (fmt.format.field) {
>  	case V4L2_FIELD_TOP:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 13/16] media: platform: rzg2l-cru: rzg2l-video: Use rzg2l_cru_ip_code_to_fmt() to validate format

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sat, Sep 28, 2024 at 12:09 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:53:54PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Utilize `rzg2l_cru_ip_code_to_fmt()` in `rzg2l_cru_mc_validate_format()`
> > to validate whether the format is supported. This change removes the need
> > to manually add new entries when a new format is added to the CRU driver,
> > simplifying the validation process.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index 014c0ff2721b..c32608c557a3 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -201,12 +201,8 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
> >       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:
> > +     if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
> >               return -EPIPE;
> > -     }
>
> This looks fine, but I think you should take it one step further and
> perform format validation in .link_validate(). See
> https://lore.kernel.org/all/20240826124106.3823-8-laurent.pinchart+renesas@ideasonboard.com/
>
OK, I'll implement link_validate() and do format checking in there and
get rid of rzg2l_cru_mc_validate_format(). Thanks for the pointer.

Cheers,
Prabhakar

[PATCH v2 14/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of rzg2l_csi2_formats array in rzg2l_csi2_enum_frame_size()

[Prabhakar]

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

Make use `rzg2l_csi2_formats` array in rzg2l_csi2_enum_frame_size().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 5 ++++-
 1 file changed, 4 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 79d99d865c1f..e630283dd1f1 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -570,7 +570,10 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
 				      struct v4l2_subdev_state *sd_state,
 				      struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->index != 0)
+	if (fse->index >= ARRAY_SIZE(rzg2l_csi2_formats))
+		return -EINVAL;
+
+	if (!rzg2l_csi2_code_to_fmt(fse->code))
 		return -EINVAL;
 
 	fse->min_width = RZG2L_CSI2_MIN_WIDTH;
-- 
2.34.1

Re: [PATCH v2 14/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of rzg2l_csi2_formats array in rzg2l_csi2_enum_frame_size()

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

I've just noticed that the subject line of most of your patches is much
longer than the 72 characters limit. Please try to shorten them. You can
replace the prefixes with "media: rzg2l-cru:", and reword the subject
lines that mention long function names.

On Tue, Sep 10, 2024 at 06:53:55PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Make use `rzg2l_csi2_formats` array in rzg2l_csi2_enum_frame_size().
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 5 ++++-
>  1 file changed, 4 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 79d99d865c1f..e630283dd1f1 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -570,7 +570,10 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_state *sd_state,
>  				      struct v4l2_subdev_frame_size_enum *fse)
>  {
> -	if (fse->index != 0)
> +	if (fse->index >= ARRAY_SIZE(rzg2l_csi2_formats))
> +		return -EINVAL;

Same comment as in 11/16. With this fixed,

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

> +
> +	if (!rzg2l_csi2_code_to_fmt(fse->code))
>  		return -EINVAL;
>  
>  	fse->min_width = RZG2L_CSI2_MIN_WIDTH;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 14/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of rzg2l_csi2_formats array in rzg2l_csi2_enum_frame_size()

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sat, Sep 28, 2024 at 12:11 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> I've just noticed that the subject line of most of your patches is much
> longer than the 72 characters limit. Please try to shorten them. You can
> replace the prefixes with "media: rzg2l-cru:", and reword the subject
> lines that mention long function names.
>
Ok, I'll rework the subject line so that it fits within 72 characters.

> On Tue, Sep 10, 2024 at 06:53:55PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Make use `rzg2l_csi2_formats` array in rzg2l_csi2_enum_frame_size().
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 5 ++++-
> >  1 file changed, 4 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 79d99d865c1f..e630283dd1f1 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -570,7 +570,10 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
> >                                     struct v4l2_subdev_state *sd_state,
> >                                     struct v4l2_subdev_frame_size_enum *fse)
> >  {
> > -     if (fse->index != 0)
> > +     if (fse->index >= ARRAY_SIZE(rzg2l_csi2_formats))
> > +             return -EINVAL;
>
> Same comment as in 11/16. With this fixed,
>
Ok, I'll drop this check.

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

Cheers,
Prabhakar

Re: [PATCH v2 14/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of rzg2l_csi2_formats array in rzg2l_csi2_enum_frame_size()

[Laurent Pinchart]

On Mon, Sep 30, 2024 at 01:19:25PM +0100, Lad, Prabhakar wrote:
> Hi Laurent,
> 
> Thank you for the review.
> 
> On Sat, Sep 28, 2024 at 12:11 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > I've just noticed that the subject line of most of your patches is much
> > longer than the 72 characters limit. Please try to shorten them. You can
> > replace the prefixes with "media: rzg2l-cru:", and reword the subject
> > lines that mention long function names.
> >
> Ok, I'll rework the subject line so that it fits within 72 characters.
> 
> > On Tue, Sep 10, 2024 at 06:53:55PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Make use `rzg2l_csi2_formats` array in rzg2l_csi2_enum_frame_size().
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 5 ++++-
> > >  1 file changed, 4 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 79d99d865c1f..e630283dd1f1 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > @@ -570,7 +570,10 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
> > >                                     struct v4l2_subdev_state *sd_state,
> > >                                     struct v4l2_subdev_frame_size_enum *fse)
> > >  {
> > > -     if (fse->index != 0)
> > > +     if (fse->index >= ARRAY_SIZE(rzg2l_csi2_formats))
> > > +             return -EINVAL;
> >
> > Same comment as in 11/16. With this fixed,
> >
> Ok, I'll drop this check.

Don't drop the check, drop the change. if (fse->index != 0) is the
right check (testing > 0 works too).

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 14/16] media: platform: rzg2l-cru: rzg2l-csi2: Make use of rzg2l_csi2_formats array in rzg2l_csi2_enum_frame_size()

[Lad, Prabhakar]

Hi Laurent,

On Mon, Sep 30, 2024 at 1:52 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Sep 30, 2024 at 01:19:25PM +0100, Lad, Prabhakar wrote:
> > Hi Laurent,
> >
> > Thank you for the review.
> >
> > On Sat, Sep 28, 2024 at 12:11 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thank you for the patch.
> > >
> > > I've just noticed that the subject line of most of your patches is much
> > > longer than the 72 characters limit. Please try to shorten them. You can
> > > replace the prefixes with "media: rzg2l-cru:", and reword the subject
> > > lines that mention long function names.
> > >
> > Ok, I'll rework the subject line so that it fits within 72 characters.
> >
> > > On Tue, Sep 10, 2024 at 06:53:55PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Make use `rzg2l_csi2_formats` array in rzg2l_csi2_enum_frame_size().
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c | 5 ++++-
> > > >  1 file changed, 4 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 79d99d865c1f..e630283dd1f1 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > > > @@ -570,7 +570,10 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
> > > >                                     struct v4l2_subdev_state *sd_state,
> > > >                                     struct v4l2_subdev_frame_size_enum *fse)
> > > >  {
> > > > -     if (fse->index != 0)
> > > > +     if (fse->index >= ARRAY_SIZE(rzg2l_csi2_formats))
> > > > +             return -EINVAL;
> > >
> > > Same comment as in 11/16. With this fixed,
> > >
> > Ok, I'll drop this check.
>
> Don't drop the check, drop the change. if (fse->index != 0) is the
> right check (testing > 0 works too).
>
Ahh sorry for not being clear, I meant I will drop the updated check.

Cheers,
Prabhakar

[PATCH v2 15/16] media: renesas: 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. This change introduces a new function
`rzg2l_cru_ip_pix_fmt_to_icndmr()` to map the pixel format to its
corresponding ICnDMR value.

Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h |  5 +++++
 drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c  | 12 ++++++++++++
 .../media/platform/renesas/rzg2l-cru/rzg2l-video.c   | 10 ++++------
 3 files changed, 21 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 24097df14881..3da9e8e7025a 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;
 };
 
 /**
@@ -165,5 +169,6 @@ struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
 const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code);
 u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
 int rzg2l_cru_ip_index_to_pix_fmt(u32 index);
+int rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
 
 #endif
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index c7bc82bf3f14..9b0563198b50 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,
 	},
 };
 
@@ -50,6 +51,17 @@ int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
 	return rzg2l_cru_ip_formats[index].format;
 }
 
+int rzg2l_cru_ip_pix_fmt_to_icndmr(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].icndmr;
+
+	return -EINVAL;
+}
+
 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 c32608c557a3..1f25dcff2515 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
@@ -316,6 +315,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
 	const struct v4l2_format_info *src_finfo, *dst_finfo;
 	const struct rzg2l_cru_ip_format *cru_ip_fmt;
 	u32 icndmr;
+	int ret;
 
 	cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
 	if (!cru_ip_fmt)
@@ -327,15 +327,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:
+	ret = rzg2l_cru_ip_pix_fmt_to_icndmr(cru->format.pixelformat);
+	if (ret < 0) {
 		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
 			cru->format.pixelformat);
 		return -EINVAL;
 	}
+	icndmr = ret;
 
 	/* If input and output use same colorspace, do bypass mode */
 	if (v4l2_is_format_yuv(src_finfo) && v4l2_is_format_yuv(dst_finfo))
-- 
2.34.1

Re: [PATCH v2 15/16] media: renesas: rzg2l-cru: Refactor ICnDMR register configuration

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:56PM +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. This change introduces a new function
> `rzg2l_cru_ip_pix_fmt_to_icndmr()` to map the pixel format to its
> corresponding ICnDMR value.

Skip this new function, use the function thar returns a
rzg2l_cru_ip_format pointer, and access the icndmr field from there.
rzg2l_cru_initialize_image_conv() already gets the format info pointer,
so the code will be simpler and more efficient.

> 
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h |  5 +++++
>  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c  | 12 ++++++++++++
>  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c   | 10 ++++------
>  3 files changed, 21 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 24097df14881..3da9e8e7025a 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)

Not a candidate for this patch, but I would recommend moving all the
register definitions to this file, or to a rzg2l-cru-regs.h file.

> +
>  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;
>  };
>  
>  /**
> @@ -165,5 +169,6 @@ struct v4l2_mbus_framefmt *rzg2l_cru_ip_get_src_fmt(struct rzg2l_cru_dev *cru);
>  const struct rzg2l_cru_ip_format *rzg2l_cru_ip_code_to_fmt(unsigned int code);
>  u8 rzg2l_cru_ip_pix_fmt_to_bpp(u32 format);
>  int rzg2l_cru_ip_index_to_pix_fmt(u32 index);
> +int rzg2l_cru_ip_pix_fmt_to_icndmr(u32 format);
>  
>  #endif
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index c7bc82bf3f14..9b0563198b50 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,
>  	},
>  };
>  
> @@ -50,6 +51,17 @@ int rzg2l_cru_ip_index_to_pix_fmt(u32 index)
>  	return rzg2l_cru_ip_formats[index].format;
>  }
>  
> +int rzg2l_cru_ip_pix_fmt_to_icndmr(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].icndmr;
> +
> +	return -EINVAL;
> +}
> +
>  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 c32608c557a3..1f25dcff2515 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
> @@ -316,6 +315,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  	const struct v4l2_format_info *src_finfo, *dst_finfo;
>  	const struct rzg2l_cru_ip_format *cru_ip_fmt;
>  	u32 icndmr;
> +	int ret;
>  
>  	cru_ip_fmt = rzg2l_cru_ip_code_to_fmt(ip_sd_fmt->code);
>  	if (!cru_ip_fmt)
> @@ -327,15 +327,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:
> +	ret = rzg2l_cru_ip_pix_fmt_to_icndmr(cru->format.pixelformat);
> +	if (ret < 0) {
>  		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
>  			cru->format.pixelformat);
>  		return -EINVAL;
>  	}
> +	icndmr = ret;
>  
>  	/* 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 v2 15/16] media: renesas: rzg2l-cru: Refactor ICnDMR register configuration

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sat, Sep 28, 2024 at 12:21 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Sep 10, 2024 at 06:53:56PM +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. This change introduces a new function
> > `rzg2l_cru_ip_pix_fmt_to_icndmr()` to map the pixel format to its
> > corresponding ICnDMR value.
>
> Skip this new function, use the function thar returns a
> rzg2l_cru_ip_format pointer, and access the icndmr field from there.
> rzg2l_cru_initialize_image_conv() already gets the format info pointer,
> so the code will be simpler and more efficient.
>
Agreed.

> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h |  5 +++++
> >  drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c  | 12 ++++++++++++
> >  .../media/platform/renesas/rzg2l-cru/rzg2l-video.c   | 10 ++++------
> >  3 files changed, 21 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 24097df14881..3da9e8e7025a 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)
>
> Not a candidate for this patch, but I would recommend moving all the
> register definitions to this file, or to a rzg2l-cru-regs.h file.
>
OK, I'll create a new patch to move all reg defs to rzg2l-cru-regs.h file.

Cheers,
Prabhakar

[PATCH v2 16/16] media: platform: 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>
---
 .../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 e630283dd1f1..d46f0bd10cec 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 9b0563198b50..9bb192655f25 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.34.1

Re: [PATCH v2 16/16] media: platform: rzg2l-cru: Add support to capture 8bit raw sRGB

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Tue, Sep 10, 2024 at 06:53:57PM +0100, Prabhakar wrote:
> 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>

> ---
>  .../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 e630283dd1f1..d46f0bd10cec 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 9b0563198b50..9bb192655f25 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)

-- 
Regards,

Laurent Pinchart