Prev: [PATCH] I2C driver of Topcliff PCH
Next: [PATCH v2 1/3] timekeeping: moving xtime's init to a later time
From: Guennadi Liakhovetski on 15 Jul 2010 04:00 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 15 Jul 2010 05:00
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/ |