[alsa-devel] [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

[Jarkko Nikula]

On Fri, Apr 18, 2008 at 5:03 PM, Daniel Mack <daniel at caiaq.org> wrote:

> On Fri, Apr 18, 2008 at 04:47:16PM +0300, Jarkko Nikula wrote:
> > if (params_rate(params) == aic3x->sysclk / (128 * pll_q))
>
> Hmm? Why do you think so? I'm afraid I don't get your point here.
>

I'm might be completely wrong here since I've used this codec only in
configuration where the interface rate or FSref equals to audio sampling
rate , but as far as I've interpreted the spec correctly we can have e.g.
the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be
disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).

And I think here equation "Fsref = CLKDIV_IN / (128 × Q)" Fsref refers to
44.1 or 48 kHz not the audio sampling rate?

But I don't want to speculate or do my own interpretation here. I'll ask
this from TI next week from my jarkko.nikula at nokia.com and can put you as a
cc as well.

Or do the driver author have any better knowledge here?


> > Probably you forgot to move bypass case in this version after writing
> the
> > AIC3X_SAMPLE_RATE_SEL_REG?
>
> No, actually not.
>


Your latest patch has:

+    if (bypass_pll)
+        return 0;
+
     /* codec sample rate select */
     data = aic3x_divs[i].sr_reg;
     data |= (data << 4);


>
> > Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values
> of
> > Q are not allowed.
>
> NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment.
> Support for more flexible settings could be done in another step.
> Also, the PLL setup could be done by calculation of values rather
> that by a lookup table.


Again, I'm might be wrong but I NDAC and NADC refers to register
AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.


> I'd like to see this patch applied now as base for further refinements.
> Do you agree?
>
>
I cannot prevent it to be applied, that's the driver author or maintainers
task to justify. I can only throw some discussion here if I see something to
be unclear (to me).

And don't take my comments as a nitpicking. Definitely patch like this is
worth to get in but we have to make sure that no possible bugs are
introduced. -EINVAL is much better alternative for some new configurations.


Jarkko