Re: [PATCH v5 1/1] media: v4l2-common: Add helper function media_bus_fmt_to_csi2_(bpp|dt)()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media_bus_fmt_to_csi2_dt() incorrectly returns 0 (MIPI_CSI2_DT_FS) instead of MIPI_CSI2_DT_INVALID for formats without a mapped CSI-2 data type.
- [Low] Mismatch between kernel-doc comment and macro definition for the invalid data type indicator.
--

commit dd2f0971f75dadcf691d8ed964c49386a1fc2170
Author: Frank Li <Frank.Li@nxp.com>

media: v4l2-common: Add helper function media_bus_fmt_to_csi2_(bpp|dt)()

This commit adds helper functions media_bus_fmt_to_csi2_bpp() and
media_bus_fmt_to_csi2_dt() to convert media bus formats to MIPI CSI-2 bit
depths and data types. This aims to reduce duplicate static mapping arrays
across different CSI-2 drivers.

> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index bceafc4e92c81..6c8afb96e60fe 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c

[ ... ]

> +static const struct media_bus_fmt_info media_bus_fmt_info[] = {
> +	{ .code = MEDIA_BUS_FMT_RGB444_1X12, .bpp = 12 },
> +	{ .code = MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE, .bpp = 16 },

[ ... ]

> +u32 media_bus_fmt_to_csi2_dt(u32 bus_fmt)
> +{
> +	const struct media_bus_fmt_info *info = media_bus_fmt_info_get(bus_fmt);
> +
> +	if (!info)
> +		return MIPI_CSI2_DT_INVALID;
> +
> +	/* Check bpp because 0 (MIPI_CSI2_DT_FS) is a valid data type code */
> +	return info->bpp ? info->dt : MIPI_CSI2_DT_INVALID;
> +}

Since uninitialized fields in a struct array are zero-initialized in C, any
entry in media_bus_fmt_info without an explicit .dt value will have .dt = 0.

Because every entry in the array defines a non-zero .bpp, info->bpp is always
true for matched formats. Doesn't this mean the function will return 0 for
unmapped formats rather than MIPI_CSI2_DT_INVALID?

If 0 is returned, this maps to MIPI_CSI2_DT_FS (Frame Start). Could this
cause drivers to silently program the camera hardware with a short-packet
Frame Start data type for pixel data instead of rejecting the unsupported
format?

> diff --git a/include/media/mipi-csi2.h b/include/media/mipi-csi2.h
> index 40fc0264250d7..bb5054f0cf310 100644
> --- a/include/media/mipi-csi2.h
> +++ b/include/media/mipi-csi2.h

[ ... ]

> +/* Use one undefined value in spec */
> +#define MIPI_CSI2_DT_INVALID		0xff
> +
> +/**
> + * media_bus_fmt_to_csi2_dt - Get MIPI CSI2 data type from media bus format
> + *
> + * @bus_fmt: media bus format identifier (MEDIA_BUS_FMT_*)
> + *
> + * Return: MIPI CSI2 data type MIPI_CSI2_DT_*, MIPI_CSI2_DT_INVALIDATE means
> + * can't get data type from bus_fmt.
> + */

This isn't a bug, but there is a typo in the documentation. The macro defined
above is MIPI_CSI2_DT_INVALID, but the kernel-doc references
MIPI_CSI2_DT_INVALIDATE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514183357.3088564-1-Frank.Li@nxp.com?part=1