| Message ID | 42f2a995-3545-40bf-82a0-fd9b8abc16ce@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] SoC: mediatek: mt8188-mt6359: clean up a return in codec_init | expand |
On Wed, 2023-06-07 at 17:57 +0300, Dan Carpenter wrote: > This code triggers a Smatch static checker warning and does sort of > look like an error path. > > sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 > mt8188_max98390_codec_init() warn: missing error code? 'ret' > > However, returning 0 is intentional. Make that explicit. Hi Dan, There are several similar instances in this file. If using 'return ret;' is not recommended, should we update all of them simultaneously? For instance, the value of 'ret' at the end of this function should also be zero. Thanks, Trevor > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: At first I thought this was an error path but it is a not. > > sound/soc/mediatek/mt8188/mt8188-mt6359.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c > index bc4b74970a46..5ad8d2a5bbc4 100644 > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c > @@ -594,7 +594,7 @@ static int mt8188_max98390_codec_init(struct > snd_soc_pcm_runtime *rtd) > } > > if (rtd->dai_link->num_codecs <= 2) > -return ret; > +return 0; > > /* add widgets/controls/dapm for rear speakers */ > ret = snd_soc_dapm_new_controls(&card->dapm, > mt8188_rear_spk_widgets, > -- > 2.39.2 >
Il 08/06/23 06:02, Trevor Wu (吳文良) ha scritto: > On Wed, 2023-06-07 at 17:57 +0300, Dan Carpenter wrote: >> This code triggers a Smatch static checker warning and does sort of >> look like an error path. >> >> sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 >> mt8188_max98390_codec_init() warn: missing error code? 'ret' >> >> However, returning 0 is intentional. Make that explicit. > > Hi Dan, > > There are several similar instances in this file. If using 'return > ret;' is not recommended, should we update all of them simultaneously? > > For instance, the value of 'ret' at the end of this function should > also be zero. > I'm already in the process of performing a further cleanup to this file, will include Dan's patch in my series. Please hold on (sending today). Regards, Angelo > Thanks, > Trevor > >> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> >> --- >> v2: At first I thought this was an error path but it is a not. >> >> sound/soc/mediatek/mt8188/mt8188-mt6359.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c >> b/sound/soc/mediatek/mt8188/mt8188-mt6359.c >> index bc4b74970a46..5ad8d2a5bbc4 100644 >> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c >> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c >> @@ -594,7 +594,7 @@ static int mt8188_max98390_codec_init(struct >> snd_soc_pcm_runtime *rtd) >> } >> >> if (rtd->dai_link->num_codecs <= 2) >> -return ret; >> +return 0; >> >> /* add widgets/controls/dapm for rear speakers */ >> ret = snd_soc_dapm_new_controls(&card->dapm, >> mt8188_rear_spk_widgets, >> -- >> 2.39.2 >>
On Thu, Jun 08, 2023 at 04:02:07AM +0000, Trevor Wu (吳文良) wrote: > On Wed, 2023-06-07 at 17:57 +0300, Dan Carpenter wrote: > > This code triggers a Smatch static checker warning and does sort of > > look like an error path. > > > > sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 > > mt8188_max98390_codec_init() warn: missing error code? 'ret' > > > > However, returning 0 is intentional. Make that explicit. > > Hi Dan, > > There are several similar instances in this file. If using 'return > ret;' is not recommended, should we update all of them simultaneously? > > For instance, the value of 'ret' at the end of this function should > also be zero. > I would prefer that, yes. "return 0;" is always more clear than "return ret;" I didn't change the "return ret;" at the end of the functions because they were obviously intentional. I'm doing static analysis and I have an unpublished check which complains about the return ret at the end of the function but I'm not going to publish it because that doesn't find bugs and I'm only focused on bugs. When we have code like: ret = frob(); if (ret) return ret; if (x == 1) return ret; More often than not, that is a bug. But if it's code like: ret = frob(); if (ret) return ret; frob(); frob(); frob(); return ret; Then that's probably not a bug. regards, dan carpenter
diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c index bc4b74970a46..5ad8d2a5bbc4 100644 --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c @@ -594,7 +594,7 @@ static int mt8188_max98390_codec_init(struct snd_soc_pcm_runtime *rtd) } if (rtd->dai_link->num_codecs <= 2) - return ret; + return 0; /* add widgets/controls/dapm for rear speakers */ ret = snd_soc_dapm_new_controls(&card->dapm, mt8188_rear_spk_widgets,
This code triggers a Smatch static checker warning and does sort of look like an error path. sound/soc/mediatek/mt8188/mt8188-mt6359.c:597 mt8188_max98390_codec_init() warn: missing error code? 'ret' However, returning 0 is intentional. Make that explicit. Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: At first I thought this was an error path but it is a not. sound/soc/mediatek/mt8188/mt8188-mt6359.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)