On Fri, Apr 18, 2008 at 5:03 PM, Daniel Mack daniel@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@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