| Message ID | 20180214144459.29813-2-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] dmaengine: qcom: bam_dma: make bam clk optional | expand |
On Wed 14 Feb 06:44 PST 2018, Srinivas Kandagatla wrote: > @@ -1233,13 +1233,14 @@ static int bam_dma_probe(struct platform_device *pdev) > "qcom,controlled-remotely"); > > bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); > - if (IS_ERR(bdev->bamclk)) > - return PTR_ERR(bdev->bamclk); > - > - ret = clk_prepare_enable(bdev->bamclk); > - if (ret) { > - dev_err(bdev->dev, "failed to prepare/enable clock\n"); > - return ret; > + if (IS_ERR(bdev->bamclk)) { In the case of !bdev->controlled_remotely I think this should still be an error. > + bdev->bamclk = NULL; > + } else { > + ret = clk_prepare_enable(bdev->bamclk); > + if (ret) { > + dev_err(bdev->dev, "failed to prepare/enable clock\n"); > + return ret; > + } The rest of the driver will keep operating the bamclk (which is okay), so for symmetry purposes I think you should just keep the clk_prepare_enable() block unmodified. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the Review, On 14/02/18 15:41, Bjorn Andersson wrote: >> >> bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); >> - if (IS_ERR(bdev->bamclk)) >> - return PTR_ERR(bdev->bamclk); >> - >> - ret = clk_prepare_enable(bdev->bamclk); >> - if (ret) { >> - dev_err(bdev->dev, "failed to prepare/enable clock\n"); >> - return ret; >> + if (IS_ERR(bdev->bamclk)) { > In the case of !bdev->controlled_remotely I think this should still be > an error. > Yep makes sense, You mean something like this? ------------------------->cut<------------------------ bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); if (IS_ERR(bdev->bamclk)) { if (!bdev->controlled_remotely); return PTR_ERR(bdev->bamclk); bdev->bamclk = NULL; } ret = clk_prepare_enable(bdev->bamclk); if (ret) { dev_err(bdev->dev, "failed to prepare/enable clock\n"); return ret; } ------------------------->cut<------------------------ >> + bdev->bamclk = NULL; >> + } else { >> + ret = clk_prepare_enable(bdev->bamclk); >> + if (ret) { >> + dev_err(bdev->dev, "failed to prepare/enable clock\n"); >> + return ret; >> + } > The rest of the driver will keep operating the bamclk (which is okay), > so for symmetry purposes I think you should just keep the > clk_prepare_enable() block unmodified. > Yep, with above change, this block should be unchanged. --srini > Regards, > Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 14 Feb 10:19 PST 2018, Srinivas Kandagatla wrote: > Thanks for the Review, > > On 14/02/18 15:41, Bjorn Andersson wrote: > > > bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); > > > - if (IS_ERR(bdev->bamclk)) > > > - return PTR_ERR(bdev->bamclk); > > > - > > > - ret = clk_prepare_enable(bdev->bamclk); > > > - if (ret) { > > > - dev_err(bdev->dev, "failed to prepare/enable clock\n"); > > > - return ret; > > > + if (IS_ERR(bdev->bamclk)) { > > In the case of !bdev->controlled_remotely I think this should still be > > an error. > > > Yep makes sense, > > You mean something like this? > > ------------------------->cut<------------------------ > bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); > if (IS_ERR(bdev->bamclk)) { > if (!bdev->controlled_remotely); > return PTR_ERR(bdev->bamclk); > > bdev->bamclk = NULL; > } > > ret = clk_prepare_enable(bdev->bamclk); > if (ret) { > dev_err(bdev->dev, "failed to prepare/enable clock\n"); > return ret; > } > > ------------------------->cut<------------------------ > Yes Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index d076940e0c69..ae4c7b6930b8 100644 --- a/drivers/dma/qcom/bam_dma.c +++ b/drivers/dma/qcom/bam_dma.c @@ -1233,13 +1233,14 @@ static int bam_dma_probe(struct platform_device *pdev) "qcom,controlled-remotely"); bdev->bamclk = devm_clk_get(bdev->dev, "bam_clk"); - if (IS_ERR(bdev->bamclk)) - return PTR_ERR(bdev->bamclk); - - ret = clk_prepare_enable(bdev->bamclk); - if (ret) { - dev_err(bdev->dev, "failed to prepare/enable clock\n"); - return ret; + if (IS_ERR(bdev->bamclk)) { + bdev->bamclk = NULL; + } else { + ret = clk_prepare_enable(bdev->bamclk); + if (ret) { + dev_err(bdev->dev, "failed to prepare/enable clock\n"); + return ret; + } } ret = bam_init(bdev);