Re: [PATCH 5/7] ASoC: tegra: add ac97 host driver

[Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]

On Thu, Dec 20, 2012 at 12:44:24PM -0700, Stephen Warren wrote:
> On 12/19/2012 04:17 PM, Lucas Stach wrote:

> > +	/* reset line is not driven by DAC pad group, have to toggle GPIO */
> > +	gpio_set_value(workdata->reset_gpio, 0);
> > +	udelay(2);

> I'm not quite sure what that implies. Might some (Tegra) HW designs use
> a dedicated signal from the AC'97 HW to reset the CODEC and others not?
> That would make the GPIO optional. Or, does the comment mean that Tegra
> doesn't ever have a dedicated CODEC reset signal, so we always have to
> use a GPIO? If so, this comment might be more appropriate inside probe()
> where I asked my related questions.

The traditional approach, pioneered by almost everyone who made an AC'97
controller, is to fill the thing with bugs.  Failing to drive the reset
line correctly is just one of the common bugs.

> > +static const struct snd_soc_dai_ops tegra20_ac97_dai_ops = {
> > +	.trigger	= tegra20_ac97_trigger,
> > +};

> No .set_fmt or .hw_params? Does the AC'97 ASoC core code handle that
> somehow? I guess that's what soc_ac97_ops is for?

There should be no controller side configurations for these things.
AC'97 defines the clocking format completely and variable sample rates
are done by the CODEC flagging when data is present in a given timeslot.

> > +	ac97->reset_gpio = of_get_named_gpio(pdev->dev.of_node,
> > +					     "nvidia,codec-reset-gpio", 0);
> > +	if (gpio_is_valid(ac97->reset_gpio)) {
> > +		ret = devm_gpio_request_one(&pdev->dev, ac97->reset_gpio,
> > +					    GPIOF_OUT_INIT_HIGH, "codec-reset");

> Shouldn't this get the flags from the GPIO specifier, and deal with
> active-high/active-low resets, or is the polarity set by the AC'97 spec?

Spec.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]