[alsa-devel] [PATCH] ASoC: Add WM8903 CODEC driver

[Takashi Iwai]

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 at 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