Received-SPF: pass (google.com: domain of linux-kernel+bounces-192735-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: i1719461a:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-491-g033e30d24-fm-20240520.001-g033e30d2
Precedence: bulk
MIME-Version: 1.0
Message-Id: <a0322878-eb27-45e4-9545-136f5ade6a0e@app.fastmail.com>
In-Reply-To: <c971cfae-e7d5-fbde-f77b-1a936c6e1ee7@linux.intel.com>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
 <20240517224323.10045-1-lsanche@lyndeno.ca>
 <20240517224323.10045-4-lsanche@lyndeno.ca>
 <c971cfae-e7d5-fbde-f77b-1a936c6e1ee7@linux.intel.com>
Date: Tue, 28 May 2024 10:16:17 -0600
From: "Lyndon Sanche" <lsanche@lyndeno.ca>
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: "Mario Limonciello" <mario.limonciello@amd.com>,
 =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>, "Armin Wolf" <W_Armin@gmx.de>,
 srinivas.pandruvada@linux.intel.com, "kernel test robot" <lkp@intel.com>,
 "Hans de Goede" <hdegoede@redhat.com>, "Yijun Shen" <Yijun.Shen@dell.com>,
 "Matthew Garrett" <mjg59@srcf.ucam.org>,
 "Vegard Nossum" <vegard.nossum@oracle.com>,
 "Jonathan Corbet" <corbet@lwn.net>, "Heiner Kallweit" <hkallweit1@gmail.com>,
 LKML <linux-kernel@vger.kernel.org>, platform-driver-x86@vger.kernel.org,
 Dell.Client.Kernel@dell.com
Subject: Re: [PATCH v7 3/3] platform/x86: dell-pc: Implement platform_profile
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Mon, May 27, 2024, at 3:39 AM, Ilpo J=C3=A4rvinen wrote:
> On Fri, 17 May 2024, Lyndon Sanche wrote:
>
>> +#include <linux/platform_profile.h>
>> +#include <linux/slab.h>
>> +#include "dell-smbios.h"
>
> Add empty line between <> and "" includes.

Agreed.

>> +enum thermal_mode_bits {
>> +	DELL_BALANCED =3D BIT(0),
>> +	DELL_COOL_BOTTOM =3D BIT(1),
>> +	DELL_QUIET =3D BIT(2),
>> +	DELL_PERFORMANCE =3D BIT(3),
>
> A few nits still to address.
>
> Can you please align these so that the values align (IIRC, I asked thi=
s=20
> earlier but perhaps my request was too unclear):
>
> 	DELL_XX			=3D BIT(X),
> 	DELL_YYYYYYYYY		=3D BIT(Y),
>

I must have missed this, I will get this corrected.

>> +static int thermal_get_supported_modes(int *supported_bits)
>> +{
>> +	struct calling_interface_buffer buffer;
>> +	int ret;
>> +
>> +	dell_fill_request(&buffer, 0x0, 0, 0, 0);
>> +	ret =3D dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAG=
EMENT);
>> +	if (ret) {
>> +		/* Thermal function not supported */
>> +		if (ret =3D=3D -ENXIO) {
>> +			*supported_bits =3D 0;
>> +			return 0;
>> +		} else {
>
> Drop else because the previous block ends into return.
>

Agreed.

>> +			return ret;
>> +		}
>> +	}
>
> Remove the outer if (ret) block and put the inner ones directly on the=20
> main level as two if () conditions.
>

Agreed.

>> +	dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD, acc_=
mode) | state, 0, 0);
>> +	ret =3D dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAG=
EMENT);
>> +	return ret;
>
> Return directly on the previous line.
>

Agreed.

>> +	int ret;
>> +	int supported_modes;
>> +
>> +	/* If thermal commands not supported, exit without error */
>
> Fix grammar, you're perhaps missing "are".
>

Agreed.

>> +	if (!dell_smbios_class_is_supported(CLASS_INFO))
>> +		return 0;
>> +
>> +	/* If thermal modes not supported, exit without error */
>
> Ditto.
>
> --=20
>  i.

Thank you for your feedback. I will include this in the next patch serie=
s.

Thanks,

Lyndon