Messages in this thread | | | Date | Mon, 29 Apr 2024 15:21:59 -0600 | From | Lyndon Sanche <> | Subject | Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile |
| |
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 >
| |