Re: [PATCH 1/6] media: Define MIPI CSI-2 data types in a shared header file

From: Pratyush Yadav <p.yadav@ti.com>
To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	"Sakari Ailus" <sakari.ailus@iki.fi>,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Jacopo Mondi" <jacopo@jmondi.org>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Dafna Hirschfeld" <dafna.hirschfeld@collabora.com>
Subject: Re: [PATCH 1/6] media: Define MIPI CSI-2 data types in a shared header file
Date: Mon, 24 Jan 2022 20:52:24 +0530	[thread overview]
Message-ID: <20220124152224.o46gxdxp3polw2eu@ti.com> (raw)
In-Reply-To: <20220123160857.24161-2-laurent.pinchart+renesas@ideasonboard.com>

Hi Laurent,

Thanks for the patch.

On 23/01/22 06:08PM, Laurent Pinchart wrote:
> There are many CSI-2-related drivers in the media subsystem that come
> with their own macros to handle the CSI-2 data types (or just hardcode
> the numerical values). Provide a shared header with definitions for
> those data types that driver can use.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  include/media/mipi-csi2.h | 45 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 include/media/mipi-csi2.h
> 
> diff --git a/include/media/mipi-csi2.h b/include/media/mipi-csi2.h
> new file mode 100644
> index 000000000000..392794e5badd
> --- /dev/null
> +++ b/include/media/mipi-csi2.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * MIPI CSI-2 Data Types
> + *
> + * Copyright (C) 2022 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +#ifndef _MEDIA_MIPI_CSI2_H
> +#define _MEDIA_MIPI_CSI2_H
> +
> +/* Short packet data types */
> +#define MIPI_CSI2_DT_FS			0x00
> +#define MIPI_CSI2_DT_FE			0x01
> +#define MIPI_CSI2_DT_LS			0x02
> +#define MIPI_CSI2_DT_LE			0x03
> +#define MIPI_CSI2_DT_GENERIC_SHORT(n)	(0x08 + (n))	/* 0..7 */

IIUC there is currently no way to actually capture packets with these 
data types, and these are added here for completeness's sake, right?

> +
> +/* Long packet data types */
> +#define MIPI_CSI2_DT_NULL		0x10
> +#define MIPI_CSI2_DT_BLANKING		0x11
> +#define MIPI_CSI2_DT_EMBEDDED_8B	0x12
> +#define MIPI_CSI2_DT_YUV420_8B		0x18
> +#define MIPI_CSI2_DT_YUV420_10B		0x19
> +#define MIPI_CSI2_DT_YUV420_8B_LEGACY	0x1a
> +#define MIPI_CSI2_DT_YUV420_8B_CS	0x1c
> +#define MIPI_CSI2_DT_YUV420_10B_CS	0x1d
> +#define MIPI_CSI2_DT_YUV422_8B		0x1e
> +#define MIPI_CSI2_DT_YUV422_10B		0x1f
> +#define MIPI_CSI2_DT_RGB444		0x20
> +#define MIPI_CSI2_DT_RGB555		0x21
> +#define MIPI_CSI2_DT_RGB565		0x22
> +#define MIPI_CSI2_DT_RGB666		0x23
> +#define MIPI_CSI2_DT_RGB888		0x24
> +#define MIPI_CSI2_DT_RAW24		0x27

I have the CSI-2 spec v1.3, and it lists 0x27 as reserved under RGB 
Image data, and I don't see a data type value for RAW24. Where did you 
get this value from?

> +#define MIPI_CSI2_DT_RAW6		0x28
> +#define MIPI_CSI2_DT_RAW7		0x29
> +#define MIPI_CSI2_DT_RAW8		0x2a
> +#define MIPI_CSI2_DT_RAW10		0x2b
> +#define MIPI_CSI2_DT_RAW12		0x2c
> +#define MIPI_CSI2_DT_RAW14		0x2d
> +#define MIPI_CSI2_DT_RAW16		0x2e
> +#define MIPI_CSI2_DT_RAW20		0x2f

These two are also listed as reserved in the spec I have. Rest of the 
values look good to me.

> +#define MIPI_CSI2_DT_USER_DEFINED(n)	(0x30 + (n))	/* 0..7 */
> +
> +#endif /* _MEDIA_MIPI_CSI2_H */
> -- 
> Regards,
> 
> Laurent Pinchart
> 

I think this patch is a good idea in general, and it should remove a lot 
of repetition in the drivers.

BTW, I also see lots of drivers adding tables having mapping between 
MBUS formats, FOURCC formats, bpp, data type, etc. It would be useful to 
have those in a central place IMO.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.