diff mbox series

watchdog: s3c2410: Fix getting the optional clock

Message ID 20211212170247.30646-1-semen.protsenko@linaro.org (mailing list archive)
State Accepted
Headers show
Series watchdog: s3c2410: Fix getting the optional clock | expand

Commit Message

Sam Protsenko Dec. 12, 2021, 5:02 p.m. UTC
"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>
---
 drivers/watchdog/s3c2410_wdt.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Guenter Roeck Dec. 12, 2021, 5:37 p.m. UTC | #1
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;
>
Krzysztof Kozlowski Dec. 12, 2021, 5:50 p.m. UTC | #2
On 12/12/2021 18:02, 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>
> ---
>  drivers/watchdog/s3c2410_wdt.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof
Sam Protsenko Dec. 20, 2021, 3:15 p.m. UTC | #3
On Sun, 12 Dec 2021 at 19:50, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 12/12/2021 18:02, 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>
> > ---
> >  drivers/watchdog/s3c2410_wdt.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
>
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>

Hi Guenter,

If there are no outstanding concerns, can you please apply this one?
Would be nice to see it in v5.17 if that's possible.

Thanks!

>
> Best regards,
> Krzysztof
Guenter Roeck Dec. 20, 2021, 9:08 p.m. UTC | #4
On 12/20/21 7:15 AM, Sam Protsenko wrote:
> On Sun, 12 Dec 2021 at 19:50, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 12/12/2021 18:02, 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>
>>> ---
>>>   drivers/watchdog/s3c2410_wdt.c | 22 ++++++++++++----------
>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>
> 
> Hi Guenter,
> 
> If there are no outstanding concerns, can you please apply this one?
> Would be nice to see it in v5.17 if that's possible.
> 

I added the patch to my watchdog-next tree, but Wim handles all pull
requests.

Thanks,
Guenter
Sam Protsenko Dec. 21, 2021, 11:52 a.m. UTC | #5
On Mon, 20 Dec 2021 at 23:08, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 12/20/21 7:15 AM, Sam Protsenko wrote:
> > On Sun, 12 Dec 2021 at 19:50, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 12/12/2021 18:02, 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>
> >>> ---
> >>>   drivers/watchdog/s3c2410_wdt.c | 22 ++++++++++++----------
> >>>   1 file changed, 12 insertions(+), 10 deletions(-)
> >>>
> >>
> >>
> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> >>
> >
> > Hi Guenter,
> >
> > If there are no outstanding concerns, can you please apply this one?
> > Would be nice to see it in v5.17 if that's possible.
> >
>
> I added the patch to my watchdog-next tree, but Wim handles all pull
> requests.
>

Thanks, Guenter! Do I need to take any other actions, or Wim is going
to take patches from your tree? I just checked [1] (master branch),
and I can't see my patches there yet.

[1] git://www.linux-watchdog.org/linux-watchdog-next.git

> Thanks,
> Guenter
>
>
Guenter Roeck Dec. 21, 2021, 3:05 p.m. UTC | #6
On Tue, Dec 21, 2021 at 01:52:32PM +0200, Sam Protsenko wrote:
> On Mon, 20 Dec 2021 at 23:08, Guenter Roeck <linux@roeck-us.net> wrote:
[ ... ]
> >
> > I added the patch to my watchdog-next tree, but Wim handles all pull
> > requests.
> >
> 
> Thanks, Guenter! Do I need to take any other actions, or Wim is going
> to take patches from your tree? I just checked [1] (master branch),
> and I can't see my patches there yet.
> 
> [1] git://www.linux-watchdog.org/linux-watchdog-next.git
> 
Wim usually takes patches either from my tree or from the list,
but he does so shortly before the commit window opens. You don't
need to do anything else.

Guenter
diff mbox series

Patch

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;