Re: [PATCH v2 04/32] media: v4l2-common: Add helper function media_bus_fmt_to_csi2_dt()

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

Hi Frank,

Thank you for the patch.

On Fri, Aug 08, 2025 at 06:39:07PM -0400, Frank Li wrote:
> CSI2 data type is defined by MIPI Camera Serial Interface 2 Spec Ver4.1.
> See section 9.4.
> 
> Add helper function media_bus_fmt_to_csi2_dt() to convert media bus fmt to
> MIPI defined data type and avoid below duplicated static array in each CSI2
> drivers.
> 
> 	{
> 		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> 		.data_type = MIPI_CSI2_DT_YUV422_8B,
> 	}
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 44 +++++++++++++++++++++++++++++++++++
>  include/media/mipi-csi2.h             |  3 +++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 3a48b6a55c6e322696b910dd519def4f0b4a58fb..fcc01030beb347499da2a3c8539793d20f6f512c 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -701,6 +701,50 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap);
>  
> +int media_bus_fmt_to_csi2_dt(int bus_fmt)
> +{
> +	switch (bus_fmt) {
> +	case MEDIA_BUS_FMT_UYVY8_1X16:
> +		return MIPI_CSI2_DT_YUV422_8B;
> +	case MEDIA_BUS_FMT_RGB565_1X16:
> +		return MIPI_CSI2_DT_RGB565;
> +	case MEDIA_BUS_FMT_BGR888_1X24:
> +		return MIPI_CSI2_DT_RGB888;
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +		return MIPI_CSI2_DT_RAW8;
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_Y10_1X10:
> +		return MIPI_CSI2_DT_RAW10;
> +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +	case MEDIA_BUS_FMT_Y12_1X12:
> +		return MIPI_CSI2_DT_RAW12;
> +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> +		return MIPI_CSI2_DT_RAW14;
> +	case MEDIA_BUS_FMT_SBGGR16_1X16:
> +	case MEDIA_BUS_FMT_SGBRG16_1X16:
> +	case MEDIA_BUS_FMT_SGRBG16_1X16:
> +	case MEDIA_BUS_FMT_SRGGB16_1X16:
> +		return MIPI_CSI2_DT_RAW16;
> +
> +	default:
> +		return MIPI_CSI2_DT_NULL;

I would use 0 here. MIPI_CSI2_DT_NULL is equal to 0x10, and I'm
concerned about driver comparing the return value of the function to 0.

> +	}
> +}
> +EXPORT_SYMBOL_GPL(media_bus_fmt_to_csi2_dt);
> +

We have something similar for pixel formats, with the v4l2_format_info
structure and the v4l2_format_info() lookup function. Adding information
about media bus formats to the V4L2 core seems to be a good idea.

Regarding the implementation, I think you should mimick what we do for
the pixel formats, and add the data to an info structure with a lookup
function.

>  int media_bus_fmt_to_csi2_bpp(int bus_fmt)
>  {
>  	switch (bus_fmt) {
> diff --git a/include/media/mipi-csi2.h b/include/media/mipi-csi2.h
> index c5b4e8e1ca93803568b1eee85f7f99c3a45a0b6e..35de536b9b65b49ad9e2914437d26d0e4240cf38 100644
> --- a/include/media/mipi-csi2.h
> +++ b/include/media/mipi-csi2.h
> @@ -8,6 +8,8 @@
>  #ifndef _MEDIA_MIPI_CSI2_H
>  #define _MEDIA_MIPI_CSI2_H
>  
> +/* DT value ref to MIPI Camera Serial Interface 2 Spec Ver4.1 section 9.4 */
> +
>  /* Short packet data types */
>  #define MIPI_CSI2_DT_FS			0x00
>  #define MIPI_CSI2_DT_FE			0x01
> @@ -44,6 +46,7 @@
>  #define MIPI_CSI2_DT_RAW20		0x2f
>  #define MIPI_CSI2_DT_USER_DEFINED(n)	(0x30 + (n))	/* 0..7 */
>  
> +int media_bus_fmt_to_csi2_dt(int buf_fmt);
>  int media_bus_fmt_to_csi2_bpp(int bus_fmt);
>  
>  #endif /* _MEDIA_MIPI_CSI2_H */
> 
> 

-- 
Regards,

Laurent Pinchart