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

[Alex Yeo]

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