| Message ID | 1491852912-18712-2-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ASoC: Improve hi6210-i2s DT bindings | expand |
On Mon, Apr 10, 2017 at 12:35:12PM -0700, John Stultz wrote: > This patch adds a few extra error returns in cases that > shouldn't happen, some style nits adding breaks to final > default cases in switch statements, and tweaks to use > devm_ variant of snd_soc_register_component. Please don't make multiple unrelated changes in a single patch, it's bad practice which can slow down the bits of the patch which are OK and makes it harder to review if the changes match up with the changelog. > static int hi6210_i2s_remove(struct platform_device *pdev) > { > snd_soc_unregister_component(&pdev->dev); Since you converted to devm_ this is now going to be a double free.
On Tue, Apr 11, 2017 at 11:34 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Apr 10, 2017 at 12:35:12PM -0700, John Stultz wrote: > >> This patch adds a few extra error returns in cases that >> shouldn't happen, some style nits adding breaks to final >> default cases in switch statements, and tweaks to use >> devm_ variant of snd_soc_register_component. > > Please don't make multiple unrelated changes in a single patch, it's bad > practice which can slow down the bits of the patch which are OK and > makes it harder to review if the changes match up with the changelog. Apologies, I'll split it up and resend. >> static int hi6210_i2s_remove(struct platform_device *pdev) >> { >> snd_soc_unregister_component(&pdev->dev); > > Since you converted to devm_ this is now going to be a double free. Ah. So it looks like can yank the whole remove implementation. thanks -john
diff --git a/sound/soc/hisilicon/hi6210-i2s.c b/sound/soc/hisilicon/hi6210-i2s.c index 45691b70..bdbf982 100644 --- a/sound/soc/hisilicon/hi6210-i2s.c +++ b/sound/soc/hisilicon/hi6210-i2s.c @@ -324,6 +324,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream, default: i2s->bits = 16; dma_data->addr_width = 2; + break; } i2s->rate = params_rate(params); i2s->channels = params_channels(params); @@ -395,6 +396,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream, break; default: WARN_ONCE(1, "Invalid i2s->fmt MASTER_MASK. This shouldn't happen\n"); + return -EINVAL; } switch (i2s->format & SND_SOC_DAIFMT_FORMAT_MASK) { @@ -409,6 +411,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream, break; default: WARN_ONCE(1, "Invalid i2s->fmt FORMAT_MASK. This shouldn't happen\n"); + return -EINVAL; } val = hi6210_read_reg(i2s, HII2S_I2S_CFG); @@ -440,6 +443,7 @@ static int hi6210_i2s_hw_params(struct snd_pcm_substream *substream, val = hi6210_read_reg(i2s, HII2S_I2S_CFG); val &= ~HII2S_I2S_CFG__S2_FRAME_MODE; hi6210_write_reg(i2s, HII2S_I2S_CFG, val); + break; } /* clear loopback, set signed type and word length */ @@ -587,20 +591,14 @@ static int hi6210_i2s_probe(struct platform_device *pdev) if (ret) return ret; - ret = snd_soc_register_component(&pdev->dev, &hi6210_i2s_i2s_comp, + ret = devm_snd_soc_register_component(&pdev->dev, &hi6210_i2s_i2s_comp, &i2s->dai, 1); - if (ret) { - dev_err(&pdev->dev, "Failed to register dai\n"); - return ret; - } - - return 0; + return ret; } static int hi6210_i2s_remove(struct platform_device *pdev) { snd_soc_unregister_component(&pdev->dev); - dev_set_drvdata(&pdev->dev, NULL); return 0; }
Mark had a few minor fixups he requested to the hi6210 i2s driver, so this patch proves them. This patch adds a few extra error returns in cases that shouldn't happen, some style nits adding breaks to final default cases in switch statements, and tweaks to use devm_ variant of snd_soc_register_component. Cc: Zhangfei Gao <zhangfei.gao@linaro.org> Cc: Liam Girdwood <lgirdwood@gmail.com> Cc: Mark Brown <broonie@kernel.org> Cc: Jaroslav Kysela <perex@perex.cz> Cc: Wei Xu <xuwei5@hisilicon.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Andy Green <andy@warmcat.com> Cc: Dave Long <dave.long@linaro.org> Cc: Guodong Xu <guodong.xu@linaro.org> Signed-off-by: John Stultz <john.stultz@linaro.org> --- sound/soc/hisilicon/hi6210-i2s.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) -- 2.7.4