Re: [PATCH V3 4/5] ALSA: x86: hdmi: Add audio support for BYT and CHT

[Takashi Iwai <tiwai@suse.de>]

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.

> +/**
> + * 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx