Skip to contentSkip to conversation

watchdog: Add watchdog timer support for the WinSystems EBC-C384

PATCH Superseded William Breathitt Gray 5 participants 18 messages Jan 25, 2016 – Feb 28, 2016

Subsystems (2)

v3 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 watchdog: Add watchdog timer support for the WinSystems EBC-C384

The WinSystems EBC-C384 has an onboard watchdog timer. The timeout range supported by the watchdog timer is 1 second to 255 minutes. Timeouts under 256 seconds have a 1 second resolution, while the rest have a 1 minute resolution.

This driver adds watchdog timer support for this onboard watchdog timer. The timeout may be configured via the timeout module parameter.

---
Changes in v3:
- Remove unnecessary explicit initialization of the timeout parameter
- Move dmi_match call to beginning of init function
- Create WATCHDOG_MAX_TIMEOUT define and explain its magic number
- Use platform_set_drvdata to match later call to platform_get_drvdata
- Use MODULE_NAME as argument for the platform_device_alloc call
- Remove duplicate return statement for the init function

 MAINTAINERS                     |   6 ++
 drivers/watchdog/Kconfig        |   9 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/ebc-c384_wdt.c | 187 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 drivers/watchdog/ebc-c384_wdt.c
MAINTAINERS +6 -0
index b1e3da7..c058abf 100644
@@ -11629,6 +11629,12 @@ M:	David Härdeman <[email protected]>
 S:	Maintained
 F:	drivers/media/rc/winbond-cir.c
 
+WINSYSTEMS EBC-C384 WATCHDOG DRIVER
+M:	William Breathitt Gray <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/watchdog/ebc-c384_wdt.c
+
 WIMAX STACK
 M:	Inaky Perez-Gonzalez <[email protected]>
 M:	[email protected]
drivers/watchdog/Kconfig +9 -0
index 4f0e7be..b5b1353 100644
@@ -711,6 +711,15 @@ config ALIM7101_WDT
 
 	  Most people will say N.
 
+config EBC_C386_WDT
+	tristate "WinSystems EBC-C384 Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	help
+	  Enables watchdog timer support for the watchdog timer on the
+	  WinSystems EBC-C384 motherboard. The timeout may be configured via
+	  the timeout module parameter.
+
 config F71808E_WDT
 	tristate "Fintek F71808E, F71862FG, F71869, F71882FG and F71889FG Watchdog"
 	depends on X86
drivers/watchdog/Makefile +1 -0
index f566753..1522316 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
 obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
 obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
 obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_EBC_C386_WDT) += ebc-c384_wdt.o
 obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
 obj-$(CONFIG_SP5100_TCO) += sp5100_tco.o
 obj-$(CONFIG_GEODE_WDT) += geodewdt.o
drivers/watchdog/ebc-c384_wdt.c new file +187 -0

187 lines not on this page · raw message

Signature · 1 line
2.4.10
#1 lore raw

drivers/watchdog/ebc-c384_wdt.c line 75

Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10 -0
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

drivers/watchdog/ebc-c384_wdt.c line 153

Quoted 7 lines
drivers/watchdog/ebc-c384_wdt.c new file +7 -0
+static int __init ebc_c384_wdt_init(void)
+{
+	int err;
+
+	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
+		return -ENODEV;
+

Is there no ACPI entry for it ?

Alan

maintainer
#2 lore raw
Quoted 6 lines · One Thousand Gnomes
Quoted 6 lines
drivers/watchdog/ebc-c384_wdt.c new file +6 -0
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
Quoted 9 lines

drivers/watchdog/ebc-c384_wdt.c line 75

Quoted 4 lines
drivers/watchdog/ebc-c384_wdt.c new file +4 -0
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

Matter of endless discussion. Some argue that the value should be rounded up, some argue that it should be rounded down, some argue that it should be rounded to the closest match. Each camp has its own valid arguments. I usually leave it up to the driver's author to decide, with a slight preference to never select a value larger than requested.

Quoted 10 lines

drivers/watchdog/ebc-c384_wdt.c line 153

Quoted 7 lines
drivers/watchdog/ebc-c384_wdt.c new file +7 -0
+static int __init ebc_c384_wdt_init(void)
+{
+	int err;
+
+	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
+		return -ENODEV;
+

Is there no ACPI entry for it ?

Same here. As long as the board is identified, I tend to leave it up to the driver author to decide _how_ to identify it.

Only question for me would be if the watchdog timer is implemented in a Super-IO chip, and if so, if it would be possible to use the chip identification instead of a DMI (or ACPI) entry to instantiate the driver.

Thanks,
Guenter

#3 lore raw
Quoted 11 lines · Guenter Roeck
Quoted 4 lines · One Thousand Gnomes

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

Matter of endless discussion. Some argue that the value should be rounded up, some argue that it should be rounded down, some argue that it should be rounded to the closest match. Each camp has its own valid arguments. I usually leave it up to the driver's author to decide, with a slight preference to never select a value larger than requested.

I implemented it to round down simply because it was the simplest solution (i.e. integer truncation). Although I see merit in an implementation that rounds to the closest valid value, I'll keep the current implementation for now due to its simplicity; if enough users of the driver prefer a different implementation, then I'll add it in a later patch.

Quoted 8 lines
Quoted 2 lines

Is there no ACPI entry for it ?

Same here. As long as the board is identified, I tend to leave it up to the driver author to decide _how_ to identify it.

Only question for me would be if the watchdog timer is implemented in a Super-IO chip, and if so, if it would be possible to use the chip identification instead of a DMI (or ACPI) entry to instantiate the driver.

I do not believe there is an ACPI entry for it. Interestingly, the watchdog timer BIOS configuration option for this motherboard is listed under the Super I/O menu; perhaps this watchdog timer is implemented in the Super I/O chip.

The manual for this motherboard does not provide much information about the Super I/O chip (no model number, etc.), and neither sensors-detect nor superiotool was able to detect it. I've sent an email to the motherboard company (WinSystems) requesting further information about the Super I/O chip and whether the watchdog timer is built-in to the Super I/O chip.

William Breathitt Gray

maintainer
#4 lore raw
Quoted 25 lines · William Breathitt Gray
Quoted 11 lines · Guenter Roeck
Quoted 4 lines · One Thousand Gnomes

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

Matter of endless discussion. Some argue that the value should be rounded up, some argue that it should be rounded down, some argue that it should be rounded to the closest match. Each camp has its own valid arguments. I usually leave it up to the driver's author to decide, with a slight preference to never select a value larger than requested.

I implemented it to round down simply because it was the simplest solution (i.e. integer truncation). Although I see merit in an implementation that rounds to the closest valid value, I'll keep the current implementation for now due to its simplicity; if enough users of the driver prefer a different implementation, then I'll add it in a later patch.

Quoted 5 lines
Quoted 2 lines

Is there no ACPI entry for it ?

Same here. As long as the board is identified, I tend to leave it up to the driver author to decide _how_ to identify it.

Quoted 9 lines
Quoted 3 lines

Only question for me would be if the watchdog timer is implemented in a Super-IO chip, and if so, if it would be possible to use the chip identification instead of a DMI (or ACPI) entry to instantiate the driver.

I do not believe there is an ACPI entry for it. Interestingly, the watchdog timer BIOS configuration option for this motherboard is listed under the Super I/O menu; perhaps this watchdog timer is implemented in the Super I/O chip.

Normally it is. Question is which one.

Quoted 7 lines

The manual for this motherboard does not provide much information about the Super I/O chip (no model number, etc.), and neither sensors-detect nor superiotool was able to detect it. I've sent an email to the motherboard company (WinSystems) requesting further information about the Super I/O chip and whether the watchdog timer is built-in to the Super I/O chip.

Ah, I somehow thought you were associated with WinSystems, since you know how to configure the chip.

Did you get any useful output from sensors-detect or superiotool (like 'unknown chip xxxx'), or did those tools find nothing ?

Thanks,
Guenter

#5 lore raw
Quoted 8 lines · Guenter Roeck
Quoted 7 lines

The manual for this motherboard does not provide much information about the Super I/O chip (no model number, etc.), and neither sensors-detect nor superiotool was able to detect it. I've sent an email to the motherboard company (WinSystems) requesting further information about the Super I/O chip and whether the watchdog timer is built-in to the Super I/O chip.

Quoted 5 lines

Ah, I somehow thought you were associated with WinSystems, since you know how to configure the chip.

Did you get any useful output from sensors-detect or superiotool (like 'unknown chip xxxx'), or did those tools find nothing ?

Unfortunately, the sensors-detect only reported "No" for each Super I/O chip test, while the superiotool gave an unhelpful "No Super I/O chip detected" message.

I haven't heard a response yet from WinSystems, but I'll give them a couple days before sending another email to their engineering department. For now, I'll submit a version 4 of this patch to get the minor updates I made out for review; for what its worth, I believe the dmi_match method will be sufficient until I get an update from WinSystems helping me get a proper check to identify the Super I/O chip.

William Breathitt Gray

maintainer
#6 lore raw
Quoted 12 lines · William Breathitt Gray
Quoted 11 lines · Guenter Roeck
Quoted 7 lines

The manual for this motherboard does not provide much information about the Super I/O chip (no model number, etc.), and neither sensors-detect nor superiotool was able to detect it. I've sent an email to the motherboard company (WinSystems) requesting further information about the Super I/O chip and whether the watchdog timer is built-in to the Super I/O chip.

Ah, I somehow thought you were associated with WinSystems, since you know how to configure the chip.

Quoted 7 lines
Quoted 2 lines

Did you get any useful output from sensors-detect or superiotool (like 'unknown chip xxxx'), or did those tools find nothing ?

Unfortunately, the sensors-detect only reported "No" for each Super I/O chip test, while the superiotool gave an unhelpful "No Super I/O chip detected" message.

Too bad. That suggests that the watchdog may in fact be implemented in the fpga.

Quoted 7 lines

I haven't heard a response yet from WinSystems, but I'll give them a couple days before sending another email to their engineering department. For now, I'll submit a version 4 of this patch to get the minor updates I made out for review; for what its worth, I believe the dmi_match method will be sufficient until I get an update from WinSystems helping me get a proper check to identify the Super I/O chip.

I added v4 to my watchdog-next branch and will include it in my pull request to Wim later in the release cycle. If there are enhancements (eg if WinSystems comes back with an actual specification), we can implement those as incremental improvements.

Thanks,
Guenter

#7 lore raw
Quoted 6 lines · Guenter Roeck
Quoted 4 lines

Unfortunately, the sensors-detect only reported "No" for each Super I/O chip test, while the superiotool gave an unhelpful "No Super I/O chip detected" message.

Too bad. That suggests that the watchdog may in fact be implemented in the fpga.

I received a response from WinSystems: the watchdog timer is implemented in the Lattice FPGA (base address 0x298), along with other WinSystems firmware. I was offered two methods of identifying the chip.

The first method is to use a 16-bit read of the register at port address 0x29E to get the version number of the watchdog timer; my machine reported a value of 0x0009. Unfortunately, I don't believe this method is very reliable since the version number may not be consistent across these motherboards, and the same value could easily happen to be returned by an unknown hardware.

The second method is slightly more involved so I'll quote WinSystems:

Quoted 4 lines

8-bit read of 299h – save this value
8-bit write of 60h to 299h
16-bit read of 29ah should return the base address of the WDT which is 564h 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address

If the system does return a value of 0x564, then it's pretty safe to say that the watchdog timer is implemented on the chip. However, I'm not sure it would be safe to send write commands to a port address until the hardware has been identified; this second method may not be the best route either.

What do you think?

William Breathitt Gray

maintainer
#8 lore raw
Quoted 26 lines · William Breathitt Gray
Quoted 6 lines · Guenter Roeck
Quoted 4 lines

Unfortunately, the sensors-detect only reported "No" for each Super I/O chip test, while the superiotool gave an unhelpful "No Super I/O chip detected" message.

Too bad. That suggests that the watchdog may in fact be implemented in the fpga.

I received a response from WinSystems: the watchdog timer is implemented in the Lattice FPGA (base address 0x298), along with other WinSystems firmware. I was offered two methods of identifying the chip.

The first method is to use a 16-bit read of the register at port address 0x29E to get the version number of the watchdog timer; my machine reported a value of 0x0009. Unfortunately, I don't believe this method is very reliable since the version number may not be consistent across these motherboards, and the same value could easily happen to be returned by an unknown hardware.

The second method is slightly more involved so I'll quote WinSystems:

Quoted 4 lines

8-bit read of 299h – save this value
8-bit write of 60h to 299h
16-bit read of 29ah should return the base address of the WDT which is 564h 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address

Quoted 8 lines

If the system does return a value of 0x564, then it's pretty safe to say that the watchdog timer is implemented on the chip. However, I'm not sure it would be safe to send write commands to a port address until the hardware has been identified; this second method may not be the best route either.

What do you think?

Let's stick with DMI.

Thanks,
Guenter

#9 lore raw
In reply to William Breathitt Gray
Quoted 5 lines
Quoted 4 lines

8-bit read of 299h – save this value
8-bit write of 60h to 299h
16-bit read of 29ah should return the base address of the WDT which is 564h 8-bit write of saved value to 299h - don’t want t accidentally change the WDT base address

Quoted 7 lines

If the system does return a value of 0x564, then it's pretty safe to say that the watchdog timer is implemented on the chip. However, I'm not sure it would be safe to send write commands to a port address until the hardware has been identified; this second method may not be the best route either.

What do you think?

Pokng at 299/29a could hang other systems, so DMI is definitely preferable if it's not described in ACPI as it perhaps ought to have been. If all boards with that DMI string have the device then sorted, if not then perhaps check 299/29a after the DMI match.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#10 lore raw
In reply to William Breathitt Gray
Quoted 34 lines · William Breathitt Gray
Quoted 11 lines · Guenter Roeck
Quoted 4 lines · One Thousand Gnomes

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

Matter of endless discussion. Some argue that the value should be rounded up, some argue that it should be rounded down, some argue that it should be rounded to the closest match. Each camp has its own valid arguments. I usually leave it up to the driver's author to decide, with a slight preference to never select a value larger than requested.

I implemented it to round down simply because it was the simplest solution (i.e. integer truncation). Although I see merit in an implementation that rounds to the closest valid value, I'll keep the current implementation for now due to its simplicity; if enough users of the driver prefer a different implementation, then I'll add it in a later patch.

Quoted 8 lines
Quoted 2 lines

Is there no ACPI entry for it ?

Same here. As long as the board is identified, I tend to leave it up to the driver author to decide _how_ to identify it.

Only question for me would be if the watchdog timer is implemented in a Super-IO chip, and if so, if it would be possible to use the chip identification instead of a DMI (or ACPI) entry to instantiate the driver.

I do not believe there is an ACPI entry for it. Interestingly, the watchdog timer BIOS configuration option for this motherboard is listed under the Super I/O menu; perhaps this watchdog timer is implemented in the Super I/O chip.

Quoted 7 lines

The manual for this motherboard does not provide much information about the Super I/O chip (no model number, etc.), and neither sensors-detect nor superiotool was able to detect it. I've sent an email to the motherboard company (WinSystems) requesting further information about the Super I/O chip and whether the watchdog timer is built-in to the Super I/O chip.

I had a close look at the board pictures in the manual. Looks like there is a NCT7802Y hardware monitoring chip on the board. This suggests that the Super-IO chip does probably not implement hardware monitoring. Not that it helps much to know that, but it reduces the possibilities.

There is also a pretty large Lattice FPGA on the board, which makes it at least somewhat likely that the functionality is implemented in that FPGA.

Guenter

#11 lore raw
In reply to One Thousand Gnomes
Quoted 6 lines
Quoted 6 lines
drivers/watchdog/ebc-c384_wdt.c new file +6 -0
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
Quoted 8 lines

drivers/watchdog/ebc-c384_wdt.c line 75

Quoted 4 lines
drivers/watchdog/ebc-c384_wdt.c new file +4 -0
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

This is my preference:

	if (t > 255)
		t = (((t - 1) / 60) + 1) * 60;

Which basically is a round-up to minutes
t = 256 -> gives 300
t = 299 -> gives 300
t = 300 -> gives 300
t = 301 -> gives 360
t = 15299 -> gives 15300
t = 15300 -> gives 15300
t = 15301 -> gives 15360

So I am also for going late.

Kind regards,
Wim.

#12 lore raw
Quoted 28 lines · Wim Van Sebroeck
Quoted 6 lines
Quoted 6 lines
drivers/watchdog/ebc-c384_wdt.c new file +6 -0
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
Quoted 8 lines

drivers/watchdog/ebc-c384_wdt.c line 75

Quoted 4 lines
drivers/watchdog/ebc-c384_wdt.c new file +4 -0
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

This is my preference:

	if (t > 255)
		t = (((t - 1) / 60) + 1) * 60;

Which basically is a round-up to minutes
t = 256 -> gives 300
t = 299 -> gives 300
t = 300 -> gives 300
t = 301 -> gives 360
t = 15299 -> gives 15300
t = 15300 -> gives 15300
t = 15301 -> gives 15360

Quoted 4 lines

So I am also for going late.

Kind regards,
Wim.

I decided to give this another consideration: perhaps we should allow it to go late rather than early. I figure that when a user configures the watchdog timer for a certain value in seconds they will have an expectation that the watchdog timer will wait at least that amount of time (with the understanding that it may be somewhat late due to the granularity of the timer).

This behavior follows a lot of other timer standards (e.g. nanosleep) so it's what the user expects and what we should follow for now. I'll submit a version 5 patch with this change.

William Breathitt Gray

maintainer
#13 lore raw
In reply to Wim Van Sebroeck
Quoted 15 lines · Wim Van Sebroeck
Quoted 6 lines
Quoted 6 lines
drivers/watchdog/ebc-c384_wdt.c new file +6 -0
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
Quoted 8 lines

drivers/watchdog/ebc-c384_wdt.c line 75

Quoted 4 lines
drivers/watchdog/ebc-c384_wdt.c new file +4 -0
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

Quoted 4 lines

This is my preference:

	if (t > 255)
		t = (((t - 1) / 60) + 1) * 60;

In case I am missing something: Why not just use DIV_ROUND_UP() ?

Thanks,
Guenter

#14 lore raw

Hi Guenter,

Quoted 17 lines · Wim Van Sebroeck
Quoted 16 lines
Quoted 15 lines

drivers/watchdog/ebc-c384_wdt.c line 75

Quoted 11 lines

+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)

drivers/watchdog/ebc-c384_wdt.c new file +9 -0
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);

If ask for 299 seconds surely I should get 300 not 240 ? (Whether to round off or round up is an interesting question for the middle range - does it go off early or late - I'd have said late but...)

Quoted 6 lines
Quoted 4 lines

This is my preference:

	if (t > 255)
		t = (((t - 1) / 60) + 1) * 60;

In case I am missing something: Why not just use DIV_ROUND_UP() ?

That's indeed also usable (from kernel.h: #define DIV_ROUND_UP(n,d) (((n) + (d) -1) / (d)) which is the same) and indeed preferable.

Kind regards,
Wim.

maintainer Reviewed-by on v3 1/1
#15 lore raw
In reply to William Breathitt Gray
Quoted 9 lines · William Breathitt Gray

The WinSystems EBC-C384 has an onboard watchdog timer. The timeout range supported by the watchdog timer is 1 second to 255 minutes. Timeouts under 256 seconds have a 1 second resolution, while the rest have a 1 minute resolution.

This driver adds watchdog timer support for this onboard watchdog timer. The timeout may be configured via the timeout module parameter.

Nitpicks only this time. Please fix and resubmit, and feel free to add

Thanks,
Guenter

Quoted 90 lines

---
Changes in v3:
- Remove unnecessary explicit initialization of the timeout parameter
- Move dmi_match call to beginning of init function
- Create WATCHDOG_MAX_TIMEOUT define and explain its magic number
- Use platform_set_drvdata to match later call to platform_get_drvdata
- Use MODULE_NAME as argument for the platform_device_alloc call
- Remove duplicate return statement for the init function

  MAINTAINERS                     |   6 ++
  drivers/watchdog/Kconfig        |   9 ++
  drivers/watchdog/Makefile       |   1 +
  drivers/watchdog/ebc-c384_wdt.c | 187 ++++++++++++++++++++++++++++++++++++++++
  4 files changed, 203 insertions(+)

create mode 100644 drivers/watchdog/ebc-c384_wdt.c

MAINTAINERS +6 -0
index b1e3da7..c058abf 100644
@@ -11629,6 +11629,12 @@ M:	David Härdeman <[email protected]>
  S:	Maintained
  F:	drivers/media/rc/winbond-cir.c
 
+WINSYSTEMS EBC-C384 WATCHDOG DRIVER
+M:	William Breathitt Gray <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/watchdog/ebc-c384_wdt.c
+
  WIMAX STACK
  M:	Inaky Perez-Gonzalez <[email protected]>
  M:	[email protected]
drivers/watchdog/Kconfig +9 -0
index 4f0e7be..b5b1353 100644
@@ -711,6 +711,15 @@ config ALIM7101_WDT
 
  	  Most people will say N.
 
+config EBC_C386_WDT
+	tristate "WinSystems EBC-C384 Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	help
+	  Enables watchdog timer support for the watchdog timer on the
+	  WinSystems EBC-C384 motherboard. The timeout may be configured via
+	  the timeout module parameter.
+
  config F71808E_WDT
  	tristate "Fintek F71808E, F71862FG, F71869, F71882FG and F71889FG Watchdog"
  	depends on X86
drivers/watchdog/Makefile +1 -0
index f566753..1522316 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
  obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
  obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
  obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_EBC_C386_WDT) += ebc-c384_wdt.o
  obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
  obj-$(CONFIG_SP5100_TCO) += sp5100_tco.o
  obj-$(CONFIG_GEODE_WDT) += geodewdt.o
drivers/watchdog/ebc-c384_wdt.c new file +19 -0
new file mode 100644
index 0000000..9166b02
@@ -0,0 +1,187 @@
+/*
+ * Watchdog timer driver for the WinSystems EBC-C384
+ * Copyright (C) 2016 William Breathitt Gray
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+#include <linux/device.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>

drivers/watchdog/ebc-c384_wdt.c line 29

Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10 -0
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#define MODULE_NAME "ebc-c384_wdt"
+#define WATCHDOG_TIMEOUT 60
+/* Since the timeout value in minutes must fit in a single byte when sent to the
+ * watchdog timer, the maximum timeout possible is 15300 (255 * 60) seconds
+ */

/*
* Please use standard multi-line comments.
*/

drivers/watchdog/ebc-c384_wdt.c line 34

Quoted 5 lines
drivers/watchdog/ebc-c384_wdt.c new file +5 -0
+#define WATCHDOG_MAX_TIMEOUT 15300
+#define BASE_ADDR 0x564
+#define ADDR_EXTENT 5
+#define CFG_ADDR (BASE_ADDR + 1)
+#define PET_ADDR (BASE_ADDR + 2)

Please use tabs before the values for alignment.

Quoted 78 lines
drivers/watchdog/ebc-c384_wdt.c new file +78 -0
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static unsigned timeout;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds (default="
+	__MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+static int ebc_c384_wdt_start(struct watchdog_device *wdev)
+{
+	unsigned t = wdev->timeout;
+
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255)
+		t /= 60;
+
+	outb(t, PET_ADDR);
+
+	return 0;
+}
+
+static int ebc_c384_wdt_stop(struct watchdog_device *wdev)
+{
+	outb(0x00, PET_ADDR);
+
+	return 0;
+}
+
+static int ebc_c384_wdt_set_timeout(struct watchdog_device *wdev, unsigned t)
+{
+	/* resolution is in minutes for timeouts greater than 255 seconds */
+	if (t > 255) {
+		/* truncate second resolution to minute resolution */
+		t /= 60;
+		wdev->timeout = t * 60;
+
+		/* set watchdog timer for minutes */
+		outb(0x00, CFG_ADDR);
+	} else {
+		wdev->timeout = t;
+
+		/* set watchdog timer for seconds */
+		outb(0x80, CFG_ADDR);
+	}
+
+	return 0;
+}
+
+static const struct watchdog_ops ebc_c384_wdt_ops = {
+	.start = ebc_c384_wdt_start,
+	.stop = ebc_c384_wdt_stop,
+	.set_timeout = ebc_c384_wdt_set_timeout
+};
+
+static const struct watchdog_info ebc_c384_wdt_info = {
+	.options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT,
+	.identity = MODULE_NAME
+};
+
+static int __init ebc_c384_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdd;
+
+	if (!devm_request_region(dev, BASE_ADDR, ADDR_EXTENT, dev_name(dev))) {
+		dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
+			BASE_ADDR, BASE_ADDR + ADDR_EXTENT);
+		return -EBUSY;
+	}
+
+	wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
+	if (!wdd)
+		return -ENOMEM;
+
+	wdd->info = &ebc_c384_wdt_info;

drivers/watchdog/ebc-c384_wdt.c line 122

Quoted 10 lines
drivers/watchdog/ebc-c384_wdt.c new file +10 -0
+	wdd->ops = &ebc_c384_wdt_ops;
+	wdd->timeout = WATCHDOG_TIMEOUT;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
+
+	watchdog_set_nowayout(wdd, nowayout);
+
+	if (watchdog_init_timeout(wdd, timeout, dev))
+		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
+			timeout, WATCHDOG_TIMEOUT);

Multi-line alignment is off by one character.

Quoted 66 lines
drivers/watchdog/ebc-c384_wdt.c new file +65 -0
+
+	platform_set_drvdata(pdev, wdd);
+
+	return watchdog_register_device(wdd);
+}
+
+static int ebc_c384_wdt_remove(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(wdd);
+
+	return 0;
+}
+
+static struct platform_driver ebc_c384_wdt_driver = {
+	.driver = {
+		.name = MODULE_NAME
+	},
+	.remove = ebc_c384_wdt_remove
+};
+
+static struct platform_device *ebc_c384_wdt_device;
+
+static int __init ebc_c384_wdt_init(void)
+{
+	int err;
+
+	if (!dmi_match(DMI_BOARD_NAME, "EBC-C384 SBC"))
+		return -ENODEV;
+
+	ebc_c384_wdt_device = platform_device_alloc(MODULE_NAME, -1);
+	if (!ebc_c384_wdt_device)
+		return -ENOMEM;
+
+	err = platform_device_add(ebc_c384_wdt_device);
+	if (err)
+		goto err_platform_device;
+
+	err = platform_driver_probe(&ebc_c384_wdt_driver, ebc_c384_wdt_probe);
+	if (err)
+		goto err_platform_driver;
+
+	return 0;
+
+err_platform_driver:
+	platform_device_del(ebc_c384_wdt_device);
+err_platform_device:
+	platform_device_put(ebc_c384_wdt_device);
+	return err;
+}
+
+static void __exit ebc_c384_wdt_exit(void)
+{
+	platform_device_unregister(ebc_c384_wdt_device);
+	platform_driver_unregister(&ebc_c384_wdt_driver);
+}
+
+module_init(ebc_c384_wdt_init);
+module_exit(ebc_c384_wdt_exit);
+
+MODULE_AUTHOR("William Breathitt Gray <[email protected]>");
+MODULE_DESCRIPTION("WinSystems EBC-C384 watchdog timer driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" MODULE_NAME);
#16 lore raw
In reply to William Breathitt Gray

drivers/watchdog/Kconfig config ALIM7101_WDT · line 721

Quoted 11 lines · William Breathitt Gray
drivers/watchdog/Kconfig +8 -0
 
+config EBC_C386_WDT
+	tristate "WinSystems EBC-C384 Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	help
+	  Enables watchdog timer support for the watchdog timer on the
+	  WinSystems EBC-C384 motherboard. The timeout may be configured via
+	  the timeout module parameter.

It's utterly trivial, but I couldn't help noticing that this supports the "EBC-C384" motherboard but the Kconfig symbol is EBC_C386_WDT (note the 6). Any particular reason?

Thanks,

Paul Bolle

#17 lore raw
Quoted 4 lines · Paul Bolle
Quoted 2 lines · William Breathitt Gray
drivers/watchdog/Kconfig +0 -0
Quoted 12 lines

drivers/watchdog/Kconfig config ALIM7101_WDT · line 721

Quoted 8 lines
drivers/watchdog/Kconfig +8 -0
+config EBC_C386_WDT
+	tristate "WinSystems EBC-C384 Watchdog Timer"
+	depends on X86
+	select WATCHDOG_CORE
+	help
+	  Enables watchdog timer support for the watchdog timer on the
+	  WinSystems EBC-C384 motherboard. The timeout may be configured via
+	  the timeout module parameter.

It's utterly trivial, but I couldn't help noticing that this supports the "EBC-C384" motherboard but the Kconfig symbol is EBC_C386_WDT (note the 6). Any particular reason?

Looks like you caught a typo of mine! I'll fix it in my next patch. Many thanks Paul for your good eye.

William Breathitt Gray