+/*standard module options for ALSA. This module supports only one
card*/
+int hdmi_card_index = SNDRV_DEFAULT_IDX1; +char *hdmi_card_id = SNDRV_DEFAULT_STR1;
+module_param(hdmi_card_index, int, 0444); +MODULE_PARM_DESC(hdmi_card_index,
"Index value for INTEL Intel HDMI Audio controller.");+module_param(hdmi_card_id, charp, 0444); +MODULE_PARM_DESC(hdmi_card_id,
"ID string for INTEL Intel HDMI Audio controller.");Drivers should expose the standard module options "index" and "id" like others. Also they should be static.
Agree.
+/* hardware capability structure */ +static const struct snd_pcm_hardware snd_intel_hadstream = {
- .info = (SNDRV_PCM_INFO_INTERLEAVED |
SNDRV_PCM_INFO_DOUBLE |SNDRV_PCM_INFO_PAUSE |SNDRV_PCM_INFO_RESUME |You have to implement corresponding stuff if you mark the driver as RESUME-able.
Yes we have implemented SNDRV_PCM_TRIGGER_PAUSE_RELEASE
SNDRV_PCM_INFO_MMAP|SNDRV_PCM_INFO_MMAP_VALID |SNDRV_PCM_INFO_BATCH |SNDRV_PCM_INFO_SYNC_START),Is sync stuff implemented?
It's not implemented, we will remove it.
- pr_debug("had: snd_intelhad_open called\n");
- intelhaddata = snd_pcm_substream_chip(substream);
So... you assign the global variable at this open callback? Any race?
It's not required, since its global variable. We will remove this.
- stream = kzalloc(sizeof(*stream), GFP_KERNEL);
- if (!stream)
return -ENOMEM;- stream->stream_status = STREAM_INIT;
- runtime->private_data = stream;
- intelhaddata->reg_ops->hdmi_audio_write_register(AUD_HDMI_STATUS,
0);
- retval = snd_pcm_hw_constraint_integer(runtime,
SNDRV_PCM_HW_PARAM_PERIODS);stream is leaked when the error returned here.
We will free the stream in error path.
- switch (cmd) {
- case SNDRV_PCM_TRIGGER_START:
pr_debug("had: Trigger Start\n");caps = HDMI_AUDIO_UNDERRUN | HDMI_AUDIO_BUFFER_DONE;retval = query_ops->hdmi_audio_set_caps(HAD_SET_ENABLE_AUDIO_INT, &caps);if (retval)return retval;retval = query_ops->hdmi_audio_set_caps(HAD_SET_ENABLE_AUDIO, NULL);if (retval)return retval;Don't you need to reset audio_caps at error?
ENABLE_AUDIO_INT and ENABLE_AUDIO just sets register bits and no impact even if it fails.
- intelhaddata->playback_cnt++;
- intelhaddata->stream_info.str_id = intelhaddata->playback_cnt;
Note that the prepare callback might be called multiple times before triggering. This counter doesn't work. If any, it should be counted in the open/close callback.
We will move the counters to _open.
+/*PCM operations structure and the calls back for the same */ +struct snd_pcm_ops snd_intelhad_playback_ops = {
Does it need to be global?
We will make it static.
Any reason to allocate extra two *_ops instead of just put these flat in the structure itself? I mean, struct intelhda { ... struct hdmi_audio_registers_ops reg_ops; struct hdmi_audio_query_set_ops query_ops; ... }
then you don't need to malloc them.
We will declare them as flat structures.
Overall I feel uneasy about the handling of global variables. They should be cleaned up more...
We will clean up the global variables.
Thanks, Sailaja and Ramesh.