drivers/watchdog: Add user space access to aspeed ext signal and reset
Subsystems (2)
PATCH 1/1 drivers/watchdog: Add user space access to aspeed ext signal and reset
Add sysfs files for 'ext signal' mode and 'system reset' mode within the aspeed watchdog device driver for wdt1 and wdt2
--- .../ABI/testing/sysfs-platform-aspeed-wdt | 40 ++++++++++ drivers/watchdog/aspeed_wdt.c | 89 +++++++++++++++++++++- 2 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-platform-aspeed-wdt
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +40 -0
@@ -0,0 +1,40 @@ +What: /sys/bus/platform/devices/1e785000.wdt/watchdog/watchdog0/ + reset_system +Date: May 2017 +KernelVersion: 4.12 +Contact: Christopher Bostic <[email protected]> +Description: + Value indicates if a watchdog time out results in a reset of + system. 'off': no system reset on timeout. 'on' there will + be a system reset on timeout. + + Default: off + + - Enable 'reset system' on watchdog timeout: + echo on > /sys/bus/platform/devices/1e785000.wdt/watchdog/ + watchdog0/reset_system; + + - Disable 'reset system' on watchdog timeout: + echo off > /sys/bus/platform/devices/1e785000.wdt/watchdog/ + watchdog0/reset_system; + +What: /sys/bus/platform/devices/1e785000.wdt/watchdog/watchdog0/ + ext_signal +Date: May 2017 +KernelVersion: 4.12 +Contact: Christopher Bostic <[email protected]> +Description: + Value indicates if a watchdog time out results in an active + high pulse sent out an output pin. 'off': No output pulse + generated on watchdog time out. 'on': output pulse + generated on watchdog time out. + + Default: off + + - Enable 'external signal' on watchdog timeout: + echo on > /sys/bus/platform/devices/1e785000.wdt/watchdog/ + watchdog0/ext_signal; + + - Disable 'external signal' on watchdog timeout: + echo off > /sys/bus/platform/devices/1e785000.wdt/watchdog/ + watchdog0/ext_signal;
drivers/watchdog/aspeed_wdt.c +88 -1
101 lines not on this page · raw message
Signature · 1 line
1.8.2.2
Quoted 2 lines · Christopher Bostic
Add sysfs files for 'ext signal' mode and 'system reset' mode within the aspeed watchdog device driver for wdt1 and wdt2
This commit message explains what the patch does, not why we need it. The code change already describes what it does, so can you please instead outline in the commit message what the motivation is (or potential motivations are). Ultimately what I'm interested in is whether this should be a platform design decision (and so should be managed via devicetree), or userspace policy, which is what we have here.
Separately, given it's fairly generic and assuming the motivation is reasonable, I'd suggest also sending it upstream.
Andrew
Quoted 137 lines
Quoted 2 lines
Signed-off-by Christopher Bostic <[email protected]>---
.../ABI/testing/sysfs-platform-aspeed-wdt | 40 ++++++++++ drivers/watchdog/aspeed_wdt.c | 89 +++++++++++++++++++++-2 files changed, 128 insertions(+), 1 deletion(-)create mode 100644
Documentation/ABI/testing/sysfs-platform-aspeed-wdtDocumentation/ABI/testing/sysfs-platform-aspeed-wdt new file +1
-0@@ -0,0 +1,40 @@ +What: /sys/bus/platform/devices/1e785000.wdt/watchdog/watchdog0/Quoted 1 line
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +1
-0+ reset_systemDocumentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+Date: May 2017 +KernelVersion: 4.12Quoted 1 line
+Contact: Christopher Bostic <[email protected]>
+Description:
+ Value indicates if a watchdog time out results in a reset ofQuoted 2 lines
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+ system. 'off': no system reset on timeout. 'on' there will + be a system reset on timeout.Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +5
-0+ + Default: off + + - Enable 'reset system' on watchdog timeout: + echo on > /sys/bus/platform/devices/1e785000.wdt/watchdog/Review
Quoted 1 line
(quoted diff) +1
-0+ watchdog0/reset_system;+
+ - Disable 'reset system' on watchdog timeout:
+ echo off >/sys/bus/platform/devices/1e785000.wdt/watchdog/Quoted 1 line
(quoted diff) +1
-0+ watchdog0/reset_system;Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+ +What: /sys/bus/platform/devices/1e785000.wdt/watchdog/watchdog0/Quoted 1 line
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +1
-0+ ext_signalDocumentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+Date: May 2017 +KernelVersion: 4.12Quoted 1 line
+Contact: Christopher Bostic <[email protected]>
+Description:
+ Value indicates if a watchdog time out results in an activeQuoted 3 lines
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +3
-0+ high pulse sent out an output pin. 'off': No output pulse + generated on watchdog time out. 'on': output pulse + generated on watchdog time out.Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +5
-0+ + Default: off + + - Enable 'external signal' on watchdog timeout: + echo on > /sys/bus/platform/devices/1e785000.wdt/watchdog/Review
Quoted 1 line
(quoted diff) +1
-0+ watchdog0/ext_signal;+
+ - Disable 'external signal' on watchdog timeout:
+ echo off >/sys/bus/platform/devices/1e785000.wdt/watchdog/Quoted 1 line
(quoted diff) +1
-0+ watchdog0/ext_signal;drivers/watchdog/aspeed_wdt.c +0
-0@@ -145,6 +145,93 @@ static int aspeed_wdt_remove(struct platform_device *pdev) > return 0; }
+staticssize_taspeed_wdt_reset_system_store(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + const char *buf, size_t count)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct platform_device *pdev = to_platform_device(dev); + struct aspeed_wdt *wdt = platform_get_drvdata(pdev);+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 164
Quoted 10 lines
drivers/watchdog/aspeed_wdt.c +10
-0+if (!strncmp (buf,"on" ,strlen ("on" ))) { + wdt->ctrl |=WDT_CTRL_RESET_SYSTEM ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else if (!strncmp (buf,"off" ,strlen ("off" ))) { + wdt->ctrl &= ~WDT_CTRL_RESET_SYSTEM ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else { +dev_warn (dev,"Unknown reset system mode command: [%s]\n" , buf); +return -EINVAL ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return count;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static ssize_t aspeed_wdt_reset_system_show(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + char *buf)+{
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 175
Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+struct platform_device *pdev =to_platform_device (dev); +struct aspeed_wdt *wdt =platform_get_drvdata (pdev); +int ret;+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 180
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+if (readl (wdt->base +WDT_CTRL ) &WDT_CTRL_RESET_SYSTEM ) + ret =sprintf (buf,"on\n" ); +else + ret =sprintf (buf,"off\n" );+
Quoted 1 line
(quoted diff) +1
-0+ return ret;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static DEVICE_ATTR(reset_system, 0644,Quoted 1 line
drivers/watchdog/aspeed_wdt.c +1
-0+ aspeed_wdt_reset_system_show, aspeed_wdt_reset_system_store);drivers/watchdog/aspeed_wdt.c +2
-0+ +static ssize_t aspeed_wdt_ext_signal_store(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + const char *buf, size_t count)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct platform_device *pdev = to_platform_device(dev); + struct aspeed_wdt *wdt = platform_get_drvdata(pdev);+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 204
Quoted 10 lines
drivers/watchdog/aspeed_wdt.c +10
-0+if (!strncmp (buf,"on" ,strlen ("on" ))) { + wdt->ctrl |=WDT_CTRL_WDT_EXT ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else if (!strncmp (buf,"off" ,strlen ("off" ))) { + wdt->ctrl &= ~WDT_CTRL_WDT_EXT ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else { +dev_warn (dev,"Unknown reset system mode command: %s\n" , buf); +return -EINVAL ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return count;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static ssize_t aspeed_wdt_ext_signal_show(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + char *buf)+{
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 175
Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+struct platform_device *pdev =to_platform_device (dev); +struct aspeed_wdt *wdt =platform_get_drvdata (pdev); +int ret;+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 220
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+if (readl (wdt->base +WDT_CTRL ) &WDT_CTRL_WDT_EXT ) + ret =sprintf (buf,"on\n" ); +else + ret =sprintf (buf,"off\n" );+
Quoted 1 line
(quoted diff) +1
-0+ return ret;
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 226
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+} + +static DEVICE_ATTR(ext_signal, 0644, + aspeed_wdt_ext_signal_show, aspeed_wdt_ext_signal_store);
The on/off implementations for show/store callbacks are very similar, only differentiated by the bit that we're flipping. We could almost cut the code in half by parameterising a function with the bit of interest and invoking it appropriately in the callback handlers. Thoughts?
Andrew
Quoted 20 lines
drivers/watchdog/aspeed_wdt.c +2
-0+ +static struct attribute *aspeed_wdt_attrs[] = {Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+ &dev_attr_reset_system.attr, + &dev_attr_ext_signal.attr, +NULL drivers/watchdog/aspeed_wdt.c +3
-0+}; +ATTRIBUTE_GROUPS (aspeed_wdt); +static int
aspeed_wdt_probe(struct platform_device *pdev){Quoted 1 line
struct
aspeed_wdt*wdt;drivers/watchdog/aspeed_wdt.c +0
-1@@ -169,7 +256,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > wdt->wdd.ops = &aspeed_wdt_ops; > wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS; > wdt->wdd.parent = &pdev->dev; - > + wdt->wdd.groups = aspeed_wdt_groups; > wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT; > watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
Quoted 12 lines · Andrew Jeffery
Quoted 2 lines · Christopher Bostic
Add sysfs files for 'ext signal' mode and 'system reset' mode within the aspeed watchdog device driver for wdt1 and wdt2
This commit message explains what the patch does, not why we need it. The code change already describes what it does, so can you please instead outline in the commit message what the motivation is (or potential motivations are). Ultimately what I'm interested in is whether this should be a platform design decision (and so should be
Witherspoon has a physical connection between one of the watchdogs on the SOC and the max31785 fan control chip. We want the line to wiggle if the watchdog is not pinged by userspace.
That said, the specific watchdog instance on the SOC that has the connection can be configured to wiggle the pin, or not, and to reset the chip, or not, independently.
So is that platform design or userspace policy? I’m not sure...
Quoted 5 lines
managed via devicetree), or userspace policy, which is what we have here.
Separately, given it's fairly generic and assuming the motivation is reasonable, I'd suggest also sending it upstream.
Agreed on doing the review directly upstream.
Quoted 171 lines
Andrew
Quoted 137 lines
Quoted 2 lines
Signed-off-by Christopher Bostic <[email protected]>--- .../ABI/testing/sysfs-platform-aspeed-wdt | 40 ++++++++++ drivers/watchdog/aspeed_wdt.c | 89 +++++++++++++++++++++- 2 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-platform-aspeed-wdtDocumentation/ABI/testing/sysfs-platform-aspeed-wdt new file +1
-0@@ -0,0 +1,40 @@ +What: /sys/bus/platform/devices/1e785000.wdt/watchdog/watchdog0/Quoted 1 line
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +1
-0+ reset_systemDocumentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+Date: May 2017 +KernelVersion: 4.12Quoted 1 line
+Contact: Christopher Bostic <[email protected]>
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+Description: + Value indicates if a watchdog time out results in a reset ofQuoted 2 lines
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+ system. 'off': no system reset on timeout. 'on' there will + be a system reset on timeout.Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +5
-0+ + Default: off + + - Enable 'reset system' on watchdog timeout: + echo on > /sys/bus/platform/devices/1e785000.wdt/watchdog/Quoted 1 line
(quoted diff) +1
-0+ watchdog0/reset_system;Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +3
-0+ + - Disable 'reset system' on watchdog timeout: + echo off > /sys/bus/platform/devices/1e785000.wdt/watchdog/Quoted 1 line
(quoted diff) +1
-0+ watchdog0/reset_system;Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+ +What: /sys/bus/platform/devices/1e785000.wdt/watchdog/watchdog0/Quoted 1 line
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +1
-0+ ext_signalDocumentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+Date: May 2017 +KernelVersion: 4.12Quoted 1 line
+Contact: Christopher Bostic <[email protected]>
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +2
-0+Description: + Value indicates if a watchdog time out results in an activeQuoted 3 lines
Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +3
-0+ high pulse sent out an output pin. 'off': No output pulse + generated on watchdog time out. 'on': output pulse + generated on watchdog time out.Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +5
-0+ + Default: off + + - Enable 'external signal' on watchdog timeout: + echo on > /sys/bus/platform/devices/1e785000.wdt/watchdog/Quoted 1 line
(quoted diff) +1
-0+ watchdog0/ext_signal;Documentation/ABI/testing/sysfs-platform-aspeed-wdt new file +3
-0+ + - Disable 'external signal' on watchdog timeout: + echo off > /sys/bus/platform/devices/1e785000.wdt/watchdog/Quoted 1 line
(quoted diff) +1
-0+ watchdog0/ext_signal;drivers/watchdog/aspeed_wdt.c +2
-0@@ -145,6 +145,93 @@ static int aspeed_wdt_remove(struct platform_device *pdev) > return 0; } +static ssize_t aspeed_wdt_reset_system_store (struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) +{ > + struct platform_device *pdev = to_platform_device(dev);Review
Quoted 1 line
(quoted diff) +1
-0+ struct aspeed_wdt *wdt = platform_get_drvdata(pdev);+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 164
Quoted 10 lines
drivers/watchdog/aspeed_wdt.c +10
-0+if (!strncmp (buf,"on" ,strlen ("on" ))) { + wdt->ctrl |=WDT_CTRL_RESET_SYSTEM ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else if (!strncmp (buf,"off" ,strlen ("off" ))) { + wdt->ctrl &= ~WDT_CTRL_RESET_SYSTEM ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else { +dev_warn (dev,"Unknown reset system mode command: [%s]\n" , buf); +return -EINVAL ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return count;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static ssize_t aspeed_wdt_reset_system_show(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + char *buf)+{
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 175
Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+struct platform_device *pdev =to_platform_device (dev); +struct aspeed_wdt *wdt =platform_get_drvdata (pdev); +int ret;+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 180
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+if (readl (wdt->base +WDT_CTRL ) &WDT_CTRL_RESET_SYSTEM ) + ret =sprintf (buf,"on\n" ); +else + ret =sprintf (buf,"off\n" );+
Quoted 1 line
(quoted diff) +1
-0+ return ret;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static DEVICE_ATTR(reset_system, 0644,Quoted 1 line
drivers/watchdog/aspeed_wdt.c +1
-0+ aspeed_wdt_reset_system_show, aspeed_wdt_reset_system_store);drivers/watchdog/aspeed_wdt.c +2
-0+ +static ssize_t aspeed_wdt_ext_signal_store(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + const char *buf, size_t count)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct platform_device *pdev = to_platform_device(dev); + struct aspeed_wdt *wdt = platform_get_drvdata(pdev);+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 204
Quoted 10 lines
drivers/watchdog/aspeed_wdt.c +10
-0+if (!strncmp (buf,"on" ,strlen ("on" ))) { + wdt->ctrl |=WDT_CTRL_WDT_EXT ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else if (!strncmp (buf,"off" ,strlen ("off" ))) { + wdt->ctrl &= ~WDT_CTRL_WDT_EXT ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else { +dev_warn (dev,"Unknown reset system mode command: %s\n" , buf); +return -EINVAL ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return count;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static ssize_t aspeed_wdt_ext_signal_show(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + char *buf)+{
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 175
Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+struct platform_device *pdev =to_platform_device (dev); +struct aspeed_wdt *wdt =platform_get_drvdata (pdev); +int ret;+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 220
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+if (readl (wdt->base +WDT_CTRL ) &WDT_CTRL_WDT_EXT ) + ret =sprintf (buf,"on\n" ); +else + ret =sprintf (buf,"off\n" );+
Quoted 1 line
(quoted diff) +1
-0+ return ret;drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 226
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+} + +static DEVICE_ATTR(ext_signal, 0644, + aspeed_wdt_ext_signal_show, aspeed_wdt_ext_signal_store);The on/off implementations for show/store callbacks are very similar, only differentiated by the bit that we're flipping. We could almost cut the code in half by parameterising a function with the bit of interest and invoking it appropriately in the callback handlers. Thoughts?
Andrew
Quoted 19 lines
drivers/watchdog/aspeed_wdt.c +2
-0+ +static struct attribute *aspeed_wdt_attrs[] = {Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+ &dev_attr_reset_system.attr, + &dev_attr_ext_signal.attr, +NULL drivers/watchdog/aspeed_wdt.c +3
-0+}; +ATTRIBUTE_GROUPS (aspeed_wdt); +static int aspeed_wdt_probe (struct platform_device *pdev) {Quoted 1 line
struct
aspeed_wdt*wdt;drivers/watchdog/aspeed_wdt.c +0
-1@@ -169,7 +256,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > wdt->wdd.ops = &aspeed_wdt_ops; > wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS; > wdt->wdd.parent = &pdev->dev; - > + wdt->wdd.groups = aspeed_wdt_groups; > wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT; > watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
Quoted 11 lines · Andrew Jeffery
Quoted 2 lines · Christopher Bostic
Add sysfs files for 'ext signal' mode and 'system reset' mode within the aspeed watchdog device driver for wdt1 and wdt2
This commit message explains what the patch does, not why we need it. The code change already describes what it does, so can you please instead outline in the commit message what the motivation is (or potential motivations are). Ultimately what I'm interested in is whether this should be a platform design decision (and so should be managed via devicetree), or userspace policy, which is what we have here.
Quoted 2 lines
Separately, given it's fairly generic and assuming the motivation is reasonable, I'd suggest also sending it upstream.
Hi Andrew,
Will change the 'what' to 'why' as per your recommendations.
Quoted 29 lines
Andrew
Quoted 27 lines
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>+
wdt->ctrl|=WDT_CTRL_WDT_EXT;drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 204
Quoted 8 lines
drivers/watchdog/aspeed_wdt.c +8
-0+writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else if (!strncmp (buf,"off" ,strlen ("off" ))) { + wdt->ctrl &= ~WDT_CTRL_WDT_EXT ; +writel (wdt->ctrl, wdt->base +WDT_CTRL ); + }else { +dev_warn (dev,"Unknown reset system mode command: %s\n" , buf); +return -EINVAL ; + }+
Quoted 1 line
(quoted diff) +1
-0+ return count;drivers/watchdog/aspeed_wdt.c +3
-0+} + +static ssize_t aspeed_wdt_ext_signal_show(struct device *dev,Review
Quoted 2 lines
(quoted diff) +2
-0+ struct device_attribute *attr, + char *buf)+{
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 175
Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+struct platform_device *pdev =to_platform_device (dev); +struct aspeed_wdt *wdt =platform_get_drvdata (pdev); +int ret;+
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 220
Quoted 4 lines
drivers/watchdog/aspeed_wdt.c +4
-0+if (readl (wdt->base +WDT_CTRL ) &WDT_CTRL_WDT_EXT ) + ret =sprintf (buf,"on\n" ); +else + ret =sprintf (buf,"off\n" );+
Quoted 9 lines
drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_remove(struct platform_device *pdev) · line 226
Quoted 5 lines
Quoted 1 line
(quoted diff) +1
-0+ return ret;drivers/watchdog/aspeed_wdt.c +4
-0+} + +static DEVICE_ATTR(ext_signal, 0644, + aspeed_wdt_ext_signal_show, aspeed_wdt_ext_signal_store);The on/off implementations for show/store callbacks are very similar, only differentiated by the bit that we're flipping. We could almost cut the code in half by parameterising a function with the bit of interest and invoking it appropriately in the callback handlers. Thoughts?
True, will optimize this.
Thanks,
Chris
Quoted 23 lines
Andrew
Quoted 20 lines
(quoted diff) +2
-0+ +static struct attribute *aspeed_wdt_attrs[] = {Quoted 3 lines
drivers/watchdog/aspeed_wdt.c +3
-0+ &dev_attr_reset_system.attr, + &dev_attr_ext_signal.attr, +NULL drivers/watchdog/aspeed_wdt.c +3
-0+}; +ATTRIBUTE_GROUPS (aspeed_wdt); +static int aspeed_wdt_probe (struct platform_device *pdev) {Quoted 1 line
struct
aspeed_wdt*wdt;(quoted diff) +0
-1@@ -169,7 +256,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > wdt->wdd.ops = &aspeed_wdt_ops; > wdt->wdd.max_hw_heartbeat_ms = WDT_MAX_TIMEOUT_MS; > wdt->wdd.parent = &pdev->dev; - > + wdt->wdd.groups = aspeed_wdt_groups; > wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT; > watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);