Re: [PATCH 02/10] ASoc: mxs: add mxs-saif driver

[Dong Aisheng <dongas86@gmail.com>]

2011/7/10 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Sun, Jul 10, 2011 at 04:17:13PM +0800, Dong Aisheng wrote:
>> 2011/7/9 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>> > On Fri, Jul 08, 2011 at 11:59:42PM +0800, Dong Aisheng wrote:
>
>> >> +     switch (clk_id) {
>> >> +     case MXS_SAIF_SYS_CLK:
>> >> +             clk_set_rate(saif->clk, freq);
>> >> +             clk_enable(saif->clk);
>
>> > How would one turn this clock off?
>
>> Currenty simply enable clock always.
>
> If that's what you're doing you should enable the clock when the device
> is probed and disable it when the device is removed.  These repeated
> enables will mean that you're constantly leaking references.
Yes, i noticed this issue.

>> The problem is that the codec may use the MCLK supplied by SAIF as its
>> system clock for normal operations such as i2c r/w.
>
> That may be true for your particular system but it is not going to be
> true in general - most devices can handle having their MCLK removed.
Yes, if i cover that, that may make driver a little complicated.
Should i do that now or as the next work?
Also i still do not have such type of codecs integrated on hand.

>> If we disable it after playback or capture. The automatic dapm
>> operations of codec may fail on i2c r/w. Will check if dapm has any
>> machnism to avoid this.
>> Can you share your experience ont this issue?
>
> You should ideally let the machine driver or other system integration
> code control if the clock is constantly enabled, or at the very least
> control it from the probe and remove.
Thanks for the suggestions.
I will try them.

>> >> +static irqreturn_t mxs_saif_irq(int irq, void *dev_id)
>> >> +{
>> >> +     struct mxs_saif *saif = dev_id;
>> >> +
>> >> +     if (saif->fifo_err_counter++ % 100 == 0)
>> >
>> > The rate limit looks awfully suspicious...
>> Originally it's just for printing less error messages when the issue happens.
>> I could remove the rate limit.
>
> Please do so.
Got it.

>> >> +     clk_set_rate(saif->clk, 12000000);
>> >> +     clk_enable(saif->clk);
>
>> > How did you pick this clock rate and why does it need to be set?  It's
>> > not an obvious audio rate...
>
>> It's initial clock rate for normal operations of codecs based on MCLK.
>> We found the default MCLK ouput(only about 7.3Khz) can not work for codecs like
>> sgtl5000 for its initialization since its clock range is 8Mhz~27Mhz.
>> So we set a common working clock 12Mhz here.
>> Maybe i should try to set it elsewhere or just set it via platform data.
>
> This is the same issue as the above one with repeated enables?  You
> should delegate the rate selection and ideally also the enable control
> to the machine driver, it can set something in its probe() function.
Not the same one. The repeated enables is a bug.(Will fix)
Here we just need set a init proper clock rate of MCLK for codec firstly.
(Codec's initialization(i2c r/w) in probe function may need it).
I would try your suggestion to let machine driver to set it.