> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Wednesday, January 18, 2017 5:33 PM > To: Anand, Jerome <jerome.anand@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; > ville.syrjala@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; pierre- > louis.bossart@xxxxxxxxxxxxxxx; Ughreja, Rakesh A > <rakesh.a.ughreja@xxxxxxxxx> > 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@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. > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
