Re: [PATCH v1 2/9] drm: add drm specific media-bus-format header file

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

Hi Sam,

Thank you for the patch.

On Sun, Feb 06, 2022 at 04:43:58PM +0100, Sam Ravnborg wrote:
> For now the header file includes a single method to retreive the bpc

s/retreive/retrieve/

> from the bus format.
> The supported MEDIA_BUS_* codes are the ones used for the current panels
> in DRM. The list can be extended as there are a need for more formats.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 include/drm/media-bus-format.h
> 
> diff --git a/include/drm/media-bus-format.h b/include/drm/media-bus-format.h
> new file mode 100644
> index 000000000000..d4d18f19a70f
> --- /dev/null
> +++ b/include/drm/media-bus-format.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Sam Ravnborg
> + */
> +
> +#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT
> +#define __LINUX_DRM_MEDIA_BUS_FORMAT
> +
> +#include <linux/bug.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/types.h>
> +
> +/**
> + * media_bus_format_to_bpc - The bits per color channel for the bus_format
> + *
> + * Based on the supplied bus_format return the maximum number of bits
> + * per color channel.
> + *
> + * RETURNS
> + * The number of bits per color channel, or -EINVAL if the bus_format
> + * is unknown.
> + */
> +static inline int media_bus_format_to_bpc(u32 bus_format)
> +{
> +	switch (bus_format) {
> +	/* DPI */
> +	case MEDIA_BUS_FMT_RGB565_1X16:
> +	case MEDIA_BUS_FMT_RGB666_1X18:
> +		return 6;
> +
> +	/* DPI */
> +	case MEDIA_BUS_FMT_RGB888_1X24:
> +	case MEDIA_BUS_FMT_RGB888_3X8:
> +	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +		return 8;
> +
> +     	/* LVDS */

Wrong indentation.

> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> +		return 6;
> +
> +     	/* LVDS */
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> +		return 8;
> +
> +	default:
> +		WARN(1, "Unknown MEDIA_BUS format %d\n", bus_format);
> +		return -EINVAL;
> +	}
> +}

This seems a bit big for an inline function.

If we add more helper functions, it will result in lots of switch...case
statements. Could we use the same approach as in drm_fourcc.h with a
format info structure ?

> +
> +#endif

-- 
Regards,

Laurent Pinchart