When using ACPI node, binding clock devices are
not available as device tree, So clock-frequency
property given in _DSD object of ACPI device is
used to calculate Watchdog rate.
Signed-off-by: Srinath Mannam <[email protected]>
---
drivers/watchdog/sp805_wdt.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 9849db0..ad5ed64 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,10 +229,27 @@ 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)) {
+ if (adev->dev.of_node) {
+ wdt->clk = devm_clk_get(&adev->dev, NULL);
+ if (IS_ERR(wdt->clk)) {
+ ret = PTR_ERR(wdt->clk);
+ wdt->clk = NULL;
+ } else
+ 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)
+ ret = -ENODEV;
+ }
+
+ if (ret) {
dev_warn(&adev->dev, "Clock not found\n");
- ret = PTR_ERR(wdt->clk);
goto err;
}
--
2.7.4
On 07/09/2018 03:19 AM, Srinath Mannam wrote:
> When using ACPI node, binding clock devices are
> not available as device tree, So clock-frequency
> property given in _DSD object of ACPI device is
> used to calculate Watchdog rate.
>
> Signed-off-by: Srinath Mannam <[email protected]>
> ---
> drivers/watchdog/sp805_wdt.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 9849db0..ad5ed64 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,10 +229,27 @@ 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)) {
> + if (adev->dev.of_node) {
> + wdt->clk = devm_clk_get(&adev->dev, NULL);
> + if (IS_ERR(wdt->clk)) {
> + ret = PTR_ERR(wdt->clk);
> + wdt->clk = NULL;
Clearing wdt->clk is useless.
> + } else
> + wdt->rate = clk_get_rate(wdt->clk);
The else branch should be in { } as well per coding style.
> + } 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);
Continuation line alignment is off.
Still not documented. Maybe that is common for ACPI devices nowadays.
If so, I'll need at least a pointer to a document or something declaring
that ACPI devices do not use well documented properties, and a confirmation
that using such properties is acceptable in the Linux kernel.
> + if (!wdt->rate)
> + ret = -ENODEV;
> + }
> +
> + if (ret) {
> dev_warn(&adev->dev, "Clock not found\n");
> - ret = PTR_ERR(wdt->clk);
> goto err;
> }
Please move the error handling to where the error occurs. While doing that,
please change the message to dev_err() - this is an error, not a warning -
and change the second error message to match the error (it did not find
the property).
Also, return directly - the goto just generates another error message
which is ridiculous.
Thanks,
Guenter
Hi Guenter,
Thank you for your feedback. Please find my answers in lined.
On Mon, Jul 9, 2018 at 7:15 PM, Guenter Roeck <[email protected]> wrote:
> On 07/09/2018 03:19 AM, Srinath Mannam wrote:
>>
>> When using ACPI node, binding clock devices are
>> not available as device tree, So clock-frequency
>> property given in _DSD object of ACPI device is
>> used to calculate Watchdog rate.
>>
>> Signed-off-by: Srinath Mannam <[email protected]>
>> ---
>> drivers/watchdog/sp805_wdt.c | 34 ++++++++++++++++++++++++++--------
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>> index 9849db0..ad5ed64 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,10 +229,27 @@ 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)) {
>> + if (adev->dev.of_node) {
>> + wdt->clk = devm_clk_get(&adev->dev, NULL);
>> + if (IS_ERR(wdt->clk)) {
>> + ret = PTR_ERR(wdt->clk);
>> + wdt->clk = NULL;
>
>
> Clearing wdt->clk is useless.
If not, clk_prepare_enable and clk_disable_unprepare functions calls
made in this driver will crash.
>
>> + } else
>> + wdt->rate = clk_get_rate(wdt->clk);
>
>
> The else branch should be in { } as well per coding style.
I will add the change.
>
>> + } 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);
>
>
> Continuation line alignment is off.
I missed it, Thank you, I will make the change.
>
> Still not documented. Maybe that is common for ACPI devices nowadays.
> If so, I'll need at least a pointer to a document or something declaring
> that ACPI devices do not use well documented properties, and a confirmation
> that using such properties is acceptable in the Linux kernel.
>
patches listed below has same approach to add ACPI support.
ex:
1. commit 515da746983bc6382e380ba8b1ce9345a9550ffe
Author: Naveen Kaje <[email protected]>
Date: Tue Oct 11 10:27:56 2016 -0600
i2c: qup: add ACPI support
2. commit 82a19035d000c8b4fd7d6f61b614f63dec75d389
Author: Lendacky, Thomas <[email protected]>
Date: Fri Jan 16 12:47:16 2015 -0600
amd-xgbe: Add ACPI support
>> + if (!wdt->rate)
>> + ret = -ENODEV;
>> + }
>> +
>> + if (ret) {
>> dev_warn(&adev->dev, "Clock not found\n");
>> - ret = PTR_ERR(wdt->clk);
>> goto err;
>> }
>
>
> Please move the error handling to where the error occurs. While doing that,
> please change the message to dev_err() - this is an error, not a warning -
> and change the second error message to match the error (it did not find
> the property).
>
> Also, return directly - the goto just generates another error message
> which is ridiculous.
Thank you, I will add the changes.
>
> Thanks,
> Guenter
Regards,
Srinath.
Hi Guenter,
I missed your comment about "Clearing wdt->clk is useless." I will
remove that line.
Regards,
Srinath.
On Tue, Jul 10, 2018 at 1:27 PM, Srinath Mannam
<[email protected]> wrote:
> Hi Guenter,
>
> Thank you for your feedback. Please find my answers in lined.
>
> On Mon, Jul 9, 2018 at 7:15 PM, Guenter Roeck <[email protected]> wrote:
>> On 07/09/2018 03:19 AM, Srinath Mannam wrote:
>>>
>>> When using ACPI node, binding clock devices are
>>> not available as device tree, So clock-frequency
>>> property given in _DSD object of ACPI device is
>>> used to calculate Watchdog rate.
>>>
>>> Signed-off-by: Srinath Mannam <[email protected]>
>>> ---
>>> drivers/watchdog/sp805_wdt.c | 34 ++++++++++++++++++++++++++--------
>>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
>>> index 9849db0..ad5ed64 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,10 +229,27 @@ 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)) {
>>> + if (adev->dev.of_node) {
>>> + wdt->clk = devm_clk_get(&adev->dev, NULL);
>>> + if (IS_ERR(wdt->clk)) {
>>> + ret = PTR_ERR(wdt->clk);
>>> + wdt->clk = NULL;
>>
>>
>> Clearing wdt->clk is useless.
>
> If not, clk_prepare_enable and clk_disable_unprepare functions calls
> made in this driver will crash.
Sorry I missed your point. I will remove that.
>
>>
>>> + } else
>>> + wdt->rate = clk_get_rate(wdt->clk);
>>
>>
>> The else branch should be in { } as well per coding style.
> I will add the change.
>>
>>> + } 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);
>>
>>
>> Continuation line alignment is off.
> I missed it, Thank you, I will make the change.
>>
>> Still not documented. Maybe that is common for ACPI devices nowadays.
>> If so, I'll need at least a pointer to a document or something declaring
>> that ACPI devices do not use well documented properties, and a confirmation
>> that using such properties is acceptable in the Linux kernel.
>>
> patches listed below has same approach to add ACPI support.
> ex:
> 1. commit 515da746983bc6382e380ba8b1ce9345a9550ffe
> Author: Naveen Kaje <[email protected]>
> Date: Tue Oct 11 10:27:56 2016 -0600
>
> i2c: qup: add ACPI support
>
> 2. commit 82a19035d000c8b4fd7d6f61b614f63dec75d389
> Author: Lendacky, Thomas <[email protected]>
> Date: Fri Jan 16 12:47:16 2015 -0600
>
> amd-xgbe: Add ACPI support
>
>>> + if (!wdt->rate)
>>> + ret = -ENODEV;
>>> + }
>>> +
>>> + if (ret) {
>>> dev_warn(&adev->dev, "Clock not found\n");
>>> - ret = PTR_ERR(wdt->clk);
>>> goto err;
>>> }
>>
>>
>> Please move the error handling to where the error occurs. While doing that,
>> please change the message to dev_err() - this is an error, not a warning -
>> and change the second error message to match the error (it did not find
>> the property).
>>
>> Also, return directly - the goto just generates another error message
>> which is ridiculous.
> Thank you, I will add the changes.
>>
>> Thanks,
>> Guenter
>
> Regards,
> Srinath.