[PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x (original) (raw)

Mark Brown broonie at kernel.org
Tue Feb 4 09:54:10 PST 2014

On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote:

Mark Brown <broonie at kernel.org> wrote:

> > + /* load the optional CODEC */ > > + ofplatformpopulate(np, NULL, NULL, &client->dev);

> Why is this using ofplatformpopulate()? That's a very odd way of > doing things.

The i2c does not populate the subnodes in the DT. I did not find why, but, what is sure is that if ofplatformpopulate() is not called, the tda CODEC module is not loaded.

You shouldn't be representing this as a separate node in the DT unless there really is a distinct and reusable IP, otherwise you're putting Linux implementation details in there. Describe the hardware, not the implemementation.

You may find an other example in drivers/mfd/twl-core.c.

The TWL drivers aren't always a shining example of how to do things - they were one of the earliest MFDs so there's warts in there.

> > +config SNDSOCTDA998X > > + tristate > > + depends on OF > > + default y if DRMI2CNXPTDA998X=y > > + default m if DRMI2CNXPTDA998X=m

> Make this visible if it can be selected from DT so it can be used with > generic cards.

I don't understand. The tda CODEC can only be used with the TDA998x I2C driver. It might have been included in the tda998x source as well.

You shouldn't have the default settings there at all, that's not the normal idiom for MFDs. I'd also not expect to have to build the CODEC driver just because I built the DRM component.

Now, the CODEC is declared inside the tda998x as a node child. But, in a bad DT, the tda CODEC could be declared anywhere, even inside a other DRM I2C slave encoder, in which case, bad things would happen...

So, part of the problem here is that this is being explicitly declared in the DT leading to more sources for error.

> What does this actually do? No information is being passed in to the > core function here, not even any information on if it's starting or > stopping. Looking at the rest of the code I can't help thinking it > might be clearer to inline this possibly with a lookup helper, the code > is very small and the lack of parameters makes it hard to follow.

I thought it was simple enough. The function tdastartstop() is called from 2 places:

It's not at all obvious - _audio_update() isn't a terribly descriptive name, just looking at that function by itself I had no idea what it was supposed to be doing.

- on audio start in tdastartup with the audio type (DAI id) priv->daiid = dai->id;

- on audio stop with a null audio type priv->daiid = 0; /* streaming stop */

On stream start, the DAI id is never null, as explained in the patch 1:

The audio format values in the encoder configuration interface are changed to non null values so that the value 0 is used in the audio function to indicate that audio streaming is stopped.

and on streaming stop the port is not meaningful.

I will add a null item in the enum (AFMTNOAUDIO).

So we can't use both streams simultaneously then? That's a bit sad. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140204/037aca8f/attachment-0001.pgp>

More information about the dri-devel mailing list