Message-ID: <1279184077.3072.21.camel@odin>
Date:	Thu, 15 Jul 2010 09:54:37 +0100
From:	Liam Girdwood <lrg@...mlogic.co.uk>
To:	axel.lin@...il.com
Cc:	Guennadi Liakhovetski <g.liakhovetski@....de>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	Tejun Heo <tj@...nel.org>, alsa-devel@...a-project.org
Subject: Re: [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register 
 error path

On Thu, 2010-07-15 at 15:42 +0800, Axel Lin wrote:

> 
> I agree that it is good to release resources at the same level,
> where they have been acquired.
> But I prefer to follow the maintainer/author's coding style.
> 
> For the changes in other drivers, I'd like to see Liam and Mark's comments.
> Or if the driver maintainers request it, I can fix it.

Fwiw, the soon to be merged ASoC multi-component branch simplifies the
codec probe() and remove() as follows (e.g. from I2C and SPI WM8750
codec) :-

static int wm8750_probe(struct snd_soc_codec *codec)
{
	struct wm8750_priv *wm8750 = snd_soc_codec_get_drvdata(codec);
	int reg, ret;

	codec->control_data = wm8750->control_data;
	ret = snd_soc_codec_set_cache_io(codec, 7, 9, wm8750->control_type);
	if (ret < 0) {
		printk(KERN_ERR "wm8750: failed to set cache I/O: %d\n", ret);
		return ret;
	}

	ret = wm8750_reset(codec);
	if (ret < 0) {
		printk(KERN_ERR "wm8750: failed to reset: %d\n", ret);
		return ret;
	}

	/* charge output caps */
	wm8750_set_bias_level(codec, SND_SOC_BIAS_STANDBY);



	snd_soc_add_controls(codec, wm8750_snd_controls,
				ARRAY_SIZE(wm8750_snd_controls));
	wm8750_add_widgets(codec);
	return ret;
}

static int wm8750_remove(struct snd_soc_codec *codec)
{
	wm8750_set_bias_level(codec, SND_SOC_BIAS_OFF);
	return 0;
}


#if defined(CONFIG_SPI_MASTER)
static int __devinit wm8750_spi_probe(struct spi_device *spi)
{
	struct wm8750_priv *wm8750;
	int ret;

	wm8750 = kzalloc(sizeof(struct wm8750_priv), GFP_KERNEL);
	if (wm8750 == NULL)
		return -ENOMEM;

	wm8750->control_data = spi;
	wm8750->control_type = SND_SOC_SPI;
	spi_set_drvdata(spi, wm8750);

	ret = snd_soc_register_codec(&spi->dev, spi->chip_select,
			&soc_codec_dev_wm8750, &wm8750_dai, 1);
	if (ret < 0)
		kfree(wm8750);
	return ret;
}

static int __devexit wm8750_spi_remove(struct spi_device *spi)
{
	snd_soc_unregister_codec(&spi->dev, spi->chip_select);
	kfree(spi_get_drvdata(spi));
	return 0;
}

static struct spi_driver wm8750_spi_driver = {
	.driver = {
		.name	= "wm8750 SPI Codec",
		.bus	= &spi_bus_type,
		.owner	= THIS_MODULE,
	},
	.probe		= wm8750_spi_probe,
	.remove		= __devexit_p(wm8750_spi_remove),
};
#endif /* CONFIG_SPI_MASTER */

#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
static __devinit int wm8750_i2c_probe(struct i2c_client *i2c,
				      const struct i2c_device_id *id)
{
	struct wm8750_priv *wm8750;
	int ret;

	wm8750 = kzalloc(sizeof(struct wm8750_priv), GFP_KERNEL);
	if (wm8750 == NULL)
		return -ENOMEM;

	i2c_set_clientdata(i2c, wm8750);
	wm8750->control_data = i2c;
	wm8750->control_type = SND_SOC_I2C;

	ret =  snd_soc_register_codec(&i2c->dev, i2c->addr,
			&soc_codec_dev_wm8750, &wm8750_dai, 1);
	if (ret < 0)
		kfree(wm8750);
	return ret;
}

static __devexit int wm8750_i2c_remove(struct i2c_client *client)
{
	snd_soc_unregister_codec(&client->dev, client->addr);
	kfree(i2c_get_clientdata(client));
	return 0;
}

static const struct i2c_device_id wm8750_i2c_id[] = {
	{ "wm8750", 0 },
	{ "wm8987", 0 },
	{ }
};
MODULE_DEVICE_TABLE(i2c, wm8750_i2c_id);

static struct i2c_driver wm8750_i2c_driver = {
	.driver = {
		.name = "wm8750 I2C Codec",
		.owner = THIS_MODULE,
	},
	.probe =    wm8750_i2c_probe,
	.remove =   __devexit_p(wm8750_i2c_remove),
	.id_table = wm8750_i2c_id,
};
#endif

static int __init wm8750_modinit(void)
{
	int ret = 0;
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
	ret = i2c_add_driver(&wm8750_i2c_driver);
	if (ret != 0) {
		printk(KERN_ERR "Failed to register wm8750 I2C driver: %d\n",
		       ret);
	}
#endif
#if defined(CONFIG_SPI_MASTER)
	ret = spi_register_driver(&wm8750_spi_driver);
	if (ret != 0) {
		printk(KERN_ERR "Failed to register wm8750 SPI driver: %d\n",
		       ret);
	}
#endif
	return ret;
}
module_init(wm8750_modinit);

static void __exit wm8750_exit(void)
{
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
	i2c_del_driver(&wm8750_i2c_driver);
#endif
#if defined(CONFIG_SPI_MASTER)
	spi_unregister_driver(&wm8750_spi_driver);
#endif
}
module_exit(wm8750_exit);


So while this patch series is useful (from a minor bug fix pov), it will
be overwritten shortly in the ASoC multi-component merge. Probably
delaying the multi-component merge too (since the changes are in the
same place).

Can we hold off this series for a week or two. I have one more update
for multi-component and if we don't have multi-component upstreamed in
that time frame we can apply this series.

Thanks

Liam 
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists