On Wed, May 09, 2012 at 11:09:20AM +0200, Ola Lilja wrote:
On 05/09/2012 10:33 AM, Mark Brown wrote:
On Wed, May 09, 2012 at 09:48:35AM +0200, Ola Lilja wrote:
Please don't just ignore review and continue to submit the same stuff unless there's been clear discussion that what's happening is actually OK.
I wasn't aware of that the comment with | above was a show-stopper since I explained before why we need to do it. I wasn't either aware that you meant that I should just move it to the machine-driver. Furthermore, I didn't want to spam to much comments each time.
At the very least this should go in the machine driver, though I have to say I remain very much unsure that this needs to be directly controlled from the application layer at all.
+int ab8500_audio_setup_if1(struct snd_soc_codec *codec,
unsigned int fmt,unsigned int wl,unsigned int delay)
Why is this not static?
Because it is called from the machine-driver.
Why? No other driver does this...
This is setting up an I2S-interface connected directly to another chip for FM-radio. It is not triggered by opening an ALSA-device. How/where do you want me to do this?
Once again, work with the frameworks not against or around them. Whenever you find yourself putting in a custom device specific API that's externally visible this should be a warning sign.
In this case there's nothing unusual here, it's exactly the same as the connection to a digital baseband or back to back link to another CODEC. The CODEC driver should just provide a DAI and let the framework connect it up, currently that should be with a CODEC<->CODEC link though older systems use a bodge where the link is described as a normal PCM.