Re: [alsa-devel] [PATCH 1/2] ASoC: qcom: change PTR_ERR argument
On Sun, Aug 30, 2015 at 12:54:15PM -0700, Julia Lawall wrote: > On Sun, 30 Aug 2015, walter harms wrote: > > Am 30.08.2015 20:05, schrieb Julia Lawall: > > > if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { > > > dev_err(&pdev->dev, > > > "%s() error getting mi2s-bit-clk: %ld\n", > > > - __func__, > PTR_ERR(drvdata->mi2s_bit_clk[i])); > > > + __func__, > > > + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); > > > return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); > > > } > > > } > > > > > > > just a note: > > using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould > help to make the code > > more readable (yes, the other code is alike). something like: > > > > struct clk *tmp = devm_clk_get(&pdev->dev,clk_name); > > Where do you suggest to put this? > > Maybe it would be reasonable to declare a variable struct clk *clk; at the > > top of the function, and then use that as a temporary variable for all > three calls. > > However, now I see that the first call, unlike the other two doesn't cause > > a return from the function. > > if (IS_ERR(drvdata->mi2s_osr_clk[dai_id])) { > dev_warn(&pdev->dev, > "%s() error getting mi2s-osr-clk: %ld\n", > __func__, > PTR_ERR(drvdata->mi2s_osr_clk[dai_id])); > } > > Is that intentional? Yes, that was intentional as the presense of the OSR clock in the DT node is optional. -- Kenneth Westfield Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH 1/2] ASoC: qcom: change PTR_ERR argument
On Sun, Aug 30, 2015 at 11:54:33AM -0700, walter harms wrote: > Am 30.08.2015 20:05, schrieb Julia Lawall: > > if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) { > > dev_err(&pdev->dev, > > "%s() error getting mi2s-bit-clk: %ld\n", > > - __func__, > PTR_ERR(drvdata->mi2s_bit_clk[i])); > > + __func__, > > + PTR_ERR(drvdata->mi2s_bit_clk[dai_id])); > > return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]); > > } > > } > > > > just a note: > using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help > to make the code > more readable (yes, the other code is alike). something like: > > struct clk *tmp = devm_clk_get(&pdev->dev,clk_name); > > if (IS_ERR(tmp)) { > dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk: > %ld\n",__func__, PTR_ERR(tmp)); > return PTR_ERR(tmp); > } > drvdata->mi2s_bit_clk[dai_id]=tmp; Yes, it would make the code more readable. > > > just one minor: > the dev_warn() just before says: " error getting mi2s-osr-clk" may be it > should be "warnig ..." > That will make it more easy to rep for real error in a log. "error [gs]etting" could be re-phrased to "could not [gs]et". The term "error" was not meant to indicate the log level, but evidently, can cause some confusion for someone reading the logs. -- Kenneth Westfield Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH 1/2] ASoC: qcom: change PTR_ERR argument
On Sun, Aug 30, 2015 at 11:05:10AM -0700, Julia Lawall wrote: > Apply PTR_ERR to the value that was recently assigned. > > The semantic match that finds this problem is as follows: > (http://coccinelle.lip6.fr/) > > // > @@ > expression x,y; > @@ > > if (IS_ERR(x) || ...) { > ... when any > when != IS_ERR(...) > ( >PTR_ERR(x) > | > * PTR_ERR(y) > ) > ... when any > } > // > > Signed-off-by: Julia Lawall > > --- The patch itself looks good. Thanks. Acked-by: Kenneth Westfield -- Kenneth Westfield Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/