2010/6/28 Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> On Mon, Jun 28, 2010 at 08:44:46AM +0200, Raffaele Recalcati wrote:
> > From: Raffaele Recalcati <raffaele.recalcati@xxxxxxxxxx>
>
> It's still very hard to understand what this patch is supposed to do.
> As previously mentioned this would probably be a lot clearer if you
> split this into multiple patches, for example one adding support for the
> fast clock mode, one adding support for selecting the pin used for any
> external clock and then further patches with any other changes.
>
Looking at other paches, they are simpler than mine.
I'll try to split it, hoping to obtain the final result.
>
> I strongly suggest looking at the commit messages for other patches in
> the kernel and trying to follow a similar style.
>
> > Added audio playback support with [frame sync master - clock master]
> mode
> > and with [frame sync master - clock slave].
>
> What are these modes - which clock are you talking about?
>
McBSP i2s interface to external codec.
>
> > i2s_fast_clock switch can be used to have better approximate or
> > symmetric waveforms.
>
> Why would someone choose not to use this?
>
I was not sure if symmetric waveform can be a must.
In general I think it is better a non symmetric, but better approximate,
waveform.
Anyway, it is better to have the possibility to choose in my opinion,
because I have not so much experience in i2s communication.
>
> > clk_input_pin board info can be used to select it depending on hw
> > connections
>
> > 3. We haven't changed the evmdm365 support (due also to CPLD that
> doesn't
> > help to understand)
> > We don't know in this mode if audio stereo works on evmdm365.
> > Probably it does.
>
> This is what makes me unsure if you're trying to add new modes or not -
> if you're adding new modes then I'd expect that existing boards would be
> unaffected with any changes to use the new feature being easily
> seperable.
>
Some tests are needed, but it requires time.
I'll try try to make some tests on evmdm365, but I'm not sure to have time
to do it.
> > + /*
> > + * This define works when both clock and FS are output for the cpu
> > + * and makes clock very fast (FS is not simmetrical, but sampling
>
> symmetrical.
>
thx
>
> > + * frequency is better approximated
> > + */
> > + int i2s_fast_clock;
>
> Is this a bool?
>
yes, I'll change it.
>
> > + /* To be used when cpu gets clock from extenal pin */
> > + int clk_input_pin;
> > +
>
> How would one use this?
>
looking at 2.5 Clock, Frames, and Data in
you can select MCBSP_CLKS or other input clock pins.
>From board file you can select the possibilites:
static struct snd_platform_data dm365_bmx_snd_data[] = {
{
},
{
.i2s_fast_clock = 0,
.clk_input_pin = MCBSP_CLKS,
},
};
If not set, it works as evm works, that is the default mode.
> > case SND_SOC_DAIFMT_CBM_CFS:
> > - /* McBSP CLKR pin is the input for the Sample Rate
> Generator.
> > - * McBSP FSR and FSX are driven by the Sample Rate
> Generator. */
> > - pcr = DAVINCI_MCBSP_PCR_SCLKME |
> > - DAVINCI_MCBSP_PCR_FSXM |
> > - DAVINCI_MCBSP_PCR_FSRM;
> > + pcr = DAVINCI_MCBSP_PCR_FSRM | DAVINCI_MCBSP_PCR_FSXM;
> > + if (dev->clk_input_pin == MCBSP_CLKS)
> > + pcr |= DAVINCI_MCBSP_PCR_CLKXM |
> > + DAVINCI_MCBSP_PCR_CLKRM;
> > + else
> > + /*
> > + * McBSP CLKR pin is the input for the Sample Rate
> > + * Generator.
> > + * McBSP FSR and FSX are driven by the Sample Rate
> > + * Generator.
> > + */
> > + pcr |= DAVINCI_MCBSP_PCR_SCLKME;
>
> This looks like you need a switch statement for the clock selection.
>
ok.
> > +static int davinci_i2s_dai_set_clkdiv(struct snd_soc_dai *cpu_dai,
> > + int div_id, int div)
> > +{
> > + struct davinci_mcbsp_dev *dev = cpu_dai->private_data;
> > + int srgr;
> > +
> > + dev->clk_div = div;
> > + return 0;
> > +}
> > +
>
> I would add a clock ID here in case someone wants to configure another
> clock in the patch.
>
Can you explain better,
please?
>
> > + if (master == SND_SOC_DAIFMT_CBS_CFS) {
>
> Use a switch staetment for this too.
>
> > + clk = clk_get(NULL, "pll1_sysclk6");
>
> You're doing a clk_get() every time you go through here but never
> freeing it - it would seem better to just do the clk_get() at startup.
>
Thx for this.
I think it could create problems, right?
I can do it in the probe, I think, similarly to other drivers.
> I'd also expect this to be doing a clk_get() using the struct device for
> the DAI so that the driver can function even if the clock tree for a new..
> SoC is different.
>
Sorry, I don't understand, can you explain me better?
>
> > + if (clk)
> > + freq = clk_get_rate(clk);
>
> clk_get() returns an error pointer on error, not NULL, and...
>
gueSsing that I have to use this example code:
aemif_clk = clk_get(NULL, "aemif");
if (IS_ERR(aemif_clk))
return;
I'll do it.
>
> > + freq = 122000000; /* FIXME ask to Texas */
>
> ...this presumably ought to be in an else clause (or perhaps just not
> using this clocking at all if you can't find the clock?).
>
freq is used to obtain clk_div.
The real problem is that, as explained in the manual, it is not clear its
value.
I hope someone can help!!
>
> > + } else if (master == SND_SOC_DAIFMT_CBM_CFS) {
> > + /* Clock given on CLKS */
>
> What if the user selected a different clock source?
>
I need another check,
right!
>
> > + if (master == SND_SOC_DAIFMT_CBS_CFS ||
> > + master == SND_SOC_DAIFMT_CBS_CFM) {
>
> Again, switch statement.
>
ok
>
> > + if (master == SND_SOC_DAIFMT_CBS_CFS ||
> > + master == SND_SOC_DAIFMT_CBS_CFM) {
>
> Here too.
>
again.
Conclusion
I split patches and I do fixes above.
Than I'll submit again and wait for new info.
The freq problem is describe here below, but it is not clear to me:
>From *TMS320DM36x DMSoC Multichannel Buffered Serial Port User's Guide (Rev.
A) <http://www.ti.com/litv/pdf/sprufi3a>*
2.5.3 Data Clock Generation
When the receive/transmit clock mode is set to 1 (CLK(R/X)M = 1 in the pin
control register (PCR)), the
data clocks (CLK(R/X)) are driven by the internal sample rate generator
output clock, CLKG. You can
select for the receiver and transmitter from a variety of data bit clocks
including:
• The input clock to the sample rate generator, which can be either the
internal clock source or a
dedicated external clock source via the MCBSP_CLKX, MCBSP_CLKR, or
MCBSP_CLKS pins. The
McBSP internal clock is the CPU/6 clock. See Section 2.5.3.1 for details on
the source of the McBSP
internal clock.
• The input clock source (internal clock source or external clock
MCBSP_CLKX/MCBSP_CLKR/MCBSP_CLKS) to the sample rate generator can be
divided-down by a
programmable value (CLKGDV bit in the sample rate generator register (SRGR))
to drive CLKG.
Regardless of the source to the sample rate generator, the rising edge of
CLKSRG (see Figure 5)
generates CLKG and FSG.
CPU/6 is not clear.
regards,
Raffaele
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
