From: Guennadi Liakhovetski on
On Thu, 15 Jul 2010, 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.

Yeah, unless that "style" is a bug. IMHO this design is just buggy,
unmaintainable, error-prone, and unjustified.

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

Well, as the author of sound/soc/codecs/wm8978.c, I'd be happy if you
change it in this way;) And also move the

kfree(wm8978);

line from wm8978_unregister() to wm8978_i2c_remove().

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Liam Girdwood on
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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/