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

On 2026/07/20 9:46 PM, Ilpo Järvinen wrote:
>>    *		- 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).
> 

Will sort the headers alphabetically within their respective blocks.

>> +	{},
>>   };
>>   
>>   /*
>> @@ -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.
> 

Sorry about that. I have identified the issue to be my text editor 
config. I have fixed the issue on my end and I'll make sure to read the 
git diff line by line for the next submission.

>> +/* 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).
> 

I will go through and apply this to the entire patch.

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

Will do

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

Will do

>> +	/* 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.
> 

Will do

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

Will add {} to each case

>> +
>> +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 sincerely apologize about this. I have made the mistake of applying 
comments where it was pointed out instead of recognizing it as a pattern 
to be applied to the entire patch in addition to repeating mistakes 
pointed out in the previous replies.

I will go back to the very first version, cross-check every comment and 
check it against the entire patch before sending a v8.

Thank you