Received-SPF: pass (google.com: domain of linux-kernel+bounces-164856-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Feedback-ID: i1719461a:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-416-g2c1796742e-fm-20240424.001-g2c179674
Precedence: bulk
MIME-Version: 1.0
Message-Id: <32d6fa42-5a8f-45a2-b07f-c16f51533e04@app.fastmail.com>
In-Reply-To: <20240501013623.ia3debwadlu2qzff@pali>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
 <20240501011507.6942-2-lsanche@lyndeno.ca>
 <20240501013623.ia3debwadlu2qzff@pali>
Date: Tue, 30 Apr 2024 19:42:26 -0600
From: "Lyndon Sanche" <lsanche@lyndeno.ca>
To: =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Cc: "Mario Limonciello" <mario.limonciello@amd.com>, W_Armin@gmx.de,
 srinivas.pandruvada@linux.intel.com,
 =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
 lkp@intel.com, "Matthew Garrett" <mjg59@srcf.ucam.org>,
 "Hans de Goede" <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org,
 LKML <linux-kernel@vger.kernel.org>, Dell.Client.Kernel@dell.com
Subject: Re: [PATCH v4] platform/x86: dell-laptop: Implement platform_profile
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 30, 2024, at 7:36 PM, Pali Roh=C3=A1r wrote:
> On Tuesday 30 April 2024 19:14:52 Lyndon Sanche wrote:
>> + *          Bit 2 AAC (Quiet)
>> + *          Bit 3 AAC (Performance)
>> + *  cbRes3, byte 3  Current Fan Failure Mode
>> + *     Bit 0 Minimal Fan Failure (at least one fan has failed, one f=
an working)
>> + *     Bit 1 Catastrophic Fan Failure (all fans have failed)
>> + *  cbArg1 0x1   (Set Thermal Information), both desired thermal mod=
e and
>
> Broken indentation. cbArg1 should have only one space after "*" and be
> at the same level as the previous cbArg1 description.
>
> And I would suggest to add a newline before cbArg1 as it start
> description of the next command.
>

I will fix this.

>> + *               desired AAC mode shall be applied
>> + *  cbArg2, byte 0  Desired Thermal Mode to set
>> + *                  (only one bit may be set for this parameter)
>> + *     Bit 0 Balanced
>> + *     Bit 1 Cool Bottom
>> + *     Bit 2 Quiet
>> + *     Bit 3 Performance
>> + *  cbArg2, byte 1  Desired Active Acoustic Controller (AAC) Mode to=
 set
>> + *     If AAC Configuration Type is Global,
>> + *         0  AAC mode disabled
>> + *         1  AAC mode enabled
>> + *
>
> And here I would suggest to remove empty line as the comment below
> belongs to the AAC description above the empty line.
>

Agreed.

>> =20
>>  /* dell-rbtn.c driver export functions which will not work correctly=
 (and could
>> diff --git a/drivers/platform/x86/dell/dell-smbios-base.c b/drivers/p=
latform/x86/dell/dell-smbios-base.c
>> index e61bfaf8b5c4..40aa4469b38b 100644
>> --- a/drivers/platform/x86/dell/dell-smbios-base.c
>> +++ b/drivers/platform/x86/dell/dell-smbios-base.c
>> @@ -9,6 +9,7 @@
>>   *  Based on documentation in the libsmbios package:
>>   *  Copyright (C) 2005-2014 Dell Inc.
>>   */
>> +#include "linux/wmi.h"
>
> Is this include file really used? Because only SELECT_THERMAL_MANAGEME=
NT
> was added and it is in the dell-smbios.h. And others constants like
> SELECT_KBD_BACKLIGHT did not needed it.
>

No it is not. It seems my IDE automatically inserted this. I glossed ove=
r it when committing it seems.

Thank you for your quick feedback.

Lyndon