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

[Alex Yeo <alexyeo362@gmail.com>]

On 2026/07/20 10:41 PM, Guenter Roeck wrote:
> 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. 
> 

If you mean linux/hwmon-sysfs.h, I agree.
I will remove that in v8.

For the other includes:

 >>> +#include <linux/sysfs.h>
this was previously missing

 >>> +#include <linux/hwmon.h>
used this for registering the fan as a PWM fan on hwmon.

> 
>>>   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.
> 

Both the hwmon and thermal devices share the same control path
(pcc_pwm_fan_speed_read and pcc_pwm_fan_speed_set). To prevent them from
executing concurrently and interleaving, I added pwm_fan_lock to 
serialize access.

Reading and writing fan speed requires a multi-step ACPI transaction 
(e.g., checking/setting manual mode first before accessing or setting 
fan speed). This is because I rely on querying ACPI for the current 
state instead of keeping state cached in the driver. Because of this, 
the mutex is placed in the outer hwmon and thermal callbacks rather than 
inside the low-level ACPI helper functions.

If there's a more appropriate way to synchronize this across both 
interfaces (e.g. use hwmon_lock in place of a driver level mutex), I'd 
be interested to hear it.

Thanks