| Message ID | 20190507051140.240245-1-tzungbi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: max98357a: release GPIO when component removing | expand |
On Tue, May 07, 2019 at 01:11:40PM +0800, Tzung-Bi Shih wrote: > +static void max98357a_component_remove(struct snd_soc_component *component) > +{ > + struct max98357a_priv *max98357a = > + snd_soc_component_get_drvdata(component); > + > + if (max98357a->sdmode) > + devm_gpiod_put(component->dev, max98357a->sdmode); > +} This is an obvious mess, if you're explicitly freeing devm_ allocated resources in the common case something is going wrong. Just move the initial allocation to the device level probe so devm can do what it's supposed to.
> On Tue, May 07, 2019 at 01:11:40PM +0800, Tzung-Bi Shih wrote: > > > +static void max98357a_component_remove(struct snd_soc_component *component) > > +{ > > + struct max98357a_priv *max98357a = > > + snd_soc_component_get_drvdata(component); > > + > > + if (max98357a->sdmode) > > + devm_gpiod_put(component->dev, max98357a->sdmode); > > +} > > This is an obvious mess, if you're explicitly freeing devm_ allocated > resources in the common case something is going wrong. Just move the > initial allocation to the device level probe so devm can do what it's > supposed to. Move the GPIO allocation to max98357a_platform_probe() should work but I am wondering the difference between device's probe() and component's probe(). What do we expect to do in component's probe()? As component's probe() is later than device's, I thought we tend to put resource allocation in component's probe() for reasons: - to speed up the booting *maybe* a little - to allocate resources when really need them I am using devm_gpiod_put() instead of gpiod_put() so that I suppose devm_ should take care of the rest of cleanup. Do you think this is still a mess?
On Wed, May 08, 2019 at 05:27:35PM +0800, Tzung-Bi Shih wrote: > probe(). What do we expect to do in component's probe()? Only things that really, really need the card. > As component's probe() is later than device's, I thought we tend to > put resource allocation in component's probe() for reasons: > - to speed up the booting *maybe* a little > - to allocate resources when really need them No, this is backwards - there's no point in running through the ASoC level initialization only to find out we don't have some critical resource. > I am using devm_gpiod_put() instead of gpiod_put() so that I suppose > devm_ should take care of the rest of cleanup. Do you think this is > still a mess? The entire point of devm_ is that it does all the cleanup for you.
diff --git a/sound/soc/codecs/max98357a.c b/sound/soc/codecs/max98357a.c index d037a3e4d323..b11c6cb81f55 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 = devm_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) + devm_gpiod_put(component->dev, 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);
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. Release the GPIO proactively when component is removing. Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- sound/soc/codecs/max98357a.c | 42 ++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 9 deletions(-)