[PATCH] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

[PATCH] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

[Michael Williamson]

This patch introduces a (spi) codec driver for the Texas Instruments
DSD1791 24 bit audio stereo DAC.

http://www.ti.com/product/dsd1791

Testing for basic operation using 16 and 24 bit I2S mode has been
performed using a MityDSP-L138 SOM and an Industrial I/O board
from Critical Link.

Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
---
This patch incorporates changes from the original RFC as a result of comments
received from Mark Brown, Leon Romanovsky, and Lars-Peter Clausen.  Thanks.

Summary of Changes:
	- Use devm_kzalloc()
	- use regmap cached I/O feature of framework
	- simplify code applying codec fmt configuration
	- clean up section attributes on local functions
	- make local functions/variables static where appropriate
	- remove set_sysclk method
	- remove unused tlv header file
	- remove chatter
	- strip "-codec" from driver name
	- fixup driver variable name for consistency
	- Add Left and Right volume control
	- Add digital mute

 sound/soc/codecs/Kconfig   |    4 +
 sound/soc/codecs/Makefile  |    2 +
 sound/soc/codecs/dsd1791.c |  256 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 262 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..41bbc61
--- /dev/null
+++ b/sound/soc/codecs/dsd1791.c
@@ -0,0 +1,256 @@
+/*
+ * 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>
+
+#define DSD1791_REG_DIGATT_L	16
+#define DSD1791_REG_DIGATT_R	17
+#define DSD1791_REG_AUDFMT	18
+#define DSD1791_REG_SRST	20
+
+#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 DSD1791_FMT_MASK	0x70
+
+/* DSD1791 register cache (16 through 23 are used) */
+static const u8 dsd1791_reg[] = {
+	[16] = 0xFF,
+	[17] = 0xFF,
+	[18] = 0x50,
+	[19] = 0x00,
+	[20] = 0x00,
+	[21] = 0x01,
+	[22] = 0x00,
+	[23] = 0x00,
+};
+
+struct dsd1791 {
+	struct spi_device *spi;
+	struct snd_soc_codec codec;
+	int dai_fmt;
+	int pcm_fmt;
+};
+
+static int dsd1791_set_format_word(struct dsd1791 *dsd1791,
+				   struct snd_soc_codec *codec)
+{
+	u8 fmt = 0;
+	u8 reg;
+
+	switch (dsd1791->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+
+	case SND_SOC_DAIFMT_I2S:
+		switch (dsd1791->pcm_fmt) {
+		case SNDRV_PCM_FORMAT_S16_LE:
+			fmt = DSD1791_FMT_16I2S;
+			break;
+		case SNDRV_PCM_FORMAT_S24_LE:
+			fmt = DSD1791_FMT_24I2S;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+
+	case SND_SOC_DAIFMT_RIGHT_J:
+		switch (dsd1791->pcm_fmt) {
+		case SNDRV_PCM_FORMAT_S16_LE:
+			fmt = DSD1791_FMT_16RJ;
+			break;
+		case SNDRV_PCM_FORMAT_S24_LE:
+			fmt = DSD1791_FMT_24RJ;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+
+	case SND_SOC_DAIFMT_LEFT_J:
+		switch (dsd1791->pcm_fmt) {
+		case SNDRV_PCM_FORMAT_S24_LE:
+			fmt = DSD1791_FMT_24LJ;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
+	reg &= ~(DSD1791_FMT_MASK);
+	reg |= fmt;
+	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
+}
+
+static int dsd1791_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	u8 reg;
+
+	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
+	if (mute)
+		reg |= 1;
+	else
+		reg &= ~1;
+	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
+}
+
+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);
+
+	dsd1791->pcm_fmt = params_format(params);
+
+	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);
+
+	dsd1791->dai_fmt = fmt;
+
+	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 const struct snd_soc_dai_ops dsd1791_dai_ops = {
+	.hw_params	= dsd1791_hw_params,
+	.set_fmt	= dsd1791_set_fmt,
+	.digital_mute	= dsd1791_mute,
+};
+
+static 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[] = {
+	SOC_SINGLE("Left Playback Volume", DSD1791_REG_DIGATT_L, 0, 255, 0),
+	SOC_SINGLE("Right Playback Volume", DSD1791_REG_DIGATT_R, 0, 255, 0),
+};
+
+static int dsd1791_probe(struct snd_soc_codec *codec)
+{
+	u8 reg;
+	int ret;
+	struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
+
+	ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_SPI);
+	if (ret) {
+		dev_err(codec->dev, "Failed to set Cache I/O: %d\n", ret);
+		goto err;
+	}
+
+	ret = snd_soc_write(codec, DSD1791_REG_SRST, 0x40);
+	if (ret) {
+		dev_err(codec->dev, "Unable to reset device: %d\n", ret);
+		goto err;
+	}
+
+	/* default format after reset */
+	dsd1791->dai_fmt = SND_SOC_DAIFMT_I2S;
+	dsd1791->pcm_fmt = SNDRV_PCM_FORMAT_S24_LE;
+
+	/* enable attenuation control */
+	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
+	reg |= 0x80;
+	snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
+
+	snd_soc_add_controls(codec, dsd1791_snd_controls,
+				ARRAY_SIZE(dsd1791_snd_controls));
+	return 0;
+err:
+	return ret;
+}
+
+struct snd_soc_codec_driver dsd1791_codec_driver = {
+	.probe			= dsd1791_probe,
+	.reg_cache_size		= ARRAY_SIZE(dsd1791_reg),
+	.reg_word_size		= sizeof(u8),
+	.reg_cache_default	= dsd1791_reg,
+};
+
+static int __devinit dsd1791_spi_probe(struct spi_device *spi)
+{
+	struct dsd1791 *dsd1791;
+
+	dsd1791 = devm_kzalloc(&spi->dev, sizeof *dsd1791, GFP_KERNEL);
+	if (!dsd1791)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, dsd1791);
+
+	return snd_soc_register_codec(&spi->dev,
+		&dsd1791_codec_driver, &dsd1791_dai, 1);
+};
+
+static int __devexit dsd1791_spi_remove(struct spi_device *spi)
+{
+	snd_soc_unregister_codec(&spi->dev);
+	return 0;
+}
+
+static struct spi_driver dsd1791_spi_driver = {
+	.driver = {
+		.name = "dsd1791",
+		.owner = THIS_MODULE,
+	},
+	.probe = dsd1791_spi_probe,
+	.remove = __devexit_p(dsd1791_spi_remove),
+};
+
+static int __init dsd1791_init(void)
+{
+	return spi_register_driver(&dsd1791_spi_driver);
+}
+module_init(dsd1791_init);
+
+static void __exit dsd1791_exit(void)
+{
+	spi_unregister_driver(&dsd1791_spi_driver);
+}
+module_exit(dsd1791_exit);
+
+MODULE_DESCRIPTION("ASoC DSD1791 codec driver");
+MODULE_AUTHOR("Michael Williamson");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

[Liam Girdwood]

On Mon, 2011-12-19 at 13:53 -0500, Michael Williamson wrote:
> This patch introduces a (spi) codec driver for the Texas Instruments
> DSD1791 24 bit audio stereo DAC.
> 
> http://www.ti.com/product/dsd1791
> 
> Testing for basic operation using 16 and 24 bit I2S mode has been
> performed using a MityDSP-L138 SOM and an Industrial I/O board
> from Critical Link.
> 
> Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
Looks mostly fine, just a few comments :-

> ---
> This patch incorporates changes from the original RFC as a result of comments
> received from Mark Brown, Leon Romanovsky, and Lars-Peter Clausen.  Thanks.
> 
> Summary of Changes:
> 	- Use devm_kzalloc()
> 	- use regmap cached I/O feature of framework
> 	- simplify code applying codec fmt configuration
> 	- clean up section attributes on local functions
> 	- make local functions/variables static where appropriate
> 	- remove set_sysclk method
> 	- remove unused tlv header file
> 	- remove chatter
> 	- strip "-codec" from driver name
> 	- fixup driver variable name for consistency
> 	- Add Left and Right volume control
> 	- Add digital mute
> 
>  sound/soc/codecs/Kconfig   |    4 +
>  sound/soc/codecs/Makefile  |    2 +
>  sound/soc/codecs/dsd1791.c |  256 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 262 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..41bbc61
> --- /dev/null
> +++ b/sound/soc/codecs/dsd1791.c
> @@ -0,0 +1,256 @@
> +/*
> + * 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>
> +
> +#define DSD1791_REG_DIGATT_L	16
> +#define DSD1791_REG_DIGATT_R	17
> +#define DSD1791_REG_AUDFMT	18
> +#define DSD1791_REG_SRST	20
> +
> +#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 DSD1791_FMT_MASK	0x70
> +
> +/* DSD1791 register cache (16 through 23 are used) */
> +static const u8 dsd1791_reg[] = {
> +	[16] = 0xFF,
> +	[17] = 0xFF,
> +	[18] = 0x50,
> +	[19] = 0x00,
> +	[20] = 0x00,
> +	[21] = 0x01,
> +	[22] = 0x00,
> +	[23] = 0x00,
> +};
> +
> +struct dsd1791 {
> +	struct spi_device *spi;
> +	struct snd_soc_codec codec;
> +	int dai_fmt;
> +	int pcm_fmt;
> +};
> +
> +static int dsd1791_set_format_word(struct dsd1791 *dsd1791,
> +				   struct snd_soc_codec *codec)
> +{
> +	u8 fmt = 0;
> +	u8 reg;
> +
> +	switch (dsd1791->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +
> +	case SND_SOC_DAIFMT_I2S:
> +		switch (dsd1791->pcm_fmt) {
> +		case SNDRV_PCM_FORMAT_S16_LE:
> +			fmt = DSD1791_FMT_16I2S;
> +			break;
> +		case SNDRV_PCM_FORMAT_S24_LE:
> +			fmt = DSD1791_FMT_24I2S;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +
> +	case SND_SOC_DAIFMT_RIGHT_J:
> +		switch (dsd1791->pcm_fmt) {
> +		case SNDRV_PCM_FORMAT_S16_LE:
> +			fmt = DSD1791_FMT_16RJ;
> +			break;
> +		case SNDRV_PCM_FORMAT_S24_LE:
> +			fmt = DSD1791_FMT_24RJ;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		switch (dsd1791->pcm_fmt) {
> +		case SNDRV_PCM_FORMAT_S24_LE:
> +			fmt = DSD1791_FMT_24LJ;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
> +	reg &= ~(DSD1791_FMT_MASK);
> +	reg |= fmt;
> +	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);

You could make the code flow easier by using snd_soc_update_bits() here
and in other places.

> +}
> +
> +static int dsd1791_mute(struct snd_soc_dai *dai, int mute)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	u8 reg;
> +
> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
> +	if (mute)
> +		reg |= 1;
> +	else
> +		reg &= ~1;
> +	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
> +}
> +
> +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);
> +
> +	dsd1791->pcm_fmt = params_format(params);
> +
> +	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);
> +
> +	dsd1791->dai_fmt = fmt;
> +
> +	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 const struct snd_soc_dai_ops dsd1791_dai_ops = {
> +	.hw_params	= dsd1791_hw_params,
> +	.set_fmt	= dsd1791_set_fmt,
> +	.digital_mute	= dsd1791_mute,
> +};
> +
> +static 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[] = {
> +	SOC_SINGLE("Left Playback Volume", DSD1791_REG_DIGATT_L, 0, 255, 0),
> +	SOC_SINGLE("Right Playback Volume", DSD1791_REG_DIGATT_R, 0, 255, 0),

Best to use SOC_DOUBLE_R here and rename to "Master Playback Volume"

> +};
> +
> +static int dsd1791_probe(struct snd_soc_codec *codec)
> +{
> +	u8 reg;
> +	int ret;
> +	struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
> +
> +	ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_SPI);
> +	if (ret) {
> +		dev_err(codec->dev, "Failed to set Cache I/O: %d\n", ret);
> +		goto err;

I dont think goto is required in either case here so easier just to
return.

> +	}
> +
> +	ret = snd_soc_write(codec, DSD1791_REG_SRST, 0x40);
> +	if (ret) {
> +		dev_err(codec->dev, "Unable to reset device: %d\n", ret);
> +		goto err;
> +	}
> +
> +	/* default format after reset */
> +	dsd1791->dai_fmt = SND_SOC_DAIFMT_I2S;
> +	dsd1791->pcm_fmt = SNDRV_PCM_FORMAT_S24_LE;
> +
> +	/* enable attenuation control */
> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
> +	reg |= 0x80;
> +	snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
> +
> +	snd_soc_add_controls(codec, dsd1791_snd_controls,
> +				ARRAY_SIZE(dsd1791_snd_controls));
> +	return 0;
> +err:
> +	return ret;
> +}
> +
> +struct snd_soc_codec_driver dsd1791_codec_driver = {
> +	.probe			= dsd1791_probe,
> +	.reg_cache_size		= ARRAY_SIZE(dsd1791_reg),
> +	.reg_word_size		= sizeof(u8),
> +	.reg_cache_default	= dsd1791_reg,
> +};
> +
> +static int __devinit dsd1791_spi_probe(struct spi_device *spi)
> +{
> +	struct dsd1791 *dsd1791;
> +
> +	dsd1791 = devm_kzalloc(&spi->dev, sizeof *dsd1791, GFP_KERNEL);
> +	if (!dsd1791)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, dsd1791);
> +
> +	return snd_soc_register_codec(&spi->dev,
> +		&dsd1791_codec_driver, &dsd1791_dai, 1);
> +};
> +
> +static int __devexit dsd1791_spi_remove(struct spi_device *spi)
> +{
> +	snd_soc_unregister_codec(&spi->dev);

What about your private data ?

> +	return 0;
> +}
> +
> +static struct spi_driver dsd1791_spi_driver = {
> +	.driver = {
> +		.name = "dsd1791",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = dsd1791_spi_probe,
> +	.remove = __devexit_p(dsd1791_spi_remove),
> +};
> +
> +static int __init dsd1791_init(void)
> +{
> +	return spi_register_driver(&dsd1791_spi_driver);
> +}
> +module_init(dsd1791_init);
> +
> +static void __exit dsd1791_exit(void)
> +{
> +	spi_unregister_driver(&dsd1791_spi_driver);
> +}
> +module_exit(dsd1791_exit);
> +
> +MODULE_DESCRIPTION("ASoC DSD1791 codec driver");
> +MODULE_AUTHOR("Michael Williamson");
> +MODULE_LICENSE("GPL");

GPL v2 according to the commnts at the top.

Thanks

Liam

Re: [PATCH] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

[Michael Williamson]

On 12/19/2011 04:44 PM, Liam Girdwood wrote:
> On Mon, 2011-12-19 at 13:53 -0500, Michael Williamson wrote:
>> This patch introduces a (spi) codec driver for the Texas Instruments
>> DSD1791 24 bit audio stereo DAC.
>>
>> http://www.ti.com/product/dsd1791
>>
>> Testing for basic operation using 16 and 24 bit I2S mode has been
>> performed using a MityDSP-L138 SOM and an Industrial I/O board
>> from Critical Link.
>>
>> Signed-off-by: Michael Williamson <michael.williamson@criticallink.com>
> Looks mostly fine, just a few comments :-

Thanks for the review.  I will address your comments. One question, below.

-Mike

> 
>> ---
>> This patch incorporates changes from the original RFC as a result of comments
>> received from Mark Brown, Leon Romanovsky, and Lars-Peter Clausen.  Thanks.
>>
>> Summary of Changes:
>> 	- Use devm_kzalloc()
>> 	- use regmap cached I/O feature of framework
>> 	- simplify code applying codec fmt configuration
>> 	- clean up section attributes on local functions
>> 	- make local functions/variables static where appropriate
>> 	- remove set_sysclk method
>> 	- remove unused tlv header file
>> 	- remove chatter
>> 	- strip "-codec" from driver name
>> 	- fixup driver variable name for consistency
>> 	- Add Left and Right volume control
>> 	- Add digital mute
>>
>>  sound/soc/codecs/Kconfig   |    4 +
>>  sound/soc/codecs/Makefile  |    2 +
>>  sound/soc/codecs/dsd1791.c |  256 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 262 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..41bbc61
>> --- /dev/null
>> +++ b/sound/soc/codecs/dsd1791.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * 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>
>> +
>> +#define DSD1791_REG_DIGATT_L	16
>> +#define DSD1791_REG_DIGATT_R	17
>> +#define DSD1791_REG_AUDFMT	18
>> +#define DSD1791_REG_SRST	20
>> +
>> +#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 DSD1791_FMT_MASK	0x70
>> +
>> +/* DSD1791 register cache (16 through 23 are used) */
>> +static const u8 dsd1791_reg[] = {
>> +	[16] = 0xFF,
>> +	[17] = 0xFF,
>> +	[18] = 0x50,
>> +	[19] = 0x00,
>> +	[20] = 0x00,
>> +	[21] = 0x01,
>> +	[22] = 0x00,
>> +	[23] = 0x00,
>> +};
>> +
>> +struct dsd1791 {
>> +	struct spi_device *spi;
>> +	struct snd_soc_codec codec;
>> +	int dai_fmt;
>> +	int pcm_fmt;
>> +};
>> +
>> +static int dsd1791_set_format_word(struct dsd1791 *dsd1791,
>> +				   struct snd_soc_codec *codec)
>> +{
>> +	u8 fmt = 0;
>> +	u8 reg;
>> +
>> +	switch (dsd1791->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>> +
>> +	case SND_SOC_DAIFMT_I2S:
>> +		switch (dsd1791->pcm_fmt) {
>> +		case SNDRV_PCM_FORMAT_S16_LE:
>> +			fmt = DSD1791_FMT_16I2S;
>> +			break;
>> +		case SNDRV_PCM_FORMAT_S24_LE:
>> +			fmt = DSD1791_FMT_24I2S;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +		break;
>> +
>> +	case SND_SOC_DAIFMT_RIGHT_J:
>> +		switch (dsd1791->pcm_fmt) {
>> +		case SNDRV_PCM_FORMAT_S16_LE:
>> +			fmt = DSD1791_FMT_16RJ;
>> +			break;
>> +		case SNDRV_PCM_FORMAT_S24_LE:
>> +			fmt = DSD1791_FMT_24RJ;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +		break;
>> +
>> +	case SND_SOC_DAIFMT_LEFT_J:
>> +		switch (dsd1791->pcm_fmt) {
>> +		case SNDRV_PCM_FORMAT_S24_LE:
>> +			fmt = DSD1791_FMT_24LJ;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
>> +	reg &= ~(DSD1791_FMT_MASK);
>> +	reg |= fmt;
>> +	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
> 
> You could make the code flow easier by using snd_soc_update_bits() here
> and in other places.
> 
>> +}
>> +
>> +static int dsd1791_mute(struct snd_soc_dai *dai, int mute)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +	u8 reg;
>> +
>> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
>> +	if (mute)
>> +		reg |= 1;
>> +	else
>> +		reg &= ~1;
>> +	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
>> +}
>> +
>> +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);
>> +
>> +	dsd1791->pcm_fmt = params_format(params);
>> +
>> +	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);
>> +
>> +	dsd1791->dai_fmt = fmt;
>> +
>> +	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 const struct snd_soc_dai_ops dsd1791_dai_ops = {
>> +	.hw_params	= dsd1791_hw_params,
>> +	.set_fmt	= dsd1791_set_fmt,
>> +	.digital_mute	= dsd1791_mute,
>> +};
>> +
>> +static 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[] = {
>> +	SOC_SINGLE("Left Playback Volume", DSD1791_REG_DIGATT_L, 0, 255, 0),
>> +	SOC_SINGLE("Right Playback Volume", DSD1791_REG_DIGATT_R, 0, 255, 0),
> 
> Best to use SOC_DOUBLE_R here and rename to "Master Playback Volume"
> 
>> +};
>> +
>> +static int dsd1791_probe(struct snd_soc_codec *codec)
>> +{
>> +	u8 reg;
>> +	int ret;
>> +	struct dsd1791 *dsd1791 = snd_soc_codec_get_drvdata(codec);
>> +
>> +	ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_SPI);
>> +	if (ret) {
>> +		dev_err(codec->dev, "Failed to set Cache I/O: %d\n", ret);
>> +		goto err;
> 
> I dont think goto is required in either case here so easier just to
> return.
> 
>> +	}
>> +
>> +	ret = snd_soc_write(codec, DSD1791_REG_SRST, 0x40);
>> +	if (ret) {
>> +		dev_err(codec->dev, "Unable to reset device: %d\n", ret);
>> +		goto err;
>> +	}
>> +
>> +	/* default format after reset */
>> +	dsd1791->dai_fmt = SND_SOC_DAIFMT_I2S;
>> +	dsd1791->pcm_fmt = SNDRV_PCM_FORMAT_S24_LE;
>> +
>> +	/* enable attenuation control */
>> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
>> +	reg |= 0x80;
>> +	snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);
>> +
>> +	snd_soc_add_controls(codec, dsd1791_snd_controls,
>> +				ARRAY_SIZE(dsd1791_snd_controls));
>> +	return 0;
>> +err:
>> +	return ret;
>> +}
>> +
>> +struct snd_soc_codec_driver dsd1791_codec_driver = {
>> +	.probe			= dsd1791_probe,
>> +	.reg_cache_size		= ARRAY_SIZE(dsd1791_reg),
>> +	.reg_word_size		= sizeof(u8),
>> +	.reg_cache_default	= dsd1791_reg,
>> +};
>> +
>> +static int __devinit dsd1791_spi_probe(struct spi_device *spi)
>> +{
>> +	struct dsd1791 *dsd1791;
>> +
>> +	dsd1791 = devm_kzalloc(&spi->dev, sizeof *dsd1791, GFP_KERNEL);
>> +	if (!dsd1791)
>> +		return -ENOMEM;
>> +
>> +	spi_set_drvdata(spi, dsd1791);
>> +
>> +	return snd_soc_register_codec(&spi->dev,
>> +		&dsd1791_codec_driver, &dsd1791_dai, 1);
>> +};
>> +
>> +static int __devexit dsd1791_spi_remove(struct spi_device *spi)
>> +{
>> +	snd_soc_unregister_codec(&spi->dev);
> 
> What about your private data ?

My understanding is devm_kzalloc'd memory would be
freed up by the device framework.  Is this not 
correct?

> 
>> +	return 0;
>> +}
>> +
>> +static struct spi_driver dsd1791_spi_driver = {
>> +	.driver = {
>> +		.name = "dsd1791",
>> +		.owner = THIS_MODULE,
>> +	},
>> +	.probe = dsd1791_spi_probe,
>> +	.remove = __devexit_p(dsd1791_spi_remove),
>> +};
>> +
>> +static int __init dsd1791_init(void)
>> +{
>> +	return spi_register_driver(&dsd1791_spi_driver);
>> +}
>> +module_init(dsd1791_init);
>> +
>> +static void __exit dsd1791_exit(void)
>> +{
>> +	spi_unregister_driver(&dsd1791_spi_driver);
>> +}
>> +module_exit(dsd1791_exit);
>> +
>> +MODULE_DESCRIPTION("ASoC DSD1791 codec driver");
>> +MODULE_AUTHOR("Michael Williamson");
>> +MODULE_LICENSE("GPL");
> 
> GPL v2 according to the commnts at the top.
> 
> Thanks
> 
> Liam
> 
> 

Re: [PATCH] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

[Mark Brown]

On Mon, Dec 19, 2011 at 01:53:30PM -0500, Michael Williamson wrote:

> +/* DSD1791 register cache (16 through 23 are used) */
> +static const u8 dsd1791_reg[] = {
> +	[16] = 0xFF,
> +	[17] = 0xFF,
> +	[18] = 0x50,
> +	[19] = 0x00,
> +	[20] = 0x00,
> +	[21] = 0x01,
> +	[22] = 0x00,
> +	[23] = 0x00,
> +};

Use the regmap API.

> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
> +	reg &= ~(DSD1791_FMT_MASK);
> +	reg |= fmt;
> +	return snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);

snd_soc_update_bits().

> +static const struct snd_kcontrol_new dsd1791_snd_controls[] = {
> +	SOC_SINGLE("Left Playback Volume", DSD1791_REG_DIGATT_L, 0, 255, 0),
> +	SOC_SINGLE("Right Playback Volume", DSD1791_REG_DIGATT_R, 0, 255, 0),

This should be a single stereo control and you should supply dB data.

> +	/* enable attenuation control */
> +	reg = snd_soc_read(codec, DSD1791_REG_AUDFMT);
> +	reg |= 0x80;
> +	snd_soc_write(codec, DSD1791_REG_AUDFMT, reg);

This should probably be runtime controllable, or if it's got a good
reason for not being it should be using snd_soc_update_bits().

> +	snd_soc_add_controls(codec, dsd1791_snd_controls,
> +				ARRAY_SIZE(dsd1791_snd_controls));

Initialize the controls from the driver.

Re: [PATCH] ASoC: dsd1791: Introduce driver for TI DSD1791 stereo codec

[Mark Brown]

On Mon, Dec 19, 2011 at 05:26:49PM -0500, Michael Williamson wrote:
> On 12/19/2011 04:44 PM, Liam Girdwood wrote:

> > Looks mostly fine, just a few comments :-

> Thanks for the review.  I will address your comments. One question, below.

Guys, please delete irrelevant context from your mails.

> >> +{
> >> +	snd_soc_unregister_codec(&spi->dev);

> > What about your private data ?

> My understanding is devm_kzalloc'd memory would be
> freed up by the device framework.  Is this not 
> correct?

Your understanding is correct, that's the whole point of devm_kzalloc().