login
Search
Acquire the I/O region for the Super I/O chip while we're working on it.
Further alter the way multiple Super I/O addresses are probed for chips
such that errors in the probing process are passed on from the module
initialisation function.
Some code cleanup: properly using, previously defined, functions rather
than duplicating their code.
---
drivers/hwmon/f71882fg.c | 38 +++++++++++++++++++++++---------------
1 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 4230729..25e1cad 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -856,10 +856,8 @@ static inline int superio_inb(int base, int reg)
static int superio_inw(int base, int reg)
{
int val;
- outb(reg++, base);
- val = inb(base + 1) << 8;
- outb(reg, base);
- val |= inb(base + 1);
+ val = superio_inb(base, reg) << 8;
+ val |= superio_inb(base, reg + 1);
return val;
}
@@ -905,10 +903,8 @@ static u16 f71882fg_read16(struct f71882fg_data *data, u8 reg)
{
u16 val;
- outb(reg++, data->addr + ADDR_REG_OFFSET);
- val = inb(data->addr + DATA_REG_OFFSET) << 8;
- outb(reg, data->addr + ADDR_REG_OFFSET);
- val |= inb(data->addr + DATA_REG_OFFSET);
+ val = f71882fg_read8(data, reg) << 8;
+ val |= f71882fg_read8(data, reg + 1);
return val;
}
@@ -921,10 +917,8 @@ static void f71882fg_write8(struct f71882fg_data *data, u8 reg, u8 val)
static void f71882fg_write16(struct f71882fg_data *data, u8 reg, u16 val)
{
- outb(reg++, data->addr + ADDR_REG_OFFSET);
- outb(val >> 8, data->addr + DATA_REG_OFFSET);
- outb(reg, data->addr + ADDR_REG_OFFSET);
- outb(val & 255, data->addr + DATA_REG_OFFSET);
+ f71882fg_write8(data, reg, val >> 8);
+ f71882fg_write8(data, reg + 1, val & 0xff);
}
static u16 f71882fg_read_temp(struct f71882fg_data *data, int nr)
@@ -2184,6 +2178,13 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
int err = -ENODEV;
u16 devid;
+ /* Don't ...Oh yes, this in preparation for adding watchdog support for the Fintek F71808E (very similar to the F71889) to this driver. I'll be sending in a patch for adding sensor hardware monitor support for the F71808E first (i.e. before the addition of watchdog support). PS I'm using the watchdog API as described in Documentation/watchdog/watchdog-api.txt. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Allow device probing to recognise the Fintek F71808E.
Sysfs interface:
* Fan/pwm control is the same as for F71889FG
* Temperature and voltage sensor handling is largely the same as for
the F71889FG
- Has one temperature sensor less (doesn't have temp3)
- Misses one voltage sensor (doesn't have V6, thus in6_input refers to
what in7_input refers for F71889FG)
For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
such that it can largely be reused.
---
Documentation/hwmon/f71882fg | 4 ++
drivers/hwmon/Kconfig | 6 ++--
drivers/hwmon/f71882fg.c | 80 +++++++++++++++++++++++++++++++++++++----
3 files changed, 79 insertions(+), 11 deletions(-)
diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
index a7952c2..1a07fd6 100644
--- a/Documentation/hwmon/f71882fg
+++ b/Documentation/hwmon/f71882fg
@@ -2,6 +2,10 @@ Kernel driver f71882fg
======================
Supported chips:
+ * Fintek F71808E
+ Prefix: 'f71808fg'
+ Addresses scanned: none, address read from Super I/O config space
+ Datasheet: Not public
* Fintek F71858FG
Prefix: 'f71858fg'
Addresses scanned: none, address read from Super I/O config space
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e4595e6..7053608 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -332,11 +332,11 @@ config SENSORS_F71805F
will be called f71805f.
config SENSORS_F71882FG
- tristate "Fintek F71858FG, F71862FG, F71882FG, F71889FG and F8000"
+ tristate "Fintek F71808E, F71858FG, F71862FG, F71882FG, F71889FG and F8000"
depends on EXPERIMENTAL
help
- If you say yes here you get support for hardware monitoring
- features of the Fintek F71858FG, F71862FG/71863FG, F71882FG/F71883FG,
+ If you say yes here you get support for hardware monitoring features
+ of the Fintek F71808E, F71858FG, F71862FG/71863FG, F71882FG/F71883FG,
F71889FG and F8000 Super-I/O chips.
This driver can also ...Rename the `address' variable that's used in some places to hwmon_addr
to indicate it refers to the hardware monitor (HWMON in datasheet)
"logical device" of the chip.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index b290b87..7b31e14 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2233,7 +2233,7 @@ static int f71882fg_remove(struct platform_device *pdev)
return 0;
}
-static int __init f71882fg_find(int sioaddr, unsigned short *address,
+static int __init f71882fg_find(int sioaddr, unsigned short *hwmon_addr,
struct f71882fg_sio_data *sio_data)
{
int err = -ENODEV;
@@ -2290,16 +2290,16 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
goto exit;
}
- *address = superio_inw(sioaddr, SIO_REG_ADDR);
- if (*address == 0) {
+ *hwmon_addr = superio_inw(sioaddr, SIO_REG_ADDR);
+ if (*hwmon_addr == 0) {
printk(KERN_WARNING DRVNAME ": Base address not set\n");
goto exit;
}
- *address &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */
+ *hwmon_addr &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */
err = 0;
printk(KERN_INFO DRVNAME ": Found %s chip at %#x, revision %d\n",
- f71882fg_names[sio_data->type], (unsigned int)*address,
+ f71882fg_names[sio_data->type], (unsigned int)*hwmon_addr,
(int)superio_inb(sioaddr, SIO_REG_DEVREV));
exit:
superio_exit(sioaddr);
@@ -2307,17 +2307,17 @@ exit:
return err;
}
-static int __init f71882fg_device_add(unsigned short address,
+static int __init f71882fg_device_add(unsigned short hwmon_addr,
const struct f71882fg_sio_data *sio_data)
{
struct resource res = {
- .start = address,
- .end = address + REGION_LENGTH - 1,
+ .start = hwmon_addr,
+ .end = hwmon_addr + REGION_LENGTH - 1,
.flags = IORESOURCE_IO,
};
int err;
- f71882fg_pdev = ...Implement the watchdog API for the Fintek F71808E. Signed-off-by: Giel van Schijndel <me@mortis.eu> --- drivers/hwmon/f71882fg.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 553 insertions(+), 0 deletions(-) diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c index 8006271..3604613 100644 --- a/drivers/hwmon/f71882fg.c +++ b/drivers/hwmon/f71882fg.c @@ -26,14 +26,19 @@ #include <linux/hwmon.h> #include <linux/hwmon-sysfs.h> #include <linux/err.h> +#include <linux/miscdevice.h> #include <linux/mutex.h> +#include <linux/notifier.h> #include <linux/io.h> #include <linux/acpi.h> +#include <linux/reboot.h> +#include <linux/watchdog.h> #define DRVNAME "f71882fg" #define SIO_F71858FG_LD_HWM 0x02 /* Hardware monitor logical device */ #define SIO_F71882FG_LD_HWM 0x04 /* Hardware monitor logical device */ +#define SIO_F71808FG_LD_WDT 0x07 /* Watchdog timer logical device */ #define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ #define SIO_LOCK_KEY 0xAA /* Key to diasble Super-I/O */ @@ -91,12 +96,52 @@ #define F71882FG_REG_START 0x01 +#define F71808FG_REG_WDO_CONF 0xf0 +#define F71808FG_REG_WDT_CONF 0xf5 +#define F71808FG_REG_WD_TIME 0xf6 + +#define F71808FG_FLAG_WDOUT_EN 7 + +#define F71808FG_FLAG_WDTMOUT_STS 5 +#define F71808FG_FLAG_WD_EN 5 +#define F71808FG_FLAG_WD_PULSE 4 +#define F71808FG_FLAG_WD_UNIT 3 + #define FAN_MIN_DETECT 366 /* Lowest detectable fanspeed */ +/* Default values */ +#define WATCHDOG_TIMEOUT 60 /* 1 minute default timeout */ +#define WATCHDOG_MAX_TIMEOUT (60 * 255) +#define WATCHDOG_PULSE_WIDTH 125 /* 125 ms, default pulse width for + watchdog signal */ + static unsigned short force_id; module_param(force_id, ushort, 0); MODULE_PARM_DESC(force_id, "Override the detected device ID"); +static const int max_timeout = WATCHDOG_MAX_TIMEOUT; +static int timeout = 60; /* default timeout in seconds */ +module_param(timeout, int, ...
Looking at the F71889FG datasheet I think implementing watchdog support for that chip can be accomplished rather easily. I think it's as simple as adding the pin initialisation (multi function select registers 0x2a and 0x2b) sequence to the switch statement in watchdog_start(). That being said; I don't have physical access to that chip myself, thus I couldn't actually test it (unlike the F71808E, for which I've already tested these patches). -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Hi, Nack: As the watchdog has its own SIO logical device number, it should have a separate driver, not have support glued to the hwmon driver. Regards, Hans --
Thus, if I understand correctly, you would suggest for me to implement a new driver in drivers/watchdog/ to implement this driver? -- Met vriendelijke groet, With kind regards, Giel van Schijndel
So I've now started out working on moving the watchdog code to a separate driver. I/O port conflict However, this driver would have to lock the SIO port range as well. Unlike the hardware monitor, however, the watchdog (VID controller on some datasheets) doesn't appear to provide for an alternative I/O port mapping. Meaning the wathdog driver would have to maintain a permanent hold on the SIO port range. This would thus interfere with the operation of the f71882fg driver. I.e. it would prevent the device probing stage from working, thus preventing it from loading *after* my in-development watchdog driver. Alternative logical device port mapping Regarding address mapping: the 16 bit SIO_REG_ADDR(0x60) register does exist for this logical device, though according to the datasheet it is set to 0x0000 by default. Reading the value from hardware it claims to be mapped on 0x0AE0, trying to read from that port range (using f71882fg_read8) yields nothing more than 0xFF for any register mentioned in the datasheet. Hardware monitor alternate port range Further, when looking at the way that SIO_REG_ADDR is currently used by For the F71808E and F71889, which both have 0x295, for hardware monitor base address that code ^^ combined with the addition of ADDR_REG_OFFSET and DATA_REG_OFFSET (see f71882fg_(read|write)8) all of this basically amounts to mangling 0x295 -> 0x290 -> (0x295,0x296). So my question: is there any particular reason for performing this address mangling? Mostly: is there anything there that I should try replicating in order to get mapping of the watchdog device on an alternate port range working? PS Perhaps this would be easier to converse about if you had the datasheet? -- Met vriendelijke groet, With kind regards, Giel van Schijndel
There are two ways to deal with that really 1. Add a multi-function driver - it finds the chip and claims the port regions and then provides methods for locked access to them as well as creating other device instances that the drivers map to (probably platform devices ?) which in turn trigger the loading/binding of the relevant low level devices. 2. Fix the kernel request_resource stuff to support a sleeping non exclusive resource so request/free of regions can be used as a resource semaphore by co-operative devices. #2 is actually not hard but when I did the patch originally it then wasn't needed by the driver I had in mind for other reasons. See http://groups.google.com/group/linux.kernel/msg/1425fc2aad32e6ea Maybe its worth resurrecting ? Alan --
Hi, Or, a bit more specific solution would be to resurrect the superio lock coordinator patches, which were written (but never merged) 2 years ago to solve exactly this problem: http://lists.lm-sensors.org/pipermail/lm-sensors/2008-July/023743.html Regards, Hans --
When performing some searches I find messages going back to at least september 2006 [1] [2]. With multiple occurences of these patches being "dusted off". They never got applied though, and for that (*not* applying them) I cannot find any reason. Is there any? Or did people just become uninterested and let the patches "collect dust"? Then regarding Alan's patch. The fact that it is a *lot* simpler than Jim Cromie's SuperIO locks coordinator is IMHO a significant advantage over the latter. Furthermore, "lock coordinator" seems like a bad name to me, since those patches seem especially concerned with centralising the way that SuperIO devices are probed for. Thus if the only thing required is to serialize resource access I think plain-ol' locking (with the ability to block on the lock, rather than polling for it). [1] http://lists.lm-sensors.org/pipermail/lm-sensors/2006-June/016476.html [2] http://lkml.org/lkml/2006/9/14/20 -- Met vriendelijke groet, With kind regards, Giel van Schijndel
For my part, I started seeing difficulties in the centralized probing, esp around the unlocking sequences; stuff thats device specific, but wanted to be hidden in the centralized probe. When it was just byte-sequences, "coordinator" was meant to imply cooperative drivers, though thats *always* the case, in that drivers would at least --
In this particular case I most definitely think it's worth resurrecting. Using your patch (I had to change the IORESOURCE_MUXED constant's value though) I can completely solve the I/O sharing issue for the f71882fg + watchdog case with only a single line modification to the f71882fg driver. One question regarding your patch though: how will it function when driver (A) locks an I/O region using request_region() then driver (B) comes along and tries to lock it using request_muxed_region()? One problem I imagine might occur is that driver (A)'s 'struct resource' doesn't have the IORESOURCE_MUXED flags set, thus simply won't wake up driver (B) on the 'muxed_resource_wait' queue. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Using Alan's patch and the one following the dashed line I can implement
(and have already done so) a separate watchdog driver for the Fintek
F71808E.
Should I just ask for this patch to be applied together with Alan's?
Then submit the new driver? (I'm a bit new to the non-technical aspects
of Linux kernel development).
========================================================================
hwmon: f71882fg: use a muxed resource lock for the Super I/O port
Sleep while acquiring a resource lock on the Super I/O port. This should
prevent collisions from causing the hardware probe to fail with -EBUSY.
---
drivers/hwmon/f71882fg.c | 28 +++++++++++++++-------------
1 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 7857ed3..e09416d 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -113,7 +113,7 @@ static struct platform_device *f71882fg_pdev;
/* Super-I/O Function prototypes */
static inline int superio_inb(int base, int reg);
static inline int superio_inw(int base, int reg);
-static inline void superio_enter(int base);
+static inline int superio_enter(int base);
static inline void superio_select(int base, int ld);
static inline void superio_exit(int base);
@@ -883,11 +883,20 @@ static int superio_inw(int base, int reg)
return val;
}
-static inline void superio_enter(int base)
+static inline int superio_enter(int base)
{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ (int)base);
+ return -EBUSY;
+ }
+
/* according to the datasheet the key must be send twice! */
outb(SIO_UNLOCK_KEY, base);
outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
}
static inline void superio_select(int base, int ld)
@@ -899,6 +908,7 @@ static inline void superio_select(int base, int ld)
static inline void superio_exit(int base)
{
...I'd expect to see a submission of three patches I think 1. Patch adding the muxed resource support 2. Patch making the hwmon driver use it 3. Patch adding the new driver which needs it and then we have a few other bits of code that probably should adopt it, but that is a separate matter and other maintainers can do that bit. Alan --
Hi Giel, What Alan said :) Although I'm not sure what the proper place is to post this entire set, I guess linux-kernel itself, with lm_sensors in the CC. Regards, Hans --
From: Alan Cox <alan@lxorguk.ukuu.org.uk> This patch was originally written by Alan Cox [1]. I only updated it to apply correctly to Linus' current tree and changed IORESOURCE_MUXED's value from 0x00200000 to 0x00400000 because the former has already been taken in use since. [1] https://patchwork.kernel.org/patch/32397/ Patch after this line: ======================================================================== SuperIO devices share regions and use lock/unlock operations to chip select. We therefore need to be able to request a resource and wait for it to be freed by whichever other SuperIO device currently hogs it. Right now you have to poll which is horrible. Add a MUXED field to IO port resources. If the MUXED field is set on the resource and on the request (via request_muxed_region) then we block until the previous owner of the muxed resource releases their region. This allows us to implement proper resource sharing and locking for superio chips using code of the form enable_my_superio_dev() { request_muxed_region(0x44, 0x02, "superio:watchdog"); outb() ..sequence to enable chip } disable_my_superio_dev() { outb() .. sequence of disable chip release_region(0x44, 0x02); } Signed-off-by: Giel van Schijndel <me@mortis.eu> --- include/linux/ioport.h | 4 +++- kernel/resource.c | 14 +++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 71ab79d..604fd29 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -52,6 +52,7 @@ struct resource_list { #define IORESOURCE_MEM_64 0x00100000 #define IORESOURCE_WINDOW 0x00200000 /* forwarded by bridge */ +#define IORESOURCE_MUXED 0x00400000 /* Resource is software muxed */ #define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map this resource */ #define IORESOURCE_DISABLED 0x10000000 @@ -141,7 +142,8 @@ static inline unsigned long resource_type(const struct resource *res) } /* ...
Add a new watchdog driver for the Fintek F71808E Super I/O chip. --- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/f71808e_wdt.c | 726 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 738 insertions(+), 0 deletions(-) create mode 100644 drivers/watchdog/f71808e_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index bdcdbd5..653896f 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -382,6 +382,17 @@ config ALIM7101_WDT Most people will say N. +config F71808E_WDT + tristate "Fintek F71808E Watchdog" + depends on X86 && EXPERIMENTAL + help + This is the driver for the hardware watchdog on the Fintek + F71808E Super I/O chip. + + You can compile this driver directly into the kernel, or use + it as a module. The module will be called f71808e_wdt. + + config GEODE_WDT tristate "AMD Geode CS5535/CS5536 Watchdog" depends on CS5535_MFGPT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5e3cb95..e2f308d 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -65,6 +65,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_F71808E_WDT) += f71808e_wdt.o obj-$(CONFIG_GEODE_WDT) += geodewdt.o obj-$(CONFIG_SC520_WDT) += sc520_wdt.o obj-$(CONFIG_SBC_FITPC2_WATCHDOG) += sbc_fitpc2_wdt.o diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c new file mode 100644 index 0000000..7d547a9 --- /dev/null +++ b/drivers/watchdog/f71808e_wdt.c @@ -0,0 +1,726 @@ +/*************************************************************************** + * Copyright (C) 2006 by Hans Edgington <hans@edgington.nl> * + * Copyright (C) 2007-2009 Hans de Goede <hdegoede@redhat.com> * + * Copyright (C) 2010 Giel van Schijndel ...
Updated patch: * fixes a bug where detection wouldn't function properly if a non-Fintek device was found (or a wrong Super I/O port chosen). * Add support for the F71882FG NOTE: This patch depends on patch [1] (applied in [2]) and patch [3]. [1] https://patchwork.kernel.org/patch/89023/ [2] http://git.kernel.org/?p=linux/kernel/git/jbarnes/pci-2.6.git;a=commit;h=316607809324c... [3] https://patchwork.kernel.org/patch/88235/ Patch after this line: ======================================================================== [RFC] watchdog: f71808e_wdt: new watchdog driver for Fintek F71808E Add a new watchdog driver for the Fintek F71808E Super I/O chip. --- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/f71808e_wdt.c | 767 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 779 insertions(+), 0 deletions(-) create mode 100644 drivers/watchdog/f71808e_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 050ee14..d973105 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -366,6 +366,17 @@ config ALIM7101_WDT Most people will say N. +config F71808E_WDT + tristate "Fintek F71808E Watchdog" + depends on X86 && EXPERIMENTAL + help + This is the driver for the hardware watchdog on the Fintek + F71808E Super I/O chip. + + You can compile this driver directly into the kernel, or use + it as a module. The module will be called f71808e_wdt. + + config GEODE_WDT tristate "AMD Geode CS5535/CS5536 Watchdog" depends on CS5535_MFGPT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 475c611..6f0f3d9 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -64,6 +64,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_F71808E_WDT) += ...
Driver looks OK to me. Can't apply it before the I/O resource sharing is in place. How do you want to proceed? Kind regards, Wim. --
I would think applying it to watchdog-next to apply it for the next kernel release (similar to Jean Delvare's linux-next). -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Will add it after 2.6.35-rc1 is out then. Kind regards, Wim. --
Sending a reminder, as 2.6.35-rc1 has been tagged for a while now. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Sleep while acquiring a resource lock on the Super I/O port. This should
prevent collisions from causing the hardware probe to fail with -EBUSY.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 28 +++++++++++++++-------------
1 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 7857ed3..e09416d 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -113,7 +113,7 @@ static struct platform_device *f71882fg_pdev;
/* Super-I/O Function prototypes */
static inline int superio_inb(int base, int reg);
static inline int superio_inw(int base, int reg);
-static inline void superio_enter(int base);
+static inline int superio_enter(int base);
static inline void superio_select(int base, int ld);
static inline void superio_exit(int base);
@@ -883,11 +883,20 @@ static int superio_inw(int base, int reg)
return val;
}
-static inline void superio_enter(int base)
+static inline int superio_enter(int base)
{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ (int)base);
+ return -EBUSY;
+ }
+
/* according to the datasheet the key must be send twice! */
outb(SIO_UNLOCK_KEY, base);
outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
}
static inline void superio_select(int base, int ld)
@@ -899,6 +908,7 @@ static inline void superio_select(int base, int ld)
static inline void superio_exit(int base)
{
outb(SIO_LOCK_KEY, base);
+ release_region(base, 2);
}
static inline int fan_from_reg(u16 reg)
@@ -2239,17 +2249,10 @@ static int f71882fg_remove(struct platform_device *pdev)
static int __init f71882fg_find(int sioaddr, unsigned short *address,
struct f71882fg_sio_data *sio_data)
{
- int err = -ENODEV;
u16 devid;
-
- /* Don't step on other drivers' I/O space by accident */
- if ...Hi, Looks good to me (assuming the previous patch in the series gets applied). Acked-by: Hans de Goede <hdegoede@redhat.com> Regards, --
Fix a bug which caused f71882fg_find() to pretend to be succesfull on
Super I/O ports which didn't have a Fintek chip attached. This was
caused by returning 0 instead of -ENODEV, adding several 'err = -ENODEV'
statements preceding the 'goto exit' statements fixed this.
Patch follows this line:
========================================================================
Sleep while acquiring a resource lock on the Super I/O port. This should
prevent collisions from causing the hardware probe to fail with -EBUSY.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 7857ed3..85512a7 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -113,7 +113,7 @@ static struct platform_device *f71882fg_pdev;
/* Super-I/O Function prototypes */
static inline int superio_inb(int base, int reg);
static inline int superio_inw(int base, int reg);
-static inline void superio_enter(int base);
+static inline int superio_enter(int base);
static inline void superio_select(int base, int ld);
static inline void superio_exit(int base);
@@ -883,11 +883,20 @@ static int superio_inw(int base, int reg)
return val;
}
-static inline void superio_enter(int base)
+static inline int superio_enter(int base)
{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ (int)base);
+ return -EBUSY;
+ }
+
/* according to the datasheet the key must be send twice! */
outb(SIO_UNLOCK_KEY, base);
outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
}
static inline void superio_select(int base, int ld)
@@ -899,6 +908,7 @@ static inline void superio_select(int base, int ld)
static inline void superio_exit(int base)
{
outb(SIO_LOCK_KEY, ...All of this patch's dependencies now reside in mainline, so I'd like to "renew" the request for applying it. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
On Thu, 25 Mar 2010 14:17:41 +0100 needs a set_current_state(TASK_UNINTERRUPTIBLE); here (error from my original test patch that I noticed re-reviewing it) --
Always fun reviewing your own code ;-)
New patch attached.
One question though. I did some reading in
Documentation/memory-barriers.txt and think it is correct *not* to call
set_current_state(TASK_INTERRUPTIBLE) after schedule() finishes. Could
you confirm or deny that?
Patch after this line:
resource: shared I/O region support
SuperIO devices share regions and use lock/unlock operations to chip
select. We therefore need to be able to request a resource and wait for
it to be freed by whichever other SuperIO device currently hogs it.
Right now you have to poll which is horrible.
Add a MUXED field to IO port resources. If the MUXED field is set on the
resource and on the request (via request_muxed_region) then we block
until the previous owner of the muxed resource releases their region.
This allows us to implement proper resource sharing and locking for
superio chips using code of the form
enable_my_superio_dev() {
request_muxed_region(0x44, 0x02, "superio:watchdog");
outb() ..sequence to enable chip
}
disable_my_superio_dev() {
outb() .. sequence of disable chip
release_region(0x44, 0x02);
}
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
include/linux/ioport.h | 4 +++-
kernel/resource.c | 16 +++++++++++++++-
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 71ab79d..604fd29 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -52,6 +52,7 @@ struct resource_list {
#define IORESOURCE_MEM_64 0x00100000
#define IORESOURCE_WINDOW 0x00200000 /* forwarded by bridge */
+#define IORESOURCE_MUXED 0x00400000 /* Resource is software muxed */
#define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map this resource */
#define IORESOURCE_DISABLED 0x10000000
@@ -141,7 +142,8 @@ static inline unsigned long resource_type(const struct resource *res)
}
/* Convenience shorthand with allocation */
-#define ...After the schedule() returns we will be in TASK_RUNNING which is where we And you'll want a -- -- "Alan, I'm getting a bit worried about you." -- Linus Torvalds --
Yes, thanks. Anyone specific I should mail to get this patch applied? I don't see any entry in MAINTAINERS for kernel/resource.c or include/linux/resource.h. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
On Mon, 29 Mar 2010 10:18:34 +0200 I can take it; sometimes Linus applies resource stuff directly though. Can you resend with the appropriate s-o-bs etc? Thanks, -- Jesse Barnes, Intel Open Source Technology Center --
Sure.
(I copied Alan's signed-off-by from his reply to the patch)
Patch after this line:
========================================================================
resource: shared I/O region support
SuperIO devices share regions and use lock/unlock operations to chip
select. We therefore need to be able to request a resource and wait for
it to be freed by whichever other SuperIO device currently hogs it.
Right now you have to poll which is horrible.
Add a MUXED field to IO port resources. If the MUXED field is set on the
resource and on the request (via request_muxed_region) then we block
until the previous owner of the muxed resource releases their region.
This allows us to implement proper resource sharing and locking for
superio chips using code of the form
enable_my_superio_dev() {
request_muxed_region(0x44, 0x02, "superio:watchdog");
outb() ..sequence to enable chip
}
disable_my_superio_dev() {
outb() .. sequence of disable chip
release_region(0x44, 0x02);
}
Signed-off-by: Giel van Schijndel <me@mortis.eu>
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
include/linux/ioport.h | 4 +++-
kernel/resource.c | 16 +++++++++++++++-
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 71ab79d..604fd29 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -52,6 +52,7 @@ struct resource_list {
#define IORESOURCE_MEM_64 0x00100000
#define IORESOURCE_WINDOW 0x00200000 /* forwarded by bridge */
+#define IORESOURCE_MUXED 0x00400000 /* Resource is software muxed */
#define IORESOURCE_EXCLUSIVE 0x08000000 /* Userland may not map this resource */
#define IORESOURCE_DISABLED 0x10000000
@@ -141,7 +142,8 @@ static inline unsigned long resource_type(const struct resource *res)
}
/* Convenience shorthand with allocation */
-#define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0)
+#define ...-- Met vriendelijke groet, With kind regards, Giel van Schijndel
I have to question this approach a bit. I would much rather see this as a two-step process, where multiple devices request the same region with a "sharable" flag, and then have a mutex associated with the struct resource (perhaps we need an outer container called "struct muxed_resource" or some such.) What I *really* object to with this patch is that it inherently assumes that there is only one multiplexed resource in the entire system... but of course nowhere enforces that. -hpa --
On Mon, 29 Mar 2010 10:45:35 -0700 Well that does keep it simple, and with just one user that's probably best. But why not use the common bus driver method? Muxing at the resource level only seems to solve part of the problem... It doesn't guarantee for example that driver A does something to a shared region that breaks driver B; it just makes sure they don't access the same region at the same time. -- Jesse Barnes, Intel Open Source Technology Center --
The common bus driver method is the obvious thing to do, but it would presumably be greatly helped by librarization. -hpa --
The obvious reason for not doing that kind of grand over-engineering is that you are assuming the devices involved are remotely related. On quite a few systems we have a collection of superio config interfaces on random low ports all with their own lock/unlock rituals. They range from parallel devices to watchdogs and god knows what else. Right now we have various bits of driver code (parport is a good one) that exist on a cross fingers, pray and poke model. It would be nice to fix that. For most super I/O devices the muxing is basically a glorified chip select line. There isn't any structure to impose over it. Where you have structure there are better ways to do it, but one does not exclude the other. Alan --
On Mon, 29 Mar 2010 19:29:57 +0100 Well I'm not sure such over-engineering would be "grand", but it does seem like overkill for the devices you're covering here. At any rate, the patch is in my linux-next tree, so it'll head to Linus next merge cycle unless some big new objections arise. -- Jesse Barnes, Intel Open Source Technology Center --
The patch does nothing of the sort. Not unless there is a bug I am not seeing anyway. It does assume nobody tries to grab pairs of such resources as it doesn't do deadlock avoidance. It's now a shared resource patch however, its a multiplexor patch and that is precisely why it is called MUX not SHARED or OVERLAY Alan --
Sorry, I missed the "continue", which of course handles the situation I was worried about. The shared wait queue is a bit inelegant, but if it turns out to be a bottleneck in real life then we either have bigger problems or it can be addressed at that time. -hpa --
On Mon, 29 Mar 2010 19:38:00 +0200 Applied to my for-linus branch with Alan as author. Thanks, -- Jesse Barnes, Intel Open Source Technology Center --
On Mon, 29 Mar 2010 19:38:00 +0200 ...looking again I see it should be in my linux-next branch instead, I'll move it over. Thanks, -- Jesse Barnes, Intel Open Source Technology Center --
Factor out code for detecting the hardware monitor into a separate
function. Then add a function for detecting the watchdog.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 74 ++++++++++++++++++++++++++++++++++++----------
1 files changed, 58 insertions(+), 16 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 7b31e14..8006271 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2233,6 +2233,54 @@ static int f71882fg_remove(struct platform_device *pdev)
return 0;
}
+static int __init f71882fg_find_watchdog(int sioaddr,
+ const struct f71882fg_sio_data *sio_data)
+{
+ switch (sio_data->type) {
+ case f71808fg:
+ break;
+
+ case f71862fg:
+ case f71882fg:
+ case f71889fg:
+ /* These have a watchdog, though it isn't implemented (yet). */
+ return -ENOSYS;
+
+ case f71858fg:
+ default:
+ /*
+ * Confirmed (by datasheet) not to have a watchdog. That is,
+ * except for chips matched by the 'default' label of course.
+ */
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static int __init f71882fg_find_hwmon(int sioaddr, unsigned short *hwmon_addr,
+ struct f71882fg_sio_data *sio_data)
+{
+ if (sio_data->type == f71858fg)
+ superio_select(sioaddr, SIO_F71858FG_LD_HWM);
+ else
+ superio_select(sioaddr, SIO_F71882FG_LD_HWM);
+
+ if (!(superio_inb(sioaddr, SIO_REG_ENABLE) & 0x01)) {
+ printk(KERN_WARNING DRVNAME ": Device not activated\n");
+ return -ENODEV;
+ }
+
+ *hwmon_addr = superio_inw(sioaddr, SIO_REG_ADDR);
+ if (*hwmon_addr == 0) {
+ printk(KERN_WARNING DRVNAME ": Base address not set\n");
+ return -ENODEV;
+ }
+ *hwmon_addr &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */
+
+ return 0;
+}
+
static int __init f71882fg_find(int sioaddr, unsigned short *hwmon_addr,
struct f71882fg_sio_data *sio_data)
{
@@ -2280,27 +2328,21 @@ static int __init f71882fg_find(int sioaddr, unsigned short *hwmon_addr,
goto exit;
}
- if (sio_data->type == ...Ack. Acked-by: Hans de Goede <hdegoede@redhat.com> --
Correction, nack on this one see me reply to patch 4/4 Regards, Hans --
Hi, See comments inline. Ugh, please don't fall through, and then have an if below to only do some parts of the case falling through. This is quite confusing at first I thought your code was buggy I had to read it twice to notice the if. Instead just duplicate the following lines: > + err = f71882fg_create_sysfs_files(pdev, > + fxxxx_temp_attr, > + ARRAY_SIZE(fxxxx_temp_attr)); In the f71862fg case, end the f71862fg case with a break and remove Regards, Hans --
Ack. New and improved patch follows this line.
========================================================================
hwmon: f71882fg: Add support for the Fintek F71808E
Allow device probing to recognise the Fintek F71808E.
Sysfs interface:
* Fan/pwm control is the same as for F71889FG
* Temperature and voltage sensor handling is largely the same as for
the F71889FG
- Has one temperature sensor less (doesn't have temp3)
- Misses one voltage sensor (doesn't have V6, thus in6_input refers to
what in7_input refers for F71889FG)
For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
such that it can largely be reused.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
Documentation/hwmon/f71882fg | 4 ++
drivers/hwmon/Kconfig | 6 ++--
drivers/hwmon/f71882fg.c | 83 ++++++++++++++++++++++++++++++++++++++----
3 files changed, 82 insertions(+), 11 deletions(-)
diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
index a7952c2..1a07fd6 100644
--- a/Documentation/hwmon/f71882fg
+++ b/Documentation/hwmon/f71882fg
@@ -2,6 +2,10 @@ Kernel driver f71882fg
======================
Supported chips:
+ * Fintek F71808E
+ Prefix: 'f71808fg'
+ Addresses scanned: none, address read from Super I/O config space
+ Datasheet: Not public
* Fintek F71858FG
Prefix: 'f71858fg'
Addresses scanned: none, address read from Super I/O config space
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e4595e6..7053608 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -332,11 +332,11 @@ config SENSORS_F71805F
will be called f71805f.
config SENSORS_F71882FG
- tristate "Fintek F71858FG, F71862FG, F71882FG, F71889FG and F8000"
+ tristate "Fintek F71808E, F71858FG, F71862FG, F71882FG, F71889FG and F8000"
depends on EXPERIMENTAL
help
- If you say yes here you get support for hardware monitoring
- features of the Fintek F71858FG, F71862FG/71863FG, ...Hi, This new version looks good to me: Acked-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans --
Thanks. Anyone else I need to poke in order to set this on track to mainline? -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Hi, No, Jean should have picked this up, I guess it has fallen through the cracks. I think it is probably best if you resend this patch and the watchdog patches re-based against the latest upstream rc, then I'll re-review them and ack them and Jean can then put them into his tree. Thanks & Regards, Hans --
Ack. I'll resend them as replies to this mail. -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Allow device probing to recognise the Fintek F71808E.
Sysfs interface:
* Fan/pwm control is the same as for F71889FG
* Temperature and voltage sensor handling is largely the same as for
the F71889FG
- Has one temperature sensor less (doesn't have temp3)
- Misses one voltage sensor (doesn't have V6, thus in6_input refers to
what in7_input refers for F71889FG)
For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
such that it can largely be reused.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
Documentation/hwmon/f71882fg | 4 ++
drivers/hwmon/Kconfig | 6 ++--
drivers/hwmon/f71882fg.c | 83 ++++++++++++++++++++++++++++++++++++++----
3 files changed, 82 insertions(+), 11 deletions(-)
diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
index a7952c2..1a07fd6 100644
--- a/Documentation/hwmon/f71882fg
+++ b/Documentation/hwmon/f71882fg
@@ -2,6 +2,10 @@ Kernel driver f71882fg
======================
Supported chips:
+ * Fintek F71808E
+ Prefix: 'f71808fg'
+ Addresses scanned: none, address read from Super I/O config space
+ Datasheet: Not public
* Fintek F71858FG
Prefix: 'f71858fg'
Addresses scanned: none, address read from Super I/O config space
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e19cf8e..7d0beac 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -332,11 +332,11 @@ config SENSORS_F71805F
will be called f71805f.
config SENSORS_F71882FG
- tristate "Fintek F71858FG, F71862FG, F71882FG, F71889FG and F8000"
+ tristate "Fintek F71808E, F71858FG, F71862FG, F71882FG, F71889FG and F8000"
depends on EXPERIMENTAL
help
- If you say yes here you get support for hardware monitoring
- features of the Fintek F71858FG, F71862FG/71863FG, F71882FG/F71883FG,
+ If you say yes here you get support for hardware monitoring features
+ of the Fintek F71808E, F71858FG, F71862FG/71863FG, F71882FG/F71883FG,
F71889FG ...Hi, I know I've reviewed this patch before, but now I have a datasheet so this time I've been a bit more thorough and I've found 2 small issues and 1 bigger one. Andrew can you please drop this patch from -mm until this is resolved? My datasheet strongly disagrees with this the F71889FG has 5 pwm zones each with their own speed divided by 4 boundary temps, where as the F71808E has 3 pwm zones divided by 2 boundary temps. So it is much more like the F71862FG, which also has 2 boundary temps, and 3 pwm zones, *but* the F71862FG has one pwm zone hardwired to 100%. So it looks like you need to create a new f71808e_auto_pwm_attr array esp. for this model, as well as a special case for reading the auto pwm attr in f71882fg_update_device. Also the auto pwm of the F71808E allows following of digital temps read to peci / amdsi / ibex rather then following a directly connected temp diode like the F71889FG, which the driver does not support, so you should check if this is enabled and if so disable the auto pwm attr entirely. Code for this is already in place for the F71889FG, There is a problem here though, the new fxxxx_temp_attr contains attributes for temp#_max_beep and temp#_crit_beep, but the F71808E lacks that function. So I think that the new fxxxx_temp_attr need to be split into fxxxx_temp_attr and fxxxx_temp_beep_attr, like is already done with fxxxx_fan_attr. Also while making changes, I must say I don't like the splitting of fxxxx_temp_attr into fxxxx_temp_attr and f71862_temp_attr just because the number of sensors differs. I think it would be better to instead make fxxxx_temp_attr a 2 dimensional array like fxxxx_fan_attr and like with fxxxx_fan_attr register as many sensor attr blocks as the specific This is wrong, as you already indicate and the datasheet as well this chip in question is an f71808e not an f71808fg, also note that there is an f71808a model as well which is different (and has a different super io chip id). One last request in the second ...
I'm assuming that by "pwm zone" you mean a separate PWM output channel?
I.e. each "pwm zone" controls a single fan?
Because in the datasheet I have for the F71889 only 3 fan controls are
Do you mean the checking of the FANx_TEMP_SEL_DIG flag in the fan's
"Temperature Mapping Select" registers currently done for the F71889 in
the second switch-statement inside f71882fg_probe? As that code is
Right, that's probably a nicer way of going about it, I think that might
be easier done in a separate patch (most likely preceding the addition
Ah yes, I think I, wrongly, assumed that 'fg' was just some suffix used
in this driver. For example, I cannot find F71889FG in the datasheet I
have, only 'F71889' along with 'F71889F' in the section "Ordering
Information" (for the F71889 I've got datasheet version V0.17P released
December 2008).
At the same time the F71808E datasheet I have clearly marks the chip as
F71808E all over the entire datasheet (version V0.17P released October
2009).
Either way I changed that ^^ portion of documentation while changing the
Wouldn't it be better, to instead replace that 'default' label with a
serie of case labels that code applies to? Along with providing the
same documentation effect (expressed in C instead of English) it would
cause GCC to warn whenever one of the chips was forgotten in a switch
statement. E.g. something similar to this patch:
========================================================================
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index b891b65..bfb2b4c 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2113,7 +2113,8 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
break;
}
/* fall through */
- default: /* f71858fg / f71882fg */
+ case f71858fg:
+ case f71882fg:
err = ...Hi, With pwm zone I mean the number of different speeds which can be programmed for one output channel, the temps divide the entire temp range into zones (number of zones == number of temps + 1) and for each zone one can then tell at what speed / pwm setting the fan should operate when the temperature is in that zone. I have a V0.27P datasheet for the 71889, but yes the fg suffix does not seem to be mentioned anywhere in the datasheet not sure where it comes from. I do know however that there are now new chips coming out with different a and e suffixes so I suggest that we stay with fg for the old chips and Ack, if you could do that that would be great! Please do that in a preparation patch though and not in the main patch. Here is my list: F71612A_V020P.pdf F71808A_V0.15P.pdf F71808E_V0.20P.pdf F71858_V026P.pdf F71862_V028P.pdf F71869E_V0.19P.pdf F71869_V1.1.pdf F71882_V0.24P.pdf F71889E_V0.19P.pdf F71889_V0.27P.pdf F8000_REG.pdf Regards, Hans --
I just noticed this patch was applied to mainline anyway. Regardless, I
will (try to) address these issues you raised.
Right now however, I am prioritising personal stuff above this
driver---bachelor's thesis and graduation presentation. When finished
with that (september) I'll allocate time to these issues again.
PS Those datasheets are written very poorly, although I have seen worse.
And as a reader I tend to become more "lazy" when I discover the
writer was "lazy" (not exactly by choice, more out of habit).
Unfortunately that doesn't work very well as reading style with
technical documentation, so this probably explains some of the errors
in this current patch.
PPS I do have a patch ready that addresses some of the issues you
raised. Those are only the cosmetic changes though (e.g. change
naming of chip, comment updates, etc.). Would you suggest me to
send it right now already or wait until I have time to address the
other issues as well?
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
Hi, I've send a mail directly to akpm asking for this to be removed, hopefully My goal for 2.6.36 is to pull the patch, and then we can wait til all issues are fixed properly before re-introducing it. Regards, Hans --
On Fri, 13 Aug 2010 12:01:36 +0200 It seems that I managed to muck up my paperwork and the patch was merged into mainline. We can revert it, or we can fix it up in place in time for 2.6.36. What do you guys suggest? --
Hi, Since both Giel and I are low on time to work on this, and since missing hwmon support on a system is not really a big deal (for most users) I suggest that this patch is reverted until we find the time to do it properly. Regards, Hans p.s. I've already seen mails that you've dropped this patch from -mm, does that mean it thas been removed from mainly too ? If not please remove it from mainline / ask Linus to remove it. Thanks. Hans --
Checking .../torvalds/linux-2.6.git you can see it's been reverted in mainline (see commit f2e41e9). -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Sleep while acquiring a resource lock on the Super I/O port. This should
prevent collisions from causing the hardware probe to fail with -EBUSY.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 7857ed3..267cb92 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -113,7 +113,7 @@ static struct platform_device *f71882fg_pdev;
/* Super-I/O Function prototypes */
static inline int superio_inb(int base, int reg);
static inline int superio_inw(int base, int reg);
-static inline void superio_enter(int base);
+static inline int superio_enter(int base);
static inline void superio_select(int base, int ld);
static inline void superio_exit(int base);
@@ -883,11 +883,20 @@ static int superio_inw(int base, int reg)
return val;
}
-static inline void superio_enter(int base)
+static inline int superio_enter(int base)
{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ base);
+ return -EBUSY;
+ }
+
/* according to the datasheet the key must be send twice! */
outb(SIO_UNLOCK_KEY, base);
outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
}
static inline void superio_select(int base, int ld)
@@ -899,6 +908,7 @@ static inline void superio_select(int base, int ld)
static inline void superio_exit(int base)
{
outb(SIO_LOCK_KEY, base);
+ release_region(base, 2);
}
static inline int fan_from_reg(u16 reg)
@@ -2239,21 +2249,15 @@ static int f71882fg_remove(struct platform_device *pdev)
static int __init f71882fg_find(int sioaddr, unsigned short *address,
struct f71882fg_sio_data *sio_data)
{
- int err = -ENODEV;
u16 devid;
-
- /* Don't step on other drivers' I/O space by accident */
- if ...Add a new watchdog driver for the Fintek F71808E and F71882FG Super I/O controllers. Signed-off-by: Giel van Schijndel <me@mortis.eu> --- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/f71808e_wdt.c | 768 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 780 insertions(+), 0 deletions(-) create mode 100644 drivers/watchdog/f71808e_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index afcfacc..edfd57d 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -401,6 +401,17 @@ config ALIM7101_WDT Most people will say N. +config F71808E_WDT + tristate "Fintek F71808E and F71882FG Watchdog" + depends on X86 && EXPERIMENTAL + help + This is the driver for the hardware watchdog on the Fintek + F71808E and F71882FG Super I/O controllers. + + You can compile this driver directly into the kernel, or use + it as a module. The module will be called f71808e_wdt. + + config GEODE_WDT tristate "AMD Geode CS5535/CS5536 Watchdog" depends on CS5535_MFGPT diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 72f3e20..9e3d616 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -66,6 +66,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_F71808E_WDT) += f71808e_wdt.o obj-$(CONFIG_GEODE_WDT) += geodewdt.o obj-$(CONFIG_SC520_WDT) += sc520_wdt.o obj-$(CONFIG_SBC_FITPC2_WATCHDOG) += sbc_fitpc2_wdt.o diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c new file mode 100644 index 0000000..7e5c266 --- /dev/null +++ b/drivers/watchdog/f71808e_wdt.c @@ -0,0 +1,768 @@ +/*************************************************************************** + * Copyright (C) 2006 by Hans Edgington <hans@edgington.nl> * + * Copyright (C) 2007-2009 ...
Ack! Acked-by: Hans de Goede <hdegoede@redhat.com> --
Would anyone please apply this patch? It's been sitting for quite some time now collecting dust (none of that fictional bit-rot thus far however). If this patch needs to be resubmitted or requires me to poke a specific individual please let me know (and what individual to poke). FYI the Message-ID of that patch is: <1280669455-31283-1-git-send-email-me@mortis.eu> PS I've added Guenter Roeck and Andrew Morton to the CC list because between now and the original submission date they got appended to scripts/get_maintainer.pl's output. -- Met vriendelijke groet, With kind regards, Giel van Schijndel -- "It would seem that perfection is attained not when no more can be added, but when no more can be removed." -- Antoine de Saint Exupéry
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com> Jean, if it is ok with you, I can apply it to my tree. I tried to find the patch on the web in downloadable form, but didn't find the correct version of it. So it would be great if you could re-send it. Thanks, Guenter --
I have no objection. I never had the time to look at these patches, and -- Jean Delvare --
Sleep while acquiring a resource lock on the Super I/O port. This should
prevent collisions from causing the hardware probe to fail with -EBUSY.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 537841e..75afb3b 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -111,7 +111,7 @@ static struct platform_device *f71882fg_pdev;
/* Super-I/O Function prototypes */
static inline int superio_inb(int base, int reg);
static inline int superio_inw(int base, int reg);
-static inline void superio_enter(int base);
+static inline int superio_enter(int base);
static inline void superio_select(int base, int ld);
static inline void superio_exit(int base);
@@ -861,11 +861,20 @@ static int superio_inw(int base, int reg)
return val;
}
-static inline void superio_enter(int base)
+static inline int superio_enter(int base)
{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ base);
+ return -EBUSY;
+ }
+
/* according to the datasheet the key must be send twice! */
outb(SIO_UNLOCK_KEY, base);
outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
}
static inline void superio_select(int base, int ld)
@@ -877,6 +886,7 @@ static inline void superio_select(int base, int ld)
static inline void superio_exit(int base)
{
outb(SIO_LOCK_KEY, base);
+ release_region(base, 2);
}
static inline int fan_from_reg(u16 reg)
@@ -2175,21 +2185,15 @@ static int f71882fg_remove(struct platform_device *pdev)
static int __init f71882fg_find(int sioaddr, unsigned short *address,
struct f71882fg_sio_data *sio_data)
{
- int err = -ENODEV;
u16 devid;
-
- /* Don't step on other drivers' I/O space by accident */
- if ...Sorry, forgot to sign off the commit message itself. So just to be sure it's understood to be signed off on by myself: Signed-off-by: Giel van Schijndel <me@mortis.eu> -- Met vriendelijke groet, With kind regards, Giel van Schijndel
Hi,
I don't have any objections against the proposed changes, but there
are 3 unrelated changes in this patch, please break them up in separate
patches:
1) code cleanup: properly using, previously defined, functions rather
than duplicating their code.
2) properly acquire I/O regions while probing
3) Make the addresses to probe an array
And IMHO you might just as well drop number 3, it does not really make
the code any better readable, and the old way is how all superio hwmon
drivers do things.
Regards,
Hans
--
Okay, broken up patches will follow as replies to this message.
Regarding number (3), my goal wasn't to put the probe addresses in an
array. My goal (as I think should have been made clear by the commit
message I had added) was to make sure that upon failure f71882fg_find's
return value gets passed back from the module's init function. This to
make sure the *proper* error code gets passed back to the user of
insmod/modprobe (as opposed to it being replaced by -ENODEV).
Further I think non -ENODEV errors should probably immediately result in
module initialisation failing (rather than retrying a probe), that's a
design choice though (which my patch doesn't bother addressing right
now).
However, regarding the for-loop/array thing, the same behaviour can be
aqcuired with a patch like the one following this line. (Please tell me
if you like this approach or something similar better. Though,
personally, I think it's "hackisher"/dirtier).
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 4230729..42d6304 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2295,9 +2295,11 @@ static int __init f71882fg_init(void)
memset(&sio_data, 0, sizeof(sio_data));
- if (f71882fg_find(0x2e, &address, &sio_data) &&
- f71882fg_find(0x4e, &address, &sio_data))
- goto exit;
+ if (f71882fg_find(0x2e, &address, &sio_data)) {
+ err = f71882fg_find(0x4e, &address, &sio_data);
+ if (err)
+ goto exit;
+ }
err = platform_driver_register(&f71882fg_driver);
if (err)
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
Some code cleanup: properly using previously defined functions, rather
than duplicating their code.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 18 ++++++------------
1 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 4230729..ca34e5c 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -856,10 +856,8 @@ static inline int superio_inb(int base, int reg)
static int superio_inw(int base, int reg)
{
int val;
- outb(reg++, base);
- val = inb(base + 1) << 8;
- outb(reg, base);
- val |= inb(base + 1);
+ val = superio_inb(base, reg) << 8;
+ val |= superio_inb(base, reg + 1);
return val;
}
@@ -905,10 +903,8 @@ static u16 f71882fg_read16(struct f71882fg_data *data, u8 reg)
{
u16 val;
- outb(reg++, data->addr + ADDR_REG_OFFSET);
- val = inb(data->addr + DATA_REG_OFFSET) << 8;
- outb(reg, data->addr + ADDR_REG_OFFSET);
- val |= inb(data->addr + DATA_REG_OFFSET);
+ val = f71882fg_read8(data, reg) << 8;
+ val |= f71882fg_read8(data, reg + 1);
return val;
}
@@ -921,10 +917,8 @@ static void f71882fg_write8(struct f71882fg_data *data, u8 reg, u8 val)
static void f71882fg_write16(struct f71882fg_data *data, u8 reg, u16 val)
{
- outb(reg++, data->addr + ADDR_REG_OFFSET);
- outb(val >> 8, data->addr + DATA_REG_OFFSET);
- outb(reg, data->addr + ADDR_REG_OFFSET);
- outb(val & 255, data->addr + DATA_REG_OFFSET);
+ f71882fg_write8(data, reg, val >> 8);
+ f71882fg_write8(data, reg + 1, val & 0xff);
}
static u16 f71882fg_read_temp(struct f71882fg_data *data, int nr)
--
1.6.4.4
--
Acquire the I/O region for the Super I/O chip while we're working on it.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index ca34e5c..fe7b671 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2178,6 +2178,13 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
int err = -ENODEV;
u16 devid;
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_region(sioaddr, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ (int)sioaddr);
+ return -EIO;
+ }
+
superio_enter(sioaddr);
devid = superio_inw(sioaddr, SIO_REG_MANID);
@@ -2232,6 +2239,7 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
(int)superio_inb(sioaddr, SIO_REG_DEVREV));
exit:
superio_exit(sioaddr);
+ release_region(sioaddr, 2);
return err;
}
--
1.6.4.4
--
Hi Giel, This doesn't make much sense. We call f71882fg_find up to twice, so we can get up to 2 error codes, but the module init function can only return one. There is no rationale for favoring the value returned by the second call over the value returned by the first call. So it seems reasonable to return an arbitrary error code. Of course we could spend code comparing the error values and returning the right one if the two functions returned the same error value, and only use the arbitrary default if they returned different values. But honestly I don't see any point in doing this in practice. Let's just log errors other than -ENODEV so that the user can see the exact cause of failure in the I disagree. If the probe of 0x2e/0x2f fails because that region was busy, this should not prevent us from giving a try to region 0x4e/0x4f. I can't think of any error that would warrant avoiding the probe at 0x4e/0x4f. Worse thing that can happen is that the second probe fails I don't like either implementation, I would leave the code as it is today. -- Jean Delvare --
Ack. New patch follows this line.
========================================================================
hwmon: f71882fg: acquire I/O regions while we're working with them
Acquire the I/O region for the Super I/O chip while we're working on it.
Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
drivers/hwmon/f71882fg.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index ca34e5c..537841e 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2178,6 +2178,13 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
int err = -ENODEV;
u16 devid;
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_region(sioaddr, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ (int)sioaddr);
+ return -EBUSY;
+ }
+
superio_enter(sioaddr);
devid = superio_inw(sioaddr, SIO_REG_MANID);
@@ -2232,6 +2239,7 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
(int)superio_inb(sioaddr, SIO_REG_DEVREV));
exit:
superio_exit(sioaddr);
+ release_region(sioaddr, 2);
return err;
}
--
1.6.4.4
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
