[PATCH 0/2] [RFC] platform/x86: toshiba_acpi: HWMON Fan RPM support

[PATCH 0/2] [RFC] platform/x86: toshiba_acpi: HWMON Fan RPM support

[Arvid Norlander]

Fan
===

Currently /sys/bus/acpi/devices/TOS6208:00/fan allows controlling the fan
by writing 0 (off) or 1 (on at low speed). However when reading I have
observed values up to 64 (fan at full speed during prime95 stress test).

Removing the check for "zero or one" shows that on the Z830 at least 64
levels do indeed seem possible. In fact higher values can be written.

But anything above ~50 seems to max out the RPM.

I don't know how to detect the supported range, so I have not created a
patch for this. Advice is welcome.


Fan RPM
=======

There is a way to read Fan RPM:

#define HCI_FAN_RPM 0x45

This one is weird. On windows I have observed the cooling self test program
(which supposedly verifies that the cooling is working correctly) calling
this a few different ways. Here is a summary of what I managed to figure
out:

HCI_SET, 0x45, 0, 1, 0, 0: This sets the fan to run at max speed, but it
will not be visible when reading /sys/bus/acpi/devices/TOS6208:00/fan.
I will refer to this operation as "set-max-fan" below.

The only way I found to stop it running at max RPM is to use HCI_FAN
(e.g. 0 > /sys/bus/acpi/devices/TOS6208:00/fan or call the ACPI method
directly).

However the get method is more interesting:

HCI_GET, 0x45, 0, 1, 0, 0 returns: {0x0, 0x45, fan_rpm, 0x1db0, 0x0, 0x0}

I believe fan_rpm is accurate, without any scaling factors needed:
* It behaves properly (higher value when fan is louder/faster/higher
  pitched, 0 when fan is off).
* It matches the value range reported by HwInfo64 on Windows (which seems
  to be able to read this, I have not looked into how it does that).
* Unfortunately there is no tool by Toshiba that exposes the numerical
  value that I can find (that would have been ideal). Nor is it shown in
  BIOS. The Windows tools "Toshiba PC Health Monitor" reports everything in
  percentages. Yes even the temperatures!
* It is definitely a loud and whiny fan, even by laptop standards, so the
  high reported RPM range of 3540-7600 RPM could make sense. Though it did
  seem a bit high.
* Finally, to be sure, I borrowed a tachometer from work. Yes, the fan
  really spins that fast. Byt it is only 30 mm, so I guess that makes
  sense.

HCI_GET 0x45, 0, 0, 0, 0 returns: {0x0, 0x45, fan_rpm, 0x0, 0x0, 0x0}

The Windows software does *not* use this variant as far as I have observed.
It appears to work the same except that it doesn't return 0x1db0 in index 3.

I'm not sure, but I strongly suspect 0x1db0 could be the max RPM (7600).
The most I have observed when using "set-max-fan" is 0x1da6 (7590 RPM),
which is very close. Note that this is significantly more than I can get
using just HCI_FAN, which seems to max out at 0x17ac (6060 RPM).


Patches
=======

I'm not personally particularly interested in user space control of fan
speed, plus the fact that there is a way to make the fan go faster than
the *other* max speed makes me wonder about the safety of running the fan
at that speed for prolonged periods of time. Thus, I have only added a
read-only hwmon interface for reading the fan RPM.

This is heavily based (read: copy, paste and modify) on the thinkpad_acpi
hwmon-code. Note that unlike that driver, I do not create a separate
platform device to be a parent to the hwmon interface. In my testing this
was un-needed and the ACPI device worked fine as a parent. Should I be
wrong about this, and there is a good reason to create a separate driver,
please elucidate me!

I elected to use the same call that the Windows code does, which fetches
what I believe is the max RPM. I think it is safer to stay as close as
possible to that code. However I don't currently make use of this value,
suggestions for where to use it are welcome.

Note! I assume that if the FAN RPM call do not result in an error, that
it is in fact supported. This may not be true. I would welcome testing by
anyone who owns a Toshiba laptop!


Arvid Norlander (2):
  platform/x86: toshiba_acpi: Add fan RPM reading (internals)
  platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

 drivers/platform/x86/Kconfig        |  1 +
 drivers/platform/x86/toshiba_acpi.c | 79 +++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)


base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555
-- 
2.37.2

[PATCH 1/2] [RFC] platform/x86: toshiba_acpi: Add fan RPM reading (internals)

[Arvid Norlander]

This add the internal feature detection and reading function for fan RPM.

The approach is based on tracing ACPI calls using AMLI (a tracer/debugger
built into ACPI.sys) while using the Windows cooling self-test software.

The call used is {HCI_GET, 0x45, 0, 1, 0, 0} which returns:
{0x0, 0x45, fan_rpm, probably_max_rpm, 0x0, 0x0}

What is probably the max RPM is not currently used.

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/toshiba_acpi.c | 30 +++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 0fc9e8b8827b..02e3522f4eeb 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -106,6 +106,7 @@ MODULE_LICENSE("GPL");
 #define HCI_VIDEO_OUT			0x001c
 #define HCI_HOTKEY_EVENT		0x001e
 #define HCI_LCD_BRIGHTNESS		0x002a
+#define HCI_FAN_RPM			0x0045
 #define HCI_WIRELESS			0x0056
 #define HCI_ACCELEROMETER		0x006d
 #define HCI_COOLING_METHOD		0x007f
@@ -185,6 +186,7 @@ struct toshiba_acpi_dev {
 	unsigned int illumination_supported:1;
 	unsigned int video_supported:1;
 	unsigned int fan_supported:1;
+	unsigned int fan_rpm_supported:1;
 	unsigned int system_event_supported:1;
 	unsigned int ntfy_supported:1;
 	unsigned int info_supported:1;
@@ -1616,6 +1618,29 @@ static const struct proc_ops fan_proc_ops = {
 	.proc_write	= fan_proc_write,
 };
 
+/* Fan RPM */
+static int get_fan_rpm(struct toshiba_acpi_dev *dev, u32 *rpm)
+{
+	u32 in[TCI_WORDS] = { HCI_GET, HCI_FAN_RPM, 0, 1, 0, 0 };
+	u32 out[TCI_WORDS];
+	acpi_status status = tci_raw(dev, in, out);
+
+	if (ACPI_FAILURE(status)) {
+		pr_err("ACPI call to get Fan speed failed\n");
+		return -EIO;
+	}
+
+	if (out[0] == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	if (out[0] == TOS_SUCCESS) {
+		*rpm = out[2];
+		return 0;
+	}
+
+	return -EIO;
+}
+
 static int keys_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
@@ -2928,6 +2953,8 @@ static void print_supported_features(struct toshiba_acpi_dev *dev)
 		pr_cont(" video-out");
 	if (dev->fan_supported)
 		pr_cont(" fan");
+	if (dev->fan_rpm_supported)
+		pr_cont(" fan-rpm");
 	if (dev->tr_backlight_supported)
 		pr_cont(" transflective-backlight");
 	if (dev->illumination_supported)
@@ -3157,6 +3184,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_status(dev, &dummy);
 	dev->fan_supported = !ret;
 
+	ret = get_fan_rpm(dev, &dummy);
+	dev->fan_rpm_supported = !ret;
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
-- 
2.37.2

[PATCH 2/2] [RFC] platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

[Arvid Norlander]

This expands on the previous commit, exporting the fan RPM via hwmon.

This will look something like the following when using the "sensors"
command from lm_sensors:

toshiba_acpi_sensors-acpi-0
Adapter: ACPI interface
fan1:           0 RPM

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/Kconfig        |  1 +
 drivers/platform/x86/toshiba_acpi.c | 49 +++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f2f98e942cf2..9a98974ab8bf 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -799,6 +799,7 @@ config ACPI_TOSHIBA
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
 	depends on RFKILL || RFKILL = n
 	depends on IIO
+	select HWMON
 	select INPUT_SPARSEKMAP
 	help
 	  This driver adds support for access to certain system settings
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 02e3522f4eeb..2b71dac34cf0 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -39,6 +39,7 @@
 #include <linux/i8042.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/hwmon.h>
 #include <linux/uaccess.h>
 #include <linux/miscdevice.h>
 #include <linux/rfkill.h>
@@ -171,6 +172,7 @@ struct toshiba_acpi_dev {
 	struct miscdevice miscdev;
 	struct rfkill *wwan_rfk;
 	struct iio_dev *indio_dev;
+	struct device *hwmon_device;
 
 	int force_fan;
 	int last_key_event;
@@ -2941,6 +2943,38 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
 	return 0;
 }
 
+
+/* HWMON support for fan */
+static ssize_t fan_fan1_input_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	u32 value;
+	int ret;
+
+	ret = get_fan_rpm(toshiba_acpi, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", value);
+}
+
+static DEVICE_ATTR(fan1_input, S_IRUGO, fan_fan1_input_show, NULL);
+
+static struct attribute *fan_attributes[] = {
+	&dev_attr_fan1_input.attr,
+	NULL
+};
+
+static const struct attribute_group fan_attr_group = {
+	.attrs = fan_attributes,
+};
+
+static const struct attribute_group *toshiba_acpi_hwmon_groups[] = {
+	&fan_attr_group,
+	NULL,
+};
+
 static void print_supported_features(struct toshiba_acpi_dev *dev)
 {
 	pr_info("Supported laptop features:");
@@ -2995,6 +3029,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
 
 	remove_toshiba_proc_entries(dev);
 
+	if (dev->hwmon_device)
+		hwmon_device_unregister(dev->hwmon_device);
+
 	if (dev->accelerometer_supported && dev->indio_dev) {
 		iio_device_unregister(dev->indio_dev);
 		iio_device_free(dev->indio_dev);
@@ -3187,6 +3224,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_rpm(dev, &dummy);
 	dev->fan_rpm_supported = !ret;
 
+	if (dev->fan_rpm_supported) {
+		dev->hwmon_device = hwmon_device_register_with_groups(
+			&dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
+			toshiba_acpi_hwmon_groups);
+		if (IS_ERR(dev->hwmon_device)) {
+			ret = PTR_ERR(dev->hwmon_device);
+			dev->hwmon_device = NULL;
+			pr_err("unable to register hwmon device\n");
+			goto error;
+		}
+	}
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
-- 
2.37.2

Re: [PATCH 2/2] [RFC] platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

[Guenter Roeck]

On 9/1/22 07:49, Arvid Norlander wrote:
> This expands on the previous commit, exporting the fan RPM via hwmon.
> 
> This will look something like the following when using the "sensors"
> command from lm_sensors:
> 
> toshiba_acpi_sensors-acpi-0
> Adapter: ACPI interface
> fan1:           0 RPM
> 
> Signed-off-by: Arvid Norlander <lkml@vorpal.se>
> ---
>   drivers/platform/x86/Kconfig        |  1 +
>   drivers/platform/x86/toshiba_acpi.c | 49 +++++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f2f98e942cf2..9a98974ab8bf 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -799,6 +799,7 @@ config ACPI_TOSHIBA
>   	depends on ACPI_VIDEO || ACPI_VIDEO = n
>   	depends on RFKILL || RFKILL = n
>   	depends on IIO
> +	select HWMON

This is wrong. I know other drivers in this directory do it, but it is
still wrong. It should be something like

	depends on HWMON || HWMON=n

and the code should deal with the conditionals.

>   	select INPUT_SPARSEKMAP
>   	help
>   	  This driver adds support for access to certain system settings
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 02e3522f4eeb..2b71dac34cf0 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -39,6 +39,7 @@
>   #include <linux/i8042.h>
>   #include <linux/acpi.h>
>   #include <linux/dmi.h>
> +#include <linux/hwmon.h>
>   #include <linux/uaccess.h>
>   #include <linux/miscdevice.h>
>   #include <linux/rfkill.h>
> @@ -171,6 +172,7 @@ struct toshiba_acpi_dev {
>   	struct miscdevice miscdev;
>   	struct rfkill *wwan_rfk;
>   	struct iio_dev *indio_dev;
> +	struct device *hwmon_device;
>   
>   	int force_fan;
>   	int last_key_event;
> @@ -2941,6 +2943,38 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>   	return 0;
>   }
>   
> +
> +/* HWMON support for fan */
> +static ssize_t fan_fan1_input_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	u32 value;
> +	int ret;
> +
> +	ret = get_fan_rpm(toshiba_acpi, &value);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%u\n", value);
> +}
> +
> +static DEVICE_ATTR(fan1_input, S_IRUGO, fan_fan1_input_show, NULL);
> +
> +static struct attribute *fan_attributes[] = {
> +	&dev_attr_fan1_input.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group fan_attr_group = {
> +	.attrs = fan_attributes,
> +};
> +
> +static const struct attribute_group *toshiba_acpi_hwmon_groups[] = {
> +	&fan_attr_group,
> +	NULL,
> +};
> +
>   static void print_supported_features(struct toshiba_acpi_dev *dev)
>   {
>   	pr_info("Supported laptop features:");
> @@ -2995,6 +3029,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>   
>   	remove_toshiba_proc_entries(dev);
>   
> +	if (dev->hwmon_device)
> +		hwmon_device_unregister(dev->hwmon_device);
> +
>   	if (dev->accelerometer_supported && dev->indio_dev) {
>   		iio_device_unregister(dev->indio_dev);
>   		iio_device_free(dev->indio_dev);
> @@ -3187,6 +3224,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>   	ret = get_fan_rpm(dev, &dummy);
>   	dev->fan_rpm_supported = !ret;
>   
> +	if (dev->fan_rpm_supported) {
> +		dev->hwmon_device = hwmon_device_register_with_groups(

New drivers should register using [devm_]hwmon_device_register_with_info().

> +			&dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
> +			toshiba_acpi_hwmon_groups);
> +		if (IS_ERR(dev->hwmon_device)) {
> +			ret = PTR_ERR(dev->hwmon_device);
> +			dev->hwmon_device = NULL;
> +			pr_err("unable to register hwmon device\n");
> +			goto error;

The driver works just fine without hwmon, and should not fail to probe
if hwmon registration fails. It did not fail before this patch was applied
either, and hwmon is not essential functionality for this driver.

Guenter

> +		}
> +	}
> +
>   	toshiba_wwan_available(dev);
>   	if (dev->wwan_supported)
>   		toshiba_acpi_setup_wwan_rfkill(dev);

Re: [PATCH 2/2] [RFC] platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

[Arvid Norlander]

On 2022-09-01 17:28, Guenter Roeck wrote:
> On 9/1/22 07:49, Arvid Norlander wrote:
>> This expands on the previous commit, exporting the fan RPM via hwmon.
>>
>> This will look something like the following when using the "sensors"
>> command from lm_sensors:
>>
>> toshiba_acpi_sensors-acpi-0
>> Adapter: ACPI interface
>> fan1:           0 RPM
>>
>> Signed-off-by: Arvid Norlander <lkml@vorpal.se>
>> ---
>>   drivers/platform/x86/Kconfig        |  1 +
>>   drivers/platform/x86/toshiba_acpi.c | 49 +++++++++++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index f2f98e942cf2..9a98974ab8bf 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -799,6 +799,7 @@ config ACPI_TOSHIBA
>>       depends on ACPI_VIDEO || ACPI_VIDEO = n
>>       depends on RFKILL || RFKILL = n
>>       depends on IIO
>> +    select HWMON
> 
> This is wrong. I know other drivers in this directory do it, but it is
> still wrong. It should be something like
> 
>     depends on HWMON || HWMON=n
> 
> and the code should deal with the conditionals.

Thanks, I will do that. Perhaps ping the thinkpad_acpi maintainers about
this as well, that is where I copied it from.

> 
>>       select INPUT_SPARSEKMAP
>>       help
>>         This driver adds support for access to certain system settings
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 02e3522f4eeb..2b71dac34cf0 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -39,6 +39,7 @@
>>   #include <linux/i8042.h>
>>   #include <linux/acpi.h>
>>   #include <linux/dmi.h>
>> +#include <linux/hwmon.h>
>>   #include <linux/uaccess.h>
>>   #include <linux/miscdevice.h>
>>   #include <linux/rfkill.h>
>> @@ -171,6 +172,7 @@ struct toshiba_acpi_dev {
>>       struct miscdevice miscdev;
>>       struct rfkill *wwan_rfk;
>>       struct iio_dev *indio_dev;
>> +    struct device *hwmon_device;
>>         int force_fan;
>>       int last_key_event;
>> @@ -2941,6 +2943,38 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>       return 0;
>>   }
>>   +
>> +/* HWMON support for fan */
>> +static ssize_t fan_fan1_input_show(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    u32 value;
>> +    int ret;
>> +
>> +    ret = get_fan_rpm(toshiba_acpi, &value);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return sysfs_emit(buf, "%u\n", value);
>> +}
>> +
>> +static DEVICE_ATTR(fan1_input, S_IRUGO, fan_fan1_input_show, NULL);
>> +
>> +static struct attribute *fan_attributes[] = {
>> +    &dev_attr_fan1_input.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group fan_attr_group = {
>> +    .attrs = fan_attributes,
>> +};
>> +
>> +static const struct attribute_group *toshiba_acpi_hwmon_groups[] = {
>> +    &fan_attr_group,
>> +    NULL,
>> +};
>> +
>>   static void print_supported_features(struct toshiba_acpi_dev *dev)
>>   {
>>       pr_info("Supported laptop features:");
>> @@ -2995,6 +3029,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>>         remove_toshiba_proc_entries(dev);
>>   +    if (dev->hwmon_device)
>> +        hwmon_device_unregister(dev->hwmon_device);
>> +
>>       if (dev->accelerometer_supported && dev->indio_dev) {
>>           iio_device_unregister(dev->indio_dev);
>>           iio_device_free(dev->indio_dev);
>> @@ -3187,6 +3224,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>       ret = get_fan_rpm(dev, &dummy);
>>       dev->fan_rpm_supported = !ret;
>>   +    if (dev->fan_rpm_supported) {
>> +        dev->hwmon_device = hwmon_device_register_with_groups(
> 
> New drivers should register using [devm_]hwmon_device_register_with_info().

Again, copied thikpad_acpi, but I'll look into that function for version 2.

> 
>> +            &dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
>> +            toshiba_acpi_hwmon_groups);
>> +        if (IS_ERR(dev->hwmon_device)) {
>> +            ret = PTR_ERR(dev->hwmon_device);
>> +            dev->hwmon_device = NULL;
>> +            pr_err("unable to register hwmon device\n");
>> +            goto error;
> 
> The driver works just fine without hwmon, and should not fail to probe
> if hwmon registration fails. It did not fail before this patch was applied
> either, and hwmon is not essential functionality for this driver.

Good point. Again, this was based on thinkpad_acpi which it appears then
has some legacy behaviour.

> 
> Guenter
> 
>> +        }
>> +    }
>> +
>>       toshiba_wwan_available(dev);
>>       if (dev->wwan_supported)
>>           toshiba_acpi_setup_wwan_rfkill(dev);
> 

Best regards,
Arvid Norlander