Re: [PATCHv3] watchdog: xilinx: Add clock support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi ,
Thanks for the review.


On Thu, Aug 11, 2016 at 7:57 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 08/11/2016 06:20 AM, Shubhrajyoti Datta wrote:
>>
>> Add support for the clock. Currently we enable
>> at probe and relinquish at remove.
>>
>> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>> Acked-by: Sören Brinkmann <soren.brinkmann@xxxxxxxxxx>
>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxxxxx>
>> ---
>> v2:
>> fix spaces and error path.
>> v3:
>> do not fail existing dts
>>
>>  .../devicetree/bindings/watchdog/of-xilinx-wdt.txt |  3 +++
>>  drivers/watchdog/of_xilinx_wdt.c                   | 29
>> ++++++++++++++++++++--
>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> index 6d63782..c6ae9c9 100644
>> --- a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>> @@ -7,6 +7,8 @@ Required properties:
>>  - reg                  : Physical base address and size
>>
>>  Optional properties:
>> +- clocks               : Input clock specifier. Refer to common clock
>> +                         bindings.
>>  - clock-frequency      : Frequency of clock in Hz
>>  - xlnx,wdt-enable-once : 0 - Watchdog can be restarted
>>                           1 - Watchdog can be enabled just once
>> @@ -17,6 +19,7 @@ Example:
>>  axi-timebase-wdt@40100000 {
>>         clock-frequency = <50000000>;
>>         compatible = "xlnx,xps-timebase-wdt-1.00.a";
>> +       clocks = <&clkc 15>;
>>         reg = <0x40100000 0x10000>;
>>         xlnx,wdt-enable-once = <0x0>;
>>         xlnx,wdt-interval = <0x1b>;
>> diff --git a/drivers/watchdog/of_xilinx_wdt.c
>> b/drivers/watchdog/of_xilinx_wdt.c
>> index b2e1b4c..120840a 100644
>> --- a/drivers/watchdog/of_xilinx_wdt.c
>> +++ b/drivers/watchdog/of_xilinx_wdt.c
>> @@ -10,6 +10,7 @@
>>   * 2 of the License, or (at your option) any later version.
>>   */
>>
>> +#include <linux/clk.h>
>>  #include <linux/err.h>
>>  #include <linux/module.h>
>>  #include <linux/types.h>
>> @@ -45,6 +46,7 @@ struct xwdt_device {
>>         u32 wdt_interval;
>>         spinlock_t spinlock;
>>         struct watchdog_device xilinx_wdt_wdd;
>> +       struct clk              *clk;
>>  };
>>
>>  static int xilinx_wdt_start(struct watchdog_device *wdd)
>> @@ -195,16 +197,34 @@ static int xwdt_probe(struct platform_device *pdev)
>>         spin_lock_init(&xdev->spinlock);
>>         watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
>>
>> +       xdev->clk = devm_clk_get(&pdev->dev, NULL);
>> +       if (IS_ERR(xdev->clk)) {
>> +               if (PTR_ERR(xdev->clk) == -ENOENT) {
>> +                       dev_err(&pdev->dev, "input clock not found\n");
>
>
> No, this is not an error. The clock is optional.
yes will make it info instead.

>
>> +                       xdev->clk = NULL;
>> +               } else {
>> +                       rc = PTR_ERR(xdev->clk);
>> +                       dev_err(&pdev->dev, "axi clock error %d\n", rc);
>
>
> This could be -EPROBE_DEFER, which would not warrant an error message.
sure also since the error code will be given to the core I am removing
the print from the driver.

>
>
>> +                       return rc;
>> +               }
>> +       }
>> +
>> +       rc = clk_prepare_enable(xdev->clk);
>> +       if (rc) {
>> +               dev_err(&pdev->dev, "unable to enable clock\n");
>> +               return rc;
>> +       }
>> +
>>         rc = xwdt_selftest(xdev);
>>         if (rc == XWT_TIMER_FAILED) {
>>                 dev_err(&pdev->dev, "SelfTest routine error\n");
>> -               return rc;
>> +               goto err_clk_disable;
>>         }
>>
>>         rc = watchdog_register_device(xilinx_wdt_wdd);
>>         if (rc) {
>>                 dev_err(&pdev->dev, "Cannot register watchdog (err=%d)\n",
>> rc);
>> -               return rc;
>> +               goto err_clk_disable;
>>         }
>>
>>         dev_info(&pdev->dev, "Xilinx Watchdog Timer at %p with timeout
>> %ds\n",
>> @@ -213,6 +233,10 @@ static int xwdt_probe(struct platform_device *pdev)
>>         platform_set_drvdata(pdev, xdev);
>>
>>         return 0;
>> +err_clk_disable:
>> +       clk_disable_unprepare(xdev->clk);
>> +
>> +       return rc;
>>  }
>>
>>  static int xwdt_remove(struct platform_device *pdev)
>> @@ -220,6 +244,7 @@ static int xwdt_remove(struct platform_device *pdev)
>>         struct xwdt_device *xdev = platform_get_drvdata(pdev);
>>
>>         watchdog_unregister_device(&xdev->xilinx_wdt_wdd);
>> +       clk_disable_unprepare(xdev->clk);
>>
>>         return 0;
>>  }
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux