[PATCH v1 0/4] ASoC: Add support for DAC PCM1789

[PATCH v1 0/4] ASoC: Add support for DAC PCM1789

[Mylène Josserand]

Hello everyone,

You will find in this series the support of Texas Instrument's DAC
PCM1789.
This DAC is very minimalist and is similar to PCM1792a except for
some differences in registers.
Series based on asoc tree, "for-next" branch (last commit 130c3888dfdc).

It is important to notice that this DAC needs to always have clocks
enabled (even without any data) otherwise it will be in a "desynchronized"
state and can not send data correctly.
This issue has been solved by performing a reset each time a sound
is played (see patch 04). This reset can produce a "pop" noise.

Thank you in advance for any review.

Best regards,
Mylène

Mylène Josserand (4):
  ASoC: codecs: pcm179x: Add PCM1789 id
  ASoC: codecs: pcm179x: Add support for PCM1789
  ASoC: codecs: pcm179x: Add reset gpio
  ASoC: codecs: pcm179x: Add trigger function to perform a reset

 .../devicetree/bindings/sound/pcm179x.txt          |   2 +-
 sound/soc/codecs/pcm179x-i2c.c                     |  17 +-
 sound/soc/codecs/pcm179x.c                         | 242 ++++++++++++++++++++-
 sound/soc/codecs/pcm179x.h                         |  10 +-
 4 files changed, 264 insertions(+), 7 deletions(-)

-- 
2.11.0

[PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id

[Mylène Josserand]

To prepare the support for the PCM1789, add a new compatible
and use the i2c_id to handle, later, the differences between
these two DACs even if they are pretty similar.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-
 sound/soc/codecs/pcm179x-i2c.c                      | 6 ++++--
 sound/soc/codecs/pcm179x.c                          | 3 ++-
 sound/soc/codecs/pcm179x.h                          | 8 +++++++-
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/pcm179x.txt b/Documentation/devicetree/bindings/sound/pcm179x.txt
index 436c2b247693..bf725d302958 100644
--- a/Documentation/devicetree/bindings/sound/pcm179x.txt
+++ b/Documentation/devicetree/bindings/sound/pcm179x.txt
@@ -4,7 +4,7 @@ This driver supports both the I2C and SPI bus.
 
 Required properties:
 
- - compatible: "ti,pcm1792a"
+ - compatible: "ti,pcm1792a" or "ti,pcm1789"
 
 For required properties on SPI, please consult
 Documentation/devicetree/bindings/spi/spi-bus.txt
diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 03747966c6bc..795a0657c097 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -36,17 +36,19 @@ static int pcm179x_i2c_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	return pcm179x_common_init(&client->dev, regmap);
+	return pcm179x_common_init(&client->dev, regmap, id->driver_data);
 }
 
 static const struct of_device_id pcm179x_of_match[] = {
 	{ .compatible = "ti,pcm1792a", },
+	{ .compatible = "ti,pcm1789", },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pcm179x_of_match);
 
 static const struct i2c_device_id pcm179x_i2c_ids[] = {
-	{ "pcm179x", 0 },
+	{ "pcm179x", PCM179X },
+	{ "pcm1789", PCM1789 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 4b311c06f97d..81cbf09319f6 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -218,7 +218,8 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
 	.non_legacy_dai_naming	= 1,
 };
 
-int pcm179x_common_init(struct device *dev, struct regmap *regmap)
+int pcm179x_common_init(struct device *dev, struct regmap *regmap,
+			enum pcm17xx_type type)
 {
 	struct pcm179x_private *pcm179x;
 
diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
index cf8681c9a373..8c08689e3b8b 100644
--- a/sound/soc/codecs/pcm179x.h
+++ b/sound/soc/codecs/pcm179x.h
@@ -17,11 +17,17 @@
 #ifndef __PCM179X_H__
 #define __PCM179X_H__
 
+enum pcm17xx_type {
+	PCM179X,
+	PCM1789,
+};
+
 #define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
 			  SNDRV_PCM_FMTBIT_S16_LE)
 
 extern const struct regmap_config pcm179x_regmap_config;
 
-int pcm179x_common_init(struct device *dev, struct regmap *regmap);
+int pcm179x_common_init(struct device *dev, struct regmap *regmap,
+			enum pcm17xx_type type);
 
 #endif
-- 
2.11.0

[PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

[Mylène Josserand]

Add PCM1789 DAC support into pcm179x file.
This DAC is pretty much the same than PCM179x but some
registers are differents (such as mute registers split in
right/left).

One particularity about this DAC is that the clocks must be
always enabled. Also, an entire software reset is necessary
while starting to play a sound otherwise, the clocks are not
synchronized (so the DAC is not able to send data).

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 sound/soc/codecs/pcm179x-i2c.c |   7 +-
 sound/soc/codecs/pcm179x.c     | 164 +++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/pcm179x.h     |   1 +
 3 files changed, 170 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 795a0657c097..83a2e1508df8 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -26,10 +26,13 @@
 static int pcm179x_i2c_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
-	struct regmap *regmap;
+	struct regmap *regmap = NULL;
 	int ret;
 
-	regmap = devm_regmap_init_i2c(client, &pcm179x_regmap_config);
+	if (id->driver_data == PCM1789)
+		regmap = devm_regmap_init_i2c(client, &pcm1789_regmap_config);
+	else
+		regmap = devm_regmap_init_i2c(client, &pcm179x_regmap_config);
 	if (IS_ERR(regmap)) {
 		ret = PTR_ERR(regmap);
 		dev_err(&client->dev, "Failed to allocate regmap: %d\n", ret);
diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 81cbf09319f6..2285a51ff9e9 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -43,6 +43,17 @@
 #define PCM179X_MUTE_SHIFT	0
 #define PCM179X_ATLD_ENABLE	(1 << 7)
 
+#define PCM1789_FMT_CONTROL	0x11
+#define PCM1789_FLT_CONTROL	0x12
+#define PCM1789_REV_CONTROL	0x13
+#define PCM1789_SOFT_MUTE	0x14
+#define PCM1789_DAC_VOL_LEFT	0x18
+#define PCM1789_DAC_VOL_RIGHT	0x19
+#define PCM1789_FMT_MASK	0x07
+#define PCM1789_MUTE_MASK	0x03
+#define PCM1789_MUTE_L_EN	BIT(0)
+#define PCM1789_MUTE_R_EN	BIT(1)
+
 static const struct reg_default pcm179x_reg_defaults[] = {
 	{ 0x10, 0xff },
 	{ 0x11, 0xff },
@@ -54,11 +65,25 @@ static const struct reg_default pcm179x_reg_defaults[] = {
 	{ 0x17, 0x00 },
 };
 
+static const struct reg_default pcm1789_reg_defaults[] = {
+	{ PCM1789_FMT_CONTROL, 0x00 },
+	{ PCM1789_FLT_CONTROL, 0x00 },
+	{ PCM1789_REV_CONTROL, 0x00 },
+	{ PCM1789_SOFT_MUTE, 0x00 },
+	{ PCM1789_DAC_VOL_LEFT, 0xff },
+	{ PCM1789_DAC_VOL_RIGHT, 0xff },
+};
+
 static bool pcm179x_accessible_reg(struct device *dev, unsigned int reg)
 {
 	return reg >= 0x10 && reg <= 0x17;
 }
 
+static bool pcm1789_accessible_reg(struct device *dev, unsigned int reg)
+{
+	return reg >= PCM1789_FMT_CONTROL && reg <= PCM1789_DAC_VOL_RIGHT;
+}
+
 static bool pcm179x_writeable_reg(struct device *dev, unsigned int reg)
 {
 	bool accessible;
@@ -68,6 +93,15 @@ static bool pcm179x_writeable_reg(struct device *dev, unsigned int reg)
 	return accessible && reg != 0x16 && reg != 0x17;
 }
 
+static bool pcm1789_writeable_reg(struct device *dev, unsigned int reg)
+{
+	bool accessible;
+
+	accessible = pcm1789_accessible_reg(dev, reg);
+
+	return accessible && reg != 0x16 && reg != 0x17;
+}
+
 struct pcm179x_private {
 	struct regmap *regmap;
 	unsigned int format;
@@ -99,6 +133,24 @@ static int pcm179x_digital_mute(struct snd_soc_dai *dai, int mute)
 	return 0;
 }
 
+static int pcm1789_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+	int ret, val;
+
+	if (mute)
+		val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);
+	else
+		val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
+	ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
+				 PCM1789_MUTE_MASK, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int pcm179x_hw_params(struct snd_pcm_substream *substream,
 			     struct snd_pcm_hw_params *params,
 			     struct snd_soc_dai *dai)
@@ -151,12 +203,76 @@ static int pcm179x_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int pcm1789_hw_params(struct snd_pcm_substream *substream,
+			     struct snd_pcm_hw_params *params,
+			     struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+	int val = 0, ret;
+
+	priv->rate = params_rate(params);
+
+	switch (priv->format & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_RIGHT_J:
+		switch (params_width(params)) {
+		case 24:
+			val = 2;
+			break;
+		case 16:
+			val = 3;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case SND_SOC_DAIFMT_I2S:
+		switch (params_width(params)) {
+		case 16:
+		case 24:
+		case 32:
+			val = 0;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		switch (params_width(params)) {
+		case 16:
+		case 24:
+		case 32:
+			val = 1;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		dev_err(component->dev, "Invalid DAI format\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(priv->regmap, PCM1789_FMT_CONTROL,
+				 PCM1789_FMT_MASK, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static const struct snd_soc_dai_ops pcm179x_dai_ops = {
 	.set_fmt	= pcm179x_set_dai_fmt,
 	.hw_params	= pcm179x_hw_params,
 	.digital_mute	= pcm179x_digital_mute,
 };
 
+static const struct snd_soc_dai_ops pcm1789_dai_ops = {
+	.set_fmt	= pcm179x_set_dai_fmt,
+	.hw_params	= pcm1789_hw_params,
+	.digital_mute	= pcm1789_digital_mute,
+};
+
 static const DECLARE_TLV_DB_SCALE(pcm179x_dac_tlv, -12000, 50, 1);
 
 static const struct snd_kcontrol_new pcm179x_controls[] = {
@@ -167,6 +283,12 @@ static const struct snd_kcontrol_new pcm179x_controls[] = {
 	SOC_SINGLE("DAC Rolloff Filter Switch", PCM179X_MODE_CONTROL, 1, 1, 0),
 };
 
+static const struct snd_kcontrol_new pcm1789_controls[] = {
+	SOC_DOUBLE_R_RANGE_TLV("DAC Playback Volume", PCM1789_DAC_VOL_LEFT,
+			       PCM1789_DAC_VOL_RIGHT, 0, 0xf, 0xff, 0,
+			       pcm179x_dac_tlv),
+};
+
 static const struct snd_soc_dapm_widget pcm179x_dapm_widgets[] = {
 SND_SOC_DAPM_OUTPUT("IOUTL+"),
 SND_SOC_DAPM_OUTPUT("IOUTL-"),
@@ -194,6 +316,19 @@ static struct snd_soc_dai_driver pcm179x_dai = {
 	.ops = &pcm179x_dai_ops,
 };
 
+static struct snd_soc_dai_driver pcm1789_dai = {
+	.name = "pcm1789-hifi",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_CONTINUOUS,
+		.rate_min = 10000,
+		.rate_max = 200000,
+		.formats = PCM1792A_FORMATS, },
+	.ops = &pcm1789_dai_ops,
+};
+
 const struct regmap_config pcm179x_regmap_config = {
 	.reg_bits		= 8,
 	.val_bits		= 8,
@@ -205,6 +340,17 @@ const struct regmap_config pcm179x_regmap_config = {
 };
 EXPORT_SYMBOL_GPL(pcm179x_regmap_config);
 
+const struct regmap_config pcm1789_regmap_config = {
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= PCM1789_DAC_VOL_RIGHT,
+	.reg_defaults		= pcm1789_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(pcm1789_reg_defaults),
+	.writeable_reg		= pcm1789_writeable_reg,
+	.readable_reg		= pcm1789_accessible_reg,
+};
+EXPORT_SYMBOL_GPL(pcm1789_regmap_config);
+
 static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
 	.controls		= pcm179x_controls,
 	.num_controls		= ARRAY_SIZE(pcm179x_controls),
@@ -218,6 +364,19 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
 	.non_legacy_dai_naming	= 1,
 };
 
+static const struct snd_soc_component_driver soc_component_dev_pcm1789 = {
+	.controls		= pcm1789_controls,
+	.num_controls		= ARRAY_SIZE(pcm1789_controls),
+	.dapm_widgets		= pcm179x_dapm_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(pcm179x_dapm_widgets),
+	.dapm_routes		= pcm179x_dapm_routes,
+	.num_dapm_routes	= ARRAY_SIZE(pcm179x_dapm_routes),
+	.idle_bias_on		= 1,
+	.use_pmdown_time	= 1,
+	.endianness		= 1,
+	.non_legacy_dai_naming	= 1,
+};
+
 int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 			enum pcm17xx_type type)
 {
@@ -231,6 +390,11 @@ int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 	pcm179x->regmap = regmap;
 	dev_set_drvdata(dev, pcm179x);
 
+	if (type == PCM1789)
+		return devm_snd_soc_register_component(dev,
+						       &soc_component_dev_pcm1789,
+						       &pcm1789_dai, 1);
+
 	return devm_snd_soc_register_component(dev,
 			&soc_component_dev_pcm179x, &pcm179x_dai, 1);
 }
diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
index 8c08689e3b8b..a79726933a3f 100644
--- a/sound/soc/codecs/pcm179x.h
+++ b/sound/soc/codecs/pcm179x.h
@@ -26,6 +26,7 @@ enum pcm17xx_type {
 			  SNDRV_PCM_FMTBIT_S16_LE)
 
 extern const struct regmap_config pcm179x_regmap_config;
+extern const struct regmap_config pcm1789_regmap_config;
 
 int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 			enum pcm17xx_type type);
-- 
2.11.0

[PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Add a reset gpio to be able to reset this line at startup.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 sound/soc/codecs/pcm179x.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 2285a51ff9e9..0242dfd67b53 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -22,6 +22,8 @@
 #include <linux/device.h>
 
 #include <sound/core.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/initval.h>
@@ -106,6 +108,7 @@ struct pcm179x_private {
 	struct regmap *regmap;
 	unsigned int format;
 	unsigned int rate;
+	int reset;
 };
 
 static int pcm179x_set_dai_fmt(struct snd_soc_dai *codec_dai,
@@ -381,6 +384,8 @@ int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 			enum pcm17xx_type type)
 {
 	struct pcm179x_private *pcm179x;
+	struct device_node *np = dev->of_node;
+	int ret;
 
 	pcm179x = devm_kzalloc(dev, sizeof(struct pcm179x_private),
 				GFP_KERNEL);
@@ -390,6 +395,21 @@ int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 	pcm179x->regmap = regmap;
 	dev_set_drvdata(dev, pcm179x);
 
+	pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
+	if (gpio_is_valid(pcm179x->reset)) {
+		ret = devm_gpio_request_one(dev, pcm179x->reset,
+					    GPIOF_OUT_INIT_LOW,
+					    "pcm179x reset");
+		if (ret) {
+			dev_err(dev,
+				"Failed to request GPIO %d as reset pin, error %d\n",
+				pcm179x->reset, ret);
+			return ret;
+		}
+
+		gpio_set_value(pcm179x->reset, 1);
+	}
+
 	if (type == PCM1789)
 		return devm_snd_soc_register_component(dev,
 						       &soc_component_dev_pcm1789,
-- 
2.11.0

[PATCH v1 4/4] ASoC: codecs: pcm179x: Add trigger function to perform a reset

[Mylène Josserand]

Add trigger function to perform a reset when we are starting to
play a sound. Thanks to that, the codec will not be in
desynchronization state anymore and the data will be sent correctly.

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 sound/soc/codecs/pcm179x-i2c.c |  4 +++
 sound/soc/codecs/pcm179x.c     | 61 +++++++++++++++++++++++++++++++++++++++---
 sound/soc/codecs/pcm179x.h     |  1 +
 3 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 83a2e1508df8..f8b4b07ce7f2 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -65,6 +65,10 @@ static struct i2c_driver pcm179x_i2c_driver = {
 	.probe		= pcm179x_i2c_probe,
 };
 
+int pcm179x_i2c_remove(struct i2c_client *client)
+{
+	return pcm179x_common_exit(&client->dev);
+}
 module_i2c_driver(pcm179x_i2c_driver);
 
 MODULE_DESCRIPTION("ASoC PCM179X I2C driver");
diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 0242dfd67b53..4c7f4010a144 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -30,6 +30,7 @@
 #include <sound/soc.h>
 #include <sound/tlv.h>
 #include <linux/of.h>
+#include <linux/workqueue.h>
 
 #include "pcm179x.h"
 
@@ -45,6 +46,7 @@
 #define PCM179X_MUTE_SHIFT	0
 #define PCM179X_ATLD_ENABLE	(1 << 7)
 
+#define PCM1789_MUTE_CONTROL	0x10
 #define PCM1789_FMT_CONTROL	0x11
 #define PCM1789_FLT_CONTROL	0x12
 #define PCM1789_REV_CONTROL	0x13
@@ -55,6 +57,7 @@
 #define PCM1789_MUTE_MASK	0x03
 #define PCM1789_MUTE_L_EN	BIT(0)
 #define PCM1789_MUTE_R_EN	BIT(1)
+#define PCM1789_MUTE_SRET	0x06
 
 static const struct reg_default pcm179x_reg_defaults[] = {
 	{ 0x10, 0xff },
@@ -83,7 +86,7 @@ static bool pcm179x_accessible_reg(struct device *dev, unsigned int reg)
 
 static bool pcm1789_accessible_reg(struct device *dev, unsigned int reg)
 {
-	return reg >= PCM1789_FMT_CONTROL && reg <= PCM1789_DAC_VOL_RIGHT;
+	return reg >= PCM1789_MUTE_CONTROL && reg <= PCM1789_DAC_VOL_RIGHT;
 }
 
 static bool pcm179x_writeable_reg(struct device *dev, unsigned int reg)
@@ -109,6 +112,8 @@ struct pcm179x_private {
 	unsigned int format;
 	unsigned int rate;
 	int reset;
+	struct work_struct work;
+	struct device *dev;
 };
 
 static int pcm179x_set_dai_fmt(struct snd_soc_dai *codec_dai,
@@ -264,6 +269,42 @@ static int pcm1789_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static void pcm1789_work_queue(struct work_struct *work)
+{
+	struct pcm179x_private *priv = container_of(work,
+						    struct pcm179x_private,
+						    work);
+
+	/* Soft reset */
+	if (regmap_update_bits(priv->regmap, PCM1789_MUTE_CONTROL,
+			       0x3 << PCM1789_MUTE_SRET, 0) < 0)
+		dev_err(priv->dev, "Error while setting SRET");
+}
+
+static int pcm1789_trigger(struct snd_pcm_substream *substream, int cmd,
+			   struct snd_soc_dai *dai)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+	int ret = 0;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		schedule_work(&priv->work);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static const struct snd_soc_dai_ops pcm179x_dai_ops = {
 	.set_fmt	= pcm179x_set_dai_fmt,
 	.hw_params	= pcm179x_hw_params,
@@ -274,6 +315,7 @@ static const struct snd_soc_dai_ops pcm1789_dai_ops = {
 	.set_fmt	= pcm179x_set_dai_fmt,
 	.hw_params	= pcm1789_hw_params,
 	.digital_mute	= pcm1789_digital_mute,
+	.trigger	= pcm1789_trigger,
 };
 
 static const DECLARE_TLV_DB_SCALE(pcm179x_dac_tlv, -12000, 50, 1);
@@ -392,6 +434,7 @@ int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 	if (!pcm179x)
 		return -ENOMEM;
 
+	pcm179x->dev = dev;
 	pcm179x->regmap = regmap;
 	dev_set_drvdata(dev, pcm179x);
 
@@ -410,16 +453,28 @@ int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 		gpio_set_value(pcm179x->reset, 1);
 	}
 
-	if (type == PCM1789)
+	if (type == PCM1789) {
+		INIT_WORK(&pcm179x->work, pcm1789_work_queue);
 		return devm_snd_soc_register_component(dev,
 						       &soc_component_dev_pcm1789,
 						       &pcm1789_dai, 1);
-
+	}
 	return devm_snd_soc_register_component(dev,
 			&soc_component_dev_pcm179x, &pcm179x_dai, 1);
 }
 EXPORT_SYMBOL_GPL(pcm179x_common_init);
 
+int pcm179x_common_exit(struct device *dev)
+{
+	struct pcm179x_private *priv = dev_get_drvdata(dev);
+
+	if (&priv->work)
+		flush_work(&priv->work);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pcm179x_common_exit);
+
 MODULE_DESCRIPTION("ASoC PCM179X driver");
 MODULE_AUTHOR("Michael Trimarchi <michael@amarulasolutions.com>");
 MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
index a79726933a3f..2cc75313c822 100644
--- a/sound/soc/codecs/pcm179x.h
+++ b/sound/soc/codecs/pcm179x.h
@@ -30,5 +30,6 @@ extern const struct regmap_config pcm1789_regmap_config;
 
 int pcm179x_common_init(struct device *dev, struct regmap *regmap,
 			enum pcm17xx_type type);
+int pcm179x_common_exit(struct device *dev);
 
 #endif
-- 
2.11.0

Re: [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id

[Thomas Petazzoni]

Hello,

On Tue, 27 Feb 2018 22:24:30 +0100, Mylène Josserand wrote:
> To prepare the support for the PCM1789, add a new compatible
> and use the i2c_id to handle, later, the differences between
> these two DACs even if they are pretty similar.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-

The DT binding change should be in a separate patch.

>  sound/soc/codecs/pcm179x-i2c.c                      | 6 ++++--
>  sound/soc/codecs/pcm179x.c                          | 3 ++-
>  sound/soc/codecs/pcm179x.h                          | 8 +++++++-

And this should be together with the PCM1789 support patch. Otherwise
your series is not bisectable: if we apply only PATCH 1/4, the driver
supports the new compatible string, but it doesn't have the actual code
to handle PCM1789. Am I missing something here ?

> -	return pcm179x_common_init(&client->dev, regmap);
> +	return pcm179x_common_init(&client->dev, regmap, id->driver_data);

This can be done in a preparation patch.

>  }
>  
>  static const struct of_device_id pcm179x_of_match[] = {
>  	{ .compatible = "ti,pcm1792a", },
> +	{ .compatible = "ti,pcm1789", },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, pcm179x_of_match);
>  
>  static const struct i2c_device_id pcm179x_i2c_ids[] = {
> -	{ "pcm179x", 0 },
> +	{ "pcm179x", PCM179X },

And also this addition.

> +	{ "pcm1789", PCM1789 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
> diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
> index 4b311c06f97d..81cbf09319f6 100644
> --- a/sound/soc/codecs/pcm179x.c
> +++ b/sound/soc/codecs/pcm179x.c
> @@ -218,7 +218,8 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
>  	.non_legacy_dai_naming	= 1,
>  };
>  
> -int pcm179x_common_init(struct device *dev, struct regmap *regmap)
> +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> +			enum pcm17xx_type type)

And this done.

>  {
>  	struct pcm179x_private *pcm179x;
>  
> diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
> index cf8681c9a373..8c08689e3b8b 100644
> --- a/sound/soc/codecs/pcm179x.h
> +++ b/sound/soc/codecs/pcm179x.h
> @@ -17,11 +17,17 @@
>  #ifndef __PCM179X_H__
>  #define __PCM179X_H__
>  
> +enum pcm17xx_type {
> +	PCM179X,

And this one.

> +	PCM1789,
> +};
> +
>  #define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
>  			  SNDRV_PCM_FMTBIT_S16_LE)
>  
>  extern const struct regmap_config pcm179x_regmap_config;
>  
> -int pcm179x_common_init(struct device *dev, struct regmap *regmap);
> +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> +			enum pcm17xx_type type);

And this one. Just as a "preparation patch" to support multiple codecs
in the existing pcm179x.c driver.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

[Thomas Petazzoni]

Hello,

On Tue, 27 Feb 2018 22:24:31 +0100, Mylène Josserand wrote:

> diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> index 795a0657c097..83a2e1508df8 100644
> --- a/sound/soc/codecs/pcm179x-i2c.c
> +++ b/sound/soc/codecs/pcm179x-i2c.c
> @@ -26,10 +26,13 @@
>  static int pcm179x_i2c_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *id)
>  {
> -	struct regmap *regmap;
> +	struct regmap *regmap = NULL;

I don't think this change is useful, since regmap is always initialized
below anyway.


> +	if (mute)
> +		val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);

That's not really useful with regmap_update_bits() which already does
the masking, no?

> +	else
> +		val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
> +	ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> +				 PCM1789_MUTE_MASK, val);

Couldn't this be:

	if (mute)
		val = 0;
	else
		val = PCM1789_MUTE_MASK;

	ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
				 PCM1789_MUTE_MASK, val);


> +static struct snd_soc_dai_driver pcm1789_dai = {
> +	.name = "pcm1789-hifi",
> +	.playback = {
> +		.stream_name = "Playback",
> +		.channels_min = 2,
> +		.channels_max = 2,
> +		.rates = SNDRV_PCM_RATE_CONTINUOUS,
> +		.rate_min = 10000,
> +		.rate_max = 200000,
> +		.formats = PCM1792A_FORMATS, },

Nit: the closing curly brace should be on a separate line.


> +	if (type == PCM1789)
> +		return devm_snd_soc_register_component(dev,
> +						       &soc_component_dev_pcm1789,
> +						       &pcm1789_dai, 1);
> +

Perhaps a "else" here ?

>  	return devm_snd_soc_register_component(dev,
>  			&soc_component_dev_pcm179x, &pcm179x_dai, 1);

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Thomas Petazzoni]

Hello,

On Tue, 27 Feb 2018 22:24:32 +0100, Mylène Josserand wrote:
> Add a reset gpio to be able to reset this line at startup.
> 
> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>

This needs an updated of the DT binding, since it's adding support for
a new reset-gpios property.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Fabio Estevam]

On Tue, Feb 27, 2018 at 6:24 PM, Mylène Josserand
<mylene.josserand@bootlin.com> wrote:

> +       pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
> +       if (gpio_is_valid(pcm179x->reset)) {
> +               ret = devm_gpio_request_one(dev, pcm179x->reset,
> +                                           GPIOF_OUT_INIT_LOW,
> +                                           "pcm179x reset");
> +               if (ret) {
> +                       dev_err(dev,
> +                               "Failed to request GPIO %d as reset pin, error %d\n",
> +                               pcm179x->reset, ret);
> +                       return ret;
> +               }
> +
> +               gpio_set_value(pcm179x->reset, 1);

It would be better to use the gpiod API, which takes the GPIO polarity
into account.

There may be systems that have an inverter connected to this pin, and
this can be changed in dts via GPIO_ACTIVE_HIGH.

Also, as the reset pin can be connected to an I2C expander, for
example, so it is safer to use the cansleep variant:

gpiod_set_value_cansleep(pcm179x->reset, 0);

Re: [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id

[Mylène Josserand]

Hello,

Thank you for the review.

On Tue, 27 Feb 2018 22:51:40 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:30 +0100, Mylène Josserand wrote:
> > To prepare the support for the PCM1789, add a new compatible
> > and use the i2c_id to handle, later, the differences between
> > these two DACs even if they are pretty similar.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-  
> 
> The DT binding change should be in a separate patch.
> 
> >  sound/soc/codecs/pcm179x-i2c.c                      | 6 ++++--
> >  sound/soc/codecs/pcm179x.c                          | 3 ++-
> >  sound/soc/codecs/pcm179x.h                          | 8 +++++++-  
> 
> And this should be together with the PCM1789 support patch. Otherwise
> your series is not bisectable: if we apply only PATCH 1/4, the driver
> supports the new compatible string, but it doesn't have the actual code
> to handle PCM1789. Am I missing something here ?

No, you are right, I will merge it with patch 02.

> 
> > -	return pcm179x_common_init(&client->dev, regmap);
> > +	return pcm179x_common_init(&client->dev, regmap, id->driver_data);  
> 
> This can be done in a preparation patch.
> 
> >  }
> >  
> >  static const struct of_device_id pcm179x_of_match[] = {
> >  	{ .compatible = "ti,pcm1792a", },
> > +	{ .compatible = "ti,pcm1789", },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, pcm179x_of_match);
> >  
> >  static const struct i2c_device_id pcm179x_i2c_ids[] = {
> > -	{ "pcm179x", 0 },
> > +	{ "pcm179x", PCM179X },  
> 
> And also this addition.
> 
> > +	{ "pcm1789", PCM1789 },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
> > diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
> > index 4b311c06f97d..81cbf09319f6 100644
> > --- a/sound/soc/codecs/pcm179x.c
> > +++ b/sound/soc/codecs/pcm179x.c
> > @@ -218,7 +218,8 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
> >  	.non_legacy_dai_naming	= 1,
> >  };
> >  
> > -int pcm179x_common_init(struct device *dev, struct regmap *regmap)
> > +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> > +			enum pcm17xx_type type)  
> 
> And this done.
> 
> >  {
> >  	struct pcm179x_private *pcm179x;
> >  
> > diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
> > index cf8681c9a373..8c08689e3b8b 100644
> > --- a/sound/soc/codecs/pcm179x.h
> > +++ b/sound/soc/codecs/pcm179x.h
> > @@ -17,11 +17,17 @@
> >  #ifndef __PCM179X_H__
> >  #define __PCM179X_H__
> >  
> > +enum pcm17xx_type {
> > +	PCM179X,  
> 
> And this one.
> 
> > +	PCM1789,
> > +};
> > +
> >  #define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
> >  			  SNDRV_PCM_FMTBIT_S16_LE)
> >  
> >  extern const struct regmap_config pcm179x_regmap_config;
> >  
> > -int pcm179x_common_init(struct device *dev, struct regmap *regmap);
> > +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> > +			enum pcm17xx_type type);  
> 
> And this one. Just as a "preparation patch" to support multiple codecs
> in the existing pcm179x.c driver.
> 
> Thanks!
> 
> Thomas

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

[Mylène Josserand]

Hello,

On Tue, 27 Feb 2018 22:56:29 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:31 +0100, Mylène Josserand wrote:
> 
> > diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> > index 795a0657c097..83a2e1508df8 100644
> > --- a/sound/soc/codecs/pcm179x-i2c.c
> > +++ b/sound/soc/codecs/pcm179x-i2c.c
> > @@ -26,10 +26,13 @@
> >  static int pcm179x_i2c_probe(struct i2c_client *client,
> >  			      const struct i2c_device_id *id)
> >  {
> > -	struct regmap *regmap;
> > +	struct regmap *regmap = NULL;  
> 
> I don't think this change is useful, since regmap is always initialized
> below anyway.

okay.

> 
> 
> > +	if (mute)
> > +		val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);  
> 
> That's not really useful with regmap_update_bits() which already does
> the masking, no?

Yep

> 
> > +	else
> > +		val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
> > +	ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> > +				 PCM1789_MUTE_MASK, val);  
> 
> Couldn't this be:
> 
> 	if (mute)
> 		val = 0;
> 	else
> 		val = PCM1789_MUTE_MASK;
> 
> 	ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> 				 PCM1789_MUTE_MASK, val);
> 

I will update my V2 with it.

> 
> > +static struct snd_soc_dai_driver pcm1789_dai = {
> > +	.name = "pcm1789-hifi",
> > +	.playback = {
> > +		.stream_name = "Playback",
> > +		.channels_min = 2,
> > +		.channels_max = 2,
> > +		.rates = SNDRV_PCM_RATE_CONTINUOUS,
> > +		.rate_min = 10000,
> > +		.rate_max = 200000,
> > +		.formats = PCM1792A_FORMATS, },  
> 
> Nit: the closing curly brace should be on a separate line.

Yep, thanks.

> 
> 
> > +	if (type == PCM1789)
> > +		return devm_snd_soc_register_component(dev,
> > +						       &soc_component_dev_pcm1789,
> > +						       &pcm1789_dai, 1);
> > +  
> 
> Perhaps a "else" here ?

Sure

> 
> >  	return devm_snd_soc_register_component(dev,
> >  			&soc_component_dev_pcm179x, &pcm179x_dai, 1);  
> 
> Thanks!
> 
> Thomas

Thank you,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Hello,

On Tue, 27 Feb 2018 23:02:00 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:32 +0100, Mylène Josserand wrote:
> > Add a reset gpio to be able to reset this line at startup.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>  
> 
> This needs an updated of the DT binding, since it's adding support for
> a new reset-gpios property.

True, I will add it in my new series.

> 
> Thanks!
> 
> Thomas

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Hello Fabio,

Thank you for the review!

On Tue, 27 Feb 2018 19:30:11 -0300
Fabio Estevam <festevam@gmail.com> wrote:

> On Tue, Feb 27, 2018 at 6:24 PM, Mylène Josserand
> <mylene.josserand@bootlin.com> wrote:
> 
> > +       pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
> > +       if (gpio_is_valid(pcm179x->reset)) {
> > +               ret = devm_gpio_request_one(dev, pcm179x->reset,
> > +                                           GPIOF_OUT_INIT_LOW,
> > +                                           "pcm179x reset");
> > +               if (ret) {
> > +                       dev_err(dev,
> > +                               "Failed to request GPIO %d as reset pin, error %d\n",
> > +                               pcm179x->reset, ret);
> > +                       return ret;
> > +               }
> > +
> > +               gpio_set_value(pcm179x->reset, 1);  
> 
> It would be better to use the gpiod API, which takes the GPIO polarity
> into account.
> 
> There may be systems that have an inverter connected to this pin, and
> this can be changed in dts via GPIO_ACTIVE_HIGH.

Oh, true, I should have used gpiod. Thanks for pointing me out.

> 
> Also, as the reset pin can be connected to an I2C expander, for
> example, so it is safer to use the cansleep variant:
> 
> gpiod_set_value_cansleep(pcm179x->reset, 0);

Sure, I will update it.

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [alsa-devel] [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id

[Michael Nazzareno Trimarchi]

Hi

On Thu, Mar 1, 2018 at 8:43 AM, Mylène Josserand
<mylene.josserand@bootlin.com> wrote:
> Hello,
>
> Thank you for the review.
>
> On Tue, 27 Feb 2018 22:51:40 +0100
> Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
>
>> Hello,
>>
>> On Tue, 27 Feb 2018 22:24:30 +0100, Mylène Josserand wrote:
>> > To prepare the support for the PCM1789, add a new compatible
>> > and use the i2c_id to handle, later, the differences between
>> > these two DACs even if they are pretty similar.
>> >
>> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
>> > ---
>> >  Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-
>>
>> The DT binding change should be in a separate patch.
>>
>> >  sound/soc/codecs/pcm179x-i2c.c                      | 6 ++++--
>> >  sound/soc/codecs/pcm179x.c                          | 3 ++-
>> >  sound/soc/codecs/pcm179x.h                          | 8 +++++++-
>>
>> And this should be together with the PCM1789 support patch. Otherwise
>> your series is not bisectable: if we apply only PATCH 1/4, the driver
>> supports the new compatible string, but it doesn't have the actual code
>> to handle PCM1789. Am I missing something here ?
>
> No, you are right, I will merge it with patch 02.
>

Can you please include me in next series?

I have several hardware running on pcm179x family

Michael

>>
>> > -   return pcm179x_common_init(&client->dev, regmap);
>> > +   return pcm179x_common_init(&client->dev, regmap, id->driver_data);
>>
>> This can be done in a preparation patch.
>>
>> >  }
>> >
>> >  static const struct of_device_id pcm179x_of_match[] = {
>> >     { .compatible = "ti,pcm1792a", },
>> > +   { .compatible = "ti,pcm1789", },
>> >     { }
>> >  };
>> >  MODULE_DEVICE_TABLE(of, pcm179x_of_match);
>> >
>> >  static const struct i2c_device_id pcm179x_i2c_ids[] = {
>> > -   { "pcm179x", 0 },
>> > +   { "pcm179x", PCM179X },
>>
>> And also this addition.
>>
>> > +   { "pcm1789", PCM1789 },
>> >     { }
>> >  };
>> >  MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
>> > diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
>> > index 4b311c06f97d..81cbf09319f6 100644
>> > --- a/sound/soc/codecs/pcm179x.c
>> > +++ b/sound/soc/codecs/pcm179x.c
>> > @@ -218,7 +218,8 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
>> >     .non_legacy_dai_naming  = 1,
>> >  };
>> >
>> > -int pcm179x_common_init(struct device *dev, struct regmap *regmap)
>> > +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
>> > +                   enum pcm17xx_type type)
>>
>> And this done.
>>
>> >  {
>> >     struct pcm179x_private *pcm179x;
>> >
>> > diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
>> > index cf8681c9a373..8c08689e3b8b 100644
>> > --- a/sound/soc/codecs/pcm179x.h
>> > +++ b/sound/soc/codecs/pcm179x.h
>> > @@ -17,11 +17,17 @@
>> >  #ifndef __PCM179X_H__
>> >  #define __PCM179X_H__
>> >
>> > +enum pcm17xx_type {
>> > +   PCM179X,
>>
>> And this one.
>>
>> > +   PCM1789,
>> > +};
>> > +
>> >  #define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
>> >                       SNDRV_PCM_FMTBIT_S16_LE)
>> >
>> >  extern const struct regmap_config pcm179x_regmap_config;
>> >
>> > -int pcm179x_common_init(struct device *dev, struct regmap *regmap);
>> > +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
>> > +                   enum pcm17xx_type type);
>>
>> And this one. Just as a "preparation patch" to support multiple codecs
>> in the existing pcm179x.c driver.
>>
>> Thanks!
>>
>> Thomas
>
> Thanks,
>
> Mylène
>
> --
> Mylène Josserand, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> http://bootlin.com
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [alsa-devel] [PATCH v1 1/4] ASoC: codecs: pcm179x: Add PCM1789 id

[Mylène Josserand]

Hello,

On Thu, 1 Mar 2018 10:04:11 +0100
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi
> 
> On Thu, Mar 1, 2018 at 8:43 AM, Mylène Josserand
> <mylene.josserand@bootlin.com> wrote:
> > Hello,
> >
> > Thank you for the review.
> >
> > On Tue, 27 Feb 2018 22:51:40 +0100
> > Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
> >  
> >> Hello,
> >>
> >> On Tue, 27 Feb 2018 22:24:30 +0100, Mylène Josserand wrote:  
> >> > To prepare the support for the PCM1789, add a new compatible
> >> > and use the i2c_id to handle, later, the differences between
> >> > these two DACs even if they are pretty similar.
> >> >
> >> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> >> > ---
> >> >  Documentation/devicetree/bindings/sound/pcm179x.txt | 2 +-  
> >>
> >> The DT binding change should be in a separate patch.
> >>  
> >> >  sound/soc/codecs/pcm179x-i2c.c                      | 6 ++++--
> >> >  sound/soc/codecs/pcm179x.c                          | 3 ++-
> >> >  sound/soc/codecs/pcm179x.h                          | 8 +++++++-  
> >>
> >> And this should be together with the PCM1789 support patch. Otherwise
> >> your series is not bisectable: if we apply only PATCH 1/4, the driver
> >> supports the new compatible string, but it doesn't have the actual code
> >> to handle PCM1789. Am I missing something here ?  
> >
> > No, you are right, I will merge it with patch 02.
> >  
> 
> Can you please include me in next series?

Sure, I will do it.

> 
> I have several hardware running on pcm179x family

Currently, some colleagues recommend me to create a new file for this
driver instead of adding it in PCM179x driver. I think that I will not
touch pcm179x driver anymore in my next series.

Best regards,

Mylène

> 
> Michael
> 
> >>  
> >> > -   return pcm179x_common_init(&client->dev, regmap);
> >> > +   return pcm179x_common_init(&client->dev, regmap, id->driver_data);  
> >>
> >> This can be done in a preparation patch.
> >>  
> >> >  }
> >> >
> >> >  static const struct of_device_id pcm179x_of_match[] = {
> >> >     { .compatible = "ti,pcm1792a", },
> >> > +   { .compatible = "ti,pcm1789", },
> >> >     { }
> >> >  };
> >> >  MODULE_DEVICE_TABLE(of, pcm179x_of_match);
> >> >
> >> >  static const struct i2c_device_id pcm179x_i2c_ids[] = {
> >> > -   { "pcm179x", 0 },
> >> > +   { "pcm179x", PCM179X },  
> >>
> >> And also this addition.
> >>  
> >> > +   { "pcm1789", PCM1789 },
> >> >     { }
> >> >  };
> >> >  MODULE_DEVICE_TABLE(i2c, pcm179x_i2c_ids);
> >> > diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
> >> > index 4b311c06f97d..81cbf09319f6 100644
> >> > --- a/sound/soc/codecs/pcm179x.c
> >> > +++ b/sound/soc/codecs/pcm179x.c
> >> > @@ -218,7 +218,8 @@ static const struct snd_soc_component_driver soc_component_dev_pcm179x = {
> >> >     .non_legacy_dai_naming  = 1,
> >> >  };
> >> >
> >> > -int pcm179x_common_init(struct device *dev, struct regmap *regmap)
> >> > +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> >> > +                   enum pcm17xx_type type)  
> >>
> >> And this done.
> >>  
> >> >  {
> >> >     struct pcm179x_private *pcm179x;
> >> >
> >> > diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
> >> > index cf8681c9a373..8c08689e3b8b 100644
> >> > --- a/sound/soc/codecs/pcm179x.h
> >> > +++ b/sound/soc/codecs/pcm179x.h
> >> > @@ -17,11 +17,17 @@
> >> >  #ifndef __PCM179X_H__
> >> >  #define __PCM179X_H__
> >> >
> >> > +enum pcm17xx_type {
> >> > +   PCM179X,  
> >>
> >> And this one.
> >>  
> >> > +   PCM1789,
> >> > +};
> >> > +
> >> >  #define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
> >> >                       SNDRV_PCM_FMTBIT_S16_LE)
> >> >
> >> >  extern const struct regmap_config pcm179x_regmap_config;
> >> >
> >> > -int pcm179x_common_init(struct device *dev, struct regmap *regmap);
> >> > +int pcm179x_common_init(struct device *dev, struct regmap *regmap,
> >> > +                   enum pcm17xx_type type);  
> >>
> >> And this one. Just as a "preparation patch" to support multiple codecs
> >> in the existing pcm179x.c driver.
> >>
> >> Thanks!
> >>
> >> Thomas  
> >
> > Thanks,
> >
> > Mylène
> >
> > --
> > Mylène Josserand, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > http://bootlin.com
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel  
> 
> 
> 

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com