Received-SPF: pass (google.com: domain of linux-kernel+bounces-163067-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Feedback-ID: i1719461a:Fastmail
Date: Mon, 29 Apr 2024 15:21:59 -0600
From: Lyndon Sanche <lsanche@lyndeno.ca>
Subject: Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile
To: Mario Limonciello <mario.limonciello@amd.com>
Cc: pali@kernel.org, W_Armin@gmx.de, srinivas.pandruvada@linux.intel.com,
	ilpo.jarvinen@linux.intel.com, Matthew Garrett <mjg59@srcf.ucam.org>,
	Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, Dell.Client.Kernel@dell.com
Message-Id: <NC3QCS.LEAYSBJ8Y8GC1@lyndeno.ca>
In-Reply-To: <5cbb8981-5e24-4dce-a78f-1cabc29f08e3@amd.com>
References: <20240425172758.67831-1-lsanche@lyndeno.ca>
	<20240429164844.7544-2-lsanche@lyndeno.ca>
	<5cbb8981-5e24-4dce-a78f-1cabc29f08e3@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed


On Mon, Apr 29 2024 at 12:45:19 PM -05:00:00, Mario Limonciello 
<mario.limonciello@amd.com> wrote:
> On 4/29/2024 11:48, Lyndon Sanche wrote:
>>   #include <linux/i8042.h>
>>   #include <linux/debugfs.h>
>>   #include <linux/seq_file.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/bitfield.h>
> 
> These should be inserted in alphabetical order.

Agree

>> +
>> +	// Clean up but do not fail
> 
> Switch comment style to /* */

Agree

> 
>> +	if (platform_profile_register(thermal_handler))
>> +		kfree(thermal_handler);
> 
> Don't you also want to return an error in this case?  Because this 
> means that the platform supports thermal modes but it failed to setup 
> due to other issues.
> 
> It's different than the case of no supported modes in which case 
> returning 0 makes sense.
> 
> Maybe like this:
> 
> 
> ret = platform_profile_register(thermal_handler);
> if (ret)
> 	kfree(thermal_handler);
> 
> return ret;

Good idea, I will propogate this error. Failure of module will then 
occur on allocation or platform_profile error.

> 
> 
>> 
>>   		goto fail_rfkill;
>>   	}
>>   +	// Do not fail module if thermal modes not supported, just skip
> 
> Switch comment style to /* */

Agree.

Thank you for this feedback.

Lyndon
>