Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751887AbdFFLeB (ORCPT ); Tue, 6 Jun 2017 07:34:01 -0400 Received: from mail-pg0-f68.google.com ([74.125.83.68]:36802 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751744AbdFFLdq (ORCPT ); Tue, 6 Jun 2017 07:33:46 -0400 Subject: Re: [PATCH] spi: pxa2xx: Handle return value of clk_prepare_enable To: Andy Shevchenko References: <4e16c0aba047b068aaa41c1d180d98ccb441afd0.1496668108.git.arvind.yadav.cs@gmail.com> Cc: Daniel Mack , Haojian Zhuang , Robert Jarzmik , Mark Brown , Geert Uytterhoeven , linux-arm Mailing List , linux-spi , "linux-kernel@vger.kernel.org" From: Arvind Yadav Message-ID: Date: Tue, 6 Jun 2017 17:03:29 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1617 Lines: 40 On Tuesday 06 June 2017 04:24 PM, Andy Shevchenko wrote: > On Tue, Jun 6, 2017 at 12:44 PM, Arvind Yadav 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