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

[Anand, Jerome]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Wednesday, January 18, 2017 5:33 PM
> To: Anand, Jerome <jerome.anand at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; alsa-devel at alsa-project.org;
> ville.syrjala at linux.intel.com; broonie at kernel.org; pierre-
> louis.bossart at linux.intel.com; Ughreja, Rakesh A
> <rakesh.a.ughreja at 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 at canonical.com> and is accomodated in
> > this patch
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart at linux.intel.com>
> > Signed-off-by: Jerome Anand <jerome.anand at 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