RE: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working mode selection in amd_pstate_param()

[Yuan, Perry]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Karny, Wyes <Wyes.Karny@xxxxxxx>
> Sent: Friday, December 23, 2022 5:45 PM
> To: Yuan, Perry <Perry.Yuan@xxxxxxx>; rafael.j.wysocki@xxxxxxxxx;
> Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Huang, Ray
> <Ray.Huang@xxxxxxx>; viresh.kumar@xxxxxxxxxx
> Cc: Sharma, Deepak <Deepak.Sharma@xxxxxxx>; Fontenot, Nathan
> <Nathan.Fontenot@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Huang, Shimmer
> <Shimmer.Huang@xxxxxxx>; Du, Xiaojian <Xiaojian.Du@xxxxxxx>; Meng,
> Li (Jassmine) <Li.Meng@xxxxxxx>; linux-pm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8 05/13] cpufreq: amd-pstate: optimize driver working
> mode selection in amd_pstate_param()
> 
> 
> 
> On 12/19/2022 12:10 PM, Perry Yuan wrote:
> --------------------->8-----------------------------
> >
> > +/**
> > + * enum amd_pstate_mode - driver working mode of amd pstate  */
> > +
> > +enum amd_pstate_mode {
> > +	/** @AMD_PSTATE_DISABLE: Driver mode is disabled */
> > +	AMD_PSTATE_DISABLE = 0,
> > +
> > +	/** @AMD_PSTATE_PASSIVE: Drier mode is passive mode */
> > +	AMD_PSTATE_PASSIVE = 1,
> > +
> > +	/** @AMD_PSTATE_ACTIVE: Driver mode is active mode */
> > +	AMD_PSTATE_ACTIVE = 2,
> > +
> > +	/** @AMD_PSTATE_GUIDE: Driver mode is guided mode */
> > +	AMD_PSTATE_GUIDE = 3,
> > +
> > +	/** @AMD_PSTATE_MAX */
> > +	AMD_PSTATE_MAX = 4,
> > +};
> 
> IMO the above enum is self explanatory we don't need to annotate.
> 
> what about below?
> 
> /**
>  * enum amd_pstate_mode - driver working mode
>  * All supported modes are explained in kernel-parameters.txt  */ enum
> amd_pstate_mode {
> 	AMD_PSTATE_DISABLE = 0,
> 	AMD_PSTATE_PASSIVE,
> 	AMD_PSTATE_ACTIVE,
> 	AMD_PSTATE_MAX,
> };

Sure , changed it in v9 like you suggested.

> 
> Plz remove GUIDED mode here because it allows user to pass
> "amd_pstate=guided"
> in kernel cmdline. Therefore it breaks the driver flow without guided patches.
> I can update the enum in my guided patch.
> 

Removed 

> > +
> > +static const char * const amd_pstate_mode_string[] = {
> > +	[AMD_PSTATE_DISABLE]     = "disable",
> > +	[AMD_PSTATE_PASSIVE]     = "passive",
> > +	[AMD_PSTATE_ACTIVE]      = "active",
> > +	[AMD_PSTATE_GUIDE]      = "guide",
> > +	NULL,
> > +};
> > +
> >  #endif /* _LINUX_AMD_PSTATE_H */
> 
> --
> Thanks & Regards,
> Wyes