-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, January 18, 2017 5:33 PM To: Anand, Jerome jerome.anand@intel.com Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; ville.syrjala@linux.intel.com; broonie@kernel.org; pierre- louis.bossart@linux.intel.com; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [PATCH V3 4/5] ALSA: x86: hdmi: Add audio support for BYT and CHT
On Tue, 10 Jan 2017 06:08:40 +0100, Jerome Anand wrote:
Hdmi audio driver based on the child platform device created by gfx driver is implemented. This audio driver is derived from legacy intel hdmi audio driver.
The interfaces for interaction between gfx and audio are updated and the driver implementation updated to derive interrupts in its own address space based on irq chip framework
The changes to calculate sub-period positions was triggered by David Henningsson david.henningsson@canonical.com and is accomodated in this patch
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Jerome Anand jerome.anand@intel.com
Again some knitpicks:
+/* Register access functions */
+inline int had_get_hwstate(struct snd_intelhad *intelhaddata)
Don't use external inline. Drop inline if this is no static. Ditto for the rest.
Ok, will remove inline
+/**
- had_read_modify_aud_config_v2 - Specific function to read-modify
No need for kernel doc comments for such local functions.
Ok, will remove the comments
- AUD_CONFIG register on VLV2.The had_read_modify() function should
+not
- directly be used on VLV2 for updating AUD_CONFIG register.
- This is because:
- Bit6 of AUD_CONFIG register is writeonly due to a silicon bug on
+VLV2
- HDMI IP. As a result a read-modify of AUD_CONFIG regiter will
+always
- clear bit6. AUD_CONFIG[6:4] represents the "channels" field of the
- register. This field should be 1xy binary for configuration with 6
+or
- more channels. Read-modify of AUD_CONFIG (Eg. for enabling audio)
- causes the "channels" field to be updated as 0xy binary resulting
+in
- bad audio. The fix is to always write the AUD_CONFIG[6:4] with
- appropriate value when doing read-modify of AUD_CONFIG register.
- @substream: the current substream or NULL if no active substream
- @data : data to be written
- @mask : mask
- */
+inline int had_read_modify_aud_config_v2(struct snd_pcm_substream
*substream,
uint32_t data, uint32_t mask)Is this really external? Or can it be static? (Ditto for some others *_v1 and *_v2.)
Will make it static
+/*
- ** ALSA API channel-map control callbacks **/
Avoid such a confusing comment style.
Ok, will change the comment
+static int had_chmap_ctl_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *uinfo)+{
- struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
- struct snd_intelhad *intelhaddata = info->private_data;
- if (intelhaddata->drv_status == HAD_DRV_DISCONNECTED)
return -ENODEV;- uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
- uinfo->count = HAD_MAX_CHANNEL;
- uinfo->value.integer.min = 0;
- uinfo->value.integer.max = SNDRV_CHMAP_LAST;
- return 0;
+}
+#ifndef USE_ALSA_DEFAULT_TLV +static int had_chmap_ctl_tlv(struct snd_kcontrol *kcontrol, int op_flag,
unsigned int size, unsigned int __user *tlv) {- struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
- struct snd_intelhad *intelhaddata = info->private_data;
- if (intelhaddata->drv_status == HAD_DRV_DISCONNECTED)
return -ENODEV;- /* TODO: Fix for query channel map */
- return -EPERM;
+} +#endif
For what reason is this dummy implementation provided?
This was taken from legacy to support chmap setting from above layers. I'll remove this as well
+static int had_chmap_ctl_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol) {- struct snd_pcm_chmap *info = snd_kcontrol_chip(kcontrol);
- struct snd_intelhad *intelhaddata = info->private_data;
- int i = 0;
- const struct snd_pcm_chmap_elem *chmap;
- if (intelhaddata->drv_status == HAD_DRV_DISCONNECTED)
return -ENODEV;- if (intelhaddata->chmap->chmap == NULL)
return -ENODATA;- chmap = intelhaddata->chmap->chmap;
- for (i = 0; i < chmap->channels; i++) {
ucontrol->value.integer.value[i] = chmap->map[i];pr_debug("chmap->map[%d] = %d\n", i, chmap->map[i]);- }
- return 0;
+}
+static int had_chmap_ctl_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol) {- /* TODO: Get channel map and set swap register */
- return -EPERM;
+}
The standard chmap doesn't support write unless explicitly changed. So you can drop the callback as well.
Ok, will remove this.
thanks,
Takashi