On 5/3/22 09:22, Hawkins, Nick wrote:
On 5/2/22 13:40, nick.hawkins@xxxxxxx wrote:
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
Where are those of_ includes used ?
They were not used anymore with latest changes. Thank you for pointing this out. I will remember to check in the future for each new commit to double check this.
+#define WDT_MAX_TIMEOUT_MS 655000
Shouldn't that be 655350 ?
Yes it should be. I will correct this.
+static int gxp_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
+ u32 actual;
+
+ wdd->timeout = timeout;
+ actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
+ writew(SECS_TO_WDOG_TICKS(actual), drvdata->base + GXP_WDT_CNT_OFS);
First, the accuracy of actual is reduced to 1 second, then SECS_TO_WDOG_TICKS() multiplies the result with 100, meaning the actual accuracy is 10ms. Why not just use 10 ms ?
actual = min(timeout * 100, wdd->max_hw_heartbeat_ms / 10);
writew(actual, drvdata->base + GXP_WDT_CNT_OFS);
I have replaced the mention code with what you recommended above.
+
+static int gxp_restart(struct watchdog_device *wdd, unsigned long action,
+ void *data)
+{
+ struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
+
+ writew(10, drvdata->base + GXP_WDT_CNT_OFS);
Doesn't that translate to 100 ms timeout ? Why such a large reboot delay instead of writing 1 ?
This has been changed to 1.
+ gxp_wdt_enable_reload(drvdata);
+ mdelay(100);
+ return 0;
+}
+
+static int gxp_wdt_probe(struct platform_device *pdev) {
+ struct device *dev = &pdev->dev;
+ struct gxp_wdt *drvdata;
+ int err;
+ u8 val;
+
+ drvdata = devm_kzalloc(dev, sizeof(struct gxp_wdt), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->base = (void __iomem *)dev->platform_data;
I'd personaly prefer if the address was passed as resource.
Just to clarify for my understanding are you asking that in the device structure I use the "void *platform_data" to pass "struct *resource"? If I am incorrect here can you elaborate on what you would like to be done? Based on feedback in review for the device tree; the watchdog is being created as a child to the timer. Therefore the conclusion reached was there should not be a gxp-wdt listed in the device tree files. I took this implementation based on what I found in ixp4xx_wdt.c.
One bad deed tends to multiply.
No, I didn't ask to pass a struct resource as platform data.
That would be no different to the current code. Resources
can be added to a platform device using
platform_device_add_resources(), and the platform driver
can then use platform_get_resource() to use it. This
would make it independent of a "private" mechanism.
Thanks,
Guenter
