[PATCH] platform/x86: panasonic-laptop: add fan speed mode for newer models

[PATCH] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
 drivers/platform/x86/panasonic-laptop.c | 98 +++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..3a8bbd633fd1 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -507,6 +507,76 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* on newer models (ex: CF-SR4), fan mode can be set */
+/* check if this is available */
+
+static acpi_status check_fan_mode_present(void)
+{
+	acpi_status status = AE_OK;
+	acpi_handle handle;
+
+	/* check if read and set are available */
+	/* read fan speed profile */
+	status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
+	if (ACPI_FAILURE(status))
+		goto out;
+	/* set fan speed profile */
+	status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
+	if (ACPI_FAILURE(status))
+		goto out;
+
+out:
+	return status;
+}
+
+/* get fan mode state */
+
+static int get_fan_mode_state(void)
+{
+	acpi_status status;
+	unsigned long long state;
+
+	/* bios default is zero which seems to be some sort of performance mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL, &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("evaluation error _SB.PC00.LPCB.EC0.CEFM\n");
+		state = -EIO;
+	}
+	int result = (int)state;
+	return result;
+}
+
+/* set fan mode */
+
+static int set_fan_mode_state(int new_state)
+{
+	int result;
+	acpi_status status;
+
+	result = get_fan_mode_state();
+	if (result < 0)
+		goto out;
+	if (new_state == result)
+		goto out;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = new_state;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM",
+			&input, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("_SB.PC00.LPCB.EC0.SEFM evaluation failed\n");
+		return -EINVAL;
+	}
+out:
+	return result;
+}
+
 
 /* sysfs user interface functions */
 
@@ -778,6 +848,29 @@ static ssize_t cdpower_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t fan_mode_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	int state = get_fan_mode_state();
+
+	if (state < 0)
+		return state;
+
+	return sysfs_emit(buf, "%d\n", state);
+}
+
+static ssize_t fan_mode_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	int err, val;
+
+	err = kstrtoint(buf, 10, &val);
+	if (err)
+		return err;
+	set_fan_mode_state(val);
+	return count;
+}
+
 static DEVICE_ATTR_RO(numbatt);
 static DEVICE_ATTR_RO(lcdtype);
 static DEVICE_ATTR_RW(mute);
@@ -787,6 +880,7 @@ static DEVICE_ATTR_RW(ac_brightness);
 static DEVICE_ATTR_RW(dc_brightness);
 static DEVICE_ATTR_RW(current_brightness);
 static DEVICE_ATTR_RW(cdpower);
+static DEVICE_ATTR_RW(fan_mode);
 
 static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
 {
@@ -803,6 +897,9 @@ static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr
 	if (attr == &dev_attr_current_brightness.attr)
 		return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0;
 
+	if (attr == &dev_attr_fan_mode.attr) /* mostly present on newer models */
+		return (ACPI_SUCCESS(check_fan_mode_present())) ? attr->mode : 0;
+
 	return attr->mode;
 }
 
@@ -816,6 +913,7 @@ static struct attribute *pcc_sysfs_entries[] = {
 	&dev_attr_dc_brightness.attr,
 	&dev_attr_current_brightness.attr,
 	&dev_attr_cdpower.attr,
+	&dev_attr_fan_mode.attr,
 	NULL,
 };
 
-- 
2.54.0

Re: [PATCH] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Ilpo Järvinen]

On Wed, 3 Jun 2026, Alex Yeo wrote:

> I have a CF-SR4 and Linux works out of the box. Compared to
> previous models, this one seems to have the fans running at high speed
> by default when the computer starts.
> 
> When Windows 11 is booted up, the high fan speeds persist just until the
> login screen. Once the login screen shows up, the fan spins down.
> 
> I had suspected that this might be the laptop ramping down the fans when
> the OS declares that it is Windows but this does not seem to be the
> case.
> 
> After analyzing the DSDT and SSDT of the computer, I have found the
> following:
> 
> File ssdt10.dsl under \_SB.PC00.LPCB.EC0:
> 
> Name (CEFM, Zero)
> Method (SEFM, 1, Serialized)
> {
>     CEFM = Arg0
>     REFM ()
> }
> 
> Method (REFM, 0, Serialized)
> {
>     If ((\S0IX == 0x03))
>     {
>         Local0 = 0x05
>     }
>     ElseIf ((CEFM == Zero))
>     {
>         Local0 = Zero
>     }
>     Else
>     {
>         Local0 = 0x02
>     }
> 
>     \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
> }
> 
> \_SB.PC00.LPCB.EC0.CEFM would seem be the current value
> for the fan profile. On startup, this is set to 0.
> 
> Based on the code SEFM seems the be the method to set the fan
> profile and REFM is executed right after.
> 
> I don't have access to information as to what the argument officially
> means but based on testing, any number above zero makes the fans spin
> down and behave like the older models where it stops or runs at low
> speed when its cool and ramps up when the processor gets hot.
> 
> The only relevant values for CEFM seem to be just 0 and any number above
> that just gets treated the same. I personally use just 0 and 1.
> 0 seems to be the high fan speed mode and 1 makes it behave like
> Windows.
> 
> Giving 0 as an argument reverts the fan back to the way it was during
> startup where the lowest fan speed is quite high and when load is
> applied, it seems to ramp up to an even higher speed which I think would
> be its 100%.
> 
> A value of 1 seems to have its max speed capped lower than 0.
> 
> For both modes, fan management is still automatic.
> 
> fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
> \_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
> show up on unsupported models. I have tried not hiding it and it just
> outputs a generic error when the value is read.
> 
> I also saw that variables such as eco_mode are kept in memory, however
> for fan_mode I rely on getting and setting the value via ACPI.
> 
> Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
> ---
>  drivers/platform/x86/panasonic-laptop.c | 98 +++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f88..3a8bbd633fd1 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
> @@ -507,6 +507,76 @@ static int set_optd_power_state(int new_state)
>  	return result;
>  }
>  
> +/* on newer models (ex: CF-SR4), fan mode can be set */
> +/* check if this is available */
> +
> +static acpi_status check_fan_mode_present(void)
> +{
> +	acpi_status status = AE_OK;

Unnecessary initialization.

> +	acpi_handle handle;
> +
> +	/* check if read and set are available */
> +	/* read fan speed profile */

The second comment line and the one below seems mostly duplicate 
information so please drop them. If you want, you can to the first line: 

	/* Check if read (CEFM) and set (SEFM) are available */

> +	status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
> +	if (ACPI_FAILURE(status))
> +		goto out;

You should return directly from here.

And please return something else than acpi_status from this function.

> +	/* set fan speed profile */
> +	status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
> +	if (ACPI_FAILURE(status))
> +		goto out;
> +
> +out:
> +	return status;
> +}
> +
> +/* get fan mode state */
> +

Remove blank line.

> +static int get_fan_mode_state(void)
> +{
> +	acpi_status status;
> +	unsigned long long state;

Please use reverse xmas-tree order.

> +
> +	/* bios default is zero which seems to be some sort of performance mode */

BIOS

> +	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL, &state);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("evaluation error _SB.PC00.LPCB.EC0.CEFM\n");
> +		state = -EIO;

Just return directly.

> +	}
> +	int result = (int)state;

This intermediate variable doesn't seem useful and is declared in wrong 
place anyway.

> +	return result;
> +}
> +
> +/* set fan mode */
> +
> +static int set_fan_mode_state(int new_state)
> +{
> +	int result;
> +	acpi_status status;

xmas-tree order.

> +
> +	result = get_fan_mode_state();
> +	if (result < 0)
> +		goto out;
> +	if (new_state == result)
> +		goto out;
> +
> +	union acpi_object param[1];
> +	struct acpi_object_list input;
> +
> +	param[0].type = ACPI_TYPE_INTEGER;
> +	param[0].integer.value = new_state;
> +	input.count = 1; /* takes one arg */
> +	input.pointer = param;
> +
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM",
> +			&input, NULL);

Please align to (

> +	if (ACPI_FAILURE(status)) {
> +		pr_err("_SB.PC00.LPCB.EC0.SEFM evaluation failed\n");

Please write the error messages so that they are meaningful for 
non-technical users.

Include for pr_err() is missing it seems.

> +		return -EINVAL;
> +	}
> +out:
> +	return result;
> +}
> +
>  
>  /* sysfs user interface functions */
>  
> @@ -778,6 +848,29 @@ static ssize_t cdpower_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> +static ssize_t fan_mode_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	int state = get_fan_mode_state();
> +
> +	if (state < 0)

As per kernel's coding style, call and its error handling should not have 
an empty line in between them (and a blank line is required after local 
var declarations) so you need to declarate variable first without 
assigning to it on the same line.

> +		return state;
> +
> +	return sysfs_emit(buf, "%d\n", state);

This file seems to be lacking sysfs.h, please add the include now.

> +}
> +
> +static ssize_t fan_mode_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	int err, val;
> +
> +	err = kstrtoint(buf, 10, &val);

kstrtouint()?

But I'm unsure whether sysfs is right place for this interface. acer-wmi 
driver seems to have fan mode auto/turbo selection using hwmon interface 
but this driver doesn't have hwmon as is...

> +	if (err)
> +		return err;
> +	set_fan_mode_state(val);
> +	return count;
> +}
> +
>  static DEVICE_ATTR_RO(numbatt);
>  static DEVICE_ATTR_RO(lcdtype);
>  static DEVICE_ATTR_RW(mute);
> @@ -787,6 +880,7 @@ static DEVICE_ATTR_RW(ac_brightness);
>  static DEVICE_ATTR_RW(dc_brightness);
>  static DEVICE_ATTR_RW(current_brightness);
>  static DEVICE_ATTR_RW(cdpower);
> +static DEVICE_ATTR_RW(fan_mode);
>  
>  static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
>  {
> @@ -803,6 +897,9 @@ static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr
>  	if (attr == &dev_attr_current_brightness.attr)
>  		return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0;
>  
> +	if (attr == &dev_attr_fan_mode.attr) /* mostly present on newer models */
> +		return (ACPI_SUCCESS(check_fan_mode_present())) ? attr->mode : 0;
> +
>  	return attr->mode;
>  }
>  
> @@ -816,6 +913,7 @@ static struct attribute *pcc_sysfs_entries[] = {
>  	&dev_attr_dc_brightness.attr,
>  	&dev_attr_current_brightness.attr,
>  	&dev_attr_cdpower.attr,
> +	&dev_attr_fan_mode.attr,
>  	NULL,
>  };
>  
> 

-- 
 i.

[RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
Thank you for taking the time to read the patch and give feedback.

I have revised the patch according to the comments.

However, for some parts I would like to clarify the following:

>Include for pr_err() is missing it seems.
>This file seems to be lacking sysfs.h, please add the include now.

For both pr_err() and sysfs.h they should be included as both are
referenced/used multiple times within the same file.

>kstrtouint()?

It takes in input via sysfs
something like:
$ echo 1 | sudo tee "/sys/devices/LNXSYSTM:00/LNXSYBUS:00/MAT0019:00/fan_mode"
and based on the other functions within the same file, the input
is expected to be a string which is converted into an int via kstrtouint()
as CEFM is an int

>But I'm unsure whether sysfs is right place for this interface. acer-wmi
>driver seems to have fan mode auto/turbo selection using hwmon interface
>but this driver doesn't have hwmon as is...

I do get what you mean. My personal goal is to have this work smoothly
with software like thermald as the vendor appears to use Intel Dynamic
Tuning Technology with non-standard ACPI methods to control the fan.
I will take a look into that and see if exposing the control via a
different way works better.

To summarize, it appears the OS calls SEFM to let the EC know the OS
has taken control of the fan. The fan then spins down until
it stops completely (level 0). This then allows the OS to set
the fan level from 0 to 100. If the processor gets too hot,
the EC intervenes and ramps up the fan.
From what I can gather, when running Windows, the vendor's driver
together with Intel DTT work together to control the fan.

Given this, I would like to ask what is the most appropriate
place in the kernel to expose this functionality?
I understand that hwmon was mentioned but I also see that
interacting with /sys/class/thermal may also be an option.

Thank you for your time.

 drivers/platform/x86/panasonic-laptop.c | 99 +++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..b05f11c223f9 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -507,6 +507,72 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* on newer models (ex: CF-SR4), fan mode can be set */
+/* check if this is available */
+
+static acpi_status check_fan_mode_present(void)
+{
+	acpi_handle handle;
+
+	/* check if read (CEFM) and set (SEFM) are available */
+	status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
+	if (ACPI_FAILURE(status))
+		return -ENOENT;
+	status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
+	if (ACPI_FAILURE(status))
+		return -ENOENT;
+
+	return 0;
+}
+
+/* get fan mode state */
+
+static int get_fan_mode_state(void)
+{
+	unsigned long long state;
+	acpi_status status;
+
+	/* BIOS default is zero which seems to be some sort of performance mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL, &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("evaluation error _SB.PC00.LPCB.EC0.CEFM\n");
+		return -EIO;
+	}
+
+	return (int)state;
+}
+
+/* set fan mode */
+
+static int set_fan_mode_state(int new_state)
+{
+	acpi_status status;
+	int result;
+
+	result = get_fan_mode_state();
+	if (result < 0)
+		return result;
+	if (new_state == result)
+		return result;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = new_state;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM",
+				      &input, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Setting of fan mode via _SB.PC00.LPCB.EC0.SEFM evaluation failed. You may have an unsupported model.\n");
+		return -EINVAL;
+	}
+
+	return result;
+}
+
 
 /* sysfs user interface functions */
 
@@ -778,6 +844,34 @@ static ssize_t cdpower_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t fan_mode_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	int state;
+
+	state = get_fan_mode_state();
+	if (state < 0)
+		return state;
+
+	return sysfs_emit(buf, "%d\n", state);
+}
+
+static ssize_t fan_mode_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	int status;
+	int err;
+	int val;
+
+	err = kstrtoint(buf, 10, &val);
+	if (err)
+		return err;
+
+	status = set_fan_mode_state(val);
+
+	return status;
+}
+
 static DEVICE_ATTR_RO(numbatt);
 static DEVICE_ATTR_RO(lcdtype);
 static DEVICE_ATTR_RW(mute);
@@ -787,6 +881,7 @@ static DEVICE_ATTR_RW(ac_brightness);
 static DEVICE_ATTR_RW(dc_brightness);
 static DEVICE_ATTR_RW(current_brightness);
 static DEVICE_ATTR_RW(cdpower);
+static DEVICE_ATTR_RW(fan_mode);
 
 static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
 {
@@ -803,6 +898,9 @@ static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr
 	if (attr == &dev_attr_current_brightness.attr)
 		return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0;
 
+	if (attr == &dev_attr_fan_mode.attr) /* mostly present on newer models */
+		return (ACPI_SUCCESS(check_fan_mode_present())) ? attr->mode : 0;
+
 	return attr->mode;
 }
 
@@ -816,6 +914,7 @@ static struct attribute *pcc_sysfs_entries[] = {
 	&dev_attr_dc_brightness.attr,
 	&dev_attr_current_brightness.attr,
 	&dev_attr_cdpower.attr,
+	&dev_attr_fan_mode.attr,
 	NULL,
 };
 
-- 
2.54.0

Re: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Ilpo Järvinen]

On Wed, 10 Jun 2026, Alex Yeo wrote:

> I have a CF-SR4 and Linux works out of the box. Compared to
> previous models, this one seems to have the fans running at high speed
> by default when the computer starts.
> 
> When Windows 11 is booted up, the high fan speeds persist just until the
> login screen. Once the login screen shows up, the fan spins down.
> 
> I had suspected that this might be the laptop ramping down the fans when
> the OS declares that it is Windows but this does not seem to be the
> case.
> 
> After analyzing the DSDT and SSDT of the computer, I have found the
> following:
> 
> File ssdt10.dsl under \_SB.PC00.LPCB.EC0:
> 
> Name (CEFM, Zero)
> Method (SEFM, 1, Serialized)
> {
>     CEFM = Arg0
>     REFM ()
> }
> 
> Method (REFM, 0, Serialized)
> {
>     If ((\S0IX == 0x03))
>     {
>         Local0 = 0x05
>     }
>     ElseIf ((CEFM == Zero))
>     {
>         Local0 = Zero
>     }
>     Else
>     {
>         Local0 = 0x02
>     }
> 
>     \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
> }
> 
> \_SB.PC00.LPCB.EC0.CEFM would seem be the current value
> for the fan profile. On startup, this is set to 0.
> 
> Based on the code SEFM seems the be the method to set the fan
> profile and REFM is executed right after.
> 
> I don't have access to information as to what the argument officially
> means but based on testing, any number above zero makes the fans spin
> down and behave like the older models where it stops or runs at low
> speed when its cool and ramps up when the processor gets hot.
> 
> The only relevant values for CEFM seem to be just 0 and any number above
> that just gets treated the same. I personally use just 0 and 1.
> 0 seems to be the high fan speed mode and 1 makes it behave like
> Windows.
> 
> Giving 0 as an argument reverts the fan back to the way it was during
> startup where the lowest fan speed is quite high and when load is
> applied, it seems to ramp up to an even higher speed which I think would
> be its 100%.
> 
> A value of 1 seems to have its max speed capped lower than 0.
> 
> For both modes, fan management is still automatic.
> 
> fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
> \_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
> show up on unsupported models. I have tried not hiding it and it just
> outputs a generic error when the value is read.
> 
> I also saw that variables such as eco_mode are kept in memory, however
> for fan_mode I rely on getting and setting the value via ACPI.
> 
> Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
> ---
> Thank you for taking the time to read the patch and give feedback.
> 
> I have revised the patch according to the comments.
> 
> However, for some parts I would like to clarify the following:
> 
> >Include for pr_err() is missing it seems.
> >This file seems to be lacking sysfs.h, please add the include now.
> 
> For both pr_err() and sysfs.h they should be included as both are
> referenced/used multiple times within the same file.

I know those are pre-existing problems. But they are still missing as 
direct includes and only become included indirectly through some other
header which is fragile if somebody wants to refactor those headers.

Thus, you should simply add the missing includes finally now when you add 
more code that uses stuff from those includes.

Obviously it would have been better to have the includes right from the 
beginning but we lack automatic tools to detect that and human reviewers 
are very fallible in checking them.

> >kstrtouint()?
> 
> It takes in input via sysfs
> something like:
> $ echo 1 | sudo tee "/sys/devices/LNXSYSTM:00/LNXSYBUS:00/MAT0019:00/fan_mode"
> and based on the other functions within the same file, the input
> is expected to be a string which is converted into an int via kstrtouint()
> as CEFM is an int

So what happens if you do echo -1 instead? Your changelog only talked 
about positive values.

It may well be other sysfs functions could use unsigned conversion variant 
too (I've not checked), but here you're adding new interface and it should 
be designed the best way possible. If negative numbers are not valid 
input, kstrtouint() does check that for you for free so why not use it?

Also, please leave more context visible next time when replying, I had to 
look it up from the other mail!

> >But I'm unsure whether sysfs is right place for this interface. acer-wmi
> >driver seems to have fan mode auto/turbo selection using hwmon interface
> >but this driver doesn't have hwmon as is...
> 
> I do get what you mean. My personal goal is to have this work smoothly
> with software like thermald as the vendor appears to use Intel Dynamic
> Tuning Technology with non-standard ACPI methods to control the fan.
> I will take a look into that and see if exposing the control via a
> different way works better.
> 
> To summarize, it appears the OS calls SEFM to let the EC know the OS
> has taken control of the fan. The fan then spins down until
> it stops completely (level 0). This then allows the OS to set
> the fan level from 0 to 100. If the processor gets too hot,
> the EC intervenes and ramps up the fan.

Sounds like pwm control to me. (Though, you haven't exactly described how 
that 0-100 control is done in OS but only about getting it to spin at low 
speed to match the speed user/you expected.)

> >From what I can gather, when running Windows, the vendor's driver
> together with Intel DTT work together to control the fan.
> 
> Given this, I would like to ask what is the most appropriate
> place in the kernel to expose this functionality?
> I understand that hwmon was mentioned but I also see that
> interacting with /sys/class/thermal may also be an option.

Can we somehow read how fast the fan currently spins so the information 
could be exposed through hwmon?

You may want to look more information about hwmon and pwm as to me it 
sounds pretty a good match to the fan control (based on your explanation 
about about OS controlling fan speed in 0-100 range).

-- 
 i.

Re: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models

[kernel test robot]

Hi Alex,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v7.1-rc7 next-20260609]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Yeo/platform-x86-panasonic-laptop-add-fan-speed-mode-for-newer-models/20260610-112852
base:   linus/master
patch link:    https://lore.kernel.org/r/20260610031756.546459-1-alexyeo362%40gmail.com
patch subject: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20260610/202606101253.EPD74jPt-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260610/202606101253.EPD74jPt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202606101253.EPD74jPt-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/platform/x86/panasonic-laptop.c: In function 'check_fan_mode_present':
>> drivers/platform/x86/panasonic-laptop.c:518:9: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
     518 |         status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
         |         ^~~~~~
         |         kstatfs
   drivers/platform/x86/panasonic-laptop.c:518:9: note: each undeclared identifier is reported only once for each function it appears in


vim +518 drivers/platform/x86/panasonic-laptop.c

   512	
   513	static acpi_status check_fan_mode_present(void)
   514	{
   515		acpi_handle handle;
   516	
   517		/* check if read (CEFM) and set (SEFM) are available */
 > 518		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
   519		if (ACPI_FAILURE(status))
   520			return -ENOENT;
   521		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
   522		if (ACPI_FAILURE(status))
   523			return -ENOENT;
   524	
   525		return 0;
   526	}
   527	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models

[kernel test robot]

Hi Alex,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v7.1-rc7 next-20260609]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Yeo/platform-x86-panasonic-laptop-add-fan-speed-mode-for-newer-models/20260610-112852
base:   linus/master
patch link:    https://lore.kernel.org/r/20260610031756.546459-1-alexyeo362%40gmail.com
patch subject: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260610/202606101201.Uas4V2ol-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project f43d6834093b19baf79beda8c0337ab020ac5f17)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260610/202606101201.Uas4V2ol-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202606101201.Uas4V2ol-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/platform/x86/panasonic-laptop.c:518:2: error: use of undeclared identifier 'status'
     518 |         status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
         |         ^~~~~~
   drivers/platform/x86/panasonic-laptop.c:519:19: error: use of undeclared identifier 'status'; did you mean 'static'?
     519 |         if (ACPI_FAILURE(status))
         |                          ^~~~~~
   drivers/platform/x86/panasonic-laptop.c:521:2: error: use of undeclared identifier 'status'
     521 |         status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
         |         ^~~~~~
   drivers/platform/x86/panasonic-laptop.c:522:19: error: use of undeclared identifier 'status'; did you mean 'static'?
     522 |         if (ACPI_FAILURE(status))
         |                          ^~~~~~
   4 errors generated.


vim +/status +518 drivers/platform/x86/panasonic-laptop.c

   512	
   513	static acpi_status check_fan_mode_present(void)
   514	{
   515		acpi_handle handle;
   516	
   517		/* check if read (CEFM) and set (SEFM) are available */
 > 518		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
   519		if (ACPI_FAILURE(status))
   520			return -ENOENT;
   521		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
   522		if (ACPI_FAILURE(status))
   523			return -ENOENT;
   524	
   525		return 0;
   526	}
   527	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models

[kernel test robot]

Hi Alex,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v7.1-rc7 next-20260610]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Yeo/platform-x86-panasonic-laptop-add-fan-speed-mode-for-newer-models/20260610-112852
base:   linus/master
patch link:    https://lore.kernel.org/r/20260610031756.546459-1-alexyeo362%40gmail.com
patch subject: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260612/202606120553.FYqbwN47-lkp@intel.com/config)
compiler: clang version 22.1.3 (https://github.com/llvm/llvm-project e9846648fd6183ee6d8cbdb4502213fcf902a211)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260612/202606120553.FYqbwN47-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202606120553.FYqbwN47-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/platform/x86/panasonic-laptop.c:518:2: error: use of undeclared identifier 'status'
     518 |         status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
         |         ^~~~~~
   drivers/platform/x86/panasonic-laptop.c:519:19: error: use of undeclared identifier 'status'; did you mean 'static'?
     519 |         if (ACPI_FAILURE(status))
         |                          ^~~~~~
   drivers/platform/x86/panasonic-laptop.c:521:2: error: use of undeclared identifier 'status'
     521 |         status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
         |         ^~~~~~
   drivers/platform/x86/panasonic-laptop.c:522:19: error: use of undeclared identifier 'status'; did you mean 'static'?
     522 |         if (ACPI_FAILURE(status))
         |                          ^~~~~~
   4 errors generated.


vim +/status +518 drivers/platform/x86/panasonic-laptop.c

   512	
   513	static acpi_status check_fan_mode_present(void)
   514	{
   515		acpi_handle handle;
   516	
   517		/* check if read (CEFM) and set (SEFM) are available */
 > 518		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
   519		if (ACPI_FAILURE(status))
   520			return -ENOENT;
   521		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
   522		if (ACPI_FAILURE(status))
   523			return -ENOENT;
   524	
   525		return 0;
   526	}
   527	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models

[kernel test robot]

Hi Alex,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v7.1-rc7 next-20260610]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Yeo/platform-x86-panasonic-laptop-add-fan-speed-mode-for-newer-models/20260610-112852
base:   linus/master
patch link:    https://lore.kernel.org/r/20260610031756.546459-1-alexyeo362%40gmail.com
patch subject: [RFC PATCH v2] platform/x86: panasonic-laptop: add fan speed mode for newer models
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20260612/202606120736.VmZFcjrX-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260612/202606120736.VmZFcjrX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202606120736.VmZFcjrX-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/platform/x86/panasonic-laptop.c: In function 'check_fan_mode_present':
>> drivers/platform/x86/panasonic-laptop.c:518:9: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
     518 |         status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
         |         ^~~~~~
         |         kstatfs
   drivers/platform/x86/panasonic-laptop.c:518:9: note: each undeclared identifier is reported only once for each function it appears in


vim +518 drivers/platform/x86/panasonic-laptop.c

   512	
   513	static acpi_status check_fan_mode_present(void)
   514	{
   515		acpi_handle handle;
   516	
   517		/* check if read (CEFM) and set (SEFM) are available */
 > 518		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", &handle);
   519		if (ACPI_FAILURE(status))
   520			return -ENOENT;
   521		status = acpi_get_handle(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &handle);
   522		if (ACPI_FAILURE(status))
   523			return -ENOENT;
   524	
   525		return 0;
   526	}
   527	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[RFC PATCH v3] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
> Can we somehow read how fast the fan currently spins so the information
> could be exposed through hwmon?
> 
> You may want to look more information about hwmon and pwm as to me it
> sounds pretty a good match to the fan control (based on your explanation
> about about OS controlling fan speed in 0-100 range).

Thank you for pointing out hwmon and pwm. I have made significant
changes and rewrote the fan control so its registered as an hwmon pwm
device. I have also added it to the thermal subsystem.
All the fan sysfs functions have been removed.

> Thus, you should simply add the missing includes finally now when you add
> more code that uses stuff from those includes.

I have also added the missing includes:
> +#include <linux/printk.h>
> +#include <linux/sysfs.h>

And I have added the following as I intend to use hwmon, thermal and
DMI:
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/thermal.h>
> +#include <linux/dmi.h>

I have added this for the errors I used such as EOPNOTSUPP:
> +#include <asm-generic/errno.h>

To play it safe, I think I should only assume that the fan works for my
specific model. I have looked into the other drivers and how this can be
implemented . Using quirks seems to be the standard way.

> +/* DMI matching for quirks copied from asus-nb-wmi.c */
> +static int dmi_matched(const struct dmi_system_id *dmi)
> +{
> +	pr_info("Identified laptop model '%s'\n", dmi->ident);
> +	quirks = dmi->driver_data;
> +	return 1;
> +}
> +
> +static const struct dmi_system_id pcc_quirks[] = {
> +	{
> +		.callback = dmi_matched,
> +		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
> +		},
> +		.driver_data = &quirk_cf_sr4,
> +	},
> +	{},
>  };

I had based it from asus-nb-wmi.c
The hwmon and thermal cooling (cdev) device registration will not happen
unless the vendor and product name match.

> +	/* use hwmon convention for pwm_mode */
> +	if (state == ACPI_PCC_FAN_PWM_AUTO) {
> +		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
> +	} else if (state != 0 && state > 0) {
> +		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
> +	} else {

In this part, I originally used the actual numerical values for the fan
being in EC mode and in manual mode. However, it may be confusing so I
declared it:

> +#define ACPI_PCC_FAN_PWM_AUTO	0x00
> +#define ACPI_PCC_FAN_PWM_MANUAL	0x01
> +#define HWMON_PCC_FAN_PWM_AUTO	0x00
> +#define HWMON_PCC_FAN_PWM_MANUAL	0x01

The values are the same but I made the distinction between the hwmon
standard value and what the ACPI function returns.
This might be useful for readability or if it is changed
or modified in the future.

> +	struct thermal_cooling_device	*pwm_fan_cdev;

While I did register the hwmon device with devm so it does not need
cleanup, this is not the case with the thermal cooling device. I added
it to the pcc_acpi struct so it can be properly cleaned up when the
driver is unloaded.

Currently, with an unmodified driver, there is a fan exposed by ACPI but
it always reads N/A when viewed using the sensors command:

acpi_fan-isa-0000
Adapter: ISA adapter
fan1:             N/A

The output is the same regardless of whether or not the fan is
controlled by the EC or manually.

When the computer starts up, control is always handed over to the
EC. Even on a restart, the BIOS will switch it back to EC control.
When a Linux OS boots up, it is expected that the PWM fan will read
N/A as the value from _FST (fan status) is not related at all to the
actual speed the fan is running.

panasonic_pwm_fan-isa-0000
Adapter: ISA adapter
pwm1:              N/A

When in manual mode, it will look like the ff:

panasonic_pwm_fan-isa-0000
Adapter: ISA adapter
pwm1:              0%  MANUAL CONTROL

I do have a question I want to ask regarding fan control. There is an
ACPI command that needs to be called first to set the fan to manual
control. When in EC mode, setting the fan speed via _FSL will not do
anything. This is why the code will check the current fan mode first
before setting the fan level.

Should the code automatically set the fan to manual mode first when the
fan speed is set?

For example,
$ echo 50 | sudo tee "/sys/devices/platform/MAT0019:00/hwmon/hwmon6/pwm1"

If the following is executed and the fan is still controlled by the EC,
should the driver automatically run the command to switch the fan into
manual mode? The current behavior is it does for both hwmon and thermal.

For the hwmon and thermal device I have named it the following:
hwmon: "panasonic_pwm_fan"
> +		hwmon_dev = devm_hwmon_device_register_with_info(
> +			&pdev->dev, "panasonic_pwm_fan", NULL,
> +			&pcc_pwm_fan_hwmon_chip_info, NULL);
thermal: "Panasonic_PWM_Fan"
> +		pcc->pwm_fan_cdev = thermal_cooling_device_register(
> +			"Panasonic_PWM_Fan", NULL, &pcc_pwm_fan_cdev_ops);

If it would be more helpful I can put these into a #define at the top of
the file.

I am currently using this patch on my computer and it works with
thermald. Though I need to specify which cooling device type to use.

The way the fan works:
\_SB.PC00.LPCB.EC0.SEFM - 0 for EC and 1 for manual (there is a 5 but
that seems to be reserved for S01X sleep)

\_SB.PC00.LPCB.EC0.TFN1._FSL - accepts 0-100 and appears to be the
standard _FSL function defined in the ACPI spec but in a non-standard
path. If set without setting the fan mode, it is ignored by the system.
0 turns off the fan and 100 sets it to full speed. When the command is
given, the EC slowly ramps up the fan speed (not instant).

\_SB.PC00.LPCB.EC0.TFN1._FST - the second element contains the current
fan speed. This is only reflective of actual speed when the fan mode is
set to manual.

When in manual mode, the EC intervenes to turn the fan on if the
processor gets hot (around ~90 degrees Celsius).

The fan would be controlled like this from the OS:
In the case of hwmon:
$ echo 0 | sudo tee /sys/class/hwmon/hwmon6/pwm1 # turn fan off
$ echo 255 | sudo tee /sys/class/hwmon/hwmon6/pwm1 # full speed
$ echo 70 | sudo tee /sys/class/hwmon/hwmon6/pwm1 # any value in between
$ echo 0 | sudo tee /sys/class/hwmon/hwmon6/pwm1_enable # EC mode
$ echo 1 | sudo tee /sys/class/hwmon/hwmon6/pwm1_enable # manual mode

Setting a speed via hwmon will automatically put the fan into manual
mode. This will be reflected when pwm1_enable is called.

For thermal:
$ echo 0 | sudo tee /sys/class/thermal/cooling_device14/cur_state # off
$ echo 100 | sudo tee /sys/class/thermal/cooling_device14/cur_state # max
$ echo 50 | sudo tee /sys/class/thermal/cooling_device14/cur_state # 50%

The driver always queries the current state of the fan (both the fan
speed and the fan mode) from ACPI and does not retain the state within
the module.

To summarize, I have added hwmon and thermal for the fan. To prevent
this from affecting other models, I have implemented quirks that use DMI
vendor and product name.

For thermald I currently use the thermal subsystem to control the fan
via trip points. With this patch and thermald, it is now set and forget on my
computer (the fan is not stuck at ~80% anymore).

I have also added one more modification that was not mentioned
earlier.

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

I have added this at the top of the file so that messages from this
module are more easily identified.

> [93927.430133] Unknown hotkey event: 0x0031
I get a lot of these on dmesg and having it be:
> [93927.430133] panasonic_laptop: Unknown hotkey event: 0x0031
instead makes it easier to determine where this is from.
(happens when I press the fn key)


> So what happens if you do echo -1 instead? Your changelog only talked
> about positive values.

> +	/* check if within bounds */
> +	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
> +		pr_err("%s: error: fan speed level out of bounds\n", __func__);
> +		result = -EIO;
> +		goto out;
> +	}

For this one I have added checks to make sure only values 0-100 are set.
Its not sysfs input so kstrtouint is not used anymore.

Thank you for taking a look.

 drivers/platform/x86/panasonic-laptop.c | 461 +++++++++++++++++++++++-
 1 file changed, 447 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..59419cdd857e 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -119,6 +119,9 @@
  *		- v0.1  start from toshiba_acpi driver written by John Belmonte
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/printk.h>
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/bits.h>
@@ -136,6 +139,12 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <acpi/video.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/thermal.h>
+#include <linux/dmi.h>
+#include <asm-generic/errno.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
@@ -164,6 +173,17 @@ MODULE_LICENSE("GPL");
 
 #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
 
+/* Define fan PWM modes */
+#define ACPI_PCC_FAN_PWM_AUTO		0x00
+#define ACPI_PCC_FAN_PWM_MANUAL	0x01
+#define HWMON_PCC_FAN_PWM_AUTO		0x00
+#define HWMON_PCC_FAN_PWM_MANUAL	0x01
+
+/* Define quirks for this driver */
+struct quirk_entry {
+	bool has_ospm_pwm_fan;
+};
+
 /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
    ECO_MODEs: 0x03 = off, 0x83 = on
 */
@@ -238,19 +258,50 @@ static const struct key_entry panasonic_keymap[] = {
 };
 
 struct pcc_acpi {
-	acpi_handle		handle;
-	unsigned long		num_sifr;
-	int			sticky_key;
-	int			eco_mode;
-	int			mute;
-	int			ac_brightness;
-	int			dc_brightness;
-	int			current_brightness;
-	u32			*sinf;
-	struct acpi_device	*device;
-	struct input_dev	*input_dev;
-	struct backlight_device	*backlight;
-	struct platform_device	*platform;
+	acpi_handle			handle;
+	unsigned long			num_sifr;
+	int				sticky_key;
+	int				eco_mode;
+	int				mute;
+	int				ac_brightness;
+	int				dc_brightness;
+	int				current_brightness;
+	u32				*sinf;
+	struct acpi_device		*device;
+	struct input_dev		*input_dev;
+	struct backlight_device		*backlight;
+	struct platform_device		*platform;
+	struct thermal_cooling_device	*pwm_fan_cdev;
+};
+
+/*
+ * Declare quirks and apply matches
+ */
+static struct quirk_entry *quirks;
+
+static struct quirk_entry quirk_cf_sr4 = {
+	.has_ospm_pwm_fan = true,
+};
+
+/* DMI matching for quirks copied from asus-nb-wmi.c */
+static int dmi_matched(const struct dmi_system_id *dmi)
+{
+	pr_info("Identified laptop model '%s'\n", dmi->ident);
+	quirks = dmi->driver_data;
+	return 1;
+}
+
+static const struct dmi_system_id pcc_quirks[] = {
+	{
+		.callback = dmi_matched,
+		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
+		},
+		.driver_data = &quirk_cf_sr4,
+	},
+	{},
 };
 
 /*
@@ -415,7 +466,6 @@ static const struct backlight_ops pcc_backlight_ops = {
 	.update_status	= bl_set_status,
 };
 
-
 /* returns ACPI_SUCCESS if methods to control optical drive are present */
 
 static acpi_status check_optd_present(void)
@@ -507,6 +557,179 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* get OSPM fan mode */
+
+static int pcc_pwm_fan_mode_read(int *pwm_mode)
+{
+	unsigned long long state;
+	acpi_status status;
+	int result;
+
+	/* BIOS default is zero which seems to be some sort of failsafe mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
+				       &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot get fan mode via CEFM\n");
+		result = -EIO;
+		goto out;
+	}
+
+	/* use hwmon convention for pwm_mode */
+	if (state == ACPI_PCC_FAN_PWM_AUTO) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
+	} else if (state != 0 && state > 0) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
+	} else {
+		/* unknown */
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
+
+/* set OSPM fan mode */
+
+static int pcc_pwm_fan_mode_set(int pwm_mode)
+{
+	acpi_status status;
+	int result;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = pwm_mode;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
+				      NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot set fan mode via SEFM\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
+
+/* read PWM fan speed */
+
+static int pcc_pwm_fan_speed_read(int *pwm_speed)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+
+	/* Get fan status first */
+	/* If fan is not in manual mode, it will return a bogus value */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		result = -ENODATA; /* Indeterminate value */
+		goto out;
+	}
+
+	/* get pwm speed */
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
+				      NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get pwm speed\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	obj = buffer.pointer;
+
+	/* the structure should have 3 values */
+	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
+		pr_err("Invalid _FST package structure (expected 3)\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	/* the second element should be the pwm speed as an int */
+	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		pr_err("_FST element at index 1 is not an integer\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	*pwm_speed = obj->package.elements[1].integer.value;
+	result = AE_OK;
+
+out:
+	kfree(buffer.pointer);
+	return result;
+}
+
+/* set PWM fan speed */
+
+static int pcc_pwm_fan_speed_set(int set_pwm_speed)
+{
+	struct acpi_object_list input;
+	union acpi_object param[1];
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+	/* Get fan status. set to manual if not set */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: set fan PWM to manual failed\n", __func__);
+			result = -EIO;
+			goto out;
+		}
+	}
+
+	/* check if within bounds */
+	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
+		pr_err("%s: error: fan speed level out of bounds\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = set_pwm_speed;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
+				      &input, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Setting of fan speed via ._FSL failed.\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
 
 /* sysfs user interface functions */
 
@@ -676,6 +899,7 @@ static ssize_t ac_brightness_show(struct device *dev, struct device_attribute *a
 	return sysfs_emit(buf, "%u\n", pcc->sinf[SINF_AC_CUR_BRIGHT]);
 }
 
+
 static ssize_t ac_brightness_store(struct device *dev, struct device_attribute *attr,
 				   const char *buf, size_t count)
 {
@@ -825,6 +1049,178 @@ static const struct attribute_group pcc_attr_group = {
 	.is_visible	= pcc_sysfs_is_visible,
 };
 
+/* hwmon */
+
+static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE), NULL
+};
+
+static int pcc_pwm_fan_hwmon_read(struct device *dev,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel, long *val)
+{
+	acpi_status status;
+	int pwm_speed;
+	int pwm_mode;
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		status = pcc_pwm_fan_speed_read(&pwm_speed);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to get fan pwm speed\n", __func__);
+			return -ENODATA;
+		}
+		*val = (pwm_speed * 255) / 100;
+		return 0;
+	case hwmon_pwm_enable:
+		status = pcc_pwm_fan_mode_read(&pwm_mode);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to get fan pwm mode\n", __func__);
+			return -ENODATA;
+		}
+		if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+			*val = HWMON_PCC_FAN_PWM_AUTO;
+		} else if (pwm_mode == HWMON_PCC_FAN_PWM_MANUAL) {
+			*val = HWMON_PCC_FAN_PWM_MANUAL;
+		} else { /* Invalid value */
+			return -EIO;
+		}
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_write(struct device *dev,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel, long val)
+{
+	int pwm_speed_vendor_val;
+	acpi_status status;
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		if (val == HWMON_PCC_FAN_PWM_AUTO) {
+			status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_AUTO);
+			if (ACPI_FAILURE(status)) {
+				pr_err("%s: failed to set fan to auto mode\n", __func__);
+				return -EIO;
+			}
+			return 0;
+		} else if (val == HWMON_PCC_FAN_PWM_MANUAL) {
+			status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
+			if (ACPI_FAILURE(status)) {
+				pr_err("%s: failed to set fan to manual mode\n", __func__);
+				return -EIO;
+			}
+			return 0;
+		}
+		return -EINVAL;
+	case hwmon_pwm_input:
+		val = clamp_val(val, 0, 255);
+		pwm_speed_vendor_val = (val * 100) / 255;
+
+		status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to set fan pwm\n", __func__);
+			return -EIO;
+		}
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
+					    enum hwmon_sensor_types type,
+					    u32 attr, int channel)
+{
+	if (type != hwmon_pwm)
+		return 0;
+
+	return 0644;
+}
+
+static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
+	.is_visible = pcc_pwm_fan_hwmon_is_visible,
+	.read = pcc_pwm_fan_hwmon_read,
+	.write = pcc_pwm_fan_hwmon_write,
+};
+
+static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
+	.ops = &pcc_pwm_fan_hwmon_ops,
+	.info = pcc_pwm_fan_hwmon_info,
+};
+
+/* thermal interface */
+static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	*state = 100; /* range of 0-100 as per UEFI spec */
+	return 0;
+}
+static int
+pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
+				    unsigned long *state)
+{
+	acpi_status status;
+	int current_pwm;
+	int pwm_mode;
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		return -ENODATA;
+	}
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		*state = 100; /* Return failsafe value on EC mode */
+		return 0;
+	}
+
+	status = pcc_pwm_fan_speed_read(&current_pwm);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		return -ENODATA;
+	}
+
+	if (current_pwm > 100)
+		current_pwm = 100;
+	if (current_pwm < 0)
+		return -EINVAL;
+
+	*state = current_pwm;
+
+	return 0;
+}
+static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
+					   unsigned long state)
+{
+	acpi_status status;
+	int set_pwm;
+
+	if (state > 100 || state < 0)
+		return -EINVAL;
+
+	set_pwm = state;
+	status = pcc_pwm_fan_speed_set(set_pwm);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to set fan speed pwm\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
+	.get_max_state = pcc_pwm_fan_thermal_max_state,
+	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
+	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
+};
 
 /* hotkey input device driver */
 
@@ -981,8 +1377,10 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
 
 static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 {
+	const struct dmi_system_id *dmi_id;
 	struct backlight_properties props;
 	struct acpi_device *device;
+	struct device *hwmon_dev;
 	struct pcc_acpi *pcc;
 	int num_sifr, result;
 
@@ -1026,6 +1424,17 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
 	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
 
+	/*
+	 * Perform quirk detection
+	 */
+	dmi_id = dmi_first_match(pcc_quirks);
+	if (dmi_id) {
+		pcc->quirks = dmi_id->driver_data;
+		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
+	} else {
+		pcc->quirks = NULL;
+	}
+
 	result = acpi_pcc_init_input(pcc);
 	if (result) {
 		pr_err("Error installing keyinput handler\n");
@@ -1097,6 +1506,27 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	}
 
 	i8042_install_filter(panasonic_i8042_filter, NULL);
+
+	/* has_ospm_pwm_fan - add hwmon and thermal if present */
+	if (pcc->quirks->has_ospm_pwm_fan) {
+		pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
+		hwmon_dev = devm_hwmon_device_register_with_info(
+			&pdev->dev, "panasonic_pwm_fan", NULL,
+			&pcc_pwm_fan_hwmon_chip_info, NULL);
+		if (IS_ERR(hwmon_dev)) {
+			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
+			return PTR_ERR(hwmon_dev);
+		}
+
+		pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
+		pcc->pwm_fan_cdev = thermal_cooling_device_register(
+			"Panasonic_PWM_Fan", NULL, &pcc_pwm_fan_cdev_ops);
+		if (IS_ERR(pcc->pwm_fan_cdev)) {
+			pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
+			pcc->pwm_fan_cdev = NULL;
+		}
+	}
+
 	return 0;
 
 out_platform:
@@ -1124,6 +1554,9 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct pcc_acpi *pcc = acpi_driver_data(device);
 
+	if (pcc->pwm_fan_cdev)
+		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
+
 	i8042_remove_filter(panasonic_i8042_filter);
 
 	if (pcc->platform) {
-- 
2.54.0

Re: [RFC PATCH v3] platform/x86: panasonic-laptop: add fan speed mode for newer models

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference on hardware without DMI quirks.
- [High] Resource leak and potential Use-After-Free due to direct return on probe error.
- [High] Race condition between hwmon sysfs and thermal cooling device operations.
- [Medium] Violation of hwmon ABI for 'pwm_enable' attribute.
- [Low] Compilation error due to missing 'quirks' field in 'struct pcc_acpi'.
- [Low] Always-false boundary check due to unsigned comparison.
- [Low] Idiomatically incorrect header inclusion for errno.
--

commit 62f80e95f10c4cb2e0e4a1e3a89a9f92dd8d3a2e
Author: Alex Yeo <alexyeo362@gmail.com>
Subject: platform/x86: panasonic-laptop: add fan speed mode for newer models

Summary: This patch adds fan speed mode support for newer Panasonic laptop
models, registering hwmon and thermal cooling devices. It relies on DMI quirks
to enable this feature and evaluates ACPI methods for retrieving and setting
the fan speeds.

> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f885..7774620b27635 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
> @@ -136,6 +139,12 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <acpi/video.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/thermal.h>
> +#include <linux/dmi.h>
> +#include <asm-generic/errno.h>

[Severity: Low]
Should this include <linux/errno.h> instead? It is usually preferred for
device drivers to use the generic linux header rather than the
architecture-specific one to ensure cross-architecture compatibility.

[ ... ]

>  struct pcc_acpi {
> -	acpi_handle		handle;
> -	unsigned long		num_sifr;
[ ... ]
> -	struct platform_device	*platform;
> +	acpi_handle			handle;
[ ... ]
> +	struct platform_device		*platform;
> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +};

[Severity: Low]
Does struct pcc_acpi miss the definition for the 'quirks' field? Later in
the patch, the probe function assigns to pcc->quirks, which will cause a
compilation error since it is not defined in this struct.

[ ... ]

> +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> +{
> +	struct acpi_object_list input;
> +	union acpi_object param[1];
> +	acpi_status status;
> +	int pwm_mode;
> +	int result;
> +
> +	/* Get fan status. set to manual if not set */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
[ ... ]
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
[ ... ]
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
> +				      &input, NULL);

[Severity: High]
Could this sequence race with other operations? This multi-step process
of reading the mode, changing it, and setting the speed isn't protected
by a lock. Since the thermal subsystem might poll this while userspace
writes to hwmon sysfs concurrently, could this interleave and corrupt the
hardware state?

[ ... ]

> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
[ ... ]
> +	case hwmon_pwm_enable:
> +		status = pcc_pwm_fan_mode_read(&pwm_mode);
[ ... ]
> +		if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +			*val = HWMON_PCC_FAN_PWM_AUTO;
> +		} else if (pwm_mode == HWMON_PCC_FAN_PWM_MANUAL) {

[Severity: Medium]
Is this returning the correct hwmon ABI value for automatic mode?
HWMON_PCC_FAN_PWM_AUTO is defined as 0, but the standard hwmon ABI expects
2 for automatic mode (and 1 for manual). This might confuse standard
userspace tools like fancontrol. This same ABI mismatch appears in
pcc_pwm_fan_hwmon_write() as well.

[ ... ]

> +static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
> +					   unsigned long state)
> +{
> +	acpi_status status;
> +	int set_pwm;
> +
> +	if (state > 100 || state < 0)
> +		return -EINVAL;

[Severity: Low]
Can 'state < 0' ever evaluate to true here? Since 'state' is an unsigned
long, it cannot be negative, making this check mathematically impossible.

[ ... ]

> @@ -981,8 +1377,10 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
>  
>  static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  {
[ ... ]
> +	/*
> +	 * Perform quirk detection
> +	 */
> +	dmi_id = dmi_first_match(pcc_quirks);
> +	if (dmi_id) {
> +		pcc->quirks = dmi_id->driver_data;
> +		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
> +	} else {
> +		pcc->quirks = NULL;
> +	}

[ ... ]

> +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +	if (pcc->quirks->has_ospm_pwm_fan) {

[Severity: High]
Will this dereference a NULL pointer on unsupported models? If the DMI
match fails, pcc->quirks is explicitly set to NULL above. Checking
pcc->quirks->has_ospm_pwm_fan right after will cause a kernel panic
during the device probe on those unquirked devices.

> +		pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> +		hwmon_dev = devm_hwmon_device_register_with_info(
> +			&pdev->dev, "panasonic_pwm_fan", NULL,
> +			&pcc_pwm_fan_hwmon_chip_info, NULL);
> +		if (IS_ERR(hwmon_dev)) {
> +			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +			return PTR_ERR(hwmon_dev);
> +		}

[Severity: High]
Does this direct return bypass the error cleanup sequence? Standard failure
paths in this function use 'goto out_notify_handler;' (and other labels) to
clean up resources. Returning directly leaks memory and leaves the ACPI
notify handler and input devices registered. If the module is later
unloaded, could this trigger a use-after-free when ACPI events fire?

> +
> +		pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> +		pcc->pwm_fan_cdev = thermal_cooling_device_register(
> +			"Panasonic_PWM_Fan", NULL, &pcc_pwm_fan_cdev_ops);

[Severity: High]
Does registering the thermal cooling device directly bypass the hwmon core
serialization? The subsystem guidelines indicate that thermal and sysfs
operations are serialized by the hwmon core if attributes are registered
via the 'info' parameter. Registering this manually out-of-band seems to
leave the thermal polling operations unprotected, exposing the race
condition in pcc_pwm_fan_speed_set() mentioned earlier.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625115034.773908-1-alexyeo362@gmail.com?part=1

[RFC PATCH v4] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
> - [High] Unconditional NULL pointer dereference on hardware without DMI quirks.
> - [High] Resource leak and potential Use-After-Free due to direct return on probe error.
> - [High] Race condition between hwmon sysfs and thermal cooling device operations.
> - [Medium] Violation of hwmon ABI for 'pwm_enable' attribute.
> - [Low] Compilation error due to missing 'quirks' field in 'struct pcc_acpi'.
> - [Low] Always-false boundary check due to unsigned comparison.
> - [Low] Idiomatically incorrect header inclusion for errno.

I have made changes to the code based on the feedback from sashiko-bot.

> > diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> > index b83113c26f885..7774620b27635 100644
> > --- a/drivers/platform/x86/panasonic-laptop.c
> > +++ b/drivers/platform/x86/panasonic-laptop.c
> > @@ -136,6 +139,12 @@
> >  #include <linux/types.h>
> >  #include <linux/uaccess.h>
> >  #include <acpi/video.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/thermal.h>
> > +#include <linux/dmi.h>
> > +#include <asm-generic/errno.h>
> 
> [Severity: Low]
> Should this include <linux/errno.h> instead? It is usually preferred for
> device drivers to use the generic linux header rather than the
> architecture-specific one to ensure cross-architecture compatibility.

The following change has been made:

> +#include <linux/errno.h>

> > diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> > index b83113c26f885..7774620b27635 100644
> > --- a/drivers/platform/x86/panasonic-laptop.c
> > +++ b/drivers/platform/x86/panasonic-laptop.c
> > @@ -136,6 +139,12 @@
> >  #include <linux/types.h>
> >  #include <linux/uaccess.h>
> >  #include <acpi/video.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/thermal.h>
> > +#include <linux/dmi.h>
> > +#include <asm-generic/errno.h>
> 
> [Severity: Low]
> Should this include <linux/errno.h> instead? It is usually preferred for
> device drivers to use the generic linux header rather than the
> architecture-specific one to ensure cross-architecture compatibility.

For this one, this has been corrected by adding the missing variable
and has been tested to compile and install.

> +	struct quirk_entry		*quirks;

> > +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> > +{
> > +     struct acpi_object_list input;
> > +     union acpi_object param[1];
> > +     acpi_status status;
> > +     int pwm_mode;
> > +     int result;
> > +
> > +     /* Get fan status. set to manual if not set */
> > +     status = pcc_pwm_fan_mode_read(&pwm_mode);
> [ ... ]
> > +     if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> > +             status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
> [ ... ]
> > +     status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
> > +                                   &input, NULL);
> 
> [Severity: High]
> Could this sequence race with other operations? This multi-step process
> of reading the mode, changing it, and setting the speed isn't protected
> by a lock. Since the thermal subsystem might poll this while userspace
> writes to hwmon sysfs concurrently, could this interleave and corrupt the
> hardware state?

For this one I have implemented a mutex to make sure hwmon and thermal
don't conflict with each other.

> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +	struct device			*pwm_fan_hwmon;
> +	struct mutex			pwm_fan_lock;

To ensure the hwmon device is unregistered before the mutex is
destroyed, it has been re-written to use hwmon_device_register_with_info
instead of devm_hwmon_device_register_with_info:

> @@ -1124,6 +1631,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct pcc_acpi *pcc = acpi_driver_data(device);
>  
> +	if (pcc->pwm_fan_hwmon)
> +		hwmon_device_unregister(pcc->pwm_fan_hwmon);
> +	if (pcc->pwm_fan_cdev)
> +		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
> +	if (pcc->pwm_fan_hwmon || pcc->pwm_fan_cdev)
> +		mutex_destroy(&pcc->pwm_fan_lock);

The mutex is only initialized if either hwmon or thermal device
registration succeeds.

> +			/* cleanup: destroy mutex if both hwmon and thermal failed */
> +			if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
> +				mutex_destroy(&pcc->pwm_fan_lock);

So there is no unnecessary mutex in memory if both fail, this was added to
the probe function. This should ensure it is present if at least one of
the devices are registered (hwmon or thermal) but destroyed if both
fail.

> > +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> > +                               enum hwmon_sensor_types type, u32 attr,
> > +                               int channel, long *val)
> > +{
> [ ... ]
> > +     case hwmon_pwm_enable:
> > +             status = pcc_pwm_fan_mode_read(&pwm_mode);
> [ ... ]
> > +             if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> > +                     *val = HWMON_PCC_FAN_PWM_AUTO;
> > +             } else if (pwm_mode == HWMON_PCC_FAN_PWM_MANUAL) {
> 
> [Severity: Medium]
> Is this returning the correct hwmon ABI value for automatic mode?
> HWMON_PCC_FAN_PWM_AUTO is defined as 0, but the standard hwmon ABI expects
> 2 for automatic mode (and 1 for manual). This might confuse standard
> userspace tools like fancontrol. This same ABI mismatch appears in
> pcc_pwm_fan_hwmon_write() as well.

I have made the following changes:

> +/* Define fan PWM modes */
> +#define ACPI_PCC_FAN_PWM_AUTO	0x00
> +#define ACPI_PCC_FAN_PWM_MANUAL	0x01
> +#define HWMON_PCC_FAN_PWM_AUTO	0x02
> +#define HWMON_PCC_FAN_PWM_MANUAL	0x01

so for hwmon, EC control maps to 2 and manual control is 1.
Thus,

$ echo 2 | sudo tee /sys/class/hwmon/hwmon6/pwm1_enable # EC mode
$ echo 1 | sudo tee /sys/class/hwmon/hwmon6/pwm1_enable # manual mode

would now be the case to enable/disable manual control.

> > +static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
> > +                                        unsigned long state)
> > +{
> > +     acpi_status status;
> > +     int set_pwm;
> > +
> > +     if (state > 100 || state < 0)
> > +             return -EINVAL;
> 
> [Severity: Low]
> Can 'state < 0' ever evaluate to true here? Since 'state' is an unsigned
> long, it cannot be negative, making this check mathematically impossible.

This was removed. This is now:

> +static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
> +					   unsigned long state)
> +{
> +	struct pcc_acpi *pcc;
> +	acpi_status status;
> +	int set_pwm;
> +	int result;
> +
> +	pcc = cdev->devdata;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);
> +
> +	if (state > 100) {
> +		result = -EINVAL;
> +		goto out;
> +	}


> > @@ -981,8 +1377,10 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
> > 
> >  static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
> >  {
> [ ... ]
> > +     /*
> > +      * Perform quirk detection
> > +      */
> > +     dmi_id = dmi_first_match(pcc_quirks);
> > +     if (dmi_id) {
> > +             pcc->quirks = dmi_id->driver_data;
> > +             pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
> > +     } else {
> > +             pcc->quirks = NULL;
> > +     }
> 
> [ ... ]
> 
> > +     /* has_ospm_pwm_fan - add hwmon and thermal if present */
> > +     if (pcc->quirks->has_ospm_pwm_fan) {
> 
> [Severity: High]
> Will this dereference a NULL pointer on unsupported models? If the DMI
> match fails, pcc->quirks is explicitly set to NULL above. Checking
> pcc->quirks->has_ospm_pwm_fan right after will cause a kernel panic
> during the device probe on those unquirked devices.

This was fixed by checking if the quirk structure is null first before
probing for specific quirks:

> > @@ -1097,6 +1571,39 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	i8042_install_filter(panasonic_i8042_filter, NULL);
> > +
> > +	/* check quirks */
> > +	if (pcc->quirks) {
> > +		/* has_ospm_pwm_fan - add hwmon and thermal if present */
> > +		if (pcc->quirks->has_ospm_pwm_fan) {
> > +			mutex_init(&pcc->pwm_fan_lock);
> 
> > +             pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> > +             hwmon_dev = devm_hwmon_device_register_with_info(
> > +                     &pdev->dev, "panasonic_pwm_fan", NULL,
> > +                     &pcc_pwm_fan_hwmon_chip_info, NULL);
> > +             if (IS_ERR(hwmon_dev)) {
> > +                     pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> > +                     return PTR_ERR(hwmon_dev);
> > +             }
> 
> [Severity: High]
> Does this direct return bypass the error cleanup sequence? Standard failure
> paths in this function use 'goto out_notify_handler;' (and other labels) to
> clean up resources. Returning directly leaks memory and leaves the ACPI
> notify handler and input devices registered. If the module is later
> unloaded, could this trigger a use-after-free when ACPI events fire?

For this one, while the other failure methods call for the driver being
unloaded entirely, I treat adding of the OSPM PWM fan as an optional
thing. The errors are handled in code, however an error in hwmon or
thermal will not cause the driver to fail.

> > +
> > +             pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> > +             pcc->pwm_fan_cdev = thermal_cooling_device_register(
> > +                     "Panasonic_PWM_Fan", NULL, &pcc_pwm_fan_cdev_ops);
> 
> [Severity: High]
> Does registering the thermal cooling device directly bypass the hwmon core
> serialization? The subsystem guidelines indicate that thermal and sysfs
> operations are serialized by the hwmon core if attributes are registered
> via the 'info' parameter. Registering this manually out-of-band seems to
> leave the thermal polling operations unprotected, exposing the race
> condition in pcc_pwm_fan_speed_set() mentioned earlier.

While this needs review,
1. I do think the mutex locks added should prevent hwmon and thermal from
stepping over each other.
2. I have put the hwmon and thermal struct pointers on the main struct
of the driver (pcc_acpi). This is so the devices can be manually unregistered
in order before the mutex is destroyed.

 drivers/platform/x86/panasonic-laptop.c | 542 +++++++++++++++++++++++-
 1 file changed, 528 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..1a360272060c 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -119,6 +119,9 @@
  *		- v0.1  start from toshiba_acpi driver written by John Belmonte
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/printk.h>
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/bits.h>
@@ -136,6 +139,12 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <acpi/video.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/thermal.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
@@ -164,6 +173,17 @@ MODULE_LICENSE("GPL");
 
 #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
 
+/* Define fan PWM modes */
+#define ACPI_PCC_FAN_PWM_AUTO		0x00
+#define ACPI_PCC_FAN_PWM_MANUAL	0x01
+#define HWMON_PCC_FAN_PWM_AUTO		0x02
+#define HWMON_PCC_FAN_PWM_MANUAL	0x01
+
+/* Define quirks for this driver */
+struct quirk_entry {
+	bool has_ospm_pwm_fan;
+};
+
 /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
    ECO_MODEs: 0x03 = off, 0x83 = on
 */
@@ -238,19 +258,53 @@ static const struct key_entry panasonic_keymap[] = {
 };
 
 struct pcc_acpi {
-	acpi_handle		handle;
-	unsigned long		num_sifr;
-	int			sticky_key;
-	int			eco_mode;
-	int			mute;
-	int			ac_brightness;
-	int			dc_brightness;
-	int			current_brightness;
-	u32			*sinf;
-	struct acpi_device	*device;
-	struct input_dev	*input_dev;
-	struct backlight_device	*backlight;
-	struct platform_device	*platform;
+	acpi_handle			handle;
+	unsigned long			num_sifr;
+	int				sticky_key;
+	int				eco_mode;
+	int				mute;
+	int				ac_brightness;
+	int				dc_brightness;
+	int				current_brightness;
+	u32				*sinf;
+	struct quirk_entry		*quirks;
+	struct acpi_device		*device;
+	struct input_dev		*input_dev;
+	struct backlight_device		*backlight;
+	struct platform_device		*platform;
+	struct thermal_cooling_device	*pwm_fan_cdev;
+	struct device			*pwm_fan_hwmon;
+	struct mutex			pwm_fan_lock;
+};
+
+/*
+ * Declare quirks and apply matches
+ */
+static struct quirk_entry *quirks;
+
+static struct quirk_entry quirk_cf_sr4 = {
+	.has_ospm_pwm_fan = true,
+};
+
+/* DMI matching for quirks copied from asus-nb-wmi.c */
+static int dmi_matched(const struct dmi_system_id *dmi)
+{
+	pr_info("Identified laptop model '%s'\n", dmi->ident);
+	quirks = dmi->driver_data;
+	return 1;
+}
+
+static const struct dmi_system_id pcc_quirks[] = {
+	{
+		.callback = dmi_matched,
+		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
+		},
+		.driver_data = &quirk_cf_sr4,
+	},
+	{},
 };
 
 /*
@@ -415,7 +469,6 @@ static const struct backlight_ops pcc_backlight_ops = {
 	.update_status	= bl_set_status,
 };
 
-
 /* returns ACPI_SUCCESS if methods to control optical drive are present */
 
 static acpi_status check_optd_present(void)
@@ -507,6 +560,179 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* get OSPM fan mode */
+
+static int pcc_pwm_fan_mode_read(int *pwm_mode)
+{
+	unsigned long long state;
+	acpi_status status;
+	int result;
+
+	/* BIOS default is zero which seems to be some sort of failsafe mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
+				       &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot get fan mode via CEFM\n");
+		result = -EIO;
+		goto out;
+	}
+
+	/* use hwmon convention for pwm_mode */
+	if (state == ACPI_PCC_FAN_PWM_AUTO) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
+	} else if (state != 0 && state > 0) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
+	} else {
+		/* unknown */
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
+
+/* set OSPM fan mode */
+
+static int pcc_pwm_fan_mode_set(int pwm_mode)
+{
+	acpi_status status;
+	int result;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = pwm_mode;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
+				      NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot set fan mode via SEFM\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
+
+/* read PWM fan speed */
+
+static int pcc_pwm_fan_speed_read(int *pwm_speed)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+
+	/* Get fan status first */
+	/* If fan is not in manual mode, it will return a bogus value */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		result = -ENODATA; /* Indeterminate value */
+		goto out;
+	}
+
+	/* get pwm speed */
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
+				      NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get pwm speed\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	obj = buffer.pointer;
+
+	/* the structure should have 3 values */
+	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
+		pr_err("Invalid _FST package structure (expected 3)\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	/* the second element should be the pwm speed as an int */
+	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		pr_err("_FST element at index 1 is not an integer\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	*pwm_speed = obj->package.elements[1].integer.value;
+	result = AE_OK;
+
+out:
+	kfree(buffer.pointer);
+	return result;
+}
+
+/* set PWM fan speed */
+
+static int pcc_pwm_fan_speed_set(int set_pwm_speed)
+{
+	struct acpi_object_list input;
+	union acpi_object param[1];
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+	/* Get fan status. set to manual if not set */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: set fan PWM to manual failed\n", __func__);
+			result = -EIO;
+			goto out;
+		}
+	}
+
+	/* check if within bounds */
+	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
+		pr_err("%s: error: fan speed level out of bounds\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = set_pwm_speed;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
+				      &input, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Setting of fan speed via ._FSL failed.\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
 
 /* sysfs user interface functions */
 
@@ -676,6 +902,7 @@ static ssize_t ac_brightness_show(struct device *dev, struct device_attribute *a
 	return sysfs_emit(buf, "%u\n", pcc->sinf[SINF_AC_CUR_BRIGHT]);
 }
 
+
 static ssize_t ac_brightness_store(struct device *dev, struct device_attribute *attr,
 				   const char *buf, size_t count)
 {
@@ -825,6 +1052,240 @@ static const struct attribute_group pcc_attr_group = {
 	.is_visible	= pcc_sysfs_is_visible,
 };
 
+/* hwmon */
+
+static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE), NULL
+};
+
+static int pcc_pwm_fan_hwmon_read(struct device *dev,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel, long *val)
+{
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int pwm_speed;
+	int pwm_mode;
+	int result;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		mutex_lock(&pcc->pwm_fan_lock);
+
+		status = pcc_pwm_fan_speed_read(&pwm_speed);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to get fan pwm speed\n", __func__);
+			result = -ENODATA;
+		} else {
+			*val = (pwm_speed * 255) / 100;
+			result = 0;
+		}
+
+		mutex_unlock(&pcc->pwm_fan_lock);
+		return result;
+	case hwmon_pwm_enable:
+		mutex_lock(&pcc->pwm_fan_lock);
+
+		status = pcc_pwm_fan_mode_read(&pwm_mode);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to get fan pwm mode\n", __func__);
+			result = -ENODATA;
+		}
+		if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+			*val = HWMON_PCC_FAN_PWM_AUTO;
+			result = 0;
+		} else if (pwm_mode == HWMON_PCC_FAN_PWM_MANUAL) {
+			*val = HWMON_PCC_FAN_PWM_MANUAL;
+			result = 0;
+		} else { /* Invalid value */
+			result = -EIO;
+		}
+
+		mutex_unlock(&pcc->pwm_fan_lock);
+		return result;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_write(struct device *dev,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel, long val)
+{
+	int pwm_speed_vendor_val;
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int result;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		if (val == HWMON_PCC_FAN_PWM_AUTO) {
+			mutex_lock(&pcc->pwm_fan_lock);
+			status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_AUTO);
+			if (ACPI_FAILURE(status)) {
+				pr_err("%s: failed to set fan to auto mode\n", __func__);
+				result = -EIO;
+			} else {
+				result = 0;
+			}
+			mutex_unlock(&pcc->pwm_fan_lock);
+			return result;
+		} else if (val == HWMON_PCC_FAN_PWM_MANUAL) {
+			mutex_lock(&pcc->pwm_fan_lock);
+			status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
+			if (ACPI_FAILURE(status)) {
+				pr_err("%s: failed to set fan to manual mode\n", __func__);
+				result = -EIO;
+			} else {
+				result = 0;
+			}
+			mutex_unlock(&pcc->pwm_fan_lock);
+			return result;
+		}
+		return -EINVAL;
+	case hwmon_pwm_input:
+		val = clamp_val(val, 0, 255);
+		pwm_speed_vendor_val = (val * 100) / 255;
+
+		mutex_lock(&pcc->pwm_fan_lock);
+
+		status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to set fan pwm\n", __func__);
+			result = -EIO;
+		} else {
+			result = 0;
+		}
+
+		mutex_unlock(&pcc->pwm_fan_lock);
+
+		return result;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
+					    enum hwmon_sensor_types type,
+					    u32 attr, int channel)
+{
+	if (type != hwmon_pwm)
+		return 0;
+
+	return 0644;
+}
+
+static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
+	.is_visible = pcc_pwm_fan_hwmon_is_visible,
+	.read = pcc_pwm_fan_hwmon_read,
+	.write = pcc_pwm_fan_hwmon_write,
+};
+
+static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
+	.ops = &pcc_pwm_fan_hwmon_ops,
+	.info = pcc_pwm_fan_hwmon_info,
+};
+
+/* thermal interface */
+static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	*state = 100; /* range of 0-100 as per UEFI spec */
+	return 0;
+}
+static int
+pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
+				    unsigned long *state)
+{
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int current_pwm;
+	int pwm_mode;
+	int result;
+
+	pcc = cdev->devdata;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = -ENODATA;
+		goto out;
+	}
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		*state = 100; /* Return failsafe value on EC mode */
+		result = 0;
+		goto out;
+	}
+
+	status = pcc_pwm_fan_speed_read(&current_pwm);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = -ENODATA;
+		goto out;
+	}
+
+	if (current_pwm > 100)
+		current_pwm = 100;
+	if (current_pwm < 0) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	*state = current_pwm;
+	result = 0;
+
+out:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
+					   unsigned long state)
+{
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int set_pwm;
+	int result;
+
+	pcc = cdev->devdata;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	if (state > 100) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	set_pwm = state;
+	status = pcc_pwm_fan_speed_set(set_pwm);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to set fan speed pwm\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	result = 0;
+
+out:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
+	.get_max_state = pcc_pwm_fan_thermal_max_state,
+	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
+	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
+};
 
 /* hotkey input device driver */
 
@@ -981,8 +1442,10 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
 
 static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 {
+	const struct dmi_system_id *dmi_id;
 	struct backlight_properties props;
 	struct acpi_device *device;
+	struct device *hwmon_dev;
 	struct pcc_acpi *pcc;
 	int num_sifr, result;
 
@@ -1026,6 +1489,17 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
 	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
 
+	/*
+	 * Perform quirk detection
+	 */
+	dmi_id = dmi_first_match(pcc_quirks);
+	if (dmi_id) {
+		pcc->quirks = dmi_id->driver_data;
+		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
+	} else {
+		pcc->quirks = NULL;
+	}
+
 	result = acpi_pcc_init_input(pcc);
 	if (result) {
 		pr_err("Error installing keyinput handler\n");
@@ -1097,6 +1571,39 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	}
 
 	i8042_install_filter(panasonic_i8042_filter, NULL);
+
+	/* check quirks */
+	if (pcc->quirks) {
+		/* has_ospm_pwm_fan - add hwmon and thermal if present */
+		if (pcc->quirks->has_ospm_pwm_fan) {
+			mutex_init(&pcc->pwm_fan_lock);
+
+			pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
+
+			hwmon_dev = hwmon_device_register_with_info(
+				&pdev->dev, "panasonic_pwm_fan", pcc,
+				&pcc_pwm_fan_hwmon_chip_info, NULL);
+			if (IS_ERR(hwmon_dev)) {
+				pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
+				/* not a critical error. just skip if error */
+				pcc->pwm_fan_hwmon = NULL;
+			}
+
+			/* proceed with thermal */
+			pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
+			pcc->pwm_fan_cdev = thermal_cooling_device_register(
+				"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
+			if (IS_ERR(pcc->pwm_fan_cdev)) {
+				pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
+				pcc->pwm_fan_cdev = NULL;
+			}
+
+			/* cleanup: destroy mutex if both hwmon and thermal failed */
+			if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
+				mutex_destroy(&pcc->pwm_fan_lock);
+		}
+	}
+
 	return 0;
 
 out_platform:
@@ -1124,6 +1631,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct pcc_acpi *pcc = acpi_driver_data(device);
 
+	if (pcc->pwm_fan_hwmon)
+		hwmon_device_unregister(pcc->pwm_fan_hwmon);
+	if (pcc->pwm_fan_cdev)
+		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
+	if (pcc->pwm_fan_hwmon || pcc->pwm_fan_cdev)
+		mutex_destroy(&pcc->pwm_fan_lock);
+
 	i8042_remove_filter(panasonic_i8042_filter);
 
 	if (pcc->platform) {
-- 
2.54.0

Re: [RFC PATCH v4] platform/x86: panasonic-laptop: add fan speed mode for newer models

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The returned hwmon device pointer from `hwmon_device_register_with_info` is never assigned to `pcc->pwm_fan_hwmon` on success.
- [Medium] Missing control flow abort after an error in `pcc_pwm_fan_hwmon_read` allows the evaluation of an uninitialized stack variable.
- [Medium] Mixing POSIX error codes with `acpi_status` types causes continuous kernel log spam when reading the hwmon PWM attribute in AUTO mode.
- [Medium] Writing 'auto' mode via the hwmon sysfs interface erroneously passes a hwmon constant to the hardware ACPI function, forcing the hardware into 'manual' mode instead.
--

commit 0a2b607080f828361b9b1616ec43f15ca1686629
Author: Alex Yeo <alexyeo362@gmail.com>

platform/x86: panasonic-laptop: add fan speed mode for newer models

This patch introduces fan speed mode control for newer Panasonic laptop models,
specifically the CF-SR4. It adds ACPI-based reading and writing of PWM fan
speeds and modes, exposing them through hwmon and thermal sysfs interfaces.

> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f885..ab1e0e549b300 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
[ ... ]
> @@ -507,6 +560,179 @@ static int set_optd_power_state(int new_state)
>  	return result;
>  }
>  
> +/* get OSPM fan mode */
> +
> +static int pcc_pwm_fan_mode_read(int *pwm_mode)
> +{
[ ... ]
> +/* read PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_read(int *pwm_speed)
> +{
[ ... ]
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		result = -ENODATA; /* Indeterminate value */
> +		goto out;
> +	}

This successfully returns the POSIX error -ENODATA when the fan is in automatic
mode. I will reference this below.

[ ... ]
> +/* set PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> +{
[ ... ]
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);

[Severity: Medium]
Are we passing the wrong constant here? HWMON_PCC_FAN_PWM_MANUAL is defined
for the hwmon sysfs interface. Does pcc_pwm_fan_mode_set() expect the hardware
constant ACPI_PCC_FAN_PWM_MANUAL instead?

[ ... ]
> @@ -825,6 +1052,240 @@ static const struct attribute_group pcc_attr_group = {
>  	.is_visible	= pcc_sysfs_is_visible,
>  };
>  
> +/* hwmon */
[ ... ]
> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
[ ... ]
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_speed_read(&pwm_speed);
> +		if (ACPI_FAILURE(status)) {
> +			pr_err("%s: failed to get fan pwm speed\n", __func__);
> +			result = -ENODATA;
> +		} else {

[Severity: Medium]
Will this cause continuous kernel log spam when the fan is in auto mode?

As noted above, pcc_pwm_fan_speed_read() returns the POSIX error -ENODATA when
in automatic mode. Here, the caller receives this negative integer into an
acpi_status variable.

Since -ENODATA is non-zero, ACPI_FAILURE() evaluates to true, which triggers
the error log. This could flood dmesg if userspace tools periodically read the
hwmon attribute while the fan is in auto mode.

[ ... ]
> +	case hwmon_pwm_enable:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_mode_read(&pwm_mode);
> +		if (ACPI_FAILURE(status)) {
> +			pr_err("%s: failed to get fan pwm mode\n", __func__);
> +			result = -ENODATA;
> +		}
> +		if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {

[Severity: Medium]
If pcc_pwm_fan_mode_read() fails, we log an error and set result = -ENODATA,
but execution continues to the next line. Does this mean we might evaluate the
uninitialized stack variable pwm_mode? Should there be a break or goto
statement in the error path?

[ ... ]
> +static int pcc_pwm_fan_hwmon_write(struct device *dev,
> +				   enum hwmon_sensor_types type, u32 attr,
> +				   int channel, long val)
> +{
[ ... ]
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		if (val == HWMON_PCC_FAN_PWM_AUTO) {
> +			mutex_lock(&pcc->pwm_fan_lock);
> +			status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_AUTO);

[Severity: Medium]
Is the wrong constant being passed to pcc_pwm_fan_mode_set() here?

val == HWMON_PCC_FAN_PWM_AUTO matches the hwmon constant (2). However,
passing HWMON_PCC_FAN_PWM_AUTO to pcc_pwm_fan_mode_set() means we are sending
2 to the ACPI firmware instead of the expected hardware constant
ACPI_PCC_FAN_PWM_AUTO (0).

Since the firmware treats any value >0 as manual mode, does writing 'auto'
(2) to the sysfs attribute inadvertently force the hardware into manual mode?

[ ... ]
> @@ -1097,6 +1571,39 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  	}
>  
>  	i8042_install_filter(panasonic_i8042_filter, NULL);
> +
> +	/* check quirks */
> +	if (pcc->quirks) {
> +		/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +		if (pcc->quirks->has_ospm_pwm_fan) {
> +			mutex_init(&pcc->pwm_fan_lock);
> +
> +			pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> +
> +			hwmon_dev = hwmon_device_register_with_info(
> +				&pdev->dev, "panasonic_pwm_fan", pcc,
> +				&pcc_pwm_fan_hwmon_chip_info, NULL);
> +			if (IS_ERR(hwmon_dev)) {
> +				pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +				/* not a critical error. just skip if error */
> +				pcc->pwm_fan_hwmon = NULL;
> +			}

[Severity: High]
If hwmon_device_register_with_info() succeeds, we never assign hwmon_dev to
pcc->pwm_fan_hwmon.

Because pcc->pwm_fan_hwmon remains NULL, the success path behaves as if the
registration failed when checking for cleanup.

> +
> +			/* proceed with thermal */
> +			pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> +			pcc->pwm_fan_cdev = thermal_cooling_device_register(
> +				"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
> +			if (IS_ERR(pcc->pwm_fan_cdev)) {
> +				pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
> +				pcc->pwm_fan_cdev = NULL;
> +			}
> +
> +			/* cleanup: destroy mutex if both hwmon and thermal failed */
> +			if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
> +				mutex_destroy(&pcc->pwm_fan_lock);

Since pcc->pwm_fan_hwmon is still NULL upon successful hwmon registration, if
thermal registration fails here, won't we prematurely destroy the mutex while
the hwmon device is still active and potentially using it?

[ ... ]
> @@ -1124,6 +1631,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct pcc_acpi *pcc = acpi_driver_data(device);
>  
> +	if (pcc->pwm_fan_hwmon)
> +		hwmon_device_unregister(pcc->pwm_fan_hwmon);

Because pcc->pwm_fan_hwmon was never assigned, this unregistration is bypassed
on module unload. This will leak the hwmon device and leave its sysfs attributes
active. Accessing them after the module is unloaded would dereference freed
memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626032524.22530-1-alexyeo362@gmail.com?part=1

[RFC PATCH v5] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
This addresses the following concerns:
> - [High] The returned hwmon device pointer from `hwmon_device_register_with_info` is never assigned to `pcc->pwm_fan_hwmon` on success.
> - [Medium] Missing control flow abort after an error in `pcc_pwm_fan_hwmon_read` allows the evaluation of an uninitialized stack variable.
> - [Medium] Mixing POSIX error codes with `acpi_status` types causes continuous kernel log spam when reading the hwmon PWM attribute in AUTO mode.
> - [Medium] Writing 'auto' mode via the hwmon sysfs interface erroneously passes a hwmon constant to the hardware ACPI function, forcing the hardware into 'manual' mode instead.

> > @@ -507,6 +560,179 @@ static int set_optd_power_state(int new_state)
> >       return result;
> >  }
> > 
> > +/* get OSPM fan mode */
> > +
> > +static int pcc_pwm_fan_mode_read(int *pwm_mode)
> > +{
> [ ... ]
> > +/* read PWM fan speed */
> > +
> > +static int pcc_pwm_fan_speed_read(int *pwm_speed)
> > +{
> [ ... ]
> > +     if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> > +             result = -ENODATA; /* Indeterminate value */
> > +             goto out;
> > +     }
> 
> This successfully returns the POSIX error -ENODATA when the fan is in automatic
> mode. I will reference this below.
> 
> [ ... ]
> > +/* set PWM fan speed */
> > +
> > +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> > +{
> [ ... ]
> > +     if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> > +             status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_MANUAL);
> 
> [Severity: Medium]
> Are we passing the wrong constant here? HWMON_PCC_FAN_PWM_MANUAL is defined
> for the hwmon sysfs interface. Does pcc_pwm_fan_mode_set() expect the hardware
> constant ACPI_PCC_FAN_PWM_MANUAL instead?

This is a mistake that has now been corrected. This is also a problem
that is mentioned below and the specifics including the code snippet
will be explained in detail below.

> [ ... ]
> > @@ -825,6 +1052,240 @@ static const struct attribute_group pcc_attr_group = {
> >       .is_visible     = pcc_sysfs_is_visible,
> >  };
> > 
> > +/* hwmon */
> [ ... ]
> > +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> > +                               enum hwmon_sensor_types type, u32 attr,
> > +                               int channel, long *val)
> > +{
> [ ... ]
> > +     switch (attr) {
> > +     case hwmon_pwm_input:
> > +             mutex_lock(&pcc->pwm_fan_lock);
> > +
> > +             status = pcc_pwm_fan_speed_read(&pwm_speed);
> > +             if (ACPI_FAILURE(status)) {
> > +                     pr_err("%s: failed to get fan pwm speed\n", __func__);
> > +                     result = -ENODATA;
> > +             } else {
> 
> [Severity: Medium]
> Will this cause continuous kernel log spam when the fan is in auto mode?
> 
> As noted above, pcc_pwm_fan_speed_read() returns the POSIX error -ENODATA when
> in automatic mode. Here, the caller receives this negative integer into an
> acpi_status variable.
> 
> Since -ENODATA is non-zero, ACPI_FAILURE() evaluates to true, which triggers
> the error log. This could flood dmesg if userspace tools periodically read the
> hwmon attribute while the fan is in auto mode.

To address both of these concerns, I have made the following code
change:

> +	case hwmon_pwm_input:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_speed_read(&pwm_speed);
> +		if (ACPI_FAILURE(status)) {
> +			result = status;
> +		} else {

Instead of generating an error message via pr_err and returning
-ENODATA, it relies on pcc_pwm_fan_speed_read instead.

> +	/* Get fan status first */
> +	/* If fan is not in manual mode, it will return a bogus value */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("%s: failed to get fan status\n", __func__);
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		result = -ENODATA; /* Indeterminate value */
> +		goto out;
> +	}
> +
> +	/* get pwm speed */
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
> +				      NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("%s: failed to get pwm speed\n", __func__);
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	obj = buffer.pointer;
> +
> +	/* the structure should have 3 values */
> +	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
> +		pr_err("Invalid _FST package structure (expected 3)\n");
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* the second element should be the pwm speed as an int */
> +	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
> +		pr_err("_FST element at index 1 is not an integer\n");
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	*pwm_speed = obj->package.elements[1].integer.value;
> +	result = AE_OK;

The function already returns its own set of error codes and error
messages. Here, the fan being on auto is normal so -ENODATA is returned
with no error message generated.

Just passing the output of this function should fix both the issue.

> > +     case hwmon_pwm_enable:
> > +             mutex_lock(&pcc->pwm_fan_lock);
> > +
> > +             status = pcc_pwm_fan_mode_read(&pwm_mode);
> > +             if (ACPI_FAILURE(status)) {
> > +                     pr_err("%s: failed to get fan pwm mode\n", __func__);
> > +                     result = -ENODATA;
> > +             }
> > +             if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> 
> [Severity: Medium]
> If pcc_pwm_fan_mode_read() fails, we log an error and set result = -ENODATA,
> but execution continues to the next line. Does this mean we might evaluate the
> uninitialized stack variable pwm_mode? Should there be a break or goto
> statement in the error path?

This mistake has been corrected by adding an else statement:

> +	case hwmon_pwm_input:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_speed_read(&pwm_speed);
> +		if (ACPI_FAILURE(status)) {
> +			result = status;
> +		} else {
> +			*val = (pwm_speed * 255) / 100;
> +			result = 0;
> +		}
> +
> +		mutex_unlock(&pcc->pwm_fan_lock);
> +		return result;

This should now go straight to mutex_unlock then return.

> > +static int pcc_pwm_fan_hwmon_write(struct device *dev,
> > +                                enum hwmon_sensor_types type, u32 attr,
> > +                                int channel, long val)
> > +{
> [ ... ]
> > +     switch (attr) {
> > +     case hwmon_pwm_enable:
> > +             if (val == HWMON_PCC_FAN_PWM_AUTO) {
> > +                     mutex_lock(&pcc->pwm_fan_lock);
> > +                     status = pcc_pwm_fan_mode_set(HWMON_PCC_FAN_PWM_AUTO);
> 
> [Severity: Medium]
> Is the wrong constant being passed to pcc_pwm_fan_mode_set() here?
> 
> val == HWMON_PCC_FAN_PWM_AUTO matches the hwmon constant (2). However,
> passing HWMON_PCC_FAN_PWM_AUTO to pcc_pwm_fan_mode_set() means we are sending
> 2 to the ACPI firmware instead of the expected hardware constant
> ACPI_PCC_FAN_PWM_AUTO (0).
> 
> Since the firmware treats any value >0 as manual mode, does writing 'auto'
> (2) to the sysfs attribute inadvertently force the hardware into manual mode?

The wrong constants were passed to pcc_pwm_fan_mode_set as it expects
the actual hardware level equivalent int.

This has now all been corrected to use ACPI values and not HWMON:

> +		status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);

> [ ... ]
> > @@ -1097,6 +1571,39 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
> >       }
> > 
> >       i8042_install_filter(panasonic_i8042_filter, NULL);
> > +
> > +     /* check quirks */
> > +     if (pcc->quirks) {
> > +             /* has_ospm_pwm_fan - add hwmon and thermal if present */
> > +             if (pcc->quirks->has_ospm_pwm_fan) {
> > +                     mutex_init(&pcc->pwm_fan_lock);
> > +
> > +                     pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> > +
> > +                     hwmon_dev = hwmon_device_register_with_info(
> > +                             &pdev->dev, "panasonic_pwm_fan", pcc,
> > +                             &pcc_pwm_fan_hwmon_chip_info, NULL);
> > +                     if (IS_ERR(hwmon_dev)) {
> > +                             pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> > +                             /* not a critical error. just skip if error */
> > +                             pcc->pwm_fan_hwmon = NULL;
> > +                     }
> 
> [Severity: High]
> If hwmon_device_register_with_info() succeeds, we never assign hwmon_dev to
> pcc->pwm_fan_hwmon.
> 
> Because pcc->pwm_fan_hwmon remains NULL, the success path behaves as if the
> registration failed when checking for cleanup.
> 
> > +
> > +                     /* proceed with thermal */
> > +                     pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> > +                     pcc->pwm_fan_cdev = thermal_cooling_device_register(
> > +                             "Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
> > +                     if (IS_ERR(pcc->pwm_fan_cdev)) {
> > +                             pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
> > +                             pcc->pwm_fan_cdev = NULL;
> > +                     }
> > +
> > +                     /* cleanup: destroy mutex if both hwmon and thermal failed */
> > +                     if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
> > +                             mutex_destroy(&pcc->pwm_fan_lock);
> 
> Since pcc->pwm_fan_hwmon is still NULL upon successful hwmon registration, if
> thermal registration fails here, won't we prematurely destroy the mutex while
> the hwmon device is still active and potentially using it?
> 
> [ ... ]
> > @@ -1124,6 +1631,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
> >       struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
> >       struct pcc_acpi *pcc = acpi_driver_data(device);
> > 
> > +     if (pcc->pwm_fan_hwmon)
> > +             hwmon_device_unregister(pcc->pwm_fan_hwmon);
> 
> Because pcc->pwm_fan_hwmon was never assigned, this unregistration is bypassed
> on module unload. This will leak the hwmon device and leave its sysfs attributes
> active. Accessing them after the module is unloaded would dereference freed
> memory.

This is now fixed:

> +			pcc->pwm_fan_hwmon = hwmon_device_register_with_info(
> +				&pdev->dev, "panasonic_pwm_fan", pcc,
> +				&pcc_pwm_fan_hwmon_chip_info, NULL);
> +			if (IS_ERR(pcc->pwm_fan_hwmon)) {
> +				pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +				/* not a critical error. just skip if error */
> +				pcc->pwm_fan_hwmon = NULL;
> +			}

I had missed this edge case when I tested my code on a CF-SR4 as this
only applies to a case where both hwmon and thermal failed (have not
encountered such a case so far). When unloading the driver via rmmod, it
did not return an error as the if statement checks first if the pointer 
is null before calling the unregister function.

After making all the changes, I have recompiled the code and tested it
on a CF-SR4 and loading and unloading the module works as expected.

I have also tested on an older model (CF-QV9) where this patch does not
apply to. I did not observe anything abnormal on dmesg. Loading and
unloading the module did not result in any error.

 drivers/platform/x86/panasonic-laptop.c | 541 +++++++++++++++++++++++-
 1 file changed, 527 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..a1837106ff6b 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -119,6 +119,9 @@
  *		- v0.1  start from toshiba_acpi driver written by John Belmonte
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/printk.h>
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/bits.h>
@@ -136,6 +139,12 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <acpi/video.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/thermal.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
@@ -164,6 +173,17 @@ MODULE_LICENSE("GPL");
 
 #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
 
+/* Define fan PWM modes */
+#define ACPI_PCC_FAN_PWM_AUTO		0x00
+#define ACPI_PCC_FAN_PWM_MANUAL		0x01
+#define HWMON_PCC_FAN_PWM_AUTO		0x02
+#define HWMON_PCC_FAN_PWM_MANUAL	0x01
+
+/* Define quirks for this driver */
+struct quirk_entry {
+	bool has_ospm_pwm_fan;
+};
+
 /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
    ECO_MODEs: 0x03 = off, 0x83 = on
 */
@@ -238,19 +258,53 @@ static const struct key_entry panasonic_keymap[] = {
 };
 
 struct pcc_acpi {
-	acpi_handle		handle;
-	unsigned long		num_sifr;
-	int			sticky_key;
-	int			eco_mode;
-	int			mute;
-	int			ac_brightness;
-	int			dc_brightness;
-	int			current_brightness;
-	u32			*sinf;
-	struct acpi_device	*device;
-	struct input_dev	*input_dev;
-	struct backlight_device	*backlight;
-	struct platform_device	*platform;
+	acpi_handle			handle;
+	unsigned long			num_sifr;
+	int				sticky_key;
+	int				eco_mode;
+	int				mute;
+	int				ac_brightness;
+	int				dc_brightness;
+	int				current_brightness;
+	u32				*sinf;
+	struct quirk_entry		*quirks;
+	struct acpi_device		*device;
+	struct input_dev		*input_dev;
+	struct backlight_device		*backlight;
+	struct platform_device		*platform;
+	struct thermal_cooling_device	*pwm_fan_cdev;
+	struct device			*pwm_fan_hwmon;
+	struct mutex			pwm_fan_lock;
+};
+
+/*
+ * Declare quirks and apply matches
+ */
+static struct quirk_entry *quirks;
+
+static struct quirk_entry quirk_cf_sr4 = {
+	.has_ospm_pwm_fan = true,
+};
+
+/* DMI matching for quirks copied from asus-nb-wmi.c */
+static int dmi_matched(const struct dmi_system_id *dmi)
+{
+	pr_info("Identified laptop model '%s'\n", dmi->ident);
+	quirks = dmi->driver_data;
+	return 1;
+}
+
+static const struct dmi_system_id pcc_quirks[] = {
+	{
+		.callback = dmi_matched,
+		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
+		},
+		.driver_data = &quirk_cf_sr4,
+	},
+	{},
 };
 
 /*
@@ -415,7 +469,6 @@ static const struct backlight_ops pcc_backlight_ops = {
 	.update_status	= bl_set_status,
 };
 
-
 /* returns ACPI_SUCCESS if methods to control optical drive are present */
 
 static acpi_status check_optd_present(void)
@@ -507,6 +560,179 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* get OSPM fan mode */
+
+static int pcc_pwm_fan_mode_read(int *pwm_mode)
+{
+	unsigned long long state;
+	acpi_status status;
+	int result;
+
+	/* BIOS default is zero which seems to be some sort of failsafe mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
+				       &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot get fan mode via CEFM\n");
+		result = -EIO;
+		goto out;
+	}
+
+	/* use hwmon convention for pwm_mode */
+	if (state == ACPI_PCC_FAN_PWM_AUTO) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
+	} else if (state != 0 && state > 0) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
+	} else {
+		/* unknown */
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
+
+/* set OSPM fan mode */
+
+static int pcc_pwm_fan_mode_set(int pwm_mode)
+{
+	acpi_status status;
+	int result;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = pwm_mode;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
+				      NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot set fan mode via SEFM\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
+
+/* read PWM fan speed */
+
+static int pcc_pwm_fan_speed_read(int *pwm_speed)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+
+	/* Get fan status first */
+	/* If fan is not in manual mode, it will return a bogus value */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		result = -ENODATA; /* Indeterminate value */
+		goto out;
+	}
+
+	/* get pwm speed */
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
+				      NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get pwm speed\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	obj = buffer.pointer;
+
+	/* the structure should have 3 values */
+	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
+		pr_err("Invalid _FST package structure (expected 3)\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	/* the second element should be the pwm speed as an int */
+	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		pr_err("_FST element at index 1 is not an integer\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	*pwm_speed = obj->package.elements[1].integer.value;
+	result = AE_OK;
+
+out:
+	kfree(buffer.pointer);
+	return result;
+}
+
+/* set PWM fan speed */
+
+static int pcc_pwm_fan_speed_set(int set_pwm_speed)
+{
+	struct acpi_object_list input;
+	union acpi_object param[1];
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+	/* Get fan status. set to manual if not set */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: set fan PWM to manual failed\n", __func__);
+			result = -EIO;
+			goto out;
+		}
+	}
+
+	/* check if within bounds */
+	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
+		pr_err("%s: error: fan speed level out of bounds\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = set_pwm_speed;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
+				      &input, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Setting of fan speed via ._FSL failed.\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = AE_OK;
+
+out:
+	return result;
+}
 
 /* sysfs user interface functions */
 
@@ -676,6 +902,7 @@ static ssize_t ac_brightness_show(struct device *dev, struct device_attribute *a
 	return sysfs_emit(buf, "%u\n", pcc->sinf[SINF_AC_CUR_BRIGHT]);
 }
 
+
 static ssize_t ac_brightness_store(struct device *dev, struct device_attribute *attr,
 				   const char *buf, size_t count)
 {
@@ -825,6 +1052,240 @@ static const struct attribute_group pcc_attr_group = {
 	.is_visible	= pcc_sysfs_is_visible,
 };
 
+/* hwmon */
+
+static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE), NULL
+};
+
+static int pcc_pwm_fan_hwmon_read(struct device *dev,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel, long *val)
+{
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int pwm_speed;
+	int pwm_mode;
+	int result;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		mutex_lock(&pcc->pwm_fan_lock);
+
+		status = pcc_pwm_fan_speed_read(&pwm_speed);
+		if (ACPI_FAILURE(status)) {
+			result = status;
+		} else {
+			*val = (pwm_speed * 255) / 100;
+			result = 0;
+		}
+
+		mutex_unlock(&pcc->pwm_fan_lock);
+		return result;
+	case hwmon_pwm_enable:
+		mutex_lock(&pcc->pwm_fan_lock);
+
+		status = pcc_pwm_fan_mode_read(&pwm_mode);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to get fan pwm mode\n", __func__);
+			result = -ENODATA;
+		} else {
+			if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+				*val = HWMON_PCC_FAN_PWM_AUTO;
+				result = 0;
+			} else if (pwm_mode == HWMON_PCC_FAN_PWM_MANUAL) {
+				*val = HWMON_PCC_FAN_PWM_MANUAL;
+				result = 0;
+			} else { /* Invalid value */
+				result = -EIO;
+			}
+		}
+
+		mutex_unlock(&pcc->pwm_fan_lock);
+		return result;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_write(struct device *dev,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel, long val)
+{
+	int pwm_speed_vendor_val;
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int result;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		if (val == HWMON_PCC_FAN_PWM_AUTO) {
+			mutex_lock(&pcc->pwm_fan_lock);
+			status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_AUTO);
+			if (ACPI_FAILURE(status)) {
+				pr_err("%s: failed to set fan to auto mode\n", __func__);
+				result = -EIO;
+			} else {
+				result = 0;
+			}
+			mutex_unlock(&pcc->pwm_fan_lock);
+			return result;
+		} else if (val == HWMON_PCC_FAN_PWM_MANUAL) {
+			mutex_lock(&pcc->pwm_fan_lock);
+			status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
+			if (ACPI_FAILURE(status)) {
+				pr_err("%s: failed to set fan to manual mode\n", __func__);
+				result = -EIO;
+			} else {
+				result = 0;
+			}
+			mutex_unlock(&pcc->pwm_fan_lock);
+			return result;
+		}
+		return -EINVAL;
+	case hwmon_pwm_input:
+		val = clamp_val(val, 0, 255);
+		pwm_speed_vendor_val = (val * 100) / 255;
+
+		mutex_lock(&pcc->pwm_fan_lock);
+
+		status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
+		if (ACPI_FAILURE(status)) {
+			pr_err("%s: failed to set fan pwm\n", __func__);
+			result = -EIO;
+		} else {
+			result = 0;
+		}
+
+		mutex_unlock(&pcc->pwm_fan_lock);
+
+		return result;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
+					    enum hwmon_sensor_types type,
+					    u32 attr, int channel)
+{
+	if (type != hwmon_pwm)
+		return 0;
+
+	return 0644;
+}
+
+static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
+	.is_visible = pcc_pwm_fan_hwmon_is_visible,
+	.read = pcc_pwm_fan_hwmon_read,
+	.write = pcc_pwm_fan_hwmon_write,
+};
+
+static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
+	.ops = &pcc_pwm_fan_hwmon_ops,
+	.info = pcc_pwm_fan_hwmon_info,
+};
+
+/* thermal interface */
+static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	*state = 100; /* range of 0-100 as per UEFI spec */
+	return 0;
+}
+static int
+pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
+				    unsigned long *state)
+{
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int current_pwm;
+	int pwm_mode;
+	int result;
+
+	pcc = cdev->devdata;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = -ENODATA;
+		goto out;
+	}
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		*state = 100; /* Return failsafe value on EC mode */
+		result = 0;
+		goto out;
+	}
+
+	status = pcc_pwm_fan_speed_read(&current_pwm);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = -ENODATA;
+		goto out;
+	}
+
+	if (current_pwm > 100)
+		current_pwm = 100;
+	if (current_pwm < 0) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	*state = current_pwm;
+	result = 0;
+
+out:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
+					   unsigned long state)
+{
+	struct pcc_acpi *pcc;
+	acpi_status status;
+	int set_pwm;
+	int result;
+
+	pcc = cdev->devdata;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	if (state > 100) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	set_pwm = state;
+	status = pcc_pwm_fan_speed_set(set_pwm);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to set fan speed pwm\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	result = 0;
+
+out:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
+	.get_max_state = pcc_pwm_fan_thermal_max_state,
+	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
+	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
+};
 
 /* hotkey input device driver */
 
@@ -981,6 +1442,7 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
 
 static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 {
+	const struct dmi_system_id *dmi_id;
 	struct backlight_properties props;
 	struct acpi_device *device;
 	struct pcc_acpi *pcc;
@@ -1026,6 +1488,17 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
 	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
 
+	/*
+	 * Perform quirk detection
+	 */
+	dmi_id = dmi_first_match(pcc_quirks);
+	if (dmi_id) {
+		pcc->quirks = dmi_id->driver_data;
+		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
+	} else {
+		pcc->quirks = NULL;
+	}
+
 	result = acpi_pcc_init_input(pcc);
 	if (result) {
 		pr_err("Error installing keyinput handler\n");
@@ -1097,6 +1570,39 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	}
 
 	i8042_install_filter(panasonic_i8042_filter, NULL);
+
+	/* check quirks */
+	if (pcc->quirks) {
+		/* has_ospm_pwm_fan - add hwmon and thermal if present */
+		if (pcc->quirks->has_ospm_pwm_fan) {
+			mutex_init(&pcc->pwm_fan_lock);
+
+			pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
+
+			pcc->pwm_fan_hwmon = hwmon_device_register_with_info(
+				&pdev->dev, "panasonic_pwm_fan", pcc,
+				&pcc_pwm_fan_hwmon_chip_info, NULL);
+			if (IS_ERR(pcc->pwm_fan_hwmon)) {
+				pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
+				/* not a critical error. just skip if error */
+				pcc->pwm_fan_hwmon = NULL;
+			}
+
+			/* proceed with thermal */
+			pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
+			pcc->pwm_fan_cdev = thermal_cooling_device_register(
+				"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
+			if (IS_ERR(pcc->pwm_fan_cdev)) {
+				pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
+				pcc->pwm_fan_cdev = NULL;
+			}
+
+			/* cleanup: destroy mutex if both hwmon and thermal failed */
+			if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
+				mutex_destroy(&pcc->pwm_fan_lock);
+		}
+	}
+
 	return 0;
 
 out_platform:
@@ -1124,6 +1630,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct pcc_acpi *pcc = acpi_driver_data(device);
 
+	if (pcc->pwm_fan_hwmon)
+		hwmon_device_unregister(pcc->pwm_fan_hwmon);
+	if (pcc->pwm_fan_cdev)
+		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
+	if (pcc->pwm_fan_hwmon || pcc->pwm_fan_cdev)
+		mutex_destroy(&pcc->pwm_fan_lock);
+
 	i8042_remove_filter(panasonic_i8042_filter);
 
 	if (pcc->platform) {
-- 
2.54.0

Re: [RFC PATCH v5] platform/x86: panasonic-laptop: add fan speed mode for newer models

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Returning `100` (max_state) for the thermal cooling device's current state when in AUTO mode defeats the thermal governor and permanently degrades cooling.
- [Medium] The driver assigns standard Linux error codes to an `acpi_status` variable and evaluates them using `ACPI_FAILURE()`.
- [Medium] The `pcc_pwm_fan_hwmon_read` function lacks bounds checking for the raw PWM speed returned by the hardware, potentially violating the hwmon ABI.
- [Low] The driver introduces a private mutex (`pwm_fan_lock`) instead of using the hwmon subsystem's locking mechanism.
- [Low] The global `quirks` variable is assigned in a DMI callback but never used.
--

commit bc34c0a562a71eb6eb4101fca6fd721a3d653df2
Author: Alex Yeo <alexyeo362@gmail.com>

platform/x86: panasonic-laptop: add fan speed mode for newer models

This commit adds fan speed and mode support for newer Panasonic laptop models 
like the CF-SR4. It utilizes ACPI methods to read and write the fan profile 
and registers the fan with both the hwmon and thermal subsystems.

> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f885..a1837106ff6b1 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
[ ... ]
> @@ -238,19 +258,53 @@ static const struct key_entry panasonic_keymap[] = {
>  };
>  
>  struct pcc_acpi {
[ ... ]
> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +	struct device			*pwm_fan_hwmon;
> +	struct mutex			pwm_fan_lock;

[Severity: Low]
Does this driver need a private mutex for the hwmon attributes? 

The hardware monitoring subsystem guidelines dictate using hwmon_lock() and 
hwmon_unlock() for attributes registered outside the core, rather than 
implementing a driver-specific lock.

>  };
> +
> +/*
> + * Declare quirks and apply matches
> + */
> +static struct quirk_entry *quirks;
> +
> +static struct quirk_entry quirk_cf_sr4 = {
> +	.has_ospm_pwm_fan = true,
> +};
> +
> +/* DMI matching for quirks copied from asus-nb-wmi.c */
> +static int dmi_matched(const struct dmi_system_id *dmi)
> +{
> +	pr_info("Identified laptop model '%s'\n", dmi->ident);
> +	quirks = dmi->driver_data;

[Severity: Low]
Is this global quirks variable necessary? 

The dmi_matched() callback assigns the matched data here, but the driver 
appears to ignore this global variable later, instead directly assigning 
pcc->quirks = dmi_id->driver_data during probe.

> +	return 1;
> +}
[ ... ]
> @@ -825,6 +1052,240 @@ static const struct attribute_group pcc_attr_group = {
>  	.is_visible	= pcc_sysfs_is_visible,
>  };
>  
> +/* hwmon */
[ ... ]
> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
> +	struct pcc_acpi *pcc;
> +	acpi_status status;
> +	int pwm_speed;
> +	int pwm_mode;
> +	int result;
> +
> +	pcc = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +
> +		status = pcc_pwm_fan_speed_read(&pwm_speed);
> +		if (ACPI_FAILURE(status)) {
> +			result = status;

[Severity: Medium]
Does this safely mix Linux error codes with ACPI status codes?

Helper functions like pcc_pwm_fan_speed_read() return negative standard
Linux error codes (e.g., -EINVAL), but here they are assigned to an
acpi_status variable and evaluated with ACPI_FAILURE().

While ACPI_FAILURE() might evaluate to true for negative values due to two's
complement representation, it violates the ACPICA API contract and can lead
to swallowed or mangled error codes being passed back to userspace.

> +		} else {
> +			*val = (pwm_speed * 255) / 100;

[Severity: Medium]
Could this calculation expose an out-of-bounds value to userspace? 

If the ACPI method returns a value greater than 100 (which can happen for 
unknown or indeterminate states like 255), this unchecked math will produce 
a value outside the standard 0-255 range mandated by the hwmon ABI for 
pwm attributes.

> +			result = 0;
> +		}
> +
> +		mutex_unlock(&pcc->pwm_fan_lock);
> +		return result;
[ ... ]
> +static int
> +pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
> +				    unsigned long *state)
> +{
> +	struct pcc_acpi *pcc;
> +	acpi_status status;
> +	int current_pwm;
> +	int pwm_mode;
> +	int result;
> +
> +	pcc = cdev->devdata;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);
> +
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("%s: failed to get fan pwm mode\n", __func__);
> +		result = -ENODATA;
> +		goto out;
> +	}
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		*state = 100; /* Return failsafe value on EC mode */

[Severity: High]
Does returning 100 (the maximum state) while in AUTO mode paralyze the
thermal governor?

If the system overheats while the fan is in AUTO mode, the step_wise
governor will read the current state as 100 and assume it is at maximum
cooling capacity, doing nothing to ramp up the fan.

When the temperature drops, it will decrease the state to 99, invoking
set_cur_state(99). This immediately and permanently switches the fan to
MANUAL mode, which has a lower maximum speed limit than AUTO mode, degrading
cooling performance permanently.

> +		result = 0;
> +		goto out;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626053235.25834-1-alexyeo362@gmail.com?part=1

[RFC PATCH v6] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
New in v6:
 - Addressed: [Low] The global `quirks` variable is assigned in a DMI
   callback but never used.
 - Addressed: [Medium] The `pcc_pwm_fan_hwmon_read` function lacks
   bounds checking for the raw PWM speed returned by the hardware,
   potentially violating the hwmon ABI.
 - Addressed: [Medium] The driver assigns standard Linux error codes to
   an `acpi_status` variable and evaluates them using `ACPI_FAILURE()`
   by clarifying return codes as int when not dealing with ACPI return
   values
 - Moved hwmon code into separate helper functions as the check logic to
   ensure safety was complex enough to go past the tab/indentation
   limit
 - hwmon: removed behavior where setting of fan PWM while on EC/auto
   mode will toggle manual mode. This current version will block that
   action. It only works if pwm1_enable is set. 
 - separated the if statement on the quirks check since having it nested
   was using too many indentations.

For the following, I have clarifications:
 - [High] Returning `100` (max_state) for the thermal cooling device's
   current state when in AUTO mode defeats the thermal governor and
   permanently degrades cooling.
 - [Low] The driver introduces a private mutex (`pwm_fan_lock`) instead
   of using the hwmon subsystem's locking mechanism.

> - [Low] The driver introduces a private mutex (`pwm_fan_lock`) instead of using the hwmon subsystem's locking mechanism.

For this concern, I have based the mutex pattern from another driver
(asus-wmi.c and thinkpad_acpi.c). From what I can gather, having
a mutex that is managed by the driver seems to be the pattern and
I utilize this since the lock is supposed to prevent resource
contention between the hwmon and thermal subsystem when the fan
mode and speed are read and set.

> - [High] Returning `100` (max_state) for the thermal cooling device's current state when in AUTO mode defeats the thermal governor and permanently degrades cooling.

This one is a bit confusing to me as cooling is never degraded during
the entire processes. In fact, the decision to report 100 when the EC
controls the fan in its failsafe mode ensures cooling is not degraded.

I will elaborate on how the specifics:

When the computer is booted up (or rebooted), the BIOS hands control of
the fan to the EC. The EC by default has an aggressive fan curve where
the lowest speed the fan could spin is close to around 80% speed. This
is noticeable as the fan is quite audible.

When Windows boots up, just before the login screen the fans suddenly
quiet down and the speed is adjusted automatically by the OS. From the
ACPI spec, this is referred to as OSPM.

In the case of Linux, when this driver is loaded, it cannot tell the
actual fan speed or the current PWM value. This is because the vendor
chose to not have those values reported via ACPI when queried. The only
thing that is certain is if the fan is in EC (auto) mode or in manual
mode. When in EC/auto mode, as the fan is spinning at a high value, it
relies on the OS to give the signal to control the fan manually and give
a value between 0-100.

On the vendor's driver on Windows this can be done, however for the
Linux driver having the driver mess with any of this commands unprompted
by the user should not happen. So instead I report 100 when the EC is on
auto mode as this should prompt userspace tools to set a fan value when
the temperature is too cold for that high of a fan speed.

Cooling is not degraded in any way in this process and it relies on a
tool like thermald to set the fan speed.

I have also observed that when it gets too hot (~90C), the EC turns
the fan on automatically.

> - [Medium] The driver assigns standard Linux error codes to an `acpi_status` variable and evaluates them using `ACPI_FAILURE()`.

I have reviewed this one and this is something I missed as I had used
acpi_status in places where it should not have been used at all.

> +	int status;

I have rewrote those instances so that an int is used instead.

> - [Medium] The `pcc_pwm_fan_hwmon_read` function lacks bounds checking for the raw PWM speed returned by the hardware, potentially violating the hwmon ABI.

> +	/* prevent out-of-bounds */
> +	if (obj->package.elements[1].integer.value > 100) {
> +		pr_err("_FST element at index 1 is an integer out of bounds\n");
> +		result = -EINVAL;
> +		goto out;
> +	}

This has been added. This is an unsigned int so I only check if its
above 100.

> - [Low] The global `quirks` variable is assigned in a DMI callback but never used.

I have rewrote the code so that a callback is not used and as an
extension the global quirks variable is not used.

> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		return pcc_pwm_fan_hwmon_speed_read(pcc, val);
> +	case hwmon_pwm_enable:
> +		return pcc_pwm_fan_hwmon_mode_read(pcc, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}

The hwmon read and write function has been modified to use helper
functions since the amount of indentations was too much.

> +	/* skip if no quirks */
> +	if (!pcc->quirks)
> +		goto out_no_quirks;
> +
> +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +	if (pcc->quirks->has_ospm_pwm_fan) {
> [...]
> +out_no_quirks:
>  	return 0;

I have also separated these checks for quirks as it was using too
many indentations.

> +	/* check first if manual control is enabled */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan pwm mode\n", __func__);
> +		result = status;
> +		goto out_unlock;
> +	}
> +
> +	/* do not allow settings speeds if not manual mode */
> +	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL) {
> +		result = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}

The hwmon fan now disallows setting of PWM values if the fan is set to
EC/auto mode.

$ echo 1 | sudo tee /sys/class/hwmon/hwmon6/pwm1_enable # manual mode

Has to be done first to allow PWM control.


I have also tested this and it compiles and works as intended in my
case.

 drivers/platform/x86/panasonic-laptop.c | 590 +++++++++++++++++++++++-
 1 file changed, 576 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..2d2acd830fa4 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -119,6 +119,9 @@
  *		- v0.1  start from toshiba_acpi driver written by John Belmonte
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/printk.h>
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/bits.h>
@@ -136,6 +139,12 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <acpi/video.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/thermal.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
@@ -164,6 +173,17 @@ MODULE_LICENSE("GPL");
 
 #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
 
+/* Define fan PWM modes */
+#define ACPI_PCC_FAN_PWM_AUTO		0x00
+#define ACPI_PCC_FAN_PWM_MANUAL		0x01
+#define HWMON_PCC_FAN_PWM_AUTO		0x02
+#define HWMON_PCC_FAN_PWM_MANUAL	0x01
+
+/* Define quirks for this driver */
+struct quirk_entry {
+	bool has_ospm_pwm_fan;
+};
+
 /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
    ECO_MODEs: 0x03 = off, 0x83 = on
 */
@@ -238,19 +258,44 @@ static const struct key_entry panasonic_keymap[] = {
 };
 
 struct pcc_acpi {
-	acpi_handle		handle;
-	unsigned long		num_sifr;
-	int			sticky_key;
-	int			eco_mode;
-	int			mute;
-	int			ac_brightness;
-	int			dc_brightness;
-	int			current_brightness;
-	u32			*sinf;
-	struct acpi_device	*device;
-	struct input_dev	*input_dev;
-	struct backlight_device	*backlight;
-	struct platform_device	*platform;
+	acpi_handle			handle;
+	unsigned long			num_sifr;
+	int				sticky_key;
+	int				eco_mode;
+	int				mute;
+	int				ac_brightness;
+	int				dc_brightness;
+	int				current_brightness;
+	u32				*sinf;
+	struct quirk_entry		*quirks;
+	struct acpi_device		*device;
+	struct input_dev		*input_dev;
+	struct backlight_device		*backlight;
+	struct platform_device		*platform;
+	struct thermal_cooling_device	*pwm_fan_cdev;
+	struct device			*pwm_fan_hwmon;
+	struct mutex			pwm_fan_lock;
+};
+
+/*
+ * Declare quirks and apply matches
+ */
+
+static struct quirk_entry quirk_cf_sr4 = {
+	.has_ospm_pwm_fan = true,
+};
+
+/* DMI matching for quirks copied from asus-nb-wmi.c */
+static const struct dmi_system_id pcc_quirks[] = {
+	{
+		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
+		},
+		.driver_data = &quirk_cf_sr4,
+	},
+	{},
 };
 
 /*
@@ -415,7 +460,6 @@ static const struct backlight_ops pcc_backlight_ops = {
 	.update_status	= bl_set_status,
 };
 
-
 /* returns ACPI_SUCCESS if methods to control optical drive are present */
 
 static acpi_status check_optd_present(void)
@@ -507,6 +551,187 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* get OSPM fan mode */
+
+static int pcc_pwm_fan_mode_read(int *pwm_mode)
+{
+	unsigned long long state;
+	acpi_status status;
+	int result;
+
+	/* BIOS default is zero which seems to be some sort of failsafe mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
+				       &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot get fan mode via CEFM\n");
+		result = -EIO;
+		goto out;
+	}
+
+	/* use hwmon convention for pwm_mode */
+	if (state == ACPI_PCC_FAN_PWM_AUTO) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
+	} else if (state != 0 && state > 0) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
+	} else {
+		/* unknown */
+		result = -EINVAL;
+		goto out;
+	}
+
+	result = 0;
+
+out:
+	return result;
+}
+
+/* set OSPM fan mode */
+
+static int pcc_pwm_fan_mode_set(int pwm_mode)
+{
+	acpi_status status;
+	int result;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = pwm_mode;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
+				      NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("error: cannot set fan mode via SEFM\n");
+		result = -EIO;
+		goto out;
+	}
+
+	result = 0;
+
+out:
+	return result;
+}
+
+/* read PWM fan speed */
+
+static int pcc_pwm_fan_speed_read(int *pwm_speed)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	int pwm_mode;
+	int result;
+
+
+	/* Get fan status first */
+	/* If fan is not in manual mode, it will return a bogus value */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = status;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		result = -ENODATA; /* Indeterminate value */
+		goto out;
+	}
+
+	/* get pwm speed */
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
+				      NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%s: failed to get pwm speed\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	obj = buffer.pointer;
+
+	/* the structure should have 3 values */
+	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
+		pr_err("Invalid _FST package structure (expected 3)\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	/* the second element should be the pwm speed as an int */
+	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		pr_err("_FST element at index 1 is not an integer\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	/* prevent out-of-bounds */
+	if (obj->package.elements[1].integer.value > 100) {
+		pr_err("_FST element at index 1 is an integer out of bounds\n");
+		result = -EINVAL;
+		goto out;
+	}
+
+	*pwm_speed = obj->package.elements[1].integer.value;
+	result = 0;
+
+out:
+	kfree(buffer.pointer);
+	return result;
+}
+
+/* set PWM fan speed */
+
+static int pcc_pwm_fan_speed_set(int set_pwm_speed)
+{
+	struct acpi_object_list input;
+	union acpi_object param[1];
+	acpi_status fsl_status;
+	int pwm_mode;
+	int status;
+	int result;
+
+	/* Get fan status. set to manual if not set */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("%s: failed to get fan status\n", __func__);
+		result = status;
+		goto out;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
+		if (status < 0) {
+			pr_err("%s: set fan PWM to manual failed\n", __func__);
+			result = status;
+			goto out;
+		}
+	}
+
+	/* check if within bounds */
+	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
+		pr_err("%s: error: fan speed level out of bounds\n", __func__);
+		result = -EIO;
+		goto out;
+	}
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = set_pwm_speed;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	fsl_status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
+				      &input, NULL);
+	if (ACPI_FAILURE(fsl_status)) {
+		pr_err("Setting of fan speed via ._FSL failed.\n");
+		result = -EIO;
+		goto out;
+	}
+
+	result = 0;
+
+out:
+	return result;
+}
 
 /* sysfs user interface functions */
 
@@ -676,6 +901,7 @@ static ssize_t ac_brightness_show(struct device *dev, struct device_attribute *a
 	return sysfs_emit(buf, "%u\n", pcc->sinf[SINF_AC_CUR_BRIGHT]);
 }
 
+
 static ssize_t ac_brightness_store(struct device *dev, struct device_attribute *attr,
 				   const char *buf, size_t count)
 {
@@ -825,6 +1051,288 @@ static const struct attribute_group pcc_attr_group = {
 	.is_visible	= pcc_sysfs_is_visible,
 };
 
+/* hwmon */
+
+static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE), NULL
+};
+
+static int pcc_pwm_fan_hwmon_speed_read(struct pcc_acpi *pcc, long *val)
+{
+	int pwm_speed;
+	int result;
+	int status;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_speed_read(&pwm_speed);
+	if (status < 0) {
+		result = status;
+		goto out_unlock;
+	}
+
+	/* protect against out-of-bounds */
+	if (pwm_speed < 0 || pwm_speed > 100) {
+		result = -EINVAL;
+		goto out_unlock;
+	}
+
+	*val = (pwm_speed * 255) / 100;
+	result = 0;
+
+out_unlock:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+
+static int pcc_pwm_fan_hwmon_mode_read(struct pcc_acpi *pcc, long *val)
+{
+	int pwm_mode;
+	int result;
+	int status;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = status;
+		goto out_unlock;
+	}
+	switch (pwm_mode) {
+	case HWMON_PCC_FAN_PWM_AUTO:
+		*val = HWMON_PCC_FAN_PWM_AUTO;
+		result = 0;
+		break;
+	case HWMON_PCC_FAN_PWM_MANUAL:
+		*val = HWMON_PCC_FAN_PWM_MANUAL;
+		result = 0;
+		break;
+	default:
+		result = -EINVAL;
+		break;
+	}
+
+out_unlock:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+
+static int pcc_pwm_fan_hwmon_read(struct device *dev,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel, long *val)
+{
+	struct pcc_acpi *pcc;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		return pcc_pwm_fan_hwmon_speed_read(pcc, val);
+	case hwmon_pwm_enable:
+		return pcc_pwm_fan_hwmon_mode_read(pcc, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_mode_set(struct pcc_acpi *pcc, long val)
+{
+	int result;
+
+	switch (val) {
+	case HWMON_PCC_FAN_PWM_AUTO:
+		mutex_lock(&pcc->pwm_fan_lock);
+		result = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_AUTO);
+		goto out_unlock;
+	case HWMON_PCC_FAN_PWM_MANUAL:
+		mutex_lock(&pcc->pwm_fan_lock);
+		result = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
+		goto out_unlock;
+	default:
+		result = -EINVAL;
+		goto out;
+	}
+
+out_unlock:
+	mutex_unlock(&pcc->pwm_fan_lock);
+out:
+	return result;
+}
+
+static int pcc_pwm_fan_hwmon_speed_set(struct pcc_acpi *pcc, long val)
+{
+	int pwm_speed_vendor_val;
+	int pwm_mode;
+	int status;
+	int result;
+
+	val = clamp_val(val, 0, 255);
+	pwm_speed_vendor_val = (val * 100) / 255;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	/* check first if manual control is enabled */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = status;
+		goto out_unlock;
+	}
+
+	/* do not allow settings speeds if not manual mode */
+	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL) {
+		result = -EOPNOTSUPP;
+		goto out_unlock;
+	}
+
+	status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
+	if (status < 0) {
+		pr_err("%s: failed to set fan pwm\n", __func__);
+		result = status;
+		goto out_unlock;
+	}
+	result = 0;
+
+out_unlock:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+
+static int pcc_pwm_fan_hwmon_write(struct device *dev,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel, long val)
+{
+	struct pcc_acpi *pcc;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		return pcc_pwm_fan_hwmon_mode_set(pcc, val);
+	case hwmon_pwm_input:
+		return pcc_pwm_fan_hwmon_speed_set(pcc, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
+					    enum hwmon_sensor_types type,
+					    u32 attr, int channel)
+{
+	if (type != hwmon_pwm)
+		return 0;
+
+	return 0644;
+}
+
+static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
+	.is_visible = pcc_pwm_fan_hwmon_is_visible,
+	.read = pcc_pwm_fan_hwmon_read,
+	.write = pcc_pwm_fan_hwmon_write,
+};
+
+static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
+	.ops = &pcc_pwm_fan_hwmon_ops,
+	.info = pcc_pwm_fan_hwmon_info,
+};
+
+/* thermal interface */
+static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	*state = 100; /* range of 0-100 as per UEFI spec */
+	return 0;
+}
+static int
+pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
+				    unsigned long *state)
+{
+	struct pcc_acpi *pcc;
+	int current_pwm;
+	int pwm_mode;
+	int result;
+	int status;
+
+	pcc = cdev->devdata;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		result = status;
+		goto out;
+	}
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		*state = 100; /* Return failsafe value on EC mode */
+		result = 0;
+		goto out;
+	}
+
+	status = pcc_pwm_fan_speed_read(&current_pwm);
+	if (status < 0) {
+		result = status; /* pass error code */
+		goto out;
+	}
+
+	if (current_pwm > 100)
+		current_pwm = 100;
+	if (current_pwm < 0) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	*state = current_pwm;
+	result = 0;
+
+out:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
+					   unsigned long state)
+{
+	struct pcc_acpi *pcc;
+	int status;
+	int set_pwm;
+	int result;
+
+	pcc = cdev->devdata;
+
+	mutex_lock(&pcc->pwm_fan_lock);
+
+	if (state > 100) {
+		result = -EINVAL;
+		goto out;
+	}
+
+	set_pwm = state;
+	status = pcc_pwm_fan_speed_set(set_pwm);
+	if (status < 0) {
+		pr_err("%s: failed to set fan speed pwm\n", __func__);
+		result = status;
+		goto out;
+	}
+
+	result = 0;
+
+out:
+	mutex_unlock(&pcc->pwm_fan_lock);
+	return result;
+}
+static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
+	.get_max_state = pcc_pwm_fan_thermal_max_state,
+	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
+	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
+};
 
 /* hotkey input device driver */
 
@@ -981,6 +1489,7 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
 
 static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 {
+	const struct dmi_system_id *dmi_id;
 	struct backlight_properties props;
 	struct acpi_device *device;
 	struct pcc_acpi *pcc;
@@ -1026,6 +1535,17 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
 	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
 
+	/*
+	 * Perform quirk detection
+	 */
+	dmi_id = dmi_first_match(pcc_quirks);
+	if (dmi_id) {
+		pcc->quirks = dmi_id->driver_data;
+		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
+	} else {
+		pcc->quirks = NULL;
+	}
+
 	result = acpi_pcc_init_input(pcc);
 	if (result) {
 		pr_err("Error installing keyinput handler\n");
@@ -1097,6 +1617,41 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	}
 
 	i8042_install_filter(panasonic_i8042_filter, NULL);
+
+	/* skip if no quirks */
+	if (!pcc->quirks)
+		goto out_no_quirks;
+
+	/* has_ospm_pwm_fan - add hwmon and thermal if present */
+	if (pcc->quirks->has_ospm_pwm_fan) {
+		mutex_init(&pcc->pwm_fan_lock);
+
+		pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
+
+		pcc->pwm_fan_hwmon = hwmon_device_register_with_info(
+			&pdev->dev, "panasonic_pwm_fan", pcc,
+			&pcc_pwm_fan_hwmon_chip_info, NULL);
+		if (IS_ERR(pcc->pwm_fan_hwmon)) {
+			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
+			/* not a critical error. just skip if error */
+			pcc->pwm_fan_hwmon = NULL;
+		}
+
+		/* proceed with thermal */
+		pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
+		pcc->pwm_fan_cdev = thermal_cooling_device_register(
+			"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
+		if (IS_ERR(pcc->pwm_fan_cdev)) {
+			pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
+			pcc->pwm_fan_cdev = NULL;
+		}
+
+		/* cleanup: destroy mutex if both hwmon and thermal failed */
+		if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
+			mutex_destroy(&pcc->pwm_fan_lock);
+	}
+
+out_no_quirks:
 	return 0;
 
 out_platform:
@@ -1124,6 +1679,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct pcc_acpi *pcc = acpi_driver_data(device);
 
+	if (pcc->pwm_fan_hwmon)
+		hwmon_device_unregister(pcc->pwm_fan_hwmon);
+	if (pcc->pwm_fan_cdev)
+		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
+	if (pcc->pwm_fan_hwmon || pcc->pwm_fan_cdev)
+		mutex_destroy(&pcc->pwm_fan_lock);
+
 	i8042_remove_filter(panasonic_i8042_filter);
 
 	if (pcc->platform) {
-- 
2.54.0

Re: [RFC PATCH v6] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Ilpo Järvinen]

On Sun, 28 Jun 2026, Alex Yeo wrote:

> I have a CF-SR4 and Linux works out of the box. Compared to
> previous models, this one seems to have the fans running at high speed
> by default when the computer starts.
> 
> When Windows 11 is booted up, the high fan speeds persist just until the
> login screen. Once the login screen shows up, the fan spins down.
> 
> I had suspected that this might be the laptop ramping down the fans when
> the OS declares that it is Windows but this does not seem to be the
> case.
> 
> After analyzing the DSDT and SSDT of the computer, I have found the
> following:
> 
> File ssdt10.dsl under \_SB.PC00.LPCB.EC0:
> 
> Name (CEFM, Zero)
> Method (SEFM, 1, Serialized)
> {
>     CEFM = Arg0
>     REFM ()
> }
> 
> Method (REFM, 0, Serialized)
> {
>     If ((\S0IX == 0x03))
>     {
>         Local0 = 0x05
>     }
>     ElseIf ((CEFM == Zero))
>     {
>         Local0 = Zero
>     }
>     Else
>     {
>         Local0 = 0x02
>     }
> 
>     \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
> }
> 
> \_SB.PC00.LPCB.EC0.CEFM would seem be the current value
> for the fan profile. On startup, this is set to 0.
> 
> Based on the code SEFM seems the be the method to set the fan
> profile and REFM is executed right after.
> 
> I don't have access to information as to what the argument officially
> means but based on testing, any number above zero makes the fans spin
> down and behave like the older models where it stops or runs at low
> speed when its cool and ramps up when the processor gets hot.
> 
> The only relevant values for CEFM seem to be just 0 and any number above
> that just gets treated the same. I personally use just 0 and 1.
> 0 seems to be the high fan speed mode and 1 makes it behave like
> Windows.
> 
> Giving 0 as an argument reverts the fan back to the way it was during
> startup where the lowest fan speed is quite high and when load is
> applied, it seems to ramp up to an even higher speed which I think would
> be its 100%.
> 
> A value of 1 seems to have its max speed capped lower than 0.
> 
> For both modes, fan management is still automatic.
> 
> fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
> \_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
> show up on unsupported models. I have tried not hiding it and it just
> outputs a generic error when the value is read.
> 
> I also saw that variables such as eco_mode are kept in memory, however
> for fan_mode I rely on getting and setting the value via ACPI.
> 
> Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
> ---
> New in v6:
>  - Addressed: [Low] The global `quirks` variable is assigned in a DMI
>    callback but never used.
>  - Addressed: [Medium] The `pcc_pwm_fan_hwmon_read` function lacks
>    bounds checking for the raw PWM speed returned by the hardware,
>    potentially violating the hwmon ABI.
>  - Addressed: [Medium] The driver assigns standard Linux error codes to
>    an `acpi_status` variable and evaluates them using `ACPI_FAILURE()`
>    by clarifying return codes as int when not dealing with ACPI return
>    values
>  - Moved hwmon code into separate helper functions as the check logic to
>    ensure safety was complex enough to go past the tab/indentation
>    limit
>  - hwmon: removed behavior where setting of fan PWM while on EC/auto
>    mode will toggle manual mode. This current version will block that
>    action. It only works if pwm1_enable is set. 
>  - separated the if statement on the quirks check since having it nested
>    was using too many indentations.
> 
> For the following, I have clarifications:
>  - [High] Returning `100` (max_state) for the thermal cooling device's
>    current state when in AUTO mode defeats the thermal governor and
>    permanently degrades cooling.
>  - [Low] The driver introduces a private mutex (`pwm_fan_lock`) instead
>    of using the hwmon subsystem's locking mechanism.
> 
> > - [Low] The driver introduces a private mutex (`pwm_fan_lock`) instead of using the hwmon subsystem's locking mechanism.
> 
> For this concern, I have based the mutex pattern from another driver
> (asus-wmi.c and thinkpad_acpi.c). From what I can gather, having
> a mutex that is managed by the driver seems to be the pattern and
> I utilize this since the lock is supposed to prevent resource
> contention between the hwmon and thermal subsystem when the fan
> mode and speed are read and set.
> 
> > - [High] Returning `100` (max_state) for the thermal cooling device's current state when in AUTO mode defeats the thermal governor and permanently degrades cooling.
> 
> This one is a bit confusing to me as cooling is never degraded during
> the entire processes. In fact, the decision to report 100 when the EC
> controls the fan in its failsafe mode ensures cooling is not degraded.
> 
> I will elaborate on how the specifics:
> 
> When the computer is booted up (or rebooted), the BIOS hands control of
> the fan to the EC. The EC by default has an aggressive fan curve where
> the lowest speed the fan could spin is close to around 80% speed. This
> is noticeable as the fan is quite audible.
> 
> When Windows boots up, just before the login screen the fans suddenly
> quiet down and the speed is adjusted automatically by the OS. From the
> ACPI spec, this is referred to as OSPM.
> 
> In the case of Linux, when this driver is loaded, it cannot tell the
> actual fan speed or the current PWM value. This is because the vendor
> chose to not have those values reported via ACPI when queried. The only
> thing that is certain is if the fan is in EC (auto) mode or in manual
> mode. When in EC/auto mode, as the fan is spinning at a high value, it
> relies on the OS to give the signal to control the fan manually and give
> a value between 0-100.
> 
> On the vendor's driver on Windows this can be done, however for the
> Linux driver having the driver mess with any of this commands unprompted
> by the user should not happen. So instead I report 100 when the EC is on
> auto mode as this should prompt userspace tools to set a fan value when
> the temperature is too cold for that high of a fan speed.
> 
> Cooling is not degraded in any way in this process and it relies on a
> tool like thermald to set the fan speed.
> 
> I have also observed that when it gets too hot (~90C), the EC turns
> the fan on automatically.
> 
> > - [Medium] The driver assigns standard Linux error codes to an `acpi_status` variable and evaluates them using `ACPI_FAILURE()`.
> 
> I have reviewed this one and this is something I missed as I had used
> acpi_status in places where it should not have been used at all.
> 
> > +	int status;
> 
> I have rewrote those instances so that an int is used instead.
> 
> > - [Medium] The `pcc_pwm_fan_hwmon_read` function lacks bounds checking for the raw PWM speed returned by the hardware, potentially violating the hwmon ABI.
> 
> > +	/* prevent out-of-bounds */
> > +	if (obj->package.elements[1].integer.value > 100) {
> > +		pr_err("_FST element at index 1 is an integer out of bounds\n");
> > +		result = -EINVAL;
> > +		goto out;
> > +	}
> 
> This has been added. This is an unsigned int so I only check if its
> above 100.
> 
> > - [Low] The global `quirks` variable is assigned in a DMI callback but never used.
> 
> I have rewrote the code so that a callback is not used and as an
> extension the global quirks variable is not used.
> 
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		return pcc_pwm_fan_hwmon_speed_read(pcc, val);
> > +	case hwmon_pwm_enable:
> > +		return pcc_pwm_fan_hwmon_mode_read(pcc, val);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> 
> The hwmon read and write function has been modified to use helper
> functions since the amount of indentations was too much.
> 
> > +	/* skip if no quirks */
> > +	if (!pcc->quirks)
> > +		goto out_no_quirks;
> > +
> > +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> > +	if (pcc->quirks->has_ospm_pwm_fan) {
> > [...]
> > +out_no_quirks:
> >  	return 0;
> 
> I have also separated these checks for quirks as it was using too
> many indentations.
> 
> > +	/* check first if manual control is enabled */
> > +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> > +	if (status < 0) {
> > +		pr_err("%s: failed to get fan pwm mode\n", __func__);
> > +		result = status;
> > +		goto out_unlock;
> > +	}
> > +
> > +	/* do not allow settings speeds if not manual mode */
> > +	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL) {
> > +		result = -EOPNOTSUPP;
> > +		goto out_unlock;
> > +	}
> 
> The hwmon fan now disallows setting of PWM values if the fan is set to
> EC/auto mode.
> 
> $ echo 1 | sudo tee /sys/class/hwmon/hwmon6/pwm1_enable # manual mode
> 
> Has to be done first to allow PWM control.
> 
> 
> I have also tested this and it compiles and works as intended in my
> case.
> 
>  drivers/platform/x86/panasonic-laptop.c | 590 +++++++++++++++++++++++-
>  1 file changed, 576 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f88..2d2acd830fa4 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
> @@ -119,6 +119,9 @@
>   *		- v0.1  start from toshiba_acpi driver written by John Belmonte
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/printk.h>
>  #include <linux/acpi.h>
>  #include <linux/backlight.h>
>  #include <linux/bits.h>
> @@ -136,6 +139,12 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <acpi/video.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/thermal.h>
> +#include <linux/dmi.h>
> +#include <linux/errno.h>
>  
>  MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
>  MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
> @@ -164,6 +173,17 @@ MODULE_LICENSE("GPL");
>  
>  #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
>  
> +/* Define fan PWM modes */
> +#define ACPI_PCC_FAN_PWM_AUTO		0x00
> +#define ACPI_PCC_FAN_PWM_MANUAL		0x01
> +#define HWMON_PCC_FAN_PWM_AUTO		0x02
> +#define HWMON_PCC_FAN_PWM_MANUAL	0x01
> +
> +/* Define quirks for this driver */
> +struct quirk_entry {
> +	bool has_ospm_pwm_fan;
> +};
> +
>  /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
>     ECO_MODEs: 0x03 = off, 0x83 = on
>  */
> @@ -238,19 +258,44 @@ static const struct key_entry panasonic_keymap[] = {
>  };
>  
>  struct pcc_acpi {
> -	acpi_handle		handle;
> -	unsigned long		num_sifr;
> -	int			sticky_key;
> -	int			eco_mode;
> -	int			mute;
> -	int			ac_brightness;
> -	int			dc_brightness;
> -	int			current_brightness;
> -	u32			*sinf;
> -	struct acpi_device	*device;
> -	struct input_dev	*input_dev;
> -	struct backlight_device	*backlight;
> -	struct platform_device	*platform;
> +	acpi_handle			handle;
> +	unsigned long			num_sifr;
> +	int				sticky_key;
> +	int				eco_mode;
> +	int				mute;
> +	int				ac_brightness;
> +	int				dc_brightness;
> +	int				current_brightness;
> +	u32				*sinf;
> +	struct quirk_entry		*quirks;
> +	struct acpi_device		*device;
> +	struct input_dev		*input_dev;
> +	struct backlight_device		*backlight;
> +	struct platform_device		*platform;
> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +	struct device			*pwm_fan_hwmon;
> +	struct mutex			pwm_fan_lock;

Please add a comment to explain what the mutex protects.

> +};
> +
> +/*
> + * Declare quirks and apply matches
> + */
> +
> +static struct quirk_entry quirk_cf_sr4 = {
> +	.has_ospm_pwm_fan = true,
> +};
> +
> +/* DMI matching for quirks copied from asus-nb-wmi.c */
> +static const struct dmi_system_id pcc_quirks[] = {
> +	{
> +		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
> +		},
> +		.driver_data = &quirk_cf_sr4,
> +	},
> +	{},
>  };
>  
>  /*
> @@ -415,7 +460,6 @@ static const struct backlight_ops pcc_backlight_ops = {
>  	.update_status	= bl_set_status,
>  };
>  
> -

Spurious change.

>  /* returns ACPI_SUCCESS if methods to control optical drive are present */
>  
>  static acpi_status check_optd_present(void)
> @@ -507,6 +551,187 @@ static int set_optd_power_state(int new_state)
>  	return result;
>  }
>  
> +/* get OSPM fan mode */
> +
> +static int pcc_pwm_fan_mode_read(int *pwm_mode)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +	int result;
> +
> +	/* BIOS default is zero which seems to be some sort of failsafe mode */
> +	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
> +				       &state);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("error: cannot get fan mode via CEFM\n");

This is already pr_err so printing also "error:" seems unnecessary.

> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	/* use hwmon convention for pwm_mode */
> +	if (state == ACPI_PCC_FAN_PWM_AUTO) {
> +		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
> +	} else if (state != 0 && state > 0) {
> +		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
> +	} else {
> +		/* unknown */
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	result = 0;
> +
> +out:
> +	return result;
> +}
> +
> +/* set OSPM fan mode */
> +
> +static int pcc_pwm_fan_mode_set(int pwm_mode)
> +{
> +	acpi_status status;
> +	int result;
> +
> +	union acpi_object param[1];
> +	struct acpi_object_list input;
> +
> +	param[0].type = ACPI_TYPE_INTEGER;
> +	param[0].integer.value = pwm_mode;
> +	input.count = 1; /* takes one arg */
> +	input.pointer = param;
> +
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
> +				      NULL);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("error: cannot set fan mode via SEFM\n");

Same here.

> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	result = 0;
> +
> +out:
> +	return result;

Drop the label and result variable, and do returns directly:
		return -EIO;
	}

	return 0;

> +}
> +
> +/* read PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_read(int *pwm_speed)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	acpi_status status;
> +	int pwm_mode;
> +	int result;
> +
> +
> +	/* Get fan status first */
> +	/* If fan is not in manual mode, it will return a bogus value */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan status\n", __func__);

Don't print __func__ in anything meant for normal user consumption, write 
the messages without C "oddities" for normal users.

> +		result = status;
> +		goto out;
> +	}
> +
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		result = -ENODATA; /* Indeterminate value */
> +		goto out;
> +	}
> +
> +	/* get pwm speed */
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
> +				      NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("%s: failed to get pwm speed\n", __func__);
> +		result = -EIO;
> +		goto out;

These should all just return directly.

> +	}
> +
> +	obj = buffer.pointer;

Please use __free() (move variable declaration here mid-function as 
explained in cleanup.h) and remove out label entirely.

> +
> +	/* the structure should have 3 values */
> +	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
> +		pr_err("Invalid _FST package structure (expected 3)\n");
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* the second element should be the pwm speed as an int */
> +	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
> +		pr_err("_FST element at index 1 is not an integer\n");
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* prevent out-of-bounds */
> +	if (obj->package.elements[1].integer.value > 100) {
> +		pr_err("_FST element at index 1 is an integer out of bounds\n");
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	*pwm_speed = obj->package.elements[1].integer.value;
> +	result = 0;
> +
> +out:
> +	kfree(buffer.pointer);

Replace this with __free()

> +	return result;
> +}
> +
> +/* set PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> +{
> +	struct acpi_object_list input;
> +	union acpi_object param[1];
> +	acpi_status fsl_status;
> +	int pwm_mode;
> +	int status;
> +	int result;
> +
> +	/* Get fan status. set to manual if not set */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan status\n", __func__);

As mentioned above no __func__ please.

> +		result = status;
> +		goto out;
> +	}
> +
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
> +		if (status < 0) {
> +			pr_err("%s: set fan PWM to manual failed\n", __func__);
> +			result = status;
> +			goto out;
> +		}
> +	}
> +
> +	/* check if within bounds */
> +	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
> +		pr_err("%s: error: fan speed level out of bounds\n", __func__);
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	param[0].type = ACPI_TYPE_INTEGER;
> +	param[0].integer.value = set_pwm_speed;
> +	input.count = 1; /* takes one arg */
> +	input.pointer = param;
> +
> +	fsl_status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
> +				      &input, NULL);
> +	if (ACPI_FAILURE(fsl_status)) {
> +		pr_err("Setting of fan speed via ._FSL failed.\n");
> +		result = -EIO;
> +		goto out;
> +	}
> +
> +	result = 0;
> +
> +out:
> +	return result;

Please apply similar return + goto removal transitions as mentioned for 
the other functions.

> +}
>  
>  /* sysfs user interface functions */
>  
> @@ -676,6 +901,7 @@ static ssize_t ac_brightness_show(struct device *dev, struct device_attribute *a
>  	return sysfs_emit(buf, "%u\n", pcc->sinf[SINF_AC_CUR_BRIGHT]);
>  }
>  
> +

Spurious change.

>  static ssize_t ac_brightness_store(struct device *dev, struct device_attribute *attr,
>  				   const char *buf, size_t count)
>  {
> @@ -825,6 +1051,288 @@ static const struct attribute_group pcc_attr_group = {
>  	.is_visible	= pcc_sysfs_is_visible,
>  };
>  
> +/* hwmon */
> +
> +static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE), NULL

Put NULL on own line for clarity.

> +};
> +
> +static int pcc_pwm_fan_hwmon_speed_read(struct pcc_acpi *pcc, long *val)
> +{
> +	int pwm_speed;
> +	int result;
> +	int status;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);

Please use guard() and remove all gotos.

> +
> +	status = pcc_pwm_fan_speed_read(&pwm_speed);
> +	if (status < 0) {
> +		result = status;
> +		goto out_unlock;
> +	}
> +
> +	/* protect against out-of-bounds */
> +	if (pwm_speed < 0 || pwm_speed > 100) {
> +		result = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	*val = (pwm_speed * 255) / 100;
> +	result = 0;
> +
> +out_unlock:
> +	mutex_unlock(&pcc->pwm_fan_lock);
> +	return result;
> +}
> +
> +static int pcc_pwm_fan_hwmon_mode_read(struct pcc_acpi *pcc, long *val)
> +{
> +	int pwm_mode;
> +	int result;
> +	int status;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);

guard()

> +
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan pwm mode\n", __func__);

You know the drill by now.

> +		result = status;
> +		goto out_unlock;
> +	}
> +	switch (pwm_mode) {
> +	case HWMON_PCC_FAN_PWM_AUTO:
> +		*val = HWMON_PCC_FAN_PWM_AUTO;
> +		result = 0;
> +		break;
> +	case HWMON_PCC_FAN_PWM_MANUAL:
> +		*val = HWMON_PCC_FAN_PWM_MANUAL;
> +		result = 0;
> +		break;
> +	default:
> +		result = -EINVAL;
> +		break;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&pcc->pwm_fan_lock);
> +	return result;
> +}
> +
> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
> +	struct pcc_acpi *pcc;
> +
> +	pcc = dev_get_drvdata(dev);

Put this assignment to the variable declaration line.

> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		return pcc_pwm_fan_hwmon_speed_read(pcc, val);
> +	case hwmon_pwm_enable:
> +		return pcc_pwm_fan_hwmon_mode_read(pcc, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pcc_pwm_fan_hwmon_mode_set(struct pcc_acpi *pcc, long val)
> +{
> +	int result;
> +
> +	switch (val) {
> +	case HWMON_PCC_FAN_PWM_AUTO:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +		result = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_AUTO);
> +		goto out_unlock;
> +	case HWMON_PCC_FAN_PWM_MANUAL:
> +		mutex_lock(&pcc->pwm_fan_lock);
> +		result = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
> +		goto out_unlock;
> +	default:
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&pcc->pwm_fan_lock);

This too should use guard().

> +out:
> +	return result;
> +}
> +
> +static int pcc_pwm_fan_hwmon_speed_set(struct pcc_acpi *pcc, long val)
> +{
> +	int pwm_speed_vendor_val;
> +	int pwm_mode;
> +	int status;
> +	int result;
> +
> +	val = clamp_val(val, 0, 255);

Add include for clamp_val().

Many earlier comments apply to this function as well and some others 
below as well.

> +	pwm_speed_vendor_val = (val * 100) / 255;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);
> +
> +	/* check first if manual control is enabled */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan pwm mode\n", __func__);
> +		result = status;
> +		goto out_unlock;
> +	}
> +
> +	/* do not allow settings speeds if not manual mode */
> +	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL) {
> +		result = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
> +
> +	status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
> +	if (status < 0) {
> +		pr_err("%s: failed to set fan pwm\n", __func__);
> +		result = status;
> +		goto out_unlock;
> +	}
> +	result = 0;
> +
> +out_unlock:
> +	mutex_unlock(&pcc->pwm_fan_lock);
> +	return result;
> +}
> +
> +static int pcc_pwm_fan_hwmon_write(struct device *dev,
> +				   enum hwmon_sensor_types type, u32 attr,
> +				   int channel, long val)
> +{
> +	struct pcc_acpi *pcc;
> +
> +	pcc = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		return pcc_pwm_fan_hwmon_mode_set(pcc, val);
> +	case hwmon_pwm_input:
> +		return pcc_pwm_fan_hwmon_speed_set(pcc, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
> +					    enum hwmon_sensor_types type,
> +					    u32 attr, int channel)
> +{
> +	if (type != hwmon_pwm)
> +		return 0;
> +
> +	return 0644;
> +}
> +
> +static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
> +	.is_visible = pcc_pwm_fan_hwmon_is_visible,
> +	.read = pcc_pwm_fan_hwmon_read,
> +	.write = pcc_pwm_fan_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
> +	.ops = &pcc_pwm_fan_hwmon_ops,
> +	.info = pcc_pwm_fan_hwmon_info,
> +};
> +
> +/* thermal interface */
> +static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
> +					 unsigned long *state)
> +{
> +	*state = 100; /* range of 0-100 as per UEFI spec */
> +	return 0;
> +}
> +static int
> +pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
> +				    unsigned long *state)
> +{
> +	struct pcc_acpi *pcc;
> +	int current_pwm;
> +	int pwm_mode;
> +	int result;
> +	int status;
> +
> +	pcc = cdev->devdata;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);
> +
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan pwm mode\n", __func__);
> +		result = status;
> +		goto out;
> +	}
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		*state = 100; /* Return failsafe value on EC mode */
> +		result = 0;
> +		goto out;
> +	}
> +
> +	status = pcc_pwm_fan_speed_read(&current_pwm);
> +	if (status < 0) {
> +		result = status; /* pass error code */
> +		goto out;
> +	}
> +
> +	if (current_pwm > 100)
> +		current_pwm = 100;
> +	if (current_pwm < 0) {
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	*state = current_pwm;
> +	result = 0;
> +
> +out:
> +	mutex_unlock(&pcc->pwm_fan_lock);
> +	return result;
> +}
> +static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
> +					   unsigned long state)
> +{
> +	struct pcc_acpi *pcc;
> +	int status;
> +	int set_pwm;
> +	int result;
> +
> +	pcc = cdev->devdata;
> +
> +	mutex_lock(&pcc->pwm_fan_lock);
> +
> +	if (state > 100) {
> +		result = -EINVAL;
> +		goto out;
> +	}
> +
> +	set_pwm = state;
> +	status = pcc_pwm_fan_speed_set(set_pwm);
> +	if (status < 0) {
> +		pr_err("%s: failed to set fan speed pwm\n", __func__);
> +		result = status;
> +		goto out;
> +	}
> +
> +	result = 0;
> +
> +out:
> +	mutex_unlock(&pcc->pwm_fan_lock);
> +	return result;
> +}
> +static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
> +	.get_max_state = pcc_pwm_fan_thermal_max_state,
> +	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
> +	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
> +};
>  
>  /* hotkey input device driver */
>  
> @@ -981,6 +1489,7 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
>  
>  static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  {
> +	const struct dmi_system_id *dmi_id;
>  	struct backlight_properties props;
>  	struct acpi_device *device;
>  	struct pcc_acpi *pcc;
> @@ -1026,6 +1535,17 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
>  	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
>  
> +	/*
> +	 * Perform quirk detection
> +	 */
> +	dmi_id = dmi_first_match(pcc_quirks);
> +	if (dmi_id) {
> +		pcc->quirks = dmi_id->driver_data;
> +		pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);

IMO, this should be debug level if useful at all.

> +	} else {
> +		pcc->quirks = NULL;

Isn't the default/initialization value NULL anyway so why you need to set 
it here?

> +	}
> +
>  	result = acpi_pcc_init_input(pcc);
>  	if (result) {
>  		pr_err("Error installing keyinput handler\n");
> @@ -1097,6 +1617,41 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  	}
>  
>  	i8042_install_filter(panasonic_i8042_filter, NULL);
> +
> +	/* skip if no quirks */
> +	if (!pcc->quirks)
> +		goto out_no_quirks;
> +
> +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +	if (pcc->quirks->has_ospm_pwm_fan) {
> +		mutex_init(&pcc->pwm_fan_lock);

I suggest you always initialize the mutex. Doing it conditionally it made 
your cleanup trickier (and more prone to corner cases if somebody changes 
some of the checks). What's the benefit for doing in conditionally?

> +
> +		pr_info("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> +
> +		pcc->pwm_fan_hwmon = hwmon_device_register_with_info(
> +			&pdev->dev, "panasonic_pwm_fan", pcc,
> +			&pcc_pwm_fan_hwmon_chip_info, NULL);
> +		if (IS_ERR(pcc->pwm_fan_hwmon)) {
> +			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +			/* not a critical error. just skip if error */
> +			pcc->pwm_fan_hwmon = NULL;
> +		}
> +
> +		/* proceed with thermal */
> +		pr_info("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> +		pcc->pwm_fan_cdev = thermal_cooling_device_register(
> +			"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
> +		if (IS_ERR(pcc->pwm_fan_cdev)) {
> +			pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
> +			pcc->pwm_fan_cdev = NULL;
> +		}
> +
> +		/* cleanup: destroy mutex if both hwmon and thermal failed */
> +		if (!pcc->pwm_fan_hwmon && !pcc->pwm_fan_cdev)
> +			mutex_destroy(&pcc->pwm_fan_lock);
> +	}
> +
> +out_no_quirks:
>  	return 0;
>  
>  out_platform:
> @@ -1124,6 +1679,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct pcc_acpi *pcc = acpi_driver_data(device);
>  
> +	if (pcc->pwm_fan_hwmon)
> +		hwmon_device_unregister(pcc->pwm_fan_hwmon);
> +	if (pcc->pwm_fan_cdev)
> +		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
> +	if (pcc->pwm_fan_hwmon || pcc->pwm_fan_cdev)
> +		mutex_destroy(&pcc->pwm_fan_lock);
> +
>  	i8042_remove_filter(panasonic_i8042_filter);
>  
>  	if (pcc->platform) {
> 

-- 
 i.

[RFC PATCH v7] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Alex Yeo]

I have a CF-SR4 and Linux works out of the box. Compared to
previous models, this one seems to have the fans running at high speed
by default when the computer starts.

When Windows 11 is booted up, the high fan speeds persist just until the
login screen. Once the login screen shows up, the fan spins down.

I had suspected that this might be the laptop ramping down the fans when
the OS declares that it is Windows but this does not seem to be the
case.

After analyzing the DSDT and SSDT of the computer, I have found the
following:

File ssdt10.dsl under \_SB.PC00.LPCB.EC0:

Name (CEFM, Zero)
Method (SEFM, 1, Serialized)
{
    CEFM = Arg0
    REFM ()
}

Method (REFM, 0, Serialized)
{
    If ((\S0IX == 0x03))
    {
        Local0 = 0x05
    }
    ElseIf ((CEFM == Zero))
    {
        Local0 = Zero
    }
    Else
    {
        Local0 = 0x02
    }

    \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
}

\_SB.PC00.LPCB.EC0.CEFM would seem be the current value
for the fan profile. On startup, this is set to 0.

Based on the code SEFM seems the be the method to set the fan
profile and REFM is executed right after.

I don't have access to information as to what the argument officially
means but based on testing, any number above zero makes the fans spin
down and behave like the older models where it stops or runs at low
speed when its cool and ramps up when the processor gets hot.

The only relevant values for CEFM seem to be just 0 and any number above
that just gets treated the same. I personally use just 0 and 1.
0 seems to be the high fan speed mode and 1 makes it behave like
Windows.

Giving 0 as an argument reverts the fan back to the way it was during
startup where the lowest fan speed is quite high and when load is
applied, it seems to ramp up to an even higher speed which I think would
be its 100%.

A value of 1 seems to have its max speed capped lower than 0.

For both modes, fan management is still automatic.

fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
\_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
show up on unsupported models. I have tried not hiding it and it just
outputs a generic error when the value is read.

I also saw that variables such as eco_mode are kept in memory, however
for fan_mode I rely on getting and setting the value via ACPI.

Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
---
 drivers/platform/x86/panasonic-laptop.c | 500 +++++++++++++++++++++++-
 1 file changed, 486 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index b83113c26f88..2d67188de2a9 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -119,6 +119,9 @@
  *		- v0.1  start from toshiba_acpi driver written by John Belmonte
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/printk.h>
 #include <linux/acpi.h>
 #include <linux/backlight.h>
 #include <linux/bits.h>
@@ -136,6 +139,14 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <acpi/video.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/thermal.h>
+#include <linux/dmi.h>
+#include <linux/errno.h>
+#include <linux/cleanup.h>
+#include <linux/minmax.h>
 
 MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
 MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
@@ -164,6 +175,17 @@ MODULE_LICENSE("GPL");
 
 #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
 
+/* Define fan PWM modes */
+#define ACPI_PCC_FAN_PWM_AUTO		0x00
+#define ACPI_PCC_FAN_PWM_MANUAL		0x01
+#define HWMON_PCC_FAN_PWM_AUTO		0x02
+#define HWMON_PCC_FAN_PWM_MANUAL	0x01
+
+/* Define quirks for this driver */
+struct quirk_entry {
+	bool has_ospm_pwm_fan;
+};
+
 /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
    ECO_MODEs: 0x03 = off, 0x83 = on
 */
@@ -238,19 +260,50 @@ static const struct key_entry panasonic_keymap[] = {
 };
 
 struct pcc_acpi {
-	acpi_handle		handle;
-	unsigned long		num_sifr;
-	int			sticky_key;
-	int			eco_mode;
-	int			mute;
-	int			ac_brightness;
-	int			dc_brightness;
-	int			current_brightness;
-	u32			*sinf;
-	struct acpi_device	*device;
-	struct input_dev	*input_dev;
-	struct backlight_device	*backlight;
-	struct platform_device	*platform;
+	acpi_handle			handle;
+	unsigned long			num_sifr;
+	int				sticky_key;
+	int				eco_mode;
+	int				mute;
+	int				ac_brightness;
+	int				dc_brightness;
+	int				current_brightness;
+	u32				*sinf;
+	struct quirk_entry		*quirks;
+	struct acpi_device		*device;
+	struct input_dev		*input_dev;
+	struct backlight_device		*backlight;
+	struct platform_device		*platform;
+	struct thermal_cooling_device	*pwm_fan_cdev;
+	struct device			*pwm_fan_hwmon;
+
+	/*
+	 * This mutex ensures that the hwmon and thermal functions
+	 * for fan operations do not conflict as the PWM fan is
+	 * exposed to both.
+	 */
+	struct mutex			pwm_fan_lock;
+};
+
+/*
+ * Declare quirks
+ */
+
+static struct quirk_entry quirk_cf_sr4 = {
+	.has_ospm_pwm_fan = true,
+};
+
+/* DMI matching for quirks copied from asus-nb-wmi.c */
+static const struct dmi_system_id pcc_quirks[] = {
+	{
+		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
+		},
+		.driver_data = &quirk_cf_sr4,
+	},
+	{},
 };
 
 /*
@@ -415,7 +468,6 @@ static const struct backlight_ops pcc_backlight_ops = {
 	.update_status	= bl_set_status,
 };
 
-
 /* returns ACPI_SUCCESS if methods to control optical drive are present */
 
 static acpi_status check_optd_present(void)
@@ -507,6 +559,155 @@ static int set_optd_power_state(int new_state)
 	return result;
 }
 
+/* get OSPM fan mode */
+
+static int pcc_pwm_fan_mode_read(int *pwm_mode)
+{
+	unsigned long long state;
+	acpi_status status;
+
+	/* BIOS default is zero which seems to be some sort of failsafe mode */
+	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
+				       &state);
+	if (ACPI_FAILURE(status)) {
+		pr_err("cannot get fan mode via CEFM\n");
+		return -EIO;
+	}
+
+	/* use hwmon convention for pwm_mode */
+	if (state == ACPI_PCC_FAN_PWM_AUTO) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
+	} else if (state != 0 && state > 0) {
+		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
+	} else {
+		/* unknown */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* set OSPM fan mode */
+
+static int pcc_pwm_fan_mode_set(int pwm_mode)
+{
+	acpi_status status;
+
+	union acpi_object param[1];
+	struct acpi_object_list input;
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = pwm_mode;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
+				      NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("cannot set fan mode via SEFM\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/* read PWM fan speed */
+
+static int pcc_pwm_fan_speed_read(int *pwm_speed)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+	int pwm_mode;
+
+
+	/* Get fan status first */
+	/* If fan is not in manual mode, it will return a bogus value */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("failed to get fan status\n");
+		return status;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO)
+		return -ENODATA; /* Indeterminate value */
+
+	/* get pwm speed */
+	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
+				      NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		pr_err("failed to get pwm speed\n");
+		return -EIO;
+	}
+
+	union acpi_object *obj __free(kfree) = buffer.pointer;
+
+	/* the structure should have 3 values */
+	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
+		pr_err("Invalid _FST package structure (expected 3)\n");
+		return -EINVAL;
+	}
+
+	/* the second element should be the pwm speed as an int */
+	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
+		pr_err("_FST element at index 1 is not an integer\n");
+		return -EINVAL;
+	}
+
+	/* prevent out-of-bounds */
+	if (obj->package.elements[1].integer.value > 100) {
+		pr_err("_FST element at index 1 is an integer out of bounds\n");
+		return -EINVAL;
+	}
+
+	*pwm_speed = obj->package.elements[1].integer.value;
+	return 0;
+}
+
+/* set PWM fan speed */
+
+static int pcc_pwm_fan_speed_set(int set_pwm_speed)
+{
+	struct acpi_object_list input;
+	union acpi_object param[1];
+	acpi_status fsl_status;
+	int pwm_mode;
+	int status;
+
+	/* Get fan status. set to manual if not set */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("failed to get fan status\n");
+		return status;
+	}
+
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
+		if (status < 0) {
+			pr_err("set fan PWM to manual failed\n");
+			return status;
+		}
+	}
+
+	/* check if within bounds */
+	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
+		pr_err("fan speed level out of bounds\n");
+		return -EIO;
+	}
+
+	param[0].type = ACPI_TYPE_INTEGER;
+	param[0].integer.value = set_pwm_speed;
+	input.count = 1; /* takes one arg */
+	input.pointer = param;
+
+	fsl_status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
+				      &input, NULL);
+	if (ACPI_FAILURE(fsl_status)) {
+		pr_err("Setting of fan speed via ._FSL failed.\n");
+		return -EIO;
+	}
+
+	return 0;
+}
 
 /* sysfs user interface functions */
 
@@ -825,6 +1026,228 @@ static const struct attribute_group pcc_attr_group = {
 	.is_visible	= pcc_sysfs_is_visible,
 };
 
+/* hwmon */
+
+static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+	NULL
+};
+
+static int pcc_pwm_fan_hwmon_speed_read(struct pcc_acpi *pcc, long *val)
+{
+	int pwm_speed;
+	int status;
+
+	guard(mutex)(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_speed_read(&pwm_speed);
+	if (status < 0)
+		return status;
+
+	/* protect against out-of-bounds */
+	if (pwm_speed < 0 || pwm_speed > 100)
+		return -EINVAL;
+
+	*val = (pwm_speed * 255) / 100;
+	return 0;
+}
+
+static int pcc_pwm_fan_hwmon_mode_read(struct pcc_acpi *pcc, long *val)
+{
+	int pwm_mode;
+	int status;
+
+	guard(mutex)(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("failed to get fan pwm mode\n");
+		return status;
+	}
+
+	switch (pwm_mode) {
+	case HWMON_PCC_FAN_PWM_AUTO:
+		*val = HWMON_PCC_FAN_PWM_AUTO;
+		return 0;
+	case HWMON_PCC_FAN_PWM_MANUAL:
+		*val = HWMON_PCC_FAN_PWM_MANUAL;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_read(struct device *dev,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel, long *val)
+{
+	struct pcc_acpi *pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		return pcc_pwm_fan_hwmon_speed_read(pcc, val);
+	case hwmon_pwm_enable:
+		return pcc_pwm_fan_hwmon_mode_read(pcc, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_mode_set(struct pcc_acpi *pcc, long val)
+{
+	switch (val) {
+	case HWMON_PCC_FAN_PWM_AUTO:
+		guard(mutex)(&pcc->pwm_fan_lock);
+		return pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_AUTO);
+	case HWMON_PCC_FAN_PWM_MANUAL:
+		guard(mutex)(&pcc->pwm_fan_lock);
+		return pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pcc_pwm_fan_hwmon_speed_set(struct pcc_acpi *pcc, long val)
+{
+	int pwm_speed_vendor_val;
+	int pwm_mode;
+	int status;
+
+	val = clamp_val(val, 0, 255);
+	pwm_speed_vendor_val = (val * 100) / 255;
+
+	guard(mutex)(&pcc->pwm_fan_lock);
+
+	/* check first if manual control is enabled */
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("failed to get fan pwm mode\n");
+		return status;
+	}
+
+	/* do not allow settings speeds if not manual mode */
+	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL)
+		return -EOPNOTSUPP;
+
+	status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
+	if (status < 0) {
+		pr_err("failed to set fan pwm\n");
+		return status;
+	}
+	return 0;
+}
+
+static int pcc_pwm_fan_hwmon_write(struct device *dev,
+				   enum hwmon_sensor_types type, u32 attr,
+				   int channel, long val)
+{
+	struct pcc_acpi *pcc;
+
+	pcc = dev_get_drvdata(dev);
+
+	if (type != hwmon_pwm)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		return pcc_pwm_fan_hwmon_mode_set(pcc, val);
+	case hwmon_pwm_input:
+		return pcc_pwm_fan_hwmon_speed_set(pcc, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
+					    enum hwmon_sensor_types type,
+					    u32 attr, int channel)
+{
+	if (type != hwmon_pwm)
+		return 0;
+
+	return 0644;
+}
+
+static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
+	.is_visible = pcc_pwm_fan_hwmon_is_visible,
+	.read = pcc_pwm_fan_hwmon_read,
+	.write = pcc_pwm_fan_hwmon_write,
+};
+
+static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
+	.ops = &pcc_pwm_fan_hwmon_ops,
+	.info = pcc_pwm_fan_hwmon_info,
+};
+
+/* thermal interface */
+static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
+					 unsigned long *state)
+{
+	*state = 100; /* range of 0-100 as per UEFI spec */
+	return 0;
+}
+static int
+pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
+				    unsigned long *state)
+{
+	struct pcc_acpi *pcc = cdev->devdata;
+	int current_pwm;
+	int pwm_mode;
+	int status;
+
+	guard(mutex)(&pcc->pwm_fan_lock);
+
+	status = pcc_pwm_fan_mode_read(&pwm_mode);
+	if (status < 0) {
+		pr_err("%s: failed to get fan pwm mode\n", __func__);
+		return status;
+	}
+	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
+		*state = 100; /* Return failsafe value on EC mode */
+		return 0;
+	}
+
+	status = pcc_pwm_fan_speed_read(&current_pwm);
+	if (status < 0)
+		return status; /* pass error code */
+
+	if (current_pwm > 100)
+		current_pwm = 100;
+	if (current_pwm < 0)
+		return -EINVAL;
+
+	*state = current_pwm;
+	return 0;
+}
+static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
+					   unsigned long state)
+{
+	struct pcc_acpi *pcc = cdev->devdata;
+	int status;
+	int set_pwm;
+
+	guard(mutex)(&pcc->pwm_fan_lock);
+
+	if (state > 100)
+		return -EINVAL;
+
+	set_pwm = state;
+	status = pcc_pwm_fan_speed_set(set_pwm);
+	if (status < 0) {
+		pr_err("%s: failed to set fan speed pwm\n", __func__);
+		return status;
+	}
+
+	return 0;
+}
+static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
+	.get_max_state = pcc_pwm_fan_thermal_max_state,
+	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
+	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
+};
 
 /* hotkey input device driver */
 
@@ -981,6 +1404,7 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
 
 static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 {
+	const struct dmi_system_id *dmi_id;
 	struct backlight_properties props;
 	struct acpi_device *device;
 	struct pcc_acpi *pcc;
@@ -1013,6 +1437,9 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	/* Initialize mutex */
+	mutex_init(&pcc->pwm_fan_lock);
+
 	pcc->sinf = kcalloc(num_sifr + 1, sizeof(u32), GFP_KERNEL);
 	if (!pcc->sinf) {
 		result = -ENOMEM;
@@ -1026,6 +1453,15 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
 	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
 
+	/*
+	 * Perform quirk detection
+	 */
+	dmi_id = dmi_first_match(pcc_quirks);
+	if (dmi_id) {
+		pcc->quirks = dmi_id->driver_data;
+		pr_debug("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
+	}
+
 	result = acpi_pcc_init_input(pcc);
 	if (result) {
 		pr_err("Error installing keyinput handler\n");
@@ -1097,6 +1533,35 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
 	}
 
 	i8042_install_filter(panasonic_i8042_filter, NULL);
+
+	/* skip if no quirks */
+	if (!pcc->quirks)
+		goto out_no_quirks;
+
+	/* has_ospm_pwm_fan - add hwmon and thermal if present */
+	if (pcc->quirks->has_ospm_pwm_fan) {
+		pr_debug("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
+
+		pcc->pwm_fan_hwmon = hwmon_device_register_with_info(
+			&pdev->dev, "panasonic_pwm_fan", pcc,
+			&pcc_pwm_fan_hwmon_chip_info, NULL);
+		if (IS_ERR(pcc->pwm_fan_hwmon)) {
+			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
+			/* not a critical error. just skip if error */
+			pcc->pwm_fan_hwmon = NULL;
+		}
+
+		/* proceed with thermal */
+		pr_debug("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
+		pcc->pwm_fan_cdev = thermal_cooling_device_register(
+			"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
+		if (IS_ERR(pcc->pwm_fan_cdev)) {
+			pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
+			pcc->pwm_fan_cdev = NULL;
+		}
+	}
+
+out_no_quirks:
 	return 0;
 
 out_platform:
@@ -1124,6 +1589,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
 	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct pcc_acpi *pcc = acpi_driver_data(device);
 
+	if (pcc->pwm_fan_hwmon)
+		hwmon_device_unregister(pcc->pwm_fan_hwmon);
+	if (pcc->pwm_fan_cdev)
+		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
+
+	mutex_destroy(&pcc->pwm_fan_lock);
+
 	i8042_remove_filter(panasonic_i8042_filter);
 
 	if (pcc->platform) {
-- 
New in v7:
 - Added commend to explain mutex pwm_fan_lock
 - Address spurious changes and other comments
 - Removed redundant "error:" when using pr_err
 - Removed __func__ when calling pr_err
 - Drop out label to return directly
 - Use guard via cleanup.h for mutexes (eliminate goto)
 - Used __free() in place of kfree(buffer.pointer)
 - Put NULL on new line for HWMON_CHANNEL_INFO

> Please add a comment to explain what the mutex protects.

I have made the changes here:

> +     /*
> +      * This mutex ensures that the hwmon and thermal functions
> +      * for fan operations do not conflict as the PWM fan is
> +      * exposed to both.
> +      */
> +     struct mutex                    pwm_fan_lock;

For the spurious change referenced here:

>> +/*
>> + * Declare quirks and apply matches
>> + */
>> +
>> +static struct quirk_entry quirk_cf_sr4 = {
>> +     .has_ospm_pwm_fan = true,
>> +};
>> +
>> +/* DMI matching for quirks copied from asus-nb-wmi.c */
>> +static const struct dmi_system_id pcc_quirks[] = {
>> +     {
>> +             .ident = "Panasonic Connect Co., Ltd. CFSR4-1",
>> +             .matches = {
>> +                     DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
>> +                     DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
>> +             },
>> +             .driver_data = &quirk_cf_sr4,
>> +     },
>> +     {},
>>  };
>>
>>  /*
>> @@ -415,7 +460,6 @@ static const struct backlight_ops pcc_backlight_ops = {
>>       .update_status  = bl_set_status,
>>  };
>>
>> -
>
>Spurious change.

The previous sashiko bot code review pointed out that a global variable
related to quirks was unused as there was previously a callback that set
a global variable. As keeping the driver data within the driver struct
is best practice, I have removed that in favor of doing the quirk
detection inside the driver's probe function.

Also, as the callback has now been removed, I have also clarified the
comment to just say "Declare quirks":

>+/*
>+ * Declare quirks
>+ */

>> +/* get OSPM fan mode */
>> +
>> +static int pcc_pwm_fan_mode_read(int *pwm_mode)
>> +{
>> +     unsigned long long state;
>> +     acpi_status status;
>> +     int result;
>> +
>> +     /* BIOS default is zero which seems to be some sort of failsafe mode */
>> +     status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
>> +                                    &state);
>> +     if (ACPI_FAILURE(status)) {
>> +             pr_err("error: cannot get fan mode via CEFM\n");
>
>This is already pr_err so printing also "error:" seems unnecessary.

I have removed all instances in the patch of passing "error:" when
calling pr_err

>> +             result = -EIO;
>> +             goto out;
>> +     }
>> +
>> +     result = 0;
>> +
>> +out:
>> +     return result;
>
>Drop the label and result variable, and do returns directly:
>                return -EIO;
>        }
>
>        return 0;

I have implemented the change to all functions within this patch that
match that pattern. Returns are now direct.

> > +     /* Get fan status first */
> > +     /* If fan is not in manual mode, it will return a bogus value */
> > +     status = pcc_pwm_fan_mode_read(&pwm_mode);
> > +     if (status < 0) {
> > +             pr_err("%s: failed to get fan status\n", __func__);
> 
> Don't print __func__ in anything meant for normal user consumption, write
> the messages without C "oddities" for normal users.

I have removed all __func__ from pr_err in this patch. I have found
additional instances of this that were not quoted. I have made the
change for those too (as indicated) here:

>> +     result = 0;
>> +
>> +out:
>> +     return result;
>
>Please apply similar return + goto removal transitions as mentioned for
>the other functions.


>> +     /* get pwm speed */
>> +     status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
>> +                                   NULL, &buffer);
>> +     if (ACPI_FAILURE(status)) {
>> +             pr_err("%s: failed to get pwm speed\n", __func__);
>> +             result = -EIO;
>> +             goto out;
>
>These should all just return directly.

Implemented for this instance and the other functions that can just
return directly.

>> +     }
>> +
>> +     obj = buffer.pointer;
>
>Please use __free() (move variable declaration here mid-function as
>explained in cleanup.h) and remove out label entirely.

This has been implemented:

> +	/* get pwm speed */
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
> +				      NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("failed to get pwm speed\n");
> +		return -EIO;
> +	}
> +
> +	union acpi_object *obj __free(kfree) = buffer.pointer;
> +

>> +}
>> 
>>  /* sysfs user interface functions */
>> 
>> @@ -676,6 +901,7 @@ static ssize_t ac_brightness_show(struct device *dev, struct device_attribute *a
>>       return sysfs_emit(buf, "%u\n", pcc->sinf[SINF_AC_CUR_BRIGHT]);
>>  }
>> 
>> +
>
>Spurious change.

This was an unintended change that has now been fixed.

>> 
>> +/* hwmon */
>> +
>> +static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
>> +     HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE), NULL
>
>Put NULL on own line for clarity.

Done here:

> +/* hwmon */
> +
> +static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> +	NULL
> +};


>> +static int pcc_pwm_fan_hwmon_speed_read(struct pcc_acpi *pcc, long *val)
>> +{
>> +     int pwm_speed;
>> +     int result;
>> +     int status;
>> +
>> +     mutex_lock(&pcc->pwm_fan_lock);
>
>Please use guard() and remove all gotos.

This has been implemented here and on other functions that it applies
to.

>> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
>> +                               enum hwmon_sensor_types type, u32 attr,
>> +                               int channel, long *val)
>> +{
>> +     struct pcc_acpi *pcc;
>> +
>> +     pcc = dev_get_drvdata(dev);
>
>Put this assignment to the variable declaration line.

This has been done:

>+static int pcc_pwm_fan_hwmon_read(struct device *dev,
>+				  enum hwmon_sensor_types type, u32 attr,
>+				  int channel, long *val)
>+{
>+	struct pcc_acpi *pcc = dev_get_drvdata(dev);
>+
>+	if (type != hwmon_pwm)
>+		return -EOPNOTSUPP;


>> +static int pcc_pwm_fan_hwmon_speed_set(struct pcc_acpi *pcc, long val)
>> +{
>> +     int pwm_speed_vendor_val;
>> +     int pwm_mode;
>> +     int status;
>> +     int result;
>> +
>> +     val = clamp_val(val, 0, 255);
>
>Add include for clamp_val().
>
>Many earlier comments apply to this function as well and some others
>below as well.

I have added this:

> +#include <linux/minmax.h>

For this function, I have also eliminated the goto and just used return
directly. guard(mutex) was also used.

> +static int pcc_pwm_fan_hwmon_speed_set(struct pcc_acpi *pcc, long val)
> +{
> +	int pwm_speed_vendor_val;
> +	int pwm_mode;
> +	int status;
> +
> +	val = clamp_val(val, 0, 255);
> +	pwm_speed_vendor_val = (val * 100) / 255;
> +
> +	guard(mutex)(&pcc->pwm_fan_lock);
> +
> +	/* check first if manual control is enabled */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("failed to get fan pwm mode\n");
> +		return status;
> +	}
> +
> +	/* do not allow settings speeds if not manual mode */
> +	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL) {
> +		return -EOPNOTSUPP;
> +	}
> +
> +	status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
> +	if (status < 0) {
> +		pr_err("failed to set fan pwm\n");
> +		return status;
> +	}
> +	return 0;
> +}


>> +     /*
>> +      * Perform quirk detection
>> +      */
>> +     dmi_id = dmi_first_match(pcc_quirks);
>> +     if (dmi_id) {
>> +             pcc->quirks = dmi_id->driver_data;
>> +             pr_info("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
>
>IMO, this should be debug level if useful at all.

I have used pr_debug instead:

> +	/*
> +	 * Perform quirk detection
> +	 */
> +	dmi_id = dmi_first_match(pcc_quirks);
> +	if (dmi_id) {
> +		pcc->quirks = dmi_id->driver_data;
> +		pr_debug("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
> +	} 
> +

Going further, I have also made changes to the following two instances:

> +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +	if (pcc->quirks->has_ospm_pwm_fan) {
> +		pr_debug("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);

and

> +
> +		/* proceed with thermal */
> +		pr_debug("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> +		pcc->pwm_fan_cdev = thermal_cooling_device_register(


>> +     } else {
>> +             pcc->quirks = NULL;
>
>Isn't the default/initialization value NULL anyway so why you need to set
>it here?

I had gone a bit overboard after the previous rounds of the sashiko bot
review. I have undone that as I do agree it is not necessary.

>> +
>> +     /* skip if no quirks */
>> +     if (!pcc->quirks)
>> +             goto out_no_quirks;
>> +
>> +     /* has_ospm_pwm_fan - add hwmon and thermal if present */
>> +     if (pcc->quirks->has_ospm_pwm_fan) {
>> +             mutex_init(&pcc->pwm_fan_lock);
>
>I suggest you always initialize the mutex. Doing it conditionally it made
>your cleanup trickier (and more prone to corner cases if somebody changes
>some of the checks). What's the benefit for doing in conditionally?

My original mindset was since this is a quirk for this particular model,
I should avoid executing code for older models in order to not break
compatibility. After thinking it trough I do agree that just
initializing the mutex every time is much simpler and makes the code
more readable.

I have placed the mutex declaration after the struct was allocated
memory:

>@@ -1012,6 +1442,9 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
> 		pr_err("Couldn't allocate mem for pcc");
> 		return -ENOMEM;
> 	}
>+	
>+	/* Initialize mutex */
>+	mutex_init(&pcc->pwm_fan_lock);

As this was changed, the conditional mutex initialization is now gone.
And the cleanup code was simplified:

> +	if (pcc->pwm_fan_hwmon)
> +		hwmon_device_unregister(pcc->pwm_fan_hwmon);
> +	if (pcc->pwm_fan_cdev)
> +		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
> +	
> +	mutex_destroy(&pcc->pwm_fan_lock);
> +

Thanks for taking the time to review the code.

2.54.0

Re: [RFC PATCH v7] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Ilpo Järvinen]

On Sun, 19 Jul 2026, Alex Yeo wrote:

> I have a CF-SR4 and Linux works out of the box. Compared to
> previous models, this one seems to have the fans running at high speed
> by default when the computer starts.
> 
> When Windows 11 is booted up, the high fan speeds persist just until the
> login screen. Once the login screen shows up, the fan spins down.
> 
> I had suspected that this might be the laptop ramping down the fans when
> the OS declares that it is Windows but this does not seem to be the
> case.
> 
> After analyzing the DSDT and SSDT of the computer, I have found the
> following:
> 
> File ssdt10.dsl under \_SB.PC00.LPCB.EC0:
> 
> Name (CEFM, Zero)
> Method (SEFM, 1, Serialized)
> {
>     CEFM = Arg0
>     REFM ()
> }
> 
> Method (REFM, 0, Serialized)
> {
>     If ((\S0IX == 0x03))
>     {
>         Local0 = 0x05
>     }
>     ElseIf ((CEFM == Zero))
>     {
>         Local0 = Zero
>     }
>     Else
>     {
>         Local0 = 0x02
>     }
> 
>     \_SB.PC00.LPCB.EC0.EC88 (0xB5, 0x79, Local0, Zero)
> }
> 
> \_SB.PC00.LPCB.EC0.CEFM would seem be the current value
> for the fan profile. On startup, this is set to 0.
> 
> Based on the code SEFM seems the be the method to set the fan
> profile and REFM is executed right after.
> 
> I don't have access to information as to what the argument officially
> means but based on testing, any number above zero makes the fans spin
> down and behave like the older models where it stops or runs at low
> speed when its cool and ramps up when the processor gets hot.
> 
> The only relevant values for CEFM seem to be just 0 and any number above
> that just gets treated the same. I personally use just 0 and 1.
> 0 seems to be the high fan speed mode and 1 makes it behave like
> Windows.
> 
> Giving 0 as an argument reverts the fan back to the way it was during
> startup where the lowest fan speed is quite high and when load is
> applied, it seems to ramp up to an even higher speed which I think would
> be its 100%.
> 
> A value of 1 seems to have its max speed capped lower than 0.
> 
> For both modes, fan management is still automatic.
> 
> fan_mode only shows up in sysfs only if \_SB.PC00.LPCB.EC0.CEFM and
> \_SB.PC00.LPCB.EC0.SEFM are both present which should mean it should not
> show up on unsupported models. I have tried not hiding it and it just
> outputs a generic error when the value is read.
> 
> I also saw that variables such as eco_mode are kept in memory, however
> for fan_mode I rely on getting and setting the value via ACPI.
> 
> Signed-off-by: Alex Yeo <alexyeo362@gmail.com>
> ---
>  drivers/platform/x86/panasonic-laptop.c | 500 +++++++++++++++++++++++-
>  1 file changed, 486 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index b83113c26f88..2d67188de2a9 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
> @@ -119,6 +119,9 @@
>   *		- v0.1  start from toshiba_acpi driver written by John Belmonte
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/printk.h>
>  #include <linux/acpi.h>
>  #include <linux/backlight.h>
>  #include <linux/bits.h>
> @@ -136,6 +139,14 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <acpi/video.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/thermal.h>
> +#include <linux/dmi.h>
> +#include <linux/errno.h>
> +#include <linux/cleanup.h>
> +#include <linux/minmax.h>

Includes should be added in alphabetical order (per each subdirectory 
block).

>  
>  MODULE_AUTHOR("Hiroshi Miura <miura@da-cha.org>");
>  MODULE_AUTHOR("David Bronaugh <dbronaugh@linuxboxen.org>");
> @@ -164,6 +175,17 @@ MODULE_LICENSE("GPL");
>  
>  #define ACPI_PCC_INPUT_PHYS	"panasonic/hkey0"
>  
> +/* Define fan PWM modes */
> +#define ACPI_PCC_FAN_PWM_AUTO		0x00
> +#define ACPI_PCC_FAN_PWM_MANUAL		0x01
> +#define HWMON_PCC_FAN_PWM_AUTO		0x02
> +#define HWMON_PCC_FAN_PWM_MANUAL	0x01
> +
> +/* Define quirks for this driver */
> +struct quirk_entry {
> +	bool has_ospm_pwm_fan;
> +};
> +
>  /* LCD_TYPEs: 0 = Normal, 1 = Semi-transparent
>     ECO_MODEs: 0x03 = off, 0x83 = on
>  */
> @@ -238,19 +260,50 @@ static const struct key_entry panasonic_keymap[] = {
>  };
>  
>  struct pcc_acpi {
> -	acpi_handle		handle;
> -	unsigned long		num_sifr;
> -	int			sticky_key;
> -	int			eco_mode;
> -	int			mute;
> -	int			ac_brightness;
> -	int			dc_brightness;
> -	int			current_brightness;
> -	u32			*sinf;
> -	struct acpi_device	*device;
> -	struct input_dev	*input_dev;
> -	struct backlight_device	*backlight;
> -	struct platform_device	*platform;
> +	acpi_handle			handle;
> +	unsigned long			num_sifr;
> +	int				sticky_key;
> +	int				eco_mode;
> +	int				mute;
> +	int				ac_brightness;
> +	int				dc_brightness;
> +	int				current_brightness;
> +	u32				*sinf;
> +	struct quirk_entry		*quirks;
> +	struct acpi_device		*device;
> +	struct input_dev		*input_dev;
> +	struct backlight_device		*backlight;
> +	struct platform_device		*platform;
> +	struct thermal_cooling_device	*pwm_fan_cdev;
> +	struct device			*pwm_fan_hwmon;
> +
> +	/*
> +	 * This mutex ensures that the hwmon and thermal functions
> +	 * for fan operations do not conflict as the PWM fan is
> +	 * exposed to both.
> +	 */
> +	struct mutex			pwm_fan_lock;
> +};
> +
> +/*
> + * Declare quirks
> + */
> +
> +static struct quirk_entry quirk_cf_sr4 = {
> +	.has_ospm_pwm_fan = true,
> +};
> +
> +/* DMI matching for quirks copied from asus-nb-wmi.c */
> +static const struct dmi_system_id pcc_quirks[] = {
> +	{
> +		.ident = "Panasonic Connect Co., Ltd. CFSR4-1",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Panasonic Connect Co., Ltd."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "CFSR4-1"),
> +		},
> +		.driver_data = &quirk_cf_sr4,
> +	},
> +	{},
>  };
>  
>  /*
> @@ -415,7 +468,6 @@ static const struct backlight_ops pcc_backlight_ops = {
>  	.update_status	= bl_set_status,
>  };
>  
> -
>  /* returns ACPI_SUCCESS if methods to control optical drive are present */

Unrelated change.

>  
>  static acpi_status check_optd_present(void)
> @@ -507,6 +559,155 @@ static int set_optd_power_state(int new_state)
>  	return result;
>  }
>  
> +/* get OSPM fan mode */
> +
> +static int pcc_pwm_fan_mode_read(int *pwm_mode)
> +{
> +	unsigned long long state;
> +	acpi_status status;
> +
> +	/* BIOS default is zero which seems to be some sort of failsafe mode */
> +	status = acpi_evaluate_integer(NULL, "\\_SB.PC00.LPCB.EC0.CEFM", NULL,
> +				       &state);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("cannot get fan mode via CEFM\n");
> +		return -EIO;
> +	}
> +
> +	/* use hwmon convention for pwm_mode */
> +	if (state == ACPI_PCC_FAN_PWM_AUTO) {
> +		*pwm_mode = HWMON_PCC_FAN_PWM_AUTO;
> +	} else if (state != 0 && state > 0) {
> +		*pwm_mode = HWMON_PCC_FAN_PWM_MANUAL;
> +	} else {
> +		/* unknown */
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/* set OSPM fan mode */
> +
> +static int pcc_pwm_fan_mode_set(int pwm_mode)
> +{
> +	acpi_status status;
> +
> +	union acpi_object param[1];
> +	struct acpi_object_list input;

Don't leave empty lines in between variable declarations. Please use 
reverse-xmas tree order when you can (when no internal dependencies 
between local var don't force your hand).

> +
> +	param[0].type = ACPI_TYPE_INTEGER;
> +	param[0].integer.value = pwm_mode;
> +	input.count = 1; /* takes one arg */

Remove comment.

> +	input.pointer = param;
> +
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.SEFM", &input,
> +				      NULL);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("cannot set fan mode via SEFM\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/* read PWM fan speed */

This comments adds zero value over what the function name already tells 
us. Please don't add comments that state obvious things.

> +
> +static int pcc_pwm_fan_speed_read(int *pwm_speed)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	acpi_status status;
> +	int pwm_mode;
> +
> +
> +	/* Get fan status first */
> +	/* If fan is not in manual mode, it will return a bogus value */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("failed to get fan status\n");
> +		return status;
> +	}
> +
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO)
> +		return -ENODATA; /* Indeterminate value */
> +
> +	/* get pwm speed */
> +	status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FST",
> +				      NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("failed to get pwm speed\n");
> +		return -EIO;
> +	}
> +
> +	union acpi_object *obj __free(kfree) = buffer.pointer;
> +
> +	/* the structure should have 3 values */

That's what can be easily read from the code. There's no need to comment 
trivialities like this.

> +	if (!obj || obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2) {
> +		pr_err("Invalid _FST package structure (expected 3)\n");
> +		return -EINVAL;
> +	}
> +
> +	/* the second element should be the pwm speed as an int */
> +	if (obj->package.elements[1].type != ACPI_TYPE_INTEGER) {
> +		pr_err("_FST element at index 1 is not an integer\n");
> +		return -EINVAL;
> +	}
> +
> +	/* prevent out-of-bounds */
> +	if (obj->package.elements[1].integer.value > 100) {
> +		pr_err("_FST element at index 1 is an integer out of bounds\n");
> +		return -EINVAL;
> +	}
> +
> +	*pwm_speed = obj->package.elements[1].integer.value;
> +	return 0;
> +}
> +
> +/* set PWM fan speed */
> +
> +static int pcc_pwm_fan_speed_set(int set_pwm_speed)
> +{
> +	struct acpi_object_list input;
> +	union acpi_object param[1];
> +	acpi_status fsl_status;
> +	int pwm_mode;
> +	int status;
> +
> +	/* Get fan status. set to manual if not set */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("failed to get fan status\n");
> +		return status;
> +	}
> +
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		status = pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
> +		if (status < 0) {
> +			pr_err("set fan PWM to manual failed\n");
> +			return status;
> +		}
> +	}
> +
> +	/* check if within bounds */
> +	if (set_pwm_speed < 0 || set_pwm_speed > 100) {
> +		pr_err("fan speed level out of bounds\n");
> +		return -EIO;
> +	}
> +
> +	param[0].type = ACPI_TYPE_INTEGER;
> +	param[0].integer.value = set_pwm_speed;
> +	input.count = 1; /* takes one arg */
> +	input.pointer = param;
> +
> +	fsl_status = acpi_evaluate_object(NULL, "\\_SB.PC00.LPCB.EC0.TFN1._FSL",
> +				      &input, NULL);
> +	if (ACPI_FAILURE(fsl_status)) {
> +		pr_err("Setting of fan speed via ._FSL failed.\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
>  
>  /* sysfs user interface functions */
>  
> @@ -825,6 +1026,228 @@ static const struct attribute_group pcc_attr_group = {
>  	.is_visible	= pcc_sysfs_is_visible,
>  };
>  
> +/* hwmon */
> +
> +static const struct hwmon_channel_info *const pcc_pwm_fan_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> +	NULL
> +};
> +
> +static int pcc_pwm_fan_hwmon_speed_read(struct pcc_acpi *pcc, long *val)
> +{
> +	int pwm_speed;
> +	int status;
> +
> +	guard(mutex)(&pcc->pwm_fan_lock);
> +
> +	status = pcc_pwm_fan_speed_read(&pwm_speed);
> +	if (status < 0)
> +		return status;
> +
> +	/* protect against out-of-bounds */
> +	if (pwm_speed < 0 || pwm_speed > 100)
> +		return -EINVAL;
> +
> +	*val = (pwm_speed * 255) / 100;
> +	return 0;
> +}
> +
> +static int pcc_pwm_fan_hwmon_mode_read(struct pcc_acpi *pcc, long *val)
> +{
> +	int pwm_mode;
> +	int status;
> +
> +	guard(mutex)(&pcc->pwm_fan_lock);
> +
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("failed to get fan pwm mode\n");
> +		return status;
> +	}
> +
> +	switch (pwm_mode) {
> +	case HWMON_PCC_FAN_PWM_AUTO:
> +		*val = HWMON_PCC_FAN_PWM_AUTO;
> +		return 0;
> +	case HWMON_PCC_FAN_PWM_MANUAL:
> +		*val = HWMON_PCC_FAN_PWM_MANUAL;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int pcc_pwm_fan_hwmon_read(struct device *dev,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel, long *val)
> +{
> +	struct pcc_acpi *pcc = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		return pcc_pwm_fan_hwmon_speed_read(pcc, val);
> +	case hwmon_pwm_enable:
> +		return pcc_pwm_fan_hwmon_mode_read(pcc, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pcc_pwm_fan_hwmon_mode_set(struct pcc_acpi *pcc, long val)
> +{
> +	switch (val) {
> +	case HWMON_PCC_FAN_PWM_AUTO:
> +		guard(mutex)(&pcc->pwm_fan_lock);

I'm not sure if clang is happy with how scoping is here so these should 
have {} around each case.

> +		return pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_AUTO);
> +	case HWMON_PCC_FAN_PWM_MANUAL:
> +		guard(mutex)(&pcc->pwm_fan_lock);
> +		return pcc_pwm_fan_mode_set(ACPI_PCC_FAN_PWM_MANUAL);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int pcc_pwm_fan_hwmon_speed_set(struct pcc_acpi *pcc, long val)
> +{
> +	int pwm_speed_vendor_val;
> +	int pwm_mode;
> +	int status;
> +
> +	val = clamp_val(val, 0, 255);
> +	pwm_speed_vendor_val = (val * 100) / 255;
> +
> +	guard(mutex)(&pcc->pwm_fan_lock);
> +
> +	/* check first if manual control is enabled */
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("failed to get fan pwm mode\n");
> +		return status;
> +	}
> +
> +	/* do not allow settings speeds if not manual mode */
> +	if (pwm_mode != HWMON_PCC_FAN_PWM_MANUAL)
> +		return -EOPNOTSUPP;
> +
> +	status = pcc_pwm_fan_speed_set(pwm_speed_vendor_val);
> +	if (status < 0) {
> +		pr_err("failed to set fan pwm\n");
> +		return status;
> +	}
> +	return 0;
> +}
> +
> +static int pcc_pwm_fan_hwmon_write(struct device *dev,
> +				   enum hwmon_sensor_types type, u32 attr,
> +				   int channel, long val)
> +{
> +	struct pcc_acpi *pcc;
> +
> +	pcc = dev_get_drvdata(dev);

And this is where I started to check whether you've ignored my earlier 
feedback. Turns out you sent a new version without addressing the 
feedback I've given you (and IIRC, it's not the first time this has 
happened).

The next time this happens, I'll move this submission and its subsequent 
versions into a very low-priority bin to avoid wasting my time on it. So 
please triple check twice you've addressed every single comment I (or 
other potential reviewer) has given you on _any earlier version_ of the 
patch before even considering sending the next version. There's no hurry 
here, I won't be accepting half-baked patches so you'd just be wasting 
everyone's time by sending it over and over again with things unaddressed.


--
 i.

> +
> +	if (type != hwmon_pwm)
> +		return -EOPNOTSUPP;
> +
> +	switch (attr) {
> +	case hwmon_pwm_enable:
> +		return pcc_pwm_fan_hwmon_mode_set(pcc, val);
> +	case hwmon_pwm_input:
> +		return pcc_pwm_fan_hwmon_speed_set(pcc, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t pcc_pwm_fan_hwmon_is_visible(const void *data,
> +					    enum hwmon_sensor_types type,
> +					    u32 attr, int channel)
> +{
> +	if (type != hwmon_pwm)
> +		return 0;
> +
> +	return 0644;
> +}
> +
> +static const struct hwmon_ops pcc_pwm_fan_hwmon_ops = {
> +	.is_visible = pcc_pwm_fan_hwmon_is_visible,
> +	.read = pcc_pwm_fan_hwmon_read,
> +	.write = pcc_pwm_fan_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info pcc_pwm_fan_hwmon_chip_info = {
> +	.ops = &pcc_pwm_fan_hwmon_ops,
> +	.info = pcc_pwm_fan_hwmon_info,
> +};
> +
> +/* thermal interface */
> +static int pcc_pwm_fan_thermal_max_state(struct thermal_cooling_device *cdev,
> +					 unsigned long *state)
> +{
> +	*state = 100; /* range of 0-100 as per UEFI spec */
> +	return 0;
> +}
> +static int
> +pcc_pwm_fan_thermal_get_current_pwm(struct thermal_cooling_device *cdev,
> +				    unsigned long *state)
> +{
> +	struct pcc_acpi *pcc = cdev->devdata;
> +	int current_pwm;
> +	int pwm_mode;
> +	int status;
> +
> +	guard(mutex)(&pcc->pwm_fan_lock);
> +
> +	status = pcc_pwm_fan_mode_read(&pwm_mode);
> +	if (status < 0) {
> +		pr_err("%s: failed to get fan pwm mode\n", __func__);
> +		return status;
> +	}
> +	if (pwm_mode == HWMON_PCC_FAN_PWM_AUTO) {
> +		*state = 100; /* Return failsafe value on EC mode */
> +		return 0;
> +	}
> +
> +	status = pcc_pwm_fan_speed_read(&current_pwm);
> +	if (status < 0)
> +		return status; /* pass error code */
> +
> +	if (current_pwm > 100)
> +		current_pwm = 100;
> +	if (current_pwm < 0)
> +		return -EINVAL;
> +
> +	*state = current_pwm;
> +	return 0;
> +}
> +static int pcc_pwm_fan_thermal_set_fan_pwm(struct thermal_cooling_device *cdev,
> +					   unsigned long state)
> +{
> +	struct pcc_acpi *pcc = cdev->devdata;
> +	int status;
> +	int set_pwm;
> +
> +	guard(mutex)(&pcc->pwm_fan_lock);
> +
> +	if (state > 100)
> +		return -EINVAL;
> +
> +	set_pwm = state;
> +	status = pcc_pwm_fan_speed_set(set_pwm);
> +	if (status < 0) {
> +		pr_err("%s: failed to set fan speed pwm\n", __func__);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +static const struct thermal_cooling_device_ops pcc_pwm_fan_cdev_ops = {
> +	.get_max_state = pcc_pwm_fan_thermal_max_state,
> +	.get_cur_state = pcc_pwm_fan_thermal_get_current_pwm,
> +	.set_cur_state = pcc_pwm_fan_thermal_set_fan_pwm,
> +};
>  
>  /* hotkey input device driver */
>  
> @@ -981,6 +1404,7 @@ static int acpi_pcc_hotkey_resume(struct device *dev)
>  
>  static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  {
> +	const struct dmi_system_id *dmi_id;
>  	struct backlight_properties props;
>  	struct acpi_device *device;
>  	struct pcc_acpi *pcc;
> @@ -1013,6 +1437,9 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	/* Initialize mutex */
> +	mutex_init(&pcc->pwm_fan_lock);
> +
>  	pcc->sinf = kcalloc(num_sifr + 1, sizeof(u32), GFP_KERNEL);
>  	if (!pcc->sinf) {
>  		result = -ENOMEM;
> @@ -1026,6 +1453,15 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  	strscpy(acpi_device_name(device), ACPI_PCC_DEVICE_NAME);
>  	strscpy(acpi_device_class(device), ACPI_PCC_CLASS);
>  
> +	/*
> +	 * Perform quirk detection
> +	 */
> +	dmi_id = dmi_first_match(pcc_quirks);
> +	if (dmi_id) {
> +		pcc->quirks = dmi_id->driver_data;
> +		pr_debug("Quirk detect: Enabled quirks for %s\n", dmi_id->ident);
> +	}
> +
>  	result = acpi_pcc_init_input(pcc);
>  	if (result) {
>  		pr_err("Error installing keyinput handler\n");
> @@ -1097,6 +1533,35 @@ static int acpi_pcc_hotkey_probe(struct platform_device *pdev)
>  	}
>  
>  	i8042_install_filter(panasonic_i8042_filter, NULL);
> +
> +	/* skip if no quirks */
> +	if (!pcc->quirks)
> +		goto out_no_quirks;
> +
> +	/* has_ospm_pwm_fan - add hwmon and thermal if present */
> +	if (pcc->quirks->has_ospm_pwm_fan) {
> +		pr_debug("has_ospm_pwm_fan quirk: adding hwmon for %s\n", dmi_id->ident);
> +
> +		pcc->pwm_fan_hwmon = hwmon_device_register_with_info(
> +			&pdev->dev, "panasonic_pwm_fan", pcc,
> +			&pcc_pwm_fan_hwmon_chip_info, NULL);
> +		if (IS_ERR(pcc->pwm_fan_hwmon)) {
> +			pr_err("has_ospm_pwm_fan: Failed to register hwmon device\n");
> +			/* not a critical error. just skip if error */
> +			pcc->pwm_fan_hwmon = NULL;
> +		}
> +
> +		/* proceed with thermal */
> +		pr_debug("has_ospm_pwm_fan quirk: adding thermal for %s\n", dmi_id->ident);
> +		pcc->pwm_fan_cdev = thermal_cooling_device_register(
> +			"Panasonic_PWM_Fan", pcc, &pcc_pwm_fan_cdev_ops);
> +		if (IS_ERR(pcc->pwm_fan_cdev)) {
> +			pr_warn("has_ospm_pwm_fan: Failed to register thermal cdev\n");
> +			pcc->pwm_fan_cdev = NULL;
> +		}
> +	}
> +
> +out_no_quirks:
>  	return 0;
>  
>  out_platform:
> @@ -1124,6 +1589,13 @@ static void acpi_pcc_hotkey_remove(struct platform_device *pdev)
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct pcc_acpi *pcc = acpi_driver_data(device);
>  
> +	if (pcc->pwm_fan_hwmon)
> +		hwmon_device_unregister(pcc->pwm_fan_hwmon);
> +	if (pcc->pwm_fan_cdev)
> +		thermal_cooling_device_unregister(pcc->pwm_fan_cdev);
> +
> +	mutex_destroy(&pcc->pwm_fan_lock);
> +
>  	i8042_remove_filter(panasonic_i8042_filter);
>  
>  	if (pcc->platform) {
> 

Re: [RFC PATCH v7] platform/x86: panasonic-laptop: add fan speed mode for newer models

[Guenter Roeck]

On 7/20/26 06:46, Ilpo Järvinen wrote:
> On Sun, 19 Jul 2026, Alex Yeo wrote:

...

>> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
>> index b83113c26f88..2d67188de2a9 100644
>> --- a/drivers/platform/x86/panasonic-laptop.c
>> +++ b/drivers/platform/x86/panasonic-laptop.c
>> @@ -119,6 +119,9 @@
>>    *		- v0.1  start from toshiba_acpi driver written by John Belmonte
>>    */
>>   
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/printk.h>
>>   #include <linux/acpi.h>
>>   #include <linux/backlight.h>
>>   #include <linux/bits.h>
>> @@ -136,6 +139,14 @@
>>   #include <linux/types.h>
>>   #include <linux/uaccess.h>
>>   #include <acpi/video.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>

FWIW, I don't see why this is needed.

...

>>   
>>   struct pcc_acpi {
...
>> +	/*
>> +	 * This mutex ensures that the hwmon and thermal functions
>> +	 * for fan operations do not conflict as the PWM fan is
>> +	 * exposed to both.
>> +	 */
>> +	struct mutex			pwm_fan_lock;

Maybe ask why this additional lock is used instead of relying on
(for hwmon attributes) and using (for thermal) the hwmon subsystem
lock.

Guenter