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

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

[Arvid Norlander]

Changelog
=========
v2: Fixed feedback on usage of HWMON interfaces in patch 2.

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.

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 | 102 ++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)


base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.37.3

[PATCH 1/2] 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.3

[PATCH v2 2/2] 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 | 72 +++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f2f98e942cf2..4d0d2676939a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -797,6 +797,7 @@ config ACPI_TOSHIBA
 	depends on INPUT
 	depends on SERIO_I8042 || SERIO_I8042 = n
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on HWMON || HWMON = n
 	depends on RFKILL || RFKILL = n
 	depends on IIO
 	select INPUT_SPARSEKMAP
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 02e3522f4eeb..a976dfb97a5e 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -46,6 +46,10 @@
 #include <linux/toshiba.h>
 #include <acpi/video.h>
 
+#ifdef CONFIG_HWMON
+#include <linux/hwmon.h>
+#endif
+
 MODULE_AUTHOR("John Belmonte");
 MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
 MODULE_LICENSE("GPL");
@@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
 	struct miscdevice miscdev;
 	struct rfkill *wwan_rfk;
 	struct iio_dev *indio_dev;
+#ifdef CONFIG_HWMON
+	struct device *hwmon_device;
+#endif
 
 	int force_fan;
 	int last_key_event;
@@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
 	return 0;
 }
 
+/* HWMON support for fan */
+#ifdef CONFIG_HWMON
+umode_t toshiba_acpi_hwmon_is_visible(const void *drvdata,
+				      enum hwmon_sensor_types type,
+				      u32 attr, int channel)
+{
+	return 0444;
+}
+
+int toshiba_acpi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, long *val)
+{
+	/*
+	 * There is only a single channel and single attribute (for the
+	 * fan) at this point.
+	 * This can be replaced with more advanced logic in the future,
+	 * should the need arise.
+	 */
+	if (type == hwmon_fan && channel == 0 && attr == hwmon_fan_input) {
+		u32 value;
+		int ret;
+
+		ret = get_fan_rpm(toshiba_acpi, &value);
+		if (ret)
+			return ret;
+
+		*val = value;
+		return 0;
+	}
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_channel_info *toshiba_acpi_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops toshiba_acpi_hwmon_ops = {
+	.is_visible = toshiba_acpi_hwmon_is_visible,
+	.read = toshiba_acpi_hwmon_read,
+};
+
+static const struct hwmon_chip_info toshiba_acpi_hwmon_chip_info = {
+	.ops = &toshiba_acpi_hwmon_ops,
+	.info = toshiba_acpi_hwmon_info,
+};
+#endif
+
 static void print_supported_features(struct toshiba_acpi_dev *dev)
 {
 	pr_info("Supported laptop features:");
@@ -2995,6 +3050,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
 
 	remove_toshiba_proc_entries(dev);
 
+#ifdef CONFIG_HWMON
+	if (dev->hwmon_device)
+		hwmon_device_unregister(dev->hwmon_device);
+#endif
+
 	if (dev->accelerometer_supported && dev->indio_dev) {
 		iio_device_unregister(dev->indio_dev);
 		iio_device_free(dev->indio_dev);
@@ -3187,6 +3247,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_rpm(dev, &dummy);
 	dev->fan_rpm_supported = !ret;
 
+#ifdef CONFIG_HWMON
+	if (dev->fan_rpm_supported) {
+		dev->hwmon_device = hwmon_device_register_with_info(
+			&dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
+			&toshiba_acpi_hwmon_chip_info, NULL);
+		if (IS_ERR(dev->hwmon_device)) {
+			dev->hwmon_device = NULL;
+			pr_warn("unable to register hwmon device, skipping\n");
+		}
+	}
+#endif
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
-- 
2.37.3

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

[Guenter Roeck]

On 9/1/22 14:58, 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 | 72 +++++++++++++++++++++++++++++
>   2 files changed, 73 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f2f98e942cf2..4d0d2676939a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>   	depends on INPUT
>   	depends on SERIO_I8042 || SERIO_I8042 = n
>   	depends on ACPI_VIDEO || ACPI_VIDEO = n
> +	depends on HWMON || HWMON = n
>   	depends on RFKILL || RFKILL = n
>   	depends on IIO
>   	select INPUT_SPARSEKMAP
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 02e3522f4eeb..a976dfb97a5e 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -46,6 +46,10 @@
>   #include <linux/toshiba.h>
>   #include <acpi/video.h>
>   
> +#ifdef CONFIG_HWMON
> +#include <linux/hwmon.h>
> +#endif

ifdef not needed here.

> +
>   MODULE_AUTHOR("John Belmonte");
>   MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>   MODULE_LICENSE("GPL");
> @@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
>   	struct miscdevice miscdev;
>   	struct rfkill *wwan_rfk;
>   	struct iio_dev *indio_dev;
> +#ifdef CONFIG_HWMON
> +	struct device *hwmon_device;
> +#endif
>   
>   	int force_fan;
>   	int last_key_event;
> @@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>   	return 0;
>   }
>   
> +/* HWMON support for fan */
> +#ifdef CONFIG_HWMON

This should be #if IS_REACHABLE(CONFIG_HWMON)

> +umode_t toshiba_acpi_hwmon_is_visible(const void *drvdata,
> +				      enum hwmon_sensor_types type,
> +				      u32 attr, int channel)
> +{
> +	return 0444;
> +}
> +
> +int toshiba_acpi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long *val)
> +{
> +	/*
> +	 * There is only a single channel and single attribute (for the
> +	 * fan) at this point.
> +	 * This can be replaced with more advanced logic in the future,
> +	 * should the need arise.
> +	 */
> +	if (type == hwmon_fan && channel == 0 && attr == hwmon_fan_input) {
> +		u32 value;
> +		int ret;
> +
> +		ret = get_fan_rpm(toshiba_acpi, &value);
> +		if (ret)
> +			return ret;
> +
> +		*val = value;
> +		return 0;
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_channel_info *toshiba_acpi_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops toshiba_acpi_hwmon_ops = {
> +	.is_visible = toshiba_acpi_hwmon_is_visible,
> +	.read = toshiba_acpi_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info toshiba_acpi_hwmon_chip_info = {
> +	.ops = &toshiba_acpi_hwmon_ops,
> +	.info = toshiba_acpi_hwmon_info,
> +};
> +#endif
> +
>   static void print_supported_features(struct toshiba_acpi_dev *dev)
>   {
>   	pr_info("Supported laptop features:");
> @@ -2995,6 +3050,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>   
>   	remove_toshiba_proc_entries(dev);
>   
> +#ifdef CONFIG_HWMON

#if IS_REACHABLE()

> +	if (dev->hwmon_device)
> +		hwmon_device_unregister(dev->hwmon_device);
> +#endif
> +
>   	if (dev->accelerometer_supported && dev->indio_dev) {
>   		iio_device_unregister(dev->indio_dev);
>   		iio_device_free(dev->indio_dev);
> @@ -3187,6 +3247,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>   	ret = get_fan_rpm(dev, &dummy);
>   	dev->fan_rpm_supported = !ret;
>   
> +#ifdef CONFIG_HWMON


... and again.

> +	if (dev->fan_rpm_supported) {
> +		dev->hwmon_device = hwmon_device_register_with_info(
> +			&dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
> +			&toshiba_acpi_hwmon_chip_info, NULL);
> +		if (IS_ERR(dev->hwmon_device)) {
> +			dev->hwmon_device = NULL;
> +			pr_warn("unable to register hwmon device, skipping\n");
> +		}
> +	}
> +#endif
> +
>   	toshiba_wwan_available(dev);
>   	if (dev->wwan_supported)
>   		toshiba_acpi_setup_wwan_rfkill(dev);

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

[Arvid Norlander]

Hi,

On 2022-09-02 00:27, Guenter Roeck wrote:
> On 9/1/22 14:58, 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 | 72 +++++++++++++++++++++++++++++
>>   2 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index f2f98e942cf2..4d0d2676939a 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>>       depends on INPUT
>>       depends on SERIO_I8042 || SERIO_I8042 = n
>>       depends on ACPI_VIDEO || ACPI_VIDEO = n
>> +    depends on HWMON || HWMON = n
>>       depends on RFKILL || RFKILL = n
>>       depends on IIO
>>       select INPUT_SPARSEKMAP
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 02e3522f4eeb..a976dfb97a5e 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -46,6 +46,10 @@
>>   #include <linux/toshiba.h>
>>   #include <acpi/video.h>
>>   +#ifdef CONFIG_HWMON
>> +#include <linux/hwmon.h>
>> +#endif
> 
> ifdef not needed here.

I will fix this to v3. Being new to kernel development, the number of rules
and patterns is quite overwhelming.

> 
>> +
>>   MODULE_AUTHOR("John Belmonte");
>>   MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>>   MODULE_LICENSE("GPL");
>> @@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
>>       struct miscdevice miscdev;
>>       struct rfkill *wwan_rfk;
>>       struct iio_dev *indio_dev;
>> +#ifdef CONFIG_HWMON
>> +    struct device *hwmon_device;
>> +#endif
>>         int force_fan;
>>       int last_key_event;
>> @@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>       return 0;
>>   }
>>   +/* HWMON support for fan */
>> +#ifdef CONFIG_HWMON
> 
> This should be #if IS_REACHABLE(CONFIG_HWMON)

Hm okay. I copied the existing pattern of "#ifdef CONFIG_PM_SLEEP" around
toshiba_acpi_suspend/toshiba_acpi_resume(). I assumed the same pattern
could be reused here. I guess not. Is the reason that this one is tristate?
(Unlike CONFIG_PM_SLEEP.)

<snip>

Best regards,
Arvid Norlander

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

[Guenter Roeck]

On 9/1/22 15:51, Arvid Norlander wrote:
> Hi,
> 
> On 2022-09-02 00:27, Guenter Roeck wrote:
>> On 9/1/22 14:58, 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 | 72 +++++++++++++++++++++++++++++
>>>    2 files changed, 73 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index f2f98e942cf2..4d0d2676939a 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>>>        depends on INPUT
>>>        depends on SERIO_I8042 || SERIO_I8042 = n
>>>        depends on ACPI_VIDEO || ACPI_VIDEO = n
>>> +    depends on HWMON || HWMON = n
>>>        depends on RFKILL || RFKILL = n
>>>        depends on IIO
>>>        select INPUT_SPARSEKMAP
>>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>>> index 02e3522f4eeb..a976dfb97a5e 100644
>>> --- a/drivers/platform/x86/toshiba_acpi.c
>>> +++ b/drivers/platform/x86/toshiba_acpi.c
>>> @@ -46,6 +46,10 @@
>>>    #include <linux/toshiba.h>
>>>    #include <acpi/video.h>
>>>    +#ifdef CONFIG_HWMON
>>> +#include <linux/hwmon.h>
>>> +#endif
>>
>> ifdef not needed here.
> 
> I will fix this to v3. Being new to kernel development, the number of rules
> and patterns is quite overwhelming.
> 
>>
>>> +
>>>    MODULE_AUTHOR("John Belmonte");
>>>    MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>>>    MODULE_LICENSE("GPL");
>>> @@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
>>>        struct miscdevice miscdev;
>>>        struct rfkill *wwan_rfk;
>>>        struct iio_dev *indio_dev;
>>> +#ifdef CONFIG_HWMON
>>> +    struct device *hwmon_device;
>>> +#endif
>>>          int force_fan;
>>>        int last_key_event;
>>> @@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>>        return 0;
>>>    }
>>>    +/* HWMON support for fan */
>>> +#ifdef CONFIG_HWMON
>>
>> This should be #if IS_REACHABLE(CONFIG_HWMON)
> 
> Hm okay. I copied the existing pattern of "#ifdef CONFIG_PM_SLEEP" around
> toshiba_acpi_suspend/toshiba_acpi_resume(). I assumed the same pattern
> could be reused here. I guess not. Is the reason that this one is tristate?
> (Unlike CONFIG_PM_SLEEP.)
> 
Exactly.

Thanks,
Guenter

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

[Hans de Goede]

Hi Guenter, Arvid,

On 9/2/22 00:27, Guenter Roeck wrote:
> On 9/1/22 14:58, 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 | 72 +++++++++++++++++++++++++++++
>>   2 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index f2f98e942cf2..4d0d2676939a 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>>       depends on INPUT
>>       depends on SERIO_I8042 || SERIO_I8042 = n
>>       depends on ACPI_VIDEO || ACPI_VIDEO = n
>> +    depends on HWMON || HWMON = n
>>       depends on RFKILL || RFKILL = n
>>       depends on IIO
>>       select INPUT_SPARSEKMAP
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 02e3522f4eeb..a976dfb97a5e 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -46,6 +46,10 @@
>>   #include <linux/toshiba.h>
>>   #include <acpi/video.h>
>>   +#ifdef CONFIG_HWMON
>> +#include <linux/hwmon.h>
>> +#endif
> 
> ifdef not needed here.

Ack.

> 
>> +
>>   MODULE_AUTHOR("John Belmonte");
>>   MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>>   MODULE_LICENSE("GPL");
>> @@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
>>       struct miscdevice miscdev;
>>       struct rfkill *wwan_rfk;
>>       struct iio_dev *indio_dev;
>> +#ifdef CONFIG_HWMON
>> +    struct device *hwmon_device;
>> +#endif
>>         int force_fan;
>>       int last_key_event;
>> @@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>       return 0;
>>   }
>>   +/* HWMON support for fan */
>> +#ifdef CONFIG_HWMON
> 
> This should be #if IS_REACHABLE(CONFIG_HWMON)

Actually that should be IS_ENABLED since you suggested that
Arvid should use:

	depends on HWMON || HWMON = n

In the Kconfig bit there is no need for IS_REACHABLE,
note IS_REACHABLE will work too but I generally prefer
to avoid it because cases which actually need it lead
to weirdness where e.g. both HWMON and TOSHIBA_ACPI are
enabled yet TOSHIBA_ACPI will still not have HWMON
support.

Arvid, sorry about the "noise" here, let me try to
explain.

First of all lets explain this bit of magic:

	depends on HWMON || HWMON = n

What this says is that if HWMON is enabled it must
be able to satisfy dependencies on it in toshiba_acpi.c
(or it may also be fully disabled).

This magic is necessary to avoid a case where
toshiba_acpi gets build into the kernel, but the
hwmon code is a module. In that case linking errors
(unresolved hwmon symbols) will happen when building
the main vmlinuz kernel image.

So basically what this does is if HWMON is configured
as a module, it limits the choices for TOSHIBA_ACPI
to either n or m and disallows y.

I hope that so far I'm making sense...

So now to the #ifdef-ery. Since HWMON can be a module
when enabled the #define's from Kconfig will either
contain:

#define CONFIG_HWMON 1   // when builtin, HWMON=y

or:

#define CONFIG_HWMON_MODULE 1   // when a module, HWMON=m

So you would need to write:

#if defined CONFIG_HWMON || defined CONFIG_HWMON_MODULE

as a condition

#if IS_ENABLED(CONFIG_HWMON)

is a shorthand (macro) for this.

###

Now back to:

#if IS_REACHABLE(CONFIG_HWMON)

This is a special macro for when your Kconfig bit would just be:

	depends on HWMON

in that case TOSHIBA_ACPI might be set to builtin (y)
while the HWMON core/class code is a module. As mentioned
above that would lead to undefined hwmon symbols when
using "#if IS_ENABLED(CONFIG_HWMON)" as test. IS_REACHABLE
is special in that it will disable (evaluate to false)
in the case where the code being build is builtin and
the dependency is a module.

But that cannot happen here because your Kconfig bit is:

	depends on HWMON || HWMON = n

So "#if IS_ENABLED(CONFIG_HWMON)" is sufficient.

TL;DR: please use "#if IS_ENABLED(CONFIG_HWMON)" to test
if the hwmon code should be build.

Regards,

Hans











> 
>> +umode_t toshiba_acpi_hwmon_is_visible(const void *drvdata,
>> +                      enum hwmon_sensor_types type,
>> +                      u32 attr, int channel)
>> +{
>> +    return 0444;
>> +}
>> +
>> +int toshiba_acpi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> +                u32 attr, int channel, long *val)
>> +{
>> +    /*
>> +     * There is only a single channel and single attribute (for the
>> +     * fan) at this point.
>> +     * This can be replaced with more advanced logic in the future,
>> +     * should the need arise.
>> +     */
>> +    if (type == hwmon_fan && channel == 0 && attr == hwmon_fan_input) {
>> +        u32 value;
>> +        int ret;
>> +
>> +        ret = get_fan_rpm(toshiba_acpi, &value);
>> +        if (ret)
>> +            return ret;
>> +
>> +        *val = value;
>> +        return 0;
>> +    }
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_channel_info *toshiba_acpi_hwmon_info[] = {
>> +    HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_ops toshiba_acpi_hwmon_ops = {
>> +    .is_visible = toshiba_acpi_hwmon_is_visible,
>> +    .read = toshiba_acpi_hwmon_read,
>> +};
>> +
>> +static const struct hwmon_chip_info toshiba_acpi_hwmon_chip_info = {
>> +    .ops = &toshiba_acpi_hwmon_ops,
>> +    .info = toshiba_acpi_hwmon_info,
>> +};
>> +#endif
>> +
>>   static void print_supported_features(struct toshiba_acpi_dev *dev)
>>   {
>>       pr_info("Supported laptop features:");
>> @@ -2995,6 +3050,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>>         remove_toshiba_proc_entries(dev);
>>   +#ifdef CONFIG_HWMON
> 
> #if IS_REACHABLE()
> 
>> +    if (dev->hwmon_device)
>> +        hwmon_device_unregister(dev->hwmon_device);
>> +#endif
>> +
>>       if (dev->accelerometer_supported && dev->indio_dev) {
>>           iio_device_unregister(dev->indio_dev);
>>           iio_device_free(dev->indio_dev);
>> @@ -3187,6 +3247,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>       ret = get_fan_rpm(dev, &dummy);
>>       dev->fan_rpm_supported = !ret;
>>   +#ifdef CONFIG_HWMON
> 
> 
> ... and again.
> 
>> +    if (dev->fan_rpm_supported) {
>> +        dev->hwmon_device = hwmon_device_register_with_info(
>> +            &dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
>> +            &toshiba_acpi_hwmon_chip_info, NULL);
>> +        if (IS_ERR(dev->hwmon_device)) {
>> +            dev->hwmon_device = NULL;
>> +            pr_warn("unable to register hwmon device, skipping\n");
>> +        }
>> +    }
>> +#endif
>> +
>>       toshiba_wwan_available(dev);
>>       if (dev->wwan_supported)
>>           toshiba_acpi_setup_wwan_rfkill(dev);
> 

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

[Guenter Roeck]

On 9/2/22 01:29, Hans de Goede wrote:
> Hi Guenter, Arvid,
> 
> On 9/2/22 00:27, Guenter Roeck wrote:
>> On 9/1/22 14:58, 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 | 72 +++++++++++++++++++++++++++++
>>>    2 files changed, 73 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index f2f98e942cf2..4d0d2676939a 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>>>        depends on INPUT
>>>        depends on SERIO_I8042 || SERIO_I8042 = n
>>>        depends on ACPI_VIDEO || ACPI_VIDEO = n
>>> +    depends on HWMON || HWMON = n
>>>        depends on RFKILL || RFKILL = n
>>>        depends on IIO
>>>        select INPUT_SPARSEKMAP
>>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>>> index 02e3522f4eeb..a976dfb97a5e 100644
>>> --- a/drivers/platform/x86/toshiba_acpi.c
>>> +++ b/drivers/platform/x86/toshiba_acpi.c
>>> @@ -46,6 +46,10 @@
>>>    #include <linux/toshiba.h>
>>>    #include <acpi/video.h>
>>>    +#ifdef CONFIG_HWMON
>>> +#include <linux/hwmon.h>
>>> +#endif
>>
>> ifdef not needed here.
> 
> Ack.
> 
>>
>>> +
>>>    MODULE_AUTHOR("John Belmonte");
>>>    MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>>>    MODULE_LICENSE("GPL");
>>> @@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
>>>        struct miscdevice miscdev;
>>>        struct rfkill *wwan_rfk;
>>>        struct iio_dev *indio_dev;
>>> +#ifdef CONFIG_HWMON
>>> +    struct device *hwmon_device;
>>> +#endif
>>>          int force_fan;
>>>        int last_key_event;
>>> @@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>>        return 0;
>>>    }
>>>    +/* HWMON support for fan */
>>> +#ifdef CONFIG_HWMON
>>
>> This should be #if IS_REACHABLE(CONFIG_HWMON)
> 
> Actually that should be IS_ENABLED since you suggested that
> Arvid should use:
> 
> 	depends on HWMON || HWMON = n
> 
Yes, you are absolutely correct.

Thanks,
Guenter

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

[Arvid Norlander]

Hi,

On 2022-09-02 10:29, Hans de Goede wrote:
> Hi Guenter, Arvid,
<snip>
> 
> Actually that should be IS_ENABLED since you suggested that
> Arvid should use:
> 
> 	depends on HWMON || HWMON = n
> 
> In the Kconfig bit there is no need for IS_REACHABLE,
> note IS_REACHABLE will work too but I generally prefer
> to avoid it because cases which actually need it lead
> to weirdness where e.g. both HWMON and TOSHIBA_ACPI are
> enabled yet TOSHIBA_ACPI will still not have HWMON
> support.
> 
> Arvid, sorry about the "noise" here, let me try to
> explain.
> 
> First of all lets explain this bit of magic:
> 
> 	depends on HWMON || HWMON = n
> 
> What this says is that if HWMON is enabled it must
> be able to satisfy dependencies on it in toshiba_acpi.c
> (or it may also be fully disabled).
> 
> This magic is necessary to avoid a case where
> toshiba_acpi gets build into the kernel, but the
> hwmon code is a module. In that case linking errors
> (unresolved hwmon symbols) will happen when building
> the main vmlinuz kernel image.
> 
> So basically what this does is if HWMON is configured
> as a module, it limits the choices for TOSHIBA_ACPI
> to either n or m and disallows y.
> 
> I hope that so far I'm making sense...

Thanks, this clears up quite a bit of confusion.

> 
> So now to the #ifdef-ery. Since HWMON can be a module
> when enabled the #define's from Kconfig will either
> contain:
> 
> #define CONFIG_HWMON 1   // when builtin, HWMON=y
> 
> or:
> 
> #define CONFIG_HWMON_MODULE 1   // when a module, HWMON=m
> 
> So you would need to write:
> 
> #if defined CONFIG_HWMON || defined CONFIG_HWMON_MODULE
> 
> as a condition
> 
> #if IS_ENABLED(CONFIG_HWMON)
> 
> is a shorthand (macro) for this.
> 
> ###
> 
> Now back to:
> 
> #if IS_REACHABLE(CONFIG_HWMON)
> 
> This is a special macro for when your Kconfig bit would just be:
> 
> 	depends on HWMON
> 
> in that case TOSHIBA_ACPI might be set to builtin (y)
> while the HWMON core/class code is a module. As mentioned
> above that would lead to undefined hwmon symbols when
> using "#if IS_ENABLED(CONFIG_HWMON)" as test. IS_REACHABLE
> is special in that it will disable (evaluate to false)
> in the case where the code being build is builtin and
> the dependency is a module.
> 
> But that cannot happen here because your Kconfig bit is:
> 
> 	depends on HWMON || HWMON = n
> 
> So "#if IS_ENABLED(CONFIG_HWMON)" is sufficient.
> 
> TL;DR: please use "#if IS_ENABLED(CONFIG_HWMON)" to test
> if the hwmon code should be build.
> 
> Regards,
> 
> Hans

All of this will be useful to know in the future as well I imagine, so
thanks again.

<snip>

Best regards,
Arvid Norlander