[alsa-devel] [PATCH 0/5] [RFC] ALSA ASoC Support for TLVaic23b audio codec
Hi all,
The following patches adds ASOC driver for TLVaic23b audio codec.
Tested playback and capture on omap 5912osk board with some hacks in the omap platform and machine driver.
Thanks, Arun -- To unsubscribe from this list: send the line "unsubscribe alsa-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arun KS wrote:
Hi all,
The following patches adds ASOC driver for TLVaic23b audio codec.
Tested playback and capture on omap 5912osk board with some hacks in the omap platform and machine driver.
Thanks, Arun --
Great. I just finished the same thing for davinci but using spi. I was waiting until the adc issue I mentioned yesterday was resolved before posting it. I'm glad I won't have to now. But my board also has I2c, just not with this codec. Can the I2c code be moved into the board directory? I have my spi code in the board directory and just pass the write function pointer. i.e.
codec->hw_write = setup_data->hw_write;
Can you email me some data captured by from your mic input? I'm interested in seeing if you have the same trouble in dsp_a mode.
Thanks, Troy
On Mon, Sep 29, 2008 at 09:47:10AM -0700, Troy Kisky wrote:
Great. I just finished the same thing for davinci but using spi. I was waiting until the adc issue I mentioned yesterday was resolved before posting it. I'm glad I won't have to now. But my board also has I2c, just not with this
It'd be great if you could work with Arun to ensure that all the features which you are using are supported in the driver - it looks like at least SPI support will need to be added.
codec. Can the I2c code be moved into the board directory? I have my spi code in the board directory and just pass the write function pointer. i.e.
No, the I2C and SPI support should both be part of the driver. The driver should just be modified to allow both I2C and SPI support to be compiled in and let the board driver register the appropriate one at run time.
Mark Brown wrote:
On Mon, Sep 29, 2008 at 09:47:10AM -0700, Troy Kisky wrote:
No, the I2C and SPI support should both be part of the driver. The driver should just be modified to allow both I2C and SPI support to be compiled in and let the board driver register the appropriate one at run time.
Seems to me a Kconfig option would be more efficient. Otherwise, one of the two options is just going to waste space.
Troy
On Mon, Sep 29, 2008 at 10:05:02AM -0700, Troy Kisky wrote:
Mark Brown wrote:
No, the I2C and SPI support should both be part of the driver. The
Seems to me a Kconfig option would be more efficient. Otherwise, one of the two options is just going to waste space.
We need to support building for both I2C and SPI simultaneously in order to allow kernels supporting multiple boards to be built. If the selection is done at build time then the number of configurations that can be built in a single image is restricted needlessly.
I'd ack patches which made it possible to configure one or the other out but given the tiny amount of glue required I'd personally not bother writing that.
Mark Brown wrote:
On Mon, Sep 29, 2008 at 09:47:10AM -0700, Troy Kisky wrote:
Great. I just finished the same thing for davinci but using spi. I was waiting until the adc issue I mentioned yesterday was resolved before posting it. I'm glad I won't have to now. But my board also has I2c, just not with this
It'd be great if you could work with Arun to ensure that all the features which you are using are supported in the driver - it looks like at least SPI support will need to be added.
codec. Can the I2c code be moved into the board directory? I have my spi code in the board directory and just pass the write function pointer. i.e.
No, the I2C and SPI support should both be part of the driver. The driver should just be modified to allow both I2C and SPI support to be compiled in and let the board driver register the appropriate one at run time.
Can you please explain to me why they should both be part of the codec driver?
Thanks Troy
On Mon, Sep 29, 2008 at 10:27:06AM -0700, Troy Kisky wrote:
Mark Brown wrote:
codec. Can the I2c code be moved into the board directory? I have my spi code in the board directory and just pass the write function pointer. i.e.
No, the I2C and SPI support should both be part of the driver. The driver should just be modified to allow both I2C and SPI support to be compiled in and let the board driver register the appropriate one at run time.
Can you please explain to me why they should both be part of the codec driver?
Neither is board specific - there's no sense in having each board that needs SPI support replicate the code to register a SPI device and do the marshalling of data for SPI writes. What motivation do you see for not doing that?
Mark Brown wrote:
On Mon, Sep 29, 2008 at 10:27:06AM -0700, Troy Kisky wrote:
Mark Brown wrote:
codec. Can the I2c code be moved into the board directory? I have my spi code in the board directory and just pass the write function pointer. i.e.
No, the I2C and SPI support should both be part of the driver. The driver should just be modified to allow both I2C and SPI support to be compiled in and let the board driver register the appropriate one at run time.
Can you please explain to me why they should both be part of the codec driver?
Neither is board specific - there's no sense in having each board that needs SPI support replicate the code to register a SPI device and do the marshalling of data for SPI writes. What motivation do you see for not doing that?
It just doesn't seem to be logically a part of the codec code. And I didn't register an spi device. I just linked the simple spi routines with my board code (separate file).
Plus, it seems a lot of code duplication if each codec registers the spi device and I2C device. Are there more boards, or more codecs???
Thanks Troy
On Mon, Sep 29, 2008 at 10:58:43AM -0700, Troy Kisky wrote:
Mark Brown wrote:
Neither is board specific - there's no sense in having each board that needs SPI support replicate the code to register a SPI device and do the marshalling of data for SPI writes. What motivation do you see for not doing that?
It just doesn't seem to be logically a part of the codec code. And I didn't register
The data marshalling is a function of the codec hardware - whatever it has been hooked up to the register address and data values written are going to need to be in a given format when they hit the codec. The SPI and I2C APIs abstract away the details of the actual controller from the codec driver.
an spi device. I just linked the simple spi routines with my board code (separate file).
I'm not sure which simple SPI API you're referring to here? In any case, ASoC is shortly going to pretty much require a struct device for the codec so it will be required to have a device of some kind registered.
Plus, it seems a lot of code duplication if each codec registers the spi device and I2C device. Are there more boards, or more codecs???
In the device model the board registers the *device*, the codec (or whatever) driver registers the *driver* - the two are separated. The driver core then instantiates the drivers based on what the board specifies. There's not really any overlap between the two areas that I can see.
Otherwise could you explain in more detail where you see the code duplication coming from?
Mark Brown wrote:
On Mon, Sep 29, 2008 at 10:58:43AM -0700, Troy Kisky wrote: In the device model the board registers the *device*, the codec (or whatever) driver registers the *driver* - the two are separated. The driver core then instantiates the drivers based on what the board specifies. There's not really any overlap between the two areas that I can see.
Otherwise could you explain in more detail where you see the code duplication coming from?
My point, though admittedly a minor one, was that the I2C device or spi device could be initialized before the codec is probed.
That would be moving code from codec files into board files. Better would be moving the code into a common soc directory file.
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) +/* + * If the i2c layer weren't so broken, we could pass this kind of data + * around + */ +static int aic23_codec_probe(struct i2c_client *i2c, + const struct i2c_device_id *i2c_id) +{ + struct snd_soc_device *socdev = aic23_socdev; + struct snd_soc_codec *codec = socdev->codec; + int ret; + + if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA)) + return -EINVAL; + + + i2c_set_clientdata(i2c, codec); + codec->control_data = i2c; + .... + ret = aic23_init(socdev); + if (ret < 0) { + printk(KERN_ERR "aic23: failed to initialise AIC23\n"); + goto err; + } ... The above would move to codec probe
+ return ret; + +err: + kfree(codec); + kfree(i2c); + return ret; +} +static int __exit aic23_i2c_remove(struct i2c_client *i2c) +{ + + put_device(&i2c->dev); + return 0; +} + +static const struct i2c_device_id tlvaic23b_id[] = { + {"tlvaic23b", 0}, + {} +}; + +MODULE_DEVICE_TABLE(i2c, tlvaic23b_id); + +static struct i2c_driver aic23b_i2c_driver = { + .driver = { + .name = "tlvaic23b", + }, + .probe = aic23_codec_probe, + .remove = __exit_p(aic23_i2c_remove), + .id_table = tlvaic23b_id, +}; + +#endif
If this code could be generalized to be useable by most codecs, I think the code would look better. This is the "almost" duplication to which I'm referring.
Troy
On Mon, Sep 29, 2008 at 12:07:53PM -0700, Troy Kisky wrote:
Mark Brown wrote:
Otherwise could you explain in more detail where you see the code duplication coming from?
My point, though admittedly a minor one, was that the I2C device or spi device could be initialized before the codec is probed.
That would be moving code from codec files into board files. Better would be moving the code into a common soc directory file.
Ah, I think I see what you're talking about for at least this part of your e-mails, though I'm not 100% sure.
Just to clarify, I *think* that what you're actually talking about here is the fact that currently ASoC registers the entire ASoC subsystem off the codec device registration with whatever control subsystem it uses and therefore the final call to register the device is done within codec driver code (though triggered by the machine driver)?
It would be much easier if you could be a *little* more verbose and/or precise in your e-mails. You started off by talking about wanting to put the "I2C code" and "SPI code" into the board drivers which rather suggests removing all bus access code.
- ret = aic23_init(socdev);
- if (ret < 0) {
printk(KERN_ERR "aic23: failed to initialise AIC23\n");goto err;- }
... The above would move to codec probe
The code you're quoting there is in the codec probe function? init() is split out so it can be shared between multiple control methods.
That said, there is an issue with this at the minute due to the fact that the core does not yet use the device model properly and (essentially) hangs all the drivers off the codec device which leads to having both ai23_probe() which registers the I2C driver and aic23_codec_probe(). This is half way to being rectified (the code exists but is in the process of being merged) so that what happens is that all the components of the system register with the ASoC core after being probed normally from registrations done in the board init code or similar. This will remove all the socdev interaction from the codec driver which is, I think, what you're getting at?
+static struct i2c_driver aic23b_i2c_driver = {
.driver = {.name = "tlvaic23b",},.probe = aic23_codec_probe,.remove = __exit_p(aic23_i2c_remove),.id_table = tlvaic23b_id,+};
+#endif
If this code could be generalized to be useable by most codecs, I think the code would look better. This is the "almost" duplication to which I'm referring.
There's always a certain amount of bolierplate required to tell the device core about drivers which isn't going away. It's really very small and the benefits in terms of things like refcounting modules and autoloading them are worthwhile.
That said, there is an issue with this at the minute due to the fact that the core does not yet use the device model properly and (essentially) hangs all the drivers off the codec device which leads to having both ai23_probe() which registers the I2C driver and aic23_codec_probe(). This is half way to being rectified (the code exists but is in the process of being merged) so that what happens is that all the components of the system register with the ASoC core after being probed normally from registrations done in the board init code or similar. This will remove all the socdev interaction from the codec driver which is, I think, what you're getting at?
Thanks for the explanation and status update. I have no problem with doing both in the codec until this code is merged.
Thanks Troy
participants (3)
-
Arun KS -
Mark Brown -
Troy Kisky