[alsa-devel] [RFC][PATCH] ELD routines and proc interface

[Takashi Iwai]

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