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@xxxxxxxxxxxxx> and is
> accomodated in this patch
>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx>
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.
> +/**
> + * had_read_modify_aud_config_v2 - Specific function to read-modify
No need for kernel doc comments for such local functions.
> + * 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.)
> +/*
> + ** ALSA API channel-map control callbacks
> + **/
Avoid such a confusing comment style.
> +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?
> +
> +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.
thanks,
Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
