Re: [PATCH 07/12] video/hdmi: Introduce helpers for the HDMI vendor specific infoframe

Thierry Reding
2013-08-15T14:59:04Z

[-- Attachment #1.1: Type: text/plain, Size: 1746 bytes --]

On Wed, Aug 14, 2013 at 06:19:10PM +0100, Damien Lespiau wrote:
[...]
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index ac84215..59c4748 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -286,6 +286,94 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
> EXPORT_SYMBOL(hdmi_audio_infoframe_pack);
>
> /**
> + * hdmi_hdmi_infoframe_init() - initialize an HDMI vendor infoframe
> + * @frame: HDMI vendor infoframe
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int hdmi_hdmi_infoframe_init(struct hdmi_hdmi_infoframe *frame)

The hdmi_hdmi_ prefix is weird. Can't we come up with a better prefix?
You refer to it as "HDMI vendor infoframe" in the comments, yet we
already have struct hdmi_vendor_infoframe. Perhaps hdmi_3d_infoframe or
hdmi_vendor_3d_infoframe would be better choices?

> +{
> + memset(frame, 0, sizeof(*frame));
> +
> + frame->type = HDMI_INFOFRAME_TYPE_VENDOR;
> + frame->version = 1;
> +
> + /* 0 is a valid value for s3d_struct, so we use a special "not set"
> + * value */

Nit: The block comment style is inconsistent again.

> +/**
> + * hdmi_hdmi_infoframe_pack() - write a HDMI vendor infoframe to binary buffer
> + * @frame: HDMI infoframe
> + * @buffer: destination buffer
> + * @size: size of buffer
> + *
> + * Packs the information contained in the @frame structure into a binary
> + * representation that can be written into the corresponding controller
> + * registers. Also computes the checksum as required by section 5.3.5 of
> + * the HDMI 1.4 specification.

I need to dig up that version of the specification. This infoframe
doesn't seem to exist in 1.3.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]