[alsa-devel] [PATCH v1 0/4] ASoC: Add support for DAC PCM1789
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(-)
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
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
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
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
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
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);
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
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
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,
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
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
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);
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
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
participants (4)
-
Fabio Estevam -
Michael Nazzareno Trimarchi -
Mylène Josserand -
Thomas Petazzoni