[alsa-devel] [PATCHv3 2/2] ALSA: hda - Add driver for Tegra SoC HDA

[Takashi Iwai]

At Wed, 16 Apr 2014 14:39:32 -0700,
Dylan Reid wrote:
> 
> This adds a driver for the HDA block in Tegra SoCs.  The HDA bus is
> used to communicate with the HDMI codec on Tegra124.
> 
> Most of the code is re-used from the Intel/PCI HDA driver.  It brings
> over only the power_save module parameter.
> 
> Signed-off-by: Dylan Reid <dgreid at chromium.org>

Thanks, this looks better, but a few comments below.
Besides, I'd like to have a review in DT part.


> +/*
> + * Register access ops. Tegra HDA register access is DWORD only.
> + */
> +static void hda_tegra_writel(u32 value, u32 *addr)
> +{
> +	writel(value, addr);
> +}
> +
> +static u32 hda_tegra_readl(u32 *addr)
> +{
> +	return readl(addr);
> +}
> +
> +static void hda_tegra_writew(u16 value, u16 *addr)
> +{
> +	unsigned long shift = ((uintptr_t)(addr) & 0x3) << 3;
> +	void *dword_addr = (void *)((uintptr_t)(addr) & ~0x3);
> +	u32 v;
> +
> +	v = readl(dword_addr);
> +	v &= ~(0xffff << shift);
> +	v |= value << shift;
> +	writel(v, dword_addr);
> +}

The shift doesn't have to be long at all, and the cast in kernel is
usually just unsigned long instead of uintptr_t.


> +#ifdef CONFIG_PM_SLEEP
> +/*
> + * power management
> + */
> +static int hda_tegra_suspend(struct device *dev)
> +{
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct azx *chip = card->private_data;
> +	struct azx_pcm *p;
> +	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
> +
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> +	list_for_each_entry(p, &chip->pcm_list, list)
> +		snd_pcm_suspend_all(p->pcm);
> +	if (chip->initialized)
> +		snd_hda_suspend(chip->bus);
> +
> +	/* enable controller wake up event */
> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> +		  STATESTS_INT_MASK);

I guess WAKEEN doesn't work for Tegra?

> +
> +	azx_stop_chip(chip);
> +	azx_enter_link_reset(chip);
> +	hda_tegra_disable_clocks(hda);
> +
> +	return 0;
> +}
> +
> +static int hda_tegra_resume(struct device *dev)
> +{
> +	struct snd_card *card = dev_get_drvdata(dev);
> +	struct azx *chip = card->private_data;
> +	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
> +	int status;
> +	struct hda_codec *codec;
> +
> +	hda_tegra_enable_clocks(hda);
> +
> +	/* Read STATESTS before controller reset */
> +	status = azx_readw(chip, STATESTS);
> +
> +	hda_tegra_init(hda);
> +
> +	if (status && chip->bus) {
> +		list_for_each_entry(codec, &chip->bus->codec_list, list)
> +			if (status & (1 << codec->addr))
> +				queue_delayed_work(codec->bus->workq,
> +						   &codec->jackpoll_work,
> +						   codec->jackpoll_interval);
> +	}

This is superfluous for the regular resume.  It was some missing call
only in the runtime resume.

> +
> +	/* disable controller Wake Up event*/
> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) & ~STATESTS_INT_MASK);

This can be removed like the suspend.


thanks,

Takashi