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 mixing
> >
> > Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
