diff mbox

[v2,1/5] dmaengine: qcom: bam_dma: make bam clk optional

Message ID 20180214144459.29813-2-srinivas.kandagatla@linaro.org (mailing list archive)
State Changes Requested
Headers show

Commit Message

Srinivas Kandagatla Feb. 14, 2018, 2:44 p.m. UTC 
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

When BAM is remotely controlled it does not sound correct to control
its clk on Linux side. Make it optional, so that its not mandatory
for remote controlled BAM instances.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/dma/qcom/bam_dma.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Bjorn Andersson Feb. 14, 2018, 3:41 p.m. UTC | #1 
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 dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla Feb. 14, 2018, 6:19 p.m. UTC | #2 
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 dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Feb. 14, 2018, 6:47 p.m. UTC | #3 
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 dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);