[alsa-devel] [PATCH 1/5] [RFC]intel_hdmi_audio:include header

[Mark Brown]

On Mon, Nov 22, 2010 at 07:09:42PM +0530, ramesh.babu at intel.com wrote:

> +#define AUD_SAMPLE_RATE_32	HAD_MIN_RATE
> +#define AUD_SAMPLE_RATE_44_1	44100
> +#define AUD_SAMPLE_RATE_48	48000
> +#define AUD_SAMPLE_RATE_88_2	88200
> +#define AUD_SAMPLE_RATE_96	96000
> +#define AUD_SAMPLE_RATE_176_4	176400
> +#define AUD_SAMPLE_RATE_192	HAD_MAX_RATE

The defines for the individual rates don't seem to serve any purpose
here - they're just defined to the sample rate as an integer, and the
one place they're used looks to be a switch statement where I found
myself wondering why either standard ALSA defines or the raw numbers
weren't being used.

> +#define DRIVER_NAME		"intelmid_hdmi_audio"
> +#define DIS_SAMPLE_RATE_25_2	25200
> +#define DIS_SAMPLE_RATE_27	27000
> +#define DIS_SAMPLE_RATE_54	54000
> +#define DIS_SAMPLE_RATE_74_25	74250
> +#define DIS_SAMPLE_RATE_148_5	148500

Ditto here.

> +#define REG_BIT_0		0x1
> +#define REG_BIT_1		0x2
> +#define REG_BIT_2		0x4
> +#define REG_BIT_3		0x8
> +#define REG_BIT_20		0x100000

There's a standard BIT() macro IIRC.

> +#define SET_BIT0		1
> +#define CLEAR_BIT0		0

These macros don't do anything like what one would expect...

> +/**
> + * enum intel_had_aud_buf_type - HDMI controller ring buffer types
> + *
> + * @HAD_BUF_TYPE_A: ring buffer A
> + * @HAD_BUF_TYPE_B: ring buffer B
> + * @HAD_BUF_TYPE_C: ring buffer C
> + * @HAD_BUF_TYPE_D: ring buffer D
> + */
> +enum intel_had_aud_buf_type {
> +	HAD_BUF_TYPE_A = 0,
> +	HAD_BUF_TYPE_B = 1,
> +	HAD_BUF_TYPE_C = 2,
> +	HAD_BUF_TYPE_D = 3,
> +};

These could use a little more explanation.

> +extern struct snd_intelhad *intelhaddata;
> +extern struct snd_pcm_ops snd_intelhad_playback_ops;
> +extern int hdmi_card_index;
> +extern char *hdmi_card_id;

These global variables look very suspicous - they look like they should
be local to some file?