Hi Alexander,
I can't comment on the codec side of things but a couple of general comments
inline. Otherwise looks good to me!
Jamie
On Wed, Dec 08, 2010 at 03:02:52PM +0300, Alexander wrote:
> From: Alexander Sverdlin <subaparts@xxxxxxxxx>
>
> Added support for CS4271 codec to ASoC.
>
> Signed-off-by: Alexander Sverdlin <subaparts@xxxxxxxxx>
> ---
> include/sound/cs4271.h | 25 ++
> sound/soc/codecs/Kconfig | 4 +
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/cs4271.c | 607 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 638 insertions(+), 0 deletions(-)
>
[...]
> diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
> new file mode 100644
> index 0000000..c0d74da
> --- /dev/null
> +++ b/sound/soc/codecs/cs4271.c
> @@ -0,0 +1,607 @@
[...]
> +struct cs4271_private {
> + /* SND_SOC_I2C or SND_SOC_SPI */
> + enum snd_soc_control_type bus_type;
> + void *control_data;
> + unsigned int mclk;
> + /* SND_SOC_DAIFMT_I2S or SND_SOC_DAIFMT_LEFT_J */
> + unsigned int mode;
> + unsigned int master;
> + /* GPIO driving Reset pin, if any */
> + int gpio_nreset;
> + /* GPIO that disable serial bus, if any */
> + int gpio_disable;
Align gpio_nreset and gpio_disable with the other fields?
[...]
> +#if defined(CONFIG_SPI_MASTER)
> +static int __devinit cs4271_spi_probe(struct spi_device *spi)
> +{
> + struct cs4271_private *cs4271;
> + int ret;
> +
> + cs4271 = kzalloc(sizeof(*cs4271), GFP_KERNEL);
> + if (!cs4271)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, cs4271);
> + cs4271->control_data = spi;
> + cs4271->bus_type = SND_SOC_SPI;
> +
> + ret = snd_soc_register_codec(&spi->dev, &soc_codec_dev_cs4271,
> + &cs4271_dai, 1);
> + if (ret < 0)
> + kfree(cs4271);
> + return ret;
> +}
> +
> +static int __devexit cs4271_spi_remove(struct spi_device *spi)
> +{
> + snd_soc_unregister_codec(&spi->dev);
> + kfree(spi_get_drvdata(spi));
> + return 0;
> +}
Is it worth using devm_kzalloc() for the i2c and spi probe functions to
simplify the error handling and release functions? Admittedly they are fairly
simple functions though.
[...]
> +/*
> + * We only register our serial bus driver here without
> + * assignment to particular chip. So if any of the below
> + * fails, there is some problem with I2C or SPI subsystem.
> + * In most cases this module will be compiled with support
> + * of only one serial bus.
> + */
> +static int __init cs4271_modinit(void)
> +{
> + int ret;
> +
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> + ret = i2c_add_driver(&cs4271_i2c_driver);
> + if (ret) {
> + pr_err("Failed to register CS4271 I2C driver: %d\n", ret);
> + return ret;
> + }
> +#endif
> +
> +#if defined(CONFIG_SPI_MASTER)
> + ret = spi_register_driver(&cs4271_spi_driver);
> + if (ret) {
> + pr_err("Failed to register CS4271 SPI driver: %d\n", ret);
> + return ret;
> + }
> +#endif
> +
> + return 0;
> +}
> +module_init(cs4271_modinit);
> +
> +static void __exit cs4271_modexit(void)
> +{
> +#if defined(CONFIG_SPI_MASTER)
> + spi_unregister_driver(&cs4271_spi_driver);
> +#endif
> +
> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> + i2c_del_driver(&cs4271_i2c_driver);
> +#endif
> +}
> +module_exit(cs4271_modexit);
Doesn't the dependency on SND_SOC_I2C_AND_SPI mean that you will always have
I2C and SPI_MASTER?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
