Re: [PATCH] Intel HDMI audio support

[Takashi Iwai <tiwai@suse.de>]

At Tue, 4 Nov 2008 17:38:39 +0800,
Wu Fengguang wrote:
> 
> Add support for Intel G45 integrated HDMI audio codecs.
> 
> This initial release supports:
> 	- 2 channel stereo sound output
> 	- report monitor's ELD information
> 
> Signed-off-by: Wu Fengguang <wfg@linux.intel.com>

Thanks.  The patch looks almost fine, but a few things to be fixed.

> +/*
> + * ELD: EDID Like Data
> + */
> +struct sink_eld {
> +	int	eld_size;
> +	int	baseline_len;
> +	int	eld_ver;	/* (eld_ver == 0) indicates invalid ELD */
> +	int	cea_edid_ver;
> +	char	monitor_name[ELD_MAX_MNL + 1];
> +	int	manufacture_id;
> +	int	product_id;
> +	long	port_id;

If you need a 64bit value, use u64 explicitly.

> +struct hdmi_audio_infoframe {
> +	u8 type; /* 0x84 */
> +	u8 ver;  /* 0x01 */
> +	u8 len;  /* 0x0a */
> +
> +	u8 checksum;	/* PB0 */
> +	u8 CC02_CT47;	/* CC in bits 0:2, CT in 4:7 */
> +	u8 SS01_SF24;
> +	u8 CXT04;
> +	u8 CA;
> +	u8 LFEPBL01_LSV36_DM_INH7;
> +	u8 reserved[5];	/* PB6  - PB10 */
> +};

> +#define GRAB_BITS(buf, byte, lowbit, bits, val) \
> +	(val) = ((buf)[(byte)] >> (lowbit)) & ((1 << (bits)) - 1)

This should be rather a form

#define GRAB_BITS(buf, byte, lowbit, bits) \
	((buf)[(byte)] >> (lowbit)) & ((1 << (bits)) - 1)
and

foo = GRAB_BITS(buf, byte, low, bits);


> +static void hdmi_update_short_audio_desc(struct cea_sad *a,
> +					 const unsigned char *buf)
> +{
> +	int i;
> +	int val;
> +
> +	GRAB_BITS(buf, 1, 0, 7, val);

The numbers is GRAB_BITS() could be better defined.

> +static int hdmi_update_sink_eld(struct hda_codec *codec,
> +				const unsigned char *buf, int size)
(snip)
> +	e->port_id	  = le64_to_cpu(*(u64 *)(buf + 8));

Use get_unaligned_*() for this, just to be more portable.
(And include <asm/unaligned.h> beforehand.)

> +	/* the spec's tendency is little endian */
> +	e->manufacture_id = le16_to_cpu(*(u16 *)(buf + 16));
> +	e->product_id	  = le16_to_cpu(*(u16 *)(buf + 18));

Ditto.

> +
> +	if (mnl > ELD_MAX_MNL) {
> +		snd_printd(KERN_INFO "MNL is reserved value %d\n", mnl);
> +		goto out_fail;
> +	} else if (ELD_FIXED_BYTES + mnl > size) {
> +		snd_printd(KERN_INFO "out of range MNL %d\n", mnl);
> +		goto out_fail;
> +	} else {
> +		strncpy(e->monitor_name, buf + ELD_FIXED_BYTES, mnl);
> +		e->monitor_name[mnl] = '\0';

Use strlcpy().

> +static void hdmi_show_eld(struct hda_codec *codec)
> +{
> +	int i;
> +	int j;
> +	struct intel_hdmi_spec *spec = codec->spec;
> +	struct sink_eld *e = &spec->sink;
> +	char buf[80];
(snip)
> +	for (i = 0; i < ARRAY_SIZE(cea_speaker_allocation_names); i++) {
> +		if (e->spk_alloc & (1 << i))
> +			j += sprintf(buf + j, " %s",
> +					cea_speaker_allocation_names[i]);

Use snprintf() instead to avoid overflow.

> +	}
> +	buf[j] = '\0';
> +	printk(KERN_INFO "speaker allocations: (0x%x)%s\n", e->spk_alloc, buf);
> +
> +	for (i = 0; i < e->sad_count; i++) {
> +		hdmi_show_short_audio_desc(e->sad + i);
> +	}

checkpatch.pl warns about these braces.
In general, it's worth to run once before submission.

Could you fix these and repost?


thanks,

Takashi