Re: [PATCH v2 1/3] ASoC: tegra: add ac97 host driver

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 01/04/2013 06:18 PM, Lucas Stach wrote:
> This adds the driver for the Tegra 2x AC97 host controller.
> 
> Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>

> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.txt

> +- nvidia,codec-sync-gpio : The Tegra GPIO controller's phandle and the number
> +  of the GPIO corresponding with the AC97 DAP _FS line
> +Example:

If you have to repost for any reason, you probably want a blank line
before the "Example:" line.

> diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c

> +static void tegra20_ac97_codec_warm_reset(struct snd_ac97 *ac97)
> +{
> +	u32 readback;
> +	unsigned long timeout;
> +
> +	/*
> +	 * although sync line is driven by the DAC pad group warm reset using
> +	 * the controller cmd is not working, have to toggle sync line
> +	 * manually.
> +	 */
> +	gpio_request(workdata->sync_gpio, "codec-sync");
> +
> +	gpio_direction_output(workdata->sync_gpio, 1);

Would it make sense to just request the GPIO during probe()? Or, is the
sync signal used as part of the AC'97 protocol at other times?

> +static inline void tegra20_ac97_start_playback(struct tegra20_ac97 *ac97)
> +{
> +	regmap_update_bits(ac97->regmap, TEGRA20_AC97_FIFO1_SCR,
> +			   TEGRA20_AC97_FIFO_SCR_PB_QRT_MT_EN,
> +			   TEGRA20_AC97_FIFO_SCR_PB_QRT_MT_EN);
> +
> +	regmap_update_bits(ac97->regmap, TEGRA20_AC97_CTRL,
> +			   TEGRA20_AC97_CTRL_PCM_DAC_EN |
> +			   TEGRA20_AC97_CTRL_STM_EN,
> +			   TEGRA20_AC97_CTRL_PCM_DAC_EN |
> +			   TEGRA20_AC97_CTRL_STM_EN);
> +}

That sets both PCM_DAC_EN and STM_EN, but ...

> +static inline void tegra20_ac97_stop_playback(struct tegra20_ac97 *ac97)
> +{
> +	regmap_update_bits(ac97->regmap, TEGRA20_AC97_FIFO1_SCR,
> +			   TEGRA20_AC97_FIFO_SCR_PB_QRT_MT_EN, 0);
> +
> +	regmap_update_bits(ac97->regmap, TEGRA20_AC97_CTRL,
> +			   TEGRA20_AC97_CTRL_PCM_DAC_EN, 0);
> +}

... that only clears DAC_EN. Should it clear STM_EN too?