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@xxxxxxxxxxxxxxx>
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
