Re: [PATCH] Update whitelisted ThinkPad models with dual fan support in thinkpad_acpi

From: Hans de Goede <hdegoede@redhat.com>
To: David Dreschner <david@dreschner.net>,
	ibm-acpi-devel@lists.sourceforge.net,
	Mark Pearson <markpearson@lenovo.com>
Cc: platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] Update whitelisted ThinkPad models with dual fan support in thinkpad_acpi
Date: Tue, 11 Jan 2022 12:09:44 +0100	[thread overview]
Message-ID: <aceddce7-6c43-967c-fadd-fa307068e916@redhat.com> (raw)
In-Reply-To: <ec04aa1e-1ac3-edbc-ac08-eec15ec5c952@dreschner.net>

Hi David,

On 1/4/22 04:41, David Dreschner wrote:
> Hey guys,
> 
> the attached patch updates the list of whitelisted ThinkPad models with dual fan support.
> 
> The changes were tested on my ThinkPad T15g Gen 2. According to Lenovo, the BIOS version is the same for the P15 Gen 2 and the P17 Gen 2 ( https://pcsupport.lenovo.com/us/en/downloads/ds551321-bios-update-utility-bootable-cd-for-windows-10-64-bit-thinkpad-p15-gen-2-p17-gen-2-t15g-gen-2 ).
> 
> I also added the P15v Gen 2 and T15p Gen 2 to the whitelist based on the BIOS version listed on the Lenovo homepage ( https://pcsupport.lenovo.com/us/en/downloads/ds551356-bios-update-utility-bootable-cd-for-windows-10-64-bit-thinkpad-p15v-gen-2-t15p-gen-2 ). The first generation had two fans and where covered by the whitelist entry for the P15 Gen 2. As the second generation has two fans, too, I made that change for completeness.
> 
> To apply the changes before it's merged in the mainline linux kernel, I made a little dkms patch: https://github.com/dreschner/thinkpad_acpi-dual-fan-patch

Thank you for your patch submission.

If I understand things correctly then you've only tested the addition of the:

TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL),  /* P15 / P17 / T15g (2nd gen) */

quirk, correct? In that case we really only want to add that quirk, we don't
want to go and add untested quirks. But perhaps Mark from Lenovo can confirm
that this quirk:

TPACPI_Q_LNV3('N', '3', '8', TPACPI_FAN_2CTL),  /* P15v / T15p (2nd gen) */

also is correct and that those models really have 2 fans, Mark ?

Mark, more in general can you perhaps talk to the firmware team and ask
if there is a better way to detect if there are 2 fans in a thinkpad then
maintaining a quirk table for this ?

Besides the issue of the untested quirk, unfortunately your patch is not
submitted in the right format, so I cannot accept it as is, esp. the
missing of a Signed-off-by is a blocker.

Kernel patches should be in git format-patch output format and
your patch is missing a Signed-off-by line in the commit-message. I can only
accept patches with a Signed-off-by line in the commit-message like this:

Signed-off-by: Your Real Name <email@your.domain>

By adding this line you indicate that you are the author of the code and
are submitting it under the existing license for the file which you are
modifying (typically GPL-2.0) or that you have permission from the author
to submit it under this license. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Given that this patch is trivial and mostly data, not code, I can turn
it into a proper patch with myself as author, crediting you like this:

Reported-and-tested-by: David Dreschner <david@dreschner.net>

And then merge it with me as the author, or you can resubmit
it in the proper format if you prefer.

Please let me know how you want to proceed with this.

Regards,

Hans