[PATCH v2 0/4] hwmon: (sch56xx) Automatically load on supported hardware

[PATCH v2 0/4] hwmon: (sch56xx) Automatically load on supported hardware

[Armin Wolf]

The hardware monitoring solutions supported by the sch5627 and
sch5636 drivers are Fujitsu-specific.
After some online searching, i found out that the solution used
with the SCH5627 is called "Antiope" by Fujitsu, just like the
"Theseus" solution inside the SCH5636.
I also found out that "Antiope" and "Theseus" are listed as
DMI onboard devices on supported Fujitsu devices, so the
sch56xx_common module can be loaded automatically an check
for the DMI devices. However some devices like the Esprimo C700
have both devices, so after verifying that at least one onboard
device is present, sch56xx_common still has to detect which chip
is present.
This is safe however if at least one device is present.

Tested on a Fujitsu Esprimo P720.

---
Changes in v2:
- fix unused variable issue reported by the kernel test robot
by assinging the platform device id list in sch5627/sch5636
to platform_driver->id_table.

Armin Wolf (4):
  hwmon: (sch56xx) Autoload modules on platform device creation
  hwmon: (sch56xx-common) Add automatic module loading on supported
    devices
  hwmon: (sch56xx-common) Replace msleep() with usleep_range()
  hwmon: (sch56xx-common) Replace WDOG_ACTIVE with WDOG_HW_RUNNING

 drivers/hwmon/sch5627.c        | 10 ++++++++
 drivers/hwmon/sch5636.c        | 10 ++++++++
 drivers/hwmon/sch56xx-common.c | 44 ++++++++++++++++++++++++++++++----
 3 files changed, 60 insertions(+), 4 deletions(-)

--
2.30.2

[PATCH v2 1/4] hwmon: (sch56xx) Autoload modules on platform device creation

[Armin Wolf]

Right now, when sch56xx-common has detected a SCH5627/SCH5636
superio chip, the corresponding module is not automatically
loaded.
Fix that by adding the necessary device tables to both modules.

Tested on a Fujitsu Esprimo P720.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch5627.c | 10 ++++++++++
 drivers/hwmon/sch5636.c | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 8f1b569c69e7..72c3f6757e34 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -7,6 +7,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/jiffies.h>
@@ -456,11 +457,20 @@ static int sch5627_probe(struct platform_device *pdev)
 	return 0;
 }

+static const struct platform_device_id sch5627_device_id[] = {
+	{
+		.name = "sch5627",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, sch5627_device_id);
+
 static struct platform_driver sch5627_driver = {
 	.driver = {
 		.name	= DRVNAME,
 	},
 	.probe		= sch5627_probe,
+	.id_table	= sch5627_device_id,
 };

 module_platform_driver(sch5627_driver);
diff --git a/drivers/hwmon/sch5636.c b/drivers/hwmon/sch5636.c
index 39ff1c9b1df5..269757bc3a9e 100644
--- a/drivers/hwmon/sch5636.c
+++ b/drivers/hwmon/sch5636.c
@@ -7,6 +7,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/jiffies.h>
@@ -501,12 +502,21 @@ static int sch5636_probe(struct platform_device *pdev)
 	return err;
 }

+static const struct platform_device_id sch5636_device_id[] = {
+	{
+		.name = "sch5636",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, sch5636_device_id);
+
 static struct platform_driver sch5636_driver = {
 	.driver = {
 		.name	= DRVNAME,
 	},
 	.probe		= sch5636_probe,
 	.remove		= sch5636_remove,
+	.id_table	= sch5636_device_id,
 };

 module_platform_driver(sch5636_driver);
--
2.30.2

[PATCH v2 2/4] hwmon: (sch56xx-common) Add automatic module loading on supported devices

[Armin Wolf]

This patch enables the sch56xx-common module to get automatically
loaded on supported machines.
If a machine supports Fujitsu's SCH56XX-based hardware monitoring
solutions, it contains a "Antiope"/" Antiope" dmi onboard device
in case of the sch5627 or a "Theseus"/" Theseus" dmi onboard device
in case of the sch5636.
Since some machines like the Esprimo C700 have a seemingly faulty
DMI table containing both onboard devices, the driver still needs
to probe for the individual superio chip, which in presence of at
least one DMI onboard device however can be considered safe.
Also add a module parameter allowing for bypassing the
DMI check.

Tested on a Fujitsu Esprimo P720.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch56xx-common.c | 40 ++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 40cdadad35e5..0172aa16dc0c 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -7,8 +7,10 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/acpi.h>
@@ -19,7 +21,10 @@
 #include <linux/slab.h>
 #include "sch56xx-common.h"

-/* Insmod parameters */
+static bool ignore_dmi;
+module_param(ignore_dmi, bool, 0);
+MODULE_PARM_DESC(ignore_dmi, "Omit DMI check for supported devices (default=0)");
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
@@ -518,11 +523,42 @@ static int __init sch56xx_device_add(int address, const char *name)
 	return PTR_ERR_OR_ZERO(sch56xx_pdev);
 }

+/* For autoloading only */
+static const struct dmi_system_id sch56xx_dmi_table[] __initconst = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
+		},
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(dmi, sch56xx_dmi_table);
+
 static int __init sch56xx_init(void)
 {
-	int address;
 	const char *name = NULL;
+	int address;

+	if (!ignore_dmi) {
+		if (!dmi_check_system(sch56xx_dmi_table))
+			return -ENODEV;
+
+		/*
+		 * Some machines like the Esprimo P720 and Esprimo C700 have
+		 * onboard devices named " Antiope"/" Theseus" instead of
+		 * "Antiope"/"Theseus", so we need to check for both.
+		 */
+		if (!dmi_find_device(DMI_DEV_TYPE_OTHER, "Antiope", NULL) &&
+		    !dmi_find_device(DMI_DEV_TYPE_OTHER, " Antiope", NULL) &&
+		    !dmi_find_device(DMI_DEV_TYPE_OTHER, "Theseus", NULL) &&
+		    !dmi_find_device(DMI_DEV_TYPE_OTHER, " Theseus", NULL))
+			return -ENODEV;
+	}
+
+	/*
+	 * Some devices like the Esprimo C700 have both onboard devices,
+	 * so we still have to check manually
+	 */
 	address = sch56xx_find(0x4e, &name);
 	if (address < 0)
 		address = sch56xx_find(0x2e, &name);
--
2.30.2

[PATCH v2 3/4] hwmon: (sch56xx-common) Replace msleep() with usleep_range()

[Armin Wolf]

msleep(1) will often sleep more than 20ms, slowing down sensor
and watchdog reads/writes. Use usleep_range() as recommended
in timers-howto.rst to fix that.

Tested on a Fujitsu Esprimo P720.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch56xx-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 0172aa16dc0c..f66e1ed4b1aa 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -139,7 +139,7 @@ static int sch56xx_send_cmd(u16 addr, u8 cmd, u16 reg, u8 v)
 	/* EM Interface Polling "Algorithm" */
 	for (i = 0; i < max_busy_polls + max_lazy_polls; i++) {
 		if (i >= max_busy_polls)
-			msleep(1);
+			usleep_range(1, 2);
 		/* Read Interrupt source Register */
 		val = inb(addr + 8);
 		/* Write Clear the interrupt source bits */
--
2.30.2

[PATCH v2 4/4] hwmon: (sch56xx-common) Replace WDOG_ACTIVE with WDOG_HW_RUNNING

[Armin Wolf]

If the watchdog was already enabled by the BIOS after booting, the
watchdog infrastructure needs to regularly send keepalives to
prevent a unexpected reset.
WDOG_ACTIVE only serves as an status indicator for userspace,
we want to use WDOG_HW_RUNNING instead.

Since my Fujitsu Esprimo P720 does not support the watchdog,
this change is compile-tested only.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Fixes: fb551405c0f8 (watchdog: sch56xx: Use watchdog core)
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/sch56xx-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index f66e1ed4b1aa..2cd146fd0562 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -427,7 +427,7 @@ void sch56xx_watchdog_register(struct device *parent, u16 addr, u32 revision,
 	data->wddev.max_timeout = 255 * 60;
 	watchdog_set_nowayout(&data->wddev, nowayout);
 	if (output_enable & SCH56XX_WDOG_OUTPUT_ENABLE)
-		set_bit(WDOG_ACTIVE, &data->wddev.status);
+		set_bit(WDOG_HW_RUNNING, &data->wddev.status);

 	/* Since the watchdog uses a downcounter there is no register to read
 	   the BIOS set timeout from (if any was set at all) ->
--
2.30.2

Re: [PATCH v2 3/4] hwmon: (sch56xx-common) Replace msleep() with usleep_range()

[Guenter Roeck]

On 1/31/22 11:31, Armin Wolf wrote:
> msleep(1) will often sleep more than 20ms, slowing down sensor
> and watchdog reads/writes. Use usleep_range() as recommended
> in timers-howto.rst to fix that.
> 
> Tested on a Fujitsu Esprimo P720.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   drivers/hwmon/sch56xx-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
> index 0172aa16dc0c..f66e1ed4b1aa 100644
> --- a/drivers/hwmon/sch56xx-common.c
> +++ b/drivers/hwmon/sch56xx-common.c
> @@ -139,7 +139,7 @@ static int sch56xx_send_cmd(u16 addr, u8 cmd, u16 reg, u8 v)
>   	/* EM Interface Polling "Algorithm" */
>   	for (i = 0; i < max_busy_polls + max_lazy_polls; i++) {
>   		if (i >= max_busy_polls)
> -			msleep(1);
> +			usleep_range(1, 2);

This replaces a 1-millisecond sleep with a 1-2 microsecond sleep.

Are you sure this is what you want to do ? Given that task switches typically
take several microseconds, the new code is pretty much identical to a busy
loop, and the maximum sleep time is reduced significantly.

Guenter

>   		/* Read Interrupt source Register */
>   		val = inb(addr + 8);
>   		/* Write Clear the interrupt source bits */
> --
> 2.30.2
> 

Re: [PATCH v2 3/4] hwmon: (sch56xx-common) Replace msleep() with usleep_range()

[Hans de Goede]

Hi,

On 1/31/22 21:10, Guenter Roeck wrote:
> On 1/31/22 11:31, Armin Wolf wrote:
>> msleep(1) will often sleep more than 20ms, slowing down sensor
>> and watchdog reads/writes. Use usleep_range() as recommended
>> in timers-howto.rst to fix that.
>>
>> Tested on a Fujitsu Esprimo P720.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/sch56xx-common.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
>> index 0172aa16dc0c..f66e1ed4b1aa 100644
>> --- a/drivers/hwmon/sch56xx-common.c
>> +++ b/drivers/hwmon/sch56xx-common.c
>> @@ -139,7 +139,7 @@ static int sch56xx_send_cmd(u16 addr, u8 cmd, u16 reg, u8 v)
>>       /* EM Interface Polling "Algorithm" */
>>       for (i = 0; i < max_busy_polls + max_lazy_polls; i++) {
>>           if (i >= max_busy_polls)
>> -            msleep(1);
>> +            usleep_range(1, 2);
> 
> This replaces a 1-millisecond sleep with a 1-2 microsecond sleep.
> 
> Are you sure this is what you want to do ? Given that task switches typically
> take several microseconds, the new code is pretty much identical to a busy
> loop, and the maximum sleep time is reduced significantly.

Ah good catch, I missed that will reviewing v1, sorry about that.

The issue is actually worse then busy-waiting the max wait time
in this code is expressed in a maximum number of polls, so
if usleep_range(1, 2) would really only sleep 1 usec, we would
wait much too short and may hit a false-positive timeout condition
here.

Regards,

Hans