Re: [PATCH 10/11] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1820 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]