Message ID | 20200622154241.29053-4-pierre-louis.bossart@linux.intel.com (mailing list archive) |
---|---|
State | Accepted |
Commit | e56054e75325c347f09c1be2f6400ef67bb9662d |
Headers | show |
Series | ASoC: add dailink .exit() callback | expand |
On Mon, Jun 22, 2020 at 10:42:39AM -0500, Pierre-Louis Bossart wrote: > The gpiod handling is inspired from the bdw-rt5677 code. Apply same > fix to avoid reference count issue while removing modules for > consistency. ... > - ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute", > - GPIOD_OUT_HIGH); > + ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute", > + GPIOD_OUT_HIGH); > if (IS_ERR(ctx->gpio_lo_mute)) { > dev_err(component->dev, "Can't find GPIO_MUTE# gpio\n"); > return PTR_ERR(ctx->gpio_lo_mute); Is it fatal? Then IS_ERR() is not needed below. For NULL I already told. > + /* > + * The .exit() can be reached without going through the .init() > + * so explicitly test if the gpiod is valid > + */ This comment should be amended after fixing the code. > + if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute)) > + gpiod_put(ctx->gpio_lo_mute); > +}
On 6/22/20 1:20 PM, Andy Shevchenko wrote: > On Mon, Jun 22, 2020 at 10:42:39AM -0500, Pierre-Louis Bossart wrote: >> The gpiod handling is inspired from the bdw-rt5677 code. Apply same >> fix to avoid reference count issue while removing modules for >> consistency. > > ... > >> - ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute", >> - GPIOD_OUT_HIGH); >> + ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute", >> + GPIOD_OUT_HIGH); >> if (IS_ERR(ctx->gpio_lo_mute)) { >> dev_err(component->dev, "Can't find GPIO_MUTE# gpio\n"); >> return PTR_ERR(ctx->gpio_lo_mute); > > Is it fatal? Then IS_ERR() is not needed below. For NULL I already told. this patch only fixes a deadlock, whether or not this is fatal or not is a different question. I would assert if if you can't mute your audio is broken. >> + /* >> + * The .exit() can be reached without going through the .init() >> + * so explicitly test if the gpiod is valid >> + */ > > This comment should be amended after fixing the code. > >> + if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute)) >> + gpiod_put(ctx->gpio_lo_mute); >> +} >
On Mon, Jun 22, 2020 at 01:26:15PM -0500, Pierre-Louis Bossart wrote: > On 6/22/20 1:20 PM, Andy Shevchenko wrote: > > On Mon, Jun 22, 2020 at 10:42:39AM -0500, Pierre-Louis Bossart wrote: > > > The gpiod handling is inspired from the bdw-rt5677 code. Apply same > > > fix to avoid reference count issue while removing modules for > > > consistency. > > > > ... > > > > > - ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute", > > > - GPIOD_OUT_HIGH); > > > + ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute", > > > + GPIOD_OUT_HIGH); > > > if (IS_ERR(ctx->gpio_lo_mute)) { > > > dev_err(component->dev, "Can't find GPIO_MUTE# gpio\n"); > > > return PTR_ERR(ctx->gpio_lo_mute); > > > > Is it fatal? Then IS_ERR() is not needed below. For NULL I already told. > > this patch only fixes a deadlock, whether or not this is fatal or not is a > different question. I would assert if if you can't mute your audio is > broken. Ah, sorry, I mean IS_ERR() is not needed for an optional case. Since it is fatal, it's fine. > > > + /* > > > + * The .exit() can be reached without going through the .init() > > > + * so explicitly test if the gpiod is valid > > > + */ > > > > This comment should be amended after fixing the code. > > > > > + if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute)) > > > + gpiod_put(ctx->gpio_lo_mute); > > > +} > >
diff --git a/sound/soc/intel/boards/kbl_rt5660.c b/sound/soc/intel/boards/kbl_rt5660.c index d2a078454784..f4c0b983c990 100644 --- a/sound/soc/intel/boards/kbl_rt5660.c +++ b/sound/soc/intel/boards/kbl_rt5660.c @@ -165,8 +165,8 @@ static int kabylake_rt5660_codec_init(struct snd_soc_pcm_runtime *rtd) dev_warn(component->dev, "Failed to add driver gpios\n"); /* Request rt5660 GPIO for lineout mute control, return if fails */ - ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute", - GPIOD_OUT_HIGH); + ctx->gpio_lo_mute = gpiod_get(component->dev, "lineout-mute", + GPIOD_OUT_HIGH); if (IS_ERR(ctx->gpio_lo_mute)) { dev_err(component->dev, "Can't find GPIO_MUTE# gpio\n"); return PTR_ERR(ctx->gpio_lo_mute); @@ -207,6 +207,18 @@ static int kabylake_rt5660_codec_init(struct snd_soc_pcm_runtime *rtd) return 0; } +static void kabylake_rt5660_codec_exit(struct snd_soc_pcm_runtime *rtd) +{ + struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(rtd->card); + + /* + * The .exit() can be reached without going through the .init() + * so explicitly test if the gpiod is valid + */ + if (!IS_ERR_OR_NULL(ctx->gpio_lo_mute)) + gpiod_put(ctx->gpio_lo_mute); +} + static int kabylake_hdmi_init(struct snd_soc_pcm_runtime *rtd, int device) { struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(rtd->card); @@ -421,6 +433,7 @@ static struct snd_soc_dai_link kabylake_rt5660_dais[] = { .id = 0, .no_pcm = 1, .init = kabylake_rt5660_codec_init, + .exit = kabylake_rt5660_codec_exit, .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS,