Re: [alsa-devel] [PATCH] ASoC: Add WM8903 CODEC driver
Hi Mark,
On Tue, 26 Aug 2008 13:05:27 +0100, Mark Brown wrote:
The WM8903 is a high performance ultra-low power stereo CODEC optimised for portable audio applications. Features include:
* 5mW power consumption for DAC to headphone playback * Stereo DAC SNR 96dB typical, THD -86dB typical * Stereo ADC SNR 93dB typical, THD -80dB typical * Up to 3 single ended inputs per stereo channel * Up to 2 pseudo differential inputs per stereo channel * Up to 1 fully differential mic input per stereo channel * Digital Dynamic Range Controller (compressor/limiter) * Digital sidetone mixingSigned-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wm8903.c | 1813 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/wm8903.h | 1463 ++++++++++++++++++++++++++++++++++++ 4 files changed, 3282 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/wm8903.c create mode 100644 sound/soc/codecs/wm8903.h
Here are minor issues I spotted in the i2c part of your driver:
(...) +static int wm8903_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)+{
- struct snd_soc_device *socdev = wm8903_socdev;
- struct snd_soc_codec *codec = socdev->codec;
- int ret;
- i2c_set_clientdata(i2c, codec);
- codec->control_data = i2c;
- ret = wm8903_init(socdev);
- if (ret < 0)
dev_err(&i2c->dev, "Device initialisation failed\n");- return ret;
+}
+static int wm8903_i2c_remove(struct i2c_client *client) +{
- struct snd_soc_codec *codec = i2c_get_clientdata(client);
- kfree(codec->reg_cache);
- return 0;
+}
+/* i2c codec control layer */ +static const struct i2c_device_id wm8903_i2c_id[] = {
{ "wm8903", 0 },{ }+}; +MODULE_DEVICE_TABLE(i2c, wm8903_i2c_id);
+static struct i2c_driver wm8903_i2c_driver = {
- .driver = {
.name = "WM8903",.owner = THIS_MODULE,- },
- .probe = wm8903_i2c_probe,
- .remove = wm8903_i2c_remove,
- .id_table = wm8903_i2c_id,
+};
+static struct i2c_client *wm8903_i2c_device;
I know that I am the one who came up with this idea, but on second thought, wm8903_i2c_device will always be equal to wm8903_socdev->codec->control_data, so we can easily do without it.
+static int wm8903_probe(struct platform_device *pdev) +{
- struct snd_soc_device *socdev = platform_get_drvdata(pdev);
- struct wm8903_setup_data *setup;
- struct snd_soc_codec *codec;
- struct wm8903_priv *wm8903;
- struct i2c_board_info board_info;
- struct i2c_adapter *adapter;
- int ret = 0;
- setup = socdev->codec_data;
- if (!setup->i2c_address) {
dev_err(&pdev->dev, "No codec address provided\n");return -ENODEV;- }
- codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
- if (codec == NULL)
return -ENOMEM;- wm8903 = kzalloc(sizeof(struct wm8903_priv), GFP_KERNEL);
- if (wm8903 == NULL) {
ret = -ENOMEM;goto err_codec;- }
- codec->private_data = wm8903;
- socdev->codec = codec;
- mutex_init(&codec->mutex);
- INIT_LIST_HEAD(&codec->dapm_widgets);
- INIT_LIST_HEAD(&codec->dapm_paths);
- wm8903_socdev = socdev;
- codec->hw_write = (hw_write_t)i2c_master_send;
- ret = i2c_add_driver(&wm8903_i2c_driver);
- if (ret != 0) {
dev_err(&pdev->dev, "can't add i2c driver");
Missing \n at end of string (looks like many other drivers have this problem - I found 11 other cases under sound/soc, I can send a patch fixing these if you want.)
goto err_priv;- } else {
memset(&board_info, 0, sizeof(board_info));strlcpy(board_info.type, "wm8903", I2C_NAME_SIZE);board_info.addr = setup->i2c_address;adapter = i2c_get_adapter(setup->i2c_bus);if (!adapter) {dev_err(&pdev->dev, "Can't get I2C bus %d\n",setup->i2c_bus);goto err_adapter;
You are jumping to the error path but ret has value 0. This means that the function returns success when you really mean failure, and trouble is likely to arise after that.
}wm8903_i2c_device = i2c_new_device(adapter, &board_info);i2c_put_adapter(adapter);if (wm8903_i2c_device == NULL) {dev_err(&pdev->dev,"I2C driver registration failed\n");ret = -ENODEV;goto err_adapter;}- }
- return ret;
+err_adapter:
- i2c_del_driver(&wm8903_i2c_driver);
+err_priv:
- kfree(codec->private_data);
+err_codec:
- kfree(codec);
- return ret;
+}
At Tue, 26 Aug 2008 15:28:56 +0200, Jean Delvare wrote:
Hi Mark,
On Tue, 26 Aug 2008 13:05:27 +0100, Mark Brown wrote:
The WM8903 is a high performance ultra-low power stereo CODEC optimised for portable audio applications. Features include:
* 5mW power consumption for DAC to headphone playback * Stereo DAC SNR 96dB typical, THD -86dB typical * Stereo ADC SNR 93dB typical, THD -80dB typical * Up to 3 single ended inputs per stereo channel * Up to 2 pseudo differential inputs per stereo channel * Up to 1 fully differential mic input per stereo channel * Digital Dynamic Range Controller (compressor/limiter) * Digital sidetone mixingSigned-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wm8903.c | 1813 +++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/wm8903.h | 1463 ++++++++++++++++++++++++++++++++++++ 4 files changed, 3282 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/wm8903.c create mode 100644 sound/soc/codecs/wm8903.h
Here are minor issues I spotted in the i2c part of your driver:
(...) +static int wm8903_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)+{
- struct snd_soc_device *socdev = wm8903_socdev;
- struct snd_soc_codec *codec = socdev->codec;
- int ret;
- i2c_set_clientdata(i2c, codec);
- codec->control_data = i2c;
- ret = wm8903_init(socdev);
- if (ret < 0)
dev_err(&i2c->dev, "Device initialisation failed\n");- return ret;
+}
+static int wm8903_i2c_remove(struct i2c_client *client) +{
- struct snd_soc_codec *codec = i2c_get_clientdata(client);
- kfree(codec->reg_cache);
- return 0;
+}
+/* i2c codec control layer */ +static const struct i2c_device_id wm8903_i2c_id[] = {
{ "wm8903", 0 },{ }+}; +MODULE_DEVICE_TABLE(i2c, wm8903_i2c_id);
+static struct i2c_driver wm8903_i2c_driver = {
- .driver = {
.name = "WM8903",.owner = THIS_MODULE,- },
- .probe = wm8903_i2c_probe,
- .remove = wm8903_i2c_remove,
- .id_table = wm8903_i2c_id,
+};
+static struct i2c_client *wm8903_i2c_device;
I know that I am the one who came up with this idea, but on second thought, wm8903_i2c_device will always be equal to wm8903_socdev->codec->control_data, so we can easily do without it.
Indeed. It's a good cleanup.
+static int wm8903_probe(struct platform_device *pdev) +{
- struct snd_soc_device *socdev = platform_get_drvdata(pdev);
- struct wm8903_setup_data *setup;
- struct snd_soc_codec *codec;
- struct wm8903_priv *wm8903;
- struct i2c_board_info board_info;
- struct i2c_adapter *adapter;
- int ret = 0;
- setup = socdev->codec_data;
- if (!setup->i2c_address) {
dev_err(&pdev->dev, "No codec address provided\n");return -ENODEV;- }
- codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
- if (codec == NULL)
return -ENOMEM;- wm8903 = kzalloc(sizeof(struct wm8903_priv), GFP_KERNEL);
- if (wm8903 == NULL) {
ret = -ENOMEM;goto err_codec;- }
- codec->private_data = wm8903;
- socdev->codec = codec;
- mutex_init(&codec->mutex);
- INIT_LIST_HEAD(&codec->dapm_widgets);
- INIT_LIST_HEAD(&codec->dapm_paths);
- wm8903_socdev = socdev;
- codec->hw_write = (hw_write_t)i2c_master_send;
- ret = i2c_add_driver(&wm8903_i2c_driver);
- if (ret != 0) {
dev_err(&pdev->dev, "can't add i2c driver");Missing \n at end of string (looks like many other drivers have this problem - I found 11 other cases under sound/soc, I can send a patch fixing these if you want.)
It'd be appreciated. If Mark didn't work on it yet, I'll merge them.
goto err_priv;- } else {
memset(&board_info, 0, sizeof(board_info));strlcpy(board_info.type, "wm8903", I2C_NAME_SIZE);board_info.addr = setup->i2c_address;adapter = i2c_get_adapter(setup->i2c_bus);if (!adapter) {dev_err(&pdev->dev, "Can't get I2C bus %d\n",setup->i2c_bus);goto err_adapter;You are jumping to the error path but ret has value 0. This means that the function returns success when you really mean failure, and trouble is likely to arise after that.
Good catch.
Mark, could you post a fix patch? The original patch was already merged, so I'd like to apply on it.
thanks,
Takashi
Hi Takashi,
On Tue, 26 Aug 2008 15:37:30 +0200, Takashi Iwai wrote:
At Tue, 26 Aug 2008 15:28:56 +0200, Jean Delvare wrote:
On Tue, 26 Aug 2008 13:05:27 +0100, Mark Brown wrote:
- if (ret != 0) {
dev_err(&pdev->dev, "can't add i2c driver");Missing \n at end of string (looks like many other drivers have this problem - I found 11 other cases under sound/soc, I can send a patch fixing these if you want.)
It'd be appreciated. If Mark didn't work on it yet, I'll merge them.
Incidentally, I am working on converting almost all the affected drivers to the new i2c device driver binding model, and my conversion changes that part of the code. So in order to avoid useless conflicts, I'd rather not touch that part of the code in a separate patch. My conversion patches will fix the strings as a side effect anyway.
I'll send a patch with 2 fixes affecting the drivers I am not going to touch, so that these fixes aren't lost.
On Tue, Aug 26, 2008 at 03:28:56PM +0200, Jean Delvare wrote:
On Tue, 26 Aug 2008 13:05:27 +0100, Mark Brown wrote:
+static struct i2c_client *wm8903_i2c_device;
I know that I am the one who came up with this idea, but on second thought, wm8903_i2c_device will always be equal to wm8903_socdev->codec->control_data, so we can easily do without it.
That'll teach me to listen to subsystem maintainers :) I'll send a patch either later today or tomorrow.
- ret = i2c_add_driver(&wm8903_i2c_driver);
- if (ret != 0) {
dev_err(&pdev->dev, "can't add i2c driver");
Missing \n at end of string (looks like many other drivers have this problem - I found 11 other cases under sound/soc, I can send a patch fixing these if you want.)
That'd be great if you could.
board_info.addr = setup->i2c_address;adapter = i2c_get_adapter(setup->i2c_bus);if (!adapter) {dev_err(&pdev->dev, "Can't get I2C bus %d\n",setup->i2c_bus);goto err_adapter;
You are jumping to the error path but ret has value 0. This means that the function returns success when you really mean failure, and trouble is likely to arise after that.
Yeah, I'll fix this too - thanks.
participants (3)
-
Jean Delvare -
Mark Brown -
Takashi Iwai