| Message ID | 20221128221152.2.I8d1993f41f0da1eac0ecba321678ac489f9c0b9b@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ASoC: qcom: lpass-sc7180: Drop unnecessary initialization in _resume() | expand |
Hi, On Mon, Nov 28, 2022 at 2:12 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > sc7180_lpass_dev_resume() returns 'ret' at the end of the function, > where 'ret' is always 0. Just return 0 to make it plain obvious that > this is always the success path. > > Also add an empty line between the error handling path and the > return. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > sound/soc/qcom/lpass-sc7180.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c > index 7a81e609727c..30a28e3152cb 100644 > --- a/sound/soc/qcom/lpass-sc7180.c > +++ b/sound/soc/qcom/lpass-sc7180.c > @@ -173,7 +173,8 @@ static int sc7180_lpass_dev_resume(struct device *dev) > dev_err(dev, "sc7180 clk prepare and enable failed\n"); > return ret; > } > - return ret; > + > + return 0; Yeah, I noticed this too when I was approving your pick. FWIW, I probably would have written this way, but it's 6 of one and half dozen of the other: if (ret) dev_err(dev, ...); return ret; ...but I just dug a tiny bit deeper and actually, there's no need for the error print here and it's just wasteful. clk_bulk_prepare_enable() already prints errors for you. So really this whole function could just be: struct lpass_data *drvdata = dev_get_drvdata(dev); return clk_bulk_prepare_enable(drvdata->num_clks, drvdata->clks); I guess theoretically one could even go further and look at pm_clk, but perhaps that's overboard. -Doug
On Mon, Nov 28, 2022 at 02:48:28PM -0800, Doug Anderson wrote: > Hi, > > On Mon, Nov 28, 2022 at 2:12 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > sc7180_lpass_dev_resume() returns 'ret' at the end of the function, > > where 'ret' is always 0. Just return 0 to make it plain obvious that > > this is always the success path. > > > > Also add an empty line between the error handling path and the > > return. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > sound/soc/qcom/lpass-sc7180.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c > > index 7a81e609727c..30a28e3152cb 100644 > > --- a/sound/soc/qcom/lpass-sc7180.c > > +++ b/sound/soc/qcom/lpass-sc7180.c > > @@ -173,7 +173,8 @@ static int sc7180_lpass_dev_resume(struct device *dev) > > dev_err(dev, "sc7180 clk prepare and enable failed\n"); > > return ret; > > } > > - return ret; > > + > > + return 0; > > Yeah, I noticed this too when I was approving your pick. FWIW, I > probably would have written this way, but it's 6 of one and half dozen > of the other: > > if (ret) > dev_err(dev, ...); > return ret; Yeah, I was considering this too, either is fine IMO :) > ...but I just dug a tiny bit deeper and actually, there's no need for > the error print here and it's just wasteful. clk_bulk_prepare_enable() > already prints errors for you. So really this whole function could > just be: > > struct lpass_data *drvdata = dev_get_drvdata(dev); > return clk_bulk_prepare_enable(drvdata->num_clks, drvdata->clks); Right, the log isn't really needed if clk_bulk_prepare_enable() already logs errors. I'll adjust the patch accordingly, and drop the first one since 'ret' is going away. > I guess theoretically one could even go further and look at pm_clk, > but perhaps that's overboard. Maybe let's leave that for another iteration :) Thanks for the review! m.
diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c index 7a81e609727c..30a28e3152cb 100644 --- a/sound/soc/qcom/lpass-sc7180.c +++ b/sound/soc/qcom/lpass-sc7180.c @@ -173,7 +173,8 @@ static int sc7180_lpass_dev_resume(struct device *dev) dev_err(dev, "sc7180 clk prepare and enable failed\n"); return ret; } - return ret; + + return 0; } static int sc7180_lpass_dev_suspend(struct device *dev)
sc7180_lpass_dev_resume() returns 'ret' at the end of the function, where 'ret' is always 0. Just return 0 to make it plain obvious that this is always the success path. Also add an empty line between the error handling path and the return. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- sound/soc/qcom/lpass-sc7180.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)