Skip to contentSkip to conversation

Add support for Cirrus Logic CS4349

PATCH Superseded [email protected] 2 participants 2 messages Jul 13, 2015

Subsystems (3)
Sound ASoC Cirrus Logic audio codecs

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/2 Add support for Cirrus Logic CS4349

From: Tim Howe <[email protected]>

Signed-off-by Tim Howe <[email protected]>
---
 sound/soc/codecs/Kconfig  |   6 +
 sound/soc/codecs/Makefile |   2 +
 sound/soc/codecs/cs4349.c | 425 ++++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/cs4349.h | 146 ++++++++++++++++
 4 files changed, 579 insertions(+)
 create mode 100644 sound/soc/codecs/cs4349.c
 create mode 100644 sound/soc/codecs/cs4349.h
sound/soc/codecs/Kconfig +6 -0
index 7223c7f..4a78606 100644
@@ -54,6 +54,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_CS4271_I2C if I2C
 	select SND_SOC_CS4271_SPI if SPI_MASTER
 	select SND_SOC_CS42XX8_I2C if I2C
+	select SND_SOC_CS4349 if I2C
 	select SND_SOC_CX20442 if TTY
 	select SND_SOC_DA7210 if SND_SOC_I2C_AND_SPI
 	select SND_SOC_DA7213 if I2C

@@ -413,6 +414,11 @@ config SND_SOC_CS42XX8_I2C
 	select SND_SOC_CS42XX8
 	select REGMAP_I2C
 
+# Cirrus Logic CS4349 HiFi DAC
+config SND_SOC_CS4349
+	tristate "Cirrus Logic CS4349 CODEC"
+	depends on I2C
+
 config SND_SOC_CX20442
 	tristate
 	depends on TTY
sound/soc/codecs/Makefile +2 -0
index 9f7136c..36a9f37 100644
@@ -46,6 +46,7 @@ snd-soc-cs4271-i2c-objs := cs4271-i2c.o
 snd-soc-cs4271-spi-objs := cs4271-spi.o
 snd-soc-cs42xx8-objs := cs42xx8.o
 snd-soc-cs42xx8-i2c-objs := cs42xx8-i2c.o
+snd-soc-cs4349-objs := cs4349.o
 snd-soc-cx20442-objs := cx20442.o
 snd-soc-da7210-objs := da7210.o
 snd-soc-da7213-objs := da7213.o

@@ -237,6 +238,7 @@ obj-$(CONFIG_SND_SOC_CS4271_I2C)	+= snd-soc-cs4271-i2c.o
 obj-$(CONFIG_SND_SOC_CS4271_SPI)	+= snd-soc-cs4271-spi.o
 obj-$(CONFIG_SND_SOC_CS42XX8)	+= snd-soc-cs42xx8.o
 obj-$(CONFIG_SND_SOC_CS42XX8_I2C) += snd-soc-cs42xx8-i2c.o
+obj-$(CONFIG_SND_SOC_CS4349)	+= snd-soc-cs4349.o
 obj-$(CONFIG_SND_SOC_CX20442)	+= snd-soc-cx20442.o
 obj-$(CONFIG_SND_SOC_DA7210)	+= snd-soc-da7210.o
 obj-$(CONFIG_SND_SOC_DA7213)	+= snd-soc-da7213.o
sound/soc/codecs/cs4349.c new file +425 -0

425 lines not on this page · raw message

sound/soc/codecs/cs4349.h new file +146 -0

146 lines not on this page · raw message

Signature · 1 line
2.4.5
maintainer
#1 lore raw
Quoted 3 lines · On Mon, Jul 13, 2015 at 03:08:25PM -0500, [email protected] wrote:

From: Tim Howe <[email protected]>

Signed-off-by Tim Howe <[email protected]>

This looks mostly good. One nit on the submission format - please use subject lines matching the style for the subsystem, "ASoC: cs4349: ..." with just [PATCH 1/2] as the prefix in sending. Also some fairly minor issues below:

sound/soc/codecs/cs4349.c line 320

Quoted 13 lines
sound/soc/codecs/cs4349.c new file +13 -0
+	if (pdata) {
+		cs4349->pdata = *pdata;
+	} else {
+		pdata = devm_kzalloc(&client->dev,
+				     sizeof(struct cs4349_platform_data),
+				     GFP_KERNEL);
+		if (!pdata) {
+			dev_err(&client->dev,
+				"could not allocate pdata\n");
+			return -ENOMEM;
+		}
+		cs4349->pdata = *pdata;
+	}

Why allocate the platform data if none is supplied only to copy the zero initialized data into the driver data then never reference it again? The entire else case can be dropped.

sound/soc/codecs/cs4349.c line 334

Quoted 2 lines
sound/soc/codecs/cs4349.c new file +2 -0
+	dev_info(&client->dev,
+		 "Cirrus Logic CS4349\n");

Remove this, it's not adding anything.

sound/soc/codecs/cs4349.c line 366

Quoted 13 lines
sound/soc/codecs/cs4349.c new file +13 -0
+static int cs4349_runtime_suspend(struct device *dev)
+{
+	struct cs4349_private *cs4349 = dev_get_drvdata(dev);
+	struct snd_soc_pcm_runtime *rtd = dev_get_drvdata(dev);
+	int ret;
+
+	/* Hold down reset */
+	if (cs4349->reset_gpio)
+		gpiod_set_value_cansleep(cs4349->reset_gpio, 0);
+
+	ret = snd_soc_update_bits(rtd->codec, CS4349_MISC, PWR_DWN, 1);
+	if (ret < 0)
+		return ret;

Shouldn't the device be put into reset after powering it down rather than before? I'd expect there might be problems with the I2C I/O if the device is held in reset...

sound/soc/codecs/cs4349.c line 384

Quoted 4 lines
sound/soc/codecs/cs4349.c new file +3 -0
+static int cs4349_runtime_resume(struct device *dev)
+{
 
+	regcache_cache_only(cs4349->regmap, false);

I'd expect the device to be marked cache only on suspend?

sound/soc/codecs/cs4349.c line 399

Quoted 4 lines
sound/soc/codecs/cs4349.c new file +4 -0
+static const struct of_device_id cs4349_of_match[] = {
+	{ .compatible = "cirrus,cs4349", },
+	{},
+};

This is adding a compatible string but the binding is undocumented, all new bindings must be documented.