Re: [PATCH] watchdog: sp805: Add clock-frequency property

From: Srinath Mannam
Date: Thu Jul 26 2018 - 01:35:05 EST


Hi Guenter,

Thank you.. I sent next version patch with modification.

Regards,
Srinath.

On Thu, Jul 26, 2018 at 10:39 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 07/25/2018 09:47 PM, Srinath Mannam wrote:
>>
>> Hi Guenter,
>>
>> It was my mistake sorry for that. I thought to add as Rivewed-by your
>> name..
>> Many changes are made based on your review comments and suggestions.
>> So, Can I add your name in Reviewed list?
>
>
> You did that already below, and I am ok with that.
>
> Guenter
>
>> I will modify and send next patchset.
>>
>> thank you.
>>
>> Regards,
>> Srinath.
>>
>> On Thu, Jul 26, 2018 at 10:12 AM, Guenter Roeck <linux@xxxxxxxxxxxx>
>> wrote:
>>>
>>> On 07/23/2018 01:37 AM, Srinath Mannam wrote:
>>>>
>>>>
>>>> Use clock-frequency property given in _DSD object
>>>> of ACPI device to calculate Watchdog rate as binding
>>>> clock devices are not available as device tree.
>>>>
>>>> Note: There is no formal review process for _DSD
>>>> properties
>>>>
>>>> Signed-off-by: Srinath Mannam <srinath.mannam@xxxxxxxxxxxx>
>>>> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>>>
>>>
>>>
>>> I just noticed this. This is completely inappropriate. It is
>>> the equivalent of forging a signature on a check.
>>>
>>> _Never_ add a Signed-off-by: statement for anyone but yourself.
>>>
>>> Guenter
>>>
>>>> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>>>> Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
>>>> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>>>> ---
>>>> drivers/watchdog/sp805_wdt.c | 35 +++++++++++++++++++++++++----------
>>>> 1 file changed, 25 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>>>> index 9849db0..a896b1c 100644
>>>> --- a/drivers/watchdog/sp805_wdt.c
>>>> +++ b/drivers/watchdog/sp805_wdt.c
>>>> @@ -11,6 +11,7 @@
>>>> * warranty of any kind, whether express or implied.
>>>> */
>>>> +#include <linux/acpi.h>
>>>> #include <linux/device.h>
>>>> #include <linux/resource.h>
>>>> #include <linux/amba/bus.h>
>>>> @@ -22,6 +23,7 @@
>>>> #include <linux/math64.h>
>>>> #include <linux/module.h>
>>>> #include <linux/moduleparam.h>
>>>> +#include <linux/of.h>
>>>> #include <linux/pm.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/spinlock.h>
>>>> @@ -65,6 +67,7 @@ struct sp805_wdt {
>>>> spinlock_t lock;
>>>> void __iomem *base;
>>>> struct clk *clk;
>>>> + u64 rate;
>>>> struct amba_device *adev;
>>>> unsigned int load_val;
>>>> };
>>>> @@ -80,7 +83,7 @@ static int wdt_setload(struct watchdog_device *wdd,
>>>> unsigned int timeout)
>>>> struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> u64 load, rate;
>>>> - rate = clk_get_rate(wdt->clk);
>>>> + rate = wdt->rate;
>>>> /*
>>>> * sp805 runs counter with given value twice, after the end of
>>>> first
>>>> @@ -106,9 +109,7 @@ static int wdt_setload(struct watchdog_device *wdd,
>>>> unsigned int timeout)
>>>> static unsigned int wdt_timeleft(struct watchdog_device *wdd)
>>>> {
>>>> struct sp805_wdt *wdt = watchdog_get_drvdata(wdd);
>>>> - u64 load, rate;
>>>> -
>>>> - rate = clk_get_rate(wdt->clk);
>>>> + u64 load;
>>>> spin_lock(&wdt->lock);
>>>> load = readl_relaxed(wdt->base + WDTVALUE);
>>>> @@ -118,7 +119,7 @@ static unsigned int wdt_timeleft(struct
>>>> watchdog_device *wdd)
>>>> load += wdt->load_val + 1;
>>>> spin_unlock(&wdt->lock);
>>>> - return div_u64(load, rate);
>>>> + return div_u64(load, wdt->rate);
>>>> }
>>>> static int
>>>> @@ -228,11 +229,25 @@ sp805_wdt_probe(struct amba_device *adev, const
>>>> struct amba_id *id)
>>>> if (IS_ERR(wdt->base))
>>>> return PTR_ERR(wdt->base);
>>>> - wdt->clk = devm_clk_get(&adev->dev, NULL);
>>>> - if (IS_ERR(wdt->clk)) {
>>>> - dev_warn(&adev->dev, "Clock not found\n");
>>>> - ret = PTR_ERR(wdt->clk);
>>>> - goto err;
>>>> + if (adev->dev.of_node) {
>>>> + wdt->clk = devm_clk_get(&adev->dev, NULL);
>>>> + if (IS_ERR(wdt->clk)) {
>>>> + dev_err(&adev->dev, "Clock not found\n");
>>>> + return PTR_ERR(wdt->clk);
>>>> + }
>>>> + wdt->rate = clk_get_rate(wdt->clk);
>>>> + } else if (has_acpi_companion(&adev->dev)) {
>>>> + /*
>>>> + * When Driver probe with ACPI device, clock devices
>>>> + * are not available, so watchdog rate get from
>>>> + * clock-frequency property given in _DSD object.
>>>> + */
>>>> + device_property_read_u64(&adev->dev, "clock-frequency",
>>>> + &wdt->rate);
>>>> + if (!wdt->rate) {
>>>> + dev_err(&adev->dev, "no clock-frequency
>>>> property\n");
>>>> + return -ENODEV;
>>>> + }
>>>> }
>>>> wdt->adev = adev;
>>>>
>>>
>>
>