Sender: Guenter Roeck <groeck7@gmail.com>
Subject: Re: [PATCH] watchdog: s3c2410: Fix getting the optional clock
To:     Sam Protsenko <semen.protsenko@linaro.org>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        linux-watchdog@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20211212170247.30646-1-semen.protsenko@linaro.org>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <5d21438c-b122-41ce-c6be-771c63b0258a@roeck-us.net>
Date:   Sun, 12 Dec 2021 09:37:30 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20211212170247.30646-1-semen.protsenko@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/12/21 9:02 AM, Sam Protsenko wrote:
> "watchdog_src" clock is optional and may not be present for some SoCs
> supported by this driver. Nevertheless, in case the clock is provided
> but some error happens during its getting, that error should be handled
> properly. Use devm_clk_get_optional() API for that. Also report possible
> errors using dev_err_probe() to handle properly -EPROBE_DEFER error (if
> clock provider is not ready by the time WDT probe function is executed).
> 
> Fixes: a4f3dc8d5fbc ("watchdog: s3c2410: Support separate source clock")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/s3c2410_wdt.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index bb374b9fc163..71c280d3e1a2 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -713,16 +713,18 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>   	 * "watchdog_src" clock is optional; if it's not present -- just skip it
>   	 * and use "watchdog" clock as both bus and source clock.
>   	 */
> -	wdt->src_clk = devm_clk_get(dev, "watchdog_src");
> -	if (!IS_ERR(wdt->src_clk)) {
> -		ret = clk_prepare_enable(wdt->src_clk);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to enable source clock\n");
> -			ret = PTR_ERR(wdt->src_clk);
> -			goto err_bus_clk;
> -		}
> -	} else {
> -		wdt->src_clk = NULL;
> +	wdt->src_clk = devm_clk_get_optional(dev, "watchdog_src");
> +	if (IS_ERR(wdt->src_clk)) {
> +		dev_err_probe(dev, PTR_ERR(wdt->src_clk),
> +			      "failed to get source clock\n");
> +		ret = PTR_ERR(wdt->src_clk);
> +		goto err_bus_clk;
> +	}
> +
> +	ret = clk_prepare_enable(wdt->src_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable source clock\n");
> +		goto err_bus_clk;
>   	}
>   
>   	wdt->wdt_device.min_timeout = 1;
>