[RFC] ASoC: max98357a: manage GPIO for component rebinding

Commit Message

Tzung-Bi Shih April 30, 2019, 8:01 a.m. UTC

Component's probe() gets GPIO for "sdmode-gpios" via
devm_gpiod_get_optional().  The GPIO binds to the device.

When the component get removed but the device does not, the GPIO
still binds to the device.

Then, when the component be probed again, devm_gpiod_get_optional()
returns EBUSY.

Signed-off-by: Tzung-Bi Shih <[e-mail]>
---
Hi,

We discovered the issue when rebinding sound card.  We found that the
rebinding failed due to the GPIO was hold by the first time component's
probe() in max98357.

The patch changes devm to non-devm API, because we don't have equivalent
API for ASoC components (e.g. compm_).

Question: we are not very sure to move from devm to non-devm, because
devm should be most encouraged.
Question: does it suggest not to use devm for GPIO in component's
probe()?  It seems there are still some such usages, for example
sound/soc/codecs/dmic.c.

 sound/soc/codecs/max98357a.c | 42 ++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 9 deletions(-)

Comments

Mark Brown May 3, 2019, 4:29 a.m. UTC | #1

On Tue, Apr 30, 2019 at 04:01:07PM +0800, Tzung-Bi Shih wrote:

> The patch changes devm to non-devm API, because we don't have equivalent
> API for ASoC components (e.g. compm_).

Don't do this, if you're missing a needed API then add it.

Patch

diff --git a/sound/soc/codecs/max98357a.c b/sound/soc/codecs/max98357a.c
index d037a3e4d323..4e90876f6ef6 100644
--- a/sound/soc/codecs/max98357a.c
+++ b/sound/soc/codecs/max98357a.c
@@ -27,24 +27,28 @@ 
 #include <sound/soc-dai.h>
 #include <sound/soc-dapm.h>
 
+struct max98357a_priv {
+	struct gpio_desc *sdmode;
+};
+
 static int max98357a_daiops_trigger(struct snd_pcm_substream *substream,
 		int cmd, struct snd_soc_dai *dai)
 {
-	struct gpio_desc *sdmode = snd_soc_dai_get_drvdata(dai);
+	struct max98357a_priv *max98357a = snd_soc_dai_get_drvdata(dai);
 
-	if (!sdmode)
+	if (!max98357a->sdmode)
 		return 0;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		gpiod_set_value(sdmode, 1);
+		gpiod_set_value(max98357a->sdmode, 1);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		gpiod_set_value(sdmode, 0);
+		gpiod_set_value(max98357a->sdmode, 0);
 		break;
 	}
 
@@ -61,19 +65,31 @@  static const struct snd_soc_dapm_route max98357a_dapm_routes[] = {
 
 static int max98357a_component_probe(struct snd_soc_component *component)
 {
-	struct gpio_desc *sdmode;
+	struct max98357a_priv *max98357a =
+			snd_soc_component_get_drvdata(component);
 
-	sdmode = devm_gpiod_get_optional(component->dev, "sdmode", GPIOD_OUT_LOW);
-	if (IS_ERR(sdmode))
-		return PTR_ERR(sdmode);
+	max98357a->sdmode = gpiod_get_optional(component->dev,
+					"sdmode", GPIOD_OUT_LOW);
+	if (IS_ERR(max98357a->sdmode))
+		return PTR_ERR(max98357a->sdmode);
 
-	snd_soc_component_set_drvdata(component, sdmode);
+	snd_soc_component_set_drvdata(component, max98357a);
 
 	return 0;
 }
 
+static void max98357a_component_remove(struct snd_soc_component *component)
+{
+	struct max98357a_priv *max98357a =
+			snd_soc_component_get_drvdata(component);
+
+	if (max98357a->sdmode)
+		gpiod_put(max98357a->sdmode);
+}
+
 static const struct snd_soc_component_driver max98357a_component_driver = {
 	.probe			= max98357a_component_probe,
+	.remove			= max98357a_component_remove,
 	.dapm_widgets		= max98357a_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(max98357a_dapm_widgets),
 	.dapm_routes		= max98357a_dapm_routes,
@@ -112,6 +128,14 @@  static struct snd_soc_dai_driver max98357a_dai_driver = {
 
 static int max98357a_platform_probe(struct platform_device *pdev)
 {
+	struct max98357a_priv *max98357a;
+
+	max98357a = devm_kzalloc(&pdev->dev, sizeof(*max98357a), GFP_KERNEL);
+	if (!max98357a)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, max98357a);
+
 	return devm_snd_soc_register_component(&pdev->dev,
 			&max98357a_component_driver,
 			&max98357a_dai_driver, 1);