Re: [PATCH v3 3/9] video: Add generic HDMI infoframe helpers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 18, 2013 at 09:56:36AM +0100, Thierry Reding wrote:
> On Wed, Jan 16, 2013 at 03:27:14PM +0200, Ville Syrjälä wrote:
> > On Mon, Jan 14, 2013 at 03:30:22PM +0100, Thierry Reding wrote:
> > > Add generic helpers to pack HDMI infoframes into binary buffers.
> [...]
> > > +/**
> > > + * hdmi_avi_infoframe_init() - initialize an HDMI AVI infoframe
> > > + * @frame: HDMI AVI infoframe
> > > + *
> > > + * Returns 0 on success or a negative error code on failure.
> > > + */
> > > +int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame)
> > > +{
> > > +	if (!frame)
> > > +		return -EINVAL;
> > 
> > There's quite a bit of error checking all around which seems a bit
> > pointless since this is all internal stuff. Any errors are bugs in
> > some driver code, so just letting the thing oops would be fine in my
> > opinion.
> 
> Okay, I can remove some of the more obvious checks. Checking for space
> requirements might still be useful, so I'll leave that in.
> 
> > > +ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, void *buffer,
> > > +				size_t size)
> [...]
> > > +	for (i = 0; i < sizeof(frame->vendor); i++)
> > > +		ptr[i] = frame->vendor[i];
> > > +
> > > +	for (i = 0; i < sizeof(frame->product); i++)
> > > +		ptr[8 + i] = frame->product[i];
> > 
> > memcpy()
> 
> Done.
> 
> > > +enum hdmi_spd_sdi {
> > > +	HDMI_SPD_SDI_UNKNOWN,
> > > +	HDMI_SPD_SDI_DSTB,
> > > +	HDMI_SPD_SDI_DVDP,
> > > +	HDMI_SPD_SDI_DVHS,
> > > +	HDMI_SPD_SDI_HDDVR,
> > > +	HDMI_SPD_SDI_DVC,
> > > +	HDMI_SPD_SDI_DSC,
> > > +	HDMI_SPD_SDI_VCD,
> > > +	HDMI_SPD_SDI_GAME,
> > > +	HDMI_SPD_SDI_PC,
> > > +	HDMI_SPD_SDI_BD,
> > > +	HDMI_SPD_SDI_SCD,
> > 
> > I believe SACD is a more correct name.
> 
> Done.
> 
> > HD DVD and PMP are missing from this list.
> 
> I can't find any reference to those in CEA-861-D. HDMI 1.4 doesn't have
> them either. Can you provide any pointers?

CEA-861-E has them.

> 
> > > +enum hdmi_audio_coding_type {
> > > +	HDMI_AUDIO_CODING_TYPE_STREAM,
> > > +	HDMI_AUDIO_CODING_TYPE_IEC_60958,
> > > +	HDMI_AUDIO_CODING_TYPE_AC3,
> > > +	HDMI_AUDIO_CODING_TYPE_MPEG1,
> > > +	HDMI_AUDIO_CODING_TYPE_MP3,
> > > +	HDMI_AUDIO_CODING_TYPE_MPEG2,
> > > +	HDMI_AUDIO_CODING_TYPE_AAC,
> > > +	HDMI_AUDIO_CODING_TYPE_DTS,
> > > +	HDMI_AUDIO_CODING_TYPE_ATRAC,
> > > +	HDMI_AUDIO_CODING_TYPE_ONE_BIT_AUDIO,
> > > +	HDMI_AUDIO_CODING_TYPE_DOLBY_DIGITAL_PLUS,
> > > +	HDMI_AUDIO_CODING_TYPE_DTS_HD,
> > > +	HDMI_AUDIO_CODING_TYPE_MAT_MLP,
> > > +	HDMI_AUDIO_CODING_TYPE_DST,
> > > +	HDMI_AUDIO_CODING_TYPE_WMPRO,
> > 
> > These don't always quite match the names in CEA-861-E. Wouldn't it
> > be better to use matching names?
> 
> Are HD-DVD and PMP above listed in CEA-861-E? I'll need to find a copy
> of it and will go over this again and match the names to those in the
> document.

Great.

> > Also the audio coding extension type is missing.
> 
> I assume this is also part of CEA-861-E as I can't find a reference in
> -D?

Yeah, I only checked against CEA-861-E.

-- 
Ville Syrjälä
Intel OTC