On Tue, Jan 18, 2011 at 10:32:55AM +0800, Zeng Zhaoming wrote:
On Mon 2011-01-17 15:32:11, Mark Brown wrote:
It's not clear why you're storing some of this stuff in the private data
- for example, capture_channels is only referred to in the place where
it is set (so you may as well use the original value) and small_pop is set unconditionally (it looks like the intention was to have platform data for it?).
yes, you are right. lrclk and capture_channels fields can be remove. small_pop is a configurable option of platform data.
Your patch unconditionally sets small_pop, there's no configuration via platform data or otherwise.
of event and never turns anything off. If this can't just be set once at startup then a comment explaining why an event is needed would be good.
the odd code is cuased by:
Please note what I'm saying about making the code clear.
case SND_SOC_DAPM_PRE_PMU:snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP,SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP);break;case SND_SOC_DAPM_POST_PMD:snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,SGTL5000_DAC_POWERUP, 0);break;
Why does the powerdown not disable VAG_POWERUP? My first thought was that VAG_POWERUP ought to be a supply widget...
My first version, VAG_POWERUP is a supply widget, but spec says: The headphone (and/or lineout) powerup should be set BEFORE clearing this bit.
But the powerdown sequence go against it. So I turn to powerup VAG with DAC, and powerdown it before HP and Line_out.
The above doesn't seem to tie in with your code terribly well. A frequent issue throughout this code is that you're doing a bunch of unusual stuff and it's really not clear what the code is supposed to do, please work to make the code more comprehensible. This is a problem both from a legibility point of view and from the point of view of being likely to break if the core changes.
The same issue applies to a lot of the other concerns I raised, for brevity I've cut most of the discussion - in general the major issue with the code is that it's really hard to read.
switch (sys_fs / sgtl5000->lrclk) {case 4:clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;break;case 2:clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;break;default:break;}
I'd expect to see this error out if it can't find an appropriate divider?
the default means sys_fs / lrclk = 1, not a error.
So what happens when sys_fs / lrclk is not 1? If nothing else the above code needs to be *much* clearer.