At Thu, 13 Nov 2008 10:21:53 +0800, Wu Fengguang wrote:
Create hda_eld.c for ELD routines and proc interface.
Thanks for the patch!
ELD handling routines can be shared by all HDMI codecs, and they are large enough to make a standalone source file.
Some notes on the code:
- Shall we show an empty file if the ELD content is not valid? Well it's not that simple. There could be partially populated ELD, and there may be malformed ELD provided by buggy drivers/monitors. So I tend to expose ELD as it is.
Fair enough.
- hdmi_show_eld() and hdmi_print_eld_info() are somehow duplicated The plan is to shrink hdmi_show_eld()/hdmi_show_short_audio_desc(), and to show a compact message at monitor hotplug and driver initialization time.
You can create a common print function to accept the callback, such as,
static void hdmi_print(void (*print)(void *, const char *line), void *data, const char *fmt, ...) { char line[80]; va_list ap; va_start(fp, fmt); vsnprintf(line, sizeof(line), fmt, ap); va_end(ap); print(data, line); }
static void hdmi_print_eld(void (*print)(void *, const char *), void *data, struct sink_eld *e) { hdmi_print(print, data, "monitor name\t\t%s\n", e->monitor_name); ... } static void print_proc(void *data, const char *line) { snd_iprintf(data, "%s", line); }
static void hdmi_print_eld_info(struct snd_info_entry *entry, struct snd_info_buffer *buffer) { hdmi_print_eld(print_proc, buffer, entry->private_data); }
static void print_console(void *data, const char *line) { printk(KERN_DEBUG "%s", line); }
void snd_hdmi_show_eld(struct sink_eld *e) { hdmi_print_eld(print_console, NULL, e); }
Just an idea, though.
The ELD retrieval routines rely on the Intel HDA interface, others are/could be universal and independent ones.
The ELD proc interface code could be moved to hda_proc.c if necessary.
- How do we name the proc file? If there are going to be two HDMI pins per codec, then the current naming scheme (eld#<codec no>) will fail.
In theory, yes, but I don't think this would happen. If this is needed, the currently existing codec#* proc must be fixed, too. So, we can use eld#codec as the simplest way.
Some review comments below:
--- sound-2.6.orig/sound/pci/hda/patch_intelhdmi.c +++ sound-2.6/sound/pci/hda/patch_intelhdmi.c
...
@@ -880,6 +410,11 @@ static int intel_hdmi_build_controls(str
static int intel_hdmi_init(struct hda_codec *codec) {
- struct intel_hdmi_spec *spec = codec->spec;
- struct sink_eld *eld = &spec->sink;
- snd_hda_eld_proc_new(codec, eld);
The proc creation should be rather in patch_intelhdmi(). The init callback is called also at suspend/resume usually.
/* disable audio output as early as possible */ hdmi_disable_output(codec);
@@ -927,3 +462,4 @@ struct hda_codec_preset snd_hda_preset_i { .id = 0x10951392, .name = "SiI1392 HDMI", .patch = patch_intel_hdmi }, {} /* terminator */ };
Useless space addition.
--- /dev/null +++ sound-2.6/sound/pci/hda/hda_eld.c +static inline unsigned char grab_bits(const unsigned char *buf,
int byte, int lowbit, int bits)+{
- BUG_ON(lowbit > 7);
- BUG_ON(bits > 8);
- BUG_ON(bits <= 0);
Can it be rather BUILD_BUG_ON(), BTW? Or, hmm, doesn't work if it's an inline function?
+int hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid)
I prefer snd_ prefix for global functions to avoid any potential name crashes.
+int hdmi_get_eld(struct sink_eld *eld,
struct hda_codec *codec, hda_nid_t nid)
Ditto.
+void hdmi_show_eld(struct sink_eld *e)
Ditto.
--- sound-2.6.orig/sound/pci/hda/Makefile +++ sound-2.6/sound/pci/hda/Makefile @@ -4,6 +4,7 @@ snd-hda-intel-y := hda_intel.o # designed to be individual modules snd-hda-intel-y += hda_codec.o snd-hda-intel-$(CONFIG_PROC_FS) += hda_proc.o +snd-hda-intel-$(CONFIG_SND_HDA_ELD) += hda_eld.o snd-hda-intel-$(CONFIG_SND_HDA_HWDEP) += hda_hwdep.o snd-hda-intel-$(CONFIG_SND_HDA_INPUT_BEEP) += hda_beep.o snd-hda-intel-$(CONFIG_SND_HDA_GENERIC) += hda_generic.o --- sound-2.6.orig/sound/pci/hda/hda_local.h +++ sound-2.6/sound/pci/hda/hda_local.h @@ -276,15 +276,6 @@ static inline int snd_hda_parse_generic_ #endif
/*
- generic proc interface
- */
-#ifdef CONFIG_PROC_FS -int snd_hda_codec_proc_new(struct hda_codec *codec); -#else -static inline int snd_hda_codec_proc_new(struct hda_codec *codec) { return 0; } -#endif
...
+/*
- generic proc interface
- */
+#ifdef CONFIG_PROC_FS +int snd_hda_codec_proc_new(struct hda_codec *codec); +int snd_hda_eld_proc_new(struct hda_codec *codec, struct sink_eld *eld);
I'd put these two declarations rather separately, since snd_hda_proc_new() has nothing to do with ELD. Keep it in the original place.
thanks,
Takashi