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