Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658560 > unrolled thread

[PATCH] spi: pxa2xx: Handle return value of clk_prepare_enable

Back to article view | Back to linux.kernel

Contents

  [PATCH] spi: pxa2xx: Handle return value of clk_prepare_enable Arvind Yadav <arvind.yadav.cs@gmail.com> - 2017-06-06 11:50 +0200
    Re: [PATCH] spi: pxa2xx: Handle return value of clk_prepare_enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-06 13:00 +0200
      Re: [PATCH] spi: pxa2xx: Handle return value of clk_prepare_enable Arvind Yadav <arvind.yadav.cs@gmail.com> - 2017-06-06 13:40 +0200

#1658560 — [PATCH] spi: pxa2xx: Handle return value of clk_prepare_enable

clk_prepare_enable() can fail here and we must check its return value.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 drivers/spi/spi-pxa2xx.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 47b65d7..e9c0fe5 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1676,7 +1676,11 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
 	}
 
 	/* Enable SOC clock */
-	clk_prepare_enable(ssp->clk);
+	status = clk_prepare_enable(ssp->clk);
+	if (status) {
+		dev_err(&pdev->dev, "Failed to prepare clock\n");
+		goto out_error_master_alloc;
+	}
 
 	master->max_speed_hz = clk_get_rate(ssp->clk);
 
@@ -1855,8 +1859,13 @@ static int pxa2xx_spi_resume(struct device *dev)
 	int status;
 
 	/* Enable the SSP clock */
-	if (!pm_runtime_suspended(dev))
-		clk_prepare_enable(ssp->clk);
+	if (!pm_runtime_suspended(dev)) {
+		status = clk_prepare_enable(ssp->clk);
+		if (status) {
+			dev_err(dev, "Failed to prepare clock\n");
+			return status;
+		}
+	}
 
 	/* Restore LPSS private register bits */
 	if (is_lpss_ssp(drv_data))
@@ -1886,8 +1895,7 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
 {
 	struct driver_data *drv_data = dev_get_drvdata(dev);
 
-	clk_prepare_enable(drv_data->ssp->clk);
-	return 0;
+	return clk_prepare_enable(drv_data->ssp->clk);
 }
 #endif
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1658615

On Tue, Jun 6, 2017 at 12:44 PM, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
> clk_prepare_enable() can fail here and we must check its return value.

> @@ -1676,7 +1676,11 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)

> -       clk_prepare_enable(ssp->clk);
> +       status = clk_prepare_enable(ssp->clk);

This one looks fine.

> @@ -1855,8 +1859,13 @@ static int pxa2xx_spi_resume(struct device *dev)

>         /* Enable the SSP clock */
> -       if (!pm_runtime_suspended(dev))
> -               clk_prepare_enable(ssp->clk);
> +       if (!pm_runtime_suspended(dev)) {
> +               status = clk_prepare_enable(ssp->clk);
> +               if (status) {
> +                       dev_err(dev, "Failed to prepare clock\n");
> +                       return status;
> +               }

This...

> @@ -1886,8 +1895,7 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
>  {
>         struct driver_data *drv_data = dev_get_drvdata(dev);
>
> -       clk_prepare_enable(drv_data->ssp->clk);
> -       return 0;
> +       return clk_prepare_enable(drv_data->ssp->clk);

...and especially this should be carefully checked since there are
differences in behaviour how system or driver will be resumed.

So, the question is how did you test it?

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1658664

On Tuesday 06 June 2017 04:24 PM, Andy Shevchenko wrote:
> On Tue, Jun 6, 2017 at 12:44 PM, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:
>> clk_prepare_enable() can fail here and we must check its return value.
>> @@ -1676,7 +1676,11 @@ static int pxa2xx_spi_probe(struct platform_device *pdev)
>> -       clk_prepare_enable(ssp->clk);
>> +       status = clk_prepare_enable(ssp->clk);
> This one looks fine.
>
>> @@ -1855,8 +1859,13 @@ static int pxa2xx_spi_resume(struct device *dev)
>>          /* Enable the SSP clock */
>> -       if (!pm_runtime_suspended(dev))
>> -               clk_prepare_enable(ssp->clk);
>> +       if (!pm_runtime_suspended(dev)) {
>> +               status = clk_prepare_enable(ssp->clk);
>> +               if (status) {
>> +                       dev_err(dev, "Failed to prepare clock\n");
>> +                       return status;
>> +               }
> This...
>
>> @@ -1886,8 +1895,7 @@ static int pxa2xx_spi_runtime_resume(struct device *dev)
>>   {
>>          struct driver_data *drv_data = dev_get_drvdata(dev);
>>
>> -       clk_prepare_enable(drv_data->ssp->clk);
>> -       return 0;
>> +       return clk_prepare_enable(drv_data->ssp->clk);
> ...and especially this should be carefully checked since there are
> differences in behaviour how system or driver will be resumed.
yes true, here clk_prepare_enable will return 0 on successful attempt.
what do you suggest here, we should not return like this.?
>
> So, the question is how did you test it?
It can fail, I am not able to produce clock failure issue. If you have 
any suggestion.
please let me know.
>
-arvind

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web