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.