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

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

On 01/08/2013 03:22 PM, Lucas Stach wrote:
> Am Dienstag, den 08.01.2013, 15:10 -0700 schrieb Stephen Warren:
>> On 01/04/2013 06:18 PM, Lucas Stach wrote:
>>> This adds the driver for the Tegra 2x AC97 host controller.

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

>>> +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?
>>
> To be honest I have no idea what STM really is, seems to be some form of
> packed format selection. If not set playback outputs only more or less
> random noise. Only PCM_EN controls FIFO and DMA operations though, so
> it's ok to just disable this to stop playback.

I'd be tempted to just hard-code those bits on in probe() then, but it's
not a big deal, so not worth spinning the patch for that.

I guess I meant to say,
Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>