Skip to contentSkip to conversation

Add Headphone Detection to TLV320AIC31xx Driver

PATCH Superseded Andrew F. Davis 2 participants 63 messages Nov 29, 2017 – Dec 7, 2017

Subsystems (4)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover Add Headphone Detection to TLV320AIC31xx Driver

Hello all,

This series has the end goal of adding headphone detection to the tlv320aic31xx driver. The first few patches are mostly cleanups. Then a couple bug fixes I noticed. Followed by adding interrupt handling and finally headphone detection.

This series (or at least patch #8) depend on this DT fix[0].

Thanks,
Andrew

Changes from v1:
- Splitup the cleanup patch a bit more
- Move the GPIO1 register fix patch before header cleanup so it can be taken back into stable
- Added Acked-by
- New patch dealing with regulator notifications
- Various small touchups
- Rebased on v4.15-rc1 + [0]

[0] http://www.spinics.net/lists/kernel/msg2660968.html

Andrew F. Davis (19):
ASoC: tlv320aic31xx: File header and copyright cleanup
ASoC: tlv320aic31xx: Change aic31xx_power_off return type to void
ASoC: tlv320aic31xx: Move ACPI table next to OF table
ASoC: tlv320aic31xx: General source formatting cleanup
ASoC: tlv320aic31xx: Fix GPIO1 register definition
ASoC: tlv320aic31xx: Reformat header file using GENMASK and BIT macros
ASoC: tlv320aic31xx: Merge init function into probe
ASoC: tlv320aic31xx: Switch GPIO handling to use gpiod_* API
ASoC: tlv320aic31xx: Remove platform data
ASoC: tlv320aic31xx: Add MICBIAS off setting
ASoC: tlv320aic31xx: Check clock and divider before division
ASoC: tlv320aic31xx: Add CODEC clock slave support
ASoC: tlv320aic31xx: Fix inverted BCLK handling
ASoC: tlv320aic31xx: Remove regulator notification handling
ASoC: tlv320aic31xx: Reset registers during power up
ASoC: tlv320aic31xx: Add short circuit detection support
ASoC: tlv320aic31xx: Add overflow detection support
ASoC: tlv320aic31xx: Add headphone/headset detection
ASoC: tlv320aic31xx: Add button press detection

 .../devicetree/bindings/sound/tlv320aic31xx.txt    |   1 +
 include/dt-bindings/sound/tlv320aic31xx-micbias.h  |   1 +
 sound/soc/codecs/tlv320aic31xx.c                   | 422 ++++++++++--------
 sound/soc/codecs/tlv320aic31xx.h                   | 496 ++++++++++-----------
 4 files changed, 471 insertions(+), 449 deletions(-)

rewrite sound/soc/codecs/tlv320aic31xx.h (83%)

Signature · 1 line
2.15.0
#1 lore raw

PATCH v2 1/19 ASoC: tlv320aic31xx: File header and copyright cleanup

Fix header copyright tags, while we are here, also switch to SPDX and fixup MODULE tags to match.

---
 sound/soc/codecs/tlv320aic31xx.c | 26 +++++++++-----------------
 sound/soc/codecs/tlv320aic31xx.h | 15 ++++-----------
 2 files changed, 13 insertions(+), 28 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +9 -17
index 4837f25b0760..b98d9b1f216f 100644
@@ -1,22 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * ALSA SoC TLV320AIC31XX codec driver
+ * ALSA SoC TLV320AIC31xx CODEC Driver
  *
- * Copyright (C) 2014 Texas Instruments, Inc.
- *
- * Author: Jyri Sarha <[email protected]>
+ * Copyright (C) 2014-2017 Texas Instruments Incorporated - http://www.ti.com/
+ *	Jyri Sarha <[email protected]>
  *
  * Based on ground work by: Ajit Kulkarni <[email protected]>
  *
- * This package 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.
- *
- * THIS PACKAGE IS PROVIDED AS IS AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
- *
- * The TLV320AIC31xx series of audio codec is a low-power, highly integrated
- * high performance codec which provides a stereo DAC, a mono ADC,
+ * The TLV320AIC31xx series of audio codecs are low-power, highly integrated
+ * high performance codecs which provides a stereo DAC, a mono ADC,
  * and mono/stereo Class-D speaker driver.
  */
 
@@ -1414,6 +1406,6 @@ static struct i2c_driver aic31xx_i2c_driver = {
 
 module_i2c_driver(aic31xx_i2c_driver);
 
-MODULE_DESCRIPTION("ASoC TLV320AIC3111 codec driver");
-MODULE_AUTHOR("Jyri Sarha");
-MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jyri Sarha <[email protected]>");
+MODULE_DESCRIPTION("ASoC TLV320AIC31xx CODEC Driver");
+MODULE_LICENSE("GPL v2");
sound/soc/codecs/tlv320aic31xx.h +4 -11
index 730fb2058869..a9ea2f99eba0 100644
@@ -1,17 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * ALSA SoC TLV320AIC31XX codec driver
- *
- * Copyright (C) 2013 Texas Instruments, Inc.
- *
- * This package 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.
- *
- * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
+ * ALSA SoC TLV320AIC31xx CODEC Driver Definitions
  *
+ * Copyright (C) 2014-2017 Texas Instruments Incorporated - http://www.ti.com/
  */
+
 #ifndef _TLV320AIC31XX_H
 #define _TLV320AIC31XX_H
 
Signature · 1 line
2.15.0
maintainer
#2 lore raw

Re: [PATCH v2 01/19] ASoC: tlv320aic31xx: File header and copyright cleanup

Quoted 7 lines · Andrew F. Davis

Fix header copyright tags, while we are here, also switch to SPDX and fixup MODULE tags to match.

- * The TLV320AIC31xx series of audio codec is a low-power, highly integrated
- * high performance codec which provides a stereo DAC, a mono ADC,
+ * The TLV320AIC31xx series of audio codecs are low-power, highly integrated
+ * high performance codecs which provides a stereo DAC, a mono ADC,

...and also correct some grammar in the description of the part.

maintainer
#3 lore raw

Applied "ASoC: tlv320aic31xx: File header and copyright cleanup" to the asoc tree

In reply to Andrew F. Davis

The patch

ASoC: tlv320aic31xx: File header and copyright cleanup

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From b1c52b7e7cc6a57128869008b84d98a4de78fe2d Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:42 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: File header and copyright cleanup

Fix header copyright tags, while we are here, also switch to SPDX and fixup MODULE tags to match.

---
 sound/soc/codecs/tlv320aic31xx.c | 26 +++++++++-----------------
 sound/soc/codecs/tlv320aic31xx.h | 15 ++++-----------
 2 files changed, 13 insertions(+), 28 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +9 -17
index 4837f25b0760..b98d9b1f216f 100644
@@ -1,22 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * ALSA SoC TLV320AIC31XX codec driver
+ * ALSA SoC TLV320AIC31xx CODEC Driver
  *
- * Copyright (C) 2014 Texas Instruments, Inc.
- *
- * Author: Jyri Sarha <[email protected]>
+ * Copyright (C) 2014-2017 Texas Instruments Incorporated - http://www.ti.com/
+ *	Jyri Sarha <[email protected]>
  *
  * Based on ground work by: Ajit Kulkarni <[email protected]>
  *
- * This package 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.
- *
- * THIS PACKAGE IS PROVIDED AS IS AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
- *
- * The TLV320AIC31xx series of audio codec is a low-power, highly integrated
- * high performance codec which provides a stereo DAC, a mono ADC,
+ * The TLV320AIC31xx series of audio codecs are low-power, highly integrated
+ * high performance codecs which provides a stereo DAC, a mono ADC,
  * and mono/stereo Class-D speaker driver.
  */
 
@@ -1414,6 +1406,6 @@ static struct i2c_driver aic31xx_i2c_driver = {
 
 module_i2c_driver(aic31xx_i2c_driver);
 
-MODULE_DESCRIPTION("ASoC TLV320AIC3111 codec driver");
-MODULE_AUTHOR("Jyri Sarha");
-MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jyri Sarha <[email protected]>");
+MODULE_DESCRIPTION("ASoC TLV320AIC31xx CODEC Driver");
+MODULE_LICENSE("GPL v2");
sound/soc/codecs/tlv320aic31xx.h +4 -11
index 730fb2058869..a9ea2f99eba0 100644
@@ -1,17 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- * ALSA SoC TLV320AIC31XX codec driver
- *
- * Copyright (C) 2013 Texas Instruments, Inc.
- *
- * This package 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.
- *
- * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
- * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
- * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
+ * ALSA SoC TLV320AIC31xx CODEC Driver Definitions
  *
+ * Copyright (C) 2014-2017 Texas Instruments Incorporated - http://www.ti.com/
  */
+
 #ifndef _TLV320AIC31XX_H
 #define _TLV320AIC31XX_H
 
Signature · 1 line
2.15.0
#4 lore raw

PATCH v2 2/19 ASoC: tlv320aic31xx: Change aic31xx_power_off return type to void

In reply to Andrew F. Davis

The return value is not checked, and even if it was there is nothing we could do about it and messages are already printed.

---
 sound/soc/codecs/tlv320aic31xx.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +3 -6
index b98d9b1f216f..0563a49cc5e4 100644
@@ -1081,16 +1081,13 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	return 0;
 }
 
-static int aic31xx_power_off(struct snd_soc_codec *codec)
+static void aic31xx_power_off(struct snd_soc_codec *codec)
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int ret = 0;
 
 	regcache_cache_only(aic31xx->regmap, true);
-	ret = regulator_bulk_disable(ARRAY_SIZE(aic31xx->supplies),
-				     aic31xx->supplies);
-
-	return ret;
+	regulator_bulk_disable(ARRAY_SIZE(aic31xx->supplies),
+			       aic31xx->supplies);
 }
 
 static int aic31xx_set_bias_level(struct snd_soc_codec *codec,
Signature · 1 line
2.15.0
maintainer
#5 lore raw

Applied "ASoC: tlv320aic31xx: Change aic31xx_power_off return type to void" to the asoc tree

The patch

ASoC: tlv320aic31xx: Change aic31xx_power_off return type to void

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From c7734e8e7eddf065c15e865b8de4224b01f03409 Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:43 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: Change aic31xx_power_off return type to
void

The return value is not checked, and even if it was there is nothing we could do about it and messages are already printed.

---
 sound/soc/codecs/tlv320aic31xx.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +3 -6
index b98d9b1f216f..0563a49cc5e4 100644
@@ -1081,16 +1081,13 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	return 0;
 }
 
-static int aic31xx_power_off(struct snd_soc_codec *codec)
+static void aic31xx_power_off(struct snd_soc_codec *codec)
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int ret = 0;
 
 	regcache_cache_only(aic31xx->regmap, true);
-	ret = regulator_bulk_disable(ARRAY_SIZE(aic31xx->supplies),
-				     aic31xx->supplies);
-
-	return ret;
+	regulator_bulk_disable(ARRAY_SIZE(aic31xx->supplies),
+			       aic31xx->supplies);
 }
 
 static int aic31xx_set_bias_level(struct snd_soc_codec *codec,
Signature · 1 line
2.15.0
#6 lore raw

PATCH v2 3/19 ASoC: tlv320aic31xx: Move ACPI table next to OF table

In reply to Andrew F. Davis
---
 sound/soc/codecs/tlv320aic31xx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +8 -8
index 0563a49cc5e4..d974e8651e30 100644
@@ -1285,6 +1285,14 @@ static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx)
 }
 #endif /* CONFIG_OF */
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id aic31xx_acpi_match[] = {
+	{ "10TI3100", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, aic31xx_acpi_match);
+#endif
+
 static int aic31xx_device_init(struct aic31xx_priv *aic31xx)
 {
 	int ret, i;
@@ -1382,14 +1390,6 @@ static const struct i2c_device_id aic31xx_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, aic31xx_i2c_id);
 
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id aic31xx_acpi_match[] = {
-	{ "10TI3100", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, aic31xx_acpi_match);
-#endif
-
 static struct i2c_driver aic31xx_i2c_driver = {
 	.driver = {
 		.name	= "tlv320aic31xx-codec",
Signature · 6 lines
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#7 lore raw

Applied "ASoC: tlv320aic31xx: Move ACPI table next to OF table" to the asoc tree

The patch

ASoC: tlv320aic31xx: Move ACPI table next to OF table

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 09303601bacda714220048a61f6864f88594b231 Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:44 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: Move ACPI table next to OF table

---
 sound/soc/codecs/tlv320aic31xx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +8 -8
index 0563a49cc5e4..d974e8651e30 100644
@@ -1285,6 +1285,14 @@ static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx)
 }
 #endif /* CONFIG_OF */
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id aic31xx_acpi_match[] = {
+	{ "10TI3100", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, aic31xx_acpi_match);
+#endif
+
 static int aic31xx_device_init(struct aic31xx_priv *aic31xx)
 {
 	int ret, i;
@@ -1382,14 +1390,6 @@ static const struct i2c_device_id aic31xx_i2c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, aic31xx_i2c_id);
 
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id aic31xx_acpi_match[] = {
-	{ "10TI3100", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, aic31xx_acpi_match);
-#endif
-
 static struct i2c_driver aic31xx_i2c_driver = {
 	.driver = {
 		.name	= "tlv320aic31xx-codec",
Signature · 1 line
2.15.0
#8 lore raw

PATCH v2 4/19 ASoC: tlv320aic31xx: General source formatting cleanup

In reply to Andrew F. Davis

Simple non-functional changes including:

* Fix spelling errors
* Reformat code for easier reading
* Remove unneeded code
* Remove assignments that are always overridden
* Normalize function return paths

---
 sound/soc/codecs/tlv320aic31xx.c | 63 ++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 28 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +35 -28
index d974e8651e30..07c014501e5e 100644
@@ -136,8 +136,7 @@ static const struct regmap_config aic31xx_i2c_regmap = {
 	.max_register = 12 * 128,
 };
 
-#define AIC31XX_NUM_SUPPLIES	6
-static const char * const aic31xx_supply_names[AIC31XX_NUM_SUPPLIES] = {
+static const char * const aic31xx_supply_names[] = {
 	"HPVDD",
 	"SPRVDD",
 	"SPLVDD",
@@ -146,6 +145,8 @@ static const char * const aic31xx_supply_names[AIC31XX_NUM_SUPPLIES] = {
 	"DVDD",
 };
 
+#define AIC31XX_NUM_SUPPLIES ARRAY_SIZE(aic31xx_supply_names)
+
 struct aic31xx_disable_nb {
 	struct notifier_block nb;
 	struct aic31xx_priv *aic31xx;
@@ -177,7 +178,7 @@ struct aic31xx_rate_divs {
 	u8 madc;
 };
 
-/* ADC dividers can be disabled by cofiguring them to 0 */
+/* ADC dividers can be disabled by configuring them to 0 */
 static const struct aic31xx_rate_divs aic31xx_divs[] = {
 	/* mclk/p    rate  pll: j     d        dosr ndac mdac  aors nadc madc */
 	/* 8k rate */
@@ -832,11 +833,17 @@ static int aic31xx_setup_pll(struct snd_soc_codec *codec,
 
 	dev_dbg(codec->dev,
 		"pll %d.%04d/%d dosr %d n %d m %d aosr %d n %d m %d bclk_n %d\n",
-		aic31xx_divs[i].pll_j, aic31xx_divs[i].pll_d,
-		aic31xx->p_div, aic31xx_divs[i].dosr,
-		aic31xx_divs[i].ndac, aic31xx_divs[i].mdac,
-		aic31xx_divs[i].aosr, aic31xx_divs[i].nadc,
-		aic31xx_divs[i].madc, bclk_n);
+		aic31xx_divs[i].pll_j,
+		aic31xx_divs[i].pll_d,
+		aic31xx->p_div,
+		aic31xx_divs[i].dosr,
+		aic31xx_divs[i].ndac,
+		aic31xx_divs[i].mdac,
+		aic31xx_divs[i].aosr,
+		aic31xx_divs[i].nadc,
+		aic31xx_divs[i].madc,
+		bclk_n
+	);
 
 	return 0;
 }
@@ -973,8 +980,9 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	dev_dbg(codec->dev, "## %s: clk_id = %d, freq = %d, dir = %d\n",
 		__func__, clk_id, freq, dir);
 
-	for (i = 1; freq/i > 20000000 && i < 8; i++)
-		;
+	for (i = 1; i < 8; i++)
+		if (freq / i <= 20000000)
+			break;
 	if (freq/i > 20000000) {
 		dev_err(aic31xx->dev, "%s: Too high mclk frequency %u\n",
 			__func__, freq);
@@ -982,9 +990,9 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	}
 	aic31xx->p_div = i;
 
-	for (i = 0; i < ARRAY_SIZE(aic31xx_divs) &&
-		     aic31xx_divs[i].mclk_p != freq/aic31xx->p_div; i++)
-		;
+	for (i = 0; i < ARRAY_SIZE(aic31xx_divs); i++)
+		if (aic31xx_divs[i].mclk_p == freq / aic31xx->p_div)
+			break;
 	if (i == ARRAY_SIZE(aic31xx_divs)) {
 		dev_err(aic31xx->dev, "%s: Unsupported frequency %d\n",
 			__func__, freq);
@@ -996,6 +1004,7 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 			    clk_id << AIC31XX_PLL_CLKIN_SHIFT);
 
 	aic31xx->sysclk = freq;
+
 	return 0;
 }
 
@@ -1057,7 +1066,7 @@ static void aic31xx_clk_off(struct snd_soc_codec *codec)
 static int aic31xx_power_on(struct snd_soc_codec *codec)
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int ret = 0;
+	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(aic31xx->supplies),
 				    aic31xx->supplies);
@@ -1070,7 +1079,7 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	}
 	regcache_cache_only(aic31xx->regmap, false);
 	ret = regcache_sync(aic31xx->regmap);
-	if (ret != 0) {
+	if (ret) {
 		dev_err(codec->dev,
 			"Failed to restore cache: %d\n", ret);
 		regcache_cache_only(aic31xx->regmap, true);
@@ -1078,6 +1087,7 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 				       aic31xx->supplies);
 		return ret;
 	}
+
 	return 0;
 }
 
@@ -1126,14 +1136,11 @@ static int aic31xx_set_bias_level(struct snd_soc_codec *codec,
 
 static int aic31xx_codec_probe(struct snd_soc_codec *codec)
 {
-	int ret = 0;
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int i;
+	int i, ret;
 
 	dev_dbg(aic31xx->dev, "## %s\n", __func__);
 
-	aic31xx = snd_soc_codec_get_drvdata(codec);
-
 	aic31xx->codec = codec;
 
 	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++) {
@@ -1158,8 +1165,10 @@ static int aic31xx_codec_probe(struct snd_soc_codec *codec)
 		return ret;
 
 	ret = aic31xx_add_widgets(codec);
+	if (ret)
+		return ret;
 
-	return ret;
+	return 0;
 }
 
 static int aic31xx_codec_remove(struct snd_soc_codec *codec)
@@ -1322,10 +1331,12 @@ static int aic31xx_device_init(struct aic31xx_priv *aic31xx)
 	ret = devm_regulator_bulk_get(aic31xx->dev,
 				      ARRAY_SIZE(aic31xx->supplies),
 				      aic31xx->supplies);
-	if (ret != 0)
+	if (ret) {
 		dev_err(aic31xx->dev, "Failed to request supplies: %d\n", ret);
+		return ret;
+	}
 
-	return ret;
+	return 0;
 }
 
 static int aic31xx_i2c_probe(struct i2c_client *i2c,
@@ -1333,18 +1344,15 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 {
 	struct aic31xx_priv *aic31xx;
 	int ret;
-	const struct regmap_config *regmap_config;
 
 	dev_dbg(&i2c->dev, "## %s: %s codec_type = %d\n", __func__,
 		id->name, (int) id->driver_data);
 
-	regmap_config = &aic31xx_i2c_regmap;
-
 	aic31xx = devm_kzalloc(&i2c->dev, sizeof(*aic31xx), GFP_KERNEL);
-	if (aic31xx == NULL)
+	if (!aic31xx)
 		return -ENOMEM;
 
-	aic31xx->regmap = devm_regmap_init_i2c(i2c, regmap_config);
+	aic31xx->regmap = devm_regmap_init_i2c(i2c, &aic31xx_i2c_regmap);
 	if (IS_ERR(aic31xx->regmap)) {
 		ret = PTR_ERR(aic31xx->regmap);
 		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
@@ -1400,7 +1408,6 @@ static struct i2c_driver aic31xx_i2c_driver = {
 	.remove		= aic31xx_i2c_remove,
 	.id_table	= aic31xx_i2c_id,
 };
-
 module_i2c_driver(aic31xx_i2c_driver);
 
 MODULE_AUTHOR("Jyri Sarha <[email protected]>");
Signature · 1 line
2.15.0
maintainer
#9 lore raw

Applied "ASoC: tlv320aic31xx: General source formatting cleanup" to the asoc tree

The patch

ASoC: tlv320aic31xx: General source formatting cleanup

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From ca7840fb47208a3521f3b60c1b78a2f0c59b4dc5 Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:45 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: General source formatting cleanup

Simple non-functional changes including:

* Fix spelling errors
* Reformat code for easier reading
* Remove unneeded code
* Remove assignments that are always overridden
* Normalize function return paths

---
 sound/soc/codecs/tlv320aic31xx.c | 63 ++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 28 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +35 -28
index d974e8651e30..07c014501e5e 100644
@@ -136,8 +136,7 @@ static const struct regmap_config aic31xx_i2c_regmap = {
 	.max_register = 12 * 128,
 };
 
-#define AIC31XX_NUM_SUPPLIES	6
-static const char * const aic31xx_supply_names[AIC31XX_NUM_SUPPLIES] = {
+static const char * const aic31xx_supply_names[] = {
 	"HPVDD",
 	"SPRVDD",
 	"SPLVDD",
@@ -146,6 +145,8 @@ static const char * const aic31xx_supply_names[AIC31XX_NUM_SUPPLIES] = {
 	"DVDD",
 };
 
+#define AIC31XX_NUM_SUPPLIES ARRAY_SIZE(aic31xx_supply_names)
+
 struct aic31xx_disable_nb {
 	struct notifier_block nb;
 	struct aic31xx_priv *aic31xx;
@@ -177,7 +178,7 @@ struct aic31xx_rate_divs {
 	u8 madc;
 };
 
-/* ADC dividers can be disabled by cofiguring them to 0 */
+/* ADC dividers can be disabled by configuring them to 0 */
 static const struct aic31xx_rate_divs aic31xx_divs[] = {
 	/* mclk/p    rate  pll: j     d        dosr ndac mdac  aors nadc madc */
 	/* 8k rate */
@@ -832,11 +833,17 @@ static int aic31xx_setup_pll(struct snd_soc_codec *codec,
 
 	dev_dbg(codec->dev,
 		"pll %d.%04d/%d dosr %d n %d m %d aosr %d n %d m %d bclk_n %d\n",
-		aic31xx_divs[i].pll_j, aic31xx_divs[i].pll_d,
-		aic31xx->p_div, aic31xx_divs[i].dosr,
-		aic31xx_divs[i].ndac, aic31xx_divs[i].mdac,
-		aic31xx_divs[i].aosr, aic31xx_divs[i].nadc,
-		aic31xx_divs[i].madc, bclk_n);
+		aic31xx_divs[i].pll_j,
+		aic31xx_divs[i].pll_d,
+		aic31xx->p_div,
+		aic31xx_divs[i].dosr,
+		aic31xx_divs[i].ndac,
+		aic31xx_divs[i].mdac,
+		aic31xx_divs[i].aosr,
+		aic31xx_divs[i].nadc,
+		aic31xx_divs[i].madc,
+		bclk_n
+	);
 
 	return 0;
 }
@@ -973,8 +980,9 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	dev_dbg(codec->dev, "## %s: clk_id = %d, freq = %d, dir = %d\n",
 		__func__, clk_id, freq, dir);
 
-	for (i = 1; freq/i > 20000000 && i < 8; i++)
-		;
+	for (i = 1; i < 8; i++)
+		if (freq / i <= 20000000)
+			break;
 	if (freq/i > 20000000) {
 		dev_err(aic31xx->dev, "%s: Too high mclk frequency %u\n",
 			__func__, freq);
@@ -982,9 +990,9 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	}
 	aic31xx->p_div = i;
 
-	for (i = 0; i < ARRAY_SIZE(aic31xx_divs) &&
-		     aic31xx_divs[i].mclk_p != freq/aic31xx->p_div; i++)
-		;
+	for (i = 0; i < ARRAY_SIZE(aic31xx_divs); i++)
+		if (aic31xx_divs[i].mclk_p == freq / aic31xx->p_div)
+			break;
 	if (i == ARRAY_SIZE(aic31xx_divs)) {
 		dev_err(aic31xx->dev, "%s: Unsupported frequency %d\n",
 			__func__, freq);
@@ -996,6 +1004,7 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 			    clk_id << AIC31XX_PLL_CLKIN_SHIFT);
 
 	aic31xx->sysclk = freq;
+
 	return 0;
 }
 
@@ -1057,7 +1066,7 @@ static void aic31xx_clk_off(struct snd_soc_codec *codec)
 static int aic31xx_power_on(struct snd_soc_codec *codec)
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int ret = 0;
+	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(aic31xx->supplies),
 				    aic31xx->supplies);
@@ -1070,7 +1079,7 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	}
 	regcache_cache_only(aic31xx->regmap, false);
 	ret = regcache_sync(aic31xx->regmap);
-	if (ret != 0) {
+	if (ret) {
 		dev_err(codec->dev,
 			"Failed to restore cache: %d\n", ret);
 		regcache_cache_only(aic31xx->regmap, true);
@@ -1078,6 +1087,7 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 				       aic31xx->supplies);
 		return ret;
 	}
+
 	return 0;
 }
 
@@ -1126,14 +1136,11 @@ static int aic31xx_set_bias_level(struct snd_soc_codec *codec,
 
 static int aic31xx_codec_probe(struct snd_soc_codec *codec)
 {
-	int ret = 0;
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int i;
+	int i, ret;
 
 	dev_dbg(aic31xx->dev, "## %s\n", __func__);
 
-	aic31xx = snd_soc_codec_get_drvdata(codec);
-
 	aic31xx->codec = codec;
 
 	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++) {
@@ -1158,8 +1165,10 @@ static int aic31xx_codec_probe(struct snd_soc_codec *codec)
 		return ret;
 
 	ret = aic31xx_add_widgets(codec);
+	if (ret)
+		return ret;
 
-	return ret;
+	return 0;
 }
 
 static int aic31xx_codec_remove(struct snd_soc_codec *codec)
@@ -1322,10 +1331,12 @@ static int aic31xx_device_init(struct aic31xx_priv *aic31xx)
 	ret = devm_regulator_bulk_get(aic31xx->dev,
 				      ARRAY_SIZE(aic31xx->supplies),
 				      aic31xx->supplies);
-	if (ret != 0)
+	if (ret) {
 		dev_err(aic31xx->dev, "Failed to request supplies: %d\n", ret);
+		return ret;
+	}
 
-	return ret;
+	return 0;
 }
 
 static int aic31xx_i2c_probe(struct i2c_client *i2c,
@@ -1333,18 +1344,15 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 {
 	struct aic31xx_priv *aic31xx;
 	int ret;
-	const struct regmap_config *regmap_config;
 
 	dev_dbg(&i2c->dev, "## %s: %s codec_type = %d\n", __func__,
 		id->name, (int) id->driver_data);
 
-	regmap_config = &aic31xx_i2c_regmap;
-
 	aic31xx = devm_kzalloc(&i2c->dev, sizeof(*aic31xx), GFP_KERNEL);
-	if (aic31xx == NULL)
+	if (!aic31xx)
 		return -ENOMEM;
 
-	aic31xx->regmap = devm_regmap_init_i2c(i2c, regmap_config);
+	aic31xx->regmap = devm_regmap_init_i2c(i2c, &aic31xx_i2c_regmap);
 	if (IS_ERR(aic31xx->regmap)) {
 		ret = PTR_ERR(aic31xx->regmap);
 		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
@@ -1400,7 +1408,6 @@ static struct i2c_driver aic31xx_i2c_driver = {
 	.remove		= aic31xx_i2c_remove,
 	.id_table	= aic31xx_i2c_id,
 };
-
 module_i2c_driver(aic31xx_i2c_driver);
 
 MODULE_AUTHOR("Jyri Sarha <[email protected]>");
Signature · 1 line
2.15.0
#10 lore raw

PATCH v2 5/19 ASoC: tlv320aic31xx: Fix GPIO1 register definition

In reply to Andrew F. Davis

GPIO1 control register is number 51, fix this here.

Fixes bafcbfe429eb ("ASoC: tlv320aic31xx: Make the register values human readable")
---
 sound/soc/codecs/tlv320aic31xx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/tlv320aic31xx.h +1 -1
index a9ea2f99eba0..6efea0485392 100644
@@ -109,7 +109,7 @@ struct aic31xx_pdata {
 /* INT2 interrupt control */
 #define AIC31XX_INT2CTRL	AIC31XX_REG(0, 49)
 /* GPIO1 control */
-#define AIC31XX_GPIO1		AIC31XX_REG(0, 50)
+#define AIC31XX_GPIO1		AIC31XX_REG(0, 51)
 
 #define AIC31XX_DACPRB		AIC31XX_REG(0, 60)
 /* ADC Instruction Set Register */
Signature · 1 line
2.15.0
maintainer
#11 lore raw

Applied "ASoC: tlv320aic31xx: Fix GPIO1 register definition" to the asoc tree

The patch

ASoC: tlv320aic31xx: Fix GPIO1 register definition

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 737e0b7b67bdfe24090fab2852044bb283282fc5 Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:46 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: Fix GPIO1 register definition

GPIO1 control register is number 51, fix this here.

Fixes bafcbfe429eb ("ASoC: tlv320aic31xx: Make the register values human readable")
---
 sound/soc/codecs/tlv320aic31xx.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/tlv320aic31xx.h +1 -1
index 730fb2058869..1ff3edb7bbb6 100644
@@ -116,7 +116,7 @@ struct aic31xx_pdata {
 /* INT2 interrupt control */
 #define AIC31XX_INT2CTRL	AIC31XX_REG(0, 49)
 /* GPIO1 control */
-#define AIC31XX_GPIO1		AIC31XX_REG(0, 50)
+#define AIC31XX_GPIO1		AIC31XX_REG(0, 51)
 
 #define AIC31XX_DACPRB		AIC31XX_REG(0, 60)
 /* ADC Instruction Set Register */
Signature · 1 line
2.15.0
#12 lore raw

PATCH v2 6/19 ASoC: tlv320aic31xx: Reformat header file using GENMASK and BIT macros

In reply to Andrew F. Davis

We also move the comments describing the registers to after the register definition to remove non-uniform vertical white-space, this makes cross-referencing with the datasheet much easier.

---
 sound/soc/codecs/tlv320aic31xx.h | 460 +++++++++++++++++----------------------
 1 file changed, 203 insertions(+), 257 deletions(-)

rewrite sound/soc/codecs/tlv320aic31xx.h (80%)

sound/soc/codecs/tlv320aic31xx.h +203 -257
dissimilarity index 80%
index 6efea0485392..15ac7cba86fe 100644
@@ -1,257 +1,203 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * ALSA SoC TLV320AIC31xx CODEC Driver Definitions
- *
- * Copyright (C) 2014-2017 Texas Instruments Incorporated - http://www.ti.com/
- */
-
-#ifndef _TLV320AIC31XX_H
-#define _TLV320AIC31XX_H
-
-#define AIC31XX_RATES	SNDRV_PCM_RATE_8000_192000
-
-#define AIC31XX_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE \
-			 | SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_LE \
-			 | SNDRV_PCM_FMTBIT_S32_LE)
-
-
-#define AIC31XX_STEREO_CLASS_D_BIT	0x1
-#define AIC31XX_MINIDSP_BIT		0x2
-#define DAC31XX_BIT			0x4
-
-enum aic31xx_type {
-	AIC3100	= 0,
-	AIC3110 = AIC31XX_STEREO_CLASS_D_BIT,
-	AIC3120 = AIC31XX_MINIDSP_BIT,
-	AIC3111 = (AIC31XX_STEREO_CLASS_D_BIT | AIC31XX_MINIDSP_BIT),
-	DAC3100 = DAC31XX_BIT,
-	DAC3101 = DAC31XX_BIT | AIC31XX_STEREO_CLASS_D_BIT,
-};
-
-struct aic31xx_pdata {
-	enum aic31xx_type codec_type;
-	unsigned int gpio_reset;
-	int micbias_vg;
-};
-
-#define AIC31XX_REG(page, reg)	((page * 128) + reg)
-
-/* Page Control Register */
-#define AIC31XX_PAGECTL		AIC31XX_REG(0, 0)
-
-/* Page 0 Registers */
-/* Software reset register */
-#define AIC31XX_RESET		AIC31XX_REG(0, 1)
-/* OT FLAG register */
-#define AIC31XX_OT_FLAG		AIC31XX_REG(0, 3)
-/* Clock clock Gen muxing, Multiplexers*/
-#define AIC31XX_CLKMUX		AIC31XX_REG(0, 4)
-/* PLL P and R-VAL register */
-#define AIC31XX_PLLPR		AIC31XX_REG(0, 5)
-/* PLL J-VAL register */
-#define AIC31XX_PLLJ		AIC31XX_REG(0, 6)
-/* PLL D-VAL MSB register */
-#define AIC31XX_PLLDMSB		AIC31XX_REG(0, 7)
-/* PLL D-VAL LSB register */
-#define AIC31XX_PLLDLSB		AIC31XX_REG(0, 8)
-/* DAC NDAC_VAL register*/
-#define AIC31XX_NDAC		AIC31XX_REG(0, 11)
-/* DAC MDAC_VAL register */
-#define AIC31XX_MDAC		AIC31XX_REG(0, 12)
-/* DAC OSR setting register 1, MSB value */
-#define AIC31XX_DOSRMSB		AIC31XX_REG(0, 13)
-/* DAC OSR setting register 2, LSB value */
-#define AIC31XX_DOSRLSB		AIC31XX_REG(0, 14)
-#define AIC31XX_MINI_DSP_INPOL	AIC31XX_REG(0, 16)
-/* Clock setting register 8, PLL */
-#define AIC31XX_NADC		AIC31XX_REG(0, 18)
-/* Clock setting register 9, PLL */
-#define AIC31XX_MADC		AIC31XX_REG(0, 19)
-/* ADC Oversampling (AOSR) Register */
-#define AIC31XX_AOSR		AIC31XX_REG(0, 20)
-/* Clock setting register 9, Multiplexers */
-#define AIC31XX_CLKOUTMUX	AIC31XX_REG(0, 25)
-/* Clock setting register 10, CLOCKOUT M divider value */
-#define AIC31XX_CLKOUTMVAL	AIC31XX_REG(0, 26)
-/* Audio Interface Setting Register 1 */
-#define AIC31XX_IFACE1		AIC31XX_REG(0, 27)
-/* Audio Data Slot Offset Programming */
-#define AIC31XX_DATA_OFFSET	AIC31XX_REG(0, 28)
-/* Audio Interface Setting Register 2 */
-#define AIC31XX_IFACE2		AIC31XX_REG(0, 29)
-/* Clock setting register 11, BCLK N Divider */
-#define AIC31XX_BCLKN		AIC31XX_REG(0, 30)
-/* Audio Interface Setting Register 3, Secondary Audio Interface */
-#define AIC31XX_IFACESEC1	AIC31XX_REG(0, 31)
-/* Audio Interface Setting Register 4 */
-#define AIC31XX_IFACESEC2	AIC31XX_REG(0, 32)
-/* Audio Interface Setting Register 5 */
-#define AIC31XX_IFACESEC3	AIC31XX_REG(0, 33)
-/* I2C Bus Condition */
-#define AIC31XX_I2C		AIC31XX_REG(0, 34)
-/* ADC FLAG */
-#define AIC31XX_ADCFLAG		AIC31XX_REG(0, 36)
-/* DAC Flag Registers */
-#define AIC31XX_DACFLAG1	AIC31XX_REG(0, 37)
-#define AIC31XX_DACFLAG2	AIC31XX_REG(0, 38)
-/* Sticky Interrupt flag (overflow) */
-#define AIC31XX_OFFLAG		AIC31XX_REG(0, 39)
-/* Sticy DAC Interrupt flags */
-#define AIC31XX_INTRDACFLAG	AIC31XX_REG(0, 44)
-/* Sticy ADC Interrupt flags */
-#define AIC31XX_INTRADCFLAG	AIC31XX_REG(0, 45)
-/* DAC Interrupt flags 2 */
-#define AIC31XX_INTRDACFLAG2	AIC31XX_REG(0, 46)
-/* ADC Interrupt flags 2 */
-#define AIC31XX_INTRADCFLAG2	AIC31XX_REG(0, 47)
-/* INT1 interrupt control */
-#define AIC31XX_INT1CTRL	AIC31XX_REG(0, 48)
-/* INT2 interrupt control */
-#define AIC31XX_INT2CTRL	AIC31XX_REG(0, 49)
-/* GPIO1 control */
-#define AIC31XX_GPIO1		AIC31XX_REG(0, 51)
-
-#define AIC31XX_DACPRB		AIC31XX_REG(0, 60)
-/* ADC Instruction Set Register */
-#define AIC31XX_ADCPRB		AIC31XX_REG(0, 61)
-/* DAC channel setup register */
-#define AIC31XX_DACSETUP	AIC31XX_REG(0, 63)
-/* DAC Mute and volume control register */
-#define AIC31XX_DACMUTE		AIC31XX_REG(0, 64)
-/* Left DAC channel digital volume control */
-#define AIC31XX_LDACVOL		AIC31XX_REG(0, 65)
-/* Right DAC channel digital volume control */
-#define AIC31XX_RDACVOL		AIC31XX_REG(0, 66)
-/* Headset detection */
-#define AIC31XX_HSDETECT	AIC31XX_REG(0, 67)
-/* ADC Digital Mic */
-#define AIC31XX_ADCSETUP	AIC31XX_REG(0, 81)
-/* ADC Digital Volume Control Fine Adjust */
-#define AIC31XX_ADCFGA		AIC31XX_REG(0, 82)
-/* ADC Digital Volume Control Coarse Adjust */
-#define AIC31XX_ADCVOL		AIC31XX_REG(0, 83)
-
-
-/* Page 1 Registers */
-/* Headphone drivers */
-#define AIC31XX_HPDRIVER	AIC31XX_REG(1, 31)
-/* Class-D Speakear Amplifier */
-#define AIC31XX_SPKAMP		AIC31XX_REG(1, 32)
-/* HP Output Drivers POP Removal Settings */
-#define AIC31XX_HPPOP		AIC31XX_REG(1, 33)
-/* Output Driver PGA Ramp-Down Period Control */
-#define AIC31XX_SPPGARAMP	AIC31XX_REG(1, 34)
-/* DAC_L and DAC_R Output Mixer Routing */
-#define AIC31XX_DACMIXERROUTE	AIC31XX_REG(1, 35)
-/* Left Analog Vol to HPL */
-#define AIC31XX_LANALOGHPL	AIC31XX_REG(1, 36)
-/* Right Analog Vol to HPR */
-#define AIC31XX_RANALOGHPR	AIC31XX_REG(1, 37)
-/* Left Analog Vol to SPL */
-#define AIC31XX_LANALOGSPL	AIC31XX_REG(1, 38)
-/* Right Analog Vol to SPR */
-#define AIC31XX_RANALOGSPR	AIC31XX_REG(1, 39)
-/* HPL Driver */
-#define AIC31XX_HPLGAIN		AIC31XX_REG(1, 40)
-/* HPR Driver */
-#define AIC31XX_HPRGAIN		AIC31XX_REG(1, 41)
-/* SPL Driver */
-#define AIC31XX_SPLGAIN		AIC31XX_REG(1, 42)
-/* SPR Driver */
-#define AIC31XX_SPRGAIN		AIC31XX_REG(1, 43)
-/* HP Driver Control */
-#define AIC31XX_HPCONTROL	AIC31XX_REG(1, 44)
-/* MIC Bias Control */
-#define AIC31XX_MICBIAS		AIC31XX_REG(1, 46)
-/* MIC PGA*/
-#define AIC31XX_MICPGA		AIC31XX_REG(1, 47)
-/* Delta-Sigma Mono ADC Channel Fine-Gain Input Selection for P-Terminal */
-#define AIC31XX_MICPGAPI	AIC31XX_REG(1, 48)
-/* ADC Input Selection for M-Terminal */
-#define AIC31XX_MICPGAMI	AIC31XX_REG(1, 49)
-/* Input CM Settings */
-#define AIC31XX_MICPGACM	AIC31XX_REG(1, 50)
-
-/* Bits, masks and shifts */
-
-/* AIC31XX_CLKMUX */
-#define AIC31XX_PLL_CLKIN_MASK			0x0c
-#define AIC31XX_PLL_CLKIN_SHIFT			2
-#define AIC31XX_PLL_CLKIN_MCLK			0
-#define AIC31XX_CODEC_CLKIN_MASK		0x03
-#define AIC31XX_CODEC_CLKIN_SHIFT		0
-#define AIC31XX_CODEC_CLKIN_PLL			3
-#define AIC31XX_CODEC_CLKIN_BCLK		1
-
-/* AIC31XX_PLLPR, AIC31XX_NDAC, AIC31XX_MDAC, AIC31XX_NADC, AIC31XX_MADC,
-   AIC31XX_BCLKN */
-#define AIC31XX_PLL_MASK		0x7f
-#define AIC31XX_PM_MASK			0x80
-
-/* AIC31XX_IFACE1 */
-#define AIC31XX_WORD_LEN_16BITS		0x00
-#define AIC31XX_WORD_LEN_20BITS		0x01
-#define AIC31XX_WORD_LEN_24BITS		0x02
-#define AIC31XX_WORD_LEN_32BITS		0x03
-#define AIC31XX_IFACE1_DATALEN_MASK	0x30
-#define AIC31XX_IFACE1_DATALEN_SHIFT	(4)
-#define AIC31XX_IFACE1_DATATYPE_MASK	0xC0
-#define AIC31XX_IFACE1_DATATYPE_SHIFT	(6)
-#define AIC31XX_I2S_MODE		0x00
-#define AIC31XX_DSP_MODE		0x01
-#define AIC31XX_RIGHT_JUSTIFIED_MODE	0x02
-#define AIC31XX_LEFT_JUSTIFIED_MODE	0x03
-#define AIC31XX_IFACE1_MASTER_MASK	0x0C
-#define AIC31XX_BCLK_MASTER		0x08
-#define AIC31XX_WCLK_MASTER		0x04
-
-/* AIC31XX_DATA_OFFSET */
-#define AIC31XX_DATA_OFFSET_MASK	0xFF
-
-/* AIC31XX_IFACE2 */
-#define AIC31XX_BCLKINV_MASK		0x08
-#define AIC31XX_BDIVCLK_MASK		0x03
-#define AIC31XX_DAC2BCLK		0x00
-#define AIC31XX_DACMOD2BCLK		0x01
-#define AIC31XX_ADC2BCLK		0x02
-#define AIC31XX_ADCMOD2BCLK		0x03
-
-/* AIC31XX_ADCFLAG */
-#define AIC31XX_ADCPWRSTATUS_MASK		0x40
-
-/* AIC31XX_DACFLAG1 */
-#define AIC31XX_LDACPWRSTATUS_MASK		0x80
-#define AIC31XX_RDACPWRSTATUS_MASK		0x08
-#define AIC31XX_HPLDRVPWRSTATUS_MASK		0x20
-#define AIC31XX_HPRDRVPWRSTATUS_MASK		0x02
-#define AIC31XX_SPLDRVPWRSTATUS_MASK		0x10
-#define AIC31XX_SPRDRVPWRSTATUS_MASK		0x01
-
-/* AIC31XX_INTRDACFLAG */
-#define AIC31XX_HPSCDETECT_MASK			0x80
-#define AIC31XX_BUTTONPRESS_MASK		0x20
-#define AIC31XX_HSPLUG_MASK			0x10
-#define AIC31XX_LDRCTHRES_MASK			0x08
-#define AIC31XX_RDRCTHRES_MASK			0x04
-#define AIC31XX_DACSINT_MASK			0x02
-#define AIC31XX_DACAINT_MASK			0x01
-
-/* AIC31XX_INT1CTRL */
-#define AIC31XX_HSPLUGDET_MASK			0x80
-#define AIC31XX_BUTTONPRESSDET_MASK		0x40
-#define AIC31XX_DRCTHRES_MASK			0x20
-#define AIC31XX_AGCNOISE_MASK			0x10
-#define AIC31XX_OC_MASK				0x08
-#define AIC31XX_ENGINE_MASK			0x04
-
-/* AIC31XX_DACSETUP */
-#define AIC31XX_SOFTSTEP_MASK			0x03
-
-/* AIC31XX_DACMUTE */
-#define AIC31XX_DACMUTE_MASK			0x0C
-
-/* AIC31XX_MICBIAS */
-#define AIC31XX_MICBIAS_MASK			0x03
-#define AIC31XX_MICBIAS_SHIFT			0
-
-#endif	/* _TLV320AIC31XX_H */
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ALSA SoC TLV320AIC31xx CODEC Driver Definitions
+ *
+ * Copyright (C) 2014-2017 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef _TLV320AIC31XX_H
+#define _TLV320AIC31XX_H
+
+#define AIC31XX_RATES	SNDRV_PCM_RATE_8000_192000
+
+#define AIC31XX_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+			 SNDRV_PCM_FMTBIT_S20_3LE | \
+			 SNDRV_PCM_FMTBIT_S24_3LE | \
+			 SNDRV_PCM_FMTBIT_S24_LE | \
+			 SNDRV_PCM_FMTBIT_S32_LE)
+
+#define AIC31XX_STEREO_CLASS_D_BIT	BIT(1)
+#define AIC31XX_MINIDSP_BIT		BIT(2)
+#define DAC31XX_BIT			BIT(3)
+
+enum aic31xx_type {
+	AIC3100	= 0,
+	AIC3110 = AIC31XX_STEREO_CLASS_D_BIT,
+	AIC3120 = AIC31XX_MINIDSP_BIT,
+	AIC3111 = AIC31XX_STEREO_CLASS_D_BIT | AIC31XX_MINIDSP_BIT,
+	DAC3100 = DAC31XX_BIT,
+	DAC3101 = DAC31XX_BIT | AIC31XX_STEREO_CLASS_D_BIT,
+};
+
+struct aic31xx_pdata {
+	enum aic31xx_type codec_type;
+	unsigned int gpio_reset;
+	int micbias_vg;
+};
+
+#define AIC31XX_REG(page, reg)	((page * 128) + reg)
+
+#define AIC31XX_PAGECTL		AIC31XX_REG(0, 0) /* Page Control Register */
+
+/* Page 0 Registers */
+#define AIC31XX_RESET		AIC31XX_REG(0, 1) /* Software reset register */
+#define AIC31XX_OT_FLAG		AIC31XX_REG(0, 3) /* OT FLAG register */
+#define AIC31XX_CLKMUX		AIC31XX_REG(0, 4) /* Clock clock Gen muxing, Multiplexers*/
+#define AIC31XX_PLLPR		AIC31XX_REG(0, 5) /* PLL P and R-VAL register */
+#define AIC31XX_PLLJ		AIC31XX_REG(0, 6) /* PLL J-VAL register */
+#define AIC31XX_PLLDMSB		AIC31XX_REG(0, 7) /* PLL D-VAL MSB register */
+#define AIC31XX_PLLDLSB		AIC31XX_REG(0, 8) /* PLL D-VAL LSB register */
+#define AIC31XX_NDAC		AIC31XX_REG(0, 11) /* DAC NDAC_VAL register*/
+#define AIC31XX_MDAC		AIC31XX_REG(0, 12) /* DAC MDAC_VAL register */
+#define AIC31XX_DOSRMSB		AIC31XX_REG(0, 13) /* DAC OSR setting register 1, MSB value */
+#define AIC31XX_DOSRLSB		AIC31XX_REG(0, 14) /* DAC OSR setting register 2, LSB value */
+#define AIC31XX_MINI_DSP_INPOL	AIC31XX_REG(0, 16)
+#define AIC31XX_NADC		AIC31XX_REG(0, 18) /* Clock setting register 8, PLL */
+#define AIC31XX_MADC		AIC31XX_REG(0, 19) /* Clock setting register 9, PLL */
+#define AIC31XX_AOSR		AIC31XX_REG(0, 20) /* ADC Oversampling (AOSR) Register */
+#define AIC31XX_CLKOUTMUX	AIC31XX_REG(0, 25) /* Clock setting register 9, Multiplexers */
+#define AIC31XX_CLKOUTMVAL	AIC31XX_REG(0, 26) /* Clock setting register 10, CLOCKOUT M divider value */
+#define AIC31XX_IFACE1		AIC31XX_REG(0, 27) /* Audio Interface Setting Register 1 */
+#define AIC31XX_DATA_OFFSET	AIC31XX_REG(0, 28) /* Audio Data Slot Offset Programming */
+#define AIC31XX_IFACE2		AIC31XX_REG(0, 29) /* Audio Interface Setting Register 2 */
+#define AIC31XX_BCLKN		AIC31XX_REG(0, 30) /* Clock setting register 11, BCLK N Divider */
+#define AIC31XX_IFACESEC1	AIC31XX_REG(0, 31) /* Audio Interface Setting Register 3, Secondary Audio Interface */
+#define AIC31XX_IFACESEC2	AIC31XX_REG(0, 32) /* Audio Interface Setting Register 4 */
+#define AIC31XX_IFACESEC3	AIC31XX_REG(0, 33) /* Audio Interface Setting Register 5 */
+#define AIC31XX_I2C		AIC31XX_REG(0, 34) /* I2C Bus Condition */
+#define AIC31XX_ADCFLAG		AIC31XX_REG(0, 36) /* ADC FLAG */
+#define AIC31XX_DACFLAG1	AIC31XX_REG(0, 37) /* DAC Flag Registers */
+#define AIC31XX_DACFLAG2	AIC31XX_REG(0, 38)
+#define AIC31XX_OFFLAG		AIC31XX_REG(0, 39) /* Sticky Interrupt flag (overflow) */
+#define AIC31XX_INTRDACFLAG	AIC31XX_REG(0, 44) /* Sticy DAC Interrupt flags */
+#define AIC31XX_INTRADCFLAG	AIC31XX_REG(0, 45) /* Sticy ADC Interrupt flags */
+#define AIC31XX_INTRDACFLAG2	AIC31XX_REG(0, 46) /* DAC Interrupt flags 2 */
+#define AIC31XX_INTRADCFLAG2	AIC31XX_REG(0, 47) /* ADC Interrupt flags 2 */
+#define AIC31XX_INT1CTRL	AIC31XX_REG(0, 48) /* INT1 interrupt control */
+#define AIC31XX_INT2CTRL	AIC31XX_REG(0, 49) /* INT2 interrupt control */
+#define AIC31XX_GPIO1		AIC31XX_REG(0, 51) /* GPIO1 control */
+#define AIC31XX_DACPRB		AIC31XX_REG(0, 60)
+#define AIC31XX_ADCPRB		AIC31XX_REG(0, 61) /* ADC Instruction Set Register */
+#define AIC31XX_DACSETUP	AIC31XX_REG(0, 63) /* DAC channel setup register */
+#define AIC31XX_DACMUTE		AIC31XX_REG(0, 64) /* DAC Mute and volume control register */
+#define AIC31XX_LDACVOL		AIC31XX_REG(0, 65) /* Left DAC channel digital volume control */
+#define AIC31XX_RDACVOL		AIC31XX_REG(0, 66) /* Right DAC channel digital volume control */
+#define AIC31XX_HSDETECT	AIC31XX_REG(0, 67) /* Headset detection */
+#define AIC31XX_ADCSETUP	AIC31XX_REG(0, 81) /* ADC Digital Mic */
+#define AIC31XX_ADCFGA		AIC31XX_REG(0, 82) /* ADC Digital Volume Control Fine Adjust */
+#define AIC31XX_ADCVOL		AIC31XX_REG(0, 83) /* ADC Digital Volume Control Coarse Adjust */
+
+/* Page 1 Registers */
+#define AIC31XX_HPDRIVER	AIC31XX_REG(1, 31) /* Headphone drivers */
+#define AIC31XX_SPKAMP		AIC31XX_REG(1, 32) /* Class-D Speakear Amplifier */
+#define AIC31XX_HPPOP		AIC31XX_REG(1, 33) /* HP Output Drivers POP Removal Settings */
+#define AIC31XX_SPPGARAMP	AIC31XX_REG(1, 34) /* Output Driver PGA Ramp-Down Period Control */
+#define AIC31XX_DACMIXERROUTE	AIC31XX_REG(1, 35) /* DAC_L and DAC_R Output Mixer Routing */
+#define AIC31XX_LANALOGHPL	AIC31XX_REG(1, 36) /* Left Analog Vol to HPL */
+#define AIC31XX_RANALOGHPR	AIC31XX_REG(1, 37) /* Right Analog Vol to HPR */
+#define AIC31XX_LANALOGSPL	AIC31XX_REG(1, 38) /* Left Analog Vol to SPL */
+#define AIC31XX_RANALOGSPR	AIC31XX_REG(1, 39) /* Right Analog Vol to SPR */
+#define AIC31XX_HPLGAIN		AIC31XX_REG(1, 40) /* HPL Driver */
+#define AIC31XX_HPRGAIN		AIC31XX_REG(1, 41) /* HPR Driver */
+#define AIC31XX_SPLGAIN		AIC31XX_REG(1, 42) /* SPL Driver */
+#define AIC31XX_SPRGAIN		AIC31XX_REG(1, 43) /* SPR Driver */
+#define AIC31XX_HPCONTROL	AIC31XX_REG(1, 44) /* HP Driver Control */
+#define AIC31XX_MICBIAS		AIC31XX_REG(1, 46) /* MIC Bias Control */
+#define AIC31XX_MICPGA		AIC31XX_REG(1, 47) /* MIC PGA*/
+#define AIC31XX_MICPGAPI	AIC31XX_REG(1, 48) /* Delta-Sigma Mono ADC Channel Fine-Gain Input Selection for P-Terminal */
+#define AIC31XX_MICPGAMI	AIC31XX_REG(1, 49) /* ADC Input Selection for M-Terminal */
+#define AIC31XX_MICPGACM	AIC31XX_REG(1, 50) /* Input CM Settings */
+
+/* Bits, masks, and shifts */
+
+/* AIC31XX_CLKMUX */
+#define AIC31XX_PLL_CLKIN_MASK		GENMASK(3, 2)
+#define AIC31XX_PLL_CLKIN_SHIFT		(2)
+#define AIC31XX_PLL_CLKIN_MCLK		0x00
+#define AIC31XX_PLL_CLKIN_BCKL		0x01
+#define AIC31XX_PLL_CLKIN_GPIO1		0x02
+#define AIC31XX_PLL_CLKIN_DIN		0x03
+#define AIC31XX_CODEC_CLKIN_MASK	GENMASK(1, 0)
+#define AIC31XX_CODEC_CLKIN_SHIFT	(0)
+#define AIC31XX_CODEC_CLKIN_MCLK	0x00
+#define AIC31XX_CODEC_CLKIN_BCLK	0x01
+#define AIC31XX_CODEC_CLKIN_GPIO1	0x02
+#define AIC31XX_CODEC_CLKIN_PLL		0x03
+
+/* AIC31XX_PLLPR */
+/* AIC31XX_NDAC */
+/* AIC31XX_MDAC */
+/* AIC31XX_NADC */
+/* AIC31XX_MADC */
+/* AIC31XX_BCLKN */
+#define AIC31XX_PLL_MASK		GENMASK(6, 0)
+#define AIC31XX_PM_MASK			BIT(7)
+
+/* AIC31XX_IFACE1 */
+#define AIC31XX_IFACE1_DATATYPE_MASK	GENMASK(7, 6)
+#define AIC31XX_IFACE1_DATATYPE_SHIFT	(6)
+#define AIC31XX_I2S_MODE		0x00
+#define AIC31XX_DSP_MODE		0x01
+#define AIC31XX_RIGHT_JUSTIFIED_MODE	0x02
+#define AIC31XX_LEFT_JUSTIFIED_MODE	0x03
+#define AIC31XX_IFACE1_DATALEN_MASK	GENMASK(5, 4)
+#define AIC31XX_IFACE1_DATALEN_SHIFT	(4)
+#define AIC31XX_WORD_LEN_16BITS		0x00
+#define AIC31XX_WORD_LEN_20BITS		0x01
+#define AIC31XX_WORD_LEN_24BITS		0x02
+#define AIC31XX_WORD_LEN_32BITS		0x03
+#define AIC31XX_IFACE1_MASTER_MASK	GENMASK(3, 2)
+#define AIC31XX_BCLK_MASTER		BIT(2)
+#define AIC31XX_WCLK_MASTER		BIT(3)
+
+/* AIC31XX_DATA_OFFSET */
+#define AIC31XX_DATA_OFFSET_MASK	GENMASK(7, 0)
+
+/* AIC31XX_IFACE2 */
+#define AIC31XX_BCLKINV_MASK		BIT(3)
+#define AIC31XX_BDIVCLK_MASK		GENMASK(1, 0)
+#define AIC31XX_DAC2BCLK		0x00
+#define AIC31XX_DACMOD2BCLK		0x01
+#define AIC31XX_ADC2BCLK		0x02
+#define AIC31XX_ADCMOD2BCLK		0x03
+
+/* AIC31XX_ADCFLAG */
+#define AIC31XX_ADCPWRSTATUS_MASK	BIT(6)
+
+/* AIC31XX_DACFLAG1 */
+#define AIC31XX_LDACPWRSTATUS_MASK	BIT(7)
+#define AIC31XX_HPLDRVPWRSTATUS_MASK	BIT(5)
+#define AIC31XX_SPLDRVPWRSTATUS_MASK	BIT(4)
+#define AIC31XX_RDACPWRSTATUS_MASK	BIT(3)
+#define AIC31XX_HPRDRVPWRSTATUS_MASK	BIT(1)
+#define AIC31XX_SPRDRVPWRSTATUS_MASK	BIT(0)
+
+/* AIC31XX_INTRDACFLAG */
+#define AIC31XX_HPLSCDETECT		BIT(7)
+#define AIC31XX_HPRSCDETECT		BIT(6)
+#define AIC31XX_BUTTONPRESS		BIT(5)
+#define AIC31XX_HSPLUG			BIT(4)
+#define AIC31XX_LDRCTHRES		BIT(3)
+#define AIC31XX_RDRCTHRES		BIT(2)
+#define AIC31XX_DACSINT			BIT(1)
+#define AIC31XX_DACAINT			BIT(0)
+
+/* AIC31XX_INT1CTRL */
+#define AIC31XX_HSPLUGDET		BIT(7)
+#define AIC31XX_BUTTONPRESSDET		BIT(6)
+#define AIC31XX_DRCTHRES		BIT(5)
+#define AIC31XX_AGCNOISE		BIT(4)
+#define AIC31XX_SC			BIT(3)
+#define AIC31XX_ENGINE			BIT(2)
+
+/* AIC31XX_DACSETUP */
+#define AIC31XX_SOFTSTEP_MASK		GENMASK(1, 0)
+
+/* AIC31XX_DACMUTE */
+#define AIC31XX_DACMUTE_MASK		GENMASK(3, 2)
+
+/* AIC31XX_MICBIAS */
+#define AIC31XX_MICBIAS_MASK		GENMASK(1, 0)
+#define AIC31XX_MICBIAS_SHIFT		0
+
+#endif	/* _TLV320AIC31XX_H */
Signature · 1 line
2.15.0
#13 lore raw

PATCH v2 7/19 ASoC: tlv320aic31xx: Merge init function into probe

In reply to Andrew F. Davis

The function aic31xx_device_init() is only called from probe and does nothing that logically shouldn't be in probe, remove this unneeded function call and move its code into probe where it was called.

---
 sound/soc/codecs/tlv320aic31xx.c | 55 ++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 33 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +22 -33
index 07c014501e5e..c84febd991a0 100644
@@ -1302,9 +1302,29 @@ static const struct acpi_device_id aic31xx_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, aic31xx_acpi_match);
 #endif
 
-static int aic31xx_device_init(struct aic31xx_priv *aic31xx)
+static int aic31xx_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
 {
-	int ret, i;
+	struct aic31xx_priv *aic31xx;
+	int i, ret;
+
+	dev_dbg(&i2c->dev, "## %s: %s codec_type = %d\n", __func__,
+		id->name, (int)id->driver_data);
+
+	aic31xx = devm_kzalloc(&i2c->dev, sizeof(*aic31xx), GFP_KERNEL);
+	if (!aic31xx)
+		return -ENOMEM;
+
+	aic31xx->regmap = devm_regmap_init_i2c(i2c, &aic31xx_i2c_regmap);
+	if (IS_ERR(aic31xx->regmap)) {
+		ret = PTR_ERR(aic31xx->regmap);
+		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+	aic31xx->dev = &i2c->dev;
+
+	aic31xx->pdata.codec_type = id->driver_data;
 
 	dev_set_drvdata(aic31xx->dev, aic31xx);
 
@@ -1336,37 +1356,6 @@ static int aic31xx_device_init(struct aic31xx_priv *aic31xx)
 		return ret;
 	}
 
-	return 0;
-}
-
-static int aic31xx_i2c_probe(struct i2c_client *i2c,
-			     const struct i2c_device_id *id)
-{
-	struct aic31xx_priv *aic31xx;
-	int ret;
-
-	dev_dbg(&i2c->dev, "## %s: %s codec_type = %d\n", __func__,
-		id->name, (int) id->driver_data);
-
-	aic31xx = devm_kzalloc(&i2c->dev, sizeof(*aic31xx), GFP_KERNEL);
-	if (!aic31xx)
-		return -ENOMEM;
-
-	aic31xx->regmap = devm_regmap_init_i2c(i2c, &aic31xx_i2c_regmap);
-	if (IS_ERR(aic31xx->regmap)) {
-		ret = PTR_ERR(aic31xx->regmap);
-		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
-			ret);
-		return ret;
-	}
-	aic31xx->dev = &i2c->dev;
-
-	aic31xx->pdata.codec_type = id->driver_data;
-
-	ret = aic31xx_device_init(aic31xx);
-	if (ret)
-		return ret;
-
 	if (aic31xx->pdata.codec_type & DAC31XX_BIT)
 		return snd_soc_register_codec(&i2c->dev,
 				&soc_codec_driver_aic31xx,
Signature · 1 line
2.15.0
#14 lore raw

PATCH v2 8/19 ASoC: tlv320aic31xx: Switch GPIO handling to use gpiod_* API

In reply to Andrew F. Davis

Move to using newer gpiod_* GPIO handling functions. This simplifies the code and eases dropping platform data in the next patch. Also remember GPIO are active low, so set "1" to reset.

---
 sound/soc/codecs/tlv320aic31xx.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +10 -13
index c84febd991a0..ab03a19f6aaa 100644
@@ -157,6 +157,7 @@ struct aic31xx_priv {
 	u8 i2c_regs_status;
 	struct device *dev;
 	struct regmap *regmap;
+	struct gpio_desc *gpio_reset;
 	struct aic31xx_pdata pdata;
 	struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES];
 	struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES];
@@ -1020,8 +1021,8 @@ static int aic31xx_regulator_event(struct notifier_block *nb,
 		 * Put codec to reset and as at least one of the
 		 * supplies was disabled.
 		 */
-		if (gpio_is_valid(aic31xx->pdata.gpio_reset))
-			gpio_set_value(aic31xx->pdata.gpio_reset, 0);
+		if (aic31xx->gpio_reset)
+			gpiod_set_value(aic31xx->gpio_reset, 1);
 
 		regcache_mark_dirty(aic31xx->regmap);
 		dev_dbg(aic31xx->dev, "## %s: DISABLE received\n", __func__);
@@ -1073,8 +1074,8 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	if (ret)
 		return ret;
 
-	if (gpio_is_valid(aic31xx->pdata.gpio_reset)) {
-		gpio_set_value(aic31xx->pdata.gpio_reset, 1);
+	if (aic31xx->gpio_reset) {
+		gpiod_set_value(aic31xx->gpio_reset, 0);
 		udelay(100);
 	}
 	regcache_cache_only(aic31xx->regmap, false);
@@ -1334,15 +1335,11 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 	else if (aic31xx->dev->of_node)
 		aic31xx_pdata_from_of(aic31xx);
 
-	if (aic31xx->pdata.gpio_reset) {
-		ret = devm_gpio_request_one(aic31xx->dev,
-					    aic31xx->pdata.gpio_reset,
-					    GPIOF_OUT_INIT_HIGH,
-					    "aic31xx-reset-pin");
-		if (ret < 0) {
-			dev_err(aic31xx->dev, "not able to acquire gpio\n");
-			return ret;
-		}
+	aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(aic31xx->gpio_reset)) {
+		dev_err(aic31xx->dev, "not able to acquire gpio\n");
+		return PTR_ERR(aic31xx->gpio_reset);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++)
Signature · 6 lines
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#15 lore raw

Re: [PATCH v2 08/19] ASoC: tlv320aic31xx: Switch GPIO handling to use gpiod_* API

Quoted 6 lines · Andrew F. Davis

Move to using newer gpiod_* GPIO handling functions. This simplifies the code and eases dropping platform data in the next patch. Also remember GPIO are active low, so set "1" to reset.

---

Kbuild bot seems mad a this one, looks like I need to include linux/gpio/consumer.h, will fix for v3.

Quoted 59 lines
 sound/soc/codecs/tlv320aic31xx.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +10 -13
index c84febd991a0..ab03a19f6aaa 100644
@@ -157,6 +157,7 @@ struct aic31xx_priv {
 	u8 i2c_regs_status;
 	struct device *dev;
 	struct regmap *regmap;
+	struct gpio_desc *gpio_reset;
 	struct aic31xx_pdata pdata;
 	struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES];
 	struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES];
@@ -1020,8 +1021,8 @@ static int aic31xx_regulator_event(struct notifier_block *nb,
 		 * Put codec to reset and as at least one of the
 		 * supplies was disabled.
 		 */
-		if (gpio_is_valid(aic31xx->pdata.gpio_reset))
-			gpio_set_value(aic31xx->pdata.gpio_reset, 0);
+		if (aic31xx->gpio_reset)
+			gpiod_set_value(aic31xx->gpio_reset, 1);
 
 		regcache_mark_dirty(aic31xx->regmap);
 		dev_dbg(aic31xx->dev, "## %s: DISABLE received\n", __func__);
@@ -1073,8 +1074,8 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	if (ret)
 		return ret;
 
-	if (gpio_is_valid(aic31xx->pdata.gpio_reset)) {
-		gpio_set_value(aic31xx->pdata.gpio_reset, 1);
+	if (aic31xx->gpio_reset) {
+		gpiod_set_value(aic31xx->gpio_reset, 0);
 		udelay(100);
 	}
 	regcache_cache_only(aic31xx->regmap, false);
@@ -1334,15 +1335,11 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 	else if (aic31xx->dev->of_node)
 		aic31xx_pdata_from_of(aic31xx);
 
-	if (aic31xx->pdata.gpio_reset) {
-		ret = devm_gpio_request_one(aic31xx->dev,
-					    aic31xx->pdata.gpio_reset,
-					    GPIOF_OUT_INIT_HIGH,
-					    "aic31xx-reset-pin");
-		if (ret < 0) {
-			dev_err(aic31xx->dev, "not able to acquire gpio\n");
-			return ret;
-		}
+	aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(aic31xx->gpio_reset)) {
+		dev_err(aic31xx->dev, "not able to acquire gpio\n");
+		return PTR_ERR(aic31xx->gpio_reset);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++)
#16 lore raw

Re: [PATCH v2 08/19] ASoC: tlv320aic31xx: Switch GPIO handling to use gpiod_* API

Quoted 5 lines · Andrew F. Davis
Quoted 4 lines · Andrew F. Davis

Move to using newer gpiod_* GPIO handling functions. This simplifies the code and eases dropping platform data in the next patch. Also remember GPIO are active low, so set "1" to reset.

Quoted 7 lines
Quoted 2 lines

---

Kbuild bot seems mad a this one, looks like I need to include linux/gpio/consumer.h, will fix for v3.

Looks like you already have this in your -next branch, how do you want this fix, I can send a delta patch with the added include, a new v3 version that you can replace the patch in-tree with, or if it's easier for you manually fix in-tree?

Quoted 60 lines
Quoted 60 lines
 sound/soc/codecs/tlv320aic31xx.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +10 -13
index c84febd991a0..ab03a19f6aaa 100644
@@ -157,6 +157,7 @@ struct aic31xx_priv {
 	u8 i2c_regs_status;
 	struct device *dev;
 	struct regmap *regmap;
+	struct gpio_desc *gpio_reset;
 	struct aic31xx_pdata pdata;
 	struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES];
 	struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES];
@@ -1020,8 +1021,8 @@ static int aic31xx_regulator_event(struct notifier_block *nb,
 		 * Put codec to reset and as at least one of the
 		 * supplies was disabled.
 		 */
-		if (gpio_is_valid(aic31xx->pdata.gpio_reset))
-			gpio_set_value(aic31xx->pdata.gpio_reset, 0);
+		if (aic31xx->gpio_reset)
+			gpiod_set_value(aic31xx->gpio_reset, 1);
 
 		regcache_mark_dirty(aic31xx->regmap);
 		dev_dbg(aic31xx->dev, "## %s: DISABLE received\n", __func__);
@@ -1073,8 +1074,8 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	if (ret)
 		return ret;
 
-	if (gpio_is_valid(aic31xx->pdata.gpio_reset)) {
-		gpio_set_value(aic31xx->pdata.gpio_reset, 1);
+	if (aic31xx->gpio_reset) {
+		gpiod_set_value(aic31xx->gpio_reset, 0);
 		udelay(100);
 	}
 	regcache_cache_only(aic31xx->regmap, false);
@@ -1334,15 +1335,11 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 	else if (aic31xx->dev->of_node)
 		aic31xx_pdata_from_of(aic31xx);
 
-	if (aic31xx->pdata.gpio_reset) {
-		ret = devm_gpio_request_one(aic31xx->dev,
-					    aic31xx->pdata.gpio_reset,
-					    GPIOF_OUT_INIT_HIGH,
-					    "aic31xx-reset-pin");
-		if (ret < 0) {
-			dev_err(aic31xx->dev, "not able to acquire gpio\n");
-			return ret;
-		}
+	aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset",
+						      GPIOD_OUT_LOW);
+	if (IS_ERR(aic31xx->gpio_reset)) {
+		dev_err(aic31xx->dev, "not able to acquire gpio\n");
+		return PTR_ERR(aic31xx->gpio_reset);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++)
maintainer
#17 lore raw

Re: [PATCH v2 08/19] ASoC: tlv320aic31xx: Switch GPIO handling to use gpiod_* API

Quoted 9 lines · Andrew F. Davis
Quoted 2 lines · Andrew F. Davis

Kbuild bot seems mad a this one, looks like I need to include linux/gpio/consumer.h, will fix for v3.

Looks like you already have this in your -next branch, how do you want this fix, I can send a delta patch with the added include, a new v3 version that you can replace the patch in-tree with, or if it's easier for you manually fix in-tree?

As the patch applied mail says:

| If any updates are required or you are submitting further changes they
| should be sent as incremental updates against current git, existing
| patches will not be replaced.
#18 lore raw

PATCH v2 9/19 ASoC: tlv320aic31xx: Remove platform data

In reply to Andrew F. Davis

Platform data is not used by anyone (at least in upstream) so drop this data and switch to using fwnode(DT/ACPI) only.

---
 sound/soc/codecs/tlv320aic31xx.c | 72 +++++++++++++---------------------------
 sound/soc/codecs/tlv320aic31xx.h |  6 ----
 2 files changed, 23 insertions(+), 55 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +23 -49
index ab03a19f6aaa..0e00421d363b 100644
@@ -157,8 +157,9 @@ struct aic31xx_priv {
 	u8 i2c_regs_status;
 	struct device *dev;
 	struct regmap *regmap;
+	enum aic31xx_type codec_type;
 	struct gpio_desc *gpio_reset;
-	struct aic31xx_pdata pdata;
+	int micbias_vg;
 	struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES];
 	struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES];
 	unsigned int sysclk;
@@ -450,7 +451,7 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w,
 		/* change mic bias voltage to user defined */
 		snd_soc_update_bits(codec, AIC31XX_MICBIAS,
 				    AIC31XX_MICBIAS_MASK,
-				    aic31xx->pdata.micbias_vg <<
+				    aic31xx->micbias_vg <<
 				    AIC31XX_MICBIAS_SHIFT);
 		dev_dbg(codec->dev, "%s: turned on\n", __func__);
 		break;
@@ -673,14 +674,14 @@ static int aic31xx_add_controls(struct snd_soc_codec *codec)
 	int ret = 0;
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
 
-	if (!(aic31xx->pdata.codec_type & DAC31XX_BIT))
+	if (!(aic31xx->codec_type & DAC31XX_BIT))
 		ret = snd_soc_add_codec_controls(
 			codec, aic31xx_snd_controls,
 			ARRAY_SIZE(aic31xx_snd_controls));
 	if (ret)
 		return ret;
 
-	if (aic31xx->pdata.codec_type & AIC31XX_STEREO_CLASS_D_BIT)
+	if (aic31xx->codec_type & AIC31XX_STEREO_CLASS_D_BIT)
 		ret = snd_soc_add_codec_controls(
 			codec, aic311x_snd_controls,
 			ARRAY_SIZE(aic311x_snd_controls));
@@ -698,7 +699,7 @@ static int aic31xx_add_widgets(struct snd_soc_codec *codec)
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
 	int ret = 0;
 
-	if (aic31xx->pdata.codec_type & DAC31XX_BIT) {
+	if (aic31xx->codec_type & DAC31XX_BIT) {
 		ret = snd_soc_dapm_new_controls(
 			dapm, dac31xx_dapm_widgets,
 			ARRAY_SIZE(dac31xx_dapm_widgets));
@@ -722,7 +723,7 @@ static int aic31xx_add_widgets(struct snd_soc_codec *codec)
 			return ret;
 	}
 
-	if (aic31xx->pdata.codec_type & AIC31XX_STEREO_CLASS_D_BIT) {
+	if (aic31xx->codec_type & AIC31XX_STEREO_CLASS_D_BIT) {
 		ret = snd_soc_dapm_new_controls(
 			dapm, aic311x_dapm_widgets,
 			ARRAY_SIZE(aic311x_dapm_widgets));
@@ -1257,42 +1258,6 @@ static const struct of_device_id tlv320aic31xx_of_match[] = {
 	{},
 };
 MODULE_DEVICE_TABLE(of, tlv320aic31xx_of_match);
-
-static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx)
-{
-	struct device_node *np = aic31xx->dev->of_node;
-	unsigned int value = MICBIAS_2_0V;
-	int ret;
-
-	of_property_read_u32(np, "ai31xx-micbias-vg", &value);
-	switch (value) {
-	case MICBIAS_2_0V:
-	case MICBIAS_2_5V:
-	case MICBIAS_AVDDV:
-		aic31xx->pdata.micbias_vg = value;
-		break;
-	default:
-		dev_err(aic31xx->dev,
-			"Bad ai31xx-micbias-vg value %d DT\n",
-			value);
-		aic31xx->pdata.micbias_vg = MICBIAS_2_0V;
-	}
-
-	ret = of_get_named_gpio(np, "reset-gpios", 0);
-	if (ret > 0) {
-		aic31xx->pdata.gpio_reset = ret;
-	} else {
-		ret = of_get_named_gpio(np, "gpio-reset", 0);
-		if (ret > 0) {
-			dev_warn(aic31xx->dev, "Using deprecated property \"gpio-reset\", please update your DT");
-			aic31xx->pdata.gpio_reset = ret;
-		}
-	}
-}
-#else /* CONFIG_OF */
-static void aic31xx_pdata_from_of(struct aic31xx_priv *aic31xx)
-{
-}
 #endif /* CONFIG_OF */
 
 #ifdef CONFIG_ACPI
@@ -1307,6 +1272,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 			     const struct i2c_device_id *id)
 {
 	struct aic31xx_priv *aic31xx;
+	unsigned int micbias_value = MICBIAS_2_0V;
 	int i, ret;
 
 	dev_dbg(&i2c->dev, "## %s: %s codec_type = %d\n", __func__,
@@ -1325,15 +1291,23 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 	}
 	aic31xx->dev = &i2c->dev;
 
-	aic31xx->pdata.codec_type = id->driver_data;
+	aic31xx->codec_type = id->driver_data;
 
 	dev_set_drvdata(aic31xx->dev, aic31xx);
 
-	if (dev_get_platdata(aic31xx->dev))
-		memcpy(&aic31xx->pdata, dev_get_platdata(aic31xx->dev),
-		       sizeof(aic31xx->pdata));
-	else if (aic31xx->dev->of_node)
-		aic31xx_pdata_from_of(aic31xx);
+	fwnode_property_read_u32(aic31xx->dev->fwnode, "ai31xx-micbias-vg",
+				 &micbias_value);
+	switch (micbias_value) {
+	case MICBIAS_2_0V:
+	case MICBIAS_2_5V:
+	case MICBIAS_AVDDV:
+		aic31xx->micbias_vg = micbias_value;
+		break;
+	default:
+		dev_err(aic31xx->dev, "Bad ai31xx-micbias-vg value %d in DT\n",
+			micbias_value);
+		aic31xx->micbias_vg = MICBIAS_2_0V;
+	}
 
 	aic31xx->gpio_reset = devm_gpiod_get_optional(aic31xx->dev, "reset",
 						      GPIOD_OUT_LOW);
@@ -1353,7 +1327,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
-	if (aic31xx->pdata.codec_type & DAC31XX_BIT)
+	if (aic31xx->codec_type & DAC31XX_BIT)
 		return snd_soc_register_codec(&i2c->dev,
 				&soc_codec_driver_aic31xx,
 				dac31xx_dai_driver,
sound/soc/codecs/tlv320aic31xx.h +0 -6
index 15ac7cba86fe..ab94e6a0c742 100644
@@ -29,12 +29,6 @@ enum aic31xx_type {
 	DAC3101 = DAC31XX_BIT | AIC31XX_STEREO_CLASS_D_BIT,
 };
 
-struct aic31xx_pdata {
-	enum aic31xx_type codec_type;
-	unsigned int gpio_reset;
-	int micbias_vg;
-};
-
 #define AIC31XX_REG(page, reg)	((page * 128) + reg)
 
 #define AIC31XX_PAGECTL		AIC31XX_REG(0, 0) /* Page Control Register */
Signature · 1 line
2.15.0
maintainer
#19 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 2 lines · Andrew F. Davis

Platform data is not used by anyone (at least in upstream) so drop this data and switch to using fwnode(DT/ACPI) only.

The advantage being...? Not all architectures use DT or ACPI so it's not clear that this is a step forwards in itself.

#20 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 7 lines · Mark Brown
Quoted 2 lines · Andrew F. Davis

Platform data is not used by anyone (at least in upstream) so drop this data and switch to using fwnode(DT/ACPI) only.

The advantage being...? Not all architectures use DT or ACPI so it's not clear that this is a step forwards in itself.

Simplifies the code in several places, and you don't need to use DT or ACPI, it probes just fine anyway you normally add an I2C device.

All we are dropping here is the platform_data way of specifying mic-bias voltage, which if you are wanting to do that in an out-of-tree board file, then I'm sure you can locally modify this driver to use your wanted voltage setting by default.

maintainer
#21 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 5 lines · Andrew F. Davis
Quoted 2 lines · Mark Brown

The advantage being...? Not all architectures use DT or ACPI so it's not clear that this is a step forwards in itself.

Quoted 7 lines

Simplifies the code in several places, and you don't need to use DT or ACPI, it probes just fine anyway you normally add an I2C device.

All we are dropping here is the platform_data way of specifying mic-bias voltage, which if you are wanting to do that in an out-of-tree board file, then I'm sure you can locally modify this driver to use your wanted voltage setting by default.

Then if you want to upstream the driver you'll have to add the platform data support again. Like I say not all architectures have anything other than board files.

#22 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 9 lines · Mark Brown
Quoted 7 lines · Andrew F. Davis
Quoted 2 lines · Mark Brown

The advantage being...? Not all architectures use DT or ACPI so it's not clear that this is a step forwards in itself.

Simplifies the code in several places, and you don't need to use DT or ACPI, it probes just fine anyway you normally add an I2C device.

Quoted 9 lines
Quoted 4 lines

All we are dropping here is the platform_data way of specifying mic-bias voltage, which if you are wanting to do that in an out-of-tree board file, then I'm sure you can locally modify this driver to use your wanted voltage setting by default.

Then if you want to upstream the driver you'll have to add the platform data support again. Like I say not all architectures have anything other than board files.

Then they can try, but they will rightfully get nack'd and told to stop using board files and use DT/ACPI. Most upstream architectures don't use board files anymore anyway, so I doubt this will ever happen.

Besides, if they haven't upstreamed their code then it is their problem if this patch breaks them, we shouldn't hold up upstream work for out-of-tree code, especially theoretical out-of-tree code that will never exist.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#23 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 9 lines · Andrew F. Davis
Quoted 3 lines · Mark Brown

Then if you want to upstream the driver you'll have to add the platform data support again. Like I say not all architectures have anything other than board files.

Then they can try, but they will rightfully get nack'd and told to stop using board files and use DT/ACPI. Most upstream architectures don't use board files anymore anyway, so I doubt this will ever happen.

No. To repeat, not all architectures use DT or ACPI. Expecting someone to impelement DT or ACPI support for an entire architecture and try to bring the ecosystem for that architecture along in order to add machine support is obviously totally unreasonable.

#24 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 7 lines · Mark Brown
Quoted 5 lines · Andrew F. Davis
Quoted 3 lines · Mark Brown

Then if you want to upstream the driver you'll have to add the platform data support again. Like I say not all architectures have anything other than board files.

Quoted 9 lines
Quoted 3 lines

Then they can try, but they will rightfully get nack'd and told to stop using board files and use DT/ACPI. Most upstream architectures don't use board files anymore anyway, so I doubt this will ever happen.

No. To repeat, not all architectures use DT or ACPI. Expecting someone to impelement DT or ACPI support for an entire architecture and try to bring the ecosystem for that architecture along in order to add machine support is obviously totally unreasonable.

That would be unreasonable I agree, but it's also completely hypothetical, as again, there are no in-tree users and most platforms are DT/ACPI, so the odds of anyone needing it are next to nothing.

maintainer
#25 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 3 lines · Andrew F. Davis

That would be unreasonable I agree, but it's also completely hypothetical, as again, there are no in-tree users and most platforms are DT/ACPI, so the odds of anyone needing it are next to nothing.

You're removing support for something someone might want to use for no clear gain. The bar for doing that needs to be higher than just random cleanup, it needs to actively bring some benefit that justifies the cost. If something is sitting there not getting in the way and is potentially going to be helpful for something in the future then there needs to be a positive reason to take it away.

#26 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 6 lines · Mark Brown
Quoted 3 lines · Andrew F. Davis

That would be unreasonable I agree, but it's also completely hypothetical, as again, there are no in-tree users and most platforms are DT/ACPI, so the odds of anyone needing it are next to nothing.

Quoted 7 lines

You're removing support for something someone might want to use for no clear gain. The bar for doing that needs to be higher than just random cleanup, it needs to actively bring some benefit that justifies the cost. If something is sitting there not getting in the way and is potentially going to be helpful for something in the future then there needs to be a positive reason to take it away.

For some userspace feature sure, but this is kernel code, there is no guarantee for a sable API, in fact some would probably argue even further that there is a guarantee that stuff *will* change and this is a good thing as it kinda serves to punish for those you don't try to upstream.

So the helpfulness bar should be zero for changes that break out-of-tree stuff.

Even more so this patch isn't a zero gain, the cleaner, better looking, and easier to maintain code *is* the benefit in itself. Plus we gain the ability to set mic-gain voltage with ACPI, something you couldn't do before this patch.

maintainer
#27 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 7 lines · Andrew F. Davis

For some userspace feature sure, but this is kernel code, there is no guarantee for a sable API, in fact some would probably argue even further that there is a guarantee that stuff *will* change and this is a good thing as it kinda serves to punish for those you don't try to upstream.

So the helpfulness bar should be zero for changes that break out-of-tree stuff.

There is no need to actively get in people's way or put up barriers to people who do in future decide to upstream things, that doesn't help anyone.

Quoted 4 lines

Even more so this patch isn't a zero gain, the cleaner, better looking, and easier to maintain code *is* the benefit in itself. Plus we gain the ability to set mic-gain voltage with ACPI, something you couldn't do before this patch.

If this patch adds ACPI support then the patch description was clearly not great (I don't think I read the patch itself since the description just said that it was removing platform data without giving a reason, that's the main review comment here).

If you want to use the device property stuff that's fine but there's no need to remove platform data to do that, it's a smaller change. I find it hard to see the platform data as a particularly big blight on the code here, looking at the driver it's just going to remove the "pdata." from a few variable accesses which isn't exactly transformational.

#28 lore raw

Re: [PATCH v2 09/19] ASoC: tlv320aic31xx: Remove platform data

Quoted 19 lines · Mark Brown
Quoted 7 lines · Andrew F. Davis

For some userspace feature sure, but this is kernel code, there is no guarantee for a sable API, in fact some would probably argue even further that there is a guarantee that stuff *will* change and this is a good thing as it kinda serves to punish for those you don't try to upstream.

So the helpfulness bar should be zero for changes that break out-of-tree stuff.

There is no need to actively get in people's way or put up barriers to people who do in future decide to upstream things, that doesn't help anyone.

Quoted 4 lines

Even more so this patch isn't a zero gain, the cleaner, better looking, and easier to maintain code *is* the benefit in itself. Plus we gain the ability to set mic-gain voltage with ACPI, something you couldn't do before this patch.

Quoted 5 lines

If this patch adds ACPI support then the patch description was clearly not great (I don't think I read the patch itself since the description just said that it was removing platform data without giving a reason, that's the main review comment here).

I may not be clear that the ACPI part is new, but the message does say "and switch to using fwnode(DT/ACPI)"

Quoted 6 lines

If you want to use the device property stuff that's fine but there's no need to remove platform data to do that, it's a smaller change. I find it hard to see the platform data as a particularly big blight on the code here, looking at the driver it's just going to remove the "pdata." from a few variable accesses which isn't exactly transformational.

If keeping platform data is that important to you then I will split the patch into fwnode addition and pdata removal, you can just not take the pdata removal if you don't want it.

#29 lore raw

PATCH v2 10/19 ASoC: tlv320aic31xx: Add MICBIAS off setting

In reply to Andrew F. Davis

Leaving microphone bias off is a valid setting and even used in the DT binding document example. Add this setting here and document the same.

---
 Documentation/devicetree/bindings/sound/tlv320aic31xx.txt | 1 +
 include/dt-bindings/sound/tlv320aic31xx-micbias.h         | 1 +
 sound/soc/codecs/tlv320aic31xx.c                          | 1 +
 3 files changed, 3 insertions(+)
Documentation/devicetree/bindings/sound/tlv320aic31xx.txt +1 -0
index 5b3c33bb99e5..411cc46a2c58 100644
@@ -24,6 +24,7 @@ Optional properties:
 
 - reset-gpios - GPIO specification for the active low RESET input.
 - ai31xx-micbias-vg - MicBias Voltage setting
+        0 or MICBIAS_OFF - MICBIAS output is powered off
         1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V
         2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V
         3 or MICBIAS_AVDD - MICBIAS output is connected to AVDD
include/dt-bindings/sound/tlv320aic31xx-micbias.h +1 -0
index c6895a18a455..069484070fcf 100644
@@ -2,6 +2,7 @@
 #ifndef __DT_TLV320AIC31XX_MICBIAS_H
 #define __DT_TLV320AIC31XX_MICBIAS_H
 
+#define MICBIAS_OFF		0
 #define MICBIAS_2_0V		1
 #define MICBIAS_2_5V		2
 #define MICBIAS_AVDDV		3
sound/soc/codecs/tlv320aic31xx.c +1 -0
index 0e00421d363b..252d99af6688 100644
@@ -1298,6 +1298,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 	fwnode_property_read_u32(aic31xx->dev->fwnode, "ai31xx-micbias-vg",
 				 &micbias_value);
 	switch (micbias_value) {
+	case MICBIAS_OFF:
 	case MICBIAS_2_0V:
 	case MICBIAS_2_5V:
 	case MICBIAS_AVDDV:
Signature · 1 line
2.15.0
maintainer
#30 lore raw

Re: [PATCH v2 10/19] ASoC: tlv320aic31xx: Add MICBIAS off setting

Quoted 8 lines · Andrew F. Davis

Leaving microphone bias off is a valid setting and even used in the DT binding document example. Add this setting here and document the same.

- ai31xx-micbias-vg - MicBias Voltage setting
+ 0 or MICBIAS_OFF - MICBIAS output is powered off
1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V 2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V 3 or MICBIAS_AVDD - MICBIAS output is connected to AVDD

This doesn't make much sense. If we enable the microphone with a voltage of 0V that's not going to work terribly well - I'd expect this property to control the voltage set when it's enabled, not if it's enabled.

#31 lore raw

Re: [PATCH v2 10/19] ASoC: tlv320aic31xx: Add MICBIAS off setting

Quoted 10 lines · Mark Brown
Quoted 8 lines · Andrew F. Davis

Leaving microphone bias off is a valid setting and even used in the DT binding document example. Add this setting here and document the same.

- ai31xx-micbias-vg - MicBias Voltage setting
+ 0 or MICBIAS_OFF - MICBIAS output is powered off
1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V 2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V 3 or MICBIAS_AVDD - MICBIAS output is connected to AVDD

Quoted 5 lines

This doesn't make much sense. If we enable the microphone with a voltage of 0V that's not going to work terribly well - I'd expect this property to control the voltage set when it's enabled, not if it's enabled.

I agree about this not making much sense, but I'm not sure the right way forward here.

Personally I wouldn't have put MicBias voltage in DT to begin with, it is runtime configurable and depends on user runtime inserted microphone, not really a hardware description :/

The enable/disable is controlled by a DAPM and should be wired to enable whenever we are recording from the related microphone pin. So then without MICBIAS_OFF the user is forced to select from the above voltages whenever recording, which may not be correct if they are using a mic that doesn't need bias.

The best option I think would be to drop this from DT, but I don't feel like fighting that battle right now.

maintainer
#32 lore raw

Re: [PATCH v2 10/19] ASoC: tlv320aic31xx: Add MICBIAS off setting

Quoted 3 lines · Andrew F. Davis

Personally I wouldn't have put MicBias voltage in DT to begin with, it is runtime configurable and depends on user runtime inserted microphone, not really a hardware description :/

Usually the system as a whole is specified to have some particular kind of microphone attached (or soldered in) and this follows from that. There's typically also soldered down analogue components that are selected based on the bias voltage so a specific system likely won't have the full flexibility.

Quoted 5 lines

The enable/disable is controlled by a DAPM and should be wired to enable whenever we are recording from the related microphone pin. So then without MICBIAS_OFF the user is forced to select from the above voltages whenever recording, which may not be correct if they are using a mic that doesn't need bias.

That'd depend on someone having a board that was able to detect that a line input was plugged in or a user control for this. If the board was permenantly set up for line input presumably the bias wouldn't even be wired up.

#33 lore raw

PATCH v2 11/19 ASoC: tlv320aic31xx: Check clock and divider before division

In reply to Andrew F. Davis

If our set_sysclk DAI callback has not been called yet p_div will be 0 and dividing by this will cause an error. Print an error message and leave before this.

---
 sound/soc/codecs/tlv320aic31xx.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
sound/soc/codecs/tlv320aic31xx.c +7 -1
index 252d99af6688..41e2d8077a62 100644
@@ -755,11 +755,17 @@ static int aic31xx_setup_pll(struct snd_soc_codec *codec,
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
 	int bclk_score = snd_soc_params_to_frame_size(params);
-	int mclk_p = aic31xx->sysclk / aic31xx->p_div;
+	int mclk_p;
 	int bclk_n = 0;
 	int match = -1;
 	int i;
 
+	if (!aic31xx->sysclk || !aic31xx->p_div) {
+		dev_err(codec->dev, "Master clock not supplied\n");
+		return -EINVAL;
+	}
+	mclk_p = aic31xx->sysclk / aic31xx->p_div;
+
 	/* Use PLL as CODEC_CLKIN and DAC_CLK as BDIV_CLKIN */
 	snd_soc_update_bits(codec, AIC31XX_CLKMUX,
 			    AIC31XX_CODEC_CLKIN_MASK, AIC31XX_CODEC_CLKIN_PLL);
Signature · 1 line
2.15.0
#34 lore raw

PATCH v2 12/19 ASoC: tlv320aic31xx: Add CODEC clock slave support

In reply to Andrew F. Davis

This CODEC supports being the WCLK and/or BCLK slave, add support for this here.

Also make the alert into an error as alert is more urgent than needed here and is rarely used.

---
 sound/soc/codecs/tlv320aic31xx.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
sound/soc/codecs/tlv320aic31xx.c +9 -1
index 41e2d8077a62..b39db3eb06d0 100644
@@ -926,8 +926,16 @@ static int aic31xx_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	case SND_SOC_DAIFMT_CBM_CFM:
 		iface_reg1 |= AIC31XX_BCLK_MASTER | AIC31XX_WCLK_MASTER;
 		break;
+	case SND_SOC_DAIFMT_CBS_CFM:
+		iface_reg1 |= AIC31XX_WCLK_MASTER;
+		break;
+	case SND_SOC_DAIFMT_CBM_CFS:
+		iface_reg1 |= AIC31XX_BCLK_MASTER;
+		break;
+	case SND_SOC_DAIFMT_CBS_CFS:
+		break;
 	default:
-		dev_alert(codec->dev, "Invalid DAI master/slave interface\n");
+		dev_err(codec->dev, "Invalid DAI master/slave interface\n");
 		return -EINVAL;
 	}
 
Signature · 1 line
2.15.0
maintainer
#35 lore raw

Applied "ASoC: tlv320aic31xx: Add CODEC clock slave support" to the asoc tree

The patch

ASoC: tlv320aic31xx: Add CODEC clock slave support

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 77f8b3cfc33cd4231cc2748bcac9f43b9eea546c Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:53 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: Add CODEC clock slave support

This CODEC supports being the WCLK and/or BCLK slave, add support for this here.

Also make the alert into an error as alert is more urgent than needed here and is rarely used.

---
 sound/soc/codecs/tlv320aic31xx.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
sound/soc/codecs/tlv320aic31xx.c +9 -1
index 05e6d194d6a9..d77cc36d7360 100644
@@ -925,8 +925,16 @@ static int aic31xx_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	case SND_SOC_DAIFMT_CBM_CFM:
 		iface_reg1 |= AIC31XX_BCLK_MASTER | AIC31XX_WCLK_MASTER;
 		break;
+	case SND_SOC_DAIFMT_CBS_CFM:
+		iface_reg1 |= AIC31XX_WCLK_MASTER;
+		break;
+	case SND_SOC_DAIFMT_CBM_CFS:
+		iface_reg1 |= AIC31XX_BCLK_MASTER;
+		break;
+	case SND_SOC_DAIFMT_CBS_CFS:
+		break;
 	default:
-		dev_alert(codec->dev, "Invalid DAI master/slave interface\n");
+		dev_err(codec->dev, "Invalid DAI master/slave interface\n");
 		return -EINVAL;
 	}
 
Signature · 1 line
2.15.0
#36 lore raw

PATCH v2 13/19 ASoC: tlv320aic31xx: Fix inverted BCLK handling

In reply to Andrew F. Davis

Currently BCLK inverting is only handled when the DAI format is DSP, but the BCLK may be inverted in any supported mode. Without this using this CODEC in any other mode than DSP with the BCLK inverted leads to bad sampling timing and very poor audio quality.

Fixes e00447fafbf7 ("ASoC: tlv320aic31xx: Add basic codec driver implementation")
---
 sound/soc/codecs/tlv320aic31xx.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +18 -10
index b39db3eb06d0..f7b1ec96d826 100644
@@ -939,6 +939,18 @@ static int aic31xx_set_dai_fmt(struct snd_soc_dai *codec_dai,
 		return -EINVAL;
 	}
 
+	/* signal polarity */
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_NB_NF:
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		iface_reg2 |= AIC31XX_BCLKINV_MASK;
+		break;
+	default:
+		dev_err(codec->dev, "Invalid DAI clock signal polarity\n");
+		return -EINVAL;
+	}
+
 	/* interface format */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
@@ -946,16 +958,12 @@ static int aic31xx_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	case SND_SOC_DAIFMT_DSP_A:
 		dsp_a_val = 0x1; /* fall through */
 	case SND_SOC_DAIFMT_DSP_B:
-		/* NOTE: BCLKINV bit value 1 equas NB and 0 equals IB */
-		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
-		case SND_SOC_DAIFMT_NB_NF:
-			iface_reg2 |= AIC31XX_BCLKINV_MASK;
-			break;
-		case SND_SOC_DAIFMT_IB_NF:
-			break;
-		default:
-			return -EINVAL;
-		}
+		/*
+		 * NOTE: This CODEC samples on the falling edge of BCLK in
+		 * DSP mode, this is inverted compared to what most DAIs
+		 * expect, so we invert for this mode
+		 */
+		iface_reg2 ^= AIC31XX_BCLKINV_MASK;
 		iface_reg1 |= (AIC31XX_DSP_MODE <<
 			       AIC31XX_IFACE1_DATATYPE_SHIFT);
 		break;
Signature · 1 line
2.15.0
maintainer
#37 lore raw

Applied "ASoC: tlv320aic31xx: Handle inverted BCLK in non-DSP modes" to the asoc tree

The patch

ASoC: tlv320aic31xx: Handle inverted BCLK in non-DSP modes

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From dcb407b257af06fa58b0544ec01ec9e0d3927e02 Mon Sep 17 00:00:00 2001

From: "Andrew F. Davis" <[email protected]>
Date: Wed, 29 Nov 2017 15:32:54 -0600
Subject: [PATCH] ASoC: tlv320aic31xx: Handle inverted BCLK in non-DSP modes

Currently BCLK inverting is only handled when the DAI format is DSP, but the BCLK may be inverted in any supported mode. Without this using this CODEC in any other mode than DSP with the BCLK inverted leads to bad sampling timing and very poor audio quality.

---
 sound/soc/codecs/tlv320aic31xx.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +18 -10
index d77cc36d7360..38fd6ea275fb 100644
@@ -938,6 +938,18 @@ static int aic31xx_set_dai_fmt(struct snd_soc_dai *codec_dai,
 		return -EINVAL;
 	}
 
+	/* signal polarity */
+	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+	case SND_SOC_DAIFMT_NB_NF:
+		break;
+	case SND_SOC_DAIFMT_IB_NF:
+		iface_reg2 |= AIC31XX_BCLKINV_MASK;
+		break;
+	default:
+		dev_err(codec->dev, "Invalid DAI clock signal polarity\n");
+		return -EINVAL;
+	}
+
 	/* interface format */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
@@ -945,16 +957,12 @@ static int aic31xx_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	case SND_SOC_DAIFMT_DSP_A:
 		dsp_a_val = 0x1; /* fall through */
 	case SND_SOC_DAIFMT_DSP_B:
-		/* NOTE: BCLKINV bit value 1 equas NB and 0 equals IB */
-		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
-		case SND_SOC_DAIFMT_NB_NF:
-			iface_reg2 |= AIC31XX_BCLKINV_MASK;
-			break;
-		case SND_SOC_DAIFMT_IB_NF:
-			break;
-		default:
-			return -EINVAL;
-		}
+		/*
+		 * NOTE: This CODEC samples on the falling edge of BCLK in
+		 * DSP mode, this is inverted compared to what most DAIs
+		 * expect, so we invert for this mode
+		 */
+		iface_reg2 ^= AIC31XX_BCLKINV_MASK;
 		iface_reg1 |= (AIC31XX_DSP_MODE <<
 			       AIC31XX_IFACE1_DATATYPE_SHIFT);
 		break;
Signature · 1 line
2.15.0
#38 lore raw

PATCH v2 14/19 ASoC: tlv320aic31xx: Remove regulator notification handling

In reply to Andrew F. Davis

A regulator being forcefully disabled is a catastrophic event that should never happen to most devices, especially not sound CODECs. In addition, our handler sets the reset line but never disables it as no one is listening for an enable event, this is certainly broken and was mosy likely just copied from other CODECs, lets just remove this code.

---
 sound/soc/codecs/tlv320aic31xx.c | 50 ----------------------------------------
 1 file changed, 50 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +0 -50
index f7b1ec96d826..b51c2777e8d1 100644
@@ -161,7 +161,6 @@ struct aic31xx_priv {
 	struct gpio_desc *gpio_reset;
 	int micbias_vg;
 	struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES];
-	struct aic31xx_disable_nb disable_nb[AIC31XX_NUM_SUPPLIES];
 	unsigned int sysclk;
 	u8 p_div;
 	int rate_div_line;
@@ -1032,28 +1031,6 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
-static int aic31xx_regulator_event(struct notifier_block *nb,
-				   unsigned long event, void *data)
-{
-	struct aic31xx_disable_nb *disable_nb =
-		container_of(nb, struct aic31xx_disable_nb, nb);
-	struct aic31xx_priv *aic31xx = disable_nb->aic31xx;
-
-	if (event & REGULATOR_EVENT_DISABLE) {
-		/*
-		 * Put codec to reset and as at least one of the
-		 * supplies was disabled.
-		 */
-		if (aic31xx->gpio_reset)
-			gpiod_set_value(aic31xx->gpio_reset, 1);
-
-		regcache_mark_dirty(aic31xx->regmap);
-		dev_dbg(aic31xx->dev, "## %s: DISABLE received\n", __func__);
-	}
-
-	return 0;
-}
-
 static void aic31xx_clk_on(struct snd_soc_codec *codec)
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
@@ -1167,20 +1144,6 @@ static int aic31xx_codec_probe(struct snd_soc_codec *codec)
 
 	aic31xx->codec = codec;
 
-	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++) {
-		aic31xx->disable_nb[i].nb.notifier_call =
-			aic31xx_regulator_event;
-		aic31xx->disable_nb[i].aic31xx = aic31xx;
-		ret = regulator_register_notifier(aic31xx->supplies[i].consumer,
-						  &aic31xx->disable_nb[i].nb);
-		if (ret) {
-			dev_err(codec->dev,
-				"Failed to request regulator notifier: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
 	regcache_cache_only(aic31xx->regmap, true);
 	regcache_mark_dirty(aic31xx->regmap);
 
@@ -1195,21 +1158,8 @@ static int aic31xx_codec_probe(struct snd_soc_codec *codec)
 	return 0;
 }
 
-static int aic31xx_codec_remove(struct snd_soc_codec *codec)
-{
-	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(aic31xx->supplies); i++)
-		regulator_unregister_notifier(aic31xx->supplies[i].consumer,
-					      &aic31xx->disable_nb[i].nb);
-
-	return 0;
-}
-
 static const struct snd_soc_codec_driver soc_codec_driver_aic31xx = {
 	.probe			= aic31xx_codec_probe,
-	.remove			= aic31xx_codec_remove,
 	.set_bias_level		= aic31xx_set_bias_level,
 	.suspend_bias_off	= true,
 
Signature · 1 line
2.15.0
maintainer
#39 lore raw

Re: [PATCH v2 14/19] ASoC: tlv320aic31xx: Remove regulator notification handling

Quoted 2 lines · Andrew F. Davis

A regulator being forcefully disabled is a catastrophic event that should never happen to most devices, especially not sound CODECs.

That's not what the disable notification handling is for. It's there so that the driver can skip having to reinitialize the device if other constraints mean the power doesn't actually get turned off when it disables the regualtors. It's nothing to do with forced disables.

#40 lore raw

Re: [PATCH v2 14/19] ASoC: tlv320aic31xx: Remove regulator notification handling

Quoted 9 lines · Mark Brown
Quoted 2 lines · Andrew F. Davis

A regulator being forcefully disabled is a catastrophic event that should never happen to most devices, especially not sound CODECs.

That's not what the disable notification handling is for. It's there so that the driver can skip having to reinitialize the device if other constraints mean the power doesn't actually get turned off when it disables the regualtors. It's nothing to do with forced disables.

Looking into the call sites, at least in this case the only time this notification will be called, outside the normal enable/disable paths (which do the same thing here: turn on regmap cache only mode and mark it dirty), will be during a force disable scenario.

maintainer
#41 lore raw

Re: [PATCH v2 14/19] ASoC: tlv320aic31xx: Remove regulator notification handling

Quoted 4 lines · Andrew F. Davis

Looking into the call sites, at least in this case the only time this notification will be called, outside the normal enable/disable paths (which do the same thing here: turn on regmap cache only mode and mark it dirty), will be during a force disable scenario.

If all the disable sites in the driver mark the cache dirty then they should be fixed not to.

#42 lore raw

PATCH v2 15/19 ASoC: tlv320aic31xx: Reset registers during power up

In reply to Andrew F. Davis

Add a reset function that toggles the reset line if available or uses the software reset command otherwise. Use this in power up to ensure the registers are in a sane state. This is useful when the driver module is reloaded, or after Kexec, warm-reboots, etc..

---
 sound/soc/codecs/tlv320aic31xx.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +22 -4
index b51c2777e8d1..77ae8f36a943 100644
@@ -1031,6 +1031,24 @@ static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+static int aic31xx_reset(struct aic31xx_priv *aic31xx)
+{
+	int ret = 0;
+
+	if (aic31xx->gpio_reset) {
+		gpiod_set_value(aic31xx->gpio_reset, 1);
+		ndelay(10); /* At least 10ns */
+		gpiod_set_value(aic31xx->gpio_reset, 0);
+	} else {
+		ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1);
+		if (ret < 0)
+			dev_err(aic31xx->dev, "Could not reset device\n");
+	}
+	mdelay(1); /* At least 1ms */
+
+	return ret;
+}
+
 static void aic31xx_clk_on(struct snd_soc_codec *codec)
 {
 	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
@@ -1074,11 +1092,11 @@ static int aic31xx_power_on(struct snd_soc_codec *codec)
 	if (ret)
 		return ret;
 
-	if (aic31xx->gpio_reset) {
-		gpiod_set_value(aic31xx->gpio_reset, 0);
-		udelay(100);
-	}
 	regcache_cache_only(aic31xx->regmap, false);
+
+	/* Reset device registers for a consistent power-on like state */
+	aic31xx_reset(aic31xx);
+
 	ret = regcache_sync(aic31xx->regmap);
 	if (ret) {
 		dev_err(codec->dev,
Signature · 1 line
2.15.0
maintainer
#43 lore raw

Re: [PATCH v2 15/19] ASoC: tlv320aic31xx: Reset registers during power up

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 1041

Quoted 3 lines · Andrew F. Davis
sound/soc/codecs/tlv320aic31xx.c +3 -0
+		gpiod_set_value(aic31xx->gpio_reset, 1);
+		ndelay(10); /* At least 10ns */
+		gpiod_set_value(aic31xx->gpio_reset, 0);

_cansleep(), this isn't being done from interrupt context.

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 1045

Quoted 4 lines
sound/soc/codecs/tlv320aic31xx.c +4 -0
+	} else {
+		ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1);
+		if (ret < 0)
+			dev_err(aic31xx->dev, "Could not reset device\n");

Print the error to help people doing debug.

#44 lore raw

Re: [PATCH v2 15/19] ASoC: tlv320aic31xx: Reset registers during power up

Quoted 8 lines · Mark Brown

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 1041

Quoted 3 lines · Andrew F. Davis
sound/soc/codecs/tlv320aic31xx.c +3 -0
+		gpiod_set_value(aic31xx->gpio_reset, 1);
+		ndelay(10); /* At least 10ns */
+		gpiod_set_value(aic31xx->gpio_reset, 0);

_cansleep(), this isn't being done from interrupt context.

will fix

Quoted 7 lines

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 1045

Quoted 4 lines
sound/soc/codecs/tlv320aic31xx.c +4 -0
+	} else {
+		ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1);
+		if (ret < 0)
+			dev_err(aic31xx->dev, "Could not reset device\n");

Print the error to help people doing debug.

Do you mean by adding the ret code to the print?

maintainer
#45 lore raw

Re: [PATCH v2 15/19] ASoC: tlv320aic31xx: Reset registers during power up

Quoted 11 lines · Andrew F. Davis
Quoted 8 lines · Mark Brown

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_set_dai_sysclk(struct snd_soc_dai *codec_dai, · line 1045

Quoted 4 lines · Andrew F. Davis
sound/soc/codecs/tlv320aic31xx.c +4 -0
+	} else {
+		ret = regmap_write(aic31xx->regmap, AIC31XX_RESET, 1);
+		if (ret < 0)
+			dev_err(aic31xx->dev, "Could not reset device\n");

Print the error to help people doing debug.

Do you mean by adding the ret code to the print?

Yes.

#46 lore raw

PATCH v2 16/19 ASoC: tlv320aic31xx: Add short circuit detection support

In reply to Andrew F. Davis

These devices support detecting and reporting short circuits across the output stages. Add support for reporting these issue. Do this by registering an interrupt if available and enabling this error to trigger that interrupt in the device.

---
 sound/soc/codecs/tlv320aic31xx.c | 42 ++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/tlv320aic31xx.h | 16 +++++++++++++++
 2 files changed, 58 insertions(+)
sound/soc/codecs/tlv320aic31xx.c +42 -0
index 77ae8f36a943..fd3109366377 100644
@@ -164,6 +164,7 @@ struct aic31xx_priv {
 	unsigned int sysclk;
 	u8 p_div;
 	int rate_div_line;
+	int irq;
 };
 
 struct aic31xx_rate_divs {
@@ -1258,6 +1259,27 @@ static const struct acpi_device_id aic31xx_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, aic31xx_acpi_match);
 #endif
 
+static irqreturn_t aic31xx_irq(int irq, void *data)
+{
+	struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data;
+	struct device *dev = aic31xx->dev;
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value);
+	if (ret) {
+		dev_err(dev, "Failed to read interrupt mask: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (value & AIC31XX_HPLSCDETECT)
+		dev_err(dev, "Short circuit on Left output is detected\n");
+	if (value & AIC31XX_HPRSCDETECT)
+		dev_err(dev, "Short circuit on Right output is detected\n");
+
+	return IRQ_HANDLED;
+}
+
 static int aic31xx_i2c_probe(struct i2c_client *i2c,
 			     const struct i2c_device_id *id)
 {
@@ -1280,6 +1302,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 	aic31xx->dev = &i2c->dev;
+	aic31xx->irq = i2c->irq;
 
 	aic31xx->codec_type = id->driver_data;
 
@@ -1318,6 +1341,25 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (aic31xx->irq > 0) {
+		regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1,
+				   AIC31XX_GPIO1_FUNC_MASK,
+				   AIC31XX_GPIO1_INT1 <<
+				   AIC31XX_GPIO1_FUNC_SHIFT);
+
+		regmap_write(aic31xx->regmap, AIC31XX_INT1CTRL,
+			     AIC31XX_SC);
+
+		ret = devm_request_threaded_irq(aic31xx->dev, aic31xx->irq,
+						NULL, aic31xx_irq,
+						IRQF_ONESHOT, "aic31xx-irq",
+						aic31xx);
+		if (ret) {
+			dev_err(aic31xx->dev, "Unable to request IRQ\n");
+			return ret;
+		}
+	}
+
 	if (aic31xx->codec_type & DAC31XX_BIT)
 		return snd_soc_register_codec(&i2c->dev,
 				&soc_codec_driver_aic31xx,
sound/soc/codecs/tlv320aic31xx.h +16 -0
index ab94e6a0c742..9dc85b6f6ad3 100644
@@ -184,6 +184,22 @@ enum aic31xx_type {
 #define AIC31XX_SC			BIT(3)
 #define AIC31XX_ENGINE			BIT(2)
 
+/* AIC31XX_GPIO1 */
+#define AIC31XX_GPIO1_FUNC_MASK		GENMASK(5, 2)
+#define AIC31XX_GPIO1_FUNC_SHIFT	2
+#define AIC31XX_GPIO1_DISABLED		0x00
+#define AIC31XX_GPIO1_INPUT		0x01
+#define AIC31XX_GPIO1_GPI		0x02
+#define AIC31XX_GPIO1_GPO		0x03
+#define AIC31XX_GPIO1_CLKOUT		0x04
+#define AIC31XX_GPIO1_INT1		0x05
+#define AIC31XX_GPIO1_INT2		0x06
+#define AIC31XX_GPIO1_ADC_WCLK		0x07
+#define AIC31XX_GPIO1_SBCLK		0x08
+#define AIC31XX_GPIO1_SWCLK		0x09
+#define AIC31XX_GPIO1_ADC_MOD_CLK	0x10
+#define AIC31XX_GPIO1_SDOUT		0x11
+
 /* AIC31XX_DACSETUP */
 #define AIC31XX_SOFTSTEP_MASK		GENMASK(1, 0)
 
Signature · 1 line
2.15.0
maintainer
#47 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

sound/soc/codecs/tlv320aic31xx.c static const struct acpi_device_id aic31xx_acpi_match[] = { · line 1264

Quoted 3 lines · Andrew F. Davis
sound/soc/codecs/tlv320aic31xx.c +3 -0
+static irqreturn_t aic31xx_irq(int irq, void *data)
+{
+	struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data;

There is no need to cast away from void, doing so can only mask bugs.

sound/soc/codecs/tlv320aic31xx.c static const struct acpi_device_id aic31xx_acpi_match[] = { · line 1280

Quoted 12 lines
sound/soc/codecs/tlv320aic31xx.c +12 -0
+	ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value);
+	if (ret) {
+		dev_err(dev, "Failed to read interrupt mask: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (value & AIC31XX_HPLSCDETECT)
+		dev_err(dev, "Short circuit on Left output is detected\n");
+	if (value & AIC31XX_HPRSCDETECT)
+		dev_err(dev, "Short circuit on Right output is detected\n");
+
+	return IRQ_HANDLED;

This will report the interrupt as handled even if we didn't see an interrupt we understand which will break shared interrupt lines. At the very least we should log other interrupt sources numerically.

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_i2c_probe(struct i2c_client *i2c, · line 1348

Quoted 5 lines
sound/soc/codecs/tlv320aic31xx.c +5 -0
+	if (aic31xx->irq > 0) {
+		regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1,
+				   AIC31XX_GPIO1_FUNC_MASK,
+				   AIC31XX_GPIO1_INT1 <<
+				   AIC31XX_GPIO1_FUNC_SHIFT);

Is the interrupt only available on GPIO1?

#48 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

Quoted 8 lines · Mark Brown

sound/soc/codecs/tlv320aic31xx.c static const struct acpi_device_id aic31xx_acpi_match[] = { · line 1264

Quoted 3 lines · Andrew F. Davis
sound/soc/codecs/tlv320aic31xx.c +3 -0
+static irqreturn_t aic31xx_irq(int irq, void *data)
+{
+	struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data;

There is no need to cast away from void, doing so can only mask bugs.

Quoted 17 lines

sound/soc/codecs/tlv320aic31xx.c static const struct acpi_device_id aic31xx_acpi_match[] = { · line 1280

Quoted 12 lines
sound/soc/codecs/tlv320aic31xx.c +12 -0
+	ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value);
+	if (ret) {
+		dev_err(dev, "Failed to read interrupt mask: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (value & AIC31XX_HPLSCDETECT)
+		dev_err(dev, "Short circuit on Left output is detected\n");
+	if (value & AIC31XX_HPRSCDETECT)
+		dev_err(dev, "Short circuit on Right output is detected\n");
+
+	return IRQ_HANDLED;

This will report the interrupt as handled even if we didn't see an interrupt we understand which will break shared interrupt lines. At the very least we should log other interrupt sources numerically.

Okay, I think I can make that work by checking if no bits are set in the interrupt regs and returning early if not, IRQ_NONE.

Quoted 8 lines

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_i2c_probe(struct i2c_client *i2c, · line 1348

Quoted 5 lines
sound/soc/codecs/tlv320aic31xx.c +5 -0
+	if (aic31xx->irq > 0) {
+		regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1,
+				   AIC31XX_GPIO1_FUNC_MASK,
+				   AIC31XX_GPIO1_INT1 <<
+				   AIC31XX_GPIO1_FUNC_SHIFT);

Is the interrupt only available on GPIO1?

Some devices can route this to GPIO2 IIRC.

I'm not sure how that would be supported, I think we would need to add interrupt names to DT so users could specify which gpio they wired their IRQ lines to.

interrupt = <&host 23>;
interrupt-name = "gpio2";

or similar?

maintainer
#49 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

Quoted 6 lines · Andrew F. Davis
Quoted 1 line · Mark Brown

Is the interrupt only available on GPIO1?

Some devices can route this to GPIO2 IIRC.

Quoted 8 lines

I'm not sure how that would be supported, I think we would need to add interrupt names to DT so users could specify which gpio they wired their IRQ lines to.

interrupt = <&host 23>;
interrupt-name = "gpio2";

or similar?

You could also use pinctrl an require the user to mux the interrupt in whatever fashion makes sense for their device.

#50 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

Quoted 11 lines · Mark Brown
Quoted 9 lines · Andrew F. Davis
Quoted 1 line · Mark Brown

Is the interrupt only available on GPIO1?

Some devices can route this to GPIO2 IIRC.

I'm not sure how that would be supported, I think we would need to add interrupt names to DT so users could specify which gpio they wired their IRQ lines to.

Quoted 8 lines
Quoted 4 lines

interrupt = <&host 23>;
interrupt-name = "gpio2";

or similar?

You could also use pinctrl an require the user to mux the interrupt in whatever fashion makes sense for their device.

If done at that layer then no change is needed in the driver right? We just request and use the IRQ passed to us from i2c data.

#51 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

In reply to Andrew F. Davis
Quoted 27 lines · Andrew F. Davis
Quoted 8 lines · Mark Brown

sound/soc/codecs/tlv320aic31xx.c static const struct acpi_device_id aic31xx_acpi_match[] = { · line 1264

Quoted 3 lines · Andrew F. Davis
sound/soc/codecs/tlv320aic31xx.c +3 -0
+static irqreturn_t aic31xx_irq(int irq, void *data)
+{
+	struct aic31xx_priv *aic31xx = (struct aic31xx_priv *)data;

There is no need to cast away from void, doing so can only mask bugs.

Quoted 17 lines

sound/soc/codecs/tlv320aic31xx.c static const struct acpi_device_id aic31xx_acpi_match[] = { · line 1280

Quoted 12 lines
sound/soc/codecs/tlv320aic31xx.c +12 -0
+	ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG, &value);
+	if (ret) {
+		dev_err(dev, "Failed to read interrupt mask: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (value & AIC31XX_HPLSCDETECT)
+		dev_err(dev, "Short circuit on Left output is detected\n");
+	if (value & AIC31XX_HPRSCDETECT)
+		dev_err(dev, "Short circuit on Right output is detected\n");
+
+	return IRQ_HANDLED;

This will report the interrupt as handled even if we didn't see an interrupt we understand which will break shared interrupt lines. At the very least we should log other interrupt sources numerically.

Quoted 3 lines

Okay, I think I can make that work by checking if no bits are set in the interrupt regs and returning early if not, IRQ_NONE.

This turned out to be more difficult than I expected, plus if I do handle an interrupt it doesn't mean the other device did not right? So this wouldn't fix shared lines as far as I can tell, but I don't register it as shared so this should be fine.

As for your other suggestion of "log other interrupt sources numerically", could you explain this or point to an example of what you mean?

Quoted 20 lines
Quoted 8 lines

sound/soc/codecs/tlv320aic31xx.c static int aic31xx_i2c_probe(struct i2c_client *i2c, · line 1348

Quoted 5 lines
sound/soc/codecs/tlv320aic31xx.c +5 -0
+	if (aic31xx->irq > 0) {
+		regmap_update_bits(aic31xx->regmap, AIC31XX_GPIO1,
+				   AIC31XX_GPIO1_FUNC_MASK,
+				   AIC31XX_GPIO1_INT1 <<
+				   AIC31XX_GPIO1_FUNC_SHIFT);

Is the interrupt only available on GPIO1?

Some devices can route this to GPIO2 IIRC.

I'm not sure how that would be supported, I think we would need to add interrupt names to DT so users could specify which gpio they wired their IRQ lines to.

interrupt = <&host 23>;
interrupt-name = "gpio2";

or similar?

maintainer
#52 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

Quoted 6 lines · Andrew F. Davis
Quoted 3 lines · Andrew F. Davis
Quoted 3 lines

This will report the interrupt as handled even if we didn't see an interrupt we understand which will break shared interrupt lines. At the very least we should log other interrupt sources numerically.

Quoted 7 lines
Quoted 2 lines

Okay, I think I can make that work by checking if no bits are set in the interrupt regs and returning early if not, IRQ_NONE.

This turned out to be more difficult than I expected, plus if I do handle an interrupt it doesn't mean the other device did not right? So this wouldn't fix shared lines as far as I can tell, but I don't register it as shared so this should be fine.

It'll mean that we don't offer the interrupt to anything else sharing the line.

Quoted 3 lines

As for your other suggestion of "log other interrupt sources numerically", could you explain this or point to an example of what you mean?

Just print out the bits that were set.

#53 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

Quoted 15 lines · Mark Brown
Quoted 6 lines · Andrew F. Davis
Quoted 3 lines · Andrew F. Davis
Quoted 3 lines

This will report the interrupt as handled even if we didn't see an interrupt we understand which will break shared interrupt lines. At the very least we should log other interrupt sources numerically.

Quoted 7 lines
Quoted 2 lines

Okay, I think I can make that work by checking if no bits are set in the interrupt regs and returning early if not, IRQ_NONE.

This turned out to be more difficult than I expected, plus if I do handle an interrupt it doesn't mean the other device did not right? So this wouldn't fix shared lines as far as I can tell, but I don't register it as shared so this should be fine.

Quoted 9 lines

It'll mean that we don't offer the interrupt to anything else sharing the line.

Quoted 3 lines

As for your other suggestion of "log other interrupt sources numerically", could you explain this or point to an example of what you mean?

Just print out the bits that were set.

I don't see anyone else doing this, what would that solve? Maybe I still don't get what you mean here. :(

maintainer
#54 lore raw

Re: [PATCH v2 16/19] ASoC: tlv320aic31xx: Add short circuit detection support

Quoted 10 lines · Andrew F. Davis
Quoted 5 lines · Mark Brown
Quoted 3 lines

As for your other suggestion of "log other interrupt sources numerically", could you explain this or point to an example of what you mean?

Just print out the bits that were set.

I don't see anyone else doing this, what would that solve? Maybe I still don't get what you mean here. :(

A good proportion of devices require explicit acks for interrupts and only ack things they handled so don't have this issue, and otherwise it's common to just handle all the interrupts that the device might fire. The goal is to not silently eat interrupts if the device starts interrupting for some reason.

#55 lore raw

PATCH v2 17/19 ASoC: tlv320aic31xx: Add overflow detection support

In reply to Andrew F. Davis

Similar to short circuit detection, when the ADC/DAC is saturated and overflows poor audio quality can result and should be reported to the user. This device support Automatic Dynamic Range Compression (DRC) to reduce this but it is not enabled currently in this driver.

---
 sound/soc/codecs/tlv320aic31xx.c | 20 +++++++++++++++++++-
 sound/soc/codecs/tlv320aic31xx.h |  7 +++++++
 2 files changed, 26 insertions(+), 1 deletion(-)
sound/soc/codecs/tlv320aic31xx.c +19 -1
index fd3109366377..094b1596f9cc 100644
@@ -1277,6 +1277,23 @@ static irqreturn_t aic31xx_irq(int irq, void *data)
 	if (value & AIC31XX_HPRSCDETECT)
 		dev_err(dev, "Short circuit on Right output is detected\n");
 
+	ret = regmap_read(aic31xx->regmap, AIC31XX_OFFLAG, &value);
+	if (ret) {
+		dev_err(dev, "Failed to read overflow flag: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (value & AIC31XX_DAC_OF_LEFT)
+		dev_err(dev, "Left-channel DAC overflow has occurred\n");
+	if (value & AIC31XX_DAC_OF_RIGHT)
+		dev_err(dev, "Right-channel DAC overflow has occurred\n");
+	if (value & AIC31XX_DAC_OF_SHIFTER)
+		dev_err(dev, "DAC barrel shifter overflow has occurred\n");
+	if (value & AIC31XX_ADC_OF)
+		dev_err(dev, "ADC overflow has occurred\n");
+	if (value & AIC31XX_ADC_OF_SHIFTER)
+		dev_err(dev, "ADC barrel shifter overflow has occurred\n");
+
 	return IRQ_HANDLED;
 }
 
@@ -1348,7 +1365,8 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 				   AIC31XX_GPIO1_FUNC_SHIFT);
 
 		regmap_write(aic31xx->regmap, AIC31XX_INT1CTRL,
-			     AIC31XX_SC);
+			     AIC31XX_SC |
+			     AIC31XX_ENGINE);
 
 		ret = devm_request_threaded_irq(aic31xx->dev, aic31xx->irq,
 						NULL, aic31xx_irq,
sound/soc/codecs/tlv320aic31xx.h +7 -0
index 9dc85b6f6ad3..d062663f66b5 100644
@@ -166,6 +166,13 @@ enum aic31xx_type {
 #define AIC31XX_HPRDRVPWRSTATUS_MASK	BIT(1)
 #define AIC31XX_SPRDRVPWRSTATUS_MASK	BIT(0)
 
+/* AIC31XX_OFFLAG */
+#define AIC31XX_DAC_OF_LEFT		BIT(7)
+#define AIC31XX_DAC_OF_RIGHT		BIT(6)
+#define AIC31XX_DAC_OF_SHIFTER		BIT(5)
+#define AIC31XX_ADC_OF			BIT(3)
+#define AIC31XX_ADC_OF_SHIFTER		BIT(1)
+
 /* AIC31XX_INTRDACFLAG */
 #define AIC31XX_HPLSCDETECT		BIT(7)
 #define AIC31XX_HPRSCDETECT		BIT(6)
Signature · 1 line
2.15.0
#56 lore raw

PATCH v2 18/19 ASoC: tlv320aic31xx: Add headphone/headset detection

In reply to Andrew F. Davis

This device can detect the insertion/removal of headphones and headsets. Enable reporting this status by enabling this interrupt and forwarding this to upper-layers if a jack has been defined.

This jack definition and the resulting operation from a jack detection event must currently be defined by sound card platform code until CODEC outputs to jack mappings can be defined generically.

---
 sound/soc/codecs/tlv320aic31xx.c | 33 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/tlv320aic31xx.h | 11 +++++++++++
 2 files changed, 44 insertions(+)
sound/soc/codecs/tlv320aic31xx.c +33 -0
index 094b1596f9cc..471b31be55d1 100644
@@ -25,6 +25,7 @@
 #include <linux/of_gpio.h>
 #include <linux/slab.h>
 #include <sound/core.h>
+#include <sound/jack.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -89,6 +90,7 @@ static bool aic31xx_volatile(struct device *dev, unsigned int reg)
 	case AIC31XX_INTRADCFLAG: /* Sticky interrupt flags */
 	case AIC31XX_INTRDACFLAG2:
 	case AIC31XX_INTRADCFLAG2:
+	case AIC31XX_HSDETECT:
 		return true;
 	}
 	return false;
@@ -161,6 +163,7 @@ struct aic31xx_priv {
 	struct gpio_desc *gpio_reset;
 	int micbias_vg;
 	struct regulator_bulk_data supplies[AIC31XX_NUM_SUPPLIES];
+	struct snd_soc_jack *jack;
 	unsigned int sysclk;
 	u8 p_div;
 	int rate_div_line;
@@ -1277,6 +1280,32 @@ static irqreturn_t aic31xx_irq(int irq, void *data)
 	if (value & AIC31XX_HPRSCDETECT)
 		dev_err(dev, "Short circuit on Right output is detected\n");
 
+	if (value & AIC31XX_HSPLUG) {
+		int status = 0;
+
+		ret = regmap_read(aic31xx->regmap, AIC31XX_HSDETECT, &value);
+		if (ret) {
+			dev_err(dev, "Failed to read headset type: %d\n", ret);
+			return IRQ_NONE;
+		}
+
+		switch ((value & AIC31XX_HSD_TYPE_MASK) >>
+			AIC31XX_HSD_TYPE_SHIFT) {
+		case AIC31XX_HSD_HP:
+			status |= SND_JACK_HEADPHONE;
+			break;
+		case AIC31XX_HSD_HS:
+			status |= SND_JACK_HEADSET;
+			break;
+		default:
+			break;
+		}
+
+		if (aic31xx->jack)
+			snd_soc_jack_report(aic31xx->jack, status,
+					    AIC31XX_JACK_MASK);
+	}
+
 	ret = regmap_read(aic31xx->regmap, AIC31XX_OFFLAG, &value);
 	if (ret) {
 		dev_err(dev, "Failed to read overflow flag: %d\n", ret);
@@ -1365,9 +1394,13 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 				   AIC31XX_GPIO1_FUNC_SHIFT);
 
 		regmap_write(aic31xx->regmap, AIC31XX_INT1CTRL,
+			     AIC31XX_HSPLUGDET |
 			     AIC31XX_SC |
 			     AIC31XX_ENGINE);
 
+		regmap_write(aic31xx->regmap, AIC31XX_HSDETECT,
+			     AIC31XX_HSD_ENABLE);
+
 		ret = devm_request_threaded_irq(aic31xx->dev, aic31xx->irq,
 						NULL, aic31xx_irq,
 						IRQF_ONESHOT, "aic31xx-irq",
sound/soc/codecs/tlv320aic31xx.h +11 -0
index d062663f66b5..66c85df4d5be 100644
@@ -20,6 +20,9 @@
 #define AIC31XX_MINIDSP_BIT		BIT(2)
 #define DAC31XX_BIT			BIT(3)
 
+#define AIC31XX_JACK_MASK (SND_JACK_HEADPHONE | \
+			   SND_JACK_HEADSET)
+
 enum aic31xx_type {
 	AIC3100	= 0,
 	AIC3110 = AIC31XX_STEREO_CLASS_D_BIT,
@@ -213,6 +216,14 @@ enum aic31xx_type {
 /* AIC31XX_DACMUTE */
 #define AIC31XX_DACMUTE_MASK		GENMASK(3, 2)
 
+/* AIC31XX_HSDETECT */
+#define AIC31XX_HSD_ENABLE		BIT(7)
+#define AIC31XX_HSD_TYPE_MASK		GENMASK(6, 5)
+#define AIC31XX_HSD_TYPE_SHIFT		5
+#define AIC31XX_HSD_NONE		0x00
+#define AIC31XX_HSD_HP			0x01
+#define AIC31XX_HSD_HS			0x03
+
 /* AIC31XX_MICBIAS */
 #define AIC31XX_MICBIAS_MASK		GENMASK(1, 0)
 #define AIC31XX_MICBIAS_SHIFT		0
Signature · 6 lines
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#57 lore raw

Re: [PATCH v2 18/19] ASoC: tlv320aic31xx: Add headphone/headset detection

Quoted 7 lines · Andrew F. Davis

This device can detect the insertion/removal of headphones and headsets. Enable reporting this status by enabling this interrupt and forwarding this to upper-layers if a jack has been defined.

This jack definition and the resulting operation from a jack detection event must currently be defined by sound card platform code until CODEC outputs to jack mappings can be defined generically.

This only does half the job, there's no way for anything to specify a jack here.

#58 lore raw

Re: [PATCH v2 18/19] ASoC: tlv320aic31xx: Add headphone/headset detection

Quoted 5 lines · Mark Brown
Quoted 4 lines · Andrew F. Davis

This device can detect the insertion/removal of headphones and headsets. Enable reporting this status by enabling this interrupt and forwarding this to upper-layers if a jack has been defined.

Quoted 7 lines
Quoted 3 lines

This jack definition and the resulting operation from a jack detection event must currently be defined by sound card platform code until CODEC outputs to jack mappings can be defined generically.

This only does half the job, there's no way for anything to specify a jack here.

Other CODECs drivers expose some kind of platform/machine specific function(s) to send the jack definition to the CODEC, we seem to be missing a generic way to report jack information up to the machine layer driver.

Perhaps a struct with a jack enable/disable and call-back functions could be created when registering the codec/platform component driver? Then machines can hook to this as they need?

maintainer
#59 lore raw

Re: [PATCH v2 18/19] ASoC: tlv320aic31xx: Add headphone/headset detection

Quoted 7 lines · Andrew F. Davis
Quoted 2 lines · Mark Brown

This only does half the job, there's no way for anything to specify a jack here.

Other CODECs drivers expose some kind of platform/machine specific function(s) to send the jack definition to the CODEC, we seem to be

Your driver doesn't do that.

Quoted 2 lines

missing a generic way to report jack information up to the machine layer driver.

snd_soc_codec_set_jack()

#60 lore raw

Re: [PATCH v2 18/19] ASoC: tlv320aic31xx: Add headphone/headset detection

Quoted 11 lines · Mark Brown
Quoted 7 lines · Andrew F. Davis
Quoted 2 lines · Mark Brown

This only does half the job, there's no way for anything to specify a jack here.

Other CODECs drivers expose some kind of platform/machine specific function(s) to send the jack definition to the CODEC, we seem to be

Your driver doesn't do that.

Right, I don't have a specific machine in mind so didn't wan to do it that way.

Quoted 5 lines
Quoted 2 lines

missing a generic way to report jack information up to the machine layer driver.

snd_soc_codec_set_jack()

Hmm, this looks close to what I was thinking (I was wanting to go the other direction and have the codec report what jacks it supports, this could allow for more than one jack), I'll see if I can make it work.

maintainer
#61 lore raw

Re: [PATCH v2 18/19] ASoC: tlv320aic31xx: Add headphone/headset detection

Quoted 10 lines · Andrew F. Davis
Quoted 4 lines · Mark Brown
Quoted 2 lines

missing a generic way to report jack information up to the machine layer driver.

snd_soc_codec_set_jack()

Hmm, this looks close to what I was thinking (I was wanting to go the other direction and have the codec report what jacks it supports, this could allow for more than one jack), I'll see if I can make it work.

Almost always the CODEC requires some extra connections in order to enable jack detection features, these are going to be board specific so they come from the machine driver.

#62 lore raw

PATCH v2 19/19 ASoC: tlv320aic31xx: Add button press detection

In reply to Andrew F. Davis

This device can optionally detect headset or microphone button presses. Add support for this by passing this event to the jack layer.

---
 sound/soc/codecs/tlv320aic31xx.c | 14 +++++++++++++-
 sound/soc/codecs/tlv320aic31xx.h |  3 ++-
 2 files changed, 15 insertions(+), 2 deletions(-)
sound/soc/codecs/tlv320aic31xx.c +13 -1
index 471b31be55d1..8cd27c99cb15 100644
@@ -1280,9 +1280,20 @@ static irqreturn_t aic31xx_irq(int irq, void *data)
 	if (value & AIC31XX_HPRSCDETECT)
 		dev_err(dev, "Short circuit on Right output is detected\n");
 
-	if (value & AIC31XX_HSPLUG) {
+	if (value & (AIC31XX_HSPLUG | AIC31XX_BUTTONPRESS)) {
 		int status = 0;
 
+		ret = regmap_read(aic31xx->regmap, AIC31XX_INTRDACFLAG2,
+				  &value);
+		if (ret) {
+			dev_err(dev, "Failed to read interrupt mask: %d\n",
+				ret);
+			return IRQ_NONE;
+		}
+
+		if (value & AIC31XX_BUTTONPRESS)
+			status |= SND_JACK_BTN_0;
+
 		ret = regmap_read(aic31xx->regmap, AIC31XX_HSDETECT, &value);
 		if (ret) {
 			dev_err(dev, "Failed to read headset type: %d\n", ret);
@@ -1395,6 +1406,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c,
 
 		regmap_write(aic31xx->regmap, AIC31XX_INT1CTRL,
 			     AIC31XX_HSPLUGDET |
+			     AIC31XX_BUTTONPRESSDET |
 			     AIC31XX_SC |
 			     AIC31XX_ENGINE);
 
sound/soc/codecs/tlv320aic31xx.h +2 -1
index 66c85df4d5be..7fce278eadac 100644
@@ -21,7 +21,8 @@
 #define DAC31XX_BIT			BIT(3)
 
 #define AIC31XX_JACK_MASK (SND_JACK_HEADPHONE | \
-			   SND_JACK_HEADSET)
+			   SND_JACK_HEADSET | \
+			   SND_JACK_BTN_0)
 
 enum aic31xx_type {
 	AIC3100	= 0,
Signature · 1 line
2.15.0