Re: [PATCH v3] platform/x86: dell-laptop: Implement platform_profile

[Lyndon Sanche]

On Mon, Apr 29 2024 at 12:45:19 PM -05:00:00, Mario Limonciello 
<mario.limonciello@xxxxxxx> 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
>