On Thu, Sep 18, 2008 at 06:08:20PM +0200, Manuel Lauss wrote:
It'd be worth updating this comment to mention what you're doing with the chip power in the standby state - it's unusual for the codec to be held with so little power in standby mode that register writes don't work, which is why this is a driver-specific thing. I guess if DAPM support were implemented then this wouldn't be required any more?
The platform I developed this driver for can cut power to the codec and amplifiers (customer requirement: no audio is playing -- save as much power as possible, which the hardware engineer interpreted as cutting all electrical power to the whole audio subsystem). To be honest I somewhat expect the I2S codec drivers to be able to cope with a situation such as this.
This is actually very unusual. Most audio subsystems take a relatively long time to power on cleanly, much longer than is generally considered acceptable for doing on the start of every playback - things like charging capacitors and stabalising reference voltages without generating pops in the output can take hundreds of miliseconds which is much longer thean the sort of latency people normally expect from direct interaction with a device. Similarly, fully powering down can also take a noticable amount of time. Even writing the registers back over a slow bus like I2C can take longer than is desirable. Maintaining the audio subsystem in a steady state usually consumes very little power and allows rapid startup of playback streams.
If this were being supported by the core it'd be done with something like explicit calls to the codec drivers, probably the existing suspend and resume callbacks since they already fulfil essentially this purpose. This would need to be requested by the machine driver in order to avoid impacting machines that don't need it (not to mention those that don't have appropriate power switches).
The other thing is, I tried to implement finer-grained PM, but to be honest, after staring at the DAPM defines, it is still a mystery to me how that stuff is supposed to be hooked up.
Broadly speaking, you define a series of controls for the power switches for elements of the codec that have power in much the same way as you define other controls. You then specify which
Perhaps looking at an existing driver in conjunction with the datasheet would help make things a bit clearer? Most of the Wolfson devices have public datasheets - off the top of my head the WM8900 is reasonably complex and has a public datasheet. As well as register information and so on there's pictures of how things are connected together which are useful to refer to along with the code.
This and some of the other code will force the ADC and DAC to have the same configuration - is that needed by the codec? If not then it'd be better to check which substream it is operating on and only configure the ADC or DAC as appropriate (you could save lots of conditionals by only checking when actually writing the configuration out at the end of the function).
It's not mandated by the chip but rather my development platform (ADC and DAC have separate I2S lines; my test hardware has bit- and frameclock of both tied together).
Surely in that case the interfaces you have provided for configuring non-shared LRCLK don't currently work?
With the code as it currently stands the codec should impose constraints to let user space know that the playback and capture streams have to be in the same mode. There's examples of how to do that in cs4270, ssm2602, wm8903 and possibly other drivers.
In my case, the constraints should be imposed by the machine driver ;-) The codec doesn't really care.
The codec hardware looks like it doesn't but the current codec driver will program both DAC and ADC clocks whenever a stream is configured so the requirement is going to affect anyone using the codec driver currently.
In general if the codec has clocking which can be configured as flexibly as this one appears to then it's normally better to allow the machine driver to configure the clocking manually via the clock configuration APIs.
Agreed; I didn't know I can actually do that with ASoCv1.
Quite a few of the existing drivers implement this model - most of the WM89xx series except WM8903, for example.
+#define AD1939_RATES \
- (SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 | \
SNDRV_PCM_RATE_192000)
Your code appeared to support rather more rates than this?
Theoretically it supports anything coming from the I2S frameclk line. How can I express that?
Currently ASoC can't cope with continuous rates so the best thing to do would be use SNDDRV_PCM_RATE_8000_192000 which will add all the fixed rates in that range. TBH, it's very rare to see
It's probably worth complaining if someone tries to set ad->mixpairs to something more than 4. It may be better to just not do this and leave the controls exposed - why is this being done?
Customer kept asking why alsa shows 4 stereo output mixer controls when actually only 2 are connected.
You might want to look at Takashi's recent patch to add a new API call snd_ctl_activate_id() which provides a generic way of doing this without requiring specific code like this be added to individual codec drivers - it's in his unstable git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable-2.6.git
Your machine driver could use this rather than add it in the audio driver.