[alsa-devel] [RFC PATCH 0/1] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec
The following patch is an introduction of a codec driver for the TI DSD1791 audio part.
This patch adds basic support and has been tested using a MityDSP-L138F SOM sitting on a Critical Link Industrial I/O evaluation board in 16 bit and 24 bit I2S mode.
This is an RFC, as I'm still wandering around the ALSA framework a bit. Comments and guidance greatly appreciated. I can follow with control registers (gain, etc.) if this stuff looks OK. Thanks.
-Mike
Michael Williamson (1): ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec
sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/dsd1791.c | 282 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/dsd1791.c
This patch introduces a (spi) codec driver for the Texas Instruments DSD1791 24 bit audio stereo DAC.
Testing for basic operation using 16 and 24 bit I2S mode has been performed.
http://www.ti.com/product/dsd1791
Signed-off-by: Michael Williamson michael.williamson@criticallink.com --- sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/dsd1791.c | 282 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/dsd1791.c
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 4584514..95b7969 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -33,6 +33,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_CX20442 select SND_SOC_DA7210 if I2C select SND_SOC_DFBMCS320 + select SND_SOC_DSD1791 if SPI_MASTER select SND_SOC_JZ4740_CODEC if SOC_JZ4740 select SND_SOC_LM4857 if I2C select SND_SOC_MAX98088 if I2C @@ -205,6 +206,9 @@ config SND_SOC_DA7210 config SND_SOC_DFBMCS320 tristate
+config SND_SOC_DSD1791 + tristate + config SND_SOC_DMIC tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index a2c7842..d6b5f6a 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -21,6 +21,7 @@ snd-soc-cx20442-objs := cx20442.o snd-soc-da7210-objs := da7210.o snd-soc-dfbmcs320-objs := dfbmcs320.o snd-soc-dmic-objs := dmic.o +snd-soc-dsd1791-objs := dsd1791.o snd-soc-l3-objs := l3.o snd-soc-max98088-objs := max98088.o snd-soc-max98095-objs := max98095.o @@ -120,6 +121,7 @@ obj-$(CONFIG_SND_SOC_CS4271) += snd-soc-cs4271.o obj-$(CONFIG_SND_SOC_CX20442) += snd-soc-cx20442.o obj-$(CONFIG_SND_SOC_DA7210) += snd-soc-da7210.o obj-$(CONFIG_SND_SOC_DFBMCS320) += snd-soc-dfbmcs320.o +obj-$(CONFIG_SND_SOC_DSD1791) += snd-soc-dsd1791.o obj-$(CONFIG_SND_SOC_DMIC) += snd-soc-dmic.o obj-$(CONFIG_SND_SOC_L3) += snd-soc-l3.o obj-$(CONFIG_SND_SOC_JZ4740_CODEC) += snd-soc-jz4740-codec.o diff --git a/sound/soc/codecs/dsd1791.c b/sound/soc/codecs/dsd1791.c new file mode 100644 index 0000000..dbba71b --- /dev/null +++ b/sound/soc/codecs/dsd1791.c @@ -0,0 +1,282 @@ +/* + * ALSA SoC codec driver for Texas Instruments DSD1791. + * + * Author: (C) Michael Williamson michael.williamson@criticallink.com + * Copyright: (C) 2011 Critical Link, LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/pm.h> +#include <linux/spi/spi.h> +#include <linux/platform_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h> +#include <sound/initval.h> +#include <sound/tlv.h> + +#define DSD1791_REG_DIGATT_L 16 +#define DSD1791_REG_DIGATT_R 17 +#define DSD1791_REG_AUDFMT 18 + +#define DSD1791_FMT_16RJ (0<<4) +#define DSD1791_FMT_20RJ (1<<4) +#define DSD1791_FMT_24RJ (2<<4) +#define DSD1791_FMT_24LJ (3<<4) +#define DSD1791_FMT_16I2S (4<<4) +#define DSD1791_FMT_24I2S (5<<4) + +#define DSD1971_FORMAT_S16_LE 0 +#define DSD1971_FORMAT_S24_LE 1 + +#define DSD1791_DAIFMT_I2S 0 +#define DSD1791_DAIFMT_RIGHT_J 1 +#define DSD1791_DAIFMT_LEFT_J 2 + +struct dsd1791 { + struct spi_device *spi; + struct snd_soc_codec codec; + int mclk; + int dai_fmt; + int pcm_fmt; +}; + +/* + * write to the dsd1791 register space + */ +static int dsd1791_write(struct snd_soc_codec *codec, unsigned int reg, + unsigned int value) +{ + struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec); + u8 buffer[2]; + int rc; + + buffer[0] = (reg & 0x7F); + buffer[1] = value & 0xFF; + rc = spi_write(dsd1791->spi, buffer, 2); + if (rc) { + dev_err(&dsd1791->spi->dev, "DSD1791 reg write error (%d)\n", + rc); + return -EIO; + } + return 0; +} + +/* + * read from the dsd1791 register space + */ +static unsigned int dsd1791_read(struct snd_soc_codec *codec, + unsigned int reg) +{ + /* + * DSD1791 read capablity requires multipurposing the ZEROL signal + * from the codec. This is a non-standard mode and is not supported + * on any available hardware for testing. For now, don't allow it. + */ + return -EIO; +} + +static int dsd1791_set_format_word(struct dsd1791 *dsd1791, + struct snd_soc_codec *codec) +{ + u8 fmt = 0; + switch (dsd1791->dai_fmt) { + + case DSD1791_DAIFMT_I2S: + if (dsd1791->pcm_fmt == DSD1971_FORMAT_S16_LE) + fmt = DSD1791_FMT_16I2S; + else if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE) + fmt = DSD1791_FMT_24I2S; + else + return -EINVAL; + break; + + case DSD1791_DAIFMT_RIGHT_J: + if (dsd1791->pcm_fmt == DSD1971_FORMAT_S16_LE) + fmt = DSD1791_FMT_16RJ; + else if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE) + fmt = DSD1791_FMT_24RJ; + else + return -EINVAL; + break; + + case DSD1791_DAIFMT_LEFT_J: + if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE) + fmt = DSD1791_FMT_24LJ; + else + return -EINVAL; + break; + + default: + return -EINVAL; + } + return dsd1791_write(codec, DSD1791_REG_AUDFMT, fmt); +} + +static int dsd1791_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_codec *codec = rtd->codec; + struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec); + + switch (params_format(params)) { + + case SNDRV_PCM_FORMAT_S16_LE: + dsd1791->pcm_fmt = DSD1971_FORMAT_S16_LE; + break; + + case SNDRV_PCM_FORMAT_S24_LE: + dsd1791->pcm_fmt = DSD1971_FORMAT_S24_LE; + break; + default: + dev_dbg(&dsd1791->spi->dev, "bad format\n"); + return -EINVAL; + } + + return dsd1791_set_format_word(dsd1791, codec); +} + +static int dsd1791_set_sysclk(struct snd_soc_dai *codec_dai, + int clk_id, unsigned int freq, int dir) +{ + struct snd_soc_codec *codec = codec_dai->codec; + struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec); + dsd1791->mclk = freq; + return 0; +} + +static int dsd1791_set_fmt(struct snd_soc_dai *codec_dai, + unsigned int fmt) +{ + struct snd_soc_codec *codec = codec_dai->codec; + struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec); + + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_I2S: + dsd1791->dai_fmt = DSD1791_DAIFMT_I2S; + break; + case SND_SOC_DAIFMT_RIGHT_J: + dsd1791->dai_fmt = DSD1791_DAIFMT_RIGHT_J; + break; + case SND_SOC_DAIFMT_LEFT_J: + dsd1791->dai_fmt = DSD1791_DAIFMT_LEFT_J; + break; + default: + dev_dbg(&dsd1791->spi->dev, "bad format\n"); + return -EINVAL; + } + + return dsd1791_set_format_word(dsd1791, codec); +} + +#define DSD1791_RATES SNDRV_PCM_RATE_8000_192000 +#define DSD1791_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\ + SNDRV_PCM_FMTBIT_S24_LE) + +static struct snd_soc_dai_ops dsd1791_dai_ops = { + .hw_params = dsd1791_hw_params, + .set_sysclk = dsd1791_set_sysclk, + .set_fmt = dsd1791_set_fmt, +}; + +struct snd_soc_dai_driver dsd1791_dai = { + .name = "dsd1791", + .playback = { + .stream_name = "Playback", + .channels_min = 2, + .channels_max = 2, + .rates = DSD1791_RATES, + .formats = DSD1791_FORMATS, + }, + .ops = &dsd1791_dai_ops, +}; + +static const struct snd_kcontrol_new dsd1791_snd_controls[] = { +}; + +static int dsd1791_probe(struct snd_soc_codec *codec) +{ + int err; + + err = snd_soc_add_controls(codec, dsd1791_snd_controls, + ARRAY_SIZE(dsd1791_snd_controls)); + WARN_ON(err < 0); + + return 0; +} + +struct snd_soc_codec_driver dsd1791_soc_codec_dev = { + .probe = dsd1791_probe, + .read = dsd1791_read, + .write = dsd1791_write, + .reg_word_size = sizeof(u8), +}; + +static int dsd1791_spi_probe(struct spi_device *spi) +{ + struct dsd1791 *dsd1791; + int ret; + + dev_info(&spi->dev, "probing dsd1791 spi device\n"); + + dsd1791 = kzalloc(sizeof *dsd1791, GFP_KERNEL); + if (!dsd1791) + return -ENOMEM; + + dsd1791->spi = spi; + dev_set_drvdata(&spi->dev, dsd1791); + + ret = snd_soc_register_codec(&spi->dev, + &dsd1791_soc_codec_dev, &dsd1791_dai, 1); + + if (ret < 0) { + kfree(dsd1791); + dev_warn(&spi->dev, "Unable to register codec (%d)\n", ret); + } else + dev_info(&spi->dev, "SPI device initialized\n"); + return 0; +}; + +static int dsd1791_spi_remove(struct spi_device *spi) +{ + snd_soc_unregister_codec(&spi->dev); + kfree(spi_get_drvdata(spi)); + return 0; +} + +static struct spi_driver dsd1791_spi = { + .driver = { + .name = "dsd1791-codec", + .owner = THIS_MODULE, + }, + .probe = dsd1791_spi_probe, + .remove = dsd1791_spi_remove, +}; + +static int __init dsd1791_init(void) +{ + spi_register_driver(&dsd1791_spi); + + return 0; +} +module_init(dsd1791_init); + +static void __exit dsd1791_exit(void) +{ + spi_unregister_driver(&dsd1791_spi); +} +module_exit(dsd1791_exit); + +MODULE_DESCRIPTION("ASoC DSD1791 codec driver"); +MODULE_AUTHOR("Michael Williamson"); +MODULE_LICENSE("GPL");
On Thu, Dec 15, 2011 at 01:40, Michael Williamson michael.williamson@criticallink.com wrote:
This patch introduces a (spi) codec driver for the Texas Instruments DSD1791 24 bit audio stereo DAC.
Testing for basic operation using 16 and 24 bit I2S mode has been performed.
http://www.ti.com/product/dsd1791
Signed-off-by: Michael Williamson michael.williamson@criticallink.com
sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/dsd1791.c | 282 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 0 deletions(-) create mode 100644 sound/soc/codecs/dsd1791.c
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 4584514..95b7969 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -33,6 +33,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_CX20442 select SND_SOC_DA7210 if I2C select SND_SOC_DFBMCS320
- select SND_SOC_DSD1791 if SPI_MASTER
select SND_SOC_JZ4740_CODEC if SOC_JZ4740 select SND_SOC_LM4857 if I2C select SND_SOC_MAX98088 if I2C @@ -205,6 +206,9 @@ config SND_SOC_DA7210 config SND_SOC_DFBMCS320 tristate
+config SND_SOC_DSD1791
- tristate
config SND_SOC_DMIC tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index a2c7842..d6b5f6a 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -21,6 +21,7 @@ snd-soc-cx20442-objs := cx20442.o snd-soc-da7210-objs := da7210.o snd-soc-dfbmcs320-objs := dfbmcs320.o snd-soc-dmic-objs := dmic.o +snd-soc-dsd1791-objs := dsd1791.o snd-soc-l3-objs := l3.o snd-soc-max98088-objs := max98088.o snd-soc-max98095-objs := max98095.o @@ -120,6 +121,7 @@ obj-$(CONFIG_SND_SOC_CS4271) += snd-soc-cs4271.o obj-$(CONFIG_SND_SOC_CX20442) += snd-soc-cx20442.o obj-$(CONFIG_SND_SOC_DA7210) += snd-soc-da7210.o obj-$(CONFIG_SND_SOC_DFBMCS320) += snd-soc-dfbmcs320.o +obj-$(CONFIG_SND_SOC_DSD1791) += snd-soc-dsd1791.o obj-$(CONFIG_SND_SOC_DMIC) += snd-soc-dmic.o obj-$(CONFIG_SND_SOC_L3) += snd-soc-l3.o obj-$(CONFIG_SND_SOC_JZ4740_CODEC) += snd-soc-jz4740-codec.o diff --git a/sound/soc/codecs/dsd1791.c b/sound/soc/codecs/dsd1791.c new file mode 100644 index 0000000..dbba71b --- /dev/null +++ b/sound/soc/codecs/dsd1791.c @@ -0,0 +1,282 @@ +/*
- ALSA SoC codec driver for Texas Instruments DSD1791.
- Author: (C) Michael Williamson michael.williamson@criticallink.com
- Copyright: (C) 2011 Critical Link, LLC
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- */
+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/init.h> +#include <linux/delay.h> +#include <linux/pm.h> +#include <linux/spi/spi.h> +#include <linux/platform_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/soc-dapm.h>
Are you sure that you need this header file ?
+#include <sound/initval.h> +#include <sound/tlv.h>
+#define DSD1791_REG_DIGATT_L 16 +#define DSD1791_REG_DIGATT_R 17 +#define DSD1791_REG_AUDFMT 18
+#define DSD1791_FMT_16RJ (0<<4) +#define DSD1791_FMT_20RJ (1<<4) +#define DSD1791_FMT_24RJ (2<<4) +#define DSD1791_FMT_24LJ (3<<4) +#define DSD1791_FMT_16I2S (4<<4) +#define DSD1791_FMT_24I2S (5<<4)
+#define DSD1971_FORMAT_S16_LE 0 +#define DSD1971_FORMAT_S24_LE 1
+#define DSD1791_DAIFMT_I2S 0 +#define DSD1791_DAIFMT_RIGHT_J 1 +#define DSD1791_DAIFMT_LEFT_J 2
+struct dsd1791 {
- struct spi_device *spi;
- struct snd_soc_codec codec;
- int mclk;
- int dai_fmt;
- int pcm_fmt;
+};
+/*
- write to the dsd1791 register space
- */
+static int dsd1791_write(struct snd_soc_codec *codec, unsigned int reg,
- unsigned int value)
+{
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- u8 buffer[2];
- int rc;
- buffer[0] = (reg & 0x7F);
- buffer[1] = value & 0xFF;
- rc = spi_write(dsd1791->spi, buffer, 2);
- if (rc) {
- dev_err(&dsd1791->spi->dev, "DSD1791 reg write error (%d)\n",
- rc);
- return -EIO;
- }
- return 0;
+}
+/*
- read from the dsd1791 register space
- */
+static unsigned int dsd1791_read(struct snd_soc_codec *codec,
- unsigned int reg)
+{
- /*
- * DSD1791 read capablity requires multipurposing the ZEROL signal
- * from the codec. This is a non-standard mode and is not supported
- * on any available hardware for testing. For now, don't allow it.
- */
- return -EIO;
+}
+static int dsd1791_set_format_word(struct dsd1791 *dsd1791,
- struct snd_soc_codec *codec)
+{
- u8 fmt = 0;
- switch (dsd1791->dai_fmt) {
- case DSD1791_DAIFMT_I2S:
- if (dsd1791->pcm_fmt == DSD1971_FORMAT_S16_LE)
- fmt = DSD1791_FMT_16I2S;
- else if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE)
- fmt = DSD1791_FMT_24I2S;
- else
- return -EINVAL;
- break;
- case DSD1791_DAIFMT_RIGHT_J:
- if (dsd1791->pcm_fmt == DSD1971_FORMAT_S16_LE)
- fmt = DSD1791_FMT_16RJ;
- else if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE)
- fmt = DSD1791_FMT_24RJ;
- else
- return -EINVAL;
- break;
- case DSD1791_DAIFMT_LEFT_J:
- if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE)
- fmt = DSD1791_FMT_24LJ;
- else
- return -EINVAL;
- break;
- default:
- return -EINVAL;
- }
- return dsd1791_write(codec, DSD1791_REG_AUDFMT, fmt);
+}
+static int dsd1791_hw_params(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params *params,
- struct snd_soc_dai *dai)
+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_codec *codec = rtd->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- switch (params_format(params)) {
- case SNDRV_PCM_FORMAT_S16_LE:
- dsd1791->pcm_fmt = DSD1971_FORMAT_S16_LE;
- break;
- case SNDRV_PCM_FORMAT_S24_LE:
- dsd1791->pcm_fmt = DSD1971_FORMAT_S24_LE;
- break;
- default:
- dev_dbg(&dsd1791->spi->dev, "bad format\n");
- return -EINVAL;
- }
- return dsd1791_set_format_word(dsd1791, codec);
+}
+static int dsd1791_set_sysclk(struct snd_soc_dai *codec_dai,
- int clk_id, unsigned int freq, int dir)
+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- dsd1791->mclk = freq;
- return 0;
+}
+static int dsd1791_set_fmt(struct snd_soc_dai *codec_dai,
- unsigned int fmt)
+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
- case SND_SOC_DAIFMT_I2S:
- dsd1791->dai_fmt = DSD1791_DAIFMT_I2S;
- break;
- case SND_SOC_DAIFMT_RIGHT_J:
- dsd1791->dai_fmt = DSD1791_DAIFMT_RIGHT_J;
- break;
- case SND_SOC_DAIFMT_LEFT_J:
- dsd1791->dai_fmt = DSD1791_DAIFMT_LEFT_J;
- break;
- default:
- dev_dbg(&dsd1791->spi->dev, "bad format\n");
- return -EINVAL;
- }
- return dsd1791_set_format_word(dsd1791, codec);
+}
+#define DSD1791_RATES SNDRV_PCM_RATE_8000_192000 +#define DSD1791_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
- SNDRV_PCM_FMTBIT_S24_LE)
+static struct snd_soc_dai_ops dsd1791_dai_ops = {
- .hw_params = dsd1791_hw_params,
- .set_sysclk = dsd1791_set_sysclk,
- .set_fmt = dsd1791_set_fmt,
+};
+struct snd_soc_dai_driver dsd1791_dai = {
- .name = "dsd1791",
- .playback = {
- .stream_name = "Playback",
- .channels_min = 2,
- .channels_max = 2,
- .rates = DSD1791_RATES,
- .formats = DSD1791_FORMATS,
- },
- .ops = &dsd1791_dai_ops,
+};
+static const struct snd_kcontrol_new dsd1791_snd_controls[] = { +};
+static int dsd1791_probe(struct snd_soc_codec *codec) +{
- int err;
- err = snd_soc_add_controls(codec, dsd1791_snd_controls,
- ARRAY_SIZE(dsd1791_snd_controls));
It doesn't make sense, because dsd1791_snd_controls is empty.
- WARN_ON(err < 0);
- return 0;
+}
+struct snd_soc_codec_driver dsd1791_soc_codec_dev = {
- .probe = dsd1791_probe,
- .read = dsd1791_read,
- .write = dsd1791_write,
- .reg_word_size = sizeof(u8),
+};
+static int dsd1791_spi_probe(struct spi_device *spi) +{
- struct dsd1791 *dsd1791;
- int ret;
- dev_info(&spi->dev, "probing dsd1791 spi device\n");
- dsd1791 = kzalloc(sizeof *dsd1791, GFP_KERNEL);
Better to use devm_kzalloc.
- if (!dsd1791)
- return -ENOMEM;
- dsd1791->spi = spi;
- dev_set_drvdata(&spi->dev, dsd1791);
- ret = snd_soc_register_codec(&spi->dev,
- &dsd1791_soc_codec_dev, &dsd1791_dai, 1);
- if (ret < 0) {
- kfree(dsd1791);
If you will use devm_kzalloc, you won't need to free memory.
- dev_warn(&spi->dev, "Unable to register codec (%d)\n", ret);
- } else
- dev_info(&spi->dev, "SPI device initialized\n");
- return 0;
+};
+static int dsd1791_spi_remove(struct spi_device *spi) +{
- snd_soc_unregister_codec(&spi->dev);
- kfree(spi_get_drvdata(spi));
devm_kzalloc
- return 0;
+}
+static struct spi_driver dsd1791_spi = {
- .driver = {
- .name = "dsd1791-codec",
- .owner = THIS_MODULE,
- },
- .probe = dsd1791_spi_probe,
- .remove = dsd1791_spi_remove,
+};
+static int __init dsd1791_init(void) +{
- spi_register_driver(&dsd1791_spi);
- return 0;
+} +module_init(dsd1791_init);
+static void __exit dsd1791_exit(void) +{
- spi_unregister_driver(&dsd1791_spi);
+} +module_exit(dsd1791_exit);
+MODULE_DESCRIPTION("ASoC DSD1791 codec driver"); +MODULE_AUTHOR("Michael Williamson");
+MODULE_LICENSE("GPL");
1.7.5.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
-- Leon Romanovsky | Independent Linux Consultant www.leon.nu | leon@leon.nu
On Wed, Dec 14, 2011 at 06:40:43PM -0500, Michael Williamson wrote:
+static int dsd1791_write(struct snd_soc_codec *codec, unsigned int reg,
unsigned int value)+{
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- u8 buffer[2];
- int rc;
- buffer[0] = (reg & 0x7F);
- buffer[1] = value & 0xFF;
- rc = spi_write(dsd1791->spi, buffer, 2);
- if (rc) {
dev_err(&dsd1791->spi->dev, "DSD1791 reg write error (%d)\n",rc);return -EIO;- }
- return 0;
You shouldn't be open coding register I/O - this looks like a basic 8x8 register map so should work just fine with regmap (or the ASoC stuff but new drivers should really use regmap).
- case DSD1791_DAIFMT_I2S:
if (dsd1791->pcm_fmt == DSD1971_FORMAT_S16_LE)fmt = DSD1791_FMT_16I2S;else if (dsd1791->pcm_fmt == DSD1971_FORMAT_S24_LE)fmt = DSD1791_FMT_24I2S;elsereturn -EINVAL;
This should be a switch statement. You've got quite a few instances of this pattern.
- default:
dev_dbg(&dsd1791->spi->dev, "bad format\n");
codec->dev is easier.
+static int dsd1791_set_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- dsd1791->mclk = freq;
- return 0;
+}
Implement this as a CODEC wide operation, it's simpler.
+static const struct snd_kcontrol_new dsd1791_snd_controls[] = { +};
Remove empty variables and functions.
- err = snd_soc_add_controls(codec, dsd1791_snd_controls,
ARRAY_SIZE(dsd1791_snd_controls));
If this were non-empty it should be registered via the CODEC structure.
- dev_info(&spi->dev, "probing dsd1791 spi device\n");
This is just noise, remove it.
- dsd1791 = kzalloc(sizeof *dsd1791, GFP_KERNEL);
- if (!dsd1791)
return -ENOMEM;
devm_kzalloc().
- } else
dev_info(&spi->dev, "SPI device initialized\n");
Again, too chatty.
+static struct spi_driver dsd1791_spi = {
- .driver = {
.name = "dsd1791-codec",
No -codec.
+static int __init dsd1791_init(void) +{
- spi_register_driver(&dsd1791_spi);
- return 0;
Return the error code from spi_register_driver().
On 12/15/2011 2:16 AM, Mark Brown wrote:
On Wed, Dec 14, 2011 at 06:40:43PM -0500, Michael Williamson wrote:
[...]
+static int dsd1791_set_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- dsd1791->mclk = freq;
- return 0;
+}
Implement this as a CODEC wide operation, it's simpler.
Not sure I follow you here. Are you meaning to create something to replace these lines (which are in many of the routines) with a local inline?
- struct snd_soc_codec *codec = codec_dai->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
Or are you talking about the entire function? I think I will remove this entire function. The mclk is not yet used, but could be if support for some additional features of the chip is added. If I get that far I'll put it back in.
I understand all your other comments and will incorporate. Thanks for your review time.
-Mike
On Thu, Dec 15, 2011 at 03:32:22PM -0500, Michael Williamson wrote:
On 12/15/2011 2:16 AM, Mark Brown wrote:
On Wed, Dec 14, 2011 at 06:40:43PM -0500, Michael Williamson wrote:
+static int dsd1791_set_sysclk(struct snd_soc_dai *codec_dai,
int clk_id, unsigned int freq, int dir)
Implement this as a CODEC wide operation, it's simpler.
Not sure I follow you here. Are you meaning to create something to replace these lines (which are in many of the routines) with a local inline?
No, I mean implement your set_sysclk operation as a CODEC wide operation.
On 12/15/2011 12:40 AM, Michael Williamson wrote:
This patch introduces a (spi) codec driver for the Texas Instruments DSD1791 24 bit audio stereo DAC.
Testing for basic operation using 16 and 24 bit I2S mode has been performed.
http://www.ti.com/product/dsd1791
Signed-off-by: Michael Williamson michael.williamson@criticallink.com
[...]
+static int dsd1791_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_codec *codec = rtd->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- switch (params_format(params)) {
- case SNDRV_PCM_FORMAT_S16_LE:
dsd1791->pcm_fmt = DSD1971_FORMAT_S16_LE;break;- case SNDRV_PCM_FORMAT_S24_LE:
dsd1791->pcm_fmt = DSD1971_FORMAT_S24_LE;break;
There is really no need to add your own constants here. Just reuse the SNDRV_PCM_FORMAT constants and assign params_format(params) directly to dsd1791->pcm_fmt.
- default:
dev_dbg(&dsd1791->spi->dev, "bad format\n");return -EINVAL;- }
- return dsd1791_set_format_word(dsd1791, codec);
+}
[...] +static int dsd1791_set_fmt(struct snd_soc_dai *codec_dai,
unsigned int fmt)+{
- struct snd_soc_codec *codec = codec_dai->codec;
- struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
- switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
- case SND_SOC_DAIFMT_I2S:
dsd1791->dai_fmt = DSD1791_DAIFMT_I2S;break;- case SND_SOC_DAIFMT_RIGHT_J:
dsd1791->dai_fmt = DSD1791_DAIFMT_RIGHT_J;break;- case SND_SOC_DAIFMT_LEFT_J:
dsd1791->dai_fmt = DSD1791_DAIFMT_LEFT_J;break;
Same here for the SND_SOC_DAIFMT constants.
- default:
dev_dbg(&dsd1791->spi->dev, "bad format\n");return -EINVAL;- }
- return dsd1791_set_format_word(dsd1791, codec);
+}
+#define DSD1791_RATES SNDRV_PCM_RATE_8000_192000 +#define DSD1791_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
SNDRV_PCM_FMTBIT_S24_LE)+static struct snd_soc_dai_ops dsd1791_dai_ops = {
const
- .hw_params = dsd1791_hw_params,
- .set_sysclk = dsd1791_set_sysclk,
- .set_fmt = dsd1791_set_fmt,
+};
[...]
+struct snd_soc_codec_driver dsd1791_soc_codec_dev = {
static, and maybe rename it to dsd1719_codec_driver. The "_soc_codec_dev" suffix which you can see in other drivers in from old times, where this used to be a struct snd_soc_codec_device.
- .probe = dsd1791_probe,
- .read = dsd1791_read,
- .write = dsd1791_write,
- .reg_word_size = sizeof(u8),
+};
+static int dsd1791_spi_probe(struct spi_device *spi)
__devinit
+{ [...]
+static int dsd1791_spi_remove(struct spi_device *spi)
__devexit
+{ [...] +}
+static struct spi_driver dsd1791_spi = {
- .driver = {
.name = "dsd1791-codec",.owner = THIS_MODULE,- },
- .probe = dsd1791_spi_probe,
- .remove = dsd1791_spi_remove,
__devexit_p
+};
On Wed, Dec 14, 2011 at 06:40:42PM -0500, Michael Williamson wrote:
The following patch is an introduction of a codec driver for the TI DSD1791 audio part.
Don't send cover letters for single patches. Either you need to be putting this information into the changelog or the message doesn't have any useful content and doesn't need to be sent.
participants (4)
-
Lars-Peter Clausen -
Leon Romanovsky -
Mark Brown -
Michael Williamson